On Wed, Sep 27, 2023 at 11:03:54PM +0200, Nirmoy Das wrote:
> Implement intel_gt_mcr_lock_reset() to provide a mechanism
> for resetting the steer semaphore when absolutely necessary.
>
> Signed-off-by: Nirmoy Das <nirmoy.das@xxxxxxxxx>
> ---
> drivers/gpu/drm/i915/gt/intel_gt_mcr.c | 29 ++++++++++++++++++++++++++
> drivers/gpu/drm/i915/gt/intel_gt_mcr.h | 1 +
> 2 files changed, 30 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/gt/intel_gt_mcr.c b/drivers/gpu/drm/i915/gt/intel_gt_mcr.c
> index bf4a933de03a..d98e0d2fc2ee 100644
> --- a/drivers/gpu/drm/i915/gt/intel_gt_mcr.c
> +++ b/drivers/gpu/drm/i915/gt/intel_gt_mcr.c
> @@ -419,6 +419,35 @@ void intel_gt_mcr_unlock(struct intel_gt *gt, unsigned long flags)
> intel_uncore_write_fw(gt->uncore, MTL_STEER_SEMAPHORE, 0x1);
> }
>
> +/**
> + * intel_gt_mcr_lock_reset - Reset MCR steering lock
> + * @gt: GT structure
> + *
> + * Performs a steer semaphore reset operation. On MTL and beyond, a hardware
> + * lock will also be taken to serialize access not only for the driver,
> + * but also for external hardware and firmware agents.
The text here makes it sound like this reset function is going to take
the lock. Since we have the same language in the lock() function's
kerneldoc, I think you can just delete this whole sentence.
> + * However, there may be situations where the driver must reset the semaphore
> + * but only when it is absolutely certain that no other agent should own the
> + * lock at that given time.
This part leads to questions about what such situations would be and how
we'd know it's safe to use. Maybe it's best to just say something like
"This will be used to sanitize the initial status of the hardware lock
during driver load and resume since there won't be any concurrent access
from other agents at those times, but it's possible that boot firmware
may have left the lock in a bad state."
> + *
> + * Context: Takes gt->mcr_lock. uncore->lock should *not* be held when this
> + * function is called, although it may be acquired after this
> + * function call.
> + */
> +void intel_gt_mcr_lock_reset(struct intel_gt *gt)
> +{
> + unsigned long __flags;
> +
> + lockdep_assert_not_held(>->uncore->lock);
> +
> + spin_lock_irqsave(>->mcr_lock, __flags);
If we're doing this to sanitize at load/resume, then presumably we
shouldn't ever be racing with other driver threads either, right? If it
was possible for some other thread to already be grabbing the MCR lock,
then that would mean it also isn't safe for us to reset it here either.
Matt
> +
> + if (GRAPHICS_VER_FULL(gt->i915) >= IP_VER(12, 70))
> + intel_uncore_write_fw(gt->uncore, MTL_STEER_SEMAPHORE, 0x1);
> +
> + spin_unlock_irqrestore(>->mcr_lock, __flags);
> +}
> +
> /**
> * intel_gt_mcr_read - read a specific instance of an MCR register
> * @gt: GT structure
> diff --git a/drivers/gpu/drm/i915/gt/intel_gt_mcr.h b/drivers/gpu/drm/i915/gt/intel_gt_mcr.h
> index 41684495b7da..485c7711f2e8 100644
> --- a/drivers/gpu/drm/i915/gt/intel_gt_mcr.h
> +++ b/drivers/gpu/drm/i915/gt/intel_gt_mcr.h
> @@ -11,6 +11,7 @@
> void intel_gt_mcr_init(struct intel_gt *gt);
> void intel_gt_mcr_lock(struct intel_gt *gt, unsigned long *flags);
> void intel_gt_mcr_unlock(struct intel_gt *gt, unsigned long flags);
> +void intel_gt_mcr_lock_reset(struct intel_gt *gt);
>
> u32 intel_gt_mcr_read(struct intel_gt *gt,
> i915_mcr_reg_t reg,
> --
> 2.41.0
>
--
Matt Roper
Graphics Software Engineer
Linux GPU Platform Enablement
Intel Corporation
