On Fri, Jul 29, 2022 at 10:07 AM Tomi Valkeinen
<tomi.valkeinen@xxxxxxxxxxxxxxxx> 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.
>
> Tomi
Sure, great!
- Satish
