[Intel-gfx] [PATCH 8/9] drm/i915: Don't populate plane->plane for cursors and sprites
Ander Conselvan De Oliveira
conselvan2 at gmail.com
Fri Dec 16 14:56:06 UTC 2016
On Fri, 2016-12-16 at 12:07 -0200, Paulo Zanoni wrote:
> Em Qui, 2016-12-15 às 21:59 +0200, Ville Syrjälä escreveu:
> >
> > On Thu, Dec 15, 2016 at 05:50:02PM -0200, Paulo Zanoni wrote:
> > >
> > >
> > > 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.co
> > > > > m
> > > > > 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,
> > Yep. Planes can be moved to any pipe in general.
> >
> > >
> > >
> > > but as far as I understand we don't even implement that.
> > Not at the moment. But I have plans...
> >
> > >
> > >
> > > 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.
> > A plane is a plane, a pipe is a pipe. IMO you're trying to make it
> > more confusing by mixing up the two.
> I think that goes against your original argument to remove the plane-
> >
> > plane assignment for everything except for primary planes.
> Anyway, your argumentation is very short and dismissive, it's hard to
> understand what exactly you are thinking and try to move the discussion
> forward.
>
> I suppose we'll just have to agree to disagree here. In one solution we
> create an inconsistency where the plane struct has a field that's only
> applicable and can only be used to some specific planes (even though
> "every plane is a plane"). In the other solution we create the
> inconsistency where the crtc<->plane connection is assigned in the way
> not chosen by the hardware and this may be a problem if we ever get to
> implement the plane assignment flexibility that's allowed by the
> hardware. Maybe having a 3rd opinion here would help.
I find the two plane fields in struct intel_plane a bit confusing, but I don't
think the right solution is to leave that in intel_crtc. If I understand
correctly, we have a problem of uniquely identifying a plane, since that takes
different forms in different platforms. Neither of the generically named enums,
enum plane and enum plane_id, is sufficiently generic.
Would it make sense to turn things up-side down? Now we are trying to make a
crtc be able to find the proper registers for a plane. Instead we could just not
do it and define everything in terms of functions that take planes. Then we
could hide the details of mapping some bit of information in intel_plane to the
appropriate registers in platform-dependent vfuncs. And perhaps give those enums
a platform-dependent name, i.e., enum i8xx_plane_id, enum skl_plane_id, etc.
Anyway, I haven't looked much into it, so just ignore me if I'm way off.
Ander
> >
> >
> > >
> > >
> > > 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.
> > If you don't mix planes and pipes there can't be any confusion.
> >
> > >
> > >
> > >
> > > (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...)
> > From the register POV hw was almost identical up to BDW at least.
> > It's
> > just a few bits (the pipe selector) that vanished from the registers.
> > SKL+ is a little more different but still the registers even live in
> > the
> > same address.
> >
> > Also given what recent history has shown us, I wouldn't be at all
> > surprised if we would go back to a flexible plane<->pipe assignment
> > in the hardware at some point in the future. A lot of the other
> > features that were present in old hardware has been making a comeback
> > in recent years.
> >
> > >
> > >
> > >
> > > >
> > > >
> > > >
> > > > >
> > > > >
> > > > >
> > > > > 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_f
> > > > > orma
> > > > > 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;
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
More information about the Intel-gfx
mailing list