Re: [PATCH 1/4] drm/gem: Share code between drm_gem_fb_{begin, end}_cpu_access()

[Javier Martinez Canillas <javierm@redhat.com>]

Hello Thomas,

On 5/9/22 10:15, Thomas Zimmermann wrote:
> The error-recovery code in drm_gem_fb_begin() is the same pattern
> as drm_gem_fb_end(). Implement both this an internal helper. No

Maybe "both of these using using an" or something like that ?

> functional changes.
> 
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> ---
>  drivers/gpu/drm/drm_gem_framebuffer_helper.c | 62 ++++++++------------
>  1 file changed, 26 insertions(+), 36 deletions(-)
>

Nice cleanup. For the patch:

Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>

I just have a few minor comments below.

> diff --git a/drivers/gpu/drm/drm_gem_framebuffer_helper.c b/drivers/gpu/drm/drm_gem_framebuffer_helper.c
> index f4619803acd0..2fcacab9f812 100644
> --- a/drivers/gpu/drm/drm_gem_framebuffer_helper.c
> +++ b/drivers/gpu/drm/drm_gem_framebuffer_helper.c
> @@ -403,6 +403,27 @@ void drm_gem_fb_vunmap(struct drm_framebuffer *fb,
>  }
>  EXPORT_SYMBOL(drm_gem_fb_vunmap);
>  
> +static void __drm_gem_fb_end_cpu_access(struct drm_framebuffer *fb, enum dma_data_direction dir,
> +					unsigned int num_planes)
> +{
> +	struct dma_buf_attachment *import_attach;
> +	struct drm_gem_object *obj;
> +	int ret;
> +
> +	while (num_planes) {
> +		--num_planes;
> +		obj = drm_gem_fb_get_obj(fb, num_planes);
> +		if (!obj)
> +			continue;
> +		import_attach = obj->import_attach;
> +		if (!import_attach)
> +			continue;
> +		ret = dma_buf_end_cpu_access(import_attach->dmabuf, dir);
> +		if (ret)
> +			drm_err(fb->dev, "dma_buf_end_cpu_access() failed: %d\n", ret);

I wonder if would be useful to also print the plane index and access mode here.

> +	}
> +}
> +
>  /**
>   * drm_gem_fb_begin_cpu_access - prepares GEM buffer objects for CPU access
>   * @fb: the framebuffer
> @@ -422,7 +443,7 @@ int drm_gem_fb_begin_cpu_access(struct drm_framebuffer *fb, enum dma_data_direct
>  	struct dma_buf_attachment *import_attach;
>  	struct drm_gem_object *obj;
>  	size_t i;
> -	int ret, ret2;
> +	int ret;
>  
>  	for (i = 0; i < ARRAY_SIZE(fb->obj); ++i) {
>  		obj = drm_gem_fb_get_obj(fb, i);
> @@ -433,28 +454,13 @@ int drm_gem_fb_begin_cpu_access(struct drm_framebuffer *fb, enum dma_data_direct
>  			continue;
>  		ret = dma_buf_begin_cpu_access(import_attach->dmabuf, dir);
>  		if (ret)
> -			goto err_dma_buf_end_cpu_access;
> +			goto err___drm_gem_fb_end_cpu_access;

I wouldn't rename this. As I read it, the goto label doesn't denote what
function is called but rather what action is being taken in an error path.

Since finally what's being done is a dma_buf_end_cpu_access in the helper,
I think that just keeping err_dma_buf_end_cpu_access is enough. Also, the
additional underscores added make it harder to read IMO.

-- 
Best regards,

Javier Martinez Canillas
Linux Engineering
Red Hat