Hi Leo, Thanks for reviewing this
On Mon, Oct 19, 2026 at 12:38:47PM -0400, Leo Li wrote:
> > @@ -736,11 +736,11 @@ dm_dp_add_mst_connector(struct
> > drm_dp_mst_topology_mgr *mgr,
> >
> > /*
> > * Initialize connector state before adding the connectror to drm and
> > * framebuffer lists
> > */
> > - amdgpu_dm_connector_funcs_reset(connector);
>
> I'm actually not sure why there was a state reset here.
> amdgpu_dm_connector_init_helper() is called earlier in this func and
> that already did a reset.
Yeah, it's something sashiko flagged too and I removed that call (and
updated the comment).
> > + connector->state = amdgpu_dm_connector_atomic_create_state(connector);
>
> That asside, looks like this can overwrite the alloc'd state from
> init_helper() without freeing. So to keep the previous behavior and
> avoid regressing, perhaps we can do this:
>
> conn_state = amdgpu_dm_connector_atomic_create_state(connector);
> if (!IS_ERR(conn_state)) {
> if (connector->state)
> connector->funcs->atomic_destroy_state(connector,
> connector->state);
> connector->state = conn_state;
> }
If we are to handle errors properly and remain equivalent, then
something like the following would be more accurate I guess?
if (connector->state) {
connector->funcs->atomic_destroy_state(connector, connector->state);
connector->state = NULL;
}
conn_state = amdgpu_dm_connector_atomic_create_state(connector);
if (IS_ERR(conn_state))
return NULL;
connector->state = conn_state;
Would you prefer that, or should I remove the call to create_state
entirely because it's redundant?
> > /*
> > * Some of the properties below require access to state, like bpc.
> > * Allocate some default initial connector state with our reset helper.
> > */
> > - if (wbcon->base.base.funcs->reset)
> > - wbcon->base.base.funcs->reset(&wbcon->base.base);
> > + if (!wbcon->base.base.state &&
> > + wbcon->base.base.funcs->atomic_create_state) {
> > + struct drm_connector_state *conn_state;
> > +
> > + conn_state =
> > amdgpu_dm_connector_atomic_create_state(&wbcon->base.base);
>
> Nit: functionally the same, but would
> `wbcon->base.base.funcs->atomic_create_state(&wbcon->base.base);` be
> clearer?
Totally, I'll fix it
Thanks!
Maxime
signature.asc
Description: PGP signature
