Re: [PATCH v2 14/16] drm/ast: astdp: Look up mode index from table

[Jocelyn Falempe <jfalempe@xxxxxxxxxx>]

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@xxxxxxxxxx>
> > > > Signed-off-by: Thomas Zimmermann <tzimmermann@xxxxxxx>
> > > > Suggested-by: Jocelyn Falempe <jfalempe@xxxxxxxxxx>
> > > > ---
> > > >   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)