Bug 23670 - [bisected i915 i965] 3~5 OGLC cases fail
[bisected i915 i965] 3~5 OGLC cases fail
Status: VERIFIED FIXED
Product: Mesa
Classification: Unclassified
Component: Drivers/DRI/i915
git
Other Linux (All)
: low normal
Assigned To: Brian Paul
:
Depends on: 24083
Blocks:
  Show dependency treegraph
 
Reported: 2009-09-02 20:05 UTC by Shuang He
Modified: 2010-05-27 23:24 UTC (History)
2 users (show)

See Also:
i915 platform:
i915 features:


Attachments
test patch (553 bytes, patch)
2009-09-03 07:14 UTC, Brian Paul
Details | Splinter Review
another patch for meta.c (IndexOffset/Bias check) (616 bytes, patch)
2009-09-09 08:43 UTC, Brian Paul
Details | Splinter Review
small case (2.51 KB, text/plain)
2009-09-09 20:35 UTC, Shuang He
Details
another small case (2.80 KB, text/plain)
2009-09-15 20:37 UTC, Shuang He
Details
simple test case to reproduce failure of OGLC/zbfunc.c (2.48 KB, text/plain)
2009-10-10 00:34 UTC, Shuang He
Details
Set ortho with z range 1.0 to -1.0 for meta_DrawPixels (594 bytes, patch)
2010-05-24 11:08 UTC, Kristian Høgsberg
Details | Splinter Review
new patch for meta.c (2.32 KB, patch)
2010-05-24 13:12 UTC, Brian Paul
Details | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Shuang He 2009-09-02 20:05:41 UTC
System Environment:
--------------------------
Libdrm:		(master)73b59c894380995a2889b98e79acadd2da0bb237
Mesa:		(master)98a8744e02c5c1aa0c97c0265680f09f92a68818
Xserver:		(master)291408980f33b1e541c89d958535e6fad55fdac9
Xf86_video_intel:		(master)5812531e08147576de776b2dd64e7f94c08eb851
Kernel:       (drm-intel-next)dd64c8407d828796f638e40931f707ab96009c6e

Bug detailed description:
-------------------------
Glean case pixelFormats failed due to following commit:
commit 886a0a715076213266b4f96118d15de5be2bff27
Author: Brian Paul <brianp@vmware.com>
Date:   Tue Sep 1 12:16:13 2009 -0600

    intel: use _mesa_meta_draw_pixels()

    The textured quad path is slightly faster and will work with POT textures
    on i945.

And this commit also impact OGL Conformance test result:
copypix.c PASS->FAIL
packpix.c FAIL->PASS
pxconv-draw.c FAIL->PASS
pxconv-depth.c PASS->FAIL
pxstore-depth.c PASS->FAIL
pxstore-draw.c FAIL->PASS
pxtrans-cidraw.c FAIL->PASS
pxtrans-depth.c PASS->FAIL


Reproduce steps:
----------------
1.xinit&
2. run glean/oglc case
Comment 1 Brian Paul 2009-09-03 07:14:44 UTC
Created attachment 29175 [details] [review]
test patch

Please try this patch.  It looks like the glDrawPixels(GL_LUMINANCE) case isn't working because of a problem with luminance textures (sampling returns A=0 instead of A=1).  This patch uses a GL_RGBA texture instead.  The real bug is probably elsewhere in the handling of GL_LUMINANCE textures...
Comment 2 Shuang He 2009-09-03 19:50:53 UTC
(In reply to comment #1)
> Created an attachment (id=29175) [details]
> test patch
> 
> Please try this patch.  It looks like the glDrawPixels(GL_LUMINANCE) case isn't
> working because of a problem with luminance textures (sampling returns A=0
> instead of A=1).  This patch uses a GL_RGBA texture instead.  The real bug is
> probably elsewhere in the handling of GL_LUMINANCE textures...
> 

This patch fixed glean/pixelFormats, I'd try OGLC later.
Comment 3 Shuang He 2009-09-04 00:20:06 UTC
(In reply to comment #2)
> (In reply to comment #1)
> > Created an attachment (id=29175) [details] [details]
> > test patch
> > 
> > Please try this patch.  It looks like the glDrawPixels(GL_LUMINANCE) case isn't
> > working because of a problem with luminance textures (sampling returns A=0
> > instead of A=1).  This patch uses a GL_RGBA texture instead.  The real bug is
> > probably elsewhere in the handling of GL_LUMINANCE textures...
> > 
> 
> This patch fixed glean/pixelFormats, I'd try OGLC later.
> 

This patch seems doesn't have affect on those OGLC result changes.
Comment 4 Brian Paul 2009-09-08 15:32:57 UTC
Could you provide more details about the failing cases (maybe run conform with -v 2)?  It looks like the failures are related to glCopyPixels(GL_DEPTH) and glDrawPixels(GL_DEPTH_COMPONENT) with pixel transfer ops.  I've tested those cases here with other test programs and found that glPixelTransfer(GL_DEPTH_SCALE/BIAS) works properly.
Comment 5 Shuang He 2009-09-08 22:40:03 UTC
(In reply to comment #4)
> Could you provide more details about the failing cases (maybe run conform with
> -v 2)?  It looks like the failures are related to glCopyPixels(GL_DEPTH) and
> glDrawPixels(GL_DEPTH_COMPONENT) with pixel transfer ops.  I've tested those
> cases here with other test programs and found that
> glPixelTransfer(GL_DEPTH_SCALE/BIAS) works properly.
> 

For OGLC/copypix.c, it fails with glDrawPixels(GL_STENCIL_INDEX) and glCopyPixels(GL_STENCIL)
Comment 6 Shuang He 2009-09-08 23:01:02 UTC
(In reply to comment #5)
> (In reply to comment #4)
> > Could you provide more details about the failing cases (maybe run conform with
> > -v 2)?  It looks like the failures are related to glCopyPixels(GL_DEPTH) and
> > glDrawPixels(GL_DEPTH_COMPONENT) with pixel transfer ops.  I've tested those
> > cases here with other test programs and found that
> > glPixelTransfer(GL_DEPTH_SCALE/BIAS) works properly.
> > 
> 
> For OGLC/copypix.c, it fails with glDrawPixels(GL_STENCIL_INDEX) and
> glCopyPixels(GL_STENCIL)
> 

For OGLC/apfunc.c, it fails with glDrawPixels(GL_DEPTH_COMPONENT), with alpha test enabled

OGLC/zbfunc.c fails with glDrawPixels(GL_RGBA, GL_FLOAT), with DEPTH func set to GL_ALWAYS

And there's also other OGLC cases (like api-rasterpos.c, depthwrite.c, spopTwoSided.c), seems to be impacted
Comment 7 Brian Paul 2009-09-09 08:43:00 UTC
Created attachment 29352 [details] [review]
another patch for meta.c (IndexOffset/Bias check)

Can you see if this patch helps with the glDrawPixels(GL_STENCIL_INDEX) cases?

Also, the -v 2 output would be helpful.
Comment 8 Shuang He 2009-09-09 20:31:19 UTC
(In reply to comment #7)
> Created an attachment (id=29352) [details]
> another patch for meta.c (IndexOffset/Bias check)
> 
> Can you see if this patch helps with the glDrawPixels(GL_STENCIL_INDEX) cases?
> 
> Also, the -v 2 output would be helpful.
> 

No, this doesn't help. And I'll upload a small case for it
Comment 9 Shuang He 2009-09-09 20:35:15 UTC
Created attachment 29361 [details]
small case

This case simply render a rectangle area in stencil buffer with glDrawPixels(GL_STENCIL). then render a red rectangle to validate the result.
This case works with software rendering, but not with i965 driver.
Comment 10 Brian Paul 2009-09-11 09:18:16 UTC
(In reply to comment #9)
> Created an attachment (id=29361) [details]
> small case
> 
> This case simply render a rectangle area in stencil buffer with
> glDrawPixels(GL_STENCIL). then render a red rectangle to validate the result.
> This case works with software rendering, but not with i965 driver.
> 

This should be fixed as of mesa commit 9e6ae75cc8d6bff139aa21bda0aa682755ab7a7c

For the other failures I need more information to know what's wrong.  I've tested a variety of pixel transfer and pixel store modes with glDrawPixels(GL_DEPTH_COMPONENT) (which I suspect pxstore-depth.c and pxtrans-cidraw.c exercise) but haven't found any issues.
Comment 11 Shuang He 2009-09-15 20:34:59 UTC
(In reply to comment #10)
> (In reply to comment #9)
> > Created an attachment (id=29361) [details] [details]
> > small case
> > 
> > This case simply render a rectangle area in stencil buffer with
> > glDrawPixels(GL_STENCIL). then render a red rectangle to validate the result.
> > This case works with software rendering, but not with i965 driver.
> > 
> 
> This should be fixed as of mesa commit 9e6ae75cc8d6bff139aa21bda0aa682755ab7a7c
> 
> For the other failures I need more information to know what's wrong.  I've
> tested a variety of pixel transfer and pixel store modes with
> glDrawPixels(GL_DEPTH_COMPONENT) (which I suspect pxstore-depth.c and
> pxtrans-cidraw.c exercise) but haven't found any issues.
> 

Yes, that's fixed. I'm uploading another small case for glDrawPixels(GL_DEPTH_COMPONENT) case
Comment 12 Shuang He 2009-09-15 20:37:59 UTC
Created attachment 29580 [details]
another small case

This case simply render two points through glDrawPixels(GL_DEPTH_COMPONENT), with ALPHA test enabled, and set to GL_ALWAYS. but it turn out the leftmost point is missed when rendering. This works well if only the leftmost point is rendered, or using software rendering
Comment 13 Brian Paul 2009-09-22 15:08:06 UTC
OK, can you retest with the latest code from git?  Commit 926b965ed53efc06a9d7cc6e07eff853b263960a should help with this.
Comment 14 Shuang He 2009-09-24 00:57:38 UTC
(In reply to comment #13)
> OK, can you retest with the latest code from git?  Commit
> 926b965ed53efc06a9d7cc6e07eff853b263960a should help with this.
> 

This makes apfunc.c, pxconv-draw.c, api-rasterpos.c, pxstore-draw.c passed on G45

And to summarize current status:
zbfunc.c, pxstore-depth.c, and depthwrite.c still failed on G45
zbfunc.c, pxconv-depth.c, pxstore-depth.c, pxtrans-depth.c, and depthwrite.c still failed on 945GM
Comment 15 Brian Paul 2009-09-24 08:28:15 UTC
(In reply to comment #14)
> (In reply to comment #13)
> > OK, can you retest with the latest code from git?  Commit
> > 926b965ed53efc06a9d7cc6e07eff853b263960a should help with this.
> > 
> 
> This makes apfunc.c, pxconv-draw.c, api-rasterpos.c, pxstore-draw.c passed on
> G45
> 
> And to summarize current status:
> zbfunc.c, pxstore-depth.c, and depthwrite.c still failed on G45
> zbfunc.c, pxconv-depth.c, pxstore-depth.c, pxtrans-depth.c, and depthwrite.c
> still failed on 945GM

I need more details to know what's wrong.  Can you make a test program for any of these?  Doesn't the -v 2 option provide more info about the failures?
Comment 16 Eric Anholt 2009-09-28 11:36:16 UTC
Please retest with:

commit cc8084932cc552587e3036dbbf62c77db3b4a08e
Author: Eric Anholt <eric@anholt.net>
Date:   Thu Sep 24 16:15:52 2009 -0700

    intel: Flush the batch when we're about to subdata into a VBO.
Comment 17 Shuang He 2009-09-28 18:49:22 UTC
(In reply to comment #16)
> Please retest with:
> 
> commit cc8084932cc552587e3036dbbf62c77db3b4a08e
> Author: Eric Anholt <eric@anholt.net>
> Date:   Thu Sep 24 16:15:52 2009 -0700
> 
>     intel: Flush the batch when we're about to subdata into a VBO.
> 

has tried mesa (master)99e1745af9a6a1fe1ebc65b17afb5f1a975348d2, which should include this commit. still gets same result.
Comment 18 Gordon Jin 2009-10-08 23:19:22 UTC
change the title since the glean case pass now.
Comment 19 Shuang He 2009-10-10 00:29:13 UTC
(In reply to comment #15)
> (In reply to comment #14)
> > (In reply to comment #13)
> > > OK, can you retest with the latest code from git?  Commit
> > > 926b965ed53efc06a9d7cc6e07eff853b263960a should help with this.
> > > 
> > 
> > This makes apfunc.c, pxconv-draw.c, api-rasterpos.c, pxstore-draw.c passed on
> > G45
> > 
> > And to summarize current status:
> > zbfunc.c, pxstore-depth.c, and depthwrite.c still failed on G45
> > zbfunc.c, pxconv-depth.c, pxstore-depth.c, pxtrans-depth.c, and depthwrite.c
> > still failed on 945GM
> 
> I need more details to know what's wrong.  Can you make a test program for any
> of these?  Doesn't the -v 2 option provide more info about the failures?
> 

for zbfunc.c, I've make a test program for it
Comment 20 Shuang He 2009-10-10 00:34:05 UTC
Created attachment 30247 [details]
simple test case to reproduce failure of OGLC/zbfunc.c

this case failed on both 945GM and G45, and get pass with software rendering.
it simply renders two points, with depth func set to GL_LESS, one point has depth value less than reference value(0.5), another point has depth value greater than reference value.
The expected result is red point should be rendered, blue point should not.
Comment 21 Eric Anholt 2009-12-02 17:10:22 UTC
I note that the new meta code doesn't have the conversion from [0,1] to NDC that my old meta code did.  Changing that doesn't seem to help, though.
Comment 22 Ian Romanick 2009-12-10 15:14:10 UTC
I'm reducing the priority to low.  The failure case is not one we expect to see in applications:  using DrawPixels with depth test using an inverted depth range.
Comment 23 Kristian Høgsberg 2010-05-24 11:08:37 UTC
Created attachment 35832 [details] [review]
Set ortho with z range 1.0 to -1.0 for meta_DrawPixels

This patch sets up glOrtho() as it was in the original intel meta code.  It fixes the testcase above and the oglc zbfunc.c case.  Honestly, not sure what's going on, but nothing regresses with the patch and swrast also still passes.
Comment 24 Brian Paul 2010-05-24 13:12:43 UTC
Created attachment 35836 [details] [review]
new patch for meta.c

Here's a new patch to try.  We were using the current raster Z position as-is instead of converting it back to an object-space Z coordinate.  The same bug was on the glDrawPixels, glCopyPixels and glBitmap paths.  The glClear path was correct.

If this fixes the issue for everyone, this patch should go into 7.8 too.
Comment 25 Kristian Høgsberg 2010-05-24 14:26:04 UTC
That's a great patch, it still fixes the test case in this bug (fdo23670-depth_test in piglit), zbfunc.c in oglc as well as quad-invariance and glsl-orangebook-ch06-bump in piglit.

I've committed it on master and picked it back to 7.8.  Thanks!
Comment 26 Shuang He 2010-05-27 23:23:34 UTC
current status is:
piglit/fdo23670-depth_test passed on both i915 and i965 platforms
piglit/fdo23670-drawpix_stencil passed on both i915 and i965 platforms
oglc/zbfunc.c passed on both i915 and i965 platforms
Depthwrite.c passed on both i915 and i965 platforms

pxstore-depth.c still failed on both i915 and i965 platforms
pxconv-depth.c still failed on i915 platoforms
pxtrans-depth.c still failed on i915 platoforms

bug #28292 is filed for tracking failure of pxstore-depth.c
bug #28293 is filed for tracking failures of pxconv-depth.c and pxtrans-depth.c
Comment 27 Shuang He 2010-05-27 23:24:29 UTC
verified for oglc/zbfunc.c and depthwrite.c.
Thanks