Graph events multi enqueue #15

raphacan · 2025-01-10T15:46:16Z

this PR contains the pipelining extension for events handling and multi graphs enqueuing.
conformance tests ran successfully:

conformance_tests/test_engine/test.h

conformance_tests/test_tiovx/test_graph_pipeline.c

source/framework/vx_data_ref_queue.c

source/include/vx_graph.h

source/framework/vx_event_queue.c

source/framework/vx_graph_pipeline.c

conformance_tests/test_tiovx/test_graph_pipeline.c

source/framework/vx_graph_pipeline.c

Signed-off-by: Cano Raphael(XC-AS/EPO3) <Raphael.Cano2@de.bosch.com>

source/framework/vx_graph_pipeline.c

Signed-off-by: Cano Raphael(XC-AS/EPO3) <Raphael.Cano2@de.bosch.com>

include/TI/tivx_obj_desc.h

brockTI · 2025-01-22T20:24:36Z

source/framework/vx_graph_pipeline.c

@@ -168,6 +170,26 @@ static vx_status ownGraphPipelineValidateRefsList(
    return status;
 }

+static vx_status ownDecrementEnqueueCount(vx_reference ref)
+{
+    ref->obj_desc->flags &= ~TIVX_OBJ_DESC_DATA_REF_GRAPH_PARAM_ENQUEUED;


Should there be a check if ref is NULL?

This internal function is used in the block from line 551 to 658. Every time we call this function, the ref is previously checked with ownIsValidSpecificReference (lines 581 & 587) and the ref_list is also checked at line 597
So if I add a check in the function to test if this can be NULL, I am afraid it will be difficult to do a test to achieve the branch testing.
What do you think?

Agreed, it would be unlikely to reach via a test - in that case, would you mind adding a comment stating that the reference has previously been checked for NULL?

I can put a comment or add the condition check and later we put a deviation for the coverage? What do you prefer?

A comment will be sufficient, thanks

brockTI · 2025-01-22T22:14:56Z

conformance_tests/test_tiovx/test_graph_pipeline.c

+    vx_status status;    
+    vx_context context = context_->vx_context_;
+
+    vx_graph graph1 = vxCreateGraph(context);


Could you please use the ASSERT_VX_OBJECT macro for the vxCreate instances in this file?

jessegv · 2025-01-23T22:27:51Z

include/TI/tivx.h

+    TIVX_REFERENCE_BUFFER_IS_ALLOCATED = VX_ATTRIBUTE_BASE(VX_ID_TI, (vx_enum)VX_TYPE_REFERENCE) + 0x2,
+
+    /*! \brief Used to query the reference for the current enqueue count. Read-write. Use a <tt>\ref vx_uint32</tt> parameter. */ 
+    TIVX_REFERENCE_ENQUEUE_COUNT = VX_ATTRIBUTE_BASE(VX_ID_TI, (vx_enum)VX_TYPE_REFERENCE) + 0x3


The Pipelining Extension says that this is added to main spec, except without the TI prefix. If there is not a plan to add to main header files, then perhaps we add it as a constant to the Pipelining extension since it is paired with that ... or at least change the name here to remove the TI prefix since it is specified in the pipelineing extension?

What do you want to do?

Please remove this line from tivx.h, and update the pipelining extension header file in this patch to match the latest one from gitlab, it has it defined there as of today.

done, updated with the latest pipelining header

jessegv · 2025-01-23T23:06:32Z

include/VX/vx_khr_pipelining.h

+ * Notice that there is only one app_value per graph parameter.
+ * 
+ */
+VX_API_ENTRY vx_status VX_API_CALL vxRegisterGraphEvent(vx_graph graph, enum vx_event_type_e type, vx_uint32 param, vx_uint32 app_value);


The API here should take a vx_reference as first parameter, which is called graph_or_node

done, code and test modified

The spec says that VX_EVENT_GRAPH_COMPLETED, VX_EVENT_NODE_COMPLETED, and VX_EVENT_NODE_ERROR should also be supported by this function.

header updated

jessegv · 2025-01-23T23:13:37Z

include/VX/vx_khr_pipelining.h

+ *
+ * \ingroup group_event
+ */
+VX_API_ENTRY vx_status VX_API_CALL vxSendUserGraphEvent(vx_graph graph, vx_uint32 app_value, const void *parameter);


I noticed that this header file has both send user events with the last argument as a const qualified pointer, but the spec does not say that. I need to thing more ... should be be const or not?

good question, the "const" says the value it points to is an input parameter (read only). Not sure if this is important for the user of the function as this is specified by the interface

the new header does not have the const anymore, discussion can be closed?

…h consumers Signed-off-by: Cano Raphael(XC-AS/EPO3) <Raphael.Cano2@de.bosch.com>

Signed-off-by: Cano Raphael(XC-AS/EPO3) <Raphael.Cano2@de.bosch.com>

…PI spec, adapt tests to cover all cases Signed-off-by: Cano Raphael(XC-AS/EPO3) <Raphael.Cano2@de.bosch.com>

Signed-off-by: Cano Raphael(XC-AS/EPO3) <Raphael.Cano2@de.bosch.com>

source/include/vx_graph.h

lcweaver · 2025-01-29T21:58:42Z

conformance_tests/test_tiovx/test_graph_pipeline.c

+
+    ASSERT_EQ_VX_STATUS(VX_SUCCESS, vxGraphParameterEnqueueReadyRef(graph1, 0, (vx_reference *)&images[0], 1));
+    ASSERT_EQ_VX_STATUS(VX_SUCCESS, status = vxGraphParameterEnqueueReadyRef(graph1, 1, (vx_reference *)&images[0], 1));
+    if (VX_SUCCESS == status)


@raphacan I am trying to understand under which conditions it would enter the if statement vs the else statement. Would the else statement need to throw some kind of error or is this a valid condition?

lcweaver · 2025-01-29T22:00:15Z

conformance_tests/test_tiovx/test_graph_pipeline.c

+
+  Therefore, enqueuing the same reference twice SHOULD return an error!
+
+  Note: we modify this rule, proposed change is two rules:


@raphacan It looks like there was a recent change to the pipelining 2.0 spec, so can we re-word the comment to indicate that this is now mandated by the spec?

lcweaver · 2025-01-29T22:04:27Z

conformance_tests/test_tiovx/test_graph_pipeline.c

+    vx_context context = context_->vx_context_;
+    /* Check some basic error returns */
+    vx_graph graph = vxCreateGraph(context);
+    vx_image images[3] = 


@raphacan It might be a good idea to define and use a macro here for the size of this array and others within this test which also use this size.

source/framework/vx_graph_pipeline.c

lcweaver · 2025-01-29T22:20:04Z

source/framework/vx_graph_pipeline.c

+                    if (ref == graph->parameters[graph_parameter_index].refs_list[buf_id])
+                    {
+                        ownPlatformSystemLock((vx_enum)TIVX_PLATFORM_LOCK_DATA_REF_QUEUE);
+                        tivx_obj_desc_t *objd = ref->obj_desc;


@raphacan I am wondering if it makes sense to split out this section into its own dedicated function for readability sake. It seems to me that this section within the lock might be a good candidate for that

source/framework/vx_graph_pipeline.c

lcweaver · 2025-01-29T22:24:19Z

source/framework/vx_graph_pipeline.c

+                /* get queue object descriptor */
+                queue_obj_desc_id = data_ref_q->done_q_obj_desc_id;
+                ownPlatformSystemLock(TIVX_PLATFORM_LOCK_DATA_REF_QUEUE);
+                status = ownObjDescQueueDequeue(queue_obj_desc_id, &ref_obj_desc_id);


@raphacan Would this part of the code also be a good candidate for splitting out into its own function?

you between the lock and unlock function calls?
what do you suggest for a function#s name?

Yes, I think that probably makes sense, although it is a little hard to read fully in github, so if another spot makes more sense, I am open to it.

As far as a name, inferring from the details of the logic and using the typical convention we have, perhaps something like the below:

ownGraphPipelineDecrementEnqueueCount

source/framework/vx_graph_pipeline.c

jessegv · 2025-01-30T00:23:07Z

source/framework/vx_event_queue.c

+    vx_status status = (vx_status)VX_SUCCESS;
+    vx_graph graph;
+    vx_node node;
+    if (VX_EVENT_GRAPH_PARAMETER_CONSUMED != type)


The spec says that VX_EVENT_GRAPH_COMPLETED, VX_EVENT_NODE_COMPLETED, and VX_EVENT_NODE_ERROR should also be supported by this function.

…misra corrections Signed-off-by: Cano Raphael(XC-AS/EPO3) <Raphael.Cano2@de.bosch.com>

lcweaver requested changes Jan 15, 2025

View reviewed changes

[TIOVX-2108] Multi enqueue and graph events implementation

4986eeb

Signed-off-by: Cano Raphael(XC-AS/EPO3) <Raphael.Cano2@de.bosch.com>

jessegv force-pushed the graph_events_multi_enqueue branch from cec56c6 to 4986eeb Compare January 21, 2025 22:30

lcweaver reviewed Jan 21, 2025

View reviewed changes

source/framework/vx_graph_pipeline.c Outdated Show resolved Hide resolved

[TIOVX-2108] fix merge issue, fix other misra

9ef14e0

Signed-off-by: Cano Raphael(XC-AS/EPO3) <Raphael.Cano2@de.bosch.com>

lcweaver reviewed Jan 22, 2025

View reviewed changes

include/TI/tivx_obj_desc.h Show resolved Hide resolved

brockTI suggested changes Jan 23, 2025

View reviewed changes

jessegv requested changes Jan 23, 2025

View reviewed changes

raphacan added 5 commits January 24, 2025 13:40

[TIOVX-2108] add missing feature: enqueues same ref into several grap…

499033d

…h consumers Signed-off-by: Cano Raphael(XC-AS/EPO3) <Raphael.Cano2@de.bosch.com>

[TIOVX-2108] add missing feature: adapt size of the array

e52a16c

Signed-off-by: Cano Raphael(XC-AS/EPO3) <Raphael.Cano2@de.bosch.com>

[TIOVX-2108] fix alignment issue on the obj_desc

966411b

Signed-off-by: Cano Raphael(XC-AS/EPO3) <Raphael.Cano2@de.bosch.com>

[TIOVX-2108] adapt the vxRegisterGraphEvent prototype to the latest A…

6d3d653

…PI spec, adapt tests to cover all cases Signed-off-by: Cano Raphael(XC-AS/EPO3) <Raphael.Cano2@de.bosch.com>

[TIOVX-2108] improve coverage results

070f729

Signed-off-by: Cano Raphael(XC-AS/EPO3) <Raphael.Cano2@de.bosch.com>

lcweaver requested changes Jan 29, 2025

View reviewed changes

jessegv reviewed Jan 30, 2025

View reviewed changes

[TIOVX-2108] update with the latest pipelining extension header, few …

69dc090

…misra corrections Signed-off-by: Cano Raphael(XC-AS/EPO3) <Raphael.Cano2@de.bosch.com>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Graph events multi enqueue #15

Graph events multi enqueue #15

raphacan commented Jan 10, 2025

brockTI Jan 22, 2025

raphacan Jan 27, 2025

brockTI Jan 29, 2025

raphacan Jan 30, 2025 •

edited

Loading

lcweaver Jan 30, 2025

brockTI Jan 22, 2025

raphacan Jan 27, 2025

jessegv Jan 23, 2025

raphacan Jan 27, 2025

jessegv Jan 30, 2025 •

edited

Loading

raphacan Jan 30, 2025

jessegv Jan 23, 2025

raphacan Jan 27, 2025

jessegv Jan 30, 2025

raphacan Jan 30, 2025

jessegv Jan 23, 2025

raphacan Jan 27, 2025

raphacan Jan 30, 2025

lcweaver Jan 29, 2025

lcweaver Jan 29, 2025

lcweaver Jan 29, 2025

lcweaver Jan 29, 2025

lcweaver Jan 29, 2025

raphacan Jan 30, 2025

lcweaver Jan 30, 2025

jessegv Jan 30, 2025


		Therefore, enqueuing the same reference twice SHOULD return an error!

		Note: we modify this rule, proposed change is two rules:

Graph events multi enqueue #15

Are you sure you want to change the base?

Graph events multi enqueue #15

Conversation

raphacan commented Jan 10, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

raphacan Jan 30, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jessegv Jan 30, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

raphacan Jan 30, 2025 •

edited

Loading

jessegv Jan 30, 2025 •

edited

Loading