On Tue, Nov 08, 2016 at 04:53:20PM -0800, Matt Roper wrote:
> On Tue, Nov 08, 2016 at 04:47:12PM +0200, ville.syrj...@linux.intel.com wrote:
> > From: Ville Syrjälä <ville.syrj...@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/076082.html
> >
> > Cc: Matt Roper <matthew.d.ro...@intel.com>
> > Cc: Daniel Vetter <dan...@ffwll.ch>
> > Signed-off-by: Ville Syrjälä <ville.syrj...@linux.intel.com>
>
> 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@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
