Re: [Intel-xe] [PATCH v12 04/13] drm/xe/guc_pc: add missing mem_access for freq_rpe_show

[Matthew Auld <matthew.auld@intel.com>]

Hi,

On 27/06/2023 07:53, Gupta, Anshuman wrote:
> 
> 
>> -----Original Message----- From: Auld, Matthew 
>> <matthew.auld@intel.com> Sent: Monday, June 26, 2023 4:21 PM To: 
>> intel-xe@lists.freedesktop.org Cc: Brost, Matthew 
>> <matthew.brost@intel.com>; Vivi, Rodrigo <rodrigo.vivi@intel.com>; 
>> Gupta, Anshuman <anshuman.gupta@intel.com> Subject: [PATCH v12 
>> 04/13] drm/xe/guc_pc: add missing mem_access for freq_rpe_show
>> 
>> The mem_access is meant to cover any kind of device level memory 
>> access, mmio included.
>> 
>> Signed-off-by: Matthew Auld <matthew.auld@intel.com> Cc: Matthew 
>> Brost <matthew.brost@intel.com> Cc: Rodrigo Vivi 
>> <rodrigo.vivi@intel.com> Cc: Anshuman Gupta 
>> <anshuman.gupta@intel.com> --- drivers/gpu/drm/xe/xe_guc_pc.c | 4 
>> ++++ 1 file changed, 4 insertions(+)
>> 
>> diff --git a/drivers/gpu/drm/xe/xe_guc_pc.c 
>> b/drivers/gpu/drm/xe/xe_guc_pc.c index 5d5cf4b0d508..e3722a805837 
>> 100644 --- a/drivers/gpu/drm/xe/xe_guc_pc.c +++ 
>> b/drivers/gpu/drm/xe/xe_guc_pc.c @@ -430,8 +430,12 @@ static 
>> ssize_t freq_rpe_show(struct device *dev, struct device_attribute 
>> *attr, char *buf)  { struct xe_guc_pc *pc = dev_to_pc(dev); + 
>> struct xe_gt *gt = pc_to_gt(pc); +	struct xe_device *xe = 
>> gt_to_xe(gt);
>> 
>> +	xe_device_mem_access_get(xe);
> What is desirable to use here , xe_pm_runtime_get() or 
> xe_device_mem_access_get() ?

Rodrigo suggested using xe_device_mem_access_get() for MMIO, since it 
can be thought of as just another type of memory access, which seems 
pretty reasonable to me. Matt Roper also had thoughts about it here:
https://patchwork.freedesktop.org/patch/542538/?series=119345&rev=1

I don't have a strong opinion either way, just so long as we are 
consistent about it and have a reliable way to 
assert_device_active/assert_mem_access.

> It is register mmio access Few places in driver using 
> xe_pm_runtime_get() for mmio access.

The one in xe_vm.c looks to be a workaround for the race in patch 1, 
which is fixed in this series. Ignoring that I only see the cases in 
compat-i915-headers/i915_drv.h, which looks to be a special case for 
display/i915, but that is also converted to use mem_access later in the 
series, since I wanted to try adding assert_mem_access from all MMIO 
calls to see what CI says (it complained about freq_rpe_show() as per 
this patch). Are there some other places I missed that are using 
xe_pm_runtime_get?

> Thanks, Anshuman Gupta.
>> pc_update_rp_values(pc); +	xe_device_mem_access_put(xe); return 
>> sysfs_emit(buf, "%d\n", pc->rpe_freq);  }  static 
>> DEVICE_ATTR_RO(freq_rpe); -- 2.41.0
>