| Message ID | 20250411130428.4104957-1-sunil.khatri@amd.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [v1,1/3] drm: function to get process name and pid | expand |
Ping? On 4/11/2025 6:34 PM, Sunil Khatri wrote: > 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"); > + 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); > + > + 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); >
Adding Pierre-eric and Tvrtko as well. 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"); > + 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); >
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. 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. 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). 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); >> >
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); >> >
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"); + 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); + + 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);
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(+)