linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 0/4] Correct timing report
@ 2021-12-22  8:21 Jammy Huang
  2021-12-22  8:21 ` [PATCH v2 1/4] media: aspeed: Correct value for h-total-pixels Jammy Huang
                   ` (3 more replies)
  0 siblings, 4 replies; 12+ messages in thread
From: Jammy Huang @ 2021-12-22  8:21 UTC (permalink / raw)
  To: eajames, mchehab, joel, andrew, linux-media, openbmc,
	linux-arm-kernel, linux-aspeed, linux-kernel

This series will correct the value of timing detected.

Changes in v2:
 - code refined to improve readability

Jammy Huang (4):
  media: aspeed: Correct value for h-total-pixels
  media: aspeed: Use FIELD_GET to improve readability
  media: aspeed: Correct values for detected timing
  media: aspeed: Fix timing polarity incorrect

 drivers/media/platform/aspeed-video.c | 78 ++++++++++++++++++---------
 1 file changed, 54 insertions(+), 24 deletions(-)

-- 
2.25.1


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

* [PATCH v2 1/4] media: aspeed: Correct value for h-total-pixels
  2021-12-22  8:21 [PATCH 0/4] Correct timing report Jammy Huang
@ 2021-12-22  8:21 ` Jammy Huang
  2021-12-22  8:21 ` [PATCH v2 2/4] media: aspeed: Use FIELD_GET to improve readability Jammy Huang
                   ` (2 subsequent siblings)
  3 siblings, 0 replies; 12+ messages in thread
From: Jammy Huang @ 2021-12-22  8:21 UTC (permalink / raw)
  To: eajames, mchehab, joel, andrew, linux-media, openbmc,
	linux-arm-kernel, linux-aspeed, linux-kernel

Previous reg-field, 0x98[11:0], stands for the period of the detected
hsync signal.
Use the correct reg, 0xa0, to get h-total in pixels.

Fixes: d2b4387f3bdf ("media: platform: Add Aspeed Video Engine driver")
Signed-off-by: Jammy Huang <jammy_huang@aspeedtech.com>
Reviewed-by: Joel Stanley <joel@jms.id.au>
---
v2:
 - no update
---
 drivers/media/platform/aspeed-video.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/drivers/media/platform/aspeed-video.c b/drivers/media/platform/aspeed-video.c
index b388bc56ce81..d5f77b205175 100644
--- a/drivers/media/platform/aspeed-video.c
+++ b/drivers/media/platform/aspeed-video.c
@@ -166,7 +166,7 @@
 #define  VE_SRC_TB_EDGE_DET_BOT		GENMASK(28, VE_SRC_TB_EDGE_DET_BOT_SHF)
 
 #define VE_MODE_DETECT_STATUS		0x098
-#define  VE_MODE_DETECT_H_PIXELS	GENMASK(11, 0)
+#define  VE_MODE_DETECT_H_PERIOD	GENMASK(11, 0)
 #define  VE_MODE_DETECT_V_LINES_SHF	16
 #define  VE_MODE_DETECT_V_LINES		GENMASK(27, VE_MODE_DETECT_V_LINES_SHF)
 #define  VE_MODE_DETECT_STATUS_VSYNC	BIT(28)
@@ -177,6 +177,8 @@
 #define  VE_SYNC_STATUS_VSYNC_SHF	16
 #define  VE_SYNC_STATUS_VSYNC		GENMASK(27, VE_SYNC_STATUS_VSYNC_SHF)
 
+#define VE_H_TOTAL_PIXELS		0x0A0
+
 #define VE_INTERRUPT_CTRL		0x304
 #define VE_INTERRUPT_STATUS		0x308
 #define  VE_INTERRUPT_MODE_DETECT_WD	BIT(0)
@@ -938,6 +940,7 @@ static void aspeed_video_get_resolution(struct aspeed_video *video)
 	u32 src_lr_edge;
 	u32 src_tb_edge;
 	u32 sync;
+	u32 htotal;
 	struct v4l2_bt_timings *det = &video->detected_timings;
 
 	det->width = MIN_WIDTH;
@@ -983,6 +986,7 @@ static void aspeed_video_get_resolution(struct aspeed_video *video)
 		src_tb_edge = aspeed_video_read(video, VE_SRC_TB_EDGE_DET);
 		mds = aspeed_video_read(video, VE_MODE_DETECT_STATUS);
 		sync = aspeed_video_read(video, VE_SYNC_STATUS);
+		htotal = aspeed_video_read(video, VE_H_TOTAL_PIXELS);
 
 		video->frame_bottom = (src_tb_edge & VE_SRC_TB_EDGE_DET_BOT) >>
 			VE_SRC_TB_EDGE_DET_BOT_SHF;
@@ -999,8 +1003,7 @@ static void aspeed_video_get_resolution(struct aspeed_video *video)
 			VE_SRC_LR_EDGE_DET_RT_SHF;
 		video->frame_left = src_lr_edge & VE_SRC_LR_EDGE_DET_LEFT;
 		det->hfrontporch = video->frame_left;
-		det->hbackporch = (mds & VE_MODE_DETECT_H_PIXELS) -
-			video->frame_right;
+		det->hbackporch = htotal - video->frame_right;
 		det->hsync = sync & VE_SYNC_STATUS_HSYNC;
 		if (video->frame_left > video->frame_right)
 			continue;
-- 
2.25.1


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

* [PATCH v2 2/4] media: aspeed: Use FIELD_GET to improve readability
  2021-12-22  8:21 [PATCH 0/4] Correct timing report Jammy Huang
  2021-12-22  8:21 ` [PATCH v2 1/4] media: aspeed: Correct value for h-total-pixels Jammy Huang
@ 2021-12-22  8:21 ` Jammy Huang
  2021-12-22  8:21 ` [PATCH v2 3/4] media: aspeed: Correct values for detected timing Jammy Huang
  2021-12-22  8:21 ` [PATCH v2 4/4] media: aspeed: Fix timing polarity incorrect Jammy Huang
  3 siblings, 0 replies; 12+ messages in thread
From: Jammy Huang @ 2021-12-22  8:21 UTC (permalink / raw)
  To: eajames, mchehab, joel, andrew, linux-media, openbmc,
	linux-arm-kernel, linux-aspeed, linux-kernel

Use the bitfield macro FIELD_GET, and GENMASK to do the shift and mask in
one go for reg values.

Signed-off-by: Jammy Huang <jammy_huang@aspeedtech.com>
Reviewed-by: Joel Stanley <joel@jms.id.au>
---
 v2:
  - Put some codes on one line
---
 drivers/media/platform/aspeed-video.c | 31 +++++++++++----------------
 1 file changed, 12 insertions(+), 19 deletions(-)

diff --git a/drivers/media/platform/aspeed-video.c b/drivers/media/platform/aspeed-video.c
index d5f77b205175..c241038ee27c 100644
--- a/drivers/media/platform/aspeed-video.c
+++ b/drivers/media/platform/aspeed-video.c
@@ -156,26 +156,22 @@
 #define  VE_SRC_LR_EDGE_DET_NO_H	BIT(13)
 #define  VE_SRC_LR_EDGE_DET_NO_DISP	BIT(14)
 #define  VE_SRC_LR_EDGE_DET_NO_CLK	BIT(15)
-#define  VE_SRC_LR_EDGE_DET_RT_SHF	16
-#define  VE_SRC_LR_EDGE_DET_RT		GENMASK(27, VE_SRC_LR_EDGE_DET_RT_SHF)
+#define  VE_SRC_LR_EDGE_DET_RT		GENMASK(27, 16)
 #define  VE_SRC_LR_EDGE_DET_INTERLACE	BIT(31)
 
 #define VE_SRC_TB_EDGE_DET		0x094
 #define  VE_SRC_TB_EDGE_DET_TOP		GENMASK(12, 0)
-#define  VE_SRC_TB_EDGE_DET_BOT_SHF	16
-#define  VE_SRC_TB_EDGE_DET_BOT		GENMASK(28, VE_SRC_TB_EDGE_DET_BOT_SHF)
+#define  VE_SRC_TB_EDGE_DET_BOT		GENMASK(28, 16)
 
 #define VE_MODE_DETECT_STATUS		0x098
 #define  VE_MODE_DETECT_H_PERIOD	GENMASK(11, 0)
-#define  VE_MODE_DETECT_V_LINES_SHF	16
-#define  VE_MODE_DETECT_V_LINES		GENMASK(27, VE_MODE_DETECT_V_LINES_SHF)
+#define  VE_MODE_DETECT_V_LINES		GENMASK(27, 16)
 #define  VE_MODE_DETECT_STATUS_VSYNC	BIT(28)
 #define  VE_MODE_DETECT_STATUS_HSYNC	BIT(29)
 
 #define VE_SYNC_STATUS			0x09c
 #define  VE_SYNC_STATUS_HSYNC		GENMASK(11, 0)
-#define  VE_SYNC_STATUS_VSYNC_SHF	16
-#define  VE_SYNC_STATUS_VSYNC		GENMASK(27, VE_SYNC_STATUS_VSYNC_SHF)
+#define  VE_SYNC_STATUS_VSYNC		GENMASK(27, 16)
 
 #define VE_H_TOTAL_PIXELS		0x0A0
 
@@ -988,23 +984,20 @@ static void aspeed_video_get_resolution(struct aspeed_video *video)
 		sync = aspeed_video_read(video, VE_SYNC_STATUS);
 		htotal = aspeed_video_read(video, VE_H_TOTAL_PIXELS);
 
-		video->frame_bottom = (src_tb_edge & VE_SRC_TB_EDGE_DET_BOT) >>
-			VE_SRC_TB_EDGE_DET_BOT_SHF;
-		video->frame_top = src_tb_edge & VE_SRC_TB_EDGE_DET_TOP;
+		video->frame_bottom = FIELD_GET(VE_SRC_TB_EDGE_DET_BOT, src_tb_edge);
+		video->frame_top = FIELD_GET(VE_SRC_TB_EDGE_DET_TOP, src_tb_edge);
 		det->vfrontporch = video->frame_top;
-		det->vbackporch = ((mds & VE_MODE_DETECT_V_LINES) >>
-			VE_MODE_DETECT_V_LINES_SHF) - video->frame_bottom;
-		det->vsync = (sync & VE_SYNC_STATUS_VSYNC) >>
-			VE_SYNC_STATUS_VSYNC_SHF;
+		det->vbackporch = FIELD_GET(VE_MODE_DETECT_V_LINES, mds) -
+			video->frame_bottom;
+		det->vsync = FIELD_GET(VE_SYNC_STATUS_VSYNC, sync);
 		if (video->frame_top > video->frame_bottom)
 			continue;
 
-		video->frame_right = (src_lr_edge & VE_SRC_LR_EDGE_DET_RT) >>
-			VE_SRC_LR_EDGE_DET_RT_SHF;
-		video->frame_left = src_lr_edge & VE_SRC_LR_EDGE_DET_LEFT;
+		video->frame_right = FIELD_GET(VE_SRC_LR_EDGE_DET_RT, src_lr_edge);
+		video->frame_left = FIELD_GET(VE_SRC_LR_EDGE_DET_LEFT, src_lr_edge);
 		det->hfrontporch = video->frame_left;
 		det->hbackporch = htotal - video->frame_right;
-		det->hsync = sync & VE_SYNC_STATUS_HSYNC;
+		det->hsync = FIELD_GET(VE_SYNC_STATUS_HSYNC, sync);
 		if (video->frame_left > video->frame_right)
 			continue;
 
-- 
2.25.1


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

* [PATCH v2 3/4] media: aspeed: Correct values for detected timing
  2021-12-22  8:21 [PATCH 0/4] Correct timing report Jammy Huang
  2021-12-22  8:21 ` [PATCH v2 1/4] media: aspeed: Correct value for h-total-pixels Jammy Huang
  2021-12-22  8:21 ` [PATCH v2 2/4] media: aspeed: Use FIELD_GET to improve readability Jammy Huang
@ 2021-12-22  8:21 ` Jammy Huang
  2022-01-20 12:31   ` Hans Verkuil
  2021-12-22  8:21 ` [PATCH v2 4/4] media: aspeed: Fix timing polarity incorrect Jammy Huang
  3 siblings, 1 reply; 12+ messages in thread
From: Jammy Huang @ 2021-12-22  8:21 UTC (permalink / raw)
  To: eajames, mchehab, joel, andrew, linux-media, openbmc,
	linux-arm-kernel, linux-aspeed, linux-kernel

Correct timing's fp/sync/bp value based on the information below.
It should be noticed that the calculation formula should be changed
per sync polarity.

The sequence of signal: sync - backporch - video data - frontporch

The following registers start counting from sync's rising edge:
1. VR090: frame edge's left and right
2. VR094: frame edge's top and bottom
3. VR09C: counting from sync's rising edge to falling edge

[Vertical timing]
            +--+     +-------------------+     +--+
            |  |     |    v i d e o      |     |  |
         +--+  +-----+                   +-----+  +---+

       vsync+--+
   frame_top+--------+
frame_bottom+----------------------------+

                  +-------------------+
                  |    v i d e o      |
      +--+  +-----+                   +-----+  +---+
         |  |                               |  |
         +--+                               +--+
       vsync+-------------------------------+
   frame_top+-----+
frame_bottom+-------------------------+

[Horizontal timing]
            +--+     +-------------------+     +--+
            |  |     |    v i d e o      |     |  |
         +--+  +-----+                   +-----+  +---+

       hsync+--+
  frame_left+--------+
 frame_right+----------------------------+

                  +-------------------+
                  |    v i d e o      |
      +--+  +-----+                   +-----+  +---+
         |  |                               |  |
         +--+                               +--+
       hsync+-------------------------------+
  frame_left+-----+
 frame_right+-------------------------+

Signed-off-by: Jammy Huang <jammy_huang@aspeedtech.com>
---
 v2:
  - Code refined per Joel's suggestion
  - Update commit message to have name matching variable
---
 drivers/media/platform/aspeed-video.c | 30 ++++++++++++++++++++-------
 1 file changed, 22 insertions(+), 8 deletions(-)

diff --git a/drivers/media/platform/aspeed-video.c b/drivers/media/platform/aspeed-video.c
index c241038ee27c..7c50567f5ab0 100644
--- a/drivers/media/platform/aspeed-video.c
+++ b/drivers/media/platform/aspeed-video.c
@@ -936,7 +936,7 @@ static void aspeed_video_get_resolution(struct aspeed_video *video)
 	u32 src_lr_edge;
 	u32 src_tb_edge;
 	u32 sync;
-	u32 htotal;
+	u32 htotal, vtotal, vsync, hsync;
 	struct v4l2_bt_timings *det = &video->detected_timings;
 
 	det->width = MIN_WIDTH;
@@ -983,21 +983,35 @@ static void aspeed_video_get_resolution(struct aspeed_video *video)
 		mds = aspeed_video_read(video, VE_MODE_DETECT_STATUS);
 		sync = aspeed_video_read(video, VE_SYNC_STATUS);
 		htotal = aspeed_video_read(video, VE_H_TOTAL_PIXELS);
+		vtotal = FIELD_GET(VE_MODE_DETECT_V_LINES, mds);
+		vsync = FIELD_GET(VE_SYNC_STATUS_VSYNC, sync);
+		hsync = FIELD_GET(VE_SYNC_STATUS_HSYNC, sync);
 
 		video->frame_bottom = FIELD_GET(VE_SRC_TB_EDGE_DET_BOT, src_tb_edge);
 		video->frame_top = FIELD_GET(VE_SRC_TB_EDGE_DET_TOP, src_tb_edge);
-		det->vfrontporch = video->frame_top;
-		det->vbackporch = FIELD_GET(VE_MODE_DETECT_V_LINES, mds) -
-			video->frame_bottom;
-		det->vsync = FIELD_GET(VE_SYNC_STATUS_VSYNC, sync);
+		if (det->polarities & V4L2_DV_VSYNC_POS_POL) {
+			det->vbackporch = video->frame_top - vsync;
+			det->vfrontporch = vtotal - video->frame_bottom;
+			det->vsync = vsync;
+		} else {
+			det->vbackporch = video->frame_top;
+			det->vfrontporch = vsync - video->frame_bottom;
+			det->vsync = vtotal - vsync;
+		}
 		if (video->frame_top > video->frame_bottom)
 			continue;
 
 		video->frame_right = FIELD_GET(VE_SRC_LR_EDGE_DET_RT, src_lr_edge);
 		video->frame_left = FIELD_GET(VE_SRC_LR_EDGE_DET_LEFT, src_lr_edge);
-		det->hfrontporch = video->frame_left;
-		det->hbackporch = htotal - video->frame_right;
-		det->hsync = FIELD_GET(VE_SYNC_STATUS_HSYNC, sync);
+		if (det->polarities & V4L2_DV_HSYNC_POS_POL) {
+			det->hbackporch = video->frame_left - hsync;
+			det->hfrontporch = htotal - video->frame_right;
+			det->hsync = hsync;
+		} else {
+			det->hbackporch = video->frame_left;
+			det->hfrontporch = hsync - video->frame_right;
+			det->hsync = htotal - hsync;
+		}
 		if (video->frame_left > video->frame_right)
 			continue;
 
-- 
2.25.1


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

* [PATCH v2 4/4] media: aspeed: Fix timing polarity incorrect
  2021-12-22  8:21 [PATCH 0/4] Correct timing report Jammy Huang
                   ` (2 preceding siblings ...)
  2021-12-22  8:21 ` [PATCH v2 3/4] media: aspeed: Correct values for detected timing Jammy Huang
@ 2021-12-22  8:21 ` Jammy Huang
  2022-01-21  7:30   ` Hans Verkuil
  3 siblings, 1 reply; 12+ messages in thread
From: Jammy Huang @ 2021-12-22  8:21 UTC (permalink / raw)
  To: eajames, mchehab, joel, andrew, linux-media, openbmc,
	linux-arm-kernel, linux-aspeed, linux-kernel

This is a workaround for sync polarity unstable.
Sync value get by VR09C counts from sync's rising edge, which means
sync's polarity is negative if sync value is bigger than total/2.

Signed-off-by: Jammy Huang <jammy_huang@aspeedtech.com>
---
 v2:
  - Use 'total/2' rather than 'total<<1'
  - Update comment
---
 drivers/media/platform/aspeed-video.c | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/drivers/media/platform/aspeed-video.c b/drivers/media/platform/aspeed-video.c
index 7c50567f5ab0..c3e3343d91e1 100644
--- a/drivers/media/platform/aspeed-video.c
+++ b/drivers/media/platform/aspeed-video.c
@@ -989,6 +989,16 @@ static void aspeed_video_get_resolution(struct aspeed_video *video)
 
 		video->frame_bottom = FIELD_GET(VE_SRC_TB_EDGE_DET_BOT, src_tb_edge);
 		video->frame_top = FIELD_GET(VE_SRC_TB_EDGE_DET_TOP, src_tb_edge);
+
+		/*
+		 * This is a workaround for polarity detection when the sync
+		 * value is larger than half.
+		 */
+		if (vsync > (vtotal / 2))
+			det->polarities &= ~V4L2_DV_VSYNC_POS_POL;
+		else
+			det->polarities |= V4L2_DV_VSYNC_POS_POL;
+
 		if (det->polarities & V4L2_DV_VSYNC_POS_POL) {
 			det->vbackporch = video->frame_top - vsync;
 			det->vfrontporch = vtotal - video->frame_bottom;
@@ -1003,6 +1013,16 @@ static void aspeed_video_get_resolution(struct aspeed_video *video)
 
 		video->frame_right = FIELD_GET(VE_SRC_LR_EDGE_DET_RT, src_lr_edge);
 		video->frame_left = FIELD_GET(VE_SRC_LR_EDGE_DET_LEFT, src_lr_edge);
+
+		/*
+		 * This is a workaround for polarity detection when the sync
+		 * value is larger than half.
+		 */
+		if (hsync > (htotal / 2))
+			det->polarities &= ~V4L2_DV_HSYNC_POS_POL;
+		else
+			det->polarities |= V4L2_DV_HSYNC_POS_POL;
+
 		if (det->polarities & V4L2_DV_HSYNC_POS_POL) {
 			det->hbackporch = video->frame_left - hsync;
 			det->hfrontporch = htotal - video->frame_right;
-- 
2.25.1


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

* Re: [PATCH v2 3/4] media: aspeed: Correct values for detected timing
  2021-12-22  8:21 ` [PATCH v2 3/4] media: aspeed: Correct values for detected timing Jammy Huang
@ 2022-01-20 12:31   ` Hans Verkuil
  2022-01-21  6:01     ` Jammy Huang
  0 siblings, 1 reply; 12+ messages in thread
From: Hans Verkuil @ 2022-01-20 12:31 UTC (permalink / raw)
  To: Jammy Huang, eajames, mchehab, joel, andrew, linux-media,
	openbmc, linux-arm-kernel, linux-aspeed, linux-kernel

Hi Jammy,

I just want to double check this: I assume you have tested this with the
various polarity combinations?

I ask because I've never seen this before in any other hardware. The
sync and porch values reported by hardware are always independent from the
polarity, so that's why I am surprised to see this.

Same for the next patch (4/4).

Regards,

	Hans

On 12/22/21 09:21, Jammy Huang wrote:
> Correct timing's fp/sync/bp value based on the information below.
> It should be noticed that the calculation formula should be changed
> per sync polarity.
> 
> The sequence of signal: sync - backporch - video data - frontporch
> 
> The following registers start counting from sync's rising edge:
> 1. VR090: frame edge's left and right
> 2. VR094: frame edge's top and bottom
> 3. VR09C: counting from sync's rising edge to falling edge
> 
> [Vertical timing]
>             +--+     +-------------------+     +--+
>             |  |     |    v i d e o      |     |  |
>          +--+  +-----+                   +-----+  +---+
> 
>        vsync+--+
>    frame_top+--------+
> frame_bottom+----------------------------+
> 
>                   +-------------------+
>                   |    v i d e o      |
>       +--+  +-----+                   +-----+  +---+
>          |  |                               |  |
>          +--+                               +--+
>        vsync+-------------------------------+
>    frame_top+-----+
> frame_bottom+-------------------------+
> 
> [Horizontal timing]
>             +--+     +-------------------+     +--+
>             |  |     |    v i d e o      |     |  |
>          +--+  +-----+                   +-----+  +---+
> 
>        hsync+--+
>   frame_left+--------+
>  frame_right+----------------------------+
> 
>                   +-------------------+
>                   |    v i d e o      |
>       +--+  +-----+                   +-----+  +---+
>          |  |                               |  |
>          +--+                               +--+
>        hsync+-------------------------------+
>   frame_left+-----+
>  frame_right+-------------------------+
> 
> Signed-off-by: Jammy Huang <jammy_huang@aspeedtech.com>
> ---
>  v2:
>   - Code refined per Joel's suggestion
>   - Update commit message to have name matching variable
> ---
>  drivers/media/platform/aspeed-video.c | 30 ++++++++++++++++++++-------
>  1 file changed, 22 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/media/platform/aspeed-video.c b/drivers/media/platform/aspeed-video.c
> index c241038ee27c..7c50567f5ab0 100644
> --- a/drivers/media/platform/aspeed-video.c
> +++ b/drivers/media/platform/aspeed-video.c
> @@ -936,7 +936,7 @@ static void aspeed_video_get_resolution(struct aspeed_video *video)
>  	u32 src_lr_edge;
>  	u32 src_tb_edge;
>  	u32 sync;
> -	u32 htotal;
> +	u32 htotal, vtotal, vsync, hsync;
>  	struct v4l2_bt_timings *det = &video->detected_timings;
>  
>  	det->width = MIN_WIDTH;
> @@ -983,21 +983,35 @@ static void aspeed_video_get_resolution(struct aspeed_video *video)
>  		mds = aspeed_video_read(video, VE_MODE_DETECT_STATUS);
>  		sync = aspeed_video_read(video, VE_SYNC_STATUS);
>  		htotal = aspeed_video_read(video, VE_H_TOTAL_PIXELS);
> +		vtotal = FIELD_GET(VE_MODE_DETECT_V_LINES, mds);
> +		vsync = FIELD_GET(VE_SYNC_STATUS_VSYNC, sync);
> +		hsync = FIELD_GET(VE_SYNC_STATUS_HSYNC, sync);
>  
>  		video->frame_bottom = FIELD_GET(VE_SRC_TB_EDGE_DET_BOT, src_tb_edge);
>  		video->frame_top = FIELD_GET(VE_SRC_TB_EDGE_DET_TOP, src_tb_edge);
> -		det->vfrontporch = video->frame_top;
> -		det->vbackporch = FIELD_GET(VE_MODE_DETECT_V_LINES, mds) -
> -			video->frame_bottom;
> -		det->vsync = FIELD_GET(VE_SYNC_STATUS_VSYNC, sync);
> +		if (det->polarities & V4L2_DV_VSYNC_POS_POL) {
> +			det->vbackporch = video->frame_top - vsync;
> +			det->vfrontporch = vtotal - video->frame_bottom;
> +			det->vsync = vsync;
> +		} else {
> +			det->vbackporch = video->frame_top;
> +			det->vfrontporch = vsync - video->frame_bottom;
> +			det->vsync = vtotal - vsync;
> +		}
>  		if (video->frame_top > video->frame_bottom)
>  			continue;
>  
>  		video->frame_right = FIELD_GET(VE_SRC_LR_EDGE_DET_RT, src_lr_edge);
>  		video->frame_left = FIELD_GET(VE_SRC_LR_EDGE_DET_LEFT, src_lr_edge);
> -		det->hfrontporch = video->frame_left;
> -		det->hbackporch = htotal - video->frame_right;
> -		det->hsync = FIELD_GET(VE_SYNC_STATUS_HSYNC, sync);
> +		if (det->polarities & V4L2_DV_HSYNC_POS_POL) {
> +			det->hbackporch = video->frame_left - hsync;
> +			det->hfrontporch = htotal - video->frame_right;
> +			det->hsync = hsync;
> +		} else {
> +			det->hbackporch = video->frame_left;
> +			det->hfrontporch = hsync - video->frame_right;
> +			det->hsync = htotal - hsync;
> +		}
>  		if (video->frame_left > video->frame_right)
>  			continue;
>  

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

* Re: [PATCH v2 3/4] media: aspeed: Correct values for detected timing
  2022-01-20 12:31   ` Hans Verkuil
@ 2022-01-21  6:01     ` Jammy Huang
  2022-01-21  7:28       ` Hans Verkuil
  0 siblings, 1 reply; 12+ messages in thread
From: Jammy Huang @ 2022-01-21  6:01 UTC (permalink / raw)
  To: Hans Verkuil, eajames, mchehab, joel, andrew, linux-media,
	openbmc, linux-arm-kernel, linux-aspeed, linux-kernel

Hi Hans,

Yes, this is a weird part of our hardware.
Because it uses the rising edge of the sync to start counting, an 
additional calculation
is needed to get the exact value of the timings.

This problem was found when I was debugging the timing detection 
unstable problem.

Reards,

Jammy

On 2022/1/20 下午 08:31, Hans Verkuil wrote:
> Hi Jammy,
>
> I just want to double check this: I assume you have tested this with the
> various polarity combinations?
>
> I ask because I've never seen this before in any other hardware. The
> sync and porch values reported by hardware are always independent from the
> polarity, so that's why I am surprised to see this.
>
> Same for the next patch (4/4).
>
> Regards,
>
> 	Hans
>
> On 12/22/21 09:21, Jammy Huang wrote:
>> Correct timing's fp/sync/bp value based on the information below.
>> It should be noticed that the calculation formula should be changed
>> per sync polarity.
>>
>> The sequence of signal: sync - backporch - video data - frontporch
>>
>> The following registers start counting from sync's rising edge:
>> 1. VR090: frame edge's left and right
>> 2. VR094: frame edge's top and bottom
>> 3. VR09C: counting from sync's rising edge to falling edge
>>
>> [Vertical timing]
>>              +--+     +-------------------+     +--+
>>              |  |     |    v i d e o      |     |  |
>>           +--+  +-----+                   +-----+  +---+
>>
>>         vsync+--+
>>     frame_top+--------+
>> frame_bottom+----------------------------+
>>
>>                    +-------------------+
>>                    |    v i d e o      |
>>        +--+  +-----+                   +-----+  +---+
>>           |  |                               |  |
>>           +--+                               +--+
>>         vsync+-------------------------------+
>>     frame_top+-----+
>> frame_bottom+-------------------------+
>>
>> [Horizontal timing]
>>              +--+     +-------------------+     +--+
>>              |  |     |    v i d e o      |     |  |
>>           +--+  +-----+                   +-----+  +---+
>>
>>         hsync+--+
>>    frame_left+--------+
>>   frame_right+----------------------------+
>>
>>                    +-------------------+
>>                    |    v i d e o      |
>>        +--+  +-----+                   +-----+  +---+
>>           |  |                               |  |
>>           +--+                               +--+
>>         hsync+-------------------------------+
>>    frame_left+-----+
>>   frame_right+-------------------------+
>>
>> Signed-off-by: Jammy Huang <jammy_huang@aspeedtech.com>
>> ---
>>   v2:
>>    - Code refined per Joel's suggestion
>>    - Update commit message to have name matching variable
>> ---
>>   drivers/media/platform/aspeed-video.c | 30 ++++++++++++++++++++-------
>>   1 file changed, 22 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/media/platform/aspeed-video.c b/drivers/media/platform/aspeed-video.c
>> index c241038ee27c..7c50567f5ab0 100644
>> --- a/drivers/media/platform/aspeed-video.c
>> +++ b/drivers/media/platform/aspeed-video.c
>> @@ -936,7 +936,7 @@ static void aspeed_video_get_resolution(struct aspeed_video *video)
>>   	u32 src_lr_edge;
>>   	u32 src_tb_edge;
>>   	u32 sync;
>> -	u32 htotal;
>> +	u32 htotal, vtotal, vsync, hsync;
>>   	struct v4l2_bt_timings *det = &video->detected_timings;
>>   
>>   	det->width = MIN_WIDTH;
>> @@ -983,21 +983,35 @@ static void aspeed_video_get_resolution(struct aspeed_video *video)
>>   		mds = aspeed_video_read(video, VE_MODE_DETECT_STATUS);
>>   		sync = aspeed_video_read(video, VE_SYNC_STATUS);
>>   		htotal = aspeed_video_read(video, VE_H_TOTAL_PIXELS);
>> +		vtotal = FIELD_GET(VE_MODE_DETECT_V_LINES, mds);
>> +		vsync = FIELD_GET(VE_SYNC_STATUS_VSYNC, sync);
>> +		hsync = FIELD_GET(VE_SYNC_STATUS_HSYNC, sync);
>>   
>>   		video->frame_bottom = FIELD_GET(VE_SRC_TB_EDGE_DET_BOT, src_tb_edge);
>>   		video->frame_top = FIELD_GET(VE_SRC_TB_EDGE_DET_TOP, src_tb_edge);
>> -		det->vfrontporch = video->frame_top;
>> -		det->vbackporch = FIELD_GET(VE_MODE_DETECT_V_LINES, mds) -
>> -			video->frame_bottom;
>> -		det->vsync = FIELD_GET(VE_SYNC_STATUS_VSYNC, sync);
>> +		if (det->polarities & V4L2_DV_VSYNC_POS_POL) {
>> +			det->vbackporch = video->frame_top - vsync;
>> +			det->vfrontporch = vtotal - video->frame_bottom;
>> +			det->vsync = vsync;
>> +		} else {
>> +			det->vbackporch = video->frame_top;
>> +			det->vfrontporch = vsync - video->frame_bottom;
>> +			det->vsync = vtotal - vsync;
>> +		}
>>   		if (video->frame_top > video->frame_bottom)
>>   			continue;
>>   
>>   		video->frame_right = FIELD_GET(VE_SRC_LR_EDGE_DET_RT, src_lr_edge);
>>   		video->frame_left = FIELD_GET(VE_SRC_LR_EDGE_DET_LEFT, src_lr_edge);
>> -		det->hfrontporch = video->frame_left;
>> -		det->hbackporch = htotal - video->frame_right;
>> -		det->hsync = FIELD_GET(VE_SYNC_STATUS_HSYNC, sync);
>> +		if (det->polarities & V4L2_DV_HSYNC_POS_POL) {
>> +			det->hbackporch = video->frame_left - hsync;
>> +			det->hfrontporch = htotal - video->frame_right;
>> +			det->hsync = hsync;
>> +		} else {
>> +			det->hbackporch = video->frame_left;
>> +			det->hfrontporch = hsync - video->frame_right;
>> +			det->hsync = htotal - hsync;
>> +		}
>>   		if (video->frame_left > video->frame_right)
>>   			continue;
>>   

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

* Re: [PATCH v2 3/4] media: aspeed: Correct values for detected timing
  2022-01-21  6:01     ` Jammy Huang
@ 2022-01-21  7:28       ` Hans Verkuil
  0 siblings, 0 replies; 12+ messages in thread
From: Hans Verkuil @ 2022-01-21  7:28 UTC (permalink / raw)
  To: Jammy Huang, eajames, mchehab, joel, andrew, linux-media,
	openbmc, linux-arm-kernel, linux-aspeed, linux-kernel

Hi Jammy,

On 1/21/22 07:01, Jammy Huang wrote:
> Hi Hans,
> 
> Yes, this is a weird part of our hardware.
> Because it uses the rising edge of the sync to start counting, an additional calculation
> is needed to get the exact value of the timings.
> 
> This problem was found when I was debugging the timing detection unstable problem.

OK, thank you for the confirmation.

I would like a v3 where basically the explanation you made in the cover letter
is copied to the code, probably as a comment block just before aspeed_video_get_resolution().

This is unusual enough that it should be documented well in the code.

Regards,

	Hans

> 
> Reards,
> 
> Jammy
> 
> On 2022/1/20 下午 08:31, Hans Verkuil wrote:
>> Hi Jammy,
>>
>> I just want to double check this: I assume you have tested this with the
>> various polarity combinations?
>>
>> I ask because I've never seen this before in any other hardware. The
>> sync and porch values reported by hardware are always independent from the
>> polarity, so that's why I am surprised to see this.
>>
>> Same for the next patch (4/4).
>>
>> Regards,
>>
>>     Hans
>>
>> On 12/22/21 09:21, Jammy Huang wrote:
>>> Correct timing's fp/sync/bp value based on the information below.
>>> It should be noticed that the calculation formula should be changed
>>> per sync polarity.
>>>
>>> The sequence of signal: sync - backporch - video data - frontporch
>>>
>>> The following registers start counting from sync's rising edge:
>>> 1. VR090: frame edge's left and right
>>> 2. VR094: frame edge's top and bottom
>>> 3. VR09C: counting from sync's rising edge to falling edge
>>>
>>> [Vertical timing]
>>>              +--+     +-------------------+     +--+
>>>              |  |     |    v i d e o      |     |  |
>>>           +--+  +-----+                   +-----+  +---+
>>>
>>>         vsync+--+
>>>     frame_top+--------+
>>> frame_bottom+----------------------------+
>>>
>>>                    +-------------------+
>>>                    |    v i d e o      |
>>>        +--+  +-----+                   +-----+  +---+
>>>           |  |                               |  |
>>>           +--+                               +--+
>>>         vsync+-------------------------------+
>>>     frame_top+-----+
>>> frame_bottom+-------------------------+
>>>
>>> [Horizontal timing]
>>>              +--+     +-------------------+     +--+
>>>              |  |     |    v i d e o      |     |  |
>>>           +--+  +-----+                   +-----+  +---+
>>>
>>>         hsync+--+
>>>    frame_left+--------+
>>>   frame_right+----------------------------+
>>>
>>>                    +-------------------+
>>>                    |    v i d e o      |
>>>        +--+  +-----+                   +-----+  +---+
>>>           |  |                               |  |
>>>           +--+                               +--+
>>>         hsync+-------------------------------+
>>>    frame_left+-----+
>>>   frame_right+-------------------------+
>>>
>>> Signed-off-by: Jammy Huang <jammy_huang@aspeedtech.com>
>>> ---
>>>   v2:
>>>    - Code refined per Joel's suggestion
>>>    - Update commit message to have name matching variable
>>> ---
>>>   drivers/media/platform/aspeed-video.c | 30 ++++++++++++++++++++-------
>>>   1 file changed, 22 insertions(+), 8 deletions(-)
>>>
>>> diff --git a/drivers/media/platform/aspeed-video.c b/drivers/media/platform/aspeed-video.c
>>> index c241038ee27c..7c50567f5ab0 100644
>>> --- a/drivers/media/platform/aspeed-video.c
>>> +++ b/drivers/media/platform/aspeed-video.c
>>> @@ -936,7 +936,7 @@ static void aspeed_video_get_resolution(struct aspeed_video *video)
>>>       u32 src_lr_edge;
>>>       u32 src_tb_edge;
>>>       u32 sync;
>>> -    u32 htotal;
>>> +    u32 htotal, vtotal, vsync, hsync;
>>>       struct v4l2_bt_timings *det = &video->detected_timings;
>>>         det->width = MIN_WIDTH;
>>> @@ -983,21 +983,35 @@ static void aspeed_video_get_resolution(struct aspeed_video *video)
>>>           mds = aspeed_video_read(video, VE_MODE_DETECT_STATUS);
>>>           sync = aspeed_video_read(video, VE_SYNC_STATUS);
>>>           htotal = aspeed_video_read(video, VE_H_TOTAL_PIXELS);
>>> +        vtotal = FIELD_GET(VE_MODE_DETECT_V_LINES, mds);
>>> +        vsync = FIELD_GET(VE_SYNC_STATUS_VSYNC, sync);
>>> +        hsync = FIELD_GET(VE_SYNC_STATUS_HSYNC, sync);
>>>             video->frame_bottom = FIELD_GET(VE_SRC_TB_EDGE_DET_BOT, src_tb_edge);
>>>           video->frame_top = FIELD_GET(VE_SRC_TB_EDGE_DET_TOP, src_tb_edge);
>>> -        det->vfrontporch = video->frame_top;
>>> -        det->vbackporch = FIELD_GET(VE_MODE_DETECT_V_LINES, mds) -
>>> -            video->frame_bottom;
>>> -        det->vsync = FIELD_GET(VE_SYNC_STATUS_VSYNC, sync);
>>> +        if (det->polarities & V4L2_DV_VSYNC_POS_POL) {
>>> +            det->vbackporch = video->frame_top - vsync;
>>> +            det->vfrontporch = vtotal - video->frame_bottom;
>>> +            det->vsync = vsync;
>>> +        } else {
>>> +            det->vbackporch = video->frame_top;
>>> +            det->vfrontporch = vsync - video->frame_bottom;
>>> +            det->vsync = vtotal - vsync;
>>> +        }
>>>           if (video->frame_top > video->frame_bottom)
>>>               continue;
>>>             video->frame_right = FIELD_GET(VE_SRC_LR_EDGE_DET_RT, src_lr_edge);
>>>           video->frame_left = FIELD_GET(VE_SRC_LR_EDGE_DET_LEFT, src_lr_edge);
>>> -        det->hfrontporch = video->frame_left;
>>> -        det->hbackporch = htotal - video->frame_right;
>>> -        det->hsync = FIELD_GET(VE_SYNC_STATUS_HSYNC, sync);
>>> +        if (det->polarities & V4L2_DV_HSYNC_POS_POL) {
>>> +            det->hbackporch = video->frame_left - hsync;
>>> +            det->hfrontporch = htotal - video->frame_right;
>>> +            det->hsync = hsync;
>>> +        } else {
>>> +            det->hbackporch = video->frame_left;
>>> +            det->hfrontporch = hsync - video->frame_right;
>>> +            det->hsync = htotal - hsync;
>>> +        }
>>>           if (video->frame_left > video->frame_right)
>>>               continue;
>>>   

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

* Re: [PATCH v2 4/4] media: aspeed: Fix timing polarity incorrect
  2021-12-22  8:21 ` [PATCH v2 4/4] media: aspeed: Fix timing polarity incorrect Jammy Huang
@ 2022-01-21  7:30   ` Hans Verkuil
  2022-01-24  2:29     ` Jammy Huang
  0 siblings, 1 reply; 12+ messages in thread
From: Hans Verkuil @ 2022-01-21  7:30 UTC (permalink / raw)
  To: Jammy Huang, eajames, mchehab, joel, andrew, linux-media,
	openbmc, linux-arm-kernel, linux-aspeed, linux-kernel

On 12/22/21 09:21, Jammy Huang wrote:
> This is a workaround for sync polarity unstable.
> Sync value get by VR09C counts from sync's rising edge, which means
> sync's polarity is negative if sync value is bigger than total/2.

Do you have an example of such a format, or is this mostly theoretical?

Either provide the example or make a note that it is theoretical.

Regards,

	Hans

> 
> Signed-off-by: Jammy Huang <jammy_huang@aspeedtech.com>
> ---
>  v2:
>   - Use 'total/2' rather than 'total<<1'
>   - Update comment
> ---
>  drivers/media/platform/aspeed-video.c | 20 ++++++++++++++++++++
>  1 file changed, 20 insertions(+)
> 
> diff --git a/drivers/media/platform/aspeed-video.c b/drivers/media/platform/aspeed-video.c
> index 7c50567f5ab0..c3e3343d91e1 100644
> --- a/drivers/media/platform/aspeed-video.c
> +++ b/drivers/media/platform/aspeed-video.c
> @@ -989,6 +989,16 @@ static void aspeed_video_get_resolution(struct aspeed_video *video)
>  
>  		video->frame_bottom = FIELD_GET(VE_SRC_TB_EDGE_DET_BOT, src_tb_edge);
>  		video->frame_top = FIELD_GET(VE_SRC_TB_EDGE_DET_TOP, src_tb_edge);
> +
> +		/*
> +		 * This is a workaround for polarity detection when the sync
> +		 * value is larger than half.
> +		 */
> +		if (vsync > (vtotal / 2))
> +			det->polarities &= ~V4L2_DV_VSYNC_POS_POL;
> +		else
> +			det->polarities |= V4L2_DV_VSYNC_POS_POL;
> +
>  		if (det->polarities & V4L2_DV_VSYNC_POS_POL) {
>  			det->vbackporch = video->frame_top - vsync;
>  			det->vfrontporch = vtotal - video->frame_bottom;
> @@ -1003,6 +1013,16 @@ static void aspeed_video_get_resolution(struct aspeed_video *video)
>  
>  		video->frame_right = FIELD_GET(VE_SRC_LR_EDGE_DET_RT, src_lr_edge);
>  		video->frame_left = FIELD_GET(VE_SRC_LR_EDGE_DET_LEFT, src_lr_edge);
> +
> +		/*
> +		 * This is a workaround for polarity detection when the sync
> +		 * value is larger than half.
> +		 */
> +		if (hsync > (htotal / 2))
> +			det->polarities &= ~V4L2_DV_HSYNC_POS_POL;
> +		else
> +			det->polarities |= V4L2_DV_HSYNC_POS_POL;
> +
>  		if (det->polarities & V4L2_DV_HSYNC_POS_POL) {
>  			det->hbackporch = video->frame_left - hsync;
>  			det->hfrontporch = htotal - video->frame_right;

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

* Re: [PATCH v2 4/4] media: aspeed: Fix timing polarity incorrect
  2022-01-21  7:30   ` Hans Verkuil
@ 2022-01-24  2:29     ` Jammy Huang
  2022-01-24 10:22       ` Hans Verkuil
  0 siblings, 1 reply; 12+ messages in thread
From: Jammy Huang @ 2022-01-24  2:29 UTC (permalink / raw)
  To: Hans Verkuil, eajames, mchehab, joel, andrew, linux-media,
	openbmc, linux-arm-kernel, linux-aspeed, linux-kernel


On 2022/1/21 下午 03:30, Hans Verkuil wrote:
> On 12/22/21 09:21, Jammy Huang wrote:
>> This is a workaround for sync polarity unstable.
>> Sync value get by VR09C counts from sync's rising edge, which means
>> sync's polarity is negative if sync value is bigger than total/2.
> Do you have an example of such a format, or is this mostly theoretical?
>
> Either provide the example or make a note that it is theoretical.

OK, I will update an example in next patch. Let me explain first.

This is a must-be result. Please refer to the graph below as I sent in 
3/4 of this
series. For negative sync, sync width equals to back porch + data enable 
+ front porch.
Thus, sync would be bigger than 90% of total in most cases.

                   +-------------------+
                   |    v i d e o      |
       +--+  +-----+                   +-----+  +---+
          |  |                               |  |
          +--+                               +--+
        vsync+-------------------------------+
    frame_top+-----+
frame_bottom+-------------------------+


Following registers are what I got for 1920x1200@60.

1e700090: 07ee206f 04c9001a c4d3efff 04cc001f

1e7000a0: 0000081f 00000000 00000000 00000000

vertical total = 0x4D3 (VR098[27:16]) = 1235
vertical sync = 0x4CC (VR09C[27:16]) = 1228

>
> Regards,
>
> 	Hans
>
>> Signed-off-by: Jammy Huang <jammy_huang@aspeedtech.com>
>> ---
>>   v2:
>>    - Use 'total/2' rather than 'total<<1'
>>    - Update comment
>> ---
>>   drivers/media/platform/aspeed-video.c | 20 ++++++++++++++++++++
>>   1 file changed, 20 insertions(+)
>>
>> diff --git a/drivers/media/platform/aspeed-video.c b/drivers/media/platform/aspeed-video.c
>> index 7c50567f5ab0..c3e3343d91e1 100644
>> --- a/drivers/media/platform/aspeed-video.c
>> +++ b/drivers/media/platform/aspeed-video.c
>> @@ -989,6 +989,16 @@ static void aspeed_video_get_resolution(struct aspeed_video *video)
>>   
>>   		video->frame_bottom = FIELD_GET(VE_SRC_TB_EDGE_DET_BOT, src_tb_edge);
>>   		video->frame_top = FIELD_GET(VE_SRC_TB_EDGE_DET_TOP, src_tb_edge);
>> +
>> +		/*
>> +		 * This is a workaround for polarity detection when the sync
>> +		 * value is larger than half.
>> +		 */
>> +		if (vsync > (vtotal / 2))
>> +			det->polarities &= ~V4L2_DV_VSYNC_POS_POL;
>> +		else
>> +			det->polarities |= V4L2_DV_VSYNC_POS_POL;
>> +
>>   		if (det->polarities & V4L2_DV_VSYNC_POS_POL) {
>>   			det->vbackporch = video->frame_top - vsync;
>>   			det->vfrontporch = vtotal - video->frame_bottom;
>> @@ -1003,6 +1013,16 @@ static void aspeed_video_get_resolution(struct aspeed_video *video)
>>   
>>   		video->frame_right = FIELD_GET(VE_SRC_LR_EDGE_DET_RT, src_lr_edge);
>>   		video->frame_left = FIELD_GET(VE_SRC_LR_EDGE_DET_LEFT, src_lr_edge);
>> +
>> +		/*
>> +		 * This is a workaround for polarity detection when the sync
>> +		 * value is larger than half.
>> +		 */
>> +		if (hsync > (htotal / 2))
>> +			det->polarities &= ~V4L2_DV_HSYNC_POS_POL;
>> +		else
>> +			det->polarities |= V4L2_DV_HSYNC_POS_POL;
>> +
>>   		if (det->polarities & V4L2_DV_HSYNC_POS_POL) {
>>   			det->hbackporch = video->frame_left - hsync;
>>   			det->hfrontporch = htotal - video->frame_right;

-- 
Best Regards
Jammy


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

* Re: [PATCH v2 4/4] media: aspeed: Fix timing polarity incorrect
  2022-01-24  2:29     ` Jammy Huang
@ 2022-01-24 10:22       ` Hans Verkuil
  2022-01-25  1:37         ` Jammy Huang
  0 siblings, 1 reply; 12+ messages in thread
From: Hans Verkuil @ 2022-01-24 10:22 UTC (permalink / raw)
  To: Jammy Huang, eajames, mchehab, joel, andrew, linux-media,
	openbmc, linux-arm-kernel, linux-aspeed, linux-kernel

On 24/01/2022 03:29, Jammy Huang wrote:
> 
> On 2022/1/21 下午 03:30, Hans Verkuil wrote:
>> On 12/22/21 09:21, Jammy Huang wrote:
>>> This is a workaround for sync polarity unstable.
>>> Sync value get by VR09C counts from sync's rising edge, which means
>>> sync's polarity is negative if sync value is bigger than total/2.
>> Do you have an example of such a format, or is this mostly theoretical?
>>
>> Either provide the example or make a note that it is theoretical.
> 
> OK, I will update an example in next patch. Let me explain first.
> 
> This is a must-be result. Please refer to the graph below as I sent in 3/4 of this
> series. For negative sync, sync width equals to back porch + data enable + front porch.
> Thus, sync would be bigger than 90% of total in most cases.

Right, I suspected that might be the case.

I think it would be better to combine patches 3 and 4 into a single
patch, since they are closely related and it is actually easier to
understand if it's just a single patch.

Regards,

	Hans

> 
>                   +-------------------+
>                   |    v i d e o      |
>       +--+  +-----+                   +-----+  +---+
>          |  |                               |  |
>          +--+                               +--+
>        vsync+-------------------------------+
>    frame_top+-----+
> frame_bottom+-------------------------+
> 
> 
> Following registers are what I got for 1920x1200@60.
> 
> 1e700090: 07ee206f 04c9001a c4d3efff 04cc001f
> 
> 1e7000a0: 0000081f 00000000 00000000 00000000
> 
> vertical total = 0x4D3 (VR098[27:16]) = 1235
> vertical sync = 0x4CC (VR09C[27:16]) = 1228
> 
>>
>> Regards,
>>
>>     Hans
>>
>>> Signed-off-by: Jammy Huang <jammy_huang@aspeedtech.com>
>>> ---
>>>   v2:
>>>    - Use 'total/2' rather than 'total<<1'
>>>    - Update comment
>>> ---
>>>   drivers/media/platform/aspeed-video.c | 20 ++++++++++++++++++++
>>>   1 file changed, 20 insertions(+)
>>>
>>> diff --git a/drivers/media/platform/aspeed-video.c b/drivers/media/platform/aspeed-video.c
>>> index 7c50567f5ab0..c3e3343d91e1 100644
>>> --- a/drivers/media/platform/aspeed-video.c
>>> +++ b/drivers/media/platform/aspeed-video.c
>>> @@ -989,6 +989,16 @@ static void aspeed_video_get_resolution(struct aspeed_video *video)
>>>             video->frame_bottom = FIELD_GET(VE_SRC_TB_EDGE_DET_BOT, src_tb_edge);
>>>           video->frame_top = FIELD_GET(VE_SRC_TB_EDGE_DET_TOP, src_tb_edge);
>>> +
>>> +        /*
>>> +         * This is a workaround for polarity detection when the sync
>>> +         * value is larger than half.
>>> +         */
>>> +        if (vsync > (vtotal / 2))
>>> +            det->polarities &= ~V4L2_DV_VSYNC_POS_POL;
>>> +        else
>>> +            det->polarities |= V4L2_DV_VSYNC_POS_POL;
>>> +
>>>           if (det->polarities & V4L2_DV_VSYNC_POS_POL) {
>>>               det->vbackporch = video->frame_top - vsync;
>>>               det->vfrontporch = vtotal - video->frame_bottom;
>>> @@ -1003,6 +1013,16 @@ static void aspeed_video_get_resolution(struct aspeed_video *video)
>>>             video->frame_right = FIELD_GET(VE_SRC_LR_EDGE_DET_RT, src_lr_edge);
>>>           video->frame_left = FIELD_GET(VE_SRC_LR_EDGE_DET_LEFT, src_lr_edge);
>>> +
>>> +        /*
>>> +         * This is a workaround for polarity detection when the sync
>>> +         * value is larger than half.
>>> +         */
>>> +        if (hsync > (htotal / 2))
>>> +            det->polarities &= ~V4L2_DV_HSYNC_POS_POL;
>>> +        else
>>> +            det->polarities |= V4L2_DV_HSYNC_POS_POL;
>>> +
>>>           if (det->polarities & V4L2_DV_HSYNC_POS_POL) {
>>>               det->hbackporch = video->frame_left - hsync;
>>>               det->hfrontporch = htotal - video->frame_right;
> 


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

* Re: [PATCH v2 4/4] media: aspeed: Fix timing polarity incorrect
  2022-01-24 10:22       ` Hans Verkuil
@ 2022-01-25  1:37         ` Jammy Huang
  0 siblings, 0 replies; 12+ messages in thread
From: Jammy Huang @ 2022-01-25  1:37 UTC (permalink / raw)
  To: Hans Verkuil, eajames, mchehab, joel, andrew, linux-media,
	openbmc, linux-arm-kernel, linux-aspeed, linux-kernel


On 2022/1/24 下午 06:22, Hans Verkuil wrote:
> On 24/01/2022 03:29, Jammy Huang wrote:
>> On 2022/1/21 下午 03:30, Hans Verkuil wrote:
>>> On 12/22/21 09:21, Jammy Huang wrote:
>>>> This is a workaround for sync polarity unstable.
>>>> Sync value get by VR09C counts from sync's rising edge, which means
>>>> sync's polarity is negative if sync value is bigger than total/2.
>>> Do you have an example of such a format, or is this mostly theoretical?
>>>
>>> Either provide the example or make a note that it is theoretical.
>> OK, I will update an example in next patch. Let me explain first.
>>
>> This is a must-be result. Please refer to the graph below as I sent in 3/4 of this
>> series. For negative sync, sync width equals to back porch + data enable + front porch.
>> Thus, sync would be bigger than 90% of total in most cases.
> Right, I suspected that might be the case.
>
> I think it would be better to combine patches 3 and 4 into a single
> patch, since they are closely related and it is actually easier to
> understand if it's just a single patch.
OK, I will combine 3/4 into a single patch in next series.
Thanks.

Best Regards,

         Jammy
>
> Regards,
>
> 	Hans
>
>>                    +-------------------+
>>                    |    v i d e o      |
>>        +--+  +-----+                   +-----+  +---+
>>           |  |                               |  |
>>           +--+                               +--+
>>         vsync+-------------------------------+
>>     frame_top+-----+
>> frame_bottom+-------------------------+
>>
>>
>> Following registers are what I got for 1920x1200@60.
>>
>> 1e700090: 07ee206f 04c9001a c4d3efff 04cc001f
>>
>> 1e7000a0: 0000081f 00000000 00000000 00000000
>>
>> vertical total = 0x4D3 (VR098[27:16]) = 1235
>> vertical sync = 0x4CC (VR09C[27:16]) = 1228
>>
>>> Regards,
>>>
>>>      Hans
>>>
>>>> Signed-off-by: Jammy Huang <jammy_huang@aspeedtech.com>
>>>> ---
>>>>    v2:
>>>>     - Use 'total/2' rather than 'total<<1'
>>>>     - Update comment
>>>> ---
>>>>    drivers/media/platform/aspeed-video.c | 20 ++++++++++++++++++++
>>>>    1 file changed, 20 insertions(+)
>>>>
>>>> diff --git a/drivers/media/platform/aspeed-video.c b/drivers/media/platform/aspeed-video.c
>>>> index 7c50567f5ab0..c3e3343d91e1 100644
>>>> --- a/drivers/media/platform/aspeed-video.c
>>>> +++ b/drivers/media/platform/aspeed-video.c
>>>> @@ -989,6 +989,16 @@ static void aspeed_video_get_resolution(struct aspeed_video *video)
>>>>              video->frame_bottom = FIELD_GET(VE_SRC_TB_EDGE_DET_BOT, src_tb_edge);
>>>>            video->frame_top = FIELD_GET(VE_SRC_TB_EDGE_DET_TOP, src_tb_edge);
>>>> +
>>>> +        /*
>>>> +         * This is a workaround for polarity detection when the sync
>>>> +         * value is larger than half.
>>>> +         */
>>>> +        if (vsync > (vtotal / 2))
>>>> +            det->polarities &= ~V4L2_DV_VSYNC_POS_POL;
>>>> +        else
>>>> +            det->polarities |= V4L2_DV_VSYNC_POS_POL;
>>>> +
>>>>            if (det->polarities & V4L2_DV_VSYNC_POS_POL) {
>>>>                det->vbackporch = video->frame_top - vsync;
>>>>                det->vfrontporch = vtotal - video->frame_bottom;
>>>> @@ -1003,6 +1013,16 @@ static void aspeed_video_get_resolution(struct aspeed_video *video)
>>>>              video->frame_right = FIELD_GET(VE_SRC_LR_EDGE_DET_RT, src_lr_edge);
>>>>            video->frame_left = FIELD_GET(VE_SRC_LR_EDGE_DET_LEFT, src_lr_edge);
>>>> +
>>>> +        /*
>>>> +         * This is a workaround for polarity detection when the sync
>>>> +         * value is larger than half.
>>>> +         */
>>>> +        if (hsync > (htotal / 2))
>>>> +            det->polarities &= ~V4L2_DV_HSYNC_POS_POL;
>>>> +        else
>>>> +            det->polarities |= V4L2_DV_HSYNC_POS_POL;
>>>> +
>>>>            if (det->polarities & V4L2_DV_HSYNC_POS_POL) {
>>>>                det->hbackporch = video->frame_left - hsync;
>>>>                det->hfrontporch = htotal - video->frame_right;

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

end of thread, other threads:[~2022-01-25  3:10 UTC | newest]

Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-12-22  8:21 [PATCH 0/4] Correct timing report Jammy Huang
2021-12-22  8:21 ` [PATCH v2 1/4] media: aspeed: Correct value for h-total-pixels Jammy Huang
2021-12-22  8:21 ` [PATCH v2 2/4] media: aspeed: Use FIELD_GET to improve readability Jammy Huang
2021-12-22  8:21 ` [PATCH v2 3/4] media: aspeed: Correct values for detected timing Jammy Huang
2022-01-20 12:31   ` Hans Verkuil
2022-01-21  6:01     ` Jammy Huang
2022-01-21  7:28       ` Hans Verkuil
2021-12-22  8:21 ` [PATCH v2 4/4] media: aspeed: Fix timing polarity incorrect Jammy Huang
2022-01-21  7:30   ` Hans Verkuil
2022-01-24  2:29     ` Jammy Huang
2022-01-24 10:22       ` Hans Verkuil
2022-01-25  1:37         ` Jammy Huang

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).