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

[Paulo Zanoni <paulo.r.zanoni@intel.com>]

Em Ter, 2016-11-08 às 16:47 +0200, ville.syrjala@linux.intel.com
escreveu:
> 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/0
> 76082.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>
> ---
>  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,
> +};

We now have two different enums defining PLANE_SOMETHING, and we even
moved some stuff from one to the other. I think this adds more
confusion to the code, so we would probably be saner with:

enum plane_id {
	PLANE_ID_PRIMARY,
	PLANE_ID_SPRITE0,
	PLANE_ID_SPRITE1,
	PLANE_ID_CURSOR,
	PLANE_ID_{MAX,NUM,TOTAL,SOMETHINGELSE},
};

Otherwise, the patch does what it says, so:
Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>

But please get Matt's ack before merging the series since he's been
touching this area of the code for his work on changing how we treat
the plane cursor.

Also, please consider the renames :).

> +
>  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;
>  
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx