Re: [PATCH v1 5/5] media: xilinx: dma: Use media_pipeline_for_each_pad()

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

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.

-- 
Regards,

Laurent Pinchart