Re: [PATCH v3 3/3] drm: bridge: ti-sn65dsi83: Add error recovery mechanism

[Herve Codina <herve.codina@bootlin.com>]

Hi Alexander,

On Wed, 08 Jan 2025 11:54:49 +0100
Alexander Stein <alexander.stein@ew.tq-group.com> wrote:

[...]
> >  #include <drm/drm_atomic_helper.h>
> >  #include <drm/drm_bridge.h>
> > +#include <drm/drm_drv.h> /* DRM_MODESET_LOCK_ALL_BEGIN() needs drm_drv_uses_atomic_modeset() */  
> 
> Shouldn't this include be added to include/drm/drm_modeset_lock.h instead?

Yes indeed. I will change that in the next iteration.

> 
> >  #include <drm/drm_mipi_dsi.h>
> >  #include <drm/drm_of.h>
> >  #include <drm/drm_panel.h>
> > @@ -147,6 +150,9 @@ struct sn65dsi83 {
> >  	struct regulator		*vcc;
> >  	bool				lvds_dual_link;
> >  	bool				lvds_dual_link_even_odd_swap;
> > +	bool				use_irq;
> > +	struct delayed_work		monitor_work;
> > +	struct work_struct		reset_work;  
> 
> Can you please rebase? You are missing commit d2b8c6d549570
> ("drm/bridge: ti-sn65dsi83: Add ti,lvds-vod-swing optional properties")

Sure, I will rebase.

[...]
> > +static void sn65dsi83_handle_errors(struct sn65dsi83 *ctx)
> > +{
> > +	unsigned int irq_stat;
> > +	int ret;
> > +
> > +	/*
> > +	 * Schedule a reset in case of:
> > +	 *  - the bridge doesn't answer
> > +	 *  - the bridge signals an error
> > +	 */
> > +
> > +	ret = regmap_read(ctx->regmap, REG_IRQ_STAT, &irq_stat);
> > +	if (ret || irq_stat)
> > +		schedule_work(&ctx->reset_work);  
> 
> Shouldn't you clear the error bits as well?

Thanks for pointing that.

I can clear the error bit but further more, I probably need to simply
disable the interrupt.

In some cases, we observed i2c access failure. In that cases clearing error
bits is simply not possible.

To avoid some possible interrupt storms (the chip detect a failure, set its
interrupt line but could be not accessible anymore), the best thing to do
is to disable the interrupt line here, let the reset work to do its job
performing a full reset of the device and re-enabling the interrupt line
when needed, probably in sn65dsi83_atomic_enable().

What do you think about that?

Best regards,
Hervé