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.
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()
Retrieve the connector state with drm_atomic_get_connector_state()
but you pointed out the bridge->encoder->crtc field.
Should I use this field or use the &drm_connector_state.crtc with the drm
connector state retrieved from bridge->base.state->state using the proposed
sequence?
[1] https://elixir.bootlin.com/linux/v6.13-rc1/source/include/drm/drm_encoder.h#L180
Best regards,
Hervé
