On Thu, Nov 30, 2017 at 08:59:19PM +0000, Carrillo, Erik G wrote:

Hi Pavan,

Thanks for the review;  I'm working on addressing the comments and have the following question (inline):

<... snipped ...>

quoted

quoted

+	adapter->data->mz = mz;
+	adapter->data->event_dev_id = conf->event_dev_id;
+	adapter->data->id = adapter_id;
+	adapter->data->socket_id = conf->socket_id;
+	adapter->data->conf = *conf;  /* copy conf structure */
+
+	/* Query eventdev PMD for timer adapter capabilities and ops */
+	ret = dev->dev_ops->timer_adapter_caps_get(dev,
+						   &adapter->data->caps,
+						   &adapter->ops);

The underlying driver needs info about the adapter flags i.e.
RTE_EVENT_TIMER_ADAPTER_F_ADJUST_RES and
RTE_EVENT_TIMER_ADAPTER_F_SP_PUT so we need to pass those too conf-

quoted

flags.

By "underlying driver", are you referring to the eventdev PMD, or the event timer adapter "driver" (i.e., the set of ops functions)?

If the latter, the adapter "driver" will have a chance to inspect the flags when adapter->ops->init() is called below, since it can look at the flags through the adapter arg.

I was refering to the timer driver, the presence of flag RTE_EVENT_TIMER_ADAPTER_F_SP_PUT
would suggest the driver to use a multi thread unsafe arm/cancel data path API
and it would set a different function pointers to adapter->arm_burst etc.

I dont think in the current scheme this is possible. Currently, if we see mempool it inspects
flags before setting ops.

Hope this clears things up.

-Pavan

If the former, will the eventdev PMD consider the flags when deciding whether or not to provide an ops definition in the timer_adapter_caps_get() call?

quoted

quoted

+	if (ret < 0) {
+		rte_errno = -EINVAL;
+		return NULL;
+	}
+
+	if (!(adapter->data->caps &
+	      RTE_EVENT_TIMER_ADAPTER_CAP_INTERNAL_PORT)) {
+		FUNC_PTR_OR_NULL_RET_WITH_ERRNO(conf_cb, -EINVAL);
+		ret = conf_cb(adapter->data->id, adapter->data-
event_dev_id,
+			      &adapter->data->event_port_id, conf_arg);
+		if (ret < 0) {
+			rte_errno = -EINVAL;
+			return NULL;
+		}
+	}
+
+	/* Allow driver to do some setup */
+	FUNC_PTR_OR_NULL_RET_WITH_ERRNO(adapter->ops->init, -

ENOTSUP);

quoted

+	ret = adapter->ops->init(adapter);
+	if (ret < 0) {
+		rte_errno = -EINVAL;
+		return NULL;
+	}
+
+	/* Set fast-path function pointers */
+	adapter->arm_burst = adapter->ops->arm_burst;
+	adapter->arm_tmo_tick_burst = adapter->ops-
arm_tmo_tick_burst;
+	adapter->cancel_burst = adapter->ops->cancel_burst;
+
+	adapter->allocated = 1;
+
+	return adapter;
+}

<... snipped ...>