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 <ckoenig.leichtzumer...@gmail.com>
Sent: 2019年1月2日 20:00
To: Zhang, Hawking <hawking.zh...@amd.com>; Quan, Evan <evan.q...@amd.com>; 
amd-gfx@lists.freedesktop.org; Koenig, Christian <christian.koe...@amd.com>
Cc: Deucher, Alexander <alexander.deuc...@amd.com>; Xu, Feifei 
<feifei...@amd.com>; Huang, Ray <ray.hu...@amd.com>
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 
<ckoenig.leichtzumer...@gmail.com><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 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<mailto: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

Reply via email to