Re: [PATCH] drm/xe/guc: Make creation of SLPC debugfs files conditional

["Summers, Stuart" <stuart.summers@intel.com>]

On Thu, 2025-05-15 at 23:06 +0530, Aradhya Bhatia wrote:
> Hi Tejas,
> 
> Thank you for reviewing the patch.
> 
> On 15-05-2025 18:46, Upadhyay, Tejas wrote:
> > 
> > 
> > > -----Original Message-----
> > > From: Bhatia, Aradhya <aradhya.bhatia@intel.com>
> > > Sent: 15 May 2025 15:19
> > > To: Roper, Matthew D <matthew.d.roper@intel.com>
> > > Cc: Intel XE List <intel-xe@lists.freedesktop.org>; Upadhyay,
> > > Tejas
> > > <tejas.upadhyay@intel.com>; Ghimiray, Himal Prasad
> > > <himal.prasad.ghimiray@intel.com>; Bhatia, Aradhya
> > > <aradhya.bhatia@intel.com>
> > > Subject: [PATCH] drm/xe/guc: Make creation of SLPC debugfs files
> > > conditional
> > > 
> > > Platforms that do not support SLPC are exempted from the GuC PC
> > > support.
> > > The GuC PC does not get initialized, and neither do its BOs get
> > > created.
> > > 
> > > This causes a problem because the GuC PC debugfs file is still
> > > being created.
> > > Whenever the file is attempted to read, it causes a NULL pointer
> > > dereference
> > > on the supposed BO of the GuC PC.
> > > 
> > > So, make the creation of SLPC debugfs files conditional to when
> > > SLPC features
> > > are supported.
> > > 
> > > Suggested-by: Matt Roper <matthew.d.roper@intel.com>
> > > Signed-off-by: Aradhya Bhatia <aradhya.bhatia@intel.com>
> > > ---
> > >  drivers/gpu/drm/xe/xe_guc_debugfs.c | 17 ++++++++++++++---
> > >  1 file changed, 14 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/xe/xe_guc_debugfs.c
> > > b/drivers/gpu/drm/xe/xe_guc_debugfs.c
> > > index f33013f8a0f3..0b102ab46c4d 100644
> > > --- a/drivers/gpu/drm/xe/xe_guc_debugfs.c
> > > +++ b/drivers/gpu/drm/xe/xe_guc_debugfs.c
> > > @@ -113,23 +113,34 @@ static const struct drm_info_list
> > > vf_safe_debugfs_list[] = {
> > >         { "guc_ctb", .show = guc_debugfs_show, .data = guc_ctb
> > > },  };
> > > 
> > > +/* For GuC debugfs files that require the SLPC support */ static
> > > const
> > > +struct drm_info_list slpc_debugfs_list[] = {
> > > +       { "guc_pc", .show = guc_debugfs_show, .data = guc_pc },
> > > };
> > > +
> > >  /* everything else should be added here */  static const struct
> > > drm_info_list
> > > pf_only_debugfs_list[] = {
> > >         { "guc_log", .show = guc_debugfs_show, .data = guc_log },
> > >         { "guc_log_dmesg", .show = guc_debugfs_show, .data =
> > > guc_log_dmesg },
> > > -       { "guc_pc", .show = guc_debugfs_show, .data = guc_pc },
> > 
> > You need fixes tag here it seems. With that,
> 
> About this, there is a slight complication.
> 
> The original patch that introduced this issue is this commit,
> 
> aaab5404b16f ("drm/xe: Introduce GuC PC debugfs") [0]
> 
> This is the patch that introduced GuC PC debugfs file but didn't
> account
> for the case when "skip_guc_pc" is set.
> 
> 
> But, the guc debugfs has had other commits _after_ the above patch,
> which refactor in various ways how guc debugfs is being handled in Xe
> driver.
> 
> 
> e15826bb3c2c ("drm/xe/guc: Refactor GuC debugfs initialization") [1]
> 387444984d7b ("drm/xe/guc: Don't expose GuC privileged debugfs files
> if
> VF") [2]
> 
> Since my patch is based after patches [1], and [2] (which are not
> "fixes", and hence were not backported to any previous kernel
> version),
> my patch will not be "backport-able" to the original patch that needs
> the fix [0].
> 
> 
> So, what should be done in this situation?

I'd just add the fixes to the original patch. The refactors will have
to take that into consideration if these get merged down the road.

Reviewed-by: Stuart Summers <stuart.summers@intel.com>

Thanks,
Stuart

> 
> 
> Regards
> Aradhya
> 
> 
> [0]:
> https://lore.kernel.org/all/20250114232443.1135355-1-rodrigo.vivi@intel.com/
> 
> [1]:
> https://lore.kernel.org/all/20250403142635.1821-2-michal.wajdeczko@intel.com/
> 
> [2]:
> https://lore.kernel.org/all/20250403142635.1821-3-michal.wajdeczko@intel.com/
> 
> 
> 
> > Reviewed-by: Tejas Upadhyay <tejas.upadhyay@intel.com>
> > 
> > Tejas
> > >  };
> > > 
> > >  void xe_guc_debugfs_register(struct xe_guc *guc, struct dentry
> > > *parent)  {
> > > -       struct drm_minor *minor = guc_to_xe(guc)->drm.primary;
> > > +       struct xe_device *xe =  guc_to_xe(guc);
> > > +       struct drm_minor *minor = xe->drm.primary;
> > > 
> > >         drm_debugfs_create_files(vf_safe_debugfs_list,
> > >                                 
> > > ARRAY_SIZE(vf_safe_debugfs_list),
> > >                                  parent, minor);
> > > 
> > > -       if (!IS_SRIOV_VF(guc_to_xe(guc)))
> > > +       if (!IS_SRIOV_VF(xe)) {
> > >                 drm_debugfs_create_files(pf_only_debugfs_list,
> > >                                         
> > > ARRAY_SIZE(pf_only_debugfs_list),
> > >                                          parent, minor);
> > > +
> > > +               if (!xe->info.skip_guc_pc)
> > > +                       drm_debugfs_create_files(slpc_debugfs_lis
> > > t,
> > > +
> > > ARRAY_SIZE(slpc_debugfs_list),
> > > +                                                parent, minor);
> > > +       }
> > >  }
> > > 
> > > base-commit: 3d6670fab64cb00b5e6ed80d2517147db533faf1
> > > --
> > > 2.43.0
> > 
>