[Mesa-dev] [PATCH] i965: Fix misapplication of gles3 srgb workaround

[Chad Versace]

On 08/21/2013 11:57 AM, Ian Romanick wrote:
> On 08/21/2013 11:09 AM, Chad Versace wrote:
>> Fixes inconsistent failure of gles2conform/GL2Tests/glUniform/glUniform.test
>> under gnome-shell. What follows is a description of the bug and its fix.
>>
>> When intel_update_renderbuffers() allocates a miptree for a winsys
>> renderbuffer, it propagates the renderbuffer's format to become also the
>> miptree's format.
>>
>> If the winsys color buffer format is SARGB, then, in the first call to
>> eglMakeCurrent, intel_gles3_srgb_workaround() changes the renderbuffer's
>> format to ARGB. That is, it changes the format from sRGB to non-sRGB.
>> However, it changes the renderbuffer's format *after*
>> intel_update_renderbuffers() has allocated the renderbuffer's miptree.
>> Therefore, when eglMakeCurrent returns, the miptree format (SARGB)
>> differs from the renderbuffer format (ARGB).
>>
>> If the X server reallocates the color buffer,
>> intel_update_renderbuffers() will create a new miptree for the
>> renderbuffer. The new miptree's format (ARGB) will differ from old
>> miptree's format (SARGB). This mismatch between old and new miptrees
>> causes bugs.
>>
>> Fix the bug by moving intel_gles3_srgb_workaround() to occur *before*
>> intel_update_renderbuffers().
>
> This sounds reasonable... but why did the test fail intermittently?

The gles3conform test renders a reference image and test image.
As far as I can tell, the test fails only when the Xserver reallocates
the color buffer after the test calls glClear for the reference image
but before it calls glClear for the test image.
During the first glClear, mt->format == SARGB8. During the second glClear, mt->format == ARGB8.
Therefore the reference image and test image differ only in background color.

But, I say "as far as I can tell", because it's very hard to diagnose
racy failures like this. I inferred my diagnosis by combing through
debug logs, so I'm not entirely confident of my conclusion.

I think the bug exposed itself only in this test because its the only
test in which sufficient time passes between the reference image and
the test image, likely due to transform feedback activity.

Does that get your r-b?

>
>> CC: "9.2" <mesa-stable at lists.freedesktop.org>
>> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=67934
>> Signed-off-by: Chad Versace <chad.versace at linux.intel.com>
>> ---
>>   src/mesa/drivers/dri/i965/intel_context.c | 10 +++++++---
>>   1 file changed, 7 insertions(+), 3 deletions(-)
>>
>> diff --git a/src/mesa/drivers/dri/i965/intel_context.c b/src/mesa/drivers/dri/i965/intel_context.c
>> index 2e38475..37c1770 100644
>> --- a/src/mesa/drivers/dri/i965/intel_context.c
>> +++ b/src/mesa/drivers/dri/i965/intel_context.c
>> @@ -755,11 +755,15 @@ intelMakeCurrent(__DRIcontext * driContextPriv,
>>        driContextPriv->dri2.read_stamp = driReadPriv->dri2.stamp - 1;
>>         }
>>
>> +      /* The sRGB workaround changes the renderbuffer's format. We must change
>> +       * the format before the renderbuffer's miptree get's allocated, otherwise
>> +       * the formats of the renderbuffer and its miptree will differ.
>> +       */
>> +      intel_gles3_srgb_workaround(brw, fb);
>> +      intel_gles3_srgb_workaround(brw, readFb);
>> +
>>         intel_prepare_render(brw);
>>         _mesa_make_current(ctx, fb, readFb);
>> -
>> -      intel_gles3_srgb_workaround(brw, ctx->WinSysDrawBuffer);
>> -      intel_gles3_srgb_workaround(brw, ctx->WinSysReadBuffer);
>>      }
>>      else {
>>         _mesa_make_current(NULL, NULL, NULL);
>>
>
>