[PATCH v4 3/5] media: mali-c55: Add Mali-C55 ISP driver

[Dan Scally]

Good morning Sakari

On 23/05/2024 22:01, Sakari Ailus wrote:
> Hi Dan,
>
> On Thu, May 23, 2024 at 12:22:45PM +0100, Dan Scally wrote:
>> Hello
>>
>> On 23/05/2024 10:49, Laurent Pinchart wrote:
>>> On Thu, May 23, 2024 at 11:48:02AM +0200, Jacopo Mondi wrote:
>>>> On Thu, May 23, 2024 at 08:03:49AM GMT, Sakari Ailus wrote:
>>>>> Hi Daniel,
>>>> [snip]
>>>>
>>>>>> +
>>>>>> +static int mali_c55_vb2_start_streaming(struct vb2_queue *q, unsigned int count)
>>>>>> +{
>>>>>> +	struct mali_c55_cap_dev *cap_dev = q->drv_priv;
>>>>>> +	struct mali_c55 *mali_c55 = cap_dev->mali_c55;
>>>>>> +	struct mali_c55_resizer *rzr = cap_dev->rzr;
>>>>>> +	struct mali_c55_isp *isp = &mali_c55->isp;
>>>>>> +	int ret;
>>>>>> +
>>>>>> +	guard(mutex)(&isp->lock);
>>>>>> +
>>>>>> +	ret = pm_runtime_resume_and_get(mali_c55->dev);
>>>>>> +	if (ret)
>>>>>> +		return ret;
>>>>>> +
>>>>>> +	ret = video_device_pipeline_start(&cap_dev->vdev,
>>>>>> +					  &cap_dev->mali_c55->pipe);
>>>>>> +	if (ret) {
>>>>>> +		dev_err(mali_c55->dev, "%s failed to start media pipeline\n",
>>>>>> +			mali_c55_cap_dev_to_name(cap_dev));
>>>>>> +		goto err_pm_put;
>>>>>> +	}
>>>>>> +
>>>>>> +	mali_c55_cap_dev_stream_enable(cap_dev);
>>>>>> +	mali_c55_rzr_start_stream(rzr);
>>>>>> +
>>>>>> +	/*
>>>>>> +	 * We only start the ISP if we're the only capture device that's
>>>>>> +	 * streaming. Otherwise, it'll already be active.
>>>>>> +	 */
>>>>>> +	if (mali_c55->pipe.start_count == 1) {
>>>>> Do you start streaming on the sensor when the first video node does?
>>>>>
>>>>> This means that frames may be lost. E.g. the IPU6 ISYS driver only starts
>>>>> streaming on the sensor once all video nodes of the pipeline have been
>>>>> started.
>>>> How would you ever know which nodes will be started ?
>>> That can be done with link setup. Any video device that has an active
>>> link to the ISP would need to be started.
>>
>> So if you don't want to stream data from one of the two capture nodes, you'd
>> need to disable the link between it and the resizer? That seems quite
>> clunky. Does it matter if one of them starts a frame or two later? As
>> opposed to both of them starting in sync a frame or two later?
> Video frames on a given queue are lost due to the driver implementation.
> I might consider that to be a driver bug.


This seems a bit odd to me; I think that the implication is when you **queue** a buffer to the 
driver you're targeting a specific frame from the sensor...is that right? What about if you want to 
start streaming on the second capture node at some later point, after the first had already been 
started? I think we'd be in the same situation there, with the already started capture node getting 
buffers filled after the second had had buffers queued, but before you could start it.


>
> It's also true that some use cases could also benefit from the behaviour
> but on regular CSI-2 receivers that's probably quite rare. We'd need
> additional APIs to be able to convey the desired behaviour to the drivers.
>