[PATCH] media: xilinx: vtc: Dynamically calculate pixel clock
Gugulothu, Rajesh
rajesh.gugulothu at amd.com
Tue Sep 22 23:38:19 PDT 2026
AMD General
Hi Tomi,
Thanks for the review. Please check my inline comments.
Thanks,
Rajesh G
>-----Original Message-----
>From: Tomi Valkeinen <tomi.valkeinen at ideasonboard.com>
>Sent: Friday, September 18, 2026 12:29 PM
>To: Gugulothu, Rajesh <rajesh.gugulothu at amd.com>
>Cc: linux-media at vger.kernel.org; linux-arm-kernel at lists.infradead.org; linux-
>kernel at vger.kernel.org; Mauro Carvalho Chehab <mchehab at kernel.org>; Laurent
>Pinchart <laurent.pinchart at ideasonboard.com>; Simek, Michal
><michal.simek at amd.com>
>Subject: Re: [PATCH] media: xilinx: vtc: Dynamically calculate pixel clock
>
>Hi,
>
>On 07/09/2026 12:50, Gugulothu, Rajesh wrote:
>> AMD General
>>
>> Hi Tomi,
>>
>> Thanks for the review, and Laurent for looping you in.
>>
>>> -----Original Message-----
>>> From: Tomi Valkeinen <tomi.valkeinen at ideasonboard.com>
>>> Sent: Thursday, September 3, 2026 3:38 PM
>>> To: Gugulothu, Rajesh <rajesh.gugulothu at amd.com>; Laurent Pinchart
>>> <laurent.pinchart at ideasonboard.com>; Simek, Michal
>>> <michal.simek at amd.com>
>>> Cc: linux-media at vger.kernel.org;
>>> linux-arm-kernel at lists.infradead.org; linux- kernel at vger.kernel.org;
>>> Mauro Carvalho Chehab <mchehab at kernel.org>
>>> Subject: Re: [PATCH] media: xilinx: vtc: Dynamically calculate pixel
>>> clock
>>>
>>> Hi,
>>>
>>> On 29/07/2026 13:18, Rajesh Gugulothu wrote:
>>>> This update enables the vtc to set the pixel clock based on the
>>>> specified timing parameters. A new fps field is added to struct
>>>> xvtc_config and the pixel rate is computed as fps * hsize * vsize.
>>>> After setting the rate, the actual clock rate is read back and a
>>>> warning is emitted if it deviates beyond a small tolerance.
>>>>
>>>> The pixel rate is computed in unsigned long arithmetic to avoid a
>>>> 32-bit overflow in the fps * hsize * vsize product.
>>>>
>>>> Signed-off-by: Rajesh Gugulothu <rajesh.gugulothu at amd.com>
>>>> ---
>>>> drivers/media/platform/xilinx/xilinx-vtc.c | 19 +++++++++++++++++++
>>>> drivers/media/platform/xilinx/xilinx-vtc.h | 1 +
>>>> 2 files changed, 20 insertions(+)
>>>>
>>>> diff --git a/drivers/media/platform/xilinx/xilinx-vtc.c
>>>> b/drivers/media/platform/xilinx/xilinx-vtc.c
>>>> index 92fec7bb4..695eb2a46 100644
>>>> --- a/drivers/media/platform/xilinx/xilinx-vtc.c
>>>> +++ b/drivers/media/platform/xilinx/xilinx-vtc.c
>>>> @@ -141,6 +141,9 @@
>>>>
>>>> #define XVTC_GENERATOR_GLOBAL_DELAY 0x0104
>>>>
>>>> +/* Value of 1 = .01% */
>>>> +#define XVTC_CLK_MAX_PCT_ERR 1
>>>> +
>>>> /**
>>>> * struct xvtc_device - Xilinx Video Timing Controller device structure
>>>> * @xvip: Xilinx Video IP device
>>>> @@ -175,10 +178,26 @@ int xvtc_generator_start(struct xvtc_device *xvtc,
>>>> const struct xvtc_config *config)
>>>> {
>>>> int ret;
>>>> + unsigned long s_rate;
>>>> + unsigned long g_rate;
>>>> + unsigned long clk_err;
>>>>
>>>> if (!xvtc->has_generator)
>>>> return -ENXIO;
>>>>
>>>> + s_rate = (unsigned long)config->fps * config->hsize * config->vsize;
>>>> + ret = clk_set_rate(xvtc->xvip.clk, s_rate);
>>>> + if (ret < 0)
>>>> + return ret;
>>>> +
>>>> + /* Verify that the clock is within a reasonable tolerance. */
>>>> + g_rate = clk_get_rate(xvtc->xvip.clk);
>>>> + clk_err = (abs(g_rate - s_rate) * 10000) / (s_rate);
>>>> + if (clk_err > XVTC_CLK_MAX_PCT_ERR)
>>>> + dev_warn(xvtc->xvip.dev,
>>>> + "Failed to set clk rate: %lu, actual rate: %lu\n",
>>>> + s_rate, g_rate);
>>>> +
>>>> ret = clk_prepare_enable(xvtc->xvip.clk);
>>>> if (ret < 0)
>>>> return ret;
>>>> diff --git a/drivers/media/platform/xilinx/xilinx-vtc.h
>>>> b/drivers/media/platform/xilinx/xilinx-vtc.h
>>>> index 855845911..0f360ed55 100644
>>>> --- a/drivers/media/platform/xilinx/xilinx-vtc.h
>>>> +++ b/drivers/media/platform/xilinx/xilinx-vtc.h
>>>> @@ -27,6 +27,7 @@ struct xvtc_config {
>>>> unsigned int vsync_start;
>>>> unsigned int vsync_end;
>>>> unsigned int vsize;
>>>> + unsigned int fps;
>>>> };
>>>>
>>>> struct xvtc_device *xvtc_of_get(struct device_node *np);
>>>
>>> In upstream there's a single user for the VTC: the TPG driver. It
>>> doesn't set the fps field, so the above code would always try to set the rate to 0,
>wouldn't it?
>>>
>> You're right. The VTC change was posted without the TPG hunk that sets
>> fps (xtpg_config_vtc(): .fps = xtpg->fi_d / xtpg->fi_n), so upstream
>> fps stays 0 and we end up calling clk_set_rate(clk, 0) and dividing by zero.
>> I will include the TPG change in the series so the VTC code has a real
>> user.
>>
>>> What is "reasonable tolerance", why did you arrive to .01%? Also, the
>>> actual result is not visible to the user.
>>>
>> Agreed, the 0.01% threshold was arbitrary. I will drop both the
>> tolerance and the dev_warn() - a log message is not something userspace can act
>on.
>> Instead the driver can use clk_round_rate() to get the achievable rate
>> and report the actual value through the API rather than dmesg. Does
>> this approach sound fine to you?
>>
>>> I think this is a slightly bigger topic than a single VTC patch.
>>> There should be a userspace API to set the FPS, and the user should
>>> see what was the actual rate he got via the API (instead of a
>>> dev_warn when the rate is off more than an arbitrary tolerance).
>>
>> Agreed. For v2 I will make the frame rate a userspace parameter and
>> report the achieved rate back through the same API, dropping the
>> tolerance/dev_warn heuristic entirely.
>>
>> I plan to use the V4L2 frame-interval interface (VIDIOC_S/G_PARM on
>> the TPG source pad): compute the pixel clock from the requested
>> interval, program it via the VTC, and write the granted interval back
>> so userspace sees what it got. Let me know if you would prefer a
>> different interface (e.g. DV timings or a dedicated control); I would
>> like to settle the API before I respin.
>I think this sounds fine.
>
>Do you have a design to test the TPG? I think Vivado dropped it many years ago,
>and I have never actually ran the TPG driver.
Thanks, I will go with VIDIOC_S/G_PARM on the TPG source pad for v2.
Yes, I have a working internal design with TPG + VTC + framebuffer write,
and Vivado still ships the Video Test Pattern Generator (v_tpg). I will
test v2 on that pipeline before posting it.
>
> Tomi
More information about the linux-arm-kernel
mailing list