From: Martin Tůma <[email protected]>
Properly document the function of the mgb4 output frame_rate sysfs parameter
and fix the corner case when the frame rate is set to zero causing a "divide
by zero" kernel panic.
Signed-off-by: Martin Tůma <[email protected]>
---
Documentation/admin-guide/media/mgb4.rst | 8 ++++++--
drivers/media/pci/mgb4/mgb4_sysfs_out.c | 9 +++++----
drivers/media/pci/mgb4/mgb4_vout.c | 12 ++++++------
drivers/media/pci/mgb4/mgb4_vout.h | 2 +-
4 files changed, 18 insertions(+), 13 deletions(-)
diff --git a/Documentation/admin-guide/media/mgb4.rst b/Documentation/admin-guide/media/mgb4.rst
index 2977f74d7e26..6fff886003e2 100644
--- a/Documentation/admin-guide/media/mgb4.rst
+++ b/Documentation/admin-guide/media/mgb4.rst
@@ -228,8 +228,12 @@ Common FPDL3/GMSL output parameters
open.*
**frame_rate** (RW):
- Output video frame rate in frames per second. The default frame rate is
- 60Hz.
+ Output video signal frame rate limit in frames per second. Due to
+ the limited output pixel clock steps, the card can not always generate
+ a frame rate perfectly matching the value required by the connected display.
+ Using this parameter one can limit the frame rate by "crippling" the signal
+ so that the lines are not equal but the signal appears like having the exact
+ frame rate to the connected display. The default frame rate limit is 60Hz.
**hsync_polarity** (RW):
HSYNC signal polarity.
diff --git a/drivers/media/pci/mgb4/mgb4_sysfs_out.c b/drivers/media/pci/mgb4/mgb4_sysfs_out.c
index f67ff2a48329..573aa61c69d4 100644
--- a/drivers/media/pci/mgb4/mgb4_sysfs_out.c
+++ b/drivers/media/pci/mgb4/mgb4_sysfs_out.c
@@ -229,9 +229,9 @@ static ssize_t frame_rate_show(struct device *dev,
struct video_device *vdev = to_video_device(dev);
struct mgb4_vout_dev *voutdev = video_get_drvdata(vdev);
u32 period = mgb4_read_reg(&voutdev->mgbdev->video,
- voutdev->config->regs.frame_period);
+ voutdev->config->regs.frame_limit);
- return sprintf(buf, "%u\n", MGB4_HW_FREQ / period);
+ return sprintf(buf, "%u\n", period ? MGB4_HW_FREQ / period : 0);
}
/*
@@ -245,14 +245,15 @@ static ssize_t frame_rate_store(struct device *dev,
struct video_device *vdev = to_video_device(dev);
struct mgb4_vout_dev *voutdev = video_get_drvdata(vdev);
unsigned long val;
- int ret;
+ int limit, ret;
ret = kstrtoul(buf, 10, &val);
if (ret)
return ret;
+ limit = val ? MGB4_HW_FREQ / val : 0;
mgb4_write_reg(&voutdev->mgbdev->video,
- voutdev->config->regs.frame_period, MGB4_HW_FREQ / val);
+ voutdev->config->regs.frame_limit, limit);
return count;
}
diff --git a/drivers/media/pci/mgb4/mgb4_vout.c b/drivers/media/pci/mgb4/mgb4_vout.c
index a6b55669f0a8..cd001ceaae63 100644
--- a/drivers/media/pci/mgb4/mgb4_vout.c
+++ b/drivers/media/pci/mgb4/mgb4_vout.c
@@ -680,12 +680,12 @@ static void fpga_init(struct mgb4_vout_dev *voutdev)
mgb4_write_reg(video, regs->config, 0x00000011);
mgb4_write_reg(video, regs->resolution,
(DEFAULT_WIDTH << 16) | DEFAULT_HEIGHT);
- mgb4_write_reg(video, regs->hsync, 0x00102020);
- mgb4_write_reg(video, regs->vsync, 0x40020202);
- mgb4_write_reg(video, regs->frame_period, DEFAULT_PERIOD);
+ mgb4_write_reg(video, regs->hsync, 0x00283232);
+ mgb4_write_reg(video, regs->vsync, 0x40141F1E);
+ mgb4_write_reg(video, regs->frame_limit, DEFAULT_PERIOD);
mgb4_write_reg(video, regs->padding, 0x00000000);
- voutdev->freq = mgb4_cmt_set_vout_freq(voutdev, 70000 >> 1) << 1;
+ voutdev->freq = mgb4_cmt_set_vout_freq(voutdev, 61150 >> 1) << 1;
mgb4_write_reg(video, regs->config,
(voutdev->config->id + MGB4_VIN_DEVICES) << 2 | 1 << 4);
@@ -711,8 +711,8 @@ static void debugfs_init(struct mgb4_vout_dev *voutdev)
voutdev->regs[3].offset = voutdev->config->regs.hsync;
voutdev->regs[4].name = "VIDEO_PARAMS_2";
voutdev->regs[4].offset = voutdev->config->regs.vsync;
- voutdev->regs[5].name = "FRAME_PERIOD";
- voutdev->regs[5].offset = voutdev->config->regs.frame_period;
+ voutdev->regs[5].name = "FRAME_LIMIT";
+ voutdev->regs[5].offset = voutdev->config->regs.frame_limit;
voutdev->regs[6].name = "PADDING_PIXELS";
voutdev->regs[6].offset = voutdev->config->regs.padding;
if (has_timeperframe(video)) {
diff --git a/drivers/media/pci/mgb4/mgb4_vout.h b/drivers/media/pci/mgb4/mgb4_vout.h
index ab9b58b1deb7..adc8fe1e7ae6 100644
--- a/drivers/media/pci/mgb4/mgb4_vout.h
+++ b/drivers/media/pci/mgb4/mgb4_vout.h
@@ -19,7 +19,7 @@ struct mgb4_vout_regs {
u32 config;
u32 status;
u32 resolution;
- u32 frame_period;
+ u32 frame_limit;
u32 hsync;
u32 vsync;
u32 padding;
--
2.44.0
On 22/03/2024 16:10, [email protected] wrote:
> From: Martin Tůma <[email protected]>
>
> Properly document the function of the mgb4 output frame_rate sysfs parameter
> and fix the corner case when the frame rate is set to zero causing a "divide
> by zero" kernel panic.
This is mixing a fix and a documentation improvement into one patch. This
should be split.
Also, shouldn't the fix be either part of the previous patch or come before
that patch?
>
> Signed-off-by: Martin Tůma <[email protected]>
> ---
> Documentation/admin-guide/media/mgb4.rst | 8 ++++++--
> drivers/media/pci/mgb4/mgb4_sysfs_out.c | 9 +++++----
> drivers/media/pci/mgb4/mgb4_vout.c | 12 ++++++------
> drivers/media/pci/mgb4/mgb4_vout.h | 2 +-
> 4 files changed, 18 insertions(+), 13 deletions(-)
>
> diff --git a/Documentation/admin-guide/media/mgb4.rst b/Documentation/admin-guide/media/mgb4.rst
> index 2977f74d7e26..6fff886003e2 100644
> --- a/Documentation/admin-guide/media/mgb4.rst
> +++ b/Documentation/admin-guide/media/mgb4.rst
> @@ -228,8 +228,12 @@ Common FPDL3/GMSL output parameters
> open.*
>
> **frame_rate** (RW):
> - Output video frame rate in frames per second. The default frame rate is
> - 60Hz.
> + Output video signal frame rate limit in frames per second. Due to
> + the limited output pixel clock steps, the card can not always generate
> + a frame rate perfectly matching the value required by the connected display.
> + Using this parameter one can limit the frame rate by "crippling" the signal
> + so that the lines are not equal but the signal appears like having the exact
> + frame rate to the connected display. The default frame rate limit is 60Hz.
It's not clear what is meant with 'crippling'. Normally when dealing with video
framerates the driver will pick the closest video timing to the requested framerate.
It is understood that you can't always get the exact framerate, so drivers can
make adjustments.
Regards,
Hans
>
> **hsync_polarity** (RW):
> HSYNC signal polarity.
> diff --git a/drivers/media/pci/mgb4/mgb4_sysfs_out.c b/drivers/media/pci/mgb4/mgb4_sysfs_out.c
> index f67ff2a48329..573aa61c69d4 100644
> --- a/drivers/media/pci/mgb4/mgb4_sysfs_out.c
> +++ b/drivers/media/pci/mgb4/mgb4_sysfs_out.c
> @@ -229,9 +229,9 @@ static ssize_t frame_rate_show(struct device *dev,
> struct video_device *vdev = to_video_device(dev);
> struct mgb4_vout_dev *voutdev = video_get_drvdata(vdev);
> u32 period = mgb4_read_reg(&voutdev->mgbdev->video,
> - voutdev->config->regs.frame_period);
> + voutdev->config->regs.frame_limit);
>
> - return sprintf(buf, "%u\n", MGB4_HW_FREQ / period);
> + return sprintf(buf, "%u\n", period ? MGB4_HW_FREQ / period : 0);
> }
>
> /*
> @@ -245,14 +245,15 @@ static ssize_t frame_rate_store(struct device *dev,
> struct video_device *vdev = to_video_device(dev);
> struct mgb4_vout_dev *voutdev = video_get_drvdata(vdev);
> unsigned long val;
> - int ret;
> + int limit, ret;
>
> ret = kstrtoul(buf, 10, &val);
> if (ret)
> return ret;
>
> + limit = val ? MGB4_HW_FREQ / val : 0;
> mgb4_write_reg(&voutdev->mgbdev->video,
> - voutdev->config->regs.frame_period, MGB4_HW_FREQ / val);
> + voutdev->config->regs.frame_limit, limit);
>
> return count;
> }
> diff --git a/drivers/media/pci/mgb4/mgb4_vout.c b/drivers/media/pci/mgb4/mgb4_vout.c
> index a6b55669f0a8..cd001ceaae63 100644
> --- a/drivers/media/pci/mgb4/mgb4_vout.c
> +++ b/drivers/media/pci/mgb4/mgb4_vout.c
> @@ -680,12 +680,12 @@ static void fpga_init(struct mgb4_vout_dev *voutdev)
> mgb4_write_reg(video, regs->config, 0x00000011);
> mgb4_write_reg(video, regs->resolution,
> (DEFAULT_WIDTH << 16) | DEFAULT_HEIGHT);
> - mgb4_write_reg(video, regs->hsync, 0x00102020);
> - mgb4_write_reg(video, regs->vsync, 0x40020202);
> - mgb4_write_reg(video, regs->frame_period, DEFAULT_PERIOD);
> + mgb4_write_reg(video, regs->hsync, 0x00283232);
> + mgb4_write_reg(video, regs->vsync, 0x40141F1E);
> + mgb4_write_reg(video, regs->frame_limit, DEFAULT_PERIOD);
> mgb4_write_reg(video, regs->padding, 0x00000000);
>
> - voutdev->freq = mgb4_cmt_set_vout_freq(voutdev, 70000 >> 1) << 1;
> + voutdev->freq = mgb4_cmt_set_vout_freq(voutdev, 61150 >> 1) << 1;
>
> mgb4_write_reg(video, regs->config,
> (voutdev->config->id + MGB4_VIN_DEVICES) << 2 | 1 << 4);
> @@ -711,8 +711,8 @@ static void debugfs_init(struct mgb4_vout_dev *voutdev)
> voutdev->regs[3].offset = voutdev->config->regs.hsync;
> voutdev->regs[4].name = "VIDEO_PARAMS_2";
> voutdev->regs[4].offset = voutdev->config->regs.vsync;
> - voutdev->regs[5].name = "FRAME_PERIOD";
> - voutdev->regs[5].offset = voutdev->config->regs.frame_period;
> + voutdev->regs[5].name = "FRAME_LIMIT";
> + voutdev->regs[5].offset = voutdev->config->regs.frame_limit;
> voutdev->regs[6].name = "PADDING_PIXELS";
> voutdev->regs[6].offset = voutdev->config->regs.padding;
> if (has_timeperframe(video)) {
> diff --git a/drivers/media/pci/mgb4/mgb4_vout.h b/drivers/media/pci/mgb4/mgb4_vout.h
> index ab9b58b1deb7..adc8fe1e7ae6 100644
> --- a/drivers/media/pci/mgb4/mgb4_vout.h
> +++ b/drivers/media/pci/mgb4/mgb4_vout.h
> @@ -19,7 +19,7 @@ struct mgb4_vout_regs {
> u32 config;
> u32 status;
> u32 resolution;
> - u32 frame_period;
> + u32 frame_limit;
> u32 hsync;
> u32 vsync;
> u32 padding;
Hi,
On 08. 04. 24 12:47, Hans Verkuil wrote:
> On 22/03/2024 16:10, [email protected] wrote:
>> From: Martin Tůma <[email protected]>
>>
>> Properly document the function of the mgb4 output frame_rate sysfs parameter
>> and fix the corner case when the frame rate is set to zero causing a "divide
>> by zero" kernel panic.
>
> This is mixing a fix and a documentation improvement into one patch. This
> should be split.
>
Well, the "core" of the patch is really just the correct description
what the "magic" frame_rate sysfs parameter really does. The code "just"
adjusts the internal naming to better fit the real purpose of the parameter.
The remaining lines change the default timings parameters so that the
default gives a correct "timings equation" without the need of
"crippling" the signal using the frame rate limit. While the numbers are
different, the outcome is the same - the defaults are still for the same
default display (Škoda Octavia) as before, but the numbers make more
sense (and the signal on the line is not "crippled")
> Also, shouldn't the fix be either part of the previous patch or come before
> that patch?
>
While "detected" when fiddling with the previous patch and "by name" it
looks like it has something to do with it, in reality it is something
different. In the V1 patch it was even send together with the
V4L2_CAP_TIMEPERFRAME patch but I have then explicitly splitted it to
make clear it is something different. This change (and our "magic"
frame_rate sysfs parameter) affects the signal on the line while the
V4L2_CAP_TIMEPERFRAME is about the frame rate the user uses to provide
the frames to the card and should be a standard v4l2 mechanism.
M.
>>
>> Signed-off-by: Martin Tůma <[email protected]>
>> ---
>> Documentation/admin-guide/media/mgb4.rst | 8 ++++++--
>> drivers/media/pci/mgb4/mgb4_sysfs_out.c | 9 +++++----
>> drivers/media/pci/mgb4/mgb4_vout.c | 12 ++++++------
>> drivers/media/pci/mgb4/mgb4_vout.h | 2 +-
>> 4 files changed, 18 insertions(+), 13 deletions(-)
>>
>> diff --git a/Documentation/admin-guide/media/mgb4.rst b/Documentation/admin-guide/media/mgb4.rst
>> index 2977f74d7e26..6fff886003e2 100644
>> --- a/Documentation/admin-guide/media/mgb4.rst
>> +++ b/Documentation/admin-guide/media/mgb4.rst
>> @@ -228,8 +228,12 @@ Common FPDL3/GMSL output parameters
>> open.*
>>
>> **frame_rate** (RW):
>> - Output video frame rate in frames per second. The default frame rate is
>> - 60Hz.
>> + Output video signal frame rate limit in frames per second. Due to
>> + the limited output pixel clock steps, the card can not always generate
>> + a frame rate perfectly matching the value required by the connected display.
>> + Using this parameter one can limit the frame rate by "crippling" the signal
>> + so that the lines are not equal but the signal appears like having the exact
>> + frame rate to the connected display. The default frame rate limit is 60Hz.
>
> It's not clear what is meant with 'crippling'. Normally when dealing with video
> framerates the driver will pick the closest video timing to the requested framerate.
> It is understood that you can't always get the exact framerate, so drivers can
> make adjustments.
>
> Regards,
>
> Hans
>
>>
>> **hsync_polarity** (RW):
>> HSYNC signal polarity.
>> diff --git a/drivers/media/pci/mgb4/mgb4_sysfs_out.c b/drivers/media/pci/mgb4/mgb4_sysfs_out.c
>> index f67ff2a48329..573aa61c69d4 100644
>> --- a/drivers/media/pci/mgb4/mgb4_sysfs_out.c
>> +++ b/drivers/media/pci/mgb4/mgb4_sysfs_out.c
>> @@ -229,9 +229,9 @@ static ssize_t frame_rate_show(struct device *dev,
>> struct video_device *vdev = to_video_device(dev);
>> struct mgb4_vout_dev *voutdev = video_get_drvdata(vdev);
>> u32 period = mgb4_read_reg(&voutdev->mgbdev->video,
>> - voutdev->config->regs.frame_period);
>> + voutdev->config->regs.frame_limit);
>>
>> - return sprintf(buf, "%u\n", MGB4_HW_FREQ / period);
>> + return sprintf(buf, "%u\n", period ? MGB4_HW_FREQ / period : 0);
>> }
>>
>> /*
>> @@ -245,14 +245,15 @@ static ssize_t frame_rate_store(struct device *dev,
>> struct video_device *vdev = to_video_device(dev);
>> struct mgb4_vout_dev *voutdev = video_get_drvdata(vdev);
>> unsigned long val;
>> - int ret;
>> + int limit, ret;
>>
>> ret = kstrtoul(buf, 10, &val);
>> if (ret)
>> return ret;
>>
>> + limit = val ? MGB4_HW_FREQ / val : 0;
>> mgb4_write_reg(&voutdev->mgbdev->video,
>> - voutdev->config->regs.frame_period, MGB4_HW_FREQ / val);
>> + voutdev->config->regs.frame_limit, limit);
>>
>> return count;
>> }
>> diff --git a/drivers/media/pci/mgb4/mgb4_vout.c b/drivers/media/pci/mgb4/mgb4_vout.c
>> index a6b55669f0a8..cd001ceaae63 100644
>> --- a/drivers/media/pci/mgb4/mgb4_vout.c
>> +++ b/drivers/media/pci/mgb4/mgb4_vout.c
>> @@ -680,12 +680,12 @@ static void fpga_init(struct mgb4_vout_dev *voutdev)
>> mgb4_write_reg(video, regs->config, 0x00000011);
>> mgb4_write_reg(video, regs->resolution,
>> (DEFAULT_WIDTH << 16) | DEFAULT_HEIGHT);
>> - mgb4_write_reg(video, regs->hsync, 0x00102020);
>> - mgb4_write_reg(video, regs->vsync, 0x40020202);
>> - mgb4_write_reg(video, regs->frame_period, DEFAULT_PERIOD);
>> + mgb4_write_reg(video, regs->hsync, 0x00283232);
>> + mgb4_write_reg(video, regs->vsync, 0x40141F1E);
>> + mgb4_write_reg(video, regs->frame_limit, DEFAULT_PERIOD);
>> mgb4_write_reg(video, regs->padding, 0x00000000);
>>
>> - voutdev->freq = mgb4_cmt_set_vout_freq(voutdev, 70000 >> 1) << 1;
>> + voutdev->freq = mgb4_cmt_set_vout_freq(voutdev, 61150 >> 1) << 1;
>>
>> mgb4_write_reg(video, regs->config,
>> (voutdev->config->id + MGB4_VIN_DEVICES) << 2 | 1 << 4);
>> @@ -711,8 +711,8 @@ static void debugfs_init(struct mgb4_vout_dev *voutdev)
>> voutdev->regs[3].offset = voutdev->config->regs.hsync;
>> voutdev->regs[4].name = "VIDEO_PARAMS_2";
>> voutdev->regs[4].offset = voutdev->config->regs.vsync;
>> - voutdev->regs[5].name = "FRAME_PERIOD";
>> - voutdev->regs[5].offset = voutdev->config->regs.frame_period;
>> + voutdev->regs[5].name = "FRAME_LIMIT";
>> + voutdev->regs[5].offset = voutdev->config->regs.frame_limit;
>> voutdev->regs[6].name = "PADDING_PIXELS";
>> voutdev->regs[6].offset = voutdev->config->regs.padding;
>> if (has_timeperframe(video)) {
>> diff --git a/drivers/media/pci/mgb4/mgb4_vout.h b/drivers/media/pci/mgb4/mgb4_vout.h
>> index ab9b58b1deb7..adc8fe1e7ae6 100644
>> --- a/drivers/media/pci/mgb4/mgb4_vout.h
>> +++ b/drivers/media/pci/mgb4/mgb4_vout.h
>> @@ -19,7 +19,7 @@ struct mgb4_vout_regs {
>> u32 config;
>> u32 status;
>> u32 resolution;
>> - u32 frame_period;
>> + u32 frame_limit;
>> u32 hsync;
>> u32 vsync;
>> u32 padding;
>
On 08. 04. 24 12:47, Hans Verkuil wrote:
>> diff --git a/Documentation/admin-guide/media/mgb4.rst b/Documentation/admin-guide/media/mgb4.rst
>> index 2977f74d7e26..6fff886003e2 100644
>> --- a/Documentation/admin-guide/media/mgb4.rst
>> +++ b/Documentation/admin-guide/media/mgb4.rst
>> @@ -228,8 +228,12 @@ Common FPDL3/GMSL output parameters
>> open.*
>>
>> **frame_rate** (RW):
>> - Output video frame rate in frames per second. The default frame rate is
>> - 60Hz.
>> + Output video signal frame rate limit in frames per second. Due to
>> + the limited output pixel clock steps, the card can not always generate
>> + a frame rate perfectly matching the value required by the connected display.
>> + Using this parameter one can limit the frame rate by "crippling" the signal
>> + so that the lines are not equal but the signal appears like having the exact
>> + frame rate to the connected display. The default frame rate limit is 60Hz.
>
> It's not clear what is meant with 'crippling'. Normally when dealing with video
> framerates the driver will pick the closest video timing to the requested framerate.
> It is understood that you can't always get the exact framerate, so drivers can
> make adjustments.
>
By "crippling" I mean the signal is modified in a obscure way so that a
frame has not all lines equal. The HW somehow (the exact way is not
known to me, the documentation is very sparse on this - before my
investigation it stated only "frames per second" which was very
confusing and the reason this patch exists) modifies the last
line(lines?) so that the overall clock ticks per frame is the desired
value. Some blanking stuff (the porches?) you have set are not equal for
all the frame lines.
What I'm trying to do is to change the original documentation which is
definitely wrong (the users are confused how you can set the timings AND
the frame rate with different values at the same time) without saying
too much about the exact algorithm as it is not exactly known and may
even slightly change in different FW versions.
Does that all make sense to you now?