Re: [PATCH 66/75] drm/amdgpu: dm: Convert to atomic_create_state

[Maxime Ripard <mripard@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 2336 bytes --]

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 273 bytes --]