> -----Original Message-----
> From: Intel-xe <intel-xe-boun...@lists.freedesktop.org> On Behalf Of Jani 
> Nikula
> Sent: Wednesday, 5 February 2025 15.02
> To: Deak, Imre <imre.d...@intel.com>
> Cc: intel-gfx@lists.freedesktop.org; intel...@lists.freedesktop.org
> Subject: Re: [PATCH 07/17] drm/i915/ddi: Simplify waiting for a port to idle 
> via
> DDI_BUF_CTL
> 
> On Wed, 05 Feb 2025, Imre Deak <imre.d...@intel.com> wrote:
> > On Wed, Feb 05, 2025 at 02:35:18PM +0200, Jani Nikula wrote:
> >> On Wed, 29 Jan 2025, Imre Deak <imre.d...@intel.com> wrote:
> >> > When waiting for a port to idle, there is no point in
> >> > distinguishing the platform specific timeouts, instead of just using the
> maximum timeout.
> >>
> >> Why?
> >>
> >> All of this sounds kind of reasonable, but we'll need a better
> >> rationale than "there is no point".
> >
> > The rational is that there is no point in the complexity of specifying
> > an exact timeout and for that the suitable wait API. The sequence in
> > particular is not performance critical at all either and due to
> > scheduling it's not guaranteed anyhow how long the wait will last at
> > the given timescale. In the usual case where the wait succeeds the
> > actual time waited does not change with the increased timeout.
> 
> Fair. Just needs to be in the commit message. ;)
> 
> BR,
> Jani.
> 
> >
> >> > Simplify things accordingly, describing the Bspec platform specific
> >> > timeouts in code comments.
> >> >

Reviewed-by: Mika Kahola <mika.kah...@intel.com>

> >> > Signed-off-by: Imre Deak <imre.d...@intel.com>
> >> > ---
> >> >  drivers/gpu/drm/i915/display/intel_ddi.c | 78
> >> > +++++++++++-------------
> >> >  1 file changed, 36 insertions(+), 42 deletions(-)
> >> >
> >> > diff --git a/drivers/gpu/drm/i915/display/intel_ddi.c
> >> > b/drivers/gpu/drm/i915/display/intel_ddi.c
> >> > index 24c56d2aa5f31..d040558b5d029 100644
> >> > --- a/drivers/gpu/drm/i915/display/intel_ddi.c
> >> > +++ b/drivers/gpu/drm/i915/display/intel_ddi.c
> >> > @@ -177,69 +177,63 @@ static void hsw_prepare_hdmi_ddi_buffers(struct
> intel_encoder *encoder,
> >> >                         trans->entries[level].hsw.trans2);
> >> >  }
> >> >
> >> > -static void mtl_wait_ddi_buf_idle(struct drm_i915_private *i915,
> >> > enum port port)
> >> > +static i915_reg_t intel_ddi_buf_status_reg(struct intel_display
> >> > +*display, enum port port)
> >> >  {
> >> > -        int ret;
> >> > +        struct drm_i915_private *i915 = to_i915(display->drm);
> >>
> >> Please don't add new i915 uses, display will work just fine here.
> >>
> >> >
> >> > -        /* FIXME: find out why Bspec's 100us timeout is too short */
> >> > -        ret = wait_for_us((intel_de_read(i915, 
> >> > XELPDP_PORT_BUF_CTL1(i915,
> port)) &
> >> > -                           XELPDP_PORT_BUF_PHY_IDLE), 10000);
> >> > -        if (ret)
> >> > -                drm_err(&i915->drm, "Timeout waiting for DDI BUF %c to 
> >> > get
> idle\n",
> >> > -                        port_name(port));
> >> > +        if (DISPLAY_VER(display) >= 14)
> >> > +                return XELPDP_PORT_BUF_CTL1(i915, port);
> >> > +        else
> >> > +                return DDI_BUF_CTL(port);
> >> >  }
> >> >
> >> >  void intel_wait_ddi_buf_idle(struct drm_i915_private *dev_priv,
> >> >                               enum port port)
> >> >  {
> >> > -        if (IS_BROXTON(dev_priv)) {
> >> > +        struct intel_display *display = &dev_priv->display;
> >> > +
> >> > +        /*
> >> > +         * Bspec's platform specific timeouts:
> >> > +         * MTL+   : 100 us
> >> > +         * BXT    : fixed 16 us
> >> > +         * HSW-ADL: 8 us
> >> > +         *
> >> > +         * FIXME: MTL requires 10 ms based on tests, find out why 100 
> >> > us is too
> short
> >> > +         */
> >>
> >> Seems a bit odd to me to list all the platform specific timeouts, and
> >> then largely ignore them without explanation!
> >
> > It's documented so after any future platform requirement changes
> > (dropping support for a platform, adding a new platform with a new
> > timeout) can be applied to the timeout used below.
> >
> >> Also, 10 ms is several orders of magnitude longer than it should need
> >> to be on all platforms.
> >
> > I described above why this doesn't matter (in the usual case the wait
> > duration will not change).
> >
> >>
> >> > +        if (display->platform.broxton) {
> >> >                  udelay(16);
> >> >                  return;
> >> >          }
> >> >
> >> > -        if (wait_for_us((intel_de_read(dev_priv, DDI_BUF_CTL(port)) &
> >> > -                         DDI_BUF_IS_IDLE), 8))
> >> > -                drm_err(&dev_priv->drm, "Timeout waiting for DDI BUF %c 
> >> > to
> get idle\n",
> >> > +        static_assert(DDI_BUF_IS_IDLE == XELPDP_PORT_BUF_PHY_IDLE);
> >> > +        if (intel_de_wait_for_set(display, 
> >> > intel_ddi_buf_status_reg(display, port),
> >> > +                                  DDI_BUF_IS_IDLE, 10))
> >> > +                drm_err(display->drm, "Timeout waiting for DDI BUF %c 
> >> > to get
> >> > +idle\n",
> >> >                          port_name(port));
> >> >  }
> >> >
> >> >  static void intel_wait_ddi_buf_active(struct intel_encoder
> >> > *encoder)  {
> >> > -        struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
> >> > +        struct intel_display *display = to_intel_display(encoder);
> >> >          enum port port = encoder->port;
> >> > -        int timeout_us;
> >> > -        int ret;
> >> >
> >> > -        /* Wait > 518 usecs for DDI_BUF_CTL to be non idle */
> >> > -        if (DISPLAY_VER(dev_priv) < 10) {
> >> > +        /*
> >> > +         * Bspec's platform specific timeouts:
> >> > +         * MTL+             : 10000 us
> >> > +         * DG2              : 1200 us
> >> > +         * TGL-ADL combo PHY: 1000 us
> >> > +         * TGL-ADL TypeC PHY: 3000 us
> >> > +         * HSW-ICL          : fixed 518 us
> >> > +         */
> >> > +        if (DISPLAY_VER(display) < 10) {
> >> >                  usleep_range(518, 1000);
> >> >                  return;
> >> >          }
> >> >
> >> > -        if (DISPLAY_VER(dev_priv) >= 14) {
> >> > -                timeout_us = 10000;
> >> > -        } else if (IS_DG2(dev_priv)) {
> >> > -                timeout_us = 1200;
> >> > -        } else if (DISPLAY_VER(dev_priv) >= 12) {
> >> > -                if (intel_encoder_is_tc(encoder))
> >> > -                        timeout_us = 3000;
> >> > -                else
> >> > -                        timeout_us = 1000;
> >> > -        } else {
> >> > -                timeout_us = 500;
> >> > -        }
> >> > -
> >> > -        if (DISPLAY_VER(dev_priv) >= 14)
> >> > -                ret = _wait_for(!(intel_de_read(dev_priv,
> >> > -
>       XELPDP_PORT_BUF_CTL1(dev_priv, port)) &
> >> > -                                  XELPDP_PORT_BUF_PHY_IDLE),
> >> > -                                timeout_us, 10, 10);
> >> > -        else
> >> > -                ret = _wait_for(!(intel_de_read(dev_priv, 
> >> > DDI_BUF_CTL(port)) &
> DDI_BUF_IS_IDLE),
> >> > -                                timeout_us, 10, 10);
> >> > -
> >> > -        if (ret)
> >> > -                drm_err(&dev_priv->drm, "Timeout waiting for DDI BUF %c 
> >> > to
> get active\n",
> >> > +        static_assert(DDI_BUF_IS_IDLE == XELPDP_PORT_BUF_PHY_IDLE);
> >> > +        if (intel_de_wait_for_clear(display, 
> >> > intel_ddi_buf_status_reg(display,
> port),
> >> > +                                    DDI_BUF_IS_IDLE, 10))
> >> > +                drm_err(display->drm, "Timeout waiting for DDI BUF %c 
> >> > to get
> >> > +active\n",
> >> >                          port_name(port));
> >> >  }
> >> >
> >> > @@ -3067,7 +3061,7 @@ static void mtl_disable_ddi_buf(struct
> intel_encoder *encoder,
> >> >          intel_de_rmw(dev_priv, DDI_BUF_CTL(port), DDI_BUF_CTL_ENABLE, 
> >> > 0);
> >> >
> >> >          /* 3.c Poll for PORT_BUF_CTL Idle Status == 1, timeout after
> >> > 100us */
> >>
> >> Comment is now stale. (Which is why we should never add comments like
> >> that.)
> >
> > Right, I remove all these later in the patchset.
> >
> >>
> >> > -        mtl_wait_ddi_buf_idle(dev_priv, port);
> >> > +        intel_wait_ddi_buf_idle(dev_priv, port);
> >> >
> >> >          /* 3.d Disable D2D Link */
> >> >          mtl_ddi_disable_d2d_link(encoder);
> >>
> >> --
> >> Jani Nikula, Intel
> 
> --
> Jani Nikula, Intel

Reply via email to