On 4/16/2025 7:55 PM, Jani Nikula wrote:
On Wed, 16 Apr 2025, Sunil Khatri<sunil.kha...@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.kha...@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);
