Re: [PATCH v8 06/22] media: camss: Refactor VFE HW version support

Hey Hans,

Thanks for having a look!

On Tue, 16 Mar 2021 at 10:41, Hans Verkuil [off-list ref] wrote:

On 15/03/2021 16:59, Robert Foss wrote:

quoted

In order to support Qualcomm ISP hardware architectures that diverge
from older architectures, the VFE subdevice driver needs to be refactored
to better abstract the different ISP architectures.

Gen1 represents the CAMSS ISP architecture. The ISP architecture developed
after CAMSS, Titan, will be referred to as Gen2.

Signed-off-by: Robert Foss <redacted>
Reviewed-by: Andrey Konovalov <redacted>
---

<snip>

quoted

diff --git a/drivers/media/platform/qcom/camss/camss-vfe.h b/drivers/media/platform/qcom/camss/camss-vfe.h
index 5bce6736e4bb..ceff4985b1cc 100644
--- a/drivers/media/platform/qcom/camss/camss-vfe.h
+++ b/drivers/media/platform/qcom/camss/camss-vfe.h

@@ -17,15 +17,26 @@
 #include <media/v4l2-subdev.h>

 #include "camss-video.h"
+#include "camss-vfe-gen1.h"

 #define MSM_VFE_PAD_SINK 0
 #define MSM_VFE_PAD_SRC 1
 #define MSM_VFE_PADS_NUM 2

-#define MSM_VFE_LINE_NUM 4
 #define MSM_VFE_IMAGE_MASTERS_NUM 7
 #define MSM_VFE_COMPOSITE_IRQ_NUM 4

+/* VFE halt timeout */
+#define VFE_HALT_TIMEOUT_MS 100
+/* Frame drop value. VAL + UPDATES - 1 should not exceed 31 */
+#define VFE_FRAME_DROP_VAL 30
+
+#define vfe_line_array(ptr_line)     \
+     ((const struct vfe_line (*)[]) &(ptr_line[-(ptr_line->id)]))

I get a checkpatch warning on this:

CHECK: Unnecessary parentheses around ptr_line[-(ptr_line->id)]
#3612: FILE: drivers/media/platform/qcom/camss/camss-vfe.h:35:
+       ((const struct vfe_line (*)[]) &(ptr_line[-(ptr_line->id)]))

This should be:

        ((const struct vfe_line (*)[]) &(ptr_line)[-(ptr_line)->id])

The checkpatch message is a bit odd, the real problem here is the missing
parenthesis around ptr_line: this would cause problems if ptr_line is an
expression like 'ptr + 5', which would lead to a bad expansion.

Ack

Regards,

        Hans

quoted

+
+#define to_vfe(ptr_line)     \
+     container_of(vfe_line_array(ptr_line), struct vfe_device, line)
+
 enum vfe_output_state {
      VFE_OUTPUT_OFF,
      VFE_OUTPUT_RESERVED,

@@ -40,23 +51,30 @@ enum vfe_line_id {
      VFE_LINE_RDI0 = 0,
      VFE_LINE_RDI1 = 1,
      VFE_LINE_RDI2 = 2,
-     VFE_LINE_PIX = 3
+     VFE_LINE_PIX = 3,
+     VFE_LINE_NUM_GEN1 = 4,
+     VFE_LINE_NUM_MAX = 4
 };

 struct vfe_output {
      u8 wm_num;
      u8 wm_idx[3];

-     int active_buf;
      struct camss_buffer *buf[2];
      struct camss_buffer *last_buffer;
      struct list_head pending_bufs;

      unsigned int drop_update_idx;

+     union {
+             struct {
+                     int active_buf;
+                     int wait_sof;
+             } gen1;
+     };
      enum vfe_output_state state;
      unsigned int sequence;
-     int wait_sof;
+
      int wait_reg_update;
      struct completion sof;
      struct completion reg_update;

@@ -78,59 +96,19 @@ struct vfe_line {
 struct vfe_device;

 struct vfe_hw_ops {
-     void (*hw_version_read)(struct vfe_device *vfe, struct device *dev);
-     u16 (*get_ub_size)(u8 vfe_id);
+     void (*enable_irq_common)(struct vfe_device *vfe);
      void (*global_reset)(struct vfe_device *vfe);
-     void (*halt_request)(struct vfe_device *vfe);
-     void (*halt_clear)(struct vfe_device *vfe);
-     void (*wm_enable)(struct vfe_device *vfe, u8 wm, u8 enable);
-     void (*wm_frame_based)(struct vfe_device *vfe, u8 wm, u8 enable);
-     void (*wm_line_based)(struct vfe_device *vfe, u32 wm,
-                           struct v4l2_pix_format_mplane *pix,
-                           u8 plane, u32 enable);
-     void (*wm_set_framedrop_period)(struct vfe_device *vfe, u8 wm, u8 per);
-     void (*wm_set_framedrop_pattern)(struct vfe_device *vfe, u8 wm,
-                                      u32 pattern);
-     void (*wm_set_ub_cfg)(struct vfe_device *vfe, u8 wm, u16 offset,
-                           u16 depth);
-     void (*bus_reload_wm)(struct vfe_device *vfe, u8 wm);
-     void (*wm_set_ping_addr)(struct vfe_device *vfe, u8 wm, u32 addr);
-     void (*wm_set_pong_addr)(struct vfe_device *vfe, u8 wm, u32 addr);
-     int (*wm_get_ping_pong_status)(struct vfe_device *vfe, u8 wm);
-     void (*bus_enable_wr_if)(struct vfe_device *vfe, u8 enable);
-     void (*bus_connect_wm_to_rdi)(struct vfe_device *vfe, u8 wm,
-                                   enum vfe_line_id id);
-     void (*wm_set_subsample)(struct vfe_device *vfe, u8 wm);
-     void (*bus_disconnect_wm_from_rdi)(struct vfe_device *vfe, u8 wm,
-                                        enum vfe_line_id id);
-     void (*set_xbar_cfg)(struct vfe_device *vfe, struct vfe_output *output,
-                          u8 enable);
-     void (*set_rdi_cid)(struct vfe_device *vfe, enum vfe_line_id id,
-                         u8 cid);
-     void (*set_realign_cfg)(struct vfe_device *vfe, struct vfe_line *line,
-                             u8 enable);
+     void (*hw_version_read)(struct vfe_device *vfe, struct device *dev);
+     irqreturn_t (*isr)(int irq, void *dev);
+     void (*isr_read)(struct vfe_device *vfe, u32 *value0, u32 *value1);
      void (*reg_update)(struct vfe_device *vfe, enum vfe_line_id line_id);
      void (*reg_update_clear)(struct vfe_device *vfe,
                               enum vfe_line_id line_id);
-     void (*enable_irq_wm_line)(struct vfe_device *vfe, u8 wm,
-                                enum vfe_line_id line_id, u8 enable);
-     void (*enable_irq_pix_line)(struct vfe_device *vfe, u8 comp,
-                                 enum vfe_line_id line_id, u8 enable);
-     void (*enable_irq_common)(struct vfe_device *vfe);
-     void (*set_demux_cfg)(struct vfe_device *vfe, struct vfe_line *line);
-     void (*set_scale_cfg)(struct vfe_device *vfe, struct vfe_line *line);
-     void (*set_crop_cfg)(struct vfe_device *vfe, struct vfe_line *line);
-     void (*set_clamp_cfg)(struct vfe_device *vfe);
-     void (*set_qos)(struct vfe_device *vfe);
-     void (*set_ds)(struct vfe_device *vfe);
-     void (*set_cgc_override)(struct vfe_device *vfe, u8 wm, u8 enable);
-     void (*set_camif_cfg)(struct vfe_device *vfe, struct vfe_line *line);
-     void (*set_camif_cmd)(struct vfe_device *vfe, u8 enable);
-     void (*set_module_cfg)(struct vfe_device *vfe, u8 enable);
-     int (*camif_wait_for_stop)(struct vfe_device *vfe, struct device *dev);
-     void (*isr_read)(struct vfe_device *vfe, u32 *value0, u32 *value1);
+     void (*subdev_init)(struct device *dev, struct vfe_device *vfe);
+     int (*vfe_disable)(struct vfe_line *line);
+     int (*vfe_enable)(struct vfe_line *line);
+     int (*vfe_halt)(struct vfe_device *vfe);
      void (*violation_read)(struct vfe_device *vfe);
-     irqreturn_t (*isr)(int irq, void *dev);
 };

 struct vfe_isr_ops {

@@ -158,11 +136,14 @@ struct vfe_device {
      int stream_count;
      spinlock_t output_lock;
      enum vfe_line_id wm_output_map[MSM_VFE_IMAGE_MASTERS_NUM];
-     struct vfe_line line[MSM_VFE_LINE_NUM];
+     struct vfe_line line[VFE_LINE_NUM_MAX];
+     u8 line_num;
      u32 reg_update;
      u8 was_streaming;
      const struct vfe_hw_ops *ops;
+     const struct vfe_hw_ops_gen1 *ops_gen1;
      struct vfe_isr_ops isr_ops;
+     struct camss_video_ops video_ops;
 };

 struct resources;

@@ -178,6 +159,37 @@ void msm_vfe_unregister_entities(struct vfe_device *vfe);
 void msm_vfe_get_vfe_id(struct media_entity *entity, u8 *id);
 void msm_vfe_get_vfe_line_id(struct media_entity *entity, enum vfe_line_id *id);

+/*
+ * vfe_buf_add_pending - Add output buffer to list of pending
+ * @output: VFE output
+ * @buffer: Video buffer
+ */
+void vfe_buf_add_pending(struct vfe_output *output, struct camss_buffer *buffer);
+
+struct camss_buffer *vfe_buf_get_pending(struct vfe_output *output);
+
+/*
+ * vfe_disable - Disable streaming on VFE line
+ * @line: VFE line
+ *
+ * Return 0 on success or a negative error code otherwise
+ */
+int vfe_disable(struct vfe_line *line);
+
+int vfe_flush_buffers(struct camss_video *vid, enum vb2_buffer_state state);
+
+/*
+ * vfe_isr_comp_done - Process composite image done interrupt
+ * @vfe: VFE Device
+ * @comp: Composite image id
+ */
+void vfe_isr_comp_done(struct vfe_device *vfe, u8 comp);
+
+void vfe_isr_reset_ack(struct vfe_device *vfe);
+int vfe_put_output(struct vfe_line *line);
+int vfe_release_wm(struct vfe_device *vfe, u8 wm);
+int vfe_reserve_wm(struct vfe_device *vfe, enum vfe_line_id line_id);
+
 extern const struct vfe_hw_ops vfe_ops_4_1;
 extern const struct vfe_hw_ops vfe_ops_4_7;
 extern const struct vfe_hw_ops vfe_ops_4_8;

diff --git a/drivers/media/platform/qcom/camss/camss.c b/drivers/media/platform/qcom/camss/camss.c
index 0c679c97da6d..49e25c837bdc 100644
--- a/drivers/media/platform/qcom/camss/camss.c
+++ b/drivers/media/platform/qcom/camss/camss.c

@@ -881,7 +881,7 @@ static int camss_register_entities(struct camss *camss)

              for (i = 0; i < camss->ispif->line_num; i++)
                      for (k = 0; k < camss->vfe_num; k++)
-                             for (j = 0; j < ARRAY_SIZE(camss->vfe[k].line); j++) {
+                             for (j = 0; j < camss->vfe[k].line_num; j++) {
                                      struct v4l2_subdev *ispif = &camss->ispif->line[i].subdev;
                                      struct v4l2_subdev *vfe = &camss->vfe[k].line[j].subdev;

@@ -902,7 +902,7 @@ static int camss_register_entities(struct camss *camss)
      } else {
              for (i = 0; i < camss->csid_num; i++)
                      for (k = 0; k < camss->vfe_num; k++)
-                             for (j = 0; j < ARRAY_SIZE(camss->vfe[k].line); j++) {
+                             for (j = 0; j < camss->vfe[k].line_num; j++) {
                                      struct v4l2_subdev *csid = &camss->csid[i].subdev;
                                      struct v4l2_subdev *vfe = &camss->vfe[k].line[j].subdev;