[Intel-gfx] [PATCH 11/11] drm/i915/skl+: consider max supported plane pixel rate while scaling
Mahesh Kumar
mahesh1.kumar at intel.com
Thu May 11 10:59:02 UTC 2017
On Thursday 11 May 2017 03:18 PM, Maarten Lankhorst wrote:
> 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 at 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.
Requested scaling here is out of supported range that's why used
-ERANGE, should I still change it to -EINVAL?
-Mahesh
>
> ~Maarten
More information about the Intel-gfx
mailing list