Re: [PATCH] drm/amd: add Kconfig dependency for ACP on DRM_AMDGPU

[Emil Velikov <emil.l.velikov@gmail.com>]

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