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 <[email protected]> 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 <[email protected]> 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/[email protected]/ > > 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/[email protected]/ > > 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/[email protected]/#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. Still, it would be great if you could test my series on your setup and see if it helps :) Maxime
signature.asc
Description: PGP signature
