| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <8b9ae441-a291-fe45-ceac-be8c211a4f73@linux.intel.com>
Date: Thu, 16 Jun 2022 08:35:49 +0100
From: Tvrtko Ursulin <tvrtko.ursulin@...ux.intel.com>
To: Mauro Carvalho Chehab <mchehab@...nel.org>
Cc: Chris Wilson <chris.p.wilson@...el.com>,
Fei Yang <fei.yang@...el.com>,
Thomas Hellstrom <thomas.hellstrom@...el.com>,
Bruce Chang <yu.bruce.chang@...el.com>,
Daniel Vetter <daniel@...ll.ch>,
Dave Airlie <airlied@...hat.com>,
David Airlie <airlied@...ux.ie>,
Jani Nikula <jani.nikula@...ux.intel.com>,
John Harrison <John.C.Harrison@...el.com>,
Joonas Lahtinen <joonas.lahtinen@...ux.intel.com>,
Matt Roper <matthew.d.roper@...el.com>,
Matthew Brost <matthew.brost@...el.com>,
Rodrigo Vivi <rodrigo.vivi@...el.com>,
Tejas Upadhyay <tejaskumarx.surendrakumar.upadhyay@...el.com>,
Umesh Nerlige Ramappa <umesh.nerlige.ramappa@...el.com>,
dri-devel@...ts.freedesktop.org, intel-gfx@...ts.freedesktop.org,
linux-kernel@...r.kernel.org, mauro.chehab@...ux.intel.com,
Mika Kuoppala <mika.kuoppala@...ux.intel.com>,
Chris Wilson <chris@...is-wilson.co.uk>,
Andi Shyti <andi.shyti@...el.com>, stable@...r.kernel.org,
Thomas Hellström
<thomas.hellstrom@...ux.intel.com>
Subject: Re: [PATCH 5/6] drm/i915/gt: Serialize GRDOM access between multiple
engine resets
On 15/06/2022 16:27, Mauro Carvalho Chehab wrote:
> From: Chris Wilson <chris.p.wilson@...el.com>
>
> Don't allow two engines to be reset in parallel, as they would both
> try to select a reset bit (and send requests to common registers)
> and wait on that register, at the same time. Serialize control of
> the reset requests/acks using the uncore->lock, which will also ensure
> that no other GT state changes at the same time as the actual reset.
>
> Fixes: 7938d61591d3 ("drm/i915: Flush TLBs before releasing backing store")
Ah okay I get it, the fixes tag was applied indiscriminately to the
whole series. :) It definitely does not belong in this patch.
Otherwise LGTM:
Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@...el.com>
Regards,
Tvrtko
> Reported-by: Mika Kuoppala <mika.kuoppala@...ux.intel.com>
> Signed-off-by: Chris Wilson <chris@...is-wilson.co.uk>
> Cc: Mika Kuoppala <mika.kuoppala@...ux.intel.com>
> Cc: Andi Shyti <andi.shyti@...el.com>
> Cc: stable@...r.kernel.org
> Acked-by: Thomas Hellström <thomas.hellstrom@...ux.intel.com>
> Signed-off-by: Mauro Carvalho Chehab <mchehab@...nel.org>
> ---
>
> See [PATCH 0/6] at: https://lore.kernel.org/all/cover.1655306128.git.mchehab@kernel.org/
>
> drivers/gpu/drm/i915/gt/intel_reset.c | 37 ++++++++++++++++++++-------
> 1 file changed, 28 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/gt/intel_reset.c b/drivers/gpu/drm/i915/gt/intel_reset.c
> index a5338c3fde7a..c68d36fb5bbd 100644
> --- a/drivers/gpu/drm/i915/gt/intel_reset.c
> +++ b/drivers/gpu/drm/i915/gt/intel_reset.c
> @@ -300,9 +300,9 @@ static int gen6_hw_domain_reset(struct intel_gt *gt, u32 hw_domain_mask)
> return err;
> }
>
> -static int gen6_reset_engines(struct intel_gt *gt,
> - intel_engine_mask_t engine_mask,
> - unsigned int retry)
> +static int __gen6_reset_engines(struct intel_gt *gt,
> + intel_engine_mask_t engine_mask,
> + unsigned int retry)
> {
> struct intel_engine_cs *engine;
> u32 hw_mask;
> @@ -321,6 +321,20 @@ static int gen6_reset_engines(struct intel_gt *gt,
> return gen6_hw_domain_reset(gt, hw_mask);
> }
>
> +static int gen6_reset_engines(struct intel_gt *gt,
> + intel_engine_mask_t engine_mask,
> + unsigned int retry)
> +{
> + unsigned long flags;
> + int ret;
> +
> + spin_lock_irqsave(>->uncore->lock, flags);
> + ret = __gen6_reset_engines(gt, engine_mask, retry);
> + spin_unlock_irqrestore(>->uncore->lock, flags);
> +
> + return ret;
> +}
> +
> static struct intel_engine_cs *find_sfc_paired_vecs_engine(struct intel_engine_cs *engine)
> {
> int vecs_id;
> @@ -487,9 +501,9 @@ static void gen11_unlock_sfc(struct intel_engine_cs *engine)
> rmw_clear_fw(uncore, sfc_lock.lock_reg, sfc_lock.lock_bit);
> }
>
> -static int gen11_reset_engines(struct intel_gt *gt,
> - intel_engine_mask_t engine_mask,
> - unsigned int retry)
> +static int __gen11_reset_engines(struct intel_gt *gt,
> + intel_engine_mask_t engine_mask,
> + unsigned int retry)
> {
> struct intel_engine_cs *engine;
> intel_engine_mask_t tmp;
> @@ -583,8 +597,11 @@ static int gen8_reset_engines(struct intel_gt *gt,
> struct intel_engine_cs *engine;
> const bool reset_non_ready = retry >= 1;
> intel_engine_mask_t tmp;
> + unsigned long flags;
> int ret;
>
> + spin_lock_irqsave(>->uncore->lock, flags);
> +
> for_each_engine_masked(engine, gt, engine_mask, tmp) {
> ret = gen8_engine_reset_prepare(engine);
> if (ret && !reset_non_ready)
> @@ -612,17 +629,19 @@ static int gen8_reset_engines(struct intel_gt *gt,
> * This is best effort, so ignore any error from the initial reset.
> */
> if (IS_DG2(gt->i915) && engine_mask == ALL_ENGINES)
> - gen11_reset_engines(gt, gt->info.engine_mask, 0);
> + __gen11_reset_engines(gt, gt->info.engine_mask, 0);
>
> if (GRAPHICS_VER(gt->i915) >= 11)
> - ret = gen11_reset_engines(gt, engine_mask, retry);
> + ret = __gen11_reset_engines(gt, engine_mask, retry);
> else
> - ret = gen6_reset_engines(gt, engine_mask, retry);
> + ret = __gen6_reset_engines(gt, engine_mask, retry);
>
> skip_reset:
> for_each_engine_masked(engine, gt, engine_mask, tmp)
> gen8_engine_reset_cancel(engine);
>
> + spin_unlock_irqrestore(>->uncore->lock, flags);
> +
> return ret;
> }
>
Powered by blists - more mailing lists