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

[Gupta, Anshuman]

> -----Original Message-----
> From: Auld, Matthew <matthew.auld at intel.com>
> Sent: Tuesday, June 27, 2023 1:51 PM
> To: Gupta, Anshuman <anshuman.gupta at intel.com>; intel-
> xe at lists.freedesktop.org
> Cc: Brost, Matthew <matthew.brost at intel.com>; Vivi, Rodrigo
> <rodrigo.vivi at intel.com>
> Subject: Re: [PATCH v12 04/13] drm/xe/guc_pc: add missing mem_access for
> freq_rpe_show
> 
> Hi,
> 
> On 27/06/2023 07:53, Gupta, Anshuman wrote:
> >
> >
> >> -----Original Message----- From: Auld, Matthew
> >> <matthew.auld at intel.com> Sent: Monday, June 26, 2023 4:21 PM To:
> >> intel-xe at lists.freedesktop.org Cc: Brost, Matthew
> >> <matthew.brost at intel.com>; Vivi, Rodrigo <rodrigo.vivi at intel.com>;
> >> Gupta, Anshuman <anshuman.gupta at 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 at intel.com> Cc: Matthew
> >> Brost <matthew.brost at intel.com> Cc: Rodrigo Vivi
> >> <rodrigo.vivi at intel.com> Cc: Anshuman Gupta
> >> <anshuman.gupta at 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 for explanation.
Reviewed-by: Anshuman Gupta <anshuman.gupta at intel.com>
> 
> > 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
> >