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.
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.
+{ + 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); + 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); + if (ret) { + dev_err(mali_c55->dev, "Failed to start ISP source\n"); + mali_c55_isp_stop(mali_c55); + return ret; + } + + return 0; +} +
