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
