Re: [PATCH] drm/i915 : Avoid superfluous invalidation of CPU cache lines

[Daniel Vetter <daniel@ffwll.ch>]

On Wed, Nov 25, 2015 at 02:57:47PM +0530, Goel, Akash wrote:
> 
> 
> On 11/25/2015 2:51 PM, Daniel Vetter wrote:
> >On Tue, Nov 24, 2015 at 10:39:38PM +0000, Chris Wilson wrote:
> >>On Tue, Nov 24, 2015 at 07:14:31PM +0100, Daniel Vetter wrote:
> >>>On Tue, Nov 24, 2015 at 12:04:06PM +0200, Ville Syrjälä wrote:
> >>>>On Tue, Nov 24, 2015 at 03:35:24PM +0530, akash.goel@intel.com wrote:
> >>>>>From: Akash Goel <akash.goel@intel.com>
> >>>>>
> >>>>>When the object is moved out of CPU read domain, the cachelines
> >>>>>are not invalidated immediately. The invalidation is deferred till
> >>>>>next time the object is brought back into CPU read domain.
> >>>>>But the invalidation is done unconditionally, i.e. even for the case
> >>>>>where the cachelines were flushed previously, when the object moved out
> >>>>>of CPU write domain. This is avoidable and would lead to some optimization.
> >>>>>Though this is not a hypothetical case, but is unlikely to occur often.
> >>>>>The aim is to detect changes to the backing storage whilst the
> >>>>>data is potentially in the CPU cache, and only clflush in those case.
> >>>>>
> >>>>>Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> >>>>>Signed-off-by: Akash Goel <akash.goel@intel.com>
> >>>>>---
> >>>>>  drivers/gpu/drm/i915/i915_drv.h | 1 +
> >>>>>  drivers/gpu/drm/i915/i915_gem.c | 9 ++++++++-
> >>>>>  2 files changed, 9 insertions(+), 1 deletion(-)
> >>>>>
> >>>>>diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> >>>>>index df9316f..fedb71d 100644
> >>>>>--- a/drivers/gpu/drm/i915/i915_drv.h
> >>>>>+++ b/drivers/gpu/drm/i915/i915_drv.h
> >>>>>@@ -2098,6 +2098,7 @@ struct drm_i915_gem_object {
> >>>>>  	unsigned long gt_ro:1;
> >>>>>  	unsigned int cache_level:3;
> >>>>>  	unsigned int cache_dirty:1;
> >>>>>+	unsigned int cache_clean:1;
> >>>>
> >>>>So now we have cache_dirty and cache_clean which seems redundant,
> >>>>except somehow cache_dirty != !cache_clean?
> >>
> >>Exactly, not entirely redundant. I did think something along MESI lines
> >>would be useful, but that didn't capture the different meanings we
> >>employ.
> >>
> >>cache_dirty tracks whether we have been eliding the clflush.
> >>
> >>cache_clean tracks whether we know the cache has been completely
> >>clflushed.
> >>
> >>(cache_clean implies !cache_dirty, but
> >>!cache_clean does not imply cache_dirty)
> >>
> >>>We also have read_domains & DOMAIN_CPU. Which is which?
> >>
> >>DOMAIN_CPU implies that the object may be in the cpu cache (modulo the
> >>clflush elision above).
> >>
> >>DOMAIN_CPU implies !cache_clean
> >>
> >>and even
> >>
> >>cache_clean implies !DOMAIN_CPU
> >>
> >>but
> >>
> >>!DOMAIN_CPU does not imply cache_clean
> >
> >All the above should be in the kerneldoc (per-struct-member comments
> >please) of drm_i915_gem_object. Akash, can you please amend your patch?
> >And please make sure we do include that kerneldoc somewhere ... might need
> >an upfront patch to do that, for just drm_i915_gem_object.
> 
> I floated the amended patch, earlier today,
> http://lists.freedesktop.org/archives/intel-gfx/2015-November/081194.html.
> Please kindly check that.

Already done and replied here because I think this should be lifted to
kerneldoc for the structure itself. That's why I replied here ;-)
-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