On Thu, Sep 24, 2015 at 06:35:35PM +0200, Thierry Reding wrote:
> From: Thierry Reding <treding at nvidia.com>
>
> None of the drivers use this in legacy mode, so it can be converted to
> use struct drm_crtc * directly. While at it, also make the sole user of
> the callback, drm_calc_vbltimestamp_from_scanoutpos(), pass through the
> CRTC directly.
>
> v2: use standard [CRTC:%u] format
I very much like this as a first small step towards untangling drm_irq.c.
Two comments below.
>
> Cc: Daniel Vetter <daniel.vetter at intel.com>
> Cc: Jani Nikula <jani.nikula at linux.intel.com>
> Cc: Ben Skeggs <bskeggs at redhat.com>
> Cc: Alex Deucher <alexander.deucher at amd.com>
> Cc: Christian König <christian.koenig at amd.com>
> Signed-off-by: Thierry Reding <treding at nvidia.com>
> diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c
> index 4a5dee5cd327..525bd82ab514 100644
> --- a/drivers/gpu/drm/drm_irq.c
> +++ b/drivers/gpu/drm/drm_irq.c
> @@ -653,8 +653,7 @@ EXPORT_SYMBOL(drm_calc_timestamping_constants);
>
> /**
> * drm_calc_vbltimestamp_from_scanoutpos - precise vblank timestamp helper
> - * @dev: DRM device
> - * @pipe: index of CRTC whose vblank timestamp to retrieve
> + * @crtc: 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 struct timeval which should receive the timestamp
> @@ -696,13 +695,13 @@ EXPORT_SYMBOL(drm_calc_timestamping_constants);
> * DRM_VBLANKTIME_INVBL - Timestamp taken while scanout was in vblank
> interval.
> *
> */
> -int drm_calc_vbltimestamp_from_scanoutpos(struct drm_device *dev,
> - unsigned int pipe,
> +int drm_calc_vbltimestamp_from_scanoutpos(struct drm_crtc *crtc,
> int *max_error,
> struct timeval *vblank_time,
> unsigned flags,
> const struct drm_display_mode *mode)
This function is actually more a helper, since if you use some hardware
vblank timestamp register you don't really need it at all. Hence I think
we should fix this properly and instead move this function into a new
drm_irq_helper.c (part of drm_kms_helper.ko) - there will be more once
drm_irq.c is untangled. And also push the callback into the corresponding
helper funcs sturctures.
> {
> + const struct drm_crtc_funcs *funcs = crtc->funcs;
> struct timeval tv_etime;
> ktime_t stime, etime;
> unsigned int vbl_status;
> @@ -710,22 +709,16 @@ int drm_calc_vbltimestamp_from_scanoutpos(struct
> drm_device *dev,
> int vpos, hpos, i;
> int delta_ns, duration_ns;
>
> - if (pipe >= dev->num_crtcs) {
> - DRM_ERROR("Invalid crtc %u\n", pipe);
> - return -EINVAL;
> - }
> -
> /* 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");
> - return -EIO;
> - }
> + if (WARN_ON(funcs->get_scanout_position == NULL))
> + return -ENOSYS;
>
> /* If mode timing undefined, just return as no-op:
> * Happens during initial modesetting of a crtc.
> */
> if (mode->crtc_clock == 0) {
> - DRM_DEBUG("crtc %u: Noop due to uninitialized mode.\n", pipe);
> + DRM_DEBUG("[CRTC:%u] Noop due to uninitialized mode.\n",
> + crtc->base.id);
> return -EAGAIN;
> }
>
> @@ -741,15 +734,14 @@ int 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, flags,
> - &vpos, &hpos,
> - &stime, &etime,
> - mode);
> + vbl_status = funcs->get_scanout_position(crtc, flags, &vpos,
> + &hpos, &stime, &etime,
> + mode);
>
> /* Return as no-op if scanout query unsupported or failed. */
> if (!(vbl_status & DRM_SCANOUTPOS_VALID)) {
> - DRM_DEBUG("crtc %u : scanoutpos query failed [0x%x].\n",
> - pipe, vbl_status);
> + DRM_DEBUG("[CRTC:%u] scanoutpos query failed [%d].\n",
> + crtc->base.id, vbl_status);
> return -EIO;
> }
>
> @@ -763,8 +755,8 @@ int drm_calc_vbltimestamp_from_scanoutpos(struct
> drm_device *dev,
>
> /* Noisy system timing? */
> if (i == DRM_TIMESTAMP_MAXRETRIES) {
> - DRM_DEBUG("crtc %u: Noisy timestamp %d us > %d us [%d reps].\n",
> - pipe, duration_ns/1000, *max_error/1000, i);
> + DRM_DEBUG("[CRTC:%u] Noisy timestamp %d us > %d us [%d
> reps].\n",
> + crtc->base.id, duration_ns/1000, *max_error/1000, i);
> }
>
> /* Return upper bound of timestamp precision error. */
> @@ -799,8 +791,8 @@ int drm_calc_vbltimestamp_from_scanoutpos(struct
> drm_device *dev,
> etime = ktime_sub_ns(etime, delta_ns);
> *vblank_time = ktime_to_timeval(etime);
>
> - DRM_DEBUG("crtc %u : v 0x%x p(%d,%d)@ %ld.%ld -> %ld.%ld [e %d us, %d
> rep]\n",
> - pipe, vbl_status, hpos, vpos,
> + DRM_DEBUG("[CRTC:%u] v 0x%x p(%d,%d)@ %ld.%ld -> %ld.%ld [e %d us, %d
> rep]\n",
> + crtc->base.id, vbl_status, hpos, vpos,
> (long)tv_etime.tv_sec, (long)tv_etime.tv_usec,
> (long)vblank_time->tv_sec, (long)vblank_time->tv_usec,
> duration_ns/1000, i);
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index 7b3aeb0f8056..6eec529b3a5b 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -767,14 +767,15 @@ static int __intel_get_crtc_scanline(struct intel_crtc
> *crtc)
> return (position + crtc->scanline_offset) % vtotal;
> }
>
> -static int i915_get_crtc_scanoutpos(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)
> +int i915_get_crtc_scanoutpos(struct drm_crtc *crtc, unsigned int flags,
> + int *vpos, int *hpos, ktime_t *stime,
> + ktime_t *etime,
> + const struct drm_display_mode *mode)
> {
> + struct drm_device *dev = crtc->dev;
> struct drm_i915_private *dev_priv = dev->dev_private;
> - struct drm_crtc *crtc = dev_priv->pipe_to_crtc_mapping[pipe];
> struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> + enum pipe pipe = intel_crtc->pipe;
> int position;
> int vbl_start, vbl_end, hsync_start, htotal, vtotal;
> bool in_vbl = true;
> @@ -929,7 +930,7 @@ static int 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,
> + return drm_calc_vbltimestamp_from_scanoutpos(crtc, max_error,
> vblank_time, flags,
> &crtc->hwmode);
> }
> @@ -4387,7 +4388,6 @@ void intel_irq_init(struct drm_i915_private *dev_priv)
> dev->vblank_disable_immediate = true;
>
> dev->driver->get_vblank_timestamp = i915_get_vblank_timestamp;
> - dev->driver->get_scanout_position = i915_get_crtc_scanoutpos;
>
> if (IS_CHERRYVIEW(dev_priv)) {
> dev->driver->irq_handler = cherryview_irq_handler;
> diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
> index 683f1421a825..c5c9e316251a 100644
> --- a/include/drm/drm_crtc.h
> +++ b/include/drm/drm_crtc.h
> @@ -307,6 +307,11 @@ struct drm_crtc_state {
> struct drm_atomic_state *state;
> };
>
> +/* get_scanout_position() return flags */
> +#define DRM_SCANOUTPOS_VALID (1 << 0)
> +#define DRM_SCANOUTPOS_IN_VBLANK (1 << 1)
> +#define DRM_SCANOUTPOS_ACCURATE (1 << 2)
> +
> /**
> * struct drm_crtc_funcs - control CRTCs for a given device
> * @save: save CRTC state
> @@ -326,6 +331,7 @@ struct drm_crtc_state {
> * (do not call directly, use drm_atomic_crtc_set_property())
> * @atomic_get_property: get a property on an atomic state for this CRTC
> * (do not call directly, use drm_atomic_crtc_get_property())
> + * @get_scanout_position: return the current scanout position
> *
> * The drm_crtc_funcs structure is the central CRTC management structure
> * in the DRM. Each CRTC controls one or more connectors (note that the name
> @@ -389,6 +395,40 @@ struct drm_crtc_funcs {
> const struct drm_crtc_state *state,
> struct drm_property *property,
> uint64_t *val);
> +
> + /**
Please use the new inline structure documentation layout for this (merged
into 4.3): Just drop the @get_scanout_position: from the top-level comment
and add that here.
-Daniel
> + * Called by vblank timestamping code.
> + *
> + * Return the current display scanout position from a crtc, and an
> + * optional accurate ktime_get timestamp of when position was measured.
> + *
> + * \param crtc CRTC to query.
> + * \param flags Flags from the caller (DRM_CALLED_FROM_VBLIRQ or 0).
> + * \param *vpos Target location for current vertical scanout position.
> + * \param *hpos Target location for current horizontal scanout position.
> + * \param *stime Target location for timestamp taken immediately before
> + * scanout position query. Can be NULL to skip timestamp.
> + * \param *etime Target location for timestamp taken immediately after
> + * scanout position query. Can be NULL to skip timestamp.
> + * \param 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."
> + *
> + * \return 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.
> + */
> + int (*get_scanout_position)(struct drm_crtc *crtc, unsigned int flags,
> + int *vpos, int *hpos, ktime_t *stime,
> + ktime_t *etime,
> + const struct drm_display_mode *mode);
> };
>
> /**
> @@ -1194,6 +1234,12 @@ extern int drm_crtc_init_with_planes(struct drm_device
> *dev,
> extern void drm_crtc_cleanup(struct drm_crtc *crtc);
> extern unsigned int drm_crtc_index(struct drm_crtc *crtc);
>
> +extern int drm_calc_vbltimestamp_from_scanoutpos(struct drm_crtc *crtc,
> + int *max_error,
> + struct timeval *vblank_time,
> + unsigned flags,
> + const struct drm_display_mode
> *mode);
> +
> /**
> * drm_crtc_mask - find the mask of a registered CRTC
> * @crtc: CRTC to find mask for
> --
> 2.5.0
>
> _______________________________________________
> dri-devel mailing list
> dri-devel at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
