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

[Heikki Krogerus]

Hi Raag,

On Wed, Jul 15, 2026 at 01:55:08PM +0200, Raag Jadav wrote:
> 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.

I used deliberately the dev_*() here so I can use the i2c client
device, which to me feels more appropriate. With drm_*() I would need
to use the drm device, right?

But I can change this if you still prefer the drm_*(). Let me know.

> > > > +		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.

I'll move it to the wq. There are no side-effects.

> > > >  	xe_mmio_rmw32(mmio, I2C_CONFIG_CMD, 0, PCI_COMMAND_INTX_DISABLE);
> > > >  	xe_mmio_rmw32(mmio, I2C_CONFIG_CMD, PCI_COMMAND_INTX_DISABLE, 0);

Thanks,

-- 
heikki