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

[Jeff Mahoney <jeffm@suse.com>]

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