Bug 90839 - [10.5.5/10.6 regression, bisected] PBO glDrawPixels no longer using blit fastpath
Summary: [10.5.5/10.6 regression, bisected] PBO glDrawPixels no longer using blit fast...
Status: RESOLVED FIXED
Alias: None
Product: Mesa
Classification: Unclassified
Component: Drivers/DRI/i965 (show other bugs)
Version: unspecified
Hardware: Other All
: medium normal
Assignee: Ian Romanick
QA Contact: Intel 3D Bugs Mailing List
URL:
Whiteboard:
Keywords:
: 90791 (view as bug list)
Depends on:
Blocks:
 
Reported: 2015-06-04 08:40 UTC by Alexander Monakov
Modified: 2015-06-12 11:05 UTC (History)
4 users (show)

See Also:
i915 platform:
i915 features:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
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.


bug/show.html.tmpl processed on Mar 25, 2017 at 17:25:23.
(provided by the Example extension).