[PATCH] drm: Add DRM_NOTICE_IF

[PATCH] drm: Add DRM_NOTICE_IF

[Chris Wilson]

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@chris-wilson.co.uk>
---
 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");
 
 	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);						\
+})
+
+/**
  * Rate limited error output.  Like DRM_ERROR() but won't flood the log.
  *
  * \param fmt printf() like format string.
-- 
2.4.6

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm: Add DRM_NOTICE_IF

[Dave Gordon]

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@chris-wilson.co.uk>
> ---
>   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@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm: Add DRM_NOTICE_IF

[Chris Wilson]

On Tue, Jul 28, 2015 at 04:58:27PM +0100, Dave Gordon wrote:
> 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@chris-wilson.co.uk>
> >---
> >  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.

On the other hand, if it printed the condition that failed as part of
the message, then the function name is much more expressive than ret. I
am also not convinced that the latter is more readable than the former
in this particular example.

> >+#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.

Because I was trying to avoid having it confused with the BUG_ON and the
NOTICE_IF did serve to make it a gentler and milder request to do something.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm: Add DRM_NOTICE_IF

[Emil Velikov]

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.
> 
Wouldn't it be better to opt for DRM_NOTICE_ON to be consistent with the
other two ? It sounds fine here, although I am a non-native English
speaker, so take it with a grain of salt.

Thanks
Emil

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

Re: [PATCH] drm: Add DRM_NOTICE_IF

[Emil Velikov]

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.
> 
Wouldn't it be better to opt for DRM_NOTICE_ON to be consistent with the
other two ? It sounds fine here, although I am a non-native English
speaker, so take it with a grain of salt.

Thanks
Emil

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel