Hi Sakari
On 23/05/2024 21:57, Sakari Ailus wrote:
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.
Actually, not sure about this...the TPG just has a single source pad, so there's no _routing_ to set
and as far as I can tell that means there's also no _streams_ to set since they depend on the
routing - v4l2_subdev_enable_streams() checks the state's stream_configs to make sure the stream
you're asking to enable exists, and those stream_configs get created when routing is created - so I
think for now that the TPG has to stay as .s_stream().
In the isp subdevice I can switch to v4l2_subdev_[enable|disable]_streams() and let it fallback to
.s_stream() for the tpg - that part's fine.
Dan
+{
+ 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;
+}
+
