[Mesa-dev] [PATCH] i965: fix MakeCurrent when switching a context between multiple drawables.
Kristian Høgsberg
krh at bitplanet.net
Mon Apr 21 23:25:56 PDT 2014
On Thu, Apr 10, 2014 at 1:31 AM, Iago Toral <itoral at igalia.com> 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.
>
> 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.
Looks like I never answered this question...
It's not a problem to call intel_prepare_render() in MakeCurrent -
MakeCurrent is allowed to make a roundtrip to the display server and
block on that. I did hit a problem where weston would wait on it's
helper client to render something and that client would block in
eglMakeCurrent, creating a deadlock. That circular dependency was the
core problem, of course, and that's been solved, but it reminded me
that I was never able to make getting buffers fully on-demand. We
need to get the buffers initially to initialize the GL viewport size,
but after that we should only get buffers when the loader invalidates
the current set.
It's not just an academic optimization - the driver code relies on
intel_prepare_render() to be called in the right places to make sure
we render to the right buffers. With an up-front call to
intel_prepare_render() in MakeCurrent, we always have some buffers and
we'll never catch cases where we miss an intel_prepare_render() call.
That's the main reason for trying to avoid calls to
intel_prepare_render() from MakeCurrent.
Kristian
>> 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