Re: [PATCH 1/3] drm/xe: Store pointer to struct xe_gt in gt/ debugfs directory

[Michal Wajdeczko <michal.wajdeczko@intel.com>]

On 25.03.2024 18:01, Rodrigo Vivi wrote:
> On Wed, Feb 14, 2024 at 12:57:54PM +0100, Michal Wajdeczko wrote:
>> Attributes added under 'gt/' directories may wish to use that
>> in case they can't obtain it from elsewhere.
>>
>> Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
>> ---
>>  drivers/gpu/drm/xe/xe_gt_debugfs.c | 3 +++
>>  1 file changed, 3 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/xe/xe_gt_debugfs.c b/drivers/gpu/drm/xe/xe_gt_debugfs.c
>> index c4b67cf09f8f..207b992f1240 100644
>> --- a/drivers/gpu/drm/xe/xe_gt_debugfs.c
>> +++ b/drivers/gpu/drm/xe/xe_gt_debugfs.c
>> @@ -225,6 +225,9 @@ void xe_gt_debugfs_register(struct xe_gt *gt)
>>  		return;
>>  	}
>>  
>> +	/* other attributes may use parent->d_inode->i_private */
> 
> what did you mean with this comment?
> if others are using, what would be the risks?
> is this a good thing? is this a bad thing?

maybe better wording should be:

/*
 * Store the xe_gt pointer as private data of the gt/ directory node
 * so other GT specific attributes under that directory may refer to
 * it by looking at its parent node private data.
 */

> 
>> +	root->d_inode->i_private = gt;
> 
> At first I thought this was intrusive, but then the following
> patches made me realize that we are already being intrusive
> when disrespecting the data:
> 
> include/drm/drm_debugfs.h
> struct drm_debugfs_info
> /** @data: Driver-private data, should not be device-specific. */
> 
> 
> So it looks that we do need something else.
> 
> Looking the i_private that you pointed out seems an alternative
> 
> include/linux/fs.h
> struct inode {
> void                    *i_private; /* fs or device private pointer */
> 
> it is a 'device' pointer rather then a 'driver', but I'm still confident
> that it is the right one to use.

GT aka xe_gt is more a device than a driver, no ?

> 
> It looks like the debugfs_create_file functions would override that
> anyway with the data. Also other places in the fs code where this is
> used for other checks.

the drm_debugfs will use i_private only on nodes that represent
individual attributes, it will not touch the parent node i_private
(which is our gt/ directory - and this where we set pointer to xe_gt)

> 
> So, perhaps we need something more flexible at the drm_debugfs layer
> that would allow us to have a sub-device pointer?
> 
> The other 2 patches are great and you can already use rv-b on them if
> we agree that this i_private change here is good.
> 
> Cc: Lucas De Marchi <lucas.demarchi@intel.com>
> Cc: Oded Gabbay <ogabbay@kernel.org>
> Cc: Thomas Hellström <thomas.hellstrom@linux.intel.com>
> 
>> +
>>  	/*
>>  	 * Allocate local copy as we need to pass in the GT to the debugfs
>>  	 * entry and drm_debugfs_create_files just references the drm_info_list
>> -- 
>> 2.43.0
>>