Re: [PATCH 44/70] drm/amdgpu: dm: Convert to atomic_create_state

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] The return value of `atomic_create_state()` is completely discarded during CRTC initialization, causing a memory leak and leaving `crtc->state` uninitialized (NULL).
--

--- Patch [44]: [PATCH 44/70] drm/amdgpu: dm: Convert to atomic_create_state ---
commit 680651b77d5bb2911f7b94020aa95ffc0ffde185
Author: Maxime Ripard <[email protected]>

drm/amdgpu: dm: Convert to atomic_create_state

The amdgpu display manager crtc implementation provides a custom reset hook.
This converts the implementation to use atomic_create_state with
__drm_atomic_helper_crtc_state_init() instead.

> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_crtc.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_crtc.c
> index d63688b9d93dd..e1cc74ba7a434 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_crtc.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_crtc.c
[ ... ]
> @@ -757,9 +756,9 @@ int amdgpu_dm_crtc_init(struct amdgpu_display_manager *dm,
>  
>  	drm_crtc_helper_add(&acrtc->base, &amdgpu_dm_crtc_helper_funcs);
>  
> -	/* Create (reset) the plane state */
> -	if (acrtc->base.funcs->reset)
> -		acrtc->base.funcs->reset(&acrtc->base);
> +	/* Create the plane state */
> +	if (acrtc->base.funcs->atomic_create_state)
> +		acrtc->base.funcs->atomic_create_state(&acrtc->base);

[Severity: High]
Does this drop the newly allocated state and leave acrtc->base.state
uninitialized (NULL)?

Previously, amdgpu_dm_crtc_reset_state() called
__drm_atomic_helper_crtc_reset(), which internally assigns the newly
allocated state to the crtc:

void __drm_atomic_helper_crtc_reset(struct drm_crtc *crtc,
                                    struct drm_crtc_state *crtc_state)
{
        if (crtc_state)
                __drm_atomic_helper_crtc_state_init(crtc_state, crtc);

        crtc->state = crtc_state;
}

With the new conversion, amdgpu_dm_crtc_create_state() allocates and returns
the state pointer:

static struct drm_crtc_state *amdgpu_dm_crtc_create_state(struct drm_crtc *crtc)
{
        struct dm_crtc_state *state;

        state = kzalloc_obj(*state);
        if (!state)
                return ERR_PTR(-ENOMEM);

        __drm_atomic_helper_crtc_state_init(&state->base, crtc);

        return &state->base;
}

However, amdgpu_dm_crtc_init() ignores the return value of
atomic_create_state() and does not assign it to the crtc.

Could this result in a memory leak of the returned dm_crtc_state object, as
well as a regression due to NULL pointer dereferences when crtc->state is
accessed later during driver initialization?

>  
>  	acrtc->max_cursor_width = dm->adev->dm.dc->caps.max_cursor_size;
>  	acrtc->max_cursor_height = dm->adev->dm.dc->caps.max_cursor_size;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=44