Re: [PATCH v3 2/3] drm/tidss: Remove max_pclk_khz from tidss display features

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

Hello Devarsh,

On 01/07/25 18:42, Devarsh Thakkar wrote:
> Hi Jayesh,
> 
> On 01/07/25 15:25, Jayesh Choudhary wrote:
>> TIDSS hardware by itself does not have variable max_pclk for each VP.
>> The maximum pixel clock is determined by the limiting factor between
>> the functional clock and the PLL
> 
> and the pixel clock

PLL is the parent to that pixel clock so seemed implicit to me.
But will make it more clear.

> .
>>
>> The limitation that has been modeled till now comes from the clock
>> (PLL can only be programmed to a particular max value). Instead of
>> putting it as a constant field in dispc_features, we can query the
>> DM to see if requested clock can be set or not and use it in
>> "mode_valid()".
>>
>> Replace constant "max_pclk_khz" in dispc_features with "curr_max_pclk"
>> in 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
>> "cur_max_pclk" and query DM again only if the requested mode clock
>> is greater than cur_max_pclk. (As the preferred display mode is usually
>> the max resolution, driver ends up checking the highest 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 curr_max_pclk. Also, move up "dispc_pclk_diff()"
>> before it is called.
>>
>> This will make the existing compatibles reusable
> 
> reusable if DSS features are same across two SoCs and only difference 
> being the pixel clock.

Sure.

> 
>>
>> Signed-off-by: Jayesh Choudhary <j-choudhary@ti.com>
>> ---
>>   drivers/gpu/drm/tidss/tidss_dispc.c | 77 +++++++++++------------------
>>   drivers/gpu/drm/tidss/tidss_dispc.h |  1 -
>>   drivers/gpu/drm/tidss/tidss_drv.h   |  2 +
>>   3 files changed, 31 insertions(+), 49 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/tidss/tidss_dispc.c 
>> b/drivers/gpu/drm/tidss/tidss_dispc.c
>> index 3f6cff2ab1b2..fb59a6a0f86a 100644
>> --- a/drivers/gpu/drm/tidss/tidss_dispc.c
>> +++ b/drivers/gpu/drm/tidss/tidss_dispc.c
>> @@ -58,10 +58,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.
>> @@ -144,11 +140,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,
>> @@ -244,11 +235,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,
>> @@ -315,11 +301,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,
>> @@ -376,15 +357,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,
>> @@ -441,10 +413,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",
>> @@ -1347,25 +1315,49 @@ 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 int check_pixel_clock(struct dispc_device *dispc,
>> +                 u32 hw_videoport, unsigned long clock)
>> +{
>> +    if (clock > dispc->tidss->curr_max_pclk[hw_videoport] &&
>> +        !dispc->tidss->is_oldi_vp[hw_videoport]) {
>> +        unsigned long round_clock = 
>> clk_round_rate(dispc->vp_clk[hw_videoport], clock);
>> +
>> +        if (dispc_pclk_diff(clock, round_clock) > 5)
>> +            return -EINVAL;
>> +
>> +        dispc->tidss->curr_max_pclk[hw_videoport] = round_clock;
>> +    }
>> +
>> +    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)
>> +    if (check_pixel_clock(dispc, hw_videoport, mode->clock * 1000))
>>           return MODE_CLOCK_HIGH;
>>       if (mode->hdisplay > 4096)
>> @@ -1437,17 +1429,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..fbfe6e304ac8 100644
>> --- a/drivers/gpu/drm/tidss/tidss_dispc.h
>> +++ b/drivers/gpu/drm/tidss/tidss_dispc.h
>> @@ -78,7 +78,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;
>> diff --git a/drivers/gpu/drm/tidss/tidss_drv.h 
>> b/drivers/gpu/drm/tidss/tidss_drv.h
>> index 82beaaceadb3..e89c38a386f7 100644
>> --- a/drivers/gpu/drm/tidss/tidss_drv.h
>> +++ b/drivers/gpu/drm/tidss/tidss_drv.h
>> @@ -25,6 +25,8 @@ struct tidss_device {
>>       const struct dispc_features *feat;
>>       struct dispc_device *dispc;
>>       bool is_oldi_vp[TIDSS_MAX_PORTS];
>> +    /* stores max supported pixel clock requested during checking 
>> modes */
> 
> Stores highest pixel clock value found to be valid while checking 
> supported modes for connected display

Will change in v4.

Thanks,
Jayesh

> 
> With suggested changes,
> Reviewed-by: Devarsh Thakkar <devarsht@ti.com>
> 
> Regards
> Devarsh
> 
>> +    unsigned long curr_max_pclk[TIDSS_MAX_PORTS];
>>       unsigned int num_crtcs;