Re: [PATCH v4 07/20] drm/colorop: Create drm_atomic_helper_colorop_create_state()

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] Missing EXPORT_SYMBOL for drm_atomic_helper_colorop_create_state
- [Medium] Incomplete implementation: missing atomic_create_state hook in struct drm_colorop_funcs
--

commit 33d70aace7968630d61946321195c9263dae2576
Author: Maxime Ripard <mripard@kernel.org>
Subject: drm/colorop: Create drm_atomic_helper_colorop_create_state()

This commit introduces drm_atomic_helper_colorop_create_state() to allocate
and initialize a pristine colorop state. It is intended to separate state
initialization from hardware reset during operations like suspend and resume.

> It was thus decided to create a new hook that would allocate and
> initialize a pristine state without any side effect:
> atomic_create_state to untangle a bit some of it, and to separate the
> initialization with the actual reset one might need during a
> suspend/resume.
> 
> Continue the transition to the new pattern with drm_colorop.

Does this patch miss adding the new atomic_create_state hook mentioned in
the commit message?

Looking at struct drm_colorop_funcs, the atomic_create_state function
pointer does not appear to be added, and drm_colorop_reset() seems to
continue handling the state allocation directly. Does this omission cause a
regression where drivers are unable to hook into the state creation process
to subclass the colorop state?

> diff --git a/drivers/gpu/drm/drm_colorop.c b/drivers/gpu/drm/drm_colorop.c
> index 4c4d0a953e357..c0eecde8c1762 100644
> --- a/drivers/gpu/drm/drm_colorop.c
> +++ b/drivers/gpu/drm/drm_colorop.c

[ ... ]

> @@ -523,6 +523,29 @@ static void __drm_colorop_state_init(struct drm_colorop_state *colorop_state,
>  	}
>  }
>  
> +/**
> + * drm_atomic_helper_colorop_create_state - Allocates and initializes colorop atomic state
> + * @colorop: drm colorop
> + *
> + * Initializes a pristine @drm_colorop_state.
> + *
> + * RETURNS:
> + * Pointer to new colorop state, or ERR_PTR on failure.
> + */
> +struct drm_colorop_state *
> +drm_atomic_helper_colorop_create_state(struct drm_colorop *colorop)
> +{
> +	struct drm_colorop_state *state;
> +
> +	state = kzalloc_obj(*state);
> +	if (!state)
> +		return ERR_PTR(-ENOMEM);
> +
> +	__drm_colorop_state_init(state, colorop);
> +
> +	return state;
> +}
> +

Could the lack of an export declaration here cause a build regression for
loadable DRM modules?

Since drm_atomic_helper_colorop_create_state() is a core helper function
intended for drivers, modules attempting to reference it might encounter
unresolved symbol errors without an EXPORT_SYMBOL or EXPORT_SYMBOL_GPL
statement.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260512-drm-mode-config-init-v4-0-591dfdcc1bf9@kernel.org?part=7