| Message ID | 20200115121652.7050-4-tzimmermann@suse.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm: Clean up VBLANK callbacks in struct drm_driver | expand |
On Wed, Jan 15, 2020 at 01:16:34PM +0100, Thomas Zimmermann wrote: > The callback get_vblank_timestamp() is currently located in struct > drm_driver, but really belongs into struct drm_crtc_funcs. Add an > equivalent there. Driver will be converted in separate patches. > > The default implementation is drm_calc_vbltimestamp_from_scanoutpos(). > The patch adds drm_crtc_vblank_helper_get_vblank_timestamp(), which is > an implementation for the CRTC callback. > > v2: > * rename helper to drm_crtc_vblank_helper_get_vblank_timestamp() > * replace drm_calc_vbltimestamp_from_scanoutpos() with > drm_crtc_vblank_helper_get_vblank_timestamp() in docs > > Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de> > --- > drivers/gpu/drm/drm_vblank.c | 236 +++++++++++++++++++++-- > include/drm/drm_crtc.h | 46 ++++- > include/drm/drm_modeset_helper_vtables.h | 4 +- > include/drm/drm_vblank.h | 26 ++- > 4 files changed, 292 insertions(+), 20 deletions(-) > > diff --git a/drivers/gpu/drm/drm_vblank.c b/drivers/gpu/drm/drm_vblank.c > index abb085c67d82..1b7af316e6a5 100644 > --- a/drivers/gpu/drm/drm_vblank.c > +++ b/drivers/gpu/drm/drm_vblank.c > @@ -333,7 +333,9 @@ u64 drm_crtc_accurate_vblank_count(struct drm_crtc *crtc) > u64 vblank; > unsigned long flags; > > - WARN_ONCE(drm_debug_enabled(DRM_UT_VBL) && !dev->driver->get_vblank_timestamp, > + WARN_ONCE(drm_debug_enabled(DRM_UT_VBL) && > + !crtc->funcs->get_vblank_timestamp && > + !dev->driver->get_vblank_timestamp, > "This function requires support for accurate vblank timestamps."); > > spin_lock_irqsave(&dev->vblank_time_lock, flags); > @@ -511,9 +513,9 @@ EXPORT_SYMBOL(drm_crtc_vblank_waitqueue); > * > * Calculate and store various constants which are later needed by vblank and > * swap-completion timestamping, e.g, by > - * drm_calc_vbltimestamp_from_scanoutpos(). They are derived from CRTC's true > - * scanout timing, so they take things like panel scaling or other adjustments > - * into account. > + * drm_crtc_vblank_helper_get_vblank_timestamp(). They are derived from > + * CRTC's true scanout timing, so they take things like panel scaling or > + * other adjustments into account. > */ > void drm_calc_timestamping_constants(struct drm_crtc *crtc, > const struct drm_display_mode *mode) > @@ -577,8 +579,9 @@ EXPORT_SYMBOL(drm_calc_timestamping_constants); > * > * Implements calculation of exact vblank timestamps from given drm_display_mode > * timings and current video scanout position of a CRTC. This can be directly > - * used as the &drm_driver.get_vblank_timestamp implementation of a kms driver > - * if &drm_crtc_helper_funcs.get_scanout_position is implemented. > + * used as the &drm_crtc_funcs.get_vblank_timestamp implementation of a kms > + * driver if &drm_crtc_helper_funcs.get_scanout_position or > + * &drm_driver.get_scanout_position is implemented. > * > * The current implementation only handles standard video modes. For double scan > * and interlaced modes the driver is supposed to adjust the hardware mode > @@ -742,15 +745,22 @@ static bool > drm_get_last_vbltimestamp(struct drm_device *dev, unsigned int pipe, > ktime_t *tvblank, bool in_vblank_irq) > { > + struct drm_crtc *crtc = drm_crtc_from_index(dev, pipe); > bool ret = false; > > /* Define requested maximum error on timestamps (nanoseconds). */ > int max_error = (int) drm_timestamp_precision * 1000; > > /* Query driver if possible and precision timestamping enabled. */ > - if (dev->driver->get_vblank_timestamp && (max_error > 0)) > + if (crtc->funcs->get_vblank_timestamp && (max_error > 0)) { Could drop the superfluous parens while at it. Are all drivers utilizing this guaranteed to have the crtc here? > + struct drm_crtc *crtc = drm_crtc_from_index(dev, pipe); > + > + ret = crtc->funcs->get_vblank_timestamp(crtc, &max_error, > + tvblank, in_vblank_irq); > + } else if (dev->driver->get_vblank_timestamp && (max_error > 0)) { > ret = dev->driver->get_vblank_timestamp(dev, pipe, &max_error, > tvblank, in_vblank_irq); > + } > > /* GPU high precision timestamp query unsupported or failed. > * Return current monotonic/gettimeofday timestamp as best estimate. > @@ -1059,11 +1069,19 @@ EXPORT_SYMBOL(drm_crtc_vblank_get); > > static bool __vblank_disable_immediate(struct drm_device *dev, unsigned int pipe) > { > + struct drm_crtc *crtc; > + > if (!dev->vblank_disable_immediate) > return false; > - if (!dev->driver->get_vblank_timestamp) > - return false; > - return true; > + > + crtc = drm_crtc_from_index(dev, pipe); > + if (crtc && crtc->funcs->get_vblank_timestamp) Here you are expecting that the crtc might be NULL. > + return true; > + > + if (dev->driver->get_vblank_timestamp) > + return true; > + > + return false; > } > > static void drm_vblank_put(struct drm_device *dev, unsigned int pipe) > @@ -1742,9 +1760,11 @@ int drm_wait_vblank_ioctl(struct drm_device *dev, void *data, > > static void drm_handle_vblank_events(struct drm_device *dev, unsigned int pipe) > { > + struct drm_crtc *crtc = drm_crtc_from_index(dev, pipe); > struct drm_pending_vblank_event *e, *t; > ktime_t now; > u64 seq; > + bool high_prec; > > assert_spin_locked(&dev->event_lock); > > @@ -1764,8 +1784,10 @@ static void drm_handle_vblank_events(struct drm_device *dev, unsigned int pipe) > send_vblank_event(dev, e, seq, now); > } > > - trace_drm_vblank_event(pipe, seq, now, > - dev->driver->get_vblank_timestamp != NULL); > + high_prec = crtc->funcs->get_vblank_timestamp || > + dev->driver->get_vblank_timestamp; Here again assuming the crtc can't be NULL. > + > + trace_drm_vblank_event(pipe, seq, now, high_prec); > } > > /** > @@ -2020,3 +2042,193 @@ int drm_crtc_queue_sequence_ioctl(struct drm_device *dev, void *data, > kfree(e); > return ret; > } > + > +/* > + * Helpers for struct drm_crtc_funcs > + */ > + > +/** > + * drm_crtc_vblank_helper_get_vblank_timestamp_internal - precise vblank > + * timestamp helper > + * @dev: DRM device > + * @pipe: index of CRTC whose vblank timestamp to retrieve > + * @max_error: Desired maximum allowable error in timestamps (nanosecs) > + * On return contains true maximum error of timestamp > + * @vblank_time: Pointer to time which should receive the timestamp > + * @in_vblank_irq: > + * True when called from drm_crtc_handle_vblank(). Some drivers > + * need to apply some workarounds for gpu-specific vblank irq quirks > + * if flag is set. > + * @get_scanout_position: > + * Callback function to retrieve the scanout position. See > + * @struct drm_crtc_helper_funcs.get_scanout_position. > + * > + * Implements calculation of exact vblank timestamps from given drm_display_mode > + * timings and current video scanout position of a CRTC. > + * > + * The current implementation only handles standard video modes. For double scan > + * and interlaced modes the driver is supposed to adjust the hardware mode > + * (taken from &drm_crtc_state.adjusted mode for atomic modeset drivers) to > + * match the scanout position reported. > + * > + * Note that atomic drivers must call drm_calc_timestamping_constants() before > + * enabling a CRTC. The atomic helpers already take care of that in > + * drm_atomic_helper_update_legacy_modeset_state(). > + * > + * Returns: > + * > + * Returns true on success, and false on failure, i.e. when no accurate > + * timestamp could be acquired. > + */ > +bool > +drm_crtc_vblank_helper_get_vblank_timestamp_internal( > + struct drm_crtc *crtc, int *max_error, ktime_t *vblank_time, > + bool in_vblank_irq, > + bool (*get_scanout_position)(struct drm_crtc *crtc, > + bool in_vblank_irq, int *vpos, int *hpos, > + ktime_t *stime, ktime_t *etime, > + const struct drm_display_mode *mode)) > +{ > + struct drm_device *dev = crtc->dev; > + unsigned int pipe = crtc->index; > + struct drm_vblank_crtc *vblank = &dev->vblank[pipe]; > + struct timespec64 ts_etime, ts_vblank_time; > + ktime_t stime, etime; > + bool vbl_status; > + const struct drm_display_mode *mode; > + int vpos, hpos, i; > + int delta_ns, duration_ns; > + > + if (pipe >= dev->num_crtcs) { > + DRM_ERROR("Invalid crtc %u\n", pipe); > + return false; > + } > + > + /* Scanout position query not supported? Should not happen. */ > + if (!get_scanout_position) { > + DRM_ERROR("Called from CRTC w/o get_scanout_position()!?\n"); > + return false; > + } > + > + if (drm_drv_uses_atomic_modeset(dev)) > + mode = &vblank->hwmode; > + else > + mode = &crtc->hwmode; > + > + /* If mode timing undefined, just return as no-op: > + * Happens during initial modesetting of a crtc. > + */ > + if (mode->crtc_clock == 0) { > + DRM_DEBUG("crtc %u: Noop due to uninitialized mode.\n", pipe); > + WARN_ON_ONCE(drm_drv_uses_atomic_modeset(dev)); > + return false; > + } > + > + /* Get current scanout position with system timestamp. > + * Repeat query up to DRM_TIMESTAMP_MAXRETRIES times > + * if single query takes longer than max_error nanoseconds. > + * > + * This guarantees a tight bound on maximum error if > + * code gets preempted or delayed for some reason. > + */ > + for (i = 0; i < DRM_TIMESTAMP_MAXRETRIES; i++) { > + /* > + * Get vertical and horizontal scanout position vpos, hpos, > + * and bounding timestamps stime, etime, pre/post query. > + */ > + vbl_status = get_scanout_position(crtc, in_vblank_irq, &vpos, > + &hpos, &stime, &etime, mode); > + > + /* Return as no-op if scanout query unsupported or failed. */ > + if (!vbl_status) { > + DRM_DEBUG("crtc %u : scanoutpos query failed.\n", > + pipe); > + return false; > + } > + > + /* Compute uncertainty in timestamp of scanout position query. */ > + duration_ns = ktime_to_ns(etime) - ktime_to_ns(stime); > + > + /* Accept result with < max_error nsecs timing uncertainty. */ > + if (duration_ns <= *max_error) > + break; > + } > + > + /* Noisy system timing? */ > + if (i == DRM_TIMESTAMP_MAXRETRIES) { > + DRM_DEBUG("crtc %u: Noisy timestamp %d us > %d us [%d reps].\n", > + pipe, duration_ns/1000, *max_error/1000, i); > + } > + > + /* Return upper bound of timestamp precision error. */ > + *max_error = duration_ns; > + > + /* Convert scanout position into elapsed time at raw_time query > + * since start of scanout at first display scanline. delta_ns > + * can be negative if start of scanout hasn't happened yet. > + */ > + delta_ns = div_s64(1000000LL * (vpos * mode->crtc_htotal + hpos), > + mode->crtc_clock); > + > + /* Subtract time delta from raw timestamp to get final > + * vblank_time timestamp for end of vblank. > + */ > + *vblank_time = ktime_sub_ns(etime, delta_ns); > + > + if (!drm_debug_enabled(DRM_UT_VBL)) > + return true; > + > + ts_etime = ktime_to_timespec64(etime); > + ts_vblank_time = ktime_to_timespec64(*vblank_time); > + > + DRM_DEBUG_VBL("crtc %u : v p(%d,%d)@ %lld.%06ld -> %lld.%06ld [e %d us, %d rep]\n", > + pipe, hpos, vpos, > + (u64)ts_etime.tv_sec, ts_etime.tv_nsec / 1000, > + (u64)ts_vblank_time.tv_sec, ts_vblank_time.tv_nsec / 1000, > + duration_ns / 1000, i); > + > + return true; > +} > +EXPORT_SYMBOL(drm_crtc_vblank_helper_get_vblank_timestamp_internal); This looks like copy paste from the current thing. Why are you duplicating the entire function instead of refactoring what we already have? > + > +/** > + * drm_crtc_vblank_helper_get_vblank_timestamp - precise vblank timestamp > + * helper > + * @crtc: CRTC whose vblank timestamp to retrieve > + * @max_error: Desired maximum allowable error in timestamps (nanosecs) > + * On return contains true maximum error of timestamp > + * @vblank_time: Pointer to time which should receive the timestamp > + * @in_vblank_irq: > + * True when called from drm_crtc_handle_vblank(). Some drivers > + * need to apply some workarounds for gpu-specific vblank irq quirks > + * if flag is set. > + * > + * Implements calculation of exact vblank timestamps from given drm_display_mode > + * timings and current video scanout position of a CRTC. This can be directly > + * used as the &drm_crtc_funcs.get_vblank_timestamp implementation of a kms > + * driver if &drm_crtc_helper_funcs.get_scanout_position is implemented. > + * > + * The current implementation only handles standard video modes. For double scan > + * and interlaced modes the driver is supposed to adjust the hardware mode > + * (taken from &drm_crtc_state.adjusted mode for atomic modeset drivers) to > + * match the scanout position reported. > + * > + * Note that atomic drivers must call drm_calc_timestamping_constants() before > + * enabling a CRTC. The atomic helpers already take care of that in > + * drm_atomic_helper_update_legacy_modeset_state(). > + * > + * Returns: > + * > + * Returns true on success, and false on failure, i.e. when no accurate > + * timestamp could be acquired. > + */ > +bool drm_crtc_vblank_helper_get_vblank_timestamp(struct drm_crtc *crtc, > + int *max_error, > + ktime_t *vblank_time, > + bool in_vblank_irq) > +{ > + return drm_crtc_vblank_helper_get_vblank_timestamp_internal( > + crtc, max_error, vblank_time, in_vblank_irq, > + crtc->helper_private->get_scanout_position); > +} > +EXPORT_SYMBOL(drm_crtc_vblank_helper_get_vblank_timestamp); > diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h > index 5e9b15a0e8c5..db46abbbf4e7 100644 > --- a/include/drm/drm_crtc.h > +++ b/include/drm/drm_crtc.h > @@ -867,6 +867,47 @@ struct drm_crtc_funcs { > * new drivers as the replacement of &drm_driver.disable_vblank hook. > */ > void (*disable_vblank)(struct drm_crtc *crtc); > + > + /** > + * @get_vblank_timestamp: > + * > + * Called by drm_get_last_vbltimestamp(). Should return a precise > + * timestamp when the most recent vblank interval ended or will end. > + * > + * Specifically, the timestamp in @vblank_time should correspond as > + * closely as possible to the time when the first video scanline of > + * the video frame after the end of vblank will start scanning out, > + * the time immediately after end of the vblank interval. If the > + * @crtc is currently inside vblank, this will be a time in the future. > + * If the @crtc is currently scanning out a frame, this will be the > + * past start time of the current scanout. This is meant to adhere > + * to the OpenML OML_sync_control extension specification. > + * > + * Parameters: > + * > + * crtc: > + * CRTC for which timestamp should be returned. > + * max_error: > + * Maximum allowable timestamp error in nanoseconds. > + * Implementation should strive to provide timestamp > + * with an error of at most max_error nanoseconds. > + * Returns true upper bound on error for timestamp. > + * vblank_time: > + * Target location for returned vblank timestamp. > + * in_vblank_irq: > + * True when called from drm_crtc_handle_vblank(). Some drivers > + * need to apply some workarounds for gpu-specific vblank irq quirks > + * if flag is set. > + * > + * Returns: > + * > + * True on success, false on failure, which means the core should > + * fallback to a simple timestamp taken in drm_crtc_handle_vblank(). > + */ > + bool (*get_vblank_timestamp)(struct drm_crtc *crtc, > + int *max_error, > + ktime_t *vblank_time, > + bool in_vblank_irq); > }; > > /** > @@ -974,11 +1015,12 @@ struct drm_crtc { > * Programmed mode in hw, after adjustments for encoders, crtc, panel > * scaling etc. Should only be used by legacy drivers, for high > * precision vblank timestamps in > - * drm_calc_vbltimestamp_from_scanoutpos(). > + * drm_crtc_vblank_helper_get_vblank_timestamp(). > * > * Note that atomic drivers should not use this, but instead use > * &drm_crtc_state.adjusted_mode. And for high-precision timestamps > - * drm_calc_vbltimestamp_from_scanoutpos() used &drm_vblank_crtc.hwmode, > + * drm_crtc_vblank_helper_get_vblank_timestamp() used > + * &drm_vblank_crtc.hwmode, > * which is filled out by calling drm_calc_timestamping_constants(). > */ > struct drm_display_mode hwmode; > diff --git a/include/drm/drm_modeset_helper_vtables.h b/include/drm/drm_modeset_helper_vtables.h > index e398512bfd5f..0afaf58da40d 100644 > --- a/include/drm/drm_modeset_helper_vtables.h > +++ b/include/drm/drm_modeset_helper_vtables.h > @@ -459,8 +459,8 @@ struct drm_crtc_helper_funcs { > * Returns the current display scanout position from a CRTC and an > * optional accurate ktime_get() timestamp of when the position was > * measured. Note that this is a helper callback which is only used > - * if a driver uses drm_calc_vbltimestamp_from_scanoutpos() for the > - * @drm_driver.get_vblank_timestamp callback. > + * if a driver uses drm_crtc_vblank_helper_get_vblank_timestamp() > + * for the @drm_crtc_funcs.get_vblank_timestamp callback. > * > * Parameters: > * > diff --git a/include/drm/drm_vblank.h b/include/drm/drm_vblank.h > index c16c44052b3d..248fbd5de177 100644 > --- a/include/drm/drm_vblank.h > +++ b/include/drm/drm_vblank.h > @@ -174,13 +174,13 @@ struct drm_vblank_crtc { > unsigned int pipe; > /** > * @framedur_ns: Frame/Field duration in ns, used by > - * drm_calc_vbltimestamp_from_scanoutpos() and computed by > + * drm_crtc_vblank_helper_get_vblank_timestamp() and computed by > * drm_calc_timestamping_constants(). > */ > int framedur_ns; > /** > * @linedur_ns: Line duration in ns, used by > - * drm_calc_vbltimestamp_from_scanoutpos() and computed by > + * drm_crtc_vblank_helper_get_vblank_timestamp() and computed by > * drm_calc_timestamping_constants(). > */ > int linedur_ns; > @@ -190,8 +190,8 @@ struct drm_vblank_crtc { > * > * Cache of the current hardware display mode. Only valid when @enabled > * is set. This is used by helpers like > - * drm_calc_vbltimestamp_from_scanoutpos(). We can't just access the > - * hardware mode by e.g. looking at &drm_crtc_state.adjusted_mode, > + * drm_crtc_vblank_helper_get_vblank_timestamp(). We can't just access > + * the hardware mode by e.g. looking at &drm_crtc_state.adjusted_mode, > * because that one is really hard to get from interrupt context. > */ > struct drm_display_mode hwmode; > @@ -238,4 +238,22 @@ void drm_calc_timestamping_constants(struct drm_crtc *crtc, > wait_queue_head_t *drm_crtc_vblank_waitqueue(struct drm_crtc *crtc); > void drm_crtc_set_max_vblank_count(struct drm_crtc *crtc, > u32 max_vblank_count); > + > +/* > + * Helpers for struct drm_crtc_funcs > + */ > + > +bool > +drm_crtc_vblank_helper_get_vblank_timestamp_internal( > + struct drm_crtc *crtc, int *max_error, ktime_t *vblank_time, > + bool in_vblank_irq, > + bool (*get_scanout_position)(struct drm_crtc *crtc, > + bool in_vblank_irq, int *vpos, int *hpos, > + ktime_t *stime, ktime_t *etime, > + const struct drm_display_mode *mode)); Ugly alignment. Could maybe add a typedef for the function pointer if it otherwise gets super horrible with proper alignment. > +bool drm_crtc_vblank_helper_get_vblank_timestamp(struct drm_crtc *crtc, > + int *max_error, > + ktime_t *vblank_time, > + bool in_vblank_irq); > + > #endif > -- > 2.24.1 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Hi Am 15.01.20 um 15:49 schrieb Ville Syrjälä: > On Wed, Jan 15, 2020 at 01:16:34PM +0100, Thomas Zimmermann wrote: >> The callback get_vblank_timestamp() is currently located in struct >> drm_driver, but really belongs into struct drm_crtc_funcs. Add an >> equivalent there. Driver will be converted in separate patches. >> >> The default implementation is drm_calc_vbltimestamp_from_scanoutpos(). >> The patch adds drm_crtc_vblank_helper_get_vblank_timestamp(), which is >> an implementation for the CRTC callback. >> >> v2: >> * rename helper to drm_crtc_vblank_helper_get_vblank_timestamp() >> * replace drm_calc_vbltimestamp_from_scanoutpos() with >> drm_crtc_vblank_helper_get_vblank_timestamp() in docs >> >> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de> >> --- >> drivers/gpu/drm/drm_vblank.c | 236 +++++++++++++++++++++-- >> include/drm/drm_crtc.h | 46 ++++- >> include/drm/drm_modeset_helper_vtables.h | 4 +- >> include/drm/drm_vblank.h | 26 ++- >> 4 files changed, 292 insertions(+), 20 deletions(-) >> >> diff --git a/drivers/gpu/drm/drm_vblank.c b/drivers/gpu/drm/drm_vblank.c >> index abb085c67d82..1b7af316e6a5 100644 >> --- a/drivers/gpu/drm/drm_vblank.c >> +++ b/drivers/gpu/drm/drm_vblank.c >> @@ -333,7 +333,9 @@ u64 drm_crtc_accurate_vblank_count(struct drm_crtc *crtc) >> u64 vblank; >> unsigned long flags; >> >> - WARN_ONCE(drm_debug_enabled(DRM_UT_VBL) && !dev->driver->get_vblank_timestamp, >> + WARN_ONCE(drm_debug_enabled(DRM_UT_VBL) && >> + !crtc->funcs->get_vblank_timestamp && >> + !dev->driver->get_vblank_timestamp, >> "This function requires support for accurate vblank timestamps."); >> >> spin_lock_irqsave(&dev->vblank_time_lock, flags); >> @@ -511,9 +513,9 @@ EXPORT_SYMBOL(drm_crtc_vblank_waitqueue); >> * >> * Calculate and store various constants which are later needed by vblank and >> * swap-completion timestamping, e.g, by >> - * drm_calc_vbltimestamp_from_scanoutpos(). They are derived from CRTC's true >> - * scanout timing, so they take things like panel scaling or other adjustments >> - * into account. >> + * drm_crtc_vblank_helper_get_vblank_timestamp(). They are derived from >> + * CRTC's true scanout timing, so they take things like panel scaling or >> + * other adjustments into account. >> */ >> void drm_calc_timestamping_constants(struct drm_crtc *crtc, >> const struct drm_display_mode *mode) >> @@ -577,8 +579,9 @@ EXPORT_SYMBOL(drm_calc_timestamping_constants); >> * >> * Implements calculation of exact vblank timestamps from given drm_display_mode >> * timings and current video scanout position of a CRTC. This can be directly >> - * used as the &drm_driver.get_vblank_timestamp implementation of a kms driver >> - * if &drm_crtc_helper_funcs.get_scanout_position is implemented. >> + * used as the &drm_crtc_funcs.get_vblank_timestamp implementation of a kms >> + * driver if &drm_crtc_helper_funcs.get_scanout_position or >> + * &drm_driver.get_scanout_position is implemented. >> * >> * The current implementation only handles standard video modes. For double scan >> * and interlaced modes the driver is supposed to adjust the hardware mode >> @@ -742,15 +745,22 @@ static bool >> drm_get_last_vbltimestamp(struct drm_device *dev, unsigned int pipe, >> ktime_t *tvblank, bool in_vblank_irq) >> { >> + struct drm_crtc *crtc = drm_crtc_from_index(dev, pipe); >> bool ret = false; >> >> /* Define requested maximum error on timestamps (nanoseconds). */ >> int max_error = (int) drm_timestamp_precision * 1000; >> >> /* Query driver if possible and precision timestamping enabled. */ >> - if (dev->driver->get_vblank_timestamp && (max_error > 0)) >> + if (crtc->funcs->get_vblank_timestamp && (max_error > 0)) { > > Could drop the superfluous parens while at it. > > Are all drivers utilizing this guaranteed to have the crtc here? Good point. This is part of the suspend and resume code. I don't think it's possible to run this code without creating a CRTC. But it's also called from drm_legacy_vblank_post_modeset(), which does not have CRTCs. Legacy drivers don't implement get_vblank_timestamp() at all. I think we can skip this entire block for legacy drivers. > >> + struct drm_crtc *crtc = drm_crtc_from_index(dev, pipe); >> + >> + ret = crtc->funcs->get_vblank_timestamp(crtc, &max_error, >> + tvblank, in_vblank_irq); >> + } else if (dev->driver->get_vblank_timestamp && (max_error > 0)) { >> ret = dev->driver->get_vblank_timestamp(dev, pipe, &max_error, >> tvblank, in_vblank_irq); >> + } >> >> /* GPU high precision timestamp query unsupported or failed. >> * Return current monotonic/gettimeofday timestamp as best estimate. >> @@ -1059,11 +1069,19 @@ EXPORT_SYMBOL(drm_crtc_vblank_get); >> >> static bool __vblank_disable_immediate(struct drm_device *dev, unsigned int pipe) >> { >> + struct drm_crtc *crtc; >> + >> if (!dev->vblank_disable_immediate) >> return false; >> - if (!dev->driver->get_vblank_timestamp) >> - return false; >> - return true; >> + >> + crtc = drm_crtc_from_index(dev, pipe); >> + if (crtc && crtc->funcs->get_vblank_timestamp) > > Here you are expecting that the crtc might be NULL. Because of legacy drivers. > >> + return true; >> + >> + if (dev->driver->get_vblank_timestamp) >> + return true; >> + >> + return false; >> } >> >> static void drm_vblank_put(struct drm_device *dev, unsigned int pipe) >> @@ -1742,9 +1760,11 @@ int drm_wait_vblank_ioctl(struct drm_device *dev, void *data, >> >> static void drm_handle_vblank_events(struct drm_device *dev, unsigned int pipe) >> { >> + struct drm_crtc *crtc = drm_crtc_from_index(dev, pipe); >> struct drm_pending_vblank_event *e, *t; >> ktime_t now; >> u64 seq; >> + bool high_prec; >> >> assert_spin_locked(&dev->event_lock); >> >> @@ -1764,8 +1784,10 @@ static void drm_handle_vblank_events(struct drm_device *dev, unsigned int pipe) >> send_vblank_event(dev, e, seq, now); >> } >> >> - trace_drm_vblank_event(pipe, seq, now, >> - dev->driver->get_vblank_timestamp != NULL); >> + high_prec = crtc->funcs->get_vblank_timestamp || >> + dev->driver->get_vblank_timestamp; > > Here again assuming the crtc can't be NULL. There should probably be a check to handle legacy devices correctly. > >> + >> + trace_drm_vblank_event(pipe, seq, now, high_prec); >> } >> >> /** >> @@ -2020,3 +2042,193 @@ int drm_crtc_queue_sequence_ioctl(struct drm_device *dev, void *data, >> kfree(e); >> return ret; >> } >> + >> +/* >> + * Helpers for struct drm_crtc_funcs >> + */ >> + >> +/** >> + * drm_crtc_vblank_helper_get_vblank_timestamp_internal - precise vblank >> + * timestamp helper >> + * @dev: DRM device >> + * @pipe: index of CRTC whose vblank timestamp to retrieve >> + * @max_error: Desired maximum allowable error in timestamps (nanosecs) >> + * On return contains true maximum error of timestamp >> + * @vblank_time: Pointer to time which should receive the timestamp >> + * @in_vblank_irq: >> + * True when called from drm_crtc_handle_vblank(). Some drivers >> + * need to apply some workarounds for gpu-specific vblank irq quirks >> + * if flag is set. >> + * @get_scanout_position: >> + * Callback function to retrieve the scanout position. See >> + * @struct drm_crtc_helper_funcs.get_scanout_position. >> + * >> + * Implements calculation of exact vblank timestamps from given drm_display_mode >> + * timings and current video scanout position of a CRTC. >> + * >> + * The current implementation only handles standard video modes. For double scan >> + * and interlaced modes the driver is supposed to adjust the hardware mode >> + * (taken from &drm_crtc_state.adjusted mode for atomic modeset drivers) to >> + * match the scanout position reported. >> + * >> + * Note that atomic drivers must call drm_calc_timestamping_constants() before >> + * enabling a CRTC. The atomic helpers already take care of that in >> + * drm_atomic_helper_update_legacy_modeset_state(). >> + * >> + * Returns: >> + * >> + * Returns true on success, and false on failure, i.e. when no accurate >> + * timestamp could be acquired. >> + */ >> +bool >> +drm_crtc_vblank_helper_get_vblank_timestamp_internal( >> + struct drm_crtc *crtc, int *max_error, ktime_t *vblank_time, >> + bool in_vblank_irq, >> + bool (*get_scanout_position)(struct drm_crtc *crtc, >> + bool in_vblank_irq, int *vpos, int *hpos, >> + ktime_t *stime, ktime_t *etime, >> + const struct drm_display_mode *mode)) >> +{ >> + struct drm_device *dev = crtc->dev; >> + unsigned int pipe = crtc->index; >> + struct drm_vblank_crtc *vblank = &dev->vblank[pipe]; >> + struct timespec64 ts_etime, ts_vblank_time; >> + ktime_t stime, etime; >> + bool vbl_status; >> + const struct drm_display_mode *mode; >> + int vpos, hpos, i; >> + int delta_ns, duration_ns; >> + >> + if (pipe >= dev->num_crtcs) { >> + DRM_ERROR("Invalid crtc %u\n", pipe); >> + return false; >> + } >> + >> + /* Scanout position query not supported? Should not happen. */ >> + if (!get_scanout_position) { >> + DRM_ERROR("Called from CRTC w/o get_scanout_position()!?\n"); >> + return false; >> + } >> + >> + if (drm_drv_uses_atomic_modeset(dev)) >> + mode = &vblank->hwmode; >> + else >> + mode = &crtc->hwmode; >> + >> + /* If mode timing undefined, just return as no-op: >> + * Happens during initial modesetting of a crtc. >> + */ >> + if (mode->crtc_clock == 0) { >> + DRM_DEBUG("crtc %u: Noop due to uninitialized mode.\n", pipe); >> + WARN_ON_ONCE(drm_drv_uses_atomic_modeset(dev)); >> + return false; >> + } >> + >> + /* Get current scanout position with system timestamp. >> + * Repeat query up to DRM_TIMESTAMP_MAXRETRIES times >> + * if single query takes longer than max_error nanoseconds. >> + * >> + * This guarantees a tight bound on maximum error if >> + * code gets preempted or delayed for some reason. >> + */ >> + for (i = 0; i < DRM_TIMESTAMP_MAXRETRIES; i++) { >> + /* >> + * Get vertical and horizontal scanout position vpos, hpos, >> + * and bounding timestamps stime, etime, pre/post query. >> + */ >> + vbl_status = get_scanout_position(crtc, in_vblank_irq, &vpos, >> + &hpos, &stime, &etime, mode); >> + >> + /* Return as no-op if scanout query unsupported or failed. */ >> + if (!vbl_status) { >> + DRM_DEBUG("crtc %u : scanoutpos query failed.\n", >> + pipe); >> + return false; >> + } >> + >> + /* Compute uncertainty in timestamp of scanout position query. */ >> + duration_ns = ktime_to_ns(etime) - ktime_to_ns(stime); >> + >> + /* Accept result with < max_error nsecs timing uncertainty. */ >> + if (duration_ns <= *max_error) >> + break; >> + } >> + >> + /* Noisy system timing? */ >> + if (i == DRM_TIMESTAMP_MAXRETRIES) { >> + DRM_DEBUG("crtc %u: Noisy timestamp %d us > %d us [%d reps].\n", >> + pipe, duration_ns/1000, *max_error/1000, i); >> + } >> + >> + /* Return upper bound of timestamp precision error. */ >> + *max_error = duration_ns; >> + >> + /* Convert scanout position into elapsed time at raw_time query >> + * since start of scanout at first display scanline. delta_ns >> + * can be negative if start of scanout hasn't happened yet. >> + */ >> + delta_ns = div_s64(1000000LL * (vpos * mode->crtc_htotal + hpos), >> + mode->crtc_clock); >> + >> + /* Subtract time delta from raw timestamp to get final >> + * vblank_time timestamp for end of vblank. >> + */ >> + *vblank_time = ktime_sub_ns(etime, delta_ns); >> + >> + if (!drm_debug_enabled(DRM_UT_VBL)) >> + return true; >> + >> + ts_etime = ktime_to_timespec64(etime); >> + ts_vblank_time = ktime_to_timespec64(*vblank_time); >> + >> + DRM_DEBUG_VBL("crtc %u : v p(%d,%d)@ %lld.%06ld -> %lld.%06ld [e %d us, %d rep]\n", >> + pipe, hpos, vpos, >> + (u64)ts_etime.tv_sec, ts_etime.tv_nsec / 1000, >> + (u64)ts_vblank_time.tv_sec, ts_vblank_time.tv_nsec / 1000, >> + duration_ns / 1000, i); >> + >> + return true; >> +} >> +EXPORT_SYMBOL(drm_crtc_vblank_helper_get_vblank_timestamp_internal); > > This looks like copy paste from the current thing. Why are you > duplicating the entire function instead of refactoring what > we already have? It is copied with a small difference. The original function is drm_calc_vbltimestamp_from_scanoutpos(). It uses either drm_driver.get_scanout_position() or drm_crtc_helper_funcs.get_scanout_position(). The former receives device pointer and pipe index, the latter receives a pointer to a crtc. (see patch 1) This function, drm_crtc_vblank_helper_get_vblank_timestamp_internal(), receives a get_scanout_position() as its argument with the same signature as drm_crtc_helper_funcs.get_scanout_position(). The function is exported, so that i915 can use the algorithm and provide it's own implementation of get_scanout_position(). i915 does not use CRTC helpers, and therefore cannot set get_scanout_position() there. Patch 21, when all drivers have been converted to use the CRTC interface, removes drm_calc_vbltimestamp_from_scanoutpos() entirely. It's only kept around for transitionng the driver over to the new interface. > >> + >> +/** >> + * drm_crtc_vblank_helper_get_vblank_timestamp - precise vblank timestamp >> + * helper >> + * @crtc: CRTC whose vblank timestamp to retrieve >> + * @max_error: Desired maximum allowable error in timestamps (nanosecs) >> + * On return contains true maximum error of timestamp >> + * @vblank_time: Pointer to time which should receive the timestamp >> + * @in_vblank_irq: >> + * True when called from drm_crtc_handle_vblank(). Some drivers >> + * need to apply some workarounds for gpu-specific vblank irq quirks >> + * if flag is set. >> + * >> + * Implements calculation of exact vblank timestamps from given drm_display_mode >> + * timings and current video scanout position of a CRTC. This can be directly >> + * used as the &drm_crtc_funcs.get_vblank_timestamp implementation of a kms >> + * driver if &drm_crtc_helper_funcs.get_scanout_position is implemented. >> + * >> + * The current implementation only handles standard video modes. For double scan >> + * and interlaced modes the driver is supposed to adjust the hardware mode >> + * (taken from &drm_crtc_state.adjusted mode for atomic modeset drivers) to >> + * match the scanout position reported. >> + * >> + * Note that atomic drivers must call drm_calc_timestamping_constants() before >> + * enabling a CRTC. The atomic helpers already take care of that in >> + * drm_atomic_helper_update_legacy_modeset_state(). >> + * >> + * Returns: >> + * >> + * Returns true on success, and false on failure, i.e. when no accurate >> + * timestamp could be acquired. >> + */ >> +bool drm_crtc_vblank_helper_get_vblank_timestamp(struct drm_crtc *crtc, >> + int *max_error, >> + ktime_t *vblank_time, >> + bool in_vblank_irq) >> +{ >> + return drm_crtc_vblank_helper_get_vblank_timestamp_internal( >> + crtc, max_error, vblank_time, in_vblank_irq, >> + crtc->helper_private->get_scanout_position); >> +} >> +EXPORT_SYMBOL(drm_crtc_vblank_helper_get_vblank_timestamp); >> diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h >> index 5e9b15a0e8c5..db46abbbf4e7 100644 >> --- a/include/drm/drm_crtc.h >> +++ b/include/drm/drm_crtc.h >> @@ -867,6 +867,47 @@ struct drm_crtc_funcs { >> * new drivers as the replacement of &drm_driver.disable_vblank hook. >> */ >> void (*disable_vblank)(struct drm_crtc *crtc); >> + >> + /** >> + * @get_vblank_timestamp: >> + * >> + * Called by drm_get_last_vbltimestamp(). Should return a precise >> + * timestamp when the most recent vblank interval ended or will end. >> + * >> + * Specifically, the timestamp in @vblank_time should correspond as >> + * closely as possible to the time when the first video scanline of >> + * the video frame after the end of vblank will start scanning out, >> + * the time immediately after end of the vblank interval. If the >> + * @crtc is currently inside vblank, this will be a time in the future. >> + * If the @crtc is currently scanning out a frame, this will be the >> + * past start time of the current scanout. This is meant to adhere >> + * to the OpenML OML_sync_control extension specification. >> + * >> + * Parameters: >> + * >> + * crtc: >> + * CRTC for which timestamp should be returned. >> + * max_error: >> + * Maximum allowable timestamp error in nanoseconds. >> + * Implementation should strive to provide timestamp >> + * with an error of at most max_error nanoseconds. >> + * Returns true upper bound on error for timestamp. >> + * vblank_time: >> + * Target location for returned vblank timestamp. >> + * in_vblank_irq: >> + * True when called from drm_crtc_handle_vblank(). Some drivers >> + * need to apply some workarounds for gpu-specific vblank irq quirks >> + * if flag is set. >> + * >> + * Returns: >> + * >> + * True on success, false on failure, which means the core should >> + * fallback to a simple timestamp taken in drm_crtc_handle_vblank(). >> + */ >> + bool (*get_vblank_timestamp)(struct drm_crtc *crtc, >> + int *max_error, >> + ktime_t *vblank_time, >> + bool in_vblank_irq); >> }; >> >> /** >> @@ -974,11 +1015,12 @@ struct drm_crtc { >> * Programmed mode in hw, after adjustments for encoders, crtc, panel >> * scaling etc. Should only be used by legacy drivers, for high >> * precision vblank timestamps in >> - * drm_calc_vbltimestamp_from_scanoutpos(). >> + * drm_crtc_vblank_helper_get_vblank_timestamp(). >> * >> * Note that atomic drivers should not use this, but instead use >> * &drm_crtc_state.adjusted_mode. And for high-precision timestamps >> - * drm_calc_vbltimestamp_from_scanoutpos() used &drm_vblank_crtc.hwmode, >> + * drm_crtc_vblank_helper_get_vblank_timestamp() used >> + * &drm_vblank_crtc.hwmode, >> * which is filled out by calling drm_calc_timestamping_constants(). >> */ >> struct drm_display_mode hwmode; >> diff --git a/include/drm/drm_modeset_helper_vtables.h b/include/drm/drm_modeset_helper_vtables.h >> index e398512bfd5f..0afaf58da40d 100644 >> --- a/include/drm/drm_modeset_helper_vtables.h >> +++ b/include/drm/drm_modeset_helper_vtables.h >> @@ -459,8 +459,8 @@ struct drm_crtc_helper_funcs { >> * Returns the current display scanout position from a CRTC and an >> * optional accurate ktime_get() timestamp of when the position was >> * measured. Note that this is a helper callback which is only used >> - * if a driver uses drm_calc_vbltimestamp_from_scanoutpos() for the >> - * @drm_driver.get_vblank_timestamp callback. >> + * if a driver uses drm_crtc_vblank_helper_get_vblank_timestamp() >> + * for the @drm_crtc_funcs.get_vblank_timestamp callback. >> * >> * Parameters: >> * >> diff --git a/include/drm/drm_vblank.h b/include/drm/drm_vblank.h >> index c16c44052b3d..248fbd5de177 100644 >> --- a/include/drm/drm_vblank.h >> +++ b/include/drm/drm_vblank.h >> @@ -174,13 +174,13 @@ struct drm_vblank_crtc { >> unsigned int pipe; >> /** >> * @framedur_ns: Frame/Field duration in ns, used by >> - * drm_calc_vbltimestamp_from_scanoutpos() and computed by >> + * drm_crtc_vblank_helper_get_vblank_timestamp() and computed by >> * drm_calc_timestamping_constants(). >> */ >> int framedur_ns; >> /** >> * @linedur_ns: Line duration in ns, used by >> - * drm_calc_vbltimestamp_from_scanoutpos() and computed by >> + * drm_crtc_vblank_helper_get_vblank_timestamp() and computed by >> * drm_calc_timestamping_constants(). >> */ >> int linedur_ns; >> @@ -190,8 +190,8 @@ struct drm_vblank_crtc { >> * >> * Cache of the current hardware display mode. Only valid when @enabled >> * is set. This is used by helpers like >> - * drm_calc_vbltimestamp_from_scanoutpos(). We can't just access the >> - * hardware mode by e.g. looking at &drm_crtc_state.adjusted_mode, >> + * drm_crtc_vblank_helper_get_vblank_timestamp(). We can't just access >> + * the hardware mode by e.g. looking at &drm_crtc_state.adjusted_mode, >> * because that one is really hard to get from interrupt context. >> */ >> struct drm_display_mode hwmode; >> @@ -238,4 +238,22 @@ void drm_calc_timestamping_constants(struct drm_crtc *crtc, >> wait_queue_head_t *drm_crtc_vblank_waitqueue(struct drm_crtc *crtc); >> void drm_crtc_set_max_vblank_count(struct drm_crtc *crtc, >> u32 max_vblank_count); >> + >> +/* >> + * Helpers for struct drm_crtc_funcs >> + */ >> + >> +bool >> +drm_crtc_vblank_helper_get_vblank_timestamp_internal( >> + struct drm_crtc *crtc, int *max_error, ktime_t *vblank_time, >> + bool in_vblank_irq, >> + bool (*get_scanout_position)(struct drm_crtc *crtc, >> + bool in_vblank_irq, int *vpos, int *hpos, >> + ktime_t *stime, ktime_t *etime, >> + const struct drm_display_mode *mode)); > > Ugly alignment. Could maybe add a typedef for the function pointer if it > otherwise gets super horrible with proper alignment. Email formatting might add to the ugliness here. How would this be aligned properly? I'm slightly reluctent to add a typedef for only this single function and it's caller in i915. Typedefs are a form of code obfuscation IMHO. But if that's the way to go, I won't mind of course. Best regards Thomas > >> +bool drm_crtc_vblank_helper_get_vblank_timestamp(struct drm_crtc *crtc, >> + int *max_error, >> + ktime_t *vblank_time, >> + bool in_vblank_irq); >> + >> #endif >> -- >> 2.24.1 >> >> _______________________________________________ >> Intel-gfx mailing list >> Intel-gfx@lists.freedesktop.org >> https://lists.freedesktop.org/mailman/listinfo/intel-gfx >
On Thu, Jan 16, 2020 at 09:44:55AM +0100, Thomas Zimmermann wrote: > Hi > > Am 15.01.20 um 15:49 schrieb Ville Syrjälä: > > On Wed, Jan 15, 2020 at 01:16:34PM +0100, Thomas Zimmermann wrote: <snip> > >> @@ -2020,3 +2042,193 @@ int drm_crtc_queue_sequence_ioctl(struct drm_device *dev, void *data, > >> kfree(e); > >> return ret; > >> } > >> + > >> +/* > >> + * Helpers for struct drm_crtc_funcs > >> + */ > >> + > >> +/** > >> + * drm_crtc_vblank_helper_get_vblank_timestamp_internal - precise vblank > >> + * timestamp helper > >> + * @dev: DRM device > >> + * @pipe: index of CRTC whose vblank timestamp to retrieve > >> + * @max_error: Desired maximum allowable error in timestamps (nanosecs) > >> + * On return contains true maximum error of timestamp > >> + * @vblank_time: Pointer to time which should receive the timestamp > >> + * @in_vblank_irq: > >> + * True when called from drm_crtc_handle_vblank(). Some drivers > >> + * need to apply some workarounds for gpu-specific vblank irq quirks > >> + * if flag is set. > >> + * @get_scanout_position: > >> + * Callback function to retrieve the scanout position. See > >> + * @struct drm_crtc_helper_funcs.get_scanout_position. > >> + * > >> + * Implements calculation of exact vblank timestamps from given drm_display_mode > >> + * timings and current video scanout position of a CRTC. > >> + * > >> + * The current implementation only handles standard video modes. For double scan > >> + * and interlaced modes the driver is supposed to adjust the hardware mode > >> + * (taken from &drm_crtc_state.adjusted mode for atomic modeset drivers) to > >> + * match the scanout position reported. > >> + * > >> + * Note that atomic drivers must call drm_calc_timestamping_constants() before > >> + * enabling a CRTC. The atomic helpers already take care of that in > >> + * drm_atomic_helper_update_legacy_modeset_state(). > >> + * > >> + * Returns: > >> + * > >> + * Returns true on success, and false on failure, i.e. when no accurate > >> + * timestamp could be acquired. > >> + */ > >> +bool > >> +drm_crtc_vblank_helper_get_vblank_timestamp_internal( > >> + struct drm_crtc *crtc, int *max_error, ktime_t *vblank_time, > >> + bool in_vblank_irq, > >> + bool (*get_scanout_position)(struct drm_crtc *crtc, > >> + bool in_vblank_irq, int *vpos, int *hpos, > >> + ktime_t *stime, ktime_t *etime, > >> + const struct drm_display_mode *mode)) > >> +{ > >> + struct drm_device *dev = crtc->dev; > >> + unsigned int pipe = crtc->index; > >> + struct drm_vblank_crtc *vblank = &dev->vblank[pipe]; > >> + struct timespec64 ts_etime, ts_vblank_time; > >> + ktime_t stime, etime; > >> + bool vbl_status; > >> + const struct drm_display_mode *mode; > >> + int vpos, hpos, i; > >> + int delta_ns, duration_ns; > >> + > >> + if (pipe >= dev->num_crtcs) { > >> + DRM_ERROR("Invalid crtc %u\n", pipe); > >> + return false; > >> + } > >> + > >> + /* Scanout position query not supported? Should not happen. */ > >> + if (!get_scanout_position) { > >> + DRM_ERROR("Called from CRTC w/o get_scanout_position()!?\n"); > >> + return false; > >> + } > >> + > >> + if (drm_drv_uses_atomic_modeset(dev)) > >> + mode = &vblank->hwmode; > >> + else > >> + mode = &crtc->hwmode; > >> + > >> + /* If mode timing undefined, just return as no-op: > >> + * Happens during initial modesetting of a crtc. > >> + */ > >> + if (mode->crtc_clock == 0) { > >> + DRM_DEBUG("crtc %u: Noop due to uninitialized mode.\n", pipe); > >> + WARN_ON_ONCE(drm_drv_uses_atomic_modeset(dev)); > >> + return false; > >> + } > >> + > >> + /* Get current scanout position with system timestamp. > >> + * Repeat query up to DRM_TIMESTAMP_MAXRETRIES times > >> + * if single query takes longer than max_error nanoseconds. > >> + * > >> + * This guarantees a tight bound on maximum error if > >> + * code gets preempted or delayed for some reason. > >> + */ > >> + for (i = 0; i < DRM_TIMESTAMP_MAXRETRIES; i++) { > >> + /* > >> + * Get vertical and horizontal scanout position vpos, hpos, > >> + * and bounding timestamps stime, etime, pre/post query. > >> + */ > >> + vbl_status = get_scanout_position(crtc, in_vblank_irq, &vpos, > >> + &hpos, &stime, &etime, mode); > >> + > >> + /* Return as no-op if scanout query unsupported or failed. */ > >> + if (!vbl_status) { > >> + DRM_DEBUG("crtc %u : scanoutpos query failed.\n", > >> + pipe); > >> + return false; > >> + } > >> + > >> + /* Compute uncertainty in timestamp of scanout position query. */ > >> + duration_ns = ktime_to_ns(etime) - ktime_to_ns(stime); > >> + > >> + /* Accept result with < max_error nsecs timing uncertainty. */ > >> + if (duration_ns <= *max_error) > >> + break; > >> + } > >> + > >> + /* Noisy system timing? */ > >> + if (i == DRM_TIMESTAMP_MAXRETRIES) { > >> + DRM_DEBUG("crtc %u: Noisy timestamp %d us > %d us [%d reps].\n", > >> + pipe, duration_ns/1000, *max_error/1000, i); > >> + } > >> + > >> + /* Return upper bound of timestamp precision error. */ > >> + *max_error = duration_ns; > >> + > >> + /* Convert scanout position into elapsed time at raw_time query > >> + * since start of scanout at first display scanline. delta_ns > >> + * can be negative if start of scanout hasn't happened yet. > >> + */ > >> + delta_ns = div_s64(1000000LL * (vpos * mode->crtc_htotal + hpos), > >> + mode->crtc_clock); > >> + > >> + /* Subtract time delta from raw timestamp to get final > >> + * vblank_time timestamp for end of vblank. > >> + */ > >> + *vblank_time = ktime_sub_ns(etime, delta_ns); > >> + > >> + if (!drm_debug_enabled(DRM_UT_VBL)) > >> + return true; > >> + > >> + ts_etime = ktime_to_timespec64(etime); > >> + ts_vblank_time = ktime_to_timespec64(*vblank_time); > >> + > >> + DRM_DEBUG_VBL("crtc %u : v p(%d,%d)@ %lld.%06ld -> %lld.%06ld [e %d us, %d rep]\n", > >> + pipe, hpos, vpos, > >> + (u64)ts_etime.tv_sec, ts_etime.tv_nsec / 1000, > >> + (u64)ts_vblank_time.tv_sec, ts_vblank_time.tv_nsec / 1000, > >> + duration_ns / 1000, i); > >> + > >> + return true; > >> +} > >> +EXPORT_SYMBOL(drm_crtc_vblank_helper_get_vblank_timestamp_internal); > > > > This looks like copy paste from the current thing. Why are you > > duplicating the entire function instead of refactoring what > > we already have? > > It is copied with a small difference. > > The original function is drm_calc_vbltimestamp_from_scanoutpos(). It > uses either drm_driver.get_scanout_position() or > drm_crtc_helper_funcs.get_scanout_position(). The former receives device > pointer and pipe index, the latter receives a pointer to a crtc. (see > patch 1) > > This function, drm_crtc_vblank_helper_get_vblank_timestamp_internal(), > receives a get_scanout_position() as its argument with the same > signature as drm_crtc_helper_funcs.get_scanout_position(). > > The function is exported, so that i915 can use the algorithm and provide > it's own implementation of get_scanout_position(). i915 does not use > CRTC helpers, and therefore cannot set get_scanout_position() there. > > Patch 21, when all drivers have been converted to use the CRTC > interface, removes drm_calc_vbltimestamp_from_scanoutpos() entirely. > It's only kept around for transitionng the driver over to the new interface. All that means reviewing is going to be a bit of a pain. Also the history will become more annoying to dig through. Eg. git blame can't help us anymore, and when looking through git log one can't see the old name of the function from the patch context so need to either guess or trawl the logs blindly looking for a similar looking function. So would definitely appreciate if you can find a way to refactor this properly. <snip> > >> struct drm_display_mode hwmode; > >> @@ -238,4 +238,22 @@ void drm_calc_timestamping_constants(struct drm_crtc *crtc, > >> wait_queue_head_t *drm_crtc_vblank_waitqueue(struct drm_crtc *crtc); > >> void drm_crtc_set_max_vblank_count(struct drm_crtc *crtc, > >> u32 max_vblank_count); > >> + > >> +/* > >> + * Helpers for struct drm_crtc_funcs > >> + */ > >> + > >> +bool > >> +drm_crtc_vblank_helper_get_vblank_timestamp_internal( > >> + struct drm_crtc *crtc, int *max_error, ktime_t *vblank_time, > >> + bool in_vblank_irq, > >> + bool (*get_scanout_position)(struct drm_crtc *crtc, > >> + bool in_vblank_irq, int *vpos, int *hpos, > >> + ktime_t *stime, ktime_t *etime, > >> + const struct drm_display_mode *mode)); > > > > Ugly alignment. Could maybe add a typedef for the function pointer if it > > otherwise gets super horrible with proper alignment. > > Email formatting might add to the ugliness here. > > How would this be aligned properly? Function arguments should be aligned after the '('. > > I'm slightly reluctent to add a typedef for only this single function > and it's caller in i915. Typedefs are a form of code obfuscation IMHO. > But if that's the way to go, I won't mind of course. Yeah, I generally avoid them too. But I tend to have a few exceptions: a) when the funciton pointer has to be stuffed into multiple places b) exactly this case where the function signature is a horrible mess and it has to be passed to a function > > Best regards > Thomas > > > > >> +bool drm_crtc_vblank_helper_get_vblank_timestamp(struct drm_crtc *crtc, > >> + int *max_error, > >> + ktime_t *vblank_time, > >> + bool in_vblank_irq); > >> + > >> #endif > >> -- > >> 2.24.1 > >> > >> _______________________________________________ > >> Intel-gfx mailing list > >> Intel-gfx@lists.freedesktop.org > >> https://lists.freedesktop.org/mailman/listinfo/intel-gfx > > > > -- > Thomas Zimmermann > Graphics Driver Developer > SUSE Software Solutions Germany GmbH > Maxfeldstr. 5, 90409 Nürnberg, Germany > (HRB 36809, AG Nürnberg) > Geschäftsführer: Felix Imendörffer >
diff --git a/drivers/gpu/drm/drm_vblank.c b/drivers/gpu/drm/drm_vblank.c index abb085c67d82..1b7af316e6a5 100644 --- a/drivers/gpu/drm/drm_vblank.c +++ b/drivers/gpu/drm/drm_vblank.c @@ -333,7 +333,9 @@ u64 drm_crtc_accurate_vblank_count(struct drm_crtc *crtc) u64 vblank; unsigned long flags; - WARN_ONCE(drm_debug_enabled(DRM_UT_VBL) && !dev->driver->get_vblank_timestamp, + WARN_ONCE(drm_debug_enabled(DRM_UT_VBL) && + !crtc->funcs->get_vblank_timestamp && + !dev->driver->get_vblank_timestamp, "This function requires support for accurate vblank timestamps."); spin_lock_irqsave(&dev->vblank_time_lock, flags); @@ -511,9 +513,9 @@ EXPORT_SYMBOL(drm_crtc_vblank_waitqueue); * * Calculate and store various constants which are later needed by vblank and * swap-completion timestamping, e.g, by - * drm_calc_vbltimestamp_from_scanoutpos(). They are derived from CRTC's true - * scanout timing, so they take things like panel scaling or other adjustments - * into account. + * drm_crtc_vblank_helper_get_vblank_timestamp(). They are derived from + * CRTC's true scanout timing, so they take things like panel scaling or + * other adjustments into account. */ void drm_calc_timestamping_constants(struct drm_crtc *crtc, const struct drm_display_mode *mode) @@ -577,8 +579,9 @@ EXPORT_SYMBOL(drm_calc_timestamping_constants); * * Implements calculation of exact vblank timestamps from given drm_display_mode * timings and current video scanout position of a CRTC. This can be directly - * used as the &drm_driver.get_vblank_timestamp implementation of a kms driver - * if &drm_crtc_helper_funcs.get_scanout_position is implemented. + * used as the &drm_crtc_funcs.get_vblank_timestamp implementation of a kms + * driver if &drm_crtc_helper_funcs.get_scanout_position or + * &drm_driver.get_scanout_position is implemented. * * The current implementation only handles standard video modes. For double scan * and interlaced modes the driver is supposed to adjust the hardware mode @@ -742,15 +745,22 @@ static bool drm_get_last_vbltimestamp(struct drm_device *dev, unsigned int pipe, ktime_t *tvblank, bool in_vblank_irq) { + struct drm_crtc *crtc = drm_crtc_from_index(dev, pipe); bool ret = false; /* Define requested maximum error on timestamps (nanoseconds). */ int max_error = (int) drm_timestamp_precision * 1000; /* Query driver if possible and precision timestamping enabled. */ - if (dev->driver->get_vblank_timestamp && (max_error > 0)) + if (crtc->funcs->get_vblank_timestamp && (max_error > 0)) { + struct drm_crtc *crtc = drm_crtc_from_index(dev, pipe); + + ret = crtc->funcs->get_vblank_timestamp(crtc, &max_error, + tvblank, in_vblank_irq); + } else if (dev->driver->get_vblank_timestamp && (max_error > 0)) { ret = dev->driver->get_vblank_timestamp(dev, pipe, &max_error, tvblank, in_vblank_irq); + } /* GPU high precision timestamp query unsupported or failed. * Return current monotonic/gettimeofday timestamp as best estimate. @@ -1059,11 +1069,19 @@ EXPORT_SYMBOL(drm_crtc_vblank_get); static bool __vblank_disable_immediate(struct drm_device *dev, unsigned int pipe) { + struct drm_crtc *crtc; + if (!dev->vblank_disable_immediate) return false; - if (!dev->driver->get_vblank_timestamp) - return false; - return true; + + crtc = drm_crtc_from_index(dev, pipe); + if (crtc && crtc->funcs->get_vblank_timestamp) + return true; + + if (dev->driver->get_vblank_timestamp) + return true; + + return false; } static void drm_vblank_put(struct drm_device *dev, unsigned int pipe) @@ -1742,9 +1760,11 @@ int drm_wait_vblank_ioctl(struct drm_device *dev, void *data, static void drm_handle_vblank_events(struct drm_device *dev, unsigned int pipe) { + struct drm_crtc *crtc = drm_crtc_from_index(dev, pipe); struct drm_pending_vblank_event *e, *t; ktime_t now; u64 seq; + bool high_prec; assert_spin_locked(&dev->event_lock); @@ -1764,8 +1784,10 @@ static void drm_handle_vblank_events(struct drm_device *dev, unsigned int pipe) send_vblank_event(dev, e, seq, now); } - trace_drm_vblank_event(pipe, seq, now, - dev->driver->get_vblank_timestamp != NULL); + high_prec = crtc->funcs->get_vblank_timestamp || + dev->driver->get_vblank_timestamp; + + trace_drm_vblank_event(pipe, seq, now, high_prec); } /** @@ -2020,3 +2042,193 @@ int drm_crtc_queue_sequence_ioctl(struct drm_device *dev, void *data, kfree(e); return ret; } + +/* + * Helpers for struct drm_crtc_funcs + */ + +/** + * drm_crtc_vblank_helper_get_vblank_timestamp_internal - precise vblank + * timestamp helper + * @dev: DRM device + * @pipe: index of CRTC whose vblank timestamp to retrieve + * @max_error: Desired maximum allowable error in timestamps (nanosecs) + * On return contains true maximum error of timestamp + * @vblank_time: Pointer to time which should receive the timestamp + * @in_vblank_irq: + * True when called from drm_crtc_handle_vblank(). Some drivers + * need to apply some workarounds for gpu-specific vblank irq quirks + * if flag is set. + * @get_scanout_position: + * Callback function to retrieve the scanout position. See + * @struct drm_crtc_helper_funcs.get_scanout_position. + * + * Implements calculation of exact vblank timestamps from given drm_display_mode + * timings and current video scanout position of a CRTC. + * + * The current implementation only handles standard video modes. For double scan + * and interlaced modes the driver is supposed to adjust the hardware mode + * (taken from &drm_crtc_state.adjusted mode for atomic modeset drivers) to + * match the scanout position reported. + * + * Note that atomic drivers must call drm_calc_timestamping_constants() before + * enabling a CRTC. The atomic helpers already take care of that in + * drm_atomic_helper_update_legacy_modeset_state(). + * + * Returns: + * + * Returns true on success, and false on failure, i.e. when no accurate + * timestamp could be acquired. + */ +bool +drm_crtc_vblank_helper_get_vblank_timestamp_internal( + struct drm_crtc *crtc, int *max_error, ktime_t *vblank_time, + bool in_vblank_irq, + bool (*get_scanout_position)(struct drm_crtc *crtc, + bool in_vblank_irq, int *vpos, int *hpos, + ktime_t *stime, ktime_t *etime, + const struct drm_display_mode *mode)) +{ + struct drm_device *dev = crtc->dev; + unsigned int pipe = crtc->index; + struct drm_vblank_crtc *vblank = &dev->vblank[pipe]; + struct timespec64 ts_etime, ts_vblank_time; + ktime_t stime, etime; + bool vbl_status; + const struct drm_display_mode *mode; + int vpos, hpos, i; + int delta_ns, duration_ns; + + if (pipe >= dev->num_crtcs) { + DRM_ERROR("Invalid crtc %u\n", pipe); + return false; + } + + /* Scanout position query not supported? Should not happen. */ + if (!get_scanout_position) { + DRM_ERROR("Called from CRTC w/o get_scanout_position()!?\n"); + return false; + } + + if (drm_drv_uses_atomic_modeset(dev)) + mode = &vblank->hwmode; + else + mode = &crtc->hwmode; + + /* If mode timing undefined, just return as no-op: + * Happens during initial modesetting of a crtc. + */ + if (mode->crtc_clock == 0) { + DRM_DEBUG("crtc %u: Noop due to uninitialized mode.\n", pipe); + WARN_ON_ONCE(drm_drv_uses_atomic_modeset(dev)); + return false; + } + + /* Get current scanout position with system timestamp. + * Repeat query up to DRM_TIMESTAMP_MAXRETRIES times + * if single query takes longer than max_error nanoseconds. + * + * This guarantees a tight bound on maximum error if + * code gets preempted or delayed for some reason. + */ + for (i = 0; i < DRM_TIMESTAMP_MAXRETRIES; i++) { + /* + * Get vertical and horizontal scanout position vpos, hpos, + * and bounding timestamps stime, etime, pre/post query. + */ + vbl_status = get_scanout_position(crtc, in_vblank_irq, &vpos, + &hpos, &stime, &etime, mode); + + /* Return as no-op if scanout query unsupported or failed. */ + if (!vbl_status) { + DRM_DEBUG("crtc %u : scanoutpos query failed.\n", + pipe); + return false; + } + + /* Compute uncertainty in timestamp of scanout position query. */ + duration_ns = ktime_to_ns(etime) - ktime_to_ns(stime); + + /* Accept result with < max_error nsecs timing uncertainty. */ + if (duration_ns <= *max_error) + break; + } + + /* Noisy system timing? */ + if (i == DRM_TIMESTAMP_MAXRETRIES) { + DRM_DEBUG("crtc %u: Noisy timestamp %d us > %d us [%d reps].\n", + pipe, duration_ns/1000, *max_error/1000, i); + } + + /* Return upper bound of timestamp precision error. */ + *max_error = duration_ns; + + /* Convert scanout position into elapsed time at raw_time query + * since start of scanout at first display scanline. delta_ns + * can be negative if start of scanout hasn't happened yet. + */ + delta_ns = div_s64(1000000LL * (vpos * mode->crtc_htotal + hpos), + mode->crtc_clock); + + /* Subtract time delta from raw timestamp to get final + * vblank_time timestamp for end of vblank. + */ + *vblank_time = ktime_sub_ns(etime, delta_ns); + + if (!drm_debug_enabled(DRM_UT_VBL)) + return true; + + ts_etime = ktime_to_timespec64(etime); + ts_vblank_time = ktime_to_timespec64(*vblank_time); + + DRM_DEBUG_VBL("crtc %u : v p(%d,%d)@ %lld.%06ld -> %lld.%06ld [e %d us, %d rep]\n", + pipe, hpos, vpos, + (u64)ts_etime.tv_sec, ts_etime.tv_nsec / 1000, + (u64)ts_vblank_time.tv_sec, ts_vblank_time.tv_nsec / 1000, + duration_ns / 1000, i); + + return true; +} +EXPORT_SYMBOL(drm_crtc_vblank_helper_get_vblank_timestamp_internal); + +/** + * drm_crtc_vblank_helper_get_vblank_timestamp - precise vblank timestamp + * helper + * @crtc: CRTC whose vblank timestamp to retrieve + * @max_error: Desired maximum allowable error in timestamps (nanosecs) + * On return contains true maximum error of timestamp + * @vblank_time: Pointer to time which should receive the timestamp + * @in_vblank_irq: + * True when called from drm_crtc_handle_vblank(). Some drivers + * need to apply some workarounds for gpu-specific vblank irq quirks + * if flag is set. + * + * Implements calculation of exact vblank timestamps from given drm_display_mode + * timings and current video scanout position of a CRTC. This can be directly + * used as the &drm_crtc_funcs.get_vblank_timestamp implementation of a kms + * driver if &drm_crtc_helper_funcs.get_scanout_position is implemented. + * + * The current implementation only handles standard video modes. For double scan + * and interlaced modes the driver is supposed to adjust the hardware mode + * (taken from &drm_crtc_state.adjusted mode for atomic modeset drivers) to + * match the scanout position reported. + * + * Note that atomic drivers must call drm_calc_timestamping_constants() before + * enabling a CRTC. The atomic helpers already take care of that in + * drm_atomic_helper_update_legacy_modeset_state(). + * + * Returns: + * + * Returns true on success, and false on failure, i.e. when no accurate + * timestamp could be acquired. + */ +bool drm_crtc_vblank_helper_get_vblank_timestamp(struct drm_crtc *crtc, + int *max_error, + ktime_t *vblank_time, + bool in_vblank_irq) +{ + return drm_crtc_vblank_helper_get_vblank_timestamp_internal( + crtc, max_error, vblank_time, in_vblank_irq, + crtc->helper_private->get_scanout_position); +} +EXPORT_SYMBOL(drm_crtc_vblank_helper_get_vblank_timestamp); diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h index 5e9b15a0e8c5..db46abbbf4e7 100644 --- a/include/drm/drm_crtc.h +++ b/include/drm/drm_crtc.h @@ -867,6 +867,47 @@ struct drm_crtc_funcs { * new drivers as the replacement of &drm_driver.disable_vblank hook. */ void (*disable_vblank)(struct drm_crtc *crtc); + + /** + * @get_vblank_timestamp: + * + * Called by drm_get_last_vbltimestamp(). Should return a precise + * timestamp when the most recent vblank interval ended or will end. + * + * Specifically, the timestamp in @vblank_time should correspond as + * closely as possible to the time when the first video scanline of + * the video frame after the end of vblank will start scanning out, + * the time immediately after end of the vblank interval. If the + * @crtc is currently inside vblank, this will be a time in the future. + * If the @crtc is currently scanning out a frame, this will be the + * past start time of the current scanout. This is meant to adhere + * to the OpenML OML_sync_control extension specification. + * + * Parameters: + * + * crtc: + * CRTC for which timestamp should be returned. + * max_error: + * Maximum allowable timestamp error in nanoseconds. + * Implementation should strive to provide timestamp + * with an error of at most max_error nanoseconds. + * Returns true upper bound on error for timestamp. + * vblank_time: + * Target location for returned vblank timestamp. + * in_vblank_irq: + * True when called from drm_crtc_handle_vblank(). Some drivers + * need to apply some workarounds for gpu-specific vblank irq quirks + * if flag is set. + * + * Returns: + * + * True on success, false on failure, which means the core should + * fallback to a simple timestamp taken in drm_crtc_handle_vblank(). + */ + bool (*get_vblank_timestamp)(struct drm_crtc *crtc, + int *max_error, + ktime_t *vblank_time, + bool in_vblank_irq); }; /** @@ -974,11 +1015,12 @@ struct drm_crtc { * Programmed mode in hw, after adjustments for encoders, crtc, panel * scaling etc. Should only be used by legacy drivers, for high * precision vblank timestamps in - * drm_calc_vbltimestamp_from_scanoutpos(). + * drm_crtc_vblank_helper_get_vblank_timestamp(). * * Note that atomic drivers should not use this, but instead use * &drm_crtc_state.adjusted_mode. And for high-precision timestamps - * drm_calc_vbltimestamp_from_scanoutpos() used &drm_vblank_crtc.hwmode, + * drm_crtc_vblank_helper_get_vblank_timestamp() used + * &drm_vblank_crtc.hwmode, * which is filled out by calling drm_calc_timestamping_constants(). */ struct drm_display_mode hwmode; diff --git a/include/drm/drm_modeset_helper_vtables.h b/include/drm/drm_modeset_helper_vtables.h index e398512bfd5f..0afaf58da40d 100644 --- a/include/drm/drm_modeset_helper_vtables.h +++ b/include/drm/drm_modeset_helper_vtables.h @@ -459,8 +459,8 @@ struct drm_crtc_helper_funcs { * Returns the current display scanout position from a CRTC and an * optional accurate ktime_get() timestamp of when the position was * measured. Note that this is a helper callback which is only used - * if a driver uses drm_calc_vbltimestamp_from_scanoutpos() for the - * @drm_driver.get_vblank_timestamp callback. + * if a driver uses drm_crtc_vblank_helper_get_vblank_timestamp() + * for the @drm_crtc_funcs.get_vblank_timestamp callback. * * Parameters: * diff --git a/include/drm/drm_vblank.h b/include/drm/drm_vblank.h index c16c44052b3d..248fbd5de177 100644 --- a/include/drm/drm_vblank.h +++ b/include/drm/drm_vblank.h @@ -174,13 +174,13 @@ struct drm_vblank_crtc { unsigned int pipe; /** * @framedur_ns: Frame/Field duration in ns, used by - * drm_calc_vbltimestamp_from_scanoutpos() and computed by + * drm_crtc_vblank_helper_get_vblank_timestamp() and computed by * drm_calc_timestamping_constants(). */ int framedur_ns; /** * @linedur_ns: Line duration in ns, used by - * drm_calc_vbltimestamp_from_scanoutpos() and computed by + * drm_crtc_vblank_helper_get_vblank_timestamp() and computed by * drm_calc_timestamping_constants(). */ int linedur_ns; @@ -190,8 +190,8 @@ struct drm_vblank_crtc { * * Cache of the current hardware display mode. Only valid when @enabled * is set. This is used by helpers like - * drm_calc_vbltimestamp_from_scanoutpos(). We can't just access the - * hardware mode by e.g. looking at &drm_crtc_state.adjusted_mode, + * drm_crtc_vblank_helper_get_vblank_timestamp(). We can't just access + * the hardware mode by e.g. looking at &drm_crtc_state.adjusted_mode, * because that one is really hard to get from interrupt context. */ struct drm_display_mode hwmode; @@ -238,4 +238,22 @@ void drm_calc_timestamping_constants(struct drm_crtc *crtc, wait_queue_head_t *drm_crtc_vblank_waitqueue(struct drm_crtc *crtc); void drm_crtc_set_max_vblank_count(struct drm_crtc *crtc, u32 max_vblank_count); + +/* + * Helpers for struct drm_crtc_funcs + */ + +bool +drm_crtc_vblank_helper_get_vblank_timestamp_internal( + struct drm_crtc *crtc, int *max_error, ktime_t *vblank_time, + bool in_vblank_irq, + bool (*get_scanout_position)(struct drm_crtc *crtc, + bool in_vblank_irq, int *vpos, int *hpos, + ktime_t *stime, ktime_t *etime, + const struct drm_display_mode *mode)); +bool drm_crtc_vblank_helper_get_vblank_timestamp(struct drm_crtc *crtc, + int *max_error, + ktime_t *vblank_time, + bool in_vblank_irq); + #endif
The callback get_vblank_timestamp() is currently located in struct drm_driver, but really belongs into struct drm_crtc_funcs. Add an equivalent there. Driver will be converted in separate patches. The default implementation is drm_calc_vbltimestamp_from_scanoutpos(). The patch adds drm_crtc_vblank_helper_get_vblank_timestamp(), which is an implementation for the CRTC callback. v2: * rename helper to drm_crtc_vblank_helper_get_vblank_timestamp() * replace drm_calc_vbltimestamp_from_scanoutpos() with drm_crtc_vblank_helper_get_vblank_timestamp() in docs Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de> --- drivers/gpu/drm/drm_vblank.c | 236 +++++++++++++++++++++-- include/drm/drm_crtc.h | 46 ++++- include/drm/drm_modeset_helper_vtables.h | 4 +- include/drm/drm_vblank.h | 26 ++- 4 files changed, 292 insertions(+), 20 deletions(-)