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

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

Hi Maxime,

On Tue, 17 Dec 2024 18:30:52 +0100
Maxime Ripard <mripard@kernel.org> wrote:

> On Tue, Dec 17, 2024 at 03:32:15PM +0100, Herve Codina wrote:
> > In some cases observed during ESD tests, the TI SN65DSI83 cannot recover
> > from errors by itself. A full restart of the bridge is needed in those
> > cases to have the bridge output LVDS signals again.
> > 
> > Also, during tests, cases were observed where reading the status of the
> > bridge was not even possible. Indeed, in those cases, the bridge stops
> > to acknowledge I2C transactions. Only a full reset of the bridge (power
> > off/on) brings back the bridge to a functional state.
> > 
> > The TI SN65DSI83 has some error detection capabilities. Introduce an
> > error recovery mechanism based on this detection.
> > 
> > The errors detected are signaled through an interrupt. On system where
> > this interrupt is not available, the driver uses a polling monitoring
> > fallback to check for errors. When an error is present or when reading
> > the bridge status leads to an I2C failure, the recovery process is
> > launched.
> > 
> > Restarting the bridge needs to redo the initialization sequence. This
> > initialization sequence has to be done with the DSI data lanes driven in
> > LP11 state. In order to do that, the recovery process resets the whole
> > output path (i.e the path from the encoder to the connector) where the
> > bridge is located.
> > 
> > Signed-off-by: Herve Codina <herve.codina@bootlin.com>
> > ---
> >  drivers/gpu/drm/bridge/ti-sn65dsi83.c | 142 ++++++++++++++++++++++++++
> >  1 file changed, 142 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi83.c b/drivers/gpu/drm/bridge/ti-sn65dsi83.c
> > index e6264514bb3f..f3d66d17f28c 100644
> > --- a/drivers/gpu/drm/bridge/ti-sn65dsi83.c
> > +++ b/drivers/gpu/drm/bridge/ti-sn65dsi83.c
> > @@ -35,9 +35,12 @@
> >  #include <linux/of_graph.h>
> >  #include <linux/regmap.h>
> >  #include <linux/regulator/consumer.h>
> > +#include <linux/timer.h>
> > +#include <linux/workqueue.h>
> >  
> >  #include <drm/drm_atomic_helper.h>
> >  #include <drm/drm_bridge.h>
> > +#include <drm/drm_drv.h> /* DRM_MODESET_LOCK_ALL_BEGIN() need drm_drv_uses_atomic_modeset() */
> >  #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;
> >  };
> >  
> >  static const struct regmap_range sn65dsi83_readable_ranges[] = {
> > @@ -328,6 +334,106 @@ static u8 sn65dsi83_get_dsi_div(struct sn65dsi83 *ctx)
> >  	return dsi_div - 1;
> >  }
> >  
> > +static int sn65dsi83_reset_drm_output(struct sn65dsi83 *sn65dsi83)
> > +{
> > +	struct drm_atomic_state *state = ERR_PTR(-EINVAL);
> > +	struct drm_device *dev = sn65dsi83->bridge.dev;
> > +	struct drm_modeset_acquire_ctx ctx;
> > +	struct drm_connector *connector;
> > +	int err;
> > +
> > +	/*
> > +	 * Reset components available from the encoder to the connector.
> > +	 * To do that, we disable then re-enable the connector linked to the
> > +	 * encoder.
> > +	 *
> > +	 * This way, drm core will reconfigure each components. In our case,
> > +	 * this will force the previous component to go back in LP11 mode and
> > +	 * so allow the reconfiguration of SN64DSI83 bridge.
> > +	 *
> > +	 * Keep the lock during the whole operation to be atomic.
> > +	 */
> > +
> > +	DRM_MODESET_LOCK_ALL_BEGIN(dev, ctx, 0, err);
> > +
> > +	state = drm_atomic_helper_duplicate_state(dev, &ctx);
> > +	if (IS_ERR(state)) {
> > +		err = PTR_ERR(state);
> > +		goto unlock;
> > +	}
> > +
> > +	connector = drm_atomic_get_old_connector_for_encoder(state,
> > +							     sn65dsi83->bridge.encoder);
> > +	if (!connector) {
> > +		err = -EINVAL;
> > +		goto unlock;
> > +	}
> > +
> > +	err = drm_atomic_helper_disable_connector(connector, &ctx);
> > +	if (err < 0)
> > +		goto unlock;
> > +
> > +	/* Restore original state to re-enable the connector */
> > +	err = drm_atomic_helper_commit_duplicated_state(state, &ctx);
> > +
> > +unlock:
> > +	DRM_MODESET_LOCK_ALL_END(dev, ctx, err);
> > +	if (!IS_ERR(state))
> > +		drm_atomic_state_put(state);
> > +	return err;
> > +}  
> 
> In the previous version, we advised to create a generic helper similar
> to vc4 and i915 reset_pipe() and and intel_modeset_commit_pipes().
> 
> It looks like you chose a different path. Can you expand why?
> 

I didn't choose a different path.
I created the drm_atomic_helper_disable_connector(). Maybe it is not enough.

I can move (copy/paste) sn65dsi83_reset_drm_output() to a new helper:
  int drm_atomic_helper_disable_output(struct drm_encoder *encoder)

Is it what you expect?

Also, are operations done in current sn65dsi83_reset_drm_output() correct
in order to reset the output? It works on my system but what is your
feedback on operations performed.

Best regards,
Hervé