On Wed, 05 Feb 2025, Imre Deak <imre.deak@xxxxxxxxx> wrote:
> On Wed, Feb 05, 2025 at 02:35:18PM +0200, Jani Nikula wrote:
>> On Wed, 29 Jan 2025, Imre Deak <imre.deak@xxxxxxxxx> 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.
>> >
>> > Signed-off-by: Imre Deak <imre.deak@xxxxxxxxx>
>> > ---
>> > 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
