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

[Kahola, Mika]

> -----Original Message-----
> From: Jani Nikula <jani.nikula at linux.intel.com>
> Sent: Wednesday, November 1, 2023 12:51 PM
> To: Kahola, Mika <mika.kahola at intel.com>; intel-gfx at lists.freedesktop.org
> Subject: Re: [Intel-gfx] [PATCH] drm/i915/mtl: Clear possible sticky bits on PICA message bus
> 
> On Wed, 01 Nov 2023, Mika Kahola <mika.kahola at intel.com> wrote:
> > 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.
> 
> Note that I don't know if this is the right thing to do, or the right place to do this, but I'll just comment on the *implementation*,
> i.e. please wait for proper review before addressing my comments.
> 
> >
> > 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);
> 
> I think it's slightly confusing to use val here, as it's then passed on to intel_cx0_wait_for_ack() and you're left wondering if that's
> required or just reuse of the val variable.

This is a bit controversial patch. I have seen "reference" implementation which does similar thing. From BSpec 65101 PORT_P2M_MSGBUS_STATUS bit31 "Response Ready" the bit is set by HW on read completion or when write ack is received by SW after read. This means that we either have response pending or not. Now, the whole idea to float patch was to spark discussion if this is the way we should do this. 

There is an issue of sporadically fail reading or writing to the PICA message bus. This extra step might be something that we need (locally I couldn't trigger the the failure with this). One can interpret the spec in two ways. One is that the hw clears these bits when executing read command and hence we could write the value back as such. The other one is that we need to read the register and clear the bits like we already do with intel_clear_response_ready_flag() function.

One sidenote is that I couldn't find this extra step from the spec. Maybe need for something like this was discovered later?

-Mika-

> 
> This should do the same thing in one line, without reusing val:
> 
> 	intel_de_rmw(i915, XELPDP_PORT_P2M_MSGBUS_STATUS(port, lane), 0, 0);
> 
> > +
> >  	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);
> > +
> >  	intel_de_write(i915, XELPDP_PORT_M2P_MSGBUS_CTL(port, lane),
> >  		       XELPDP_PORT_M2P_TRANSACTION_PENDING |
> >  		       (committed ? XELPDP_PORT_M2P_COMMAND_WRITE_COMMITTED :
> 
> --
> Jani Nikula, Intel