Hi Jerin,

thanks for your feedback, further comments below.

On 7/7/2017 9:27 PM, Jerin Jacob wrote:
-----Original Message-----
Date: Fri, 7 Jul 2017 20:33:19 +0530
From: Jerin Jacob <jerin.ja...@caviumnetworks.com>
/* adapter has inbuilt port, no need to create producer port */
#define RTE_EVENT_ETHDEV_CAP_INBUILT_PORT  (1ULL << 0)
/* adapter does not need service function */
#define RTE_EVENT_ETHDEV_CAP_NO_SERVICE_FUNC (1ULL << 1)

I just provided a name to share the view. You can choose better name.

OK.

int rte_event_eth_rx_adapter_create(uint8_t id, uint8_t dev_id, uint8_t
eth_port_id);


I also think that the application should be able to call create() with > 1 ports. This would allow a single service to poll multiple NICs with WRR priority.

The side effect of is that the queue add/del APIs would need to specify the port_id as well.

int rte_event_eth_rx_adapter_get_info(uint8_t id, struct
rte_event_eth_rx_adap_info *info);
int rte_event_eth_rx_adapter_configure(uint8_t id, struct
rte_event_eth_rx_adap_config *cfg);
int rte_event_eth_rx_adapter_queue_add(uint8_t id, int32_t rx_queue_id,
const struct rte_eth_rx_event_adapter_queue_config *config);
int rte_event_eth_rx_adapter_queue_del(uint8_t id, int32_t rx_queue_id)

int rte_event_eth_rx_adapter_run();

The adapter's run function is not part of the public interface, agree ?
int rte_event_eth_rx_adapter_free(uint8_t id);


If I understood your idea, the function pointer struct would look something
like this.

struct rte_eventdev_rx_adapter_ops {
        rx_adapter_create_t     create,
        rx_adapter_get_info_t   info,
        rx_adapter_configure_t  configure,
        rx_adapter_queue_add_t  queue_add,
        rx_adapter_queue_del_t  queue_del,
        rx_adapter_queue_free_t queue_free,
       rx_adapter_free_t        free
};

Yes. But, I think, adapter create and free goes in struct rte_eventdev_op .
See below.



struct rte_eventdev {
        ..
        const struct rte_eventdev_rx_adapter_ops *rx_adapter_ops;
        ..
};

An eventdev instance can have N adapters not just one. So this will be
pointer to pointer, indexed through adapter_id.

In SW PMD case, the eventdev may have only one adapter.
In HW PMD cases, There will more than one. example,
- if octeontx eventdev dev_id + any external PCI n/w card like i40e or
nicvf case an adapter with similar adapter ops as SW PMD
- if octeontx eventdev dev_id + octeontx ethdev dev_id case another
adapter that does not need service cores to inject event to octeontx
eventdev

- Your generic Rx adapter we will make it as common code so that both SW PMD and
octeontx PMD in service core mode as use the functions and register the
ops.

OK.


I was thinking like this

int rte_event_eth_rx_adapter_create(uint8_t id, uint8_t dev_id, uint8_t 
eth_port_id)
{
        eventdev_ops = ... /* access through dev_id */

        /* common code to get ops memory from adapter id */
        struct rte_eventdev_rx_adapter_ops* ops = 
rte_event_pmd_adapter_allocate(id);

        /* Fill in adapter ops from driver */
        eventdev_ops->create_adapter(ops, dev_id, eth_port_id)

As an implementation detail, the function pointer setup needs to account for DPDK primary/secondary processes, i.e, function addresses could be different in the 2 processes.


}

int rte_event_eth_rx_adapter_get_info(uint8_t id, struct 
rte_event_eth_rx_adap_info *info)
{

Argument list is missing the dev_id (as are the configure/queue_add/queue_del) functions.

        eventdev_ops = ... /* access through dev_id */

        struct rte_eventdev_rx_adapter_ops* ops = 
eventdev_ops->rx_adapter_ops[id];

        /* adapter specific info ops)
        ops->info(ops,....);

}

IMO, the mapping with your current proposal may go like this.

- rte_event_eth_rx_adapter_create() // Will just fill the adapter ops from 
driver, probably
   your existing rte_event_eth_rx_adapter_init() can go here
- exiting rte_event_eth_rx_adapter_create() will be more of
proposed rte_event_eth_rx_adapter_configure() on the given adapter.
- existing rte_event_eth_rx_adapter_queue_add() maps 1:1. But you can
   remove eth_dev_id as it is provided proposed 
rte_event_eth_rx_adapter_create()

See above for the case where the application could call create() with 2 different ports.

- same applies to rte_event_eth_rx_adapter_queue_del()
- exiting rte_event_eth_rx_adapter_stats_get() can be changed to
   "xstat"(more like eventdev xstat scheme) so based on the adapter
capability application can get stats)


flow shall be:
1) application calls rte_event_eth_rx_adapter_create(id, eventdev_id,
ethdev_id) to create the adapter.(Drivers fills the adapter ops based on
eventdev_id + ethdev_id capabilities)
2) application calls rte_event_eth_rx_adapter_info_get() to get the
capability and other information of the adapter.
3) application calls rte_event_eth_rx_adapter_configure() to configure
based on the application need and the adapter capability.
4) application calls rte_event_eth_rx_adapter_queue_add() to link ethdev queues 
to
eventdev queues. On this call, The driver creates the service functions or 
programs the HW registers
based on the adapter capability to take the events from ethdev and inject to 
eventdev.


The flow looks good to me.




But from previous emails (see struct rte_event_dev_producer_conf below), it
appears that the rx queue id and event information would be needed to create
the adapter that enqueues from the ethdev queue id to the event pmd, however
that information is available only at queue add time - thoughts ?

Just eventdev_id and ethdev_id is enough to create adapter.

OK.

Reply via email to