Re: [PATCH v7 1/4] drm/i915: Introduce intel_gt_mcr_lock_sanitize()

[Nirmoy Das <nirmoy.das@xxxxxxxxxxxxxxx>]

On 9/28/2023 11:42 PM, Andrzej Hajda wrote:
> On 28.09.2023 15:00, Nirmoy Das wrote:
> > Implement intel_gt_mcr_lock_sanitize() to provide a mechanism
> > for cleaning the steer semaphore when absolutely necessary.
> >
> > v2: remove unnecessary lock(Andi, Matt)
> >      improve the kernel doc(Matt)
> >      s/intel_gt_mcr_lock_clear/intel_gt_mcr_lock_sanitize
> >
> > Signed-off-by: Nirmoy Das <nirmoy.das@xxxxxxxxx>
> > ---
> >   drivers/gpu/drm/i915/gt/intel_gt_mcr.c | 22 ++++++++++++++++++++++
> >   drivers/gpu/drm/i915/gt/intel_gt_mcr.h |  1 +
> >   2 files changed, 23 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..326c2ed1d99b 100644
> > --- a/drivers/gpu/drm/i915/gt/intel_gt_mcr.c
> > +++ b/drivers/gpu/drm/i915/gt/intel_gt_mcr.c
> > @@ -419,6 +419,28 @@ 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_sanitize - Sanitize MCR steering lock
> > + * @gt: GT structure
> > + *
> > + * 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.
> > + *
> > + */
> > +void intel_gt_mcr_lock_sanitize(struct intel_gt *gt)
> > +{
> > +    /*
> > +     * This gets called at load/resume time, so we shouldn't be
> > +     * racing with other driver threads grabbing the mcr lock.
> > +     */
> > +    lockdep_assert_not_held(&gt->mcr_lock);
> > +
> > +    if (GRAPHICS_VER_FULL(gt->i915) >= IP_VER(12, 70))
> > +        intel_uncore_write_fw(gt->uncore, MTL_STEER_SEMAPHORE, 0x1);
>
> I wonder if it wouldn't be useful to check and report if it is locked 
> before unconditional release, no strong feelings.
Not so useful for user but may be as debug log if we need.
> Reviewed-by: Andrzej Hajda <andrzej.hajda@xxxxxxxxx>


Thanks,

Nirmoy

> Regards
> Andrzej
>
>
> > +}
> > +
> >   /**
> >    * 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..01ac565a56a4 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_sanitize(struct intel_gt *gt);
> >     u32 intel_gt_mcr_read(struct intel_gt *gt,
> >                 i915_mcr_reg_t reg,