Bug 90839

Summary: [10.5.5/10.6 regression, bisected] PBO glDrawPixels no longer using blit fastpath
Product: Mesa Reporter: Alexander Monakov <amonakov>
Component: Drivers/DRI/i965Assignee: Ian Romanick <idr>
Status: RESOLVED FIXED QA Contact: Intel 3D Bugs Mailing List <intel-3d-bugs>
Severity: normal    
Priority: medium CC: anakin.cs, cravchik, jason, mavoga
Version: unspecified   
Hardware: Other   
OS: All   
Whiteboard:
i915 platform: i915 features:

Description Alexander Monakov 2015-06-04 08:40:00 UTC
Starting from commit http://cgit.freedesktop.org/mesa/mesa/commit/?id=536003c11e4cb1172c540932ce3cce06f03bf44e (i965: Add XRGB8888 format to intel_screen_make_configs), PBO-based glDrawPixels may be unable to use the blitter "fast path" (apparently if the application happens to get an XRGB window fbconfig).  It is rejected in do_blit_drawpixels, because _mesa_format_matches_format_and_type returns false for BGRX formats.  INTEL_DEBUG=perf,pix says:

do_blit_drawpixels
do_blit_drawpixels: bad format for blit
intelDrawPixels: fallback to generic code in PBO case

A repro is available at https://bugs.freedesktop.org/attachment.cgi?id=97258 (run as "./a.out 1000").

A possible workaround on the glx client side is to explicitely request "GLX_ALPHA_BITS, 1" in the fbconfig, but is that really expected now?
Comment 1 Alexander Monakov 2015-06-04 08:40:45 UTC
*** Bug 90791 has been marked as a duplicate of this bug. ***
Comment 2 Matt Turner 2015-06-04 16:07:37 UTC
This sounds like something Jason would know about.
Comment 3 Jason Ekstrand 2015-06-04 17:50:54 UTC
Yeah, this is gnarly.  Unfortunately, I'm not able to reproduce at the moment probably due to getting different visuals.  However, I do see what the problem is and I think we can probably start just using the meta code and that might fix it.
Comment 4 Alexander Monakov 2015-06-04 21:05:41 UTC
I can't reproduce it either; actually, I don't see RGBX configs in glxinfo output, should they manifest there?

Anyhow, can the check in do_blit_drawpixels be relaxed to allow RGBX<->RGBA blits?   intel_miptree_blit is able to handle it per this comment: http://cgit.freedesktop.org/mesa/mesa/tree/src/mesa/drivers/dri/i965/intel_blit.c?id=536003c11e4cb1172c540932ce3cce06f03bf44e#n170

About using Meta for Drawpixels, it needs work to avoid regressing performance, please see discussion in comments 2, 3, 4 at https://bugs.freedesktop.org/show_bug.cgi?id=77412
Comment 5 Chris Wilson 2015-06-08 17:01:22 UTC
I've pushed my drive-by fix to enable the blit path for glDrawPixels onto a RGBX framebuffer, 

commit 922c0c9fd526ce19b87bc74a3159dec7705c1de1
Author: Chris Wilson <chris@chris-wilson.co.uk>
Date:   Fri Jun 5 14:45:18 2015 +0100

    i965: Export format comparison for blitting between miptrees

hopefully this fixes the immediate regression of falling all the way back to pulling and pushing pixels. Presumably this is only a stop gap until meta replaces this intermediate fallback?
Comment 6 Jason Ekstrand 2015-06-08 17:45:17 UTC
(In reply to Chris Wilson from comment #5)
> I've pushed my drive-by fix to enable the blit path for glDrawPixels onto a
> RGBX framebuffer, 
> 
> commit 922c0c9fd526ce19b87bc74a3159dec7705c1de1
> Author: Chris Wilson <chris@chris-wilson.co.uk>
> Date:   Fri Jun 5 14:45:18 2015 +0100
> 
>     i965: Export format comparison for blitting between miptrees
> 
> hopefully this fixes the immediate regression of falling all the way back to
> pulling and pushing pixels. Presumably this is only a stop gap until meta
> replaces this intermediate fallback?

Thanks for getting that fixed quicly!  Yes, using meta was my plan and it really shouldn't take much code.  I just need to find time to get to it.
Comment 7 AnAkkk 2015-06-08 23:11:49 UTC
I've switch to mesa-git and this doesn't fix the issue :/

Still the same:
do_blit_drawpixels
do_blit_drawpixels: bad format for blit
intelDrawPixels: fallback to generic code in PBO case
Comment 8 Alexander Monakov 2015-06-09 05:54:45 UTC
Are you using lib32-mesa-git for 32-bit libraries?
Comment 9 AnAkkk 2015-06-09 06:59:01 UTC
Yes, both mesa-git and lib32-mesa-git.
Comment 10 Alexander Monakov 2015-06-09 08:02:51 UTC
Do you get the same INTEL_DEBUG=perf,pix diagnostics on the small repro referenced in the opening comment?

Can you apply the following patch and collect INTEL_DEBUG=perf,pix output again?  At least we'll know which exact formats are rejected.

From 981de62fb518cae8a2278123597131f4d22e930d Mon Sep 17 00:00:00 2001
From: Alexander Monakov <amonakov@ispras.ru>
Date: Tue, 9 Jun 2015 10:58:29 +0300
Subject: [PATCH] i965: spell out incompatible formats in do_blit_drawpixels

---
 src/mesa/drivers/dri/i965/intel_pixel_draw.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/src/mesa/drivers/dri/i965/intel_pixel_draw.c b/src/mesa/drivers/dri/i965/intel_pixel_draw.c
index d68cbb6..d39c698 100644
--- a/src/mesa/drivers/dri/i965/intel_pixel_draw.c
+++ b/src/mesa/drivers/dri/i965/intel_pixel_draw.c
@@ -85,7 +85,9 @@ do_blit_drawpixels(struct gl_context * ctx,
    dst_format = _mesa_get_srgb_format_linear(dst_format);
 
    if (!intel_miptree_blit_compatible_formats(src_format, dst_format)) {
-      DBG("%s: bad format for blit\n", __func__);
+      DBG("%s: bad format for blit: %s to %s\n", __func__,
+	  _mesa_get_format_name(src_format),
+	  _mesa_get_format_name(dst_format));
       return false;
    }
Comment 11 AnAkkk 2015-06-09 10:07:31 UTC
Yes, I get the same on the small app.

With the debug patch the game just crash when I set INTEL_DEBUG=perf,pix.
Comment 12 Alexander Monakov 2015-06-09 11:25:05 UTC
Does it also crash the small repro with the patch?  If not, can you show INTEL_DEBUG=perf,pix output on the repro?  If yes, can you show crash backtraces?
Comment 13 AnAkkk 2015-06-09 13:37:37 UTC
For the small repro, it crashes without primusrun, and with primusrun this is the output:

...
do_blit_drawpixels: bad format for blit: (null) to MESA_FORMAT_B8G8R8X8_UNORM
intelDrawPixels: fallback to generic code in PBO case
do_blit_drawpixels
...
Comment 14 Alexander Monakov 2015-06-09 18:00:54 UTC
Sigh.  Please apply this:

From dae0d4b5716b9a5d9227539bb6312f1f389bfcd3 Mon Sep 17 00:00:00 2001
From: Alexander Monakov <amonakov@ispras.ru>
Date: Tue, 9 Jun 2015 20:58:22 +0300
Subject: [PATCH] i965: do_blit_drawpixels: decode array formats

---
 src/mesa/drivers/dri/i965/intel_pixel_draw.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/src/mesa/drivers/dri/i965/intel_pixel_draw.c b/src/mesa/drivers/dri/i965/intel_pixel_draw.c
index d68cbb6..189a592 100644
--- a/src/mesa/drivers/dri/i965/intel_pixel_draw.c
+++ b/src/mesa/drivers/dri/i965/intel_pixel_draw.c
@@ -78,6 +78,8 @@ do_blit_drawpixels(struct gl_context * ctx,
    struct intel_renderbuffer *irb = intel_renderbuffer(rb);
 
    mesa_format src_format = _mesa_format_from_format_and_type(format, type);
+   if (_mesa_format_is_mesa_array_format(src_format))
+      src_format = _mesa_format_from_array_format(src_format);
    mesa_format dst_format = irb->mt->format;
 
    /* We can safely discard sRGB encode/decode for the DrawPixels interface */
Comment 15 Ian Romanick 2015-06-09 18:05:23 UTC
(In reply to Alexander Monakov from comment #14)
> Sigh.  Please apply this:
> 
> From dae0d4b5716b9a5d9227539bb6312f1f389bfcd3 Mon Sep 17 00:00:00 2001
> From: Alexander Monakov <amonakov@ispras.ru>
> Date: Tue, 9 Jun 2015 20:58:22 +0300
> Subject: [PATCH] i965: do_blit_drawpixels: decode array formats
> 
> ---
>  src/mesa/drivers/dri/i965/intel_pixel_draw.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/src/mesa/drivers/dri/i965/intel_pixel_draw.c
> b/src/mesa/drivers/dri/i965/intel_pixel_draw.c
> index d68cbb6..189a592 100644
> --- a/src/mesa/drivers/dri/i965/intel_pixel_draw.c
> +++ b/src/mesa/drivers/dri/i965/intel_pixel_draw.c
> @@ -78,6 +78,8 @@ do_blit_drawpixels(struct gl_context * ctx,
>     struct intel_renderbuffer *irb = intel_renderbuffer(rb);
>  
>     mesa_format src_format = _mesa_format_from_format_and_type(format, type);
> +   if (_mesa_format_is_mesa_array_format(src_format))
> +      src_format = _mesa_format_from_array_format(src_format);

Assuming this fixes the problem, I'd rather see it as:

   const mesa_format src_format = _mesa_format_is_mesa_array_format(src_format)
      ? _mesa_format_from_array_format(src_format)
      : _mesa_format_from_format_and_type(format, type);

>     mesa_format dst_format = irb->mt->format;
>  
>     /* We can safely discard sRGB encode/decode for the DrawPixels interface
> */
Comment 16 Alexander Monakov 2015-06-09 18:55:58 UTC
> >     struct intel_renderbuffer *irb = intel_renderbuffer(rb);
> >  
> >     mesa_format src_format = _mesa_format_from_format_and_type(format, type);
> > +   if (_mesa_format_is_mesa_array_format(src_format))
> > +      src_format = _mesa_format_from_array_format(src_format);
> 
> Assuming this fixes the problem, I'd rather see it as:
> 
>    const mesa_format src_format =
> _mesa_format_is_mesa_array_format(src_format)
                                    ^^^^^^^^^^

Huh?  You can't define src_format based on some predicate computed from yet-uninitialized src_format.

>       ? _mesa_format_from_array_format(src_format)
>       : _mesa_format_from_format_and_type(format, type);
Comment 17 AnAkkk 2015-06-09 20:08:14 UTC
(In reply to Alexander Monakov from comment #14)
> Sigh.  Please apply this:
> 
> From dae0d4b5716b9a5d9227539bb6312f1f389bfcd3 Mon Sep 17 00:00:00 2001
> From: Alexander Monakov <amonakov@ispras.ru>
> Date: Tue, 9 Jun 2015 20:58:22 +0300
> Subject: [PATCH] i965: do_blit_drawpixels: decode array formats
> 
> ---
>  src/mesa/drivers/dri/i965/intel_pixel_draw.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/src/mesa/drivers/dri/i965/intel_pixel_draw.c
> b/src/mesa/drivers/dri/i965/intel_pixel_draw.c
> index d68cbb6..189a592 100644
> --- a/src/mesa/drivers/dri/i965/intel_pixel_draw.c
> +++ b/src/mesa/drivers/dri/i965/intel_pixel_draw.c
> @@ -78,6 +78,8 @@ do_blit_drawpixels(struct gl_context * ctx,
>     struct intel_renderbuffer *irb = intel_renderbuffer(rb);
>  
>     mesa_format src_format = _mesa_format_from_format_and_type(format, type);
> +   if (_mesa_format_is_mesa_array_format(src_format))
> +      src_format = _mesa_format_from_array_format(src_format);
>     mesa_format dst_format = irb->mt->format;
>  
>     /* We can safely discard sRGB encode/decode for the DrawPixels interface
> */

I can confirm this fix the issue, thanks!
Comment 18 Jason Ekstrand 2015-06-09 21:22:17 UTC
(In reply to Alexander Monakov from comment #14)
> Sigh.  Please apply this:
> 
> From dae0d4b5716b9a5d9227539bb6312f1f389bfcd3 Mon Sep 17 00:00:00 2001
> From: Alexander Monakov <amonakov@ispras.ru>
> Date: Tue, 9 Jun 2015 20:58:22 +0300
> Subject: [PATCH] i965: do_blit_drawpixels: decode array formats
> 
> ---
>  src/mesa/drivers/dri/i965/intel_pixel_draw.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/src/mesa/drivers/dri/i965/intel_pixel_draw.c
> b/src/mesa/drivers/dri/i965/intel_pixel_draw.c
> index d68cbb6..189a592 100644
> --- a/src/mesa/drivers/dri/i965/intel_pixel_draw.c
> +++ b/src/mesa/drivers/dri/i965/intel_pixel_draw.c
> @@ -78,6 +78,8 @@ do_blit_drawpixels(struct gl_context * ctx,
>     struct intel_renderbuffer *irb = intel_renderbuffer(rb);
>  
>     mesa_format src_format = _mesa_format_from_format_and_type(format, type);
> +   if (_mesa_format_is_mesa_array_format(src_format))
> +      src_format = _mesa_format_from_array_format(src_format);
>     mesa_format dst_format = irb->mt->format;
>  
>     /* We can safely discard sRGB encode/decode for the DrawPixels interface
> */

Please send this to the mesa mailing list.
Comment 20 AnAkkk 2015-06-12 11:05:04 UTC
Everything works fine with mesa-git now, I guess this can be marked as fixed.

Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.