[Intel-gfx] [PATCH 2/9] drm/i915: Add per-pipe plane identifier
Paulo Zanoni
paulo.r.zanoni at intel.com
Fri Nov 18 20:40:27 UTC 2016
Em Sex, 2016-11-18 às 16:32 +0200, Ville Syrjälä escreveu:
> 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 at linux.intel.co
> > > > m
> > > > escreveu:
> > > > >
> > > > >
> > > > > From: Ville Syrjälä <ville.syrjala at 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-Sep
> > > > > temb
> > > > > er/0
> > > > > 76082.html
> > > > >
> > > > > Cc: Matt Roper <matthew.d.roper at intel.com>
> > > > > Cc: Daniel Vetter <daniel at ffwll.ch>
> > > > > Signed-off-by: Ville Syrjälä <ville.syrjala at 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 at 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.
I was talking about my suggestion of PLANE_ID_PRIMARY, etc.
>
> >
> > 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.
Well, at least I tried :).
I agree it's hard to come up with a good name for this thing. I can
definitely live with legacy_plane_id.
>
> 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).
Well, I already gave the r-b for the current version of the patch, so
it's up to you. My goal here was to try to add some suggestions to make
the code even better to read, but they are not required. I won't block
your series based on the renames. Also, you can even do this as an
additional patch at the end of the series if you want. Feel free to
submit v2 with what you think is the best approach.
>
> >
> >
> > 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;
> > > > >
> > >
>
More information about the Intel-gfx
mailing list