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

[Laurent Pinchart]

On Sun, May 26, 2024 at 03:42:54PM +0000, Sakari Ailus wrote:
> On Fri, May 24, 2024 at 09:37:04AM +0100, Dan Scally wrote:
> > On 23/05/2024 22:01, Sakari Ailus wrote:
> > > On Thu, May 23, 2024 at 12:22:45PM +0100, Dan Scally wrote:
> > > > 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.

Why so ? I think it's a neat solution to communicate userspace's
expected behaviour to the driver.

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

I agree. If we want some of the outputs to be optional, we need the
request API. Without it, all video devices that are started (or I'd like
to say included in the pipeline) will need buffers, and I think they
should start at the same time.

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

-- 
Regards,

Laurent Pinchart