| Message ID | 20250415184318.2465197-1-sunil.khatri@amd.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [v3,1/4] drm: add function drm_file_err to print proc information too | expand |
On 15/04/2025 19:43, Sunil Khatri wrote: > Add a drm helper function which get the process information for > the drm_file and append the process information using the existing > drm_err. > > Signed-off-by: Sunil Khatri <sunil.khatri@amd.com> > --- > include/drm/drm_file.h | 40 ++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 40 insertions(+) > > diff --git a/include/drm/drm_file.h b/include/drm/drm_file.h > index 94d365b22505..e329299a2b2c 100644 > --- a/include/drm/drm_file.h > +++ b/include/drm/drm_file.h > @@ -37,6 +37,7 @@ > #include <uapi/drm/drm.h> > > #include <drm/drm_prime.h> > +#include <drm/drm_print.h> > > struct dma_fence; > struct drm_file; > @@ -446,6 +447,45 @@ static inline bool drm_is_accel_client(const struct drm_file *file_priv) > return file_priv->minor->type == DRM_MINOR_ACCEL; > } > > +/** > + * drm_file_err - Fill info string with process name and pid > + * @file_priv: context of interest for process name and pid > + * @fmt: prinf() like format string > + * > + * This update the user provided buffer with process > + * name and pid information for @file_priv > + */ > +__printf(2, 3) > +static inline void drm_file_err(struct drm_file *file_priv, const char *fmt, ...) > +{ > + struct task_struct *task; > + struct pid *pid; > + struct drm_device *dev = file_priv->minor->dev; > + char new_fmt[256]; > + char final_fmt[512]; > + va_list args; > + > + mutex_lock(&file_priv->client_name_lock); > + rcu_read_lock(); > + pid = rcu_dereference(file_priv->pid); > + task = pid_task(pid, PIDTYPE_TGID); > + > + if (drm_WARN_ON_ONCE(dev, !task)) > + return; > + > + snprintf(new_fmt, sizeof(new_fmt), "proc:%s pid:%d client_name:%s %s", > + task->comm, task->pid, file_priv->client_name ?: "Unset", fmt); > + > + va_start(args, fmt); > + vsnprintf(final_fmt, sizeof(final_fmt), new_fmt, args); > + > + drm_err(dev, "%s", final_fmt); > + va_end(args); > + > + rcu_read_unlock(); > + mutex_unlock(&file_priv->client_name_lock); > +} > + I was hoping something primitive could be enough. With no temporary stack space required. Primitive on the level of (but simplified for illustration purpose): #define some_err(_file, _fmt, ...) \ drm_err(dev, "client-%s: " _fmt, (_this)->client_name, ##__VA_ARGS__) Am I missing something or that would work? Regards, Tvrtko > void drm_file_update_pid(struct drm_file *); > > struct drm_minor *drm_minor_acquire(struct xarray *minors_xa, unsigned int minor_id);
On 4/16/2025 12:37 PM, Tvrtko Ursulin wrote: > > On 15/04/2025 19:43, Sunil Khatri wrote: >> Add a drm helper function which get the process information for >> the drm_file and append the process information using the existing >> drm_err. >> >> Signed-off-by: Sunil Khatri <sunil.khatri@amd.com> >> --- >> include/drm/drm_file.h | 40 ++++++++++++++++++++++++++++++++++++++++ >> 1 file changed, 40 insertions(+) >> >> diff --git a/include/drm/drm_file.h b/include/drm/drm_file.h >> index 94d365b22505..e329299a2b2c 100644 >> --- a/include/drm/drm_file.h >> +++ b/include/drm/drm_file.h >> @@ -37,6 +37,7 @@ >> #include <uapi/drm/drm.h> >> #include <drm/drm_prime.h> >> +#include <drm/drm_print.h> >> struct dma_fence; >> struct drm_file; >> @@ -446,6 +447,45 @@ static inline bool drm_is_accel_client(const >> struct drm_file *file_priv) >> return file_priv->minor->type == DRM_MINOR_ACCEL; >> } >> +/** >> + * drm_file_err - Fill info string with process name and pid >> + * @file_priv: context of interest for process name and pid >> + * @fmt: prinf() like format string >> + * >> + * This update the user provided buffer with process >> + * name and pid information for @file_priv >> + */ >> +__printf(2, 3) >> +static inline void drm_file_err(struct drm_file *file_priv, const >> char *fmt, ...) >> +{ >> + struct task_struct *task; >> + struct pid *pid; >> + struct drm_device *dev = file_priv->minor->dev; >> + char new_fmt[256]; >> + char final_fmt[512]; >> + va_list args; >> + >> + mutex_lock(&file_priv->client_name_lock); >> + rcu_read_lock(); >> + pid = rcu_dereference(file_priv->pid); >> + task = pid_task(pid, PIDTYPE_TGID); >> + >> + if (drm_WARN_ON_ONCE(dev, !task)) >> + return; >> + >> + snprintf(new_fmt, sizeof(new_fmt), "proc:%s pid:%d >> client_name:%s %s", >> + task->comm, task->pid, file_priv->client_name ?: "Unset", fmt); >> + >> + va_start(args, fmt); >> + vsnprintf(final_fmt, sizeof(final_fmt), new_fmt, args); >> + >> + drm_err(dev, "%s", final_fmt); >> + va_end(args); >> + >> + rcu_read_unlock(); >> + mutex_unlock(&file_priv->client_name_lock); >> +} >> + > > I was hoping something primitive could be enough. With no temporary > stack space required. Primitive on the level of (but simplified for > illustration purpose): > > #define some_err(_file, _fmt, ...) \ > drm_err(dev, "client-%s: " _fmt, (_this)->client_name, ##__VA_ARGS__) I also thought of doing it similarly but that dint work. There was lot of code to get the process name and pid along with client_name too. So ##__VA_ARGS__ dont work as soon as its a function and not macro. Also drm_err gave me errors and this is the way i find it not complaining. new_fmt is a string directly anymore and hence need to %s to pass but then the drm_err complain too many args for args to pass. So i have to combine new_fmt and args in one to get final_fmt and atleast functionally it worked. Yesterday even i though that i would be as simple as adding a macro. > > Am I missing something or that would work? > > Regards, > > Tvrtko > >> void drm_file_update_pid(struct drm_file *); >> struct drm_minor *drm_minor_acquire(struct xarray *minors_xa, >> unsigned int minor_id); >
Am 16.04.25 um 10:39 schrieb Khatri, Sunil: > > On 4/16/2025 12:37 PM, Tvrtko Ursulin wrote: >> >> On 15/04/2025 19:43, Sunil Khatri wrote: >>> [SNIP] >>> + >> >> I was hoping something primitive could be enough. With no temporary stack space required. Primitive on the level of (but simplified for illustration purpose): >> >> #define some_err(_file, _fmt, ...) \ >> drm_err(dev, "client-%s: " _fmt, (_this)->client_name, ##__VA_ARGS__) > I also thought of doing it similarly but that dint work. There was lot of code to get the process name and pid along with client_name too. So ##__VA_ARGS__ dont work as soon as its a function and not macro. > Also drm_err gave me errors and this is the way i find it not complaining. new_fmt is a string directly anymore and hence need to %s to pass but then the drm_err complain too many args for args to pass. So i have to combine new_fmt and args in one to get final_fmt and atleast functionally it worked. > > Yesterday even i though that i would be as simple as adding a macro. It's a bit tricky, but I think that is doable. You need something like this here: #define drm_file_err(file, fmt, ...) do { struct task_struct *task = drm_file_lock_pid(file); drm_err(file->dev, "task: %s pid: %d client: %s" fmt, task, file->pid, ##__VA_ARGS_); drm_file_unlock_pid(file); } while (0); You then just need to implement drm_file_lock_pid (maybe come up with a better name) to grab the mutex and take the RCU read lock. Christian. >> >> Am I missing something or that would work? >> >> Regards, >> >> Tvrtko >> >>> void drm_file_update_pid(struct drm_file *); >>> struct drm_minor *drm_minor_acquire(struct xarray *minors_xa, unsigned int minor_id); >>
diff --git a/include/drm/drm_file.h b/include/drm/drm_file.h index 94d365b22505..e329299a2b2c 100644 --- a/include/drm/drm_file.h +++ b/include/drm/drm_file.h @@ -37,6 +37,7 @@ #include <uapi/drm/drm.h> #include <drm/drm_prime.h> +#include <drm/drm_print.h> struct dma_fence; struct drm_file; @@ -446,6 +447,45 @@ static inline bool drm_is_accel_client(const struct drm_file *file_priv) return file_priv->minor->type == DRM_MINOR_ACCEL; } +/** + * drm_file_err - Fill info string with process name and pid + * @file_priv: context of interest for process name and pid + * @fmt: prinf() like format string + * + * This update the user provided buffer with process + * name and pid information for @file_priv + */ +__printf(2, 3) +static inline void drm_file_err(struct drm_file *file_priv, const char *fmt, ...) +{ + struct task_struct *task; + struct pid *pid; + struct drm_device *dev = file_priv->minor->dev; + char new_fmt[256]; + char final_fmt[512]; + va_list args; + + mutex_lock(&file_priv->client_name_lock); + rcu_read_lock(); + pid = rcu_dereference(file_priv->pid); + task = pid_task(pid, PIDTYPE_TGID); + + if (drm_WARN_ON_ONCE(dev, !task)) + return; + + snprintf(new_fmt, sizeof(new_fmt), "proc:%s pid:%d client_name:%s %s", + task->comm, task->pid, file_priv->client_name ?: "Unset", fmt); + + va_start(args, fmt); + vsnprintf(final_fmt, sizeof(final_fmt), new_fmt, args); + + drm_err(dev, "%s", final_fmt); + va_end(args); + + rcu_read_unlock(); + mutex_unlock(&file_priv->client_name_lock); +} + void drm_file_update_pid(struct drm_file *); struct drm_minor *drm_minor_acquire(struct xarray *minors_xa, unsigned int minor_id);
Add a drm helper function which get the process information for the drm_file and append the process information using the existing drm_err. Signed-off-by: Sunil Khatri <sunil.khatri@amd.com> --- include/drm/drm_file.h | 40 ++++++++++++++++++++++++++++++++++++++++ 1 file changed, 40 insertions(+)