On 2026-10-20 09:08, Maxime Ripard wrote:
> Hi Leo,
>
> Thanks for reviewing this
Hi Maxime,
Thanks for cleaning up all the drivers, are these all part of the
DRM_MODE_ATOMIC_RESET effort?
>
> 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.
Thanks,
Leo
>
>>> /*
>>> * 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
