Re: [PATCH RESEND 1/2] drm/virtio: plane: use drm managed resources

[Danilo Krummrich]

Hi Sam,

On 7/20/22 17:17, Sam Ravnborg wrote:
> Hi Danilo,
>
> thanks for submitting this patch.
>
> On Wed, Jul 20, 2022 at 04:02:13PM +0200, Danilo Krummrich wrote:
> > Use drm managed resource allocation (drmm_universal_plane_alloc()) in
> > order to cleanup/simplify drm plane .destroy callback.
> >
> > Signed-off-by: Danilo Krummrich <dakr@xxxxxxxxxx>
> > ---
> >   drivers/gpu/drm/virtio/virtgpu_plane.c | 30 +++++++-------------------
> >   1 file changed, 8 insertions(+), 22 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/virtio/virtgpu_plane.c b/drivers/gpu/drm/virtio/virtgpu_plane.c
> > index 6d3cc9e238a4..3008551d6a05 100644
> > --- a/drivers/gpu/drm/virtio/virtgpu_plane.c
> > +++ b/drivers/gpu/drm/virtio/virtgpu_plane.c
> > @@ -67,16 +67,10 @@ uint32_t virtio_gpu_translate_format(uint32_t drm_fourcc)
> >   	return format;
> >   }
> >   
> > -static void virtio_gpu_plane_destroy(struct drm_plane *plane)
> > -{
> > -	drm_plane_cleanup(plane);
> > -	kfree(plane);
> > -}
> > -
> >   static const struct drm_plane_funcs virtio_gpu_plane_funcs = {
> >   	.update_plane		= drm_atomic_helper_update_plane,
> >   	.disable_plane		= drm_atomic_helper_disable_plane,
> > -	.destroy		= virtio_gpu_plane_destroy,
> > +	.destroy		= drm_plane_cleanup,
>
>  From the documentation of drmm_universal_plane_alloc:
>
>      The @drm_plane_funcs.destroy hook must be NULL.
>
> So the above assignment looks wrong.

Good catch! Indeed it registers the drm_plane_cleanup() with 
drmm_add_action_or_reset() already.

I'll send a v2.

> The rest of this patch looks OK.
>
> 	Sam

Danilo

> >   	.reset			= drm_atomic_helper_plane_reset,
> >   	.atomic_duplicate_state = drm_atomic_helper_plane_duplicate_state,
> >   	.atomic_destroy_state	= drm_atomic_helper_plane_destroy_state,
> > @@ -379,11 +373,7 @@ struct drm_plane *virtio_gpu_plane_init(struct virtio_gpu_device *vgdev,
> >   	const struct drm_plane_helper_funcs *funcs;
> >   	struct drm_plane *plane;
> >   	const uint32_t *formats;
> > -	int ret, nformats;
> > -
> > -	plane = kzalloc(sizeof(*plane), GFP_KERNEL);
> > -	if (!plane)
> > -		return ERR_PTR(-ENOMEM);
> > +	int nformats;
> >   
> >   	if (type == DRM_PLANE_TYPE_CURSOR) {
> >   		formats = virtio_gpu_cursor_formats;
> > @@ -394,17 +384,13 @@ struct drm_plane *virtio_gpu_plane_init(struct virtio_gpu_device *vgdev,
> >   		nformats = ARRAY_SIZE(virtio_gpu_formats);
> >   		funcs = &virtio_gpu_primary_helper_funcs;
> >   	}
> > -	ret = drm_universal_plane_init(dev, plane, 1 << index,
> > -				       &virtio_gpu_plane_funcs,
> > -				       formats, nformats,
> > -				       NULL, type, NULL);
> > -	if (ret)
> > -		goto err_plane_init;
> > +
> > +	plane = drmm_universal_plane_alloc(dev, struct drm_plane, dev,
> > +					   1 << index, &virtio_gpu_plane_funcs,
> > +					   formats, nformats, NULL, type, NULL);
> > +	if (IS_ERR(plane))
> > +		return plane;
> >   
> >   	drm_plane_helper_add(plane, funcs);
> >   	return plane;
> > -
> > -err_plane_init:
> > -	kfree(plane);
> > -	return ERR_PTR(ret);
> >   }
> > --
> > 2.36.1