Re: [PATCH 43/60] drm/atmel-hlcdc: Convert to atomic_create_state

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] Hardware modification inside `atomic_create_state` violates the DRM API contract.
- [Medium] The function returns `-EINVAL` instead of `-ENOMEM` when memory allocation for DMA descriptors fails.
--

--- Patch [43]: [PATCH 43/60] drm/atmel-hlcdc: Convert to atomic_create_state ---
commit 28ffcf15c4fc4a6c19751342c0e8a7f7e0d7a988
Author: Maxime Ripard <mripard@kernel.org>
drm/atmel-hlcdc: Convert to atomic_create_state

This commit converts the atmel-hlcdc plane reset implementation to use
atomic_create_state. The conversion was primarily done using a Coccinelle
semantic patch.

> The plane reset implementation creates a custom state
> subclass, but only initializes a pristine state without resetting any
> hardware. This is equivalent to what atomic_create_state expects.

Is this description of the old reset function accurate?

Looking at atmel_hlcdc_csc_init(), which is called via the lcdc_csc_init()
hook, it appears to perform direct register writes:

atmel_hlcdc_csc_init() {
    ...
    atmel_hlcdc_layer_write_cfg(&plane->layer,
                                desc->layout.csc + i,
                                hlcdc_csc_coeffs[i]);
}

This seems to indicate the old reset function did actually touch hardware,
contrary to the commit message.

> diff --git a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_plane.c b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_plane.c
> index f17a832351e9d..ff35251a01846 100644
> --- a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_plane.c
> +++ b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_plane.c

[ ... ]

> @@ -1183,36 +1183,33 @@ static void atmel_hlcdc_plane_atomic_destroy_state(struct drm_plane *p,

[ ... ]

> -	if (state) {
> -		if (atmel_hlcdc_plane_alloc_dscrs(p, state)) {
> -			kfree(state);
> -			drm_err(p->dev,
> -				"Failed to allocate initial plane state\n");
> -			return;
> -		}
> -		__drm_atomic_helper_plane_reset(p, &state->base);
> +	if (!state)
> +		return ERR_PTR(-ENOMEM);
> +
> +	if (atmel_hlcdc_plane_alloc_dscrs(p, state)) {
> +		kfree(state);
> +		drm_err(p->dev, "Failed to allocate initial plane state\n");
> +		return ERR_PTR(-EINVAL);

[Severity: Medium]
Since atmel_hlcdc_plane_alloc_dscrs() internally returns -ENOMEM when
dma_pool_alloc() fails, should this propagate that error or return
ERR_PTR(-ENOMEM) instead of hardcoding -EINVAL?

>  	}
> +	__drm_atomic_helper_plane_state_init(&state->base, p);
>  
>  	if (plane->layer.desc->layout.csc)
>  		dc->desc->ops->lcdc_csc_init(plane, plane->layer.desc);

[Severity: Medium]
Does this break the DRM API contract for atomic_create_state?

The atomic_create_state hook is documented to have no side effects, but as
noted above, lcdc_csc_init() writes directly to hardware registers.

If the driver is later initialized using
drm_mode_config_create_initial_state() before power domains are fully
enabled, could these hardware accesses cause a crash or hang?

> +
> +	return &state->base;
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260709-drm-no-more-plane-reset-v1-0-302d986fe5f0@kernel.org?part=43