Re: [PATCH for drm-misc-fixes v5 4/4] drm/hisilicon/hibmc: use clock to look up the PLL value
From: Yongbang Shi
Date: Tue Apr 28 2026 - 02:14:23 EST
> Hi
>
> Am 23.04.26 um 08:32 schrieb Yongbang Shi:
>> From: Lin He <helin52@xxxxxxxxxx>
>>
>> In the past, we use width and height to look up our PLL value.
>> But actually the actual clock check is also necessnary. There are
>> some resolutions that width and height same, but its clock different.
>> Add the clock check when using pll_table to determine the PLL value.
>>
>> Fixes: da52605eea8f ("drm/hisilicon/hibmc: Add support for display engine")
>> Signed-off-by: Lin He <helin52@xxxxxxxxxx>
>> Signed-off-by: Yongbang Shi <shiyongbang@xxxxxxxxxx>
>> ---
>> ChangeLog:
>> v2 -> v3:
>> - remove unused macro CLOCK_TOLERANCE.
>> v1 -> v2:
>> - remove tag "Reviewed-by: Tao Tian <tiantao6@xxxxxxxxxxxxx>", witch will
>> be given in public.
>> - add 'drm-misc-fixes' in subject prefix.
>> ---
>> .../gpu/drm/hisilicon/hibmc/hibmc_drm_de.c | 78 +++++++++++--------
>> 1 file changed, 44 insertions(+), 34 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_de.c b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_de.c
>> index 89bed78f1466..1a07e8146eee 100644
>> --- a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_de.c
>> +++ b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_de.c
>> @@ -32,26 +32,43 @@ struct hibmc_display_panel_pll {
>> struct hibmc_dislay_pll_config {
>> u64 hdisplay;
>> u64 vdisplay;
>> + int clock;
>> u32 pll1_config_value;
>> u32 pll2_config_value;
>> };
>> static const struct hibmc_dislay_pll_config hibmc_pll_table[] = {
>> - {640, 480, CRT_PLL1_HS_25MHZ, CRT_PLL2_HS_25MHZ},
>> - {800, 600, CRT_PLL1_HS_40MHZ, CRT_PLL2_HS_40MHZ},
>> - {1024, 768, CRT_PLL1_HS_65MHZ, CRT_PLL2_HS_65MHZ},
>> - {1152, 864, CRT_PLL1_HS_80MHZ_1152, CRT_PLL2_HS_80MHZ},
>> - {1280, 768, CRT_PLL1_HS_80MHZ, CRT_PLL2_HS_80MHZ},
>> - {1280, 720, CRT_PLL1_HS_74MHZ, CRT_PLL2_HS_74MHZ},
>> - {1280, 960, CRT_PLL1_HS_108MHZ, CRT_PLL2_HS_108MHZ},
>> - {1280, 1024, CRT_PLL1_HS_108MHZ, CRT_PLL2_HS_108MHZ},
>> - {1440, 900, CRT_PLL1_HS_106MHZ, CRT_PLL2_HS_106MHZ},
>> - {1600, 900, CRT_PLL1_HS_108MHZ, CRT_PLL2_HS_108MHZ},
>> - {1600, 1200, CRT_PLL1_HS_162MHZ, CRT_PLL2_HS_162MHZ},
>> - {1920, 1080, CRT_PLL1_HS_148MHZ, CRT_PLL2_HS_148MHZ},
>> - {1920, 1200, CRT_PLL1_HS_193MHZ, CRT_PLL2_HS_193MHZ},
>> + {640, 480, 25000, CRT_PLL1_HS_25MHZ, CRT_PLL2_HS_25MHZ},
>> + {800, 600, 40000, CRT_PLL1_HS_40MHZ, CRT_PLL2_HS_40MHZ},
>> + {1024, 768, 65000, CRT_PLL1_HS_65MHZ, CRT_PLL2_HS_65MHZ},
>> + {1152, 864, 78750, CRT_PLL1_HS_80MHZ_1152, CRT_PLL2_HS_80MHZ},
>> + {1280, 768, 80000, CRT_PLL1_HS_80MHZ, CRT_PLL2_HS_80MHZ},
>> + {1280, 720, 74375, CRT_PLL1_HS_74MHZ, CRT_PLL2_HS_74MHZ},
>> + {1280, 960, 108000, CRT_PLL1_HS_108MHZ, CRT_PLL2_HS_108MHZ},
>> + {1280, 1024, 108000, CRT_PLL1_HS_108MHZ, CRT_PLL2_HS_108MHZ},
>> + {1440, 900, 105952, CRT_PLL1_HS_106MHZ, CRT_PLL2_HS_106MHZ},
>> + {1600, 900, 108000, CRT_PLL1_HS_108MHZ, CRT_PLL2_HS_108MHZ},
>> + {1600, 1200, 162500, CRT_PLL1_HS_162MHZ, CRT_PLL2_HS_162MHZ},
>> + {1920, 1080, 148750, CRT_PLL1_HS_148MHZ, CRT_PLL2_HS_148MHZ},
>> + {1920, 1200, 193750, CRT_PLL1_HS_193MHZ, CRT_PLL2_HS_193MHZ},
>> };
>> +static int hibmc_get_best_clock_idx(const struct drm_display_mode *mode)
>> +{
>> + int i, diff;
>> +
>> + for (i = 0; i < ARRAY_SIZE(hibmc_pll_table); i++) {
>> + if (hibmc_pll_table[i].hdisplay == mode->hdisplay &&
>> + hibmc_pll_table[i].vdisplay == mode->vdisplay) {
>> + diff = abs(mode->clock - hibmc_pll_table[i].clock);
>> + if (diff < mode->clock / 100) /* tolerance 1/100 */
>> + return i;
>> + }
>> + }
>> +
>> + return -EOPNOTSUPP;
>
> This errno code is for sockets.
>
> Rather return -MODE_CLOCK_RANGE here, which you can then return from .mode_valid as well.
>
Okay. I will fix in v6.
Thanks,
Yongbang.
> Best regards
> Thomas
>
>
>> +}
>> +
>> static int hibmc_plane_atomic_check(struct drm_plane *plane,
>> struct drm_atomic_state *state)
>> {
>> @@ -214,17 +231,13 @@ static enum drm_mode_status
>> hibmc_crtc_mode_valid(struct drm_crtc *crtc,
>> const struct drm_display_mode *mode)
>> {
>> - size_t i = 0;
>> int vrefresh = drm_mode_vrefresh(mode);
>> if (vrefresh < 59 || vrefresh > 61)
>> return MODE_NOCLOCK;
>> - for (i = 0; i < ARRAY_SIZE(hibmc_pll_table); i++) {
>> - if (hibmc_pll_table[i].hdisplay == mode->hdisplay &&
>> - hibmc_pll_table[i].vdisplay == mode->vdisplay)
>> - return MODE_OK;
>> - }
>> + if (hibmc_get_best_clock_idx(mode) >= 0)
>> + return MODE_OK;
>> return MODE_BAD;
>> }
>> @@ -281,23 +294,20 @@ static void set_vclock_hisilicon(struct drm_device *dev, u64 pll)
>> writel(val, priv->mmio + CRT_PLL1_HS);
>> }
>> -static void get_pll_config(u64 x, u64 y, u32 *pll1, u32 *pll2)
>> +static void get_pll_config(struct drm_display_mode *mode, u32 *pll1, u32 *pll2)
>> {
>> - size_t i;
>> - size_t count = ARRAY_SIZE(hibmc_pll_table);
>> -
>> - for (i = 0; i < count; i++) {
>> - if (hibmc_pll_table[i].hdisplay == x &&
>> - hibmc_pll_table[i].vdisplay == y) {
>> - *pll1 = hibmc_pll_table[i].pll1_config_value;
>> - *pll2 = hibmc_pll_table[i].pll2_config_value;
>> - return;
>> - }
>> + int idx;
>> +
>> + idx = hibmc_get_best_clock_idx(mode);
>> + if (idx < 0) {
>> + /* if found none, we use default value */
>> + *pll1 = CRT_PLL1_HS_25MHZ;
>> + *pll2 = CRT_PLL2_HS_25MHZ;
>> + return;
>> }
>> - /* if found none, we use default value */
>> - *pll1 = CRT_PLL1_HS_25MHZ;
>> - *pll2 = CRT_PLL2_HS_25MHZ;
>> + *pll1 = hibmc_pll_table[idx].pll1_config_value;
>> + *pll2 = hibmc_pll_table[idx].pll2_config_value;
>> }
>> /*
>> @@ -319,7 +329,7 @@ static u32 display_ctrl_adjust(struct drm_device *dev,
>> x = mode->hdisplay;
>> y = mode->vdisplay;
>> - get_pll_config(x, y, &pll1, &pll2);
>> + get_pll_config(mode, &pll1, &pll2);
>> writel(pll2, priv->mmio + CRT_PLL2_HS);
>> set_vclock_hisilicon(dev, pll1);
>>
>