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

["Goel, Akash" <akash.goel@intel.com>]

On 11/30/2015 1:45 PM, Daniel Vetter wrote:
> 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@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
>> 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

Hi Daniel,

I take an AR to later float a kernel-doc patch for the 
'drm_i915_gem_object' structure, on this pretext can you please consider 
this patch,
http://lists.freedesktop.org/archives/intel-gfx/2015-November/081515.html

Best regards
Akash

>

>>
>> For the current patch, have added the per-struct-member comments for the
>> 'cache_clean' field.
>>
>> Best regards
>> Akash
>>
>>>
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx