[PATCH] omap3isp: preview: Add support for non-GRBG Bayer patterns

[PATCH] omap3isp: preview: Add support for non-GRBG Bayer patterns

[Laurent Pinchart]

Rearrange the CFA interpolation coefficients table based on the Bayer
pattern. Modifying the table during streaming isn't supported anymore,
but didn't make sense in the first place anyway.

Support for non-Bayer CFA patterns is dropped as they were not correctly
supported, and have never been tested.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 drivers/media/video/omap3isp/isppreview.c |  118 ++++++++++++++++------------
 1 files changed, 67 insertions(+), 51 deletions(-)

Jean-Philippe,

Could you please test this patch on your hardware ?

diff --git a/drivers/media/video/omap3isp/isppreview.c b/drivers/media/video/omap3isp/isppreview.c
index 8a4935e..bfa3107 100644
--- a/drivers/media/video/omap3isp/isppreview.c
+++ b/drivers/media/video/omap3isp/isppreview.c
@@ -309,36 +309,6 @@ preview_config_dcor(struct isp_prev_device *prev, const void *prev_dcor)
 }
 
 /*
- * preview_config_cfa - Configures the CFA Interpolation parameters.
- * @prev_cfa: Structure containing the CFA interpolation table, CFA format
- *            in the image, vertical and horizontal gradient threshold.
- */
-static void
-preview_config_cfa(struct isp_prev_device *prev, const void *prev_cfa)
-{
-	struct isp_device *isp = to_isp_device(prev);
-	const struct omap3isp_prev_cfa *cfa = prev_cfa;
-	unsigned int i;
-
-	isp_reg_clr_set(isp, OMAP3_ISP_IOMEM_PREV, ISPPRV_PCR,
-			ISPPRV_PCR_CFAFMT_MASK,
-			cfa->format << ISPPRV_PCR_CFAFMT_SHIFT);
-
-	isp_reg_writel(isp,
-		(cfa->gradthrs_vert << ISPPRV_CFA_GRADTH_VER_SHIFT) |
-		(cfa->gradthrs_horz << ISPPRV_CFA_GRADTH_HOR_SHIFT),
-		OMAP3_ISP_IOMEM_PREV, ISPPRV_CFA);
-
-	isp_reg_writel(isp, ISPPRV_CFA_TABLE_ADDR,
-		       OMAP3_ISP_IOMEM_PREV, ISPPRV_SET_TBL_ADDR);
-
-	for (i = 0; i < OMAP3ISP_PREV_CFA_TBL_SIZE; i++) {
-		isp_reg_writel(isp, cfa->table[i],
-			       OMAP3_ISP_IOMEM_PREV, ISPPRV_SET_TBL_DATA);
-	}
-}
-
-/*
  * preview_config_gammacorrn - Configures the Gamma Correction table values
  * @gtable: Structure containing the table for red, blue, green gamma table.
  */
@@ -813,7 +783,7 @@ static const struct preview_update update_attrs[] = {
 		FIELD_SIZEOF(struct prev_params, hmed),
 		offsetof(struct omap3isp_prev_update_config, hmed),
 	}, /* OMAP3ISP_PREV_CFA */ {
-		preview_config_cfa,
+		NULL,
 		NULL,
 		offsetof(struct prev_params, cfa),
 		FIELD_SIZEOF(struct prev_params, cfa),
@@ -1043,42 +1013,88 @@ preview_config_ycpos(struct isp_prev_device *prev,
 static void preview_config_averager(struct isp_prev_device *prev, u8 average)
 {
 	struct isp_device *isp = to_isp_device(prev);
-	struct prev_params *params;
-	int reg = 0;
 
-	params = (prev->params.active & OMAP3ISP_PREV_CFA)
-	       ? &prev->params.params[0] : &prev->params.params[1];
-
-	if (params->cfa.format == OMAP3ISP_CFAFMT_BAYER)
-		reg = ISPPRV_AVE_EVENDIST_2 << ISPPRV_AVE_EVENDIST_SHIFT |
-		      ISPPRV_AVE_ODDDIST_2 << ISPPRV_AVE_ODDDIST_SHIFT |
-		      average;
-	else if (params->cfa.format == OMAP3ISP_CFAFMT_RGBFOVEON)
-		reg = ISPPRV_AVE_EVENDIST_3 << ISPPRV_AVE_EVENDIST_SHIFT |
-		      ISPPRV_AVE_ODDDIST_3 << ISPPRV_AVE_ODDDIST_SHIFT |
-		      average;
-	isp_reg_writel(isp, reg, OMAP3_ISP_IOMEM_PREV, ISPPRV_AVE);
+	isp_reg_writel(isp, ISPPRV_AVE_EVENDIST_2 << ISPPRV_AVE_EVENDIST_SHIFT |
+		       ISPPRV_AVE_ODDDIST_2 << ISPPRV_AVE_ODDDIST_SHIFT |
+		       average, OMAP3_ISP_IOMEM_PREV, ISPPRV_AVE);
 }
 
+
+#define OMAP3ISP_PREV_CFA_BLK_SIZE	(OMAP3ISP_PREV_CFA_TBL_SIZE / 4)
+
 /*
  * preview_config_input_format - Configure the input format
  * @prev: The preview engine
  * @format: Format on the preview engine sink pad
  *
- * Enable CFA interpolation for Bayer formats and disable it for greyscale
- * formats.
+ * Enable and configure CFA interpolation for Bayer formats and disable it for
+ * greyscale formats.
+ *
+ * The CFA table is organised in four blocks, one per Bayer component. The
+ * hardware expects blocks to follow the Bayer order of the input data, while
+ * the driver stores the table in GRBG order in memory. The blocks need to be
+ * reordered to support non-GRBG Bayer patterns.
  */
 static void preview_config_input_format(struct isp_prev_device *prev,
 					const struct v4l2_mbus_framefmt *format)
 {
+	static const unsigned int cfa_coef_order[4][4] = {
+		{ 0, 1, 2, 3 }, /* GRBG */
+		{ 1, 0, 3, 2 }, /* RGGB */
+		{ 2, 3, 0, 1 }, /* BGGR */
+		{ 3, 2, 1, 0 }, /* GBRG */
+	};
 	struct isp_device *isp = to_isp_device(prev);
+	struct prev_params *params;
+	const unsigned int *order;
+	unsigned int i;
+	unsigned int j;
 
-	if (format->code != V4L2_MBUS_FMT_Y10_1X10)
-		isp_reg_set(isp, OMAP3_ISP_IOMEM_PREV, ISPPRV_PCR,
-			    ISPPRV_PCR_CFAEN);
-	else
+	if (format->code == V4L2_MBUS_FMT_Y10_1X10) {
 		isp_reg_clr(isp, OMAP3_ISP_IOMEM_PREV, ISPPRV_PCR,
 			    ISPPRV_PCR_CFAEN);
+		return;
+	}
+
+	params = (prev->params.active & OMAP3ISP_PREV_CFA)
+	       ? &prev->params.params[0] : &prev->params.params[1];
+
+	isp_reg_set(isp, OMAP3_ISP_IOMEM_PREV, ISPPRV_PCR, ISPPRV_PCR_CFAEN);
+	isp_reg_clr_set(isp, OMAP3_ISP_IOMEM_PREV, ISPPRV_PCR,
+			ISPPRV_PCR_CFAFMT_MASK, ISPPRV_PCR_CFAFMT_BAYER);
+
+	isp_reg_writel(isp,
+		(params->cfa.gradthrs_vert << ISPPRV_CFA_GRADTH_VER_SHIFT) |
+		(params->cfa.gradthrs_horz << ISPPRV_CFA_GRADTH_HOR_SHIFT),
+		OMAP3_ISP_IOMEM_PREV, ISPPRV_CFA);
+
+	switch (prev->formats[PREV_PAD_SINK].code) {
+	case V4L2_MBUS_FMT_SGRBG10_1X10:
+	default:
+		order = cfa_coef_order[0];
+		break;
+	case V4L2_MBUS_FMT_SRGGB10_1X10:
+		order = cfa_coef_order[1];
+		break;
+	case V4L2_MBUS_FMT_SBGGR10_1X10:
+		order = cfa_coef_order[2];
+		break;
+	case V4L2_MBUS_FMT_SGBRG10_1X10:
+		order = cfa_coef_order[3];
+		break;
+	}
+
+	isp_reg_writel(isp, ISPPRV_CFA_TABLE_ADDR,
+		       OMAP3_ISP_IOMEM_PREV, ISPPRV_SET_TBL_ADDR);
+
+	for (i = 0; i < 4; ++i) {
+		__u32 *block = params->cfa.table
+			     + order[i] * OMAP3ISP_PREV_CFA_BLK_SIZE;
+
+		for (j = 0; j < OMAP3ISP_PREV_CFA_BLK_SIZE; ++j)
+			isp_reg_writel(isp, block[j], OMAP3_ISP_IOMEM_PREV,
+				       ISPPRV_SET_TBL_DATA);
+	}
 }
 
 /*
-- 
Regards,

Laurent Pinchart

Re: [PATCH] omap3isp: preview: Add support for non-GRBG Bayer patterns

[jean-philippe francois]

2012/6/18 Laurent Pinchart <laurent.pinchart@ideasonboard.com>:
> Rearrange the CFA interpolation coefficients table based on the Bayer
> pattern. Modifying the table during streaming isn't supported anymore,
> but didn't make sense in the first place anyway.
>
> Support for non-Bayer CFA patterns is dropped as they were not correctly
> supported, and have never been tested.
>
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
>  drivers/media/video/omap3isp/isppreview.c |  118 ++++++++++++++++------------
>  1 files changed, 67 insertions(+), 51 deletions(-)
>
> Jean-Philippe,
>
> Could you please test this patch on your hardware ?
>

Hi,

I have applied it on top of your omap3isp-next branch, but my board is
oopsing right after the boot.  I will try to get rid of this oops,
but if you eventually now another tree that includes the changes necessary
for this patch to apply, it could perhaps save me some time.

Here is a oops, in case somebody can point me to a patch, but the oops
is not very specific :

<6>Total of 96 interrupts on 1 active controller
<4>omap_hwmod: arm_fck: missing clockdomain for arm_fck.
<4>omap_hwmod: gpt1_fck: missing clockdomain for gpt1_fck.
<6>OMAP clockevent source: GPTIMER1 at 32768 Hz
<6>sched_clock: 32 bits at 32kHz, resolution 30517ns, wraps every 131071999ms
<1>Unable to handle kernel NULL pointer dereference at virtual address 00000000
<1>pgd = c0004000
<1>[00000000] *pgd=00000000
<0>Internal error: Oops: 80000005 [#1] PREEMPT ARM
<d>Modules linked in:
CPU: 0    Not tainted  (3.4.0-rc3 #2)
PC is at 0x0
LR is at __irq_svc+0x40/0x70
pc : [<00000000>]    lr : [<c000e280>]    psr: 000001d3
sp : c0461f88  ip : 0000000f  fp : 00000000
r10: 00000000  r9 : 413fc082  r8 : 80004059
r7 : c0461fbc  r6 : ffffffff  r5 : 00000153  r4 : c04367e0
r3 : c0010108  r2 : c0461fd0  r1 : c046b00c  r0 : c0461f88
Flags: nzcv  IRQs off  FIQs off  Mode SVC_32  ISA ARM  Segment kernel
Control: 10c5387d  Table: 80004019  DAC: 00000015
<0>Process swapper (pid: 0, stack limit = 0xc04602e8)
<0>Stack: (0xc0461f88 to 0xc0462000)
<0>1f80:                   00007735 000001d3 01ffffff c0468118 00000000 c046808c
<0>1fa0: c0456ec0 c046b004 80004059 413fc082 00000000 00000000 0000000f c0461fd0
<0>1fc0: c0010108 c04367e0 00000153 ffffffff 00000000 00000000 c04364fc 00000000
<0>1fe0: 00000000 c0456ec4 00000000 10c53c7d c046808c 8000803c 00000000 00000000
[<c000e280>] (__irq_svc+0x40/0x70) from [<c04367e0>] (start_kernel+0x138/0x254)
[<c04367e0>] (start_kernel+0x138/0x254) from [<8000803c>] (0x8000803c)
<0>Code: bad PC value



> diff --git a/drivers/media/video/omap3isp/isppreview.c b/drivers/media/video/omap3isp/isppreview.c
> index 8a4935e..bfa3107 100644
> --- a/drivers/media/video/omap3isp/isppreview.c
> +++ b/drivers/media/video/omap3isp/isppreview.c
> @@ -309,36 +309,6 @@ preview_config_dcor(struct isp_prev_device *prev, const void *prev_dcor)
>  }
>
>  /*
> - * preview_config_cfa - Configures the CFA Interpolation parameters.
> - * @prev_cfa: Structure containing the CFA interpolation table, CFA format
> - *            in the image, vertical and horizontal gradient threshold.
> - */
> -static void
> -preview_config_cfa(struct isp_prev_device *prev, const void *prev_cfa)
> -{
> -       struct isp_device *isp = to_isp_device(prev);
> -       const struct omap3isp_prev_cfa *cfa = prev_cfa;
> -       unsigned int i;
> -
> -       isp_reg_clr_set(isp, OMAP3_ISP_IOMEM_PREV, ISPPRV_PCR,
> -                       ISPPRV_PCR_CFAFMT_MASK,
> -                       cfa->format << ISPPRV_PCR_CFAFMT_SHIFT);
> -
> -       isp_reg_writel(isp,
> -               (cfa->gradthrs_vert << ISPPRV_CFA_GRADTH_VER_SHIFT) |
> -               (cfa->gradthrs_horz << ISPPRV_CFA_GRADTH_HOR_SHIFT),
> -               OMAP3_ISP_IOMEM_PREV, ISPPRV_CFA);
> -
> -       isp_reg_writel(isp, ISPPRV_CFA_TABLE_ADDR,
> -                      OMAP3_ISP_IOMEM_PREV, ISPPRV_SET_TBL_ADDR);
> -
> -       for (i = 0; i < OMAP3ISP_PREV_CFA_TBL_SIZE; i++) {
> -               isp_reg_writel(isp, cfa->table[i],
> -                              OMAP3_ISP_IOMEM_PREV, ISPPRV_SET_TBL_DATA);
> -       }
> -}
> -
> -/*
>  * preview_config_gammacorrn - Configures the Gamma Correction table values
>  * @gtable: Structure containing the table for red, blue, green gamma table.
>  */
> @@ -813,7 +783,7 @@ static const struct preview_update update_attrs[] = {
>                FIELD_SIZEOF(struct prev_params, hmed),
>                offsetof(struct omap3isp_prev_update_config, hmed),
>        }, /* OMAP3ISP_PREV_CFA */ {
> -               preview_config_cfa,
> +               NULL,
>                NULL,
>                offsetof(struct prev_params, cfa),
>                FIELD_SIZEOF(struct prev_params, cfa),
> @@ -1043,42 +1013,88 @@ preview_config_ycpos(struct isp_prev_device *prev,
>  static void preview_config_averager(struct isp_prev_device *prev, u8 average)
>  {
>        struct isp_device *isp = to_isp_device(prev);
> -       struct prev_params *params;
> -       int reg = 0;
>
> -       params = (prev->params.active & OMAP3ISP_PREV_CFA)
> -              ? &prev->params.params[0] : &prev->params.params[1];
> -
> -       if (params->cfa.format == OMAP3ISP_CFAFMT_BAYER)
> -               reg = ISPPRV_AVE_EVENDIST_2 << ISPPRV_AVE_EVENDIST_SHIFT |
> -                     ISPPRV_AVE_ODDDIST_2 << ISPPRV_AVE_ODDDIST_SHIFT |
> -                     average;
> -       else if (params->cfa.format == OMAP3ISP_CFAFMT_RGBFOVEON)
> -               reg = ISPPRV_AVE_EVENDIST_3 << ISPPRV_AVE_EVENDIST_SHIFT |
> -                     ISPPRV_AVE_ODDDIST_3 << ISPPRV_AVE_ODDDIST_SHIFT |
> -                     average;
> -       isp_reg_writel(isp, reg, OMAP3_ISP_IOMEM_PREV, ISPPRV_AVE);
> +       isp_reg_writel(isp, ISPPRV_AVE_EVENDIST_2 << ISPPRV_AVE_EVENDIST_SHIFT |
> +                      ISPPRV_AVE_ODDDIST_2 << ISPPRV_AVE_ODDDIST_SHIFT |
> +                      average, OMAP3_ISP_IOMEM_PREV, ISPPRV_AVE);
>  }
>
> +
> +#define OMAP3ISP_PREV_CFA_BLK_SIZE     (OMAP3ISP_PREV_CFA_TBL_SIZE / 4)
> +
>  /*
>  * preview_config_input_format - Configure the input format
>  * @prev: The preview engine
>  * @format: Format on the preview engine sink pad
>  *
> - * Enable CFA interpolation for Bayer formats and disable it for greyscale
> - * formats.
> + * Enable and configure CFA interpolation for Bayer formats and disable it for
> + * greyscale formats.
> + *
> + * The CFA table is organised in four blocks, one per Bayer component. The
> + * hardware expects blocks to follow the Bayer order of the input data, while
> + * the driver stores the table in GRBG order in memory. The blocks need to be
> + * reordered to support non-GRBG Bayer patterns.
>  */
>  static void preview_config_input_format(struct isp_prev_device *prev,
>                                        const struct v4l2_mbus_framefmt *format)
>  {
> +       static const unsigned int cfa_coef_order[4][4] = {
> +               { 0, 1, 2, 3 }, /* GRBG */
> +               { 1, 0, 3, 2 }, /* RGGB */
> +               { 2, 3, 0, 1 }, /* BGGR */
> +               { 3, 2, 1, 0 }, /* GBRG */
> +       };
>        struct isp_device *isp = to_isp_device(prev);
> +       struct prev_params *params;
> +       const unsigned int *order;
> +       unsigned int i;
> +       unsigned int j;
>
> -       if (format->code != V4L2_MBUS_FMT_Y10_1X10)
> -               isp_reg_set(isp, OMAP3_ISP_IOMEM_PREV, ISPPRV_PCR,
> -                           ISPPRV_PCR_CFAEN);
> -       else
> +       if (format->code == V4L2_MBUS_FMT_Y10_1X10) {
>                isp_reg_clr(isp, OMAP3_ISP_IOMEM_PREV, ISPPRV_PCR,
>                            ISPPRV_PCR_CFAEN);
> +               return;
> +       }
> +
> +       params = (prev->params.active & OMAP3ISP_PREV_CFA)
> +              ? &prev->params.params[0] : &prev->params.params[1];
> +
> +       isp_reg_set(isp, OMAP3_ISP_IOMEM_PREV, ISPPRV_PCR, ISPPRV_PCR_CFAEN);
> +       isp_reg_clr_set(isp, OMAP3_ISP_IOMEM_PREV, ISPPRV_PCR,
> +                       ISPPRV_PCR_CFAFMT_MASK, ISPPRV_PCR_CFAFMT_BAYER);
> +
> +       isp_reg_writel(isp,
> +               (params->cfa.gradthrs_vert << ISPPRV_CFA_GRADTH_VER_SHIFT) |
> +               (params->cfa.gradthrs_horz << ISPPRV_CFA_GRADTH_HOR_SHIFT),
> +               OMAP3_ISP_IOMEM_PREV, ISPPRV_CFA);
> +
> +       switch (prev->formats[PREV_PAD_SINK].code) {
> +       case V4L2_MBUS_FMT_SGRBG10_1X10:
> +       default:
> +               order = cfa_coef_order[0];
> +               break;
> +       case V4L2_MBUS_FMT_SRGGB10_1X10:
> +               order = cfa_coef_order[1];
> +               break;
> +       case V4L2_MBUS_FMT_SBGGR10_1X10:
> +               order = cfa_coef_order[2];
> +               break;
> +       case V4L2_MBUS_FMT_SGBRG10_1X10:
> +               order = cfa_coef_order[3];
> +               break;
> +       }
> +
> +       isp_reg_writel(isp, ISPPRV_CFA_TABLE_ADDR,
> +                      OMAP3_ISP_IOMEM_PREV, ISPPRV_SET_TBL_ADDR);
> +
> +       for (i = 0; i < 4; ++i) {
> +               __u32 *block = params->cfa.table
> +                            + order[i] * OMAP3ISP_PREV_CFA_BLK_SIZE;
> +
> +               for (j = 0; j < OMAP3ISP_PREV_CFA_BLK_SIZE; ++j)
> +                       isp_reg_writel(isp, block[j], OMAP3_ISP_IOMEM_PREV,
> +                                      ISPPRV_SET_TBL_DATA);
> +       }
>  }
>
>  /*
> --
> Regards,
>
> Laurent Pinchart
>

Re: [PATCH] omap3isp: preview: Add support for non-GRBG Bayer patterns

[Laurent Pinchart]

Hi Jean-Philippe,

On Thursday 21 June 2012 15:35:52 jean-philippe francois wrote:
> 2012/6/18 Laurent Pinchart <laurent.pinchart@ideasonboard.com>:
> > Rearrange the CFA interpolation coefficients table based on the Bayer
> > pattern. Modifying the table during streaming isn't supported anymore,
> > but didn't make sense in the first place anyway.
> > 
> > Support for non-Bayer CFA patterns is dropped as they were not correctly
> > supported, and have never been tested.
> > 
> > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > ---
> >  drivers/media/video/omap3isp/isppreview.c |  118 ++++++++++++++----------
> >  1 files changed, 67 insertions(+), 51 deletions(-)
> > 
> > Jean-Philippe,
> > 
> > Could you please test this patch on your hardware ?
> 
> Hi,
> 
> I have applied it on top of your omap3isp-next branch, but my board is
> oopsing right after the boot. I will try to get rid of this oops, but if you
> eventually now another tree that includes the changes necessary for this
> patch to apply, it could perhaps save me some time.

The patch should apply on top of the omap3isp-omap3isp-next branch that I've 
just pushed to my linuxtv tree.

> Here is a oops, in case somebody can point me to a patch, but the oops
> is not very specific :
> 
> <6>Total of 96 interrupts on 1 active controller
> <4>omap_hwmod: arm_fck: missing clockdomain for arm_fck.
> <4>omap_hwmod: gpt1_fck: missing clockdomain for gpt1_fck.
> <6>OMAP clockevent source: GPTIMER1 at 32768 Hz
> <6>sched_clock: 32 bits at 32kHz, resolution 30517ns, wraps every
> 131071999ms <1>Unable to handle kernel NULL pointer dereference at virtual
> address 00000000 <1>pgd = c0004000
> <1>[00000000] *pgd=00000000
> <0>Internal error: Oops: 80000005 [#1] PREEMPT ARM
> <d>Modules linked in:
> CPU: 0    Not tainted  (3.4.0-rc3 #2)
> PC is at 0x0
> LR is at __irq_svc+0x40/0x70
> pc : [<00000000>]    lr : [<c000e280>]    psr: 000001d3
> sp : c0461f88  ip : 0000000f  fp : 00000000
> r10: 00000000  r9 : 413fc082  r8 : 80004059
> r7 : c0461fbc  r6 : ffffffff  r5 : 00000153  r4 : c04367e0
> r3 : c0010108  r2 : c0461fd0  r1 : c046b00c  r0 : c0461f88
> Flags: nzcv  IRQs off  FIQs off  Mode SVC_32  ISA ARM  Segment kernel
> Control: 10c5387d  Table: 80004019  DAC: 00000015
> <0>Process swapper (pid: 0, stack limit = 0xc04602e8)
> <0>Stack: (0xc0461f88 to 0xc0462000)
> <0>1f80:                   00007735 000001d3 01ffffff c0468118 00000000
> c046808c <0>1fa0: c0456ec0 c046b004 80004059 413fc082 00000000 00000000
> 0000000f c0461fd0 <0>1fc0: c0010108 c04367e0 00000153 ffffffff 00000000
> 00000000 c04364fc 00000000 <0>1fe0: 00000000 c0456ec4 00000000 10c53c7d
> c046808c 8000803c 00000000 00000000 [<c000e280>] (__irq_svc+0x40/0x70) from
> [<c04367e0>] (start_kernel+0x138/0x254) [<c04367e0>]
> (start_kernel+0x138/0x254) from [<8000803c>] (0x8000803c) <0>Code: bad PC
> value

-- 
Regards,

Laurent Pinchart

Re: [PATCH] omap3isp: preview: Add support for non-GRBG Bayer patterns

[jean-philippe francois]

2012/6/22 Laurent Pinchart <laurent.pinchart@ideasonboard.com>:
> Hi Jean-Philippe,
>
> On Thursday 21 June 2012 15:35:52 jean-philippe francois wrote:
>> 2012/6/18 Laurent Pinchart <laurent.pinchart@ideasonboard.com>:
>> > Rearrange the CFA interpolation coefficients table based on the Bayer
>> > pattern. Modifying the table during streaming isn't supported anymore,
>> > but didn't make sense in the first place anyway.
>> >
>> > Support for non-Bayer CFA patterns is dropped as they were not correctly
>> > supported, and have never been tested.
>> >
>> > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>> > ---
>> >  drivers/media/video/omap3isp/isppreview.c |  118 ++++++++++++++----------
>> >  1 files changed, 67 insertions(+), 51 deletions(-)
>> >
>> > Jean-Philippe,
>> >
>> > Could you please test this patch on your hardware ?
>>
>> Hi,
>>
>> I have applied it on top of your omap3isp-next branch, but my board is
>> oopsing right after the boot. I will try to get rid of this oops, but if you
>> eventually now another tree that includes the changes necessary for this
>> patch to apply, it could perhaps save me some time.
>
> The patch should apply on top of the omap3isp-omap3isp-next branch that I've
> just pushed to my linuxtv tree.
>

Turns out some work was necessary  to do to have a bootable board with
vanilla 3.4.

I successfully tested your patch on top of omap3isp-omap3isp-next
with SRGGB and SBGGR sources.

I tested it on top of your tree as it was 4 days ago, ie with
the last commit being 5472d3f17845c4....

Jean-Philippe François

Re: [PATCH] omap3isp: preview: Add support for non-GRBG Bayer patterns

[Sakari Ailus]

Hi Laurent,

On Mon, Jun 18, 2012 at 04:30:53PM +0200, Laurent Pinchart wrote:
> Rearrange the CFA interpolation coefficients table based on the Bayer
> pattern. Modifying the table during streaming isn't supported anymore,
> but didn't make sense in the first place anyway.

Why not? I could imagine someone might want to change the table while
streaming to change the white balance, for example. Gamma tables or the SRGB
matrix can be used to do mostly the same but we should leave the decision
which one to use to the user space.

> Support for non-Bayer CFA patterns is dropped as they were not correctly
> supported, and have never been tested.
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
>  drivers/media/video/omap3isp/isppreview.c |  118 ++++++++++++++++------------
>  1 files changed, 67 insertions(+), 51 deletions(-)
> 
> Jean-Philippe,
> 
> Could you please test this patch on your hardware ?
> 
> diff --git a/drivers/media/video/omap3isp/isppreview.c b/drivers/media/video/omap3isp/isppreview.c
> index 8a4935e..bfa3107 100644
> --- a/drivers/media/video/omap3isp/isppreview.c
> +++ b/drivers/media/video/omap3isp/isppreview.c
> @@ -309,36 +309,6 @@ preview_config_dcor(struct isp_prev_device *prev, const void *prev_dcor)
>  }
>  
>  /*
> - * preview_config_cfa - Configures the CFA Interpolation parameters.
> - * @prev_cfa: Structure containing the CFA interpolation table, CFA format
> - *            in the image, vertical and horizontal gradient threshold.
> - */
> -static void
> -preview_config_cfa(struct isp_prev_device *prev, const void *prev_cfa)
> -{
> -	struct isp_device *isp = to_isp_device(prev);
> -	const struct omap3isp_prev_cfa *cfa = prev_cfa;
> -	unsigned int i;
> -
> -	isp_reg_clr_set(isp, OMAP3_ISP_IOMEM_PREV, ISPPRV_PCR,
> -			ISPPRV_PCR_CFAFMT_MASK,
> -			cfa->format << ISPPRV_PCR_CFAFMT_SHIFT);
> -
> -	isp_reg_writel(isp,
> -		(cfa->gradthrs_vert << ISPPRV_CFA_GRADTH_VER_SHIFT) |
> -		(cfa->gradthrs_horz << ISPPRV_CFA_GRADTH_HOR_SHIFT),
> -		OMAP3_ISP_IOMEM_PREV, ISPPRV_CFA);
> -
> -	isp_reg_writel(isp, ISPPRV_CFA_TABLE_ADDR,
> -		       OMAP3_ISP_IOMEM_PREV, ISPPRV_SET_TBL_ADDR);
> -
> -	for (i = 0; i < OMAP3ISP_PREV_CFA_TBL_SIZE; i++) {
> -		isp_reg_writel(isp, cfa->table[i],
> -			       OMAP3_ISP_IOMEM_PREV, ISPPRV_SET_TBL_DATA);
> -	}
> -}
> -
> -/*
>   * preview_config_gammacorrn - Configures the Gamma Correction table values
>   * @gtable: Structure containing the table for red, blue, green gamma table.
>   */
> @@ -813,7 +783,7 @@ static const struct preview_update update_attrs[] = {
>  		FIELD_SIZEOF(struct prev_params, hmed),
>  		offsetof(struct omap3isp_prev_update_config, hmed),
>  	}, /* OMAP3ISP_PREV_CFA */ {
> -		preview_config_cfa,
> +		NULL,
>  		NULL,
>  		offsetof(struct prev_params, cfa),
>  		FIELD_SIZEOF(struct prev_params, cfa),
> @@ -1043,42 +1013,88 @@ preview_config_ycpos(struct isp_prev_device *prev,
>  static void preview_config_averager(struct isp_prev_device *prev, u8 average)
>  {
>  	struct isp_device *isp = to_isp_device(prev);
> -	struct prev_params *params;
> -	int reg = 0;
>  
> -	params = (prev->params.active & OMAP3ISP_PREV_CFA)
> -	       ? &prev->params.params[0] : &prev->params.params[1];
> -
> -	if (params->cfa.format == OMAP3ISP_CFAFMT_BAYER)
> -		reg = ISPPRV_AVE_EVENDIST_2 << ISPPRV_AVE_EVENDIST_SHIFT |
> -		      ISPPRV_AVE_ODDDIST_2 << ISPPRV_AVE_ODDDIST_SHIFT |
> -		      average;
> -	else if (params->cfa.format == OMAP3ISP_CFAFMT_RGBFOVEON)
> -		reg = ISPPRV_AVE_EVENDIST_3 << ISPPRV_AVE_EVENDIST_SHIFT |
> -		      ISPPRV_AVE_ODDDIST_3 << ISPPRV_AVE_ODDDIST_SHIFT |
> -		      average;
> -	isp_reg_writel(isp, reg, OMAP3_ISP_IOMEM_PREV, ISPPRV_AVE);
> +	isp_reg_writel(isp, ISPPRV_AVE_EVENDIST_2 << ISPPRV_AVE_EVENDIST_SHIFT |
> +		       ISPPRV_AVE_ODDDIST_2 << ISPPRV_AVE_ODDDIST_SHIFT |
> +		       average, OMAP3_ISP_IOMEM_PREV, ISPPRV_AVE);
>  }
>  
> +
> +#define OMAP3ISP_PREV_CFA_BLK_SIZE	(OMAP3ISP_PREV_CFA_TBL_SIZE / 4)
> +
>  /*
>   * preview_config_input_format - Configure the input format
>   * @prev: The preview engine
>   * @format: Format on the preview engine sink pad
>   *
> - * Enable CFA interpolation for Bayer formats and disable it for greyscale
> - * formats.
> + * Enable and configure CFA interpolation for Bayer formats and disable it for
> + * greyscale formats.
> + *
> + * The CFA table is organised in four blocks, one per Bayer component. The
> + * hardware expects blocks to follow the Bayer order of the input data, while
> + * the driver stores the table in GRBG order in memory. The blocks need to be
> + * reordered to support non-GRBG Bayer patterns.
>   */
>  static void preview_config_input_format(struct isp_prev_device *prev,
>  					const struct v4l2_mbus_framefmt *format)
>  {
> +	static const unsigned int cfa_coef_order[4][4] = {
> +		{ 0, 1, 2, 3 }, /* GRBG */
> +		{ 1, 0, 3, 2 }, /* RGGB */
> +		{ 2, 3, 0, 1 }, /* BGGR */
> +		{ 3, 2, 1, 0 }, /* GBRG */
> +	};
>  	struct isp_device *isp = to_isp_device(prev);
> +	struct prev_params *params;
> +	const unsigned int *order;
> +	unsigned int i;
> +	unsigned int j;
>  
> -	if (format->code != V4L2_MBUS_FMT_Y10_1X10)
> -		isp_reg_set(isp, OMAP3_ISP_IOMEM_PREV, ISPPRV_PCR,
> -			    ISPPRV_PCR_CFAEN);
> -	else
> +	if (format->code == V4L2_MBUS_FMT_Y10_1X10) {
>  		isp_reg_clr(isp, OMAP3_ISP_IOMEM_PREV, ISPPRV_PCR,
>  			    ISPPRV_PCR_CFAEN);
> +		return;
> +	}
> +
> +	params = (prev->params.active & OMAP3ISP_PREV_CFA)
> +	       ? &prev->params.params[0] : &prev->params.params[1];
> +
> +	isp_reg_set(isp, OMAP3_ISP_IOMEM_PREV, ISPPRV_PCR, ISPPRV_PCR_CFAEN);
> +	isp_reg_clr_set(isp, OMAP3_ISP_IOMEM_PREV, ISPPRV_PCR,
> +			ISPPRV_PCR_CFAFMT_MASK, ISPPRV_PCR_CFAFMT_BAYER);
> +
> +	isp_reg_writel(isp,
> +		(params->cfa.gradthrs_vert << ISPPRV_CFA_GRADTH_VER_SHIFT) |
> +		(params->cfa.gradthrs_horz << ISPPRV_CFA_GRADTH_HOR_SHIFT),
> +		OMAP3_ISP_IOMEM_PREV, ISPPRV_CFA);
> +
> +	switch (prev->formats[PREV_PAD_SINK].code) {
> +	case V4L2_MBUS_FMT_SGRBG10_1X10:
> +	default:

Is the "default" case expected to ever happen?

> +		order = cfa_coef_order[0];
> +		break;
> +	case V4L2_MBUS_FMT_SRGGB10_1X10:
> +		order = cfa_coef_order[1];
> +		break;
> +	case V4L2_MBUS_FMT_SBGGR10_1X10:
> +		order = cfa_coef_order[2];
> +		break;
> +	case V4L2_MBUS_FMT_SGBRG10_1X10:
> +		order = cfa_coef_order[3];
> +		break;
> +	}
> +
> +	isp_reg_writel(isp, ISPPRV_CFA_TABLE_ADDR,
> +		       OMAP3_ISP_IOMEM_PREV, ISPPRV_SET_TBL_ADDR);
> +
> +	for (i = 0; i < 4; ++i) {
> +		__u32 *block = params->cfa.table
> +			     + order[i] * OMAP3ISP_PREV_CFA_BLK_SIZE;
> +
> +		for (j = 0; j < OMAP3ISP_PREV_CFA_BLK_SIZE; ++j)
> +			isp_reg_writel(isp, block[j], OMAP3_ISP_IOMEM_PREV,
> +				       ISPPRV_SET_TBL_DATA);
> +	}
>  }
>  
>  /*

Cheers,

-- 
Sakari Ailus
e-mail: sakari.ailus@iki.fi	jabber/XMPP/Gmail: sailus@retiisi.org.uk

Re: [PATCH] omap3isp: preview: Add support for non-GRBG Bayer patterns

[Laurent Pinchart]

Hi Sakari,

On Saturday 23 June 2012 11:22:37 Sakari Ailus wrote:
> On Mon, Jun 18, 2012 at 04:30:53PM +0200, Laurent Pinchart wrote:
> > Rearrange the CFA interpolation coefficients table based on the Bayer
> > pattern. Modifying the table during streaming isn't supported anymore,
> > but didn't make sense in the first place anyway.
> 
> Why not? I could imagine someone might want to change the table while
> streaming to change the white balance, for example. Gamma tables or the SRGB
> matrix can be used to do mostly the same but we should leave the decision
> which one to use to the user space.

Because making the CFA table runtime-configurable brings an additional 
complexity without a use case I'm aware of. The preview engine has separate 
gamma tables, white balance matrices, and RGB-to-RGB and RGB-to-YUV matrices 
that can be modified during streaming. If a user really needs to modify the 
CFA tables during streaming I'll be happy to implement that (and even happier 
to receive a patch :-)), but I'm a bit reluctant to add complexity to an 
already complex code without a real use case.

> > Support for non-Bayer CFA patterns is dropped as they were not correctly
> > supported, and have never been tested.
> > 
> > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > ---
> > 
> >  drivers/media/video/omap3isp/isppreview.c |  118 ++++++++++++++----------
> >  1 files changed, 67 insertions(+), 51 deletions(-)
> > 
> > Jean-Philippe,
> > 
> > Could you please test this patch on your hardware ?
> > 
> > diff --git a/drivers/media/video/omap3isp/isppreview.c
> > b/drivers/media/video/omap3isp/isppreview.c index 8a4935e..bfa3107 100644
> > --- a/drivers/media/video/omap3isp/isppreview.c
> > +++ b/drivers/media/video/omap3isp/isppreview.c
> > @@ -309,36 +309,6 @@ preview_config_dcor(struct isp_prev_device *prev,
> > const void *prev_dcor)> 
> >  }
> >  
> >  /*
> > 
> > - * preview_config_cfa - Configures the CFA Interpolation parameters.
> > - * @prev_cfa: Structure containing the CFA interpolation table, CFA
> > format
> > - *            in the image, vertical and horizontal gradient threshold.
> > - */
> > -static void
> > -preview_config_cfa(struct isp_prev_device *prev, const void *prev_cfa)
> > -{
> > -	struct isp_device *isp = to_isp_device(prev);
> > -	const struct omap3isp_prev_cfa *cfa = prev_cfa;
> > -	unsigned int i;
> > -
> > -	isp_reg_clr_set(isp, OMAP3_ISP_IOMEM_PREV, ISPPRV_PCR,
> > -			ISPPRV_PCR_CFAFMT_MASK,
> > -			cfa->format << ISPPRV_PCR_CFAFMT_SHIFT);
> > -
> > -	isp_reg_writel(isp,
> > -		(cfa->gradthrs_vert << ISPPRV_CFA_GRADTH_VER_SHIFT) |
> > -		(cfa->gradthrs_horz << ISPPRV_CFA_GRADTH_HOR_SHIFT),
> > -		OMAP3_ISP_IOMEM_PREV, ISPPRV_CFA);
> > -
> > -	isp_reg_writel(isp, ISPPRV_CFA_TABLE_ADDR,
> > -		       OMAP3_ISP_IOMEM_PREV, ISPPRV_SET_TBL_ADDR);
> > -
> > -	for (i = 0; i < OMAP3ISP_PREV_CFA_TBL_SIZE; i++) {
> > -		isp_reg_writel(isp, cfa->table[i],
> > -			       OMAP3_ISP_IOMEM_PREV, ISPPRV_SET_TBL_DATA);
> > -	}
> > -}
> > -
> > -/*
> > 
> >   * preview_config_gammacorrn - Configures the Gamma Correction table
> >   values
> >   * @gtable: Structure containing the table for red, blue, green gamma
> >   table.
> >   */
> > 
> > @@ -813,7 +783,7 @@ static const struct preview_update update_attrs[] = {
> > 
> >  		FIELD_SIZEOF(struct prev_params, hmed),
> >  		offsetof(struct omap3isp_prev_update_config, hmed),
> >  	
> >  	}, /* OMAP3ISP_PREV_CFA */ {
> > 
> > -		preview_config_cfa,
> > +		NULL,
> > 
> >  		NULL,
> >  		offsetof(struct prev_params, cfa),
> >  		FIELD_SIZEOF(struct prev_params, cfa),
> > 
> > @@ -1043,42 +1013,88 @@ preview_config_ycpos(struct isp_prev_device *prev,
> > 
> >  static void preview_config_averager(struct isp_prev_device *prev, u8
> >  average) {
> >  
> >  	struct isp_device *isp = to_isp_device(prev);
> > 
> > -	struct prev_params *params;
> > -	int reg = 0;
> > 
> > -	params = (prev->params.active & OMAP3ISP_PREV_CFA)
> > -	       ? &prev->params.params[0] : &prev->params.params[1];
> > -
> > -	if (params->cfa.format == OMAP3ISP_CFAFMT_BAYER)
> > -		reg = ISPPRV_AVE_EVENDIST_2 << ISPPRV_AVE_EVENDIST_SHIFT |
> > -		      ISPPRV_AVE_ODDDIST_2 << ISPPRV_AVE_ODDDIST_SHIFT |
> > -		      average;
> > -	else if (params->cfa.format == OMAP3ISP_CFAFMT_RGBFOVEON)
> > -		reg = ISPPRV_AVE_EVENDIST_3 << ISPPRV_AVE_EVENDIST_SHIFT |
> > -		      ISPPRV_AVE_ODDDIST_3 << ISPPRV_AVE_ODDDIST_SHIFT |
> > -		      average;
> > -	isp_reg_writel(isp, reg, OMAP3_ISP_IOMEM_PREV, ISPPRV_AVE);
> > +	isp_reg_writel(isp, ISPPRV_AVE_EVENDIST_2 << 
ISPPRV_AVE_EVENDIST_SHIFT |
> > +		       ISPPRV_AVE_ODDDIST_2 << ISPPRV_AVE_ODDDIST_SHIFT |
> > +		       average, OMAP3_ISP_IOMEM_PREV, ISPPRV_AVE);
> > 
> >  }
> > 
> > +
> > +#define OMAP3ISP_PREV_CFA_BLK_SIZE	(OMAP3ISP_PREV_CFA_TBL_SIZE / 4)
> > +
> > 
> >  /*
> >  
> >   * preview_config_input_format - Configure the input format
> >   * @prev: The preview engine
> >   * @format: Format on the preview engine sink pad
> >   *
> > 
> > - * Enable CFA interpolation for Bayer formats and disable it for
> > greyscale
> > - * formats.
> > + * Enable and configure CFA interpolation for Bayer formats and disable
> > it for + * greyscale formats.
> > + *
> > + * The CFA table is organised in four blocks, one per Bayer component.
> > The
> > + * hardware expects blocks to follow the Bayer order of the input data,
> > while + * the driver stores the table in GRBG order in memory. The blocks
> > need to be + * reordered to support non-GRBG Bayer patterns.
> > 
> >   */
> >  
> >  static void preview_config_input_format(struct isp_prev_device *prev,
> >  
> >  					const struct v4l2_mbus_framefmt *format)
> >  
> >  {
> > 
> > +	static const unsigned int cfa_coef_order[4][4] = {
> > +		{ 0, 1, 2, 3 }, /* GRBG */
> > +		{ 1, 0, 3, 2 }, /* RGGB */
> > +		{ 2, 3, 0, 1 }, /* BGGR */
> > +		{ 3, 2, 1, 0 }, /* GBRG */
> > +	};
> > 
> >  	struct isp_device *isp = to_isp_device(prev);
> > 
> > +	struct prev_params *params;
> > +	const unsigned int *order;
> > +	unsigned int i;
> > +	unsigned int j;
> > 
> > -	if (format->code != V4L2_MBUS_FMT_Y10_1X10)
> > -		isp_reg_set(isp, OMAP3_ISP_IOMEM_PREV, ISPPRV_PCR,
> > -			    ISPPRV_PCR_CFAEN);
> > -	else
> > +	if (format->code == V4L2_MBUS_FMT_Y10_1X10) {
> > 
> >  		isp_reg_clr(isp, OMAP3_ISP_IOMEM_PREV, ISPPRV_PCR,
> >  		
> >  			    ISPPRV_PCR_CFAEN);
> > 
> > +		return;
> > +	}
> > +
> > +	params = (prev->params.active & OMAP3ISP_PREV_CFA)
> > +	       ? &prev->params.params[0] : &prev->params.params[1];
> > +
> > +	isp_reg_set(isp, OMAP3_ISP_IOMEM_PREV, ISPPRV_PCR, ISPPRV_PCR_CFAEN);
> > +	isp_reg_clr_set(isp, OMAP3_ISP_IOMEM_PREV, ISPPRV_PCR,
> > +			ISPPRV_PCR_CFAFMT_MASK, ISPPRV_PCR_CFAFMT_BAYER);
> > +
> > +	isp_reg_writel(isp,
> > +		(params->cfa.gradthrs_vert << ISPPRV_CFA_GRADTH_VER_SHIFT) |
> > +		(params->cfa.gradthrs_horz << ISPPRV_CFA_GRADTH_HOR_SHIFT),
> > +		OMAP3_ISP_IOMEM_PREV, ISPPRV_CFA);
> > +
> > +	switch (prev->formats[PREV_PAD_SINK].code) {
> > +	case V4L2_MBUS_FMT_SGRBG10_1X10:
> 
> > +	default:
> Is the "default" case expected to ever happen?
> 
> > +		order = cfa_coef_order[0];
> > +		break;
> > +	case V4L2_MBUS_FMT_SRGGB10_1X10:
> > +		order = cfa_coef_order[1];
> > +		break;
> > +	case V4L2_MBUS_FMT_SBGGR10_1X10:
> > +		order = cfa_coef_order[2];
> > +		break;
> > +	case V4L2_MBUS_FMT_SGBRG10_1X10:
> > +		order = cfa_coef_order[3];
> > +		break;
> > +	}
> > +
> > +	isp_reg_writel(isp, ISPPRV_CFA_TABLE_ADDR,
> > +		       OMAP3_ISP_IOMEM_PREV, ISPPRV_SET_TBL_ADDR);
> > +
> > +	for (i = 0; i < 4; ++i) {
> > +		__u32 *block = params->cfa.table
> > +			     + order[i] * OMAP3ISP_PREV_CFA_BLK_SIZE;
> > +
> > +		for (j = 0; j < OMAP3ISP_PREV_CFA_BLK_SIZE; ++j)
> > +			isp_reg_writel(isp, block[j], OMAP3_ISP_IOMEM_PREV,
> > +				       ISPPRV_SET_TBL_DATA);
> > +	}
> > 
> >  }
> >  
> >  /*

-- 
Regards,

Laurent Pinchart

Re: [PATCH] omap3isp: preview: Add support for non-GRBG Bayer patterns

[Sakari Ailus]

On Tue, Jun 26, 2012 at 03:30:09AM +0200, Laurent Pinchart wrote:
> Hi Sakari,

Hi Laurent,

> On Saturday 23 June 2012 11:22:37 Sakari Ailus wrote:
> > On Mon, Jun 18, 2012 at 04:30:53PM +0200, Laurent Pinchart wrote:
> > > Rearrange the CFA interpolation coefficients table based on the Bayer
> > > pattern. Modifying the table during streaming isn't supported anymore,
> > > but didn't make sense in the first place anyway.
> > 
> > Why not? I could imagine someone might want to change the table while
> > streaming to change the white balance, for example. Gamma tables or the SRGB
> > matrix can be used to do mostly the same but we should leave the decision
> > which one to use to the user space.
> 
> Because making the CFA table runtime-configurable brings an additional 
> complexity without a use case I'm aware of. The preview engine has separate 
> gamma tables, white balance matrices, and RGB-to-RGB and RGB-to-YUV matrices 
> that can be modified during streaming. If a user really needs to modify the 
> CFA tables during streaming I'll be happy to implement that (and even happier 
> to receive a patch :-)), but I'm a bit reluctant to add complexity to an 
> already complex code without a real use case.

I'm fine with that. Let's get back to the topic once this is really needed.

...

> > > +		return;
> > > +	}
> > > +
> > > +	params = (prev->params.active & OMAP3ISP_PREV_CFA)
> > > +	       ? &prev->params.params[0] : &prev->params.params[1];
> > > +
> > > +	isp_reg_set(isp, OMAP3_ISP_IOMEM_PREV, ISPPRV_PCR, ISPPRV_PCR_CFAEN);
> > > +	isp_reg_clr_set(isp, OMAP3_ISP_IOMEM_PREV, ISPPRV_PCR,
> > > +			ISPPRV_PCR_CFAFMT_MASK, ISPPRV_PCR_CFAFMT_BAYER);
> > > +
> > > +	isp_reg_writel(isp,
> > > +		(params->cfa.gradthrs_vert << ISPPRV_CFA_GRADTH_VER_SHIFT) |
> > > +		(params->cfa.gradthrs_horz << ISPPRV_CFA_GRADTH_HOR_SHIFT),
> > > +		OMAP3_ISP_IOMEM_PREV, ISPPRV_CFA);
> > > +
> > > +	switch (prev->formats[PREV_PAD_SINK].code) {
> > > +	case V4L2_MBUS_FMT_SGRBG10_1X10:
> > 
> > > +	default:
> > Is the "default" case expected to ever happen?

How about this one?

> > > +		order = cfa_coef_order[0];
> > > +		break;
> > > +	case V4L2_MBUS_FMT_SRGGB10_1X10:
> > > +		order = cfa_coef_order[1];
> > > +		break;
> > > +	case V4L2_MBUS_FMT_SBGGR10_1X10:
> > > +		order = cfa_coef_order[2];
> > > +		break;
> > > +	case V4L2_MBUS_FMT_SGBRG10_1X10:
> > > +		order = cfa_coef_order[3];
> > > +		break;
> > > +	}
> > > +
> > > +	isp_reg_writel(isp, ISPPRV_CFA_TABLE_ADDR,
> > > +		       OMAP3_ISP_IOMEM_PREV, ISPPRV_SET_TBL_ADDR);
> > > +
> > > +	for (i = 0; i < 4; ++i) {
> > > +		__u32 *block = params->cfa.table
> > > +			     + order[i] * OMAP3ISP_PREV_CFA_BLK_SIZE;
> > > +
> > > +		for (j = 0; j < OMAP3ISP_PREV_CFA_BLK_SIZE; ++j)
> > > +			isp_reg_writel(isp, block[j], OMAP3_ISP_IOMEM_PREV,
> > > +				       ISPPRV_SET_TBL_DATA);
> > > +	}
> > > 
> > >  }
> > >  
> > >  /*

Regards,

-- 
Sakari Ailus
e-mail: sakari.ailus@iki.fi	jabber/XMPP/Gmail: sailus@retiisi.org.uk

Re: [PATCH] omap3isp: preview: Add support for non-GRBG Bayer patterns

[Ivan T. Ivanov]

Hi, 

On Tue, 2012-06-26 at 03:30 +0200, Laurent Pinchart wrote:
> Hi Sakari,
> 
> On Saturday 23 June 2012 11:22:37 Sakari Ailus wrote:
> > On Mon, Jun 18, 2012 at 04:30:53PM +0200, Laurent Pinchart wrote:
> > > Rearrange the CFA interpolation coefficients table based on the Bayer
> > > pattern. Modifying the table during streaming isn't supported anymore,
> > > but didn't make sense in the first place anyway.
> > 
> > Why not? I could imagine someone might want to change the table while
> > streaming to change the white balance, for example. Gamma tables or the SRGB
> > matrix can be used to do mostly the same but we should leave the decision
> > which one to use to the user space.
> 
> Because making the CFA table runtime-configurable brings an additional 
> complexity without a use case I'm aware of. The preview engine has separate 
> gamma tables, white balance matrices, and RGB-to-RGB and RGB-to-YUV matrices 
> that can be modified during streaming. If a user really needs to modify the 
> CFA tables during streaming I'll be happy to implement that (and even happier 
> to receive a patch :-)), but I'm a bit reluctant to add complexity to an 
> already complex code without a real use case.
> 

Sorry for not following this thread very closely. One use case for 
changing CFA table is to adjust sharpness of the frames coming out
of the ISP. And we are doing exactly this in N9.

Regards,
Ivan.

Re: [PATCH] omap3isp: preview: Add support for non-GRBG Bayer patterns

[Laurent Pinchart]

Hi Ivan,

On Wednesday 27 June 2012 16:42:01 Ivan T. Ivanov wrote:
> On Tue, 2012-06-26 at 03:30 +0200, Laurent Pinchart wrote:
> > On Saturday 23 June 2012 11:22:37 Sakari Ailus wrote:
> > > On Mon, Jun 18, 2012 at 04:30:53PM +0200, Laurent Pinchart wrote:
> > > > Rearrange the CFA interpolation coefficients table based on the Bayer
> > > > pattern. Modifying the table during streaming isn't supported anymore,
> > > > but didn't make sense in the first place anyway.
> > > 
> > > Why not? I could imagine someone might want to change the table while
> > > streaming to change the white balance, for example. Gamma tables or the
> > > SRGB matrix can be used to do mostly the same but we should leave the
> > > decision which one to use to the user space.
> > 
> > Because making the CFA table runtime-configurable brings an additional
> > complexity without a use case I'm aware of. The preview engine has
> > separate
> > gamma tables, white balance matrices, and RGB-to-RGB and RGB-to-YUV
> > matrices that can be modified during streaming. If a user really needs to
> > modify the CFA tables during streaming I'll be happy to implement that
> > (and even happier to receive a patch :-)), but I'm a bit reluctant to add
> > complexity to an already complex code without a real use case.
> 
> Sorry for not following this thread very closely. One use case for
> changing CFA table is to adjust sharpness of the frames coming out
> of the ISP. And we are doing exactly this in N9.

Thank you for the valuable feedback. Now we have a use case :-) I'll make sure 
the CFA table can be updated during streaming then. Are you fine with always 
specifying the table in SGRBG order, and letting the driver rearrange the 4 
blocks based on the input bayer pattern ?

-- 
Regards,

Laurent Pinchart

Re: [PATCH] omap3isp: preview: Add support for non-GRBG Bayer patterns

[Laurent Pinchart]

Hi Sakari,

On Tuesday 26 June 2012 22:01:14 Sakari Ailus wrote:
> On Tue, Jun 26, 2012 at 03:30:09AM +0200, Laurent Pinchart wrote:
> > On Saturday 23 June 2012 11:22:37 Sakari Ailus wrote:
> > > On Mon, Jun 18, 2012 at 04:30:53PM +0200, Laurent Pinchart wrote:
> > > > Rearrange the CFA interpolation coefficients table based on the Bayer
> > > > pattern. Modifying the table during streaming isn't supported anymore,
> > > > but didn't make sense in the first place anyway.
> > > 
> > > Why not? I could imagine someone might want to change the table while
> > > streaming to change the white balance, for example. Gamma tables or the
> > > SRGB matrix can be used to do mostly the same but we should leave the
> > > decision which one to use to the user space.
> > 
> > Because making the CFA table runtime-configurable brings an additional
> > complexity without a use case I'm aware of. The preview engine has
> > separate
> > gamma tables, white balance matrices, and RGB-to-RGB and RGB-to-YUV
> > matrices that can be modified during streaming. If a user really needs to
> > modify the CFA tables during streaming I'll be happy to implement that
> > (and even happier to receive a patch :-)), but I'm a bit reluctant to add
> > complexity to an already complex code without a real use case.
> 
> I'm fine with that. Let's get back to the topic once this is really needed.

It seems to be really needed now, so I'll fix this.

> ...
> 
> > > > +		return;
> > > > +	}
> > > > +
> > > > +	params = (prev->params.active & OMAP3ISP_PREV_CFA)
> > > > +	       ? &prev->params.params[0] : &prev->params.params[1];
> > > > +
> > > > +	isp_reg_set(isp, OMAP3_ISP_IOMEM_PREV, ISPPRV_PCR,
> > > > ISPPRV_PCR_CFAEN);
> > > > +	isp_reg_clr_set(isp, OMAP3_ISP_IOMEM_PREV, ISPPRV_PCR,
> > > > +			ISPPRV_PCR_CFAFMT_MASK, ISPPRV_PCR_CFAFMT_BAYER);
> > > > +
> > > > +	isp_reg_writel(isp,
> > > > +		(params->cfa.gradthrs_vert << ISPPRV_CFA_GRADTH_VER_SHIFT) |
> > > > +		(params->cfa.gradthrs_horz << ISPPRV_CFA_GRADTH_HOR_SHIFT),
> > > > +		OMAP3_ISP_IOMEM_PREV, ISPPRV_CFA);
> > > > +
> > > > +	switch (prev->formats[PREV_PAD_SINK].code) {
> > > > +	case V4L2_MBUS_FMT_SGRBG10_1X10:
> > > 
> > > > +	default:
> > > Is the "default" case expected to ever happen?
> 
> How about this one?

It's not expected to happen, no. I expected the compiler to produce a warning, 
but it doesn't. I'm not sure if that's good or bad though.

I'll reorder the code to avoid crashes if we get an unexpected format.

> > > > +		order = cfa_coef_order[0];
> > > > +		break;
> > > > +	case V4L2_MBUS_FMT_SRGGB10_1X10:
> > > > +		order = cfa_coef_order[1];
> > > > +		break;
> > > > +	case V4L2_MBUS_FMT_SBGGR10_1X10:
> > > > +		order = cfa_coef_order[2];
> > > > +		break;
> > > > +	case V4L2_MBUS_FMT_SGBRG10_1X10:
> > > > +		order = cfa_coef_order[3];
> > > > +		break;
> > > > +	}
> > > > +
> > > > +	isp_reg_writel(isp, ISPPRV_CFA_TABLE_ADDR,
> > > > +		       OMAP3_ISP_IOMEM_PREV, ISPPRV_SET_TBL_ADDR);
> > > > +
> > > > +	for (i = 0; i < 4; ++i) {
> > > > +		__u32 *block = params->cfa.table
> > > > +			     + order[i] * OMAP3ISP_PREV_CFA_BLK_SIZE;
> > > > +
> > > > +		for (j = 0; j < OMAP3ISP_PREV_CFA_BLK_SIZE; ++j)
> > > > +			isp_reg_writel(isp, block[j], OMAP3_ISP_IOMEM_PREV,
> > > > +				       ISPPRV_SET_TBL_DATA);
> > > > +	}
> > > > 
> > > >  }
> > > >  
> > > >  /*

-- 
Regards,

Laurent Pinchart

Re: [PATCH] omap3isp: preview: Add support for non-GRBG Bayer patterns

[Ivan T. Ivanov]

Hi, 

On Wed, 2012-06-27 at 15:54 +0200, Laurent Pinchart wrote:
> Hi Ivan,
> 
> On Wednesday 27 June 2012 16:42:01 Ivan T. Ivanov wrote:
> > On Tue, 2012-06-26 at 03:30 +0200, Laurent Pinchart wrote:
> > > On Saturday 23 June 2012 11:22:37 Sakari Ailus wrote:
> > > > On Mon, Jun 18, 2012 at 04:30:53PM +0200, Laurent Pinchart wrote:
> > > > > Rearrange the CFA interpolation coefficients table based on the Bayer
> > > > > pattern. Modifying the table during streaming isn't supported anymore,
> > > > > but didn't make sense in the first place anyway.
> > > > 
> > > > Why not? I could imagine someone might want to change the table while
> > > > streaming to change the white balance, for example. Gamma tables or the
> > > > SRGB matrix can be used to do mostly the same but we should leave the
> > > > decision which one to use to the user space.
> > > 
> > > Because making the CFA table runtime-configurable brings an additional
> > > complexity without a use case I'm aware of. The preview engine has
> > > separate
> > > gamma tables, white balance matrices, and RGB-to-RGB and RGB-to-YUV
> > > matrices that can be modified during streaming. If a user really needs to
> > > modify the CFA tables during streaming I'll be happy to implement that
> > > (and even happier to receive a patch :-)), but I'm a bit reluctant to add
> > > complexity to an already complex code without a real use case.
> > 
> > Sorry for not following this thread very closely. One use case for
> > changing CFA table is to adjust sharpness of the frames coming out
> > of the ISP. And we are doing exactly this in N9.
> 
> Thank you for the valuable feedback. Now we have a use case :-) I'll make sure 
> the CFA table can be updated during streaming then. Are you fine with always 
> specifying the table in SGRBG order, and letting the driver rearrange the 4 
> blocks based on the input bayer pattern ?

I am afraid that I am not :-). Primary and secondary cameras of the
above device have different order of the color channels. We are 
selecting desired CFA table pattern based on sensor used. Probably
we can add yet another IOCTL to previewer sub-device, which will 
explicitly overwrite "order" of the user supplied table?

Regards,
Ivan

Re: [PATCH] omap3isp: preview: Add support for non-GRBG Bayer patterns

[Laurent Pinchart]

Hi Ivan,

On Wednesday 27 June 2012 17:30:32 Ivan T. Ivanov wrote:
> On Wed, 2012-06-27 at 15:54 +0200, Laurent Pinchart wrote:
> > On Wednesday 27 June 2012 16:42:01 Ivan T. Ivanov wrote:
> > > On Tue, 2012-06-26 at 03:30 +0200, Laurent Pinchart wrote:
> > > > On Saturday 23 June 2012 11:22:37 Sakari Ailus wrote:
> > > > > On Mon, Jun 18, 2012 at 04:30:53PM +0200, Laurent Pinchart wrote:
> > > > > > Rearrange the CFA interpolation coefficients table based on the
> > > > > > Bayer pattern. Modifying the table during streaming isn't
> > > > > > supported anymore, but didn't make sense in the first place
> > > > > > anyway.
> > > > > 
> > > > > Why not? I could imagine someone might want to change the table
> > > > > while streaming to change the white balance, for example. Gamma
> > > > > tables or the SRGB matrix can be used to do mostly the same but we
> > > > > should leave the decision which one to use to the user space.
> > > > 
> > > > Because making the CFA table runtime-configurable brings an additional
> > > > complexity without a use case I'm aware of. The preview engine has
> > > > separate gamma tables, white balance matrices, and RGB-to-RGB and RGB-
> > > > to-YUV matrices that can be modified during streaming. If a user
> > > > really needs to modify the CFA tables during streaming I'll be happy
> > > > to implement that (and even happier to receive a patch :-)), but I'm a
> > > > bit reluctant to add complexity to an already complex code without a
> > > > real use case.
> > > 
> > > Sorry for not following this thread very closely. One use case for
> > > changing CFA table is to adjust sharpness of the frames coming out
> > > of the ISP. And we are doing exactly this in N9.
> > 
> > Thank you for the valuable feedback. Now we have a use case :-) I'll make
> > sure the CFA table can be updated during streaming then. Are you fine
> > with always specifying the table in SGRBG order, and letting the driver
> > rearrange the 4 blocks based on the input bayer pattern ?
> 
> I am afraid that I am not :-). Primary and secondary cameras of the above
> device have different order of the color channels. We are selecting desired
> CFA table pattern based on sensor used. Probably we can add yet another
> IOCTL to previewer sub-device, which will explicitly overwrite "order" of
> the user supplied table?

The idea is that applications should supply a CFA table in the SGRBG order, 
regardless of the real sensor pattern. The ISP driver will then rearrange the 
table based on the pattern of the select sensor.

This will break compatibility with libomap3camd, but the N9 isn't supported by 
Nokia anymore anyway :-/ BTW, are the CFA tables hardcoded in the libomap3camd 
binary, or are they loaded from an external file ?

-- 
Regards,

Laurent Pinchart

Re: [PATCH] omap3isp: preview: Add support for non-GRBG Bayer patterns

[Ivan T. Ivanov]

Hi Laurent,

On Wed, 2012-06-27 at 16:42 +0200, Laurent Pinchart wrote:
> Hi Ivan,
> 
> On Wednesday 27 June 2012 17:30:32 Ivan T. Ivanov wrote:
> > On Wed, 2012-06-27 at 15:54 +0200, Laurent Pinchart wrote:
> > > On Wednesday 27 June 2012 16:42:01 Ivan T. Ivanov wrote:
> > > > On Tue, 2012-06-26 at 03:30 +0200, Laurent Pinchart wrote:
> > > > > On Saturday 23 June 2012 11:22:37 Sakari Ailus wrote:
> > > > > > On Mon, Jun 18, 2012 at 04:30:53PM +0200, Laurent Pinchart wrote:
> > > > > > > Rearrange the CFA interpolation coefficients table based on the
> > > > > > > Bayer pattern. Modifying the table during streaming isn't
> > > > > > > supported anymore, but didn't make sense in the first place
> > > > > > > anyway.
> > > > > > 
> > > > > > Why not? I could imagine someone might want to change the table
> > > > > > while streaming to change the white balance, for example. Gamma
> > > > > > tables or the SRGB matrix can be used to do mostly the same but we
> > > > > > should leave the decision which one to use to the user space.
> > > > > 
> > > > > Because making the CFA table runtime-configurable brings an additional
> > > > > complexity without a use case I'm aware of. The preview engine has
> > > > > separate gamma tables, white balance matrices, and RGB-to-RGB and RGB-
> > > > > to-YUV matrices that can be modified during streaming. If a user
> > > > > really needs to modify the CFA tables during streaming I'll be happy
> > > > > to implement that (and even happier to receive a patch :-)), but I'm a
> > > > > bit reluctant to add complexity to an already complex code without a
> > > > > real use case.
> > > > 
> > > > Sorry for not following this thread very closely. One use case for
> > > > changing CFA table is to adjust sharpness of the frames coming out
> > > > of the ISP. And we are doing exactly this in N9.
> > > 
> > > Thank you for the valuable feedback. Now we have a use case :-) I'll make
> > > sure the CFA table can be updated during streaming then. Are you fine
> > > with always specifying the table in SGRBG order, and letting the driver
> > > rearrange the 4 blocks based on the input bayer pattern ?
> > 
> > I am afraid that I am not :-). Primary and secondary cameras of the above
> > device have different order of the color channels. We are selecting desired
> > CFA table pattern based on sensor used. Probably we can add yet another
> > IOCTL to previewer sub-device, which will explicitly overwrite "order" of
> > the user supplied table?
> 
> The idea is that applications should supply a CFA table in the SGRBG order, 
> regardless of the real sensor pattern. The ISP driver will then rearrange the 
> table based on the pattern of the select sensor.

I like this idea.

> 
> This will break compatibility with libomap3camd, but the N9 isn't supported by 
> Nokia anymore anyway :-/ 

Same feelings :-/. In this case, I suppose you are free to change it
as you like it.

> BTW, are the CFA tables hardcoded in the libomap3camd 
> binary, or are they loaded from an external file ?

No comments :-)

Regards,
Ivan