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

[Tvrtko Ursulin]

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 at 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 at 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);