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

[Saarinen, Jani]

Hi, 

> -----Original Message-----
> From: Ville Syrjälä <ville.syrjala at linux.intel.com>
> Sent: perjantai 26. kesäkuuta 2020 17.09
> To: Saarinen, Jani <jani.saarinen at intel.com>
> Cc: Lisovskiy, Stanislav <stanislav.lisovskiy at intel.com>; intel-
> gfx at lists.freedesktop.org
> Subject: Re: [Intel-gfx] [PATCH] drm/i915: Clamp linetime wm to <64usec
> 
> 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?
Perhaps, issue where seen in Dell UP2715K. 

> 
> >
> > > 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