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

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

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?

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