drm/vblank: drop the mode argument from drm_calc_vbltimestamp_from_scanoutpos
diff mbox

Message ID 20170322205612.26619-1-daniel.vetter@ffwll.ch
State New
Headers show

Commit Message

Daniel Vetter March 22, 2017, 8:56 p.m. UTC
If we restrict this helper to only kms drivers (which is the case) we
can look up the correct mode easily ourselves. But it's a bit tricky:

- All legacy drivers look at crtc->hwmode. But that is update already
  at the beginning of the modeset helper, which means when we disable
  a pipe. Hence the final timestamps might be a bit off. But since
  this is an existing bug I'm not going to change it, but just try to
  be bug-for-bug compatible with the current code. This only applies
  to radeon&amdgpu.

- i915 tries to get it perfect by updating crtc->hwmode when the pipe
  is off (i.e. vblank->enabled = false).

- All other atomic drivers look at crtc->state->adjusted_mode. Those
  that look at state->requested_mode simply don't adjust their mode,
  so it's the same. That has two problems: Accessing crtc->state from
  interrupt handling code is unsafe, and it's updated before we shut
  down the pipe. For nonblocking modesets it's even worse.

For atomic drivers try to implement what i915 does. To do that we add
a new hwmode field to the vblank structure, and update it from
drm_calc_timestamping_constants(). For atomic drivers that's called
from the right spot by the helper library already, so all fine. But
for safety let's enforce that.

For legacy driver this function is only called at the end (oh the
fun), which is broken, so again let's not bother and just stay
bug-for-bug compatible.

The  benefit is that we can use drm_calc_vbltimestamp_from_scanoutpos
directly to implement ->get_vblank_timestamp in every driver, deleting
a lot of code.

v2: Completely new approach, trying to mimick the i915 solution.

v3: Fixup kerneldoc.

v4: Drop the WARN_ON to check that the vblank is off, atomic helpers
currently unconditionally call this. Recomputing the same stuff should
be harmless.

Cc: Mario Kleiner <mario.kleiner@tuebingen.mpg.de>
Cc: Eric Anholt <eric@anholt.net>
Cc: Rob Clark <robdclark@gmail.com>
Cc: linux-arm-msm@vger.kernel.org
Cc: freedreno@lists.freedesktop.org
Cc: Alex Deucher <alexander.deucher@amd.com>
Cc: Christian König <christian.koenig@amd.com>
Cc: Ben Skeggs <bskeggs@redhat.com>
Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu.h       |  4 ---
 drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c   |  2 +-
 drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c   | 41 -------------------------------
 drivers/gpu/drm/drm_irq.c                 | 27 +++++++++++++++++---
 drivers/gpu/drm/i915/i915_irq.c           | 33 +------------------------
 drivers/gpu/drm/msm/mdp/mdp5/mdp5_kms.c   | 26 +-------------------
 drivers/gpu/drm/nouveau/nouveau_display.c | 22 -----------------
 drivers/gpu/drm/nouveau/nouveau_display.h |  2 --
 drivers/gpu/drm/nouveau/nouveau_drm.c     |  2 +-
 drivers/gpu/drm/radeon/radeon_drv.c       |  6 +----
 drivers/gpu/drm/radeon/radeon_kms.c       | 37 ----------------------------
 drivers/gpu/drm/vc4/vc4_crtc.c            | 13 ----------
 drivers/gpu/drm/vc4/vc4_drv.c             |  2 +-
 drivers/gpu/drm/vc4/vc4_drv.h             |  3 ---
 include/drm/drm_irq.h                     | 15 +++++++++--
 15 files changed, 42 insertions(+), 193 deletions(-)

Comments

Ville Syrjälä March 30, 2017, 12:03 p.m. UTC | #1
On Wed, Mar 22, 2017 at 09:56:12PM +0100, Daniel Vetter wrote:
> If we restrict this helper to only kms drivers (which is the case) we
> can look up the correct mode easily ourselves. But it's a bit tricky:
> 
> - All legacy drivers look at crtc->hwmode. But that is update already
>   at the beginning of the modeset helper, which means when we disable
>   a pipe. Hence the final timestamps might be a bit off. But since
>   this is an existing bug I'm not going to change it, but just try to
>   be bug-for-bug compatible with the current code. This only applies
>   to radeon&amdgpu.
> 
> - i915 tries to get it perfect by updating crtc->hwmode when the pipe
>   is off (i.e. vblank->enabled = false).
> 
> - All other atomic drivers look at crtc->state->adjusted_mode. Those
>   that look at state->requested_mode simply don't adjust their mode,
>   so it's the same. That has two problems: Accessing crtc->state from
>   interrupt handling code is unsafe, and it's updated before we shut
>   down the pipe. For nonblocking modesets it's even worse.
> 
> For atomic drivers try to implement what i915 does. To do that we add
> a new hwmode field to the vblank structure, and update it from
> drm_calc_timestamping_constants().

i915 clear crtc->hwmode.crtc_clock when turning the crtc off, which
this does not do for the new vblank->hwmode. I guess no one should
really end up in these codepaths with a disabled crtc, but parts of
drm_irq.c sort of look like they're expecting it to happen.

So should we have some way to clear out the vblank->hwmode.crtc_clock
for disabled crtcs? And then maybe make some of these crtc_clock checks
WARN and eventually just nuke it all if it looks like nothing is hitting
those?

> For atomic drivers that's called
> from the right spot by the helper library already, so all fine. But
> for safety let's enforce that.
> 
> For legacy driver this function is only called at the end (oh the
> fun), which is broken, so again let's not bother and just stay
> bug-for-bug compatible.
> 
> The  benefit is that we can use drm_calc_vbltimestamp_from_scanoutpos
> directly to implement ->get_vblank_timestamp in every driver, deleting
> a lot of code.
> 
> v2: Completely new approach, trying to mimick the i915 solution.
> 
> v3: Fixup kerneldoc.
> 
> v4: Drop the WARN_ON to check that the vblank is off, atomic helpers
> currently unconditionally call this. Recomputing the same stuff should
> be harmless.
> 
> Cc: Mario Kleiner <mario.kleiner@tuebingen.mpg.de>
> Cc: Eric Anholt <eric@anholt.net>
> Cc: Rob Clark <robdclark@gmail.com>
> Cc: linux-arm-msm@vger.kernel.org
> Cc: freedreno@lists.freedesktop.org
> Cc: Alex Deucher <alexander.deucher@amd.com>
> Cc: Christian König <christian.koenig@amd.com>
> Cc: Ben Skeggs <bskeggs@redhat.com>
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> ---
>  drivers/gpu/drm/amd/amdgpu/amdgpu.h       |  4 ---
>  drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c   |  2 +-
>  drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c   | 41 -------------------------------
>  drivers/gpu/drm/drm_irq.c                 | 27 +++++++++++++++++---
>  drivers/gpu/drm/i915/i915_irq.c           | 33 +------------------------
>  drivers/gpu/drm/msm/mdp/mdp5/mdp5_kms.c   | 26 +-------------------
>  drivers/gpu/drm/nouveau/nouveau_display.c | 22 -----------------
>  drivers/gpu/drm/nouveau/nouveau_display.h |  2 --
>  drivers/gpu/drm/nouveau/nouveau_drm.c     |  2 +-
>  drivers/gpu/drm/radeon/radeon_drv.c       |  6 +----
>  drivers/gpu/drm/radeon/radeon_kms.c       | 37 ----------------------------
>  drivers/gpu/drm/vc4/vc4_crtc.c            | 13 ----------
>  drivers/gpu/drm/vc4/vc4_drv.c             |  2 +-
>  drivers/gpu/drm/vc4/vc4_drv.h             |  3 ---
>  include/drm/drm_irq.h                     | 15 +++++++++--
>  15 files changed, 42 insertions(+), 193 deletions(-)
> 
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> index edb3bb83e1a9..61bef9609b24 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> @@ -1768,10 +1768,6 @@ int amdgpu_device_resume(struct drm_device *dev, bool resume, bool fbcon);
>  u32 amdgpu_get_vblank_counter_kms(struct drm_device *dev, unsigned int pipe);
>  int amdgpu_enable_vblank_kms(struct drm_device *dev, unsigned int pipe);
>  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,
> -				     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_drv.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> index 8a61296fd4cc..ba169a0699d5 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> @@ -692,7 +692,7 @@ static struct drm_driver kms_driver = {
>  	.get_vblank_counter = amdgpu_get_vblank_counter_kms,
>  	.enable_vblank = amdgpu_enable_vblank_kms,
>  	.disable_vblank = amdgpu_disable_vblank_kms,
> -	.get_vblank_timestamp = amdgpu_get_vblank_timestamp_kms,
> +	.get_vblank_timestamp = drm_calc_vbltimestamp_from_scanoutpos,
>  	.get_scanout_position = amdgpu_get_crtc_scanoutpos,
>  #if defined(CONFIG_DEBUG_FS)
>  	.debugfs_init = amdgpu_debugfs_init,
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
> index ad295e822d45..32a492bd4e51 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
> @@ -827,47 +827,6 @@ void amdgpu_disable_vblank_kms(struct drm_device *dev, unsigned int pipe)
>  	amdgpu_irq_put(adev, &adev->crtc_irq, idx);
>  }
>  
> -/**
> - * amdgpu_get_vblank_timestamp_kms - get vblank timestamp
> - *
> - * @dev: drm dev pointer
> - * @crtc: crtc to get the timestamp for
> - * @max_error: max error
> - * @vblank_time: time value
> - * @in_vblank_irq: called from drm_handle_vblank()
> - *
> - * Gets the timestamp on the requested crtc based on the
> - * scanout position.  (all asics).
> - * Returns true on success, false on failure.
> - */
> -bool amdgpu_get_vblank_timestamp_kms(struct drm_device *dev, unsigned int pipe,
> -				     int *max_error,
> -				     struct timeval *vblank_time,
> -				     bool in_vblank_irq)
> -{
> -	struct drm_crtc *crtc;
> -	struct amdgpu_device *adev = dev->dev_private;
> -
> -	if (pipe >= dev->num_crtcs) {
> -		DRM_ERROR("Invalid crtc %u\n", pipe);
> -		return false;
> -	}
> -
> -	/* Get associated drm_crtc: */
> -	crtc = &adev->mode_info.crtcs[pipe]->base;
> -	if (!crtc) {
> -		/* This can occur on driver load if some component fails to
> -		 * initialize completely and driver is unloaded */
> -		DRM_ERROR("Uninitialized crtc %d\n", pipe);
> -		return false;
> -	}
> -
> -	/* Helper routine in DRM core does all the work: */
> -	return drm_calc_vbltimestamp_from_scanoutpos(dev, pipe, max_error,
> -						     vblank_time, in_vblank_irq,
> -						     &crtc->hwmode);
> -}
> -
>  const struct drm_ioctl_desc amdgpu_ioctls_kms[] = {
>  	DRM_IOCTL_DEF_DRV(AMDGPU_GEM_CREATE, amdgpu_gem_create_ioctl, DRM_AUTH|DRM_RENDER_ALLOW),
>  	DRM_IOCTL_DEF_DRV(AMDGPU_CTX, amdgpu_ctx_ioctl, DRM_AUTH|DRM_RENDER_ALLOW),
> diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c
> index cdb064b46d04..46c923848c16 100644
> --- a/drivers/gpu/drm/drm_irq.c
> +++ b/drivers/gpu/drm/drm_irq.c
> @@ -682,6 +682,7 @@ void drm_calc_timestamping_constants(struct drm_crtc *crtc,
>  
>  	vblank->linedur_ns  = linedur_ns;
>  	vblank->framedur_ns = framedur_ns;
> +	vblank->hwmode = *mode;
>  
>  	DRM_DEBUG("crtc %u: hwmode: htotal %d, vtotal %d, vdisplay %d\n",
>  		  crtc->base.id, mode->crtc_htotal,
> @@ -702,7 +703,6 @@ EXPORT_SYMBOL(drm_calc_timestamping_constants);
>   *     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
>   * timings and current video scanout position of a CRTC. This can be called from
> @@ -722,6 +722,13 @@ EXPORT_SYMBOL(drm_calc_timestamping_constants);
>   * returns as no operation if a doublescan or interlaced video mode is
>   * active. Higher level code is expected to handle this.
>   *
> + * This function can be used to implement the &drm_driver.get_vblank_timestamp
> + * directly, if the driver implements the &drm_driver.get_scanout_position hook.
> + *
> + * 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
> @@ -731,17 +738,24 @@ bool drm_calc_vbltimestamp_from_scanoutpos(struct drm_device *dev,
>  					   unsigned int pipe,
>  					   int *max_error,
>  					   struct timeval *vblank_time,
> -					   bool in_vblank_irq,
> -					   const struct drm_display_mode *mode)
> +					   bool in_vblank_irq)
>  {
>  	struct timeval tv_etime;
>  	ktime_t stime, etime;
>  	unsigned int 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;
>  	unsigned flags = in_vblank_irq ? DRM_CALLED_FROM_VBLIRQ : 0;
>  
> -	if (pipe >= dev->num_crtcs) {
> +	if (!drm_core_check_feature(dev, DRIVER_MODESET))
> +		return false;
> +
> +	crtc = drm_crtc_from_index(dev, pipe);
> +
> +	if (pipe >= dev->num_crtcs || !crtc) {
>  		DRM_ERROR("Invalid crtc %u\n", pipe);
>  		return false;
>  	}
> @@ -752,6 +766,11 @@ bool drm_calc_vbltimestamp_from_scanoutpos(struct drm_device *dev,
>  		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.
>  	 */
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index 6c8a7e1284c3..feadfea77354 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -964,37 +964,6 @@ int intel_get_crtc_scanline(struct intel_crtc *crtc)
>  	return position;
>  }
>  
> -static bool i915_get_vblank_timestamp(struct drm_device *dev, unsigned int pipe,
> -			      int *max_error,
> -			      struct timeval *vblank_time,
> -			      bool in_vblank_irq)
> -{
> -	struct drm_i915_private *dev_priv = to_i915(dev);
> -	struct intel_crtc *crtc;
> -
> -	if (pipe >= INTEL_INFO(dev_priv)->num_pipes) {
> -		DRM_ERROR("Invalid crtc %u\n", pipe);
> -		return false;
> -	}
> -
> -	/* Get drm_crtc to timestamp: */
> -	crtc = intel_get_crtc_for_pipe(dev_priv, pipe);
> -	if (crtc == NULL) {
> -		DRM_ERROR("Invalid crtc %u\n", pipe);
> -		return false;
> -	}
> -
> -	if (!crtc->base.hwmode.crtc_clock) {
> -		DRM_DEBUG_KMS("crtc %u is disabled\n", pipe);
> -		return false;
> -	}
> -
> -	/* Helper routine in DRM core does all the work: */
> -	return drm_calc_vbltimestamp_from_scanoutpos(dev, pipe, max_error,
> -						     vblank_time, in_vblank_irq,
> -						     &crtc->base.hwmode);
> -}
> -
>  static void ironlake_rps_change_irq_handler(struct drm_i915_private *dev_priv)
>  {
>  	u32 busy_up, busy_down, max_avg, min_avg;
> @@ -4328,7 +4297,7 @@ void intel_irq_init(struct drm_i915_private *dev_priv)
>  
>  	dev_priv->hotplug.hpd_storm_threshold = HPD_STORM_DEFAULT_THRESHOLD;
>  
> -	dev->driver->get_vblank_timestamp = i915_get_vblank_timestamp;
> +	dev->driver->get_vblank_timestamp = drm_calc_vbltimestamp_from_scanoutpos;
>  	dev->driver->get_scanout_position = i915_get_crtc_scanoutpos;
>  
>  	if (IS_CHERRYVIEW(dev_priv)) {
> diff --git a/drivers/gpu/drm/msm/mdp/mdp5/mdp5_kms.c b/drivers/gpu/drm/msm/mdp/mdp5/mdp5_kms.c
> index 16184ccbdd3b..6ba216b8bba9 100644
> --- a/drivers/gpu/drm/msm/mdp/mdp5/mdp5_kms.c
> +++ b/drivers/gpu/drm/msm/mdp/mdp5/mdp5_kms.c
> @@ -595,30 +595,6 @@ static int mdp5_get_scanoutpos(struct drm_device *dev, unsigned int pipe,
>  	return ret;
>  }
>  
> -static bool mdp5_get_vblank_timestamp(struct drm_device *dev, unsigned int pipe,
> -				      int *max_error,
> -				      struct timeval *vblank_time,
> -				      bool in_vblank_irq)
> -{
> -	struct msm_drm_private *priv = dev->dev_private;
> -	struct drm_crtc *crtc;
> -
> -	if (pipe < 0 || pipe >= priv->num_crtcs) {
> -		DRM_ERROR("Invalid crtc %d\n", pipe);
> -		return false;
> -	}
> -
> -	crtc = priv->crtcs[pipe];
> -	if (!crtc) {
> -		DRM_ERROR("Invalid crtc %d\n", pipe);
> -		return false;
> -	}
> -
> -	return drm_calc_vbltimestamp_from_scanoutpos(dev, pipe, max_error,
> -						     vblank_time, in_vblank_irq,
> -						     &crtc->mode);
> -}
> -
>  static u32 mdp5_get_vblank_counter(struct drm_device *dev, unsigned int pipe)
>  {
>  	struct msm_drm_private *priv = dev->dev_private;
> @@ -728,7 +704,7 @@ struct msm_kms *mdp5_kms_init(struct drm_device *dev)
>  	dev->mode_config.max_width = 0xffff;
>  	dev->mode_config.max_height = 0xffff;
>  
> -	dev->driver->get_vblank_timestamp = mdp5_get_vblank_timestamp;
> +	dev->driver->get_vblank_timestamp = drm_calc_vbltimestamp_from_scanoutpos;
>  	dev->driver->get_scanout_position = mdp5_get_scanoutpos;
>  	dev->driver->get_vblank_counter = mdp5_get_vblank_counter;
>  	dev->max_vblank_count = 0xffffffff;
> diff --git a/drivers/gpu/drm/nouveau/nouveau_display.c b/drivers/gpu/drm/nouveau/nouveau_display.c
> index be8ec18ba126..e4bdac13d4e9 100644
> --- a/drivers/gpu/drm/nouveau/nouveau_display.c
> +++ b/drivers/gpu/drm/nouveau/nouveau_display.c
> @@ -156,28 +156,6 @@ nouveau_display_scanoutpos(struct drm_device *dev, unsigned int pipe,
>  	return 0;
>  }
>  
> -bool
> -nouveau_display_vblstamp(struct drm_device *dev, unsigned int pipe,
> -			 int *max_error, struct timeval *time, bool in_vblank_irq)
> -{
> -	struct drm_crtc *crtc;
> -
> -	list_for_each_entry(crtc, &dev->mode_config.crtc_list, head) {
> -		if (nouveau_crtc(crtc)->index == pipe) {
> -			struct drm_display_mode *mode;
> -			if (drm_drv_uses_atomic_modeset(dev))
> -				mode = &crtc->state->adjusted_mode;
> -			else
> -				mode = &crtc->hwmode;
> -			return drm_calc_vbltimestamp_from_scanoutpos(dev,
> -					pipe, max_error, time, in_vblank_irq,
> -					mode);
> -		}
> -	}
> -
> -	return false;
> -}
> -
>  static void
>  nouveau_display_vblank_fini(struct drm_device *dev)
>  {
> diff --git a/drivers/gpu/drm/nouveau/nouveau_display.h b/drivers/gpu/drm/nouveau/nouveau_display.h
> index f821fc9e2de3..8360a85ed5ef 100644
> --- a/drivers/gpu/drm/nouveau/nouveau_display.h
> +++ b/drivers/gpu/drm/nouveau/nouveau_display.h
> @@ -71,8 +71,6 @@ void nouveau_display_vblank_disable(struct drm_device *, unsigned int);
>  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 *, 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/nouveau/nouveau_drm.c b/drivers/gpu/drm/nouveau/nouveau_drm.c
> index ec719df619a6..1f751a3f570c 100644
> --- a/drivers/gpu/drm/nouveau/nouveau_drm.c
> +++ b/drivers/gpu/drm/nouveau/nouveau_drm.c
> @@ -978,7 +978,7 @@ driver_stub = {
>  	.enable_vblank = nouveau_display_vblank_enable,
>  	.disable_vblank = nouveau_display_vblank_disable,
>  	.get_scanout_position = nouveau_display_scanoutpos,
> -	.get_vblank_timestamp = nouveau_display_vblstamp,
> +	.get_vblank_timestamp = drm_calc_vbltimestamp_from_scanoutpos,
>  
>  	.ioctls = nouveau_ioctls,
>  	.num_ioctls = ARRAY_SIZE(nouveau_ioctls),
> diff --git a/drivers/gpu/drm/radeon/radeon_drv.c b/drivers/gpu/drm/radeon/radeon_drv.c
> index 5fbbc6ac165d..a4bf09cd33f7 100644
> --- a/drivers/gpu/drm/radeon/radeon_drv.c
> +++ b/drivers/gpu/drm/radeon/radeon_drv.c
> @@ -114,10 +114,6 @@ int radeon_resume_kms(struct drm_device *dev, bool resume, bool fbcon);
>  u32 radeon_get_vblank_counter_kms(struct drm_device *dev, unsigned int pipe);
>  int radeon_enable_vblank_kms(struct drm_device *dev, unsigned int pipe);
>  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,
> -				     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);
> @@ -543,7 +539,7 @@ static struct drm_driver kms_driver = {
>  	.get_vblank_counter = radeon_get_vblank_counter_kms,
>  	.enable_vblank = radeon_enable_vblank_kms,
>  	.disable_vblank = radeon_disable_vblank_kms,
> -	.get_vblank_timestamp = radeon_get_vblank_timestamp_kms,
> +	.get_vblank_timestamp = drm_calc_vbltimestamp_from_scanoutpos,
>  	.get_scanout_position = radeon_get_crtc_scanoutpos,
>  	.irq_preinstall = radeon_driver_irq_preinstall_kms,
>  	.irq_postinstall = radeon_driver_irq_postinstall_kms,
> diff --git a/drivers/gpu/drm/radeon/radeon_kms.c b/drivers/gpu/drm/radeon/radeon_kms.c
> index 41765d18f863..33b8b3d22969 100644
> --- a/drivers/gpu/drm/radeon/radeon_kms.c
> +++ b/drivers/gpu/drm/radeon/radeon_kms.c
> @@ -857,43 +857,6 @@ void radeon_disable_vblank_kms(struct drm_device *dev, int crtc)
>  	spin_unlock_irqrestore(&rdev->irq.lock, irqflags);
>  }
>  
> -/**
> - * radeon_get_vblank_timestamp_kms - get vblank timestamp
> - *
> - * @dev: drm dev pointer
> - * @crtc: crtc to get the timestamp for
> - * @max_error: max error
> - * @vblank_time: time value
> - * @flags: flags passed to the driver
> - *
> - * Gets the timestamp on the requested crtc based on the
> - * scanout position.  (all asics).
> - * Returns true on success, false on failure.
> - */
> -bool radeon_get_vblank_timestamp_kms(struct drm_device *dev, int crtc,
> -				     int *max_error,
> -				     struct timeval *vblank_time,
> -				     bool in_vblank_irq)
> -{
> -	struct drm_crtc *drmcrtc;
> -	struct radeon_device *rdev = dev->dev_private;
> -
> -	if (crtc < 0 || crtc >= dev->num_crtcs) {
> -		DRM_ERROR("Invalid crtc %d\n", crtc);
> -		return false;
> -	}
> -
> -	/* Get associated drm_crtc: */
> -	drmcrtc = &rdev->mode_info.crtcs[crtc]->base;
> -	if (!drmcrtc)
> -		return false;
> -
> -	/* Helper routine in DRM core does all the work: */
> -	return drm_calc_vbltimestamp_from_scanoutpos(dev, crtc, max_error,
> -						     vblank_time, in_vblank_irq,
> -						     &drmcrtc->hwmode);
> -}
> -
>  const struct drm_ioctl_desc radeon_ioctls_kms[] = {
>  	DRM_IOCTL_DEF_DRV(RADEON_CP_INIT, drm_invalid_op, DRM_AUTH|DRM_MASTER|DRM_ROOT_ONLY),
>  	DRM_IOCTL_DEF_DRV(RADEON_CP_START, drm_invalid_op, DRM_AUTH|DRM_MASTER|DRM_ROOT_ONLY),
> diff --git a/drivers/gpu/drm/vc4/vc4_crtc.c b/drivers/gpu/drm/vc4/vc4_crtc.c
> index 18bd0d816fe3..2567d6c9a4ee 100644
> --- a/drivers/gpu/drm/vc4/vc4_crtc.c
> +++ b/drivers/gpu/drm/vc4/vc4_crtc.c
> @@ -270,19 +270,6 @@ int vc4_crtc_get_scanoutpos(struct drm_device *dev, unsigned int crtc_id,
>  	return ret;
>  }
>  
> -bool vc4_crtc_get_vblank_timestamp(struct drm_device *dev, unsigned int crtc_id,
> -				  int *max_error, struct timeval *vblank_time,
> -				  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, in_vblank_irq,
> -						     &state->adjusted_mode);
> -}
> -
>  static void vc4_crtc_destroy(struct drm_crtc *crtc)
>  {
>  	drm_crtc_cleanup(crtc);
> diff --git a/drivers/gpu/drm/vc4/vc4_drv.c b/drivers/gpu/drm/vc4/vc4_drv.c
> index 61e674baf3a6..e864256e12e5 100644
> --- a/drivers/gpu/drm/vc4/vc4_drv.c
> +++ b/drivers/gpu/drm/vc4/vc4_drv.c
> @@ -154,7 +154,7 @@ static struct drm_driver vc4_drm_driver = {
>  	.irq_uninstall = vc4_irq_uninstall,
>  
>  	.get_scanout_position = vc4_crtc_get_scanoutpos,
> -	.get_vblank_timestamp = vc4_crtc_get_vblank_timestamp,
> +	.get_vblank_timestamp = drm_calc_vbltimestamp_from_scanoutpos,
>  
>  #if defined(CONFIG_DEBUG_FS)
>  	.debugfs_init = vc4_debugfs_init,
> diff --git a/drivers/gpu/drm/vc4/vc4_drv.h b/drivers/gpu/drm/vc4/vc4_drv.h
> index 815cdeb54971..64c92e0eb8f7 100644
> --- a/drivers/gpu/drm/vc4/vc4_drv.h
> +++ b/drivers/gpu/drm/vc4/vc4_drv.h
> @@ -450,9 +450,6 @@ int vc4_crtc_get_scanoutpos(struct drm_device *dev, unsigned int crtc_id,
>  			    unsigned int flags, int *vpos, int *hpos,
>  			    ktime_t *stime, ktime_t *etime,
>  			    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,
> -				  bool in_vblank_irq);
>  
>  /* vc4_debugfs.c */
>  int vc4_debugfs_init(struct drm_minor *minor);
> diff --git a/include/drm/drm_irq.h b/include/drm/drm_irq.h
> index 445406efb8dc..b489cc856e7a 100644
> --- a/include/drm/drm_irq.h
> +++ b/include/drm/drm_irq.h
> @@ -121,6 +121,18 @@ struct drm_vblank_crtc {
>  	 * drm_calc_timestamping_constants().
>  	 */
>  	int linedur_ns;
> +
> +	/**
> +	 * @hwmode:
> +	 *
> +	 * Cache of the current hardware display mode. Only valide 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,
> +	 * because that one is really hard to get at from interrupt context.
> +	 */
> +	struct drm_display_mode hwmode;
> +
>  	/**
>  	 * @enabled: Tracks the enabling state of the corresponding &drm_crtc to
>  	 * avoid double-disabling and hence corrupting saved state. Needed by
> @@ -156,8 +168,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,
> -					   bool in_vblank_irq,
> -					   const struct drm_display_mode *mode);
> +					   bool in_vblank_irq);
>  void drm_calc_timestamping_constants(struct drm_crtc *crtc,
>  				     const struct drm_display_mode *mode);
>  
> -- 
> 2.11.0
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Daniel Vetter March 30, 2017, 1:27 p.m. UTC | #2
On Thu, Mar 30, 2017 at 2:03 PM, Ville Syrjälä
<ville.syrjala@linux.intel.com> wrote:
> On Wed, Mar 22, 2017 at 09:56:12PM +0100, Daniel Vetter wrote:
>> If we restrict this helper to only kms drivers (which is the case) we
>> can look up the correct mode easily ourselves. But it's a bit tricky:
>>
>> - All legacy drivers look at crtc->hwmode. But that is update already
>>   at the beginning of the modeset helper, which means when we disable
>>   a pipe. Hence the final timestamps might be a bit off. But since
>>   this is an existing bug I'm not going to change it, but just try to
>>   be bug-for-bug compatible with the current code. This only applies
>>   to radeon&amdgpu.
>>
>> - i915 tries to get it perfect by updating crtc->hwmode when the pipe
>>   is off (i.e. vblank->enabled = false).
>>
>> - All other atomic drivers look at crtc->state->adjusted_mode. Those
>>   that look at state->requested_mode simply don't adjust their mode,
>>   so it's the same. That has two problems: Accessing crtc->state from
>>   interrupt handling code is unsafe, and it's updated before we shut
>>   down the pipe. For nonblocking modesets it's even worse.
>>
>> For atomic drivers try to implement what i915 does. To do that we add
>> a new hwmode field to the vblank structure, and update it from
>> drm_calc_timestamping_constants().
>
> i915 clear crtc->hwmode.crtc_clock when turning the crtc off, which
> this does not do for the new vblank->hwmode. I guess no one should
> really end up in these codepaths with a disabled crtc, but parts of
> drm_irq.c sort of look like they're expecting it to happen.
>
> So should we have some way to clear out the vblank->hwmode.crtc_clock
> for disabled crtcs? And then maybe make some of these crtc_clock checks
> WARN and eventually just nuke it all if it looks like nothing is hitting
> those?

So the trouble is that with a pile of dpms on/off/on/off you could run
drm_crtc_vblank_on/off a lot, without ever calling the
drm_calc_vbltimestamps helper again to re-upload the mode. So I don't
think we can clear vblank->hwmode.crtc_clock unfortunately in
drm_crtc_vblank_off.

But what we could do (at least with atomic) is WARN in the vblank
helper if it's called outside of drm_vblank_on/off ... Not sure how
useful that is (it won't catch when a driver outright forgets to call
these) or whether we have enough checks already. Would be a separate
patch (can do ofc if we agree on what exactly).
-Daniel
Ville Syrjälä March 30, 2017, 1:41 p.m. UTC | #3
On Thu, Mar 30, 2017 at 03:27:57PM +0200, Daniel Vetter wrote:
> On Thu, Mar 30, 2017 at 2:03 PM, Ville Syrjälä
> <ville.syrjala@linux.intel.com> wrote:
> > On Wed, Mar 22, 2017 at 09:56:12PM +0100, Daniel Vetter wrote:
> >> If we restrict this helper to only kms drivers (which is the case) we
> >> can look up the correct mode easily ourselves. But it's a bit tricky:
> >>
> >> - All legacy drivers look at crtc->hwmode. But that is update already
> >>   at the beginning of the modeset helper, which means when we disable
> >>   a pipe. Hence the final timestamps might be a bit off. But since
> >>   this is an existing bug I'm not going to change it, but just try to
> >>   be bug-for-bug compatible with the current code. This only applies
> >>   to radeon&amdgpu.
> >>
> >> - i915 tries to get it perfect by updating crtc->hwmode when the pipe
> >>   is off (i.e. vblank->enabled = false).
> >>
> >> - All other atomic drivers look at crtc->state->adjusted_mode. Those
> >>   that look at state->requested_mode simply don't adjust their mode,
> >>   so it's the same. That has two problems: Accessing crtc->state from
> >>   interrupt handling code is unsafe, and it's updated before we shut
> >>   down the pipe. For nonblocking modesets it's even worse.
> >>
> >> For atomic drivers try to implement what i915 does. To do that we add
> >> a new hwmode field to the vblank structure, and update it from
> >> drm_calc_timestamping_constants().
> >
> > i915 clear crtc->hwmode.crtc_clock when turning the crtc off, which
> > this does not do for the new vblank->hwmode. I guess no one should
> > really end up in these codepaths with a disabled crtc, but parts of
> > drm_irq.c sort of look like they're expecting it to happen.
> >
> > So should we have some way to clear out the vblank->hwmode.crtc_clock
> > for disabled crtcs? And then maybe make some of these crtc_clock checks
> > WARN and eventually just nuke it all if it looks like nothing is hitting
> > those?
> 
> So the trouble is that with a pile of dpms on/off/on/off you could run
> drm_crtc_vblank_on/off a lot, without ever calling the
> drm_calc_vbltimestamps helper again to re-upload the mode. So I don't
> think we can clear vblank->hwmode.crtc_clock unfortunately in
> drm_crtc_vblank_off.

I was thinking that we'd just try to avoid making pontetially functional
changes here. Ie. reset where we currently reset, which I think is somewhere
in the atomic commit for i915, but definitely not in drm_crtc_vblank_off().

> 
> But what we could do (at least with atomic) is WARN in the vblank
> helper if it's called outside of drm_vblank_on/off ... Not sure how
> useful that is (it won't catch when a driver outright forgets to call
> these) or whether we have enough checks already. Would be a separate
> patch (can do ofc if we agree on what exactly).
> -Daniel
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> +41 (0) 79 365 57 48 - http://blog.ffwll.ch
Daniel Vetter March 30, 2017, 6:27 p.m. UTC | #4
On Thu, Mar 30, 2017 at 04:41:57PM +0300, Ville Syrjälä wrote:
> On Thu, Mar 30, 2017 at 03:27:57PM +0200, Daniel Vetter wrote:
> > On Thu, Mar 30, 2017 at 2:03 PM, Ville Syrjälä
> > <ville.syrjala@linux.intel.com> wrote:
> > > On Wed, Mar 22, 2017 at 09:56:12PM +0100, Daniel Vetter wrote:
> > >> If we restrict this helper to only kms drivers (which is the case) we
> > >> can look up the correct mode easily ourselves. But it's a bit tricky:
> > >>
> > >> - All legacy drivers look at crtc->hwmode. But that is update already
> > >>   at the beginning of the modeset helper, which means when we disable
> > >>   a pipe. Hence the final timestamps might be a bit off. But since
> > >>   this is an existing bug I'm not going to change it, but just try to
> > >>   be bug-for-bug compatible with the current code. This only applies
> > >>   to radeon&amdgpu.
> > >>
> > >> - i915 tries to get it perfect by updating crtc->hwmode when the pipe
> > >>   is off (i.e. vblank->enabled = false).
> > >>
> > >> - All other atomic drivers look at crtc->state->adjusted_mode. Those
> > >>   that look at state->requested_mode simply don't adjust their mode,
> > >>   so it's the same. That has two problems: Accessing crtc->state from
> > >>   interrupt handling code is unsafe, and it's updated before we shut
> > >>   down the pipe. For nonblocking modesets it's even worse.
> > >>
> > >> For atomic drivers try to implement what i915 does. To do that we add
> > >> a new hwmode field to the vblank structure, and update it from
> > >> drm_calc_timestamping_constants().
> > >
> > > i915 clear crtc->hwmode.crtc_clock when turning the crtc off, which
> > > this does not do for the new vblank->hwmode. I guess no one should
> > > really end up in these codepaths with a disabled crtc, but parts of
> > > drm_irq.c sort of look like they're expecting it to happen.
> > >
> > > So should we have some way to clear out the vblank->hwmode.crtc_clock
> > > for disabled crtcs? And then maybe make some of these crtc_clock checks
> > > WARN and eventually just nuke it all if it looks like nothing is hitting
> > > those?
> > 
> > So the trouble is that with a pile of dpms on/off/on/off you could run
> > drm_crtc_vblank_on/off a lot, without ever calling the
> > drm_calc_vbltimestamps helper again to re-upload the mode. So I don't
> > think we can clear vblank->hwmode.crtc_clock unfortunately in
> > drm_crtc_vblank_off.
> 
> I was thinking that we'd just try to avoid making pontetially functional
> changes here. Ie. reset where we currently reset, which I think is somewhere
> in the atomic commit for i915, but definitely not in drm_crtc_vblank_off().

Hm, sounds like a good idea. I'll try to shuffle things around. Upon
re-reading all that stuff I also noticed that atm i915 only calls the
helper that would update vblank->hwmode when we do a full modeset, but not
when we just fastset. Shouldn't matter, because the hwmode will match, but
better safe than sorry.

I'll re-read all that stuff again and will try to lock it down fully, like
we have now, and remove the i915 crtc->hwmode access maybe entirely while
at it.
-Daniel

> 
> > 
> > But what we could do (at least with atomic) is WARN in the vblank
> > helper if it's called outside of drm_vblank_on/off ... Not sure how
> > useful that is (it won't catch when a driver outright forgets to call
> > these) or whether we have enough checks already. Would be a separate
> > patch (can do ofc if we agree on what exactly).
> > -Daniel
> > -- 
> > Daniel Vetter
> > Software Engineer, Intel Corporation
> > +41 (0) 79 365 57 48 - http://blog.ffwll.ch
> 
> -- 
> Ville Syrjälä
> Intel OTC
Daniel Vetter April 4, 2017, 9:54 a.m. UTC | #5
On Thu, Mar 30, 2017 at 08:27:40PM +0200, Daniel Vetter wrote:
> On Thu, Mar 30, 2017 at 04:41:57PM +0300, Ville Syrjälä wrote:
> > On Thu, Mar 30, 2017 at 03:27:57PM +0200, Daniel Vetter wrote:
> > > On Thu, Mar 30, 2017 at 2:03 PM, Ville Syrjälä
> > > <ville.syrjala@linux.intel.com> wrote:
> > > > On Wed, Mar 22, 2017 at 09:56:12PM +0100, Daniel Vetter wrote:
> > > >> If we restrict this helper to only kms drivers (which is the case) we
> > > >> can look up the correct mode easily ourselves. But it's a bit tricky:
> > > >>
> > > >> - All legacy drivers look at crtc->hwmode. But that is update already
> > > >>   at the beginning of the modeset helper, which means when we disable
> > > >>   a pipe. Hence the final timestamps might be a bit off. But since
> > > >>   this is an existing bug I'm not going to change it, but just try to
> > > >>   be bug-for-bug compatible with the current code. This only applies
> > > >>   to radeon&amdgpu.
> > > >>
> > > >> - i915 tries to get it perfect by updating crtc->hwmode when the pipe
> > > >>   is off (i.e. vblank->enabled = false).
> > > >>
> > > >> - All other atomic drivers look at crtc->state->adjusted_mode. Those
> > > >>   that look at state->requested_mode simply don't adjust their mode,
> > > >>   so it's the same. That has two problems: Accessing crtc->state from
> > > >>   interrupt handling code is unsafe, and it's updated before we shut
> > > >>   down the pipe. For nonblocking modesets it's even worse.
> > > >>
> > > >> For atomic drivers try to implement what i915 does. To do that we add
> > > >> a new hwmode field to the vblank structure, and update it from
> > > >> drm_calc_timestamping_constants().
> > > >
> > > > i915 clear crtc->hwmode.crtc_clock when turning the crtc off, which
> > > > this does not do for the new vblank->hwmode. I guess no one should
> > > > really end up in these codepaths with a disabled crtc, but parts of
> > > > drm_irq.c sort of look like they're expecting it to happen.
> > > >
> > > > So should we have some way to clear out the vblank->hwmode.crtc_clock
> > > > for disabled crtcs? And then maybe make some of these crtc_clock checks
> > > > WARN and eventually just nuke it all if it looks like nothing is hitting
> > > > those?
> > > 
> > > So the trouble is that with a pile of dpms on/off/on/off you could run
> > > drm_crtc_vblank_on/off a lot, without ever calling the
> > > drm_calc_vbltimestamps helper again to re-upload the mode. So I don't
> > > think we can clear vblank->hwmode.crtc_clock unfortunately in
> > > drm_crtc_vblank_off.
> > 
> > I was thinking that we'd just try to avoid making pontetially functional
> > changes here. Ie. reset where we currently reset, which I think is somewhere
> > in the atomic commit for i915, but definitely not in drm_crtc_vblank_off().
> 
> Hm, sounds like a good idea. I'll try to shuffle things around. Upon
> re-reading all that stuff I also noticed that atm i915 only calls the
> helper that would update vblank->hwmode when we do a full modeset, but not
> when we just fastset. Shouldn't matter, because the hwmode will match, but
> better safe than sorry.
> 
> I'll re-read all that stuff again and will try to lock it down fully, like
> we have now, and remove the i915 crtc->hwmode access maybe entirely while
> at it.

Ok, I stitched something together which I think should work well. Done as
a follow-up patch though since this refactoring patch here was already
pretty big. Resend the entire pile for CI to double-check, review from you
on the vblank stuff would be much appreciated.
-Daniel

Patch
diff mbox

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
index edb3bb83e1a9..61bef9609b24 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
@@ -1768,10 +1768,6 @@  int amdgpu_device_resume(struct drm_device *dev, bool resume, bool fbcon);
 u32 amdgpu_get_vblank_counter_kms(struct drm_device *dev, unsigned int pipe);
 int amdgpu_enable_vblank_kms(struct drm_device *dev, unsigned int pipe);
 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,
-				     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_drv.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
index 8a61296fd4cc..ba169a0699d5 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
@@ -692,7 +692,7 @@  static struct drm_driver kms_driver = {
 	.get_vblank_counter = amdgpu_get_vblank_counter_kms,
 	.enable_vblank = amdgpu_enable_vblank_kms,
 	.disable_vblank = amdgpu_disable_vblank_kms,
-	.get_vblank_timestamp = amdgpu_get_vblank_timestamp_kms,
+	.get_vblank_timestamp = drm_calc_vbltimestamp_from_scanoutpos,
 	.get_scanout_position = amdgpu_get_crtc_scanoutpos,
 #if defined(CONFIG_DEBUG_FS)
 	.debugfs_init = amdgpu_debugfs_init,
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
index ad295e822d45..32a492bd4e51 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
@@ -827,47 +827,6 @@  void amdgpu_disable_vblank_kms(struct drm_device *dev, unsigned int pipe)
 	amdgpu_irq_put(adev, &adev->crtc_irq, idx);
 }
 
-/**
- * amdgpu_get_vblank_timestamp_kms - get vblank timestamp
- *
- * @dev: drm dev pointer
- * @crtc: crtc to get the timestamp for
- * @max_error: max error
- * @vblank_time: time value
- * @in_vblank_irq: called from drm_handle_vblank()
- *
- * Gets the timestamp on the requested crtc based on the
- * scanout position.  (all asics).
- * Returns true on success, false on failure.
- */
-bool amdgpu_get_vblank_timestamp_kms(struct drm_device *dev, unsigned int pipe,
-				     int *max_error,
-				     struct timeval *vblank_time,
-				     bool in_vblank_irq)
-{
-	struct drm_crtc *crtc;
-	struct amdgpu_device *adev = dev->dev_private;
-
-	if (pipe >= dev->num_crtcs) {
-		DRM_ERROR("Invalid crtc %u\n", pipe);
-		return false;
-	}
-
-	/* Get associated drm_crtc: */
-	crtc = &adev->mode_info.crtcs[pipe]->base;
-	if (!crtc) {
-		/* This can occur on driver load if some component fails to
-		 * initialize completely and driver is unloaded */
-		DRM_ERROR("Uninitialized crtc %d\n", pipe);
-		return false;
-	}
-
-	/* Helper routine in DRM core does all the work: */
-	return drm_calc_vbltimestamp_from_scanoutpos(dev, pipe, max_error,
-						     vblank_time, in_vblank_irq,
-						     &crtc->hwmode);
-}
-
 const struct drm_ioctl_desc amdgpu_ioctls_kms[] = {
 	DRM_IOCTL_DEF_DRV(AMDGPU_GEM_CREATE, amdgpu_gem_create_ioctl, DRM_AUTH|DRM_RENDER_ALLOW),
 	DRM_IOCTL_DEF_DRV(AMDGPU_CTX, amdgpu_ctx_ioctl, DRM_AUTH|DRM_RENDER_ALLOW),
diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c
index cdb064b46d04..46c923848c16 100644
--- a/drivers/gpu/drm/drm_irq.c
+++ b/drivers/gpu/drm/drm_irq.c
@@ -682,6 +682,7 @@  void drm_calc_timestamping_constants(struct drm_crtc *crtc,
 
 	vblank->linedur_ns  = linedur_ns;
 	vblank->framedur_ns = framedur_ns;
+	vblank->hwmode = *mode;
 
 	DRM_DEBUG("crtc %u: hwmode: htotal %d, vtotal %d, vdisplay %d\n",
 		  crtc->base.id, mode->crtc_htotal,
@@ -702,7 +703,6 @@  EXPORT_SYMBOL(drm_calc_timestamping_constants);
  *     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
  * timings and current video scanout position of a CRTC. This can be called from
@@ -722,6 +722,13 @@  EXPORT_SYMBOL(drm_calc_timestamping_constants);
  * returns as no operation if a doublescan or interlaced video mode is
  * active. Higher level code is expected to handle this.
  *
+ * This function can be used to implement the &drm_driver.get_vblank_timestamp
+ * directly, if the driver implements the &drm_driver.get_scanout_position hook.
+ *
+ * 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
@@ -731,17 +738,24 @@  bool drm_calc_vbltimestamp_from_scanoutpos(struct drm_device *dev,
 					   unsigned int pipe,
 					   int *max_error,
 					   struct timeval *vblank_time,
-					   bool in_vblank_irq,
-					   const struct drm_display_mode *mode)
+					   bool in_vblank_irq)
 {
 	struct timeval tv_etime;
 	ktime_t stime, etime;
 	unsigned int 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;
 	unsigned flags = in_vblank_irq ? DRM_CALLED_FROM_VBLIRQ : 0;
 
-	if (pipe >= dev->num_crtcs) {
+	if (!drm_core_check_feature(dev, DRIVER_MODESET))
+		return false;
+
+	crtc = drm_crtc_from_index(dev, pipe);
+
+	if (pipe >= dev->num_crtcs || !crtc) {
 		DRM_ERROR("Invalid crtc %u\n", pipe);
 		return false;
 	}
@@ -752,6 +766,11 @@  bool drm_calc_vbltimestamp_from_scanoutpos(struct drm_device *dev,
 		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.
 	 */
diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index 6c8a7e1284c3..feadfea77354 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -964,37 +964,6 @@  int intel_get_crtc_scanline(struct intel_crtc *crtc)
 	return position;
 }
 
-static bool i915_get_vblank_timestamp(struct drm_device *dev, unsigned int pipe,
-			      int *max_error,
-			      struct timeval *vblank_time,
-			      bool in_vblank_irq)
-{
-	struct drm_i915_private *dev_priv = to_i915(dev);
-	struct intel_crtc *crtc;
-
-	if (pipe >= INTEL_INFO(dev_priv)->num_pipes) {
-		DRM_ERROR("Invalid crtc %u\n", pipe);
-		return false;
-	}
-
-	/* Get drm_crtc to timestamp: */
-	crtc = intel_get_crtc_for_pipe(dev_priv, pipe);
-	if (crtc == NULL) {
-		DRM_ERROR("Invalid crtc %u\n", pipe);
-		return false;
-	}
-
-	if (!crtc->base.hwmode.crtc_clock) {
-		DRM_DEBUG_KMS("crtc %u is disabled\n", pipe);
-		return false;
-	}
-
-	/* Helper routine in DRM core does all the work: */
-	return drm_calc_vbltimestamp_from_scanoutpos(dev, pipe, max_error,
-						     vblank_time, in_vblank_irq,
-						     &crtc->base.hwmode);
-}
-
 static void ironlake_rps_change_irq_handler(struct drm_i915_private *dev_priv)
 {
 	u32 busy_up, busy_down, max_avg, min_avg;
@@ -4328,7 +4297,7 @@  void intel_irq_init(struct drm_i915_private *dev_priv)
 
 	dev_priv->hotplug.hpd_storm_threshold = HPD_STORM_DEFAULT_THRESHOLD;
 
-	dev->driver->get_vblank_timestamp = i915_get_vblank_timestamp;
+	dev->driver->get_vblank_timestamp = drm_calc_vbltimestamp_from_scanoutpos;
 	dev->driver->get_scanout_position = i915_get_crtc_scanoutpos;
 
 	if (IS_CHERRYVIEW(dev_priv)) {
diff --git a/drivers/gpu/drm/msm/mdp/mdp5/mdp5_kms.c b/drivers/gpu/drm/msm/mdp/mdp5/mdp5_kms.c
index 16184ccbdd3b..6ba216b8bba9 100644
--- a/drivers/gpu/drm/msm/mdp/mdp5/mdp5_kms.c
+++ b/drivers/gpu/drm/msm/mdp/mdp5/mdp5_kms.c
@@ -595,30 +595,6 @@  static int mdp5_get_scanoutpos(struct drm_device *dev, unsigned int pipe,
 	return ret;
 }
 
-static bool mdp5_get_vblank_timestamp(struct drm_device *dev, unsigned int pipe,
-				      int *max_error,
-				      struct timeval *vblank_time,
-				      bool in_vblank_irq)
-{
-	struct msm_drm_private *priv = dev->dev_private;
-	struct drm_crtc *crtc;
-
-	if (pipe < 0 || pipe >= priv->num_crtcs) {
-		DRM_ERROR("Invalid crtc %d\n", pipe);
-		return false;
-	}
-
-	crtc = priv->crtcs[pipe];
-	if (!crtc) {
-		DRM_ERROR("Invalid crtc %d\n", pipe);
-		return false;
-	}
-
-	return drm_calc_vbltimestamp_from_scanoutpos(dev, pipe, max_error,
-						     vblank_time, in_vblank_irq,
-						     &crtc->mode);
-}
-
 static u32 mdp5_get_vblank_counter(struct drm_device *dev, unsigned int pipe)
 {
 	struct msm_drm_private *priv = dev->dev_private;
@@ -728,7 +704,7 @@  struct msm_kms *mdp5_kms_init(struct drm_device *dev)
 	dev->mode_config.max_width = 0xffff;
 	dev->mode_config.max_height = 0xffff;
 
-	dev->driver->get_vblank_timestamp = mdp5_get_vblank_timestamp;
+	dev->driver->get_vblank_timestamp = drm_calc_vbltimestamp_from_scanoutpos;
 	dev->driver->get_scanout_position = mdp5_get_scanoutpos;
 	dev->driver->get_vblank_counter = mdp5_get_vblank_counter;
 	dev->max_vblank_count = 0xffffffff;
diff --git a/drivers/gpu/drm/nouveau/nouveau_display.c b/drivers/gpu/drm/nouveau/nouveau_display.c
index be8ec18ba126..e4bdac13d4e9 100644
--- a/drivers/gpu/drm/nouveau/nouveau_display.c
+++ b/drivers/gpu/drm/nouveau/nouveau_display.c
@@ -156,28 +156,6 @@  nouveau_display_scanoutpos(struct drm_device *dev, unsigned int pipe,
 	return 0;
 }
 
-bool
-nouveau_display_vblstamp(struct drm_device *dev, unsigned int pipe,
-			 int *max_error, struct timeval *time, bool in_vblank_irq)
-{
-	struct drm_crtc *crtc;
-
-	list_for_each_entry(crtc, &dev->mode_config.crtc_list, head) {
-		if (nouveau_crtc(crtc)->index == pipe) {
-			struct drm_display_mode *mode;
-			if (drm_drv_uses_atomic_modeset(dev))
-				mode = &crtc->state->adjusted_mode;
-			else
-				mode = &crtc->hwmode;
-			return drm_calc_vbltimestamp_from_scanoutpos(dev,
-					pipe, max_error, time, in_vblank_irq,
-					mode);
-		}
-	}
-
-	return false;
-}
-
 static void
 nouveau_display_vblank_fini(struct drm_device *dev)
 {
diff --git a/drivers/gpu/drm/nouveau/nouveau_display.h b/drivers/gpu/drm/nouveau/nouveau_display.h
index f821fc9e2de3..8360a85ed5ef 100644
--- a/drivers/gpu/drm/nouveau/nouveau_display.h
+++ b/drivers/gpu/drm/nouveau/nouveau_display.h
@@ -71,8 +71,6 @@  void nouveau_display_vblank_disable(struct drm_device *, unsigned int);
 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 *, 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/nouveau/nouveau_drm.c b/drivers/gpu/drm/nouveau/nouveau_drm.c
index ec719df619a6..1f751a3f570c 100644
--- a/drivers/gpu/drm/nouveau/nouveau_drm.c
+++ b/drivers/gpu/drm/nouveau/nouveau_drm.c
@@ -978,7 +978,7 @@  driver_stub = {
 	.enable_vblank = nouveau_display_vblank_enable,
 	.disable_vblank = nouveau_display_vblank_disable,
 	.get_scanout_position = nouveau_display_scanoutpos,
-	.get_vblank_timestamp = nouveau_display_vblstamp,
+	.get_vblank_timestamp = drm_calc_vbltimestamp_from_scanoutpos,
 
 	.ioctls = nouveau_ioctls,
 	.num_ioctls = ARRAY_SIZE(nouveau_ioctls),
diff --git a/drivers/gpu/drm/radeon/radeon_drv.c b/drivers/gpu/drm/radeon/radeon_drv.c
index 5fbbc6ac165d..a4bf09cd33f7 100644
--- a/drivers/gpu/drm/radeon/radeon_drv.c
+++ b/drivers/gpu/drm/radeon/radeon_drv.c
@@ -114,10 +114,6 @@  int radeon_resume_kms(struct drm_device *dev, bool resume, bool fbcon);
 u32 radeon_get_vblank_counter_kms(struct drm_device *dev, unsigned int pipe);
 int radeon_enable_vblank_kms(struct drm_device *dev, unsigned int pipe);
 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,
-				     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);
@@ -543,7 +539,7 @@  static struct drm_driver kms_driver = {
 	.get_vblank_counter = radeon_get_vblank_counter_kms,
 	.enable_vblank = radeon_enable_vblank_kms,
 	.disable_vblank = radeon_disable_vblank_kms,
-	.get_vblank_timestamp = radeon_get_vblank_timestamp_kms,
+	.get_vblank_timestamp = drm_calc_vbltimestamp_from_scanoutpos,
 	.get_scanout_position = radeon_get_crtc_scanoutpos,
 	.irq_preinstall = radeon_driver_irq_preinstall_kms,
 	.irq_postinstall = radeon_driver_irq_postinstall_kms,
diff --git a/drivers/gpu/drm/radeon/radeon_kms.c b/drivers/gpu/drm/radeon/radeon_kms.c
index 41765d18f863..33b8b3d22969 100644
--- a/drivers/gpu/drm/radeon/radeon_kms.c
+++ b/drivers/gpu/drm/radeon/radeon_kms.c
@@ -857,43 +857,6 @@  void radeon_disable_vblank_kms(struct drm_device *dev, int crtc)
 	spin_unlock_irqrestore(&rdev->irq.lock, irqflags);
 }
 
-/**
- * radeon_get_vblank_timestamp_kms - get vblank timestamp
- *
- * @dev: drm dev pointer
- * @crtc: crtc to get the timestamp for
- * @max_error: max error
- * @vblank_time: time value
- * @flags: flags passed to the driver
- *
- * Gets the timestamp on the requested crtc based on the
- * scanout position.  (all asics).
- * Returns true on success, false on failure.
- */
-bool radeon_get_vblank_timestamp_kms(struct drm_device *dev, int crtc,
-				     int *max_error,
-				     struct timeval *vblank_time,
-				     bool in_vblank_irq)
-{
-	struct drm_crtc *drmcrtc;
-	struct radeon_device *rdev = dev->dev_private;
-
-	if (crtc < 0 || crtc >= dev->num_crtcs) {
-		DRM_ERROR("Invalid crtc %d\n", crtc);
-		return false;
-	}
-
-	/* Get associated drm_crtc: */
-	drmcrtc = &rdev->mode_info.crtcs[crtc]->base;
-	if (!drmcrtc)
-		return false;
-
-	/* Helper routine in DRM core does all the work: */
-	return drm_calc_vbltimestamp_from_scanoutpos(dev, crtc, max_error,
-						     vblank_time, in_vblank_irq,
-						     &drmcrtc->hwmode);
-}
-
 const struct drm_ioctl_desc radeon_ioctls_kms[] = {
 	DRM_IOCTL_DEF_DRV(RADEON_CP_INIT, drm_invalid_op, DRM_AUTH|DRM_MASTER|DRM_ROOT_ONLY),
 	DRM_IOCTL_DEF_DRV(RADEON_CP_START, drm_invalid_op, DRM_AUTH|DRM_MASTER|DRM_ROOT_ONLY),
diff --git a/drivers/gpu/drm/vc4/vc4_crtc.c b/drivers/gpu/drm/vc4/vc4_crtc.c
index 18bd0d816fe3..2567d6c9a4ee 100644
--- a/drivers/gpu/drm/vc4/vc4_crtc.c
+++ b/drivers/gpu/drm/vc4/vc4_crtc.c
@@ -270,19 +270,6 @@  int vc4_crtc_get_scanoutpos(struct drm_device *dev, unsigned int crtc_id,
 	return ret;
 }
 
-bool vc4_crtc_get_vblank_timestamp(struct drm_device *dev, unsigned int crtc_id,
-				  int *max_error, struct timeval *vblank_time,
-				  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, in_vblank_irq,
-						     &state->adjusted_mode);
-}
-
 static void vc4_crtc_destroy(struct drm_crtc *crtc)
 {
 	drm_crtc_cleanup(crtc);
diff --git a/drivers/gpu/drm/vc4/vc4_drv.c b/drivers/gpu/drm/vc4/vc4_drv.c
index 61e674baf3a6..e864256e12e5 100644
--- a/drivers/gpu/drm/vc4/vc4_drv.c
+++ b/drivers/gpu/drm/vc4/vc4_drv.c
@@ -154,7 +154,7 @@  static struct drm_driver vc4_drm_driver = {
 	.irq_uninstall = vc4_irq_uninstall,
 
 	.get_scanout_position = vc4_crtc_get_scanoutpos,
-	.get_vblank_timestamp = vc4_crtc_get_vblank_timestamp,
+	.get_vblank_timestamp = drm_calc_vbltimestamp_from_scanoutpos,
 
 #if defined(CONFIG_DEBUG_FS)
 	.debugfs_init = vc4_debugfs_init,
diff --git a/drivers/gpu/drm/vc4/vc4_drv.h b/drivers/gpu/drm/vc4/vc4_drv.h
index 815cdeb54971..64c92e0eb8f7 100644
--- a/drivers/gpu/drm/vc4/vc4_drv.h
+++ b/drivers/gpu/drm/vc4/vc4_drv.h
@@ -450,9 +450,6 @@  int vc4_crtc_get_scanoutpos(struct drm_device *dev, unsigned int crtc_id,
 			    unsigned int flags, int *vpos, int *hpos,
 			    ktime_t *stime, ktime_t *etime,
 			    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,
-				  bool in_vblank_irq);
 
 /* vc4_debugfs.c */
 int vc4_debugfs_init(struct drm_minor *minor);
diff --git a/include/drm/drm_irq.h b/include/drm/drm_irq.h
index 445406efb8dc..b489cc856e7a 100644
--- a/include/drm/drm_irq.h
+++ b/include/drm/drm_irq.h
@@ -121,6 +121,18 @@  struct drm_vblank_crtc {
 	 * drm_calc_timestamping_constants().
 	 */
 	int linedur_ns;
+
+	/**
+	 * @hwmode:
+	 *
+	 * Cache of the current hardware display mode. Only valide 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,
+	 * because that one is really hard to get at from interrupt context.
+	 */
+	struct drm_display_mode hwmode;
+
 	/**
 	 * @enabled: Tracks the enabling state of the corresponding &drm_crtc to
 	 * avoid double-disabling and hence corrupting saved state. Needed by
@@ -156,8 +168,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,
-					   bool in_vblank_irq,
-					   const struct drm_display_mode *mode);
+					   bool in_vblank_irq);
 void drm_calc_timestamping_constants(struct drm_crtc *crtc,
 				     const struct drm_display_mode *mode);