<html> <head> <meta http-equiv="Content-Type" content="text/html; charset=utf-8"> <meta name="Generator" content="Microsoft Exchange Server">  <style></style> </head> <body> <font face="Calibri" size="2"><span style="font-size:11pt;"> <div>Yep, We are in progress to move IP specific code into common interfaces. For instance, <b>psp_vXX_get_fw_type/psp_vXX_prep_cmd_buf</b> should be common interfaces, instead of IP specific ones. We’ll see that soon. </div> <div> </div> <div>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.</div> <div> </div> <div><b>For AS</b><b>D</b>, 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. </div> <div><font face="Calibri">+psp_load_asd(psp) \</font></div> <div><font face="Calibri">+ ((psp)->funcs->asd_load ? (psp)->funcs->asd_load((psp)) : 0) #define </font></div> <div><font face="Calibri">+psp_unload_asd(psp) \</font></div> <div><font face="Calibri">+ ((psp)->funcs->asd_unload ? (psp)->funcs->asd_unload((psp)) : 0) </font></div> <div><font face="Calibri">+#define psp_destory_asd(psp) \</font></div> <div><font face="Calibri">+ ((psp)->funcs->asd_destory ? (psp)->funcs->asd_destory((psp)) : 0) </font></div> <div><font face="Times New Roman"> </font></div> <div>Similar case <b>for TMR</b>. Therefore below interfaces seems unnecessary. </div> <div><font face="Calibri">+#define psp_init_tmr(psp) \</font></div> <div><font face="Calibri">+ ((psp)->funcs->tmr_init ? (psp)->funcs->tmr_init((psp)) : 0) #define </font></div> <div><font face="Calibri">+psp_load_tmr(psp) \</font></div> <div><font face="Calibri">+ ((psp)->funcs->tmr_load ? (psp)->funcs->tmr_load((psp)) : 0) #define </font></div> <div><font face="Calibri">+psp_unload_tmr(psp) \</font></div> <div><font face="Calibri">+ ((psp)->funcs->tmr_unload ? (psp)->funcs->tmr_unload((psp)) : 0) </font></div> <div><font face="Calibri">+#define psp_destory_tmr(psp) \</font></div> <div><font face="Calibri">+ ((psp)->funcs->tmr_destory ? (psp)->funcs->tmr_destory((psp)) : 0) </font></div> <div><font face="Times New Roman"> </font></div> <div><font face="Calibri"><b>For XGMI</b>, 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.</font></div> <div><font face="Calibri">+#define psp_init_xgmi(psp) \</font></div> <div><font face="Calibri">+ ((psp)->funcs->xgmi_init ? (psp)->funcs->xgmi_init((psp)) : 0) </font></div> <div><font face="Calibri">+#define psp_load_xgmi(psp) \</font></div> <div><font face="Calibri">+ ((psp)->funcs->xgmi_load ? (psp)->funcs->xgmi_load((psp)) : 0) </font></div> <div><font face="Calibri">+#define psp_unload_xgmi(psp) \</font></div> <div><font face="Calibri">+ ((psp)->funcs->xgmi_unload ? (psp)->funcs->xgmi_unload((psp)) : 0) </font></div> <div><font face="Calibri">+#define psp_destory_xgmi(psp) \</font></div> <div><font face="Calibri">+ ((psp)->funcs->xgmi_destory ? (psp)->funcs->xgmi_destory((psp)) : 0)</font></div> <div><font face="Times New Roman"> </font></div> <div><font face="Calibri">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? </font></div> <div><font face="Times New Roman"> </font></div> <div>In sum, I’d prefer to stick with the old structures: <font face="Calibri">common interfaces in amdgpu_psp.c/.h, and IP specific ones in IP specific file</font><font face="Calibri">. </font></div> <div><font face="Times New Roman"> </font></div> <div>Regards,<br> Hawking</div> <div><font face="DengXian"><b>From:</b> Christian König <ckoenig.leichtzumerken@gmail.com> <br> <b>Sent:</b> 2019<font face="微软雅黑">年</font>1<font face="微软雅黑">月</font>2<font face="微软雅黑">日</font> 20:00<br> <b>To:</b> Zhang, Hawking <Hawking.Zhang@amd.com>; Quan, Evan <Evan.Quan@amd.com>; amd-gfx@lists.freedesktop.org; Koenig, Christian <Christian.Koenig@amd.com><br> <b>Cc:</b> Deucher, Alexander <Alexander.Deucher@amd.com>; Xu, Feifei <Feifei.Xu@amd.com>; Huang, Ray <Ray.Huang@amd.com><br> <b>Subject:</b> Re: [PATCH 0/9] PSP cleanup</font></div> <div><font face="Times New Roman"> </font></div> <div style="margin-top:5pt;margin-bottom:5pt;"><font face="Calibri">I'd prefer to keep the old structures: common interfaces in amdgpu_psp.c/.h, and IP specific ones in IP specific file.</font></div> <div><font face="DengXian">That works for me as well.<br> <br> Key take away from the change overview is this:<br> </font></div> <div><font face="DengXian">> drivers/gpu/drm/amd/amdgpu/psp_v10_0.c | 381 +----------------<br> > drivers/gpu/drm/amd/amdgpu/psp_v11_0.c | 539 +-----------------------<br> > drivers/gpu/drm/amd/amdgpu/psp_v3_1.c | 480 +--------------------</font></div> <div><font face="DengXian">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.<br> <br> No strong opinion if the common code should go into amdgpu_psp.c/h, amdgpu_xgmi.c/h or amdgpu_psp_xgmi.c/h.<br> <br> The only restriction I have is that we should just stick with the existing naming convention.<br> <br> Christian.<br> <br> Am 02.01.19 um 11:21 schrieb Zhang, Hawking:</font></div> <div style="margin-top:5pt;margin-bottom:5pt;"><font face="Calibri" size="3"><span style="font-size:12pt;">I'd prefer to keep the old structures: common interfaces in amdgpu_psp.c/.h, and IP specific ones in IP specific file.</span></font></div> <div style="margin-top:5pt;margin-bottom:5pt;"><font face="Calibri" size="3"><span style="font-size:12pt;"> </span></font></div> <div style="margin-top:5pt;margin-bottom:5pt;"><font face="Calibri" size="3"><span style="font-size:12pt;">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. </span></font></div> <div style="margin-top:5pt;margin-bottom:5pt;"><font face="Calibri" size="3"><span style="font-size:12pt;"> </span></font></div> <div style="margin-top:5pt;margin-bottom:5pt;"><font face="Calibri" size="3"><span style="font-size:12pt;">Regards,<br> Hawking</span></font></div> <div style="text-align:center;"><font face="DengXian"><img width="83" height="41" src="rtfimage://"></font></div> <div><font face="DengXian"><b>From:</b> Christian König <a href="mailto:ckoenig.leichtzumerken@gmail.com"><font color="blue"><u><ckoenig.leichtzumerken@gmail.com></u></font></a><br> <b>Sent:</b> Wednesday, January 2, 2019 6:01:56 PM<br> <b>To:</b> Quan, Evan; <a href="mailto:amd-gfx@lists.freedesktop.org"><font color="blue"><u>amd-gfx@lists.freedesktop.org</u></font></a><br> <b>Cc:</b> Deucher, Alexander; Xu, Feifei; Huang, Ray; Zhang, Hawking<br> <b>Subject:</b> Re: [PATCH 0/9] PSP cleanup </font></div> <div><font face="DengXian"> </font></div> <div style="margin-bottom:12pt;"><font face="DengXian">The general idea looks good, but can we change the file and symbol <br> naming a bit?<br> <br> So far we have named all non-ip version related functions amdgpu_* and <br> ip related functions ip_version.<br> <br> E.g. following this xgmi functions should go into amdgpu_xgmi.c and not <br> psp_xgmi.c<br> <br> Christian.<br> <br> Am 02.01.19 um 10:21 schrieb Evan Quan:<br> > *** BLURB HERE ***<br> ><br> > Evan Quan (9):<br> > drm/amdgpu: separate the PSP ring related APIs<br> > drm/amdgpu: separate commonly used PSP APIs<br> > drm/amdgpu: separate the xgmi related APIs<br> > drm/amdgpu: separate the tmr related APIs<br> > drm/amdgpu: separate the asd related APIs<br> > drm/amdgpu: drop useless PSP APIs and structures<br> > drm/amdgpu: check PSP support before adding the ip block<br> > drm/amdgpu: make PSP sub modules(ASD/XGMI/TMR) support configurable<br> > drm/amdgpu: move psp_funcs related to a more proper place<br> ><br> > drivers/gpu/drm/amd/amdgpu/Makefile | 7 +-<br> > drivers/gpu/drm/amd/amdgpu/amdgpu_psp.c | 504 +++-------------------<br> > drivers/gpu/drm/amd/amdgpu/amdgpu_psp.h | 93 +---<br> > drivers/gpu/drm/amd/amdgpu/psp_asd.c | 86 ++++<br> > drivers/gpu/drm/amd/amdgpu/psp_asd.h | 32 ++<br> > drivers/gpu/drm/amd/amdgpu/psp_cmn.c | 289 +++++++++++++<br> > drivers/gpu/drm/amd/amdgpu/psp_cmn.h | 84 ++++<br> > drivers/gpu/drm/amd/amdgpu/psp_funcs.h | 98 +++++<br> > drivers/gpu/drm/amd/amdgpu/psp_ring.c | 354 ++++++++++++++++<br> > drivers/gpu/drm/amd/amdgpu/psp_ring.h | 43 ++<br> > drivers/gpu/drm/amd/amdgpu/psp_tmr.c | 84 ++++<br> > drivers/gpu/drm/amd/amdgpu/psp_tmr.h | 32 ++<br> > drivers/gpu/drm/amd/amdgpu/psp_v10_0.c | 381 +----------------<br> > drivers/gpu/drm/amd/amdgpu/psp_v11_0.c | 539 +-----------------------<br> > drivers/gpu/drm/amd/amdgpu/psp_v3_1.c | 480 +--------------------<br> > drivers/gpu/drm/amd/amdgpu/psp_xgmi.c | 207 +++++++++<br> > drivers/gpu/drm/amd/amdgpu/psp_xgmi.h | 33 ++<br> > drivers/gpu/drm/amd/amdgpu/soc15.c | 13 +-<br> > 18 files changed, 1493 insertions(+), 1866 deletions(-)<br> > create mode 100644 drivers/gpu/drm/amd/amdgpu/psp_asd.c<br> > create mode 100644 drivers/gpu/drm/amd/amdgpu/psp_asd.h<br> > create mode 100644 drivers/gpu/drm/amd/amdgpu/psp_cmn.c<br> > create mode 100644 drivers/gpu/drm/amd/amdgpu/psp_cmn.h<br> > create mode 100644 drivers/gpu/drm/amd/amdgpu/psp_funcs.h<br> > create mode 100644 drivers/gpu/drm/amd/amdgpu/psp_ring.c<br> > create mode 100644 drivers/gpu/drm/amd/amdgpu/psp_ring.h<br> > create mode 100644 drivers/gpu/drm/amd/amdgpu/psp_tmr.c<br> > create mode 100644 drivers/gpu/drm/amd/amdgpu/psp_tmr.h<br> > create mode 100644 drivers/gpu/drm/amd/amdgpu/psp_xgmi.c<br> > create mode 100644 drivers/gpu/drm/amd/amdgpu/psp_xgmi.h<br> ></font></div> <div><font face="Times New Roman"><br> </font></div> <div><font face="Courier New" size="2"><span style="font-size:10pt;">_______________________________________________</span></font></div> <div><font face="Courier New" size="2"><span style="font-size:10pt;">amd-gfx mailing list</span></font></div> <div><font face="Times New Roman" size="2"><span style="font-size:10pt;"><a href="mailto:amd-gfx@lists.freedesktop.org"><font face="Courier New" color="blue"><u>amd-gfx@lists.freedesktop.org</u></font></a></span></font></div> <div><font face="Times New Roman" size="2"><span style="font-size:10pt;"><a href="https://lists.freedesktop.org/mailman/listinfo/amd-gfx"><font face="Courier New" color="blue"><u>https://lists.freedesktop.org/mailman/listinfo/amd-gfx</u></font></a></span></font></div> <div><font face="Times New Roman"> </font></div> <div><font face="Times New Roman"> </font></div> </span></font> </body> </html>