Hi Ola,
Maybe I'm wrong, but let me try to explain change in question.
The main reason in this change is the lack of possibility to convert
buf_hdr to
buf_handle. The function odp_timeout_to_event requires this conversion.
By and
large it's not required the tmo_handle to be the tmo_header for
linux-generic.
It can be tmo_handle = buf_handle. In this case odp_timeout_to_event can
avoid
buf_hdr -> buf_handle conversion. After what it can be reused for
keystone or
other platforms that avoid buffer header to handle conversion.
The second reason is function _odp_buffer_type(), On linux-generic
buffer header
holds a field for event type, in Keysonte, the h/w descriptor contains field
for event type. So there is no reason to save it twice, in h/w
descriptor and
in buffer header. In keystone h/w descriptor = event handle. Holding
event type
in event is more logical. And Taras, probably, wanted to avoid implementing
it's own _odp_buffet_type and replaced it on odp_event_type.
Why this is problematic to implement on Kestone platform?
It's not a problem, it's redundancy.
From what I see, buf_handle = h/w descriptor pointer. H/w descriptor
contains
enough fields to hold buffer information, that's why actual buffer header is
empty for now. It doesn't hold handle and event type, so it cannot be easily
converted. Currently, there is no reason it this, especially if timeout
handle
can be equal to buffer handle.
Am I right, Taras?
But it's not a problem to add link on handle to buffer header, but why?
On 01.06.15 15:12, Ola Liljedahl wrote:
If I understand your patch correctly, a timeout event is really a buffer
with the timeout-specific header stored just after the buffer header (as
before). You don't change the definition of odp_timeout_hdr_t and the
implementation of odp_timeout_hdr(odp_buffer_t buf) has just moved from
odp_timer_internal.h to odp_timer.c where is renamed
to timeout_hdr_from_buf(). There is also a new
function timeout_hdr(odp_timeout_t tmo) which helps with the conversion
from timeout to timeout header.
So you essential change seems to make the timeout event handle the same
as a buffer event handle and not necessarily a pointer (as in the
original code). I don't see why this is an important change (from a
portability or structural point of view), can you please explain this?
-- Ola
On 31 May 2015 at 08:50, Taras Kondratiuk <[email protected]
<mailto:[email protected]>> wrote:
On 05/29/2015 07:26 AM, Ola Liljedahl wrote:
> On 13 May 2015 at 17:31, Ola Liljedahl <[email protected]
<mailto:[email protected]>
> <mailto:[email protected] <mailto:[email protected]>>>
wrote:
>
> On 13 May 2015 at 12:19, Maxim Uvarov <[email protected]
<mailto:[email protected]>
> <mailto:[email protected] <mailto:[email protected]>>>
wrote:
>
> Patch looks good. Validation test passed. Need one more review.
>
> Thanks. I did this primarily for KS2 being able to use the
> linux-generic timer implementation without any changes. I am waiting
> for Taras to verify this.
>
> Well it seems now that Taras is not going to verify that this patch
> solves his problems on KS2. I like the patch anyway and would like to
> see it merged. Any reviewers?
Sorry for a long delay.
I had a slightly different approach in mind.
----8<-----------------------------------------------------------------
From: Taras Kondratiuk <[email protected]
<mailto:[email protected]>>
Date: Sat, 30 May 2015 23:26:19 -0700
Subject: [PATCH] linux-generic: timer: convert odp_timeout_t to event
Signed-off-by: Taras Kondratiuk <[email protected]
<mailto:[email protected]>>
---
.../linux-generic/include/odp/plat/timer_types.h | 8 ++++--
.../linux-generic/include/odp_timer_internal.h | 8 ------
platform/linux-generic/odp_timer.c | 31
++++++++++++----------
3 files changed, 23 insertions(+), 24 deletions(-)
diff --git a/platform/linux-generic/include/odp/plat/timer_types.h
b/platform/linux-generic/include/odp/plat/timer_types.h
index 006683e..d987096 100644
--- a/platform/linux-generic/include/odp/plat/timer_types.h
+++ b/platform/linux-generic/include/odp/plat/timer_types.h
@@ -18,6 +18,10 @@
extern "C" {
#endif
+#include <odp/std_types.h>
+#include <odp/plat/strong_types.h>
+#include <odp/plat/event_types.h>
+
/** @addtogroup odp_timer
* @{
**/
@@ -32,9 +36,9 @@ typedef uint32_t odp_timer_t;
#define ODP_TIMER_INVALID ((uint32_t)~0U)
-typedef void *odp_timeout_t;
+typedef odp_handle_t odp_timeout_t;
-#define ODP_TIMEOUT_INVALID NULL
+#define ODP_TIMEOUT_INVALID (odp_timeout_t)ODP_EVENT_INVALID
/**
* @}
diff --git a/platform/linux-generic/include/odp_timer_internal.h
b/platform/linux-generic/include/odp_timer_internal.h
index 90af62c..db379ec 100644
--- a/platform/linux-generic/include/odp_timer_internal.h
+++ b/platform/linux-generic/include/odp_timer_internal.h
@@ -40,12 +40,4 @@ typedef struct odp_timeout_hdr_stride {
} odp_timeout_hdr_stride;
-/**
- * Return the timeout header
- */
-static inline odp_timeout_hdr_t *odp_timeout_hdr(odp_buffer_t buf)
-{
- return (odp_timeout_hdr_t *)odp_buf_to_hdr(buf);
-}
-
#endif
diff --git a/platform/linux-generic/odp_timer.c
b/platform/linux-generic/odp_timer.c
index b7cb04f..39e76e1 100644
--- a/platform/linux-generic/odp_timer.c
+++ b/platform/linux-generic/odp_timer.c
@@ -80,7 +80,13 @@ static _odp_atomic_flag_t locks[NUM_LOCKS]; /*
Multiple locks per cache line! */
static odp_timeout_hdr_t *timeout_hdr_from_buf(odp_buffer_t buf)
{
- return (odp_timeout_hdr_t *)odp_buf_to_hdr(buf);
+ return (odp_timeout_hdr_t *)(void *)odp_buf_to_hdr(buf);
+}
+
+static odp_timeout_hdr_t *timeout_hdr(odp_timeout_t tmo)
+{
+ odp_buffer_t buf =
odp_buffer_from_event(odp_timeout_to_event(tmo));
+ return timeout_hdr_from_buf(buf);
}
/******************************************************************************
@@ -422,7 +428,8 @@ static bool timer_reset(uint32_t idx,
} else {
/* We have a new timeout buffer which replaces any
old one */
/* Fill in some (constant) header fields for
timeout events */
- if (_odp_buffer_type(*tmo_buf) == ODP_EVENT_TIMEOUT) {
+ if (odp_event_type(odp_buffer_to_event(*tmo_buf)) ==
+ ODP_EVENT_TIMEOUT) {
/* Convert from buffer to timeout hdr */
odp_timeout_hdr_t *tmo_hdr =
timeout_hdr_from_buf(*tmo_buf);
@@ -567,7 +574,8 @@ static unsigned timer_expire(odp_timer_pool *tp,
uint32_t idx, uint64_t tick)
#endif
if (odp_likely(tmo_buf != ODP_BUFFER_INVALID)) {
/* Fill in expiration tick for timeout events */
- if (_odp_buffer_type(tmo_buf) == ODP_EVENT_TIMEOUT) {
+ if (odp_event_type(odp_buffer_to_event(tmo_buf)) ==
+ ODP_EVENT_TIMEOUT) {
/* Convert from buffer to timeout hdr */
odp_timeout_hdr_t *tmo_hdr =
timeout_hdr_from_buf(tmo_buf);
@@ -811,19 +819,17 @@ odp_timeout_t
odp_timeout_from_event(odp_event_t ev)
/* This check not mandated by the API specification */
if (odp_event_type(ev) != ODP_EVENT_TIMEOUT)
ODP_ABORT("Event not a timeout");
- return
(odp_timeout_t)timeout_hdr_from_buf(odp_buffer_from_event(ev));
+ return (odp_timeout_t)ev;
}
odp_event_t odp_timeout_to_event(odp_timeout_t tmo)
{
- odp_timeout_hdr_t *tmo_hdr = (odp_timeout_hdr_t *)tmo;
- odp_buffer_t buf = odp_hdr_to_buf(&tmo_hdr->buf_hdr);
- return odp_buffer_to_event(buf);
+ return (odp_event_t)tmo;
}
int odp_timeout_fresh(odp_timeout_t tmo)
{
- const odp_timeout_hdr_t *hdr = (odp_timeout_hdr_t *)tmo;
+ const odp_timeout_hdr_t *hdr = timeout_hdr(tmo);
odp_timer_t hdl = hdr->timer;
odp_timer_pool *tp = handle_to_tp(hdl);
uint32_t idx = handle_to_idx(hdl, tp);
@@ -836,20 +842,17 @@ int odp_timeout_fresh(odp_timeout_t tmo)
odp_timer_t odp_timeout_timer(odp_timeout_t tmo)
{
- const odp_timeout_hdr_t *hdr = (odp_timeout_hdr_t *)tmo;
- return hdr->timer;
+ return timeout_hdr(tmo)->timer;
}
uint64_t odp_timeout_tick(odp_timeout_t tmo)
{
- const odp_timeout_hdr_t *hdr = (odp_timeout_hdr_t *)tmo;
- return hdr->expiration;
+ return timeout_hdr(tmo)->expiration;
}
void *odp_timeout_user_ptr(odp_timeout_t tmo)
{
- const odp_timeout_hdr_t *hdr = (odp_timeout_hdr_t *)tmo;
- return hdr->user_ptr;
+ return timeout_hdr(tmo)->user_ptr;
}
odp_timeout_t odp_timeout_alloc(odp_pool_t pool)
--
1.9.1
_______________________________________________
lng-odp mailing list
[email protected]
https://lists.linaro.org/mailman/listinfo/lng-odp
_______________________________________________
lng-odp mailing list
[email protected]
https://lists.linaro.org/mailman/listinfo/lng-odp
