On 11/23/2015 04:51 PM, Ville Syrjälä wrote:
On Mon, Nov 23, 2015 at 04:23:21PM +0100, Mario Kleiner wrote:
On 11/20/2015 04:34 PM, Ville Syrjälä wrote:
On Fri, Nov 20, 2015 at 04:24:50PM +0100, Mario Kleiner wrote:
...
Ok, but why would that be a bad thing? I think we want it to think it is
in the previous frame if it is called outside the vblank irq context.
The only reason we fudge it to the next frames vblank if i vblank irq is
because we know the vblank irq handler we are executing atm. was meant
to execute within the upcoming vblank for the next frame, so we fudge
the scanout positions and thereby timestamp to correspond to that new
frame. But if something called outside irq context it should get a
scanout position/timestamp that corresponds to "reality".
It would be a bad thing since it would cause the timestamp to jump
backwards, and that would also cause the frame count guesstimate to go
backwards.
But only if we don't use the dev->driver->get_vblank_counter() method,
which we try to use on AMD. Also dev->vblank_time_lock should protect us
from concurrent execution of the vblank counting/timestamping in irq
context and non-irq context? At least that was one of the purposes of
that lock in the past?
-mario
It would maybe be a problem to get different answers for a query at the
same scanout position if we used .get_scanout_position() for something
else than for calculating vblank timestamps, but we don't do that atm.
Maybe i'm overlooking something here related to the somewhat rewritten
code from the last year or so? But in the original design this would be
exactly what i intended?
...
So it's good enough for typical desktop
applications/compositors/games/media players, and a nice improvement
over the previous state, but not quite sufficient for applications that
need long time consistent vblank counts for long waits of multiple
seconds or even minutes. So it's still good to have hw counters if we
can get some that are reliable enough.
Ah, I didn't realize you care about small errors in the counter for
such long periods of vblank off.
Actually, you are right, i was stupid - not enough sleep last friday. I
do care about such small errors, but the long vblank off periods don't
matter at all the way my software works. I query the current count and
timestamp (glXGetSyncValuesOML), calculate a target count based on those
and then schedule a swap for that target count via glXSwapBuffersMscOML.
That swapbuffers call will keep vblank irqs on until the kms pageflip is
queued. So i only care about vblank counts/timestamps being consistent
for short amounts of time, typically 1 video refresh from vblank query
to queueing a vblank event, and then from reception of that
event/queuing the pageflip to pageflip completion event. So even if the
system would be heavily loaded and my code would have big preemption
delays i think counts that are consistent over a few seconds would be
enough to keep things working. Otherwise it wouldn't work now either
with a vblank off after 5 seconds and nouveau not having vblank hw counters.
-mario
-mario
-mario
It almost sort of works on the rs600 code path, but i need a bit of info
from you:
1. There's this register from the old specs for m76.pdf, which is not
part of the current register defines for radeon-kms:
"D1CRTC_STATUS_VF_COUNT - RW - 32 bits - [GpuF0MMReg:0x60A8]"
It contains the lower 16 bits of framecounter and the 13 bits of
vertical scanout position. It seems to give the same readings as the 24
bit R_0060A4_D1CRTC_STATUS_FRAME_COUNT we use for the hw counter. This
would come handy.
Does Evergreen and later have a same/similar register and where is it?
2. The hw framecounter seems to increment when the vertical scanout
position wraps back from (VTOTAL - 1) to 0, at least on the one DCE-3
gpu i tested so far. Is this so on all asics? And is the hw counter
increment happening exactly at the moment that vertical scanout position
jumps back to zero, ie. both events are driven by the same signal? Or is
the framecounter increment just happening somewhere inside either
scanline VTOTAL-1 or scanline 0?
If we can fix this and get it into rc2 or rc3 then we could avoid a bad
regression and with a bit of luck at the same time improve by being able
to set dev->vblank_disable_immediate = true then and allow vblank irqs
to get turned off more aggressively for a bit of extra power saving.
thanks,
-mario
-- a/drivers/gpu/drm/drm_irq.c
+++ b/drivers/gpu/drm/drm_irq.c
@@ -172,9 +172,11 @@ static void drm_update_vblank_count(struct drm_device *dev, unsigned int pipe,
unsigned long flags)
{
struct drm_vblank_crtc *vblank = &dev->vblank[pipe];
- u32 cur_vblank, diff;
+ u32 cur_vblank, diff = 0;
bool rc;
struct timeval t_vblank;
+ const struct timeval *t_old;
+ u64 diff_ns;
int count = DRM_TIMESTAMP_MAXRETRIES;
int framedur_ns = vblank->framedur_ns;
@@ -195,13 +197,15 @@ static void drm_update_vblank_count(struct drm_device *dev, unsigned int pipe,
rc = drm_get_last_vbltimestamp(dev, pipe, &t_vblank, flags);
} while (cur_vblank != dev->driver->get_vblank_counter(dev, pipe) && --count > 0);
- if (dev->max_vblank_count != 0) {
- /* trust the hw counter when it's around */
- diff = (cur_vblank - vblank->last) & dev->max_vblank_count;
- } else if (rc && framedur_ns) {
- const struct timeval *t_old;
- u64 diff_ns;
-
+ /*
+ * Always use vblank timestamping based method if supported to reject
+ * redundant vblank irqs. E.g., AMD hardware needs this to not screw up
+ * due to some irq handling quirk.
Hmm. I'm thinking it would be better to simply not claim that there's a hw
counter if it isn't reliable.
+ *
+ * This also sets the diff value for use as fallback below in case the
+ * hw does not support a suitable hw vblank counter.
+ */
+ if (rc && framedur_ns) {
t_old = &vblanktimestamp(dev, pipe, vblank->count);
diff_ns = timeval_to_ns(&t_vblank) - timeval_to_ns(t_old);
@@ -212,11 +216,28 @@ static void drm_update_vblank_count(struct drm_device *dev, unsigned int pipe,
*/
diff = DIV_ROUND_CLOSEST_ULL(diff_ns, framedur_ns);
- if (diff == 0 && flags & DRM_CALLED_FROM_VBLIRQ)
- DRM_DEBUG_VBL("crtc %u: Redundant vblirq ignored."
- " diff_ns = %lld, framedur_ns = %d)\n",
- pipe, (long long) diff_ns, framedur_ns);
- } else {
+ if (diff == 0 && flags & DRM_CALLED_FROM_VBLIRQ) {
+ DRM_DEBUG_VBL("crtc %u: Redundant vblirq ignored."
+ " diff_ns = %lld, framedur_ns = %d)\n",
+ pipe, (long long) diff_ns, framedur_ns);
+
+ /* Treat this redundant invocation as no-op. */
+ WARN_ON_ONCE(cur_vblank != vblank->last);
+ return;
+ }
+ }
+
+ /*
+ * hw counters, if marked as supported via max_vblank_count != 0,
+ * *must* increment at leading edge of vblank and in sync with
+ * the firing of the hw vblank irq, otherwise we have a race here
+ * between gpu and us, where small variations in our execution
+ * timing can cause off-by-one miscounting of vblanks.
+ */
+ if (dev->max_vblank_count != 0) {
+ /* trust the hw counter when it's around */
+ diff = (cur_vblank - vblank->last) & dev->max_vblank_count;
+ } else if (!(rc && framedur_ns)) {
/* some kind of default for drivers w/o accurate vbl timestamping */
diff = (flags & DRM_CALLED_FROM_VBLIRQ) != 0;
}
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/dri-devel
