RE: [PATCH 0/9] PSP cleanup
Yep, We are in progress to move IP specific code into common interfaces. For instance, psp_vXX_get_fw_type/psp_vXX_prep_cmd_buf should be common interfaces, instead of IP specific ones. We’ll see that soon. And I agree with you that we should stick with the existing naming style. And that’s why I have concern we created some unnecessary files like psp_asd.c/psp_tmr.c/psp_xgmi.c/psp_cmn.c.etc. All of these are just play with common psp gfx cmd. The interfaces are limited and actually do similar jobs. For ASD, it is common for all the ASICs since from vega10. Although not all the ASICs really used ASD fw, the fact is we’ve already upstreamed ASD fw for vega series and onwards. Therefore, below interfaces seems unnecessary. +psp_load_asd(psp) \ + ((psp)->funcs->asd_load ? (psp)->funcs->asd_load((psp)) : 0) #define +psp_unload_asd(psp) \ + ((psp)->funcs->asd_unload ? (psp)->funcs->asd_unload((psp)) : 0) +#define psp_destory_asd(psp) \ + ((psp)->funcs->asd_destory ? (psp)->funcs->asd_destory((psp)) : 0) Similar case for TMR. Therefore below interfaces seems unnecessary. +#define psp_init_tmr(psp) \ + ((psp)->funcs->tmr_init ? (psp)->funcs->tmr_init((psp)) : 0) #define +psp_load_tmr(psp) \ + ((psp)->funcs->tmr_load ? (psp)->funcs->tmr_load((psp)) : 0) #define +psp_unload_tmr(psp) \ + ((psp)->funcs->tmr_unload ? (psp)->funcs->tmr_unload((psp)) : 0) +#define psp_destory_tmr(psp) \ + ((psp)->funcs->tmr_destory ? (psp)->funcs->tmr_destory((psp)) : 0) For XGMI, we can use either the “supported” flags in amdgpu_xgmi or num_physical_nodes to decide whether to load the TA or not. Therefore, below interfaces seems unnecessary. +#define psp_init_xgmi(psp) \ + ((psp)->funcs->xgmi_init ? (psp)->funcs->xgmi_init((psp)) : 0) +#define psp_load_xgmi(psp) \ + ((psp)->funcs->xgmi_load ? (psp)->funcs->xgmi_load((psp)) : 0) +#define psp_unload_xgmi(psp) \ + ((psp)->funcs->xgmi_unload ? (psp)->funcs->xgmi_unload((psp)) : 0) +#define psp_destory_xgmi(psp) \ + ((psp)->funcs->xgmi_destory ? (psp)->funcs->xgmi_destory((psp)) : 0) For the upcoming functionalities that need to play with TA ucode, they share exactly the same TA cmd with xgmi. Do we really need to separate them into different psp_xxx source files? In sum, I’d prefer to stick with the old structures: common interfaces in amdgpu_psp.c/.h, and IP specific ones in IP specific file. Regards, Hawking From: Christian König Sent: 2019年1月2日 20:00 To: Zhang, Hawking ; Quan, Evan ; amd-gfx@lists.freedesktop.org; Koenig, Christian Cc: Deucher, Alexander ; Xu, Feifei ; Huang, Ray Subject: Re: [PATCH 0/9] PSP cleanup I'd prefer to keep the old structures: common interfaces in amdgpu_psp.c/.h, and IP specific ones in IP specific file. That works for me as well. Key take away from the change overview is this: > drivers/gpu/drm/amd/amdgpu/psp_v10_0.c | 381 + > drivers/gpu/drm/amd/amdgpu/psp_v11_0.c | 539 +--- > drivers/gpu/drm/amd/amdgpu/psp_v3_1.c | 480 + That looks like we can move a good bunch of the per IP specific code into the common interface. And that is something I really like to see. No strong opinion if the common code should go into amdgpu_psp.c/h, amdgpu_xgmi.c/h or amdgpu_psp_xgmi.c/h. The only restriction I have is that we should just stick with the existing naming convention. Christian. Am 02.01.19 um 11:21 schrieb Zhang, Hawking: I'd prefer to keep the old structures: common interfaces in amdgpu_psp.c/.h, and IP specific ones in IP specific file. No matter it's something related to ASD,TMR, or XGMI.etc, all of these are just communication/handshake jobs between driver and psp fw. Driver plays messenger role with several psp cmd that are shared among ASIC generations. a unified amdgpu_psp.c file is good enough to hold all the common stuffs. Regards, Hawking [X] From: Christian König <mailto:ckoenig.leichtzumer...@gmail.com> Sent: Wednesday, January 2, 2019 6:01:56 PM To: Quan, Evan; amd-gfx@lists.freedesktop.org<mailto:amd-gfx@lists.freedesktop.org> Cc: Deucher, Alexander; Xu, Feifei; Huang, Ray; Zhang, Hawking Subject: Re: [PATCH 0/9] PSP cleanup The general idea looks good, but can we change the file and symbol naming a bit? So far we have named all non-ip version related functions amdgpu_* and ip related functions ip_version. E.g. following this xgmi functions should go into amdgpu_xgmi.c and not psp_xgmi.c Christian. Am 02.01.19 um 10:21 schrieb Evan Quan: > *** BLURB HERE *** > > Evan Quan (9): >drm/amdgpu: separate the PSP ring related APIs >drm/amdgpu: separate commonly used PSP APIs >drm/amdgpu: separate the xgm
Re: [PATCH 0/9] PSP cleanup
I'd prefer to keep the old structures: common interfaces in amdgpu_psp.c/.h, and IP specific ones in IP specific file. That works for me as well. Key take away from the change overview is this: > drivers/gpu/drm/amd/amdgpu/psp_v10_0.c | 381 + > drivers/gpu/drm/amd/amdgpu/psp_v11_0.c | 539 +--- > drivers/gpu/drm/amd/amdgpu/psp_v3_1.c | 480 + That looks like we can move a good bunch of the per IP specific code into the common interface. And that is something I really like to see. No strong opinion if the common code should go into amdgpu_psp.c/h, amdgpu_xgmi.c/h or amdgpu_psp_xgmi.c/h. The only restriction I have is that we should just stick with the existing naming convention. Christian. Am 02.01.19 um 11:21 schrieb Zhang, Hawking: I'd prefer to keep the old structures: common interfaces in amdgpu_psp.c/.h, and IP specific ones in IP specific file. No matter it's something related to ASD,TMR, or XGMI.etc, all of these are just communication/handshake jobs between driver and psp fw. Driver plays messenger role with several psp cmd that are shared among ASIC generations. a unified amdgpu_psp.c file is good enough to hold all the common stuffs. Regards, Hawking *From:* Christian König *Sent:* Wednesday, January 2, 2019 6:01:56 PM *To:* Quan, Evan; amd-gfx@lists.freedesktop.org *Cc:* Deucher, Alexander; Xu, Feifei; Huang, Ray; Zhang, Hawking *Subject:* Re: [PATCH 0/9] PSP cleanup The general idea looks good, but can we change the file and symbol naming a bit? So far we have named all non-ip version related functions amdgpu_* and ip related functions ip_version. E.g. following this xgmi functions should go into amdgpu_xgmi.c and not psp_xgmi.c Christian. Am 02.01.19 um 10:21 schrieb Evan Quan: > *** BLURB HERE *** > > Evan Quan (9): > drm/amdgpu: separate the PSP ring related APIs > drm/amdgpu: separate commonly used PSP APIs > drm/amdgpu: separate the xgmi related APIs > drm/amdgpu: separate the tmr related APIs > drm/amdgpu: separate the asd related APIs > drm/amdgpu: drop useless PSP APIs and structures > drm/amdgpu: check PSP support before adding the ip block > drm/amdgpu: make PSP sub modules(ASD/XGMI/TMR) support configurable > drm/amdgpu: move psp_funcs related to a more proper place > > drivers/gpu/drm/amd/amdgpu/Makefile | 7 +- > drivers/gpu/drm/amd/amdgpu/amdgpu_psp.c | 504 +++--- > drivers/gpu/drm/amd/amdgpu/amdgpu_psp.h | 93 +--- > drivers/gpu/drm/amd/amdgpu/psp_asd.c | 86 > drivers/gpu/drm/amd/amdgpu/psp_asd.h | 32 ++ > drivers/gpu/drm/amd/amdgpu/psp_cmn.c | 289 + > drivers/gpu/drm/amd/amdgpu/psp_cmn.h | 84 > drivers/gpu/drm/amd/amdgpu/psp_funcs.h | 98 + > drivers/gpu/drm/amd/amdgpu/psp_ring.c | 354 > drivers/gpu/drm/amd/amdgpu/psp_ring.h | 43 ++ > drivers/gpu/drm/amd/amdgpu/psp_tmr.c | 84 > drivers/gpu/drm/amd/amdgpu/psp_tmr.h | 32 ++ > drivers/gpu/drm/amd/amdgpu/psp_v10_0.c | 381 + > drivers/gpu/drm/amd/amdgpu/psp_v11_0.c | 539 +--- > drivers/gpu/drm/amd/amdgpu/psp_v3_1.c | 480 + > drivers/gpu/drm/amd/amdgpu/psp_xgmi.c | 207 + > drivers/gpu/drm/amd/amdgpu/psp_xgmi.h | 33 ++ > drivers/gpu/drm/amd/amdgpu/soc15.c | 13 +- > 18 files changed, 1493 insertions(+), 1866 deletions(-) > create mode 100644 drivers/gpu/drm/amd/amdgpu/psp_asd.c > create mode 100644 drivers/gpu/drm/amd/amdgpu/psp_asd.h > create mode 100644 drivers/gpu/drm/amd/amdgpu/psp_cmn.c > create mode 100644 drivers/gpu/drm/amd/amdgpu/psp_cmn.h > create mode 100644 drivers/gpu/drm/amd/amdgpu/psp_funcs.h > create mode 100644 drivers/gpu/drm/amd/amdgpu/psp_ring.c > create mode 100644 drivers/gpu/drm/amd/amdgpu/psp_ring.h > create mode 100644 drivers/gpu/drm/amd/amdgpu/psp_tmr.c > create mode 100644 drivers/gpu/drm/amd/amdgpu/psp_tmr.h > create mode 100644 drivers/gpu/drm/amd/amdgpu/psp_xgmi.c > create mode 100644 drivers/gpu/drm/amd/amdgpu/psp_xgmi.h > ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [PATCH 0/9] PSP cleanup
I'd prefer to keep the old structures: common interfaces in amdgpu_psp.c/.h, and IP specific ones in IP specific file. No matter it's something related to ASD,TMR, or XGMI.etc, all of these are just communication/handshake jobs between driver and psp fw. Driver plays messenger role with several psp cmd that are shared among ASIC generations. a unified amdgpu_psp.c file is good enough to hold all the common stuffs. Regards, Hawking From: Christian K?nig Sent: Wednesday, January 2, 2019 6:01:56 PM To: Quan, Evan; amd-gfx@lists.freedesktop.org Cc: Deucher, Alexander; Xu, Feifei; Huang, Ray; Zhang, Hawking Subject: Re: [PATCH 0/9] PSP cleanup The general idea looks good, but can we change the file and symbol naming a bit? So far we have named all non-ip version related functions amdgpu_* and ip related functions ip_version. E.g. following this xgmi functions should go into amdgpu_xgmi.c and not psp_xgmi.c Christian. Am 02.01.19 um 10:21 schrieb Evan Quan: > *** BLURB HERE *** > > Evan Quan (9): >drm/amdgpu: separate the PSP ring related APIs >drm/amdgpu: separate commonly used PSP APIs >drm/amdgpu: separate the xgmi related APIs >drm/amdgpu: separate the tmr related APIs >drm/amdgpu: separate the asd related APIs >drm/amdgpu: drop useless PSP APIs and structures >drm/amdgpu: check PSP support before adding the ip block >drm/amdgpu: make PSP sub modules(ASD/XGMI/TMR) support configurable >drm/amdgpu: move psp_funcs related to a more proper place > > drivers/gpu/drm/amd/amdgpu/Makefile | 7 +- > drivers/gpu/drm/amd/amdgpu/amdgpu_psp.c | 504 +++--- > drivers/gpu/drm/amd/amdgpu/amdgpu_psp.h | 93 +--- > drivers/gpu/drm/amd/amdgpu/psp_asd.c| 86 > drivers/gpu/drm/amd/amdgpu/psp_asd.h| 32 ++ > drivers/gpu/drm/amd/amdgpu/psp_cmn.c| 289 + > drivers/gpu/drm/amd/amdgpu/psp_cmn.h| 84 > drivers/gpu/drm/amd/amdgpu/psp_funcs.h | 98 + > drivers/gpu/drm/amd/amdgpu/psp_ring.c | 354 > drivers/gpu/drm/amd/amdgpu/psp_ring.h | 43 ++ > drivers/gpu/drm/amd/amdgpu/psp_tmr.c| 84 > drivers/gpu/drm/amd/amdgpu/psp_tmr.h| 32 ++ > drivers/gpu/drm/amd/amdgpu/psp_v10_0.c | 381 + > drivers/gpu/drm/amd/amdgpu/psp_v11_0.c | 539 +--- > drivers/gpu/drm/amd/amdgpu/psp_v3_1.c | 480 + > drivers/gpu/drm/amd/amdgpu/psp_xgmi.c | 207 + > drivers/gpu/drm/amd/amdgpu/psp_xgmi.h | 33 ++ > drivers/gpu/drm/amd/amdgpu/soc15.c | 13 +- > 18 files changed, 1493 insertions(+), 1866 deletions(-) > create mode 100644 drivers/gpu/drm/amd/amdgpu/psp_asd.c > create mode 100644 drivers/gpu/drm/amd/amdgpu/psp_asd.h > create mode 100644 drivers/gpu/drm/amd/amdgpu/psp_cmn.c > create mode 100644 drivers/gpu/drm/amd/amdgpu/psp_cmn.h > create mode 100644 drivers/gpu/drm/amd/amdgpu/psp_funcs.h > create mode 100644 drivers/gpu/drm/amd/amdgpu/psp_ring.c > create mode 100644 drivers/gpu/drm/amd/amdgpu/psp_ring.h > create mode 100644 drivers/gpu/drm/amd/amdgpu/psp_tmr.c > create mode 100644 drivers/gpu/drm/amd/amdgpu/psp_tmr.h > create mode 100644 drivers/gpu/drm/amd/amdgpu/psp_xgmi.c > create mode 100644 drivers/gpu/drm/amd/amdgpu/psp_xgmi.h > ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [PATCH 0/9] PSP cleanup
The general idea looks good, but can we change the file and symbol naming a bit? So far we have named all non-ip version related functions amdgpu_* and ip related functions ip_version. E.g. following this xgmi functions should go into amdgpu_xgmi.c and not psp_xgmi.c Christian. Am 02.01.19 um 10:21 schrieb Evan Quan: *** BLURB HERE *** Evan Quan (9): drm/amdgpu: separate the PSP ring related APIs drm/amdgpu: separate commonly used PSP APIs drm/amdgpu: separate the xgmi related APIs drm/amdgpu: separate the tmr related APIs drm/amdgpu: separate the asd related APIs drm/amdgpu: drop useless PSP APIs and structures drm/amdgpu: check PSP support before adding the ip block drm/amdgpu: make PSP sub modules(ASD/XGMI/TMR) support configurable drm/amdgpu: move psp_funcs related to a more proper place drivers/gpu/drm/amd/amdgpu/Makefile | 7 +- drivers/gpu/drm/amd/amdgpu/amdgpu_psp.c | 504 +++--- drivers/gpu/drm/amd/amdgpu/amdgpu_psp.h | 93 +--- drivers/gpu/drm/amd/amdgpu/psp_asd.c| 86 drivers/gpu/drm/amd/amdgpu/psp_asd.h| 32 ++ drivers/gpu/drm/amd/amdgpu/psp_cmn.c| 289 + drivers/gpu/drm/amd/amdgpu/psp_cmn.h| 84 drivers/gpu/drm/amd/amdgpu/psp_funcs.h | 98 + drivers/gpu/drm/amd/amdgpu/psp_ring.c | 354 drivers/gpu/drm/amd/amdgpu/psp_ring.h | 43 ++ drivers/gpu/drm/amd/amdgpu/psp_tmr.c| 84 drivers/gpu/drm/amd/amdgpu/psp_tmr.h| 32 ++ drivers/gpu/drm/amd/amdgpu/psp_v10_0.c | 381 + drivers/gpu/drm/amd/amdgpu/psp_v11_0.c | 539 +--- drivers/gpu/drm/amd/amdgpu/psp_v3_1.c | 480 + drivers/gpu/drm/amd/amdgpu/psp_xgmi.c | 207 + drivers/gpu/drm/amd/amdgpu/psp_xgmi.h | 33 ++ drivers/gpu/drm/amd/amdgpu/soc15.c | 13 +- 18 files changed, 1493 insertions(+), 1866 deletions(-) create mode 100644 drivers/gpu/drm/amd/amdgpu/psp_asd.c create mode 100644 drivers/gpu/drm/amd/amdgpu/psp_asd.h create mode 100644 drivers/gpu/drm/amd/amdgpu/psp_cmn.c create mode 100644 drivers/gpu/drm/amd/amdgpu/psp_cmn.h create mode 100644 drivers/gpu/drm/amd/amdgpu/psp_funcs.h create mode 100644 drivers/gpu/drm/amd/amdgpu/psp_ring.c create mode 100644 drivers/gpu/drm/amd/amdgpu/psp_ring.h create mode 100644 drivers/gpu/drm/amd/amdgpu/psp_tmr.c create mode 100644 drivers/gpu/drm/amd/amdgpu/psp_tmr.h create mode 100644 drivers/gpu/drm/amd/amdgpu/psp_xgmi.c create mode 100644 drivers/gpu/drm/amd/amdgpu/psp_xgmi.h ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
