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 <[email protected]> > --- > 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 <[email protected]> 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
