[Intel-gfx] [PATCH] drm/i915/mtl: Clear possible sticky bits on PICA message bus

[Kahola, Mika]

> -----Original Message-----
> From: Sousa, Gustavo <gustavo.sousa at intel.com>
> Sent: Thursday, November 2, 2023 4:23 PM
> To: Kahola, Mika <mika.kahola at intel.com>; intel-gfx at lists.freedesktop.org
> Cc: Kahola, Mika <mika.kahola at intel.com>; Nikula, Jani <jani.nikula at intel.com>; Syrjala, Ville <ville.syrjala at intel.com>
> Subject: Re: [PATCH] drm/i915/mtl: Clear possible sticky bits on PICA message bus
> 
> Quoting Mika Kahola (2023-11-01 07:31:01-03:00)
> >It is possible that sticky bits or error bits are left on message bus
> >status register. Reading and then writing the value back to messagebus
> >status register clears all possible sticky bits and errors.
> >
> >Signed-off-by: Mika Kahola <mika.kahola at intel.com>
> >---
> > drivers/gpu/drm/i915/display/intel_cx0_phy.c | 14 ++++++++++++++
> > 1 file changed, 14 insertions(+)
> >
> >diff --git a/drivers/gpu/drm/i915/display/intel_cx0_phy.c
> >b/drivers/gpu/drm/i915/display/intel_cx0_phy.c
> >index b2ad4c6172f6..f439f0c7b400 100644
> >--- a/drivers/gpu/drm/i915/display/intel_cx0_phy.c
> >+++ b/drivers/gpu/drm/i915/display/intel_cx0_phy.c
> >@@ -195,6 +195,13 @@ static int __intel_cx0_read_once(struct drm_i915_private *i915, enum port port,
> >                 return -ETIMEDOUT;
> >         }
> >
> >+        /*
> >+         * write XELPDP_PORT_P2M_MSGBUS_STATUS register after read to clear
> >+         * any error sticky bits set from previous transactions
> >+         */
> >+        val = intel_de_read(i915, XELPDP_PORT_P2M_MSGBUS_STATUS(port, lane));
> >+        intel_de_write(i915, XELPDP_PORT_P2M_MSGBUS_STATUS(port,
> >+ lane), val);
> >+
> >         intel_de_write(i915, XELPDP_PORT_M2P_MSGBUS_CTL(port, lane),
> >                        XELPDP_PORT_M2P_TRANSACTION_PENDING |
> >                        XELPDP_PORT_M2P_COMMAND_READ | @@ -262,6
> >+269,13 @@ static int __intel_cx0_write_once(struct drm_i915_private *i915, enum port port,
> >                 return -ETIMEDOUT;
> >         }
> >
> >+        /*
> >+         * write XELPDP_PORT_P2M_MSGBUS_STATUS register after read to clear
> >+         * any error sticky bits set from previous transactions
> >+         */
> >+        val = intel_de_read(i915, XELPDP_PORT_P2M_MSGBUS_STATUS(port, lane));
> >+        intel_de_write(i915, XELPDP_PORT_P2M_MSGBUS_STATUS(port,
> >+ lane), val);
> >+
> 
> Looking at the current state of the code, looks like to me that we already clear the bits from both the "success" and "failure" paths.
> For the "success"
> paths, that is done by a direct call to intel_clear_response_ready_flag(); for the "failure" case, the call to
> intel_clear_response_ready_flag() is done as part of intel_cx0_bus_reset().
> 
> Thus, considering that we start using the msgbus from a clean state, maybe these extra steps are not necessary? Have you tried
> adding a call to
> intel_cx0_bus_reset() as part of intel_cx0_phy_transaction_begin()?
That I haven't try to reset bus at the stage. I can give it a go and check what happens. To me it seems, that we are sometimes stuck at waiting the ack and eventually we time out and bail out. I have no clue why this happens from time to time. We already reset the bus after every read/write operation. In addition, a small delay seem to help and clear the sporadic read/write failures to the bus. However, this is more like a workaround and I'm not really happy about this sort of hack.

I will give a go for your suggestion and come back once I have the results.

Thanks!
-Mika-

> 
> Also, I think it would be good if we understood better were those uncleared bits are coming from...
> 
> --
> Gustavo Sousa
> 
> >         intel_de_write(i915, XELPDP_PORT_M2P_MSGBUS_CTL(port, lane),
> >                        XELPDP_PORT_M2P_TRANSACTION_PENDING |
> >                        (committed ? XELPDP_PORT_M2P_COMMAND_WRITE_COMMITTED :
> >--
> >2.34.1
> >