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

["Kumar, Mahesh" <mahesh1.kumar@intel.com>]

Hi,


On 2/7/2018 10:12 PM, Sharma, Shashank wrote:
> 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.
it's plane-number.
>>   {
>>       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 ?
yes, non-planar formats will have single plane, that's why if format != 
NV12 & plane_num == 1 it's an error.
>
>> +
>>       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)? () : ().
agree.
>>             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.
index start from zero for plane_num as well :), same thing is used in 
drm_fourcc.h file while defining planar formats.

-Mahesh
>>           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;
>

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx