| Message ID | 20200819180442.11630-5-kgupta@es.iitr.ac.in (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | media: vimc: Multiple stream support in vimc | expand |
Am 19.08.20 um 20:04 schrieb Kaaira Gupta: > Separate the process of initialising pipeline array from starting > streaming for all entities in path of a stream. This is needed because > multiple streams can stream, but only one pipeline object is needed. > > Process frames only for those entities in a pipeline which are > streaming. This is known through their use counts. > > Signed-off-by: Kaaira Gupta <kgupta@es.iitr.ac.in> > --- > .../media/test-drivers/vimc/vimc-streamer.c | 95 ++++++++++++++++--- > 1 file changed, 83 insertions(+), 12 deletions(-) > > diff --git a/drivers/media/test-drivers/vimc/vimc-streamer.c b/drivers/media/test-drivers/vimc/vimc-streamer.c > index c1644d69686d..cc40ecabe95a 100644 > --- a/drivers/media/test-drivers/vimc/vimc-streamer.c > +++ b/drivers/media/test-drivers/vimc/vimc-streamer.c > @@ -40,33 +40,30 @@ static void vimc_streamer_pipeline_terminate(struct vimc_stream *stream) > } > > /** > - * vimc_streamer_pipeline_init - Initializes the stream structure > + * vimc_streamer_stream_start - Starts streaming for all entities > + * in a stream > * > - * @stream: the pointer to the stream structure to be initialized > * @ved: the pointer to the vimc entity initializing the stream > * > - * Initializes the stream structure. Walks through the entity graph to > - * construct the pipeline used later on the streamer thread. > - * Calls vimc_streamer_s_stream() to enable stream in all entities of > - * the pipeline. > + * Walks through the entity graph to call vimc_streamer_s_stream() > + * to enable stream in all entities in path of a stream. > * > * Return: 0 if success, error code otherwise. > */ > -static int vimc_streamer_pipeline_init(struct vimc_stream *stream, > - struct vimc_ent_device *ved) > +static int vimc_streamer_stream_start(struct vimc_stream *stream, > + struct vimc_ent_device *ved) > { > struct media_entity *entity; > struct video_device *vdev; > struct v4l2_subdev *sd; > + int stream_size = 0; > int ret = 0; > > - stream->pipe_size = 0; > - while (stream->pipe_size < VIMC_STREAMER_PIPELINE_MAX_SIZE) { > + while (stream_size < VIMC_STREAMER_PIPELINE_MAX_SIZE) { > if (!ved) { > vimc_streamer_pipeline_terminate(stream); > return -EINVAL; > } > - stream->ved_pipeline[stream->pipe_size++] = ved; > > if (is_media_entity_v4l2_subdev(ved->ent)) { > sd = media_entity_to_v4l2_subdev(ved->ent); > @@ -104,6 +101,73 @@ static int vimc_streamer_pipeline_init(struct vimc_stream *stream, > entity); > ved = video_get_drvdata(vdev); > } > + stream_size++; > + } > + > + vimc_streamer_pipeline_terminate(stream); > + return -EINVAL; > +} > + > +/** > + * vimc_streamer_pipeline_init - Initialises pipeline and pipe size > + * > + * @stream: the pointer to the stream structure > + * @ved: the pointer to the vimc entity initializing the stream pipeline > + * > + * Walks through the entity graph to initialise ved_pipeline and updates > + * pipe_size too. > + * > + * Return: 0 if success, error code otherwise. > + */ > +static int vimc_streamer_pipeline_init(struct vimc_stream *stream, > + struct vimc_ent_device *ved) > +{ > + struct media_entity *entity; > + struct media_device *mdev; > + struct media_graph graph; > + struct video_device *vdev; > + struct v4l2_subdev *sd; > + int ret; > + > + entity = ved->ent; > + mdev = entity->graph_obj.mdev; > + > + ret = media_graph_walk_init(&graph, mdev); > + if (ret) > + return ret; > + > + media_graph_walk_start(&graph, entity); > + > + /* > + * Start pipeline array initialisation from RAW Capture only to get > + * entities in the correct order of their frame processing. > + */ > + if (!strncmp(entity->name, "RGB", 3)) { I don't understand this condition, way is it good for? I think the function should be generic and not assume names of entities or specific topology. > + entity = media_graph_walk_next(&graph); > + mdev = entity->graph_obj.mdev; > + media_graph_walk_cleanup(&graph); > + > + ret = media_graph_walk_init(&graph, mdev); > + if (ret) > + return ret; > + media_graph_walk_start(&graph, entity); > + } > + > + while (stream->pipe_size < VIMC_STREAMER_PIPELINE_MAX_SIZE) { > + if (is_media_entity_v4l2_subdev(entity)) { > + sd = media_entity_to_v4l2_subdev(entity); > + ved = v4l2_get_subdevdata(sd); > + } else { > + vdev = container_of(entity, struct video_device, entity); > + ved = video_get_drvdata(vdev); > + } > + stream->ved_pipeline[stream->pipe_size++] = ved; > + entity = media_graph_walk_next(&graph); > + > + if (!strcmp(entity->name, stream->ved_pipeline[0]->ent->name)) { I also don't understand this condition > + media_graph_walk_cleanup(&graph); > + return 0; > + } > } It is not clear what this function does, it looks like it adds to 'ved_pipeline' all entities that are connected to the video node, in addition to the entities that where there from previous calls, so some entities appear several times. I think there is no need to use the graph walk here but to access the source entity in each iteration, the way done in vimc_streamer_stream_start also. I think the code should iterate here until it reaches an entity that is already streaming, this means that the entity is already in the `ved_pipeline`, also you should make sure that the sensor is the first entity that process a frame, therefore the sensor should be at the end/start of the list of entities. Generally each entity should appear exactly once in the 'ved_pipeline' array and the entities should be ordered such that when calling 'process_frame' on one entity should be after calling 'process_frame' on its source entity. maybe it is easyer to implement if 'ved_pipeline' is a linked list. Thanks, Dafna > > vimc_streamer_pipeline_terminate(stream); > @@ -138,8 +202,11 @@ static int vimc_streamer_thread(void *data) > > for (i = stream->pipe_size - 1; i >= 0; i--) { > ved = stream->ved_pipeline[i]; > - ret = ved->process_frame(ved); > > + if (atomic_read(&ved->use_count) == 0) > + continue; > + > + ret = ved->process_frame(ved); > if (ret) > break; > } > @@ -179,6 +246,10 @@ int vimc_streamer_s_stream(struct vimc_stream *stream, > if (stream->kthread) > return 0; > > + ret = vimc_streamer_stream_start(stream, ved); > + if (ret) > + return ret; > + > ret = vimc_streamer_pipeline_init(stream, ved); > if (ret) > return ret; >
Hi, On Fri, Aug 21, 2020 at 05:11:23PM +0200, Dafna Hirschfeld wrote: > > > Am 19.08.20 um 20:04 schrieb Kaaira Gupta: > > Separate the process of initialising pipeline array from starting > > streaming for all entities in path of a stream. This is needed because > > multiple streams can stream, but only one pipeline object is needed. > > > > Process frames only for those entities in a pipeline which are > > streaming. This is known through their use counts. > > > > Signed-off-by: Kaaira Gupta <kgupta@es.iitr.ac.in> > > --- > > .../media/test-drivers/vimc/vimc-streamer.c | 95 ++++++++++++++++--- > > 1 file changed, 83 insertions(+), 12 deletions(-) > > > > diff --git a/drivers/media/test-drivers/vimc/vimc-streamer.c b/drivers/media/test-drivers/vimc/vimc-streamer.c > > index c1644d69686d..cc40ecabe95a 100644 > > --- a/drivers/media/test-drivers/vimc/vimc-streamer.c > > +++ b/drivers/media/test-drivers/vimc/vimc-streamer.c > > @@ -40,33 +40,30 @@ static void vimc_streamer_pipeline_terminate(struct vimc_stream *stream) > > } > > /** > > - * vimc_streamer_pipeline_init - Initializes the stream structure > > + * vimc_streamer_stream_start - Starts streaming for all entities > > + * in a stream > > * > > - * @stream: the pointer to the stream structure to be initialized > > * @ved: the pointer to the vimc entity initializing the stream > > * > > - * Initializes the stream structure. Walks through the entity graph to > > - * construct the pipeline used later on the streamer thread. > > - * Calls vimc_streamer_s_stream() to enable stream in all entities of > > - * the pipeline. > > + * Walks through the entity graph to call vimc_streamer_s_stream() > > + * to enable stream in all entities in path of a stream. > > * > > * Return: 0 if success, error code otherwise. > > */ > > -static int vimc_streamer_pipeline_init(struct vimc_stream *stream, > > - struct vimc_ent_device *ved) > > +static int vimc_streamer_stream_start(struct vimc_stream *stream, > > + struct vimc_ent_device *ved) > > { > > struct media_entity *entity; > > struct video_device *vdev; > > struct v4l2_subdev *sd; > > + int stream_size = 0; > > int ret = 0; > > - stream->pipe_size = 0; > > - while (stream->pipe_size < VIMC_STREAMER_PIPELINE_MAX_SIZE) { > > + while (stream_size < VIMC_STREAMER_PIPELINE_MAX_SIZE) { > > if (!ved) { > > vimc_streamer_pipeline_terminate(stream); > > return -EINVAL; > > } > > - stream->ved_pipeline[stream->pipe_size++] = ved; > > if (is_media_entity_v4l2_subdev(ved->ent)) { > > sd = media_entity_to_v4l2_subdev(ved->ent); > > @@ -104,6 +101,73 @@ static int vimc_streamer_pipeline_init(struct vimc_stream *stream, > > entity); > > ved = video_get_drvdata(vdev); > > } > > + stream_size++; > > + } > > + > > + vimc_streamer_pipeline_terminate(stream); > > + return -EINVAL; > > +} > > + > > +/** > > + * vimc_streamer_pipeline_init - Initialises pipeline and pipe size > > + * > > + * @stream: the pointer to the stream structure > > + * @ved: the pointer to the vimc entity initializing the stream pipeline > > + * > > + * Walks through the entity graph to initialise ved_pipeline and updates > > + * pipe_size too. > > + * > > + * Return: 0 if success, error code otherwise. > > + */ > > +static int vimc_streamer_pipeline_init(struct vimc_stream *stream, > > + struct vimc_ent_device *ved) > > +{ > > + struct media_entity *entity; > > + struct media_device *mdev; > > + struct media_graph graph; > > + struct video_device *vdev; > > + struct v4l2_subdev *sd; > > + int ret; > > + > > + entity = ved->ent; > > + mdev = entity->graph_obj.mdev; > > + > > + ret = media_graph_walk_init(&graph, mdev); > > + if (ret) > > + return ret; > > + > > + media_graph_walk_start(&graph, entity); > > + > > + /* > > + * Start pipeline array initialisation from RAW Capture only to get > > + * entities in the correct order of their frame processing. > > + */ > > + if (!strncmp(entity->name, "RGB", 3)) { > > I don't understand this condition, way is it good for? I have explained that later in the reply > > I think the function should be generic and not assume names of entities > or specific topology. It doesn't assume the topology, rather it is in place just to make sure that the entities in ved_pipeline are in correct order. > > > > + entity = media_graph_walk_next(&graph); > > + mdev = entity->graph_obj.mdev; > > + media_graph_walk_cleanup(&graph); > > + > > + ret = media_graph_walk_init(&graph, mdev); > > + if (ret) > > + return ret; > > + media_graph_walk_start(&graph, entity); > > + } > > + > > + while (stream->pipe_size < VIMC_STREAMER_PIPELINE_MAX_SIZE) { > > + if (is_media_entity_v4l2_subdev(entity)) { > > + sd = media_entity_to_v4l2_subdev(entity); > > + ved = v4l2_get_subdevdata(sd); > > + } else { > > + vdev = container_of(entity, struct video_device, entity); > > + ved = video_get_drvdata(vdev); > > + } > > + stream->ved_pipeline[stream->pipe_size++] = ved; > > + entity = media_graph_walk_next(&graph); > > + > > + if (!strcmp(entity->name, stream->ved_pipeline[0]->ent->name)) { > > I also don't understand this condition > > > + media_graph_walk_cleanup(&graph); > > + return 0; > > + } > > } > > It is not clear what this function does, it looks like it adds to 'ved_pipeline' > all entities that are connected to the video node, in addition to the entities > that where there from previous calls, so some entities appear several times. This function returns all the entities in a pipe, weather they are streaming or not. For example, if only the RAW Capture 1 streams, or RGB/YUB capture streams, or both stream..in all three cases ved_pipeline will have the same entities, in exactly same order, and all will occur just once. Since media_graph_walk goes to each node of the graph, it returns back to the first one (as its a graph), hence the last condition, ie, if (!strcmp(entity->name, stream->ved_pipeline[0]->ent->name)) { makes sure that the first entity is not added again to the array. First condition, ie if (!strncmp(entity->name, "RGB", 3)) { Just makes sure that the search starts at RGB capture only. This is because, in case of any other video node, the order of entities, as you have mentioned later in the mail, will not be desirable, ie it won't start at sensor and end at captures. So this condition just takes care that all the entities in ved_pipeline array are in correct order (starting at sensor, ending at captures). Thanks, Kaaira > > I think there is no need to use the graph walk here but to access the source entity > in each iteration, the way done in vimc_streamer_stream_start > also. > I think the code should iterate here until it reaches an entity that is already streaming, > this means that the entity is already in the `ved_pipeline`, also you should make sure > that the sensor is the first entity that process a frame, therefore the sensor should be > at the end/start of the list of entities. Generally each entity should appear exactly once > in the 'ved_pipeline' array and the entities should be ordered such that when calling 'process_frame' > on one entity should be after calling 'process_frame' on its source entity. > maybe it is easyer to implement if 'ved_pipeline' is a linked list. > > Thanks, > Dafna > > > vimc_streamer_pipeline_terminate(stream); > > @@ -138,8 +202,11 @@ static int vimc_streamer_thread(void *data) > > for (i = stream->pipe_size - 1; i >= 0; i--) { > > ved = stream->ved_pipeline[i]; > > - ret = ved->process_frame(ved); > > + if (atomic_read(&ved->use_count) == 0) > > + continue; > > + > > + ret = ved->process_frame(ved); > > if (ret) > > break; > > } > > @@ -179,6 +246,10 @@ int vimc_streamer_s_stream(struct vimc_stream *stream, > > if (stream->kthread) > > return 0; > > + ret = vimc_streamer_stream_start(stream, ved); > > + if (ret) > > + return ret; > > + > > ret = vimc_streamer_pipeline_init(stream, ved); > > if (ret) > > return ret; > >
Hi, Am 21.08.20 um 23:01 schrieb Kaaira Gupta: > Hi, > > On Fri, Aug 21, 2020 at 05:11:23PM +0200, Dafna Hirschfeld wrote: >> >> >> Am 19.08.20 um 20:04 schrieb Kaaira Gupta: >>> Separate the process of initialising pipeline array from starting >>> streaming for all entities in path of a stream. This is needed because >>> multiple streams can stream, but only one pipeline object is needed. >>> >>> Process frames only for those entities in a pipeline which are >>> streaming. This is known through their use counts. >>> >>> Signed-off-by: Kaaira Gupta <kgupta@es.iitr.ac.in> >>> --- >>> .../media/test-drivers/vimc/vimc-streamer.c | 95 ++++++++++++++++--- >>> 1 file changed, 83 insertions(+), 12 deletions(-) >>> >>> diff --git a/drivers/media/test-drivers/vimc/vimc-streamer.c b/drivers/media/test-drivers/vimc/vimc-streamer.c >>> index c1644d69686d..cc40ecabe95a 100644 >>> --- a/drivers/media/test-drivers/vimc/vimc-streamer.c >>> +++ b/drivers/media/test-drivers/vimc/vimc-streamer.c >>> @@ -40,33 +40,30 @@ static void vimc_streamer_pipeline_terminate(struct vimc_stream *stream) >>> } >>> /** >>> - * vimc_streamer_pipeline_init - Initializes the stream structure >>> + * vimc_streamer_stream_start - Starts streaming for all entities >>> + * in a stream >>> * >>> - * @stream: the pointer to the stream structure to be initialized >>> * @ved: the pointer to the vimc entity initializing the stream >>> * >>> - * Initializes the stream structure. Walks through the entity graph to >>> - * construct the pipeline used later on the streamer thread. >>> - * Calls vimc_streamer_s_stream() to enable stream in all entities of >>> - * the pipeline. >>> + * Walks through the entity graph to call vimc_streamer_s_stream() >>> + * to enable stream in all entities in path of a stream. >>> * >>> * Return: 0 if success, error code otherwise. >>> */ >>> -static int vimc_streamer_pipeline_init(struct vimc_stream *stream, >>> - struct vimc_ent_device *ved) >>> +static int vimc_streamer_stream_start(struct vimc_stream *stream, >>> + struct vimc_ent_device *ved) >>> { >>> struct media_entity *entity; >>> struct video_device *vdev; >>> struct v4l2_subdev *sd; >>> + int stream_size = 0; >>> int ret = 0; >>> - stream->pipe_size = 0; >>> - while (stream->pipe_size < VIMC_STREAMER_PIPELINE_MAX_SIZE) { >>> + while (stream_size < VIMC_STREAMER_PIPELINE_MAX_SIZE) { >>> if (!ved) { >>> vimc_streamer_pipeline_terminate(stream); >>> return -EINVAL; >>> } >>> - stream->ved_pipeline[stream->pipe_size++] = ved; >>> if (is_media_entity_v4l2_subdev(ved->ent)) { >>> sd = media_entity_to_v4l2_subdev(ved->ent); >>> @@ -104,6 +101,73 @@ static int vimc_streamer_pipeline_init(struct vimc_stream *stream, >>> entity); >>> ved = video_get_drvdata(vdev); >>> } >>> + stream_size++; >>> + } >>> + >>> + vimc_streamer_pipeline_terminate(stream); >>> + return -EINVAL; >>> +} >>> + >>> +/** >>> + * vimc_streamer_pipeline_init - Initialises pipeline and pipe size >>> + * >>> + * @stream: the pointer to the stream structure >>> + * @ved: the pointer to the vimc entity initializing the stream pipeline >>> + * >>> + * Walks through the entity graph to initialise ved_pipeline and updates >>> + * pipe_size too. >>> + * >>> + * Return: 0 if success, error code otherwise. >>> + */ >>> +static int vimc_streamer_pipeline_init(struct vimc_stream *stream, >>> + struct vimc_ent_device *ved) >>> +{ >>> + struct media_entity *entity; >>> + struct media_device *mdev; >>> + struct media_graph graph; >>> + struct video_device *vdev; >>> + struct v4l2_subdev *sd; >>> + int ret; >>> + >>> + entity = ved->ent; >>> + mdev = entity->graph_obj.mdev; >>> + >>> + ret = media_graph_walk_init(&graph, mdev); >>> + if (ret) >>> + return ret; >>> + >>> + media_graph_walk_start(&graph, entity); >>> + >>> + /* >>> + * Start pipeline array initialisation from RAW Capture only to get >>> + * entities in the correct order of their frame processing. >>> + */ >>> + if (!strncmp(entity->name, "RGB", 3)) { >> >> I don't understand this condition, way is it good for? > > I have explained that later in the reply > >> >> I think the function should be generic and not assume names of entities >> or specific topology. > > It doesn't assume the topology, rather it is in place just to make sure > that the entities in ved_pipeline are in correct order. > >> >> >>> + entity = media_graph_walk_next(&graph); >>> + mdev = entity->graph_obj.mdev; >>> + media_graph_walk_cleanup(&graph); >>> + >>> + ret = media_graph_walk_init(&graph, mdev); >>> + if (ret) >>> + return ret; >>> + media_graph_walk_start(&graph, entity); Hi, can you explain what this code does? Why does it start the search in the next entity? >>> + } >>> + >>> + while (stream->pipe_size < VIMC_STREAMER_PIPELINE_MAX_SIZE) { >>> + if (is_media_entity_v4l2_subdev(entity)) { >>> + sd = media_entity_to_v4l2_subdev(entity); >>> + ved = v4l2_get_subdevdata(sd); >>> + } else { >>> + vdev = container_of(entity, struct video_device, entity); >>> + ved = video_get_drvdata(vdev); >>> + } >>> + stream->ved_pipeline[stream->pipe_size++] = ved; >>> + entity = media_graph_walk_next(&graph); >>> + >>> + if (!strcmp(entity->name, stream->ved_pipeline[0]->ent->name)) { >> >> I also don't understand this condition >> >>> + media_graph_walk_cleanup(&graph); >>> + return 0; >>> + } >>> } >> >> It is not clear what this function does, it looks like it adds to 'ved_pipeline' >> all entities that are connected to the video node, in addition to the entities >> that where there from previous calls, so some entities appear several times. > > This function returns all the entities in a pipe, weather they are > streaming or not. For example, if only the RAW Capture 1 streams, or > RGB/YUB capture streams, or both stream..in all three cases ved_pipeline > will have the same entities, in exactly same order, and all will occur just once. > Since media_graph_walk goes to each node of the graph, it returns back > to the first one (as its a graph), hence the last condition, ie, > > if (!strcmp(entity->name, stream->ved_pipeline[0]->ent->name)) { > > makes sure that the first entity is not added again to the array. > > First condition, ie > > if (!strncmp(entity->name, "RGB", 3)) { > > Just makes sure that the search starts at RGB capture only. This is > because, in case of any other video node, the order of entities, as you > have mentioned later in the mail, will not be desirable, ie it won't > start at sensor and end at captures. So this condition just takes care > that all the entities in ved_pipeline array are in correct order > (starting at sensor, ending at captures). It is better to compare to the whole name of the entity to make it more clear. Also I think it is better to document that this function is called only upon the first streaming node. I still think this function should be independent of the topology. Maybe you can use Helen's patch that allow walking a graph only opposite to the link direction: https://patchwork.kernel.org/patch/11564899/ This ensures that the Sensor will be first in the graph walk. Then the streaming thread iterates the ved_pipeline from 0 upwards and not from 'pipe_size' downwards. Thanks, Dafna > > Thanks, > Kaaira > >> >> I think there is no need to use the graph walk here but to access the source entity >> in each iteration, the way done in vimc_streamer_stream_start >> also. >> I think the code should iterate here until it reaches an entity that is already streaming, >> this means that the entity is already in the `ved_pipeline`, also you should make sure >> that the sensor is the first entity that process a frame, therefore the sensor should be >> at the end/start of the list of entities. Generally each entity should appear exactly once >> in the 'ved_pipeline' array and the entities should be ordered such that when calling 'process_frame' >> on one entity should be after calling 'process_frame' on its source entity. >> maybe it is easyer to implement if 'ved_pipeline' is a linked list. >> >> Thanks, >> Dafna >> >>> vimc_streamer_pipeline_terminate(stream); >>> @@ -138,8 +202,11 @@ static int vimc_streamer_thread(void *data) >>> for (i = stream->pipe_size - 1; i >= 0; i--) { >>> ved = stream->ved_pipeline[i]; >>> - ret = ved->process_frame(ved); >>> + if (atomic_read(&ved->use_count) == 0) >>> + continue; >>> + >>> + ret = ved->process_frame(ved); >>> if (ret) >>> break; >>> } >>> @@ -179,6 +246,10 @@ int vimc_streamer_s_stream(struct vimc_stream *stream, >>> if (stream->kthread) >>> return 0; >>> + ret = vimc_streamer_stream_start(stream, ved); >>> + if (ret) >>> + return ret; >>> + >>> ret = vimc_streamer_pipeline_init(stream, ved); >>> if (ret) >>> return ret; >>>
Hi Kaaira, Dafna, On 28/08/2020 21:37, Dafna Hirschfeld wrote: > Hi, > > Am 21.08.20 um 23:01 schrieb Kaaira Gupta: >> Hi, >> >> On Fri, Aug 21, 2020 at 05:11:23PM +0200, Dafna Hirschfeld wrote: >>> >>> >>> Am 19.08.20 um 20:04 schrieb Kaaira Gupta: >>>> Separate the process of initialising pipeline array from starting >>>> streaming for all entities in path of a stream. This is needed because >>>> multiple streams can stream, but only one pipeline object is needed. >>>> >>>> Process frames only for those entities in a pipeline which are >>>> streaming. This is known through their use counts. >>>> >>>> Signed-off-by: Kaaira Gupta <kgupta@es.iitr.ac.in> >>>> --- >>>> .../media/test-drivers/vimc/vimc-streamer.c | 95 >>>> ++++++++++++++++--- >>>> 1 file changed, 83 insertions(+), 12 deletions(-) >>>> >>>> diff --git a/drivers/media/test-drivers/vimc/vimc-streamer.c >>>> b/drivers/media/test-drivers/vimc/vimc-streamer.c >>>> index c1644d69686d..cc40ecabe95a 100644 >>>> --- a/drivers/media/test-drivers/vimc/vimc-streamer.c >>>> +++ b/drivers/media/test-drivers/vimc/vimc-streamer.c >>>> @@ -40,33 +40,30 @@ static void >>>> vimc_streamer_pipeline_terminate(struct vimc_stream *stream) >>>> } >>>> /** >>>> - * vimc_streamer_pipeline_init - Initializes the stream structure >>>> + * vimc_streamer_stream_start - Starts streaming for all entities >>>> + * in a stream >>>> * >>>> - * @stream: the pointer to the stream structure to be initialized >>>> * @ved: the pointer to the vimc entity initializing the stream >>>> * >>>> - * Initializes the stream structure. Walks through the entity graph to >>>> - * construct the pipeline used later on the streamer thread. >>>> - * Calls vimc_streamer_s_stream() to enable stream in all entities of >>>> - * the pipeline. >>>> + * Walks through the entity graph to call vimc_streamer_s_stream() >>>> + * to enable stream in all entities in path of a stream. >>>> * >>>> * Return: 0 if success, error code otherwise. >>>> */ >>>> -static int vimc_streamer_pipeline_init(struct vimc_stream *stream, >>>> - struct vimc_ent_device *ved) >>>> +static int vimc_streamer_stream_start(struct vimc_stream *stream, >>>> + struct vimc_ent_device *ved) >>>> { >>>> struct media_entity *entity; >>>> struct video_device *vdev; >>>> struct v4l2_subdev *sd; >>>> + int stream_size = 0; >>>> int ret = 0; >>>> - stream->pipe_size = 0; >>>> - while (stream->pipe_size < VIMC_STREAMER_PIPELINE_MAX_SIZE) { >>>> + while (stream_size < VIMC_STREAMER_PIPELINE_MAX_SIZE) { >>>> if (!ved) { >>>> vimc_streamer_pipeline_terminate(stream); >>>> return -EINVAL; >>>> } >>>> - stream->ved_pipeline[stream->pipe_size++] = ved; >>>> if (is_media_entity_v4l2_subdev(ved->ent)) { >>>> sd = media_entity_to_v4l2_subdev(ved->ent); >>>> @@ -104,6 +101,73 @@ static int vimc_streamer_pipeline_init(struct >>>> vimc_stream *stream, >>>> entity); >>>> ved = video_get_drvdata(vdev); >>>> } >>>> + stream_size++; >>>> + } >>>> + >>>> + vimc_streamer_pipeline_terminate(stream); >>>> + return -EINVAL; >>>> +} >>>> + >>>> +/** >>>> + * vimc_streamer_pipeline_init - Initialises pipeline and pipe size >>>> + * >>>> + * @stream: the pointer to the stream structure >>>> + * @ved: the pointer to the vimc entity initializing the stream >>>> pipeline Which entity is this? Is it the start, or the end of the pipeline? I.e. the sensor? or the capture ? >>>> + * >>>> + * Walks through the entity graph to initialise ved_pipeline and >>>> updates >>>> + * pipe_size too. >>>> + * >>>> + * Return: 0 if success, error code otherwise. >>>> + */ >>>> +static int vimc_streamer_pipeline_init(struct vimc_stream *stream, >>>> + struct vimc_ent_device *ved) >>>> +{ >>>> + struct media_entity *entity; >>>> + struct media_device *mdev; >>>> + struct media_graph graph; >>>> + struct video_device *vdev; >>>> + struct v4l2_subdev *sd; >>>> + int ret; >>>> + >>>> + entity = ved->ent; >>>> + mdev = entity->graph_obj.mdev; >>>> + >>>> + ret = media_graph_walk_init(&graph, mdev); >>>> + if (ret) >>>> + return ret; >>>> + >>>> + media_graph_walk_start(&graph, entity); >>>> + >>>> + /* >>>> + * Start pipeline array initialisation from RAW Capture only to >>>> get >>>> + * entities in the correct order of their frame processing. >>>> + */ >>>> + if (!strncmp(entity->name, "RGB", 3)) { >>> >>> I don't understand this condition, way is it good for? >> >> I have explained that later in the reply Matching on entity names is a bit awkward, as they could be changed I guess quite easily, and easily missed in this string matching. I haven't fully understood this code block yet to work out if there's another way we could do this though, but reading ahead I see there might be a way to 'walk the graph' on a per-stream basis which might be a good way of factoring this path. Although there still needs to be a construction of the paths available to a stream which splits from the sensor. >> >>> >>> I think the function should be generic and not assume names of entities >>> or specific topology. >> >> It doesn't assume the topology, rather it is in place just to make sure >> that the entities in ved_pipeline are in correct order. >> >>> >>> >>>> + entity = media_graph_walk_next(&graph); >>>> + mdev = entity->graph_obj.mdev; >>>> + media_graph_walk_cleanup(&graph); >>>> + >>>> + ret = media_graph_walk_init(&graph, mdev); >>>> + if (ret) >>>> + return ret; >>>> + media_graph_walk_start(&graph, entity); > > Hi, can you explain what this code does? > Why does it start the search in the next entity? > >>>> + } >>>> + >>>> + while (stream->pipe_size < VIMC_STREAMER_PIPELINE_MAX_SIZE) { >>>> + if (is_media_entity_v4l2_subdev(entity)) { >>>> + sd = media_entity_to_v4l2_subdev(entity); >>>> + ved = v4l2_get_subdevdata(sd); >>>> + } else { >>>> + vdev = container_of(entity, struct video_device, entity); >>>> + ved = video_get_drvdata(vdev); >>>> + } >>>> + stream->ved_pipeline[stream->pipe_size++] = ved; >>>> + entity = media_graph_walk_next(&graph); >>>> + >>>> + if (!strcmp(entity->name, >>>> stream->ved_pipeline[0]->ent->name)) { >>> >>> I also don't understand this condition >>> >>>> + media_graph_walk_cleanup(&graph); >>>> + return 0; >>>> + } >>>> } >>> >>> It is not clear what this function does, it looks like it adds to >>> 'ved_pipeline' >>> all entities that are connected to the video node, in addition to the >>> entities >>> that where there from previous calls, so some entities appear several >>> times. >> >> This function returns all the entities in a pipe, weather they are >> streaming or not. For example, if only the RAW Capture 1 streams, or >> RGB/YUB capture streams, or both stream..in all three cases ved_pipeline >> will have the same entities, in exactly same order, and all will occur >> just once. >> Since media_graph_walk goes to each node of the graph, it returns back >> to the first one (as its a graph), hence the last condition, ie, >> >> if (!strcmp(entity->name, stream->ved_pipeline[0]->ent->name)) { >> >> makes sure that the first entity is not added again to the array. >> >> First condition, ie >> >> if (!strncmp(entity->name, "RGB", 3)) { >> >> Just makes sure that the search starts at RGB capture only. This is >> because, in case of any other video node, the order of entities, as you >> have mentioned later in the mail, will not be desirable, ie it won't >> start at sensor and end at captures. So this condition just takes care >> that all the entities in ved_pipeline array are in correct order >> (starting at sensor, ending at captures). > > It is better to compare to the whole name of the entity to make it more > clear. > Also I think it is better to document that this function is called only > upon the > first streaming node. If this doesn't end up refactored with other helpers, then indeed a few comments might help the readabilty here. The distinctions of each re-initialisation of the graph walk are hard to interpret the purpose. > > I still think this function should be independent of the topology. > Maybe you can use Helen's patch that allow walking a graph only opposite > to the link direction: https://patchwork.kernel.org/patch/11564899/ > This ensures that the Sensor will be first in the graph walk. Then the > streaming thread > iterates the ved_pipeline from 0 upwards and not from 'pipe_size' > downwards. Being able to use a direct helper to walk the pipeline cleanly sounds promising indeed. I suspect at some point int he next patches though - I'm going to see something that needs to have full visibility of all paths enabled from the sensor, as I think I recall that the thread will then process all (enabled) entities in a single pass. -- Kieran > > Thanks, > Dafna > > > >> >> Thanks, >> Kaaira >> >>> >>> I think there is no need to use the graph walk here but to access the >>> source entity >>> in each iteration, the way done in vimc_streamer_stream_start >>> also. >>> I think the code should iterate here until it reaches an entity that >>> is already streaming, >>> this means that the entity is already in the `ved_pipeline`, also you >>> should make sure >>> that the sensor is the first entity that process a frame, therefore >>> the sensor should be >>> at the end/start of the list of entities. Generally each entity >>> should appear exactly once >>> in the 'ved_pipeline' array and the entities should be ordered such >>> that when calling 'process_frame' >>> on one entity should be after calling 'process_frame' on its source >>> entity. >>> maybe it is easyer to implement if 'ved_pipeline' is a linked list. >>> >>> Thanks, >>> Dafna >>> >>>> vimc_streamer_pipeline_terminate(stream); >>>> @@ -138,8 +202,11 @@ static int vimc_streamer_thread(void *data) >>>> for (i = stream->pipe_size - 1; i >= 0; i--) { >>>> ved = stream->ved_pipeline[i]; >>>> - ret = ved->process_frame(ved); >>>> + if (atomic_read(&ved->use_count) == 0) >>>> + continue; >>>> + >>>> + ret = ved->process_frame(ved); >>>> if (ret) >>>> break; >>>> } >>>> @@ -179,6 +246,10 @@ int vimc_streamer_s_stream(struct vimc_stream >>>> *stream, >>>> if (stream->kthread) >>>> return 0; >>>> + ret = vimc_streamer_stream_start(stream, ved); >>>> + if (ret) >>>> + return ret; >>>> + >>>> ret = vimc_streamer_pipeline_init(stream, ved); >>>> if (ret) >>>> return ret; >>>>
Hi, Sorry for the late reply On Fri, Aug 28, 2020 at 10:37:14PM +0200, Dafna Hirschfeld wrote: > Hi, > > Am 21.08.20 um 23:01 schrieb Kaaira Gupta: > > Hi, > > > > On Fri, Aug 21, 2020 at 05:11:23PM +0200, Dafna Hirschfeld wrote: > > > > > > > > > Am 19.08.20 um 20:04 schrieb Kaaira Gupta: > > > > Separate the process of initialising pipeline array from starting > > > > streaming for all entities in path of a stream. This is needed because > > > > multiple streams can stream, but only one pipeline object is needed. > > > > > > > > Process frames only for those entities in a pipeline which are > > > > streaming. This is known through their use counts. > > > > > > > > Signed-off-by: Kaaira Gupta <kgupta@es.iitr.ac.in> > > > > --- > > > > .../media/test-drivers/vimc/vimc-streamer.c | 95 ++++++++++++++++--- > > > > 1 file changed, 83 insertions(+), 12 deletions(-) > > > > > > > > diff --git a/drivers/media/test-drivers/vimc/vimc-streamer.c b/drivers/media/test-drivers/vimc/vimc-streamer.c > > > > index c1644d69686d..cc40ecabe95a 100644 > > > > --- a/drivers/media/test-drivers/vimc/vimc-streamer.c > > > > +++ b/drivers/media/test-drivers/vimc/vimc-streamer.c > > > > @@ -40,33 +40,30 @@ static void vimc_streamer_pipeline_terminate(struct vimc_stream *stream) > > > > } > > > > /** > > > > - * vimc_streamer_pipeline_init - Initializes the stream structure > > > > + * vimc_streamer_stream_start - Starts streaming for all entities > > > > + * in a stream > > > > * > > > > - * @stream: the pointer to the stream structure to be initialized > > > > * @ved: the pointer to the vimc entity initializing the stream > > > > * > > > > - * Initializes the stream structure. Walks through the entity graph to > > > > - * construct the pipeline used later on the streamer thread. > > > > - * Calls vimc_streamer_s_stream() to enable stream in all entities of > > > > - * the pipeline. > > > > + * Walks through the entity graph to call vimc_streamer_s_stream() > > > > + * to enable stream in all entities in path of a stream. > > > > * > > > > * Return: 0 if success, error code otherwise. > > > > */ > > > > -static int vimc_streamer_pipeline_init(struct vimc_stream *stream, > > > > - struct vimc_ent_device *ved) > > > > +static int vimc_streamer_stream_start(struct vimc_stream *stream, > > > > + struct vimc_ent_device *ved) > > > > { > > > > struct media_entity *entity; > > > > struct video_device *vdev; > > > > struct v4l2_subdev *sd; > > > > + int stream_size = 0; > > > > int ret = 0; > > > > - stream->pipe_size = 0; > > > > - while (stream->pipe_size < VIMC_STREAMER_PIPELINE_MAX_SIZE) { > > > > + while (stream_size < VIMC_STREAMER_PIPELINE_MAX_SIZE) { > > > > if (!ved) { > > > > vimc_streamer_pipeline_terminate(stream); > > > > return -EINVAL; > > > > } > > > > - stream->ved_pipeline[stream->pipe_size++] = ved; > > > > if (is_media_entity_v4l2_subdev(ved->ent)) { > > > > sd = media_entity_to_v4l2_subdev(ved->ent); > > > > @@ -104,6 +101,73 @@ static int vimc_streamer_pipeline_init(struct vimc_stream *stream, > > > > entity); > > > > ved = video_get_drvdata(vdev); > > > > } > > > > + stream_size++; > > > > + } > > > > + > > > > + vimc_streamer_pipeline_terminate(stream); > > > > + return -EINVAL; > > > > +} > > > > + > > > > +/** > > > > + * vimc_streamer_pipeline_init - Initialises pipeline and pipe size > > > > + * > > > > + * @stream: the pointer to the stream structure > > > > + * @ved: the pointer to the vimc entity initializing the stream pipeline > > > > + * > > > > + * Walks through the entity graph to initialise ved_pipeline and updates > > > > + * pipe_size too. > > > > + * > > > > + * Return: 0 if success, error code otherwise. > > > > + */ > > > > +static int vimc_streamer_pipeline_init(struct vimc_stream *stream, > > > > + struct vimc_ent_device *ved) > > > > +{ > > > > + struct media_entity *entity; > > > > + struct media_device *mdev; > > > > + struct media_graph graph; > > > > + struct video_device *vdev; > > > > + struct v4l2_subdev *sd; > > > > + int ret; > > > > + > > > > + entity = ved->ent; > > > > + mdev = entity->graph_obj.mdev; > > > > + > > > > + ret = media_graph_walk_init(&graph, mdev); > > > > + if (ret) > > > > + return ret; > > > > + > > > > + media_graph_walk_start(&graph, entity); > > > > + > > > > + /* > > > > + * Start pipeline array initialisation from RAW Capture only to get > > > > + * entities in the correct order of their frame processing. > > > > + */ > > > > + if (!strncmp(entity->name, "RGB", 3)) { > > > > > > I don't understand this condition, way is it good for? > > > > I have explained that later in the reply > > > > > > > > I think the function should be generic and not assume names of entities > > > or specific topology. > > > > It doesn't assume the topology, rather it is in place just to make sure > > that the entities in ved_pipeline are in correct order. > > > > > > > > > > > > + entity = media_graph_walk_next(&graph); > > > > + mdev = entity->graph_obj.mdev; > > > > + media_graph_walk_cleanup(&graph); > > > > + > > > > + ret = media_graph_walk_init(&graph, mdev); > > > > + if (ret) > > > > + return ret; > > > > + media_graph_walk_start(&graph, entity); > > Hi, can you explain what this code does? > Why does it start the search in the next entity? Yes, so there can be two cases in the current topology; one, where this function is called by RAW capture, and other where it is called by RGB. So sinse walking the graph (as already implemented) is a DFS, due to the nature of the graph, when walking starts at RAW, it moves in this order (raw-rgb-scaler-debayer-sensor), which is also the order we want in ved_pipeline so that process_frame can be called in order..while when RGB calls it, the waling is in this order (rgb-raw-sensor-scaler-debayer). Hence, in case of RGB, we start the walking from the next entity (ie RAW) so that the order is correct again. I will put better comments for this in the code > > > > > + } > > > > + > > > > + while (stream->pipe_size < VIMC_STREAMER_PIPELINE_MAX_SIZE) { > > > > + if (is_media_entity_v4l2_subdev(entity)) { > > > > + sd = media_entity_to_v4l2_subdev(entity); > > > > + ved = v4l2_get_subdevdata(sd); > > > > + } else { > > > > + vdev = container_of(entity, struct video_device, entity); > > > > + ved = video_get_drvdata(vdev); > > > > + } > > > > + stream->ved_pipeline[stream->pipe_size++] = ved; > > > > + entity = media_graph_walk_next(&graph); > > > > + > > > > + if (!strcmp(entity->name, stream->ved_pipeline[0]->ent->name)) { > > > > > > I also don't understand this condition > > > > > > > + media_graph_walk_cleanup(&graph); > > > > + return 0; > > > > + } > > > > } > > > > > > It is not clear what this function does, it looks like it adds to 'ved_pipeline' > > > all entities that are connected to the video node, in addition to the entities > > > that where there from previous calls, so some entities appear several times. > > > > This function returns all the entities in a pipe, weather they are > > streaming or not. For example, if only the RAW Capture 1 streams, or > > RGB/YUB capture streams, or both stream..in all three cases ved_pipeline > > will have the same entities, in exactly same order, and all will occur just once. > > Since media_graph_walk goes to each node of the graph, it returns back > > to the first one (as its a graph), hence the last condition, ie, > > > > if (!strcmp(entity->name, stream->ved_pipeline[0]->ent->name)) { > > > > makes sure that the first entity is not added again to the array. > > > > First condition, ie > > > > if (!strncmp(entity->name, "RGB", 3)) { > > > > Just makes sure that the search starts at RGB capture only. This is > > because, in case of any other video node, the order of entities, as you > > have mentioned later in the mail, will not be desirable, ie it won't > > start at sensor and end at captures. So this condition just takes care > > that all the entities in ved_pipeline array are in correct order > > (starting at sensor, ending at captures). > > It is better to compare to the whole name of the entity to make it more clear. > Also I think it is better to document that this function is called only upon the > first streaming node. Agreed, I will add that. > > I still think this function should be independent of the topology. > Maybe you can use Helen's patch that allow walking a graph only opposite to the link direction: https://patchwork.kernel.org/patch/11564899/ > This ensures that the Sensor will be first in the graph walk. Then the streaming thread > iterates the ved_pipeline from 0 upwards and not from 'pipe_size' downwards. I didn't understand how this can be used. This moves forwards for one stream (from sensor to say rgb capture), but it this case, we need both the captures at the end. Also, this would need us to pass a sensor too, so it would require walking the graph twice, once from the current capture to sensor (to see whoch capture is connected), and then from the sensor forwards to the captures to add them to ved_pipeline, which I don;t think is a good way. Can you suggest some othet way? Or do you find iterating twice fine? If everyone agrees, I can change it Thanks for the review :D Kaaira > > Thanks, > Dafna > > > > > > > Thanks, > > Kaaira > > > > > > > > I think there is no need to use the graph walk here but to access the source entity > > > in each iteration, the way done in vimc_streamer_stream_start > > > also. > > > I think the code should iterate here until it reaches an entity that is already streaming, > > > this means that the entity is already in the `ved_pipeline`, also you should make sure > > > that the sensor is the first entity that process a frame, therefore the sensor should be > > > at the end/start of the list of entities. Generally each entity should appear exactly once > > > in the 'ved_pipeline' array and the entities should be ordered such that when calling 'process_frame' > > > on one entity should be after calling 'process_frame' on its source entity. > > > maybe it is easyer to implement if 'ved_pipeline' is a linked list. > > > > > > Thanks, > > > Dafna > > > > > > > vimc_streamer_pipeline_terminate(stream); > > > > @@ -138,8 +202,11 @@ static int vimc_streamer_thread(void *data) > > > > for (i = stream->pipe_size - 1; i >= 0; i--) { > > > > ved = stream->ved_pipeline[i]; > > > > - ret = ved->process_frame(ved); > > > > + if (atomic_read(&ved->use_count) == 0) > > > > + continue; > > > > + > > > > + ret = ved->process_frame(ved); > > > > if (ret) > > > > break; > > > > } > > > > @@ -179,6 +246,10 @@ int vimc_streamer_s_stream(struct vimc_stream *stream, > > > > if (stream->kthread) > > > > return 0; > > > > + ret = vimc_streamer_stream_start(stream, ved); > > > > + if (ret) > > > > + return ret; > > > > + > > > > ret = vimc_streamer_pipeline_init(stream, ved); > > > > if (ret) > > > > return ret; > > > >
On Wed, Sep 02, 2020 at 10:56:46AM +0100, Kieran Bingham wrote: > Hi Kaaira, Dafna, > > On 28/08/2020 21:37, Dafna Hirschfeld wrote: > > Hi, > > > > Am 21.08.20 um 23:01 schrieb Kaaira Gupta: > >> Hi, > >> > >> On Fri, Aug 21, 2020 at 05:11:23PM +0200, Dafna Hirschfeld wrote: > >>> > >>> > >>> Am 19.08.20 um 20:04 schrieb Kaaira Gupta: > >>>> Separate the process of initialising pipeline array from starting > >>>> streaming for all entities in path of a stream. This is needed because > >>>> multiple streams can stream, but only one pipeline object is needed. > >>>> > >>>> Process frames only for those entities in a pipeline which are > >>>> streaming. This is known through their use counts. > >>>> > >>>> Signed-off-by: Kaaira Gupta <kgupta@es.iitr.ac.in> > >>>> --- > >>>> .../media/test-drivers/vimc/vimc-streamer.c | 95 > >>>> ++++++++++++++++--- > >>>> 1 file changed, 83 insertions(+), 12 deletions(-) > >>>> > >>>> diff --git a/drivers/media/test-drivers/vimc/vimc-streamer.c > >>>> b/drivers/media/test-drivers/vimc/vimc-streamer.c > >>>> index c1644d69686d..cc40ecabe95a 100644 > >>>> --- a/drivers/media/test-drivers/vimc/vimc-streamer.c > >>>> +++ b/drivers/media/test-drivers/vimc/vimc-streamer.c > >>>> @@ -40,33 +40,30 @@ static void > >>>> vimc_streamer_pipeline_terminate(struct vimc_stream *stream) > >>>> } > >>>> /** > >>>> - * vimc_streamer_pipeline_init - Initializes the stream structure > >>>> + * vimc_streamer_stream_start - Starts streaming for all entities > >>>> + * in a stream > >>>> * > >>>> - * @stream: the pointer to the stream structure to be initialized > >>>> * @ved: the pointer to the vimc entity initializing the stream > >>>> * > >>>> - * Initializes the stream structure. Walks through the entity graph to > >>>> - * construct the pipeline used later on the streamer thread. > >>>> - * Calls vimc_streamer_s_stream() to enable stream in all entities of > >>>> - * the pipeline. > >>>> + * Walks through the entity graph to call vimc_streamer_s_stream() > >>>> + * to enable stream in all entities in path of a stream. > >>>> * > >>>> * Return: 0 if success, error code otherwise. > >>>> */ > >>>> -static int vimc_streamer_pipeline_init(struct vimc_stream *stream, > >>>> - struct vimc_ent_device *ved) > >>>> +static int vimc_streamer_stream_start(struct vimc_stream *stream, > >>>> + struct vimc_ent_device *ved) > >>>> { > >>>> struct media_entity *entity; > >>>> struct video_device *vdev; > >>>> struct v4l2_subdev *sd; > >>>> + int stream_size = 0; > >>>> int ret = 0; > >>>> - stream->pipe_size = 0; > >>>> - while (stream->pipe_size < VIMC_STREAMER_PIPELINE_MAX_SIZE) { > >>>> + while (stream_size < VIMC_STREAMER_PIPELINE_MAX_SIZE) { > >>>> if (!ved) { > >>>> vimc_streamer_pipeline_terminate(stream); > >>>> return -EINVAL; > >>>> } > >>>> - stream->ved_pipeline[stream->pipe_size++] = ved; > >>>> if (is_media_entity_v4l2_subdev(ved->ent)) { > >>>> sd = media_entity_to_v4l2_subdev(ved->ent); > >>>> @@ -104,6 +101,73 @@ static int vimc_streamer_pipeline_init(struct > >>>> vimc_stream *stream, > >>>> entity); > >>>> ved = video_get_drvdata(vdev); > >>>> } > >>>> + stream_size++; > >>>> + } > >>>> + > >>>> + vimc_streamer_pipeline_terminate(stream); > >>>> + return -EINVAL; > >>>> +} > >>>> + > >>>> +/** > >>>> + * vimc_streamer_pipeline_init - Initialises pipeline and pipe size > >>>> + * > >>>> + * @stream: the pointer to the stream structure > >>>> + * @ved: the pointer to the vimc entity initializing the stream > >>>> pipeline > > Which entity is this? Is it the start, or the end of the pipeline? I.e. > the sensor? or the capture ? It is the capture, I will add it to the documentation..thanks > > >>>> + * > >>>> + * Walks through the entity graph to initialise ved_pipeline and > >>>> updates > >>>> + * pipe_size too. > >>>> + * > >>>> + * Return: 0 if success, error code otherwise. > >>>> + */ > >>>> +static int vimc_streamer_pipeline_init(struct vimc_stream *stream, > >>>> + struct vimc_ent_device *ved) > >>>> +{ > >>>> + struct media_entity *entity; > >>>> + struct media_device *mdev; > >>>> + struct media_graph graph; > >>>> + struct video_device *vdev; > >>>> + struct v4l2_subdev *sd; > >>>> + int ret; > >>>> + > >>>> + entity = ved->ent; > >>>> + mdev = entity->graph_obj.mdev; > >>>> + > >>>> + ret = media_graph_walk_init(&graph, mdev); > >>>> + if (ret) > >>>> + return ret; > >>>> + > >>>> + media_graph_walk_start(&graph, entity); > >>>> + > >>>> + /* > >>>> + * Start pipeline array initialisation from RAW Capture only to > >>>> get > >>>> + * entities in the correct order of their frame processing. > >>>> + */ > >>>> + if (!strncmp(entity->name, "RGB", 3)) { > >>> > >>> I don't understand this condition, way is it good for? > >> > >> I have explained that later in the reply > > Matching on entity names is a bit awkward, as they could be changed I > guess quite easily, and easily missed in this string matching. Agreed, I need to think of a better way to prevent this > > I haven't fully understood this code block yet to work out if there's > another way we could do this though, but reading ahead I see there might > be a way to 'walk the graph' on a per-stream basis which might be a good > way of factoring this path. > > Although there still needs to be a construction of the paths available > to a stream which splits from the sensor. > > > >> > >>> > >>> I think the function should be generic and not assume names of entities > >>> or specific topology. > >> > >> It doesn't assume the topology, rather it is in place just to make sure > >> that the entities in ved_pipeline are in correct order. > >> > >>> > >>> > >>>> + entity = media_graph_walk_next(&graph); > >>>> + mdev = entity->graph_obj.mdev; > >>>> + media_graph_walk_cleanup(&graph); > >>>> + > >>>> + ret = media_graph_walk_init(&graph, mdev); > >>>> + if (ret) > >>>> + return ret; > >>>> + media_graph_walk_start(&graph, entity); > > > > Hi, can you explain what this code does? > > Why does it start the search in the next entity? > > > >>>> + } > >>>> + > >>>> + while (stream->pipe_size < VIMC_STREAMER_PIPELINE_MAX_SIZE) { > >>>> + if (is_media_entity_v4l2_subdev(entity)) { > >>>> + sd = media_entity_to_v4l2_subdev(entity); > >>>> + ved = v4l2_get_subdevdata(sd); > >>>> + } else { > >>>> + vdev = container_of(entity, struct video_device, entity); > >>>> + ved = video_get_drvdata(vdev); > >>>> + } > >>>> + stream->ved_pipeline[stream->pipe_size++] = ved; > >>>> + entity = media_graph_walk_next(&graph); > >>>> + > >>>> + if (!strcmp(entity->name, > >>>> stream->ved_pipeline[0]->ent->name)) { > >>> > >>> I also don't understand this condition > >>> > >>>> + media_graph_walk_cleanup(&graph); > >>>> + return 0; > >>>> + } > >>>> } > >>> > >>> It is not clear what this function does, it looks like it adds to > >>> 'ved_pipeline' > >>> all entities that are connected to the video node, in addition to the > >>> entities > >>> that where there from previous calls, so some entities appear several > >>> times. > >> > >> This function returns all the entities in a pipe, weather they are > >> streaming or not. For example, if only the RAW Capture 1 streams, or > >> RGB/YUB capture streams, or both stream..in all three cases ved_pipeline > >> will have the same entities, in exactly same order, and all will occur > >> just once. > >> Since media_graph_walk goes to each node of the graph, it returns back > >> to the first one (as its a graph), hence the last condition, ie, > >> > >> if (!strcmp(entity->name, stream->ved_pipeline[0]->ent->name)) { > >> > >> makes sure that the first entity is not added again to the array. > >> > >> First condition, ie > >> > >> if (!strncmp(entity->name, "RGB", 3)) { > >> > >> Just makes sure that the search starts at RGB capture only. This is > >> because, in case of any other video node, the order of entities, as you > >> have mentioned later in the mail, will not be desirable, ie it won't > >> start at sensor and end at captures. So this condition just takes care > >> that all the entities in ved_pipeline array are in correct order > >> (starting at sensor, ending at captures). > > > > It is better to compare to the whole name of the entity to make it more > > clear. > > Also I think it is better to document that this function is called only > > upon the > > first streaming node. > > If this doesn't end up refactored with other helpers, then indeed a few > comments might help the readabilty here. The distinctions of each > re-initialisation of the graph walk are hard to interpret the purpose. > > > > > > > I still think this function should be independent of the topology. > > Maybe you can use Helen's patch that allow walking a graph only opposite > > to the link direction: https://patchwork.kernel.org/patch/11564899/ > > This ensures that the Sensor will be first in the graph walk. Then the > > streaming thread > > iterates the ved_pipeline from 0 upwards and not from 'pipe_size' > > downwards. > > Being able to use a direct helper to walk the pipeline cleanly sounds > promising indeed. > > I suspect at some point int he next patches though - I'm going to see > something that needs to have full visibility of all paths enabled from > the sensor, as I think I recall that the thread will then process all > (enabled) entities in a single pass. Yes, that exactly is the problem :( This helper (that dafna has shared) can walk through one path, while the thread (which processes the frames) needs to view all the entites in all connected paths > -- > Kieran > > > > > > Thanks, > > Dafna > > > > > > > >> > >> Thanks, > >> Kaaira > >> > >>> > >>> I think there is no need to use the graph walk here but to access the > >>> source entity > >>> in each iteration, the way done in vimc_streamer_stream_start > >>> also. > >>> I think the code should iterate here until it reaches an entity that > >>> is already streaming, > >>> this means that the entity is already in the `ved_pipeline`, also you > >>> should make sure > >>> that the sensor is the first entity that process a frame, therefore > >>> the sensor should be > >>> at the end/start of the list of entities. Generally each entity > >>> should appear exactly once > >>> in the 'ved_pipeline' array and the entities should be ordered such > >>> that when calling 'process_frame' > >>> on one entity should be after calling 'process_frame' on its source > >>> entity. > >>> maybe it is easyer to implement if 'ved_pipeline' is a linked list. > >>> > >>> Thanks, > >>> Dafna > >>> > >>>> vimc_streamer_pipeline_terminate(stream); > >>>> @@ -138,8 +202,11 @@ static int vimc_streamer_thread(void *data) > >>>> for (i = stream->pipe_size - 1; i >= 0; i--) { > >>>> ved = stream->ved_pipeline[i]; > >>>> - ret = ved->process_frame(ved); > >>>> + if (atomic_read(&ved->use_count) == 0) > >>>> + continue; > >>>> + > >>>> + ret = ved->process_frame(ved); > >>>> if (ret) > >>>> break; > >>>> } > >>>> @@ -179,6 +246,10 @@ int vimc_streamer_s_stream(struct vimc_stream > >>>> *stream, > >>>> if (stream->kthread) > >>>> return 0; > >>>> + ret = vimc_streamer_stream_start(stream, ved); > >>>> + if (ret) > >>>> + return ret; > >>>> + > >>>> ret = vimc_streamer_pipeline_init(stream, ved); > >>>> if (ret) > >>>> return ret; > >>>> > > -- > Regards > -- > Kieran
diff --git a/drivers/media/test-drivers/vimc/vimc-streamer.c b/drivers/media/test-drivers/vimc/vimc-streamer.c index c1644d69686d..cc40ecabe95a 100644 --- a/drivers/media/test-drivers/vimc/vimc-streamer.c +++ b/drivers/media/test-drivers/vimc/vimc-streamer.c @@ -40,33 +40,30 @@ static void vimc_streamer_pipeline_terminate(struct vimc_stream *stream) } /** - * vimc_streamer_pipeline_init - Initializes the stream structure + * vimc_streamer_stream_start - Starts streaming for all entities + * in a stream * - * @stream: the pointer to the stream structure to be initialized * @ved: the pointer to the vimc entity initializing the stream * - * Initializes the stream structure. Walks through the entity graph to - * construct the pipeline used later on the streamer thread. - * Calls vimc_streamer_s_stream() to enable stream in all entities of - * the pipeline. + * Walks through the entity graph to call vimc_streamer_s_stream() + * to enable stream in all entities in path of a stream. * * Return: 0 if success, error code otherwise. */ -static int vimc_streamer_pipeline_init(struct vimc_stream *stream, - struct vimc_ent_device *ved) +static int vimc_streamer_stream_start(struct vimc_stream *stream, + struct vimc_ent_device *ved) { struct media_entity *entity; struct video_device *vdev; struct v4l2_subdev *sd; + int stream_size = 0; int ret = 0; - stream->pipe_size = 0; - while (stream->pipe_size < VIMC_STREAMER_PIPELINE_MAX_SIZE) { + while (stream_size < VIMC_STREAMER_PIPELINE_MAX_SIZE) { if (!ved) { vimc_streamer_pipeline_terminate(stream); return -EINVAL; } - stream->ved_pipeline[stream->pipe_size++] = ved; if (is_media_entity_v4l2_subdev(ved->ent)) { sd = media_entity_to_v4l2_subdev(ved->ent); @@ -104,6 +101,73 @@ static int vimc_streamer_pipeline_init(struct vimc_stream *stream, entity); ved = video_get_drvdata(vdev); } + stream_size++; + } + + vimc_streamer_pipeline_terminate(stream); + return -EINVAL; +} + +/** + * vimc_streamer_pipeline_init - Initialises pipeline and pipe size + * + * @stream: the pointer to the stream structure + * @ved: the pointer to the vimc entity initializing the stream pipeline + * + * Walks through the entity graph to initialise ved_pipeline and updates + * pipe_size too. + * + * Return: 0 if success, error code otherwise. + */ +static int vimc_streamer_pipeline_init(struct vimc_stream *stream, + struct vimc_ent_device *ved) +{ + struct media_entity *entity; + struct media_device *mdev; + struct media_graph graph; + struct video_device *vdev; + struct v4l2_subdev *sd; + int ret; + + entity = ved->ent; + mdev = entity->graph_obj.mdev; + + ret = media_graph_walk_init(&graph, mdev); + if (ret) + return ret; + + media_graph_walk_start(&graph, entity); + + /* + * Start pipeline array initialisation from RAW Capture only to get + * entities in the correct order of their frame processing. + */ + if (!strncmp(entity->name, "RGB", 3)) { + entity = media_graph_walk_next(&graph); + mdev = entity->graph_obj.mdev; + media_graph_walk_cleanup(&graph); + + ret = media_graph_walk_init(&graph, mdev); + if (ret) + return ret; + media_graph_walk_start(&graph, entity); + } + + while (stream->pipe_size < VIMC_STREAMER_PIPELINE_MAX_SIZE) { + if (is_media_entity_v4l2_subdev(entity)) { + sd = media_entity_to_v4l2_subdev(entity); + ved = v4l2_get_subdevdata(sd); + } else { + vdev = container_of(entity, struct video_device, entity); + ved = video_get_drvdata(vdev); + } + stream->ved_pipeline[stream->pipe_size++] = ved; + entity = media_graph_walk_next(&graph); + + if (!strcmp(entity->name, stream->ved_pipeline[0]->ent->name)) { + media_graph_walk_cleanup(&graph); + return 0; + } } vimc_streamer_pipeline_terminate(stream); @@ -138,8 +202,11 @@ static int vimc_streamer_thread(void *data) for (i = stream->pipe_size - 1; i >= 0; i--) { ved = stream->ved_pipeline[i]; - ret = ved->process_frame(ved); + if (atomic_read(&ved->use_count) == 0) + continue; + + ret = ved->process_frame(ved); if (ret) break; } @@ -179,6 +246,10 @@ int vimc_streamer_s_stream(struct vimc_stream *stream, if (stream->kthread) return 0; + ret = vimc_streamer_stream_start(stream, ved); + if (ret) + return ret; + ret = vimc_streamer_pipeline_init(stream, ved); if (ret) return ret;
Separate the process of initialising pipeline array from starting streaming for all entities in path of a stream. This is needed because multiple streams can stream, but only one pipeline object is needed. Process frames only for those entities in a pipeline which are streaming. This is known through their use counts. Signed-off-by: Kaaira Gupta <kgupta@es.iitr.ac.in> --- .../media/test-drivers/vimc/vimc-streamer.c | 95 ++++++++++++++++--- 1 file changed, 83 insertions(+), 12 deletions(-)