[dpdk-dev] [PATCH v8 7/9] test: add event timer adapter auto-test

[Carrillo, Erik G]

Hi Pavan,

> -----Original Message-----
> From: Bhagavatula, Pavan [mailto:Pavan.Bhagavatula at cavium.com]
> Sent: Friday, March 30, 2018 10:49 AM
> To: Carrillo, Erik G <erik.g.carrillo at intel.com>; Jacob, Jerin
> <Jerin.JacobKollanukkaran at cavium.com>; nipun.gupta at nxp.com;
> hemant.agrawal at nxp.com
> Cc: dev at dpdk.org
> Subject: Re: [PATCH v8 7/9] test: add event timer adapter auto-test
> 
> Hi Erik,
> 
> Few comments below,
> 

<...snipped...>

> > +/* Test that adapter stops correctly. */ static int
> > +adapter_stop(void)
> > +{
> > +	uint32_t evdev_service_id, adapter_service_id;
> > +	struct rte_event_timer_adapter *l_adapter = NULL;
> > +
> Please use INTERNAL_PORT capability to determine if service core is
> required.
> 
 
Good catch.  I meant to remove the references to service ids here, so I've gone ahead and done that.

<...snipped...>

> > +stat_inc_reset_ev_enq(void)
> > +{
> > +	int ret, i, n;
> > +	int num_evtims = MAX_TIMERS;
> > +	struct rte_event_timer *evtims[num_evtims];
> > +	struct rte_event evs[BATCH_SIZE];
> > +	struct rte_event_timer_adapter_stats stats;
> > +	const struct rte_event_timer init_tim = {
> > +		.ev.op = RTE_EVENT_OP_NEW,
> > +		.ev.queue_id = TEST_QUEUE_ID,
> > +		.ev.sched_type = RTE_SCHED_TYPE_ATOMIC,
> > +		.ev.priority = RTE_EVENT_DEV_PRIORITY_NORMAL,
> > +		.ev.event_type =  RTE_EVENT_TYPE_TIMER,
> > +		.state = RTE_EVENT_TIMER_NOT_ARMED,
> > +		.timeout_ticks = 5,	// expire in .5 sec
> > +	};
> > +
> > +	ret = rte_mempool_get_bulk(eventdev_test_mempool, (void
> **)evtims,
> > +				   num_evtims);
> > +	TEST_ASSERT_EQUAL(ret, 0, "Failed to get array of timer objs: ret =
> %d",
> > +			  ret);
> > +
> > +	for (i = 0; i < num_evtims; i++) {
> > +		*evtims[i] = init_tim;
> > +		evtims[i]->ev.event_ptr = evtims[i];
> > +	}
> > +
> > +	ret = rte_event_timer_adapter_stats_get(timdev, &stats);
> > +	TEST_ASSERT_EQUAL(ret, 0, "Failed to get stats");
> > +	TEST_ASSERT_EQUAL((int)stats.ev_enq_count, 0, "Stats not clear at "
> > +			  "startup");
> > +
> > +	/* Test with the max value for the adapter */
> > +	ret = rte_event_timer_arm_burst(timdev, evtims, num_evtims);
> > +	TEST_ASSERT_EQUAL(ret, num_evtims,
> > +			  "Failed to arm all event timers: attempted = %d, "
> > +			  "succeeded = %d, rte_errno = %s",
> > +			  num_evtims, ret, rte_strerror(rte_errno));
> > +
> > +	rte_delay_ms(1000);
> > +
> > +#define MAX_TRIES 1000
> 
> Please make MAX_TRIES equivalent to num_evtims, here we are trying to
> deq 10 events in burst and assume it to succeed but in case event dev
> doesn't support burst mode event will be stuck as it will have only 1000 tries
> instead of 4096.

Good catch again - I'll make these changes.
 
<...snipped...>

> > +static int
> > +adapter_create_max(void)
> > +{
> > +	int i;
> > +	uint32_t svc_start_count, svc_end_count;
> > +	struct rte_event_timer_adapter *adapters[
> > +
> 	RTE_EVENT_TIMER_ADAPTER_NUM_MAX + 1];
> > +
> > +	struct rte_event_timer_adapter_conf conf = {
> > +		.event_dev_id = evdev,
> > +		// timer_adapter_id set in loop
> > +		.clk_src = RTE_EVENT_TIMER_ADAPTER_CPU_CLK,
> > +		.timer_tick_ns = NSECPERSEC / 10,
> > +		.max_tmo_ns = 180 * NSECPERSEC,
> > +		.nb_timers = MAX_TIMERS,
> > +		.flags = 0,
> > +	};
> > +
> > +	svc_start_count = rte_service_get_count();
> > +
> > +	/* This test expects that there are sufficient service IDs available
> > +	 * to be allocated. I.e., RTE_EVENT_TIMER_ADAPTER_NUM_MAX
> may need to
> > +	 * be less than RTE_SERVICE_NUM_MAX if anything else uses a
> service
> > +	 * (the SW event device, for example).
> > +	 */
> Same as above service use need to be dependent on INTERNAL_PORT
> capability.
> 
> Also, in software event dev case this can be a valid test but in case of a hw
> event dev RTE_EVENT_TIMER_ADAPTER_NUM_MAX number of event
> devicesmight not be binded to dpdk. Either we need to provide a API to
> check how many event devices are supported or use
> eventdev_timer_adapter_caps_get_t to get the number.
> 
> Thoughts?
> 

For now, I've modified the test case so that it returns ENOTSUP for the hw case.
But I like the idea of adding an API to query the count of devices.

Thanks,
Gabriel