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. No
Maybe "both of these using using an" or something like that ?
Of course.
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.
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