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. 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@lists.freedesktop.org > > http://lists.freedesktop.org/mailman/listinfo/mesa-dev > > > _______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev