Re: [PATCH 3/8] drm/i915/skl: Move the per-latency maximum test earlier

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

On Tue, Oct 14, 2014 at 05:31:01PM +0100, Damien Lespiau wrote:
> To align with the ilk WM code and because it makes sense to test against
> the upper bounds as soon as possible, let's move the maximum checks from
> skl_compute_wm_results() to skl_compute_plane_wm().
> 
> This has the nice benefit to be done in come common plane code and so
> remove the duplication we had between the regular planes and the cursor.
> 
> Suggested-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Signed-off-by: Damien Lespiau <damien.lespiau@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_pm.c | 14 +++-----------
>  1 file changed, 3 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index 475a3d4..65df074 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -3361,6 +3361,9 @@ static bool skl_compute_plane_wm(struct skl_pipe_wm_parameters *p,
>  	*res_blocks = DIV_ROUND_UP(result_bytes, 512) + 1;
>  	*res_lines = DIV_ROUND_UP(result_bytes, plane_bytes_per_line);
>  
> +	if (*res_blocks > ddb_allocation || *res_lines > 31)
> +		return false;
> +

Unless I completely misread things we would still stick the computed *res
values into the register which still risks an overflow there. The ILK
code leaves the enrtire wm struct zeroed if any value can't fit in the
register (well apart from fbc_wm which is handled in a special way).

My impression is that we could just zero the values here any time we see
that they might exceed the limits. And I think we don't especially have
to care about the register max vs. current ddb allocation max
distinction on SKL like we do on ILK. Although if we did make that
distinction, and I've not thought it really though yet, maybe we can
skip recomputing some plane watermarks when the only thing changing for
the plane is the DDB allocation. We'd just have to re-evaluate the state
of the wm level enable bit when we're about to blast the watermarks into
the register. But maybe that would just complicate the code too much. I
guess it's better left as a potential future optimization.

>  	return true;
>  }
>  
> @@ -3422,17 +3425,11 @@ static void skl_compute_wm_results(struct drm_device *dev,
>  	enum pipe pipe = intel_crtc->pipe;
>  
>  	for (level = 0; level <= max_level; level++) {
> -		uint16_t ddb_blocks;
>  		uint32_t temp;
>  		int i;
>  
>  		for (i = 0; i < intel_num_planes(intel_crtc); i++) {
>  			temp = 0;
> -			ddb_blocks = skl_ddb_entry_size(&r->ddb.plane[pipe][i]);
> -
> -			if ((p_wm->wm[level].plane_res_b[i] > ddb_blocks) ||
> -				(p_wm->wm[level].plane_res_l[i] > 31))
> -				p_wm->wm[level].plane_en[i] = false;
>  
>  			temp |= p_wm->wm[level].plane_res_l[i] <<
>  					PLANE_WM_LINES_SHIFT;
> @@ -3447,11 +3444,6 @@ static void skl_compute_wm_results(struct drm_device *dev,
>  		}
>  
>  		temp = 0;
> -		ddb_blocks = skl_ddb_entry_size(&r->ddb.cursor[pipe]);
> -
> -		if ((p_wm->wm[level].cursor_res_b > ddb_blocks) ||
> -			(p_wm->wm[level].cursor_res_l > 31))
> -			p_wm->wm[level].cursor_en = false;
>  
>  		temp |= p_wm->wm[level].cursor_res_l << PLANE_WM_LINES_SHIFT;
>  		temp |= p_wm->wm[level].cursor_res_b;
> -- 
> 1.8.3.1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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