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

[Sakari Ailus]

Hi Dan,

On Thu, May 23, 2024 at 03:27:36PM +0100, Dan Scally wrote:
> Hi Sakari - sorry, one part I missed...
> 
> On 23/05/2024 09:03, Sakari Ailus wrote:
> > > +
> > > +int mali_c55_isp_s_stream(struct mali_c55_isp *isp, int enable)
> > Have you considered {enable,disable}_streams? All new drivers should use
> > these instead of s_stream() now.
> 
> 
> Although named s_stream this is actually a purely internal function - it's
> not exposed as part of the subdev video ops. The resizer subdevices
> similarly don't expose an .s_stream() operation, they're simply started in
> the callpath of mali_c55_vb2_start_streaming(). I'll split the stop
> functionality into mali_c55_isp_stop_stream() and rename this
> mali_c55_isp_start_stream() to make that less confusing.

Ack. But this might require some rework, depending on based on what
streaming is actually started. I'm referring to the discussion elsewhere
in the same thread.

> 
> 
> The TPG subdevice on the other hand does expose an .s_stream() operation,
> since the intention was to model it exactly like a connected external
> subdevice. I can switch to the .enable_streams() operation there.

Sounds good.

> 
> > 
> > > +{
> > > +	struct mali_c55 *mali_c55 = isp->mali_c55;
> > > +	struct media_pad *source_pad;
> > > +	struct media_pad *sink_pad;
> > > +	int ret;
> > > +
> > > +	if (!enable) {
> > > +		if (isp->source)
> > > +			v4l2_subdev_call(isp->source, video, s_stream, false);

This call could be v4l2_subdev_disable_streams().

> > > +		isp->source = NULL;
> > > +
> > > +		mali_c55_isp_stop(mali_c55);
> > > +
> > > +		return 0;
> > > +	}
> > > +
> > > +	sink_pad = &isp->pads[MALI_C55_ISP_PAD_SINK_VIDEO];
> > > +	source_pad = media_pad_remote_pad_unique(sink_pad);
> > > +	if (IS_ERR(source_pad)) {
> > > +		dev_err(mali_c55->dev, "Failed to get source for ISP\n");
> > > +		return PTR_ERR(source_pad);
> > > +	}
> > > +
> > > +	isp->source = media_entity_to_v4l2_subdev(source_pad->entity);
> > > +
> > > +	isp->frame_sequence = 0;
> > > +	ret = mali_c55_isp_start(mali_c55);
> > > +	if (ret) {
> > > +		dev_err(mali_c55->dev, "Failed to start ISP\n");
> > > +		isp->source = NULL;
> > > +		return ret;
> > > +	}
> > > +
> > > +	ret = v4l2_subdev_call(isp->source, video, s_stream, true);

And this could be v4l2_subdev_enable_streams() as well.

> > > +	if (ret) {
> > > +		dev_err(mali_c55->dev, "Failed to start ISP source\n");
> > > +		mali_c55_isp_stop(mali_c55);
> > > +		return ret;
> > > +	}
> > > +
> > > +	return 0;
> > > +}
> > > +

-- 
Regards,

Sakari Ailus