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 <[email protected]> in case the rename gets done as a follow-up. > >Regards, >Suraj Kandpal > >> >> -- >> Gustavo Sousa >> >> > >> >Bspec: 65101 >> >Signed-off-by: Suraj Kandpal <[email protected]> >> >--- >> > 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 >> >
