On Wed, 2007-10-24 at 16:10 -0700, Jesse Barnes wrote:
> On Tuesday, October 23, 2007 10:32 am Jesse Barnes wrote:
> > On Tuesday, October 23, 2007 10:03 am Jesse Barnes wrote:
> > > On Tuesday, October 23, 2007 7:32 am Michel Dänzer wrote:
> > > > > Thinking about this more, I think we can make the counter not
> > > > > decrease, but I don't think we can avoid bad behavior.
> > > >
> > > > Why not, with something like the scheme Ian outlined above?
> > >
> > > You snipped out the reasons: we'll get bad behavior of one sort or
> > > another no matter what, unless we have genlocked displays.
> >
> > Ok, you guys banged this into my head on IRC. So the calculation
> > will be:
> > msc += last_pipe_vblank_count - cur_pipeX_vblank_count
> > like Ian said (where last_pipeX_vblank_count is updated everytime the
> > drawable moves onto pipe X). This gets a little ugly since I'll need
> > to track both a per-drawable MSC count as well as per-pipe
> > last_pipeX_vblank_count values. And unfortunately, since the
> > drawable movement is tracked in per-driver SAREA fields, this code
> > can't be generic. Ugg.
>
> Well at least some of the code can be generic, fortunately. Here's what
> I have. The extension support seems to have changed a bit since my
> last patch (this one is against Mesa HEAD as of yesterday), and I'm no
> longer sure about the compatibility issues with the patch.
>
> 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.
> 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.
> diff --git a/include/GL/internal/dri_interface.h
> b/include/GL/internal/dri_interface.h
> index e7fbf8e..f1d236a 100644
> --- a/include/GL/internal/dri_interface.h
> +++ b/include/GL/internal/dri_interface.h
> @@ -189,6 +189,18 @@ struct __DRImediaStreamCounterExtensionRec {
> int (*waitForMSC)(__DRIdrawable *drawable,
> int64_t target_msc, int64_t divisor, int64_t remainder,
> int64_t * msc, int64_t * sbc);
> +
> + /**
> + * Like the screen version of getMSC, but also takes a drawable so that
> + * the appropriate pipe's counter can be retrieved.
> + *
> + * Get the number of vertical refreshes since some point in time before
> + * 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.
> diff --git a/src/glx/x11/glxcmds.c b/src/glx/x11/glxcmds.c
> index 707e398..cbc498a 100644
> --- a/src/glx/x11/glxcmds.c
> +++ b/src/glx/x11/glxcmds.c
[...]
> + if ( psc->driScreen.private ) {
Should be
if ( psc->msc && psc->driScreen.private ) {
> + /*
> + * 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.
> diff --git a/src/mesa/drivers/dri/common/dri_util.c
> b/src/mesa/drivers/dri/common/dri_util.c
> index d59ea0d..d023231 100644
> --- a/src/mesa/drivers/dri/common/dri_util.c
> +++ b/src/mesa/drivers/dri/common/dri_util.c
> @@ -471,6 +480,8 @@ static void *driCreateNewDrawable(__DRIscreen *screen,
> pdp->numBackClipRects = 0;
> pdp->pClipRects = NULL;
> pdp->pBackClipRects = NULL;
> + pdp->vblSeq = 0;
> + pdp->vblFlags = 0;
>
> psp = (__DRIscreenPrivate *)screen->private;
> pdp->driScreenPriv = psp;
> @@ -485,6 +496,11 @@ static void *driCreateNewDrawable(__DRIscreen *screen,
> pdraw->private = pdp;
> pdraw->destroyDrawable = driDestroyDrawable;
> 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.
> diff --git a/src/mesa/drivers/dri/common/dri_util.h
> b/src/mesa/drivers/dri/common/dri_util.h
> index 91992a9..1e5301f 100644
> --- a/src/mesa/drivers/dri/common/dri_util.h
> +++ b/src/mesa/drivers/dri/common/dri_util.h
[...]
> + /**
> + * \name Monotonic MSC tracking
> + *
> + * Low level driver is responsible for updating base_msc, primary and
> + * secondary_vblank_base values so that higher level code can calculate
> + * a new msc value or msc target for a WaitMSC call. The new value will
> + * be:
> + * if (pipe == primary)
> + * msc = base_msc + get_vblank_count(primary) - primary_vblank_base;
> + * else
> + * msc = base_msc + get_vblank_count(secondary) -
> secondary_vblank_base;
> + *
> + * And for waiting on a value, core code will use:
> + * actual_target = target_msc - base_msc +
> + * (primary|secondary)_vblank_base;
> + */
> + /[EMAIL PROTECTED]/
> + int64_t msc;
> + int64_t base_msc;
> + int64_t primary_vblank_base;
> + int64_t secondary_vblank_base;
Shouldn't these be unsigned?
Do we really need two vblank_base values? Couldn't there be just one for
the current pipe?
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?
(BTW, the i915 driver changes for this seem to be missing.)
> diff --git a/src/mesa/drivers/dri/common/vblank.c
> b/src/mesa/drivers/dri/common/vblank.c
> index 3b5acfe..032cd40 100644
> --- a/src/mesa/drivers/dri/common/vblank.c
> +++ b/src/mesa/drivers/dri/common/vblank.c
[...]
> - *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.
--
Earthling Michel Dänzer | http://tungstengraphics.com
Libre software enthusiast | Debian, X and DRI developer
-------------------------------------------------------------------------
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
