LKML Archive mirror
 help / color / mirror / Atom feed

* [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).