[PATCH v2 14/16] drm/ast: astdp: Look up mode index from table
Jocelyn Falempe
jfalempe at redhat.com
Wed Jan 29 14:05:30 UTC 2025
On 29/01/2025 13:01, Thomas Zimmermann wrote:
> Hi
>
>
> Am 29.01.25 um 12:27 schrieb Jocelyn Falempe:
>> On 29/01/2025 10:55, Thomas Zimmermann wrote:
>>> Replace the large switch statement with a look-up table when selecting
>>> the mode index. Makes the code easier to read. The table is sorted by
>>> resolutions; if run-time overhead from traversal becomes significant,
>>> binary search would be a possible optimization.
>>>
>>> The mode index requires a refresh-rate index to be added or subtracted,
>>> which still requires a minimal switch.
>>>
>> I think there is a problem in the mode_index/refresh_index
>> calculation, see below:
>>
>>
>>> Signed-off-by: Thomas Zimmermann <tzimmermann at suse.de>
>>> Suggested-by: Jocelyn Falempe <jfalempe at redhat.com>
>>> ---
>>> drivers/gpu/drm/ast/ast_dp.c | 116 +++++++++++++++++------------------
>>> 1 file changed, 55 insertions(+), 61 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/ast/ast_dp.c b/drivers/gpu/drm/ast/ast_dp.c
>>> index e1ca012e639be..70fa754432bca 100644
>>> --- a/drivers/gpu/drm/ast/ast_dp.c
>>> +++ b/drivers/gpu/drm/ast/ast_dp.c
>>> @@ -14,80 +14,74 @@
>>> #include "ast_drv.h"
>>> #include "ast_vbios.h"
>>> +struct ast_astdp_mode_index_table_entry {
>>> + unsigned int hdisplay;
>>> + unsigned int vdisplay;
>>> + unsigned int mode_index;
>>> +};
>>> +
>>> +/* FIXME: Do refresh rate and flags actually matter? */
>>> +static const struct ast_astdp_mode_index_table_entry
>>> ast_astdp_mode_index_table[] = {
>>> + { 320, 240, ASTDP_320x240_60 },
>>> + { 400, 300, ASTDP_400x300_60 },
>>> + { 512, 384, ASTDP_512x384_60 },
>>> + { 640, 480, ASTDP_640x480_60 },
>>> + { 800, 600, ASTDP_800x600_56 },
>>> + { 1024, 768, ASTDP_1024x768_60 },
>>> + { 1152, 864, ASTDP_1152x864_75 },
>>> + { 1280, 800, ASTDP_1280x800_60_RB },
>>> + { 1280, 1024, ASTDP_1280x1024_60 },
>>> + { 1360, 768, ASTDP_1366x768_60 }, // same as 1366x786
>>> + { 1366, 768, ASTDP_1366x768_60 },
>>> + { 1440, 900, ASTDP_1440x900_60_RB },
>>> + { 1600, 900, ASTDP_1600x900_60_RB },
>>> + { 1600, 1200, ASTDP_1600x1200_60 },
>>> + { 1680, 1050, ASTDP_1680x1050_60_RB },
>>> + { 1920, 1080, ASTDP_1920x1080_60 },
>>> + { 1920, 1200, ASTDP_1920x1200_60 },
>>> + { 0 }
>>> +};
>>> +
>>> +static int __ast_astdp_get_mode_index(unsigned int hdisplay,
>>> unsigned int vdisplay)
>>> +{
>>> + const struct ast_astdp_mode_index_table_entry *entry =
>>> ast_astdp_mode_index_table;
>>> +
>>> + while (entry->hdisplay && entry->vdisplay) {
>>> + if (entry->hdisplay == hdisplay && entry->vdisplay == vdisplay)
>>> + return entry->mode_index;
>>> + ++entry;
>>> + }
>>> +
>>> + return -EINVAL;
>>> +}
>>> +
>>> static int ast_astdp_get_mode_index(const struct ast_vbios_enhtable
>>> *vmode)
>>> {
>>> + int mode_index;
>>> u8 refresh_rate_index;
>>> + mode_index = __ast_astdp_get_mode_index(vmode->hde, vmode->vde);
>>> + if (mode_index < 0)
>>> + return mode_index;
>>> +
>>> if (vmode->refresh_rate_index < 1 || vmode->refresh_rate_index
>>> > 255)
>>> return -EINVAL;
>>> -
>>> refresh_rate_index = vmode->refresh_rate_index - 1;
>>> - switch (vmode->hde) {
>>> - case 320:
>>> - if (vmode->vde == 240)
>>> - return ASTDP_320x240_60;
>>> - break;
>>> - case 400:
>>> - if (vmode->vde == 300)
>>> - return ASTDP_400x300_60;
>>> - break;
>>> - case 512:
>>> - if (vmode->vde == 384)
>>> - return ASTDP_512x384_60;
>>> - break;
>>> - case 640:
>>> - if (vmode->vde == 480)
>>> - return (u8)(ASTDP_640x480_60 + (u8)refresh_rate_index);
>>> - break;
>>> - case 800:
>>> - if (vmode->vde == 600)
>>> - return (u8)(ASTDP_800x600_56 + (u8)refresh_rate_index);
>>> - break;
>>> - case 1024:
>>> - if (vmode->vde == 768)
>>> - return (u8)(ASTDP_1024x768_60 + (u8)refresh_rate_index);
>>> - break;
>>> - case 1152:
>>> - if (vmode->vde == 864)
>>> - return ASTDP_1152x864_75;
>>> - break;
>>> - case 1280:
>>> - if (vmode->vde == 800)
>>> - return (u8)(ASTDP_1280x800_60_RB - (u8)refresh_rate_index);
>>> - if (vmode->vde == 1024)
>>> - return (u8)(ASTDP_1280x1024_60 + (u8)refresh_rate_index);
>>> - break;
>>> - case 1360:
>>> - case 1366:
>>> - if (vmode->vde == 768)
>>> - return ASTDP_1366x768_60;
>>> - break;
>>> - case 1440:
>>> - if (vmode->vde == 900)
>>> - return (u8)(ASTDP_1440x900_60_RB - (u8)refresh_rate_index);
>>> - break;
>>> - case 1600:
>>> - if (vmode->vde == 900)
>>> - return (u8)(ASTDP_1600x900_60_RB - (u8)refresh_rate_index);
>>> - if (vmode->vde == 1200)
>>
>>> - break;
>>> - case 1680:
>>> - if (vmode->vde == 1050)
>>> - return (u8)(ASTDP_1680x1050_60_RB -
>>> (u8)refresh_rate_index);
>>> - break;
>>> - case 1920:
>>> - if (vmode->vde == 1080)
>>> - return ASTDP_1920x1080_60;
>>> - if (vmode->vde == 1200)
>>> - return ASTDP_1920x1200_60;
>>> + /* FIXME: Why are we doing this? */
>>> + switch (mode_index) {
>>> + case ASTDP_1280x800_60_RB:
>>> + case ASTDP_1440x900_60_RB:
>>> + case ASTDP_1600x900_60_RB:
>>> + case ASTDP_1680x1050_60_RB:
>>> + mode_index = (u8)(mode_index - (u8)refresh_rate_index);
>>> break;
>> I think you should add this to do the same as before:
>
> It's intentional. The refresh-rate index stored in vmode-
> >refresh_rate_index is at least one. The function then subtracts 1 to
> compute refresh_rate_index, so we have 0 by default. And that's what we
> always used for cases that did not explicitly add refresh_rate_index
> before. I guess I should add this to the commit message's second paragraph.
>
> Apart from that, I honestly don't understand the purpose of this
> computation.
Yes, I have no clue either. Thanks for the clarification.> Best regards
> Thomas
>
>>
>> case ASTDP_640x480_60:
>> case ASTDP_800x600_56:
>> case ASTDP_1024x768_60:
>> case ASTDP_1280x1024_60:
>> mode_index = (u8)(mode_index + (u8)refresh_rate_index);
>> break;
>> default:
>> break;
>>
>>> default:
>>> + mode_index = (u8)(mode_index + (u8)refresh_rate_index);
>>> break;
>>> }
>>> - return -EINVAL;
>>> + return mode_index;
>>> }
>>> static bool ast_astdp_is_connected(struct ast_device *ast)
>>
>
More information about the dri-devel
mailing list