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

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

[Jeff Mahoney]

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


-- 
Jeff Mahoney
SUSE Labs
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Alex Deucher]

On Tue, May 24, 2016 at 1:47 PM, 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>

Applied.  thanks!

Alex

> ---
>  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
>
>
> --
> Jeff Mahoney
> SUSE Labs
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Emil Velikov]

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

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

[Jeff Mahoney]

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

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

[Emil Velikov]

On 26 May 2016 at 14:52, Jeff Mahoney <jeffm@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@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.
>
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.

>> 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@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [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@gmail.com> wrote:
> On 26 May 2016 at 14:52, Jeff Mahoney <jeffm@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@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.
>>
> 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@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel