[Intel-gfx] [PATCH 8/9] drm/i915: Don't populate plane->plane for cursors and sprites
Paulo Zanoni
paulo.r.zanoni at intel.com
Thu Dec 15 19:50:02 UTC 2016
Em Qui, 2016-12-15 às 21:11 +0200, Ville Syrjälä escreveu:
> On Thu, Dec 15, 2016 at 05:05:05PM -0200, Paulo Zanoni wrote:
> >
> > Em Ter, 2016-11-22 às 18:02 +0200, ville.syrjala at linux.intel.com
> > escreveu:
> > >
> > > From: Ville Syrjälä <ville.syrjala at linux.intel.com>
> > >
> > > With plane->plane now purely reserved for the primary planes,
> > > let's
> > > not even populate it for cursors and sprites. Let's switch the
> > > type
> > > to enum plane as well since it's no longer being abused for
> > > anything
> > > else.
> >
> > Since it's a primary plane thing, I think it makes more sense to
> > just
> > leave it at struct intel_crtc instead of having a field that's
> > unused
> > and doesn't make sense in some cases.
>
> Primary plane aren't really primary planes on old platforms though.
> That's just a software concept that has no bearing on the hardware.
Please explain more since that's not my understanding. I just opened
the gen 2 and 3 spec docs and they do contain tons of "primary plane"
references. I know that the hardware is a little more flexible there,
but as far as I understand we don't even implement that. Besides, both
our driver and our API do have the concept of a primary plane, so it
makes sense for the code to be organized that way, IMHO. Besides, I
think our code organizations should always better fit the new hardware,
since otherwise we'll make i915.ko development even harder than it is
for new contributors/employees.
(And when I see those specs and realize how different the HW was, then
I remember that in order to explain to the new people why some things
are the way they are in our code I have to explain how the HW was 10
years ago, I start thinking again that maybe we should just have i915-
old.ko and i915-new.ko, since either we make our abstractions/design
fit one thing or we make it fit another...)
>
> >
> > The crtc struct already includes
> > some fields that are specific to primary planes, so I think it's a
> > good
> > place. Or we could create a new class: struct intel_primary_plane {
> > struct intel_plane base; enum plane legacy_plane };
> >
> > Following is a patch suggestion (probably impossible to apply due
> > to my
> > editor breaking long lines). Any arguments against it?
> >
> > --8<----------------------------------------------------------
> >
> > diff --git a/drivers/gpu/drm/i915/intel_display.c
> > b/drivers/gpu/drm/i915/intel_display.c
> > index bc1af87..c54b1a7 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -15065,14 +15065,6 @@ intel_primary_plane_create(struct
> > drm_i915_private *dev_priv, enum pipe pipe)
> > state->scaler_id = -1;
> > }
> > primary->pipe = pipe;
> > - /*
> > - * On gen2/3 only plane A can do FBC, but the panel fitter
> > and
> > LVDS
> > - * port is hooked to pipe B. Hence we want plane A feeding
> > pipe B.
> > - */
> > - if (HAS_FBC(dev_priv) && INTEL_GEN(dev_priv) < 4)
> > - 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;
> > @@ -15120,7 +15112,7 @@ intel_primary_plane_create(struct
> > drm_i915_private *dev_priv, enum pipe pipe)
> > 0,
> > &intel_plane_funcs,
> > intel_primary_forma
> > ts,
> > num_formats,
> > DRM_PLANE_TYPE_PRIM
> > ARY,
> > - "plane %c",
> > plane_name(primary->plane));
> > + "plane %c",
> > pipe_name(pipe)); /* FIXME */
> > if (ret)
> > goto fail;
> >
> > @@ -15272,7 +15264,6 @@ intel_cursor_plane_create(struct
> > drm_i915_private *dev_priv, enum pipe pipe)
> > cursor->can_scale = false;
> > 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;
> > @@ -15390,7 +15381,14 @@ static int intel_crtc_init(struct
> > drm_i915_private *dev_priv, enum pipe pipe)
> > goto fail;
> >
> > intel_crtc->pipe = pipe;
> > - intel_crtc->plane = primary->plane;
> > + /*
> > + * On gen2/3 only plane A can do FBC, but the panel fitter
> > and
> > LVDS
> > + * port is hooked to pipe B. Hence we want plane A feeding
> > pipe B.
> > + */
> > + if (HAS_FBC(dev_priv) && INTEL_GEN(dev_priv) < 4)
> > + intel_crtc->plane = (enum plane) !pipe;
> > + else
> > + intel_crtc->plane = (enum plane) pipe;
> >
> > intel_crtc->cursor_base = ~0;
> > intel_crtc->cursor_cntl = ~0;
> > @@ -16866,11 +16864,11 @@ void i915_redisable_vga(struct
> > drm_i915_private *dev_priv)
> > intel_display_power_put(dev_priv, POWER_DOMAIN_VGA);
> > }
> >
> > -static bool primary_get_hw_state(struct intel_plane *plane)
> > +static bool primary_get_hw_state(struct intel_crtc *crtc)
> > {
> > - struct drm_i915_private *dev_priv = to_i915(plane-
> > >base.dev);
> > + struct drm_i915_private *dev_priv = to_i915(crtc-
> > >base.dev);
> >
> > - return I915_READ(DSPCNTR(plane->plane)) &
> > DISPLAY_PLANE_ENABLE;
> > + return I915_READ(DSPCNTR(crtc->plane)) &
> > DISPLAY_PLANE_ENABLE;
> > }
> >
> > /* FIXME read out full plane state for all planes */
> > @@ -16880,8 +16878,7 @@ static void readout_plane_state(struct
> > intel_crtc *crtc)
> > struct intel_plane_state *plane_state =
> > to_intel_plane_state(primary->state);
> >
> > - plane_state->base.visible = crtc->active &&
> > - primary_get_hw_state(to_intel_plane(primary));
> > + plane_state->base.visible = crtc->active &&
> > primary_get_hw_state(crtc);
> >
> > if (plane_state->base.visible)
> > crtc->base.state->plane_mask |= 1 <<
> > drm_plane_index(primary);
> > diff --git a/drivers/gpu/drm/i915/intel_drv.h
> > b/drivers/gpu/drm/i915/intel_drv.h
> > index 362f698..b2bdd49 100644
> > --- a/drivers/gpu/drm/i915/intel_drv.h
> > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > @@ -767,7 +767,6 @@ struct intel_plane_wm_parameters {
> >
> > struct intel_plane {
> > struct drm_plane base;
> > - u8 plane;
> > enum plane_id id;
> > enum pipe pipe;
> > bool can_scale;
> > diff --git a/drivers/gpu/drm/i915/intel_sprite.c
> > b/drivers/gpu/drm/i915/intel_sprite.c
> > index 63154a5..1044095 100644
> > --- a/drivers/gpu/drm/i915/intel_sprite.c
> > +++ b/drivers/gpu/drm/i915/intel_sprite.c
> > @@ -1111,7 +1111,6 @@ 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;
> >
> > --8<----------------------------------------------------------
> >
> > >
> > >
> > > Signed-off-by: Ville Syrjälä <ville.syrjala at linux.intel.com>
> > > ---
> > > drivers/gpu/drm/i915/intel_display.c | 1 -
> > > drivers/gpu/drm/i915/intel_drv.h | 2 +-
> > > drivers/gpu/drm/i915/intel_sprite.c | 1 -
> > > 3 files changed, 1 insertion(+), 3 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/i915/intel_display.c
> > > b/drivers/gpu/drm/i915/intel_display.c
> > > index b6d5d5e5cc99..f180f14fcf3a 100644
> > > --- a/drivers/gpu/drm/i915/intel_display.c
> > > +++ b/drivers/gpu/drm/i915/intel_display.c
> > > @@ -15188,7 +15188,6 @@ intel_cursor_plane_create(struct
> > > drm_i915_private *dev_priv, enum pipe pipe)
> > > cursor->can_scale = false;
> > > 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;
> > > diff --git a/drivers/gpu/drm/i915/intel_drv.h
> > > b/drivers/gpu/drm/i915/intel_drv.h
> > > index c1b245853ba9..d14718e09911 100644
> > > --- a/drivers/gpu/drm/i915/intel_drv.h
> > > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > > @@ -767,7 +767,7 @@ struct intel_plane_wm_parameters {
> > >
> > > struct intel_plane {
> > > struct drm_plane base;
> > > - u8 plane;
> > > + enum plane plane;
> > > enum plane_id id;
> > > enum pipe pipe;
> > > bool can_scale;
> > > diff --git a/drivers/gpu/drm/i915/intel_sprite.c
> > > b/drivers/gpu/drm/i915/intel_sprite.c
> > > index 63154a5a9305..1044095d0084 100644
> > > --- a/drivers/gpu/drm/i915/intel_sprite.c
> > > +++ b/drivers/gpu/drm/i915/intel_sprite.c
> > > @@ -1111,7 +1111,6 @@ 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