[Intel-gfx] [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 Intel-gfx
mailing list