Re: [PATCH] drm/amdgpu: Mark mmhub_v1_8_mmea_err_status_reg as __maybe_unused

[Nick Desaulniers]

On Thu, May 25, 2023 at 9:42 AM Alex Deucher  wrote:
>
> On Thu, May 25, 2023 at 12:29 PM Nathan Chancellor  wrote:
> >
> > On Thu, May 25, 2023 at 12:26:56PM -0400, Luben Tuikov wrote:
> > > On 2023-05-25 11:22, Nathan Chancellor wrote:
> > > > On Fri, May 19, 2023 at 06:14:38PM +0530, Srinivasan Shanmugam wrote:
> > > >> Silencing the compiler from below compilation error:
> > > >>
> > > >> drivers/gpu/drm/amd/amdgpu/mmhub_v1_8.c:704:23: error: variable 
> > > >> 'mmhub_v1_8_mmea_err_status_reg' is not needed and will not be emitted 
> > > >> [-Werror,-Wunneeded-internal-declaration]
> > > >> static const uint32_t mmhub_v1_8_mmea_err_status_reg[] = {
> > > >>   ^
> > > >> 1 error generated.
> > > >>
> > > >> Mark the variable as __maybe_unused to make it clear to clang that this
> > > >> is expected, so there is no more warning.
> > > >>
> > > >> Cc: Christian König 
> > > >> Cc: Lijo Lazar 
> > > >> Cc: Luben Tuikov 
> > > >> Cc: Alex Deucher 
> > > >> Signed-off-by: Srinivasan Shanmugam 
> > > >
> > > > Traditionally, this attribute would go between the [] and =, but that is
> > > > a nit. Can someone please pick this up to unblock our builds on -next?
> > > >
> > > > Reviewed-by: Nathan Chancellor 
> > >
> > > I'll pick this up, fix it, and submit to amd-staging-drm-next.
> >
> > Thanks a lot :)
> >
> > > Which -next are you referring to, Nathan?
> >
> > linux-next, this warning breaks the build when -Werror is enabled, such
> > as with allmodconfig:
> >
> > https://storage.tuxsuite.com/public/clangbuiltlinux/continuous-integration2/builds/2QHtlCTz2JL0yXNpRB5hVmiP9lq/build.log
> >
>
> Srinivasan has already pushed it.  I'll push it out once CI has
> completed.  We are trying to figure out the best way to enable -WERROR
> in our CI system as it is almost always broken depending on what
> compiler you are using.  Also, I'm not sure fixing these is always
> better.  A lot of these warnings seem spurious and in a lot of cases
> the "fix" doesn't really improve the code, it just silences a warning.
> As one of my coworkers put it, there is a reason warnings are not
> errors.

https://www.theregister.com/2021/09/08/compromise_linux_kernel_compiler_warnings/

>
> Alex
>
>
> > Cheers,
> > Nathan
> >
> > > >> ---
> > > >>  drivers/gpu/drm/amd/amdgpu/mmhub_v1_8.c | 1 +
> > > >>  1 file changed, 1 insertion(+)
> > > >>
> > > >> diff --git a/drivers/gpu/drm/amd/amdgpu/mmhub_v1_8.c 
> > > >> b/drivers/gpu/drm/amd/amdgpu/mmhub_v1_8.c
> > > >> index 3648994724c2..cba087e529c0 100644
> > > >> --- a/drivers/gpu/drm/amd/amdgpu/mmhub_v1_8.c
> > > >> +++ b/drivers/gpu/drm/amd/amdgpu/mmhub_v1_8.c
> > > >> @@ -701,6 +701,7 @@ static void 
> > > >> mmhub_v1_8_reset_ras_error_count(struct amdgpu_device *adev)
> > > >>mmhub_v1_8_inst_reset_ras_error_count(adev, i);
> > > >>  }
> > > >>
> > > >> +__maybe_unused
> > > >>  static const uint32_t mmhub_v1_8_mmea_err_status_reg[] = {
> > > >>regMMEA0_ERR_STATUS,
> > > >>regMMEA1_ERR_STATUS,
> > > >> --
> > > >> 2.25.1
> > > >>
> > >
>


-- 
Thanks,
~Nick Desaulniers

Re: [PATCH] drm/amd/pm: remove unused num_of_active_display variable

[Nick Desaulniers]

On Fri, Mar 31, 2023 at 9:40 AM Tom Rix  wrote:
>
> clang with W=1 reports
> drivers/gpu/drm/amd/amdgpu/../pm/swsmu/amdgpu_smu.c:1700:6: error: variable
>   'num_of_active_display' set but not used [-Werror,-Wunused-but-set-variable]
> int num_of_active_display = 0;
> ^
> This variable is not used so remove it.
>
> Signed-off-by: Tom Rix 

Thanks for the patch!
Fixes: commit 75145aab7a0d ("drm/amdgpu/swsmu: clean up a bunch of
stale interfaces")
Reviewed-by: Nick Desaulniers 

> ---
>  drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c | 7 ---
>  1 file changed, 7 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c 
> b/drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c
> index b5d64749990e..f93f7a9ed631 100644
> --- a/drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c
> +++ b/drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c
> @@ -1696,8 +1696,6 @@ static int smu_display_configuration_change(void 
> *handle,
> const struct 
> amd_pp_display_configuration *display_config)
>  {
> struct smu_context *smu = handle;
> -   int index = 0;
> -   int num_of_active_display = 0;
>
> if (!smu->pm_enabled || !smu->adev->pm.dpm_enabled)
> return -EOPNOTSUPP;
> @@ -1708,11 +1706,6 @@ static int smu_display_configuration_change(void 
> *handle,
> smu_set_min_dcef_deep_sleep(smu,
> 
> display_config->min_dcef_deep_sleep_set_clk / 100);
>
> -   for (index = 0; index < 
> display_config->num_path_including_non_display; index++) {
> -   if (display_config->displays[index].controller_id != 0)
> -   num_of_active_display++;
> -   }
> -
> return 0;
>  }
>
> --
> 2.27.0
>


-- 
Thanks,
~Nick Desaulniers

Re: [PATCH] drm/amd/display: remove unused matching_stream_ptrs variable

[Nick Desaulniers]

Jimmy, can you review?

The change LGTM; but I'm not sure if there was something else intended here.

On Sat, Mar 25, 2023 at 6:45 AM Tom Rix  wrote:
>
> clang with W=1 reports
> drivers/gpu/drm/amd/amdgpu/../display/dc/core/dc_link_enc_cfg.c:625:6: error:
>   variable 'matching_stream_ptrs' set but not used 
> [-Werror,-Wunused-but-set-variable]
> int matching_stream_ptrs = 0;
> ^
> This variable is not used so remove it.
>
> Signed-off-by: Tom Rix 
> ---
>  drivers/gpu/drm/amd/display/dc/core/dc_link_enc_cfg.c | 5 +
>  1 file changed, 1 insertion(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/display/dc/core/dc_link_enc_cfg.c 
> b/drivers/gpu/drm/amd/display/dc/core/dc_link_enc_cfg.c
> index 41198c729d90..30c0644d4418 100644
> --- a/drivers/gpu/drm/amd/display/dc/core/dc_link_enc_cfg.c
> +++ b/drivers/gpu/drm/amd/display/dc/core/dc_link_enc_cfg.c
> @@ -622,7 +622,6 @@ bool link_enc_cfg_validate(struct dc *dc, struct dc_state 
> *state)
> int i, j;
> uint8_t valid_count = 0;
> uint8_t dig_stream_count = 0;
> -   int matching_stream_ptrs = 0;
> int eng_ids_per_ep_id[MAX_PIPES] = {0};
> int ep_ids_per_eng_id[MAX_PIPES] = {0};
> int valid_bitmap = 0;
> @@ -645,9 +644,7 @@ bool link_enc_cfg_validate(struct dc *dc, struct dc_state 
> *state)
> struct link_enc_assignment assignment = 
> state->res_ctx.link_enc_cfg_ctx.link_enc_assignments[i];
>
> if (assignment.valid) {
> -   if (assignment.stream == state->streams[i])
> -   matching_stream_ptrs++;
> -   else
> +   if (assignment.stream != state->streams[i])
>     valid_stream_ptrs = false;
> }
> }
> --
> 2.27.0
>


-- 
Thanks,
~Nick Desaulniers

Re: [PATCH] drm/amd/display: remove unused matching_stream_ptrs variable

[Nick Desaulniers]

On Fri, Apr 7, 2023 at 10:52 AM Nick Desaulniers
 wrote:
>
> Jimmy, can you review?
>
> The change LGTM; but I'm not sure if there was something else intended here.

Nevermind, Jimmy's email address bounced.
Reviewed-by: Nick Desaulniers 

>
> On Sat, Mar 25, 2023 at 6:45 AM Tom Rix  wrote:
> >
> > clang with W=1 reports
> > drivers/gpu/drm/amd/amdgpu/../display/dc/core/dc_link_enc_cfg.c:625:6: 
> > error:
> >   variable 'matching_stream_ptrs' set but not used 
> > [-Werror,-Wunused-but-set-variable]
> > int matching_stream_ptrs = 0;
> > ^
> > This variable is not used so remove it.
> >
> > Signed-off-by: Tom Rix 
> > ---
> >  drivers/gpu/drm/amd/display/dc/core/dc_link_enc_cfg.c | 5 +
> >  1 file changed, 1 insertion(+), 4 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/amd/display/dc/core/dc_link_enc_cfg.c 
> > b/drivers/gpu/drm/amd/display/dc/core/dc_link_enc_cfg.c
> > index 41198c729d90..30c0644d4418 100644
> > --- a/drivers/gpu/drm/amd/display/dc/core/dc_link_enc_cfg.c
> > +++ b/drivers/gpu/drm/amd/display/dc/core/dc_link_enc_cfg.c
> > @@ -622,7 +622,6 @@ bool link_enc_cfg_validate(struct dc *dc, struct 
> > dc_state *state)
> > int i, j;
> > uint8_t valid_count = 0;
> > uint8_t dig_stream_count = 0;
> > -   int matching_stream_ptrs = 0;
> > int eng_ids_per_ep_id[MAX_PIPES] = {0};
> > int ep_ids_per_eng_id[MAX_PIPES] = {0};
> > int valid_bitmap = 0;
> > @@ -645,9 +644,7 @@ bool link_enc_cfg_validate(struct dc *dc, struct 
> > dc_state *state)
> > struct link_enc_assignment assignment = 
> > state->res_ctx.link_enc_cfg_ctx.link_enc_assignments[i];
> >
> > if (assignment.valid) {
> > -   if (assignment.stream == state->streams[i])
> > -   matching_stream_ptrs++;
> > -   else
> > +   if (assignment.stream != state->streams[i])
> > valid_stream_ptrs = false;
> > }
> > }
> > --
> > 2.27.0
> >
>
>
> --
> Thanks,
> ~Nick Desaulniers



-- 
Thanks,
~Nick Desaulniers

Re: [PATCH] drm/amdgpu: make DRM_AMD_DC x86-only again

[Nick Desaulniers]

On Mon, Dec 7, 2020 at 1:57 PM Arnd Bergmann  wrote:
>
> On Mon, Dec 7, 2020 at 9:50 PM Christian König  
> wrote:
> > Am 07.12.20 um 21:47 schrieb Alex Deucher:
> > > On Fri, Dec 4, 2020 at 3:13 AM Arnd Bergmann  wrote:
> > >> From: Arnd Bergmann 
> > >>
> > >> As the DRM_AMD_DC_DCN3_0 code was x86-only and fails to build on
> > >> arm64, merging it into DRM_AMD_DC means that the top-level symbol
> > >> is now x86-only as well.
> > >>
> > >> Compilation fails on arm64 with clang-12 with
> > >>
> > >> drivers/gpu/drm/amd/amdgpu/../display/dc/dml/dcn30/display_mode_vba_30.c:3641:6:
> > >>  error: stack frame size of 2416 bytes in function 
> > >> 'dml30_ModeSupportAndSystemConfigurationFull' 
> > >> [-Werror,-Wframe-larger-than=]
> > >> void dml30_ModeSupportAndSystemConfigurationFull(struct display_mode_lib 
> > >> *mode_lib)
> > >>
> > >> I tried to see if the stack usage can be reduced, but this is code
> > >> that is described as "This file is gcc-parsable HW gospel, coming
> > >> straight from HW engineers." and is written in a way that is inherently
> > >> nonportable and not meant to be understood by humans.
> > >>
> > >> There are probably no non-x86 users of this code, so simplify
> > >> the dependency list accordingly.
> > > + Daniel, Timothy
> > >
> > > Others contributed code to enable this on PPC64 and ARM64.
> > > Unfortunately, we don't have these platforms to test with within AMD.
> > > Does PPC64 have the same stack limitations as ARM64?  Harry, Leo, can
> > > you take a look at fixing the stack usage?
> >
> > This reminds me that I wanted to reply on this.
> >
> > 2416 is even to much on x86 if you add -Werror :)
> >
> > So this needs to be fixed anyway.
>
> Right, looking at my latest randconfig logs, I see the same problem on x86
> builds with clang as well, though I'm not entirely sure which other
> configuration
> options are needed to trigger it.
>
> So my patch can be disregarded, but I agree this needs a better fix,
> either in clang or in the dcn driver.

If you could give https://github.com/ClangBuiltLinux/frame-larger-than
a spin again, I would appreciate any feedback.


-- 
Thanks,
~Nick Desaulniers
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [Intel-wired-lan] [PATCH 000/141] Fix fall-through warnings for Clang

[Nick Desaulniers]

m you trust.  I don't doubt it's hard to find maintainers, but
existing maintainers should go out of their way to entrust
co-maintainers especially when they find their workload becomes too
high.  And reviewing/picking up trivial patches is probably a great
way to get started.  If we allow too much knowledge of any one
subsystem to collect with one maintainer, what happens when that
maintainer leaves the community (which, given a finite lifespan, is an
inevitability)?
-- 
Thanks,
~Nick Desaulniers
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH] drm/amdgpu/dc: don't pass -mhard-float to clang

[Nick Desaulniers]

On Tue, May 5, 2020 at 7:25 AM Arnd Bergmann  wrote:
>
> Clang does not appear to care, and instead prints a warning:
>
> clang: warning: argument unused during compilation: '-mhard-float' 
> [-Wunused-command-line-argument]
>
> Signed-off-by: Arnd Bergmann 

I want to be super careful here, this part of the build has been super
tricky in the past.  Just noting before this potentially gets merged
without any testing; we should verify the generated code does not
change with Clang.  In the past, this code compiled but would GPF
sometimes when called into via userspace (see my previous commits
here).

> ---
>  drivers/gpu/drm/amd/display/dc/calcs/Makefile | 5 +++--
>  drivers/gpu/drm/amd/display/dc/dcn20/Makefile | 5 +++--
>  drivers/gpu/drm/amd/display/dc/dcn21/Makefile | 5 +++--
>  drivers/gpu/drm/amd/display/dc/dml/Makefile   | 5 +++--
>  drivers/gpu/drm/amd/display/dc/dsc/Makefile   | 5 +++--
>  5 files changed, 15 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/display/dc/calcs/Makefile 
> b/drivers/gpu/drm/amd/display/dc/calcs/Makefile
> index 4674aca8f206..64195cacf6fc 100644
> --- a/drivers/gpu/drm/amd/display/dc/calcs/Makefile
> +++ b/drivers/gpu/drm/amd/display/dc/calcs/Makefile
> @@ -26,14 +26,15 @@
>  #
>
>  ifdef CONFIG_X86
> -calcs_ccflags := -mhard-float -msse
> +calcs_ccflags := -msse
>  endif
>
>  ifdef CONFIG_PPC64
> -calcs_ccflags := -mhard-float -maltivec
> +calcs_ccflags := -maltivec
>  endif
>
>  ifdef CONFIG_CC_IS_GCC
> +calcs_ccflags += -mhard-float
>  ifeq ($(call cc-ifversion, -lt, 0701, y), y)
>  IS_OLD_GCC = 1
>  endif
> diff --git a/drivers/gpu/drm/amd/display/dc/dcn20/Makefile 
> b/drivers/gpu/drm/amd/display/dc/dcn20/Makefile
> index 5fcaf78334ff..0d3ce716c753 100644
> --- a/drivers/gpu/drm/amd/display/dc/dcn20/Makefile
> +++ b/drivers/gpu/drm/amd/display/dc/dcn20/Makefile
> @@ -10,14 +10,15 @@ DCN20 = dcn20_resource.o dcn20_init.o dcn20_hwseq.o 
> dcn20_dpp.o dcn20_dpp_cm.o d
>  DCN20 += dcn20_dsc.o
>
>  ifdef CONFIG_X86
> -CFLAGS_$(AMDDALPATH)/dc/dcn20/dcn20_resource.o := -mhard-float -msse
> +CFLAGS_$(AMDDALPATH)/dc/dcn20/dcn20_resource.o := -msse
>  endif
>
>  ifdef CONFIG_PPC64
> -CFLAGS_$(AMDDALPATH)/dc/dcn20/dcn20_resource.o := -mhard-float -maltivec
> +CFLAGS_$(AMDDALPATH)/dc/dcn20/dcn20_resource.o := -maltivec
>  endif
>
>  ifdef CONFIG_CC_IS_GCC
> +CFLAGS_$(AMDDALPATH)/dc/dcn20/dcn20_resource.o += -mhard-float
>  ifeq ($(call cc-ifversion, -lt, 0701, y), y)
>  IS_OLD_GCC = 1
>  endif
> diff --git a/drivers/gpu/drm/amd/display/dc/dcn21/Makefile 
> b/drivers/gpu/drm/amd/display/dc/dcn21/Makefile
> index 07684d3e375a..fd209d1cf6bb 100644
> --- a/drivers/gpu/drm/amd/display/dc/dcn21/Makefile
> +++ b/drivers/gpu/drm/amd/display/dc/dcn21/Makefile
> @@ -6,14 +6,15 @@ DCN21 = dcn21_init.o dcn21_hubp.o dcn21_hubbub.o 
> dcn21_resource.o \
>  dcn21_hwseq.o dcn21_link_encoder.o
>
>  ifdef CONFIG_X86
> -CFLAGS_$(AMDDALPATH)/dc/dcn21/dcn21_resource.o := -mhard-float -msse
> +CFLAGS_$(AMDDALPATH)/dc/dcn21/dcn21_resource.o := -msse
>  endif
>
>  ifdef CONFIG_PPC64
> -CFLAGS_$(AMDDALPATH)/dc/dcn21/dcn21_resource.o := -mhard-float -maltivec
> +CFLAGS_$(AMDDALPATH)/dc/dcn21/dcn21_resource.o := -maltivec
>  endif
>
>  ifdef CONFIG_CC_IS_GCC
> +CFLAGS_$(AMDDALPATH)/dc/dcn21/dcn21_resource.o += -mhard-float
>  ifeq ($(call cc-ifversion, -lt, 0701, y), y)
>  IS_OLD_GCC = 1
>  endif
> diff --git a/drivers/gpu/drm/amd/display/dc/dml/Makefile 
> b/drivers/gpu/drm/amd/display/dc/dml/Makefile
> index 7ee8b8460a9b..fb74e79e15a2 100644
> --- a/drivers/gpu/drm/amd/display/dc/dml/Makefile
> +++ b/drivers/gpu/drm/amd/display/dc/dml/Makefile
> @@ -26,14 +26,15 @@
>  # subcomponents.
>
>  ifdef CONFIG_X86
> -dml_ccflags := -mhard-float -msse
> +dml_ccflags := -msse
>  endif
>
>  ifdef CONFIG_PPC64
> -dml_ccflags := -mhard-float -maltivec
> +dml_ccflags := -maltivec
>  endif
>
>  ifdef CONFIG_CC_IS_GCC
> +dml_ccflags += -mhard-float
>  ifeq ($(call cc-ifversion, -lt, 0701, y), y)
>  IS_OLD_GCC = 1
>  endif
> diff --git a/drivers/gpu/drm/amd/display/dc/dsc/Makefile 
> b/drivers/gpu/drm/amd/display/dc/dsc/Makefile
> index 3f66868df171..b0077f5c318d 100644
> --- a/drivers/gpu/drm/amd/display/dc/dsc/Makefile
> +++ b/drivers/gpu/drm/amd/display/dc/dsc/Makefile
> @@ -3,14 +3,15 @@
>  # Makefile for the 'dsc' sub-component of DAL.
>
>  ifdef CONFIG_X86
> -dsc_ccflags := -mhard-float -msse
> +dsc_ccflags := -msse
>  endif
>
>  ifdef CONFIG_PPC64
> -dsc_ccflags := -mhard-float -maltivec
> +dsc_ccflags := -maltivec
>  endif
>
>  ifdef CONFIG_CC_IS_GCC
> +dsc_ccflags += -mhard-float
>  ifeq ($(call cc-ifversion, -lt, 0701, y), y)
>  IS_OLD_GCC = 1
>  endif
> --
> 2.26.0
>


-- 
Thanks,
~Nick Desaulniers
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH] amdgpu: fix integer overflow on 32-bit architectures

[Nick Desaulniers]

See also 
https://lore.kernel.org/lkml/cadnq5_ndtzh5_rgdwkj9c_42xlvrnccs5ddu1ysptfzp94k...@mail.gmail.com/T/#me707e09e92c6e487285e8bb382a607e4e782c249

On Tue, May 5, 2020 at 7:17 AM Christian König  wrote:
>
> Am 05.05.20 um 16:15 schrieb Arnd Bergmann:
> > Multiplying 10 by four overruns a 'long' variable, as clang
> > points out:
> >
> > drivers/gpu/drm/amd/amdgpu/amdgpu_device.c:4160:53: error: overflow in 
> > expression; result is -294967296 with type 'long' 
> > [-Werror,-Winteger-overflow]
> >  expires = ktime_get_mono_fast_ns() + NSEC_PER_SEC * 4L;
> >^
> > Make this a 'long long' constant instead.
> >
> > Fixes: 3f12acc8d6d4 ("drm/amdgpu: put the audio codec into suspend state 
> > before gpu reset V3")
> > Signed-off-by: Arnd Bergmann 
>
> Reviewed-by: Christian König 
>
> > ---
> > I'm not sure the ktime_get_mono_fast_ns() call is necessary here
> > either. Is it intentional because ktime_get_ns() doesn't work
> > during a driver suspend, or just a mistake?
> > ---
> >   drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 2 +-
> >   1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c 
> > b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> > index 6f93af972b0a..2e07e3e6b036 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> > @@ -4157,7 +4157,7 @@ static int amdgpu_device_suspend_display_audio(struct 
> > amdgpu_device *adev)
> >* the audio controller default autosuspend delay setting.
> >* 4S used here is guaranteed to cover that.
> >*/
> > - expires = ktime_get_mono_fast_ns() + NSEC_PER_SEC * 4L;
> > + expires = ktime_get_mono_fast_ns() + NSEC_PER_SEC * 4LL;
> >
> >   while (!pm_runtime_status_suspended(&(p->dev))) {
> >   if (!pm_runtime_suspend(&(p->dev)))
>
> --
> You received this message because you are subscribed to the Google Groups 
> "Clang Built Linux" group.
> To unsubscribe from this group and stop receiving emails from it, send an 
> email to clang-built-linux+unsubscr...@googlegroups.com.
> To view this discussion on the web visit 
> https://groups.google.com/d/msgid/clang-built-linux/e4a852b2-807b-bc73-7328-bcc399341085%40amd.com.



-- 
Thanks,
~Nick Desaulniers
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH] drm/amdgpu: Initialize shadow to false in gfx_v9_0_rlcg_wreg

[Nick Desaulniers]

On Wed, Mar 18, 2020 at 1:28 PM Joe Perches  wrote:
>
> On Tue, 2020-03-17 at 17:25 -0700, Nathan Chancellor wrote:
> > clang warns:
> >
> > drivers/gpu/drm/amd/amdgpu/gfx_v9_0.c:754:6: warning: variable 'shadow'
> > is used uninitialized whenever 'if' condition is
> > false [-Wsometimes-uninitialized]
> > if (offset == grbm_cntl || offset == grbm_idx)
> > ^
> > drivers/gpu/drm/amd/amdgpu/gfx_v9_0.c:757:6: note: uninitialized use
> > occurs here
> > if (shadow) {
> > ^~
>
> Wouldn't it be better to get rid of the shadow variable completely?

Yes, much better indeed. Seems it only has one use.

> ---
>  drivers/gpu/drm/amd/amdgpu/gfx_v9_0.c | 6 +-
>  1 file changed, 1 insertion(+), 5 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/gfx_v9_0.c 
> b/drivers/gpu/drm/amd/amdgpu/gfx_v9_0.c
> index 7bc248..496b9e 100644
> --- a/drivers/gpu/drm/amd/amdgpu/gfx_v9_0.c
> +++ b/drivers/gpu/drm/amd/amdgpu/gfx_v9_0.c
> @@ -735,7 +735,6 @@ void gfx_v9_0_rlcg_wreg(struct amdgpu_device *adev, u32 
> offset, u32 v)
> static void *spare_int;
> static uint32_t grbm_cntl;
> static uint32_t grbm_idx;
> -   bool shadow;
>
> scratch_reg0 = adev->rmmio + 
> (adev->reg_offset[GC_HWIP][0][mmSCRATCH_REG0_BASE_IDX] + mmSCRATCH_REG0)*4;
> scratch_reg1 = adev->rmmio + 
> (adev->reg_offset[GC_HWIP][0][mmSCRATCH_REG1_BASE_IDX] + mmSCRATCH_REG1)*4;
> @@ -751,10 +750,7 @@ void gfx_v9_0_rlcg_wreg(struct amdgpu_device *adev, u32 
> offset, u32 v)
> return;
> }
>
> -   if (offset == grbm_cntl || offset == grbm_idx)
> -   shadow = true;
> -
> -   if (shadow) {
> +   if (offset == grbm_cntl || offset == grbm_idx) {
> if (offset  == grbm_cntl)
> writel(v, scratch_reg2);
> else if (offset == grbm_idx)
>
>
> --
> You received this message because you are subscribed to the Google Groups 
> "Clang Built Linux" group.
> To unsubscribe from this group and stop receiving emails from it, send an 
> email to clang-built-linux+unsubscr...@googlegroups.com.
> To view this discussion on the web visit 
> https://groups.google.com/d/msgid/clang-built-linux/3a997f4ee640e607a171a19668f5f5484062116c.camel%40perches.com.



-- 
Thanks,
~Nick Desaulniers
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH] drm/amdgpu: Initialize shadow to false in gfx_v9_0_rlcg_wreg

[Nick Desaulniers]

On Tue, Mar 17, 2020 at 5:25 PM Nathan Chancellor
 wrote:
>
> clang warns:
>
> drivers/gpu/drm/amd/amdgpu/gfx_v9_0.c:754:6: warning: variable 'shadow'
> is used uninitialized whenever 'if' condition is
> false [-Wsometimes-uninitialized]
> if (offset == grbm_cntl || offset == grbm_idx)
> ^
> drivers/gpu/drm/amd/amdgpu/gfx_v9_0.c:757:6: note: uninitialized use
> occurs here
> if (shadow) {
> ^~
> drivers/gpu/drm/amd/amdgpu/gfx_v9_0.c:754:2: note: remove the 'if' if
> its condition is always true
> if (offset == grbm_cntl || offset == grbm_idx)
> ^~
> drivers/gpu/drm/amd/amdgpu/gfx_v9_0.c:738:13: note: initialize the
> variable 'shadow' to silence this warning
> bool shadow;
>^
> = 0
> 1 warning generated.
>
> It is not wrong so initialize shadow to false to ensure shadow is always
> used initialized.

Yep, thanks for the patch.
Reviewed-by: Nick Desaulniers 

>
> Fixes: 2e0cc4d48b91 ("drm/amdgpu: revise RLCG access path")
> Link: https://github.com/ClangBuiltLinux/linux/issues/936
> Signed-off-by: Nathan Chancellor 
> ---
>  drivers/gpu/drm/amd/amdgpu/gfx_v9_0.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/gfx_v9_0.c 
> b/drivers/gpu/drm/amd/amdgpu/gfx_v9_0.c
> index 7bc2486167e7..affbff76758c 100644
> --- a/drivers/gpu/drm/amd/amdgpu/gfx_v9_0.c
> +++ b/drivers/gpu/drm/amd/amdgpu/gfx_v9_0.c
> @@ -735,7 +735,7 @@ void gfx_v9_0_rlcg_wreg(struct amdgpu_device *adev, u32 
> offset, u32 v)
> static void *spare_int;
> static uint32_t grbm_cntl;
> static uint32_t grbm_idx;
> -   bool shadow;
> +   bool shadow = false;
>
> scratch_reg0 = adev->rmmio + 
> (adev->reg_offset[GC_HWIP][0][mmSCRATCH_REG0_BASE_IDX] + mmSCRATCH_REG0)*4;
> scratch_reg1 = adev->rmmio + 
> (adev->reg_offset[GC_HWIP][0][mmSCRATCH_REG1_BASE_IDX] + mmSCRATCH_REG1)*4;
> --
> 2.26.0.rc1
>
> --
> You received this message because you are subscribed to the Google Groups 
> "Clang Built Linux" group.
> To unsubscribe from this group and stop receiving emails from it, send an 
> email to clang-built-linux+unsubscr...@googlegroups.com.
> To view this discussion on the web visit 
> https://groups.google.com/d/msgid/clang-built-linux/20200318002500.52471-1-natechancellor%40gmail.com.



-- 
Thanks,
~Nick Desaulniers
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [CI-NOTIFY]: TCWG Bisect tcwg_kernel/llvm-release-aarch64-next-allmodconfig - Build # 48 - Successful!

[Nick Desaulniers]

  hdcp, "h_prime_available"))
> +   goto out;
> +   if (!mod_hdcp_execute_and_set(mod_hdcp_read_h_prime,
> +   >h_prime_read, ,
> +   hdcp, "h_prime_read"))
> +   goto out;
> +   if (!mod_hdcp_execute_and_set(mod_hdcp_hdcp2_validate_h_prime,
> +   >h_prime_validation, ,
> +   hdcp, "h_prime_validation"))
> +   goto out;
> +out:
> +   return status;
> +}
> +
> +static enum mod_hdcp_status locality_check(struct mod_hdcp *hdcp,
> +   struct mod_hdcp_event_context *event_ctx,
> +   struct mod_hdcp_transition_input_hdcp2 *input)
> +{
> +   enum mod_hdcp_status status = MOD_HDCP_STATUS_SUCCESS;
> +
> +   if (event_ctx->event != MOD_HDCP_EVENT_CALLBACK) {
> +   event_ctx->unexpected_event = 1;
> +   goto out;
> +   }
> +
> +   if (!mod_hdcp_execute_and_set(mod_hdcp_hdcp2_prepare_lc_init,
> +   >lc_init_prepare, ,
> +   hdcp, "lc_init_prepare"))
> +   goto out;
> +   if (!mod_hdcp_execute_and_set(mod_hdcp_write_lc_init,
> +   >lc_init_write, ,
> +hdcp, "lc_init_write"))
> +   goto out;
> +   if (is_dp_hdcp(hdcp))
> +   udelay(16000);
> +   else
> +   if (!mod_hdcp_execute_and_set(poll_l_prime_available,
> +   >l_prime_available_poll, ,
> +   hdcp, "l_prime_available_poll"))
> +   goto out;
> +   if (!mod_hdcp_execute_and_set(mod_hdcp_read_l_prime,
> +   >l_prime_read, ,
> +   hdcp, "l_prime_read"))
> +   goto out;
> +   if (!mod_hdcp_execute_and_set(mod_hdcp_hdcp2_validate_l_prime,
> +   >l_prime_validation, ,
> +   hdcp, "l_prime_validation"))
> +   goto out;
> +out:
> +   return status;
> +}
> +
> +static enum mod_hdcp_status exchange_ks_and_test_for_repeater(struct 
> mod_hdcp *hdcp,
> +   struct mod_hdcp_event_context *event_ctx,
> +   struct mod_hdcp_transition_input_hdcp2 *input)
> +{
> +   enum mod_hdcp_status status = MOD_HDCP_STATUS_SUCCESS;
> +
> +   if (event_ctx->event != MOD_HDCP_EVENT_CALLBACK) {
> +   event_ctx->unexpected_event = 1;
> +   goto out;
> +   }
> +
> +   if (!mod_hdcp_execute_and_set(mod_hdcp_hdcp2_prepare_eks,
> +   >e

Re: [PATCH 0/3] drm/amdgpu: fix stack alignment ABI mismatch

[Nick Desaulniers]

On Wed, Oct 16, 2019 at 4:02 PM Nick Desaulniers
 wrote:
>
> The x86 kernel is compiled with an 8B stack alignment via
> `-mpreferred-stack-boundary=3` for GCC since 3.6-rc1 via
> commit d9b0cde91c60 ("x86-64, gcc: Use -mpreferred-stack-boundary=3 if 
> supported")
> or `-mstack-alignment=8` for Clang. Parts of the AMDGPU driver are
> compiled with 16B stack alignment.
>
> Generally, the stack alignment is part of the ABI. Linking together two
> different translation units with differing stack alignment is dangerous,
> particularly when the translation unit with the smaller stack alignment
> makes calls into the translation unit with the larger stack alignment.
> While 8B aligned stacks are sometimes also 16B aligned, they are not
> always.
>
> Multiple users have reported General Protection Faults (GPF) when using
> the AMDGPU driver compiled with Clang. Clang is placing objects in stack
> slots assuming the stack is 16B aligned, and selecting instructions that
> require 16B aligned memory operands.
>
> At runtime, syscall handlers with 8B aligned stack call into code that
> assumes 16B stack alignment.  When the stack is a multiple of 8B but not
> 16B, these instructions result in a GPF.
>
> Remove the code that added compatibility between the differing compiler
> flags, as it will result in runtime GPFs when built with Clang.
>
> The series is broken into 3 patches, the first is an important fix for
> Clang for ChromeOS. The rest are attempted cleanups for GCC, but require
> additional boot testing. The first patch is critical, the rest are nice
> to have. I've compile tested the series with ToT Clang, GCC 4.9, and GCC
> 8.3 **but** I do not have hardware to test on, so I need folks with the
> above compilers and relevant hardware to help test the series.
>
> The first patch is a functional change for Clang only. It does not
> change anything for any version of GCC. Yuxuan boot tested a previous
> incarnation on hardware, but I've changed it enough that I think it made
> sense to drop the previous tested by tag.

Thanks for testing the first patch Shirish. Are you or Yuxuan able to
test the rest of the series with any combination of {clang|gcc < 7.1|
gcc >= 7.1} on hardware and report your findings?

>
> The second patch is a functional change for GCC 7.1+ only. It does not
> affect older versions of GCC or Clang (though if someone wanted to
> double check with pre-GCC 7.1 it wouldn't hurt).  It should be boot
> tested on GCC 7.1+ on the relevant hardware.
>
> The final patch is also a functional change for GCC 7.1+ only. It does
> not affect older versions of GCC or Clang. It should be boot tested on
> GCC 7.1+ on the relevant hardware. Theoretically, there may be an issue
> with it, and it's ok to drop it. The series was intentional broken into
> 3 in order to allow them to be incrementally tested and accepted. It's
> ok to take earlier patches without the later patches.
>
> And finally, I do not condone linking object files of differing stack
> alignments.  Idealistically, we'd mark the driver broken for pre-GCC
> 7.1.  Pragmatically, "if it ain't broke, don't fix it."

Harry, Alex,
Thoughts on the series? Has AMD been able to stress test these more internally?

>
> Nick Desaulniers (3):
>   drm/amdgpu: fix stack alignment ABI mismatch for Clang
>   drm/amdgpu: fix stack alignment ABI mismatch for GCC 7.1+
>   drm/amdgpu: enable -msse2 for GCC 7.1+ users
>
>  drivers/gpu/drm/amd/display/dc/calcs/Makefile | 19 ---
>  drivers/gpu/drm/amd/display/dc/dcn20/Makefile | 19 ---
>  drivers/gpu/drm/amd/display/dc/dcn21/Makefile | 19 ---
>  drivers/gpu/drm/amd/display/dc/dml/Makefile   | 19 ---
>  drivers/gpu/drm/amd/display/dc/dsc/Makefile   | 19 ---
>  5 files changed, 60 insertions(+), 35 deletions(-)
>
> --
> 2.23.0.700.g56cf767bdb-goog
>


-- 
Thanks,
~Nick Desaulniers
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: AMDGPU and 16B stack alignment

[Nick Desaulniers]

On Wed, Oct 16, 2019 at 11:55 AM Arvind Sankar  wrote:
>
> On Tue, Oct 15, 2019 at 06:51:26PM -0700, Nick Desaulniers wrote:
> > On Tue, Oct 15, 2019 at 1:26 PM Arvind Sankar  wrote:
> > >
> > > On Tue, Oct 15, 2019 at 11:05:56AM -0700, Nick Desaulniers wrote:
> > > > Hmmm...I would have liked to remove it outright, as it is an ABI
> > > > mismatch that is likely to result in instability and non-fun-to-debug
> > > > runtime issues in the future.  I suspect my patch does work for GCC
> > > > 7.1+.  The question is: Do we want to either:
> > > > 1. mark AMDGPU broken for GCC < 7.1, or
> > > > 2. continue supporting it via stack alignment mismatch?
> > > >
> > > > 2 is brittle, and may break at any point in the future, but if it's
> > > > working for someone it does make me feel bad to outright disable it.
> > > > What I'd image 2 looks like is (psuedo code in a Makefile):
> > > >
> > > > if CC_IS_GCC && GCC_VERSION < 7.1:
> > > >   set stack alignment to 16B and hope for the best
> > > >
> > > > So my diff would be amended to keep the stack alignment flags, but
> > > > only to support GCC < 7.1.  And that assumes my change compiles with
> > > > GCC 7.1+. (Looks like it does for me locally with GCC 8.3, but I would
> > > > feel even more confident if someone with hardware to test on and GCC
> > > > 7.1+ could boot test).
> > > > --
> > > > Thanks,
> > > > ~Nick Desaulniers
> > >
> > > If we do keep it, would adding -mstackrealign make it more robust?
> > > That's simple and will only add the alignment to functions that require
> > > 16-byte alignment (at least on gcc).
> >
> > I think there's also `-mincoming-stack-boundary=`.
> > https://github.com/ClangBuiltLinux/linux/issues/735#issuecomment-540038017
>
> Yes, but -mstackrealign looks like it's supported by clang as well.

Good to know, but I want less duct tape, not more.

> >
> > >
> > > Alternative is to use
> > > __attribute__((force_align_arg_pointer)) on functions that might be
> > > called from 8-byte-aligned code.
> >
> > Which is hard to automate and easy to forget.  Likely a large diff to fix 
> > today.
>
> Right, this is a no-go, esp to just fix old compilers.
> >
> > >
> > > It looks like -mstackrealign should work from gcc 5.3 onwards.
> >
> > The kernel would generally like to support GCC 4.9+.
> >
> > There's plenty of different ways to keep layering on duct tape and
> > bailing wire to support differing ABIs, but that's just adding
> > technical debt that will have to be repaid one day.  That's why the
> > cleanest solution IMO is mark the driver broken for old toolchains,
> > and use a code-base-consistent stack alignment.  Bending over
> > backwards to support old toolchains means accepting stack alignment
> > mismatches, which is in the "unspecified behavior" ring of the
> > "undefined behavior" Venn diagram.  I have the same opinion on relying
> > on explicitly undefined behavior.
> >
> > I'll send patches for fixing up Clang, but please consider my strong
> > advice to generally avoid stack alignment mismatches, regardless of
> > compiler.
> > --
> > Thanks,
> > ~Nick Desaulniers
>
> What I suggested was in reference to your proposal for dropping the
> -mpreferred-stack-boundary=4 for modern compilers, but keeping it for
> <7.1. -mstackrealign would at least let 5.3 onwards be less likely to
> break (and it doesn't error before then, I think it just doesn't
> actually do anything, so no worse than now at least).
>
> Simply dropping support for <7.1 would be cleanest, yes, but it sounds
> like people don't want to go that far.

That's fair.  I've included your suggestions in the commit message of
02/03 of a series I just sent but forgot to in reply to this thread:
https://lkml.org/lkml/2019/10/16/1700

Also, I do appreciate the suggestions and understand the value of brainstorming.
-- 
Thanks,
~Nick Desaulniers

[PATCH 2/3] drm/amdgpu: fix stack alignment ABI mismatch for GCC 7.1+

[Nick Desaulniers]

GCC earlier than 7.1 errors when compiling code that makes use of
`double`s and sets a stack alignment outside of the range of [2^4-2^12]:

$ cat foo.c
double foo(double x, double y) {
  return x + y;
}
$ gcc-4.9 -mpreferred-stack-boundary=3 foo.c
error: -mpreferred-stack-boundary=3 is not between 4 and 12

This is likely why the AMDGPU driver was ever compiled with a different
stack alignment (and thus different ABI) than the rest of the x86
kernel. The kernel uses 8B stack alignment, while the driver was using
16B stack alignment in a few places.

Since GCC 7.1+ doesn't error, fix the ABI mismatch for users of newer
versions of GCC.

There was discussion about whether to mark the driver broken or not for
users of GCC earlier than 7.1, but since the driver currently is
working, don't explicitly break the driver for them here.

Relying on differing stack alignment is unspecified behavior, and
brittle, and may break in the future.

This patch is no functional change for GCC users earlier than 7.1. It's
been compile tested on GCC 4.9 and 8.3 to check the correct flags. It
should be boot tested when built with GCC 7.1+.

-mincoming-stack-boundary= or -mstackrealign may help keep this code
building for pre-GCC 7.1 users.

The version check for GCC is broken into two conditionals, both because
cc-ifversion is currently GCC specific, and it simplifies a subsequent
patch.

Signed-off-by: Nick Desaulniers 
---
 drivers/gpu/drm/amd/display/dc/calcs/Makefile | 9 +
 drivers/gpu/drm/amd/display/dc/dcn20/Makefile | 9 +
 drivers/gpu/drm/amd/display/dc/dcn21/Makefile | 9 +
 drivers/gpu/drm/amd/display/dc/dml/Makefile   | 9 +
 drivers/gpu/drm/amd/display/dc/dsc/Makefile   | 9 +
 5 files changed, 45 insertions(+)

diff --git a/drivers/gpu/drm/amd/display/dc/calcs/Makefile 
b/drivers/gpu/drm/amd/display/dc/calcs/Makefile
index 4b1a8a08a5de..a1af55a86508 100644
--- a/drivers/gpu/drm/amd/display/dc/calcs/Makefile
+++ b/drivers/gpu/drm/amd/display/dc/calcs/Makefile
@@ -27,6 +27,15 @@
 calcs_ccflags := -mhard-float -msse
 
 ifdef CONFIG_CC_IS_GCC
+ifeq ($(call cc-ifversion, -lt, 0701, y), y)
+IS_OLD_GCC = 1
+endif
+endif
+
+ifdef IS_OLD_GCC
+# Stack alignment mismatch, proceed with caution.
+# GCC < 7.1 cannot compile code using `double` and -mpreferred-stack-boundary=3
+# (8B stack alignment).
 calcs_ccflags += -mpreferred-stack-boundary=4
 endif
 
diff --git a/drivers/gpu/drm/amd/display/dc/dcn20/Makefile 
b/drivers/gpu/drm/amd/display/dc/dcn20/Makefile
index 5fe3eb80075d..cb0ac131f74a 100644
--- a/drivers/gpu/drm/amd/display/dc/dcn20/Makefile
+++ b/drivers/gpu/drm/amd/display/dc/dcn20/Makefile
@@ -13,6 +13,15 @@ endif
 CFLAGS_$(AMDDALPATH)/dc/dcn20/dcn20_resource.o := -mhard-float -msse
 
 ifdef CONFIG_CC_IS_GCC
+ifeq ($(call cc-ifversion, -lt, 0701, y), y)
+IS_OLD_GCC = 1
+endif
+endif
+
+ifdef IS_OLD_GCC
+# Stack alignment mismatch, proceed with caution.
+# GCC < 7.1 cannot compile code using `double` and -mpreferred-stack-boundary=3
+# (8B stack alignment).
 CFLAGS_$(AMDDALPATH)/dc/dcn20/dcn20_resource.o += -mpreferred-stack-boundary=4
 endif
 
diff --git a/drivers/gpu/drm/amd/display/dc/dcn21/Makefile 
b/drivers/gpu/drm/amd/display/dc/dcn21/Makefile
index 7057e20748b9..f92320ddd27f 100644
--- a/drivers/gpu/drm/amd/display/dc/dcn21/Makefile
+++ b/drivers/gpu/drm/amd/display/dc/dcn21/Makefile
@@ -6,6 +6,15 @@ DCN21 = dcn21_hubp.o dcn21_hubbub.o dcn21_resource.o
 CFLAGS_$(AMDDALPATH)/dc/dcn21/dcn21_resource.o := -mhard-float -msse
 
 ifdef CONFIG_CC_IS_GCC
+ifeq ($(call cc-ifversion, -lt, 0701, y), y)
+IS_OLD_GCC = 1
+endif
+endif
+
+ifdef IS_OLD_GCC
+# Stack alignment mismatch, proceed with caution.
+# GCC < 7.1 cannot compile code using `double` and -mpreferred-stack-boundary=3
+# (8B stack alignment).
 CFLAGS_$(AMDDALPATH)/dc/dcn21/dcn21_resource.o += -mpreferred-stack-boundary=4
 endif
 
diff --git a/drivers/gpu/drm/amd/display/dc/dml/Makefile 
b/drivers/gpu/drm/amd/display/dc/dml/Makefile
index 1bd6e307b7f8..ef1bdd20b425 100644
--- a/drivers/gpu/drm/amd/display/dc/dml/Makefile
+++ b/drivers/gpu/drm/amd/display/dc/dml/Makefile
@@ -27,6 +27,15 @@
 dml_ccflags := -mhard-float -msse
 
 ifdef CONFIG_CC_IS_GCC
+ifeq ($(call cc-ifversion, -lt, 0701, y), y)
+IS_OLD_GCC = 1
+endif
+endif
+
+ifdef IS_OLD_GCC
+# Stack alignment mismatch, proceed with caution.
+# GCC < 7.1 cannot compile code using `double` and -mpreferred-stack-boundary=3
+# (8B stack alignment).
 dml_ccflags += -mpreferred-stack-boundary=4
 endif
 
diff --git a/drivers/gpu/drm/amd/display/dc/dsc/Makefile 
b/drivers/gpu/drm/amd/display/dc/dsc/Makefile
index 932c3055230e..3f7840828a9f 100644
--- a/drivers/gpu/drm/amd/display/dc/dsc/Makefile
+++ b/drivers/gpu/drm/amd/display/dc/dsc/Makefile
@@ -4,6 +4,15 @@
 dsc_ccflags := -mhard-float -msse
 
 ifdef CONFIG_CC_IS_GCC
+ifeq ($(call cc-ifversion, -lt, 0701, y), y)
+IS_OLD_GCC = 1
+endif
+endif
+
+ifdef IS_OLD_GCC
+# Stack alignment mismatch, proceed with caution.

[PATCH 0/3] drm/amdgpu: fix stack alignment ABI mismatch

[Nick Desaulniers]

The x86 kernel is compiled with an 8B stack alignment via
`-mpreferred-stack-boundary=3` for GCC since 3.6-rc1 via
commit d9b0cde91c60 ("x86-64, gcc: Use -mpreferred-stack-boundary=3 if 
supported")
or `-mstack-alignment=8` for Clang. Parts of the AMDGPU driver are
compiled with 16B stack alignment.

Generally, the stack alignment is part of the ABI. Linking together two
different translation units with differing stack alignment is dangerous,
particularly when the translation unit with the smaller stack alignment
makes calls into the translation unit with the larger stack alignment.
While 8B aligned stacks are sometimes also 16B aligned, they are not
always.

Multiple users have reported General Protection Faults (GPF) when using
the AMDGPU driver compiled with Clang. Clang is placing objects in stack
slots assuming the stack is 16B aligned, and selecting instructions that
require 16B aligned memory operands.

At runtime, syscall handlers with 8B aligned stack call into code that
assumes 16B stack alignment.  When the stack is a multiple of 8B but not
16B, these instructions result in a GPF.

Remove the code that added compatibility between the differing compiler
flags, as it will result in runtime GPFs when built with Clang.

The series is broken into 3 patches, the first is an important fix for
Clang for ChromeOS. The rest are attempted cleanups for GCC, but require
additional boot testing. The first patch is critical, the rest are nice
to have. I've compile tested the series with ToT Clang, GCC 4.9, and GCC
8.3 **but** I do not have hardware to test on, so I need folks with the
above compilers and relevant hardware to help test the series.

The first patch is a functional change for Clang only. It does not
change anything for any version of GCC. Yuxuan boot tested a previous
incarnation on hardware, but I've changed it enough that I think it made
sense to drop the previous tested by tag.

The second patch is a functional change for GCC 7.1+ only. It does not
affect older versions of GCC or Clang (though if someone wanted to
double check with pre-GCC 7.1 it wouldn't hurt).  It should be boot
tested on GCC 7.1+ on the relevant hardware.

The final patch is also a functional change for GCC 7.1+ only. It does
not affect older versions of GCC or Clang. It should be boot tested on
GCC 7.1+ on the relevant hardware. Theoretically, there may be an issue
with it, and it's ok to drop it. The series was intentional broken into
3 in order to allow them to be incrementally tested and accepted. It's
ok to take earlier patches without the later patches.

And finally, I do not condone linking object files of differing stack
alignments.  Idealistically, we'd mark the driver broken for pre-GCC
7.1.  Pragmatically, "if it ain't broke, don't fix it."

Nick Desaulniers (3):
  drm/amdgpu: fix stack alignment ABI mismatch for Clang
  drm/amdgpu: fix stack alignment ABI mismatch for GCC 7.1+
  drm/amdgpu: enable -msse2 for GCC 7.1+ users

 drivers/gpu/drm/amd/display/dc/calcs/Makefile | 19 ---
 drivers/gpu/drm/amd/display/dc/dcn20/Makefile | 19 ---
 drivers/gpu/drm/amd/display/dc/dcn21/Makefile | 19 ---
 drivers/gpu/drm/amd/display/dc/dml/Makefile   | 19 ---
 drivers/gpu/drm/amd/display/dc/dsc/Makefile   | 19 ---
 5 files changed, 60 insertions(+), 35 deletions(-)

-- 
2.23.0.700.g56cf767bdb-goog

[PATCH 3/3] drm/amdgpu: enable -msse2 for GCC 7.1+ users

[Nick Desaulniers]

A final attempt at enabling sse2 for GCC users.

Orininally attempted in:
commit 10117450735c ("drm/amd/display: add -msse2 to prevent Clang from 
emitting libcalls to undefined SW FP routines")

Reverted due to "reported instability" in:
commit 193392ed9f69 ("Revert "drm/amd/display: add -msse2 to prevent Clang from 
emitting libcalls to undefined SW FP routines"")

Re-added just for Clang in:
commit 0f0727d971f6 ("drm/amd/display: readd -msse2 to prevent Clang from 
emitting libcalls to undefined SW FP routines")

The original report didn't have enough information to know if the GPF
was due to misalignment, but I suspect that it was. (The missing
information was the disassembly of the function at the bottom of the
trace, to see if the instruction pointer pointed to an instruction with
16B alignment memory operand requirements.  The stack trace does show
the stack was only 8B but not 16B aligned though, which makes this a
strong possibility).

Now that the stack misalignment issue has been fixed for users of GCC
7.1+, reattempt adding -msse2. This matches Clang.

It will likely never be safe to enable this for pre-GCC 7.1 AND use a
16B aligned stack in these translation units.

This is only a functional change for GCC 7.1+ users, and should be boot
tested.

Link: https://bugs.freedesktop.org/show_bug.cgi?id=109487
Signed-off-by: Nick Desaulniers 
---
 drivers/gpu/drm/amd/display/dc/calcs/Makefile | 4 +---
 drivers/gpu/drm/amd/display/dc/dcn20/Makefile | 4 +---
 drivers/gpu/drm/amd/display/dc/dcn21/Makefile | 4 +---
 drivers/gpu/drm/amd/display/dc/dml/Makefile   | 4 +---
 drivers/gpu/drm/amd/display/dc/dsc/Makefile   | 4 +---
 5 files changed, 5 insertions(+), 15 deletions(-)

diff --git a/drivers/gpu/drm/amd/display/dc/calcs/Makefile 
b/drivers/gpu/drm/amd/display/dc/calcs/Makefile
index a1af55a86508..26c6d735cdc7 100644
--- a/drivers/gpu/drm/amd/display/dc/calcs/Makefile
+++ b/drivers/gpu/drm/amd/display/dc/calcs/Makefile
@@ -37,9 +37,7 @@ ifdef IS_OLD_GCC
 # GCC < 7.1 cannot compile code using `double` and -mpreferred-stack-boundary=3
 # (8B stack alignment).
 calcs_ccflags += -mpreferred-stack-boundary=4
-endif
-
-ifdef CONFIG_CC_IS_CLANG
+else
 calcs_ccflags += -msse2
 endif
 
diff --git a/drivers/gpu/drm/amd/display/dc/dcn20/Makefile 
b/drivers/gpu/drm/amd/display/dc/dcn20/Makefile
index cb0ac131f74a..63f3bddba7da 100644
--- a/drivers/gpu/drm/amd/display/dc/dcn20/Makefile
+++ b/drivers/gpu/drm/amd/display/dc/dcn20/Makefile
@@ -23,9 +23,7 @@ ifdef IS_OLD_GCC
 # GCC < 7.1 cannot compile code using `double` and -mpreferred-stack-boundary=3
 # (8B stack alignment).
 CFLAGS_$(AMDDALPATH)/dc/dcn20/dcn20_resource.o += -mpreferred-stack-boundary=4
-endif
-
-ifdef CONFIG_CC_IS_CLANG
+else
 CFLAGS_$(AMDDALPATH)/dc/dcn20/dcn20_resource.o += -msse2
 endif
 
diff --git a/drivers/gpu/drm/amd/display/dc/dcn21/Makefile 
b/drivers/gpu/drm/amd/display/dc/dcn21/Makefile
index f92320ddd27f..ff50ae71fe27 100644
--- a/drivers/gpu/drm/amd/display/dc/dcn21/Makefile
+++ b/drivers/gpu/drm/amd/display/dc/dcn21/Makefile
@@ -16,9 +16,7 @@ ifdef IS_OLD_GCC
 # GCC < 7.1 cannot compile code using `double` and -mpreferred-stack-boundary=3
 # (8B stack alignment).
 CFLAGS_$(AMDDALPATH)/dc/dcn21/dcn21_resource.o += -mpreferred-stack-boundary=4
-endif
-
-ifdef CONFIG_CC_IS_CLANG
+else
 CFLAGS_$(AMDDALPATH)/dc/dcn21/dcn21_resource.o += -msse2
 endif
 
diff --git a/drivers/gpu/drm/amd/display/dc/dml/Makefile 
b/drivers/gpu/drm/amd/display/dc/dml/Makefile
index ef1bdd20b425..8df251626e22 100644
--- a/drivers/gpu/drm/amd/display/dc/dml/Makefile
+++ b/drivers/gpu/drm/amd/display/dc/dml/Makefile
@@ -37,9 +37,7 @@ ifdef IS_OLD_GCC
 # GCC < 7.1 cannot compile code using `double` and -mpreferred-stack-boundary=3
 # (8B stack alignment).
 dml_ccflags += -mpreferred-stack-boundary=4
-endif
-
-ifdef CONFIG_CC_IS_CLANG
+else
 dml_ccflags += -msse2
 endif
 
diff --git a/drivers/gpu/drm/amd/display/dc/dsc/Makefile 
b/drivers/gpu/drm/amd/display/dc/dsc/Makefile
index 3f7840828a9f..970737217e53 100644
--- a/drivers/gpu/drm/amd/display/dc/dsc/Makefile
+++ b/drivers/gpu/drm/amd/display/dc/dsc/Makefile
@@ -14,9 +14,7 @@ ifdef IS_OLD_GCC
 # GCC < 7.1 cannot compile code using `double` and -mpreferred-stack-boundary=3
 # (8B stack alignment).
 dsc_ccflags += -mpreferred-stack-boundary=4
-endif
-
-ifdef CONFIG_CC_IS_CLANG
+else
 dsc_ccflags += -msse2
 endif
 
-- 
2.23.0.700.g56cf767bdb-goog

[PATCH 1/3] drm/amdgpu: fix stack alignment ABI mismatch for Clang

[Nick Desaulniers]

The x86 kernel is compiled with an 8B stack alignment via
`-mpreferred-stack-boundary=3` for GCC since 3.6-rc1 via
commit d9b0cde91c60 ("x86-64, gcc: Use -mpreferred-stack-boundary=3 if 
supported")
or `-mstack-alignment=8` for Clang. Parts of the AMDGPU driver are
compiled with 16B stack alignment.

Generally, the stack alignment is part of the ABI. Linking together two
different translation units with differing stack alignment is dangerous,
particularly when the translation unit with the smaller stack alignment
makes calls into the translation unit with the larger stack alignment.
While 8B aligned stacks are sometimes also 16B aligned, they are not
always.

Multiple users have reported General Protection Faults (GPF) when using
the AMDGPU driver compiled with Clang. Clang is placing objects in stack
slots assuming the stack is 16B aligned, and selecting instructions that
require 16B aligned memory operands.

At runtime, syscall handlers with 8B aligned stack call into code that
assumes 16B stack alignment.  When the stack is a multiple of 8B but not
16B, these instructions result in a GPF.

Remove the code that added compatibility between the differing compiler
flags, as it will result in runtime GPFs when built with Clang. Cleanups
for GCC will be sent in later patches in the series.

Link: https://github.com/ClangBuiltLinux/linux/issues/735
Debugged-by: Yuxuan Shui 
Reported-by: Shirish S 
Reported-by: Yuxuan Shui 
Suggested-by: Andrew Cooper 
Signed-off-by: Nick Desaulniers 
---
 drivers/gpu/drm/amd/display/dc/calcs/Makefile | 10 --
 drivers/gpu/drm/amd/display/dc/dcn20/Makefile | 10 --
 drivers/gpu/drm/amd/display/dc/dcn21/Makefile | 10 --
 drivers/gpu/drm/amd/display/dc/dml/Makefile   | 10 --
 drivers/gpu/drm/amd/display/dc/dsc/Makefile   | 10 --
 5 files changed, 20 insertions(+), 30 deletions(-)

diff --git a/drivers/gpu/drm/amd/display/dc/calcs/Makefile 
b/drivers/gpu/drm/amd/display/dc/calcs/Makefile
index 985633c08a26..4b1a8a08a5de 100644
--- a/drivers/gpu/drm/amd/display/dc/calcs/Makefile
+++ b/drivers/gpu/drm/amd/display/dc/calcs/Makefile
@@ -24,13 +24,11 @@
 # It calculates Bandwidth and Watermarks values for HW programming
 #
 
-ifneq ($(call cc-option, -mpreferred-stack-boundary=4),)
-   cc_stack_align := -mpreferred-stack-boundary=4
-else ifneq ($(call cc-option, -mstack-alignment=16),)
-   cc_stack_align := -mstack-alignment=16
-endif
+calcs_ccflags := -mhard-float -msse
 
-calcs_ccflags := -mhard-float -msse $(cc_stack_align)
+ifdef CONFIG_CC_IS_GCC
+calcs_ccflags += -mpreferred-stack-boundary=4
+endif
 
 ifdef CONFIG_CC_IS_CLANG
 calcs_ccflags += -msse2
diff --git a/drivers/gpu/drm/amd/display/dc/dcn20/Makefile 
b/drivers/gpu/drm/amd/display/dc/dcn20/Makefile
index ddb8d5649e79..5fe3eb80075d 100644
--- a/drivers/gpu/drm/amd/display/dc/dcn20/Makefile
+++ b/drivers/gpu/drm/amd/display/dc/dcn20/Makefile
@@ -10,13 +10,11 @@ ifdef CONFIG_DRM_AMD_DC_DSC_SUPPORT
 DCN20 += dcn20_dsc.o
 endif
 
-ifneq ($(call cc-option, -mpreferred-stack-boundary=4),)
-   cc_stack_align := -mpreferred-stack-boundary=4
-else ifneq ($(call cc-option, -mstack-alignment=16),)
-   cc_stack_align := -mstack-alignment=16
-endif
+CFLAGS_$(AMDDALPATH)/dc/dcn20/dcn20_resource.o := -mhard-float -msse
 
-CFLAGS_$(AMDDALPATH)/dc/dcn20/dcn20_resource.o := -mhard-float -msse 
$(cc_stack_align)
+ifdef CONFIG_CC_IS_GCC
+CFLAGS_$(AMDDALPATH)/dc/dcn20/dcn20_resource.o += -mpreferred-stack-boundary=4
+endif
 
 ifdef CONFIG_CC_IS_CLANG
 CFLAGS_$(AMDDALPATH)/dc/dcn20/dcn20_resource.o += -msse2
diff --git a/drivers/gpu/drm/amd/display/dc/dcn21/Makefile 
b/drivers/gpu/drm/amd/display/dc/dcn21/Makefile
index ef673bffc241..7057e20748b9 100644
--- a/drivers/gpu/drm/amd/display/dc/dcn21/Makefile
+++ b/drivers/gpu/drm/amd/display/dc/dcn21/Makefile
@@ -3,13 +3,11 @@
 
 DCN21 = dcn21_hubp.o dcn21_hubbub.o dcn21_resource.o
 
-ifneq ($(call cc-option, -mpreferred-stack-boundary=4),)
-   cc_stack_align := -mpreferred-stack-boundary=4
-else ifneq ($(call cc-option, -mstack-alignment=16),)
-   cc_stack_align := -mstack-alignment=16
-endif
+CFLAGS_$(AMDDALPATH)/dc/dcn21/dcn21_resource.o := -mhard-float -msse
 
-CFLAGS_$(AMDDALPATH)/dc/dcn21/dcn21_resource.o := -mhard-float -msse 
$(cc_stack_align)
+ifdef CONFIG_CC_IS_GCC
+CFLAGS_$(AMDDALPATH)/dc/dcn21/dcn21_resource.o += -mpreferred-stack-boundary=4
+endif
 
 ifdef CONFIG_CC_IS_CLANG
 CFLAGS_$(AMDDALPATH)/dc/dcn21/dcn21_resource.o += -msse2
diff --git a/drivers/gpu/drm/amd/display/dc/dml/Makefile 
b/drivers/gpu/drm/amd/display/dc/dml/Makefile
index 5b2a65b42403..1bd6e307b7f8 100644
--- a/drivers/gpu/drm/amd/display/dc/dml/Makefile
+++ b/drivers/gpu/drm/amd/display/dc/dml/Makefile
@@ -24,13 +24,11 @@
 # It provides the general basic services required by other DAL
 # subcomponents.
 
-ifneq ($(call cc-option, -mpreferred-stack-boundary=4),)
-   cc_stack_align := -mpreferred-stack-boundary=4
-else ifneq ($(call cc-optio

Re: AMDGPU and 16B stack alignment

[Nick Desaulniers]

On Tue, Oct 15, 2019 at 1:26 PM Arvind Sankar  wrote:
>
> On Tue, Oct 15, 2019 at 11:05:56AM -0700, Nick Desaulniers wrote:
> > Hmmm...I would have liked to remove it outright, as it is an ABI
> > mismatch that is likely to result in instability and non-fun-to-debug
> > runtime issues in the future.  I suspect my patch does work for GCC
> > 7.1+.  The question is: Do we want to either:
> > 1. mark AMDGPU broken for GCC < 7.1, or
> > 2. continue supporting it via stack alignment mismatch?
> >
> > 2 is brittle, and may break at any point in the future, but if it's
> > working for someone it does make me feel bad to outright disable it.
> > What I'd image 2 looks like is (psuedo code in a Makefile):
> >
> > if CC_IS_GCC && GCC_VERSION < 7.1:
> >   set stack alignment to 16B and hope for the best
> >
> > So my diff would be amended to keep the stack alignment flags, but
> > only to support GCC < 7.1.  And that assumes my change compiles with
> > GCC 7.1+. (Looks like it does for me locally with GCC 8.3, but I would
> > feel even more confident if someone with hardware to test on and GCC
> > 7.1+ could boot test).
> > --
> > Thanks,
> > ~Nick Desaulniers
>
> If we do keep it, would adding -mstackrealign make it more robust?
> That's simple and will only add the alignment to functions that require
> 16-byte alignment (at least on gcc).

I think there's also `-mincoming-stack-boundary=`.
https://github.com/ClangBuiltLinux/linux/issues/735#issuecomment-540038017

>
> Alternative is to use
> __attribute__((force_align_arg_pointer)) on functions that might be
> called from 8-byte-aligned code.

Which is hard to automate and easy to forget.  Likely a large diff to fix today.

>
> It looks like -mstackrealign should work from gcc 5.3 onwards.

The kernel would generally like to support GCC 4.9+.

There's plenty of different ways to keep layering on duct tape and
bailing wire to support differing ABIs, but that's just adding
technical debt that will have to be repaid one day.  That's why the
cleanest solution IMO is mark the driver broken for old toolchains,
and use a code-base-consistent stack alignment.  Bending over
backwards to support old toolchains means accepting stack alignment
mismatches, which is in the "unspecified behavior" ring of the
"undefined behavior" Venn diagram.  I have the same opinion on relying
on explicitly undefined behavior.

I'll send patches for fixing up Clang, but please consider my strong
advice to generally avoid stack alignment mismatches, regardless of
compiler.
--
Thanks,
~Nick Desaulniers

Re: AMDGPU and 16B stack alignment

[Nick Desaulniers]

On Tue, Oct 15, 2019 at 11:30 AM Alex Deucher  wrote:
>
> On Tue, Oct 15, 2019 at 2:07 PM Nick Desaulniers
>  wrote:
> >
> > On Tue, Oct 15, 2019 at 12:19 AM Arnd Bergmann  wrote:
> > >
> > > On Tue, Oct 15, 2019 at 9:08 AM S, Shirish  wrote:
> > > > On 10/15/2019 3:52 AM, Nick Desaulniers wrote:
> > >
> > > > My gcc build fails with below errors:
> > > >
> > > > dcn_calcs.c:1:0: error: -mpreferred-stack-boundary=3 is not between 4 
> > > > and 12
> > > >
> > > > dcn_calc_math.c:1:0: error: -mpreferred-stack-boundary=3 is not between 
> > > > 4 and 12
> >
> > I was able to reproduce this failure on pre-7.1 versions of GCC.  It
> > seems that when:
> > 1. code is using doubles
> > 2. setting -mpreferred-stack-boundary=3 -mno-sse2, ie. 8B stack alignment
> > than GCC produces that error:
> > https://godbolt.org/z/7T8nbH
> >
> > That's already a tall order of constraints, so it's understandable
> > that the compiler would just error likely during instruction
> > selection, but was eventually taught how to solve such constraints.
> >
> > > >
> > > > While GPF observed on clang builds seem to be fixed.
> >
> > Thanks for the report.  Your testing these patches is invaluable, Shirish!
> >
> > >
> > > Ok, so it seems that gcc insists on having at least 2^4 bytes stack
> > > alignment when
> > > SSE is enabled on x86-64, but does not actually rely on that for
> > > correct operation
> > > unless it's using sse2. So -msse always has to be paired with
> > >  -mpreferred-stack-boundary=3.
> >
> > Seemingly only for older versions of GCC, pre 7.1.
> >
> > >
> > > For clang, it sounds like the opposite is true: when passing 16 byte
> > > stack alignment
> > > and having sse/sse2 enabled, it requires the incoming stack to be 16
> > > byte aligned,
> >
> > I don't think it requires the incoming stack to be 16B aligned for
> > sse2, I think it requires the incoming and current stack alignment to
> > match. Today it does not, which is why we observe GPFs.
> >
> > > but passing 8 byte alignment makes it do the right thing.
> > >
> > > So, should we just always pass $(call cc-option, 
> > > -mpreferred-stack-boundary=4)
> > > to get the desired outcome on both?
> >
> > Hmmm...I would have liked to remove it outright, as it is an ABI
> > mismatch that is likely to result in instability and non-fun-to-debug
> > runtime issues in the future.  I suspect my patch does work for GCC
> > 7.1+.  The question is: Do we want to either:
> > 1. mark AMDGPU broken for GCC < 7.1, or
> > 2. continue supporting it via stack alignment mismatch?
> >
> > 2 is brittle, and may break at any point in the future, but if it's
> > working for someone it does make me feel bad to outright disable it.
> > What I'd image 2 looks like is (psuedo code in a Makefile):
>
> Well, it's been working as is for years now, at least with gcc, so I'd
> hate to break that.

Ok, I'm happy to leave that as is for GCC, then.  Would you prefer I
modify it for GCC >7.1 or just leave it alone (maybe I'll add a
comment about *why* it's done for GCC)? Would you prefer 1 patch or 4?

>
> Alex
>
> >
> > if CC_IS_GCC && GCC_VERSION < 7.1:
> >   set stack alignment to 16B and hope for the best

Ie, this ^

> >
> > So my diff would be amended to keep the stack alignment flags, but
> > only to support GCC < 7.1.  And that assumes my change compiles with
> > GCC 7.1+. (Looks like it does for me locally with GCC 8.3, but I would
> > feel even more confident if someone with hardware to test on and GCC
> > 7.1+ could boot test).

-- 
Thanks,
~Nick Desaulniers

Re: AMDGPU and 16B stack alignment

[Nick Desaulniers]

On Tue, Oct 15, 2019 at 11:05 AM Nick Desaulniers
 wrote:
>
> On Tue, Oct 15, 2019 at 12:19 AM Arnd Bergmann  wrote:
> >
> > On Tue, Oct 15, 2019 at 9:08 AM S, Shirish  wrote:
> > > On 10/15/2019 3:52 AM, Nick Desaulniers wrote:
> >
> > > My gcc build fails with below errors:
> > >
> > > dcn_calcs.c:1:0: error: -mpreferred-stack-boundary=3 is not between 4 and 
> > > 12
> > >
> > > dcn_calc_math.c:1:0: error: -mpreferred-stack-boundary=3 is not between 4 
> > > and 12
>
> I was able to reproduce this failure on pre-7.1 versions of GCC.  It
> seems that when:
> 1. code is using doubles
> 2. setting -mpreferred-stack-boundary=3 -mno-sse2, ie. 8B stack alignment
> than GCC produces that error:
> https://godbolt.org/z/7T8nbH
>

Also, I suspect that very error solves the mystery of "why was 16B
stack alignment ever specified?"
-- 
Thanks,
~Nick Desaulniers

Re: AMDGPU and 16B stack alignment

[Nick Desaulniers]

On Tue, Oct 15, 2019 at 12:19 AM Arnd Bergmann  wrote:
>
> On Tue, Oct 15, 2019 at 9:08 AM S, Shirish  wrote:
> > On 10/15/2019 3:52 AM, Nick Desaulniers wrote:
>
> > My gcc build fails with below errors:
> >
> > dcn_calcs.c:1:0: error: -mpreferred-stack-boundary=3 is not between 4 and 12
> >
> > dcn_calc_math.c:1:0: error: -mpreferred-stack-boundary=3 is not between 4 
> > and 12

I was able to reproduce this failure on pre-7.1 versions of GCC.  It
seems that when:
1. code is using doubles
2. setting -mpreferred-stack-boundary=3 -mno-sse2, ie. 8B stack alignment
than GCC produces that error:
https://godbolt.org/z/7T8nbH

That's already a tall order of constraints, so it's understandable
that the compiler would just error likely during instruction
selection, but was eventually taught how to solve such constraints.

> >
> > While GPF observed on clang builds seem to be fixed.

Thanks for the report.  Your testing these patches is invaluable, Shirish!

>
> Ok, so it seems that gcc insists on having at least 2^4 bytes stack
> alignment when
> SSE is enabled on x86-64, but does not actually rely on that for
> correct operation
> unless it's using sse2. So -msse always has to be paired with
>  -mpreferred-stack-boundary=3.

Seemingly only for older versions of GCC, pre 7.1.

>
> For clang, it sounds like the opposite is true: when passing 16 byte
> stack alignment
> and having sse/sse2 enabled, it requires the incoming stack to be 16
> byte aligned,

I don't think it requires the incoming stack to be 16B aligned for
sse2, I think it requires the incoming and current stack alignment to
match. Today it does not, which is why we observe GPFs.

> but passing 8 byte alignment makes it do the right thing.
>
> So, should we just always pass $(call cc-option, -mpreferred-stack-boundary=4)
> to get the desired outcome on both?

Hmmm...I would have liked to remove it outright, as it is an ABI
mismatch that is likely to result in instability and non-fun-to-debug
runtime issues in the future.  I suspect my patch does work for GCC
7.1+.  The question is: Do we want to either:
1. mark AMDGPU broken for GCC < 7.1, or
2. continue supporting it via stack alignment mismatch?

2 is brittle, and may break at any point in the future, but if it's
working for someone it does make me feel bad to outright disable it.
What I'd image 2 looks like is (psuedo code in a Makefile):

if CC_IS_GCC && GCC_VERSION < 7.1:
  set stack alignment to 16B and hope for the best

So my diff would be amended to keep the stack alignment flags, but
only to support GCC < 7.1.  And that assumes my change compiles with
GCC 7.1+. (Looks like it does for me locally with GCC 8.3, but I would
feel even more confident if someone with hardware to test on and GCC
7.1+ could boot test).
-- 
Thanks,
~Nick Desaulniers
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

AMDGPU and 16B stack alignment

[Nick Desaulniers]

Hello!

The x86 kernel is compiled with an 8B stack alignment via
`-mpreferred-stack-boundary=3` for GCC since 3.6-rc1 via
commit d9b0cde91c60 ("x86-64, gcc: Use
-mpreferred-stack-boundary=3 if supported")
or `-mstack-alignment=8` for Clang. Parts of the AMDGPU driver are
compiled with 16B stack alignment.

Generally, the stack alignment is part of the ABI. Linking together two
different translation units with differing stack alignment is dangerous,
particularly when the translation unit with the smaller stack alignment
makes calls into the translation unit with the larger stack alignment.
While 8B aligned stacks are sometimes also 16B aligned, they are not
always.

Multiple users have reported General Protection Faults (GPF) when using
the AMDGPU driver compiled with Clang. Clang is placing objects in stack
slots assuming the stack is 16B aligned, and selecting instructions that
require 16B aligned memory operands. At runtime, syscalls handling 8B
stack aligned code calls into code that assumes 16B stack alignment.
When the stack is a multiple of 8B but not 16B, these instructions
result in a GPF.

GCC doesn't select instructions with alignment requirements, so the GPFs
aren't observed, but it is still considered an ABI breakage to mix and
match stack alignment.

I have patches that basically remove -mpreferred-stack-boundary=4 and
-mstack-alignment=16 from AMDGPU:
https://github.com/ClangBuiltLinux/linux/issues/735#issuecomment-541247601
Yuxuan has tested with Clang and GCC and reported it fixes the GPF's observed.

I've split the patch into 4; same commit message but different Fixes
tags so that they backport to stable on finer granularity. 2 questions
BEFORE I send the series:

1. Would you prefer 4 patches with unique `fixes` tags, or 1 patch?
2. Was there or is there still a good reason for the stack alignment mismatch?

(Further, I think we can use -msse2 for BOTH clang+gcc after my patch,
but I don't have hardware to test on. I'm happy to write/send the
follow up patch, but I'd need help testing).
-- 
Thanks,
~Nick Desaulniers

Re: [PATCH 4/6] drm/amd/display: fix dcn21 Makefile for clang

[Nick Desaulniers]

On Wed, Oct 2, 2019 at 2:24 PM Alex Deucher  wrote:
>
> On Wed, Oct 2, 2019 at 5:19 PM Nick Desaulniers  
> wrote:
> >
> > Alex, do you know why the AMDGPU driver uses a different stack
> > alignment (16B) than the rest of the x86 kernel?  (see
> > arch/x86/Makefile which uses 8B stack alignment).
>
> Not sure.  Maybe Harry can comment.  I think it was added for the
> floating point stuff.  Not sure if it's strictly required or not.

Can you find out for me please who knows more about this and setup a
chat with all of us? (I don't want to deride this patch's review
thread, so let's start a new thread once we know more) We're facing
some interesting runtime issues when built with Clang.

-- 
Thanks,
~Nick Desaulniers

Re: [PATCH 4/6] drm/amd/display: fix dcn21 Makefile for clang

[Nick Desaulniers]

On Wed, Oct 2, 2019 at 5:03 AM Arnd Bergmann  wrote:
>
> Just like all the other variants, this one passes invalid
> compile-time options with clang after the new code got
> merged:
>
> clang: error: unknown argument: '-mpreferred-stack-boundary=4'
> scripts/Makefile.build:265: recipe for target 
> 'drivers/gpu/drm/amd/amdgpu/../display/dc/dcn21/dcn21_resource.o' failed
>
> Use the same variant that we have for dcn20 to fix compilation.
>
> Fixes: eced51f9babb ("drm/amd/display: Add hubp block for Renoir (v2)")
> Signed-off-by: Arnd Bergmann 

Thanks for the patch!
Reviewed-by: Nick Desaulniers 
Tested-by: Nick Desaulniers 
(Though I think it's already been merged)

Alex, do you know why the AMDGPU driver uses a different stack
alignment (16B) than the rest of the x86 kernel?  (see
arch/x86/Makefile which uses 8B stack alignment).

> ---
>  drivers/gpu/drm/amd/display/dc/dcn21/Makefile | 12 +++-
>  1 file changed, 11 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/amd/display/dc/dcn21/Makefile 
> b/drivers/gpu/drm/amd/display/dc/dcn21/Makefile
> index 8cd9de8b1a7a..ef673bffc241 100644
> --- a/drivers/gpu/drm/amd/display/dc/dcn21/Makefile
> +++ b/drivers/gpu/drm/amd/display/dc/dcn21/Makefile
> @@ -3,7 +3,17 @@
>
>  DCN21 = dcn21_hubp.o dcn21_hubbub.o dcn21_resource.o
>
> -CFLAGS_$(AMDDALPATH)/dc/dcn21/dcn21_resource.o := -mhard-float -msse 
> -mpreferred-stack-boundary=4
> +ifneq ($(call cc-option, -mpreferred-stack-boundary=4),)
> +   cc_stack_align := -mpreferred-stack-boundary=4
> +else ifneq ($(call cc-option, -mstack-alignment=16),)
> +   cc_stack_align := -mstack-alignment=16
> +endif
> +
> +CFLAGS_$(AMDDALPATH)/dc/dcn21/dcn21_resource.o := -mhard-float -msse 
> $(cc_stack_align)
> +
> +ifdef CONFIG_CC_IS_CLANG
> +CFLAGS_$(AMDDALPATH)/dc/dcn21/dcn21_resource.o += -msse2
> +endif
>
>  AMD_DAL_DCN21 = $(addprefix $(AMDDALPATH)/dc/dcn21/,$(DCN21))
>
> --
> 2.20.0
>
> --
> You received this message because you are subscribed to the Google Groups 
> "Clang Built Linux" group.
> To unsubscribe from this group and stop receiving emails from it, send an 
> email to clang-built-linux+unsubscr...@googlegroups.com.
> To view this discussion on the web visit 
> https://groups.google.com/d/msgid/clang-built-linux/20191002120136.1777161-5-arnd%40arndb.de.



-- 
Thanks,
~Nick Desaulniers

Re: [PATCH 6/6] [RESEND] drm/amdgpu: work around llvm bug #42576

[Nick Desaulniers]

On Wed, Oct 2, 2019 at 10:07 AM Nathan Chancellor
 wrote:
>
> On Wed, Oct 02, 2019 at 09:51:37AM -0700, 'Nick Desaulniers' via Clang Built 
> Linux wrote:
> > > Apparently this bug is still present in both the released clang-9
> > > and the current development version of clang-10.
> > > I was hoping we would not need a workaround in clang-9+, but
> > > it seems that we do.

Here's a fix: https://reviews.llvm.org/D68356
Can't possibly land until clang-10 though.

> >
> > I think I'd rather:
> > 1. mark AMDGPU BROKEN if CC_IS_CLANG. There are numerous other issues 
> > building
> >a working driver here.
>
> The only reason I am not thrilled about this is we will lose out on
> warning coverage while the compiler bug gets fixed. I think the AMDGPU
> drivers have been the single biggest source of clang warnings.
>
> I think something like:
>
> depends on CC_IS_GCC || (CC_IS_CLANG && COMPILE_TEST)
>
> would end up avoiding the runtime issues and give us warning coverage.
> The only issue is that we would still need this patch...
>
> Cheers,
> Nathan



-- 
Thanks,
~Nick Desaulniers

Re: [PATCH 6/6] [RESEND] drm/amdgpu: work around llvm bug #42576

[Nick Desaulniers]

> Apparently this bug is still present in both the released clang-9
> and the current development version of clang-10.
> I was hoping we would not need a workaround in clang-9+, but
> it seems that we do.

I think I'd rather:
1. mark AMDGPU BROKEN if CC_IS_CLANG. There are numerous other issues building
   a working driver here.
2. Fix the compiler bug.

Re: [PATCH] drm/amd/display: build failed for DCN2.1

[Nick Desaulniers]

On Mon, Sep 16, 2019 at 2:03 AM Xinpeng Liu  wrote:
>
> drivers/gpu/drm/amd/amdgpu/../display/dc/dml/Makefile:70: *** missing
> `endif'.  Stop.
> make[4]: *** [drivers/gpu/drm/amd/amdgpu] Error 2
>
> Signed-off-by: Xinpeng Liu 

Tested-by: Nick Desaulniers 

+ Mark
I think this was a result of the resolved merge conflict.  See the
-next only commit titled:
Merge remote-tracking branch 'drm/drm-next'


> ---
>  drivers/gpu/drm/amd/display/dc/dml/Makefile | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/drivers/gpu/drm/amd/display/dc/dml/Makefile 
> b/drivers/gpu/drm/amd/display/dc/dml/Makefile
> index a2eb59e..5b2a65b 100644
> --- a/drivers/gpu/drm/amd/display/dc/dml/Makefile
> +++ b/drivers/gpu/drm/amd/display/dc/dml/Makefile
> @@ -44,6 +44,7 @@ CFLAGS_$(AMDDALPATH)/dc/dml/dcn20/display_mode_vba_20.o := 
> $(dml_ccflags)
>  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_rq_dlg_calc_20v2.o := 
> $(dml_ccflags)
> +endif
>  ifdef CONFIG_DRM_AMD_DC_DCN2_1
>  CFLAGS_$(AMDDALPATH)/dc/dml/dcn21/display_mode_vba_21.o := $(dml_ccflags)
>  CFLAGS_$(AMDDALPATH)/dc/dml/dcn21/display_rq_dlg_calc_21.o := $(dml_ccflags)
> --
> 1.8.3.1
>


-- 
Thanks,
~Nick Desaulniers
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH] drm/amd/display: Support clang option for stack alignment

[Nick Desaulniers]

On Fri, Jul 12, 2019 at 2:37 AM Arnd Bergmann  wrote:
>
> As previously fixed for dml in commit 4769278e5c7f ("amdgpu/dc/dml:
> Support clang option for stack alignment") and calcs in commit
> cc32ad8f559c ("amdgpu/dc/calcs: Support clang option for stack
> alignment"), dcn20 uses an option that is not available with clang:
>
> clang: error: unknown argument: '-mpreferred-stack-boundary=4'
> scripts/Makefile.build:281: recipe for target 
> 'drivers/gpu/drm/amd/amdgpu/../display/dc/dcn20/dcn20_resource.o' failed
>
> Use the same trick that we have in the other two files.

Maybe time for a macro in Kbuild.include or some such, if we see this
pattern being repeated?

>
> Fixes: 7ed4e6352c16 ("drm/amd/display: Add DCN2 HW Sequencer and Resource")
> Signed-off-by: Arnd Bergmann 
> ---
>  drivers/gpu/drm/amd/display/dc/dcn20/Makefile |  8 +++-
>  drivers/gpu/drm/amd/display/dc/dsc/Makefile   | 16 
>  2 files changed, 19 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/display/dc/dcn20/Makefile 
> b/drivers/gpu/drm/amd/display/dc/dcn20/Makefile
> index 1b68de27ba74..e9721a906592 100644
> --- a/drivers/gpu/drm/amd/display/dc/dcn20/Makefile
> +++ b/drivers/gpu/drm/amd/display/dc/dcn20/Makefile
> @@ -10,7 +10,13 @@ ifdef CONFIG_DRM_AMD_DC_DSC_SUPPORT
>  DCN20 += dcn20_dsc.o
>  endif
>
> -CFLAGS_dcn20_resource.o := -mhard-float -msse -mpreferred-stack-boundary=4
> +ifneq ($(call cc-option, -mpreferred-stack-boundary=4),)
> +   cc_stack_align := -mpreferred-stack-boundary=4
> +else ifneq ($(call cc-option, -mstack-alignment=16),)
> +   cc_stack_align := -mstack-alignment=16
> +endif
> +
> +CFLAGS_dcn20_resource.o := -mhard-float -msse $(cc_stack_align)
>
>  AMD_DAL_DCN20 = $(addprefix $(AMDDALPATH)/dc/dcn20/,$(DCN20))
>
> diff --git a/drivers/gpu/drm/amd/display/dc/dsc/Makefile 
> b/drivers/gpu/drm/amd/display/dc/dsc/Makefile
> index c5d5b94e2604..e019cd9447e8 100644
> --- a/drivers/gpu/drm/amd/display/dc/dsc/Makefile
> +++ b/drivers/gpu/drm/amd/display/dc/dsc/Makefile
> @@ -1,10 +1,18 @@
>  #
>  # Makefile for the 'dsc' sub-component of DAL.
>
> -CFLAGS_rc_calc.o := -mhard-float -msse -mpreferred-stack-boundary=4
> -CFLAGS_rc_calc_dpi.o := -mhard-float -msse -mpreferred-stack-boundary=4
> -CFLAGS_codec_main_amd.o := -mhard-float -msse -mpreferred-stack-boundary=4
> -CFLAGS_dc_dsc.o := -mhard-float -msse -mpreferred-stack-boundary=4
> +ifneq ($(call cc-option, -mpreferred-stack-boundary=4),)
> +   cc_stack_align := -mpreferred-stack-boundary=4
> +else ifneq ($(call cc-option, -mstack-alignment=16),)
> +   cc_stack_align := -mstack-alignment=16
> +endif
> +
> +dsc_ccflags := -mhard-float -msse $(cc_stack_align)
> +
> +CFLAGS_rc_calc.o := $(dsc_ccflags)
> +CFLAGS_rc_calc_dpi.o := $(dsc_ccflags)
> +CFLAGS_codec_main_amd.o := $(dsc_ccflags)
> +CFLAGS_dc_dsc.o := $(dsc_ccflags)
-- 
Thanks,
~Nick Desaulniers

Re: Clang warning in drivers/gpu/drm/amd/amdgpu/amdgpu_ras.c

[Nick Desaulniers]

On Wed, Mar 20, 2019 at 2:37 AM Koenig, Christian
 wrote:
>
> Am 20.03.19 um 05:34 schrieb Nathan Chancellor:
> > On Wed, Mar 20, 2019 at 01:31:27AM +, Pan, Xinhui wrote:
> >> these two enumerated types are same for now. both of them might change in 
> >> the future.

Please consider if the YAGNI principle applies here.
https://martinfowler.com/bliki/Yagni.html

> >>
> >> I have not used clang, but would  .block_id =  (int)head->block fix your 
> >> warning? If such change is acceptable, I can make one then.

My preference on solutions, in order:
1. One enum (this is the simplest most type safe solution).  Add
another enum when you actually need it.  Otherwise, YAGNI.
2. Safe casting function (like the one Nathan supplied, maybe with
WARN_ONCE rather than WARN).  This ensures that at least if the types
diverge you get a runtime warning.  A compile-time warning would be
preferred, but I haven't taken the time to think through how that
might be implemented.
3. Cast to int (this has been used in other places throughout the
kernel, but provides the weakest type safety and doesn't catch future
divergence).
4. Disabling the warning. (I almost never advocate for this).

> Another question is if it is such a good idea to just silence the warning?

For the kernel, it seems that each maintainer can choose what to apply
to their subsystem.  I would recommend against disabling additional
warnings that aren't disable kernel-wide for most cases.
-Wenum-conversion has spotted many bugs.  While the enums in question
today are not different, they MIGHT eventually diverge and lead to
bugs, like the others we've found and fixed throughout the kernel.  So
I would recommend fixing now, and be insulated in the future.

-- 
Thanks,
~Nick Desaulniers
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH] drm/amd/powerplay: Zero initialize num_of_levels in vega20_set_single_dpm_table

[Nick Desaulniers]

On Tue, Mar 19, 2019 at 6:00 PM Nathan Chancellor
 wrote:
>
> When building with -Wsometimes-uninitialized, Clang warns:
>
> drivers/gpu/drm/amd/amdgpu/../powerplay/vega20_ppt.c:456:2: warning:
> variable 'num_of_levels' is used uninitialized whenever '?:' condition
> is false [-Wsometimes-uninitialized]
> smu_read_smc_arg(smu, _of_levels);
> ^
> drivers/gpu/drm/amd/amdgpu/../powerplay/inc/amdgpu_smu.h:608:3: note:
> expanded from macro 'smu_read_smc_arg'
> ((smu)->funcs->read_smc_arg? (smu)->funcs->read_smc_arg((smu), (arg)) 
> : 0)
>  ^~
> drivers/gpu/drm/amd/amdgpu/../powerplay/vega20_ppt.c:457:7: note:
> uninitialized use occurs here
> if (!num_of_levels) {
>  ^
> drivers/gpu/drm/amd/amdgpu/../powerplay/vega20_ppt.c:456:2: note: remove
> the '?:' if its condition is always true
> smu_read_smc_arg(smu, _of_levels);
> ^
> drivers/gpu/drm/amd/amdgpu/../powerplay/inc/amdgpu_smu.h:608:3: note:
> expanded from macro 'smu_read_smc_arg'
> ((smu)->funcs->read_smc_arg? (smu)->funcs->read_smc_arg((smu), (arg)) 
> : 0)
>  ^
> drivers/gpu/drm/amd/amdgpu/../powerplay/vega20_ppt.c:446:27: note:
> initialize the variable 'num_of_levels' to silence this warning
> uint32_t i, num_of_levels, clk;
>  ^
>   = 0
> 1 warning generated.
>
> The if statement it mentions as potentially problematic is currently
> always true because the read_smc_arg callback is assigned at the
> bottom of this file but Clang can't tell that. If the callback were
> ever to disappear, num_of_levels would never be initialized. Just
> zero initialize it to ensure that the intent behind this code
> remains the same.

Thanks for the simple fix.
Reviewed-by: Nick Desaulniers 

>
> Fixes: 870b996f955f ("drm/amd/powerplay: set defalut dpm table for smu")
> Link: https://github.com/ClangBuiltLinux/linux/issues/425
> Signed-off-by: Nathan Chancellor 
> ---
>  drivers/gpu/drm/amd/powerplay/vega20_ppt.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/amd/powerplay/vega20_ppt.c 
> b/drivers/gpu/drm/amd/powerplay/vega20_ppt.c
> index 7e9e8ad9a300..41e6f49c9cb6 100644
> --- a/drivers/gpu/drm/amd/powerplay/vega20_ppt.c
> +++ b/drivers/gpu/drm/amd/powerplay/vega20_ppt.c
> @@ -443,7 +443,7 @@ vega20_set_single_dpm_table(struct smu_context *smu,
> PPCLK_e clk_id)
>  {
> int ret = 0;
> -   uint32_t i, num_of_levels, clk;
> +   uint32_t i, num_of_levels = 0, clk;
>
> ret = smu_send_smc_msg_with_param(smu,
> SMU_MSG_GetDpmFreqByIndex,
> --
> 2.21.0
>


-- 
Thanks,
~Nick Desaulniers
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH] drm/amd/display: add -msse2 to prevent Clang from emitting libcalls to undefined SW FP routines

[Nick Desaulniers]

Suggestions:
1. revert patch
2. get me the disassembly from gcc of the translation unit in question.
3. land patch that adds clang guards or something different based on 2.

Revert first; ask questions later.

On Tue, Jan 29, 2019 at 11:01 AM Wentland, Harry  wrote:
>
> On 2019-01-29 1:56 p.m., Guenter Roeck wrote:
> > On Tue, Jan 29, 2019 at 10:30:31AM -0500, Alex Deucher wrote:
> >> On Fri, Jan 25, 2019 at 10:29 AM Wentland, Harry  
> >> wrote:
> >>>
> >>> On 2019-01-24 7:52 p.m., ndesaulni...@google.com wrote:
> >>>> arch/x86/Makefile disables SSE and SSE2 for the whole kernel.  The
> >>>> AMDGPU drivers modified in this patch re-enable SSE but not SSE2.  Turn
> >>>> on SSE2 to support emitting double precision floating point instructions
> >>>> rather than calls to non-existent (usually available from gcc_s or
> >>>> compiler_rt) floating point helper routines.
> >>>>
> >>>> Link: 
> >>>> https://gcc.gnu.org/onlinedocs/gccint/Soft-float-library-routines.html
> >>>> Link: https://github.com/ClangBuiltLinux/linux/issues/327
> >>>> Cc: sta...@vger.kernel.org # 4.19
> >>>> Reported-by: S, Shirish 
> >>>> Reported-by: Matthias Kaehlcke 
> >>>> Suggested-by: James Y Knight 
> >>>> Suggested-by: Nathan Chancellor 
> >>>> Signed-off-by: Nick Desaulniers 
> >>>> Tested-by: Guenter Roeck 
> >>>
> >>> Reviewed-by: Harry Wentland 
> >>>
> >>> and applied.
> >>>
> >>
> >> This patch causes a segfault:
> >> https://bugs.freedesktop.org/show_bug.cgi?id=109487
> >>
> >> Any ideas?
> >>
> >
> > Set -msse2 only for clang ? I suspect, though, that this might only
> > solve the compile problem. If I understand correctly, the first
> > warning in the log is due to BREAK_TO_DEBUGGER(), suggesting that
> > something is seriously wrong. Maybe enabling sse2 results in different
> > results from floating point operations.
> >
> > Unfortunately I don't have a system with the affected GPU,
> > so I can't run any tests on real hardware. Unless someone can test
> > with real hardware, I think we have no choice but to revert the
> > patch.
> >
>
> Might be a good idea. Even though, best to revert for now until we understand 
> what's going on.
>
> It seems like people at AMD building with gcc 5.4 are fine, but those using 
> gcc 7.3 or 8.2 experience the crash.
>
> Harry
>
> > Guenter
> >
> >> Alex
> >>
> >>> Harry
> >>>
> >>>> ---
> >>>>  drivers/gpu/drm/amd/display/dc/calcs/Makefile | 2 +-
> >>>>  drivers/gpu/drm/amd/display/dc/dml/Makefile   | 2 +-
> >>>>  2 files changed, 2 insertions(+), 2 deletions(-)
> >>>>
> >>>> diff --git a/drivers/gpu/drm/amd/display/dc/calcs/Makefile 
> >>>> b/drivers/gpu/drm/amd/display/dc/calcs/Makefile
> >>>> index 95f332ee3e7e..dc85a3c088af 100644
> >>>> --- a/drivers/gpu/drm/amd/display/dc/calcs/Makefile
> >>>> +++ b/drivers/gpu/drm/amd/display/dc/calcs/Makefile
> >>>> @@ -30,7 +30,7 @@ else ifneq ($(call cc-option, -mstack-alignment=16),)
> >>>>   cc_stack_align := -mstack-alignment=16
> >>>>  endif
> >>>>
> >>>> -calcs_ccflags := -mhard-float -msse $(cc_stack_align)
> >>>> +calcs_ccflags := -mhard-float -msse -msse2 $(cc_stack_align)
> >>>>
> >>>>  CFLAGS_dcn_calcs.o := $(calcs_ccflags)
> >>>>  CFLAGS_dcn_calc_auto.o := $(calcs_ccflags)
> >>>> diff --git a/drivers/gpu/drm/amd/display/dc/dml/Makefile 
> >>>> b/drivers/gpu/drm/amd/display/dc/dml/Makefile
> >>>> index d97ca6528f9d..33c7d7588712 100644
> >>>> --- a/drivers/gpu/drm/amd/display/dc/dml/Makefile
> >>>> +++ b/drivers/gpu/drm/amd/display/dc/dml/Makefile
> >>>> @@ -30,7 +30,7 @@ else ifneq ($(call cc-option, -mstack-alignment=16),)
> >>>>   cc_stack_align := -mstack-alignment=16
> >>>>  endif
> >>>>
> >>>> -dml_ccflags := -mhard-float -msse $(cc_stack_align)
> >>>> +dml_ccflags := -mhard-float -msse -msse2 $(cc_stack_align)
> >>>>
> >>>>  CFLAGS_display_mode_lib.o := $(dml_ccflags)
> >>>>  CFLAGS_display_pipe_clocks.o := $(dml_ccflags)
> >>>>
> >>> ___
> >>> amd-gfx mailing list
> >>> amd-gfx@lists.freedesktop.org
> >>> https://lists.freedesktop.org/mailman/listinfo/amd-gfx



-- 
Thanks,
~Nick Desaulniers
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [Linux-v4.18-rc6] modpost-errors when compiling with clang-7 and CONFIG_DRM_AMDGPU=m

[Nick Desaulniers]

On Fri, Jan 25, 2019 at 8:05 AM Sedat Dilek  wrote:
>
> On Fri, Jan 25, 2019 at 4:15 PM Wentland, Harry  
> wrote:
> >
> > On 2019-01-25 3:25 a.m., Koenig, Christian wrote:
> > > Am 25.01.19 um 08:42 schrieb Sedat Dilek:
> > >> [ CC Nick D. ]
> > >>
> > >> Hi Christian,
> > >>
> > >> Nick has posted a fix in [2].
> > >> Is that OK for you?
> > >
> > > I've seen that and yeah I'm perfectly fine with it, but Harry is the one
> > > who needs to decide.
> > >
> >
> > Looks good to me.
> >
> > Reviewed-by: Harry Wentland 
> >
> > And applied.
> >
>
> Thanks.
>
> Can you point me to the commit and Git tree?

I suspect the tree it will be pushed to is:
https://cgit.freedesktop.org/~agd5f/linux/
But I suspect it has not been pushed yet.

-- 
Thanks,
~Nick Desaulniers
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH] drm/amd/display: Pass app_tf by value rather than by reference

[Nick Desaulniers]

On Tue, Dec 11, 2018 at 2:24 PM Nathan Chancellor 
wrote:

> On Tue, Dec 11, 2018 at 02:07:31PM -0800, Nick Desaulniers wrote:
> > On Tue, Dec 11, 2018 at 1:42 PM Nathan Chancellor
> >  wrote:
> > >
> > > On Tue, Dec 11, 2018 at 01:25:00PM -0800, Nick Desaulniers wrote:
> > > > On Mon, Dec 10, 2018 at 3:42 PM Nathan Chancellor
> > > >  wrote:
> > > > >
> > > > > Clang warns when an expression that equals zero is used as a null
> > > > > pointer constant (in lieu of NULL):
> > > > >
> > > > > drivers/gpu/drm/amd/amdgpu/../display/amdgpu_dm/amdgpu_dm.c:4435:3:
> > > > > warning: expression which evaluates to zero treated as a null
> pointer
> > > > > constant of type 'const enum color_transfer_func *'
> > > > > [-Wnon-literal-null-conversion]
> > > > > TRANSFER_FUNC_UNKNOWN,
> > > > > ^
> > > > > 1 warning generated.
> > > > >
> > > > > This warning is caused by commit bb47de736661 ("drm/amdgpu: Set
> FreeSync
> > > > > state using drm VRR properties") and it could be solved by using
> NULL
> > > > > instead of TRANSFER_FUNC_UNKNOWN or casting TRANSFER_FUNC_UNKNOWN
> as a
> > > > > pointer. However, after looking into it, there doesn't appear to
> be a
> > > > > good reason to pass app_tf by reference as it is never mutated
> along the
> > > > > way. This is the only code path in which app_tf is used:
> > > > >
> > > > > mod_freesync_build_vrr_infopacket ->
> > > > > build_vrr_infopacket_v2 ->
> > > > > build_vrr_infopacket_fs2_data
> > > > >
> > > > > Neither mod_freesync_build_vrr_infopacket or
> build_vrr_infopacket_v2
> > > > > modify app_tf's value and build_vrr_infopacket_fs2_data expects
> just
> > > > > the value so we can avoid dereferencing anything by just passing in
> > > > > app_tf's value to mod_freesync_build_vrr_infopacket and
> > > > > build_vrr_infopacket_v2.
> > > > >
> > > > > There is no functional change because build_vrr_infopacket_fs2_data
> > > > > doesn't do anything if TRANSFER_FUNC_UNKNOWN is passed to it, the
> same
> > > > > as not calling build_vrr_infopacket_fs2_data at all like before
> this
> > > > > change when NULL was used for app_tf.
> > > >
> > > > Nathan,
> > > > Thanks for sending this patch.  I was hoping to provide review
> sooner,
> > > > but have been quite busy lately.
> > > >
> > >
> > > Late review is better than no review, I appeciate you taking the time
> to
> > > do this!
> > >
> > > > Yeah, especially for LP64 targets, the pointer to enum is larger than
> > > > just the enum, and if it's not being updated ("in/out paramter")
> > > > there's no need to pass by pointer.
> > > >
> > >
> > > Thanks for confirming!
> > >
> > > > >
> > > > > Signed-off-by: Nathan Chancellor 
> > > > > ---
> > > > >  drivers/gpu/drm/amd/display/modules/freesync/freesync.c | 7
> +++
> > > > >  drivers/gpu/drm/amd/display/modules/inc/mod_freesync.h  | 2 +-
> > > > >  2 files changed, 4 insertions(+), 5 deletions(-)
> > > > >
> > > > > diff --git
> a/drivers/gpu/drm/amd/display/modules/freesync/freesync.c
> b/drivers/gpu/drm/amd/display/modules/freesync/freesync.c
> > > > > index 620a171620ee..520665a9d81a 100644
> > > > > --- a/drivers/gpu/drm/amd/display/modules/freesync/freesync.c
> > > > > +++ b/drivers/gpu/drm/amd/display/modules/freesync/freesync.c
> > > > > @@ -656,7 +656,7 @@ static void build_vrr_infopacket_v1(enum
> signal_type signal,
> > > > >
> > > > >  static void build_vrr_infopacket_v2(enum signal_type signal,
> > > > > const struct mod_vrr_params *vrr,
> > > > > -   const enum color_transfer_func *app_tf,
> > > > > +   enum color_transfer_func app_tf,
> > > > > struct dc_info_packet *infopacket)
> > > > >  {
> > > > > unsigned int payload_size = 0;
> > > > > @@ -664,8 +664,7 @@ static void build_vrr_infopacket_v2(enum
> signal_ty

Re: [PATCH] drm/amd/display: Pass app_tf by value rather than by reference

[Nick Desaulniers]

On Tue, Dec 11, 2018 at 1:42 PM Nathan Chancellor
 wrote:
>
> On Tue, Dec 11, 2018 at 01:25:00PM -0800, Nick Desaulniers wrote:
> > On Mon, Dec 10, 2018 at 3:42 PM Nathan Chancellor
> >  wrote:
> > >
> > > Clang warns when an expression that equals zero is used as a null
> > > pointer constant (in lieu of NULL):
> > >
> > > drivers/gpu/drm/amd/amdgpu/../display/amdgpu_dm/amdgpu_dm.c:4435:3:
> > > warning: expression which evaluates to zero treated as a null pointer
> > > constant of type 'const enum color_transfer_func *'
> > > [-Wnon-literal-null-conversion]
> > > TRANSFER_FUNC_UNKNOWN,
> > > ^
> > > 1 warning generated.
> > >
> > > This warning is caused by commit bb47de736661 ("drm/amdgpu: Set FreeSync
> > > state using drm VRR properties") and it could be solved by using NULL
> > > instead of TRANSFER_FUNC_UNKNOWN or casting TRANSFER_FUNC_UNKNOWN as a
> > > pointer. However, after looking into it, there doesn't appear to be a
> > > good reason to pass app_tf by reference as it is never mutated along the
> > > way. This is the only code path in which app_tf is used:
> > >
> > > mod_freesync_build_vrr_infopacket ->
> > > build_vrr_infopacket_v2 ->
> > > build_vrr_infopacket_fs2_data
> > >
> > > Neither mod_freesync_build_vrr_infopacket or build_vrr_infopacket_v2
> > > modify app_tf's value and build_vrr_infopacket_fs2_data expects just
> > > the value so we can avoid dereferencing anything by just passing in
> > > app_tf's value to mod_freesync_build_vrr_infopacket and
> > > build_vrr_infopacket_v2.
> > >
> > > There is no functional change because build_vrr_infopacket_fs2_data
> > > doesn't do anything if TRANSFER_FUNC_UNKNOWN is passed to it, the same
> > > as not calling build_vrr_infopacket_fs2_data at all like before this
> > > change when NULL was used for app_tf.
> >
> > Nathan,
> > Thanks for sending this patch.  I was hoping to provide review sooner,
> > but have been quite busy lately.
> >
>
> Late review is better than no review, I appeciate you taking the time to
> do this!
>
> > Yeah, especially for LP64 targets, the pointer to enum is larger than
> > just the enum, and if it's not being updated ("in/out paramter")
> > there's no need to pass by pointer.
> >
>
> Thanks for confirming!
>
> > >
> > > Signed-off-by: Nathan Chancellor 
> > > ---
> > >  drivers/gpu/drm/amd/display/modules/freesync/freesync.c | 7 +++
> > >  drivers/gpu/drm/amd/display/modules/inc/mod_freesync.h  | 2 +-
> > >  2 files changed, 4 insertions(+), 5 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/amd/display/modules/freesync/freesync.c 
> > > b/drivers/gpu/drm/amd/display/modules/freesync/freesync.c
> > > index 620a171620ee..520665a9d81a 100644
> > > --- a/drivers/gpu/drm/amd/display/modules/freesync/freesync.c
> > > +++ b/drivers/gpu/drm/amd/display/modules/freesync/freesync.c
> > > @@ -656,7 +656,7 @@ static void build_vrr_infopacket_v1(enum signal_type 
> > > signal,
> > >
> > >  static void build_vrr_infopacket_v2(enum signal_type signal,
> > > const struct mod_vrr_params *vrr,
> > > -   const enum color_transfer_func *app_tf,
> > > +   enum color_transfer_func app_tf,
> > > struct dc_info_packet *infopacket)
> > >  {
> > > unsigned int payload_size = 0;
> > > @@ -664,8 +664,7 @@ static void build_vrr_infopacket_v2(enum signal_type 
> > > signal,
> > > build_vrr_infopacket_header_v2(signal, infopacket, _size);
> > > build_vrr_infopacket_data(vrr, infopacket);
> > >
> > > -   if (app_tf != NULL)
> > > -   build_vrr_infopacket_fs2_data(*app_tf, infopacket);
> > > +   build_vrr_infopacket_fs2_data(app_tf, infopacket);
> > >
> > > build_vrr_infopacket_checksum(_size, infopacket);
> > >
> > > @@ -676,7 +675,7 @@ void mod_freesync_build_vrr_infopacket(struct 
> > > mod_freesync *mod_freesync,
> > > const struct dc_stream_state *stream,
> > > const struct mod_vrr_params *vrr,
> > > enum vrr_packet_type packet_type,
> > > -   const enum color_transfer_func *app_tf,
> > > +   enum color_transfer_func app_

Re: [PATCH] drm/amd/display: Pass app_tf by value rather than by reference

[Nick Desaulniers]

L)` could be replaced with
`if (app_tf != transfer_func_unknown)` hoisted from
`build_vrr_infopacket_fs2_data`? (There's only one caller of
`build_vrr_infopacket_fs2_data` today, maybe fine to leave the
unconditional call and check).

Either way, I suspect without the change to
drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c, that this fails to
compile?
-- 
Thanks,
~Nick Desaulniers
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH v2] drm/amd/display: Use proper enums in process_channel_reply

[Nick Desaulniers]

On Thu, Sep 27, 2018 at 11:08 AM Nathan Chancellor
 wrote:
>
> On Thu, Sep 27, 2018 at 11:06:33AM -0700, Nathan Chancellor wrote:
> > Clang warns when one enumerated type is implicitly converted to another.
> >
> > drivers/gpu/drm/amd/amdgpu/../display/dc/dce/dce_aux.c:315:19: warning:
> > implicit conversion from enumeration type 'enum
> > aux_channel_operation_result' to different enumeration type 'enum
> > aux_transaction_reply' [-Wenum-conversion]
> > reply->status = AUX_CHANNEL_OPERATION_FAILED_HPD_DISCON;
> >   ~ ^~~
> > drivers/gpu/drm/amd/amdgpu/../display/dc/i2caux/dce110/aux_engine_dce110.c:349:19:
> > warning: implicit conversion from enumeration type 'enum
> > aux_channel_operation_result' to different enumeration type 'enum
> > aux_transaction_reply' [-Wenum-conversion]
> > reply->status = AUX_CHANNEL_OPERATION_FAILED_HPD_DISCON;
> >   ~ ^~~
> >
> > The current enum is incorrect, it should be from aux_transaction_reply,
> > so use AUX_TRANSACTION_REPLY_HPD_DISCON.
> >
> > Reported-by: Nick Desaulniers 
> > Suggested-by: Nick Desaulniers 
> > Signed-off-by: Nathan Chancellor 

Reviewed-by: Nick Desaulniers 

> > ---
> >
> > v1 -> v2:
> >
> > * Rather than change status to an integer, use the proper enumerated
> >   type from aux_transaction reply as suggested by Nick and confirmed
> >   by Henry.
>
> Sigh Harry, sorry...

Thanks for the patch, Nathan.  Don't worry about sending a v3 over
this; its below the commit line so this detail wont get committed.

>
> >
> >  drivers/gpu/drm/amd/display/dc/dce/dce_aux.c| 2 +-
> >  .../gpu/drm/amd/display/dc/i2caux/dce110/aux_engine_dce110.c| 2 +-
> >  2 files changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/amd/display/dc/dce/dce_aux.c 
> > b/drivers/gpu/drm/amd/display/dc/dce/dce_aux.c
> > index 3f5b2e6f7553..aaeb7faac0c4 100644
> > --- a/drivers/gpu/drm/amd/display/dc/dce/dce_aux.c
> > +++ b/drivers/gpu/drm/amd/display/dc/dce/dce_aux.c
> > @@ -312,7 +312,7 @@ static void process_channel_reply(
> >
> >   /* in case HPD is LOW, exit AUX transaction */
> >   if ((sw_status & AUX_SW_STATUS__AUX_SW_HPD_DISCON_MASK)) {
> > - reply->status = AUX_CHANNEL_OPERATION_FAILED_HPD_DISCON;
> > + reply->status = AUX_TRANSACTION_REPLY_HPD_DISCON;
> >   return;
> >   }
> >
> > diff --git 
> > a/drivers/gpu/drm/amd/display/dc/i2caux/dce110/aux_engine_dce110.c 
> > b/drivers/gpu/drm/amd/display/dc/i2caux/dce110/aux_engine_dce110.c
> > index 8eee8ace1259..59c3ed43d609 100644
> > --- a/drivers/gpu/drm/amd/display/dc/i2caux/dce110/aux_engine_dce110.c
> > +++ b/drivers/gpu/drm/amd/display/dc/i2caux/dce110/aux_engine_dce110.c
> > @@ -346,7 +346,7 @@ static void process_channel_reply(
> >
> >   /* in case HPD is LOW, exit AUX transaction */
> >   if ((sw_status & AUX_SW_STATUS__AUX_SW_HPD_DISCON_MASK)) {
> > - reply->status = AUX_CHANNEL_OPERATION_FAILED_HPD_DISCON;
> > + reply->status = AUX_TRANSACTION_REPLY_HPD_DISCON;
> >   return;
> >   }
> >
> > --
> > 2.19.0
> >



-- 
Thanks,
~Nick Desaulniers
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH] drm/amd/display: Change status's type in aux_reply_transaction_data

[Nick Desaulniers]

On Fri, Sep 21, 2018 at 2:55 PM Nathan Chancellor
 wrote:
>
> Clang warns when one enumerated type is implicitly converted to another.
>
> drivers/gpu/drm/amd/amdgpu/../display/dc/dce/dce_aux.c:315:19: warning:
> implicit conversion from enumeration type 'enum
> aux_channel_operation_result' to different enumeration type 'enum
> aux_transaction_reply' [-Wenum-conversion]
> reply->status = AUX_CHANNEL_OPERATION_FAILED_HPD_DISCON;
>   ~ ^~~
> drivers/gpu/drm/amd/amdgpu/../display/dc/i2caux/dce110/aux_engine_dce110.c:349:19:
> warning: implicit conversion from enumeration type 'enum
> aux_channel_operation_result' to different enumeration type 'enum
> aux_transaction_reply' [-Wenum-conversion]
> reply->status = AUX_CHANNEL_OPERATION_FAILED_HPD_DISCON;
>   ~ ^~~

I think the enum is actually wrong here.  I think the correct fix would be:

- reply->status = AUX_CHANNEL_OPERATION_FAILED_HPD_DISCON;
+ reply->status = AUX_TRANSACTION_REPLY_HPD_DISCON;

The identifiers are so similar, my guess was that it was easy to mix
them up.  This looks like an actual bug to me, since the identifiers
have different values between the 2 different enums.

>
> Instead of implicitly or explicitly converting between types, just
> change status to type uint8_t (since its max size is 255) which avoids
> this construct altogether.
>
> Reported-by: Nick Desaulniers 
> Signed-off-by: Nathan Chancellor 
> ---
>  drivers/gpu/drm/amd/display/dc/dc_ddc_types.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/amd/display/dc/dc_ddc_types.h 
> b/drivers/gpu/drm/amd/display/dc/dc_ddc_types.h
> index 05c8c31d8b31..97e1d4d19263 100644
> --- a/drivers/gpu/drm/amd/display/dc/dc_ddc_types.h
> +++ b/drivers/gpu/drm/amd/display/dc/dc_ddc_types.h
> @@ -79,7 +79,7 @@ enum aux_transaction_reply {
>  };
>
>  struct aux_reply_transaction_data {
> -   enum aux_transaction_reply status;
> +   uint8_t status;
> uint32_t length;
> uint8_t *data;
>  };
> --
> 2.19.0
>


-- 
Thanks,
~Nick Desaulniers
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH] drm/amd/powerplay: Change id parameter type in pp_atomfwctrl_get_clk_information_by_clkid

[Nick Desaulniers]

On Fri, Sep 21, 2018 at 2:01 PM Nathan Chancellor
 wrote:
>
> Clang generates warnings when one enumerated type is implicitly
> converted to another.
>
> drivers/gpu/drm/amd/amdgpu/../powerplay/hwmgr/ppatomfwctrl.c:532:57:
> warning: implicit conversion from enumeration type 'enum
> atom_smu11_syspll0_clock_id' to different enumeration type 'BIOS_CLKID'
>   (aka 'enum atom_smu9_syspll0_clock_id') [-Wenum-conversion]
> if (!pp_atomfwctrl_get_clk_information_by_clkid(hwmgr,
> SMU11_SYSPLL0_SOCCLK_ID, ))

Grepping the call sites, via:
$ grep -r pp_atomfwctrl_get_clk_information_by_clkid

shows that sometimes this function is called with instances of:

enum atom_smu9_syspll0_clock_id
enum atom_smu11_syspll0_clock_id

before your patch, the function defined to take a BIOS_CLKID, which is
typedef'd to the *9* enum:

typedef enum atom_smu9_syspll0_clock_id BIOS_CLKID;

While the current values are < 256, the enums are not packed, so you
should use either int or a union of the two types.
Do the maintainers have a preference, before sending a v2?

>
> In this case, that is expected behavior. To make that clear to Clang
> without explicitly casting these values, change id's type to uint8_t
> in pp_atomfwctrl_get_clk_information_by_clkid so no conversion happens.
>
> Reported-by: Nick Desaulniers 
> Signed-off-by: Nathan Chancellor 
> ---
>  drivers/gpu/drm/amd/powerplay/hwmgr/ppatomfwctrl.c | 3 ++-
>  drivers/gpu/drm/amd/powerplay/hwmgr/ppatomfwctrl.h | 2 +-
>  2 files changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/powerplay/hwmgr/ppatomfwctrl.c 
> b/drivers/gpu/drm/amd/powerplay/hwmgr/ppatomfwctrl.c
> index d27c1c9df286..4588bddf8b33 100644
> --- a/drivers/gpu/drm/amd/powerplay/hwmgr/ppatomfwctrl.c
> +++ b/drivers/gpu/drm/amd/powerplay/hwmgr/ppatomfwctrl.c
> @@ -488,7 +488,8 @@ int pp_atomfwctrl_get_gpio_information(struct pp_hwmgr 
> *hwmgr,
> return 0;
>  }
>
> -int pp_atomfwctrl_get_clk_information_by_clkid(struct pp_hwmgr *hwmgr, 
> BIOS_CLKID id, uint32_t *frequency)
> +int pp_atomfwctrl_get_clk_information_by_clkid(struct pp_hwmgr *hwmgr,
> +  uint8_t id, uint32_t 
> *frequency)
>  {
> struct amdgpu_device *adev = hwmgr->adev;
> struct atom_get_smu_clock_info_parameters_v3_1   parameters;
> diff --git a/drivers/gpu/drm/amd/powerplay/hwmgr/ppatomfwctrl.h 
> b/drivers/gpu/drm/amd/powerplay/hwmgr/ppatomfwctrl.h
> index 22e21668c93a..fe9e8ceef50e 100644
> --- a/drivers/gpu/drm/amd/powerplay/hwmgr/ppatomfwctrl.h
> +++ b/drivers/gpu/drm/amd/powerplay/hwmgr/ppatomfwctrl.h
> @@ -236,7 +236,7 @@ int pp_atomfwctrl_get_vbios_bootup_values(struct pp_hwmgr 
> *hwmgr,
>  int pp_atomfwctrl_get_smc_dpm_information(struct pp_hwmgr *hwmgr,
> struct pp_atomfwctrl_smc_dpm_parameters *param);
>  int pp_atomfwctrl_get_clk_information_by_clkid(struct pp_hwmgr *hwmgr,
> -   BIOS_CLKID id, uint32_t *frequency);
> +   uint8_t id, uint32_t *frequency);
>
>  #endif
>
> --
> 2.19.0
>


--
Thanks,
~Nick Desaulniers
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH] drm/amdgpu: Add braces to initialize task_info subojects

[Nick Desaulniers]

On Wed, Sep 12, 2018 at 1:24 PM Richard Smith  wrote:
>
> On Wed, Sep 12, 2018 at 10:38 AM Nick Desaulniers  
> wrote:
>>
>> On Tue, Sep 11, 2018 at 5:26 PM Nathan Chancellor
>>  wrote:
>> >
>> > Clang warns if there are missing braces around a subobject
>> > initializer.
>> >
>> > drivers/gpu/drm/amd/amdgpu/gmc_v8_0.c:1447:41: warning: suggest braces
>> > around initialization of subobject [-Wmissing-braces]
>> > struct amdgpu_task_info task_info = { 0 };
>> >   ^
>> >   {}
>> > 1 warning generated.
>> >
>> > drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c:262:41: warning: suggest braces
>> > around initialization of subobject [-Wmissing-braces]
>> > struct amdgpu_task_info task_info = { 0 };
>> >       ^
>> >   {}
>> > 1 warning generated.
>> >
>> > Reported-by: Nick Desaulniers 
>> > Signed-off-by: Nathan Chancellor 
>> > ---
>> >  drivers/gpu/drm/amd/amdgpu/gmc_v8_0.c | 2 +-
>> >  drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c | 2 +-
>> >  2 files changed, 2 insertions(+), 2 deletions(-)
>> >
>> > diff --git a/drivers/gpu/drm/amd/amdgpu/gmc_v8_0.c 
>> > b/drivers/gpu/drm/amd/amdgpu/gmc_v8_0.c
>> > index 9333109b210d..968cc1b8cdff 100644
>> > --- a/drivers/gpu/drm/amd/amdgpu/gmc_v8_0.c
>> > +++ b/drivers/gpu/drm/amd/amdgpu/gmc_v8_0.c
>> > @@ -1444,7 +1444,7 @@ static int gmc_v8_0_process_interrupt(struct 
>> > amdgpu_device *adev,
>> > gmc_v8_0_set_fault_enable_default(adev, false);
>> >
>> > if (printk_ratelimit()) {
>> > -   struct amdgpu_task_info task_info = { 0 };
>> > +   struct amdgpu_task_info task_info = { { 0 } };
>>
>> Hi Nathan,
>> Thanks for this patch.  I discussed this syntax with our language
>> lawyers.  Turns out, this is not quite correct, as you're now saying
>> "initialize the first subobject to zero, but not the rest of the
>> object."  -Wmissing-field-initializers would highlight this, but it's
>> not part of -Wall.  It would be more correct to zero initialize the
>> full struct, including all of its subobjects with `= {};`.
>
>
> Sorry, I think I've caused some confusion here.
>
> Elements with an omitted initializer get implicitly zero-initialized. In C++, 
> it's idiomatic to write `= {}` to perform aggregate zero-initialization, but 
> in C, that's invalid because at least one initializer is syntactically 
> required within the braces. As a result, `= {0}` is an idiomatic way to 
> perform zero-initialization of an aggregate in C.

That doesn't seem to be the case:
https://godbolt.org/z/TZzfo6 shouldn't Clang warn in the case of bar()?

> Clang intends to suppress the -Wmissing-braces in that case; if the warning 
> is still being produced in a recent version of Clang, that's a bug. However, 
> the warning suppression was added between Clang 5 and Clang 6, so it's very 
> plausible that the compiler being used here is simply older than the warning 
> fix.
>
> (Long story short: the change here seems fine, but should be unnecessary as 
> of Clang 6.)

The warning was identified from clang-8 ToT synced yesterday.

-- 
Thanks,
~Nick Desaulniers
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH] drm/amdgpu: Add braces to initialize task_info subojects

[Nick Desaulniers]

On Tue, Sep 11, 2018 at 5:26 PM Nathan Chancellor
 wrote:
>
> Clang warns if there are missing braces around a subobject
> initializer.
>
> drivers/gpu/drm/amd/amdgpu/gmc_v8_0.c:1447:41: warning: suggest braces
> around initialization of subobject [-Wmissing-braces]
> struct amdgpu_task_info task_info = { 0 };
>   ^
>   {}
> 1 warning generated.
>
> drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c:262:41: warning: suggest braces
> around initialization of subobject [-Wmissing-braces]
> struct amdgpu_task_info task_info = { 0 };
>   ^
>   {}
> 1 warning generated.
>
> Reported-by: Nick Desaulniers 
> Signed-off-by: Nathan Chancellor 
> ---
>  drivers/gpu/drm/amd/amdgpu/gmc_v8_0.c | 2 +-
>  drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c | 2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/gmc_v8_0.c 
> b/drivers/gpu/drm/amd/amdgpu/gmc_v8_0.c
> index 9333109b210d..968cc1b8cdff 100644
> --- a/drivers/gpu/drm/amd/amdgpu/gmc_v8_0.c
> +++ b/drivers/gpu/drm/amd/amdgpu/gmc_v8_0.c
> @@ -1444,7 +1444,7 @@ static int gmc_v8_0_process_interrupt(struct 
> amdgpu_device *adev,
> gmc_v8_0_set_fault_enable_default(adev, false);
>
> if (printk_ratelimit()) {
> -   struct amdgpu_task_info task_info = { 0 };
> +   struct amdgpu_task_info task_info = { { 0 } };

Hi Nathan,
Thanks for this patch.  I discussed this syntax with our language
lawyers.  Turns out, this is not quite correct, as you're now saying
"initialize the first subobject to zero, but not the rest of the
object."  -Wmissing-field-initializers would highlight this, but it's
not part of -Wall.  It would be more correct to zero initialize the
full struct, including all of its subobjects with `= {};`.

>
> amdgpu_vm_get_task_info(adev, entry->pasid, _info);
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c 
> b/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c
> index 72f8018fa2a8..a781a5027212 100644
> --- a/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c
> +++ b/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c
> @@ -259,7 +259,7 @@ static int gmc_v9_0_process_interrupt(struct 
> amdgpu_device *adev,
> }
>
> if (printk_ratelimit()) {
> -   struct amdgpu_task_info task_info = { 0 };
> +       struct amdgpu_task_info task_info = { { 0 } };
>
> amdgpu_vm_get_task_info(adev, entry->pasid, _info);
>
> --
> 2.18.0
>


-- 
Thanks,
~Nick Desaulniers
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx