Message ID | 20240324220854.15010-4-laurent.pinchart@ideasonboard.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | media: Add driver for the Raspberry Pi <5 CSI-2 receiver | expand |
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; > > /*
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; > > /*
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; > > /* >
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; >>> /* >> >
Moikka, On Thu, Apr 04, 2024 at 08:29:36AM +0000, Sakari Ailus wrote: > On Thu, Apr 04, 2024 at 11:19:22AM +0300, Tomi Valkeinen wrote: > > 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. I'll review it :-) > > > 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). Works for me. > > > 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 --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; /*