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

[Sakari Ailus]

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.

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.

-- 
Regards,

Sakari Ailus