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

[Paulo Zanoni <paulo.r.zanoni@intel.com>]

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@linux.intel.co
> > > > m
> > > > 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...
> 
> > 
> > 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.

> 
> > 
> > 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@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@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx