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

[Thomas Zimmermann]

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 <tzimmerm...@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 <javi...@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);

+                                       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);

+static void __drm_gem_fb_end_cpu_access(struct drm_framebuffer *fb, enum dma_data_direction dir,

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;

                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;

for (i = 0; i < ARRAY_SIZE(fb->obj); ++i) {

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
Thomas

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.

--
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

OpenPGP_signature
Description: OpenPGP digital signature