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.
Sure, right thing to do, will update. Not sure why i added it, may be in
initial versions needed that :)
regards Sunil
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...
I guess that's true. Let me try that and will make it inline if no warning.
Regards
Sunil
+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.
It builds and works well and i dont see the need to sched.h and etc. I
dont see which function you mean by inlining here, mind to share which
functions ?
Regards Sunil Khatri
+/**
+ * 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.
Not getting to know which function again you mean inline here ?
Regards
Sunil Khatri
Make all of these real functions, no need to include drm_print.h, and
everything is better.
Sure i will do that and thanks a lot for those inputs.
BR,
Jani.
void drm_file_update_pid(struct drm_file *);
struct drm_minor *drm_minor_acquire(struct xarray *minors_xa, unsigned int minor_id);
