Message-ID: <964be974-bdd1-4858-b278-ae78cc34c5b9@gmail.com>
Date: Wed, 10 Jan 2024 17:12:43 +0200
From: Cosmin Tanislav <demonsingur@...il.com>
To: Tomi Valkeinen <tomi.valkeinen@...asonboard.com>
Cc: Mauro Carvalho Chehab <mchehab@...nel.org>,
 Sakari Ailus <sakari.ailus@...ux.intel.com>,
 Laurent Pinchart <laurent.pinchart@...asonboard.com>,
 Jacopo Mondi <jacopo+renesas@...ndi.org>, Hans de Goede
 <hdegoede@...hat.com>, linux-media@...r.kernel.org,
 linux-kernel@...r.kernel.org
Subject: Re: [RFC PATCH] media: v4l: implement virtual channels


On 1/10/24 15:36, Tomi Valkeinen wrote:
> Hi,
>
> On 10/01/2024 15:27, Cosmin Tanislav wrote:
>> Hi, Tomi.
>>
>> The usecase for this is crossbar devices that allow for mapping virtual channel
>>
>> ids dynamically.
>>
>> The remapping happens based on VC and DT. DT can be determined based on
>>
>> the pixel code, but VC has no way of being determined at runtime through
>>
>> standard V4L2 APIs.
>
> The DT shouldn't be determined based on the pixel code. The DT, and the VC, should be queried from the source subdev using the get_frame_desc() subdev op.
>
> So, in general, the streams API presents a logical view of the streams without a strict direct mapping to the hardware. When the streaming is started, the subdev drivers need to figure out the hardware details, and, specifically for the DT & VC of incoming streams, can ask details from the source subdevice using get_frame_desc().
>
> Or if I misunderstand your case, can you provide a concrete example of the pipeline and the media tree in question to help the discussion.
>
>  Tomi

---------------------------------------------------------------------------

I've attached the result of media-ctl --print-dot. I'm not sure if this is
visible in the mailing list.

There are 4 cameras attached to 4 serializers, which connect to 4 ports of
a single deserializer.

I've split each device into multiple subdevs and introduced crossbar
devices which can have their routing table changed.
This mostly matches the hardware blocks available within the chip.

The connection between cameras and serializer PHYs, and between serializer
links and deserializer links are specified in device tree using the
standard graph API.

In each serializer, the phy_pipe_xbar subdev allows mapping the output of
each PHY to a pipe, allowing stream duplication. One serializer can have
multiple PHYs, although this is not pictured here.
The pipe_link_xbar subdev allows mapping the output of each pipe to a
hardware stream of the link. This mapping is 1:1. One serializer can have
multiple pipes, as pictured here for serializer 0 and serializer 1.

On the deserializer side, the link_pipe_xbar subdev allows selecting which
link maps to which pipe. Some deserializer chips don't support automatic
link hardware stream detection, in which case each deserializer link will
have a source port for each hardware stream, and the link_pipe_xbar will
have a source port for each hardware stream of each link too. This subdev
also allows stream duplication by having multiple pipes receive data from
the same link + hardware stream. It also allows multiple pipes to receive
data from the same link but different hardware streams.

Further down there's a pipe_phy_xbar subdev. This is the one where this
patch is necessary. It allows remapping the DT and VC of all incoming data
streams to new values. This is useful because we might have a downstream
device that is able to multiplex the incoming data streams into separate
video devices based on VC, allowing us to connect multiple cameras over
the same CSI interface, but having them show up as multiple video devices.

I know this is a very complicated pipeline, but doing all this logic
automatically and correctly is very hard, while configuring it manually is
pretty easy, which is why we would like to expose all these configurations
to the user, which is able to take better decisions based on their usecase.

>
>>
>> The alternative would be to hardcode them in device tree, which is not very
>>
>> nice, and cannot account for all possible user configurations.
>>
>> I'm aware that VC is a bus-specific feature, which is why this is an RFC.
>>
>> Having a V4L2 implementation would solve a lot of devices having to
>>
>> hardcode VCs.
>>
>>
>> On 1/10/24 14:50, Tomi Valkeinen wrote:
>>> Hi!
>>>
>>> On 10/01/2024 14:51, Cosmin Tanislav wrote:
>>>> With experimental support for multiple streams per pad being added, the
>>>> pieces are in place to support a virtual channel id per stream.
>>>>
>>>> This is necessary because stream ids cannot be directly mapped to a virtual
>>>> channel id, since the same virtual channel id can be assigned to multiple
>>>> streams of data, each with a different data type.
>>>>
>>>> To implement this, the following steps have been taken.
>>>>
>>>> Add subdev ioctls for getting and setting the virtual channel for a
>>>> specific pad and stream.
>>>>
>>>> Add pad .get_vc() and .set_vc() ops.
>>>>
>>>> Add the virtual channel to the stream config in V4L2 subdev central state.
>>>>
>>>> Add a default .get_vc() implementation that retrieves the virtual channel
>>>> from the central state, or, if that is not supported, default to virtual
>>>> channel 0.
>>>
>>> Why do you need this?
>>>
>>> The design idea with streams was that the streams are not tied to CSI-2 streams (or to any specific HW). The CSI-2 virtual channels should be handled by the drivers internally, and they should not be visible to the userspace at all.
>>>
>>>   Tomi
>>>
>>>> Signed-off-by: Cosmin Tanislav <demonsingur@...il.com>
>>>> ---
>>>>    drivers/media/v4l2-core/v4l2-subdev.c | 57 +++++++++++++++++++++++++++
>>>>    include/media/v4l2-subdev.h           | 39 ++++++++++++++++++
>>>>    include/uapi/linux/v4l2-subdev.h      | 18 +++++++++
>>>>    3 files changed, 114 insertions(+)
>>>>
>>>> diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c
>>>> index be86b906c985..8945bfd0fe12 100644
>>>> --- a/drivers/media/v4l2-core/v4l2-subdev.c
>>>> +++ b/drivers/media/v4l2-core/v4l2-subdev.c
>>>> @@ -535,6 +535,9 @@ subdev_ioctl_get_state(struct v4l2_subdev *sd, struct v4l2_subdev_fh *subdev_fh,
>>>>        case VIDIOC_SUBDEV_S_ROUTING:
>>>>            which = ((struct v4l2_subdev_routing *)arg)->which;
>>>>            break;
>>>> +    case VIDIOC_SUBDEV_G_VC:
>>>> +    case VIDIOC_SUBDEV_S_VC:
>>>> +        which = ((struct v4l2_subdev_vc *)arg)->which;
>>>>        }
>>>>          return which == V4L2_SUBDEV_FORMAT_TRY ?
>>>> @@ -969,6 +972,26 @@ static long subdev_do_ioctl(struct file *file, unsigned int cmd, void *arg,
>>>>                        routing->which, &krouting);
>>>>        }
>>>>    +    case VIDIOC_SUBDEV_G_VC: {
>>>> +        struct v4l2_subdev_vc *vc = arg;
>>>> +
>>>> +        if (!client_supports_streams)
>>>> +            vc->stream = 0;
>>>> +
>>>> +        memset(vc->reserved, 0, sizeof(vc->reserved));
>>>> +        return v4l2_subdev_call(sd, pad, get_vc, state, vc);
>>>> +    }
>>>> +
>>>> +    case VIDIOC_SUBDEV_S_VC: {
>>>> +        struct v4l2_subdev_vc *vc = arg;
>>>> +
>>>> +        if (!client_supports_streams)
>>>> +            vc->stream = 0;
>>>> +
>>>> +        memset(vc->reserved, 0, sizeof(vc->reserved));
>>>> +        return v4l2_subdev_call(sd, pad, set_vc, state, vc);
>>>> +    }
>>>> +
>>>>        case VIDIOC_SUBDEV_G_CLIENT_CAP: {
>>>>            struct v4l2_subdev_client_capability *client_cap = arg;
>>>>    @@ -1602,6 +1625,20 @@ int v4l2_subdev_get_fmt(struct v4l2_subdev *sd, struct v4l2_subdev_state *state,
>>>>    }
>>>>    EXPORT_SYMBOL_GPL(v4l2_subdev_get_fmt);
>>>>    +int v4l2_subdev_get_vc(struct v4l2_subdev *sd, struct v4l2_subdev_state *state,
>>>> +               struct v4l2_subdev_vc *vc)
>>>> +{
>>>> +    u32 vc_id = 0;
>>>> +
>>>> +    if (sd->flags & V4L2_SUBDEV_FL_STREAMS)
>>>> +        vc_id = v4l2_subdev_state_get_stream_vc(state, vc->pad, vc->stream);
>>>> +
>>>> +    vc->vc = vc_id;
>>>> +
>>>> +    return 0;
>>>> +}
>>>> +EXPORT_SYMBOL_GPL(v4l2_subdev_get_vc);
>>>> +
>>>>    int v4l2_subdev_set_routing(struct v4l2_subdev *sd,
>>>>                    struct v4l2_subdev_state *state,
>>>>                    const struct v4l2_subdev_krouting *routing)
>>>> @@ -1745,6 +1782,26 @@ v4l2_subdev_state_get_stream_compose(struct v4l2_subdev_state *state,
>>>>    }
>>>>    EXPORT_SYMBOL_GPL(v4l2_subdev_state_get_stream_compose);
>>>>    +u32 v4l2_subdev_state_get_stream_vc(struct v4l2_subdev_state *state,
>>>> +                    unsigned int pad, u32 stream)
>>>> +{
>>>> +    struct v4l2_subdev_stream_configs *stream_configs;
>>>> +    unsigned int i;
>>>> +
>>>> +    lockdep_assert_held(state->lock);
>>>> +
>>>> +    stream_configs = &state->stream_configs;
>>>> +
>>>> +    for (i = 0; i < stream_configs->num_configs; ++i) {
>>>> +        if (stream_configs->configs[i].pad == pad &&
>>>> +            stream_configs->configs[i].stream == stream)
>>>> +            return stream_configs->configs[i].vc;
>>>> +    }
>>>> +
>>>> +    return 0;
>>>> +}
>>>> +EXPORT_SYMBOL_GPL(v4l2_subdev_state_get_stream_vc);
>>>> +
>>>>    int v4l2_subdev_routing_find_opposite_end(const struct v4l2_subdev_krouting *routing,
>>>>                          u32 pad, u32 stream, u32 *other_pad,
>>>>                          u32 *other_stream)
>>>> diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
>>>> index c1f90c1223a7..ed1fdd79c2bb 100644
>>>> --- a/include/media/v4l2-subdev.h
>>>> +++ b/include/media/v4l2-subdev.h
>>>> @@ -722,6 +722,7 @@ struct v4l2_subdev_stream_config {
>>>>        u32 stream;
>>>>        bool enabled;
>>>>    +    u32 vc;
>>>>        struct v4l2_mbus_framefmt fmt;
>>>>        struct v4l2_rect crop;
>>>>        struct v4l2_rect compose;
>>>> @@ -858,6 +859,12 @@ struct v4l2_subdev_pad_ops {
>>>>        int (*set_fmt)(struct v4l2_subdev *sd,
>>>>                   struct v4l2_subdev_state *state,
>>>>                   struct v4l2_subdev_format *format);
>>>> +    int (*get_vc)(struct v4l2_subdev *sd,
>>>> +              struct v4l2_subdev_state *state,
>>>> +              struct v4l2_subdev_vc *vc);
>>>> +    int (*set_vc)(struct v4l2_subdev *sd,
>>>> +              struct v4l2_subdev_state *state,
>>>> +              struct v4l2_subdev_vc *vc);
>>>>        int (*get_selection)(struct v4l2_subdev *sd,
>>>>                     struct v4l2_subdev_state *state,
>>>>                     struct v4l2_subdev_selection *sel);
>>>> @@ -1494,6 +1501,23 @@ v4l2_subdev_lock_and_get_active_state(struct v4l2_subdev *sd)
>>>>    int v4l2_subdev_get_fmt(struct v4l2_subdev *sd, struct v4l2_subdev_state *state,
>>>>                struct v4l2_subdev_format *format);
>>>>    +/**
>>>> + * v4l2_subdev_get_vc() - Fill virtual channel based on state
>>>> + * @sd: subdevice
>>>> + * @state: subdevice state
>>>> + * @vc: pointer to &struct v4l2_subdev_vc
>>>> + *
>>>> + * Fill @vc->vc field based on the information in the @vc struct.
>>>> + *
>>>> + * This function can be used by the subdev drivers which support active state to
>>>> + * implement v4l2_subdev_pad_ops.get_vc if the subdev driver does not need to
>>>> + * do anything special in their get_vc op.
>>>> + *
>>>> + * Returns 0 on success, error value otherwise.
>>>> + */
>>>> +int v4l2_subdev_get_vc(struct v4l2_subdev *sd, struct v4l2_subdev_state *state,
>>>> +               struct v4l2_subdev_vc *vc);
>>>> +
>>>>    /**
>>>>     * v4l2_subdev_set_routing() - Set given routing to subdev state
>>>>     * @sd: The subdevice
>>>> @@ -1585,6 +1609,21 @@ struct v4l2_rect *
>>>>    v4l2_subdev_state_get_stream_compose(struct v4l2_subdev_state *state,
>>>>                         unsigned int pad, u32 stream);
>>>>    +/**
>>>> + * v4l2_subdev_state_get_stream_vc() - Get the virtual channel of a stream
>>>> + * @state: subdevice state
>>>> + * @pad: pad id
>>>> + * @stream: stream id
>>>> + *
>>>> + * This returns the virtual channel for the given pad + stream in the
>>>> + * subdev state.
>>>> + *
>>>> + * If the state does not contain the given pad + stream, 0 is returned.
>>>> + */
>>>> +u32
>>>> +v4l2_subdev_state_get_stream_vc(struct v4l2_subdev_state *state,
>>>> +                unsigned int pad, u32 stream);
>>>> +
>>>>    /**
>>>>     * v4l2_subdev_routing_find_opposite_end() - Find the opposite stream
>>>>     * @routing: routing used to find the opposite side
>>>> diff --git a/include/uapi/linux/v4l2-subdev.h b/include/uapi/linux/v4l2-subdev.h
>>>> index b383c2fe0cf3..8e90405bb1e6 100644
>>>> --- a/include/uapi/linux/v4l2-subdev.h
>>>> +++ b/include/uapi/linux/v4l2-subdev.h
>>>> @@ -187,6 +187,22 @@ struct v4l2_subdev_capability {
>>>>        __u32 reserved[14];
>>>>    };
>>>>    +/**
>>>> + * struct v4l2_subdev_vc - Pad-level virtual channel settings
>>>> + * @which: format type (from enum v4l2_subdev_format_whence)
>>>> + * @pad: pad number, as reported by the media API
>>>> + * @vc: virtual channel
>>>> + * @stream: stream number, defined in subdev routing
>>>> + * @reserved: drivers and applications must zero this array
>>>> + */
>>>> +struct v4l2_subdev_vc {
>>>> +    __u32 which;
>>>> +    __u32 pad;
>>>> +    __u32 vc;
>>>> +    __u32 stream;
>>>> +    __u32 reserved[7];
>>>> +};
>>>> +
>>>>    /* The v4l2 sub-device video device node is registered in read-only mode. */
>>>>    #define V4L2_SUBDEV_CAP_RO_SUBDEV        0x00000001
>>>>    @@ -268,6 +284,8 @@ struct v4l2_subdev_client_capability {
>>>>    #define VIDIOC_SUBDEV_S_SELECTION        _IOWR('V', 62, struct v4l2_subdev_selection)
>>>>    #define VIDIOC_SUBDEV_G_ROUTING            _IOWR('V', 38, struct v4l2_subdev_routing)
>>>>    #define VIDIOC_SUBDEV_S_ROUTING            _IOWR('V', 39, struct v4l2_subdev_routing)
>>>> +#define VIDIOC_SUBDEV_G_VC            _IOWR('V', 40, struct v4l2_subdev_vc)
>>>> +#define VIDIOC_SUBDEV_S_VC            _IOWR('V', 41, struct v4l2_subdev_vc)
>>>>    #define VIDIOC_SUBDEV_G_CLIENT_CAP        _IOR('V',  101, struct v4l2_subdev_client_capability)
>>>>    #define VIDIOC_SUBDEV_S_CLIENT_CAP        _IOWR('V',  102, struct v4l2_subdev_client_capability)
>>>>    
>>>
>
Download attachment "graph.dot" of type "application/msword-template" (10430 bytes)

Powered by blists - more mailing lists