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

[Umesh Nerlige Ramappa <umesh.nerlige.ramappa@intel.com>]

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.

>> 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?  
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)