* [PATCH] drm/amd/amdgpu: enable W=1 for amdgpu @ 2023-06-09 16:42 Hamza Mahfooz 2023-06-09 20:17 ` Nathan Chancellor 0 siblings, 1 reply; 4+ messages in thread From: Hamza Mahfooz @ 2023-06-09 16:42 UTC (permalink / raw) To: amd-gfx Cc: Hamza Mahfooz, Alex Deucher, Nathan Chancellor, Christian König, Pan, Xinhui, David Airlie, Daniel Vetter, Hawking Zhang, Le Ma, Tao Zhou, YiPeng Chai, James Zhu, Xiaojian Du, Lijo Lazar, dri-devel, linux-kernel We have a clean build with W=1 as of commit 12a15dd589ac ("drm/amd/display/amdgpu_dm/amdgpu_dm_helpers: Move SYNAPTICS_DEVICE_ID into CONFIG_DRM_AMD_DC_DCN ifdef"). So, let's enable these checks unconditionally for the entire module to catch these errors during development. Cc: Alex Deucher <alexander.deucher@amd.com> Cc: Nathan Chancellor <nathan@kernel.org> Signed-off-by: Hamza Mahfooz <hamza.mahfooz@amd.com> --- drivers/gpu/drm/amd/amdgpu/Makefile | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-) diff --git a/drivers/gpu/drm/amd/amdgpu/Makefile b/drivers/gpu/drm/amd/amdgpu/Makefile index 86b833085f19..8d16f280b695 100644 --- a/drivers/gpu/drm/amd/amdgpu/Makefile +++ b/drivers/gpu/drm/amd/amdgpu/Makefile @@ -40,7 +40,18 @@ ccflags-y := -I$(FULL_AMD_PATH)/include/asic_reg \ -I$(FULL_AMD_PATH)/amdkfd subdir-ccflags-y := -Wextra -subdir-ccflags-y += $(call cc-option, -Wunused-but-set-variable) +subdir-ccflags-y += -Wunused +subdir-ccflags-y += -Wmissing-prototypes +subdir-ccflags-y += -Wmissing-declarations +subdir-ccflags-y += -Wmissing-include-dirs +subdir-ccflags-y += -Wold-style-definition +subdir-ccflags-y += -Wmissing-format-attribute +# Need this to avoid recursive variable evaluation issues +cond-flags := $(call cc-option, -Wunused-but-set-variable) \ + $(call cc-option, -Wunused-const-variable) \ + $(call cc-option, -Wstringop-truncation) \ + $(call cc-option, -Wpacked-not-aligned) +subdir-ccflags-y += $(cond-flags) subdir-ccflags-y += -Wno-unused-parameter subdir-ccflags-y += -Wno-type-limits subdir-ccflags-y += -Wno-sign-compare -- 2.40.1 ^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] drm/amd/amdgpu: enable W=1 for amdgpu 2023-06-09 16:42 [PATCH] drm/amd/amdgpu: enable W=1 for amdgpu Hamza Mahfooz @ 2023-06-09 20:17 ` Nathan Chancellor 2023-06-10 1:20 ` Masahiro Yamada 0 siblings, 1 reply; 4+ messages in thread From: Nathan Chancellor @ 2023-06-09 20:17 UTC (permalink / raw) To: Hamza Mahfooz Cc: amd-gfx, Alex Deucher, Christian König, Pan, Xinhui, David Airlie, Daniel Vetter, Hawking Zhang, Le Ma, Tao Zhou, YiPeng Chai, James Zhu, Xiaojian Du, Lijo Lazar, dri-devel, linux-kernel, Masahiro Yamada, linux-kbuild + Masahiro and linux-kbuild On Fri, Jun 09, 2023 at 12:42:06PM -0400, Hamza Mahfooz wrote: > We have a clean build with W=1 as of > commit 12a15dd589ac ("drm/amd/display/amdgpu_dm/amdgpu_dm_helpers: Move > SYNAPTICS_DEVICE_ID into CONFIG_DRM_AMD_DC_DCN ifdef"). So, let's enable > these checks unconditionally for the entire module to catch these errors > during development. > > Cc: Alex Deucher <alexander.deucher@amd.com> > Cc: Nathan Chancellor <nathan@kernel.org> > Signed-off-by: Hamza Mahfooz <hamza.mahfooz@amd.com> I think this is fine, especially since it will help catch issues in amdgpu quickly and hopefully encourage developers to fix their problems before they make it to a tree with wider impact lika -next. However, this is now the third place that W=1 has been effectively enabled (i915 and btrfs are the other two I know of) and it would be nice if this was a little more unified, especially since it is not uncommon for the warnings under W=1 to shift around and keeping them unified will make maintainence over the longer term a little easier. I am not sure if this has been brought up in the past and I don't want to hold up this change but I suspect this sentiment of wanting to enable W=1 on a per-subsystem basis is going to continue to grow. Regardless, for clang 11.1.0 to 16.0.5, I see no warnings when building drivers/gpu/drm/amd/amdgpu/ with Arch Linux's configuration or allmodconfig. Reviewed-by: Nathan Chancellor <nathan@kernel.org> Tested-by: Nathan Chancellor <nathan@kernel.org> > --- > drivers/gpu/drm/amd/amdgpu/Makefile | 13 ++++++++++++- > 1 file changed, 12 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/amd/amdgpu/Makefile b/drivers/gpu/drm/amd/amdgpu/Makefile > index 86b833085f19..8d16f280b695 100644 > --- a/drivers/gpu/drm/amd/amdgpu/Makefile > +++ b/drivers/gpu/drm/amd/amdgpu/Makefile > @@ -40,7 +40,18 @@ ccflags-y := -I$(FULL_AMD_PATH)/include/asic_reg \ > -I$(FULL_AMD_PATH)/amdkfd > > subdir-ccflags-y := -Wextra > -subdir-ccflags-y += $(call cc-option, -Wunused-but-set-variable) > +subdir-ccflags-y += -Wunused > +subdir-ccflags-y += -Wmissing-prototypes > +subdir-ccflags-y += -Wmissing-declarations > +subdir-ccflags-y += -Wmissing-include-dirs > +subdir-ccflags-y += -Wold-style-definition > +subdir-ccflags-y += -Wmissing-format-attribute > +# Need this to avoid recursive variable evaluation issues > +cond-flags := $(call cc-option, -Wunused-but-set-variable) \ > + $(call cc-option, -Wunused-const-variable) \ > + $(call cc-option, -Wstringop-truncation) \ > + $(call cc-option, -Wpacked-not-aligned) > +subdir-ccflags-y += $(cond-flags) > subdir-ccflags-y += -Wno-unused-parameter > subdir-ccflags-y += -Wno-type-limits > subdir-ccflags-y += -Wno-sign-compare > -- > 2.40.1 > ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] drm/amd/amdgpu: enable W=1 for amdgpu 2023-06-09 20:17 ` Nathan Chancellor @ 2023-06-10 1:20 ` Masahiro Yamada 2023-06-10 6:49 ` Jani Nikula 0 siblings, 1 reply; 4+ messages in thread From: Masahiro Yamada @ 2023-06-10 1:20 UTC (permalink / raw) To: Nathan Chancellor Cc: Hamza Mahfooz, amd-gfx, Alex Deucher, Christian König, Pan, Xinhui, David Airlie, Daniel Vetter, Hawking Zhang, Le Ma, Tao Zhou, YiPeng Chai, James Zhu, Xiaojian Du, Lijo Lazar, dri-devel, linux-kernel, linux-kbuild On Sat, Jun 10, 2023 at 5:17 AM Nathan Chancellor <nathan@kernel.org> wrote: > > + Masahiro and linux-kbuild > > On Fri, Jun 09, 2023 at 12:42:06PM -0400, Hamza Mahfooz wrote: > > We have a clean build with W=1 as of > > commit 12a15dd589ac ("drm/amd/display/amdgpu_dm/amdgpu_dm_helpers: Move > > SYNAPTICS_DEVICE_ID into CONFIG_DRM_AMD_DC_DCN ifdef"). So, let's enable > > these checks unconditionally for the entire module to catch these errors > > during development. > > > > Cc: Alex Deucher <alexander.deucher@amd.com> > > Cc: Nathan Chancellor <nathan@kernel.org> > > Signed-off-by: Hamza Mahfooz <hamza.mahfooz@amd.com> > > I think this is fine, especially since it will help catch issues in > amdgpu quickly and hopefully encourage developers to fix their problems > before they make it to a tree with wider impact lika -next. > > However, this is now the third place that W=1 has been effectively > enabled (i915 and btrfs are the other two I know of) and it would be > nice if this was a little more unified, especially since it is not > uncommon for the warnings under W=1 to shift around and keeping them > unified will make maintainence over the longer term a little easier. I > am not sure if this has been brought up in the past and I don't want to > hold up this change but I suspect this sentiment of wanting to enable > W=1 on a per-subsystem basis is going to continue to grow. I believe this patch is the right way because we will be able to add a new warning option to scripts/Makefile.extrawarn without fixing any code. I remember somebody argued that drivers should be able to do subdir-ccflags-y += $(W1_FLAGS) However, if a new flag, -Wfoo, emits warnings for drivers/gpu/drm/{i915,amd}, you cannot add it to W=1 until fixing the code. If many drivers start to do likewise, W=1 warning will not be W=1 any more. Another good thing for hard-coding warning options is you can lift up a warning flag one by one. Let's say you fixed the entire DRM subsystem so it is -Wunused free now. Then, you can move -Wunused to drivers/gpu/drm/Makefile, while other warning options stay in drivers Makefiles. > > Regardless, for clang 11.1.0 to 16.0.5, I see no warnings when building > drivers/gpu/drm/amd/amdgpu/ with Arch Linux's configuration or > allmodconfig. > > Reviewed-by: Nathan Chancellor <nathan@kernel.org> > Tested-by: Nathan Chancellor <nathan@kernel.org> > > > --- > > drivers/gpu/drm/amd/amdgpu/Makefile | 13 ++++++++++++- > > 1 file changed, 12 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/gpu/drm/amd/amdgpu/Makefile b/drivers/gpu/drm/amd/amdgpu/Makefile > > index 86b833085f19..8d16f280b695 100644 > > --- a/drivers/gpu/drm/amd/amdgpu/Makefile > > +++ b/drivers/gpu/drm/amd/amdgpu/Makefile > > @@ -40,7 +40,18 @@ ccflags-y := -I$(FULL_AMD_PATH)/include/asic_reg \ > > -I$(FULL_AMD_PATH)/amdkfd > > > > subdir-ccflags-y := -Wextra > > -subdir-ccflags-y += $(call cc-option, -Wunused-but-set-variable) > > +subdir-ccflags-y += -Wunused > > +subdir-ccflags-y += -Wmissing-prototypes > > +subdir-ccflags-y += -Wmissing-declarations > > +subdir-ccflags-y += -Wmissing-include-dirs > > +subdir-ccflags-y += -Wold-style-definition > > +subdir-ccflags-y += -Wmissing-format-attribute > > +# Need this to avoid recursive variable evaluation issues > > +cond-flags := $(call cc-option, -Wunused-but-set-variable) \ > > + $(call cc-option, -Wunused-const-variable) \ > > + $(call cc-option, -Wstringop-truncation) \ > > + $(call cc-option, -Wpacked-not-aligned) > > +subdir-ccflags-y += $(cond-flags) > > subdir-ccflags-y += -Wno-unused-parameter > > subdir-ccflags-y += -Wno-type-limits > > subdir-ccflags-y += -Wno-sign-compare > > -- > > 2.40.1 > > -- Best Regards Masahiro Yamada ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] drm/amd/amdgpu: enable W=1 for amdgpu 2023-06-10 1:20 ` Masahiro Yamada @ 2023-06-10 6:49 ` Jani Nikula 0 siblings, 0 replies; 4+ messages in thread From: Jani Nikula @ 2023-06-10 6:49 UTC (permalink / raw) To: Masahiro Yamada, Nathan Chancellor Cc: Tao Zhou, Xiaojian Du, linux-kbuild, dri-devel, Pan, Xinhui, linux-kernel, amd-gfx, Lijo Lazar, Le Ma, YiPeng Chai, Hamza Mahfooz, Alex Deucher, James Zhu, Christian König, Hawking Zhang On Sat, 10 Jun 2023, Masahiro Yamada <masahiroy@kernel.org> wrote: > On Sat, Jun 10, 2023 at 5:17 AM Nathan Chancellor <nathan@kernel.org> wrote: >> >> + Masahiro and linux-kbuild >> >> On Fri, Jun 09, 2023 at 12:42:06PM -0400, Hamza Mahfooz wrote: >> > We have a clean build with W=1 as of >> > commit 12a15dd589ac ("drm/amd/display/amdgpu_dm/amdgpu_dm_helpers: Move >> > SYNAPTICS_DEVICE_ID into CONFIG_DRM_AMD_DC_DCN ifdef"). So, let's enable >> > these checks unconditionally for the entire module to catch these errors >> > during development. >> > >> > Cc: Alex Deucher <alexander.deucher@amd.com> >> > Cc: Nathan Chancellor <nathan@kernel.org> >> > Signed-off-by: Hamza Mahfooz <hamza.mahfooz@amd.com> >> >> I think this is fine, especially since it will help catch issues in >> amdgpu quickly and hopefully encourage developers to fix their problems >> before they make it to a tree with wider impact lika -next. >> >> However, this is now the third place that W=1 has been effectively >> enabled (i915 and btrfs are the other two I know of) and it would be >> nice if this was a little more unified, especially since it is not >> uncommon for the warnings under W=1 to shift around and keeping them >> unified will make maintainence over the longer term a little easier. I >> am not sure if this has been brought up in the past and I don't want to >> hold up this change but I suspect this sentiment of wanting to enable >> W=1 on a per-subsystem basis is going to continue to grow. > > > > I believe this patch is the right way because > we will be able to add a new warning option to > scripts/Makefile.extrawarn without fixing any code. > > I remember somebody argued that drivers should be > able to do > subdir-ccflags-y += $(W1_FLAGS) Personally, I think this would be the viable way to make the kernel free of W=1 warnings. Make it clean driver and subsystem at a time, with constant progress. Currently, there's haphazard fixing of issues, with new ones creeping back in, because kernel-wide W=1 is too verbose for most developers. It's whac-a-mole. > However, if a new flag, -Wfoo, emits warnings > for drivers/gpu/drm/{i915,amd}, > you cannot add it to W=1 until fixing the code. Or adding -Wno-foo where it breaks, until fixed. > If many drivers start to do likewise, > W=1 warning will not be W=1 any more. I don't know, is the goal to fix the warnings, or keep adding stuff to W=1 so that it'll always emit warnings? :p BR, Jani. > Another good thing for hard-coding warning options > is you can lift up a warning flag one by one. > > > Let's say you fixed the entire DRM subsystem so > it is -Wunused free now. > > Then, you can move -Wunused to drivers/gpu/drm/Makefile, > while other warning options stay in drivers Makefiles. > > > > > > > >> >> Regardless, for clang 11.1.0 to 16.0.5, I see no warnings when building >> drivers/gpu/drm/amd/amdgpu/ with Arch Linux's configuration or >> allmodconfig. >> >> Reviewed-by: Nathan Chancellor <nathan@kernel.org> >> Tested-by: Nathan Chancellor <nathan@kernel.org> >> >> > --- >> > drivers/gpu/drm/amd/amdgpu/Makefile | 13 ++++++++++++- >> > 1 file changed, 12 insertions(+), 1 deletion(-) >> > >> > diff --git a/drivers/gpu/drm/amd/amdgpu/Makefile b/drivers/gpu/drm/amd/amdgpu/Makefile >> > index 86b833085f19..8d16f280b695 100644 >> > --- a/drivers/gpu/drm/amd/amdgpu/Makefile >> > +++ b/drivers/gpu/drm/amd/amdgpu/Makefile >> > @@ -40,7 +40,18 @@ ccflags-y := -I$(FULL_AMD_PATH)/include/asic_reg \ >> > -I$(FULL_AMD_PATH)/amdkfd >> > >> > subdir-ccflags-y := -Wextra >> > -subdir-ccflags-y += $(call cc-option, -Wunused-but-set-variable) >> > +subdir-ccflags-y += -Wunused >> > +subdir-ccflags-y += -Wmissing-prototypes >> > +subdir-ccflags-y += -Wmissing-declarations >> > +subdir-ccflags-y += -Wmissing-include-dirs >> > +subdir-ccflags-y += -Wold-style-definition >> > +subdir-ccflags-y += -Wmissing-format-attribute >> > +# Need this to avoid recursive variable evaluation issues >> > +cond-flags := $(call cc-option, -Wunused-but-set-variable) \ >> > + $(call cc-option, -Wunused-const-variable) \ >> > + $(call cc-option, -Wstringop-truncation) \ >> > + $(call cc-option, -Wpacked-not-aligned) >> > +subdir-ccflags-y += $(cond-flags) >> > subdir-ccflags-y += -Wno-unused-parameter >> > subdir-ccflags-y += -Wno-type-limits >> > subdir-ccflags-y += -Wno-sign-compare >> > -- >> > 2.40.1 >> > -- Jani Nikula, Intel Open Source Graphics Center ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2023-06-10 6:49 UTC | newest] Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2023-06-09 16:42 [PATCH] drm/amd/amdgpu: enable W=1 for amdgpu Hamza Mahfooz 2023-06-09 20:17 ` Nathan Chancellor 2023-06-10 1:20 ` Masahiro Yamada 2023-06-10 6:49 ` Jani Nikula
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).