[Intel-gfx] [PATCH] drm/i915 : Avoid superfluous invalidation of CPU cache lines

[Daniel Vetter]

On Mon, Nov 30, 2015 at 11:54:14AM +0530, Goel, Akash wrote:
> 
> 
> On 11/25/2015 3:30 PM, Daniel Vetter wrote:
> >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 at intel.com wrote:
> >>>>>>>From: Akash Goel <akash.goel at 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 at chris-wilson.co.uk>
> >>>>>>>Signed-off-by: Akash Goel <akash.goel at 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
> Hi Daniel,
> 
> I think the patch to provide a kernel-doc for just the drm_i915_gem_object
> structure can be submitted independently of this patch. The kernel-doc part
> can be done as a follow up patch.

Imo it should be done first, so that your cache optimization can also
correctly update the documentation.
-Daniel

> 
> For the current patch, have added the per-struct-member comments for the
> 'cache_clean' field.
> 
> Best regards
> Akash
> 
> >

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch