* [PATCH] drm/i915: Performed deferred clflush inside set-cache-level
@ 2015-01-13 13:32 Chris Wilson
2015-01-13 15:21 ` Jani Nikula
` (3 more replies)
0 siblings, 4 replies; 9+ messages in thread
From: Chris Wilson @ 2015-01-13 13:32 UTC (permalink / raw)
To: intel-gfx
Currently we are hitting the WARN inside
i915_gem_object_set_cache_level() as we can now have an unbound object
in the GTT write domain (due to 43566dedde54f9 "drm/i915: Broaden
application of set-domain(GTT)"). To avoid the warning, we need to track
when we elided the clflush on a cacheable object and then evict the
cache for the object when we move the object out of a cacheable domain.
Reported-by: Jani Nikula <jani.nikula@linux.intel.com>
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Jani Nikula <jani.nikula@linux.intel.com>
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
drivers/gpu/drm/i915/i915_drv.h | 1 +
drivers/gpu/drm/i915/i915_gem.c | 32 +++++++++-----------------------
2 files changed, 10 insertions(+), 23 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index bf18a5238887..7070482000cd 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1998,6 +1998,7 @@ struct drm_i915_gem_object {
*/
unsigned long gt_ro:1;
unsigned int cache_level:3;
+ unsigned int cache_dirty:1;
unsigned int has_dma_mapping:1;
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 9bafe50d3df7..aa089e7c31bf 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -3639,11 +3639,14 @@ i915_gem_clflush_object(struct drm_i915_gem_object *obj,
* snooping behaviour occurs naturally as the result of our domain
* tracking.
*/
- if (!force && cpu_cache_is_coherent(obj->base.dev, obj->cache_level))
+ if (!force && cpu_cache_is_coherent(obj->base.dev, obj->cache_level)) {
+ obj->cache_dirty = true;
return false;
+ }
trace_i915_gem_object_clflush(obj);
drm_clflush_sg(obj->pages);
+ obj->cache_dirty = false;
return true;
}
@@ -3826,28 +3829,11 @@ int i915_gem_object_set_cache_level(struct drm_i915_gem_object *obj,
vma->node.color = cache_level;
obj->cache_level = cache_level;
- if (cpu_write_needs_clflush(obj)) {
- u32 old_read_domains, old_write_domain;
-
- /* If we're coming from LLC cached, then we haven't
- * actually been tracking whether the data is in the
- * CPU cache or not, since we only allow one bit set
- * in obj->write_domain and have been skipping the clflushes.
- * Just set it to the CPU cache for now.
- */
- i915_gem_object_retire(obj);
- WARN_ON(obj->base.write_domain & ~I915_GEM_DOMAIN_CPU);
-
- old_read_domains = obj->base.read_domains;
- old_write_domain = obj->base.write_domain;
-
- obj->base.read_domains = I915_GEM_DOMAIN_CPU;
- obj->base.write_domain = I915_GEM_DOMAIN_CPU;
-
- trace_i915_gem_object_change_domain(obj,
- old_read_domains,
- old_write_domain);
- }
+ if (obj->cache_dirty &&
+ obj->base.write_domain != I915_GEM_DOMAIN_CPU &&
+ cpu_write_needs_clflush(obj) &&
+ i915_gem_clflush_object(obj, true))
+ i915_gem_chipset_flush(obj->base.dev);
return 0;
}
--
2.1.4
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH] drm/i915: Performed deferred clflush inside set-cache-level
2015-01-13 13:32 [PATCH] drm/i915: Performed deferred clflush inside set-cache-level Chris Wilson
@ 2015-01-13 15:21 ` Jani Nikula
2015-01-13 20:23 ` Ville Syrjälä
` (2 subsequent siblings)
3 siblings, 0 replies; 9+ messages in thread
From: Jani Nikula @ 2015-01-13 15:21 UTC (permalink / raw)
To: Chris Wilson, intel-gfx
On Tue, 13 Jan 2015, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> Currently we are hitting the WARN inside
> i915_gem_object_set_cache_level() as we can now have an unbound object
> in the GTT write domain (due to 43566dedde54f9 "drm/i915: Broaden
> application of set-domain(GTT)"). To avoid the warning, we need to track
> when we elided the clflush on a cacheable object and then evict the
> cache for the object when we move the object out of a cacheable domain.
>
> Reported-by: Jani Nikula <jani.nikula@linux.intel.com>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Jani Nikula <jani.nikula@linux.intel.com>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
I saw the warn when unplugging DP from a DP+eDP configuration.
Tested-by: Jani Nikula <jani.nikula@intel.com>
> ---
> drivers/gpu/drm/i915/i915_drv.h | 1 +
> drivers/gpu/drm/i915/i915_gem.c | 32 +++++++++-----------------------
> 2 files changed, 10 insertions(+), 23 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index bf18a5238887..7070482000cd 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -1998,6 +1998,7 @@ struct drm_i915_gem_object {
> */
> unsigned long gt_ro:1;
> unsigned int cache_level:3;
> + unsigned int cache_dirty:1;
>
> unsigned int has_dma_mapping:1;
>
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index 9bafe50d3df7..aa089e7c31bf 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -3639,11 +3639,14 @@ i915_gem_clflush_object(struct drm_i915_gem_object *obj,
> * snooping behaviour occurs naturally as the result of our domain
> * tracking.
> */
> - if (!force && cpu_cache_is_coherent(obj->base.dev, obj->cache_level))
> + if (!force && cpu_cache_is_coherent(obj->base.dev, obj->cache_level)) {
> + obj->cache_dirty = true;
> return false;
> + }
>
> trace_i915_gem_object_clflush(obj);
> drm_clflush_sg(obj->pages);
> + obj->cache_dirty = false;
>
> return true;
> }
> @@ -3826,28 +3829,11 @@ int i915_gem_object_set_cache_level(struct drm_i915_gem_object *obj,
> vma->node.color = cache_level;
> obj->cache_level = cache_level;
>
> - if (cpu_write_needs_clflush(obj)) {
> - u32 old_read_domains, old_write_domain;
> -
> - /* If we're coming from LLC cached, then we haven't
> - * actually been tracking whether the data is in the
> - * CPU cache or not, since we only allow one bit set
> - * in obj->write_domain and have been skipping the clflushes.
> - * Just set it to the CPU cache for now.
> - */
> - i915_gem_object_retire(obj);
> - WARN_ON(obj->base.write_domain & ~I915_GEM_DOMAIN_CPU);
> -
> - old_read_domains = obj->base.read_domains;
> - old_write_domain = obj->base.write_domain;
> -
> - obj->base.read_domains = I915_GEM_DOMAIN_CPU;
> - obj->base.write_domain = I915_GEM_DOMAIN_CPU;
> -
> - trace_i915_gem_object_change_domain(obj,
> - old_read_domains,
> - old_write_domain);
> - }
> + if (obj->cache_dirty &&
> + obj->base.write_domain != I915_GEM_DOMAIN_CPU &&
> + cpu_write_needs_clflush(obj) &&
> + i915_gem_clflush_object(obj, true))
> + i915_gem_chipset_flush(obj->base.dev);
>
> return 0;
> }
> --
> 2.1.4
>
--
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] drm/i915: Performed deferred clflush inside set-cache-level
2015-01-13 13:32 [PATCH] drm/i915: Performed deferred clflush inside set-cache-level Chris Wilson
2015-01-13 15:21 ` Jani Nikula
@ 2015-01-13 20:23 ` Ville Syrjälä
2015-01-13 20:34 ` Chris Wilson
2015-01-13 21:12 ` shuang.he
2015-01-14 19:46 ` Daniel Vetter
3 siblings, 1 reply; 9+ messages in thread
From: Ville Syrjälä @ 2015-01-13 20:23 UTC (permalink / raw)
To: Chris Wilson; +Cc: intel-gfx
On Tue, Jan 13, 2015 at 01:32:52PM +0000, Chris Wilson wrote:
> Currently we are hitting the WARN inside
> i915_gem_object_set_cache_level() as we can now have an unbound object
> in the GTT write domain (due to 43566dedde54f9 "drm/i915: Broaden
> application of set-domain(GTT)"). To avoid the warning, we need to track
> when we elided the clflush on a cacheable object and then evict the
> cache for the object when we move the object out of a cacheable domain.
>
> Reported-by: Jani Nikula <jani.nikula@linux.intel.com>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Jani Nikula <jani.nikula@linux.intel.com>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
> drivers/gpu/drm/i915/i915_drv.h | 1 +
> drivers/gpu/drm/i915/i915_gem.c | 32 +++++++++-----------------------
> 2 files changed, 10 insertions(+), 23 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index bf18a5238887..7070482000cd 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -1998,6 +1998,7 @@ struct drm_i915_gem_object {
> */
> unsigned long gt_ro:1;
> unsigned int cache_level:3;
> + unsigned int cache_dirty:1;
>
> unsigned int has_dma_mapping:1;
>
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index 9bafe50d3df7..aa089e7c31bf 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -3639,11 +3639,14 @@ i915_gem_clflush_object(struct drm_i915_gem_object *obj,
> * snooping behaviour occurs naturally as the result of our domain
> * tracking.
> */
> - if (!force && cpu_cache_is_coherent(obj->base.dev, obj->cache_level))
> + if (!force && cpu_cache_is_coherent(obj->base.dev, obj->cache_level)) {
> + obj->cache_dirty = true;
Hmm. This would mark the cache dirty even when just moving to the CPU read
domain. A subsequent set_cache_level might then flush a bit needlessly.
But maybe that's rare enoguh to ignore?
I suppose we could follow this up with another patch to fix
kms_pwrite_crc using cache_dirty as well. But actually that would
require doing the clflush even when the cache_level didn't change,
which might aggravate the needless flushing issue somewhat. Or
perhaps we should just add another cache_dirty dependent flush to
i915_gem_object_pin_to_display_plane() to deal with that particular
problem?
> return false;
> + }
>
> trace_i915_gem_object_clflush(obj);
> drm_clflush_sg(obj->pages);
> + obj->cache_dirty = false;
>
> return true;
> }
> @@ -3826,28 +3829,11 @@ int i915_gem_object_set_cache_level(struct drm_i915_gem_object *obj,
> vma->node.color = cache_level;
> obj->cache_level = cache_level;
>
> - if (cpu_write_needs_clflush(obj)) {
> - u32 old_read_domains, old_write_domain;
> -
> - /* If we're coming from LLC cached, then we haven't
> - * actually been tracking whether the data is in the
> - * CPU cache or not, since we only allow one bit set
> - * in obj->write_domain and have been skipping the clflushes.
> - * Just set it to the CPU cache for now.
> - */
> - i915_gem_object_retire(obj);
> - WARN_ON(obj->base.write_domain & ~I915_GEM_DOMAIN_CPU);
> -
> - old_read_domains = obj->base.read_domains;
> - old_write_domain = obj->base.write_domain;
> -
> - obj->base.read_domains = I915_GEM_DOMAIN_CPU;
> - obj->base.write_domain = I915_GEM_DOMAIN_CPU;
> -
> - trace_i915_gem_object_change_domain(obj,
> - old_read_domains,
> - old_write_domain);
> - }
> + if (obj->cache_dirty &&
> + obj->base.write_domain != I915_GEM_DOMAIN_CPU &&
> + cpu_write_needs_clflush(obj) &&
> + i915_gem_clflush_object(obj, true))
> + i915_gem_chipset_flush(obj->base.dev);
>
> return 0;
> }
> --
> 2.1.4
--
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] drm/i915: Performed deferred clflush inside set-cache-level
2015-01-13 20:23 ` Ville Syrjälä
@ 2015-01-13 20:34 ` Chris Wilson
0 siblings, 0 replies; 9+ messages in thread
From: Chris Wilson @ 2015-01-13 20:34 UTC (permalink / raw)
To: Ville Syrjälä; +Cc: intel-gfx
On Tue, Jan 13, 2015 at 10:23:55PM +0200, Ville Syrjälä wrote:
> On Tue, Jan 13, 2015 at 01:32:52PM +0000, Chris Wilson wrote:
> > Currently we are hitting the WARN inside
> > i915_gem_object_set_cache_level() as we can now have an unbound object
> > in the GTT write domain (due to 43566dedde54f9 "drm/i915: Broaden
> > application of set-domain(GTT)"). To avoid the warning, we need to track
> > when we elided the clflush on a cacheable object and then evict the
> > cache for the object when we move the object out of a cacheable domain.
> >
> > Reported-by: Jani Nikula <jani.nikula@linux.intel.com>
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > Cc: Jani Nikula <jani.nikula@linux.intel.com>
> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> > drivers/gpu/drm/i915/i915_drv.h | 1 +
> > drivers/gpu/drm/i915/i915_gem.c | 32 +++++++++-----------------------
> > 2 files changed, 10 insertions(+), 23 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> > index bf18a5238887..7070482000cd 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.h
> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > @@ -1998,6 +1998,7 @@ struct drm_i915_gem_object {
> > */
> > unsigned long gt_ro:1;
> > unsigned int cache_level:3;
> > + unsigned int cache_dirty:1;
> >
> > unsigned int has_dma_mapping:1;
> >
> > diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> > index 9bafe50d3df7..aa089e7c31bf 100644
> > --- a/drivers/gpu/drm/i915/i915_gem.c
> > +++ b/drivers/gpu/drm/i915/i915_gem.c
> > @@ -3639,11 +3639,14 @@ i915_gem_clflush_object(struct drm_i915_gem_object *obj,
> > * snooping behaviour occurs naturally as the result of our domain
> > * tracking.
> > */
> > - if (!force && cpu_cache_is_coherent(obj->base.dev, obj->cache_level))
> > + if (!force && cpu_cache_is_coherent(obj->base.dev, obj->cache_level)) {
> > + obj->cache_dirty = true;
>
> Hmm. This would mark the cache dirty even when just moving to the CPU read
> domain. A subsequent set_cache_level might then flush a bit needlessly.
> But maybe that's rare enoguh to ignore?
Right, we err on the side of doing an extra clflush on cache-level
transitions. I thought keeping the rule as simple as always do the
clflush if we have skipped any clflushes was a good idea. And yes,
moving dirty objects into uncached is rare, roughly once every 10s on a
bad day.
> I suppose we could follow this up with another patch to fix
> kms_pwrite_crc using cache_dirty as well. But actually that would
> require doing the clflush even when the cache_level didn't change,
> which might aggravate the needless flushing issue somewhat. Or
> perhaps we should just add another cache_dirty dependent flush to
> i915_gem_object_pin_to_display_plane() to deal with that particular
> problem?
I like the idea. I think it will help document the requirements if done
well.
-Chris
--
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] drm/i915: Performed deferred clflush inside set-cache-level
2015-01-13 13:32 [PATCH] drm/i915: Performed deferred clflush inside set-cache-level Chris Wilson
2015-01-13 15:21 ` Jani Nikula
2015-01-13 20:23 ` Ville Syrjälä
@ 2015-01-13 21:12 ` shuang.he
2015-01-14 19:46 ` Daniel Vetter
3 siblings, 0 replies; 9+ messages in thread
From: shuang.he @ 2015-01-13 21:12 UTC (permalink / raw)
To: shuang.he, ethan.gao, intel-gfx, chris
Tested-By: PRC QA PRTS (Patch Regression Test System Contact: shuang.he@intel.com)
-------------------------------------Summary-------------------------------------
Platform Delta drm-intel-nightly Series Applied
PNV 354/354 354/354
ILK 201/201 201/201
SNB +1-1 401/424 401/424
IVB -1 488/488 487/488
BYT 278/278 278/278
HSW -41 529/529 488/529
BDW -1 405/405 404/405
-------------------------------------Detailed-------------------------------------
Platform Test drm-intel-nightly Series Applied
SNB igt_kms_flip_flip-vs-dpms-off-vs-modeset-interruptible NSPT(1, M35)PASS(11, M35M22) PASS(1, M22)
*SNB igt_gem_concurrent_blit_gtt-rcs-early-read-interruptible PASS(12, M35M22) DMESG_WARN(1, M22)
*IVB igt_gem_userptr_blits_forked-sync-normal PASS(2, M21) DMESG_WARN(1, M21)
HSW igt_kms_cursor_crc_cursor-size-change NSPT(7, M40M19)PASS(1, M20) NSPT(1, M40)
HSW igt_kms_fence_pin_leak NSPT(7, M40M19)PASS(1, M20) NSPT(1, M40)
HSW igt_kms_flip_event_leak NSPT(6, M40M19)PASS(1, M20) NSPT(1, M40)
HSW igt_kms_mmio_vs_cs_flip_setcrtc_vs_cs_flip NSPT(7, M40M19)PASS(1, M20) NSPT(1, M40)
HSW igt_kms_mmio_vs_cs_flip_setplane_vs_cs_flip NSPT(7, M40M19)PASS(1, M20) NSPT(1, M40)
HSW igt_pm_lpsp_non-edp NSPT(7, M40M19)PASS(1, M20) NSPT(1, M40)
HSW igt_pm_rpm_cursor NSPT(7, M40M19)PASS(1, M20) NSPT(1, M40)
HSW igt_pm_rpm_cursor-dpms NSPT(7, M40M19)PASS(1, M20) NSPT(1, M40)
HSW igt_pm_rpm_dpms-mode-unset-non-lpsp NSPT(7, M40M19)PASS(1, M20) NSPT(1, M40)
HSW igt_pm_rpm_dpms-non-lpsp NSPT(7, M40M19)PASS(1, M20) NSPT(1, M40)
HSW igt_pm_rpm_drm-resources-equal NSPT(7, M40M19)PASS(1, M20) NSPT(1, M40)
HSW igt_pm_rpm_fences NSPT(7, M40M19)PASS(1, M20) NSPT(1, M40)
HSW igt_pm_rpm_fences-dpms NSPT(7, M40M19)PASS(1, M20) NSPT(1, M40)
HSW igt_pm_rpm_gem-execbuf NSPT(7, M40M19)PASS(1, M20) NSPT(1, M40)
HSW igt_pm_rpm_gem-mmap-cpu NSPT(7, M40M19)PASS(1, M20) NSPT(1, M40)
HSW igt_pm_rpm_gem-mmap-gtt NSPT(7, M40M19)PASS(1, M20) NSPT(1, M40)
HSW igt_pm_rpm_gem-pread NSPT(7, M40M19)PASS(1, M20) NSPT(1, M40)
HSW igt_pm_rpm_i2c NSPT(7, M40M19)PASS(1, M20) NSPT(1, M40)
HSW igt_pm_rpm_modeset-non-lpsp NSPT(7, M40M19)PASS(1, M20) NSPT(1, M40)
HSW igt_pm_rpm_modeset-non-lpsp-stress-no-wait NSPT(7, M40M19)PASS(1, M20) NSPT(1, M40)
HSW igt_pm_rpm_pci-d3-state NSPT(7, M40M19)PASS(1, M20) NSPT(1, M40)
HSW igt_pm_rpm_rte NSPT(7, M40M19)PASS(1, M20) NSPT(1, M40)
HSW igt_gem_concurrent_blit_gtt-bcs-early-read-forked DMESG_WARN(7, M40M19)PASS(1, M20) DMESG_WARN(1, M40)
HSW igt_gem_concurrent_blit_gtt-bcs-early-read-interruptible DMESG_WARN(7, M40M19)PASS(1, M20) DMESG_WARN(1, M40)
HSW igt_gem_concurrent_blit_gtt-bcs-gpu-read-after-write-forked DMESG_WARN(7, M40M19)PASS(1, M20) DMESG_WARN(1, M40)
HSW igt_gem_concurrent_blit_gtt-bcs-gpu-read-after-write-interruptible DMESG_WARN(7, M40M19)PASS(1, M20) DMESG_WARN(1, M40)
HSW igt_gem_concurrent_blit_gtt-bcs-overwrite-source-forked DMESG_WARN(7, M40M19)PASS(1, M20) DMESG_WARN(1, M40)
HSW igt_gem_concurrent_blit_gtt-bcs-overwrite-source-interruptible DMESG_WARN(6, M40M19)PASS(1, M20) DMESG_WARN(1, M40)
HSW igt_gem_concurrent_blit_gtt-rcs-early-read-forked DMESG_WARN(7, M40M19)PASS(1, M20) DMESG_WARN(1, M40)
HSW igt_gem_concurrent_blit_gtt-rcs-early-read-interruptible DMESG_WARN(7, M40M19)PASS(1, M20) DMESG_WARN(1, M40)
HSW igt_gem_concurrent_blit_gtt-rcs-gpu-read-after-write-forked DMESG_WARN(7, M40M19)PASS(1, M20) DMESG_WARN(1, M40)
HSW igt_gem_concurrent_blit_gtt-rcs-gpu-read-after-write-interruptible DMESG_WARN(7, M40M19)PASS(1, M20) DMESG_WARN(1, M40)
HSW igt_gem_concurrent_blit_gtt-rcs-overwrite-source-forked DMESG_WARN(7, M40M19)PASS(1, M20) DMESG_WARN(1, M40)
HSW igt_gem_concurrent_blit_gtt-rcs-overwrite-source-interruptible DMESG_WARN(7, M40M19)PASS(1, M20) DMESG_WARN(1, M40)
HSW igt_gem_concurrent_blit_gttX-bcs-gpu-read-after-write-interruptible DMESG_WARN(7, M40M19)PASS(1, M20) DMESG_WARN(1, M40)
HSW igt_gem_concurrent_blit_gttX-bcs-overwrite-source-forked DMESG_WARN(7, M40M19)PASS(1, M20) DMESG_WARN(1, M40)
HSW igt_gem_concurrent_blit_gttX-bcs-overwrite-source-interruptible DMESG_WARN(7, M40M19)PASS(1, M20) DMESG_WARN(1, M40)
HSW igt_gem_concurrent_blit_gttX-rcs-early-read-interruptible DMESG_WARN(7, M40M19)PASS(1, M20) DMESG_WARN(1, M40)
HSW igt_gem_concurrent_blit_gttX-rcs-gpu-read-after-write-interruptible DMESG_WARN(6, M40M19)PASS(2, M20M19) DMESG_WARN(1, M40)
HSW igt_gem_concurrent_blit_gttX-rcs-overwrite-source-forked DMESG_WARN(7, M40M19)PASS(1, M20) DMESG_WARN(1, M40)
HSW igt_gem_concurrent_blit_gttX-rcs-overwrite-source-interruptible DMESG_WARN(4, M40M19)PASS(2, M20M40) DMESG_WARN(1, M40)
*BDW igt_gem_concurrent_blit_gtt-bcs-gpu-read-after-write-interruptible PASS(11, M30M28) DMESG_WARN(1, M28)
Note: You need to pay more attention to line start with '*'
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] drm/i915: Performed deferred clflush inside set-cache-level
2015-01-13 13:32 [PATCH] drm/i915: Performed deferred clflush inside set-cache-level Chris Wilson
` (2 preceding siblings ...)
2015-01-13 21:12 ` shuang.he
@ 2015-01-14 19:46 ` Daniel Vetter
2015-01-15 8:54 ` Chris Wilson
3 siblings, 1 reply; 9+ messages in thread
From: Daniel Vetter @ 2015-01-14 19:46 UTC (permalink / raw)
To: Chris Wilson; +Cc: intel-gfx
On Tue, Jan 13, 2015 at 01:32:52PM +0000, Chris Wilson wrote:
> Currently we are hitting the WARN inside
> i915_gem_object_set_cache_level() as we can now have an unbound object
> in the GTT write domain (due to 43566dedde54f9 "drm/i915: Broaden
> application of set-domain(GTT)"). To avoid the warning, we need to track
> when we elided the clflush on a cacheable object and then evict the
> cache for the object when we move the object out of a cacheable domain.
>
> Reported-by: Jani Nikula <jani.nikula@linux.intel.com>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Jani Nikula <jani.nikula@linux.intel.com>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
> drivers/gpu/drm/i915/i915_drv.h | 1 +
> drivers/gpu/drm/i915/i915_gem.c | 32 +++++++++-----------------------
> 2 files changed, 10 insertions(+), 23 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index bf18a5238887..7070482000cd 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -1998,6 +1998,7 @@ struct drm_i915_gem_object {
> */
> unsigned long gt_ro:1;
> unsigned int cache_level:3;
> + unsigned int cache_dirty:1;
>
> unsigned int has_dma_mapping:1;
>
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index 9bafe50d3df7..aa089e7c31bf 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -3639,11 +3639,14 @@ i915_gem_clflush_object(struct drm_i915_gem_object *obj,
> * snooping behaviour occurs naturally as the result of our domain
> * tracking.
> */
> - if (!force && cpu_cache_is_coherent(obj->base.dev, obj->cache_level))
> + if (!force && cpu_cache_is_coherent(obj->base.dev, obj->cache_level)) {
> + obj->cache_dirty = true;
> return false;
> + }
>
> trace_i915_gem_object_clflush(obj);
> drm_clflush_sg(obj->pages);
> + obj->cache_dirty = false;
>
> return true;
> }
> @@ -3826,28 +3829,11 @@ int i915_gem_object_set_cache_level(struct drm_i915_gem_object *obj,
> vma->node.color = cache_level;
> obj->cache_level = cache_level;
>
> - if (cpu_write_needs_clflush(obj)) {
> - u32 old_read_domains, old_write_domain;
> -
> - /* If we're coming from LLC cached, then we haven't
> - * actually been tracking whether the data is in the
> - * CPU cache or not, since we only allow one bit set
> - * in obj->write_domain and have been skipping the clflushes.
> - * Just set it to the CPU cache for now.
> - */
> - i915_gem_object_retire(obj);
> - WARN_ON(obj->base.write_domain & ~I915_GEM_DOMAIN_CPU);
> -
> - old_read_domains = obj->base.read_domains;
> - old_write_domain = obj->base.write_domain;
> -
> - obj->base.read_domains = I915_GEM_DOMAIN_CPU;
> - obj->base.write_domain = I915_GEM_DOMAIN_CPU;
> -
> - trace_i915_gem_object_change_domain(obj,
> - old_read_domains,
> - old_write_domain);
> - }
> + if (obj->cache_dirty &&
> + obj->base.write_domain != I915_GEM_DOMAIN_CPU &&
> + cpu_write_needs_clflush(obj) &&
> + i915_gem_clflush_object(obj, true))
Imo hiding the actual action in the if condition like this is a bit too
evil. Also, can we please have a testcase to at lest exercise the
codepath? It sounds like a real functional tests using crc is a bit more
work, but just poking at the WARN_ON would be good already.
-Daniel
> + i915_gem_chipset_flush(obj->base.dev);
>
> return 0;
> }
> --
> 2.1.4
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] drm/i915: Performed deferred clflush inside set-cache-level
2015-01-14 19:46 ` Daniel Vetter
@ 2015-01-15 8:54 ` Chris Wilson
2015-01-21 9:31 ` Chris Wilson
2015-01-21 12:26 ` Daniel Vetter
0 siblings, 2 replies; 9+ messages in thread
From: Chris Wilson @ 2015-01-15 8:54 UTC (permalink / raw)
To: Daniel Vetter; +Cc: intel-gfx
On Wed, Jan 14, 2015 at 08:46:09PM +0100, Daniel Vetter wrote:
> > + if (obj->cache_dirty &&
> > + obj->base.write_domain != I915_GEM_DOMAIN_CPU &&
> > + cpu_write_needs_clflush(obj) &&
> > + i915_gem_clflush_object(obj, true))
>
> Imo hiding the actual action in the if condition like this is a bit too
> evil.
Split it out into 2 ifs:
if (cache_dirty && !not-in-cpu-cache && needs_clflush)
if (i915_gem_clflush_object(obj, true))
i915_gem_chipset_flush();
Moving the chipset_flush around can tidy this up (at the expense of some
brain power to only do the flush when required).
> Also, can we please have a testcase to at lest exercise the
> codepath? It sounds like a real functional tests using crc is a bit more
> work, but just poking at the WARN_ON would be good already.
Testcase: igt/gem_mmap_wc/set-cache-level
-Chris
--
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] drm/i915: Performed deferred clflush inside set-cache-level
2015-01-15 8:54 ` Chris Wilson
@ 2015-01-21 9:31 ` Chris Wilson
2015-01-21 12:26 ` Daniel Vetter
1 sibling, 0 replies; 9+ messages in thread
From: Chris Wilson @ 2015-01-21 9:31 UTC (permalink / raw)
To: Daniel Vetter, intel-gfx
On Thu, Jan 15, 2015 at 08:54:54AM +0000, Chris Wilson wrote:
> On Wed, Jan 14, 2015 at 08:46:09PM +0100, Daniel Vetter wrote:
> > Also, can we please have a testcase to at lest exercise the
> > codepath? It sounds like a real functional tests using crc is a bit more
> > work, but just poking at the WARN_ON would be good already.
>
> Testcase: igt/gem_mmap_wc/set-cache-level
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=88607
Tested-by: huax.lu@intel.com
-Chris
--
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] drm/i915: Performed deferred clflush inside set-cache-level
2015-01-15 8:54 ` Chris Wilson
2015-01-21 9:31 ` Chris Wilson
@ 2015-01-21 12:26 ` Daniel Vetter
1 sibling, 0 replies; 9+ messages in thread
From: Daniel Vetter @ 2015-01-21 12:26 UTC (permalink / raw)
To: Chris Wilson, Daniel Vetter, intel-gfx
On Thu, Jan 15, 2015 at 08:54:54AM +0000, Chris Wilson wrote:
> On Wed, Jan 14, 2015 at 08:46:09PM +0100, Daniel Vetter wrote:
> > > + if (obj->cache_dirty &&
> > > + obj->base.write_domain != I915_GEM_DOMAIN_CPU &&
> > > + cpu_write_needs_clflush(obj) &&
> > > + i915_gem_clflush_object(obj, true))
> >
> > Imo hiding the actual action in the if condition like this is a bit too
> > evil.
>
> Split it out into 2 ifs:
>
> if (cache_dirty && !not-in-cpu-cache && needs_clflush)
> if (i915_gem_clflush_object(obj, true))
> i915_gem_chipset_flush();
With that applied this is Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
>
> Moving the chipset_flush around can tidy this up (at the expense of some
> brain power to only do the flush when required).
Wrt cleanups I think more clarity would come from pushing the decision
whether the clflush should be forced (with the bool force argument for
both clflush_object and flush_cpu_write_domain) and replace it with a bool
write. The clflush_object could switch between cpu_cache_is_coherent and
cpu_write_needs_clflush. Probably we could even inline the later.
Or do I miss something?
-Daniel
>
> > Also, can we please have a testcase to at lest exercise the
> > codepath? It sounds like a real functional tests using crc is a bit more
> > work, but just poking at the WARN_ON would be good already.
>
> Testcase: igt/gem_mmap_wc/set-cache-level
> -Chris
>
> --
> Chris Wilson, Intel Open Source Technology Centre
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2015-01-21 12:26 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-01-13 13:32 [PATCH] drm/i915: Performed deferred clflush inside set-cache-level Chris Wilson
2015-01-13 15:21 ` Jani Nikula
2015-01-13 20:23 ` Ville Syrjälä
2015-01-13 20:34 ` Chris Wilson
2015-01-13 21:12 ` shuang.he
2015-01-14 19:46 ` Daniel Vetter
2015-01-15 8:54 ` Chris Wilson
2015-01-21 9:31 ` Chris Wilson
2015-01-21 12:26 ` Daniel Vetter
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.