[PATCH 2/2] DRI2: Add error message when working around driver bug

[Jesse Barnes]

On Tue, 26 Oct 2010 11:39:11 +0300
Pauli Nieminen <ext-pauli.nieminen at nokia.com> wrote:

> On 26/10/10 03:00 +0200, ext Mario Kleiner wrote:
> > On Oct 25, 2010, at 6:52 PM, Jesse Barnes wrote:
> > 
> > > On Mon, 25 Oct 2010 17:13:58 +0300
> > > Pauli Nieminen <ext-pauli.nieminen at nokia.com> wrote:
> > >
> > >> There isn't API that allows application atomically query for msc  
> > >> changes
> > >> and schedule swaps. If msc changes dramatically between query and
> > >> scheduling application would schedule swap to happen at wrong time.
> > >>
> > >> Because of API limitations driver has to make msc increment for each
> > >> vblank affecting the drawable.
> > >>
> > >> Signed-off-by: Pauli Nieminen <ext-pauli.nieminen at nokia.com>
> > >> CC: Kristian Høgsberg <krh at bitplanet.net>
> > >> ---
> > >>  hw/xfree86/dri2/dri2.c |    7 +++++--
> > >>  1 files changed, 5 insertions(+), 2 deletions(-)
> > >>
> > >> diff --git a/hw/xfree86/dri2/dri2.c b/hw/xfree86/dri2/dri2.c
> > >> index d9b9d57..d70c115 100644
> > >> --- a/hw/xfree86/dri2/dri2.c
> > >> +++ b/hw/xfree86/dri2/dri2.c
> > >> @@ -860,9 +860,12 @@ DRI2SwapBuffers(ClientPtr client, DrawablePtr  
> > >> pDraw, CARD64 target_msc,
> > >>  	    if (!(*ds->GetMSC)(pDraw, &ust, &current_msc))
> > >>  		pPriv->last_swap_target = 0;
> > >>
> > >> -	    if (current_msc < pPriv->last_swap_target)
> > >> +	    if (current_msc < pPriv->last_swap_target) {
> > >>  		pPriv->last_swap_target = current_msc;
> > >> -
> > >> +		xf86DrvMsg(pScreen->myNum, X_ERROR,
> > >> +			"[DRI2] %s: GetMSC returned swap count that is in "
> > >> +			"past. Working around driver bug.\n", __func__);
> > >> +	    }
> > >>  	}
> > >
> > > This one scares me a little.  We added this so we could also catch
> > > drawables moving between screens with different msc bases, so this
> > > patch could cause a lot of false positives (no question that the specs
> > > could use some additions here though).
> > >
> > > Making it a debug message that only shows up with -verbose would be
> > > fine though.
> > >
> > > -- 
> > > Jesse Barnes, Intel Open Source Technology Center
> > 
> > I agree with Jesse, as a debug message at -verbose it would be fine.
> > 
> 
> The "fix" that you added to common code doesn't fix the root cause. In that
> case error message is correct because common code is working around driver
> bug for single case that happens to be common. Driver that hits that error
> line shouldn't expose OML_sync_control because application can't use it
> reliably in that case.

It's not just OML_sync_control; it's also SGI_video_sync and
SGI_swap_interval (also part of the core EGL spec).  Disabling these
due to unsynced multihead would be a bit harsh; it would also be hard
because it's something that can change at runtime (what if a single
head config with these extensions exposed becomes multihead with
variable rate MSC due to a monitor getting plugged in?).

That's why I think we need to fix the spec instead; it would be easy
enough to add a new event to notify clients when the MSC rate and base
change due to a move to a new pipe, someone just needs to write it up
and code it (shouldn't be too hard).

> But if you really want only verbose message I'm happy to change the patch.

Yes please, until we get the spec and such worked out.  These sorts of
messages always confuse users; we end up with a bunch of bug reports
and OSVs patching it out anyway.

Thanks,
-- 
Jesse Barnes, Intel Open Source Technology Center