On Wed, Mar 22, 2017 at 09:36:13AM +0100, Daniel Vetter wrote:
> It's overkill to have a flag parameter which is essentially used just
> as a boolean. This takes care of core + adjusting drivers.
>
> Adjusting the scanout position callback is a bit harder, since radeon
> also supplies it's own driver-private flags in there.
This part worried me, but indeed radeon only passes the custom flag to
the scanout position hook. Patch lgtm
Reviewed-by: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
>
> Cc: Mario Kleiner <mario.kleiner@xxxxxxxxxxxxxxxx>
> Cc: Eric Anholt <eric@xxxxxxxxxx>
> Cc: Rob Clark <robdclark@xxxxxxxxx>
> Cc: linux-arm-msm@xxxxxxxxxxxxxxx
> Cc: freedreno@xxxxxxxxxxxxxxxxxxxxx
> Cc: Alex Deucher <alexander.deucher@xxxxxxx>
> Cc: Christian König <christian.koenig@xxxxxxx>
> Cc: Ben Skeggs <bskeggs@xxxxxxxxxx>
> Signed-off-by: Daniel Vetter <daniel.vetter@xxxxxxxxx>
> ---
> drivers/gpu/drm/amd/amdgpu/amdgpu.h | 2 +-
> drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c | 6 ++---
> drivers/gpu/drm/drm_irq.c | 41 +++++++++++++++++--------------
> drivers/gpu/drm/i915/i915_irq.c | 4 +--
> drivers/gpu/drm/msm/mdp/mdp5/mdp5_kms.c | 4 +--
> drivers/gpu/drm/nouveau/nouveau_display.c | 5 ++--
> drivers/gpu/drm/nouveau/nouveau_display.h | 2 +-
> drivers/gpu/drm/radeon/radeon_drv.c | 2 +-
> drivers/gpu/drm/radeon/radeon_kms.c | 4 +--
> drivers/gpu/drm/vc4/vc4_crtc.c | 4 +--
> drivers/gpu/drm/vc4/vc4_drv.h | 2 +-
> include/drm/drm_drv.h | 11 ++++-----
> include/drm/drm_irq.h | 2 +-
> 13 files changed, 46 insertions(+), 43 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> index acd8631d8024..edb3bb83e1a9 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> @@ -1771,7 +1771,7 @@ void amdgpu_disable_vblank_kms(struct drm_device *dev, unsigned int pipe);
> bool amdgpu_get_vblank_timestamp_kms(struct drm_device *dev, unsigned int pipe,
> int *max_error,
> struct timeval *vblank_time,
> - unsigned flags);
> + bool in_vblank_irq);
> long amdgpu_kms_compat_ioctl(struct file *filp, unsigned int cmd,
> unsigned long arg);
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
> index ac42f707c046..ad295e822d45 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
> @@ -834,7 +834,7 @@ void amdgpu_disable_vblank_kms(struct drm_device *dev, unsigned int pipe)
> * @crtc: crtc to get the timestamp for
> * @max_error: max error
> * @vblank_time: time value
> - * @flags: flags passed to the driver
> + * @in_vblank_irq: called from drm_handle_vblank()
> *
> * Gets the timestamp on the requested crtc based on the
> * scanout position. (all asics).
> @@ -843,7 +843,7 @@ void amdgpu_disable_vblank_kms(struct drm_device *dev, unsigned int pipe)
> bool amdgpu_get_vblank_timestamp_kms(struct drm_device *dev, unsigned int pipe,
> int *max_error,
> struct timeval *vblank_time,
> - unsigned flags)
> + bool in_vblank_irq)
> {
> struct drm_crtc *crtc;
> struct amdgpu_device *adev = dev->dev_private;
> @@ -864,7 +864,7 @@ bool amdgpu_get_vblank_timestamp_kms(struct drm_device *dev, unsigned int pipe,
>
> /* Helper routine in DRM core does all the work: */
> return drm_calc_vbltimestamp_from_scanoutpos(dev, pipe, max_error,
> - vblank_time, flags,
> + vblank_time, in_vblank_irq,
> &crtc->hwmode);
> }
>
> diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c
> index 2121ea29e1b2..059c3346db68 100644
> --- a/drivers/gpu/drm/drm_irq.c
> +++ b/drivers/gpu/drm/drm_irq.c
> @@ -54,7 +54,7 @@
>
> static bool
> drm_get_last_vbltimestamp(struct drm_device *dev, unsigned int pipe,
> - struct timeval *tvblank, unsigned flags);
> + struct timeval *tvblank, bool in_vblank_irq);
>
> static unsigned int drm_timestamp_precision = 20; /* Default to 20 usecs. */
>
> @@ -138,7 +138,7 @@ static void drm_reset_vblank_timestamp(struct drm_device *dev, unsigned int pipe
> */
> do {
> cur_vblank = __get_vblank_counter(dev, pipe);
> - rc = drm_get_last_vbltimestamp(dev, pipe, &t_vblank, 0);
> + rc = drm_get_last_vbltimestamp(dev, pipe, &t_vblank, false);
> } while (cur_vblank != __get_vblank_counter(dev, pipe) && --count > 0);
>
> /*
> @@ -171,7 +171,7 @@ static void drm_reset_vblank_timestamp(struct drm_device *dev, unsigned int pipe
> * device vblank fields.
> */
> static void drm_update_vblank_count(struct drm_device *dev, unsigned int pipe,
> - unsigned long flags)
> + bool in_vblank_irq)
> {
> struct drm_vblank_crtc *vblank = &dev->vblank[pipe];
> u32 cur_vblank, diff;
> @@ -194,7 +194,7 @@ static void drm_update_vblank_count(struct drm_device *dev, unsigned int pipe,
> */
> do {
> cur_vblank = __get_vblank_counter(dev, pipe);
> - rc = drm_get_last_vbltimestamp(dev, pipe, &t_vblank, flags);
> + rc = drm_get_last_vbltimestamp(dev, pipe, &t_vblank, in_vblank_irq);
> } while (cur_vblank != __get_vblank_counter(dev, pipe) && --count > 0);
>
> if (dev->max_vblank_count != 0) {
> @@ -214,13 +214,13 @@ 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)
> + if (diff == 0 && in_vblank_irq)
> DRM_DEBUG_VBL("crtc %u: Redundant vblirq ignored."
> " diff_ns = %lld, framedur_ns = %d)\n",
> pipe, (long long) diff_ns, framedur_ns);
> } else {
> /* some kind of default for drivers w/o accurate vbl timestamping */
> - diff = (flags & DRM_CALLED_FROM_VBLIRQ) != 0;
> + diff = in_vblank_irq ? 1 : 0;
> }
>
> /*
> @@ -253,7 +253,7 @@ static void drm_update_vblank_count(struct drm_device *dev, unsigned int pipe,
> * Otherwise reinitialize delayed at next vblank interrupt and assign 0
> * for now, to mark the vblanktimestamp as invalid.
> */
> - if (!rc && (flags & DRM_CALLED_FROM_VBLIRQ) == 0)
> + if (!rc && in_vblank_irq)
> t_vblank = (struct timeval) {0, 0};
>
> store_vblank(dev, pipe, diff, &t_vblank, cur_vblank);
> @@ -291,7 +291,7 @@ u32 drm_accurate_vblank_count(struct drm_crtc *crtc)
>
> spin_lock_irqsave(&dev->vblank_time_lock, flags);
>
> - drm_update_vblank_count(dev, pipe, 0);
> + drm_update_vblank_count(dev, pipe, false);
> vblank = drm_vblank_count(dev, pipe);
>
> spin_unlock_irqrestore(&dev->vblank_time_lock, flags);
> @@ -347,7 +347,7 @@ static void vblank_disable_and_save(struct drm_device *dev, unsigned int pipe)
> * this time. This makes the count account for the entire time
> * between drm_crtc_vblank_on() and drm_crtc_vblank_off().
> */
> - drm_update_vblank_count(dev, pipe, 0);
> + drm_update_vblank_count(dev, pipe, false);
>
> spin_unlock_irqrestore(&dev->vblank_time_lock, irqflags);
> }
> @@ -698,9 +698,10 @@ EXPORT_SYMBOL(drm_calc_timestamping_constants);
> * @max_error: Desired maximum allowable error in timestamps (nanosecs)
> * On return contains true maximum error of timestamp
> * @vblank_time: Pointer to struct timeval which should receive the timestamp
> - * @flags: Flags to pass to driver:
> - * 0 = Default,
> - * DRM_CALLED_FROM_VBLIRQ = If function is called from vbl IRQ handler
> + * @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.
> * @mode: mode which defines the scanout timings
> *
> * Implements calculation of exact vblank timestamps from given drm_display_mode
> @@ -730,7 +731,7 @@ bool drm_calc_vbltimestamp_from_scanoutpos(struct drm_device *dev,
> unsigned int pipe,
> int *max_error,
> struct timeval *vblank_time,
> - unsigned flags,
> + bool in_vblank_irq,
> const struct drm_display_mode *mode)
> {
> struct timeval tv_etime;
> @@ -738,6 +739,7 @@ bool drm_calc_vbltimestamp_from_scanoutpos(struct drm_device *dev,
> unsigned int vbl_status;
> int vpos, hpos, i;
> int delta_ns, duration_ns;
> + unsigned flags = in_vblank_irq ? DRM_CALLED_FROM_VBLIRQ : 0;
>
> if (pipe >= dev->num_crtcs) {
> DRM_ERROR("Invalid crtc %u\n", pipe);
> @@ -841,9 +843,10 @@ static struct timeval get_drm_timestamp(void)
> * @dev: DRM device
> * @pipe: index of CRTC whose vblank timestamp to retrieve
> * @tvblank: Pointer to target struct timeval which should receive the timestamp
> - * @flags: Flags to pass to driver:
> - * 0 = Default,
> - * DRM_CALLED_FROM_VBLIRQ = If function is called from vbl IRQ handler
> + * @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.
> *
> * Fetches the system timestamp corresponding to the time of the most recent
> * vblank interval on specified CRTC. May call into kms-driver to
> @@ -857,7 +860,7 @@ static struct timeval get_drm_timestamp(void)
> */
> static bool
> drm_get_last_vbltimestamp(struct drm_device *dev, unsigned int pipe,
> - struct timeval *tvblank, unsigned flags)
> + struct timeval *tvblank, bool in_vblank_irq)
> {
> int ret;
>
> @@ -867,7 +870,7 @@ drm_get_last_vbltimestamp(struct drm_device *dev, unsigned int pipe,
> /* Query driver if possible and precision timestamping enabled. */
> if (dev->driver->get_vblank_timestamp && (max_error > 0)) {
> ret = dev->driver->get_vblank_timestamp(dev, pipe, &max_error,
> - tvblank, flags);
> + tvblank, in_vblank_irq);
> if (ret > 0)
> return true;
> }
> @@ -1710,7 +1713,7 @@ bool drm_handle_vblank(struct drm_device *dev, unsigned int pipe)
> return false;
> }
>
> - drm_update_vblank_count(dev, pipe, DRM_CALLED_FROM_VBLIRQ);
> + drm_update_vblank_count(dev, pipe, true);
>
> spin_unlock(&dev->vblank_time_lock);
>
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index df149d159ce7..6c8a7e1284c3 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -967,7 +967,7 @@ int intel_get_crtc_scanline(struct intel_crtc *crtc)
> static bool i915_get_vblank_timestamp(struct drm_device *dev, unsigned int pipe,
> int *max_error,
> struct timeval *vblank_time,
> - unsigned flags)
> + bool in_vblank_irq)
> {
> struct drm_i915_private *dev_priv = to_i915(dev);
> struct intel_crtc *crtc;
> @@ -991,7 +991,7 @@ static bool i915_get_vblank_timestamp(struct drm_device *dev, unsigned int pipe,
>
> /* Helper routine in DRM core does all the work: */
> return drm_calc_vbltimestamp_from_scanoutpos(dev, pipe, max_error,
> - vblank_time, flags,
> + vblank_time, in_vblank_irq,
> &crtc->base.hwmode);
> }
>
> diff --git a/drivers/gpu/drm/msm/mdp/mdp5/mdp5_kms.c b/drivers/gpu/drm/msm/mdp/mdp5/mdp5_kms.c
> index 655700eb42ba..16184ccbdd3b 100644
> --- a/drivers/gpu/drm/msm/mdp/mdp5/mdp5_kms.c
> +++ b/drivers/gpu/drm/msm/mdp/mdp5/mdp5_kms.c
> @@ -598,7 +598,7 @@ static int mdp5_get_scanoutpos(struct drm_device *dev, unsigned int pipe,
> static bool mdp5_get_vblank_timestamp(struct drm_device *dev, unsigned int pipe,
> int *max_error,
> struct timeval *vblank_time,
> - unsigned flags)
> + bool in_vblank_irq)
> {
> struct msm_drm_private *priv = dev->dev_private;
> struct drm_crtc *crtc;
> @@ -615,7 +615,7 @@ static bool mdp5_get_vblank_timestamp(struct drm_device *dev, unsigned int pipe,
> }
>
> return drm_calc_vbltimestamp_from_scanoutpos(dev, pipe, max_error,
> - vblank_time, flags,
> + vblank_time, in_vblank_irq,
> &crtc->mode);
> }
>
> diff --git a/drivers/gpu/drm/nouveau/nouveau_display.c b/drivers/gpu/drm/nouveau/nouveau_display.c
> index 42f18b0b9c43..be8ec18ba126 100644
> --- a/drivers/gpu/drm/nouveau/nouveau_display.c
> +++ b/drivers/gpu/drm/nouveau/nouveau_display.c
> @@ -158,7 +158,7 @@ nouveau_display_scanoutpos(struct drm_device *dev, unsigned int pipe,
>
> bool
> nouveau_display_vblstamp(struct drm_device *dev, unsigned int pipe,
> - int *max_error, struct timeval *time, unsigned flags)
> + int *max_error, struct timeval *time, bool in_vblank_irq)
> {
> struct drm_crtc *crtc;
>
> @@ -170,7 +170,8 @@ nouveau_display_vblstamp(struct drm_device *dev, unsigned int pipe,
> else
> mode = &crtc->hwmode;
> return drm_calc_vbltimestamp_from_scanoutpos(dev,
> - pipe, max_error, time, flags, mode);
> + pipe, max_error, time, in_vblank_irq,
> + mode);
> }
> }
>
> diff --git a/drivers/gpu/drm/nouveau/nouveau_display.h b/drivers/gpu/drm/nouveau/nouveau_display.h
> index bc9d1e7b0117..f821fc9e2de3 100644
> --- a/drivers/gpu/drm/nouveau/nouveau_display.h
> +++ b/drivers/gpu/drm/nouveau/nouveau_display.h
> @@ -72,7 +72,7 @@ int nouveau_display_scanoutpos(struct drm_device *, unsigned int,
> unsigned int, int *, int *, ktime_t *,
> ktime_t *, const struct drm_display_mode *);
> bool nouveau_display_vblstamp(struct drm_device *, unsigned int, int *,
> - struct timeval *, unsigned);
> + struct timeval *, bool);
>
> int nouveau_crtc_page_flip(struct drm_crtc *crtc, struct drm_framebuffer *fb,
> struct drm_pending_vblank_event *event,
> diff --git a/drivers/gpu/drm/radeon/radeon_drv.c b/drivers/gpu/drm/radeon/radeon_drv.c
> index 6d79b5c2805b..5fbbc6ac165d 100644
> --- a/drivers/gpu/drm/radeon/radeon_drv.c
> +++ b/drivers/gpu/drm/radeon/radeon_drv.c
> @@ -117,7 +117,7 @@ void radeon_disable_vblank_kms(struct drm_device *dev, unsigned int pipe);
> bool radeon_get_vblank_timestamp_kms(struct drm_device *dev, unsigned int pipe,
> int *max_error,
> struct timeval *vblank_time,
> - unsigned flags);
> + bool in_vblank_irq);
> void radeon_driver_irq_preinstall_kms(struct drm_device *dev);
> int radeon_driver_irq_postinstall_kms(struct drm_device *dev);
> void radeon_driver_irq_uninstall_kms(struct drm_device *dev);
> diff --git a/drivers/gpu/drm/radeon/radeon_kms.c b/drivers/gpu/drm/radeon/radeon_kms.c
> index 9afe72010c64..41765d18f863 100644
> --- a/drivers/gpu/drm/radeon/radeon_kms.c
> +++ b/drivers/gpu/drm/radeon/radeon_kms.c
> @@ -873,7 +873,7 @@ void radeon_disable_vblank_kms(struct drm_device *dev, int crtc)
> bool radeon_get_vblank_timestamp_kms(struct drm_device *dev, int crtc,
> int *max_error,
> struct timeval *vblank_time,
> - unsigned flags)
> + bool in_vblank_irq)
> {
> struct drm_crtc *drmcrtc;
> struct radeon_device *rdev = dev->dev_private;
> @@ -890,7 +890,7 @@ bool radeon_get_vblank_timestamp_kms(struct drm_device *dev, int crtc,
>
> /* Helper routine in DRM core does all the work: */
> return drm_calc_vbltimestamp_from_scanoutpos(dev, crtc, max_error,
> - vblank_time, flags,
> + vblank_time, in_vblank_irq,
> &drmcrtc->hwmode);
> }
>
> diff --git a/drivers/gpu/drm/vc4/vc4_crtc.c b/drivers/gpu/drm/vc4/vc4_crtc.c
> index 74260990b6bf..18bd0d816fe3 100644
> --- a/drivers/gpu/drm/vc4/vc4_crtc.c
> +++ b/drivers/gpu/drm/vc4/vc4_crtc.c
> @@ -272,14 +272,14 @@ int vc4_crtc_get_scanoutpos(struct drm_device *dev, unsigned int crtc_id,
>
> bool vc4_crtc_get_vblank_timestamp(struct drm_device *dev, unsigned int crtc_id,
> int *max_error, struct timeval *vblank_time,
> - unsigned flags)
> + bool in_vblank_irq)
> {
> struct drm_crtc *crtc = drm_crtc_from_index(dev, crtc_id);
> struct drm_crtc_state *state = crtc->state;
>
> /* Helper routine in DRM core does all the work: */
> return drm_calc_vbltimestamp_from_scanoutpos(dev, crtc_id, max_error,
> - vblank_time, flags,
> + vblank_time, in_vblank_irq,
> &state->adjusted_mode);
> }
>
> diff --git a/drivers/gpu/drm/vc4/vc4_drv.h b/drivers/gpu/drm/vc4/vc4_drv.h
> index 8a5d0e12ee02..815cdeb54971 100644
> --- a/drivers/gpu/drm/vc4/vc4_drv.h
> +++ b/drivers/gpu/drm/vc4/vc4_drv.h
> @@ -452,7 +452,7 @@ int vc4_crtc_get_scanoutpos(struct drm_device *dev, unsigned int crtc_id,
> const struct drm_display_mode *mode);
> bool vc4_crtc_get_vblank_timestamp(struct drm_device *dev, unsigned int crtc_id,
> int *max_error, struct timeval *vblank_time,
> - unsigned flags);
> + bool in_vblank_irq);
>
> /* vc4_debugfs.c */
> int vc4_debugfs_init(struct drm_minor *minor);
> diff --git a/include/drm/drm_drv.h b/include/drm/drm_drv.h
> index da78e248d9d8..9fe6301edd6a 100644
> --- a/include/drm/drm_drv.h
> +++ b/include/drm/drm_drv.h
> @@ -308,11 +308,10 @@ struct drm_driver {
> * Returns true upper bound on error for timestamp.
> * vblank_time:
> * Target location for returned vblank timestamp.
> - * flags:
> - * 0 = Defaults, no special treatment needed.
> - * DRM_CALLED_FROM_VBLIRQ = Function is called from vblank
> - * irq handler. Some drivers need to apply some workarounds
> - * for gpu-specific vblank irq quirks if flag is set.
> + * 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:
> *
> @@ -322,7 +321,7 @@ struct drm_driver {
> bool (*get_vblank_timestamp) (struct drm_device *dev, unsigned int pipe,
> int *max_error,
> struct timeval *vblank_time,
> - unsigned flags);
> + bool in_vblank_irq);
>
> /* these have to be filled in */
>
> diff --git a/include/drm/drm_irq.h b/include/drm/drm_irq.h
> index f0d5ccf9b282..445406efb8dc 100644
> --- a/include/drm/drm_irq.h
> +++ b/include/drm/drm_irq.h
> @@ -156,7 +156,7 @@ u32 drm_accurate_vblank_count(struct drm_crtc *crtc);
> bool drm_calc_vbltimestamp_from_scanoutpos(struct drm_device *dev,
> unsigned int pipe, int *max_error,
> struct timeval *vblank_time,
> - unsigned flags,
> + bool in_vblank_irq,
> const struct drm_display_mode *mode);
> void drm_calc_timestamping_constants(struct drm_crtc *crtc,
> const struct drm_display_mode *mode);
> --
> 2.11.0
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@xxxxxxxxxxxxxxxxxxxxx
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
--
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
