[v1,00/10] baseband/acc200

Message ID	1657238503-143836-1-git-send-email-nicolas.chautru@intel.com (mailing list archive)
Headers	From: Nicolas Chautru <nicolas.chautru@intel.com> To: dev@dpdk.org, thomas@monjalon.net, gakhil@marvell.com, hemant.agrawal@nxp.com, trix@redhat.com Cc: maxime.coquelin@redhat.com, mdr@ashroe.eu, bruce.richardson@intel.com, david.marchand@redhat.com, stephen@networkplumber.org, Nicolas Chautru <nicolas.chautru@intel.com> Subject: [PATCH v1 00/10] baseband/acc200 Date: Thu, 7 Jul 2022 17:01:33 -0700 Message-Id: <1657238503-143836-1-git-send-email-nicolas.chautru@intel.com> Precedence: list Errors-To: dev-bounces@dpdk.org
Series	baseband/acc200 \| [v1,00/10] baseband/acc200 [v1,01/10] baseband/acc200: introduce PMD for ACC200 [v1,02/10] baseband/acc200: add HW register definitions [v1,03/10] baseband/acc200: add info get function [v1,04/10] baseband/acc200: add queue configuration [v1,05/10] baseband/acc200: add LDPC processing functions [v1,06/10] baseband/acc200: add LTE processing functions [v1,07/10] baseband/acc200: add support for FFT operations [v1,08/10] baseband/acc200: support interrupt [v1,09/10] baseband/acc200: add device status and vf2pf comms [v1,10/10] baseband/acc200: add PF configure companion function

Message ID

1657238503-143836-1-git-send-email-nicolas.chautru@intel.com (mailing list archive)

Headers

From: Nicolas Chautru <nicolas.chautru@intel.com>
To: dev@dpdk.org, thomas@monjalon.net, gakhil@marvell.com,
 hemant.agrawal@nxp.com, trix@redhat.com
Cc: maxime.coquelin@redhat.com, mdr@ashroe.eu, bruce.richardson@intel.com,
 david.marchand@redhat.com, stephen@networkplumber.org,
 Nicolas Chautru <nicolas.chautru@intel.com>
Subject: [PATCH v1 00/10] baseband/acc200
Date: Thu,  7 Jul 2022 17:01:33 -0700
Message-Id: <1657238503-143836-1-git-send-email-nicolas.chautru@intel.com>
Precedence: list
Errors-To: dev-bounces@dpdk.org

Series

baseband/acc200 |

Message

Chautru, Nicolas July 8, 2022, 12:01 a.m. UTC

  This is targeting 22.11 and includes the PMD for the 
integrated accelerator on Intel Xeon SPR-EEC. 
There is a dependency on that parallel serie still in-flight
which extends the bbdev api https://patches.dpdk.org/project/dpdk/list/?series=23894

I will be offline for a few weeks for the summer break but
Hernan will cover for me during that time if required.

Thanks
Nic

Nicolas Chautru (10):
  baseband/acc200: introduce PMD for ACC200
  baseband/acc200: add HW register definitions
  baseband/acc200: add info get function
  baseband/acc200: add queue configuration
  baseband/acc200: add LDPC processing functions
  baseband/acc200: add LTE processing functions
  baseband/acc200: add support for FFT operations
  baseband/acc200: support interrupt
  baseband/acc200: add device status and vf2pf comms
  baseband/acc200: add PF configure companion function

 MAINTAINERS                              |    3 +
 app/test-bbdev/meson.build               |    3 +
 app/test-bbdev/test_bbdev_perf.c         |   76 +
 doc/guides/bbdevs/acc200.rst             |  244 ++
 doc/guides/bbdevs/index.rst              |    1 +
 drivers/baseband/acc200/acc200_pf_enum.h |  468 +++
 drivers/baseband/acc200/acc200_pmd.h     |  690 ++++
 drivers/baseband/acc200/acc200_vf_enum.h |   89 +
 drivers/baseband/acc200/meson.build      |    8 +
 drivers/baseband/acc200/rte_acc200_cfg.h |  115 +
 drivers/baseband/acc200/rte_acc200_pmd.c | 5403 ++++++++++++++++++++++++++++++
 drivers/baseband/acc200/version.map      |   10 +
 drivers/baseband/meson.build             |    1 +
 13 files changed, 7111 insertions(+)
 create mode 100644 doc/guides/bbdevs/acc200.rst
 create mode 100644 drivers/baseband/acc200/acc200_pf_enum.h
 create mode 100644 drivers/baseband/acc200/acc200_pmd.h
 create mode 100644 drivers/baseband/acc200/acc200_vf_enum.h
 create mode 100644 drivers/baseband/acc200/meson.build
 create mode 100644 drivers/baseband/acc200/rte_acc200_cfg.h
 create mode 100644 drivers/baseband/acc200/rte_acc200_pmd.c
 create mode 100644 drivers/baseband/acc200/version.map

Comments

Maxime Coquelin July 12, 2022, 1:48 p.m. UTC | #1

Hi Nicolas, Hernan,

(Adding Hernan in the recipients list)

On 7/8/22 02:01, Nicolas Chautru wrote:
> This is targeting 22.11 and includes the PMD for the
> integrated accelerator on Intel Xeon SPR-EEC.
> There is a dependency on that parallel serie still in-flight
> which extends the bbdev api https://patches.dpdk.org/project/dpdk/list/?series=23894
> 
> I will be offline for a few weeks for the summer break but
> Hernan will cover for me during that time if required.
> 
> Thanks
> Nic
> 
> Nicolas Chautru (10):
>    baseband/acc200: introduce PMD for ACC200
>    baseband/acc200: add HW register definitions
>    baseband/acc200: add info get function
>    baseband/acc200: add queue configuration
>    baseband/acc200: add LDPC processing functions
>    baseband/acc200: add LTE processing functions
>    baseband/acc200: add support for FFT operations
>    baseband/acc200: support interrupt
>    baseband/acc200: add device status and vf2pf comms
>    baseband/acc200: add PF configure companion function
> 
>   MAINTAINERS                              |    3 +
>   app/test-bbdev/meson.build               |    3 +
>   app/test-bbdev/test_bbdev_perf.c         |   76 +
>   doc/guides/bbdevs/acc200.rst             |  244 ++
>   doc/guides/bbdevs/index.rst              |    1 +
>   drivers/baseband/acc200/acc200_pf_enum.h |  468 +++
>   drivers/baseband/acc200/acc200_pmd.h     |  690 ++++
>   drivers/baseband/acc200/acc200_vf_enum.h |   89 +
>   drivers/baseband/acc200/meson.build      |    8 +
>   drivers/baseband/acc200/rte_acc200_cfg.h |  115 +
>   drivers/baseband/acc200/rte_acc200_pmd.c | 5403 ++++++++++++++++++++++++++++++
>   drivers/baseband/acc200/version.map      |   10 +
>   drivers/baseband/meson.build             |    1 +
>   13 files changed, 7111 insertions(+)
>   create mode 100644 doc/guides/bbdevs/acc200.rst
>   create mode 100644 drivers/baseband/acc200/acc200_pf_enum.h
>   create mode 100644 drivers/baseband/acc200/acc200_pmd.h
>   create mode 100644 drivers/baseband/acc200/acc200_vf_enum.h
>   create mode 100644 drivers/baseband/acc200/meson.build
>   create mode 100644 drivers/baseband/acc200/rte_acc200_cfg.h
>   create mode 100644 drivers/baseband/acc200/rte_acc200_pmd.c
>   create mode 100644 drivers/baseband/acc200/version.map
> 

Comparing ACC200 & ACC100 header files, I understand ACC200 is an
evolution of the ACC10x family. The FEC bits are really close, ACC200
main addition seems to be FFT acceleration which could be handled in
ACC10x driver based on device ID.

I think both drivers have to be merged in order to avoid code
duplication. That's how other families of devices (e.g. i40e) are
handled.

Thanks,
Maxime

Vargas, Hernan July 14, 2022, 6:49 p.m. UTC | #2

Hi Tom, Maxime,

Could you please review the v5 series that Nic submitted last week?
https://patches.dpdk.org/project/dpdk/list/?series=23912

Thanks,
Hernan


-----Original Message-----
From: Maxime Coquelin <maxime.coquelin@redhat.com> 
Sent: Tuesday, July 12, 2022 8:49 AM
To: Chautru, Nicolas <nicolas.chautru@intel.com>; dev@dpdk.org; thomas@monjalon.net; gakhil@marvell.com; hemant.agrawal@nxp.com; trix@redhat.com; Vargas, Hernan <hernan.vargas@intel.com>
Cc: mdr@ashroe.eu; Richardson, Bruce <bruce.richardson@intel.com>; david.marchand@redhat.com; stephen@networkplumber.org
Subject: Re: [PATCH v1 00/10] baseband/acc200

Hi Nicolas, Hernan,

(Adding Hernan in the recipients list)

On 7/8/22 02:01, Nicolas Chautru wrote:
> This is targeting 22.11 and includes the PMD for the integrated 
> accelerator on Intel Xeon SPR-EEC.
> There is a dependency on that parallel serie still in-flight which 
> extends the bbdev api 
> https://patches.dpdk.org/project/dpdk/list/?series=23894
> 
> I will be offline for a few weeks for the summer break but Hernan will 
> cover for me during that time if required.
> 
> Thanks
> Nic
> 
> Nicolas Chautru (10):
>    baseband/acc200: introduce PMD for ACC200
>    baseband/acc200: add HW register definitions
>    baseband/acc200: add info get function
>    baseband/acc200: add queue configuration
>    baseband/acc200: add LDPC processing functions
>    baseband/acc200: add LTE processing functions
>    baseband/acc200: add support for FFT operations
>    baseband/acc200: support interrupt
>    baseband/acc200: add device status and vf2pf comms
>    baseband/acc200: add PF configure companion function
> 
>   MAINTAINERS                              |    3 +
>   app/test-bbdev/meson.build               |    3 +
>   app/test-bbdev/test_bbdev_perf.c         |   76 +
>   doc/guides/bbdevs/acc200.rst             |  244 ++
>   doc/guides/bbdevs/index.rst              |    1 +
>   drivers/baseband/acc200/acc200_pf_enum.h |  468 +++
>   drivers/baseband/acc200/acc200_pmd.h     |  690 ++++
>   drivers/baseband/acc200/acc200_vf_enum.h |   89 +
>   drivers/baseband/acc200/meson.build      |    8 +
>   drivers/baseband/acc200/rte_acc200_cfg.h |  115 +
>   drivers/baseband/acc200/rte_acc200_pmd.c | 5403 ++++++++++++++++++++++++++++++
>   drivers/baseband/acc200/version.map      |   10 +
>   drivers/baseband/meson.build             |    1 +
>   13 files changed, 7111 insertions(+)
>   create mode 100644 doc/guides/bbdevs/acc200.rst
>   create mode 100644 drivers/baseband/acc200/acc200_pf_enum.h
>   create mode 100644 drivers/baseband/acc200/acc200_pmd.h
>   create mode 100644 drivers/baseband/acc200/acc200_vf_enum.h
>   create mode 100644 drivers/baseband/acc200/meson.build
>   create mode 100644 drivers/baseband/acc200/rte_acc200_cfg.h
>   create mode 100644 drivers/baseband/acc200/rte_acc200_pmd.c
>   create mode 100644 drivers/baseband/acc200/version.map
> 

Comparing ACC200 & ACC100 header files, I understand ACC200 is an evolution of the ACC10x family. The FEC bits are really close, ACC200 main addition seems to be FFT acceleration which could be handled in ACC10x driver based on device ID.

I think both drivers have to be merged in order to avoid code duplication. That's how other families of devices (e.g. i40e) are handled.

Thanks,
Maxime

Tom Rix July 17, 2022, 1:08 p.m. UTC | #3

On 7/14/22 11:49 AM, Vargas, Hernan wrote:
> Hi Tom, Maxime,
>
> Could you please review the v5 series that Nic submitted last week?
> https://patches.dpdk.org/project/dpdk/list/?series=23912
>
> Thanks,
> Hernan

Hernan,

For this patch series for the acc200, will you be able to refactor it so 
acc has a common base ?

Or will this be on hold until Nic is back ?

Tom

>
>
> -----Original Message-----
> From: Maxime Coquelin <maxime.coquelin@redhat.com>
> Sent: Tuesday, July 12, 2022 8:49 AM
> To: Chautru, Nicolas <nicolas.chautru@intel.com>; dev@dpdk.org; thomas@monjalon.net; gakhil@marvell.com; hemant.agrawal@nxp.com; trix@redhat.com; Vargas, Hernan <hernan.vargas@intel.com>
> Cc: mdr@ashroe.eu; Richardson, Bruce <bruce.richardson@intel.com>; david.marchand@redhat.com; stephen@networkplumber.org
> Subject: Re: [PATCH v1 00/10] baseband/acc200
>
> Hi Nicolas, Hernan,
>
> (Adding Hernan in the recipients list)
>
> On 7/8/22 02:01, Nicolas Chautru wrote:
>> This is targeting 22.11 and includes the PMD for the integrated
>> accelerator on Intel Xeon SPR-EEC.
>> There is a dependency on that parallel serie still in-flight which
>> extends the bbdev api
>> https://patches.dpdk.org/project/dpdk/list/?series=23894
>>
>> I will be offline for a few weeks for the summer break but Hernan will
>> cover for me during that time if required.
>>
>> Thanks
>> Nic
>>
>> Nicolas Chautru (10):
>>     baseband/acc200: introduce PMD for ACC200
>>     baseband/acc200: add HW register definitions
>>     baseband/acc200: add info get function
>>     baseband/acc200: add queue configuration
>>     baseband/acc200: add LDPC processing functions
>>     baseband/acc200: add LTE processing functions
>>     baseband/acc200: add support for FFT operations
>>     baseband/acc200: support interrupt
>>     baseband/acc200: add device status and vf2pf comms
>>     baseband/acc200: add PF configure companion function
>>
>>    MAINTAINERS                              |    3 +
>>    app/test-bbdev/meson.build               |    3 +
>>    app/test-bbdev/test_bbdev_perf.c         |   76 +
>>    doc/guides/bbdevs/acc200.rst             |  244 ++
>>    doc/guides/bbdevs/index.rst              |    1 +
>>    drivers/baseband/acc200/acc200_pf_enum.h |  468 +++
>>    drivers/baseband/acc200/acc200_pmd.h     |  690 ++++
>>    drivers/baseband/acc200/acc200_vf_enum.h |   89 +
>>    drivers/baseband/acc200/meson.build      |    8 +
>>    drivers/baseband/acc200/rte_acc200_cfg.h |  115 +
>>    drivers/baseband/acc200/rte_acc200_pmd.c | 5403 ++++++++++++++++++++++++++++++
>>    drivers/baseband/acc200/version.map      |   10 +
>>    drivers/baseband/meson.build             |    1 +
>>    13 files changed, 7111 insertions(+)
>>    create mode 100644 doc/guides/bbdevs/acc200.rst
>>    create mode 100644 drivers/baseband/acc200/acc200_pf_enum.h
>>    create mode 100644 drivers/baseband/acc200/acc200_pmd.h
>>    create mode 100644 drivers/baseband/acc200/acc200_vf_enum.h
>>    create mode 100644 drivers/baseband/acc200/meson.build
>>    create mode 100644 drivers/baseband/acc200/rte_acc200_cfg.h
>>    create mode 100644 drivers/baseband/acc200/rte_acc200_pmd.c
>>    create mode 100644 drivers/baseband/acc200/version.map
>>
> Comparing ACC200 & ACC100 header files, I understand ACC200 is an evolution of the ACC10x family. The FEC bits are really close, ACC200 main addition seems to be FFT acceleration which could be handled in ACC10x driver based on device ID.
>
> I think both drivers have to be merged in order to avoid code duplication. That's how other families of devices (e.g. i40e) are handled.
>
> Thanks,
> Maxime
>

Vargas, Hernan July 22, 2022, 6:29 p.m. UTC | #4

Hi Tom,

The patch series for the ACC200 can wait until Nic's back.
Our priority are the changes for the bbdev API here: https://patches.dpdk.org/project/dpdk/list/?series=23912

Thanks,
Hernan

-----Original Message-----
From: Tom Rix <trix@redhat.com> 
Sent: Sunday, July 17, 2022 8:08 AM
To: Vargas, Hernan <hernan.vargas@intel.com>; Maxime Coquelin <maxime.coquelin@redhat.com>; Chautru, Nicolas <nicolas.chautru@intel.com>; dev@dpdk.org; thomas@monjalon.net; gakhil@marvell.com; hemant.agrawal@nxp.com
Cc: mdr@ashroe.eu; Richardson, Bruce <bruce.richardson@intel.com>; david.marchand@redhat.com; stephen@networkplumber.org
Subject: Re: [PATCH v1 00/10] baseband/acc200


On 7/14/22 11:49 AM, Vargas, Hernan wrote:
> Hi Tom, Maxime,
>
> Could you please review the v5 series that Nic submitted last week?
> https://patches.dpdk.org/project/dpdk/list/?series=23912
>
> Thanks,
> Hernan

Hernan,

For this patch series for the acc200, will you be able to refactor it so acc has a common base ?

Or will this be on hold until Nic is back ?

Tom

>
>
> -----Original Message-----
> From: Maxime Coquelin <maxime.coquelin@redhat.com>
> Sent: Tuesday, July 12, 2022 8:49 AM
> To: Chautru, Nicolas <nicolas.chautru@intel.com>; dev@dpdk.org; 
> thomas@monjalon.net; gakhil@marvell.com; hemant.agrawal@nxp.com; 
> trix@redhat.com; Vargas, Hernan <hernan.vargas@intel.com>
> Cc: mdr@ashroe.eu; Richardson, Bruce <bruce.richardson@intel.com>; 
> david.marchand@redhat.com; stephen@networkplumber.org
> Subject: Re: [PATCH v1 00/10] baseband/acc200
>
> Hi Nicolas, Hernan,
>
> (Adding Hernan in the recipients list)
>
> On 7/8/22 02:01, Nicolas Chautru wrote:
>> This is targeting 22.11 and includes the PMD for the integrated 
>> accelerator on Intel Xeon SPR-EEC.
>> There is a dependency on that parallel serie still in-flight which 
>> extends the bbdev api
>> https://patches.dpdk.org/project/dpdk/list/?series=23894
>>
>> I will be offline for a few weeks for the summer break but Hernan 
>> will cover for me during that time if required.
>>
>> Thanks
>> Nic
>>
>> Nicolas Chautru (10):
>>     baseband/acc200: introduce PMD for ACC200
>>     baseband/acc200: add HW register definitions
>>     baseband/acc200: add info get function
>>     baseband/acc200: add queue configuration
>>     baseband/acc200: add LDPC processing functions
>>     baseband/acc200: add LTE processing functions
>>     baseband/acc200: add support for FFT operations
>>     baseband/acc200: support interrupt
>>     baseband/acc200: add device status and vf2pf comms
>>     baseband/acc200: add PF configure companion function
>>
>>    MAINTAINERS                              |    3 +
>>    app/test-bbdev/meson.build               |    3 +
>>    app/test-bbdev/test_bbdev_perf.c         |   76 +
>>    doc/guides/bbdevs/acc200.rst             |  244 ++
>>    doc/guides/bbdevs/index.rst              |    1 +
>>    drivers/baseband/acc200/acc200_pf_enum.h |  468 +++
>>    drivers/baseband/acc200/acc200_pmd.h     |  690 ++++
>>    drivers/baseband/acc200/acc200_vf_enum.h |   89 +
>>    drivers/baseband/acc200/meson.build      |    8 +
>>    drivers/baseband/acc200/rte_acc200_cfg.h |  115 +
>>    drivers/baseband/acc200/rte_acc200_pmd.c | 5403 ++++++++++++++++++++++++++++++
>>    drivers/baseband/acc200/version.map      |   10 +
>>    drivers/baseband/meson.build             |    1 +
>>    13 files changed, 7111 insertions(+)
>>    create mode 100644 doc/guides/bbdevs/acc200.rst
>>    create mode 100644 drivers/baseband/acc200/acc200_pf_enum.h
>>    create mode 100644 drivers/baseband/acc200/acc200_pmd.h
>>    create mode 100644 drivers/baseband/acc200/acc200_vf_enum.h
>>    create mode 100644 drivers/baseband/acc200/meson.build
>>    create mode 100644 drivers/baseband/acc200/rte_acc200_cfg.h
>>    create mode 100644 drivers/baseband/acc200/rte_acc200_pmd.c
>>    create mode 100644 drivers/baseband/acc200/version.map
>>
> Comparing ACC200 & ACC100 header files, I understand ACC200 is an evolution of the ACC10x family. The FEC bits are really close, ACC200 main addition seems to be FFT acceleration which could be handled in ACC10x driver based on device ID.
>
> I think both drivers have to be merged in order to avoid code duplication. That's how other families of devices (e.g. i40e) are handled.
>
> Thanks,
> Maxime
>

Tom Rix July 22, 2022, 8:19 p.m. UTC | #5

Hernan

The changes I requested in v4, were not addressed in v5.

Can you make these changes for v6?

Tom

On 7/22/22 11:29 AM, Vargas, Hernan wrote:
> Hi Tom,
>
> The patch series for the ACC200 can wait until Nic's back.
> Our priority are the changes for the bbdev API here: https://patches.dpdk.org/project/dpdk/list/?series=23912
>
> Thanks,
> Hernan
>
> -----Original Message-----
> From: Tom Rix <trix@redhat.com>
> Sent: Sunday, July 17, 2022 8:08 AM
> To: Vargas, Hernan <hernan.vargas@intel.com>; Maxime Coquelin <maxime.coquelin@redhat.com>; Chautru, Nicolas <nicolas.chautru@intel.com>; dev@dpdk.org; thomas@monjalon.net; gakhil@marvell.com; hemant.agrawal@nxp.com
> Cc: mdr@ashroe.eu; Richardson, Bruce <bruce.richardson@intel.com>; david.marchand@redhat.com; stephen@networkplumber.org
> Subject: Re: [PATCH v1 00/10] baseband/acc200
>
>
> On 7/14/22 11:49 AM, Vargas, Hernan wrote:
>> Hi Tom, Maxime,
>>
>> Could you please review the v5 series that Nic submitted last week?
>> https://patches.dpdk.org/project/dpdk/list/?series=23912
>>
>> Thanks,
>> Hernan
> Hernan,
>
> For this patch series for the acc200, will you be able to refactor it so acc has a common base ?
>
> Or will this be on hold until Nic is back ?
>
> Tom
>
>>
>> -----Original Message-----
>> From: Maxime Coquelin <maxime.coquelin@redhat.com>
>> Sent: Tuesday, July 12, 2022 8:49 AM
>> To: Chautru, Nicolas <nicolas.chautru@intel.com>; dev@dpdk.org;
>> thomas@monjalon.net; gakhil@marvell.com; hemant.agrawal@nxp.com;
>> trix@redhat.com; Vargas, Hernan <hernan.vargas@intel.com>
>> Cc: mdr@ashroe.eu; Richardson, Bruce <bruce.richardson@intel.com>;
>> david.marchand@redhat.com; stephen@networkplumber.org
>> Subject: Re: [PATCH v1 00/10] baseband/acc200
>>
>> Hi Nicolas, Hernan,
>>
>> (Adding Hernan in the recipients list)
>>
>> On 7/8/22 02:01, Nicolas Chautru wrote:
>>> This is targeting 22.11 and includes the PMD for the integrated
>>> accelerator on Intel Xeon SPR-EEC.
>>> There is a dependency on that parallel serie still in-flight which
>>> extends the bbdev api
>>> https://patches.dpdk.org/project/dpdk/list/?series=23894
>>>
>>> I will be offline for a few weeks for the summer break but Hernan
>>> will cover for me during that time if required.
>>>
>>> Thanks
>>> Nic
>>>
>>> Nicolas Chautru (10):
>>>      baseband/acc200: introduce PMD for ACC200
>>>      baseband/acc200: add HW register definitions
>>>      baseband/acc200: add info get function
>>>      baseband/acc200: add queue configuration
>>>      baseband/acc200: add LDPC processing functions
>>>      baseband/acc200: add LTE processing functions
>>>      baseband/acc200: add support for FFT operations
>>>      baseband/acc200: support interrupt
>>>      baseband/acc200: add device status and vf2pf comms
>>>      baseband/acc200: add PF configure companion function
>>>
>>>     MAINTAINERS                              |    3 +
>>>     app/test-bbdev/meson.build               |    3 +
>>>     app/test-bbdev/test_bbdev_perf.c         |   76 +
>>>     doc/guides/bbdevs/acc200.rst             |  244 ++
>>>     doc/guides/bbdevs/index.rst              |    1 +
>>>     drivers/baseband/acc200/acc200_pf_enum.h |  468 +++
>>>     drivers/baseband/acc200/acc200_pmd.h     |  690 ++++
>>>     drivers/baseband/acc200/acc200_vf_enum.h |   89 +
>>>     drivers/baseband/acc200/meson.build      |    8 +
>>>     drivers/baseband/acc200/rte_acc200_cfg.h |  115 +
>>>     drivers/baseband/acc200/rte_acc200_pmd.c | 5403 ++++++++++++++++++++++++++++++
>>>     drivers/baseband/acc200/version.map      |   10 +
>>>     drivers/baseband/meson.build             |    1 +
>>>     13 files changed, 7111 insertions(+)
>>>     create mode 100644 doc/guides/bbdevs/acc200.rst
>>>     create mode 100644 drivers/baseband/acc200/acc200_pf_enum.h
>>>     create mode 100644 drivers/baseband/acc200/acc200_pmd.h
>>>     create mode 100644 drivers/baseband/acc200/acc200_vf_enum.h
>>>     create mode 100644 drivers/baseband/acc200/meson.build
>>>     create mode 100644 drivers/baseband/acc200/rte_acc200_cfg.h
>>>     create mode 100644 drivers/baseband/acc200/rte_acc200_pmd.c
>>>     create mode 100644 drivers/baseband/acc200/version.map
>>>
>> Comparing ACC200 & ACC100 header files, I understand ACC200 is an evolution of the ACC10x family. The FEC bits are really close, ACC200 main addition seems to be FFT acceleration which could be handled in ACC10x driver based on device ID.
>>
>> I think both drivers have to be merged in order to avoid code duplication. That's how other families of devices (e.g. i40e) are handled.
>>
>> Thanks,
>> Maxime
>>

Chautru, Nicolas Aug. 15, 2022, 5:52 p.m. UTC | #6

Hi Tom, 
I had answered all of your comments from v4 before I went on time off. 
Let me know if any concern acking that v5, thanks
Nic

> -----Original Message-----
> From: Tom Rix <trix@redhat.com>
> Sent: Friday, July 22, 2022 1:20 PM
> To: Vargas, Hernan <hernan.vargas@intel.com>; Maxime Coquelin
> <maxime.coquelin@redhat.com>; Chautru, Nicolas
> <nicolas.chautru@intel.com>; dev@dpdk.org; thomas@monjalon.net;
> gakhil@marvell.com; hemant.agrawal@nxp.com
> Cc: mdr@ashroe.eu; Richardson, Bruce <bruce.richardson@intel.com>;
> david.marchand@redhat.com; stephen@networkplumber.org
> Subject: Re: [PATCH v1 00/10] baseband/acc200
> 
> Hernan
> 
> The changes I requested in v4, were not addressed in v5.
> 
> Can you make these changes for v6?
> 
> Tom
> 
> On 7/22/22 11:29 AM, Vargas, Hernan wrote:
> > Hi Tom,
> >
> > The patch series for the ACC200 can wait until Nic's back.
> > Our priority are the changes for the bbdev API here:
> > https://patches.dpdk.org/project/dpdk/list/?series=23912
> >
> > Thanks,
> > Hernan
> >
> > -----Original Message-----
> > From: Tom Rix <trix@redhat.com>
> > Sent: Sunday, July 17, 2022 8:08 AM
> > To: Vargas, Hernan <hernan.vargas@intel.com>; Maxime Coquelin
> > <maxime.coquelin@redhat.com>; Chautru, Nicolas
> > <nicolas.chautru@intel.com>; dev@dpdk.org; thomas@monjalon.net;
> > gakhil@marvell.com; hemant.agrawal@nxp.com
> > Cc: mdr@ashroe.eu; Richardson, Bruce <bruce.richardson@intel.com>;
> > david.marchand@redhat.com; stephen@networkplumber.org
> > Subject: Re: [PATCH v1 00/10] baseband/acc200
> >
> >
> > On 7/14/22 11:49 AM, Vargas, Hernan wrote:
> >> Hi Tom, Maxime,
> >>
> >> Could you please review the v5 series that Nic submitted last week?
> >> https://patches.dpdk.org/project/dpdk/list/?series=23912
> >>
> >> Thanks,
> >> Hernan
> > Hernan,
> >
> > For this patch series for the acc200, will you be able to refactor it so acc
> has a common base ?
> >
> > Or will this be on hold until Nic is back ?
> >
> > Tom
> >
> >>
> >> -----Original Message-----
> >> From: Maxime Coquelin <maxime.coquelin@redhat.com>
> >> Sent: Tuesday, July 12, 2022 8:49 AM
> >> To: Chautru, Nicolas <nicolas.chautru@intel.com>; dev@dpdk.org;
> >> thomas@monjalon.net; gakhil@marvell.com; hemant.agrawal@nxp.com;
> >> trix@redhat.com; Vargas, Hernan <hernan.vargas@intel.com>
> >> Cc: mdr@ashroe.eu; Richardson, Bruce <bruce.richardson@intel.com>;
> >> david.marchand@redhat.com; stephen@networkplumber.org
> >> Subject: Re: [PATCH v1 00/10] baseband/acc200
> >>
> >> Hi Nicolas, Hernan,
> >>
> >> (Adding Hernan in the recipients list)
> >>
> >> On 7/8/22 02:01, Nicolas Chautru wrote:
> >>> This is targeting 22.11 and includes the PMD for the integrated
> >>> accelerator on Intel Xeon SPR-EEC.
> >>> There is a dependency on that parallel serie still in-flight which
> >>> extends the bbdev api
> >>> https://patches.dpdk.org/project/dpdk/list/?series=23894
> >>>
> >>> I will be offline for a few weeks for the summer break but Hernan
> >>> will cover for me during that time if required.
> >>>
> >>> Thanks
> >>> Nic
> >>>
> >>> Nicolas Chautru (10):
> >>>      baseband/acc200: introduce PMD for ACC200
> >>>      baseband/acc200: add HW register definitions
> >>>      baseband/acc200: add info get function
> >>>      baseband/acc200: add queue configuration
> >>>      baseband/acc200: add LDPC processing functions
> >>>      baseband/acc200: add LTE processing functions
> >>>      baseband/acc200: add support for FFT operations
> >>>      baseband/acc200: support interrupt
> >>>      baseband/acc200: add device status and vf2pf comms
> >>>      baseband/acc200: add PF configure companion function
> >>>
> >>>     MAINTAINERS                              |    3 +
> >>>     app/test-bbdev/meson.build               |    3 +
> >>>     app/test-bbdev/test_bbdev_perf.c         |   76 +
> >>>     doc/guides/bbdevs/acc200.rst             |  244 ++
> >>>     doc/guides/bbdevs/index.rst              |    1 +
> >>>     drivers/baseband/acc200/acc200_pf_enum.h |  468 +++
> >>>     drivers/baseband/acc200/acc200_pmd.h     |  690 ++++
> >>>     drivers/baseband/acc200/acc200_vf_enum.h |   89 +
> >>>     drivers/baseband/acc200/meson.build      |    8 +
> >>>     drivers/baseband/acc200/rte_acc200_cfg.h |  115 +
> >>>     drivers/baseband/acc200/rte_acc200_pmd.c | 5403
> ++++++++++++++++++++++++++++++
> >>>     drivers/baseband/acc200/version.map      |   10 +
> >>>     drivers/baseband/meson.build             |    1 +
> >>>     13 files changed, 7111 insertions(+)
> >>>     create mode 100644 doc/guides/bbdevs/acc200.rst
> >>>     create mode 100644 drivers/baseband/acc200/acc200_pf_enum.h
> >>>     create mode 100644 drivers/baseband/acc200/acc200_pmd.h
> >>>     create mode 100644 drivers/baseband/acc200/acc200_vf_enum.h
> >>>     create mode 100644 drivers/baseband/acc200/meson.build
> >>>     create mode 100644 drivers/baseband/acc200/rte_acc200_cfg.h
> >>>     create mode 100644 drivers/baseband/acc200/rte_acc200_pmd.c
> >>>     create mode 100644 drivers/baseband/acc200/version.map
> >>>
> >> Comparing ACC200 & ACC100 header files, I understand ACC200 is an
> evolution of the ACC10x family. The FEC bits are really close, ACC200 main
> addition seems to be FFT acceleration which could be handled in ACC10x
> driver based on device ID.
> >>
> >> I think both drivers have to be merged in order to avoid code duplication.
> That's how other families of devices (e.g. i40e) are handled.
> >>
> >> Thanks,
> >> Maxime
> >>

Maxime Coquelin Aug. 30, 2022, 7:44 a.m. UTC | #7

Hi Nicolas,

On 7/12/22 15:48, Maxime Coquelin wrote:
> Hi Nicolas, Hernan,
> 
> (Adding Hernan in the recipients list)
> 
> On 7/8/22 02:01, Nicolas Chautru wrote:
>> This is targeting 22.11 and includes the PMD for the
>> integrated accelerator on Intel Xeon SPR-EEC.
>> There is a dependency on that parallel serie still in-flight
>> which extends the bbdev api 
>> https://patches.dpdk.org/project/dpdk/list/?series=23894
>>
>> I will be offline for a few weeks for the summer break but
>> Hernan will cover for me during that time if required.
>>
>> Thanks
>> Nic
>>
>> Nicolas Chautru (10):
>>    baseband/acc200: introduce PMD for ACC200
>>    baseband/acc200: add HW register definitions
>>    baseband/acc200: add info get function
>>    baseband/acc200: add queue configuration
>>    baseband/acc200: add LDPC processing functions
>>    baseband/acc200: add LTE processing functions
>>    baseband/acc200: add support for FFT operations
>>    baseband/acc200: support interrupt
>>    baseband/acc200: add device status and vf2pf comms
>>    baseband/acc200: add PF configure companion function
>>
>>   MAINTAINERS                              |    3 +
>>   app/test-bbdev/meson.build               |    3 +
>>   app/test-bbdev/test_bbdev_perf.c         |   76 +
>>   doc/guides/bbdevs/acc200.rst             |  244 ++
>>   doc/guides/bbdevs/index.rst              |    1 +
>>   drivers/baseband/acc200/acc200_pf_enum.h |  468 +++
>>   drivers/baseband/acc200/acc200_pmd.h     |  690 ++++
>>   drivers/baseband/acc200/acc200_vf_enum.h |   89 +
>>   drivers/baseband/acc200/meson.build      |    8 +
>>   drivers/baseband/acc200/rte_acc200_cfg.h |  115 +
>>   drivers/baseband/acc200/rte_acc200_pmd.c | 5403 
>> ++++++++++++++++++++++++++++++
>>   drivers/baseband/acc200/version.map      |   10 +
>>   drivers/baseband/meson.build             |    1 +
>>   13 files changed, 7111 insertions(+)
>>   create mode 100644 doc/guides/bbdevs/acc200.rst
>>   create mode 100644 drivers/baseband/acc200/acc200_pf_enum.h
>>   create mode 100644 drivers/baseband/acc200/acc200_pmd.h
>>   create mode 100644 drivers/baseband/acc200/acc200_vf_enum.h
>>   create mode 100644 drivers/baseband/acc200/meson.build
>>   create mode 100644 drivers/baseband/acc200/rte_acc200_cfg.h
>>   create mode 100644 drivers/baseband/acc200/rte_acc200_pmd.c
>>   create mode 100644 drivers/baseband/acc200/version.map
>>
> 
> Comparing ACC200 & ACC100 header files, I understand ACC200 is an
> evolution of the ACC10x family. The FEC bits are really close, ACC200
> main addition seems to be FFT acceleration which could be handled in
> ACC10x driver based on device ID.
> 
> I think both drivers have to be merged in order to avoid code
> duplication. That's how other families of devices (e.g. i40e) are
> handled.

I haven't seen your reply on this point.
Do you confirm you are working on a single driver for ACC family in
order to avoid code duplication?

Maxime

> Thanks,
> Maxime

Chautru, Nicolas Aug. 30, 2022, 7:45 p.m. UTC | #8

Hi Maxime, 

> -----Original Message-----
> From: Maxime Coquelin <maxime.coquelin@redhat.com>
> Sent: Tuesday, August 30, 2022 12:45 AM
> To: Chautru, Nicolas <nicolas.chautru@intel.com>; dev@dpdk.org;
> thomas@monjalon.net; gakhil@marvell.com; hemant.agrawal@nxp.com;
> trix@redhat.com; Vargas, Hernan <hernan.vargas@intel.com>
> Cc: mdr@ashroe.eu; Richardson, Bruce <bruce.richardson@intel.com>;
> david.marchand@redhat.com; stephen@networkplumber.org
> Subject: Re: [PATCH v1 00/10] baseband/acc200
> 
> Hi Nicolas,
> 
> On 7/12/22 15:48, Maxime Coquelin wrote:
> > Hi Nicolas, Hernan,
> >
> > (Adding Hernan in the recipients list)
> >
> > On 7/8/22 02:01, Nicolas Chautru wrote:
> >> This is targeting 22.11 and includes the PMD for the integrated
> >> accelerator on Intel Xeon SPR-EEC.
> >> There is a dependency on that parallel serie still in-flight which
> >> extends the bbdev api
> >> https://patches.dpdk.org/project/dpdk/list/?series=23894
> >>
> >> I will be offline for a few weeks for the summer break but Hernan
> >> will cover for me during that time if required.
> >>
> >> Thanks
> >> Nic
> >>
> >> Nicolas Chautru (10):
> >>    baseband/acc200: introduce PMD for ACC200
> >>    baseband/acc200: add HW register definitions
> >>    baseband/acc200: add info get function
> >>    baseband/acc200: add queue configuration
> >>    baseband/acc200: add LDPC processing functions
> >>    baseband/acc200: add LTE processing functions
> >>    baseband/acc200: add support for FFT operations
> >>    baseband/acc200: support interrupt
> >>    baseband/acc200: add device status and vf2pf comms
> >>    baseband/acc200: add PF configure companion function
> >>
> >>   MAINTAINERS                              |    3 +
> >>   app/test-bbdev/meson.build               |    3 +
> >>   app/test-bbdev/test_bbdev_perf.c         |   76 +
> >>   doc/guides/bbdevs/acc200.rst             |  244 ++
> >>   doc/guides/bbdevs/index.rst              |    1 +
> >>   drivers/baseband/acc200/acc200_pf_enum.h |  468 +++
> >>   drivers/baseband/acc200/acc200_pmd.h     |  690 ++++
> >>   drivers/baseband/acc200/acc200_vf_enum.h |   89 +
> >>   drivers/baseband/acc200/meson.build      |    8 +
> >>   drivers/baseband/acc200/rte_acc200_cfg.h |  115 +
> >>   drivers/baseband/acc200/rte_acc200_pmd.c | 5403
> >> ++++++++++++++++++++++++++++++
> >>   drivers/baseband/acc200/version.map      |   10 +
> >>   drivers/baseband/meson.build             |    1 +
> >>   13 files changed, 7111 insertions(+)
> >>   create mode 100644 doc/guides/bbdevs/acc200.rst
> >>   create mode 100644 drivers/baseband/acc200/acc200_pf_enum.h
> >>   create mode 100644 drivers/baseband/acc200/acc200_pmd.h
> >>   create mode 100644 drivers/baseband/acc200/acc200_vf_enum.h
> >>   create mode 100644 drivers/baseband/acc200/meson.build
> >>   create mode 100644 drivers/baseband/acc200/rte_acc200_cfg.h
> >>   create mode 100644 drivers/baseband/acc200/rte_acc200_pmd.c
> >>   create mode 100644 drivers/baseband/acc200/version.map
> >>
> >
> > Comparing ACC200 & ACC100 header files, I understand ACC200 is an
> > evolution of the ACC10x family. The FEC bits are really close, ACC200
> > main addition seems to be FFT acceleration which could be handled in
> > ACC10x driver based on device ID.
> >
> > I think both drivers have to be merged in order to avoid code
> > duplication. That's how other families of devices (e.g. i40e) are
> > handled.
> 
> I haven't seen your reply on this point.
> Do you confirm you are working on a single driver for ACC family in order to
> avoid code duplication?
> 

The implementation is based on distinct ACC100 and ACC200 drivers. The 2 devices are fundamentally different generation, processes and IP.
MountBryce is an eASIC device over PCIe while ACC200 is an integrated accelerator on Xeon CPU. 
The actual implementation are not the same, underlying IP are all distinct even if many of the descriptor format have similarities. 
The actual capabilities of the acceleration are different and/or new. 
The workaround and silicon errata are also different causing different limitation and implementation in the driver (see the serie with ongoing changes for ACC100 in parallel).
This is fundamentally distinct from ACC101 which was a derivative product from ACC100 and where it made sense to share implementation between ACC100 and ACC101. 
So in a nutshell these 2 devices and drivers are 2 different beasts and the intention is to keep them intentionally separate as in the serie.
Let me know if unclear, thanks!

Thanks
Nic


> Maxime
> 
> > Thanks,
> > Maxime

Maxime Coquelin Aug. 31, 2022, 4:43 p.m. UTC | #9

Hello Nicolas,

On 8/30/22 21:45, Chautru, Nicolas wrote:
> Hi Maxime,
> 
>> -----Original Message-----
>> From: Maxime Coquelin <maxime.coquelin@redhat.com>
>> Sent: Tuesday, August 30, 2022 12:45 AM
>> To: Chautru, Nicolas <nicolas.chautru@intel.com>; dev@dpdk.org;
>> thomas@monjalon.net; gakhil@marvell.com; hemant.agrawal@nxp.com;
>> trix@redhat.com; Vargas, Hernan <hernan.vargas@intel.com>
>> Cc: mdr@ashroe.eu; Richardson, Bruce <bruce.richardson@intel.com>;
>> david.marchand@redhat.com; stephen@networkplumber.org
>> Subject: Re: [PATCH v1 00/10] baseband/acc200
>>
>> Hi Nicolas,
>>
>> On 7/12/22 15:48, Maxime Coquelin wrote:
>>> Hi Nicolas, Hernan,
>>>
>>> (Adding Hernan in the recipients list)
>>>
>>> On 7/8/22 02:01, Nicolas Chautru wrote:
>>>> This is targeting 22.11 and includes the PMD for the integrated
>>>> accelerator on Intel Xeon SPR-EEC.
>>>> There is a dependency on that parallel serie still in-flight which
>>>> extends the bbdev api
>>>> https://patches.dpdk.org/project/dpdk/list/?series=23894
>>>>
>>>> I will be offline for a few weeks for the summer break but Hernan
>>>> will cover for me during that time if required.
>>>>
>>>> Thanks
>>>> Nic
>>>>
>>>> Nicolas Chautru (10):
>>>>     baseband/acc200: introduce PMD for ACC200
>>>>     baseband/acc200: add HW register definitions
>>>>     baseband/acc200: add info get function
>>>>     baseband/acc200: add queue configuration
>>>>     baseband/acc200: add LDPC processing functions
>>>>     baseband/acc200: add LTE processing functions
>>>>     baseband/acc200: add support for FFT operations
>>>>     baseband/acc200: support interrupt
>>>>     baseband/acc200: add device status and vf2pf comms
>>>>     baseband/acc200: add PF configure companion function
>>>>
>>>>    MAINTAINERS                              |    3 +
>>>>    app/test-bbdev/meson.build               |    3 +
>>>>    app/test-bbdev/test_bbdev_perf.c         |   76 +
>>>>    doc/guides/bbdevs/acc200.rst             |  244 ++
>>>>    doc/guides/bbdevs/index.rst              |    1 +
>>>>    drivers/baseband/acc200/acc200_pf_enum.h |  468 +++
>>>>    drivers/baseband/acc200/acc200_pmd.h     |  690 ++++
>>>>    drivers/baseband/acc200/acc200_vf_enum.h |   89 +
>>>>    drivers/baseband/acc200/meson.build      |    8 +
>>>>    drivers/baseband/acc200/rte_acc200_cfg.h |  115 +
>>>>    drivers/baseband/acc200/rte_acc200_pmd.c | 5403
>>>> ++++++++++++++++++++++++++++++
>>>>    drivers/baseband/acc200/version.map      |   10 +
>>>>    drivers/baseband/meson.build             |    1 +
>>>>    13 files changed, 7111 insertions(+)
>>>>    create mode 100644 doc/guides/bbdevs/acc200.rst
>>>>    create mode 100644 drivers/baseband/acc200/acc200_pf_enum.h
>>>>    create mode 100644 drivers/baseband/acc200/acc200_pmd.h
>>>>    create mode 100644 drivers/baseband/acc200/acc200_vf_enum.h
>>>>    create mode 100644 drivers/baseband/acc200/meson.build
>>>>    create mode 100644 drivers/baseband/acc200/rte_acc200_cfg.h
>>>>    create mode 100644 drivers/baseband/acc200/rte_acc200_pmd.c
>>>>    create mode 100644 drivers/baseband/acc200/version.map
>>>>
>>>
>>> Comparing ACC200 & ACC100 header files, I understand ACC200 is an
>>> evolution of the ACC10x family. The FEC bits are really close, ACC200
>>> main addition seems to be FFT acceleration which could be handled in
>>> ACC10x driver based on device ID.
>>>
>>> I think both drivers have to be merged in order to avoid code
>>> duplication. That's how other families of devices (e.g. i40e) are
>>> handled.
>>
>> I haven't seen your reply on this point.
>> Do you confirm you are working on a single driver for ACC family in order to
>> avoid code duplication?
>>
> 
> The implementation is based on distinct ACC100 and ACC200 drivers. The 2 devices are fundamentally different generation, processes and IP.
> MountBryce is an eASIC device over PCIe while ACC200 is an integrated accelerator on Xeon CPU.

The underlying technology does not matter much. For example we use same
Virtio driver for SW emulated devices and fully HW offloaded ones.

I have spent some time today comparing the drivers and what I can see is
the ACC200 driver is a copy-paste of the ACC100, modulo FFT addition and
other small changes that I think could be handled dynamically based on
capabilities flags and device ID.

> The actual implementation are not the same, underlying IP are all distinct even if many of the descriptor format have similarities.
> The actual capabilities of the acceleration are different and/or new.

New capabilities should be backed by device capabilities flags.

> The workaround and silicon errata are also different causing different limitation and implementation in the driver (see the serie with ongoing changes for ACC100 in parallel).
> This is fundamentally distinct from ACC101 which was a derivative product from ACC100 and where it made sense to share implementation between ACC100 and ACC101.
> So in a nutshell these 2 devices and drivers are 2 different beasts and the intention is to keep them intentionally separate as in the serie.
> Let me know if unclear, thanks!

Thanks for the information.
I still think it should be a single driver, I would appreciate a second
opinion. Thomas, Bruce, Stephen, do you have time to have a look?

Thanks,
Maxime

> Thanks
> Nic
> 
> 
>> Maxime
>>
>>> Thanks,
>>> Maxime
>

Thomas Monjalon Aug. 31, 2022, 7:20 p.m. UTC | #10

31/08/2022 18:43, Maxime Coquelin:
> Hello Nicolas,
> 
> On 8/30/22 21:45, Chautru, Nicolas wrote:
> > Hi Maxime,
> > 
> >> -----Original Message-----
> >> From: Maxime Coquelin <maxime.coquelin@redhat.com>
> >> Sent: Tuesday, August 30, 2022 12:45 AM
> >> To: Chautru, Nicolas <nicolas.chautru@intel.com>; dev@dpdk.org;
> >> thomas@monjalon.net; gakhil@marvell.com; hemant.agrawal@nxp.com;
> >> trix@redhat.com; Vargas, Hernan <hernan.vargas@intel.com>
> >> Cc: mdr@ashroe.eu; Richardson, Bruce <bruce.richardson@intel.com>;
> >> david.marchand@redhat.com; stephen@networkplumber.org
> >> Subject: Re: [PATCH v1 00/10] baseband/acc200
> >>
> >> Hi Nicolas,
> >>
> >> On 7/12/22 15:48, Maxime Coquelin wrote:
> >>> Hi Nicolas, Hernan,
> >>>
> >>> (Adding Hernan in the recipients list)
> >>>
> >>> On 7/8/22 02:01, Nicolas Chautru wrote:
> >>>> This is targeting 22.11 and includes the PMD for the integrated
> >>>> accelerator on Intel Xeon SPR-EEC.
> >>>> There is a dependency on that parallel serie still in-flight which
> >>>> extends the bbdev api
> >>>> https://patches.dpdk.org/project/dpdk/list/?series=23894
> >>>>
> >>>> I will be offline for a few weeks for the summer break but Hernan
> >>>> will cover for me during that time if required.
> >>>>
> >>>> Thanks
> >>>> Nic
> >>>>
> >>>> Nicolas Chautru (10):
> >>>>     baseband/acc200: introduce PMD for ACC200
> >>>>     baseband/acc200: add HW register definitions
> >>>>     baseband/acc200: add info get function
> >>>>     baseband/acc200: add queue configuration
> >>>>     baseband/acc200: add LDPC processing functions
> >>>>     baseband/acc200: add LTE processing functions
> >>>>     baseband/acc200: add support for FFT operations
> >>>>     baseband/acc200: support interrupt
> >>>>     baseband/acc200: add device status and vf2pf comms
> >>>>     baseband/acc200: add PF configure companion function
> >>>>
> >>>>    MAINTAINERS                              |    3 +
> >>>>    app/test-bbdev/meson.build               |    3 +
> >>>>    app/test-bbdev/test_bbdev_perf.c         |   76 +
> >>>>    doc/guides/bbdevs/acc200.rst             |  244 ++
> >>>>    doc/guides/bbdevs/index.rst              |    1 +
> >>>>    drivers/baseband/acc200/acc200_pf_enum.h |  468 +++
> >>>>    drivers/baseband/acc200/acc200_pmd.h     |  690 ++++
> >>>>    drivers/baseband/acc200/acc200_vf_enum.h |   89 +
> >>>>    drivers/baseband/acc200/meson.build      |    8 +
> >>>>    drivers/baseband/acc200/rte_acc200_cfg.h |  115 +
> >>>>    drivers/baseband/acc200/rte_acc200_pmd.c | 5403
> >>>> ++++++++++++++++++++++++++++++
> >>>>    drivers/baseband/acc200/version.map      |   10 +
> >>>>    drivers/baseband/meson.build             |    1 +
> >>>>    13 files changed, 7111 insertions(+)
> >>>>    create mode 100644 doc/guides/bbdevs/acc200.rst
> >>>>    create mode 100644 drivers/baseband/acc200/acc200_pf_enum.h
> >>>>    create mode 100644 drivers/baseband/acc200/acc200_pmd.h
> >>>>    create mode 100644 drivers/baseband/acc200/acc200_vf_enum.h
> >>>>    create mode 100644 drivers/baseband/acc200/meson.build
> >>>>    create mode 100644 drivers/baseband/acc200/rte_acc200_cfg.h
> >>>>    create mode 100644 drivers/baseband/acc200/rte_acc200_pmd.c
> >>>>    create mode 100644 drivers/baseband/acc200/version.map
> >>>>
> >>>
> >>> Comparing ACC200 & ACC100 header files, I understand ACC200 is an
> >>> evolution of the ACC10x family. The FEC bits are really close, ACC200
> >>> main addition seems to be FFT acceleration which could be handled in
> >>> ACC10x driver based on device ID.
> >>>
> >>> I think both drivers have to be merged in order to avoid code
> >>> duplication. That's how other families of devices (e.g. i40e) are
> >>> handled.
> >>
> >> I haven't seen your reply on this point.
> >> Do you confirm you are working on a single driver for ACC family in order to
> >> avoid code duplication?
> >>
> > 
> > The implementation is based on distinct ACC100 and ACC200 drivers. The 2 devices are fundamentally different generation, processes and IP.
> > MountBryce is an eASIC device over PCIe while ACC200 is an integrated accelerator on Xeon CPU.
> 
> The underlying technology does not matter much. For example we use same
> Virtio driver for SW emulated devices and fully HW offloaded ones.
> 
> I have spent some time today comparing the drivers and what I can see is
> the ACC200 driver is a copy-paste of the ACC100, modulo FFT addition and
> other small changes that I think could be handled dynamically based on
> capabilities flags and device ID.
> 
> > The actual implementation are not the same, underlying IP are all distinct even if many of the descriptor format have similarities.
> > The actual capabilities of the acceleration are different and/or new.
> 
> New capabilities should be backed by device capabilities flags.
> 
> > The workaround and silicon errata are also different causing different limitation and implementation in the driver (see the serie with ongoing changes for ACC100 in parallel).
> > This is fundamentally distinct from ACC101 which was a derivative product from ACC100 and where it made sense to share implementation between ACC100 and ACC101.
> > So in a nutshell these 2 devices and drivers are 2 different beasts and the intention is to keep them intentionally separate as in the serie.
> > Let me know if unclear, thanks!
> 
> Thanks for the information.
> I still think it should be a single driver, I would appreciate a second
> opinion. Thomas, Bruce, Stephen, do you have time to have a look?

If most code is similar, it should be the same driver.

Tom Rix Aug. 31, 2022, 7:26 p.m. UTC | #11

On 8/30/22 12:45 PM, Chautru, Nicolas wrote:
> Hi Maxime,
>
>> -----Original Message-----
>> From: Maxime Coquelin <maxime.coquelin@redhat.com>
>> Sent: Tuesday, August 30, 2022 12:45 AM
>> To: Chautru, Nicolas <nicolas.chautru@intel.com>; dev@dpdk.org;
>> thomas@monjalon.net; gakhil@marvell.com; hemant.agrawal@nxp.com;
>> trix@redhat.com; Vargas, Hernan <hernan.vargas@intel.com>
>> Cc: mdr@ashroe.eu; Richardson, Bruce <bruce.richardson@intel.com>;
>> david.marchand@redhat.com; stephen@networkplumber.org
>> Subject: Re: [PATCH v1 00/10] baseband/acc200
>>
>> Hi Nicolas,
>>
>> On 7/12/22 15:48, Maxime Coquelin wrote:
>>> Hi Nicolas, Hernan,
>>>
>>> (Adding Hernan in the recipients list)
>>>
>>> On 7/8/22 02:01, Nicolas Chautru wrote:
>>>> This is targeting 22.11 and includes the PMD for the integrated
>>>> accelerator on Intel Xeon SPR-EEC.
>>>> There is a dependency on that parallel serie still in-flight which
>>>> extends the bbdev api
>>>> https://patches.dpdk.org/project/dpdk/list/?series=23894
>>>>
>>>> I will be offline for a few weeks for the summer break but Hernan
>>>> will cover for me during that time if required.
>>>>
>>>> Thanks
>>>> Nic
>>>>
>>>> Nicolas Chautru (10):
>>>>     baseband/acc200: introduce PMD for ACC200
>>>>     baseband/acc200: add HW register definitions
>>>>     baseband/acc200: add info get function
>>>>     baseband/acc200: add queue configuration
>>>>     baseband/acc200: add LDPC processing functions
>>>>     baseband/acc200: add LTE processing functions
>>>>     baseband/acc200: add support for FFT operations
>>>>     baseband/acc200: support interrupt
>>>>     baseband/acc200: add device status and vf2pf comms
>>>>     baseband/acc200: add PF configure companion function
>>>>
>>>>    MAINTAINERS                              |    3 +
>>>>    app/test-bbdev/meson.build               |    3 +
>>>>    app/test-bbdev/test_bbdev_perf.c         |   76 +
>>>>    doc/guides/bbdevs/acc200.rst             |  244 ++
>>>>    doc/guides/bbdevs/index.rst              |    1 +
>>>>    drivers/baseband/acc200/acc200_pf_enum.h |  468 +++
>>>>    drivers/baseband/acc200/acc200_pmd.h     |  690 ++++
>>>>    drivers/baseband/acc200/acc200_vf_enum.h |   89 +
>>>>    drivers/baseband/acc200/meson.build      |    8 +
>>>>    drivers/baseband/acc200/rte_acc200_cfg.h |  115 +
>>>>    drivers/baseband/acc200/rte_acc200_pmd.c | 5403
>>>> ++++++++++++++++++++++++++++++
>>>>    drivers/baseband/acc200/version.map      |   10 +
>>>>    drivers/baseband/meson.build             |    1 +
>>>>    13 files changed, 7111 insertions(+)
>>>>    create mode 100644 doc/guides/bbdevs/acc200.rst
>>>>    create mode 100644 drivers/baseband/acc200/acc200_pf_enum.h
>>>>    create mode 100644 drivers/baseband/acc200/acc200_pmd.h
>>>>    create mode 100644 drivers/baseband/acc200/acc200_vf_enum.h
>>>>    create mode 100644 drivers/baseband/acc200/meson.build
>>>>    create mode 100644 drivers/baseband/acc200/rte_acc200_cfg.h
>>>>    create mode 100644 drivers/baseband/acc200/rte_acc200_pmd.c
>>>>    create mode 100644 drivers/baseband/acc200/version.map
>>>>
>>> Comparing ACC200 & ACC100 header files, I understand ACC200 is an
>>> evolution of the ACC10x family. The FEC bits are really close, ACC200
>>> main addition seems to be FFT acceleration which could be handled in
>>> ACC10x driver based on device ID.
>>>
>>> I think both drivers have to be merged in order to avoid code
>>> duplication. That's how other families of devices (e.g. i40e) are
>>> handled.
>> I haven't seen your reply on this point.
>> Do you confirm you are working on a single driver for ACC family in order to
>> avoid code duplication?
>>
> The implementation is based on distinct ACC100 and ACC200 drivers. The 2 devices are fundamentally different generation, processes and IP.
> MountBryce is an eASIC device over PCIe while ACC200 is an integrated accelerator on Xeon CPU.
> The actual implementation are not the same, underlying IP are all distinct even if many of the descriptor format have similarities.
> The actual capabilities of the acceleration are different and/or new.
> The workaround and silicon errata are also different causing different limitation and implementation in the driver (see the serie with ongoing changes for ACC100 in parallel).
> This is fundamentally distinct from ACC101 which was a derivative product from ACC100 and where it made sense to share implementation between ACC100 and ACC101.
> So in a nutshell these 2 devices and drivers are 2 different beasts and the intention is to keep them intentionally separate as in the serie.
> Let me know if unclear, thanks!

Nic,

I used a similarity checker to compare acc100 and acc200

https://dickgrune.com/Programs/similarity_tester/

l=simum.log
if [ -f $l ]; then
     rm $l
fi

sim_c -s -R -o$l -R -p -P -a .

There results are

./acc200/acc200_pf_enum.h consists for 100 % of 
./acc100/acc100_pf_enum.h material
./acc100/acc100_pf_enum.h consists for 98 % of ./acc200/acc200_pf_enum.h 
material
./acc100/rte_acc100_pmd.h consists for 98 % of ./acc200/acc200_pmd.h 
material
./acc200/acc200_vf_enum.h consists for 95 % of ./acc100/acc100_pf_enum.h 
material
./acc200/acc200_pmd.h consists for 92 % of ./acc100/rte_acc100_pmd.h 
material
./acc200/rte_acc200_cfg.h consists for 92 % of ./acc100/rte_acc100_cfg.h 
material
./acc100/rte_acc100_pmd.c consists for 87 % of ./acc200/rte_acc200_pmd.c 
material
./acc100/acc100_vf_enum.h consists for 80 % of ./acc200/acc200_pf_enum.h 
material
./acc200/rte_acc200_pmd.c consists for 78 % of ./acc100/rte_acc100_pmd.c 
material
./acc100/rte_acc100_cfg.h consists for 75 % of ./acc200/rte_acc200_cfg.h 
material

Spot checking the first *pf_enum.h at 100%, these are the devices' 
registers, they are the same.

I raised this similarity issue with 100 vs 101.

Having multiple copies is difficult to support and should be avoided.

For the end user, they should have to use only one driver.

Tom

>
> Thanks
> Nic
>
>
>> Maxime
>>
>>> Thanks,
>>> Maxime

Chautru, Nicolas Aug. 31, 2022, 10:37 p.m. UTC | #12

Hi Thomas, Tom,

> -----Original Message-----
> From: Tom Rix <trix@redhat.com>
> Sent: Wednesday, August 31, 2022 12:26 PM
> To: Chautru, Nicolas <nicolas.chautru@intel.com>; Maxime Coquelin
> <maxime.coquelin@redhat.com>; dev@dpdk.org; thomas@monjalon.net;
> gakhil@marvell.com; hemant.agrawal@nxp.com; Vargas, Hernan
> <hernan.vargas@intel.com>
> Cc: mdr@ashroe.eu; Richardson, Bruce <bruce.richardson@intel.com>;
> david.marchand@redhat.com; stephen@networkplumber.org
> Subject: Re: [PATCH v1 00/10] baseband/acc200
> 
> 
> On 8/30/22 12:45 PM, Chautru, Nicolas wrote:
> > Hi Maxime,
> >
> >> -----Original Message-----
> >> From: Maxime Coquelin <maxime.coquelin@redhat.com>
> >> Sent: Tuesday, August 30, 2022 12:45 AM
> >> To: Chautru, Nicolas <nicolas.chautru@intel.com>; dev@dpdk.org;
> >> thomas@monjalon.net; gakhil@marvell.com; hemant.agrawal@nxp.com;
> >> trix@redhat.com; Vargas, Hernan <hernan.vargas@intel.com>
> >> Cc: mdr@ashroe.eu; Richardson, Bruce <bruce.richardson@intel.com>;
> >> david.marchand@redhat.com; stephen@networkplumber.org
> >> Subject: Re: [PATCH v1 00/10] baseband/acc200
> >>
> >> Hi Nicolas,
> >>
> >> On 7/12/22 15:48, Maxime Coquelin wrote:
> >>> Hi Nicolas, Hernan,
> >>>
> >>> (Adding Hernan in the recipients list)
> >>>
> >>> On 7/8/22 02:01, Nicolas Chautru wrote:
> >>>> This is targeting 22.11 and includes the PMD for the integrated
> >>>> accelerator on Intel Xeon SPR-EEC.
> >>>> There is a dependency on that parallel serie still in-flight which
> >>>> extends the bbdev api
> >>>> https://patches.dpdk.org/project/dpdk/list/?series=23894
> >>>>
> >>>> I will be offline for a few weeks for the summer break but Hernan
> >>>> will cover for me during that time if required.
> >>>>
> >>>> Thanks
> >>>> Nic
> >>>>
> >>>> Nicolas Chautru (10):
> >>>>     baseband/acc200: introduce PMD for ACC200
> >>>>     baseband/acc200: add HW register definitions
> >>>>     baseband/acc200: add info get function
> >>>>     baseband/acc200: add queue configuration
> >>>>     baseband/acc200: add LDPC processing functions
> >>>>     baseband/acc200: add LTE processing functions
> >>>>     baseband/acc200: add support for FFT operations
> >>>>     baseband/acc200: support interrupt
> >>>>     baseband/acc200: add device status and vf2pf comms
> >>>>     baseband/acc200: add PF configure companion function
> >>>>
> >>>>    MAINTAINERS                              |    3 +
> >>>>    app/test-bbdev/meson.build               |    3 +
> >>>>    app/test-bbdev/test_bbdev_perf.c         |   76 +
> >>>>    doc/guides/bbdevs/acc200.rst             |  244 ++
> >>>>    doc/guides/bbdevs/index.rst              |    1 +
> >>>>    drivers/baseband/acc200/acc200_pf_enum.h |  468 +++
> >>>>    drivers/baseband/acc200/acc200_pmd.h     |  690 ++++
> >>>>    drivers/baseband/acc200/acc200_vf_enum.h |   89 +
> >>>>    drivers/baseband/acc200/meson.build      |    8 +
> >>>>    drivers/baseband/acc200/rte_acc200_cfg.h |  115 +
> >>>>    drivers/baseband/acc200/rte_acc200_pmd.c | 5403
> >>>> ++++++++++++++++++++++++++++++
> >>>>    drivers/baseband/acc200/version.map      |   10 +
> >>>>    drivers/baseband/meson.build             |    1 +
> >>>>    13 files changed, 7111 insertions(+)
> >>>>    create mode 100644 doc/guides/bbdevs/acc200.rst
> >>>>    create mode 100644 drivers/baseband/acc200/acc200_pf_enum.h
> >>>>    create mode 100644 drivers/baseband/acc200/acc200_pmd.h
> >>>>    create mode 100644 drivers/baseband/acc200/acc200_vf_enum.h
> >>>>    create mode 100644 drivers/baseband/acc200/meson.build
> >>>>    create mode 100644 drivers/baseband/acc200/rte_acc200_cfg.h
> >>>>    create mode 100644 drivers/baseband/acc200/rte_acc200_pmd.c
> >>>>    create mode 100644 drivers/baseband/acc200/version.map
> >>>>
> >>> Comparing ACC200 & ACC100 header files, I understand ACC200 is an
> >>> evolution of the ACC10x family. The FEC bits are really close,
> >>> ACC200 main addition seems to be FFT acceleration which could be
> >>> handled in ACC10x driver based on device ID.
> >>>
> >>> I think both drivers have to be merged in order to avoid code
> >>> duplication. That's how other families of devices (e.g. i40e) are
> >>> handled.
> >> I haven't seen your reply on this point.
> >> Do you confirm you are working on a single driver for ACC family in
> >> order to avoid code duplication?
> >>
> > The implementation is based on distinct ACC100 and ACC200 drivers. The 2
> devices are fundamentally different generation, processes and IP.
> > MountBryce is an eASIC device over PCIe while ACC200 is an integrated
> accelerator on Xeon CPU.
> > The actual implementation are not the same, underlying IP are all distinct
> even if many of the descriptor format have similarities.
> > The actual capabilities of the acceleration are different and/or new.
> > The workaround and silicon errata are also different causing different
> limitation and implementation in the driver (see the serie with ongoing
> changes for ACC100 in parallel).
> > This is fundamentally distinct from ACC101 which was a derivative product
> from ACC100 and where it made sense to share implementation between
> ACC100 and ACC101.
> > So in a nutshell these 2 devices and drivers are 2 different beasts and the
> intention is to keep them intentionally separate as in the serie.
> > Let me know if unclear, thanks!
> 
> Nic,
> 
> I used a similarity checker to compare acc100 and acc200
> 
> https://dickgrune.com/Programs/similarity_tester/
> 
> l=simum.log
> if [ -f $l ]; then
>      rm $l
> fi
> 
> sim_c -s -R -o$l -R -p -P -a .
> 
> There results are
> 
> ./acc200/acc200_pf_enum.h consists for 100 % of ./acc100/acc100_pf_enum.h
> material ./acc100/acc100_pf_enum.h consists for 98 % of
> ./acc200/acc200_pf_enum.h material ./acc100/rte_acc100_pmd.h consists for
> 98 % of ./acc200/acc200_pmd.h material ./acc200/acc200_vf_enum.h consists
> for 95 % of ./acc100/acc100_pf_enum.h material ./acc200/acc200_pmd.h
> consists for 92 % of ./acc100/rte_acc100_pmd.h material
> ./acc200/rte_acc200_cfg.h consists for 92 % of ./acc100/rte_acc100_cfg.h
> material ./acc100/rte_acc100_pmd.c consists for 87 % of
> ./acc200/rte_acc200_pmd.c material ./acc100/acc100_vf_enum.h consists for
> 80 % of ./acc200/acc200_pf_enum.h material ./acc200/rte_acc200_pmd.c
> consists for 78 % of ./acc100/rte_acc100_pmd.c material
> ./acc100/rte_acc100_cfg.h consists for 75 % of ./acc200/rte_acc200_cfg.h
> material
> 
> Spot checking the first *pf_enum.h at 100%, these are the devices'
> registers, they are the same.
> 
> I raised this similarity issue with 100 vs 101.
> 
> Having multiple copies is difficult to support and should be avoided.
> 
> For the end user, they should have to use only one driver.
> 

There are really different IP and do not have the same interface (PCIe/DDR vs integrated) and there is big serie of changes which are specific to ACC100 coming in parallel. Any workaround, optimization would be different. 
I agree that for the coming serie of integrated accelerator we will use a unified driver approach but for that very case that would be quite messy to artificially put them within the same PMD.

Tom Rix Sept. 1, 2022, 12:28 a.m. UTC | #13

On 8/31/22 3:37 PM, Chautru, Nicolas wrote:
> Hi Thomas, Tom,
>
>> -----Original Message-----
>> From: Tom Rix <trix@redhat.com>
>> Sent: Wednesday, August 31, 2022 12:26 PM
>> To: Chautru, Nicolas <nicolas.chautru@intel.com>; Maxime Coquelin
>> <maxime.coquelin@redhat.com>; dev@dpdk.org; thomas@monjalon.net;
>> gakhil@marvell.com; hemant.agrawal@nxp.com; Vargas, Hernan
>> <hernan.vargas@intel.com>
>> Cc: mdr@ashroe.eu; Richardson, Bruce <bruce.richardson@intel.com>;
>> david.marchand@redhat.com; stephen@networkplumber.org
>> Subject: Re: [PATCH v1 00/10] baseband/acc200
>>
>>
>> On 8/30/22 12:45 PM, Chautru, Nicolas wrote:
>>> Hi Maxime,
>>>
>>>> -----Original Message-----
>>>> From: Maxime Coquelin <maxime.coquelin@redhat.com>
>>>> Sent: Tuesday, August 30, 2022 12:45 AM
>>>> To: Chautru, Nicolas <nicolas.chautru@intel.com>; dev@dpdk.org;
>>>> thomas@monjalon.net; gakhil@marvell.com; hemant.agrawal@nxp.com;
>>>> trix@redhat.com; Vargas, Hernan <hernan.vargas@intel.com>
>>>> Cc: mdr@ashroe.eu; Richardson, Bruce <bruce.richardson@intel.com>;
>>>> david.marchand@redhat.com; stephen@networkplumber.org
>>>> Subject: Re: [PATCH v1 00/10] baseband/acc200
>>>>
>>>> Hi Nicolas,
>>>>
>>>> On 7/12/22 15:48, Maxime Coquelin wrote:
>>>>> Hi Nicolas, Hernan,
>>>>>
>>>>> (Adding Hernan in the recipients list)
>>>>>
>>>>> On 7/8/22 02:01, Nicolas Chautru wrote:
>>>>>> This is targeting 22.11 and includes the PMD for the integrated
>>>>>> accelerator on Intel Xeon SPR-EEC.
>>>>>> There is a dependency on that parallel serie still in-flight which
>>>>>> extends the bbdev api
>>>>>> https://patches.dpdk.org/project/dpdk/list/?series=23894
>>>>>>
>>>>>> I will be offline for a few weeks for the summer break but Hernan
>>>>>> will cover for me during that time if required.
>>>>>>
>>>>>> Thanks
>>>>>> Nic
>>>>>>
>>>>>> Nicolas Chautru (10):
>>>>>>      baseband/acc200: introduce PMD for ACC200
>>>>>>      baseband/acc200: add HW register definitions
>>>>>>      baseband/acc200: add info get function
>>>>>>      baseband/acc200: add queue configuration
>>>>>>      baseband/acc200: add LDPC processing functions
>>>>>>      baseband/acc200: add LTE processing functions
>>>>>>      baseband/acc200: add support for FFT operations
>>>>>>      baseband/acc200: support interrupt
>>>>>>      baseband/acc200: add device status and vf2pf comms
>>>>>>      baseband/acc200: add PF configure companion function
>>>>>>
>>>>>>     MAINTAINERS                              |    3 +
>>>>>>     app/test-bbdev/meson.build               |    3 +
>>>>>>     app/test-bbdev/test_bbdev_perf.c         |   76 +
>>>>>>     doc/guides/bbdevs/acc200.rst             |  244 ++
>>>>>>     doc/guides/bbdevs/index.rst              |    1 +
>>>>>>     drivers/baseband/acc200/acc200_pf_enum.h |  468 +++
>>>>>>     drivers/baseband/acc200/acc200_pmd.h     |  690 ++++
>>>>>>     drivers/baseband/acc200/acc200_vf_enum.h |   89 +
>>>>>>     drivers/baseband/acc200/meson.build      |    8 +
>>>>>>     drivers/baseband/acc200/rte_acc200_cfg.h |  115 +
>>>>>>     drivers/baseband/acc200/rte_acc200_pmd.c | 5403
>>>>>> ++++++++++++++++++++++++++++++
>>>>>>     drivers/baseband/acc200/version.map      |   10 +
>>>>>>     drivers/baseband/meson.build             |    1 +
>>>>>>     13 files changed, 7111 insertions(+)
>>>>>>     create mode 100644 doc/guides/bbdevs/acc200.rst
>>>>>>     create mode 100644 drivers/baseband/acc200/acc200_pf_enum.h
>>>>>>     create mode 100644 drivers/baseband/acc200/acc200_pmd.h
>>>>>>     create mode 100644 drivers/baseband/acc200/acc200_vf_enum.h
>>>>>>     create mode 100644 drivers/baseband/acc200/meson.build
>>>>>>     create mode 100644 drivers/baseband/acc200/rte_acc200_cfg.h
>>>>>>     create mode 100644 drivers/baseband/acc200/rte_acc200_pmd.c
>>>>>>     create mode 100644 drivers/baseband/acc200/version.map
>>>>>>
>>>>> Comparing ACC200 & ACC100 header files, I understand ACC200 is an
>>>>> evolution of the ACC10x family. The FEC bits are really close,
>>>>> ACC200 main addition seems to be FFT acceleration which could be
>>>>> handled in ACC10x driver based on device ID.
>>>>>
>>>>> I think both drivers have to be merged in order to avoid code
>>>>> duplication. That's how other families of devices (e.g. i40e) are
>>>>> handled.
>>>> I haven't seen your reply on this point.
>>>> Do you confirm you are working on a single driver for ACC family in
>>>> order to avoid code duplication?
>>>>
>>> The implementation is based on distinct ACC100 and ACC200 drivers. The 2
>> devices are fundamentally different generation, processes and IP.
>>> MountBryce is an eASIC device over PCIe while ACC200 is an integrated
>> accelerator on Xeon CPU.
>>> The actual implementation are not the same, underlying IP are all distinct
>> even if many of the descriptor format have similarities.
>>> The actual capabilities of the acceleration are different and/or new.
>>> The workaround and silicon errata are also different causing different
>> limitation and implementation in the driver (see the serie with ongoing
>> changes for ACC100 in parallel).
>>> This is fundamentally distinct from ACC101 which was a derivative product
>> from ACC100 and where it made sense to share implementation between
>> ACC100 and ACC101.
>>> So in a nutshell these 2 devices and drivers are 2 different beasts and the
>> intention is to keep them intentionally separate as in the serie.
>>> Let me know if unclear, thanks!
>> Nic,
>>
>> I used a similarity checker to compare acc100 and acc200
>>
>> https://dickgrune.com/Programs/similarity_tester/
>>
>> l=simum.log
>> if [ -f $l ]; then
>>       rm $l
>> fi
>>
>> sim_c -s -R -o$l -R -p -P -a .
>>
>> There results are
>>
>> ./acc200/acc200_pf_enum.h consists for 100 % of ./acc100/acc100_pf_enum.h
>> material ./acc100/acc100_pf_enum.h consists for 98 % of
>> ./acc200/acc200_pf_enum.h material ./acc100/rte_acc100_pmd.h consists for
>> 98 % of ./acc200/acc200_pmd.h material ./acc200/acc200_vf_enum.h consists
>> for 95 % of ./acc100/acc100_pf_enum.h material ./acc200/acc200_pmd.h
>> consists for 92 % of ./acc100/rte_acc100_pmd.h material
>> ./acc200/rte_acc200_cfg.h consists for 92 % of ./acc100/rte_acc100_cfg.h
>> material ./acc100/rte_acc100_pmd.c consists for 87 % of
>> ./acc200/rte_acc200_pmd.c material ./acc100/acc100_vf_enum.h consists for
>> 80 % of ./acc200/acc200_pf_enum.h material ./acc200/rte_acc200_pmd.c
>> consists for 78 % of ./acc100/rte_acc100_pmd.c material
>> ./acc100/rte_acc100_cfg.h consists for 75 % of ./acc200/rte_acc200_cfg.h
>> material
>>
>> Spot checking the first *pf_enum.h at 100%, these are the devices'
>> registers, they are the same.
>>
>> I raised this similarity issue with 100 vs 101.
>>
>> Having multiple copies is difficult to support and should be avoided.
>>
>> For the end user, they should have to use only one driver.
>>
> There are really different IP and do not have the same interface (PCIe/DDR vs integrated) and there is big serie of changes which are specific to ACC100 coming in parallel. Any workaround, optimization would be different.
> I agree that for the coming serie of integrated accelerator we will use a unified driver approach but for that very case that would be quite messy to artificially put them within the same PMD.

How is the IP different when 100% of the registers are the same ?

Tom

>
>

Chautru, Nicolas Sept. 1, 2022, 1:26 a.m. UTC | #14

Hi Tom, 

> -----Original Message-----
> From: Tom Rix <trix@redhat.com>
> Sent: Wednesday, August 31, 2022 5:28 PM
> To: Chautru, Nicolas <nicolas.chautru@intel.com>; Maxime Coquelin
> <maxime.coquelin@redhat.com>; dev@dpdk.org; thomas@monjalon.net;
> gakhil@marvell.com; hemant.agrawal@nxp.com; Vargas, Hernan
> <hernan.vargas@intel.com>
> Cc: mdr@ashroe.eu; Richardson, Bruce <bruce.richardson@intel.com>;
> david.marchand@redhat.com; stephen@networkplumber.org
> Subject: Re: [PATCH v1 00/10] baseband/acc200
> 
> 
> On 8/31/22 3:37 PM, Chautru, Nicolas wrote:
> > Hi Thomas, Tom,
> >
> >> -----Original Message-----
> >> From: Tom Rix <trix@redhat.com>
> >> Sent: Wednesday, August 31, 2022 12:26 PM
> >> To: Chautru, Nicolas <nicolas.chautru@intel.com>; Maxime Coquelin
> >> <maxime.coquelin@redhat.com>; dev@dpdk.org; thomas@monjalon.net;
> >> gakhil@marvell.com; hemant.agrawal@nxp.com; Vargas, Hernan
> >> <hernan.vargas@intel.com>
> >> Cc: mdr@ashroe.eu; Richardson, Bruce <bruce.richardson@intel.com>;
> >> david.marchand@redhat.com; stephen@networkplumber.org
> >> Subject: Re: [PATCH v1 00/10] baseband/acc200
> >>
> >>
> >> On 8/30/22 12:45 PM, Chautru, Nicolas wrote:
> >>> Hi Maxime,
> >>>
> >>>> -----Original Message-----
> >>>> From: Maxime Coquelin <maxime.coquelin@redhat.com>
> >>>> Sent: Tuesday, August 30, 2022 12:45 AM
> >>>> To: Chautru, Nicolas <nicolas.chautru@intel.com>; dev@dpdk.org;
> >>>> thomas@monjalon.net; gakhil@marvell.com; hemant.agrawal@nxp.com;
> >>>> trix@redhat.com; Vargas, Hernan <hernan.vargas@intel.com>
> >>>> Cc: mdr@ashroe.eu; Richardson, Bruce <bruce.richardson@intel.com>;
> >>>> david.marchand@redhat.com; stephen@networkplumber.org
> >>>> Subject: Re: [PATCH v1 00/10] baseband/acc200
> >>>>
> >>>> Hi Nicolas,
> >>>>
> >>>> On 7/12/22 15:48, Maxime Coquelin wrote:
> >>>>> Hi Nicolas, Hernan,
> >>>>>
> >>>>> (Adding Hernan in the recipients list)
> >>>>>
> >>>>> On 7/8/22 02:01, Nicolas Chautru wrote:
> >>>>>> This is targeting 22.11 and includes the PMD for the integrated
> >>>>>> accelerator on Intel Xeon SPR-EEC.
> >>>>>> There is a dependency on that parallel serie still in-flight
> >>>>>> which extends the bbdev api
> >>>>>> https://patches.dpdk.org/project/dpdk/list/?series=23894
> >>>>>>
> >>>>>> I will be offline for a few weeks for the summer break but Hernan
> >>>>>> will cover for me during that time if required.
> >>>>>>
> >>>>>> Thanks
> >>>>>> Nic
> >>>>>>
> >>>>>> Nicolas Chautru (10):
> >>>>>>      baseband/acc200: introduce PMD for ACC200
> >>>>>>      baseband/acc200: add HW register definitions
> >>>>>>      baseband/acc200: add info get function
> >>>>>>      baseband/acc200: add queue configuration
> >>>>>>      baseband/acc200: add LDPC processing functions
> >>>>>>      baseband/acc200: add LTE processing functions
> >>>>>>      baseband/acc200: add support for FFT operations
> >>>>>>      baseband/acc200: support interrupt
> >>>>>>      baseband/acc200: add device status and vf2pf comms
> >>>>>>      baseband/acc200: add PF configure companion function
> >>>>>>
> >>>>>>     MAINTAINERS                              |    3 +
> >>>>>>     app/test-bbdev/meson.build               |    3 +
> >>>>>>     app/test-bbdev/test_bbdev_perf.c         |   76 +
> >>>>>>     doc/guides/bbdevs/acc200.rst             |  244 ++
> >>>>>>     doc/guides/bbdevs/index.rst              |    1 +
> >>>>>>     drivers/baseband/acc200/acc200_pf_enum.h |  468 +++
> >>>>>>     drivers/baseband/acc200/acc200_pmd.h     |  690 ++++
> >>>>>>     drivers/baseband/acc200/acc200_vf_enum.h |   89 +
> >>>>>>     drivers/baseband/acc200/meson.build      |    8 +
> >>>>>>     drivers/baseband/acc200/rte_acc200_cfg.h |  115 +
> >>>>>>     drivers/baseband/acc200/rte_acc200_pmd.c | 5403
> >>>>>> ++++++++++++++++++++++++++++++
> >>>>>>     drivers/baseband/acc200/version.map      |   10 +
> >>>>>>     drivers/baseband/meson.build             |    1 +
> >>>>>>     13 files changed, 7111 insertions(+)
> >>>>>>     create mode 100644 doc/guides/bbdevs/acc200.rst
> >>>>>>     create mode 100644 drivers/baseband/acc200/acc200_pf_enum.h
> >>>>>>     create mode 100644 drivers/baseband/acc200/acc200_pmd.h
> >>>>>>     create mode 100644 drivers/baseband/acc200/acc200_vf_enum.h
> >>>>>>     create mode 100644 drivers/baseband/acc200/meson.build
> >>>>>>     create mode 100644 drivers/baseband/acc200/rte_acc200_cfg.h
> >>>>>>     create mode 100644 drivers/baseband/acc200/rte_acc200_pmd.c
> >>>>>>     create mode 100644 drivers/baseband/acc200/version.map
> >>>>>>
> >>>>> Comparing ACC200 & ACC100 header files, I understand ACC200 is an
> >>>>> evolution of the ACC10x family. The FEC bits are really close,
> >>>>> ACC200 main addition seems to be FFT acceleration which could be
> >>>>> handled in ACC10x driver based on device ID.
> >>>>>
> >>>>> I think both drivers have to be merged in order to avoid code
> >>>>> duplication. That's how other families of devices (e.g. i40e) are
> >>>>> handled.
> >>>> I haven't seen your reply on this point.
> >>>> Do you confirm you are working on a single driver for ACC family in
> >>>> order to avoid code duplication?
> >>>>
> >>> The implementation is based on distinct ACC100 and ACC200 drivers.
> >>> The 2
> >> devices are fundamentally different generation, processes and IP.
> >>> MountBryce is an eASIC device over PCIe while ACC200 is an
> >>> integrated
> >> accelerator on Xeon CPU.
> >>> The actual implementation are not the same, underlying IP are all
> >>> distinct
> >> even if many of the descriptor format have similarities.
> >>> The actual capabilities of the acceleration are different and/or new.
> >>> The workaround and silicon errata are also different causing
> >>> different
> >> limitation and implementation in the driver (see the serie with
> >> ongoing changes for ACC100 in parallel).
> >>> This is fundamentally distinct from ACC101 which was a derivative
> >>> product
> >> from ACC100 and where it made sense to share implementation between
> >> ACC100 and ACC101.
> >>> So in a nutshell these 2 devices and drivers are 2 different beasts
> >>> and the
> >> intention is to keep them intentionally separate as in the serie.
> >>> Let me know if unclear, thanks!
> >> Nic,
> >>
> >> I used a similarity checker to compare acc100 and acc200
> >>
> >> https://dickgrune.com/Programs/similarity_tester/
> >>
> >> l=simum.log
> >> if [ -f $l ]; then
> >>       rm $l
> >> fi
> >>
> >> sim_c -s -R -o$l -R -p -P -a .
> >>
> >> There results are
> >>
> >> ./acc200/acc200_pf_enum.h consists for 100 % of
> >> ./acc100/acc100_pf_enum.h material ./acc100/acc100_pf_enum.h consists
> >> for 98 % of ./acc200/acc200_pf_enum.h material
> >> ./acc100/rte_acc100_pmd.h consists for
> >> 98 % of ./acc200/acc200_pmd.h material ./acc200/acc200_vf_enum.h
> >> consists for 95 % of ./acc100/acc100_pf_enum.h material
> >> ./acc200/acc200_pmd.h consists for 92 % of ./acc100/rte_acc100_pmd.h
> >> material ./acc200/rte_acc200_cfg.h consists for 92 % of
> >> ./acc100/rte_acc100_cfg.h material ./acc100/rte_acc100_pmd.c consists
> >> for 87 % of ./acc200/rte_acc200_pmd.c material
> >> ./acc100/acc100_vf_enum.h consists for
> >> 80 % of ./acc200/acc200_pf_enum.h material ./acc200/rte_acc200_pmd.c
> >> consists for 78 % of ./acc100/rte_acc100_pmd.c material
> >> ./acc100/rte_acc100_cfg.h consists for 75 % of
> >> ./acc200/rte_acc200_cfg.h material
> >>
> >> Spot checking the first *pf_enum.h at 100%, these are the devices'
> >> registers, they are the same.
> >>
> >> I raised this similarity issue with 100 vs 101.
> >>
> >> Having multiple copies is difficult to support and should be avoided.
> >>
> >> For the end user, they should have to use only one driver.
> >>
> > There are really different IP and do not have the same interface (PCIe/DDR vs
> integrated) and there is big serie of changes which are specific to ACC100
> coming in parallel. Any workaround, optimization would be different.
> > I agree that for the coming serie of integrated accelerator we will use a
> unified driver approach but for that very case that would be quite messy to
> artificially put them within the same PMD.
> 
> How is the IP different when 100% of the registers are the same ?
> 

These are 2 different HW aspects. The base toplevel configuration registers are kept similar on purpose but the underlying IP are totally different design and implementation. 
Even the registers have differences but not visible here, the actual RDL file would define more specifically these registers bitfields and implementation including which ones are not implemented (but that is proprietary information), and at bbdev level the interface is not some much register based than processing based on data from DMA. 
Basically even if there was a common driver, all these would be duplicated and they are indeed different IP (including different vendors).. 
But I agree with the general intent and to have a common driver for the integrated driver serie (ACC200, ACC300...) now that we are moving away from PCIe/DDR lookaside acceleration and eASIC/FPGA implementation (ACC100/AC101). 

Thanks
Nic

> Tom
> 
> >
> >

Tom Rix Sept. 1, 2022, 1:49 p.m. UTC | #15

On 8/31/22 6:26 PM, Chautru, Nicolas wrote:
> Hi Tom,
>
>> -----Original Message-----
>> From: Tom Rix <trix@redhat.com>
>> Sent: Wednesday, August 31, 2022 5:28 PM
>> To: Chautru, Nicolas <nicolas.chautru@intel.com>; Maxime Coquelin
>> <maxime.coquelin@redhat.com>; dev@dpdk.org; thomas@monjalon.net;
>> gakhil@marvell.com; hemant.agrawal@nxp.com; Vargas, Hernan
>> <hernan.vargas@intel.com>
>> Cc: mdr@ashroe.eu; Richardson, Bruce <bruce.richardson@intel.com>;
>> david.marchand@redhat.com; stephen@networkplumber.org
>> Subject: Re: [PATCH v1 00/10] baseband/acc200
>>
>>
>> On 8/31/22 3:37 PM, Chautru, Nicolas wrote:
>>> Hi Thomas, Tom,
>>>
>>>> -----Original Message-----
>>>> From: Tom Rix <trix@redhat.com>
>>>> Sent: Wednesday, August 31, 2022 12:26 PM
>>>> To: Chautru, Nicolas <nicolas.chautru@intel.com>; Maxime Coquelin
>>>> <maxime.coquelin@redhat.com>; dev@dpdk.org; thomas@monjalon.net;
>>>> gakhil@marvell.com; hemant.agrawal@nxp.com; Vargas, Hernan
>>>> <hernan.vargas@intel.com>
>>>> Cc: mdr@ashroe.eu; Richardson, Bruce <bruce.richardson@intel.com>;
>>>> david.marchand@redhat.com; stephen@networkplumber.org
>>>> Subject: Re: [PATCH v1 00/10] baseband/acc200
>>>>
>>>>
>>>> On 8/30/22 12:45 PM, Chautru, Nicolas wrote:
>>>>> Hi Maxime,
>>>>>
>>>>>> -----Original Message-----
>>>>>> From: Maxime Coquelin <maxime.coquelin@redhat.com>
>>>>>> Sent: Tuesday, August 30, 2022 12:45 AM
>>>>>> To: Chautru, Nicolas <nicolas.chautru@intel.com>; dev@dpdk.org;
>>>>>> thomas@monjalon.net; gakhil@marvell.com; hemant.agrawal@nxp.com;
>>>>>> trix@redhat.com; Vargas, Hernan <hernan.vargas@intel.com>
>>>>>> Cc: mdr@ashroe.eu; Richardson, Bruce <bruce.richardson@intel.com>;
>>>>>> david.marchand@redhat.com; stephen@networkplumber.org
>>>>>> Subject: Re: [PATCH v1 00/10] baseband/acc200
>>>>>>
>>>>>> Hi Nicolas,
>>>>>>
>>>>>> On 7/12/22 15:48, Maxime Coquelin wrote:
>>>>>>> Hi Nicolas, Hernan,
>>>>>>>
>>>>>>> (Adding Hernan in the recipients list)
>>>>>>>
>>>>>>> On 7/8/22 02:01, Nicolas Chautru wrote:
>>>>>>>> This is targeting 22.11 and includes the PMD for the integrated
>>>>>>>> accelerator on Intel Xeon SPR-EEC.
>>>>>>>> There is a dependency on that parallel serie still in-flight
>>>>>>>> which extends the bbdev api
>>>>>>>> https://patches.dpdk.org/project/dpdk/list/?series=23894
>>>>>>>>
>>>>>>>> I will be offline for a few weeks for the summer break but Hernan
>>>>>>>> will cover for me during that time if required.
>>>>>>>>
>>>>>>>> Thanks
>>>>>>>> Nic
>>>>>>>>
>>>>>>>> Nicolas Chautru (10):
>>>>>>>>       baseband/acc200: introduce PMD for ACC200
>>>>>>>>       baseband/acc200: add HW register definitions
>>>>>>>>       baseband/acc200: add info get function
>>>>>>>>       baseband/acc200: add queue configuration
>>>>>>>>       baseband/acc200: add LDPC processing functions
>>>>>>>>       baseband/acc200: add LTE processing functions
>>>>>>>>       baseband/acc200: add support for FFT operations
>>>>>>>>       baseband/acc200: support interrupt
>>>>>>>>       baseband/acc200: add device status and vf2pf comms
>>>>>>>>       baseband/acc200: add PF configure companion function
>>>>>>>>
>>>>>>>>      MAINTAINERS                              |    3 +
>>>>>>>>      app/test-bbdev/meson.build               |    3 +
>>>>>>>>      app/test-bbdev/test_bbdev_perf.c         |   76 +
>>>>>>>>      doc/guides/bbdevs/acc200.rst             |  244 ++
>>>>>>>>      doc/guides/bbdevs/index.rst              |    1 +
>>>>>>>>      drivers/baseband/acc200/acc200_pf_enum.h |  468 +++
>>>>>>>>      drivers/baseband/acc200/acc200_pmd.h     |  690 ++++
>>>>>>>>      drivers/baseband/acc200/acc200_vf_enum.h |   89 +
>>>>>>>>      drivers/baseband/acc200/meson.build      |    8 +
>>>>>>>>      drivers/baseband/acc200/rte_acc200_cfg.h |  115 +
>>>>>>>>      drivers/baseband/acc200/rte_acc200_pmd.c | 5403
>>>>>>>> ++++++++++++++++++++++++++++++
>>>>>>>>      drivers/baseband/acc200/version.map      |   10 +
>>>>>>>>      drivers/baseband/meson.build             |    1 +
>>>>>>>>      13 files changed, 7111 insertions(+)
>>>>>>>>      create mode 100644 doc/guides/bbdevs/acc200.rst
>>>>>>>>      create mode 100644 drivers/baseband/acc200/acc200_pf_enum.h
>>>>>>>>      create mode 100644 drivers/baseband/acc200/acc200_pmd.h
>>>>>>>>      create mode 100644 drivers/baseband/acc200/acc200_vf_enum.h
>>>>>>>>      create mode 100644 drivers/baseband/acc200/meson.build
>>>>>>>>      create mode 100644 drivers/baseband/acc200/rte_acc200_cfg.h
>>>>>>>>      create mode 100644 drivers/baseband/acc200/rte_acc200_pmd.c
>>>>>>>>      create mode 100644 drivers/baseband/acc200/version.map
>>>>>>>>
>>>>>>> Comparing ACC200 & ACC100 header files, I understand ACC200 is an
>>>>>>> evolution of the ACC10x family. The FEC bits are really close,
>>>>>>> ACC200 main addition seems to be FFT acceleration which could be
>>>>>>> handled in ACC10x driver based on device ID.
>>>>>>>
>>>>>>> I think both drivers have to be merged in order to avoid code
>>>>>>> duplication. That's how other families of devices (e.g. i40e) are
>>>>>>> handled.
>>>>>> I haven't seen your reply on this point.
>>>>>> Do you confirm you are working on a single driver for ACC family in
>>>>>> order to avoid code duplication?
>>>>>>
>>>>> The implementation is based on distinct ACC100 and ACC200 drivers.
>>>>> The 2
>>>> devices are fundamentally different generation, processes and IP.
>>>>> MountBryce is an eASIC device over PCIe while ACC200 is an
>>>>> integrated
>>>> accelerator on Xeon CPU.
>>>>> The actual implementation are not the same, underlying IP are all
>>>>> distinct
>>>> even if many of the descriptor format have similarities.
>>>>> The actual capabilities of the acceleration are different and/or new.
>>>>> The workaround and silicon errata are also different causing
>>>>> different
>>>> limitation and implementation in the driver (see the serie with
>>>> ongoing changes for ACC100 in parallel).
>>>>> This is fundamentally distinct from ACC101 which was a derivative
>>>>> product
>>>> from ACC100 and where it made sense to share implementation between
>>>> ACC100 and ACC101.
>>>>> So in a nutshell these 2 devices and drivers are 2 different beasts
>>>>> and the
>>>> intention is to keep them intentionally separate as in the serie.
>>>>> Let me know if unclear, thanks!
>>>> Nic,
>>>>
>>>> I used a similarity checker to compare acc100 and acc200
>>>>
>>>> https://dickgrune.com/Programs/similarity_tester/
>>>>
>>>> l=simum.log
>>>> if [ -f $l ]; then
>>>>        rm $l
>>>> fi
>>>>
>>>> sim_c -s -R -o$l -R -p -P -a .
>>>>
>>>> There results are
>>>>
>>>> ./acc200/acc200_pf_enum.h consists for 100 % of
>>>> ./acc100/acc100_pf_enum.h material ./acc100/acc100_pf_enum.h consists
>>>> for 98 % of ./acc200/acc200_pf_enum.h material
>>>> ./acc100/rte_acc100_pmd.h consists for
>>>> 98 % of ./acc200/acc200_pmd.h material ./acc200/acc200_vf_enum.h
>>>> consists for 95 % of ./acc100/acc100_pf_enum.h material
>>>> ./acc200/acc200_pmd.h consists for 92 % of ./acc100/rte_acc100_pmd.h
>>>> material ./acc200/rte_acc200_cfg.h consists for 92 % of
>>>> ./acc100/rte_acc100_cfg.h material ./acc100/rte_acc100_pmd.c consists
>>>> for 87 % of ./acc200/rte_acc200_pmd.c material
>>>> ./acc100/acc100_vf_enum.h consists for
>>>> 80 % of ./acc200/acc200_pf_enum.h material ./acc200/rte_acc200_pmd.c
>>>> consists for 78 % of ./acc100/rte_acc100_pmd.c material
>>>> ./acc100/rte_acc100_cfg.h consists for 75 % of
>>>> ./acc200/rte_acc200_cfg.h material
>>>>
>>>> Spot checking the first *pf_enum.h at 100%, these are the devices'
>>>> registers, they are the same.
>>>>
>>>> I raised this similarity issue with 100 vs 101.
>>>>
>>>> Having multiple copies is difficult to support and should be avoided.
>>>>
>>>> For the end user, they should have to use only one driver.
>>>>
>>> There are really different IP and do not have the same interface (PCIe/DDR vs
>> integrated) and there is big serie of changes which are specific to ACC100
>> coming in parallel. Any workaround, optimization would be different.
>>> I agree that for the coming serie of integrated accelerator we will use a
>> unified driver approach but for that very case that would be quite messy to
>> artificially put them within the same PMD.
>>
>> How is the IP different when 100% of the registers are the same ?
>>
> These are 2 different HW aspects. The base toplevel configuration registers are kept similar on purpose but the underlying IP are totally different design and implementation.
> Even the registers have differences but not visible here, the actual RDL file would define more specifically these registers bitfields and implementation including which ones are not implemented (but that is proprietary information), and at bbdev level the interface is not some much register based than processing based on data from DMA.
> Basically even if there was a common driver, all these would be duplicated and they are indeed different IP (including different vendors)..
> But I agree with the general intent and to have a common driver for the integrated driver serie (ACC200, ACC300...) now that we are moving away from PCIe/DDR lookaside acceleration and eASIC/FPGA implementation (ACC100/AC101).

Looking a little deeper, at how the driver is lays out some of its 
bitfields and private data by reviewing the

./acc200/acc200_pmd.h consists for 92 % of ./acc100/rte_acc100_pmd.h

There are some minor changes to existing reserved bitfields.
A new structure for fft.
The acc200_device, the private data for the driver, is an exact copy of acc100_device.

acc200_pmd.h is the superset and could be used with little changes as a common acc_pmd.h.
acc200 is doing everything the acc100 did in a very similar if not exact way, adding the fft feature.

Can you point to some portion of this patchset that is so unique that it could not be abstracted to an if-check or function and so requiring this separate, nearly identical driver ?

Tom
  


>> Tom
>>
>>>

Chautru, Nicolas Sept. 1, 2022, 8:34 p.m. UTC | #16

Hi Tom,

> -----Original Message-----
> From: Tom Rix <trix@redhat.com>
> Sent: Thursday, September 1, 2022 6:49 AM
> To: Chautru, Nicolas <nicolas.chautru@intel.com>; Maxime Coquelin
> <maxime.coquelin@redhat.com>; dev@dpdk.org; thomas@monjalon.net;
> gakhil@marvell.com; hemant.agrawal@nxp.com; Vargas, Hernan
> <hernan.vargas@intel.com>
> Cc: mdr@ashroe.eu; Richardson, Bruce <bruce.richardson@intel.com>;
> david.marchand@redhat.com; stephen@networkplumber.org
> Subject: Re: [PATCH v1 00/10] baseband/acc200
> 
> 
> On 8/31/22 6:26 PM, Chautru, Nicolas wrote:
> > Hi Tom,
> >
> >> -----Original Message-----
> >> From: Tom Rix <trix@redhat.com>
> >> Sent: Wednesday, August 31, 2022 5:28 PM
> >> To: Chautru, Nicolas <nicolas.chautru@intel.com>; Maxime Coquelin
> >> <maxime.coquelin@redhat.com>; dev@dpdk.org;
> thomas@monjalon.net;
> >> gakhil@marvell.com; hemant.agrawal@nxp.com; Vargas, Hernan
> >> <hernan.vargas@intel.com>
> >> Cc: mdr@ashroe.eu; Richardson, Bruce <bruce.richardson@intel.com>;
> >> david.marchand@redhat.com; stephen@networkplumber.org
> >> Subject: Re: [PATCH v1 00/10] baseband/acc200
> >>
> >>
> >> On 8/31/22 3:37 PM, Chautru, Nicolas wrote:
> >>> Hi Thomas, Tom,
> >>>
> >>>> -----Original Message-----
> >>>> From: Tom Rix <trix@redhat.com>
> >>>> Sent: Wednesday, August 31, 2022 12:26 PM
> >>>> To: Chautru, Nicolas <nicolas.chautru@intel.com>; Maxime Coquelin
> >>>> <maxime.coquelin@redhat.com>; dev@dpdk.org;
> thomas@monjalon.net;
> >>>> gakhil@marvell.com; hemant.agrawal@nxp.com; Vargas, Hernan
> >>>> <hernan.vargas@intel.com>
> >>>> Cc: mdr@ashroe.eu; Richardson, Bruce <bruce.richardson@intel.com>;
> >>>> david.marchand@redhat.com; stephen@networkplumber.org
> >>>> Subject: Re: [PATCH v1 00/10] baseband/acc200
> >>>>
> >>>>
> >>>> On 8/30/22 12:45 PM, Chautru, Nicolas wrote:
> >>>>> Hi Maxime,
> >>>>>
> >>>>>> -----Original Message-----
> >>>>>> From: Maxime Coquelin <maxime.coquelin@redhat.com>
> >>>>>> Sent: Tuesday, August 30, 2022 12:45 AM
> >>>>>> To: Chautru, Nicolas <nicolas.chautru@intel.com>; dev@dpdk.org;
> >>>>>> thomas@monjalon.net; gakhil@marvell.com;
> hemant.agrawal@nxp.com;
> >>>>>> trix@redhat.com; Vargas, Hernan <hernan.vargas@intel.com>
> >>>>>> Cc: mdr@ashroe.eu; Richardson, Bruce
> >>>>>> <bruce.richardson@intel.com>; david.marchand@redhat.com;
> >>>>>> stephen@networkplumber.org
> >>>>>> Subject: Re: [PATCH v1 00/10] baseband/acc200
> >>>>>>
> >>>>>> Hi Nicolas,
> >>>>>>
> >>>>>> On 7/12/22 15:48, Maxime Coquelin wrote:
> >>>>>>> Hi Nicolas, Hernan,
> >>>>>>>
> >>>>>>> (Adding Hernan in the recipients list)
> >>>>>>>
> >>>>>>> On 7/8/22 02:01, Nicolas Chautru wrote:
> >>>>>>>> This is targeting 22.11 and includes the PMD for the integrated
> >>>>>>>> accelerator on Intel Xeon SPR-EEC.
> >>>>>>>> There is a dependency on that parallel serie still in-flight
> >>>>>>>> which extends the bbdev api
> >>>>>>>> https://patches.dpdk.org/project/dpdk/list/?series=23894
> >>>>>>>>
> >>>>>>>> I will be offline for a few weeks for the summer break but
> >>>>>>>> Hernan will cover for me during that time if required.
> >>>>>>>>
> >>>>>>>> Thanks
> >>>>>>>> Nic
> >>>>>>>>
> >>>>>>>> Nicolas Chautru (10):
> >>>>>>>>       baseband/acc200: introduce PMD for ACC200
> >>>>>>>>       baseband/acc200: add HW register definitions
> >>>>>>>>       baseband/acc200: add info get function
> >>>>>>>>       baseband/acc200: add queue configuration
> >>>>>>>>       baseband/acc200: add LDPC processing functions
> >>>>>>>>       baseband/acc200: add LTE processing functions
> >>>>>>>>       baseband/acc200: add support for FFT operations
> >>>>>>>>       baseband/acc200: support interrupt
> >>>>>>>>       baseband/acc200: add device status and vf2pf comms
> >>>>>>>>       baseband/acc200: add PF configure companion function
> >>>>>>>>
> >>>>>>>>      MAINTAINERS                              |    3 +
> >>>>>>>>      app/test-bbdev/meson.build               |    3 +
> >>>>>>>>      app/test-bbdev/test_bbdev_perf.c         |   76 +
> >>>>>>>>      doc/guides/bbdevs/acc200.rst             |  244 ++
> >>>>>>>>      doc/guides/bbdevs/index.rst              |    1 +
> >>>>>>>>      drivers/baseband/acc200/acc200_pf_enum.h |  468 +++
> >>>>>>>>      drivers/baseband/acc200/acc200_pmd.h     |  690 ++++
> >>>>>>>>      drivers/baseband/acc200/acc200_vf_enum.h |   89 +
> >>>>>>>>      drivers/baseband/acc200/meson.build      |    8 +
> >>>>>>>>      drivers/baseband/acc200/rte_acc200_cfg.h |  115 +
> >>>>>>>>      drivers/baseband/acc200/rte_acc200_pmd.c | 5403
> >>>>>>>> ++++++++++++++++++++++++++++++
> >>>>>>>>      drivers/baseband/acc200/version.map      |   10 +
> >>>>>>>>      drivers/baseband/meson.build             |    1 +
> >>>>>>>>      13 files changed, 7111 insertions(+)
> >>>>>>>>      create mode 100644 doc/guides/bbdevs/acc200.rst
> >>>>>>>>      create mode 100644
> drivers/baseband/acc200/acc200_pf_enum.h
> >>>>>>>>      create mode 100644 drivers/baseband/acc200/acc200_pmd.h
> >>>>>>>>      create mode 100644
> drivers/baseband/acc200/acc200_vf_enum.h
> >>>>>>>>      create mode 100644 drivers/baseband/acc200/meson.build
> >>>>>>>>      create mode 100644
> drivers/baseband/acc200/rte_acc200_cfg.h
> >>>>>>>>      create mode 100644
> drivers/baseband/acc200/rte_acc200_pmd.c
> >>>>>>>>      create mode 100644 drivers/baseband/acc200/version.map
> >>>>>>>>
> >>>>>>> Comparing ACC200 & ACC100 header files, I understand ACC200 is
> >>>>>>> an evolution of the ACC10x family. The FEC bits are really
> >>>>>>> close,
> >>>>>>> ACC200 main addition seems to be FFT acceleration which could be
> >>>>>>> handled in ACC10x driver based on device ID.
> >>>>>>>
> >>>>>>> I think both drivers have to be merged in order to avoid code
> >>>>>>> duplication. That's how other families of devices (e.g. i40e)
> >>>>>>> are handled.
> >>>>>> I haven't seen your reply on this point.
> >>>>>> Do you confirm you are working on a single driver for ACC family
> >>>>>> in order to avoid code duplication?
> >>>>>>
> >>>>> The implementation is based on distinct ACC100 and ACC200 drivers.
> >>>>> The 2
> >>>> devices are fundamentally different generation, processes and IP.
> >>>>> MountBryce is an eASIC device over PCIe while ACC200 is an
> >>>>> integrated
> >>>> accelerator on Xeon CPU.
> >>>>> The actual implementation are not the same, underlying IP are all
> >>>>> distinct
> >>>> even if many of the descriptor format have similarities.
> >>>>> The actual capabilities of the acceleration are different and/or new.
> >>>>> The workaround and silicon errata are also different causing
> >>>>> different
> >>>> limitation and implementation in the driver (see the serie with
> >>>> ongoing changes for ACC100 in parallel).
> >>>>> This is fundamentally distinct from ACC101 which was a derivative
> >>>>> product
> >>>> from ACC100 and where it made sense to share implementation
> between
> >>>> ACC100 and ACC101.
> >>>>> So in a nutshell these 2 devices and drivers are 2 different
> >>>>> beasts and the
> >>>> intention is to keep them intentionally separate as in the serie.
> >>>>> Let me know if unclear, thanks!
> >>>> Nic,
> >>>>
> >>>> I used a similarity checker to compare acc100 and acc200
> >>>>
> >>>> https://dickgrune.com/Programs/similarity_tester/
> >>>>
> >>>> l=simum.log
> >>>> if [ -f $l ]; then
> >>>>        rm $l
> >>>> fi
> >>>>
> >>>> sim_c -s -R -o$l -R -p -P -a .
> >>>>
> >>>> There results are
> >>>>
> >>>> ./acc200/acc200_pf_enum.h consists for 100 % of
> >>>> ./acc100/acc100_pf_enum.h material ./acc100/acc100_pf_enum.h
> >>>> consists for 98 % of ./acc200/acc200_pf_enum.h material
> >>>> ./acc100/rte_acc100_pmd.h consists for
> >>>> 98 % of ./acc200/acc200_pmd.h material ./acc200/acc200_vf_enum.h
> >>>> consists for 95 % of ./acc100/acc100_pf_enum.h material
> >>>> ./acc200/acc200_pmd.h consists for 92 % of
> >>>> ./acc100/rte_acc100_pmd.h material ./acc200/rte_acc200_cfg.h
> >>>> consists for 92 % of ./acc100/rte_acc100_cfg.h material
> >>>> ./acc100/rte_acc100_pmd.c consists for 87 % of
> >>>> ./acc200/rte_acc200_pmd.c material ./acc100/acc100_vf_enum.h
> >>>> consists for
> >>>> 80 % of ./acc200/acc200_pf_enum.h material
> >>>> ./acc200/rte_acc200_pmd.c consists for 78 % of
> >>>> ./acc100/rte_acc100_pmd.c material ./acc100/rte_acc100_cfg.h
> >>>> consists for 75 % of ./acc200/rte_acc200_cfg.h material
> >>>>
> >>>> Spot checking the first *pf_enum.h at 100%, these are the devices'
> >>>> registers, they are the same.
> >>>>
> >>>> I raised this similarity issue with 100 vs 101.
> >>>>
> >>>> Having multiple copies is difficult to support and should be avoided.
> >>>>
> >>>> For the end user, they should have to use only one driver.
> >>>>
> >>> There are really different IP and do not have the same interface
> >>> (PCIe/DDR vs
> >> integrated) and there is big serie of changes which are specific to
> >> ACC100 coming in parallel. Any workaround, optimization would be
> different.
> >>> I agree that for the coming serie of integrated accelerator we will
> >>> use a
> >> unified driver approach but for that very case that would be quite
> >> messy to artificially put them within the same PMD.
> >>
> >> How is the IP different when 100% of the registers are the same ?
> >>
> > These are 2 different HW aspects. The base toplevel configuration registers
> are kept similar on purpose but the underlying IP are totally different design
> and implementation.
> > Even the registers have differences but not visible here, the actual RDL file
> would define more specifically these registers bitfields and implementation
> including which ones are not implemented (but that is proprietary
> information), and at bbdev level the interface is not some much register
> based than processing based on data from DMA.
> > Basically even if there was a common driver, all these would be duplicated
> and they are indeed different IP (including different vendors)..
> > But I agree with the general intent and to have a common driver for the
> integrated driver serie (ACC200, ACC300...) now that we are moving away
> from PCIe/DDR lookaside acceleration and eASIC/FPGA implementation
> (ACC100/AC101).
> 
> Looking a little deeper, at how the driver is lays out some of its bitfields and
> private data by reviewing the
> 
> ./acc200/acc200_pmd.h consists for 92 % of ./acc100/rte_acc100_pmd.h
> 
> There are some minor changes to existing reserved bitfields.
> A new structure for fft.
> The acc200_device, the private data for the driver, is an exact copy of
> acc100_device.
> 
> acc200_pmd.h is the superset and could be used with little changes as a
> common acc_pmd.h.
> acc200 is doing everything the acc100 did in a very similar if not exact way,
> adding the fft feature.
> 
> Can you point to some portion of this patchset that is so unique that it could
> not be abstracted to an if-check or function and so requiring this separate,
> nearly identical driver ?
> 

You used a similarity checker really, there are actually way more relevent differences than what you imply here. 
With regards to the 2 pf_enum.h file, there are many registers that have same or similar names but have now different values being mapped hence you just cannot use one for the other. 
Saying that "./acc200/acc200_pmd.h consists for 92 % of ./acc100/rte_acc100_pmd.h" is just not correct and really irrelevant.
Just do a diff side by side please and check, that should be extremely obvious, that metrics tells more about the similarity checker limitation than anything else. 
Even when using a common driver for ACC200/300 they will have distinct register enum files being auto-generated and coming from distinct RDL.
Again just do a diff of these 2 files. I believe you will agree that is not relevant for these files to try to artificially merged these together. 

With regards to the pmd.h, some structure/defines are indeed common and could be moved to a common file (for instance turboencoder and LDPC encoder which are more vanilla and unlikely to change for future product unlike the decoders which have different feature set and behaviour; or some 3GPP constant that can be defined once).
We can definitely change these to put together shared structures/defines, but not intending to try to artificially put things together with spaghetti code.
We would like to keep 3 parallel versions of these PMD for 3 different product lines which are indeed fundamentally different designs (including different workaround required as can be seen on the parallel ACC100 serie under review).
- one version for FPGA implementation (support for N3000, N6000, ...)
- one version for eASIC lookaside card implementation (ACC100, ACC101, ...)
- one version for the integrated Xeon accelerators (ACC200, ACC300, ...)

Let me know if unclear
Nic







> Tom
> 
> 
> 
> >> Tom
> >>
> >>>

Tom Rix Sept. 6, 2022, 12:51 p.m. UTC | #17

On 9/1/22 1:34 PM, Chautru, Nicolas wrote:
> Hi Tom,
>
>> -----Original Message-----
>> From: Tom Rix <trix@redhat.com>
>> Sent: Thursday, September 1, 2022 6:49 AM
>> To: Chautru, Nicolas <nicolas.chautru@intel.com>; Maxime Coquelin
>> <maxime.coquelin@redhat.com>; dev@dpdk.org; thomas@monjalon.net;
>> gakhil@marvell.com; hemant.agrawal@nxp.com; Vargas, Hernan
>> <hernan.vargas@intel.com>
>> Cc: mdr@ashroe.eu; Richardson, Bruce <bruce.richardson@intel.com>;
>> david.marchand@redhat.com; stephen@networkplumber.org
>> Subject: Re: [PATCH v1 00/10] baseband/acc200
>>
>>
>> On 8/31/22 6:26 PM, Chautru, Nicolas wrote:
>>> Hi Tom,
>>>
>>>> -----Original Message-----
>>>> From: Tom Rix <trix@redhat.com>
>>>> Sent: Wednesday, August 31, 2022 5:28 PM
>>>> To: Chautru, Nicolas <nicolas.chautru@intel.com>; Maxime Coquelin
>>>> <maxime.coquelin@redhat.com>; dev@dpdk.org;
>> thomas@monjalon.net;
>>>> gakhil@marvell.com; hemant.agrawal@nxp.com; Vargas, Hernan
>>>> <hernan.vargas@intel.com>
>>>> Cc: mdr@ashroe.eu; Richardson, Bruce <bruce.richardson@intel.com>;
>>>> david.marchand@redhat.com; stephen@networkplumber.org
>>>> Subject: Re: [PATCH v1 00/10] baseband/acc200
>>>>
>>>>
>>>> On 8/31/22 3:37 PM, Chautru, Nicolas wrote:
>>>>> Hi Thomas, Tom,
>>>>>
>>>>>> -----Original Message-----
>>>>>> From: Tom Rix <trix@redhat.com>
>>>>>> Sent: Wednesday, August 31, 2022 12:26 PM
>>>>>> To: Chautru, Nicolas <nicolas.chautru@intel.com>; Maxime Coquelin
>>>>>> <maxime.coquelin@redhat.com>; dev@dpdk.org;
>> thomas@monjalon.net;
>>>>>> gakhil@marvell.com; hemant.agrawal@nxp.com; Vargas, Hernan
>>>>>> <hernan.vargas@intel.com>
>>>>>> Cc: mdr@ashroe.eu; Richardson, Bruce <bruce.richardson@intel.com>;
>>>>>> david.marchand@redhat.com; stephen@networkplumber.org
>>>>>> Subject: Re: [PATCH v1 00/10] baseband/acc200
>>>>>>
>>>>>>
>>>>>> On 8/30/22 12:45 PM, Chautru, Nicolas wrote:
>>>>>>> Hi Maxime,
>>>>>>>
>>>>>>>> -----Original Message-----
>>>>>>>> From: Maxime Coquelin <maxime.coquelin@redhat.com>
>>>>>>>> Sent: Tuesday, August 30, 2022 12:45 AM
>>>>>>>> To: Chautru, Nicolas <nicolas.chautru@intel.com>; dev@dpdk.org;
>>>>>>>> thomas@monjalon.net; gakhil@marvell.com;
>> hemant.agrawal@nxp.com;
>>>>>>>> trix@redhat.com; Vargas, Hernan <hernan.vargas@intel.com>
>>>>>>>> Cc: mdr@ashroe.eu; Richardson, Bruce
>>>>>>>> <bruce.richardson@intel.com>; david.marchand@redhat.com;
>>>>>>>> stephen@networkplumber.org
>>>>>>>> Subject: Re: [PATCH v1 00/10] baseband/acc200
>>>>>>>>
>>>>>>>> Hi Nicolas,
>>>>>>>>
>>>>>>>> On 7/12/22 15:48, Maxime Coquelin wrote:
>>>>>>>>> Hi Nicolas, Hernan,
>>>>>>>>>
>>>>>>>>> (Adding Hernan in the recipients list)
>>>>>>>>>
>>>>>>>>> On 7/8/22 02:01, Nicolas Chautru wrote:
>>>>>>>>>> This is targeting 22.11 and includes the PMD for the integrated
>>>>>>>>>> accelerator on Intel Xeon SPR-EEC.
>>>>>>>>>> There is a dependency on that parallel serie still in-flight
>>>>>>>>>> which extends the bbdev api
>>>>>>>>>> https://patches.dpdk.org/project/dpdk/list/?series=23894
>>>>>>>>>>
>>>>>>>>>> I will be offline for a few weeks for the summer break but
>>>>>>>>>> Hernan will cover for me during that time if required.
>>>>>>>>>>
>>>>>>>>>> Thanks
>>>>>>>>>> Nic
>>>>>>>>>>
>>>>>>>>>> Nicolas Chautru (10):
>>>>>>>>>>        baseband/acc200: introduce PMD for ACC200
>>>>>>>>>>        baseband/acc200: add HW register definitions
>>>>>>>>>>        baseband/acc200: add info get function
>>>>>>>>>>        baseband/acc200: add queue configuration
>>>>>>>>>>        baseband/acc200: add LDPC processing functions
>>>>>>>>>>        baseband/acc200: add LTE processing functions
>>>>>>>>>>        baseband/acc200: add support for FFT operations
>>>>>>>>>>        baseband/acc200: support interrupt
>>>>>>>>>>        baseband/acc200: add device status and vf2pf comms
>>>>>>>>>>        baseband/acc200: add PF configure companion function
>>>>>>>>>>
>>>>>>>>>>       MAINTAINERS                              |    3 +
>>>>>>>>>>       app/test-bbdev/meson.build               |    3 +
>>>>>>>>>>       app/test-bbdev/test_bbdev_perf.c         |   76 +
>>>>>>>>>>       doc/guides/bbdevs/acc200.rst             |  244 ++
>>>>>>>>>>       doc/guides/bbdevs/index.rst              |    1 +
>>>>>>>>>>       drivers/baseband/acc200/acc200_pf_enum.h |  468 +++
>>>>>>>>>>       drivers/baseband/acc200/acc200_pmd.h     |  690 ++++
>>>>>>>>>>       drivers/baseband/acc200/acc200_vf_enum.h |   89 +
>>>>>>>>>>       drivers/baseband/acc200/meson.build      |    8 +
>>>>>>>>>>       drivers/baseband/acc200/rte_acc200_cfg.h |  115 +
>>>>>>>>>>       drivers/baseband/acc200/rte_acc200_pmd.c | 5403
>>>>>>>>>> ++++++++++++++++++++++++++++++
>>>>>>>>>>       drivers/baseband/acc200/version.map      |   10 +
>>>>>>>>>>       drivers/baseband/meson.build             |    1 +
>>>>>>>>>>       13 files changed, 7111 insertions(+)
>>>>>>>>>>       create mode 100644 doc/guides/bbdevs/acc200.rst
>>>>>>>>>>       create mode 100644
>> drivers/baseband/acc200/acc200_pf_enum.h
>>>>>>>>>>       create mode 100644 drivers/baseband/acc200/acc200_pmd.h
>>>>>>>>>>       create mode 100644
>> drivers/baseband/acc200/acc200_vf_enum.h
>>>>>>>>>>       create mode 100644 drivers/baseband/acc200/meson.build
>>>>>>>>>>       create mode 100644
>> drivers/baseband/acc200/rte_acc200_cfg.h
>>>>>>>>>>       create mode 100644
>> drivers/baseband/acc200/rte_acc200_pmd.c
>>>>>>>>>>       create mode 100644 drivers/baseband/acc200/version.map
>>>>>>>>>>
>>>>>>>>> Comparing ACC200 & ACC100 header files, I understand ACC200 is
>>>>>>>>> an evolution of the ACC10x family. The FEC bits are really
>>>>>>>>> close,
>>>>>>>>> ACC200 main addition seems to be FFT acceleration which could be
>>>>>>>>> handled in ACC10x driver based on device ID.
>>>>>>>>>
>>>>>>>>> I think both drivers have to be merged in order to avoid code
>>>>>>>>> duplication. That's how other families of devices (e.g. i40e)
>>>>>>>>> are handled.
>>>>>>>> I haven't seen your reply on this point.
>>>>>>>> Do you confirm you are working on a single driver for ACC family
>>>>>>>> in order to avoid code duplication?
>>>>>>>>
>>>>>>> The implementation is based on distinct ACC100 and ACC200 drivers.
>>>>>>> The 2
>>>>>> devices are fundamentally different generation, processes and IP.
>>>>>>> MountBryce is an eASIC device over PCIe while ACC200 is an
>>>>>>> integrated
>>>>>> accelerator on Xeon CPU.
>>>>>>> The actual implementation are not the same, underlying IP are all
>>>>>>> distinct
>>>>>> even if many of the descriptor format have similarities.
>>>>>>> The actual capabilities of the acceleration are different and/or new.
>>>>>>> The workaround and silicon errata are also different causing
>>>>>>> different
>>>>>> limitation and implementation in the driver (see the serie with
>>>>>> ongoing changes for ACC100 in parallel).
>>>>>>> This is fundamentally distinct from ACC101 which was a derivative
>>>>>>> product
>>>>>> from ACC100 and where it made sense to share implementation
>> between
>>>>>> ACC100 and ACC101.
>>>>>>> So in a nutshell these 2 devices and drivers are 2 different
>>>>>>> beasts and the
>>>>>> intention is to keep them intentionally separate as in the serie.
>>>>>>> Let me know if unclear, thanks!
>>>>>> Nic,
>>>>>>
>>>>>> I used a similarity checker to compare acc100 and acc200
>>>>>>
>>>>>> https://dickgrune.com/Programs/similarity_tester/
>>>>>>
>>>>>> l=simum.log
>>>>>> if [ -f $l ]; then
>>>>>>         rm $l
>>>>>> fi
>>>>>>
>>>>>> sim_c -s -R -o$l -R -p -P -a .
>>>>>>
>>>>>> There results are
>>>>>>
>>>>>> ./acc200/acc200_pf_enum.h consists for 100 % of
>>>>>> ./acc100/acc100_pf_enum.h material ./acc100/acc100_pf_enum.h
>>>>>> consists for 98 % of ./acc200/acc200_pf_enum.h material
>>>>>> ./acc100/rte_acc100_pmd.h consists for
>>>>>> 98 % of ./acc200/acc200_pmd.h material ./acc200/acc200_vf_enum.h
>>>>>> consists for 95 % of ./acc100/acc100_pf_enum.h material
>>>>>> ./acc200/acc200_pmd.h consists for 92 % of
>>>>>> ./acc100/rte_acc100_pmd.h material ./acc200/rte_acc200_cfg.h
>>>>>> consists for 92 % of ./acc100/rte_acc100_cfg.h material
>>>>>> ./acc100/rte_acc100_pmd.c consists for 87 % of
>>>>>> ./acc200/rte_acc200_pmd.c material ./acc100/acc100_vf_enum.h
>>>>>> consists for
>>>>>> 80 % of ./acc200/acc200_pf_enum.h material
>>>>>> ./acc200/rte_acc200_pmd.c consists for 78 % of
>>>>>> ./acc100/rte_acc100_pmd.c material ./acc100/rte_acc100_cfg.h
>>>>>> consists for 75 % of ./acc200/rte_acc200_cfg.h material
>>>>>>
>>>>>> Spot checking the first *pf_enum.h at 100%, these are the devices'
>>>>>> registers, they are the same.
>>>>>>
>>>>>> I raised this similarity issue with 100 vs 101.
>>>>>>
>>>>>> Having multiple copies is difficult to support and should be avoided.
>>>>>>
>>>>>> For the end user, they should have to use only one driver.
>>>>>>
>>>>> There are really different IP and do not have the same interface
>>>>> (PCIe/DDR vs
>>>> integrated) and there is big serie of changes which are specific to
>>>> ACC100 coming in parallel. Any workaround, optimization would be
>> different.
>>>>> I agree that for the coming serie of integrated accelerator we will
>>>>> use a
>>>> unified driver approach but for that very case that would be quite
>>>> messy to artificially put them within the same PMD.
>>>>
>>>> How is the IP different when 100% of the registers are the same ?
>>>>
>>> These are 2 different HW aspects. The base toplevel configuration registers
>> are kept similar on purpose but the underlying IP are totally different design
>> and implementation.
>>> Even the registers have differences but not visible here, the actual RDL file
>> would define more specifically these registers bitfields and implementation
>> including which ones are not implemented (but that is proprietary
>> information), and at bbdev level the interface is not some much register
>> based than processing based on data from DMA.
>>> Basically even if there was a common driver, all these would be duplicated
>> and they are indeed different IP (including different vendors)..
>>> But I agree with the general intent and to have a common driver for the
>> integrated driver serie (ACC200, ACC300...) now that we are moving away
>> from PCIe/DDR lookaside acceleration and eASIC/FPGA implementation
>> (ACC100/AC101).
>>
>> Looking a little deeper, at how the driver is lays out some of its bitfields and
>> private data by reviewing the
>>
>> ./acc200/acc200_pmd.h consists for 92 % of ./acc100/rte_acc100_pmd.h
>>
>> There are some minor changes to existing reserved bitfields.
>> A new structure for fft.
>> The acc200_device, the private data for the driver, is an exact copy of
>> acc100_device.
>>
>> acc200_pmd.h is the superset and could be used with little changes as a
>> common acc_pmd.h.
>> acc200 is doing everything the acc100 did in a very similar if not exact way,
>> adding the fft feature.
>>
>> Can you point to some portion of this patchset that is so unique that it could
>> not be abstracted to an if-check or function and so requiring this separate,
>> nearly identical driver ?
>>
> You used a similarity checker really, there are actually way more relevent differences than what you imply here.
> With regards to the 2 pf_enum.h file, there are many registers that have same or similar names but have now different values being mapped hence you just cannot use one for the other.
> Saying that "./acc200/acc200_pmd.h consists for 92 % of ./acc100/rte_acc100_pmd.h" is just not correct and really irrelevant.
> Just do a diff side by side please and check, that should be extremely obvious, that metrics tells more about the similarity checker limitation than anything else.
> Even when using a common driver for ACC200/300 they will have distinct register enum files being auto-generated and coming from distinct RDL.
> Again just do a diff of these 2 files. I believe you will agree that is not relevant for these files to try to artificially merged these together.
>
> With regards to the pmd.h, some structure/defines are indeed common and could be moved to a common file (for instance turboencoder and LDPC encoder which are more vanilla and unlikely to change for future product unlike the decoders which have different feature set and behaviour; or some 3GPP constant that can be defined once).
> We can definitely change these to put together shared structures/defines, but not intending to try to artificially put things together with spaghetti code.
> We would like to keep 3 parallel versions of these PMD for 3 different product lines which are indeed fundamentally different designs (including different workaround required as can be seen on the parallel ACC100 serie under review).
> - one version for FPGA implementation (support for N3000, N6000, ...)
> - one version for eASIC lookaside card implementation (ACC100, ACC101, ...)
> - one version for the integrated Xeon accelerators (ACC200, ACC300, ...)

Some suggestions on refactoring,

For the registers, have a common file.

For the shared functionality, ex/ ldpc encoder, break these out to its 
own shared file.

The public interface, see my earlier comments on the documentation, 
should be have the same interfaces and the few differences highlighted.

Tom

>
> Let me know if unclear
> Nic
>
>
>
>
>
>
>
>> Tom
>>
>>
>>
>>>> Tom
>>>>

Thomas Monjalon Sept. 14, 2022, 10:35 a.m. UTC | #18

06/09/2022 14:51, Tom Rix:
> On 9/1/22 1:34 PM, Chautru, Nicolas wrote:
> > From: Tom Rix <trix@redhat.com>
> >> On 8/31/22 6:26 PM, Chautru, Nicolas wrote:
> >>> From: Tom Rix <trix@redhat.com>
> >>>> On 8/31/22 3:37 PM, Chautru, Nicolas wrote:
> >>>>>>>>> Comparing ACC200 & ACC100 header files, I understand ACC200 is
> >>>>>>>>> an evolution of the ACC10x family. The FEC bits are really
> >>>>>>>>> close,
> >>>>>>>>> ACC200 main addition seems to be FFT acceleration which could be
> >>>>>>>>> handled in ACC10x driver based on device ID.
> >>>>>>>>>
> >>>>>>>>> I think both drivers have to be merged in order to avoid code
> >>>>>>>>> duplication. That's how other families of devices (e.g. i40e)
> >>>>>>>>> are handled.
> >>>>>>>> I haven't seen your reply on this point.
> >>>>>>>> Do you confirm you are working on a single driver for ACC family
> >>>>>>>> in order to avoid code duplication?
> >>>>>>>>
> >>>>>>> The implementation is based on distinct ACC100 and ACC200 drivers.
> >>>>>>> The 2
> >>>>>> devices are fundamentally different generation, processes and IP.
> >>>>>>> MountBryce is an eASIC device over PCIe while ACC200 is an
> >>>>>>> integrated
> >>>>>> accelerator on Xeon CPU.
> >>>>>>> The actual implementation are not the same, underlying IP are all
> >>>>>>> distinct
> >>>>>> even if many of the descriptor format have similarities.
> >>>>>>> The actual capabilities of the acceleration are different and/or new.
> >>>>>>> The workaround and silicon errata are also different causing
> >>>>>>> different
> >>>>>> limitation and implementation in the driver (see the serie with
> >>>>>> ongoing changes for ACC100 in parallel).
> >>>>>>> This is fundamentally distinct from ACC101 which was a derivative
> >>>>>>> product
> >>>>>> from ACC100 and where it made sense to share implementation
> >> between
> >>>>>> ACC100 and ACC101.
> >>>>>>> So in a nutshell these 2 devices and drivers are 2 different
> >>>>>>> beasts and the
> >>>>>> intention is to keep them intentionally separate as in the serie.
> >>>>>>> Let me know if unclear, thanks!
> >>>>>> Nic,
> >>>>>>
> >>>>>> I used a similarity checker to compare acc100 and acc200
> >>>>>>
> >>>>>> https://dickgrune.com/Programs/similarity_tester/
> >>>>>>
> >>>>>> l=simum.log
> >>>>>> if [ -f $l ]; then
> >>>>>>         rm $l
> >>>>>> fi
> >>>>>>
> >>>>>> sim_c -s -R -o$l -R -p -P -a .
> >>>>>>
> >>>>>> There results are
> >>>>>>
> >>>>>> ./acc200/acc200_pf_enum.h consists for 100 % of
> >>>>>> ./acc100/acc100_pf_enum.h material ./acc100/acc100_pf_enum.h
> >>>>>> consists for 98 % of ./acc200/acc200_pf_enum.h material
> >>>>>> ./acc100/rte_acc100_pmd.h consists for
> >>>>>> 98 % of ./acc200/acc200_pmd.h material ./acc200/acc200_vf_enum.h
> >>>>>> consists for 95 % of ./acc100/acc100_pf_enum.h material
> >>>>>> ./acc200/acc200_pmd.h consists for 92 % of
> >>>>>> ./acc100/rte_acc100_pmd.h material ./acc200/rte_acc200_cfg.h
> >>>>>> consists for 92 % of ./acc100/rte_acc100_cfg.h material
> >>>>>> ./acc100/rte_acc100_pmd.c consists for 87 % of
> >>>>>> ./acc200/rte_acc200_pmd.c material ./acc100/acc100_vf_enum.h
> >>>>>> consists for
> >>>>>> 80 % of ./acc200/acc200_pf_enum.h material
> >>>>>> ./acc200/rte_acc200_pmd.c consists for 78 % of
> >>>>>> ./acc100/rte_acc100_pmd.c material ./acc100/rte_acc100_cfg.h
> >>>>>> consists for 75 % of ./acc200/rte_acc200_cfg.h material
> >>>>>>
> >>>>>> Spot checking the first *pf_enum.h at 100%, these are the devices'
> >>>>>> registers, they are the same.
> >>>>>>
> >>>>>> I raised this similarity issue with 100 vs 101.
> >>>>>>
> >>>>>> Having multiple copies is difficult to support and should be avoided.
> >>>>>>
> >>>>>> For the end user, they should have to use only one driver.
> >>>>>>
> >>>>> There are really different IP and do not have the same interface
> >>>>> (PCIe/DDR vs
> >>>> integrated) and there is big serie of changes which are specific to
> >>>> ACC100 coming in parallel. Any workaround, optimization would be
> >> different.
> >>>>> I agree that for the coming serie of integrated accelerator we will
> >>>>> use a
> >>>> unified driver approach but for that very case that would be quite
> >>>> messy to artificially put them within the same PMD.
> >>>>
> >>>> How is the IP different when 100% of the registers are the same ?
> >>>>
> >>> These are 2 different HW aspects. The base toplevel configuration registers
> >> are kept similar on purpose but the underlying IP are totally different design
> >> and implementation.
> >>> Even the registers have differences but not visible here, the actual RDL file
> >> would define more specifically these registers bitfields and implementation
> >> including which ones are not implemented (but that is proprietary
> >> information), and at bbdev level the interface is not some much register
> >> based than processing based on data from DMA.
> >>> Basically even if there was a common driver, all these would be duplicated
> >> and they are indeed different IP (including different vendors)..
> >>> But I agree with the general intent and to have a common driver for the
> >> integrated driver serie (ACC200, ACC300...) now that we are moving away
> >> from PCIe/DDR lookaside acceleration and eASIC/FPGA implementation
> >> (ACC100/AC101).
> >>
> >> Looking a little deeper, at how the driver is lays out some of its bitfields and
> >> private data by reviewing the
> >>
> >> ./acc200/acc200_pmd.h consists for 92 % of ./acc100/rte_acc100_pmd.h
> >>
> >> There are some minor changes to existing reserved bitfields.
> >> A new structure for fft.
> >> The acc200_device, the private data for the driver, is an exact copy of
> >> acc100_device.
> >>
> >> acc200_pmd.h is the superset and could be used with little changes as a
> >> common acc_pmd.h.
> >> acc200 is doing everything the acc100 did in a very similar if not exact way,
> >> adding the fft feature.
> >>
> >> Can you point to some portion of this patchset that is so unique that it could
> >> not be abstracted to an if-check or function and so requiring this separate,
> >> nearly identical driver ?
> >>
> > You used a similarity checker really, there are actually way more relevent differences than what you imply here.
> > With regards to the 2 pf_enum.h file, there are many registers that have same or similar names but have now different values being mapped hence you just cannot use one for the other.
> > Saying that "./acc200/acc200_pmd.h consists for 92 % of ./acc100/rte_acc100_pmd.h" is just not correct and really irrelevant.
> > Just do a diff side by side please and check, that should be extremely obvious, that metrics tells more about the similarity checker limitation than anything else.
> > Even when using a common driver for ACC200/300 they will have distinct register enum files being auto-generated and coming from distinct RDL.
> > Again just do a diff of these 2 files. I believe you will agree that is not relevant for these files to try to artificially merged these together.
> >
> > With regards to the pmd.h, some structure/defines are indeed common and could be moved to a common file (for instance turboencoder and LDPC encoder which are more vanilla and unlikely to change for future product unlike the decoders which have different feature set and behaviour; or some 3GPP constant that can be defined once).
> > We can definitely change these to put together shared structures/defines, but not intending to try to artificially put things together with spaghetti code.
> > We would like to keep 3 parallel versions of these PMD for 3 different product lines which are indeed fundamentally different designs (including different workaround required as can be seen on the parallel ACC100 serie under review).
> > - one version for FPGA implementation (support for N3000, N6000, ...)
> > - one version for eASIC lookaside card implementation (ACC100, ACC101, ...)
> > - one version for the integrated Xeon accelerators (ACC200, ACC300, ...)
> 
> Some suggestions on refactoring,
> 
> For the registers, have a common file.
> 
> For the shared functionality, ex/ ldpc encoder, break these out to its 
> own shared file.
> 
> The public interface, see my earlier comments on the documentation, 
> should be have the same interfaces and the few differences highlighted.

+1 to have common files, and all in a single directory drivers/baseband/acc100/

Maxime Coquelin Sept. 14, 2022, 11:50 a.m. UTC | #19

On 9/14/22 12:35, Thomas Monjalon wrote:
> 06/09/2022 14:51, Tom Rix:
>> On 9/1/22 1:34 PM, Chautru, Nicolas wrote:
>>> From: Tom Rix <trix@redhat.com>
>>>> On 8/31/22 6:26 PM, Chautru, Nicolas wrote:
>>>>> From: Tom Rix <trix@redhat.com>
>>>>>> On 8/31/22 3:37 PM, Chautru, Nicolas wrote:
>>>>>>>>>>> Comparing ACC200 & ACC100 header files, I understand ACC200 is
>>>>>>>>>>> an evolution of the ACC10x family. The FEC bits are really
>>>>>>>>>>> close,
>>>>>>>>>>> ACC200 main addition seems to be FFT acceleration which could be
>>>>>>>>>>> handled in ACC10x driver based on device ID.
>>>>>>>>>>>
>>>>>>>>>>> I think both drivers have to be merged in order to avoid code
>>>>>>>>>>> duplication. That's how other families of devices (e.g. i40e)
>>>>>>>>>>> are handled.
>>>>>>>>>> I haven't seen your reply on this point.
>>>>>>>>>> Do you confirm you are working on a single driver for ACC family
>>>>>>>>>> in order to avoid code duplication?
>>>>>>>>>>
>>>>>>>>> The implementation is based on distinct ACC100 and ACC200 drivers.
>>>>>>>>> The 2
>>>>>>>> devices are fundamentally different generation, processes and IP.
>>>>>>>>> MountBryce is an eASIC device over PCIe while ACC200 is an
>>>>>>>>> integrated
>>>>>>>> accelerator on Xeon CPU.
>>>>>>>>> The actual implementation are not the same, underlying IP are all
>>>>>>>>> distinct
>>>>>>>> even if many of the descriptor format have similarities.
>>>>>>>>> The actual capabilities of the acceleration are different and/or new.
>>>>>>>>> The workaround and silicon errata are also different causing
>>>>>>>>> different
>>>>>>>> limitation and implementation in the driver (see the serie with
>>>>>>>> ongoing changes for ACC100 in parallel).
>>>>>>>>> This is fundamentally distinct from ACC101 which was a derivative
>>>>>>>>> product
>>>>>>>> from ACC100 and where it made sense to share implementation
>>>> between
>>>>>>>> ACC100 and ACC101.
>>>>>>>>> So in a nutshell these 2 devices and drivers are 2 different
>>>>>>>>> beasts and the
>>>>>>>> intention is to keep them intentionally separate as in the serie.
>>>>>>>>> Let me know if unclear, thanks!
>>>>>>>> Nic,
>>>>>>>>
>>>>>>>> I used a similarity checker to compare acc100 and acc200
>>>>>>>>
>>>>>>>> https://dickgrune.com/Programs/similarity_tester/
>>>>>>>>
>>>>>>>> l=simum.log
>>>>>>>> if [ -f $l ]; then
>>>>>>>>          rm $l
>>>>>>>> fi
>>>>>>>>
>>>>>>>> sim_c -s -R -o$l -R -p -P -a .
>>>>>>>>
>>>>>>>> There results are
>>>>>>>>
>>>>>>>> ./acc200/acc200_pf_enum.h consists for 100 % of
>>>>>>>> ./acc100/acc100_pf_enum.h material ./acc100/acc100_pf_enum.h
>>>>>>>> consists for 98 % of ./acc200/acc200_pf_enum.h material
>>>>>>>> ./acc100/rte_acc100_pmd.h consists for
>>>>>>>> 98 % of ./acc200/acc200_pmd.h material ./acc200/acc200_vf_enum.h
>>>>>>>> consists for 95 % of ./acc100/acc100_pf_enum.h material
>>>>>>>> ./acc200/acc200_pmd.h consists for 92 % of
>>>>>>>> ./acc100/rte_acc100_pmd.h material ./acc200/rte_acc200_cfg.h
>>>>>>>> consists for 92 % of ./acc100/rte_acc100_cfg.h material
>>>>>>>> ./acc100/rte_acc100_pmd.c consists for 87 % of
>>>>>>>> ./acc200/rte_acc200_pmd.c material ./acc100/acc100_vf_enum.h
>>>>>>>> consists for
>>>>>>>> 80 % of ./acc200/acc200_pf_enum.h material
>>>>>>>> ./acc200/rte_acc200_pmd.c consists for 78 % of
>>>>>>>> ./acc100/rte_acc100_pmd.c material ./acc100/rte_acc100_cfg.h
>>>>>>>> consists for 75 % of ./acc200/rte_acc200_cfg.h material
>>>>>>>>
>>>>>>>> Spot checking the first *pf_enum.h at 100%, these are the devices'
>>>>>>>> registers, they are the same.
>>>>>>>>
>>>>>>>> I raised this similarity issue with 100 vs 101.
>>>>>>>>
>>>>>>>> Having multiple copies is difficult to support and should be avoided.
>>>>>>>>
>>>>>>>> For the end user, they should have to use only one driver.
>>>>>>>>
>>>>>>> There are really different IP and do not have the same interface
>>>>>>> (PCIe/DDR vs
>>>>>> integrated) and there is big serie of changes which are specific to
>>>>>> ACC100 coming in parallel. Any workaround, optimization would be
>>>> different.
>>>>>>> I agree that for the coming serie of integrated accelerator we will
>>>>>>> use a
>>>>>> unified driver approach but for that very case that would be quite
>>>>>> messy to artificially put them within the same PMD.
>>>>>>
>>>>>> How is the IP different when 100% of the registers are the same ?
>>>>>>
>>>>> These are 2 different HW aspects. The base toplevel configuration registers
>>>> are kept similar on purpose but the underlying IP are totally different design
>>>> and implementation.
>>>>> Even the registers have differences but not visible here, the actual RDL file
>>>> would define more specifically these registers bitfields and implementation
>>>> including which ones are not implemented (but that is proprietary
>>>> information), and at bbdev level the interface is not some much register
>>>> based than processing based on data from DMA.
>>>>> Basically even if there was a common driver, all these would be duplicated
>>>> and they are indeed different IP (including different vendors)..
>>>>> But I agree with the general intent and to have a common driver for the
>>>> integrated driver serie (ACC200, ACC300...) now that we are moving away
>>>> from PCIe/DDR lookaside acceleration and eASIC/FPGA implementation
>>>> (ACC100/AC101).
>>>>
>>>> Looking a little deeper, at how the driver is lays out some of its bitfields and
>>>> private data by reviewing the
>>>>
>>>> ./acc200/acc200_pmd.h consists for 92 % of ./acc100/rte_acc100_pmd.h
>>>>
>>>> There are some minor changes to existing reserved bitfields.
>>>> A new structure for fft.
>>>> The acc200_device, the private data for the driver, is an exact copy of
>>>> acc100_device.
>>>>
>>>> acc200_pmd.h is the superset and could be used with little changes as a
>>>> common acc_pmd.h.
>>>> acc200 is doing everything the acc100 did in a very similar if not exact way,
>>>> adding the fft feature.
>>>>
>>>> Can you point to some portion of this patchset that is so unique that it could
>>>> not be abstracted to an if-check or function and so requiring this separate,
>>>> nearly identical driver ?
>>>>
>>> You used a similarity checker really, there are actually way more relevent differences than what you imply here.
>>> With regards to the 2 pf_enum.h file, there are many registers that have same or similar names but have now different values being mapped hence you just cannot use one for the other.
>>> Saying that "./acc200/acc200_pmd.h consists for 92 % of ./acc100/rte_acc100_pmd.h" is just not correct and really irrelevant.
>>> Just do a diff side by side please and check, that should be extremely obvious, that metrics tells more about the similarity checker limitation than anything else.
>>> Even when using a common driver for ACC200/300 they will have distinct register enum files being auto-generated and coming from distinct RDL.
>>> Again just do a diff of these 2 files. I believe you will agree that is not relevant for these files to try to artificially merged these together.
>>>
>>> With regards to the pmd.h, some structure/defines are indeed common and could be moved to a common file (for instance turboencoder and LDPC encoder which are more vanilla and unlikely to change for future product unlike the decoders which have different feature set and behaviour; or some 3GPP constant that can be defined once).
>>> We can definitely change these to put together shared structures/defines, but not intending to try to artificially put things together with spaghetti code.
>>> We would like to keep 3 parallel versions of these PMD for 3 different product lines which are indeed fundamentally different designs (including different workaround required as can be seen on the parallel ACC100 serie under review).
>>> - one version for FPGA implementation (support for N3000, N6000, ...)
>>> - one version for eASIC lookaside card implementation (ACC100, ACC101, ...)
>>> - one version for the integrated Xeon accelerators (ACC200, ACC300, ...)
>>
>> Some suggestions on refactoring,
>>
>> For the registers, have a common file.
>>
>> For the shared functionality, ex/ ldpc encoder, break these out to its
>> own shared file.
>>
>> The public interface, see my earlier comments on the documentation,
>> should be have the same interfaces and the few differences highlighted.
> 
> +1 to have common files, and all in a single directory drivers/baseband/acc100/

Jus to be sure we are aligned, do you mean to have both drivers in the
same directory, which will share some common files? That's the way I
would go.

Thanks,
Maxime

Bruce Richardson Sept. 14, 2022, 1:19 p.m. UTC | #20

On Wed, Sep 14, 2022 at 01:50:05PM +0200, Maxime Coquelin wrote:
> 
> 
> On 9/14/22 12:35, Thomas Monjalon wrote:
> > 06/09/2022 14:51, Tom Rix:
> > > On 9/1/22 1:34 PM, Chautru, Nicolas wrote:
> > > > From: Tom Rix <trix@redhat.com>
> > > > > On 8/31/22 6:26 PM, Chautru, Nicolas wrote:
> > > > > > From: Tom Rix <trix@redhat.com>
> > > > > > > On 8/31/22 3:37 PM, Chautru, Nicolas wrote:
> > > > > > > > > > > > Comparing ACC200 & ACC100 header files, I
> > > > > > > > > > > > understand ACC200 is an evolution of the ACC10x
> > > > > > > > > > > > family. The FEC bits are really close, ACC200 main
> > > > > > > > > > > > addition seems to be FFT acceleration which could
> > > > > > > > > > > > be handled in ACC10x driver based on device ID.
> > > > > > > > > > > > 
> > > > > > > > > > > > I think both drivers have to be merged in order to
> > > > > > > > > > > > avoid code duplication. That's how other families
> > > > > > > > > > > > of devices (e.g. i40e) are handled.
> > > > > > > > > > > I haven't seen your reply on this point.  Do you
> > > > > > > > > > > confirm you are working on a single driver for ACC
> > > > > > > > > > > family in order to avoid code duplication?
> > > > > > > > > > > 
> > > > > > > > > > The implementation is based on distinct ACC100 and
> > > > > > > > > > ACC200 drivers.  The 2
> > > > > > > > > devices are fundamentally different generation, processes
> > > > > > > > > and IP.
> > > > > > > > > > MountBryce is an eASIC device over PCIe while ACC200 is
> > > > > > > > > > an integrated
> > > > > > > > > accelerator on Xeon CPU.
> > > > > > > > > > The actual implementation are not the same, underlying
> > > > > > > > > > IP are all distinct
> > > > > > > > > even if many of the descriptor format have similarities.
> > > > > > > > > > The actual capabilities of the acceleration are
> > > > > > > > > > different and/or new.  The workaround and silicon
> > > > > > > > > > errata are also different causing different
> > > > > > > > > limitation and implementation in the driver (see the
> > > > > > > > > serie with ongoing changes for ACC100 in parallel).
> > > > > > > > > > This is fundamentally distinct from ACC101 which was a
> > > > > > > > > > derivative product
> > > > > > > > > from ACC100 and where it made sense to share
> > > > > > > > > implementation
> > > > > between
> > > > > > > > > ACC100 and ACC101.
> > > > > > > > > > So in a nutshell these 2 devices and drivers are 2
> > > > > > > > > > different beasts and the
> > > > > > > > > intention is to keep them intentionally separate as in
> > > > > > > > > the serie.
> > > > > > > > > > Let me know if unclear, thanks!
> > > > > > > > > Nic,
> > > > > > > > > 
> > > > > > > > > I used a similarity checker to compare acc100 and acc200
> > > > > > > > > 
> > > > > > > > > https://dickgrune.com/Programs/similarity_tester/
> > > > > > > > > 
> > > > > > > > > l=simum.log if [ -f $l ]; then rm $l fi
> > > > > > > > > 
> > > > > > > > > sim_c -s -R -o$l -R -p -P -a .
> > > > > > > > > 
> > > > > > > > > There results are
> > > > > > > > > 
> > > > > > > > > ./acc200/acc200_pf_enum.h consists for 100 % of
> > > > > > > > > ./acc100/acc100_pf_enum.h material
> > > > > > > > > ./acc100/acc100_pf_enum.h consists for 98 % of
> > > > > > > > > ./acc200/acc200_pf_enum.h material
> > > > > > > > > ./acc100/rte_acc100_pmd.h consists for 98 % of
> > > > > > > > > ./acc200/acc200_pmd.h material ./acc200/acc200_vf_enum.h
> > > > > > > > > consists for 95 % of ./acc100/acc100_pf_enum.h material
> > > > > > > > > ./acc200/acc200_pmd.h consists for 92 % of
> > > > > > > > > ./acc100/rte_acc100_pmd.h material
> > > > > > > > > ./acc200/rte_acc200_cfg.h consists for 92 % of
> > > > > > > > > ./acc100/rte_acc100_cfg.h material
> > > > > > > > > ./acc100/rte_acc100_pmd.c consists for 87 % of
> > > > > > > > > ./acc200/rte_acc200_pmd.c material
> > > > > > > > > ./acc100/acc100_vf_enum.h consists for 80 % of
> > > > > > > > > ./acc200/acc200_pf_enum.h material
> > > > > > > > > ./acc200/rte_acc200_pmd.c consists for 78 % of
> > > > > > > > > ./acc100/rte_acc100_pmd.c material
> > > > > > > > > ./acc100/rte_acc100_cfg.h consists for 75 % of
> > > > > > > > > ./acc200/rte_acc200_cfg.h material
> > > > > > > > > 
> > > > > > > > > Spot checking the first *pf_enum.h at 100%, these are the
> > > > > > > > > devices' registers, they are the same.
> > > > > > > > > 
> > > > > > > > > I raised this similarity issue with 100 vs 101.
> > > > > > > > > 
> > > > > > > > > Having multiple copies is difficult to support and should
> > > > > > > > > be avoided.
> > > > > > > > > 
> > > > > > > > > For the end user, they should have to use only one
> > > > > > > > > driver.
> > > > > > > > > 
> > > > > > > > There are really different IP and do not have the same
> > > > > > > > interface (PCIe/DDR vs
> > > > > > > integrated) and there is big serie of changes which are
> > > > > > > specific to ACC100 coming in parallel. Any workaround,
> > > > > > > optimization would be
> > > > > different.
> > > > > > > > I agree that for the coming serie of integrated accelerator
> > > > > > > > we will use a
> > > > > > > unified driver approach but for that very case that would be
> > > > > > > quite messy to artificially put them within the same PMD.
> > > > > > > 
> > > > > > > How is the IP different when 100% of the registers are the
> > > > > > > same ?
> > > > > > > 
> > > > > > These are 2 different HW aspects. The base toplevel
> > > > > > configuration registers
> > > > > are kept similar on purpose but the underlying IP are totally
> > > > > different design and implementation.
> > > > > > Even the registers have differences but not visible here, the
> > > > > > actual RDL file
> > > > > would define more specifically these registers bitfields and
> > > > > implementation including which ones are not implemented (but that
> > > > > is proprietary information), and at bbdev level the interface is
> > > > > not some much register based than processing based on data from
> > > > > DMA.
> > > > > > Basically even if there was a common driver, all these would be
> > > > > > duplicated
> > > > > and they are indeed different IP (including different vendors)..
> > > > > > But I agree with the general intent and to have a common driver
> > > > > > for the
> > > > > integrated driver serie (ACC200, ACC300...) now that we are
> > > > > moving away from PCIe/DDR lookaside acceleration and eASIC/FPGA
> > > > > implementation (ACC100/AC101).
> > > > > 
> > > > > Looking a little deeper, at how the driver is lays out some of
> > > > > its bitfields and private data by reviewing the
> > > > > 
> > > > > ./acc200/acc200_pmd.h consists for 92 % of
> > > > > ./acc100/rte_acc100_pmd.h
> > > > > 
> > > > > There are some minor changes to existing reserved bitfields.  A
> > > > > new structure for fft.  The acc200_device, the private data for
> > > > > the driver, is an exact copy of acc100_device.
> > > > > 
> > > > > acc200_pmd.h is the superset and could be used with little
> > > > > changes as a common acc_pmd.h.  acc200 is doing everything the
> > > > > acc100 did in a very similar if not exact way, adding the fft
> > > > > feature.
> > > > > 
> > > > > Can you point to some portion of this patchset that is so unique
> > > > > that it could not be abstracted to an if-check or function and so
> > > > > requiring this separate, nearly identical driver ?
> > > > > 
> > > > You used a similarity checker really, there are actually way more
> > > > relevent differences than what you imply here.  With regards to the
> > > > 2 pf_enum.h file, there are many registers that have same or
> > > > similar names but have now different values being mapped hence you
> > > > just cannot use one for the other.  Saying that
> > > > "./acc200/acc200_pmd.h consists for 92 % of
> > > > ./acc100/rte_acc100_pmd.h" is just not correct and really
> > > > irrelevant.  Just do a diff side by side please and check, that
> > > > should be extremely obvious, that metrics tells more about the
> > > > similarity checker limitation than anything else.  Even when using
> > > > a common driver for ACC200/300 they will have distinct register
> > > > enum files being auto-generated and coming from distinct RDL.
> > > > Again just do a diff of these 2 files. I believe you will agree
> > > > that is not relevant for these files to try to artificially merged
> > > > these together.
> > > > 
> > > > With regards to the pmd.h, some structure/defines are indeed common
> > > > and could be moved to a common file (for instance turboencoder and
> > > > LDPC encoder which are more vanilla and unlikely to change for
> > > > future product unlike the decoders which have different feature set
> > > > and behaviour; or some 3GPP constant that can be defined once).  We
> > > > can definitely change these to put together shared
> > > > structures/defines, but not intending to try to artificially put
> > > > things together with spaghetti code.  We would like to keep 3
> > > > parallel versions of these PMD for 3 different product lines which
> > > > are indeed fundamentally different designs (including different
> > > > workaround required as can be seen on the parallel ACC100 serie
> > > > under review).  - one version for FPGA implementation (support for
> > > > N3000, N6000, ...) - one version for eASIC lookaside card
> > > > implementation (ACC100, ACC101, ...) - one version for the
> > > > integrated Xeon accelerators (ACC200, ACC300, ...)
> > > 
> > > Some suggestions on refactoring,
> > > 
> > > For the registers, have a common file.
> > > 
> > > For the shared functionality, ex/ ldpc encoder, break these out to
> > > its own shared file.
> > > 
> > > The public interface, see my earlier comments on the documentation,
> > > should be have the same interfaces and the few differences
> > > highlighted.
> > 
> > +1 to have common files, and all in a single directory
> > drivers/baseband/acc100/
> 
> Jus to be sure we are aligned, do you mean to have both drivers in the
> same directory, which will share some common files? That's the way I
> would go.
> 

I think the expectation is that the two drivers will diverge in future, so
having separate directories should be ok, even with common files placed in
one directory are shared with another. With meson include paths its pretty
trivial to manage if it's just header files, and even if there are common C
files, there is always the option of using drivers/common if we want to
split them out. As I understand it, right now it's only headers inluding
functions which can be static inline, so simple sharing via include paths
should work fine.

/Bruce

Maxime Coquelin Sept. 14, 2022, 1:27 p.m. UTC | #21

On 9/14/22 15:19, Bruce Richardson wrote:
> On Wed, Sep 14, 2022 at 01:50:05PM +0200, Maxime Coquelin wrote:
>>
>>
>> On 9/14/22 12:35, Thomas Monjalon wrote:
>>> 06/09/2022 14:51, Tom Rix:
>>>> On 9/1/22 1:34 PM, Chautru, Nicolas wrote:
>>>>> From: Tom Rix <trix@redhat.com>
>>>>>> On 8/31/22 6:26 PM, Chautru, Nicolas wrote:
>>>>>>> From: Tom Rix <trix@redhat.com>
>>>>>>>> On 8/31/22 3:37 PM, Chautru, Nicolas wrote:
>>>>>>>>>>>>> Comparing ACC200 & ACC100 header files, I
>>>>>>>>>>>>> understand ACC200 is an evolution of the ACC10x
>>>>>>>>>>>>> family. The FEC bits are really close, ACC200 main
>>>>>>>>>>>>> addition seems to be FFT acceleration which could
>>>>>>>>>>>>> be handled in ACC10x driver based on device ID.
>>>>>>>>>>>>>
>>>>>>>>>>>>> I think both drivers have to be merged in order to
>>>>>>>>>>>>> avoid code duplication. That's how other families
>>>>>>>>>>>>> of devices (e.g. i40e) are handled.
>>>>>>>>>>>> I haven't seen your reply on this point.  Do you
>>>>>>>>>>>> confirm you are working on a single driver for ACC
>>>>>>>>>>>> family in order to avoid code duplication?
>>>>>>>>>>>>
>>>>>>>>>>> The implementation is based on distinct ACC100 and
>>>>>>>>>>> ACC200 drivers.  The 2
>>>>>>>>>> devices are fundamentally different generation, processes
>>>>>>>>>> and IP.
>>>>>>>>>>> MountBryce is an eASIC device over PCIe while ACC200 is
>>>>>>>>>>> an integrated
>>>>>>>>>> accelerator on Xeon CPU.
>>>>>>>>>>> The actual implementation are not the same, underlying
>>>>>>>>>>> IP are all distinct
>>>>>>>>>> even if many of the descriptor format have similarities.
>>>>>>>>>>> The actual capabilities of the acceleration are
>>>>>>>>>>> different and/or new.  The workaround and silicon
>>>>>>>>>>> errata are also different causing different
>>>>>>>>>> limitation and implementation in the driver (see the
>>>>>>>>>> serie with ongoing changes for ACC100 in parallel).
>>>>>>>>>>> This is fundamentally distinct from ACC101 which was a
>>>>>>>>>>> derivative product
>>>>>>>>>> from ACC100 and where it made sense to share
>>>>>>>>>> implementation
>>>>>> between
>>>>>>>>>> ACC100 and ACC101.
>>>>>>>>>>> So in a nutshell these 2 devices and drivers are 2
>>>>>>>>>>> different beasts and the
>>>>>>>>>> intention is to keep them intentionally separate as in
>>>>>>>>>> the serie.
>>>>>>>>>>> Let me know if unclear, thanks!
>>>>>>>>>> Nic,
>>>>>>>>>>
>>>>>>>>>> I used a similarity checker to compare acc100 and acc200
>>>>>>>>>>
>>>>>>>>>> https://dickgrune.com/Programs/similarity_tester/
>>>>>>>>>>
>>>>>>>>>> l=simum.log if [ -f $l ]; then rm $l fi
>>>>>>>>>>
>>>>>>>>>> sim_c -s -R -o$l -R -p -P -a .
>>>>>>>>>>
>>>>>>>>>> There results are
>>>>>>>>>>
>>>>>>>>>> ./acc200/acc200_pf_enum.h consists for 100 % of
>>>>>>>>>> ./acc100/acc100_pf_enum.h material
>>>>>>>>>> ./acc100/acc100_pf_enum.h consists for 98 % of
>>>>>>>>>> ./acc200/acc200_pf_enum.h material
>>>>>>>>>> ./acc100/rte_acc100_pmd.h consists for 98 % of
>>>>>>>>>> ./acc200/acc200_pmd.h material ./acc200/acc200_vf_enum.h
>>>>>>>>>> consists for 95 % of ./acc100/acc100_pf_enum.h material
>>>>>>>>>> ./acc200/acc200_pmd.h consists for 92 % of
>>>>>>>>>> ./acc100/rte_acc100_pmd.h material
>>>>>>>>>> ./acc200/rte_acc200_cfg.h consists for 92 % of
>>>>>>>>>> ./acc100/rte_acc100_cfg.h material
>>>>>>>>>> ./acc100/rte_acc100_pmd.c consists for 87 % of
>>>>>>>>>> ./acc200/rte_acc200_pmd.c material
>>>>>>>>>> ./acc100/acc100_vf_enum.h consists for 80 % of
>>>>>>>>>> ./acc200/acc200_pf_enum.h material
>>>>>>>>>> ./acc200/rte_acc200_pmd.c consists for 78 % of
>>>>>>>>>> ./acc100/rte_acc100_pmd.c material
>>>>>>>>>> ./acc100/rte_acc100_cfg.h consists for 75 % of
>>>>>>>>>> ./acc200/rte_acc200_cfg.h material
>>>>>>>>>>
>>>>>>>>>> Spot checking the first *pf_enum.h at 100%, these are the
>>>>>>>>>> devices' registers, they are the same.
>>>>>>>>>>
>>>>>>>>>> I raised this similarity issue with 100 vs 101.
>>>>>>>>>>
>>>>>>>>>> Having multiple copies is difficult to support and should
>>>>>>>>>> be avoided.
>>>>>>>>>>
>>>>>>>>>> For the end user, they should have to use only one
>>>>>>>>>> driver.
>>>>>>>>>>
>>>>>>>>> There are really different IP and do not have the same
>>>>>>>>> interface (PCIe/DDR vs
>>>>>>>> integrated) and there is big serie of changes which are
>>>>>>>> specific to ACC100 coming in parallel. Any workaround,
>>>>>>>> optimization would be
>>>>>> different.
>>>>>>>>> I agree that for the coming serie of integrated accelerator
>>>>>>>>> we will use a
>>>>>>>> unified driver approach but for that very case that would be
>>>>>>>> quite messy to artificially put them within the same PMD.
>>>>>>>>
>>>>>>>> How is the IP different when 100% of the registers are the
>>>>>>>> same ?
>>>>>>>>
>>>>>>> These are 2 different HW aspects. The base toplevel
>>>>>>> configuration registers
>>>>>> are kept similar on purpose but the underlying IP are totally
>>>>>> different design and implementation.
>>>>>>> Even the registers have differences but not visible here, the
>>>>>>> actual RDL file
>>>>>> would define more specifically these registers bitfields and
>>>>>> implementation including which ones are not implemented (but that
>>>>>> is proprietary information), and at bbdev level the interface is
>>>>>> not some much register based than processing based on data from
>>>>>> DMA.
>>>>>>> Basically even if there was a common driver, all these would be
>>>>>>> duplicated
>>>>>> and they are indeed different IP (including different vendors)..
>>>>>>> But I agree with the general intent and to have a common driver
>>>>>>> for the
>>>>>> integrated driver serie (ACC200, ACC300...) now that we are
>>>>>> moving away from PCIe/DDR lookaside acceleration and eASIC/FPGA
>>>>>> implementation (ACC100/AC101).
>>>>>>
>>>>>> Looking a little deeper, at how the driver is lays out some of
>>>>>> its bitfields and private data by reviewing the
>>>>>>
>>>>>> ./acc200/acc200_pmd.h consists for 92 % of
>>>>>> ./acc100/rte_acc100_pmd.h
>>>>>>
>>>>>> There are some minor changes to existing reserved bitfields.  A
>>>>>> new structure for fft.  The acc200_device, the private data for
>>>>>> the driver, is an exact copy of acc100_device.
>>>>>>
>>>>>> acc200_pmd.h is the superset and could be used with little
>>>>>> changes as a common acc_pmd.h.  acc200 is doing everything the
>>>>>> acc100 did in a very similar if not exact way, adding the fft
>>>>>> feature.
>>>>>>
>>>>>> Can you point to some portion of this patchset that is so unique
>>>>>> that it could not be abstracted to an if-check or function and so
>>>>>> requiring this separate, nearly identical driver ?
>>>>>>
>>>>> You used a similarity checker really, there are actually way more
>>>>> relevent differences than what you imply here.  With regards to the
>>>>> 2 pf_enum.h file, there are many registers that have same or
>>>>> similar names but have now different values being mapped hence you
>>>>> just cannot use one for the other.  Saying that
>>>>> "./acc200/acc200_pmd.h consists for 92 % of
>>>>> ./acc100/rte_acc100_pmd.h" is just not correct and really
>>>>> irrelevant.  Just do a diff side by side please and check, that
>>>>> should be extremely obvious, that metrics tells more about the
>>>>> similarity checker limitation than anything else.  Even when using
>>>>> a common driver for ACC200/300 they will have distinct register
>>>>> enum files being auto-generated and coming from distinct RDL.
>>>>> Again just do a diff of these 2 files. I believe you will agree
>>>>> that is not relevant for these files to try to artificially merged
>>>>> these together.
>>>>>
>>>>> With regards to the pmd.h, some structure/defines are indeed common
>>>>> and could be moved to a common file (for instance turboencoder and
>>>>> LDPC encoder which are more vanilla and unlikely to change for
>>>>> future product unlike the decoders which have different feature set
>>>>> and behaviour; or some 3GPP constant that can be defined once).  We
>>>>> can definitely change these to put together shared
>>>>> structures/defines, but not intending to try to artificially put
>>>>> things together with spaghetti code.  We would like to keep 3
>>>>> parallel versions of these PMD for 3 different product lines which
>>>>> are indeed fundamentally different designs (including different
>>>>> workaround required as can be seen on the parallel ACC100 serie
>>>>> under review).  - one version for FPGA implementation (support for
>>>>> N3000, N6000, ...) - one version for eASIC lookaside card
>>>>> implementation (ACC100, ACC101, ...) - one version for the
>>>>> integrated Xeon accelerators (ACC200, ACC300, ...)
>>>>
>>>> Some suggestions on refactoring,
>>>>
>>>> For the registers, have a common file.
>>>>
>>>> For the shared functionality, ex/ ldpc encoder, break these out to
>>>> its own shared file.
>>>>
>>>> The public interface, see my earlier comments on the documentation,
>>>> should be have the same interfaces and the few differences
>>>> highlighted.
>>>
>>> +1 to have common files, and all in a single directory
>>> drivers/baseband/acc100/
>>
>> Jus to be sure we are aligned, do you mean to have both drivers in the
>> same directory, which will share some common files? That's the way I
>> would go.
>>
> 
> I think the expectation is that the two drivers will diverge in future, so
> having separate directories should be ok, even with common files placed in
> one directory are shared with another. With meson include paths its pretty
> trivial to manage if it's just header files, and even if there are common C
> files, there is always the option of using drivers/common if we want to
> split them out. As I understand it, right now it's only headers inluding
> functions which can be static inline, so simple sharing via include paths
> should work fine.

Ok, then I prefer having the common parts in drivers/common/acc, in
order to make it clear changes to these common files have impact on
other drivers than ACC100.

Is that good for you?

Thanks,
Maxime

> /Bruce
>

Akhil Goyal Sept. 14, 2022, 1:44 p.m. UTC | #22

> >
> > On 9/14/22 12:35, Thomas Monjalon wrote:
> > > 06/09/2022 14:51, Tom Rix:
> > > > On 9/1/22 1:34 PM, Chautru, Nicolas wrote:
> > > > > From: Tom Rix <trix@redhat.com>
> > > > > > On 8/31/22 6:26 PM, Chautru, Nicolas wrote:
> > > > > > > From: Tom Rix <trix@redhat.com>
> > > > > > > > On 8/31/22 3:37 PM, Chautru, Nicolas wrote:
> > > > > > > > > > > > > Comparing ACC200 & ACC100 header files, I
> > > > > > > > > > > > > understand ACC200 is an evolution of the ACC10x
> > > > > > > > > > > > > family. The FEC bits are really close, ACC200 main
> > > > > > > > > > > > > addition seems to be FFT acceleration which could
> > > > > > > > > > > > > be handled in ACC10x driver based on device ID.
> > > > > > > > > > > > >
> > > > > > > > > > > > > I think both drivers have to be merged in order to
> > > > > > > > > > > > > avoid code duplication. That's how other families
> > > > > > > > > > > > > of devices (e.g. i40e) are handled.
> > > > > > > > > > > > I haven't seen your reply on this point.  Do you
> > > > > > > > > > > > confirm you are working on a single driver for ACC
> > > > > > > > > > > > family in order to avoid code duplication?
> > > > > > > > > > > >
> > > > > > > > > > > The implementation is based on distinct ACC100 and
> > > > > > > > > > > ACC200 drivers.  The 2
> > > > > > > > > > devices are fundamentally different generation, processes
> > > > > > > > > > and IP.
> > > > > > > > > > > MountBryce is an eASIC device over PCIe while ACC200 is
> > > > > > > > > > > an integrated
> > > > > > > > > > accelerator on Xeon CPU.
> > > > > > > > > > > The actual implementation are not the same, underlying
> > > > > > > > > > > IP are all distinct
> > > > > > > > > > even if many of the descriptor format have similarities.
> > > > > > > > > > > The actual capabilities of the acceleration are
> > > > > > > > > > > different and/or new.  The workaround and silicon
> > > > > > > > > > > errata are also different causing different
> > > > > > > > > > limitation and implementation in the driver (see the
> > > > > > > > > > serie with ongoing changes for ACC100 in parallel).
> > > > > > > > > > > This is fundamentally distinct from ACC101 which was a
> > > > > > > > > > > derivative product
> > > > > > > > > > from ACC100 and where it made sense to share
> > > > > > > > > > implementation
> > > > > > between
> > > > > > > > > > ACC100 and ACC101.
> > > > > > > > > > > So in a nutshell these 2 devices and drivers are 2
> > > > > > > > > > > different beasts and the
> > > > > > > > > > intention is to keep them intentionally separate as in
> > > > > > > > > > the serie.
> > > > > > > > > > > Let me know if unclear, thanks!
> > > > > > > > > > Nic,
> > > > > > > > > >
> > > > > > > > > > I used a similarity checker to compare acc100 and acc200
> > > > > > > > > >
> > > > > > > > > > https://urldefense.proofpoint.com/v2/url?u=https-
> 3A__dickgrune.com_Programs_similarity-
> 5Ftester_&d=DwIBAg&c=nKjWec2b6R0mOyPaz7xtfQ&r=DnL7Si2wl_PRwpZ9TW
> ey3eu68gBzn7DkPwuqhd6WNyo&m=m846AfMSeaddoC0hcxp1mHU4LV3jRUpw
> P-Ie_41buNb9nACOR5La8n8LEFBCnn4t&s=9dgMzk9UMFLA-
> b1EJbi4lK3GG6mNMgOXZRyDZqe00TU&e=
> > > > > > > > > >
> > > > > > > > > > l=simum.log if [ -f $l ]; then rm $l fi
> > > > > > > > > >
> > > > > > > > > > sim_c -s -R -o$l -R -p -P -a .
> > > > > > > > > >
> > > > > > > > > > There results are
> > > > > > > > > >
> > > > > > > > > > ./acc200/acc200_pf_enum.h consists for 100 % of
> > > > > > > > > > ./acc100/acc100_pf_enum.h material
> > > > > > > > > > ./acc100/acc100_pf_enum.h consists for 98 % of
> > > > > > > > > > ./acc200/acc200_pf_enum.h material
> > > > > > > > > > ./acc100/rte_acc100_pmd.h consists for 98 % of
> > > > > > > > > > ./acc200/acc200_pmd.h material ./acc200/acc200_vf_enum.h
> > > > > > > > > > consists for 95 % of ./acc100/acc100_pf_enum.h material
> > > > > > > > > > ./acc200/acc200_pmd.h consists for 92 % of
> > > > > > > > > > ./acc100/rte_acc100_pmd.h material
> > > > > > > > > > ./acc200/rte_acc200_cfg.h consists for 92 % of
> > > > > > > > > > ./acc100/rte_acc100_cfg.h material
> > > > > > > > > > ./acc100/rte_acc100_pmd.c consists for 87 % of
> > > > > > > > > > ./acc200/rte_acc200_pmd.c material
> > > > > > > > > > ./acc100/acc100_vf_enum.h consists for 80 % of
> > > > > > > > > > ./acc200/acc200_pf_enum.h material
> > > > > > > > > > ./acc200/rte_acc200_pmd.c consists for 78 % of
> > > > > > > > > > ./acc100/rte_acc100_pmd.c material
> > > > > > > > > > ./acc100/rte_acc100_cfg.h consists for 75 % of
> > > > > > > > > > ./acc200/rte_acc200_cfg.h material
> > > > > > > > > >
> > > > > > > > > > Spot checking the first *pf_enum.h at 100%, these are the
> > > > > > > > > > devices' registers, they are the same.
> > > > > > > > > >
> > > > > > > > > > I raised this similarity issue with 100 vs 101.
> > > > > > > > > >
> > > > > > > > > > Having multiple copies is difficult to support and should
> > > > > > > > > > be avoided.
> > > > > > > > > >
> > > > > > > > > > For the end user, they should have to use only one
> > > > > > > > > > driver.
> > > > > > > > > >
> > > > > > > > > There are really different IP and do not have the same
> > > > > > > > > interface (PCIe/DDR vs
> > > > > > > > integrated) and there is big serie of changes which are
> > > > > > > > specific to ACC100 coming in parallel. Any workaround,
> > > > > > > > optimization would be
> > > > > > different.
> > > > > > > > > I agree that for the coming serie of integrated accelerator
> > > > > > > > > we will use a
> > > > > > > > unified driver approach but for that very case that would be
> > > > > > > > quite messy to artificially put them within the same PMD.
> > > > > > > >
> > > > > > > > How is the IP different when 100% of the registers are the
> > > > > > > > same ?
> > > > > > > >
> > > > > > > These are 2 different HW aspects. The base toplevel
> > > > > > > configuration registers
> > > > > > are kept similar on purpose but the underlying IP are totally
> > > > > > different design and implementation.
> > > > > > > Even the registers have differences but not visible here, the
> > > > > > > actual RDL file
> > > > > > would define more specifically these registers bitfields and
> > > > > > implementation including which ones are not implemented (but that
> > > > > > is proprietary information), and at bbdev level the interface is
> > > > > > not some much register based than processing based on data from
> > > > > > DMA.
> > > > > > > Basically even if there was a common driver, all these would be
> > > > > > > duplicated
> > > > > > and they are indeed different IP (including different vendors)..
> > > > > > > But I agree with the general intent and to have a common driver
> > > > > > > for the
> > > > > > integrated driver serie (ACC200, ACC300...) now that we are
> > > > > > moving away from PCIe/DDR lookaside acceleration and eASIC/FPGA
> > > > > > implementation (ACC100/AC101).
> > > > > >
> > > > > > Looking a little deeper, at how the driver is lays out some of
> > > > > > its bitfields and private data by reviewing the
> > > > > >
> > > > > > ./acc200/acc200_pmd.h consists for 92 % of
> > > > > > ./acc100/rte_acc100_pmd.h
> > > > > >
> > > > > > There are some minor changes to existing reserved bitfields.  A
> > > > > > new structure for fft.  The acc200_device, the private data for
> > > > > > the driver, is an exact copy of acc100_device.
> > > > > >
> > > > > > acc200_pmd.h is the superset and could be used with little
> > > > > > changes as a common acc_pmd.h.  acc200 is doing everything the
> > > > > > acc100 did in a very similar if not exact way, adding the fft
> > > > > > feature.
> > > > > >
> > > > > > Can you point to some portion of this patchset that is so unique
> > > > > > that it could not be abstracted to an if-check or function and so
> > > > > > requiring this separate, nearly identical driver ?
> > > > > >
> > > > > You used a similarity checker really, there are actually way more
> > > > > relevent differences than what you imply here.  With regards to the
> > > > > 2 pf_enum.h file, there are many registers that have same or
> > > > > similar names but have now different values being mapped hence you
> > > > > just cannot use one for the other.  Saying that
> > > > > "./acc200/acc200_pmd.h consists for 92 % of
> > > > > ./acc100/rte_acc100_pmd.h" is just not correct and really
> > > > > irrelevant.  Just do a diff side by side please and check, that
> > > > > should be extremely obvious, that metrics tells more about the
> > > > > similarity checker limitation than anything else.  Even when using
> > > > > a common driver for ACC200/300 they will have distinct register
> > > > > enum files being auto-generated and coming from distinct RDL.
> > > > > Again just do a diff of these 2 files. I believe you will agree
> > > > > that is not relevant for these files to try to artificially merged
> > > > > these together.
> > > > >
> > > > > With regards to the pmd.h, some structure/defines are indeed common
> > > > > and could be moved to a common file (for instance turboencoder and
> > > > > LDPC encoder which are more vanilla and unlikely to change for
> > > > > future product unlike the decoders which have different feature set
> > > > > and behaviour; or some 3GPP constant that can be defined once).  We
> > > > > can definitely change these to put together shared
> > > > > structures/defines, but not intending to try to artificially put
> > > > > things together with spaghetti code.  We would like to keep 3
> > > > > parallel versions of these PMD for 3 different product lines which
> > > > > are indeed fundamentally different designs (including different
> > > > > workaround required as can be seen on the parallel ACC100 serie
> > > > > under review).  - one version for FPGA implementation (support for
> > > > > N3000, N6000, ...) - one version for eASIC lookaside card
> > > > > implementation (ACC100, ACC101, ...) - one version for the
> > > > > integrated Xeon accelerators (ACC200, ACC300, ...)
> > > >
> > > > Some suggestions on refactoring,
> > > >
> > > > For the registers, have a common file.
> > > >
> > > > For the shared functionality, ex/ ldpc encoder, break these out to
> > > > its own shared file.
> > > >
> > > > The public interface, see my earlier comments on the documentation,
> > > > should be have the same interfaces and the few differences
> > > > highlighted.
> > >
> > > +1 to have common files, and all in a single directory
> > > drivers/baseband/acc100/
> >
> > Jus to be sure we are aligned, do you mean to have both drivers in the
> > same directory, which will share some common files? That's the way I
> > would go.
> >
> 
> I think the expectation is that the two drivers will diverge in future, so
> having separate directories should be ok, even with common files placed in
> one directory are shared with another. With meson include paths its pretty
> trivial to manage if it's just header files, and even if there are common C
> files, there is always the option of using drivers/common if we want to
> split them out. As I understand it, right now it's only headers inluding
> functions which can be static inline, so simple sharing via include paths
> should work fine.
> 
It can be ok to have 2 separate directories, but 
- is it not possible to have them in same directory say 'acc'  for all affiliated devices.
Similar to other vendors' devices (cnxk, i40e, mlx).
- Can both the devices - acc100 and acc200 coexist? If not, same directory is good enough.
- there can be multiple files or directories in 'acc' which can be named appropriately to
denote the actual device(acc100/200).

Having cross dependency across different drivers of same type looks a kind of hacking the meson.
This was a reason we moved to have a drivers/common/ for some of the drivers.
Also including "../acc100/abc.h" does not look appropriate to me.

IMO, we should not add unnecessary directories when the code is common and can be managed in a single one.

However, technically it is also ok to have 2 separate directories. But, agreeing on this will set a
precedence for future next generation devices from the same vendors. It may be a topic of discussion in techboard.

-Akhil

Thomas Monjalon Sept. 14, 2022, 2:23 p.m. UTC | #23

14/09/2022 15:44, Akhil Goyal:
> > > On 9/14/22 12:35, Thomas Monjalon wrote:
> > > > 06/09/2022 14:51, Tom Rix:
> > > > > On 9/1/22 1:34 PM, Chautru, Nicolas wrote:
> > > > > > From: Tom Rix <trix@redhat.com>
> > > > > > > On 8/31/22 6:26 PM, Chautru, Nicolas wrote:
> > > > > > > > From: Tom Rix <trix@redhat.com>
> > > > > > > > > On 8/31/22 3:37 PM, Chautru, Nicolas wrote:
> > > > > > > > > > > > > > Comparing ACC200 & ACC100 header files, I
> > > > > > > > > > > > > > understand ACC200 is an evolution of the ACC10x
> > > > > > > > > > > > > > family. The FEC bits are really close, ACC200 main
> > > > > > > > > > > > > > addition seems to be FFT acceleration which could
> > > > > > > > > > > > > > be handled in ACC10x driver based on device ID.
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > I think both drivers have to be merged in order to
> > > > > > > > > > > > > > avoid code duplication. That's how other families
> > > > > > > > > > > > > > of devices (e.g. i40e) are handled.
> > > > > > > > > > > > > I haven't seen your reply on this point.  Do you
> > > > > > > > > > > > > confirm you are working on a single driver for ACC
> > > > > > > > > > > > > family in order to avoid code duplication?
> > > > > > > > > > > > >
> > > > > > > > > > > > The implementation is based on distinct ACC100 and
> > > > > > > > > > > > ACC200 drivers.  The 2
> > > > > > > > > > > devices are fundamentally different generation, processes
> > > > > > > > > > > and IP.
> > > > > > > > > > > > MountBryce is an eASIC device over PCIe while ACC200 is
> > > > > > > > > > > > an integrated
> > > > > > > > > > > accelerator on Xeon CPU.
> > > > > > > > > > > > The actual implementation are not the same, underlying
> > > > > > > > > > > > IP are all distinct
> > > > > > > > > > > even if many of the descriptor format have similarities.
> > > > > > > > > > > > The actual capabilities of the acceleration are
> > > > > > > > > > > > different and/or new.  The workaround and silicon
> > > > > > > > > > > > errata are also different causing different
> > > > > > > > > > > limitation and implementation in the driver (see the
> > > > > > > > > > > serie with ongoing changes for ACC100 in parallel).
> > > > > > > > > > > > This is fundamentally distinct from ACC101 which was a
> > > > > > > > > > > > derivative product
> > > > > > > > > > > from ACC100 and where it made sense to share
> > > > > > > > > > > implementation
> > > > > > > between
> > > > > > > > > > > ACC100 and ACC101.
> > > > > > > > > > > > So in a nutshell these 2 devices and drivers are 2
> > > > > > > > > > > > different beasts and the
> > > > > > > > > > > intention is to keep them intentionally separate as in
> > > > > > > > > > > the serie.
> > > > > > > > > > > > Let me know if unclear, thanks!
> > > > > > > > > > > Nic,
> > > > > > > > > > >
> > > > > > > > > > > I used a similarity checker to compare acc100 and acc200
> > > > > > > > > > >
> > > > > > > > > > > https://urldefense.proofpoint.com/v2/url?u=https-
> > 3A__dickgrune.com_Programs_similarity-
> > 5Ftester_&d=DwIBAg&c=nKjWec2b6R0mOyPaz7xtfQ&r=DnL7Si2wl_PRwpZ9TW
> > ey3eu68gBzn7DkPwuqhd6WNyo&m=m846AfMSeaddoC0hcxp1mHU4LV3jRUpw
> > P-Ie_41buNb9nACOR5La8n8LEFBCnn4t&s=9dgMzk9UMFLA-
> > b1EJbi4lK3GG6mNMgOXZRyDZqe00TU&e=
> > > > > > > > > > >
> > > > > > > > > > > l=simum.log if [ -f $l ]; then rm $l fi
> > > > > > > > > > >
> > > > > > > > > > > sim_c -s -R -o$l -R -p -P -a .
> > > > > > > > > > >
> > > > > > > > > > > There results are
> > > > > > > > > > >
> > > > > > > > > > > ./acc200/acc200_pf_enum.h consists for 100 % of
> > > > > > > > > > > ./acc100/acc100_pf_enum.h material
> > > > > > > > > > > ./acc100/acc100_pf_enum.h consists for 98 % of
> > > > > > > > > > > ./acc200/acc200_pf_enum.h material
> > > > > > > > > > > ./acc100/rte_acc100_pmd.h consists for 98 % of
> > > > > > > > > > > ./acc200/acc200_pmd.h material ./acc200/acc200_vf_enum.h
> > > > > > > > > > > consists for 95 % of ./acc100/acc100_pf_enum.h material
> > > > > > > > > > > ./acc200/acc200_pmd.h consists for 92 % of
> > > > > > > > > > > ./acc100/rte_acc100_pmd.h material
> > > > > > > > > > > ./acc200/rte_acc200_cfg.h consists for 92 % of
> > > > > > > > > > > ./acc100/rte_acc100_cfg.h material
> > > > > > > > > > > ./acc100/rte_acc100_pmd.c consists for 87 % of
> > > > > > > > > > > ./acc200/rte_acc200_pmd.c material
> > > > > > > > > > > ./acc100/acc100_vf_enum.h consists for 80 % of
> > > > > > > > > > > ./acc200/acc200_pf_enum.h material
> > > > > > > > > > > ./acc200/rte_acc200_pmd.c consists for 78 % of
> > > > > > > > > > > ./acc100/rte_acc100_pmd.c material
> > > > > > > > > > > ./acc100/rte_acc100_cfg.h consists for 75 % of
> > > > > > > > > > > ./acc200/rte_acc200_cfg.h material
> > > > > > > > > > >
> > > > > > > > > > > Spot checking the first *pf_enum.h at 100%, these are the
> > > > > > > > > > > devices' registers, they are the same.
> > > > > > > > > > >
> > > > > > > > > > > I raised this similarity issue with 100 vs 101.
> > > > > > > > > > >
> > > > > > > > > > > Having multiple copies is difficult to support and should
> > > > > > > > > > > be avoided.
> > > > > > > > > > >
> > > > > > > > > > > For the end user, they should have to use only one
> > > > > > > > > > > driver.
> > > > > > > > > > >
> > > > > > > > > > There are really different IP and do not have the same
> > > > > > > > > > interface (PCIe/DDR vs
> > > > > > > > > integrated) and there is big serie of changes which are
> > > > > > > > > specific to ACC100 coming in parallel. Any workaround,
> > > > > > > > > optimization would be
> > > > > > > different.
> > > > > > > > > > I agree that for the coming serie of integrated accelerator
> > > > > > > > > > we will use a
> > > > > > > > > unified driver approach but for that very case that would be
> > > > > > > > > quite messy to artificially put them within the same PMD.
> > > > > > > > >
> > > > > > > > > How is the IP different when 100% of the registers are the
> > > > > > > > > same ?
> > > > > > > > >
> > > > > > > > These are 2 different HW aspects. The base toplevel
> > > > > > > > configuration registers
> > > > > > > are kept similar on purpose but the underlying IP are totally
> > > > > > > different design and implementation.
> > > > > > > > Even the registers have differences but not visible here, the
> > > > > > > > actual RDL file
> > > > > > > would define more specifically these registers bitfields and
> > > > > > > implementation including which ones are not implemented (but that
> > > > > > > is proprietary information), and at bbdev level the interface is
> > > > > > > not some much register based than processing based on data from
> > > > > > > DMA.
> > > > > > > > Basically even if there was a common driver, all these would be
> > > > > > > > duplicated
> > > > > > > and they are indeed different IP (including different vendors)..
> > > > > > > > But I agree with the general intent and to have a common driver
> > > > > > > > for the
> > > > > > > integrated driver serie (ACC200, ACC300...) now that we are
> > > > > > > moving away from PCIe/DDR lookaside acceleration and eASIC/FPGA
> > > > > > > implementation (ACC100/AC101).
> > > > > > >
> > > > > > > Looking a little deeper, at how the driver is lays out some of
> > > > > > > its bitfields and private data by reviewing the
> > > > > > >
> > > > > > > ./acc200/acc200_pmd.h consists for 92 % of
> > > > > > > ./acc100/rte_acc100_pmd.h
> > > > > > >
> > > > > > > There are some minor changes to existing reserved bitfields.  A
> > > > > > > new structure for fft.  The acc200_device, the private data for
> > > > > > > the driver, is an exact copy of acc100_device.
> > > > > > >
> > > > > > > acc200_pmd.h is the superset and could be used with little
> > > > > > > changes as a common acc_pmd.h.  acc200 is doing everything the
> > > > > > > acc100 did in a very similar if not exact way, adding the fft
> > > > > > > feature.
> > > > > > >
> > > > > > > Can you point to some portion of this patchset that is so unique
> > > > > > > that it could not be abstracted to an if-check or function and so
> > > > > > > requiring this separate, nearly identical driver ?
> > > > > > >
> > > > > > You used a similarity checker really, there are actually way more
> > > > > > relevent differences than what you imply here.  With regards to the
> > > > > > 2 pf_enum.h file, there are many registers that have same or
> > > > > > similar names but have now different values being mapped hence you
> > > > > > just cannot use one for the other.  Saying that
> > > > > > "./acc200/acc200_pmd.h consists for 92 % of
> > > > > > ./acc100/rte_acc100_pmd.h" is just not correct and really
> > > > > > irrelevant.  Just do a diff side by side please and check, that
> > > > > > should be extremely obvious, that metrics tells more about the
> > > > > > similarity checker limitation than anything else.  Even when using
> > > > > > a common driver for ACC200/300 they will have distinct register
> > > > > > enum files being auto-generated and coming from distinct RDL.
> > > > > > Again just do a diff of these 2 files. I believe you will agree
> > > > > > that is not relevant for these files to try to artificially merged
> > > > > > these together.
> > > > > >
> > > > > > With regards to the pmd.h, some structure/defines are indeed common
> > > > > > and could be moved to a common file (for instance turboencoder and
> > > > > > LDPC encoder which are more vanilla and unlikely to change for
> > > > > > future product unlike the decoders which have different feature set
> > > > > > and behaviour; or some 3GPP constant that can be defined once).  We
> > > > > > can definitely change these to put together shared
> > > > > > structures/defines, but not intending to try to artificially put
> > > > > > things together with spaghetti code.  We would like to keep 3
> > > > > > parallel versions of these PMD for 3 different product lines which
> > > > > > are indeed fundamentally different designs (including different
> > > > > > workaround required as can be seen on the parallel ACC100 serie
> > > > > > under review).  - one version for FPGA implementation (support for
> > > > > > N3000, N6000, ...) - one version for eASIC lookaside card
> > > > > > implementation (ACC100, ACC101, ...) - one version for the
> > > > > > integrated Xeon accelerators (ACC200, ACC300, ...)
> > > > >
> > > > > Some suggestions on refactoring,
> > > > >
> > > > > For the registers, have a common file.
> > > > >
> > > > > For the shared functionality, ex/ ldpc encoder, break these out to
> > > > > its own shared file.
> > > > >
> > > > > The public interface, see my earlier comments on the documentation,
> > > > > should be have the same interfaces and the few differences
> > > > > highlighted.
> > > >
> > > > +1 to have common files, and all in a single directory
> > > > drivers/baseband/acc100/
> > >
> > > Jus to be sure we are aligned, do you mean to have both drivers in the
> > > same directory, which will share some common files? That's the way I
> > > would go.
> > >
> > 
> > I think the expectation is that the two drivers will diverge in future, so
> > having separate directories should be ok, even with common files placed in
> > one directory are shared with another. With meson include paths its pretty
> > trivial to manage if it's just header files, and even if there are common C
> > files, there is always the option of using drivers/common if we want to
> > split them out. As I understand it, right now it's only headers inluding
> > functions which can be static inline, so simple sharing via include paths
> > should work fine.
> > 
> It can be ok to have 2 separate directories, but 
> - is it not possible to have them in same directory say 'acc'  for all affiliated devices.
> Similar to other vendors' devices (cnxk, i40e, mlx).
> - Can both the devices - acc100 and acc200 coexist? If not, same directory is good enough.
> - there can be multiple files or directories in 'acc' which can be named appropriately to
> denote the actual device(acc100/200).
> 
> Having cross dependency across different drivers of same type looks a kind of hacking the meson.
> This was a reason we moved to have a drivers/common/ for some of the drivers.
> Also including "../acc100/abc.h" does not look appropriate to me.
> 
> IMO, we should not add unnecessary directories when the code is common and can be managed in a single one.
> 
> However, technically it is also ok to have 2 separate directories. But, agreeing on this will set a
> precedence for future next generation devices from the same vendors. It may be a topic of discussion in techboard.

Let me be frank, I don't trust Intel saying the hardware will be too much different in future.
For mlx5, we manage to handle very different devices (like DPU and changing processors)
in a single driver.
So I agree with Maxime and Akhil that a single driver in a single directory should be enough.
Having different registers in different devices is not enough to split.

The worst case would be to have a common directory acc/
but it may be a bit disappointing.

Chautru, Nicolas Sept. 14, 2022, 7:57 p.m. UTC | #24

Hi Thomas, Akhil, Bruce, Maxime, 

> -----Original Message-----
> From: Thomas Monjalon <thomas@monjalon.net>
> Sent: Wednesday, September 14, 2022 7:23 AM
> To: Richardson, Bruce <bruce.richardson@intel.com>; Maxime Coquelin
> <maxime.coquelin@redhat.com>; Akhil Goyal <gakhil@marvell.com>;
> Chautru, Nicolas <nicolas.chautru@intel.com>
> Cc: dev@dpdk.org; hemant.agrawal@nxp.com; Vargas, Hernan
> <hernan.vargas@intel.com>; Tom Rix <trix@redhat.com>; mdr@ashroe.eu;
> david.marchand@redhat.com; stephen@networkplumber.org
> Subject: Re: [EXT] Re: [PATCH v1 00/10] baseband/acc200
> 
> 14/09/2022 15:44, Akhil Goyal:
> > > > On 9/14/22 12:35, Thomas Monjalon wrote:
> > > > > 06/09/2022 14:51, Tom Rix:
> > > > > > On 9/1/22 1:34 PM, Chautru, Nicolas wrote:
> > > > > > > From: Tom Rix <trix@redhat.com>
> > > > > > > > On 8/31/22 6:26 PM, Chautru, Nicolas wrote:
> > > > > > > > > From: Tom Rix <trix@redhat.com>
> > > > > > > > > > On 8/31/22 3:37 PM, Chautru, Nicolas wrote:
<snip>
> > > > > > >
> > > > > > > With regards to the pmd.h, some structure/defines are indeed
> > > > > > > common and could be moved to a common file (for instance
> > > > > > > turboencoder and LDPC encoder which are more vanilla and
> > > > > > > unlikely to change for future product unlike the decoders
> > > > > > > which have different feature set and behaviour; or some 3GPP
> > > > > > > constant that can be defined once).  We can definitely
> > > > > > > change these to put together shared structures/defines, but
> > > > > > > not intending to try to artificially put things together
> > > > > > > with spaghetti code.  We would like to keep 3 parallel
> > > > > > > versions of these PMD for 3 different product lines which
> > > > > > > are indeed fundamentally different designs (including
> > > > > > > different workaround required as can be seen on the parallel
> > > > > > > ACC100 serie under review).  - one version for FPGA
> > > > > > > implementation (support for N3000, N6000, ...) - one version
> > > > > > > for eASIC lookaside card implementation (ACC100, ACC101,
> > > > > > > ...) - one version for the integrated Xeon accelerators
> > > > > > > (ACC200, ACC300, ...)
> > > > > >
> > > > > > Some suggestions on refactoring,
> > > > > >
> > > > > > For the registers, have a common file.
> > > > > >
> > > > > > For the shared functionality, ex/ ldpc encoder, break these
> > > > > > out to its own shared file.
> > > > > >
> > > > > > The public interface, see my earlier comments on the
> > > > > > documentation, should be have the same interfaces and the few
> > > > > > differences highlighted.
> > > > >
> > > > > +1 to have common files, and all in a single directory
> > > > > drivers/baseband/acc100/
> > > >
> > > > Jus to be sure we are aligned, do you mean to have both drivers in
> > > > the same directory, which will share some common files? That's the
> > > > way I would go.
> > > >
> > >
> > > I think the expectation is that the two drivers will diverge in
> > > future, so having separate directories should be ok, even with
> > > common files placed in one directory are shared with another. With
> > > meson include paths its pretty trivial to manage if it's just header
> > > files, and even if there are common C files, there is always the
> > > option of using drivers/common if we want to split them out. As I
> > > understand it, right now it's only headers inluding functions which
> > > can be static inline, so simple sharing via include paths should work fine.
> > >
> > It can be ok to have 2 separate directories, but
> > - is it not possible to have them in same directory say 'acc'  for all affiliated
> devices.
> > Similar to other vendors' devices (cnxk, i40e, mlx).
> > - Can both the devices - acc100 and acc200 coexist? If not, same directory
> is good enough.
> > - there can be multiple files or directories in 'acc' which can be
> > named appropriately to denote the actual device(acc100/200).
> >
> > Having cross dependency across different drivers of same type looks a kind
> of hacking the meson.
> > This was a reason we moved to have a drivers/common/ for some of the
> drivers.
> > Also including "../acc100/abc.h" does not look appropriate to me.
> >
> > IMO, we should not add unnecessary directories when the code is common
> and can be managed in a single one.
> >
> > However, technically it is also ok to have 2 separate directories.
> > But, agreeing on this will set a precedence for future next generation
> devices from the same vendors. It may be a topic of discussion in techboard.
> 
> Let me be frank, I don't trust Intel saying the hardware will be too much
> different in future.

Thanks for the review and discussion. 

Let me clarify, this PMD segregation is specific to ACC1xx vs ACC2xxx. There is a clear intent to have a common PMD to encompass the future multiple integrated solutions VRAN accelerators on Xeon  (based on ACC200 and future Xeon products in roadmap) but not for ACC1xx.
Here we are splitting the  ACC1xx and the ACC2xx series (eASIC process with off-die PCIe device with on-card DDR vs a straight integrated Xeon accelerator) which are fundamentally different devices, and notably the ACC100 requiring a lot of SW workaround/mitigations/protections in the code which would not apply moving forward and would clutter the next generations which would be managed and optimized largely independently. Basically these are not just a few registers differences truly. 
Again future integrated Xeon will shared common driver but always distinct from ACC1xx (only sharing some common code and structure when possible). 
Here the refactoring effort was to gather all reusable code and structure together; which was useful indeed as there are several common functionalities and structures which could be superseded to be shared relatively seamlessly. 

> For mlx5, we manage to handle very different devices (like DPU and changing
> processors) in a single driver.
> So I agree with Maxime and Akhil that a single driver in a single directory
> should be enough.
> Having different registers in different devices is not enough to split.
> 
> The worst case would be to have a common directory acc/ but it may be a bit
> disappointing.
> 

I believe that I hear 2 different options compatible with the 2 PMDs approach:  
- The one suggested by Akhil and Maxime I think, is to put both ACC100 and ACC200 PMDs under ./baseband/acc/ similarly to what is done for cnxk for instance. In that case the common files are still all in same directory as the 2 PMDs so we don't have do the awkard "includes += include_directories('../acc100')" in meson which was frown upon, since everything in already under /drivers/baseband/acc. 
- other option suggested by Thomas to put the shared code and structures under ./drivers/common/acc instead of being under ./drivers/acc/acc_common.h which also used for many drivers. 

My preference may probably be personally for the former option at the moment, but happy to get some form of consensus on this. 

Thanks and regards, 
Nic

Maxime Coquelin Sept. 14, 2022, 8:08 p.m. UTC | #25

On 9/14/22 21:57, Chautru, Nicolas wrote:
> Hi Thomas, Akhil, Bruce, Maxime,
> 
>> -----Original Message-----
>> From: Thomas Monjalon <thomas@monjalon.net>
>> Sent: Wednesday, September 14, 2022 7:23 AM
>> To: Richardson, Bruce <bruce.richardson@intel.com>; Maxime Coquelin
>> <maxime.coquelin@redhat.com>; Akhil Goyal <gakhil@marvell.com>;
>> Chautru, Nicolas <nicolas.chautru@intel.com>
>> Cc: dev@dpdk.org; hemant.agrawal@nxp.com; Vargas, Hernan
>> <hernan.vargas@intel.com>; Tom Rix <trix@redhat.com>; mdr@ashroe.eu;
>> david.marchand@redhat.com; stephen@networkplumber.org
>> Subject: Re: [EXT] Re: [PATCH v1 00/10] baseband/acc200
>>
>> 14/09/2022 15:44, Akhil Goyal:
>>>>> On 9/14/22 12:35, Thomas Monjalon wrote:
>>>>>> 06/09/2022 14:51, Tom Rix:
>>>>>>> On 9/1/22 1:34 PM, Chautru, Nicolas wrote:
>>>>>>>> From: Tom Rix <trix@redhat.com>
>>>>>>>>> On 8/31/22 6:26 PM, Chautru, Nicolas wrote:
>>>>>>>>>> From: Tom Rix <trix@redhat.com>
>>>>>>>>>>> On 8/31/22 3:37 PM, Chautru, Nicolas wrote:
> <snip>
>>>>>>>>
>>>>>>>> With regards to the pmd.h, some structure/defines are indeed
>>>>>>>> common and could be moved to a common file (for instance
>>>>>>>> turboencoder and LDPC encoder which are more vanilla and
>>>>>>>> unlikely to change for future product unlike the decoders
>>>>>>>> which have different feature set and behaviour; or some 3GPP
>>>>>>>> constant that can be defined once).  We can definitely
>>>>>>>> change these to put together shared structures/defines, but
>>>>>>>> not intending to try to artificially put things together
>>>>>>>> with spaghetti code.  We would like to keep 3 parallel
>>>>>>>> versions of these PMD for 3 different product lines which
>>>>>>>> are indeed fundamentally different designs (including
>>>>>>>> different workaround required as can be seen on the parallel
>>>>>>>> ACC100 serie under review).  - one version for FPGA
>>>>>>>> implementation (support for N3000, N6000, ...) - one version
>>>>>>>> for eASIC lookaside card implementation (ACC100, ACC101,
>>>>>>>> ...) - one version for the integrated Xeon accelerators
>>>>>>>> (ACC200, ACC300, ...)
>>>>>>>
>>>>>>> Some suggestions on refactoring,
>>>>>>>
>>>>>>> For the registers, have a common file.
>>>>>>>
>>>>>>> For the shared functionality, ex/ ldpc encoder, break these
>>>>>>> out to its own shared file.
>>>>>>>
>>>>>>> The public interface, see my earlier comments on the
>>>>>>> documentation, should be have the same interfaces and the few
>>>>>>> differences highlighted.
>>>>>>
>>>>>> +1 to have common files, and all in a single directory
>>>>>> drivers/baseband/acc100/
>>>>>
>>>>> Jus to be sure we are aligned, do you mean to have both drivers in
>>>>> the same directory, which will share some common files? That's the
>>>>> way I would go.
>>>>>
>>>>
>>>> I think the expectation is that the two drivers will diverge in
>>>> future, so having separate directories should be ok, even with
>>>> common files placed in one directory are shared with another. With
>>>> meson include paths its pretty trivial to manage if it's just header
>>>> files, and even if there are common C files, there is always the
>>>> option of using drivers/common if we want to split them out. As I
>>>> understand it, right now it's only headers inluding functions which
>>>> can be static inline, so simple sharing via include paths should work fine.
>>>>
>>> It can be ok to have 2 separate directories, but
>>> - is it not possible to have them in same directory say 'acc'  for all affiliated
>> devices.
>>> Similar to other vendors' devices (cnxk, i40e, mlx).
>>> - Can both the devices - acc100 and acc200 coexist? If not, same directory
>> is good enough.
>>> - there can be multiple files or directories in 'acc' which can be
>>> named appropriately to denote the actual device(acc100/200).
>>>
>>> Having cross dependency across different drivers of same type looks a kind
>> of hacking the meson.
>>> This was a reason we moved to have a drivers/common/ for some of the
>> drivers.
>>> Also including "../acc100/abc.h" does not look appropriate to me.
>>>
>>> IMO, we should not add unnecessary directories when the code is common
>> and can be managed in a single one.
>>>
>>> However, technically it is also ok to have 2 separate directories.
>>> But, agreeing on this will set a precedence for future next generation
>> devices from the same vendors. It may be a topic of discussion in techboard.
>>
>> Let me be frank, I don't trust Intel saying the hardware will be too much
>> different in future.
> 
> Thanks for the review and discussion.
> 
> Let me clarify, this PMD segregation is specific to ACC1xx vs ACC2xxx. There is a clear intent to have a common PMD to encompass the future multiple integrated solutions VRAN accelerators on Xeon  (based on ACC200 and future Xeon products in roadmap) but not for ACC1xx.
> Here we are splitting the  ACC1xx and the ACC2xx series (eASIC process with off-die PCIe device with on-card DDR vs a straight integrated Xeon accelerator) which are fundamentally different devices, and notably the ACC100 requiring a lot of SW workaround/mitigations/protections in the code which would not apply moving forward and would clutter the next generations which would be managed and optimized largely independently. Basically these are not just a few registers differences truly.
> Again future integrated Xeon will shared common driver but always distinct from ACC1xx (only sharing some common code and structure when possible).
> Here the refactoring effort was to gather all reusable code and structure together; which was useful indeed as there are several common functionalities and structures which could be superseded to be shared relatively seamlessly.
> 
>> For mlx5, we manage to handle very different devices (like DPU and changing
>> processors) in a single driver.
>> So I agree with Maxime and Akhil that a single driver in a single directory
>> should be enough.
>> Having different registers in different devices is not enough to split.
>>
>> The worst case would be to have a common directory acc/ but it may be a bit
>> disappointing.
>>
> 
> I believe that I hear 2 different options compatible with the 2 PMDs approach:
> - The one suggested by Akhil and Maxime I think, is to put both ACC100 and ACC200 PMDs under ./baseband/acc/ similarly to what is done for cnxk for instance. In that case the common files are still all in same directory as the 2 PMDs so we don't have do the awkard "includes += include_directories('../acc100')" in meson which was frown upon, since everything in already under /drivers/baseband/acc.
> - other option suggested by Thomas to put the shared code and structures under ./drivers/common/acc instead of being under ./drivers/acc/acc_common.h which also used for many drivers.
> 
> My preference may probably be personally for the former option at the moment, but happy to get some form of consensus on this.

I am fine with former option.
drivers/common is especially useful when code is to be shared by
different types of devices (e.g. net and crypto).

Maxime

> 
> Thanks and regards,
> Nic
>