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

[Aradhya Bhatia]

Hi Stuart,

Thank you for reviewing the patch.

On 16-05-2025 00:07, Summers, Stuart wrote:
> 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 at intel.com>
>>>> Sent: 15 May 2025 15:19
>>>> To: Roper, Matthew D <matthew.d.roper at intel.com>
>>>> Cc: Intel XE List <intel-xe at lists.freedesktop.org>; Upadhyay,
>>>> Tejas
>>>> <tejas.upadhyay at intel.com>; Ghimiray, Himal Prasad
>>>> <himal.prasad.ghimiray at intel.com>; Bhatia, Aradhya
>>>> <aradhya.bhatia at 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 at intel.com>
>>>> Signed-off-by: Aradhya Bhatia <aradhya.bhatia at 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.

Please help me understand, what do you meany by "if these get merged
down the road".

drm-xe-next(-fixes) (the branch this patch is based on top of) has both
those refactoring patches merged.

However, the v6.15-rc6 (and drm-xe-fixes) do not.

Please correct me if I am wrong, but the only way I understand that the
fix patch can be merged _before_ the refactoring patches, is to go
through drm-xe-fixes.

Is that what you meant?

> 
> Reviewed-by: Stuart Summers <stuart.summers at 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 at 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
>>>
>>
>