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 06:41:39PM -0200, Paulo Zanoni wrote:
> Em Sex, 2016-11-18 às 16:34 +0200, Ville Syrjälä escreveu:
> > 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.
> 
> But then, if there's no better name, isn't just "plane" better than
> "plane_id" here since it allows us to keep the "plane" namespace for
> the legacy A/B/C assignment, while plane_id stays for the newer things?

But then I can't use 'plane' for the struct intel_plane* which is going
to suck. There's no good answer I fear.

> 
> > 
> > > 
> > > 
> > > > 
> > > > 
> > > > 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