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

[Maxime Ripard]

On Tue, Feb 04, 2025 at 04:34:04PM +0100, Herve Codina wrote:
> On Tue, 4 Feb 2025 16:17:10 +0100
> Maxime Ripard <mripard@xxxxxxxxxx> wrote:
> 
> > Hi,
> > 
> > On Fri, Jan 17, 2025 at 09:12:13AM +0100, Herve Codina wrote:
> > > Hi Maxime,
> > > 
> > > On Thu, 16 Jan 2025 09:38:45 +0100
> > > Maxime Ripard <mripard@xxxxxxxxxx> wrote:
> > >   
> > > > On Tue, Jan 14, 2025 at 01:54:56PM +0100, Herve Codina wrote:  
> > > > > Hi Maxime,
> > > > > 
> > > > > On Tue, 14 Jan 2025 08:40:51 +0100
> > > > > Maxime Ripard <mripard@xxxxxxxxxx> wrote:
> > > > > 
> > > > > ...
> > > > >     
> > > > > > >  
> > > > > > > +static int sn65dsi83_reset_pipe(struct sn65dsi83 *sn65dsi83)
> > > > > > > +{
> > > > > > > +	struct drm_atomic_state *state = ERR_PTR(-EINVAL);
> > > > > > > +	struct drm_device *dev = sn65dsi83->bridge.dev;
> > > > > > > +	struct drm_connector_state *connector_state;
> > > > > > > +	struct drm_modeset_acquire_ctx ctx;
> > > > > > > +	struct drm_connector *connector;
> > > > > > > +	int err;
> > > > > > > +
> > > > > > > +	/*
> > > > > > > +	 * Reset active outputs of the related CRTC.
> > > > > > > +	 *
> > > > > > > +	 * This way, drm core will reconfigure each components in the CRTC
> > > > > > > +	 * outputs path. 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;
> > > > > > > +	}      
> > > > > > 
> > > > > > No, you must not allocate a new state for this, you need to reuse the
> > > > > > existing state. You'll find it in bridge->base.state->state.    
> > > > > 
> > > > > Thanks for pointing that. I didn't know about bridge->base.state->state.
> > > > > 
> > > > > I will use that if using the state is still relevant (see next comment).
> > > > >     
> > > > > >     
> > > > > > > +	state->acquire_ctx = &ctx;
> > > > > > > +
> > > > > > > +	connector = drm_atomic_get_old_connector_for_encoder(state,
> > > > > > > +							     sn65dsi83->bridge.encoder);
> > > > > > > +	if (!connector) {
> > > > > > > +		err = -EINVAL;
> > > > > > > +		goto unlock;
> > > > > > > +	}
> > > > > > > +
> > > > > > > +	connector_state = drm_atomic_get_connector_state(state, connector);
> > > > > > > +	if (IS_ERR(connector_state)) {
> > > > > > > +		err = PTR_ERR(connector_state);
> > > > > > > +		goto unlock;
> > > > > > > +	}
> > > > > > > +
> > > > > > > +	err = drm_atomic_helper_reset_pipe(connector_state->crtc, &ctx);
> > > > > > > +	if (err < 0)
> > > > > > > +		goto unlock;      
> > > > > > 
> > > > > > And you'll find the crtc in bridge->encoder->crtc.    
> > > > > 
> > > > > I am a bit confused. I looked at the drm_encoder structure [1] and the crtc
> > > > > field available in this structure should not be used by atomic drivers. They
> > > > > should rely on &drm_connector_state.crtc.    
> > > > 
> > > > You're right, it's deprecated but used by most bridges anyway.
> > > > 
> > > > I made a series of changes after reviewing your series to address some
> > > > issues with the current bridge API, most notably
> > > > 
> > > > https://lore.kernel.org/dri-devel/20250115-bridge-connector-v1-25-9a2fecd886a6@xxxxxxxxxx/  
> > > 
> > > Thanks for pointing that, indeed, it clarify many things!
> > >   
> > > >   
> > > > > In my case, I have the feeling that I should get the ctrc from the current
> > > > > state (i.e. bridge->base.state->state) using the sequence provided in this
> > > > > current patch:
> > > > >   Retrieve the connector with drm_atomic_get_old_connector_for_encoder()    
> > > > 
> > > > Retrieving the old connector makes no sense though. It's the connector
> > > > that was formerly associated with your encoder. It might work, it might
> > > > not, it's not what you're looking for.
> > > >   
> > > > >   Retrieve the connector state with drm_atomic_get_connector_state()    
> > > > 
> > > > drm_atomic_get_connector_state will allocate and pull the connector
> > > > state into the drm_atomic_state, even if it wasn't part of it before, so
> > > > it's not great. And you don't need it in the first place, you only need
> > > > the current active CRTC.  
> > > 
> > > Yes, I agree with that, I only need the active CRTC.
> > > 
> > > I tried to get the current atomic_state from:
> > >   1) bridge->base.state->state
> > >   2) drm_bridge_state->base.state
> > > 
> > > In both cases, it is NULL. Looking at Sima's reply in your series
> > > explained that:
> > >   https://lore.kernel.org/dri-devel/Z4juJy7kKPbI2BDb@phenom.ffwll.local/
> > > 
> > > If I understood correctly those pointers are explicitly cleared.
> > > 
> > > So, with all of that, either:
> > >   a) I wait for your series to be applied in order to use your the crtc field from
> > >      drm_bridge_state added by:
> > >        https://lore.kernel.org/dri-devel/20250115-bridge-connector-v1-25-9a2fecd886a6@xxxxxxxxxx/#t
> > >   b) I use the old school bridge->encoder->crtc for the moment
> > > 
> > > Do you mind if I use the bridge->encoder->crtc way for the next iteration of
> > > my series?  
> > 
> > Yeah, it makes sense.
> 
> I already send a wrong v4 (sorry) and a correct v5 with modifications in
> this way :)
> 
> > 
> > Still, it would be great if you could test my series on your setup and see if it helps :)
> 
> Of course, I can test updated version of your series.
> 
> I already try to get the current atomic_state exactly the same way as you do
> in your series and the pointer is NULL in my case.

I sent a second version today, let me know if it works.

Maxime

Attachment: signature.asc
Description: PGP signature