* [PATCH] drm/amd: add Kconfig dependency for ACP on DRM_AMDGPU @ 2016-05-24 17:47 Jeff Mahoney 2016-05-25 14:06 ` Alex Deucher 2016-05-25 21:09 ` Emil Velikov 0 siblings, 2 replies; 6+ messages in thread From: Jeff Mahoney @ 2016-05-24 17:47 UTC (permalink / raw) To: Alex Deucher, dri-devel The DRM_AMD_ACP option doesn't have any dependencies and selects MFD_CORE, which results in MFD_CORE=y. Since the code is only called from DRM_AMDGPU, it should depend on it. Adding the dependency results in MFD_CORE being selected as a module again if amdgpu is also a module. Signed-off-by: Jeff Mahoney <jeffm@suse.com> --- drivers/gpu/drm/amd/acp/Kconfig | 1 + 1 file changed, 1 insertion(+) diff --git a/drivers/gpu/drm/amd/acp/Kconfig b/drivers/gpu/drm/amd/acp/Kconfig index ca77ec1..e503e3d 100644 --- a/drivers/gpu/drm/amd/acp/Kconfig +++ b/drivers/gpu/drm/amd/acp/Kconfig @@ -2,6 +2,7 @@ menu "ACP (Audio CoProcessor) Configuration" config DRM_AMD_ACP bool "Enable AMD Audio CoProcessor IP support" + depends on DRM_AMDGPU select MFD_CORE select PM_GENERIC_DOMAINS if PM help -- Jeff Mahoney SUSE Labs _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] drm/amd: add Kconfig dependency for ACP on DRM_AMDGPU 2016-05-24 17:47 [PATCH] drm/amd: add Kconfig dependency for ACP on DRM_AMDGPU Jeff Mahoney @ 2016-05-25 14:06 ` Alex Deucher 2016-05-25 21:09 ` Emil Velikov 1 sibling, 0 replies; 6+ messages in thread From: Alex Deucher @ 2016-05-25 14:06 UTC (permalink / raw) To: Jeff Mahoney; +Cc: Alex Deucher, Maling list - DRI developers On Tue, May 24, 2016 at 1:47 PM, Jeff Mahoney <jeffm@suse.com> wrote: > The DRM_AMD_ACP option doesn't have any dependencies and selects > MFD_CORE, which results in MFD_CORE=y. Since the code is only called > from DRM_AMDGPU, it should depend on it. Adding the dependency results > in MFD_CORE being selected as a module again if amdgpu is also a module. > > Signed-off-by: Jeff Mahoney <jeffm@suse.com> Applied. thanks! Alex > --- > drivers/gpu/drm/amd/acp/Kconfig | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/drivers/gpu/drm/amd/acp/Kconfig b/drivers/gpu/drm/amd/acp/Kconfig > index ca77ec1..e503e3d 100644 > --- a/drivers/gpu/drm/amd/acp/Kconfig > +++ b/drivers/gpu/drm/amd/acp/Kconfig > @@ -2,6 +2,7 @@ menu "ACP (Audio CoProcessor) Configuration" > > config DRM_AMD_ACP > bool "Enable AMD Audio CoProcessor IP support" > + depends on DRM_AMDGPU > select MFD_CORE > select PM_GENERIC_DOMAINS if PM > help > > > -- > Jeff Mahoney > SUSE Labs > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] drm/amd: add Kconfig dependency for ACP on DRM_AMDGPU 2016-05-24 17:47 [PATCH] drm/amd: add Kconfig dependency for ACP on DRM_AMDGPU Jeff Mahoney 2016-05-25 14:06 ` Alex Deucher @ 2016-05-25 21:09 ` Emil Velikov 2016-05-26 13:52 ` Jeff Mahoney 1 sibling, 1 reply; 6+ messages in thread From: Emil Velikov @ 2016-05-25 21:09 UTC (permalink / raw) To: Jeff Mahoney; +Cc: Alex Deucher, ML dri-devel Hi Jeff, I'm thinking out loud so here are a few suggestions/ideas. Please don't take them too seriously although they do make sense from this end. On 24 May 2016 at 18:47, Jeff Mahoney <jeffm@suse.com> wrote: > The DRM_AMD_ACP option doesn't have any dependencies and selects > MFD_CORE, which results in MFD_CORE=y. Since the code is only called > from DRM_AMDGPU, it should depend on it. Adding the dependency results > in MFD_CORE being selected as a module again if amdgpu is also a module. > > Signed-off-by: Jeff Mahoney <jeffm@suse.com> > --- > drivers/gpu/drm/amd/acp/Kconfig | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/drivers/gpu/drm/amd/acp/Kconfig b/drivers/gpu/drm/amd/acp/Kconfig > index ca77ec1..e503e3d 100644 > --- a/drivers/gpu/drm/amd/acp/Kconfig > +++ b/drivers/gpu/drm/amd/acp/Kconfig > @@ -2,6 +2,7 @@ menu "ACP (Audio CoProcessor) Configuration" > > config DRM_AMD_ACP > bool "Enable AMD Audio CoProcessor IP support" > + depends on DRM_AMDGPU > select MFD_CORE > select PM_GENERIC_DOMAINS if PM > help > Afaict ACP/Powerplay doesn't make sense on their own so here is an alternative solution: - create a Kconfig in drm/amd and move/consolidate amdgpu, powerplay, acp in there. amdkfd can be moved as well afaict. - make DRM_AMD_ACP and DRM_AMD_POWERPLAY submenu items for DRM_AMDGPU. AMDKFD on the other hand depends on RADEON || AMDGPU so it should stay separate. - crazy idea: add select and/or depends on SND_SOC_AMD_ACP. since one is useless without the other. Or perhaps make the SOC entry should select/depend on AMD_ACP ? And some future work (cleanups): - fold the acp folder (or inline respective files) within amdgpu. - add forward declarations in acp/include/acp_gfx_if.h and move the cgs* includes where needed (acp/acp_hw.c and amdgpu/amdgpu_acp.c) - apply similar polish for amdgpu/amdgpu_acp.h. - drop the (unneeded ?) include of amdgpu_acp.h include from amdgpu/vi.c - kill off amdgpu_acp::private. Afaict there's not a single place (as of 95306975e9dd38ba2775dd96cb29987ecc7d9360) that actually defines the amd_acp_private struct. Is something broken on my end or this does not build without warnings/errors ? - kill off amdgpu_acp::acp_genpd::cgs_dev (use amdgpu_acp::cgs_device directly) and inline the ::acp_genpd::gpd into amdgpu_acp. Regards, Emil _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] drm/amd: add Kconfig dependency for ACP on DRM_AMDGPU 2016-05-25 21:09 ` Emil Velikov @ 2016-05-26 13:52 ` Jeff Mahoney 2016-05-26 23:36 ` Emil Velikov 0 siblings, 1 reply; 6+ messages in thread From: Jeff Mahoney @ 2016-05-26 13:52 UTC (permalink / raw) To: Emil Velikov; +Cc: Alex Deucher, ML dri-devel [-- Attachment #1.1.1: Type: text/plain, Size: 3494 bytes --] On 5/25/16 5:09 PM, Emil Velikov wrote: > Hi Jeff, > > I'm thinking out loud so here are a few suggestions/ideas. Please > don't take them too seriously although they do make sense from this > end. Hi Emil - I'm not at all involved in amdgpu development. This patch came from me fielding reports that the openSUSE Tumbleweed kernel had MFD_CORE=y as part of a version update. In the process of troubleshooting, I did see how the code was structured and there are some interesting choices in there. > On 24 May 2016 at 18:47, Jeff Mahoney <jeffm@suse.com> wrote: >> The DRM_AMD_ACP option doesn't have any dependencies and selects >> MFD_CORE, which results in MFD_CORE=y. Since the code is only called >> from DRM_AMDGPU, it should depend on it. Adding the dependency results >> in MFD_CORE being selected as a module again if amdgpu is also a module. >> >> Signed-off-by: Jeff Mahoney <jeffm@suse.com> >> --- >> drivers/gpu/drm/amd/acp/Kconfig | 1 + >> 1 file changed, 1 insertion(+) >> >> diff --git a/drivers/gpu/drm/amd/acp/Kconfig b/drivers/gpu/drm/amd/acp/Kconfig >> index ca77ec1..e503e3d 100644 >> --- a/drivers/gpu/drm/amd/acp/Kconfig >> +++ b/drivers/gpu/drm/amd/acp/Kconfig >> @@ -2,6 +2,7 @@ menu "ACP (Audio CoProcessor) Configuration" >> >> config DRM_AMD_ACP >> bool "Enable AMD Audio CoProcessor IP support" >> + depends on DRM_AMDGPU >> select MFD_CORE >> select PM_GENERIC_DOMAINS if PM >> help >> > Afaict ACP/Powerplay doesn't make sense on their own so here is an > alternative solution: > - create a Kconfig in drm/amd and move/consolidate amdgpu, powerplay, > acp in there. I have a patch to do this already. The bigger issue, though, is the weird hierarchy that may make sense for an external driver but is an outlier for in-kernel drivers. > amdkfd can be moved as well afaict. > - make DRM_AMD_ACP and DRM_AMD_POWERPLAY submenu items for > DRM_AMDGPU. AMDKFD on the other hand depends on RADEON || AMDGPU so it > should stay separate. > - crazy idea: add select and/or depends on SND_SOC_AMD_ACP. since one > is useless without the other. Or perhaps make the SOC entry should > select/depend on AMD_ACP ? > > And some future work (cleanups): > - fold the acp folder (or inline respective files) within amdgpu. Yes. This should be part of amdgpu. All of the code except for this one function is there already. Having this weird build setup for a single source file with a single function is silly. I have a patch for that too but didn't know if there was a reason for the way it's structured that isn't immediately visible. > - add forward declarations in acp/include/acp_gfx_if.h and move the > cgs* includes where needed (acp/acp_hw.c and amdgpu/amdgpu_acp.c) > - apply similar polish for amdgpu/amdgpu_acp.h. > - drop the (unneeded ?) include of amdgpu_acp.h include from amdgpu/vi.c > - kill off amdgpu_acp::private. Afaict there's not a single place (as > of 95306975e9dd38ba2775dd96cb29987ecc7d9360) that actually defines the > amd_acp_private struct. Is something broken on my end or this does not > build without warnings/errors ? > - kill off amdgpu_acp::acp_genpd::cgs_dev (use amdgpu_acp::cgs_device > directly) and inline the ::acp_genpd::gpd into amdgpu_acp. This starts getting into the code itself and I don't have hardware to test with, so I didn't dig in at all. -Jeff -- Jeff Mahoney SUSE Labs [-- Attachment #1.2: OpenPGP digital signature --] [-- Type: application/pgp-signature, Size: 881 bytes --] [-- Attachment #2: Type: text/plain, Size: 160 bytes --] _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] drm/amd: add Kconfig dependency for ACP on DRM_AMDGPU 2016-05-26 13:52 ` Jeff Mahoney @ 2016-05-26 23:36 ` Emil Velikov 2016-05-27 16:50 ` Alex Deucher 0 siblings, 1 reply; 6+ messages in thread From: Emil Velikov @ 2016-05-26 23:36 UTC (permalink / raw) To: Jeff Mahoney; +Cc: Alex Deucher, ML dri-devel On 26 May 2016 at 14:52, Jeff Mahoney <jeffm@suse.com> wrote: > On 5/25/16 5:09 PM, Emil Velikov wrote: >> Hi Jeff, >> >> I'm thinking out loud so here are a few suggestions/ideas. Please >> don't take them too seriously although they do make sense from this >> end. > > Hi Emil - > > I'm not at all involved in amdgpu development. About the same here, sadly. > This patch came from me > fielding reports that the openSUSE Tumbleweed kernel had MFD_CORE=y as > part of a version update. In the process of troubleshooting, I did see > how the code was structured and there are some interesting choices in there. > >> On 24 May 2016 at 18:47, Jeff Mahoney <jeffm@suse.com> wrote: >>> The DRM_AMD_ACP option doesn't have any dependencies and selects >>> MFD_CORE, which results in MFD_CORE=y. Since the code is only called >>> from DRM_AMDGPU, it should depend on it. Adding the dependency results >>> in MFD_CORE being selected as a module again if amdgpu is also a module. >>> >>> Signed-off-by: Jeff Mahoney <jeffm@suse.com> >>> --- >>> drivers/gpu/drm/amd/acp/Kconfig | 1 + >>> 1 file changed, 1 insertion(+) >>> >>> diff --git a/drivers/gpu/drm/amd/acp/Kconfig b/drivers/gpu/drm/amd/acp/Kconfig >>> index ca77ec1..e503e3d 100644 >>> --- a/drivers/gpu/drm/amd/acp/Kconfig >>> +++ b/drivers/gpu/drm/amd/acp/Kconfig >>> @@ -2,6 +2,7 @@ menu "ACP (Audio CoProcessor) Configuration" >>> >>> config DRM_AMD_ACP >>> bool "Enable AMD Audio CoProcessor IP support" >>> + depends on DRM_AMDGPU >>> select MFD_CORE >>> select PM_GENERIC_DOMAINS if PM >>> help >>> >> Afaict ACP/Powerplay doesn't make sense on their own so here is an >> alternative solution: >> - create a Kconfig in drm/amd and move/consolidate amdgpu, powerplay, >> acp in there. > > I have a patch to do this already. The bigger issue, though, is the > weird hierarchy that may make sense for an external driver but is an > outlier for in-kernel drivers. > Glad to hear we've both reached ~same conclusion. Alex and other AMD devs are quite open to input so don't be shy to put the patch forward. >> amdkfd can be moved as well afaict. >> - make DRM_AMD_ACP and DRM_AMD_POWERPLAY submenu items for >> DRM_AMDGPU. AMDKFD on the other hand depends on RADEON || AMDGPU so it >> should stay separate. >> - crazy idea: add select and/or depends on SND_SOC_AMD_ACP. since one >> is useless without the other. Or perhaps make the SOC entry should >> select/depend on AMD_ACP ? >> >> And some future work (cleanups): >> - fold the acp folder (or inline respective files) within amdgpu. > > Yes. This should be part of amdgpu. All of the code except for this > one function is there already. Having this weird build setup for a > single source file with a single function is silly. I have a patch for > that too but didn't know if there was a reason for the way it's > structured that isn't immediately visible. > Just send it out and devs involved will weight in ;-) >> - add forward declarations in acp/include/acp_gfx_if.h and move the >> cgs* includes where needed (acp/acp_hw.c and amdgpu/amdgpu_acp.c) >> - apply similar polish for amdgpu/amdgpu_acp.h. >> - drop the (unneeded ?) include of amdgpu_acp.h include from amdgpu/vi.c >> - kill off amdgpu_acp::private. Afaict there's not a single place (as >> of 95306975e9dd38ba2775dd96cb29987ecc7d9360) that actually defines the >> amd_acp_private struct. Is something broken on my end or this does not >> build without warnings/errors ? >> - kill off amdgpu_acp::acp_genpd::cgs_dev (use amdgpu_acp::cgs_device >> directly) and inline the ::acp_genpd::gpd into amdgpu_acp. > > This starts getting into the code itself and I don't have hardware to > test with, so I didn't dig in at all. > No problems. It was just some ideas if you/others are interested in doing some tidying up. Thanks Emil _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] drm/amd: add Kconfig dependency for ACP on DRM_AMDGPU 2016-05-26 23:36 ` Emil Velikov @ 2016-05-27 16:50 ` Alex Deucher 0 siblings, 0 replies; 6+ messages in thread From: Alex Deucher @ 2016-05-27 16:50 UTC (permalink / raw) To: Emil Velikov; +Cc: Alex Deucher, Jeff Mahoney, ML dri-devel On Thu, May 26, 2016 at 7:36 PM, Emil Velikov <emil.l.velikov@gmail.com> wrote: > On 26 May 2016 at 14:52, Jeff Mahoney <jeffm@suse.com> wrote: >> On 5/25/16 5:09 PM, Emil Velikov wrote: >>> Hi Jeff, >>> >>> I'm thinking out loud so here are a few suggestions/ideas. Please >>> don't take them too seriously although they do make sense from this >>> end. >> >> Hi Emil - >> >> I'm not at all involved in amdgpu development. > About the same here, sadly. > >> This patch came from me >> fielding reports that the openSUSE Tumbleweed kernel had MFD_CORE=y as >> part of a version update. In the process of troubleshooting, I did see >> how the code was structured and there are some interesting choices in there. >> >>> On 24 May 2016 at 18:47, Jeff Mahoney <jeffm@suse.com> wrote: >>>> The DRM_AMD_ACP option doesn't have any dependencies and selects >>>> MFD_CORE, which results in MFD_CORE=y. Since the code is only called >>>> from DRM_AMDGPU, it should depend on it. Adding the dependency results >>>> in MFD_CORE being selected as a module again if amdgpu is also a module. >>>> >>>> Signed-off-by: Jeff Mahoney <jeffm@suse.com> >>>> --- >>>> drivers/gpu/drm/amd/acp/Kconfig | 1 + >>>> 1 file changed, 1 insertion(+) >>>> >>>> diff --git a/drivers/gpu/drm/amd/acp/Kconfig b/drivers/gpu/drm/amd/acp/Kconfig >>>> index ca77ec1..e503e3d 100644 >>>> --- a/drivers/gpu/drm/amd/acp/Kconfig >>>> +++ b/drivers/gpu/drm/amd/acp/Kconfig >>>> @@ -2,6 +2,7 @@ menu "ACP (Audio CoProcessor) Configuration" >>>> >>>> config DRM_AMD_ACP >>>> bool "Enable AMD Audio CoProcessor IP support" >>>> + depends on DRM_AMDGPU >>>> select MFD_CORE >>>> select PM_GENERIC_DOMAINS if PM >>>> help >>>> >>> Afaict ACP/Powerplay doesn't make sense on their own so here is an >>> alternative solution: >>> - create a Kconfig in drm/amd and move/consolidate amdgpu, powerplay, >>> acp in there. >> >> I have a patch to do this already. The bigger issue, though, is the >> weird hierarchy that may make sense for an external driver but is an >> outlier for in-kernel drivers. >> > Glad to hear we've both reached ~same conclusion. Alex and other AMD > devs are quite open to input so don't be shy to put the patch forward. Yes, please do. We are happy to review any patches you might have. Alex > >>> amdkfd can be moved as well afaict. >>> - make DRM_AMD_ACP and DRM_AMD_POWERPLAY submenu items for >>> DRM_AMDGPU. AMDKFD on the other hand depends on RADEON || AMDGPU so it >>> should stay separate. >>> - crazy idea: add select and/or depends on SND_SOC_AMD_ACP. since one >>> is useless without the other. Or perhaps make the SOC entry should >>> select/depend on AMD_ACP ? >>> >>> And some future work (cleanups): >>> - fold the acp folder (or inline respective files) within amdgpu. >> >> Yes. This should be part of amdgpu. All of the code except for this >> one function is there already. Having this weird build setup for a >> single source file with a single function is silly. I have a patch for >> that too but didn't know if there was a reason for the way it's >> structured that isn't immediately visible. >> > Just send it out and devs involved will weight in ;-) > >>> - add forward declarations in acp/include/acp_gfx_if.h and move the >>> cgs* includes where needed (acp/acp_hw.c and amdgpu/amdgpu_acp.c) >>> - apply similar polish for amdgpu/amdgpu_acp.h. >>> - drop the (unneeded ?) include of amdgpu_acp.h include from amdgpu/vi.c >>> - kill off amdgpu_acp::private. Afaict there's not a single place (as >>> of 95306975e9dd38ba2775dd96cb29987ecc7d9360) that actually defines the >>> amd_acp_private struct. Is something broken on my end or this does not >>> build without warnings/errors ? >>> - kill off amdgpu_acp::acp_genpd::cgs_dev (use amdgpu_acp::cgs_device >>> directly) and inline the ::acp_genpd::gpd into amdgpu_acp. >> >> This starts getting into the code itself and I don't have hardware to >> test with, so I didn't dig in at all. >> > No problems. It was just some ideas if you/others are interested in > doing some tidying up. > > Thanks > Emil > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2016-05-27 16:50 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2016-05-24 17:47 [PATCH] drm/amd: add Kconfig dependency for ACP on DRM_AMDGPU Jeff Mahoney 2016-05-25 14:06 ` Alex Deucher 2016-05-25 21:09 ` Emil Velikov 2016-05-26 13:52 ` Jeff Mahoney 2016-05-26 23:36 ` Emil Velikov 2016-05-27 16:50 ` Alex Deucher
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox