Re: [PATCH 2/9] drm/i915: Add per-pipe plane identifier

[Matt Roper <matthew.d.roper@intel.com>]

On Tue, Nov 08, 2016 at 04:47:12PM +0200, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> As I told people in [1] we really should not be confusing enum plane
> as a per-pipe plane identifier. Looks like that happened nonetheless, so
> let's fix it up by splitting the two into two enums.
> 
> We'll also want something we just directly pass to various register
> offset macros and whatnot on SKL+. So let's make this new thing work for that.
> Currently we pass intel_plane->plane for the "sprites" and just a
> hardcoded zero for the "primary" planes. We want to get rid of that
> hardocoding so that we can share the same code for all planes (apart
> from the legacy cursor of course).
> 
> [1] https://lists.freedesktop.org/archives/intel-gfx/2015-September/076082.html
> 
> Cc: Matt Roper <matthew.d.roper@intel.com>
> Cc: Daniel Vetter <daniel@ffwll.ch>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

So the goal here is to make intel_plane->plane represent which of the
system's primary planes (A, B, or C) the plane structure refers to?
From a quick Cocci test, it looks like there's only a single use of the
value for that purpose in our driver (in primary_get_hw_state).  I think
all of the other calls to DSPCNTR are actually using crtc->plane as
their index, which should have the same value.  Would it make more sense
to just drop intel_plane->plane entirely and switch the last user over
to crtc->plane so that we're not carrying around a structure field that
is either bogus or empty on the majority of the platform's planes?

While we're at it, we could rename 'enum plane' to something like 'enum
primary_plane' to make it extra clear what its purpose is and avoid
future confusion.  And maybe a similar rename to crtc->plane as well.
We use the standalone term 'plane' in a generic manner in too many
places in our driver and it means something slightly different
everywhere...


Matt

> ---
>  drivers/gpu/drm/i915/i915_drv.h      | 28 +++++++++++++++++++++-------
>  drivers/gpu/drm/i915/intel_display.c |  2 ++
>  drivers/gpu/drm/i915/intel_drv.h     |  3 ++-
>  drivers/gpu/drm/i915/intel_sprite.c  |  1 +
>  4 files changed, 26 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 30777dee3f9c..2451b88b1e82 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -171,22 +171,36 @@ static inline bool transcoder_is_dsi(enum transcoder transcoder)
>  }
>  
>  /*
> - * I915_MAX_PLANES in the enum below is the maximum (across all platforms)
> - * number of planes per CRTC.  Not all platforms really have this many planes,
> - * which means some arrays of size I915_MAX_PLANES may have unused entries
> - * between the topmost sprite plane and the cursor plane.
> + * Global legacy plane identifier. Valid only for primary/sprite
> + * planes on pre-g4x, and only for primary planes on g4x+.
>   */
>  enum plane {
> -	PLANE_A = 0,
> +	PLANE_A,
>  	PLANE_B,
>  	PLANE_C,
> -	PLANE_CURSOR,
> -	I915_MAX_PLANES,
>  };
>  #define plane_name(p) ((p) + 'A')
>  
>  #define sprite_name(p, s) ((p) * INTEL_INFO(dev_priv)->num_sprites[(p)] + (s) + 'A')
>  
> +/*
> + * Per-pipe plane identifier.
> + * I915_MAX_PLANES in the enum below is the maximum (across all platforms)
> + * number of planes per CRTC.  Not all platforms really have this many planes,
> + * which means some arrays of size I915_MAX_PLANES may have unused entries
> + * between the topmost sprite plane and the cursor plane.
> + *
> + * This is expected to be passed to various register macros
> + * (eg. PLANE_CTL(), PS_PLANE_SEL(), etc.) so adjust with care.
> + */
> +enum plane_id {
> +	PLANE_PRIMARY,
> +	PLANE_SPRITE0,
> +	PLANE_SPRITE1,
> +	PLANE_CURSOR,
> +	I915_MAX_PLANES,
> +};
> +
>  enum port {
>  	PORT_NONE = -1,
>  	PORT_A = 0,
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 10869360cfdc..b318119330e8 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -15008,6 +15008,7 @@ intel_primary_plane_create(struct drm_i915_private *dev_priv, enum pipe pipe)
>  		primary->plane = (enum plane) !pipe;
>  	else
>  		primary->plane = (enum plane) pipe;
> +	primary->id = PLANE_PRIMARY;
>  	primary->frontbuffer_bit = INTEL_FRONTBUFFER_PRIMARY(pipe);
>  	primary->check_plane = intel_check_primary_plane;
>  
> @@ -15203,6 +15204,7 @@ intel_cursor_plane_create(struct drm_i915_private *dev_priv, enum pipe pipe)
>  	cursor->max_downscale = 1;
>  	cursor->pipe = pipe;
>  	cursor->plane = pipe;
> +	cursor->id = PLANE_CURSOR;
>  	cursor->frontbuffer_bit = INTEL_FRONTBUFFER_CURSOR(pipe);
>  	cursor->check_plane = intel_check_cursor_plane;
>  	cursor->update_plane = intel_update_cursor_plane;
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 398195bf6dd1..58fc8e1d2aa8 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -767,7 +767,8 @@ struct intel_plane_wm_parameters {
>  
>  struct intel_plane {
>  	struct drm_plane base;
> -	int plane;
> +	u8 plane;
> +	enum plane_id id;
>  	enum pipe pipe;
>  	bool can_scale;
>  	int max_downscale;
> diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
> index 5e4eb7cafef0..4b44863a07c2 100644
> --- a/drivers/gpu/drm/i915/intel_sprite.c
> +++ b/drivers/gpu/drm/i915/intel_sprite.c
> @@ -1126,6 +1126,7 @@ intel_sprite_plane_create(struct drm_i915_private *dev_priv,
>  
>  	intel_plane->pipe = pipe;
>  	intel_plane->plane = plane;
> +	intel_plane->id = PLANE_SPRITE0 + plane;
>  	intel_plane->frontbuffer_bit = INTEL_FRONTBUFFER_SPRITE(pipe, plane);
>  	intel_plane->check_plane = intel_check_sprite_plane;
>  
> -- 
> 2.7.4
> 

-- 
Matt Roper
Graphics Software Engineer
IoTG Platform Enabling & Development
Intel Corporation
(916) 356-2795
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx