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

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

On 12/1/2015 7:30 PM, Ville Syrjälä wrote:
> On Tue, Dec 01, 2015 at 01:49:10PM +0000, Chris Wilson wrote:
>> On Tue, Dec 01, 2015 at 03:28:28PM +0200, Ville Syrjälä wrote:
>>> On Tue, Dec 01, 2015 at 01:09:33PM +0000, Chris Wilson wrote:
>>>> On Tue, Dec 01, 2015 at 02:34:41PM +0200, Ville Syrjälä wrote:
>>>>> On Mon, Nov 30, 2015 at 12:41:05PM +0530, akash.goel@intel.com wrote:
>>>>>> @@ -3982,7 +3983,21 @@ i915_gem_object_set_to_cpu_domain(struct drm_i915_gem_object *obj, bool write)
>>>>>>
>>>>>>   	/* Flush the CPU cache if it's still invalid. */
>>>>>>   	if ((obj->base.read_domains & I915_GEM_DOMAIN_CPU) == 0) {
>>>>>> -		i915_gem_clflush_object(obj, false);
>>>>>> +		/* If an object is moved out of the CPU domain following a
>>>>>> +		 * CPU write and before a GPU or GTT write, we will clflush
>>>>>> +		 * it out of the CPU cache, and mark the cache as clean.
>>>>>> +		 * After clflushing we know that this object cannot be in the
>>>>>> +		 * CPU cache, nor can it be speculatively loaded into the CPU
>>>>>> +		 * cache as our objects are page-aligned (& speculation cannot
>>>>>> +		 * cross page boundaries). Whilst this flag is set, we know
>>>>>> +		 * that any future access to the object's pages will miss the
>>>>>> +		 * stale cache and have to be serviced from main memory, i.e.
>>>>>> +		 * we do not need another clflush to invalidate the CPU cache
>>>>>> +		 * in preparing to read from the object.
>>>>>> +		 */
>>>>>> +		if (!obj->cache_clean)
>>>>>> +			i915_gem_clflush_object(obj, false);
>>>>>> +		obj->cache_clean = false;
>>>>>
>>>>> Having the comment here talk about moving stuff out of the cpu domain
>>>>> made me think there's a bug here (false vs. true). But actually this
>>>>> code moves it into the cpu domain so it's actually fine, I wonder if
>>>>> there's a better place for the comment (eg. where we do set
>>>>> cache_clean=true)?
>>>>
>>>> I thought it made more sense here because this is where we playing the
>>>> trick to avoid the clflush.
>>>>
>>>> Hmm, would s/If an object/When the object/ and
>>>> s/cache_clean/cache_flushed/ suffice?
>>>
>>> Maybe 'When the object is eventually moved out...' ?
>>>
>>> That extra word might convey more clearly that's it's not talking
>>> about moving it out right now.
>>
>> Hmm, the change of tense is good. Let's try that again:
>>
>> When the object was moved out the CPU domain following a CPU write, we
>> will have flushed it out of the CPU cache (and marked the object as
>> cache_flushed). After the clflush, we know that this object cannot be in
>> the CPU cache, nor can it be speculatively loaded into the CPU cache as
>> our objects are page-aligned and speculation cannot cross page
>> boundaries. So whilst the cache_flushed flag is set, we know that any
>> future access to the object's pages will miss the GPU cache and have to
>> be serviced from main memory (where they will pick up any writes
>> through the GTT or by the GPU) i.e. we do not need another clflush here
>> and now to invalidate the CPU cache as we prepare to read from the object.
>
> Hmm, yeah referring to past events clearly now. That does make more
> sense that referring to future events. lgtm

Thanks, will update the 'comments' in the next version.

And will also rename 'cache_clean' flag to 'cache_flushed'.

Best Regards
Akash


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