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

[Iago Toral]

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.

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