Bug 23670

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...

(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.

(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.

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.

(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)

(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

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.

(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

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.

(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.

(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

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

OK, can you retest with the latest code from git?  Commit 926b965ed53efc06a9d7cc6e07eff853b263960a should help with this.

(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

(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?

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.

(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.

change the title since the glean case pass now.

(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

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.

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.

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.

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.

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.

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!

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

verified for oglc/zbfunc.c and depthwrite.c.
Thanks