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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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@xxxxxxxxxxxxxxxx>
> > ---
> >  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




[Index of Archives]     [Linux Input]     [Video for Linux]     [Gstreamer Embedded]     [Mplayer Users]     [Linux USB Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [Yosemite Backpacking]

  Powered by Linux