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

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Fri, Nov 17, 2017 at 12:12:15PM -0800, Rodrigo Vivi wrote:
> On Fri, Nov 17, 2017 at 07:19:10PM +0000, Ville Syrjala wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > Rename enum plane to enum i9xx_plane_id to make it clear that it only
> > applies to pre-SKL platforms.
> 
> Oh! I should had read this before commenting on the cover letter... sorry...
> 
> Anyway I don't believe it make clear that it is not skl... because it means
> it started on i9xx, but not that ended on skl... just "plane" was already
> good enough imho... but it is really just a comment...
> don't take this as a nack anyhow... ;)

I think our unwritten rule is that i9xx often refers to
gen2/3/4, sometimes including g4x sometimes not, and sometimes
even vlv/chv gets included.

And sometimes we just call things by the name i915 instead of
i9xx, meaning gmch. But on other occasions i915 means just gen3,
and i9xx means anything else gmch not otherwise named more
specifically.

Got it? Good :)

In this case that "pattern" is a bit more broken since here
i9xx extends all the way to bdw. But it does match the fact that
the primary plane functions are now called i9xx_plane_whatever().

I think I need to include the cursor planes in the i9xx_plane_id
soon enough, which will make it partially extend all the way to cnl
and beyond. We really need to tell the hw people to redesign the
cursor planes so that we have an excuse to split the code :P

> 
> > 
> > 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
> > v6: s/plane/i9xx_plane/ etc. (James)
> > 
> > Cc: James Ausmus <james.ausmus@intel.com>
> > Cc: Daniel Vetter <daniel@ffwll.ch>
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_drv.h      |  6 +--
> >  drivers/gpu/drm/i915/intel_display.c | 98 ++++++++++++++++++------------------
> >  drivers/gpu/drm/i915/intel_drv.h     |  6 +--
> >  drivers/gpu/drm/i915/intel_fbc.c     | 12 ++---
> >  drivers/gpu/drm/i915/intel_sprite.c  |  2 +-
> >  5 files changed, 62 insertions(+), 62 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> > index 2158a758a17d..55dd602582cb 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.h
> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > @@ -304,9 +304,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,
> > @@ -1145,7 +1145,7 @@ struct intel_fbc {
> >  
> >  		struct {
> >  			enum pipe pipe;
> > -			enum plane plane;
> > +			enum i9xx_plane_id i9xx_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 91f74c5373b3..16ac86816f28 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -3220,16 +3220,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 i9xx_plane = plane->i9xx_plane;
> >  	u32 linear_offset;
> >  	u32 dspcntr = plane_state->ctl;
> > -	i915_reg_t reg = DSPCNTR(plane);
> > +	i915_reg_t reg = DSPCNTR(i9xx_plane);
> >  	int x = plane_state->main.x;
> >  	int y = plane_state->main.y;
> >  	unsigned long irqflags;
> > @@ -3248,34 +3248,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(i9xx_plane),
> >  			      ((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(i9xx_plane), 0);
> > +	} else if (IS_CHERRYVIEW(dev_priv) && i9xx_plane == PLANE_B) {
> > +		I915_WRITE_FW(PRIMSIZE(i9xx_plane),
> >  			      ((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(i9xx_plane), 0);
> > +		I915_WRITE_FW(PRIMCNSTALPHA(i9xx_plane), 0);
> >  	}
> >  
> >  	I915_WRITE_FW(reg, dspcntr);
> >  
> > -	I915_WRITE_FW(DSPSTRIDE(plane), fb->pitches[0]);
> > +	I915_WRITE_FW(DSPSTRIDE(i9xx_plane), fb->pitches[0]);
> >  	if (IS_HASWELL(dev_priv) || IS_BROADWELL(dev_priv)) {
> > -		I915_WRITE_FW(DSPSURF(plane),
> > +		I915_WRITE_FW(DSPSURF(i9xx_plane),
> >  			      intel_plane_ggtt_offset(plane_state) +
> >  			      dspaddr_offset);
> > -		I915_WRITE_FW(DSPOFFSET(plane), (y << 16) | x);
> > +		I915_WRITE_FW(DSPOFFSET(i9xx_plane), (y << 16) | x);
> >  	} else if (INTEL_GEN(dev_priv) >= 4) {
> > -		I915_WRITE_FW(DSPSURF(plane),
> > +		I915_WRITE_FW(DSPSURF(i9xx_plane),
> >  			      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(i9xx_plane), (y << 16) | x);
> > +		I915_WRITE_FW(DSPLINOFF(i9xx_plane), linear_offset);
> >  	} else {
> > -		I915_WRITE_FW(DSPADDR(plane),
> > +		I915_WRITE_FW(DSPADDR(i9xx_plane),
> >  			      intel_plane_ggtt_offset(plane_state) +
> >  			      dspaddr_offset);
> >  	}
> > @@ -3284,32 +3284,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 i9xx_plane = plane->i9xx_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(i9xx_plane), 0);
> > +	if (INTEL_GEN(dev_priv) >= 4)
> > +		I915_WRITE_FW(DSPSURF(i9xx_plane), 0);
> >  	else
> > -		I915_WRITE_FW(DSPADDR(plane), 0);
> > -	POSTING_READ_FW(DSPCNTR(plane));
> > +		I915_WRITE_FW(DSPADDR(i9xx_plane), 0);
> > +	POSTING_READ_FW(DSPCNTR(i9xx_plane));
> >  
> >  	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 i9xx_plane = plane->i9xx_plane;
> > +	enum pipe pipe = plane->pipe;
> >  	bool ret;
> >  
> >  	/*
> > @@ -3321,7 +3320,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(i9xx_plane)) & DISPLAY_PLANE_ENABLE;
> >  
> >  	intel_display_power_put(dev_priv, power_domain);
> >  
> > @@ -7406,7 +7405,7 @@ i9xx_get_initial_plane_config(struct intel_crtc *crtc,
> >  	struct drm_device *dev = crtc->base.dev;
> >  	struct drm_i915_private *dev_priv = to_i915(dev);
> >  	u32 val, base, offset;
> > -	int pipe = crtc->pipe, plane = crtc->plane;
> > +	int pipe = crtc->pipe, plane = crtc->i9xx_plane;
> >  	int fourcc, pixel_format;
> >  	unsigned int aligned_height;
> >  	struct drm_framebuffer *fb;
> > @@ -13272,9 +13271,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->i9xx_plane = (enum i9xx_plane_id) !pipe;
> >  	else
> > -		primary->plane = (enum plane) pipe;
> > +		primary->i9xx_plane = (enum i9xx_plane_id) pipe;
> >  	primary->id = PLANE_PRIMARY;
> >  	primary->frontbuffer_bit = INTEL_FRONTBUFFER_PRIMARY(pipe);
> >  	primary->check_plane = intel_check_primary_plane;
> > @@ -13303,16 +13302,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;
> >  	}
> >  
> > @@ -13336,7 +13335,8 @@ intel_primary_plane_create(struct drm_i915_private *dev_priv, enum pipe pipe)
> >  					       intel_primary_formats, num_formats,
> >  					       modifiers,
> >  					       DRM_PLANE_TYPE_PRIMARY,
> > -					       "plane %c", plane_name(primary->plane));
> > +					       "plane %c",
> > +					       plane_name(primary->i9xx_plane));
> >  	if (ret)
> >  		goto fail;
> >  
> > @@ -13396,7 +13396,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->i9xx_plane = (enum i9xx_plane_id) pipe;
> >  	cursor->id = PLANE_CURSOR;
> >  	cursor->frontbuffer_bit = INTEL_FRONTBUFFER_CURSOR(pipe);
> >  
> > @@ -13524,14 +13524,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;
> > +	intel_crtc->i9xx_plane = primary->i9xx_plane;
> >  
> >  	/* initialize shared scalers */
> >  	intel_crtc_init_scalers(intel_crtc, crtc_state);
> >  
> >  	BUG_ON(pipe >= ARRAY_SIZE(dev_priv->plane_to_crtc_mapping) ||
> > -	       dev_priv->plane_to_crtc_mapping[intel_crtc->plane] != NULL);
> > -	dev_priv->plane_to_crtc_mapping[intel_crtc->plane] = intel_crtc;
> > +	       dev_priv->plane_to_crtc_mapping[intel_crtc->i9xx_plane] != NULL);
> > +	dev_priv->plane_to_crtc_mapping[intel_crtc->i9xx_plane] = intel_crtc;
> >  	dev_priv->pipe_to_crtc_mapping[intel_crtc->pipe] = intel_crtc;
> >  
> >  	drm_crtc_helper_add(&intel_crtc->base, &intel_helper_funcs);
> > @@ -14788,11 +14788,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 i9xx_plane = plane->i9xx_plane;
> > +	u32 val = I915_READ(DSPCNTR(i9xx_plane));
> >  
> >  	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 99840f3940c7..d1fe7be94b62 100644
> > --- a/drivers/gpu/drm/i915/intel_drv.h
> > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > @@ -799,7 +799,7 @@ struct intel_crtc_state {
> >  struct intel_crtc {
> >  	struct drm_crtc base;
> >  	enum pipe pipe;
> > -	enum plane plane;
> > +	enum i9xx_plane_id i9xx_plane;
> >  	/*
> >  	 * Whether the crtc and the connected output pipeline is active. Implies
> >  	 * that crtc->enabled is set, i.e. the current mode configuration has
> > @@ -844,7 +844,7 @@ struct intel_crtc {
> >  
> >  struct intel_plane {
> >  	struct drm_plane base;
> > -	u8 plane;
> > +	enum i9xx_plane_id i9xx_plane;
> >  	enum plane_id id;
> >  	enum pipe pipe;
> >  	bool can_scale;
> > @@ -1130,7 +1130,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];
> >  }
> > diff --git a/drivers/gpu/drm/i915/intel_fbc.c b/drivers/gpu/drm/i915/intel_fbc.c
> > index 1a0f5e0c8d10..3133131306a9 100644
> > --- a/drivers/gpu/drm/i915/intel_fbc.c
> > +++ b/drivers/gpu/drm/i915/intel_fbc.c
> > @@ -151,7 +151,7 @@ static void i8xx_fbc_activate(struct drm_i915_private *dev_priv)
> >  
> >  		/* Set it up... */
> >  		fbc_ctl2 = FBC_CTL_FENCE_DBL | FBC_CTL_IDLE_IMM | FBC_CTL_CPU_FENCE;
> > -		fbc_ctl2 |= FBC_CTL_PLANE(params->crtc.plane);
> > +		fbc_ctl2 |= FBC_CTL_PLANE(params->crtc.i9xx_plane);
> >  		I915_WRITE(FBC_CONTROL2, fbc_ctl2);
> >  		I915_WRITE(FBC_FENCE_OFF, params->crtc.fence_y_offset);
> >  	}
> > @@ -177,7 +177,7 @@ static void g4x_fbc_activate(struct drm_i915_private *dev_priv)
> >  	struct intel_fbc_reg_params *params = &dev_priv->fbc.params;
> >  	u32 dpfc_ctl;
> >  
> > -	dpfc_ctl = DPFC_CTL_PLANE(params->crtc.plane) | DPFC_SR_EN;
> > +	dpfc_ctl = DPFC_CTL_PLANE(params->crtc.i9xx_plane) | DPFC_SR_EN;
> >  	if (params->fb.format->cpp[0] == 2)
> >  		dpfc_ctl |= DPFC_CTL_LIMIT_2X;
> >  	else
> > @@ -224,7 +224,7 @@ static void ilk_fbc_activate(struct drm_i915_private *dev_priv)
> >  	u32 dpfc_ctl;
> >  	int threshold = dev_priv->fbc.threshold;
> >  
> > -	dpfc_ctl = DPFC_CTL_PLANE(params->crtc.plane);
> > +	dpfc_ctl = DPFC_CTL_PLANE(params->crtc.i9xx_plane);
> >  	if (params->fb.format->cpp[0] == 2)
> >  		threshold++;
> >  
> > @@ -306,7 +306,7 @@ static void gen7_fbc_activate(struct drm_i915_private *dev_priv)
> >  
> >  	dpfc_ctl = 0;
> >  	if (IS_IVYBRIDGE(dev_priv))
> > -		dpfc_ctl |= IVB_DPFC_CTL_PLANE(params->crtc.plane);
> > +		dpfc_ctl |= IVB_DPFC_CTL_PLANE(params->crtc.i9xx_plane);
> >  
> >  	if (params->fb.format->cpp[0] == 2)
> >  		threshold++;
> > @@ -890,7 +890,7 @@ static void intel_fbc_get_reg_params(struct intel_crtc *crtc,
> >  	params->vma = cache->vma;
> >  
> >  	params->crtc.pipe = crtc->pipe;
> > -	params->crtc.plane = crtc->plane;
> > +	params->crtc.i9xx_plane = crtc->i9xx_plane;
> >  	params->crtc.fence_y_offset = get_crtc_fence_y_offset(fbc);
> >  
> >  	params->fb.format = cache->fb.format;
> > @@ -1088,7 +1088,7 @@ void intel_fbc_choose_crtc(struct drm_i915_private *dev_priv,
> >  		if (fbc_on_pipe_a_only(dev_priv) && crtc->pipe != PIPE_A)
> >  			continue;
> >  
> > -		if (fbc_on_plane_a_only(dev_priv) && crtc->plane != PLANE_A)
> > +		if (fbc_on_plane_a_only(dev_priv) && crtc->i9xx_plane != PLANE_A)
> >  			continue;
> >  
> >  		intel_crtc_state = to_intel_crtc_state(
> > diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
> > index 5baa0023964e..dd485f59eb1d 100644
> > --- a/drivers/gpu/drm/i915/intel_sprite.c
> > +++ b/drivers/gpu/drm/i915/intel_sprite.c
> > @@ -1384,7 +1384,7 @@ intel_sprite_plane_create(struct drm_i915_private *dev_priv,
> >  	}
> >  
> >  	intel_plane->pipe = pipe;
> > -	intel_plane->plane = plane;
> > +	intel_plane->i9xx_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;
> > -- 
> > 2.13.6
> > 
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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