[PATCH v4 1/5] drm: add macro drm_file_err to print process info

[Khatri, Sunil]

On 4/16/2025 7:55 PM, Jani Nikula wrote:
> On Wed, 16 Apr 2025, Sunil Khatri<sunil.khatri at amd.com> wrote:
>> Add a drm helper macro which append the process information for
>> the drm_file over drm_err.
>>
>> Signed-off-by: Sunil Khatri<sunil.khatri at amd.com>
>> ---
>>   include/drm/drm_file.h | 41 +++++++++++++++++++++++++++++++++++++++++
>>   1 file changed, 41 insertions(+)
>>
>> diff --git a/include/drm/drm_file.h b/include/drm/drm_file.h
>> index 94d365b22505..5ae5ad1048fb 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>
> Not a fan of including drm_print.h everywhere that drm_file.h is
> included. We've been trying to get rid of this, and go the other
> way. It's really hard to manage dependencies when everything ends up
> including everything.
>
>>   
>>   struct dma_fence;
>>   struct drm_file;
>> @@ -446,6 +447,46 @@ static inline bool drm_is_accel_client(const struct drm_file *file_priv)
>>   	return file_priv->minor->type == DRM_MINOR_ACCEL;
>>   }
>>   
>> +static struct task_struct *drm_task_lock(struct drm_file *file_priv)
>> +					__attribute__((__maybe_unused));
> inline is the keyword you're missing here, and that's why you've had to
> add maybe unused...
>
>> +static struct task_struct *drm_task_lock(struct drm_file *file_priv)
>> +{
>> +	struct task_struct *task;
>> +	struct pid *pid;
>> +
>> +	mutex_lock(&file_priv->client_name_lock);
>> +	rcu_read_lock();
>> +	pid = rcu_dereference(file_priv->pid);
>> +	task = pid_task(pid, PIDTYPE_TGID);
>> +	return task;
>> +}
>> +
>> +static void drm_task_unlock(struct drm_file *file_priv) __attribute__((__maybe_unused));
>> +static void drm_task_unlock(struct drm_file *file_priv)
>> +{
>> +	rcu_read_unlock();
>> +	mutex_unlock(&file_priv->client_name_lock);
>> +}
> ...but *why* are you inlining these? To make this header self-contained,
> I think you'd need to add maybe sched.h, pid.h, rcupdate.h, mutex.h, or
> something. I consider static inlines actively harmful if they force you
> to pull in a lot of other headers.

Code readability and easy maintenance is the key to make these as 
inline. Also we are  keeping the logic function which gets the task with 
locks in separate function then actually

passing that in the drm_err as string.

>
>> +/**
>> + * 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
>> + */
>> +#define drm_file_err(file_priv, fmt, ...)						\
>> +	do {										\
>> +		struct task_struct *task;						\
>> +		struct drm_device *dev = file_priv->minor->dev;				\
>> +											\
>> +		task = drm_task_lock(file_priv);					\
>> +		drm_err(dev, "comm: %s pid: %d client: %s " fmt,			\
>> +			task ? task->comm : "", task ? task->pid : 0,			\
>> +			file_priv->client_name ?: "Unset", ##__VA_ARGS__);		\
>> +		drm_task_unlock(file_priv);						\
>> +	} while (0)
>> +
> For that matter, why is *this* inline? For debugs it makes a little more
> sense when it adds the function, but drm_err() doesn't.
>
> Make all of these real functions, no need to include drm_print.h, and
> everything is better.
>
Only reason of hacing drm_file_err as a macro as the variadic fmt and 
args are not possible to pass without using local variables to make 
strings of fmt and args separately and with macro to macro they Are 
passed cleanly and no local variables needed. you can check V3 i guess 
where this whole was an function only.

Regards Sunil Khatri

> BR,
> Jani.
>
>>   void drm_file_update_pid(struct drm_file *);
>>   
>>   struct drm_minor *drm_minor_acquire(struct xarray *minors_xa, unsigned int minor_id);
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/amd-gfx/attachments/20250417/e28cbf4f/attachment.htm>