diff mbox series

drm/amd/display: Increase frame warning limit for clang in dml2

Message ID 20231102-amdgpu-dml2-increase-frame-size-warning-for-clang-v1-1-6eb157352931@kernel.org (mailing list archive)
State New, archived
Headers show
Series drm/amd/display: Increase frame warning limit for clang in dml2 | expand

Commit Message

Nathan Chancellor Nov. 2, 2023, 4:24 p.m. UTC
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 <nathan@kernel.org>
---
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(-)


---
base-commit: 21e80f3841c01aeaf32d7aee7bbc87b3db1aa0c6
change-id: 20231102-amdgpu-dml2-increase-frame-size-warning-for-clang-c93bd2d6a871

Best regards,

Comments

Hamza Mahfooz Nov. 2, 2023, 4:59 p.m. UTC | #1
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 <nathan@kernel.org>
> ---
> 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.

>   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 Nov. 2, 2023, 5:12 p.m. UTC | #2
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 <nathan@kernel.org>
> > ---
> > 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
>
Alex Deucher Nov. 2, 2023, 5:21 p.m. UTC | #3
On Thu, Nov 2, 2023 at 1:12 PM Nathan Chancellor <nathan@kernel.org> wrote:
>
> 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 <nathan@kernel.org>
> > > ---
> > > 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?

I think replacing the clang check entirely.  A similar issue was just
reported on different GCC versions:
https://lists.freedesktop.org/archives/amd-gfx/2023-November/100725.html

Alex

>
> 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
> >
Hamza Mahfooz Nov. 2, 2023, 5:22 p.m. UTC | #4
On 11/2/23 13:12, Nathan Chancellor wrote:
> 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 <nathan@kernel.org>
>>> ---
>>> 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?

I think for the time being replacing the clang check with a KASAN/KCSAN
check would make more sense. Considering that, the allmodconfig for older
versions of gcc is also broken (see [1]).

1. 
https://lore.kernel.org/amd-gfx/CADVatmO9NCs=ryNg72HNzMDpqg862gpGnnFhQ4uwTpEkjOkCLw@mail.gmail.com/

> 
> 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
>>
diff mbox series

Patch

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)