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 <e...@anholt.net> wrote: > >>>> Iago Toral Quiroga <ito...@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 <mic...@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 _______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
