| Message ID | 20240430103956.60190-4-jacopo.mondi@ideasonboard.com (mailing list archive) |
|---|---|
| State | New |
| Delegated to: | Kieran Bingham |
| Headers | show |
| Series | [01/19] media: adv748x: Add support for active state | expand |
Hi Jacopo, Thank you for the patch. On Tue, Apr 30, 2024 at 12:39:39PM +0200, Jacopo Mondi wrote: > Initialize the CSI-2 subdevice with the V4L2_SUBDEV_FL_STREAMS flag > and initialize a simple routing table by implementing the .init_state() > operation. > > Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com> > --- > drivers/media/i2c/adv748x/adv748x-csi2.c | 28 ++++++++++++++++++++++-- > 1 file changed, 26 insertions(+), 2 deletions(-) > > diff --git a/drivers/media/i2c/adv748x/adv748x-csi2.c b/drivers/media/i2c/adv748x/adv748x-csi2.c > index 60bf1dc0f58b..d929db7e8ef2 100644 > --- a/drivers/media/i2c/adv748x/adv748x-csi2.c > +++ b/drivers/media/i2c/adv748x/adv748x-csi2.c > @@ -59,7 +59,30 @@ static int adv748x_csi2_register_link(struct adv748x_csi2 *tx, > > /* ----------------------------------------------------------------------------- > * v4l2_subdev_internal_ops > - * > + */ > + > +static int adv748x_csi2_init_state(struct v4l2_subdev *sd, > + struct v4l2_subdev_state *state) > +{ > + struct v4l2_subdev_route routes[] = { > + { > + .sink_pad = ADV748X_CSI2_SINK, > + .sink_stream = 0, > + .source_pad = ADV748X_CSI2_SOURCE, > + .source_stream = 0, > + .flags = V4L2_SUBDEV_ROUTE_FL_ACTIVE, > + }, > + }; > + > + struct v4l2_subdev_krouting routing = { > + .num_routes = ARRAY_SIZE(routes), > + .routes = routes, > + }; > + > + return v4l2_subdev_set_routing(sd, state, &routing); You need to initialize formats too. > +} > + > +/* > * We use the internal registered operation to be able to ensure that our > * incremental subdevices (not connected in the forward path) can be registered > * against the resulting video path and media device. > @@ -109,6 +132,7 @@ static int adv748x_csi2_registered(struct v4l2_subdev *sd) > } > > static const struct v4l2_subdev_internal_ops adv748x_csi2_internal_ops = { > + .init_state = adv748x_csi2_init_state, The .init_state() operation needs to be provided along with the call to v4l2_subdev_init_finalize() in patch 01/19. > .registered = adv748x_csi2_registered, > }; > > @@ -245,7 +269,7 @@ int adv748x_csi2_init(struct adv748x_state *state, struct adv748x_csi2 *tx) > return 0; > > adv748x_subdev_init(&tx->sd, state, &adv748x_csi2_ops, > - MEDIA_ENT_F_VID_IF_BRIDGE, 0, > + MEDIA_ENT_F_VID_IF_BRIDGE, V4L2_SUBDEV_FL_STREAMS, > is_txa(tx) ? "txa" : "txb"); > > /* Register internal ops for incremental subdev registration */ > -- > 2.44.0 >
Hi Laurent On Thu, May 02, 2024 at 08:40:51PM GMT, Laurent Pinchart wrote: > Hi Jacopo, > > Thank you for the patch. > > On Tue, Apr 30, 2024 at 12:39:39PM +0200, Jacopo Mondi wrote: > > Initialize the CSI-2 subdevice with the V4L2_SUBDEV_FL_STREAMS flag > > and initialize a simple routing table by implementing the .init_state() > > operation. > > > > Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com> > > --- > > drivers/media/i2c/adv748x/adv748x-csi2.c | 28 ++++++++++++++++++++++-- > > 1 file changed, 26 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/media/i2c/adv748x/adv748x-csi2.c b/drivers/media/i2c/adv748x/adv748x-csi2.c > > index 60bf1dc0f58b..d929db7e8ef2 100644 > > --- a/drivers/media/i2c/adv748x/adv748x-csi2.c > > +++ b/drivers/media/i2c/adv748x/adv748x-csi2.c > > @@ -59,7 +59,30 @@ static int adv748x_csi2_register_link(struct adv748x_csi2 *tx, > > > > /* ----------------------------------------------------------------------------- > > * v4l2_subdev_internal_ops > > - * > > + */ > > + > > +static int adv748x_csi2_init_state(struct v4l2_subdev *sd, > > + struct v4l2_subdev_state *state) > > +{ > > + struct v4l2_subdev_route routes[] = { > > + { > > + .sink_pad = ADV748X_CSI2_SINK, > > + .sink_stream = 0, > > + .source_pad = ADV748X_CSI2_SOURCE, > > + .source_stream = 0, > > + .flags = V4L2_SUBDEV_ROUTE_FL_ACTIVE, > > + }, > > + }; > > + > > + struct v4l2_subdev_krouting routing = { > > + .num_routes = ARRAY_SIZE(routes), > > + .routes = routes, > > + }; > > + > > + return v4l2_subdev_set_routing(sd, state, &routing); > > You need to initialize formats too. > The adv748x driver handles formats very poorly, doesn't implement enum_mbus_codes and does not allow userspace to change the format (while at the same time it doesn't check that the format is the expected one in set_format()). This is from a freshly booted renesas-drivers/main - entity 30: adv748x 4-0070 txa (2 pads, 3 links, 0 routes) type V4L2 subdev subtype Unknown flags 0 device node name /dev/v4l-subdev5 pad0: Sink [stream:0 fmt:unknown/0x0] <- "adv748x 4-0070 afe":8 [] <- "adv748x 4-0070 hdmi":1 [ENABLED] pad1: Source [stream:0 fmt:unknown/0x0] -> "rcar_csi2 feaa0000.csi2":0 [ENABLED,IMMUTABLE] It would probably be better to handle the formats properly and the introduce streams or use the introduction of streams to also fix the format handling ? > > +} > > + > > +/* > > * We use the internal registered operation to be able to ensure that our > > * incremental subdevices (not connected in the forward path) can be registered > > * against the resulting video path and media device. > > @@ -109,6 +132,7 @@ static int adv748x_csi2_registered(struct v4l2_subdev *sd) > > } > > > > static const struct v4l2_subdev_internal_ops adv748x_csi2_internal_ops = { > > + .init_state = adv748x_csi2_init_state, > > The .init_state() operation needs to be provided along with the call to > v4l2_subdev_init_finalize() in patch 01/19. > I'll squash, however even if it might be a requirement for having a fully working implementation, not having init_state() will not lead to any crash and maybe smaller incremental patches are easier to handle. if (sd->internal_ops && sd->internal_ops->init_state) { /* * There can be no race at this point, but we lock the state * anyway to satisfy lockdep checks. */ v4l2_subdev_lock_state(state); ret = sd->internal_ops->init_state(sd, state); v4l2_subdev_unlock_state(state); > > .registered = adv748x_csi2_registered, > > }; > > > > @@ -245,7 +269,7 @@ int adv748x_csi2_init(struct adv748x_state *state, struct adv748x_csi2 *tx) > > return 0; > > > > adv748x_subdev_init(&tx->sd, state, &adv748x_csi2_ops, > > - MEDIA_ENT_F_VID_IF_BRIDGE, 0, > > + MEDIA_ENT_F_VID_IF_BRIDGE, V4L2_SUBDEV_FL_STREAMS, > > is_txa(tx) ? "txa" : "txb"); > > > > /* Register internal ops for incremental subdev registration */ > > -- > > 2.44.0 > > > > -- > Regards, > > Laurent Pinchart
Hi Jacopo, On Fri, May 03, 2024 at 09:59:55AM +0200, Jacopo Mondi wrote: > On Thu, May 02, 2024 at 08:40:51PM GMT, Laurent Pinchart wrote: > > On Tue, Apr 30, 2024 at 12:39:39PM +0200, Jacopo Mondi wrote: > > > Initialize the CSI-2 subdevice with the V4L2_SUBDEV_FL_STREAMS flag > > > and initialize a simple routing table by implementing the .init_state() > > > operation. > > > > > > Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com> > > > --- > > > drivers/media/i2c/adv748x/adv748x-csi2.c | 28 ++++++++++++++++++++++-- > > > 1 file changed, 26 insertions(+), 2 deletions(-) > > > > > > diff --git a/drivers/media/i2c/adv748x/adv748x-csi2.c b/drivers/media/i2c/adv748x/adv748x-csi2.c > > > index 60bf1dc0f58b..d929db7e8ef2 100644 > > > --- a/drivers/media/i2c/adv748x/adv748x-csi2.c > > > +++ b/drivers/media/i2c/adv748x/adv748x-csi2.c > > > @@ -59,7 +59,30 @@ static int adv748x_csi2_register_link(struct adv748x_csi2 *tx, > > > > > > /* ----------------------------------------------------------------------------- > > > * v4l2_subdev_internal_ops > > > - * > > > + */ > > > + > > > +static int adv748x_csi2_init_state(struct v4l2_subdev *sd, > > > + struct v4l2_subdev_state *state) > > > +{ > > > + struct v4l2_subdev_route routes[] = { > > > + { > > > + .sink_pad = ADV748X_CSI2_SINK, > > > + .sink_stream = 0, > > > + .source_pad = ADV748X_CSI2_SOURCE, > > > + .source_stream = 0, > > > + .flags = V4L2_SUBDEV_ROUTE_FL_ACTIVE, > > > + }, > > > + }; > > > + > > > + struct v4l2_subdev_krouting routing = { > > > + .num_routes = ARRAY_SIZE(routes), > > > + .routes = routes, > > > + }; > > > + > > > + return v4l2_subdev_set_routing(sd, state, &routing); > > > > You need to initialize formats too. > > > > The adv748x driver handles formats very poorly, doesn't implement > enum_mbus_codes and does not allow userspace to change the format > (while at the same time it doesn't check that the format is the > expected one in set_format()). > > This is from a freshly booted renesas-drivers/main > > - entity 30: adv748x 4-0070 txa (2 pads, 3 links, 0 routes) > type V4L2 subdev subtype Unknown flags 0 > device node name /dev/v4l-subdev5 > pad0: Sink > [stream:0 fmt:unknown/0x0] > <- "adv748x 4-0070 afe":8 [] > <- "adv748x 4-0070 hdmi":1 [ENABLED] > pad1: Source > [stream:0 fmt:unknown/0x0] > -> "rcar_csi2 feaa0000.csi2":0 [ENABLED,IMMUTABLE] > > It would probably be better to handle the formats properly and the > introduce streams or use the introduction of streams to also fix the > format handling ? As Niklas pointed out in the review of some patches, fixing issues first, and moving to the active subdev state, would be better done before adding streams in my opinion. At least if those fixes are not too difficult without streams. For this specific patch, the addition of the .init_state() operation should be squashed with 01/19, without routing, and routing should be added on top. > > > +} > > > + > > > +/* > > > * We use the internal registered operation to be able to ensure that our > > > * incremental subdevices (not connected in the forward path) can be registered > > > * against the resulting video path and media device. > > > @@ -109,6 +132,7 @@ static int adv748x_csi2_registered(struct v4l2_subdev *sd) > > > } > > > > > > static const struct v4l2_subdev_internal_ops adv748x_csi2_internal_ops = { > > > + .init_state = adv748x_csi2_init_state, > > > > The .init_state() operation needs to be provided along with the call to > > v4l2_subdev_init_finalize() in patch 01/19. > > I'll squash, however even if it might be a requirement for having a > fully working implementation, not having init_state() will not lead to > any crash and maybe smaller incremental patches are easier to handle. > > if (sd->internal_ops && sd->internal_ops->init_state) { > /* > * There can be no race at this point, but we lock the state > * anyway to satisfy lockdep checks. > */ > v4l2_subdev_lock_state(state); > ret = sd->internal_ops->init_state(sd, state); > v4l2_subdev_unlock_state(state); I think it's a mistake in the core to not require .init_state() for subdevs using the active state. Tomi, what do you think ? > > > .registered = adv748x_csi2_registered, > > > }; > > > > > > @@ -245,7 +269,7 @@ int adv748x_csi2_init(struct adv748x_state *state, struct adv748x_csi2 *tx) > > > return 0; > > > > > > adv748x_subdev_init(&tx->sd, state, &adv748x_csi2_ops, > > > - MEDIA_ENT_F_VID_IF_BRIDGE, 0, > > > + MEDIA_ENT_F_VID_IF_BRIDGE, V4L2_SUBDEV_FL_STREAMS, > > > is_txa(tx) ? "txa" : "txb"); > > > > > > /* Register internal ops for incremental subdev registration */
On 03/05/2024 11:31, Laurent Pinchart wrote: >>>> +} >>>> + >>>> +/* >>>> * We use the internal registered operation to be able to ensure that our >>>> * incremental subdevices (not connected in the forward path) can be registered >>>> * against the resulting video path and media device. >>>> @@ -109,6 +132,7 @@ static int adv748x_csi2_registered(struct v4l2_subdev *sd) >>>> } >>>> >>>> static const struct v4l2_subdev_internal_ops adv748x_csi2_internal_ops = { >>>> + .init_state = adv748x_csi2_init_state, >>> >>> The .init_state() operation needs to be provided along with the call to >>> v4l2_subdev_init_finalize() in patch 01/19. >> >> I'll squash, however even if it might be a requirement for having a >> fully working implementation, not having init_state() will not lead to >> any crash and maybe smaller incremental patches are easier to handle. >> >> if (sd->internal_ops && sd->internal_ops->init_state) { >> /* >> * There can be no race at this point, but we lock the state >> * anyway to satisfy lockdep checks. >> */ >> v4l2_subdev_lock_state(state); >> ret = sd->internal_ops->init_state(sd, state); >> v4l2_subdev_unlock_state(state); > > I think it's a mistake in the core to not require .init_state() for > subdevs using the active state. Tomi, what do you think ? If I'm not mistaken, the v4l2 rules say that a subdev configuration should always be a valid one (for that specific device). To fulfill that, you need .init_state(). So yes, I agree. This is probably one more thing that can be added to the "[PATCH v6 03/11] media: subdev: Add checks for subdev features". Tomi
diff --git a/drivers/media/i2c/adv748x/adv748x-csi2.c b/drivers/media/i2c/adv748x/adv748x-csi2.c index 60bf1dc0f58b..d929db7e8ef2 100644 --- a/drivers/media/i2c/adv748x/adv748x-csi2.c +++ b/drivers/media/i2c/adv748x/adv748x-csi2.c @@ -59,7 +59,30 @@ static int adv748x_csi2_register_link(struct adv748x_csi2 *tx, /* ----------------------------------------------------------------------------- * v4l2_subdev_internal_ops - * + */ + +static int adv748x_csi2_init_state(struct v4l2_subdev *sd, + struct v4l2_subdev_state *state) +{ + struct v4l2_subdev_route routes[] = { + { + .sink_pad = ADV748X_CSI2_SINK, + .sink_stream = 0, + .source_pad = ADV748X_CSI2_SOURCE, + .source_stream = 0, + .flags = V4L2_SUBDEV_ROUTE_FL_ACTIVE, + }, + }; + + struct v4l2_subdev_krouting routing = { + .num_routes = ARRAY_SIZE(routes), + .routes = routes, + }; + + return v4l2_subdev_set_routing(sd, state, &routing); +} + +/* * We use the internal registered operation to be able to ensure that our * incremental subdevices (not connected in the forward path) can be registered * against the resulting video path and media device. @@ -109,6 +132,7 @@ static int adv748x_csi2_registered(struct v4l2_subdev *sd) } static const struct v4l2_subdev_internal_ops adv748x_csi2_internal_ops = { + .init_state = adv748x_csi2_init_state, .registered = adv748x_csi2_registered, }; @@ -245,7 +269,7 @@ int adv748x_csi2_init(struct adv748x_state *state, struct adv748x_csi2 *tx) return 0; adv748x_subdev_init(&tx->sd, state, &adv748x_csi2_ops, - MEDIA_ENT_F_VID_IF_BRIDGE, 0, + MEDIA_ENT_F_VID_IF_BRIDGE, V4L2_SUBDEV_FL_STREAMS, is_txa(tx) ? "txa" : "txb"); /* Register internal ops for incremental subdev registration */
Initialize the CSI-2 subdevice with the V4L2_SUBDEV_FL_STREAMS flag and initialize a simple routing table by implementing the .init_state() operation. Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com> --- drivers/media/i2c/adv748x/adv748x-csi2.c | 28 ++++++++++++++++++++++-- 1 file changed, 26 insertions(+), 2 deletions(-)