linux-media.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 0/2] usbtv: Add SECAM support
@ 2018-02-24 18:24 Hugo Grostabussiat
  2018-02-24 18:24 ` [PATCH 1/2] usbtv: Use same decoder sequence as Windows driver Hugo Grostabussiat
                   ` (2 more replies)
  0 siblings, 3 replies; 17+ messages in thread
From: Hugo Grostabussiat @ 2018-02-24 18:24 UTC (permalink / raw)
  To: Mauro Carvalho Chehab, linux-media; +Cc: Hugo Grostabussiat

This patch series adds support for the SECAM standard to the USBTV
video grabber driver.

The first patch prepares for the inclusion of further decoder
configuration sequences by making them follow the same order and length
as the sequences which can be found inside the Windows driver's .INF
file.

The second patch adds the SECAM decoder configuration sequence found in
the .INF file, and exposes SECAM support to userspace.

Hugo Grostabussiat (2):
  usbtv: Use same decoder sequence as Windows driver
  usbtv: Add SECAM support

 drivers/media/usb/usbtv/usbtv-video.c | 60 +++++++++++++++++++++++++++++------
 drivers/media/usb/usbtv/usbtv.h       |  2 +-
 2 files changed, 51 insertions(+), 11 deletions(-)

-- 
2.16.2

^ permalink raw reply	[flat|nested] 17+ messages in thread

* [PATCH 1/2] usbtv: Use same decoder sequence as Windows driver
  2018-02-24 18:24 [PATCH 0/2] usbtv: Add SECAM support Hugo Grostabussiat
@ 2018-02-24 18:24 ` Hugo Grostabussiat
  2018-02-26 14:12   ` Hans Verkuil
  2018-02-24 18:24 ` [PATCH 2/2] usbtv: Add SECAM support Hugo Grostabussiat
  2018-04-08 21:11 ` [PATCH v2 0/6] usbtv: Add SECAM support and fix color encoding selection Hugo Grostabussiat
  2 siblings, 1 reply; 17+ messages in thread
From: Hugo Grostabussiat @ 2018-02-24 18:24 UTC (permalink / raw)
  To: Mauro Carvalho Chehab, linux-media; +Cc: Hugo Grostabussiat

Re-format the register {address, value} pairs so they follow the same
order as the decoder configuration sequences in the Windows driver's .INF
file.

For instance, for PAL, the "AVPAL" sequence in the .INF file is:
0x04,0x68,0xD3,0x72,0xA2,0xB0,0x15,0x01,0x2C,0x10,0x20,0x2e,0x08,0x02,
0x02,0x59,0x16,0x35,0x17,0x16,0x36

Signed-off-by: Hugo Grostabussiat <bonstra@bonstra.fr.eu.org>
---
 drivers/media/usb/usbtv/usbtv-video.c | 26 +++++++++++++++++---------
 1 file changed, 17 insertions(+), 9 deletions(-)

diff --git a/drivers/media/usb/usbtv/usbtv-video.c b/drivers/media/usb/usbtv/usbtv-video.c
index 3668a04359e8..52d06b30fabb 100644
--- a/drivers/media/usb/usbtv/usbtv-video.c
+++ b/drivers/media/usb/usbtv/usbtv-video.c
@@ -124,15 +124,26 @@ static int usbtv_select_input(struct usbtv *usbtv, int input)
 static int usbtv_select_norm(struct usbtv *usbtv, v4l2_std_id norm)
 {
 	int ret;
+	/* These are the series of register values used to configure the
+	 * decoder for a specific standard.
+	 * They are copied from the Settings\DecoderDefaults registry keys
+	 * present in the Windows driver .INF file for each norm.
+	 */
 	static const u16 pal[][2] = {
+		{ USBTV_BASE + 0x0003, 0x0004 },
 		{ USBTV_BASE + 0x001a, 0x0068 },
+		{ USBTV_BASE + 0x0100, 0x00d3 },
 		{ USBTV_BASE + 0x010e, 0x0072 },
 		{ USBTV_BASE + 0x010f, 0x00a2 },
 		{ USBTV_BASE + 0x0112, 0x00b0 },
+		{ USBTV_BASE + 0x0115, 0x0015 },
 		{ USBTV_BASE + 0x0117, 0x0001 },
 		{ USBTV_BASE + 0x0118, 0x002c },
 		{ USBTV_BASE + 0x012d, 0x0010 },
 		{ USBTV_BASE + 0x012f, 0x0020 },
+		{ USBTV_BASE + 0x0220, 0x002e },
+		{ USBTV_BASE + 0x0225, 0x0008 },
+		{ USBTV_BASE + 0x024e, 0x0002 },
 		{ USBTV_BASE + 0x024f, 0x0002 },
 		{ USBTV_BASE + 0x0254, 0x0059 },
 		{ USBTV_BASE + 0x025a, 0x0016 },
@@ -143,14 +154,20 @@ static int usbtv_select_norm(struct usbtv *usbtv, v4l2_std_id norm)
 	};
 
 	static const u16 ntsc[][2] = {
+		{ USBTV_BASE + 0x0003, 0x0004 },
 		{ USBTV_BASE + 0x001a, 0x0079 },
+		{ USBTV_BASE + 0x0100, 0x00d3 },
 		{ USBTV_BASE + 0x010e, 0x0068 },
 		{ USBTV_BASE + 0x010f, 0x009c },
 		{ USBTV_BASE + 0x0112, 0x00f0 },
+		{ USBTV_BASE + 0x0115, 0x0015 },
 		{ USBTV_BASE + 0x0117, 0x0000 },
 		{ USBTV_BASE + 0x0118, 0x00fc },
 		{ USBTV_BASE + 0x012d, 0x0004 },
 		{ USBTV_BASE + 0x012f, 0x0008 },
+		{ USBTV_BASE + 0x0220, 0x002e },
+		{ USBTV_BASE + 0x0225, 0x0008 },
+		{ USBTV_BASE + 0x024e, 0x0002 },
 		{ USBTV_BASE + 0x024f, 0x0001 },
 		{ USBTV_BASE + 0x0254, 0x005f },
 		{ USBTV_BASE + 0x025a, 0x0012 },
@@ -236,15 +253,6 @@ static int usbtv_setup_capture(struct usbtv *usbtv)
 		{ USBTV_BASE + 0x0158, 0x001f },
 		{ USBTV_BASE + 0x0159, 0x0006 },
 		{ USBTV_BASE + 0x015d, 0x0000 },
-
-		{ USBTV_BASE + 0x0003, 0x0004 },
-		{ USBTV_BASE + 0x0100, 0x00d3 },
-		{ USBTV_BASE + 0x0115, 0x0015 },
-		{ USBTV_BASE + 0x0220, 0x002e },
-		{ USBTV_BASE + 0x0225, 0x0008 },
-		{ USBTV_BASE + 0x024e, 0x0002 },
-		{ USBTV_BASE + 0x024e, 0x0002 },
-		{ USBTV_BASE + 0x024f, 0x0002 },
 	};
 
 	ret = usbtv_set_regs(usbtv, setup, ARRAY_SIZE(setup));
-- 
2.16.2

^ permalink raw reply related	[flat|nested] 17+ messages in thread

* [PATCH 2/2] usbtv: Add SECAM support
  2018-02-24 18:24 [PATCH 0/2] usbtv: Add SECAM support Hugo Grostabussiat
  2018-02-24 18:24 ` [PATCH 1/2] usbtv: Use same decoder sequence as Windows driver Hugo Grostabussiat
@ 2018-02-24 18:24 ` Hugo Grostabussiat
  2018-04-08 21:11 ` [PATCH v2 0/6] usbtv: Add SECAM support and fix color encoding selection Hugo Grostabussiat
  2 siblings, 0 replies; 17+ messages in thread
From: Hugo Grostabussiat @ 2018-02-24 18:24 UTC (permalink / raw)
  To: Mauro Carvalho Chehab, linux-media; +Cc: Hugo Grostabussiat

Add support for the SECAM norm, using the "AVSECAM" decoder configuration
sequence found in Windows driver's .INF file.

For reference, the "AVSECAM" sequence in the .INF file is:
0x04,0x73,0xDC,0x72,0xA2,0x90,0x35,0x01,0x30,0x04,0x08,0x2D,0x28,0x08,
0x02,0x69,0x16,0x35,0x21,0x16,0x36

Signed-off-by: Hugo Grostabussiat <bonstra@bonstra.fr.eu.org>
---
 drivers/media/usb/usbtv/usbtv-video.c | 34 +++++++++++++++++++++++++++++++++-
 drivers/media/usb/usbtv/usbtv.h       |  2 +-
 2 files changed, 34 insertions(+), 2 deletions(-)

diff --git a/drivers/media/usb/usbtv/usbtv-video.c b/drivers/media/usb/usbtv/usbtv-video.c
index 52d06b30fabb..7dd4f25203db 100644
--- a/drivers/media/usb/usbtv/usbtv-video.c
+++ b/drivers/media/usb/usbtv/usbtv-video.c
@@ -57,6 +57,11 @@ static struct usbtv_norm_params norm_params[] = {
 		.norm = V4L2_STD_PAL,
 		.cap_width = 720,
 		.cap_height = 576,
+	},
+	{
+		.norm = V4L2_STD_SECAM,
+		.cap_width = 720,
+		.cap_height = 576,
 	}
 };
 
@@ -177,6 +182,30 @@ static int usbtv_select_norm(struct usbtv *usbtv, v4l2_std_id norm)
 		{ USBTV_BASE + 0x0267, 0x0005 }
 	};
 
+	static const u16 secam[][2] = {
+		{ USBTV_BASE + 0x0003, 0x0004 },
+		{ USBTV_BASE + 0x001a, 0x0073 },
+		{ USBTV_BASE + 0x0100, 0x00dc },
+		{ USBTV_BASE + 0x010e, 0x0072 },
+		{ USBTV_BASE + 0x010f, 0x00a2 },
+		{ USBTV_BASE + 0x0112, 0x0090 },
+		{ USBTV_BASE + 0x0115, 0x0035 },
+		{ USBTV_BASE + 0x0117, 0x0001 },
+		{ USBTV_BASE + 0x0118, 0x0030 },
+		{ USBTV_BASE + 0x012d, 0x0004 },
+		{ USBTV_BASE + 0x012f, 0x0008 },
+		{ USBTV_BASE + 0x0220, 0x002d },
+		{ USBTV_BASE + 0x0225, 0x0028 },
+		{ USBTV_BASE + 0x024e, 0x0008 },
+		{ USBTV_BASE + 0x024f, 0x0002 },
+		{ USBTV_BASE + 0x0254, 0x0069 },
+		{ USBTV_BASE + 0x025a, 0x0016 },
+		{ USBTV_BASE + 0x025b, 0x0035 },
+		{ USBTV_BASE + 0x0263, 0x0021 },
+		{ USBTV_BASE + 0x0266, 0x0016 },
+		{ USBTV_BASE + 0x0267, 0x0036 }
+	};
+
 	ret = usbtv_configure_for_norm(usbtv, norm);
 
 	if (!ret) {
@@ -184,6 +213,8 @@ static int usbtv_select_norm(struct usbtv *usbtv, v4l2_std_id norm)
 			ret = usbtv_set_regs(usbtv, ntsc, ARRAY_SIZE(ntsc));
 		else if (norm & V4L2_STD_PAL)
 			ret = usbtv_set_regs(usbtv, pal, ARRAY_SIZE(pal));
+		else if (norm & V4L2_STD_SECAM)
+			ret = usbtv_set_regs(usbtv, secam, ARRAY_SIZE(secam));
 	}
 
 	return ret;
@@ -595,7 +626,8 @@ static int usbtv_s_std(struct file *file, void *priv, v4l2_std_id norm)
 	int ret = -EINVAL;
 	struct usbtv *usbtv = video_drvdata(file);
 
-	if ((norm & V4L2_STD_525_60) || (norm & V4L2_STD_PAL))
+	if ((norm & V4L2_STD_525_60) || (norm & V4L2_STD_PAL) ||
+			(norm & V4L2_STD_SECAM))
 		ret = usbtv_select_norm(usbtv, norm);
 
 	return ret;
diff --git a/drivers/media/usb/usbtv/usbtv.h b/drivers/media/usb/usbtv/usbtv.h
index 0231e449877e..77a368e90fd0 100644
--- a/drivers/media/usb/usbtv/usbtv.h
+++ b/drivers/media/usb/usbtv/usbtv.h
@@ -68,7 +68,7 @@
 #define USBTV_ODD(chunk)	((be32_to_cpu(chunk[0]) & 0x0000f000) >> 15)
 #define USBTV_CHUNK_NO(chunk)	(be32_to_cpu(chunk[0]) & 0x00000fff)
 
-#define USBTV_TV_STD  (V4L2_STD_525_60 | V4L2_STD_PAL)
+#define USBTV_TV_STD  (V4L2_STD_525_60 | V4L2_STD_PAL | V4L2_STD_SECAM)
 
 /* parameters for supported TV norms */
 struct usbtv_norm_params {
-- 
2.16.2

^ permalink raw reply related	[flat|nested] 17+ messages in thread

* Re: [PATCH 1/2] usbtv: Use same decoder sequence as Windows driver
  2018-02-24 18:24 ` [PATCH 1/2] usbtv: Use same decoder sequence as Windows driver Hugo Grostabussiat
@ 2018-02-26 14:12   ` Hans Verkuil
  2018-02-27 22:35     ` Hugo "Bonstra" Grostabussiat
  0 siblings, 1 reply; 17+ messages in thread
From: Hans Verkuil @ 2018-02-26 14:12 UTC (permalink / raw)
  To: Hugo Grostabussiat, Mauro Carvalho Chehab, linux-media

Hi Hugo,

Thanks for this patch, but I am a bit hesitant to apply it. Did you test
that PAL and NTSC still work after this change?

Unless you've tested it then I'm inclined to just apply the second patch that
adds the SECAM sequence.

Regards,

	Hans

On 02/24/2018 07:24 PM, Hugo Grostabussiat wrote:
> Re-format the register {address, value} pairs so they follow the same
> order as the decoder configuration sequences in the Windows driver's .INF
> file.
> 
> For instance, for PAL, the "AVPAL" sequence in the .INF file is:
> 0x04,0x68,0xD3,0x72,0xA2,0xB0,0x15,0x01,0x2C,0x10,0x20,0x2e,0x08,0x02,
> 0x02,0x59,0x16,0x35,0x17,0x16,0x36
> 
> Signed-off-by: Hugo Grostabussiat <bonstra@bonstra.fr.eu.org>
> ---
>  drivers/media/usb/usbtv/usbtv-video.c | 26 +++++++++++++++++---------
>  1 file changed, 17 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/media/usb/usbtv/usbtv-video.c b/drivers/media/usb/usbtv/usbtv-video.c
> index 3668a04359e8..52d06b30fabb 100644
> --- a/drivers/media/usb/usbtv/usbtv-video.c
> +++ b/drivers/media/usb/usbtv/usbtv-video.c
> @@ -124,15 +124,26 @@ static int usbtv_select_input(struct usbtv *usbtv, int input)
>  static int usbtv_select_norm(struct usbtv *usbtv, v4l2_std_id norm)
>  {
>  	int ret;
> +	/* These are the series of register values used to configure the
> +	 * decoder for a specific standard.
> +	 * They are copied from the Settings\DecoderDefaults registry keys
> +	 * present in the Windows driver .INF file for each norm.
> +	 */
>  	static const u16 pal[][2] = {
> +		{ USBTV_BASE + 0x0003, 0x0004 },
>  		{ USBTV_BASE + 0x001a, 0x0068 },
> +		{ USBTV_BASE + 0x0100, 0x00d3 },
>  		{ USBTV_BASE + 0x010e, 0x0072 },
>  		{ USBTV_BASE + 0x010f, 0x00a2 },
>  		{ USBTV_BASE + 0x0112, 0x00b0 },
> +		{ USBTV_BASE + 0x0115, 0x0015 },
>  		{ USBTV_BASE + 0x0117, 0x0001 },
>  		{ USBTV_BASE + 0x0118, 0x002c },
>  		{ USBTV_BASE + 0x012d, 0x0010 },
>  		{ USBTV_BASE + 0x012f, 0x0020 },
> +		{ USBTV_BASE + 0x0220, 0x002e },
> +		{ USBTV_BASE + 0x0225, 0x0008 },
> +		{ USBTV_BASE + 0x024e, 0x0002 },
>  		{ USBTV_BASE + 0x024f, 0x0002 },
>  		{ USBTV_BASE + 0x0254, 0x0059 },
>  		{ USBTV_BASE + 0x025a, 0x0016 },
> @@ -143,14 +154,20 @@ static int usbtv_select_norm(struct usbtv *usbtv, v4l2_std_id norm)
>  	};
>  
>  	static const u16 ntsc[][2] = {
> +		{ USBTV_BASE + 0x0003, 0x0004 },
>  		{ USBTV_BASE + 0x001a, 0x0079 },
> +		{ USBTV_BASE + 0x0100, 0x00d3 },
>  		{ USBTV_BASE + 0x010e, 0x0068 },
>  		{ USBTV_BASE + 0x010f, 0x009c },
>  		{ USBTV_BASE + 0x0112, 0x00f0 },
> +		{ USBTV_BASE + 0x0115, 0x0015 },
>  		{ USBTV_BASE + 0x0117, 0x0000 },
>  		{ USBTV_BASE + 0x0118, 0x00fc },
>  		{ USBTV_BASE + 0x012d, 0x0004 },
>  		{ USBTV_BASE + 0x012f, 0x0008 },
> +		{ USBTV_BASE + 0x0220, 0x002e },
> +		{ USBTV_BASE + 0x0225, 0x0008 },
> +		{ USBTV_BASE + 0x024e, 0x0002 },
>  		{ USBTV_BASE + 0x024f, 0x0001 },
>  		{ USBTV_BASE + 0x0254, 0x005f },
>  		{ USBTV_BASE + 0x025a, 0x0012 },
> @@ -236,15 +253,6 @@ static int usbtv_setup_capture(struct usbtv *usbtv)
>  		{ USBTV_BASE + 0x0158, 0x001f },
>  		{ USBTV_BASE + 0x0159, 0x0006 },
>  		{ USBTV_BASE + 0x015d, 0x0000 },
> -
> -		{ USBTV_BASE + 0x0003, 0x0004 },
> -		{ USBTV_BASE + 0x0100, 0x00d3 },
> -		{ USBTV_BASE + 0x0115, 0x0015 },
> -		{ USBTV_BASE + 0x0220, 0x002e },
> -		{ USBTV_BASE + 0x0225, 0x0008 },
> -		{ USBTV_BASE + 0x024e, 0x0002 },
> -		{ USBTV_BASE + 0x024e, 0x0002 },
> -		{ USBTV_BASE + 0x024f, 0x0002 },
>  	};
>  
>  	ret = usbtv_set_regs(usbtv, setup, ARRAY_SIZE(setup));
> 

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH 1/2] usbtv: Use same decoder sequence as Windows driver
  2018-02-26 14:12   ` Hans Verkuil
@ 2018-02-27 22:35     ` Hugo "Bonstra" Grostabussiat
  2018-03-09 10:55       ` Hans Verkuil
  0 siblings, 1 reply; 17+ messages in thread
From: Hugo "Bonstra" Grostabussiat @ 2018-02-27 22:35 UTC (permalink / raw)
  To: Hans Verkuil; +Cc: Mauro Carvalho Chehab, linux-media

Le 26/02/2018 à 15:12, Hans Verkuil a écrit :
> Thanks for this patch, but I am a bit hesitant to apply it. Did you test
> that PAL and NTSC still work after this change?

I did test with both a PAL and a NTSC source before submitting the
patch. It seemed to work fine.

However, after reading your reply, I needed to be certain, so I
double-checked using the Windows driver USB dumps I collected.

When it sets the TV standard, the Windows driver:

  1. plays the decoder configuration sequence, in the exact same order
     as the one specified in the .INF file.
     -> My patch 1 does that, the unpatched driver doesn't

  2. sets registers 0xc24e and 0xc24f to value 0x02.
     -> The unpatched driver does this, my patch 1 doesn't, which is
        a regression

  3. sets register 0xc16f to a different value for PAL, NTSC and
     SECAM.
     -> No write is ever done to that register by the patched
        or the unpatched drivers

About step 3, I added it to the usbtv driver, and quickly understood
what register 0xc16f is for. It actually sets the standard for color
encoding!
It would seem that by default, the decoder auto-detects the standard
used to encode the color signal, and that writing a value to that
register forces selection of a specific standard.

So, with the current unpatched driver, capturing a PAL source while
setting the V4L2 norm to NTSC will still get the colors right (mostly),
but the image will be clipped at the bottom because of the lower
vertical resolution used for NTSC.

With the Windows driver or the usbtv driver patched to set the
norm in register 0xc16f, capturing a PAL source with the adapter
configured for NTSC would give the result you would expect from
misconfigured hardware: incorrect resolution and messed-up colors.

Here are some screenshots to illustrate the matter:

* Unpatched driver, PAL source, adapter configured for PAL.
  Picture is fully displayed, and colors are ok. Used as reference:

https://www.bonstra.fr.eu.org/pub/img/usbtv_unpatched-PAL_source-PAL_setting.png

* Unpatched driver, PAL source, adapter configured for NTSC.
  A part of the picture is cut at the bottom, colors are ok:

https://www.bonstra.fr.eu.org/pub/img/usbtv_unpatched-PAL_source-NTSC_setting.png

* Patched driver, PAL source, adapter configured for NTSC:
  Picture bottom is clipped, vertical stripes with the wrong colors
  are present over the colored areas:

https://www.bonstra.fr.eu.org/pub/img/usbtv_patched-PAL_source-NTSC_setting.png

Now about part 1; the sequence of register writes before actually
setting the color system register is there to set the image correction
parameters, such as color correction or image sharpness, appropriate
for the selected standard.

> Unless you've tested it then I'm inclined to just apply the second patch that
> adds the SECAM sequence.

Applying only patch 2 would get some values of the image correction
registers overwritten with PAL/NTSC values which were put in common
since they were identical (registers 0xc100, 0xc115, 0xc220, 0xc225 and
0xc24e).

I think I should make a v2 of this patch series which:
  1. fixes the mistakes I made in patch 1
  2. add SECAM image correction settings sequence
  3. writes the standard setting to register 0xc16f so that we get as
     closes as possible to the Windows driver behavior
  4. handles the PAL_60 case (NTSC resolution with PAL-like color
     subcarrier) which was working "by accident" until now

What do you think of all this?

Regards
--
Hugo

> On 02/24/2018 07:24 PM, Hugo Grostabussiat wrote:
>> Re-format the register {address, value} pairs so they follow the same
>> order as the decoder configuration sequences in the Windows driver's .INF
>> file.
>>
>> For instance, for PAL, the "AVPAL" sequence in the .INF file is:
>> 0x04,0x68,0xD3,0x72,0xA2,0xB0,0x15,0x01,0x2C,0x10,0x20,0x2e,0x08,0x02,
>> 0x02,0x59,0x16,0x35,0x17,0x16,0x36
>>
>> Signed-off-by: Hugo Grostabussiat <bonstra@bonstra.fr.eu.org>
>> ---
>>  drivers/media/usb/usbtv/usbtv-video.c | 26 +++++++++++++++++---------
>>  1 file changed, 17 insertions(+), 9 deletions(-)
>>
>> diff --git a/drivers/media/usb/usbtv/usbtv-video.c b/drivers/media/usb/usbtv/usbtv-video.c
>> index 3668a04359e8..52d06b30fabb 100644
>> --- a/drivers/media/usb/usbtv/usbtv-video.c
>> +++ b/drivers/media/usb/usbtv/usbtv-video.c
>> @@ -124,15 +124,26 @@ static int usbtv_select_input(struct usbtv *usbtv, int input)
>>  static int usbtv_select_norm(struct usbtv *usbtv, v4l2_std_id norm)
>>  {
>>  	int ret;
>> +	/* These are the series of register values used to configure the
>> +	 * decoder for a specific standard.
>> +	 * They are copied from the Settings\DecoderDefaults registry keys
>> +	 * present in the Windows driver .INF file for each norm.
>> +	 */
>>  	static const u16 pal[][2] = {
>> +		{ USBTV_BASE + 0x0003, 0x0004 },
>>  		{ USBTV_BASE + 0x001a, 0x0068 },
>> +		{ USBTV_BASE + 0x0100, 0x00d3 },
>>  		{ USBTV_BASE + 0x010e, 0x0072 },
>>  		{ USBTV_BASE + 0x010f, 0x00a2 },
>>  		{ USBTV_BASE + 0x0112, 0x00b0 },
>> +		{ USBTV_BASE + 0x0115, 0x0015 },
>>  		{ USBTV_BASE + 0x0117, 0x0001 },
>>  		{ USBTV_BASE + 0x0118, 0x002c },
>>  		{ USBTV_BASE + 0x012d, 0x0010 },
>>  		{ USBTV_BASE + 0x012f, 0x0020 },
>> +		{ USBTV_BASE + 0x0220, 0x002e },
>> +		{ USBTV_BASE + 0x0225, 0x0008 },
>> +		{ USBTV_BASE + 0x024e, 0x0002 },
>>  		{ USBTV_BASE + 0x024f, 0x0002 },
>>  		{ USBTV_BASE + 0x0254, 0x0059 },
>>  		{ USBTV_BASE + 0x025a, 0x0016 },
>> @@ -143,14 +154,20 @@ static int usbtv_select_norm(struct usbtv *usbtv, v4l2_std_id norm)
>>  	};
>>  
>>  	static const u16 ntsc[][2] = {
>> +		{ USBTV_BASE + 0x0003, 0x0004 },
>>  		{ USBTV_BASE + 0x001a, 0x0079 },
>> +		{ USBTV_BASE + 0x0100, 0x00d3 },
>>  		{ USBTV_BASE + 0x010e, 0x0068 },
>>  		{ USBTV_BASE + 0x010f, 0x009c },
>>  		{ USBTV_BASE + 0x0112, 0x00f0 },
>> +		{ USBTV_BASE + 0x0115, 0x0015 },
>>  		{ USBTV_BASE + 0x0117, 0x0000 },
>>  		{ USBTV_BASE + 0x0118, 0x00fc },
>>  		{ USBTV_BASE + 0x012d, 0x0004 },
>>  		{ USBTV_BASE + 0x012f, 0x0008 },
>> +		{ USBTV_BASE + 0x0220, 0x002e },
>> +		{ USBTV_BASE + 0x0225, 0x0008 },
>> +		{ USBTV_BASE + 0x024e, 0x0002 },
>>  		{ USBTV_BASE + 0x024f, 0x0001 },
>>  		{ USBTV_BASE + 0x0254, 0x005f },
>>  		{ USBTV_BASE + 0x025a, 0x0012 },
>> @@ -236,15 +253,6 @@ static int usbtv_setup_capture(struct usbtv *usbtv)
>>  		{ USBTV_BASE + 0x0158, 0x001f },
>>  		{ USBTV_BASE + 0x0159, 0x0006 },
>>  		{ USBTV_BASE + 0x015d, 0x0000 },
>> -
>> -		{ USBTV_BASE + 0x0003, 0x0004 },
>> -		{ USBTV_BASE + 0x0100, 0x00d3 },
>> -		{ USBTV_BASE + 0x0115, 0x0015 },
>> -		{ USBTV_BASE + 0x0220, 0x002e },
>> -		{ USBTV_BASE + 0x0225, 0x0008 },
>> -		{ USBTV_BASE + 0x024e, 0x0002 },
>> -		{ USBTV_BASE + 0x024e, 0x0002 },
>> -		{ USBTV_BASE + 0x024f, 0x0002 },
>>  	};
>>  
>>  	ret = usbtv_set_regs(usbtv, setup, ARRAY_SIZE(setup));
>>
> 

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH 1/2] usbtv: Use same decoder sequence as Windows driver
  2018-02-27 22:35     ` Hugo "Bonstra" Grostabussiat
@ 2018-03-09 10:55       ` Hans Verkuil
  0 siblings, 0 replies; 17+ messages in thread
From: Hans Verkuil @ 2018-03-09 10:55 UTC (permalink / raw)
  To: Hugo "Bonstra" Grostabussiat; +Cc: Mauro Carvalho Chehab, linux-media

On 27/02/18 23:35, Hugo "Bonstra" Grostabussiat wrote:
> Le 26/02/2018 à 15:12, Hans Verkuil a écrit :
>> Thanks for this patch, but I am a bit hesitant to apply it. Did you test
>> that PAL and NTSC still work after this change?
> 
> I did test with both a PAL and a NTSC source before submitting the
> patch. It seemed to work fine.

That's great!

> However, after reading your reply, I needed to be certain, so I
> double-checked using the Windows driver USB dumps I collected.
> 
> When it sets the TV standard, the Windows driver:
> 
>   1. plays the decoder configuration sequence, in the exact same order
>      as the one specified in the .INF file.
>      -> My patch 1 does that, the unpatched driver doesn't
> 
>   2. sets registers 0xc24e and 0xc24f to value 0x02.
>      -> The unpatched driver does this, my patch 1 doesn't, which is
>         a regression
> 
>   3. sets register 0xc16f to a different value for PAL, NTSC and
>      SECAM.
>      -> No write is ever done to that register by the patched
>         or the unpatched drivers
> 
> About step 3, I added it to the usbtv driver, and quickly understood
> what register 0xc16f is for. It actually sets the standard for color
> encoding!
> It would seem that by default, the decoder auto-detects the standard
> used to encode the color signal, and that writing a value to that
> register forces selection of a specific standard.
> 
> So, with the current unpatched driver, capturing a PAL source while
> setting the V4L2 norm to NTSC will still get the colors right (mostly),
> but the image will be clipped at the bottom because of the lower
> vertical resolution used for NTSC.
> 
> With the Windows driver or the usbtv driver patched to set the
> norm in register 0xc16f, capturing a PAL source with the adapter
> configured for NTSC would give the result you would expect from
> misconfigured hardware: incorrect resolution and messed-up colors.
> 
> Here are some screenshots to illustrate the matter:
> 
> * Unpatched driver, PAL source, adapter configured for PAL.
>   Picture is fully displayed, and colors are ok. Used as reference:
> 
> https://www.bonstra.fr.eu.org/pub/img/usbtv_unpatched-PAL_source-PAL_setting.png
> 
> * Unpatched driver, PAL source, adapter configured for NTSC.
>   A part of the picture is cut at the bottom, colors are ok:
> 
> https://www.bonstra.fr.eu.org/pub/img/usbtv_unpatched-PAL_source-NTSC_setting.png
> 
> * Patched driver, PAL source, adapter configured for NTSC:
>   Picture bottom is clipped, vertical stripes with the wrong colors
>   are present over the colored areas:
> 
> https://www.bonstra.fr.eu.org/pub/img/usbtv_patched-PAL_source-NTSC_setting.png
> 
> Now about part 1; the sequence of register writes before actually
> setting the color system register is there to set the image correction
> parameters, such as color correction or image sharpness, appropriate
> for the selected standard.
> 
>> Unless you've tested it then I'm inclined to just apply the second patch that
>> adds the SECAM sequence.
> 
> Applying only patch 2 would get some values of the image correction
> registers overwritten with PAL/NTSC values which were put in common
> since they were identical (registers 0xc100, 0xc115, 0xc220, 0xc225 and
> 0xc24e).
> 
> I think I should make a v2 of this patch series which:
>   1. fixes the mistakes I made in patch 1
>   2. add SECAM image correction settings sequence
>   3. writes the standard setting to register 0xc16f so that we get as
>      closes as possible to the Windows driver behavior
>   4. handles the PAL_60 case (NTSC resolution with PAL-like color
>      subcarrier) which was working "by accident" until now
> 
> What do you think of all this?

Sounds good. Since you can test with PAL and NTSC sources (besides SECAM) you
can verify that everything works. So I'm happy to apply a v2.

Regards,

	Hans

> 
> Regards
> --
> Hugo
> 
>> On 02/24/2018 07:24 PM, Hugo Grostabussiat wrote:
>>> Re-format the register {address, value} pairs so they follow the same
>>> order as the decoder configuration sequences in the Windows driver's .INF
>>> file.
>>>
>>> For instance, for PAL, the "AVPAL" sequence in the .INF file is:
>>> 0x04,0x68,0xD3,0x72,0xA2,0xB0,0x15,0x01,0x2C,0x10,0x20,0x2e,0x08,0x02,
>>> 0x02,0x59,0x16,0x35,0x17,0x16,0x36
>>>
>>> Signed-off-by: Hugo Grostabussiat <bonstra@bonstra.fr.eu.org>
>>> ---
>>>  drivers/media/usb/usbtv/usbtv-video.c | 26 +++++++++++++++++---------
>>>  1 file changed, 17 insertions(+), 9 deletions(-)
>>>
>>> diff --git a/drivers/media/usb/usbtv/usbtv-video.c b/drivers/media/usb/usbtv/usbtv-video.c
>>> index 3668a04359e8..52d06b30fabb 100644
>>> --- a/drivers/media/usb/usbtv/usbtv-video.c
>>> +++ b/drivers/media/usb/usbtv/usbtv-video.c
>>> @@ -124,15 +124,26 @@ static int usbtv_select_input(struct usbtv *usbtv, int input)
>>>  static int usbtv_select_norm(struct usbtv *usbtv, v4l2_std_id norm)
>>>  {
>>>  	int ret;
>>> +	/* These are the series of register values used to configure the
>>> +	 * decoder for a specific standard.
>>> +	 * They are copied from the Settings\DecoderDefaults registry keys
>>> +	 * present in the Windows driver .INF file for each norm.
>>> +	 */
>>>  	static const u16 pal[][2] = {
>>> +		{ USBTV_BASE + 0x0003, 0x0004 },
>>>  		{ USBTV_BASE + 0x001a, 0x0068 },
>>> +		{ USBTV_BASE + 0x0100, 0x00d3 },
>>>  		{ USBTV_BASE + 0x010e, 0x0072 },
>>>  		{ USBTV_BASE + 0x010f, 0x00a2 },
>>>  		{ USBTV_BASE + 0x0112, 0x00b0 },
>>> +		{ USBTV_BASE + 0x0115, 0x0015 },
>>>  		{ USBTV_BASE + 0x0117, 0x0001 },
>>>  		{ USBTV_BASE + 0x0118, 0x002c },
>>>  		{ USBTV_BASE + 0x012d, 0x0010 },
>>>  		{ USBTV_BASE + 0x012f, 0x0020 },
>>> +		{ USBTV_BASE + 0x0220, 0x002e },
>>> +		{ USBTV_BASE + 0x0225, 0x0008 },
>>> +		{ USBTV_BASE + 0x024e, 0x0002 },
>>>  		{ USBTV_BASE + 0x024f, 0x0002 },
>>>  		{ USBTV_BASE + 0x0254, 0x0059 },
>>>  		{ USBTV_BASE + 0x025a, 0x0016 },
>>> @@ -143,14 +154,20 @@ static int usbtv_select_norm(struct usbtv *usbtv, v4l2_std_id norm)
>>>  	};
>>>  
>>>  	static const u16 ntsc[][2] = {
>>> +		{ USBTV_BASE + 0x0003, 0x0004 },
>>>  		{ USBTV_BASE + 0x001a, 0x0079 },
>>> +		{ USBTV_BASE + 0x0100, 0x00d3 },
>>>  		{ USBTV_BASE + 0x010e, 0x0068 },
>>>  		{ USBTV_BASE + 0x010f, 0x009c },
>>>  		{ USBTV_BASE + 0x0112, 0x00f0 },
>>> +		{ USBTV_BASE + 0x0115, 0x0015 },
>>>  		{ USBTV_BASE + 0x0117, 0x0000 },
>>>  		{ USBTV_BASE + 0x0118, 0x00fc },
>>>  		{ USBTV_BASE + 0x012d, 0x0004 },
>>>  		{ USBTV_BASE + 0x012f, 0x0008 },
>>> +		{ USBTV_BASE + 0x0220, 0x002e },
>>> +		{ USBTV_BASE + 0x0225, 0x0008 },
>>> +		{ USBTV_BASE + 0x024e, 0x0002 },
>>>  		{ USBTV_BASE + 0x024f, 0x0001 },
>>>  		{ USBTV_BASE + 0x0254, 0x005f },
>>>  		{ USBTV_BASE + 0x025a, 0x0012 },
>>> @@ -236,15 +253,6 @@ static int usbtv_setup_capture(struct usbtv *usbtv)
>>>  		{ USBTV_BASE + 0x0158, 0x001f },
>>>  		{ USBTV_BASE + 0x0159, 0x0006 },
>>>  		{ USBTV_BASE + 0x015d, 0x0000 },
>>> -
>>> -		{ USBTV_BASE + 0x0003, 0x0004 },
>>> -		{ USBTV_BASE + 0x0100, 0x00d3 },
>>> -		{ USBTV_BASE + 0x0115, 0x0015 },
>>> -		{ USBTV_BASE + 0x0220, 0x002e },
>>> -		{ USBTV_BASE + 0x0225, 0x0008 },
>>> -		{ USBTV_BASE + 0x024e, 0x0002 },
>>> -		{ USBTV_BASE + 0x024e, 0x0002 },
>>> -		{ USBTV_BASE + 0x024f, 0x0002 },
>>>  	};
>>>  
>>>  	ret = usbtv_set_regs(usbtv, setup, ARRAY_SIZE(setup));
>>>
>>
> 

^ permalink raw reply	[flat|nested] 17+ messages in thread

* [PATCH v2 0/6] usbtv: Add SECAM support and fix color encoding selection
  2018-02-24 18:24 [PATCH 0/2] usbtv: Add SECAM support Hugo Grostabussiat
  2018-02-24 18:24 ` [PATCH 1/2] usbtv: Use same decoder sequence as Windows driver Hugo Grostabussiat
  2018-02-24 18:24 ` [PATCH 2/2] usbtv: Add SECAM support Hugo Grostabussiat
@ 2018-04-08 21:11 ` Hugo Grostabussiat
  2018-04-08 21:11   ` [PATCH v2 1/6] usbtv: Use same decoder sequence as Windows driver Hugo Grostabussiat
                     ` (5 more replies)
  2 siblings, 6 replies; 17+ messages in thread
From: Hugo Grostabussiat @ 2018-04-08 21:11 UTC (permalink / raw)
  To: Hans Verkuil; +Cc: Mauro Carvalho Chehab, linux-media, Hugo Grostabussiat

This series adds SECAM support to the usbtv driver, while
also attempting to mimic the Windows driver's behavior
regarding color encoding selection.

I made USB captures of the device as it is set up by the Windows driver,
for all the supported video standards. The analog source used is the
composite output of a video card, which is configured to output a signal
using the same standard as the one configured for the capture, when that
is possible. This enabled me to find new values for the 0x016f register
that I had missed in the v1, as well as test my patches.
Unfortunately, I did not have a SECAM source for the tests, so they were
limited to setting the standard to SECAM for a PAL source, and making
sure the output was the same between the Windows and the Linux driver.

The capture adapter used for the tests is a QSonic VG-310, with USB ID
1b71:3002. The Windows driver used as reference is EasyCap driver
version 2.1.1.2 (2011-06-08).

The changes since the v1 are:
- the output resolution is selected independently of the color encoding.
  For instance, PAL-M, while using a PAL-like color encoding, has the
  same resolution as NTSC-M, so the NTSC resolution will be used in that
  case
- conversely, the color encoding is configured independently of the
  resolution
- PAL and NTSC variants have a different value for register 0x016f
- the norm value set by the user is no longer overwritten by the driver
  with a generic value when selecting the output resolution, so specific
  standards (e.g. NTSC-443) can actually be selected
- minor cosmetic changes

Hugo Grostabussiat (6):
  usbtv: Use same decoder sequence as Windows driver
  usbtv: Add SECAM support
  usbtv: Use V4L2 defines to select capture resolution
  usbtv: Keep norm parameter specific
  usbtv: Enforce standard for color decoding
  usbtv: Use the constant for supported standards

 drivers/media/usb/usbtv/usbtv-video.c | 115 ++++++++++++++++++++++----
 drivers/media/usb/usbtv/usbtv.h       |   2 +-
 2 files changed, 100 insertions(+), 17 deletions(-)

-- 
2.17.0

^ permalink raw reply	[flat|nested] 17+ messages in thread

* [PATCH v2 1/6] usbtv: Use same decoder sequence as Windows driver
  2018-04-08 21:11 ` [PATCH v2 0/6] usbtv: Add SECAM support and fix color encoding selection Hugo Grostabussiat
@ 2018-04-08 21:11   ` Hugo Grostabussiat
  2018-04-08 21:11   ` [PATCH v2 2/6] usbtv: Add SECAM support Hugo Grostabussiat
                     ` (4 subsequent siblings)
  5 siblings, 0 replies; 17+ messages in thread
From: Hugo Grostabussiat @ 2018-04-08 21:11 UTC (permalink / raw)
  To: Hans Verkuil; +Cc: Mauro Carvalho Chehab, linux-media, Hugo Grostabussiat

Re-format the register {address, value} pairs so they follow the same
order as the decoder configuration sequences in the Windows driver's .INF
file.

For instance, for PAL, the "AVPAL" sequence in the .INF file is:
0x04,0x68,0xD3,0x72,0xA2,0xB0,0x15,0x01,0x2C,0x10,0x20,0x2e,0x08,0x02,
0x02,0x59,0x16,0x35,0x17,0x16,0x36

Signed-off-by: Hugo Grostabussiat <bonstra@bonstra.fr.eu.org>
---
 drivers/media/usb/usbtv/usbtv-video.c | 40 +++++++++++++++++++--------
 1 file changed, 29 insertions(+), 11 deletions(-)

diff --git a/drivers/media/usb/usbtv/usbtv-video.c b/drivers/media/usb/usbtv/usbtv-video.c
index 3668a04359e8..97f9790954f9 100644
--- a/drivers/media/usb/usbtv/usbtv-video.c
+++ b/drivers/media/usb/usbtv/usbtv-video.c
@@ -124,40 +124,67 @@ static int usbtv_select_input(struct usbtv *usbtv, int input)
 static int usbtv_select_norm(struct usbtv *usbtv, v4l2_std_id norm)
 {
 	int ret;
+	/* These are the series of register values used to configure the
+	 * decoder for a specific standard.
+	 * The first 21 register writes are copied from the
+	 * Settings\DecoderDefaults registry keys present in the Windows driver
+	 * .INF file, and control various image tuning parameters (color
+	 * correction, sharpness, ...).
+	 */
 	static const u16 pal[][2] = {
+		/* "AVPAL" tuning sequence from .INF file */
+		{ USBTV_BASE + 0x0003, 0x0004 },
 		{ USBTV_BASE + 0x001a, 0x0068 },
+		{ USBTV_BASE + 0x0100, 0x00d3 },
 		{ USBTV_BASE + 0x010e, 0x0072 },
 		{ USBTV_BASE + 0x010f, 0x00a2 },
 		{ USBTV_BASE + 0x0112, 0x00b0 },
+		{ USBTV_BASE + 0x0115, 0x0015 },
 		{ USBTV_BASE + 0x0117, 0x0001 },
 		{ USBTV_BASE + 0x0118, 0x002c },
 		{ USBTV_BASE + 0x012d, 0x0010 },
 		{ USBTV_BASE + 0x012f, 0x0020 },
+		{ USBTV_BASE + 0x0220, 0x002e },
+		{ USBTV_BASE + 0x0225, 0x0008 },
+		{ USBTV_BASE + 0x024e, 0x0002 },
 		{ USBTV_BASE + 0x024f, 0x0002 },
 		{ USBTV_BASE + 0x0254, 0x0059 },
 		{ USBTV_BASE + 0x025a, 0x0016 },
 		{ USBTV_BASE + 0x025b, 0x0035 },
 		{ USBTV_BASE + 0x0263, 0x0017 },
 		{ USBTV_BASE + 0x0266, 0x0016 },
-		{ USBTV_BASE + 0x0267, 0x0036 }
+		{ USBTV_BASE + 0x0267, 0x0036 },
+		/* Epilog */
+		{ USBTV_BASE + 0x024e, 0x0002 },
+		{ USBTV_BASE + 0x024f, 0x0002 },
 	};
 
 	static const u16 ntsc[][2] = {
+		/* "AVNTSC" tuning sequence from .INF file */
+		{ USBTV_BASE + 0x0003, 0x0004 },
 		{ USBTV_BASE + 0x001a, 0x0079 },
+		{ USBTV_BASE + 0x0100, 0x00d3 },
 		{ USBTV_BASE + 0x010e, 0x0068 },
 		{ USBTV_BASE + 0x010f, 0x009c },
 		{ USBTV_BASE + 0x0112, 0x00f0 },
+		{ USBTV_BASE + 0x0115, 0x0015 },
 		{ USBTV_BASE + 0x0117, 0x0000 },
 		{ USBTV_BASE + 0x0118, 0x00fc },
 		{ USBTV_BASE + 0x012d, 0x0004 },
 		{ USBTV_BASE + 0x012f, 0x0008 },
+		{ USBTV_BASE + 0x0220, 0x002e },
+		{ USBTV_BASE + 0x0225, 0x0008 },
+		{ USBTV_BASE + 0x024e, 0x0002 },
 		{ USBTV_BASE + 0x024f, 0x0001 },
 		{ USBTV_BASE + 0x0254, 0x005f },
 		{ USBTV_BASE + 0x025a, 0x0012 },
 		{ USBTV_BASE + 0x025b, 0x0001 },
 		{ USBTV_BASE + 0x0263, 0x001c },
 		{ USBTV_BASE + 0x0266, 0x0011 },
-		{ USBTV_BASE + 0x0267, 0x0005 }
+		{ USBTV_BASE + 0x0267, 0x0005 },
+		/* Epilog */
+		{ USBTV_BASE + 0x024e, 0x0002 },
+		{ USBTV_BASE + 0x024f, 0x0002 },
 	};
 
 	ret = usbtv_configure_for_norm(usbtv, norm);
@@ -236,15 +263,6 @@ static int usbtv_setup_capture(struct usbtv *usbtv)
 		{ USBTV_BASE + 0x0158, 0x001f },
 		{ USBTV_BASE + 0x0159, 0x0006 },
 		{ USBTV_BASE + 0x015d, 0x0000 },
-
-		{ USBTV_BASE + 0x0003, 0x0004 },
-		{ USBTV_BASE + 0x0100, 0x00d3 },
-		{ USBTV_BASE + 0x0115, 0x0015 },
-		{ USBTV_BASE + 0x0220, 0x002e },
-		{ USBTV_BASE + 0x0225, 0x0008 },
-		{ USBTV_BASE + 0x024e, 0x0002 },
-		{ USBTV_BASE + 0x024e, 0x0002 },
-		{ USBTV_BASE + 0x024f, 0x0002 },
 	};
 
 	ret = usbtv_set_regs(usbtv, setup, ARRAY_SIZE(setup));
-- 
2.17.0

^ permalink raw reply related	[flat|nested] 17+ messages in thread

* [PATCH v2 2/6] usbtv: Add SECAM support
  2018-04-08 21:11 ` [PATCH v2 0/6] usbtv: Add SECAM support and fix color encoding selection Hugo Grostabussiat
  2018-04-08 21:11   ` [PATCH v2 1/6] usbtv: Use same decoder sequence as Windows driver Hugo Grostabussiat
@ 2018-04-08 21:11   ` Hugo Grostabussiat
  2018-04-08 21:11   ` [PATCH v2 3/6] usbtv: Use V4L2 defines to select capture resolution Hugo Grostabussiat
                     ` (3 subsequent siblings)
  5 siblings, 0 replies; 17+ messages in thread
From: Hugo Grostabussiat @ 2018-04-08 21:11 UTC (permalink / raw)
  To: Hans Verkuil; +Cc: Mauro Carvalho Chehab, linux-media, Hugo Grostabussiat

Add support for the SECAM norm, using the "AVSECAM" decoder configuration
sequence found in Windows driver's .INF file.

For reference, the "AVSECAM" sequence in the .INF file is:
0x04,0x73,0xDC,0x72,0xA2,0x90,0x35,0x01,0x30,0x04,0x08,0x2D,0x28,0x08,
0x02,0x69,0x16,0x35,0x21,0x16,0x36

Signed-off-by: Hugo Grostabussiat <bonstra@bonstra.fr.eu.org>
---
 drivers/media/usb/usbtv/usbtv-video.c | 38 ++++++++++++++++++++++++++-
 drivers/media/usb/usbtv/usbtv.h       |  2 +-
 2 files changed, 38 insertions(+), 2 deletions(-)

diff --git a/drivers/media/usb/usbtv/usbtv-video.c b/drivers/media/usb/usbtv/usbtv-video.c
index 97f9790954f9..6b0a10173388 100644
--- a/drivers/media/usb/usbtv/usbtv-video.c
+++ b/drivers/media/usb/usbtv/usbtv-video.c
@@ -57,6 +57,11 @@ static struct usbtv_norm_params norm_params[] = {
 		.norm = V4L2_STD_PAL,
 		.cap_width = 720,
 		.cap_height = 576,
+	},
+	{
+		.norm = V4L2_STD_SECAM,
+		.cap_width = 720,
+		.cap_height = 576,
 	}
 };
 
@@ -187,6 +192,34 @@ static int usbtv_select_norm(struct usbtv *usbtv, v4l2_std_id norm)
 		{ USBTV_BASE + 0x024f, 0x0002 },
 	};
 
+	static const u16 secam[][2] = {
+		/* "AVSECAM" tuning sequence from .INF file */
+		{ USBTV_BASE + 0x0003, 0x0004 },
+		{ USBTV_BASE + 0x001a, 0x0073 },
+		{ USBTV_BASE + 0x0100, 0x00dc },
+		{ USBTV_BASE + 0x010e, 0x0072 },
+		{ USBTV_BASE + 0x010f, 0x00a2 },
+		{ USBTV_BASE + 0x0112, 0x0090 },
+		{ USBTV_BASE + 0x0115, 0x0035 },
+		{ USBTV_BASE + 0x0117, 0x0001 },
+		{ USBTV_BASE + 0x0118, 0x0030 },
+		{ USBTV_BASE + 0x012d, 0x0004 },
+		{ USBTV_BASE + 0x012f, 0x0008 },
+		{ USBTV_BASE + 0x0220, 0x002d },
+		{ USBTV_BASE + 0x0225, 0x0028 },
+		{ USBTV_BASE + 0x024e, 0x0008 },
+		{ USBTV_BASE + 0x024f, 0x0002 },
+		{ USBTV_BASE + 0x0254, 0x0069 },
+		{ USBTV_BASE + 0x025a, 0x0016 },
+		{ USBTV_BASE + 0x025b, 0x0035 },
+		{ USBTV_BASE + 0x0263, 0x0021 },
+		{ USBTV_BASE + 0x0266, 0x0016 },
+		{ USBTV_BASE + 0x0267, 0x0036 },
+		/* Epilog */
+		{ USBTV_BASE + 0x024e, 0x0002 },
+		{ USBTV_BASE + 0x024f, 0x0002 },
+	};
+
 	ret = usbtv_configure_for_norm(usbtv, norm);
 
 	if (!ret) {
@@ -194,6 +227,8 @@ static int usbtv_select_norm(struct usbtv *usbtv, v4l2_std_id norm)
 			ret = usbtv_set_regs(usbtv, ntsc, ARRAY_SIZE(ntsc));
 		else if (norm & V4L2_STD_PAL)
 			ret = usbtv_set_regs(usbtv, pal, ARRAY_SIZE(pal));
+		else if (norm & V4L2_STD_SECAM)
+			ret = usbtv_set_regs(usbtv, secam, ARRAY_SIZE(secam));
 	}
 
 	return ret;
@@ -605,7 +640,8 @@ static int usbtv_s_std(struct file *file, void *priv, v4l2_std_id norm)
 	int ret = -EINVAL;
 	struct usbtv *usbtv = video_drvdata(file);
 
-	if ((norm & V4L2_STD_525_60) || (norm & V4L2_STD_PAL))
+	if ((norm & V4L2_STD_525_60) || (norm & V4L2_STD_PAL) ||
+			(norm & V4L2_STD_SECAM))
 		ret = usbtv_select_norm(usbtv, norm);
 
 	return ret;
diff --git a/drivers/media/usb/usbtv/usbtv.h b/drivers/media/usb/usbtv/usbtv.h
index 0231e449877e..77a368e90fd0 100644
--- a/drivers/media/usb/usbtv/usbtv.h
+++ b/drivers/media/usb/usbtv/usbtv.h
@@ -68,7 +68,7 @@
 #define USBTV_ODD(chunk)	((be32_to_cpu(chunk[0]) & 0x0000f000) >> 15)
 #define USBTV_CHUNK_NO(chunk)	(be32_to_cpu(chunk[0]) & 0x00000fff)
 
-#define USBTV_TV_STD  (V4L2_STD_525_60 | V4L2_STD_PAL)
+#define USBTV_TV_STD  (V4L2_STD_525_60 | V4L2_STD_PAL | V4L2_STD_SECAM)
 
 /* parameters for supported TV norms */
 struct usbtv_norm_params {
-- 
2.17.0

^ permalink raw reply related	[flat|nested] 17+ messages in thread

* [PATCH v2 3/6] usbtv: Use V4L2 defines to select capture resolution
  2018-04-08 21:11 ` [PATCH v2 0/6] usbtv: Add SECAM support and fix color encoding selection Hugo Grostabussiat
  2018-04-08 21:11   ` [PATCH v2 1/6] usbtv: Use same decoder sequence as Windows driver Hugo Grostabussiat
  2018-04-08 21:11   ` [PATCH v2 2/6] usbtv: Add SECAM support Hugo Grostabussiat
@ 2018-04-08 21:11   ` Hugo Grostabussiat
  2018-04-08 21:11   ` [PATCH v2 4/6] usbtv: Keep norm parameter specific Hugo Grostabussiat
                     ` (2 subsequent siblings)
  5 siblings, 0 replies; 17+ messages in thread
From: Hugo Grostabussiat @ 2018-04-08 21:11 UTC (permalink / raw)
  To: Hans Verkuil; +Cc: Mauro Carvalho Chehab, linux-media, Hugo Grostabussiat

Make use of the V4L2_STD_525_60 and V4L2_STD_625_50 defines to
determine the vertical resolution to use when capturing.

V4L2_STD_525_60 (resp. V4L2_STD_625_50) is the set of standards using
525 (resp. 625) lines per frame, independently of the color encoding.

Signed-off-by: Hugo Grostabussiat <bonstra@bonstra.fr.eu.org>
---
 drivers/media/usb/usbtv/usbtv-video.c | 7 +------
 1 file changed, 1 insertion(+), 6 deletions(-)

diff --git a/drivers/media/usb/usbtv/usbtv-video.c b/drivers/media/usb/usbtv/usbtv-video.c
index 6b0a10173388..29e245083247 100644
--- a/drivers/media/usb/usbtv/usbtv-video.c
+++ b/drivers/media/usb/usbtv/usbtv-video.c
@@ -54,12 +54,7 @@ static struct usbtv_norm_params norm_params[] = {
 		.cap_height = 480,
 	},
 	{
-		.norm = V4L2_STD_PAL,
-		.cap_width = 720,
-		.cap_height = 576,
-	},
-	{
-		.norm = V4L2_STD_SECAM,
+		.norm = V4L2_STD_625_50,
 		.cap_width = 720,
 		.cap_height = 576,
 	}
-- 
2.17.0

^ permalink raw reply related	[flat|nested] 17+ messages in thread

* [PATCH v2 4/6] usbtv: Keep norm parameter specific
  2018-04-08 21:11 ` [PATCH v2 0/6] usbtv: Add SECAM support and fix color encoding selection Hugo Grostabussiat
                     ` (2 preceding siblings ...)
  2018-04-08 21:11   ` [PATCH v2 3/6] usbtv: Use V4L2 defines to select capture resolution Hugo Grostabussiat
@ 2018-04-08 21:11   ` Hugo Grostabussiat
  2018-04-08 21:12   ` [PATCH v2 5/6] usbtv: Enforce standard for color decoding Hugo Grostabussiat
  2018-04-08 21:12   ` [PATCH v2 6/6] usbtv: Use the constant for supported standards Hugo Grostabussiat
  5 siblings, 0 replies; 17+ messages in thread
From: Hugo Grostabussiat @ 2018-04-08 21:11 UTC (permalink / raw)
  To: Hans Verkuil; +Cc: Mauro Carvalho Chehab, linux-media, Hugo Grostabussiat

The user-supplied norm value gets overwritten by the generic .norm
member from the norm_params. That way, we lose the specific norm the
user may want to set.

For instance, if the user specifies V4L2_STD_PAL_60, the value actually
used will be V4L2_STD_525_60, which in the end will be as if the user
had specified V4L2_STD_NTSC, since this is always the first bitfield we
match the norm value against before configuring the hardware.

The norm_params array is only there to match a norm with an output
resolution. The norm value itself should not be changed.

Signed-off-by: Hugo Grostabussiat <bonstra@bonstra.fr.eu.org>
---
 drivers/media/usb/usbtv/usbtv-video.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/media/usb/usbtv/usbtv-video.c b/drivers/media/usb/usbtv/usbtv-video.c
index 29e245083247..6cad50d1e5f8 100644
--- a/drivers/media/usb/usbtv/usbtv-video.c
+++ b/drivers/media/usb/usbtv/usbtv-video.c
@@ -77,7 +77,7 @@ static int usbtv_configure_for_norm(struct usbtv *usbtv, v4l2_std_id norm)
 		usbtv->height = params->cap_height;
 		usbtv->n_chunks = usbtv->width * usbtv->height
 						/ 4 / USBTV_CHUNK;
-		usbtv->norm = params->norm;
+		usbtv->norm = norm;
 	} else
 		ret = -EINVAL;
 
-- 
2.17.0

^ permalink raw reply related	[flat|nested] 17+ messages in thread

* [PATCH v2 5/6] usbtv: Enforce standard for color decoding
  2018-04-08 21:11 ` [PATCH v2 0/6] usbtv: Add SECAM support and fix color encoding selection Hugo Grostabussiat
                     ` (3 preceding siblings ...)
  2018-04-08 21:11   ` [PATCH v2 4/6] usbtv: Keep norm parameter specific Hugo Grostabussiat
@ 2018-04-08 21:12   ` Hugo Grostabussiat
  2018-04-09  5:17     ` kbuild test robot
  2018-04-10 22:29     ` [PATCH v2 5/6] " kbuild test robot
  2018-04-08 21:12   ` [PATCH v2 6/6] usbtv: Use the constant for supported standards Hugo Grostabussiat
  5 siblings, 2 replies; 17+ messages in thread
From: Hugo Grostabussiat @ 2018-04-08 21:12 UTC (permalink / raw)
  To: Hans Verkuil; +Cc: Mauro Carvalho Chehab, linux-media, Hugo Grostabussiat

Depending on the chosen standard, configure the decoder to use the
appropriate color encoding standard (PAL-like, NTSC-like or SECAM).

Until now, the decoder was not configured for a specific color standard,
making it autodetect the color encoding.

While this may sound fine, it potentially causes the wrong image tuning
parameters to be applied (e.g. tuning parameters for NTSC are applied to
a PAL source), and may confuse users about what the actual standard is
in use.

This commit explicitly configures the color standard the decoder will
use, making it visually obvious if a wrong standard was chosen.

Signed-off-by: Hugo Grostabussiat <bonstra@bonstra.fr.eu.org>
---
 drivers/media/usb/usbtv/usbtv-video.c | 45 ++++++++++++++++++++++++---
 1 file changed, 40 insertions(+), 5 deletions(-)

diff --git a/drivers/media/usb/usbtv/usbtv-video.c b/drivers/media/usb/usbtv/usbtv-video.c
index 6cad50d1e5f8..d0bf5eb217b1 100644
--- a/drivers/media/usb/usbtv/usbtv-video.c
+++ b/drivers/media/usb/usbtv/usbtv-video.c
@@ -121,6 +121,25 @@ static int usbtv_select_input(struct usbtv *usbtv, int input)
 	return ret;
 }
 
+static uint16_t usbtv_norm_to_16f_reg(v4l2_std_id norm)
+{
+	/* NTSC M/M-JP/M-KR */
+	if (norm & V4L2_STD_NTSC)
+		return 0x00b8;
+	/* PAL BG/DK/H/I */
+	if (norm & V4L2_STD_PAL)
+		return 0x00ee;
+	/* SECAM B/D/G/H/K/K1/L/Lc */
+	if (norm & V4L2_STD_SECAM)
+		return 0x00ff;
+	if (norm & V4L2_STD_NTSC_443)
+		return 0x00a8;
+	if (norm & (V4L2_STD_PAL_M | V4L2_STD_PAL_60))
+		return 0x00bc;
+	/* Fallback to automatic detection for other standards */
+	return 0x0000;
+}
+
 static int usbtv_select_norm(struct usbtv *usbtv, v4l2_std_id norm)
 {
 	int ret;
@@ -154,7 +173,7 @@ static int usbtv_select_norm(struct usbtv *usbtv, v4l2_std_id norm)
 		{ USBTV_BASE + 0x0263, 0x0017 },
 		{ USBTV_BASE + 0x0266, 0x0016 },
 		{ USBTV_BASE + 0x0267, 0x0036 },
-		/* Epilog */
+		/* End image tuning */
 		{ USBTV_BASE + 0x024e, 0x0002 },
 		{ USBTV_BASE + 0x024f, 0x0002 },
 	};
@@ -182,7 +201,7 @@ static int usbtv_select_norm(struct usbtv *usbtv, v4l2_std_id norm)
 		{ USBTV_BASE + 0x0263, 0x001c },
 		{ USBTV_BASE + 0x0266, 0x0011 },
 		{ USBTV_BASE + 0x0267, 0x0005 },
-		/* Epilog */
+		/* End image tuning */
 		{ USBTV_BASE + 0x024e, 0x0002 },
 		{ USBTV_BASE + 0x024f, 0x0002 },
 	};
@@ -210,7 +229,7 @@ static int usbtv_select_norm(struct usbtv *usbtv, v4l2_std_id norm)
 		{ USBTV_BASE + 0x0263, 0x0021 },
 		{ USBTV_BASE + 0x0266, 0x0016 },
 		{ USBTV_BASE + 0x0267, 0x0036 },
-		/* Epilog */
+		/* End image tuning */
 		{ USBTV_BASE + 0x024e, 0x0002 },
 		{ USBTV_BASE + 0x024f, 0x0002 },
 	};
@@ -218,12 +237,28 @@ static int usbtv_select_norm(struct usbtv *usbtv, v4l2_std_id norm)
 	ret = usbtv_configure_for_norm(usbtv, norm);
 
 	if (!ret) {
-		if (norm & V4L2_STD_525_60)
+		/* Masks for norms using a NTSC or PAL color encoding. */
+		static const v4l2_std_id ntsc_mask =
+			V4L2_STD_NTSC | V4L2_STD_NTSC_443;
+		static const v4l2_std_id pal_mask =
+			V4L2_STD_PAL | V4L2_STD_PAL_60 | V4L2_STD_PAL_M;
+
+		if (norm & ntsc_mask)
 			ret = usbtv_set_regs(usbtv, ntsc, ARRAY_SIZE(ntsc));
-		else if (norm & V4L2_STD_PAL)
+		else if (norm & pal_mask)
 			ret = usbtv_set_regs(usbtv, pal, ARRAY_SIZE(pal));
 		else if (norm & V4L2_STD_SECAM)
 			ret = usbtv_set_regs(usbtv, secam, ARRAY_SIZE(secam));
+		else
+			ret = -EINVAL;
+	}
+
+	if (!ret) {
+		/* Configure the decoder for the color standard */
+		u16 cfg[][2] = {
+			{ USBTV_BASE + 0x016f, usbtv_norm_to_16f_reg(norm) }
+		};
+		ret = usbtv_set_regs(usbtv, cfg, ARRAY_SIZE(cfg));
 	}
 
 	return ret;
-- 
2.17.0

^ permalink raw reply related	[flat|nested] 17+ messages in thread

* [PATCH v2 6/6] usbtv: Use the constant for supported standards
  2018-04-08 21:11 ` [PATCH v2 0/6] usbtv: Add SECAM support and fix color encoding selection Hugo Grostabussiat
                     ` (4 preceding siblings ...)
  2018-04-08 21:12   ` [PATCH v2 5/6] usbtv: Enforce standard for color decoding Hugo Grostabussiat
@ 2018-04-08 21:12   ` Hugo Grostabussiat
  5 siblings, 0 replies; 17+ messages in thread
From: Hugo Grostabussiat @ 2018-04-08 21:12 UTC (permalink / raw)
  To: Hans Verkuil; +Cc: Mauro Carvalho Chehab, linux-media, Hugo Grostabussiat

Use the USBTV_TV_STD define instead of repeating ourselves.

Signed-off-by: Hugo Grostabussiat <bonstra@bonstra.fr.eu.org>
---
 drivers/media/usb/usbtv/usbtv-video.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/media/usb/usbtv/usbtv-video.c b/drivers/media/usb/usbtv/usbtv-video.c
index d0bf5eb217b1..eda51dbf063b 100644
--- a/drivers/media/usb/usbtv/usbtv-video.c
+++ b/drivers/media/usb/usbtv/usbtv-video.c
@@ -670,8 +670,7 @@ static int usbtv_s_std(struct file *file, void *priv, v4l2_std_id norm)
 	int ret = -EINVAL;
 	struct usbtv *usbtv = video_drvdata(file);
 
-	if ((norm & V4L2_STD_525_60) || (norm & V4L2_STD_PAL) ||
-			(norm & V4L2_STD_SECAM))
+	if (norm & USBTV_TV_STD)
 		ret = usbtv_select_norm(usbtv, norm);
 
 	return ret;
-- 
2.17.0

^ permalink raw reply related	[flat|nested] 17+ messages in thread

* Re: [PATCH v2 5/6] usbtv: Enforce standard for color decoding
  2018-04-08 21:12   ` [PATCH v2 5/6] usbtv: Enforce standard for color decoding Hugo Grostabussiat
@ 2018-04-09  5:17     ` kbuild test robot
  2018-04-09 23:13       ` [PATCH v3] " Hugo Grostabussiat
  2018-04-10 22:29     ` [PATCH v2 5/6] " kbuild test robot
  1 sibling, 1 reply; 17+ messages in thread
From: kbuild test robot @ 2018-04-09  5:17 UTC (permalink / raw)
  To: Hugo Grostabussiat
  Cc: kbuild-all, Hans Verkuil, Mauro Carvalho Chehab, linux-media,
	Hugo Grostabussiat

Hi Hugo,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on linuxtv-media/master]
[also build test WARNING on v4.16 next-20180406]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Hugo-Grostabussiat/usbtv-Add-SECAM-support-and-fix-color-encoding-selection/20180409-054206
base:   git://linuxtv.org/media_tree.git master
reproduce:
        # apt-get install sparse
        make ARCH=x86_64 allmodconfig
        make C=1 CF=-D__CHECK_ENDIAN__


sparse warnings: (new ones prefixed by >>)

>> drivers/media/usb/usbtv/usbtv-video.c:261:45: sparse: incorrect type in argument 2 (different modifiers) @@    expected unsigned short const [usertype] ( *regs )[2] @@    got ed short const [usertype] ( *regs )[2] @@
   drivers/media/usb/usbtv/usbtv-video.c:261:45:    expected unsigned short const [usertype] ( *regs )[2]
   drivers/media/usb/usbtv/usbtv-video.c:261:45:    got unsigned short [usertype] ( *<noident> )[2]

vim +261 drivers/media/usb/usbtv/usbtv-video.c

   142	
   143	static int usbtv_select_norm(struct usbtv *usbtv, v4l2_std_id norm)
   144	{
   145		int ret;
   146		/* These are the series of register values used to configure the
   147		 * decoder for a specific standard.
   148		 * The first 21 register writes are copied from the
   149		 * Settings\DecoderDefaults registry keys present in the Windows driver
   150		 * .INF file, and control various image tuning parameters (color
   151		 * correction, sharpness, ...).
   152		 */
   153		static const u16 pal[][2] = {
   154			/* "AVPAL" tuning sequence from .INF file */
   155			{ USBTV_BASE + 0x0003, 0x0004 },
   156			{ USBTV_BASE + 0x001a, 0x0068 },
   157			{ USBTV_BASE + 0x0100, 0x00d3 },
   158			{ USBTV_BASE + 0x010e, 0x0072 },
   159			{ USBTV_BASE + 0x010f, 0x00a2 },
   160			{ USBTV_BASE + 0x0112, 0x00b0 },
   161			{ USBTV_BASE + 0x0115, 0x0015 },
   162			{ USBTV_BASE + 0x0117, 0x0001 },
   163			{ USBTV_BASE + 0x0118, 0x002c },
   164			{ USBTV_BASE + 0x012d, 0x0010 },
   165			{ USBTV_BASE + 0x012f, 0x0020 },
   166			{ USBTV_BASE + 0x0220, 0x002e },
   167			{ USBTV_BASE + 0x0225, 0x0008 },
   168			{ USBTV_BASE + 0x024e, 0x0002 },
   169			{ USBTV_BASE + 0x024f, 0x0002 },
   170			{ USBTV_BASE + 0x0254, 0x0059 },
   171			{ USBTV_BASE + 0x025a, 0x0016 },
   172			{ USBTV_BASE + 0x025b, 0x0035 },
   173			{ USBTV_BASE + 0x0263, 0x0017 },
   174			{ USBTV_BASE + 0x0266, 0x0016 },
   175			{ USBTV_BASE + 0x0267, 0x0036 },
   176			/* End image tuning */
   177			{ USBTV_BASE + 0x024e, 0x0002 },
   178			{ USBTV_BASE + 0x024f, 0x0002 },
   179		};
   180	
   181		static const u16 ntsc[][2] = {
   182			/* "AVNTSC" tuning sequence from .INF file */
   183			{ USBTV_BASE + 0x0003, 0x0004 },
   184			{ USBTV_BASE + 0x001a, 0x0079 },
   185			{ USBTV_BASE + 0x0100, 0x00d3 },
   186			{ USBTV_BASE + 0x010e, 0x0068 },
   187			{ USBTV_BASE + 0x010f, 0x009c },
   188			{ USBTV_BASE + 0x0112, 0x00f0 },
   189			{ USBTV_BASE + 0x0115, 0x0015 },
   190			{ USBTV_BASE + 0x0117, 0x0000 },
   191			{ USBTV_BASE + 0x0118, 0x00fc },
   192			{ USBTV_BASE + 0x012d, 0x0004 },
   193			{ USBTV_BASE + 0x012f, 0x0008 },
   194			{ USBTV_BASE + 0x0220, 0x002e },
   195			{ USBTV_BASE + 0x0225, 0x0008 },
   196			{ USBTV_BASE + 0x024e, 0x0002 },
   197			{ USBTV_BASE + 0x024f, 0x0001 },
   198			{ USBTV_BASE + 0x0254, 0x005f },
   199			{ USBTV_BASE + 0x025a, 0x0012 },
   200			{ USBTV_BASE + 0x025b, 0x0001 },
   201			{ USBTV_BASE + 0x0263, 0x001c },
   202			{ USBTV_BASE + 0x0266, 0x0011 },
   203			{ USBTV_BASE + 0x0267, 0x0005 },
   204			/* End image tuning */
   205			{ USBTV_BASE + 0x024e, 0x0002 },
   206			{ USBTV_BASE + 0x024f, 0x0002 },
   207		};
   208	
   209		static const u16 secam[][2] = {
   210			/* "AVSECAM" tuning sequence from .INF file */
   211			{ USBTV_BASE + 0x0003, 0x0004 },
   212			{ USBTV_BASE + 0x001a, 0x0073 },
   213			{ USBTV_BASE + 0x0100, 0x00dc },
   214			{ USBTV_BASE + 0x010e, 0x0072 },
   215			{ USBTV_BASE + 0x010f, 0x00a2 },
   216			{ USBTV_BASE + 0x0112, 0x0090 },
   217			{ USBTV_BASE + 0x0115, 0x0035 },
   218			{ USBTV_BASE + 0x0117, 0x0001 },
   219			{ USBTV_BASE + 0x0118, 0x0030 },
   220			{ USBTV_BASE + 0x012d, 0x0004 },
   221			{ USBTV_BASE + 0x012f, 0x0008 },
   222			{ USBTV_BASE + 0x0220, 0x002d },
   223			{ USBTV_BASE + 0x0225, 0x0028 },
   224			{ USBTV_BASE + 0x024e, 0x0008 },
   225			{ USBTV_BASE + 0x024f, 0x0002 },
   226			{ USBTV_BASE + 0x0254, 0x0069 },
   227			{ USBTV_BASE + 0x025a, 0x0016 },
   228			{ USBTV_BASE + 0x025b, 0x0035 },
   229			{ USBTV_BASE + 0x0263, 0x0021 },
   230			{ USBTV_BASE + 0x0266, 0x0016 },
   231			{ USBTV_BASE + 0x0267, 0x0036 },
   232			/* End image tuning */
   233			{ USBTV_BASE + 0x024e, 0x0002 },
   234			{ USBTV_BASE + 0x024f, 0x0002 },
   235		};
   236	
   237		ret = usbtv_configure_for_norm(usbtv, norm);
   238	
   239		if (!ret) {
   240			/* Masks for norms using a NTSC or PAL color encoding. */
   241			static const v4l2_std_id ntsc_mask =
   242				V4L2_STD_NTSC | V4L2_STD_NTSC_443;
   243			static const v4l2_std_id pal_mask =
   244				V4L2_STD_PAL | V4L2_STD_PAL_60 | V4L2_STD_PAL_M;
   245	
   246			if (norm & ntsc_mask)
   247				ret = usbtv_set_regs(usbtv, ntsc, ARRAY_SIZE(ntsc));
   248			else if (norm & pal_mask)
   249				ret = usbtv_set_regs(usbtv, pal, ARRAY_SIZE(pal));
   250			else if (norm & V4L2_STD_SECAM)
   251				ret = usbtv_set_regs(usbtv, secam, ARRAY_SIZE(secam));
   252			else
   253				ret = -EINVAL;
   254		}
   255	
   256		if (!ret) {
   257			/* Configure the decoder for the color standard */
   258			u16 cfg[][2] = {
   259				{ USBTV_BASE + 0x016f, usbtv_norm_to_16f_reg(norm) }
   260			};
 > 261			ret = usbtv_set_regs(usbtv, cfg, ARRAY_SIZE(cfg));
   262		}
   263	
   264		return ret;
   265	}
   266	

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

^ permalink raw reply	[flat|nested] 17+ messages in thread

* [PATCH v3] usbtv: Enforce standard for color decoding
  2018-04-09  5:17     ` kbuild test robot
@ 2018-04-09 23:13       ` Hugo Grostabussiat
  2018-04-09 23:35         ` Hugo "Bonstra" Grostabussiat
  0 siblings, 1 reply; 17+ messages in thread
From: Hugo Grostabussiat @ 2018-04-09 23:13 UTC (permalink / raw)
  To: Hans Verkuil; +Cc: Mauro Carvalho Chehab, linux-media, Hugo Grostabussiat

Depending on the chosen standard, configure the decoder to use the
appropriate color encoding standard (PAL-like, NTSC-like or SECAM).

Until now, the decoder was not configured for a specific color standard,
making it autodetect the color encoding.

While this may sound fine, it potentially causes the wrong image tuning
parameters to be applied (e.g. tuning parameters for NTSC are applied to
a PAL source), and may confuse users about what the actual standard is
in use.

This commit explicitly configures the color standard the decoder will
use, making it visually obvious if a wrong standard was chosen.

Signed-off-by: Hugo Grostabussiat <bonstra@bonstra.fr.eu.org>
---
 drivers/media/usb/usbtv/usbtv-video.c | 45 ++++++++++++++++++++++++---
 1 file changed, 40 insertions(+), 5 deletions(-)

diff --git a/drivers/media/usb/usbtv/usbtv-video.c b/drivers/media/usb/usbtv/usbtv-video.c
index 6cad50d1e5f8..767fab1cc5cf 100644
--- a/drivers/media/usb/usbtv/usbtv-video.c
+++ b/drivers/media/usb/usbtv/usbtv-video.c
@@ -121,6 +121,25 @@ static int usbtv_select_input(struct usbtv *usbtv, int input)
 	return ret;
 }
 
+static uint16_t usbtv_norm_to_16f_reg(v4l2_std_id norm)
+{
+	/* NTSC M/M-JP/M-KR */
+	if (norm & V4L2_STD_NTSC)
+		return 0x00b8;
+	/* PAL BG/DK/H/I */
+	if (norm & V4L2_STD_PAL)
+		return 0x00ee;
+	/* SECAM B/D/G/H/K/K1/L/Lc */
+	if (norm & V4L2_STD_SECAM)
+		return 0x00ff;
+	if (norm & V4L2_STD_NTSC_443)
+		return 0x00a8;
+	if (norm & (V4L2_STD_PAL_M | V4L2_STD_PAL_60))
+		return 0x00bc;
+	/* Fallback to automatic detection for other standards */
+	return 0x0000;
+}
+
 static int usbtv_select_norm(struct usbtv *usbtv, v4l2_std_id norm)
 {
 	int ret;
@@ -154,7 +173,7 @@ static int usbtv_select_norm(struct usbtv *usbtv, v4l2_std_id norm)
 		{ USBTV_BASE + 0x0263, 0x0017 },
 		{ USBTV_BASE + 0x0266, 0x0016 },
 		{ USBTV_BASE + 0x0267, 0x0036 },
-		/* Epilog */
+		/* End image tuning */
 		{ USBTV_BASE + 0x024e, 0x0002 },
 		{ USBTV_BASE + 0x024f, 0x0002 },
 	};
@@ -182,7 +201,7 @@ static int usbtv_select_norm(struct usbtv *usbtv, v4l2_std_id norm)
 		{ USBTV_BASE + 0x0263, 0x001c },
 		{ USBTV_BASE + 0x0266, 0x0011 },
 		{ USBTV_BASE + 0x0267, 0x0005 },
-		/* Epilog */
+		/* End image tuning */
 		{ USBTV_BASE + 0x024e, 0x0002 },
 		{ USBTV_BASE + 0x024f, 0x0002 },
 	};
@@ -210,7 +229,7 @@ static int usbtv_select_norm(struct usbtv *usbtv, v4l2_std_id norm)
 		{ USBTV_BASE + 0x0263, 0x0021 },
 		{ USBTV_BASE + 0x0266, 0x0016 },
 		{ USBTV_BASE + 0x0267, 0x0036 },
-		/* Epilog */
+		/* End image tuning */
 		{ USBTV_BASE + 0x024e, 0x0002 },
 		{ USBTV_BASE + 0x024f, 0x0002 },
 	};
@@ -218,12 +237,28 @@ static int usbtv_select_norm(struct usbtv *usbtv, v4l2_std_id norm)
 	ret = usbtv_configure_for_norm(usbtv, norm);
 
 	if (!ret) {
-		if (norm & V4L2_STD_525_60)
+		/* Masks for norms using a NTSC or PAL color encoding. */
+		static const v4l2_std_id ntsc_mask =
+			V4L2_STD_NTSC | V4L2_STD_NTSC_443;
+		static const v4l2_std_id pal_mask =
+			V4L2_STD_PAL | V4L2_STD_PAL_60 | V4L2_STD_PAL_M;
+
+		if (norm & ntsc_mask)
 			ret = usbtv_set_regs(usbtv, ntsc, ARRAY_SIZE(ntsc));
-		else if (norm & V4L2_STD_PAL)
+		else if (norm & pal_mask)
 			ret = usbtv_set_regs(usbtv, pal, ARRAY_SIZE(pal));
 		else if (norm & V4L2_STD_SECAM)
 			ret = usbtv_set_regs(usbtv, secam, ARRAY_SIZE(secam));
+		else
+			ret = -EINVAL;
+	}
+
+	if (!ret) {
+		/* Configure the decoder for the color standard */
+		const u16 cfg[][2] = {
+			{ USBTV_BASE + 0x016f, usbtv_norm_to_16f_reg(norm) }
+		};
+		ret = usbtv_set_regs(usbtv, cfg, ARRAY_SIZE(cfg));
 	}
 
 	return ret;
-- 
2.17.0

^ permalink raw reply related	[flat|nested] 17+ messages in thread

* Re: [PATCH v3] usbtv: Enforce standard for color decoding
  2018-04-09 23:13       ` [PATCH v3] " Hugo Grostabussiat
@ 2018-04-09 23:35         ` Hugo "Bonstra" Grostabussiat
  0 siblings, 0 replies; 17+ messages in thread
From: Hugo "Bonstra" Grostabussiat @ 2018-04-09 23:35 UTC (permalink / raw)
  To: Hans Verkuil; +Cc: Mauro Carvalho Chehab, linux-media, bonstra

Le 10/04/2018 à 01:13, Hugo Grostabussiat a écrit :
>  	if (!ret) {
>  		/* Configure the decoder for the color standard */
> -		u16 cfg[][2] = {
> +		const u16 cfg[][2] = {
>  			{ USBTV_BASE + 0x016f, usbtv_norm_to_16f_reg(norm) }
>  		};
>  		ret = usbtv_set_regs(usbtv, cfg, ARRAY_SIZE(cfg));

This sums up the differences between the v2 and the v3 of this patch

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v2 5/6] usbtv: Enforce standard for color decoding
  2018-04-08 21:12   ` [PATCH v2 5/6] usbtv: Enforce standard for color decoding Hugo Grostabussiat
  2018-04-09  5:17     ` kbuild test robot
@ 2018-04-10 22:29     ` kbuild test robot
  1 sibling, 0 replies; 17+ messages in thread
From: kbuild test robot @ 2018-04-10 22:29 UTC (permalink / raw)
  To: Hugo Grostabussiat
  Cc: kbuild-all, Hans Verkuil, Mauro Carvalho Chehab, linux-media,
	Hugo Grostabussiat

[-- Attachment #1: Type: text/plain, Size: 6301 bytes --]

Hi Hugo,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on linuxtv-media/master]
[also build test WARNING on v4.16 next-20180410]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Hugo-Grostabussiat/usbtv-Add-SECAM-support-and-fix-color-encoding-selection/20180409-054206
base:   git://linuxtv.org/media_tree.git master
config: i386-randconfig-h0-04110123 (attached as .config)
compiler: gcc-4.9 (Debian 4.9.4-2) 4.9.4
reproduce:
        # save the attached .config to linux build tree
        make ARCH=i386 

All warnings (new ones prefixed by >>):

   drivers/media/usb/usbtv/usbtv-video.c: In function 'usbtv_select_norm':
>> drivers/media/usb/usbtv/usbtv-video.c:261:31: warning: passing argument 2 of 'usbtv_set_regs' from incompatible pointer type
      ret = usbtv_set_regs(usbtv, cfg, ARRAY_SIZE(cfg));
                                  ^
   In file included from drivers/media/usb/usbtv/usbtv-video.c:48:0:
   drivers/media/usb/usbtv/usbtv.h:129:5: note: expected 'const u16 (*)[2]' but argument is of type 'u16 (*)[2]'
    int usbtv_set_regs(struct usbtv *usbtv, const u16 regs[][2], int size);
        ^

vim +/usbtv_set_regs +261 drivers/media/usb/usbtv/usbtv-video.c

   142	
   143	static int usbtv_select_norm(struct usbtv *usbtv, v4l2_std_id norm)
   144	{
   145		int ret;
   146		/* These are the series of register values used to configure the
   147		 * decoder for a specific standard.
   148		 * The first 21 register writes are copied from the
   149		 * Settings\DecoderDefaults registry keys present in the Windows driver
   150		 * .INF file, and control various image tuning parameters (color
   151		 * correction, sharpness, ...).
   152		 */
   153		static const u16 pal[][2] = {
   154			/* "AVPAL" tuning sequence from .INF file */
   155			{ USBTV_BASE + 0x0003, 0x0004 },
   156			{ USBTV_BASE + 0x001a, 0x0068 },
   157			{ USBTV_BASE + 0x0100, 0x00d3 },
   158			{ USBTV_BASE + 0x010e, 0x0072 },
   159			{ USBTV_BASE + 0x010f, 0x00a2 },
   160			{ USBTV_BASE + 0x0112, 0x00b0 },
   161			{ USBTV_BASE + 0x0115, 0x0015 },
   162			{ USBTV_BASE + 0x0117, 0x0001 },
   163			{ USBTV_BASE + 0x0118, 0x002c },
   164			{ USBTV_BASE + 0x012d, 0x0010 },
   165			{ USBTV_BASE + 0x012f, 0x0020 },
   166			{ USBTV_BASE + 0x0220, 0x002e },
   167			{ USBTV_BASE + 0x0225, 0x0008 },
   168			{ USBTV_BASE + 0x024e, 0x0002 },
   169			{ USBTV_BASE + 0x024f, 0x0002 },
   170			{ USBTV_BASE + 0x0254, 0x0059 },
   171			{ USBTV_BASE + 0x025a, 0x0016 },
   172			{ USBTV_BASE + 0x025b, 0x0035 },
   173			{ USBTV_BASE + 0x0263, 0x0017 },
   174			{ USBTV_BASE + 0x0266, 0x0016 },
   175			{ USBTV_BASE + 0x0267, 0x0036 },
   176			/* End image tuning */
   177			{ USBTV_BASE + 0x024e, 0x0002 },
   178			{ USBTV_BASE + 0x024f, 0x0002 },
   179		};
   180	
   181		static const u16 ntsc[][2] = {
   182			/* "AVNTSC" tuning sequence from .INF file */
   183			{ USBTV_BASE + 0x0003, 0x0004 },
   184			{ USBTV_BASE + 0x001a, 0x0079 },
   185			{ USBTV_BASE + 0x0100, 0x00d3 },
   186			{ USBTV_BASE + 0x010e, 0x0068 },
   187			{ USBTV_BASE + 0x010f, 0x009c },
   188			{ USBTV_BASE + 0x0112, 0x00f0 },
   189			{ USBTV_BASE + 0x0115, 0x0015 },
   190			{ USBTV_BASE + 0x0117, 0x0000 },
   191			{ USBTV_BASE + 0x0118, 0x00fc },
   192			{ USBTV_BASE + 0x012d, 0x0004 },
   193			{ USBTV_BASE + 0x012f, 0x0008 },
   194			{ USBTV_BASE + 0x0220, 0x002e },
   195			{ USBTV_BASE + 0x0225, 0x0008 },
   196			{ USBTV_BASE + 0x024e, 0x0002 },
   197			{ USBTV_BASE + 0x024f, 0x0001 },
   198			{ USBTV_BASE + 0x0254, 0x005f },
   199			{ USBTV_BASE + 0x025a, 0x0012 },
   200			{ USBTV_BASE + 0x025b, 0x0001 },
   201			{ USBTV_BASE + 0x0263, 0x001c },
   202			{ USBTV_BASE + 0x0266, 0x0011 },
   203			{ USBTV_BASE + 0x0267, 0x0005 },
   204			/* End image tuning */
   205			{ USBTV_BASE + 0x024e, 0x0002 },
   206			{ USBTV_BASE + 0x024f, 0x0002 },
   207		};
   208	
   209		static const u16 secam[][2] = {
   210			/* "AVSECAM" tuning sequence from .INF file */
   211			{ USBTV_BASE + 0x0003, 0x0004 },
   212			{ USBTV_BASE + 0x001a, 0x0073 },
   213			{ USBTV_BASE + 0x0100, 0x00dc },
   214			{ USBTV_BASE + 0x010e, 0x0072 },
   215			{ USBTV_BASE + 0x010f, 0x00a2 },
   216			{ USBTV_BASE + 0x0112, 0x0090 },
   217			{ USBTV_BASE + 0x0115, 0x0035 },
   218			{ USBTV_BASE + 0x0117, 0x0001 },
   219			{ USBTV_BASE + 0x0118, 0x0030 },
   220			{ USBTV_BASE + 0x012d, 0x0004 },
   221			{ USBTV_BASE + 0x012f, 0x0008 },
   222			{ USBTV_BASE + 0x0220, 0x002d },
   223			{ USBTV_BASE + 0x0225, 0x0028 },
   224			{ USBTV_BASE + 0x024e, 0x0008 },
   225			{ USBTV_BASE + 0x024f, 0x0002 },
   226			{ USBTV_BASE + 0x0254, 0x0069 },
   227			{ USBTV_BASE + 0x025a, 0x0016 },
   228			{ USBTV_BASE + 0x025b, 0x0035 },
   229			{ USBTV_BASE + 0x0263, 0x0021 },
   230			{ USBTV_BASE + 0x0266, 0x0016 },
   231			{ USBTV_BASE + 0x0267, 0x0036 },
   232			/* End image tuning */
   233			{ USBTV_BASE + 0x024e, 0x0002 },
   234			{ USBTV_BASE + 0x024f, 0x0002 },
   235		};
   236	
   237		ret = usbtv_configure_for_norm(usbtv, norm);
   238	
   239		if (!ret) {
   240			/* Masks for norms using a NTSC or PAL color encoding. */
   241			static const v4l2_std_id ntsc_mask =
   242				V4L2_STD_NTSC | V4L2_STD_NTSC_443;
   243			static const v4l2_std_id pal_mask =
   244				V4L2_STD_PAL | V4L2_STD_PAL_60 | V4L2_STD_PAL_M;
   245	
   246			if (norm & ntsc_mask)
   247				ret = usbtv_set_regs(usbtv, ntsc, ARRAY_SIZE(ntsc));
   248			else if (norm & pal_mask)
   249				ret = usbtv_set_regs(usbtv, pal, ARRAY_SIZE(pal));
   250			else if (norm & V4L2_STD_SECAM)
   251				ret = usbtv_set_regs(usbtv, secam, ARRAY_SIZE(secam));
   252			else
   253				ret = -EINVAL;
   254		}
   255	
   256		if (!ret) {
   257			/* Configure the decoder for the color standard */
   258			u16 cfg[][2] = {
   259				{ USBTV_BASE + 0x016f, usbtv_norm_to_16f_reg(norm) }
   260			};
 > 261			ret = usbtv_set_regs(usbtv, cfg, ARRAY_SIZE(cfg));
   262		}
   263	
   264		return ret;
   265	}
   266	

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 28821 bytes --]

^ permalink raw reply	[flat|nested] 17+ messages in thread

end of thread, other threads:[~2018-04-10 22:30 UTC | newest]

Thread overview: 17+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-02-24 18:24 [PATCH 0/2] usbtv: Add SECAM support Hugo Grostabussiat
2018-02-24 18:24 ` [PATCH 1/2] usbtv: Use same decoder sequence as Windows driver Hugo Grostabussiat
2018-02-26 14:12   ` Hans Verkuil
2018-02-27 22:35     ` Hugo "Bonstra" Grostabussiat
2018-03-09 10:55       ` Hans Verkuil
2018-02-24 18:24 ` [PATCH 2/2] usbtv: Add SECAM support Hugo Grostabussiat
2018-04-08 21:11 ` [PATCH v2 0/6] usbtv: Add SECAM support and fix color encoding selection Hugo Grostabussiat
2018-04-08 21:11   ` [PATCH v2 1/6] usbtv: Use same decoder sequence as Windows driver Hugo Grostabussiat
2018-04-08 21:11   ` [PATCH v2 2/6] usbtv: Add SECAM support Hugo Grostabussiat
2018-04-08 21:11   ` [PATCH v2 3/6] usbtv: Use V4L2 defines to select capture resolution Hugo Grostabussiat
2018-04-08 21:11   ` [PATCH v2 4/6] usbtv: Keep norm parameter specific Hugo Grostabussiat
2018-04-08 21:12   ` [PATCH v2 5/6] usbtv: Enforce standard for color decoding Hugo Grostabussiat
2018-04-09  5:17     ` kbuild test robot
2018-04-09 23:13       ` [PATCH v3] " Hugo Grostabussiat
2018-04-09 23:35         ` Hugo "Bonstra" Grostabussiat
2018-04-10 22:29     ` [PATCH v2 5/6] " kbuild test robot
2018-04-08 21:12   ` [PATCH v2 6/6] usbtv: Use the constant for supported standards Hugo Grostabussiat

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).