RE: [PATCH] media: xilinx: vtc: Dynamically calculate pixel clock

[Gugulothu, Rajesh]

AMD General

Hi Tom,

A gentle reminder on this patch. When you have a chance, could you please review it.

Thanks,
Rajesh G

>-----Original Message-----
>From: Gugulothu, Rajesh
>Sent: Monday, September 7, 2026 3:21 PM
>To: 'Tomi Valkeinen' <tomi.valkeinen@xxxxxxxxxxxxxxxx>; Laurent Pinchart
><laurent.pinchart@xxxxxxxxxxxxxxxx>; Simek, Michal <michal.simek@xxxxxxx>
>Cc: linux-media@xxxxxxxxxxxxxxx; linux-arm-kernel@xxxxxxxxxxxxxxxxxxx; linux-
>kernel@xxxxxxxxxxxxxxx; Mauro Carvalho Chehab <mchehab@xxxxxxxxxx>
>Subject: RE: [PATCH] media: xilinx: vtc: Dynamically calculate pixel clock
>
>Hi Tomi,
>
>Thanks for the review, and Laurent for looping you in.
>
>>-----Original Message-----
>>From: Tomi Valkeinen <tomi.valkeinen@xxxxxxxxxxxxxxxx>
>>Sent: Thursday, September 3, 2026 3:38 PM
>>To: Gugulothu, Rajesh <rajesh.gugulothu@xxxxxxx>; Laurent Pinchart
>><laurent.pinchart@xxxxxxxxxxxxxxxx>; Simek, Michal
>><michal.simek@xxxxxxx>
>>Cc: linux-media@xxxxxxxxxxxxxxx; linux-arm-kernel@xxxxxxxxxxxxxxxxxxx;
>>linux- kernel@xxxxxxxxxxxxxxx; Mauro Carvalho Chehab
>><mchehab@xxxxxxxxxx>
>>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@xxxxxxx>
>>> ---
>>>   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.
>>
>>  Tomi