| Message ID | 20220720140214.199492-2-dakr@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/virtio: use drm managed resources | expand |
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@redhat.com> > --- > 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. The rest of this patch looks OK. Sam > .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
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@redhat.com> >> --- >> 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 >
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, .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); }
Use drm managed resource allocation (drmm_universal_plane_alloc()) in order to cleanup/simplify drm plane .destroy callback. Signed-off-by: Danilo Krummrich <dakr@redhat.com> --- drivers/gpu/drm/virtio/virtgpu_plane.c | 30 +++++++------------------- 1 file changed, 8 insertions(+), 22 deletions(-)