On Tue, 2025-09-16 at 20:01 -0600, Alex Hung wrote:
>
>
> On 9/5/25 11:12, Louis Chauvet wrote:
> >
> >
> > Le 15/08/2025 à 05:50, Alex Hung a écrit :
> > > The functions are to clean up color pipeline when a device driver
> > > fails to create its color pipeline.
> > >
> > > Signed-off-by: Alex Hung <alex.h...@amd.com>
> > > Reviewed-by: Daniel Stone <dani...@collabora.com>
> > > Reviewed-by: Simon Ser <cont...@emersion.fr>
> > > Reviewed-by: Melissa Wen <m...@igalia.com>
> > > ---
> > > v11:
> > > - destroy function takes drm_device *dev instead of drm_plane
> > > *plane
> > > (Nícolas Prado)
> > >
> > > v9:
> > > - Move from from latest commit to here, and
> > > drm_colorop_pipeline_destroy
> > > is called in respective commits.
> > >
> > > drivers/gpu/drm/drm_colorop.c | 35
> > > +++++++++++++++++++++++++++++++++++
> > > include/drm/drm_colorop.h | 2 ++
> > > 2 files changed, 37 insertions(+)
> > >
> > > diff --git a/drivers/gpu/drm/drm_colorop.c b/drivers/gpu/drm/
> > > drm_colorop.c
> > > index 7b3ecf7ddd11..6930d39c8ad3 100644
> > > --- a/drivers/gpu/drm/drm_colorop.c
> > > +++ b/drivers/gpu/drm/drm_colorop.c
> > > @@ -135,6 +135,41 @@ static int drm_plane_colorop_init(struct
> > > drm_device *dev, struct drm_colorop *co
> > > return ret;
> > > }
> > > +/**
> > > + * drm_colorop_cleanup - Cleanup a drm_colorop object in
> > > color_pipeline
> > > + *
> > > + * @colorop: The drm_colorop object to be cleaned
> > > + */
> > > +static void drm_colorop_cleanup(struct drm_colorop *colorop)
> > > +{
> > > + struct drm_device *dev = colorop->dev;
> > > + struct drm_mode_config *config = &dev->mode_config;
> > > +
> > > + list_del(&colorop->head);
> > > + config->num_colorop--;
> > > +
> > > + kfree(colorop->state);
> > > +}
> > > +
> > > +/**
> > > + * drm_colorop_pipeline_destroy - Helper for color pipeline
> > > destruction
> > > + *
> > > + * @dev: - The drm_device containing the drm_planes with the
> > > color_pipelines
> > > + *
> > > + * Provides a default color pipeline destroy handler for
> > > drm_device.
> > > + */
> > > +void drm_colorop_pipeline_destroy(struct drm_device *dev)
> > > +{
> > > + struct drm_mode_config *config = &dev->mode_config;
> > > + struct drm_colorop *colorop, *next;
> > > +
> > > + list_for_each_entry_safe(colorop, next, &config-
> > > >colorop_list,
> > > head) {
> > > + drm_colorop_cleanup(colorop);
> > > + kfree(colorop);
> >
> > This free here seems a bit strange. I don't see any requirement on
> > the
> > colorop pointer in the init function, so we can expect it to be
> > embedded
> > in a bigger structure, so this kfree may affect a non-allocated
> > pointer.
> >
> > I would expect one of:
> >
> > - a clear documentation in drm_plane_colorop_*_init documentation
> > that
> > explicitly says that you need to pass a kzalloc pointer => very
> > error
> > prone, if the user don't read carefully the documentation it may
> > lead to
> > undefined behavior
> >
> > - that drm_plane_colorop_*_init will do the kzalloc itself (so we
> > garantee that the pointer is always a kzalloc pointer) => it will
> > forbid
> > to embed colorop structure in bigger structure. I don't think this
> > is
> > the case today, but I don't know if this can become a limitation
> > for
> > other drivers.
>
> Yes it makes to have kzalloc and kfree done both in vkms/amdgpu or
> both
> in drm_*.
>
> Does the following change make sense to you?
>
> diff --git a/drivers/gpu/drm/drm_colorop.c
> b/drivers/gpu/drm/drm_colorop.c
> index 1551b86471ce..67aa443e53e7 100644
> --- a/drivers/gpu/drm/drm_colorop.c
> +++ b/drivers/gpu/drm/drm_colorop.c
>
> @@ -214,6 +216,13 @@ int drm_plane_colorop_curve_1d_init(struct
> drm_device *dev, struct drm_colorop *
> struct drm_property *prop;
> int ret;
>
> + colorop = kzalloc(sizeof(struct drm_colorop), GFP_KERNEL);
> + if (!colorop) {
> + drm_err(dev, "KMS: Failed to allocate colorop\n");
> + ret = -ENOMEM;
> + goto cleanup;
> + }
> +
> if (!supported_tfs) {
> drm_err(dev,
> "No supported TFs for new 1D curve colorop
> on [PLANE:%d:%s]\n",
> diff --git a/drivers/gpu/drm/vkms/vkms_colorop.c
> b/drivers/gpu/drm/vkms/vkms_colorop.c
> index 0191ac44dec0..f11dca61b5ce 100644
> --- a/drivers/gpu/drm/vkms/vkms_colorop.c
> +++ b/drivers/gpu/drm/vkms/vkms_colorop.c
> @@ -24,12 +24,6 @@ static int vkms_initialize_color_pipeline(struct
> drm_plane *plane, struct drm_pr
> memset(ops, 0, sizeof(ops));
>
> /* 1st op: 1d curve */
> - ops[i] = kzalloc(sizeof(*ops[i]), GFP_KERNEL);
> - if (!ops[i]) {
> - drm_err(dev, "KMS: Failed to allocate colorop\n");
> - ret = -ENOMEM;
> - goto cleanup;
> - }
>
> ret = drm_plane_colorop_curve_1d_init(dev, ops[i], plane,
> supported_tfs,
>
> DRM_COLOROP_FLAG_ALLOW_BYPASS);
>
>
Well, as Louis mentions, this doesn't play well with embedding a
colorop into a larger struct, which is something I'm relying on to
track additional bits for post-blend colorops in the MediaTek driver in
[1].
I don't see an issue with the existing solution. From the function's
documentation: "Provides a default color pipeline destroy handler for
drm_device.", "default" is the key word here, each driver is currently
responsible for allocating every colorop and cleaning it up, and this
function is a helper for the common case where a drm_colorop is not
embedded into another struct. For the cases where it is, the driver
should create its own destroy() that properly frees the pointer for the
container struct, which is exactly what I'm doing in [1]. Perhaps we
should just add to the destroy() documentation that it frees the
pointer and so drivers embedding the struct should implement their own
helper.
On that train of thought, I think it would make sense to make
drm_colorop_cleanup() public [2] as part of this series so that those
drivers can reuse it.
[1]
https://lore.kernel.org/dri-devel/20250822-mtk-post-blend-color-pipeline-v1-0-a9446d4ac...@collabora.com/T/#ma6b44d451ed84c9c6b32d26e6192ad951612444d
[2]
https://lore.kernel.org/dri-devel/20250822-mtk-post-blend-color-pipeline-v1-0-a9446d4ac...@collabora.com/T/#m15e8f52a03d711a4d3ab9cecc7b9cafb2954c489
>
--
Thanks,
Nícolas
