diff mbox series

[V6,1/5] drm: add drm_file_err function to add process info

Message ID 20250417123155.4002358-1-sunil.khatri@amd.com (mailing list archive)
State New
Headers show
Series [V6,1/5] drm: add drm_file_err function to add process info | expand

Commit Message

Sunil Khatri April 17, 2025, 12:31 p.m. UTC
Add a drm helper function which append the process information for
the drm_file over drm_err formated output.

v5: change to macro from function (Christian Koenig)
    add helper functions for lock/unlock (Christian Koenig)

v6: remove __maybe_unused and make function inline (Jani Nikula)
    remove drm_print.h

v7: Use va_format and %pV to concatenate fmt and vargs (Jani Nikula)

Signed-off-by: Sunil Khatri <sunil.khatri@amd.com>
---
 drivers/gpu/drm/drm_file.c | 34 ++++++++++++++++++++++++++++++++++
 include/drm/drm_file.h     |  3 +++
 2 files changed, 37 insertions(+)

Comments

Tvrtko Ursulin April 17, 2025, 1:34 p.m. UTC | #1
On 17/04/2025 13:31, Sunil Khatri wrote:
> Add a drm helper function which append the process information for

appends

> the drm_file over drm_err formated output.

formatted

> v5: change to macro from function (Christian Koenig)
>      add helper functions for lock/unlock (Christian Koenig)
> 
> v6: remove __maybe_unused and make function inline (Jani Nikula)
>      remove drm_print.h
> 
> v7: Use va_format and %pV to concatenate fmt and vargs (Jani Nikula)
> 
> Signed-off-by: Sunil Khatri <sunil.khatri@amd.com>
> ---
>   drivers/gpu/drm/drm_file.c | 34 ++++++++++++++++++++++++++++++++++
>   include/drm/drm_file.h     |  3 +++
>   2 files changed, 37 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_file.c b/drivers/gpu/drm/drm_file.c
> index c299cd94d3f7..7e64d84d4e2d 100644
> --- a/drivers/gpu/drm/drm_file.c
> +++ b/drivers/gpu/drm/drm_file.c
> @@ -986,6 +986,40 @@ void drm_show_fdinfo(struct seq_file *m, struct file *f)
>   }
>   EXPORT_SYMBOL(drm_show_fdinfo);
>   
> +/**
> + * drm_file_err - Fill info string with process name and pid

Update to something like:

drm_file_err - Log error message associated with a drm_file client

> + * @file_priv: context of interest for process name and pid
> + * @fmt: prinf() like format string

printf

> + *
> + * This update the user provided buffer with process
> + * name and pid information for @file_priv

Also stale but it may be okay to drop altogether since short description 
feels enough for a logging helper.

> + */
> +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;
> +	va_list args;
> +	struct va_format vaf;

You could tidy the declaration block a bit, usually ordering from longer 
to narrower lines for readability but up to you.

> +
> +	va_start(args, fmt);
> +	vaf.fmt = fmt;
> +	vaf.va = &args;
> +
> +	mutex_lock(&file_priv->client_name_lock);
> +	rcu_read_lock();
> +	pid = rcu_dereference(file_priv->pid);
> +	task = pid_task(pid, PIDTYPE_TGID);
> +
> +	drm_err(dev, "comm: %s pid: %d client: %s %pV", task ? task->comm : "",
> +		task ? task->pid : 0, file_priv->client_name ?: "Unset", &vaf);
> +

Hm is task->pid the tid or tgid? Could be the same thing due getting 
PIDTYPE_GID.. PID namespaces are always endlessly confusing to me since 
I don't work in that area often enough.

And I have some bikeshedding ideas for the format, like maybe 
consolidating with naming used in drm_clients_info() and adding some 
separator between the client data and the log message that follows. Like:

"Client %s[%d] (%s): %pV",
task ? task->comm : "<unknown>",
pid_nr(pid),
file_priv->client_name ?: "<unset>",
...

And even if I am a bit unsure about the "<unknown>", I think it doesn't 
harm to be consistent. It is fine as is though, so you decide if you 
want to change anything or not.

If you could please double check task->pid is indeed definitely TGID:

Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>

Regards,

Tvrtko

> +	va_end(args);
> +	rcu_read_unlock();
> +	mutex_unlock(&file_priv->client_name_lock);
> +}
> +EXPORT_SYMBOL(drm_file_err);
> +
>   /**
>    * 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 94d365b22505..5c3b2aa3e69d 100644
> --- a/include/drm/drm_file.h
> +++ b/include/drm/drm_file.h
> @@ -446,6 +446,9 @@ static inline bool drm_is_accel_client(const struct drm_file *file_priv)
>   	return file_priv->minor->type == DRM_MINOR_ACCEL;
>   }
>   
> +__printf(2, 3)
> +void drm_file_err(struct drm_file *file_priv, const char *fmt, ...);
> +
>   void drm_file_update_pid(struct drm_file *);
>   
>   struct drm_minor *drm_minor_acquire(struct xarray *minors_xa, unsigned int minor_id);
Khatri, Sunil April 17, 2025, 2:23 p.m. UTC | #2
On 4/17/2025 7:04 PM, Tvrtko Ursulin wrote:
>
> On 17/04/2025 13:31, Sunil Khatri wrote:
>> Add a drm helper function which append the process information for
>
> appends
Noted
>
>> the drm_file over drm_err formated output.
>
> formatted
'Noted
>
>> v5: change to macro from function (Christian Koenig)
>>      add helper functions for lock/unlock (Christian Koenig)
>>
>> v6: remove __maybe_unused and make function inline (Jani Nikula)
>>      remove drm_print.h
>>
>> v7: Use va_format and %pV to concatenate fmt and vargs (Jani Nikula)
>>
>> Signed-off-by: Sunil Khatri <sunil.khatri@amd.com>
>> ---
>>   drivers/gpu/drm/drm_file.c | 34 ++++++++++++++++++++++++++++++++++
>>   include/drm/drm_file.h     |  3 +++
>>   2 files changed, 37 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/drm_file.c b/drivers/gpu/drm/drm_file.c
>> index c299cd94d3f7..7e64d84d4e2d 100644
>> --- a/drivers/gpu/drm/drm_file.c
>> +++ b/drivers/gpu/drm/drm_file.c
>> @@ -986,6 +986,40 @@ void drm_show_fdinfo(struct seq_file *m, struct 
>> file *f)
>>   }
>>   EXPORT_SYMBOL(drm_show_fdinfo);
>>   +/**
>> + * drm_file_err - Fill info string with process name and pid
>
> Update to something like:
>
> drm_file_err - Log error message associated with a drm_file client
>
>> + * @file_priv: context of interest for process name and pid
>> + * @fmt: prinf() like format string
>
> printf
>
>> + *
>> + * This update the user provided buffer with process
>> + * name and pid information for @file_priv
>
> Also stale but it may be okay to drop altogether since short 
> description feels enough for a logging helper.
>
>> + */
>> +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;
>> +    va_list args;
>> +    struct va_format vaf;
>
> You could tidy the declaration block a bit, usually ordering from 
> longer to narrower lines for readability but up to you.
Sure
>> +
>> +    va_start(args, fmt);
>> +    vaf.fmt = fmt;
>> +    vaf.va = &args;
>> +
>> +    mutex_lock(&file_priv->client_name_lock);
>> +    rcu_read_lock();
>> +    pid = rcu_dereference(file_priv->pid);
>> +    task = pid_task(pid, PIDTYPE_TGID);
>> +
>> +    drm_err(dev, "comm: %s pid: %d client: %s %pV", task ? 
>> task->comm : "",
>> +        task ? task->pid : 0, file_priv->client_name ?: "Unset", &vaf);
>> +
>
> Hm is task->pid the tid or tgid? Could be the same thing due getting 
> PIDTYPE_GID.. PID namespaces are always endlessly confusing to me 
> since I don't work in that area often enough.
>
There are two parameters in task_struct one is pid and tgid and both are 
different if i am not wrong. In case of multi thread pid and tgid is 
different if its one thread i guess in that case pid and tgid are same 
but even i am not 100% sure.
> And I have some bikeshedding ideas for the format, like maybe 
> consolidating with naming used in drm_clients_info() and adding some 
> separator between the client data and the log message that follows. Like:
>
> "Client %s[%d] (%s): %pV",
> task ? task->comm : "<unknown>",
> pid_nr(pid),
> file_priv->client_name ?: "<unset>",
> ...
Sure
>
> And even if I am a bit unsure about the "<unknown>", I think it 
> doesn't harm to be consistent. It is fine as is though, so you decide 
> if you want to change anything or not.
>
> If you could please double check task->pid is indeed definitely TGID:
>
> Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>
>
> Regards,
>
> Tvrtko
>
>> +    va_end(args);
>> +    rcu_read_unlock();
>> +    mutex_unlock(&file_priv->client_name_lock);
>> +}
>> +EXPORT_SYMBOL(drm_file_err);
>> +
>>   /**
>>    * 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 94d365b22505..5c3b2aa3e69d 100644
>> --- a/include/drm/drm_file.h
>> +++ b/include/drm/drm_file.h
>> @@ -446,6 +446,9 @@ static inline bool drm_is_accel_client(const 
>> struct drm_file *file_priv)
>>       return file_priv->minor->type == DRM_MINOR_ACCEL;
>>   }
>>   +__printf(2, 3)
>> +void drm_file_err(struct drm_file *file_priv, const char *fmt, ...);
>> +
>>   void drm_file_update_pid(struct drm_file *);
>>     struct drm_minor *drm_minor_acquire(struct xarray *minors_xa, 
>> unsigned int minor_id);
>
Khatri, Sunil April 17, 2025, 2:53 p.m. UTC | #3
For rest of the patches which are part of the amdgpu tree will push 
incorporating changes as shared by @Tvrtko Ursulin 
<tvrtko.ursulin@igalia.com> once drm change is merged.

Thanks a lot all for the reviews.

Regards

Sunil Khatri

On 4/17/2025 6:01 PM, Sunil Khatri wrote:
> Add a drm helper function which append the process information for
> the drm_file over drm_err formated output.
>
> v5: change to macro from function (Christian Koenig)
>      add helper functions for lock/unlock (Christian Koenig)
>
> v6: remove __maybe_unused and make function inline (Jani Nikula)
>      remove drm_print.h
>
> v7: Use va_format and %pV to concatenate fmt and vargs (Jani Nikula)
>
> Signed-off-by: Sunil Khatri <sunil.khatri@amd.com>
> ---
>   drivers/gpu/drm/drm_file.c | 34 ++++++++++++++++++++++++++++++++++
>   include/drm/drm_file.h     |  3 +++
>   2 files changed, 37 insertions(+)
>
> diff --git a/drivers/gpu/drm/drm_file.c b/drivers/gpu/drm/drm_file.c
> index c299cd94d3f7..7e64d84d4e2d 100644
> --- a/drivers/gpu/drm/drm_file.c
> +++ b/drivers/gpu/drm/drm_file.c
> @@ -986,6 +986,40 @@ void drm_show_fdinfo(struct seq_file *m, struct file *f)
>   }
>   EXPORT_SYMBOL(drm_show_fdinfo);
>   
> +/**
> + * 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
> + */
> +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;
> +	va_list args;
> +	struct va_format vaf;
> +
> +	va_start(args, fmt);
> +	vaf.fmt = fmt;
> +	vaf.va = &args;
> +
> +	mutex_lock(&file_priv->client_name_lock);
> +	rcu_read_lock();
> +	pid = rcu_dereference(file_priv->pid);
> +	task = pid_task(pid, PIDTYPE_TGID);
> +
> +	drm_err(dev, "comm: %s pid: %d client: %s %pV", task ? task->comm : "",
> +		task ? task->pid : 0, file_priv->client_name ?: "Unset", &vaf);
> +
> +	va_end(args);
> +	rcu_read_unlock();
> +	mutex_unlock(&file_priv->client_name_lock);
> +}
> +EXPORT_SYMBOL(drm_file_err);
> +
>   /**
>    * 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 94d365b22505..5c3b2aa3e69d 100644
> --- a/include/drm/drm_file.h
> +++ b/include/drm/drm_file.h
> @@ -446,6 +446,9 @@ static inline bool drm_is_accel_client(const struct drm_file *file_priv)
>   	return file_priv->minor->type == DRM_MINOR_ACCEL;
>   }
>   
> +__printf(2, 3)
> +void drm_file_err(struct drm_file *file_priv, const char *fmt, ...);
> +
>   void drm_file_update_pid(struct drm_file *);
>   
>   struct drm_minor *drm_minor_acquire(struct xarray *minors_xa, unsigned int minor_id);
diff mbox series

Patch

diff --git a/drivers/gpu/drm/drm_file.c b/drivers/gpu/drm/drm_file.c
index c299cd94d3f7..7e64d84d4e2d 100644
--- a/drivers/gpu/drm/drm_file.c
+++ b/drivers/gpu/drm/drm_file.c
@@ -986,6 +986,40 @@  void drm_show_fdinfo(struct seq_file *m, struct file *f)
 }
 EXPORT_SYMBOL(drm_show_fdinfo);
 
+/**
+ * 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
+ */
+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;
+	va_list args;
+	struct va_format vaf;
+
+	va_start(args, fmt);
+	vaf.fmt = fmt;
+	vaf.va = &args;
+
+	mutex_lock(&file_priv->client_name_lock);
+	rcu_read_lock();
+	pid = rcu_dereference(file_priv->pid);
+	task = pid_task(pid, PIDTYPE_TGID);
+
+	drm_err(dev, "comm: %s pid: %d client: %s %pV", task ? task->comm : "",
+		task ? task->pid : 0, file_priv->client_name ?: "Unset", &vaf);
+
+	va_end(args);
+	rcu_read_unlock();
+	mutex_unlock(&file_priv->client_name_lock);
+}
+EXPORT_SYMBOL(drm_file_err);
+
 /**
  * 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 94d365b22505..5c3b2aa3e69d 100644
--- a/include/drm/drm_file.h
+++ b/include/drm/drm_file.h
@@ -446,6 +446,9 @@  static inline bool drm_is_accel_client(const struct drm_file *file_priv)
 	return file_priv->minor->type == DRM_MINOR_ACCEL;
 }
 
+__printf(2, 3)
+void drm_file_err(struct drm_file *file_priv, const char *fmt, ...);
+
 void drm_file_update_pid(struct drm_file *);
 
 struct drm_minor *drm_minor_acquire(struct xarray *minors_xa, unsigned int minor_id);