Re: [PATCH v4 2/3] drm/xe/i2c: Fix the interrupt handling

[Raag Jadav <raag.jadav@intel.com>]

On Wed, Jul 15, 2026 at 12:53:16PM +0300, Heikki Krogerus wrote:
> On Wed, Jul 15, 2026 at 08:26:22AM +0200, Raag Jadav wrote:
> > On Mon, Jul 13, 2026 at 05:56:00PM +0200, Heikki Krogerus wrote:
> > > The platforms that support the interrupt from the I2C
> > > adapter can not handle the amount of interrupts the adapter
> > > generates because of the way the IRQ is routed in the
> > > hardware. The I2C controller driver has to be kept in
> > > polling mode because of that.
> > > 
> > > The AMC MCU can still generate critical alerts that have to
> > > be handled. The interrupt from SMBus Alert is left enabled
> > > and handled separately in the Xe. The alerts from the AMC
> > > will cause the device to be declared wedged for now.

...

> > > +	    memcmp(&response.message, &request->message, sizeof(struct amc_message))) {
> > > +		dev_err(&client->dev, "response does not match the request\n");

Forgot add, use drm_*() variants where possible.

> > > +		return;
> > > +	}
> > > +
> > > +	if (response.error) {
> > > +		dev_err(&client->dev, "AMC error 0x%02x\n", response.error);

Ditto.

> > > +		return;
> > > +	}
> > > +
> > > +	dev_dbg(&client->dev, "%s: Alert reason: %d\n", __func__, response.value);

Ditto.

> > See below [1].
> > 
> > > +	switch (response.value) {
> > > +	case AMC_ALERT_FW_DOWNLOAD:
> > > +	case AMC_ALERT_THERMAL_TRIP:
> > > +	case AMC_ALERT_OOB_REQUEST:
> > > +	case AMC_ALERT_OOB_RESET:
> > > +	case AMC_ALERT_CATERR:
> > > +		xe_device_declare_wedged(i2c_client_to_xe_device(client));
> > > +		break;
> > > +	default:
> > > +		break;
> > > +	}
> > > +}
> > 
> > ...
> > 
> > > @@ -181,8 +187,7 @@ void xe_i2c_irq_handler(struct xe_device *xe, u32 master_ctl)
> > >  	if (!(master_ctl & I2C_IRQ) || !xe_i2c_irq_present(xe))
> > >  		return;
> > >  
> > > -	/* Forward interrupt to I2C adapter */
> > > -	generic_handle_irq_safe(xe->i2c->adapter_irq);
> > > +	xe_i2c_handle_smbus_alert(xe->i2c);
> > 
> > [1] Can we move the below re-assert code to wq now? Or do you suspect any
> > side-effects?
> 
> I think that you know this better than I do. But at this point
> interrupt is cleared, so why should we wait for the wq?

When does AMC clear the alert signal? Is it when you query from the wq?
If the answer is yes, there's a possibility we might end up with an
interrupt storm here.

> To play it safe, can we change this as a followup if necessary?

Sure, I'll leave it to you.

> > >  	/* Deassert after I2C adapter clears the interrupt */
> > >  	xe_mmio_rmw32(mmio, I2C_CONFIG_CMD, 0, PCI_COMMAND_INTX_DISABLE);
> > > @@ -212,45 +217,6 @@ void xe_i2c_irq_postinstall(struct xe_device *xe)
> > >  	xe_mmio_rmw32(mmio, I2C_CONFIG_CMD, PCI_COMMAND_INTX_DISABLE, 0);
> > >  }
> > 
> > ...
> > 
> > > -#define XE_I2C_MAX_CLIENTS		3
> > > -
> > >  #define XE_I2C_EP_COOKIE_DEVICE		0xde
> > >  
> > >  /* Endpoint Capabilities */
> > >  #define XE_I2C_EP_CAP_IRQ		BIT(0)
> > >  
> > > +enum XE_I2C_CLIENT {
> > > +	XE_I2C_CLIENT_AMC,
> > > +	XE_I2C_MAX_CLIENTS = 3,
> > 
> > I know it was already like this but I probably missed why do we have 3
> > (atleast from driver POV)?
> 
> That is the maximum number of clients these platforms can support.
> The AMC address is actually at a fixed offset 1. I'll change this so
> that XE_I2C_CLIENT_AMC matches the offset:
> 
> enum XE_I2C_CLIENT {
> 	XE_I2C_CLIENT_AMC = 1,
> 	XE_I2C_MAX_CLIENTS = 3,
> };
> 
> That probable makes this a bit more clear (right?).

With that I think these can be used as ep.addr[] indexes as well, but
I'll leave them at your mercy.

Raag