[Intel-gfx] i915 dmabuf cleanup

[Intel-gfx] i915 dmabuf cleanup

[Michael J. Ruhl]

While studying this code I noticed a few inconsistencies and possible
cleanups.

Please consider these patches.

[Intel-gfx] [PATCH 1/2] drm/i915/dmabuf: dmabuf cleanup

[Michael J. Ruhl]

Some minor cleanup of some variables for consistency.

Normalize struct sg_table to sgt.
Normalize struct dma_buf_attachment to attach.
checkpatch issues sizeof(), !NULL updates.

Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Signed-off-by: Michael J. Ruhl <michael.j.ruhl@intel.com>
---
 drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c | 47 ++++++++++++----------
 1 file changed, 25 insertions(+), 22 deletions(-)

diff --git a/drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c b/drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c
index f5062d0c6333..5f327eac26e6 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c
@@ -25,43 +25,46 @@ static struct drm_i915_gem_object *dma_buf_to_obj(struct dma_buf *buf)
 	return to_intel_bo(buf->priv);
 }
 
-static struct sg_table *i915_gem_map_dma_buf(struct dma_buf_attachment *attachment,
+static struct sg_table *i915_gem_map_dma_buf(struct dma_buf_attachment *attach,
 					     enum dma_data_direction dir)
 {
-	struct drm_i915_gem_object *obj = dma_buf_to_obj(attachment->dmabuf);
-	struct sg_table *st;
+	struct drm_i915_gem_object *obj = dma_buf_to_obj(attach->dmabuf);
+	struct sg_table *sgt;
 	struct scatterlist *src, *dst;
 	int ret, i;
 
-	/* Copy sg so that we make an independent mapping */
-	st = kmalloc(sizeof(struct sg_table), GFP_KERNEL);
-	if (st == NULL) {
+	/*
+	 * Make a copy of the object's sgt, so that we can make an independent
+	 * mapping
+	 */
+	sgt = kmalloc(sizeof(*sgt), GFP_KERNEL);
+	if (!sgt) {
 		ret = -ENOMEM;
 		goto err;
 	}
 
-	ret = sg_alloc_table(st, obj->mm.pages->nents, GFP_KERNEL);
+	ret = sg_alloc_table(sgt, obj->mm.pages->nents, GFP_KERNEL);
 	if (ret)
 		goto err_free;
 
 	src = obj->mm.pages->sgl;
-	dst = st->sgl;
+	dst = sgt->sgl;
 	for (i = 0; i < obj->mm.pages->nents; i++) {
 		sg_set_page(dst, sg_page(src), src->length, 0);
 		dst = sg_next(dst);
 		src = sg_next(src);
 	}
 
-	ret = dma_map_sgtable(attachment->dev, st, dir, DMA_ATTR_SKIP_CPU_SYNC);
+	ret = dma_map_sgtable(attach->dev, sgt, dir, DMA_ATTR_SKIP_CPU_SYNC);
 	if (ret)
 		goto err_free_sg;
 
-	return st;
+	return sgt;
 
 err_free_sg:
-	sg_free_table(st);
+	sg_free_table(sgt);
 err_free:
-	kfree(st);
+	kfree(sgt);
 err:
 	return ERR_PTR(ret);
 }
@@ -236,15 +239,15 @@ struct dma_buf *i915_gem_prime_export(struct drm_gem_object *gem_obj, int flags)
 static int i915_gem_object_get_pages_dmabuf(struct drm_i915_gem_object *obj)
 {
 	struct drm_i915_private *i915 = to_i915(obj->base.dev);
-	struct sg_table *pages;
+	struct sg_table *sgt;
 	unsigned int sg_page_sizes;
 
 	assert_object_held(obj);
 
-	pages = dma_buf_map_attachment(obj->base.import_attach,
-				       DMA_BIDIRECTIONAL);
-	if (IS_ERR(pages))
-		return PTR_ERR(pages);
+	sgt = dma_buf_map_attachment(obj->base.import_attach,
+				     DMA_BIDIRECTIONAL);
+	if (IS_ERR(sgt))
+		return PTR_ERR(sgt);
 
 	/*
 	 * DG1 is special here since it still snoops transactions even with
@@ -261,16 +264,16 @@ static int i915_gem_object_get_pages_dmabuf(struct drm_i915_gem_object *obj)
 	    (!HAS_LLC(i915) && !IS_DG1(i915)))
 		wbinvd_on_all_cpus();
 
-	sg_page_sizes = i915_sg_dma_sizes(pages->sgl);
-	__i915_gem_object_set_pages(obj, pages, sg_page_sizes);
+	sg_page_sizes = i915_sg_dma_sizes(sgt->sgl);
+	__i915_gem_object_set_pages(obj, sgt, sg_page_sizes);
 
 	return 0;
 }
 
 static void i915_gem_object_put_pages_dmabuf(struct drm_i915_gem_object *obj,
-					     struct sg_table *pages)
+					     struct sg_table *sgt)
 {
-	dma_buf_unmap_attachment(obj->base.import_attach, pages,
+	dma_buf_unmap_attachment(obj->base.import_attach, sgt,
 				 DMA_BIDIRECTIONAL);
 }
 
@@ -313,7 +316,7 @@ struct drm_gem_object *i915_gem_prime_import(struct drm_device *dev,
 	get_dma_buf(dma_buf);
 
 	obj = i915_gem_object_alloc();
-	if (obj == NULL) {
+	if (!obj) {
 		ret = -ENOMEM;
 		goto fail_detach;
 	}
-- 
2.31.1

[Intel-gfx] [PATCH 2/2] drm/i915/dmabuf: Use scatterlist for_each_sg API

[Michael J. Ruhl]

Update open coded for loop to use the standard scatterlist
for_each_sg API.

Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Signed-off-by: Michael J. Ruhl <michael.j.ruhl@intel.com>
---
 drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c b/drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c
index 5f327eac26e6..3006e60e2d51 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c
@@ -47,12 +47,10 @@ static struct sg_table *i915_gem_map_dma_buf(struct dma_buf_attachment *attach,
 	if (ret)
 		goto err_free;
 
-	src = obj->mm.pages->sgl;
 	dst = sgt->sgl;
-	for (i = 0; i < obj->mm.pages->nents; i++) {
+	for_each_sg(obj->mm.pages->sgl, src, obj->mm.pages->nents, i) {
 		sg_set_page(dst, sg_page(src), src->length, 0);
 		dst = sg_next(dst);
-		src = sg_next(src);
 	}
 
 	ret = dma_map_sgtable(attach->dev, sgt, dir, DMA_ATTR_SKIP_CPU_SYNC);
-- 
2.31.1

[Intel-gfx] ✓ Fi.CI.BAT: success for series starting with [1/2] drm/i915/dmabuf: dmabuf cleanup

[Patchwork]

[-- Attachment #1: Type: text/plain, Size: 3824 bytes --]

== Series Details ==

Series: series starting with [1/2] drm/i915/dmabuf: dmabuf cleanup
URL   : https://patchwork.freedesktop.org/series/103696/
State : success

== Summary ==

CI Bug Log - changes from CI_DRM_11618 -> Patchwork_103696v1
====================================================

Summary
-------

  **SUCCESS**

  No regressions found.

  External URL: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_103696v1/index.html

Participating hosts (35 -> 44)
------------------------------

  Additional (11): bat-adls-5 bat-dg1-6 bat-dg2-8 fi-icl-u2 bat-dg2-9 bat-adlp-6 bat-adln-1 bat-rpls-1 bat-rpls-2 bat-jsl-2 bat-jsl-1 
  Missing    (2): fi-bsw-cyan fi-bdw-5557u 

Known issues
------------

  Here are the changes found in Patchwork_103696v1 that come from known issues:

### IGT changes ###

#### Issues hit ####

  * igt@runner@aborted:
    - fi-icl-u2:          NOTRUN -> [FAIL][1] ([i915#3690])
   [1]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_103696v1/fi-icl-u2/igt@runner@aborted.html
    - bat-dg1-6:          NOTRUN -> [FAIL][2] ([i915#5616])
   [2]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_103696v1/bat-dg1-6/igt@runner@aborted.html

  
  {name}: This element is suppressed. This means it is ignored when computing
          the status of the difference (SUCCESS, WARNING, or FAILURE).

  [fdo#109285]: https://bugs.freedesktop.org/show_bug.cgi?id=109285
  [fdo#111827]: https://bugs.freedesktop.org/show_bug.cgi?id=111827
  [i915#1072]: https://gitlab.freedesktop.org/drm/intel/issues/1072
  [i915#1155]: https://gitlab.freedesktop.org/drm/intel/issues/1155
  [i915#2582]: https://gitlab.freedesktop.org/drm/intel/issues/2582
  [i915#3291]: https://gitlab.freedesktop.org/drm/intel/issues/3291
  [i915#3555]: https://gitlab.freedesktop.org/drm/intel/issues/3555
  [i915#3595]: https://gitlab.freedesktop.org/drm/intel/issues/3595
  [i915#3690]: https://gitlab.freedesktop.org/drm/intel/issues/3690
  [i915#3708]: https://gitlab.freedesktop.org/drm/intel/issues/3708
  [i915#4077]: https://gitlab.freedesktop.org/drm/intel/issues/4077
  [i915#4079]: https://gitlab.freedesktop.org/drm/intel/issues/4079
  [i915#4083]: https://gitlab.freedesktop.org/drm/intel/issues/4083
  [i915#4103]: https://gitlab.freedesktop.org/drm/intel/issues/4103
  [i915#4212]: https://gitlab.freedesktop.org/drm/intel/issues/4212
  [i915#4213]: https://gitlab.freedesktop.org/drm/intel/issues/4213
  [i915#4215]: https://gitlab.freedesktop.org/drm/intel/issues/4215
  [i915#4873]: https://gitlab.freedesktop.org/drm/intel/issues/4873
  [i915#4897]: https://gitlab.freedesktop.org/drm/intel/issues/4897
  [i915#5190]: https://gitlab.freedesktop.org/drm/intel/issues/5190
  [i915#5274]: https://gitlab.freedesktop.org/drm/intel/issues/5274
  [i915#5275]: https://gitlab.freedesktop.org/drm/intel/issues/5275
  [i915#5354]: https://gitlab.freedesktop.org/drm/intel/issues/5354
  [i915#5602]: https://gitlab.freedesktop.org/drm/intel/issues/5602
  [i915#5616]: https://gitlab.freedesktop.org/drm/intel/issues/5616
  [i915#5763]: https://gitlab.freedesktop.org/drm/intel/issues/5763
  [i915#5917]: https://gitlab.freedesktop.org/drm/intel/issues/5917


Build changes
-------------

  * Linux: CI_DRM_11618 -> Patchwork_103696v1

  CI-20190529: 20190529
  CI_DRM_11618: 07c7d578e3b8a85c79e9a7f8dace075192d5fd91 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_6468: cffa5fffe9acddf49565b4caeeb5e3355ff2ea44 @ https://gitlab.freedesktop.org/drm/igt-gpu-tools.git
  Patchwork_103696v1: 07c7d578e3b8a85c79e9a7f8dace075192d5fd91 @ git://anongit.freedesktop.org/gfx-ci/linux


### Linux commits

faf34fa0cba3 drm/i915/dmabuf: Use scatterlist for_each_sg API
1383a2c06fc0 drm/i915/dmabuf: dmabuf cleanup

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_103696v1/index.html

[-- Attachment #2: Type: text/html, Size: 2771 bytes --]

[Intel-gfx] ✓ Fi.CI.IGT: success for series starting with [1/2] drm/i915/dmabuf: dmabuf cleanup

[Patchwork]

[-- Attachment #1: Type: text/plain, Size: 1807 bytes --]

== Series Details ==

Series: series starting with [1/2] drm/i915/dmabuf: dmabuf cleanup
URL   : https://patchwork.freedesktop.org/series/103696/
State : success

== Summary ==

CI Bug Log - changes from CI_DRM_11618_full -> Patchwork_103696v1_full
====================================================

Summary
-------

  **SUCCESS**

  No regressions found.

  External URL: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_103696v1/index.html

Participating hosts (10 -> 7)
------------------------------

  Missing    (3): pig-skl-6260u pig-kbl-iris pig-glk-j5005 

Known issues
------------

  Here are the changes found in Patchwork_103696v1_full that come from known issues:

### IGT changes ###

#### Possible fixes ####

  * igt@gem_exec_flush@basic-wb-pro-default:
    - shard-snb:          [SKIP][1] ([fdo#109271]) -> [PASS][2]
   [1]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11618/shard-snb6/igt@gem_exec_flush@basic-wb-pro-default.html
   [2]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_103696v1/shard-snb7/igt@gem_exec_flush@basic-wb-pro-default.html

  
  [fdo#109271]: https://bugs.freedesktop.org/show_bug.cgi?id=109271


Build changes
-------------

  * Linux: CI_DRM_11618 -> Patchwork_103696v1
  * Piglit: piglit_4509 -> None

  CI-20190529: 20190529
  CI_DRM_11618: 07c7d578e3b8a85c79e9a7f8dace075192d5fd91 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_6468: cffa5fffe9acddf49565b4caeeb5e3355ff2ea44 @ https://gitlab.freedesktop.org/drm/igt-gpu-tools.git
  Patchwork_103696v1: 07c7d578e3b8a85c79e9a7f8dace075192d5fd91 @ git://anongit.freedesktop.org/gfx-ci/linux
  piglit_4509: fdc5a4ca11124ab8413c7988896eec4c97336694 @ git://anongit.freedesktop.org/piglit

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_103696v1/index.html

[-- Attachment #2: Type: text/html, Size: 2406 bytes --]

Re: [Intel-gfx] [PATCH 1/2] drm/i915/dmabuf: dmabuf cleanup

[Ruhl, Michael J]

ping?

Any comments on this cleanup and the open coding fix (patch 2?)

Thanks,

Mike


>-----Original Message-----
>From: Ruhl, Michael J <michael.j.ruhl@intel.com>
>Sent: Friday, May 6, 2022 3:09 PM
>To: intel-gfx@lists.freedesktop.org; airlied@linux.ie; daniel@ffwll.ch
>Cc: tvrtko.ursulin@linux.intel.com; De Marchi, Lucas
><lucas.demarchi@intel.com>; Ruhl, Michael J <michael.j.ruhl@intel.com>;
>Ursulin, Tvrtko <tvrtko.ursulin@intel.com>
>Subject: [PATCH 1/2] drm/i915/dmabuf: dmabuf cleanup
>
>Some minor cleanup of some variables for consistency.
>
>Normalize struct sg_table to sgt.
>Normalize struct dma_buf_attachment to attach.
>checkpatch issues sizeof(), !NULL updates.
>
>Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>Signed-off-by: Michael J. Ruhl <michael.j.ruhl@intel.com>
>---
> drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c | 47 ++++++++++++----------
> 1 file changed, 25 insertions(+), 22 deletions(-)
>
>diff --git a/drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c
>b/drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c
>index f5062d0c6333..5f327eac26e6 100644
>--- a/drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c
>+++ b/drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c
>@@ -25,43 +25,46 @@ static struct drm_i915_gem_object
>*dma_buf_to_obj(struct dma_buf *buf)
> 	return to_intel_bo(buf->priv);
> }
>
>-static struct sg_table *i915_gem_map_dma_buf(struct
>dma_buf_attachment *attachment,
>+static struct sg_table *i915_gem_map_dma_buf(struct
>dma_buf_attachment *attach,
> 					     enum dma_data_direction dir)
> {
>-	struct drm_i915_gem_object *obj = dma_buf_to_obj(attachment-
>>dmabuf);
>-	struct sg_table *st;
>+	struct drm_i915_gem_object *obj = dma_buf_to_obj(attach-
>>dmabuf);
>+	struct sg_table *sgt;
> 	struct scatterlist *src, *dst;
> 	int ret, i;
>
>-	/* Copy sg so that we make an independent mapping */
>-	st = kmalloc(sizeof(struct sg_table), GFP_KERNEL);
>-	if (st == NULL) {
>+	/*
>+	 * Make a copy of the object's sgt, so that we can make an
>independent
>+	 * mapping
>+	 */
>+	sgt = kmalloc(sizeof(*sgt), GFP_KERNEL);
>+	if (!sgt) {
> 		ret = -ENOMEM;
> 		goto err;
> 	}
>
>-	ret = sg_alloc_table(st, obj->mm.pages->nents, GFP_KERNEL);
>+	ret = sg_alloc_table(sgt, obj->mm.pages->nents, GFP_KERNEL);
> 	if (ret)
> 		goto err_free;
>
> 	src = obj->mm.pages->sgl;
>-	dst = st->sgl;
>+	dst = sgt->sgl;
> 	for (i = 0; i < obj->mm.pages->nents; i++) {
> 		sg_set_page(dst, sg_page(src), src->length, 0);
> 		dst = sg_next(dst);
> 		src = sg_next(src);
> 	}
>
>-	ret = dma_map_sgtable(attachment->dev, st, dir,
>DMA_ATTR_SKIP_CPU_SYNC);
>+	ret = dma_map_sgtable(attach->dev, sgt, dir,
>DMA_ATTR_SKIP_CPU_SYNC);
> 	if (ret)
> 		goto err_free_sg;
>
>-	return st;
>+	return sgt;
>
> err_free_sg:
>-	sg_free_table(st);
>+	sg_free_table(sgt);
> err_free:
>-	kfree(st);
>+	kfree(sgt);
> err:
> 	return ERR_PTR(ret);
> }
>@@ -236,15 +239,15 @@ struct dma_buf *i915_gem_prime_export(struct
>drm_gem_object *gem_obj, int flags)
> static int i915_gem_object_get_pages_dmabuf(struct drm_i915_gem_object
>*obj)
> {
> 	struct drm_i915_private *i915 = to_i915(obj->base.dev);
>-	struct sg_table *pages;
>+	struct sg_table *sgt;
> 	unsigned int sg_page_sizes;
>
> 	assert_object_held(obj);
>
>-	pages = dma_buf_map_attachment(obj->base.import_attach,
>-				       DMA_BIDIRECTIONAL);
>-	if (IS_ERR(pages))
>-		return PTR_ERR(pages);
>+	sgt = dma_buf_map_attachment(obj->base.import_attach,
>+				     DMA_BIDIRECTIONAL);
>+	if (IS_ERR(sgt))
>+		return PTR_ERR(sgt);
>
> 	/*
> 	 * DG1 is special here since it still snoops transactions even with
>@@ -261,16 +264,16 @@ static int
>i915_gem_object_get_pages_dmabuf(struct drm_i915_gem_object *obj)
> 	    (!HAS_LLC(i915) && !IS_DG1(i915)))
> 		wbinvd_on_all_cpus();
>
>-	sg_page_sizes = i915_sg_dma_sizes(pages->sgl);
>-	__i915_gem_object_set_pages(obj, pages, sg_page_sizes);
>+	sg_page_sizes = i915_sg_dma_sizes(sgt->sgl);
>+	__i915_gem_object_set_pages(obj, sgt, sg_page_sizes);
>
> 	return 0;
> }
>
> static void i915_gem_object_put_pages_dmabuf(struct
>drm_i915_gem_object *obj,
>-					     struct sg_table *pages)
>+					     struct sg_table *sgt)
> {
>-	dma_buf_unmap_attachment(obj->base.import_attach, pages,
>+	dma_buf_unmap_attachment(obj->base.import_attach, sgt,
> 				 DMA_BIDIRECTIONAL);
> }
>
>@@ -313,7 +316,7 @@ struct drm_gem_object
>*i915_gem_prime_import(struct drm_device *dev,
> 	get_dma_buf(dma_buf);
>
> 	obj = i915_gem_object_alloc();
>-	if (obj == NULL) {
>+	if (!obj) {
> 		ret = -ENOMEM;
> 		goto fail_detach;
> 	}
>--
>2.31.1