[Intel-gfx] [RFC] drm: enable W=1 warnings by default across the subsystem

[Jani Nikula]

On Wed, 29 Nov 2023, Hamza Mahfooz <hamza.mahfooz at amd.com> wrote:
> Cc: Nathan Chancellor <nathan at kernel.org>
>
> On 11/29/23 13:12, Jani Nikula wrote:
>> At least the i915 and amd drivers enable a bunch more compiler warnings
>> than the kernel defaults.
>> 
>> Extend the W=1 warnings to the entire drm subsystem by default. Use the
>> copy-pasted warnings from scripts/Makefile.extrawarn with
>> s/KBUILD_CFLAGS/subdir-ccflags-y/ to make it easier to compare and keep
>> up with them in the future.
>> 
>> This is similar to the approach currently used in i915.
>> 
>> Some of the -Wextra warnings do need to be disabled, just like in
>> Makefile.extrawarn, but take care to not disable them for W=2 or W=3
>> builds, depending on the warning.
>
> I think this should go in after drm-misc-next has a clean build (for
> COMPILE_TEST builds) with this patch applied. Otherwise, it will break a
> lot of build configs.

Oh, I'm absolutely not suggesting this should be merged before known
warnings have been addressed one way or another. Either by fixing them
or by disabling said warning in driver local Makefiles, depending on the
case.

While I'm suggesting this, I obviously (I hope) can't take on fixing all
the warnings this exposes. One way to scale would be for folks to apply
this locally, see what it uncovers in their drivers, and fix some.

As an intermediate step we might also apply this to a topic branch in
drm-tip, so you'd see the warnings when building drm-tip, but not in
indidual branches or in anything that's merged upstream.

BR,
Jani.


>
>> 
>> Cc: David Airlie <airlied at gmail.com>
>> Cc: Daniel Vetter <daniel at ffwll.ch>
>> Cc: Maarten Lankhorst <maarten.lankhorst at linux.intel.com>
>> Cc: Maxime Ripard <mripard at kernel.org>
>> Cc: Thomas Zimmermann <tzimmermann at suse.de>
>> Cc: Alex Deucher <alexander.deucher at amd.com>
>> Cc: Christian König <christian.koenig at amd.com>
>> Cc: Pan, Xinhui <Xinhui.Pan at amd.com>
>> Cc: Karol Herbst <kherbst at redhat.com>
>> Cc: Lyude Paul <lyude at redhat.com>
>> Cc: Danilo Krummrich <dakr at redhat.com>
>> Cc: Rob Clark <robdclark at gmail.com>
>> Cc: Abhinav Kumar <quic_abhinavk at quicinc.com>
>> Cc: Dmitry Baryshkov <dmitry.baryshkov at linaro.org>
>> Cc: Sean Paul <sean at poorly.run>
>> Cc: Marijn Suijten <marijn.suijten at somainline.org>
>> Signed-off-by: Jani Nikula <jani.nikula at intel.com>
>> 
>> ---
>> 
>> With my admittedly limited and very much x86 focused kernel config, I
>> get some -Wunused-but-set-variable and -Wformat-truncation= warnings,
>> but nothing we can't handle.
>> 
>> We could fix them up front, or disable the extra warnings on a per
>> driver basis with a FIXME comment in their respective Makefiles.
>> 
>> With the experience from i915, I think this would significantly reduce
>> the constant loop of warnings added by people not using W=1 and
>> subsequently fixed by people using W=1.
>> 
>> Note: I've Cc'd the maintainers of drm, drm misc and some of the biggest
>> drivers.
>> ---
>>   drivers/gpu/drm/Makefile | 27 +++++++++++++++++++++++++++
>>   1 file changed, 27 insertions(+)
>> 
>> diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile
>> index b4cb0835620a..6939e4ea13d5 100644
>> --- a/drivers/gpu/drm/Makefile
>> +++ b/drivers/gpu/drm/Makefile
>> @@ -5,6 +5,33 @@
>>   
>>   CFLAGS-$(CONFIG_DRM_USE_DYNAMIC_DEBUG)	+= -DDYNAMIC_DEBUG_MODULE
>>   
>> +# Unconditionally enable W=1 warnings locally
>> +# --- begin copy-paste W=1 warnings from scripts/Makefile.extrawarn
>> +subdir-ccflags-y += -Wextra -Wunused -Wno-unused-parameter
>> +subdir-ccflags-y += -Wmissing-declarations
>> +subdir-ccflags-y += $(call cc-option, -Wrestrict)
>> +subdir-ccflags-y += -Wmissing-format-attribute
>> +subdir-ccflags-y += -Wmissing-prototypes
>> +subdir-ccflags-y += -Wold-style-definition
>> +subdir-ccflags-y += -Wmissing-include-dirs
>> +subdir-ccflags-y += $(call cc-option, -Wunused-but-set-variable)
>> +subdir-ccflags-y += $(call cc-option, -Wunused-const-variable)
>> +subdir-ccflags-y += $(call cc-option, -Wpacked-not-aligned)
>> +subdir-ccflags-y += $(call cc-option, -Wformat-overflow)
>> +subdir-ccflags-y += $(call cc-option, -Wformat-truncation)
>> +subdir-ccflags-y += $(call cc-option, -Wstringop-overflow)
>> +subdir-ccflags-y += $(call cc-option, -Wstringop-truncation)
>> +# The following turn off the warnings enabled by -Wextra
>> +ifeq ($(findstring 2, $(KBUILD_EXTRA_WARN)),)
>> +subdir-ccflags-y += -Wno-missing-field-initializers
>> +subdir-ccflags-y += -Wno-type-limits
>> +subdir-ccflags-y += -Wno-shift-negative-value
>> +endif
>> +ifeq ($(findstring 3, $(KBUILD_EXTRA_WARN)),)
>> +subdir-ccflags-y += -Wno-sign-compare
>> +endif
>> +# --- end copy-paste
>> +
>>   drm-y := \
>>   	drm_aperture.o \
>>   	drm_atomic.o \

-- 
Jani Nikula, Intel