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.
--
Terveisin,
Sakari Ailus
