[PATCH] drm/i915: Performed deferred clflush inside set-cache-level

[PATCH] drm/i915: Performed deferred clflush inside set-cache-level

[Chris Wilson]

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

Re: [PATCH] drm/i915: Performed deferred clflush inside set-cache-level

[Jani Nikula]

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

Re: [PATCH] drm/i915: Performed deferred clflush inside set-cache-level

[Ville Syrjälä]

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

Re: [PATCH] drm/i915: Performed deferred clflush inside set-cache-level

[Chris Wilson]

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

Re: [PATCH] drm/i915: Performed deferred clflush inside set-cache-level

[shuang.he]

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

Re: [PATCH] drm/i915: Performed deferred clflush inside set-cache-level

[Daniel Vetter]

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

Re: [PATCH] drm/i915: Performed deferred clflush inside set-cache-level

[Chris Wilson]

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

Re: [PATCH] drm/i915: Performed deferred clflush inside set-cache-level

[Chris Wilson]

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

Re: [PATCH] drm/i915: Performed deferred clflush inside set-cache-level

[Daniel Vetter]

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