Re: [PATCH v2] media: soc_camera: rcar_vin: Add BT.709 24-bit RGB888 input support

[Simon Horman <horms@verge.net.au>]

On Wed, Oct 29, 2014 at 02:31:41PM +0300, Sergei Shtylyov wrote:
> Hello.
> 
> On 10/29/2014 7:11 AM, Simon Horman wrote:
> 
> >>>>From: Koji Matsuoka <koji.matsuoka.xm@renesas.com>
> 
> >>>>Signed-off-by: Koji Matsuoka <koji.matsuoka.xm@renesas.com>
> >>>>Signed-off-by: Yoshihiro Kaneko <ykaneko0929@gmail.com>
> >>>>---
> 
> >>>>This patch is against master branch of linuxtv.org/media_tree.git.
> 
> >>>>v2 [Yoshihiro Kaneko]
> >>>>* remove unused/useless definition as suggested by Sergei Shtylyov
> 
> >>>    I didn't say it's useless, I just suspected that you missed the necessary
> >>>test somewhere...
> 
> >>Sorry for my inaccurate description.
> 
> >>>>   drivers/media/platform/soc_camera/rcar_vin.c | 9 +++++++++
> >>>>   1 file changed, 9 insertions(+)
> 
> >>>>diff --git a/drivers/media/platform/soc_camera/rcar_vin.c
> >>>>b/drivers/media/platform/soc_camera/rcar_vin.c
> >>>>index 20defcb..cb5e682 100644
> >>>>--- a/drivers/media/platform/soc_camera/rcar_vin.c
> >>>>+++ b/drivers/media/platform/soc_camera/rcar_vin.c
> >>>>@@ -74,6 +74,7 @@
> >>>>   #define VNMC_INF_YUV10_BT656  (2 << 16)
> >>>>   #define VNMC_INF_YUV10_BT601  (3 << 16)
> >>>>   #define VNMC_INF_YUV16                (5 << 16)
> >>>>+#define VNMC_INF_RGB888                (6 << 16)
> >>>>   #define VNMC_VUP              (1 << 10)
> >>>>   #define VNMC_IM_ODD           (0 << 3)
> >>>>   #define VNMC_IM_ODD_EVEN      (1 << 3)
> 
> >>>[...]
> 
> >>>>@@ -331,6 +336,9 @@ static int rcar_vin_setup(struct rcar_vin_priv *priv)
> >>>>         if (output_is_yuv)
> >>>>                 vnmc |= VNMC_BPS;
> >>>>
> >>>>+       if (vnmc & VNMC_INF_RGB888)
> >>>>+               vnmc ^= VNMC_BPS;
> >>>>+
> 
> >>>    Hm, this also changes the behavior for VNMC_INF_YUV16 and
> >>>VNMC_INF_YUV10_BT{601|656}. Is this actually intended?
> 
> >>Probably this code is incorrect.
> >>Thank you for your review.
> 
> >Thanks, I have confirmed with Matsuoka-san that there is a problem here.
> 
> >He has provided the following fix. Could you see about squashing it into
> >the above patch and reposting?
> 
> >From: Koji Matsuoka <koji.matsuoka.xm@renesas.com>
> 
> >[PATCH] media: soc_camera: rcar_vin: Fix bit field check
> 
> >Signed-off-by: Koji Matsuoka <koji.matsuoka.xm@renesas.com>
> 
> >diff --git a/drivers/media/platform/soc_camera/rcar_vin.c b/drivers/media/platform/soc_camera/rcar_vin.c
> >index 013d75c..da62d94 100644
> >--- a/drivers/media/platform/soc_camera/rcar_vin.c
> >+++ b/drivers/media/platform/soc_camera/rcar_vin.c
> >@@ -94,7 +94,7 @@
> >  #define VNMC_INF_YUV8_BT601	(1 << 16)
> >  #define VNMC_INF_YUV16		(5 << 16)
> >  #define VNMC_INF_RGB888		(6 << 16)
> >-#define VNMC_INF_RGB_MASK	(6 << 16)
> >+#define VNMC_INF_MASK		(7 << 16)
> 
>    #define it above VNMC_INF_YUV8_BT656 please.
> 
> >  #define VNMC_VUP		(1 << 10)
> >  #define VNMC_IM_ODD		(0 << 3)
> >  #define VNMC_IM_ODD_EVEN	(1 << 3)
> >@@ -675,7 +675,7 @@ static int rcar_vin_setup(struct rcar_vin_priv *priv)
> >  	if (output_is_yuv)
> >  		vnmc |= VNMC_BPS;
> >
> >-	if (vnmc & VNMC_INF_RGB_MASK)
> >+	if ((vnmc & VNMC_INF_MASK) = VNMC_INF_RGB888)
> 
>    Is he sure it shouldn't be (vnmc & VNMC_INF_RGB888) = VNMC_INF_RGB888 to
> also cover 16-bit RGB666 and 12-bit RGB88?

Nice, I think that is a good idea (although the latter formats aren't
supported by the driver yet, right?).

Its somewhat unobvious how that logic works so perhaps we should add a
comment like this

	/* If input and output use the same colorspace, use bypass mode */
	if (output_is_yuv)
		vnmc |= VNMC_BPS;

	/* The above assumes YUV input, toggle BPS for RGB input.
	 * RGB inputs can be detected by checking that the most-significant
	 * two bits of INF are set. This corresponds to the bits
	 * set in VNMC_INF_RGB888. */
	if ((vnmc & VNMC_INF_RGB888)) = VNMC_INF_RGB888)
		vnmc ^= VNMC_BPS;