Re: [PATCH v10 1/2] Added Digiteq Automotive MGB4 driver
From: Martin Tůma
Date: Thu Sep 21 2023 - 16:48:30 EST
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".
+
+ 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