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

[Ville Syrjälä]

On Fri, Jun 26, 2020 at 02:03:23PM +0000, Saarinen, Jani wrote:
> Hi, 
> 
> > -----Original Message-----
> > From: Intel-gfx <intel-gfx-bounces at lists.freedesktop.org> On Behalf Of Ville Syrjälä
> > Sent: perjantai 26. kesäkuuta 2020 16.47
> > To: Lisovskiy, Stanislav <stanislav.lisovskiy at intel.com>
> > Cc: intel-gfx at lists.freedesktop.org
> > Subject: Re: [Intel-gfx] [PATCH] drm/i915: Clamp linetime wm to <64usec
> > 
> > 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
> No, there is no gtilab issue as no tiled display at CI. 

Can't see what this has to do with tiled displays. I guess we're talking
about some specific display that just happens to have that super slow
mode?

> 
> > 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
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx at lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel