[1/7] drm/i915: Clear the GGTT_WRITE bit on unbinding the vma
diff mbox series

Message ID 20200121222447.419489-1-chris@chris-wilson.co.uk
State New
Headers show
Series
  • [1/7] drm/i915: Clear the GGTT_WRITE bit on unbinding the vma
Related show

Commit Message

Chris Wilson Jan. 21, 2020, 10:24 p.m. UTC
While we do flush writes to the vma before unbinding (to make sure they
go through the right detiling register), we may also be concurrently
poking at the GGTT_WRITE bit from set-domain, as we mark all GGTT vma
associated with an object. We know this is for another vma, as we
are currently unbind this one -- so if this vma will be reused, it will
be refaulted and have its dirty bit set before the next write.

Closes: https://gitlab.freedesktop.org/drm/intel/issues/999
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/i915_vma.c       | 11 +++++++++--
 drivers/gpu/drm/i915/i915_vma_types.h |  1 +
 2 files changed, 10 insertions(+), 2 deletions(-)

Comments

Mika Kuoppala Jan. 22, 2020, 1:15 p.m. UTC | #1
Chris Wilson <chris@chris-wilson.co.uk> writes:

> While we do flush writes to the vma before unbinding (to make sure they
> go through the right detiling register), we may also be concurrently
> poking at the GGTT_WRITE bit from set-domain, as we mark all GGTT vma
> associated with an object. We know this is for another vma, as we
> are currently unbind this one -- so if this vma will be reused, it will
> be refaulted and have its dirty bit set before the next write.
>
> Closes: https://gitlab.freedesktop.org/drm/intel/issues/999
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> ---
>  drivers/gpu/drm/i915/i915_vma.c       | 11 +++++++++--
>  drivers/gpu/drm/i915/i915_vma_types.h |  1 +
>  2 files changed, 10 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_vma.c b/drivers/gpu/drm/i915/i915_vma.c
> index 17d7c525ea5c..eb18b56af3af 100644
> --- a/drivers/gpu/drm/i915/i915_vma.c
> +++ b/drivers/gpu/drm/i915/i915_vma.c
> @@ -1218,9 +1218,15 @@ int __i915_vma_unbind(struct i915_vma *vma)
>  		 * before the unbind, other due to non-strict nature of those
>  		 * indirect writes they may end up referencing the GGTT PTE
>  		 * after the unbind.
> +		 *
> +		 * Note that we may be concurrently poking at the GGTT_WRITE
> +		 * bit from set-domain, as we mark all GGTT vma associated
> +		 * with an object. We know this is for another vma, as we
> +		 * are currently unbind this one -- so if this vma will be
> +		 * reused, it will be refaulted and have its dirty bit set
> +		 * before the next write.
>  		 */
>  		i915_vma_flush_writes(vma);
> -		GEM_BUG_ON(i915_vma_has_ggtt_write(vma));
>  
>  		/* release the fence reg _after_ flushing */
>  		ret = i915_vma_revoke_fence(vma);
> @@ -1240,7 +1246,8 @@ int __i915_vma_unbind(struct i915_vma *vma)
>  		trace_i915_vma_unbind(vma);
>  		vma->ops->unbind_vma(vma);
>  	}
> -	atomic_and(~(I915_VMA_BIND_MASK | I915_VMA_ERROR), &vma->flags);
> +	atomic_and(~(I915_VMA_BIND_MASK | I915_VMA_ERROR | I915_VMA_DIRTY),
> +		   &vma->flags);
>  
>  	i915_vma_detach(vma);
>  	vma_unbind_pages(vma);
> diff --git a/drivers/gpu/drm/i915/i915_vma_types.h b/drivers/gpu/drm/i915/i915_vma_types.h
> index e0942efd5236..1ddc450ae766 100644
> --- a/drivers/gpu/drm/i915/i915_vma_types.h
> +++ b/drivers/gpu/drm/i915/i915_vma_types.h
> @@ -244,6 +244,7 @@ struct i915_vma {
>  #define I915_VMA_CAN_FENCE_BIT	15
>  #define I915_VMA_USERFAULT_BIT	16
>  #define I915_VMA_GGTT_WRITE_BIT	17
> +#define I915_VMA_DIRTY		((int)BIT(I915_VMA_GGTT_WRITE_BIT))

You can omit this and use I915_VMA_GGTT_WRITE.

With that,
Reviewed-by: Mika Kuoppala <mika.kuoppala@linux.intel.com>

>  
>  #define I915_VMA_GGTT		((int)BIT(I915_VMA_GGTT_BIT))
>  #define I915_VMA_CAN_FENCE	((int)BIT(I915_VMA_CAN_FENCE_BIT))
> -- 
> 2.25.0

Patch
diff mbox series

diff --git a/drivers/gpu/drm/i915/i915_vma.c b/drivers/gpu/drm/i915/i915_vma.c
index 17d7c525ea5c..eb18b56af3af 100644
--- a/drivers/gpu/drm/i915/i915_vma.c
+++ b/drivers/gpu/drm/i915/i915_vma.c
@@ -1218,9 +1218,15 @@  int __i915_vma_unbind(struct i915_vma *vma)
 		 * before the unbind, other due to non-strict nature of those
 		 * indirect writes they may end up referencing the GGTT PTE
 		 * after the unbind.
+		 *
+		 * Note that we may be concurrently poking at the GGTT_WRITE
+		 * bit from set-domain, as we mark all GGTT vma associated
+		 * with an object. We know this is for another vma, as we
+		 * are currently unbind this one -- so if this vma will be
+		 * reused, it will be refaulted and have its dirty bit set
+		 * before the next write.
 		 */
 		i915_vma_flush_writes(vma);
-		GEM_BUG_ON(i915_vma_has_ggtt_write(vma));
 
 		/* release the fence reg _after_ flushing */
 		ret = i915_vma_revoke_fence(vma);
@@ -1240,7 +1246,8 @@  int __i915_vma_unbind(struct i915_vma *vma)
 		trace_i915_vma_unbind(vma);
 		vma->ops->unbind_vma(vma);
 	}
-	atomic_and(~(I915_VMA_BIND_MASK | I915_VMA_ERROR), &vma->flags);
+	atomic_and(~(I915_VMA_BIND_MASK | I915_VMA_ERROR | I915_VMA_DIRTY),
+		   &vma->flags);
 
 	i915_vma_detach(vma);
 	vma_unbind_pages(vma);
diff --git a/drivers/gpu/drm/i915/i915_vma_types.h b/drivers/gpu/drm/i915/i915_vma_types.h
index e0942efd5236..1ddc450ae766 100644
--- a/drivers/gpu/drm/i915/i915_vma_types.h
+++ b/drivers/gpu/drm/i915/i915_vma_types.h
@@ -244,6 +244,7 @@  struct i915_vma {
 #define I915_VMA_CAN_FENCE_BIT	15
 #define I915_VMA_USERFAULT_BIT	16
 #define I915_VMA_GGTT_WRITE_BIT	17
+#define I915_VMA_DIRTY		((int)BIT(I915_VMA_GGTT_WRITE_BIT))
 
 #define I915_VMA_GGTT		((int)BIT(I915_VMA_GGTT_BIT))
 #define I915_VMA_CAN_FENCE	((int)BIT(I915_VMA_CAN_FENCE_BIT))