[Mesa-dev] [PATCH] i965: fix MakeCurrent when switching a context between multiple drawables.
Iago Toral
itoral at igalia.com
Fri Apr 11 00:42:03 PDT 2014
On Thu, 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.
Yes, but notice that this code also depends on having proper Width and
Height to work as well. _Xmin and cia are updated through
_mesa_scissor_bounding_box(), and the docs for that function say:
"\warning This function assumes that the framebuffer dimensions are up
to date (e.g., update_framebuffer_size has been recently called on \c
buffer)."
This function uses the buffer Width and Height and then tests the size
of the scissor rect against it. In fact, for the failing test case, the
size of the scissor rect is not 0, but Width and Height are, so that
makes _Xmin and cia all be 0 anyway.
update_framebuffer_size() is only for user-defined FBOs, but for the
system framebuffer the equivalent would be _mesa_resize_framebuffer()
which is called from intel_prepare_render() at least.
> The other code was added earlier in 2007:
>
> commit 4d2eb637a20e4fdf5d5f6c0ea4d4627894594661
> Author: Brian <brian at yutani.localnet.net>
> Date: Thu Mar 15 11:16:41 2007 -0600
>
> no-op clear if buffer width or height is zero (bug 7205)
>
> That bug is... quite long. It looks like comments #31 and #32 discuss
> this particular point.
Right, comment 32 in particular seems to be the reason the checks for
Width and Height in _mesa_Clear where added but it does not seem that it
had anything to do with the actual fix for the bug that was being
investigated there, it looks more like Brian thought it would make sense
to do that kind of check in general while he was checking the code. And
I guess it does, but it requires that we had computed the size of the
framebuffer before for it to work.
If we were updating our buffers every time on MakeCurrent those checks
would always work (and looking at intelMakeCurrent at that moment in
time it looks like we were doing exactly that) but after Kristian's
changes that won't happen any more.
> I also note that _mesa_ClearBufferiv and related functions do not have
> this check. That tells me that this check should probably happen
> somewhere else. Brian and Michel were involved with the original clear
> code, so they might have some thoughts... it was a long time ago, though.
Since the stacktraces posted in the bug go through glClear specifically
my guess is that Brian focused on glClear only and probably did not
think about other cases, but it is difficult to know for sure...
> > In any case, my conclusion is that if we don't call
> > intel_prepare_render() in MakeCurrent we have to remove these checks
> > from Mesa or at least skip them while we don't have buffers, unless I am
> > missing something else of course... so, thoughts?
> >
> >> I have a question though: Reading your initial commit I understand this
> >> is not because of performance reasons but mostly because always calling
> >> intel_prepare_render() in MakeCurrent was was creating a problem in
> >> other places... but should that happen? I guess what I mean to ask is if
> >> calling intel_prepare_render() in MaKeCurrent is simply unnecessary or
> >> an actual mistake that will create problems in some scenarios.
> >>
> >>> Also, for reference, we need the buffer size for the initial value of
> >>> the context viewport. So the first time a context is made current, we
> >>> call intel_prepare_render() to get the buffers so we can see what size
> >>> they are. When the same context is later made current with a
> >>> different drawable, we have a value for the viewport and we're not
> >>> supposed to change it, so there's no point in getting buffers.
> >>>
> >>> Kristian
> >>>
> > _______________________________________________
> > mesa-dev mailing list
> > mesa-dev at lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/mesa-dev
>
>
More information about the mesa-dev
mailing list