Re: [PATCH 05/16] drm/i915/skl+: NV12 related changes for WM

Sharma, Shashank
2018-02-07T16:42:13Z

Regards

Shashank

On 2/6/2018 6:28 PM, Vidya Srinivas wrote:
> From: Mahesh Kumar <mahesh1.kumar@intel.com>
>
> NV12 requires WM calculation for UV plane as well.
> UV plane WM should also fulfill all the WM related restrictions.
>
> Signed-off-by: Mahesh Kumar <mahesh1.kumar@intel.com>
> ---
> drivers/gpu/drm/i915/i915_drv.h | 1 +
> drivers/gpu/drm/i915/intel_drv.h | 1 +
> drivers/gpu/drm/i915/intel_pm.c | 54 ++++++++++++++++++++++++++++++++--------
> 3 files changed, 45 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index cca5414..778dc5a 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -1459,6 +1459,7 @@ struct skl_wm_level {
> struct skl_wm_params {
> bool x_tiled, y_tiled;
> bool rc_surface;
> + bool is_nv12;
> uint32_t width;
> uint8_t cpp;
> uint32_t plane_pixel_rate;
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index ed33840..65dac21 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -594,6 +594,7 @@ struct intel_pipe_wm {
>
> struct skl_plane_wm {
> struct skl_wm_level wm[8];
> + struct skl_wm_level uv_wm[8];
> struct skl_wm_level trans_wm;
> bool is_nv12;
> };
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index 4c9a811..b3d1bf7 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -4421,7 +4421,7 @@ static int
> skl_compute_plane_wm_params(const struct drm_i915_private *dev_priv,
> struct intel_crtc_state *cstate,
> const struct intel_plane_state *intel_pstate,
> - struct skl_wm_params *wp)
> + struct skl_wm_params *wp, int plane_num)
Is this plane_num intended for 'no of planes' or 'plane number' ? from
the usage below looks like 'no of planes' but I could be wrong.
> {
> struct intel_plane *plane = to_intel_plane(intel_pstate->base.plane);
> const struct drm_plane_state *pstate = &intel_pstate->base;
> @@ -4434,6 +4434,12 @@ skl_compute_plane_wm_params(const struct drm_i915_private *dev_priv,
> if (!intel_wm_plane_visible(cstate, intel_pstate))
> return 0;
>
> + /* only NV12 format has two planes */
> + if (plane_num == 1 && fb->format->format != DRM_FORMAT_NV12) {
> + DRM_DEBUG_KMS("Non NV12 format have single plane\n");
> + return -EINVAL;
> + }
The comment says only NV12 format can have two planes, but if format !=
NV12 && plane_num == 1, why is this an error ? Also the debug message
says "Non NV12 format have single plane". Shouldn't it ?
Looks like you are expecting plane_number = 0, 1 for NV12 planes, and
when plane_num is non-zero, you expect it to be NV12 only ?
> +
> wp->y_tiled = fb->modifier == I915_FORMAT_MOD_Y_TILED ||
> fb->modifier == I915_FORMAT_MOD_Yf_TILED ||
> fb->modifier == I915_FORMAT_MOD_Y_TILED_CCS ||
> @@ -4441,6 +4447,7 @@ skl_compute_plane_wm_params(const struct drm_i915_private *dev_priv,
> wp->x_tiled = fb->modifier == I915_FORMAT_MOD_X_TILED;
> wp->rc_surface = fb->modifier == I915_FORMAT_MOD_Y_TILED_CCS ||
> fb->modifier == I915_FORMAT_MOD_Yf_TILED_CCS;
> + wp->is_nv12 = fb->format->format == DRM_FORMAT_NV12;
>
> if (plane->id == PLANE_CURSOR) {
> wp->width = intel_pstate->base.crtc_w;
> @@ -4453,7 +4460,10 @@ skl_compute_plane_wm_params(const struct drm_i915_private *dev_priv,
> wp->width = drm_rect_width(&intel_pstate->base.src) >> 16;
> }
>
> - wp->cpp = fb->format->cpp[0];
> + if (plane_num == 1 && wp->is_nv12)
> + wp->width /= 2;
> +
> + wp->cpp = fb->format->cpp[plane_num];
> wp->plane_pixel_rate = skl_adjusted_plane_pixel_rate(cstate,
> intel_pstate);
>
> @@ -4649,7 +4659,8 @@ skl_compute_wm_levels(const struct drm_i915_private *dev_priv,
> struct intel_crtc_state *cstate,
> const struct intel_plane_state *intel_pstate,
> const struct skl_wm_params *wm_params,
> - struct skl_plane_wm *wm)
> + struct skl_plane_wm *wm,
> + int plane_num)
Same question here
> {
> struct intel_crtc *intel_crtc = to_intel_crtc(cstate->base.crtc);
> struct drm_plane *plane = intel_pstate->base.plane;
> @@ -4657,15 +4668,20 @@ skl_compute_wm_levels(const struct drm_i915_private *dev_priv,
> uint16_t ddb_blocks;
> enum pipe pipe = intel_crtc->pipe;
> int level, max_level = ilk_wm_max_level(dev_priv);
> + enum plane_id plane_id = intel_plane->id;
> int ret;
>
> if (WARN_ON(!intel_pstate->base.fb))
> return -EINVAL;
>
> - ddb_blocks = skl_ddb_entry_size(&ddb->plane[pipe][intel_plane->id]);
> + if (plane_num == 0)
> + ddb_blocks = skl_ddb_entry_size(&ddb->plane[pipe][plane_id]);
> + else
> + ddb_blocks = skl_ddb_entry_size(&ddb->uv_plane[pipe][plane_id]);
>
> for (level = 0; level <= max_level; level++) {
> - struct skl_wm_level *result = &wm->wm[level];
> + struct skl_wm_level *result = plane_num ? &wm->uv_wm[level] :
> + &wm->wm[level];
It would look neat if both above condition as well as this one follows
the same pattern, if() else() Or (cond)? () : ().
>
> ret = skl_compute_plane_wm(dev_priv,
> cstate,
> @@ -4680,9 +4696,6 @@ skl_compute_wm_levels(const struct drm_i915_private *dev_priv,
> return ret;
> }
>
> - if (intel_pstate->base.fb->format->format == DRM_FORMAT_NV12)
> - wm->is_nv12 = true;
> -
> return 0;
> }
>
> @@ -4791,20 +4804,39 @@ static int skl_build_pipe_wm(struct intel_crtc_state *cstate,
>
> wm = &pipe_wm->planes[plane_id];
> ddb_blocks = skl_ddb_entry_size(&ddb->plane[pipe][plane_id]);
> - memset(&wm_params, 0, sizeof(struct skl_wm_params));
>
> ret = skl_compute_plane_wm_params(dev_priv, cstate,
> - intel_pstate, &wm_params);
> + intel_pstate, &wm_params, 0);
This is confusing that you refer to 0 as plane_num = 1 and 1 as
plane_num = 2.
> if (ret)
> return ret;
>
> ret = skl_compute_wm_levels(dev_priv, ddb, cstate,
> - intel_pstate, &wm_params, wm);
> + intel_pstate, &wm_params, wm, 0);
> if (ret)
> return ret;
> +
> skl_compute_transition_wm(cstate, &wm_params, &wm->wm[0],
> ddb_blocks, &wm->trans_wm);
> +
> + /* uv plane watermarks must also be validated for NV12 */
> + if (wm_params.is_nv12) {
> + memset(&wm_params, 0, sizeof(struct skl_wm_params));
> + wm->is_nv12 = true;
> +
> + ret = skl_compute_plane_wm_params(dev_priv, cstate,
> + intel_pstate,
> + &wm_params, 1);
Same here, there should be a better way to do this.

- Shashank
> + if (ret)
> + return ret;
> +
> + ret = skl_compute_wm_levels(dev_priv, ddb, cstate,
> + intel_pstate, &wm_params,
> + wm, 1);
> + if (ret)
> + return ret;
> + }
> }
> +
> pipe_wm->linetime = skl_compute_linetime_wm(cstate);
>
> return 0;