Re: [PATCH v3 26/31] adv748x: csi2: add internal routing configuration

[Luca Ceresoli <luca@lucaceresoli.net>]

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@ragnatech.se>
>>>>
>>>> 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@ragnatech.se>
>>>> Reviewed-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
>>>> ---
>>>>  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