Re: [EXT] Re: [EXT] Re: [EXT] [PATCH v12 09/30] media: mc: entity: Rewrite media_pipeline_start() to support routes

[Satish Nagireddy <satish.nagireddy@getcruise.com>]

On Fri, Jul 29, 2022 at 3:27 AM Tomi Valkeinen
<tomi.valkeinen@ideasonboard.com> wrote:
>
> On 29/07/2022 12:19, Satish Nagireddy wrote:
> > On Fri, Jul 29, 2022 at 1:53 AM Tomi Valkeinen
> > <tomi.valkeinen@ideasonboard.com> 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;
}

- Satish

>
> >>>>    }
> >>>>    EXPORT_SYMBOL_GPL(media_entity_pipeline);
> >>>
> >>> nit, It would be nice to rename this function to media_entity_get_pipe
> >>> or media_entity_get_pipeline for better readability.
> >>
> >> I'm ok with that, but we do have other functions with this style:
> >> media_entity_remote_pad(), media_entity_id(), ...
> >>
> >>    Tomi
> >
> > I could only see one function with the similar style ==>
> > media_entity_get_fwnode_pad
>
> Right, so, do you agree that we should keep the name as
> media_entity_pipeline, as we already have many other functions with
> similar style?
>
>   Tomi

Sure, we can go with the same function name.

- Satish