[PATCH 2/4] drm/i915: Use maximum write flush for pwrite_gtt

[PATCH 2/4] drm/i915: Use maximum write flush for pwrite_gtt

[Chris Wilson]

As recently disovered by forcing big-core (!llc) machines to use the GTT
paths, we need our full GTT write flush before manipulating the GTT PTE
or else the writes may be directed to the wrong page.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
Cc: Matthew Auld <matthew.william.auld@gmail.com>
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Cc: stable@vger.kernel.org
---
 drivers/gpu/drm/i915/i915_gem.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index fed0bc421a55..c6ba350e6e4f 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -610,7 +610,8 @@ i915_gem_gtt_pwrite_fast(struct drm_i915_gem_object *obj,
 		unsigned int page_length = PAGE_SIZE - page_offset;
 		page_length = remain < page_length ? remain : page_length;
 		if (node.allocated) {
-			wmb(); /* flush the write before we modify the GGTT */
+			/* flush the write before we modify the GGTT */
+			intel_gt_flush_ggtt_writes(ggtt->vm.gt);
 			ggtt->vm.insert_page(&ggtt->vm,
 					     i915_gem_object_get_dma_address(obj, offset >> PAGE_SHIFT),
 					     node.start, I915_CACHE_NONE, 0);
@@ -639,8 +640,8 @@ i915_gem_gtt_pwrite_fast(struct drm_i915_gem_object *obj,
 	i915_gem_object_unlock_fence(obj, fence);
 out_unpin:
 	mutex_lock(&i915->drm.struct_mutex);
+	intel_gt_flush_ggtt_writes(ggtt->vm.gt);
 	if (node.allocated) {
-		wmb();
 		ggtt->vm.clear_range(&ggtt->vm, node.start, node.size);
 		remove_mappable_node(&node);
 	} else {
-- 
2.22.0

[PATCH 4/4] drm/i915: Flush stale cachelines on set-cache-level

[Chris Wilson]

Ensure that we flush any cache dirt out to main memory before the user
changes the cache-level as they may elect to bypass the cache (even after
declaring their access cache-coherent) via use of unprivileged MOCS.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
Cc: stable@vger.kernel.org
---
 drivers/gpu/drm/i915/gem/i915_gem_domain.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/gpu/drm/i915/gem/i915_gem_domain.c b/drivers/gpu/drm/i915/gem/i915_gem_domain.c
index 2e3ce2a69653..5d41e769a428 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_domain.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_domain.c
@@ -277,6 +277,11 @@ int i915_gem_object_set_cache_level(struct drm_i915_gem_object *obj,
 
 	list_for_each_entry(vma, &obj->vma.list, obj_link)
 		vma->node.color = cache_level;
+
+	/* Flush any previous cache dirt in case of cache bypass */
+	if (obj->cache_dirty & ~obj->cache_coherent)
+		i915_gem_clflush_object(obj, I915_CLFLUSH_SYNC);
+
 	i915_gem_object_set_cache_coherency(obj, cache_level);
 	obj->cache_dirty = true; /* Always invalidate stale cachelines */
 
-- 
2.22.0

Re: [PATCH 2/4] drm/i915: Use maximum write flush for pwrite_gtt

[Ville Syrjälä]

On Thu, Jul 18, 2019 at 03:54:05PM +0100, Chris Wilson wrote:
> As recently disovered by forcing big-core (!llc) machines to use the GTT
> paths, we need our full GTT write flush before manipulating the GTT PTE
> or else the writes may be directed to the wrong page.
> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> Cc: Matthew Auld <matthew.william.auld@gmail.com>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Cc: stable@vger.kernel.org
> ---
>  drivers/gpu/drm/i915/i915_gem.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index fed0bc421a55..c6ba350e6e4f 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -610,7 +610,8 @@ i915_gem_gtt_pwrite_fast(struct drm_i915_gem_object *obj,
>  		unsigned int page_length = PAGE_SIZE - page_offset;
>  		page_length = remain < page_length ? remain : page_length;
>  		if (node.allocated) {
> -			wmb(); /* flush the write before we modify the GGTT */
> +			/* flush the write before we modify the GGTT */
> +			intel_gt_flush_ggtt_writes(ggtt->vm.gt);

Matches the story told by intel_gt_flush_ggtt_writes().

Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

>  			ggtt->vm.insert_page(&ggtt->vm,
>  					     i915_gem_object_get_dma_address(obj, offset >> PAGE_SHIFT),
>  					     node.start, I915_CACHE_NONE, 0);
> @@ -639,8 +640,8 @@ i915_gem_gtt_pwrite_fast(struct drm_i915_gem_object *obj,
>  	i915_gem_object_unlock_fence(obj, fence);
>  out_unpin:
>  	mutex_lock(&i915->drm.struct_mutex);
> +	intel_gt_flush_ggtt_writes(ggtt->vm.gt);
>  	if (node.allocated) {
> -		wmb();
>  		ggtt->vm.clear_range(&ggtt->vm, node.start, node.size);
>  		remove_mappable_node(&node);
>  	} else {
> -- 
> 2.22.0

-- 
Ville Syrjälä
Intel

Re: [PATCH 2/4] drm/i915: Use maximum write flush for pwrite_gtt

[Chris Wilson]

Quoting Ville Syrjälä (2019-07-18 19:28:43)
> On Thu, Jul 18, 2019 at 03:54:05PM +0100, Chris Wilson wrote:
> > As recently disovered by forcing big-core (!llc) machines to use the GTT
> > paths, we need our full GTT write flush before manipulating the GTT PTE
> > or else the writes may be directed to the wrong page.
> > 
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> > Cc: Matthew Auld <matthew.william.auld@gmail.com>
> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > Cc: stable@vger.kernel.org
> > ---
> >  drivers/gpu/drm/i915/i915_gem.c | 5 +++--
> >  1 file changed, 3 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> > index fed0bc421a55..c6ba350e6e4f 100644
> > --- a/drivers/gpu/drm/i915/i915_gem.c
> > +++ b/drivers/gpu/drm/i915/i915_gem.c
> > @@ -610,7 +610,8 @@ i915_gem_gtt_pwrite_fast(struct drm_i915_gem_object *obj,
> >               unsigned int page_length = PAGE_SIZE - page_offset;
> >               page_length = remain < page_length ? remain : page_length;
> >               if (node.allocated) {
> > -                     wmb(); /* flush the write before we modify the GGTT */
> > +                     /* flush the write before we modify the GGTT */
> > +                     intel_gt_flush_ggtt_writes(ggtt->vm.gt);
> 
> Matches the story told by intel_gt_flush_ggtt_writes().
> 
> Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

Ta, pushed to dinq. Hopefully, this may explain some mystery fails!
(Not that any sane userspace does for(;;) { gem_write(); gem_read(); })
-Chris

Re: [PATCH 4/4] drm/i915: Flush stale cachelines on set-cache-level

[Joonas Lahtinen]

Quoting Chris Wilson (2019-07-18 17:54:07)
> Ensure that we flush any cache dirt out to main memory before the user
> changes the cache-level as they may elect to bypass the cache (even after
> declaring their access cache-coherent) via use of unprivileged MOCS.
> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> Cc: stable@vger.kernel.org

Reviewed-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>

Regards, Joonas

Re: [Intel-gfx] [PATCH 4/4] drm/i915: Flush stale cachelines on set-cache-level

[Daniel Vetter]

On Thu, Jul 18, 2019 at 4:54 PM Chris Wilson <chris@chris-wilson.co.uk> wrote:
>
> Ensure that we flush any cache dirt out to main memory before the user
> changes the cache-level as they may elect to bypass the cache (even after
> declaring their access cache-coherent) via use of unprivileged MOCS.
>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> Cc: stable@vger.kernel.org
> ---
>  drivers/gpu/drm/i915/gem/i915_gem_domain.c | 5 +++++
>  1 file changed, 5 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_domain.c b/drivers/gpu/drm/i915/gem/i915_gem_domain.c
> index 2e3ce2a69653..5d41e769a428 100644
> --- a/drivers/gpu/drm/i915/gem/i915_gem_domain.c
> +++ b/drivers/gpu/drm/i915/gem/i915_gem_domain.c
> @@ -277,6 +277,11 @@ int i915_gem_object_set_cache_level(struct drm_i915_gem_object *obj,
>
>         list_for_each_entry(vma, &obj->vma.list, obj_link)
>                 vma->node.color = cache_level;
> +
> +       /* Flush any previous cache dirt in case of cache bypass */
> +       if (obj->cache_dirty & ~obj->cache_coherent)
> +               i915_gem_clflush_object(obj, I915_CLFLUSH_SYNC);

I think writing out the bit logic instead of implicitly relying on the
#defines would be much better, i.e. && !(cache_coherent &
COHERENT_FOR_READ). Plus I think we only need to set cache_dirty =
true if we don't flush here already, to avoid double flushing?
-Daniel

> +
>         i915_gem_object_set_cache_coherency(obj, cache_level);
>         obj->cache_dirty = true; /* Always invalidate stale cachelines */
>
> --
> 2.22.0
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx



--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [Intel-gfx] [PATCH 4/4] drm/i915: Flush stale cachelines on set-cache-level

[Chris Wilson]

Quoting Daniel Vetter (2019-11-12 09:09:06)
> On Thu, Jul 18, 2019 at 4:54 PM Chris Wilson <chris@chris-wilson.co.uk> wrote:
> >
> > Ensure that we flush any cache dirt out to main memory before the user
> > changes the cache-level as they may elect to bypass the cache (even after
> > declaring their access cache-coherent) via use of unprivileged MOCS.
> >
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> > Cc: stable@vger.kernel.org
> > ---
> >  drivers/gpu/drm/i915/gem/i915_gem_domain.c | 5 +++++
> >  1 file changed, 5 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/i915/gem/i915_gem_domain.c b/drivers/gpu/drm/i915/gem/i915_gem_domain.c
> > index 2e3ce2a69653..5d41e769a428 100644
> > --- a/drivers/gpu/drm/i915/gem/i915_gem_domain.c
> > +++ b/drivers/gpu/drm/i915/gem/i915_gem_domain.c
> > @@ -277,6 +277,11 @@ int i915_gem_object_set_cache_level(struct drm_i915_gem_object *obj,
> >
> >         list_for_each_entry(vma, &obj->vma.list, obj_link)
> >                 vma->node.color = cache_level;
> > +
> > +       /* Flush any previous cache dirt in case of cache bypass */
> > +       if (obj->cache_dirty & ~obj->cache_coherent)
> > +               i915_gem_clflush_object(obj, I915_CLFLUSH_SYNC);
> 
> I think writing out the bit logic instead of implicitly relying on the
> #defines would be much better, i.e. && !(cache_coherent &
> COHERENT_FOR_READ). Plus I think we only need to set cache_dirty =
> true if we don't flush here already, to avoid double flushing?

No. The mask is being updated, so you need to flush before you lose
track. The cache is then cleared of the dirty bit so won't be flushed
again until dirty and no longer coherent. We need to flag that the page
is no longer coherent at the end of its lifetime (passing back to the
system) to force the flush then.
-Chris

Re: [Intel-gfx] [PATCH 4/4] drm/i915: Flush stale cachelines on set-cache-level

[Daniel Vetter]

On Tue, Nov 12, 2019 at 10:43 AM Chris Wilson <chris@chris-wilson.co.uk> wrote:
>
> Quoting Daniel Vetter (2019-11-12 09:09:06)
> > On Thu, Jul 18, 2019 at 4:54 PM Chris Wilson <chris@chris-wilson.co.uk> wrote:
> > >
> > > Ensure that we flush any cache dirt out to main memory before the user
> > > changes the cache-level as they may elect to bypass the cache (even after
> > > declaring their access cache-coherent) via use of unprivileged MOCS.
> > >
> > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > > Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> > > Cc: stable@vger.kernel.org
> > > ---
> > >  drivers/gpu/drm/i915/gem/i915_gem_domain.c | 5 +++++
> > >  1 file changed, 5 insertions(+)
> > >
> > > diff --git a/drivers/gpu/drm/i915/gem/i915_gem_domain.c b/drivers/gpu/drm/i915/gem/i915_gem_domain.c
> > > index 2e3ce2a69653..5d41e769a428 100644
> > > --- a/drivers/gpu/drm/i915/gem/i915_gem_domain.c
> > > +++ b/drivers/gpu/drm/i915/gem/i915_gem_domain.c
> > > @@ -277,6 +277,11 @@ int i915_gem_object_set_cache_level(struct drm_i915_gem_object *obj,
> > >
> > >         list_for_each_entry(vma, &obj->vma.list, obj_link)
> > >                 vma->node.color = cache_level;
> > > +
> > > +       /* Flush any previous cache dirt in case of cache bypass */
> > > +       if (obj->cache_dirty & ~obj->cache_coherent)
> > > +               i915_gem_clflush_object(obj, I915_CLFLUSH_SYNC);
> >
> > I think writing out the bit logic instead of implicitly relying on the
> > #defines would be much better, i.e. && !(cache_coherent &
> > COHERENT_FOR_READ). Plus I think we only need to set cache_dirty =
> > true if we don't flush here already, to avoid double flushing?
>
> No. The mask is being updated, so you need to flush before you lose
> track. The cache is then cleared of the dirty bit so won't be flushed
> again until dirty and no longer coherent. We need to flag that the page
> is no longer coherent at the end of its lifetime (passing back to the
> system) to force the flush then.

Hm I think I overlooked that we only clear cache_dirty in
i915_gem_clflush_object when it's a coherent mode.

I also spotted more cases for (obj->cache_dirty
&~obj->cache_coherent), so that obscure/fragile pattern is
pre-existing :-/ One of them also checks outside of the object lock,
which I think is how these states are supposed to be protected. Smells
a bit fishy still, would be good to make a bit clearer.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [Intel-gfx] [PATCH 4/4] drm/i915: Flush stale cachelines on set-cache-level

[Daniel Vetter]

On Tue, Nov 12, 2019 at 11:57 AM Daniel Vetter <daniel@ffwll.ch> wrote:
>
> On Tue, Nov 12, 2019 at 10:43 AM Chris Wilson <chris@chris-wilson.co.uk> wrote:
> >
> > Quoting Daniel Vetter (2019-11-12 09:09:06)
> > > On Thu, Jul 18, 2019 at 4:54 PM Chris Wilson <chris@chris-wilson.co.uk> wrote:
> > > >
> > > > Ensure that we flush any cache dirt out to main memory before the user
> > > > changes the cache-level as they may elect to bypass the cache (even after
> > > > declaring their access cache-coherent) via use of unprivileged MOCS.
> > > >
> > > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > > > Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> > > > Cc: stable@vger.kernel.org
> > > > ---
> > > >  drivers/gpu/drm/i915/gem/i915_gem_domain.c | 5 +++++
> > > >  1 file changed, 5 insertions(+)
> > > >
> > > > diff --git a/drivers/gpu/drm/i915/gem/i915_gem_domain.c b/drivers/gpu/drm/i915/gem/i915_gem_domain.c
> > > > index 2e3ce2a69653..5d41e769a428 100644
> > > > --- a/drivers/gpu/drm/i915/gem/i915_gem_domain.c
> > > > +++ b/drivers/gpu/drm/i915/gem/i915_gem_domain.c
> > > > @@ -277,6 +277,11 @@ int i915_gem_object_set_cache_level(struct drm_i915_gem_object *obj,
> > > >
> > > >         list_for_each_entry(vma, &obj->vma.list, obj_link)
> > > >                 vma->node.color = cache_level;
> > > > +
> > > > +       /* Flush any previous cache dirt in case of cache bypass */
> > > > +       if (obj->cache_dirty & ~obj->cache_coherent)
> > > > +               i915_gem_clflush_object(obj, I915_CLFLUSH_SYNC);
> > >
> > > I think writing out the bit logic instead of implicitly relying on the
> > > #defines would be much better, i.e. && !(cache_coherent &
> > > COHERENT_FOR_READ). Plus I think we only need to set cache_dirty =
> > > true if we don't flush here already, to avoid double flushing?
> >
> > No. The mask is being updated, so you need to flush before you lose
> > track. The cache is then cleared of the dirty bit so won't be flushed
> > again until dirty and no longer coherent. We need to flag that the page
> > is no longer coherent at the end of its lifetime (passing back to the
> > system) to force the flush then.
>
> Hm I think I overlooked that we only clear cache_dirty in
> i915_gem_clflush_object when it's a coherent mode.

Hm, the clear/blt code recently merged doesn't preserve the
->cache_dirty setting for this case, unlike clfush_object. Do we have
a bug there?

> I also spotted more cases for (obj->cache_dirty
> &~obj->cache_coherent), so that obscure/fragile pattern is
> pre-existing :-/ One of them also checks outside of the object lock,
> which I think is how these states are supposed to be protected. Smells
> a bit fishy still, would be good to make a bit clearer.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch