[PATCH RESEND 0/2] drm/virtio: use drm managed resources

[PATCH RESEND 0/2] drm/virtio: use drm managed resources

[Danilo Krummrich]

This patch series converts plain memory allocations for driver structures and
planes to drm managed allocations in order to cleanup/simply the corresponding
release/destroy callbacks.

Danilo Krummrich (2):
  drm/virtio: plane: use drm managed resources
  drm/virtio: kms: use drm managed resources

 drivers/gpu/drm/virtio/virtgpu_kms.c   | 16 ++++++--------
 drivers/gpu/drm/virtio/virtgpu_plane.c | 30 +++++++-------------------
 2 files changed, 15 insertions(+), 31 deletions(-)

-- 
2.36.1

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

[Danilo Krummrich]

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,
 	.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

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

[Sam Ravnborg]

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

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@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
> 

[PATCH RESEND 2/2] drm/virtio: kms: use drm managed resources

[Danilo Krummrich]

Allocate driver structures with drm managed resource allocators in order
to cleanup/simplify the drm driver .release callback.

Signed-off-by: Danilo Krummrich <dakr@redhat.com>
---
 drivers/gpu/drm/virtio/virtgpu_kms.c | 16 +++++++---------
 1 file changed, 7 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/virtio/virtgpu_kms.c b/drivers/gpu/drm/virtio/virtgpu_kms.c
index 3313b92db531..63ebe63ef409 100644
--- a/drivers/gpu/drm/virtio/virtgpu_kms.c
+++ b/drivers/gpu/drm/virtio/virtgpu_kms.c
@@ -28,6 +28,7 @@
 #include <linux/virtio_ring.h>
 
 #include <drm/drm_file.h>
+#include <drm/drm_managed.h>
 
 #include "virtgpu_drv.h"
 
@@ -66,10 +67,11 @@ static void virtio_gpu_get_capsets(struct virtio_gpu_device *vgdev,
 {
 	int i, ret;
 	bool invalid_capset_id = false;
+	struct drm_device *drm = vgdev->ddev;
 
-	vgdev->capsets = kcalloc(num_capsets,
-				 sizeof(struct virtio_gpu_drv_capset),
-				 GFP_KERNEL);
+	vgdev->capsets = drmm_kcalloc(drm, num_capsets,
+				      sizeof(struct virtio_gpu_drv_capset),
+				      GFP_KERNEL);
 	if (!vgdev->capsets) {
 		DRM_ERROR("failed to allocate cap sets\n");
 		return;
@@ -94,7 +96,7 @@ static void virtio_gpu_get_capsets(struct virtio_gpu_device *vgdev,
 
 		if (ret == 0 || invalid_capset_id) {
 			spin_lock(&vgdev->display_info_lock);
-			kfree(vgdev->capsets);
+			drmm_kfree(drm, vgdev->capsets);
 			vgdev->capsets = NULL;
 			spin_unlock(&vgdev->display_info_lock);
 			return;
@@ -126,7 +128,7 @@ int virtio_gpu_init(struct drm_device *dev)
 	if (!virtio_has_feature(dev_to_virtio(dev->dev), VIRTIO_F_VERSION_1))
 		return -ENODEV;
 
-	vgdev = kzalloc(sizeof(struct virtio_gpu_device), GFP_KERNEL);
+	vgdev = drmm_kzalloc(dev, sizeof(struct virtio_gpu_device), GFP_KERNEL);
 	if (!vgdev)
 		return -ENOMEM;
 
@@ -257,7 +259,6 @@ int virtio_gpu_init(struct drm_device *dev)
 	vgdev->vdev->config->del_vqs(vgdev->vdev);
 err_vqs:
 	dev->dev_private = NULL;
-	kfree(vgdev);
 	return ret;
 }
 
@@ -296,9 +297,6 @@ void virtio_gpu_release(struct drm_device *dev)
 
 	if (vgdev->has_host_visible)
 		drm_mm_takedown(&vgdev->host_visible_mm);
-
-	kfree(vgdev->capsets);
-	kfree(vgdev);
 }
 
 int virtio_gpu_driver_open(struct drm_device *dev, struct drm_file *file)
-- 
2.36.1

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

[Sam Ravnborg]

Hi Danilo,

On Wed, Jul 20, 2022 at 04:02:14PM +0200, Danilo Krummrich wrote:
> Allocate driver structures with drm managed resource allocators in order
> to cleanup/simplify the drm driver .release callback.
> 
> Signed-off-by: Danilo Krummrich <dakr@redhat.com>

This patch is already applied to drm-misc (drm-misc-next) as
90caf42527a40d09e0eed9fcbca08d757f4fd493

	Sam