| Message ID | f20bd21c94610dae59824b8040e5a9400de6f963.1657800199.git.mchehab@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Fix performance regressions with TLB and add GuC support | expand |
On 14/07/2022 13:06, Mauro Carvalho Chehab wrote: > From: Chris Wilson <chris.p.wilson@intel.com> > > Skip all further TLB invalidations once the device is wedged and > had been reset, as, on such cases, it can no longer process instructions > on the GPU and the user no longer has access to the TLB's in each engine. > > That helps to reduce the performance regression introduced by TLB > invalidate logic. > > Cc: stable@vger.kernel.org > Fixes: 7938d61591d3 ("drm/i915: Flush TLBs before releasing backing store") Is the claim of a performance regression this solved based on a wedged GPU which does not work any more to the extend where mmio tlb invalidation requests keep timing out? If so please clarify in the commit text and then it looks good to me. Even if it is IMO a very borderline situation to declare something a fix. Regards, Tvrtko > Signed-off-by: Chris Wilson <chris.p.wilson@intel.com> > Cc: Fei Yang <fei.yang@intel.com> > Cc: Andi Shyti <andi.shyti@linux.intel.com> > Acked-by: Thomas Hellström <thomas.hellstrom@linux.intel.com> > Signed-off-by: Mauro Carvalho Chehab <mchehab@kernel.org> > --- > > To avoid mailbombing on a large number of people, only mailing lists were C/C on the cover. > See [PATCH v2 00/21] at: https://lore.kernel.org/all/cover.1657800199.git.mchehab@kernel.org/ > > drivers/gpu/drm/i915/gt/intel_gt.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/drivers/gpu/drm/i915/gt/intel_gt.c b/drivers/gpu/drm/i915/gt/intel_gt.c > index 1d84418e8676..5c55a90672f4 100644 > --- a/drivers/gpu/drm/i915/gt/intel_gt.c > +++ b/drivers/gpu/drm/i915/gt/intel_gt.c > @@ -934,6 +934,9 @@ void intel_gt_invalidate_tlbs(struct intel_gt *gt) > if (I915_SELFTEST_ONLY(gt->awake == -ENODEV)) > return; > > + if (intel_gt_is_wedged(gt)) > + return; > + > if (GRAPHICS_VER(i915) == 12) { > regs = gen12_regs; > num = ARRAY_SIZE(gen12_regs);
On Mon, 18 Jul 2022 14:45:22 +0100 Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com> wrote: > On 14/07/2022 13:06, Mauro Carvalho Chehab wrote: > > From: Chris Wilson <chris.p.wilson@intel.com> > > > > Skip all further TLB invalidations once the device is wedged and > > had been reset, as, on such cases, it can no longer process instructions > > on the GPU and the user no longer has access to the TLB's in each engine. > > > > That helps to reduce the performance regression introduced by TLB > > invalidate logic. > > > > Cc: stable@vger.kernel.org > > Fixes: 7938d61591d3 ("drm/i915: Flush TLBs before releasing backing store") > > Is the claim of a performance regression this solved based on a wedged > GPU which does not work any more to the extend where mmio tlb > invalidation requests keep timing out? If so please clarify in the > commit text and then it looks good to me. Even if it is IMO a very > borderline situation to declare something a fix. Indeed this helps on a borderline situation: if GT is wedged, TLB invalidation will timeout, so it makes sense to keep the patch with a comment like: drm/i915/gt: Skip TLB invalidations once wedged Skip all further TLB invalidations once the device is wedged and had been reset, as, on such cases, it can no longer process instructions on the GPU and the user no longer has access to the TLB's in each engine. So, an attempt to do a TLB cache invalidation will produce a timeout. That helps to reduce the performance regression introduced by TLB invalidate logic. Regards, Mauro
On 18/07/2022 17:06, Mauro Carvalho Chehab wrote: > On Mon, 18 Jul 2022 14:45:22 +0100 > Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com> wrote: > >> On 14/07/2022 13:06, Mauro Carvalho Chehab wrote: >>> From: Chris Wilson <chris.p.wilson@intel.com> >>> >>> Skip all further TLB invalidations once the device is wedged and >>> had been reset, as, on such cases, it can no longer process instructions >>> on the GPU and the user no longer has access to the TLB's in each engine. >>> >>> That helps to reduce the performance regression introduced by TLB >>> invalidate logic. >>> >>> Cc: stable@vger.kernel.org >>> Fixes: 7938d61591d3 ("drm/i915: Flush TLBs before releasing backing store") >> >> Is the claim of a performance regression this solved based on a wedged >> GPU which does not work any more to the extend where mmio tlb >> invalidation requests keep timing out? If so please clarify in the >> commit text and then it looks good to me. Even if it is IMO a very >> borderline situation to declare something a fix. > > Indeed this helps on a borderline situation: if GT is wedged, TLB > invalidation will timeout, so it makes sense to keep the patch with a > comment like: > > drm/i915/gt: Skip TLB invalidations once wedged > > Skip all further TLB invalidations once the device is wedged and > had been reset, as, on such cases, it can no longer process instructions > on the GPU and the user no longer has access to the TLB's in each engine. > > So, an attempt to do a TLB cache invalidation will produce a timeout. > > That helps to reduce the performance regression introduced by TLB > invalidate logic. Yeah that is better but whether bothering stable with it is the question. Wedged GPU means constant endless -EIO to userspace so very hard to imagine that after a TLB invalidation timeout or two there would be further ones. But okay, it's tiny so fine I guess. Regards, Tvrtko
Hi Mauro, On Thu, Jul 14, 2022 at 01:06:10PM +0100, Mauro Carvalho Chehab wrote: > From: Chris Wilson <chris.p.wilson@intel.com> > > Skip all further TLB invalidations once the device is wedged and > had been reset, as, on such cases, it can no longer process instructions > on the GPU and the user no longer has access to the TLB's in each engine. > > That helps to reduce the performance regression introduced by TLB > invalidate logic. > > Cc: stable@vger.kernel.org > Fixes: 7938d61591d3 ("drm/i915: Flush TLBs before releasing backing store") > Signed-off-by: Chris Wilson <chris.p.wilson@intel.com> > Cc: Fei Yang <fei.yang@intel.com> > Cc: Andi Shyti <andi.shyti@linux.intel.com> > Acked-by: Thomas Hellström <thomas.hellstrom@linux.intel.com> > Signed-off-by: Mauro Carvalho Chehab <mchehab@kernel.org> I haven't read any concern from Tvrtko here, in any case: Reviewed-by: Andi Shyti <andi.shyti@linux.intel.com> thanks, Andi
diff --git a/drivers/gpu/drm/i915/gt/intel_gt.c b/drivers/gpu/drm/i915/gt/intel_gt.c index 1d84418e8676..5c55a90672f4 100644 --- a/drivers/gpu/drm/i915/gt/intel_gt.c +++ b/drivers/gpu/drm/i915/gt/intel_gt.c @@ -934,6 +934,9 @@ void intel_gt_invalidate_tlbs(struct intel_gt *gt) if (I915_SELFTEST_ONLY(gt->awake == -ENODEV)) return; + if (intel_gt_is_wedged(gt)) + return; + if (GRAPHICS_VER(i915) == 12) { regs = gen12_regs; num = ARRAY_SIZE(gen12_regs);