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

[Maxime Ripard]

On Wed, Dec 18, 2024 at 05:37:28PM +0100, Herve Codina wrote:
> Hi Maxime,
> 
> On Wed, 18 Dec 2024 16:54:02 +0100
> Maxime Ripard <mripard at kernel.org> wrote:
> 
> > > > > +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.  
> > 
> > It's not that it's not enough, it's that you're not doing the same
> > thing, see below.
> > 
> > > 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.  
> > 
> > You don't need any of that. Both the reset_pipe() and
> > intel_modeset_commit_pipes() functions will flag the connectors as
> > updated in the commit, and the core will consider that it needs to
> > disable / enable the encoders and bridges below that CRTC.
> > 
> > See
> > https://elixir.bootlin.com/linux/v6.12.5/source/drivers/gpu/drm/drm_atomic_helper.c#L1155
> > https://elixir.bootlin.com/linux/v6.12.5/source/drivers/gpu/drm/drm_atomic_helper.c#L1476
> > 
> > So you really only need to convert any of these two functions into a
> > helper, and it does exactly what you need.
> > 
> 
> I see but if I set crtc_state->connectors_changed = true; as it is done in
> reset_pipe(), in my understanding, all outputs will be reset.

Not all outputs, all active outputs connected to that CRTC. If you have
only one encoder connected to that CRTC, which is pretty typical on ARM
platforms, it's equivalent to what you're asking for.

And we should probably shut down the CRTC (and thus all active outputs)
anyway. Some encoders and bridges have internal FIFOs/state machines
that need to be enabled disabled at specific points during the
initialization, and the CRTC is a part of that.

Maxime
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 273 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/dri-devel/attachments/20241219/3a1e587e/attachment.sig>