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

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Tue, Dec 16, 2014 at 06:00:14PM +0100, Thierry Reding wrote:
> From: Thierry Reding <treding@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.
> 
> Cc: Daniel Vetter <daniel.vetter@intel.com>
> Cc: Jani Nikula <jani.nikula@linux.intel.com>
> Cc: Ben Skeggs <bskeggs@redhat.com>
> Cc: Alex Deucher <alexander.deucher@amd.com>
> Cc: Christian König <christian.koenig@amd.com>
> Signed-off-by: Thierry Reding <treding@nvidia.com>
> ---
> Note: This is compile-tested only. It is a straightforward conversion so
> I wouldn't expect any fallout, but it'd certainly be best if this can be
> tested on all three drivers.
> 
>  drivers/gpu/drm/drm_irq.c                 | 34 +++++++++-------------
>  drivers/gpu/drm/i915/i915_irq.c           | 15 +++++-----
>  drivers/gpu/drm/i915/intel_display.c      |  1 +
>  drivers/gpu/drm/i915/intel_drv.h          |  3 ++
>  drivers/gpu/drm/nouveau/dispnv04/crtc.c   |  1 +
>  drivers/gpu/drm/nouveau/nouveau_display.c | 25 ++++------------
>  drivers/gpu/drm/nouveau/nouveau_display.h |  4 +--
>  drivers/gpu/drm/nouveau/nouveau_drm.c     |  1 -
>  drivers/gpu/drm/nouveau/nv50_display.c    |  1 +
>  drivers/gpu/drm/radeon/radeon_display.c   | 39 ++++++++++++-------------
>  drivers/gpu/drm/radeon/radeon_drv.c       |  1 -
>  drivers/gpu/drm/radeon/radeon_kms.c       |  2 +-
>  drivers/gpu/drm/radeon/radeon_mode.h      |  2 +-
>  drivers/gpu/drm/radeon/radeon_pm.c        |  4 ++-
>  include/drm/drmP.h                        | 47 -------------------------------
>  include/drm/drm_crtc.h                    | 45 +++++++++++++++++++++++++++++
>  16 files changed, 104 insertions(+), 121 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c
> index 37b536c57cd2..18f1ccad7ee0 100644
> --- a/drivers/gpu/drm/drm_irq.c
> +++ b/drivers/gpu/drm/drm_irq.c
> @@ -161,14 +161,14 @@ 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_crtc *refcrtc,
>  					  const struct drm_display_mode *mode)
>  {
> +	const struct drm_crtc_funcs *funcs = crtc->funcs;
>  	struct timeval tv_etime;
>  	ktime_t stime, etime;
>  	int vbl_status;
> @@ -176,16 +176,9 @@ int drm_calc_vbltimestamp_from_scanoutpos(struct drm_device *dev,
>  	int framedur_ns, linedur_ns, pixeldur_ns, delta_ns, duration_ns;
>  	bool invbl;
>  
> -	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;
>  
>  	/* Durations of frames, lines, pixels in nanoseconds. */
>  	framedur_ns = refcrtc->framedur_ns;
> @@ -196,7 +189,8 @@ int drm_calc_vbltimestamp_from_scanoutpos(struct drm_device *dev,
>  	 * Happens during initial modesetting of a crtc.
>  	 */
>  	if (framedur_ns == 0) {
> -		DRM_DEBUG("crtc %u: Noop due to uninitialized mode.\n", pipe);
> +		DRM_DEBUG("crtc %x: Noop due to uninitialized mode.\n",
> +			  crtc->base.id);

I don't think we want the obj ID in hex.

And I must say I hate looking at obj IDs. You basically need to build
some kind of obj ID to hw crtc cross reference every time you try to
read a log. So keeping to drm_crtc_index() would better IMO. Or maybe
even add crtc->name so each driver could provide its own name for the
thing, but that would be a slightly bigger task.

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx