diff mbox series

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

Message ID 20240324220854.15010-4-laurent.pinchart@ideasonboard.com
State New
Headers show
Series media: Add driver for the Raspberry Pi <5 CSI-2 receiver | expand

Commit Message

Laurent Pinchart March 24, 2024, 10:08 p.m. UTC 
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
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.

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(-)

Comments

Tomi Valkeinen March 27, 2024, 9:56 a.m. UTC | #1 
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
> rectangles access from pads to streams. As the route is immutable,

No "switch formats and selection rectangles access from pads to stream" 
in this patch.

  Tomi

> there's no need to implement the .set_routing() operation, and we can
> hardcode the sink and source stream IDs to 0.
> 
> 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;
>   
>   	/*
Tomi Valkeinen April 4, 2024, 8:19 a.m. UTC | #2 
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_).

> 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().

  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;
>   
>   	/*
Sakari Ailus April 4, 2024, 8:29 a.m. UTC | #3 
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? :-)

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).

> 
>  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;
> >   	/*
>
Tomi Valkeinen April 4, 2024, 10:29 a.m. UTC | #4 
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;
>>>    	/*
>>
>
diff mbox series

Patch

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;
 
 	/*