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

[Alex Deucher]

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