| Message ID | 20191119100929.2628356-6-chris@chris-wilson.co.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [01/17] drm/i915/gem: Manually dump the debug trace on GEM_BUG_ON | expand |
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
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
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:
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(-)