[06/17] drm/i915/gem: Merge GGTT vma flush into a single loop
diff mbox series

Message ID 20191119100929.2628356-6-chris@chris-wilson.co.uk
State New
Headers show
Series
  • [01/17] drm/i915/gem: Manually dump the debug trace on GEM_BUG_ON
Related show

Commit Message

Chris Wilson Nov. 19, 2019, 10:09 a.m. UTC
We only need the one loop to find the dirty vma flush them, and their
chipset.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
---
 drivers/gpu/drm/i915/gem/i915_gem_object.c | 12 +++---------
 1 file changed, 3 insertions(+), 9 deletions(-)

Comments

Mika Kuoppala Nov. 19, 2019, 10:48 a.m. UTC | #1
Chris Wilson <chris@chris-wilson.co.uk> writes:

> We only need the one loop to find the dirty vma flush them, and their
> chipset.
>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> ---
>  drivers/gpu/drm/i915/gem/i915_gem_object.c | 12 +++---------
>  1 file changed, 3 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_object.c b/drivers/gpu/drm/i915/gem/i915_gem_object.c
> index db103d3c8760..63bd3ff84f5e 100644
> --- a/drivers/gpu/drm/i915/gem/i915_gem_object.c
> +++ b/drivers/gpu/drm/i915/gem/i915_gem_object.c
> @@ -279,18 +279,12 @@ i915_gem_object_flush_write_domain(struct drm_i915_gem_object *obj,
>  
>  	switch (obj->write_domain) {
>  	case I915_GEM_DOMAIN_GTT:
> -		for_each_ggtt_vma(vma, obj)
> -			intel_gt_flush_ggtt_writes(vma->vm->gt);
> -
> -		intel_frontbuffer_flush(obj->frontbuffer, ORIGIN_CPU);
> -
>  		for_each_ggtt_vma(vma, obj) {
> -			if (vma->iomap)
> -				continue;
> -

Is the story with iomap to just avoid fragility and
go with the same path, even if the flushes would be
superfluous?

-Mika

> -			i915_vma_unset_ggtt_write(vma);
> +			if (i915_vma_unset_ggtt_write(vma))
> +				intel_gt_flush_ggtt_writes(vma->vm->gt);
>  		}
>  
> +		intel_frontbuffer_flush(obj->frontbuffer, ORIGIN_CPU);
>  		break;
>  
>  	case I915_GEM_DOMAIN_WC:
> -- 
> 2.24.0
Chris Wilson Nov. 19, 2019, 11:17 a.m. UTC | #2
Quoting Mika Kuoppala (2019-11-19 10:48:22)
> Chris Wilson <chris@chris-wilson.co.uk> writes:
> 
> > We only need the one loop to find the dirty vma flush them, and their
> > chipset.
> >
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> > ---
> >  drivers/gpu/drm/i915/gem/i915_gem_object.c | 12 +++---------
> >  1 file changed, 3 insertions(+), 9 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/gem/i915_gem_object.c b/drivers/gpu/drm/i915/gem/i915_gem_object.c
> > index db103d3c8760..63bd3ff84f5e 100644
> > --- a/drivers/gpu/drm/i915/gem/i915_gem_object.c
> > +++ b/drivers/gpu/drm/i915/gem/i915_gem_object.c
> > @@ -279,18 +279,12 @@ i915_gem_object_flush_write_domain(struct drm_i915_gem_object *obj,
> >  
> >       switch (obj->write_domain) {
> >       case I915_GEM_DOMAIN_GTT:
> > -             for_each_ggtt_vma(vma, obj)
> > -                     intel_gt_flush_ggtt_writes(vma->vm->gt);
> > -
> > -             intel_frontbuffer_flush(obj->frontbuffer, ORIGIN_CPU);
> > -
> >               for_each_ggtt_vma(vma, obj) {
> > -                     if (vma->iomap)
> > -                             continue;
> > -
> 
> Is the story with iomap to just avoid fragility and
> go with the same path, even if the flushes would be
> superfluous?

The subtle difference between the two loops is the first is just
flushing anything the user had their hands on, and the second anything
the kernel was doing for itself.

I don't think it's that simple.

For userspace to invoke this function, it has called a set-domain ioctl.
So it should equally be marking its write with a set-domain ioctl for
set-domain to even work. At which point, we might as well just say that
this can only work if userspace does its due diligence in using
set-domain or userspace has to care about the CPU caches itself.

So given that userspace has to take care anyway, I don't see any harm
here in skipping the flushes if we have not marked them as dirty. Now,
having said that, we should then be marking all the ggtt as dirty for a
set-domain ggtt write...
-Chris

Patch
diff mbox series

diff --git a/drivers/gpu/drm/i915/gem/i915_gem_object.c b/drivers/gpu/drm/i915/gem/i915_gem_object.c
index db103d3c8760..63bd3ff84f5e 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_object.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_object.c
@@ -279,18 +279,12 @@  i915_gem_object_flush_write_domain(struct drm_i915_gem_object *obj,
 
 	switch (obj->write_domain) {
 	case I915_GEM_DOMAIN_GTT:
-		for_each_ggtt_vma(vma, obj)
-			intel_gt_flush_ggtt_writes(vma->vm->gt);
-
-		intel_frontbuffer_flush(obj->frontbuffer, ORIGIN_CPU);
-
 		for_each_ggtt_vma(vma, obj) {
-			if (vma->iomap)
-				continue;
-
-			i915_vma_unset_ggtt_write(vma);
+			if (i915_vma_unset_ggtt_write(vma))
+				intel_gt_flush_ggtt_writes(vma->vm->gt);
 		}
 
+		intel_frontbuffer_flush(obj->frontbuffer, ORIGIN_CPU);
 		break;
 
 	case I915_GEM_DOMAIN_WC: