Re: [Intel-gfx] [PATCH] drm/i915: Clamp linetime wm to <64usec
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ä
> > > > > > >
> > > > > > > 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
> > > > > > > Signed-off-by: Ville Syrjälä
> > > > > > > ---
> > > > > > > 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
> >
> > >
> > > >
> > > > 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(line
Re: [Intel-gfx] [PATCH] drm/i915: Clamp linetime wm to <64usec
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ä
> > > > > >
> > > > > > 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
> > > > > > Signed-off-by: Ville Syrjälä
> > > > > > ---
> > > > > > 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
>
> >
> > >
> > > 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
Re: [Intel-gfx] [PATCH] drm/i915: Clamp linetime wm to <64usec
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ä
> > > > >
> > > > > 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
> > > > > Signed-off-by: Ville Syrjälä
> > > > > ---
> > > > > 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
>
> >
> > 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 st
Re: [Intel-gfx] [PATCH] drm/i915: Clamp linetime wm to <64usec
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ä
> > > >
> > > > 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
> > > > Signed-off-by: Ville Syrjälä
> > > > ---
> > > > 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
Re: [Intel-gfx] [PATCH] drm/i915: Clamp linetime wm to <64usec
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ä
> > >
> > > 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
> > > Signed-off-by: Ville Syrjälä
> > > ---
> > > 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
Hi,
> -Original Message-
> From: Ville Syrjälä
> Sent: perjantai 26. kesäkuuta 2020 17.09
> To: Saarinen, Jani
> Cc: Lisovskiy, Stanislav ; intel-
> g...@lists.freedesktop.org
> Subject: Re: [Intel-gfx] [PATCH] drm/i915: Clamp linetime wm to <64usec
>
> On Fri, Jun 26, 2020 at 02:03:23PM +, Saarinen, Jani wrote:
> > Hi,
> >
> > > -Original Message-
> > > From: Intel-gfx On Behalf
> > > Of Ville Syrjälä
> > > Sent: perjantai 26. kesäkuuta 2020 16.47
> > > To: Lisovskiy, Stanislav
> > > 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ä
> > > > >
> > > > > 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
> > > > > Signed-off-by: Ville Syrjälä
> > > > > ---
> > > > > 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 stru
Re: [Intel-gfx] [PATCH] drm/i915: Clamp linetime wm to <64usec
On Fri, Jun 26, 2020 at 02:03:23PM +, Saarinen, Jani wrote:
> Hi,
>
> > -Original Message-
> > From: Intel-gfx On Behalf Of
> > Ville Syrjälä
> > Sent: perjantai 26. kesäkuuta 2020 16.47
> > To: Lisovskiy, Stanislav
> > 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ä
> > > >
> > > > 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
> > > > Signed-off-by: Ville Syrjälä
> > > > ---
> > > > 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,
> > > > +
> &g
Re: [Intel-gfx] [PATCH] drm/i915: Clamp linetime wm to <64usec
Hi,
> -Original Message-
> From: Intel-gfx On Behalf Of Ville
> Syrjälä
> Sent: perjantai 26. kesäkuuta 2020 16.47
> To: Lisovskiy, Stanislav
> 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ä
> > >
> > > 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
> > > Signed-off-by: Ville Syrjälä
> > > ---
> > > 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
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ä
> >
> > 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
> > Signed-off-by: Ville Syrjälä
> > ---
> > 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
On Thu, Jun 25, 2020 at 11:00:03PM +0300, Ville Syrjala wrote:
> From: Ville Syrjälä
>
> 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
> Signed-off-by: Ville Syrjälä
> ---
> 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
[Intel-gfx] [PATCH] drm/i915: Clamp linetime wm to <64usec
From: Ville Syrjälä
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
Signed-off-by: Ville Syrjälä
---
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
