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

[Mahesh Kumar <mahesh1.kumar@xxxxxxxxx>]

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@xxxxxxxxx>
> > ---
> >   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;
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> >   static unsigned int
> >   skl_plane_relative_data_rate(const struct intel_crtc_state *cstate,
> >   			     const struct drm_plane_state *pstate,

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