* [PATCH] drm/i915: logical-not-parenthesis gcc 5.0 fixes
@ 2015-02-22 19:10 François Tigeot
2015-02-22 22:04 ` Chris Wilson
2015-02-23 13:52 ` Dave Gordon
0 siblings, 2 replies; 4+ messages in thread
From: François Tigeot @ 2015-02-22 19:10 UTC (permalink / raw)
To: intel-gfx
* 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
^ permalink raw reply related [flat|nested] 4+ messages in thread* Re: [PATCH] drm/i915: logical-not-parenthesis gcc 5.0 fixes
2015-02-22 19:10 [PATCH] drm/i915: logical-not-parenthesis gcc 5.0 fixes François Tigeot
@ 2015-02-22 22:04 ` Chris Wilson
2015-02-23 9:25 ` Jani Nikula
2015-02-23 13:52 ` Dave Gordon
1 sibling, 1 reply; 4+ messages in thread
From: Chris Wilson @ 2015-02-22 22:04 UTC (permalink / raw)
To: François Tigeot; +Cc: intel-gfx
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
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH] drm/i915: logical-not-parenthesis gcc 5.0 fixes
2015-02-22 22:04 ` Chris Wilson
@ 2015-02-23 9:25 ` Jani Nikula
0 siblings, 0 replies; 4+ messages in thread
From: Jani Nikula @ 2015-02-23 9:25 UTC (permalink / raw)
To: Chris Wilson, François Tigeot; +Cc: intel-gfx
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
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] drm/i915: logical-not-parenthesis gcc 5.0 fixes
2015-02-22 19:10 [PATCH] drm/i915: logical-not-parenthesis gcc 5.0 fixes François Tigeot
2015-02-22 22:04 ` Chris Wilson
@ 2015-02-23 13:52 ` Dave Gordon
1 sibling, 0 replies; 4+ messages in thread
From: Dave Gordon @ 2015-02-23 13:52 UTC (permalink / raw)
To: François Tigeot, intel-gfx
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
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2015-02-23 13:52 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2015-02-22 19:10 [PATCH] drm/i915: logical-not-parenthesis gcc 5.0 fixes François Tigeot
2015-02-22 22:04 ` Chris Wilson
2015-02-23 9:25 ` Jani Nikula
2015-02-23 13:52 ` Dave Gordon
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox