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@xxxxxxxxxxxxxxx
> > > escreveu:
> > > >
> > > > From: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> > > >
> > > > 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.
> 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_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@xxxxxxxxxxxxxxx>
> > > > ---
> > > > 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;
> >
--
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
