Re: [Intel-gfx] [PATCH 1/2] i915/uncore: Acquire fw before loop in intel_uncore_read64_2x32

["Dixit, Ashutosh" <ashutosh.dixit@intel.com>]

On Mon, 07 Nov 2022 16:11:27 -0800, Umesh Nerlige Ramappa wrote:
>
> On Mon, Nov 07, 2022 at 01:23:19PM -0800, Dixit, Ashutosh wrote:
> > On Mon, 07 Nov 2022 02:13:46 -0800, Tvrtko Ursulin wrote:
> >>
> >> On 05/11/2022 00:32, Umesh Nerlige Ramappa wrote:
> >> > PMU reads the GT timestamp as a 2x32 mmio read and since upper and lower
> >> > 32 bit registers are read in a loop, there is a latency involved between
> >> > getting the GT timestamp and the CPU timestamp. As part of the
> >> > resolution, refactor intel_uncore_read64_2x32 to acquire forcewake and
> >> > uncore lock prior to reading upper and lower regs.
> >> >
> >> > Signed-off-by: Umesh Nerlige Ramappa <umesh.nerlige.ramappa@intel.com>
> >> > ---
> >> >   drivers/gpu/drm/i915/intel_uncore.h | 44 ++++++++++++++++++++---------
> >> >   1 file changed, 30 insertions(+), 14 deletions(-)
> >> >
> >> > diff --git a/drivers/gpu/drm/i915/intel_uncore.h b/drivers/gpu/drm/i915/intel_uncore.h
> >> > index 5449146a0624..e9e38490815d 100644
> >> > --- a/drivers/gpu/drm/i915/intel_uncore.h
> >> > +++ b/drivers/gpu/drm/i915/intel_uncore.h
> >> > @@ -382,20 +382,6 @@ __uncore_write(write_notrace, 32, l, false)
> >> >    */
> >> >   __uncore_read(read64, 64, q, true)
> >> >   -static inline u64
> >> > -intel_uncore_read64_2x32(struct intel_uncore *uncore,
> >> > -			 i915_reg_t lower_reg, i915_reg_t upper_reg)
> >> > -{
> >> > -	u32 upper, lower, old_upper, loop = 0;
> >> > -	upper = intel_uncore_read(uncore, upper_reg);
> >> > -	do {
> >> > -		old_upper = upper;
> >> > -		lower = intel_uncore_read(uncore, lower_reg);
> >> > -		upper = intel_uncore_read(uncore, upper_reg);
> >> > -	} while (upper != old_upper && loop++ < 2);
> >> > -	return (u64)upper << 32 | lower;
> >> > -}
> >> > -
> >> >   #define intel_uncore_posting_read(...) ((void)intel_uncore_read_notrace(__VA_ARGS__))
> >> >   #define intel_uncore_posting_read16(...) ((void)intel_uncore_read16_notrace(__VA_ARGS__))
> >> >   @@ -455,6 +441,36 @@ static inline void intel_uncore_rmw_fw(struct
> >> > intel_uncore *uncore,
> >> >		intel_uncore_write_fw(uncore, reg, val);
> >> >   }
> >> >   +static inline u64
> >> > +intel_uncore_read64_2x32(struct intel_uncore *uncore,
> >> > +			 i915_reg_t lower_reg, i915_reg_t upper_reg)
> >> > +{
> >> > +	u32 upper, lower, old_upper, loop = 0;
> >> > +	enum forcewake_domains fw_domains;
> >> > +	unsigned long flags;
> >> > +
> >> > +	fw_domains = intel_uncore_forcewake_for_reg(uncore, lower_reg,
> >> > +						    FW_REG_READ);
> >> > +
> >> > +	fw_domains |= intel_uncore_forcewake_for_reg(uncore, upper_reg,
> >> > +						    FW_REG_READ);
> >> > +
> >> > +	spin_lock_irqsave(&uncore->lock, flags);
> >> > +	intel_uncore_forcewake_get__locked(uncore, fw_domains);
> >> > +
> >> > +	upper = intel_uncore_read_fw(uncore, upper_reg);
> >> > +	do {
> >> > +		old_upper = upper;
> >> > +		lower = intel_uncore_read_fw(uncore, lower_reg);
> >> > +		upper = intel_uncore_read_fw(uncore, upper_reg);
> >> > +	} while (upper != old_upper && loop++ < 2);
> >> > +
> >> > +	intel_uncore_forcewake_put__locked(uncore, fw_domains);
> >>
> >> I mulled over the fact this no longer applies the put hysteresis, but then
> >> I saw GuC busyness is essentially the only current caller so thought it
> >> doesn't really warrant adding a super long named
> >> intel_uncore_forcewake_put_delayed__locked helper.
> >>
> >> Perhaps it would make sense to move this out of static inline,
>
> Are you saying - drop the inline OR drop static inline? I am assuming the
> former.

No you need to have 'static inline' for functions defined in a header
file. I also don't understand completely but seems what Tvrtko is saying is
move the function to the .c leaving only the declarations in the .h? Anyway
let Tvrtko explain more.

>
> >> in which
> >> case it would also be easier to have the hysteresis without needing to
> >> export any new helpers,
>
> I don't understand this part. Do you mean that it makes it easier to just
> call __intel_uncore_forcewake_put(uncore, fw_domains, true) then?

Yes I think this will work, drop the lock and call
__intel_uncore_forcewake_put.

> Just
> wondering how 'static inline' has any effect on that.
>
> >> but mostly because it does not feel the static
> >> inline is justified.
>
> Agree, just carried it over from the previous helper definition.
>
> >> Sounds an attractive option but it is passable as is.
> >
> > Yup, copy that. Also see now how this reduces the read latency. And also it
> > would increase the latency a bit for a different thread trying to do an
> > uncore read/write since we hold uncore->lock longer but should be ok I
> > think.
>
> Didn't think about it from that perspective. Worst case is that
> gt_park/gt_unpark may happen very frequently (as seen on some use
> cases). In that case, the unpark would end up calling this helper each
> time.
>
> >
> >> Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> >
> > Copy that too:
> >
> > Reviewed-by: Ashutosh Dixit <ashutosh.dixit@intel.com>
>
> Thanks,
> Umesh
> >
>
> >>
> >> > +	spin_unlock_irqrestore(&uncore->lock, flags);
> >> > +
> >> > +	return (u64)upper << 32 | lower;
> >> > +}
> >> > +
> >> >   static inline int intel_uncore_write_and_verify(struct intel_uncore *uncore,
> >> >						i915_reg_t reg, u32 val,
> >> >						u32 mask, u32 expected_val)