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;
}
- 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
