Re: [PATCH v13 14/34] media: mc: entity: Rewrite media_pipeline_start()

From: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
To: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Cc: linux-media@vger.kernel.org, sakari.ailus@linux.intel.com,
	Jacopo Mondi <jacopo+renesas@jmondi.org>,
	niklas.soderlund+renesas@ragnatech.se,
	Mauro Carvalho Chehab <mchehab@kernel.org>,
	Hans Verkuil <hverkuil-cisco@xs4all.nl>,
	Pratyush Yadav <p.yadav@ti.com>,
	Kishon Vijay Abraham <kishon@ti.com>,
	satish.nagireddy@getcruise.com, Tomasz Figa <tfiga@chromium.org>
Subject: Re: [PATCH v13 14/34] media: mc: entity: Rewrite media_pipeline_start()
Date: Tue, 30 Aug 2022 15:40:30 +0300	[thread overview]
Message-ID: <fbf717fc-ecb7-ddc1-096f-f2bcd93a25e3@ideasonboard.com> (raw)
In-Reply-To: <51d2f837-6234-eb66-c1cc-a12a9e51c136@ideasonboard.com>

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.

Or should this function iterate through the pads and return the first 
non-null pipe?

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.

  Tomi