Re: [PATCH 11/11] drm/i915/skl+: consider max supported plane pixel rate while scaling

[Maarten Lankhorst <maarten.lankhorst@linux.intel.com>]

Op 11-05-17 om 10:36 schreef Mahesh Kumar:
> Hi,
>
> Thanks for review.
>
> On Wednesday 10 May 2017 06:52 PM, Maarten Lankhorst wrote:
>> Op 08-05-17 om 13:49 schreef Mahesh Kumar:
>>> A display resolution is only supported if it meets all the restrictions
>>> below for Maximum Pipe Pixel Rate.
>>>
>>> The display resolution must fit within the maximum pixel rate output
>>> from the pipe. Make sure that the display pipe is able to feed pixels at
>>> a rate required to support the desired resolution.
>>> For each enabled plane on the pipe {
>>>      If plane scaling enabled {
>>>     Horizontal down scale amount = Maximum[1, plane horizontal size /
>>>             scaler horizontal window size]
>>>     Vertical down scale amount = Maximum[1, plane vertical size /
>>>             scaler vertical window size]
>>>     Plane down scale amount = Horizontal down scale amount *
>>>             Vertical down scale amount
>>>     Plane Ratio = 1 / Plane down scale amount
>>>      }
>>>      Else {
>>>     Plane Ratio = 1
>>>      }
>>>      If plane source pixel format is 64 bits per pixel {
>>>     Plane Ratio = Plane Ratio * 8/9
>>>      }
>>> }
>>>
>>> Pipe Ratio = Minimum Plane Ratio of all enabled planes on the pipe
>>>
>>> If pipe scaling is enabled {
>>>      Horizontal down scale amount = Maximum[1, pipe horizontal source size /
>>>         scaler horizontal window size]
>>>      Vertical down scale amount = Maximum[1, pipe vertical source size /
>>>         scaler vertical window size]
>>>      Note: The progressive fetch - interlace display mode is equivalent to a
>>>         2.0 vertical down scale
>>>      Pipe down scale amount = Horizontal down scale amount *
>>>         Vertical down scale amount
>>>      Pipe Ratio = Pipe Ratio / Pipe down scale amount
>>> }
>>>
>>> Pipe maximum pixel rate = CDCLK frequency * Pipe Ratio
>>>
>>> Signed-off-by: Mahesh Kumar <mahesh1.kumar@intel.com>
>>> ---
>>>   drivers/gpu/drm/i915/intel_display.c |  3 ++
>>>   drivers/gpu/drm/i915/intel_drv.h     |  2 +
>>>   drivers/gpu/drm/i915/intel_pm.c      | 87 ++++++++++++++++++++++++++++++++++++
>>>   3 files changed, 92 insertions(+)
>>>
>>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>>> index 85b9e2f521a0..d64367e810f8 100644
>>> --- a/drivers/gpu/drm/i915/intel_display.c
>>> +++ b/drivers/gpu/drm/i915/intel_display.c
>>> @@ -10992,6 +10992,9 @@ static int intel_crtc_atomic_check(struct drm_crtc *crtc,
>>>               ret = skl_update_scaler_crtc(pipe_config);
>>>             if (!ret)
>>> +            ret = skl_check_pipe_max_pixel_rate(intel_crtc,
>>> +                                pipe_config);
>>> +        if (!ret)
>>>               ret = intel_atomic_setup_scalers(dev_priv, intel_crtc,
>>>                                pipe_config);
>>>       }
>>> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
>>> index 54f3ff840812..8323fc2ec4f2 100644
>>> --- a/drivers/gpu/drm/i915/intel_drv.h
>>> +++ b/drivers/gpu/drm/i915/intel_drv.h
>>> @@ -1859,6 +1859,8 @@ bool skl_ddb_allocation_overlaps(const struct skl_ddb_entry **entries,
>>>                    int ignore);
>>>   bool ilk_disable_lp_wm(struct drm_device *dev);
>>>   int sanitize_rc6_option(struct drm_i915_private *dev_priv, int enable_rc6);
>>> +int skl_check_pipe_max_pixel_rate(struct intel_crtc *intel_crtc,
>>> +                  struct intel_crtc_state *cstate);
>>>   static inline int intel_enable_rc6(void)
>>>   {
>>>       return i915.enable_rc6;
>>> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
>>> index 92600cf42e12..69b1692ffd07 100644
>>> --- a/drivers/gpu/drm/i915/intel_pm.c
>>> +++ b/drivers/gpu/drm/i915/intel_pm.c
>>> @@ -3397,6 +3397,93 @@ skl_plane_downscale_amount(const struct intel_crtc_state *cstate,
>>>       return mul_fixed_16_16(downscale_w, downscale_h);
>>>   }
>>>   +static uint_fixed_16_16_t
>>> +skl_pipe_downscale_amount(const struct intel_crtc_state *config)
>>> +{
>>> +    uint_fixed_16_16_t pipe_downscale = u32_to_fixed_16_16(1);
>>> +
>>> +    if (!config->base.active)
>>> +        return pipe_downscale;
>>> +
>>> +    if (config->pch_pfit.enabled) {
>>> +        uint32_t src_w, src_h, dst_w, dst_h;
>>> +        uint32_t pfit_size = config->pch_pfit.size;
>>> +        uint_fixed_16_16_t fp_w_ratio, fp_h_ratio;
>>> +        uint_fixed_16_16_t downscale_h, downscale_w;
>>> +
>>> +        src_w = config->pipe_src_w;
>>> +        src_h = config->pipe_src_h;
>>> +        dst_w = pfit_size >> 16;
>>> +        dst_h = pfit_size & 0xffff;
>>> +
>>> +        if (!dst_w || !dst_h)
>>> +            return pipe_downscale;
>>> +
>>> +        fp_w_ratio = fixed_16_16_div(src_w, dst_w);
>>> +        fp_h_ratio = fixed_16_16_div(src_h, dst_h);
>>> +        downscale_w = max_fixed_16_16(fp_w_ratio, u32_to_fixed_16_16(1));
>>> +        downscale_h = max_fixed_16_16(fp_h_ratio, u32_to_fixed_16_16(1));
>>> +
>>> +        pipe_downscale = mul_fixed_16_16(downscale_w, downscale_h);
>>> +    }
>>> +
>>> +    return pipe_downscale;
>>> +}
>>> +
>>> +int skl_check_pipe_max_pixel_rate(struct intel_crtc *intel_crtc,
>>> +                  struct intel_crtc_state *cstate)
>>> +{
>>> +    struct drm_crtc_state *crtc_state = &cstate->base;
>>> +    struct drm_atomic_state *state = crtc_state->state;
>>> +    struct drm_plane *plane;
>>> +    const struct drm_plane_state *pstate;
>>> +    struct intel_plane_state *intel_pstate;
>>> +    int crtc_clock, cdclk;
>>> +    uint32_t pipe_max_pixel_rate;
>>> +    uint_fixed_16_16_t pipe_downscale;
>>> +    uint_fixed_16_16_t max_downscale = u32_to_fixed_16_16(1);
>>> +
>>> +    if (cstate->base.active)
>>> +        return 0;
>> ^This check looks buggy, are there any tests?
>>
>> In general we try to do the same for !active as we do with active, so please to remove any !active checks altogether..
> Thanks for catching this, yes it's a bug, during my testing this check was not there but I added this as an extra precaution & screwed-up :P
>>> +    drm_atomic_crtc_state_for_each_plane_state(plane, pstate, crtc_state) {
>>> +        uint_fixed_16_16_t plane_downscale;
>>> +        uint_fixed_16_16_t fp_9_div_8 = fixed_16_16_div(9, 8);
>>> +        int bpp;
>>> +
>>> +        if (!pstate->visible)
>>> +            continue;
>> Best use intel_wm_plane_visible here.
> will do,
>
> -Mahesh
>>> +
>>> +        if (WARN_ON(!pstate->fb))
>>> +            continue;
>>> +
>>> +        intel_pstate = to_intel_plane_state(pstate);
>>> +        plane_downscale = skl_plane_downscale_amount(cstate,
>>> +                                 intel_pstate);
>>> +        bpp = pstate->fb->format->cpp[0] * 8;
>>> +        if (bpp == 64)
>>> +            plane_downscale = mul_fixed_16_16(plane_downscale,
>>> +                              fp_9_div_8);
>>> +
>>> +        max_downscale = max_fixed_16_16(plane_downscale, max_downscale);
>>> +    }
>>> +    pipe_downscale = skl_pipe_downscale_amount(cstate);
>>> +
>>> +    pipe_downscale = mul_fixed_16_16(pipe_downscale, max_downscale);
>>> +
>>> +    crtc_clock = crtc_state->adjusted_mode.crtc_clock;
>>> +    cdclk = to_intel_atomic_state(state)->cdclk.logical.cdclk;
>>> +    pipe_max_pixel_rate = fixed_16_16_div_round_up_u64(cdclk,
>>> +                               pipe_downscale);
>>> +
>>> +    if (pipe_max_pixel_rate < crtc_clock) {
>>> +        DRM_ERROR("Max supported pixel clock with scaling exceeds\n");
>>> +        return -ERANGE; 
Oops, please also only return -EINVAL, definitely not ERANGE.

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