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

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

[Rajesh Gugulothu]

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@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);
-- 
2.34.1

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

[Gugulothu, Rajesh]

AMD General

Hi All,

A gentle reminder on this patch. When you have a chance, could you please review it and let me know if any changes are needed?

Thanks,
Rajesh G

>-----Original Message-----
>From: Rajesh Gugulothu <rajesh.gugulothu@amd.com>
>Sent: Wednesday, July 29, 2026 3:48 PM
>To: Laurent Pinchart <laurent.pinchart@ideasonboard.com>; Mauro Carvalho
>Chehab <mchehab@kernel.org>; Simek, Michal <michal.simek@amd.com>
>Cc: Gugulothu, Rajesh <rajesh.gugulothu@amd.com>; linux-media@vger.kernel.org;
>linux-arm-kernel@lists.infradead.org; linux-kernel@vger.kernel.org
>Subject: [PATCH] media: xilinx: vtc: Dynamically calculate pixel clock
>
>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@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);
>--
>2.34.1

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

[Laurent Pinchart]

CC'ing Tomi who is working on Xilinx V4L2 drivers.

On Wed, Jul 29, 2026 at 03:48:08PM +0530, 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@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);

-- 
Regards,

Laurent Pinchart

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

[Tomi Valkeinen]

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@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?

What is "reasonable tolerance", why did you arrive to .01%? Also, the 
actual result is not visible to the user.

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).

  Tomi

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

[Gugulothu, Rajesh]

AMD General

Hi Tomi,

Thanks for the review, and Laurent for looping you in.

>-----Original Message-----
>From: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
>Sent: Thursday, September 3, 2026 3:38 PM
>To: Gugulothu, Rajesh <rajesh.gugulothu@amd.com>; Laurent Pinchart
><laurent.pinchart@ideasonboard.com>; Simek, Michal <michal.simek@amd.com>
>Cc: linux-media@vger.kernel.org; linux-arm-kernel@lists.infradead.org; linux-
>kernel@vger.kernel.org; Mauro Carvalho Chehab <mchehab@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@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.
>
>  Tomi

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@ideasonboard.com>; Laurent Pinchart
><laurent.pinchart@ideasonboard.com>; Simek, Michal <michal.simek@amd.com>
>Cc: linux-media@vger.kernel.org; linux-arm-kernel@lists.infradead.org; linux-
>kernel@vger.kernel.org; Mauro Carvalho Chehab <mchehab@kernel.org>
>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@ideasonboard.com>
>>Sent: Thursday, September 3, 2026 3:38 PM
>>To: Gugulothu, Rajesh <rajesh.gugulothu@amd.com>; Laurent Pinchart
>><laurent.pinchart@ideasonboard.com>; Simek, Michal
>><michal.simek@amd.com>
>>Cc: linux-media@vger.kernel.org; linux-arm-kernel@lists.infradead.org;
>>linux- kernel@vger.kernel.org; Mauro Carvalho Chehab
>><mchehab@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@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.
>>
>>  Tomi

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

[Tomi Valkeinen]

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@ideasonboard.com>
>> Sent: Thursday, September 3, 2026 3:38 PM
>> To: Gugulothu, Rajesh <rajesh.gugulothu@amd.com>; Laurent Pinchart
>> <laurent.pinchart@ideasonboard.com>; Simek, Michal <michal.simek@amd.com>
>> Cc: linux-media@vger.kernel.org; linux-arm-kernel@lists.infradead.org; linux-
>> kernel@vger.kernel.org; Mauro Carvalho Chehab <mchehab@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@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.

  Tomi

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

[Gugulothu, Rajesh]

AMD General

Hi Tomi,

Thanks for the review. Please check my inline comments.

Thanks,
Rajesh G

>-----Original Message-----
>From: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
>Sent: Friday, September 18, 2026 12:29 PM
>To: Gugulothu, Rajesh <rajesh.gugulothu@amd.com>
>Cc: linux-media@vger.kernel.org; linux-arm-kernel@lists.infradead.org; linux-
>kernel@vger.kernel.org; Mauro Carvalho Chehab <mchehab@kernel.org>; Laurent
>Pinchart <laurent.pinchart@ideasonboard.com>; Simek, Michal
><michal.simek@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@ideasonboard.com>
>>> Sent: Thursday, September 3, 2026 3:38 PM
>>> To: Gugulothu, Rajesh <rajesh.gugulothu@amd.com>; Laurent Pinchart
>>> <laurent.pinchart@ideasonboard.com>; Simek, Michal
>>> <michal.simek@amd.com>
>>> Cc: linux-media@vger.kernel.org;
>>> linux-arm-kernel@lists.infradead.org; linux- kernel@vger.kernel.org;
>>> Mauro Carvalho Chehab <mchehab@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@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