Hi Naush,
On Wed, Mar 20, 2024 at 12:30:36PM +0000, Naushir Patuck wrote:
> On Fri, 1 Mar 2024 at 21:32, Laurent Pinchart wrote:
> >
> > From: Dave Stevenson <dave.stevenson@xxxxxxxxxxxxxxx>
> >
> > Add a driver for the Unicam camera receiver block on BCM283x processors.
> > It is represented as two video device nodes: unicam-image and
> > unicam-embedded which are connected to an internal subdev (named
> > unicam-subdev) in order to manage streams routing.
> >
> > Signed-off-by: Dave Stevenson <dave.stevenson@xxxxxxxxxxxxxxx>
> > Co-developed-by: Naushir Patuck <naush@xxxxxxxxxxxxxxx>
> > Signed-off-by: Naushir Patuck <naush@xxxxxxxxxxxxxxx>
> > Co-developed-by: Jean-Michel Hautbois <jeanmichel.hautbois@xxxxxxxxxxxxxxxx>
> > Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois@xxxxxxxxxxxxxxxx>
> > Co-developed-by: Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx>
> > Signed-off-by: Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx>
> > ---
[snip]
> > ---
> > MAINTAINERS | 1 +
> > drivers/media/platform/Kconfig | 1 +
> > drivers/media/platform/Makefile | 1 +
> > drivers/media/platform/broadcom/Kconfig | 23 +
> > drivers/media/platform/broadcom/Makefile | 3 +
> > .../platform/broadcom/bcm2835-unicam-regs.h | 255 ++
> > .../media/platform/broadcom/bcm2835-unicam.c | 2607 +++++++++++++++++
> > 7 files changed, 2891 insertions(+)
> > create mode 100644 drivers/media/platform/broadcom/Kconfig
> > create mode 100644 drivers/media/platform/broadcom/Makefile
> > create mode 100644 drivers/media/platform/broadcom/bcm2835-unicam-regs.h
> > create mode 100644 drivers/media/platform/broadcom/bcm2835-unicam.c
[snip]
> > diff --git a/drivers/media/platform/broadcom/bcm2835-unicam.c b/drivers/media/platform/broadcom/bcm2835-unicam.c
> > new file mode 100644
> > index 000000000000..716c89b8a217
> > --- /dev/null
> > +++ b/drivers/media/platform/broadcom/bcm2835-unicam.c
> > @@ -0,0 +1,2607 @@
[snip]
> > +static int unicam_start_streaming(struct vb2_queue *vq, unsigned int count)
> > +{
> > + struct unicam_node *node = vb2_get_drv_priv(vq);
> > + struct unicam_device *unicam = node->dev;
> > + struct v4l2_subdev_state *state;
> > + struct unicam_buffer *buf;
> > + unsigned long flags;
> > + int ret;
> > + u32 pad, stream;
> > + u32 remote_pad = is_image_node(node) ? UNICAM_SD_PAD_SOURCE_IMAGE
> > + : UNICAM_SD_PAD_SOURCE_METADATA;
> > +
> > + /* Look for the route for the given pad and stream. */
> > + state = v4l2_subdev_lock_and_get_active_state(&unicam->subdev.sd);
> > + ret = v4l2_subdev_routing_find_opposite_end(&state->routing,
> > + remote_pad, 0,
> > + &pad, &stream);
> > + v4l2_subdev_unlock_state(state);
> > +
> > + if (ret)
> > + goto err_return_buffers;
> > +
> > + dev_dbg(unicam->dev, "Starting stream on %s: %u/%u -> %u/%u (%s)\n",
> > + unicam->subdev.sd.name, pad, stream, remote_pad, 0,
> > + is_metadata_node(node) ? "metadata" : "image");
> > +
> > + /* The metadata node can't be started alone. */
> > + if (is_metadata_node(node)) {
> > + if (!unicam->node[UNICAM_IMAGE_NODE].streaming) {
> > + dev_err(unicam->dev,
> > + "Can't start metadata without image\n");
> > + ret = -EINVAL;
> > + goto err_return_buffers;
> > + }
>
> There's a slight change of behaviour in this function when compared to
> the downstream/BSP non-streams enabled driver.
>
> In the BSP driver, if the embedded data node has been enabled, we wait
> for both image and embedded data nodes to have start_streaming()
> called before starting the sensor (see
> https://github.com/raspberrypi/linux/blob/c04af98514c26014a4f29ec87b3ece95626059bd/drivers/media/platform/bcm2835/bcm2835-unicam.c#L2559).
> This is also the same for the Pi 5 CFE driver.
>
> With the logic in this function, we only wait for start_streaming() on
> the image node then start the sensor streaming immediately. When
> start_streaming() for the embedded data node is subsequently called,
> we end up with the first N buffers missing and/or invalid as the HW
> channel is enabled while the sensor is streaming. I noticed this when
> using libcamera where we start image then embedded node. If I flip
> things around (start embedded first then image), everything works as
> expected.
>
> Could we add back the test to ensure all nodes are streaming before
> starting the sensor?
Yes, I don't think the current implementation is good. I'm not sure why
the logic got changed, but I'll address it in the next version.
> > +
> > + spin_lock_irqsave(&node->dma_queue_lock, flags);
> > + buf = list_first_entry(&node->dma_queue,
> > + struct unicam_buffer, list);
> > + dev_dbg(unicam->dev, "buffer %p\n", buf);
> > + node->cur_frm = buf;
> > + node->next_frm = buf;
> > + list_del(&buf->list);
> > + spin_unlock_irqrestore(&node->dma_queue_lock, flags);
> > +
> > + unicam_start_metadata(unicam, buf);
> > + node->streaming = true;
> > + return 0;
> > + }
> > +
> > + ret = pm_runtime_resume_and_get(unicam->dev);
> > + if (ret < 0) {
> > + dev_err(unicam->dev, "PM runtime resume failed: %d\n", ret);
> > + goto err_return_buffers;
> > + }
> > +
> > + ret = video_device_pipeline_start(&node->video_dev, &unicam->pipe);
> > + if (ret < 0) {
> > + dev_dbg(unicam->dev, "Failed to start media pipeline: %d\n", ret);
> > + goto err_pm_put;
> > + }
> > +
> > + spin_lock_irqsave(&node->dma_queue_lock, flags);
> > + buf = list_first_entry(&node->dma_queue,
> > + struct unicam_buffer, list);
> > + dev_dbg(unicam->dev, "buffer %p\n", buf);
> > + node->cur_frm = buf;
> > + node->next_frm = buf;
> > + list_del(&buf->list);
> > + spin_unlock_irqrestore(&node->dma_queue_lock, flags);
> > +
> > + unicam_start_rx(unicam, buf);
> > +
> > + ret = v4l2_subdev_enable_streams(&unicam->subdev.sd, remote_pad, BIT(0));
> > + if (ret < 0) {
> > + dev_err(unicam->dev, "stream on failed in subdev\n");
> > + goto error_pipeline;
> > + }
> > +
> > + node->streaming = true;
> > +
> > + return 0;
> > +
> > +error_pipeline:
> > + video_device_pipeline_stop(&node->video_dev);
> > +err_pm_put:
> > + pm_runtime_put_sync(unicam->dev);
> > +err_return_buffers:
> > + unicam_return_buffers(node, VB2_BUF_STATE_QUEUED);
> > + return ret;
> > +}
[snip]
--
Regards,
Laurent Pinchart
