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@xxxxxxxxxxxxxxx>
> > > > > > >
> > > > > > > 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@xxxxxxxxx>
> > > > > > > Signed-off-by: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> > > > > > > ---
> > > > > > > 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@xxxxxxxxx>
> >
> > >
> > > >
> > > > 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@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
