| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <d0ab6151-3ac7-4366-bdae-0b3c58518fb9@ti.com>
Date: Wed, 12 Nov 2025 15:30:23 +0530
From: Swamil Jain <s-jain1@...com>
To: Tomi Valkeinen <tomi.valkeinen@...asonboard.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 Tomi,
On 11/12/25 15:21, Tomi Valkeinen wrote:
> 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".
Yeah, I think we can move it inline and accordingly modify the next
patch also. But I think having a separate function makes it clear that
we are skipping check for VPs with external clocking and we are having a
similar check within the bridge driver. Please drop the "inline". Thanks
for the re-work.
Regards,
Swamil
>
> 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