Re: [PATCH 2/3] drm/i915: Always flush the dirty CPU cache when pinning the scanout

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

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