Message-ID: <8f8b63cb-6ea4-4620-afec-5f4f228eb743@ideasonboard.com>
Date: Wed, 12 Nov 2025 11:51:09 +0200
From: Tomi Valkeinen <tomi.valkeinen@...asonboard.com>
To: Swamil Jain <s-jain1@...com>, aradhya.bhatia@...ux.dev, devarsht@...com,
 mripard@...nel.org, jyri.sarha@....fi, maarten.lankhorst@...ux.intel.com,
 simona@...ll.ch, airlied@...il.com, tzimmermann@...e.de, h-shenoy@...com
Cc: praneeth@...com, u-kumar1@...com, vigneshr@...com,
 dri-devel@...ts.freedesktop.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v8 1/2] drm/tidss: Remove max_pclk_khz and min_pclk_khz
 from tidss display features

Hi,

On 12/11/2025 11:43, Swamil Jain wrote:
> Hi Tomi,
> 
> On 11/12/25 15:10, Tomi Valkeinen wrote:
>> Hi,
>>
>> On 04/11/2025 17:14, Swamil Jain wrote:
>>> From: Jayesh Choudhary <j-choudhary@...com>
>>>
>>> The TIDSS hardware does not have independent maximum or minimum pixel
>>> clock limits for each video port. Instead, these limits are determined
>>> by the SoC's clock architecture. Previously, this constraint was
>>> modeled using the 'max_pclk_khz' and 'min_pclk_khz' fields in
>>> 'dispc_features', but this approach is static and does not account for
>>> the dynamic behavior of PLLs.
>>>
>>> This patch removes the 'max_pclk_khz' and 'min_pclk_khz' fields from
>>> 'dispc_features'. The correct way to check if a requested mode's pixel
>>> clock is supported is by using 'clk_round_rate()' in the 'mode_valid()'
>>> hook. If the best frequency match for the mode clock falls within the
>>> supported tolerance, it is approved. TIDSS supports a 5% pixel clock
>>> tolerance, which is now reflected in the validation logic.
>>>
>>> This change allows existing DSS-compatible drivers to be reused across
>>> SoCs that only differ in their pixel clock characteristics. The
>>> validation uses 'clk_round_rate()' for each mode, which may introduce
>>> additional delay (about 3.5 ms for 30 modes), but this is generally
>>> negligible. Users desiring faster validation may bypass these calls
>>> selectively, for example, checking only the highest resolution mode,
>>> as shown here[1].
>>>
>>> [1]: https://lore.kernel.org/all/20250704094851.182131-3-j-
>>> choudhary@...com/
>>>
>>> Tested-by: Michael Walle <mwalle@...nel.org>
>>> Reviewed-by: Devarsh Thakkar <devarsht@...com>
>>> Reviewed-by: Tomi Valkeinen <tomi.valkeinen@...asonboard.com>
>>> Signed-off-by: Jayesh Choudhary <j-choudhary@...com>
>>> Signed-off-by: Swamil Jain <s-jain1@...com>
>>> ---
>>>   drivers/gpu/drm/tidss/tidss_dispc.c | 86 +++++++++++------------------
>>>   drivers/gpu/drm/tidss/tidss_dispc.h |  3 -
>>>   2 files changed, 31 insertions(+), 58 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/tidss/tidss_dispc.c b/drivers/gpu/drm/
>>> tidss/tidss_dispc.c
>>> index d0b191c470ca..b11880178cba 100644
>>> --- a/drivers/gpu/drm/tidss/tidss_dispc.c
>>> +++ b/drivers/gpu/drm/tidss/tidss_dispc.c
>>> @@ -57,12 +57,6 @@ static const u16
>>> tidss_k2g_common_regs[DISPC_COMMON_REG_TABLE_LEN] = {
>>>   };
>>>     const struct dispc_features dispc_k2g_feats = {
>>> -    .min_pclk_khz = 4375,
>>> -
>>> -    .max_pclk_khz = {
>>> -        [DISPC_VP_DPI] = 150000,
>>> -    },
>>> -
>>>       /*
>>>        * XXX According TRM the RGB input buffer width up to 2560 should
>>>        *     work on 3 taps, but in practice it only works up to 1280.
>>> @@ -145,11 +139,6 @@ static const u16
>>> tidss_am65x_common_regs[DISPC_COMMON_REG_TABLE_LEN] = {
>>>   };
>>>     const struct dispc_features dispc_am65x_feats = {
>>> -    .max_pclk_khz = {
>>> -        [DISPC_VP_DPI] = 165000,
>>> -        [DISPC_VP_OLDI_AM65X] = 165000,
>>> -    },
>>> -
>>>       .scaling = {
>>>           .in_width_max_5tap_rgb = 1280,
>>>           .in_width_max_3tap_rgb = 2560,
>>> @@ -245,11 +234,6 @@ static const u16
>>> tidss_j721e_common_regs[DISPC_COMMON_REG_TABLE_LEN] = {
>>>   };
>>>     const struct dispc_features dispc_j721e_feats = {
>>> -    .max_pclk_khz = {
>>> -        [DISPC_VP_DPI] = 170000,
>>> -        [DISPC_VP_INTERNAL] = 600000,
>>> -    },
>>> -
>>>       .scaling = {
>>>           .in_width_max_5tap_rgb = 2048,
>>>           .in_width_max_3tap_rgb = 4096,
>>> @@ -316,11 +300,6 @@ const struct dispc_features dispc_j721e_feats = {
>>>   };
>>>     const struct dispc_features dispc_am625_feats = {
>>> -    .max_pclk_khz = {
>>> -        [DISPC_VP_DPI] = 165000,
>>> -        [DISPC_VP_INTERNAL] = 170000,
>>> -    },
>>> -
>>>       .scaling = {
>>>           .in_width_max_5tap_rgb = 1280,
>>>           .in_width_max_3tap_rgb = 2560,
>>> @@ -377,15 +356,6 @@ const struct dispc_features dispc_am625_feats = {
>>>   };
>>>     const struct dispc_features dispc_am62a7_feats = {
>>> -    /*
>>> -     * if the code reaches dispc_mode_valid with VP1,
>>> -     * it should return MODE_BAD.
>>> -     */
>>> -    .max_pclk_khz = {
>>> -        [DISPC_VP_TIED_OFF] = 0,
>>> -        [DISPC_VP_DPI] = 165000,
>>> -    },
>>> -
>>>       .scaling = {
>>>           .in_width_max_5tap_rgb = 1280,
>>>           .in_width_max_3tap_rgb = 2560,
>>> @@ -442,10 +412,6 @@ const struct dispc_features dispc_am62a7_feats = {
>>>   };
>>>     const struct dispc_features dispc_am62l_feats = {
>>> -    .max_pclk_khz = {
>>> -        [DISPC_VP_DPI] = 165000,
>>> -    },
>>> -
>>>       .subrev = DISPC_AM62L,
>>>         .common = "common",
>>> @@ -1333,33 +1299,54 @@ static void dispc_vp_set_default_color(struct
>>> dispc_device *dispc,
>>>               DISPC_OVR_DEFAULT_COLOR2, (v >> 32) & 0xffff);
>>>   }
>>>   +/*
>>> + * Calculate the percentage difference between the requested pixel
>>> clock rate
>>> + * and the effective rate resulting from calculating the clock
>>> divider value.
>>> + */
>>> +unsigned int dispc_pclk_diff(unsigned long rate, unsigned long
>>> real_rate)
>>> +{
>>> +    int r = rate / 100, rr = real_rate / 100;
>>> +
>>> +    return (unsigned int)(abs(((rr - r) * 100) / r));
>>> +}
>>> +
>>> +static inline int check_pixel_clock(struct dispc_device *dispc,
>>> +                 u32 hw_videoport, unsigned long clock)
>>> +{
>>
>> Ah... Sorry, I was quite unclear in my comment to v7. I did not mean
>> mark it as inline. I meant "move it inline", i.e. move this code into
>> the dispc_vp_mode_valid function. This is just a few lines, and having
>> it in a separate function makes it a bit more difficult to understand
>> what are all the checks done in dispc_vp_mode_valid().
>>
>> I can do that change when applying the patches, if that's ok for you.
>>
> 
> Thanks Tomi, please do the required changes.

Actually, now that I was about to do the change, I realized the next
patch adds an if check here. So maybe it's better to keep it as a
separate function. I'll just drop the "inline".

 Tomi

> 
> Regards,
> Swamil
> 
>>   Tomi
>>
>>> +    unsigned long round_clock;
>>> +
>>> +    round_clock = clk_round_rate(dispc->vp_clk[hw_videoport], clock);
>>> +    /*
>>> +     * To keep the check consistent with dispc_vp_set_clk_rate(), we
>>> +     * use the same 5% check here.
>>> +     */
>>> +    if (dispc_pclk_diff(clock, round_clock) > 5)
>>> +        return -EINVAL;
>>> +
>>> +    return 0;
>>> +}
>>> +
>>>   enum drm_mode_status dispc_vp_mode_valid(struct dispc_device *dispc,
>>>                        u32 hw_videoport,
>>>                        const struct drm_display_mode *mode)
>>>   {
>>>       u32 hsw, hfp, hbp, vsw, vfp, vbp;
>>>       enum dispc_vp_bus_type bus_type;
>>> -    int max_pclk;
>>>         bus_type = dispc->feat->vp_bus_type[hw_videoport];
>>>   -    max_pclk = dispc->feat->max_pclk_khz[bus_type];
>>> -
>>> -    if (WARN_ON(max_pclk == 0))
>>> +    if (WARN_ON(bus_type == DISPC_VP_TIED_OFF))
>>>           return MODE_BAD;
>>>   -    if (mode->clock < dispc->feat->min_pclk_khz)
>>> -        return MODE_CLOCK_LOW;
>>> -
>>> -    if (mode->clock > max_pclk)
>>> -        return MODE_CLOCK_HIGH;
>>> -
>>>       if (mode->hdisplay > 4096)
>>>           return MODE_BAD;
>>>         if (mode->vdisplay > 4096)
>>>           return MODE_BAD;
>>>   +    if (check_pixel_clock(dispc, hw_videoport, mode->clock * 1000))
>>> +        return MODE_CLOCK_RANGE;
>>> +
>>>       /* TODO: add interlace support */
>>>       if (mode->flags & DRM_MODE_FLAG_INTERLACE)
>>>           return MODE_NO_INTERLACE;
>>> @@ -1423,17 +1410,6 @@ void dispc_vp_disable_clk(struct dispc_device
>>> *dispc, u32 hw_videoport)
>>>       clk_disable_unprepare(dispc->vp_clk[hw_videoport]);
>>>   }
>>>   -/*
>>> - * Calculate the percentage difference between the requested pixel
>>> clock rate
>>> - * and the effective rate resulting from calculating the clock
>>> divider value.
>>> - */
>>> -unsigned int dispc_pclk_diff(unsigned long rate, unsigned long
>>> real_rate)
>>> -{
>>> -    int r = rate / 100, rr = real_rate / 100;
>>> -
>>> -    return (unsigned int)(abs(((rr - r) * 100) / r));
>>> -}
>>> -
>>>   int dispc_vp_set_clk_rate(struct dispc_device *dispc, u32
>>> hw_videoport,
>>>                 unsigned long rate)
>>>   {
>>> diff --git a/drivers/gpu/drm/tidss/tidss_dispc.h b/drivers/gpu/drm/
>>> tidss/tidss_dispc.h
>>> index 60c1b400eb89..42279312dcc1 100644
>>> --- a/drivers/gpu/drm/tidss/tidss_dispc.h
>>> +++ b/drivers/gpu/drm/tidss/tidss_dispc.h
>>> @@ -77,9 +77,6 @@ enum dispc_dss_subrevision {
>>>   };
>>>     struct dispc_features {
>>> -    int min_pclk_khz;
>>> -    int max_pclk_khz[DISPC_VP_MAX_BUS_TYPE];
>>> -
>>>       struct dispc_features_scaling scaling;
>>>         enum dispc_dss_subrevision subrev;
>>
> 

Powered by blists - more mailing lists