Re: [PATCH 0/8] drm: fix and enable warnings on unused static inlines
On Tue, Sep 10, 2024 at 01:03:36PM +0300, Jani Nikula wrote: > Follow-up to [1]. > > Annotate unused static inlines with __maybe_unused. In some cases it > might be better to remove them, but it's really up to the maintainers > what to do. Then enable the warning on default across subsystem. I merged drm-misc-next into next-20240910 and applied this series on top of the result and it looks like all of the instances are fixed. Tested-by: Nathan Chancellor # build Cheers, Nathan
Re: (subset) [PATCH v7 00/16] Add audio support for the MediaTek Genio 350-evk board
On Fri, Sep 06, 2024 at 07:27:01PM +0100, Mark Brown wrote: > Are these just warnings introduced by recent versions of the toolchains? > These commits passed an x86 allmodconfig with GCC 12 at each step (I did > catch one warning there with another patch in the series that didn't get > applied) and 0day didn't say anything at any point. Not sure, I did not look too hard. At cursory glance, I am not sure x86 allmodconfig would catch these, as this code depends on ARCH_MEDIATEK (not COMPILE_TEST), which only exists for arm and arm64. > > Clang 19: > > That's relatively modern, though some of the warnings don't look > particularly new and exciting. Fair although I still see some of them on old versions too: https://github.com/ClangBuiltLinux/continuous-integration2/actions/runs/10738441894 > > sound/soc/mediatek/mt8365/mt8365-dai-adda.c:93:8: error: implicit > > conversion from 'unsigned long' to 'unsigned int' changes value from > > 18446744073709551614 to 4294967294 [-Werror,-Wconstant-conversion] > > 91 | regmap_update_bits(afe->regmap, > > AFE_ADDA_UL_DL_CON0, > > | ~~ > > 92 |AFE_ADDA_UL_DL_ADDA_AFE_ON, > > 93 |~AFE_ADDA_UL_DL_ADDA_AFE_ON); > > |^~~ > > 1 error generated. > > That's a bit surprising, regmap_update_bits() takes an unsigned long? I > suspect the constants need to be defined as unsigned. Does it? I see it taking 'unsigned int' for all of its parameters. $ sed -n '1242,1250p' include/linux/regmap.h int regmap_update_bits_base(struct regmap *map, unsigned int reg, unsigned int mask, unsigned int val, bool *change, bool async, bool force); static inline int regmap_update_bits(struct regmap *map, unsigned int reg, unsigned int mask, unsigned int val) { return regmap_update_bits_base(map, reg, mask, val, NULL, false, false); } Cheers, Nathan
Re: (subset) [PATCH v7 00/16] Add audio support for the MediaTek Genio 350-evk board
On Wed, Sep 04, 2024 at 12:16:48PM +0100, Mark Brown wrote: > [01/16] ASoC: dt-bindings: mediatek,mt8365-afe: Add audio afe document > commit: ceb3ca2876243e3ea02f78b3d488b1f2d734de49 > [02/16] ASoC: dt-bindings: mediatek,mt8365-mt6357: Add audio sound card > document > commit: 76d80dcdd55f70b28930edb97b96ee375e1cce5a > [03/16] dt-bindings: mfd: mediatek: Add codec property for MT6357 PMIC > commit: 761cab667898d86c04867948f1b7aec1090be796 > [04/16] ASoC: mediatek: mt8365: Add common header > commit: 38c7c9ddc74033406461d64e541bbc8268e77f73 > [05/16] ASoC: mediatek: mt8365: Add audio clock control support > commit: ef307b40b7f0042d54f020bccb3e728ced292282 > [06/16] ASoC: mediatek: mt8365: Add I2S DAI support > commit: 402bbb13a195caa83b3279ebecdabfb11ddee084 > [07/16] ASoC: mediatek: mt8365: Add ADDA DAI support > commit: 7c58c88e524180e8439acdfc44872325e7f6d33d > [08/16] ASoC: mediatek: mt8365: Add DMIC DAI support > commit: 1c50ec75ce6c0c6b5736499393e522f73e19d0cf > [09/16] ASoC: mediatek: mt8365: Add PCM DAI support > commit: 5097c0c8634d703e3c59cfb89831b7db9dc46339 > [10/16] ASoc: mediatek: mt8365: Add a specific soundcard for EVK > commit: 1bf6dbd75f7603dd026660bebf324f812200dc1b > [11/16] ASoC: mediatek: mt8365: Add the AFE driver support > commit: e1991d102bc2abb32331c462f8f3e77059c69578 I am seeing several warnings/errors from both GCC and Clang with ARCH=arm64 allmodconfig after this series appeared in next-20240906. As far as I can tell, they appear to agree. I wondered how this was not caught during the series development but perhaps it was written against a development tree that did not have Arnd's extrawarn series from 6.10 in it yet? I was going to work on a series but I was not sure about the best way to address the overflow errors, hence just the report. Clang 19: sound/soc/mediatek/mt8365/mt8365-afe-clk.c:298:5: error: no previous prototype for function 'mt8365_afe_hd_engen_enable' [-Werror,-Wmissing-prototypes] 298 | int mt8365_afe_hd_engen_enable(struct mtk_base_afe *afe, bool apll1) | ^ sound/soc/mediatek/mt8365/mt8365-afe-clk.c:298:1: note: declare 'static' if the function is not intended to be used outside of this translation unit 298 | int mt8365_afe_hd_engen_enable(struct mtk_base_afe *afe, bool apll1) | ^ | static sound/soc/mediatek/mt8365/mt8365-afe-clk.c:310:5: error: no previous prototype for function 'mt8365_afe_hd_engen_disable' [-Werror,-Wmissing-prototypes] 310 | int mt8365_afe_hd_engen_disable(struct mtk_base_afe *afe, bool apll1) | ^ sound/soc/mediatek/mt8365/mt8365-afe-clk.c:310:1: note: declare 'static' if the function is not intended to be used outside of this translation unit 310 | int mt8365_afe_hd_engen_disable(struct mtk_base_afe *afe, bool apll1) | ^ | static sound/soc/mediatek/mt8365/mt8365-afe-clk.c:314:24: error: implicit conversion from 'unsigned long' to 'unsigned int' changes value from 18446744073709551614 to 4294967294 [-Werror,-Wconstant-conversion] 313 | regmap_update_bits(afe->regmap, AFE_HD_ENGEN_ENABLE, | ~~ 314 |AFE_22M_PLL_EN, ~AFE_22M_PLL_EN); |^~~ sound/soc/mediatek/mt8365/mt8365-afe-clk.c:317:24: error: implicit conversion from 'unsigned long' to 'unsigned int' changes value from 18446744073709551613 to 4294967293 [-Werror,-Wconstant-conversion] 316 | regmap_update_bits(afe->regmap, AFE_HD_ENGEN_ENABLE, | ~~ 317 |AFE_24M_PLL_EN, ~AFE_24M_PLL_EN); |^~~ 4 errors generated. sound/soc/mediatek/mt8365/mt8365-dai-adda.c:93:8: error: implicit conversion from 'unsigned long' to 'unsigned int' changes value from 18446744073709551614 to 4294967294 [-Werror,-Wconstant-conversion] 91 | regmap_update_bits(afe->regmap, AFE_ADDA_UL_DL_CON0, | ~~ 92 |AFE_ADDA_UL_DL_ADDA_AFE_ON, 93 |~AFE_ADDA_UL_DL_ADDA_AFE_ON); |^~~ 1 error generated. sound/soc/mediatek/mt8365/mt8365-mt6357.c:293:22: error: unused variable 'platform_node' [-Werror,-Wunused-variable] 293 | struct device_node *platform_node; | ^ sound/soc/mediatek/mt8365/mt8365-mt6357.c:295:6: error: unused variable 'i' [-Werror,-Wunused-variable] 295 | int i, ret; | ^ 2 errors generated. sound/soc/mediatek/mt8365/mt8365-dai-dmic.c:64:7: error: implicit conversion from 'unsi
Re: [PATCH] drm: enable warnings on unused static inlines
Hi Jani, On Wed, Sep 04, 2024 at 03:38:19PM +0300, Jani Nikula wrote: > We enable most W=1 warnings by default subsystem wide. Also enable > warnings on unused static inlines when building with clang. > > See also commit 6863f5643dd7 ("kbuild: allow Clang to find unused static > inline functions for W=1 build"). > > Cc: Nathan Chancellor > Signed-off-by: Jani Nikula > --- > drivers/gpu/drm/Makefile | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile > index 784229d4504d..6bd2cdb08be7 100644 > --- a/drivers/gpu/drm/Makefile > +++ b/drivers/gpu/drm/Makefile > @@ -19,6 +19,9 @@ subdir-ccflags-y += $(call cc-option, -Wformat-overflow) > # FIXME: fix -Wformat-truncation warnings and uncomment > #subdir-ccflags-y += $(call cc-option, -Wformat-truncation) > subdir-ccflags-y += $(call cc-option, -Wstringop-truncation) > + > +subdir-ccflags-y += -DKBUILD_EXTRA_WARN1 > + > # The following turn off the warnings enabled by -Wextra > ifeq ($(findstring 2, $(KBUILD_EXTRA_WARN)),) > subdir-ccflags-y += -Wno-missing-field-initializers > -- > 2.39.2 > I ran this through my test matrix and this is what it found (across various configuration options, I can give specifics as necessary): drivers/gpu/drm/bridge/synopsys/dw-hdmi-i2s-audio.c:30:18: warning: unused function 'hdmi_read' [-Wunused-function] 30 | static inline u8 hdmi_read(struct dw_hdmi_i2s_audio_data *audio, int offset) | ^ drivers/gpu/drm/bridge/ti-sn65dsi86.c:1638:19: warning: unused function 'ti_sn_pwm_pin_request' [-Wunused-function] 1638 | static inline int ti_sn_pwm_pin_request(struct ti_sn65dsi86 *pdata) { return 0; } | ^ drivers/gpu/drm/bridge/ti-sn65dsi86.c:1639:20: warning: unused function 'ti_sn_pwm_pin_release' [-Wunused-function] 1639 | static inline void ti_sn_pwm_pin_release(struct ti_sn65dsi86 *pdata) {} |^ drivers/gpu/drm/drm_mm.c:152:1: warning: unused function 'drm_mm_interval_tree_insert' [-Wunused-function] 152 | INTERVAL_TREE_DEFINE(struct drm_mm_node, rb, | ^~~~ 153 | u64, __subtree_last, | 154 | START, LAST, static inline, drm_mm_interval_tree) | ~ include/linux/interval_tree_generic.h:38:15: note: expanded from macro 'INTERVAL_TREE_DEFINE' 38 | ITSTATIC void ITPREFIX ## _insert(ITSTRUCT *node, \ | ^~~ :38:1: note: expanded from here 38 | drm_mm_interval_tree_insert | ^~~ drivers/gpu/drm/drm_mm.c:152:1: warning: unused function 'drm_mm_interval_tree_iter_next' [-Wunused-function] 152 | INTERVAL_TREE_DEFINE(struct drm_mm_node, rb, | ^~~~ 153 | u64, __subtree_last, | 154 | START, LAST, static inline, drm_mm_interval_tree) | ~ include/linux/interval_tree_generic.h:151:33: note: expanded from macro 'INTERVAL_TREE_DEFINE' 151 | ITSTATIC ITSTRUCT * \ | ^ 152 | ITPREFIX ## _iter_next(ITSTRUCT *node, ITTYPE start, ITTYPE last) \ | ~~ :50:1: note: expanded from here 50 | drm_mm_interval_tree_iter_next | ^~ drivers/gpu/drm/drm_mm.c:614:20: warning: function 'drm_mm_node_scanned_block' is not needed and will not be emitted [-Wunneeded-internal-declaration] 614 | static inline bool drm_mm_node_scanned_block(const struct drm_mm_node *node) |^ drivers/gpu/drm/i915/i915_sw_fence.c:97:20: error: unused function 'debug_fence_init_onstack' [-Werror,-Wunused-function] 97 | static inline void debug_fence_init_onstack(struct i915_sw_fence *fence) |^~~~ drivers/gpu/drm/i915/i915_sw_fence.c:118:20: error: unused function 'debug_fence_free' [-Werror,-Wunused-function] 118 | static inline void debug_fence_free(struct i915_sw_fence *fence) |^~~~ drivers/gpu/drm/imagination/pvr_drv.c:224:1: warning: unused function 'pvr_fw_version_packed' [-Wunused-function] 224 | pvr_fw_version_packed(u32 major, u32 minor) | ^ driv
Re: [PATCH v1 0/2] drm/i915/fence: A couple of build fixes
On Thu, Aug 29, 2024 at 09:37:34PM +0300, Jani Nikula wrote: > On Thu, 29 Aug 2024, Nathan Chancellor wrote: > > Since commit 6863f5643dd7 ("kbuild: allow Clang to find unused static > > inline functions for W=1 build"). clang warns about unused static inline > > functions in .c files, unlike GCC (they both do not warn for functions > > coming from .h files). This difference is worked around for the normal > > build by adding '__maybe_unused' to the definition of 'inline' but > > Masahiro wanted to disable it for W=1 to allow this difference to find > > unused/dead code. There have not been too many complaints as far as I am > > aware but I can see how it is surprising. > > Heh, I was just going to reply citing the same commit. > > I occasionally build with clang myself, and we do enable most W=1 by > default in the drm subsystem, so I was wondering why I hadn't hit > this. The crucial difference is that we lack -DKBUILD_EXTRA_WARN1 which > W=1 adds. > > I see there's no subdir-cppflags-y, but I don't see any harm in us > adding -Wundef and -DKBUILD_EXTRA_WARN1 to subdir-ccflags-y. After we > fix the fallout, of course. Do you? No, that seems entirely reasonable when your goal is to enable W=1 for your subsystem. > I don't much like the __maybe_unused stuff, but I guess it's fine as a > stopgap measure, and then we can grep for that when running out of > things to do. :p Perhaps worth a TODO or something? Maybe a kernel newbie could work on that at some point if it is not high enough priority. Cheers, Nathan
Re: [PATCH v1 0/2] drm/i915/fence: A couple of build fixes
On Thu, Aug 29, 2024 at 09:10:41PM +0300, Andy Shevchenko wrote: > On Thu, Aug 29, 2024 at 07:53:25PM +0300, Andy Shevchenko wrote: > > On Thu, Aug 29, 2024 at 07:38:08PM +0300, Jani Nikula wrote: > > > On Thu, 29 Aug 2024, Andy Shevchenko > > > wrote: > > > > With CONFIG_WERROR=y and `make W=1` build fails on my x86_64 machine. > > > > This is due to some unused functions. Hence these quick fixes. > > > > > > Since when have we been getting the warnings for static inlines? Since commit 6863f5643dd7 ("kbuild: allow Clang to find unused static inline functions for W=1 build"). clang warns about unused static inline functions in .c files, unlike GCC (they both do not warn for functions coming from .h files). This difference is worked around for the normal build by adding '__maybe_unused' to the definition of 'inline' but Masahiro wanted to disable it for W=1 to allow this difference to find unused/dead code. There have not been too many complaints as far as I am aware but I can see how it is surprising. Cheers, Nathan
[PATCH] drm/xe: Fix total initialization in xe_ggtt_print_holes()
Clang warns (or errors with CONFIG_DRM_WERROR or CONFIG_WERROR): drivers/gpu/drm/xe/xe_ggtt.c:810:3: error: variable 'total' is uninitialized when used here [-Werror,-Wuninitialized] 810 | total += hole_size; | ^ drivers/gpu/drm/xe/xe_ggtt.c:798:11: note: initialize the variable 'total' to silence this warning 798 | u64 total; | ^ | = 0 1 error generated. Move the zero initialization of total from xe_gt_sriov_pf_config_print_available_ggtt() to xe_ggtt_print_holes() to resolve the warning. Fixes: 136367290ea5 ("drm/xe: Introduce xe_ggtt_print_holes") Signed-off-by: Nathan Chancellor --- drivers/gpu/drm/xe/xe_ggtt.c | 2 +- drivers/gpu/drm/xe/xe_gt_sriov_pf_config.c | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/gpu/drm/xe/xe_ggtt.c b/drivers/gpu/drm/xe/xe_ggtt.c index a3ce91decdce..86fc6afa43bd 100644 --- a/drivers/gpu/drm/xe/xe_ggtt.c +++ b/drivers/gpu/drm/xe/xe_ggtt.c @@ -795,7 +795,7 @@ u64 xe_ggtt_print_holes(struct xe_ggtt *ggtt, u64 alignment, struct drm_printer const struct drm_mm_node *entry; u64 hole_min_start = xe_wopcm_size(tile_to_xe(ggtt->tile)); u64 hole_start, hole_end, hole_size; - u64 total; + u64 total = 0; char buf[10]; mutex_lock(&ggtt->lock); diff --git a/drivers/gpu/drm/xe/xe_gt_sriov_pf_config.c b/drivers/gpu/drm/xe/xe_gt_sriov_pf_config.c index c8835d9eead6..41ed07b153b5 100644 --- a/drivers/gpu/drm/xe/xe_gt_sriov_pf_config.c +++ b/drivers/gpu/drm/xe/xe_gt_sriov_pf_config.c @@ -2110,7 +2110,7 @@ int xe_gt_sriov_pf_config_print_available_ggtt(struct xe_gt *gt, struct drm_prin { struct xe_ggtt *ggtt = gt_to_tile(gt)->mem.ggtt; u64 alignment = pf_get_ggtt_alignment(gt); - u64 spare, avail, total = 0; + u64 spare, avail, total; char buf[10]; xe_gt_assert(gt, IS_SRIOV_PF(gt_to_xe(gt))); --- base-commit: 66a0f6b9f5fc205272035b6ffa4830be51e3f787 change-id: 20240823-drm-xe-fix-total-in-xe_ggtt_print_holes-0cf2c399aa2a Best regards, -- Nathan Chancellor
[PATCH] drm/amd/display: Reapply 2fde4fdddc1f
Commit 2563391e57b5 ("drm/amd/display: DML2.1 resynchronization") blew away the compiler warning fix from commit 2fde4fdddc1f ("drm/amd/display: Avoid -Wenum-float-conversion in add_margin_and_round_to_dfs_grainularity()"), causing the warning to reappear. drivers/gpu/drm/amd/amdgpu/../display/dc/dml2/dml21/src/dml2_dpmm/dml2_dpmm_dcn4.c:183:58: error: arithmetic between enumeration type 'enum dentist_divider_range' and floating-point type 'double' [-Werror,-Wenum-float-conversion] 183 | divider = (unsigned int)(DFS_DIVIDER_RANGE_SCALE_FACTOR * (vco_freq_khz / clock_khz)); | ~~ ^ ~~ 1 error generated. Apply the fix again to resolve the warning. Fixes: 2563391e57b5 ("drm/amd/display: DML2.1 resynchronization") Signed-off-by: Nathan Chancellor --- .../gpu/drm/amd/display/dc/dml2/dml21/src/dml2_dpmm/dml2_dpmm_dcn4.c| 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/gpu/drm/amd/display/dc/dml2/dml21/src/dml2_dpmm/dml2_dpmm_dcn4.c b/drivers/gpu/drm/amd/display/dc/dml2/dml21/src/dml2_dpmm/dml2_dpmm_dcn4.c index 0021bbaa4b91..f19f6ebaae13 100644 --- a/drivers/gpu/drm/amd/display/dc/dml2/dml21/src/dml2_dpmm/dml2_dpmm_dcn4.c +++ b/drivers/gpu/drm/amd/display/dc/dml2/dml21/src/dml2_dpmm/dml2_dpmm_dcn4.c @@ -180,7 +180,7 @@ static bool add_margin_and_round_to_dfs_grainularity(double clock_khz, double ma clock_khz *= 1.0 + margin; - divider = (unsigned int)(DFS_DIVIDER_RANGE_SCALE_FACTOR * (vco_freq_khz / clock_khz)); + divider = (unsigned int)((int)DFS_DIVIDER_RANGE_SCALE_FACTOR * (vco_freq_khz / clock_khz)); /* we want to floor here to get higher clock than required rather than lower */ if (divider < DFS_DIVIDER_RANGE_2_START) { --- base-commit: bd4bea5ab2bda37ddb092a978218c4d9b46927e6 change-id: 20240724-amdgpu-dml2-fix-float-enum-warning-again-647a33789138 Best regards, -- Nathan Chancellor
Re: linux-next: build failure after merge of the drm tree
On Mon, Jul 01, 2024 at 07:13:19PM +1000, Stephen Rothwell wrote: > Hi all, > > After merging the drm tree, today's linux-next build (powerpc > allyesconfig) failed like this: > > In file included from arch/powerpc/include/asm/mmu.h:144, > from arch/powerpc/include/asm/paca.h:18, > from arch/powerpc/include/asm/current.h:13, > from include/linux/sched.h:12, > from include/linux/ratelimit.h:6, > from include/linux/dev_printk.h:16, > from include/linux/device.h:15, > from include/linux/dma-mapping.h:8, > from drivers/gpu/drm/omapdrm/omap_gem.c:7: > drivers/gpu/drm/omapdrm/omap_gem.c: In function 'omap_gem_pin_tiler': > arch/powerpc/include/asm/page.h:25:33: error: conversion from 'long unsigned > int' to 'u16' {aka 'short unsigned int'} changes value from '65536' to '0' > [-Werror=overflow] >25 | #define PAGE_SIZE (ASM_CONST(1) << PAGE_SHIFT) > | ^~~~ > drivers/gpu/drm/omapdrm/omap_gem.c:758:42: note: in expansion of macro > 'PAGE_SIZE' > 758 | PAGE_SIZE); > | ^ > drivers/gpu/drm/omapdrm/omap_gem.c: In function 'omap_gem_init': > arch/powerpc/include/asm/page.h:25:33: error: conversion from 'long unsigned > int' to 'u16' {aka 'short unsigned int'} changes value from '65536' to '0' > [-Werror=overflow] >25 | #define PAGE_SIZE (ASM_CONST(1) << PAGE_SHIFT) > | ^~~~ > drivers/gpu/drm/omapdrm/omap_gem.c:1504:65: note: in expansion of macro > 'PAGE_SIZE' > 1504 | block = tiler_reserve_2d(fmts[i], w, h, > PAGE_SIZE); > | > ^ > cc1: all warnings being treated as errors > > Exposed by commit > > dc6fcaaba5a5 ("drm/omap: Allow build with COMPILE_TEST=y") > > PowerPC 64 bit uses 64k pages. > > I have reverted that commit for today. FWIW, I sent a patch to address this in a bit of a more targeted manner over a week ago: https://lore.kernel.org/20240620-omapdrm-restrict-compile-test-to-sub-64kb-page-size-v1-1-5e56de71f...@kernel.org/ Although somehow, I left off Ville from that patch :/ Cheers, Nathan
[PATCH] drm/omap: Restrict compile testing to PAGE_SIZE less than 64KB
Prior to commit dc6fcaaba5a5 ("drm/omap: Allow build with COMPILE_TEST=y"), it was only possible to build the omapdrm driver with a 4KB page size. After that change, when the PAGE_SIZE is 64KB or larger, clang points out that the driver has some assumptions around the page size implicitly by passing PAGE_SIZE to a parameter with a type of u16: drivers/gpu/drm/omapdrm/omap_gem.c:758:7: error: implicit conversion from 'unsigned long' to 'u16' (aka 'unsigned short') changes value from 65536 to 0 [-Werror,-Wconstant-conversion] 757 | block = tiler_reserve_2d(fmt, omap_obj->width, omap_obj->height, | 758 | PAGE_SIZE); | ^ arch/powerpc/include/asm/page.h:25:34: note: expanded from macro 'PAGE_SIZE' 25 | #define PAGE_SIZE (ASM_CONST(1) << PAGE_SHIFT) | ~^ drivers/gpu/drm/omapdrm/omap_gem.c:1504:44: error: implicit conversion from 'unsigned long' to 'u16' (aka 'unsigned short') changes value from 65536 to 0 [-Werror,-Wconstant-conversion] 1504 | block = tiler_reserve_2d(fmts[i], w, h, PAGE_SIZE); | ^ arch/powerpc/include/asm/page.h:25:34: note: expanded from macro 'PAGE_SIZE' 25 | #define PAGE_SIZE (ASM_CONST(1) << PAGE_SHIFT) | ~^ 2 errors generated. As there is a lot of use of a u16 type throughout this driver and it will only ever be run on hardware that has a 4KB page size, just restrict compile testing to when the page size is less than 64KB (as no other issues have been discussed and it keeps compile testing relatively more available). Signed-off-by: Nathan Chancellor --- drivers/gpu/drm/omapdrm/Kconfig | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/gpu/drm/omapdrm/Kconfig b/drivers/gpu/drm/omapdrm/Kconfig index 85ed92042b74..3f7139e211d2 100644 --- a/drivers/gpu/drm/omapdrm/Kconfig +++ b/drivers/gpu/drm/omapdrm/Kconfig @@ -2,7 +2,7 @@ config DRM_OMAP tristate "OMAP DRM" depends on DRM && OF - depends on ARCH_OMAP2PLUS || COMPILE_TEST + depends on ARCH_OMAP2PLUS || (COMPILE_TEST && PAGE_SIZE_LESS_THAN_64KB) select DRM_KMS_HELPER select FB_DMAMEM_HELPERS_DEFERRED if DRM_FBDEV_EMULATION select VIDEOMODE_HELPERS --- base-commit: c62b4fc4b9b86ab35e5c4236f2053ce21ee81ebc change-id: 20240620-omapdrm-restrict-compile-test-to-sub-64kb-page-size-0c8d37e380bb Best regards, -- Nathan Chancellor
[PATCH] drm/amd/display: Disable CONFIG_DRM_AMD_DC_FP for RISC-V with clang
Commit 77acc6b55ae4 ("riscv: add support for kernel-mode FPU") and commit a28e4b672f04 ("drm/amd/display: use ARCH_HAS_KERNEL_FPU_SUPPORT") enabled support for CONFIG_DRM_AMD_DC_FP with RISC-V. Unfortunately, this exposed -Wframe-larger-than warnings (which become fatal with CONFIG_WERROR=y) when building ARCH=riscv allmodconfig with clang: drivers/gpu/drm/amd/amdgpu/../display/dc/dml/dcn32/display_mode_vba_32.c:58:13: error: stack frame size (2448) exceeds limit (2048) in 'DISPCLKDPPCLKDCFCLKDeepSleepPrefetchParametersWatermarksAndPerformanceCalculation' [-Werror,-Wframe-larger-than] 58 | static void DISPCLKDPPCLKDCFCLKDeepSleepPrefetchParametersWatermarksAndPerformanceCalculation( | ^ 1 error generated. Many functions in this file use a large number of parameters, which must be passed on the stack at a certain pointer due to register exhaustion, which can cause high stack usage when inlining and issues with stack slot analysis get involved. While the compiler can and should do better (as GCC uses less than half the amount of stack space for the same function), it is not as simple as a fix as adjusting the functions not to take a large number of parameters. Unfortunately, modifying these files to avoid the problem is a difficult to justify approach because any revisions to the files in the kernel tree never make it back to the original source (so copies of the code for newer hardware revisions just reintroduce the issue) and the files are hard to read/modify due to being "gcc-parsable HW gospel, coming straight from HW engineers". Avoid building the problematic code for RISC-V by modifying the existing condition for arm64 that exists for the same reason. Factor out the logical not to make the condition a little more readable naturally. Fixes: a28e4b672f04 ("drm/amd/display: use ARCH_HAS_KERNEL_FPU_SUPPORT") Reported-by: Palmer Dabbelt Closes: https://lore.kernel.org/20240530145741.7506-2-pal...@rivosinc.com/ Signed-off-by: Nathan Chancellor --- drivers/gpu/drm/amd/display/Kconfig | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/gpu/drm/amd/display/Kconfig b/drivers/gpu/drm/amd/display/Kconfig index 5fcd4f778dc3..47b8b49da8a7 100644 --- a/drivers/gpu/drm/amd/display/Kconfig +++ b/drivers/gpu/drm/amd/display/Kconfig @@ -8,7 +8,7 @@ config DRM_AMD_DC depends on BROKEN || !CC_IS_CLANG || ARM64 || RISCV || SPARC64 || X86_64 select SND_HDA_COMPONENT if SND_HDA_CORE # !CC_IS_CLANG: https://github.com/ClangBuiltLinux/linux/issues/1752 - select DRM_AMD_DC_FP if ARCH_HAS_KERNEL_FPU_SUPPORT && (!ARM64 || !CC_IS_CLANG) + select DRM_AMD_DC_FP if ARCH_HAS_KERNEL_FPU_SUPPORT && !(CC_IS_CLANG && (ARM64 || RISCV)) help Choose this option if you want to use the new display engine support for AMDGPU. This adds required support for Vega and --- base-commit: c6c4dd54012551cce5cde408b35468f2c62b0cce change-id: 20240614-amdgpu-disable-drm-amd-dc-fp-riscv-clang-31c84f6b990d Best regards, -- Nathan Chancellor
Re: [PATCH] drm/amd/display: Increase frame-larger-than warning limit
Hi Palmer (and AMD folks), On Tue, Jun 04, 2024 at 09:04:23AM -0700, Palmer Dabbelt wrote: > On Mon, 03 Jun 2024 15:29:48 PDT (-0700), nat...@kernel.org wrote: > > On Thu, May 30, 2024 at 07:57:42AM -0700, Palmer Dabbelt wrote: > > > From: Palmer Dabbelt > > > > > > I get a handful of build errors along the lines of > > > > > > > > > linux/drivers/gpu/drm/amd/amdgpu/../display/dc/dml/dcn32/display_mode_vba_32.c:58:13: > > > error: stack frame size (2352) exceeds limit (2048) in > > > 'DISPCLKDPPCLKDCFCLKDeepSleepPrefetchParametersWatermarksAndPerformanceCalculation' > > > [-Werror,-Wframe-larger-than] > > > static void > > > DISPCLKDPPCLKDCFCLKDeepSleepPrefetchParametersWatermarksAndPerformanceCalculation( > > > ^ > > > > > > linux/drivers/gpu/drm/amd/amdgpu/../display/dc/dml/dcn32/display_mode_vba_32.c:1724:6: > > > error: stack frame size (2096) exceeds limit (2048) in > > > 'dml32_ModeSupportAndSystemConfigurationFull' > > > [-Werror,-Wframe-larger-than] > > > void dml32_ModeSupportAndSystemConfigurationFull(struct > > > display_mode_lib *mode_lib) > > > ^ > > > > Judging from the message, this is clang/LLVM? What version? > > Yes, LLVM. Looks like I'm on 16.0.6. Probably time for an update, so I'll > give it a shot. FWIW, I can reproduce this with tip of tree, I was just curious in case that ended up mattering. > > I assume > > this showed up in 6.10-rc1 because of commit 77acc6b55ae4 ("riscv: add > > support for kernel-mode FPU"), which allows this driver to be built for > > RISC-V. > > Seems reasonable. This didn't show up until post-merge, not 100% sure why. > I didn't really dig any farther. Perhaps you fast forwarded your tree to include that commit? > > Is this allmodconfig or some other configuration? > > IIRC both "allmodconfig" and "allyesconfig" show it, but I don't have a > build tree sitting around to be 100% sure. Yeah, allmodconfig triggers it. I was able to come up with a "trivial" reproducer using cvise (attached to this mail if you are curious) that has worse stack usage by a rough factor of 2: $ clang --target=riscv64-linux-gnu -O2 -Wall -Wframe-larger-than=512 -c -o /dev/null display_mode_vba_32.i display_mode_vba_32.i:598:6: warning: stack frame size (1264) exceeds limit (512) in 'DISPCLKDPPCLKDCFCLKDeepSleepPrefetchParametersWatermarksAndPerformanceCalculation' [-Wframe-larger-than] 598 | void DISPCLKDPPCLKDCFCLKDeepSleepPrefetchParametersWatermarksAndPerformanceCalculation() { | ^ 1 warning generated. $ riscv64-linux-gcc -O2 -Wall -Wframe-larger-than=512 -c -o /dev/null display_mode_vba_32.i display_mode_vba_32.i: In function 'DISPCLKDPPCLKDCFCLKDeepSleepPrefetchParametersWatermarksAndPerformanceCalculation': display_mode_vba_32.i:1729:1: warning: the frame size of 528 bytes is larger than 512 bytes [-Wframe-larger-than=] 1729 | } | ^ I have not done too much further investigation but this is almost certainly the same issue that has come up before [1][2] with the AMD display code using functions with a large number of parameters, such that they have to passed on the stack, coupled with inlining (if I remember correctly, LLVM gives more of an inlining discount the less a function is used in a file). While clang does poorly with that code, I am not interested in continuing to fix this code new hardware revision after new hardware revision. We could just avoid this code like we do for arm64 for a similar reason: diff --git a/drivers/gpu/drm/amd/display/Kconfig b/drivers/gpu/drm/amd/display/Kconfig index 5fcd4f778dc3..64df713df878 100644 --- a/drivers/gpu/drm/amd/display/Kconfig +++ b/drivers/gpu/drm/amd/display/Kconfig @@ -8,7 +8,7 @@ config DRM_AMD_DC depends on BROKEN || !CC_IS_CLANG || ARM64 || RISCV || SPARC64 || X86_64 select SND_HDA_COMPONENT if SND_HDA_CORE # !CC_IS_CLANG: https://github.com/ClangBuiltLinux/linux/issues/1752 - select DRM_AMD_DC_FP if ARCH_HAS_KERNEL_FPU_SUPPORT && (!ARM64 || !CC_IS_CLANG) + select DRM_AMD_DC_FP if ARCH_HAS_KERNEL_FPU_SUPPORT && (!(ARM64 || RISCV) || !CC_IS_CLANG) help Choose this option if you want to use the new display engine support for AMDGPU. This adds required support for Vega and [1]: https://lore.kernel.org/20231019205117.GA839902@dev-arch.thelio-3990X/ [2]: https://lore.kernel.org/20220830203409.3491379-1-nat...@kernel.org/ Cheers, Nathan enum { false, true }; enum output_encoder_class { dm_dp2p0 }; enum output_format_class { dm_420 }; enum source_format_class { dm_444_32 }; enum scan_direction_class { dm_vert }; enum dm_swizzle_mode { dm_sw_linear }; enum clock_change_support { dm_std_cvt }; enum odm_combine_mode { dm_odm_combine_mode_2to1dm_odm_combine_mode_4to1 }; enum immediate_flip_requirement { dm_immediate_flip_not_required }; enum unbounded_requesting_policy { dm_unbounded_requesting_disable }; enum dm_rotation_angle { dm_rotation_270m };
Re: [PATCH v3] drm/msm/a6xx: use __unused__ to fix compiler warnings for gen7_* includes
Hi Abhinav, Just a drive by style comment. On Tue, Jun 04, 2024 at 05:38:28PM -0700, Abhinav Kumar wrote: > diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gpu_state.c > b/drivers/gpu/drm/msm/adreno/a6xx_gpu_state.c > index 0a7717a4fc2f..a958e2b3c025 100644 > --- a/drivers/gpu/drm/msm/adreno/a6xx_gpu_state.c > +++ b/drivers/gpu/drm/msm/adreno/a6xx_gpu_state.c > @@ -8,19 +8,15 @@ > #include "a6xx_gpu_state.h" > #include "a6xx_gmu.xml.h" > > -/* Ignore diagnostics about register tables that we aren't using yet. We > don't > - * want to modify these headers too much from their original source. > - */ > -#pragma GCC diagnostic push > -#pragma GCC diagnostic ignored "-Wunused-variable" > -#pragma GCC diagnostic ignored "-Wunused-const-variable" > +static const unsigned int *gen7_0_0_external_core_regs[] > __attribute((__unused__)); > +static const unsigned int *gen7_2_0_external_core_regs[] > __attribute((__unused__)); > +static const unsigned int *gen7_9_0_external_core_regs[] > __attribute((__unused__)); > +static struct gen7_sptp_cluster_registers gen7_9_0_sptp_clusters[] > __attribute((__unused__)); Please do not open code attributes. This is available as either '__always_unused' or '__maybe_unused', depending on the context. checkpatch would have warned about this if it was '__attribute__' instead of '__attribute'. Cheers, Nathan
Re: [PATCH] drm/amd/display: Increase frame-larger-than warning limit
Hi Palmer, On Thu, May 30, 2024 at 07:57:42AM -0700, Palmer Dabbelt wrote: > From: Palmer Dabbelt > > I get a handful of build errors along the lines of > > > linux/drivers/gpu/drm/amd/amdgpu/../display/dc/dml/dcn32/display_mode_vba_32.c:58:13: > error: stack frame size (2352) exceeds limit (2048) in > 'DISPCLKDPPCLKDCFCLKDeepSleepPrefetchParametersWatermarksAndPerformanceCalculation' > [-Werror,-Wframe-larger-than] > static void > DISPCLKDPPCLKDCFCLKDeepSleepPrefetchParametersWatermarksAndPerformanceCalculation( > ^ > > linux/drivers/gpu/drm/amd/amdgpu/../display/dc/dml/dcn32/display_mode_vba_32.c:1724:6: > error: stack frame size (2096) exceeds limit (2048) in > 'dml32_ModeSupportAndSystemConfigurationFull' [-Werror,-Wframe-larger-than] > void dml32_ModeSupportAndSystemConfigurationFull(struct display_mode_lib > *mode_lib) > ^ Judging from the message, this is clang/LLVM? What version? I assume this showed up in 6.10-rc1 because of commit 77acc6b55ae4 ("riscv: add support for kernel-mode FPU"), which allows this driver to be built for RISC-V. Is this allmodconfig or some other configuration? > as of 6.10-rc1. > > Signed-off-by: Palmer Dabbelt > --- > drivers/gpu/drm/amd/display/dc/dml/Makefile | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/amd/display/dc/dml/Makefile > b/drivers/gpu/drm/amd/display/dc/dml/Makefile > index c4a5efd2dda5..b2bd72e63734 100644 > --- a/drivers/gpu/drm/amd/display/dc/dml/Makefile > +++ b/drivers/gpu/drm/amd/display/dc/dml/Makefile > @@ -62,9 +62,9 @@ endif > > ifneq ($(CONFIG_FRAME_WARN),0) > ifeq ($(filter y,$(CONFIG_KASAN)$(CONFIG_KCSAN)),y) > -frame_warn_flag := -Wframe-larger-than=3072 > +frame_warn_flag := -Wframe-larger-than=4096 > else > -frame_warn_flag := -Wframe-larger-than=2048 > +frame_warn_flag := -Wframe-larger-than=3072 > endif > endif > > -- > 2.45.1 >
[PATCH] drm/radeon: Remove __counted_by from StateArray.states[]
From: Bill Wendling Work for __counted_by on generic pointers in structures (not just flexible array members) has started landing in Clang 19 (current tip of tree). During the development of this feature, a restriction was added to __counted_by to prevent the flexible array member's element type from including a flexible array member itself such as: struct foo { int count; char buf[]; }; struct bar { int count; struct foo data[] __counted_by(count); }; because the size of data cannot be calculated with the standard array size formula: sizeof(struct foo) * count This restriction was downgraded to a warning but due to CONFIG_WERROR, it can still break the build. The application of __counted_by on the states member of 'struct _StateArray' triggers this restriction, resulting in: drivers/gpu/drm/radeon/pptable.h:442:5: error: 'counted_by' should not be applied to an array with element of unknown size because 'ATOM_PPLIB_STATE_V2' (aka 'struct _ATOM_PPLIB_STATE_V2') is a struct type with a flexible array member. This will be an error in a future compiler version [-Werror,-Wbounds-safety-counted-by-elt-type-unknown-size] 442 | ATOM_PPLIB_STATE_V2 states[] __counted_by(ucNumEntries); | ^~~~ 1 error generated. Remove this use of __counted_by to fix the warning/error. However, rather than remove it altogether, leave it commented, as it may be possible to support this in future compiler releases. Cc: sta...@vger.kernel.org Closes: https://github.com/ClangBuiltLinux/linux/issues/2028 Fixes: efade6fe50e7 ("drm/radeon: silence UBSAN warning (v3)") Signed-off-by: Bill Wendling Co-developed-by: Nathan Chancellor Signed-off-by: Nathan Chancellor --- drivers/gpu/drm/radeon/pptable.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/gpu/drm/radeon/pptable.h b/drivers/gpu/drm/radeon/pptable.h index b7f22597ee95..969a8fb0ee9e 100644 --- a/drivers/gpu/drm/radeon/pptable.h +++ b/drivers/gpu/drm/radeon/pptable.h @@ -439,7 +439,7 @@ typedef struct _StateArray{ //how many states we have UCHAR ucNumEntries; -ATOM_PPLIB_STATE_V2 states[] __counted_by(ucNumEntries); +ATOM_PPLIB_STATE_V2 states[] /* __counted_by(ucNumEntries) */; }StateArray; --- base-commit: e64e8f7c178e5228e0b2dbb504b9dc75953a319f change-id: 20240529-drop-counted-by-radeon-states-state-array-01936ded4c18 Best regards, -- Nathan Chancellor
Re: [PATCH] drm/msm/gen_header: allow skipping the validation
Hi Dmitry, On Tue, Apr 09, 2024 at 05:22:54PM +0300, Dmitry Baryshkov wrote: > We don't need to run the validation of the XML files if we are just > compiling the kernel. Skip the validation unless the user enables > corresponding Kconfig option. This removes a warning from gen_header.py > about lxml being not installed. > > Reported-by: Stephen Rothwell > Closes: https://lore.kernel.org/all/20240409120108.2303d...@canb.auug.org.au/ > Signed-off-by: Dmitry Baryshkov > --- > drivers/gpu/drm/msm/Kconfig | 8 > drivers/gpu/drm/msm/Makefile| 9 - > drivers/gpu/drm/msm/registers/gen_header.py | 14 +++--- > 3 files changed, 27 insertions(+), 4 deletions(-) > > diff --git a/drivers/gpu/drm/msm/Kconfig b/drivers/gpu/drm/msm/Kconfig > index f202f26adab2..4c9bf237d4a2 100644 > --- a/drivers/gpu/drm/msm/Kconfig > +++ b/drivers/gpu/drm/msm/Kconfig > @@ -54,6 +54,14 @@ config DRM_MSM_GPU_SUDO > Only use this if you are a driver developer. This should *not* > be enabled for production kernels. If unsure, say N. > > +config DRM_MSM_VALIDATE_XML > + bool "Validate XML register files against schema" > + depends on DRM_MSM && EXPERT > + depends on $(success,$(PYTHON3) -c "import lxml") > + help > + Validate XML files with register definitions against rules-fd schema. > + This option is mostly targeting DRM MSM developers. If unsure, say N. Is this change going to be applied? I have gotten a little tired of seeing "lxml not found, skipping validation" in all of my builds :) Cheers, Nathan
Re: [PATCH v3 0/2] Fix Kernel CI issues
Hi Tomi, On Sat, Apr 27, 2024 at 10:48:16AM +0300, Tomi Valkeinen wrote: > On 26/04/2024 22:27, Anatoliy Klymenko wrote: > > Fix number of CI reported W=1 build issues. > > > > Patch 1/2: Fix function arguments description. > > Closes: > > https://lore.kernel.org/oe-kbuild-all/202404260616.kfgdpcdn-...@intel.com/ > > > > Patch 2/2: Fix clang compilation error. > > Closes: > > https://lore.kernel.org/oe-kbuild-all/202404260946.4ozxvhd2-...@intel.com/ > > > > Signed-off-by: Anatoliy Klymenko > > --- > > Changes in v3: > > - Add Signed-off-by tag. > > > > - Link to v2: > > https://lore.kernel.org/r/20240425-dp-live-fmt-fix-v2-0-6048e8121...@amd.com > > > > Changes in v2: > > - Compilation error fix added. > > > > - Link to v1: > > https://lore.kernel.org/r/20240425-dp-live-fmt-fix-v1-1-405f352d3...@amd.com > > > > --- > > Anatoliy Klymenko (2): > >drm: xlnx: zynqmp_dpsub: Fix few function comments > >drm: xlnx: zynqmp_dpsub: Fix compilation error > > > > drivers/gpu/drm/xlnx/zynqmp_disp.c | 6 +++--- > > 1 file changed, 3 insertions(+), 3 deletions(-) > > --- > > base-commit: 2bdb481bf7a93c22b9fea8daefa2834aab23a70f > > change-id: 20240425-dp-live-fmt-fix-a10bf7973596 > > > > Best regards, > > Thanks, pushed to drm-misc-next. I think the second patch also needs to go to drm-misc-next-fixes? The clang warning fixed by it has returned in next-20240503 because it appears that for-linux-next was switch from drm-misc-next to drm-misc-next-fixes, as I see for-linux-next was pointing to commit 235e60653f8d ("drm/debugfs: Drop conditionals around of_node pointers") on drm-misc-next in next-20240502 but it is now pointing to commit be3f3042391d ("drm: zynqmp_dpsub: Always register bridge") on drm-misc-next-fixes in next-20240503. Cheers, Nathan
[PATCH 2/2] drm/amd/display: Fix CFLAGS for dml2_core_dcn4_calcs.o
-Wframe-larger-than=2048 is a part of both CFLAGS and CFLAGS_REMOVE for dml2_core_dcn4_calcs.o, which means that it ultimately gets removed altogether for 64-bit targets, as 2048 is the default FRAME_WARN value for 64-bit platforms, resulting in no -Wframe-larger-than coverage for this file. Remove -Wframe-larger-than from CFLAGS_REMOVE_dml2_core_dcn4_calcs.o and move to $(frame_warn_flag) for CFLAGS_dml2_core_dcn4_calcs.o, as that accounts for the fact that -Wframe-larger-than may need to be larger than 2048 in certain situations, such as when the sanitizers are enabled. Fixes: d546a39c6b10 ("drm/amd/display: Add misc DC changes for DCN401") Signed-off-by: Nathan Chancellor --- drivers/gpu/drm/amd/display/dc/dml2/Makefile | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/gpu/drm/amd/display/dc/dml2/Makefile b/drivers/gpu/drm/amd/display/dc/dml2/Makefile index c35212a4a968..904a2d419638 100644 --- a/drivers/gpu/drm/amd/display/dc/dml2/Makefile +++ b/drivers/gpu/drm/amd/display/dc/dml2/Makefile @@ -111,7 +111,7 @@ CFLAGS_$(AMDDALPATH)/dc/dml2/dml21/src/dml2_top/dml_top.o := $(dml2_ccflags) CFLAGS_$(AMDDALPATH)/dc/dml2/dml21/src/dml2_top/dml_top_mcache.o := $(dml2_ccflags) CFLAGS_$(AMDDALPATH)/dc/dml2/dml21/src/dml2_top/dml2_top_optimization := $(dml2_ccflags) CFLAGS_$(AMDDALPATH)/dc/dml2/dml21/src/dml2_core/dml2_core_dcn4.o := $(dml2_ccflags) -CFLAGS_$(AMDDALPATH)/dc/dml2/dml21/src/dml2_core/dml2_core_dcn4_calcs.o := $(dml2_ccflags) -Wframe-larger-than=2048 +CFLAGS_$(AMDDALPATH)/dc/dml2/dml21/src/dml2_core/dml2_core_dcn4_calcs.o := $(dml2_ccflags) $(frame_warn_flag) CFLAGS_$(AMDDALPATH)/dc/dml2/dml21/src/dml2_core/dml2_core_factory.o := $(dml2_ccflags) CFLAGS_$(AMDDALPATH)/dc/dml2/dml21/src/dml2_core/dml2_core_shared.o := $(dml2_ccflags) $(frame_warn_flag) CFLAGS_$(AMDDALPATH)/dc/dml2/dml21/src/dml2_dpmm/dml2_dpmm_dcn4.o := $(dml2_ccflags) @@ -134,7 +134,7 @@ CFLAGS_REMOVE_$(AMDDALPATH)/dc/dml2/dml21/src/dml2_top/dml_top.o := $(dml2_rcfla CFLAGS_REMOVE_$(AMDDALPATH)/dc/dml2/dml21/src/dml2_top/dml_top_mcache.o := $(dml2_rcflags) CFLAGS_REMOVE_$(AMDDALPATH)/dc/dml2/dml21/src/dml2_top/dml2_top_optimization.o := $(dml2_rcflags) CFLAGS_REMOVE_$(AMDDALPATH)/dc/dml2/dml21/src/dml2_core/dml2_core_dcn4.o := $(dml2_rcflags) -CFLAGS_REMOVE_$(AMDDALPATH)/dc/dml2/dml21/src/dml2_core/dml2_core_dcn4_calcs.o := $(dml2_rcflags) -Wframe-larger-than=2048 +CFLAGS_REMOVE_$(AMDDALPATH)/dc/dml2/dml21/src/dml2_core/dml2_core_dcn4_calcs.o := $(dml2_rcflags) CFLAGS_REMOVE_$(AMDDALPATH)/dc/dml2/dml21/src/dml2_core/dml2_core_factory.o := $(dml2_rcflags) CFLAGS_REMOVE_$(AMDDALPATH)/dc/dml2/dml21/src/dml2_core/dml2_core_shared.o := $(dml2_rcflags) CFLAGS_REMOVE_$(AMDDALPATH)/dc/dml2/dml21/src/dml2_dpmm/dml2_dpmm_dcn4.o := $(dml2_rcflags) -- 2.44.0
[PATCH 1/2] drm/amd/display: Add frame_warn_flag to dml2_core_shared.o
When building with tip of tree Clang, there are some new instances of -Wframe-larger-than from the new display code (which become fatal with CONFIG_WERROR=y): drivers/gpu/drm/amd/amdgpu/../display/dc/dml2/dml21/src/dml2_core/dml2_core_shared.c:754:6: error: stack frame size (2488) exceeds limit (2048) in 'dml2_core_shared_mode_support' [-Werror,-Wframe-larger-than] 754 | bool dml2_core_shared_mode_support(struct dml2_core_calcs_mode_support_ex *in_out_params) | ^ drivers/gpu/drm/amd/amdgpu/../display/dc/dml2/dml21/src/dml2_core/dml2_core_shared.c:9834:6: error: stack frame size (2152) exceeds limit (2048) in 'dml2_core_shared_mode_programming' [-Werror,-Wframe-larger-than] 9834 | bool dml2_core_shared_mode_programming(struct dml2_core_calcs_mode_programming_ex *in_out_params) | ^ 2 errors generated. These warnings do not occur when CONFIG_K{A,C,M}SAN are disabled, so add $(frame_warn_flag) to dml2_core_shared.o's CFLAGS, which was added in commit 6740ec97bcdb ("drm/amd/display: Increase frame warning limit with KASAN or KCSAN in dml2") to account for this situation. Fixes: d546a39c6b10 ("drm/amd/display: Add misc DC changes for DCN401") Signed-off-by: Nathan Chancellor --- drivers/gpu/drm/amd/display/dc/dml2/Makefile | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/gpu/drm/amd/display/dc/dml2/Makefile b/drivers/gpu/drm/amd/display/dc/dml2/Makefile index 6c76f346b237..c35212a4a968 100644 --- a/drivers/gpu/drm/amd/display/dc/dml2/Makefile +++ b/drivers/gpu/drm/amd/display/dc/dml2/Makefile @@ -113,7 +113,7 @@ CFLAGS_$(AMDDALPATH)/dc/dml2/dml21/src/dml2_top/dml2_top_optimization := $(dml2_ CFLAGS_$(AMDDALPATH)/dc/dml2/dml21/src/dml2_core/dml2_core_dcn4.o := $(dml2_ccflags) CFLAGS_$(AMDDALPATH)/dc/dml2/dml21/src/dml2_core/dml2_core_dcn4_calcs.o := $(dml2_ccflags) -Wframe-larger-than=2048 CFLAGS_$(AMDDALPATH)/dc/dml2/dml21/src/dml2_core/dml2_core_factory.o := $(dml2_ccflags) -CFLAGS_$(AMDDALPATH)/dc/dml2/dml21/src/dml2_core/dml2_core_shared.o := $(dml2_ccflags) +CFLAGS_$(AMDDALPATH)/dc/dml2/dml21/src/dml2_core/dml2_core_shared.o := $(dml2_ccflags) $(frame_warn_flag) CFLAGS_$(AMDDALPATH)/dc/dml2/dml21/src/dml2_dpmm/dml2_dpmm_dcn4.o := $(dml2_ccflags) CFLAGS_$(AMDDALPATH)/dc/dml2/dml21/src/dml2_dpmm/dml2_dpmm_factory.o := $(dml2_ccflags) CFLAGS_$(AMDDALPATH)/dc/dml2/dml21/src/dml2_mcg/dml2_mcg_dcn4.o := $(dml2_ccflags) -- 2.44.0
[PATCH 0/2] drm/amd/display: Use frame_warn_flag consistently in dml2 Makefile
Hi all, This series resolves a couple instances of -Wframe-larger-than from the new display code that appear with newer versions of clang along without another inconsistency I noticed while fixing this, which have been accounted for with the $(frame_warn_flag) variable. --- Nathan Chancellor (2): drm/amd/display: Add frame_warn_flag to dml2_core_shared.o drm/amd/display: Fix CFLAGS for dml2_core_dcn4_calcs.o drivers/gpu/drm/amd/display/dc/dml2/Makefile | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) --- base-commit: d60dc4dd72412d5d9566fdf391e4202b05f88912 change-id: 20240424-amdgpu-dml2-fix-frame-larger-than-dcn401-48ff7e1f51ea Best regards, -- Nathan Chancellor
[PATCH] drm/amd/display: Avoid -Wenum-float-conversion in add_margin_and_round_to_dfs_grainularity()
When building with clang 19 or newer (which strengthened some of the enum conversion warnings for C), there is a warning (or error with CONFIG_WERROR=y) around doing arithmetic with an enumerated type and a floating point expression. drivers/gpu/drm/amd/amdgpu/../display/dc/dml2/dml21/src/dml2_dpmm/dml2_dpmm_dcn4.c:181:58: error: arithmetic between enumeration type 'enum dentist_divider_range' and floating-point type 'double' [-Werror,-Wenum-float-conversion] 181 | divider = (unsigned int)(DFS_DIVIDER_RANGE_SCALE_FACTOR * (vco_freq_khz / clock_khz)); | ~~ ^ ~~ 1 error generated. This conversion is expected due to the nature of the enumerated value and definition, so silence the warning by casting the enumeration to an integer explicitly to make it clear to the compiler. Fixes: 3df48ddedee4 ("drm/amd/display: Add new DCN401 sources") Signed-off-by: Nathan Chancellor --- Alternatively, perhaps the potential truncation could happen before the multiplication? divider = DFS_DIVIDER_RANGE_SCALE_FACTOR * (unsigned int)(vco_freq_khz / clock_khz); I suspect the result of the division is probably not very large (certainly not within UINT_MAX / 4), so I would not expect the multiplication to overflow, but I was not sure so I decided to take the safer, NFC change. I am happy to respin as necessary. --- .../gpu/drm/amd/display/dc/dml2/dml21/src/dml2_dpmm/dml2_dpmm_dcn4.c| 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/gpu/drm/amd/display/dc/dml2/dml21/src/dml2_dpmm/dml2_dpmm_dcn4.c b/drivers/gpu/drm/amd/display/dc/dml2/dml21/src/dml2_dpmm/dml2_dpmm_dcn4.c index e6698ee65843..65eb0187e965 100644 --- a/drivers/gpu/drm/amd/display/dc/dml2/dml21/src/dml2_dpmm/dml2_dpmm_dcn4.c +++ b/drivers/gpu/drm/amd/display/dc/dml2/dml21/src/dml2_dpmm/dml2_dpmm_dcn4.c @@ -178,7 +178,7 @@ static bool add_margin_and_round_to_dfs_grainularity(double clock_khz, double ma clock_khz *= 1.0 + margin; - divider = (unsigned int)(DFS_DIVIDER_RANGE_SCALE_FACTOR * (vco_freq_khz / clock_khz)); + divider = (unsigned int)((int)DFS_DIVIDER_RANGE_SCALE_FACTOR * (vco_freq_khz / clock_khz)); /* we want to floor here to get higher clock than required rather than lower */ if (divider < DFS_DIVIDER_RANGE_2_START) { --- base-commit: d60dc4dd72412d5d9566fdf391e4202b05f88912 change-id: 20240424-amdgpu-display-dcn401-enum-float-conversion-c09cc1826ea2 Best regards, -- Nathan Chancellor
[PATCH] drm/xe: Add xe_guc_ads.c to uses_generated_oob
A recent change added a use of xe_wa_oob.h without adding the file that uses it to uses_generated_oob, which means xe_wa_oob.h does not get properly generated before attempting to build the object file: LINK resolve_btfids CC [M] drivers/gpu/drm/xe/xe_guc_ads.o drivers/gpu/drm/xe/xe_guc_ads.c:10:10: fatal error: generated/xe_wa_oob.h: No such file or directory 10 | #include | ^~~ After adding '$(obj)/xe_guc_ads.o' to uses_generated_oob, xe_wa_oob.h is always generated before building the file, resulting in no errors: LINK resolve_btfids HOSTCC drivers/gpu/drm/xe/xe_gen_wa_oob GEN xe_wa_oob.c xe_wa_oob.h CC [M] drivers/gpu/drm/xe/xe_guc_ads.o Fixes: c151ff5c9053 ("drm/xe/lnl: Enable GuC Wa_14019882105") Signed-off-by: Nathan Chancellor --- drivers/gpu/drm/xe/Makefile | 1 + 1 file changed, 1 insertion(+) diff --git a/drivers/gpu/drm/xe/Makefile b/drivers/gpu/drm/xe/Makefile index e106767c9a6e..60c90dc918b2 100644 --- a/drivers/gpu/drm/xe/Makefile +++ b/drivers/gpu/drm/xe/Makefile @@ -49,6 +49,7 @@ $(obj)/generated/%_wa_oob.c $(obj)/generated/%_wa_oob.h: $(obj)/xe_gen_wa_oob \ uses_generated_oob := \ $(obj)/xe_gsc.o \ $(obj)/xe_guc.o \ + $(obj)/xe_guc_ads.o \ $(obj)/xe_migrate.o \ $(obj)/xe_ring_ops.o \ $(obj)/xe_vm.o \ --- base-commit: 9c1857d587e91dfc10875a8c1083360db047404f change-id: 20240410-drm-xe-fix-xe_guc_ads-using-xe_wa_oob-9cb394101ad8 Best regards, -- Nathan Chancellor
[PATCH] drm/panthor: Fix clang -Wunused-but-set-variable in tick_ctx_apply()
Clang warns (or errors with CONFIG_WERROR): drivers/gpu/drm/panthor/panthor_sched.c:2048:6: error: variable 'csg_mod_mask' set but not used [-Werror,-Wunused-but-set-variable] 2048 | u32 csg_mod_mask = 0, free_csg_slots = 0; | ^ 1 error generated. The variable is an artifact left over from refactoring that occurred during the development of the initial series for this driver. Remove it to resolve the warning. Fixes: de8548813824 ("drm/panthor: Add the scheduler logical block") Signed-off-by: Nathan Chancellor --- drivers/gpu/drm/panthor/panthor_sched.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/drivers/gpu/drm/panthor/panthor_sched.c b/drivers/gpu/drm/panthor/panthor_sched.c index 5f7803b6fc48..e5a710f190d2 100644 --- a/drivers/gpu/drm/panthor/panthor_sched.c +++ b/drivers/gpu/drm/panthor/panthor_sched.c @@ -2045,7 +2045,7 @@ tick_ctx_apply(struct panthor_scheduler *sched, struct panthor_sched_tick_ctx *c struct panthor_device *ptdev = sched->ptdev; struct panthor_csg_slot *csg_slot; int prio, new_csg_prio = MAX_CSG_PRIO, i; - u32 csg_mod_mask = 0, free_csg_slots = 0; + u32 free_csg_slots = 0; struct panthor_csg_slots_upd_ctx upd_ctx; int ret; @@ -2139,7 +2139,6 @@ tick_ctx_apply(struct panthor_scheduler *sched, struct panthor_sched_tick_ctx *c csg_iface = panthor_fw_get_csg_iface(ptdev, csg_id); csg_slot = &sched->csg_slots[csg_id]; - csg_mod_mask |= BIT(csg_id); group_bind_locked(group, csg_id); csg_slot_prog_locked(ptdev, csg_id, new_csg_prio--); csgs_upd_ctx_queue_reqs(ptdev, &upd_ctx, csg_id, --- base-commit: d180649238f04183950d9c8a7d8a2c2f1788a89c change-id: 20240328-panthor-drop-csg_mod_mask-b4bbe317d690 Best regards, -- Nathan Chancellor
Re: [PATCH v6 10/14] drm/panthor: Add the scheduler logical block
Hi Boris, On Thu, Feb 29, 2024 at 05:22:24PM +0100, Boris Brezillon wrote: > --- > drivers/gpu/drm/panthor/panthor_sched.c | 3502 +++ > drivers/gpu/drm/panthor/panthor_sched.h | 50 + > 2 files changed, 3552 insertions(+) > create mode 100644 drivers/gpu/drm/panthor/panthor_sched.c > create mode 100644 drivers/gpu/drm/panthor/panthor_sched.h > > diff --git a/drivers/gpu/drm/panthor/panthor_sched.c > b/drivers/gpu/drm/panthor/panthor_sched.c > new file mode 100644 > index ..5f7803b6fc48 > --- /dev/null > +++ b/drivers/gpu/drm/panthor/panthor_sched.c > +static void > +tick_ctx_apply(struct panthor_scheduler *sched, struct > panthor_sched_tick_ctx *ctx) > +{ > + struct panthor_group *group, *tmp; > + struct panthor_device *ptdev = sched->ptdev; > + struct panthor_csg_slot *csg_slot; > + int prio, new_csg_prio = MAX_CSG_PRIO, i; > + u32 csg_mod_mask = 0, free_csg_slots = 0; > + struct panthor_csg_slots_upd_ctx upd_ctx; > + int ret; > + > + csgs_upd_ctx_init(&upd_ctx); > + > + for (prio = PANTHOR_CSG_PRIORITY_COUNT - 1; prio >= 0; prio--) { > + /* Suspend or terminate evicted groups. */ > + list_for_each_entry(group, &ctx->old_groups[prio], run_node) { > + bool term = !group_can_run(group); > + int csg_id = group->csg_id; > + > + if (drm_WARN_ON(&ptdev->base, csg_id < 0)) > + continue; > + > + csg_slot = &sched->csg_slots[csg_id]; > + csgs_upd_ctx_queue_reqs(ptdev, &upd_ctx, csg_id, > + term ? CSG_STATE_TERMINATE : > CSG_STATE_SUSPEND, > + CSG_STATE_MASK); > + } > + > + /* Update priorities on already running groups. */ > + list_for_each_entry(group, &ctx->groups[prio], run_node) { > + struct panthor_fw_csg_iface *csg_iface; > + int csg_id = group->csg_id; > + > + if (csg_id < 0) { > + new_csg_prio--; > + continue; > + } > + > + csg_slot = &sched->csg_slots[csg_id]; > + csg_iface = panthor_fw_get_csg_iface(ptdev, csg_id); > + if (csg_slot->priority == new_csg_prio) { > + new_csg_prio--; > + continue; > + } > + > + panthor_fw_update_reqs(csg_iface, endpoint_req, > + > CSG_EP_REQ_PRIORITY(new_csg_prio), > +CSG_EP_REQ_PRIORITY_MASK); > + csgs_upd_ctx_queue_reqs(ptdev, &upd_ctx, csg_id, > + csg_iface->output->ack ^ > CSG_ENDPOINT_CONFIG, > + CSG_ENDPOINT_CONFIG); > + new_csg_prio--; > + } > + } > + > + ret = csgs_upd_ctx_apply_locked(ptdev, &upd_ctx); > + if (ret) { > + panthor_device_schedule_reset(ptdev); > + ctx->csg_upd_failed_mask |= upd_ctx.timedout_mask; > + return; > + } > + > + /* Unbind evicted groups. */ > + for (prio = PANTHOR_CSG_PRIORITY_COUNT - 1; prio >= 0; prio--) { > + list_for_each_entry(group, &ctx->old_groups[prio], run_node) { > + /* This group is gone. Process interrupts to clear > + * any pending interrupts before we start the new > + * group. > + */ > + if (group->csg_id >= 0) > + sched_process_csg_irq_locked(ptdev, > group->csg_id); > + > + group_unbind_locked(group); > + } > + } > + > + for (i = 0; i < sched->csg_slot_count; i++) { > + if (!sched->csg_slots[i].group) > + free_csg_slots |= BIT(i); > + } > + > + csgs_upd_ctx_init(&upd_ctx); > + new_csg_prio = MAX_CSG_PRIO; > + > + /* Start new groups. */ > + for (prio = PANTHOR_CSG_PRIORITY_COUNT - 1; prio >= 0; prio--) { > + list_for_each_entry(group, &ctx->groups[prio], run_node) { > + int csg_id = group->csg_id; > + struct panthor_fw_csg_iface *csg_iface; > + > + if (csg_id >= 0) { > + new_csg_prio--; > + continue; > + } > + > + csg_id = ffs(free_csg_slots) - 1; > + if (drm_WARN_ON(&ptdev->base, csg_id < 0)) > + break; > + > + csg_iface = panthor_fw_get_csg_iface(ptdev, csg_id); > + csg_slot = &sched->csg_slots[csg_id]; > +
Re: [RESEND v3 2/2] drm: Add CONFIG_DRM_WERROR
On Wed, Mar 27, 2024 at 09:59:01AM +0200, Jani Nikula wrote: > On Wed, 27 Mar 2024, Maxime Ripard wrote: > > On Tue, Mar 26, 2024 at 03:56:50PM -0700, Nathan Chancellor wrote: > >> On Tue, Mar 05, 2024 at 11:07:36AM +0200, Jani Nikula wrote: > >> > Add kconfig to enable -Werror subsystem wide. This is useful for > >> > development and CI to keep the subsystem warning free, while avoiding > >> > issues outside of the subsystem that kernel wide CONFIG_WERROR=y might > >> > hit. > >> > > >> > v2: Don't depend on COMPILE_TEST > >> > > >> > Reviewed-by: Hamza Mahfooz # v1 > >> > Signed-off-by: Jani Nikula > >> > --- > >> > drivers/gpu/drm/Kconfig | 13 + > >> > drivers/gpu/drm/Makefile | 3 +++ > >> > 2 files changed, 16 insertions(+) > >> > > >> > diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig > >> > index 6e853acf15da..c08e18108c2a 100644 > >> > --- a/drivers/gpu/drm/Kconfig > >> > +++ b/drivers/gpu/drm/Kconfig > >> > @@ -416,3 +416,16 @@ config DRM_LIB_RANDOM > >> > config DRM_PRIVACY_SCREEN > >> > bool > >> > default n > >> > + > >> > +config DRM_WERROR > >> > +bool "Compile the drm subsystem with warnings as errors" > >> > +depends on EXPERT > >> > +default n > >> > +help > >> > + A kernel build should not cause any compiler warnings, and > >> > this > >> > + enables the '-Werror' flag to enforce that rule in the drm > >> > subsystem. > >> > + > >> > + The drm subsystem enables more warnings than the kernel > >> > default, so > >> > + this config option is disabled by default. > >> > + > >> > + If in doubt, say N. > >> > >> While I understand the desire for an easy switch that maintainers and > >> developers can use to ensure that their changes are warning free for the > >> drm subsystem specifically, I think subsystem specific configuration > >> options like this are actively detrimental to developers and continuous > >> integration systems that build test the entire kernel. For example, we > >> turned off CONFIG_WERROR for our Hexagon builds because of warnings that > >> appear with -Wextra that are legitimate but require treewide changes to > >> resolve in a manner sufficient for Linus: > >> > >> https://github.com/ClangBuiltLinux/linux/issues/1285 > >> https://lore.kernel.org/all/CAHk-=wg80je=k7madf4e7wrrnp37e3qh6y10svhdc7o8sz_...@mail.gmail.com/ > >> https://lore.kernel.org/all/20230522105049.1467313-1-schne...@linux.ibm.com/ > >> > >> But now, due to CONFIG_DRM_WERROR getting enabled by all{mod,yes}config > >> and -Wextra being unconditionally enabled for DRM, those warnings hard > >> break the build despite CONFIG_WERROR=n... > > > > Would making DRM_WERROR depends on WERROR address your concerns? > > But then what would be the point of having DRM_WERROR at all? For me the > point is, "werror in drm, ignore the rest, they're someone else's > problem". Right, I do think this is a valid view point and one I am sympathetic to, especially since it is in the pursuit of increased code quality. I do not want to disrupt that. > An alternative would be to "depends on !COMPILE_TEST" that we have in > i915, but then some folks want to have COMPILE_TEST in drm, because some > drivers are otherwise hard for people to build. Right. I think it is unfortunate how (at least in my opinion) CONFIG_COMPILE_TEST has two meanings: genuinely just compile testing or "allmodconfig". For the first case, we would want CONFIG_DRM_WERROR=y but for the second case, it would be nice for CONFIG_DRM_WERROR to default to off (because CONFIG_WERROR is enabled) but allow developers to turn it on explicitly. Another lofty/wistful idea to solve this would be to implement something similar to compiler diagnostic groups for Kconfig, where there would be a hierarchy like CONFIG_WERROR CONFIG_DRM_WERROR CONFIG_SUBSYSTEM_A_WERROR CONFIG_SUBSYSTEM_B_WERROR where the value of CONFIG_WERROR is the same value for all subconfigurations under it but they could still be enabled individually without any additional dependencies (ala something like '-Wno-unused -Wunused-variable'), which would allow my use case of CONFIG_WERROR=n removing all instances of -Werror t
Re: [RESEND v3 2/2] drm: Add CONFIG_DRM_WERROR
On Tue, Mar 05, 2024 at 11:07:36AM +0200, Jani Nikula wrote: > Add kconfig to enable -Werror subsystem wide. This is useful for > development and CI to keep the subsystem warning free, while avoiding > issues outside of the subsystem that kernel wide CONFIG_WERROR=y might > hit. > > v2: Don't depend on COMPILE_TEST > > Reviewed-by: Hamza Mahfooz # v1 > Signed-off-by: Jani Nikula > --- > drivers/gpu/drm/Kconfig | 13 + > drivers/gpu/drm/Makefile | 3 +++ > 2 files changed, 16 insertions(+) > > diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig > index 6e853acf15da..c08e18108c2a 100644 > --- a/drivers/gpu/drm/Kconfig > +++ b/drivers/gpu/drm/Kconfig > @@ -416,3 +416,16 @@ config DRM_LIB_RANDOM > config DRM_PRIVACY_SCREEN > bool > default n > + > +config DRM_WERROR > + bool "Compile the drm subsystem with warnings as errors" > + depends on EXPERT > + default n > + help > + A kernel build should not cause any compiler warnings, and this > + enables the '-Werror' flag to enforce that rule in the drm subsystem. > + > + The drm subsystem enables more warnings than the kernel default, so > + this config option is disabled by default. > + > + If in doubt, say N. While I understand the desire for an easy switch that maintainers and developers can use to ensure that their changes are warning free for the drm subsystem specifically, I think subsystem specific configuration options like this are actively detrimental to developers and continuous integration systems that build test the entire kernel. For example, we turned off CONFIG_WERROR for our Hexagon builds because of warnings that appear with -Wextra that are legitimate but require treewide changes to resolve in a manner sufficient for Linus: https://github.com/ClangBuiltLinux/linux/issues/1285 https://lore.kernel.org/all/CAHk-=wg80je=k7madf4e7wrrnp37e3qh6y10svhdc7o8sz_...@mail.gmail.com/ https://lore.kernel.org/all/20230522105049.1467313-1-schne...@linux.ibm.com/ But now, due to CONFIG_DRM_WERROR getting enabled by all{mod,yes}config and -Wextra being unconditionally enabled for DRM, those warnings hard break the build despite CONFIG_WERROR=n... https://storage.tuxsuite.com/public/clangbuiltlinux/continuous-integration2/builds/2eEBDGEqfmMZjGg3ZvDx2af2pde/build.log Same thing with PowerPC allmodconfig because we see -Wframe-larger-than that appears because allmodconfig enables CONFIG_KASAN or CONFIG_KCSAN usually: https://storage.tuxsuite.com/public/clangbuiltlinux/continuous-integration2/builds/2eE2HDsODudQGqkMKAPQnId7pRd/build.log I don't know what the solution for this conflict is through. I guess it is just the nature of the kernel being a federation of independent subsystems that want to have their own policies. I suppose we can just set CONFIG_DRM_WERROR=n and be done with it but I would like to avoid this issue from spreading to other subsystems because it does not scale for folks like us who do many builds across many trees. It would be nice if there was something like CONFIG_WERROR_DIRS or something that could take a set of directories that should have -Werror enabled so that you could do something like CONFIG_WERROR_DIRS="drivers/gpu/drm" and have -Werror automatically added to all commands within that directory like subdir-ccflags-y but it is explicitly opt in on the part of the developer/tester, rather than just happening to get enabled due to all{mod,yes}config. No idea if that is feasible or not though. > diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile > index ea456f057e8a..a73c04d2d7a3 100644 > --- a/drivers/gpu/drm/Makefile > +++ b/drivers/gpu/drm/Makefile > @@ -30,6 +30,9 @@ subdir-ccflags-y += -Wno-sign-compare > endif > # --- end copy-paste > > +# Enable -Werror in CI and development > +subdir-ccflags-$(CONFIG_DRM_WERROR) += -Werror > + > drm-y := \ > drm_aperture.o \ > drm_atomic.o \ > -- > 2.39.2 >
Re: [PATCH V8 08/12] drm/bridge: imx: add driver for HDMI TX Parallel Video Interface
On Tue, Feb 06, 2024 at 12:50:16PM -0600, Adam Ford wrote: > On Tue, Feb 6, 2024 at 11:06 AM Nathan Chancellor wrote: > > > > Hi all, > > > > On Sat, Feb 03, 2024 at 10:52:48AM -0600, Adam Ford wrote: > > > From: Lucas Stach > > > > > > This IP block is found in the HDMI subsystem of the i.MX8MP SoC. It has a > > > full timing generator and can switch between different video sources. On > > > the i.MX8MP however the only supported source is the LCDIF. The block > > > just needs to be powered up and told about the polarity of the video > > > sync signals to act in bypass mode. > > > > > > Signed-off-by: Lucas Stach > > > Reviewed-by: Luca Ceresoli (v7) > > > Tested-by: Marek Vasut (v1) > > > Tested-by: Luca Ceresoli (v7) > > > Tested-by: Richard Leitner (v2) > > > Tested-by: Frieder Schrempf (v2) > > > Reviewed-by: Laurent Pinchart (v3) > > > Reviewed-by: Luca Ceresoli > > > Tested-by: Luca Ceresoli > > > Tested-by: Fabio Estevam > > > Signed-off-by: Adam Ford > > > > > > > > > diff --git a/drivers/gpu/drm/bridge/imx/imx8mp-hdmi-pvi.c > > > b/drivers/gpu/drm/bridge/imx/imx8mp-hdmi-pvi.c > > > new file mode 100644 > > > index ..a76b7669fe8a > > > --- /dev/null > > > +++ b/drivers/gpu/drm/bridge/imx/imx8mp-hdmi-pvi.c > > ... > > > +static void imx8mp_hdmi_pvi_bridge_enable(struct drm_bridge *bridge, > > > + struct drm_bridge_state > > > *bridge_state) > > > +{ > > > + struct drm_atomic_state *state = bridge_state->base.state; > > > + struct imx8mp_hdmi_pvi *pvi = to_imx8mp_hdmi_pvi(bridge); > > > + struct drm_connector_state *conn_state; > > > + const struct drm_display_mode *mode; > > > + struct drm_crtc_state *crtc_state; > > > + struct drm_connector *connector; > > > + u32 bus_flags, val; > > > + > > > + connector = drm_atomic_get_new_connector_for_encoder(state, > > > bridge->encoder); > > > + conn_state = drm_atomic_get_new_connector_state(state, connector); > > > + crtc_state = drm_atomic_get_new_crtc_state(state, conn_state->crtc); > > > + > > > + if (WARN_ON(pm_runtime_resume_and_get(pvi->dev))) > > > + return; > > > + > > > + mode = &crtc_state->adjusted_mode; > > > + > > > + val = FIELD_PREP(PVI_CTRL_MODE_MASK, PVI_CTRL_MODE_LCDIF) | > > > PVI_CTRL_EN; > > > + > > > + if (mode->flags & DRM_MODE_FLAG_PVSYNC) > > > + val |= PVI_CTRL_OP_VSYNC_POL | PVI_CTRL_INP_VSYNC_POL; > > > + > > > + if (mode->flags & DRM_MODE_FLAG_PHSYNC) > > > + val |= PVI_CTRL_OP_HSYNC_POL | PVI_CTRL_INP_HSYNC_POL; > > > + > > > + if (pvi->next_bridge->timings) > > > + bus_flags = pvi->next_bridge->timings->input_bus_flags; > > > + else if (bridge_state) > > > + bus_flags = bridge_state->input_bus_cfg.flags; > > > + > > > + if (bus_flags & DRM_BUS_FLAG_DE_HIGH) > > > + val |= PVI_CTRL_OP_DE_POL | PVI_CTRL_INP_DE_POL; > > > + > > > + writel(val, pvi->regs + HTX_PVI_CTRL); > > > +} > > > > Apologies if this has already been reported or fixed, I searched lore > > and did not find anything. Clang warns (or errors with CONFIG_WERROR=y) > > for this function: > > > > drivers/gpu/drm/bridge/imx/imx8mp-hdmi-pvi.c:81:11: error: variable > > 'bus_flags' is used uninitialized whenever 'if' condition is false > > [-Werror,-Wsometimes-uninitialized] > > 81 | else if (bridge_state) > > | ^~~~ > > drivers/gpu/drm/bridge/imx/imx8mp-hdmi-pvi.c:84:6: note: uninitialized > > use occurs here > > 84 | if (bus_flags & DRM_BUS_FLAG_DE_HIGH) > > | ^ > > drivers/gpu/drm/bridge/imx/imx8mp-hdmi-pvi.c:81:7: note: remove the 'if' > > if its condition is always true > > 81 | else if (bridge_state) > > | ^ > > 82 | bus_flags = bridge_state->input_bus_cfg.flags; > > drivers/gpu/drm/bridge/imx/imx8mp-hdmi-pvi.c:60:15: note: initialize the > > variable 'bus_flags' to silence this warning > > 60 | u32 bus
Re: [PATCH V8 08/12] drm/bridge: imx: add driver for HDMI TX Parallel Video Interface
Hi all, On Sat, Feb 03, 2024 at 10:52:48AM -0600, Adam Ford wrote: > From: Lucas Stach > > This IP block is found in the HDMI subsystem of the i.MX8MP SoC. It has a > full timing generator and can switch between different video sources. On > the i.MX8MP however the only supported source is the LCDIF. The block > just needs to be powered up and told about the polarity of the video > sync signals to act in bypass mode. > > Signed-off-by: Lucas Stach > Reviewed-by: Luca Ceresoli (v7) > Tested-by: Marek Vasut (v1) > Tested-by: Luca Ceresoli (v7) > Tested-by: Richard Leitner (v2) > Tested-by: Frieder Schrempf (v2) > Reviewed-by: Laurent Pinchart (v3) > Reviewed-by: Luca Ceresoli > Tested-by: Luca Ceresoli > Tested-by: Fabio Estevam > Signed-off-by: Adam Ford > diff --git a/drivers/gpu/drm/bridge/imx/imx8mp-hdmi-pvi.c > b/drivers/gpu/drm/bridge/imx/imx8mp-hdmi-pvi.c > new file mode 100644 > index ..a76b7669fe8a > --- /dev/null > +++ b/drivers/gpu/drm/bridge/imx/imx8mp-hdmi-pvi.c ... > +static void imx8mp_hdmi_pvi_bridge_enable(struct drm_bridge *bridge, > + struct drm_bridge_state *bridge_state) > +{ > + struct drm_atomic_state *state = bridge_state->base.state; > + struct imx8mp_hdmi_pvi *pvi = to_imx8mp_hdmi_pvi(bridge); > + struct drm_connector_state *conn_state; > + const struct drm_display_mode *mode; > + struct drm_crtc_state *crtc_state; > + struct drm_connector *connector; > + u32 bus_flags, val; > + > + connector = drm_atomic_get_new_connector_for_encoder(state, > bridge->encoder); > + conn_state = drm_atomic_get_new_connector_state(state, connector); > + crtc_state = drm_atomic_get_new_crtc_state(state, conn_state->crtc); > + > + if (WARN_ON(pm_runtime_resume_and_get(pvi->dev))) > + return; > + > + mode = &crtc_state->adjusted_mode; > + > + val = FIELD_PREP(PVI_CTRL_MODE_MASK, PVI_CTRL_MODE_LCDIF) | PVI_CTRL_EN; > + > + if (mode->flags & DRM_MODE_FLAG_PVSYNC) > + val |= PVI_CTRL_OP_VSYNC_POL | PVI_CTRL_INP_VSYNC_POL; > + > + if (mode->flags & DRM_MODE_FLAG_PHSYNC) > + val |= PVI_CTRL_OP_HSYNC_POL | PVI_CTRL_INP_HSYNC_POL; > + > + if (pvi->next_bridge->timings) > + bus_flags = pvi->next_bridge->timings->input_bus_flags; > + else if (bridge_state) > + bus_flags = bridge_state->input_bus_cfg.flags; > + > + if (bus_flags & DRM_BUS_FLAG_DE_HIGH) > + val |= PVI_CTRL_OP_DE_POL | PVI_CTRL_INP_DE_POL; > + > + writel(val, pvi->regs + HTX_PVI_CTRL); > +} Apologies if this has already been reported or fixed, I searched lore and did not find anything. Clang warns (or errors with CONFIG_WERROR=y) for this function: drivers/gpu/drm/bridge/imx/imx8mp-hdmi-pvi.c:81:11: error: variable 'bus_flags' is used uninitialized whenever 'if' condition is false [-Werror,-Wsometimes-uninitialized] 81 | else if (bridge_state) | ^~~~ drivers/gpu/drm/bridge/imx/imx8mp-hdmi-pvi.c:84:6: note: uninitialized use occurs here 84 | if (bus_flags & DRM_BUS_FLAG_DE_HIGH) | ^ drivers/gpu/drm/bridge/imx/imx8mp-hdmi-pvi.c:81:7: note: remove the 'if' if its condition is always true 81 | else if (bridge_state) | ^ 82 | bus_flags = bridge_state->input_bus_cfg.flags; drivers/gpu/drm/bridge/imx/imx8mp-hdmi-pvi.c:60:15: note: initialize the variable 'bus_flags' to silence this warning 60 | u32 bus_flags, val; | ^ | = 0 1 error generated. This seems legitimate. If bridge_state can be NULL, should bus_flags be initialized to zero like it suggests or should that 'else if' be turned into a plain 'else'? I am happy to send a patch with that guidance. Cheers, Nathan
[PATCH] drm/amd/display: Increase frame-larger-than for all display_mode_vba files
After a recent change in LLVM, allmodconfig (which has CONFIG_KCSAN=y and CONFIG_WERROR=y enabled) has a few new instances of -Wframe-larger-than for the mode support and system configuration functions: drivers/gpu/drm/amd/amdgpu/../display/dc/dml/dcn20/display_mode_vba_20v2.c:3393:6: error: stack frame size (2144) exceeds limit (2048) in 'dml20v2_ModeSupportAndSystemConfigurationFull' [-Werror,-Wframe-larger-than] 3393 | void dml20v2_ModeSupportAndSystemConfigurationFull(struct display_mode_lib *mode_lib) | ^ 1 error generated. drivers/gpu/drm/amd/amdgpu/../display/dc/dml/dcn21/display_mode_vba_21.c:3520:6: error: stack frame size (2192) exceeds limit (2048) in 'dml21_ModeSupportAndSystemConfigurationFull' [-Werror,-Wframe-larger-than] 3520 | void dml21_ModeSupportAndSystemConfigurationFull(struct display_mode_lib *mode_lib) | ^ 1 error generated. drivers/gpu/drm/amd/amdgpu/../display/dc/dml/dcn20/display_mode_vba_20.c:3286:6: error: stack frame size (2128) exceeds limit (2048) in 'dml20_ModeSupportAndSystemConfigurationFull' [-Werror,-Wframe-larger-than] 3286 | void dml20_ModeSupportAndSystemConfigurationFull(struct display_mode_lib *mode_lib) | ^ 1 error generated. Without the sanitizers enabled, there are no warnings. This was the catalyst for commit 6740ec97bcdb ("drm/amd/display: Increase frame warning limit with KASAN or KCSAN in dml2") and that same change was made to dml in commit 5b750b22530f ("drm/amd/display: Increase frame warning limit with KASAN or KCSAN in dml") but the frame_warn_flag variable was not applied to all files. Do so now to clear up the warnings and make all these files consistent. Cc: sta...@vger.kernel.org Closes: https://github.com/ClangBuiltLinux/linux/issue/1990 Signed-off-by: Nathan Chancellor --- drivers/gpu/drm/amd/display/dc/dml/Makefile | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/drivers/gpu/drm/amd/display/dc/dml/Makefile b/drivers/gpu/drm/amd/display/dc/dml/Makefile index 6042a5a6a44f..59ade76ffb18 100644 --- a/drivers/gpu/drm/amd/display/dc/dml/Makefile +++ b/drivers/gpu/drm/amd/display/dc/dml/Makefile @@ -72,11 +72,11 @@ CFLAGS_$(AMDDALPATH)/dc/dml/display_mode_lib.o := $(dml_ccflags) CFLAGS_$(AMDDALPATH)/dc/dml/display_mode_vba.o := $(dml_ccflags) CFLAGS_$(AMDDALPATH)/dc/dml/dcn10/dcn10_fpu.o := $(dml_ccflags) CFLAGS_$(AMDDALPATH)/dc/dml/dcn20/dcn20_fpu.o := $(dml_ccflags) -CFLAGS_$(AMDDALPATH)/dc/dml/dcn20/display_mode_vba_20.o := $(dml_ccflags) +CFLAGS_$(AMDDALPATH)/dc/dml/dcn20/display_mode_vba_20.o := $(dml_ccflags) $(frame_warn_flag) CFLAGS_$(AMDDALPATH)/dc/dml/dcn20/display_rq_dlg_calc_20.o := $(dml_ccflags) -CFLAGS_$(AMDDALPATH)/dc/dml/dcn20/display_mode_vba_20v2.o := $(dml_ccflags) +CFLAGS_$(AMDDALPATH)/dc/dml/dcn20/display_mode_vba_20v2.o := $(dml_ccflags) $(frame_warn_flag) CFLAGS_$(AMDDALPATH)/dc/dml/dcn20/display_rq_dlg_calc_20v2.o := $(dml_ccflags) -CFLAGS_$(AMDDALPATH)/dc/dml/dcn21/display_mode_vba_21.o := $(dml_ccflags) +CFLAGS_$(AMDDALPATH)/dc/dml/dcn21/display_mode_vba_21.o := $(dml_ccflags) $(frame_warn_flag) CFLAGS_$(AMDDALPATH)/dc/dml/dcn21/display_rq_dlg_calc_21.o := $(dml_ccflags) CFLAGS_$(AMDDALPATH)/dc/dml/dcn30/display_mode_vba_30.o := $(dml_ccflags) $(frame_warn_flag) CFLAGS_$(AMDDALPATH)/dc/dml/dcn30/display_rq_dlg_calc_30.o := $(dml_ccflags) --- base-commit: 6813cdca4ab94a238f8eb0cef3d3f3fcbdfb0ee0 change-id: 20240205-amdgpu-raise-flt-for-dml-vba-files-ee5b5a9c5e43 Best regards, -- Nathan Chancellor
Re: [PATCH 1/3] selftests/bpf: Update LLVM Phabricator links
Hi Alexei, On Thu, Jan 11, 2024 at 12:00:50PM -0800, Alexei Starovoitov wrote: > On Thu, Jan 11, 2024 at 11:40 AM Nathan Chancellor wrote: > > > > Hi Yonghong, > > > > On Wed, Jan 10, 2024 at 08:05:36PM -0800, Yonghong Song wrote: > > > > > > On 1/9/24 2:16 PM, Nathan Chancellor wrote: > > > > reviews.llvm.org was LLVM's Phabricator instances for code review. It > > > > has been abandoned in favor of GitHub pull requests. While the majority > > > > of links in the kernel sources still work because of the work Fangrui > > > > has done turning the dynamic Phabricator instance into a static archive, > > > > there are some issues with that work, so preemptively convert all the > > > > links in the kernel sources to point to the commit on GitHub. > > > > > > > > Most of the commits have the corresponding differential review link in > > > > the commit message itself so there should not be any loss of fidelity in > > > > the relevant information. > > > > > > > > Additionally, fix a typo in the xdpwall.c print ("LLMV" -> "LLVM") while > > > > in the area. > > > > > > > > Link: > > > > https://discourse.llvm.org/t/update-on-github-pull-requests/71540/172 > > > > Signed-off-by: Nathan Chancellor > > > > > > Ack with one nit below. > > > > > > Acked-by: Yonghong Song > > > > > > > > > > @@ -304,6 +304,6 @@ from running test_progs will look like: > > > > .. code-block:: console > > > > - test_xdpwall:FAIL:Does LLVM have https://reviews.llvm.org/D109073? > > > > unexpected error: -4007 > > > > + test_xdpwall:FAIL:Does LLVM have > > > > https://github.com/llvm/llvm-project/commit/ea72b0319d7b0f0c2fcf41d121afa5d031b319d5? > > > > unexpected error: -4007 > > > > -__ https://reviews.llvm.org/D109073 > > > > +__ > > > > https://github.com/llvm/llvm-project/commit/ea72b0319d7b0f0c2fcf41d121afa5d031b319d > > > > > > To be consistent with other links, could you add the missing last alnum > > > '5' to the above link? > > > > Thanks a lot for catching this and providing an ack. Andrew, could you > > squash this update into selftests-bpf-update-llvm-phabricator-links.patch? > > Please send a new patch. > We'd like to take all bpf patches through the bpf tree to avoid conflicts. Very well, I've sent a standalone v2 on top of bpf-next: https://lore.kernel.org/20240111-bpf-update-llvm-phabricator-links-v2-1-9a7ae976b...@kernel.org/ Andrew, just drop selftests-bpf-update-llvm-phabricator-links.patch altogether in that case, the other two patches are fine to go via -mm I think. Cheers, Nathan
Re: [PATCH 1/3] selftests/bpf: Update LLVM Phabricator links
Hi Yonghong, On Wed, Jan 10, 2024 at 08:05:36PM -0800, Yonghong Song wrote: > > On 1/9/24 2:16 PM, Nathan Chancellor wrote: > > reviews.llvm.org was LLVM's Phabricator instances for code review. It > > has been abandoned in favor of GitHub pull requests. While the majority > > of links in the kernel sources still work because of the work Fangrui > > has done turning the dynamic Phabricator instance into a static archive, > > there are some issues with that work, so preemptively convert all the > > links in the kernel sources to point to the commit on GitHub. > > > > Most of the commits have the corresponding differential review link in > > the commit message itself so there should not be any loss of fidelity in > > the relevant information. > > > > Additionally, fix a typo in the xdpwall.c print ("LLMV" -> "LLVM") while > > in the area. > > > > Link: https://discourse.llvm.org/t/update-on-github-pull-requests/71540/172 > > Signed-off-by: Nathan Chancellor > > Ack with one nit below. > > Acked-by: Yonghong Song > > @@ -304,6 +304,6 @@ from running test_progs will look like: > > .. code-block:: console > > - test_xdpwall:FAIL:Does LLVM have https://reviews.llvm.org/D109073? > > unexpected error: -4007 > > + test_xdpwall:FAIL:Does LLVM have > > https://github.com/llvm/llvm-project/commit/ea72b0319d7b0f0c2fcf41d121afa5d031b319d5? > > unexpected error: -4007 > > -__ https://reviews.llvm.org/D109073 > > +__ > > https://github.com/llvm/llvm-project/commit/ea72b0319d7b0f0c2fcf41d121afa5d031b319d > > To be consistent with other links, could you add the missing last alnum '5' > to the above link? Thanks a lot for catching this and providing an ack. Andrew, could you squash this update into selftests-bpf-update-llvm-phabricator-links.patch? diff --git a/tools/testing/selftests/bpf/README.rst b/tools/testing/selftests/bpf/README.rst index b9a493f66557..e56034abb3c2 100644 --- a/tools/testing/selftests/bpf/README.rst +++ b/tools/testing/selftests/bpf/README.rst @@ -306,4 +306,4 @@ from running test_progs will look like: test_xdpwall:FAIL:Does LLVM have https://github.com/llvm/llvm-project/commit/ea72b0319d7b0f0c2fcf41d121afa5d031b319d5? unexpected error: -4007 -__ https://github.com/llvm/llvm-project/commit/ea72b0319d7b0f0c2fcf41d121afa5d031b319d +__ https://github.com/llvm/llvm-project/commit/ea72b0319d7b0f0c2fcf41d121afa5d031b319d5
[PATCH] drm/amd/display: Avoid enum conversion warning
Clang warns (or errors with CONFIG_WERROR=y) when performing arithmetic with different enumerated types, which is usually a bug: drivers/gpu/drm/amd/amdgpu/../display/dc/link/protocols/link_dp_dpia_bw.c:548:24: error: arithmetic between different enumeration types ('const enum dc_link_rate' and 'const enum dc_lane_count') [-Werror,-Wenum-enum-conversion] 548 | link_cap->link_rate * link_cap->lane_count * LINK_RATE_REF_FREQ_IN_KHZ * 8; | ~~~ ^ 1 error generated. In this case, there is not a problem because the enumerated types are basically treated as '#define' values. Add an explicit cast to an integral type to silence the warning. Closes: https://github.com/ClangBuiltLinux/linux/issues/1976 Fixes: 5f3bce13266e ("drm/amd/display: Request usb4 bw for mst streams") Signed-off-by: Nathan Chancellor --- drivers/gpu/drm/amd/display/dc/link/protocols/link_dp_dpia_bw.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/drivers/gpu/drm/amd/display/dc/link/protocols/link_dp_dpia_bw.c b/drivers/gpu/drm/amd/display/dc/link/protocols/link_dp_dpia_bw.c index 4ef1a6a1d129..dd0d2b206462 100644 --- a/drivers/gpu/drm/amd/display/dc/link/protocols/link_dp_dpia_bw.c +++ b/drivers/gpu/drm/amd/display/dc/link/protocols/link_dp_dpia_bw.c @@ -544,8 +544,9 @@ int link_dp_dpia_get_dp_overhead_in_dp_tunneling(struct dc_link *link) */ const struct dc_link_settings *link_cap = dc_link_get_link_cap(link); - uint32_t link_bw_in_kbps = - link_cap->link_rate * link_cap->lane_count * LINK_RATE_REF_FREQ_IN_KHZ * 8; + uint32_t link_bw_in_kbps = (uint32_t)link_cap->link_rate * + (uint32_t)link_cap->lane_count * + LINK_RATE_REF_FREQ_IN_KHZ * 8; link_mst_overhead = (link_bw_in_kbps / 64) + ((link_bw_in_kbps % 64) ? 1 : 0); } --- base-commit: 6e7a29f011ac03a765f53844f7c3f04cdd421715 change-id: 20240110-amdgpu-display-enum-enum-conversion-e2451adbf4a7 Best regards, -- Nathan Chancellor
[PATCH 1/3] selftests/bpf: Update LLVM Phabricator links
reviews.llvm.org was LLVM's Phabricator instances for code review. It has been abandoned in favor of GitHub pull requests. While the majority of links in the kernel sources still work because of the work Fangrui has done turning the dynamic Phabricator instance into a static archive, there are some issues with that work, so preemptively convert all the links in the kernel sources to point to the commit on GitHub. Most of the commits have the corresponding differential review link in the commit message itself so there should not be any loss of fidelity in the relevant information. Additionally, fix a typo in the xdpwall.c print ("LLMV" -> "LLVM") while in the area. Link: https://discourse.llvm.org/t/update-on-github-pull-requests/71540/172 Signed-off-by: Nathan Chancellor --- Cc: a...@kernel.org Cc: dan...@iogearbox.net Cc: and...@kernel.org Cc: myko...@fb.com Cc: b...@vger.kernel.org Cc: linux-kselft...@vger.kernel.org --- tools/testing/selftests/bpf/README.rst | 32 +++--- tools/testing/selftests/bpf/prog_tests/xdpwall.c | 2 +- .../selftests/bpf/progs/test_core_reloc_type_id.c | 2 +- 3 files changed, 18 insertions(+), 18 deletions(-) diff --git a/tools/testing/selftests/bpf/README.rst b/tools/testing/selftests/bpf/README.rst index cb9b95702ac6..b9a493f66557 100644 --- a/tools/testing/selftests/bpf/README.rst +++ b/tools/testing/selftests/bpf/README.rst @@ -115,7 +115,7 @@ the insn 20 undoes map_value addition. It is currently impossible for the verifier to understand such speculative pointer arithmetic. Hence `this patch`__ addresses it on the compiler side. It was committed on llvm 12. -__ https://reviews.llvm.org/D85570 +__ https://github.com/llvm/llvm-project/commit/ddf1864ace484035e3cde5e83b3a31ac81e059c6 The corresponding C code @@ -165,7 +165,7 @@ This is due to a llvm BPF backend bug. `The fix`__ has been pushed to llvm 10.x release branch and will be available in 10.0.1. The patch is available in llvm 11.0.0 trunk. -__ https://reviews.llvm.org/D78466 +__ https://github.com/llvm/llvm-project/commit/3cb7e7bf959dcd3b8080986c62e10a75c7af43f0 bpf_verif_scale/loop6.bpf.o test failure with Clang 12 == @@ -204,7 +204,7 @@ r5(w5) is eventually saved on stack at insn #24 for later use. This cause later verifier failure. The bug has been `fixed`__ in Clang 13. -__ https://reviews.llvm.org/D97479 +__ https://github.com/llvm/llvm-project/commit/1959ead525b8830cc8a345f45e1c3ef9902d3229 BPF CO-RE-based tests and Clang version === @@ -221,11 +221,11 @@ failures: - __builtin_btf_type_id() [0_, 1_, 2_]; - __builtin_preserve_type_info(), __builtin_preserve_enum_value() [3_, 4_]. -.. _0: https://reviews.llvm.org/D74572 -.. _1: https://reviews.llvm.org/D74668 -.. _2: https://reviews.llvm.org/D85174 -.. _3: https://reviews.llvm.org/D83878 -.. _4: https://reviews.llvm.org/D83242 +.. _0: https://github.com/llvm/llvm-project/commit/6b01b465388b204d543da3cf49efd6080db094a9 +.. _1: https://github.com/llvm/llvm-project/commit/072cde03aaa13a2c57acf62d79876bf79aa1919f +.. _2: https://github.com/llvm/llvm-project/commit/00602ee7ef0bf6c68d690a2bd729c12b95c95c99 +.. _3: https://github.com/llvm/llvm-project/commit/6d218b4adb093ff2e9764febbbc89f429412006c +.. _4: https://github.com/llvm/llvm-project/commit/6d6750696400e7ce988d66a1a00e1d0cb32815f8 Floating-point tests and Clang version == @@ -234,7 +234,7 @@ Certain selftests, e.g. core_reloc, require support for the floating-point types, which was introduced in `Clang 13`__. The older Clang versions will either crash when compiling these tests, or generate an incorrect BTF. -__ https://reviews.llvm.org/D83289 +__ https://github.com/llvm/llvm-project/commit/a7137b238a07d9399d3ae96c0b461571bd5aa8b2 Kernel function call test and Clang version === @@ -248,7 +248,7 @@ Without it, the error from compiling bpf selftests looks like: libbpf: failed to find BTF for extern 'tcp_slow_start' [25] section: -2 -__ https://reviews.llvm.org/D93563 +__ https://github.com/llvm/llvm-project/commit/886f9ff53155075bd5f1e994f17b85d1e1b7470c btf_tag test and Clang version == @@ -264,8 +264,8 @@ Without them, the btf_tag selftest will be skipped and you will observe: # btf_tag:SKIP -.. _0: https://reviews.llvm.org/D111588 -.. _1: https://reviews.llvm.org/D99 +.. _0: https://github.com/llvm/llvm-project/commit/a162b67c98066218d0d00aa13b99afb95d9bb5e6 +.. _1: https://github.com/llvm/llvm-project/commit/3466e00716e12e32fdb100e3fcfca5c2b3e8d784 Clang dependencies for static linking tests === @@ -274,7 +274,7 @@ linked_vars, linked_maps, and linked_funcs tests depend on `Clang fix`__ to generate valid BTF infor
[PATCH 2/3] arch and include: Update LLVM Phabricator links
reviews.llvm.org was LLVM's Phabricator instances for code review. It has been abandoned in favor of GitHub pull requests. While the majority of links in the kernel sources still work because of the work Fangrui has done turning the dynamic Phabricator instance into a static archive, there are some issues with that work, so preemptively convert all the links in the kernel sources to point to the commit on GitHub. Most of the commits have the corresponding differential review link in the commit message itself so there should not be any loss of fidelity in the relevant information. Link: https://discourse.llvm.org/t/update-on-github-pull-requests/71540/172 Signed-off-by: Nathan Chancellor --- arch/arm64/Kconfig | 4 ++-- arch/riscv/Kconfig | 2 +- arch/riscv/include/asm/ftrace.h | 2 +- include/linux/compiler-clang.h | 2 +- 4 files changed, 5 insertions(+), 5 deletions(-) diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig index 7b071a00425d..3304ba7c6c2a 100644 --- a/arch/arm64/Kconfig +++ b/arch/arm64/Kconfig @@ -380,7 +380,7 @@ config BROKEN_GAS_INST config BUILTIN_RETURN_ADDRESS_STRIPS_PAC bool # Clang's __builtin_return_adddress() strips the PAC since 12.0.0 - # https://reviews.llvm.org/D75044 + # https://github.com/llvm/llvm-project/commit/2a96f47c5ffca84cd774ad402cacd137f4bf45e2 default y if CC_IS_CLANG && (CLANG_VERSION >= 12) # GCC's __builtin_return_address() strips the PAC since 11.1.0, # and this was backported to 10.2.0, 9.4.0, 8.5.0, but not earlier @@ -2202,7 +2202,7 @@ config STACKPROTECTOR_PER_TASK config UNWIND_PATCH_PAC_INTO_SCS bool "Enable shadow call stack dynamically using code patching" - # needs Clang with https://reviews.llvm.org/D111780 incorporated + # needs Clang with https://github.com/llvm/llvm-project/commit/de07cde67b5d205d58690be012106022aea6d2b3 incorporated depends on CC_IS_CLANG && CLANG_VERSION >= 15 depends on ARM64_PTR_AUTH_KERNEL && CC_HAS_BRANCH_PROT_PAC_RET depends on SHADOW_CALL_STACK diff --git a/arch/riscv/Kconfig b/arch/riscv/Kconfig index cd4c9a204d08..f7453eba0b62 100644 --- a/arch/riscv/Kconfig +++ b/arch/riscv/Kconfig @@ -291,7 +291,7 @@ config AS_HAS_INSN def_bool $(as-instr,.insn r 51$(comma) 0$(comma) 0$(comma) t0$(comma) t0$(comma) zero) config AS_HAS_OPTION_ARCH - # https://reviews.llvm.org/D123515 + # https://github.com/llvm/llvm-project/commit/9e8ed3403c191ab9c4903e8eeb8f732ff8a43cb4 def_bool y depends on $(as-instr, .option arch$(comma) +m) depends on !$(as-instr, .option arch$(comma) -i) diff --git a/arch/riscv/include/asm/ftrace.h b/arch/riscv/include/asm/ftrace.h index 2b2f5df7ef2c..3f526404a718 100644 --- a/arch/riscv/include/asm/ftrace.h +++ b/arch/riscv/include/asm/ftrace.h @@ -15,7 +15,7 @@ /* * Clang prior to 13 had "mcount" instead of "_mcount": - * https://reviews.llvm.org/D98881 + * https://github.com/llvm/llvm-project/commit/ef58ae86ba778ed7d01cd3f6bd6d08f943abab44 */ #if defined(CONFIG_CC_IS_GCC) || CONFIG_CLANG_VERSION >= 13 #define MCOUNT_NAME _mcount diff --git a/include/linux/compiler-clang.h b/include/linux/compiler-clang.h index ddab1ef22bee..f0a47afef125 100644 --- a/include/linux/compiler-clang.h +++ b/include/linux/compiler-clang.h @@ -9,7 +9,7 @@ * Clang prior to 17 is being silly and considers many __cleanup() variables * as unused (because they are, their sole purpose is to go out of scope). * - * https://reviews.llvm.org/D152180 + * https://github.com/llvm/llvm-project/commit/877210faa447f4cc7db87812f8ed80e398fedd61 */ #undef __cleanup #define __cleanup(func) __maybe_unused __attribute__((__cleanup__(func))) -- 2.43.0
[PATCH 3/3] treewide: Update LLVM Bugzilla links
LLVM moved their issue tracker from their own Bugzilla instance to GitHub issues. While all of the links are still valid, they may not necessarily show the most up to date information around the issues, as all updates will occur on GitHub, not Bugzilla. Another complication is that the Bugzilla issue number is not always the same as the GitHub issue number. Thankfully, LLVM maintains this mapping through two shortlinks: https://llvm.org/bz -> https://bugs.llvm.org/show_bug.cgi?id= https://llvm.org/pr -> https://github.com/llvm/llvm-project/issues/ Switch all "https://bugs.llvm.org/show_bug.cgi?id=" links to the "https://llvm.org/pr" shortlink so that the links show the most up to date information. Each migrated issue links back to the Bugzilla entry, so there should be no loss of fidelity of information here. Signed-off-by: Nathan Chancellor --- arch/powerpc/Makefile | 4 ++-- arch/powerpc/kvm/book3s_hv_nested.c | 2 +- arch/s390/include/asm/ftrace.h | 2 +- arch/x86/power/Makefile | 2 +- crypto/blake2b_generic.c| 2 +- drivers/firmware/efi/libstub/Makefile | 2 +- drivers/gpu/drm/amd/amdgpu/sdma_v4_4_2.c| 2 +- drivers/media/test-drivers/vicodec/codec-fwht.c | 2 +- drivers/regulator/Kconfig | 2 +- include/asm-generic/vmlinux.lds.h | 2 +- lib/Kconfig.kasan | 2 +- lib/raid6/Makefile | 2 +- lib/stackinit_kunit.c | 2 +- mm/slab_common.c| 2 +- net/bridge/br_multicast.c | 2 +- security/Kconfig| 2 +- 16 files changed, 17 insertions(+), 17 deletions(-) diff --git a/arch/powerpc/Makefile b/arch/powerpc/Makefile index f19dbaa1d541..cd6aaa45f355 100644 --- a/arch/powerpc/Makefile +++ b/arch/powerpc/Makefile @@ -133,11 +133,11 @@ CFLAGS-$(CONFIG_PPC64)+= $(call cc-option,-mno-pointers-to-nested-functions) CFLAGS-$(CONFIG_PPC64) += $(call cc-option,-mlong-double-128) # Clang unconditionally reserves r2 on ppc32 and does not support the flag -# https://bugs.llvm.org/show_bug.cgi?id=39555 +# https://llvm.org/pr39555 CFLAGS-$(CONFIG_PPC32) := $(call cc-option, -ffixed-r2) # Clang doesn't support -mmultiple / -mno-multiple -# https://bugs.llvm.org/show_bug.cgi?id=39556 +# https://llvm.org/pr39556 CFLAGS-$(CONFIG_PPC32) += $(call cc-option, $(MULTIPLEWORD)) CFLAGS-$(CONFIG_PPC32) += $(call cc-option,-mno-readonly-in-sdata) diff --git a/arch/powerpc/kvm/book3s_hv_nested.c b/arch/powerpc/kvm/book3s_hv_nested.c index 3b658b8696bc..3f5970f74c6b 100644 --- a/arch/powerpc/kvm/book3s_hv_nested.c +++ b/arch/powerpc/kvm/book3s_hv_nested.c @@ -55,7 +55,7 @@ void kvmhv_save_hv_regs(struct kvm_vcpu *vcpu, struct hv_guest_state *hr) hr->dawrx1 = vcpu->arch.dawrx1; } -/* Use noinline_for_stack due to https://bugs.llvm.org/show_bug.cgi?id=49610 */ +/* Use noinline_for_stack due to https://llvm.org/pr49610 */ static noinline_for_stack void byteswap_pt_regs(struct pt_regs *regs) { unsigned long *addr = (unsigned long *) regs; diff --git a/arch/s390/include/asm/ftrace.h b/arch/s390/include/asm/ftrace.h index 5a82b08f03cd..621f23d5ae30 100644 --- a/arch/s390/include/asm/ftrace.h +++ b/arch/s390/include/asm/ftrace.h @@ -9,7 +9,7 @@ #ifndef __ASSEMBLY__ #ifdef CONFIG_CC_IS_CLANG -/* https://bugs.llvm.org/show_bug.cgi?id=41424 */ +/* https://llvm.org/pr41424 */ #define ftrace_return_address(n) 0UL #else #define ftrace_return_address(n) __builtin_return_address(n) diff --git a/arch/x86/power/Makefile b/arch/x86/power/Makefile index 379777572bc9..e0cd7afd5302 100644 --- a/arch/x86/power/Makefile +++ b/arch/x86/power/Makefile @@ -5,7 +5,7 @@ CFLAGS_cpu.o := -fno-stack-protector # Clang may incorrectly inline functions with stack protector enabled into -# __restore_processor_state(): https://bugs.llvm.org/show_bug.cgi?id=47479 +# __restore_processor_state(): https://llvm.org/pr47479 CFLAGS_REMOVE_cpu.o := $(CC_FLAGS_LTO) obj-$(CONFIG_PM_SLEEP) += cpu.o diff --git a/crypto/blake2b_generic.c b/crypto/blake2b_generic.c index 6704c0355889..32e380b714b6 100644 --- a/crypto/blake2b_generic.c +++ b/crypto/blake2b_generic.c @@ -102,7 +102,7 @@ static void blake2b_compress_one_generic(struct blake2b_state *S, ROUND(10); ROUND(11); #ifdef CONFIG_CC_IS_CLANG -#pragma nounroll /* https://bugs.llvm.org/show_bug.cgi?id=45803 */ +#pragma nounroll /* https://llvm.org/pr45803 */ #endif for (i = 0; i < 8; ++i) S->h[i] = S->h[i] ^ v[i] ^ v[i + 8]; diff --git a/drivers/firmware/efi/libstub/Makefile b/drivers/firmware/efi/libstub/Makefile index 06964a3c130f..a223bd10564b 100644 --- a/drivers/firmware/efi/libstub/Makefile +++ b/drivers/firmware/
[PATCH 0/3] Update LLVM Phabricator and Bugzilla links
This series updates all instances of LLVM Phabricator and Bugzilla links to point to GitHub commits directly and LLVM's Bugzilla to GitHub issue shortlinks respectively. I split up the Phabricator patch into BPF selftests and the rest of the kernel in case the BPF folks want to take it separately from the rest of the series, there are obviously no dependency issues in that case. The Bugzilla change was mechanical enough and should have no conflicts. I am aiming this at Andrew and CC'ing other lists, in case maintainers want to chime in, but I think this is pretty uncontroversial (famous last words...). --- Nathan Chancellor (3): selftests/bpf: Update LLVM Phabricator links arch and include: Update LLVM Phabricator links treewide: Update LLVM Bugzilla links arch/arm64/Kconfig | 4 +-- arch/powerpc/Makefile | 4 +-- arch/powerpc/kvm/book3s_hv_nested.c| 2 +- arch/riscv/Kconfig | 2 +- arch/riscv/include/asm/ftrace.h| 2 +- arch/s390/include/asm/ftrace.h | 2 +- arch/x86/power/Makefile| 2 +- crypto/blake2b_generic.c | 2 +- drivers/firmware/efi/libstub/Makefile | 2 +- drivers/gpu/drm/amd/amdgpu/sdma_v4_4_2.c | 2 +- drivers/media/test-drivers/vicodec/codec-fwht.c| 2 +- drivers/regulator/Kconfig | 2 +- include/asm-generic/vmlinux.lds.h | 2 +- include/linux/compiler-clang.h | 2 +- lib/Kconfig.kasan | 2 +- lib/raid6/Makefile | 2 +- lib/stackinit_kunit.c | 2 +- mm/slab_common.c | 2 +- net/bridge/br_multicast.c | 2 +- security/Kconfig | 2 +- tools/testing/selftests/bpf/README.rst | 32 +++--- tools/testing/selftests/bpf/prog_tests/xdpwall.c | 2 +- .../selftests/bpf/progs/test_core_reloc_type_id.c | 2 +- 23 files changed, 40 insertions(+), 40 deletions(-) --- base-commit: 0dd3ee31125508cd67f7e7172247f05b7fd1753a change-id: 20240109-update-llvm-links-d03f9d649e1e Best regards, -- Nathan Chancellor
[PATCH 3/3] drm/bridge: Return NULL instead of plain 0 in drm_dp_hpd_bridge_register() stub
sparse complains: drivers/usb/typec/tcpm/qcom/qcom_pmic_typec.c: note: in included file: include/drm/bridge/aux-bridge.h:29:16: sparse: sparse: Using plain integer as NULL pointer Return NULL to clear up the warning. Reported-by: kernel test robot Closes: https://lore.kernel.org/oe-kbuild-all/202312060025.bdeqzrwx-...@intel.com/ Fixes: e560518a6c2e ("drm/bridge: implement generic DP HPD bridge") Signed-off-by: Nathan Chancellor --- include/drm/bridge/aux-bridge.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/include/drm/bridge/aux-bridge.h b/include/drm/bridge/aux-bridge.h index 66249ff0858e..c4c423e97f06 100644 --- a/include/drm/bridge/aux-bridge.h +++ b/include/drm/bridge/aux-bridge.h @@ -26,7 +26,7 @@ void drm_aux_hpd_bridge_notify(struct device *dev, enum drm_connector_status sta static inline struct device *drm_dp_hpd_bridge_register(struct device *parent, struct device_node *np) { - return 0; + return NULL; } static inline void drm_aux_hpd_bridge_notify(struct device *dev, enum drm_connector_status status) -- 2.43.0
[PATCH 2/3] usb: typec: qcom-pmic-typec: Only select DRM_AUX_HPD_BRIDGE with OF
CONFIG_DRM_AUX_HPD_BRIDGE depends on CONFIG_OF but that dependency is not included when CONFIG_TYPEC_QCOM_PMIC selects it, resulting in a Kconfig warning when CONFIG_OF is disabled: WARNING: unmet direct dependencies detected for DRM_AUX_HPD_BRIDGE Depends on [n]: HAS_IOMEM [=y] && DRM_BRIDGE [=y] && OF [=n] Selected by [m]: - TYPEC_QCOM_PMIC [=m] && USB_SUPPORT [=y] && TYPEC [=m] && TYPEC_TCPM [=m] && (ARCH_QCOM || COMPILE_TEST [=y]) && (DRM [=m] || DRM [=m]=n) && DRM_BRIDGE [=y] Only select CONFIG_DRM_AUX_HPD_BRIDGE with both CONFIG_DRM_BRIDGE and CONFIG_OF to clear up the warning. Fixes: 7d9f1b72b296 ("usb: typec: qcom-pmic-typec: switch to DRM_AUX_HPD_BRIDGE") Signed-off-by: Nathan Chancellor --- drivers/usb/typec/tcpm/Kconfig | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/usb/typec/tcpm/Kconfig b/drivers/usb/typec/tcpm/Kconfig index 64d5421c69e6..8cdd84ca5d6f 100644 --- a/drivers/usb/typec/tcpm/Kconfig +++ b/drivers/usb/typec/tcpm/Kconfig @@ -80,7 +80,7 @@ config TYPEC_QCOM_PMIC tristate "Qualcomm PMIC USB Type-C Port Controller Manager driver" depends on ARCH_QCOM || COMPILE_TEST depends on DRM || DRM=n - select DRM_AUX_HPD_BRIDGE if DRM_BRIDGE + select DRM_AUX_HPD_BRIDGE if DRM_BRIDGE && OF help A Type-C port and Power Delivery driver which aggregates two discrete pieces of silicon in the PM8150b PMIC block: the -- 2.43.0
[PATCH 0/3] A few fixes for transparent bridge support
Hi all, This series fixes two Kconfig issues that I noticed with the selection of CONFIG_DRM_AUX{,_HPD}_BRIDGE along with a fix for a sparse report that I noticed while seeing if these had already been resolved. --- Nathan Chancellor (3): usb: typec: nb7vpq904m: Only select DRM_AUX_BRIDGE with OF usb: typec: qcom-pmic-typec: Only select DRM_AUX_HPD_BRIDGE with OF drm/bridge: Return NULL instead of plain 0 in drm_dp_hpd_bridge_register() stub drivers/usb/typec/mux/Kconfig | 2 +- drivers/usb/typec/tcpm/Kconfig | 2 +- include/drm/bridge/aux-bridge.h | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) --- base-commit: 4900e0396e59be233cfa636369d4eec6b40dbeca change-id: 20231205-drm_aux_bridge-fixes-162780ed704a Best regards, -- Nathan Chancellor
[PATCH 1/3] usb: typec: nb7vpq904m: Only select DRM_AUX_BRIDGE with OF
CONFIG_DRM_AUX_BRIDGE depends on CONFIG_OF but that dependency is not included when CONFIG_TYPEC_MUX_NB7VPQ904M selects it, resulting in a Kconfig warning when CONFIG_OF is disabled: WARNING: unmet direct dependencies detected for DRM_AUX_BRIDGE Depends on [n]: HAS_IOMEM [=y] && DRM_BRIDGE [=y] && OF [=n] Selected by [y]: - TYPEC_MUX_NB7VPQ904M [=y] && USB_SUPPORT [=y] && TYPEC [=y] && I2C [=y] && (DRM [=y] || DRM [=y]=n) && DRM_BRIDGE [=y] Only select CONFIG_DRM_AUX_BRIDGE with both CONFIG_DRM_BRIDGE and CONFIG_OF to clear up the warning. Fixes: c5d296bad640 ("usb: typec: nb7vpq904m: switch to DRM_AUX_BRIDGE") Signed-off-by: Nathan Chancellor --- drivers/usb/typec/mux/Kconfig | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/usb/typec/mux/Kconfig b/drivers/usb/typec/mux/Kconfig index 5120942f309d..38416fb0cc3c 100644 --- a/drivers/usb/typec/mux/Kconfig +++ b/drivers/usb/typec/mux/Kconfig @@ -40,7 +40,7 @@ config TYPEC_MUX_NB7VPQ904M tristate "On Semiconductor NB7VPQ904M Type-C redriver driver" depends on I2C depends on DRM || DRM=n - select DRM_AUX_BRIDGE if DRM_BRIDGE + select DRM_AUX_BRIDGE if DRM_BRIDGE && OF select REGMAP_I2C help Say Y or M if your system has a On Semiconductor NB7VPQ904M Type-C -- 2.43.0
Re: [RFC] drm: enable W=1 warnings by default across the subsystem
On Thu, Nov 30, 2023 at 10:52:17AM +0200, Jani Nikula wrote: > On Wed, 29 Nov 2023, Hamza Mahfooz wrote: > > Cc: Nathan Chancellor > > > > 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. For what it's worth, I ran this through my personal build matrix with LLVM/clang and it only found a few instances of -Wunused-but-set-variable: drivers/gpu/drm/mediatek/mtk_disp_gamma.c:121:6: warning: variable 'cfg_val' set but not used [-Wunused-but-set-variable] drivers/gpu/drm/nouveau/nouveau_svm.c:115:24: warning: variable 'priority' set but not used [-Wunused-but-set-variable] drivers/gpu/drm/nouveau/nouveau_svm.c:929:6: warning: variable 'ret' set but not used [-Wunused-but-set-variable] drivers/gpu/drm/nouveau/nvkm/subdev/acr/lsfw.c:221:7: warning: variable 'loc' set but not used [-Wunused-but-set-variable] drivers/gpu/drm/qxl/qxl_cmd.c:424:6: warning: variable 'count' set but not used [-Wunused-but-set-variable] drivers/gpu/drm/qxl/qxl_ioctl.c:148:14: warning: variable 'num_relocs' set but not used [-Wunused-but-set-variable] I know that clang does not support all the warnings that are being enabled here but I suspect there won't be too many instances of the other warnings. -Wformat-overflow and -Wformat-truncation might be the big ones. I know -Wstringop-overflow is being turned on globally in -next so that is one that you shouldn't have to worry much about. Cheers, Nathan
Re: [PATCH 3/3] drm/amd/display: Support DRM_AMD_DC_FP on RISC-V
On Thu, Nov 23, 2023 at 02:23:01PM +, Conor Dooley wrote: > On Tue, Nov 21, 2023 at 07:05:15PM -0800, Samuel Holland wrote: > > RISC-V uses kernel_fpu_begin()/kernel_fpu_end() like several other > > architectures. Enabling hardware FP requires overriding the ISA string > > for the relevant compilation units. > > Ah yes, bringing the joy of frame-larger-than warnings to RISC-V: > ../drivers/gpu/drm/amd/amdgpu/../display/dc/dml/dcn32/display_mode_vba_32.c:58:13: > warning: stack frame size (2416) exceeds limit (2048) in > 'DISPCLKDPPCLKDCFCLKDeepSleepPrefetchParametersWatermarksAndPerformanceCalculation' > [-Wframe-larger-than] :( > Nathan, have you given up on these being sorted out? Does your configuration have KASAN (I don't think RISC-V supports KCSAN)? It is possible that dml/dcn32 needs something similar to commit 6740ec97bcdb ("drm/amd/display: Increase frame warning limit with KASAN or KCSAN in dml2")? I am not really interested in playing whack-a-mole with these warnings like I have done in the past for the reasons I outlined here: https://lore.kernel.org/20231019205117.GA839902@dev-arch.thelio-3990X/ > Also, what on earth is that function name, it exceeds 80 characters > before even considering anything else? Actually, I don't think I want > to know. Welcome to "gcc-parsable HW gospel, coming straight from HW engineers" :) Cheers, Nathan
Re: [PATCH 1/3] kunit: Add a macro to wrap a deferred action function
0x1a/0x30 [08:06:03] [08:06:03] ---[ end trace ]--- [08:06:03] RIP: 0010:__kunit_action_free+0x18/0x20 ... With this series applied with https://lore.kernel.org/20231106172557.2963-1...@opensource.cirrus.com/, all the tests pass for arm64 and x86_64 on my machine. I see no remaining casts in the tree in this state. It seems like the documentation in Documentation/dev-tools/kunit/usage.rst may want to be updated to remove mention of casting to kunit_action_t as well? Regardless: Reviewed-by: Nathan Chancellor Tested-by: Nathan Chancellor > include/kunit/resource.h | 9 + > lib/kunit/kunit-test.c | 5 + > lib/kunit/test.c | 6 -- > 3 files changed, 14 insertions(+), 6 deletions(-) > > diff --git a/include/kunit/resource.h b/include/kunit/resource.h > index c7383e90f5c9..4110e13970dc 100644 > --- a/include/kunit/resource.h > +++ b/include/kunit/resource.h > @@ -390,6 +390,15 @@ void kunit_remove_resource(struct kunit *test, struct > kunit_resource *res); > /* A 'deferred action' function to be used with kunit_add_action. */ > typedef void (kunit_action_t)(void *); > > +/* We can't cast function pointers to kunit_action_t if CFI is enabled. */ > +#define KUNIT_DEFINE_ACTION_WRAPPER(wrapper, orig, arg_type) \ > + static void wrapper(void *in) \ > + { \ > + arg_type arg = (arg_type)in; \ > + orig(arg); \ > + } > + > + > /** > * kunit_add_action() - Call a function when the test ends. > * @test: Test case to associate the action with. > diff --git a/lib/kunit/kunit-test.c b/lib/kunit/kunit-test.c > index de2113a58fa0..ee6927c60979 100644 > --- a/lib/kunit/kunit-test.c > +++ b/lib/kunit/kunit-test.c > @@ -538,10 +538,7 @@ static struct kunit_suite kunit_resource_test_suite = { > #if IS_BUILTIN(CONFIG_KUNIT_TEST) > > /* This avoids a cast warning if kfree() is passed direct to > kunit_add_action(). */ > -static void kfree_wrapper(void *p) > -{ > - kfree(p); > -} > +KUNIT_DEFINE_ACTION_WRAPPER(kfree_wrapper, kfree, const void *); > > static void kunit_log_test(struct kunit *test) > { > diff --git a/lib/kunit/test.c b/lib/kunit/test.c > index f2eb71f1a66c..0308865194bb 100644 > --- a/lib/kunit/test.c > +++ b/lib/kunit/test.c > @@ -772,6 +772,8 @@ static struct notifier_block kunit_mod_nb = { > }; > #endif > > +KUNIT_DEFINE_ACTION_WRAPPER(kfree_action_wrapper, kfree, const void *) > + > void *kunit_kmalloc_array(struct kunit *test, size_t n, size_t size, gfp_t > gfp) > { > void *data; > @@ -781,7 +783,7 @@ void *kunit_kmalloc_array(struct kunit *test, size_t n, > size_t size, gfp_t gfp) > if (!data) > return NULL; > > - if (kunit_add_action_or_reset(test, (kunit_action_t *)kfree, data) != 0) > + if (kunit_add_action_or_reset(test, kfree_action_wrapper, data) != 0) > return NULL; > > return data; > @@ -793,7 +795,7 @@ void kunit_kfree(struct kunit *test, const void *ptr) > if (!ptr) > return; > > - kunit_release_action(test, (kunit_action_t *)kfree, (void *)ptr); > + kunit_release_action(test, kfree_action_wrapper, (void *)ptr); > } > EXPORT_SYMBOL_GPL(kunit_kfree); > > -- > 2.42.0.869.gea05f2083d-goog >
Re: [PATCH 02/22] fb: atmel_lcdfb: Stop using platform_driver_probe()
On Tue, Nov 07, 2023 at 10:17:43AM +0100, Uwe Kleine-König wrote: > On today's platforms the benefit of platform_driver_probe() isn't that > relevant any more. It allows to drop some code after booting (or module > loading) for .probe() and discard the .remove() function completely if > the driver is built-in. This typically saves a few 100k. > > The downside of platform_driver_probe() is that the driver cannot be > bound and unbound at runtime which is ancient and also slightly > complicates testing. There are also thoughts to deprecate > platform_driver_probe() because it adds some complexity in the driver > core for little gain. Also many drivers don't use it correctly. This > driver for example misses to mark the driver struct with __refdata which > is needed to suppress a (W=1) modpost warning: > > WARNING: modpost: drivers/video/fbdev/atmel_lcdfb: section mismatch in > reference: atmel_lcdfb_driver+0x4 (section: .data) -> atmel_lcdfb_remove > (section: .exit.text) > > Signed-off-by: Uwe Kleine-König > --- > drivers/video/fbdev/atmel_lcdfb.c | 9 + > 1 file changed, 5 insertions(+), 4 deletions(-) > > diff --git a/drivers/video/fbdev/atmel_lcdfb.c > b/drivers/video/fbdev/atmel_lcdfb.c > index a908db233409..b218731ef732 100644 > --- a/drivers/video/fbdev/atmel_lcdfb.c > +++ b/drivers/video/fbdev/atmel_lcdfb.c > @@ -1017,7 +1017,7 @@ static int atmel_lcdfb_of_init(struct atmel_lcdfb_info > *sinfo) > return ret; > } > > -static int __init atmel_lcdfb_probe(struct platform_device *pdev) > +static int atmel_lcdfb_probe(struct platform_device *pdev) > { > struct device *dev = &pdev->dev; > struct fb_info *info; > @@ -1223,7 +1223,7 @@ static int __init atmel_lcdfb_probe(struct > platform_device *pdev) > return ret; > } > > -static int __exit atmel_lcdfb_remove(struct platform_device *pdev) > +static int atmel_lcdfb_remove(struct platform_device *pdev) > { > struct device *dev = &pdev->dev; > struct fb_info *info = dev_get_drvdata(dev); > @@ -1301,7 +1301,8 @@ static int atmel_lcdfb_resume(struct platform_device > *pdev) > #endif > > static struct platform_driver atmel_lcdfb_driver = { > - .remove = __exit_p(atmel_lcdfb_remove), > + .probe = atmel_lcdfb_probe, > + .remove = atmel_lcdfb_remove, > .suspend= atmel_lcdfb_suspend, > .resume = atmel_lcdfb_resume, > .driver = { > @@ -1310,7 +1311,7 @@ static struct platform_driver atmel_lcdfb_driver = { > }, > }; > > -module_platform_driver_probe(atmel_lcdfb_driver, atmel_lcdfb_probe); > +module_platform_driver(atmel_lcdfb_driver, ); > > MODULE_DESCRIPTION("AT91 LCD Controller framebuffer driver"); > MODULE_AUTHOR("Nicolas Ferre "); > -- > 2.42.0 > For what it's worth, this introduces a warning when building certain configurations (such as ARCH=arm multi_v5_defconfig) with clang: WARNING: modpost: vmlinux: section mismatch in reference: atmel_lcdfb_probe+0x6c4 (section: .text) -> atmel_lcdfb_init_fbinfo (section: .init.text) WARNING: modpost: vmlinux: section mismatch in reference: atmel_lcdfb_probe+0x858 (section: .text) -> atmel_lcdfb_fix (section: .init.rodata) This appears to be legitimate to me? GCC did not warn but I assume that is due to differences in inlining. The following clears it up for me, should I send a standalone patch or should this be squashed in? Cheers, Nathan diff --git a/drivers/video/fbdev/atmel_lcdfb.c b/drivers/video/fbdev/atmel_lcdfb.c index 88c75ae7d315..9e391e5eaf9d 100644 --- a/drivers/video/fbdev/atmel_lcdfb.c +++ b/drivers/video/fbdev/atmel_lcdfb.c @@ -220,7 +220,7 @@ static inline void atmel_lcdfb_power_control(struct atmel_lcdfb_info *sinfo, int } } -static const struct fb_fix_screeninfo atmel_lcdfb_fix __initconst = { +static const struct fb_fix_screeninfo atmel_lcdfb_fix = { .type = FB_TYPE_PACKED_PIXELS, .visual = FB_VISUAL_TRUECOLOR, .xpanstep = 0, @@ -841,7 +841,7 @@ static void atmel_lcdfb_task(struct work_struct *work) atmel_lcdfb_reset(sinfo); } -static int __init atmel_lcdfb_init_fbinfo(struct atmel_lcdfb_info *sinfo) +static int atmel_lcdfb_init_fbinfo(struct atmel_lcdfb_info *sinfo) { struct fb_info *info = sinfo->info; int ret = 0;
Re: [PATCH] drm/amd/display: Increase frame warning limit for clang in dml2
On Thu, Nov 02, 2023 at 12:59:00PM -0400, Hamza Mahfooz wrote: > On 11/2/23 12:24, Nathan Chancellor wrote: > > When building ARCH=x86_64 allmodconfig with clang, which have sanitizers > > enabled, there is a warning about a large stack frame. > > > > > > drivers/gpu/drm/amd/amdgpu/../display/dc/dml2/display_mode_core.c:6265:13: > > error: stack frame size (2520) exceeds limit (2048) in 'dml_prefetch_check' > > [-Werror,-Wframe-larger-than] > > 6265 | static void dml_prefetch_check(struct display_mode_lib_st > > *mode_lib) > > | ^ > >1 error generated. > > > > Notably, GCC 13.2.0 does not do too much of a better job, as it is right > > at the current limit of 2048: > > > >drivers/gpu/drm/amd/amdgpu/../display/dc/dml2/display_mode_core.c: In > > function 'dml_prefetch_check': > > > > drivers/gpu/drm/amd/amdgpu/../display/dc/dml2/display_mode_core.c:6705:1: > > error: the frame size of 2048 bytes is larger than 1800 bytes > > [-Werror=frame-larger-than=] > > 6705 | } > > | ^ > > > > In the past, these warnings have been avoided by reducing the number of > > parameters to various functions so that not as many arguments need to be > > passed on the stack. However, these patches take a good amount of effort > > to write despite being mechanical due to code structure and complexity > > and they are never carried forward to new generations of the code so > > that effort has to be expended every new hardware generation, which > > becomes harder to justify as time goes on. > > > > There is some effort to improve clang's code generation but that may > > take some time between code review, shifting priorities, and release > > cycles. To avoid having a noticeable or lengthy breakage in > > all{mod,yes}config, which are easy testing targets that have -Werror > > enabled, increase the limit for clang by 50% so that cases of extremely > > poor code generation can still be caught while not breaking the majority > > of builds. When clang's code generation improves, the limit increase can > > be restricted to older clang versions. > > > > Signed-off-by: Nathan Chancellor > > --- > > If there is another DRM pull before 6.7-rc1, it would be much > > appreciated if this could make that so that other trees are not > > potentially broken by this. If not, no worries, as it was my fault for > > not sending this sooner. > > --- > > drivers/gpu/drm/amd/display/dc/dml2/Makefile | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/drivers/gpu/drm/amd/display/dc/dml2/Makefile > > b/drivers/gpu/drm/amd/display/dc/dml2/Makefile > > index 70ae5eba624e..dff8237c0999 100644 > > --- a/drivers/gpu/drm/amd/display/dc/dml2/Makefile > > +++ b/drivers/gpu/drm/amd/display/dc/dml2/Makefile > > @@ -60,7 +60,7 @@ endif > > endif > > ifneq ($(CONFIG_FRAME_WARN),0) > > -frame_warn_flag := -Wframe-larger-than=2048 > > +frame_warn_flag := -Wframe-larger-than=$(if > > $(CONFIG_CC_IS_CLANG),3072,2048) > > I would prefer checking for `CONFIG_KASAN || CONFIG_KCSAN` instead > since the stack usage shouldn't change much if both of those are disabled. So something like this? Or were you talking about replacing the clang check entirely with the KASAN/KCSAN check? diff --git a/drivers/gpu/drm/amd/display/dc/dml2/Makefile b/drivers/gpu/drm/amd/display/dc/dml2/Makefile index 70ae5eba624e..0fc1b13295eb 100644 --- a/drivers/gpu/drm/amd/display/dc/dml2/Makefile +++ b/drivers/gpu/drm/amd/display/dc/dml2/Makefile @@ -60,8 +60,12 @@ endif endif ifneq ($(CONFIG_FRAME_WARN),0) +ifeq ($(CONFIG_CC_IS_CLANG)$(filter y,$(CONFIG_KASAN)$(CONFIG_KCSAN)),yy) +frame_warn_flag := -Wframe-larger-than=3072 +else frame_warn_flag := -Wframe-larger-than=2048 endif +endif CFLAGS_$(AMDDALPATH)/dc/dml2/display_mode_core.o := $(dml2_ccflags) $(frame_warn_flag) CFLAGS_$(AMDDALPATH)/dc/dml2/display_mode_util.o := $(dml2_ccflags) > > endif > > CFLAGS_$(AMDDALPATH)/dc/dml2/display_mode_core.o := $(dml2_ccflags) > > $(frame_warn_flag) > > > > --- > > base-commit: 21e80f3841c01aeaf32d7aee7bbc87b3db1aa0c6 > > change-id: > > 20231102-amdgpu-dml2-increase-frame-size-warning-for-clang-c93bd2d6a871 > > > > Best regards, > -- > Hamza >
[PATCH v2] drm/amd/display: Increase frame warning limit with KASAN or KCSAN in dml2
When building ARCH=x86_64 allmodconfig with clang, which will typically have sanitizers enabled, there is a warning about a large stack frame. drivers/gpu/drm/amd/amdgpu/../display/dc/dml2/display_mode_core.c:6265:13: error: stack frame size (2520) exceeds limit (2048) in 'dml_prefetch_check' [-Werror,-Wframe-larger-than] 6265 | static void dml_prefetch_check(struct display_mode_lib_st *mode_lib) | ^ 1 error generated. Notably, GCC 13.2.0 does not do too much of a better job, as it is right at the current limit of 2048 (and others have reported being over with older GCC versions): drivers/gpu/drm/amd/amdgpu/../display/dc/dml2/display_mode_core.c: In function 'dml_prefetch_check': drivers/gpu/drm/amd/amdgpu/../display/dc/dml2/display_mode_core.c:6705:1: error: the frame size of 2048 bytes is larger than 1800 bytes [-Werror=frame-larger-than=] 6705 | } | ^ In the past, these warnings have been avoided by reducing the number of parameters to various functions so that not as many arguments need to be passed on the stack. However, these patches take a good amount of effort to write despite being mechanical due to code structure and complexity and they are never carried forward to new generations of the code so that effort has to be expended every new hardware generation, which becomes harder to justify as time goes on. To avoid having a noticeable or lengthy breakage in all{mod,yes}config, which are easy testing targets that have -Werror enabled, increase the limit for configurations that have KASAN or KCSAN enabled by 50% so that cases of extremely poor code generation can still be caught while not breaking the majority of builds. CONFIG_KMSAN also causes high stack usage but the frame limit is already set to zero when it is enabled, which is accounted for by the check for CONFIG_FRAME_WARN=0 in the dml2 Makefile. Signed-off-by: Nathan Chancellor --- If there is another DRM pull before 6.7-rc1, it would be much appreciated if this could make that so that other trees are not potentially broken by this. If not, no worries, as it was my fault for not sending this sooner. Changes in v2: - Adjust workaround to check for either CONFIG_KASAN=y or CONFIG_KCSAN=y, as the same problem has been reported with older versions of GCC (Hamza, Alex) - Link to v1: https://lore.kernel.org/r/20231102-amdgpu-dml2-increase-frame-size-warning-for-clang-v1-1-6eb157352...@kernel.org --- drivers/gpu/drm/amd/display/dc/dml2/Makefile | 4 1 file changed, 4 insertions(+) diff --git a/drivers/gpu/drm/amd/display/dc/dml2/Makefile b/drivers/gpu/drm/amd/display/dc/dml2/Makefile index 70ae5eba624e..acff3449b8d7 100644 --- a/drivers/gpu/drm/amd/display/dc/dml2/Makefile +++ b/drivers/gpu/drm/amd/display/dc/dml2/Makefile @@ -60,8 +60,12 @@ endif endif ifneq ($(CONFIG_FRAME_WARN),0) +ifeq ($(filter y,$(CONFIG_KASAN)$(CONFIG_KCSAN)),y) +frame_warn_flag := -Wframe-larger-than=3072 +else frame_warn_flag := -Wframe-larger-than=2048 endif +endif CFLAGS_$(AMDDALPATH)/dc/dml2/display_mode_core.o := $(dml2_ccflags) $(frame_warn_flag) CFLAGS_$(AMDDALPATH)/dc/dml2/display_mode_util.o := $(dml2_ccflags) --- base-commit: 21e80f3841c01aeaf32d7aee7bbc87b3db1aa0c6 change-id: 20231102-amdgpu-dml2-increase-frame-size-warning-for-clang-c93bd2d6a871 Best regards, -- Nathan Chancellor
[PATCH] drm/amd/display: Increase frame warning limit for clang in dml2
When building ARCH=x86_64 allmodconfig with clang, which have sanitizers enabled, there is a warning about a large stack frame. drivers/gpu/drm/amd/amdgpu/../display/dc/dml2/display_mode_core.c:6265:13: error: stack frame size (2520) exceeds limit (2048) in 'dml_prefetch_check' [-Werror,-Wframe-larger-than] 6265 | static void dml_prefetch_check(struct display_mode_lib_st *mode_lib) | ^ 1 error generated. Notably, GCC 13.2.0 does not do too much of a better job, as it is right at the current limit of 2048: drivers/gpu/drm/amd/amdgpu/../display/dc/dml2/display_mode_core.c: In function 'dml_prefetch_check': drivers/gpu/drm/amd/amdgpu/../display/dc/dml2/display_mode_core.c:6705:1: error: the frame size of 2048 bytes is larger than 1800 bytes [-Werror=frame-larger-than=] 6705 | } | ^ In the past, these warnings have been avoided by reducing the number of parameters to various functions so that not as many arguments need to be passed on the stack. However, these patches take a good amount of effort to write despite being mechanical due to code structure and complexity and they are never carried forward to new generations of the code so that effort has to be expended every new hardware generation, which becomes harder to justify as time goes on. There is some effort to improve clang's code generation but that may take some time between code review, shifting priorities, and release cycles. To avoid having a noticeable or lengthy breakage in all{mod,yes}config, which are easy testing targets that have -Werror enabled, increase the limit for clang by 50% so that cases of extremely poor code generation can still be caught while not breaking the majority of builds. When clang's code generation improves, the limit increase can be restricted to older clang versions. Signed-off-by: Nathan Chancellor --- If there is another DRM pull before 6.7-rc1, it would be much appreciated if this could make that so that other trees are not potentially broken by this. If not, no worries, as it was my fault for not sending this sooner. --- drivers/gpu/drm/amd/display/dc/dml2/Makefile | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/gpu/drm/amd/display/dc/dml2/Makefile b/drivers/gpu/drm/amd/display/dc/dml2/Makefile index 70ae5eba624e..dff8237c0999 100644 --- a/drivers/gpu/drm/amd/display/dc/dml2/Makefile +++ b/drivers/gpu/drm/amd/display/dc/dml2/Makefile @@ -60,7 +60,7 @@ endif endif ifneq ($(CONFIG_FRAME_WARN),0) -frame_warn_flag := -Wframe-larger-than=2048 +frame_warn_flag := -Wframe-larger-than=$(if $(CONFIG_CC_IS_CLANG),3072,2048) endif CFLAGS_$(AMDDALPATH)/dc/dml2/display_mode_core.o := $(dml2_ccflags) $(frame_warn_flag) --- base-commit: 21e80f3841c01aeaf32d7aee7bbc87b3db1aa0c6 change-id: 20231102-amdgpu-dml2-increase-frame-size-warning-for-clang-c93bd2d6a871 Best regards, -- Nathan Chancellor
[PATCH] drm/amd/display: Respect CONFIG_FRAME_WARN=0 in DML2
display_mode_code.c is unconditionally built with -Wframe-larger-than=2048, which causes warnings even when CONFIG_FRAME_WARN has been set to 0, which should show no warnings. Use the existing $(frame_warn_flag) variable, which handles this situation. This is basically commit 25f178bbd078 ("drm/amd/display: Respect CONFIG_FRAME_WARN=0 in dml Makefile") but for DML2. Fixes: 7966f319c66d ("drm/amd/display: Introduce DML2") Signed-off-by: Nathan Chancellor --- drivers/gpu/drm/amd/display/dc/dml2/Makefile | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/gpu/drm/amd/display/dc/dml2/Makefile b/drivers/gpu/drm/amd/display/dc/dml2/Makefile index f35ed8de260d..66431525f2a0 100644 --- a/drivers/gpu/drm/amd/display/dc/dml2/Makefile +++ b/drivers/gpu/drm/amd/display/dc/dml2/Makefile @@ -61,7 +61,7 @@ ifneq ($(CONFIG_FRAME_WARN),0) frame_warn_flag := -Wframe-larger-than=2048 endif -CFLAGS_$(AMDDALPATH)/dc/dml2/display_mode_core.o := $(dml2_ccflags) -Wframe-larger-than=2048 +CFLAGS_$(AMDDALPATH)/dc/dml2/display_mode_core.o := $(dml2_ccflags) $(frame_warn_flag) CFLAGS_$(AMDDALPATH)/dc/dml2/display_mode_util.o := $(dml2_ccflags) CFLAGS_$(AMDDALPATH)/dc/dml2/dml2_wrapper.o := $(dml2_ccflags) CFLAGS_$(AMDDALPATH)/dc/dml2/dml2_utils.o := $(dml2_ccflags) --- base-commit: cd90511557fdfb394bb4ac4c3b539b007383914c change-id: 20231018-amdgpu-dml2-respect-frame_warn-536e19b69ce0 Best regards, -- Nathan Chancellor
[PATCH] drm/debugfs: Fix drm_debugfs_remove_files() stub
When building without CONFIG_DEBUG_FS: drivers/gpu/drm/tegra/dc.c:1757:59: error: too many arguments to function call, expected 3, have 4 1757 | drm_debugfs_remove_files(dc->debugfs_files, count, root, minor); | ^ include/drm/drm_debugfs.h:162:19: note: 'drm_debugfs_remove_files' declared here 162 | static inline int drm_debugfs_remove_files(const struct drm_info_list *files, | ^ ~~ 163 |int count, struct drm_minor *minor) | ~~ 1 error generated. Update the stub to include the root parameter. Fixes: 8e455145d8f1 ("drm/debugfs: rework drm_debugfs_create_files implementation v2") Signed-off-by: Nathan Chancellor --- include/drm/drm_debugfs.h | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/include/drm/drm_debugfs.h b/include/drm/drm_debugfs.h index 7213ce15e4ff..3bba169f9bae 100644 --- a/include/drm/drm_debugfs.h +++ b/include/drm/drm_debugfs.h @@ -160,7 +160,8 @@ static inline void drm_debugfs_create_files(const struct drm_info_list *files, {} static inline int drm_debugfs_remove_files(const struct drm_info_list *files, - int count, struct drm_minor *minor) + int count, struct dentry *root, + struct drm_minor *minor) { return 0; } --- base-commit: fc71f615fd08a530d24c7af0a1efa72ec6ea8e34 change-id: 20230913-fix-drm_debugfs_remove_files-stub-bd864127c162 Best regards, -- Nathan Chancellor
[PATCH] drm/amd/display: Fix -Wuninitialized in dm_helpers_dp_mst_send_payload_allocation()
When building with clang, there is a warning (or error when CONFIG_WERROR is set): drivers/gpu/drm/amd/amdgpu/../display/amdgpu_dm/amdgpu_dm_helpers.c:368:21: error: variable 'old_payload' is uninitialized when used here [-Werror,-Wuninitialized] 368 | new_payload, old_payload); | ^~~ drivers/gpu/drm/amd/amdgpu/../display/amdgpu_dm/amdgpu_dm_helpers.c:344:61: note: initialize the variable 'old_payload' to silence this warning 344 | struct drm_dp_mst_atomic_payload *new_payload, *old_payload; |^ | = NULL 1 error generated. This variable is not required outside of this function so allocate old_payload on the stack and pass it by reference to dm_helpers_construct_old_payload(), resolving the warning. Closes: https://github.com/ClangBuiltLinux/linux/issues/1931 Fixes: 5aa1dfcdf0a4 ("drm/mst: Refactor the flow for payload allocation/removement") Signed-off-by: Nathan Chancellor --- drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c index 9ad509279b0a..c4c35f6844f4 100644 --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c @@ -341,7 +341,7 @@ bool dm_helpers_dp_mst_send_payload_allocation( struct amdgpu_dm_connector *aconnector; struct drm_dp_mst_topology_state *mst_state; struct drm_dp_mst_topology_mgr *mst_mgr; - struct drm_dp_mst_atomic_payload *new_payload, *old_payload; + struct drm_dp_mst_atomic_payload *new_payload, old_payload; enum mst_progress_status set_flag = MST_ALLOCATE_NEW_PAYLOAD; enum mst_progress_status clr_flag = MST_CLEAR_ALLOCATED_PAYLOAD; int ret = 0; @@ -365,8 +365,8 @@ bool dm_helpers_dp_mst_send_payload_allocation( ret = drm_dp_add_payload_part2(mst_mgr, mst_state->base.state, new_payload); } else { dm_helpers_construct_old_payload(stream->link, mst_state->pbn_div, -new_payload, old_payload); - drm_dp_remove_payload_part2(mst_mgr, mst_state, old_payload, new_payload); +new_payload, &old_payload); + drm_dp_remove_payload_part2(mst_mgr, mst_state, &old_payload, new_payload); } if (ret) { --- base-commit: 8569c31545385195bdb0c021124e68336e91c693 change-id: 20230913-fix-wuninitialized-dm_helpers_dp_mst_send_payload_allocation-c37b33aaad18 Best regards, -- Nathan Chancellor
Re: [PATCH] backlight: lp855x: Drop ret variable in brightness change function
On Wed, Aug 09, 2023 at 01:42:16PM +0200, Artur Weber wrote: > Fixes the following warning: > > drivers/video/backlight/lp855x_bl.c:252:7: warning: variable 'ret' is used > uninitialized whenever 'if' condition is false [-Wsometimes-uninitialized] > > Signed-off-by: Artur Weber > Fixes: 5145531be5fb ("backlight: lp855x: Catch errors when changing > brightness") > Reported-by: kernel test robot > Closes: > https://lore.kernel.org/oe-kbuild-all/202308091728.nejhgupp-...@intel.com/ Reviewed-by: Nathan Chancellor > --- > drivers/video/backlight/lp855x_bl.c | 8 +++- > 1 file changed, 3 insertions(+), 5 deletions(-) > > diff --git a/drivers/video/backlight/lp855x_bl.c > b/drivers/video/backlight/lp855x_bl.c > index 61a7f45bfad8..da1f124db69c 100644 > --- a/drivers/video/backlight/lp855x_bl.c > +++ b/drivers/video/backlight/lp855x_bl.c > @@ -241,19 +241,17 @@ static int lp855x_bl_update_status(struct > backlight_device *bl) > { > struct lp855x *lp = bl_get_data(bl); > int brightness = bl->props.brightness; > - int ret; > > if (bl->props.state & (BL_CORE_SUSPENDED | BL_CORE_FBBLANK)) > brightness = 0; > > if (lp->mode == PWM_BASED) > - ret = lp855x_pwm_ctrl(lp, brightness, > + return lp855x_pwm_ctrl(lp, brightness, > bl->props.max_brightness); > else if (lp->mode == REGISTER_BASED) > - ret = lp855x_write_byte(lp, lp->cfg->reg_brightness, > + return lp855x_write_byte(lp, lp->cfg->reg_brightness, > (u8)brightness); > - > - return ret; > + return -EINVAL; > } > > static const struct backlight_ops lp855x_bl_ops = { > > base-commit: 21ef7b1e17d039053edaeaf41142423810572741 > -- > 2.41.0 >
Re: [PATCH 1/2] drm/exec: use unique instead of local label
On Thu, Aug 10, 2023 at 08:48:05AM +0200, Christian König wrote: > Am 10.08.23 um 08:40 schrieb Boris Brezillon: > > On Wed, 9 Aug 2023 08:37:55 -0700 > > Nathan Chancellor wrote: > > > > > Hi Christian, > > > > > > Can this be applied to drm-misc? Other drivers are starting to make use > > > of this API and our builds with clang-17 and clang-18 have been broken > > > for some time due to this. > > Queued to drm-misc-next. > > Sorry for the delay I have been on vacation last week and haven't yet > catched up to this point in my mails. No worries, 'tis the season :) hope it was a good time and thank you both for getting this fixed! Cheers, Nathan
Re: [PATCH 1/2] drm/exec: use unique instead of local label
Hi Christian, Can this be applied to drm-misc? Other drivers are starting to make use of this API and our builds with clang-17 and clang-18 have been broken for some time due to this. Cheers, Nathan On Mon, Jul 31, 2023 at 02:36:24PM +0200, Christian König wrote: > GCC forbids to jump to labels in loop conditions and a new clang > check stumbled over this. > > So instead using a local label inside the loop condition use an > unique label outside of it. > > Fixes: commit 09593216bff1 ("drm: execution context for GEM buffers v7") > Link: https://gcc.gnu.org/onlinedocs/gcc/Statement-Exprs.html > Link: https://github.com/ClangBuiltLinux/linux/issues/1890 > Link: > https://github.com/llvm/llvm-project/commit/20219106060208f0c2f5d096eb3aed7b712f5067 > Reported-by: Nathan Chancellor > Reported-by: Naresh Kamboju > CC: Boris Brezillon > Signed-off-by: Christian König > --- > include/drm/drm_exec.h | 14 +++--- > 1 file changed, 7 insertions(+), 7 deletions(-) > > diff --git a/include/drm/drm_exec.h b/include/drm/drm_exec.h > index 73205afec162..e0462361adf9 100644 > --- a/include/drm/drm_exec.h > +++ b/include/drm/drm_exec.h > @@ -3,6 +3,7 @@ > #ifndef __DRM_EXEC_H__ > #define __DRM_EXEC_H__ > > +#include > #include > > #define DRM_EXEC_INTERRUPTIBLE_WAIT BIT(0) > @@ -74,13 +75,12 @@ struct drm_exec { > * Since labels can't be defined local to the loops body we use a jump > pointer > * to make sure that the retry is only used from within the loops body. > */ > -#define drm_exec_until_all_locked(exec) \ > - for (void *__drm_exec_retry_ptr; ({ \ > - __label__ __drm_exec_retry; \ > -__drm_exec_retry:\ > - __drm_exec_retry_ptr = &&__drm_exec_retry; \ > - (void)__drm_exec_retry_ptr; \ > - drm_exec_cleanup(exec); \ > +#define drm_exec_until_all_locked(exec) > \ > +__PASTE(__drm_exec_, __LINE__): > \ > + for (void *__drm_exec_retry_ptr; ({ \ > + __drm_exec_retry_ptr = &&__PASTE(__drm_exec_, __LINE__);\ > + (void)__drm_exec_retry_ptr; \ > + drm_exec_cleanup(exec); \ > });) > > /** > -- > 2.34.1 >
Re: [PATCH 1/2] drm/exec: use unique instead of local label
On Mon, Jul 31, 2023 at 02:36:24PM +0200, Christian König wrote: > GCC forbids to jump to labels in loop conditions and a new clang > check stumbled over this. > > So instead using a local label inside the loop condition use an > unique label outside of it. > > Fixes: commit 09593216bff1 ("drm: execution context for GEM buffers v7") > Link: https://gcc.gnu.org/onlinedocs/gcc/Statement-Exprs.html > Link: https://github.com/ClangBuiltLinux/linux/issues/1890 > Link: > https://github.com/llvm/llvm-project/commit/20219106060208f0c2f5d096eb3aed7b712f5067 > Reported-by: Nathan Chancellor > Reported-by: Naresh Kamboju > CC: Boris Brezillon > Signed-off-by: Christian König Passes my build tests and I inspected the preprocessed output to make sure it should work. I ran the KUnit tests, which all pass (although [1] is needed to fix a tangential issue): Tested-by: Nathan Chancellor Thanks for fixing this! [1]: https://lore.kernel.org/20230728183400.306193-1-arthurgri...@riseup.net/ > --- > include/drm/drm_exec.h | 14 +++--- > 1 file changed, 7 insertions(+), 7 deletions(-) > > diff --git a/include/drm/drm_exec.h b/include/drm/drm_exec.h > index 73205afec162..e0462361adf9 100644 > --- a/include/drm/drm_exec.h > +++ b/include/drm/drm_exec.h > @@ -3,6 +3,7 @@ > #ifndef __DRM_EXEC_H__ > #define __DRM_EXEC_H__ > > +#include > #include > > #define DRM_EXEC_INTERRUPTIBLE_WAIT BIT(0) > @@ -74,13 +75,12 @@ struct drm_exec { > * Since labels can't be defined local to the loops body we use a jump > pointer > * to make sure that the retry is only used from within the loops body. > */ > -#define drm_exec_until_all_locked(exec) \ > - for (void *__drm_exec_retry_ptr; ({ \ > - __label__ __drm_exec_retry; \ > -__drm_exec_retry:\ > - __drm_exec_retry_ptr = &&__drm_exec_retry; \ > - (void)__drm_exec_retry_ptr; \ > - drm_exec_cleanup(exec); \ > +#define drm_exec_until_all_locked(exec) > \ > +__PASTE(__drm_exec_, __LINE__): > \ > + for (void *__drm_exec_retry_ptr; ({ \ > + __drm_exec_retry_ptr = &&__PASTE(__drm_exec_, __LINE__);\ > + (void)__drm_exec_retry_ptr; \ > + drm_exec_cleanup(exec); \ > });) > > /** > -- > 2.34.1 > >
Re: [PATCH] drm/radeon: Prefer 'unsigned int' to bare use of 'unsigned'
On Sat, Jul 29, 2023 at 09:12:05PM +0700, Bagas Sanjaya wrote: > On Fri, Jul 28, 2023 at 10:35:19PM +0800, 孙冉 wrote: > > WARNING: Prefer 'unsigned int' to bare use of 'unsigned' > > > > Signed-off-by: Ran Sun > > Your From: address != SoB identity While the comment below is a completely valid complaint, I think this comment is being rather pedantic. Google Translate will tell you that 孙冉 is Sun Ran, so while the name does not strictly match, it is clearly the same... > > --- > > drivers/gpu/drm/radeon/radeon_object.h | 8 > > 1 file changed, 4 insertions(+), 4 deletions(-) > > > > diff --git a/drivers/gpu/drm/radeon/radeon_object.h > > b/drivers/gpu/drm/radeon/radeon_object.h > > index 39cc87a59a9a..9b55a7103cfd 100644 > > --- a/drivers/gpu/drm/radeon/radeon_object.h > > +++ b/drivers/gpu/drm/radeon/radeon_object.h > > @@ -37,7 +37,7 @@ > > * > > * Returns corresponding domain of the ttm mem_type > > */ > > -static inline unsigned radeon_mem_type_to_domain(u32 mem_type) > > +static inline unsigned int radeon_mem_type_to_domain(u32 mem_type) > > { > > switch (mem_type) { > > case TTM_PL_VRAM: > > @@ -112,12 +112,12 @@ static inline unsigned long radeon_bo_size(struct > > radeon_bo *bo) > > return bo->tbo.base.size; > > } > > > > -static inline unsigned radeon_bo_ngpu_pages(struct radeon_bo *bo) > > +static inline unsigned int radeon_bo_ngpu_pages(struct radeon_bo *bo) > > { > > return bo->tbo.base.size / RADEON_GPU_PAGE_SIZE; > > } > > > > -static inline unsigned radeon_bo_gpu_page_alignment(struct radeon_bo *bo) > > +static inline unsigned int radeon_bo_gpu_page_alignment(struct radeon_bo > > *bo) > > { > > return (bo->tbo.page_alignment << PAGE_SHIFT) / RADEON_GPU_PAGE_SIZE; > > } > > @@ -189,7 +189,7 @@ static inline void *radeon_sa_bo_cpu_addr(struct > > drm_suballoc *sa_bo) > > > > extern int radeon_sa_bo_manager_init(struct radeon_device *rdev, > > struct radeon_sa_manager *sa_manager, > > - unsigned size, u32 align, u32 domain, > > + unsigned int size, u32 align, u32 domain, > > u32 flags); > > extern void radeon_sa_bo_manager_fini(struct radeon_device *rdev, > >struct radeon_sa_manager *sa_manager); > > The patch is whitespace-corrupted. Use git-send-email(1) to submit patches. > Also, your patch is also MIME-encoded, hence the corruption. > > To Alex: Please don't apply this patch due to reasons above. > > Thanks. > > -- > An old man doll... just what I always wanted! - Clara
Re: [PATCH v2] drm: fix indirect goto into statement expression UB
+ people from trailers of 09593216bff1 On Thu, Jul 27, 2023 at 03:50:58PM -0700, ndesaulni...@google.com wrote: > A new diagnostic in clang-17 now produces the following build error: > > drivers/gpu/drm/tests/drm_exec_test.c:41:3: error: cannot jump from this > indirect goto statement to one of its possible targets >41 | drm_exec_retry_on_contention(&exec); > | ^ > include/drm/drm_exec.h:96:4: note: expanded from macro > 'drm_exec_retry_on_contention' >96 | goto *__drm_exec_retry_ptr; > | ^ > drivers/gpu/drm/tests/drm_exec_test.c:39:2: note: possible target of > indirect goto statement >39 | drm_exec_until_all_locked(&exec) { > | ^ > include/drm/drm_exec.h:79:33: note: expanded from macro > 'drm_exec_until_all_locked' >79 | __label__ __drm_exec_retry; > drivers/gpu/drm/tests/drm_exec_test.c:39:2: note: jump enters a > statement expression > > The GCC manually currently states that: ^ manual > >> Jumping into a statement expression with a computed goto (see Labels > >> as Values) has undefined behavior. > > So the diagnostic appears correct, even if codegen happened to produce > working code. > > Looking closer at this code, while the original combination of statement > expression, local label, and computed/indirect goto GNU C expressions > were clever, a simple while loop and continue block might have sufficed. > > This approach might not work as expected if drm_exec_until_all_locked > "loops" can be nested, but that doesn't appear to be an existing use > case in the codebase. > > Fixes: commit 09593216bff1 ("drm: execution context for GEM buffers v7") > Link: https://gcc.gnu.org/onlinedocs/gcc/Statement-Exprs.html > Link: https://github.com/ClangBuiltLinux/linux/issues/1890 > Link: > https://github.com/llvm/llvm-project/commit/20219106060208f0c2f5d096eb3aed7b712f5067 > Reported-by: Nathan Chancellor > Reported-by: Naresh Kamboju > Signed-off-by: Nick Desaulniers Thanks for the patch! Tested-by: Nathan Chancellor # build > --- > Changes in v2: > Fix the continue to be outside of the do while > - Link to v1: > https://lore.kernel.org/r/20230727-amdgpu-v1-1-a95690e75...@google.com > --- > include/drm/drm_exec.h | 21 + > 1 file changed, 5 insertions(+), 16 deletions(-) > > diff --git a/include/drm/drm_exec.h b/include/drm/drm_exec.h > index 73205afec162..fa1cc5c3d021 100644 > --- a/include/drm/drm_exec.h > +++ b/include/drm/drm_exec.h > @@ -70,18 +70,8 @@ struct drm_exec { > * Core functionality of the drm_exec object. Loops until all GEM objects are > * locked and no more contention exists. At the beginning of the loop it is > * guaranteed that no GEM object is locked. > - * > - * Since labels can't be defined local to the loops body we use a jump > pointer > - * to make sure that the retry is only used from within the loops body. > */ > -#define drm_exec_until_all_locked(exec) \ > - for (void *__drm_exec_retry_ptr; ({ \ > - __label__ __drm_exec_retry; \ > -__drm_exec_retry:\ > - __drm_exec_retry_ptr = &&__drm_exec_retry; \ > - (void)__drm_exec_retry_ptr; \ > - drm_exec_cleanup(exec); \ > - });) > +#define drm_exec_until_all_locked(exec) while(drm_exec_cleanup(exec)) > > /** > * drm_exec_retry_on_contention - restart the loop to grap all locks > @@ -90,11 +80,10 @@ __drm_exec_retry: > \ > * Control flow helper to continue when a contention was detected and we > need to > * clean up and re-start the loop to prepare all GEM objects. > */ > -#define drm_exec_retry_on_contention(exec) \ > - do {\ > - if (unlikely(drm_exec_is_contended(exec))) \ > - goto *__drm_exec_retry_ptr; \ > - } while (0) > +#define drm_exec_retry_on_contention(exec) \ > + if (unlikely(drm_exec_is_contended(exec))) \ > + continue; \ > + do {} while (0) > > /** > * drm_exec_is_contended - check for contention > > --- > base-commit: 451cc82bd11eb6a374f4dbcfc1cf007eafea91ab > change-id: 20230727-amdgpu-93c0e5302951 > > Best regards, > -- > Nick Desaulniers >
Re: [PATCH 1/2] drm/v3d: Avoid -Wconstant-logical-operand in nsecs_to_jiffies_timeout()
Hi Maira, On Thu, Jul 27, 2023 at 11:01:27AM -0300, Maira Canal wrote: > Hi Nathan, > > On 7/18/23 18:44, Nathan Chancellor wrote: > > A proposed update to clang's -Wconstant-logical-operand to warn when the > > left hand side is a constant shows the following instance in > > nsecs_to_jiffies_timeout() when NSEC_PER_SEC is not a multiple of HZ, > > such as CONFIG_HZ=300: > > > >In file included from drivers/gpu/drm/v3d/v3d_debugfs.c:12: > >drivers/gpu/drm/v3d/v3d_drv.h:343:24: warning: use of logical '&&' with > > constant operand [-Wconstant-logical-operand] > > 343 | if (NSEC_PER_SEC % HZ && > > | ~ ^ > >drivers/gpu/drm/v3d/v3d_drv.h:343:24: note: use '&' for a bitwise > > operation > > 343 | if (NSEC_PER_SEC % HZ && > > | ^~ > > | & > >drivers/gpu/drm/v3d/v3d_drv.h:343:24: note: remove constant to silence > > this warning > >1 warning generated. > > > > Turn this into an explicit comparison against zero to make the > > expression a boolean to make it clear this should be a logical check, > > not a bitwise one. > > > > Link: https://reviews.llvm.org/D142609 > > Signed-off-by: Nathan Chancellor > > Reviewed-by: Maíra Canal > > Thanks for all the hard work with clang! Let me know if you need someone > to push it to drm-misc-next. Yes I will, I do not have drm commit rights. I think the i915 patch can go to drm-misc as well with Tvrtko's ack. Thanks a lot for taking a look! Cheers, Nathan > Best Regards, > - Maíra > > > --- > > drivers/gpu/drm/v3d/v3d_drv.h | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/drivers/gpu/drm/v3d/v3d_drv.h b/drivers/gpu/drm/v3d/v3d_drv.h > > index b74b1351bfc8..7f664a4b2a75 100644 > > --- a/drivers/gpu/drm/v3d/v3d_drv.h > > +++ b/drivers/gpu/drm/v3d/v3d_drv.h > > @@ -340,7 +340,7 @@ struct v3d_submit_ext { > > static inline unsigned long nsecs_to_jiffies_timeout(const u64 n) > > { > > /* nsecs_to_jiffies64() does not guard against overflow */ > > - if (NSEC_PER_SEC % HZ && > > + if ((NSEC_PER_SEC % HZ) != 0 && > > div_u64(n, NSEC_PER_SEC) >= MAX_JIFFY_OFFSET / HZ) > > return MAX_JIFFY_OFFSET; > >
Re: [PATCH 1/6] drm: execution context for GEM buffers v7
Hi Christian, On Tue, Jul 11, 2023 at 03:31:17PM +0200, Christian König wrote: > This adds the infrastructure for an execution context for GEM buffers > which is similar to the existing TTMs execbuf util and intended to replace > it in the long term. > > The basic functionality is that we abstracts the necessary loop to lock > many different GEM buffers with automated deadlock and duplicate handling. > > v2: drop xarray and use dynamic resized array instead, the locking > overhead is unnecessary and measurable. > v3: drop duplicate tracking, radeon is really the only one needing that. > v4: fixes issues pointed out by Danilo, some typos in comments and a > helper for lock arrays of GEM objects. > v5: some suggestions by Boris Brezillon, especially just use one retry > macro, drop loop in prepare_array, use flags instead of bool > v6: minor changes suggested by Thomas, Boris and Danilo > v7: minor typos pointed out by checkpatch.pl fixed > > Signed-off-by: Christian König > Reviewed-by: Boris Brezillon > Reviewed-by: Danilo Krummrich > Tested-by: Danilo Krummrich > diff --git a/include/drm/drm_exec.h b/include/drm/drm_exec.h > new file mode 100644 > index ..73205afec162 > --- /dev/null > +++ b/include/drm/drm_exec.h > + * Since labels can't be defined local to the loops body we use a jump > pointer > + * to make sure that the retry is only used from within the loops body. > + */ > +#define drm_exec_until_all_locked(exec) \ > + for (void *__drm_exec_retry_ptr; ({ \ > + __label__ __drm_exec_retry; \ > +__drm_exec_retry:\ > + __drm_exec_retry_ptr = &&__drm_exec_retry; \ > + (void)__drm_exec_retry_ptr; \ > + drm_exec_cleanup(exec); \ > + });) > + > +/** > + * drm_exec_retry_on_contention - restart the loop to grap all locks > + * @exec: drm_exec object > + * > + * Control flow helper to continue when a contention was detected and we > need to > + * clean up and re-start the loop to prepare all GEM objects. > + */ > +#define drm_exec_retry_on_contention(exec) \ > + do {\ > + if (unlikely(drm_exec_is_contended(exec))) \ > + goto *__drm_exec_retry_ptr; \ > + } while (0) This construct of using a label as a value and goto to jump into a GNU C statement expression is explicitly documented in GCC's manual [1] as undefined behavior: "Jumping into a statement expression with a computed goto (see Labels as Values [2]) has undefined behavior. " A recent change in clang [3] to prohibit this altogether points this out, so builds using clang-17 and newer will break with this change applied: drivers/gpu/drm/tests/drm_exec_test.c:41:3: error: cannot jump from this indirect goto statement to one of its possible targets 41 | drm_exec_retry_on_contention(&exec); | ^ include/drm/drm_exec.h:96:4: note: expanded from macro 'drm_exec_retry_on_contention' 96 | goto *__drm_exec_retry_ptr; \ | ^ drivers/gpu/drm/tests/drm_exec_test.c:39:2: note: possible target of indirect goto statement 39 | drm_exec_until_all_locked(&exec) { | ^ include/drm/drm_exec.h:79:33: note: expanded from macro 'drm_exec_until_all_locked' 79 | __label__ __drm_exec_retry; \ | ^ drivers/gpu/drm/tests/drm_exec_test.c:39:2: note: jump enters a statement expression It seems like if this construct works, it is by chance, although I am not sure if there is another solution. [1]: https://gcc.gnu.org/onlinedocs/gcc/Statement-Exprs.html [2]: https://gcc.gnu.org/onlinedocs/gcc/Labels-as-Values.html [3]: https://github.com/llvm/llvm-project/commit/20219106060208f0c2f5d096eb3aed7b712f5067 Cheers, Nathan
Re: [PATCH 2/2] drm/i915: Avoid -Wconstant-logical-operand in nsecs_to_jiffies_timeout()
On Thu, Jul 20, 2023 at 09:43:05AM +0100, Tvrtko Ursulin wrote: > > On 18/07/2023 22:44, Nathan Chancellor wrote: > > A proposed update to clang's -Wconstant-logical-operand to warn when the > > left hand side is a constant shows the following instance in > > nsecs_to_jiffies_timeout() when NSEC_PER_SEC is not a multiple of HZ, > > such as CONFIG_HZ=300: > > > >drivers/gpu/drm/i915/gem/i915_gem_wait.c:189:24: warning: use of logical > > '&&' with constant operand [-Wconstant-logical-operand] > > 189 | if (NSEC_PER_SEC % HZ && > > | ~ ^ > >drivers/gpu/drm/i915/gem/i915_gem_wait.c:189:24: note: use '&' for a > > bitwise operation > > 189 | if (NSEC_PER_SEC % HZ && > > | ^~ > > | & > >drivers/gpu/drm/i915/gem/i915_gem_wait.c:189:24: note: remove constant > > to silence this warning > >1 warning generated. > > > > Turn this into an explicit comparison against zero to make the > > expression a boolean to make it clear this should be a logical check, > > not a bitwise one. > > So -Wconstant-logical-operand only triggers when it is a > constant but not zero constant? Why does that make sense is not > a kludge to avoid too much noise? Yes, the warning purposefully does not trigger when the constant is a 1 or 0 (as those are usually indicative of an intentional logical operation): https://github.com/llvm/llvm-project/blob/dfdfd306cfaf54fbc43e2d5eb36489dac3eb9976/clang/lib/Sema/SemaExpr.cpp#L13917-L13919 In this case, it is 100, so I kind of understand why this might be ambiguous to the compiler. > Personally, it all feels a bit over the top as a warning, > since code in both cases should optimise away. And we may end I do not necessarily disagree, as you can see from the differential review that I linked in the message, but I also understand it is a fine line to tread when writing compiler warnings between wanting to catch as many potential problems as possible and having too much noise for developers to sift through. I think this is erring on the side of caution. > up papering over it if it becomes a default. diagtool tree tells me this warning is already on by default. > Then again this patch IMO does make the code more readable, so I think so too. > I am happy to take this one via our tree. Or either give ack to > bring it in via drm-misc-next: > > Acked-by: Tvrtko Ursulin > > Let me know which route works best. Thanks for the feedback! Either route is fine with me but if the v3d patch is going to go in via drm-misc-next, it seems like it would not be too much trouble to push this one with it. Cheers, Nathan
[PATCH 0/2] Avoid -Wconstant-logical-operand in nsecs_to_jiffies_timeout()
Hi all, A proposed update to clang's -Wconstant-logical-operand [1] to warn when the left hand side is a constant as well now triggers with the modulo expression in nsecs_to_jiffies_timeout() when NSEC_PER_SEC is not a multiple of HZ, such as CONFIG_HZ=300: drivers/gpu/drm/i915/gem/i915_gem_wait.c:189:24: warning: use of logical '&&' with constant operand [-Wconstant-logical-operand] 189 | if (NSEC_PER_SEC % HZ && | ~ ^ drivers/gpu/drm/i915/gem/i915_gem_wait.c:189:24: note: use '&' for a bitwise operation 189 | if (NSEC_PER_SEC % HZ && | ^~ | & drivers/gpu/drm/i915/gem/i915_gem_wait.c:189:24: note: remove constant to silence this warning 1 warning generated. In file included from drivers/gpu/drm/v3d/v3d_debugfs.c:12: drivers/gpu/drm/v3d/v3d_drv.h:343:24: warning: use of logical '&&' with constant operand [-Wconstant-logical-operand] 343 | if (NSEC_PER_SEC % HZ && | ~ ^ drivers/gpu/drm/v3d/v3d_drv.h:343:24: note: use '&' for a bitwise operation 343 | if (NSEC_PER_SEC % HZ && | ^~ | & drivers/gpu/drm/v3d/v3d_drv.h:343:24: note: remove constant to silence this warning 1 warning generated. These patches add an explicit comparison to zero to make the expression a boolean, which clears up the warning. The patches have no real dependency on each other but I felt like they made send together since it is the same code. If these could go into mainline sooner rather than later to avoid breaking builds that can hit this with CONFIG_WERROR, that would be nice, but I won't insist since I don't think our own CI has builds that has those conditions, but others might. --- Nathan Chancellor (2): drm/v3d: Avoid -Wconstant-logical-operand in nsecs_to_jiffies_timeout() drm/i915: Avoid -Wconstant-logical-operand in nsecs_to_jiffies_timeout() drivers/gpu/drm/i915/gem/i915_gem_wait.c | 2 +- drivers/gpu/drm/v3d/v3d_drv.h| 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) --- base-commit: fdf0eaf11452d72945af31804e2a1048ee1b574c change-id: 20230718-nsecs_to_jiffies_timeout-constant-logical-operand-4a944690f3e9 Best regards, -- Nathan Chancellor
[PATCH 2/2] drm/i915: Avoid -Wconstant-logical-operand in nsecs_to_jiffies_timeout()
A proposed update to clang's -Wconstant-logical-operand to warn when the left hand side is a constant shows the following instance in nsecs_to_jiffies_timeout() when NSEC_PER_SEC is not a multiple of HZ, such as CONFIG_HZ=300: drivers/gpu/drm/i915/gem/i915_gem_wait.c:189:24: warning: use of logical '&&' with constant operand [-Wconstant-logical-operand] 189 | if (NSEC_PER_SEC % HZ && | ~ ^ drivers/gpu/drm/i915/gem/i915_gem_wait.c:189:24: note: use '&' for a bitwise operation 189 | if (NSEC_PER_SEC % HZ && | ^~ | & drivers/gpu/drm/i915/gem/i915_gem_wait.c:189:24: note: remove constant to silence this warning 1 warning generated. Turn this into an explicit comparison against zero to make the expression a boolean to make it clear this should be a logical check, not a bitwise one. Link: https://reviews.llvm.org/D142609 Signed-off-by: Nathan Chancellor --- drivers/gpu/drm/i915/gem/i915_gem_wait.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/gpu/drm/i915/gem/i915_gem_wait.c b/drivers/gpu/drm/i915/gem/i915_gem_wait.c index 4a33ad2d122b..d4b918fb11ce 100644 --- a/drivers/gpu/drm/i915/gem/i915_gem_wait.c +++ b/drivers/gpu/drm/i915/gem/i915_gem_wait.c @@ -186,7 +186,7 @@ i915_gem_object_wait(struct drm_i915_gem_object *obj, static inline unsigned long nsecs_to_jiffies_timeout(const u64 n) { /* nsecs_to_jiffies64() does not guard against overflow */ - if (NSEC_PER_SEC % HZ && + if ((NSEC_PER_SEC % HZ) != 0 && div_u64(n, NSEC_PER_SEC) >= MAX_JIFFY_OFFSET / HZ) return MAX_JIFFY_OFFSET; -- 2.41.0
[PATCH 1/2] drm/v3d: Avoid -Wconstant-logical-operand in nsecs_to_jiffies_timeout()
A proposed update to clang's -Wconstant-logical-operand to warn when the left hand side is a constant shows the following instance in nsecs_to_jiffies_timeout() when NSEC_PER_SEC is not a multiple of HZ, such as CONFIG_HZ=300: In file included from drivers/gpu/drm/v3d/v3d_debugfs.c:12: drivers/gpu/drm/v3d/v3d_drv.h:343:24: warning: use of logical '&&' with constant operand [-Wconstant-logical-operand] 343 | if (NSEC_PER_SEC % HZ && | ~ ^ drivers/gpu/drm/v3d/v3d_drv.h:343:24: note: use '&' for a bitwise operation 343 | if (NSEC_PER_SEC % HZ && | ^~ | & drivers/gpu/drm/v3d/v3d_drv.h:343:24: note: remove constant to silence this warning 1 warning generated. Turn this into an explicit comparison against zero to make the expression a boolean to make it clear this should be a logical check, not a bitwise one. Link: https://reviews.llvm.org/D142609 Signed-off-by: Nathan Chancellor --- drivers/gpu/drm/v3d/v3d_drv.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/gpu/drm/v3d/v3d_drv.h b/drivers/gpu/drm/v3d/v3d_drv.h index b74b1351bfc8..7f664a4b2a75 100644 --- a/drivers/gpu/drm/v3d/v3d_drv.h +++ b/drivers/gpu/drm/v3d/v3d_drv.h @@ -340,7 +340,7 @@ struct v3d_submit_ext { static inline unsigned long nsecs_to_jiffies_timeout(const u64 n) { /* nsecs_to_jiffies64() does not guard against overflow */ - if (NSEC_PER_SEC % HZ && + if ((NSEC_PER_SEC % HZ) != 0 && div_u64(n, NSEC_PER_SEC) >= MAX_JIFFY_OFFSET / HZ) return MAX_JIFFY_OFFSET; -- 2.41.0
Re: [PATCH] drm/amd/display: Allow building DC with clang on RISC-V
On Mon, Jul 17, 2023 at 03:29:23PM -0700, Samuel Holland wrote: > clang on RISC-V appears to be unaffected by the bug causing excessive > stack usage in calculate_bandwidth(). clang 16 with -fstack-usage > reports a 304 byte stack frame size with CONFIG_ARCH_RV32I, and 512 > bytes with CONFIG_ARCH_RV64I. > > Signed-off-by: Samuel Holland I built ARCH=riscv allmodconfig drivers/gpu/drm/amd/amdgpu/ (confirming that CONFIG_DRM_AMD_DC gets enabled) with LLVM 11 through 17 with and without CONFIG_KASAN=y and I never saw the -Wframe-larger-than instance that this was disabled for, so I agree. Reviewed-by: Nathan Chancellor Tested-by: Nathan Chancellor > > drivers/gpu/drm/amd/display/Kconfig | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/amd/display/Kconfig > b/drivers/gpu/drm/amd/display/Kconfig > index bf0a655d009e..901d1961b739 100644 > --- a/drivers/gpu/drm/amd/display/Kconfig > +++ b/drivers/gpu/drm/amd/display/Kconfig > @@ -5,7 +5,7 @@ menu "Display Engine Configuration" > config DRM_AMD_DC > bool "AMD DC - Enable new display engine" > default y > - depends on BROKEN || !CC_IS_CLANG || X86_64 || SPARC64 || ARM64 > + depends on BROKEN || !CC_IS_CLANG || ARM64 || RISCV || SPARC64 || X86_64 > select SND_HDA_COMPONENT if SND_HDA_CORE > # !CC_IS_CLANG: https://github.com/ClangBuiltLinux/linux/issues/1752 > select DRM_AMD_DC_FP if (X86 || LOONGARCH || (PPC64 && ALTIVEC) || > (ARM64 && KERNEL_MODE_NEON && !CC_IS_CLANG)) > -- > 2.40.1 >
Re: [PATCH v4 18/21] compiler.h: RFC - s/__LINE__/__COUNTER__/ in __UNIQUE_ID fallback
Hi Jim On Thu, Jul 13, 2023 at 10:36:23AM -0600, Jim Cromie wrote: > We currently have 3 defns for __UNIQUE_ID(); gcc and clang are using > __COUNTER__ for real uniqueness, 3rd just uses __LINE__, which should > fail on this (and harder to avoid situations): > > DECLARE_FOO(); DECLARE_FOO(); > > Its 2023, can we haz a no-fallback __UNIQUE_ID ? Yeah, I fail to see how this fallback definition can actually be used after commit 95207db8166a ("Remove Intel compiler support"); even before that, it would be pretty unlikely since icc usage has not been visible for a long time. The kernel only officially supports clang or GCC now, so the definitions of __UNIQUE_ID() in include/linux/compiler-clang.h and include/linux/compiler-gcc.h should always be used because of the include in include/linux/compiler_types.h, right? I think the correct clean up is to just hoist the definition of __UNIQUE_ID() out of the individual compiler headers into the common one here but... > NOTE: > > This also changes __UNIQUE_ID_ to _kaUID_. Ive been getting > lkp-reports of collisions on names which should be unique; this > shouldnt happen on gcc & clang, but does on some older ones, on some > platforms, on some allyes & rand-configs. Like this: > > mips64-linux-ld: > drivers/gpu/drm/display/drm_dp_helper.o:(__dyndbg_class_users+0x0): > multiple definition of `__UNIQUE_ID_ddebug_class_user405'; > drivers/gpu/drm/drm_gem_shmem_helper.o:(__dyndbg_class_users+0x0): > first defined here This problem cannot be addressed with this patch given the above information, no? Seems like that might mean that __COUNTER__ has issues in earlier compilers? Cheers, Nathan > Like above, the collision reports appear to always be 3-digit > counters, which look like line-numbers. Changing to _kaUID_ in this > defn should make it more obvious (in *.i file) when a fallback has > happened. To be clear, I havent seen it yet. Nor have I seen the > multiple-defn problem above since adding this patch. > > Lets see what lkp-robot says about this. > > CC: Luc Van Oostenryck (maintainer:SPARSE > CHECKER) > CC: Nathan Chancellor (supporter:CLANG/LLVM BUILD SUPPORT) > CC: Nick Desaulniers (supporter:CLANG/LLVM BUILD > SUPPORT) > CC: Tom Rix (reviewer:CLANG/LLVM BUILD SUPPORT) > CC: linux-spa...@vger.kernel.org (open list:SPARSE CHECKER) > CC: linux-ker...@vger.kernel.org (open list) > CC: l...@lists.linux.dev (open list:CLANG/LLVM BUILD SUPPORT) > Signed-off-by: Jim Cromie > --- > include/linux/compiler.h | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/include/linux/compiler.h b/include/linux/compiler.h > index d7779a18b24f..677d6c47cd9e 100644 > --- a/include/linux/compiler.h > +++ b/include/linux/compiler.h > @@ -177,9 +177,9 @@ void ftrace_likely_update(struct ftrace_likely_data *f, > int val, > __asm__ ("" : "=r" (var) : "0" (var)) > #endif > > -/* Not-quite-unique ID. */ > +/* JFTI: to fix Not-quite-unique ID */ > #ifndef __UNIQUE_ID > -# define __UNIQUE_ID(prefix) __PASTE(__PASTE(__UNIQUE_ID_, prefix), __LINE__) > +# define __UNIQUE_ID(prefix) __PASTE(__PASTE(_kaUID_, prefix), __COUNTER__) > #endif > > /** > -- > 2.41.0 >
[PATCH 0/2] drm/amdgpu: Fix instances of -Wunused-const-variable with CONFIG_DEBUG_FS=n
Hi all, After commit a25a9dae2067 ("drm/amd/amdgpu: enable W=1 for amdgpu"), I see a few instances of -Wunused-const-variable with configurations that do not enable CONFIG_DEBUG_FS, such as Alpine Linux's. This series includes two patches to resolve each warning I see. --- Nathan Chancellor (2): drm/amdgpu: Remove CONFIG_DEBUG_FS guard around body of amdgpu_rap_debugfs_init() drm/amdgpu: Move clocks closer to its only usage in amdgpu_parse_cg_state() drivers/gpu/drm/amd/amdgpu/amdgpu_rap.c | 2 - drivers/gpu/drm/amd/pm/amdgpu_pm.c | 76 - 2 files changed, 38 insertions(+), 40 deletions(-) --- base-commit: d297eedf83f5af96751c0da1e4355c19244a55a2 change-id: 20230615-amdgpu-wunused-const-variable-wo-debugfs-308ce8e17329 Best regards, -- Nathan Chancellor
[PATCH 2/2] drm/amdgpu: Move clocks closer to its only usage in amdgpu_parse_cg_state()
After commit a25a9dae2067 ("drm/amd/amdgpu: enable W=1 for amdgpu"), there is an instance of -Wunused-const-variable when CONFIG_DEBUG_FS is disabled: drivers/gpu/drm/amd/amdgpu/../pm/amdgpu_pm.c:38:34: error: unused variable 'clocks' [-Werror,-Wunused-const-variable] 38 | static const struct cg_flag_name clocks[] = { | ^ 1 error generated. clocks is only used when CONFIG_DEBUG_FS is set, so move the definition into the CONFIG_DEBUG_FS block right above its only usage to clear up the warning. Signed-off-by: Nathan Chancellor --- drivers/gpu/drm/amd/pm/amdgpu_pm.c | 76 +++--- 1 file changed, 38 insertions(+), 38 deletions(-) diff --git a/drivers/gpu/drm/amd/pm/amdgpu_pm.c b/drivers/gpu/drm/amd/pm/amdgpu_pm.c index a57952b93e73..386ccf11e657 100644 --- a/drivers/gpu/drm/amd/pm/amdgpu_pm.c +++ b/drivers/gpu/drm/amd/pm/amdgpu_pm.c @@ -35,44 +35,6 @@ #include #include -static const struct cg_flag_name clocks[] = { - {AMD_CG_SUPPORT_GFX_FGCG, "Graphics Fine Grain Clock Gating"}, - {AMD_CG_SUPPORT_GFX_MGCG, "Graphics Medium Grain Clock Gating"}, - {AMD_CG_SUPPORT_GFX_MGLS, "Graphics Medium Grain memory Light Sleep"}, - {AMD_CG_SUPPORT_GFX_CGCG, "Graphics Coarse Grain Clock Gating"}, - {AMD_CG_SUPPORT_GFX_CGLS, "Graphics Coarse Grain memory Light Sleep"}, - {AMD_CG_SUPPORT_GFX_CGTS, "Graphics Coarse Grain Tree Shader Clock Gating"}, - {AMD_CG_SUPPORT_GFX_CGTS_LS, "Graphics Coarse Grain Tree Shader Light Sleep"}, - {AMD_CG_SUPPORT_GFX_CP_LS, "Graphics Command Processor Light Sleep"}, - {AMD_CG_SUPPORT_GFX_RLC_LS, "Graphics Run List Controller Light Sleep"}, - {AMD_CG_SUPPORT_GFX_3D_CGCG, "Graphics 3D Coarse Grain Clock Gating"}, - {AMD_CG_SUPPORT_GFX_3D_CGLS, "Graphics 3D Coarse Grain memory Light Sleep"}, - {AMD_CG_SUPPORT_MC_LS, "Memory Controller Light Sleep"}, - {AMD_CG_SUPPORT_MC_MGCG, "Memory Controller Medium Grain Clock Gating"}, - {AMD_CG_SUPPORT_SDMA_LS, "System Direct Memory Access Light Sleep"}, - {AMD_CG_SUPPORT_SDMA_MGCG, "System Direct Memory Access Medium Grain Clock Gating"}, - {AMD_CG_SUPPORT_BIF_MGCG, "Bus Interface Medium Grain Clock Gating"}, - {AMD_CG_SUPPORT_BIF_LS, "Bus Interface Light Sleep"}, - {AMD_CG_SUPPORT_UVD_MGCG, "Unified Video Decoder Medium Grain Clock Gating"}, - {AMD_CG_SUPPORT_VCE_MGCG, "Video Compression Engine Medium Grain Clock Gating"}, - {AMD_CG_SUPPORT_HDP_LS, "Host Data Path Light Sleep"}, - {AMD_CG_SUPPORT_HDP_MGCG, "Host Data Path Medium Grain Clock Gating"}, - {AMD_CG_SUPPORT_DRM_MGCG, "Digital Right Management Medium Grain Clock Gating"}, - {AMD_CG_SUPPORT_DRM_LS, "Digital Right Management Light Sleep"}, - {AMD_CG_SUPPORT_ROM_MGCG, "Rom Medium Grain Clock Gating"}, - {AMD_CG_SUPPORT_DF_MGCG, "Data Fabric Medium Grain Clock Gating"}, - {AMD_CG_SUPPORT_VCN_MGCG, "VCN Medium Grain Clock Gating"}, - {AMD_CG_SUPPORT_HDP_DS, "Host Data Path Deep Sleep"}, - {AMD_CG_SUPPORT_HDP_SD, "Host Data Path Shutdown"}, - {AMD_CG_SUPPORT_IH_CG, "Interrupt Handler Clock Gating"}, - {AMD_CG_SUPPORT_JPEG_MGCG, "JPEG Medium Grain Clock Gating"}, - {AMD_CG_SUPPORT_REPEATER_FGCG, "Repeater Fine Grain Clock Gating"}, - {AMD_CG_SUPPORT_GFX_PERF_CLK, "Perfmon Clock Gating"}, - {AMD_CG_SUPPORT_ATHUB_MGCG, "Address Translation Hub Medium Grain Clock Gating"}, - {AMD_CG_SUPPORT_ATHUB_LS, "Address Translation Hub Light Sleep"}, - {0, NULL}, -}; - static const struct hwmon_temp_label { enum PP_HWMON_TEMP channel; const char *label; @@ -3684,6 +3646,44 @@ static int amdgpu_debugfs_pm_info_pp(struct seq_file *m, struct amdgpu_device *a return 0; } +static const struct cg_flag_name clocks[] = { + {AMD_CG_SUPPORT_GFX_FGCG, "Graphics Fine Grain Clock Gating"}, + {AMD_CG_SUPPORT_GFX_MGCG, "Graphics Medium Grain Clock Gating"}, + {AMD_CG_SUPPORT_GFX_MGLS, "Graphics Medium Grain memory Light Sleep"}, + {AMD_CG_SUPPORT_GFX_CGCG, "Graphics Coarse Grain Clock Gating"}, + {AMD_CG_SUPPORT_GFX_CGLS, "Graphics Coarse Grain memory Light Sleep"}, + {AMD_CG_SUPPORT_GFX_CGTS, "Graphics Coarse Grain Tree Shader Clock Gating"}, + {AMD_CG_SUPPORT_GFX_CGTS_LS, "Graphics Coarse Grain Tree Shader Light Sleep"}, + {AMD_CG_SUPPORT_GFX_CP_LS, "Graphics Command Processor Light Sleep"}, + {AMD_CG_SUP
[PATCH 1/2] drm/amdgpu: Remove CONFIG_DEBUG_FS guard around body of amdgpu_rap_debugfs_init()
After commit a25a9dae2067 ("drm/amd/amdgpu: enable W=1 for amdgpu"), there is an instance of -Wunused-const-variable when CONFIG_DEBUG_FS is disabled: drivers/gpu/drm/amd/amdgpu/amdgpu_rap.c:110:37: error: unused variable 'amdgpu_rap_debugfs_ops' [-Werror,-Wunused-const-variable] 110 | static const struct file_operations amdgpu_rap_debugfs_ops = { | ^ 1 error generated. There is no reason for the body of this function to be guarded when CONFIG_DEBUG_FS is disabled, as debugfs_create_file() is a stub that just returns an error pointer in that situation. Remove the preprocessor guards so that the variable never appears unused, while not changing anything at run time. Signed-off-by: Nathan Chancellor --- drivers/gpu/drm/amd/amdgpu/amdgpu_rap.c | 2 -- 1 file changed, 2 deletions(-) diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_rap.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_rap.c index 12010c988c8b..123bcf5c2bb1 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_rap.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_rap.c @@ -116,7 +116,6 @@ static const struct file_operations amdgpu_rap_debugfs_ops = { void amdgpu_rap_debugfs_init(struct amdgpu_device *adev) { -#if defined(CONFIG_DEBUG_FS) struct drm_minor *minor = adev_to_drm(adev)->primary; if (!adev->psp.rap_context.context.initialized) @@ -124,5 +123,4 @@ void amdgpu_rap_debugfs_init(struct amdgpu_device *adev) debugfs_create_file("rap_test", S_IWUSR, minor->debugfs_root, adev, &amdgpu_rap_debugfs_ops); -#endif } -- 2.41.0
Re: [PATCH] drm/amdgpu: Wrap -Wunused-but-set-variable in cc-option
On Sat, Jun 10, 2023 at 10:14:05AM +0300, Jani Nikula wrote: > On Thu, 08 Jun 2023, Nathan Chancellor wrote: > > -Wunused-but-set-variable was only supported in clang starting with > > 13.0.0, so earlier versions will emit a warning, which is turned into a > > hard error for the kernel to mirror GCC: > > > > error: unknown warning option '-Wunused-but-set-variable'; did you mean > > '-Wunused-const-variable'? [-Werror,-Wunknown-warning-option] > > > > The minimum supported version of clang for building the kernel is > > 11.0.0, so match the rest of the kernel and wrap > > -Wunused-but-set-variable in a cc-option call, so that it is only used > > when supported by the compiler. > > I wonder if there's a table somewhere listing all the warning options, > which GCC and Clang versions support them, and which versions have them > in -Wall and -Wextra. Would be really useful. I don't think there is anything other than the official documentations for each listing all the warning options. I know each version has its own documentation for comparing warnings between releases but that is obviously tedious. The clang -Wall question is easy enough to answer based on the test case: https://github.com/llvm/llvm-project/blob/llvmorg-16.0.0/clang/test/Misc/warning-wall.c https://github.com/llvm/llvm-project/blob/llvmorg-15.0.0/clang/test/Misc/warning-wall.c https://github.com/llvm/llvm-project/blob/llvmorg-14.0.0/clang/test/Misc/warning-wall.c https://github.com/llvm/llvm-project/blob/llvmorg-13.0.0/clang/test/Misc/warning-wall.c https://github.com/llvm/llvm-project/blob/llvmorg-12.0.0/clang/test/Misc/warning-wall.c https://github.com/llvm/llvm-project/blob/llvmorg-11.0.0/clang/test/Misc/warning-wall.c Clang has a tool, diagtool, that can print information about -Wextra, but I do not ship it with the kernel.org LLVM releases, nor does Debian it seems. On a recent clang-17 (the colors don't matter for this exercise): $ diagtool tree -Wextra GREEN = enabled by default YELLOW = disabled by default RED = unimplemented (accepted for GCC compatibility) -Wextra -Wdeprecated-copy -Wdeprecated-copy-with-user-provided-copy -Wmissing-field-initializers -Wignored-qualifiers -Wignored-reference-qualifiers -Winitializer-overrides -Wsemicolon-before-method-body -Wmissing-method-return-type -Wsign-compare -Wunused-parameter -Wunused-but-set-parameter -Wnull-pointer-arithmetic -Wgnu-null-pointer-arithmetic -Wnull-pointer-subtraction -Wempty-init-stmt -Wstring-concatenation -Wfuse-ld-path Maybe some of that can be useful for future travelers. > If there isn't one, it would be really helpful. *wink*. Heh, that does sound like an interesting project but I am not sure I have the bandwidth at the moment to do something like that, especially since the number of warnings that are different between GCC and clang are continuing to dwindle :) Cheers, Nathan > > Closes: https://github.com/ClangBuiltLinux/linux/issues/1869 > > Fixes: a0fd5a5f676c ("drm/amd/amdgpu: introduce DRM_AMDGPU_WERROR") > > Signed-off-by: Nathan Chancellor > > --- > > drivers/gpu/drm/amd/amdgpu/Makefile | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/drivers/gpu/drm/amd/amdgpu/Makefile > > b/drivers/gpu/drm/amd/amdgpu/Makefile > > index 7ee68b1bbfed..86b833085f19 100644 > > --- a/drivers/gpu/drm/amd/amdgpu/Makefile > > +++ b/drivers/gpu/drm/amd/amdgpu/Makefile > > @@ -40,7 +40,7 @@ ccflags-y := -I$(FULL_AMD_PATH)/include/asic_reg \ > > -I$(FULL_AMD_PATH)/amdkfd > > > > subdir-ccflags-y := -Wextra > > -subdir-ccflags-y += -Wunused-but-set-variable > > +subdir-ccflags-y += $(call cc-option, -Wunused-but-set-variable) > > subdir-ccflags-y += -Wno-unused-parameter > > subdir-ccflags-y += -Wno-type-limits > > subdir-ccflags-y += -Wno-sign-compare > > > > --- > > base-commit: 6bd4b01e8938779b0d959bdf33949a9aa258a363 > > change-id: > > 20230608-amdgpu-wrap-wunused-but-set-variable-in-cc-option-0be9528ac5c8 > > > > Best regards, > > -- > Jani Nikula, Intel Open Source Graphics Center
Re: [PATCH] drm/amd/amdgpu: enable W=1 for amdgpu
+ 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 > Cc: Nathan Chancellor > Signed-off-by: Hamza Mahfooz 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 Tested-by: Nathan Chancellor > --- > 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 >
[PATCH] drm/amdgpu: Wrap -Wunused-but-set-variable in cc-option
-Wunused-but-set-variable was only supported in clang starting with 13.0.0, so earlier versions will emit a warning, which is turned into a hard error for the kernel to mirror GCC: error: unknown warning option '-Wunused-but-set-variable'; did you mean '-Wunused-const-variable'? [-Werror,-Wunknown-warning-option] The minimum supported version of clang for building the kernel is 11.0.0, so match the rest of the kernel and wrap -Wunused-but-set-variable in a cc-option call, so that it is only used when supported by the compiler. Closes: https://github.com/ClangBuiltLinux/linux/issues/1869 Fixes: a0fd5a5f676c ("drm/amd/amdgpu: introduce DRM_AMDGPU_WERROR") Signed-off-by: Nathan Chancellor --- drivers/gpu/drm/amd/amdgpu/Makefile | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/gpu/drm/amd/amdgpu/Makefile b/drivers/gpu/drm/amd/amdgpu/Makefile index 7ee68b1bbfed..86b833085f19 100644 --- a/drivers/gpu/drm/amd/amdgpu/Makefile +++ b/drivers/gpu/drm/amd/amdgpu/Makefile @@ -40,7 +40,7 @@ ccflags-y := -I$(FULL_AMD_PATH)/include/asic_reg \ -I$(FULL_AMD_PATH)/amdkfd subdir-ccflags-y := -Wextra -subdir-ccflags-y += -Wunused-but-set-variable +subdir-ccflags-y += $(call cc-option, -Wunused-but-set-variable) subdir-ccflags-y += -Wno-unused-parameter subdir-ccflags-y += -Wno-type-limits subdir-ccflags-y += -Wno-sign-compare --- base-commit: 6bd4b01e8938779b0d959bdf33949a9aa258a363 change-id: 20230608-amdgpu-wrap-wunused-but-set-variable-in-cc-option-0be9528ac5c8 Best regards, -- Nathan Chancellor
[PATCH] drm/i915/pxp: Fix size_t format specifier in gsccs_send_message()
When building ARCH=i386 allmodconfig, the following warning occurs: In file included from include/linux/device.h:15, from include/linux/node.h:18, from include/linux/cpu.h:17, from include/linux/static_call.h:135, from arch/x86/include/asm/perf_event.h:5, from include/linux/perf_event.h:25, from drivers/gpu/drm/i915/i915_pmu.h:11, from drivers/gpu/drm/i915/gt/intel_engine_types.h:21, from drivers/gpu/drm/i915/gt/intel_context_types.h:18, from drivers/gpu/drm/i915/gem/i915_gem_context_types.h:20, from drivers/gpu/drm/i915/i915_request.h:34, from drivers/gpu/drm/i915/i915_active.h:13, from drivers/gpu/drm/i915/gt/intel_context.h:13, from drivers/gpu/drm/i915/pxp/intel_pxp_gsccs.c:8: drivers/gpu/drm/i915/pxp/intel_pxp_gsccs.c: In function 'gsccs_send_message': include/drm/drm_print.h:456:39: error: format '%ld' expects argument of type 'long int', but argument 4 has type 'size_t' {aka 'unsigned int'} [-Werror=format=] 456 | dev_##level##type((drm)->dev, "[drm] " fmt, ##__VA_ARGS__) | ^~~~ include/linux/dev_printk.h:110:30: note: in definition of macro 'dev_printk_index_wrap' 110 | _p_func(dev, fmt, ##__VA_ARGS__); \ | ^~~ include/linux/dev_printk.h:146:61: note: in expansion of macro 'dev_fmt' 146 | dev_printk_index_wrap(_dev_warn, KERN_WARNING, dev, dev_fmt(fmt), ##__VA_ARGS__) | ^~~ include/drm/drm_print.h:456:9: note: in expansion of macro 'dev_warn' 456 | dev_##level##type((drm)->dev, "[drm] " fmt, ##__VA_ARGS__) | ^~~~ include/drm/drm_print.h:466:9: note: in expansion of macro '__drm_printk' 466 | __drm_printk((drm), warn,, fmt, ##__VA_ARGS__) | ^~~~ drivers/gpu/drm/i915/pxp/intel_pxp_gsccs.c:146:17: note: in expansion of macro 'drm_warn' 146 | drm_warn(&i915->drm, "caller with insufficient PXP reply size %u (%ld)\n", | ^~~~ cc1: all warnings being treated as errors Use the '%zu' format specifier, as the variable is a 'size_t'. Fixes: dc9ac125d81f ("drm/i915/pxp: Add GSC-CS backend to send GSC fw messages") Signed-off-by: Nathan Chancellor --- drivers/gpu/drm/i915/pxp/intel_pxp_gsccs.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp_gsccs.c b/drivers/gpu/drm/i915/pxp/intel_pxp_gsccs.c index 8dc41de3f6f7..a217821eb0fb 100644 --- a/drivers/gpu/drm/i915/pxp/intel_pxp_gsccs.c +++ b/drivers/gpu/drm/i915/pxp/intel_pxp_gsccs.c @@ -143,7 +143,7 @@ gsccs_send_message(struct intel_pxp *pxp, reply_size = header->message_size - sizeof(*header); if (reply_size > msg_out_size_max) { - drm_warn(&i915->drm, "caller with insufficient PXP reply size %u (%ld)\n", + drm_warn(&i915->drm, "caller with insufficient PXP reply size %u (%zu)\n", reply_size, msg_out_size_max); reply_size = msg_out_size_max; } --- base-commit: 08264f85c5c05ecc38d409c84d48cfb00ccd3bc4 change-id: 20230530-i915-pxp-size_t-wformat-1d73ed1f8d23 Best regards, -- Nathan Chancellor
[PATCH 2/2] drm/i915/gt: Fix parameter in gmch_ggtt_insert_{entries,page}()
When building with clang's -Wincompatible-function-pointer-types-strict, the following warnings occur: drivers/gpu/drm/i915/gt/intel_ggtt_gmch.c:102:23: error: incompatible function pointer types assigning to 'void (*)(struct i915_address_space *, dma_addr_t, u64, unsigned int, u32)' (aka 'void (*)(struct i915_address_space *, unsigned int, unsigned long long, unsigned int, unsigned int)') from 'void (struct i915_address_space *, dma_addr_t, u64, enum i915_cache_level, u32)' (aka 'void (struct i915_address_space *, unsigned int, unsigned long long, enum i915_cache_level, unsigned int)') [-Werror,-Wincompatible-function-pointer-types-strict] ggtt->vm.insert_page = gmch_ggtt_insert_page; ^ ~ drivers/gpu/drm/i915/gt/intel_ggtt_gmch.c:103:26: error: incompatible function pointer types assigning to 'void (*)(struct i915_address_space *, struct i915_vma_resource *, unsigned int, u32)' (aka 'void (*)(struct i915_address_space *, struct i915_vma_resource *, unsigned int, unsigned int)') from 'void (struct i915_address_space *, struct i915_vma_resource *, enum i915_cache_level, u32)' (aka 'void (struct i915_address_space *, struct i915_vma_resource *, enum i915_cache_level, unsigned int)') [-Werror, -Wincompatible-function-pointer-types-strict] ggtt->vm.insert_entries = gmch_ggtt_insert_entries; ^ 2 errors generated. The warning is pointing out that while 'enum i915_cache_level' and 'unsigned int' are ABI compatible, these indirect calls will fail clang's kernel Control Flow Integrity (kCFI) checks, as the callback's signature does not exactly match the prototype's signature. To fix this, replace the cache_level parameter with pat_index, as was done in other places within i915 where there is no difference between cache_level and pat_index on certain generations. Fixes: 9275277d5324 ("drm/i915: use pat_index instead of cache_level") Signed-off-by: Nathan Chancellor --- drivers/gpu/drm/i915/gt/intel_ggtt_gmch.c | 8 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/drivers/gpu/drm/i915/gt/intel_ggtt_gmch.c b/drivers/gpu/drm/i915/gt/intel_ggtt_gmch.c index d6a74ae2527b..866c416afb73 100644 --- a/drivers/gpu/drm/i915/gt/intel_ggtt_gmch.c +++ b/drivers/gpu/drm/i915/gt/intel_ggtt_gmch.c @@ -18,10 +18,10 @@ static void gmch_ggtt_insert_page(struct i915_address_space *vm, dma_addr_t addr, u64 offset, - enum i915_cache_level cache_level, + unsigned int pat_index, u32 unused) { - unsigned int flags = (cache_level == I915_CACHE_NONE) ? + unsigned int flags = (pat_index == I915_CACHE_NONE) ? AGP_USER_MEMORY : AGP_USER_CACHED_MEMORY; intel_gmch_gtt_insert_page(addr, offset >> PAGE_SHIFT, flags); @@ -29,10 +29,10 @@ static void gmch_ggtt_insert_page(struct i915_address_space *vm, static void gmch_ggtt_insert_entries(struct i915_address_space *vm, struct i915_vma_resource *vma_res, -enum i915_cache_level cache_level, +unsigned int pat_index, u32 unused) { - unsigned int flags = (cache_level == I915_CACHE_NONE) ? + unsigned int flags = (pat_index == I915_CACHE_NONE) ? AGP_USER_MEMORY : AGP_USER_CACHED_MEMORY; intel_gmch_gtt_insert_sg_entries(vma_res->bi.pages, vma_res->start >> PAGE_SHIFT, -- 2.40.1
[PATCH 1/2] drm/i915/gt: Fix second parameter type of pre-gen8 pte_encode callbacks
When booting a kernel compiled with CONFIG_CFI_CLANG (kCFI), there is a CFI failure in ggtt_probe_common() when trying to call hsw_pte_encode() via an indirect call: [5.030027] CFI failure at ggtt_probe_common+0xd1/0x130 [i915] (target: hsw_pte_encode+0x0/0x30 [i915]; expected type: 0xf5c1d0fc) With kCFI, indirect calls are validated against their expected type versus actual type and failures occur when the two types do not match. clang's -Wincompatible-function-pointer-types-strict can catch this at compile time but it is not enabled for the kernel yet: drivers/gpu/drm/i915/gt/intel_ggtt.c:1155:23: error: incompatible function pointer types assigning to 'u64 (*)(dma_addr_t, unsigned int, u32)' (aka 'unsigned long long (*)(unsigned int, unsigned int, unsigned int)') from 'u64 (dma_addr_t, enum i915_cache_level, u32)' (aka 'unsigned long long (unsigned int, enum i915_cache_level, unsigned int)') [-Werror,-Wincompatible-function-pointer-types-strict] ggtt->vm.pte_encode = iris_pte_encode; ^ ~~~ drivers/gpu/drm/i915/gt/intel_ggtt.c:1157:23: error: incompatible function pointer types assigning to 'u64 (*)(dma_addr_t, unsigned int, u32)' (aka 'unsigned long long (*)(unsigned int, unsigned int, unsigned int)') from 'u64 (dma_addr_t, enum i915_cache_level, u32)' (aka 'unsigned long long (unsigned int, enum i915_cache_level, unsigned int)') [-Werror,-Wincompatible-function-pointer-types-strict] ggtt->vm.pte_encode = hsw_pte_encode; ^ ~~ drivers/gpu/drm/i915/gt/intel_ggtt.c:1159:23: error: incompatible function pointer types assigning to 'u64 (*)(dma_addr_t, unsigned int, u32)' (aka 'unsigned long long (*)(unsigned int, unsigned int, unsigned int)') from 'u64 (dma_addr_t, enum i915_cache_level, u32)' (aka 'unsigned long long (unsigned int, enum i915_cache_level, unsigned int)') [-Werror,-Wincompatible-function-pointer-types-strict] ggtt->vm.pte_encode = byt_pte_encode; ^ ~~ drivers/gpu/drm/i915/gt/intel_ggtt.c:1161:23: error: incompatible function pointer types assigning to 'u64 (*)(dma_addr_t, unsigned int, u32)' (aka 'unsigned long long (*)(unsigned int, unsigned int, unsigned int)') from 'u64 (dma_addr_t, enum i915_cache_level, u32)' (aka 'unsigned long long (unsigned int, enum i915_cache_level, unsigned int)') [-Werror,-Wincompatible-function-pointer-types-strict] ggtt->vm.pte_encode = ivb_pte_encode; ^ ~~ drivers/gpu/drm/i915/gt/intel_ggtt.c:1163:23: error: incompatible function pointer types assigning to 'u64 (*)(dma_addr_t, unsigned int, u32)' (aka 'unsigned long long (*)(unsigned int, unsigned int, unsigned int)') from 'u64 (dma_addr_t, enum i915_cache_level, u32)' (aka 'unsigned long long (unsigned int, enum i915_cache_level, unsigned int)') [-Werror,-Wincompatible-function-pointer-types-strict] ggtt->vm.pte_encode = snb_pte_encode; ^ ~~ 5 errors generated. In this case, the pre-gen8 pte_encode functions have a second parameter type of 'enum i915_cache_level' whereas the function pointer prototype in 'struct i915_address_space' expects a second parameter type of 'unsigned int'. Update the second parameter of the callbacks and the comment above them noting that these statements are still valid, which matches other functions and files, to clear up the kCFI failures at run time. Fixes: 9275277d5324 ("drm/i915: use pat_index instead of cache_level") Signed-off-by: Nathan Chancellor --- drivers/gpu/drm/i915/gt/intel_ggtt.c | 26 +- 1 file changed, 13 insertions(+), 13 deletions(-) diff --git a/drivers/gpu/drm/i915/gt/intel_ggtt.c b/drivers/gpu/drm/i915/gt/intel_ggtt.c index 2a7942fac798..122197737ef2 100644 --- a/drivers/gpu/drm/i915/gt/intel_ggtt.c +++ b/drivers/gpu/drm/i915/gt/intel_ggtt.c @@ -1015,16 +1015,16 @@ static int gen8_gmch_probe(struct i915_ggtt *ggtt) /* * For pre-gen8 platforms pat_index is the same as enum i915_cache_level, - * so these PTE encode functions are left with using cache_level. + * so the switch-case statements in these PTE encode functions are still valid. * See translation table LEGACY_CACHELEVEL. */ static u64 snb_pte_encode(dma_addr_t addr, - enum i915_cache_level level, + unsigned int pat_index, u32 flags) { gen6_pte_t pte = GEN6_PTE_ADDR_ENCODE(addr) | GEN6_PTE_VALID; - switch (level) { +
[PATCH 0/2] drm/i915/gt: Fix recent kCFI violations
Hi all, This series fixes a few clang kernel Control Flow Integrity (kCFI) violations that appear after commit 9275277d5324 ("drm/i915: use pat_index instead of cache_level"). They were found between run time testing on real hardware and compile time testing with -Wincompatible-function-pointer-types-strict (which is not yet enabled for the kernel but I build with it locally to catch new instances). If there are any problems or questions, please let me know. --- Nathan Chancellor (2): drm/i915/gt: Fix second parameter type of pre-gen8 pte_encode callbacks drm/i915/gt: Fix parameter in gmch_ggtt_insert_{entries,page}() drivers/gpu/drm/i915/gt/intel_ggtt.c | 26 +- drivers/gpu/drm/i915/gt/intel_ggtt_gmch.c | 8 2 files changed, 17 insertions(+), 17 deletions(-) --- base-commit: 08264f85c5c05ecc38d409c84d48cfb00ccd3bc4 change-id: 20230530-i915-gt-cache_level-wincompatible-function-pointer-types-strict-32a5c65249a5 Best regards, -- Nathan Chancellor
Re: [PATCH] drm/amdkfd: remove unused function get_reserved_sdma_queues_bitmap
On Thu, May 25, 2023 at 04:07:59PM -0400, Tom Rix wrote: > clang with W=1 reports > drivers/gpu/drm/amd/amdgpu/../amdkfd/kfd_device_queue_manager.c:122:24: error: > unused function 'get_reserved_sdma_queues_bitmap' > [-Werror,-Wunused-function] > static inline uint64_t get_reserved_sdma_queues_bitmap(struct > device_queue_manager *dqm) >^ > This function is not used so remove it. > > Signed-off-by: Tom Rix Caused by commit 09a95a85cf3e ("drm/amdkfd: Update SDMA queue management for GFX9.4.3") it seems. You can actually go a step farther and remove the reserved_sdma_queues_bitmap member from 'struct kfd_device_info' because it is now only assigned, never read. $ git grep reserved_sdma_queues_bitmap next-20230525 next:20230525:drivers/gpu/drm/amd/amdkfd/kfd_device.c: kfd->device_info.reserved_sdma_queues_bitmap = 0xFULL; next:20230525:drivers/gpu/drm/amd/amdkfd/kfd_device.c: kfd->device_info.reserved_sdma_queues_bitmap = 0x3ULL; next:20230525:drivers/gpu/drm/amd/amdkfd/kfd_device_queue_manager.c:static inline uint64_t get_reserved_sdma_queues_bitmap(struct device_queue_manager *dqm) next:20230525:drivers/gpu/drm/amd/amdkfd/kfd_device_queue_manager.c:return dqm->dev->kfd->device_info.reserved_sdma_queues_bitmap; next:20230525:drivers/gpu/drm/amd/amdkfd/kfd_priv.h:uint64_t reserved_sdma_queues_bitmap; > --- > drivers/gpu/drm/amd/amdkfd/kfd_device_queue_manager.c | 5 - > 1 file changed, 5 deletions(-) > > diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_device_queue_manager.c > b/drivers/gpu/drm/amd/amdkfd/kfd_device_queue_manager.c > index 493b4b66f180..2fbd0a96424f 100644 > --- a/drivers/gpu/drm/amd/amdkfd/kfd_device_queue_manager.c > +++ b/drivers/gpu/drm/amd/amdkfd/kfd_device_queue_manager.c > @@ -119,11 +119,6 @@ unsigned int get_num_xgmi_sdma_queues(struct > device_queue_manager *dqm) > dqm->dev->kfd->device_info.num_sdma_queues_per_engine; > } > > -static inline uint64_t get_reserved_sdma_queues_bitmap(struct > device_queue_manager *dqm) > -{ > - return dqm->dev->kfd->device_info.reserved_sdma_queues_bitmap; > -} > - > static void init_sdma_bitmaps(struct device_queue_manager *dqm) > { > bitmap_zero(dqm->sdma_bitmap, KFD_MAX_SDMA_QUEUES); > -- > 2.27.0 >
Re: [PATCH v2] drm/amd/display: enable more strict compile checks
On Thu, May 25, 2023 at 08:37:07AM -0700, Kees Cook wrote: > Hi! > > On Wed, May 24, 2023 at 04:27:31PM -0400, Hamza Mahfooz wrote: > > + Kees > > > > On 5/24/23 15:50, Alex Deucher wrote: > > > On Wed, May 24, 2023 at 3:46 PM Felix Kuehling > > > wrote: > > > > > > > > Sure, I think we tried enabling warnings as errors before and had to > > > > revert it because of weird compiler quirks or the variety of compiler > > > > versions that need to be supported. > > > > > > > > Alex, are you planning to upstream this, or is this only to enforce more > > > > internal discipline about not ignoring warnings? > > > > > > I'd like to upstream it. Upstream already has CONFIG_WERROR as a > > > config option, but it's been problematic to enable in CI because of > > > various breakages outside of the driver and in different compilers. > > > That said, I don't know how much trouble enabling it will cause with > > > various compilers in the wild. > > -Wmisleading-indentation is already part of -Wall, so this is globally > enabled already. > > -Wunused is enabled under W=1, and it's pretty noisy still. If you can > get builds clean in drm, that'll be a good step towards getting it > enabled globally. (A middle ground with less to clean up might be > -Wunused-but-set-variable) > > I agree about -Werror: just stick with CONFIG_WERROR instead. There is also W=e, added by commit c77d06e70d59 ("kbuild: support W=e to make build abort in case of warning") in 5.19, which works well for building with configurations that do not have CONFIG_WERROR enabled and avoiding dipping into menuconfig. Unconditionally enabling -Werror with no way to turn it off is just asking for problems over time with new compiler versions, either due to new warnings in -Wall or warnings that have been improved or changed. Should that still be desired, consider doing what i915 and PowerPC have done and add a Kconfig option that can be disabled. Cheers, Nathan
Re: [PATCH] drm/i915: Fix clang -Wimplicit-fallthrough in intel_async_flip_check_hw()
On Wed, May 24, 2023 at 11:32:32AM -0700, Nick Desaulniers wrote: > On Wed, May 24, 2023 at 8:38 AM Nathan Chancellor wrote: > > > > Clang warns: > > > > drivers/gpu/drm/i915/display/intel_display.c:6012:3: error: unannotated > > fall-through between switch labels [-Werror,-Wimplicit-fallthrough] > > case I915_FORMAT_MOD_X_TILED: > > ^ > > drivers/gpu/drm/i915/display/intel_display.c:6012:3: note: insert > > 'break;' to avoid fall-through > > case I915_FORMAT_MOD_X_TILED: > > ^ > > break; > > 1 error generated. > > > > Clang is a little more pedantic than GCC, which does not warn when > > falling through to a case that is just break or return. Clang's version > > is more in line with the kernel's own stance in deprecated.rst, which > > states that all switch/case blocks must end in either break, > > fallthrough, continue, goto, or return. Add the missing break to silence > > the warning. > > > > Fixes: 937859485aef ("drm/i915: Support Async Flip on Linear buffers") > > Reported-by: kernel test robot > > Closes: https://lore.kernel.org/202305241902.uvhtmoxa-...@intel.com/ > > Reported-by: Naresh Kamboju > > Closes: > > https://lore.kernel.org/CA+G9fYv68V3ewK0Qj-syQj7qX-hQr0H1MFL=qfnudoe_j2z...@mail.gmail.com/ > > Signed-off-by: Nathan Chancellor > > Thanks for the patch! I've never seen the closes tag before, that's > new to me. Can you tell me more about it? It is new to me (at least in the context of the kernel) as well. I only used it over Link: because checkpatch.pl told me to: WARNING: Reported-by: should be immediately followed by Closes: with a URL to the report #26: Reported-by: kernel test robot Reported-by: Naresh Kamboju WARNING: Reported-by: should be immediately followed by Closes: with a URL to the report #27: Reported-by: Naresh Kamboju Signed-off-by: Nathan Chancellor It was Link: for a bit but commit 44c31888098a ("checkpatch: allow Closes tags with links") changed it to Closes:. Looks odd to me but whatever the linter says I suppose. Thanks for the review! Cheers, Nathan > A few more tags > > Reported-by: Tom Rix > Link: https://lore.kernel.org/all/20230523125116.1669057-1-t...@redhat.com/ > Reviewed-by: Nick Desaulniers > > > > --- > > drivers/gpu/drm/i915/display/intel_display.c | 1 + > > 1 file changed, 1 insertion(+) > > > > diff --git a/drivers/gpu/drm/i915/display/intel_display.c > > b/drivers/gpu/drm/i915/display/intel_display.c > > index 0490c6412ab5..6d49e0ab3e85 100644 > > --- a/drivers/gpu/drm/i915/display/intel_display.c > > +++ b/drivers/gpu/drm/i915/display/intel_display.c > > @@ -6008,6 +6008,7 @@ static int intel_async_flip_check_hw(struct > > intel_atomic_state *state, struct in > > plane->base.base.id, > > plane->base.name); > > return -EINVAL; > > } > > + break; > > > > case I915_FORMAT_MOD_X_TILED: > > case I915_FORMAT_MOD_Y_TILED: > > > > --- > > base-commit: 9a2cb1b31c040e2f1b313e2f7921f0f5e6b66d82 > > change-id: > > 20230524-intel_async_flip_check_hw-implicit-fallthrough-c4c40b03802f > > > > Best regards, > > -- > > Nathan Chancellor > > > > > -- > Thanks, > ~Nick Desaulniers
[PATCH] drm/amdgpu: Fix return types of certain NBIOv7.9 callbacks
When building with clang's -Wincompatible-function-pointer-types-strict, which ensures that function pointer signatures match exactly to avoid tripping clang's Control Flow Integrity (kCFI) checks at run time and will eventually be turned on for the kernel, the following instances appear in the NBIOv7.9 code: drivers/gpu/drm/amd/amdgpu/nbio_v7_9.c:465:32: error: incompatible function pointer types initializing 'int (*)(struct amdgpu_device *)' with an expression of type 'enum amdgpu_gfx_partition (struct amdgpu_device *)' [-Werror,-Wincompatible-function-pointer-types-strict] .get_compute_partition_mode = nbio_v7_9_get_compute_partition_mode, ^~~~ drivers/gpu/drm/amd/amdgpu/nbio_v7_9.c:467:31: error: incompatible function pointer types initializing 'u32 (*)(struct amdgpu_device *, u32 *)' (aka 'unsigned int (*)(struct amdgpu_device *, unsigned int *)') with an expression of type 'enum amdgpu_memory_partition (struct amdgpu_device *, u32 *)' (aka 'enum amdgpu_memory_partition (struct amdgpu_device *, unsigned int *)') [-Werror,-Wincompatible-function-pointer-types-strict] .get_memory_partition_mode = nbio_v7_9_get_memory_partition_mode, ^~~ 2 errors generated. Change the return types of these callbacks to match the prototypes to clear up the warning and avoid tripping kCFI at run time. Both functions return a value from ffs(), which is an integer that can fit into either int or unsigned int. Fixes: 11f64eb1472f ("drm/amdgpu: add sysfs node for compute partition mode") Fixes: 41a717ea8afc ("drm/amdgpu: detect current GPU memory partition mode") Signed-off-by: Nathan Chancellor --- drivers/gpu/drm/amd/amdgpu/nbio_v7_9.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/drivers/gpu/drm/amd/amdgpu/nbio_v7_9.c b/drivers/gpu/drm/amd/amdgpu/nbio_v7_9.c index e082f6343d20..d19325476752 100644 --- a/drivers/gpu/drm/amd/amdgpu/nbio_v7_9.c +++ b/drivers/gpu/drm/amd/amdgpu/nbio_v7_9.c @@ -382,7 +382,7 @@ static void nbio_v7_9_enable_doorbell_interrupt(struct amdgpu_device *adev, DOORBELL_INTERRUPT_DISABLE, enable ? 0 : 1); } -static enum amdgpu_gfx_partition nbio_v7_9_get_compute_partition_mode(struct amdgpu_device *adev) +static int nbio_v7_9_get_compute_partition_mode(struct amdgpu_device *adev) { u32 tmp, px; @@ -408,8 +408,8 @@ static void nbio_v7_9_set_compute_partition_mode(struct amdgpu_device *adev, WREG32_SOC15(NBIO, 0, regBIF_BX_PF0_PARTITION_COMPUTE_STATUS, tmp); } -static enum amdgpu_memory_partition -nbio_v7_9_get_memory_partition_mode(struct amdgpu_device *adev, u32 *supp_modes) +static u32 nbio_v7_9_get_memory_partition_mode(struct amdgpu_device *adev, + u32 *supp_modes) { u32 tmp; --- base-commit: fd8f7bb391fa9c1979232cb5ab5bedb08abc855d change-id: 20230524-nbio_v7_9-wincompatible-function-pointer-types-strict-c894636ce146 Best regards, -- Nathan Chancellor
Re: next: clang: x86_64: /intel_display.c:6012:3: error: unannotated fall-through between switch labels [-Werror,-Wimplicit-fallthrough]
Hi Naresh, On Wed, May 24, 2023 at 12:32:24PM +0530, Naresh Kamboju wrote: > Linux next-20230523 and next-20230524 the x86_64 and i386 builds failed > with clang. > > Reported-by: Linux Kernel Functional Testing > > make --silent --keep-going \ > --jobs=8 O=/home/tuxbuild/.cache/tuxmake/builds/1/build ARCH=x86_64 \ > SRCARCH=x86 CROSS_COMPILE=x86_64-linux-gnu- \ > 'HOSTCC=sccache clang' 'CC=sccache clang' \ >LLVM=1 LLVM_IAS=1 > > drivers/gpu/drm/i915/display/intel_display.c:6012:3: error: > unannotated fall-through between switch labels > [-Werror,-Wimplicit-fallthrough] > case I915_FORMAT_MOD_X_TILED: > ^ > drivers/gpu/drm/i915/display/intel_display.c:6012:3: note: insert > 'break;' to avoid fall-through > case I915_FORMAT_MOD_X_TILED: > ^ > break; > 1 error generated. Thanks for the report, I have sent https://lore.kernel.org/20230524-intel_async_flip_check_hw-implicit-fallthrough-v1-1-83de89e37...@kernel.org/ for this. Cheers, Nathan
[PATCH] drm/i915: Fix clang -Wimplicit-fallthrough in intel_async_flip_check_hw()
Clang warns: drivers/gpu/drm/i915/display/intel_display.c:6012:3: error: unannotated fall-through between switch labels [-Werror,-Wimplicit-fallthrough] case I915_FORMAT_MOD_X_TILED: ^ drivers/gpu/drm/i915/display/intel_display.c:6012:3: note: insert 'break;' to avoid fall-through case I915_FORMAT_MOD_X_TILED: ^ break; 1 error generated. Clang is a little more pedantic than GCC, which does not warn when falling through to a case that is just break or return. Clang's version is more in line with the kernel's own stance in deprecated.rst, which states that all switch/case blocks must end in either break, fallthrough, continue, goto, or return. Add the missing break to silence the warning. Fixes: 937859485aef ("drm/i915: Support Async Flip on Linear buffers") Reported-by: kernel test robot Closes: https://lore.kernel.org/202305241902.uvhtmoxa-...@intel.com/ Reported-by: Naresh Kamboju Closes: https://lore.kernel.org/CA+G9fYv68V3ewK0Qj-syQj7qX-hQr0H1MFL=qfnudoe_j2z...@mail.gmail.com/ Signed-off-by: Nathan Chancellor --- drivers/gpu/drm/i915/display/intel_display.c | 1 + 1 file changed, 1 insertion(+) diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c index 0490c6412ab5..6d49e0ab3e85 100644 --- a/drivers/gpu/drm/i915/display/intel_display.c +++ b/drivers/gpu/drm/i915/display/intel_display.c @@ -6008,6 +6008,7 @@ static int intel_async_flip_check_hw(struct intel_atomic_state *state, struct in plane->base.base.id, plane->base.name); return -EINVAL; } + break; case I915_FORMAT_MOD_X_TILED: case I915_FORMAT_MOD_Y_TILED: --- base-commit: 9a2cb1b31c040e2f1b313e2f7921f0f5e6b66d82 change-id: 20230524-intel_async_flip_check_hw-implicit-fallthrough-c4c40b03802f Best regards, -- Nathan Chancellor
Re: [PATCH v5 2/3] drm/panel: Add Samsung S6D7AA0 panel controller driver
Hi Artur, On Fri, May 19, 2023 at 07:03:53PM +0200, Artur Weber wrote: > Initial driver for S6D7AA0-controlled panels. Currently, the following > panels are supported: > > - S6D7AA0-LSL080AL02 (Samsung Galaxy Tab 3 8.0) > - S6D7AA0-LSL080AL03 (Samsung Galaxy Tab A 8.0 2015) > - S6D7AA0-LTL101AT01 (Samsung Galaxy Tab A 9.7 2015) > > It should be possible to extend this driver to work with other panels > using this IC. > > Tested-by: Nikita Travkin #ltl101at01 > Signed-off-by: Artur Weber This change as commit 6810bb390282 ("drm/panel: Add Samsung S6D7AA0 panel controller driver") in -next causes the following build errors with clang and GCC older than 8.x (the kernel supports back to GCC 5.1). With clang: drivers/gpu/drm/panel/panel-samsung-s6d7aa0.c:312:14: error: initializer element is not a compile-time constant .drm_mode = s6d7aa0_lsl080al02_mode, ^~~ drivers/gpu/drm/panel/panel-samsung-s6d7aa0.c:415:14: error: initializer element is not a compile-time constant .drm_mode = s6d7aa0_lsl080al03_mode, ^~~ drivers/gpu/drm/panel/panel-samsung-s6d7aa0.c:443:14: error: initializer element is not a compile-time constant .drm_mode = s6d7aa0_ltl101at01_mode, ^~~ 3 errors generated. With GCC: drivers/gpu/drm/panel/panel-samsung-s6d7aa0.c:312:14: error: initializer element is not constant .drm_mode = s6d7aa0_lsl080al02_mode, ^~~ drivers/gpu/drm/panel/panel-samsung-s6d7aa0.c:312:14: note: (near initialization for 's6d7aa0_lsl080al02_desc.drm_mode') drivers/gpu/drm/panel/panel-samsung-s6d7aa0.c:415:14: error: initializer element is not constant .drm_mode = s6d7aa0_lsl080al03_mode, ^~~ drivers/gpu/drm/panel/panel-samsung-s6d7aa0.c:415:14: note: (near initialization for 's6d7aa0_lsl080al03_desc.drm_mode') drivers/gpu/drm/panel/panel-samsung-s6d7aa0.c:443:14: error: initializer element is not constant .drm_mode = s6d7aa0_ltl101at01_mode, ^~~ drivers/gpu/drm/panel/panel-samsung-s6d7aa0.c:443:14: note: (near initialization for 's6d7aa0_ltl101at01_desc.drm_mode') You can find these toolchains at https://mirrors.edge.kernel.org/pub/tools/crosstool/ and https://mirrors.edge.kernel.org/pub/tools/llvm/ if you need help testing. clang may eventually match GCC's newer behavior but there appears to be some unresolved concerns around the proposed implementation and we have not been able to double back to it: https://reviews.llvm.org/D76096 > +static const struct drm_display_mode s6d7aa0_lsl080al03_mode = { > + .clock = (768 + 18 + 16 + 126) * (1024 + 8 + 2 + 6) * 60 / 1000, > + .hdisplay = 768, > + .hsync_start = 768 + 18, > + .hsync_end = 768 + 18 + 16, > + .htotal = 768 + 18 + 16 + 126, > + .vdisplay = 1024, > + .vsync_start = 1024 + 8, > + .vsync_end = 1024 + 8 + 2, > + .vtotal = 1024 + 8 + 2 + 6, > + .width_mm = 122, > + .height_mm = 163, > +}; > + > +static const struct s6d7aa0_panel_desc s6d7aa0_lsl080al03_desc = { > + .panel_type = S6D7AA0_PANEL_LSL080AL03, > + .init_func = s6d7aa0_lsl080al03_init, > + .off_func = s6d7aa0_lsl080al03_off, > + .drm_mode = s6d7aa0_lsl080al03_mode, > + .mode_flags = MIPI_DSI_MODE_NO_EOT_PACKET, > + .bus_flags = 0, > + > + .has_backlight = true, > + .use_passwd3 = true, > +}; > + > +/* Initialization structures for LTL101AT01 panel */ > + > +static const struct drm_display_mode s6d7aa0_ltl101at01_mode = { > + .clock = (768 + 96 + 16 + 184) * (1024 + 8 + 2 + 6) * 60 / 1000, > + .hdisplay = 768, > + .hsync_start = 768 + 96, > + .hsync_end = 768 + 96 + 16, > + .htotal = 768 + 96 + 16 + 184, > + .vdisplay = 1024, > + .vsync_start = 1024 + 8, > + .vsync_end = 1024 + 8 + 2, > + .vtotal = 1024 + 8 + 2 + 6, > + .width_mm = 148, > + .height_mm = 197, > +}; > + > +static const struct s6d7aa0_panel_desc s6d7aa0_ltl101at01_desc = { > + .panel_type = S6D7AA0_PANEL_LTL101AT01, > + .init_func = s6d7aa0_lsl080al03_init, /* Similar init to LSL080AL03 */ > + .off_func = s6d7aa0_lsl080al03_off, > + .drm_mode = s6d7aa0_ltl101at01_mode, > + .mode_flags = MIPI_DSI_MODE_NO_EOT_PACKET, > + .bus_flags = 0, > + > + .has_backlight = true, > + .use_passwd3 = true, > +}; Cheers, Nathan
Re: [PATCH v2 1/2] phy: mediatek: hdmi: mt8195: fix uninitialized variable usage in pll_calc
On Fri, Apr 14, 2023 at 06:07:46PM +0200, Guillaume Ranquet wrote: > The ret variable in mtk_hdmi_pll_calc() was used unitialized as reported > by the kernel test robot. > > Fix the issue by removing the variable altogether and testing out the > return value of mtk_hdmi_pll_set_hw() > > Fixes: 45810d486bb44 ("phy: mediatek: add support for phy-mtk-hdmi-mt8195") > Reported-by: kernel test robot > Signed-off-by: Guillaume Ranquet Reviewed-by: Nathan Chancellor Can somebody pick this up? It fixes a rather obvious warning, which is breaking clang builds (as evidenced by three versions of the same fix). > --- > drivers/phy/mediatek/phy-mtk-hdmi-mt8195.c | 8 ++-- > 1 file changed, 2 insertions(+), 6 deletions(-) > > diff --git a/drivers/phy/mediatek/phy-mtk-hdmi-mt8195.c > b/drivers/phy/mediatek/phy-mtk-hdmi-mt8195.c > index abfc077fb0a8..054b73cb31ee 100644 > --- a/drivers/phy/mediatek/phy-mtk-hdmi-mt8195.c > +++ b/drivers/phy/mediatek/phy-mtk-hdmi-mt8195.c > @@ -213,7 +213,7 @@ static int mtk_hdmi_pll_calc(struct mtk_hdmi_phy > *hdmi_phy, struct clk_hw *hw, > u64 tmds_clk, pixel_clk, da_hdmitx21_ref_ck, ns_hdmipll_ck, pcw; > u8 txpredivs[4] = { 2, 4, 6, 12 }; > u32 fbkdiv_low; > - int i, ret; > + int i; > > pixel_clk = rate; > tmds_clk = pixel_clk; > @@ -292,13 +292,9 @@ static int mtk_hdmi_pll_calc(struct mtk_hdmi_phy > *hdmi_phy, struct clk_hw *hw, > if (!(digital_div <= 32 && digital_div >= 1)) > return -EINVAL; > > - mtk_hdmi_pll_set_hw(hw, PLL_PREDIV, fbkdiv_high, fbkdiv_low, > + return mtk_hdmi_pll_set_hw(hw, PLL_PREDIV, fbkdiv_high, fbkdiv_low, > PLL_FBKDIV_HS3, posdiv1, posdiv2, txprediv, > txposdiv, digital_div); > - if (ret) > - return -EINVAL; > - > - return 0; > } > > static int mtk_hdmi_pll_drv_setting(struct clk_hw *hw) > > -- > 2.40.0 >
Re: linux-next: manual merge of the drm-misc tree with the mm-stable tree
On Wed, Apr 19, 2023 at 06:24:37PM +0200, Daniel Vetter wrote: > On Tue, Apr 18, 2023 at 07:34:44PM +0100, Mark Brown wrote: > > On Sun, Apr 16, 2023 at 09:58:50AM +0200, Daniel Vetter wrote: > > > > > Note there was a ppc compile fail, which is why we pushed the ttm revert. > > > That /should/ be fixed now, but would be good if you can confirm? > > > > According to Nathan (CCed) there's still issues with the interaction > > with the PowerPC tree. > > So this revert was supposed to fix this: 56e51681246e ("drm/ttm: revert > "Reduce the number of used allocation orders for TTM pages"") > > If there's anything left then I need to chase that asap since the merge > window will open soon. I think we are talking about two different issues here. My issue is not a compilation failure, it is an incorrect merge resolution that is happening in -next because of two independent changes in the drm and powerpc tree, the thread below should have more information. https://lore.kernel.org/20230413184725.GA3183133@dev-arch.thelio-3990X/ I do not think this is something that either tree can solve independently of each other, -next has to resolve the conflict correctly (which is what I point out in the message above) and a note of it should be passed along to Linus so it can be resolved correctly in mainline when the time comes. Cheers, Nathan
Re: linux-next: manual merge of the drm tree with the powerpc tree
On Tue, Apr 18, 2023 at 07:25:00PM +0100, Mark Brown wrote: > On Tue, Apr 18, 2023 at 11:21:45AM -0700, Nathan Chancellor wrote: > > On Fri, Apr 14, 2023 at 05:55:10PM +0100, Mark Brown wrote: > > > > Done. > > > Thanks a lot, sorry for not saying it sooner! It looks like this > > regressed in next-20230417 and next-20230418 though. > > Someone sent a mail saying they thought they'd fixed the DRM tree - is > that not the case? Does not seem like it: $ git show -s --format='%h ("%s")' 67d5d9f013d6 ("Add linux-next specific files for 20230418") $ git grep DRM_AMD_DC_DCN drivers/gpu/drm/amd/display/Kconfig:select DRM_AMD_DC_DCN if (X86 || (PPC64 && ALTIVEC) || (ARM64 && KERNEL_MODE_NEON && !CC_IS_CLANG)) Cheers, Nathan
Re: linux-next: manual merge of the drm tree with the powerpc tree
On Fri, Apr 14, 2023 at 05:55:10PM +0100, Mark Brown wrote: > On Thu, Apr 13, 2023 at 11:47:25AM -0700, Nathan Chancellor wrote: > > On Wed, Apr 12, 2023 at 11:22:13AM +1000, Stephen Rothwell wrote: > > > select SND_HDA_COMPONENT if SND_HDA_CORE > > # !CC_IS_CLANG: https://github.com/ClangBuiltLinux/linux/issues/1752 > > - select DRM_AMD_DC_DCN if (X86 || (PPC64 && ALTIVEC) || (ARM64 && > > KERNEL_MODE_NEON && !CC_IS_CLANG)) > > + select DRM_AMD_DC_FP if (X86 || (PPC64 && ALTIVEC) || (ARM64 && > > KERNEL_MODE_NEON && !CC_IS_CLANG)) > > help > > Choose this option if you want to use the new display engine > > support for AMDGPU. This adds required support for Vega and > > > Please consider resolving this in a future -next update, I was rather > > surprised that my AMD test machine's graphical output was not working > > until I noticed the configuration difference :) > > Done. Thanks a lot, sorry for not saying it sooner! It looks like this regressed in next-20230417 and next-20230418 though. Cheers, Nathan
Re: [PATCH] phy: mediatek: fix returning garbage
On Fri, Apr 14, 2023 at 08:22:53AM -0400, Tom Rix wrote: > clang reports > drivers/phy/mediatek/phy-mtk-hdmi-mt8195.c:298:6: error: variable > 'ret' is uninitialized when used here [-Werror,-Wuninitialized] > if (ret) > ^~~ > ret should have been set by the preceding call to mtk_hdmi_pll_set_hw. > > Fixes: 45810d486bb4 ("phy: mediatek: add support for phy-mtk-hdmi-mt8195") > Signed-off-by: Tom Rix Reviewed-by: Nathan Chancellor Angelo brought up a good point on another patch to fix this issue that we should probably be returning ret instead of unconditionally returning -EINVAL but it sounds like it does not really matter at the moment since mtk_hdmi_pll_set_hw() can only return -EINVAL or 0. https://lore.kernel.org/ada769e0-0141-634f-3753-eb3a50f0e...@collabora.com/ > --- > drivers/phy/mediatek/phy-mtk-hdmi-mt8195.c | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > > diff --git a/drivers/phy/mediatek/phy-mtk-hdmi-mt8195.c > b/drivers/phy/mediatek/phy-mtk-hdmi-mt8195.c > index abfc077fb0a8..c63294e451d6 100644 > --- a/drivers/phy/mediatek/phy-mtk-hdmi-mt8195.c > +++ b/drivers/phy/mediatek/phy-mtk-hdmi-mt8195.c > @@ -292,9 +292,9 @@ static int mtk_hdmi_pll_calc(struct mtk_hdmi_phy > *hdmi_phy, struct clk_hw *hw, > if (!(digital_div <= 32 && digital_div >= 1)) > return -EINVAL; > > - mtk_hdmi_pll_set_hw(hw, PLL_PREDIV, fbkdiv_high, fbkdiv_low, > - PLL_FBKDIV_HS3, posdiv1, posdiv2, txprediv, > - txposdiv, digital_div); > + ret = mtk_hdmi_pll_set_hw(hw, PLL_PREDIV, fbkdiv_high, fbkdiv_low, > + PLL_FBKDIV_HS3, posdiv1, posdiv2, txprediv, > + txposdiv, digital_div); > if (ret) > return -EINVAL; > > -- > 2.27.0 >
Re: linux-next: manual merge of the drm tree with the powerpc tree
Hi Mark, On Wed, Apr 12, 2023 at 11:22:13AM +1000, Stephen Rothwell wrote: > Hi all, > > Today's linux-next merge of the drm tree got a conflict in: > > drivers/gpu/drm/amd/display/Kconfig > > between commit: > > 78f0929884d4 ("powerpc/64: Always build with 128-bit long double") > > from the powerpc tree and commit: > > 4652ae7a51b7 ("drm/amd/display: Rename DCN config to FP") > > from the drm tree. > > I fixed it up (I used the powerpc version - with "(PPC64 && ALTIVEC)") > and can carry the fix as necessary. This is now fixed as far as > linux-next is concerned, but any non trivial conflicts should be > mentioned to your upstream maintainer when your tree is submitted for > merging. You may also want to consider cooperating with the maintainer > of the conflicting tree to minimise any particularly complex conflicts. This resolution is not quite right on next-20230412 and next-20230413, as the drm tree's rename was not taken into account on the conflicting line. In other words, I need this diff for everything to work properly. diff --git a/drivers/gpu/drm/amd/display/Kconfig b/drivers/gpu/drm/amd/display/Kconfig index b990ef80d686..2d8e55e29637 100644 --- a/drivers/gpu/drm/amd/display/Kconfig +++ b/drivers/gpu/drm/amd/display/Kconfig @@ -8,7 +8,7 @@ config DRM_AMD_DC depends on BROKEN || !CC_IS_CLANG || X86_64 || SPARC64 || ARM64 select SND_HDA_COMPONENT if SND_HDA_CORE # !CC_IS_CLANG: https://github.com/ClangBuiltLinux/linux/issues/1752 - select DRM_AMD_DC_DCN if (X86 || (PPC64 && ALTIVEC) || (ARM64 && KERNEL_MODE_NEON && !CC_IS_CLANG)) + select DRM_AMD_DC_FP if (X86 || (PPC64 && ALTIVEC) || (ARM64 && KERNEL_MODE_NEON && !CC_IS_CLANG)) help Choose this option if you want to use the new display engine support for AMDGPU. This adds required support for Vega and Please consider resolving this in a future -next update, I was rather surprised that my AMD test machine's graphical output was not working until I noticed the configuration difference :) Cheers, Nathan
Re: [PATCH v2] drm/vblank: Fix for drivers that do not drm_vblank_init()
On Mon, Apr 03, 2023 at 09:03:14AM -0700, Rob Clark wrote: > From: Rob Clark > > This should fix a crash that was reported on ast (and possibly other > drivers which do not initialize vblank). > >fbcon: Taking over console >Unable to handle kernel NULL pointer dereference at virtual address > 0074 >Mem abort info: > ESR = 0x9604 > EC = 0x25: DABT (current EL), IL = 32 bits > SET = 0, FnV = 0 > EA = 0, S1PTW = 0 > FSC = 0x04: level 0 translation fault >Data abort info: > ISV = 0, ISS = 0x0004 > CM = 0, WnR = 0 >user pgtable: 4k pages, 48-bit VAs, pgdp=080009d16000 >[0074] pgd=, p4d= >Internal error: Oops: 9604 [#1] SMP >Modules linked in: ip6table_nat tun nft_fib_inet nft_fib_ipv4 nft_fib_ipv6 > nft_fib nft_reject_inet nf_reject_ipv4 nf_reject_ipv6 nft_reject nft_ct > nft_chain_nat nf_nat nf_conntrack nf_defrag_ipv6 nf_defrag_ipv4 rfkill ip_set > nf_tables nfnetlink qrtr sunrpc binfmt_misc vfat fat xfs snd_usb_audio > snd_hwdep snd_usbmidi_lib snd_seq snd_pcm snd_rawmidi snd_timer > snd_seq_device snd soundcore joydev mc ipmi_ssif ipmi_devintf ipmi_msghandler > arm_spe_pmu arm_cmn arm_dsu_pmu arm_dmc620_pmu cppc_cpufreq loop zram > crct10dif_ce polyval_ce nvme polyval_generic ghash_ce sbsa_gwdt igb nvme_core > ast nvme_common i2c_algo_bit xgene_hwmon gpio_dwapb scsi_dh_rdac scsi_dh_emc > scsi_dh_alua ip6_tables ip_tables dm_multipath fuse >CPU: 12 PID: 469 Comm: kworker/12:1 Not tainted > 6.3.0-rc2-8-gd39e48ca80c0 #1 >Hardware name: ADLINK AVA Developer Platform/AVA Developer Platform, BIOS > TianoCore 2.04.100.07 (SYS: 2.06.20220308) 09/08/2022 >Workqueue: events fbcon_register_existing_fbs >pstate: 2049 (nzCv daif +PAN -UAO -TCO -DIT -SSBS BTYPE=--) >pc : drm_crtc_next_vblank_start+0x2c/0x98 >lr : drm_atomic_helper_wait_for_fences+0x90/0x240 >sp : 8d583960 >x29: 8d583960 x28: 07ff8fc187b0 x27: >x26: 07ff99c08c00 x25: 0038 x24: 07ff99c0c000 >x23: 0001 x22: 0038 x21: >x20: 07ff9640a280 x19: x18: >x17: x16: b24d2eece1c0 x15: 003038303178 >x14: 303239310048 x13: x12: >x11: x10: x9 : b24d2eeeaca0 >x8 : 8d583628 x7 : 080077783000 x6 : >x5 : 8d584000 x4 : 07ff99c0c000 x3 : 0130 >x2 : x1 : 8d5839c0 x0 : 07ff99c0cc08 >Call trace: > drm_crtc_next_vblank_start+0x2c/0x98 > drm_atomic_helper_wait_for_fences+0x90/0x240 > drm_atomic_helper_commit+0xb0/0x188 > drm_atomic_commit+0xb0/0xf0 > drm_client_modeset_commit_atomic+0x218/0x280 > drm_client_modeset_commit_locked+0x64/0x1a0 > drm_client_modeset_commit+0x38/0x68 > __drm_fb_helper_restore_fbdev_mode_unlocked+0xb0/0xf8 > drm_fb_helper_set_par+0x44/0x88 > fbcon_init+0x1e0/0x4a8 > visual_init+0xbc/0x118 > do_bind_con_driver.isra.0+0x194/0x3a0 > do_take_over_console+0x50/0x70 > do_fbcon_takeover+0x74/0xf8 > do_fb_registered+0x13c/0x158 > fbcon_register_existing_fbs+0x78/0xc0 > process_one_work+0x1ec/0x478 > worker_thread+0x74/0x418 > kthread+0xec/0x100 > ret_from_fork+0x10/0x20 >Code: f944 b9409013 f940a082 9ba30a73 (b9407662) >---[ end trace ]--- > > v2: Use drm_dev_has_vblank() > > Reported-by: Nathan Chancellor > Fixes: d39e48ca80c0 ("drm/atomic-helper: Set fence deadline for vblank") > Signed-off-by: Rob Clark > Reviewed-by: Thomas Zimmermann Still appears to work for me: Tested-by: Nathan Chancellor > --- > drivers/gpu/drm/drm_vblank.c | 10 -- > 1 file changed, 8 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/drm_vblank.c b/drivers/gpu/drm/drm_vblank.c > index 299fa2a19a90..877e2067534f 100644 > --- a/drivers/gpu/drm/drm_vblank.c > +++ b/drivers/gpu/drm/drm_vblank.c > @@ -996,10 +996,16 @@ EXPORT_SYMBOL(drm_crtc_vblank_count_and_time); > int drm_crtc_next_vblank_start(struct drm_crtc *crtc, ktime_t *vblanktime) > { > unsigned int pipe = drm_crtc_index(crtc); > - struct drm_vblank_crtc *vblank = &crtc->dev->vblank[pipe]; > - struct drm_display_mode *mode = &vblank->hwmode; > + struct drm_vblank_crtc *vblank; > + struct drm_display_mode *mode; > u64 vblank_start; > > + if (!drm_dev_has_vblank(crtc->dev)) > + return -EINVAL; > + > + vblank = &crtc->dev->vblank[pipe]; > + mode = &vblank->hwmode; > + > if (!vblank->framedur_ns || !vblank->linedur_ns) > return -EINVAL; > > -- > 2.39.2 >
Re: [PATCH] drm/vblank: Fix for drivers that do not drm_vblank_init()
On Sat, Apr 01, 2023 at 08:38:02AM -0700, Rob Clark wrote: > From: Rob Clark > > This should fix a crash that was reported on ast (and possibly other > drivers which do not initialize vblank). > >fbcon: Taking over console >Unable to handle kernel NULL pointer dereference at virtual address > 0074 >Mem abort info: > ESR = 0x9604 > EC = 0x25: DABT (current EL), IL = 32 bits > SET = 0, FnV = 0 > EA = 0, S1PTW = 0 > FSC = 0x04: level 0 translation fault >Data abort info: > ISV = 0, ISS = 0x0004 > CM = 0, WnR = 0 >user pgtable: 4k pages, 48-bit VAs, pgdp=080009d16000 >[0074] pgd=, p4d= >Internal error: Oops: 9604 [#1] SMP >Modules linked in: ip6table_nat tun nft_fib_inet nft_fib_ipv4 nft_fib_ipv6 > nft_fib nft_reject_inet nf_reject_ipv4 nf_reject_ipv6 nft_reject nft_ct > nft_chain_nat nf_nat nf_conntrack nf_defrag_ipv6 nf_defrag_ipv4 rfkill ip_set > nf_tables nfnetlink qrtr sunrpc binfmt_misc vfat fat xfs snd_usb_audio > snd_hwdep snd_usbmidi_lib snd_seq snd_pcm snd_rawmidi snd_timer > snd_seq_device snd soundcore joydev mc ipmi_ssif ipmi_devintf ipmi_msghandler > arm_spe_pmu arm_cmn arm_dsu_pmu arm_dmc620_pmu cppc_cpufreq loop zram > crct10dif_ce polyval_ce nvme polyval_generic ghash_ce sbsa_gwdt igb nvme_core > ast nvme_common i2c_algo_bit xgene_hwmon gpio_dwapb scsi_dh_rdac scsi_dh_emc > scsi_dh_alua ip6_tables ip_tables dm_multipath fuse >CPU: 12 PID: 469 Comm: kworker/12:1 Not tainted > 6.3.0-rc2-8-gd39e48ca80c0 #1 >Hardware name: ADLINK AVA Developer Platform/AVA Developer Platform, BIOS > TianoCore 2.04.100.07 (SYS: 2.06.20220308) 09/08/2022 >Workqueue: events fbcon_register_existing_fbs >pstate: 2049 (nzCv daif +PAN -UAO -TCO -DIT -SSBS BTYPE=--) >pc : drm_crtc_next_vblank_start+0x2c/0x98 >lr : drm_atomic_helper_wait_for_fences+0x90/0x240 >sp : 8d583960 >x29: 8d583960 x28: 07ff8fc187b0 x27: >x26: 07ff99c08c00 x25: 0038 x24: 07ff99c0c000 >x23: 0001 x22: 0038 x21: >x20: 07ff9640a280 x19: x18: >x17: x16: b24d2eece1c0 x15: 003038303178 >x14: 303239310048 x13: x12: >x11: x10: x9 : b24d2eeeaca0 >x8 : 8d583628 x7 : 080077783000 x6 : >x5 : 8d584000 x4 : 07ff99c0c000 x3 : 0130 >x2 : x1 : 8d5839c0 x0 : 07ff99c0cc08 >Call trace: > drm_crtc_next_vblank_start+0x2c/0x98 > drm_atomic_helper_wait_for_fences+0x90/0x240 > drm_atomic_helper_commit+0xb0/0x188 > drm_atomic_commit+0xb0/0xf0 > drm_client_modeset_commit_atomic+0x218/0x280 > drm_client_modeset_commit_locked+0x64/0x1a0 > drm_client_modeset_commit+0x38/0x68 > __drm_fb_helper_restore_fbdev_mode_unlocked+0xb0/0xf8 > drm_fb_helper_set_par+0x44/0x88 > fbcon_init+0x1e0/0x4a8 > visual_init+0xbc/0x118 > do_bind_con_driver.isra.0+0x194/0x3a0 > do_take_over_console+0x50/0x70 > do_fbcon_takeover+0x74/0xf8 > do_fb_registered+0x13c/0x158 > fbcon_register_existing_fbs+0x78/0xc0 > process_one_work+0x1ec/0x478 > worker_thread+0x74/0x418 > kthread+0xec/0x100 > ret_from_fork+0x10/0x20 >Code: f944 b9409013 f940a082 9ba30a73 (b9407662) >---[ end trace 0000 ]--- > > Reported-by: Nathan Chancellor > Fixes: d39e48ca80c0 ("drm/atomic-helper: Set fence deadline for vblank") > Signed-off-by: Rob Clark Seems to work for me, I no longer see the above crash. Tested-by: Nathan Chancellor > --- > drivers/gpu/drm/drm_vblank.c | 10 -- > 1 file changed, 8 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/drm_vblank.c b/drivers/gpu/drm/drm_vblank.c > index 299fa2a19a90..e98e3cefba3a 100644 > --- a/drivers/gpu/drm/drm_vblank.c > +++ b/drivers/gpu/drm/drm_vblank.c > @@ -996,10 +996,16 @@ EXPORT_SYMBOL(drm_crtc_vblank_count_and_time); > int drm_crtc_next_vblank_start(struct drm_crtc *crtc, ktime_t *vblanktime) > { > unsigned int pipe = drm_crtc_index(crtc); > - struct drm_vblank_crtc *vblank = &crtc->dev->vblank[pipe]; > - struct drm_display_mode *mode = &vblank->hwmode; > + struct drm_vblank_crtc *vblank; > + struct drm_display_mode *mode; > u64 vblank_start; > > + if (!crtc->dev->vblank) > + return -EINVAL; > + > + vblank = &crtc->dev->vblank[pipe]; > + mode = &vblank->hwmode; > + > if (!vblank->framedur_ns || !vblank->linedur_ns) > return -EINVAL; > > -- > 2.39.2 >
Re: [PATCH v10 11/15] drm/atomic-helper: Set fence deadline for vblank
On Fri, Mar 31, 2023 at 03:14:30PM -0700, Rob Clark wrote: > On Fri, Mar 31, 2023 at 1:44 PM Nathan Chancellor wrote: > > > > Hi Rob, > > > > On Wed, Mar 08, 2023 at 07:53:02AM -0800, Rob Clark wrote: > > > From: Rob Clark > > > > > > For an atomic commit updating a single CRTC (ie. a pageflip) calculate > > > the next vblank time, and inform the fence(s) of that deadline. > > > > > > v2: Comment typo fix (danvet) > > > v3: If there are multiple CRTCs, consider the time of the soonest vblank > > > > > > Signed-off-by: Rob Clark > > > Reviewed-by: Daniel Vetter > > > Signed-off-by: Rob Clark > > > > I apologize if this has already been reported or fixed, I searched lore > > but did not find anything. > > > > This change as commit d39e48ca80c0 ("drm/atomic-helper: Set fence > > deadline for vblank") in -next causes a hang while running LTP's > > read_all test on /proc on my Ampere Altra system (it seems it is hanging > > on a pagemap file?). Additionally, I have this splat in dmesg, which > > seems related based on the call stack. > > Hi, I'm not familiar with this hardware.. do you know which drm driver > is used? I can't tell from the call-stack. I think it is drivers/gpu/drm/ast, as I see ast in lsmod? > > [ 20.542591] fbcon: Taking over console > > [ 20.550772] Unable to handle kernel NULL pointer dereference at virtual > > address 0074 > > [ 20.550776] Mem abort info: > > [ 20.550777] ESR = 0x9604 > > [ 20.550779] EC = 0x25: DABT (current EL), IL = 32 bits > > [ 20.550781] SET = 0, FnV = 0 > > [ 20.550782] EA = 0, S1PTW = 0 > > [ 20.550784] FSC = 0x04: level 0 translation fault > > [ 20.550785] Data abort info: > > [ 20.550786] ISV = 0, ISS = 0x0004 > > [ 20.550788] CM = 0, WnR = 0 > > [ 20.550789] user pgtable: 4k pages, 48-bit VAs, pgdp=080009d16000 > > [ 20.550791] [0074] pgd=, p4d= > > [ 20.550796] Internal error: Oops: 9604 [#1] SMP > > [ 20.550800] Modules linked in: ip6table_nat tun nft_fib_inet > > nft_fib_ipv4 nft_fib_ipv6 nft_fib nft_reject_inet nf_reject_ipv4 > > nf_reject_ipv6 nft_reject nft_ct nft_chain_nat nf_nat nf_conntrack > > nf_defrag_ipv6 nf_defrag_ipv4 rfkill ip_set nf_tables nfnetlink qrtr sunrpc > > binfmt_misc vfat fat xfs snd_usb_audio snd_hwdep snd_usbmidi_lib snd_seq > > snd_pcm snd_rawmidi snd_timer snd_seq_device snd soundcore joydev mc > > ipmi_ssif ipmi_devintf ipmi_msghandler arm_spe_pmu arm_cmn arm_dsu_pmu > > arm_dmc620_pmu cppc_cpufreq loop zram crct10dif_ce polyval_ce nvme > > polyval_generic ghash_ce sbsa_gwdt igb nvme_core ast nvme_common > > i2c_algo_bit xgene_hwmon gpio_dwapb scsi_dh_rdac scsi_dh_emc scsi_dh_alua > > ip6_tables ip_tables dm_multipath fuse > > [ 20.550869] CPU: 12 PID: 469 Comm: kworker/12:1 Not tainted > > 6.3.0-rc2-8-gd39e48ca80c0 #1 > > [ 20.550872] Hardware name: ADLINK AVA Developer Platform/AVA Developer > > Platform, BIOS TianoCore 2.04.100.07 (SYS: 2.06.20220308) 09/08/2022 > > [ 20.550875] Workqueue: events fbcon_register_existing_fbs > > [ 20.550884] pstate: 2049 (nzCv daif +PAN -UAO -TCO -DIT -SSBS > > BTYPE=--) > > [ 20.550888] pc : drm_crtc_next_vblank_start+0x2c/0x98 > > [ 20.550894] lr : drm_atomic_helper_wait_for_fences+0x90/0x240 > > [ 20.550898] sp : 8d583960 > > [ 20.550900] x29: 8d583960 x28: 07ff8fc187b0 x27: > > > > [ 20.550904] x26: 07ff99c08c00 x25: 0038 x24: > > 07ff99c0c000 > > [ 20.550908] x23: 0001 x22: 0038 x21: > > > > [ 20.550912] x20: 07ff9640a280 x19: x18: > > > > [ 20.550915] x17: x16: b24d2eece1c0 x15: > > 003038303178 > > [ 20.550919] x14: 303239310048 x13: x12: > > > > [ 20.550923] x11: x10: x9 : > > b24d2eeeaca0 > > [ 20.550926] x8 : 8d583628 x7 : 080077783000 x6 : > > > > [ 20.550930] x5 : 8d584000 x4 : 07ff99c0c000 x3 : > > 0130 > > [ 20.550934] x2 : x1 : 8d5839c0 x0 : > > 07ff99c0cc08 > > [ 20.550937] Call trace: > > [ 20.550939] drm_crtc_next_vblank_start+0x2c/0x98 > > [ 20.550942] drm_atomic_helper_wait_for_fences+0
Re: [PATCH v10 11/15] drm/atomic-helper: Set fence deadline for vblank
Hi Rob, On Wed, Mar 08, 2023 at 07:53:02AM -0800, Rob Clark wrote: > From: Rob Clark > > For an atomic commit updating a single CRTC (ie. a pageflip) calculate > the next vblank time, and inform the fence(s) of that deadline. > > v2: Comment typo fix (danvet) > v3: If there are multiple CRTCs, consider the time of the soonest vblank > > Signed-off-by: Rob Clark > Reviewed-by: Daniel Vetter > Signed-off-by: Rob Clark I apologize if this has already been reported or fixed, I searched lore but did not find anything. This change as commit d39e48ca80c0 ("drm/atomic-helper: Set fence deadline for vblank") in -next causes a hang while running LTP's read_all test on /proc on my Ampere Altra system (it seems it is hanging on a pagemap file?). Additionally, I have this splat in dmesg, which seems related based on the call stack. [ 20.542591] fbcon: Taking over console [ 20.550772] Unable to handle kernel NULL pointer dereference at virtual address 0074 [ 20.550776] Mem abort info: [ 20.550777] ESR = 0x9604 [ 20.550779] EC = 0x25: DABT (current EL), IL = 32 bits [ 20.550781] SET = 0, FnV = 0 [ 20.550782] EA = 0, S1PTW = 0 [ 20.550784] FSC = 0x04: level 0 translation fault [ 20.550785] Data abort info: [ 20.550786] ISV = 0, ISS = 0x0004 [ 20.550788] CM = 0, WnR = 0 [ 20.550789] user pgtable: 4k pages, 48-bit VAs, pgdp=080009d16000 [ 20.550791] [0074] pgd=, p4d= [ 20.550796] Internal error: Oops: 9604 [#1] SMP [ 20.550800] Modules linked in: ip6table_nat tun nft_fib_inet nft_fib_ipv4 nft_fib_ipv6 nft_fib nft_reject_inet nf_reject_ipv4 nf_reject_ipv6 nft_reject nft_ct nft_chain_nat nf_nat nf_conntrack nf_defrag_ipv6 nf_defrag_ipv4 rfkill ip_set nf_tables nfnetlink qrtr sunrpc binfmt_misc vfat fat xfs snd_usb_audio snd_hwdep snd_usbmidi_lib snd_seq snd_pcm snd_rawmidi snd_timer snd_seq_device snd soundcore joydev mc ipmi_ssif ipmi_devintf ipmi_msghandler arm_spe_pmu arm_cmn arm_dsu_pmu arm_dmc620_pmu cppc_cpufreq loop zram crct10dif_ce polyval_ce nvme polyval_generic ghash_ce sbsa_gwdt igb nvme_core ast nvme_common i2c_algo_bit xgene_hwmon gpio_dwapb scsi_dh_rdac scsi_dh_emc scsi_dh_alua ip6_tables ip_tables dm_multipath fuse [ 20.550869] CPU: 12 PID: 469 Comm: kworker/12:1 Not tainted 6.3.0-rc2-8-gd39e48ca80c0 #1 [ 20.550872] Hardware name: ADLINK AVA Developer Platform/AVA Developer Platform, BIOS TianoCore 2.04.100.07 (SYS: 2.06.20220308) 09/08/2022 [ 20.550875] Workqueue: events fbcon_register_existing_fbs [ 20.550884] pstate: 2049 (nzCv daif +PAN -UAO -TCO -DIT -SSBS BTYPE=--) [ 20.550888] pc : drm_crtc_next_vblank_start+0x2c/0x98 [ 20.550894] lr : drm_atomic_helper_wait_for_fences+0x90/0x240 [ 20.550898] sp : 8d583960 [ 20.550900] x29: 8d583960 x28: 07ff8fc187b0 x27: [ 20.550904] x26: 07ff99c08c00 x25: 0038 x24: 07ff99c0c000 [ 20.550908] x23: 0001 x22: 0038 x21: [ 20.550912] x20: 07ff9640a280 x19: x18: [ 20.550915] x17: x16: b24d2eece1c0 x15: 003038303178 [ 20.550919] x14: 303239310048 x13: x12: [ 20.550923] x11: x10: x9 : b24d2eeeaca0 [ 20.550926] x8 : 8d583628 x7 : 080077783000 x6 : [ 20.550930] x5 : 8d584000 x4 : 07ff99c0c000 x3 : 0130 [ 20.550934] x2 : x1 : 8d5839c0 x0 : 07ff99c0cc08 [ 20.550937] Call trace: [ 20.550939] drm_crtc_next_vblank_start+0x2c/0x98 [ 20.550942] drm_atomic_helper_wait_for_fences+0x90/0x240 [ 20.550946] drm_atomic_helper_commit+0xb0/0x188 [ 20.550949] drm_atomic_commit+0xb0/0xf0 [ 20.550953] drm_client_modeset_commit_atomic+0x218/0x280 [ 20.550957] drm_client_modeset_commit_locked+0x64/0x1a0 [ 20.550961] drm_client_modeset_commit+0x38/0x68 [ 20.550965] __drm_fb_helper_restore_fbdev_mode_unlocked+0xb0/0xf8 [ 20.550970] drm_fb_helper_set_par+0x44/0x88 [ 20.550973] fbcon_init+0x1e0/0x4a8 [ 20.550976] visual_init+0xbc/0x118 [ 20.550981] do_bind_con_driver.isra.0+0x194/0x3a0 [ 20.550984] do_take_over_console+0x50/0x70 [ 20.550987] do_fbcon_takeover+0x74/0xf8 [ 20.550989] do_fb_registered+0x13c/0x158 [ 20.550992] fbcon_register_existing_fbs+0x78/0xc0 [ 20.550995] process_one_work+0x1ec/0x478 [ 20.551000] worker_thread+0x74/0x418 [ 20.551002] kthread+0xec/0x100 [ 20.551005] ret_from_fork+0x10/0x20 [ 20.551011] Code: f944 b9409013 f940a082 9ba30a73 (b9407662) [ 20.551013] ---[ end trace ]--- If there is any additional information that I can provide or patches I can test, I am more than happy to do so. Cheers, Nathan # bad: [4b0f4525dc4fe8af17b3daefe585f0c2eb0fe0a5] Add linux-next specific files for
Re: Linux 6.3-rc3
On Fri, Mar 24, 2023 at 05:23:12PM +0200, Kalle Valo wrote: > Nathan Chancellor writes: > > >> This is nitpicking but it would be nice if the tarball contents wouldn't > >> conflict with each other. Now both llvm-16.0.0-aarch64.tar.gz and > >> llvm-16.0.0-x86_64.tar extract to the same directory llvm-16.0.0 with > >> same binary names. It would be much better if they would extract to > >> llvm-16.0.0-aarch64 and llvm-16.0.0-x86_64, respectively. > >> > >> For example, Arnd's crosstool packages don't conflict with each other: > >> > >> https://mirrors.edge.kernel.org/pub/tools/crosstool/ > > > > I could certainly do that but what is the use case for extracting both? > > You cannot run the aarch64 version on an x86_64 host and vice versa, so > > why bother extracting them? > > Ah, I didn't realise that. I assumed llvm-16.0.0-aarch64.tar.gz was a > cross compiler. I'm sure you documented that in the page but hey who > reads the documentation ;) :) I have adjusted the README to hopefully make that clearer. > > I had figured the architecture would be irrelevant once installed on > > the host, so I opted only to include it in the tarball name. Perhaps I > > should make it clearer that these are the host architectures, not the > > target architectures (because clang is multi-targeted, unlike GCC)? > > Makes sense now. But I still think it's good style that a tarball named > llvm-16.0.0-aarch64.tar.gz extracts to llvm-16.0.0-aarch64. Indeed, I have adjusted it for future builds: https://github.com/nathanchance/env/commit/314837e6706889138121a32140d2acdc7895d390 > >> And maybe request a similar llvm directory under pub/tools to make it > >> more official? :) We now have https://kernel.org/pub/tools/llvm/, which is about as official as we can get I suppose :) https://kernel.org/pub/linux/kernel/people/nathan/llvm/ now points people there. Cheers, Nathan
Re: [PATCH v8 2/2] drm: add kms driver for loongson display controller
On Tue, Mar 28, 2023 at 11:22:50PM +0800, Sui Jingfeng wrote: > HI, > > On 2023/3/28 17:27, kernel test robot wrote: > > Hi Sui, > > > > Thank you for the patch! Perhaps something to improve: > > > > [auto build test WARNING on drm-misc/drm-misc-next] > > [also build test WARNING on linus/master v6.3-rc4 next-20230328] > > [If your patch is applied to the wrong git tree, kindly drop us a note. > > And when submitting patch, we suggest to use '--base' as documented in > > https://git-scm.com/docs/git-format-patch#_base_tree_information] > > > > url: > > https://github.com/intel-lab-lkp/linux/commits/Sui-Jingfeng/MAINTAINERS-add-maintainers-for-DRM-LOONGSON-driver/20230320-180408 > > base: git://anongit.freedesktop.org/drm/drm-misc drm-misc-next > > patch link: > > https://lore.kernel.org/r/20230320100131.1277034-3-15330273260%40189.cn > > patch subject: [PATCH v8 2/2] drm: add kms driver for loongson display > > controller > > config: i386-allyesconfig > > (https://download.01.org/0day-ci/archive/20230328/202303281754.jwi20j2c-...@intel.com/config) > > compiler: clang version 14.0.6 (https://github.com/llvm/llvm-project > > f28c006a5895fc0e329fe15fead81e37457cb1d1) > > reproduce (this is a W=1 build): > > wget > > https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O > > ~/bin/make.cross > > chmod +x ~/bin/make.cross > > # > > https://github.com/intel-lab-lkp/linux/commit/80b4115f44993f4ebf47b1cb9e8f02953575b977 > > git remote add linux-review https://github.com/intel-lab-lkp/linux > > git fetch --no-tags linux-review > > Sui-Jingfeng/MAINTAINERS-add-maintainers-for-DRM-LOONGSON-driver/20230320-180408 > > git checkout 80b4115f44993f4ebf47b1cb9e8f02953575b977 > > # save the config file > > mkdir build_dir && cp config build_dir/.config > > COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 > > O=build_dir ARCH=i386 olddefconfig > > COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 > > O=build_dir ARCH=i386 SHELL=/bin/bash drivers/accel/ > > drivers/gpu/drm/loongson/ drivers/iio/light/ drivers/media/pci/intel/ > > > > If you fix the issue, kindly add following tag where applicable > > | Reported-by: kernel test robot > > | Link: > > https://lore.kernel.org/oe-kbuild-all/202303281754.jwi20j2c-...@intel.com/ > > > > All warnings (new ones prefixed by >>): > > > > > > drivers/gpu/drm/loongson/lsdc_drv.c:232:11: warning: variable 'gpu' is > > > > used uninitialized whenever 'if' condition is false > > > > [-Wsometimes-uninitialized] > > else if (descp->chip == CHIP_LS7A2000) > > ^~~~ > > drivers/gpu/drm/loongson/lsdc_drv.c:235:7: note: uninitialized use > > occurs here > > if (!gpu) { > > ^~~ > > drivers/gpu/drm/loongson/lsdc_drv.c:232:7: note: remove the 'if' if its > > condition is always true > > else if (descp->chip == CHIP_LS7A2000) > > ^ > > drivers/gpu/drm/loongson/lsdc_drv.c:217:21: note: initialize the > > variable 'gpu' to silence this warning > > struct pci_dev *gpu; > >^ > > = NULL > > 1 warning generated. > > -- > > In practice, either descp->chip == CHIP_LS7A2000 or descp->chip == > CHIP_LS7A1000 will be happened at runtime. > > the variable 'gpu' is guaranteed to be initialized when code run at > drivers/gpu/drm/loongson/lsdc_drv.c:235 > > This warnning is almost wrong here. Clang's semantic analysis happens before optimizations, meaning it does not perform interprocedural analysis, so it does not have enough information at this point to tell that. Either just initialize gpu to NULL and let the existing 'if (!gpu)' handle it or add a separate else branch that warns about an unhandled chip value so that it is obvious what needs to be done if someone forgets to update this statement when a new chip is supported by this driver. > > > > drivers/gpu/drm/loongson/lsdc_pll.c:188:14: warning: variable 'diff' is > > > > used uninitialized whenever 'if' condition is false > > > > [-Wsometimes-uninitialized] > > else if (clock_khz < computed) > > ^~~~ > > drivers/gpu/drm/loongson/lsdc_pll.c:191:9: note: uninitialized use > > occurs here > > if (diff < min) { > > ^~~~ > > drivers/gpu/drm/loongson/lsdc_pll.c:188:10: note: remove the 'if' if > > its condition is always true > > else if (clock_khz < computed) > > ^ > > drivers/gpu/drm/loongson/lsdc_pll.c:177:22: note: initialize the > > variable 'diff' to silence this
Re: Linux 6.3-rc3
On Fri, Mar 24, 2023 at 12:54:01PM +0200, Kalle Valo wrote: > Nathan Chancellor writes: > > > On Wed, Mar 22, 2023 at 02:44:47PM +0200, Kalle Valo wrote: > >> Nathan Chancellor writes: > >> > >> > Perhaps these would make doing allmodconfig builds with clang more > >> > frequently less painful for you? > >> > > >> > https://lore.kernel.org/llvm/20230319235619.GA18547@dev-arch.thelio-3990X/ > >> > >> Thank you, at least for me this is really helpful. > > > > Really glad to hear! I hope this helps make testing and verifying > > changes with clang and LLVM easier for developers and maintainers. > > It really does. And I hope you are able to update these packages in > future as well so that it would be easy to get the latest clang. That is the current plan (I will push 16.0.1, 16.0.2, etc. as they are released), I have a relatively automated process for this going forward. > >> I tried now clang for the first time but seeing a strange problem. > >> > >> I prefer to define the compiler in GNUmakefile so it's easy to change > >> compilers and I don't need to remember the exact command line. So I have > >> this in the top level GNUmakefile (all the rest commented out): > >> > >> LLVM=/opt/clang/llvm-16.0.0/bin/ > >> > >> If I run 'make oldconfig' it seems to use clang but after I run just > >> 'make' it seems to switch back to the host GCC compiler and ask for GCC > >> specific config questions again. Workaround for this seems to be adding > >> 'export LLVM' to GNUmakefile, after that also 'make' uses clang as > >> expected. > > > > Interesting... I just tested with a basic GNUmakefile and everything > > seems to work fine without an export. At the same time, the export > > should not hurt anything, so as long as it works, that is what matters. > > Sure, once I figured out the quirks I can workaround them. I was just > hoping that other users would not have to go through the same hassle as > I did :) > > > If you have any further issues, please do not hesitate to reach out! > > This is nitpicking but it would be nice if the tarball contents wouldn't > conflict with each other. Now both llvm-16.0.0-aarch64.tar.gz and > llvm-16.0.0-x86_64.tar extract to the same directory llvm-16.0.0 with > same binary names. It would be much better if they would extract to > llvm-16.0.0-aarch64 and llvm-16.0.0-x86_64, respectively. > > For example, Arnd's crosstool packages don't conflict with each other: > > https://mirrors.edge.kernel.org/pub/tools/crosstool/ I could certainly do that but what is the use case for extracting both? You cannot run the aarch64 version on an x86_64 host and vice versa, so why bother extracting them? I had figured the architecture would be irrelevant once installed on the host, so I opted only to include it in the tarball name. Perhaps I should make it clearer that these are the host architectures, not the target architectures (because clang is multi-targeted, unlike GCC)? > And maybe request a similar llvm directory under pub/tools to make it > more official? :) Yes, I was talking that over with Nick recently, as having it under a group on kernel.org would make taking over maintainership easier should something happen to me :) Thanks for all the feedback so far, it is much appreciated! Cheers, Nathan
Re: Linux 6.3-rc3
On Wed, Mar 22, 2023 at 09:36:37AM -0700, Nathan Chancellor wrote: > On Wed, Mar 22, 2023 at 02:44:47PM +0200, Kalle Valo wrote: > > Nathan Chancellor writes: > > > > > On Mon, Mar 20, 2023 at 11:26:17AM -0700, Linus Torvalds wrote: > > >> On Mon, Mar 20, 2023 at 11:05 AM Nathan Chancellor > > >> wrote: > > >> > > > >> > On the clang front, I am still seeing the following warning turned > > >> > error > > >> > for arm64 allmodconfig at least: > > >> > > > >> > drivers/gpu/host1x/dev.c:520:6: error: variable 'syncpt_irq' is > > >> > uninitialized when used here [-Werror,-Wuninitialized] > > >> > if (syncpt_irq < 0) > > >> > ^~ > > >> > > >> Hmm. I do my arm64 allmodconfig builds with gcc, and I'm surprised > > >> that gcc doesn't warn about this. > > > > > > Perhaps these would make doing allmodconfig builds with clang more > > > frequently less painful for you? > > > > > > https://lore.kernel.org/llvm/20230319235619.GA18547@dev-arch.thelio-3990X/ > > > > Thank you, at least for me this is really helpful. > > Really glad to hear! I hope this helps make testing and verifying > changes with clang and LLVM easier for developers and maintainers. > > > I tried now clang for the first time but seeing a strange problem. > > > > I prefer to define the compiler in GNUmakefile so it's easy to change > > compilers and I don't need to remember the exact command line. So I have > > this in the top level GNUmakefile (all the rest commented out): > > > > LLVM=/opt/clang/llvm-16.0.0/bin/ > > > > If I run 'make oldconfig' it seems to use clang but after I run just > > 'make' it seems to switch back to the host GCC compiler and ask for GCC > > specific config questions again. Workaround for this seems to be adding > > 'export LLVM' to GNUmakefile, after that also 'make' uses clang as > > expected. > > Interesting... I just tested with a basic GNUmakefile and everything > seems to work fine without an export. At the same time, the export > should not hurt anything, so as long as it works, that is what matters. Ah, the export is needed so that mixed-build works properly (see lines 324 to 361 in Makefile), as 'make' will be called to process each target individually; without the export, LLVM is not set for the subsequent 'make' calls, so gcc is called. I just saw the same behavior as you did while testing with $ make -j(nproc) clean defconfig all without the export (GCC was used instead of LLVM). > $ gcc --version > fish: Unknown command: gcc > > > $ fd -t x . $CBL_TC_LLVM_STORE/16.0.0/bin -x basename > clang-16 > llvm-nm > llvm-objdump > llvm-objcopy > llvm-symbolizer > llvm-strings > llvm-readobj > llvm-dwarfdump > lld > llvm-ar > > > $ cat GNUmakefile > LLVM := $(CBL_TC_LLVM_STORE)/16.0.0/bin/ > > include Makefile > > > $ make -sj(nproc) defconfig > > > $ head -13 .config > # > # Automatically generated file; DO NOT EDIT. > # Linux/x86 6.3.0-rc3 Kernel Configuration > # > CONFIG_CC_VERSION_TEXT="ClangBuiltLinux clang version 16.0.0" > CONFIG_GCC_VERSION=0 > CONFIG_CC_IS_CLANG=y > CONFIG_CLANG_VERSION=16 > CONFIG_AS_IS_LLVM=y > CONFIG_AS_VERSION=16 > CONFIG_LD_VERSION=0 > CONFIG_LD_IS_LLD=y > CONFIG_LLD_VERSION=16 > > > $ make -sj(nproc) init/main.o > > > $ $CBL_TC_LLVM_STORE/16.0.0/bin/llvm-readelf -p .comment init/main.o > String dump of section '.comment': > [ 1] ClangBuiltLinux clang version 16.0.0 > > > I added an informational print and I always saw the correct value: > > diff --git a/Makefile b/Makefile > index a2c310df2145..070394c4cb8c 100644 > --- a/Makefile > +++ b/Makefile > @@ -431,6 +431,7 @@ HOST_LFS_LDFLAGS := $(shell getconf LFS_LDFLAGS > 2>/dev/null) > HOST_LFS_LIBS := $(shell getconf LFS_LIBS 2>/dev/null) > > ifneq ($(LLVM),) > +$(info LLVM: $(LLVM)) > ifneq ($(filter %/,$(LLVM)),) > LLVM_PREFIX := $(LLVM) > else ifneq ($(filter -%,$(LLVM)),) > > If you have any further issues, please do not hesitate to reach out! > > Cheers, > Nathan >
Re: Linux 6.3-rc3
On Wed, Mar 22, 2023 at 02:44:47PM +0200, Kalle Valo wrote: > Nathan Chancellor writes: > > > On Mon, Mar 20, 2023 at 11:26:17AM -0700, Linus Torvalds wrote: > >> On Mon, Mar 20, 2023 at 11:05 AM Nathan Chancellor > >> wrote: > >> > > >> > On the clang front, I am still seeing the following warning turned error > >> > for arm64 allmodconfig at least: > >> > > >> > drivers/gpu/host1x/dev.c:520:6: error: variable 'syncpt_irq' is > >> > uninitialized when used here [-Werror,-Wuninitialized] > >> > if (syncpt_irq < 0) > >> > ^~ > >> > >> Hmm. I do my arm64 allmodconfig builds with gcc, and I'm surprised > >> that gcc doesn't warn about this. > > > > Perhaps these would make doing allmodconfig builds with clang more > > frequently less painful for you? > > > > https://lore.kernel.org/llvm/20230319235619.GA18547@dev-arch.thelio-3990X/ > > Thank you, at least for me this is really helpful. Really glad to hear! I hope this helps make testing and verifying changes with clang and LLVM easier for developers and maintainers. > I tried now clang for the first time but seeing a strange problem. > > I prefer to define the compiler in GNUmakefile so it's easy to change > compilers and I don't need to remember the exact command line. So I have > this in the top level GNUmakefile (all the rest commented out): > > LLVM=/opt/clang/llvm-16.0.0/bin/ > > If I run 'make oldconfig' it seems to use clang but after I run just > 'make' it seems to switch back to the host GCC compiler and ask for GCC > specific config questions again. Workaround for this seems to be adding > 'export LLVM' to GNUmakefile, after that also 'make' uses clang as > expected. Interesting... I just tested with a basic GNUmakefile and everything seems to work fine without an export. At the same time, the export should not hurt anything, so as long as it works, that is what matters. $ gcc --version fish: Unknown command: gcc $ fd -t x . $CBL_TC_LLVM_STORE/16.0.0/bin -x basename clang-16 llvm-nm llvm-objdump llvm-objcopy llvm-symbolizer llvm-strings llvm-readobj llvm-dwarfdump lld llvm-ar $ cat GNUmakefile LLVM := $(CBL_TC_LLVM_STORE)/16.0.0/bin/ include Makefile $ make -sj(nproc) defconfig $ head -13 .config # # Automatically generated file; DO NOT EDIT. # Linux/x86 6.3.0-rc3 Kernel Configuration # CONFIG_CC_VERSION_TEXT="ClangBuiltLinux clang version 16.0.0" CONFIG_GCC_VERSION=0 CONFIG_CC_IS_CLANG=y CONFIG_CLANG_VERSION=16 CONFIG_AS_IS_LLVM=y CONFIG_AS_VERSION=16 CONFIG_LD_VERSION=0 CONFIG_LD_IS_LLD=y CONFIG_LLD_VERSION=16 $ make -sj(nproc) init/main.o $ $CBL_TC_LLVM_STORE/16.0.0/bin/llvm-readelf -p .comment init/main.o String dump of section '.comment': [ 1] ClangBuiltLinux clang version 16.0.0 I added an informational print and I always saw the correct value: diff --git a/Makefile b/Makefile index a2c310df2145..070394c4cb8c 100644 --- a/Makefile +++ b/Makefile @@ -431,6 +431,7 @@ HOST_LFS_LDFLAGS := $(shell getconf LFS_LDFLAGS 2>/dev/null) HOST_LFS_LIBS := $(shell getconf LFS_LIBS 2>/dev/null) ifneq ($(LLVM),) +$(info LLVM: $(LLVM)) ifneq ($(filter %/,$(LLVM)),) LLVM_PREFIX := $(LLVM) else ifneq ($(filter -%,$(LLVM)),) If you have any further issues, please do not hesitate to reach out! Cheers, Nathan
Re: Linux 6.3-rc3
On Mon, Mar 20, 2023 at 01:30:17PM -0700, Linus Torvalds wrote: > On Mon, Mar 20, 2023 at 1:05 PM Guenter Roeck wrote: > > > > I have noticed that gcc doesn't always warn about uninitialized variables > > in most architectures. > > Yeah, I'm getting the feeling that when the gcc people were trying to > make -Wmaybe-uninitialized work better (when moving it into "-Wall"), > they ended up moving a lot of "clearly uninitialized" cases into it. > > So then because we disable the "maybe" case (with > -Wno-maybe-uninitialized) because it had too many random false > positives, we end up not seeing the obvious cases either. Right, this seems like a subtle difference in semantics between -Wuninitialized between clang and GCC. My naive attempt to reduce the problem with cvise spits out: $ cat dev.i void *host1x_probe___trans_tmp_1; void host1x_unregister(); int host1x_probe_syncpt_irqhost1x_probe() { int err; if (host1x_probe___trans_tmp_1) return 2; if (err) host1x_unregister(); return err; } $ gcc -O2 -Wall -c -o /dev/null dev.i dev.i: In function ‘host1x_probe_syncpt_irqhost1x_probe’: dev.i:7:6: warning: ‘err’ may be used uninitialized [-Wmaybe-uninitialized] 7 | if (err) | ^ dev.i:4:7: note: ‘err’ was declared here 4 | int err; | ^~~ $ clang -Wall -fsyntax-only dev.i dev.i:7:7: warning: variable 'err' is uninitialized when used here [-Wuninitialized] if (err) ^~~ dev.i:4:10: note: initialize the variable 'err' to silence this warning int err; ^ = 0 1 warning generated. If I remove the first branch, both compilers show -Wuninitialized. $ cat dev.i void *host1x_probe___trans_tmp_1; void host1x_unregister(); int host1x_probe_syncpt_irqhost1x_probe() { int err; if (err) host1x_unregister(); return err; } $ gcc -O2 -Wall -c -o /dev/null dev.i dev.i: In function ‘host1x_probe_syncpt_irqhost1x_probe’: dev.i:5:6: warning: ‘err’ is used uninitialized [-Wuninitialized] 5 | if (err) | ^ dev.i:4:7: note: ‘err’ was declared here 4 | int err; | ^~~ $ clang -Wall -fsyntax-only dev.i dev.i:5:7: warning: variable 'err' is uninitialized when used here [-Wuninitialized] if (err) ^~~ dev.i:4:10: note: initialize the variable 'err' to silence this warning int err; ^ = 0 1 warning generated. It seems like clang takes into account that the branch has no effect on how uninitialized err is, although it does acknowledge there may be control flow where err is not used uninitialized because it is not used at all by stating "when used here". I guess GCC does not make this distinction and places it under -Wmaybe-uninitialized. I could be totally wrong though :) Cheers, Nathan
Re: Linux 6.3-rc3
On Mon, Mar 20, 2023 at 11:53:37AM -0700, Nathan Chancellor wrote: > On Mon, Mar 20, 2023 at 11:26:17AM -0700, Linus Torvalds wrote: > > On Mon, Mar 20, 2023 at 11:05 AM Nathan Chancellor > > wrote: > > > > > > On the clang front, I am still seeing the following warning turned error > > > for arm64 allmodconfig at least: > > > > > > drivers/gpu/host1x/dev.c:520:6: error: variable 'syncpt_irq' is > > > uninitialized when used here [-Werror,-Wuninitialized] > > > if (syncpt_irq < 0) > > > ^~ > > > > Hmm. I do my arm64 allmodconfig builds with gcc, and I'm surprised > > that gcc doesn't warn about this. > > Perhaps these would make doing allmodconfig builds with clang more > frequently less painful for you? > > https://lore.kernel.org/llvm/20230319235619.GA18547@dev-arch.thelio-3990X/ > > > That syncpt_irq thing isn't written to anywhere, so that's pretty egregious. > > > > We use -Wno-maybe-uninitialized because gcc gets it so wrong, but > > that's different from the "-Wuninitialized" thing (without the > > "maybe"). > > > > I've seen gcc mess this up when there is one single assignment, > > because then the SSA format makes it *so* easy to just use that > > assignment out-of-order (or unconditionally), but this case looks > > unusually clear-cut. > > > > So the fact that gcc doesn't warn about it is outright odd. > > > > > If that does not come to you through other means before -rc4, could you > > > just apply it directly so that I can stop applying it to our CI? :) > > > > Bah. I took it now, there's no excuse for that thing. > > Thanks! > > > Do we have any gcc people around that could explain why gcc failed so > > miserably at this trivial case? > > Cc'ing linux-toolchains. The start of the thread is here: > > https://lore.kernel.org/CAHk-=wgsqpdkejbb92m37jntdrqjrnruaprahke8ughtqqu...@mail.gmail.com/ > > The problematic function before the fix is here: > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/gpu/host1x/dev.c?id=3d3699bde4b043eea17993e4e76804a8128f0fdb#n487 > > I will see if I have some cycles to try and reduce something out for the > GCC folks. While setting up the reduction, I noticed that there is an instance of -Wmaybe-uninitialized at this site. Seems odd that it is not sure, I will reduce on that. ../drivers/gpu/host1x/dev.c: In function 'host1x_probe': ../drivers/gpu/host1x/dev.c:520:12: error: 'syncpt_irq' may be used uninitialized [-Werror=maybe-uninitialized] 520 | if (syncpt_irq < 0) |^ ../drivers/gpu/host1x/dev.c:490:13: note: 'syncpt_irq' was declared here 490 | int syncpt_irq; | ^~ cc1: all warnings being treated as errors Cheers, Nathan
Re: Linux 6.3-rc3
On Mon, Mar 20, 2023 at 11:49:55AM -0700, Linus Torvalds wrote: > On Mon, Mar 20, 2023 at 11:26 AM Linus Torvalds > wrote: > > > > Hmm. I do my arm64 allmodconfig builds with gcc, and I'm surprised > > that gcc doesn't warn about this. > > Side note: I'm also wondering why that TEGRA_HOST1X config has that > ARM dependency in > > depends on ARCH_TEGRA || (ARM && COMPILE_TEST) > > because it seems to build just fine at least on x86-64 if I change it to be > just > > depends on ARCH_TEGRA || COMPILE_TEST > > ie there seems to be nothing ARM-specific in there. > > Limiting it to just the tegra platform by default makes 100% sense, > but that "only do compile-testing on ARM" seems a bit bogus. > > That limit goes back to forever (commit 6f44c2b5280f: "gpu: host1x: > Increase compile test coverage" back in Nov 2013), so maybe things > didn't use to work as well back in the dark ages? > > None of this explains why gcc didn't catch it, but at least allowing > the build on x86-64 would likely have made it easier for people to see > clang catching this. I did see a patch fly by to fix that: https://lore.kernel.org/20230316082035.567520-3-christian.koe...@amd.com/ It seems like the DRM_TEGRA half of it is broken though: https://lore.kernel.org/202303170635.a2rsq1wu-...@intel.com/ Cheers, Nathan
Re: Linux 6.3-rc3
On Mon, Mar 20, 2023 at 11:26:17AM -0700, Linus Torvalds wrote: > On Mon, Mar 20, 2023 at 11:05 AM Nathan Chancellor wrote: > > > > On the clang front, I am still seeing the following warning turned error > > for arm64 allmodconfig at least: > > > > drivers/gpu/host1x/dev.c:520:6: error: variable 'syncpt_irq' is > > uninitialized when used here [-Werror,-Wuninitialized] > > if (syncpt_irq < 0) > > ^~ > > Hmm. I do my arm64 allmodconfig builds with gcc, and I'm surprised > that gcc doesn't warn about this. Perhaps these would make doing allmodconfig builds with clang more frequently less painful for you? https://lore.kernel.org/llvm/20230319235619.GA18547@dev-arch.thelio-3990X/ > That syncpt_irq thing isn't written to anywhere, so that's pretty egregious. > > We use -Wno-maybe-uninitialized because gcc gets it so wrong, but > that's different from the "-Wuninitialized" thing (without the > "maybe"). > > I've seen gcc mess this up when there is one single assignment, > because then the SSA format makes it *so* easy to just use that > assignment out-of-order (or unconditionally), but this case looks > unusually clear-cut. > > So the fact that gcc doesn't warn about it is outright odd. > > > If that does not come to you through other means before -rc4, could you > > just apply it directly so that I can stop applying it to our CI? :) > > Bah. I took it now, there's no excuse for that thing. Thanks! > Do we have any gcc people around that could explain why gcc failed so > miserably at this trivial case? Cc'ing linux-toolchains. The start of the thread is here: https://lore.kernel.org/CAHk-=wgsqpdkejbb92m37jntdrqjrnruaprahke8ughtqqu...@mail.gmail.com/ The problematic function before the fix is here: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/gpu/host1x/dev.c?id=3d3699bde4b043eea17993e4e76804a8128f0fdb#n487 I will see if I have some cycles to try and reduce something out for the GCC folks. Cheers, Nathan