[PATCH 09/11] drm/vblank: Simplify the get_scanout_position helper hook

Neil Armstrong narmstrong at baylibre.com
Tue Apr 4 15:11:46 UTC 2017


On 04/04/2017 11:53 AM, Daniel Vetter wrote:
> - We can drop the different return value flags, the only caller only
>   cares about whether the scanout position is valid or not. Also, it's
>   entirely undefined what "accurate" means, if we'd really care we
>   should probably wire the max_error through. But since we never even
>   report this to userspace it's kinda moot.
> 
> - Drop all the fancy input flags, there's really only the "called from
>   vblank irq" one. Well except for radeon/amdgpu, which added their
>   own private flags.
> 
> Since amdgpu/radoen also use the scanoutposition function internally I
> just gave them a tiny wrapper, plus copies of all the old #defines
> they need. Everyone else gets simplified code.
> 
> Note how we could remove a lot of error conditions if we'd move this
> helper hook to drm_crtc_helper_funcs and would pass it a crtc
> directly.
> 
> v2: Make it compile on arm.
> 
> v3: Squash in fixup from 0day.
> 
> Cc: Mario Kleiner <mario.kleiner at tuebingen.mpg.de>
> Cc: Eric Anholt <eric at anholt.net>
> Cc: Rob Clark <robdclark at gmail.com>
> Cc: linux-arm-msm at vger.kernel.org
> Cc: freedreno at lists.freedesktop.org
> Cc: Alex Deucher <alexander.deucher at amd.com>
> Cc: Christian K├Ânig <christian.koenig at amd.com>
> Cc: Ben Skeggs <bskeggs at redhat.com>
> Signed-off-by: Daniel Vetter <daniel.vetter at intel.com>
> ---
>  drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c   | 12 +++++++++++-
>  drivers/gpu/drm/amd/amdgpu/amdgpu_mode.h  |  3 +++
>  drivers/gpu/drm/drm_irq.c                 | 16 ++++++++--------
>  drivers/gpu/drm/i915/i915_irq.c           | 19 ++++++-------------
>  drivers/gpu/drm/msm/mdp/mdp5/mdp5_kms.c   | 19 +++++++------------
>  drivers/gpu/drm/nouveau/nouveau_display.c | 18 ++++++++----------
>  drivers/gpu/drm/nouveau/nouveau_display.h |  6 +++---
>  drivers/gpu/drm/radeon/radeon_drv.c       | 12 +++++++++++-
>  drivers/gpu/drm/radeon/radeon_mode.h      |  3 +++
>  drivers/gpu/drm/vc4/vc4_crtc.c            | 21 ++++++++++-----------
>  drivers/gpu/drm/vc4/vc4_drv.h             |  8 ++++----
>  include/drm/drmP.h                        |  8 --------
>  include/drm/drm_drv.h                     | 26 ++++++++++----------------
>  13 files changed, 84 insertions(+), 87 deletions(-)
> 

[...]

> diff --git a/drivers/gpu/drm/vc4/vc4_crtc.c b/drivers/gpu/drm/vc4/vc4_crtc.c
> index 2acfecc5b811..04c390a487ba 100644
> --- a/drivers/gpu/drm/vc4/vc4_crtc.c
> +++ b/drivers/gpu/drm/vc4/vc4_crtc.c
> @@ -151,10 +151,10 @@ int vc4_crtc_debugfs_regs(struct seq_file *m, void *unused)
>  }
>  #endif
>  
> -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_scanoutpos(struct drm_device *dev, unsigned int crtc_id,
> +			     bool in_vblank_irq, int *vpos, int *hpos,
> +			     ktime_t *stime, ktime_t *etime,
> +			     const struct drm_display_mode *mode)
>  {
>  	struct vc4_dev *vc4 = to_vc4_dev(dev);
>  	struct drm_crtc *crtc = drm_crtc_from_index(dev, crtc_id);
> @@ -162,7 +162,7 @@ int vc4_crtc_get_scanoutpos(struct drm_device *dev, unsigned int crtc_id,
>  	u32 val;
>  	int fifo_lines;
>  	int vblank_lines;
> -	int ret = 0;
> +	bool ret = false;
>  
>  	/* preempt_disable_rt() should go right here in PREEMPT_RT patchset. */
>  
> @@ -198,7 +198,7 @@ int vc4_crtc_get_scanoutpos(struct drm_device *dev, unsigned int crtc_id,
>  	fifo_lines = vc4_crtc->cob_size / mode->crtc_hdisplay;
>  
>  	if (fifo_lines > 0)
> -		ret |= DRM_SCANOUTPOS_VALID;
> +		ret = true;
>  
>  	/* HVS more than fifo_lines into frame for compositing? */
>  	if (*vpos > fifo_lines) {
> @@ -216,7 +216,6 @@ int vc4_crtc_get_scanoutpos(struct drm_device *dev, unsigned int crtc_id,
>  		 */
>  		*vpos -= fifo_lines + 1;
>  
> -		ret |= DRM_SCANOUTPOS_ACCURATE;
>  		return ret;
>  	}
>  
> @@ -229,10 +228,9 @@ int vc4_crtc_get_scanoutpos(struct drm_device *dev, unsigned int crtc_id,
>  	 * We can't get meaningful readings wrt. scanline position of the PV
>  	 * and need to make things up in a approximative but consistent way.
>  	 */
> -	ret |= DRM_SCANOUTPOS_IN_VBLANK;
>  	vblank_lines = mode->vtotal - mode->vdisplay;
>  
> -	if (flags & DRM_CALLED_FROM_VBLIRQ) {
> +	if (in_vblank_irq) {

Shouldn't this go in patch 06/11 ?

>  		/*
>  		 * Assume the irq handler got called close to first
>  		 * line of vblank, so PV has about a full vblank
> @@ -254,9 +252,10 @@ int vc4_crtc_get_scanoutpos(struct drm_device *dev, unsigned int crtc_id,
>  		 * we are at the very beginning of vblank, as the hvs just
>  		 * started refilling, and the stime and etime timestamps
>  		 * truly correspond to start of vblank.
> +		 *
> +		 * Unfortunately there's no way to report this to upper levels
> +		 * and make it more useful.
>  		 */
> -		if ((val & SCALER_DISPSTATX_FULL) != SCALER_DISPSTATX_FULL)
> -			ret |= DRM_SCANOUTPOS_ACCURATE;
>  	} else {
>  		/*
>  		 * No clue where we are inside vblank. Return a vpos of zero,
> diff --git a/drivers/gpu/drm/vc4/vc4_drv.h b/drivers/gpu/drm/vc4/vc4_drv.h
> index 64c92e0eb8f7..c34a0915e49d 100644
> --- a/drivers/gpu/drm/vc4/vc4_drv.h
> +++ b/drivers/gpu/drm/vc4/vc4_drv.h
> @@ -446,10 +446,10 @@ int vc4_bo_stats_debugfs(struct seq_file *m, void *arg);
>  extern struct platform_driver vc4_crtc_driver;
>  bool vc4_event_pending(struct drm_crtc *crtc);
>  int vc4_crtc_debugfs_regs(struct seq_file *m, void *arg);
> -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_scanoutpos(struct drm_device *dev, unsigned int crtc_id,
> +			     bool in_vblank_irq, int *vpos, int *hpos,
> +			     ktime_t *stime, ktime_t *etime,
> +			     const struct drm_display_mode *mode);
>  
>  /* vc4_debugfs.c */
>  int vc4_debugfs_init(struct drm_minor *minor);
> diff --git a/include/drm/drmP.h b/include/drm/drmP.h
> index 1b3f3cd7b230..6c7ea497e3a6 100644
> --- a/include/drm/drmP.h
> +++ b/include/drm/drmP.h
> @@ -321,14 +321,6 @@ struct pci_controller;
>  #define DRM_IF_VERSION(maj, min) (maj << 16 | min)
>  
>  
> -/* Flags and return codes for get_vblank_timestamp() driver function. */
> -#define DRM_CALLED_FROM_VBLIRQ 1
> -
> -/* get_scanout_position() return flags */
> -#define DRM_SCANOUTPOS_VALID        (1 << 0)
> -#define DRM_SCANOUTPOS_IN_VBLANK    (1 << 1)
> -#define DRM_SCANOUTPOS_ACCURATE     (1 << 2)
> -
>  /**
>   * DRM device structure. This structure represent a complete card that
>   * may contain multiple heads.
> diff --git a/include/drm/drm_drv.h b/include/drm/drm_drv.h
> index 0a367cf5d8d5..e64e33b9dd26 100644
> --- a/include/drm/drm_drv.h
> +++ b/include/drm/drm_drv.h
> @@ -241,8 +241,10 @@ struct drm_driver {
>  	 *     DRM device.
>  	 * pipe:
>  	 *     Id of the crtc to query.
> -	 * flags:
> -	 *     Flags from the caller (DRM_CALLED_FROM_VBLIRQ or 0).
> +	 * 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.

Same here, should be in 06/11 ?

>  	 * vpos:
>  	 *     Target location for current vertical scanout position.
>  	 * hpos:
> @@ -263,16 +265,8 @@ struct drm_driver {
>  	 *
>  	 * Returns:
>  	 *
> -	 * Flags, or'ed together as follows:
> -	 *
> -	 * DRM_SCANOUTPOS_VALID:
> -	 *     Query successful.
> -	 * DRM_SCANOUTPOS_INVBL:
> -	 *     Inside vblank.
> -	 * DRM_SCANOUTPOS_ACCURATE: Returned position is accurate. A lack of
> -	 *     this flag means that returned position may be offset by a
> -	 *     constant but unknown small number of scanlines wrt. real scanout
> -	 *     position.
> +	 * True on success, false if a reliable scanout position counter could
> +	 * not be read out.
>  	 *
>  	 * FIXME:
>  	 *
> @@ -280,10 +274,10 @@ struct drm_driver {
>  	 * move it to &struct drm_crtc_helper_funcs, like all the other
>  	 * helper-internal hooks.
>  	 */
> -	int (*get_scanout_position) (struct drm_device *dev, unsigned int pipe,
> -				     unsigned int flags, int *vpos, int *hpos,
> -				     ktime_t *stime, ktime_t *etime,
> -				     const struct drm_display_mode *mode);
> +	bool (*get_scanout_position) (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);
>  
>  	/**
>  	 * @get_vblank_timestamp:
> 

Reviewed-by: Neil Armstrong <narmstrong at baylibre.com>


More information about the dri-devel mailing list