Hi Jacopo, Sakari,
On 15/03/19 11:06, Sakari Ailus wrote:
> Hi Luca, Jacopo,
>
> On Fri, Mar 15, 2019 at 10:45:38AM +0100, Jacopo Mondi wrote:
>> Hi Luca,
>>
>> On Thu, Mar 14, 2019 at 03:45:27PM +0100, Luca Ceresoli wrote:
>>> Hi,
>>>
>>> begging your pardon for the noob question below...
>>>
>>
>> Let a noob help another noob then
>>
>>> On 05/03/19 19:51, Jacopo Mondi wrote:
>>>> From: Niklas Söderlund <niklas.soderlund+renesas@xxxxxxxxxxxx>
>>>>
>>>> Add support to get and set the internal routing between the adv748x
>>>> CSI-2 transmitters sink pad and its multiplexed source pad. This routing
>>>> includes which stream of the multiplexed pad to use, allowing the user
>>>> to select which CSI-2 virtual channel to use when transmitting the
>>>> stream.
>>>>
>>>> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@xxxxxxxxxxxx>
>>>> Reviewed-by: Jacopo Mondi <jacopo+renesas@xxxxxxxxxx>
>>>> ---
>>>> drivers/media/i2c/adv748x/adv748x-csi2.c | 65 ++++++++++++++++++++++++
>>>> 1 file changed, 65 insertions(+)
>>>>
>>>> diff --git a/drivers/media/i2c/adv748x/adv748x-csi2.c b/drivers/media/i2c/adv748x/adv748x-csi2.c
>>>> index d8f7cbee86e7..13454af72c6e 100644
>>>> --- a/drivers/media/i2c/adv748x/adv748x-csi2.c
>>>> +++ b/drivers/media/i2c/adv748x/adv748x-csi2.c
>>>> @@ -14,6 +14,8 @@
>>>>
>>>> #include "adv748x.h"
>>>>
>>>> +#define ADV748X_CSI2_ROUTES_MAX 4
>>>> +
>>>> struct adv748x_csi2_format {
>>>> unsigned int code;
>>>> unsigned int datatype;
>>>> @@ -253,10 +255,73 @@ static int adv748x_csi2_get_frame_desc(struct v4l2_subdev *sd, unsigned int pad,
>>>> return 0;
>>>> }
>>>>
>>>> +static int adv748x_csi2_get_routing(struct v4l2_subdev *sd,
>>>> + struct v4l2_subdev_krouting *routing)
>>>> +{
>>>> + struct adv748x_csi2 *tx = adv748x_sd_to_csi2(sd);
>>>> + struct v4l2_subdev_route *r = routing->routes;
>>>> + unsigned int vc;
>>>> +
>>>> + if (routing->num_routes < ADV748X_CSI2_ROUTES_MAX) {
>>>> + routing->num_routes = ADV748X_CSI2_ROUTES_MAX;
>>>> + return -ENOSPC;
>>>> + }
>>>> +
>>>> + routing->num_routes = ADV748X_CSI2_ROUTES_MAX;
>>>> +
>>>> + for (vc = 0; vc < ADV748X_CSI2_ROUTES_MAX; vc++) {
>>>> + r->sink_pad = ADV748X_CSI2_SINK;
>>>> + r->sink_stream = 0;
>>>> + r->source_pad = ADV748X_CSI2_SOURCE;
>>>> + r->source_stream = vc;
>>>> + r->flags = vc == tx->vc ? V4L2_SUBDEV_ROUTE_FL_ACTIVE : 0;
>>>> + r++;
>>>> + }
>>>> +
>>>> + return 0;
>>>> +}
>>>> +
>>>> +static int adv748x_csi2_set_routing(struct v4l2_subdev *sd,
>>>> + struct v4l2_subdev_krouting *routing)
>>>> +{
>>>> + struct adv748x_csi2 *tx = adv748x_sd_to_csi2(sd);
>>>> + struct v4l2_subdev_route *r = routing->routes;
>>>> + unsigned int i;
>>>> + int vc = -1;
>>>> +
>>>> + if (routing->num_routes > ADV748X_CSI2_ROUTES_MAX)
>>>> + return -ENOSPC;
>>>> +
>>>> + for (i = 0; i < routing->num_routes; i++) {
>>>> + if (r->sink_pad != ADV748X_CSI2_SINK ||
>>>> + r->sink_stream != 0 ||
>>>> + r->source_pad != ADV748X_CSI2_SOURCE ||
>>>> + r->source_stream >= ADV748X_CSI2_ROUTES_MAX)
>>>> + return -EINVAL;
>>>> +
>>>> + if (r->flags & V4L2_SUBDEV_ROUTE_FL_ACTIVE) {
>>>> + if (vc != -1)
>>>> + return -EMLINK;
>>>> +
>>>> + vc = r->source_stream;
>>>> + }
>>>> + r++;
>>>> + }
>>>> +
>>>> + if (vc != -1)
>>>> + tx->vc = vc;
>>>> +
>>>> + adv748x_csi2_set_virtual_channel(tx, tx->vc);
>>>> +
>>>> + return 0;
>>>> +}
>>>
>>> Not specific to this patch but rather to the set_routing idea as a
>>> whole: can the set_routing ioctl be called while the stream is running?
>>>
>>> If it cannot, I find it a limiting factor for nowadays use cases. I also
>>> didn't find where the ioctl is rejected.
>>>
>>
>> The framework does not make assumptions about that at the moment.
>>
>>> If it can, then shouldn't this function call s_stream(stop) through the
>>> sink pad whose route becomes disabled, and a s_stream(start) through the
>>> one that gets enabled?
>>>
>>
>> If I got this right, you're here rightfully pointing out that changing
>> the routing between pads in an entity migh impact the pipeline as a
>> whole, and this would require, to activate/deactivate devices that
>> where not part of the pipeline.
>
> I'd say that ultimately this depends on the devices themselves, whether
> they support this or not. In practice I don't think we have any such cases
> at the moment, but it's possible in principle. Changes on the framework may
> well be needed but likely the biggest complications will still be in
> drivers supporting that.
I understand V4L2 currently does not support changing a pipeline that is
running. However there are many use cases that would require it.
Most of the use cases that come to my mind involve a multiplexer with
multiple inputs, one of which can be selected to be forwarded. In those
cases s_routing deselects an input and selects another one. How the can
we handle such cases without sending a s_stream on the two upstreams?
Having all possible inputs always running is not a real solution.
> The media links have a dynamic flag for this purpose but I don't think it's
> ever been used.
>
>>
>> This is probably the wrong patch to use an example, as this one is for
>> a multiplexed interface, where there is no need to go through an
>> s_stream() for the two CSI-2 endpoints, but as you pointed out in our
>> brief offline chat, the AFE->TX routing example for this very device
>> is a good one: if we change the analogue source that is internally
>> routed to the CSI-2 output of the adv748x, do we need to s_stream(1)
>> the now routed entity and s_stream(0) on the not not-anymore-routed
>> one?
>>
>> My gut feeling is that this is up to userspace, as it should know
>> what are the requirements of the devices in the system, but this mean
>> going through an s_stream(0)/s_stream(1) sequence on the video device,
>> and that would interrupt the streaming for sure.
>>
>> At the same time, I don't feel too much at ease with the idea of
>> s_routing calling s_stream on the entity' remote subdevices, as this
>> would skip the link format validation that media_pipeline_start()
>> performs.
>
> The link validation must be done in this case as well, it may not be
> simply skipped.
Agreed.
The routing VS pipeline validation point is a very important one. The
current proposed workflow is:
1. the pipeline is validated as a whole, having knowledge of all the
entities
2. streaming is started
3. s_routing is called on an entity (not on the pipeline!)
Now the s_routing function in the entity driver is not in a good
position to validate the candidate future pipeline as a whole.
Naively I'd say there are two possible solutions:
1. the s_routing reaches the pipeline first, then the new pipeline is
computed and verified, and if verification succeeds it is applied
2. a partial pipeline verification mechanism is added, so the entity
receiving a s_routing request to e.g. change the sink pad can invoke
a verification on the part of pipeline that is about to be
activated, and if verification succeeds it is applied
Somehow I suspect neither is trivial...
--
Luca
