Hi Javier Am 16.05.22 um 15:00 schrieb Javier Martinez Canillas:
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. NoMaybe "both of these using using an" or something like that ?
Of course.
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.
Ok.
+ } +} + /** * 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.
I usually name the goto labels after the function that comes next. It's not really pretty here, but being inconsistent would probably annoy me more than the underscores.
Best regards Thomas
-- Thomas Zimmermann Graphics Driver Developer SUSE Software Solutions Germany GmbH Maxfeldstr. 5, 90409 Nürnberg, Germany (HRB 36809, AG Nürnberg) Geschäftsführer: Ivo Totev
Attachment:
OpenPGP_signature
Description: OpenPGP digital signature
