Re: [Intel-gfx] [PATCH 1/7] drm/i915: Clear the GGTT_WRITE bit on unbinding the vma

[Mika Kuoppala <mika.kuoppala@linux.intel.com>]

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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx