Re: [PATCH] drm/tidss: Decouple max_pclk from tidss feats to remove clock dependency

[Jayesh Choudhary <j-choudhary@ti.com>]

Hello Maxime,

On 18/06/25 14:02, Maxime Ripard wrote:
> Hi,
> 
> On Wed, Jun 18, 2025 at 01:28:04PM +0530, Jayesh Choudhary wrote:
>> TIDSS hardware by itself does not have variable max_pclk for each VP.
>> Each VP supports a fixed maximum pixel clock. K2 devices and AM62*
>> devices uses "ultra-light" version where each VP supports a max of
>> 300MHz whereas J7* devices uses TIDSS where all VP can support a
>> max pclk of 600MHz.
>> The limitation that has been modeled till now comes from the clock
>> (PLL can only be programmed to a particular max value). Due to this
>> we end up using different compatible for each SoC when the clocking
>> architecture changes for VPs, even when the hardware is essentially
>> the same.
>> max_pclk cannot be entirely removed since the display controller
>> should tell if a particular mode clock can be supported or not in crtc's
>> "mode_valid()" call. So remove "max_pclk_khz" from the static display
>> feat and add it to "tidss_device" structure which would be modified in
>> runtime. In mode_valid() call, check if a best frequency match for mode
>> clock can be found or not using "clk_round_rate()". Based on that,
>> propagate "max_pclk" and check max_clk again only if the requested mode
>> clock is greater than saved value. (As the preferred display mode is
>> usually the max resolution, driver ends up checking the maximum clock
>> the first time itself which is used in subsequent checks)
>> Since TIDSS display controller provides clock tolerance of 5%, we use
>> this while checking the max_pclk. Also, move up "dispc_pclk_diff()"
>> before it is called.
>>
>> This will make the existing compatibles reusable.
>>
>> Signed-off-by: Jayesh Choudhary <j-choudhary@ti.com>
> 
> We've generally used 0.5% in drivers with similar requirements, because
> that's what VESA requires for some mode generation techniques (and
> what's required by HDMI too iirc).
> 
> We've never had to revisit it on any driver afaik, so unless you have a
> specific reason to do so, I'd really prefer if you stuck with that as
> well.

I agree with 0.5% but I was just trying to keep it in sync with existing
tidss tolerance.

In tidss_crtc.c when we set the clk rate in crtc_atomic_enable() using
dispc_vp_set_clk_rate(), we are giving it 5% tolerance:
https://web.git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/drivers/gpu/drm/tidss/tidss_dispc.c#n1446

So I kept it 5%.

Warm Regards,
Jayesh


> 
> Maxime