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
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(-)
--
1.9.1
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]
Cc: [email protected]
---
include/uapi/linux/videodev2.h | 7 +++++++
1 file changed, 7 insertions(+)
diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
index 45184a2..dd7b426 100644
--- a/include/uapi/linux/videodev2.h
+++ b/include/uapi/linux/videodev2.h
@@ -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) \
--
1.9.1
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]
Cc: [email protected]
---
drivers/media/v4l2-core/v4l2-dv-timings.c | 39 +++++++++++++++++++++++++++++++
include/media/v4l2-dv-timings.h | 11 +++++++++
2 files changed, 50 insertions(+)
diff --git a/drivers/media/v4l2-core/v4l2-dv-timings.c b/drivers/media/v4l2-core/v4l2-dv-timings.c
index 5c8c49d..b7b39c2 100644
--- a/drivers/media/v4l2-core/v4l2-dv-timings.c
+++ b/drivers/media/v4l2-core/v4l2-dv-timings.c
@@ -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);
diff --git a/include/media/v4l2-dv-timings.h b/include/media/v4l2-dv-timings.h
index 61a1889..c1eef08 100644
--- a/include/media/v4l2-dv-timings.h
+++ b/include/media/v4l2-dv-timings.h
@@ -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
--
1.9.1
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]
Cc: [email protected]
---
drivers/media/pci/cobalt/cobalt-v4l2.c | 9 +++++++--
1 file changed, 7 insertions(+), 2 deletions(-)
diff --git a/drivers/media/pci/cobalt/cobalt-v4l2.c b/drivers/media/pci/cobalt/cobalt-v4l2.c
index def4a3b..25532ae 100644
--- a/drivers/media/pci/cobalt/cobalt-v4l2.c
+++ b/drivers/media/pci/cobalt/cobalt-v4l2.c
@@ -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;
}
--
1.9.1
Hi Hans,
On 21-03-2017 11:49, Jose Abreu wrote:
> 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
>
> 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
On 03/24/17 12:03, Jose Abreu wrote:
> Hi Hans,
>
>
> On 21-03-2017 11:49, Jose Abreu wrote:
>> 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
>>
>> 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 :)
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
Hi Hans,
>> Can you please review this series, when possible? And if you
>> could test it on cobalt it would be great :)
> Hopefully next week.
Thanks :)
> 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.
>
> 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
>
> Regards,
>
> Hans
On 03/24/17 12:52, Jose Abreu wrote:
> Hi Hans,
>
>
>>> Can you please review this series, when possible? And if you
>>> could test it on cobalt it would be great :)
>> Hopefully next week.
>
> Thanks :)
>
>> 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.
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.
>
>>
>> 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
Hi Hans,
On 24-03-2017 12:12, Hans Verkuil wrote:
> On 03/24/17 12:52, Jose Abreu wrote:
>> Hi Hans,
>>
>>
>>>> Can you please review this series, when possible? And if you
>>>> could test it on cobalt it would be great :)
>>> Hopefully next week.
>> Thanks :)
>>
>>> 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.
> 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.
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
>
>>> 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
>
On 03/24/17 13:21, Jose Abreu wrote:
> Hi Hans,
>
>
> On 24-03-2017 12:12, Hans Verkuil wrote:
>> On 03/24/17 12:52, Jose Abreu wrote:
>>> Hi Hans,
>>>
>>>
>>>>> Can you please review this series, when possible? And if you
>>>>> could test it on cobalt it would be great :)
>>>> Hopefully next week.
>>> Thanks :)
>>>
>>>> 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.
>> 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.
>
> 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".
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
>
> Best regards,
> Jose Miguel Abreu
>
>>
>>>> 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
>>
>
Hi Hans,
On 24-03-2017 12:28, Hans Verkuil wrote:
> On 03/24/17 13:21, Jose Abreu wrote:
>> Hi Hans,
>>
>>
>> On 24-03-2017 12:12, Hans Verkuil wrote:
>>> On 03/24/17 12:52, Jose Abreu wrote:
>>>> Hi Hans,
>>>>
>>>>
>>>>>> Can you please review this series, when possible? And if you
>>>>>> could test it on cobalt it would be great :)
>>>>> Hopefully next week.
>>>> Thanks :)
>>>>
>>>>> 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.
>>> 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.
>> 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".
> 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
>
> Regards,
>
> Hans
>
>> Best regards,
>> Jose Miguel Abreu
>>
>>>>> 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
>>>
On 27/03/17 13:58, Jose Abreu wrote:
> Hi Hans,
>
>
> On 24-03-2017 12:28, Hans Verkuil wrote:
>> On 03/24/17 13:21, Jose Abreu wrote:
>>> Hi Hans,
>>>
>>>
>>> On 24-03-2017 12:12, Hans Verkuil wrote:
>>>> On 03/24/17 12:52, Jose Abreu wrote:
>>>>> Hi Hans,
>>>>>
>>>>>
>>>>>>> Can you please review this series, when possible? And if you
>>>>>>> could test it on cobalt it would be great :)
>>>>>> Hopefully next week.
>>>>> Thanks :)
>>>>>
>>>>>> 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.
>>>> 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.
>>> 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".
>> 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
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.
> ------------------------------------------------------------------------------
>
> 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
Hi Hans,
On 28-03-2017 11:07, Hans Verkuil wrote:
> On 27/03/17 13:58, Jose Abreu wrote:
>> Hi Hans,
>>
>>
>> On 24-03-2017 12:28, Hans Verkuil wrote:
>>> On 03/24/17 13:21, Jose Abreu wrote:
>>>> Hi Hans,
>>>>
>>>>
>>>> On 24-03-2017 12:12, Hans Verkuil wrote:
>>>>> On 03/24/17 12:52, Jose Abreu wrote:
>>>>>> Hi Hans,
>>>>>>
>>>>>>
>>>>>>>> Can you please review this series, when possible? And if you
>>>>>>>> could test it on cobalt it would be great :)
>>>>>>> Hopefully next week.
>>>>>> Thanks :)
>>>>>>
>>>>>>> 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.
>>>>> 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.
>>>> 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".
>>> 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
> 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? )
>
>> ------------------------------------------------------------------------------
>>
>> 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
>
> Regards,
>
> Hans
Hi Jose,
On 21/03/17 12:49, Jose Abreu wrote:
> 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
>
> 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]
> Cc: [email protected]
> ---
> drivers/media/pci/cobalt/cobalt-v4l2.c | 9 +++++++--
> 1 file changed, 7 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/media/pci/cobalt/cobalt-v4l2.c b/drivers/media/pci/cobalt/cobalt-v4l2.c
> index def4a3b..25532ae 100644
> --- a/drivers/media/pci/cobalt/cobalt-v4l2.c
> +++ b/drivers/media/pci/cobalt/cobalt-v4l2.c
> @@ -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;
> }
>
Hi Hans,
On 30-03-2017 14:42, Hans Verkuil wrote:
> Hi Jose,
>
> On 21/03/17 12:49, Jose Abreu wrote:
>> 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.
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
>
> Regards,
>
> Hans
>
>> 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]
>> Cc: [email protected]
>> ---
>> drivers/media/pci/cobalt/cobalt-v4l2.c | 9 +++++++--
>> 1 file changed, 7 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/media/pci/cobalt/cobalt-v4l2.c b/drivers/media/pci/cobalt/cobalt-v4l2.c
>> index def4a3b..25532ae 100644
>> --- a/drivers/media/pci/cobalt/cobalt-v4l2.c
>> +++ b/drivers/media/pci/cobalt/cobalt-v4l2.c
>> @@ -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;
>> }
>>
Hi Hans,
On 31-03-2017 09:59, Jose Abreu wrote:
> Hi Hans,
>
>
> On 30-03-2017 14:42, Hans Verkuil wrote:
>> Hi Jose,
>>
>> On 21/03/17 12:49, Jose Abreu wrote:
>>> 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.
> 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
>
> Best regards,
> Jose Miguel Abreu
>
>> Regards,
>>
>> Hans
>>
>>> 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]
>>> Cc: [email protected]
>>> ---
>>> drivers/media/pci/cobalt/cobalt-v4l2.c | 9 +++++++--
>>> 1 file changed, 7 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/media/pci/cobalt/cobalt-v4l2.c b/drivers/media/pci/cobalt/cobalt-v4l2.c
>>> index def4a3b..25532ae 100644
>>> --- a/drivers/media/pci/cobalt/cobalt-v4l2.c
>>> +++ b/drivers/media/pci/cobalt/cobalt-v4l2.c
>>> @@ -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;
>>> }
>>>