[Mesa-dev] [PATCH] i965: fix MakeCurrent when switching a context between multiple drawables.

Thu Apr 10 19:33:31 PDT 2014

On Don, 2014-04-10 at 12:22 -0700, Ian Romanick wrote:
> On 04/10/2014 03:39 AM, Iago Toral wrote:
> > On Thu, 2014-04-10 at 10:31 +0200, Iago Toral wrote:
> >> Hi Kristian,
> >>
> >> On Tue, 2014-04-08 at 10:14 -0700, Kristian Høgsberg wrote:
> >>> On Mon, Apr 7, 2014 at 9:24 AM, Eric Anholt <eric at anholt.net> wrote:
> >>>> Iago Toral Quiroga <itoral at igalia.com> writes:
> >>>>
> >>>>> Commit 11baad35088dfd4bdabc1710df650dbfb413e7a3 produces a regression when
> >>>>> switching a single context between multiple drawables.
> >>>>>
> >>>>> The problem is that we check whether we have a viewport set to decide if we
> >>>>> need to generate buffers for the drawble, but the viewport is initialized
> >>>>> with the first call to MakeCurrent for that context, so calling MakeCurrent on
> >>>>> the same context with a different drawable will have the viewport already
> >>>>> initialized and will not generate buffers for the new drawable.
> >>>>>
> >>>>> This patch fixes the problem by reverting to the previous solution implemented
> >>>>> in commit 05da4a7a5e7d5bd988cb31f94ed8e1f053d9ee39 with a small fix to suppport
> >>>>> single buffer drawables too. This solution checks if we have a renderbuffer for
> >>>>> the drawable to decide if we need to generate a buffer or not. The original
> >>>>> implementation, however, did this by checking the BACK_LEFT buffer, which is
> >>>>> not a valid solution for single buffer drawables. This patch modifies this
> >>>>> to check the FRONT_LEFT buffer instead, which should work in both scenarios.
> >>>>>
> >>>>> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=74005
> >>>>> ---
> >>>>>  src/mesa/drivers/dri/i965/brw_context.c | 9 +++++----
> >>>>>  1 file changed, 5 insertions(+), 4 deletions(-)
> >>>>>
> >>>>> diff --git a/src/mesa/drivers/dri/i965/brw_context.c b/src/mesa/drivers/dri/i965/brw_context.c
> >>>>> index c9719f5..c593286 100644
> >>>>> --- a/src/mesa/drivers/dri/i965/brw_context.c
> >>>>> +++ b/src/mesa/drivers/dri/i965/brw_context.c
> >>>>> @@ -926,6 +926,7 @@ intelMakeCurrent(__DRIcontext * driContextPriv,
> >>>>>  {
> >>>>>     struct brw_context *brw;
> >>>>>     GET_CURRENT_CONTEXT(curCtx);
> >>>>> +   struct intel_renderbuffer *rb = NULL;
> >>>>>
> >>>>>     if (driContextPriv)
> >>>>>        brw = (struct brw_context *) driContextPriv->driverPrivate;
> >>>>> @@ -950,6 +951,7 @@ intelMakeCurrent(__DRIcontext * driContextPriv,
> >>>>>        } else {
> >>>>>           fb = driDrawPriv->driverPrivate;
> >>>>>           readFb = driReadPriv->driverPrivate;
> >>>>> +         rb = intel_get_renderbuffer(fb, BUFFER_FRONT_LEFT);
> >>>>>           driContextPriv->dri2.draw_stamp = driDrawPriv->dri2.stamp - 1;
> >>>>>           driContextPriv->dri2.read_stamp = driReadPriv->dri2.stamp - 1;
> >>>>>        }
> >>>>> @@ -961,10 +963,9 @@ intelMakeCurrent(__DRIcontext * driContextPriv,
> >>>>>        intel_gles3_srgb_workaround(brw, fb);
> >>>>>        intel_gles3_srgb_workaround(brw, readFb);
> >>>>>
> >>>>> -      /* If the context viewport hasn't been initialized, force a call out to
> >>>>> -       * the loader to get buffers so we have a drawable size for the initial
> >>>>> -       * viewport. */
> >>>>> -      if (!brw->ctx.ViewportInitialized)
> >>>>> +      /* If we don't have buffers for the drawable yet, force a call to
> >>>>> +       * getbuffers here so we can have a default drawable size. */
> >>>>> +      if (rb && !rb->mt)
> >>>>>           intel_prepare_render(brw);
> >>>>
> >>>> We won't have an rb->mt for the front unless you're doing front buffer
> >>>> rendering, so I think you're basically just backing out krh's change.
> >>>> Which I think is good -- it looks like he was papering over a bug
> >>>> elsewhere, and I think we *should* just prepare_render in makecurrent.
> >>>> But if we're going to revert, let's just actually revert.
> >>>
> >>> Here's what I wrote in https://bugs.freedesktop.org/show_bug.cgi?id=74005:
> >>> We don't want to revert the behaviour.  The initial patch removed a
> >>> call to intel_prepare_render() in intelMakeCurrent().  We're supposed
> >>> to call intel_prepare_render() any time we're about to touch the
> >>> buffers, but the up-front call to intel_prepare_render() in
> >>> intelMakeCurrent covered up a few places where we forgot.  The fix now
> >>> isn't to put back the up-front intel_prepare_render() call but to add
> >>> it in the rendering paths that are missing it.
> >>
> >> Thanks for explaining. I will try to find the place where we are missing
> >> the intel_prepare_render() call in this particular bug.
> > 
> > Actually, as far as I can see, we are already calling
> > intel_prepare_render() in the render path that is causing this problem,
> > but it turns out that by the time we have the chance to do that it is
> > already too late. I explain:
> > 
> > The test case is pretty much this:
> > 
> > glXMakeCurrent(dpy, win1, ctx);
> > glClear(GL_COLOR_BUFFER_BIT);
> > 
> > glXMakeCurrent(dpy, win2, ctx);
> > glClear(GL_COLOR_BUFFER_BIT);
> > 
> > If we don't generate buffers for the second drawable when we do the
> > second make current then we have to do it at some point during
> > glClear(). glClear() is implemented in _mesa_Clear() which will
> > eventually call ctx->Driver.Clear() after running some checks. The
> > problem here is that some of these checks will fail and make us bail out
> > before actually flowing into the driver specific code, so we won't have
> > an opportunity to call intel_prepare_render() before things break.
> > 
> > Actually, Driver.Clear() in the intel driver is already calling
> > intel_prepare_render() -see brw_clear()-, so the problem, as far as I
> > can see, is not that someone forgot to do this but that it is too late
> > since _mesa_Clear() will return early before getting there, making the
> > glClear() call a no-op at least for the first frame we render.
> > 
> > The code that produces the early exit in _mesa_Clear() is this:
> > 
> > if (ctx->DrawBuffer->Width == 0 || ctx->DrawBuffer->Height == 0 ||
> >     ctx->DrawBuffer->_Xmin >= ctx->DrawBuffer->_Xmax ||
> >     ctx->DrawBuffer->_Ymin >= ctx->DrawBuffer->_Ymax)
> >    return;
> > 
> > All these checks will make it return early because all the values
> > involved are zero before we call intel_prepare_render(). Removing these
> > checks lets the code flow into the driver code, which will then call
> > intel_prepare_render() and do the clear, producing the expected result
> > and making the corresponding piglit test pass.
> > 
> > I am not sure about the purpose of these checks though: is Mesa implying
> > we are expected to always have buffers before getting here and is simply
> > checking for this? or was this intended to be an optimization for
> > null-sized drawables? (but in any case why would we do these checks
> > specifically for the clear operation only when there are probably other
> > places in the code that could use similar checks in both scenarios?)
> 
> I'm not sure about the Width & Height check, but _Xmin and friends take
> the current scissor rectangle into account.  Effectively that part
> checks to see whether the clear is empty due to the scissor rectangle.
> 
> A bit of git archaeology shows the _Xmin et al. checks were added in
> 2007 for this very reason:
> 
> commit 86b81ef5aa14a2fa7be6e5c319c00324028a1761
> Author: Michel Dänzer <michel at tungstengraphics.com>
> Date:   Wed Oct 17 18:28:03 2007 +0200
> 
>     Don't call the driver clear hook when the effective scissor
> rectangle is empty.

I don't remember any details about this, but I'd assume the idea was to
avoid driver bugs like the one fixed by the previous commit.

-- 
Earthling Michel Dänzer            |                  http://www.amd.com
Libre software enthusiast          |                Mesa and X developer