[v3,02/12] baseband/acc: add FFT window width in the VRB PMD
Checks
Commit Message
This allows to expose the FFT window width being introduced in
previous commit based on what is configured dynamically on the
device platform.
Signed-off-by: Nicolas Chautru <nicolas.chautru@intel.com>
---
drivers/baseband/acc/acc_common.h | 3 +++
drivers/baseband/acc/rte_vrb_pmd.c | 29 +++++++++++++++++++++++++++++
2 files changed, 32 insertions(+)
Comments
On 9/29/23 18:35, Nicolas Chautru wrote:
> This allows to expose the FFT window width being introduced in
> previous commit based on what is configured dynamically on the
> device platform.
>
> Signed-off-by: Nicolas Chautru <nicolas.chautru@intel.com>
> ---
> drivers/baseband/acc/acc_common.h | 3 +++
> drivers/baseband/acc/rte_vrb_pmd.c | 29 +++++++++++++++++++++++++++++
> 2 files changed, 32 insertions(+)
>
> diff --git a/drivers/baseband/acc/acc_common.h b/drivers/baseband/acc/acc_common.h
> index 5bb00746c3..7d24c644c0 100644
> --- a/drivers/baseband/acc/acc_common.h
> +++ b/drivers/baseband/acc/acc_common.h
> @@ -512,6 +512,8 @@ struct acc_deq_intr_details {
> enum {
> ACC_VF2PF_STATUS_REQUEST = 1,
> ACC_VF2PF_USING_VF = 2,
> + ACC_VF2PF_LUT_VER_REQUEST = 3,
> + ACC_VF2PF_FFT_WIN_REQUEST = 4,
> };
>
>
> @@ -558,6 +560,7 @@ struct acc_device {
> queue_offset_fun_t queue_offset; /* Device specific queue offset */
> uint16_t num_qgroups;
> uint16_t num_aqs;
> + uint16_t fft_window_width[RTE_BBDEV_MAX_FFT_WIN]; /* FFT windowing width. */
> };
>
> /* Structure associated with each queue. */
> diff --git a/drivers/baseband/acc/rte_vrb_pmd.c b/drivers/baseband/acc/rte_vrb_pmd.c
> index 9e5a73c9c7..c5a74bae11 100644
> --- a/drivers/baseband/acc/rte_vrb_pmd.c
> +++ b/drivers/baseband/acc/rte_vrb_pmd.c
> @@ -298,6 +298,34 @@ vrb_device_status(struct rte_bbdev *dev)
> return reg;
> }
>
> +/* Request device FFT windowing information. */
> +static inline void
> +vrb_device_fft_win(struct rte_bbdev *dev, struct rte_bbdev_driver_info *dev_info)
> +{
> + struct acc_device *d = dev->data->dev_private;
> + uint32_t reg, time_out = 0, win;
> +
> + if (d->pf_device)
> + return;
> +
> + /* Check from the device the first time. */
> + if (d->fft_window_width[0] == 0) {
O is not a possible value? Might not be a big deal as it would just
perform reading of 16 x 2 registers every time .info_get() is called,
just wondering.
> + for (win = 0; win < RTE_BBDEV_MAX_FFT_WIN; win++) {
> + vrb_vf2pf(d, ACC_VF2PF_FFT_WIN_REQUEST | win);
That looks broken, as extending RTE_BBDEV_MAX_FFT_WIN to support other
devices may break this implementation.
To me, it tends to show how this fft_window_width array is more device
specific than generic.
> + reg = acc_reg_read(d, d->reg_addr->pf2vf_doorbell);
> + while ((time_out < ACC_STATUS_TO) && (reg == RTE_BBDEV_DEV_NOSTATUS)) {
> + usleep(ACC_STATUS_WAIT); /*< Wait or VF->PF->VF Comms */
> + reg = acc_reg_read(d, d->reg_addr->pf2vf_doorbell);
> + time_out++;
> + }
> + d->fft_window_width[win] = reg;
> + }
> + }
> +
> + for (win = 0; win < RTE_BBDEV_MAX_FFT_WIN; win++)
> + dev_info->fft_window_width[win] = d->fft_window_width[win];
> +}
> +
> /* Checks PF Info Ring to find the interrupt cause and handles it accordingly. */
> static inline void
> vrb_check_ir(struct acc_device *acc_dev)
> @@ -1100,6 +1128,7 @@ vrb_dev_info_get(struct rte_bbdev *dev, struct rte_bbdev_driver_info *dev_info)
> fetch_acc_config(dev);
> /* Check the status of device. */
> dev_info->device_status = vrb_device_status(dev);
> + vrb_device_fft_win(dev, dev_info);
>
> /* Exposed number of queues. */
> dev_info->num_queues[RTE_BBDEV_OP_NONE] = 0;
Hi Maxime,
> -----Original Message-----
> From: Maxime Coquelin <maxime.coquelin@redhat.com>
> Sent: Tuesday, October 3, 2023 4:52 AM
> To: Chautru, Nicolas <nicolas.chautru@intel.com>; dev@dpdk.org
> Cc: hemant.agrawal@nxp.com; david.marchand@redhat.com; Vargas, Hernan
> <hernan.vargas@intel.com>
> Subject: Re: [PATCH v3 02/12] baseband/acc: add FFT window width in the
> VRB PMD
>
>
>
> On 9/29/23 18:35, Nicolas Chautru wrote:
> > This allows to expose the FFT window width being introduced in
> > previous commit based on what is configured dynamically on the device
> > platform.
> >
> > Signed-off-by: Nicolas Chautru <nicolas.chautru@intel.com>
> > ---
> > drivers/baseband/acc/acc_common.h | 3 +++
> > drivers/baseband/acc/rte_vrb_pmd.c | 29
> +++++++++++++++++++++++++++++
> > 2 files changed, 32 insertions(+)
> >
> > diff --git a/drivers/baseband/acc/acc_common.h
> > b/drivers/baseband/acc/acc_common.h
> > index 5bb00746c3..7d24c644c0 100644
> > --- a/drivers/baseband/acc/acc_common.h
> > +++ b/drivers/baseband/acc/acc_common.h
> > @@ -512,6 +512,8 @@ struct acc_deq_intr_details {
> > enum {
> > ACC_VF2PF_STATUS_REQUEST = 1,
> > ACC_VF2PF_USING_VF = 2,
> > + ACC_VF2PF_LUT_VER_REQUEST = 3,
> > + ACC_VF2PF_FFT_WIN_REQUEST = 4,
> > };
> >
> >
> > @@ -558,6 +560,7 @@ struct acc_device {
> > queue_offset_fun_t queue_offset; /* Device specific queue offset */
> > uint16_t num_qgroups;
> > uint16_t num_aqs;
> > + uint16_t fft_window_width[RTE_BBDEV_MAX_FFT_WIN]; /* FFT
> windowing
> > +width. */
> > };
> >
> > /* Structure associated with each queue. */ diff --git
> > a/drivers/baseband/acc/rte_vrb_pmd.c
> > b/drivers/baseband/acc/rte_vrb_pmd.c
> > index 9e5a73c9c7..c5a74bae11 100644
> > --- a/drivers/baseband/acc/rte_vrb_pmd.c
> > +++ b/drivers/baseband/acc/rte_vrb_pmd.c
> > @@ -298,6 +298,34 @@ vrb_device_status(struct rte_bbdev *dev)
> > return reg;
> > }
> >
> > +/* Request device FFT windowing information. */ static inline void
> > +vrb_device_fft_win(struct rte_bbdev *dev, struct
> > +rte_bbdev_driver_info *dev_info) {
> > + struct acc_device *d = dev->data->dev_private;
> > + uint32_t reg, time_out = 0, win;
> > +
> > + if (d->pf_device)
> > + return;
> > +
> > + /* Check from the device the first time. */
> > + if (d->fft_window_width[0] == 0) {
>
> O is not a possible value? Might not be a big deal as it would just perform
> reading of 16 x 2 registers every time .info_get() is called, just wondering.
This is impossible for this to be null. It would mean forcing a zero output all the time. Cannot happen fundamentally.
>
> > + for (win = 0; win < RTE_BBDEV_MAX_FFT_WIN; win++) {
> > + vrb_vf2pf(d, ACC_VF2PF_FFT_WIN_REQUEST | win);
>
> That looks broken, as extending RTE_BBDEV_MAX_FFT_WIN to support other
> devices may break this implementation.
I don’t believe so. 16 windows shapes is a fairly large, this already takes a lot of memory to store all this.
>
> To me, it tends to show how this fft_window_width array is more device
> specific than generic.
I don't see why you say this really. This is fundamentally what windowing is. This is a given section of the FFT output where you apply a point-wise multiplication based on a given window shape, hence the size is scaled up and down based on the FFT size.
This width information is required to estimate about much noise to remove by applying such windowing, hence this is enumerated during device enumeration.
Then the number of windows available is a discrete numbers as mentioned above based on how many of these are programmed on the device (these needs to be stored in HW memory).
Would you prefer to expose an additional parameter for the number of windows in the capability (ie. size of that array) then a pointer for the actual array? That is okay with me and probably better. Please kindly confirm.
Also Herman feel free to chime in.
Ie.
{
.type = RTE_BBDEV_OP_FFT,
.cap.fft = {
.capability_flags = (...),
.num_windows = 16,
}
},
>
> > + reg = acc_reg_read(d, d->reg_addr->pf2vf_doorbell);
> > + while ((time_out < ACC_STATUS_TO) && (reg ==
> RTE_BBDEV_DEV_NOSTATUS)) {
> > + usleep(ACC_STATUS_WAIT); /*< Wait or VF-
> >PF->VF Comms */
> > + reg = acc_reg_read(d, d->reg_addr-
> >pf2vf_doorbell);
> > + time_out++;
> > + }
> > + d->fft_window_width[win] = reg;
> > + }
> > + }
> > +
> > + for (win = 0; win < RTE_BBDEV_MAX_FFT_WIN; win++)
> > + dev_info->fft_window_width[win] = d-
> >fft_window_width[win]; }
> > +
> > /* Checks PF Info Ring to find the interrupt cause and handles it
> accordingly. */
> > static inline void
> > vrb_check_ir(struct acc_device *acc_dev) @@ -1100,6 +1128,7 @@
> > vrb_dev_info_get(struct rte_bbdev *dev, struct rte_bbdev_driver_info
> *dev_info)
> > fetch_acc_config(dev);
> > /* Check the status of device. */
> > dev_info->device_status = vrb_device_status(dev);
> > + vrb_device_fft_win(dev, dev_info);
> >
> > /* Exposed number of queues. */
> > dev_info->num_queues[RTE_BBDEV_OP_NONE] = 0;
On 10/3/23 21:06, Chautru, Nicolas wrote:
> Hi Maxime,
>
>> -----Original Message-----
>> From: Maxime Coquelin <maxime.coquelin@redhat.com>
>> Sent: Tuesday, October 3, 2023 4:52 AM
>> To: Chautru, Nicolas <nicolas.chautru@intel.com>; dev@dpdk.org
>> Cc: hemant.agrawal@nxp.com; david.marchand@redhat.com; Vargas, Hernan
>> <hernan.vargas@intel.com>
>> Subject: Re: [PATCH v3 02/12] baseband/acc: add FFT window width in the
>> VRB PMD
>>
>>
>>
>> On 9/29/23 18:35, Nicolas Chautru wrote:
>>> This allows to expose the FFT window width being introduced in
>>> previous commit based on what is configured dynamically on the device
>>> platform.
>>>
>>> Signed-off-by: Nicolas Chautru <nicolas.chautru@intel.com>
>>> ---
>>> drivers/baseband/acc/acc_common.h | 3 +++
>>> drivers/baseband/acc/rte_vrb_pmd.c | 29
>> +++++++++++++++++++++++++++++
>>> 2 files changed, 32 insertions(+)
>>>
>>> diff --git a/drivers/baseband/acc/acc_common.h
>>> b/drivers/baseband/acc/acc_common.h
>>> index 5bb00746c3..7d24c644c0 100644
>>> --- a/drivers/baseband/acc/acc_common.h
>>> +++ b/drivers/baseband/acc/acc_common.h
>>> @@ -512,6 +512,8 @@ struct acc_deq_intr_details {
>>> enum {
>>> ACC_VF2PF_STATUS_REQUEST = 1,
>>> ACC_VF2PF_USING_VF = 2,
>>> + ACC_VF2PF_LUT_VER_REQUEST = 3,
>>> + ACC_VF2PF_FFT_WIN_REQUEST = 4,
>>> };
>>>
>>>
>>> @@ -558,6 +560,7 @@ struct acc_device {
>>> queue_offset_fun_t queue_offset; /* Device specific queue offset */
>>> uint16_t num_qgroups;
>>> uint16_t num_aqs;
>>> + uint16_t fft_window_width[RTE_BBDEV_MAX_FFT_WIN]; /* FFT
>> windowing
>>> +width. */
>>> };
>>>
>>> /* Structure associated with each queue. */ diff --git
>>> a/drivers/baseband/acc/rte_vrb_pmd.c
>>> b/drivers/baseband/acc/rte_vrb_pmd.c
>>> index 9e5a73c9c7..c5a74bae11 100644
>>> --- a/drivers/baseband/acc/rte_vrb_pmd.c
>>> +++ b/drivers/baseband/acc/rte_vrb_pmd.c
>>> @@ -298,6 +298,34 @@ vrb_device_status(struct rte_bbdev *dev)
>>> return reg;
>>> }
>>>
>>> +/* Request device FFT windowing information. */ static inline void
>>> +vrb_device_fft_win(struct rte_bbdev *dev, struct
>>> +rte_bbdev_driver_info *dev_info) {
>>> + struct acc_device *d = dev->data->dev_private;
>>> + uint32_t reg, time_out = 0, win;
>>> +
>>> + if (d->pf_device)
>>> + return;
>>> +
>>> + /* Check from the device the first time. */
>>> + if (d->fft_window_width[0] == 0) {
>>
>> O is not a possible value? Might not be a big deal as it would just perform
>> reading of 16 x 2 registers every time .info_get() is called, just wondering.
>
> This is impossible for this to be null. It would mean forcing a zero output all the time. Cannot happen fundamentally.
Ack.
>
>>
>>> + for (win = 0; win < RTE_BBDEV_MAX_FFT_WIN; win++) {
>>> + vrb_vf2pf(d, ACC_VF2PF_FFT_WIN_REQUEST | win);
>>
>> That looks broken, as extending RTE_BBDEV_MAX_FFT_WIN to support other
>> devices may break this implementation.
>
> I don’t believe so. 16 windows shapes is a fairly large, this already takes a lot of memory to store all this.
Maybe, but the issue here is you rely on some generic BBDEV defines as
an offset to access HW registers in your device, that's wrong IMO as the
define may evolve in the future. At least you should define what is the
maximum FFT windows for your device, a use the minimum value between the
two.
But the suggestion you make below is better
>
>>
>> To me, it tends to show how this fft_window_width array is more device
>> specific than generic.
>
> I don't see why you say this really. This is fundamentally what windowing is. This is a given section of the FFT output where you apply a point-wise multiplication based on a given window shape, hence the size is scaled up and down based on the FFT size.
> This width information is required to estimate about much noise to remove by applying such windowing, hence this is enumerated during device enumeration.
> Then the number of windows available is a discrete numbers as mentioned above based on how many of these are programmed on the device (these needs to be stored in HW memory).
>
> Would you prefer to expose an additional parameter for the number of windows in the capability (ie. size of that array) then a pointer for the actual array? That is okay with me and probably better. Please kindly confirm.
> Also Herman feel free to chime in.
>
> Ie.
> {
> .type = RTE_BBDEV_OP_FFT,
> .cap.fft = {
> .capability_flags = (...),
> .num_windows = 16,
> }
> },
That would be better IMO.
>>
>>> + reg = acc_reg_read(d, d->reg_addr->pf2vf_doorbell);
>>> + while ((time_out < ACC_STATUS_TO) && (reg ==
>> RTE_BBDEV_DEV_NOSTATUS)) {
>>> + usleep(ACC_STATUS_WAIT); /*< Wait or VF-
>>> PF->VF Comms */
>>> + reg = acc_reg_read(d, d->reg_addr-
>>> pf2vf_doorbell);
>>> + time_out++;
>>> + }
>>> + d->fft_window_width[win] = reg;
>>> + }
>>> + }
>>> +
>>> + for (win = 0; win < RTE_BBDEV_MAX_FFT_WIN; win++)
>>> + dev_info->fft_window_width[win] = d-
>>> fft_window_width[win]; }
>>> +
>>> /* Checks PF Info Ring to find the interrupt cause and handles it
>> accordingly. */
>>> static inline void
>>> vrb_check_ir(struct acc_device *acc_dev) @@ -1100,6 +1128,7 @@
>>> vrb_dev_info_get(struct rte_bbdev *dev, struct rte_bbdev_driver_info
>> *dev_info)
>>> fetch_acc_config(dev);
>>> /* Check the status of device. */
>>> dev_info->device_status = vrb_device_status(dev);
>>> + vrb_device_fft_win(dev, dev_info);
>>>
>>> /* Exposed number of queues. */
>>> dev_info->num_queues[RTE_BBDEV_OP_NONE] = 0;
>
@@ -512,6 +512,8 @@ struct acc_deq_intr_details {
enum {
ACC_VF2PF_STATUS_REQUEST = 1,
ACC_VF2PF_USING_VF = 2,
+ ACC_VF2PF_LUT_VER_REQUEST = 3,
+ ACC_VF2PF_FFT_WIN_REQUEST = 4,
};
@@ -558,6 +560,7 @@ struct acc_device {
queue_offset_fun_t queue_offset; /* Device specific queue offset */
uint16_t num_qgroups;
uint16_t num_aqs;
+ uint16_t fft_window_width[RTE_BBDEV_MAX_FFT_WIN]; /* FFT windowing width. */
};
/* Structure associated with each queue. */
@@ -298,6 +298,34 @@ vrb_device_status(struct rte_bbdev *dev)
return reg;
}
+/* Request device FFT windowing information. */
+static inline void
+vrb_device_fft_win(struct rte_bbdev *dev, struct rte_bbdev_driver_info *dev_info)
+{
+ struct acc_device *d = dev->data->dev_private;
+ uint32_t reg, time_out = 0, win;
+
+ if (d->pf_device)
+ return;
+
+ /* Check from the device the first time. */
+ if (d->fft_window_width[0] == 0) {
+ for (win = 0; win < RTE_BBDEV_MAX_FFT_WIN; win++) {
+ vrb_vf2pf(d, ACC_VF2PF_FFT_WIN_REQUEST | win);
+ reg = acc_reg_read(d, d->reg_addr->pf2vf_doorbell);
+ while ((time_out < ACC_STATUS_TO) && (reg == RTE_BBDEV_DEV_NOSTATUS)) {
+ usleep(ACC_STATUS_WAIT); /*< Wait or VF->PF->VF Comms */
+ reg = acc_reg_read(d, d->reg_addr->pf2vf_doorbell);
+ time_out++;
+ }
+ d->fft_window_width[win] = reg;
+ }
+ }
+
+ for (win = 0; win < RTE_BBDEV_MAX_FFT_WIN; win++)
+ dev_info->fft_window_width[win] = d->fft_window_width[win];
+}
+
/* Checks PF Info Ring to find the interrupt cause and handles it accordingly. */
static inline void
vrb_check_ir(struct acc_device *acc_dev)
@@ -1100,6 +1128,7 @@ vrb_dev_info_get(struct rte_bbdev *dev, struct rte_bbdev_driver_info *dev_info)
fetch_acc_config(dev);
/* Check the status of device. */
dev_info->device_status = vrb_device_status(dev);
+ vrb_device_fft_win(dev, dev_info);
/* Exposed number of queues. */
dev_info->num_queues[RTE_BBDEV_OP_NONE] = 0;