Re: [PATCH 8/9] drm/i915: Rename the local 'plane' variable to 'plane_id' in primary plane code

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

On Fri, Nov 18, 2016 at 12:25:30PM -0200, Paulo Zanoni wrote:
> Em Ter, 2016-11-08 às 16:47 +0200, ville.syrjala@linux.intel.com
> escreveu:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > Now we've rename the local plane id variable as 'plane_id' everywhere
> > except the pre-SKL primary plane code. Let's do the rename there as
> > well
> > so that we'll free up the name 'plane' for use with struct
> > intel_plane*.
> 
> As you pointed, my second email do patch 7 was supposed to be a reply
> to this patch. So let's move the discussion to the appropriate thread.
> And I'll try to better write my argumentation here.
> 
> I think this series does a really nice job of consistently using the
> term "plane_id" to refer to the plane Z ordering that we have in the
> newer platforms, while keeping "plane" as the variable that refers to
> the HW plane (A/B/C) the struct represents. And, IMHO, this patch goes
> against that trend, since it starts using plane_id to talk about A/B/C.
> 
> I see your goal is to leave the "plane" namespace restricted to
> variables that point to our plane structs, but I think that if we're
> going to do this, we should probably use something different than
> plane_id when talking about the hw a/b/c planes. So maybe using a
> different variable name here would solve the problem.
> 
> What do you think?

Perhaps. But it's hard coming up with a name that pleases the eye
sufficiently.

> 
> > 
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_display.c | 57 +++++++++++++++++---------
> > ----------
> >  1 file changed, 27 insertions(+), 30 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_display.c
> > b/drivers/gpu/drm/i915/intel_display.c
> > index 95644c8cc568..bd084b085421 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -3014,10 +3014,9 @@ static void i9xx_update_primary_plane(struct
> > drm_plane *primary,
> >  	struct drm_i915_private *dev_priv = to_i915(dev);
> >  	struct intel_crtc *intel_crtc = to_intel_crtc(crtc_state-
> > >base.crtc);
> >  	struct drm_framebuffer *fb = plane_state->base.fb;
> > -	int plane = intel_crtc->plane;
> > +	enum plane plane_id = to_intel_plane(primary)->plane;
> >  	u32 linear_offset;
> >  	u32 dspcntr;
> > -	i915_reg_t reg = DSPCNTR(plane);
> >  	unsigned int rotation = plane_state->base.rotation;
> >  	int x = plane_state->base.src.x1 >> 16;
> >  	int y = plane_state->base.src.y1 >> 16;
> > @@ -3033,16 +3032,16 @@ static void i9xx_update_primary_plane(struct
> > drm_plane *primary,
> >  		/* pipesrc and dspsize control the size that is
> > scaled from,
> >  		 * which should always be the user's requested size.
> >  		 */
> > -		I915_WRITE(DSPSIZE(plane),
> > +		I915_WRITE(DSPSIZE(plane_id),
> >  			   ((crtc_state->pipe_src_h - 1) << 16) |
> >  			   (crtc_state->pipe_src_w - 1));
> > -		I915_WRITE(DSPPOS(plane), 0);
> > -	} else if (IS_CHERRYVIEW(dev_priv) && plane == PLANE_B) {
> > -		I915_WRITE(PRIMSIZE(plane),
> > +		I915_WRITE(DSPPOS(plane_id), 0);
> > +	} else if (IS_CHERRYVIEW(dev_priv) && plane_id == PLANE_B) {
> > +		I915_WRITE(PRIMSIZE(plane_id),
> >  			   ((crtc_state->pipe_src_h - 1) << 16) |
> >  			   (crtc_state->pipe_src_w - 1));
> > -		I915_WRITE(PRIMPOS(plane), 0);
> > -		I915_WRITE(PRIMCNSTALPHA(plane), 0);
> > +		I915_WRITE(PRIMPOS(plane_id), 0);
> > +		I915_WRITE(PRIMCNSTALPHA(plane_id), 0);
> >  	}
> >  
> >  	switch (fb->pixel_format) {
> > @@ -3099,21 +3098,21 @@ static void i9xx_update_primary_plane(struct
> > drm_plane *primary,
> >  	intel_crtc->adjusted_x = x;
> >  	intel_crtc->adjusted_y = y;
> >  
> > -	I915_WRITE(reg, dspcntr);
> > +	I915_WRITE(DSPCNTR(plane_id), dspcntr);
> >  
> > -	I915_WRITE(DSPSTRIDE(plane), fb->pitches[0]);
> > +	I915_WRITE(DSPSTRIDE(plane_id), fb->pitches[0]);
> >  	if (INTEL_INFO(dev)->gen >= 4) {
> > -		I915_WRITE(DSPSURF(plane),
> > +		I915_WRITE(DSPSURF(plane_id),
> >  			   intel_fb_gtt_offset(fb, rotation) +
> >  			   intel_crtc->dspaddr_offset);
> > -		I915_WRITE(DSPTILEOFF(plane), (y << 16) | x);
> > -		I915_WRITE(DSPLINOFF(plane), linear_offset);
> > +		I915_WRITE(DSPTILEOFF(plane_id), (y << 16) | x);
> > +		I915_WRITE(DSPLINOFF(plane_id), linear_offset);
> >  	} else {
> > -		I915_WRITE(DSPADDR(plane),
> > +		I915_WRITE(DSPADDR(plane_id),
> >  			   intel_fb_gtt_offset(fb, rotation) +
> >  			   intel_crtc->dspaddr_offset);
> >  	}
> > -	POSTING_READ(reg);
> > +	POSTING_READ(DSPCNTR(plane_id));
> >  }
> >  
> >  static void i9xx_disable_primary_plane(struct drm_plane *primary,
> > @@ -3121,15 +3120,14 @@ static void i9xx_disable_primary_plane(struct
> > drm_plane *primary,
> >  {
> >  	struct drm_device *dev = crtc->dev;
> >  	struct drm_i915_private *dev_priv = to_i915(dev);
> > -	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> > -	int plane = intel_crtc->plane;
> > +	enum plane plane_id = to_intel_plane(primary)->plane;
> >  
> > -	I915_WRITE(DSPCNTR(plane), 0);
> > +	I915_WRITE(DSPCNTR(plane_id), 0);
> >  	if (INTEL_INFO(dev_priv)->gen >= 4)
> > -		I915_WRITE(DSPSURF(plane), 0);
> > +		I915_WRITE(DSPSURF(plane_id), 0);
> >  	else
> > -		I915_WRITE(DSPADDR(plane), 0);
> > -	POSTING_READ(DSPCNTR(plane));
> > +		I915_WRITE(DSPADDR(plane_id), 0);
> > +	POSTING_READ(DSPCNTR(plane_id));
> >  }
> >  
> >  static void ironlake_update_primary_plane(struct drm_plane *primary,
> > @@ -3140,10 +3138,9 @@ static void
> > ironlake_update_primary_plane(struct drm_plane *primary,
> >  	struct drm_i915_private *dev_priv = to_i915(dev);
> >  	struct intel_crtc *intel_crtc = to_intel_crtc(crtc_state-
> > >base.crtc);
> >  	struct drm_framebuffer *fb = plane_state->base.fb;
> > -	int plane = intel_crtc->plane;
> > +	enum plane plane_id = to_intel_plane(primary)->plane;
> >  	u32 linear_offset;
> >  	u32 dspcntr;
> > -	i915_reg_t reg = DSPCNTR(plane);
> >  	unsigned int rotation = plane_state->base.rotation;
> >  	int x = plane_state->base.src.x1 >> 16;
> >  	int y = plane_state->base.src.y1 >> 16;
> > @@ -3202,19 +3199,19 @@ static void
> > ironlake_update_primary_plane(struct drm_plane *primary,
> >  	intel_crtc->adjusted_x = x;
> >  	intel_crtc->adjusted_y = y;
> >  
> > -	I915_WRITE(reg, dspcntr);
> > +	I915_WRITE(DSPCNTR(plane_id), dspcntr);
> >  
> > -	I915_WRITE(DSPSTRIDE(plane), fb->pitches[0]);
> > -	I915_WRITE(DSPSURF(plane),
> > +	I915_WRITE(DSPSTRIDE(plane_id), fb->pitches[0]);
> > +	I915_WRITE(DSPSURF(plane_id),
> >  		   intel_fb_gtt_offset(fb, rotation) +
> >  		   intel_crtc->dspaddr_offset);
> >  	if (IS_HASWELL(dev_priv) || IS_BROADWELL(dev_priv)) {
> > -		I915_WRITE(DSPOFFSET(plane), (y << 16) | x);
> > +		I915_WRITE(DSPOFFSET(plane_id), (y << 16) | x);
> >  	} else {
> > -		I915_WRITE(DSPTILEOFF(plane), (y << 16) | x);
> > -		I915_WRITE(DSPLINOFF(plane), linear_offset);
> > +		I915_WRITE(DSPTILEOFF(plane_id), (y << 16) | x);
> > +		I915_WRITE(DSPLINOFF(plane_id), linear_offset);
> >  	}
> > -	POSTING_READ(reg);
> > +	POSTING_READ(DSPCNTR(plane_id));
> >  }
> >  
> >  u32 intel_fb_stride_alignment(const struct drm_i915_private
> > *dev_priv,

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