Re: [PATCH 03/13] drm/i915/skl: Simplify wm structures slightly

[Ander Conselvan De Oliveira <conselvan2@gmail.com>]

On Thu, 2015-08-20 at 18:11 -0700, Matt Roper wrote:
> A bunch of SKL watermark-related structures have the cursor plane as a
> separate entry from the rest of the planes.  If we just extend the
> relevant plane arrays by one entry and use the top-most array element as
> the cursor, it will simplify future patches.
> 
> There shouldn't be any functional change here; this is just shuffling
> around how the data is stored in some of the data structures.  The whole
> patch is generated with Coccinelle via the following semantic patch:
> 
>         @@ struct skl_pipe_wm_parameters WMP; @@
>         - WMP.cursor
>         + WMP.plane[I915_MAX_PLANES]

This would make the code more confusing. I see that just below the I915_MAX_PLANES there is a enum
plane. Would it make sense to combine I915_MAX_PLANES with that? 

enum plane {
	PLANE_A,
	PLANE_B,
	PLANE_C,
	PLANE_D, /* this would be new, for BXT */
	I915_MAX_PLANES,
	PLANE_CURSOR = I915_MAX_PLANES,
};

Then the code would reference WMP.plane[PLANE_CURSOR].

Ander

> 
>         @@ struct skl_pipe_wm_parameters *WMP; @@
>         - WMP->cursor
>         + WMP->plane[I915_MAX_PLANES]
> 
>         @@ @@
>         struct skl_pipe_wm_parameters {
>         ...
>         - struct intel_plane_wm_parameters plane[I915_MAX_PLANES];
>         + struct intel_plane_wm_parameters plane[I915_MAX_PLANES+1];
>         - struct intel_plane_wm_parameters cursor;
>         ...
>         };
> 
>         @@
>         struct skl_ddb_allocation DDB;
>         expression E;
>         @@
>         - DDB.cursor[E]
>         + DDB.plane[E][I915_MAX_PLANES]
> 
>         @@
>         struct skl_ddb_allocation *DDB;
>         expression E;
>         @@
>         - DDB->cursor[E]
>         + DDB->plane[E][I915_MAX_PLANES]
> 
>         @@ @@
>         struct skl_ddb_allocation {
>         ...
>         - struct skl_ddb_entry plane[I915_MAX_PIPES][I915_MAX_PLANES];
>         + struct skl_ddb_entry plane[I915_MAX_PIPES][I915_MAX_PLANES+1];
>         ...
>         - struct skl_ddb_entry cursor[I915_MAX_PIPES];
>         ...
>         };
> 
>         @@
>         struct skl_wm_values WMV;
>         expression E1, E2;
>         @@
>         (
>         - WMV.cursor[E1][E2]
>         + WMV.plane[E1][I915_MAX_PLANES][E2]
>         |
>         - WMV.cursor_trans[E1]
>         + WMV.plane_trans[E1][I915_MAX_PLANES]
>         )
> 
>         @@
>         struct skl_wm_values *WMV;
>         expression E1, E2;
>         @@
>         (
>         - WMV->cursor[E1][E2]
>         + WMV->plane[E1][I915_MAX_PLANES][E2]
>         |
>         - WMV->cursor_trans[E1]
>         + WMV->plane_trans[E1][I915_MAX_PLANES]
>         )
> 
>         @@ @@
>         struct skl_wm_values {
>         ...
>         - uint32_t plane[I915_MAX_PIPES][I915_MAX_PLANES][8];
>         + uint32_t plane[I915_MAX_PIPES][I915_MAX_PLANES+1][8];
>         ...
>         - uint32_t cursor[I915_MAX_PIPES][8];
>         ...
>         - uint32_t plane_trans[I915_MAX_PIPES][I915_MAX_PLANES];
>         + uint32_t plane_trans[I915_MAX_PIPES][I915_MAX_PLANES+1];
>         ...
>         - uint32_t cursor_trans[I915_MAX_PIPES];
>         ...
>         };
> 
>         @@ struct skl_wm_level WML; @@
>         (
>         - WML.cursor_en
>         + WML.plane_en[I915_MAX_PLANES]
>         |
>         - WML.cursor_res_b
>         + WML.plane_res_b[I915_MAX_PLANES]
>         |
>         - WML.cursor_res_l
>         + WML.plane_res_l[I915_MAX_PLANES]
>         )
> 
>         @@ struct skl_wm_level *WML; @@
>         (
>         - WML->cursor_en
>         + WML->plane_en[I915_MAX_PLANES]
>         |
>         - WML->cursor_res_b
>         + WML->plane_res_b[I915_MAX_PLANES]
>         |
>         - WML->cursor_res_l
>         + WML->plane_res_l[I915_MAX_PLANES]
>         )
> 
>         @@ @@
>         struct skl_wm_level {
>         ...
>         - bool plane_en[I915_MAX_PLANES];
>         + bool plane_en[I915_MAX_PLANES+1];
>         ...
>         - bool cursor_en;
>         ...
>         - uint16_t plane_res_b[I915_MAX_PLANES];
>         + uint16_t plane_res_b[I915_MAX_PLANES+1];
>         ...
>         - uint8_t plane_res_l[I915_MAX_PLANES];
>         + uint8_t plane_res_l[I915_MAX_PLANES+1];
>         ...
>         - uint16_t cursor_res_b;
>         - uint8_t cursor_res_l;
>         ...
>         };
> 
> Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_debugfs.c  |  2 +-
>  drivers/gpu/drm/i915/i915_drv.h      | 20 +++-----
>  drivers/gpu/drm/i915/intel_display.c |  4 +-
>  drivers/gpu/drm/i915/intel_pm.c      | 89 +++++++++++++++++++-----------------
>  4 files changed, 56 insertions(+), 59 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> index 7a28de5..54508f1 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -3148,7 +3148,7 @@ static int i915_ddb_info(struct seq_file *m, void *unused)
>  				   skl_ddb_entry_size(entry));
>  		}
>  
> -		entry = &ddb->cursor[pipe];
> +		entry = &ddb->plane[pipe][I915_MAX_PLANES];
>  		seq_printf(m, "  %-13s%8u%8u%8u\n", "Cursor", entry->start,
>  			   entry->end, skl_ddb_entry_size(entry));
>  	}
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index e0f3f05..f04b67e 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -1571,28 +1571,22 @@ static inline bool skl_ddb_entry_equal(const struct skl_ddb_entry *e1,
>  
>  struct skl_ddb_allocation {
>  	struct skl_ddb_entry pipe[I915_MAX_PIPES];
> -	struct skl_ddb_entry plane[I915_MAX_PIPES][I915_MAX_PLANES]; /* packed/uv */
> -	struct skl_ddb_entry y_plane[I915_MAX_PIPES][I915_MAX_PLANES]; /* y-plane */
> -	struct skl_ddb_entry cursor[I915_MAX_PIPES];
> +	struct skl_ddb_entry plane[I915_MAX_PIPES][I915_MAX_PLANES + 1]; /* packed/uv */
> +	struct skl_ddb_entry y_plane[I915_MAX_PIPES][I915_MAX_PLANES];
>  };
>  
>  struct skl_wm_values {
>  	bool dirty[I915_MAX_PIPES];
>  	struct skl_ddb_allocation ddb;
>  	uint32_t wm_linetime[I915_MAX_PIPES];
> -	uint32_t plane[I915_MAX_PIPES][I915_MAX_PLANES][8];
> -	uint32_t cursor[I915_MAX_PIPES][8];
> -	uint32_t plane_trans[I915_MAX_PIPES][I915_MAX_PLANES];
> -	uint32_t cursor_trans[I915_MAX_PIPES];
> +	uint32_t plane[I915_MAX_PIPES][I915_MAX_PLANES + 1][8];
> +	uint32_t plane_trans[I915_MAX_PIPES][I915_MAX_PLANES + 1];
>  };
>  
>  struct skl_wm_level {
> -	bool plane_en[I915_MAX_PLANES];
> -	bool cursor_en;
> -	uint16_t plane_res_b[I915_MAX_PLANES];
> -	uint8_t plane_res_l[I915_MAX_PLANES];
> -	uint16_t cursor_res_b;
> -	uint8_t cursor_res_l;
> +	bool plane_en[I915_MAX_PLANES + 1];
> +	uint16_t plane_res_b[I915_MAX_PLANES + 1];
> +	uint8_t plane_res_l[I915_MAX_PLANES + 1];
>  };
>  
>  /*
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index f604ce1..455b1bd 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -12552,8 +12552,8 @@ static void check_wm_state(struct drm_device *dev)
>  		}
>  
>  		/* cursor */
> -		hw_entry = &hw_ddb.cursor[pipe];
> -		sw_entry = &sw_ddb->cursor[pipe];
> +		hw_entry = &hw_ddb.plane[pipe][I915_MAX_PLANES];
> +		sw_entry = &sw_ddb->plane[pipe][I915_MAX_PLANES];
>  
>  		if (skl_ddb_entry_equal(hw_entry, sw_entry))
>  			continue;
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index d82ea82..b9bcb85 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -1783,8 +1783,7 @@ struct skl_pipe_wm_parameters {
>  	bool active;
>  	uint32_t pipe_htotal;
>  	uint32_t pixel_rate; /* in KHz */
> -	struct intel_plane_wm_parameters plane[I915_MAX_PLANES];
> -	struct intel_plane_wm_parameters cursor;
> +	struct intel_plane_wm_parameters plane[I915_MAX_PLANES + 1];
>  };
>  
>  struct ilk_wm_maximums {
> @@ -2898,7 +2897,8 @@ void skl_ddb_get_hw_state(struct drm_i915_private *dev_priv,
>  		}
>  
>  		val = I915_READ(CUR_BUF_CFG(pipe));
> -		skl_ddb_entry_init_from_hw(&ddb->cursor[pipe], val);
> +		skl_ddb_entry_init_from_hw(&ddb->plane[pipe][I915_MAX_PLANES],
> +					   val);
>  	}
>  }
>  
> @@ -2967,13 +2967,14 @@ skl_allocate_pipe_ddb(struct drm_crtc *crtc,
>  	alloc_size = skl_ddb_entry_size(alloc);
>  	if (alloc_size == 0) {
>  		memset(ddb->plane[pipe], 0, sizeof(ddb->plane[pipe]));
> -		memset(&ddb->cursor[pipe], 0, sizeof(ddb->cursor[pipe]));
> +		memset(&ddb->plane[pipe][I915_MAX_PLANES], 0,
> +		       sizeof(ddb->plane[pipe][I915_MAX_PLANES]));
>  		return;
>  	}
>  
>  	cursor_blocks = skl_cursor_allocation(config);
> -	ddb->cursor[pipe].start = alloc->end - cursor_blocks;
> -	ddb->cursor[pipe].end = alloc->end;
> +	ddb->plane[pipe][I915_MAX_PLANES].start = alloc->end - cursor_blocks;
> +	ddb->plane[pipe][I915_MAX_PLANES].end = alloc->end;
>  
>  	alloc_size -= cursor_blocks;
>  	alloc->end -= cursor_blocks;
> @@ -3112,8 +3113,8 @@ static bool skl_ddb_allocation_changed(const struct skl_ddb_allocation 
> *new_ddb,
>  		   sizeof(new_ddb->plane[pipe])))
>  		return true;
>  
> -	if (memcmp(&new_ddb->cursor[pipe], &cur_ddb->cursor[pipe],
> -		    sizeof(new_ddb->cursor[pipe])))
> +	if (memcmp(&new_ddb->plane[pipe][I915_MAX_PLANES], &cur_ddb
> ->plane[pipe][I915_MAX_PLANES],
> +		    sizeof(new_ddb->plane[pipe][I915_MAX_PLANES])))
>  		return true;
>  
>  	return false;
> @@ -3172,17 +3173,17 @@ static void skl_compute_wm_pipe_parameters(struct drm_crtc *crtc,
>  		p->plane[0].rotation = crtc->primary->state->rotation;
>  
>  		fb = crtc->cursor->state->fb;
> -		p->cursor.y_bytes_per_pixel = 0;
> +		p->plane[I915_MAX_PLANES].y_bytes_per_pixel = 0;
>  		if (fb) {
> -			p->cursor.enabled = true;
> -			p->cursor.bytes_per_pixel = fb->bits_per_pixel / 8;
> -			p->cursor.horiz_pixels = crtc->cursor->state->crtc_w;
> -			p->cursor.vert_pixels = crtc->cursor->state->crtc_h;
> +			p->plane[I915_MAX_PLANES].enabled = true;
> +			p->plane[I915_MAX_PLANES].bytes_per_pixel = fb->bits_per_pixel / 8;
> +			p->plane[I915_MAX_PLANES].horiz_pixels = crtc->cursor->state->crtc_w;
> +			p->plane[I915_MAX_PLANES].vert_pixels = crtc->cursor->state->crtc_h;
>  		} else {
> -			p->cursor.enabled = false;
> -			p->cursor.bytes_per_pixel = 0;
> -			p->cursor.horiz_pixels = 64;
> -			p->cursor.vert_pixels = 64;
> +			p->plane[I915_MAX_PLANES].enabled = false;
> +			p->plane[I915_MAX_PLANES].bytes_per_pixel = 0;
> +			p->plane[I915_MAX_PLANES].horiz_pixels = 64;
> +			p->plane[I915_MAX_PLANES].vert_pixels = 64;
>  		}
>  	}
>  
> @@ -3296,11 +3297,12 @@ static void skl_compute_wm_level(const struct drm_i915_private *dev_priv,
>  						&result->plane_res_l[i]);
>  	}
>  
> -	ddb_blocks = skl_ddb_entry_size(&ddb->cursor[pipe]);
> -	result->cursor_en = skl_compute_plane_wm(dev_priv, p, &p->cursor,
> +	ddb_blocks = skl_ddb_entry_size(&ddb->plane[pipe][I915_MAX_PLANES]);
> +	result->plane_en[I915_MAX_PLANES] = skl_compute_plane_wm(dev_priv, p,
> +						 &p->plane[I915_MAX_PLANES],
>  						 ddb_blocks, level,
> -						 &result->cursor_res_b,
> -						 &result->cursor_res_l);
> +						 &result->plane_res_b[I915_MAX_PLANES],
> +						 &result->plane_res_l[I915_MAX_PLANES]);
>  }
>  
>  static uint32_t
> @@ -3328,7 +3330,7 @@ static void skl_compute_transition_wm(struct drm_crtc *crtc,
>  	/* Until we know more, just disable transition WMs */
>  	for (i = 0; i < intel_num_planes(intel_crtc); i++)
>  		trans_wm->plane_en[i] = false;
> -	trans_wm->cursor_en = false;
> +	trans_wm->plane_en[I915_MAX_PLANES] = false;
>  }
>  
>  static void skl_compute_pipe_wm(struct drm_crtc *crtc,
> @@ -3377,13 +3379,13 @@ static void skl_compute_wm_results(struct drm_device *dev,
>  
>  		temp = 0;
>  
> -		temp |= p_wm->wm[level].cursor_res_l << PLANE_WM_LINES_SHIFT;
> -		temp |= p_wm->wm[level].cursor_res_b;
> +		temp |= p_wm->wm[level].plane_res_l[I915_MAX_PLANES] << PLANE_WM_LINES_SHIFT;
> +		temp |= p_wm->wm[level].plane_res_b[I915_MAX_PLANES];
>  
> -		if (p_wm->wm[level].cursor_en)
> +		if (p_wm->wm[level].plane_en[I915_MAX_PLANES])
>  			temp |= PLANE_WM_EN;
>  
> -		r->cursor[pipe][level] = temp;
> +		r->plane[pipe][I915_MAX_PLANES][level] = temp;
>  
>  	}
>  
> @@ -3399,12 +3401,12 @@ static void skl_compute_wm_results(struct drm_device *dev,
>  	}
>  
>  	temp = 0;
> -	temp |= p_wm->trans_wm.cursor_res_l << PLANE_WM_LINES_SHIFT;
> -	temp |= p_wm->trans_wm.cursor_res_b;
> -	if (p_wm->trans_wm.cursor_en)
> +	temp |= p_wm->trans_wm.plane_res_l[I915_MAX_PLANES] << PLANE_WM_LINES_SHIFT;
> +	temp |= p_wm->trans_wm.plane_res_b[I915_MAX_PLANES];
> +	if (p_wm->trans_wm.plane_en[I915_MAX_PLANES])
>  		temp |= PLANE_WM_EN;
>  
> -	r->cursor_trans[pipe] = temp;
> +	r->plane_trans[pipe][I915_MAX_PLANES] = temp;
>  
>  	r->wm_linetime[pipe] = p_wm->linetime;
>  }
> @@ -3438,12 +3440,13 @@ static void skl_write_wm_values(struct drm_i915_private *dev_priv,
>  				I915_WRITE(PLANE_WM(pipe, i, level),
>  					   new->plane[pipe][i][level]);
>  			I915_WRITE(CUR_WM(pipe, level),
> -				   new->cursor[pipe][level]);
> +				   new->plane[pipe][I915_MAX_PLANES][level]);
>  		}
>  		for (i = 0; i < intel_num_planes(crtc); i++)
>  			I915_WRITE(PLANE_WM_TRANS(pipe, i),
>  				   new->plane_trans[pipe][i]);
> -		I915_WRITE(CUR_WM_TRANS(pipe), new->cursor_trans[pipe]);
> +		I915_WRITE(CUR_WM_TRANS(pipe),
> +			   new->plane_trans[pipe][I915_MAX_PLANES]);
>  
>  		for (i = 0; i < intel_num_planes(crtc); i++) {
>  			skl_ddb_entry_write(dev_priv,
> @@ -3455,7 +3458,7 @@ static void skl_write_wm_values(struct drm_i915_private *dev_priv,
>  		}
>  
>  		skl_ddb_entry_write(dev_priv, CUR_BUF_CFG(pipe),
> -				    &new->ddb.cursor[pipe]);
> +				    &new->ddb.plane[pipe][I915_MAX_PLANES]);
>  	}
>  }
>  
> @@ -3809,10 +3812,10 @@ static void skl_pipe_wm_active_state(uint32_t val,
>  					(val >> PLANE_WM_LINES_SHIFT) &
>  						PLANE_WM_LINES_MASK;
>  		} else {
> -			active->wm[level].cursor_en = is_enabled;
> -			active->wm[level].cursor_res_b =
> +			active->wm[level].plane_en[I915_MAX_PLANES] = is_enabled;
> +			active->wm[level].plane_res_b[I915_MAX_PLANES] =
>  					val & PLANE_WM_BLOCKS_MASK;
> -			active->wm[level].cursor_res_l =
> +			active->wm[level].plane_res_l[I915_MAX_PLANES] =
>  					(val >> PLANE_WM_LINES_SHIFT) &
>  						PLANE_WM_LINES_MASK;
>  		}
> @@ -3825,10 +3828,10 @@ static void skl_pipe_wm_active_state(uint32_t val,
>  					(val >> PLANE_WM_LINES_SHIFT) &
>  						PLANE_WM_LINES_MASK;
>  		} else {
> -			active->trans_wm.cursor_en = is_enabled;
> -			active->trans_wm.cursor_res_b =
> +			active->trans_wm.plane_en[I915_MAX_PLANES] = is_enabled;
> +			active->trans_wm.plane_res_b[I915_MAX_PLANES] =
>  					val & PLANE_WM_BLOCKS_MASK;
> -			active->trans_wm.cursor_res_l =
> +			active->trans_wm.plane_res_l[I915_MAX_PLANES] =
>  					(val >> PLANE_WM_LINES_SHIFT) &
>  						PLANE_WM_LINES_MASK;
>  		}
> @@ -3854,12 +3857,12 @@ static void skl_pipe_wm_get_hw_state(struct drm_crtc *crtc)
>  		for (i = 0; i < intel_num_planes(intel_crtc); i++)
>  			hw->plane[pipe][i][level] =
>  					I915_READ(PLANE_WM(pipe, i, level));
> -		hw->cursor[pipe][level] = I915_READ(CUR_WM(pipe, level));
> +		hw->plane[pipe][I915_MAX_PLANES][level] = I915_READ(CUR_WM(pipe, level));
>  	}
>  
>  	for (i = 0; i < intel_num_planes(intel_crtc); i++)
>  		hw->plane_trans[pipe][i] = I915_READ(PLANE_WM_TRANS(pipe, i));
> -	hw->cursor_trans[pipe] = I915_READ(CUR_WM_TRANS(pipe));
> +	hw->plane_trans[pipe][I915_MAX_PLANES] = I915_READ(CUR_WM_TRANS(pipe));
>  
>  	if (!intel_crtc->active)
>  		return;
> @@ -3874,7 +3877,7 @@ static void skl_pipe_wm_get_hw_state(struct drm_crtc *crtc)
>  			skl_pipe_wm_active_state(temp, active, false,
>  						false, i, level);
>  		}
> -		temp = hw->cursor[pipe][level];
> +		temp = hw->plane[pipe][I915_MAX_PLANES][level];
>  		skl_pipe_wm_active_state(temp, active, false, true, i, level);
>  	}
>  
> @@ -3883,7 +3886,7 @@ static void skl_pipe_wm_get_hw_state(struct drm_crtc *crtc)
>  		skl_pipe_wm_active_state(temp, active, true, false, i, 0);
>  	}
>  
> -	temp = hw->cursor_trans[pipe];
> +	temp = hw->plane_trans[pipe][I915_MAX_PLANES];
>  	skl_pipe_wm_active_state(temp, active, true, true, i, 0);
>  }
>  
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx