Hi Tomi,
On Tue, Aug 30, 2022 at 03:40:30PM +0300, Tomi Valkeinen wrote:
> On 30/08/2022 14:44, Tomi Valkeinen wrote:
>
> >>> @@ -966,10 +1318,29 @@ EXPORT_SYMBOL_GPL(media_entity_get_fwnode_pad);
> >>> struct media_pipeline *media_entity_pipeline(struct media_entity
> >>> *entity)
> >>> {
> >>> - return entity->pipe;
> >>> + struct media_pipeline *pipe;
> >>> + struct media_pad *pad;
> >>> +
> >>> + if (entity->num_pads == 0)
> >>> + return NULL;
> >>> +
> >>> + pipe = entity->pads->pipe;
> >>> +
> >>> + media_entity_for_each_pad(entity, pad) {
> >>> + if (WARN_ON(pad->pipe != pipe))
> >>> + return NULL;
> >>> + }
> >>
> >> This wasn't done in v12, could you explain the rationale ? The
> >> documentation of media_entity_pipeline() should probably be updated too.
> >
> > This is just a sanity check.
> >
> > The doc says "This function is to be used only for entities that do not
> > support multiple pipelines". So the pipeline must be the same for all
> > pads, and if that's not the case, something is horribly wrong.
> >
> > However, now that I think about it, can we have NULL pipes in some pads,
> > if those pads are e.g. not linked... Then I think the check should be
> > (pipe && pad->pipe && pipe != pad->pipe).
> >
> > But if that's the case, does this even work... The code expects the
> > first pad to contain the pipeline for the entity. If the first pad's
> > pipe can be NULL, but, say, second and fourth pads contain the pipeline
> > (as those pads are actually used), then this is quite broken.
>
> Looks like my concern has some grounds. While I don't have a perfect
> setup to test this, I think my test with UB960 shows that unlinked pads
> have NULL pipeline. So if we have an entity with two sink pads (0 and 1)
> and two source pads (2 and 3), and the pipeline goes through pads 1 and
> 3, this function will always return NULL.
>
> I'm not sure how to fix this. One option would be to make this function
> available only for entities with 1 or 2 (a sink and a source) pads. But
> I have no idea if that covers the current users.
I fear that could introduce regressions silently.
> Or should this function iterate through the pads and return the first
> non-null pipe?
Another option is to make sure all pads are included in the pipeline,
even if they're not linked. Have you checked why that's not the case ?
> Looks like there are only a few users for this. The one in
> imx-media-utils.c can be dropped easily. omap4iss and omap3isp uses are
> not clear to me yet.
--
Regards,
Laurent Pinchart
