From mboxrd@z Thu Jan 1 00:00:00 1970 From: Jeff Mahoney Subject: Re: [PATCH] drm/amd: add Kconfig dependency for ACP on DRM_AMDGPU Date: Thu, 26 May 2016 09:52:03 -0400 Message-ID: <4f2067da-9db4-8cb3-ea7f-4ffbfd3f3aa5@suse.com> References: <8b2a86a0-4653-d816-dd39-bbfa2899e676@suse.com> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============2019155129==" Return-path: Received: from mx2.suse.de (mx2.suse.de [195.135.220.15]) by gabe.freedesktop.org (Postfix) with ESMTPS id 9640F6E2CD for ; Thu, 26 May 2016 13:52:12 +0000 (UTC) In-Reply-To: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dri-devel-bounces@lists.freedesktop.org Sender: "dri-devel" To: Emil Velikov Cc: Alex Deucher , ML dri-devel List-Id: dri-devel@lists.freedesktop.org This is an OpenPGP/MIME signed message (RFC 4880 and 3156) --===============2019155129== Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="0fABjVVfBxplucFop9XsC23aBsvgnVsgQ" This is an OpenPGP/MIME signed message (RFC 4880 and 3156) --0fABjVVfBxplucFop9XsC23aBsvgnVsgQ Content-Type: multipart/mixed; boundary="xfeKd7B8U74hVHS6OHOc5lmM2IB6c5B72" From: Jeff Mahoney To: Emil Velikov Cc: Alex Deucher , ML dri-devel Message-ID: <4f2067da-9db4-8cb3-ea7f-4ffbfd3f3aa5@suse.com> Subject: Re: [PATCH] drm/amd: add Kconfig dependency for ACP on DRM_AMDGPU References: <8b2a86a0-4653-d816-dd39-bbfa2899e676@suse.com> In-Reply-To: --xfeKd7B8U74hVHS6OHOc5lmM2IB6c5B72 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable On 5/25/16 5:09 PM, Emil Velikov wrote: > Hi Jeff, >=20 > 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=3Dy 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 the= re. > On 24 May 2016 at 18:47, Jeff Mahoney wrote: >> The DRM_AMD_ACP option doesn't have any dependencies and selects >> MFD_CORE, which results in MFD_CORE=3Dy. Since the code is only calle= d >> from DRM_AMDGPU, it should depend on it. Adding the dependency result= s >> in MFD_CORE being selected as a module again if amdgpu is also a modul= e. >> >> Signed-off-by: Jeff Mahoney >> --- >> 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 ? >=20 > 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= =2Ec > - 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 --=20 Jeff Mahoney SUSE Labs --xfeKd7B8U74hVHS6OHOc5lmM2IB6c5B72-- --0fABjVVfBxplucFop9XsC23aBsvgnVsgQ Content-Type: application/pgp-signature; name="signature.asc" Content-Description: OpenPGP digital signature Content-Disposition: attachment; filename="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG/MacGPG2 v2.0.19 (Darwin) Comment: GPGTools - http://gpgtools.org iQIcBAEBAgAGBQJXRv+GAAoJEB57S2MheeWymtcQAKc9Y5ZcSC/PNDNoOTW3Eyab 6fBuZIrGqwzZVdZvTGhwxkKS7GtHf61LbaJrDXeJqXgNNV7ilUHePki+LpenH+Sa 8AnmwkN198cE6fosZ2l06zexxXKrC4I4c/gtTTkriDa7S63Nznhos0Mh5NM9B5lO yqZE6DmRY5Fk3S5wrU4x1GdzkgCk37wS0Td3XwvFlVZPt8+/fBG2lCqe2bdC0pus 6XSOeniBcAvKXhSuUocWcPq+5CpimEIO/KtegK9G9eXj+CUiJWktqx9z3a6njXl4 zMcLf9XP7y0uef80u6QIy4Z1PkpApO4O7G4rxc9xKg54zVyxIi2ab2o1H4GOlXoA kzN6LS2Ka9k0EjvuEnQI5nWIy0n3/FC5gSVdq/++fa0/Z/iJpFltVGJifeJuhut2 uvQzGZNMudGvqKrUPAN+s+s44WpEevlMt//oTtZE7/SnlvdgxppbPyOsUl8tAlKi FA8U04CMNf2Qzgw/DYzMdsa3bCAU1VtbhyhnXoKLFtACxNSTLVOQjHs+rethHPGo OZIZcVZeZUi4KwYGbp43CpzSF5zOKMbUdM8I/QsaO54aDNFxi02JqE8+P8MGj38O bUIlpx58ANrAWb9/AdxIXJkv3wpQJ0OhFQOuk/BSsYxtreEWRrFZ88HF17+KuCxW g8HYNBSmpvbFy2E+DUwx =eZ7G -----END PGP SIGNATURE----- --0fABjVVfBxplucFop9XsC23aBsvgnVsgQ-- --===============2019155129== Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: base64 Content-Disposition: inline X19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX18KZHJpLWRldmVs IG1haWxpbmcgbGlzdApkcmktZGV2ZWxAbGlzdHMuZnJlZWRlc2t0b3Aub3JnCmh0dHBzOi8vbGlz dHMuZnJlZWRlc2t0b3Aub3JnL21haWxtYW4vbGlzdGluZm8vZHJpLWRldmVsCg== --===============2019155129==--