Re: [PATCH v6 6/6] media: sun6i-csi: Add support for hooking to the isp devices

From: Paul Kocialkowski
Date: Thu Sep 01 2022 - 10:55:26 EST

Next message: Punit Agrawal: "Re: [PATCH v6] cpufreq: amd-pstate: cleanup the unused and duplicated headers declaration"
Previous message: Hans de Goede: "Re: [PATCH] platform/x86: dell-wmi: Add WMI event 0x0012 0x0003 to the list"
Next in thread: Laurent Pinchart: "Re: [PATCH v6 6/6] media: sun6i-csi: Add support for hooking to the isp devices"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

Hi Laurent,

On Sat 27 Aug 22, 01:44, Laurent Pinchart wrote:
> Hi Paul,
>
> Thank you for the patch.
>
> On Fri, Aug 26, 2022 at 08:41:44PM +0200, Paul Kocialkowski wrote:
> > In order to use the isp and csi together, both devices need to be
> > parented to the same v4l2 and media devices. We use the isp as
> > top-level device and let the csi code hook to its v4l2 and media
> > devices when async subdev registration takes place.
>
> Have you considered the option of making the CSI the master device, with
> the ISP registering an async subdev instead ?

Yes I did consider it but the issue is that some platforms using these drivers
have 2 CSI blocks and always 1 ISP block that can be fed from either CSI.
So if we want to have a single media device where we can switch the ISP between
the two CSIs, the only choice is to have the media/v4l2 devices registered
by the ISP driver.

For the next generation it would be absolutely necessary to have a single driver
using the component framework instead of separate drivers because the number of
components and routing capabilities are much more complex.

> I'm also wondering, what will happen if userspace tries to capture from
> both the CSI output and the ISP output at the same time ?

Well there's a media link to select where the sun6i-csi-bridge data should
flow, so if it's routed to the sun6i-isp-proc instead of sun6i-csi-capture
it should fail when trying to capture via sun6i-csi.

I'll double-check that this is actually the case, but I think
media_pipeline_start in sun6i_csi_capture_start_streaming should error out
in this case.

Cheers,

Paul

> > As a result v4l2/media device setup is only called when the ISP
> > is missing and the capture device is registered after the devices
> > are hooked. The bridge subdev and its notifier are registered
> > without any device when the ISP is available. Top-level pointers
> > for the devices are introduced to either redirect to the hooked ones
> > (isp available) or the registered ones (isp missing).
> >
> > Also keep track of whether the capture node was setup or not to
> > avoid cleaning up resources when it wasn't.
> >
> > Signed-off-by: Paul Kocialkowski <paul.kocialkowski@xxxxxxxxxxx>
> > ---
> > .../platform/sunxi/sun6i-csi/sun6i_csi.c | 45 +++++++++++++++----
> > .../platform/sunxi/sun6i-csi/sun6i_csi.h | 7 +++
> > .../sunxi/sun6i-csi/sun6i_csi_bridge.c | 32 +++++++++++--
> > .../sunxi/sun6i-csi/sun6i_csi_capture.c | 19 ++++++--
> > .../sunxi/sun6i-csi/sun6i_csi_capture.h | 1 +
> > 5 files changed, 89 insertions(+), 15 deletions(-)
> >
> > diff --git a/drivers/media/platform/sunxi/sun6i-csi/sun6i_csi.c b/drivers/media/platform/sunxi/sun6i-csi/sun6i_csi.c
> > index b16166cba2ef..0bac89d8112f 100644
> > --- a/drivers/media/platform/sunxi/sun6i-csi/sun6i_csi.c
> > +++ b/drivers/media/platform/sunxi/sun6i-csi/sun6i_csi.c
> > @@ -26,6 +26,18 @@
> >
> > /* ISP */
> >
> > +int sun6i_csi_isp_complete(struct sun6i_csi_device *csi_dev,
> > + struct v4l2_device *v4l2_dev)
> > +{
> > + if (csi_dev->v4l2_dev && csi_dev->v4l2_dev != v4l2_dev)
> > + return -EINVAL;
> > +
> > + csi_dev->v4l2_dev = v4l2_dev;
> > + csi_dev->media_dev = v4l2_dev->mdev;
> > +
> > + return sun6i_csi_capture_setup(csi_dev);
> > +}
> > +
> > static bool sun6i_csi_isp_detect(struct sun6i_csi_device *csi_dev)
> > {
> > struct device *dev = csi_dev->dev;
> > @@ -95,6 +107,9 @@ static int sun6i_csi_v4l2_setup(struct sun6i_csi_device *csi_dev)
> > goto error_media;
> > }
> >
> > + csi_dev->v4l2_dev = v4l2_dev;
> > + csi_dev->media_dev = media_dev;
> > +
> > return 0;
> >
> > error_media:
> > @@ -323,17 +338,27 @@ static int sun6i_csi_probe(struct platform_device *platform_dev)
> > if (ret)
> > goto error_resources;
> >
> > - ret = sun6i_csi_v4l2_setup(csi_dev);
> > - if (ret)
> > - goto error_resources;
> > + /*
> > + * Register our own v4l2 and media devices when there is no ISP around.
> > + * Otherwise the ISP will use async subdev registration with our bridge,
> > + * which will provide v4l2 and media devices that are used to register
> > + * the video interface.
> > + */
> > + if (!csi_dev->isp_available) {
> > + ret = sun6i_csi_v4l2_setup(csi_dev);
> > + if (ret)
> > + goto error_resources;
> > + }
> >
> > ret = sun6i_csi_bridge_setup(csi_dev);
> > if (ret)
> > goto error_v4l2;
> >
> > - ret = sun6i_csi_capture_setup(csi_dev);
> > - if (ret)
> > - goto error_bridge;
> > + if (!csi_dev->isp_available) {
> > + ret = sun6i_csi_capture_setup(csi_dev);
> > + if (ret)
> > + goto error_bridge;
> > + }
> >
> > return 0;
> >
> > @@ -341,7 +366,8 @@ static int sun6i_csi_probe(struct platform_device *platform_dev)
> > sun6i_csi_bridge_cleanup(csi_dev);
> >
> > error_v4l2:
> > - sun6i_csi_v4l2_cleanup(csi_dev);
> > + if (!csi_dev->isp_available)
> > + sun6i_csi_v4l2_cleanup(csi_dev);
> >
> > error_resources:
> > sun6i_csi_resources_cleanup(csi_dev);
> > @@ -355,7 +381,10 @@ static int sun6i_csi_remove(struct platform_device *pdev)
> >
> > sun6i_csi_capture_cleanup(csi_dev);
> > sun6i_csi_bridge_cleanup(csi_dev);
> > - sun6i_csi_v4l2_cleanup(csi_dev);
> > +
> > + if (!csi_dev->isp_available)
> > + sun6i_csi_v4l2_cleanup(csi_dev);
> > +
> > sun6i_csi_resources_cleanup(csi_dev);
> >
> > return 0;
> > diff --git a/drivers/media/platform/sunxi/sun6i-csi/sun6i_csi.h b/drivers/media/platform/sunxi/sun6i-csi/sun6i_csi.h
> > index 8e232cd91ebe..bc3f0dae35df 100644
> > --- a/drivers/media/platform/sunxi/sun6i-csi/sun6i_csi.h
> > +++ b/drivers/media/platform/sunxi/sun6i-csi/sun6i_csi.h
> > @@ -36,6 +36,8 @@ struct sun6i_csi_v4l2 {
> >
> > struct sun6i_csi_device {
> > struct device *dev;
> > + struct v4l2_device *v4l2_dev;
> > + struct media_device *media_dev;
> >
> > struct sun6i_csi_v4l2 v4l2;
> > struct sun6i_csi_bridge bridge;
> > @@ -53,4 +55,9 @@ struct sun6i_csi_variant {
> > unsigned long clock_mod_rate;
> > };
> >
> > +/* ISP */
> > +
> > +int sun6i_csi_isp_complete(struct sun6i_csi_device *csi_dev,
> > + struct v4l2_device *v4l2_dev);
> > +
> > #endif
> > diff --git a/drivers/media/platform/sunxi/sun6i-csi/sun6i_csi_bridge.c b/drivers/media/platform/sunxi/sun6i-csi/sun6i_csi_bridge.c
> > index 492f93b0db28..86d20c1c35ed 100644
> > --- a/drivers/media/platform/sunxi/sun6i-csi/sun6i_csi_bridge.c
> > +++ b/drivers/media/platform/sunxi/sun6i-csi/sun6i_csi_bridge.c
> > @@ -653,6 +653,7 @@ sun6i_csi_bridge_notifier_bound(struct v4l2_async_notifier *notifier,
> > struct sun6i_csi_bridge *bridge = &csi_dev->bridge;
> > struct sun6i_csi_bridge_source *source = bridge_async_subdev->source;
> > bool enabled;
> > + int ret;
> >
> > switch (source->endpoint.base.port) {
> > case SUN6I_CSI_PORT_PARALLEL:
> > @@ -667,6 +668,16 @@ sun6i_csi_bridge_notifier_bound(struct v4l2_async_notifier *notifier,
> >
> > source->subdev = remote_subdev;
> >
> > + if (csi_dev->isp_available) {
> > + /*
> > + * Hook to the first available remote subdev to get v4l2 and
> > + * media devices and register the capture device then.
> > + */
> > + ret = sun6i_csi_isp_complete(csi_dev, remote_subdev->v4l2_dev);
> > + if (ret)
> > + return ret;
> > + }
> > +
> > return sun6i_csi_bridge_link(csi_dev, SUN6I_CSI_BRIDGE_PAD_SINK,
> > remote_subdev, enabled);
> > }
> > @@ -679,6 +690,9 @@ sun6i_csi_bridge_notifier_complete(struct v4l2_async_notifier *notifier)
> > bridge.notifier);
> > struct v4l2_device *v4l2_dev = &csi_dev->v4l2.v4l2_dev;
> >
> > + if (csi_dev->isp_available)
> > + return 0;
> > +
> > return v4l2_device_register_subdev_nodes(v4l2_dev);
> > }
> >
> > @@ -752,7 +766,7 @@ int sun6i_csi_bridge_setup(struct sun6i_csi_device *csi_dev)
> > {
> > struct device *dev = csi_dev->dev;
> > struct sun6i_csi_bridge *bridge = &csi_dev->bridge;
> > - struct v4l2_device *v4l2_dev = &csi_dev->v4l2.v4l2_dev;
> > + struct v4l2_device *v4l2_dev = csi_dev->v4l2_dev;
> > struct v4l2_subdev *subdev = &bridge->subdev;
> > struct v4l2_async_notifier *notifier = &bridge->notifier;
> > struct media_pad *pads = bridge->pads;
> > @@ -793,7 +807,11 @@ int sun6i_csi_bridge_setup(struct sun6i_csi_device *csi_dev)
> >
> > /* V4L2 Subdev */
> >
> > - ret = v4l2_device_register_subdev(v4l2_dev, subdev);
> > + if (csi_dev->isp_available)
> > + ret = v4l2_async_register_subdev(subdev);
> > + else
> > + ret = v4l2_device_register_subdev(v4l2_dev, subdev);
> > +
> > if (ret) {
> > dev_err(dev, "failed to register v4l2 subdev: %d\n", ret);
> > goto error_media_entity;
> > @@ -810,7 +828,10 @@ int sun6i_csi_bridge_setup(struct sun6i_csi_device *csi_dev)
> > sun6i_csi_bridge_source_setup(csi_dev, &bridge->source_mipi_csi2,
> > SUN6I_CSI_PORT_MIPI_CSI2, NULL);
> >
> > - ret = v4l2_async_nf_register(v4l2_dev, notifier);
> > + if (csi_dev->isp_available)
> > + ret = v4l2_async_subdev_nf_register(subdev, notifier);
> > + else
> > + ret = v4l2_async_nf_register(v4l2_dev, notifier);
> > if (ret) {
> > dev_err(dev, "failed to register v4l2 async notifier: %d\n",
> > ret);
> > @@ -822,7 +843,10 @@ int sun6i_csi_bridge_setup(struct sun6i_csi_device *csi_dev)
> > error_v4l2_async_notifier:
> > v4l2_async_nf_cleanup(notifier);
> >
> > - v4l2_device_unregister_subdev(subdev);
> > + if (csi_dev->isp_available)
> > + v4l2_async_unregister_subdev(subdev);
> > + else
> > + v4l2_device_unregister_subdev(subdev);
> >
> > error_media_entity:
> > media_entity_cleanup(&subdev->entity);
> > diff --git a/drivers/media/platform/sunxi/sun6i-csi/sun6i_csi_capture.c b/drivers/media/platform/sunxi/sun6i-csi/sun6i_csi_capture.c
> > index c9e7526b84c4..69ea1cbaea0c 100644
> > --- a/drivers/media/platform/sunxi/sun6i-csi/sun6i_csi_capture.c
> > +++ b/drivers/media/platform/sunxi/sun6i-csi/sun6i_csi_capture.c
> > @@ -570,7 +570,7 @@ static int sun6i_csi_capture_buffer_prepare(struct vb2_buffer *buffer)
> > {
> > struct sun6i_csi_device *csi_dev = vb2_get_drv_priv(buffer->vb2_queue);
> > struct sun6i_csi_capture *capture = &csi_dev->capture;
> > - struct v4l2_device *v4l2_dev = &csi_dev->v4l2.v4l2_dev;
> > + struct v4l2_device *v4l2_dev = csi_dev->v4l2_dev;
> > struct vb2_v4l2_buffer *v4l2_buffer = to_vb2_v4l2_buffer(buffer);
> > unsigned long size = capture->format.fmt.pix.sizeimage;
> >
> > @@ -889,7 +889,7 @@ static int sun6i_csi_capture_link_validate(struct media_link *link)
> > struct video_device *video_dev =
> > media_entity_to_video_device(link->sink->entity);
> > struct sun6i_csi_device *csi_dev = video_get_drvdata(video_dev);
> > - struct v4l2_device *v4l2_dev = &csi_dev->v4l2.v4l2_dev;
> > + struct v4l2_device *v4l2_dev = csi_dev->v4l2_dev;
> > const struct sun6i_csi_capture_format *capture_format;
> > const struct sun6i_csi_bridge_format *bridge_format;
> > unsigned int capture_width, capture_height;
> > @@ -971,7 +971,7 @@ int sun6i_csi_capture_setup(struct sun6i_csi_device *csi_dev)
> > {
> > struct sun6i_csi_capture *capture = &csi_dev->capture;
> > struct sun6i_csi_capture_state *state = &capture->state;
> > - struct v4l2_device *v4l2_dev = &csi_dev->v4l2.v4l2_dev;
> > + struct v4l2_device *v4l2_dev = csi_dev->v4l2_dev;
> > struct v4l2_subdev *bridge_subdev = &csi_dev->bridge.subdev;
> > struct video_device *video_dev = &capture->video_dev;
> > struct vb2_queue *queue = &capture->queue;
> > @@ -980,6 +980,10 @@ int sun6i_csi_capture_setup(struct sun6i_csi_device *csi_dev)
> > struct v4l2_pix_format *pix_format = &format->fmt.pix;
> > int ret;
> >
> > + /* This may happen with multiple bridge notifier bound calls. */
> > + if (state->setup)
> > + return 0;
> > +
> > /* State */
> >
> > INIT_LIST_HEAD(&state->queue);
> > @@ -1055,6 +1059,7 @@ int sun6i_csi_capture_setup(struct sun6i_csi_device *csi_dev)
> > ret = media_create_pad_link(&bridge_subdev->entity,
> > SUN6I_CSI_BRIDGE_PAD_SOURCE,
> > &video_dev->entity, 0,
> > + csi_dev->isp_available ? 0 :
> > MEDIA_LNK_FL_ENABLED |
> > MEDIA_LNK_FL_IMMUTABLE);
> > if (ret < 0) {
> > @@ -1065,6 +1070,8 @@ int sun6i_csi_capture_setup(struct sun6i_csi_device *csi_dev)
> > goto error_video_device;
> > }
> >
> > + state->setup = true;
> > +
> > return 0;
> >
> > error_video_device:
> > @@ -1083,7 +1090,13 @@ void sun6i_csi_capture_cleanup(struct sun6i_csi_device *csi_dev)
> > struct sun6i_csi_capture *capture = &csi_dev->capture;
> > struct video_device *video_dev = &capture->video_dev;
> >
> > + /* This may happen if async registration failed to complete. */
> > + if (!capture->state.setup)
> > + return;
> > +
> > vb2_video_unregister_device(video_dev);
> > media_entity_cleanup(&video_dev->entity);
> > mutex_destroy(&capture->lock);
> > +
> > + capture->state.setup = false;
> > }
> > diff --git a/drivers/media/platform/sunxi/sun6i-csi/sun6i_csi_capture.h b/drivers/media/platform/sunxi/sun6i-csi/sun6i_csi_capture.h
> > index 29893cf96f6b..3ee5ccefbd10 100644
> > --- a/drivers/media/platform/sunxi/sun6i-csi/sun6i_csi_capture.h
> > +++ b/drivers/media/platform/sunxi/sun6i-csi/sun6i_csi_capture.h
> > @@ -45,6 +45,7 @@ struct sun6i_csi_capture_state {
> >
> > unsigned int sequence;
> > bool streaming;
> > + bool setup;
> > };
> >
> > struct sun6i_csi_capture {
>
> --
> Regards,
>
> Laurent Pinchart

--
Paul Kocialkowski, Bootlin
Embedded Linux and kernel engineering
https://bootlin.com

Attachment: signature.asc
Description: PGP signature

Next message: Punit Agrawal: "Re: [PATCH v6] cpufreq: amd-pstate: cleanup the unused and duplicated headers declaration"
Previous message: Hans de Goede: "Re: [PATCH] platform/x86: dell-wmi: Add WMI event 0x0012 0x0003 to the list"
Next in thread: Laurent Pinchart: "Re: [PATCH v6 6/6] media: sun6i-csi: Add support for hooking to the isp devices"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]