Re: [PATCH 8/9] drm/i915: Don't populate plane->plane for cursors and sprites

[Matt Roper <matthew.d.roper@intel.com>]

On Thu, Dec 15, 2016 at 09:59:51PM +0200, Ville Syrjälä wrote:
> 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@linux.intel.com
> > > > escreveu:
> > > > > 
> > > > > From: Ville Syrjälä <ville.syrjala@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...

I'm not even sure where to find bspecs for the really old
platforms...how flexible were those primary planes?  Can we drive both
"primary" planes at the same time on a single CRTC if we're not using
the second pipe?  If not, is there some other benefit we get from not
enforcing a fixed pipe-plane association?


Matt

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

-- 
Matt Roper
Graphics Software Engineer
IoTG Platform Enabling & Development
Intel Corporation
(916) 356-2795
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx