| Message ID | 20220128221020.188253-3-michael.cheng@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Use drm_clflush* instead of clflush | expand |
> -----Original Message----- > From: Cheng, Michael <michael.cheng@intel.com> > Sent: Friday, January 28, 2022 2:10 PM > To: intel-gfx@lists.freedesktop.org > Cc: Cheng, Michael <michael.cheng@intel.com>; Bowman, Casey G > <casey.g.bowman@intel.com>; De Marchi, Lucas > <lucas.demarchi@intel.com>; Boyer, Wayne <wayne.boyer@intel.com>; > ville.syrjala@linux.intel.com; Kuoppala, Mika <mika.kuoppala@intel.com>; > Auld, Matthew <matthew.auld@intel.com> > Subject: [PATCH v2 2/4] drm/i915/gt: Re-work invalidate_csb_entries > > Re-work invalidate_csb_entries to use drm_clflush_virt_range. This will > prevent compiler errors when building for non-x86 architectures. > > Signed-off-by: Michael Cheng <michael.cheng@intel.com> Reviewed-by: Casey Bowman <casey.g.bowman@intel.com> > --- > drivers/gpu/drm/i915/gt/intel_execlists_submission.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/i915/gt/intel_execlists_submission.c > b/drivers/gpu/drm/i915/gt/intel_execlists_submission.c > index 960a9aaf4f3a..90b5daf9433d 100644 > --- a/drivers/gpu/drm/i915/gt/intel_execlists_submission.c > +++ b/drivers/gpu/drm/i915/gt/intel_execlists_submission.c > @@ -1647,8 +1647,8 @@ cancel_port_requests(struct intel_engine_execlists > * const execlists, > > static void invalidate_csb_entries(const u64 *first, const u64 *last) { > - clflush((void *)first); > - clflush((void *)last); > + drm_clflush_virt_range((void *)first, sizeof(*first)); > + drm_clflush_virt_range((void *)last, sizeof(*last)); > } > > /* > -- > 2.25.1
On 28/01/2022 22:10, Michael Cheng wrote: > Re-work invalidate_csb_entries to use drm_clflush_virt_range. This will > prevent compiler errors when building for non-x86 architectures. > > Signed-off-by: Michael Cheng <michael.cheng@intel.com> > --- > drivers/gpu/drm/i915/gt/intel_execlists_submission.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/i915/gt/intel_execlists_submission.c b/drivers/gpu/drm/i915/gt/intel_execlists_submission.c > index 960a9aaf4f3a..90b5daf9433d 100644 > --- a/drivers/gpu/drm/i915/gt/intel_execlists_submission.c > +++ b/drivers/gpu/drm/i915/gt/intel_execlists_submission.c > @@ -1647,8 +1647,8 @@ cancel_port_requests(struct intel_engine_execlists * const execlists, > > static void invalidate_csb_entries(const u64 *first, const u64 *last) > { > - clflush((void *)first); > - clflush((void *)last); > + drm_clflush_virt_range((void *)first, sizeof(*first)); > + drm_clflush_virt_range((void *)last, sizeof(*last)); How about dropping the helper and from the single call site do: drm_clflush_virt_range(&buf[0], num_entries * sizeof(buf[0])); One less function call and CSB is a single cacheline before Gen11 ayway, two afterwards, so overall better conversion I think. How does that sound? Regards, Tvrtko > } > > /* >
Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com> writes: > On 28/01/2022 22:10, Michael Cheng wrote: >> Re-work invalidate_csb_entries to use drm_clflush_virt_range. This will >> prevent compiler errors when building for non-x86 architectures. >> >> Signed-off-by: Michael Cheng <michael.cheng@intel.com> >> --- >> drivers/gpu/drm/i915/gt/intel_execlists_submission.c | 4 ++-- >> 1 file changed, 2 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/gt/intel_execlists_submission.c b/drivers/gpu/drm/i915/gt/intel_execlists_submission.c >> index 960a9aaf4f3a..90b5daf9433d 100644 >> --- a/drivers/gpu/drm/i915/gt/intel_execlists_submission.c >> +++ b/drivers/gpu/drm/i915/gt/intel_execlists_submission.c >> @@ -1647,8 +1647,8 @@ cancel_port_requests(struct intel_engine_execlists * const execlists, >> >> static void invalidate_csb_entries(const u64 *first, const u64 *last) >> { >> - clflush((void *)first); >> - clflush((void *)last); >> + drm_clflush_virt_range((void *)first, sizeof(*first)); >> + drm_clflush_virt_range((void *)last, sizeof(*last)); > > How about dropping the helper and from the single call site do: > > drm_clflush_virt_range(&buf[0], num_entries * sizeof(buf[0])); > > One less function call and CSB is a single cacheline before Gen11 ayway, > two afterwards, so overall better conversion I think. How does that sound? It would definitely work. Now trying to remember why it went into explicit clflushes: iirc as this is gpu/cpu coherency, the wbinvd_on_all_cpus we get with *virt_range would then be just unnecessary perf hit. -Mika > > Regards, > > Tvrtko > >> } >> >> /* >>
On 31/01/2022 14:15, Mika Kuoppala wrote: > Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com> writes: > >> On 28/01/2022 22:10, Michael Cheng wrote: >>> Re-work invalidate_csb_entries to use drm_clflush_virt_range. This will >>> prevent compiler errors when building for non-x86 architectures. >>> >>> Signed-off-by: Michael Cheng <michael.cheng@intel.com> >>> --- >>> drivers/gpu/drm/i915/gt/intel_execlists_submission.c | 4 ++-- >>> 1 file changed, 2 insertions(+), 2 deletions(-) >>> >>> diff --git a/drivers/gpu/drm/i915/gt/intel_execlists_submission.c b/drivers/gpu/drm/i915/gt/intel_execlists_submission.c >>> index 960a9aaf4f3a..90b5daf9433d 100644 >>> --- a/drivers/gpu/drm/i915/gt/intel_execlists_submission.c >>> +++ b/drivers/gpu/drm/i915/gt/intel_execlists_submission.c >>> @@ -1647,8 +1647,8 @@ cancel_port_requests(struct intel_engine_execlists * const execlists, >>> >>> static void invalidate_csb_entries(const u64 *first, const u64 *last) >>> { >>> - clflush((void *)first); >>> - clflush((void *)last); >>> + drm_clflush_virt_range((void *)first, sizeof(*first)); >>> + drm_clflush_virt_range((void *)last, sizeof(*last)); >> >> How about dropping the helper and from the single call site do: >> >> drm_clflush_virt_range(&buf[0], num_entries * sizeof(buf[0])); >> >> One less function call and CSB is a single cacheline before Gen11 ayway, >> two afterwards, so overall better conversion I think. How does that sound? > > It would definitely work. Now trying to remember why it went into > explicit clflushes: iirc as this is gpu/cpu coherency, the > wbinvd_on_all_cpus we get with *virt_range would then be just > unnecessary perf hit. Right, apart that AFAICS wbinvd_on_all_cpus does not run on the X86_FEATURE_CLFLUSH path of drm_clflush_virt_range, which made me think invalidate_csb_entries might have been an a) optimisation which used the knowledge CSB is at most two cachelines large, and b) there is no need for the memory barrier since as you say it is about CPU/GPU effect so CPU ordering is not a concern. Anyway, larger hammer probably does not harm much, apart that it really should be one call to drm_clflush_virt_range. Regards, Tvrtko
diff --git a/drivers/gpu/drm/i915/gt/intel_execlists_submission.c b/drivers/gpu/drm/i915/gt/intel_execlists_submission.c index 960a9aaf4f3a..90b5daf9433d 100644 --- a/drivers/gpu/drm/i915/gt/intel_execlists_submission.c +++ b/drivers/gpu/drm/i915/gt/intel_execlists_submission.c @@ -1647,8 +1647,8 @@ cancel_port_requests(struct intel_engine_execlists * const execlists, static void invalidate_csb_entries(const u64 *first, const u64 *last) { - clflush((void *)first); - clflush((void *)last); + drm_clflush_virt_range((void *)first, sizeof(*first)); + drm_clflush_virt_range((void *)last, sizeof(*last)); } /*
Re-work invalidate_csb_entries to use drm_clflush_virt_range. This will prevent compiler errors when building for non-x86 architectures. Signed-off-by: Michael Cheng <michael.cheng@intel.com> --- drivers/gpu/drm/i915/gt/intel_execlists_submission.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)