On Tuesday, February 3, 2009 1:34 am Michel Dänzer wrote:
> On Mon, 2009-02-02 at 18:15 -0800, Jesse Barnes wrote:
> > On Monday, February 2, 2009 2:49 pm Michel Dänzer wrote:
> >
> > As for the higher level question, I hope we both agree that we should fix
> > this up,
>
> I'm fine with it, as long as it's clear that it's just a workaround for
> bad counter behaviour.
>
> > though I'm not sure why we need a frame count cap at all. We've already
> > got the timeout, so shouldn't we just keep the code simple? Is debugging
> > tearing easier than debugging a hang? I guess that's what we're really
> > getting at...
>
> I'm afraid you lost me here, care to elaborate on what you mean by
> 'frame count cap' and what this has to do with debugging tearing vs.
> hangs?
>
> If by 'frame count cap' you mean the limit for the number of frames that
> can be waited for, that's implicitly there (even if we only considered a
> single delta as 'has passed', one couldn't wait for more than (2 ^ 32 -
> 1) frames) and your proposal changes it in the same direction, just only
> half the way. I'm suggesting to take it further if at all, as the
> motivation is to catch pathological cases rather than being able to wait
> as long as possible.
Yeah I mean a (small) limit to the number of frames we can wait for. Without
an explicit limit we'll be capped at 2^31 frames. What I'm saying is that
anything greater than a few hundred frames (up to the limit) will be caught
by the timeout, and anything greater than the limit will return instantly.
It sounds like you're suggesting we make the code return instantly if
anything greater than a few thousand frames is passed in, which is fine
(though I don't know if there are displays with khz or mhz refresh rates out
there where that would cause problems).
>
> > > > The jumping counter exposed that problem and should probably be fixed
> > > > too (just to keep the counter values more friendly) but the core
> > > > problem should be fixed first, imo.
> > >
> > > I still disagree there: If the counter doesn't jump, even the current
> > > code works just fine as long as userspace calls the ioctl at least once
> > > a day. We can try to contain the damage caused by a jumping counter,
> > > but we can't completely eliminate it in all cases. Even if we can
> > > usually handle it sanely in the kernel, userspace libraries / apps may
> > > not be so robust. From my POV the core problem is clearly the jumping
> > > counter, everything else discussed in this thread is just workarounds
> > > to paper over it.
> >
> > I looked at this more; apparently the framecount reg on at least my
> > machine is running way too fast, which triggered the behavior we talked
> > about earlier. But given that we didn't use to track frame counts across
> > VT switch (many drivers removed their IRQ handler, so the frame count
> > wouldn't increase between LeaveVT and EnterVT) and things seemed ok,
>
> Yeah, I think a counter that doesn't move is less problematic than one
> that moves erratically from the POV of users of this functionality.
>
> > it's probably fine to just return 0 from the get_vblank_counter hook,
> > and ignore frames we lost between interrupt/vblank off periods.
>
> Not sure I understand what you're proposing here, do you have a patch?
No you got it; it would keep the counter from jumping at all between
IRQ/vblank off->on transitions. The only downside is that apps that do their
own calculations for vblank sequence numbers and then try to use them across
such periods might be surprised, but the GL wrappers around the ioctl should
handle that case. The patch below is all it takes. Remind me again why we
bothered to keep an accurate count across off->on periods? :)
--
Jesse Barnes, Intel Open Source Technology Center
diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index 6290219..879a5d3 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -137,41 +137,17 @@ i915_pipe_enabled(struct drm_device *dev, int pipe)
return 0;
}
-/* Called from drm generic code, passed a 'crtc', which
- * we use as a pipe index
+/*
+ * Stubbing this out keeps us from tracking lost frames across
+ * vblank/IRQ off periods, but that should be fine, since it will
+ * stay accurate as long as there's a client waiting. And at
+ * VT switch any frames that occur between LeaveVT and EnterVT
+ * will be invisible to the client, which is arguably correct
+ * anyway.
*/
u32 i915_get_vblank_counter(struct drm_device *dev, int pipe)
{
- drm_i915_private_t *dev_priv = (drm_i915_private_t *) dev->dev_private;
- unsigned long high_frame;
- unsigned long low_frame;
- u32 high1, high2, low, count;
-
- high_frame = pipe ? PIPEBFRAMEHIGH : PIPEAFRAMEHIGH;
- low_frame = pipe ? PIPEBFRAMEPIXEL : PIPEAFRAMEPIXEL;
-
- if (!i915_pipe_enabled(dev, pipe)) {
- DRM_ERROR("trying to get vblank count for disabled pipe %d\n",
pipe);
- return 0;
- }
-
- /*
- * High & low register fields aren't synchronized, so make sure
- * we get a low value that's stable across two reads of the high
- * register.
- */
- do {
- high1 = ((I915_READ(high_frame) & PIPE_FRAME_HIGH_MASK) >>
- PIPE_FRAME_HIGH_SHIFT);
- low = ((I915_READ(low_frame) & PIPE_FRAME_LOW_MASK) >>
- PIPE_FRAME_LOW_SHIFT);
- high2 = ((I915_READ(high_frame) & PIPE_FRAME_HIGH_MASK) >>
- PIPE_FRAME_HIGH_SHIFT);
- } while (high1 != high2);
-
- count = (high1 << 8) | low;
-
- return count;
+ return 0;
}
irqreturn_t i915_driver_irq_handler(DRM_IRQ_ARGS)
------------------------------------------------------------------------------
Create and Deploy Rich Internet Apps outside the browser with Adobe(R)AIR(TM)
software. With Adobe AIR, Ajax developers can use existing skills and code to
build responsive, highly engaging applications that combine the power of local
resources and data with the reach of the web. Download the Adobe AIR SDK and
Ajax docs to start building applications today-http://p.sf.net/sfu/adobe-com
--
_______________________________________________
Dri-devel mailing list
Dri-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/dri-devel
