Re: [PATCH 3/3] drm/xe: Use VF_CAP_REG for device wmb

[Michal Wajdeczko <michal.wajdeczko@intel.com>]

On 27.08.2024 01:24, Dixit, Ashutosh wrote:
> On Tue, 02 Jul 2024 11:37:04 -0700, Michal Wajdeczko wrote:
>>
> 
> I have a couple of questions about xe_device_wmb() below:
> 
>> To force a write barrier on the device memory, we write to the
>> SOFTWARE_FLAGS_SPR33 register, but this particular register was
>> selected because it was one of the writable and unused register.
>>
>> Since a write barrier should also work if we use the read-only
>> register, switch to VF_CAP_REG register that is also marked as
>> accessible for VFs.
>>
>> While at it, add simple kernel-doc for xe_device_wmb() function.
>>
>> Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
>> Cc: Matt Roper <matthew.d.roper@intel.com>
>> ---
>>  drivers/gpu/drm/xe/xe_device.c | 11 ++++++++++-
>>  1 file changed, 10 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/xe/xe_device.c b/drivers/gpu/drm/xe/xe_device.c
>> index cfda7cb5df2c..74beddb55284 100644
>> --- a/drivers/gpu/drm/xe/xe_device.c
>> +++ b/drivers/gpu/drm/xe/xe_device.c
>> @@ -744,13 +744,22 @@ void xe_device_shutdown(struct xe_device *xe)
>>  {
>>  }
>>
>> +/**
>> + * xe_device_wmb() - Device specific write memory barrier
>> + * @xe: the &xe_device
>> + *
>> + * While wmb() is sufficient for a barrier if we use system memory, on discrete
>> + * platforms with device memory we additionally need to issue a register write.
>> + * Since it doesn't matter which register we write to, use the read-only VF_CAP
>> + * register that is also marked as accessible by the VFs.
>> + */
>>  void xe_device_wmb(struct xe_device *xe)
>>  {
>> 	struct xe_gt *gt = xe_root_mmio_gt(xe);
>>
>> 	wmb();
>> 	if (IS_DGFX(xe))
>> -		xe_mmio_write32(gt, SOFTWARE_FLAGS_SPR33, 0);
>> +		xe_mmio_write32(gt, VF_CAP_REG, 0);
> 
> * What is the purpose of this register write following wmb (aka sfence)?
>   As far as I understand, wmb/sfence should be sufficient by itself.
> 
>   If we wanted to flush PCI transactions, we should have to issue a PCI
>   read, but here we are not doing that.
> 
> * Why only Xe has such a function, why is such a function not present in
>   any of the other drivers, including i915?

what about [1] intel_guc_write_barrier

[1]
https://elixir.bootlin.com/linux/v6.11-rc5/source/drivers/gpu/drm/i915/gt/uc/intel_guc.c#L936

> 
> * Also, as an aside, use of write barriers in the kernel requires a comment
>   saying which read barrier the write barrier is paired with. Generally
>   speaking, such comments are missing in Xe.
> 
> I have sent a patch with xe_device_wmb() replaced just with wmb() here:
> 
> 	https://patchwork.freedesktop.org/series/137818/
> 
> Let's see if we see any failures in CI with this patch.
> 
> Thanks.
> --
> Ashutosh