RE: [PATCH 3/3] drm/i915/cx0: Clear response ready & error bit

[Gustavo Sousa <gustavo.sousa@intel.com>]

Quoting Kandpal, Suraj (2025-12-31 01:59:29-03:00)
>> Subject: Re: [PATCH 3/3] drm/i915/cx0: Clear response ready & error bit
>> 
>> Quoting Suraj Kandpal (2025-12-30 05:31:42-03:00)
>> >Clear the response ready and error bit of PORT_P2M_MESSAGE_BUS_STATUS
>> >before writing the transaction pending bit of PORT_M2P_MSGBUS_CTL as
>> >that is a hard requirement. If not done we find that the PHY hangs
>> >since it ends up in a weird state if left idle for more than 1 hour.
>> 
>> Since the series title refers to suspend/resume, is there an easy way of
>> reproducing this via some power state transition?
>
>Sadly no we did try to no avail went as low as DC9 too. Tried sleep then wake,
>Tried hibernating too.

I see.  I was wondering if DMC could be at play here.

So the only known way of reproducing this is to leave it idle for more
than 1 hour?  Do you know if that happens if we load the driver with
DC5/6 disabled (i.e. enable_dc=0)?

>
>> 
>> I'm wondering if we are looking at a driver issue here or if this is really
>> something else.  I see that we usually call intel_cx0_bus_reset() in error paths,
>> which contains a call to intel_clear_response_ready_flag(), but it could end up
>> being not called if the reset times out.
>
>Yes the reset times out and this is because PHY is in hanged state here
>
>> 
>> Do we see error messages from the driver when the PHY hangs?
>
>Yes we do see a PHY hang when we are at the stage of swing programming setting
>And the error happens when we try to read the msg access bit itself which is because the PHY ends up in
>Weird state where it has a write ack but no response ready bit getting set.
>But with my observation I can certainly say this is PHY issue.
>I also double confirmed this behavior with windows folks and their code where they said they saw an issue in the same lines.
>Also the bspec basically adds this as a disclaimer that response ready and error bit should be cleared no matter what before writing
>The transaction pending bit.

Agreed on the need to clear the response ready bit before starting
another transaction, so the patch looks fine.  I believe Jani's
request to rename the function wouldn't necessarily change the semantics
of this patch, so

Reviewed-by: Gustavo Sousa <gustavo.sousa@intel.com>

in case the rename gets done as a follow-up.

>
>Regards,
>Suraj Kandpal
>
>> 
>> --
>> Gustavo Sousa
>> 
>> >
>> >Bspec: 65101
>> >Signed-off-by: Suraj Kandpal <suraj.kandpal@intel.com>
>> >---
>> > drivers/gpu/drm/i915/display/intel_cx0_phy.c | 4 ++++
>> > 1 file changed, 4 insertions(+)
>> >
>> >diff --git a/drivers/gpu/drm/i915/display/intel_cx0_phy.c
>> >b/drivers/gpu/drm/i915/display/intel_cx0_phy.c
>> >index 5edd293b533b..5ebc3404eee2 100644
>> >--- a/drivers/gpu/drm/i915/display/intel_cx0_phy.c
>> >+++ b/drivers/gpu/drm/i915/display/intel_cx0_phy.c
>> >@@ -222,6 +222,8 @@ static int __intel_cx0_read_once(struct intel_encoder
>> *encoder,
>> >                 return -ETIMEDOUT;
>> >         }
>> >
>> >+        intel_clear_response_ready_flag(encoder, lane);
>> >+
>> >         intel_de_write(display, XELPDP_PORT_M2P_MSGBUS_CTL(display, port,
>> lane),
>> >                        XELPDP_PORT_M2P_TRANSACTION_PENDING |
>> >                        XELPDP_PORT_M2P_COMMAND_READ | @@ -293,6 +295,8
>> >@@ static int __intel_cx0_write_once(struct intel_encoder *encoder,
>> >                 return -ETIMEDOUT;
>> >         }
>> >
>> >+        intel_clear_response_ready_flag(encoder, lane);
>> >+
>> >         intel_de_write(display, XELPDP_PORT_M2P_MSGBUS_CTL(display, port,
>> lane),
>> >                        XELPDP_PORT_M2P_TRANSACTION_PENDING |
>> >                        (committed ?
>> XELPDP_PORT_M2P_COMMAND_WRITE_COMMITTED :
>> >--
>> >2.34.1
>> >