Re: [PATCH] drm/xe/debugfs: Do not register residency files for SRIOV VF

[Rodrigo Vivi <rodrigo.vivi@intel.com>]

On Tue, Jul 29, 2025 at 04:04:38PM -0300, Gustavo Sousa wrote:
> Quoting Cavitt, Jonathan (2025-07-29 15:41:00-03:00)
> >-----Original Message-----
> >From: Sousa, Gustavo <gustavo.sousa@intel.com> 
> >Sent: Tuesday, July 29, 2025 9:47 AM
> >To: intel-xe@lists.freedesktop.org
> >Cc: Purkait, Soham <soham.purkait@intel.com>; Cavitt, Jonathan <jonathan.cavitt@intel.com>; Poosa, Karthik <karthik.poosa@intel.com>; Tauro, Riana <riana.tauro@intel.com>; Vivi, Rodrigo <rodrigo.vivi@intel.com>; Sousa, Gustavo <gustavo.sousa@intel.com>
> >Subject: [PATCH] drm/xe/debugfs: Do not register residency files for SRIOV VF
> >> 
> >> Registers accessed by the recently added[1] residency counters debugfs
> >> are marked as inaccessible to SRIOV VFs. That causes warnings to be
> >> raised when an attempt is made to read those files. Skip registering
> >> those files for VFs.
> >> 
> >> [1] Via commit 487579fd8524 ("drm/xe/xe_debugfs: Exposure of G-State and
> >>     pcie link state residency counters through debugfs").

This duplication is not needed. Just use the Fixes tag here before the Cc list.

> >> 
> >> Cc: Soham Purkait <soham.purkait@intel.com>
> >> Cc: Jonathan Cavitt <jonathan.cavitt@intel.com>
> >> Cc: Karthik Poosa <karthik.poosa@intel.com>
> >> Cc: Riana Tauro <riana.tauro@intel.com>
> >> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> >> Fixes: 487579fd8524 ("drm/xe/xe_debugfs: Exposure of G-State and pcie link state residency counters through debugfs")
> >> Signed-off-by: Gustavo Sousa <gustavo.sousa@intel.com>
> >> ---
> >>  drivers/gpu/drm/xe/xe_debugfs.c | 2 +-
> >>  1 file changed, 1 insertion(+), 1 deletion(-)
> >> 
> >> diff --git a/drivers/gpu/drm/xe/xe_debugfs.c b/drivers/gpu/drm/xe/xe_debugfs.c
> >> index 0b4a532f7c45..53b89528692a 100644
> >> --- a/drivers/gpu/drm/xe/xe_debugfs.c
> >> +++ b/drivers/gpu/drm/xe/xe_debugfs.c
> >> @@ -360,7 +360,7 @@ void xe_debugfs_register(struct xe_device *xe)
> >>                                   ARRAY_SIZE(debugfs_list),
> >>                                   root, minor);
> >>  
> >> -        if (xe->info.platform == XE_BATTLEMAGE)
> >> +        if (!IS_SRIOV_VF(xe) && xe->info.platform == XE_BATTLEMAGE)
> >>                  drm_debugfs_create_files(debugfs_residencies,
> >
> >It sounds like it's specifically these debugfs_residencies files, and not a property of
> >drm_debugfs_create_files, that has troubles on VF.

The commit message let's that clear:

"Registers accessed ... are marked as inaccessible to SRIOV VFs"

The important part of a review in a patch like this is to go and check the Spec
information. Even when it looks obvious that telemetry data should only be
accessible it is expected that the reviewer confirms the given information.

Gustavo, could you please point out where in the spec this is documented?

Is there any bug opened or any backtrace that should be in the commit message
as well?

> 
> I think we should not advertise something that we do not support, that's
> why my suggestion is to not even create the files if we do not support
> getting such information for VFs (at least not with the current
> implementation).

Yes, I agree that this is the best approach. Do not even expose the files
if functionality is not available.

Thanks,
Rodrigo.

> 
> --
> Gustavo Sousa
> 
> >
> >I don't see a reason to block this, so
> >Reviewed-by: Jonathan Cavitt <jonathan.cavitt@intel.com>
> >-Jonathan Cavitt
> >
> >>                                           ARRAY_SIZE(debugfs_residencies),
> >>                                           root, minor);
> >> 
> >> ---
> >> base-commit: b6a72d53a8082ee6ef701042e7cb8a93d6a2b678
> >> change-id: 20250729-skip-residency-debugfs-for-vfs-4d707779fcb1
> >> 
> >> Best regards,
> >> -- 
> >> Gustavo Sousa <gustavo.sousa@intel.com>
> >> 
> >>