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

[Maxime Ripard <mripard@kernel.org>]

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

On Tue, Oct 20, 2026 at 12:11:56PM -0400, Leo Li wrote:
> Thanks for cleaning up all the drivers, are these all part of the
> DRM_MODE_ATOMIC_RESET effort?

Somewhat, yes. It actually originated from the state readout work I've
been doing, but it's now a dependency to enable DRM_MODE_ATOMIC_RESET
indeed.

> > 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?
> 
> My preference would be to copy existing behavior, and keep the scope of this
> series to just dropping the reset hook. Dropping the redundant create_state and
> handling associated testing/regressions can be a followup.
> 
> In my parsing of the old reset behavior, we don't return a NULL connector on
> error. Rather, we keep the existing connector->state and return the connector as
> is. The rest of reset() only runs if state was alloc'd successfully. My previous
> snippet should copy this.

Ack. I'll use your earlier suggestion then

Maxime

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