> -----Original Message-----
> From: Burakov, Anatoly <anatoly.bura...@intel.com>
> Sent: Monday, October 12, 2020 17:45
> To: Wang, Haiyue <haiyue.w...@intel.com>; dev@dpdk.org
> Cc: Ma, Liang J <liang.j...@intel.com>; Guo, Jia <jia....@intel.com>; Hunt,
> David
> <david.h...@intel.com>; Ananyev, Konstantin <konstantin.anan...@intel.com>;
> jerinjac...@gmail.com;
> Richardson, Bruce <bruce.richard...@intel.com>; tho...@monjalon.net;
> McDaniel, Timothy
> <timothy.mcdan...@intel.com>; Eads, Gage <gage.e...@intel.com>; Macnamara,
> Chris
> <chris.macnam...@intel.com>
> Subject: Re: [PATCH v5 06/10] net/ixgbe: implement power management API
>
> On 12-Oct-20 8:46 AM, Wang, Haiyue wrote:
> > Hi Liang,
> >
> >> -----Original Message-----
> >> From: Burakov, Anatoly <anatoly.bura...@intel.com>
> >> Sent: Saturday, October 10, 2020 00:02
> >> To: dev@dpdk.org
> >> Cc: Ma, Liang J <liang.j...@intel.com>; Guo, Jia <jia....@intel.com>;
> >> Wang, Haiyue
> >> <haiyue.w...@intel.com>; Hunt, David <david.h...@intel.com>; Ananyev,
> >> Konstantin
> >> <konstantin.anan...@intel.com>; jerinjac...@gmail.com; Richardson, Bruce
> <bruce.richard...@intel.com>;
> >> tho...@monjalon.net; McDaniel, Timothy <timothy.mcdan...@intel.com>; Eads,
> >> Gage
> <gage.e...@intel.com>;
> >> Macnamara, Chris <chris.macnam...@intel.com>
> >> Subject: [PATCH v5 06/10] net/ixgbe: implement power management API
> >>
> >> From: Liang Ma <liang.j...@intel.com>
> >>
> >> Implement support for the power management API by implementing a
> >> `get_wake_addr` function that will return an address of an RX ring's
> >> status bit.
> >>
> >> Signed-off-by: Anatoly Burakov <anatoly.bura...@intel.com>
> >> Signed-off-by: Liang Ma <liang.j...@intel.com>
> >> ---
> >> drivers/net/ixgbe/ixgbe_ethdev.c | 1 +
> >> drivers/net/ixgbe/ixgbe_rxtx.c | 22 ++++++++++++++++++++++
> >> drivers/net/ixgbe/ixgbe_rxtx.h | 2 ++
> >> 3 files changed, 25 insertions(+)
> >>
> >> diff --git a/drivers/net/ixgbe/ixgbe_ethdev.c
> >> b/drivers/net/ixgbe/ixgbe_ethdev.c
> >> index 0b98e210e7..30b3f416d4 100644
> >> --- a/drivers/net/ixgbe/ixgbe_ethdev.c
> >> +++ b/drivers/net/ixgbe/ixgbe_ethdev.c
> >> @@ -588,6 +588,7 @@ static const struct eth_dev_ops ixgbe_eth_dev_ops = {
> >> .udp_tunnel_port_del = ixgbe_dev_udp_tunnel_port_del,
> >> .tm_ops_get = ixgbe_tm_ops_get,
> >> .tx_done_cleanup = ixgbe_dev_tx_done_cleanup,
> >> +.get_wake_addr = ixgbe_get_wake_addr,
> >> };
> >>
> >> /*
> >> diff --git a/drivers/net/ixgbe/ixgbe_rxtx.c
> >> b/drivers/net/ixgbe/ixgbe_rxtx.c
> >> index 977ecf5137..7a9fd2aec6 100644
> >> --- a/drivers/net/ixgbe/ixgbe_rxtx.c
> >> +++ b/drivers/net/ixgbe/ixgbe_rxtx.c
> >> @@ -1366,6 +1366,28 @@ const uint32_t
> >> RTE_PTYPE_INNER_L3_IPV4_EXT | RTE_PTYPE_INNER_L4_UDP,
> >> };
> >>
> >> +int ixgbe_get_wake_addr(void *rx_queue, volatile void **tail_desc_addr,
> >> +uint64_t *expected, uint64_t *mask)
> >> +{
> >> +volatile union ixgbe_adv_rx_desc *rxdp;
> >> +struct ixgbe_rx_queue *rxq = rx_queue;
> >> +uint16_t desc;
> >> +
> >> +desc = rxq->rx_tail;
> >> +rxdp = &rxq->rx_ring[desc];
> >> +/* watch for changes in status bit */
> >> +*tail_desc_addr = &rxdp->wb.upper.status_error;
> >> +
> >> +/*
> >> + * we expect the DD bit to be set to 1 if this descriptor was already
> >> + * written to.
> >> + */
> >> +*expected = rte_cpu_to_le_32(IXGBE_RXDADV_STAT_DD);
> >> +*mask = rte_cpu_to_le_32(IXGBE_RXDADV_STAT_DD);
> >> +
> >> +return 0;
> >> +}
> >> +
> >
> > I'm wondering that whether the '.get_wake_addr' can be specific to
> > like 'rxq_tailq_addr_get' ? So that one day this wake up mechanism
> > can be applied to 'txq_tailq_addr_get' ? :-)
>
> What would be the point of sleeping on TX queue though?
I checked, seems that the PMD uses internal index, no address, please ignore
this bad idea. ;-)
>
> >
> > Also, "volatile void **tail_desc_addr, uint64_t *expected, uint64_t *mask"
> > can be merged into 'struct xxx' ? So that you can expand the API easily.
>
> Actually, i don't think we can do that. Well, we can, but we'll have to
> either define a new struct for ethdev, or define it in the power library
> and make ethdev dependent on the power library. The latter is a no-go,
> and the former i don't think is a good idea because adding a new struct
> to ethdev is big deal and i'd like to avoid that if i can.
Understood the design now, thanks!
>
> >
> > Just my thoughts.
> >
> > Anyway, LGTM
> >
> > Acked-by: Haiyue Wang <haiyue.w...@intel.com>
> >
> >> --
> >> 2.17.1
>
>
> --
> Thanks,
> Anatoly
