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

[Saarinen, Jani]

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. 

> 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