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

[Ian Romanick]

On 08/21/2013 04:22 PM, Chad Versace wrote:
> 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?

Yes.  Thanks for diagnosing this.  I'm sure it was irritating.

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