[RFC PATCH] omap3isp : fix cfa demosaicing for format other than GRBG

[RFC PATCH] omap3isp : fix cfa demosaicing for format other than GRBG

[jean-philippe francois]

Hi,

omap3 ISP previewer block can convert a raw bayer image
into a UYVY image. Debayering coefficient are stored in
an undocumented table. In the current form, only
GRBG format are converted correctly.

However, the other CFA arrangement can be transformed
in GRBG arrangement by shifting the image window one pixel
to the left or to the bottom.

Here is a patch against vanilla 3.2.17, that was only tested with a
BGGR arrangement.
Is it the right way to fix this issue ?

Thank you,

Jean-Philippe François


Index: b/drivers/media/video/omap3isp/isppreview.c
===================================================================
--- a/drivers/media/video/omap3isp/isppreview.c
+++ b/drivers/media/video/omap3isp/isppreview.c
@@ -96,21 +96,26 @@
  *					  2 lines in other modes
  * Color suppression		2 pixels
  * or luma enhancement
+ *
+ * Bayer pattern shifting 2 pixels, 1 line
  * -------------------------------------------------------------
- * Maximum total		14 pixels, 8 lines
+ * Maximum total		18 pixels, 9 lines
  *
  * The color suppression and luma enhancement filters are applied
after bayer to
  * YUV conversion. They thus can crop one pixel on the left and one
pixel on the
  * right side of the image without changing the color pattern. When both those
  * filters are disabled, the driver must crop the two pixels on the
same side of
- * the image to avoid changing the bayer pattern. The left margin is
thus set to
- * 8 pixels and the right margin to 6 pixels.
+ * the image to avoid changing the bayer pattern.
+ *
+ * Bayer pattern shifting is needed for some bayer pattern. Shifting
+ * will be in the right and bottom direction.
+ * The left margin is thus set to 8 pixels and the right margin to 10 pixels.
  */

 #define PREV_MARGIN_LEFT	8
-#define PREV_MARGIN_RIGHT	6
+#define PREV_MARGIN_RIGHT	10
 #define PREV_MARGIN_TOP		4
-#define PREV_MARGIN_BOTTOM	4
+#define PREV_MARGIN_BOTTOM	5

 #define PREV_MIN_IN_WIDTH	64
 #define PREV_MIN_IN_HEIGHT	8
@@ -1038,6 +1043,34 @@
 		eph += 2;
 		slv -= 2;
 		elv += 2;
+		/* CFA table coef only handle GRBG format. Other format
+		 * can be transformed in GRBG by shifting the pattern :
+		 * BGGR -> GRBG is obtained by a 1 row shift
+		 * RGGB -> GRBG is obtained by a 1 column shift
+		 * GBRG -> GRBG is obtained by a row and column shift
+		 */
+		switch(prev->formats[PREV_PAD_SINK].code) {
+		case V4L2_MBUS_FMT_SRGGB10_1X10:
+			sph += 1;
+			eph += 1;
+			break;
+
+		case V4L2_MBUS_FMT_SBGGR10_1X10:
+			slv += 1;
+			elv += 1;
+			break;
+
+		case V4L2_MBUS_FMT_SGBRG10_1X10:
+			sph += 1;
+			eph += 1;
+			slv += 1;
+			elv += 1;
+			break;
+
+		default:
+			break;
+		}
+
 	}
 	if (params->features & (PREV_DEFECT_COR | PREV_NOISE_FILTER)) {
 		sph -= 2;

Re: [RFC PATCH] omap3isp : fix cfa demosaicing for format other than GRBG

[Laurent Pinchart]

Hi Jean-Philippe,

On Tuesday 29 May 2012 10:24:45 jean-philippe francois wrote:
> Hi,
> 
> omap3 ISP previewer block can convert a raw bayer image into a UYVY image.
> Debayering coefficient are stored in an undocumented table. In the current
> form, only GRBG format are converted correctly.
> 
> However, the other CFA arrangement can be transformed in GRBG arrangement by
> shifting the image window one pixel to the left or to the bottom.
> 
> Here is a patch against vanilla 3.2.17, that was only tested with a BGGR
> arrangement.
> Is it the right way to fix this issue ?

That's really a hack. I'd much rather support other Bayer orders properly by 
modifying the CFA coefficients table.

The table is arranged as 4 blocks of 144 coefficients. If I'm not mistaken (I 
haven't tested it), the blocks should be arranged as follows:

GRBG 0 1 2 3
RGGB 1 0 3 2
BGGR 2 3 0 1
GBRG 3 2 1 0

Would you be able to test that with your BGGR sensor ?

If that's correct, it shouldn't be too difficult to modify the order 
dynamically based on the format.

-- 
Regards,

Laurent Pinchart

Re: [RFC PATCH] omap3isp : fix cfa demosaicing for format other than GRBG

[jean-philippe francois]

2012/5/29 Laurent Pinchart <laurent.pinchart@ideasonboard.com>:
> Hi Jean-Philippe,
>
> On Tuesday 29 May 2012 10:24:45 jean-philippe francois wrote:
>> Hi,
>>
>> omap3 ISP previewer block can convert a raw bayer image into a UYVY image.
>> Debayering coefficient are stored in an undocumented table. In the current
>> form, only GRBG format are converted correctly.
>>
>> However, the other CFA arrangement can be transformed in GRBG arrangement by
>> shifting the image window one pixel to the left or to the bottom.
>>
>> Here is a patch against vanilla 3.2.17, that was only tested with a BGGR
>> arrangement.
>> Is it the right way to fix this issue ?
>
> That's really a hack. I'd much rather support other Bayer orders properly by
> modifying the CFA coefficients table.
>
> The table is arranged as 4 blocks of 144 coefficients. If I'm not mistaken (I
> haven't tested it), the blocks should be arranged as follows:
>
> GRBG 0 1 2 3
> RGGB 1 0 3 2
> BGGR 2 3 0 1
> GBRG 3 2 1 0
>
> Would you be able to test that with your BGGR sensor ?

It is indeed working for BGGR.

>
> If that's correct, it shouldn't be too difficult to modify the order
> dynamically based on the format.

What about something like the patch below ?
(It is easier for me to build images using mainline kernel + patch queue,
but should this patch be a good start, I would switch to a git workflow)

Index: b/drivers/media/video/omap3isp/isppreview.c
===================================================================
--- a/drivers/media/video/omap3isp/isppreview.c
+++ b/drivers/media/video/omap3isp/isppreview.c
@@ -308,6 +308,19 @@
 			dcor->couplet_mode_en ? ISPPRV_PCR_DCCOUP : 0);
 }

+
+/* cfa table is organised in four blocks.
+ * Default ordering is for GRBG arrangement, but changing
+ * the block order allows to interpolate other cfa arrangement
+ */
+static 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 */
+};
+#define CFA_BLK_SIZE (OMAP3ISP_PREV_CFA_TBL_SIZE / 4)
+
 /*
  * preview_config_cfa - Configures the CFA Interpolation parameters.
  * @prev_cfa: Structure containing the CFA interpolation table, CFA format
@@ -319,6 +332,7 @@
 	struct isp_device *isp = to_isp_device(prev);
 	const struct omap3isp_prev_cfa *cfa = prev_cfa;
 	unsigned int i;
+	unsigned int * block_order;

 	isp_reg_clr_set(isp, OMAP3_ISP_IOMEM_PREV, ISPPRV_PCR,
 			ISPPRV_PCR_CFAFMT_MASK,
@@ -332,8 +346,30 @@
 	isp_reg_writel(isp, ISPPRV_CFA_TABLE_ADDR,
 		       OMAP3_ISP_IOMEM_PREV, ISPPRV_SET_TBL_ADDR);

+
+	switch(prev->formats[PREV_PAD_SINK].code) {
+	case V4L2_MBUS_FMT_SRGGB10_1X10 :
+		block_order = cfa_coef_order[1];
+		break;
+
+	case V4L2_MBUS_FMT_SBGGR10_1X10 :
+		block_order = cfa_coef_order[2];
+		break;
+
+	case V4L2_MBUS_FMT_SGBRG10_1X10 :
+		block_order = cfa_coef_order[3];
+		break;
+
+	default :
+		block_order = cfa_coef_order[0];
+	}
+
+
 	for (i = 0; i < OMAP3ISP_PREV_CFA_TBL_SIZE; i++) {
-		isp_reg_writel(isp, cfa->table[i],
+		unsigned int base, offset;
+		base = block_order[i / CFA_BLK_SIZE]*CFA_BLK_SIZE;
+		offset = i % CFA_BLK_SIZE;
+		isp_reg_writel(isp, cfa->table[base + offset],
 			       OMAP3_ISP_IOMEM_PREV, ISPPRV_SET_TBL_DATA);
 	}
 }
@@ -1344,7 +1380,8 @@
 	preview_adjust_bandwidth(prev);

 	preview_config_input_size(prev);
-
+	preview_config_cfa(prev, &prev->params.cfa);
+
 	if (prev->input == PREVIEW_INPUT_CCDC)
 		preview_config_inlineoffset(prev, 0);
 	else


>
> --
> Regards,
>
> Laurent Pinchart