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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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@xxxxxxxxx>
---

  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

--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html



[Index of Archives]     [Linux Input]     [Video for Linux]     [Gstreamer Embedded]     [Mplayer Users]     [Linux USB Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [Yosemite Backpacking]
  Powered by Linux