Re: [PATCH v7 03/15] media: i2c: imx219: Report internal routes to userspace

[Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>]

On 04/04/2024 11:29, Sakari Ailus wrote:
> Moi,
> 
> On Thu, Apr 04, 2024 at 11:19:22AM +0300, Tomi Valkeinen wrote:
>> Hi,
>>
>> On 25/03/2024 00:08, Laurent Pinchart wrote:
>>> Usage of internal pads creates a route internal to the subdev, and the
>>> V4L2 camera sensor API requires such routes to be reported to userspace.
>>> Create the route in the .init_state() operation.
>>>
>>> Internal routing support requires stream support, so set the
>>> V4L2_SUBDEV_FL_HAS_STREAMS flag and switch formats and selection
>>
>> It's V4L2_SUBDEV_FL_STREAMS (not sure why, as the other flags have _HAS_).
> 
> Didn't you write the patch to add the flag? :-)

Se joka vanhoja muistelee, on ite se.

> In any case, I think the flat is used for a very similar purpose than the
> rest, I'd add "HAS_" here, too. I can write a patch.
> 
>>
>>> rectangles access from pads to streams. As the route is immutable,
>>> there's no need to implement the .set_routing() operation, and we can
>>> hardcode the sink and source stream IDs to 0.
>>
>> This doesn't implement .enable/disable_streams(), but continues using
>> .s_stream(). My understanding was that streams support requires
>> .enable/disable_streams(), but obviously the framework doesn't require this
>> at the moment.
>>
>> I encountered this while working on improving the
>> v4l2_subdev_enable/disable_streams(), and my code assumed that only
>> implementing .s_stream() means no streams support.
>>
>> So, how is it?
>>
>> I would say that .s_stream() is legacy, and streams enabled subdevices
>> should not implement (only) it. Streams enabled subdevices can use
>> v4l2_subdev_s_stream_helper for .s_stream, in addition to implementing
>> .enable/disable_streams().
> 
> I agree, if the driver supports streams, then it needs to implement the
> appropriate callbacks (i.e. not s_stream).

I've created a patch to check this. I'll send it shortly.

  Tomi

> 
>>
>>   Tomi
>>
>>> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>>> Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
>>> ---
>>> Changes since v6:
>>>
>>> - Drop change to get format API in imx219_set_ctrl()
>>> - Fix function name in commit message
>>> - Set V4L2_SUBDEV_ROUTE_FL_IMMUTABLE flag on route
>>> ---
>>>    drivers/media/i2c/imx219.c | 24 +++++++++++++++++++++++-
>>>    1 file changed, 23 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/media/i2c/imx219.c b/drivers/media/i2c/imx219.c
>>> index 817bf192e4d9..6602250834be 100644
>>> --- a/drivers/media/i2c/imx219.c
>>> +++ b/drivers/media/i2c/imx219.c
>>> @@ -993,15 +993,36 @@ static int imx219_get_selection(struct v4l2_subdev *sd,
>>>    static int imx219_init_state(struct v4l2_subdev *sd,
>>>    			     struct v4l2_subdev_state *state)
>>>    {
>>> +	struct v4l2_subdev_route routes[1] = {
>>> +		{
>>> +			.sink_pad = IMX219_PAD_IMAGE,
>>> +			.sink_stream = 0,
>>> +			.source_pad = IMX219_PAD_SOURCE,
>>> +			.source_stream = 0,
>>> +			.flags = V4L2_SUBDEV_ROUTE_FL_ACTIVE |
>>> +				 V4L2_SUBDEV_ROUTE_FL_IMMUTABLE,
>>> +		},
>>> +	};
>>> +	struct v4l2_subdev_krouting routing = {
>>> +		.len_routes = ARRAY_SIZE(routes),
>>> +		.num_routes = ARRAY_SIZE(routes),
>>> +		.routes = routes,
>>> +	};
>>>    	struct v4l2_subdev_format fmt = {
>>>    		.which = V4L2_SUBDEV_FORMAT_TRY,
>>>    		.pad = IMX219_PAD_SOURCE,
>>> +		.stream = 0,
>>>    		.format = {
>>>    			.code = MEDIA_BUS_FMT_SRGGB10_1X10,
>>>    			.width = supported_modes[0].width,
>>>    			.height = supported_modes[0].height,
>>>    		},
>>>    	};
>>> +	int ret;
>>> +
>>> +	ret = v4l2_subdev_set_routing(sd, state, &routing);
>>> +	if (ret)
>>> +		return ret;
>>>    	imx219_set_pad_format(sd, state, &fmt);
>>> @@ -1260,7 +1281,8 @@ static int imx219_probe(struct i2c_client *client)
>>>    	/* Initialize subdev */
>>>    	imx219->sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE |
>>> -			    V4L2_SUBDEV_FL_HAS_EVENTS;
>>> +			    V4L2_SUBDEV_FL_HAS_EVENTS |
>>> +			    V4L2_SUBDEV_FL_STREAMS;
>>>    	imx219->sd.entity.function = MEDIA_ENT_F_CAM_SENSOR;
>>>    	/*
>>
>