[Intel-gfx] [PATCH] drm/i915: Clamp linetime wm to <64usec

[Ville Syrjälä]

On Fri, Jun 26, 2020 at 12:16:06PM +0300, Lisovskiy, Stanislav wrote:
> On Thu, Jun 25, 2020 at 11:00:03PM +0300, Ville Syrjala wrote:
> > From: Ville Syrjälä <ville.syrjala at linux.intel.com>
> > 
> > The linetime watermark is a 9 bit value, which gives us
> > a maximum linetime of just below 64 usec. If the linetime
> > exceeds that value we currently just discard the high bits
> > and program the rest into the register, which angers the
> > state checker.
> > 
> > To avoid that let's just clamp the value to the max. I believe
> > it should be perfectly fine to program a smaller linetime wm
> > than strictly required, just means the hardware may fetch data
> > sooner than strictly needed. We are further reassured by the
> > fact that with DRRS the spec tells us to program the smaller
> > of the two linetimes corresponding to the two refresh rates.
> > 
> > Cc: Stanislav Lisovskiy <stanislav.lisovskiy at intel.com>
> > Signed-off-by: Ville Syrjälä <ville.syrjala at linux.intel.com>
> > ---
> >  drivers/gpu/drm/i915/display/intel_display.c | 18 ++++++++++++------
> >  1 file changed, 12 insertions(+), 6 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> > index a11bb675f9b3..d486d675166f 100644
> > --- a/drivers/gpu/drm/i915/display/intel_display.c
> > +++ b/drivers/gpu/drm/i915/display/intel_display.c
> > @@ -12581,12 +12581,15 @@ static u16 hsw_linetime_wm(const struct intel_crtc_state *crtc_state)
> >  {
> >  	const struct drm_display_mode *adjusted_mode =
> >  		&crtc_state->hw.adjusted_mode;
> > +	int linetime_wm;
> >  
> >  	if (!crtc_state->hw.enable)
> >  		return 0;
> >  
> > -	return DIV_ROUND_CLOSEST(adjusted_mode->crtc_htotal * 1000 * 8,
> > -				 adjusted_mode->crtc_clock);
> > +	linetime_wm = DIV_ROUND_CLOSEST(adjusted_mode->crtc_htotal * 1000 * 8,
> > +					adjusted_mode->crtc_clock);
> > +
> > +	return min(linetime_wm, 0x1ff);
> 
> Are we actually doing the right thing here? I just mean that we get value
> 543 in the bug because pixel rate is 14874 which doesn't seem correct.

As explained in the commit msg programming this to lower than necessary
value should be totally fine. It just won't be optimal.

The values in the jira (was there an actual gitlab bug for this btw?)
look quite sensible to me. Some kind of low res 848xsomething mode with
dotclock of 14.874 Mhz, which gives us that linetime of ~68 usec.

> 
> Stan
> >  }
> >  
> >  static u16 hsw_ips_linetime_wm(const struct intel_crtc_state *crtc_state,
> > @@ -12594,12 +12597,15 @@ static u16 hsw_ips_linetime_wm(const struct intel_crtc_state *crtc_state,
> >  {
> >  	const struct drm_display_mode *adjusted_mode =
> >  		&crtc_state->hw.adjusted_mode;
> > +	int linetime_wm;
> >  
> >  	if (!crtc_state->hw.enable)
> >  		return 0;
> >  
> > -	return DIV_ROUND_CLOSEST(adjusted_mode->crtc_htotal * 1000 * 8,
> > -				 cdclk_state->logical.cdclk);
> > +	linetime_wm = DIV_ROUND_CLOSEST(adjusted_mode->crtc_htotal * 1000 * 8,
> > +					cdclk_state->logical.cdclk);
> > +
> > +	return min(linetime_wm, 0x1ff);
> >  }
> >  
> >  static u16 skl_linetime_wm(const struct intel_crtc_state *crtc_state)
> > @@ -12608,7 +12614,7 @@ static u16 skl_linetime_wm(const struct intel_crtc_state *crtc_state)
> >  	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
> >  	const struct drm_display_mode *adjusted_mode =
> >  		&crtc_state->hw.adjusted_mode;
> > -	u16 linetime_wm;
> > +	int linetime_wm;
> >  
> >  	if (!crtc_state->hw.enable)
> >  		return 0;
> > @@ -12620,7 +12626,7 @@ static u16 skl_linetime_wm(const struct intel_crtc_state *crtc_state)
> >  	if (IS_GEN9_LP(dev_priv) && dev_priv->ipc_enabled)
> >  		linetime_wm /= 2;
> >  
> > -	return linetime_wm;
> > +	return min(linetime_wm, 0x1ff);
> >  }
> >  
> >  static int hsw_compute_linetime_wm(struct intel_atomic_state *state,
> > -- 
> > 2.26.2
> > 

-- 
Ville Syrjälä
Intel