Re: [PATCH] drm: Add DRM_NOTICE_IF

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 28/07/15 14:14, Chris Wilson wrote:
Styled after WARN_ON/DRM_ERROR_ON, this prints a mild warning message (a
KERN_NOTICE for significant but mild events) that allows us to insert
interesting events without alarming the user or bug reporting tools.

For an example I have changed a DRM_ERROR for being unable to set a
performance enhancement in i915.

Signed-off-by: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>
---
  drivers/gpu/drm/i915/intel_lrc.c |  5 ++---
  include/drm/drmP.h               | 20 ++++++++++++++++++++
  2 files changed, 22 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
index 184d5f2dce21..f62cd78f8691 100644
--- a/drivers/gpu/drm/i915/intel_lrc.c
+++ b/drivers/gpu/drm/i915/intel_lrc.c
@@ -1902,13 +1902,12 @@ static int gen8_init_rcs_context(struct drm_i915_gem_request *req)
  	if (ret)
  		return ret;

-	ret = intel_rcs_context_init_mocs(req);
  	/*
  	 * Failing to program the MOCS is non-fatal.The system will not
  	 * run at peak performance. So generate an error and carry on.
  	 */
-	if (ret)
-		DRM_ERROR("MOCS failed to program: expect performance issues.\n");
+	DRM_NOTICE_IF(intel_rcs_context_init_mocs(req),
+		      "MOCS failed to program: expect performance issues.\n");

I like the general idea of the macro, but I don't like this style of usage, specifically, embedding a function with side-effects inside the macro call. I'd much prefer

	ret = intel_rcs_context_init_mocs(req);
	DRM_NOTICE_IF(ret, "MOCS failed to program: expect performance issues.\n");

I think it's because the shouty MACRO_IN_ALL_CAPS distracts from looking at the details of the boolean thing-being-tested. Or maybe that anything that looks like a JUST_PRINT_A_MESSAGE() call should be optional, so I can delete it or comment it out without making a difference to the rest of the code - and putting important calls inside the macro invocation violates that principle.

  	return intel_lr_context_render_state_init(req);
  }
diff --git a/include/drm/drmP.h b/include/drm/drmP.h
index b76af322d812..f2d68d185274 100644
--- a/include/drm/drmP.h
+++ b/include/drm/drmP.h
@@ -181,6 +181,26 @@ void drm_err(const char *format, ...);
  })

  /**
+ * Mild warning on assertion-esque failure.
+ *
+ * \param cond condition on which to *fail*
+ * \param fmt printf() like format string.
+ * \param arg arguments
+ *
+ * This is similar to WARN_ON but only prints a NOTICE rather than a warning
+ * and the whole stacktrace. It is only intended for mild issues which
+ * while significant do not critically impact the user (such as a performance
+ * issue).
+ */
+#define DRM_NOTICE_IF(cond, fmt, ...) ({				\
+	bool __cond = !!(cond);						\
+	if (unlikely(__cond))						\
+		printk(KERN_NOTICE "[" DRM_NAME ":%s] " fmt,		\
+		       __func__, ##__VA_ARGS__);			\
+	unlikely(__cond);						\
+})

Why DRM_NOTICE_IF() rather than DRM_NOTICE_ON() ? It might actually be a more sensible name but sort of loses the connection with the BUG_ON and WARN_ON macros.

.Dave.

_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx




[Index of Archives]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]
  Powered by Linux