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

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

[François Tigeot]

* 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)) {
 			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) ))
 		return false;
 
 	return true;
-- 
2.2.2

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

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

[Chris Wilson]

On Sun, Feb 22, 2015 at 08:10:11PM +0100, 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)) {

That doesn't improve the code, and here it looks like gcc's warning is
simply misleading.

static inline bool new_page_needs_bit17_swizzle(struct page *page)
{
	return page_to_phys(page) & (1 << 17);
}

static inline bool old_page_has_bit7_swizzle(struct drm_i915_gem_object *obj, 
					     int page)
{
	return test_bit(page, obj->bit_17);
}

if (new_page_needs_bit17_swizzle(page) != old_page_has_bit17_swizzle(obj, i)) {

Should be clearer.

>  			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) ))

That's wrong.
-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/i915: logical-not-parenthesis gcc 5.0 fixes

[Jani Nikula]

On Mon, 23 Feb 2015, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> On Sun, Feb 22, 2015 at 08:10:11PM +0100, François Tigeot wrote:
>> * Originally added by John Marino in DragonFly's eecf6c3c3b6f7127edd8b8f8c2a83e2f882ed0da
>>   commit.

...

>> 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) ))
>
> That's wrong.

Indeed; this has introduced a bug in DragonFly. Not that I care, but so
you know.

BR,
Jani.


-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Dave Gordon]

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