On Wed, Jun 15, 2022 at 04:45:16PM -0400, Alex Deucher wrote:
> On Wed, Jun 15, 2022 at 4:24 PM Nathan Chancellor <[email protected]> wrote:
> >
> > On Wed, Jun 15, 2022 at 03:28:52PM -0400, Alex Deucher wrote:
> > > On Wed, Jun 15, 2022 at 3:01 PM Randy Dunlap <[email protected]> wrote:
> > > >
> > > >
> > > >
> > > > On 6/14/22 23:01, Stephen Rothwell wrote:
> > > > > Hi all,
> > > > >
> > > > > Changes since 20220614:
> > > > >
> > > >
> > > > on i386:
> > > > # CONFIG_DEBUG_FS is not set
> > > >
> > > >
> > > > ../drivers/gpu/drm/amd/amdgpu/../display/amdgpu_dm/amdgpu_dm.c: In function ‘amdgpu_dm_crtc_late_register’:
> > > > ../drivers/gpu/drm/amd/amdgpu/../display/amdgpu_dm/amdgpu_dm.c:6599:2: error: implicit declaration of function ‘crtc_debugfs_init’; did you mean ‘amdgpu_debugfs_init’? [-Werror=implicit-function-declaration]
> > > > crtc_debugfs_init(crtc);
> > > > ^~~~~~~~~~~~~~~~~
> > > > amdgpu_debugfs_init
> > > >
> > > >
> > > > Full randconfig file is attached.
> > >
> > > I tried building with your config and I can't repro this. As Harry
> > > noted, that function and the whole secure display feature depend on
> > > debugfs. It should never be built without CONFIG_DEBUG_FS. See
> > > drivers/gpu/drm/amd/display/Kconfig:
> > >
> > > > config DRM_AMD_SECURE_DISPLAY
> > > > bool "Enable secure display support"
> > > > default n
> > > > depends on DEBUG_FS
> > > > depends on DRM_AMD_DC_DCN
> > > > help
> > > > Choose this option if you want to
> > > > support secure display
> > > >
> > > > This option enables the calculation
> > > > of crc of specific region via debugfs.
> > > > Cooperate with specific DMCU FW.
> > >
> > > amdgpu_dm_crtc_late_register is guarded by
> > > CONIG_DRM_AMD_SECURE_DISPLAY. It's not clear to me how we could hit
> > > this.
> >
> > I think the problem is that you are not looking at the right tree.
> >
> > The kernel test robot reported [1] [2] this error is caused by commit
> > 4cd79f614b50 ("drm/amd/display: Move connector debugfs to drm"), which
> > is in the drm-misc tree on the drm-misc-next branch. That change removes
> > the #ifdef around amdgpu_dm_crtc_late_register(), meaning that
> > crtc_debugfs_init() can be called without CONFIG_DRM_AMD_SECURE_DISPLAY
> > and CONFIG_DEBUG_FS.
> >
> > $ git show -s --format='%h ("%s")'
> > abf0ba5a34ea ("drm/bridge: it6505: Add missing CRYPTO_HASH dependency")
> >
> > $ make -skj"$(nproc)" ARCH=x86_64 mrproper defconfig
> >
> > $ scripts/config -d BLK_DEV_IO_TRACE -d DEBUG_FS -e DRM_AMDGPU
> >
> > $ make -skj"$(nproc)" ARCH=x86_64 olddefconfig drivers/gpu/drm/amd/amdgpu/../display/amdgpu_dm/amdgpu_dm.o
> > drivers/gpu/drm/amd/amdgpu/../display/amdgpu_dm/amdgpu_dm.c: In function ‘amdgpu_dm_crtc_late_register’:
> > drivers/gpu/drm/amd/amdgpu/../display/amdgpu_dm/amdgpu_dm.c:6622:9: error: implicit declaration of function ‘crtc_debugfs_init’; did you mean ‘amdgpu_debugfs_init’? [-Werror=implicit-function-declaration]
> > 6622 | crtc_debugfs_init(crtc);
> > | ^~~~~~~~~~~~~~~~~
> > | amdgpu_debugfs_init
> > cc1: all warnings being treated as errors
> >
> > Contrast that with the current top of your tree:
> >
> > $ git show -s --format='%h ("%s")'
> > c435f61d0eb3 ("drm/amd/display: Drop unnecessary guard from DC resource")
> >
> > $ make -skj"$(nproc)" ARCH=x86_64 mrproper defconfig
> >
> > $ scripts/config -d BLK_DEV_IO_TRACE -d DEBUG_FS -e DRM_AMDGPU
> >
> > $ make -skj"$(nproc)" ARCH=x86_64 olddefconfig drivers/gpu/drm/amd/amdgpu/../display/amdgpu_dm/amdgpu_dm.o
> >
> > $ echo $?
> > 0
> >
> > Randy's patch [3] seems like it should resolve the issue just fine but
> > it needs to be applied to drm-misc-next, not the amdgpu tree.
>
> Thanks for tracking this down. I think something like the attached
> patch is cleaner since the whole thing is only valid for debugfs.
Makes sense! I tested the below patch with and without DEBUG_FS and saw
no errors.
> From b0bcacd86344998e0ca757f89c6c4cd3b6298999 Mon Sep 17 00:00:00 2001
> From: Alex Deucher <[email protected]>
> Date: Wed, 15 Jun 2022 16:40:39 -0400
> Subject: [PATCH] drm/amdgpu/display: fix build when CONFIG_DEBUG_FS is not set
>
> amdgpu_dm_crtc_late_register is only used when CONFIG_DEBUG_FS
> so make it dependent on that.
>
> Fixes: 4cd79f614b50 ("drm/amd/display: Move connector debugfs to drm")
> Reported-by: Randy Dunlap <[email protected]>
> Reported-by: Nathan Chancellor <[email protected]>
Tested-by: Nathan Chancellor <[email protected]> # build
> Signed-off-by: Alex Deucher <[email protected]>
> ---
> drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 4 ++++
> 1 file changed, 4 insertions(+)
>
> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> index c9004f7e700d..33cd7a3d4ecb 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> @@ -6594,12 +6594,14 @@ dm_crtc_duplicate_state(struct drm_crtc *crtc)
> return &state->base;
> }
>
> +#ifdef CONFIG_DEBUG_FS
> static int amdgpu_dm_crtc_late_register(struct drm_crtc *crtc)
> {
> crtc_debugfs_init(crtc);
>
> return 0;
> }
> +#endif
>
> static inline int dm_set_vupdate_irq(struct drm_crtc *crtc, bool enable)
> {
> @@ -6693,7 +6695,9 @@ static const struct drm_crtc_funcs amdgpu_dm_crtc_funcs = {
> .enable_vblank = dm_enable_vblank,
> .disable_vblank = dm_disable_vblank,
> .get_vblank_timestamp = drm_crtc_vblank_helper_get_vblank_timestamp,
> +#if defined(CONFIG_DEBUG_FS)
> .late_register = amdgpu_dm_crtc_late_register,
> +#endif
> };
>
> static enum drm_connector_status
> --
> 2.35.3
>
Hi all,
On Wed, 15 Jun 2022 13:52:34 -0700 Nathan Chancellor <[email protected]> wrote:
>
> On Wed, Jun 15, 2022 at 04:45:16PM -0400, Alex Deucher wrote:
> > On Wed, Jun 15, 2022 at 4:24 PM Nathan Chancellor <[email protected]> wrote:
> > >
> > > On Wed, Jun 15, 2022 at 03:28:52PM -0400, Alex Deucher wrote:
> > > > On Wed, Jun 15, 2022 at 3:01 PM Randy Dunlap <[email protected]> wrote:
> > > > >
> > > > >
> > > > >
> > > > > On 6/14/22 23:01, Stephen Rothwell wrote:
> > > > > > Hi all,
> > > > > >
> > > > > > Changes since 20220614:
> > > > > >
> > > > >
> > > > > on i386:
> > > > > # CONFIG_DEBUG_FS is not set
> > > > >
> > > > >
> > > > > ../drivers/gpu/drm/amd/amdgpu/../display/amdgpu_dm/amdgpu_dm.c: In function ‘amdgpu_dm_crtc_late_register’:
> > > > > ../drivers/gpu/drm/amd/amdgpu/../display/amdgpu_dm/amdgpu_dm.c:6599:2: error: implicit declaration of function ‘crtc_debugfs_init’; did you mean ‘amdgpu_debugfs_init’? [-Werror=implicit-function-declaration]
> > > > > crtc_debugfs_init(crtc);
> > > > > ^~~~~~~~~~~~~~~~~
> > > > > amdgpu_debugfs_init
> > > > >
> > > > >
> > > > > Full randconfig file is attached.
> > > >
> > > > I tried building with your config and I can't repro this. As Harry
> > > > noted, that function and the whole secure display feature depend on
> > > > debugfs. It should never be built without CONFIG_DEBUG_FS. See
> > > > drivers/gpu/drm/amd/display/Kconfig:
> > > >
> > > > > config DRM_AMD_SECURE_DISPLAY
> > > > > bool "Enable secure display support"
> > > > > default n
> > > > > depends on DEBUG_FS
> > > > > depends on DRM_AMD_DC_DCN
> > > > > help
> > > > > Choose this option if you want to
> > > > > support secure display
> > > > >
> > > > > This option enables the calculation
> > > > > of crc of specific region via debugfs.
> > > > > Cooperate with specific DMCU FW.
> > > >
> > > > amdgpu_dm_crtc_late_register is guarded by
> > > > CONIG_DRM_AMD_SECURE_DISPLAY. It's not clear to me how we could hit
> > > > this.
> > >
> > > I think the problem is that you are not looking at the right tree.
> > >
> > > The kernel test robot reported [1] [2] this error is caused by commit
> > > 4cd79f614b50 ("drm/amd/display: Move connector debugfs to drm"), which
> > > is in the drm-misc tree on the drm-misc-next branch. That change removes
> > > the #ifdef around amdgpu_dm_crtc_late_register(), meaning that
> > > crtc_debugfs_init() can be called without CONFIG_DRM_AMD_SECURE_DISPLAY
> > > and CONFIG_DEBUG_FS.
> > >
> > > $ git show -s --format='%h ("%s")'
> > > abf0ba5a34ea ("drm/bridge: it6505: Add missing CRYPTO_HASH dependency")
> > >
> > > $ make -skj"$(nproc)" ARCH=x86_64 mrproper defconfig
> > >
> > > $ scripts/config -d BLK_DEV_IO_TRACE -d DEBUG_FS -e DRM_AMDGPU
> > >
> > > $ make -skj"$(nproc)" ARCH=x86_64 olddefconfig drivers/gpu/drm/amd/amdgpu/../display/amdgpu_dm/amdgpu_dm.o
> > > drivers/gpu/drm/amd/amdgpu/../display/amdgpu_dm/amdgpu_dm.c: In function ‘amdgpu_dm_crtc_late_register’:
> > > drivers/gpu/drm/amd/amdgpu/../display/amdgpu_dm/amdgpu_dm.c:6622:9: error: implicit declaration of function ‘crtc_debugfs_init’; did you mean ‘amdgpu_debugfs_init’? [-Werror=implicit-function-declaration]
> > > 6622 | crtc_debugfs_init(crtc);
> > > | ^~~~~~~~~~~~~~~~~
> > > | amdgpu_debugfs_init
> > > cc1: all warnings being treated as errors
> > >
> > > Contrast that with the current top of your tree:
> > >
> > > $ git show -s --format='%h ("%s")'
> > > c435f61d0eb3 ("drm/amd/display: Drop unnecessary guard from DC resource")
> > >
> > > $ make -skj"$(nproc)" ARCH=x86_64 mrproper defconfig
> > >
> > > $ scripts/config -d BLK_DEV_IO_TRACE -d DEBUG_FS -e DRM_AMDGPU
> > >
> > > $ make -skj"$(nproc)" ARCH=x86_64 olddefconfig drivers/gpu/drm/amd/amdgpu/../display/amdgpu_dm/amdgpu_dm.o
> > >
> > > $ echo $?
> > > 0
> > >
> > > Randy's patch [3] seems like it should resolve the issue just fine but
> > > it needs to be applied to drm-misc-next, not the amdgpu tree.
> >
> > Thanks for tracking this down. I think something like the attached
> > patch is cleaner since the whole thing is only valid for debugfs.
>
> Makes sense! I tested the below patch with and without DEBUG_FS and saw
> no errors.
>
> > From b0bcacd86344998e0ca757f89c6c4cd3b6298999 Mon Sep 17 00:00:00 2001
> > From: Alex Deucher <[email protected]>
> > Date: Wed, 15 Jun 2022 16:40:39 -0400
> > Subject: [PATCH] drm/amdgpu/display: fix build when CONFIG_DEBUG_FS is not set
> >
> > amdgpu_dm_crtc_late_register is only used when CONFIG_DEBUG_FS
> > so make it dependent on that.
> >
> > Fixes: 4cd79f614b50 ("drm/amd/display: Move connector debugfs to drm")
> > Reported-by: Randy Dunlap <[email protected]>
> > Reported-by: Nathan Chancellor <[email protected]>
>
> Tested-by: Nathan Chancellor <[email protected]> # build
>
> > Signed-off-by: Alex Deucher <[email protected]>
> > ---
> > drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 4 ++++
> > 1 file changed, 4 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> > index c9004f7e700d..33cd7a3d4ecb 100644
> > --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> > +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> > @@ -6594,12 +6594,14 @@ dm_crtc_duplicate_state(struct drm_crtc *crtc)
> > return &state->base;
> > }
> >
> > +#ifdef CONFIG_DEBUG_FS
> > static int amdgpu_dm_crtc_late_register(struct drm_crtc *crtc)
> > {
> > crtc_debugfs_init(crtc);
> >
> > return 0;
> > }
> > +#endif
> >
> > static inline int dm_set_vupdate_irq(struct drm_crtc *crtc, bool enable)
> > {
> > @@ -6693,7 +6695,9 @@ static const struct drm_crtc_funcs amdgpu_dm_crtc_funcs = {
> > .enable_vblank = dm_enable_vblank,
> > .disable_vblank = dm_disable_vblank,
> > .get_vblank_timestamp = drm_crtc_vblank_helper_get_vblank_timestamp,
> > +#if defined(CONFIG_DEBUG_FS)
> > .late_register = amdgpu_dm_crtc_late_register,
> > +#endif
> > };
> >
> > static enum drm_connector_status
OK, I will apply that patch to the merge of the drm-misc tree from
today (until someone actaully applies it to some tree).
--
Cheers,
Stephen Rothwell
Pushed to drm-misc-next.
Alex
On Wed, Jun 15, 2022 at 7:26 PM Stephen Rothwell <[email protected]> wrote:
>
> Hi all,
>
> On Wed, 15 Jun 2022 13:52:34 -0700 Nathan Chancellor <[email protected]> wrote:
> >
> > On Wed, Jun 15, 2022 at 04:45:16PM -0400, Alex Deucher wrote:
> > > On Wed, Jun 15, 2022 at 4:24 PM Nathan Chancellor <[email protected]> wrote:
> > > >
> > > > On Wed, Jun 15, 2022 at 03:28:52PM -0400, Alex Deucher wrote:
> > > > > On Wed, Jun 15, 2022 at 3:01 PM Randy Dunlap <[email protected]> wrote:
> > > > > >
> > > > > >
> > > > > >
> > > > > > On 6/14/22 23:01, Stephen Rothwell wrote:
> > > > > > > Hi all,
> > > > > > >
> > > > > > > Changes since 20220614:
> > > > > > >
> > > > > >
> > > > > > on i386:
> > > > > > # CONFIG_DEBUG_FS is not set
> > > > > >
> > > > > >
> > > > > > ../drivers/gpu/drm/amd/amdgpu/../display/amdgpu_dm/amdgpu_dm.c: In function ‘amdgpu_dm_crtc_late_register’:
> > > > > > ../drivers/gpu/drm/amd/amdgpu/../display/amdgpu_dm/amdgpu_dm.c:6599:2: error: implicit declaration of function ‘crtc_debugfs_init’; did you mean ‘amdgpu_debugfs_init’? [-Werror=implicit-function-declaration]
> > > > > > crtc_debugfs_init(crtc);
> > > > > > ^~~~~~~~~~~~~~~~~
> > > > > > amdgpu_debugfs_init
> > > > > >
> > > > > >
> > > > > > Full randconfig file is attached.
> > > > >
> > > > > I tried building with your config and I can't repro this. As Harry
> > > > > noted, that function and the whole secure display feature depend on
> > > > > debugfs. It should never be built without CONFIG_DEBUG_FS. See
> > > > > drivers/gpu/drm/amd/display/Kconfig:
> > > > >
> > > > > > config DRM_AMD_SECURE_DISPLAY
> > > > > > bool "Enable secure display support"
> > > > > > default n
> > > > > > depends on DEBUG_FS
> > > > > > depends on DRM_AMD_DC_DCN
> > > > > > help
> > > > > > Choose this option if you want to
> > > > > > support secure display
> > > > > >
> > > > > > This option enables the calculation
> > > > > > of crc of specific region via debugfs.
> > > > > > Cooperate with specific DMCU FW.
> > > > >
> > > > > amdgpu_dm_crtc_late_register is guarded by
> > > > > CONIG_DRM_AMD_SECURE_DISPLAY. It's not clear to me how we could hit
> > > > > this.
> > > >
> > > > I think the problem is that you are not looking at the right tree.
> > > >
> > > > The kernel test robot reported [1] [2] this error is caused by commit
> > > > 4cd79f614b50 ("drm/amd/display: Move connector debugfs to drm"), which
> > > > is in the drm-misc tree on the drm-misc-next branch. That change removes
> > > > the #ifdef around amdgpu_dm_crtc_late_register(), meaning that
> > > > crtc_debugfs_init() can be called without CONFIG_DRM_AMD_SECURE_DISPLAY
> > > > and CONFIG_DEBUG_FS.
> > > >
> > > > $ git show -s --format='%h ("%s")'
> > > > abf0ba5a34ea ("drm/bridge: it6505: Add missing CRYPTO_HASH dependency")
> > > >
> > > > $ make -skj"$(nproc)" ARCH=x86_64 mrproper defconfig
> > > >
> > > > $ scripts/config -d BLK_DEV_IO_TRACE -d DEBUG_FS -e DRM_AMDGPU
> > > >
> > > > $ make -skj"$(nproc)" ARCH=x86_64 olddefconfig drivers/gpu/drm/amd/amdgpu/../display/amdgpu_dm/amdgpu_dm.o
> > > > drivers/gpu/drm/amd/amdgpu/../display/amdgpu_dm/amdgpu_dm.c: In function ‘amdgpu_dm_crtc_late_register’:
> > > > drivers/gpu/drm/amd/amdgpu/../display/amdgpu_dm/amdgpu_dm.c:6622:9: error: implicit declaration of function ‘crtc_debugfs_init’; did you mean ‘amdgpu_debugfs_init’? [-Werror=implicit-function-declaration]
> > > > 6622 | crtc_debugfs_init(crtc);
> > > > | ^~~~~~~~~~~~~~~~~
> > > > | amdgpu_debugfs_init
> > > > cc1: all warnings being treated as errors
> > > >
> > > > Contrast that with the current top of your tree:
> > > >
> > > > $ git show -s --format='%h ("%s")'
> > > > c435f61d0eb3 ("drm/amd/display: Drop unnecessary guard from DC resource")
> > > >
> > > > $ make -skj"$(nproc)" ARCH=x86_64 mrproper defconfig
> > > >
> > > > $ scripts/config -d BLK_DEV_IO_TRACE -d DEBUG_FS -e DRM_AMDGPU
> > > >
> > > > $ make -skj"$(nproc)" ARCH=x86_64 olddefconfig drivers/gpu/drm/amd/amdgpu/../display/amdgpu_dm/amdgpu_dm.o
> > > >
> > > > $ echo $?
> > > > 0
> > > >
> > > > Randy's patch [3] seems like it should resolve the issue just fine but
> > > > it needs to be applied to drm-misc-next, not the amdgpu tree.
> > >
> > > Thanks for tracking this down. I think something like the attached
> > > patch is cleaner since the whole thing is only valid for debugfs.
> >
> > Makes sense! I tested the below patch with and without DEBUG_FS and saw
> > no errors.
> >
> > > From b0bcacd86344998e0ca757f89c6c4cd3b6298999 Mon Sep 17 00:00:00 2001
> > > From: Alex Deucher <[email protected]>
> > > Date: Wed, 15 Jun 2022 16:40:39 -0400
> > > Subject: [PATCH] drm/amdgpu/display: fix build when CONFIG_DEBUG_FS is not set
> > >
> > > amdgpu_dm_crtc_late_register is only used when CONFIG_DEBUG_FS
> > > so make it dependent on that.
> > >
> > > Fixes: 4cd79f614b50 ("drm/amd/display: Move connector debugfs to drm")
> > > Reported-by: Randy Dunlap <[email protected]>
> > > Reported-by: Nathan Chancellor <[email protected]>
> >
> > Tested-by: Nathan Chancellor <[email protected]> # build
> >
> > > Signed-off-by: Alex Deucher <[email protected]>
> > > ---
> > > drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 4 ++++
> > > 1 file changed, 4 insertions(+)
> > >
> > > diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> > > index c9004f7e700d..33cd7a3d4ecb 100644
> > > --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> > > +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> > > @@ -6594,12 +6594,14 @@ dm_crtc_duplicate_state(struct drm_crtc *crtc)
> > > return &state->base;
> > > }
> > >
> > > +#ifdef CONFIG_DEBUG_FS
> > > static int amdgpu_dm_crtc_late_register(struct drm_crtc *crtc)
> > > {
> > > crtc_debugfs_init(crtc);
> > >
> > > return 0;
> > > }
> > > +#endif
> > >
> > > static inline int dm_set_vupdate_irq(struct drm_crtc *crtc, bool enable)
> > > {
> > > @@ -6693,7 +6695,9 @@ static const struct drm_crtc_funcs amdgpu_dm_crtc_funcs = {
> > > .enable_vblank = dm_enable_vblank,
> > > .disable_vblank = dm_disable_vblank,
> > > .get_vblank_timestamp = drm_crtc_vblank_helper_get_vblank_timestamp,
> > > +#if defined(CONFIG_DEBUG_FS)
> > > .late_register = amdgpu_dm_crtc_late_register,
> > > +#endif
> > > };
> > >
> > > static enum drm_connector_status
>
> OK, I will apply that patch to the merge of the drm-misc tree from
> today (until someone actaully applies it to some tree).
> --
> Cheers,
> Stephen Rothwell