On Thu, May 26, 2016 at 7:36 PM, Emil Velikov <emil.l.velikov@xxxxxxxxx> wrote: > On 26 May 2016 at 14:52, Jeff Mahoney <jeffm@xxxxxxxx> 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@xxxxxxxx> 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@xxxxxxxx> >>>> --- >>>> 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@xxxxxxxxxxxxxxxxxxxxx > https://lists.freedesktop.org/mailman/listinfo/dri-devel _______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/dri-devel
