[PATCH 1/2] drm/i915: Add -Wall -Wextra to our build, set warnings to full

[PATCH 1/2] drm/i915: Add -Wall -Wextra to our build, set warnings to full

[Chris Wilson]

Recently W=1 on gcc-7.2 (-Wunused-const-variable) caught a regression
that had been lurking for 6 months, so lets try enabling the full set of
warnings for CI builds. This means more patches will be rejected early
that contain trivial and sometimes not so trivial bugs. However, our
code does not yet compile cleanly with W=1, so we have to apply a filter
to the set of warnings until we can eliminate the mistakes. It also
means that developers will have to be running the full gamut of gcc to
ensure that as warnings come and go with gcc updates, we have the CI
build prepared.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
Cc: Jani Nikula <jani.nikula@intel.com>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Cc: Tomi Sarvela <tomi.p.sarvela@intel.com>
Cc: Micahal Wajdeczko <michal.wajdeczko@intel.com>
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/Makefile | 18 +++++++++++++++++-
 1 file changed, 17 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile
index 66d23b619db1..52f0f8f101ec 100644
--- a/drivers/gpu/drm/i915/Makefile
+++ b/drivers/gpu/drm/i915/Makefile
@@ -2,7 +2,23 @@
 # Makefile for the drm device driver.  This driver provides support for the
 # Direct Rendering Infrastructure (DRI) in XFree86 4.1.0 and higher.
 
-subdir-ccflags-$(CONFIG_DRM_I915_WERROR) := -Werror
+# Add a set of useful warning flags and enable -Werror for CI to prevent
+# trivial mistakes from creeping in. We have to do this piecemeal as we reject
+# any patch that isn't warning clean, so turning on -Wall -Wextra (or W=1) we
+# need to filter out dubious warnings.  Still it is our interest
+# to keep running locally with W=1 C=1 until we are completely clean.
+#
+# Note the danger in using -Wall -Wextra is that when CI updates gcc we
+# will most likely get a sudden build breakage... Hopefully we will fix
+# new warnings before CI updates!
+subdir-ccflags-y := -Wall -Wextra
+subdir-ccflags-y += $(call cc-option,-Wno-unused-parameter,)
+subdir-ccflags-y += $(call cc-option,-Wno-type-limits,)
+subdir-ccflags-y += $(call cc-option,-Wno-missing-field-initializers,)
+subdir-ccflags-y += $(call cc-option,-Wno-override-init,)
+subdir-ccflags-y += $(call cc-option,-Wno-implicit-fallthrough,)
+subdir-ccflags-$(CONFIG_DRM_I915_WERROR) += -Werror
+
 subdir-ccflags-y += \
 	$(call as-instr,movntdqa (%eax)$(comma)%xmm0,-DCONFIG_AS_MOVNTDQA)
 
-- 
2.15.0.rc0

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

[PATCH 2/2] drm/i915: Remove -Wno-override-init

[Chris Wilson]

Mark up the locations where we are intentionally overriding
initialization parameters for inheritance with a local pragma so that we
can enable -Woverride-init globally and catch accidental mistakes.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
Cc: Jani Nikula <jani.nikula@intel.com>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Cc: Tomi Sarvela <tomi.p.sarvela@intel.com>
Cc: Micahal Wajdeczko <michal.wajdeczko@intel.com>
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/Makefile      | 1 -
 drivers/gpu/drm/i915/i915_pci.c    | 5 +++++
 drivers/gpu/drm/i915/intel_fbdev.c | 5 +++++
 3 files changed, 10 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile
index 52f0f8f101ec..e1bbb2565426 100644
--- a/drivers/gpu/drm/i915/Makefile
+++ b/drivers/gpu/drm/i915/Makefile
@@ -15,7 +15,6 @@ subdir-ccflags-y := -Wall -Wextra
 subdir-ccflags-y += $(call cc-option,-Wno-unused-parameter,)
 subdir-ccflags-y += $(call cc-option,-Wno-type-limits,)
 subdir-ccflags-y += $(call cc-option,-Wno-missing-field-initializers,)
-subdir-ccflags-y += $(call cc-option,-Wno-override-init,)
 subdir-ccflags-y += $(call cc-option,-Wno-implicit-fallthrough,)
 subdir-ccflags-$(CONFIG_DRM_I915_WERROR) += -Werror
 
diff --git a/drivers/gpu/drm/i915/i915_pci.c b/drivers/gpu/drm/i915/i915_pci.c
index bf467f30c99b..2ad0738eb2e7 100644
--- a/drivers/gpu/drm/i915/i915_pci.c
+++ b/drivers/gpu/drm/i915/i915_pci.c
@@ -29,6 +29,9 @@
 #include "i915_drv.h"
 #include "i915_selftest.h"
 
+#pragma GCC diagnostic push
+#pragma GCC diagnostic ignored "-Woverride-init" /* used for inheritance */
+
 #define GEN_DEFAULT_PIPEOFFSETS \
 	.pipe_offsets = { PIPE_A_OFFSET, PIPE_B_OFFSET, \
 			  PIPE_C_OFFSET, PIPE_EDP_OFFSET }, \
@@ -640,6 +643,8 @@ static const struct pci_device_id pciidlist[] = {
 };
 MODULE_DEVICE_TABLE(pci, pciidlist);
 
+#pragma GCC diagnostic pop
+
 static void i915_pci_remove(struct pci_dev *pdev)
 {
 	struct drm_device *dev = pci_get_drvdata(pdev);
diff --git a/drivers/gpu/drm/i915/intel_fbdev.c b/drivers/gpu/drm/i915/intel_fbdev.c
index b8af35187d22..7fc7f83d4b74 100644
--- a/drivers/gpu/drm/i915/intel_fbdev.c
+++ b/drivers/gpu/drm/i915/intel_fbdev.c
@@ -96,6 +96,9 @@ static int intel_fbdev_pan_display(struct fb_var_screeninfo *var,
 	return ret;
 }
 
+#pragma GCC diagnostic push
+#pragma GCC diagnostic ignored "-Woverride-init" /* used for inheritance */
+
 static struct fb_ops intelfb_ops = {
 	.owner = THIS_MODULE,
 	DRM_FB_HELPER_DEFAULT_OPS,
@@ -107,6 +110,8 @@ static struct fb_ops intelfb_ops = {
 	.fb_blank = intel_fbdev_blank,
 };
 
+#pragma GCC diagnostic pop
+
 static int intelfb_alloc(struct drm_fb_helper *helper,
 			 struct drm_fb_helper_surface_size *sizes)
 {
-- 
2.15.0.rc0

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

Re: [PATCH 2/2] drm/i915: Remove -Wno-override-init

[Michal Wajdeczko]

On Sat, 14 Oct 2017 12:12:50 +0200, Chris Wilson  
<chris@chris-wilson.co.uk> wrote:

> Mark up the locations where we are intentionally overriding
> initialization parameters for inheritance with a local pragma so that we
> can enable -Woverride-init globally and catch accidental mistakes.
>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> Cc: Jani Nikula <jani.nikula@intel.com>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> Cc: Tomi Sarvela <tomi.p.sarvela@intel.com>
> Cc: Micahal Wajdeczko <michal.wajdeczko@intel.com>
       ^^^^^^^
s/Micahal/Michal please ;)

> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/Makefile      | 1 -
>  drivers/gpu/drm/i915/i915_pci.c    | 5 +++++
>  drivers/gpu/drm/i915/intel_fbdev.c | 5 +++++
>  3 files changed, 10 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/i915/Makefile  
> b/drivers/gpu/drm/i915/Makefile
> index 52f0f8f101ec..e1bbb2565426 100644
> --- a/drivers/gpu/drm/i915/Makefile
> +++ b/drivers/gpu/drm/i915/Makefile
> @@ -15,7 +15,6 @@ subdir-ccflags-y := -Wall -Wextra
>  subdir-ccflags-y += $(call cc-option,-Wno-unused-parameter,)
>  subdir-ccflags-y += $(call cc-option,-Wno-type-limits,)
>  subdir-ccflags-y += $(call cc-option,-Wno-missing-field-initializers,)
> -subdir-ccflags-y += $(call cc-option,-Wno-override-init,)

Maybe it would be better to re-order patches to avoid this step

>  subdir-ccflags-y += $(call cc-option,-Wno-implicit-fallthrough,)
>  subdir-ccflags-$(CONFIG_DRM_I915_WERROR) += -Werror
> diff --git a/drivers/gpu/drm/i915/i915_pci.c  
> b/drivers/gpu/drm/i915/i915_pci.c
> index bf467f30c99b..2ad0738eb2e7 100644
> --- a/drivers/gpu/drm/i915/i915_pci.c
> +++ b/drivers/gpu/drm/i915/i915_pci.c
> @@ -29,6 +29,9 @@
>  #include "i915_drv.h"
>  #include "i915_selftest.h"
> +#pragma GCC diagnostic push
> +#pragma GCC diagnostic ignored "-Woverride-init" /* used for  
> inheritance */
> +

As -Wextra is also supported by clang then maybe it's worth do add

#if defined (__clang__)
#pragma clang diagnostic push
#pragma clang diagnostic ignored "-Winitializer-overrides"
...
#pragma clang diagnostic pop

Michal

>  #define GEN_DEFAULT_PIPEOFFSETS \
>  	.pipe_offsets = { PIPE_A_OFFSET, PIPE_B_OFFSET, \
>  			  PIPE_C_OFFSET, PIPE_EDP_OFFSET }, \
> @@ -640,6 +643,8 @@ static const struct pci_device_id pciidlist[] = {
>  };
>  MODULE_DEVICE_TABLE(pci, pciidlist);
> +#pragma GCC diagnostic pop
> +
>  static void i915_pci_remove(struct pci_dev *pdev)
>  {
>  	struct drm_device *dev = pci_get_drvdata(pdev);
> diff --git a/drivers/gpu/drm/i915/intel_fbdev.c  
> b/drivers/gpu/drm/i915/intel_fbdev.c
> index b8af35187d22..7fc7f83d4b74 100644
> --- a/drivers/gpu/drm/i915/intel_fbdev.c
> +++ b/drivers/gpu/drm/i915/intel_fbdev.c
> @@ -96,6 +96,9 @@ static int intel_fbdev_pan_display(struct  
> fb_var_screeninfo *var,
>  	return ret;
>  }
> +#pragma GCC diagnostic push
> +#pragma GCC diagnostic ignored "-Woverride-init" /* used for  
> inheritance */
> +
>  static struct fb_ops intelfb_ops = {
>  	.owner = THIS_MODULE,
>  	DRM_FB_HELPER_DEFAULT_OPS,
> @@ -107,6 +110,8 @@ static struct fb_ops intelfb_ops = {
>  	.fb_blank = intel_fbdev_blank,
>  };
> +#pragma GCC diagnostic pop
> +
>  static int intelfb_alloc(struct drm_fb_helper *helper,
>  			 struct drm_fb_helper_surface_size *sizes)
>  {
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/2] drm/i915: Remove -Wno-override-init

[Chris Wilson]

Quoting Michal Wajdeczko (2017-10-14 21:02:05)
> On Sat, 14 Oct 2017 12:12:50 +0200, Chris Wilson  
> > diff --git a/drivers/gpu/drm/i915/Makefile  
> > b/drivers/gpu/drm/i915/Makefile
> > index 52f0f8f101ec..e1bbb2565426 100644
> > --- a/drivers/gpu/drm/i915/Makefile
> > +++ b/drivers/gpu/drm/i915/Makefile
> > @@ -15,7 +15,6 @@ subdir-ccflags-y := -Wall -Wextra
> >  subdir-ccflags-y += $(call cc-option,-Wno-unused-parameter,)
> >  subdir-ccflags-y += $(call cc-option,-Wno-type-limits,)
> >  subdir-ccflags-y += $(call cc-option,-Wno-missing-field-initializers,)
> > -subdir-ccflags-y += $(call cc-option,-Wno-override-init,)
> 
> Maybe it would be better to re-order patches to avoid this step

My intention was for this to be an example of the steps we probably want
to take with these and new warnings in future. If gcc brings in a new
warning, we will probably first do the simple patch to disable it, and
then a series to fix the errors before re-enabling. Simply to minimise
the pain of breaking the build.

> >  subdir-ccflags-y += $(call cc-option,-Wno-implicit-fallthrough,)
> >  subdir-ccflags-$(CONFIG_DRM_I915_WERROR) += -Werror
> > diff --git a/drivers/gpu/drm/i915/i915_pci.c  
> > b/drivers/gpu/drm/i915/i915_pci.c
> > index bf467f30c99b..2ad0738eb2e7 100644
> > --- a/drivers/gpu/drm/i915/i915_pci.c
> > +++ b/drivers/gpu/drm/i915/i915_pci.c
> > @@ -29,6 +29,9 @@
> >  #include "i915_drv.h"
> >  #include "i915_selftest.h"
> > +#pragma GCC diagnostic push
> > +#pragma GCC diagnostic ignored "-Woverride-init" /* used for  
> > inheritance */
> > +
> 
> As -Wextra is also supported by clang then maybe it's worth do add

Bleh. I guess we actually want to make the -Werror conditional on using
gcc. I don't really want to maintain two sets of suppressions, we
already multiple for different gcc versions, but anything outside of
common use (read CI) simply won't be used that much and liable to rot.

Otoh, if did compile every patch with both gcc and clang, that should
give us the widest set of error detection -- and possibly also the most
noise. clang is not a drop in replacement, that increases the burden of
adoption significantly.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✓ Fi.CI.BAT: success for series starting with [1/2] drm/i915: Add -Wall -Wextra to our build, set warnings to full

[Patchwork]

== Series Details ==

Series: series starting with [1/2] drm/i915: Add -Wall -Wextra to our build, set warnings to full
URL   : https://patchwork.freedesktop.org/series/31974/
State : success

== Summary ==

Series 31974v1 series starting with [1/2] drm/i915: Add -Wall -Wextra to our build, set warnings to full
https://patchwork.freedesktop.org/api/1.0/series/31974/revisions/1/mbox/

Test chamelium:
        Subgroup dp-crc-fast:
                pass       -> FAIL       (fi-kbl-7500u) fdo#102514

fdo#102514 https://bugs.freedesktop.org/show_bug.cgi?id=102514

fi-bdw-5557u     total:289  pass:268  dwarn:0   dfail:0   fail:0   skip:21  time:458s
fi-bdw-gvtdvm    total:289  pass:265  dwarn:0   dfail:0   fail:0   skip:24  time:476s
fi-blb-e6850     total:289  pass:223  dwarn:1   dfail:0   fail:0   skip:65  time:388s
fi-bsw-n3050     total:289  pass:243  dwarn:0   dfail:0   fail:0   skip:46  time:567s
fi-bwr-2160      total:289  pass:183  dwarn:0   dfail:0   fail:0   skip:106 time:285s
fi-bxt-dsi       total:289  pass:259  dwarn:0   dfail:0   fail:0   skip:30  time:520s
fi-bxt-j4205     total:289  pass:260  dwarn:0   dfail:0   fail:0   skip:29  time:523s
fi-byt-j1900     total:289  pass:253  dwarn:1   dfail:0   fail:0   skip:35  time:534s
fi-byt-n2820     total:289  pass:249  dwarn:1   dfail:0   fail:0   skip:39  time:517s
fi-cfl-s         total:289  pass:253  dwarn:4   dfail:0   fail:0   skip:32  time:566s
fi-elk-e7500     total:289  pass:229  dwarn:0   dfail:0   fail:0   skip:60  time:434s
fi-gdg-551       total:289  pass:178  dwarn:1   dfail:0   fail:1   skip:109 time:272s
fi-glk-1         total:289  pass:261  dwarn:0   dfail:0   fail:0   skip:28  time:601s
fi-hsw-4770r     total:289  pass:262  dwarn:0   dfail:0   fail:0   skip:27  time:440s
fi-ilk-650       total:289  pass:228  dwarn:0   dfail:0   fail:0   skip:61  time:458s
fi-ivb-3520m     total:289  pass:260  dwarn:0   dfail:0   fail:0   skip:29  time:498s
fi-ivb-3770      total:289  pass:260  dwarn:0   dfail:0   fail:0   skip:29  time:472s
fi-kbl-7500u     total:289  pass:263  dwarn:1   dfail:0   fail:1   skip:24  time:493s
fi-kbl-7567u     total:289  pass:269  dwarn:0   dfail:0   fail:0   skip:20  time:486s
fi-kbl-r         total:289  pass:262  dwarn:0   dfail:0   fail:0   skip:27  time:597s
fi-pnv-d510      total:289  pass:222  dwarn:1   dfail:0   fail:0   skip:66  time:664s
fi-skl-6260u     total:289  pass:269  dwarn:0   dfail:0   fail:0   skip:20  time:467s
fi-skl-6700hq    total:289  pass:263  dwarn:0   dfail:0   fail:0   skip:26  time:659s
fi-skl-6700k     total:289  pass:265  dwarn:0   dfail:0   fail:0   skip:24  time:534s
fi-skl-6770hq    total:289  pass:269  dwarn:0   dfail:0   fail:0   skip:20  time:568s
fi-skl-gvtdvm    total:289  pass:266  dwarn:0   dfail:0   fail:0   skip:23  time:475s
fi-snb-2520m     total:289  pass:250  dwarn:0   dfail:0   fail:0   skip:39  time:589s
fi-snb-2600      total:289  pass:249  dwarn:0   dfail:0   fail:0   skip:40  time:427s

3d7ee91be487380ef6cad329fafbe424f6885372 drm-tip: 2017y-10m-14d-00h-14m-47s UTC integration manifest
cb4daf4e71ac drm/i915: Remove -Wno-override-init
935e8ae6f6ea drm/i915: Add -Wall -Wextra to our build, set warnings to full

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_6038/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✗ Fi.CI.IGT: failure for series starting with [1/2] drm/i915: Add -Wall -Wextra to our build, set warnings to full

[Patchwork]

== Series Details ==

Series: series starting with [1/2] drm/i915: Add -Wall -Wextra to our build, set warnings to full
URL   : https://patchwork.freedesktop.org/series/31974/
State : failure

== Summary ==

Test kms_flip:
        Subgroup basic-flip-vs-wf_vblank:
                pass       -> FAIL       (shard-hsw)
Test kms_plane:
        Subgroup plane-panning-bottom-right-suspend-pipe-C-planes:
                skip       -> PASS       (shard-hsw)
Test kms_frontbuffer_tracking:
        Subgroup fbc-rgb101010-draw-mmap-gtt:
                skip       -> PASS       (shard-hsw)

shard-hsw        total:2553 pass:1440 dwarn:0   dfail:0   fail:10  skip:1103 time:9638s

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_6038/shards.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/2] drm/i915: Add -Wall -Wextra to our build, set warnings to full

[Joonas Lahtinen]

On Sat, 2017-10-14 at 11:12 +0100, Chris Wilson wrote:
> Recently W=1 on gcc-7.2 (-Wunused-const-variable) caught a regression
> that had been lurking for 6 months, so lets try enabling the full set of
> warnings for CI builds. This means more patches will be rejected early
> that contain trivial and sometimes not so trivial bugs. However, our
> code does not yet compile cleanly with W=1, so we have to apply a filter
> to the set of warnings until we can eliminate the mistakes. It also
> means that developers will have to be running the full gamut of gcc to
> ensure that as warnings come and go with gcc updates, we have the CI
> build prepared.
> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> Cc: Jani Nikula <jani.nikula@intel.com>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> Cc: Tomi Sarvela <tomi.p.sarvela@intel.com>
> Cc: Micahal Wajdeczko <michal.wajdeczko@intel.com>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>

The first portion (this patch) I can easily agree on. I'm not so sure
about the #pragma stuff to move the information to source files. It
sounds like making better use of the compiler warnings, but would we
maybe want to isolate at file level the ones that have the exemption?

Regards, Joonas
-- 
Joonas Lahtinen
Open Source Technology Center
Intel Corporation
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx