| Message ID | 20221212141621.724-6-laurent.pinchart@ideasonboard.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | media: Replace media graph walk in several drivers | expand |
On 12/12/2022 16:16, Laurent Pinchart wrote: > Replace usage of the deprecated media graph walk API with the new > media_pipeline_for_each_pad() macro. > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > --- > drivers/media/platform/xilinx/xilinx-dma.c | 28 +++++----------------- > 1 file changed, 6 insertions(+), 22 deletions(-) > > diff --git a/drivers/media/platform/xilinx/xilinx-dma.c b/drivers/media/platform/xilinx/xilinx-dma.c > index 0a7fd8642a65..fee02c8c85fd 100644 > --- a/drivers/media/platform/xilinx/xilinx-dma.c > +++ b/drivers/media/platform/xilinx/xilinx-dma.c > @@ -173,31 +173,19 @@ static int xvip_pipeline_set_stream(struct xvip_pipeline *pipe, bool on) > static int xvip_pipeline_validate(struct xvip_pipeline *pipe, > struct xvip_dma *start) > { > - struct media_graph graph; > - struct media_entity *entity = &start->video.entity; > - struct media_device *mdev = entity->graph_obj.mdev; > + struct media_pipeline_pad_iter iter; > unsigned int num_inputs = 0; > unsigned int num_outputs = 0; > - int ret; > + struct media_pad *pad; > > - mutex_lock(&mdev->graph_mutex); > - > - /* Walk the graph to locate the video nodes. */ > - ret = media_graph_walk_init(&graph, mdev); > - if (ret) { > - mutex_unlock(&mdev->graph_mutex); > - return ret; > - } > - > - media_graph_walk_start(&graph, entity); > - > - while ((entity = media_graph_walk_next(&graph))) { > + /* Locate the video nodes in the pipeline. */ > + media_pipeline_for_each_pad(&pipe->pipe, &iter, pad) { > struct xvip_dma *dma; > > - if (entity->function != MEDIA_ENT_F_IO_V4L) > + if (pad->entity->function != MEDIA_ENT_F_IO_V4L) > continue; Why do you iterate the pads here, instead of using media_pipeline_for_each_entity()? Tomi
Hi Tomi, On Thu, Dec 15, 2022 at 03:29:13PM +0200, Tomi Valkeinen wrote: > On 12/12/2022 16:16, Laurent Pinchart wrote: > > Replace usage of the deprecated media graph walk API with the new > > media_pipeline_for_each_pad() macro. > > > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > --- > > drivers/media/platform/xilinx/xilinx-dma.c | 28 +++++----------------- > > 1 file changed, 6 insertions(+), 22 deletions(-) > > > > diff --git a/drivers/media/platform/xilinx/xilinx-dma.c b/drivers/media/platform/xilinx/xilinx-dma.c > > index 0a7fd8642a65..fee02c8c85fd 100644 > > --- a/drivers/media/platform/xilinx/xilinx-dma.c > > +++ b/drivers/media/platform/xilinx/xilinx-dma.c > > @@ -173,31 +173,19 @@ static int xvip_pipeline_set_stream(struct xvip_pipeline *pipe, bool on) > > static int xvip_pipeline_validate(struct xvip_pipeline *pipe, > > struct xvip_dma *start) > > { > > - struct media_graph graph; > > - struct media_entity *entity = &start->video.entity; > > - struct media_device *mdev = entity->graph_obj.mdev; > > + struct media_pipeline_pad_iter iter; > > unsigned int num_inputs = 0; > > unsigned int num_outputs = 0; > > - int ret; > > + struct media_pad *pad; > > > > - mutex_lock(&mdev->graph_mutex); > > - > > - /* Walk the graph to locate the video nodes. */ > > - ret = media_graph_walk_init(&graph, mdev); > > - if (ret) { > > - mutex_unlock(&mdev->graph_mutex); > > - return ret; > > - } > > - > > - media_graph_walk_start(&graph, entity); > > - > > - while ((entity = media_graph_walk_next(&graph))) { > > + /* Locate the video nodes in the pipeline. */ > > + media_pipeline_for_each_pad(&pipe->pipe, &iter, pad) { > > struct xvip_dma *dma; > > > > - if (entity->function != MEDIA_ENT_F_IO_V4L) > > + if (pad->entity->function != MEDIA_ENT_F_IO_V4L) > > continue; > > Why do you iterate the pads here, instead of using > media_pipeline_for_each_entity()? The entity iterator still iterates over all pads internally, so it's not more efficient. It also requires explicit initialization and cleanup, so is more complicated to use. In this specific case, the driver ignores all pads belonging to entities that are not V4L2 video nodes (MEDIA_ENT_F_IO_V4L). As such entities have a single pad, there's a guarantee they will not be visited twice. If you think this is too fragile due to the assumption that MEDIA_ENT_F_IO_V4L entities have a single pad, I can use the entity iterator, it's not a big deal.
On 16/12/2022 02:36, Laurent Pinchart wrote: > Hi Tomi, > > On Thu, Dec 15, 2022 at 03:29:13PM +0200, Tomi Valkeinen wrote: >> On 12/12/2022 16:16, Laurent Pinchart wrote: >>> Replace usage of the deprecated media graph walk API with the new >>> media_pipeline_for_each_pad() macro. >>> >>> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> >>> --- >>> drivers/media/platform/xilinx/xilinx-dma.c | 28 +++++----------------- >>> 1 file changed, 6 insertions(+), 22 deletions(-) >>> >>> diff --git a/drivers/media/platform/xilinx/xilinx-dma.c b/drivers/media/platform/xilinx/xilinx-dma.c >>> index 0a7fd8642a65..fee02c8c85fd 100644 >>> --- a/drivers/media/platform/xilinx/xilinx-dma.c >>> +++ b/drivers/media/platform/xilinx/xilinx-dma.c >>> @@ -173,31 +173,19 @@ static int xvip_pipeline_set_stream(struct xvip_pipeline *pipe, bool on) >>> static int xvip_pipeline_validate(struct xvip_pipeline *pipe, >>> struct xvip_dma *start) >>> { >>> - struct media_graph graph; >>> - struct media_entity *entity = &start->video.entity; >>> - struct media_device *mdev = entity->graph_obj.mdev; >>> + struct media_pipeline_pad_iter iter; >>> unsigned int num_inputs = 0; >>> unsigned int num_outputs = 0; >>> - int ret; >>> + struct media_pad *pad; >>> >>> - mutex_lock(&mdev->graph_mutex); >>> - >>> - /* Walk the graph to locate the video nodes. */ >>> - ret = media_graph_walk_init(&graph, mdev); >>> - if (ret) { >>> - mutex_unlock(&mdev->graph_mutex); >>> - return ret; >>> - } >>> - >>> - media_graph_walk_start(&graph, entity); >>> - >>> - while ((entity = media_graph_walk_next(&graph))) { >>> + /* Locate the video nodes in the pipeline. */ >>> + media_pipeline_for_each_pad(&pipe->pipe, &iter, pad) { >>> struct xvip_dma *dma; >>> >>> - if (entity->function != MEDIA_ENT_F_IO_V4L) >>> + if (pad->entity->function != MEDIA_ENT_F_IO_V4L) >>> continue; >> >> Why do you iterate the pads here, instead of using >> media_pipeline_for_each_entity()? > > The entity iterator still iterates over all pads internally, so it's not > more efficient. It also requires explicit initialization and cleanup, so > is more complicated to use. In this specific case, the driver ignores > all pads belonging to entities that are not V4L2 video nodes > (MEDIA_ENT_F_IO_V4L). As such entities have a single pad, there's a > guarantee they will not be visited twice. > > If you think this is too fragile due to the assumption that > MEDIA_ENT_F_IO_V4L entities have a single pad, I can use the entity > iterator, it's not a big deal. Well, the reason I commented was that the old code iterates entities, in this series you add a new way to iterate entities, and then... you don't use it for iterating entities =). I think it hints that the entity iteration is not as good as one could wish, but I don't right away see the means to improve it. I do miss C++ in cases like this. I think I would have gone with the entity iterator even if it's slightly more complex to use, just to highlight the point that this is iterating entities. But I don't mind either way. Tomi
Hi Tomi, On Fri, Dec 16, 2022 at 08:47:13AM +0200, Tomi Valkeinen wrote: > On 16/12/2022 02:36, Laurent Pinchart wrote: > > On Thu, Dec 15, 2022 at 03:29:13PM +0200, Tomi Valkeinen wrote: > >> On 12/12/2022 16:16, Laurent Pinchart wrote: > >>> Replace usage of the deprecated media graph walk API with the new > >>> media_pipeline_for_each_pad() macro. > >>> > >>> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > >>> --- > >>> drivers/media/platform/xilinx/xilinx-dma.c | 28 +++++----------------- > >>> 1 file changed, 6 insertions(+), 22 deletions(-) > >>> > >>> diff --git a/drivers/media/platform/xilinx/xilinx-dma.c b/drivers/media/platform/xilinx/xilinx-dma.c > >>> index 0a7fd8642a65..fee02c8c85fd 100644 > >>> --- a/drivers/media/platform/xilinx/xilinx-dma.c > >>> +++ b/drivers/media/platform/xilinx/xilinx-dma.c > >>> @@ -173,31 +173,19 @@ static int xvip_pipeline_set_stream(struct xvip_pipeline *pipe, bool on) > >>> static int xvip_pipeline_validate(struct xvip_pipeline *pipe, > >>> struct xvip_dma *start) > >>> { > >>> - struct media_graph graph; > >>> - struct media_entity *entity = &start->video.entity; > >>> - struct media_device *mdev = entity->graph_obj.mdev; > >>> + struct media_pipeline_pad_iter iter; > >>> unsigned int num_inputs = 0; > >>> unsigned int num_outputs = 0; > >>> - int ret; > >>> + struct media_pad *pad; > >>> > >>> - mutex_lock(&mdev->graph_mutex); > >>> - > >>> - /* Walk the graph to locate the video nodes. */ > >>> - ret = media_graph_walk_init(&graph, mdev); > >>> - if (ret) { > >>> - mutex_unlock(&mdev->graph_mutex); > >>> - return ret; > >>> - } > >>> - > >>> - media_graph_walk_start(&graph, entity); > >>> - > >>> - while ((entity = media_graph_walk_next(&graph))) { > >>> + /* Locate the video nodes in the pipeline. */ > >>> + media_pipeline_for_each_pad(&pipe->pipe, &iter, pad) { > >>> struct xvip_dma *dma; > >>> > >>> - if (entity->function != MEDIA_ENT_F_IO_V4L) > >>> + if (pad->entity->function != MEDIA_ENT_F_IO_V4L) > >>> continue; > >> > >> Why do you iterate the pads here, instead of using > >> media_pipeline_for_each_entity()? > > > > The entity iterator still iterates over all pads internally, so it's not > > more efficient. It also requires explicit initialization and cleanup, so > > is more complicated to use. In this specific case, the driver ignores > > all pads belonging to entities that are not V4L2 video nodes > > (MEDIA_ENT_F_IO_V4L). As such entities have a single pad, there's a > > guarantee they will not be visited twice. > > > > If you think this is too fragile due to the assumption that > > MEDIA_ENT_F_IO_V4L entities have a single pad, I can use the entity > > iterator, it's not a big deal. > > Well, the reason I commented was that the old code iterates entities, in > this series you add a new way to iterate entities, and then... you don't > use it for iterating entities =). > > I think it hints that the entity iteration is not as good as one could > wish, but I don't right away see the means to improve it. I do miss C++ > in cases like this. Indeed. In this specific case, it would be possible to initialize the iterator on first use, but not to destroy it automatically if the code breaks from the loop. There's something else I've been thinking about: we could cache the list of entities in the media_pipeline structure when starting the pipeline, to facilitate iteration. The entity iterator API would become simpler (and more efficient), but pipeline start would be more costly. Any opinion ? > I think I would have gone with the entity iterator even if it's slightly > more complex to use, just to highlight the point that this is iterating > entities. But I don't mind either way.
diff --git a/drivers/media/platform/xilinx/xilinx-dma.c b/drivers/media/platform/xilinx/xilinx-dma.c index 0a7fd8642a65..fee02c8c85fd 100644 --- a/drivers/media/platform/xilinx/xilinx-dma.c +++ b/drivers/media/platform/xilinx/xilinx-dma.c @@ -173,31 +173,19 @@ static int xvip_pipeline_set_stream(struct xvip_pipeline *pipe, bool on) static int xvip_pipeline_validate(struct xvip_pipeline *pipe, struct xvip_dma *start) { - struct media_graph graph; - struct media_entity *entity = &start->video.entity; - struct media_device *mdev = entity->graph_obj.mdev; + struct media_pipeline_pad_iter iter; unsigned int num_inputs = 0; unsigned int num_outputs = 0; - int ret; + struct media_pad *pad; - mutex_lock(&mdev->graph_mutex); - - /* Walk the graph to locate the video nodes. */ - ret = media_graph_walk_init(&graph, mdev); - if (ret) { - mutex_unlock(&mdev->graph_mutex); - return ret; - } - - media_graph_walk_start(&graph, entity); - - while ((entity = media_graph_walk_next(&graph))) { + /* Locate the video nodes in the pipeline. */ + media_pipeline_for_each_pad(&pipe->pipe, &iter, pad) { struct xvip_dma *dma; - if (entity->function != MEDIA_ENT_F_IO_V4L) + if (pad->entity->function != MEDIA_ENT_F_IO_V4L) continue; - dma = to_xvip_dma(media_entity_to_video_device(entity)); + dma = to_xvip_dma(media_entity_to_video_device(pad->entity)); if (dma->pad.flags & MEDIA_PAD_FL_SINK) { pipe->output = dma; @@ -207,10 +195,6 @@ static int xvip_pipeline_validate(struct xvip_pipeline *pipe, } } - mutex_unlock(&mdev->graph_mutex); - - media_graph_walk_cleanup(&graph); - /* We need exactly one output and zero or one input. */ if (num_outputs != 1 || num_inputs > 1) return -EPIPE;
Replace usage of the deprecated media graph walk API with the new media_pipeline_for_each_pad() macro. Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> --- drivers/media/platform/xilinx/xilinx-dma.c | 28 +++++----------------- 1 file changed, 6 insertions(+), 22 deletions(-)