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

[Pauli Nieminen]

On 26/10/10 17:41 +0200, ext Jesse Barnes wrote:
> 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).

SGI_video_sync:
"The kernel maintains a video sync counter for 
each physical hardware pipe in a system; the counter is incremented 
upon the completion of the display of each full frame of video data. An
OpenGL context always corresponds to a pipe."

"The single video sync counter is shared by all GLXContexts."

Yes. You have to extent both OML_sync_control and SGI_video_sync to expose
separate MSC for different CRTCs. 

I can see race condition even with events.

* Client checks for event (none yet in queue)
* Client prepares to call some blocking msc call
* Window manager decides to move the window
* xserver send MSC change event
* Client calls blocking MSC call

But maybe there is solution. All blocking calls could fail if MSC base
changes. Client would have to query for new base and rate before trying to
issue same request again.

For swap interval it would be enough if DDX would notify DRI2 about crtc
changes with offset (msc_for_new_crtc - msc_for_old_crtc) that can be applied
to swaptarget.

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

That is too bad

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