[PATCH v4 3/5] media: mali-c55: Add Mali-C55 ISP driver
Dan Scally
dan.scally at ideasonboard.com
Fri May 24 03:31:28 PDT 2024
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;
>>>> +}
>>>> +
More information about the linux-arm-kernel
mailing list