diff mbox

[07/11] media: adv7180: change mbus format to UYVY

Message ID 1467846004-12731-8-git-send-email-steve_longerbeam@mentor.com (mailing list archive)
State New, archived
Headers show

Commit Message

Steve Longerbeam July 6, 2016, 11 p.m. UTC
Change the media bus format from YUYV8_2X8 to UYVY8_2X8. Colors
now look correct when capturing with the i.mx6 backend. The other
option is to set the SWPC bit in register 0x27 to swap the Cr and Cb
output samples.

Signed-off-by: Steve Longerbeam <steve_longerbeam@mentor.com>
---
 drivers/media/i2c/adv7180.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Comments

Lars-Peter Clausen July 7, 2016, 3:18 p.m. UTC | #1
On 07/07/2016 01:00 AM, Steve Longerbeam wrote:
> Change the media bus format from YUYV8_2X8 to UYVY8_2X8. Colors
> now look correct when capturing with the i.mx6 backend. The other
> option is to set the SWPC bit in register 0x27 to swap the Cr and Cb
> output samples.
> 
> Signed-off-by: Steve Longerbeam <steve_longerbeam@mentor.com>

The patch is certainly correct from the technical point of view. But we need
to be careful not to break any existing platforms which rely on this
setting. So the alternative solution of changing the default output order is
not an option.

Looking at things it seems like the Renesas vin driver, which is used in
combination with the adv7180 on some boards, uses the return value from
enum_mbus_code to setup the video pipeline. Adding Niklas to Cc, maybe he
can help to test this.

But otherwise

Acked-by: Lars-Peter Clausen <lars@metafoo.de>

> ---
>  drivers/media/i2c/adv7180.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/media/i2c/adv7180.c b/drivers/media/i2c/adv7180.c
> index fff887c..427695d 100644
> --- a/drivers/media/i2c/adv7180.c
> +++ b/drivers/media/i2c/adv7180.c
> @@ -654,7 +654,7 @@ static int adv7180_enum_mbus_code(struct v4l2_subdev *sd,
>  	if (code->index != 0)
>  		return -EINVAL;
>  
> -	code->code = MEDIA_BUS_FMT_YUYV8_2X8;
> +	code->code = MEDIA_BUS_FMT_UYVY8_2X8;
>  
>  	return 0;
>  }
> @@ -664,7 +664,7 @@ static int adv7180_mbus_fmt(struct v4l2_subdev *sd,
>  {
>  	struct adv7180_state *state = to_state(sd);
>  
> -	fmt->code = MEDIA_BUS_FMT_YUYV8_2X8;
> +	fmt->code = MEDIA_BUS_FMT_UYVY8_2X8;
>  	fmt->colorspace = V4L2_COLORSPACE_SMPTE170M;
>  	fmt->width = 720;
>  	fmt->height = state->curr_norm & V4L2_STD_525_60 ? 480 : 576;
> 

--
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
Tim Harvey July 7, 2016, 3:25 p.m. UTC | #2
On Wed, Jul 6, 2016 at 4:00 PM, Steve Longerbeam <slongerbeam@gmail.com> wrote:
> Change the media bus format from YUYV8_2X8 to UYVY8_2X8. Colors
> now look correct when capturing with the i.mx6 backend. The other
> option is to set the SWPC bit in register 0x27 to swap the Cr and Cb
> output samples.
>
> Signed-off-by: Steve Longerbeam <steve_longerbeam@mentor.com>
> ---
>  drivers/media/i2c/adv7180.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/media/i2c/adv7180.c b/drivers/media/i2c/adv7180.c
> index fff887c..427695d 100644
> --- a/drivers/media/i2c/adv7180.c
> +++ b/drivers/media/i2c/adv7180.c
> @@ -654,7 +654,7 @@ static int adv7180_enum_mbus_code(struct v4l2_subdev *sd,
>         if (code->index != 0)
>                 return -EINVAL;
>
> -       code->code = MEDIA_BUS_FMT_YUYV8_2X8;
> +       code->code = MEDIA_BUS_FMT_UYVY8_2X8;
>
>         return 0;
>  }
> @@ -664,7 +664,7 @@ static int adv7180_mbus_fmt(struct v4l2_subdev *sd,
>  {
>         struct adv7180_state *state = to_state(sd);
>
> -       fmt->code = MEDIA_BUS_FMT_YUYV8_2X8;
> +       fmt->code = MEDIA_BUS_FMT_UYVY8_2X8;
>         fmt->colorspace = V4L2_COLORSPACE_SMPTE170M;
>         fmt->width = 720;
>         fmt->height = state->curr_norm & V4L2_STD_525_60 ? 480 : 576;
> --

Steve,

Tested on an IMX6 Gateworks Ventana with IMX6 capture drivers [1].

Tested-by: Tim Harvey <tharvey@gateworks.com>
Acked-by: Tim Harvey <tharvey@gateworks.com>

Added to Cc:
Cc: Lars-Peter Clausen <lars@metafoo.de>

Also adding Cc's to the people who are using the adv7180 on other
boards (renesas r8a779* boards) so we can get some feedback and/or
Tested-by from them:
Cc: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>
Cc: Simon Horman <horms+renesas@verge.net.au>

Regards,

Tim

[1] - http://thread.gmane.org/gmane.linux.drivers.video-input-infrastructure/102914
--
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
Niklas Söderlund July 8, 2016, 10:52 a.m. UTC | #3
On 2016-07-07 17:18:25 +0200, Lars-Peter Clausen wrote:
> On 07/07/2016 01:00 AM, Steve Longerbeam wrote:
> > Change the media bus format from YUYV8_2X8 to UYVY8_2X8. Colors
> > now look correct when capturing with the i.mx6 backend. The other
> > option is to set the SWPC bit in register 0x27 to swap the Cr and Cb
> > output samples.
> > 
> > Signed-off-by: Steve Longerbeam <steve_longerbeam@mentor.com>
> 
> The patch is certainly correct from the technical point of view. But we need
> to be careful not to break any existing platforms which rely on this
> setting. So the alternative solution of changing the default output order is
> not an option.
> 
> Looking at things it seems like the Renesas vin driver, which is used in
> combination with the adv7180 on some boards, uses the return value from
> enum_mbus_code to setup the video pipeline. Adding Niklas to Cc, maybe he
> can help to test this.

Yes this change will make the rcar-vin driver fail to probe since it do 
not recognise the MEDIA_BUS_FMT_UYVY8_2X8 format.

There is a error in the Renesas VIN driver where it looks for the wrong 
media format YUVU but expects UYVY data. This error have masked the bug 
in adv7180 fixed in this commit.

I have have sent a patch '[media] rcar-vin: add legacy mode for wrong 
media bus formats' which tries to remedy this. I would like to see that 
patched (or similar solution) merged before this one as to not break the 
Renesas R-Car2 Koelsch board which uses the adv7180.

If that can be arranged

Acked-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>

> 
> But otherwise
> 
> Acked-by: Lars-Peter Clausen <lars@metafoo.de>
> 
> > ---
> >  drivers/media/i2c/adv7180.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/media/i2c/adv7180.c b/drivers/media/i2c/adv7180.c
> > index fff887c..427695d 100644
> > --- a/drivers/media/i2c/adv7180.c
> > +++ b/drivers/media/i2c/adv7180.c
> > @@ -654,7 +654,7 @@ static int adv7180_enum_mbus_code(struct v4l2_subdev *sd,
> >  	if (code->index != 0)
> >  		return -EINVAL;
> >  
> > -	code->code = MEDIA_BUS_FMT_YUYV8_2X8;
> > +	code->code = MEDIA_BUS_FMT_UYVY8_2X8;
> >  
> >  	return 0;
> >  }
> > @@ -664,7 +664,7 @@ static int adv7180_mbus_fmt(struct v4l2_subdev *sd,
> >  {
> >  	struct adv7180_state *state = to_state(sd);
> >  
> > -	fmt->code = MEDIA_BUS_FMT_YUYV8_2X8;
> > +	fmt->code = MEDIA_BUS_FMT_UYVY8_2X8;
> >  	fmt->colorspace = V4L2_COLORSPACE_SMPTE170M;
> >  	fmt->width = 720;
> >  	fmt->height = state->curr_norm & V4L2_STD_525_60 ? 480 : 576;
> > 
>
diff mbox

Patch

diff --git a/drivers/media/i2c/adv7180.c b/drivers/media/i2c/adv7180.c
index fff887c..427695d 100644
--- a/drivers/media/i2c/adv7180.c
+++ b/drivers/media/i2c/adv7180.c
@@ -654,7 +654,7 @@  static int adv7180_enum_mbus_code(struct v4l2_subdev *sd,
 	if (code->index != 0)
 		return -EINVAL;
 
-	code->code = MEDIA_BUS_FMT_YUYV8_2X8;
+	code->code = MEDIA_BUS_FMT_UYVY8_2X8;
 
 	return 0;
 }
@@ -664,7 +664,7 @@  static int adv7180_mbus_fmt(struct v4l2_subdev *sd,
 {
 	struct adv7180_state *state = to_state(sd);
 
-	fmt->code = MEDIA_BUS_FMT_YUYV8_2X8;
+	fmt->code = MEDIA_BUS_FMT_UYVY8_2X8;
 	fmt->colorspace = V4L2_COLORSPACE_SMPTE170M;
 	fmt->width = 720;
 	fmt->height = state->curr_norm & V4L2_STD_525_60 ? 480 : 576;