On 30/07/2022 14:56, Sakari Ailus wrote:
Moi,
On Fri, Jul 29, 2022 at 08:07:19PM +0300, Tomi Valkeinen wrote:
On 29/07/2022 20:00, Satish Nagireddy wrote:
On Fri, Jul 29, 2022 at 3:27 AM Tomi Valkeinen
<tomi.valkeinen@xxxxxxxxxxxxxxxx> wrote:
On 29/07/2022 12:19, Satish Nagireddy wrote:
On Fri, Jul 29, 2022 at 1:53 AM Tomi Valkeinen
<tomi.valkeinen@xxxxxxxxxxxxxxxx> wrote:
On 29/07/2022 11:45, Satish Nagireddy wrote:
@@ -1011,7 +1342,7 @@ EXPORT_SYMBOL_GPL(media_entity_get_fwnode_pad);
struct media_pipeline *media_entity_pipeline(struct media_entity *entity)
{
- return entity->pipe;
+ return entity->pads->pipe;
I am not sure If it is always safe to return the pipe associated with
the first pad. I think this will work with all the existing drivers.
Let's say If pads of an entity are associated with different pipes,
this function might require extending the support of returning
pipe based on pad index. Please let me know your opinion.
That's true. The kdoc for this function says:
* In general, entities can be part of multiple pipelines, when carrying
* multiple streams (either on different pads, or on the same pad using
* multiplexed streams). This function is ill-defined in that case. It
* currently returns the pipeline associated with the first pad of the
entity.
I did consider adding a warning if the function is called for entities
with more than one pad. But that probably would give false warnings,
e.g. for a simple entity with one sink and one source pad. In that case
both pads are always part of the same pipeline, and
media_entity_pipeline() works correctly.
We could perhaps add a check here which verifies that all the pads in
the entity have the same pipe.
Perhaps something like:
struct media_pipeline *media_entity_pipeline(struct media_entity *entity)
{
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;
}
return pipe;
}
The above code means that we do not support multiple pipelines per
entity. Or leave the implementation as is now and
this can be done as a different patch later, I will leave it to you.
He is what I'm thinking, assuming that every media_pad has it's own pipe.
struct media_pipeline *media_entity_pipeline(struct media_entity
*entity, u32 pad_index)
{
if (pad_index >= entity->num_pads)
return NULL;
return entity->pads[pad_index].pipe;
}
Oh, I see now where the confusion is.
media_entity_pipeline() is a convenience helper for the currently existing
drivers. They access entity->pipe directly (or when we move the pipe to pad,
entity->pads->pipe). So this function is just refactoring the direct access
away from the drivers, which makes it easier in the future to find those
drivers accessing the pipe, or change where the pipe is stored without doing
any changes to the drivers.
A driver which supports streams should not use this function, as there's no
"pipe for entity". We seem to be missing a similar helper function for these
cases, media_pad_pipeline() or such. We can add when needed.
I think this could be done now, for consistency. Either the existing
drivers could call it on entity->pads, thus assuming they only use a single
pipeline for that entity. Or keep media_entity_pipeline(), but there's some
chance for confusion on where the pipeline is bound. At least there should
be a warning it's just for old existing drivers.
I clarified in the kdoc that media_entity_pipeline() is only to be used
by drivers that do not support multiple pipelines, and I added
media_pad_pipeline().
Tomi
