[PATCH v2 14/16] drm/ast: astdp: Look up mode index from table
Thomas Zimmermann
tzimmermann at suse.de
Wed Jan 29 12:01:27 UTC 2025
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.
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)
>
--
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstrasse 146, 90461 Nuernberg, Germany
GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman
HRB 36809 (AG Nuernberg)
More information about the dri-devel
mailing list