[PATCH] drm/amdgpu: Transfer fences to dmabuf importer

[PATCH] drm/amdgpu: Transfer fences to dmabuf importer

[Chris Wilson]

amdgpu only uses shared-fences internally, but dmabuf importers rely on
implicit write hazard tracking via the reservation_object.fence_excl.
For example, the importer use the write hazard for timing a page flip to
only occur after the exporter has finished flushing its write into the
surface. As such, on exporting a dmabuf, we must either flush all
outstanding fences (for we do not know which are writes and should have
been exclusive) or alternatively create a new exclusive fence that is
the composite of all the existing shared fences, and so will only be
signaled when all earlier fences are signaled (ensuring that we can not
be signaled before the completion of any earlier write).

Testcase: igt/amd_prime/amd-to-i915
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Alex Deucher <alexander.deucher@amd.com>
Cc: "Christian König" <christian.koenig@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_prime.c | 70 ++++++++++++++++++++---
 1 file changed, 62 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_prime.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_prime.c
index 1c5d97f4b4dd..47e6ec5510b6 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_prime.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_prime.c
@@ -37,6 +37,7 @@
 #include "amdgpu_display.h"
 #include <drm/amdgpu_drm.h>
 #include <linux/dma-buf.h>
+#include <linux/dma-fence-array.h>
 
 static const struct dma_buf_ops amdgpu_dmabuf_ops;
 
@@ -188,6 +189,57 @@ amdgpu_gem_prime_import_sg_table(struct drm_device *dev,
 	return ERR_PTR(ret);
 }
 
+static int
+__reservation_object_make_exclusive(struct reservation_object *obj)
+{
+	struct reservation_object_list *fobj;
+	struct dma_fence_array *array;
+	struct dma_fence **fences;
+	unsigned int count, i;
+
+	fobj = reservation_object_get_list(obj);
+	if (!fobj)
+		return 0;
+
+	count = !!rcu_access_pointer(obj->fence_excl);
+	count += fobj->shared_count;
+
+	fences = kmalloc_array(sizeof(*fences), count, GFP_KERNEL);
+	if (!fences)
+		return -ENOMEM;
+
+	for (i = 0; i < fobj->shared_count; i++) {
+		struct dma_fence *f =
+			rcu_dereference_protected(fobj->shared[i],
+						  reservation_object_held(obj));
+
+		fences[i] = dma_fence_get(f);
+	}
+
+	if (rcu_access_pointer(obj->fence_excl)) {
+		struct dma_fence *f =
+			rcu_dereference_protected(obj->fence_excl,
+						  reservation_object_held(obj));
+
+		fences[i] = dma_fence_get(f);
+	}
+
+	array = dma_fence_array_create(count, fences,
+				       dma_fence_context_alloc(1), 0,
+				       false);
+	if (!array)
+		goto err_fences_put;
+
+	reservation_object_add_excl_fence(obj, &array->base);
+	return 0;
+
+err_fences_put:
+	for (i = 0; i < count; i++)
+		dma_fence_put(fences[i]);
+	kfree(fences);
+	return -ENOMEM;
+}
+
 /**
  * amdgpu_gem_map_attach - &dma_buf_ops.attach implementation
  * @dma_buf: shared DMA buffer
@@ -219,16 +271,18 @@ static int amdgpu_gem_map_attach(struct dma_buf *dma_buf,
 
 	if (attach->dev->driver != adev->dev->driver) {
 		/*
-		 * Wait for all shared fences to complete before we switch to future
-		 * use of exclusive fence on this prime shared bo.
+		 * We only create shared fences for internal use, but importers
+		 * of the dmabuf rely on exclusive fences for implicitly
+		 * tracking write hazards. As any of the current fences may
+		 * correspond to a write, we need to convert all existing
+		 * fences on the resevation object into a single exclusive
+		 * fence.
 		 */
-		r = reservation_object_wait_timeout_rcu(bo->tbo.resv,
-							true, false,
-							MAX_SCHEDULE_TIMEOUT);
-		if (unlikely(r < 0)) {
-			DRM_DEBUG_PRIME("Fence wait failed: %li\n", r);
+		reservation_object_lock(bo->tbo.resv, NULL);
+		r = __reservation_object_make_exclusive(bo->tbo.resv);
+		reservation_object_unlock(bo->tbo.resv);
+		if (r)
 			goto error_unreserve;
-		}
 	}
 
 	/* pin buffer into GTT */
-- 
2.18.0

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH] drm/amdgpu: Transfer fences to dmabuf importer

[Huang Rui]

On Tue, Aug 07, 2018 at 11:45:00AM +0100, Chris Wilson wrote:
> amdgpu only uses shared-fences internally, but dmabuf importers rely on
> implicit write hazard tracking via the reservation_object.fence_excl.
> For example, the importer use the write hazard for timing a page flip to
> only occur after the exporter has finished flushing its write into the
> surface. As such, on exporting a dmabuf, we must either flush all
> outstanding fences (for we do not know which are writes and should have
> been exclusive) or alternatively create a new exclusive fence that is
> the composite of all the existing shared fences, and so will only be
> signaled when all earlier fences are signaled (ensuring that we can not
> be signaled before the completion of any earlier write).
> 
> Testcase: igt/amd_prime/amd-to-i915
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Alex Deucher <alexander.deucher@amd.com>
> Cc: "Christian König" <christian.koenig@amd.com>
> ---
>  drivers/gpu/drm/amd/amdgpu/amdgpu_prime.c | 70 ++++++++++++++++++++---
>  1 file changed, 62 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_prime.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_prime.c
> index 1c5d97f4b4dd..47e6ec5510b6 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_prime.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_prime.c
> @@ -37,6 +37,7 @@
>  #include "amdgpu_display.h"
>  #include <drm/amdgpu_drm.h>
>  #include <linux/dma-buf.h>
> +#include <linux/dma-fence-array.h>
>  
>  static const struct dma_buf_ops amdgpu_dmabuf_ops;
>  
> @@ -188,6 +189,57 @@ amdgpu_gem_prime_import_sg_table(struct drm_device *dev,
>  	return ERR_PTR(ret);
>  }
>  
> +static int
> +__reservation_object_make_exclusive(struct reservation_object *obj)
> +{

Why not you move the helper to reservation.c, and then export symbol for
this file?

Thanks,
Ray

> +	struct reservation_object_list *fobj;
> +	struct dma_fence_array *array;
> +	struct dma_fence **fences;
> +	unsigned int count, i;
> +
> +	fobj = reservation_object_get_list(obj);
> +	if (!fobj)
> +		return 0;
> +
> +	count = !!rcu_access_pointer(obj->fence_excl);
> +	count += fobj->shared_count;
> +
> +	fences = kmalloc_array(sizeof(*fences), count, GFP_KERNEL);
> +	if (!fences)
> +		return -ENOMEM;
> +
> +	for (i = 0; i < fobj->shared_count; i++) {
> +		struct dma_fence *f =
> +			rcu_dereference_protected(fobj->shared[i],
> +						  reservation_object_held(obj));
> +
> +		fences[i] = dma_fence_get(f);
> +	}
> +
> +	if (rcu_access_pointer(obj->fence_excl)) {
> +		struct dma_fence *f =
> +			rcu_dereference_protected(obj->fence_excl,
> +						  reservation_object_held(obj));
> +
> +		fences[i] = dma_fence_get(f);
> +	}
> +
> +	array = dma_fence_array_create(count, fences,
> +				       dma_fence_context_alloc(1), 0,
> +				       false);
> +	if (!array)
> +		goto err_fences_put;
> +
> +	reservation_object_add_excl_fence(obj, &array->base);
> +	return 0;
> +
> +err_fences_put:
> +	for (i = 0; i < count; i++)
> +		dma_fence_put(fences[i]);
> +	kfree(fences);
> +	return -ENOMEM;
> +}
> +
>  /**
>   * amdgpu_gem_map_attach - &dma_buf_ops.attach implementation
>   * @dma_buf: shared DMA buffer
> @@ -219,16 +271,18 @@ static int amdgpu_gem_map_attach(struct dma_buf *dma_buf,
>  
>  	if (attach->dev->driver != adev->dev->driver) {
>  		/*
> -		 * Wait for all shared fences to complete before we switch to future
> -		 * use of exclusive fence on this prime shared bo.
> +		 * We only create shared fences for internal use, but importers
> +		 * of the dmabuf rely on exclusive fences for implicitly
> +		 * tracking write hazards. As any of the current fences may
> +		 * correspond to a write, we need to convert all existing
> +		 * fences on the resevation object into a single exclusive
> +		 * fence.
>  		 */
> -		r = reservation_object_wait_timeout_rcu(bo->tbo.resv,
> -							true, false,
> -							MAX_SCHEDULE_TIMEOUT);
> -		if (unlikely(r < 0)) {
> -			DRM_DEBUG_PRIME("Fence wait failed: %li\n", r);
> +		reservation_object_lock(bo->tbo.resv, NULL);
> +		r = __reservation_object_make_exclusive(bo->tbo.resv);
> +		reservation_object_unlock(bo->tbo.resv);
> +		if (r)
>  			goto error_unreserve;
> -		}
>  	}
>  
>  	/* pin buffer into GTT */
> -- 
> 2.18.0
> 
> _______________________________________________
> amd-gfx mailing list
> amd-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

✗ Fi.CI.CHECKPATCH: warning for drm/amdgpu: Transfer fences to dmabuf importer

[Patchwork]

== Series Details ==

Series: drm/amdgpu: Transfer fences to dmabuf importer
URL   : https://patchwork.freedesktop.org/series/47803/
State : warning

== Summary ==

$ dim checkpatch origin/drm-tip
dce1b6e891b2 drm/amdgpu: Transfer fences to dmabuf importer
-:56: WARNING:ALLOC_ARRAY_ARGS: kmalloc_array uses number as first arg, sizeof is generally wrong
#56: FILE: drivers/gpu/drm/amd/amdgpu/amdgpu_prime.c:207:
+	fences = kmalloc_array(sizeof(*fences), count, GFP_KERNEL);

total: 0 errors, 1 warnings, 0 checks, 90 lines checked

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✗ Fi.CI.SPARSE: warning for drm/amdgpu: Transfer fences to dmabuf importer

[Patchwork]

== Series Details ==

Series: drm/amdgpu: Transfer fences to dmabuf importer
URL   : https://patchwork.freedesktop.org/series/47803/
State : warning

== Summary ==

$ dim sparse origin/drm-tip
Commit: drm/amdgpu: Transfer fences to dmabuf importer
-
+./include/linux/slab.h:631:13: error: undefined identifier '__builtin_mul_overflow'
+./include/linux/slab.h:631:13: warning: call with no type!

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v2] drm/amdgpu: Transfer fences to dmabuf importer

[Chris Wilson]

amdgpu only uses shared-fences internally, but dmabuf importers rely on
implicit write hazard tracking via the reservation_object.fence_excl.
For example, the importer use the write hazard for timing a page flip to
only occur after the exporter has finished flushing its write into the
surface. As such, on exporting a dmabuf, we must either flush all
outstanding fences (for we do not know which are writes and should have
been exclusive) or alternatively create a new exclusive fence that is
the composite of all the existing shared fences, and so will only be
signaled when all earlier fences are signaled (ensuring that we can not
be signaled before the completion of any earlier write).

v2: reservation_object is already locked by amdgpu_bo_reserve()

Testcase: igt/amd_prime/amd-to-i915
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Alex Deucher <alexander.deucher@amd.com>
Cc: "Christian König" <christian.koenig@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_prime.c | 68 ++++++++++++++++++++---
 1 file changed, 60 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_prime.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_prime.c
index 1c5d97f4b4dd..27f639c69e35 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_prime.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_prime.c
@@ -37,6 +37,7 @@
 #include "amdgpu_display.h"
 #include <drm/amdgpu_drm.h>
 #include <linux/dma-buf.h>
+#include <linux/dma-fence-array.h>
 
 static const struct dma_buf_ops amdgpu_dmabuf_ops;
 
@@ -188,6 +189,57 @@ amdgpu_gem_prime_import_sg_table(struct drm_device *dev,
 	return ERR_PTR(ret);
 }
 
+static int
+__reservation_object_make_exclusive(struct reservation_object *obj)
+{
+	struct reservation_object_list *fobj;
+	struct dma_fence_array *array;
+	struct dma_fence **fences;
+	unsigned int count, i;
+
+	fobj = reservation_object_get_list(obj);
+	if (!fobj)
+		return 0;
+
+	count = !!rcu_access_pointer(obj->fence_excl);
+	count += fobj->shared_count;
+
+	fences = kmalloc_array(sizeof(*fences), count, GFP_KERNEL);
+	if (!fences)
+		return -ENOMEM;
+
+	for (i = 0; i < fobj->shared_count; i++) {
+		struct dma_fence *f =
+			rcu_dereference_protected(fobj->shared[i],
+						  reservation_object_held(obj));
+
+		fences[i] = dma_fence_get(f);
+	}
+
+	if (rcu_access_pointer(obj->fence_excl)) {
+		struct dma_fence *f =
+			rcu_dereference_protected(obj->fence_excl,
+						  reservation_object_held(obj));
+
+		fences[i] = dma_fence_get(f);
+	}
+
+	array = dma_fence_array_create(count, fences,
+				       dma_fence_context_alloc(1), 0,
+				       false);
+	if (!array)
+		goto err_fences_put;
+
+	reservation_object_add_excl_fence(obj, &array->base);
+	return 0;
+
+err_fences_put:
+	for (i = 0; i < count; i++)
+		dma_fence_put(fences[i]);
+	kfree(fences);
+	return -ENOMEM;
+}
+
 /**
  * amdgpu_gem_map_attach - &dma_buf_ops.attach implementation
  * @dma_buf: shared DMA buffer
@@ -219,16 +271,16 @@ static int amdgpu_gem_map_attach(struct dma_buf *dma_buf,
 
 	if (attach->dev->driver != adev->dev->driver) {
 		/*
-		 * Wait for all shared fences to complete before we switch to future
-		 * use of exclusive fence on this prime shared bo.
+		 * We only create shared fences for internal use, but importers
+		 * of the dmabuf rely on exclusive fences for implicitly
+		 * tracking write hazards. As any of the current fences may
+		 * correspond to a write, we need to convert all existing
+		 * fences on the resevation object into a single exclusive
+		 * fence.
 		 */
-		r = reservation_object_wait_timeout_rcu(bo->tbo.resv,
-							true, false,
-							MAX_SCHEDULE_TIMEOUT);
-		if (unlikely(r < 0)) {
-			DRM_DEBUG_PRIME("Fence wait failed: %li\n", r);
+		r = __reservation_object_make_exclusive(bo->tbo.resv);
+		if (r)
 			goto error_unreserve;
-		}
 	}
 
 	/* pin buffer into GTT */
-- 
2.18.0

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH] drm/amdgpu: Transfer fences to dmabuf importer

[Chris Wilson]

Quoting Huang Rui (2018-08-07 11:56:24)
> On Tue, Aug 07, 2018 at 11:45:00AM +0100, Chris Wilson wrote:
> > amdgpu only uses shared-fences internally, but dmabuf importers rely on
> > implicit write hazard tracking via the reservation_object.fence_excl.
> > For example, the importer use the write hazard for timing a page flip to
> > only occur after the exporter has finished flushing its write into the
> > surface. As such, on exporting a dmabuf, we must either flush all
> > outstanding fences (for we do not know which are writes and should have
> > been exclusive) or alternatively create a new exclusive fence that is
> > the composite of all the existing shared fences, and so will only be
> > signaled when all earlier fences are signaled (ensuring that we can not
> > be signaled before the completion of any earlier write).
> > 
> > Testcase: igt/amd_prime/amd-to-i915
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > Cc: Alex Deucher <alexander.deucher@amd.com>
> > Cc: "Christian König" <christian.koenig@amd.com>
> > ---
> >  drivers/gpu/drm/amd/amdgpu/amdgpu_prime.c | 70 ++++++++++++++++++++---
> >  1 file changed, 62 insertions(+), 8 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_prime.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_prime.c
> > index 1c5d97f4b4dd..47e6ec5510b6 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_prime.c
> > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_prime.c
> > @@ -37,6 +37,7 @@
> >  #include "amdgpu_display.h"
> >  #include <drm/amdgpu_drm.h>
> >  #include <linux/dma-buf.h>
> > +#include <linux/dma-fence-array.h>
> >  
> >  static const struct dma_buf_ops amdgpu_dmabuf_ops;
> >  
> > @@ -188,6 +189,57 @@ amdgpu_gem_prime_import_sg_table(struct drm_device *dev,
> >       return ERR_PTR(ret);
> >  }
> >  
> > +static int
> > +__reservation_object_make_exclusive(struct reservation_object *obj)
> > +{
> 
> Why not you move the helper to reservation.c, and then export symbol for
> this file?

I have not seen anything else that would wish to use this helper. The
first task is to solve this issue here before worrying about
generalisation.
-Chris
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

[PATCH v3] drm/amdgpu: Transfer fences to dmabuf importer

[Chris Wilson]

amdgpu only uses shared-fences internally, but dmabuf importers rely on
implicit write hazard tracking via the reservation_object.fence_excl.
For example, the importer use the write hazard for timing a page flip to
only occur after the exporter has finished flushing its write into the
surface. As such, on exporting a dmabuf, we must either flush all
outstanding fences (for we do not know which are writes and should have
been exclusive) or alternatively create a new exclusive fence that is
the composite of all the existing shared fences, and so will only be
signaled when all earlier fences are signaled (ensuring that we can not
be signaled before the completion of any earlier write).

v2: reservation_object is already locked by amdgpu_bo_reserve()

Testcase: igt/amd_prime/amd-to-i915
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Alex Deucher <alexander.deucher@amd.com>
Cc: "Christian König" <christian.koenig@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_prime.c | 68 ++++++++++++++++++++---
 1 file changed, 60 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_prime.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_prime.c
index 1c5d97f4b4dd..576a83946c25 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_prime.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_prime.c
@@ -37,6 +37,7 @@
 #include "amdgpu_display.h"
 #include <drm/amdgpu_drm.h>
 #include <linux/dma-buf.h>
+#include <linux/dma-fence-array.h>
 
 static const struct dma_buf_ops amdgpu_dmabuf_ops;
 
@@ -188,6 +189,57 @@ amdgpu_gem_prime_import_sg_table(struct drm_device *dev,
 	return ERR_PTR(ret);
 }
 
+static int
+__reservation_object_make_exclusive(struct reservation_object *obj)
+{
+	struct reservation_object_list *fobj;
+	struct dma_fence_array *array;
+	struct dma_fence **fences;
+	unsigned int count, i;
+
+	fobj = reservation_object_get_list(obj);
+	if (!fobj)
+		return 0;
+
+	count = !!rcu_access_pointer(obj->fence_excl);
+	count += fobj->shared_count;
+
+	fences = kmalloc_array(count, sizeof(*fences), GFP_KERNEL);
+	if (!fences)
+		return -ENOMEM;
+
+	for (i = 0; i < fobj->shared_count; i++) {
+		struct dma_fence *f =
+			rcu_dereference_protected(fobj->shared[i],
+						  reservation_object_held(obj));
+
+		fences[i] = dma_fence_get(f);
+	}
+
+	if (rcu_access_pointer(obj->fence_excl)) {
+		struct dma_fence *f =
+			rcu_dereference_protected(obj->fence_excl,
+						  reservation_object_held(obj));
+
+		fences[i] = dma_fence_get(f);
+	}
+
+	array = dma_fence_array_create(count, fences,
+				       dma_fence_context_alloc(1), 0,
+				       false);
+	if (!array)
+		goto err_fences_put;
+
+	reservation_object_add_excl_fence(obj, &array->base);
+	return 0;
+
+err_fences_put:
+	for (i = 0; i < count; i++)
+		dma_fence_put(fences[i]);
+	kfree(fences);
+	return -ENOMEM;
+}
+
 /**
  * amdgpu_gem_map_attach - &dma_buf_ops.attach implementation
  * @dma_buf: shared DMA buffer
@@ -219,16 +271,16 @@ static int amdgpu_gem_map_attach(struct dma_buf *dma_buf,
 
 	if (attach->dev->driver != adev->dev->driver) {
 		/*
-		 * Wait for all shared fences to complete before we switch to future
-		 * use of exclusive fence on this prime shared bo.
+		 * We only create shared fences for internal use, but importers
+		 * of the dmabuf rely on exclusive fences for implicitly
+		 * tracking write hazards. As any of the current fences may
+		 * correspond to a write, we need to convert all existing
+		 * fences on the resevation object into a single exclusive
+		 * fence.
 		 */
-		r = reservation_object_wait_timeout_rcu(bo->tbo.resv,
-							true, false,
-							MAX_SCHEDULE_TIMEOUT);
-		if (unlikely(r < 0)) {
-			DRM_DEBUG_PRIME("Fence wait failed: %li\n", r);
+		r = __reservation_object_make_exclusive(bo->tbo.resv);
+		if (r)
 			goto error_unreserve;
-		}
 	}
 
 	/* pin buffer into GTT */
-- 
2.18.0

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✓ Fi.CI.BAT: success for drm/amdgpu: Transfer fences to dmabuf importer

[Patchwork]

== Series Details ==

Series: drm/amdgpu: Transfer fences to dmabuf importer
URL   : https://patchwork.freedesktop.org/series/47803/
State : success

== Summary ==

= CI Bug Log - changes from CI_DRM_4625 -> Patchwork_9867 =

== Summary - SUCCESS ==

  No regressions found.

  External URL: https://patchwork.freedesktop.org/api/1.0/series/47803/revisions/1/mbox/

== Possible new issues ==

  Here are the unknown changes that may have been introduced in Patchwork_9867:

  === IGT changes ===

    ==== Possible regressions ====

    igt@gem_flink_basic@double-flink:
      {fi-bsw-kefka}:     PASS -> INCOMPLETE

    
== Known issues ==

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

  === IGT changes ===

    ==== Issues hit ====

    igt@drv_selftest@live_coherency:
      fi-gdg-551:         PASS -> DMESG-FAIL (fdo#107164)

    igt@drv_selftest@live_hangcheck:
      fi-skl-guc:         PASS -> DMESG-FAIL (fdo#107174)

    igt@kms_pipe_crc_basic@hang-read-crc-pipe-b:
      fi-skl-guc:         PASS -> FAIL (fdo#103191)

    igt@kms_pipe_crc_basic@nonblocking-crc-pipe-b:
      {fi-byt-clapper}:   PASS -> FAIL (fdo#107362)

    {igt@kms_psr@primary_mmap_gtt}:
      fi-cnl-psr:         PASS -> DMESG-WARN (fdo#107372)

    
    ==== Possible fixes ====

    igt@drv_selftest@live_workarounds:
      fi-kbl-7500u:       DMESG-FAIL (fdo#107292) -> PASS

    igt@gem_exec_suspend@basic-s3:
      {fi-cfl-8109u}:     DMESG-WARN (k.org#200587) -> PASS

    igt@kms_pipe_crc_basic@suspend-read-crc-pipe-a:
      {fi-cfl-8109u}:     DMESG-WARN (fdo#107345) -> PASS

    igt@kms_pipe_crc_basic@suspend-read-crc-pipe-b:
      {fi-cfl-8109u}:     INCOMPLETE (fdo#106070) -> PASS

    
    ==== Warnings ====

    {igt@kms_psr@primary_page_flip}:
      fi-cnl-psr:         DMESG-FAIL (fdo#107372) -> DMESG-WARN (fdo#107372)

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

  fdo#103191 https://bugs.freedesktop.org/show_bug.cgi?id=103191
  fdo#106070 https://bugs.freedesktop.org/show_bug.cgi?id=106070
  fdo#107164 https://bugs.freedesktop.org/show_bug.cgi?id=107164
  fdo#107174 https://bugs.freedesktop.org/show_bug.cgi?id=107174
  fdo#107292 https://bugs.freedesktop.org/show_bug.cgi?id=107292
  fdo#107345 https://bugs.freedesktop.org/show_bug.cgi?id=107345
  fdo#107362 https://bugs.freedesktop.org/show_bug.cgi?id=107362
  fdo#107372 https://bugs.freedesktop.org/show_bug.cgi?id=107372
  k.org#200587 https://bugzilla.kernel.org/show_bug.cgi?id=200587


== Participating hosts (53 -> 47) ==

  Missing    (6): fi-ilk-m540 fi-hsw-4200u fi-byt-squawks fi-bsw-cyan fi-ctg-p8600 fi-kbl-8809g 


== Build changes ==

    * Linux: CI_DRM_4625 -> Patchwork_9867

  CI_DRM_4625: 1c551a0db398467c305e99ede600b3919609729f @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_4587: 5d78c73d871525ec9caecd88ad7d9abe36637314 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_9867: dce1b6e891b206c85719ce823234de527c1576b2 @ git://anongit.freedesktop.org/gfx-ci/linux


== Linux commits ==

dce1b6e891b2 drm/amdgpu: Transfer fences to dmabuf importer

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_9867/issues.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✗ Fi.CI.SPARSE: warning for drm/amdgpu: Transfer fences to dmabuf importer (rev3)

[Patchwork]

== Series Details ==

Series: drm/amdgpu: Transfer fences to dmabuf importer (rev3)
URL   : https://patchwork.freedesktop.org/series/47803/
State : warning

== Summary ==

$ dim sparse origin/drm-tip
Commit: drm/amdgpu: Transfer fences to dmabuf importer
-
+./include/linux/slab.h:631:13: error: undefined identifier '__builtin_mul_overflow'
+./include/linux/slab.h:631:13: warning: call with no type!

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v3] drm/amdgpu: Transfer fences to dmabuf importer

[Christian König]

Am 07.08.2018 um 13:05 schrieb Chris Wilson:
> amdgpu only uses shared-fences internally, but dmabuf importers rely on
> implicit write hazard tracking via the reservation_object.fence_excl.

Well I would rather suggest a solution where we stop doing this.

The problem here is that i915 assumes that a write operation always 
needs exclusive access to an object protected by an reservation object.

At least for amdgpu, radeon and nouveau this assumption is incorrect, 
but only amdgpu has a design where userspace is not lying to the kernel 
about it's read/write access.

What we should do instead is to add a flag to each shared fence to note 
if it is a write operation or not. Then we can trivially add a function 
to wait on on those in i915.

I should have pushed harder for this solution when the problem came up 
initially,
Christian.

> For example, the importer use the write hazard for timing a page flip to
> only occur after the exporter has finished flushing its write into the
> surface. As such, on exporting a dmabuf, we must either flush all
> outstanding fences (for we do not know which are writes and should have
> been exclusive) or alternatively create a new exclusive fence that is
> the composite of all the existing shared fences, and so will only be
> signaled when all earlier fences are signaled (ensuring that we can not
> be signaled before the completion of any earlier write).
>
> v2: reservation_object is already locked by amdgpu_bo_reserve()
>
> Testcase: igt/amd_prime/amd-to-i915
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Alex Deucher <alexander.deucher@amd.com>
> Cc: "Christian König" <christian.koenig@amd.com>
> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_prime.c | 68 ++++++++++++++++++++---
>   1 file changed, 60 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_prime.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_prime.c
> index 1c5d97f4b4dd..576a83946c25 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_prime.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_prime.c
> @@ -37,6 +37,7 @@
>   #include "amdgpu_display.h"
>   #include <drm/amdgpu_drm.h>
>   #include <linux/dma-buf.h>
> +#include <linux/dma-fence-array.h>
>   
>   static const struct dma_buf_ops amdgpu_dmabuf_ops;
>   
> @@ -188,6 +189,57 @@ amdgpu_gem_prime_import_sg_table(struct drm_device *dev,
>   	return ERR_PTR(ret);
>   }
>   
> +static int
> +__reservation_object_make_exclusive(struct reservation_object *obj)
> +{
> +	struct reservation_object_list *fobj;
> +	struct dma_fence_array *array;
> +	struct dma_fence **fences;
> +	unsigned int count, i;
> +
> +	fobj = reservation_object_get_list(obj);
> +	if (!fobj)
> +		return 0;
> +
> +	count = !!rcu_access_pointer(obj->fence_excl);
> +	count += fobj->shared_count;
> +
> +	fences = kmalloc_array(count, sizeof(*fences), GFP_KERNEL);
> +	if (!fences)
> +		return -ENOMEM;
> +
> +	for (i = 0; i < fobj->shared_count; i++) {
> +		struct dma_fence *f =
> +			rcu_dereference_protected(fobj->shared[i],
> +						  reservation_object_held(obj));
> +
> +		fences[i] = dma_fence_get(f);
> +	}
> +
> +	if (rcu_access_pointer(obj->fence_excl)) {
> +		struct dma_fence *f =
> +			rcu_dereference_protected(obj->fence_excl,
> +						  reservation_object_held(obj));
> +
> +		fences[i] = dma_fence_get(f);
> +	}
> +
> +	array = dma_fence_array_create(count, fences,
> +				       dma_fence_context_alloc(1), 0,
> +				       false);
> +	if (!array)
> +		goto err_fences_put;
> +
> +	reservation_object_add_excl_fence(obj, &array->base);
> +	return 0;
> +
> +err_fences_put:
> +	for (i = 0; i < count; i++)
> +		dma_fence_put(fences[i]);
> +	kfree(fences);
> +	return -ENOMEM;
> +}
> +
>   /**
>    * amdgpu_gem_map_attach - &dma_buf_ops.attach implementation
>    * @dma_buf: shared DMA buffer
> @@ -219,16 +271,16 @@ static int amdgpu_gem_map_attach(struct dma_buf *dma_buf,
>   
>   	if (attach->dev->driver != adev->dev->driver) {
>   		/*
> -		 * Wait for all shared fences to complete before we switch to future
> -		 * use of exclusive fence on this prime shared bo.
> +		 * We only create shared fences for internal use, but importers
> +		 * of the dmabuf rely on exclusive fences for implicitly
> +		 * tracking write hazards. As any of the current fences may
> +		 * correspond to a write, we need to convert all existing
> +		 * fences on the resevation object into a single exclusive
> +		 * fence.
>   		 */
> -		r = reservation_object_wait_timeout_rcu(bo->tbo.resv,
> -							true, false,
> -							MAX_SCHEDULE_TIMEOUT);
> -		if (unlikely(r < 0)) {
> -			DRM_DEBUG_PRIME("Fence wait failed: %li\n", r);
> +		r = __reservation_object_make_exclusive(bo->tbo.resv);
> +		if (r)
>   			goto error_unreserve;
> -		}
>   	}
>   
>   	/* pin buffer into GTT */

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

✓ Fi.CI.BAT: success for drm/amdgpu: Transfer fences to dmabuf importer (rev3)

[Patchwork]

== Series Details ==

Series: drm/amdgpu: Transfer fences to dmabuf importer (rev3)
URL   : https://patchwork.freedesktop.org/series/47803/
State : success

== Summary ==

= CI Bug Log - changes from CI_DRM_4625 -> Patchwork_9868 =

== Summary - SUCCESS ==

  No regressions found.

  External URL: https://patchwork.freedesktop.org/api/1.0/series/47803/revisions/3/mbox/

== Known issues ==

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

  === IGT changes ===

    ==== Issues hit ====

    igt@drv_selftest@live_workarounds:
      {fi-bsw-kefka}:     PASS -> DMESG-FAIL (fdo#107292)
      fi-bsw-n3050:       PASS -> DMESG-FAIL (fdo#107292)
      {fi-kbl-8809g}:     PASS -> DMESG-FAIL (fdo#107292)
      fi-skl-6700k2:      PASS -> DMESG-FAIL (fdo#107292)

    igt@kms_frontbuffer_tracking@basic:
      {fi-byt-clapper}:   PASS -> FAIL (fdo#103167)

    igt@kms_pipe_crc_basic@suspend-read-crc-pipe-c:
      fi-bxt-dsi:         PASS -> INCOMPLETE (fdo#103927)

    {igt@kms_psr@primary_mmap_gtt}:
      fi-cnl-psr:         PASS -> DMESG-WARN (fdo#107372)

    
    ==== Possible fixes ====

    {igt@amdgpu/amd_prime@amd-to-i915}:
      {fi-kbl-8809g}:     FAIL (fdo#107341) -> PASS

    igt@drv_selftest@live_workarounds:
      fi-kbl-7500u:       DMESG-FAIL (fdo#107292) -> PASS

    igt@gem_exec_suspend@basic-s3:
      {fi-cfl-8109u}:     DMESG-WARN (k.org#200587) -> PASS

    igt@kms_pipe_crc_basic@suspend-read-crc-pipe-a:
      {fi-cfl-8109u}:     DMESG-WARN (fdo#107345) -> PASS

    igt@kms_pipe_crc_basic@suspend-read-crc-pipe-b:
      {fi-cfl-8109u}:     INCOMPLETE (fdo#106070) -> PASS

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

  fdo#103167 https://bugs.freedesktop.org/show_bug.cgi?id=103167
  fdo#103927 https://bugs.freedesktop.org/show_bug.cgi?id=103927
  fdo#106070 https://bugs.freedesktop.org/show_bug.cgi?id=106070
  fdo#107292 https://bugs.freedesktop.org/show_bug.cgi?id=107292
  fdo#107341 https://bugs.freedesktop.org/show_bug.cgi?id=107341
  fdo#107345 https://bugs.freedesktop.org/show_bug.cgi?id=107345
  fdo#107372 https://bugs.freedesktop.org/show_bug.cgi?id=107372
  k.org#200587 https://bugzilla.kernel.org/show_bug.cgi?id=200587


== Participating hosts (53 -> 48) ==

  Missing    (5): fi-ctg-p8600 fi-ilk-m540 fi-byt-squawks fi-bsw-cyan fi-hsw-4200u 


== Build changes ==

    * Linux: CI_DRM_4625 -> Patchwork_9868

  CI_DRM_4625: 1c551a0db398467c305e99ede600b3919609729f @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_4587: 5d78c73d871525ec9caecd88ad7d9abe36637314 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_9868: 4dac514761a842ea0674d39c80a939238836ed71 @ git://anongit.freedesktop.org/gfx-ci/linux


== Linux commits ==

4dac514761a8 drm/amdgpu: Transfer fences to dmabuf importer

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_9868/issues.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✓ Fi.CI.IGT: success for drm/amdgpu: Transfer fences to dmabuf importer (rev3)

[Patchwork]

== Series Details ==

Series: drm/amdgpu: Transfer fences to dmabuf importer (rev3)
URL   : https://patchwork.freedesktop.org/series/47803/
State : success

== Summary ==

= CI Bug Log - changes from CI_DRM_4625_full -> Patchwork_9868_full =

== Summary - SUCCESS ==

  No regressions found.

  

== Known issues ==

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

  === IGT changes ===

    ==== Issues hit ====

    igt@gem_ppgtt@blt-vs-render-ctx0:
      shard-kbl:          PASS -> INCOMPLETE (fdo#106023, fdo#103665)

    igt@kms_cursor_crc@cursor-128x128-suspend:
      shard-glk:          PASS -> FAIL (fdo#103375)
      shard-kbl:          PASS -> INCOMPLETE (fdo#103665)

    igt@kms_cursor_legacy@cursor-vs-flip-toggle:
      shard-hsw:          PASS -> FAIL (fdo#103355)

    igt@kms_draw_crc@draw-method-rgb565-mmap-gtt-ytiled:
      shard-glk:          PASS -> FAIL (fdo#103184)

    igt@kms_setmode@basic:
      shard-glk:          PASS -> FAIL (fdo#99912)

    igt@perf@short-reads:
      shard-kbl:          PASS -> FAIL (fdo#103183)

    
    ==== Possible fixes ====

    igt@gem_eio@reset-stress:
      shard-hsw:          FAIL (fdo#107500) -> PASS

    igt@kms_flip@flip-vs-expired-vblank-interruptible:
      shard-glk:          FAIL (fdo#105363, fdo#102887) -> PASS

    igt@kms_frontbuffer_tracking@fbc-2p-scndscrn-pri-indfb-draw-mmap-cpu:
      shard-glk:          FAIL (fdo#103167) -> PASS

    igt@perf@polling:
      shard-hsw:          FAIL (fdo#102252) -> PASS

    
  fdo#102252 https://bugs.freedesktop.org/show_bug.cgi?id=102252
  fdo#102887 https://bugs.freedesktop.org/show_bug.cgi?id=102887
  fdo#103167 https://bugs.freedesktop.org/show_bug.cgi?id=103167
  fdo#103183 https://bugs.freedesktop.org/show_bug.cgi?id=103183
  fdo#103184 https://bugs.freedesktop.org/show_bug.cgi?id=103184
  fdo#103355 https://bugs.freedesktop.org/show_bug.cgi?id=103355
  fdo#103375 https://bugs.freedesktop.org/show_bug.cgi?id=103375
  fdo#103665 https://bugs.freedesktop.org/show_bug.cgi?id=103665
  fdo#105363 https://bugs.freedesktop.org/show_bug.cgi?id=105363
  fdo#106023 https://bugs.freedesktop.org/show_bug.cgi?id=106023
  fdo#107500 https://bugs.freedesktop.org/show_bug.cgi?id=107500
  fdo#99912 https://bugs.freedesktop.org/show_bug.cgi?id=99912


== Participating hosts (5 -> 5) ==

  No changes in participating hosts


== Build changes ==

    * Linux: CI_DRM_4625 -> Patchwork_9868

  CI_DRM_4625: 1c551a0db398467c305e99ede600b3919609729f @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_4587: 5d78c73d871525ec9caecd88ad7d9abe36637314 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_9868: 4dac514761a842ea0674d39c80a939238836ed71 @ 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_9868/shards.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v3] drm/amdgpu: Transfer fences to dmabuf importer

[Daniel Vetter]

On Tue, Aug 07, 2018 at 01:23:19PM +0200, Christian König wrote:
> Am 07.08.2018 um 13:05 schrieb Chris Wilson:
> > amdgpu only uses shared-fences internally, but dmabuf importers rely on
> > implicit write hazard tracking via the reservation_object.fence_excl.
> 
> Well I would rather suggest a solution where we stop doing this.
> 
> The problem here is that i915 assumes that a write operation always needs
> exclusive access to an object protected by an reservation object.
> 
> At least for amdgpu, radeon and nouveau this assumption is incorrect, but
> only amdgpu has a design where userspace is not lying to the kernel about
> it's read/write access.
> 
> What we should do instead is to add a flag to each shared fence to note if
> it is a write operation or not. Then we can trivially add a function to wait
> on on those in i915.
> 
> I should have pushed harder for this solution when the problem came up
> initially,

For shared buffers in an implicit syncing setup like dri2 the exclusive
fence _is_ your write fence. That's how this stuff works. Note it's only
for implicit fencing for dri2 shared buffers. i915 lies as much as
everyone else for explicit syncing.

Now as long as you only share within amdgpu you can do whatever you want
to, but for anything shared outside of amdgpu, if the buffer isn't shared
through an explicit syncing protocol, then you do have to set the
exclusive fence. That's at least how this stuff works right now with all
other drivers.

For i915 we had to do some uapi trickery to make this work in all cases,
since only really userspace knows whether a write should be a shared or
exclusive write. But that's stricly an issue limited to your driver, and
dosn't need changes to reservation object all throughout the stack.

Aside: If you want to attach multiple write fences to the exclusive fence,
that should be doable with the array fences.
-Daniel

> Christian.
> 
> > For example, the importer use the write hazard for timing a page flip to
> > only occur after the exporter has finished flushing its write into the
> > surface. As such, on exporting a dmabuf, we must either flush all
> > outstanding fences (for we do not know which are writes and should have
> > been exclusive) or alternatively create a new exclusive fence that is
> > the composite of all the existing shared fences, and so will only be
> > signaled when all earlier fences are signaled (ensuring that we can not
> > be signaled before the completion of any earlier write).
> > 
> > v2: reservation_object is already locked by amdgpu_bo_reserve()
> > 
> > Testcase: igt/amd_prime/amd-to-i915
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > Cc: Alex Deucher <alexander.deucher@amd.com>
> > Cc: "Christian König" <christian.koenig@amd.com>
> > ---
> >   drivers/gpu/drm/amd/amdgpu/amdgpu_prime.c | 68 ++++++++++++++++++++---
> >   1 file changed, 60 insertions(+), 8 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_prime.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_prime.c
> > index 1c5d97f4b4dd..576a83946c25 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_prime.c
> > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_prime.c
> > @@ -37,6 +37,7 @@
> >   #include "amdgpu_display.h"
> >   #include <drm/amdgpu_drm.h>
> >   #include <linux/dma-buf.h>
> > +#include <linux/dma-fence-array.h>
> >   static const struct dma_buf_ops amdgpu_dmabuf_ops;
> > @@ -188,6 +189,57 @@ amdgpu_gem_prime_import_sg_table(struct drm_device *dev,
> >   	return ERR_PTR(ret);
> >   }
> > +static int
> > +__reservation_object_make_exclusive(struct reservation_object *obj)
> > +{
> > +	struct reservation_object_list *fobj;
> > +	struct dma_fence_array *array;
> > +	struct dma_fence **fences;
> > +	unsigned int count, i;
> > +
> > +	fobj = reservation_object_get_list(obj);
> > +	if (!fobj)
> > +		return 0;
> > +
> > +	count = !!rcu_access_pointer(obj->fence_excl);
> > +	count += fobj->shared_count;
> > +
> > +	fences = kmalloc_array(count, sizeof(*fences), GFP_KERNEL);
> > +	if (!fences)
> > +		return -ENOMEM;
> > +
> > +	for (i = 0; i < fobj->shared_count; i++) {
> > +		struct dma_fence *f =
> > +			rcu_dereference_protected(fobj->shared[i],
> > +						  reservation_object_held(obj));
> > +
> > +		fences[i] = dma_fence_get(f);
> > +	}
> > +
> > +	if (rcu_access_pointer(obj->fence_excl)) {
> > +		struct dma_fence *f =
> > +			rcu_dereference_protected(obj->fence_excl,
> > +						  reservation_object_held(obj));
> > +
> > +		fences[i] = dma_fence_get(f);
> > +	}
> > +
> > +	array = dma_fence_array_create(count, fences,
> > +				       dma_fence_context_alloc(1), 0,
> > +				       false);
> > +	if (!array)
> > +		goto err_fences_put;
> > +
> > +	reservation_object_add_excl_fence(obj, &array->base);
> > +	return 0;
> > +
> > +err_fences_put:
> > +	for (i = 0; i < count; i++)
> > +		dma_fence_put(fences[i]);
> > +	kfree(fences);
> > +	return -ENOMEM;
> > +}
> > +
> >   /**
> >    * amdgpu_gem_map_attach - &dma_buf_ops.attach implementation
> >    * @dma_buf: shared DMA buffer
> > @@ -219,16 +271,16 @@ static int amdgpu_gem_map_attach(struct dma_buf *dma_buf,
> >   	if (attach->dev->driver != adev->dev->driver) {
> >   		/*
> > -		 * Wait for all shared fences to complete before we switch to future
> > -		 * use of exclusive fence on this prime shared bo.
> > +		 * We only create shared fences for internal use, but importers
> > +		 * of the dmabuf rely on exclusive fences for implicitly
> > +		 * tracking write hazards. As any of the current fences may
> > +		 * correspond to a write, we need to convert all existing
> > +		 * fences on the resevation object into a single exclusive
> > +		 * fence.
> >   		 */
> > -		r = reservation_object_wait_timeout_rcu(bo->tbo.resv,
> > -							true, false,
> > -							MAX_SCHEDULE_TIMEOUT);
> > -		if (unlikely(r < 0)) {
> > -			DRM_DEBUG_PRIME("Fence wait failed: %li\n", r);
> > +		r = __reservation_object_make_exclusive(bo->tbo.resv);
> > +		if (r)
> >   			goto error_unreserve;
> > -		}
> >   	}
> >   	/* pin buffer into GTT */
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v3] drm/amdgpu: Transfer fences to dmabuf importer

[Daniel Vetter]

On Tue, Aug 07, 2018 at 02:43:22PM +0200, Daniel Vetter wrote:
> On Tue, Aug 07, 2018 at 01:23:19PM +0200, Christian König wrote:
> > Am 07.08.2018 um 13:05 schrieb Chris Wilson:
> > > amdgpu only uses shared-fences internally, but dmabuf importers rely on
> > > implicit write hazard tracking via the reservation_object.fence_excl.
> > 
> > Well I would rather suggest a solution where we stop doing this.
> > 
> > The problem here is that i915 assumes that a write operation always needs
> > exclusive access to an object protected by an reservation object.
> > 
> > At least for amdgpu, radeon and nouveau this assumption is incorrect, but
> > only amdgpu has a design where userspace is not lying to the kernel about
> > it's read/write access.
> > 
> > What we should do instead is to add a flag to each shared fence to note if
> > it is a write operation or not. Then we can trivially add a function to wait
> > on on those in i915.
> > 
> > I should have pushed harder for this solution when the problem came up
> > initially,
> 
> For shared buffers in an implicit syncing setup like dri2 the exclusive
> fence _is_ your write fence. That's how this stuff works. Note it's only
> for implicit fencing for dri2 shared buffers. i915 lies as much as
> everyone else for explicit syncing.

Just realized that dri3 is exactly in the same boat still afaiui. Anyway,
tldr here isn't that i915 is the exception, but amdgpu. If you have a
shared buffer used in an implicitly synced protocol, you have to set the
exclusive fence to guard writes. How you do that is up to you really. And
if a bitfield in reservation_object would help in tracking these I guess
we could add that, but I think that could as well just be put into an
amdgpu structure somewhere. And would be less confusing for everyone else
if it's not in struct reservation_object.
-Daniel

> 
> Now as long as you only share within amdgpu you can do whatever you want
> to, but for anything shared outside of amdgpu, if the buffer isn't shared
> through an explicit syncing protocol, then you do have to set the
> exclusive fence. That's at least how this stuff works right now with all
> other drivers.
> 
> For i915 we had to do some uapi trickery to make this work in all cases,
> since only really userspace knows whether a write should be a shared or
> exclusive write. But that's stricly an issue limited to your driver, and
> dosn't need changes to reservation object all throughout the stack.
> 
> Aside: If you want to attach multiple write fences to the exclusive fence,
> that should be doable with the array fences.
> -Daniel
> 
> > Christian.
> > 
> > > For example, the importer use the write hazard for timing a page flip to
> > > only occur after the exporter has finished flushing its write into the
> > > surface. As such, on exporting a dmabuf, we must either flush all
> > > outstanding fences (for we do not know which are writes and should have
> > > been exclusive) or alternatively create a new exclusive fence that is
> > > the composite of all the existing shared fences, and so will only be
> > > signaled when all earlier fences are signaled (ensuring that we can not
> > > be signaled before the completion of any earlier write).
> > > 
> > > v2: reservation_object is already locked by amdgpu_bo_reserve()
> > > 
> > > Testcase: igt/amd_prime/amd-to-i915
> > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > > Cc: Alex Deucher <alexander.deucher@amd.com>
> > > Cc: "Christian König" <christian.koenig@amd.com>
> > > ---
> > >   drivers/gpu/drm/amd/amdgpu/amdgpu_prime.c | 68 ++++++++++++++++++++---
> > >   1 file changed, 60 insertions(+), 8 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_prime.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_prime.c
> > > index 1c5d97f4b4dd..576a83946c25 100644
> > > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_prime.c
> > > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_prime.c
> > > @@ -37,6 +37,7 @@
> > >   #include "amdgpu_display.h"
> > >   #include <drm/amdgpu_drm.h>
> > >   #include <linux/dma-buf.h>
> > > +#include <linux/dma-fence-array.h>
> > >   static const struct dma_buf_ops amdgpu_dmabuf_ops;
> > > @@ -188,6 +189,57 @@ amdgpu_gem_prime_import_sg_table(struct drm_device *dev,
> > >   	return ERR_PTR(ret);
> > >   }
> > > +static int
> > > +__reservation_object_make_exclusive(struct reservation_object *obj)
> > > +{
> > > +	struct reservation_object_list *fobj;
> > > +	struct dma_fence_array *array;
> > > +	struct dma_fence **fences;
> > > +	unsigned int count, i;
> > > +
> > > +	fobj = reservation_object_get_list(obj);
> > > +	if (!fobj)
> > > +		return 0;
> > > +
> > > +	count = !!rcu_access_pointer(obj->fence_excl);
> > > +	count += fobj->shared_count;
> > > +
> > > +	fences = kmalloc_array(count, sizeof(*fences), GFP_KERNEL);
> > > +	if (!fences)
> > > +		return -ENOMEM;
> > > +
> > > +	for (i = 0; i < fobj->shared_count; i++) {
> > > +		struct dma_fence *f =
> > > +			rcu_dereference_protected(fobj->shared[i],
> > > +						  reservation_object_held(obj));
> > > +
> > > +		fences[i] = dma_fence_get(f);
> > > +	}
> > > +
> > > +	if (rcu_access_pointer(obj->fence_excl)) {
> > > +		struct dma_fence *f =
> > > +			rcu_dereference_protected(obj->fence_excl,
> > > +						  reservation_object_held(obj));
> > > +
> > > +		fences[i] = dma_fence_get(f);
> > > +	}
> > > +
> > > +	array = dma_fence_array_create(count, fences,
> > > +				       dma_fence_context_alloc(1), 0,
> > > +				       false);
> > > +	if (!array)
> > > +		goto err_fences_put;
> > > +
> > > +	reservation_object_add_excl_fence(obj, &array->base);
> > > +	return 0;
> > > +
> > > +err_fences_put:
> > > +	for (i = 0; i < count; i++)
> > > +		dma_fence_put(fences[i]);
> > > +	kfree(fences);
> > > +	return -ENOMEM;
> > > +}
> > > +
> > >   /**
> > >    * amdgpu_gem_map_attach - &dma_buf_ops.attach implementation
> > >    * @dma_buf: shared DMA buffer
> > > @@ -219,16 +271,16 @@ static int amdgpu_gem_map_attach(struct dma_buf *dma_buf,
> > >   	if (attach->dev->driver != adev->dev->driver) {
> > >   		/*
> > > -		 * Wait for all shared fences to complete before we switch to future
> > > -		 * use of exclusive fence on this prime shared bo.
> > > +		 * We only create shared fences for internal use, but importers
> > > +		 * of the dmabuf rely on exclusive fences for implicitly
> > > +		 * tracking write hazards. As any of the current fences may
> > > +		 * correspond to a write, we need to convert all existing
> > > +		 * fences on the resevation object into a single exclusive
> > > +		 * fence.
> > >   		 */
> > > -		r = reservation_object_wait_timeout_rcu(bo->tbo.resv,
> > > -							true, false,
> > > -							MAX_SCHEDULE_TIMEOUT);
> > > -		if (unlikely(r < 0)) {
> > > -			DRM_DEBUG_PRIME("Fence wait failed: %li\n", r);
> > > +		r = __reservation_object_make_exclusive(bo->tbo.resv);
> > > +		if (r)
> > >   			goto error_unreserve;
> > > -		}
> > >   	}
> > >   	/* pin buffer into GTT */
> > 
> > _______________________________________________
> > dri-devel mailing list
> > dri-devel@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/dri-devel
> 
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH v3] drm/amdgpu: Transfer fences to dmabuf importer

[Christian König]

Am 07.08.2018 um 14:43 schrieb Daniel Vetter:
> On Tue, Aug 07, 2018 at 01:23:19PM +0200, Christian König wrote:
>> Am 07.08.2018 um 13:05 schrieb Chris Wilson:
>>> amdgpu only uses shared-fences internally, but dmabuf importers rely on
>>> implicit write hazard tracking via the reservation_object.fence_excl.
>> Well I would rather suggest a solution where we stop doing this.
>>
>> The problem here is that i915 assumes that a write operation always needs
>> exclusive access to an object protected by an reservation object.
>>
>> At least for amdgpu, radeon and nouveau this assumption is incorrect, but
>> only amdgpu has a design where userspace is not lying to the kernel about
>> it's read/write access.
>>
>> What we should do instead is to add a flag to each shared fence to note if
>> it is a write operation or not. Then we can trivially add a function to wait
>> on on those in i915.
>>
>> I should have pushed harder for this solution when the problem came up
>> initially,
> For shared buffers in an implicit syncing setup like dri2 the exclusive
> fence _is_ your write fence.

And exactly that is absolutely not correct if you ask me.

The exclusive fence is for two use cases, the first one is for drivers 
which don't care about concurrent accesses and only use serialized 
accesses and the second is for kernel uses under the hood of userspace, 
evictions, buffer moves etc..

What i915 does is to abuse that concept for write once read many 
situations, and that in turn is the bug we need to fix here.

> That's how this stuff works. Note it's only
> for implicit fencing for dri2 shared buffers. i915 lies as much as
> everyone else for explicit syncing.

That is really really ugly and should be fixed instead.

> Now as long as you only share within amdgpu you can do whatever you want
> to, but for anything shared outside of amdgpu, if the buffer isn't shared
> through an explicit syncing protocol, then you do have to set the
> exclusive fence. That's at least how this stuff works right now with all
> other drivers.
>
> For i915 we had to do some uapi trickery to make this work in all cases,
> since only really userspace knows whether a write should be a shared or
> exclusive write. But that's stricly an issue limited to your driver, and
> dosn't need changes to reservation object all throughout the stack.

You don't need to change the reservation object all throughout the 
stack. A simple flag if a shared fence is a write or not should be doable.

Give me a day or two to prototype that,
Christian.

> Aside: If you want to attach multiple write fences to the exclusive fence,
> that should be doable with the array fences.
> -Daniel
>
>> Christian.
>>
>>> For example, the importer use the write hazard for timing a page flip to
>>> only occur after the exporter has finished flushing its write into the
>>> surface. As such, on exporting a dmabuf, we must either flush all
>>> outstanding fences (for we do not know which are writes and should have
>>> been exclusive) or alternatively create a new exclusive fence that is
>>> the composite of all the existing shared fences, and so will only be
>>> signaled when all earlier fences are signaled (ensuring that we can not
>>> be signaled before the completion of any earlier write).
>>>
>>> v2: reservation_object is already locked by amdgpu_bo_reserve()
>>>
>>> Testcase: igt/amd_prime/amd-to-i915
>>> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
>>> Cc: Alex Deucher <alexander.deucher@amd.com>
>>> Cc: "Christian König" <christian.koenig@amd.com>
>>> ---
>>>    drivers/gpu/drm/amd/amdgpu/amdgpu_prime.c | 68 ++++++++++++++++++++---
>>>    1 file changed, 60 insertions(+), 8 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_prime.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_prime.c
>>> index 1c5d97f4b4dd..576a83946c25 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_prime.c
>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_prime.c
>>> @@ -37,6 +37,7 @@
>>>    #include "amdgpu_display.h"
>>>    #include <drm/amdgpu_drm.h>
>>>    #include <linux/dma-buf.h>
>>> +#include <linux/dma-fence-array.h>
>>>    static const struct dma_buf_ops amdgpu_dmabuf_ops;
>>> @@ -188,6 +189,57 @@ amdgpu_gem_prime_import_sg_table(struct drm_device *dev,
>>>    	return ERR_PTR(ret);
>>>    }
>>> +static int
>>> +__reservation_object_make_exclusive(struct reservation_object *obj)
>>> +{
>>> +	struct reservation_object_list *fobj;
>>> +	struct dma_fence_array *array;
>>> +	struct dma_fence **fences;
>>> +	unsigned int count, i;
>>> +
>>> +	fobj = reservation_object_get_list(obj);
>>> +	if (!fobj)
>>> +		return 0;
>>> +
>>> +	count = !!rcu_access_pointer(obj->fence_excl);
>>> +	count += fobj->shared_count;
>>> +
>>> +	fences = kmalloc_array(count, sizeof(*fences), GFP_KERNEL);
>>> +	if (!fences)
>>> +		return -ENOMEM;
>>> +
>>> +	for (i = 0; i < fobj->shared_count; i++) {
>>> +		struct dma_fence *f =
>>> +			rcu_dereference_protected(fobj->shared[i],
>>> +						  reservation_object_held(obj));
>>> +
>>> +		fences[i] = dma_fence_get(f);
>>> +	}
>>> +
>>> +	if (rcu_access_pointer(obj->fence_excl)) {
>>> +		struct dma_fence *f =
>>> +			rcu_dereference_protected(obj->fence_excl,
>>> +						  reservation_object_held(obj));
>>> +
>>> +		fences[i] = dma_fence_get(f);
>>> +	}
>>> +
>>> +	array = dma_fence_array_create(count, fences,
>>> +				       dma_fence_context_alloc(1), 0,
>>> +				       false);
>>> +	if (!array)
>>> +		goto err_fences_put;
>>> +
>>> +	reservation_object_add_excl_fence(obj, &array->base);
>>> +	return 0;
>>> +
>>> +err_fences_put:
>>> +	for (i = 0; i < count; i++)
>>> +		dma_fence_put(fences[i]);
>>> +	kfree(fences);
>>> +	return -ENOMEM;
>>> +}
>>> +
>>>    /**
>>>     * amdgpu_gem_map_attach - &dma_buf_ops.attach implementation
>>>     * @dma_buf: shared DMA buffer
>>> @@ -219,16 +271,16 @@ static int amdgpu_gem_map_attach(struct dma_buf *dma_buf,
>>>    	if (attach->dev->driver != adev->dev->driver) {
>>>    		/*
>>> -		 * Wait for all shared fences to complete before we switch to future
>>> -		 * use of exclusive fence on this prime shared bo.
>>> +		 * We only create shared fences for internal use, but importers
>>> +		 * of the dmabuf rely on exclusive fences for implicitly
>>> +		 * tracking write hazards. As any of the current fences may
>>> +		 * correspond to a write, we need to convert all existing
>>> +		 * fences on the resevation object into a single exclusive
>>> +		 * fence.
>>>    		 */
>>> -		r = reservation_object_wait_timeout_rcu(bo->tbo.resv,
>>> -							true, false,
>>> -							MAX_SCHEDULE_TIMEOUT);
>>> -		if (unlikely(r < 0)) {
>>> -			DRM_DEBUG_PRIME("Fence wait failed: %li\n", r);
>>> +		r = __reservation_object_make_exclusive(bo->tbo.resv);
>>> +		if (r)
>>>    			goto error_unreserve;
>>> -		}
>>>    	}
>>>    	/* pin buffer into GTT */
>> _______________________________________________
>> dri-devel mailing list
>> dri-devel@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/dri-devel

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v3] drm/amdgpu: Transfer fences to dmabuf importer

[Daniel Vetter]

On Tue, Aug 07, 2018 at 02:51:50PM +0200, Christian König wrote:
> Am 07.08.2018 um 14:43 schrieb Daniel Vetter:
> > On Tue, Aug 07, 2018 at 01:23:19PM +0200, Christian König wrote:
> > > Am 07.08.2018 um 13:05 schrieb Chris Wilson:
> > > > amdgpu only uses shared-fences internally, but dmabuf importers rely on
> > > > implicit write hazard tracking via the reservation_object.fence_excl.
> > > Well I would rather suggest a solution where we stop doing this.
> > > 
> > > The problem here is that i915 assumes that a write operation always needs
> > > exclusive access to an object protected by an reservation object.
> > > 
> > > At least for amdgpu, radeon and nouveau this assumption is incorrect, but
> > > only amdgpu has a design where userspace is not lying to the kernel about
> > > it's read/write access.
> > > 
> > > What we should do instead is to add a flag to each shared fence to note if
> > > it is a write operation or not. Then we can trivially add a function to wait
> > > on on those in i915.
> > > 
> > > I should have pushed harder for this solution when the problem came up
> > > initially,
> > For shared buffers in an implicit syncing setup like dri2 the exclusive
> > fence _is_ your write fence.
> 
> And exactly that is absolutely not correct if you ask me.
> 
> The exclusive fence is for two use cases, the first one is for drivers which
> don't care about concurrent accesses and only use serialized accesses and
> the second is for kernel uses under the hood of userspace, evictions, buffer
> moves etc..
> 
> What i915 does is to abuse that concept for write once read many situations,
> and that in turn is the bug we need to fix here.

Again, the exclusive fence was added for implicit sync usage like dri2/3.
_Not_ for your own buffer manager. If you want to separate these two
usages, then I guess we can do that, but claiming that i915 is the odd one
out just aint true. You're arguing against all other kms drivers we have
here.

> > That's how this stuff works. Note it's only
> > for implicit fencing for dri2 shared buffers. i915 lies as much as
> > everyone else for explicit syncing.
> 
> That is really really ugly and should be fixed instead.

It works and is uapi now. What's the gain in "fixing" it? And this was
discussed at length when intel and freedreno folks talked about
implementing explicit sync and android sync pts.

> > Now as long as you only share within amdgpu you can do whatever you want
> > to, but for anything shared outside of amdgpu, if the buffer isn't shared
> > through an explicit syncing protocol, then you do have to set the
> > exclusive fence. That's at least how this stuff works right now with all
> > other drivers.
> > 
> > For i915 we had to do some uapi trickery to make this work in all cases,
> > since only really userspace knows whether a write should be a shared or
> > exclusive write. But that's stricly an issue limited to your driver, and
> > dosn't need changes to reservation object all throughout the stack.
> 
> You don't need to change the reservation object all throughout the stack. A
> simple flag if a shared fence is a write or not should be doable.
> 
> Give me a day or two to prototype that,

See my other reply, i think that's only needed for amdgpu internal
book-keeping, so that you can create the correct exclusive fence on first
export. But yeah, adding a bitfield to track which fences need to become
exclusive shouldn't be a tricky solution to implement for amdgpu.
-Daniel

> Christian.
> 
> > Aside: If you want to attach multiple write fences to the exclusive fence,
> > that should be doable with the array fences.
> > -Daniel
> > 
> > > Christian.
> > > 
> > > > For example, the importer use the write hazard for timing a page flip to
> > > > only occur after the exporter has finished flushing its write into the
> > > > surface. As such, on exporting a dmabuf, we must either flush all
> > > > outstanding fences (for we do not know which are writes and should have
> > > > been exclusive) or alternatively create a new exclusive fence that is
> > > > the composite of all the existing shared fences, and so will only be
> > > > signaled when all earlier fences are signaled (ensuring that we can not
> > > > be signaled before the completion of any earlier write).
> > > > 
> > > > v2: reservation_object is already locked by amdgpu_bo_reserve()
> > > > 
> > > > Testcase: igt/amd_prime/amd-to-i915
> > > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > > > Cc: Alex Deucher <alexander.deucher@amd.com>
> > > > Cc: "Christian König" <christian.koenig@amd.com>
> > > > ---
> > > >    drivers/gpu/drm/amd/amdgpu/amdgpu_prime.c | 68 ++++++++++++++++++++---
> > > >    1 file changed, 60 insertions(+), 8 deletions(-)
> > > > 
> > > > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_prime.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_prime.c
> > > > index 1c5d97f4b4dd..576a83946c25 100644
> > > > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_prime.c
> > > > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_prime.c
> > > > @@ -37,6 +37,7 @@
> > > >    #include "amdgpu_display.h"
> > > >    #include <drm/amdgpu_drm.h>
> > > >    #include <linux/dma-buf.h>
> > > > +#include <linux/dma-fence-array.h>
> > > >    static const struct dma_buf_ops amdgpu_dmabuf_ops;
> > > > @@ -188,6 +189,57 @@ amdgpu_gem_prime_import_sg_table(struct drm_device *dev,
> > > >    	return ERR_PTR(ret);
> > > >    }
> > > > +static int
> > > > +__reservation_object_make_exclusive(struct reservation_object *obj)
> > > > +{
> > > > +	struct reservation_object_list *fobj;
> > > > +	struct dma_fence_array *array;
> > > > +	struct dma_fence **fences;
> > > > +	unsigned int count, i;
> > > > +
> > > > +	fobj = reservation_object_get_list(obj);
> > > > +	if (!fobj)
> > > > +		return 0;
> > > > +
> > > > +	count = !!rcu_access_pointer(obj->fence_excl);
> > > > +	count += fobj->shared_count;
> > > > +
> > > > +	fences = kmalloc_array(count, sizeof(*fences), GFP_KERNEL);
> > > > +	if (!fences)
> > > > +		return -ENOMEM;
> > > > +
> > > > +	for (i = 0; i < fobj->shared_count; i++) {
> > > > +		struct dma_fence *f =
> > > > +			rcu_dereference_protected(fobj->shared[i],
> > > > +						  reservation_object_held(obj));
> > > > +
> > > > +		fences[i] = dma_fence_get(f);
> > > > +	}
> > > > +
> > > > +	if (rcu_access_pointer(obj->fence_excl)) {
> > > > +		struct dma_fence *f =
> > > > +			rcu_dereference_protected(obj->fence_excl,
> > > > +						  reservation_object_held(obj));
> > > > +
> > > > +		fences[i] = dma_fence_get(f);
> > > > +	}
> > > > +
> > > > +	array = dma_fence_array_create(count, fences,
> > > > +				       dma_fence_context_alloc(1), 0,
> > > > +				       false);
> > > > +	if (!array)
> > > > +		goto err_fences_put;
> > > > +
> > > > +	reservation_object_add_excl_fence(obj, &array->base);
> > > > +	return 0;
> > > > +
> > > > +err_fences_put:
> > > > +	for (i = 0; i < count; i++)
> > > > +		dma_fence_put(fences[i]);
> > > > +	kfree(fences);
> > > > +	return -ENOMEM;
> > > > +}
> > > > +
> > > >    /**
> > > >     * amdgpu_gem_map_attach - &dma_buf_ops.attach implementation
> > > >     * @dma_buf: shared DMA buffer
> > > > @@ -219,16 +271,16 @@ static int amdgpu_gem_map_attach(struct dma_buf *dma_buf,
> > > >    	if (attach->dev->driver != adev->dev->driver) {
> > > >    		/*
> > > > -		 * Wait for all shared fences to complete before we switch to future
> > > > -		 * use of exclusive fence on this prime shared bo.
> > > > +		 * We only create shared fences for internal use, but importers
> > > > +		 * of the dmabuf rely on exclusive fences for implicitly
> > > > +		 * tracking write hazards. As any of the current fences may
> > > > +		 * correspond to a write, we need to convert all existing
> > > > +		 * fences on the resevation object into a single exclusive
> > > > +		 * fence.
> > > >    		 */
> > > > -		r = reservation_object_wait_timeout_rcu(bo->tbo.resv,
> > > > -							true, false,
> > > > -							MAX_SCHEDULE_TIMEOUT);
> > > > -		if (unlikely(r < 0)) {
> > > > -			DRM_DEBUG_PRIME("Fence wait failed: %li\n", r);
> > > > +		r = __reservation_object_make_exclusive(bo->tbo.resv);
> > > > +		if (r)
> > > >    			goto error_unreserve;
> > > > -		}
> > > >    	}
> > > >    	/* pin buffer into GTT */
> > > _______________________________________________
> > > dri-devel mailing list
> > > dri-devel@lists.freedesktop.org
> > > https://lists.freedesktop.org/mailman/listinfo/dri-devel
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH v3] drm/amdgpu: Transfer fences to dmabuf importer

[Christian König]

Am 07.08.2018 um 14:59 schrieb Daniel Vetter:
> On Tue, Aug 07, 2018 at 02:51:50PM +0200, Christian König wrote:
>> Am 07.08.2018 um 14:43 schrieb Daniel Vetter:
>>> On Tue, Aug 07, 2018 at 01:23:19PM +0200, Christian König wrote:
>>>> Am 07.08.2018 um 13:05 schrieb Chris Wilson:
>>>>> amdgpu only uses shared-fences internally, but dmabuf importers rely on
>>>>> implicit write hazard tracking via the reservation_object.fence_excl.
>>>> Well I would rather suggest a solution where we stop doing this.
>>>>
>>>> The problem here is that i915 assumes that a write operation always needs
>>>> exclusive access to an object protected by an reservation object.
>>>>
>>>> At least for amdgpu, radeon and nouveau this assumption is incorrect, but
>>>> only amdgpu has a design where userspace is not lying to the kernel about
>>>> it's read/write access.
>>>>
>>>> What we should do instead is to add a flag to each shared fence to note if
>>>> it is a write operation or not. Then we can trivially add a function to wait
>>>> on on those in i915.
>>>>
>>>> I should have pushed harder for this solution when the problem came up
>>>> initially,
>>> For shared buffers in an implicit syncing setup like dri2 the exclusive
>>> fence _is_ your write fence.
>> And exactly that is absolutely not correct if you ask me.
>>
>> The exclusive fence is for two use cases, the first one is for drivers which
>> don't care about concurrent accesses and only use serialized accesses and
>> the second is for kernel uses under the hood of userspace, evictions, buffer
>> moves etc..
>>
>> What i915 does is to abuse that concept for write once read many situations,
>> and that in turn is the bug we need to fix here.
> Again, the exclusive fence was added for implicit sync usage like dri2/3.
> _Not_ for your own buffer manager. If you want to separate these two
> usages, then I guess we can do that, but claiming that i915 is the odd one
> out just aint true. You're arguing against all other kms drivers we have
> here.

No I'm not. I discussed exactly that use case with Maarten when the 
reservation object was introduced.

I think that Maarten named the explicit fence on purpose "explicit" 
fence and not "write" fence to make that distinction clear.

I have to admit that this wasn't really documented, but it indeed was 
the original purpose to get away from the idea that writes needs to be 
exclusive.

>>> That's how this stuff works. Note it's only
>>> for implicit fencing for dri2 shared buffers. i915 lies as much as
>>> everyone else for explicit syncing.
>> That is really really ugly and should be fixed instead.
> It works and is uapi now. What's the gain in "fixing" it?

It allows you to implement explicit fencing in the uapi without breaking 
backward compatibility.

See the situation with amdgpu and intel is the same as with userspace 
with mixed implicit and explicit fencing.

If that isn't fixed we will run into the same problem when DRI3 gets 
implicit fencing and some applications starts to use it while others 
still rely on the explicit fencing.

Regards,
Christian.

> And this was
> discussed at length when intel and freedreno folks talked about
> implementing explicit sync and android sync pts.
>
>>> Now as long as you only share within amdgpu you can do whatever you want
>>> to, but for anything shared outside of amdgpu, if the buffer isn't shared
>>> through an explicit syncing protocol, then you do have to set the
>>> exclusive fence. That's at least how this stuff works right now with all
>>> other drivers.
>>>
>>> For i915 we had to do some uapi trickery to make this work in all cases,
>>> since only really userspace knows whether a write should be a shared or
>>> exclusive write. But that's stricly an issue limited to your driver, and
>>> dosn't need changes to reservation object all throughout the stack.
>> You don't need to change the reservation object all throughout the stack. A
>> simple flag if a shared fence is a write or not should be doable.
>>
>> Give me a day or two to prototype that,
> See my other reply, i think that's only needed for amdgpu internal
> book-keeping, so that you can create the correct exclusive fence on first
> export. But yeah, adding a bitfield to track which fences need to become
> exclusive shouldn't be a tricky solution to implement for amdgpu.
> -Daniel
>
>> Christian.
>>
>>> Aside: If you want to attach multiple write fences to the exclusive fence,
>>> that should be doable with the array fences.
>>> -Daniel
>>>
>>>> Christian.
>>>>
>>>>> For example, the importer use the write hazard for timing a page flip to
>>>>> only occur after the exporter has finished flushing its write into the
>>>>> surface. As such, on exporting a dmabuf, we must either flush all
>>>>> outstanding fences (for we do not know which are writes and should have
>>>>> been exclusive) or alternatively create a new exclusive fence that is
>>>>> the composite of all the existing shared fences, and so will only be
>>>>> signaled when all earlier fences are signaled (ensuring that we can not
>>>>> be signaled before the completion of any earlier write).
>>>>>
>>>>> v2: reservation_object is already locked by amdgpu_bo_reserve()
>>>>>
>>>>> Testcase: igt/amd_prime/amd-to-i915
>>>>> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
>>>>> Cc: Alex Deucher <alexander.deucher@amd.com>
>>>>> Cc: "Christian König" <christian.koenig@amd.com>
>>>>> ---
>>>>>     drivers/gpu/drm/amd/amdgpu/amdgpu_prime.c | 68 ++++++++++++++++++++---
>>>>>     1 file changed, 60 insertions(+), 8 deletions(-)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_prime.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_prime.c
>>>>> index 1c5d97f4b4dd..576a83946c25 100644
>>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_prime.c
>>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_prime.c
>>>>> @@ -37,6 +37,7 @@
>>>>>     #include "amdgpu_display.h"
>>>>>     #include <drm/amdgpu_drm.h>
>>>>>     #include <linux/dma-buf.h>
>>>>> +#include <linux/dma-fence-array.h>
>>>>>     static const struct dma_buf_ops amdgpu_dmabuf_ops;
>>>>> @@ -188,6 +189,57 @@ amdgpu_gem_prime_import_sg_table(struct drm_device *dev,
>>>>>     	return ERR_PTR(ret);
>>>>>     }
>>>>> +static int
>>>>> +__reservation_object_make_exclusive(struct reservation_object *obj)
>>>>> +{
>>>>> +	struct reservation_object_list *fobj;
>>>>> +	struct dma_fence_array *array;
>>>>> +	struct dma_fence **fences;
>>>>> +	unsigned int count, i;
>>>>> +
>>>>> +	fobj = reservation_object_get_list(obj);
>>>>> +	if (!fobj)
>>>>> +		return 0;
>>>>> +
>>>>> +	count = !!rcu_access_pointer(obj->fence_excl);
>>>>> +	count += fobj->shared_count;
>>>>> +
>>>>> +	fences = kmalloc_array(count, sizeof(*fences), GFP_KERNEL);
>>>>> +	if (!fences)
>>>>> +		return -ENOMEM;
>>>>> +
>>>>> +	for (i = 0; i < fobj->shared_count; i++) {
>>>>> +		struct dma_fence *f =
>>>>> +			rcu_dereference_protected(fobj->shared[i],
>>>>> +						  reservation_object_held(obj));
>>>>> +
>>>>> +		fences[i] = dma_fence_get(f);
>>>>> +	}
>>>>> +
>>>>> +	if (rcu_access_pointer(obj->fence_excl)) {
>>>>> +		struct dma_fence *f =
>>>>> +			rcu_dereference_protected(obj->fence_excl,
>>>>> +						  reservation_object_held(obj));
>>>>> +
>>>>> +		fences[i] = dma_fence_get(f);
>>>>> +	}
>>>>> +
>>>>> +	array = dma_fence_array_create(count, fences,
>>>>> +				       dma_fence_context_alloc(1), 0,
>>>>> +				       false);
>>>>> +	if (!array)
>>>>> +		goto err_fences_put;
>>>>> +
>>>>> +	reservation_object_add_excl_fence(obj, &array->base);
>>>>> +	return 0;
>>>>> +
>>>>> +err_fences_put:
>>>>> +	for (i = 0; i < count; i++)
>>>>> +		dma_fence_put(fences[i]);
>>>>> +	kfree(fences);
>>>>> +	return -ENOMEM;
>>>>> +}
>>>>> +
>>>>>     /**
>>>>>      * amdgpu_gem_map_attach - &dma_buf_ops.attach implementation
>>>>>      * @dma_buf: shared DMA buffer
>>>>> @@ -219,16 +271,16 @@ static int amdgpu_gem_map_attach(struct dma_buf *dma_buf,
>>>>>     	if (attach->dev->driver != adev->dev->driver) {
>>>>>     		/*
>>>>> -		 * Wait for all shared fences to complete before we switch to future
>>>>> -		 * use of exclusive fence on this prime shared bo.
>>>>> +		 * We only create shared fences for internal use, but importers
>>>>> +		 * of the dmabuf rely on exclusive fences for implicitly
>>>>> +		 * tracking write hazards. As any of the current fences may
>>>>> +		 * correspond to a write, we need to convert all existing
>>>>> +		 * fences on the resevation object into a single exclusive
>>>>> +		 * fence.
>>>>>     		 */
>>>>> -		r = reservation_object_wait_timeout_rcu(bo->tbo.resv,
>>>>> -							true, false,
>>>>> -							MAX_SCHEDULE_TIMEOUT);
>>>>> -		if (unlikely(r < 0)) {
>>>>> -			DRM_DEBUG_PRIME("Fence wait failed: %li\n", r);
>>>>> +		r = __reservation_object_make_exclusive(bo->tbo.resv);
>>>>> +		if (r)
>>>>>     			goto error_unreserve;
>>>>> -		}
>>>>>     	}
>>>>>     	/* pin buffer into GTT */
>>>> _______________________________________________
>>>> dri-devel mailing list
>>>> dri-devel@lists.freedesktop.org
>>>> https://lists.freedesktop.org/mailman/listinfo/dri-devel

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v3] drm/amdgpu: Transfer fences to dmabuf importer

[Daniel Vetter]

On Tue, Aug 07, 2018 at 03:17:06PM +0200, Christian König wrote:
> Am 07.08.2018 um 14:59 schrieb Daniel Vetter:
> > On Tue, Aug 07, 2018 at 02:51:50PM +0200, Christian König wrote:
> > > Am 07.08.2018 um 14:43 schrieb Daniel Vetter:
> > > > On Tue, Aug 07, 2018 at 01:23:19PM +0200, Christian König wrote:
> > > > > Am 07.08.2018 um 13:05 schrieb Chris Wilson:
> > > > > > amdgpu only uses shared-fences internally, but dmabuf importers rely on
> > > > > > implicit write hazard tracking via the reservation_object.fence_excl.
> > > > > Well I would rather suggest a solution where we stop doing this.
> > > > > 
> > > > > The problem here is that i915 assumes that a write operation always needs
> > > > > exclusive access to an object protected by an reservation object.
> > > > > 
> > > > > At least for amdgpu, radeon and nouveau this assumption is incorrect, but
> > > > > only amdgpu has a design where userspace is not lying to the kernel about
> > > > > it's read/write access.
> > > > > 
> > > > > What we should do instead is to add a flag to each shared fence to note if
> > > > > it is a write operation or not. Then we can trivially add a function to wait
> > > > > on on those in i915.
> > > > > 
> > > > > I should have pushed harder for this solution when the problem came up
> > > > > initially,
> > > > For shared buffers in an implicit syncing setup like dri2 the exclusive
> > > > fence _is_ your write fence.
> > > And exactly that is absolutely not correct if you ask me.
> > > 
> > > The exclusive fence is for two use cases, the first one is for drivers which
> > > don't care about concurrent accesses and only use serialized accesses and
> > > the second is for kernel uses under the hood of userspace, evictions, buffer
> > > moves etc..
> > > 
> > > What i915 does is to abuse that concept for write once read many situations,
> > > and that in turn is the bug we need to fix here.
> > Again, the exclusive fence was added for implicit sync usage like dri2/3.
> > _Not_ for your own buffer manager. If you want to separate these two
> > usages, then I guess we can do that, but claiming that i915 is the odd one
> > out just aint true. You're arguing against all other kms drivers we have
> > here.
> 
> No I'm not. I discussed exactly that use case with Maarten when the
> reservation object was introduced.
> 
> I think that Maarten named the explicit fence on purpose "explicit" fence
> and not "write" fence to make that distinction clear.
> 
> I have to admit that this wasn't really documented, but it indeed was the
> original purpose to get away from the idea that writes needs to be
> exclusive.
> 
> > > > That's how this stuff works. Note it's only
> > > > for implicit fencing for dri2 shared buffers. i915 lies as much as
> > > > everyone else for explicit syncing.
> > > That is really really ugly and should be fixed instead.
> > It works and is uapi now. What's the gain in "fixing" it?
> 
> It allows you to implement explicit fencing in the uapi without breaking
> backward compatibility.
> 
> See the situation with amdgpu and intel is the same as with userspace with
> mixed implicit and explicit fencing.
> 
> If that isn't fixed we will run into the same problem when DRI3 gets
> implicit fencing and some applications starts to use it while others still
> rely on the explicit fencing.

I think we're a bit cross-eyed on exact semantics, but yes this is exactly
the use-case. Chris Wilson's use-case tries to emulate exactly what would
happen if implicitly fenced amdgpu rendered buffer should be displayed on
an i915 output. Then you need to set the exclusive fence correctly.

And yes we called them exclusive/shared because shared fences could also
be write fences. But for the implicitly synced userspace case, exclusive =
The write fence.

So probably Chris' patch ends up syncing too much (since for explicit
syncing you don't want to attach an exclusive fence, because userspace
passes the fence around already to the atomic ioctl). But at least it's
correct for the implicit case. But that's entirely an optimization problem
within amdgpu.

Summary: If you have a shared buffer used in implicitly synced buffer
sharing, you _must_ set the exclusive fence to cover all write access.

In any other case (not shared, or not as part of implicitly synced
protocol), you can do whatever you want. So we're not conflagrating
exclusive with write here at all. And yes this is exactly what i915 and
all other drivers do - for explicit fencing we don't set the exclusive
fence, but leave that all to userspace. Also, userspace tells us (because
it knows how the protocol works, not the kernel) when to set an exclusive
fence. For historical reasons the relevant uapi parts are called
read/write, but that's just an accident of (maybe too clever) uapi reuse,
not their actual semantics.
-Daniel
> 
> Regards,
> Christian.
> 
> > And this was
> > discussed at length when intel and freedreno folks talked about
> > implementing explicit sync and android sync pts.
> > 
> > > > Now as long as you only share within amdgpu you can do whatever you want
> > > > to, but for anything shared outside of amdgpu, if the buffer isn't shared
> > > > through an explicit syncing protocol, then you do have to set the
> > > > exclusive fence. That's at least how this stuff works right now with all
> > > > other drivers.
> > > > 
> > > > For i915 we had to do some uapi trickery to make this work in all cases,
> > > > since only really userspace knows whether a write should be a shared or
> > > > exclusive write. But that's stricly an issue limited to your driver, and
> > > > dosn't need changes to reservation object all throughout the stack.
> > > You don't need to change the reservation object all throughout the stack. A
> > > simple flag if a shared fence is a write or not should be doable.
> > > 
> > > Give me a day or two to prototype that,
> > See my other reply, i think that's only needed for amdgpu internal
> > book-keeping, so that you can create the correct exclusive fence on first
> > export. But yeah, adding a bitfield to track which fences need to become
> > exclusive shouldn't be a tricky solution to implement for amdgpu.
> > -Daniel
> > 
> > > Christian.
> > > 
> > > > Aside: If you want to attach multiple write fences to the exclusive fence,
> > > > that should be doable with the array fences.
> > > > -Daniel
> > > > 
> > > > > Christian.
> > > > > 
> > > > > > For example, the importer use the write hazard for timing a page flip to
> > > > > > only occur after the exporter has finished flushing its write into the
> > > > > > surface. As such, on exporting a dmabuf, we must either flush all
> > > > > > outstanding fences (for we do not know which are writes and should have
> > > > > > been exclusive) or alternatively create a new exclusive fence that is
> > > > > > the composite of all the existing shared fences, and so will only be
> > > > > > signaled when all earlier fences are signaled (ensuring that we can not
> > > > > > be signaled before the completion of any earlier write).
> > > > > > 
> > > > > > v2: reservation_object is already locked by amdgpu_bo_reserve()
> > > > > > 
> > > > > > Testcase: igt/amd_prime/amd-to-i915
> > > > > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > > > > > Cc: Alex Deucher <alexander.deucher@amd.com>
> > > > > > Cc: "Christian König" <christian.koenig@amd.com>
> > > > > > ---
> > > > > >     drivers/gpu/drm/amd/amdgpu/amdgpu_prime.c | 68 ++++++++++++++++++++---
> > > > > >     1 file changed, 60 insertions(+), 8 deletions(-)
> > > > > > 
> > > > > > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_prime.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_prime.c
> > > > > > index 1c5d97f4b4dd..576a83946c25 100644
> > > > > > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_prime.c
> > > > > > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_prime.c
> > > > > > @@ -37,6 +37,7 @@
> > > > > >     #include "amdgpu_display.h"
> > > > > >     #include <drm/amdgpu_drm.h>
> > > > > >     #include <linux/dma-buf.h>
> > > > > > +#include <linux/dma-fence-array.h>
> > > > > >     static const struct dma_buf_ops amdgpu_dmabuf_ops;
> > > > > > @@ -188,6 +189,57 @@ amdgpu_gem_prime_import_sg_table(struct drm_device *dev,
> > > > > >     	return ERR_PTR(ret);
> > > > > >     }
> > > > > > +static int
> > > > > > +__reservation_object_make_exclusive(struct reservation_object *obj)
> > > > > > +{
> > > > > > +	struct reservation_object_list *fobj;
> > > > > > +	struct dma_fence_array *array;
> > > > > > +	struct dma_fence **fences;
> > > > > > +	unsigned int count, i;
> > > > > > +
> > > > > > +	fobj = reservation_object_get_list(obj);
> > > > > > +	if (!fobj)
> > > > > > +		return 0;
> > > > > > +
> > > > > > +	count = !!rcu_access_pointer(obj->fence_excl);
> > > > > > +	count += fobj->shared_count;
> > > > > > +
> > > > > > +	fences = kmalloc_array(count, sizeof(*fences), GFP_KERNEL);
> > > > > > +	if (!fences)
> > > > > > +		return -ENOMEM;
> > > > > > +
> > > > > > +	for (i = 0; i < fobj->shared_count; i++) {
> > > > > > +		struct dma_fence *f =
> > > > > > +			rcu_dereference_protected(fobj->shared[i],
> > > > > > +						  reservation_object_held(obj));
> > > > > > +
> > > > > > +		fences[i] = dma_fence_get(f);
> > > > > > +	}
> > > > > > +
> > > > > > +	if (rcu_access_pointer(obj->fence_excl)) {
> > > > > > +		struct dma_fence *f =
> > > > > > +			rcu_dereference_protected(obj->fence_excl,
> > > > > > +						  reservation_object_held(obj));
> > > > > > +
> > > > > > +		fences[i] = dma_fence_get(f);
> > > > > > +	}
> > > > > > +
> > > > > > +	array = dma_fence_array_create(count, fences,
> > > > > > +				       dma_fence_context_alloc(1), 0,
> > > > > > +				       false);
> > > > > > +	if (!array)
> > > > > > +		goto err_fences_put;
> > > > > > +
> > > > > > +	reservation_object_add_excl_fence(obj, &array->base);
> > > > > > +	return 0;
> > > > > > +
> > > > > > +err_fences_put:
> > > > > > +	for (i = 0; i < count; i++)
> > > > > > +		dma_fence_put(fences[i]);
> > > > > > +	kfree(fences);
> > > > > > +	return -ENOMEM;
> > > > > > +}
> > > > > > +
> > > > > >     /**
> > > > > >      * amdgpu_gem_map_attach - &dma_buf_ops.attach implementation
> > > > > >      * @dma_buf: shared DMA buffer
> > > > > > @@ -219,16 +271,16 @@ static int amdgpu_gem_map_attach(struct dma_buf *dma_buf,
> > > > > >     	if (attach->dev->driver != adev->dev->driver) {
> > > > > >     		/*
> > > > > > -		 * Wait for all shared fences to complete before we switch to future
> > > > > > -		 * use of exclusive fence on this prime shared bo.
> > > > > > +		 * We only create shared fences for internal use, but importers
> > > > > > +		 * of the dmabuf rely on exclusive fences for implicitly
> > > > > > +		 * tracking write hazards. As any of the current fences may
> > > > > > +		 * correspond to a write, we need to convert all existing
> > > > > > +		 * fences on the resevation object into a single exclusive
> > > > > > +		 * fence.
> > > > > >     		 */
> > > > > > -		r = reservation_object_wait_timeout_rcu(bo->tbo.resv,
> > > > > > -							true, false,
> > > > > > -							MAX_SCHEDULE_TIMEOUT);
> > > > > > -		if (unlikely(r < 0)) {
> > > > > > -			DRM_DEBUG_PRIME("Fence wait failed: %li\n", r);
> > > > > > +		r = __reservation_object_make_exclusive(bo->tbo.resv);
> > > > > > +		if (r)
> > > > > >     			goto error_unreserve;
> > > > > > -		}
> > > > > >     	}
> > > > > >     	/* pin buffer into GTT */
> > > > > _______________________________________________
> > > > > dri-devel mailing list
> > > > > dri-devel@lists.freedesktop.org
> > > > > https://lists.freedesktop.org/mailman/listinfo/dri-devel
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v3] drm/amdgpu: Transfer fences to dmabuf importer

[Christian König]

Am 07.08.2018 um 15:37 schrieb Daniel Vetter:
> On Tue, Aug 07, 2018 at 03:17:06PM +0200, Christian König wrote:
>> Am 07.08.2018 um 14:59 schrieb Daniel Vetter:
>>> On Tue, Aug 07, 2018 at 02:51:50PM +0200, Christian König wrote:
>>>> Am 07.08.2018 um 14:43 schrieb Daniel Vetter:
>>>>> On Tue, Aug 07, 2018 at 01:23:19PM +0200, Christian König wrote:
>>>>>> Am 07.08.2018 um 13:05 schrieb Chris Wilson:
>>>>>>> amdgpu only uses shared-fences internally, but dmabuf importers rely on
>>>>>>> implicit write hazard tracking via the reservation_object.fence_excl.
>>>>>> Well I would rather suggest a solution where we stop doing this.
>>>>>>
>>>>>> The problem here is that i915 assumes that a write operation always needs
>>>>>> exclusive access to an object protected by an reservation object.
>>>>>>
>>>>>> At least for amdgpu, radeon and nouveau this assumption is incorrect, but
>>>>>> only amdgpu has a design where userspace is not lying to the kernel about
>>>>>> it's read/write access.
>>>>>>
>>>>>> What we should do instead is to add a flag to each shared fence to note if
>>>>>> it is a write operation or not. Then we can trivially add a function to wait
>>>>>> on on those in i915.
>>>>>>
>>>>>> I should have pushed harder for this solution when the problem came up
>>>>>> initially,
>>>>> For shared buffers in an implicit syncing setup like dri2 the exclusive
>>>>> fence _is_ your write fence.
>>>> And exactly that is absolutely not correct if you ask me.
>>>>
>>>> The exclusive fence is for two use cases, the first one is for drivers which
>>>> don't care about concurrent accesses and only use serialized accesses and
>>>> the second is for kernel uses under the hood of userspace, evictions, buffer
>>>> moves etc..
>>>>
>>>> What i915 does is to abuse that concept for write once read many situations,
>>>> and that in turn is the bug we need to fix here.
>>> Again, the exclusive fence was added for implicit sync usage like dri2/3.
>>> _Not_ for your own buffer manager. If you want to separate these two
>>> usages, then I guess we can do that, but claiming that i915 is the odd one
>>> out just aint true. You're arguing against all other kms drivers we have
>>> here.
>> No I'm not. I discussed exactly that use case with Maarten when the
>> reservation object was introduced.
>>
>> I think that Maarten named the explicit fence on purpose "explicit" fence
>> and not "write" fence to make that distinction clear.
>>
>> I have to admit that this wasn't really documented, but it indeed was the
>> original purpose to get away from the idea that writes needs to be
>> exclusive.
>>
>>>>> That's how this stuff works. Note it's only
>>>>> for implicit fencing for dri2 shared buffers. i915 lies as much as
>>>>> everyone else for explicit syncing.
>>>> That is really really ugly and should be fixed instead.
>>> It works and is uapi now. What's the gain in "fixing" it?
>> It allows you to implement explicit fencing in the uapi without breaking
>> backward compatibility.
>>
>> See the situation with amdgpu and intel is the same as with userspace with
>> mixed implicit and explicit fencing.
>>
>> If that isn't fixed we will run into the same problem when DRI3 gets
>> implicit fencing and some applications starts to use it while others still
>> rely on the explicit fencing.
> I think we're a bit cross-eyed on exact semantics, but yes this is exactly
> the use-case. Chris Wilson's use-case tries to emulate exactly what would
> happen if implicitly fenced amdgpu rendered buffer should be displayed on
> an i915 output. Then you need to set the exclusive fence correctly.

Yes, exactly. That's what I totally agree on.

> And yes we called them exclusive/shared because shared fences could also
> be write fences. But for the implicitly synced userspace case, exclusive =
> The write fence.
>
> So probably Chris' patch ends up syncing too much (since for explicit
> syncing you don't want to attach an exclusive fence, because userspace
> passes the fence around already to the atomic ioctl). But at least it's
> correct for the implicit case. But that's entirely an optimization problem
> within amdgpu.

Completely agree as well, but Chris patch actually just optimizes 
things. It is not necessary for correct operation.

See previously we just waited for the BO to be idle, now Chris patch 
collects all fences (shared and exclusive) and adds that as single 
elusive fence to avoid blocking.

> Summary: If you have a shared buffer used in implicitly synced buffer
> sharing, you _must_ set the exclusive fence to cover all write access.

And how should command submission know that?

I mean we add the exclusive or shared fence during command submission, 
but that IOCTL has not the slightest idea if the BO is then used for 
explicit or for implicit fencing.

> In any other case (not shared, or not as part of implicitly synced
> protocol), you can do whatever you want. So we're not conflagrating
> exclusive with write here at all. And yes this is exactly what i915 and
> all other drivers do - for explicit fencing we don't set the exclusive
> fence, but leave that all to userspace. Also, userspace tells us (because
> it knows how the protocol works, not the kernel) when to set an exclusive
> fence.

To extend that at least the lower layers of the user space driver 
doesn't know if we have explicit or implicit fencing either.

The only component who really does know that is the DDX or more general 
the driver instance inside the display server.

And when that component gets the buffer to display it command submission 
should already be long done.

Regards,
Christian.


>   For historical reasons the relevant uapi parts are called
> read/write, but that's just an accident of (maybe too clever) uapi reuse,
> not their actual semantics.
> -Daniel

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v3] drm/amdgpu: Transfer fences to dmabuf importer

[Daniel Vetter]

On Tue, Aug 7, 2018 at 3:54 PM, Christian König
<christian.koenig@amd.com> wrote:
> Am 07.08.2018 um 15:37 schrieb Daniel Vetter:
>>
>> On Tue, Aug 07, 2018 at 03:17:06PM +0200, Christian König wrote:
>>>
>>> Am 07.08.2018 um 14:59 schrieb Daniel Vetter:
>>>>
>>>> On Tue, Aug 07, 2018 at 02:51:50PM +0200, Christian König wrote:
>>>>>
>>>>> Am 07.08.2018 um 14:43 schrieb Daniel Vetter:
>>>>>>
>>>>>> On Tue, Aug 07, 2018 at 01:23:19PM +0200, Christian König wrote:
>>>>>>>
>>>>>>> Am 07.08.2018 um 13:05 schrieb Chris Wilson:
>>>>>>>>
>>>>>>>> amdgpu only uses shared-fences internally, but dmabuf importers rely
>>>>>>>> on
>>>>>>>> implicit write hazard tracking via the
>>>>>>>> reservation_object.fence_excl.
>>>>>>>
>>>>>>> Well I would rather suggest a solution where we stop doing this.
>>>>>>>
>>>>>>> The problem here is that i915 assumes that a write operation always
>>>>>>> needs
>>>>>>> exclusive access to an object protected by an reservation object.
>>>>>>>
>>>>>>> At least for amdgpu, radeon and nouveau this assumption is incorrect,
>>>>>>> but
>>>>>>> only amdgpu has a design where userspace is not lying to the kernel
>>>>>>> about
>>>>>>> it's read/write access.
>>>>>>>
>>>>>>> What we should do instead is to add a flag to each shared fence to
>>>>>>> note if
>>>>>>> it is a write operation or not. Then we can trivially add a function
>>>>>>> to wait
>>>>>>> on on those in i915.
>>>>>>>
>>>>>>> I should have pushed harder for this solution when the problem came
>>>>>>> up
>>>>>>> initially,
>>>>>>
>>>>>> For shared buffers in an implicit syncing setup like dri2 the
>>>>>> exclusive
>>>>>> fence _is_ your write fence.
>>>>>
>>>>> And exactly that is absolutely not correct if you ask me.
>>>>>
>>>>> The exclusive fence is for two use cases, the first one is for drivers
>>>>> which
>>>>> don't care about concurrent accesses and only use serialized accesses
>>>>> and
>>>>> the second is for kernel uses under the hood of userspace, evictions,
>>>>> buffer
>>>>> moves etc..
>>>>>
>>>>> What i915 does is to abuse that concept for write once read many
>>>>> situations,
>>>>> and that in turn is the bug we need to fix here.
>>>>
>>>> Again, the exclusive fence was added for implicit sync usage like
>>>> dri2/3.
>>>> _Not_ for your own buffer manager. If you want to separate these two
>>>> usages, then I guess we can do that, but claiming that i915 is the odd
>>>> one
>>>> out just aint true. You're arguing against all other kms drivers we have
>>>> here.
>>>
>>> No I'm not. I discussed exactly that use case with Maarten when the
>>> reservation object was introduced.
>>>
>>> I think that Maarten named the explicit fence on purpose "explicit" fence
>>> and not "write" fence to make that distinction clear.
>>>
>>> I have to admit that this wasn't really documented, but it indeed was the
>>> original purpose to get away from the idea that writes needs to be
>>> exclusive.
>>>
>>>>>> That's how this stuff works. Note it's only
>>>>>> for implicit fencing for dri2 shared buffers. i915 lies as much as
>>>>>> everyone else for explicit syncing.
>>>>>
>>>>> That is really really ugly and should be fixed instead.
>>>>
>>>> It works and is uapi now. What's the gain in "fixing" it?
>>>
>>> It allows you to implement explicit fencing in the uapi without breaking
>>> backward compatibility.
>>>
>>> See the situation with amdgpu and intel is the same as with userspace
>>> with
>>> mixed implicit and explicit fencing.
>>>
>>> If that isn't fixed we will run into the same problem when DRI3 gets
>>> implicit fencing and some applications starts to use it while others
>>> still
>>> rely on the explicit fencing.
>>
>> I think we're a bit cross-eyed on exact semantics, but yes this is exactly
>> the use-case. Chris Wilson's use-case tries to emulate exactly what would
>> happen if implicitly fenced amdgpu rendered buffer should be displayed on
>> an i915 output. Then you need to set the exclusive fence correctly.
>
>
> Yes, exactly. That's what I totally agree on.
>
>> And yes we called them exclusive/shared because shared fences could also
>> be write fences. But for the implicitly synced userspace case, exclusive =
>> The write fence.
>>
>> So probably Chris' patch ends up syncing too much (since for explicit
>> syncing you don't want to attach an exclusive fence, because userspace
>> passes the fence around already to the atomic ioctl). But at least it's
>> correct for the implicit case. But that's entirely an optimization problem
>> within amdgpu.
>
>
> Completely agree as well, but Chris patch actually just optimizes things. It
> is not necessary for correct operation.

Duh, I didn't read the patch carefully enough ...

> See previously we just waited for the BO to be idle, now Chris patch
> collects all fences (shared and exclusive) and adds that as single elusive
> fence to avoid blocking.
>
>> Summary: If you have a shared buffer used in implicitly synced buffer
>> sharing, you _must_ set the exclusive fence to cover all write access.
>
>
> And how should command submission know that?
>
> I mean we add the exclusive or shared fence during command submission, but
> that IOCTL has not the slightest idea if the BO is then used for explicit or
> for implicit fencing.

The ioctl doesn't know, but the winsys in userspace does know. Well,
with one exception: Bare metal egl on gbm or similar doesn't, so there
the heuristics is that we assume implicit fencing until userspace
starts using the explicit fence extensions. Then we switch over.

And once your winsys knows whether shared buffers should be implicit
or explicit synced, it can tell the kernel. Either through a new flag,
our by retroshoehorning the semantics into existing flags. The latter
is what we've done for i915 and freedrone afaiui.

>> In any other case (not shared, or not as part of implicitly synced
>> protocol), you can do whatever you want. So we're not conflagrating
>> exclusive with write here at all. And yes this is exactly what i915 and
>> all other drivers do - for explicit fencing we don't set the exclusive
>> fence, but leave that all to userspace. Also, userspace tells us (because
>> it knows how the protocol works, not the kernel) when to set an exclusive
>> fence.
>
>
> To extend that at least the lower layers of the user space driver doesn't
> know if we have explicit or implicit fencing either.
>
> The only component who really does know that is the DDX or more general the
> driver instance inside the display server.
>
> And when that component gets the buffer to display it command submission
> should already be long done.

Why does only the DDX know? If you're running gl on GLX your driver
knows how to sync the shared buffer - it also knows how to hand it
over to the DDX after all. Same for gles on android/surfaceflinger,
you know it's going to be explicit sync for shared buffers.

There's some cases where you can't decide this right away, but a
context flag that enables explicit sync once that's clear seemed to be
all that's needed. Worst case the first frame is artifially limited
due to the implicit fencing hurting a bit. And in general that issue
is only for bare metal buffers, i.e. your compositor takes a small hit
once at startup. If this is a real issue we could add a gbm interface
to select implict/explicit before we start allocating anything.

Again this is only for shared buffers, anything private to a given
context can be handled however you want really.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

[PATCH v4] drm/amdgpu: Transfer fences to dmabuf importer

[Chris Wilson]

amdgpu only uses shared-fences internally, but dmabuf importers rely on
implicit write hazard tracking via the reservation_object.fence_excl.
For example, the importer use the write hazard for timing a page flip to
only occur after the exporter has finished flushing its write into the
surface. As such, on exporting a dmabuf, we must either flush all
outstanding fences (for we do not know which are writes and should have
been exclusive) or alternatively create a new exclusive fence that is
the composite of all the existing shared fences, and so will only be
signaled when all earlier fences are signaled (ensuring that we can not
be signaled before the completion of any earlier write).

v2: reservation_object is already locked by amdgpu_bo_reserve()

Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=107341
Testcase: igt/amd_prime/amd-to-i915
References: 8e94a46c1770 ("drm/amdgpu: Attach exclusive fence to prime exported bo's. (v5)")
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Alex Deucher <alexander.deucher@amd.com>
Cc: "Christian König" <christian.koenig@amd.com>
---

This time, hopefully proofread and references complete.
-Chris

---
 drivers/gpu/drm/amd/amdgpu/amdgpu_prime.c | 68 ++++++++++++++++++++---
 1 file changed, 60 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_prime.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_prime.c
index 1c5d97f4b4dd..dff2b01a3d89 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_prime.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_prime.c
@@ -37,6 +37,7 @@
 #include "amdgpu_display.h"
 #include <drm/amdgpu_drm.h>
 #include <linux/dma-buf.h>
+#include <linux/dma-fence-array.h>
 
 static const struct dma_buf_ops amdgpu_dmabuf_ops;
 
@@ -188,6 +189,57 @@ amdgpu_gem_prime_import_sg_table(struct drm_device *dev,
 	return ERR_PTR(ret);
 }
 
+static int
+__reservation_object_make_exclusive(struct reservation_object *obj)
+{
+	struct reservation_object_list *fobj;
+	struct dma_fence_array *array;
+	struct dma_fence **fences;
+	unsigned int count, i;
+
+	fobj = reservation_object_get_list(obj);
+	if (!fobj)
+		return 0;
+
+	count = !!rcu_access_pointer(obj->fence_excl);
+	count += fobj->shared_count;
+
+	fences = kmalloc_array(count, sizeof(*fences), GFP_KERNEL);
+	if (!fences)
+		return -ENOMEM;
+
+	for (i = 0; i < fobj->shared_count; i++) {
+		struct dma_fence *f =
+			rcu_dereference_protected(fobj->shared[i],
+						  reservation_object_held(obj));
+
+		fences[i] = dma_fence_get(f);
+	}
+
+	if (rcu_access_pointer(obj->fence_excl)) {
+		struct dma_fence *f =
+			rcu_dereference_protected(obj->fence_excl,
+						  reservation_object_held(obj));
+
+		fences[i] = dma_fence_get(f);
+	}
+
+	array = dma_fence_array_create(count, fences,
+				       dma_fence_context_alloc(1), 0,
+				       false);
+	if (!array)
+		goto err_fences_put;
+
+	reservation_object_add_excl_fence(obj, &array->base);
+	return 0;
+
+err_fences_put:
+	for (i = 0; i < count; i++)
+		dma_fence_put(fences[i]);
+	kfree(fences);
+	return -ENOMEM;
+}
+
 /**
  * amdgpu_gem_map_attach - &dma_buf_ops.attach implementation
  * @dma_buf: shared DMA buffer
@@ -219,16 +271,16 @@ static int amdgpu_gem_map_attach(struct dma_buf *dma_buf,
 
 	if (attach->dev->driver != adev->dev->driver) {
 		/*
-		 * Wait for all shared fences to complete before we switch to future
-		 * use of exclusive fence on this prime shared bo.
+		 * We only create shared fences for internal use, but importers
+		 * of the dmabuf rely on exclusive fences for implicitly
+		 * tracking write hazards. As any of the current fences may
+		 * correspond to a write, we need to convert all existing
+		 * fences on the reservation object into a single exclusive
+		 * fence.
 		 */
-		r = reservation_object_wait_timeout_rcu(bo->tbo.resv,
-							true, false,
-							MAX_SCHEDULE_TIMEOUT);
-		if (unlikely(r < 0)) {
-			DRM_DEBUG_PRIME("Fence wait failed: %li\n", r);
+		r = __reservation_object_make_exclusive(bo->tbo.resv);
+		if (r)
 			goto error_unreserve;
-		}
 	}
 
 	/* pin buffer into GTT */
-- 
2.18.0

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

✗ Fi.CI.CHECKPATCH: warning for drm/amdgpu: Transfer fences to dmabuf importer (rev4)

[Patchwork]

== Series Details ==

Series: drm/amdgpu: Transfer fences to dmabuf importer (rev4)
URL   : https://patchwork.freedesktop.org/series/47803/
State : warning

== Summary ==

$ dim checkpatch origin/drm-tip
2d40cca5a9be drm/amdgpu: Transfer fences to dmabuf importer
-:24: WARNING:COMMIT_LOG_LONG_LINE: Possible unwrapped commit description (prefer a maximum 75 chars per line)
#24: 
References: 8e94a46c1770 ("drm/amdgpu: Attach exclusive fence to prime exported bo's. (v5)")

-:24: ERROR:GIT_COMMIT_ID: Please use git commit description style 'commit <12+ chars of sha1> ("<title line>")' - ie: 'commit 8e94a46c1770 ("drm/amdgpu: Attach exclusive fence to prime exported bo's. (v5)")'
#24: 
References: 8e94a46c1770 ("drm/amdgpu: Attach exclusive fence to prime exported bo's. (v5)")

total: 1 errors, 1 warnings, 0 checks, 88 lines checked

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✗ Fi.CI.SPARSE: warning for drm/amdgpu: Transfer fences to dmabuf importer (rev4)

[Patchwork]

== Series Details ==

Series: drm/amdgpu: Transfer fences to dmabuf importer (rev4)
URL   : https://patchwork.freedesktop.org/series/47803/
State : warning

== Summary ==

$ dim sparse origin/drm-tip
Commit: drm/amdgpu: Transfer fences to dmabuf importer
-
+./include/linux/slab.h:631:13: error: undefined identifier '__builtin_mul_overflow'
+./include/linux/slab.h:631:13: warning: call with no type!

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v4] drm/amdgpu: Transfer fences to dmabuf importer

[Christian König]

Am 07.08.2018 um 18:08 schrieb Chris Wilson:
> amdgpu only uses shared-fences internally, but dmabuf importers rely on
> implicit write hazard tracking via the reservation_object.fence_excl.
> For example, the importer use the write hazard for timing a page flip to
> only occur after the exporter has finished flushing its write into the
> surface. As such, on exporting a dmabuf, we must either flush all
> outstanding fences (for we do not know which are writes and should have
> been exclusive) or alternatively create a new exclusive fence that is
> the composite of all the existing shared fences, and so will only be
> signaled when all earlier fences are signaled (ensuring that we can not
> be signaled before the completion of any earlier write).
>
> v2: reservation_object is already locked by amdgpu_bo_reserve()
>
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=107341
> Testcase: igt/amd_prime/amd-to-i915
> References: 8e94a46c1770 ("drm/amdgpu: Attach exclusive fence to prime exported bo's. (v5)")
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Alex Deucher <alexander.deucher@amd.com>
> Cc: "Christian König" <christian.koenig@amd.com>
> ---
>
> This time, hopefully proofread and references complete.
> -Chris
>
> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_prime.c | 68 ++++++++++++++++++++---
>   1 file changed, 60 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_prime.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_prime.c
> index 1c5d97f4b4dd..dff2b01a3d89 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_prime.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_prime.c
> @@ -37,6 +37,7 @@
>   #include "amdgpu_display.h"
>   #include <drm/amdgpu_drm.h>
>   #include <linux/dma-buf.h>
> +#include <linux/dma-fence-array.h>
>   
>   static const struct dma_buf_ops amdgpu_dmabuf_ops;
>   
> @@ -188,6 +189,57 @@ amdgpu_gem_prime_import_sg_table(struct drm_device *dev,
>   	return ERR_PTR(ret);
>   }
>   
> +static int
> +__reservation_object_make_exclusive(struct reservation_object *obj)
> +{
> +	struct reservation_object_list *fobj;
> +	struct dma_fence_array *array;
> +	struct dma_fence **fences;
> +	unsigned int count, i;
> +
> +	fobj = reservation_object_get_list(obj);
> +	if (!fobj)
> +		return 0;
> +
> +	count = !!rcu_access_pointer(obj->fence_excl);
> +	count += fobj->shared_count;
> +
> +	fences = kmalloc_array(count, sizeof(*fences), GFP_KERNEL);
> +	if (!fences)
> +		return -ENOMEM;
> +
> +	for (i = 0; i < fobj->shared_count; i++) {
> +		struct dma_fence *f =
> +			rcu_dereference_protected(fobj->shared[i],
> +						  reservation_object_held(obj));
> +
> +		fences[i] = dma_fence_get(f);
> +	}
> +
> +	if (rcu_access_pointer(obj->fence_excl)) {
> +		struct dma_fence *f =
> +			rcu_dereference_protected(obj->fence_excl,
> +						  reservation_object_held(obj));
> +
> +		fences[i] = dma_fence_get(f);
> +	}
> +
> +	array = dma_fence_array_create(count, fences,
> +				       dma_fence_context_alloc(1), 0,
> +				       false);
> +	if (!array)
> +		goto err_fences_put;
> +
> +	reservation_object_add_excl_fence(obj, &array->base);
> +	return 0;
> +
> +err_fences_put:
> +	for (i = 0; i < count; i++)
> +		dma_fence_put(fences[i]);
> +	kfree(fences);
> +	return -ENOMEM;
> +}
> +

This can be simplified a lot. See amdgpu_pasid_free_delayed() for an 
example:
>         r = reservation_object_get_fences_rcu(resv, NULL, &count, 
> &fences);
>         if (r)
>                 goto fallback;
>
>         if (count == 0) {
>                 amdgpu_pasid_free(pasid);
>                 return;
>         }
>
>         if (count == 1) {
>                 fence = fences[0];
>                 kfree(fences);
>         } else {
>                 uint64_t context = dma_fence_context_alloc(1);
>                 struct dma_fence_array *array;
>
>                 array = dma_fence_array_create(count, fences, context,
>                                                1, false);
>                 if (!array) {
>                         kfree(fences);
>                         goto fallback;
>                 }
>                 fence = &array->base;
>         }


Regards,
Christian.

>   /**
>    * amdgpu_gem_map_attach - &dma_buf_ops.attach implementation
>    * @dma_buf: shared DMA buffer
> @@ -219,16 +271,16 @@ static int amdgpu_gem_map_attach(struct dma_buf *dma_buf,
>   
>   	if (attach->dev->driver != adev->dev->driver) {
>   		/*
> -		 * Wait for all shared fences to complete before we switch to future
> -		 * use of exclusive fence on this prime shared bo.
> +		 * We only create shared fences for internal use, but importers
> +		 * of the dmabuf rely on exclusive fences for implicitly
> +		 * tracking write hazards. As any of the current fences may
> +		 * correspond to a write, we need to convert all existing
> +		 * fences on the reservation object into a single exclusive
> +		 * fence.
>   		 */
> -		r = reservation_object_wait_timeout_rcu(bo->tbo.resv,
> -							true, false,
> -							MAX_SCHEDULE_TIMEOUT);
> -		if (unlikely(r < 0)) {
> -			DRM_DEBUG_PRIME("Fence wait failed: %li\n", r);
> +		r = __reservation_object_make_exclusive(bo->tbo.resv);
> +		if (r)
>   			goto error_unreserve;
> -		}
>   	}
>   
>   	/* pin buffer into GTT */

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH v4] drm/amdgpu: Transfer fences to dmabuf importer

[Chris Wilson]

Quoting Christian König (2018-08-07 18:57:16)
> Am 07.08.2018 um 18:08 schrieb Chris Wilson:
> > amdgpu only uses shared-fences internally, but dmabuf importers rely on
> > implicit write hazard tracking via the reservation_object.fence_excl.
> > For example, the importer use the write hazard for timing a page flip to
> > only occur after the exporter has finished flushing its write into the
> > surface. As such, on exporting a dmabuf, we must either flush all
> > outstanding fences (for we do not know which are writes and should have
> > been exclusive) or alternatively create a new exclusive fence that is
> > the composite of all the existing shared fences, and so will only be
> > signaled when all earlier fences are signaled (ensuring that we can not
> > be signaled before the completion of any earlier write).
> >
> > v2: reservation_object is already locked by amdgpu_bo_reserve()
> >
> > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=107341
> > Testcase: igt/amd_prime/amd-to-i915
> > References: 8e94a46c1770 ("drm/amdgpu: Attach exclusive fence to prime exported bo's. (v5)")
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > Cc: Alex Deucher <alexander.deucher@amd.com>
> > Cc: "Christian König" <christian.koenig@amd.com>
> > ---
> >
> > This time, hopefully proofread and references complete.
> > -Chris
> >
> > ---
> >   drivers/gpu/drm/amd/amdgpu/amdgpu_prime.c | 68 ++++++++++++++++++++---
> >   1 file changed, 60 insertions(+), 8 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_prime.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_prime.c
> > index 1c5d97f4b4dd..dff2b01a3d89 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_prime.c
> > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_prime.c
> > @@ -37,6 +37,7 @@
> >   #include "amdgpu_display.h"
> >   #include <drm/amdgpu_drm.h>
> >   #include <linux/dma-buf.h>
> > +#include <linux/dma-fence-array.h>
> >   
> >   static const struct dma_buf_ops amdgpu_dmabuf_ops;
> >   
> > @@ -188,6 +189,57 @@ amdgpu_gem_prime_import_sg_table(struct drm_device *dev,
> >       return ERR_PTR(ret);
> >   }
> >   
> > +static int
> > +__reservation_object_make_exclusive(struct reservation_object *obj)
> > +{
> > +     struct reservation_object_list *fobj;
> > +     struct dma_fence_array *array;
> > +     struct dma_fence **fences;
> > +     unsigned int count, i;
> > +
> > +     fobj = reservation_object_get_list(obj);
> > +     if (!fobj)
> > +             return 0;
> > +
> > +     count = !!rcu_access_pointer(obj->fence_excl);
> > +     count += fobj->shared_count;
> > +
> > +     fences = kmalloc_array(count, sizeof(*fences), GFP_KERNEL);
> > +     if (!fences)
> > +             return -ENOMEM;
> > +
> > +     for (i = 0; i < fobj->shared_count; i++) {
> > +             struct dma_fence *f =
> > +                     rcu_dereference_protected(fobj->shared[i],
> > +                                               reservation_object_held(obj));
> > +
> > +             fences[i] = dma_fence_get(f);
> > +     }
> > +
> > +     if (rcu_access_pointer(obj->fence_excl)) {
> > +             struct dma_fence *f =
> > +                     rcu_dereference_protected(obj->fence_excl,
> > +                                               reservation_object_held(obj));
> > +
> > +             fences[i] = dma_fence_get(f);
> > +     }
> > +
> > +     array = dma_fence_array_create(count, fences,
> > +                                    dma_fence_context_alloc(1), 0,
> > +                                    false);
> > +     if (!array)
> > +             goto err_fences_put;
> > +
> > +     reservation_object_add_excl_fence(obj, &array->base);
> > +     return 0;
> > +
> > +err_fences_put:
> > +     for (i = 0; i < count; i++)
> > +             dma_fence_put(fences[i]);
> > +     kfree(fences);
> > +     return -ENOMEM;
> > +}
> > +
> 
> This can be simplified a lot. See amdgpu_pasid_free_delayed() for an 
> example:

{
	if (!reservation_object_get_list(obj))
		return 0;

	r = reservation_object_get_fences_rcu(obj, NULL, &count, &fences);
	if (r)
		return r;

	array = dma_fence_array_create(count, fences,
				       dma_fence_context_alloc(1), 0,
				       false);
	if (!array)
		goto err_fences_put;

	reservation_object_add_excl_fence(obj, &array->base);
	return 0;

err:
	...
}

My starting point was going to be use get_fences_rcu, but get_fences_rcu
can hardly be called simple for where the lock is required to be held
start to finish ;)
-Chris
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH v4] drm/amdgpu: Transfer fences to dmabuf importer

[Christian König]

Am 07.08.2018 um 20:14 schrieb Chris Wilson:
> Quoting Christian König (2018-08-07 18:57:16)
>> Am 07.08.2018 um 18:08 schrieb Chris Wilson:
>>> amdgpu only uses shared-fences internally, but dmabuf importers rely on
>>> implicit write hazard tracking via the reservation_object.fence_excl.
>>> For example, the importer use the write hazard for timing a page flip to
>>> only occur after the exporter has finished flushing its write into the
>>> surface. As such, on exporting a dmabuf, we must either flush all
>>> outstanding fences (for we do not know which are writes and should have
>>> been exclusive) or alternatively create a new exclusive fence that is
>>> the composite of all the existing shared fences, and so will only be
>>> signaled when all earlier fences are signaled (ensuring that we can not
>>> be signaled before the completion of any earlier write).
>>>
>>> v2: reservation_object is already locked by amdgpu_bo_reserve()
>>>
>>> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=107341
>>> Testcase: igt/amd_prime/amd-to-i915
>>> References: 8e94a46c1770 ("drm/amdgpu: Attach exclusive fence to prime exported bo's. (v5)")
>>> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
>>> Cc: Alex Deucher <alexander.deucher@amd.com>
>>> Cc: "Christian König" <christian.koenig@amd.com>
>>> ---
>>>
>>> This time, hopefully proofread and references complete.
>>> -Chris
>>>
>>> ---
>>>    drivers/gpu/drm/amd/amdgpu/amdgpu_prime.c | 68 ++++++++++++++++++++---
>>>    1 file changed, 60 insertions(+), 8 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_prime.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_prime.c
>>> index 1c5d97f4b4dd..dff2b01a3d89 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_prime.c
>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_prime.c
>>> @@ -37,6 +37,7 @@
>>>    #include "amdgpu_display.h"
>>>    #include <drm/amdgpu_drm.h>
>>>    #include <linux/dma-buf.h>
>>> +#include <linux/dma-fence-array.h>
>>>    
>>>    static const struct dma_buf_ops amdgpu_dmabuf_ops;
>>>    
>>> @@ -188,6 +189,57 @@ amdgpu_gem_prime_import_sg_table(struct drm_device *dev,
>>>        return ERR_PTR(ret);
>>>    }
>>>    
>>> +static int
>>> +__reservation_object_make_exclusive(struct reservation_object *obj)
>>> +{
>>> +     struct reservation_object_list *fobj;
>>> +     struct dma_fence_array *array;
>>> +     struct dma_fence **fences;
>>> +     unsigned int count, i;
>>> +
>>> +     fobj = reservation_object_get_list(obj);
>>> +     if (!fobj)
>>> +             return 0;
>>> +
>>> +     count = !!rcu_access_pointer(obj->fence_excl);
>>> +     count += fobj->shared_count;
>>> +
>>> +     fences = kmalloc_array(count, sizeof(*fences), GFP_KERNEL);
>>> +     if (!fences)
>>> +             return -ENOMEM;
>>> +
>>> +     for (i = 0; i < fobj->shared_count; i++) {
>>> +             struct dma_fence *f =
>>> +                     rcu_dereference_protected(fobj->shared[i],
>>> +                                               reservation_object_held(obj));
>>> +
>>> +             fences[i] = dma_fence_get(f);
>>> +     }
>>> +
>>> +     if (rcu_access_pointer(obj->fence_excl)) {
>>> +             struct dma_fence *f =
>>> +                     rcu_dereference_protected(obj->fence_excl,
>>> +                                               reservation_object_held(obj));
>>> +
>>> +             fences[i] = dma_fence_get(f);
>>> +     }
>>> +
>>> +     array = dma_fence_array_create(count, fences,
>>> +                                    dma_fence_context_alloc(1), 0,
>>> +                                    false);
>>> +     if (!array)
>>> +             goto err_fences_put;
>>> +
>>> +     reservation_object_add_excl_fence(obj, &array->base);
>>> +     return 0;
>>> +
>>> +err_fences_put:
>>> +     for (i = 0; i < count; i++)
>>> +             dma_fence_put(fences[i]);
>>> +     kfree(fences);
>>> +     return -ENOMEM;
>>> +}
>>> +
>> This can be simplified a lot. See amdgpu_pasid_free_delayed() for an
>> example:
> {
> 	if (!reservation_object_get_list(obj))
> 		return 0;
>
> 	r = reservation_object_get_fences_rcu(obj, NULL, &count, &fences);
> 	if (r)
> 		return r;
>
> 	array = dma_fence_array_create(count, fences,
> 				       dma_fence_context_alloc(1), 0,
> 				       false);
> 	if (!array)
> 		goto err_fences_put;
>
> 	reservation_object_add_excl_fence(obj, &array->base);
> 	return 0;
>
> err:
> 	...
> }
>
> My starting point was going to be use get_fences_rcu, but get_fences_rcu
> can hardly be called simple for where the lock is required to be held
> start to finish ;)

What are you talking about? get_fences_rcu doesn't require any locking 
at all.

You only need to the locking to make sure that between creating the 
fence array and calling reservation_object_add_excl_fence() no other 
fence is added.

Christian.

> -Chris
> _______________________________________________
> amd-gfx mailing list
> amd-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH v4] drm/amdgpu: Transfer fences to dmabuf importer

[Chris Wilson]

Quoting Christian König (2018-08-07 19:18:55)
> Am 07.08.2018 um 20:14 schrieb Chris Wilson:
> > Quoting Christian König (2018-08-07 18:57:16)
> >> Am 07.08.2018 um 18:08 schrieb Chris Wilson:
> >>> amdgpu only uses shared-fences internally, but dmabuf importers rely on
> >>> implicit write hazard tracking via the reservation_object.fence_excl.
> >>> For example, the importer use the write hazard for timing a page flip to
> >>> only occur after the exporter has finished flushing its write into the
> >>> surface. As such, on exporting a dmabuf, we must either flush all
> >>> outstanding fences (for we do not know which are writes and should have
> >>> been exclusive) or alternatively create a new exclusive fence that is
> >>> the composite of all the existing shared fences, and so will only be
> >>> signaled when all earlier fences are signaled (ensuring that we can not
> >>> be signaled before the completion of any earlier write).
> >>>
> >>> v2: reservation_object is already locked by amdgpu_bo_reserve()
> >>>
> >>> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=107341
> >>> Testcase: igt/amd_prime/amd-to-i915
> >>> References: 8e94a46c1770 ("drm/amdgpu: Attach exclusive fence to prime exported bo's. (v5)")
> >>> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> >>> Cc: Alex Deucher <alexander.deucher@amd.com>
> >>> Cc: "Christian König" <christian.koenig@amd.com>
> >>> ---
> >>>
> >>> This time, hopefully proofread and references complete.
> >>> -Chris
> >>>
> >>> ---
> >>>    drivers/gpu/drm/amd/amdgpu/amdgpu_prime.c | 68 ++++++++++++++++++++---
> >>>    1 file changed, 60 insertions(+), 8 deletions(-)
> >>>
> >>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_prime.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_prime.c
> >>> index 1c5d97f4b4dd..dff2b01a3d89 100644
> >>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_prime.c
> >>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_prime.c
> >>> @@ -37,6 +37,7 @@
> >>>    #include "amdgpu_display.h"
> >>>    #include <drm/amdgpu_drm.h>
> >>>    #include <linux/dma-buf.h>
> >>> +#include <linux/dma-fence-array.h>
> >>>    
> >>>    static const struct dma_buf_ops amdgpu_dmabuf_ops;
> >>>    
> >>> @@ -188,6 +189,57 @@ amdgpu_gem_prime_import_sg_table(struct drm_device *dev,
> >>>        return ERR_PTR(ret);
> >>>    }
> >>>    
> >>> +static int
> >>> +__reservation_object_make_exclusive(struct reservation_object *obj)
> >>> +{
> >>> +     struct reservation_object_list *fobj;
> >>> +     struct dma_fence_array *array;
> >>> +     struct dma_fence **fences;
> >>> +     unsigned int count, i;
> >>> +
> >>> +     fobj = reservation_object_get_list(obj);
> >>> +     if (!fobj)
> >>> +             return 0;
> >>> +
> >>> +     count = !!rcu_access_pointer(obj->fence_excl);
> >>> +     count += fobj->shared_count;
> >>> +
> >>> +     fences = kmalloc_array(count, sizeof(*fences), GFP_KERNEL);
> >>> +     if (!fences)
> >>> +             return -ENOMEM;
> >>> +
> >>> +     for (i = 0; i < fobj->shared_count; i++) {
> >>> +             struct dma_fence *f =
> >>> +                     rcu_dereference_protected(fobj->shared[i],
> >>> +                                               reservation_object_held(obj));
> >>> +
> >>> +             fences[i] = dma_fence_get(f);
> >>> +     }
> >>> +
> >>> +     if (rcu_access_pointer(obj->fence_excl)) {
> >>> +             struct dma_fence *f =
> >>> +                     rcu_dereference_protected(obj->fence_excl,
> >>> +                                               reservation_object_held(obj));
> >>> +
> >>> +             fences[i] = dma_fence_get(f);
> >>> +     }
> >>> +
> >>> +     array = dma_fence_array_create(count, fences,
> >>> +                                    dma_fence_context_alloc(1), 0,
> >>> +                                    false);
> >>> +     if (!array)
> >>> +             goto err_fences_put;
> >>> +
> >>> +     reservation_object_add_excl_fence(obj, &array->base);
> >>> +     return 0;
> >>> +
> >>> +err_fences_put:
> >>> +     for (i = 0; i < count; i++)
> >>> +             dma_fence_put(fences[i]);
> >>> +     kfree(fences);
> >>> +     return -ENOMEM;
> >>> +}
> >>> +
> >> This can be simplified a lot. See amdgpu_pasid_free_delayed() for an
> >> example:
> > {
> >       if (!reservation_object_get_list(obj))
> >               return 0;
> >
> >       r = reservation_object_get_fences_rcu(obj, NULL, &count, &fences);
> >       if (r)
> >               return r;
> >
> >       array = dma_fence_array_create(count, fences,
> >                                      dma_fence_context_alloc(1), 0,
> >                                      false);
> >       if (!array)
> >               goto err_fences_put;
> >
> >       reservation_object_add_excl_fence(obj, &array->base);
> >       return 0;
> >
> > err:
> >       ...
> > }
> >
> > My starting point was going to be use get_fences_rcu, but get_fences_rcu
> > can hardly be called simple for where the lock is required to be held
> > start to finish ;)
> 
> What are you talking about? get_fences_rcu doesn't require any locking 
> at all.
> 
> You only need to the locking to make sure that between creating the 
> fence array and calling reservation_object_add_excl_fence() no other 
> fence is added.

Exactly. That's what need to be absolutely clear from the context.
I didn't say anything about the locking requirements for get_fences_rcu
just the opposite.
-Chris
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

[PATCH v5] drm/amdgpu: Transfer fences to dmabuf importer

[Chris Wilson]

amdgpu only uses shared-fences internally, but dmabuf importers rely on
implicit write hazard tracking via the reservation_object.fence_excl.
For example, the importer use the write hazard for timing a page flip to
only occur after the exporter has finished flushing its write into the
surface. As such, on exporting a dmabuf, we must either flush all
outstanding fences (for we do not know which are writes and should have
been exclusive) or alternatively create a new exclusive fence that is
the composite of all the existing shared fences, and so will only be
signaled when all earlier fences are signaled (ensuring that we can not
be signaled before the completion of any earlier write).

v2: reservation_object is already locked by amdgpu_bo_reserve()
v3: Replace looping with get_fences_rcu and special case the promotion
of a single shared fence directly to an exclusive fence, bypassing the
fence array.

Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=107341
Testcase: igt/amd_prime/amd-to-i915
References: 8e94a46c1770 ("drm/amdgpu: Attach exclusive fence to prime exported bo's. (v5)")
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Alex Deucher <alexander.deucher@amd.com>
Cc: "Christian König" <christian.koenig@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_prime.c | 58 +++++++++++++++++++----
 1 file changed, 50 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_prime.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_prime.c
index 3fdd5688da0b..06a310b5b07b 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_prime.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_prime.c
@@ -37,6 +37,7 @@
 #include "amdgpu_display.h"
 #include <drm/amdgpu_drm.h>
 #include <linux/dma-buf.h>
+#include <linux/dma-fence-array.h>
 
 static const struct dma_buf_ops amdgpu_dmabuf_ops;
 
@@ -188,6 +189,47 @@ amdgpu_gem_prime_import_sg_table(struct drm_device *dev,
 	return ERR_PTR(ret);
 }
 
+static int
+__reservation_object_make_exclusive(struct reservation_object *obj)
+{
+	struct dma_fence **fences;
+	unsigned int count;
+	int r;
+
+	if (!reservation_object_get_list(obj)) /* no shared fences to convert */
+		return 0;
+
+	r = reservation_object_get_fences_rcu(obj, NULL, &count, &fences);
+	if (r)
+		return r;
+
+	if (count == 0) {
+		/* Now that was unexpected. */
+	} else if (count == 1) {
+		reservation_object_add_excl_fence(obj, fences[0]);
+		dma_fence_put(fences[0]);
+		kfree(fences);
+	} else {
+		struct dma_fence_array *array;
+
+		array = dma_fence_array_create(count, fences,
+					       dma_fence_context_alloc(1), 0,
+					       false);
+		if (!array)
+			goto err_fences_put;
+
+		reservation_object_add_excl_fence(obj, &array->base);
+	}
+
+	return 0;
+
+err_fences_put:
+	while (count--)
+		dma_fence_put(fences[count]);
+	kfree(fences);
+	return -ENOMEM;
+}
+
 /**
  * amdgpu_gem_map_attach - &dma_buf_ops.attach implementation
  * @dma_buf: shared DMA buffer
@@ -219,16 +261,16 @@ static int amdgpu_gem_map_attach(struct dma_buf *dma_buf,
 
 	if (attach->dev->driver != adev->dev->driver) {
 		/*
-		 * Wait for all shared fences to complete before we switch to future
-		 * use of exclusive fence on this prime shared bo.
+		 * We only create shared fences for internal use, but importers
+		 * of the dmabuf rely on exclusive fences for implicitly
+		 * tracking write hazards. As any of the current fences may
+		 * correspond to a write, we need to convert all existing
+		 * fences on the reservation object into a single exclusive
+		 * fence.
 		 */
-		r = reservation_object_wait_timeout_rcu(bo->tbo.resv,
-							true, false,
-							MAX_SCHEDULE_TIMEOUT);
-		if (unlikely(r < 0)) {
-			DRM_DEBUG_PRIME("Fence wait failed: %li\n", r);
+		r = __reservation_object_make_exclusive(bo->tbo.resv);
+		if (r)
 			goto error_unreserve;
-		}
 	}
 
 	/* pin buffer into GTT */
-- 
2.18.0

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v5] drm/amdgpu: Transfer fences to dmabuf importer

[Christian König]

Am 07.08.2018 um 20:32 schrieb Chris Wilson:
> amdgpu only uses shared-fences internally, but dmabuf importers rely on
> implicit write hazard tracking via the reservation_object.fence_excl.
> For example, the importer use the write hazard for timing a page flip to
> only occur after the exporter has finished flushing its write into the
> surface. As such, on exporting a dmabuf, we must either flush all
> outstanding fences (for we do not know which are writes and should have
> been exclusive) or alternatively create a new exclusive fence that is
> the composite of all the existing shared fences, and so will only be
> signaled when all earlier fences are signaled (ensuring that we can not
> be signaled before the completion of any earlier write).
>
> v2: reservation_object is already locked by amdgpu_bo_reserve()
> v3: Replace looping with get_fences_rcu and special case the promotion
> of a single shared fence directly to an exclusive fence, bypassing the
> fence array.
>
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=107341
> Testcase: igt/amd_prime/amd-to-i915
> References: 8e94a46c1770 ("drm/amdgpu: Attach exclusive fence to prime exported bo's. (v5)")
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Alex Deucher <alexander.deucher@amd.com>
> Cc: "Christian König" <christian.koenig@amd.com>

Reviewed-by: Christian König <christian.koenig@amd.com>

> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_prime.c | 58 +++++++++++++++++++----
>   1 file changed, 50 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_prime.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_prime.c
> index 3fdd5688da0b..06a310b5b07b 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_prime.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_prime.c
> @@ -37,6 +37,7 @@
>   #include "amdgpu_display.h"
>   #include <drm/amdgpu_drm.h>
>   #include <linux/dma-buf.h>
> +#include <linux/dma-fence-array.h>
>   
>   static const struct dma_buf_ops amdgpu_dmabuf_ops;
>   
> @@ -188,6 +189,47 @@ amdgpu_gem_prime_import_sg_table(struct drm_device *dev,
>   	return ERR_PTR(ret);
>   }
>   
> +static int
> +__reservation_object_make_exclusive(struct reservation_object *obj)
> +{
> +	struct dma_fence **fences;
> +	unsigned int count;
> +	int r;
> +
> +	if (!reservation_object_get_list(obj)) /* no shared fences to convert */
> +		return 0;
> +
> +	r = reservation_object_get_fences_rcu(obj, NULL, &count, &fences);
> +	if (r)
> +		return r;
> +
> +	if (count == 0) {
> +		/* Now that was unexpected. */
> +	} else if (count == 1) {
> +		reservation_object_add_excl_fence(obj, fences[0]);
> +		dma_fence_put(fences[0]);
> +		kfree(fences);
> +	} else {
> +		struct dma_fence_array *array;
> +
> +		array = dma_fence_array_create(count, fences,
> +					       dma_fence_context_alloc(1), 0,
> +					       false);
> +		if (!array)
> +			goto err_fences_put;
> +
> +		reservation_object_add_excl_fence(obj, &array->base);
> +	}
> +
> +	return 0;
> +
> +err_fences_put:
> +	while (count--)
> +		dma_fence_put(fences[count]);
> +	kfree(fences);
> +	return -ENOMEM;
> +}
> +
>   /**
>    * amdgpu_gem_map_attach - &dma_buf_ops.attach implementation
>    * @dma_buf: shared DMA buffer
> @@ -219,16 +261,16 @@ static int amdgpu_gem_map_attach(struct dma_buf *dma_buf,
>   
>   	if (attach->dev->driver != adev->dev->driver) {
>   		/*
> -		 * Wait for all shared fences to complete before we switch to future
> -		 * use of exclusive fence on this prime shared bo.
> +		 * We only create shared fences for internal use, but importers
> +		 * of the dmabuf rely on exclusive fences for implicitly
> +		 * tracking write hazards. As any of the current fences may
> +		 * correspond to a write, we need to convert all existing
> +		 * fences on the reservation object into a single exclusive
> +		 * fence.
>   		 */
> -		r = reservation_object_wait_timeout_rcu(bo->tbo.resv,
> -							true, false,
> -							MAX_SCHEDULE_TIMEOUT);
> -		if (unlikely(r < 0)) {
> -			DRM_DEBUG_PRIME("Fence wait failed: %li\n", r);
> +		r = __reservation_object_make_exclusive(bo->tbo.resv);
> +		if (r)
>   			goto error_unreserve;
> -		}
>   	}
>   
>   	/* pin buffer into GTT */

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✗ Fi.CI.CHECKPATCH: warning for drm/amdgpu: Transfer fences to dmabuf importer (rev5)

[Patchwork]

== Series Details ==

Series: drm/amdgpu: Transfer fences to dmabuf importer (rev5)
URL   : https://patchwork.freedesktop.org/series/47803/
State : warning

== Summary ==

$ dim checkpatch origin/drm-tip
11096db8b2d5 drm/amdgpu: Transfer fences to dmabuf importer
-:27: WARNING:COMMIT_LOG_LONG_LINE: Possible unwrapped commit description (prefer a maximum 75 chars per line)
#27: 
References: 8e94a46c1770 ("drm/amdgpu: Attach exclusive fence to prime exported bo's. (v5)")

-:27: ERROR:GIT_COMMIT_ID: Please use git commit description style 'commit <12+ chars of sha1> ("<title line>")' - ie: 'commit 8e94a46c1770 ("drm/amdgpu: Attach exclusive fence to prime exported bo's. (v5)")'
#27: 
References: 8e94a46c1770 ("drm/amdgpu: Attach exclusive fence to prime exported bo's. (v5)")

total: 1 errors, 1 warnings, 0 checks, 78 lines checked

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✓ Fi.CI.BAT: success for drm/amdgpu: Transfer fences to dmabuf importer (rev5)

[Patchwork]

== Series Details ==

Series: drm/amdgpu: Transfer fences to dmabuf importer (rev5)
URL   : https://patchwork.freedesktop.org/series/47803/
State : success

== Summary ==

= CI Bug Log - changes from CI_DRM_4627 -> Patchwork_9873 =

== Summary - SUCCESS ==

  No regressions found.

  External URL: https://patchwork.freedesktop.org/api/1.0/series/47803/revisions/5/mbox/

== Known issues ==

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

  === IGT changes ===

    ==== Issues hit ====

    igt@drv_selftest@live_hangcheck:
      fi-skl-guc:         PASS -> DMESG-FAIL (fdo#107174)

    igt@drv_selftest@live_workarounds:
      {fi-bsw-kefka}:     PASS -> DMESG-FAIL (fdo#107292)

    
    ==== Possible fixes ====

    {igt@amdgpu/amd_prime@amd-to-i915}:
      {fi-kbl-8809g}:     FAIL (fdo#107341) -> PASS

    igt@debugfs_test@read_all_entries:
      fi-snb-2520m:       INCOMPLETE (fdo#103713) -> PASS

    igt@drv_module_reload@basic-reload-inject:
      fi-hsw-4770r:       DMESG-WARN (fdo#107425) -> PASS

    igt@drv_selftest@live_hangcheck:
      fi-cfl-guc:         DMESG-FAIL -> PASS
      fi-kbl-7567u:       DMESG-FAIL (fdo#106560, fdo#106947) -> PASS

    igt@drv_selftest@live_workarounds:
      fi-kbl-x1275:       DMESG-FAIL (fdo#107292) -> PASS
      fi-cnl-psr:         DMESG-FAIL (fdo#107292) -> PASS

    igt@kms_pipe_crc_basic@nonblocking-crc-pipe-b:
      {fi-byt-clapper}:   FAIL (fdo#107362) -> PASS

    
    ==== Warnings ====

    {igt@kms_psr@primary_page_flip}:
      fi-cnl-psr:         DMESG-FAIL (fdo#107372) -> DMESG-WARN (fdo#107372)

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

  fdo#103713 https://bugs.freedesktop.org/show_bug.cgi?id=103713
  fdo#106560 https://bugs.freedesktop.org/show_bug.cgi?id=106560
  fdo#106947 https://bugs.freedesktop.org/show_bug.cgi?id=106947
  fdo#107174 https://bugs.freedesktop.org/show_bug.cgi?id=107174
  fdo#107292 https://bugs.freedesktop.org/show_bug.cgi?id=107292
  fdo#107341 https://bugs.freedesktop.org/show_bug.cgi?id=107341
  fdo#107362 https://bugs.freedesktop.org/show_bug.cgi?id=107362
  fdo#107372 https://bugs.freedesktop.org/show_bug.cgi?id=107372
  fdo#107425 https://bugs.freedesktop.org/show_bug.cgi?id=107425


== Participating hosts (53 -> 48) ==

  Missing    (5): fi-ctg-p8600 fi-ilk-m540 fi-byt-squawks fi-bsw-cyan fi-hsw-4200u 


== Build changes ==

    * Linux: CI_DRM_4627 -> Patchwork_9873

  CI_DRM_4627: 02a516dd8c6aeb57812d08034a90624bc7fc431a @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_4587: 5d78c73d871525ec9caecd88ad7d9abe36637314 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_9873: 11096db8b2d593ad05914418d631122e17f1dc3d @ git://anongit.freedesktop.org/gfx-ci/linux


== Linux commits ==

11096db8b2d5 drm/amdgpu: Transfer fences to dmabuf importer

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_9873/issues.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v6] drm/amdgpu: Transfer fences to dmabuf importer

[Chris Wilson]

amdgpu only uses shared-fences internally, but dmabuf importers rely on
implicit write hazard tracking via the reservation_object.fence_excl.
For example, the importer use the write hazard for timing a page flip to
only occur after the exporter has finished flushing its write into the
surface. As such, on exporting a dmabuf, we must either flush all
outstanding fences (for we do not know which are writes and should have
been exclusive) or alternatively create a new exclusive fence that is
the composite of all the existing shared fences, and so will only be
signaled when all earlier fences are signaled (ensuring that we can not
be signaled before the completion of any earlier write).

v2: reservation_object is already locked by amdgpu_bo_reserve()
v3: Replace looping with get_fences_rcu and special case the promotion
of a single shared fence directly to an exclusive fence, bypassing the
fence array.
v4: Drop the fence array ref after assigning to reservation_object

Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=107341
Testcase: igt/amd_prime/amd-to-i915
References: 8e94a46c1770 ("drm/amdgpu: Attach exclusive fence to prime exported bo's. (v5)")
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Alex Deucher <alexander.deucher@amd.com>
Cc: "Christian König" <christian.koenig@amd.com>
Reviewed-by: "Christian König" <christian.koenig@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_prime.c | 59 ++++++++++++++++++++---
 1 file changed, 51 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_prime.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_prime.c
index 3fdd5688da0b..0394f5c77f81 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_prime.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_prime.c
@@ -37,6 +37,7 @@
 #include "amdgpu_display.h"
 #include <drm/amdgpu_drm.h>
 #include <linux/dma-buf.h>
+#include <linux/dma-fence-array.h>
 
 static const struct dma_buf_ops amdgpu_dmabuf_ops;
 
@@ -188,6 +189,48 @@ amdgpu_gem_prime_import_sg_table(struct drm_device *dev,
 	return ERR_PTR(ret);
 }
 
+static int
+__reservation_object_make_exclusive(struct reservation_object *obj)
+{
+	struct dma_fence **fences;
+	unsigned int count;
+	int r;
+
+	if (!reservation_object_get_list(obj)) /* no shared fences to convert */
+		return 0;
+
+	r = reservation_object_get_fences_rcu(obj, NULL, &count, &fences);
+	if (r)
+		return r;
+
+	if (count == 0) {
+		/* Now that was unexpected. */
+	} else if (count == 1) {
+		reservation_object_add_excl_fence(obj, fences[0]);
+		dma_fence_put(fences[0]);
+		kfree(fences);
+	} else {
+		struct dma_fence_array *array;
+
+		array = dma_fence_array_create(count, fences,
+					       dma_fence_context_alloc(1), 0,
+					       false);
+		if (!array)
+			goto err_fences_put;
+
+		reservation_object_add_excl_fence(obj, &array->base);
+		dma_fence_put(&array->base);
+	}
+
+	return 0;
+
+err_fences_put:
+	while (count--)
+		dma_fence_put(fences[count]);
+	kfree(fences);
+	return -ENOMEM;
+}
+
 /**
  * amdgpu_gem_map_attach - &dma_buf_ops.attach implementation
  * @dma_buf: shared DMA buffer
@@ -219,16 +262,16 @@ static int amdgpu_gem_map_attach(struct dma_buf *dma_buf,
 
 	if (attach->dev->driver != adev->dev->driver) {
 		/*
-		 * Wait for all shared fences to complete before we switch to future
-		 * use of exclusive fence on this prime shared bo.
+		 * We only create shared fences for internal use, but importers
+		 * of the dmabuf rely on exclusive fences for implicitly
+		 * tracking write hazards. As any of the current fences may
+		 * correspond to a write, we need to convert all existing
+		 * fences on the reservation object into a single exclusive
+		 * fence.
 		 */
-		r = reservation_object_wait_timeout_rcu(bo->tbo.resv,
-							true, false,
-							MAX_SCHEDULE_TIMEOUT);
-		if (unlikely(r < 0)) {
-			DRM_DEBUG_PRIME("Fence wait failed: %li\n", r);
+		r = __reservation_object_make_exclusive(bo->tbo.resv);
+		if (r)
 			goto error_unreserve;
-		}
 	}
 
 	/* pin buffer into GTT */
-- 
2.18.0

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✗ Fi.CI.CHECKPATCH: warning for drm/amdgpu: Transfer fences to dmabuf importer (rev6)

[Patchwork]

== Series Details ==

Series: drm/amdgpu: Transfer fences to dmabuf importer (rev6)
URL   : https://patchwork.freedesktop.org/series/47803/
State : warning

== Summary ==

$ dim checkpatch origin/drm-tip
153e529b6e66 drm/amdgpu: Transfer fences to dmabuf importer
-:28: WARNING:COMMIT_LOG_LONG_LINE: Possible unwrapped commit description (prefer a maximum 75 chars per line)
#28: 
References: 8e94a46c1770 ("drm/amdgpu: Attach exclusive fence to prime exported bo's. (v5)")

-:28: ERROR:GIT_COMMIT_ID: Please use git commit description style 'commit <12+ chars of sha1> ("<title line>")' - ie: 'commit 8e94a46c1770 ("drm/amdgpu: Attach exclusive fence to prime exported bo's. (v5)")'
#28: 
References: 8e94a46c1770 ("drm/amdgpu: Attach exclusive fence to prime exported bo's. (v5)")

total: 1 errors, 1 warnings, 0 checks, 79 lines checked

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✓ Fi.CI.BAT: success for drm/amdgpu: Transfer fences to dmabuf importer (rev6)

[Patchwork]

== Series Details ==

Series: drm/amdgpu: Transfer fences to dmabuf importer (rev6)
URL   : https://patchwork.freedesktop.org/series/47803/
State : success

== Summary ==

= CI Bug Log - changes from CI_DRM_4630 -> Patchwork_9875 =

== Summary - SUCCESS ==

  No regressions found.

  External URL: https://patchwork.freedesktop.org/api/1.0/series/47803/revisions/6/mbox/

== Known issues ==

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

  === IGT changes ===

    ==== Issues hit ====

    igt@drv_selftest@live_workarounds:
      fi-bxt-dsi:         PASS -> DMESG-FAIL (fdo#107292)
      fi-kbl-7560u:       PASS -> DMESG-FAIL (fdo#107292)

    igt@kms_frontbuffer_tracking@basic:
      {fi-byt-clapper}:   PASS -> FAIL (fdo#103167)

    
    ==== Possible fixes ====

    {igt@amdgpu/amd_prime@amd-to-i915}:
      {fi-kbl-8809g}:     FAIL (fdo#107341) -> PASS

    igt@drv_selftest@live_coherency:
      fi-gdg-551:         DMESG-FAIL (fdo#107164) -> PASS

    igt@drv_selftest@live_hangcheck:
      fi-skl-guc:         DMESG-FAIL (fdo#107174) -> PASS

    igt@kms_pipe_crc_basic@read-crc-pipe-b-frame-sequence:
      {fi-byt-clapper}:   FAIL (fdo#103191, fdo#107362) -> PASS

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

  fdo#103167 https://bugs.freedesktop.org/show_bug.cgi?id=103167
  fdo#103191 https://bugs.freedesktop.org/show_bug.cgi?id=103191
  fdo#107164 https://bugs.freedesktop.org/show_bug.cgi?id=107164
  fdo#107174 https://bugs.freedesktop.org/show_bug.cgi?id=107174
  fdo#107292 https://bugs.freedesktop.org/show_bug.cgi?id=107292
  fdo#107341 https://bugs.freedesktop.org/show_bug.cgi?id=107341
  fdo#107362 https://bugs.freedesktop.org/show_bug.cgi?id=107362


== Participating hosts (52 -> 47) ==

  Missing    (5): fi-ctg-p8600 fi-ilk-m540 fi-bsw-cyan fi-icl-u fi-hsw-4200u 


== Build changes ==

    * Linux: CI_DRM_4630 -> Patchwork_9875

  CI_DRM_4630: c2cb1ea0083a18ca6a3e6aad41fd55b7f7c12af5 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_4587: 5d78c73d871525ec9caecd88ad7d9abe36637314 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_9875: 153e529b6e66b3d66c45ce89a15ee841acbf2279 @ git://anongit.freedesktop.org/gfx-ci/linux


== Linux commits ==

153e529b6e66 drm/amdgpu: Transfer fences to dmabuf importer

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_9875/issues.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✓ Fi.CI.IGT: success for drm/amdgpu: Transfer fences to dmabuf importer (rev6)

[Patchwork]

== Series Details ==

Series: drm/amdgpu: Transfer fences to dmabuf importer (rev6)
URL   : https://patchwork.freedesktop.org/series/47803/
State : success

== Summary ==

= CI Bug Log - changes from CI_DRM_4630_full -> Patchwork_9875_full =

== Summary - WARNING ==

  Minor unknown changes coming with Patchwork_9875_full need to be verified
  manually.
  
  If you think the reported changes have nothing to do with the changes
  introduced in Patchwork_9875_full, please notify your bug team to allow them
  to document this new failure mode, which will reduce false positives in CI.

  

== Possible new issues ==

  Here are the unknown changes that may have been introduced in Patchwork_9875_full:

  === IGT changes ===

    ==== Warnings ====

    igt@pm_rc6_residency@rc6-accuracy:
      shard-snb:          PASS -> SKIP

    
== Known issues ==

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

  === IGT changes ===

    ==== Issues hit ====

    igt@kms_draw_crc@draw-method-rgb565-mmap-gtt-ytiled:
      shard-glk:          PASS -> FAIL (fdo#103184)

    igt@kms_setmode@basic:
      shard-kbl:          NOTRUN -> FAIL (fdo#99912)

    
    ==== Possible fixes ====

    igt@gem_ppgtt@blt-vs-render-ctx0:
      shard-kbl:          INCOMPLETE (fdo#106023, fdo#103665) -> PASS

    igt@kms_atomic_transition@1x-modeset-transitions-nonblocking-fencing:
      shard-glk:          FAIL (fdo#105703, fdo#107409) -> PASS

    
  fdo#103184 https://bugs.freedesktop.org/show_bug.cgi?id=103184
  fdo#103665 https://bugs.freedesktop.org/show_bug.cgi?id=103665
  fdo#105703 https://bugs.freedesktop.org/show_bug.cgi?id=105703
  fdo#106023 https://bugs.freedesktop.org/show_bug.cgi?id=106023
  fdo#107409 https://bugs.freedesktop.org/show_bug.cgi?id=107409
  fdo#99912 https://bugs.freedesktop.org/show_bug.cgi?id=99912


== Participating hosts (5 -> 5) ==

  No changes in participating hosts


== Build changes ==

    * Linux: CI_DRM_4630 -> Patchwork_9875

  CI_DRM_4630: c2cb1ea0083a18ca6a3e6aad41fd55b7f7c12af5 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_4587: 5d78c73d871525ec9caecd88ad7d9abe36637314 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_9875: 153e529b6e66b3d66c45ce89a15ee841acbf2279 @ 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_9875/shards.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx