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

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

2015-10-18 Thread 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 
> > > ---
> > > 
> > >   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 */
> >

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

2015-10-18 Thread 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 
---

   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

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

2015-10-14 Thread 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 
---

  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(>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 majord...@vger.kernel.org
More

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

2015-10-14 Thread 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 
---

  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(>lock);

/* Clear any pending interrupt */
--
1.9.1


Thanks
Guennadi



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

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

2015-10-04 Thread 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 
> ---
> 
>  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(>lock);
>   /* Clear any pending interrupt */
> -- 
> 1.9.1
> 

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

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

2015-10-04 Thread 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 
> > ---
> > 
> >  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(>lock);
> > /* Clear any pending interrupt */
> > -- 
> > 1.9.1
> > 
> 
> Thanks
> Guennadi
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

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

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

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

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

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

6 matches

Site Navigation

Mail list logo

Footer information