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