On Tue, Jun 20, 2017 at 3:12 PM, Savolainen, Petri (Nokia - FI/Espoo)
<petri.savolai...@nokia.com> wrote:
>
>
>> -----Original Message-----
>> From: shally verma [mailto:shallyvermacav...@gmail.com]
>> Sent: Tuesday, June 20, 2017 8:55 AM
>> To: Bill Fischofer <bill.fischo...@linaro.org>
>> Cc: Savolainen, Petri (Nokia - FI/Espoo) <petri.savolai...@nokia.com>;
>> lng-odp-forward <lng-odp@lists.linaro.org>
>> Subject: Re: [lng-odp] [API-NEXT PATCH v3 1/3] api: event: add subtype to
>> expand event type
>>
>> On Fri, Jun 16, 2017 at 7:15 PM, Bill Fischofer
>> <bill.fischo...@linaro.org> wrote:
>> > On Fri, Jun 16, 2017 at 8:33 AM, shally verma
>> > <shallyvermacav...@gmail.com> wrote:
>> >> On Fri, Jun 16, 2017 at 6:42 PM, Bill Fischofer
>> >> <bill.fischo...@linaro.org> wrote:
>> >>> On Fri, Jun 16, 2017 at 8:04 AM, Savolainen, Petri (Nokia - FI/Espoo)
>> >>> <petri.savolai...@nokia.com> wrote:
>> >>>>> > diff --git a/include/odp/api/spec/event.h
>> b/include/odp/api/spec/event.h
>> >>>>> > index f22efce5..2ad3ce84 100644
>> >>>>> > --- a/include/odp/api/spec/event.h
>> >>>>> > +++ b/include/odp/api/spec/event.h
>> >>>>> > @@ -37,21 +37,91 @@ extern "C" {
>> >>>>> >
>> >>>>> > /**
>> >>>>> > * @typedef odp_event_type_t
>> >>>>> > - * ODP event types:
>> >>>>> > - * ODP_EVENT_BUFFER, ODP_EVENT_PACKET, ODP_EVENT_TIMEOUT,
>> >>>>> > - * ODP_EVENT_CRYPTO_COMPL, ODP_EVENT_IPSEC_RESULT,
>> >>>>> ODP_EVENT_IPSEC_STATUS
>> >>>>> > + * Event type
>> >>>>> > + *
>> >>>>> > + * Event type specifies purpose and general format of an event.
>> It can
>> >>>>> be
>> >>>>> > + * checked with odp_event_type() or odp_event_types(). Each event
>> type
>> >>>>> has
>> >>>>> > + * functions (e.g. odp_buffer_from_event()) to convert between
>> the
>> >>>>> generic event
>> >>>>> > + * handle (odp_event_t) and the type specific handle (e.g.
>> >>>>> odp_buffer_t).
>> >>>>> > + * Results are undefined, if conversion function of a wrong event
>> type
>> >>>>> is used.
>> >>>>> > + * Application cannot change event type by chaining conversion
>> >>>>> functions.
>> >>>>> > + *
>> >>>>> > + * List of event types:
>> >>>>> > + * - ODP_EVENT_BUFFER
>> >>>>> > + * - Buffer event (odp_buffer_t) for simple data storage and
>> >>>>> message passing
>> >>>>> > + * - ODP_EVENT_PACKET
>> >>>>> > + * - Packet event (odp_packet_t) containing packet data and
>> plenty
>> >>>>> of
>> >>>>>
>> >>>>> Why "plenty of"? "containing packet data and associated metadata"
>> >>>>> seems sufficient.
>> >>>>
>> >>>> It highlights that packets should be used only for packets, not as
>> random buffer space (buffers are simple vs. packets are complex).
>> >>>
>> >>> Ok, it just looked a bit odd. Not a biggie.
>> >>>
>> >>>>
>> >>>> BTW. It would have been nice to get all review comments in one go. I
>> did sent v3 as there were no more comments. I cannot do v4 before our lab
>> is back online from a maintenance break... Hopefully v4 is not necessary.
>> >>>
>> >>> Still working on it. This series needs Bala and Nikhil's reviews as a
>> >>> check against their HW. But so far this looks very reasonable.
>> >>>
>> >>>>
>> >>>> -Petri
>> >>
>> >> So, how does this affect event notification mechanism now?
>> >> Currently we had ODP_EVENT_COMP_COMPL set on a packet to mark
>> >> compression/decompression complete status on requested input. So that
>> >> still remain valid Or we need to introduce it like:
>> >>
>> >> event ODP_EVENT_PACKET
>> >> subtype ODP_EVENT_PACKET_COMP_COMPL?
>> >
>> > For consistency with the subtype pattern defined here I'd guess
>> > ODP_EVENT_PACKET_COMP would be the name to use.
>> >
>> >>
>> Ok. but it still leave question open for me. Should existing
>> Compression/Crypto completion event is going to be there or do we need
>> to change as per current proposal? Or current proposal is for newer
>> event notifications. When an compression/encryption event should be
>> considered as sub-event to packet? What event should be if user send a
>> buffer for compression/decompression than packet? May be I am missing
>> broader perspective to this proposal.
>
> If/when this patch is merged, new work (like comp) should be done against it.
> So, comp packet would be another subtype. Also, crypto completion APIs will
> be changed to this (with deprecation).
>
Ok.
> By buffer, do you mean odp_buffer_t event, or void *ptr ? Maybe ptr/len is
> more relevant addition than odp_buffer_t.
Though not fully decide yet on it but most likely it will ptr/len struct.
> Sync version of that (ptr/len) is easy, async version would need another
> event type.
if so, then I probably retain ODP_EVENT_COMP_COMPL for event
notification on buffer data.
>>
>> Also, I have bit of confusion with event type enums placement.
>> ODO_EVENT_PACKET/ODP_EVENT_CRYPTO_COMPL .. are these standard event
>> types and public? Because I see their definition inside
>> platform/linux-generic/include/odp/api/plat/event_types.h instead of
>> standard spec file at include/odp/api/spec/event.h giving me an
>> impression they are platform specific enum defines.
>
> API spec defines the enum names, but not the values. Application must not
> rely on the values. Depending on implementation, values may be HW defined.
>
Ok.
> /**
> * @typedef odp_event_type_t
> * ODP event types:
> * ODP_EVENT_BUFFER, ODP_EVENT_PACKET,
>
>
>
>>
>> So far I was under impression because they are platform specific so we
>> are introducing conversion APIs like
>> odp_xxx_event_from_event() so that user don't need to know specific
>> event enumeration. As example (referring to crypto as base design) ,
>> we have:
>>
>> odp_comp_compl_t odp_comp_compl_from_event(odp_event_t);
>> odp_comp_compl_result(odp_comp_compl_t comp_handle,
>> odp_comp_op_result_t *result);
>>
>> Am I missing any base assumption here?
>
> odp_event_t is the base class of actual events (e.g. odp_packet_t).
> Odp_event_t offers minimal operations such as check types or free event
> without checking the type. Odp_event_t handle needs to be converted to the
> type specific handle, before application is able to access more
> features/metadata. The result call is one example of getting metadata from an
> event.
>
> With the subtype specification these would turn into:
>
> odp_packet_t odp_comp_packet_from_event(odp_event_t ev);
> int odp_comp_result(odp_comp_result_t *result, odp_packet_t pkt);
>
> ... or
>
> int odp_comp_pkt_result(odp_comp_pkt_result_t *result, odp_packet_t pkt);
> int odp_comp_xxx_result(odp_comp_xxx_result_t *result, odp_comp_xxx_t xxx);
>
> ... if you speculate with another non-packet result type.
>
Can it simply be translated to one single API like odp_xxx_get_event_data() .
For ex. for compression, it translates to:
odp_event_type_t event_t = odp_event_get_type_subtype(event, &subtype);
if(event_t == ODP_EVENT_PACKET) {
switch(subtype) {
case ODP_EVENT_PACKET_COMP:
odp_comp_op_result_t result;
odp_comp_get_event_data(event, (void *)&result);
/* Or since event is originating from packet so it can be
odp_packet_get_event_data() as well.
*/
break;
}
}
this way odp_xxx_get_event_data() can further scale to handle more
event types or sub-types to it. Otherwise I see we will need to add
conversion API for each new type and sub-type specific data. What is
more preferred approach here?
Thanks
Shally
>
> -Petri
>
>
