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@xxxxxxxxxxxxxxx > > > escreveu: > > > > > > > > From: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx> > > > > > > > > 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@xxxxxxxxx> > > > > Cc: Daniel Vetter <daniel@xxxxxxxx> > > > > Signed-off-by: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx> > > > > --- > > > > 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@xxxxxxxxx> > > > > > > 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@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/intel-gfx
