Handling of reduced FPS in V4L2

PATCH In review Jose Abreu 3 participants 16 messages Mar 21, 2017 – Apr 19, 2017

Subsystems (2)

Media (V4L/DVB) Cobalt media driver

lore

Share & tools

lore

Jose Abreu

author

2017-03-21 11:49 UTC #0 lore raw

cover Handling of reduced FPS in V4L2

Hi all,

This is a follow up patch from this discussion [1]. It should be seen more as a starting point to introduce better handling of time per frame in v4l2. Quoting Hans Verkuil from [1]:

1) "Add a flag V4L2_DV_FL_CAN_DETECT_REDUCED_FPS. If set, then the hw can detect the difference between regular fps and 1000/1001 fps. Note: this is only valid for timings of VIC codes with the V4L2_DV_FL_CAN_REDUCE_FPS flag set."

2) "Allow V4L2_DV_FL_REDUCED_FPS to be used for receivers if V4L2_DV_FL_CAN_DETECT_REDUCED_FPS is set."

3) "For standard VIC codes the pixelclock returned by query_dv_timings is that of the corresponding VIC timing, not what is measured. This will ensure fixed fps values"

4) "g_parm should calculate the fps based on the v4l2_bt_timings struct, looking at the REDUCES_FPS flags. For those receivers that cannot detect the difference, the fps will be 24/30/60 Hz, for those that can detect the difference g_parm can check if both V4L2_DV_FL_CAN_DETECT_REDUCED_FPS and V4L2_DV_FL_REDUCED_FPS are set and reduce the fps by 1000/1001."

-----------
In terms of implementation:
- Point 1) is done in patch 1/3
- Point 2) and 3) should be done by a HDMI Receiver driver (I think?).
- Point 4) is done in patch 2/3.
- The patch 3/3 is a simple implementation (which was not tested) in the cobalt driver
-----------

[1] https://patchwork.kernel.org/patch/9609441/

Best regards,
Jose Miguel Abreu

Cc Carlos Palminha <[email protected]>

Cc Mauro Carvalho Chehab <[email protected]>

Cc Hans Verkuil <[email protected]>

Cc [email protected]

Jose Abreu (3):
[media] videodev2.h: Add new DV flag CAN_DETECT_REDUCED_FPS [media] v4l2-dv-timings: Introduce v4l2_calc_timeperframe helper [media] cobalt: Use v4l2_calc_timeperframe helper

 drivers/media/pci/cobalt/cobalt-v4l2.c    |  9 +++++--
 drivers/media/v4l2-core/v4l2-dv-timings.c | 39 +++++++++++++++++++++++++++++++
 include/media/v4l2-dv-timings.h           | 11 +++++++++
 include/uapi/linux/videodev2.h            |  7 ++++++
 4 files changed, 64 insertions(+), 2 deletions(-)

Signature · 1 line

1.9.1

Jose Abreu

author

2017-03-21 11:49 UTC #1 lore raw

PATCH 1/3 videodev2.h: Add new DV flag CAN_DETECT_REDUCED_FPS

Add a new flag to UAPI for DV timings which, whenever set, indicates that hardware can detect the difference between regular FPS and 1000/1001 FPS.

This is specific to HDMI receivers. Also, it is only valid when V4L2_DV_FL_CAN_REDUCE_FPS is set.

Signed-off-by Jose Abreu <[email protected]>

Cc Carlos Palminha <[email protected]>

Cc Mauro Carvalho Chehab <[email protected]>

Cc Hans Verkuil <[email protected]>

Cc [email protected]

---
 include/uapi/linux/videodev2.h | 7 +++++++
 1 file changed, 7 insertions(+)

include/uapi/linux/videodev2.h +7 -0

index 45184a2..dd7b426 100644
@@ -1371,6 +1371,13 @@ struct v4l2_bt_timings {
  * InfoFrame).
  */
 #define V4L2_DV_FL_HAS_HDMI_VIC			(1 << 8)
+/*
+ * CEA-861 specific: only valid for video receivers.
+ * If set, then HW can detect the difference between regular FPS and
+ * 1000/1001 FPS. Note: This flag is only valid for HDMI VIC codes with
+ * the V4L2_DV_FL_CAN_REDUCE_FPS flag set.
+ */
+#define V4L2_DV_FL_CAN_DETECT_REDUCED_FPS	(1 << 9)
 
 /* A few useful defines to calculate the total blanking and frame sizes */
 #define V4L2_DV_BT_BLANKING_WIDTH(bt) \

Signature · 1 line

1.9.1

Jose Abreu

author

2017-03-21 11:50 UTC #2 lore raw

PATCH 2/3 v4l2-dv-timings: Introduce v4l2_calc_timeperframe helper

In reply to Jose Abreu

A new helper function was introduced to facilitate the calculation of time per frame value whenever we have access to the full v4l2_dv_timings structure.

This should be used only for receivers and only when there is enough accuracy in the measured pixel clock value as well as in the horizontal/vertical values.

Signed-off-by Jose Abreu <[email protected]>

Cc Carlos Palminha <[email protected]>

Cc Mauro Carvalho Chehab <[email protected]>

Cc Hans Verkuil <[email protected]>

Cc [email protected]

---
 drivers/media/v4l2-core/v4l2-dv-timings.c | 39 +++++++++++++++++++++++++++++++
 include/media/v4l2-dv-timings.h           | 11 +++++++++
 2 files changed, 50 insertions(+)

drivers/media/v4l2-core/v4l2-dv-timings.c +39 -0

index 5c8c49d..b7b39c2 100644
@@ -814,3 +814,42 @@ struct v4l2_fract v4l2_calc_aspect_ratio(u8 hor_landscape, u8 vert_portrait)
 	return aspect;
 }
 EXPORT_SYMBOL_GPL(v4l2_calc_aspect_ratio);
+
+/** v4l2_calc_timeperframe - helper function to calculate timeperframe based
+ *	v4l2_dv_timings fields.
+ * @t - Timings for the video mode.
+ *
+ * Calculates the expected timeperframe using the pixel clock value and
+ * horizontal/vertical measures. This means that v4l2_dv_timings structure
+ * must be correctly and fully filled.
+ */
+struct v4l2_fract v4l2_calc_timeperframe(const struct v4l2_dv_timings *t)
+{
+	const struct v4l2_bt_timings *bt = &t->bt;
+	struct v4l2_fract fps_fract = { 1, 1 };
+	unsigned long n, d;
+	u32 htot, vtot, fps;
+	u64 pclk;
+
+	if (t->type != V4L2_DV_BT_656_1120)
+		return fps_fract;
+
+	htot = V4L2_DV_BT_FRAME_WIDTH(bt);
+	vtot = V4L2_DV_BT_FRAME_HEIGHT(bt);
+	pclk = bt->pixelclock;
+
+	if ((bt->flags & V4L2_DV_FL_CAN_DETECT_REDUCED_FPS) &&
+	    (bt->flags & V4L2_DV_FL_REDUCED_FPS))
+		pclk = div_u64(pclk * 1000ULL, 1001);
+
+	fps = (htot * vtot) > 0 ? div_u64((100 * pclk), (htot * vtot)) : 0;
+	if (!fps)
+		return fps_fract;
+
+	rational_best_approximation(fps, 100, fps, 100, &n, &d);
+
+	fps_fract.numerator = d;
+	fps_fract.denominator = n;
+	return fps_fract;
+}
+EXPORT_SYMBOL_GPL(v4l2_calc_timeperframe);

include/media/v4l2-dv-timings.h +11 -0

index 61a1889..c1eef08 100644
@@ -203,6 +203,17 @@ bool v4l2_detect_gtf(unsigned frame_height, unsigned hfreq, unsigned vsync,
  */
 struct v4l2_fract v4l2_dv_timings_aspect_ratio(const struct v4l2_dv_timings *t);
 
+/**
+ * v4l2_calc_timeperframe - helper function to calculate timeperframe based
+ *	v4l2_dv_timings fields.
+ * @t - Timings for the video mode.
+ *
+ * Calculates the expected timeperframe using the pixel clock value and
+ * horizontal/vertical measures. This means that v4l2_dv_timings structure
+ * must be correctly and fully filled.
+ */
+struct v4l2_fract v4l2_calc_timeperframe(const struct v4l2_dv_timings *t);
+
 /*
  * reduce_fps - check if conditions for reduced fps are true.
  * bt - v4l2 timing structure

Signature · 1 line

1.9.1

Jose Abreu

author

2017-03-21 11:50 UTC #3 lore raw

PATCH 3/3 cobalt: Use v4l2_calc_timeperframe helper

In reply to Jose Abreu

Currently, cobalt driver always returns 60fps in g_parm. This patch uses the new v4l2_calc_timeperframe helper to calculate the time per frame value.

Signed-off-by Jose Abreu <[email protected]>

Cc Carlos Palminha <[email protected]>

Cc Mauro Carvalho Chehab <[email protected]>

Cc Hans Verkuil <[email protected]>

Cc [email protected]

---
 drivers/media/pci/cobalt/cobalt-v4l2.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

drivers/media/pci/cobalt/cobalt-v4l2.c +7 -2

index def4a3b..25532ae 100644
@@ -1076,10 +1076,15 @@ static int cobalt_subscribe_event(struct v4l2_fh *fh,
 
 static int cobalt_g_parm(struct file *file, void *fh, struct v4l2_streamparm *a)
 {
+	struct cobalt_stream *s = video_drvdata(file);
+	struct v4l2_fract fps;
+
 	if (a->type != V4L2_BUF_TYPE_VIDEO_CAPTURE)
 		return -EINVAL;
-	a->parm.capture.timeperframe.numerator = 1;
-	a->parm.capture.timeperframe.denominator = 60;
+
+	fps = v4l2_calc_timeperframe(&s->timings);
+	a->parm.capture.timeperframe.numerator = fps.numerator;
+	a->parm.capture.timeperframe.denominator = fps.denominator;
 	a->parm.capture.readbuffers = 3;
 	return 0;
 }

Signature · 1 line

1.9.1

Hans Verkuil

Tested-by on v1 3/3

2017-03-30 13:42 UTC #4 lore raw

Re: [PATCH 3/3] [media] cobalt: Use v4l2_calc_timeperframe helper

Hi Jose,

Quoted 3 lines · Jose Abreu

Currently, cobalt driver always returns 60fps in g_parm. This patch uses the new v4l2_calc_timeperframe helper to calculate the time per frame value.

I can verify that g_parm works, so:

Tested-by Hans Verkuil <[email protected]>

However, the adv7604 pixelclock detection resolution is only 0.25 MHz, so it can't tell the difference between 24 and 23.97 Hz. I can't set the new V4L2_DV_FL_CAN_DETECT_REDUCED_FPS flag there.

It might be possible to implement this for the adv7842 receiver, I think that that hardware is much more precise.

I would have to try this, but that will have to wait until Friday next week.

Regards,

Hans

Quoted 34 lines

Signed-off-by Jose Abreu <[email protected]>

Cc Carlos Palminha <[email protected]>

Cc Mauro Carvalho Chehab <[email protected]>

Cc Hans Verkuil <[email protected]>

Cc [email protected]

---
 drivers/media/pci/cobalt/cobalt-v4l2.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

drivers/media/pci/cobalt/cobalt-v4l2.c +7 -2

index def4a3b..25532ae 100644
@@ -1076,10 +1076,15 @@ static int cobalt_subscribe_event(struct v4l2_fh *fh,
 
 static int cobalt_g_parm(struct file *file, void *fh, struct v4l2_streamparm *a)
 {
+	struct cobalt_stream *s = video_drvdata(file);
+	struct v4l2_fract fps;
+
 	if (a->type != V4L2_BUF_TYPE_VIDEO_CAPTURE)
 		return -EINVAL;
-	a->parm.capture.timeperframe.numerator = 1;
-	a->parm.capture.timeperframe.denominator = 60;
+
+	fps = v4l2_calc_timeperframe(&s->timings);
+	a->parm.capture.timeperframe.numerator = fps.numerator;
+	a->parm.capture.timeperframe.denominator = fps.denominator;
 	a->parm.capture.readbuffers = 3;
 	return 0;
 }

Jose Abreu

author

2017-03-31 09:00 UTC #5 lore raw

Re: [PATCH 3/3] [media] cobalt: Use v4l2_calc_timeperframe helper

Hi Hans,

Quoted 10 lines · Hans Verkuil

Hi Jose,
Quoted 3 lines · Jose Abreu
Currently, cobalt driver always returns 60fps in g_parm. This patch uses the new v4l2_calc_timeperframe helper to calculate the time per frame value.
I can verify that g_parm works, so:
Tested-by Hans Verkuil <[email protected]>

Quoted 8 lines

However, the adv7604 pixelclock detection resolution is only 0.25 MHz, so it can't tell the difference between 24 and 23.97 Hz. I can't set the new V4L2_DV_FL_CAN_DETECT_REDUCED_FPS flag there.
It might be possible to implement this for the adv7842 receiver, I think that that hardware is much more precise.
I would have to try this, but that will have to wait until Friday next week.

Thanks! Yes, maybe its better to test with a different receiver, if it has more precision then its great. Let me know if you need any help :)

Best regards,
Jose Miguel Abreu

Quoted 38 lines

Regards,

Hans

Quoted 33 lines

Signed-off-by Jose Abreu <[email protected]>

Cc Carlos Palminha <[email protected]>

Cc Mauro Carvalho Chehab <[email protected]>

Cc Hans Verkuil <[email protected]>

Cc [email protected]

---
 drivers/media/pci/cobalt/cobalt-v4l2.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

drivers/media/pci/cobalt/cobalt-v4l2.c +7 -2

index def4a3b..25532ae 100644
@@ -1076,10 +1076,15 @@ static int cobalt_subscribe_event(struct v4l2_fh *fh,
 
 static int cobalt_g_parm(struct file *file, void *fh, struct v4l2_streamparm *a)
 {
+	struct cobalt_stream *s = video_drvdata(file);
+	struct v4l2_fract fps;
+
 	if (a->type != V4L2_BUF_TYPE_VIDEO_CAPTURE)
 		return -EINVAL;
-	a->parm.capture.timeperframe.numerator = 1;
-	a->parm.capture.timeperframe.denominator = 60;
+
+	fps = v4l2_calc_timeperframe(&s->timings);
+	a->parm.capture.timeperframe.numerator = fps.numerator;
+	a->parm.capture.timeperframe.denominator = fps.denominator;
 	a->parm.capture.readbuffers = 3;
 	return 0;
 }

Jose Abreu

author

2017-04-19 10:33 UTC #6 lore raw

Re: [PATCH 3/3] [media] cobalt: Use v4l2_calc_timeperframe helper

Hi Hans,

Quoted 18 lines · Jose Abreu

Hi Hans,
Quoted 14 lines · Hans Verkuil
Hi Jose,
Quoted 3 lines · Jose Abreu
Currently, cobalt driver always returns 60fps in g_parm. This patch uses the new v4l2_calc_timeperframe helper to calculate the time per frame value.
I can verify that g_parm works, so:
Tested-by Hans Verkuil <[email protected]>
However, the adv7604 pixelclock detection resolution is only 0.25 MHz, so it can't tell the difference between 24 and 23.97 Hz. I can't set the new V4L2_DV_FL_CAN_DETECT_REDUCED_FPS flag there.

Quoted 7 lines

Quoted 4 lines
It might be possible to implement this for the adv7842 receiver, I think that that hardware is much more precise.
I would have to try this, but that will have to wait until Friday next week.
Thanks! Yes, maybe its better to test with a different receiver, if it has more precision then its great. Let me know if you need any help :)

Any news regarding this?

Best regards,
Jose Miguel Abreu

Quoted 41 lines

Best regards,
Jose Miguel Abreu

Quoted 37 lines

Regards,

Hans

Quoted 33 lines

Signed-off-by Jose Abreu <[email protected]>

Cc Carlos Palminha <[email protected]>

Cc Mauro Carvalho Chehab <[email protected]>

Cc Hans Verkuil <[email protected]>

Cc [email protected]

---
 drivers/media/pci/cobalt/cobalt-v4l2.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

drivers/media/pci/cobalt/cobalt-v4l2.c +7 -2

index def4a3b..25532ae 100644
@@ -1076,10 +1076,15 @@ static int cobalt_subscribe_event(struct v4l2_fh *fh,
 
 static int cobalt_g_parm(struct file *file, void *fh, struct v4l2_streamparm *a)
 {
+	struct cobalt_stream *s = video_drvdata(file);
+	struct v4l2_fract fps;
+
 	if (a->type != V4L2_BUF_TYPE_VIDEO_CAPTURE)
 		return -EINVAL;
-	a->parm.capture.timeperframe.numerator = 1;
-	a->parm.capture.timeperframe.denominator = 60;
+
+	fps = v4l2_calc_timeperframe(&s->timings);
+	a->parm.capture.timeperframe.numerator = fps.numerator;
+	a->parm.capture.timeperframe.denominator = fps.denominator;
 	a->parm.capture.readbuffers = 3;
 	return 0;
 }

Jose Abreu

author

2017-03-24 11:15 UTC #7 lore raw

In reply to Jose Abreu

Hi Hans,

Quoted 51 lines · Jose Abreu

Hi all,
This is a follow up patch from this discussion [1]. It should be seen more as a starting point to introduce better handling of time per frame in v4l2. Quoting Hans Verkuil from [1]:
1) "Add a flag V4L2_DV_FL_CAN_DETECT_REDUCED_FPS. If set, then the hw can detect the difference between regular fps and 1000/1001 fps. Note: this is only valid for timings of VIC codes with the V4L2_DV_FL_CAN_REDUCE_FPS flag set."
2) "Allow V4L2_DV_FL_REDUCED_FPS to be used for receivers if V4L2_DV_FL_CAN_DETECT_REDUCED_FPS is set."
3) "For standard VIC codes the pixelclock returned by query_dv_timings is that of the corresponding VIC timing, not what is measured. This will ensure fixed fps values"
4) "g_parm should calculate the fps based on the v4l2_bt_timings struct, looking at the REDUCES_FPS flags. For those receivers that cannot detect the difference, the fps will be 24/30/60 Hz, for those that can detect the difference g_parm can check if both V4L2_DV_FL_CAN_DETECT_REDUCED_FPS and V4L2_DV_FL_REDUCED_FPS are set and reduce the fps by 1000/1001."
-----------
In terms of implementation:
- Point 1) is done in patch 1/3
- Point 2) and 3) should be done by a HDMI Receiver driver (I think?).
- Point 4) is done in patch 2/3.
- The patch 3/3 is a simple implementation (which was not tested) in the cobalt driver
-----------
[1] https://patchwork.kernel.org/patch/9609441/
Best regards,
Jose Miguel Abreu
Cc Carlos Palminha <[email protected]>
Cc Mauro Carvalho Chehab <[email protected]>
Cc Hans Verkuil <[email protected]>
Cc [email protected]
Cc [email protected]
Jose Abreu (3):
[media] videodev2.h: Add new DV flag CAN_DETECT_REDUCED_FPS [media] v4l2-dv-timings: Introduce v4l2_calc_timeperframe helper [media] cobalt: Use v4l2_calc_timeperframe helper

Quoted 6 lines

 drivers/media/pci/cobalt/cobalt-v4l2.c    |  9 +++++--
 drivers/media/v4l2-core/v4l2-dv-timings.c | 39 +++++++++++++++++++++++++++++++
 include/media/v4l2-dv-timings.h           | 11 +++++++++
 include/uapi/linux/videodev2.h            |  7 ++++++
 4 files changed, 64 insertions(+), 2 deletions(-)

Can you please review this series, when possible? And if you could test it on cobalt it would be great :)

Best regards,
Jose Miguel Abreu

Hans Verkuil

2017-03-24 11:24 UTC #8 lore raw

Quoted 62 lines · Jose Abreu

Hi Hans,
Quoted 51 lines · Jose Abreu
Hi all,
This is a follow up patch from this discussion [1]. It should be seen more as a starting point to introduce better handling of time per frame in v4l2. Quoting Hans Verkuil from [1]:
1) "Add a flag V4L2_DV_FL_CAN_DETECT_REDUCED_FPS. If set, then the hw can detect the difference between regular fps and 1000/1001 fps. Note: this is only valid for timings of VIC codes with the V4L2_DV_FL_CAN_REDUCE_FPS flag set."
2) "Allow V4L2_DV_FL_REDUCED_FPS to be used for receivers if V4L2_DV_FL_CAN_DETECT_REDUCED_FPS is set."
3) "For standard VIC codes the pixelclock returned by query_dv_timings is that of the corresponding VIC timing, not what is measured. This will ensure fixed fps values"
4) "g_parm should calculate the fps based on the v4l2_bt_timings struct, looking at the REDUCES_FPS flags. For those receivers that cannot detect the difference, the fps will be 24/30/60 Hz, for those that can detect the difference g_parm can check if both V4L2_DV_FL_CAN_DETECT_REDUCED_FPS and V4L2_DV_FL_REDUCED_FPS are set and reduce the fps by 1000/1001."
-----------
In terms of implementation:
- Point 1) is done in patch 1/3
- Point 2) and 3) should be done by a HDMI Receiver driver (I think?).
- Point 4) is done in patch 2/3.
- The patch 3/3 is a simple implementation (which was not tested) in the cobalt driver
-----------
[1] https://patchwork.kernel.org/patch/9609441/
Best regards,
Jose Miguel Abreu
Cc Carlos Palminha <[email protected]>
Cc Mauro Carvalho Chehab <[email protected]>
Cc Hans Verkuil <[email protected]>
Cc [email protected]
Cc [email protected]
Jose Abreu (3):
[media] videodev2.h: Add new DV flag CAN_DETECT_REDUCED_FPS [media] v4l2-dv-timings: Introduce v4l2_calc_timeperframe helper [media] cobalt: Use v4l2_calc_timeperframe helper
Quoted 6 lines
 drivers/media/pci/cobalt/cobalt-v4l2.c    |  9 +++++--
 drivers/media/v4l2-core/v4l2-dv-timings.c | 39 +++++++++++++++++++++++++++++++
 include/media/v4l2-dv-timings.h           | 11 +++++++++
 include/uapi/linux/videodev2.h            |  7 ++++++
 4 files changed, 64 insertions(+), 2 deletions(-)

Quoted 2 lines

Can you please review this series, when possible? And if you could test it on cobalt it would be great :)

Hopefully next week. Did you have some real-world numbers w.r.t. measured pixelclock frequencies and 60 vs 59.94 Hz and 24 vs 23.976 Hz?

I do want to see that, since this patch series only makes sense if you can actually make use of it to reliably detect the difference.

I will try to test that myself with cobalt, but almost certainly I won't be able to tell the difference; if memory serves it can't detect the freq with high enough precision.

Regards,

Hans

Jose Abreu

author

2017-03-24 11:52 UTC #9 lore raw

Hi Hans,

Quoted 3 lines

Quoted 2 lines
Can you please review this series, when possible? And if you could test it on cobalt it would be great :)
Hopefully next week.

Thanks :)

Quoted 2 lines

Did you have some real-world numbers w.r.t. measured pixelclock frequencies and 60 vs 59.94 Hz and 24 vs 23.976 Hz?

I did make some measurements but I'm afraid I didn't yet test with many sources (I mostly tested with signal generators which should have a higher precision clock than real sources). I have a bunch of players here, I will test them as soon as I can. Regarding precision: for our controller is theoretically and effectively enough: The worst case is for 640x480, and even in that case the difference between 60Hz and 59.94Hz is > 1 unit of the measuring register. This still doesn't solve the problem of having a bad source with a bad clock, but I don't know if we can do much more about that.

Quoted 7 lines

I do want to see that, since this patch series only makes sense if you can actually make use of it to reliably detect the difference.
I will try to test that myself with cobalt, but almost certainly I won't be able to tell the difference; if memory serves it can't detect the freq with high enough precision.

Ok, thanks, this would be great because I didn't test the series exactly "as is" because I'm using 4.10. I did look at vivid driver but it already handles reduced frame rate, so it kind of does what it is proposed in this series. If this helper is integrated in the v4l2 core then I can send the patch to vivid.

Best regards,
Jose Miguel Abreu

Quoted 4 lines

Regards,
Hans

Hans Verkuil

2017-03-24 12:12 UTC #10 lore raw

Quoted 12 lines · Jose Abreu

Hi Hans,
Quoted 3 lines
Quoted 2 lines
Can you please review this series, when possible? And if you could test it on cobalt it would be great :)
Hopefully next week.
Thanks :)
Quoted 2 lines
Did you have some real-world numbers w.r.t. measured pixelclock frequencies and 60 vs 59.94 Hz and 24 vs 23.976 Hz?

Quoted 10 lines

I did make some measurements but I'm afraid I didn't yet test with many sources (I mostly tested with signal generators which should have a higher precision clock than real sources). I have a bunch of players here, I will test them as soon as I can. Regarding precision: for our controller is theoretically and effectively enough: The worst case is for 640x480, and even in that case the difference between 60Hz and 59.94Hz is > 1 unit of the measuring register. This still doesn't solve the problem of having a bad source with a bad clock, but I don't know if we can do much more about that.

I would really like to see a table with different sources sending these different framerates and the value that your HW detects.

If there is an obvious and clear difference, then this feature makes sense. If it is all over the place, then I need to think about this some more.

To be honest, I expect that you will see 'an obvious and clear' difference, but that is no more than a gut feeling at the moment and I would like to see some proper test results.

Quoted 9 lines

Quoted 8 lines
I do want to see that, since this patch series only makes sense if you can actually make use of it to reliably detect the difference.
I will try to test that myself with cobalt, but almost certainly I won't be able to tell the difference; if memory serves it can't detect the freq with high enough precision.

Quoted 5 lines

Ok, thanks, this would be great because I didn't test the series exactly "as is" because I'm using 4.10. I did look at vivid driver but it already handles reduced frame rate, so it kind of does what it is proposed in this series. If this helper is integrated in the v4l2 core then I can send the patch to vivid.

That would be nice to have in vivid.

Regards,

Hans

Jose Abreu

author

2017-03-24 12:21 UTC #11 lore raw

Hi Hans,

Quoted 24 lines · Hans Verkuil

Quoted 10 lines · Jose Abreu
Hi Hans,
Quoted 3 lines
Quoted 2 lines
Can you please review this series, when possible? And if you could test it on cobalt it would be great :)
Hopefully next week.
Thanks :)
Quoted 2 lines
Did you have some real-world numbers w.r.t. measured pixelclock frequencies and 60 vs 59.94 Hz and 24 vs 23.976 Hz?
Quoted 10 lines
I did make some measurements but I'm afraid I didn't yet test with many sources (I mostly tested with signal generators which should have a higher precision clock than real sources). I have a bunch of players here, I will test them as soon as I can. Regarding precision: for our controller is theoretically and effectively enough: The worst case is for 640x480, and even in that case the difference between 60Hz and 59.94Hz is > 1 unit of the measuring register. This still doesn't solve the problem of having a bad source with a bad clock, but I don't know if we can do much more about that.
I would really like to see a table with different sources sending these different framerates and the value that your HW detects.

Quoted 7 lines

If there is an obvious and clear difference, then this feature makes sense. If it is all over the place, then I need to think about this some more.
To be honest, I expect that you will see 'an obvious and clear' difference, but that is no more than a gut feeling at the moment and I would like to see some proper test results.

Ok, I will make a table. The test procedure will be like this:
- Measure pixel clock value using certified HDMI analyzer
- Measure pixel clock using our controller
- Compare the values obtained from analyzer, controller and
the values that the source is telling to send (the value displayed in source menu for example [though, some of them may not discriminate the exact frame rate, thats why analyzer should be used also]).

Seems ok? I will need some time, something like a week because my setup was "borrowed".

Best regards,
Jose Miguel Abreu

Quoted 18 lines

Quoted 4 lines
Quoted 4 lines
I do want to see that, since this patch series only makes sense if you can actually make use of it to reliably detect the difference.
Quoted 8 lines
Quoted 3 lines
I will try to test that myself with cobalt, but almost certainly I won't be able to tell the difference; if memory serves it can't detect the freq with high enough precision.
Ok, thanks, this would be great because I didn't test the series exactly "as is" because I'm using 4.10. I did look at vivid driver but it already handles reduced frame rate, so it kind of does what it is proposed in this series. If this helper is integrated in the v4l2 core then I can send the patch to vivid.
That would be nice to have in vivid.
Regards,
Hans

Hans Verkuil

2017-03-24 12:28 UTC #12 lore raw

Quoted 45 lines · Jose Abreu

Hi Hans,
Quoted 24 lines · Hans Verkuil
Quoted 10 lines · Jose Abreu
Hi Hans,
Quoted 3 lines
Quoted 2 lines
Can you please review this series, when possible? And if you could test it on cobalt it would be great :)
Hopefully next week.
Thanks :)
Quoted 2 lines
Did you have some real-world numbers w.r.t. measured pixelclock frequencies and 60 vs 59.94 Hz and 24 vs 23.976 Hz?
Quoted 10 lines
I did make some measurements but I'm afraid I didn't yet test with many sources (I mostly tested with signal generators which should have a higher precision clock than real sources). I have a bunch of players here, I will test them as soon as I can. Regarding precision: for our controller is theoretically and effectively enough: The worst case is for 640x480, and even in that case the difference between 60Hz and 59.94Hz is > 1 unit of the measuring register. This still doesn't solve the problem of having a bad source with a bad clock, but I don't know if we can do much more about that.
I would really like to see a table with different sources sending these different framerates and the value that your HW detects.
Quoted 7 lines
If there is an obvious and clear difference, then this feature makes sense. If it is all over the place, then I need to think about this some more.
To be honest, I expect that you will see 'an obvious and clear' difference, but that is no more than a gut feeling at the moment and I would like to see some proper test results.
Ok, I will make a table. The test procedure will be like this:
- Measure pixel clock value using certified HDMI analyzer
- Measure pixel clock using our controller
- Compare the values obtained from analyzer, controller and
the values that the source is telling to send (the value displayed in source menu for example [though, some of them may not discriminate the exact frame rate, thats why analyzer should be used also]).

Quoted 2 lines

Seems ok? I will need some time, something like a week because my setup was "borrowed".

That sounds good. Sorry for adding to your workload, but there is no point to have a flag that in practice is meaningless.

I'm actually very curious about the results!

Regards,

Hans

Quoted 23 lines

Best regards,
Jose Miguel Abreu
Quoted 18 lines
Quoted 4 lines
Quoted 4 lines
I do want to see that, since this patch series only makes sense if you can actually make use of it to reliably detect the difference.
Quoted 8 lines
Quoted 3 lines
I will try to test that myself with cobalt, but almost certainly I won't be able to tell the difference; if memory serves it can't detect the freq with high enough precision.
Ok, thanks, this would be great because I didn't test the series exactly "as is" because I'm using 4.10. I did look at vivid driver but it already handles reduced frame rate, so it kind of does what it is proposed in this series. If this helper is integrated in the v4l2 core then I can send the patch to vivid.
That would be nice to have in vivid.
Regards,
Hans

Jose Abreu

author

2017-03-27 12:07 UTC #13 lore raw

Hi Hans,

Quoted 45 lines · Hans Verkuil

Quoted 44 lines · Jose Abreu
Hi Hans,
Quoted 24 lines · Hans Verkuil
Quoted 10 lines · Jose Abreu
Hi Hans,
Quoted 3 lines
Quoted 2 lines
Can you please review this series, when possible? And if you could test it on cobalt it would be great :)
Hopefully next week.
Thanks :)
Quoted 2 lines
Did you have some real-world numbers w.r.t. measured pixelclock frequencies and 60 vs 59.94 Hz and 24 vs 23.976 Hz?
Quoted 10 lines
I did make some measurements but I'm afraid I didn't yet test with many sources (I mostly tested with signal generators which should have a higher precision clock than real sources). I have a bunch of players here, I will test them as soon as I can. Regarding precision: for our controller is theoretically and effectively enough: The worst case is for 640x480, and even in that case the difference between 60Hz and 59.94Hz is > 1 unit of the measuring register. This still doesn't solve the problem of having a bad source with a bad clock, but I don't know if we can do much more about that.
I would really like to see a table with different sources sending these different framerates and the value that your HW detects.
Quoted 7 lines
If there is an obvious and clear difference, then this feature makes sense. If it is all over the place, then I need to think about this some more.
To be honest, I expect that you will see 'an obvious and clear' difference, but that is no more than a gut feeling at the moment and I would like to see some proper test results.
Ok, I will make a table. The test procedure will be like this:
- Measure pixel clock value using certified HDMI analyzer
- Measure pixel clock using our controller
- Compare the values obtained from analyzer, controller and
the values that the source is telling to send (the value displayed in source menu for example [though, some of them may not discriminate the exact frame rate, thats why analyzer should be used also]).

Quoted 6 lines

Quoted 2 lines
Seems ok? I will need some time, something like a week because my setup was "borrowed".
That sounds good. Sorry for adding to your workload, but there is no point to have a flag that in practice is meaningless.
I'm actually very curious about the results!

I managed to do the tests but unfortunately I can't publish the full results (at least until I get approval).

I can say that the results look good. As you expected we have some sources with a bad clock but this is correctly detected by the controller (and also by the HDMI analyzer).

Using the v4l2_calc_framerate function I managed to get this:

| Source | Resolution | v4l2_calc_framerate()
------------------------------------------------------------------------------

| Analyzer 1 | [email protected]         | 59.92
| Analyzer 1 | 640x480@60              | 60
| Analyzer 1 | 1920x1080@60          | 60
| Player 1     | [email protected]     | 59.94
| Player 2     | [email protected]     | 59.93
| Player 3     | [email protected]     | 59.94
| Player 4     | [email protected]     | 59.94
| Player 5     | [email protected]     | 59.93
| Player 6     | 1280x720@50            | 50
| Player 7     | [email protected]     | 59.93
| Player 8     | 1920x1080@60          | 60
| Analyzer 2 | [email protected]         | 59.94
| Analyzer 2 | 720x480@60              | 60
| Analyzer 2 | [email protected]     | 59.93
| Analyzer 2 | 1920x180@60            | 60
| Analyzer 2 | [email protected]     | 23.97
| Analyzer 2 | 3840x2160@24          | 24
| Analyzer 2 | [email protected]     | 29.96
| Analyzer 2 | 3840x2160@30          | 30
| Analyzer 2 | [email protected]     | 59.93
| Analyzer 2 | 3840x2160@60          | 60

------------------------------------------------------------------------------

What do you think? Shall we continue integrating this new patch or drop it?

Best regards,
Jose Miguel Abreu

Quoted 25 lines

Regards,
Hans
Quoted 20 lines
Best regards,
Jose Miguel Abreu
Quoted 17 lines
Quoted 11 lines
Quoted 6 lines
I do want to see that, since this patch series only makes sense if you can actually make use of it to reliably detect the difference.
I will try to test that myself with cobalt, but almost certainly I won't be able to tell the difference; if memory serves it can't detect the freq with high enough precision.
Ok, thanks, this would be great because I didn't test the series exactly "as is" because I'm using 4.10. I did look at vivid driver but it already handles reduced frame rate, so it kind of does what it is proposed in this series. If this helper is integrated in the v4l2 core then I can send the patch to vivid.
That would be nice to have in vivid.
Regards,
Hans

Hans Verkuil

2017-03-28 10:07 UTC #14 lore raw

Quoted 80 lines · Jose Abreu

Hi Hans,
Quoted 45 lines · Hans Verkuil
Quoted 44 lines · Jose Abreu
Hi Hans,
Quoted 24 lines · Hans Verkuil
Quoted 10 lines · Jose Abreu
Hi Hans,
Quoted 3 lines
Quoted 2 lines
Can you please review this series, when possible? And if you could test it on cobalt it would be great :)
Hopefully next week.
Thanks :)
Quoted 2 lines
Did you have some real-world numbers w.r.t. measured pixelclock frequencies and 60 vs 59.94 Hz and 24 vs 23.976 Hz?
Quoted 10 lines
I did make some measurements but I'm afraid I didn't yet test with many sources (I mostly tested with signal generators which should have a higher precision clock than real sources). I have a bunch of players here, I will test them as soon as I can. Regarding precision: for our controller is theoretically and effectively enough: The worst case is for 640x480, and even in that case the difference between 60Hz and 59.94Hz is > 1 unit of the measuring register. This still doesn't solve the problem of having a bad source with a bad clock, but I don't know if we can do much more about that.
I would really like to see a table with different sources sending these different framerates and the value that your HW detects.
Quoted 7 lines
If there is an obvious and clear difference, then this feature makes sense. If it is all over the place, then I need to think about this some more.
To be honest, I expect that you will see 'an obvious and clear' difference, but that is no more than a gut feeling at the moment and I would like to see some proper test results.
Ok, I will make a table. The test procedure will be like this:
- Measure pixel clock value using certified HDMI analyzer
- Measure pixel clock using our controller
- Compare the values obtained from analyzer, controller and
the values that the source is telling to send (the value displayed in source menu for example [though, some of them may not discriminate the exact frame rate, thats why analyzer should be used also]).
Quoted 6 lines
Quoted 2 lines
Seems ok? I will need some time, something like a week because my setup was "borrowed".
That sounds good. Sorry for adding to your workload, but there is no point to have a flag that in practice is meaningless.
I'm actually very curious about the results!
I managed to do the tests but unfortunately I can't publish the full results (at least until I get approval).
I can say that the results look good. As you expected we have some sources with a bad clock but this is correctly detected by the controller (and also by the HDMI analyzer).
Using the v4l2_calc_framerate function I managed to get this:
| Source | Resolution | v4l2_calc_framerate()
------------------------------------------------------------------------------
| Analyzer 1 | [email protected]         | 59.92
| Analyzer 1 | 640x480@60              | 60
| Analyzer 1 | 1920x1080@60          | 60
| Player 1     | [email protected]     | 59.94
| Player 2     | [email protected]     | 59.93
| Player 3     | [email protected]     | 59.94
| Player 4     | [email protected]     | 59.94
| Player 5     | [email protected]     | 59.93
| Player 6     | 1280x720@50            | 50
| Player 7     | [email protected]     | 59.93
| Player 8     | 1920x1080@60          | 60
| Analyzer 2 | [email protected]         | 59.94
| Analyzer 2 | 720x480@60              | 60

Quoted 8 lines

| Analyzer 2 | [email protected]     | 59.93
| Analyzer 2 | 1920x180@60            | 60
| Analyzer 2 | [email protected]     | 23.97
| Analyzer 2 | 3840x2160@24          | 24
| Analyzer 2 | [email protected]     | 29.96
| Analyzer 2 | 3840x2160@30          | 30
| Analyzer 2 | [email protected]     | 59.93
| Analyzer 2 | 3840x2160@60          | 60

Nice!

Are the sources with a bad clock included in these results? I only see deviations of 0.02 at most, so I don't think so.

Quoted 4 lines

------------------------------------------------------------------------------
What do you think? Shall we continue integrating this new patch or drop it?

Yes, we can continue. This is what I wanted to know :-) Thank you for testing this, much appreciated.

Regards,

Hans

Jose Abreu

author

2017-03-29 13:26 UTC #15 lore raw

Hi Hans,

Quoted 88 lines · Hans Verkuil

Quoted 79 lines · Jose Abreu
Hi Hans,
Quoted 45 lines · Hans Verkuil
Quoted 44 lines · Jose Abreu
Hi Hans,
Quoted 24 lines · Hans Verkuil
Quoted 10 lines · Jose Abreu
Hi Hans,
Quoted 3 lines
Quoted 2 lines
Can you please review this series, when possible? And if you could test it on cobalt it would be great :)
Hopefully next week.
Thanks :)
Quoted 2 lines
Did you have some real-world numbers w.r.t. measured pixelclock frequencies and 60 vs 59.94 Hz and 24 vs 23.976 Hz?
Quoted 10 lines
I did make some measurements but I'm afraid I didn't yet test with many sources (I mostly tested with signal generators which should have a higher precision clock than real sources). I have a bunch of players here, I will test them as soon as I can. Regarding precision: for our controller is theoretically and effectively enough: The worst case is for 640x480, and even in that case the difference between 60Hz and 59.94Hz is > 1 unit of the measuring register. This still doesn't solve the problem of having a bad source with a bad clock, but I don't know if we can do much more about that.
I would really like to see a table with different sources sending these different framerates and the value that your HW detects.
Quoted 7 lines
If there is an obvious and clear difference, then this feature makes sense. If it is all over the place, then I need to think about this some more.
To be honest, I expect that you will see 'an obvious and clear' difference, but that is no more than a gut feeling at the moment and I would like to see some proper test results.
Ok, I will make a table. The test procedure will be like this:
- Measure pixel clock value using certified HDMI analyzer
- Measure pixel clock using our controller
- Compare the values obtained from analyzer, controller and
the values that the source is telling to send (the value displayed in source menu for example [though, some of them may not discriminate the exact frame rate, thats why analyzer should be used also]).
Quoted 6 lines
Quoted 2 lines
Seems ok? I will need some time, something like a week because my setup was "borrowed".
That sounds good. Sorry for adding to your workload, but there is no point to have a flag that in practice is meaningless.
I'm actually very curious about the results!
I managed to do the tests but unfortunately I can't publish the full results (at least until I get approval).
I can say that the results look good. As you expected we have some sources with a bad clock but this is correctly detected by the controller (and also by the HDMI analyzer).
Using the v4l2_calc_framerate function I managed to get this:
| Source | Resolution | v4l2_calc_framerate()
------------------------------------------------------------------------------
| Analyzer 1 | [email protected]         | 59.92
| Analyzer 1 | 640x480@60              | 60
| Analyzer 1 | 1920x1080@60          | 60
| Player 1     | [email protected]     | 59.94
| Player 2     | [email protected]     | 59.93
| Player 3     | [email protected]     | 59.94
| Player 4     | [email protected]     | 59.94
| Player 5     | [email protected]     | 59.93
| Player 6     | 1280x720@50            | 50
| Player 7     | [email protected]     | 59.93
| Player 8     | 1920x1080@60          | 60
| Analyzer 2 | [email protected]         | 59.94
| Analyzer 2 | 720x480@60              | 60
Quoted 8 lines
| Analyzer 2 | [email protected]     | 59.93
| Analyzer 2 | 1920x180@60            | 60
| Analyzer 2 | [email protected]     | 23.97
| Analyzer 2 | 3840x2160@24          | 24
| Analyzer 2 | [email protected]     | 29.96
| Analyzer 2 | 3840x2160@30          | 30
| Analyzer 2 | [email protected]     | 59.93
| Analyzer 2 | 3840x2160@60          | 60

Quoted 4 lines

Nice!
Are the sources with a bad clock included in these results? I only see deviations of 0.02 at most, so I don't think so.

The results include all the sources I have to test (Player x indicates a real player available in the market while Analyzer x indicates HDMI protocol analyzer). From the data I've collected the players are the ones with the less precise clock, thats what I was referring as a bad clock. But even with that deviations the algorithm computes the value ok. I think I don't have any player else to test here. Maybe, if you could, test the patch series with cobalt + adv with a player and check the precision? ( I think cobalt uses an adv as subdev, right? )

Quoted 7 lines

Quoted 5 lines
------------------------------------------------------------------------------
What do you think? Shall we continue integrating this new patch or drop it?
Yes, we can continue. This is what I wanted to know :-) Thank you for testing this, much appreciated.

No problem :) Please review the patch series (when you can) so that I can submit a next version.

Best regards,
Jose Miguel Abreu

Quoted 4 lines

Regards,
Hans