| Message ID | 20220826184144.605605-1-paul.kocialkowski@bootlin.com |
|---|---|
| Headers | show |
| Series | Allwinner A31/A83T MIPI CSI-2 and A31 ISP / ISP Driver | expand |
Hi Paul, Thank you for the patch. On Fri, Aug 26, 2022 at 08:41:42PM +0200, Paul Kocialkowski wrote: > Add myself as maintainer of the Allwinner A31 ISP media driver. > > Signed-off-by: Paul Kocialkowski <paul.kocialkowski@bootlin.com> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > --- > MAINTAINERS | 9 +++++++++ > 1 file changed, 9 insertions(+) > > diff --git a/MAINTAINERS b/MAINTAINERS > index 0bd7b7d14f08..571348195560 100644 > --- a/MAINTAINERS > +++ b/MAINTAINERS > @@ -787,6 +787,15 @@ T: git git://linuxtv.org/media_tree.git > F: Documentation/devicetree/bindings/media/allwinner,sun6i-a31-csi.yaml > F: drivers/media/platform/sunxi/sun6i-csi/ > > +ALLWINNER A31 ISP DRIVER > +M: Paul Kocialkowski <paul.kocialkowski@bootlin.com> > +L: linux-media@vger.kernel.org > +S: Maintained > +T: git git://linuxtv.org/media_tree.git > +F: Documentation/devicetree/bindings/media/allwinner,sun6i-a31-isp.yaml > +F: drivers/staging/media/sunxi/sun6i-isp/ > +F: drivers/staging/media/sunxi/sun6i-isp/uapi/sun6i-isp-config.h > + > ALLWINNER A31 MIPI CSI-2 BRIDGE DRIVER > M: Paul Kocialkowski <paul.kocialkowski@bootlin.com> > L: linux-media@vger.kernel.org
Hi Paul, Thank you for the patch. On Fri, Aug 26, 2022 at 08:41:43PM +0200, Paul Kocialkowski wrote: > Add a helper to detect whether the ISP is available and connected > and store the indication in a driver-wide variable. This sounds like it would be a global variable, while it's stored in the driver-specific device structure. > > Signed-off-by: Paul Kocialkowski <paul.kocialkowski@bootlin.com> > --- > .../platform/sunxi/sun6i-csi/sun6i_csi.c | 33 +++++++++++++++++++ > .../platform/sunxi/sun6i-csi/sun6i_csi.h | 3 ++ > 2 files changed, 36 insertions(+) > > diff --git a/drivers/media/platform/sunxi/sun6i-csi/sun6i_csi.c b/drivers/media/platform/sunxi/sun6i-csi/sun6i_csi.c > index 00521f966cee..b16166cba2ef 100644 > --- a/drivers/media/platform/sunxi/sun6i-csi/sun6i_csi.c > +++ b/drivers/media/platform/sunxi/sun6i-csi/sun6i_csi.c > @@ -24,6 +24,35 @@ > #include "sun6i_csi_capture.h" > #include "sun6i_csi_reg.h" > > +/* ISP */ > + > +static bool sun6i_csi_isp_detect(struct sun6i_csi_device *csi_dev) > +{ > + struct device *dev = csi_dev->dev; > + struct fwnode_handle *handle = NULL; No need to initialize this to NULL. > + > + /* ISP is not available if disabled in kernel config. */ > + if (!IS_ENABLED(CONFIG_VIDEO_SUN6I_ISP)) > + return 0; Hmmm... The ISP driver may be disabled when compiling the sun6i-csi driver, but later enabled and deployed. Disabling ISP support silently like this could be confusing. Could it be better to move this check after the graph check, and print a warning message in this case ? > + > + /* > + * ISP is not available if not connected via fwnode graph. > + * This weill also check that the remote parent node is available. s/weill/will/ * ISP is not available if not connected via fwnode graph. This will * also check that the remote parent node is available. > + */ > + handle = fwnode_graph_get_endpoint_by_id(dev_fwnode(dev), > + SUN6I_CSI_PORT_ISP, 0, > + FWNODE_GRAPH_ENDPOINT_NEXT); > + if (!handle) > + return 0; > + > + fwnode_handle_put(handle); > + > + dev_info(dev, "ISP link is available\n"); You could make that a debug message, it's not crucial information that needs to be printed when the driver is loaded. If you prefer keeping an info message, then I'd move it to the probe function and print that the CSI has been probed, and indicate in that message if the ISP is available. > + csi_dev->isp_available = true; > + > + return 0; > +} > + > /* Media */ > > static const struct media_device_ops sun6i_csi_media_ops = { > @@ -290,6 +319,10 @@ static int sun6i_csi_probe(struct platform_device *platform_dev) > if (ret) > return ret; > > + ret = sun6i_csi_isp_detect(csi_dev); > + if (ret) > + goto error_resources; > + > ret = sun6i_csi_v4l2_setup(csi_dev); > if (ret) > goto error_resources; > diff --git a/drivers/media/platform/sunxi/sun6i-csi/sun6i_csi.h b/drivers/media/platform/sunxi/sun6i-csi/sun6i_csi.h > index e611bdd6e9b2..8e232cd91ebe 100644 > --- a/drivers/media/platform/sunxi/sun6i-csi/sun6i_csi.h > +++ b/drivers/media/platform/sunxi/sun6i-csi/sun6i_csi.h > @@ -21,6 +21,7 @@ > enum sun6i_csi_port { > SUN6I_CSI_PORT_PARALLEL = 0, > SUN6I_CSI_PORT_MIPI_CSI2 = 1, > + SUN6I_CSI_PORT_ISP = 2, > }; > > struct sun6i_csi_buffer { > @@ -44,6 +45,8 @@ struct sun6i_csi_device { > struct clk *clock_mod; > struct clk *clock_ram; > struct reset_control *reset; > + > + bool isp_available; > }; > > struct sun6i_csi_variant {
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 ? 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 ? > 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@bootlin.com> > --- > .../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 {
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@bootlin.com> > > --- > > .../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
Hi Laurent, On Sat 27 Aug 22, 01:39, Laurent Pinchart wrote: > Hi Paul, > > Thank you for the patch. Thanks for the review! > On Fri, Aug 26, 2022 at 08:41:43PM +0200, Paul Kocialkowski wrote: > > Add a helper to detect whether the ISP is available and connected > > and store the indication in a driver-wide variable. > > This sounds like it would be a global variable, while it's stored in the > driver-specific device structure. Okay I can clarify the commit message here. > > > > Signed-off-by: Paul Kocialkowski <paul.kocialkowski@bootlin.com> > > --- > > .../platform/sunxi/sun6i-csi/sun6i_csi.c | 33 +++++++++++++++++++ > > .../platform/sunxi/sun6i-csi/sun6i_csi.h | 3 ++ > > 2 files changed, 36 insertions(+) > > > > diff --git a/drivers/media/platform/sunxi/sun6i-csi/sun6i_csi.c b/drivers/media/platform/sunxi/sun6i-csi/sun6i_csi.c > > index 00521f966cee..b16166cba2ef 100644 > > --- a/drivers/media/platform/sunxi/sun6i-csi/sun6i_csi.c > > +++ b/drivers/media/platform/sunxi/sun6i-csi/sun6i_csi.c > > @@ -24,6 +24,35 @@ > > #include "sun6i_csi_capture.h" > > #include "sun6i_csi_reg.h" > > > > +/* ISP */ > > + > > +static bool sun6i_csi_isp_detect(struct sun6i_csi_device *csi_dev) > > +{ > > + struct device *dev = csi_dev->dev; > > + struct fwnode_handle *handle = NULL; > > No need to initialize this to NULL. Indeed. > > + > > + /* ISP is not available if disabled in kernel config. */ > > + if (!IS_ENABLED(CONFIG_VIDEO_SUN6I_ISP)) > > + return 0; > > Hmmm... The ISP driver may be disabled when compiling the sun6i-csi > driver, but later enabled and deployed. Disabling ISP support silently > like this could be confusing. Could it be better to move this check > after the graph check, and print a warning message in this case ? Yeah I'm not too surprised corner cases like this can exist. Agreed that printing a warning message would be good, but I don't follow the point of moving the check later on. Do you have something in mind there? > > + > > + /* > > + * ISP is not available if not connected via fwnode graph. > > + * This weill also check that the remote parent node is available. > > s/weill/will/ Good catch, thanks! > * ISP is not available if not connected via fwnode graph. This will > * also check that the remote parent node is available. > > > + */ > > + handle = fwnode_graph_get_endpoint_by_id(dev_fwnode(dev), > > + SUN6I_CSI_PORT_ISP, 0, > > + FWNODE_GRAPH_ENDPOINT_NEXT); > > + if (!handle) > > + return 0; > > + > > + fwnode_handle_put(handle); > > + > > + dev_info(dev, "ISP link is available\n"); > > You could make that a debug message, it's not crucial information that > needs to be printed when the driver is loaded. If you prefer keeping an > info message, then I'd move it to the probe function and print that the > CSI has been probed, and indicate in that message if the ISP is > available. You're right, let's make this debug. It's more the opposite case that is worth a warning message. Thanks, Paul > > + csi_dev->isp_available = true; > > + > > + return 0; > > +} > > + > > /* Media */ > > > > static const struct media_device_ops sun6i_csi_media_ops = { > > @@ -290,6 +319,10 @@ static int sun6i_csi_probe(struct platform_device *platform_dev) > > if (ret) > > return ret; > > > > + ret = sun6i_csi_isp_detect(csi_dev); > > + if (ret) > > + goto error_resources; > > + > > ret = sun6i_csi_v4l2_setup(csi_dev); > > if (ret) > > goto error_resources; > > diff --git a/drivers/media/platform/sunxi/sun6i-csi/sun6i_csi.h b/drivers/media/platform/sunxi/sun6i-csi/sun6i_csi.h > > index e611bdd6e9b2..8e232cd91ebe 100644 > > --- a/drivers/media/platform/sunxi/sun6i-csi/sun6i_csi.h > > +++ b/drivers/media/platform/sunxi/sun6i-csi/sun6i_csi.h > > @@ -21,6 +21,7 @@ > > enum sun6i_csi_port { > > SUN6I_CSI_PORT_PARALLEL = 0, > > SUN6I_CSI_PORT_MIPI_CSI2 = 1, > > + SUN6I_CSI_PORT_ISP = 2, > > }; > > > > struct sun6i_csi_buffer { > > @@ -44,6 +45,8 @@ struct sun6i_csi_device { > > struct clk *clock_mod; > > struct clk *clock_ram; > > struct reset_control *reset; > > + > > + bool isp_available; > > }; > > > > struct sun6i_csi_variant { > > -- > Regards, > > Laurent Pinchart
Hi Paul, On Thu, Sep 01, 2022 at 05:11:00PM +0200, Paul Kocialkowski wrote: > On Sat 27 Aug 22, 01:39, Laurent Pinchart wrote: > > On Fri, Aug 26, 2022 at 08:41:43PM +0200, Paul Kocialkowski wrote: > > > Add a helper to detect whether the ISP is available and connected > > > and store the indication in a driver-wide variable. > > > > This sounds like it would be a global variable, while it's stored in the > > driver-specific device structure. > > Okay I can clarify the commit message here. > > > > > > > Signed-off-by: Paul Kocialkowski <paul.kocialkowski@bootlin.com> > > > --- > > > .../platform/sunxi/sun6i-csi/sun6i_csi.c | 33 +++++++++++++++++++ > > > .../platform/sunxi/sun6i-csi/sun6i_csi.h | 3 ++ > > > 2 files changed, 36 insertions(+) > > > > > > diff --git a/drivers/media/platform/sunxi/sun6i-csi/sun6i_csi.c b/drivers/media/platform/sunxi/sun6i-csi/sun6i_csi.c > > > index 00521f966cee..b16166cba2ef 100644 > > > --- a/drivers/media/platform/sunxi/sun6i-csi/sun6i_csi.c > > > +++ b/drivers/media/platform/sunxi/sun6i-csi/sun6i_csi.c > > > @@ -24,6 +24,35 @@ > > > #include "sun6i_csi_capture.h" > > > #include "sun6i_csi_reg.h" > > > > > > +/* ISP */ > > > + > > > +static bool sun6i_csi_isp_detect(struct sun6i_csi_device *csi_dev) > > > +{ > > > + struct device *dev = csi_dev->dev; > > > + struct fwnode_handle *handle = NULL; > > > > No need to initialize this to NULL. > > Indeed. > > > > + > > > + /* ISP is not available if disabled in kernel config. */ > > > + if (!IS_ENABLED(CONFIG_VIDEO_SUN6I_ISP)) > > > + return 0; > > > > Hmmm... The ISP driver may be disabled when compiling the sun6i-csi > > driver, but later enabled and deployed. Disabling ISP support silently > > like this could be confusing. Could it be better to move this check > > after the graph check, and print a warning message in this case ? > > Yeah I'm not too surprised corner cases like this can exist. > Agreed that printing a warning message would be good, but I don't follow the > point of moving the check later on. Do you have something in mind there? I meant that the warning should only be printed of there's an endpoint connected to the ISP, so I'd first check if the endpoint is present, return 0 if it isn't, and then check if the ISP driver is enabled and print a warning if it isn't. > > > + > > > + /* > > > + * ISP is not available if not connected via fwnode graph. > > > + * This weill also check that the remote parent node is available. > > > > s/weill/will/ > > Good catch, thanks! > > > * ISP is not available if not connected via fwnode graph. This will > > * also check that the remote parent node is available. > > > > > + */ > > > + handle = fwnode_graph_get_endpoint_by_id(dev_fwnode(dev), > > > + SUN6I_CSI_PORT_ISP, 0, > > > + FWNODE_GRAPH_ENDPOINT_NEXT); > > > + if (!handle) > > > + return 0; > > > + > > > + fwnode_handle_put(handle); > > > + > > > + dev_info(dev, "ISP link is available\n"); > > > > You could make that a debug message, it's not crucial information that > > needs to be printed when the driver is loaded. If you prefer keeping an > > info message, then I'd move it to the probe function and print that the > > CSI has been probed, and indicate in that message if the ISP is > > available. > > You're right, let's make this debug. It's more the opposite case that is worth > a warning message. > > > > + csi_dev->isp_available = true; > > > + > > > + return 0; > > > +} > > > + > > > /* Media */ > > > > > > static const struct media_device_ops sun6i_csi_media_ops = { > > > @@ -290,6 +319,10 @@ static int sun6i_csi_probe(struct platform_device *platform_dev) > > > if (ret) > > > return ret; > > > > > > + ret = sun6i_csi_isp_detect(csi_dev); > > > + if (ret) > > > + goto error_resources; > > > + > > > ret = sun6i_csi_v4l2_setup(csi_dev); > > > if (ret) > > > goto error_resources; > > > diff --git a/drivers/media/platform/sunxi/sun6i-csi/sun6i_csi.h b/drivers/media/platform/sunxi/sun6i-csi/sun6i_csi.h > > > index e611bdd6e9b2..8e232cd91ebe 100644 > > > --- a/drivers/media/platform/sunxi/sun6i-csi/sun6i_csi.h > > > +++ b/drivers/media/platform/sunxi/sun6i-csi/sun6i_csi.h > > > @@ -21,6 +21,7 @@ > > > enum sun6i_csi_port { > > > SUN6I_CSI_PORT_PARALLEL = 0, > > > SUN6I_CSI_PORT_MIPI_CSI2 = 1, > > > + SUN6I_CSI_PORT_ISP = 2, > > > }; > > > > > > struct sun6i_csi_buffer { > > > @@ -44,6 +45,8 @@ struct sun6i_csi_device { > > > struct clk *clock_mod; > > > struct clk *clock_ram; > > > struct reset_control *reset; > > > + > > > + bool isp_available; > > > }; > > > > > > struct sun6i_csi_variant {
Hi Paul, On Thu, Sep 01, 2022 at 04:55:14PM +0200, Paul Kocialkowski wrote: > On Sat 27 Aug 22, 01:44, Laurent Pinchart wrote: > > 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. Indeed. > 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. Not the component framework please :-) But we do need something, I agree. There's a media device allocator API that was meant to care for this kind of use cases, but I'm not happy with the implementation, we need something better. We can discuss it later once you'll have a need for that. > > 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. OK, that sounds good. > > > 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@bootlin.com> > > > --- > > > .../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 {