RE: [PATCH 0/9] PSP cleanup

["Zhang, Hawking" <Hawking.Zhang@xxxxxxx>]

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.leichtzumerken@xxxxxxxxx>
Sent: 2019年1月2日 20:00
To: Zhang, Hawking <Hawking.Zhang@xxxxxxx>; Quan, Evan <Evan.Quan@xxxxxxx>; amd-gfx@xxxxxxxxxxxxxxxxxxxxx; Koenig, Christian <Christian.Koenig@xxxxxxx>
Cc: Deucher, Alexander <Alexander.Deucher@xxxxxxx>; Xu, Feifei <Feifei.Xu@xxxxxxx>; Huang, Ray <Ray.Huang@xxxxxxx>
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

From: Christian König <ckoenig.leichtzumerken@xxxxxxxxx>
Sent: Wednesday, January 2, 2019 6:01:56 PM
To: Quan, Evan; amd-gfx@xxxxxxxxxxxxxxxxxxxxx
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@xxxxxxxxxxxxxxxxxxxxx

https://lists.freedesktop.org/mailman/listinfo/amd-gfx

 

 

_______________________________________________
amd-gfx mailing list
amd-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/amd-gfx