Hi Sakari & Laurent
On Tue, 26 Mar 2024 at 01:37, Laurent Pinchart
<laurent.pinchart@xxxxxxxxxxxxxxxx> wrote:
>
> Hi Sakari,
>
> On Mon, Mar 25, 2024 at 06:36:49PM +0000, Sakari Ailus wrote:
> > On Fri, Mar 01, 2024 at 11:32:24PM +0200, 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>
> >
> > Thanks for submitting this, it's the cleanest and neatest Unicom driver
>
> Unicam, or if you insist Unicorn, but not Unicom :-)
>
> > I've ever seen!
> >
> > Some mostly unimportant comments below, however the bus-type issue needs to
> > be addressed.
> >
> > > ---
> > > Changes since v5:
> > >
> > > - Move to drivers/media/platform/broadcom/
> > > - Port to the upstream V4L2 streams API
> > > - Rebase on latest metadata API proposal
> > > - Add missing error message
> > > - Drop unneeded documentation block for unicam_isr()
> > > - Drop unneeded dev_dbg() and dev_err() messages
> > > - Drop unneeded streams_mask and fmt checks
> > > - Drop unused unicam_sd_pad_is_sink()
> > > - Drop unneeded includes
> > > - Drop v4l2_ctrl_subscribe_event() call
> > > - Use pm_runtime_resume_and_get()
> > > - Indentation and line wrap fixes
> > > - Let the framework set bus_info
> > > - Use v4l2_fwnode_endpoint_parse()
> > > - Fix media device cleanup
> > > - Drop lane reordering checks
> > > - Fix subdev state locking
> > > - Drop extra debug messages
> > > - Move clock handling to runtime PM handlers
> > > - Reorder functions
> > > - Rename init functions for more clarity
> > > - Initialize runtime PM earlier
> > > - Clarify error messages
> > > - Simplify subdev init with local variable
> > > - Fix subdev cleanup
> > > - Fix typos and indentation
> > > - Don't initialize local variables needlessly
> > > - Simplify num lanes check
> > > - Fix metadata handling in subdev set_fmt
> > > - Drop manual fallback to .s_stream()
> > > - Pass v4l2_pix_format to unicam_calc_format_size_bpl()
> > > - Simplify unicam_set_default_format()
> > > - Fix default format settings
> > > - Add busy check in unicam_s_fmt_meta()
> > > - Add missing \n at end of format strings
> > > - Fix metadata handling in subdev set_fmt
> > > - Fix locking when starting streaming
> > > - Return buffers from start streaming fails
> > > - Fix format validation for metadata node
> > > - Use video_device_pipeline_{start,stop}() helpers
> > > - Simplify format enumeration
> > > - Drop unset variable
> > > - Update MAINTAINERS entry
> > > - Update to the upstream v4l2_async_nf API
> > > - Update to the latest subdev routing API
> > > - Update to the latest subdev state API
> > > - Move from subdev .init_cfg() to .init_state()
> > > - Update to the latest videobuf2 API
> > > - Fix v4l2_subdev_enable_streams() error check
> > > - Use correct pad for the connected subdev
> > > - Return buffers to vb2 when start streaming fails
> > > - Improve debugging in start streaming handler
> > > - Simplify DMA address management
> > > - Drop comment about bcm2835-camera driver
> > > - Clarify comments that explain min/max sizes
> > > - Pass v4l2_pix_format to unicam_try_fmt()
> > > - Drop unneeded local variables
> > > - Rename image-related constants and functions
> > > - Turn unicam_fmt.metadata_fmt into bool
> > > - Rename unicam_fmt to unicam_format_info
> > > - Rename unicam_format_info variables to fmtinfo
> > > - Rename unicam_node.v_fmt to fmt
> > > - Add metadata formats for RAW10, RAW12 and RAW14
> > > - Make metadata formats line-based
> > > - Validate format on metadata video device
> > > - Add Co-devlopped-by tags
> > >
> > > Changes since v3:
> > >
> > > - Add the vendor prefix for DT name
> > > - Use the reg-names in DT parsing
> > > - Remove MAINTAINERS entry
> > >
> > > Changes since v2:
> > >
> > > - Change code organization
> > > - Remove unused variables
> > > - Correct the fmt_meta functions
> > > - Rewrite the start/stop streaming
> > > - You can now start the image node alone, but not the metadata one
> > > - The buffers are allocated per-node
> > > - only the required stream is started, if the route exists and is
> > > enabled
> > > - Prefix the macros with UNICAM_ to not have too generic names
> > > - Drop colorspace support
> > >
> > > Changes since v1:
> > >
> > > - Replace the unicam_{info,debug,error} macros with dev_*()
> > > ---
> > > 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
> > >
> > > diff --git a/MAINTAINERS b/MAINTAINERS
> > > index e50a59654e6e..cc350729f467 100644
> > > --- a/MAINTAINERS
> > > +++ b/MAINTAINERS
> > > @@ -4002,6 +4002,7 @@ M: Raspberry Pi Kernel Maintenance <kernel-list@xxxxxxxxxxxxxxx>
> > > L: linux-media@xxxxxxxxxxxxxxx
> > > S: Maintained
> > > F: Documentation/devicetree/bindings/media/brcm,bcm2835-unicam.yaml
> > > +F: drivers/media/platform/bcm2835/
> > >
> > > BROADCOM BCM47XX MIPS ARCHITECTURE
> > > M: Hauke Mehrtens <hauke@xxxxxxxxxx>
> > > diff --git a/drivers/media/platform/Kconfig b/drivers/media/platform/Kconfig
> > > index 91e54215de3a..2d79bfc68c15 100644
> > > --- a/drivers/media/platform/Kconfig
> > > +++ b/drivers/media/platform/Kconfig
> > > @@ -67,6 +67,7 @@ source "drivers/media/platform/amlogic/Kconfig"
> > > source "drivers/media/platform/amphion/Kconfig"
> > > source "drivers/media/platform/aspeed/Kconfig"
> > > source "drivers/media/platform/atmel/Kconfig"
> > > +source "drivers/media/platform/broadcom/Kconfig"
> > > source "drivers/media/platform/cadence/Kconfig"
> > > source "drivers/media/platform/chips-media/Kconfig"
> > > source "drivers/media/platform/intel/Kconfig"
> > > diff --git a/drivers/media/platform/Makefile b/drivers/media/platform/Makefile
> > > index 3296ec1ebe16..da17301f7439 100644
> > > --- a/drivers/media/platform/Makefile
> > > +++ b/drivers/media/platform/Makefile
> > > @@ -10,6 +10,7 @@ obj-y += amlogic/
> > > obj-y += amphion/
> > > obj-y += aspeed/
> > > obj-y += atmel/
> > > +obj-y += broadcom/
> > > obj-y += cadence/
> > > obj-y += chips-media/
> > > obj-y += intel/
> > > diff --git a/drivers/media/platform/broadcom/Kconfig b/drivers/media/platform/broadcom/Kconfig
> > > new file mode 100644
> > > index 000000000000..cc2c9afcc948
> > > --- /dev/null
> > > +++ b/drivers/media/platform/broadcom/Kconfig
> > > @@ -0,0 +1,23 @@
> > > +# SPDX-License-Identifier: GPL-2.0
> > > +
> > > +config VIDEO_BCM2835_UNICAM
> > > + tristate "Broadcom BCM283x/BCM271x Unicam video capture driver"
> > > + depends on ARCH_BCM2835 || COMPILE_TEST
> > > + depends on PM
> > > + depends on VIDEO_DEV
> > > + select MEDIA_CONTROLLER
> > > + select V4L2_FWNODE
> > > + select VIDEO_V4L2_SUBDEV_API
> > > + select VIDEOBUF2_DMA_CONTIG
> > > + help
> > > + Say Y here to enable support for the BCM283x/BCM271x CSI-2 receiver.
> > > + This is a V4L2 driver that controls the CSI-2 receiver directly,
> > > + independently from the VC4 firmware.
> > > +
> > > + This driver is mutually exclusive with the use of bcm2835-camera. The
> > > + firmware will disable all access to the peripheral from within the
> > > + firmware if it finds a DT node using it, and bcm2835-camera will
> > > + therefore fail to probe.
> > > +
> > > + To compile this driver as a module, choose M here. The module will be
> > > + called bcm2835-unicam.
> > > diff --git a/drivers/media/platform/broadcom/Makefile b/drivers/media/platform/broadcom/Makefile
> > > new file mode 100644
> > > index 000000000000..03d2045aba2e
> > > --- /dev/null
> > > +++ b/drivers/media/platform/broadcom/Makefile
> > > @@ -0,0 +1,3 @@
> > > +# SPDX-License-Identifier: GPL-2.0
> > > +
> > > +obj-$(CONFIG_VIDEO_BCM2835_UNICAM) += bcm2835-unicam.o
> > > diff --git a/drivers/media/platform/broadcom/bcm2835-unicam-regs.h b/drivers/media/platform/broadcom/bcm2835-unicam-regs.h
> > > new file mode 100644
> > > index 000000000000..84775fd2fac5
> > > --- /dev/null
> > > +++ b/drivers/media/platform/broadcom/bcm2835-unicam-regs.h
> > > @@ -0,0 +1,255 @@
> > > +/* SPDX-License-Identifier: GPL-2.0-only */
> > > +
> > > +/*
> > > + * Copyright (C) 2017-2020 Raspberry Pi Trading.
> >
> > Anything up to 2024?
>
> Not really. The registers haven't really changed :-) I'll update the
> copyright in the .c file though.
>
> > > + * Dave Stevenson <dave.stevenson@xxxxxxxxxxxxxxx>
> > > + */
>
> [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 irqreturn_t unicam_isr(int irq, void *dev)
> > > +{
> > > + struct unicam_device *unicam = dev;
> > > + unsigned int lines_done = unicam_get_lines_done(dev);
> > > + unsigned int sequence = unicam->sequence;
> > > + unsigned int i;
> > > + u32 ista, sta;
> > > + bool fe;
> > > + u64 ts;
> > > +
> > > + sta = unicam_reg_read(unicam, UNICAM_STA);
> > > + /* Write value back to clear the interrupts */
> > > + unicam_reg_write(unicam, UNICAM_STA, sta);
> > > +
> > > + ista = unicam_reg_read(unicam, UNICAM_ISTA);
> > > + /* Write value back to clear the interrupts */
> > > + unicam_reg_write(unicam, UNICAM_ISTA, ista);
> > > +
> > > + dev_dbg(unicam->dev, "ISR: ISTA: 0x%X, STA: 0x%X, sequence %d, lines done %d\n",
> > > + ista, sta, sequence, lines_done);
> > > +
> > > + if (!(sta & (UNICAM_IS | UNICAM_PI0)))
> > > + return IRQ_HANDLED;
> > > +
> > > + /*
> > > + * Look for either the Frame End interrupt or the Packet Capture status
> > > + * to signal a frame end.
> > > + */
> > > + fe = ista & UNICAM_FEI || sta & UNICAM_PI0;
> > > +
> > > + /*
> > > + * We must run the frame end handler first. If we have a valid next_frm
> > > + * and we get a simultaneout FE + FS interrupt, running the FS handler
> > > + * first would null out the next_frm ptr and we would have lost the
> > > + * buffer forever.
> > > + */
> > > + if (fe) {
> > > + /*
> > > + * Ensure we have swapped buffers already as we can't
> > > + * stop the peripheral. If no buffer is available, use a
> > > + * dummy buffer to dump out frames until we get a new buffer
> > > + * to use.
> > > + */
> > > + for (i = 0; i < ARRAY_SIZE(unicam->node); i++) {
> > > + if (!unicam->node[i].streaming)
> > > + continue;
> > > +
> > > + /*
> > > + * If cur_frm == next_frm, it means we have not had
> > > + * a chance to swap buffers, likely due to having
> > > + * multiple interrupts occurring simultaneously (like FE
> > > + * + FS + LS). In this case, we cannot signal the buffer
> > > + * as complete, as the HW will reuse that buffer.
> > > + */
> > > + if (unicam->node[i].cur_frm &&
> > > + unicam->node[i].cur_frm != unicam->node[i].next_frm)
> > > + unicam_process_buffer_complete(&unicam->node[i],
> > > + sequence);
> > > + unicam->node[i].cur_frm = unicam->node[i].next_frm;
> > > + }
> > > + unicam->sequence++;
> >
> > Does access to this data need to be serialised somehow.
>
> Given that it's only accessed from the interrupt handler (beside
> start_streaming time, before starting the hardware), I don't think so.
>
> > > + }
> > > +
> > > + if (ista & UNICAM_FSI) {
> > > + /*
> > > + * Timestamp is to be when the first data byte was captured,
> > > + * aka frame start.
> > > + */
> > > + ts = ktime_get_ns();
> > > + for (i = 0; i < ARRAY_SIZE(unicam->node); i++) {
> > > + if (!unicam->node[i].streaming)
> > > + continue;
> > > +
> > > + if (unicam->node[i].cur_frm)
> > > + unicam->node[i].cur_frm->vb.vb2_buf.timestamp =
> > > + ts;
> > > + else
> > > + dev_dbg(unicam->v4l2_dev.dev,
> > > + "ISR: [%d] Dropping frame, buffer not available at FS\n",
> > > + i);
> > > + /*
> > > + * Set the next frame output to go to a dummy frame
> > > + * if we have not managed to obtain another frame
> > > + * from the queue.
> > > + */
> > > + unicam_schedule_dummy_buffer(&unicam->node[i]);
> > > + }
> > > +
> > > + unicam_queue_event_sof(unicam);
> > > + }
> > > +
> > > + /*
> > > + * Cannot swap buffer at frame end, there may be a race condition
> > > + * where the HW does not actually swap it if the new frame has
> > > + * already started.
> > > + */
> > > + if (ista & (UNICAM_FSI | UNICAM_LCI) && !fe) {
> > > + for (i = 0; i < ARRAY_SIZE(unicam->node); i++) {
> > > + if (!unicam->node[i].streaming)
> > > + continue;
> > > +
> > > + spin_lock(&unicam->node[i].dma_queue_lock);
> > > + if (!list_empty(&unicam->node[i].dma_queue) &&
> > > + !unicam->node[i].next_frm)
> > > + unicam_schedule_next_buffer(&unicam->node[i]);
> > > + spin_unlock(&unicam->node[i].dma_queue_lock);
> > > + }
> > > + }
> > > +
> > > + if (unicam_reg_read(unicam, UNICAM_ICTL) & UNICAM_FCM) {
> > > + /* Switch out of trigger mode if selected */
> > > + unicam_reg_write_field(unicam, UNICAM_ICTL, 1, UNICAM_TFC);
> > > + unicam_reg_write_field(unicam, UNICAM_ICTL, 0, UNICAM_FCM);
> > > + }
> > > + return IRQ_HANDLED;
> > > +}
> > > +
> > > +static void unicam_set_packing_config(struct unicam_device *unicam)
> > > +{
> > > + struct unicam_node *node = &unicam->node[UNICAM_IMAGE_NODE];
> > > + u32 pack, unpack;
> > > + u32 val;
> > > +
> > > + if (node->fmt.fmt.pix.pixelformat == node->fmtinfo->fourcc) {
> > > + unpack = UNICAM_PUM_NONE;
> > > + pack = UNICAM_PPM_NONE;
> > > + } else {
> > > + switch (node->fmtinfo->depth) {
> > > + case 8:
> > > + unpack = UNICAM_PUM_UNPACK8;
> > > + break;
> > > + case 10:
> > > + unpack = UNICAM_PUM_UNPACK10;
> > > + break;
> > > + case 12:
> > > + unpack = UNICAM_PUM_UNPACK12;
> > > + break;
> > > + case 14:
> > > + unpack = UNICAM_PUM_UNPACK14;
> > > + break;
> > > + case 16:
> > > + unpack = UNICAM_PUM_UNPACK16;
> > > + break;
> > > + default:
> > > + unpack = UNICAM_PUM_NONE;
> > > + break;
> > > + }
> > > +
> > > + /* Repacking is always to 16bpp */
> > > + pack = UNICAM_PPM_PACK16;
> >
> > Also 8-bit data?
>
> Not that I know of. The 8-bit entries in unicam_image_formats have no
> .unpacked_fourcc field, so the condition in the if above will always be
> true for those as they can only be selected by setting the pixel format
> to fmtinfo->fourcc.
Correct.
The unpacking / packing doesn't bit shift, hence 10P can be unpacked
to 10, 12P to 12, and 14P to 14, but there is no 8bit unpacked into
the LSBs of 16 bit words defined as a V4L2 format.
You can configure packing to produce 10P, 12P, etc, but without the
bitshift it has no real use.
MSBs get dropped if you try packing to a smaller bit depth than the
source, so packing to 8 bit from any other depth results in unusable
output.
If being really critical, then there is no point in having the case 8
and case 16 in this switch as they will never be selected.
Dave
> > > + }
<snip>
