Re: [PATCH v1 1/3] drm: function to get process name and pid

["Khatri, Sunil" <sukhatri@amd.com>]

On 4/15/2025 2:14 PM, Tvrtko Ursulin wrote:
>
> On 14/04/2025 18:58, Christian König wrote:
>> Adding Pierre-eric and Tvrtko as well.
>
> Thanks!
>
>> Am 11.04.25 um 15:04 schrieb Sunil Khatri:
>>> Add helper function which get the process information for
>>> the drm_file and updates the user provided character buffer
>>> with the information of process name and pid as a string.
>>>
>>> Signed-off-by: Sunil Khatri <sunil.khatri@amd.com>
>>> ---
>>>   drivers/gpu/drm/drm_file.c | 30 ++++++++++++++++++++++++++++++
>>>   include/drm/drm_file.h     |  1 +
>>>   2 files changed, 31 insertions(+)
>>>
>>> diff --git a/drivers/gpu/drm/drm_file.c b/drivers/gpu/drm/drm_file.c
>>> index cb5f22f5bbb6..4434258d21b5 100644
>>> --- a/drivers/gpu/drm/drm_file.c
>>> +++ b/drivers/gpu/drm/drm_file.c
>>> @@ -965,6 +965,36 @@ void drm_show_fdinfo(struct seq_file *m, struct 
>>> file *f)
>>>   }
>>>   EXPORT_SYMBOL(drm_show_fdinfo);
>>>   +/**
>>> + * drm_process_info - Fill info string with process name and pid
>>> + * @file_priv: context of interest for process name and pid
>>> + * @proc_info: user char ptr to write the string to
>>> + * @buff_size: size of the buffer passed for the string
>>> + *
>>> + * This update the user provided buffer with process
>>> + * name and pid information for @file_priv
>>> + */
>>> +void drm_process_info(struct drm_file *file_priv, char *proc_info, 
>>> size_t buff_size)
>>> +{
>>> +    struct task_struct *task;
>>> +    struct pid *pid;
>>> +    struct drm_device *dev = file_priv->minor->dev;
>>> +
>>> +    if (!proc_info) {
>>> +        drm_err(dev, "Invalid user buffer\n");
>
> I'd replace this with drm_WARN_ON_ONCE.
> This sounds fine, will update with warn once in next version.
>
> Another thing I would consider is avoiding the need for stack space by 
> exporting a logging helper instead. Something like (from patch 3/3):
>
> drm_file_err(uq_mgr->file, "Timed out waiting for fence %p\n", f);
>
> Which would output the client name info as a prefix or something.
I guess here we are making a generic function and nothing specific to 
the driver or a feature like uq_manager. this is supposed to be a common 
helper function for all drm clients
and based on drm_file. With respect to the user i guess the 
driver/feature specific information can we placed in the caller itself 
and thats upto the user.
>
> Especially attractive if you add client name.
>
> Also while here, is %p for the fence is useful? FWIW in the tracing 
> series we are going for %llu:%llu (context:seqno).
> To be frank i dont see the fence ptr to be very useful as they are 
> being reused too and we do see same fence ptr again too. but this we 
> could improve later. I will push a new patch series for this with the 
> comments taken into.
Regards
Sunil Khatri
>
> Regards,
>
> Tvrtko
>
>>> +        return;
>>> +    }
>>> +
>>> +    rcu_read_lock();
>>> +    pid = rcu_dereference(file_priv->pid);
>>> +    task = pid_task(pid, PIDTYPE_TGID);
>>> +    if (task)
>>> +        snprintf(proc_info, buff_size, "comm:%s pid:%d", 
>>> task->comm, task->pid);
>>
>> Looks good in general, but I think people would like to see the 
>> optional client name here as well.
>>
>> It's rather useful to have for native context.
>>
>> Regards,
>> Christian.
>>
>>> +
>>> +    rcu_read_unlock();
>>> +}
>>> +EXPORT_SYMBOL(drm_process_info);
>>> +
>>>   /**
>>>    * mock_drm_getfile - Create a new struct file for the drm device
>>>    * @minor: drm minor to wrap (e.g. #drm_device.primary)
>>> diff --git a/include/drm/drm_file.h b/include/drm/drm_file.h
>>> index f0ef32e9fa5e..c01b34936968 100644
>>> --- a/include/drm/drm_file.h
>>> +++ b/include/drm/drm_file.h
>>> @@ -501,6 +501,7 @@ void drm_print_memory_stats(struct drm_printer *p,
>>>     void drm_show_memory_stats(struct drm_printer *p, struct 
>>> drm_file *file);
>>>   void drm_show_fdinfo(struct seq_file *m, struct file *f);
>>> +void drm_process_info(struct drm_file *file_priv, char *proc_info, 
>>> size_t buff_size);
>>>     struct file *mock_drm_getfile(struct drm_minor *minor, unsigned 
>>> int flags);
>>
>