On Mon, Nov 23, 2015 at 06:58:34PM +0100, Mario Kleiner wrote:
>
>
> 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.
Well, if you do it that way then you have the problem of the hw counter
seeming to jump forward by one after crossing the start of vblank (when
compared to the value you sampled when you processed the early vblank
interrupt).
I guess one silly idea would be to defer the vblank interrupt processing
to a timer, and just schedule it a bit into the future from the actual
interrupt handler.
> 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?
The scenarion I outlined doesn't require anything to happen
concurrently.
>
> -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;
> >>>>>>>> }
> >>>>>>>
> >>>>>>>
> >>>>>
> >>>
> >
--
Ville Syrjälä
Intel OTC
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/dri-devel
