Re: [PATCH 03/19] media: adv748x: Use V4L2 streams

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

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

-- 
Regards,

Laurent Pinchart