On Tue, Nov 08, 2016 at 04:53:20PM -0800, Matt Roper wrote: > On Tue, Nov 08, 2016 at 04:47:12PM +0200, ville.syrjala@xxxxxxxxxxxxxxx wrote: > > 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-September/076082.html > > > > Cc: Matt Roper <matthew.d.roper@xxxxxxxxx> > > Cc: Daniel Vetter <daniel@xxxxxxxx> > > Signed-off-by: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx> > > So the goal here is to make intel_plane->plane represent which of the > system's primary planes (A, B, or C) the plane structure refers to? It's the index we pass to DSPCNTR & co. On SKL+ we theoretically don't need it since we index most of plane registers via the plane->id, but there are still a few exceptions left perhaps, which is why I didn't outlaw it fully on SKL+ at this point. > >From a quick Cocci test, it looks like there's only a single use of the > value for that purpose in our driver (in primary_get_hw_state). I think > all of the other calls to DSPCNTR are actually using crtc->plane as > their index, which should have the same value. Would it make more sense > to just drop intel_plane->plane entirely and switch the last user over > to crtc->plane so that we're not carrying around a structure field that > is either bogus or empty on the majority of the platform's planes? crtc->plane needs to die. We want the planes to be independent of crtcs on pre-g4x. > > While we're at it, we could rename 'enum plane' to something like 'enum > primary_plane' to make it extra clear what its purpose is and avoid > future confusion. And maybe a similar rename to crtc->plane as well. > We use the standalone term 'plane' in a generic manner in too many > places in our driver and it means something slightly different > everywhere... I think I want to resurrect plane->plane for cursor at some point since on gen2/3 cursors can move between pipes as well. Alternative we could add some other enum for those. Anyways, I was thinking of calling this thing legacy_plane_id or something like that, but couldn't really convince myself that any particular name was good, so I left it as is for now. > > > Matt > > > --- > > 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, > > +}; > > + > > 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; > > > > -- > > 2.7.4 > > > > -- > Matt Roper > Graphics Software Engineer > IoTG Platform Enabling & Development > Intel Corporation > (916) 356-2795 -- Ville Syrjälä Intel OTC _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/intel-gfx
