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 helpAfaict 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