Re: [PATCH 2/2] drm/i915: Consolidate max_cdclk_freq check in intel_crtc_compute_min_cdclk()

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Tue, Jul 11, 2017 at 08:21:32PM +0000, Pandiyan, Dhinakaran wrote:
> 
> 
> 
> On Mon, 2017-07-10 at 22:33 +0300, ville.syrjala@linux.intel.com wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > Currently the .modeset_calc_cdclk() hooks check the final cdclk value
> > against the max allowed. That's not really sufficient since the low
> > level calc_cdclk() functions effectively clamp the minimum required
> > cdclk to the max supported by the platform. Hence if the minimum
> > required exceeds the platforms capabilities we'd keep going anyway
> > using the max cdclk frequency.
> > 
> > To fix that let's move the check earlier into
> > intel_crtc_compute_min_cdclk() and we'll check the minimum required
> > cdclk of the pipe against the maximum supported by the platform.
> > 
> 
> I suppose this should save some power in the case where one of the
> CRTC's pixel rate exceeds platform capabilities.

Rather it keeps the display working. If the cdclk can't supply enough
pixels to satisfy the port's demands then in the best case we get
constant underruns, in the worst case the box probably hard hangs.

> And failing the
> atomic_check instead of going with max_cdclk will help the userspace try
> a lower mode. Is that the idea?

Well, in theory you shouldn't even get this far as we should have
rejected the mode earlier. But I figured there's no harm in keeping the
checks since we do adjust the required cdcdlk here compared to earlier
estimates we might have made purely on the pixel rate.

> 
> Moving the checks makes sense to me because that seems like the original
> intention anyway, but I think it's a good idea to get someone else to
> take a look too.
> 
> Reviewed-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> 
> > Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_cdclk.c   | 96 +++++++++++++++++-------------------
> >  drivers/gpu/drm/i915/intel_display.c |  5 +-
> >  2 files changed, 48 insertions(+), 53 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_cdclk.c b/drivers/gpu/drm/i915/intel_cdclk.c
> > index 50f153dbea14..603950f1b87f 100644
> > --- a/drivers/gpu/drm/i915/intel_cdclk.c
> > +++ b/drivers/gpu/drm/i915/intel_cdclk.c
> > @@ -1789,6 +1789,12 @@ int intel_crtc_compute_min_cdclk(const struct intel_crtc_state *crtc_state)
> >  	if (crtc_state->has_audio && INTEL_GEN(dev_priv) >= 9)
> >  		min_cdclk = max(2 * 96000, min_cdclk);
> >  
> > +	if (min_cdclk > dev_priv->max_cdclk_freq) {
> > +		DRM_DEBUG_KMS("required cdclk (%d kHz) exceeds max (%d kHz)\n",
> > +			      min_cdclk, dev_priv->max_cdclk_freq);
> > +		return -EINVAL;
> > +	}
> > +
> >  	return min_cdclk;
> >  }
> >  
> > @@ -1798,16 +1804,21 @@ static int intel_compute_min_cdclk(struct drm_atomic_state *state)
> >  	struct drm_i915_private *dev_priv = to_i915(state->dev);
> >  	struct intel_crtc *crtc;
> >  	struct intel_crtc_state *crtc_state;
> > -	int min_cdclk = 0, i;
> > +	int min_cdclk, i;
> >  	enum pipe pipe;
> >  
> >  	memcpy(intel_state->min_cdclk, dev_priv->min_cdclk,
> >  	       sizeof(intel_state->min_cdclk));
> >  
> > -	for_each_new_intel_crtc_in_state(intel_state, crtc, crtc_state, i)
> > -		intel_state->min_cdclk[i] =
> > -			intel_crtc_compute_min_cdclk(crtc_state);
> > +	for_each_new_intel_crtc_in_state(intel_state, crtc, crtc_state, i) {
> > +		min_cdclk = intel_crtc_compute_min_cdclk(crtc_state);
> > +		if (min_cdclk < 0)
> > +			return min_cdclk;
> > +
> > +		intel_state->min_cdclk[i] = min_cdclk;
> > +	}
> >  
> > +	min_cdclk = 0;
> >  	for_each_pipe(dev_priv, pipe)
> >  		min_cdclk = max(intel_state->min_cdclk[pipe], min_cdclk);
> >  
> > @@ -1817,18 +1828,14 @@ static int intel_compute_min_cdclk(struct drm_atomic_state *state)
> >  static int vlv_modeset_calc_cdclk(struct drm_atomic_state *state)
> >  {
> >  	struct drm_i915_private *dev_priv = to_i915(state->dev);
> > -	int min_cdclk = intel_compute_min_cdclk(state);
> > -	struct intel_atomic_state *intel_state =
> > -		to_intel_atomic_state(state);
> > -	int cdclk;
> > +	struct intel_atomic_state *intel_state = to_intel_atomic_state(state);
> > +	int min_cdclk, cdclk;
> >  
> > -	cdclk = vlv_calc_cdclk(dev_priv, min_cdclk);
> > +	min_cdclk = intel_compute_min_cdclk(state);
> > +	if (min_cdclk < 0)
> > +		return min_cdclk;
> >  
> > -	if (cdclk > dev_priv->max_cdclk_freq) {
> > -		DRM_DEBUG_KMS("requested cdclk (%d kHz) exceeds max (%d kHz)\n",
> > -			      cdclk, dev_priv->max_cdclk_freq);
> > -		return -EINVAL;
> > -	}
> > +	cdclk = vlv_calc_cdclk(dev_priv, min_cdclk);
> >  
> >  	intel_state->cdclk.logical.cdclk = cdclk;
> >  
> > @@ -1846,10 +1853,12 @@ static int vlv_modeset_calc_cdclk(struct drm_atomic_state *state)
> >  
> >  static int bdw_modeset_calc_cdclk(struct drm_atomic_state *state)
> >  {
> > -	struct drm_i915_private *dev_priv = to_i915(state->dev);
> >  	struct intel_atomic_state *intel_state = to_intel_atomic_state(state);
> > -	int min_cdclk = intel_compute_min_cdclk(state);
> > -	int cdclk;
> > +	int min_cdclk, cdclk;
> > +
> > +	min_cdclk = intel_compute_min_cdclk(state);
> > +	if (min_cdclk < 0)
> > +		return min_cdclk;
> >  
> >  	/*
> >  	 * FIXME should also account for plane ratio
> > @@ -1857,12 +1866,6 @@ static int bdw_modeset_calc_cdclk(struct drm_atomic_state *state)
> >  	 */
> >  	cdclk = bdw_calc_cdclk(min_cdclk);
> >  
> > -	if (cdclk > dev_priv->max_cdclk_freq) {
> > -		DRM_DEBUG_KMS("requested cdclk (%d kHz) exceeds max (%d kHz)\n",
> > -			      cdclk, dev_priv->max_cdclk_freq);
> > -		return -EINVAL;
> > -	}
> > -
> >  	intel_state->cdclk.logical.cdclk = cdclk;
> >  
> >  	if (!intel_state->active_crtcs) {
> > @@ -1879,10 +1882,13 @@ static int bdw_modeset_calc_cdclk(struct drm_atomic_state *state)
> >  
> >  static int skl_modeset_calc_cdclk(struct drm_atomic_state *state)
> >  {
> > -	struct intel_atomic_state *intel_state = to_intel_atomic_state(state);
> >  	struct drm_i915_private *dev_priv = to_i915(state->dev);
> > -	int min_cdclk = intel_compute_min_cdclk(state);
> > -	int cdclk, vco;
> > +	struct intel_atomic_state *intel_state = to_intel_atomic_state(state);
> > +	int min_cdclk, cdclk, vco;
> > +
> > +	min_cdclk = intel_compute_min_cdclk(state);
> > +	if (min_cdclk < 0)
> > +		return min_cdclk;
> >  
> >  	vco = intel_state->cdclk.logical.vco;
> >  	if (!vco)
> > @@ -1894,12 +1900,6 @@ static int skl_modeset_calc_cdclk(struct drm_atomic_state *state)
> >  	 */
> >  	cdclk = skl_calc_cdclk(min_cdclk, vco);
> >  
> > -	if (cdclk > dev_priv->max_cdclk_freq) {
> > -		DRM_DEBUG_KMS("requested cdclk (%d kHz) exceeds max (%d kHz)\n",
> > -			      cdclk, dev_priv->max_cdclk_freq);
> > -		return -EINVAL;
> > -	}
> > -
> >  	intel_state->cdclk.logical.vco = vco;
> >  	intel_state->cdclk.logical.cdclk = cdclk;
> >  
> > @@ -1919,10 +1919,12 @@ static int skl_modeset_calc_cdclk(struct drm_atomic_state *state)
> >  static int bxt_modeset_calc_cdclk(struct drm_atomic_state *state)
> >  {
> >  	struct drm_i915_private *dev_priv = to_i915(state->dev);
> > -	int min_cdclk = intel_compute_min_cdclk(state);
> > -	struct intel_atomic_state *intel_state =
> > -		to_intel_atomic_state(state);
> > -	int cdclk, vco;
> > +	struct intel_atomic_state *intel_state = to_intel_atomic_state(state);
> > +	int min_cdclk, cdclk, vco;
> > +
> > +	min_cdclk = intel_compute_min_cdclk(state);
> > +	if (min_cdclk < 0)
> > +		return min_cdclk;
> >  
> >  	if (IS_GEMINILAKE(dev_priv)) {
> >  		cdclk = glk_calc_cdclk(min_cdclk);
> > @@ -1932,12 +1934,6 @@ static int bxt_modeset_calc_cdclk(struct drm_atomic_state *state)
> >  		vco = bxt_de_pll_vco(dev_priv, cdclk);
> >  	}
> >  
> > -	if (cdclk > dev_priv->max_cdclk_freq) {
> > -		DRM_DEBUG_KMS("requested cdclk (%d kHz) exceeds max (%d kHz)\n",
> > -			      cdclk, dev_priv->max_cdclk_freq);
> > -		return -EINVAL;
> > -	}
> > -
> >  	intel_state->cdclk.logical.vco = vco;
> >  	intel_state->cdclk.logical.cdclk = cdclk;
> >  
> > @@ -1963,20 +1959,16 @@ static int bxt_modeset_calc_cdclk(struct drm_atomic_state *state)
> >  static int cnl_modeset_calc_cdclk(struct drm_atomic_state *state)
> >  {
> >  	struct drm_i915_private *dev_priv = to_i915(state->dev);
> > -	struct intel_atomic_state *intel_state =
> > -		to_intel_atomic_state(state);
> > -	int min_cdclk = intel_compute_min_cdclk(state);
> > -	int cdclk, vco;
> > +	struct intel_atomic_state *intel_state = to_intel_atomic_state(state);
> > +	int min_cdclk, cdclk, vco;
> > +
> > +	min_cdclk = intel_compute_min_cdclk(state);
> > +	if (min_cdclk < 0)
> > +		return min_cdclk;
> >  
> >  	cdclk = cnl_calc_cdclk(min_cdclk);
> >  	vco = cnl_cdclk_pll_vco(dev_priv, cdclk);
> >  
> > -	if (cdclk > dev_priv->max_cdclk_freq) {
> > -		DRM_DEBUG_KMS("requested cdclk (%d kHz) exceeds max (%d kHz)\n",
> > -			      cdclk, dev_priv->max_cdclk_freq);
> > -		return -EINVAL;
> > -	}
> > -
> >  	intel_state->cdclk.logical.vco = vco;
> >  	intel_state->cdclk.logical.cdclk = cdclk;
> >  
> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > index b47535f5d95d..1caf0ef82e36 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -15590,8 +15590,11 @@ static void intel_modeset_readout_hw_state(struct drm_device *dev)
> >  
> >  			intel_crtc_compute_pixel_rate(crtc_state);
> >  
> > -			if (dev_priv->display.modeset_calc_cdclk)
> > +			if (dev_priv->display.modeset_calc_cdclk) {
> >  				min_cdclk = intel_crtc_compute_min_cdclk(crtc_state);
> > +				if (WARN_ON(min_cdclk < 0))
> > +					min_cdclk = 0;
> > +			}
> >  
> >  			drm_calc_timestamping_constants(&crtc->base,
> >  							&crtc_state->base.adjusted_mode);

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx