Re: [PATCH v3 1/2] drm/i915: Simplify _STATE_ debug macros

[Joonas Lahtinen <joonas.lahtinen@linux.intel.com>]

On ma, 2015-12-21 at 14:41 +0100, Daniel Vetter wrote:
> On Mon, Dec 21, 2015 at 11:53:52AM +0000, Dave Gordon wrote:
> > On 21/12/15 08:11, Joonas Lahtinen wrote:
> > > On pe, 2015-12-18 at 16:18 +0000, Dave Gordon wrote:
> > > > On 18/12/15 12:27, Joonas Lahtinen wrote:
> > > > > Take advantage of WARN return value to simplify the flow.
> > > > > 
> > > > > Cc: Rob Clark <robdclark@gmail.com>
> > > > > Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
> > > > > Reported-by: Chris Wilson <chris@chris-wilson.co.uk>
> > > > > Signed-off-by: Joonas Lahtinen <
> > > > > joonas.lahtinen@linux.intel.com>
> > > > > ---
> > > > >   drivers/gpu/drm/i915/i915_drv.h | 15 +++++----------
> > > > >   1 file changed, 5 insertions(+), 10 deletions(-)
> > > > > 
> > > > > diff --git a/drivers/gpu/drm/i915/i915_drv.h
> > > > > b/drivers/gpu/drm/i915/i915_drv.h
> > > > > index 1d28d90..5a5a3e0 100644
> > > > > --- a/drivers/gpu/drm/i915/i915_drv.h
> > > > > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > > > > @@ -87,23 +87,18 @@
> > > > >    */
> > > > >   #define I915_STATE_WARN(condition, format...) ({		
> > > > > 	
> > > > > \
> > > > >   	int __ret_warn_on = !!(condition);			
> > > > > 	\
> > > > > -	if (unlikely(__ret_warn_on)) {			
> > > > > 	
> > > > > 	\
> > > > > -		if (i915.verbose_state_checks)		
> > > > > 	
> > > > > 	\
> > > > > -			WARN(1, format);			
> > > > > 	
> > > > > \
> > > > > -		else 					
> > > > > 	
> > > > > 	\
> > > > > +	if (unlikely(__ret_warn_on))				
> > > > > 	\
> > > > > +		if (!WARN(i915.verbose_state_checks,
> > > > > format))	
> > > > > 	\
> > > > >   			DRM_ERROR(format);			
> > > > > 	\
> > > > > -	}							
> > > > > 	
> > > > > \
> > > > >   	unlikely(__ret_warn_on);				
> > > > > 	
> > > > > \
> > > > >   })
> > > > > 
> > > > >   #define I915_STATE_WARN_ON(condition) ({			
> > > > > 	
> > > > > \
> > > > >   	int __ret_warn_on = !!(condition);			
> > > > > 	\
> > > > > -	if (unlikely(__ret_warn_on)) {			
> > > > > 	
> > > > > 	\
> > > > > -		if (i915.verbose_state_checks)		
> > > > > 	
> > > > > 	\
> > > > > -			WARN(1, "WARN_ON(" #condition
> > > > > ")\n");	
> > > > > 	\
> > > > > -		else 					
> > > > > 	
> > > > > 	\
> > > > > +	if (unlikely(__ret_warn_on))				
> > > > > 	\
> > > > > +		if (!WARN(i915.verbose_state_checks,		
> > > > > 	\
> > > > > +			  "WARN_ON(" #condition ")\n"))	
> > > > > 	
> > > > > 	\
> > > > >   			DRM_ERROR("WARN_ON(" #condition
> > > > > ")\n");	
> > > > > 	\
> > > > 
> > > > These last two lines still have the text of the condition as
> > > > part of
> > > > a
> > > > format string :(
> > > > 
> > > > For compile-testing, you might want to change:
> > > > 
> > > >    static void lpt_bend_clkout_dp(struct drm_i915_private
> > > > *dev_priv
> > > >    ...
> > > >      if (WARN_ON(steps % 5 != 0))
> > > >        return;
> > > > 
> > > > to use I915_STATE_WARN_ON() instead of WARN_ON, then you should
> > > > get a
> > > > compile-time warning if the '%' ends up in the format string.
> > > > 
> > > 
> > > This is just a patch to convert the old macros to different order
> > > before changing them. The way of constructing the strings is
> > > intact.
> > > 
> > > Regards, Joonas
> > 
> > Yes, I agree, you didn't break them -- they were already wrong!
> 
> Yeah I think it makes sense to fix that. I'll wait for v4.

Ok, to be perfectly clear; This is patch 1/2, which does as the commit
message specifies, it simplifies the function flow, patch 2/2 of the
series changes the string construction and addresses the mentioned
problem. Sorry for being little bit unclear about that on the first
comment.

Regards, Joonas

> -Daniel
-- 
Joonas Lahtinen
Open Source Technology Center
Intel Corporation

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