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

[Sakari Ailus]

Hi Dan,

On Fri, May 24, 2024 at 09:37:04AM +0100, Dan Scally wrote:
> 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

The behaviour there without the request API is somewhat driver specific.
Most driver's don't use it.

> 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.

In that case you'd  start both queues but of course you can queue buffers
just to the first node before you need anything from the second one
(assuming this is supported by the driver).

> 
> 
> > 
> > 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.
> > 

-- 
Kind regards,

Sakari Ailus