All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 1/3] drm/i915: Remove chipset flush after cache flush
@ 2016-11-06 12:59 Chris Wilson
  2016-11-06 13:00 ` [PATCH 2/3] drm/i915: Always flush the dirty CPU cache when pinning the scanout Chris Wilson
                   ` (3 more replies)
  0 siblings, 4 replies; 11+ messages in thread
From: Chris Wilson @ 2016-11-06 12:59 UTC (permalink / raw)
  To: intel-gfx

We always flush the chipset prior to executing with the GPU, so we can
skip the flush during ordinary domain management.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/i915_drv.h |  2 +-
 drivers/gpu/drm/i915/i915_gem.c | 23 ++++++++---------------
 2 files changed, 9 insertions(+), 16 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 4735b4177100..b35f96315930 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -3405,7 +3405,7 @@ static inline u32 i915_reset_count(struct i915_gpu_error *error)
 
 void i915_gem_reset(struct drm_i915_private *dev_priv);
 void i915_gem_set_wedged(struct drm_i915_private *dev_priv);
-bool i915_gem_clflush_object(struct drm_i915_gem_object *obj, bool force);
+void i915_gem_clflush_object(struct drm_i915_gem_object *obj, bool force);
 int __must_check i915_gem_init(struct drm_device *dev);
 int __must_check i915_gem_init_hw(struct drm_device *dev);
 void i915_gem_init_swizzling(struct drm_device *dev);
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 0dbf38c51d14..d48509783e41 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -3179,23 +3179,22 @@ i915_vma_insert(struct i915_vma *vma, u64 size, u64 alignment, u64 flags)
 	return ret;
 }
 
-bool
-i915_gem_clflush_object(struct drm_i915_gem_object *obj,
-			bool force)
+void i915_gem_clflush_object(struct drm_i915_gem_object *obj,
+			     bool force)
 {
 	/* If we don't have a page list set up, then we're not pinned
 	 * to GPU, and we can ignore the cache flush because it'll happen
 	 * again at bind time.
 	 */
 	if (!obj->mm.pages)
-		return false;
+		return;
 
 	/*
 	 * Stolen memory is always coherent with the GPU as it is explicitly
 	 * marked as wc by the system, or the system is cache-coherent.
 	 */
 	if (obj->stolen || obj->phys_handle)
-		return false;
+		return;
 
 	/* If the GPU is snooping the contents of the CPU cache,
 	 * we do not need to manually clear the CPU cache lines.  However,
@@ -3207,14 +3206,12 @@ i915_gem_clflush_object(struct drm_i915_gem_object *obj,
 	 */
 	if (!force && cpu_cache_is_coherent(obj->base.dev, obj->cache_level)) {
 		obj->cache_dirty = true;
-		return false;
+		return;
 	}
 
 	trace_i915_gem_object_clflush(obj);
 	drm_clflush_sg(obj->mm.pages);
 	obj->cache_dirty = false;
-
-	return true;
 }
 
 /** Flushes the GTT write domain for the object if it's dirty. */
@@ -3260,9 +3257,7 @@ i915_gem_object_flush_cpu_write_domain(struct drm_i915_gem_object *obj)
 	if (obj->base.write_domain != I915_GEM_DOMAIN_CPU)
 		return;
 
-	if (i915_gem_clflush_object(obj, obj->pin_display))
-		i915_gem_chipset_flush(to_i915(obj->base.dev));
-
+	i915_gem_clflush_object(obj, obj->pin_display);
 	intel_fb_obj_flush(obj, false, ORIGIN_CPU);
 
 	obj->base.write_domain = 0;
@@ -3469,10 +3464,8 @@ int i915_gem_object_set_cache_level(struct drm_i915_gem_object *obj,
 	 * object is now coherent at its new cache level (with respect
 	 * to the access domain).
 	 */
-	if (obj->cache_dirty && cpu_write_needs_clflush(obj)) {
-		if (i915_gem_clflush_object(obj, true))
-			i915_gem_chipset_flush(to_i915(obj->base.dev));
-	}
+	if (obj->cache_dirty && cpu_write_needs_clflush(obj))
+		i915_gem_clflush_object(obj, true);
 
 	return 0;
 }
-- 
2.10.2

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

^ permalink raw reply related	[flat|nested] 11+ messages in thread

* [PATCH 2/3] drm/i915: Always flush the dirty CPU cache when pinning the scanout
  2016-11-06 12:59 [PATCH 1/3] drm/i915: Remove chipset flush after cache flush Chris Wilson
@ 2016-11-06 13:00 ` Chris Wilson
  2016-11-08 11:33   ` Ville Syrjälä
  2016-11-06 13:00 ` [PATCH 3/3] drm/i915: Mark all skipped clflushes as leaving the CPU cache dirty Chris Wilson
                   ` (2 subsequent siblings)
  3 siblings, 1 reply; 11+ messages in thread
From: Chris Wilson @ 2016-11-06 13:00 UTC (permalink / raw)
  To: intel-gfx

Currently we only clflush the scanout if it is in the CPU domain. Also
flush if we have a pending CPU clflush. We also want to treat the
dirtyfb path similar, and flush any pending writes there as well.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/i915_gem.c      | 9 +++++----
 drivers/gpu/drm/i915/intel_display.c | 2 ++
 2 files changed, 7 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index d48509783e41..6845cf03287c 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -3356,12 +3356,12 @@ int i915_gem_object_set_cache_level(struct drm_i915_gem_object *obj,
 				    enum i915_cache_level cache_level)
 {
 	struct i915_vma *vma;
-	int ret = 0;
+	int ret;
 
 	lockdep_assert_held(&obj->base.dev->struct_mutex);
 
 	if (obj->cache_level == cache_level)
-		goto out;
+		return 0;
 
 	/* Inspect the list of currently bound VMA and unbind any that would
 	 * be invalid given the new cache-level. This is principally to
@@ -3459,7 +3459,6 @@ int i915_gem_object_set_cache_level(struct drm_i915_gem_object *obj,
 		vma->node.color = cache_level;
 	obj->cache_level = cache_level;
 
-out:
 	/* Flush the dirty CPU caches to the backing storage so that the
 	 * object is now coherent at its new cache level (with respect
 	 * to the access domain).
@@ -3608,7 +3607,9 @@ i915_gem_object_pin_to_display_plane(struct drm_i915_gem_object *obj,
 
 	vma->display_alignment = max_t(u64, vma->display_alignment, alignment);
 
-	i915_gem_object_flush_cpu_write_domain(obj);
+	if (obj->cache_dirty || obj->base.write_domain == I915_GEM_DOMAIN_CPU)
+		i915_gem_clflush_object(obj, true);
+	intel_fb_obj_flush(obj, false, ORIGIN_CPU);
 
 	old_write_domain = obj->base.write_domain;
 	old_read_domains = obj->base.read_domains;
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 92ab01f33208..47233b242c99 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -15698,6 +15698,8 @@ static int intel_user_framebuffer_dirty(struct drm_framebuffer *fb,
 	struct drm_i915_gem_object *obj = intel_fb->obj;
 
 	mutex_lock(&dev->struct_mutex);
+	if (obj->pin_display && obj->cache_dirty)
+		i915_gem_clflush_object(obj, true);
 	intel_fb_obj_flush(obj, false, ORIGIN_DIRTYFB);
 	mutex_unlock(&dev->struct_mutex);
 
-- 
2.10.2

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

^ permalink raw reply related	[flat|nested] 11+ messages in thread

* [PATCH 3/3] drm/i915: Mark all skipped clflushes as leaving the CPU cache dirty
  2016-11-06 12:59 [PATCH 1/3] drm/i915: Remove chipset flush after cache flush Chris Wilson
  2016-11-06 13:00 ` [PATCH 2/3] drm/i915: Always flush the dirty CPU cache when pinning the scanout Chris Wilson
@ 2016-11-06 13:00 ` Chris Wilson
  2016-11-06 13:45 ` ✓ Fi.CI.BAT: success for series starting with [1/3] drm/i915: Remove chipset flush after cache flush Patchwork
  2016-11-07 12:01 ` [PATCH 1/3] " Joonas Lahtinen
  3 siblings, 0 replies; 11+ messages in thread
From: Chris Wilson @ 2016-11-06 13:00 UTC (permalink / raw)
  To: intel-gfx

Currently, we may skip the clflush on an object if the object has not
yet allocated any pages. However, the object may still be polluting the
CPU cache and may now be in a coherent uncached domain - and so no
longer generating clflushing.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/i915_gem.c | 21 +++++++++++++--------
 1 file changed, 13 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 6845cf03287c..e43fad3ccf24 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -3182,20 +3182,25 @@ i915_vma_insert(struct i915_vma *vma, u64 size, u64 alignment, u64 flags)
 void i915_gem_clflush_object(struct drm_i915_gem_object *obj,
 			     bool force)
 {
-	/* If we don't have a page list set up, then we're not pinned
-	 * to GPU, and we can ignore the cache flush because it'll happen
-	 * again at bind time.
-	 */
-	if (!obj->mm.pages)
-		return;
-
 	/*
 	 * Stolen memory is always coherent with the GPU as it is explicitly
 	 * marked as wc by the system, or the system is cache-coherent.
+	 * Similarly, we only access struct pages through the CPU cache, so
+	 * anything not backed by physical memory we consider to be always
+	 * coherent and not need clflushing.
 	 */
-	if (obj->stolen || obj->phys_handle)
+	if (!i915_gem_object_has_struct_page(obj))
 		return;
 
+	/* If we don't have a page list set up, then we're not pinned
+	 * to GPU, and we can ignore the cache flush because it'll happen
+	 * again at bind time.
+	 */
+	if (!obj->mm.pages) {
+		obj->cache_dirty = true;
+		return;
+	}
+
 	/* If the GPU is snooping the contents of the CPU cache,
 	 * we do not need to manually clear the CPU cache lines.  However,
 	 * the caches are only snooped when the render cache is
-- 
2.10.2

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

^ permalink raw reply related	[flat|nested] 11+ messages in thread

* ✓ Fi.CI.BAT: success for series starting with [1/3] drm/i915: Remove chipset flush after cache flush
  2016-11-06 12:59 [PATCH 1/3] drm/i915: Remove chipset flush after cache flush Chris Wilson
  2016-11-06 13:00 ` [PATCH 2/3] drm/i915: Always flush the dirty CPU cache when pinning the scanout Chris Wilson
  2016-11-06 13:00 ` [PATCH 3/3] drm/i915: Mark all skipped clflushes as leaving the CPU cache dirty Chris Wilson
@ 2016-11-06 13:45 ` Patchwork
  2016-11-07 12:01 ` [PATCH 1/3] " Joonas Lahtinen
  3 siblings, 0 replies; 11+ messages in thread
From: Patchwork @ 2016-11-06 13:45 UTC (permalink / raw)
  To: Chris Wilson; +Cc: intel-gfx

== Series Details ==

Series: series starting with [1/3] drm/i915: Remove chipset flush after cache flush
URL   : https://patchwork.freedesktop.org/series/14897/
State : success

== Summary ==

Series 14897v1 Series without cover letter
https://patchwork.freedesktop.org/api/1.0/series/14897/revisions/1/mbox/

Test gem_sync:
        Subgroup basic-store-all:
                fail       -> PASS       (fi-hsw-4770r)

fi-bdw-5557u     total:241  pass:226  dwarn:0   dfail:0   fail:0   skip:15 
fi-bsw-n3050     total:241  pass:201  dwarn:0   dfail:0   fail:0   skip:40 
fi-bxt-t5700     total:241  pass:213  dwarn:0   dfail:0   fail:0   skip:28 
fi-byt-j1900     total:241  pass:213  dwarn:0   dfail:0   fail:0   skip:28 
fi-byt-n2820     total:241  pass:209  dwarn:0   dfail:0   fail:0   skip:32 
fi-hsw-4770      total:241  pass:221  dwarn:0   dfail:0   fail:0   skip:20 
fi-hsw-4770r     total:241  pass:221  dwarn:0   dfail:0   fail:0   skip:20 
fi-ilk-650       total:241  pass:188  dwarn:0   dfail:0   fail:0   skip:53 
fi-ivb-3520m     total:241  pass:219  dwarn:0   dfail:0   fail:0   skip:22 
fi-ivb-3770      total:241  pass:219  dwarn:0   dfail:0   fail:0   skip:22 
fi-kbl-7200u     total:241  pass:219  dwarn:0   dfail:0   fail:0   skip:22 
fi-skl-6260u     total:241  pass:227  dwarn:0   dfail:0   fail:0   skip:14 
fi-skl-6700hq    total:241  pass:220  dwarn:0   dfail:0   fail:0   skip:21 
fi-skl-6700k     total:241  pass:219  dwarn:1   dfail:0   fail:0   skip:21 
fi-skl-6770hq    total:241  pass:227  dwarn:0   dfail:0   fail:0   skip:14 
fi-snb-2520m     total:241  pass:209  dwarn:0   dfail:0   fail:0   skip:32 
fi-snb-2600      total:241  pass:208  dwarn:0   dfail:0   fail:0   skip:33 

49a651a2e66ef603995f88a470d0986c2ef8b5b8 drm-intel-nightly: 2016y-11m-04d-18h-04m-36s UTC integration manifest
c1d20e5 drm/i915: Mark all skipped clflushes as leaving the CPU cache dirty
2f4ce55 drm/i915: Always flush the dirty CPU cache when pinning the scanout
51df37f drm/i915: Remove chipset flush after cache flush

== Logs ==

For more details see: https://intel-gfx-ci.01.org/CI/Patchwork_2914/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 1/3] drm/i915: Remove chipset flush after cache flush
  2016-11-06 12:59 [PATCH 1/3] drm/i915: Remove chipset flush after cache flush Chris Wilson
                   ` (2 preceding siblings ...)
  2016-11-06 13:45 ` ✓ Fi.CI.BAT: success for series starting with [1/3] drm/i915: Remove chipset flush after cache flush Patchwork
@ 2016-11-07 12:01 ` Joonas Lahtinen
  2016-11-07 12:10   ` Chris Wilson
  3 siblings, 1 reply; 11+ messages in thread
From: Joonas Lahtinen @ 2016-11-07 12:01 UTC (permalink / raw)
  To: Chris Wilson, intel-gfx

On su, 2016-11-06 at 12:59 +0000, Chris Wilson wrote:
> We always flush the chipset prior to executing with the GPU, so we can
> skip the flush during ordinary domain management.
> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>

Maybe even add?

Fixes: dcd79934b0dd ("drm/i915: Unconditionally flush any chipset buffers before execbuf")

Reviewed-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>

Regards, Joonas
-- 
Joonas Lahtinen
Open Source Technology Center
Intel Corporation
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 1/3] drm/i915: Remove chipset flush after cache flush
  2016-11-07 12:01 ` [PATCH 1/3] " Joonas Lahtinen
@ 2016-11-07 12:10   ` Chris Wilson
  2016-11-07 16:16     ` Joonas Lahtinen
  0 siblings, 1 reply; 11+ messages in thread
From: Chris Wilson @ 2016-11-07 12:10 UTC (permalink / raw)
  To: Joonas Lahtinen; +Cc: intel-gfx

On Mon, Nov 07, 2016 at 02:01:46PM +0200, Joonas Lahtinen wrote:
> On su, 2016-11-06 at 12:59 +0000, Chris Wilson wrote:
> > We always flush the chipset prior to executing with the GPU, so we can
> > skip the flush during ordinary domain management.
> > 
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> 
> Maybe even add?
> 
> Fixes: dcd79934b0dd ("drm/i915: Unconditionally flush any chipset buffers before execbuf")

It is just an optimisation - I don't want to imply that we should be
pushing this to stable trees as it should not be fixing user visible
bugs. Slightly worried about the GTT access paths, they have never been
as coherent as claimed, so I worry...
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 1/3] drm/i915: Remove chipset flush after cache flush
  2016-11-07 12:10   ` Chris Wilson
@ 2016-11-07 16:16     ` Joonas Lahtinen
  2016-11-08 11:08       ` Chris Wilson
  0 siblings, 1 reply; 11+ messages in thread
From: Joonas Lahtinen @ 2016-11-07 16:16 UTC (permalink / raw)
  To: Chris Wilson; +Cc: intel-gfx

On ma, 2016-11-07 at 12:10 +0000, Chris Wilson wrote:
> On Mon, Nov 07, 2016 at 02:01:46PM +0200, Joonas Lahtinen wrote:
> > 
> > On su, 2016-11-06 at 12:59 +0000, Chris Wilson wrote:
> > > 
> > > We always flush the chipset prior to executing with the GPU, so we can
> > > skip the flush during ordinary domain management.
> > > 
> > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > 
> > Maybe even add?
> > 
> > Fixes: dcd79934b0dd ("drm/i915: Unconditionally flush any chipset buffers before execbuf")
> 
> It is just an optimisation - I don't want to imply that we should be

Well, it's an optimization that fixes a commit which duplicated the
call. That might be a regression in some micro-benchmark.

But fine without the tag too, if you're sure it's not a big one.

Regards, Joonas
-- 
Joonas Lahtinen
Open Source Technology Center
Intel Corporation
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 1/3] drm/i915: Remove chipset flush after cache flush
  2016-11-07 16:16     ` Joonas Lahtinen
@ 2016-11-08 11:08       ` Chris Wilson
  0 siblings, 0 replies; 11+ messages in thread
From: Chris Wilson @ 2016-11-08 11:08 UTC (permalink / raw)
  To: Joonas Lahtinen; +Cc: intel-gfx

On Mon, Nov 07, 2016 at 06:16:00PM +0200, Joonas Lahtinen wrote:
> On ma, 2016-11-07 at 12:10 +0000, Chris Wilson wrote:
> > On Mon, Nov 07, 2016 at 02:01:46PM +0200, Joonas Lahtinen wrote:
> > > 
> > > On su, 2016-11-06 at 12:59 +0000, Chris Wilson wrote:
> > > > 
> > > > We always flush the chipset prior to executing with the GPU, so we can
> > > > skip the flush during ordinary domain management.
> > > > 
> > > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > > 
> > > Maybe even add?
> > > 
> > > Fixes: dcd79934b0dd ("drm/i915: Unconditionally flush any chipset buffers before execbuf")
> > 
> > It is just an optimisation - I don't want to imply that we should be
> 
> Well, it's an optimization that fixes a commit which duplicated the
> call. That might be a regression in some micro-benchmark.
> 
> But fine without the tag too, if you're sure it's not a big one.

Added the reference to the commit, but without the fixes tag since I
don't think it is a big deal (as the number of unconditional chipset
flushes will be much greater than the number of unneeded flushes removed
in this patch).
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 2/3] drm/i915: Always flush the dirty CPU cache when pinning the scanout
  2016-11-06 13:00 ` [PATCH 2/3] drm/i915: Always flush the dirty CPU cache when pinning the scanout Chris Wilson
@ 2016-11-08 11:33   ` Ville Syrjälä
  2016-11-08 11:42     ` Chris Wilson
  0 siblings, 1 reply; 11+ messages in thread
From: Ville Syrjälä @ 2016-11-08 11:33 UTC (permalink / raw)
  To: Chris Wilson; +Cc: intel-gfx

On Sun, Nov 06, 2016 at 01:00:00PM +0000, Chris Wilson wrote:
> Currently we only clflush the scanout if it is in the CPU domain. Also
> flush if we have a pending CPU clflush. We also want to treat the
> dirtyfb path similar, and flush any pending writes there as well.
> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> ---
>  drivers/gpu/drm/i915/i915_gem.c      | 9 +++++----
>  drivers/gpu/drm/i915/intel_display.c | 2 ++
>  2 files changed, 7 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index d48509783e41..6845cf03287c 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -3356,12 +3356,12 @@ int i915_gem_object_set_cache_level(struct drm_i915_gem_object *obj,
>  				    enum i915_cache_level cache_level)
>  {
>  	struct i915_vma *vma;
> -	int ret = 0;
> +	int ret;
>  
>  	lockdep_assert_held(&obj->base.dev->struct_mutex);
>  
>  	if (obj->cache_level == cache_level)
> -		goto out;
> +		return 0;

I added this for the pin_display case, which you now handle explicitly.
So yeah, looks like this can go. At least I can't think of another valid
use case to keep flushing on every set_cache_level(NONE).

>  
>  	/* Inspect the list of currently bound VMA and unbind any that would
>  	 * be invalid given the new cache-level. This is principally to
> @@ -3459,7 +3459,6 @@ int i915_gem_object_set_cache_level(struct drm_i915_gem_object *obj,
>  		vma->node.color = cache_level;
>  	obj->cache_level = cache_level;
>  
> -out:
>  	/* Flush the dirty CPU caches to the backing storage so that the
>  	 * object is now coherent at its new cache level (with respect
>  	 * to the access domain).
> @@ -3608,7 +3607,9 @@ i915_gem_object_pin_to_display_plane(struct drm_i915_gem_object *obj,
>  
>  	vma->display_alignment = max_t(u64, vma->display_alignment, alignment);
>  
> -	i915_gem_object_flush_cpu_write_domain(obj);
> +	if (obj->cache_dirty || obj->base.write_domain == I915_GEM_DOMAIN_CPU)
> +		i915_gem_clflush_object(obj, true);
> +	intel_fb_obj_flush(obj, false, ORIGIN_CPU);

So one slight change in behaviour is that we won't clear the write domain
here, but that's fine since we'll clear it below anyway.

The other is that we now call intel_fb_obj_flush() unconditionally,
whereas before we only called if when the bo was in the CPU write
domain. Hmm. Should we keep it conditional?

>  
>  	old_write_domain = obj->base.write_domain;
>  	old_read_domains = obj->base.read_domains;
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 92ab01f33208..47233b242c99 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -15698,6 +15698,8 @@ static int intel_user_framebuffer_dirty(struct drm_framebuffer *fb,
>  	struct drm_i915_gem_object *obj = intel_fb->obj;
>  
>  	mutex_lock(&dev->struct_mutex);
> +	if (obj->pin_display && obj->cache_dirty)
> +		i915_gem_clflush_object(obj, true);
>  	intel_fb_obj_flush(obj, false, ORIGIN_DIRTYFB);
>  	mutex_unlock(&dev->struct_mutex);
>  
> -- 
> 2.10.2
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 2/3] drm/i915: Always flush the dirty CPU cache when pinning the scanout
  2016-11-08 11:33   ` Ville Syrjälä
@ 2016-11-08 11:42     ` Chris Wilson
  2016-11-08 13:45       ` Ville Syrjälä
  0 siblings, 1 reply; 11+ messages in thread
From: Chris Wilson @ 2016-11-08 11:42 UTC (permalink / raw)
  To: Ville Syrjälä; +Cc: intel-gfx

On Tue, Nov 08, 2016 at 01:33:41PM +0200, Ville Syrjälä wrote:
> On Sun, Nov 06, 2016 at 01:00:00PM +0000, Chris Wilson wrote:
> > -	i915_gem_object_flush_cpu_write_domain(obj);
> > +	if (obj->cache_dirty || obj->base.write_domain == I915_GEM_DOMAIN_CPU)
> > +		i915_gem_clflush_object(obj, true);
> > +	intel_fb_obj_flush(obj, false, ORIGIN_CPU);
> 
> So one slight change in behaviour is that we won't clear the write domain
> here, but that's fine since we'll clear it below anyway.
> 
> The other is that we now call intel_fb_obj_flush() unconditionally,
> whereas before we only called if when the bo was in the CPU write
> domain. Hmm. Should we keep it conditional?

I was thinking that we should call it in the case we deferred clflush,
and I was wanting to make the preflip path closer to dirtyfb. Since to
me they both demark the frame boundary and the preparatory flushing
should be more or less the same.

> >  	old_write_domain = obj->base.write_domain;
> >  	old_read_domains = obj->base.read_domains;
> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > index 92ab01f33208..47233b242c99 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -15698,6 +15698,8 @@ static int intel_user_framebuffer_dirty(struct drm_framebuffer *fb,
> >  	struct drm_i915_gem_object *obj = intel_fb->obj;
> >  
> >  	mutex_lock(&dev->struct_mutex);
> > +	if (obj->pin_display && obj->cache_dirty)
> > +		i915_gem_clflush_object(obj, true);
> >  	intel_fb_obj_flush(obj, false, ORIGIN_DIRTYFB);
> >  	mutex_unlock(&dev->struct_mutex);

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 2/3] drm/i915: Always flush the dirty CPU cache when pinning the scanout
  2016-11-08 11:42     ` Chris Wilson
@ 2016-11-08 13:45       ` Ville Syrjälä
  0 siblings, 0 replies; 11+ messages in thread
From: Ville Syrjälä @ 2016-11-08 13:45 UTC (permalink / raw)
  To: Chris Wilson, intel-gfx

On Tue, Nov 08, 2016 at 11:42:38AM +0000, Chris Wilson wrote:
> On Tue, Nov 08, 2016 at 01:33:41PM +0200, Ville Syrjälä wrote:
> > On Sun, Nov 06, 2016 at 01:00:00PM +0000, Chris Wilson wrote:
> > > -	i915_gem_object_flush_cpu_write_domain(obj);
> > > +	if (obj->cache_dirty || obj->base.write_domain == I915_GEM_DOMAIN_CPU)
> > > +		i915_gem_clflush_object(obj, true);
> > > +	intel_fb_obj_flush(obj, false, ORIGIN_CPU);
> > 
> > So one slight change in behaviour is that we won't clear the write domain
> > here, but that's fine since we'll clear it below anyway.
> > 
> > The other is that we now call intel_fb_obj_flush() unconditionally,
> > whereas before we only called if when the bo was in the CPU write
> > domain. Hmm. Should we keep it conditional?
> 
> I was thinking that we should call it in the case we deferred clflush,
> and I was wanting to make the preflip path closer to dirtyfb. Since to
> me they both demark the frame boundary and the preparatory flushing
> should be more or less the same.

All right. Seems sane enough to me.

Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

> 
> > >  	old_write_domain = obj->base.write_domain;
> > >  	old_read_domains = obj->base.read_domains;
> > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > > index 92ab01f33208..47233b242c99 100644
> > > --- a/drivers/gpu/drm/i915/intel_display.c
> > > +++ b/drivers/gpu/drm/i915/intel_display.c
> > > @@ -15698,6 +15698,8 @@ static int intel_user_framebuffer_dirty(struct drm_framebuffer *fb,
> > >  	struct drm_i915_gem_object *obj = intel_fb->obj;
> > >  
> > >  	mutex_lock(&dev->struct_mutex);
> > > +	if (obj->pin_display && obj->cache_dirty)
> > > +		i915_gem_clflush_object(obj, true);
> > >  	intel_fb_obj_flush(obj, false, ORIGIN_DIRTYFB);
> > >  	mutex_unlock(&dev->struct_mutex);
> 
> -- 
> Chris Wilson, Intel Open Source Technology Centre

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 11+ messages in thread

end of thread, other threads:[~2016-11-08 13:45 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-11-06 12:59 [PATCH 1/3] drm/i915: Remove chipset flush after cache flush Chris Wilson
2016-11-06 13:00 ` [PATCH 2/3] drm/i915: Always flush the dirty CPU cache when pinning the scanout Chris Wilson
2016-11-08 11:33   ` Ville Syrjälä
2016-11-08 11:42     ` Chris Wilson
2016-11-08 13:45       ` Ville Syrjälä
2016-11-06 13:00 ` [PATCH 3/3] drm/i915: Mark all skipped clflushes as leaving the CPU cache dirty Chris Wilson
2016-11-06 13:45 ` ✓ Fi.CI.BAT: success for series starting with [1/3] drm/i915: Remove chipset flush after cache flush Patchwork
2016-11-07 12:01 ` [PATCH 1/3] " Joonas Lahtinen
2016-11-07 12:10   ` Chris Wilson
2016-11-07 16:16     ` Joonas Lahtinen
2016-11-08 11:08       ` Chris Wilson

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.