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

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

On Fri, Nov 18, 2016 at 12:17:06PM -0200, Paulo Zanoni wrote:
> Em Qui, 2016-11-17 às 21:43 +0200, Ville Syrjälä escreveu:
> > 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-Septemb
> > > > er/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},
> > > };
> > 
> > I did have _ID_ in there originally, but then decided it was just
> > wasted
> > space. Weren't you complaining about me exceeding 80cols already too
> > much? ;)
> 
> I like to have big_descriptive_variable_names but still stay under 80
> columns. It's a design pattern called Masochism.
> 
> > 
> > I915_MAX_PLANES I wanted to leave be since it was already in use, and
> > it
> > does match our weird naming convention for these things. So if we
> > change
> > it we should change all other similar things.
> 
> Makes sense.
> 
> > 
> > > 
> > > 
> > > 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.
> > 
> > Nod.
> > 
> > > 
> > > 
> > > Also, please consider the renames :).
> > 
> > Which ones? The _ID_ thing, or just renaming the entire enum plane to
> > something else? For the latter my best idea was legacy_plane_id, but
> > not sure anyone else likes it. I guess we could also make it
> > primary_plane_id, but like I said that might complicate my long term
> > plan of restoring its use for cursor planes.
> 
> The big problem addressed by this series is: we have two different
> things we call "plane". One refers to the fact that in older hardware
> Plane A isn't necessarily attached to Pipe A, so we need the variable
> to know how things are connected in the hardware, which hardware plane
> we're talking about. The other is for the new platforms, and it refers
> to the Z order of the planes on a pipe.

Not Z order on VLV/CHV as that stuff is actually configurable. But
it's some kind of per pipe plane identifier.

> 
> I think this series does a nice job of using "plane_id" to refer to
> everything related to the Z order (except for the fact that its enum
> doesn't say "id"),

It does. Or yoy mean the actual values? Those don't indeed spell it out.

> but we still use "plane" to refer to the first case.
> Having the "plane" vs "plane_id" is already an improvement, but IMHO
> there's room to make it even better.
> 
> So there are two problems.
> 
> Problem 1 is making sure we use "id" every time we're talking about the
> Z order. IMHO the only missing thing here would be adding "id" to the
> enums.
> 
> Problem 2 is finding a more appropriated name to what's still being
> called just "plane". How about just renaming it to hw_plane? Or really
> just plane_name? plane_selected?

I don't really like any of those. legacy_plane_id is still what I think
I like best.

I mean, we could even go as far as eliminating the enum plane usage
entirely on g4x+, but that would mean duplicating the primary plane code
which would be silly. We have already too much duplication the plane
programming code. For skl+ we shouldn't need it at all since we already
have the plane registers indexed the other way.

The good thing is that the enum plane usage is entirely encapsulated in
the pre-skl primary plane code. So IMO getting the naming just right is
not very important. Limited use, so chances of doing something wrong
are also limited. Although that will change a little eventually as I'll
probably want to start using it a bit more extensively in the pre-g4x
watermark code (when I get around to rewriting it all).

> 
> I'm always in favor of having the dumbest possible code, so I wouldn't
> complain if you just renamed everything to plane_letter and
> plane_number :). I know it sounds super silly, but I bet people would
> quickly understand it.
> 
> Anyway, you already have the R-B tag for the patch since I believe
> we're already better with the code you wrote. Feel free to adopt the
> suggestions you think make more sense. Maybe do some of the renames as
> later patches if you want.
> 
> 
> > 
> > > 
> > > 
> > > > 
> > > > +
> > > >  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;
> > > >  
> > 

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