Re: [PATCH] drm/i915: logical-not-parenthesis gcc 5.0 fixes

[Dave Gordon <david.s.gordon@intel.com>]

On 22/02/15 19:10, François Tigeot wrote:
> * This change prevents "logical not is only applied to the left hand side of comparison"
>   gcc 5.0 warnings.
> 
> * Originally added by John Marino in DragonFly's eecf6c3c3b6f7127edd8b8f8c2a83e2f882ed0da
>   commit.
> 
> Signed-off-by: François Tigeot <ftigeot@wolfpond.org>
> ---
>  drivers/gpu/drm/i915/i915_gem_tiling.c | 2 +-
>  drivers/gpu/drm/i915/intel_display.c   | 2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem_tiling.c b/drivers/gpu/drm/i915/i915_gem_tiling.c
> index 7a24bd1..402179c 100644
> --- a/drivers/gpu/drm/i915/i915_gem_tiling.c
> +++ b/drivers/gpu/drm/i915/i915_gem_tiling.c
> @@ -512,7 +512,7 @@ i915_gem_object_do_bit_17_swizzle(struct drm_i915_gem_object *obj)
>  		struct page *page = sg_page_iter_page(&sg_iter);
>  		char new_bit_17 = page_to_phys(page) >> 17;
>  		if ((new_bit_17 & 0x1) !=
> -		    (test_bit(i, obj->bit_17) != 0)) {
> +		    (test_bit(i, obj->bit_17) ? 1 : 0)) {

test_bit() already returns a bool; the last line of the definition says:

        return ((mask & *addr) != 0);

so comparing it with "!= 0" OR "? 1 : 0" is completely redundant. I'd
suggest that the clearest formulation is:

+	char new_bit_17 = (page_to_phys(page) >> 17) & 1;
+	if (new_bit_17 != test_bit(i, obj->bit_17)) {

>  			i915_gem_swizzle_page(page);
>  			set_page_dirty(page);
>  		}
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 3b0fe9f..91264b2 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -13299,7 +13299,7 @@ intel_check_plane_mapping(struct intel_crtc *crtc)
>  	val = I915_READ(reg);
>  
>  	if ((val & DISPLAY_PLANE_ENABLE) &&
> -	    (!!(val & DISPPLANE_SEL_PIPE_MASK) == crtc->pipe))
> +	    (!!( (val & DISPPLANE_SEL_PIPE_MASK) == crtc->pipe) ))

You never need a "!!" after a "&&" as it's already implicitly "!= 0".
Also, the "!!" here applies to the result of "==" which is already bool.
So it's not equivalent to the previous version which applied the "!!" to
the result of the "&".

It might be clearer to break it into several steps, especially as in
general DISPPLANE_SEL_PIPE_MASK is more than one bit, and crtc->pipe is
an enum not a bool!

BTW, are these definitions right?

#define   DISPPLANE_STEREO_ENABLE               (1<<25)
#define   DISPPLANE_STEREO_DISABLE              0
#define   DISPPLANE_PIPE_CSC_ENABLE             (1<<24)
#define   DISPPLANE_SEL_PIPE_SHIFT              24
#define   DISPPLANE_SEL_PIPE_MASK
(3<<DISPPLANE_SEL_PIPE_SHIFT)
#define   DISPPLANE_SEL_PIPE_A                  0
#define   DISPPLANE_SEL_PIPE_B
(1<<DISPPLANE_SEL_PIPE_SHIFT)

... implying that the STEREO and CRC bits occupy the same positions as
the (two?) pipe select bits? Presumably on different GENs? Maybe the
function above should be renamed to emphasise that it's an early-gen
only sort of thing ... ?

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