<!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>
<p><span style="white-space: pre-wrap">Sure, right thing to do, will update. Not sure why i added it, may be in initial versions needed that :)</span></p>
<p><span style="white-space: pre-wrap">regards
Sunil
</span></p>
<blockquote type="cite" cite="mid:87lds0p4jh.fsf@intel.com">
<pre wrap="" class="moz-quote-pre">
</pre>
</blockquote>
<blockquote type="cite" cite="mid:87lds0p4jh.fsf@intel.com">
<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>
<p>I guess that's true. Let me try that and will make it inline if
no warning. <br>
</p>
<p>Regards<br>
Sunil<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">+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><span style="white-space: pre-wrap">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 ?</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">
</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.</pre>
</blockquote>
<p>Not getting to know which function again you mean inline here ? <br>
</p>
<p>Regards<br>
Sunil Khatri<br>
</p>
<blockquote type="cite" cite="mid:87lds0p4jh.fsf@intel.com">
<pre wrap="" class="moz-quote-pre">
Make all of these real functions, no need to include drm_print.h, and
everything is better.</pre>
</blockquote>
Sure i will do that and thanks a lot for those inputs.<br>
<blockquote type="cite" cite="mid:87lds0p4jh.fsf@intel.com">
<pre wrap="" class="moz-quote-pre">
BR,
Jani.
</pre>
<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>