Re: [PATCH v3 05/22] media: camss: Refactor VFE HW version support

2021-01-28 Thread Robert Foss
Hey Nicolas,

Thanks for the review!

On Thu, 28 Jan 2021 at 01:19, Nicolas Boichat  wrote:
>
> On Wed, Jan 27, 2021 at 10:56 PM Robert Foss  wrote:
> >
> > 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 
> > ---
> > [snip]
> > diff --git a/drivers/media/platform/qcom/camss/camss-vfe-4-8.c 
> > b/drivers/media/platform/qcom/camss/camss-vfe-4-8.c
> > new file mode 100644
> > index ..153e0e20664e
> > --- /dev/null
> > +++ b/drivers/media/platform/qcom/camss/camss-vfe-4-8.c
> > [snip]
> > +/*
> > + * 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)
> > +{
> > +   struct vfe_device *vfe = dev;
> > +   u32 value0, value1;
> > +   int i, j;
> > +
> > +   vfe->ops->isr_read(vfe, , );
> > +
> > +   trace_printk("VFE: status0 = 0x%08x, status1 = 0x%08x\n",
> > +value0, value1);
>
> Please do not use trace_printk in production code [1,2], it is only
> meant for debug use. Consider using trace events, or dev_dbg.

Ack, this is a copy paste error, I'll add a commit fixing all
occurrences of this in the driver

>
> [1] https://elixir.bootlin.com/linux/v5.8/source/kernel/trace/trace.c#L3158
> [2] https://elixir.bootlin.com/linux/v5.8/source/include/linux/kernel.h#L766
>
> > [snip]


Re: [PATCH v3 05/22] media: camss: Refactor VFE HW version support

2021-01-27 Thread Nicolas Boichat
On Wed, Jan 27, 2021 at 10:56 PM Robert Foss  wrote:
>
> 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 
> ---
> [snip]
> diff --git a/drivers/media/platform/qcom/camss/camss-vfe-4-8.c 
> b/drivers/media/platform/qcom/camss/camss-vfe-4-8.c
> new file mode 100644
> index ..153e0e20664e
> --- /dev/null
> +++ b/drivers/media/platform/qcom/camss/camss-vfe-4-8.c
> [snip]
> +/*
> + * 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)
> +{
> +   struct vfe_device *vfe = dev;
> +   u32 value0, value1;
> +   int i, j;
> +
> +   vfe->ops->isr_read(vfe, , );
> +
> +   trace_printk("VFE: status0 = 0x%08x, status1 = 0x%08x\n",
> +value0, value1);

Please do not use trace_printk in production code [1,2], it is only
meant for debug use. Consider using trace events, or dev_dbg.

[1] https://elixir.bootlin.com/linux/v5.8/source/kernel/trace/trace.c#L3158
[2] https://elixir.bootlin.com/linux/v5.8/source/include/linux/kernel.h#L766

> [snip]


[PATCH v3 05/22] media: camss: Refactor VFE HW version support

2021-01-27 Thread Robert Foss
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 
---

Changes since v1
 - kernel test robot: Re-add chunk missing from
   vfe_output_update_pong_addr
 - Andrey: Fix file name error
 - Andrey: Change hardware version number in comment
 - Changed copyright year to 2021 for camss-vfe-4-8.c


 drivers/media/platform/qcom/camss/Makefile|2 +
 .../media/platform/qcom/camss/camss-vfe-4-1.c |  117 +-
 .../media/platform/qcom/camss/camss-vfe-4-7.c |  238 ++--
 .../media/platform/qcom/camss/camss-vfe-4-8.c | 1166 +
 .../platform/qcom/camss/camss-vfe-gen1.c  |  763 +++
 .../platform/qcom/camss/camss-vfe-gen1.h  |  110 ++
 drivers/media/platform/qcom/camss/camss-vfe.c |  770 +--
 drivers/media/platform/qcom/camss/camss-vfe.h |  113 +-
 8 files changed, 2244 insertions(+), 1035 deletions(-)
 create mode 100644 drivers/media/platform/qcom/camss/camss-vfe-4-8.c
 create mode 100644 drivers/media/platform/qcom/camss/camss-vfe-gen1.c
 create mode 100644 drivers/media/platform/qcom/camss/camss-vfe-gen1.h

diff --git a/drivers/media/platform/qcom/camss/Makefile 
b/drivers/media/platform/qcom/camss/Makefile
index 63c1b1b2943c..940c0ae3e003 100644
--- a/drivers/media/platform/qcom/camss/Makefile
+++ b/drivers/media/platform/qcom/camss/Makefile
@@ -10,6 +10,8 @@ qcom-camss-objs += \
camss-ispif.o \
camss-vfe-4-1.o \
camss-vfe-4-7.o \
+   camss-vfe-4-8.o \
+   camss-vfe-gen1.o \
camss-vfe.o \
camss-video.o \
 
diff --git a/drivers/media/platform/qcom/camss/camss-vfe-4-1.c 
b/drivers/media/platform/qcom/camss/camss-vfe-4-1.c
index a1b56b89130d..269dc0860de9 100644
--- a/drivers/media/platform/qcom/camss/camss-vfe-4-1.c
+++ b/drivers/media/platform/qcom/camss/camss-vfe-4-1.c
@@ -12,7 +12,9 @@
 #include 
 #include 
 
+#include "camss.h"
 #include "camss-vfe.h"
+#include "camss-vfe-gen1.h"
 
 #define VFE_0_HW_VERSION   0x000
 
@@ -283,30 +285,6 @@ static void vfe_wm_frame_based(struct vfe_device *vfe, u8 
wm, u8 enable)
1 << VFE_0_BUS_IMAGE_MASTER_n_WR_CFG_FRM_BASED_SHIFT);
 }
 
-#define CALC_WORD(width, M, N) (((width) * (M) + (N) - 1) / (N))
-
-static int vfe_word_per_line(u32 format, u32 pixel_per_line)
-{
-   int val = 0;
-
-   switch (format) {
-   case V4L2_PIX_FMT_NV12:
-   case V4L2_PIX_FMT_NV21:
-   case V4L2_PIX_FMT_NV16:
-   case V4L2_PIX_FMT_NV61:
-   val = CALC_WORD(pixel_per_line, 1, 8);
-   break;
-   case V4L2_PIX_FMT_YUYV:
-   case V4L2_PIX_FMT_YVYU:
-   case V4L2_PIX_FMT_UYVY:
-   case V4L2_PIX_FMT_VYUY:
-   val = CALC_WORD(pixel_per_line, 2, 8);
-   break;
-   }
-
-   return val;
-}
-
 static void vfe_get_wm_sizes(struct v4l2_pix_format_mplane *pix, u8 plane,
 u16 *width, u16 *height, u16 *bytesperline)
 {
@@ -665,20 +643,6 @@ static void vfe_set_demux_cfg(struct vfe_device *vfe, 
struct vfe_line *line)
writel_relaxed(odd_cfg, vfe->base + VFE_0_DEMUX_ODD_CFG);
 }
 
-static inline u8 vfe_calc_interp_reso(u16 input, u16 output)
-{
-   if (input / output >= 16)
-   return 0;
-
-   if (input / output >= 8)
-   return 1;
-
-   if (input / output >= 4)
-   return 2;
-
-   return 3;
-}
-
 static void vfe_set_scale_cfg(struct vfe_device *vfe, struct vfe_line *line)
 {
u32 p = line->video_out.active_fmt.fmt.pix_mp.pixelformat;
@@ -974,46 +938,61 @@ static irqreturn_t vfe_isr(int irq, void *dev)
return IRQ_HANDLED;
 }
 
-const struct vfe_hw_ops vfe_ops_4_1 = {
-   .hw_version_read = vfe_hw_version_read,
+
+const struct vfe_hw_ops_gen1 vfe_ops_gen1_4_1 = {
+   .bus_connect_wm_to_rdi = vfe_bus_connect_wm_to_rdi,
+   .bus_disconnect_wm_from_rdi = vfe_bus_disconnect_wm_from_rdi,
+   .bus_enable_wr_if = vfe_bus_enable_wr_if,
+   .bus_reload_wm = vfe_bus_reload_wm,
+   .camif_wait_for_stop = vfe_camif_wait_for_stop,
+   .enable_irq_common = vfe_enable_irq_common,
+   .enable_irq_pix_line = vfe_enable_irq_pix_line,
+   .enable_irq_wm_line = vfe_enable_irq_wm_line,
.get_ub_size = vfe_get_ub_size,
-   .global_reset = vfe_global_reset,
-   .halt_request = vfe_halt_request,
.halt_clear = vfe_halt_clear,
+   .halt_request = vfe_halt_request,
+   .set_camif_cfg = vfe_set_camif_cfg,
+   .set_camif_cmd = vfe_set_camif_cmd,
+   .set_cgc_override = vfe_set_cgc_override,
+   .set_clamp_cfg = vfe_set_clamp_cfg,
+   .set_crop_cfg = vfe_set_crop_cfg,
+   .set_demux_cfg =