[PATCH] drm/i915: clflush on pin_to_display after pwrite to UC bo in LLC

[PATCH] drm/i915: clflush on pin_to_display after pwrite to UC bo in LLC

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Currently we don't clflush on pin_to_display if the bo is already
UC/WT and is not in the CPU write domain. This causes problems with
pwrite since pwrite doesn't change the write domain, and it avoids
clflushing on UC/WT buffers on LLC platforms unless the buffer is
currently being scanned out.

Fix the problem by marking the cache dirty and adjusting
i915_gem_object_set_cache_level() to clflush when the cache is dirty
even if the cache_level doesn't change.

My last attempt [1] at fixing this via write domain frobbing was shot
down, but now with the cache_dirty flag we can do things in a nicer way.

[1] http://lists.freedesktop.org/archives/intel-gfx/2014-November/055390.html

Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=86422
Testcase: igt/kms_pwrite_crc
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_gem.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 73293b4..73eff2e 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -1005,12 +1005,15 @@ out:
 		if (!needs_clflush_after &&
 		    obj->base.write_domain != I915_GEM_DOMAIN_CPU) {
 			if (i915_gem_clflush_object(obj, obj->pin_display))
-				i915_gem_chipset_flush(dev);
+				needs_clflush_after = true;
 		}
 	}
 
 	if (needs_clflush_after)
 		i915_gem_chipset_flush(dev);
+	else if (obj->cache_level == I915_CACHE_NONE ||
+		 obj->cache_level == I915_CACHE_WT)
+		obj->cache_dirty = true;
 
 	intel_fb_obj_flush(obj, false, ORIGIN_CPU);
 	return ret;
@@ -3639,10 +3642,10 @@ int i915_gem_object_set_cache_level(struct drm_i915_gem_object *obj,
 {
 	struct drm_device *dev = obj->base.dev;
 	struct i915_vma *vma, *next;
-	int ret;
+	int ret = 0;
 
 	if (obj->cache_level == cache_level)
-		return 0;
+		goto out;
 
 	if (i915_gem_obj_is_pinned(obj)) {
 		DRM_DEBUG("can not change the cache level of pinned objects\n");
@@ -3687,6 +3690,7 @@ int i915_gem_object_set_cache_level(struct drm_i915_gem_object *obj,
 		vma->node.color = cache_level;
 	obj->cache_level = cache_level;
 
+out:
 	if (obj->cache_dirty &&
 	    obj->base.write_domain != I915_GEM_DOMAIN_CPU &&
 	    cpu_write_needs_clflush(obj)) {
-- 
2.4.6

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH i-g-t] tests/kms_pwrite_crc: Use drmModeSetPlane() instead of igt_plane_set_fb()

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

igt_plane_set_fb()+igt_display_commit() have too much overhead, and that
causes the cache to get flushed before we flip, making the test
useless, at least on machines with small LLC. Switch to
drmModeSetPlane() to reduce the chance that the cache gets flushed
before we grab the crc.

Still nowhere near 100% reliable on my IVB laptop with 3 MiB LLC,
but at least it can now hit the problem occasioanally. My desktop
IVB with 8 MiB LLC seems to hit it rather reliably.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 tests/kms_pwrite_crc.c | 25 ++++++++++++++++++-------
 1 file changed, 18 insertions(+), 7 deletions(-)

diff --git a/tests/kms_pwrite_crc.c b/tests/kms_pwrite_crc.c
index 05b9e38..990866f 100644
--- a/tests/kms_pwrite_crc.c
+++ b/tests/kms_pwrite_crc.c
@@ -52,7 +52,6 @@ typedef struct {
 
 static void test(data_t *data)
 {
-	igt_display_t *display = &data->display;
 	igt_output_t *output = data->output;
 	struct igt_fb *fb = &data->fb[1];
 	drmModeModeInfo *mode;
@@ -72,12 +71,20 @@ static void test(data_t *data)
 	cairo_destroy(cr);
 
 	/* flip to it to make it UC/WC and fully flushed */
-	igt_plane_set_fb(data->primary, fb);
-	igt_display_commit(display);
+	drmModeSetPlane(data->drm_fd,
+			data->primary->drm_plane->plane_id,
+			output->config.crtc->crtc_id,
+			fb->fb_id, 0,
+			0, 0, fb->width, fb->height,
+			0, 0, fb->width << 16, fb->height << 16);
 
 	/* flip back the original white buffer */
-	igt_plane_set_fb(data->primary, &data->fb[0]);
-	igt_display_commit(display);
+	drmModeSetPlane(data->drm_fd,
+			data->primary->drm_plane->plane_id,
+			output->config.crtc->crtc_id,
+			data->fb[0].fb_id, 0,
+			0, 0, fb->width, fb->height,
+			0, 0, fb->width << 16, fb->height << 16);
 
 	/* make sure caching mode has become UC/WT */
 	caching = gem_get_caching(data->drm_fd, fb->gem_handle);
@@ -91,8 +98,12 @@ static void test(data_t *data)
 	free(buf);
 
 	/* and flip to it */
-	igt_plane_set_fb(data->primary, fb);
-	igt_display_commit(display);
+	drmModeSetPlane(data->drm_fd,
+			data->primary->drm_plane->plane_id,
+			output->config.crtc->crtc_id,
+			fb->fb_id, 0,
+			0, 0, fb->width, fb->height,
+			0, 0, fb->width << 16, fb->height << 16);
 
 	/* check that the crc is as expected, which requires that caches got flushed */
 	igt_pipe_crc_collect_crc(data->pipe_crc, &crc);
-- 
2.4.6

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: clflush on pin_to_display after pwrite to UC bo in LLC

[Chris Wilson]

On Tue, Aug 11, 2015 at 04:59:14PM +0300, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> Currently we don't clflush on pin_to_display if the bo is already
> UC/WT and is not in the CPU write domain. This causes problems with
> pwrite since pwrite doesn't change the write domain, and it avoids
> clflushing on UC/WT buffers on LLC platforms unless the buffer is
> currently being scanned out.
> 
> Fix the problem by marking the cache dirty and adjusting
> i915_gem_object_set_cache_level() to clflush when the cache is dirty
> even if the cache_level doesn't change.
> 
> My last attempt [1] at fixing this via write domain frobbing was shot
> down, but now with the cache_dirty flag we can do things in a nicer way.
> 
> [1] http://lists.freedesktop.org/archives/intel-gfx/2014-November/055390.html
> 
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=86422
> Testcase: igt/kms_pwrite_crc
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/i915_gem.c | 10 +++++++---
>  1 file changed, 7 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index 73293b4..73eff2e 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -1005,12 +1005,15 @@ out:
>  		if (!needs_clflush_after &&
>  		    obj->base.write_domain != I915_GEM_DOMAIN_CPU) {
>  			if (i915_gem_clflush_object(obj, obj->pin_display))
> -				i915_gem_chipset_flush(dev);
> +				needs_clflush_after = true;
>  		}
>  	}
>  
>  	if (needs_clflush_after)
>  		i915_gem_chipset_flush(dev);
> +	else if (obj->cache_level == I915_CACHE_NONE ||
> +		 obj->cache_level == I915_CACHE_WT)
> +		obj->cache_dirty = true;
>  
>  	intel_fb_obj_flush(obj, false, ORIGIN_CPU);
>  	return ret;

Ok, this seems reasonable.

> @@ -3639,10 +3642,10 @@ int i915_gem_object_set_cache_level(struct drm_i915_gem_object *obj,
>  {
>  	struct drm_device *dev = obj->base.dev;
>  	struct i915_vma *vma, *next;
> -	int ret;
> +	int ret = 0;
>  
>  	if (obj->cache_level == cache_level)
> -		return 0;
> +		goto out;
>  
>  	if (i915_gem_obj_is_pinned(obj)) {
>  		DRM_DEBUG("can not change the cache level of pinned objects\n");
> @@ -3687,6 +3690,7 @@ int i915_gem_object_set_cache_level(struct drm_i915_gem_object *obj,
>  		vma->node.color = cache_level;
>  	obj->cache_level = cache_level;
>  
> +out:
>  	if (obj->cache_dirty &&
>  	    obj->base.write_domain != I915_GEM_DOMAIN_CPU &&
>  	    cpu_write_needs_clflush(obj)) {

This we can do better (and I know I am guilty of the original sin). It
just feels a little too loose that we expect pin-to-display-plane should
always call set-cache-level (yes, it has to, but it feels like we are
putting pin-to-display-plane specifics into set-cache-level).

I think this chunk is much more understandable if we did:

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 5e7fcf77e57a..fc6bcc19cca1 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -3656,13 +3656,6 @@ int i915_gem_object_set_cache_level(struct drm_i915_gem_object *obj,
                vma->node.color = cache_level;
        obj->cache_level = cache_level;
 
-       if (obj->cache_dirty &&
-           obj->base.write_domain != I915_GEM_DOMAIN_CPU &&
-           cpu_write_needs_clflush(obj)) {
-               if (i915_gem_clflush_object(obj, true))
-                       i915_gem_chipset_flush(obj->base.dev);
-       }
-
        return 0;
 }
 
@@ -3795,6 +3788,10 @@ i915_gem_object_pin_to_display_plane(struct drm_i915_gem_object *obj,
        WARN_ON(obj->pin_display > vma->pin_count);
 
        i915_gem_object_flush_cpu_write_domain(obj);
+       if (obj->cache_dirty) {
+               if (i915_gem_clflush_object(obj, true))
+                       i915_gem_chipset_flush(obj->base.dev);
+       }
 
        /* It should now be out of any other write domains, and we can update
         * the domain values for our changes.

Maybe even

/* Whilst the object was away from the scanout we may have been eliding the coherent
 * writes into the CPU cache. However, the moment it has to be read by the display
 * engine, those writes into the CPU cache become inaccessible and so we have to 
 * clflush them out to main memory. We track elided flushes with obj->cache_dirty
 * and hope they are rare.
 */


The other user of set-cache-level (set_caching_ioctl) should not care
about the clflush side-effect and the clflush elision should work just
fine instead.

Either way,
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
but I'd prefer a v2 with the comments :)
-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: clflush on pin_to_display after pwrite to UC bo in LLC

[Ville Syrjälä]

On Tue, Aug 11, 2015 at 03:28:27PM +0100, Chris Wilson wrote:
> On Tue, Aug 11, 2015 at 04:59:14PM +0300, ville.syrjala@linux.intel.com wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > Currently we don't clflush on pin_to_display if the bo is already
> > UC/WT and is not in the CPU write domain. This causes problems with
> > pwrite since pwrite doesn't change the write domain, and it avoids
> > clflushing on UC/WT buffers on LLC platforms unless the buffer is
> > currently being scanned out.
> > 
> > Fix the problem by marking the cache dirty and adjusting
> > i915_gem_object_set_cache_level() to clflush when the cache is dirty
> > even if the cache_level doesn't change.
> > 
> > My last attempt [1] at fixing this via write domain frobbing was shot
> > down, but now with the cache_dirty flag we can do things in a nicer way.
> > 
> > [1] http://lists.freedesktop.org/archives/intel-gfx/2014-November/055390.html
> > 
> > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=86422
> > Testcase: igt/kms_pwrite_crc
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_gem.c | 10 +++++++---
> >  1 file changed, 7 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> > index 73293b4..73eff2e 100644
> > --- a/drivers/gpu/drm/i915/i915_gem.c
> > +++ b/drivers/gpu/drm/i915/i915_gem.c
> > @@ -1005,12 +1005,15 @@ out:
> >  		if (!needs_clflush_after &&
> >  		    obj->base.write_domain != I915_GEM_DOMAIN_CPU) {
> >  			if (i915_gem_clflush_object(obj, obj->pin_display))
> > -				i915_gem_chipset_flush(dev);
> > +				needs_clflush_after = true;
> >  		}
> >  	}
> >  
> >  	if (needs_clflush_after)
> >  		i915_gem_chipset_flush(dev);
> > +	else if (obj->cache_level == I915_CACHE_NONE ||
> > +		 obj->cache_level == I915_CACHE_WT)
> > +		obj->cache_dirty = true;
> >  
> >  	intel_fb_obj_flush(obj, false, ORIGIN_CPU);
> >  	return ret;
> 
> Ok, this seems reasonable.
> 
> > @@ -3639,10 +3642,10 @@ int i915_gem_object_set_cache_level(struct drm_i915_gem_object *obj,
> >  {
> >  	struct drm_device *dev = obj->base.dev;
> >  	struct i915_vma *vma, *next;
> > -	int ret;
> > +	int ret = 0;
> >  
> >  	if (obj->cache_level == cache_level)
> > -		return 0;
> > +		goto out;
> >  
> >  	if (i915_gem_obj_is_pinned(obj)) {
> >  		DRM_DEBUG("can not change the cache level of pinned objects\n");
> > @@ -3687,6 +3690,7 @@ int i915_gem_object_set_cache_level(struct drm_i915_gem_object *obj,
> >  		vma->node.color = cache_level;
> >  	obj->cache_level = cache_level;
> >  
> > +out:
> >  	if (obj->cache_dirty &&
> >  	    obj->base.write_domain != I915_GEM_DOMAIN_CPU &&
> >  	    cpu_write_needs_clflush(obj)) {
> 
> This we can do better (and I know I am guilty of the original sin). It
> just feels a little too loose that we expect pin-to-display-plane should
> always call set-cache-level (yes, it has to, but it feels like we are
> putting pin-to-display-plane specifics into set-cache-level).
> 
> I think this chunk is much more understandable if we did:
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index 5e7fcf77e57a..fc6bcc19cca1 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -3656,13 +3656,6 @@ int i915_gem_object_set_cache_level(struct drm_i915_gem_object *obj,
>                 vma->node.color = cache_level;
>         obj->cache_level = cache_level;
>  
> -       if (obj->cache_dirty &&
> -           obj->base.write_domain != I915_GEM_DOMAIN_CPU &&
> -           cpu_write_needs_clflush(obj)) {
> -               if (i915_gem_clflush_object(obj, true))
> -                       i915_gem_chipset_flush(obj->base.dev);
> -       }
> -
>         return 0;
>  }
>  
> @@ -3795,6 +3788,10 @@ i915_gem_object_pin_to_display_plane(struct drm_i915_gem_object *obj,
>         WARN_ON(obj->pin_display > vma->pin_count);
>  
>         i915_gem_object_flush_cpu_write_domain(obj);
> +       if (obj->cache_dirty) {
> +               if (i915_gem_clflush_object(obj, true))
> +                       i915_gem_chipset_flush(obj->base.dev);
> +       }
>  
>         /* It should now be out of any other write domains, and we can update
>          * the domain values for our changes.
> 
> Maybe even
> 
> /* Whilst the object was away from the scanout we may have been eliding the coherent
>  * writes into the CPU cache. However, the moment it has to be read by the display
>  * engine, those writes into the CPU cache become inaccessible and so we have to 
>  * clflush them out to main memory. We track elided flushes with obj->cache_dirty
>  * and hope they are rare.
>  */
> 
> 
> The other user of set-cache-level (set_caching_ioctl) should not care
> about the clflush side-effect and the clflush elision should work just
> fine instead.

Hmm. So what would happen on !LLC if we start with a cached bo, then pwrite it
and afterwards make it uncached?

> 
> Either way,
> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
> but I'd prefer a v2 with the comments :)
> -Chris
> 
> -- 
> Chris Wilson, Intel Open Source Technology Centre

-- 
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: clflush on pin_to_display after pwrite to UC bo in LLC

[Ville Syrjälä]

On Tue, Aug 11, 2015 at 05:56:28PM +0300, Ville Syrjälä wrote:
> On Tue, Aug 11, 2015 at 03:28:27PM +0100, Chris Wilson wrote:
> > On Tue, Aug 11, 2015 at 04:59:14PM +0300, ville.syrjala@linux.intel.com wrote:
> > > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > 
> > > Currently we don't clflush on pin_to_display if the bo is already
> > > UC/WT and is not in the CPU write domain. This causes problems with
> > > pwrite since pwrite doesn't change the write domain, and it avoids
> > > clflushing on UC/WT buffers on LLC platforms unless the buffer is
> > > currently being scanned out.
> > > 
> > > Fix the problem by marking the cache dirty and adjusting
> > > i915_gem_object_set_cache_level() to clflush when the cache is dirty
> > > even if the cache_level doesn't change.
> > > 
> > > My last attempt [1] at fixing this via write domain frobbing was shot
> > > down, but now with the cache_dirty flag we can do things in a nicer way.
> > > 
> > > [1] http://lists.freedesktop.org/archives/intel-gfx/2014-November/055390.html
> > > 
> > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=86422
> > > Testcase: igt/kms_pwrite_crc
> > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/i915_gem.c | 10 +++++++---
> > >  1 file changed, 7 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> > > index 73293b4..73eff2e 100644
> > > --- a/drivers/gpu/drm/i915/i915_gem.c
> > > +++ b/drivers/gpu/drm/i915/i915_gem.c
> > > @@ -1005,12 +1005,15 @@ out:
> > >  		if (!needs_clflush_after &&
> > >  		    obj->base.write_domain != I915_GEM_DOMAIN_CPU) {
> > >  			if (i915_gem_clflush_object(obj, obj->pin_display))
> > > -				i915_gem_chipset_flush(dev);
> > > +				needs_clflush_after = true;
> > >  		}
> > >  	}
> > >  
> > >  	if (needs_clflush_after)
> > >  		i915_gem_chipset_flush(dev);
> > > +	else if (obj->cache_level == I915_CACHE_NONE ||
> > > +		 obj->cache_level == I915_CACHE_WT)
> > > +		obj->cache_dirty = true;
> > >  
> > >  	intel_fb_obj_flush(obj, false, ORIGIN_CPU);
> > >  	return ret;
> > 
> > Ok, this seems reasonable.
> > 
> > > @@ -3639,10 +3642,10 @@ int i915_gem_object_set_cache_level(struct drm_i915_gem_object *obj,
> > >  {
> > >  	struct drm_device *dev = obj->base.dev;
> > >  	struct i915_vma *vma, *next;
> > > -	int ret;
> > > +	int ret = 0;
> > >  
> > >  	if (obj->cache_level == cache_level)
> > > -		return 0;
> > > +		goto out;
> > >  
> > >  	if (i915_gem_obj_is_pinned(obj)) {
> > >  		DRM_DEBUG("can not change the cache level of pinned objects\n");
> > > @@ -3687,6 +3690,7 @@ int i915_gem_object_set_cache_level(struct drm_i915_gem_object *obj,
> > >  		vma->node.color = cache_level;
> > >  	obj->cache_level = cache_level;
> > >  
> > > +out:
> > >  	if (obj->cache_dirty &&
> > >  	    obj->base.write_domain != I915_GEM_DOMAIN_CPU &&
> > >  	    cpu_write_needs_clflush(obj)) {
> > 
> > This we can do better (and I know I am guilty of the original sin). It
> > just feels a little too loose that we expect pin-to-display-plane should
> > always call set-cache-level (yes, it has to, but it feels like we are
> > putting pin-to-display-plane specifics into set-cache-level).
> > 
> > I think this chunk is much more understandable if we did:
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> > index 5e7fcf77e57a..fc6bcc19cca1 100644
> > --- a/drivers/gpu/drm/i915/i915_gem.c
> > +++ b/drivers/gpu/drm/i915/i915_gem.c
> > @@ -3656,13 +3656,6 @@ int i915_gem_object_set_cache_level(struct drm_i915_gem_object *obj,
> >                 vma->node.color = cache_level;
> >         obj->cache_level = cache_level;
> >  
> > -       if (obj->cache_dirty &&
> > -           obj->base.write_domain != I915_GEM_DOMAIN_CPU &&
> > -           cpu_write_needs_clflush(obj)) {
> > -               if (i915_gem_clflush_object(obj, true))
> > -                       i915_gem_chipset_flush(obj->base.dev);
> > -       }
> > -
> >         return 0;
> >  }
> >  
> > @@ -3795,6 +3788,10 @@ i915_gem_object_pin_to_display_plane(struct drm_i915_gem_object *obj,
> >         WARN_ON(obj->pin_display > vma->pin_count);
> >  
> >         i915_gem_object_flush_cpu_write_domain(obj);
> > +       if (obj->cache_dirty) {
> > +               if (i915_gem_clflush_object(obj, true))
> > +                       i915_gem_chipset_flush(obj->base.dev);
> > +       }
> >  
> >         /* It should now be out of any other write domains, and we can update
> >          * the domain values for our changes.
> > 
> > Maybe even
> > 
> > /* Whilst the object was away from the scanout we may have been eliding the coherent
> >  * writes into the CPU cache. However, the moment it has to be read by the display
> >  * engine, those writes into the CPU cache become inaccessible and so we have to 
> >  * clflush them out to main memory. We track elided flushes with obj->cache_dirty
> >  * and hope they are rare.
> >  */
> > 
> > 
> > The other user of set-cache-level (set_caching_ioctl) should not care
> > about the clflush side-effect and the clflush elision should work just
> > fine instead.
> 
> Hmm. So what would happen on !LLC if we start with a cached bo, then pwrite it
> and afterwards make it uncached?

In fact that would still fail even with my patch, and wouldn't work with current
upstream code either. To fix that I'd need to drop the I915_CACHE_NONE/WT checks
from pwrite in my patch and always set cache_dirty=true when it didn't
clflush. Doing that would seem perfectly reasonable to me.

-- 
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: clflush on pin_to_display after pwrite to UC bo in LLC

[Chris Wilson]

On Tue, Aug 11, 2015 at 06:10:29PM +0300, Ville Syrjälä wrote:
> On Tue, Aug 11, 2015 at 05:56:28PM +0300, Ville Syrjälä wrote:
> > Hmm. So what would happen on !LLC if we start with a cached bo, then pwrite it
> > and afterwards make it uncached?
> 
> In fact that would still fail even with my patch, and wouldn't work with current
> upstream code either. To fix that I'd need to drop the I915_CACHE_NONE/WT checks
> from pwrite in my patch and always set cache_dirty=true when it didn't
> clflush. Doing that would seem perfectly reasonable to me.

Yes. I agree. Marking obj->cache_dirty whenever we dirty the cpu cache
and clear it upon clflush seems sane. It will only take effect on
transition to the display plane, so not going to impact us except when
correctness is required. So maybe we should call it obj->display_dirty?
-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

[PATCH v2] drm/i915: clflush on pin_to_display after pwrite to UC bo in LLC

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Currently we don't clflush on pin_to_display if the bo is already
UC/WT and is not in the CPU write domain. This causes problems with
pwrite since pwrite doesn't change the write domain, and it avoids
clflushing on UC/WT buffers on LLC platforms unless the buffer is
currently being scanned out.

Fix the problem by marking the cache dirty and adjusting
i915_gem_object_set_cache_level() to clflush when the cache is dirty
even if the cache_level doesn't change.

My last attempt [1] at fixing this via write domain frobbing was shot
down, but now with the cache_dirty flag we can do things in a nicer way.

[1] http://lists.freedesktop.org/archives/intel-gfx/2014-November/055390.html

v2: Drop the I915_CACHE_NONE/WT checks from pwrite

Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=86422
Testcase: igt/kms_pwrite_crc
Testcase: igt/gem_pwrite_snooped
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_gem.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 73293b4..f828dc7 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -1005,12 +1005,14 @@ out:
 		if (!needs_clflush_after &&
 		    obj->base.write_domain != I915_GEM_DOMAIN_CPU) {
 			if (i915_gem_clflush_object(obj, obj->pin_display))
-				i915_gem_chipset_flush(dev);
+				needs_clflush_after = true;
 		}
 	}
 
 	if (needs_clflush_after)
 		i915_gem_chipset_flush(dev);
+	else
+		obj->cache_dirty = true;
 
 	intel_fb_obj_flush(obj, false, ORIGIN_CPU);
 	return ret;
@@ -3639,10 +3641,10 @@ int i915_gem_object_set_cache_level(struct drm_i915_gem_object *obj,
 {
 	struct drm_device *dev = obj->base.dev;
 	struct i915_vma *vma, *next;
-	int ret;
+	int ret = 0;
 
 	if (obj->cache_level == cache_level)
-		return 0;
+		goto out;
 
 	if (i915_gem_obj_is_pinned(obj)) {
 		DRM_DEBUG("can not change the cache level of pinned objects\n");
@@ -3687,6 +3689,7 @@ int i915_gem_object_set_cache_level(struct drm_i915_gem_object *obj,
 		vma->node.color = cache_level;
 	obj->cache_level = cache_level;
 
+out:
 	if (obj->cache_dirty &&
 	    obj->base.write_domain != I915_GEM_DOMAIN_CPU &&
 	    cpu_write_needs_clflush(obj)) {
-- 
2.4.6

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2] drm/i915: clflush on pin_to_display after pwrite to UC bo in LLC

[Chris Wilson]

On Tue, Aug 11, 2015 at 07:47:10PM +0300, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> Currently we don't clflush on pin_to_display if the bo is already
> UC/WT and is not in the CPU write domain. This causes problems with
> pwrite since pwrite doesn't change the write domain, and it avoids
> clflushing on UC/WT buffers on LLC platforms unless the buffer is
> currently being scanned out.
> 
> Fix the problem by marking the cache dirty and adjusting
> i915_gem_object_set_cache_level() to clflush when the cache is dirty
> even if the cache_level doesn't change.
> 
> My last attempt [1] at fixing this via write domain frobbing was shot
> down, but now with the cache_dirty flag we can do things in a nicer way.
> 
> [1] http://lists.freedesktop.org/archives/intel-gfx/2014-November/055390.html
> 
> v2: Drop the I915_CACHE_NONE/WT checks from pwrite
> 
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=86422
> Testcase: igt/kms_pwrite_crc
> Testcase: igt/gem_pwrite_snooped
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
-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 v2] drm/i915: clflush on pin_to_display after pwrite to UC bo in LLC

[Daniel Vetter]

On Tue, Aug 11, 2015 at 06:12:04PM +0100, Chris Wilson wrote:
> On Tue, Aug 11, 2015 at 07:47:10PM +0300, ville.syrjala@linux.intel.com wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > Currently we don't clflush on pin_to_display if the bo is already
> > UC/WT and is not in the CPU write domain. This causes problems with
> > pwrite since pwrite doesn't change the write domain, and it avoids
> > clflushing on UC/WT buffers on LLC platforms unless the buffer is
> > currently being scanned out.
> > 
> > Fix the problem by marking the cache dirty and adjusting
> > i915_gem_object_set_cache_level() to clflush when the cache is dirty
> > even if the cache_level doesn't change.
> > 
> > My last attempt [1] at fixing this via write domain frobbing was shot
> > down, but now with the cache_dirty flag we can do things in a nicer way.
> > 
> > [1] http://lists.freedesktop.org/archives/intel-gfx/2014-November/055390.html
> > 
> > v2: Drop the I915_CACHE_NONE/WT checks from pwrite
> > 
> > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=86422
> > Testcase: igt/kms_pwrite_crc
> > Testcase: igt/gem_pwrite_snooped
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>

Queued for -next, thanks for the patch.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2] drm/i915: clflush on pin_to_display after pwrite to UC bo in LLC

[shuang.he]

Tested-By: Intel Graphics QA PRTS (Patch Regression Test System Contact: shuang.he@intel.com)
Task id: 7152
-------------------------------------Summary-------------------------------------
Platform          Delta          drm-intel-nightly          Series Applied
ILK                                  302/302              302/302
SNB                                  315/315              315/315
IVB                                  336/336              336/336
BYT                                  283/283              283/283
HSW                                  378/378              378/378
-------------------------------------Detailed-------------------------------------
Platform  Test                                drm-intel-nightly          Series Applied
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

[PATCH i-g-t] tests/gem_pwrite_snooped: Verify set_caching vs. pwrite clflush behaviour

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

The test does the following
1. set_domain src GTT
2. set_caching src NONE
3. pwrite src
4. set_caching src CACHED
5. blt src->dst
6. pread dst
7. verify data matches

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 tests/Makefile.sources     |   1 +
 tests/gem_pwrite_snooped.c | 140 +++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 141 insertions(+)
 create mode 100644 tests/gem_pwrite_snooped.c

diff --git a/tests/Makefile.sources b/tests/Makefile.sources
index c94714b..b9a4cb4 100644
--- a/tests/Makefile.sources
+++ b/tests/Makefile.sources
@@ -47,6 +47,7 @@ TESTS_progs_M = \
 	gem_pread_after_blit \
 	gem_pwrite \
 	gem_pwrite_pread \
+	gem_pwrite_snooped \
 	gem_readwrite \
 	gem_read_read_speed \
 	gem_reloc_overflow \
diff --git a/tests/gem_pwrite_snooped.c b/tests/gem_pwrite_snooped.c
new file mode 100644
index 0000000..890c61d
--- /dev/null
+++ b/tests/gem_pwrite_snooped.c
@@ -0,0 +1,140 @@
+/*
+ * Copyright © 2015 Intel Corporation
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a
+ * copy of this software and associated documentation files (the "Software"),
+ * to deal in the Software without restriction, including without limitation
+ * the rights to use, copy, modify, merge, publish, distribute, sublicense,
+ * and/or sell copies of the Software, and to permit persons to whom the
+ * Software is furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice (including the next
+ * paragraph) shall be included in all copies or substantial portions of the
+ * Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
+ * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS
+ * IN THE SOFTWARE.
+ *
+ */
+
+#include <unistd.h>
+#include <stdlib.h>
+#include <stdint.h>
+#include <stdio.h>
+#include <string.h>
+#include <fcntl.h>
+#include <inttypes.h>
+#include <errno.h>
+#include <sys/stat.h>
+#include <sys/ioctl.h>
+#include <sys/time.h>
+#include "drm.h"
+#include "ioctl_wrappers.h"
+#include "intel_chipset.h"
+#include "drmtest.h"
+#include "igt_aux.h"
+
+IGT_TEST_DESCRIPTION(
+   "pwrite to a snooped bo then make it uncached and check that the GPU sees the data.");
+
+static int fd;
+static uint32_t devid;
+static drm_intel_bufmgr *bufmgr;
+
+static void blit(drm_intel_bo *dst, drm_intel_bo *src,
+		 unsigned int width, unsigned int height,
+		 unsigned int dst_pitch, unsigned int src_pitch)
+{
+	struct intel_batchbuffer *batch;
+
+	batch = intel_batchbuffer_alloc(bufmgr, devid);
+	igt_assert(batch);
+
+	BLIT_COPY_BATCH_START(0);
+	OUT_BATCH((3 << 24) | /* 32 bits */
+		  (0xcc << 16) | /* copy ROP */
+		  dst_pitch);
+	OUT_BATCH(0 << 16 | 0);
+	OUT_BATCH(height << 16 | width);
+	OUT_RELOC_FENCED(dst, I915_GEM_DOMAIN_RENDER, I915_GEM_DOMAIN_RENDER, 0);
+	OUT_BATCH(0 << 16 | 0);
+	OUT_BATCH(src_pitch);
+	OUT_RELOC_FENCED(src, I915_GEM_DOMAIN_RENDER, 0, 0);
+	ADVANCE_BATCH();
+
+	if (batch->gen >= 6) {
+		BEGIN_BATCH(3, 0);
+		OUT_BATCH(XY_SETUP_CLIP_BLT_CMD);
+		OUT_BATCH(0);
+		OUT_BATCH(0);
+		ADVANCE_BATCH();
+	}
+
+	intel_batchbuffer_flush(batch);
+	intel_batchbuffer_free(batch);
+}
+
+static void *memchr_inv(const void *s, int c, size_t n)
+{
+	const unsigned char *us = s;
+	unsigned char uc = c;
+
+	while (n--) {
+		if (*us != uc)
+			return (void *) us;
+		us++;
+	}
+
+	return NULL;
+}
+
+static void test(int w, int h)
+{
+	int object_size = w * h * 4;
+	drm_intel_bo *src, *dst;
+	void *buf;
+
+	src = drm_intel_bo_alloc(bufmgr, "src", object_size, 4096);
+	igt_assert(src);
+	dst = drm_intel_bo_alloc(bufmgr, "dst", object_size, 4096);
+	igt_assert(dst);
+
+	buf = malloc(object_size);
+	igt_assert(buf);
+	memset(buf, 0xff, object_size);
+
+	gem_set_domain(fd, src->handle, I915_GEM_DOMAIN_GTT,
+		       I915_GEM_DOMAIN_GTT);
+
+	gem_set_caching(fd, src->handle, I915_CACHING_CACHED);
+
+	gem_write(fd, src->handle, 0, buf, object_size);
+
+	gem_set_caching(fd, src->handle, I915_CACHING_NONE);
+
+	blit(dst, src, w, h, w * 4, h * 4);
+
+	memset(buf, 0x00, object_size);
+	gem_read(fd, dst->handle, 0, buf, object_size);
+
+	igt_assert(memchr_inv(buf, 0xff, object_size) == NULL);
+}
+
+igt_simple_main
+{
+	igt_skip_on_simulation();
+
+	fd = drm_open_any();
+	devid = intel_get_drm_devid(fd);
+	bufmgr = drm_intel_bufmgr_gem_init(fd, 4096);
+
+	test(256, 256);
+
+	drm_intel_bufmgr_destroy(bufmgr);
+	close(fd);
+}
-- 
2.4.6

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx