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