Re: [PATCH v3 38/38] media: ti-vpe: cal: add multiplexed streams support

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

On 27/05/2021 19:30, Laurent Pinchart wrote:
> On Thu, May 27, 2021 at 07:10:42PM +0300, Tomi Valkeinen wrote:
>> On 27/05/2021 19:06, Pratyush Yadav wrote:
>>> On 24/05/21 02:09PM, Tomi Valkeinen wrote:
>>>> Add routing and stream_config support to CAL driver.
>>>>
>>>> Add multiplexed streams support. CAL has 8 dma-engines and can capture 8
>>>> separate streams at the same time.
>>>>
>>>> Add 8 video device nodes, each representing a single dma-engine, and set
>>>> the number of source pads on camerarx to 8. Each video node can be
>>>> connected to any of the source pads on either of the camerarx instances
>>>> using media links. Camerarx internal routing is used to route the
>>>> incoming CSI-2 streams to one of the 8 source pads.
>>>>
>>>> CAL doesn't support transcoding, so the driver currently allows changes
>>>> only on the camerarx sink side, and then copies the sink pad config to
>>>> the source pad. This becomes slighly more complex with 8 source pads and
>>>> multiple streams on the sink pad. A helper,
>>>> cal_camerarx_get_opposite_stream_format(), is added, which uses the
>>>> routing table to get the format from the "opposite" side.
>>>>
>>>> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
>>>> ---
>>>>    drivers/media/platform/ti-vpe/cal-camerarx.c | 303 ++++++++++++++++---
>>>>    drivers/media/platform/ti-vpe/cal-video.c    | 103 ++++++-
>>>>    drivers/media/platform/ti-vpe/cal.c          |  34 ++-
>>>>    drivers/media/platform/ti-vpe/cal.h          |  12 +-
>>>>    4 files changed, 385 insertions(+), 67 deletions(-)
>>>>
>>> [...]
>>>> @@ -1178,18 +1177,33 @@ static int cal_probe(struct platform_device *pdev)
>>>>    	}
>>>>    
>>>>    	/* Create contexts. */
>>>> -	for (i = 0; i < cal->data->num_csi2_phy; ++i) {
>>>> -		if (!cal->phy[i]->source_node)
>>>> -			continue;
>>>> +	if (!cal_mc_api) {
>>>> +		for (i = 0; i < cal->data->num_csi2_phy; ++i) {
>>>> +			if (!cal->phy[i]->source_node)
>>>> +				continue;
>>>> +
>>>> +			cal->ctx[i] = cal_ctx_create(cal, i);
>>>> +			if (!cal->ctx[i]) {
>>>> +				cal_err(cal, "Failed to create context %u\n", i);
>>>> +				ret = -ENODEV;
>>>> +				goto error_context;
>>>> +			}
>>>>    
>>>> -		cal->ctx[i] = cal_ctx_create(cal, i);
>>>> -		if (!cal->ctx[i]) {
>>>> -			cal_err(cal, "Failed to create context %u\n", i);
>>>> -			ret = -ENODEV;
>>>> -			goto error_context;
>>>> +			cal->ctx[i]->phy = cal->phy[i];
>>>> +
>>>> +			cal->num_contexts++;
>>>>    		}
>>>> +	} else {
>>>> +		for (i = 0; i < ARRAY_SIZE(cal->ctx); ++i) {
>>>> +			cal->ctx[i] = cal_ctx_create(cal, i);
>>>> +			if (!cal->ctx[i]) {
>>>> +				cal_err(cal, "Failed to create context %u\n", i);
>>>> +				ret = -ENODEV;
>>>> +				goto error_context;
>>>> +			}
>>>>    
>>>> -		cal->num_contexts++;
>>>> +			cal->num_contexts++;
>>>
>>> In cal_async_notifier_complete() I see:
>>>
>>>     for (i = 0; i < cal->num_contexts; i++)
>>>       ret = cal_ctx_v4l2_register();
>>>
>>> This means that if the CAL device has 8 DMA contexts it will create 8
>>> /dev/videoX nodes, even if the hardware setup is only capable of 1
>>> stream.
>>>
>>> Would it make more sense to populate /dev/videoX nodes based on the
>>> configured routing? So for example, if only one pad is being used to
>>> output, only create one node corresponding to that pad. If there are 3
>>> pads being populated then create 3 nodes and so on.
>>
>> Routing is a runtime configuration, so it could mean creating or
>> removing video nodes every time the user changes the routing. I believe
>> video nodes are supposed to be more permanent than that.
>>
>> If we knew that the HW setup can only ever have N routes, we could limit
>> the number of video nodes, but I don't think we have means to figure
>> that out.
> 
> And even if we did, I think that wouldn't help userspace. The media
> graph is meant to model the hardware topology, it's best to minimize the
> complexity on the kernel side and let userspace deal with routing
> configuration.

I think it's a valid question. Maybe a CSI-2 RX uses system DMA, and can 
support, say, 128 contexts. We probably don't want 128 video nodes (of 
which perhaps 1-4 are ever used). But in CAL's case, I think always 
having all the 8 video nodes is acceptable.

  Tomi