| Message ID | 20250129095840.20629-15-tzimmermann@suse.de (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | drm/ast: Clean up display-mode detection and validation | expand |
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@suse.de> > Suggested-by: Jocelyn Falempe <jfalempe@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: 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)
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@suse.de> >> Suggested-by: Jocelyn Falempe <jfalempe@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) >
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@suse.de> >>> Suggested-by: Jocelyn Falempe <jfalempe@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) >> >
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@redhat.com> >>> >>> >>>> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de> >>>> Suggested-by: Jocelyn Falempe <jfalempe@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) >>> >> >
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) - return ASTDP_1600x1200_60; - 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; 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)
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. Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de> Suggested-by: Jocelyn Falempe <jfalempe@redhat.com> --- drivers/gpu/drm/ast/ast_dp.c | 116 +++++++++++++++++------------------ 1 file changed, 55 insertions(+), 61 deletions(-)