Re: [PATCH v5 03/10] drm/i915: s/enum plane/enum i9xx_plane_id/

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Thu, Nov 16, 2017 at 03:21:32PM -0800, James Ausmus wrote:
> On Mon, Oct 23, 2017 at 05:50:32PM +0300, Ville Syrjala wrote:
> > From: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> > 
> > Rename enum plane to enum i9xx_plane_id to make it clear that it only
> > applies to pre-SKL platforms.
> > 
> > enum i9xx_plane_id is a global identifier, whereas enum plane_id is
> > per-pipe. We need the old global identifier to index the primary plane
> > (and the pre-g4x sprite C if we ever expose it) registers on pre-SKL
> > platforms.
> > 
> > v2: Reorder patches
> > v3: s/old_plane_id/i9xx_plane_id/ (Daniel)
> >     Pimp the commit message a bit
> >     Note that i9xx_plane_id doesn't apply to SKL+
> > v4: Rebase due to power domain handling in plane readout
> > v5: Rebase due to crtc->dspaddr_offset removal
> > 
> > Cc: Daniel Vetter <daniel@xxxxxxxx>
> > Signed-off-by: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> > ---
> >  drivers/gpu/drm/i915/i915_drv.h      |  6 +--
> >  drivers/gpu/drm/i915/intel_display.c | 87 ++++++++++++++++++------------------
> >  drivers/gpu/drm/i915/intel_drv.h     |  6 +--
> >  3 files changed, 49 insertions(+), 50 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> > index 54b5d4c582b6..a6b912c646f9 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.h
> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > @@ -305,9 +305,9 @@ static inline bool transcoder_is_dsi(enum transcoder transcoder)
> >  
> >  /*
> >   * Global legacy plane identifier. Valid only for primary/sprite
> > - * planes on pre-g4x, and only for primary planes on g4x+.
> > + * planes on pre-g4x, and only for primary planes on g4x-bdw.
> >   */
> > -enum plane {
> > +enum i9xx_plane_id {
> >  	PLANE_A,
> >  	PLANE_B,
> >  	PLANE_C,
> > @@ -1137,7 +1137,7 @@ struct intel_fbc {
> >  
> >  		struct {
> >  			enum pipe pipe;
> > -			enum plane plane;
> > +			enum i9xx_plane_id plane;
> >  			unsigned int fence_y_offset;
> >  		} crtc;
> >  
> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > index 4ea0f1ef249e..e726b65588aa 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -3223,16 +3223,16 @@ int i9xx_check_plane_surface(struct intel_plane_state *plane_state)
> >  	return 0;
> >  }
> >  
> > -static void i9xx_update_primary_plane(struct intel_plane *primary,
> > -				      const struct intel_crtc_state *crtc_state,
> > -				      const struct intel_plane_state *plane_state)
> > +static void i9xx_update_plane(struct intel_plane *plane,
> > +			      const struct intel_crtc_state *crtc_state,
> > +			      const struct intel_plane_state *plane_state)
> >  {
> > -	struct drm_i915_private *dev_priv = to_i915(primary->base.dev);
> > +	struct drm_i915_private *dev_priv = to_i915(plane->base.dev);
> >  	const struct drm_framebuffer *fb = plane_state->base.fb;
> > -	enum plane plane = primary->plane;
> > +	enum i9xx_plane_id plane_id = plane->plane;
> 
> It feels a bit ugly and counter-intuitive to have the two "plane"s in
> "plane->plane"

It's always been like that. Well, ever since we had planes.
Nothing new there. At least I got rid of the magic
'plane->plane + 1' from the sprite code.

> be different types - since i9xx_plane_id is a global id,
> would it make sense to change the member naming to plane_gid or some

"gid" would confuse me more. It makes me think of uid/gid. We could name
it to plane->i9xx_plane[_id] I suppose to match the type.

> such (both in struct intel_plane and in struct intel_fbc->crtc)?

The fbc mess is going away thankfully. At which point the uses of
i9xx_plane_id will be tucked away neatly in platform specific code
instead of leaking too badly to common code.

> It
> feels like struct intel_plane should continue to be "plane", but we need
> something else for enum i9xx_plane_id just for clarity's sake.

This is I think the third attempt at coming up with something.

I might just have to rename plane->plane to plane->bikeshed to more
accurately reflect its role in i915 development ;)

> 
> >  	u32 linear_offset;
> >  	u32 dspcntr = plane_state->ctl;
> > -	i915_reg_t reg = DSPCNTR(plane);
> > +	i915_reg_t reg = DSPCNTR(plane_id);
> >  	int x = plane_state->main.x;
> >  	int y = plane_state->main.y;
> >  	unsigned long irqflags;
> > @@ -3251,34 +3251,34 @@ static void i9xx_update_primary_plane(struct intel_plane *primary,
> >  		/* pipesrc and dspsize control the size that is scaled from,
> >  		 * which should always be the user's requested size.
> >  		 */
> > -		I915_WRITE_FW(DSPSIZE(plane),
> > +		I915_WRITE_FW(DSPSIZE(plane_id),
> >  			      ((crtc_state->pipe_src_h - 1) << 16) |
> >  			      (crtc_state->pipe_src_w - 1));
> > -		I915_WRITE_FW(DSPPOS(plane), 0);
> > -	} else if (IS_CHERRYVIEW(dev_priv) && plane == PLANE_B) {
> > -		I915_WRITE_FW(PRIMSIZE(plane),
> > +		I915_WRITE_FW(DSPPOS(plane_id), 0);
> > +	} else if (IS_CHERRYVIEW(dev_priv) && plane_id == PLANE_B) {
> > +		I915_WRITE_FW(PRIMSIZE(plane_id),
> >  			      ((crtc_state->pipe_src_h - 1) << 16) |
> >  			      (crtc_state->pipe_src_w - 1));
> > -		I915_WRITE_FW(PRIMPOS(plane), 0);
> > -		I915_WRITE_FW(PRIMCNSTALPHA(plane), 0);
> > +		I915_WRITE_FW(PRIMPOS(plane_id), 0);
> > +		I915_WRITE_FW(PRIMCNSTALPHA(plane_id), 0);
> >  	}
> >  
> >  	I915_WRITE_FW(reg, dspcntr);
> >  
> > -	I915_WRITE_FW(DSPSTRIDE(plane), fb->pitches[0]);
> > +	I915_WRITE_FW(DSPSTRIDE(plane_id), fb->pitches[0]);
> >  	if (IS_HASWELL(dev_priv) || IS_BROADWELL(dev_priv)) {
> > -		I915_WRITE_FW(DSPSURF(plane),
> > +		I915_WRITE_FW(DSPSURF(plane_id),
> >  			      intel_plane_ggtt_offset(plane_state) +
> >  			      dspaddr_offset);
> > -		I915_WRITE_FW(DSPOFFSET(plane), (y << 16) | x);
> > +		I915_WRITE_FW(DSPOFFSET(plane_id), (y << 16) | x);
> >  	} else if (INTEL_GEN(dev_priv) >= 4) {
> > -		I915_WRITE_FW(DSPSURF(plane),
> > +		I915_WRITE_FW(DSPSURF(plane_id),
> >  			      intel_plane_ggtt_offset(plane_state) +
> >  			      dspaddr_offset);
> > -		I915_WRITE_FW(DSPTILEOFF(plane), (y << 16) | x);
> > -		I915_WRITE_FW(DSPLINOFF(plane), linear_offset);
> > +		I915_WRITE_FW(DSPTILEOFF(plane_id), (y << 16) | x);
> > +		I915_WRITE_FW(DSPLINOFF(plane_id), linear_offset);
> >  	} else {
> > -		I915_WRITE_FW(DSPADDR(plane),
> > +		I915_WRITE_FW(DSPADDR(plane_id),
> >  			      intel_plane_ggtt_offset(plane_state) +
> >  			      dspaddr_offset);
> >  	}
> > @@ -3287,32 +3287,31 @@ static void i9xx_update_primary_plane(struct intel_plane *primary,
> >  	spin_unlock_irqrestore(&dev_priv->uncore.lock, irqflags);
> >  }
> >  
> > -static void i9xx_disable_primary_plane(struct intel_plane *primary,
> > -				       struct intel_crtc *crtc)
> > +static void i9xx_disable_plane(struct intel_plane *plane,
> > +			       struct intel_crtc *crtc)
> >  {
> > -	struct drm_i915_private *dev_priv = to_i915(primary->base.dev);
> > -	enum plane plane = primary->plane;
> > +	struct drm_i915_private *dev_priv = to_i915(plane->base.dev);
> > +	enum i9xx_plane_id plane_id = plane->plane;
> >  	unsigned long irqflags;
> >  
> >  	spin_lock_irqsave(&dev_priv->uncore.lock, irqflags);
> >  
> > -	I915_WRITE_FW(DSPCNTR(plane), 0);
> > -	if (INTEL_INFO(dev_priv)->gen >= 4)
> > -		I915_WRITE_FW(DSPSURF(plane), 0);
> > +	I915_WRITE_FW(DSPCNTR(plane_id), 0);
> > +	if (INTEL_GEN(dev_priv) >= 4)
> 
> Nit: unrelated change/cleanup that's not mentioned in the commit message
> 
> > +		I915_WRITE_FW(DSPSURF(plane_id), 0);
> >  	else
> > -		I915_WRITE_FW(DSPADDR(plane), 0);
> > -	POSTING_READ_FW(DSPCNTR(plane));
> > +		I915_WRITE_FW(DSPADDR(plane_id), 0);
> > +	POSTING_READ_FW(DSPCNTR(plane_id));
> >  
> >  	spin_unlock_irqrestore(&dev_priv->uncore.lock, irqflags);
> >  }
> >  
> > -static bool i9xx_plane_get_hw_state(struct intel_plane *primary)
> > +static bool i9xx_plane_get_hw_state(struct intel_plane *plane)
> >  {
> > -
> > -	struct drm_i915_private *dev_priv = to_i915(primary->base.dev);
> > +	struct drm_i915_private *dev_priv = to_i915(plane->base.dev);
> >  	enum intel_display_power_domain power_domain;
> > -	enum plane plane = primary->plane;
> > -	enum pipe pipe = primary->pipe;
> > +	enum i9xx_plane_id plane_id = plane->plane;
> > +	enum pipe pipe = plane->pipe;
> >  	bool ret;
> >  
> >  	/*
> > @@ -3324,7 +3323,7 @@ static bool i9xx_plane_get_hw_state(struct intel_plane *primary)
> >  	if (!intel_display_power_get_if_enabled(dev_priv, power_domain))
> >  		return false;
> >  
> > -	ret = I915_READ(DSPCNTR(plane)) & DISPLAY_PLANE_ENABLE;
> > +	ret = I915_READ(DSPCNTR(plane_id)) & DISPLAY_PLANE_ENABLE;
> >  
> >  	intel_display_power_put(dev_priv, power_domain);
> >  
> > @@ -13176,9 +13175,9 @@ intel_primary_plane_create(struct drm_i915_private *dev_priv, enum pipe pipe)
> >  	 * 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;
> > +		primary->plane = (enum i9xx_plane_id) !pipe;
> >  	else
> > -		primary->plane = (enum plane) pipe;
> > +		primary->plane = (enum i9xx_plane_id) pipe;
> >  	primary->id = PLANE_PRIMARY;
> >  	primary->frontbuffer_bit = INTEL_FRONTBUFFER_PRIMARY(pipe);
> >  	primary->check_plane = intel_check_primary_plane;
> > @@ -13207,16 +13206,16 @@ intel_primary_plane_create(struct drm_i915_private *dev_priv, enum pipe pipe)
> >  		num_formats = ARRAY_SIZE(i965_primary_formats);
> >  		modifiers = i9xx_format_modifiers;
> >  
> > -		primary->update_plane = i9xx_update_primary_plane;
> > -		primary->disable_plane = i9xx_disable_primary_plane;
> > +		primary->update_plane = i9xx_update_plane;
> > +		primary->disable_plane = i9xx_disable_plane;
> >  		primary->get_hw_state = i9xx_plane_get_hw_state;
> >  	} else {
> >  		intel_primary_formats = i8xx_primary_formats;
> >  		num_formats = ARRAY_SIZE(i8xx_primary_formats);
> >  		modifiers = i9xx_format_modifiers;
> >  
> > -		primary->update_plane = i9xx_update_primary_plane;
> > -		primary->disable_plane = i9xx_disable_primary_plane;
> > +		primary->update_plane = i9xx_update_plane;
> > +		primary->disable_plane = i9xx_disable_plane;
> >  		primary->get_hw_state = i9xx_plane_get_hw_state;
> >  	}
> >  
> > @@ -13300,7 +13299,7 @@ intel_cursor_plane_create(struct drm_i915_private *dev_priv,
> >  	cursor->can_scale = false;
> >  	cursor->max_downscale = 1;
> >  	cursor->pipe = pipe;
> > -	cursor->plane = pipe;
> > +	cursor->plane = (enum i9xx_plane_id) pipe;
> >  	cursor->id = PLANE_CURSOR;
> >  	cursor->frontbuffer_bit = INTEL_FRONTBUFFER_CURSOR(pipe);
> >  
> > @@ -14674,11 +14673,11 @@ void i830_disable_pipe(struct drm_i915_private *dev_priv, enum pipe pipe)
> >  }
> >  
> >  static bool intel_plane_mapping_ok(struct intel_crtc *crtc,
> > -				   struct intel_plane *primary)
> > +				   struct intel_plane *plane)
> >  {
> >  	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
> > -	enum plane plane = primary->plane;
> > -	u32 val = I915_READ(DSPCNTR(plane));
> > +	enum i9xx_plane_id plane_id = plane->plane;
> > +	u32 val = I915_READ(DSPCNTR(plane_id));
> >  
> >  	return (val & DISPLAY_PLANE_ENABLE) == 0 ||
> >  		(val & DISPPLANE_SEL_PIPE_MASK) == DISPPLANE_SEL_PIPE(crtc->pipe);
> > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> > index 07ba376c6fff..8e20924ab9df 100644
> > --- a/drivers/gpu/drm/i915/intel_drv.h
> > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > @@ -796,7 +796,7 @@ struct intel_crtc_state {
> >  struct intel_crtc {
> >  	struct drm_crtc base;
> >  	enum pipe pipe;
> > -	enum plane plane;
> > +	enum i9xx_plane_id plane;
> >  	/*
> >  	 * Whether the crtc and the connected output pipeline is active. Implies
> >  	 * that crtc->enabled is set, i.e. the current mode configuration has
> > @@ -841,7 +841,7 @@ struct intel_crtc {
> >  
> >  struct intel_plane {
> >  	struct drm_plane base;
> > -	u8 plane;
> > +	enum i9xx_plane_id plane;
> >  	enum plane_id id;
> >  	enum pipe pipe;
> >  	bool can_scale;
> > @@ -1128,7 +1128,7 @@ intel_get_crtc_for_pipe(struct drm_i915_private *dev_priv, enum pipe pipe)
> >  }
> >  
> >  static inline struct intel_crtc *
> > -intel_get_crtc_for_plane(struct drm_i915_private *dev_priv, enum plane plane)
> > +intel_get_crtc_for_plane(struct drm_i915_private *dev_priv, enum i9xx_plane_id plane)
> >  {
> >  	return dev_priv->plane_to_crtc_mapping[plane];
> >  }
> > -- 
> > 2.13.6
> > 
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx




[Index of Archives]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]
  Powered by Linux