[Intel-gfx] [PATCH 11/12] drm/i915/skl: New ddb allocation algorithm

[Matt Roper]

On Wed, May 17, 2017 at 05:28:30PM +0530, Mahesh Kumar wrote:
> From: "Kumar, Mahesh" <mahesh1.kumar at intel.com>
> 
> This patch implements new DDB allocation algorithm as per HW team
> recommendation. This algo takecare of scenario where we allocate less DDB
> for the planes with lower relative pixel rate, but they require more DDB
> to work.
> It also takes care of enabling same watermark level for each
> plane in crtc, for efficient power saving.
> 
> Changes since v1:
>  - Rebase on top of Paulo's patch series
> 
> Changes since v2:
>  - Fix the for loop condition to enable WM
> 
> Changes since v3:
>  - Fix crash in cursor i-g-t reported by Maarten
>  - Rebase after addressing Paulo's comments
>  - Few other ULT fixes
> Changes since v4:
>  - Rebase on drm-tip
>  - Added separate function to enable WM levels
> Changes since v5:
>  - Fix a crash identified in skl-6770HQ system
> Changes since v6:
>  - Address review comments from Matt
> 
> Signed-off-by: Mahesh Kumar <mahesh1.kumar at intel.com>
> Reviewed-by: Maarten Lankhorst <maarten.lankhorst at linux.intel.com>
> ---
...
> @@ -4096,10 +4126,48 @@ skl_allocate_pipe_ddb(struct intel_crtc_state *cstate,
...
> +
> +		/*
> +		 * If This this level can successfully be enabled with the

Typo; repeated word ("This this").

...
> @@ -4381,64 +4452,41 @@ static int skl_compute_plane_wm(const struct drm_i915_private *dev_priv,
>  		}
>  	}
>  
> -	if (res_blocks >= ddb_allocation || res_lines > 31) {
> -		*enabled = false;
> +	if (res_lines >= 31 && level == 0) {
> +		struct drm_plane *plane = pstate->plane;
>  
> -		/*
> -		 * If there are no valid level 0 watermarks, then we can't
> -		 * support this display configuration.
> -		 */
> -		if (level) {
> -			return 0;
> -		} else {
> -			struct drm_plane *plane = pstate->plane;
> -
> -			DRM_DEBUG_KMS("Requested display configuration exceeds system watermark limitations\n");
> -			DRM_DEBUG_KMS("[PLANE:%d:%s] blocks required = %u/%u, lines required = %u/31\n",
> -				      plane->base.id, plane->name,
> -				      res_blocks, ddb_allocation, res_lines);
> -			return -EINVAL;
> -		}
> +		DRM_DEBUG_KMS("Requested display configuration exceeds system watermark limitations\n");
> +		DRM_DEBUG_KMS("[PLANE:%d:%s] lines required = %u/31\n",
> +				plane->base.id, plane->name, res_lines);
>  	}

I'm still confused why we don't need to return an error in this case?
We print the debug message here, but the function still returns 0
(success).  When we run skl_enable_plane_wm_levels(), it will disable
level 0 for that plane, which means we should be rejecting the whole
atomic flip (since this plane fails at all levels), but I don't see
anywhere that we actually check that again and trigger the failure?


Matt

>  
>  	*out_blocks = res_blocks;
>  	*out_lines = res_lines;
> -	*enabled = true;
>  
>  	return 0;
>  }
>  
...

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