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

[Lucas De Marchi <lucas.demarchi@intel.com>]

On Mon, Mar 25, 2024 at 06:34:01PM +0100, Michal Wajdeczko wrote:
>
>
>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)

I completely agree with this. It seems nice to be able to easily
retrieve xe_gt. I´d just double check the lifecycle if we may not end up
freeing something that's being used during unbind.

I can't do a thorough review of this and the other patches right now,
but ack on the approach in this patch.


Acked-by: Lucas De Marchi <lucas.demarchi@intel.com>


Lucas De Marchi