Re: [PATCH v2 2/2] drm/i915: Invalidate the guc ggtt TLB upon insertion

From: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
To: Chris Wilson <chris@chris-wilson.co.uk>,
	intel-gfx@lists.freedesktop.org,
	Daniel Vetter # v1 <daniel.vetter@ffwll.ch>
Subject: Re: [PATCH v2 2/2] drm/i915: Invalidate the guc ggtt TLB upon insertion
Date: Thu, 1 Dec 2016 11:04:26 +0000	[thread overview]
Message-ID: <d8d705bd-9ee0-17d5-b773-2aa6a85a3374@linux.intel.com> (raw)
In-Reply-To: <20161201105748.GH4580@nuc-i3427.alporthouse.com>

On 01/12/2016 10:57, Chris Wilson wrote:
> On Thu, Dec 01, 2016 at 10:49:31AM +0000, Tvrtko Ursulin wrote:
>>
>> On 01/12/2016 10:26, Chris Wilson wrote:
>>> On Thu, Dec 01, 2016 at 10:06:25AM +0000, Tvrtko Ursulin wrote:
>>>>
>>>> On 01/12/2016 09:46, Chris Wilson wrote:
>>>>> Move the GuC invalidation of its ggtt TLB to where we perform the ggtt
>>>>> modification rather than proliferate it into all the callers of the
>>>>> insert (which may or may not in fact have to do the insertion).
>>>>>
>>>>> v2: Just do the guc invalidate unconditionally, (afaict) it has no impact
>>>>> without the guc loaded on gen8+
>>>>
>>>> Why do you find it tempting to do it unconditionally? I would rather
>>>> not touch it on gen8 at all and would also prefer the conditional
>>>> flush in gen9.
>>>
>>> Because if I add a conditional here, I end up wanting writing a new vfunc
>>> for invalidate (if I can coax the gmch / gen6 / guc usage into something
>>> consistent). And I'm lazy. :)
>>
>> To make sure I fully understand - just because you would not like to
>> see the conditional in gen8_ggtt_invalidate? So you would add
>> gen8_ggtt_invalidate and gen9_ggtt_invalidate with a GuC flush?
>
> gen9_guc_ggtt_invalidate. Because I don't like the conditional in
> i915_ggtt_flush() - and that shows we have another place missing the guc
> invalidate. And also because at one point, I was toying with
> the idea of pushing the flush out to the caller of ->insert_page() in
> case they wanted to manipulate multiple pages. (However, that looks
> like it will remain single pages only.) I
>
>> I would have thought conditional is less bothersome than making the
>> unused piece of the GPU (on gen8) do stuff.
>
> I thought you wouldn't want to punch a bit on the register if it wasn't
> being used at all.

Yes I wouldn't, and not only unused but not validated probably.

Regards,

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