diff mbox series

[REPOST] TDA1997x: replace video detection routine

Message ID m3k0lmejlw.fsf@t19.piap.pl
State New
Headers show
Series [REPOST] TDA1997x: replace video detection routine | expand

Commit Message

Krzysztof Hałasa July 19, 2021, 9:30 a.m. UTC
The TDA1997x (HDMI receiver) driver currently uses a specific video
format detection scheme. The frame (or field in interlaced mode), line
and HSync pulse durations are compared to those of known, standard video
modes. If a match is found, the mode is assumed to be detected,
otherwise -ERANGE is returned (then possibly ignored). This means that:
- another mode with similar timings will be detected incorrectly
  (e.g. 2x faster clock and lines twice as long)
- non-standard modes will not work.

I propose to replace this scheme with a direct read of geometry
registers. This way all modes recognized by the chip will be supported.

How does this change look like from the user's perspective?

v4l2-ctl -d $(media-ctl -e "tda19971 2-0048") --query-dv-timings

Original code - known mode:           Patched:
Active width: 1152                    Active width: 1152
Active height: 864		      Active height: 864
Total width: 1600		      Total width: 1600
Total height: 900		      Total height: 900
Frame format: progressive	      Frame format: progressive
Polarities: +vsync +hsync	      Polarities: -vsync -hsync
Pixelclock: 108000000 Hz (75.00 FPS)  Pixelclock: 107998200 Hz (75.00 FPS)
Horizontal frontporch: 64	      Horizontal frontporch: 64
Horizontal sync: 128		      Horizontal sync: 128
Horizontal backporch: 256	      Horizontal backporch: 256
Vertical frontporch: 1		      Vertical frontporch: 1
Vertical sync: 3		      Vertical sync: 3
Vertical backporch: 32		      Vertical backporch: 32
Standards: DMT			      Standards:
Flags:				      Flags:

Original code - "unknown" mode:       Patched:
Active width: 0                       Active width: 1400
Active height: 0		      Active height: 1050
Total width: 0			      Total width: 1560
Total height: 0			      Total height: 1080
Frame format: progressive	      Frame format: progressive
Polarities: -vsync -hsync	      Polarities: -vsync -hsync
Pixelclock: 0 Hz		      Pixelclock: 79998593 Hz (47.48 FPS)
Horizontal frontporch: 0	      Horizontal frontporch: 48
Horizontal sync: 0		      Horizontal sync: 32
Horizontal backporch: 0		      Horizontal backporch: 80
Vertical frontporch: 0		      Vertical frontporch: 3
Vertical sync: 0		      Vertical sync: 4
Vertical backporch: 0		      Vertical backporch: 23
Standards:			      Standards:
Flags:				      Flags:

In the kernel logs - original code:
Signal Timings: 360006/399/31
Detected format: 1152x864p75.00 (1600x900)

Signal Timings: 568630/525/9
no resolution match for timings: 568630/525/9

Patched:
Signal Timings: 360006/399/31
Geometry: 1600 1152 64 128 256   900 864 1 0 3 32 0
Detected format: 1152x864p74.99 (1600x900)

Signal Timings: 568646/525/9
Geometry: 1560 1400 48 32 80   1080 1050 3 0 4 23 0
Detected format: 1400x1050p47.48 (1560x1080)

Note the differences. The original code uses values in
v4l2_dv_timings_presets[], while with the patch it reads the data from
the chip. Thus 108000000 Hz changes into (this time) 107998200 Hz.

Also, the flags. This is HDMI receiver, so I don't exactly think H and V
sync signals have any polarity (they are symbols embedded in the HDMI
stream).
I don't know if the "VESA Discrete Monitor Timings" (DMT) flag makes any
difference.

The code assumes the V sync signal has the same duration for both
fields. While this may be not necessarily true, I can't see any way to
get the "other" V sync width. This is most probably harmless.

I have checked the register values in interlaced mode, but currently
can't test such a setup (I only have remote access to a device working
in interlaced mode). Perhaps this will change in time.

All tests have been performed on Gateworks' Ventana GW54xx board, with
a TDA19971 chip.

Signed-off-by: Krzysztof Hałasa <khalasa@piap.pl>
---
... also testing UTF-8 path :-)

Krzysztof "Chris" Hałasa

Sieć Badawcza Łukasiewicz
Przemysłowy Instytut Automatyki i Pomiarów PIAP
Al. Jerozolimskie 202, 02-486 Warszawa

Comments

Hans Verkuil July 21, 2021, 7:12 a.m. UTC | #1
Hi Krzysztof,


On 19/07/2021 11:30, Krzysztof Hałasa wrote:
> The TDA1997x (HDMI receiver) driver currently uses a specific video

> format detection scheme. The frame (or field in interlaced mode), line

> and HSync pulse durations are compared to those of known, standard video

> modes. If a match is found, the mode is assumed to be detected,

> otherwise -ERANGE is returned (then possibly ignored). This means that:

> - another mode with similar timings will be detected incorrectly

>   (e.g. 2x faster clock and lines twice as long)

> - non-standard modes will not work.

> 

> I propose to replace this scheme with a direct read of geometry

> registers. This way all modes recognized by the chip will be supported.

> 

> How does this change look like from the user's perspective?

> 

> v4l2-ctl -d $(media-ctl -e "tda19971 2-0048") --query-dv-timings

> 

> Original code - known mode:           Patched:

> Active width: 1152                    Active width: 1152

> Active height: 864		      Active height: 864

> Total width: 1600		      Total width: 1600

> Total height: 900		      Total height: 900

> Frame format: progressive	      Frame format: progressive

> Polarities: +vsync +hsync	      Polarities: -vsync -hsync

> Pixelclock: 108000000 Hz (75.00 FPS)  Pixelclock: 107998200 Hz (75.00 FPS)

> Horizontal frontporch: 64	      Horizontal frontporch: 64

> Horizontal sync: 128		      Horizontal sync: 128

> Horizontal backporch: 256	      Horizontal backporch: 256

> Vertical frontporch: 1		      Vertical frontporch: 1

> Vertical sync: 3		      Vertical sync: 3

> Vertical backporch: 32		      Vertical backporch: 32

> Standards: DMT			      Standards:

> Flags:				      Flags:

> 

> Original code - "unknown" mode:       Patched:

> Active width: 0                       Active width: 1400

> Active height: 0		      Active height: 1050

> Total width: 0			      Total width: 1560

> Total height: 0			      Total height: 1080

> Frame format: progressive	      Frame format: progressive

> Polarities: -vsync -hsync	      Polarities: -vsync -hsync

> Pixelclock: 0 Hz		      Pixelclock: 79998593 Hz (47.48 FPS)

> Horizontal frontporch: 0	      Horizontal frontporch: 48

> Horizontal sync: 0		      Horizontal sync: 32

> Horizontal backporch: 0		      Horizontal backporch: 80

> Vertical frontporch: 0		      Vertical frontporch: 3

> Vertical sync: 0		      Vertical sync: 4

> Vertical backporch: 0		      Vertical backporch: 23

> Standards:			      Standards:

> Flags:				      Flags:

> 

> In the kernel logs - original code:

> Signal Timings: 360006/399/31

> Detected format: 1152x864p75.00 (1600x900)

> 

> Signal Timings: 568630/525/9

> no resolution match for timings: 568630/525/9

> 

> Patched:

> Signal Timings: 360006/399/31

> Geometry: 1600 1152 64 128 256   900 864 1 0 3 32 0

> Detected format: 1152x864p74.99 (1600x900)

> 

> Signal Timings: 568646/525/9

> Geometry: 1560 1400 48 32 80   1080 1050 3 0 4 23 0

> Detected format: 1400x1050p47.48 (1560x1080)

> 

> Note the differences. The original code uses values in

> v4l2_dv_timings_presets[], while with the patch it reads the data from

> the chip. Thus 108000000 Hz changes into (this time) 107998200 Hz.

> 

> Also, the flags. This is HDMI receiver, so I don't exactly think H and V

> sync signals have any polarity (they are symbols embedded in the HDMI

> stream).

> I don't know if the "VESA Discrete Monitor Timings" (DMT) flag makes any

> difference.

> 

> The code assumes the V sync signal has the same duration for both

> fields. While this may be not necessarily true, I can't see any way to

> get the "other" V sync width. This is most probably harmless.

> 

> I have checked the register values in interlaced mode, but currently

> can't test such a setup (I only have remote access to a device working

> in interlaced mode). Perhaps this will change in time.

> 

> All tests have been performed on Gateworks' Ventana GW54xx board, with

> a TDA19971 chip.

> 

> Signed-off-by: Krzysztof Hałasa <khalasa@piap.pl>

> ---

> ... also testing UTF-8 path :-)

> 

> Krzysztof "Chris" Hałasa

> 

> Sieć Badawcza Łukasiewicz

> Przemysłowy Instytut Automatyki i Pomiarów PIAP

> Al. Jerozolimskie 202, 02-486 Warszawa

> 

> --- a/drivers/media/i2c/tda1997x.c

> +++ b/drivers/media/i2c/tda1997x.c

> @@ -1092,66 +1094,71 @@ tda1997x_detect_std(struct tda1997x_state *state,

>  		    struct v4l2_dv_timings *timings)

>  {

>  	struct v4l2_subdev *sd = &state->sd;

> -	u32 vper;

> -	u16 hper;

> -	u16 hsper;

> -	int i;

>  

>  	/*

>  	 * Read the FMT registers

> -	 *   REG_V_PER: Period of a frame (or two fields) in MCLK(27MHz) cycles

> -	 *   REG_H_PER: Period of a line in MCLK(27MHz) cycles

> -	 *   REG_HS_WIDTH: Period of horiz sync pulse in MCLK(27MHz) cycles

> +	 *   REG_V_PER: Period of a frame (or field) in MCLK (27MHz) cycles

> +	 *   REG_H_PER: Period of a line in MCLK (27MHz) cycles

> +	 *   REG_HS_WIDTH: Period of horiz sync pulse in MCLK (27MHz) cycles

>  	 */

> -	vper = io_read24(sd, REG_V_PER) & MASK_VPER;

> -	hper = io_read16(sd, REG_H_PER) & MASK_HPER;

> -	hsper = io_read16(sd, REG_HS_WIDTH) & MASK_HSWIDTH;

> +	u32 vper = io_read24(sd, REG_V_PER) & MASK_VPER;

> +	u16 hper = io_read16(sd, REG_H_PER) & MASK_HPER;

> +	u16 hswidth = io_read16(sd, REG_HS_WIDTH);

> +	u16 hsper = hswidth & MASK_HSWIDTH;

> +	u16 htot, hact, hfront, hsync, hback;

> +	u16 vtot, vact, vfront1, vfront2, vsync, vback1, vback2;

> +

>  	v4l2_dbg(1, debug, sd, "Signal Timings: %u/%u/%u\n", vper, hper, hsper);

>  	if (!vper || !hper || !hsper)

>  		return -ENOLINK;

>  

> -	for (i = 0; v4l2_dv_timings_presets[i].bt.width; i++) {

> -		const struct v4l2_bt_timings *bt;

> -		u32 lines, width, _hper, _hsper;

> -		u32 vmin, vmax, hmin, hmax, hsmin, hsmax;

> -		bool vmatch, hmatch, hsmatch;

> -

> -		bt = &v4l2_dv_timings_presets[i].bt;

> -		width = V4L2_DV_BT_FRAME_WIDTH(bt);

> -		lines = V4L2_DV_BT_FRAME_HEIGHT(bt);

> -		_hper = (u32)bt->pixelclock / width;

> -		if (bt->interlaced)

> -			lines /= 2;

> -		/* vper +/- 0.7% */

> -		vmin = ((27000000 / 1000) * 993) / _hper * lines;

> -		vmax = ((27000000 / 1000) * 1007) / _hper * lines;

> -		/* hper +/- 1.0% */

> -		hmin = ((27000000 / 100) * 99) / _hper;

> -		hmax = ((27000000 / 100) * 101) / _hper;

> -		/* hsper +/- 2 (take care to avoid 32bit overflow) */

> -		_hsper = 27000 * bt->hsync / ((u32)bt->pixelclock/1000);

> -		hsmin = _hsper - 2;

> -		hsmax = _hsper + 2;

> -

> -		/* vmatch matches the framerate */

> -		vmatch = ((vper <= vmax) && (vper >= vmin)) ? 1 : 0;

> -		/* hmatch matches the width */

> -		hmatch = ((hper <= hmax) && (hper >= hmin)) ? 1 : 0;

> -		/* hsmatch matches the hswidth */

> -		hsmatch = ((hsper <= hsmax) && (hsper >= hsmin)) ? 1 : 0;

> -		if (hmatch && vmatch && hsmatch) {

> -			v4l2_print_dv_timings(sd->name, "Detected format: ",

> -					      &v4l2_dv_timings_presets[i],

> -					      false);

> -			if (timings)

> -				*timings = v4l2_dv_timings_presets[i];

> -			return 0;

> -		}

> +	htot = io_read16(sd, REG_FMT_H_TOT);

> +	hact = io_read16(sd, REG_FMT_H_ACT);

> +	hfront = io_read16(sd, REG_FMT_H_FRONT);

> +	hsync = io_read16(sd, REG_FMT_H_SYNC);

> +	hback = io_read16(sd, REG_FMT_H_BACK);

> +

> +	vtot = io_read16(sd, REG_FMT_V_TOT);

> +	vact = io_read16(sd, REG_FMT_V_ACT);

> +	vfront1 = io_read(sd, REG_FMT_V_FRONT_F1);

> +	vfront2 = io_read(sd, REG_FMT_V_FRONT_F2);

> +	vsync = io_read(sd, REG_FMT_V_SYNC);

> +	vback1 = io_read(sd, REG_FMT_V_BACK_F1);

> +	vback2 = io_read(sd, REG_FMT_V_BACK_F2);

> +

> +	v4l2_dbg(1, debug, sd, "Geometry: %u %u %u %u %u   %u %u %u %u %u %u %u\n",

> +		 htot, hact, hfront, hsync, hback,

> +		 vtot, vact, vfront1, vfront2, vsync, vback1, vback2);

> +

> +	if (!timings)

> +		return 0;


This check isn't necessary, timings is never NULL.

> +

> +	timings->type = V4L2_DV_BT_656_1120;

> +	timings->bt.width = hact;

> +	timings->bt.hfrontporch = hfront;

> +	timings->bt.hsync = hsync;

> +	timings->bt.hbackporch = hback;

> +	timings->bt.height = vact;

> +	timings->bt.vfrontporch = vfront1;

> +	timings->bt.vsync = vsync;

> +	timings->bt.vbackporch = vback1;

> +	timings->bt.interlaced = hswidth & MASK_HSWIDTH_INTERLACED ?

> +		V4L2_DV_INTERLACED : V4L2_DV_PROGRESSIVE;

> +

> +	timings->bt.pixelclock = (u64)htot * vtot * 27000000;

> +	if (timings->bt.interlaced == V4L2_DV_INTERLACED) {

> +		timings->bt.il_vfrontporch = vfront2;

> +		timings->bt.il_vsync = timings->bt.vsync;

> +		timings->bt.il_vbackporch = vback2;

> +		do_div(timings->bt.pixelclock, vper * 2 /* full frame */);

> +	} else {

> +		timings->bt.il_vfrontporch = 0;

> +		timings->bt.il_vsync = 0;

> +		timings->bt.il_vbackporch = 0;

> +		do_div(timings->bt.pixelclock, vper);

>  	}

> -

> -	v4l_err(state->client, "no resolution match for timings: %d/%d/%d\n",

> -		vper, hper, hsper);

> -	return -ERANGE;


This is all very good. But one more step is needed:

	v4l2_find_dv_timings_cap(timings, &tda1997x_dv_timings_cap,
				 timings->bt.pixelclock / 1000,
				 NULL, NULL);

This will find matching timings in the presets as long as they don't differ
by more that 1/1000th of the pixelclock (to allow for natural variations since
clocks are never precise). If found, it will update the timing to the of the
preset. It's really what you want, since knowing that a timing is a CEA timing
or DMT etc. can be useful for userspace.

If there is no match, then the measured timings are just passed on to userspace.

Regards,

	Hans

> +	v4l2_print_dv_timings(sd->name, "Detected format: ", timings, false);

> +	return 0;

>  }

>  

>  /* some sort of errata workaround for chip revision 0 (N1) */

> --- a/drivers/media/i2c/tda1997x_regs.h

> +++ b/drivers/media/i2c/tda1997x_regs.h

> @@ -120,6 +120,7 @@

>  #define MASK_VHREF		0x3fff

>  #define MASK_HPER		0x0fff

>  #define MASK_HSWIDTH		0x03ff

> +#define MASK_HSWIDTH_INTERLACED	0x8000

>  

>  /* HPD Detection */

>  #define DETECT_UTIL		BIT(7)	/* utility of HDMI level */

>
Krzysztof Hałasa July 21, 2021, 7:56 a.m. UTC | #2
Hi Hans,

Hans Verkuil <hverkuil@xs4all.nl> writes:

>> --- a/drivers/media/i2c/tda1997x.c

>> +++ b/drivers/media/i2c/tda1997x.c

>> @@ -1092,66 +1094,71 @@ tda1997x_detect_std(struct tda1997x_state *state,

>>  		    struct v4l2_dv_timings *timings)


...

>> +	if (!timings)

>> +		return 0;

>

> This check isn't necessary, timings is never NULL.


Well, the tda1997x_irq_sus() does this:

	if (debug)
		tda1997x_detect_std(state, NULL);

Perhaps there is a better way, but I think I will leave it for now.
Also there is the issue of ignoring tda1997x_detect_std() return value,
but I can now see it's only a case in tda1997x_query_dv_timings(), easy
to fix.

Will post an update shortly.

Thanks for your comments,
-- 
Krzysztof "Chris" Hałasa

Sieć Badawcza Łukasiewicz
Przemysłowy Instytut Automatyki i Pomiarów PIAP
Al. Jerozolimskie 202, 02-486 Warszawa
Hans Verkuil July 21, 2021, 10:04 a.m. UTC | #3
On 21/07/2021 09:56, Krzysztof Hałasa wrote:
> Hi Hans,

> 

> Hans Verkuil <hverkuil@xs4all.nl> writes:

> 

>>> --- a/drivers/media/i2c/tda1997x.c

>>> +++ b/drivers/media/i2c/tda1997x.c

>>> @@ -1092,66 +1094,71 @@ tda1997x_detect_std(struct tda1997x_state *state,

>>>  		    struct v4l2_dv_timings *timings)

> 

> ...

> 

>>> +	if (!timings)

>>> +		return 0;

>>

>> This check isn't necessary, timings is never NULL.

> 

> Well, the tda1997x_irq_sus() does this:

> 

> 	if (debug)

> 		tda1997x_detect_std(state, NULL);


Ah, I missed that. Then you can keep that check.

Regards,

	Hans

> 

> Perhaps there is a better way, but I think I will leave it for now.

> Also there is the issue of ignoring tda1997x_detect_std() return value,

> but I can now see it's only a case in tda1997x_query_dv_timings(), easy

> to fix.

> 

> Will post an update shortly.

> 

> Thanks for your comments,

>
Krzysztof Hałasa July 22, 2021, 6:27 a.m. UTC | #4
Hans,

I have a problem with matching H/V sync polarities. It appears the
standard modes use different polarity settings, and HDMI doesn't have
separate sync lines, it uses in-band signaling. I don't know what
polarity should I set for the v4l2_find_dv_timings_cap() ->
v4l2_match_dv_timings() to succeed.

1. I can try all 4 combinations (+/+, +/-, -/+, -/-) and see if there is
a match. Seems weird to me.

2. I can add a flag to v4l2_find_dv_timings_cap() ->
v4l2_match_dv_timings() which asks for the polarities to be ignored.

3. I could add
 #define V4L2_DV_VSYNC_POS_POL   0x00000001
 #define V4L2_DV_HSYNC_POS_POL   0x00000002
+#define V4L2_DV_VSYNC_ANY_POL   0x00000004
+#define V4L2_DV_HSYNC_ANY_POL   0x00000008

and v4l2_match_dv_timings() would skip comparing them. A bit hacky.

4. I could also add
 #define V4L2_DV_VSYNC_POS_POL   0x00000001
 #define V4L2_DV_HSYNC_POS_POL   0x00000002
+#define V4L2_DV_VSYNC_NEG_POL   0x00000004
+#define V4L2_DV_HSYNC_NEG_POL   0x00000008

and, likewise, v4l2_match_dv_timings() would know what to do. This is
IMHO cleaner but requires much more changes in other code (like
drivers).

Any other idea?
Perhaps I should do "2"?

Thanks,
-- 
Krzysztof "Chris" Hałasa

Sieć Badawcza Łukasiewicz
Przemysłowy Instytut Automatyki i Pomiarów PIAP
Al. Jerozolimskie 202, 02-486 Warszawa
Krzysztof Hałasa July 22, 2021, 6:43 a.m. UTC | #5
Also, I have spotted an interesting loop condition in
drivers/media/v4l2-core/v4l2-dv-timings.c (dated 2013,
d1c65ad6a44b0ff79d2f0bf726fa6fd9248991f4).
It obviously works (unless the table grows to 600+ entries), but I guess
I should make it "standard", shouldn't I?

The first case is pretty normal:
@@ -159,10 +161,10 @@ int v4l2_enum_dv_timings_cap(struct v4l2_enum_dv_timings *t,
     u32 i, idx;
 
     memset(t->reserved, 0, sizeof(t->reserved));
-    for (i = idx = 0; i < ARRAY_SIZE(timings); i++) {
-        if (v4l2_dv_valid_timings(timings + i, cap) &&
+    for (i = idx = 0; v4l2_dv_timings_presets[i].bt.width; i++) {
+        if (v4l2_dv_valid_timings(v4l2_dv_timings_presets + i, cap) &&
             idx++ == t->index) {
-            t->timings = timings[i];
+            t->timings = v4l2_dv_timings_presets[i];
             return 0;
         }
     }

This is the interesting part:
@@ -179,10 +181,10 @@ bool v4l2_find_dv_timings_cap(struct v4l2_dv_timings *t,
     if (!v4l2_dv_valid_timings(t, cap))
         return false;
 
-    for (i = 0; i < ARRAY_SIZE(timings); i++) {
-        if (v4l2_dv_valid_timings(timings + i, cap) &&
-            v4l2_match_dv_timings(t, timings + i, pclock_delta)) {
-            *t = timings[i];
+    for (i = 0; i < v4l2_dv_timings_presets[i].bt.width; i++) {
                 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+        if (v4l2_dv_valid_timings(v4l2_dv_timings_presets + i, cap) &&
+            v4l2_match_dv_timings(t, v4l2_dv_timings_presets + i, pclock_delta)) {
+            *t = v4l2_dv_timings_presets[i];
             return true;
         }
     }

-- 
Krzysztof "Chris" Hałasa

Sieć Badawcza Łukasiewicz
Przemysłowy Instytut Automatyki i Pomiarów PIAP
Al. Jerozolimskie 202, 02-486 Warszawa
Hans Verkuil July 23, 2021, 8:24 a.m. UTC | #6
Hi Krzysztof,

On 22/07/2021 08:43, Krzysztof Hałasa wrote:
> Also, I have spotted an interesting loop condition in

> drivers/media/v4l2-core/v4l2-dv-timings.c (dated 2013,

> d1c65ad6a44b0ff79d2f0bf726fa6fd9248991f4).

> It obviously works (unless the table grows to 600+ entries), but I guess

> I should make it "standard", shouldn't I?


Thanks for catching this!

I've posted a patch fixing it. Luckily it never broke anything, but it definitely
had to be fixed.

Regards,

	Hans

> 

> The first case is pretty normal:

> @@ -159,10 +161,10 @@ int v4l2_enum_dv_timings_cap(struct v4l2_enum_dv_timings *t,

>      u32 i, idx;

>  

>      memset(t->reserved, 0, sizeof(t->reserved));

> -    for (i = idx = 0; i < ARRAY_SIZE(timings); i++) {

> -        if (v4l2_dv_valid_timings(timings + i, cap) &&

> +    for (i = idx = 0; v4l2_dv_timings_presets[i].bt.width; i++) {

> +        if (v4l2_dv_valid_timings(v4l2_dv_timings_presets + i, cap) &&

>              idx++ == t->index) {

> -            t->timings = timings[i];

> +            t->timings = v4l2_dv_timings_presets[i];

>              return 0;

>          }

>      }

> 

> This is the interesting part:

> @@ -179,10 +181,10 @@ bool v4l2_find_dv_timings_cap(struct v4l2_dv_timings *t,

>      if (!v4l2_dv_valid_timings(t, cap))

>          return false;

>  

> -    for (i = 0; i < ARRAY_SIZE(timings); i++) {

> -        if (v4l2_dv_valid_timings(timings + i, cap) &&

> -            v4l2_match_dv_timings(t, timings + i, pclock_delta)) {

> -            *t = timings[i];

> +    for (i = 0; i < v4l2_dv_timings_presets[i].bt.width; i++) {

>                  ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

> +        if (v4l2_dv_valid_timings(v4l2_dv_timings_presets + i, cap) &&

> +            v4l2_match_dv_timings(t, v4l2_dv_timings_presets + i, pclock_delta)) {

> +            *t = v4l2_dv_timings_presets[i];

>              return true;

>          }

>      }

>
Hans Verkuil July 23, 2021, 8:49 a.m. UTC | #7
On 22/07/2021 08:27, Krzysztof Hałasa wrote:
> Hans,

> 

> I have a problem with matching H/V sync polarities. It appears the

> standard modes use different polarity settings, and HDMI doesn't have

> separate sync lines, it uses in-band signaling. I don't know what

> polarity should I set for the v4l2_find_dv_timings_cap() ->

> v4l2_match_dv_timings() to succeed.


The tda1997x doesn't have any method of obtaining the sync polarities?

HSync and VSync are, as you say, using in-band signaling, but the 'polarity'
still matters when it comes to detecting timings. All HDMI receivers I have
ever seen have a register that reports the polarities. I find it *very* hard
to imagine that this device doesn't support that.

Do you have a datasheet for this device?

> 

> 1. I can try all 4 combinations (+/+, +/-, -/+, -/-) and see if there is

> a match. Seems weird to me.

> 

> 2. I can add a flag to v4l2_find_dv_timings_cap() ->

> v4l2_match_dv_timings() which asks for the polarities to be ignored.

> 

> 3. I could add

>  #define V4L2_DV_VSYNC_POS_POL   0x00000001

>  #define V4L2_DV_HSYNC_POS_POL   0x00000002

> +#define V4L2_DV_VSYNC_ANY_POL   0x00000004

> +#define V4L2_DV_HSYNC_ANY_POL   0x00000008

> 

> and v4l2_match_dv_timings() would skip comparing them. A bit hacky.

> 

> 4. I could also add

>  #define V4L2_DV_VSYNC_POS_POL   0x00000001

>  #define V4L2_DV_HSYNC_POS_POL   0x00000002

> +#define V4L2_DV_VSYNC_NEG_POL   0x00000004

> +#define V4L2_DV_HSYNC_NEG_POL   0x00000008

> 

> and, likewise, v4l2_match_dv_timings() would know what to do. This is

> IMHO cleaner but requires much more changes in other code (like

> drivers).

> 

> Any other idea?

> Perhaps I should do "2"?


If there really is no register that reports the polarities, then option 2 is
the best approach. There are other reasons why you might want to ignore the
polarities: not all video sources use the correct polarities.

What I think would be best is to make a flags argument that allows the
match code to skip certain tests:

#define V4L2_DV_TIM_IGNORE_POLARITIES	BIT(0)
#define V4L2_DV_TIM_IGNORE_HPORCHES	BIT(1)
#define V4L2_DV_TIM_IGNORE_VPORCHES	BIT(2)
#define V4L2_DV_TIM_IGNORE_HSYNC	BIT(3)
#define V4L2_DV_TIM_IGNORE_VSYNC	BIT(4)

IGNORE_HPORCHES would just check hsync and total horizontal blanking,
IGNORE_HSYNC would just check total horizontal blanking.
IGNORE_HSYNC implies IGNORE_HPORCHES.

Regards,

	Hans
Krzysztof Hałasa July 23, 2021, 11:57 a.m. UTC | #8
Hi Hans,

Hans Verkuil <hverkuil@xs4all.nl> writes:

> The tda1997x doesn't have any method of obtaining the sync polarities?

>

> HSync and VSync are, as you say, using in-band signaling, but the 'polarity'

> still matters when it comes to detecting timings. All HDMI receivers I have

> ever seen have a register that reports the polarities. I find it *very* hard

> to imagine that this device doesn't support that.


I didn't know that HDMI serializers send the logical levels of those
lines, I thought some frame/field/line start/end codes are sent instead.
I will have to have a closer look at this chip, then.

> Do you have a datasheet for this device?


No, there is only the current driver and also the old one (in Gateworks'
Freescale/NXP kernel). Tim may have something perhaps under NDA, maaybe.

I will see if I can find these bits in the registers.

Thanks.
-- 
Krzysztof "Chris" Hałasa

Sieć Badawcza Łukasiewicz
Przemysłowy Instytut Automatyki i Pomiarów PIAP
Al. Jerozolimskie 202, 02-486 Warszawa
diff mbox series

Patch

--- a/drivers/media/i2c/tda1997x.c
+++ b/drivers/media/i2c/tda1997x.c
@@ -1092,66 +1094,71 @@  tda1997x_detect_std(struct tda1997x_state *state,
 		    struct v4l2_dv_timings *timings)
 {
 	struct v4l2_subdev *sd = &state->sd;
-	u32 vper;
-	u16 hper;
-	u16 hsper;
-	int i;
 
 	/*
 	 * Read the FMT registers
-	 *   REG_V_PER: Period of a frame (or two fields) in MCLK(27MHz) cycles
-	 *   REG_H_PER: Period of a line in MCLK(27MHz) cycles
-	 *   REG_HS_WIDTH: Period of horiz sync pulse in MCLK(27MHz) cycles
+	 *   REG_V_PER: Period of a frame (or field) in MCLK (27MHz) cycles
+	 *   REG_H_PER: Period of a line in MCLK (27MHz) cycles
+	 *   REG_HS_WIDTH: Period of horiz sync pulse in MCLK (27MHz) cycles
 	 */
-	vper = io_read24(sd, REG_V_PER) & MASK_VPER;
-	hper = io_read16(sd, REG_H_PER) & MASK_HPER;
-	hsper = io_read16(sd, REG_HS_WIDTH) & MASK_HSWIDTH;
+	u32 vper = io_read24(sd, REG_V_PER) & MASK_VPER;
+	u16 hper = io_read16(sd, REG_H_PER) & MASK_HPER;
+	u16 hswidth = io_read16(sd, REG_HS_WIDTH);
+	u16 hsper = hswidth & MASK_HSWIDTH;
+	u16 htot, hact, hfront, hsync, hback;
+	u16 vtot, vact, vfront1, vfront2, vsync, vback1, vback2;
+
 	v4l2_dbg(1, debug, sd, "Signal Timings: %u/%u/%u\n", vper, hper, hsper);
 	if (!vper || !hper || !hsper)
 		return -ENOLINK;
 
-	for (i = 0; v4l2_dv_timings_presets[i].bt.width; i++) {
-		const struct v4l2_bt_timings *bt;
-		u32 lines, width, _hper, _hsper;
-		u32 vmin, vmax, hmin, hmax, hsmin, hsmax;
-		bool vmatch, hmatch, hsmatch;
-
-		bt = &v4l2_dv_timings_presets[i].bt;
-		width = V4L2_DV_BT_FRAME_WIDTH(bt);
-		lines = V4L2_DV_BT_FRAME_HEIGHT(bt);
-		_hper = (u32)bt->pixelclock / width;
-		if (bt->interlaced)
-			lines /= 2;
-		/* vper +/- 0.7% */
-		vmin = ((27000000 / 1000) * 993) / _hper * lines;
-		vmax = ((27000000 / 1000) * 1007) / _hper * lines;
-		/* hper +/- 1.0% */
-		hmin = ((27000000 / 100) * 99) / _hper;
-		hmax = ((27000000 / 100) * 101) / _hper;
-		/* hsper +/- 2 (take care to avoid 32bit overflow) */
-		_hsper = 27000 * bt->hsync / ((u32)bt->pixelclock/1000);
-		hsmin = _hsper - 2;
-		hsmax = _hsper + 2;
-
-		/* vmatch matches the framerate */
-		vmatch = ((vper <= vmax) && (vper >= vmin)) ? 1 : 0;
-		/* hmatch matches the width */
-		hmatch = ((hper <= hmax) && (hper >= hmin)) ? 1 : 0;
-		/* hsmatch matches the hswidth */
-		hsmatch = ((hsper <= hsmax) && (hsper >= hsmin)) ? 1 : 0;
-		if (hmatch && vmatch && hsmatch) {
-			v4l2_print_dv_timings(sd->name, "Detected format: ",
-					      &v4l2_dv_timings_presets[i],
-					      false);
-			if (timings)
-				*timings = v4l2_dv_timings_presets[i];
-			return 0;
-		}
+	htot = io_read16(sd, REG_FMT_H_TOT);
+	hact = io_read16(sd, REG_FMT_H_ACT);
+	hfront = io_read16(sd, REG_FMT_H_FRONT);
+	hsync = io_read16(sd, REG_FMT_H_SYNC);
+	hback = io_read16(sd, REG_FMT_H_BACK);
+
+	vtot = io_read16(sd, REG_FMT_V_TOT);
+	vact = io_read16(sd, REG_FMT_V_ACT);
+	vfront1 = io_read(sd, REG_FMT_V_FRONT_F1);
+	vfront2 = io_read(sd, REG_FMT_V_FRONT_F2);
+	vsync = io_read(sd, REG_FMT_V_SYNC);
+	vback1 = io_read(sd, REG_FMT_V_BACK_F1);
+	vback2 = io_read(sd, REG_FMT_V_BACK_F2);
+
+	v4l2_dbg(1, debug, sd, "Geometry: %u %u %u %u %u   %u %u %u %u %u %u %u\n",
+		 htot, hact, hfront, hsync, hback,
+		 vtot, vact, vfront1, vfront2, vsync, vback1, vback2);
+
+	if (!timings)
+		return 0;
+
+	timings->type = V4L2_DV_BT_656_1120;
+	timings->bt.width = hact;
+	timings->bt.hfrontporch = hfront;
+	timings->bt.hsync = hsync;
+	timings->bt.hbackporch = hback;
+	timings->bt.height = vact;
+	timings->bt.vfrontporch = vfront1;
+	timings->bt.vsync = vsync;
+	timings->bt.vbackporch = vback1;
+	timings->bt.interlaced = hswidth & MASK_HSWIDTH_INTERLACED ?
+		V4L2_DV_INTERLACED : V4L2_DV_PROGRESSIVE;
+
+	timings->bt.pixelclock = (u64)htot * vtot * 27000000;
+	if (timings->bt.interlaced == V4L2_DV_INTERLACED) {
+		timings->bt.il_vfrontporch = vfront2;
+		timings->bt.il_vsync = timings->bt.vsync;
+		timings->bt.il_vbackporch = vback2;
+		do_div(timings->bt.pixelclock, vper * 2 /* full frame */);
+	} else {
+		timings->bt.il_vfrontporch = 0;
+		timings->bt.il_vsync = 0;
+		timings->bt.il_vbackporch = 0;
+		do_div(timings->bt.pixelclock, vper);
 	}
-
-	v4l_err(state->client, "no resolution match for timings: %d/%d/%d\n",
-		vper, hper, hsper);
-	return -ERANGE;
+	v4l2_print_dv_timings(sd->name, "Detected format: ", timings, false);
+	return 0;
 }
 
 /* some sort of errata workaround for chip revision 0 (N1) */
--- a/drivers/media/i2c/tda1997x_regs.h
+++ b/drivers/media/i2c/tda1997x_regs.h
@@ -120,6 +120,7 @@ 
 #define MASK_VHREF		0x3fff
 #define MASK_HPER		0x0fff
 #define MASK_HSWIDTH		0x03ff
+#define MASK_HSWIDTH_INTERLACED	0x8000
 
 /* HPD Detection */
 #define DETECT_UTIL		BIT(7)	/* utility of HDMI level */