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

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

Hi Maxime,

On Wed, 18 Dec 2024 16:54:02 +0100
Maxime Ripard <mripard@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.

I understood during v1 review that I need to reset only one output path, the
path where the bridge is present. In other words, the path from the encoder
attached to the bridge to the related connector and not all connectors
related to all encoders attached to the crtc.

Do you confirm that setting crtc_state->connectors_changed = true is the
correct way to do?

Best regards,
Hervé