[PATCH v2 14/16] drm/ast: astdp: Look up mode index from table
Jocelyn Falempe
jfalempe at redhat.com
Thu Jan 30 11:33:34 UTC 2025
On 29/01/2025 15:05, Jocelyn Falempe wrote:
> 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:
Sorry, I though I already reviewed it. With the added explanations, it
looks good to me.
Reviewed-by: Jocelyn Falempe <jfalempe at redhat.com>
>>>
>>>
>>>> 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