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

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

[Ville Syrjala]

From: Ville Syrjälä <ville.syrjala@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@intel.com>
Signed-off-by: Ville Syrjälä <ville.syrjala@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);
 }
 
 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

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Lisovskiy, Stanislav]

On Thu, Jun 25, 2020 at 11:00:03PM +0300, Ville Syrjala wrote:
> From: Ville Syrjälä <ville.syrjala@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@intel.com>
> Signed-off-by: Ville Syrjälä <ville.syrjala@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.

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

Re: [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@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@intel.com>
> > Signed-off-by: Ville Syrjälä <ville.syrjala@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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Saarinen, Jani]

Hi, 

> -----Original Message-----
> From: Intel-gfx <intel-gfx-bounces@lists.freedesktop.org> On Behalf Of Ville Syrjälä
> Sent: perjantai 26. kesäkuuta 2020 16.47
> To: Lisovskiy, Stanislav <stanislav.lisovskiy@intel.com>
> Cc: intel-gfx@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@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@intel.com>
> > > Signed-off-by: Ville Syrjälä <ville.syrjala@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@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [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@lists.freedesktop.org> On Behalf Of Ville Syrjälä
> > Sent: perjantai 26. kesäkuuta 2020 16.47
> > To: Lisovskiy, Stanislav <stanislav.lisovskiy@intel.com>
> > Cc: intel-gfx@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@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@intel.com>
> > > > Signed-off-by: Ville Syrjälä <ville.syrjala@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@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

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

[Saarinen, Jani]

Hi, 

> -----Original Message-----
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Sent: perjantai 26. kesäkuuta 2020 17.09
> To: Saarinen, Jani <jani.saarinen@intel.com>
> Cc: Lisovskiy, Stanislav <stanislav.lisovskiy@intel.com>; intel-
> gfx@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@lists.freedesktop.org> On Behalf
> > > Of Ville Syrjälä
> > > Sent: perjantai 26. kesäkuuta 2020 16.47
> > > To: Lisovskiy, Stanislav <stanislav.lisovskiy@intel.com>
> > > Cc: intel-gfx@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@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@intel.com>
> > > > > Signed-off-by: Ville Syrjälä <ville.syrjala@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@lists.freedesktop.org
> > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> --
> Ville Syrjälä
> Intel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Lisovskiy, Stanislav]

On Fri, Jun 26, 2020 at 04:46:41PM +0300, Ville Syrjälä wrote:
> 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@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@intel.com>
> > > Signed-off-by: Ville Syrjälä <ville.syrjala@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.

Htotal from modeline "848x480": 30 14874 848 896 928 1008 480 483 488 494 0x40 0x9
is 1008.

According to the formula above htotal(1008)*1000*8 / 14874(crtc_clock) = 542.154

So what's the catch? :)

Stan
> 
> > 
> > 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@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Ville Syrjälä]

On Fri, Jun 26, 2020 at 06:13:36PM +0300, Lisovskiy, Stanislav wrote:
> On Fri, Jun 26, 2020 at 04:46:41PM +0300, Ville Syrjälä wrote:
> > 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@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@intel.com>
> > > > Signed-off-by: Ville Syrjälä <ville.syrjala@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.
> 
> Htotal from modeline "848x480": 30 14874 848 896 928 1008 480 483 488 494 0x40 0x9
> is 1008.
> 
> According to the formula above htotal(1008)*1000*8 / 14874(crtc_clock) = 542.154
> 
> So what's the catch? :)

What catch? Looks totally consistent to me.

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

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

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

[Lisovskiy, Stanislav]

On Sat, Jun 27, 2020 at 07:57:31PM +0300, Ville Syrjälä wrote:
> On Fri, Jun 26, 2020 at 06:13:36PM +0300, Lisovskiy, Stanislav wrote:
> > On Fri, Jun 26, 2020 at 04:46:41PM +0300, Ville Syrjälä wrote:
> > > 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@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@intel.com>
> > > > > Signed-off-by: Ville Syrjälä <ville.syrjala@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.
> > 
> > Htotal from modeline "848x480": 30 14874 848 896 928 1008 480 483 488 494 0x40 0x9
> > is 1008.
> > 
> > According to the formula above htotal(1008)*1000*8 / 14874(crtc_clock) = 542.154
> > 
> > So what's the catch? :)
> 
> What catch? Looks totally consistent to me.

I meant as I understood from your comment we were supposed to get 68 usec linetime, not
542.

Reviewed-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>

> 
> > 
> > Stan
> > > 
> > > > 
> > > > 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
> 
> -- 
> Ville Syrjälä
> Intel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Ville Syrjälä]

On Mon, Jun 29, 2020 at 11:24:53AM +0300, Lisovskiy, Stanislav wrote:
> On Sat, Jun 27, 2020 at 07:57:31PM +0300, Ville Syrjälä wrote:
> > On Fri, Jun 26, 2020 at 06:13:36PM +0300, Lisovskiy, Stanislav wrote:
> > > On Fri, Jun 26, 2020 at 04:46:41PM +0300, Ville Syrjälä wrote:
> > > > 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@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@intel.com>
> > > > > > Signed-off-by: Ville Syrjälä <ville.syrjala@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.
> > > 
> > > Htotal from modeline "848x480": 30 14874 848 896 928 1008 480 483 488 494 0x40 0x9
> > > is 1008.
> > > 
> > > According to the formula above htotal(1008)*1000*8 / 14874(crtc_clock) = 542.154
> > > 
> > > So what's the catch? :)
> > 
> > What catch? Looks totally consistent to me.
> 
> I meant as I understood from your comment we were supposed to get 68 usec linetime, not
> 542.

It's in units of .125 usec, or put another way .3 binary fixed point.

> 
> Reviewed-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>
> 
> > 
> > > 
> > > Stan
> > > > 
> > > > > 
> > > > > 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
> > 
> > -- 
> > Ville Syrjälä
> > Intel

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

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

[Lisovskiy, Stanislav]

On Mon, Jun 29, 2020 at 06:48:05PM +0300, Ville Syrjälä wrote:
> On Mon, Jun 29, 2020 at 11:24:53AM +0300, Lisovskiy, Stanislav wrote:
> > On Sat, Jun 27, 2020 at 07:57:31PM +0300, Ville Syrjälä wrote:
> > > On Fri, Jun 26, 2020 at 06:13:36PM +0300, Lisovskiy, Stanislav wrote:
> > > > On Fri, Jun 26, 2020 at 04:46:41PM +0300, Ville Syrjälä wrote:
> > > > > 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@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@intel.com>
> > > > > > > Signed-off-by: Ville Syrjälä <ville.syrjala@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.
> > > > 
> > > > Htotal from modeline "848x480": 30 14874 848 896 928 1008 480 483 488 494 0x40 0x9
> > > > is 1008.
> > > > 
> > > > According to the formula above htotal(1008)*1000*8 / 14874(crtc_clock) = 542.154
> > > > 
> > > > So what's the catch? :)
> > > 
> > > What catch? Looks totally consistent to me.
> > 
> > I meant as I understood from your comment we were supposed to get 68 usec linetime, not
> > 542.
> 
> It's in units of .125 usec, or put another way .3 binary fixed point.

Yep, found this in BSpec already for WM_LINETIME reg. 
  
> 
> > 
> > Reviewed-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>
> > 
> > > 
> > > > 
> > > > Stan
> > > > > 
> > > > > > 
> > > > > > 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
> > > 
> > > -- 
> > > Ville Syrjälä
> > > Intel
> 
> -- 
> Ville Syrjälä
> Intel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx