Re: [PATCH 13/16] drm: Move ->get_scanout_position() to struct drm_crtc_funcs

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Thu, Sep 24, 2015 at 06:35:35PM +0200, Thierry Reding wrote:
> From: Thierry Reding <treding@xxxxxxxxxx>
> 
> 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@xxxxxxxxx>
> Cc: Jani Nikula <jani.nikula@xxxxxxxxxxxxxxx>
> Cc: Ben Skeggs <bskeggs@xxxxxxxxxx>
> Cc: Alex Deucher <alexander.deucher@xxxxxxx>
> Cc: Christian König <christian.koenig@xxxxxxx>
> Signed-off-by: Thierry Reding <treding@xxxxxxxxxx>


> 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@xxxxxxxxxxxxxxxxxxxxx
> http://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/dri-devel




[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux