On 06/09/2022 11:48, Laurent Pinchart wrote:
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 ?
That doesn't sound correct to me, especially if the only reason to
create the pipeline in such a way would be to accommodate two old drivers.
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.
Only omap4iss and omap3isdp use this function now. I wonder if we should
just drop the function and add a private functions for both drivers. The
question remains, of course, what the function should do. I think
returning the first non-NULL pipe should work (which is what the func
does in v14).
Tomi
