[PATCH 1/2] drm/i915: Simplify flush_cpu_write_domain

[PATCH 1/2] drm/i915: Simplify flush_cpu_write_domain

[Daniel Vetter]

We can push down the decision whether to force flushing into the
implementation since in all places that matter obj->pin_display is
accurate already. The only place where the optimization really matters
is the sw_finish_ioctl, and that already checks for obj->pin_display
on its own.

I suspect that this was simply an artifact of how

commit 2c22569bba8af6c2976d5f9479fe54a53a39966b
Author: Chris Wilson <chris@chris-wilson.co.uk>
Date:   Fri Aug 9 12:26:45 2013 +0100

    drm/i915: Update rules for writing through the LLC with the cpu

evolved - only v2 added the pin_display tracking.

Note that we still retain the gist of this logic from the above commit
with the explicit force argument for the low-level clflush function.

Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
---
 drivers/gpu/drm/i915/i915_gem.c | 14 ++++++--------
 1 file changed, 6 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index f486555fb4a8..783d1040bf83 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -39,8 +39,7 @@
 #include <linux/dma-buf.h>
 
 static void i915_gem_object_flush_gtt_write_domain(struct drm_i915_gem_object *obj);
-static void i915_gem_object_flush_cpu_write_domain(struct drm_i915_gem_object *obj,
-						   bool force);
+static void i915_gem_object_flush_cpu_write_domain(struct drm_i915_gem_object *obj);
 static __must_check int
 i915_gem_object_wait_rendering(struct drm_i915_gem_object *obj,
 			       bool readonly);
@@ -1516,7 +1515,7 @@ i915_gem_sw_finish_ioctl(struct drm_device *dev, void *data,
 
 	/* Pinned buffers may be scanout, so flush the cache */
 	if (obj->pin_display)
-		i915_gem_object_flush_cpu_write_domain(obj, true);
+		i915_gem_object_flush_cpu_write_domain(obj);
 
 	drm_gem_object_unreference(&obj->base);
 unlock:
@@ -3680,15 +3679,14 @@ i915_gem_object_flush_gtt_write_domain(struct drm_i915_gem_object *obj)
 
 /** Flushes the CPU write domain for the object if it's dirty. */
 static void
-i915_gem_object_flush_cpu_write_domain(struct drm_i915_gem_object *obj,
-				       bool force)
+i915_gem_object_flush_cpu_write_domain(struct drm_i915_gem_object *obj)
 {
 	uint32_t old_write_domain;
 
 	if (obj->base.write_domain != I915_GEM_DOMAIN_CPU)
 		return;
 
-	if (i915_gem_clflush_object(obj, force))
+	if (i915_gem_clflush_object(obj, obj->pin_display))
 		i915_gem_chipset_flush(obj->base.dev);
 
 	old_write_domain = obj->base.write_domain;
@@ -3735,7 +3733,7 @@ i915_gem_object_set_to_gtt_domain(struct drm_i915_gem_object *obj, bool write)
 	if (ret)
 		return ret;
 
-	i915_gem_object_flush_cpu_write_domain(obj, false);
+	i915_gem_object_flush_cpu_write_domain(obj);
 
 	/* Serialise direct access to this object with the barriers for
 	 * coherent writes from the GPU, by effectively invalidating the
@@ -3981,7 +3979,7 @@ i915_gem_object_pin_to_display_plane(struct drm_i915_gem_object *obj,
 	if (ret)
 		goto err_unpin_display;
 
-	i915_gem_object_flush_cpu_write_domain(obj, true);
+	i915_gem_object_flush_cpu_write_domain(obj);
 
 	old_write_domain = obj->base.write_domain;
 	old_read_domains = obj->base.read_domains;
-- 
2.1.4

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

[PATCH 2/2] drm/i915: Remove open-coded callers of flush_cpu_write_domain

[Daniel Vetter]

Both places check the cpu domain and through some indirection
obj->pin_display and whether cpu access is coherent. And then flush
both cpu caches and the chipset cache. Which is what
flush_cpu_write_domain does.

Only difference is that on top we get a tracepoint, frontbuffer
tracking and and domain tracking updates. Which doesn't matter since
both paths are slowpaths.

So lets go with more shared code for clearer idioms.

Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
---
 drivers/gpu/drm/i915/i915_gem.c | 15 ++++-----------
 1 file changed, 4 insertions(+), 11 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 783d1040bf83..b48c39230b80 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -1019,11 +1019,8 @@ out:
 		 * cachelines in-line while writing and the object moved
 		 * out of the cpu write domain while we've dropped the lock.
 		 */
-		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);
-		}
+		if (!needs_clflush_after)
+			i915_gem_object_flush_cpu_write_domain(obj);
 	}
 
 	if (needs_clflush_after)
@@ -3825,12 +3822,8 @@ 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);
-	}
+	if (obj->cache_dirty)
+		i915_gem_object_flush_cpu_write_domain(obj);
 
 	return 0;
 }
-- 
2.1.4

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

Re: [PATCH 1/2] drm/i915: Simplify flush_cpu_write_domain

[Chris Wilson]

On Wed, Jan 21, 2015 at 02:53:48PM +0100, Daniel Vetter wrote:
> We can push down the decision whether to force flushing into the
> implementation since in all places that matter obj->pin_display is
> accurate already. The only place where the optimization really matters
> is the sw_finish_ioctl, and that already checks for obj->pin_display
> on its own.
> 
> I suspect that this was simply an artifact of how
> 
> commit 2c22569bba8af6c2976d5f9479fe54a53a39966b
> Author: Chris Wilson <chris@chris-wilson.co.uk>
> Date:   Fri Aug 9 12:26:45 2013 +0100
> 
>     drm/i915: Update rules for writing through the LLC with the cpu
> 
> evolved - only v2 added the pin_display tracking.
> 
> Note that we still retain the gist of this logic from the above commit
> with the explicit force argument for the low-level clflush function.
> 
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Signed-off-by: Daniel Vetter <daniel.vetter@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 2/2] drm/i915: Remove open-coded callers of flush_cpu_write_domain

[Chris Wilson]

On Wed, Jan 21, 2015 at 02:53:49PM +0100, Daniel Vetter wrote:
> Both places check the cpu domain and through some indirection
> obj->pin_display and whether cpu access is coherent. And then flush
> both cpu caches and the chipset cache. Which is what
> flush_cpu_write_domain does.
> 
> Only difference is that on top we get a tracepoint, frontbuffer
> tracking and and domain tracking updates. Which doesn't matter since
> both paths are slowpaths.
> 
> So lets go with more shared code for clearer idioms.
> 
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>

Nice, took a few double checks to make sure that the
cpu_write_needs_clflush() does indeed equate to flush_cpu_write_domain()
here (the key is the passing obj->pin_display in the previous patch). I
think that is worth highlighting in the commit message.

Other than that,
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 1/2] drm/i915: Simplify flush_cpu_write_domain

[Ville Syrjälä]

On Wed, Jan 21, 2015 at 02:53:48PM +0100, Daniel Vetter wrote:
> We can push down the decision whether to force flushing into the
> implementation since in all places that matter obj->pin_display is
> accurate already. The only place where the optimization really matters
> is the sw_finish_ioctl, and that already checks for obj->pin_display
> on its own.
> 
> I suspect that this was simply an artifact of how
> 
> commit 2c22569bba8af6c2976d5f9479fe54a53a39966b
> Author: Chris Wilson <chris@chris-wilson.co.uk>
> Date:   Fri Aug 9 12:26:45 2013 +0100
> 
>     drm/i915: Update rules for writing through the LLC with the cpu
> 
> evolved - only v2 added the pin_display tracking.
> 
> Note that we still retain the gist of this logic from the above commit
> with the explicit force argument for the low-level clflush function.
> 
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_gem.c | 14 ++++++--------
>  1 file changed, 6 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index f486555fb4a8..783d1040bf83 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -39,8 +39,7 @@
>  #include <linux/dma-buf.h>
>  
>  static void i915_gem_object_flush_gtt_write_domain(struct drm_i915_gem_object *obj);
> -static void i915_gem_object_flush_cpu_write_domain(struct drm_i915_gem_object *obj,
> -						   bool force);
> +static void i915_gem_object_flush_cpu_write_domain(struct drm_i915_gem_object *obj);
>  static __must_check int
>  i915_gem_object_wait_rendering(struct drm_i915_gem_object *obj,
>  			       bool readonly);
> @@ -1516,7 +1515,7 @@ i915_gem_sw_finish_ioctl(struct drm_device *dev, void *data,
>  
>  	/* Pinned buffers may be scanout, so flush the cache */
>  	if (obj->pin_display)
> -		i915_gem_object_flush_cpu_write_domain(obj, true);
> +		i915_gem_object_flush_cpu_write_domain(obj);
>  
>  	drm_gem_object_unreference(&obj->base);
>  unlock:
> @@ -3680,15 +3679,14 @@ i915_gem_object_flush_gtt_write_domain(struct drm_i915_gem_object *obj)
>  
>  /** Flushes the CPU write domain for the object if it's dirty. */
>  static void
> -i915_gem_object_flush_cpu_write_domain(struct drm_i915_gem_object *obj,
> -				       bool force)
> +i915_gem_object_flush_cpu_write_domain(struct drm_i915_gem_object *obj)
>  {
>  	uint32_t old_write_domain;
>  
>  	if (obj->base.write_domain != I915_GEM_DOMAIN_CPU)
>  		return;
>  
> -	if (i915_gem_clflush_object(obj, force))
> +	if (i915_gem_clflush_object(obj, obj->pin_display))
>  		i915_gem_chipset_flush(obj->base.dev);
>  
>  	old_write_domain = obj->base.write_domain;
> @@ -3735,7 +3733,7 @@ i915_gem_object_set_to_gtt_domain(struct drm_i915_gem_object *obj, bool write)
>  	if (ret)
>  		return ret;
>  
> -	i915_gem_object_flush_cpu_write_domain(obj, false);
> +	i915_gem_object_flush_cpu_write_domain(obj);

This is the only place where there's a slight change in behaviour.
Previosuly we would not clflush here when pin_display==true, but from
now on we will. I had a patch to do only this change (part of some FBC
series), but IIRC you argued it could hide bugs. I guess you've
now changed you mind ;)

Anyway this makes sense to me, so
Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

>  
>  	/* Serialise direct access to this object with the barriers for
>  	 * coherent writes from the GPU, by effectively invalidating the
> @@ -3981,7 +3979,7 @@ i915_gem_object_pin_to_display_plane(struct drm_i915_gem_object *obj,
>  	if (ret)
>  		goto err_unpin_display;
>  
> -	i915_gem_object_flush_cpu_write_domain(obj, true);
> +	i915_gem_object_flush_cpu_write_domain(obj);
>  
>  	old_write_domain = obj->base.write_domain;
>  	old_read_domains = obj->base.read_domains;
> -- 
> 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 2/2] drm/i915: Remove open-coded callers of flush_cpu_write_domain

[Ville Syrjälä]

On Wed, Jan 21, 2015 at 02:53:49PM +0100, Daniel Vetter wrote:
> Both places check the cpu domain and through some indirection
> obj->pin_display and whether cpu access is coherent. And then flush
> both cpu caches and the chipset cache. Which is what
> flush_cpu_write_domain does.
> 
> Only difference is that on top we get a tracepoint, frontbuffer
> tracking and and domain tracking updates. Which doesn't matter since
> both paths are slowpaths.
> 
> So lets go with more shared code for clearer idioms.
> 
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_gem.c | 15 ++++-----------
>  1 file changed, 4 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index 783d1040bf83..b48c39230b80 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -1019,11 +1019,8 @@ out:
>  		 * cachelines in-line while writing and the object moved
>  		 * out of the cpu write domain while we've dropped the lock.
>  		 */
> -		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);
> -		}
> +		if (!needs_clflush_after)
> +			i915_gem_object_flush_cpu_write_domain(obj);

Nack. You've just inverted the write domain check.

>  	}
>  
>  	if (needs_clflush_after)
> @@ -3825,12 +3822,8 @@ 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);
> -	}
> +	if (obj->cache_dirty)
> +		i915_gem_object_flush_cpu_write_domain(obj);
>  
>  	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 1/2] drm/i915: Simplify flush_cpu_write_domain

[Daniel Vetter]

On Wed, Jan 21, 2015 at 3:08 PM, Ville Syrjälä
<ville.syrjala@linux.intel.com> wrote:
>> @@ -3735,7 +3733,7 @@ i915_gem_object_set_to_gtt_domain(struct drm_i915_gem_object *obj, bool write)
>>       if (ret)
>>               return ret;
>>
>> -     i915_gem_object_flush_cpu_write_domain(obj, false);
>> +     i915_gem_object_flush_cpu_write_domain(obj);
>
> This is the only place where there's a slight change in behaviour.
> Previosuly we would not clflush here when pin_display==true, but from
> now on we will. I had a patch to do only this change (part of some FBC
> series), but IIRC you argued it could hide bugs. I guess you've
> now changed you mind ;)

Hm, I don't remember that I've shot down a patch to change this and I
can't come up with any reason any more why. But you're right that the
commit message is a bit too silent about the behavioral change here.
Lazy me will try harder and augment it when merging.
-Daniel
-- 
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 1/2] drm/i915: Simplify flush_cpu_write_domain

[Ville Syrjälä]

On Wed, Jan 21, 2015 at 03:13:58PM +0100, Daniel Vetter wrote:
> On Wed, Jan 21, 2015 at 3:08 PM, Ville Syrjälä
> <ville.syrjala@linux.intel.com> wrote:
> >> @@ -3735,7 +3733,7 @@ i915_gem_object_set_to_gtt_domain(struct drm_i915_gem_object *obj, bool write)
> >>       if (ret)
> >>               return ret;
> >>
> >> -     i915_gem_object_flush_cpu_write_domain(obj, false);
> >> +     i915_gem_object_flush_cpu_write_domain(obj);
> >
> > This is the only place where there's a slight change in behaviour.
> > Previosuly we would not clflush here when pin_display==true, but from
> > now on we will. I had a patch to do only this change (part of some FBC
> > series), but IIRC you argued it could hide bugs. I guess you've
> > now changed you mind ;)
> 
> Hm, I don't remember that I've shot down a patch to change this and I
> can't come up with any reason any more why. But you're right that the
> commit message is a bit too silent about the behavioral change here.
> Lazy me will try harder and augment it when merging.

http://lists.freedesktop.org/archives/intel-gfx/2013-November/036421.html

Also seems Chris already suggested dropping the force parameter back
then, but no one took the bait:
http://lists.freedesktop.org/archives/intel-gfx/2013-November/036346.html

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

Re: [PATCH 1/2] drm/i915: Simplify flush_cpu_write_domain

[Daniel Vetter]

On Wed, Jan 21, 2015 at 04:21:20PM +0200, Ville Syrjälä wrote:
> On Wed, Jan 21, 2015 at 03:13:58PM +0100, Daniel Vetter wrote:
> > On Wed, Jan 21, 2015 at 3:08 PM, Ville Syrjälä
> > <ville.syrjala@linux.intel.com> wrote:
> > >> @@ -3735,7 +3733,7 @@ i915_gem_object_set_to_gtt_domain(struct drm_i915_gem_object *obj, bool write)
> > >>       if (ret)
> > >>               return ret;
> > >>
> > >> -     i915_gem_object_flush_cpu_write_domain(obj, false);
> > >> +     i915_gem_object_flush_cpu_write_domain(obj);
> > >
> > > This is the only place where there's a slight change in behaviour.
> > > Previosuly we would not clflush here when pin_display==true, but from
> > > now on we will. I had a patch to do only this change (part of some FBC
> > > series), but IIRC you argued it could hide bugs. I guess you've
> > > now changed you mind ;)
> > 
> > Hm, I don't remember that I've shot down a patch to change this and I
> > can't come up with any reason any more why. But you're right that the
> > commit message is a bit too silent about the behavioral change here.
> > Lazy me will try harder and augment it when merging.
> 
> http://lists.freedesktop.org/archives/intel-gfx/2013-November/036421.html

Well I didn't want the abi change really. But as a cleanup it seems
acceptable ;-)

Anyway added a bit of blurb and merged the patch, thanks for the feedback.
-Daniel
-- 
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 2/2] drm/i915: Remove open-coded callers of flush_cpu_write_domain

[Daniel Vetter]

On Wed, Jan 21, 2015 at 04:12:07PM +0200, Ville Syrjälä wrote:
> On Wed, Jan 21, 2015 at 02:53:49PM +0100, Daniel Vetter wrote:
> > Both places check the cpu domain and through some indirection
> > obj->pin_display and whether cpu access is coherent. And then flush
> > both cpu caches and the chipset cache. Which is what
> > flush_cpu_write_domain does.
> > 
> > Only difference is that on top we get a tracepoint, frontbuffer
> > tracking and and domain tracking updates. Which doesn't matter since
> > both paths are slowpaths.
> > 
> > So lets go with more shared code for clearer idioms.
> > 
> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > Cc: Chris Wilson <chris@chris-wilson.co.uk>
> > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_gem.c | 15 ++++-----------
> >  1 file changed, 4 insertions(+), 11 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> > index 783d1040bf83..b48c39230b80 100644
> > --- a/drivers/gpu/drm/i915/i915_gem.c
> > +++ b/drivers/gpu/drm/i915/i915_gem.c
> > @@ -1019,11 +1019,8 @@ out:
> >  		 * cachelines in-line while writing and the object moved
> >  		 * out of the cpu write domain while we've dropped the lock.
> >  		 */
> > -		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);
> > -		}
> > +		if (!needs_clflush_after)
> > +			i915_gem_object_flush_cpu_write_domain(obj);
> 
> Nack. You've just inverted the write domain check.

Indeed, brain must have been offline. And thinking about this more this
really is a low-level clflush from a function playing tricks, so doesn't
make sense to use higher level functions anyway.

I'll respin with just the 2nd hunk and try to explain a bit better why
it's ok, like Chris requested.
-Daniel

> 
> >  	}
> >  
> >  	if (needs_clflush_after)
> > @@ -3825,12 +3822,8 @@ 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);
> > -	}
> > +	if (obj->cache_dirty)
> > +		i915_gem_object_flush_cpu_write_domain(obj);
> >  
> >  	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

-- 
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 2/2] drm/i915: Remove open-coded callers of flush_cpu_write_domain

[Ville Syrjälä]

On Wed, Jan 21, 2015 at 03:55:18PM +0100, Daniel Vetter wrote:
> On Wed, Jan 21, 2015 at 04:12:07PM +0200, Ville Syrjälä wrote:
> > On Wed, Jan 21, 2015 at 02:53:49PM +0100, Daniel Vetter wrote:
> > > Both places check the cpu domain and through some indirection
> > > obj->pin_display and whether cpu access is coherent. And then flush
> > > both cpu caches and the chipset cache. Which is what
> > > flush_cpu_write_domain does.
> > > 
> > > Only difference is that on top we get a tracepoint, frontbuffer
> > > tracking and and domain tracking updates. Which doesn't matter since
> > > both paths are slowpaths.
> > > 
> > > So lets go with more shared code for clearer idioms.
> > > 
> > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > Cc: Chris Wilson <chris@chris-wilson.co.uk>
> > > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/i915_gem.c | 15 ++++-----------
> > >  1 file changed, 4 insertions(+), 11 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> > > index 783d1040bf83..b48c39230b80 100644
> > > --- a/drivers/gpu/drm/i915/i915_gem.c
> > > +++ b/drivers/gpu/drm/i915/i915_gem.c
> > > @@ -1019,11 +1019,8 @@ out:
> > >  		 * cachelines in-line while writing and the object moved
> > >  		 * out of the cpu write domain while we've dropped the lock.
> > >  		 */
> > > -		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);
> > > -		}
> > > +		if (!needs_clflush_after)
> > > +			i915_gem_object_flush_cpu_write_domain(obj);
> > 
> > Nack. You've just inverted the write domain check.
> 
> Indeed, brain must have been offline. And thinking about this more this
> really is a low-level clflush from a function playing tricks, so doesn't
> make sense to use higher level functions anyway.
> 
> I'll respin with just the 2nd hunk and try to explain a bit better why
> it's ok, like Chris requested.

The 2nd hunk has the same problem.

> -Daniel
> 
> > 
> > >  	}
> > >  
> > >  	if (needs_clflush_after)
> > > @@ -3825,12 +3822,8 @@ 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);
> > > -	}
> > > +	if (obj->cache_dirty)
> > > +		i915_gem_object_flush_cpu_write_domain(obj);
> > >  
> > >  	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
> 
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> +41 (0) 79 365 57 48 - http://blog.ffwll.ch

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