| Message ID | 20210914195151.560793-1-lucas.demarchi@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/i915/guc/slpc: remove unneeded clflush calls | expand |
On 9/14/2021 12:51 PM, Lucas De Marchi wrote: > The clflush calls here aren't doing anything since we are not writting > something and flushing the cache lines to be visible to GuC. Here the > intention seems to be to make sure whatever GuC has written is visible > to the CPU before we read them. However a clflush from the CPU side is > the wrong instruction to use. > > From code inspection on the other clflush() calls in i915/gt/uc/ these > are the only ones with this behavrior. The others are apparently making > sure what we write is visible to GuC. > > Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com> > --- > drivers/gpu/drm/i915/gt/uc/intel_guc_slpc.c | 3 --- > 1 file changed, 3 deletions(-) > > diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_slpc.c b/drivers/gpu/drm/i915/gt/uc/intel_guc_slpc.c > index 65a3e7fdb2b2..2e996b77df80 100644 > --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_slpc.c > +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_slpc.c > @@ -108,7 +108,6 @@ static u32 slpc_get_state(struct intel_guc_slpc *slpc) > > GEM_BUG_ON(!slpc->vma); > > - drm_clflush_virt_range(slpc->vaddr, sizeof(u32)); > data = slpc->vaddr; > > return data->header.global_state; > @@ -172,8 +171,6 @@ static int slpc_query_task_state(struct intel_guc_slpc *slpc) > drm_err(&i915->drm, "Failed to query task state (%pe)\n", > ERR_PTR(ret)); > > - drm_clflush_virt_range(slpc->vaddr, SLPC_PAGE_SIZE_BYTES); > - LGTM. Reviewed-by: Vinay Belgaumkar <vinay.belgaumkar@intel.com> > return ret; > } > >
On 9/15/2021 12:24, Belgaumkar, Vinay wrote: > On 9/14/2021 12:51 PM, Lucas De Marchi wrote: >> The clflush calls here aren't doing anything since we are not writting >> something and flushing the cache lines to be visible to GuC. Here the >> intention seems to be to make sure whatever GuC has written is visible >> to the CPU before we read them. However a clflush from the CPU side is >> the wrong instruction to use. Is there a right instruction to use? Either we need to verify that no flush/invalidate is required or we need to add in a replacement that does the correct thing? John. >> >> From code inspection on the other clflush() calls in i915/gt/uc/ these >> are the only ones with this behavrior. The others are apparently making >> sure what we write is visible to GuC. >> >> Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com> >> --- >> drivers/gpu/drm/i915/gt/uc/intel_guc_slpc.c | 3 --- >> 1 file changed, 3 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_slpc.c >> b/drivers/gpu/drm/i915/gt/uc/intel_guc_slpc.c >> index 65a3e7fdb2b2..2e996b77df80 100644 >> --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_slpc.c >> +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_slpc.c >> @@ -108,7 +108,6 @@ static u32 slpc_get_state(struct intel_guc_slpc >> *slpc) >> GEM_BUG_ON(!slpc->vma); >> - drm_clflush_virt_range(slpc->vaddr, sizeof(u32)); >> data = slpc->vaddr; >> return data->header.global_state; >> @@ -172,8 +171,6 @@ static int slpc_query_task_state(struct >> intel_guc_slpc *slpc) >> drm_err(&i915->drm, "Failed to query task state (%pe)\n", >> ERR_PTR(ret)); >> - drm_clflush_virt_range(slpc->vaddr, SLPC_PAGE_SIZE_BYTES); >> - > > LGTM. > Reviewed-by: Vinay Belgaumkar <vinay.belgaumkar@intel.com> > >> return ret; >> } >>
On Wed, Sep 15, 2021 at 12:29:12PM -0700, John Harrison wrote: >On 9/15/2021 12:24, Belgaumkar, Vinay wrote: >>On 9/14/2021 12:51 PM, Lucas De Marchi wrote: >>>The clflush calls here aren't doing anything since we are not writting >>>something and flushing the cache lines to be visible to GuC. Here the >>>intention seems to be to make sure whatever GuC has written is visible >>>to the CPU before we read them. However a clflush from the CPU side is >>>the wrong instruction to use. >Is there a right instruction to use? Either we need to verify that no how can there be a right instruction? If the GuC needs to flush, then the GuC needs to do it, nothing to be done by the CPU. Flushing the CPU cache line here is doing nothing to guarantee that what was written by GuC hit the memory and we are reading it. Not sure why it was actually added, but since it was added by Vinay and he reviewed this patch, I'm assuming he also agrees Lucas De Marchi
On Mon, Sep 20, 2021 at 10:47:08PM -0700, Lucas De Marchi wrote: > On Wed, Sep 15, 2021 at 12:29:12PM -0700, John Harrison wrote: > >On 9/15/2021 12:24, Belgaumkar, Vinay wrote: > >>On 9/14/2021 12:51 PM, Lucas De Marchi wrote: > >>>The clflush calls here aren't doing anything since we are not writting > >>>something and flushing the cache lines to be visible to GuC. Here the > >>>intention seems to be to make sure whatever GuC has written is visible > >>>to the CPU before we read them. However a clflush from the CPU side is > >>>the wrong instruction to use. > >Is there a right instruction to use? Either we need to verify that no > > how can there be a right instruction? If the GuC needs to flush, then > the GuC needs to do it, nothing to be done by the CPU. > > Flushing the CPU cache line here is doing nothing to guarantee that what > was written by GuC hit the memory and we are reading it. Not sure why it > was actually added, but since it was added by Vinay and he reviewed this > patch, I'm assuming he also agrees clflush == writeback + invalidate. The invalidate is the important part when the CPU has to read something written by something else that's not cache coherent. Now, I have no idea if the guc has its own (CPU invisible) caches or not. If it does then it will need to trigger a writeback. But regardless, if the guc bypasses the CPU caches the CPU will need to invalidate before it reads anything in case it has stale data sitting in its cache.
On Tue, Sep 21, 2021 at 04:06:00PM +0300, Ville Syrjälä wrote: >On Mon, Sep 20, 2021 at 10:47:08PM -0700, Lucas De Marchi wrote: >> On Wed, Sep 15, 2021 at 12:29:12PM -0700, John Harrison wrote: >> >On 9/15/2021 12:24, Belgaumkar, Vinay wrote: >> >>On 9/14/2021 12:51 PM, Lucas De Marchi wrote: >> >>>The clflush calls here aren't doing anything since we are not writting >> >>>something and flushing the cache lines to be visible to GuC. Here the >> >>>intention seems to be to make sure whatever GuC has written is visible >> >>>to the CPU before we read them. However a clflush from the CPU side is >> >>>the wrong instruction to use. >> >Is there a right instruction to use? Either we need to verify that no >> >> how can there be a right instruction? If the GuC needs to flush, then >> the GuC needs to do it, nothing to be done by the CPU. >> >> Flushing the CPU cache line here is doing nothing to guarantee that what >> was written by GuC hit the memory and we are reading it. Not sure why it >> was actually added, but since it was added by Vinay and he reviewed this >> patch, I'm assuming he also agrees > >clflush == writeback + invalidate. The invalidate is the important part >when the CPU has to read something written by something else that's not >cache coherent. Although the invalidate would be the important part, how would that work if there is still a flush? Wouldn't we be overriding whatever was written by the other side? Or are we using the fact that we shouldn't be writting to this cacheline so we know it's not dirty? > >Now, I have no idea if the guc has its own (CPU invisible) caches or not. >If it does then it will need to trigger a writeback. But regardless, if >the guc bypasses the CPU caches the CPU will need to invalidate before >it reads anything in case it has stale data sitting in its cache. Indeed, thanks... but another case would be if caches are coherent through snoop. Do you know what is the cache architecture with GuC and CPU? Another question comes to mind, but first some context: I'm looking at this in order to support other archs besides x86... the only platforms in which this would be relevant would be on the discrete ones (I'm currently running an arm64 guest on qemu and using pci passthrough). I see that for dgfx intel_guc_allocate_vma() uses i915_gem_object_create_lmem() instead of i915_gem_object_create_shmem(). Would that make a difference? thanks Lucas De Marchi
diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_slpc.c b/drivers/gpu/drm/i915/gt/uc/intel_guc_slpc.c index 65a3e7fdb2b2..2e996b77df80 100644 --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_slpc.c +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_slpc.c @@ -108,7 +108,6 @@ static u32 slpc_get_state(struct intel_guc_slpc *slpc) GEM_BUG_ON(!slpc->vma); - drm_clflush_virt_range(slpc->vaddr, sizeof(u32)); data = slpc->vaddr; return data->header.global_state; @@ -172,8 +171,6 @@ static int slpc_query_task_state(struct intel_guc_slpc *slpc) drm_err(&i915->drm, "Failed to query task state (%pe)\n", ERR_PTR(ret)); - drm_clflush_virt_range(slpc->vaddr, SLPC_PAGE_SIZE_BYTES); - return ret; }
The clflush calls here aren't doing anything since we are not writting something and flushing the cache lines to be visible to GuC. Here the intention seems to be to make sure whatever GuC has written is visible to the CPU before we read them. However a clflush from the CPU side is the wrong instruction to use. From code inspection on the other clflush() calls in i915/gt/uc/ these are the only ones with this behavrior. The others are apparently making sure what we write is visible to GuC. Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com> --- drivers/gpu/drm/i915/gt/uc/intel_guc_slpc.c | 3 --- 1 file changed, 3 deletions(-)