On Thu, Jan 23, 2020 at 2:59 PM Thomas Zimmermann <tzimmermann@xxxxxxx> wrote:
>
> The new callback get_scanout_position() reads the current location
> of the scanout process. The operation is currently located in struct
> drm_driver, but really belongs to the CRTC. Drivers will be converted
> in separate patches.
>
> To help with the conversion, the timestamp calculation has been
> moved from drm_calc_vbltimestamp_from_scanoutpos() to
> drm_crtc_vblank_helper_get_vblank_timestamp_internal(). The helper
> function supports the new and old interface of get_scanout_position().
> drm_calc_vbltimestamp_from_scanoutpos() remains as a wrapper around
> the new function.
>
> Callback functions return the scanout position from the CRTC. The
> legacy version of the interface receives the device and pipe index,
> the modern version receives a pointer to the CRTC. We keep the
> legacy version until all drivers have been converted.
>
> v4:
> * 80-character line fixes
> v3:
> * refactor drm_calc_vbltimestamp_from_scanoutpos() to minimize
> code duplication
> * define types for get_scanout_position() callbacks
> v2:
> * fix logical op in drm_calc_vbltimestamp_from_scanoutpos()
>
> Signed-off-by: Thomas Zimmermann <tzimmermann@xxxxxxx>
> Tested-by: Yannick Fertré <yannick.fertre@xxxxxx>
> Reviewed-by: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
This patch causes new kerneldoc build warnings:
./drivers/gpu/drm/drm_vblank.c:623: warning: Excess function parameter
'dev' description in
'drm_crtc_vblank_helper_get_vblank_timestamp_internal'
./drivers/gpu/drm/drm_vblank.c:623: warning: Excess function parameter
'pipe' description in
'drm_crtc_vblank_helper_get_vblank_timestamp_internal'
./drivers/gpu/drm/drm_vblank.c:624: warning: Function parameter or
member 'crtc' not described in
'drm_crtc_vblank_helper_get_vblank_timestamp_internal'
./drivers/gpu/drm/drm_vblank.c:624: warning: Excess function parameter
'dev' description in
'drm_crtc_vblank_helper_get_vblank_timestamp_internal'
./drivers/gpu/drm/drm_vblank.c:624: warning: Excess function parameter
'pipe' description in
'drm_crtc_vblank_helper_get_vblank_timestamp_internal'
Please fix.
-Daniel
> ---
> drivers/gpu/drm/drm_vblank.c | 101 +++++++++++++++++++----
> include/drm/drm_drv.h | 7 +-
> include/drm/drm_modeset_helper_vtables.h | 47 +++++++++++
> include/drm/drm_vblank.h | 25 ++++++
> 4 files changed, 157 insertions(+), 23 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_vblank.c b/drivers/gpu/drm/drm_vblank.c
> index 326db52f2ad8..7e962c29780c 100644
> --- a/drivers/gpu/drm/drm_vblank.c
> +++ b/drivers/gpu/drm/drm_vblank.c
> @@ -30,6 +30,7 @@
> #include <drm/drm_crtc.h>
> #include <drm/drm_drv.h>
> #include <drm/drm_framebuffer.h>
> +#include <drm/drm_modeset_helper_vtables.h>
> #include <drm/drm_print.h>
> #include <drm/drm_vblank.h>
>
> @@ -577,7 +578,7 @@ 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_driver.get_scanout_position is implemented.
> + * 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
> @@ -599,28 +600,85 @@ bool drm_calc_vbltimestamp_from_scanoutpos(struct drm_device *dev,
> ktime_t *vblank_time,
> bool in_vblank_irq)
> {
> - struct timespec64 ts_etime, ts_vblank_time;
> - ktime_t stime, etime;
> - bool vbl_status;
> struct drm_crtc *crtc;
> - const struct drm_display_mode *mode;
> - struct drm_vblank_crtc *vblank = &dev->vblank[pipe];
> - int vpos, hpos, i;
> - int delta_ns, duration_ns;
>
> if (!drm_core_check_feature(dev, DRIVER_MODESET))
> return false;
>
> crtc = drm_crtc_from_index(dev, pipe);
> + if (!crtc)
> + return false;
>
> - if (pipe >= dev->num_crtcs || !crtc) {
> + return drm_crtc_vblank_helper_get_vblank_timestamp_internal(crtc,
> + max_error,
> + vblank_time,
> + in_vblank_irq,
> + crtc->helper_private->get_scanout_position,
> + dev->driver->get_scanout_position);
> +}
> +EXPORT_SYMBOL(drm_calc_vbltimestamp_from_scanoutpos);
> +
> +/**
> + * 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.
> + * @get_scanout_position_legacy:
> + * Callback function to retrieve the scanout position. See
> + * @struct drm_driver.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,
> + drm_vblank_get_scanout_position_func get_scanout_position,
> + drm_vblank_get_scanout_position_legacy_func get_scanout_position_legacy)
> +{
> + 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 (!dev->driver->get_scanout_position) {
> - DRM_ERROR("Called from driver w/o get_scanout_position()!?\n");
> + if (!get_scanout_position && !get_scanout_position_legacy) {
> + DRM_ERROR("Called from CRTC w/o get_scanout_position()!?\n");
> return false;
> }
>
> @@ -635,7 +693,6 @@ bool drm_calc_vbltimestamp_from_scanoutpos(struct drm_device *dev,
> 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;
> }
>
> @@ -651,11 +708,19 @@ bool drm_calc_vbltimestamp_from_scanoutpos(struct drm_device *dev,
> * Get vertical and horizontal scanout position vpos, hpos,
> * and bounding timestamps stime, etime, pre/post query.
> */
> - vbl_status = dev->driver->get_scanout_position(dev, pipe,
> - in_vblank_irq,
> - &vpos, &hpos,
> - &stime, &etime,
> - mode);
> + if (get_scanout_position) {
> + vbl_status = get_scanout_position(crtc,
> + in_vblank_irq,
> + &vpos, &hpos,
> + &stime, &etime,
> + mode);
> + } else {
> + vbl_status = get_scanout_position_legacy(dev, pipe,
> + in_vblank_irq,
> + &vpos, &hpos,
> + &stime, &etime,
> + mode);
> + }
>
> /* Return as no-op if scanout query unsupported or failed. */
> if (!vbl_status) {
> @@ -707,7 +772,7 @@ bool drm_calc_vbltimestamp_from_scanoutpos(struct drm_device *dev,
>
> return true;
> }
> -EXPORT_SYMBOL(drm_calc_vbltimestamp_from_scanoutpos);
> +EXPORT_SYMBOL(drm_crtc_vblank_helper_get_vblank_timestamp_internal);
>
> /**
> * drm_get_last_vbltimestamp - retrieve raw timestamp for the most recent
> diff --git a/include/drm/drm_drv.h b/include/drm/drm_drv.h
> index cf13470810a5..d0049e5786fc 100644
> --- a/include/drm/drm_drv.h
> +++ b/include/drm/drm_drv.h
> @@ -362,11 +362,8 @@ struct drm_driver {
> * True on success, false if a reliable scanout position counter could
> * not be read out.
> *
> - * FIXME:
> - *
> - * Since this is a helper to implement @get_vblank_timestamp, we should
> - * move it to &struct drm_crtc_helper_funcs, like all the other
> - * helper-internal hooks.
> + * This is deprecated and should not be used by new drivers.
> + * Use &drm_crtc_helper_funcs.get_scanout_position instead.
> */
> bool (*get_scanout_position) (struct drm_device *dev, unsigned int pipe,
> bool in_vblank_irq, int *vpos, int *hpos,
> diff --git a/include/drm/drm_modeset_helper_vtables.h b/include/drm/drm_modeset_helper_vtables.h
> index 5a87f1bd7a3f..e398512bfd5f 100644
> --- a/include/drm/drm_modeset_helper_vtables.h
> +++ b/include/drm/drm_modeset_helper_vtables.h
> @@ -450,6 +450,53 @@ struct drm_crtc_helper_funcs {
> */
> void (*atomic_disable)(struct drm_crtc *crtc,
> struct drm_crtc_state *old_crtc_state);
> +
> + /**
> + * @get_scanout_position:
> + *
> + * Called by vblank timestamping code.
> + *
> + * 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.
> + *
> + * Parameters:
> + *
> + * crtc:
> + * The CRTC.
> + * 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 the flag is set.
> + * vpos:
> + * Target location for current vertical scanout position.
> + * hpos:
> + * Target location for current horizontal scanout position.
> + * stime:
> + * Target location for timestamp taken immediately before
> + * scanout position query. Can be NULL to skip timestamp.
> + * etime:
> + * Target location for timestamp taken immediately after
> + * scanout position query. Can be NULL to skip timestamp.
> + * mode:
> + * Current display timings.
> + *
> + * Returns vpos as a positive number while in active scanout area.
> + * Returns vpos as a negative number inside vblank, counting the number
> + * of scanlines to go until end of vblank, e.g., -1 means "one scanline
> + * until start of active scanout / end of vblank."
> + *
> + * Returns:
> + *
> + * True on success, false if a reliable scanout position counter could
> + * not be read out.
> + */
> + 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);
> };
>
> /**
> diff --git a/include/drm/drm_vblank.h b/include/drm/drm_vblank.h
> index c16c44052b3d..66d1fb376600 100644
> --- a/include/drm/drm_vblank.h
> +++ b/include/drm/drm_vblank.h
> @@ -238,4 +238,29 @@ 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);
> +
> +typedef bool (*drm_vblank_get_scanout_position_func)(struct drm_crtc *crtc,
> + bool in_vblank_irq,
> + int *vpos, int *hpos,
> + ktime_t *stime,
> + ktime_t *etime,
> + const struct drm_display_mode *mode);
> +
> +typedef bool (*drm_vblank_get_scanout_position_legacy_func)(struct drm_device *dev,
> + unsigned int pipe,
> + 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_internal(struct drm_crtc *crtc,
> + int *max_error,
> + ktime_t *vblank_time,
> + bool in_vblank_irq,
> + drm_vblank_get_scanout_position_func get_scanout_position,
> + drm_vblank_get_scanout_position_legacy_func get_scanout_position_legacy);
> +
> #endif
> --
> 2.24.1
>
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
amd-gfx mailing list
amd-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
