[PATCH 0/7] drm: Provide framebuffer dma-buf helpers

[PATCH 0/7] drm: Provide framebuffer dma-buf helpers

[Thomas Zimmermann]

Provide helpers that wrap dma_buf_{begin,end}_cpu_access() for all
GEM BOs attached to a framebuffer. Convert drivers and remove ugly
boilerplate code.

Thomas Zimmermann (7):
  drm/gem: Provide drm_gem_fb_{begin,end}_cpu_access() helpers
  drm/udl: Use framebuffer dma-buf helpers
  drm/mipi-dbi: Use framebuffer dma-buf helpers
  drm/gud: Use framebuffer dma-buf helpers
  drm/gm12u320: Use framebuffer dma-buf helpers
  drm/repaper: Use framebuffer dma-buf helpers
  drm/st7586: Use framebuffer dma-buf helpers

 drivers/gpu/drm/drm_gem_framebuffer_helper.c | 89 ++++++++++++++++++++
 drivers/gpu/drm/drm_mipi_dbi.c               | 20 ++---
 drivers/gpu/drm/gud/gud_pipe.c               | 13 ++-
 drivers/gpu/drm/tiny/gm12u320.c              | 19 ++---
 drivers/gpu/drm/tiny/repaper.c               | 18 +---
 drivers/gpu/drm/tiny/st7586.c                | 18 ++--
 drivers/gpu/drm/udl/udl_modeset.c            | 29 ++-----
 include/drm/drm_gem_framebuffer_helper.h     |  6 ++
 8 files changed, 130 insertions(+), 82 deletions(-)

--
2.32.0

[PATCH 1/7] drm/gem: Provide drm_gem_fb_{begin, end}_cpu_access() helpers

[Thomas Zimmermann]

Implement helpers drm_gem_fb_begin_cpu_access() and _end_cpu_access(),
which call the rsp dma-buf functions for all GEM BOs of the given
framebuffer.

Calls to dma_buf_end_cpu_access() can return an error code on failure,
while drm_gem_fb_end_cpu_access() does not. The latter runs during DRM's
atomic commit or during cleanup. Both cases don't allow for errors, so
leave out the return value.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/gpu/drm/drm_gem_framebuffer_helper.c | 89 ++++++++++++++++++++
 include/drm/drm_gem_framebuffer_helper.h     |  6 ++
 2 files changed, 95 insertions(+)

diff --git a/drivers/gpu/drm/drm_gem_framebuffer_helper.c b/drivers/gpu/drm/drm_gem_framebuffer_helper.c
index e2c68822e05c..94a1c0b0edfd 100644
--- a/drivers/gpu/drm/drm_gem_framebuffer_helper.c
+++ b/drivers/gpu/drm/drm_gem_framebuffer_helper.c
@@ -306,6 +306,95 @@ drm_gem_fb_create_with_dirty(struct drm_device *dev, struct drm_file *file,
 }
 EXPORT_SYMBOL_GPL(drm_gem_fb_create_with_dirty);
 
+/**
+ * drm_gem_fb_begin_cpu_access - prepares GEM buffer objects for CPU access
+ * @fb: the framebuffer
+ * @dir: access mode
+ *
+ * Prepares a framebuffer'S GEM buffer objects for CPU access. This function
+ * must be called before accessing the BO data within the kernel. For imported
+ * BOs, the function calls dma_buf_begin_cpu_access().
+ *
+ * See drm_gem_fb_end_cpu_access() for signalling the end of CPU access.
+ *
+ * Returns:
+ * 0 on success, or a negative errno code otherwise.
+ */
+int drm_gem_fb_begin_cpu_access(struct drm_framebuffer *fb, enum dma_data_direction dir)
+{
+	struct dma_buf_attachment *import_attach;
+	struct drm_gem_object *obj;
+	size_t i;
+	int ret, ret2;
+
+	for (i = 0; i < ARRAY_SIZE(fb->obj); ++i) {
+		obj = drm_gem_fb_get_obj(fb, i);
+		if (!obj)
+			continue;
+		import_attach = obj->import_attach;
+		if (!import_attach)
+			continue;
+		ret = dma_buf_begin_cpu_access(import_attach->dmabuf, dir);
+		if (ret)
+			goto err_dma_buf_end_cpu_access;
+	}
+
+	return 0;
+
+err_dma_buf_end_cpu_access:
+	while (i) {
+		--i;
+		obj = drm_gem_fb_get_obj(fb, i);
+		if (!obj)
+			continue;
+		import_attach = obj->import_attach;
+		if (!import_attach)
+			continue;
+		ret2 = dma_buf_end_cpu_access(import_attach->dmabuf, dir);
+		if (ret2) {
+			drm_err(fb->dev,
+				"dma_buf_end_cpu_access() failed during error handling: %d\n",
+				ret2);
+		}
+	}
+
+	return ret;
+}
+EXPORT_SYMBOL(drm_gem_fb_begin_cpu_access);
+
+/**
+ * drm_gem_fb_end_cpu_access - signals end of CPU access to GEM buffer objects
+ * @fb: the framebuffer
+ * @dir: access mode
+ *
+ * Signals the end of CPU access to the given framebuffer'S GEM buffer objects. This
+ * function must be paired with a corresponding call to drm_gem_fb_begin_cpu_access().
+ * For imported BOs, the function calls dma_buf_end_cpu_access().
+ *
+ * See also drm_gem_fb_begin_cpu_access().
+ */
+void drm_gem_fb_end_cpu_access(struct drm_framebuffer *fb, enum dma_data_direction dir)
+{
+	size_t i = ARRAY_SIZE(fb->obj);
+	struct dma_buf_attachment *import_attach;
+	struct drm_gem_object *obj;
+	int ret;
+
+	while (i) {
+		--i;
+		obj = drm_gem_fb_get_obj(fb, i);
+		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);
+	}
+}
+EXPORT_SYMBOL(drm_gem_fb_end_cpu_access);
+
 static __u32 drm_gem_afbc_get_bpp(struct drm_device *dev,
 				  const struct drm_mode_fb_cmd2 *mode_cmd)
 {
diff --git a/include/drm/drm_gem_framebuffer_helper.h b/include/drm/drm_gem_framebuffer_helper.h
index 6bdffc7aa124..5705722f0855 100644
--- a/include/drm/drm_gem_framebuffer_helper.h
+++ b/include/drm/drm_gem_framebuffer_helper.h
@@ -1,6 +1,9 @@
 #ifndef __DRM_GEM_FB_HELPER_H__
 #define __DRM_GEM_FB_HELPER_H__
 
+#include <linux/dma-buf.h>
+#include <linux/dma-buf-map.h>
+
 struct drm_afbc_framebuffer;
 struct drm_device;
 struct drm_fb_helper_surface_size;
@@ -34,6 +37,9 @@ struct drm_framebuffer *
 drm_gem_fb_create_with_dirty(struct drm_device *dev, struct drm_file *file,
 			     const struct drm_mode_fb_cmd2 *mode_cmd);
 
+int drm_gem_fb_begin_cpu_access(struct drm_framebuffer *fb, enum dma_data_direction dir);
+void drm_gem_fb_end_cpu_access(struct drm_framebuffer *fb, enum dma_data_direction dir);
+
 #define drm_is_afbc(modifier) \
 	(((modifier) & AFBC_VENDOR_AND_TYPE_MASK) == DRM_FORMAT_MOD_ARM_AFBC(0))
 
-- 
2.32.0

[PATCH 2/7] drm/udl: Use framebuffer dma-buf helpers

[Thomas Zimmermann]

Replace dma_buf_begin_cpu_access() with drm_gem_fb_begin_cpu_access();
same for _end_cpu_access(). Remove some boiler-plate code. No functional
changes.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/gpu/drm/udl/udl_modeset.c | 29 ++++++++---------------------
 1 file changed, 8 insertions(+), 21 deletions(-)

diff --git a/drivers/gpu/drm/udl/udl_modeset.c b/drivers/gpu/drm/udl/udl_modeset.c
index 8d98bf69d075..8a6b94b1511b 100644
--- a/drivers/gpu/drm/udl/udl_modeset.c
+++ b/drivers/gpu/drm/udl/udl_modeset.c
@@ -6,11 +6,8 @@
  * Copyright (C) 2009 Roberto De Ioris <roberto@unbit.it>
  * Copyright (C) 2009 Jaya Kumar <jayakumar.lkml@gmail.com>
  * Copyright (C) 2009 Bernie Thompson <bernie@plugable.com>
-
  */
 
-#include <linux/dma-buf.h>
-
 #include <drm/drm_atomic_helper.h>
 #include <drm/drm_crtc_helper.h>
 #include <drm/drm_damage_helper.h>
@@ -271,9 +268,8 @@ static int udl_handle_damage(struct drm_framebuffer *fb, const struct dma_buf_ma
 			     int x, int y, int width, int height)
 {
 	struct drm_device *dev = fb->dev;
-	struct dma_buf_attachment *import_attach = fb->obj[0]->import_attach;
 	void *vaddr = map->vaddr; /* TODO: Use mapping abstraction properly */
-	int i, ret, tmp_ret;
+	int i, ret;
 	char *cmd;
 	struct urb *urb;
 	struct drm_rect clip;
@@ -290,17 +286,14 @@ static int udl_handle_damage(struct drm_framebuffer *fb, const struct dma_buf_ma
 	else if ((clip.x2 > fb->width) || (clip.y2 > fb->height))
 		return -EINVAL;
 
-	if (import_attach) {
-		ret = dma_buf_begin_cpu_access(import_attach->dmabuf,
-					       DMA_FROM_DEVICE);
-		if (ret)
-			return ret;
-	}
+	ret = drm_gem_fb_begin_cpu_access(fb, DMA_FROM_DEVICE);
+	if (ret)
+		return ret;
 
 	urb = udl_get_urb(dev);
 	if (!urb) {
 		ret = -ENOMEM;
-		goto out_dma_buf_end_cpu_access;
+		goto out_drm_gem_fb_end_cpu_access;
 	}
 	cmd = urb->transfer_buffer;
 
@@ -313,7 +306,7 @@ static int udl_handle_damage(struct drm_framebuffer *fb, const struct dma_buf_ma
 				       &cmd, byte_offset, dev_byte_offset,
 				       byte_width);
 		if (ret)
-			goto out_dma_buf_end_cpu_access;
+			goto out_drm_gem_fb_end_cpu_access;
 	}
 
 	if (cmd > (char *)urb->transfer_buffer) {
@@ -329,14 +322,8 @@ static int udl_handle_damage(struct drm_framebuffer *fb, const struct dma_buf_ma
 
 	ret = 0;
 
-out_dma_buf_end_cpu_access:
-	if (import_attach) {
-		tmp_ret = dma_buf_end_cpu_access(import_attach->dmabuf,
-						 DMA_FROM_DEVICE);
-		if (tmp_ret && !ret)
-			ret = tmp_ret; /* only update ret if not set yet */
-	}
-
+out_drm_gem_fb_end_cpu_access:
+	drm_gem_fb_end_cpu_access(fb, DMA_FROM_DEVICE);
 	return ret;
 }
 
-- 
2.32.0

[PATCH 3/7] drm/mipi-dbi: Use framebuffer dma-buf helpers

[Thomas Zimmermann]

Replace dma_buf_begin_cpu_access() with drm_gem_fb_begin_cpu_access();
same for _end_cpu_access(). Remove some boiler-plate code. No functional
changes.

There's one left-over reference to the imported attachment that we
keep. GEM BOs with imported attachment are considered uncached and
enables special handling within the drm_fb_swab().

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/gpu/drm/drm_mipi_dbi.c | 20 +++++++-------------
 1 file changed, 7 insertions(+), 13 deletions(-)

diff --git a/drivers/gpu/drm/drm_mipi_dbi.c b/drivers/gpu/drm/drm_mipi_dbi.c
index 10b4e59384ae..71b646c4131f 100644
--- a/drivers/gpu/drm/drm_mipi_dbi.c
+++ b/drivers/gpu/drm/drm_mipi_dbi.c
@@ -7,7 +7,6 @@
 
 #include <linux/debugfs.h>
 #include <linux/delay.h>
-#include <linux/dma-buf.h>
 #include <linux/gpio/consumer.h>
 #include <linux/module.h>
 #include <linux/regulator/consumer.h>
@@ -202,21 +201,17 @@ int mipi_dbi_buf_copy(void *dst, struct drm_framebuffer *fb,
 {
 	struct drm_gem_object *gem = drm_gem_fb_get_obj(fb, 0);
 	struct drm_gem_cma_object *cma_obj = to_drm_gem_cma_obj(gem);
-	struct dma_buf_attachment *import_attach = gem->import_attach;
 	void *src = cma_obj->vaddr;
-	int ret = 0;
+	int ret;
 
-	if (import_attach) {
-		ret = dma_buf_begin_cpu_access(import_attach->dmabuf,
-					       DMA_FROM_DEVICE);
-		if (ret)
-			return ret;
-	}
+	ret = drm_gem_fb_begin_cpu_access(fb, DMA_FROM_DEVICE);
+	if (ret)
+		return ret;
 
 	switch (fb->format->format) {
 	case DRM_FORMAT_RGB565:
 		if (swap)
-			drm_fb_swab(dst, src, fb, clip, !import_attach);
+			drm_fb_swab(dst, src, fb, clip, !gem->import_attach);
 		else
 			drm_fb_memcpy(dst, src, fb, clip);
 		break;
@@ -229,9 +224,8 @@ int mipi_dbi_buf_copy(void *dst, struct drm_framebuffer *fb,
 		return -EINVAL;
 	}
 
-	if (import_attach)
-		ret = dma_buf_end_cpu_access(import_attach->dmabuf,
-					     DMA_FROM_DEVICE);
+	drm_gem_fb_end_cpu_access(fb, DMA_FROM_DEVICE);
+
 	return ret;
 }
 EXPORT_SYMBOL(mipi_dbi_buf_copy);
-- 
2.32.0

[PATCH 4/7] drm/gud: Use framebuffer dma-buf helpers

[Thomas Zimmermann]

Replace dma_buf_begin_cpu_access() with drm_gem_fb_begin_cpu_access();
same for _end_cpu_access(). Remove some boiler-plate code. No functional
changes.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/gpu/drm/gud/gud_pipe.c | 13 +++++--------
 1 file changed, 5 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/gud/gud_pipe.c b/drivers/gpu/drm/gud/gud_pipe.c
index 8f56bf618ac2..4d7a26b68a2e 100644
--- a/drivers/gpu/drm/gud/gud_pipe.c
+++ b/drivers/gpu/drm/gud/gud_pipe.c
@@ -3,7 +3,6 @@
  * Copyright 2020 Noralf Trønnes
  */
 
-#include <linux/dma-buf.h>
 #include <linux/lz4.h>
 #include <linux/usb.h>
 #include <linux/workqueue.h>
@@ -15,6 +14,7 @@
 #include <drm/drm_format_helper.h>
 #include <drm/drm_fourcc.h>
 #include <drm/drm_framebuffer.h>
+#include <drm/drm_gem_framebuffer_helper.h>
 #include <drm/drm_gem_shmem_helper.h>
 #include <drm/drm_print.h>
 #include <drm/drm_rect.h>
@@ -168,11 +168,9 @@ static int gud_prep_flush(struct gud_device *gdrm, struct drm_framebuffer *fb,
 
 	vaddr = map.vaddr + fb->offsets[0];
 
-	if (import_attach) {
-		ret = dma_buf_begin_cpu_access(import_attach->dmabuf, DMA_FROM_DEVICE);
-		if (ret)
-			goto vunmap;
-	}
+	ret = drm_gem_fb_begin_cpu_access(fb, DMA_FROM_DEVICE);
+	if (ret)
+		goto vunmap;
 retry:
 	if (compression)
 		buf = gdrm->compress_buf;
@@ -225,8 +223,7 @@ static int gud_prep_flush(struct gud_device *gdrm, struct drm_framebuffer *fb,
 	}
 
 end_cpu_access:
-	if (import_attach)
-		dma_buf_end_cpu_access(import_attach->dmabuf, DMA_FROM_DEVICE);
+	drm_gem_fb_end_cpu_access(fb, DMA_FROM_DEVICE);
 vunmap:
 	drm_gem_shmem_vunmap(fb->obj[0], &map);
 
-- 
2.32.0

[PATCH 5/7] drm/gm12u320: Use framebuffer dma-buf helpers

[Thomas Zimmermann]

Replace dma_buf_begin_cpu_access() with drm_gem_fb_begin_cpu_access();
same for _end_cpu_access(). Remove some boiler-plate code. No functional
changes.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/gpu/drm/tiny/gm12u320.c | 19 +++++--------------
 1 file changed, 5 insertions(+), 14 deletions(-)

diff --git a/drivers/gpu/drm/tiny/gm12u320.c b/drivers/gpu/drm/tiny/gm12u320.c
index a233c86d428b..cf7287fccd72 100644
--- a/drivers/gpu/drm/tiny/gm12u320.c
+++ b/drivers/gpu/drm/tiny/gm12u320.c
@@ -3,7 +3,6 @@
  * Copyright 2019 Hans de Goede <hdegoede@redhat.com>
  */
 
-#include <linux/dma-buf.h>
 #include <linux/module.h>
 #include <linux/usb.h>
 
@@ -268,13 +267,10 @@ static void gm12u320_copy_fb_to_blocks(struct gm12u320_device *gm12u320)
 	y2 = gm12u320->fb_update.rect.y2;
 	vaddr = gm12u320->fb_update.src_map.vaddr; /* TODO: Use mapping abstraction properly */
 
-	if (fb->obj[0]->import_attach) {
-		ret = dma_buf_begin_cpu_access(
-			fb->obj[0]->import_attach->dmabuf, DMA_FROM_DEVICE);
-		if (ret) {
-			GM12U320_ERR("dma_buf_begin_cpu_access err: %d\n", ret);
-			goto put_fb;
-		}
+	ret = drm_gem_fb_begin_cpu_access(fb, DMA_FROM_DEVICE);
+	if (ret) {
+		GM12U320_ERR("drm_gem_fb_begin_cpu_access err: %d\n", ret);
+		goto put_fb;
 	}
 
 	src = vaddr + y1 * fb->pitches[0] + x1 * 4;
@@ -311,12 +307,7 @@ static void gm12u320_copy_fb_to_blocks(struct gm12u320_device *gm12u320)
 		src += fb->pitches[0];
 	}
 
-	if (fb->obj[0]->import_attach) {
-		ret = dma_buf_end_cpu_access(fb->obj[0]->import_attach->dmabuf,
-					     DMA_FROM_DEVICE);
-		if (ret)
-			GM12U320_ERR("dma_buf_end_cpu_access err: %d\n", ret);
-	}
+	drm_gem_fb_end_cpu_access(fb, DMA_FROM_DEVICE);
 put_fb:
 	drm_framebuffer_put(fb);
 	gm12u320->fb_update.fb = NULL;
-- 
2.32.0

[PATCH 6/7] drm/repaper: Use framebuffer dma-buf helpers

[Thomas Zimmermann]

Replace dma_buf_begin_cpu_access() with drm_gem_fb_begin_cpu_access();
same for _end_cpu_access(). Remove some boiler-plate code. No functional
changes.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/gpu/drm/tiny/repaper.c | 18 ++++--------------
 1 file changed, 4 insertions(+), 14 deletions(-)

diff --git a/drivers/gpu/drm/tiny/repaper.c b/drivers/gpu/drm/tiny/repaper.c
index 007d9d59f01c..4d07b21a16e6 100644
--- a/drivers/gpu/drm/tiny/repaper.c
+++ b/drivers/gpu/drm/tiny/repaper.c
@@ -14,7 +14,6 @@
  */
 
 #include <linux/delay.h>
-#include <linux/dma-buf.h>
 #include <linux/gpio/consumer.h>
 #include <linux/module.h>
 #include <linux/property.h>
@@ -532,7 +531,6 @@ static void repaper_gray8_to_mono_reversed(u8 *buf, u32 width, u32 height)
 static int repaper_fb_dirty(struct drm_framebuffer *fb)
 {
 	struct drm_gem_cma_object *cma_obj = drm_fb_cma_get_gem_obj(fb, 0);
-	struct dma_buf_attachment *import_attach = cma_obj->base.import_attach;
 	struct repaper_epd *epd = drm_to_epd(fb->dev);
 	struct drm_rect clip;
 	int idx, ret = 0;
@@ -558,21 +556,13 @@ static int repaper_fb_dirty(struct drm_framebuffer *fb)
 		goto out_exit;
 	}
 
-	if (import_attach) {
-		ret = dma_buf_begin_cpu_access(import_attach->dmabuf,
-					       DMA_FROM_DEVICE);
-		if (ret)
-			goto out_free;
-	}
+	ret = drm_gem_fb_begin_cpu_access(fb, DMA_FROM_DEVICE);
+	if (ret)
+		goto out_free;
 
 	drm_fb_xrgb8888_to_gray8(buf, cma_obj->vaddr, fb, &clip);
 
-	if (import_attach) {
-		ret = dma_buf_end_cpu_access(import_attach->dmabuf,
-					     DMA_FROM_DEVICE);
-		if (ret)
-			goto out_free;
-	}
+	drm_gem_fb_end_cpu_access(fb, DMA_FROM_DEVICE);
 
 	repaper_gray8_to_mono_reversed(buf, fb->width, fb->height);
 
-- 
2.32.0

[PATCH 7/7] drm/st7586: Use framebuffer dma-buf helpers

[Thomas Zimmermann]

Replace dma_buf_begin_cpu_access() with drm_gem_fb_begin_cpu_access();
same for _end_cpu_access(). Remove some boiler-plate code. No functional
changes.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/gpu/drm/tiny/st7586.c | 18 ++++++------------
 1 file changed, 6 insertions(+), 12 deletions(-)

diff --git a/drivers/gpu/drm/tiny/st7586.c b/drivers/gpu/drm/tiny/st7586.c
index 1be55bed609a..ad0faa8723c2 100644
--- a/drivers/gpu/drm/tiny/st7586.c
+++ b/drivers/gpu/drm/tiny/st7586.c
@@ -6,7 +6,6 @@
  */
 
 #include <linux/delay.h>
-#include <linux/dma-buf.h>
 #include <linux/gpio/consumer.h>
 #include <linux/module.h>
 #include <linux/property.h>
@@ -21,6 +20,7 @@
 #include <drm/drm_format_helper.h>
 #include <drm/drm_gem_atomic_helper.h>
 #include <drm/drm_gem_cma_helper.h>
+#include <drm/drm_gem_framebuffer_helper.h>
 #include <drm/drm_managed.h>
 #include <drm/drm_mipi_dbi.h>
 #include <drm/drm_rect.h>
@@ -92,24 +92,18 @@ static int st7586_buf_copy(void *dst, struct drm_framebuffer *fb,
 			   struct drm_rect *clip)
 {
 	struct drm_gem_cma_object *cma_obj = drm_fb_cma_get_gem_obj(fb, 0);
-	struct dma_buf_attachment *import_attach = cma_obj->base.import_attach;
 	void *src = cma_obj->vaddr;
 	int ret = 0;
 
-	if (import_attach) {
-		ret = dma_buf_begin_cpu_access(import_attach->dmabuf,
-					       DMA_FROM_DEVICE);
-		if (ret)
-			return ret;
-	}
+	ret = drm_gem_fb_begin_cpu_access(fb, DMA_FROM_DEVICE);
+	if (ret)
+		return ret;
 
 	st7586_xrgb8888_to_gray332(dst, src, fb, clip);
 
-	if (import_attach)
-		ret = dma_buf_end_cpu_access(import_attach->dmabuf,
-					     DMA_FROM_DEVICE);
+	drm_gem_fb_end_cpu_access(fb, DMA_FROM_DEVICE);
 
-	return ret;
+	return 0;
 }
 
 static void st7586_fb_dirty(struct drm_framebuffer *fb, struct drm_rect *rect)
-- 
2.32.0

Re: [PATCH 7/7] drm/st7586: Use framebuffer dma-buf helpers

[David Lechner]

On 7/16/21 9:08 AM, Thomas Zimmermann wrote:
> Replace dma_buf_begin_cpu_access() with drm_gem_fb_begin_cpu_access();
> same for _end_cpu_access(). Remove some boiler-plate code. No functional
> changes.
> 
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> ---

Acked-by: David Lechner <david@lechnology.com>

Re: [PATCH 3/7] drm/mipi-dbi: Use framebuffer dma-buf helpers

[Daniel Vetter]

On Fri, Jul 16, 2021 at 04:07:57PM +0200, Thomas Zimmermann wrote:
> Replace dma_buf_begin_cpu_access() with drm_gem_fb_begin_cpu_access();
> same for _end_cpu_access(). Remove some boiler-plate code. No functional
> changes.
> 
> There's one left-over reference to the imported attachment that we
> keep. GEM BOs with imported attachment are considered uncached and
> enables special handling within the drm_fb_swab().
> 
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> ---
>  drivers/gpu/drm/drm_mipi_dbi.c | 20 +++++++-------------
>  1 file changed, 7 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_mipi_dbi.c b/drivers/gpu/drm/drm_mipi_dbi.c
> index 10b4e59384ae..71b646c4131f 100644
> --- a/drivers/gpu/drm/drm_mipi_dbi.c
> +++ b/drivers/gpu/drm/drm_mipi_dbi.c
> @@ -7,7 +7,6 @@
>  
>  #include <linux/debugfs.h>
>  #include <linux/delay.h>
> -#include <linux/dma-buf.h>
>  #include <linux/gpio/consumer.h>
>  #include <linux/module.h>
>  #include <linux/regulator/consumer.h>
> @@ -202,21 +201,17 @@ int mipi_dbi_buf_copy(void *dst, struct drm_framebuffer *fb,
>  {
>  	struct drm_gem_object *gem = drm_gem_fb_get_obj(fb, 0);
>  	struct drm_gem_cma_object *cma_obj = to_drm_gem_cma_obj(gem);
> -	struct dma_buf_attachment *import_attach = gem->import_attach;
>  	void *src = cma_obj->vaddr;
> -	int ret = 0;
> +	int ret;
>  
> -	if (import_attach) {
> -		ret = dma_buf_begin_cpu_access(import_attach->dmabuf,
> -					       DMA_FROM_DEVICE);
> -		if (ret)
> -			return ret;
> -	}
> +	ret = drm_gem_fb_begin_cpu_access(fb, DMA_FROM_DEVICE);
> +	if (ret)
> +		return ret;
>  
>  	switch (fb->format->format) {
>  	case DRM_FORMAT_RGB565:
>  		if (swap)
> -			drm_fb_swab(dst, src, fb, clip, !import_attach);
> +			drm_fb_swab(dst, src, fb, clip, !gem->import_attach);

I freaked out about this because for dma-buf WC vs WB is undefined, but
it's purely a perf optimization. So it's fine. Plus we're not even
bothering with the iomem vs normal memory distinction here.

Anyway, that aside, all looks good. On the series:

Acked-by: Daniel Vetter <daniel.vetter@ffwll.ch>

>  		else
>  			drm_fb_memcpy(dst, src, fb, clip);
>  		break;
> @@ -229,9 +224,8 @@ int mipi_dbi_buf_copy(void *dst, struct drm_framebuffer *fb,
>  		return -EINVAL;
>  	}
>  
> -	if (import_attach)
> -		ret = dma_buf_end_cpu_access(import_attach->dmabuf,
> -					     DMA_FROM_DEVICE);
> +	drm_gem_fb_end_cpu_access(fb, DMA_FROM_DEVICE);
> +
>  	return ret;
>  }
>  EXPORT_SYMBOL(mipi_dbi_buf_copy);
> -- 
> 2.32.0
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [PATCH 1/7] drm/gem: Provide drm_gem_fb_{begin, end}_cpu_access() helpers

[Daniel Vetter]

On Fri, Jul 16, 2021 at 04:07:55PM +0200, Thomas Zimmermann wrote:
> Implement helpers drm_gem_fb_begin_cpu_access() and _end_cpu_access(),
> which call the rsp dma-buf functions for all GEM BOs of the given
> framebuffer.
> 
> Calls to dma_buf_end_cpu_access() can return an error code on failure,
> while drm_gem_fb_end_cpu_access() does not. The latter runs during DRM's
> atomic commit or during cleanup. Both cases don't allow for errors, so
> leave out the return value.
> 
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> ---
>  drivers/gpu/drm/drm_gem_framebuffer_helper.c | 89 ++++++++++++++++++++
>  include/drm/drm_gem_framebuffer_helper.h     |  6 ++
>  2 files changed, 95 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_gem_framebuffer_helper.c b/drivers/gpu/drm/drm_gem_framebuffer_helper.c
> index e2c68822e05c..94a1c0b0edfd 100644
> --- a/drivers/gpu/drm/drm_gem_framebuffer_helper.c
> +++ b/drivers/gpu/drm/drm_gem_framebuffer_helper.c
> @@ -306,6 +306,95 @@ drm_gem_fb_create_with_dirty(struct drm_device *dev, struct drm_file *file,
>  }
>  EXPORT_SYMBOL_GPL(drm_gem_fb_create_with_dirty);
>  
> +/**
> + * drm_gem_fb_begin_cpu_access - prepares GEM buffer objects for CPU access
> + * @fb: the framebuffer
> + * @dir: access mode
> + *
> + * Prepares a framebuffer'S GEM buffer objects for CPU access. This function

			s/S/s/

> + * must be called before accessing the BO data within the kernel. For imported
> + * BOs, the function calls dma_buf_begin_cpu_access().
> + *
> + * See drm_gem_fb_end_cpu_access() for signalling the end of CPU access.
> + *
> + * Returns:
> + * 0 on success, or a negative errno code otherwise.
> + */
> +int drm_gem_fb_begin_cpu_access(struct drm_framebuffer *fb, enum dma_data_direction dir)
> +{
> +	struct dma_buf_attachment *import_attach;
> +	struct drm_gem_object *obj;
> +	size_t i;
> +	int ret, ret2;
> +
> +	for (i = 0; i < ARRAY_SIZE(fb->obj); ++i) {
> +		obj = drm_gem_fb_get_obj(fb, i);
> +		if (!obj)
> +			continue;
> +		import_attach = obj->import_attach;
> +		if (!import_attach)
> +			continue;
> +		ret = dma_buf_begin_cpu_access(import_attach->dmabuf, dir);
> +		if (ret)
> +			goto err_dma_buf_end_cpu_access;
> +	}
> +
> +	return 0;
> +
> +err_dma_buf_end_cpu_access:
> +	while (i) {
> +		--i;
> +		obj = drm_gem_fb_get_obj(fb, i);
> +		if (!obj)
> +			continue;
> +		import_attach = obj->import_attach;
> +		if (!import_attach)
> +			continue;
> +		ret2 = dma_buf_end_cpu_access(import_attach->dmabuf, dir);
> +		if (ret2) {
> +			drm_err(fb->dev,
> +				"dma_buf_end_cpu_access() failed during error handling: %d\n",
> +				ret2);
> +		}
> +	}
> +
> +	return ret;
> +}
> +EXPORT_SYMBOL(drm_gem_fb_begin_cpu_access);
> +
> +/**
> + * drm_gem_fb_end_cpu_access - signals end of CPU access to GEM buffer objects
> + * @fb: the framebuffer
> + * @dir: access mode
> + *
> + * Signals the end of CPU access to the given framebuffer'S GEM buffer objects. This

							s/S/s/

> + * function must be paired with a corresponding call to drm_gem_fb_begin_cpu_access().
> + * For imported BOs, the function calls dma_buf_end_cpu_access().
> + *
> + * See also drm_gem_fb_begin_cpu_access().
> + */
> +void drm_gem_fb_end_cpu_access(struct drm_framebuffer *fb, enum dma_data_direction dir)
> +{
> +	size_t i = ARRAY_SIZE(fb->obj);
> +	struct dma_buf_attachment *import_attach;
> +	struct drm_gem_object *obj;
> +	int ret;
> +
> +	while (i) {
> +		--i;
> +		obj = drm_gem_fb_get_obj(fb, i);
> +		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);
> +	}
> +}
> +EXPORT_SYMBOL(drm_gem_fb_end_cpu_access);
> +
>  static __u32 drm_gem_afbc_get_bpp(struct drm_device *dev,
>  				  const struct drm_mode_fb_cmd2 *mode_cmd)
>  {
> diff --git a/include/drm/drm_gem_framebuffer_helper.h b/include/drm/drm_gem_framebuffer_helper.h
> index 6bdffc7aa124..5705722f0855 100644
> --- a/include/drm/drm_gem_framebuffer_helper.h
> +++ b/include/drm/drm_gem_framebuffer_helper.h
> @@ -1,6 +1,9 @@
>  #ifndef __DRM_GEM_FB_HELPER_H__
>  #define __DRM_GEM_FB_HELPER_H__
>  
> +#include <linux/dma-buf.h>
> +#include <linux/dma-buf-map.h>
> +
>  struct drm_afbc_framebuffer;
>  struct drm_device;
>  struct drm_fb_helper_surface_size;
> @@ -34,6 +37,9 @@ struct drm_framebuffer *
>  drm_gem_fb_create_with_dirty(struct drm_device *dev, struct drm_file *file,
>  			     const struct drm_mode_fb_cmd2 *mode_cmd);
>  
> +int drm_gem_fb_begin_cpu_access(struct drm_framebuffer *fb, enum dma_data_direction dir);
> +void drm_gem_fb_end_cpu_access(struct drm_framebuffer *fb, enum dma_data_direction dir);
> +
>  #define drm_is_afbc(modifier) \
>  	(((modifier) & AFBC_VENDOR_AND_TYPE_MASK) == DRM_FORMAT_MOD_ARM_AFBC(0))

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

>  
> -- 
> 2.32.0
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [PATCH 3/7] drm/mipi-dbi: Use framebuffer dma-buf helpers

[Thomas Zimmermann]

[-- Attachment #1.1: Type: text/plain, Size: 3191 bytes --]

Hi

Am 20.07.21 um 16:03 schrieb Daniel Vetter:
> On Fri, Jul 16, 2021 at 04:07:57PM +0200, Thomas Zimmermann wrote:
>> Replace dma_buf_begin_cpu_access() with drm_gem_fb_begin_cpu_access();
>> same for _end_cpu_access(). Remove some boiler-plate code. No functional
>> changes.
>>
>> There's one left-over reference to the imported attachment that we
>> keep. GEM BOs with imported attachment are considered uncached and
>> enables special handling within the drm_fb_swab().
>>
>> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
>> ---
>>   drivers/gpu/drm/drm_mipi_dbi.c | 20 +++++++-------------
>>   1 file changed, 7 insertions(+), 13 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/drm_mipi_dbi.c b/drivers/gpu/drm/drm_mipi_dbi.c
>> index 10b4e59384ae..71b646c4131f 100644
>> --- a/drivers/gpu/drm/drm_mipi_dbi.c
>> +++ b/drivers/gpu/drm/drm_mipi_dbi.c
>> @@ -7,7 +7,6 @@
>>   
>>   #include <linux/debugfs.h>
>>   #include <linux/delay.h>
>> -#include <linux/dma-buf.h>
>>   #include <linux/gpio/consumer.h>
>>   #include <linux/module.h>
>>   #include <linux/regulator/consumer.h>
>> @@ -202,21 +201,17 @@ int mipi_dbi_buf_copy(void *dst, struct drm_framebuffer *fb,
>>   {
>>   	struct drm_gem_object *gem = drm_gem_fb_get_obj(fb, 0);
>>   	struct drm_gem_cma_object *cma_obj = to_drm_gem_cma_obj(gem);
>> -	struct dma_buf_attachment *import_attach = gem->import_attach;
>>   	void *src = cma_obj->vaddr;
>> -	int ret = 0;
>> +	int ret;
>>   
>> -	if (import_attach) {
>> -		ret = dma_buf_begin_cpu_access(import_attach->dmabuf,
>> -					       DMA_FROM_DEVICE);
>> -		if (ret)
>> -			return ret;
>> -	}
>> +	ret = drm_gem_fb_begin_cpu_access(fb, DMA_FROM_DEVICE);
>> +	if (ret)
>> +		return ret;
>>   
>>   	switch (fb->format->format) {
>>   	case DRM_FORMAT_RGB565:
>>   		if (swap)
>> -			drm_fb_swab(dst, src, fb, clip, !import_attach);
>> +			drm_fb_swab(dst, src, fb, clip, !gem->import_attach);
> 
> I freaked out about this because for dma-buf WC vs WB is undefined, but
> it's purely a perf optimization. So it's fine. Plus we're not even
> bothering with the iomem vs normal memory distinction here.

Indeed. Because of the line above, I made a proto-patchset to store 
caching flags within struct dma_buf_map. But it's a lot of churn, so I 
kept the current heuristic for now.

> 
> Anyway, that aside, all looks good. On the series:
> 
> Acked-by: Daniel Vetter <daniel.vetter@ffwll.ch>

Thanks.

Best regards
Thomas

> 
>>   		else
>>   			drm_fb_memcpy(dst, src, fb, clip);
>>   		break;
>> @@ -229,9 +224,8 @@ int mipi_dbi_buf_copy(void *dst, struct drm_framebuffer *fb,
>>   		return -EINVAL;
>>   	}
>>   
>> -	if (import_attach)
>> -		ret = dma_buf_end_cpu_access(import_attach->dmabuf,
>> -					     DMA_FROM_DEVICE);
>> +	drm_gem_fb_end_cpu_access(fb, DMA_FROM_DEVICE);
>> +
>>   	return ret;
>>   }
>>   EXPORT_SYMBOL(mipi_dbi_buf_copy);
>> -- 
>> 2.32.0
>>
> 

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Felix Imendörffer


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]

Re: [PATCH 0/7] drm: Provide framebuffer dma-buf helpers

[Noralf Trønnes]

Den 16.07.2021 16.07, skrev Thomas Zimmermann:
> Provide helpers that wrap dma_buf_{begin,end}_cpu_access() for all
> GEM BOs attached to a framebuffer. Convert drivers and remove ugly
> boilerplate code.
> 

Nice, for the series:

Reviewed-by: Noralf Trønnes <noralf@tronnes.org>


> Thomas Zimmermann (7):
>   drm/gem: Provide drm_gem_fb_{begin,end}_cpu_access() helpers
>   drm/udl: Use framebuffer dma-buf helpers
>   drm/mipi-dbi: Use framebuffer dma-buf helpers
>   drm/gud: Use framebuffer dma-buf helpers
>   drm/gm12u320: Use framebuffer dma-buf helpers
>   drm/repaper: Use framebuffer dma-buf helpers
>   drm/st7586: Use framebuffer dma-buf helpers
> 
>  drivers/gpu/drm/drm_gem_framebuffer_helper.c | 89 ++++++++++++++++++++
>  drivers/gpu/drm/drm_mipi_dbi.c               | 20 ++---
>  drivers/gpu/drm/gud/gud_pipe.c               | 13 ++-
>  drivers/gpu/drm/tiny/gm12u320.c              | 19 ++---
>  drivers/gpu/drm/tiny/repaper.c               | 18 +---
>  drivers/gpu/drm/tiny/st7586.c                | 18 ++--
>  drivers/gpu/drm/udl/udl_modeset.c            | 29 ++-----
>  include/drm/drm_gem_framebuffer_helper.h     |  6 ++
>  8 files changed, 130 insertions(+), 82 deletions(-)
> 
> --
> 2.32.0
>