Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Wed, 20 Jul 2022 17:17:22 +0200
From:   Sam Ravnborg <sam@ravnborg.org>
To:     Danilo Krummrich <dakr@redhat.com>
Cc:     airlied@linux.ie, daniel@ffwll.ch, kraxel@redhat.com,
        linux-kernel@vger.kernel.org, dri-devel@lists.freedesktop.org
Subject: Re: [PATCH RESEND 1/2] drm/virtio: plane: use drm managed resources
Message-ID: <YtgcgovHX6jAqbt8@ravnborg.org>
References: <20220720140214.199492-1-dakr@redhat.com>
 <20220720140214.199492-2-dakr@redhat.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20220720140214.199492-2-dakr@redhat.com>
Precedence: bulk

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