[Intel-gfx] [PATCH 6/9] drm/i915: Keep plane watermarks enabled more aggressively

[Matt Roper]

On Tue, Mar 12, 2019 at 10:58:41PM +0200, Ville Syrjala wrote:
> From: Ville Syrjälä <ville.syrjala at linux.intel.com>
> 
> Currently we disable all the watermarks above the selected max
> level for every plane. That would mean that the cursor's watermarks
> may also get modified when another plane causes the selected
> max watermark level to change. That is not so great as we would
> like to keep the cursor as indepenedent as possible to avoid
> having to throttle it in resposne to other plane activity.
> 
> To avoid that let's keep the watermarks enabled even for levels
> above the max selected watermark level, iff the plane has enough
> ddb for that particular level. This way the cursor's enabled
> watermarks only depend on the cursor itself. This is safe because
> the hardware will never choose to use a watermark level unless
> all enabled planes have also enabled that level.
> 
> Cc: Neel Desai <neel.desai at intel.com>
> Signed-off-by: Ville Syrjälä <ville.syrjala at linux.intel.com>

Iirc, we also had different levels enabled for different planes with the
old algorithm that calculated DDB first and watermarks second.  So
agreed; this should be very safe.

Patches 1-6 are

Reviewed-by: Matt Roper <matthew.d.roper at intel.com>


Matt

> ---
>  drivers/gpu/drm/i915/intel_pm.c | 17 ++++++++++++++++-
>  1 file changed, 16 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index c866663b31bc..8afbc56ad89a 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -4500,7 +4500,22 @@ skl_allocate_pipe_ddb(struct intel_crtc_state *cstate,
>  	for (level++; level <= ilk_wm_max_level(dev_priv); level++) {
>  		for_each_plane_id_on_crtc(intel_crtc, plane_id) {
>  			wm = &cstate->wm.skl.optimal.planes[plane_id];
> -			memset(&wm->wm[level], 0, sizeof(wm->wm[level]));
> +
> +			/*
> +			 * We only disable the watermarks for each plane if
> +			 * they exceed the ddb allocation of said plane. This
> +			 * is done so that we don't end up touching cursor
> +			 * watermarks needlessly when some other plane reduces
> +			 * our max possible watermark level.
> +			 *
> +			 * Bspec has this to say about the PLANE_WM enable bit:
> +			 * "All the watermarks at this level for all enabled
> +			 *  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]));
>  
>  			/*
>  			 * Wa_1408961008:icl
> -- 
> 2.19.2
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Matt Roper
Graphics Software Engineer
IoTG Platform Enabling & Development
Intel Corporation
(916) 356-2795