<!DOCTYPE html><html><head>
<meta http-equiv="Content-Type" content="text/html; charset=utf-8">
</head>
<body>
<p><br>
</p>
<div class="moz-cite-prefix">On 4/16/2025 7:55 PM, Jani Nikula
wrote:<br>
</div>
<blockquote type="cite" cite="mid:87lds0p4jh.fsf@intel.com">
<pre wrap="" class="moz-quote-pre">On Wed, 16 Apr 2025, Sunil Khatri <a class="moz-txt-link-rfc2396E" href="mailto:sunil.khatri@amd.com"><sunil.khatri@amd.com></a> wrote:
</pre>
<blockquote type="cite">
<pre wrap="" class="moz-quote-pre">Add a drm helper macro which append the process information for
the drm_file over drm_err.
Signed-off-by: Sunil Khatri <a class="moz-txt-link-rfc2396E" href="mailto:sunil.khatri@amd.com"><sunil.khatri@amd.com></a>
---
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>
</pre>
</blockquote>
<pre wrap="" class="moz-quote-pre">
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.
</pre>
<blockquote type="cite">
<pre wrap="" class="moz-quote-pre">
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));
</pre>
</blockquote>
<pre wrap="" class="moz-quote-pre">
inline is the keyword you're missing here, and that's why you've had to
add maybe unused...
</pre>
<blockquote type="cite">
<pre wrap="" class="moz-quote-pre">+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);
+}
</pre>
</blockquote>
<pre wrap="" class="moz-quote-pre">
...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.</pre>
</blockquote>
<p>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</p>
<p>passing that in the drm_err as string.<br>
</p>
<blockquote type="cite" cite="mid:87lds0p4jh.fsf@intel.com">
<pre wrap="" class="moz-quote-pre">
</pre>
<blockquote type="cite">
<pre wrap="" class="moz-quote-pre">+/**
+ * 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)
+
</pre>
</blockquote>
<pre wrap="" class="moz-quote-pre">
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.
</pre>
</blockquote>
<p><span style="white-space: pre-wrap">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.</span></p>
<p><span style="white-space: pre-wrap">Regards
Sunil Khatri
</span></p>
<blockquote type="cite" cite="mid:87lds0p4jh.fsf@intel.com">
<pre wrap="" class="moz-quote-pre">
BR,
Jani.
</pre>
</blockquote>
<blockquote type="cite" cite="mid:87lds0p4jh.fsf@intel.com">
<blockquote type="cite">
<pre wrap="" class="moz-quote-pre"> void drm_file_update_pid(struct drm_file *);
struct drm_minor *drm_minor_acquire(struct xarray *minors_xa, unsigned int minor_id);
</pre>
</blockquote>
<pre wrap="" class="moz-quote-pre">
</pre>
</blockquote>
</body>
</html>