Re: [PATCH v19 2/8] drm/i915: Introduce skl_plane_wm_level accessor.

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Mon, Mar 09, 2020 at 06:11:58PM +0200, Stanislav Lisovskiy wrote:
> For future Gen12 SAGV implementation we need to
> seemlessly alter wm levels calculated, depending
> on whether we are allowed to enable SAGV or not.
> 
> So this accessor will give additional flexibility
> to do that.
> 
> Currently this accessor is still simply working
> as "pass-through" function. This will be changed
> in next coming patches from this series.
> 
> v2: - plane_id -> plane->id(Ville Syrjälä)
>     - Moved wm_level var to have more local scope
>       (Ville Syrjälä)
>     - Renamed yuv to color_plane(Ville Syrjälä) in
>       skl_plane_wm_level
> 
> v3: - plane->id -> plane_id(this time for real, Ville Syrjälä)
>     - Changed colorplane id type from boolean to int as index
>       (Ville Syrjälä)
>     - Moved crtc_state param so that it is first now
>       (Ville Syrjälä)
>     - Moved wm_level declaration to tigher scope in
>       skl_write_plane_wm(Ville Syrjälä)
> 
> Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy@xxxxxxxxx>
> ---
>  drivers/gpu/drm/i915/intel_pm.c | 85 ++++++++++++++++++++++++++-------
>  1 file changed, 67 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index c7928c870b0a..c72fa59a8302 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -4547,6 +4547,18 @@ icl_get_total_relative_data_rate(struct intel_crtc_state *crtc_state,
>  	return total_data_rate;
>  }
>  
> +static const struct skl_wm_level *
> +skl_plane_wm_level(const struct intel_crtc_state *crtc_state,
> +		   enum plane_id plane_id,
> +		   int level,
> +		   int color_plane)
> +{
> +	const struct skl_plane_wm *wm =
> +		&crtc_state->wm.skl.optimal.planes[plane_id];
> +
> +	return color_plane == 0 ? &wm->wm[level] : &wm->uv_wm[level];
> +}
> +
>  static int
>  skl_allocate_pipe_ddb(struct intel_crtc_state *crtc_state)
>  {
> @@ -4606,22 +4618,29 @@ skl_allocate_pipe_ddb(struct intel_crtc_state *crtc_state)
>  	 */
>  	for (level = ilk_wm_max_level(dev_priv); level >= 0; level--) {
>  		blocks = 0;
> +
>  		for_each_plane_id_on_crtc(crtc, plane_id) {
> -			const struct skl_plane_wm *wm =
> -				&crtc_state->wm.skl.optimal.planes[plane_id];
> +			const struct skl_wm_level *wm_level;
> +			const struct skl_wm_level *wm_uv_level;
> +			int color_plane = 0;

These color_plane variables seems kinda pointless. I'd just pass 0/1 directly
(pretty sure that's what we do elsewhere too).

> +
> +			wm_level = skl_plane_wm_level(crtc_state, plane_id,
> +						      level, color_plane);
> +			wm_uv_level = skl_plane_wm_level(crtc_state, plane_id,
> +							 level, color_plane + 1);
>  
>  			if (plane_id == PLANE_CURSOR) {
> -				if (wm->wm[level].min_ddb_alloc > total[PLANE_CURSOR]) {
> +				if (wm_level->min_ddb_alloc > total[PLANE_CURSOR]) {
>  					drm_WARN_ON(&dev_priv->drm,
> -						    wm->wm[level].min_ddb_alloc != U16_MAX);
> +						    wm_level->min_ddb_alloc != U16_MAX);
>  					blocks = U32_MAX;
>  					break;
>  				}
>  				continue;
>  			}
>  
> -			blocks += wm->wm[level].min_ddb_alloc;
> -			blocks += wm->uv_wm[level].min_ddb_alloc;
> +			blocks += wm_level->min_ddb_alloc;
> +			blocks += wm_uv_level->min_ddb_alloc;
>  		}
>  
>  		if (blocks <= alloc_size) {
> @@ -4644,10 +4663,16 @@ skl_allocate_pipe_ddb(struct intel_crtc_state *crtc_state)
>  	 * proportional to its relative data rate.
>  	 */
>  	for_each_plane_id_on_crtc(crtc, plane_id) {
> -		const struct skl_plane_wm *wm =
> -			&crtc_state->wm.skl.optimal.planes[plane_id];
> +		const struct skl_wm_level *wm_level;
> +		const struct skl_wm_level *wm_uv_level;
>  		u64 rate;
>  		u16 extra;
> +		int color_plane = 0;
> +
> +		wm_level = skl_plane_wm_level(crtc_state, plane_id,
> +					      level, color_plane);
> +		wm_uv_level = skl_plane_wm_level(crtc_state, plane_id,
> +						 level, color_plane + 1);
>  
>  		if (plane_id == PLANE_CURSOR)
>  			continue;
> @@ -4663,7 +4688,7 @@ skl_allocate_pipe_ddb(struct intel_crtc_state *crtc_state)
>  		extra = min_t(u16, alloc_size,
>  			      DIV64_U64_ROUND_UP(alloc_size * rate,
>  						 total_data_rate));
> -		total[plane_id] = wm->wm[level].min_ddb_alloc + extra;
> +		total[plane_id] = wm_level->min_ddb_alloc + extra;
>  		alloc_size -= extra;
>  		total_data_rate -= rate;
>  
> @@ -4674,7 +4699,7 @@ skl_allocate_pipe_ddb(struct intel_crtc_state *crtc_state)
>  		extra = min_t(u16, alloc_size,
>  			      DIV64_U64_ROUND_UP(alloc_size * rate,
>  						 total_data_rate));
> -		uv_total[plane_id] = wm->uv_wm[level].min_ddb_alloc + extra;
> +		uv_total[plane_id] = wm_uv_level->min_ddb_alloc + extra;
>  		alloc_size -= extra;
>  		total_data_rate -= rate;
>  	}
> @@ -4717,8 +4742,16 @@ skl_allocate_pipe_ddb(struct intel_crtc_state *crtc_state)
>  	 */
>  	for (level++; level <= ilk_wm_max_level(dev_priv); level++) {
>  		for_each_plane_id_on_crtc(crtc, plane_id) {
> +			const struct skl_wm_level *wm_level;
> +			const struct skl_wm_level *wm_uv_level;
>  			struct skl_plane_wm *wm =
>  				&crtc_state->wm.skl.optimal.planes[plane_id];
> +			int color_plane = 0;
> +
> +			wm_level = skl_plane_wm_level(crtc_state, plane_id,
> +						      level, color_plane);
> +			wm_uv_level = skl_plane_wm_level(crtc_state, plane_id,
> +							 level, color_plane + 1);
>  
>  			/*
>  			 * We only disable the watermarks for each plane if
> @@ -4732,9 +4765,10 @@ skl_allocate_pipe_ddb(struct intel_crtc_state *crtc_state)
>  			 *  planes must be enabled before the level will be used."
>  			 * So this is actually safe to do.
>  			 */
> -			if (wm->wm[level].min_ddb_alloc > total[plane_id] ||
> -			    wm->uv_wm[level].min_ddb_alloc > uv_total[plane_id])
> -				memset(&wm->wm[level], 0, sizeof(wm->wm[level]));
> +			if (wm_level->min_ddb_alloc > total[plane_id] ||
> +			    wm_uv_level->min_ddb_alloc > uv_total[plane_id])
> +				memset(&wm->wm[level], 0,
> +				       sizeof(struct skl_wm_level));

memset(wm_level, 0, sizeof(*wm_level)) ?

Hmm. Also wondering why we're not clearing wm_uv here as well. I suppose
it might not mater since the hw doesn't use wm_uv (and I fixed the
"did the wms change?" check to ignore it too). Bit might be nice to clear
it for consistency. Should be a separate patch though.

>  
>  			/*
>  			 * Wa_1408961008:icl, ehl
> @@ -4742,9 +4776,14 @@ skl_allocate_pipe_ddb(struct intel_crtc_state *crtc_state)
>  			 */
>  			if (IS_GEN(dev_priv, 11) &&
>  			    level == 1 && wm->wm[0].plane_en) {
> -				wm->wm[level].plane_res_b = wm->wm[0].plane_res_b;
> -				wm->wm[level].plane_res_l = wm->wm[0].plane_res_l;
> -				wm->wm[level].ignore_lines = wm->wm[0].ignore_lines;
> +				wm_level = skl_plane_wm_level(crtc_state, plane_id,
> +							      0, color_plane);
> +				wm->wm[level].plane_res_b =
> +					wm_level->plane_res_b;
> +				wm->wm[level].plane_res_l =
> +					wm_level->plane_res_l;
> +				wm->wm[level].ignore_lines =
> +					wm_level->ignore_lines;

I would suggest we want this to read something like:

const struct skl_wm_level *wm_level0 = skl_plane_wm_level(...)

wm_level->foo = wm_level0->foo;
...

And with those we can throw out the 'wm' variable from this loop as
well.

>  			}
>  		}
>  	}
> @@ -5358,8 +5397,13 @@ void skl_write_plane_wm(struct intel_plane *plane,
>  		&crtc_state->wm.skl.plane_ddb_uv[plane_id];
>  
>  	for (level = 0; level <= max_level; level++) {
> +		const struct skl_wm_level *wm_level;
> +		int color_plane = 0;
> +
> +		wm_level = skl_plane_wm_level(crtc_state, plane_id, level, color_plane);
> +
>  		skl_write_wm_level(dev_priv, PLANE_WM(pipe, plane_id, level),
> -				   &wm->wm[level]);
> +				   wm_level);
>  	}
>  	skl_write_wm_level(dev_priv, PLANE_WM_TRANS(pipe, plane_id),
>  			   &wm->trans_wm);
> @@ -5392,8 +5436,13 @@ void skl_write_cursor_wm(struct intel_plane *plane,
>  		&crtc_state->wm.skl.plane_ddb_y[plane_id];
>  
>  	for (level = 0; level <= max_level; level++) {
> +		const struct skl_wm_level *wm_level;
> +		int color_plane = 0;
> +
> +		wm_level = skl_plane_wm_level(crtc_state, plane_id, level, color_plane);
> +
>  		skl_write_wm_level(dev_priv, CUR_WM(pipe, level),
> -				   &wm->wm[level]);
> +				   wm_level);
>  	}
>  	skl_write_wm_level(dev_priv, CUR_WM_TRANS(pipe), &wm->trans_wm);
>  
> -- 
> 2.24.1.485.gad05a3d8e5

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




[Index of Archives]     [AMD Graphics]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux