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

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

On Thu, Nov 17, 2016 at 05:09:38PM -0200, Paulo Zanoni wrote:
> 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.

Don't worry about conflicting with my series for now.  My patches are
probably on hold for a while until we deal with the stuff being
discussed on the other mailing list.  I'll take care of updating and
rebasing my patches when that's straightened out.


Matt

> 
> 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

-- 
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