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@xxxxxxxxxxxxxxxx>
> > > Reviewed-by: Jacopo Mondi <jacopo.mondi@xxxxxxxxxxxxxxxx>
> > > ---
> > > 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;
> > > /*
--
Regards,
Laurent Pinchart
