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

[Aradhya Bhatia <aradhya.bhatia@intel.com>]

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?


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_list,
>> +
>> ARRAY_SIZE(slpc_debugfs_list),
>> +						 parent, minor);
>> +	}
>>  }
>>
>> base-commit: 3d6670fab64cb00b5e6ed80d2517147db533faf1
>> --
>> 2.43.0
>