Re: [PATCH 10/13] media: qcom: camss: Add support for VFE hardware version Titan 780

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi Bryan,


On 7/10/2024 7:47 PM, Bryan O'Donoghue wrote:
On 09/07/2024 17:06, Depeng Shao wrote:
Add support for VFE found on SM8550 (Titan 780). This implementation is
based on the titan 480 implementation. It supports the normal and lite
VFE.

Co-developed-by: Yongsheng Li <quic_yon@xxxxxxxxxxx>
Signed-off-by: Yongsheng Li <quic_yon@xxxxxxxxxxx>
Signed-off-by: Depeng Shao <quic_depengs@xxxxxxxxxxx>
---
  drivers/media/platform/qcom/camss/Makefile    |   1 +
  .../media/platform/qcom/camss/camss-vfe-780.c | 404 ++++++++++++++++++
  2 files changed, 405 insertions(+)
  create mode 100644 drivers/media/platform/qcom/camss/camss-vfe-780.c

diff --git a/drivers/media/platform/qcom/camss/Makefile b/drivers/media/platform/qcom/camss/Makefile
index c336e4c1a399..a83b7a8dcef7 100644
--- a/drivers/media/platform/qcom/camss/Makefile
+++ b/drivers/media/platform/qcom/camss/Makefile
@@ -17,6 +17,7 @@ qcom-camss-objs += \
          camss-vfe-4-8.o \
          camss-vfe-17x.o \
          camss-vfe-480.o \
+        camss-vfe-780.o \
          camss-vfe-gen1.o \
          camss-vfe.o \
          camss-video.o \
diff --git a/drivers/media/platform/qcom/camss/camss-vfe-780.c b/drivers/media/platform/qcom/camss/camss-vfe-780.c
new file mode 100644
index 000000000000..abef2d5b9c2e
--- /dev/null
+++ b/drivers/media/platform/qcom/camss/camss-vfe-780.c
@@ -0,0 +1,404 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * camss-vfe-780.c

+
+static u32 vfe_hw_version(struct vfe_device *vfe)
+{
+    u32 hw_version = readl_relaxed(vfe->base + VFE_HW_VERSION);
+
+    u32 gen = (hw_version >> 28) & 0xF;
+    u32 rev = (hw_version >> 16) & 0xFFF;
+    u32 step = hw_version & 0xFFFF;
+
+    dev_info(vfe->camss->dev, "VFE HW Version = %u.%u.%u\n", gen, rev, step);
+
+    return hw_version;
+}

This could be functionally decomposed into vfe_hw_version_v2() or similar and exported by camss-vfe.c

+

Yes, same with below comments, I will try to figure out which functions can be moved to common files.


+
+/*
+ * vfe_isr - VFE module interrupt handler
+ * @irq: Interrupt line
+ * @dev: VFE device
+ *
+ * Return IRQ_HANDLED on success
+ */
+static irqreturn_t vfe_isr(int irq, void *dev)
+{
+    /* Buf Done has beem moved to CSID in Titan 780.
+     * Disable VFE related IRQ.
+     * Clear the contents of this function.
+     * Return IRQ_HANDLED.
+     */
+    return IRQ_HANDLED;
+}

What's the point of this ISR at all if it never fires and just returns done ?

Since it takes no action - it can't do anything useful and therefore if it ever did fire, would fire ad infinitum.

Please drop
Sure, will drop it.


+
+static int vfe_get_output(struct vfe_line *line)
+{
+    struct vfe_device *vfe = to_vfe(line);
+    struct vfe_output *output;
+    unsigned long flags;
+
+    spin_lock_irqsave(&vfe->output_lock, flags);
+
+    output = &line->output;
+    if (output->state > VFE_OUTPUT_RESERVED) {
+        dev_err(vfe->camss->dev, "Output is running\n");
+        goto error;
+    }
+
+    output->wm_num = 1;
+
+    /* Correspondence between VFE line number and WM number.
+     * line 0 -> RDI 0, line 1 -> RDI1, line 2 -> RDI2, line 3 -> PIX/RDI3
+     * Note this 1:1 mapping will not work for PIX streams.
+     */
+    output->wm_idx[0] = line->id;
+    vfe->wm_output_map[line->id] = line->id;
+
+    output->drop_update_idx = 0;
+
+    spin_unlock_irqrestore(&vfe->output_lock, flags);
+
+    return 0;
+
+error:
+    spin_unlock_irqrestore(&vfe->output_lock, flags);
+    output->state = VFE_OUTPUT_OFF;
+
+    return -EINVAL;
+}

This is copy/paste from vfe480 and should be functionally decomposed into a common function in camss-vfe.
Sure, the flow of some functions are same with other platform, and don't read/write registers, this can be moved to a common file and reused by all platform.
I will think about this.

+
+static int vfe_enable_output(struct vfe_line *line)
+{
+    struct vfe_device *vfe = to_vfe(line);
+    struct vfe_output *output = &line->output;
+    unsigned long flags;
+    unsigned int i;
+
+    spin_lock_irqsave(&vfe->output_lock, flags);
+
+    vfe_reg_update_clear(vfe, line->id);
+
+    if (output->state > VFE_OUTPUT_RESERVED) {
+        dev_err(vfe->camss->dev, "Output is not in reserved state %d\n",
+            output->state);
+        spin_unlock_irqrestore(&vfe->output_lock, flags);
+        return -EINVAL;
+    }
+
+    WARN_ON(output->gen2.active_num);
+
+    output->state = VFE_OUTPUT_ON;
+
+    output->sequence = 0;
+
+    vfe_wm_start(vfe, output->wm_idx[0], line);
+
+    for (i = 0; i < MAX_VFE_ACT_BUF; i++) {
+        output->buf[i] = vfe_buf_get_pending(output);
+        if (!output->buf[i])
+            break;
+        output->gen2.active_num++;
+        vfe_wm_update(vfe, output->wm_idx[0], output->buf[i]->addr[0], line);
+
+        vfe_reg_update(vfe, line->id);

I see this differs from vfe480 in that vfe_reg_update(vfe, line-id); is done on each iteration of this loop whereas in 480 it is done directly after the loop, seems to me this would be a valid fix for 480 too leading to my next comment


Yes, vfe-480 also need this.

+    }
+
+    spin_unlock_irqrestore(&vfe->output_lock, flags);
+
+    return 0;
+}

This function is so similar across different SoCs with very minor differences that instead of copy/pasting and very slightly tweaking, we should be functionally decomposing and using a flag of some kind to differentaite between wait_reg_update logic in 480 and not in 780.

Again I think we should functionally decompose into camss-vfe.c and use a flag to branch the logic for the very slight logical difference between the two

vfe-480.c

         output->sequence = 0;
         output->wait_reg_update = 0;
         reinit_completion(&output->reg_update);

As a result your fix for line->id would be useful across SoCs instead of isolated to vfe 780.


Yes, some functions are same code flow, and don't read/write register, this can be moved to a common file and reused by all platform.
I will think about this.

+
+/*
+ * vfe_enable - Enable streaming on VFE line
+ * @line: VFE line
+ *
+ * Return 0 on success or a negative error code otherwise
+ */
+static int vfe_enable(struct vfe_line *line)
+{
+    struct vfe_device *vfe = to_vfe(line);
+    int ret;
+
+    mutex_lock(&vfe->stream_lock);
+
+    vfe->stream_count++;
+
+    mutex_unlock(&vfe->stream_lock);
+
+    ret = vfe_get_output(line);
+    if (ret < 0)
+        goto error_get_output;
+
+    ret = vfe_enable_output(line);
+    if (ret < 0)
+        goto error_enable_output;
+
+    vfe->was_streaming = 1;
+
+    return 0;
+
+error_enable_output:
+    vfe_put_output(line);
+
+error_get_output:
+    mutex_lock(&vfe->stream_lock);
+
+    vfe->stream_count--;
+
+    mutex_unlock(&vfe->stream_lock);
+
+    return ret;
+}

Same thesis on functional decomposition - this should be moved to camss-vfe.c and made common - its only a minor difference betwen the required logic on different SoCs so there's not a compelling reason to have largely identical functions living in difference .c files in the same driver.


Sure, I will check which functions can be moved to camss-vfe.c.

Thanks,
Depeng




[Index of Archives]     [Linux Input]     [Video for Linux]     [Gstreamer Embedded]     [Mplayer Users]     [Linux USB Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [Yosemite Backpacking]

  Powered by Linux