On Thursday, October 25, 2007 2:02 am Michel Dänzer wrote:
> > It still has some bugs. When moving windows between screens, Mesa
> > seems to lose track of the right vblank count to sync against
> > sometimes, so my test app's calls to glXWaitVideoSyncSGI return
> > immediately. Moving the window back and forth between the screens
> > a few times fixes this problem though, maybe there's a race in the
> > update of base_msc and which pipe to sync against?
>
> There can't really be a race with a single context... If this is
> still an issue with your updated patch, it might be related to some
> of the issues below.
I was thinking the internal state may be updated by the intelWindowMoved
function after the client calls glXWaitVideoSyncSGI but before it
returned... So not really a race between two threads, but possible
confusion anyway. Or is that not possible?
> > On the plus side, I never see the MSC count decrease, so at least
> > that part is working I think.
>
> Nice!
>
> > diff --git a/configs/default b/configs/default
>
> The configs/ changes probably shouldn't be in here.
Yeah, just a bit of frustration avoidance that snuck in...
> > + * this function was first called (i.e., system start up).
> > + *
> > + * \since Internal API version 20070925.
>
> __DRImediaStreamCounterExtensionRec has its own versioning now, you
> need to bump __DRI_MEDIA_STREAM_COUNTER_VERSION to 2.
Ah ok, I missed that.
> > + if ( psc->driScreen.private ) {
>
> Should be
>
> if ( psc->msc && psc->driScreen.private ) {
Ok.
>
> > + /*
> > + * Try to use getDrawableMSC first so we get the right
> > + * counter...
> > + */
> > + if (psc->msc->getDrawableMSC)
> > + ret = (*psc->msc->getDrawableMSC)( &psc->driScreen,
> > + pdraw->private,
> > + & temp);
> > + else
> > + ret = (*psc->msc->getMSC)( &psc->driScreen, & temp);
>
> I think you need to verify that (psc->msc->base.version >= 2) before
> accessing psc->msc->getDrawableMSC.
Ok.
> > pdraw->swapBuffers = driSwapBuffers; /* called by
> > glXSwapBuffers() */
> > + if (driCompareGLXAPIVersion(20070925) >= 0) {
> > + pdp->msc = 0;
> > + pdp->base_msc = 0;
> > + }
> > +
>
> I'm not sure this API version needs to be checked here or even
> bumped. Either way though, all added fields should probably be
> treated the same way.
I guess I'll just remove the check then.
> > + /[EMAIL PROTECTED]/
> > + int64_t msc;
> > + int64_t base_msc;
> > + int64_t primary_vblank_base;
> > + int64_t secondary_vblank_base;
>
> Shouldn't these be unsigned?
Elsewhere we pass around an int64_t *msc, so I just kept it consistent.
But yes, the values are inherently unsigned.
> Do we really need two vblank_base values? Couldn't there be just one
> for the current pipe?
Yeah, you're right I could just consolidate them like the vblFlags
field.
> Also, I'm not sure the msc field is needed here, couldn't the driver
> just increase base_msc by <current sequence of old pipe> -
> vblank_base? Actually I think something like that is needed anyway,
> as the msc field could be stale at that point?
Yeah, I was thinking the same thing after I posted, I'll drop the msc
field too.
> (BTW, the i915 driver changes for this seem to be missing.)
Yeah, I've been testing on 965. I'll add the 915 changes when I have
965 working, then test there.
> > - *count = (int64_t)vbl.reply.sequence;
> > +
> > + if ( dPriv && !(dPriv->vblFlags & VBLANK_FLAG_SECONDARY) ) {
> > + /* Primary pipe */
> > + dPriv->msc = dPriv->base_msc + vbl.reply.sequence -
> > + dPriv->primary_vblank_base;
> > + } else if (dPriv && (dPriv->vblFlags & VBLANK_FLAG_SECONDARY) )
> > { + /* Secondary pipe */
> > + dPriv->msc = dPriv->base_msc + vbl.reply.sequence -
> > + dPriv->secondary_vblank_base;
> > + } else {
> > + /* Old driver (no knowledge of per-drawable MSC callback) */
> > + dPriv->msc += vbl.reply.sequence;
> > + }
>
> The last else block doesn't seem to make sense - for one, it could
> only seem to be hit if dPriv == NULL, in which case it would
> segfault.
Oh yeah, another reason for dropping dPriv->msc. :) Obviously this
codepath was untested. It really should just return vbl.reply.sequence
as the count if dPriv is NULL (indicating a call from an old driver).
Thanks,
Jesse
-------------------------------------------------------------------------
This SF.net email is sponsored by: Splunk Inc.
Still grepping through log files to find problems? Stop.
Now Search log events and configuration files using AJAX and a browser.
Download your FREE copy of Splunk now >> http://get.splunk.com/
--
_______________________________________________
Dri-devel mailing list
Dri-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/dri-devel
