Re: [PATCH v10 1/2] Added Digiteq Automotive MGB4 driver
From: Martin Tůma
Date: Thu Sep 21 2023 - 16:02:44 EST
On 21. 09. 23 12:49, Hans Verkuil wrote:
On 21/09/2023 12:43, Martin Tůma wrote:
On 21. 09. 23 9:28, Hans Verkuil wrote:
On 19/09/2023 18:59, tumic@xxxxxxxxxx wrote:
From: Martin Tůma <martin.tuma@xxxxxxxxxxxxxxxxxxxxx>
Digiteq Automotive MGB4 is a modular frame grabber PCIe card for automotive
video interfaces. As for now, two modules - FPD-Link and GMSL - are
available and supported by the driver. The card has two inputs and two
outputs (FPD-Link only).
In addition to the video interfaces it also provides a trigger signal
interface and a MTD interface for FPGA firmware upload.
Signed-off-by: Martin Tůma <martin.tuma@xxxxxxxxxxxxxxxxxxxxx>
---
MAINTAINERS | 7 +
drivers/media/pci/Kconfig | 1 +
drivers/media/pci/Makefile | 1 +
drivers/media/pci/mgb4/Kconfig | 17 +
drivers/media/pci/mgb4/Makefile | 6 +
drivers/media/pci/mgb4/mgb4_cmt.c | 244 +++++++
drivers/media/pci/mgb4/mgb4_cmt.h | 17 +
drivers/media/pci/mgb4/mgb4_core.c | 686 +++++++++++++++++
drivers/media/pci/mgb4/mgb4_core.h | 74 ++
drivers/media/pci/mgb4/mgb4_dma.c | 123 ++++
drivers/media/pci/mgb4/mgb4_dma.h | 18 +
drivers/media/pci/mgb4/mgb4_i2c.c | 140 ++++
drivers/media/pci/mgb4/mgb4_i2c.h | 35 +
drivers/media/pci/mgb4/mgb4_io.h | 33 +
drivers/media/pci/mgb4/mgb4_regs.c | 30 +
drivers/media/pci/mgb4/mgb4_regs.h | 35 +
drivers/media/pci/mgb4/mgb4_sysfs.h | 18 +
drivers/media/pci/mgb4/mgb4_sysfs_in.c | 744 +++++++++++++++++++
drivers/media/pci/mgb4/mgb4_sysfs_out.c | 681 +++++++++++++++++
drivers/media/pci/mgb4/mgb4_sysfs_pci.c | 71 ++
drivers/media/pci/mgb4/mgb4_trigger.c | 208 ++++++
drivers/media/pci/mgb4/mgb4_trigger.h | 8 +
drivers/media/pci/mgb4/mgb4_vin.c | 934 ++++++++++++++++++++++++
drivers/media/pci/mgb4/mgb4_vin.h | 69 ++
drivers/media/pci/mgb4/mgb4_vout.c | 597 +++++++++++++++
drivers/media/pci/mgb4/mgb4_vout.h | 65 ++
26 files changed, 4862 insertions(+)
create mode 100644 drivers/media/pci/mgb4/Kconfig
create mode 100644 drivers/media/pci/mgb4/Makefile
create mode 100644 drivers/media/pci/mgb4/mgb4_cmt.c
create mode 100644 drivers/media/pci/mgb4/mgb4_cmt.h
create mode 100644 drivers/media/pci/mgb4/mgb4_core.c
create mode 100644 drivers/media/pci/mgb4/mgb4_core.h
create mode 100644 drivers/media/pci/mgb4/mgb4_dma.c
create mode 100644 drivers/media/pci/mgb4/mgb4_dma.h
create mode 100644 drivers/media/pci/mgb4/mgb4_i2c.c
create mode 100644 drivers/media/pci/mgb4/mgb4_i2c.h
create mode 100644 drivers/media/pci/mgb4/mgb4_io.h
create mode 100644 drivers/media/pci/mgb4/mgb4_regs.c
create mode 100644 drivers/media/pci/mgb4/mgb4_regs.h
create mode 100644 drivers/media/pci/mgb4/mgb4_sysfs.h
create mode 100644 drivers/media/pci/mgb4/mgb4_sysfs_in.c
create mode 100644 drivers/media/pci/mgb4/mgb4_sysfs_out.c
create mode 100644 drivers/media/pci/mgb4/mgb4_sysfs_pci.c
create mode 100644 drivers/media/pci/mgb4/mgb4_trigger.c
create mode 100644 drivers/media/pci/mgb4/mgb4_trigger.h
create mode 100644 drivers/media/pci/mgb4/mgb4_vin.c
create mode 100644 drivers/media/pci/mgb4/mgb4_vin.h
create mode 100644 drivers/media/pci/mgb4/mgb4_vout.c
create mode 100644 drivers/media/pci/mgb4/mgb4_vout.h
<snip>
diff --git a/drivers/media/pci/mgb4/mgb4_sysfs_in.c b/drivers/media/pci/mgb4/mgb4_sysfs_in.c
new file mode 100644
index 000000000000..61b1ee969ed0
--- /dev/null
+++ b/drivers/media/pci/mgb4/mgb4_sysfs_in.c
@@ -0,0 +1,744 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Copyright (C) 2021-2023 Digiteq Automotive
+ * author: Martin Tuma <martin.tuma@xxxxxxxxxxxxxxxxxxxxx>
+ *
+ * This module handles all the sysfs info/configuration that is related to the
+ * v4l2 input devices.
+ */
+
+#include <linux/device.h>
+#include "mgb4_core.h"
+#include "mgb4_i2c.h"
+#include "mgb4_vin.h"
+#include "mgb4_cmt.h"
+#include "mgb4_sysfs.h"
+
+/* Common for both FPDL3 and GMSL */
+
+static ssize_t input_id_show(struct device *dev,
+ struct device_attribute *attr, char *buf)
+{
+ struct video_device *vdev = to_video_device(dev);
+ struct mgb4_vin_dev *vindev = video_get_drvdata(vdev);
+
+ return sprintf(buf, "%d\n", vindev->config->id);
+}
+
+static ssize_t oldi_lane_width_show(struct device *dev,
+ struct device_attribute *attr, char *buf)
+{
+ struct video_device *vdev = to_video_device(dev);
+ struct mgb4_vin_dev *vindev = video_get_drvdata(vdev);
+ struct mgb4_dev *mgbdev = vindev->mgbdev;
+ u16 i2c_reg;
+ u8 i2c_mask, i2c_single_val, i2c_dual_val;
+ u32 config;
+ int ret;
+
+ i2c_reg = MGB4_IS_GMSL(mgbdev) ? 0x1CE : 0x49;
+ i2c_mask = MGB4_IS_GMSL(mgbdev) ? 0x0E : 0x03;
+ i2c_single_val = MGB4_IS_GMSL(mgbdev) ? 0x00 : 0x02;
+ i2c_dual_val = MGB4_IS_GMSL(mgbdev) ? 0x0E : 0x00;
+
+ mutex_lock(&mgbdev->i2c_lock);
+ ret = mgb4_i2c_read_byte(&vindev->deser, i2c_reg);
+ mutex_unlock(&mgbdev->i2c_lock);
+ if (ret < 0)
+ return -EIO;
+
+ config = mgb4_read_reg(&mgbdev->video, vindev->config->regs.config);
+
+ if (((config & (1U << 9)) && ((ret & i2c_mask) != i2c_dual_val)) ||
+ (!(config & (1U << 9)) && ((ret & i2c_mask) != i2c_single_val))) {
+ dev_err(dev, "I2C/FPGA register value mismatch\n");
+ return -EINVAL;
+ }
+
+ return sprintf(buf, "%s\n", config & (1U << 9) ? "1" : "0");
+}
+
+static ssize_t oldi_lane_width_store(struct device *dev,
+ struct device_attribute *attr,
+ const char *buf, size_t count)
+{
+ struct video_device *vdev = to_video_device(dev);
+ struct mgb4_vin_dev *vindev = video_get_drvdata(vdev);
+ struct mgb4_dev *mgbdev = vindev->mgbdev;
+ u32 fpga_data;
+ u16 i2c_reg;
+ u8 i2c_mask, i2c_data;
+ unsigned long val;
+ int ret;
+
+ ret = kstrtoul(buf, 10, &val);
+ if (ret)
+ return ret;
+
+ switch (val) {
+ case 0: /* single */
+ fpga_data = 0;
+ i2c_data = MGB4_IS_GMSL(mgbdev) ? 0x00 : 0x02;
+ break;
+ case 1: /* dual */
+ fpga_data = 1U << 9;
+ i2c_data = MGB4_IS_GMSL(mgbdev) ? 0x0E : 0x00;
+ break;
+ default:
+ return -EINVAL;
+ }
+
+ i2c_reg = MGB4_IS_GMSL(mgbdev) ? 0x1CE : 0x49;
+ i2c_mask = MGB4_IS_GMSL(mgbdev) ? 0x0E : 0x03;
Isn't this sequence needed as well?
mutex_lock(vindev->vdev.lock);
if (vb2_is_busy(vindev->vdev.queue)) {
mutex_unlock(vindev->vdev.lock);
return -EBUSY;
}
I would expect this to be present in almost all store functions.
You don't want to change a setting like this when the queue is busy.
If a store function doesn't need the lock, then perhaps add a comment
like: 'This can be changed at any time, even if vb2_is_busy() is true.'
Can you go through all the store functions and verify this?
Basically any store function that changes timings/video source/buffer size
needs this check.
Similar to VIDIOC_S_FMT and VIDIOC_S_DV_TIMINGS ioctls: you can't change
those while buffers are allocated.
Any store function, that could change the video source/size and thus would require new buffers is guarded by this check. In the admin guide documentation, all those functions have the "This parameter
can not be changed while the output v4l2 device is open." note.
Functions that change the signal "on the wire" like oldi_lane_width_store() do however not have this guard as for me it seems that those can not break anything in v4l2 - the queue remains the same.
Also when practically tested, nothing seemed to break. But I can of course add those checks to some more functions if you think they are necessary. Not having this restrictions makes it possible to
fiddle with the parameters on live streams to set the correct parameters. For example in this case (oldi_lane_width) the image switches between "two images in one" and "the correct image" if you
switch it during playback, but the queue (buffer size) is the same. The FPGA is also designed to be capable of changing those parameters "live".
Ah, OK. Then this should be documented in the source code. Basically you want
to see in the code that it is intentional that no locking takes place and that
it is safe to do so.
Most devices are not able to handle this, at minimum you will see video glitches.
Well, you get video/image glitches in this case too, but they are
expected. Fiddling with this parameters is expected to be done while
"exploring" the radio/display (getting some documentation on the device
is usually harder than to "trial and error" it - yes, it's crazy, but
that's how it works in the VW group...). After that you create the
appropriate udev rules for the signal parameters and access the device
through the standard v4l2 API.
As this "trial and error" phase is the expected use-case, the FPGA is
designed not to break the buffer queue wherever possible. But I do
understand that "normal" devices do not work that way and people looking
into the code may find that suspicious. I will add a comment like "this
change can only break the image, not the buffer queue and is expected to
be called on live streams" and submit a v11 patch.
M.
With that change I should be able to merge v11.
Regards,
Hans
+
+ mutex_lock(&mgbdev->i2c_lock);
+ ret = mgb4_i2c_mask_byte(&vindev->deser, i2c_reg, i2c_mask, i2c_data);
+ mutex_unlock(&mgbdev->i2c_lock);
+ if (ret < 0)
+ return -EIO;
+ mgb4_mask_reg(&mgbdev->video, vindev->config->regs.config, 1U << 9,
+ fpga_data);
+ if (MGB4_IS_GMSL(mgbdev)) {
+ /* reset input link */
+ mutex_lock(&mgbdev->i2c_lock);
+ ret = mgb4_i2c_mask_byte(&vindev->deser, 0x10, 1U << 5, 1U << 5);
+ mutex_unlock(&mgbdev->i2c_lock);
+ if (ret < 0)
+ return -EIO;
+ }
+
+ return count;
+}
+
+static ssize_t color_mapping_show(struct device *dev,
+ struct device_attribute *attr, char *buf)
+{
+ struct video_device *vdev = to_video_device(dev);
+ struct mgb4_vin_dev *vindev = video_get_drvdata(vdev);
+ u32 config = mgb4_read_reg(&vindev->mgbdev->video,
+ vindev->config->regs.config);
+
+ return sprintf(buf, "%s\n", config & (1U << 8) ? "0" : "1");
+}
+
+static ssize_t color_mapping_store(struct device *dev,
+ struct device_attribute *attr,
+ const char *buf, size_t count)
+{
+ struct video_device *vdev = to_video_device(dev);
+ struct mgb4_vin_dev *vindev = video_get_drvdata(vdev);
+ u32 fpga_data;
+ unsigned long val;
+ int ret;
+
+ ret = kstrtoul(buf, 10, &val);
+ if (ret)
+ return ret;
+
+ switch (val) {
+ case 0: /* OLDI/JEIDA */
+ fpga_data = (1U << 8);
+ break;
+ case 1: /* SPWG/VESA */
+ fpga_data = 0;
+ break;
+ default:
+ return -EINVAL;
+ }
+
+ mgb4_mask_reg(&vindev->mgbdev->video, vindev->config->regs.config,
+ 1U << 8, fpga_data);
This is likely a store function that can be called at any time as this
doesn't interrupt video streaming or changes buffer sizes.
Yes, this parameter definitely does not break anything else than the color scheme of the image and is without doubt safe to do on running streams. But the others should be as well.
+
+ return count;
+}
+
Regards,
Hans