Re: [PATCH v11 34/36] media: v4l2-subdev: Add v4l2_subdev_state_xlate_streams() helper

[Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>]

On 17/03/2022 11:27, Tomi Valkeinen wrote:
> On 16/03/2022 13:26, Jacopo Mondi wrote:
>> Hi Tomi,
>>
>> On Tue, Mar 01, 2022 at 06:11:54PM +0200, Tomi Valkeinen wrote:
>>> From: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>>>
>>> Add a helper function to translate streams between two pads of a subdev,
>>> using the subdev's internal routing table.
>>>
>>> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>>> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
>>> ---
>>>   drivers/media/v4l2-core/v4l2-subdev.c | 25 +++++++++++++++++++++++++
>>>   include/media/v4l2-subdev.h           | 23 +++++++++++++++++++++++
>>>   2 files changed, 48 insertions(+)
>>>
>>> diff --git a/drivers/media/v4l2-core/v4l2-subdev.c 
>>> b/drivers/media/v4l2-core/v4l2-subdev.c
>>> index e30338a53088..6a9fc62dacbf 100644
>>> --- a/drivers/media/v4l2-core/v4l2-subdev.c
>>> +++ b/drivers/media/v4l2-core/v4l2-subdev.c
>>> @@ -1529,6 +1529,31 @@ 
>>> v4l2_subdev_state_get_opposite_stream_format(struct v4l2_subdev_state 
>>> *state,
>>>   }
>>>   EXPORT_SYMBOL_GPL(v4l2_subdev_state_get_opposite_stream_format);
>>>
>>> +u64 v4l2_subdev_state_xlate_streams(const struct v4l2_subdev_state 
>>> *state,
>>> +                    u32 pad0, u32 pad1, u64 *streams)
>>> +{
>>> +    const struct v4l2_subdev_krouting *routing = &state->routing;
>>> +    struct v4l2_subdev_route *route;
>>> +    u64 streams0 = 0;
>>> +    u64 streams1 = 0;
>>> +
>>> +    for_each_active_route(routing, route) {
>>> +        if (route->sink_pad == pad0 && route->source_pad == pad1 &&
>>> +            (*streams & BIT(route->sink_stream))) {
>>> +            streams0 |= BIT(route->sink_stream);
>>> +            streams1 |= BIT(route->source_stream);
>>> +        }
>>> +        if (route->source_pad == pad0 && route->sink_pad == pad1 &&
>>> +            (*streams & BIT(route->source_stream))) {
>>> +            streams0 |= BIT(route->source_stream);
>>> +            streams1 |= BIT(route->sink_stream);
>>> +        }
>>> +    }
>>> +
>>> +    *streams = streams0;
>>> +    return streams1;
>>
>> I'll probably learn how this is used later, but I found it hard to
>> immagine how the returned mask should be used.
>>
>> To a sink stream mask that contains multiple streams a source maks
>> with multiple entries will be associated
>>
>> In example, the sink stream mask might look lik
>>
>>          sink_stream_mask = {1, 0, 1, 0}
>>
>> So we are interested in translating stream 0 and 2
>> Assume 0->4 and 2->1 in the routing tabe, the resulting source stream
>> mask will be
>>
>>         source_stream_mask = {1, 0, 0, 1}
>>
>> How should the caller know what stream goes where ?
> 
> The use case for the function is for cases where all the streams go from 
> one pad to another. Probably the main use is in subdevs with a single 
> source and sink pad. This makes it easy to implement enable_streams() 
> op: you just basically call v4l2_subdev_state_xlate_streams() to get the 
> streams on the other side, and call v4l2_subdev_enable_streams(). Here's 
> a version from ub913:
> 
>> static int ub913_enable_streams(struct v4l2_subdev *sd,
>>                 struct v4l2_subdev_state *state, u32 pad,
>>                 u64 streams_mask)
>> {
>>     struct ub913_data *priv = sd_to_ub913(sd);
>>     struct media_pad *remote_pad;
>>     u64 sink_streams;
>>     int ret;
>>
>>     if (streams_mask & priv->enabled_source_streams)
>>         return -EALREADY;
>>
>>     sink_streams = v4l2_subdev_state_xlate_streams(
>>         state, UB913_PAD_SOURCE, UB913_PAD_SINK, &streams_mask);
>>
>>     remote_pad = media_entity_remote_pad(&priv->pads[UB913_PAD_SINK]);
>>
>>     ret = v4l2_subdev_enable_streams(priv->source_sd, remote_pad->index,
>>                      sink_streams);
>>     if (ret)
>>         return ret;
>>
>>     priv->enabled_source_streams |= streams_mask;
>>
>>     return 0;
>> }
> 
> 
> 
>>> +}
>>> +
>>>   int v4l2_subdev_get_fmt(struct v4l2_subdev *sd, struct 
>>> v4l2_subdev_state *state,
>>>               struct v4l2_subdev_format *format)
>>>   {
>>> diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
>>> index 1eced0f47296..992debe116ac 100644
>>> --- a/include/media/v4l2-subdev.h
>>> +++ b/include/media/v4l2-subdev.h
>>> @@ -1497,6 +1497,29 @@ struct v4l2_mbus_framefmt *
>>>   v4l2_subdev_state_get_opposite_stream_format(struct 
>>> v4l2_subdev_state *state,
>>>                            u32 pad, u32 stream);
>>>
>>> +/**
>>> + * v4l2_subdev_state_xlate_streams() - Translate streams from one 
>>> pad to another
>>
>> Could we stress out that pads are on the same entity and this is not
>> meant to translate over media links ? Is this necessary ?
> 
> The streams on both sides of a link are identical, so there can be no 
> translation in that case.
> 
> The doc below explains the use, I'd rather not try to squeeze in a 
> longer explanation in the title.
> 
> But I think we should emphasize that this only makes sense on a simple 
> routing case.

Ah, never mind. The function can be used in multi-pad cases too, so we 
don't need to emphasize the above.

The function tells how a set of streams from pad A translate to a set of 
streams on pad B. So you only know the set of streams, not how a single 
stream goes. For that you need to look at the routing table.

  Tomi