[PATCH 0/5] media: atmel-isi: enable preview path to output RGB565 format

[PATCH 0/5] media: atmel-isi: enable preview path to output RGB565 format

[Josh Wu]

This series will enable preview path support in atmel-isi. Which can
make atmel-isi convert the YUV format (from sensor) to RGB565 format.


Josh Wu (5):
  media: atmel-isi: correct yuv swap according to different sensor
    outputs
  media: atmel-isi: prepare for the support of preview path
  media: atmel-isi: add code to setup correct resolution for preview
    path
  media: atmel-isi: setup YCC_SWAP correctly when using preview path
  media: atmel-isi: support RGB565 output when sensor output YUV formats

 drivers/media/platform/soc_camera/atmel-isi.c | 181 ++++++++++++++++++++------
 drivers/media/platform/soc_camera/atmel-isi.h |  10 ++
 2 files changed, 148 insertions(+), 43 deletions(-)

-- 
1.9.1

[PATCH 1/5] media: atmel-isi: correct yuv swap according to different sensor outputs

[Josh Wu]

we need to configure the YCC_SWAP bits in ISI_CFG2 according to current
sensor output and Atmel ISI output format.

Current there are two cases Atmel ISI supported:
  1. Atmel ISI outputs YUYV format.
     This case we need to setup YCC_SWAP according to sensor output
     format.
  2. Atmel ISI output a pass-through formats, which means no swap.
     Just setup YCC_SWAP as default with no swap.

Signed-off-by: Josh Wu <josh.wu@atmel.com>
---

 drivers/media/platform/soc_camera/atmel-isi.c | 43 ++++++++++++++++++++-------
 1 file changed, 33 insertions(+), 10 deletions(-)

diff --git a/drivers/media/platform/soc_camera/atmel-isi.c b/drivers/media/platform/soc_camera/atmel-isi.c
index 45e304a..df64294 100644
--- a/drivers/media/platform/soc_camera/atmel-isi.c
+++ b/drivers/media/platform/soc_camera/atmel-isi.c
@@ -103,13 +103,41 @@ static u32 isi_readl(struct atmel_isi *isi, u32 reg)
 	return readl(isi->regs + reg);
 }
 
+static u32 setup_cfg2_yuv_swap(struct atmel_isi *isi,
+		const struct soc_camera_format_xlate *xlate)
+{
+	/* By default, no swap for the codec path of Atmel ISI. So codec
+	* output is same as sensor's output.
+	* For instance, if sensor's output is YUYV, then codec outputs YUYV.
+	* And if sensor's output is UYVY, then codec outputs UYVY.
+	*/
+	u32 cfg2_yuv_swap = ISI_CFG2_YCC_SWAP_DEFAULT;
+
+	if (xlate->host_fmt->fourcc == V4L2_PIX_FMT_YUYV) {
+		/* all convert to YUYV */
+		switch (xlate->code) {
+		case MEDIA_BUS_FMT_VYUY8_2X8:
+			cfg2_yuv_swap = ISI_CFG2_YCC_SWAP_MODE_3;
+			break;
+		case MEDIA_BUS_FMT_UYVY8_2X8:
+			cfg2_yuv_swap = ISI_CFG2_YCC_SWAP_MODE_2;
+			break;
+		case MEDIA_BUS_FMT_YVYU8_2X8:
+			cfg2_yuv_swap = ISI_CFG2_YCC_SWAP_MODE_1;
+			break;
+		}
+	}
+
+	return cfg2_yuv_swap;
+}
+
 static void configure_geometry(struct atmel_isi *isi, u32 width,
-			u32 height, u32 code)
+		u32 height, const struct soc_camera_format_xlate *xlate)
 {
 	u32 cfg2;
 
 	/* According to sensor's output format to set cfg2 */
-	switch (code) {
+	switch (xlate->code) {
 	default:
 	/* Grey */
 	case MEDIA_BUS_FMT_Y8_1X8:
@@ -117,16 +145,11 @@ static void configure_geometry(struct atmel_isi *isi, u32 width,
 		break;
 	/* YUV */
 	case MEDIA_BUS_FMT_VYUY8_2X8:
-		cfg2 = ISI_CFG2_YCC_SWAP_MODE_3 | ISI_CFG2_COL_SPACE_YCbCr;
-		break;
 	case MEDIA_BUS_FMT_UYVY8_2X8:
-		cfg2 = ISI_CFG2_YCC_SWAP_MODE_2 | ISI_CFG2_COL_SPACE_YCbCr;
-		break;
 	case MEDIA_BUS_FMT_YVYU8_2X8:
-		cfg2 = ISI_CFG2_YCC_SWAP_MODE_1 | ISI_CFG2_COL_SPACE_YCbCr;
-		break;
 	case MEDIA_BUS_FMT_YUYV8_2X8:
-		cfg2 = ISI_CFG2_YCC_SWAP_DEFAULT | ISI_CFG2_COL_SPACE_YCbCr;
+		cfg2 = ISI_CFG2_COL_SPACE_YCbCr |
+				setup_cfg2_yuv_swap(isi, xlate);
 		break;
 	/* RGB, TODO */
 	}
@@ -407,7 +430,7 @@ static int start_streaming(struct vb2_queue *vq, unsigned int count)
 	isi_writel(isi, ISI_INTDIS, (u32)~0UL);
 
 	configure_geometry(isi, icd->user_width, icd->user_height,
-				icd->current_fmt->code);
+				icd->current_fmt);
 
 	spin_lock_irq(&isi->lock);
 	/* Clear any pending interrupt */
-- 
1.9.1

[PATCH 2/5] media: atmel-isi: prepare for the support of preview path

[Josh Wu]

Atmel ISI support a preview path which can output RGB data.

So this patch introduces a bool variable to choose which path is
enabled currently. And also we need setup corresponding path registers.

By default the preview path is disabled. We only use Codec path.

Signed-off-by: Josh Wu <josh.wu@atmel.com>
---

 drivers/media/platform/soc_camera/atmel-isi.c | 72 ++++++++++++++++++---------
 1 file changed, 49 insertions(+), 23 deletions(-)

diff --git a/drivers/media/platform/soc_camera/atmel-isi.c b/drivers/media/platform/soc_camera/atmel-isi.c
index df64294..e6f4ade 100644
--- a/drivers/media/platform/soc_camera/atmel-isi.c
+++ b/drivers/media/platform/soc_camera/atmel-isi.c
@@ -79,6 +79,7 @@ struct atmel_isi {
 	dma_addr_t			fb_descriptors_phys;
 	struct				list_head dma_desc_head;
 	struct isi_dma_desc		dma_desc[MAX_BUFFER_NUM];
+	bool				enable_preview_path;
 
 	struct completion		complete;
 	/* ISI peripherial clock */
@@ -199,11 +200,19 @@ static irqreturn_t atmel_isi_handle_streaming(struct atmel_isi *isi)
 		/* start next dma frame. */
 		isi->active = list_entry(isi->video_buffer_list.next,
 					struct frame_buffer, list);
-		isi_writel(isi, ISI_DMA_C_DSCR,
-			(u32)isi->active->p_dma_desc->fbd_phys);
-		isi_writel(isi, ISI_DMA_C_CTRL,
-			ISI_DMA_CTRL_FETCH | ISI_DMA_CTRL_DONE);
-		isi_writel(isi, ISI_DMA_CHER, ISI_DMA_CHSR_C_CH);
+		if (!isi->enable_preview_path) {
+			isi_writel(isi, ISI_DMA_C_DSCR,
+				(u32)isi->active->p_dma_desc->fbd_phys);
+			isi_writel(isi, ISI_DMA_C_CTRL,
+				ISI_DMA_CTRL_FETCH | ISI_DMA_CTRL_DONE);
+			isi_writel(isi, ISI_DMA_CHER, ISI_DMA_CHSR_C_CH);
+		} else {
+			isi_writel(isi, ISI_DMA_P_DSCR,
+				(u32)isi->active->p_dma_desc->fbd_phys);
+			isi_writel(isi, ISI_DMA_P_CTRL,
+				ISI_DMA_CTRL_FETCH | ISI_DMA_CTRL_DONE);
+			isi_writel(isi, ISI_DMA_CHER, ISI_DMA_CHSR_P_CH);
+		}
 	}
 	return IRQ_HANDLED;
 }
@@ -230,7 +239,8 @@ static irqreturn_t isi_interrupt(int irq, void *dev_id)
 		isi_writel(isi, ISI_INTDIS, ISI_CTRL_DIS);
 		ret = IRQ_HANDLED;
 	} else {
-		if (likely(pending & ISI_SR_CXFR_DONE))
+		if (likely(pending & ISI_SR_CXFR_DONE) ||
+				likely(pending & ISI_SR_PXFR_DONE))
 			ret = atmel_isi_handle_streaming(isi);
 	}
 
@@ -372,21 +382,35 @@ static void start_dma(struct atmel_isi *isi, struct frame_buffer *buffer)
 			ISI_SR_CXFR_DONE | ISI_SR_PXFR_DONE);
 
 	/* Check if already in a frame */
-	if (isi_readl(isi, ISI_STATUS) & ISI_CTRL_CDC) {
-		dev_err(isi->soc_host.icd->parent, "Already in frame handling.\n");
-		return;
-	}
+	if (!isi->enable_preview_path) {
+		if (isi_readl(isi, ISI_STATUS) & ISI_CTRL_CDC) {
+			dev_err(isi->soc_host.icd->parent, "Already in frame handling.\n");
+			return;
+		}
 
-	isi_writel(isi, ISI_DMA_C_DSCR, (u32)buffer->p_dma_desc->fbd_phys);
-	isi_writel(isi, ISI_DMA_C_CTRL, ISI_DMA_CTRL_FETCH | ISI_DMA_CTRL_DONE);
-	isi_writel(isi, ISI_DMA_CHER, ISI_DMA_CHSR_C_CH);
+		isi_writel(isi, ISI_DMA_C_DSCR,
+				(u32)buffer->p_dma_desc->fbd_phys);
+		isi_writel(isi, ISI_DMA_C_CTRL,
+				ISI_DMA_CTRL_FETCH | ISI_DMA_CTRL_DONE);
+		isi_writel(isi, ISI_DMA_CHER, ISI_DMA_CHSR_C_CH);
+	} else {
+		isi_writel(isi, ISI_DMA_P_DSCR,
+				(u32)buffer->p_dma_desc->fbd_phys);
+		isi_writel(isi, ISI_DMA_P_CTRL,
+				ISI_DMA_CTRL_FETCH | ISI_DMA_CTRL_DONE);
+		isi_writel(isi, ISI_DMA_CHER, ISI_DMA_CHSR_P_CH);
+	}
 
 	cfg1 &= ~ISI_CFG1_FRATE_DIV_MASK;
 	/* Enable linked list */
 	cfg1 |= isi->pdata.frate | ISI_CFG1_DISCR;
 
-	/* Enable codec path and ISI */
-	ctrl = ISI_CTRL_CDC | ISI_CTRL_EN;
+	/* Enable ISI */
+	ctrl = ISI_CTRL_EN;
+
+	if (!isi->enable_preview_path)
+		ctrl |= ISI_CTRL_CDC;
+
 	isi_writel(isi, ISI_CTRL, ctrl);
 	isi_writel(isi, ISI_CFG1, cfg1);
 }
@@ -462,15 +486,17 @@ static void stop_streaming(struct vb2_queue *vq)
 	}
 	spin_unlock_irq(&isi->lock);
 
-	timeout = jiffies + FRAME_INTERVAL_MILLI_SEC * HZ;
-	/* Wait until the end of the current frame. */
-	while ((isi_readl(isi, ISI_STATUS) & ISI_CTRL_CDC) &&
-			time_before(jiffies, timeout))
-		msleep(1);
+	if (!isi->enable_preview_path) {
+		timeout = jiffies + FRAME_INTERVAL_MILLI_SEC * HZ;
+		/* Wait until the end of the current frame. */
+		while ((isi_readl(isi, ISI_STATUS) & ISI_CTRL_CDC) &&
+				time_before(jiffies, timeout))
+			msleep(1);
 
-	if (time_after(jiffies, timeout))
-		dev_err(icd->parent,
-			"Timeout waiting for finishing codec request\n");
+		if (time_after(jiffies, timeout))
+			dev_err(icd->parent,
+				"Timeout waiting for finishing codec request\n");
+	}
 
 	/* Disable interrupts */
 	isi_writel(isi, ISI_INTDIS,
-- 
1.9.1

[PATCH 3/5] media: atmel-isi: add code to setup correct resolution for preview path

[Josh Wu]

Not like codec path, preview path can do downsampling, so we should setup
a extra preview width, height for it.

This patch add preview resolution setup without down sampling. So currently
preview path will output same size as sensor output size.

Signed-off-by: Josh Wu <josh.wu@atmel.com>
---

 drivers/media/platform/soc_camera/atmel-isi.c | 12 +++++++++++-
 drivers/media/platform/soc_camera/atmel-isi.h | 10 ++++++++++
 2 files changed, 21 insertions(+), 1 deletion(-)

diff --git a/drivers/media/platform/soc_camera/atmel-isi.c b/drivers/media/platform/soc_camera/atmel-isi.c
index e6f4ade..bbf6449 100644
--- a/drivers/media/platform/soc_camera/atmel-isi.c
+++ b/drivers/media/platform/soc_camera/atmel-isi.c
@@ -135,7 +135,7 @@ static u32 setup_cfg2_yuv_swap(struct atmel_isi *isi,
 static void configure_geometry(struct atmel_isi *isi, u32 width,
 		u32 height, const struct soc_camera_format_xlate *xlate)
 {
-	u32 cfg2;
+	u32 cfg2, psize;
 
 	/* According to sensor's output format to set cfg2 */
 	switch (xlate->code) {
@@ -163,6 +163,16 @@ static void configure_geometry(struct atmel_isi *isi, u32 width,
 	cfg2 |= ((height - 1) << ISI_CFG2_IM_VSIZE_OFFSET)
 			& ISI_CFG2_IM_VSIZE_MASK;
 	isi_writel(isi, ISI_CFG2, cfg2);
+
+	/* No down sampling, preview size equal to sensor output size */
+	psize = ((width - 1) << ISI_PSIZE_PREV_HSIZE_OFFSET) &
+		ISI_PSIZE_PREV_HSIZE_MASK;
+	psize |= ((height - 1) << ISI_PSIZE_PREV_VSIZE_OFFSET) &
+		ISI_PSIZE_PREV_VSIZE_MASK;
+	isi_writel(isi, ISI_PSIZE, psize);
+	isi_writel(isi, ISI_PDECF, ISI_PDECF_NO_SAMPLING);
+
+	return;
 }
 
 static bool is_supported(struct soc_camera_device *icd,
diff --git a/drivers/media/platform/soc_camera/atmel-isi.h b/drivers/media/platform/soc_camera/atmel-isi.h
index 5acc771..0acb32a 100644
--- a/drivers/media/platform/soc_camera/atmel-isi.h
+++ b/drivers/media/platform/soc_camera/atmel-isi.h
@@ -79,6 +79,16 @@
 #define ISI_CFG2_IM_VSIZE_MASK		(0x7FF << ISI_CFG2_IM_VSIZE_OFFSET)
 #define ISI_CFG2_IM_HSIZE_MASK		(0x7FF << ISI_CFG2_IM_HSIZE_OFFSET)
 
+/* Bitfields in PSIZE */
+#define ISI_PSIZE_PREV_VSIZE_OFFSET	0
+#define ISI_PSIZE_PREV_HSIZE_OFFSET	16
+#define ISI_PSIZE_PREV_VSIZE_MASK	(0x3FF << ISI_PSIZE_PREV_VSIZE_OFFSET)
+#define ISI_PSIZE_PREV_HSIZE_MASK	(0x3FF << ISI_PSIZE_PREV_HSIZE_OFFSET)
+
+/* Bitfields in PDECF */
+#define ISI_PDECF_DEC_FACTOR_MASK	(0xFF << 0)
+#define	ISI_PDECF_NO_SAMPLING		(16)
+
 /* Bitfields in CTRL */
 /* Also using in SR(ISI_V2) */
 #define ISI_CTRL_EN				(1 << 0)
-- 
1.9.1

[PATCH 4/5] media: atmel-isi: setup YCC_SWAP correctly when using preview path

[Josh Wu]

The preview path only can convert UYVY format to RGB data.

To make preview path work correctly, we need to set up YCC_SWAP
according to sensor output and convert them to UYVY.

Signed-off-by: Josh Wu <josh.wu@atmel.com>
---

 drivers/media/platform/soc_camera/atmel-isi.c | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/drivers/media/platform/soc_camera/atmel-isi.c b/drivers/media/platform/soc_camera/atmel-isi.c
index bbf6449..e87d354 100644
--- a/drivers/media/platform/soc_camera/atmel-isi.c
+++ b/drivers/media/platform/soc_camera/atmel-isi.c
@@ -127,6 +127,22 @@ static u32 setup_cfg2_yuv_swap(struct atmel_isi *isi,
 			cfg2_yuv_swap = ISI_CFG2_YCC_SWAP_MODE_1;
 			break;
 		}
+	} else if (xlate->host_fmt->fourcc == V4L2_PIX_FMT_RGB565) {
+		/* Preview path is enabled, it will convert UYVY to RGB format.
+		 * But if sensor output format is not UYVY, we need to set
+		 * YCC_SWAP_MODE to convert it as UYVY.
+		 */
+		switch (xlate->code) {
+		case MEDIA_BUS_FMT_VYUY8_2X8:
+			cfg2_yuv_swap = ISI_CFG2_YCC_SWAP_MODE_1;
+			break;
+		case MEDIA_BUS_FMT_YUYV8_2X8:
+			cfg2_yuv_swap = ISI_CFG2_YCC_SWAP_MODE_2;
+			break;
+		case MEDIA_BUS_FMT_YVYU8_2X8:
+			cfg2_yuv_swap = ISI_CFG2_YCC_SWAP_MODE_3;
+			break;
+		}
 	}
 
 	return cfg2_yuv_swap;
-- 
1.9.1

[PATCH 5/5] media: atmel-isi: support RGB565 output when sensor output YUV formats

[Josh Wu]

This patch enable Atmel ISI preview path to convert the YUV to RGB format.

Signed-off-by: Josh Wu <josh.wu@atmel.com>
---

 drivers/media/platform/soc_camera/atmel-isi.c | 38 ++++++++++++++++++++-------
 1 file changed, 29 insertions(+), 9 deletions(-)

diff --git a/drivers/media/platform/soc_camera/atmel-isi.c b/drivers/media/platform/soc_camera/atmel-isi.c
index e87d354..e33a16a 100644
--- a/drivers/media/platform/soc_camera/atmel-isi.c
+++ b/drivers/media/platform/soc_camera/atmel-isi.c
@@ -201,13 +201,20 @@ static bool is_supported(struct soc_camera_device *icd,
 	case V4L2_PIX_FMT_UYVY:
 	case V4L2_PIX_FMT_YVYU:
 	case V4L2_PIX_FMT_VYUY:
+	/* RGB */
+	case V4L2_PIX_FMT_RGB565:
 		return true;
-	/* RGB, TODO */
 	default:
 		return false;
 	}
 }
 
+static bool is_output_rgb(const struct soc_mbus_pixelfmt *host_fmt)
+{
+	return host_fmt->fourcc == V4L2_PIX_FMT_RGB565 ||
+			host_fmt->fourcc == V4L2_PIX_FMT_RGB32;
+}
+
 static irqreturn_t atmel_isi_handle_streaming(struct atmel_isi *isi)
 {
 	if (isi->active) {
@@ -467,6 +474,8 @@ static int start_streaming(struct vb2_queue *vq, unsigned int count)
 	struct atmel_isi *isi = ici->priv;
 	int ret;
 
+	isi->enable_preview_path = is_output_rgb(icd->current_fmt->host_fmt);
+
 	pm_runtime_get_sync(ici->v4l2_dev.dev);
 
 	/* Reset ISI */
@@ -688,6 +697,14 @@ static const struct soc_mbus_pixelfmt isi_camera_formats[] = {
 		.order			= SOC_MBUS_ORDER_LE,
 		.layout			= SOC_MBUS_LAYOUT_PACKED,
 	},
+	{
+		.fourcc			= V4L2_PIX_FMT_RGB565,
+		.name			= "RGB565",
+		.bits_per_sample	= 8,
+		.packing		= SOC_MBUS_PACKING_2X8_PADHI,
+		.order			= SOC_MBUS_ORDER_LE,
+		.layout			= SOC_MBUS_LAYOUT_PACKED,
+	},
 };
 
 /* This will be corrected as we get more formats */
@@ -744,7 +761,7 @@ static int isi_camera_get_formats(struct soc_camera_device *icd,
 				  struct soc_camera_format_xlate *xlate)
 {
 	struct v4l2_subdev *sd = soc_camera_to_subdev(icd);
-	int formats = 0, ret;
+	int formats = 0, ret, i, n;
 	/* sensor format */
 	struct v4l2_subdev_mbus_code_enum code = {
 		.which = V4L2_SUBDEV_FORMAT_ACTIVE,
@@ -778,13 +795,16 @@ static int isi_camera_get_formats(struct soc_camera_device *icd,
 	case MEDIA_BUS_FMT_VYUY8_2X8:
 	case MEDIA_BUS_FMT_YUYV8_2X8:
 	case MEDIA_BUS_FMT_YVYU8_2X8:
-		formats++;
-		if (xlate) {
-			xlate->host_fmt	= &isi_camera_formats[0];
-			xlate->code	= code.code;
-			xlate++;
-			dev_dbg(icd->parent, "Providing format %s using code %d\n",
-				isi_camera_formats[0].name, code.code);
+		n = ARRAY_SIZE(isi_camera_formats);
+		formats += n;
+		for (i = 0; i < n; i++) {
+			if (xlate) {
+				xlate->host_fmt	= &isi_camera_formats[i];
+				xlate->code	= code.code;
+				dev_dbg(icd->parent, "Providing format %s using code %d\n",
+					isi_camera_formats[0].name, code.code);
+				xlate++;
+			}
 		}
 		break;
 	default:
-- 
1.9.1

Re: [PATCH 1/5] media: atmel-isi: correct yuv swap according to different sensor outputs

[Guennadi Liakhovetski]

Hi Josh,

On Tue, 22 Sep 2015, Josh Wu wrote:

> we need to configure the YCC_SWAP bits in ISI_CFG2 according to current
> sensor output and Atmel ISI output format.
> 
> Current there are two cases Atmel ISI supported:
>   1. Atmel ISI outputs YUYV format.
>      This case we need to setup YCC_SWAP according to sensor output
>      format.
>   2. Atmel ISI output a pass-through formats, which means no swap.
>      Just setup YCC_SWAP as default with no swap.
> 
> Signed-off-by: Josh Wu <josh.wu@atmel.com>
> ---
> 
>  drivers/media/platform/soc_camera/atmel-isi.c | 43 ++++++++++++++++++++-------
>  1 file changed, 33 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/media/platform/soc_camera/atmel-isi.c b/drivers/media/platform/soc_camera/atmel-isi.c
> index 45e304a..df64294 100644
> --- a/drivers/media/platform/soc_camera/atmel-isi.c
> +++ b/drivers/media/platform/soc_camera/atmel-isi.c
> @@ -103,13 +103,41 @@ static u32 isi_readl(struct atmel_isi *isi, u32 reg)
>  	return readl(isi->regs + reg);
>  }
>  
> +static u32 setup_cfg2_yuv_swap(struct atmel_isi *isi,
> +		const struct soc_camera_format_xlate *xlate)
> +{

This function will be called only for the four media codes from the 
switch-case statement below, namely for

MEDIA_BUS_FMT_VYUY8_2X8
MEDIA_BUS_FMT_UYVY8_2X8
MEDIA_BUS_FMT_YVYU8_2X8
MEDIA_BUS_FMT_YUYV8_2X8

> +	/* By default, no swap for the codec path of Atmel ISI. So codec
> +	* output is same as sensor's output.
> +	* For instance, if sensor's output is YUYV, then codec outputs YUYV.
> +	* And if sensor's output is UYVY, then codec outputs UYVY.
> +	*/
> +	u32 cfg2_yuv_swap = ISI_CFG2_YCC_SWAP_DEFAULT;

Then this ISI_CFG2_YCC_SWAP_DEFAULT will only hold for 
MEDIA_BUS_FMT_YUYV8_2X8? Why don't you just add one more case below? Don't 
think this initialisation is really justified.

> +
> +	if (xlate->host_fmt->fourcc == V4L2_PIX_FMT_YUYV) {
> +		/* all convert to YUYV */
> +		switch (xlate->code) {
> +		case MEDIA_BUS_FMT_VYUY8_2X8:
> +			cfg2_yuv_swap = ISI_CFG2_YCC_SWAP_MODE_3;
> +			break;
> +		case MEDIA_BUS_FMT_UYVY8_2X8:
> +			cfg2_yuv_swap = ISI_CFG2_YCC_SWAP_MODE_2;
> +			break;
> +		case MEDIA_BUS_FMT_YVYU8_2X8:
> +			cfg2_yuv_swap = ISI_CFG2_YCC_SWAP_MODE_1;
> +			break;
> +		}
> +	}
> +
> +	return cfg2_yuv_swap;
> +}
> +
>  static void configure_geometry(struct atmel_isi *isi, u32 width,
> -			u32 height, u32 code)
> +		u32 height, const struct soc_camera_format_xlate *xlate)
>  {
>  	u32 cfg2;
>  
>  	/* According to sensor's output format to set cfg2 */
> -	switch (code) {
> +	switch (xlate->code) {
>  	default:
>  	/* Grey */
>  	case MEDIA_BUS_FMT_Y8_1X8:
> @@ -117,16 +145,11 @@ static void configure_geometry(struct atmel_isi *isi, u32 width,
>  		break;
>  	/* YUV */
>  	case MEDIA_BUS_FMT_VYUY8_2X8:
> -		cfg2 = ISI_CFG2_YCC_SWAP_MODE_3 | ISI_CFG2_COL_SPACE_YCbCr;
> -		break;
>  	case MEDIA_BUS_FMT_UYVY8_2X8:
> -		cfg2 = ISI_CFG2_YCC_SWAP_MODE_2 | ISI_CFG2_COL_SPACE_YCbCr;
> -		break;
>  	case MEDIA_BUS_FMT_YVYU8_2X8:
> -		cfg2 = ISI_CFG2_YCC_SWAP_MODE_1 | ISI_CFG2_COL_SPACE_YCbCr;
> -		break;
>  	case MEDIA_BUS_FMT_YUYV8_2X8:
> -		cfg2 = ISI_CFG2_YCC_SWAP_DEFAULT | ISI_CFG2_COL_SPACE_YCbCr;
> +		cfg2 = ISI_CFG2_COL_SPACE_YCbCr |
> +				setup_cfg2_yuv_swap(isi, xlate);
>  		break;
>  	/* RGB, TODO */
>  	}

I would move this switch-case completely inside setup_cfg2_yuv_swap(). 
Just do

	cfg2 = setup_cfg2_yuv_swap(isi, xlate);

and handle the

 	case MEDIA_BUS_FMT_Y8_1X8:

in the function too. These two switch-case statements really look 
redundant.

> @@ -407,7 +430,7 @@ static int start_streaming(struct vb2_queue *vq, unsigned int count)
>  	isi_writel(isi, ISI_INTDIS, (u32)~0UL);
>  
>  	configure_geometry(isi, icd->user_width, icd->user_height,
> -				icd->current_fmt->code);
> +				icd->current_fmt);
>  
>  	spin_lock_irq(&isi->lock);
>  	/* Clear any pending interrupt */
> -- 
> 1.9.1
> 

Thanks
Guennadi

Re: [PATCH 4/5] media: atmel-isi: setup YCC_SWAP correctly when using preview path

[Guennadi Liakhovetski]

On Tue, 22 Sep 2015, Josh Wu wrote:

> The preview path only can convert UYVY format to RGB data.
> 
> To make preview path work correctly, we need to set up YCC_SWAP
> according to sensor output and convert them to UYVY.
> 
> Signed-off-by: Josh Wu <josh.wu@atmel.com>
> ---
> 
>  drivers/media/platform/soc_camera/atmel-isi.c | 16 ++++++++++++++++
>  1 file changed, 16 insertions(+)
> 
> diff --git a/drivers/media/platform/soc_camera/atmel-isi.c b/drivers/media/platform/soc_camera/atmel-isi.c
> index bbf6449..e87d354 100644
> --- a/drivers/media/platform/soc_camera/atmel-isi.c
> +++ b/drivers/media/platform/soc_camera/atmel-isi.c
> @@ -127,6 +127,22 @@ static u32 setup_cfg2_yuv_swap(struct atmel_isi *isi,
>  			cfg2_yuv_swap = ISI_CFG2_YCC_SWAP_MODE_1;
>  			break;
>  		}
> +	} else if (xlate->host_fmt->fourcc == V4L2_PIX_FMT_RGB565) {
> +		/* Preview path is enabled, it will convert UYVY to RGB format.
> +		 * But if sensor output format is not UYVY, we need to set
> +		 * YCC_SWAP_MODE to convert it as UYVY.
> +		 */

Please, fix multiline comment style:

		/*
		 * ...
		 * ...
		 */

> +		switch (xlate->code) {
> +		case MEDIA_BUS_FMT_VYUY8_2X8:
> +			cfg2_yuv_swap = ISI_CFG2_YCC_SWAP_MODE_1;
> +			break;
> +		case MEDIA_BUS_FMT_YUYV8_2X8:
> +			cfg2_yuv_swap = ISI_CFG2_YCC_SWAP_MODE_2;
> +			break;
> +		case MEDIA_BUS_FMT_YVYU8_2X8:
> +			cfg2_yuv_swap = ISI_CFG2_YCC_SWAP_MODE_3;
> +			break;
> +		}
>  	}
>  
>  	return cfg2_yuv_swap;
> -- 
> 1.9.1
> 

Re: [PATCH 5/5] media: atmel-isi: support RGB565 output when sensor output YUV formats

[Guennadi Liakhovetski]

On Tue, 22 Sep 2015, Josh Wu wrote:

> This patch enable Atmel ISI preview path to convert the YUV to RGB format.
> 
> Signed-off-by: Josh Wu <josh.wu@atmel.com>
> ---
> 
>  drivers/media/platform/soc_camera/atmel-isi.c | 38 ++++++++++++++++++++-------
>  1 file changed, 29 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/media/platform/soc_camera/atmel-isi.c b/drivers/media/platform/soc_camera/atmel-isi.c
> index e87d354..e33a16a 100644
> --- a/drivers/media/platform/soc_camera/atmel-isi.c
> +++ b/drivers/media/platform/soc_camera/atmel-isi.c
> @@ -201,13 +201,20 @@ static bool is_supported(struct soc_camera_device *icd,
>  	case V4L2_PIX_FMT_UYVY:
>  	case V4L2_PIX_FMT_YVYU:
>  	case V4L2_PIX_FMT_VYUY:
> +	/* RGB */
> +	case V4L2_PIX_FMT_RGB565:
>  		return true;
> -	/* RGB, TODO */
>  	default:
>  		return false;
>  	}
>  }
>  
> +static bool is_output_rgb(const struct soc_mbus_pixelfmt *host_fmt)
> +{
> +	return host_fmt->fourcc == V4L2_PIX_FMT_RGB565 ||
> +			host_fmt->fourcc == V4L2_PIX_FMT_RGB32;
> +}
> +

Why not just pass fourcc to this function? Or maybe just embed it in 
start_streaming - it won't clutter it a lot.

>  static irqreturn_t atmel_isi_handle_streaming(struct atmel_isi *isi)
>  {
>  	if (isi->active) {
> @@ -467,6 +474,8 @@ static int start_streaming(struct vb2_queue *vq, unsigned int count)
>  	struct atmel_isi *isi = ici->priv;
>  	int ret;
>  
> +	isi->enable_preview_path = is_output_rgb(icd->current_fmt->host_fmt);
> +
>  	pm_runtime_get_sync(ici->v4l2_dev.dev);
>  
>  	/* Reset ISI */
> @@ -688,6 +697,14 @@ static const struct soc_mbus_pixelfmt isi_camera_formats[] = {
>  		.order			= SOC_MBUS_ORDER_LE,
>  		.layout			= SOC_MBUS_LAYOUT_PACKED,
>  	},
> +	{
> +		.fourcc			= V4L2_PIX_FMT_RGB565,
> +		.name			= "RGB565",
> +		.bits_per_sample	= 8,
> +		.packing		= SOC_MBUS_PACKING_2X8_PADHI,
> +		.order			= SOC_MBUS_ORDER_LE,
> +		.layout			= SOC_MBUS_LAYOUT_PACKED,
> +	},
>  };
>  
>  /* This will be corrected as we get more formats */
> @@ -744,7 +761,7 @@ static int isi_camera_get_formats(struct soc_camera_device *icd,
>  				  struct soc_camera_format_xlate *xlate)
>  {
>  	struct v4l2_subdev *sd = soc_camera_to_subdev(icd);
> -	int formats = 0, ret;
> +	int formats = 0, ret, i, n;
>  	/* sensor format */
>  	struct v4l2_subdev_mbus_code_enum code = {
>  		.which = V4L2_SUBDEV_FORMAT_ACTIVE,
> @@ -778,13 +795,16 @@ static int isi_camera_get_formats(struct soc_camera_device *icd,
>  	case MEDIA_BUS_FMT_VYUY8_2X8:
>  	case MEDIA_BUS_FMT_YUYV8_2X8:
>  	case MEDIA_BUS_FMT_YVYU8_2X8:
> -		formats++;
> -		if (xlate) {
> -			xlate->host_fmt	= &isi_camera_formats[0];
> -			xlate->code	= code.code;
> -			xlate++;
> -			dev_dbg(icd->parent, "Providing format %s using code %d\n",
> -				isi_camera_formats[0].name, code.code);
> +		n = ARRAY_SIZE(isi_camera_formats);
> +		formats += n;
> +		for (i = 0; i < n; i++) {
> +			if (xlate) {

I'd put if outside of the loop, or just do

+		for (i = 0; xlate && i < n; i++) {


> +				xlate->host_fmt	= &isi_camera_formats[i];
> +				xlate->code	= code.code;
> +				dev_dbg(icd->parent, "Providing format %s using code %d\n",
> +					isi_camera_formats[0].name, code.code);
> +				xlate++;
> +			}
>  		}
>  		break;
>  	default:
> -- 
> 1.9.1
> 

Re: [PATCH 1/5] media: atmel-isi: correct yuv swap according to different sensor outputs

[Guennadi Liakhovetski]

Even simpler:

On Sun, 4 Oct 2015, Guennadi Liakhovetski wrote:

> Hi Josh,
> 
> On Tue, 22 Sep 2015, Josh Wu wrote:
> 
> > we need to configure the YCC_SWAP bits in ISI_CFG2 according to current
> > sensor output and Atmel ISI output format.
> > 
> > Current there are two cases Atmel ISI supported:
> >   1. Atmel ISI outputs YUYV format.
> >      This case we need to setup YCC_SWAP according to sensor output
> >      format.
> >   2. Atmel ISI output a pass-through formats, which means no swap.
> >      Just setup YCC_SWAP as default with no swap.
> > 
> > Signed-off-by: Josh Wu <josh.wu@atmel.com>
> > ---
> > 
> >  drivers/media/platform/soc_camera/atmel-isi.c | 43 ++++++++++++++++++++-------
> >  1 file changed, 33 insertions(+), 10 deletions(-)
> > 
> > diff --git a/drivers/media/platform/soc_camera/atmel-isi.c b/drivers/media/platform/soc_camera/atmel-isi.c
> > index 45e304a..df64294 100644
> > --- a/drivers/media/platform/soc_camera/atmel-isi.c
> > +++ b/drivers/media/platform/soc_camera/atmel-isi.c
> > @@ -103,13 +103,41 @@ static u32 isi_readl(struct atmel_isi *isi, u32 reg)
> >  	return readl(isi->regs + reg);
> >  }
> >  
> > +static u32 setup_cfg2_yuv_swap(struct atmel_isi *isi,
> > +		const struct soc_camera_format_xlate *xlate)
> > +{
> 
> This function will be called only for the four media codes from the 
> switch-case statement below, namely for
> 
> MEDIA_BUS_FMT_VYUY8_2X8
> MEDIA_BUS_FMT_UYVY8_2X8
> MEDIA_BUS_FMT_YVYU8_2X8
> MEDIA_BUS_FMT_YUYV8_2X8
> 
> > +	/* By default, no swap for the codec path of Atmel ISI. So codec
> > +	* output is same as sensor's output.
> > +	* For instance, if sensor's output is YUYV, then codec outputs YUYV.
> > +	* And if sensor's output is UYVY, then codec outputs UYVY.
> > +	*/
> > +	u32 cfg2_yuv_swap = ISI_CFG2_YCC_SWAP_DEFAULT;
> 
> Then this ISI_CFG2_YCC_SWAP_DEFAULT will only hold for 
> MEDIA_BUS_FMT_YUYV8_2X8? Why don't you just add one more case below? Don't 
> think this initialisation is really justified.
> 
> > +
> > +	if (xlate->host_fmt->fourcc == V4L2_PIX_FMT_YUYV) {
> > +		/* all convert to YUYV */
> > +		switch (xlate->code) {
> > +		case MEDIA_BUS_FMT_VYUY8_2X8:
> > +			cfg2_yuv_swap = ISI_CFG2_YCC_SWAP_MODE_3;
> > +			break;

Why don't you just return the result value here directly? Then you don't 
need the variable at all

> > +		case MEDIA_BUS_FMT_UYVY8_2X8:
> > +			cfg2_yuv_swap = ISI_CFG2_YCC_SWAP_MODE_2;
> > +			break;
> > +		case MEDIA_BUS_FMT_YVYU8_2X8:
> > +			cfg2_yuv_swap = ISI_CFG2_YCC_SWAP_MODE_1;
> > +			break;
> > +		}
> > +	}
> > +
> > +	return cfg2_yuv_swap;
> > +}
> > +
> >  static void configure_geometry(struct atmel_isi *isi, u32 width,
> > -			u32 height, u32 code)
> > +		u32 height, const struct soc_camera_format_xlate *xlate)
> >  {
> >  	u32 cfg2;
> >  
> >  	/* According to sensor's output format to set cfg2 */
> > -	switch (code) {
> > +	switch (xlate->code) {
> >  	default:
> >  	/* Grey */
> >  	case MEDIA_BUS_FMT_Y8_1X8:
> > @@ -117,16 +145,11 @@ static void configure_geometry(struct atmel_isi *isi, u32 width,
> >  		break;
> >  	/* YUV */
> >  	case MEDIA_BUS_FMT_VYUY8_2X8:
> > -		cfg2 = ISI_CFG2_YCC_SWAP_MODE_3 | ISI_CFG2_COL_SPACE_YCbCr;
> > -		break;
> >  	case MEDIA_BUS_FMT_UYVY8_2X8:
> > -		cfg2 = ISI_CFG2_YCC_SWAP_MODE_2 | ISI_CFG2_COL_SPACE_YCbCr;
> > -		break;
> >  	case MEDIA_BUS_FMT_YVYU8_2X8:
> > -		cfg2 = ISI_CFG2_YCC_SWAP_MODE_1 | ISI_CFG2_COL_SPACE_YCbCr;
> > -		break;
> >  	case MEDIA_BUS_FMT_YUYV8_2X8:
> > -		cfg2 = ISI_CFG2_YCC_SWAP_DEFAULT | ISI_CFG2_COL_SPACE_YCbCr;
> > +		cfg2 = ISI_CFG2_COL_SPACE_YCbCr |
> > +				setup_cfg2_yuv_swap(isi, xlate);
> >  		break;
> >  	/* RGB, TODO */
> >  	}
> 
> I would move this switch-case completely inside setup_cfg2_yuv_swap(). 
> Just do
> 
> 	cfg2 = setup_cfg2_yuv_swap(isi, xlate);
> 
> and handle the
> 
>  	case MEDIA_BUS_FMT_Y8_1X8:
> 
> in the function too. These two switch-case statements really look 
> redundant.
> 
> > @@ -407,7 +430,7 @@ static int start_streaming(struct vb2_queue *vq, unsigned int count)
> >  	isi_writel(isi, ISI_INTDIS, (u32)~0UL);
> >  
> >  	configure_geometry(isi, icd->user_width, icd->user_height,
> > -				icd->current_fmt->code);
> > +				icd->current_fmt);
> >  
> >  	spin_lock_irq(&isi->lock);
> >  	/* Clear any pending interrupt */
> > -- 
> > 1.9.1
> > 
> 
> Thanks
> Guennadi
> 

Re: [PATCH 1/5] media: atmel-isi: correct yuv swap according to different sensor outputs

[Josh Wu]

Hi, Dear Guennadi

Thanks for the review.

On 10/5/2015 12:43 AM, Guennadi Liakhovetski wrote:
> Hi Josh,
>
> On Tue, 22 Sep 2015, Josh Wu wrote:
>
>> we need to configure the YCC_SWAP bits in ISI_CFG2 according to current
>> sensor output and Atmel ISI output format.
>>
>> Current there are two cases Atmel ISI supported:
>>    1. Atmel ISI outputs YUYV format.
>>       This case we need to setup YCC_SWAP according to sensor output
>>       format.
>>    2. Atmel ISI output a pass-through formats, which means no swap.
>>       Just setup YCC_SWAP as default with no swap.
>>
>> Signed-off-by: Josh Wu <josh.wu@atmel.com>
>> ---
>>
>>   drivers/media/platform/soc_camera/atmel-isi.c | 43 ++++++++++++++++++++-------
>>   1 file changed, 33 insertions(+), 10 deletions(-)
>>
>> diff --git a/drivers/media/platform/soc_camera/atmel-isi.c b/drivers/media/platform/soc_camera/atmel-isi.c
>> index 45e304a..df64294 100644
>> --- a/drivers/media/platform/soc_camera/atmel-isi.c
>> +++ b/drivers/media/platform/soc_camera/atmel-isi.c
>> @@ -103,13 +103,41 @@ static u32 isi_readl(struct atmel_isi *isi, u32 reg)
>>   	return readl(isi->regs + reg);
>>   }
>>   
>> +static u32 setup_cfg2_yuv_swap(struct atmel_isi *isi,
>> +		const struct soc_camera_format_xlate *xlate)
>> +{
> This function will be called only for the four media codes from the
> switch-case statement below, namely for
>
> MEDIA_BUS_FMT_VYUY8_2X8
> MEDIA_BUS_FMT_UYVY8_2X8
> MEDIA_BUS_FMT_YVYU8_2X8
> MEDIA_BUS_FMT_YUYV8_2X8
>
>> +	/* By default, no swap for the codec path of Atmel ISI. So codec
>> +	* output is same as sensor's output.
>> +	* For instance, if sensor's output is YUYV, then codec outputs YUYV.
>> +	* And if sensor's output is UYVY, then codec outputs UYVY.
>> +	*/
>> +	u32 cfg2_yuv_swap = ISI_CFG2_YCC_SWAP_DEFAULT;
> Then this ISI_CFG2_YCC_SWAP_DEFAULT will only hold for
> MEDIA_BUS_FMT_YUYV8_2X8? Why don't you just add one more case below? Don't
> think this initialisation is really justified.
This default initial value is for all host_fmt_fourcc case. Not just for 
V4L2_PIX_FMT_YUYV this case.

>
>> +
>> +	if (xlate->host_fmt->fourcc == V4L2_PIX_FMT_YUYV) {
>> +		/* all convert to YUYV */
>> +		switch (xlate->code) {
>> +		case MEDIA_BUS_FMT_VYUY8_2X8:
>> +			cfg2_yuv_swap = ISI_CFG2_YCC_SWAP_MODE_3;
>> +			break;
>> +		case MEDIA_BUS_FMT_UYVY8_2X8:
>> +			cfg2_yuv_swap = ISI_CFG2_YCC_SWAP_MODE_2;
>> +			break;
>> +		case MEDIA_BUS_FMT_YVYU8_2X8:
>> +			cfg2_yuv_swap = ISI_CFG2_YCC_SWAP_MODE_1;
>> +			break;
>> +		}
>> +	}
>> +
>> +	return cfg2_yuv_swap;
>> +}
>> +
>>   static void configure_geometry(struct atmel_isi *isi, u32 width,
>> -			u32 height, u32 code)
>> +		u32 height, const struct soc_camera_format_xlate *xlate)
>>   {
>>   	u32 cfg2;
>>   
>>   	/* According to sensor's output format to set cfg2 */
>> -	switch (code) {
>> +	switch (xlate->code) {
>>   	default:
>>   	/* Grey */
>>   	case MEDIA_BUS_FMT_Y8_1X8:
>> @@ -117,16 +145,11 @@ static void configure_geometry(struct atmel_isi *isi, u32 width,
>>   		break;
>>   	/* YUV */
>>   	case MEDIA_BUS_FMT_VYUY8_2X8:
>> -		cfg2 = ISI_CFG2_YCC_SWAP_MODE_3 | ISI_CFG2_COL_SPACE_YCbCr;
>> -		break;
>>   	case MEDIA_BUS_FMT_UYVY8_2X8:
>> -		cfg2 = ISI_CFG2_YCC_SWAP_MODE_2 | ISI_CFG2_COL_SPACE_YCbCr;
>> -		break;
>>   	case MEDIA_BUS_FMT_YVYU8_2X8:
>> -		cfg2 = ISI_CFG2_YCC_SWAP_MODE_1 | ISI_CFG2_COL_SPACE_YCbCr;
>> -		break;
>>   	case MEDIA_BUS_FMT_YUYV8_2X8:
>> -		cfg2 = ISI_CFG2_YCC_SWAP_DEFAULT | ISI_CFG2_COL_SPACE_YCbCr;
>> +		cfg2 = ISI_CFG2_COL_SPACE_YCbCr |
>> +				setup_cfg2_yuv_swap(isi, xlate);
>>   		break;
>>   	/* RGB, TODO */
>>   	}
> I would move this switch-case completely inside setup_cfg2_yuv_swap().
> Just do
>
> 	cfg2 = setup_cfg2_yuv_swap(isi, xlate);
>
> and handle the
>
>   	case MEDIA_BUS_FMT_Y8_1X8:
>
> in the function too. These two switch-case statements really look
> redundant.
Technically, you can do that. But for my point of view, the 
setup_cfg2_yuv_swap() only need to setup the yuv swap register field.

And other cfg2 field need to be configured as well, especially in the 
case sensor output a RGB data. That should be implement soon.

>
>> @@ -407,7 +430,7 @@ static int start_streaming(struct vb2_queue *vq, unsigned int count)
>>   	isi_writel(isi, ISI_INTDIS, (u32)~0UL);
>>   
>>   	configure_geometry(isi, icd->user_width, icd->user_height,
>> -				icd->current_fmt->code);
>> +				icd->current_fmt);
>>   
>>   	spin_lock_irq(&isi->lock);
>>   	/* Clear any pending interrupt */
>> -- 
>> 1.9.1
>>
> Thanks
> Guennadi

Best Regards,
Josh Wu

Re: [PATCH 4/5] media: atmel-isi: setup YCC_SWAP correctly when using preview path

[Josh Wu]

Hi, Dear Guennadi

On 10/5/2015 12:50 AM, Guennadi Liakhovetski wrote:
> On Tue, 22 Sep 2015, Josh Wu wrote:
>
>> The preview path only can convert UYVY format to RGB data.
>>
>> To make preview path work correctly, we need to set up YCC_SWAP
>> according to sensor output and convert them to UYVY.
>>
>> Signed-off-by: Josh Wu <josh.wu@atmel.com>
>> ---
>>
>>   drivers/media/platform/soc_camera/atmel-isi.c | 16 ++++++++++++++++
>>   1 file changed, 16 insertions(+)
>>
>> diff --git a/drivers/media/platform/soc_camera/atmel-isi.c b/drivers/media/platform/soc_camera/atmel-isi.c
>> index bbf6449..e87d354 100644
>> --- a/drivers/media/platform/soc_camera/atmel-isi.c
>> +++ b/drivers/media/platform/soc_camera/atmel-isi.c
>> @@ -127,6 +127,22 @@ static u32 setup_cfg2_yuv_swap(struct atmel_isi *isi,
>>   			cfg2_yuv_swap = ISI_CFG2_YCC_SWAP_MODE_1;
>>   			break;
>>   		}
>> +	} else if (xlate->host_fmt->fourcc == V4L2_PIX_FMT_RGB565) {
>> +		/* Preview path is enabled, it will convert UYVY to RGB format.
>> +		 * But if sensor output format is not UYVY, we need to set
>> +		 * YCC_SWAP_MODE to convert it as UYVY.
>> +		 */
> Please, fix multiline comment style:
>
> 		/*
> 		 * ...
> 		 * ...
> 		 */
oh, yes, Sure. I'll fix this. Thanks.

Best Regards,
Josh Wu

>> +		switch (xlate->code) {
>> +		case MEDIA_BUS_FMT_VYUY8_2X8:
>> +			cfg2_yuv_swap = ISI_CFG2_YCC_SWAP_MODE_1;
>> +			break;
>> +		case MEDIA_BUS_FMT_YUYV8_2X8:
>> +			cfg2_yuv_swap = ISI_CFG2_YCC_SWAP_MODE_2;
>> +			break;
>> +		case MEDIA_BUS_FMT_YVYU8_2X8:
>> +			cfg2_yuv_swap = ISI_CFG2_YCC_SWAP_MODE_3;
>> +			break;
>> +		}
>>   	}
>>   
>>   	return cfg2_yuv_swap;
>> -- 
>> 1.9.1
>>

Re: [PATCH 1/5] media: atmel-isi: correct yuv swap according to different sensor outputs

[Josh Wu]

Dear Gueannadi,

On 10/5/2015 1:04 AM, Guennadi Liakhovetski wrote:
> Even simpler:
>
> On Sun, 4 Oct 2015, Guennadi Liakhovetski wrote:
>
>> Hi Josh,
>>
>> On Tue, 22 Sep 2015, Josh Wu wrote:
>>
>>> we need to configure the YCC_SWAP bits in ISI_CFG2 according to current
>>> sensor output and Atmel ISI output format.
>>>
>>> Current there are two cases Atmel ISI supported:
>>>    1. Atmel ISI outputs YUYV format.
>>>       This case we need to setup YCC_SWAP according to sensor output
>>>       format.
>>>    2. Atmel ISI output a pass-through formats, which means no swap.
>>>       Just setup YCC_SWAP as default with no swap.
>>>
>>> Signed-off-by: Josh Wu <josh.wu@atmel.com>
>>> ---
>>>
>>>   drivers/media/platform/soc_camera/atmel-isi.c | 43 ++++++++++++++++++++-------
>>>   1 file changed, 33 insertions(+), 10 deletions(-)
>>>
>>> diff --git a/drivers/media/platform/soc_camera/atmel-isi.c b/drivers/media/platform/soc_camera/atmel-isi.c
>>> index 45e304a..df64294 100644
>>> --- a/drivers/media/platform/soc_camera/atmel-isi.c
>>> +++ b/drivers/media/platform/soc_camera/atmel-isi.c
>>> @@ -103,13 +103,41 @@ static u32 isi_readl(struct atmel_isi *isi, u32 reg)
>>>   	return readl(isi->regs + reg);
>>>   }
>>>   
>>> +static u32 setup_cfg2_yuv_swap(struct atmel_isi *isi,
>>> +		const struct soc_camera_format_xlate *xlate)
>>> +{
>> This function will be called only for the four media codes from the
>> switch-case statement below, namely for
>>
>> MEDIA_BUS_FMT_VYUY8_2X8
>> MEDIA_BUS_FMT_UYVY8_2X8
>> MEDIA_BUS_FMT_YVYU8_2X8
>> MEDIA_BUS_FMT_YUYV8_2X8
>>
>>> +	/* By default, no swap for the codec path of Atmel ISI. So codec
>>> +	* output is same as sensor's output.
>>> +	* For instance, if sensor's output is YUYV, then codec outputs YUYV.
>>> +	* And if sensor's output is UYVY, then codec outputs UYVY.
>>> +	*/
>>> +	u32 cfg2_yuv_swap = ISI_CFG2_YCC_SWAP_DEFAULT;
>> Then this ISI_CFG2_YCC_SWAP_DEFAULT will only hold for
>> MEDIA_BUS_FMT_YUYV8_2X8? Why don't you just add one more case below? Don't
>> think this initialisation is really justified.
>>
>>> +
>>> +	if (xlate->host_fmt->fourcc == V4L2_PIX_FMT_YUYV) {
>>> +		/* all convert to YUYV */
>>> +		switch (xlate->code) {
>>> +		case MEDIA_BUS_FMT_VYUY8_2X8:
>>> +			cfg2_yuv_swap = ISI_CFG2_YCC_SWAP_MODE_3;
>>> +			break;
> Why don't you just return the result value here directly? Then you don't
> need the variable at all

yes, This sounds good. I'll take rid of the cfg2_yuv_swap variable. Thanks.

Best Regards,
Josh Wu
>
>>> +		case MEDIA_BUS_FMT_UYVY8_2X8:
>>> +			cfg2_yuv_swap = ISI_CFG2_YCC_SWAP_MODE_2;
>>> +			break;
>>> +		case MEDIA_BUS_FMT_YVYU8_2X8:
>>> +			cfg2_yuv_swap = ISI_CFG2_YCC_SWAP_MODE_1;
>>> +			break;
>>> +		}
>>> +	}
>>> +
>>> +	return cfg2_yuv_swap;
>>> +}
>>> +
>>>   static void configure_geometry(struct atmel_isi *isi, u32 width,
>>> -			u32 height, u32 code)
>>> +		u32 height, const struct soc_camera_format_xlate *xlate)
>>>   {
>>>   	u32 cfg2;
>>>   
>>>   	/* According to sensor's output format to set cfg2 */
>>> -	switch (code) {
>>> +	switch (xlate->code) {
>>>   	default:
>>>   	/* Grey */
>>>   	case MEDIA_BUS_FMT_Y8_1X8:
>>> @@ -117,16 +145,11 @@ static void configure_geometry(struct atmel_isi *isi, u32 width,
>>>   		break;
>>>   	/* YUV */
>>>   	case MEDIA_BUS_FMT_VYUY8_2X8:
>>> -		cfg2 = ISI_CFG2_YCC_SWAP_MODE_3 | ISI_CFG2_COL_SPACE_YCbCr;
>>> -		break;
>>>   	case MEDIA_BUS_FMT_UYVY8_2X8:
>>> -		cfg2 = ISI_CFG2_YCC_SWAP_MODE_2 | ISI_CFG2_COL_SPACE_YCbCr;
>>> -		break;
>>>   	case MEDIA_BUS_FMT_YVYU8_2X8:
>>> -		cfg2 = ISI_CFG2_YCC_SWAP_MODE_1 | ISI_CFG2_COL_SPACE_YCbCr;
>>> -		break;
>>>   	case MEDIA_BUS_FMT_YUYV8_2X8:
>>> -		cfg2 = ISI_CFG2_YCC_SWAP_DEFAULT | ISI_CFG2_COL_SPACE_YCbCr;
>>> +		cfg2 = ISI_CFG2_COL_SPACE_YCbCr |
>>> +				setup_cfg2_yuv_swap(isi, xlate);
>>>   		break;
>>>   	/* RGB, TODO */
>>>   	}
>> I would move this switch-case completely inside setup_cfg2_yuv_swap().
>> Just do
>>
>> 	cfg2 = setup_cfg2_yuv_swap(isi, xlate);
>>
>> and handle the
>>
>>   	case MEDIA_BUS_FMT_Y8_1X8:
>>
>> in the function too. These two switch-case statements really look
>> redundant.
>>
>>> @@ -407,7 +430,7 @@ static int start_streaming(struct vb2_queue *vq, unsigned int count)
>>>   	isi_writel(isi, ISI_INTDIS, (u32)~0UL);
>>>   
>>>   	configure_geometry(isi, icd->user_width, icd->user_height,
>>> -				icd->current_fmt->code);
>>> +				icd->current_fmt);
>>>   
>>>   	spin_lock_irq(&isi->lock);
>>>   	/* Clear any pending interrupt */
>>> -- 
>>> 1.9.1
>>>
>> Thanks
>> Guennadi
>>

Re: [PATCH 5/5] media: atmel-isi: support RGB565 output when sensor output YUV formats

[Josh Wu]

Dear Guennadi,

Thanks for the review.

On 10/5/2015 1:02 AM, Guennadi Liakhovetski wrote:
> On Tue, 22 Sep 2015, Josh Wu wrote:
>
>> This patch enable Atmel ISI preview path to convert the YUV to RGB format.
>>
>> Signed-off-by: Josh Wu <josh.wu@atmel.com>
>> ---
>>
>>   drivers/media/platform/soc_camera/atmel-isi.c | 38 ++++++++++++++++++++-------
>>   1 file changed, 29 insertions(+), 9 deletions(-)
>>
>> diff --git a/drivers/media/platform/soc_camera/atmel-isi.c b/drivers/media/platform/soc_camera/atmel-isi.c
>> index e87d354..e33a16a 100644
>> --- a/drivers/media/platform/soc_camera/atmel-isi.c
>> +++ b/drivers/media/platform/soc_camera/atmel-isi.c
>> @@ -201,13 +201,20 @@ static bool is_supported(struct soc_camera_device *icd,
>>   	case V4L2_PIX_FMT_UYVY:
>>   	case V4L2_PIX_FMT_YVYU:
>>   	case V4L2_PIX_FMT_VYUY:
>> +	/* RGB */
>> +	case V4L2_PIX_FMT_RGB565:
>>   		return true;
>> -	/* RGB, TODO */
>>   	default:
>>   		return false;
>>   	}
>>   }
>>   
>> +static bool is_output_rgb(const struct soc_mbus_pixelfmt *host_fmt)
>> +{
>> +	return host_fmt->fourcc == V4L2_PIX_FMT_RGB565 ||
>> +			host_fmt->fourcc == V4L2_PIX_FMT_RGB32;
>> +}
>> +
> Why not just pass fourcc to this function? Or maybe just embed it in
> start_streaming - it won't clutter it a lot.

I think pass fourcc to the function is good.
Since configure_geometry() is hardware related, and the 
enable_preview_path is also hardware related, so I prefer initialize 
enable_preview_path in configure_geometry().

>
>>   static irqreturn_t atmel_isi_handle_streaming(struct atmel_isi *isi)
>>   {
>>   	if (isi->active) {
>> @@ -467,6 +474,8 @@ static int start_streaming(struct vb2_queue *vq, unsigned int count)
>>   	struct atmel_isi *isi = ici->priv;
>>   	int ret;
>>   
>> +	isi->enable_preview_path = is_output_rgb(icd->current_fmt->host_fmt);
>> +
>>   	pm_runtime_get_sync(ici->v4l2_dev.dev);
>>   
>>   	/* Reset ISI */
>> @@ -688,6 +697,14 @@ static const struct soc_mbus_pixelfmt isi_camera_formats[] = {
>>   		.order			= SOC_MBUS_ORDER_LE,
>>   		.layout			= SOC_MBUS_LAYOUT_PACKED,
>>   	},
>> +	{
>> +		.fourcc			= V4L2_PIX_FMT_RGB565,
>> +		.name			= "RGB565",
>> +		.bits_per_sample	= 8,
>> +		.packing		= SOC_MBUS_PACKING_2X8_PADHI,
>> +		.order			= SOC_MBUS_ORDER_LE,
>> +		.layout			= SOC_MBUS_LAYOUT_PACKED,
>> +	},
>>   };
>>   
>>   /* This will be corrected as we get more formats */
>> @@ -744,7 +761,7 @@ static int isi_camera_get_formats(struct soc_camera_device *icd,
>>   				  struct soc_camera_format_xlate *xlate)
>>   {
>>   	struct v4l2_subdev *sd = soc_camera_to_subdev(icd);
>> -	int formats = 0, ret;
>> +	int formats = 0, ret, i, n;
>>   	/* sensor format */
>>   	struct v4l2_subdev_mbus_code_enum code = {
>>   		.which = V4L2_SUBDEV_FORMAT_ACTIVE,
>> @@ -778,13 +795,16 @@ static int isi_camera_get_formats(struct soc_camera_device *icd,
>>   	case MEDIA_BUS_FMT_VYUY8_2X8:
>>   	case MEDIA_BUS_FMT_YUYV8_2X8:
>>   	case MEDIA_BUS_FMT_YVYU8_2X8:
>> -		formats++;
>> -		if (xlate) {
>> -			xlate->host_fmt	= &isi_camera_formats[0];
>> -			xlate->code	= code.code;
>> -			xlate++;
>> -			dev_dbg(icd->parent, "Providing format %s using code %d\n",
>> -				isi_camera_formats[0].name, code.code);
>> +		n = ARRAY_SIZE(isi_camera_formats);
>> +		formats += n;
>> +		for (i = 0; i < n; i++) {
>> +			if (xlate) {
> I'd put if outside of the loop, or just do
>
> +		for (i = 0; xlate && i < n; i++) {

yes, that simpler one. I'll take it. Thanks.

Best Regards,
Josh Wu
>
>
>> +				xlate->host_fmt	= &isi_camera_formats[i];
>> +				xlate->code	= code.code;
>> +				dev_dbg(icd->parent, "Providing format %s using code %d\n",
>> +					isi_camera_formats[0].name, code.code);
>> +				xlate++;
>> +			}
>>   		}
>>   		break;
>>   	default:
>> -- 
>> 1.9.1
>>

Re: [PATCH 1/5] media: atmel-isi: correct yuv swap according to different sensor outputs

[Guennadi Liakhovetski]

Hi Josh,

On Wed, 14 Oct 2015, Josh Wu wrote:

> Hi, Dear Guennadi
> 
> Thanks for the review.
> 
> On 10/5/2015 12:43 AM, Guennadi Liakhovetski wrote:
> > Hi Josh,
> > 
> > On Tue, 22 Sep 2015, Josh Wu wrote:
> > 
> > > we need to configure the YCC_SWAP bits in ISI_CFG2 according to current
> > > sensor output and Atmel ISI output format.
> > > 
> > > Current there are two cases Atmel ISI supported:
> > >    1. Atmel ISI outputs YUYV format.
> > >       This case we need to setup YCC_SWAP according to sensor output
> > >       format.
> > >    2. Atmel ISI output a pass-through formats, which means no swap.
> > >       Just setup YCC_SWAP as default with no swap.
> > > 
> > > Signed-off-by: Josh Wu <josh.wu@atmel.com>
> > > ---
> > > 
> > >   drivers/media/platform/soc_camera/atmel-isi.c | 43
> > > ++++++++++++++++++++-------
> > >   1 file changed, 33 insertions(+), 10 deletions(-)
> > > 
> > > diff --git a/drivers/media/platform/soc_camera/atmel-isi.c
> > > b/drivers/media/platform/soc_camera/atmel-isi.c
> > > index 45e304a..df64294 100644
> > > --- a/drivers/media/platform/soc_camera/atmel-isi.c
> > > +++ b/drivers/media/platform/soc_camera/atmel-isi.c
> > > @@ -103,13 +103,41 @@ static u32 isi_readl(struct atmel_isi *isi, u32 reg)
> > >   	return readl(isi->regs + reg);
> > >   }
> > >   +static u32 setup_cfg2_yuv_swap(struct atmel_isi *isi,
> > > +		const struct soc_camera_format_xlate *xlate)
> > > +{
> > This function will be called only for the four media codes from the
> > switch-case statement below, namely for
> > 
> > MEDIA_BUS_FMT_VYUY8_2X8
> > MEDIA_BUS_FMT_UYVY8_2X8
> > MEDIA_BUS_FMT_YVYU8_2X8
> > MEDIA_BUS_FMT_YUYV8_2X8
> > 
> > > +	/* By default, no swap for the codec path of Atmel ISI. So codec
> > > +	* output is same as sensor's output.
> > > +	* For instance, if sensor's output is YUYV, then codec outputs YUYV.
> > > +	* And if sensor's output is UYVY, then codec outputs UYVY.
> > > +	*/
> > > +	u32 cfg2_yuv_swap = ISI_CFG2_YCC_SWAP_DEFAULT;
> > Then this ISI_CFG2_YCC_SWAP_DEFAULT will only hold for
> > MEDIA_BUS_FMT_YUYV8_2X8? Why don't you just add one more case below? Don't
> > think this initialisation is really justified.
> This default initial value is for all host_fmt_fourcc case. Not just for
> V4L2_PIX_FMT_YUYV this case.

Right, yes, missed this. How about this then:

	if (xlate->host_fmt->fourcc != V4L2_PIX_FMT_YUYV)
		return ISI_CFG2_YCC_SWAP_DEFAULT;

	switch (xlate->code) {
	case MEDIA_BUS_FMT_VYUY8_2X8:
		return ISI_CFG2_YCC_SWAP_MODE_3;
	case MEDIA_BUS_FMT_UYVY8_2X8:
		return ISI_CFG2_YCC_SWAP_MODE_2;
	case MEDIA_BUS_FMT_YVYU8_2X8:
		return ISI_CFG2_YCC_SWAP_MODE_1;
	}

	return ISI_CFG2_YCC_SWAP_DEFAULT;

Or even exactly as you have it in your patch only remove the cfg2_yuv_swap 
variable and do "return" directly after each "case MEDIA_..." and one more 
with SWAP_DEFAULT in the end.

> > > +
> > > +	if (xlate->host_fmt->fourcc == V4L2_PIX_FMT_YUYV) {
> > > +		/* all convert to YUYV */
> > > +		switch (xlate->code) {
> > > +		case MEDIA_BUS_FMT_VYUY8_2X8:
> > > +			cfg2_yuv_swap = ISI_CFG2_YCC_SWAP_MODE_3;
> > > +			break;
> > > +		case MEDIA_BUS_FMT_UYVY8_2X8:
> > > +			cfg2_yuv_swap = ISI_CFG2_YCC_SWAP_MODE_2;
> > > +			break;
> > > +		case MEDIA_BUS_FMT_YVYU8_2X8:
> > > +			cfg2_yuv_swap = ISI_CFG2_YCC_SWAP_MODE_1;
> > > +			break;
> > > +		}
> > > +	}
> > > +
> > > +	return cfg2_yuv_swap;
> > > +}
> > > +
> > >   static void configure_geometry(struct atmel_isi *isi, u32 width,
> > > -			u32 height, u32 code)
> > > +		u32 height, const struct soc_camera_format_xlate *xlate)
> > >   {
> > >   	u32 cfg2;
> > >     	/* According to sensor's output format to set cfg2 */
> > > -	switch (code) {
> > > +	switch (xlate->code) {
> > >   	default:
> > >   	/* Grey */
> > >   	case MEDIA_BUS_FMT_Y8_1X8:
> > > @@ -117,16 +145,11 @@ static void configure_geometry(struct atmel_isi
> > > *isi, u32 width,
> > >   		break;
> > >   	/* YUV */
> > >   	case MEDIA_BUS_FMT_VYUY8_2X8:
> > > -		cfg2 = ISI_CFG2_YCC_SWAP_MODE_3 | ISI_CFG2_COL_SPACE_YCbCr;
> > > -		break;
> > >   	case MEDIA_BUS_FMT_UYVY8_2X8:
> > > -		cfg2 = ISI_CFG2_YCC_SWAP_MODE_2 | ISI_CFG2_COL_SPACE_YCbCr;
> > > -		break;
> > >   	case MEDIA_BUS_FMT_YVYU8_2X8:
> > > -		cfg2 = ISI_CFG2_YCC_SWAP_MODE_1 | ISI_CFG2_COL_SPACE_YCbCr;
> > > -		break;
> > >   	case MEDIA_BUS_FMT_YUYV8_2X8:
> > > -		cfg2 = ISI_CFG2_YCC_SWAP_DEFAULT | ISI_CFG2_COL_SPACE_YCbCr;
> > > +		cfg2 = ISI_CFG2_COL_SPACE_YCbCr |
> > > +				setup_cfg2_yuv_swap(isi, xlate);
> > >   		break;
> > >   	/* RGB, TODO */
> > >   	}
> > I would move this switch-case completely inside setup_cfg2_yuv_swap().
> > Just do
> > 
> > 	cfg2 = setup_cfg2_yuv_swap(isi, xlate);
> > 
> > and handle the
> > 
> >   	case MEDIA_BUS_FMT_Y8_1X8:
> > 
> > in the function too. These two switch-case statements really look
> > redundant.
> Technically, you can do that. But for my point of view, the
> setup_cfg2_yuv_swap() only need to setup the yuv swap register field.
> 
> And other cfg2 field need to be configured as well, especially in the case
> sensor output a RGB data. That should be implement soon.

Understand. I don't like redundant code and this certainly looks redundant 
to me. I'd be glad if you could remove this redundancy, but if you feel 
strongly about it, feel free to keep as is.

Thanks
Guennadi

> > > @@ -407,7 +430,7 @@ static int start_streaming(struct vb2_queue *vq,
> > > unsigned int count)
> > >   	isi_writel(isi, ISI_INTDIS, (u32)~0UL);
> > >     	configure_geometry(isi, icd->user_width, icd->user_height,
> > > -				icd->current_fmt->code);
> > > +				icd->current_fmt);
> > >     	spin_lock_irq(&isi->lock);
> > >   	/* Clear any pending interrupt */
> > > -- 
> > > 1.9.1
> > > 
> > Thanks
> > Guennadi
> 
> Best Regards,
> Josh Wu
> 

Re: [PATCH 5/5] media: atmel-isi: support RGB565 output when sensor output YUV formats

[Guennadi Liakhovetski]

Hi Josh,

On Wed, 14 Oct 2015, Josh Wu wrote:

> Dear Guennadi,
> 
> Thanks for the review.
> 
> On 10/5/2015 1:02 AM, Guennadi Liakhovetski wrote:
> > On Tue, 22 Sep 2015, Josh Wu wrote:
> > 
> > > This patch enable Atmel ISI preview path to convert the YUV to RGB format.
> > > 
> > > Signed-off-by: Josh Wu <josh.wu@atmel.com>
> > > ---
> > > 
> > >   drivers/media/platform/soc_camera/atmel-isi.c | 38
> > > ++++++++++++++++++++-------
> > >   1 file changed, 29 insertions(+), 9 deletions(-)
> > > 
> > > diff --git a/drivers/media/platform/soc_camera/atmel-isi.c
> > > b/drivers/media/platform/soc_camera/atmel-isi.c
> > > index e87d354..e33a16a 100644
> > > --- a/drivers/media/platform/soc_camera/atmel-isi.c
> > > +++ b/drivers/media/platform/soc_camera/atmel-isi.c
> > > @@ -201,13 +201,20 @@ static bool is_supported(struct soc_camera_device
> > > *icd,
> > >   	case V4L2_PIX_FMT_UYVY:
> > >   	case V4L2_PIX_FMT_YVYU:
> > >   	case V4L2_PIX_FMT_VYUY:
> > > +	/* RGB */
> > > +	case V4L2_PIX_FMT_RGB565:
> > >   		return true;
> > > -	/* RGB, TODO */
> > >   	default:
> > >   		return false;
> > >   	}
> > >   }
> > >   +static bool is_output_rgb(const struct soc_mbus_pixelfmt *host_fmt)
> > > +{
> > > +	return host_fmt->fourcc == V4L2_PIX_FMT_RGB565 ||
> > > +			host_fmt->fourcc == V4L2_PIX_FMT_RGB32;
> > > +}
> > > +
> > Why not just pass fourcc to this function? Or maybe just embed it in
> > start_streaming - it won't clutter it a lot.
> 
> I think pass fourcc to the function is good.
> Since configure_geometry() is hardware related, and the enable_preview_path is
> also hardware related, so I prefer initialize enable_preview_path in
> configure_geometry().

But you don't, you do it in start_streaming() below. But actually my 
comment was not about _where_ to do it, but whether this calculation is 
worth a separate function. I would just perform this calculation directly 
where you're calling it:

static ... start_streaming(...)
{
	...
	u32 fourcc = icd->current_fmt->host_fmt->fourcc;

	isi->enable_preview_path = fourcc == V4L2_PIX_FMT_RGB565 ||
				fourcc == V4L2_PIX_FMT_RGB32;

Thanks
Guennadi

> > >   static irqreturn_t atmel_isi_handle_streaming(struct atmel_isi *isi)
> > >   {
> > >   	if (isi->active) {
> > > @@ -467,6 +474,8 @@ static int start_streaming(struct vb2_queue *vq,
> > > unsigned int count)
> > >   	struct atmel_isi *isi = ici->priv;
> > >   	int ret;
> > >   +	isi->enable_preview_path = is_output_rgb(icd->current_fmt->host_fmt);
> > > +
> > >   	pm_runtime_get_sync(ici->v4l2_dev.dev);
> > >     	/* Reset ISI */
> > > @@ -688,6 +697,14 @@ static const struct soc_mbus_pixelfmt
> > > isi_camera_formats[] = {
> > >   		.order			= SOC_MBUS_ORDER_LE,
> > >   		.layout			= SOC_MBUS_LAYOUT_PACKED,
> > >   	},
> > > +	{
> > > +		.fourcc			= V4L2_PIX_FMT_RGB565,
> > > +		.name			= "RGB565",
> > > +		.bits_per_sample	= 8,
> > > +		.packing		= SOC_MBUS_PACKING_2X8_PADHI,
> > > +		.order			= SOC_MBUS_ORDER_LE,
> > > +		.layout			= SOC_MBUS_LAYOUT_PACKED,
> > > +	},
> > >   };
> > >     /* This will be corrected as we get more formats */
> > > @@ -744,7 +761,7 @@ static int isi_camera_get_formats(struct
> > > soc_camera_device *icd,
> > >   				  struct soc_camera_format_xlate *xlate)
> > >   {
> > >   	struct v4l2_subdev *sd = soc_camera_to_subdev(icd);
> > > -	int formats = 0, ret;
> > > +	int formats = 0, ret, i, n;
> > >   	/* sensor format */
> > >   	struct v4l2_subdev_mbus_code_enum code = {
> > >   		.which = V4L2_SUBDEV_FORMAT_ACTIVE,
> > > @@ -778,13 +795,16 @@ static int isi_camera_get_formats(struct
> > > soc_camera_device *icd,
> > >   	case MEDIA_BUS_FMT_VYUY8_2X8:
> > >   	case MEDIA_BUS_FMT_YUYV8_2X8:
> > >   	case MEDIA_BUS_FMT_YVYU8_2X8:
> > > -		formats++;
> > > -		if (xlate) {
> > > -			xlate->host_fmt	= &isi_camera_formats[0];
> > > -			xlate->code	= code.code;
> > > -			xlate++;
> > > -			dev_dbg(icd->parent, "Providing format %s using code
> > > %d\n",
> > > -				isi_camera_formats[0].name, code.code);
> > > +		n = ARRAY_SIZE(isi_camera_formats);
> > > +		formats += n;
> > > +		for (i = 0; i < n; i++) {
> > > +			if (xlate) {
> > I'd put if outside of the loop, or just do
> > 
> > +		for (i = 0; xlate && i < n; i++) {
> 
> yes, that simpler one. I'll take it. Thanks.
> 
> Best Regards,
> Josh Wu
> > 
> > 
> > > +				xlate->host_fmt	= &isi_camera_formats[i];
> > > +				xlate->code	= code.code;
> > > +				dev_dbg(icd->parent, "Providing format %s
> > > using code %d\n",
> > > +					isi_camera_formats[0].name,
> > > code.code);
> > > +				xlate++;
> > > +			}
> > >   		}
> > >   		break;
> > >   	default:
> > > -- 
> > > 1.9.1
> > > 
> 

Re: [PATCH 1/5] media: atmel-isi: correct yuv swap according to different sensor outputs

[Josh Wu]

Dear Guennadi,

On 10/19/2015 9:48 AM, Guennadi Liakhovetski wrote:
> Hi Josh,
>
> On Wed, 14 Oct 2015, Josh Wu wrote:
>
>> Hi, Dear Guennadi
>>
>> Thanks for the review.
>>
>> On 10/5/2015 12:43 AM, Guennadi Liakhovetski wrote:
>>> Hi Josh,
>>>
>>> On Tue, 22 Sep 2015, Josh Wu wrote:
>>>
>>>> we need to configure the YCC_SWAP bits in ISI_CFG2 according to current
>>>> sensor output and Atmel ISI output format.
>>>>
>>>> Current there are two cases Atmel ISI supported:
>>>>     1. Atmel ISI outputs YUYV format.
>>>>        This case we need to setup YCC_SWAP according to sensor output
>>>>        format.
>>>>     2. Atmel ISI output a pass-through formats, which means no swap.
>>>>        Just setup YCC_SWAP as default with no swap.
>>>>
>>>> Signed-off-by: Josh Wu <josh.wu@atmel.com>
>>>> ---
>>>>
>>>>    drivers/media/platform/soc_camera/atmel-isi.c | 43
>>>> ++++++++++++++++++++-------
>>>>    1 file changed, 33 insertions(+), 10 deletions(-)
>>>>
>>>> diff --git a/drivers/media/platform/soc_camera/atmel-isi.c
>>>> b/drivers/media/platform/soc_camera/atmel-isi.c
>>>> index 45e304a..df64294 100644
>>>> --- a/drivers/media/platform/soc_camera/atmel-isi.c
>>>> +++ b/drivers/media/platform/soc_camera/atmel-isi.c
>>>> @@ -103,13 +103,41 @@ static u32 isi_readl(struct atmel_isi *isi, u32 reg)
>>>>    	return readl(isi->regs + reg);
>>>>    }
>>>>    +static u32 setup_cfg2_yuv_swap(struct atmel_isi *isi,
>>>> +		const struct soc_camera_format_xlate *xlate)
>>>> +{
>>> This function will be called only for the four media codes from the
>>> switch-case statement below, namely for
>>>
>>> MEDIA_BUS_FMT_VYUY8_2X8
>>> MEDIA_BUS_FMT_UYVY8_2X8
>>> MEDIA_BUS_FMT_YVYU8_2X8
>>> MEDIA_BUS_FMT_YUYV8_2X8
>>>
>>>> +	/* By default, no swap for the codec path of Atmel ISI. So codec
>>>> +	* output is same as sensor's output.
>>>> +	* For instance, if sensor's output is YUYV, then codec outputs YUYV.
>>>> +	* And if sensor's output is UYVY, then codec outputs UYVY.
>>>> +	*/
>>>> +	u32 cfg2_yuv_swap = ISI_CFG2_YCC_SWAP_DEFAULT;
>>> Then this ISI_CFG2_YCC_SWAP_DEFAULT will only hold for
>>> MEDIA_BUS_FMT_YUYV8_2X8? Why don't you just add one more case below? Don't
>>> think this initialisation is really justified.
>> This default initial value is for all host_fmt_fourcc case. Not just for
>> V4L2_PIX_FMT_YUYV this case.
> Right, yes, missed this. How about this then:
>
> 	if (xlate->host_fmt->fourcc != V4L2_PIX_FMT_YUYV)
> 		return ISI_CFG2_YCC_SWAP_DEFAULT;
>
> 	switch (xlate->code) {
> 	case MEDIA_BUS_FMT_VYUY8_2X8:
> 		return ISI_CFG2_YCC_SWAP_MODE_3;
> 	case MEDIA_BUS_FMT_UYVY8_2X8:
> 		return ISI_CFG2_YCC_SWAP_MODE_2;
> 	case MEDIA_BUS_FMT_YVYU8_2X8:
> 		return ISI_CFG2_YCC_SWAP_MODE_1;
> 	}
>
> 	return ISI_CFG2_YCC_SWAP_DEFAULT;
>
> Or even exactly as you have it in your patch only remove the cfg2_yuv_swap
> variable and do "return" directly after each "case MEDIA_..." and one more
> with SWAP_DEFAULT in the end.

Right, that's exactly what I want to do. That looks very neat. Thanks 
for your advice.
I'll fix it in v2.

>
>>>> +
>>>> +	if (xlate->host_fmt->fourcc == V4L2_PIX_FMT_YUYV) {
>>>> +		/* all convert to YUYV */
>>>> +		switch (xlate->code) {
>>>> +		case MEDIA_BUS_FMT_VYUY8_2X8:
>>>> +			cfg2_yuv_swap = ISI_CFG2_YCC_SWAP_MODE_3;
>>>> +			break;
>>>> +		case MEDIA_BUS_FMT_UYVY8_2X8:
>>>> +			cfg2_yuv_swap = ISI_CFG2_YCC_SWAP_MODE_2;
>>>> +			break;
>>>> +		case MEDIA_BUS_FMT_YVYU8_2X8:
>>>> +			cfg2_yuv_swap = ISI_CFG2_YCC_SWAP_MODE_1;
>>>> +			break;
>>>> +		}
>>>> +	}
>>>> +
>>>> +	return cfg2_yuv_swap;
>>>> +}
>>>> +
>>>>    static void configure_geometry(struct atmel_isi *isi, u32 width,
>>>> -			u32 height, u32 code)
>>>> +		u32 height, const struct soc_camera_format_xlate *xlate)
>>>>    {
>>>>    	u32 cfg2;
>>>>      	/* According to sensor's output format to set cfg2 */
>>>> -	switch (code) {
>>>> +	switch (xlate->code) {
>>>>    	default:
>>>>    	/* Grey */
>>>>    	case MEDIA_BUS_FMT_Y8_1X8:
>>>> @@ -117,16 +145,11 @@ static void configure_geometry(struct atmel_isi
>>>> *isi, u32 width,
>>>>    		break;
>>>>    	/* YUV */
>>>>    	case MEDIA_BUS_FMT_VYUY8_2X8:
>>>> -		cfg2 = ISI_CFG2_YCC_SWAP_MODE_3 | ISI_CFG2_COL_SPACE_YCbCr;
>>>> -		break;
>>>>    	case MEDIA_BUS_FMT_UYVY8_2X8:
>>>> -		cfg2 = ISI_CFG2_YCC_SWAP_MODE_2 | ISI_CFG2_COL_SPACE_YCbCr;
>>>> -		break;
>>>>    	case MEDIA_BUS_FMT_YVYU8_2X8:
>>>> -		cfg2 = ISI_CFG2_YCC_SWAP_MODE_1 | ISI_CFG2_COL_SPACE_YCbCr;
>>>> -		break;
>>>>    	case MEDIA_BUS_FMT_YUYV8_2X8:
>>>> -		cfg2 = ISI_CFG2_YCC_SWAP_DEFAULT | ISI_CFG2_COL_SPACE_YCbCr;
>>>> +		cfg2 = ISI_CFG2_COL_SPACE_YCbCr |
>>>> +				setup_cfg2_yuv_swap(isi, xlate);
>>>>    		break;
>>>>    	/* RGB, TODO */
>>>>    	}
>>> I would move this switch-case completely inside setup_cfg2_yuv_swap().
>>> Just do
>>>
>>> 	cfg2 = setup_cfg2_yuv_swap(isi, xlate);
>>>
>>> and handle the
>>>
>>>    	case MEDIA_BUS_FMT_Y8_1X8:
>>>
>>> in the function too. These two switch-case statements really look
>>> redundant.
>> Technically, you can do that. But for my point of view, the
>> setup_cfg2_yuv_swap() only need to setup the yuv swap register field.
>>
>> And other cfg2 field need to be configured as well, especially in the case
>> sensor output a RGB data. That should be implement soon.
> Understand. I don't like redundant code and this certainly looks redundant
> to me. I'd be glad if you could remove this redundancy, but if you feel
> strongly about it, feel free to keep as is.

Good, thanks.

Best Regards,
Josh Wu

>
> Thanks
> Guennadi
>
>>>> @@ -407,7 +430,7 @@ static int start_streaming(struct vb2_queue *vq,
>>>> unsigned int count)
>>>>    	isi_writel(isi, ISI_INTDIS, (u32)~0UL);
>>>>      	configure_geometry(isi, icd->user_width, icd->user_height,
>>>> -				icd->current_fmt->code);
>>>> +				icd->current_fmt);
>>>>      	spin_lock_irq(&isi->lock);
>>>>    	/* Clear any pending interrupt */
>>>> -- 
>>>> 1.9.1
>>>>
>>> Thanks
>>> Guennadi
>> Best Regards,
>> Josh Wu
>>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-media" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 5/5] media: atmel-isi: support RGB565 output when sensor output YUV formats

[Josh Wu]

Dear Guennadi,

On 10/19/2015 10:03 AM, Guennadi Liakhovetski wrote:
> Hi Josh,
>
> On Wed, 14 Oct 2015, Josh Wu wrote:
>
>> Dear Guennadi,
>>
>> Thanks for the review.
>>
>> On 10/5/2015 1:02 AM, Guennadi Liakhovetski wrote:
>>> On Tue, 22 Sep 2015, Josh Wu wrote:
>>>
>>>> This patch enable Atmel ISI preview path to convert the YUV to RGB format.
>>>>
>>>> Signed-off-by: Josh Wu <josh.wu@atmel.com>
>>>> ---
>>>>
>>>>    drivers/media/platform/soc_camera/atmel-isi.c | 38
>>>> ++++++++++++++++++++-------
>>>>    1 file changed, 29 insertions(+), 9 deletions(-)
>>>>
>>>> diff --git a/drivers/media/platform/soc_camera/atmel-isi.c
>>>> b/drivers/media/platform/soc_camera/atmel-isi.c
>>>> index e87d354..e33a16a 100644
>>>> --- a/drivers/media/platform/soc_camera/atmel-isi.c
>>>> +++ b/drivers/media/platform/soc_camera/atmel-isi.c
>>>> @@ -201,13 +201,20 @@ static bool is_supported(struct soc_camera_device
>>>> *icd,
>>>>    	case V4L2_PIX_FMT_UYVY:
>>>>    	case V4L2_PIX_FMT_YVYU:
>>>>    	case V4L2_PIX_FMT_VYUY:
>>>> +	/* RGB */
>>>> +	case V4L2_PIX_FMT_RGB565:
>>>>    		return true;
>>>> -	/* RGB, TODO */
>>>>    	default:
>>>>    		return false;
>>>>    	}
>>>>    }
>>>>    +static bool is_output_rgb(const struct soc_mbus_pixelfmt *host_fmt)
>>>> +{
>>>> +	return host_fmt->fourcc == V4L2_PIX_FMT_RGB565 ||
>>>> +			host_fmt->fourcc == V4L2_PIX_FMT_RGB32;
>>>> +}
>>>> +
>>> Why not just pass fourcc to this function? Or maybe just embed it in
>>> start_streaming - it won't clutter it a lot.
>> I think pass fourcc to the function is good.
>> Since configure_geometry() is hardware related, and the enable_preview_path is
>> also hardware related, so I prefer initialize enable_preview_path in
>> configure_geometry().
> But you don't, you do it in start_streaming() below.

Right, then I'll move it to configure_geometry() in v2..

> But actually my
> comment was not about _where_ to do it, but whether this calculation is
> worth a separate function. I would just perform this calculation directly
> where you're calling it:
>
> static ... start_streaming(...)
> {
> 	...
> 	u32 fourcc = icd->current_fmt->host_fmt->fourcc;
>
> 	isi->enable_preview_path = fourcc == V4L2_PIX_FMT_RGB565 ||
> 				fourcc == V4L2_PIX_FMT_RGB32;

I thought this "function" might be called in other place, but actually 
no one call it.
So yes, I think there is no need to have such separated function. I'll 
fix it in v2. Thanks.

Best Regards,
Josh Wu

>
> Thanks
> Guennadi
>
>>>>    static irqreturn_t atmel_isi_handle_streaming(struct atmel_isi *isi)
>>>>    {
>>>>    	if (isi->active) {
>>>> @@ -467,6 +474,8 @@ static int start_streaming(struct vb2_queue *vq,
>>>> unsigned int count)
>>>>    	struct atmel_isi *isi = ici->priv;
>>>>    	int ret;
>>>>    +	isi->enable_preview_path = is_output_rgb(icd->current_fmt->host_fmt);
>>>> +
>>>>    	pm_runtime_get_sync(ici->v4l2_dev.dev);
>>>>      	/* Reset ISI */
>>>> @@ -688,6 +697,14 @@ static const struct soc_mbus_pixelfmt
>>>> isi_camera_formats[] = {
>>>>    		.order			= SOC_MBUS_ORDER_LE,
>>>>    		.layout			= SOC_MBUS_LAYOUT_PACKED,
>>>>    	},
>>>> +	{
>>>> +		.fourcc			= V4L2_PIX_FMT_RGB565,
>>>> +		.name			= "RGB565",
>>>> +		.bits_per_sample	= 8,
>>>> +		.packing		= SOC_MBUS_PACKING_2X8_PADHI,
>>>> +		.order			= SOC_MBUS_ORDER_LE,
>>>> +		.layout			= SOC_MBUS_LAYOUT_PACKED,
>>>> +	},
>>>>    };
>>>>      /* This will be corrected as we get more formats */
>>>> @@ -744,7 +761,7 @@ static int isi_camera_get_formats(struct
>>>> soc_camera_device *icd,
>>>>    				  struct soc_camera_format_xlate *xlate)
>>>>    {
>>>>    	struct v4l2_subdev *sd = soc_camera_to_subdev(icd);
>>>> -	int formats = 0, ret;
>>>> +	int formats = 0, ret, i, n;
>>>>    	/* sensor format */
>>>>    	struct v4l2_subdev_mbus_code_enum code = {
>>>>    		.which = V4L2_SUBDEV_FORMAT_ACTIVE,
>>>> @@ -778,13 +795,16 @@ static int isi_camera_get_formats(struct
>>>> soc_camera_device *icd,
>>>>    	case MEDIA_BUS_FMT_VYUY8_2X8:
>>>>    	case MEDIA_BUS_FMT_YUYV8_2X8:
>>>>    	case MEDIA_BUS_FMT_YVYU8_2X8:
>>>> -		formats++;
>>>> -		if (xlate) {
>>>> -			xlate->host_fmt	= &isi_camera_formats[0];
>>>> -			xlate->code	= code.code;
>>>> -			xlate++;
>>>> -			dev_dbg(icd->parent, "Providing format %s using code
>>>> %d\n",
>>>> -				isi_camera_formats[0].name, code.code);
>>>> +		n = ARRAY_SIZE(isi_camera_formats);
>>>> +		formats += n;
>>>> +		for (i = 0; i < n; i++) {
>>>> +			if (xlate) {
>>> I'd put if outside of the loop, or just do
>>>
>>> +		for (i = 0; xlate && i < n; i++) {
>> yes, that simpler one. I'll take it. Thanks.
>>
>> Best Regards,
>> Josh Wu
>>>
>>>> +				xlate->host_fmt	= &isi_camera_formats[i];
>>>> +				xlate->code	= code.code;
>>>> +				dev_dbg(icd->parent, "Providing format %s
>>>> using code %d\n",
>>>> +					isi_camera_formats[0].name,
>>>> code.code);
>>>> +				xlate++;
>>>> +			}
>>>>    		}
>>>>    		break;
>>>>    	default:
>>>> -- 
>>>> 1.9.1
>>>>