fbdev: fix the fb_find_nearest_mode() function

PATCH In review Michal Januszewski 2 participants 5 messages Oct 9, 2005 – Oct 14, 2005

Subsystems (2)

Framebuffer layer Console

lore

Share & tools

lore

Michal Januszewski

author

2005-10-09 09:29 UTC #0 lore raw

PATCH 1/1 fbdev: fix the fb_find_nearest_mode() function

Currently the fb_find_nearest_mode() function finds a mode with screen resolution closest to that described by the 'var' argument and with some arbitrary refresh rate (eg. in the following sequence of refresh rates: 70 60 53 85 75, 53 is selected).

This patch fixes the function so that it looks for the closest mode as far as both resolution and refresh rate are concerned. The function's first argument is changed to fb_videomode so that the refresh rate can be specified by the caller, as fb_var_screeninfo doesn't have any fields that could directly hold this data.

Signed-off-by Michal Januszewski <[email protected]>

---

drivers/video/console/fbcon.c +1 -1

diff -Nurp fbdev-orig/drivers/video/console/fbcon.c fbdev/drivers/video/console/fbcon.c
@@ -2758,7 +2758,7 @@ static void fbcon_new_modelist(struct fb
 			continue;
 		vc = vc_cons[i].d;
 		display_to_var(&var, &fb_display[i]);
-		mode = fb_find_nearest_mode(&var, &info->modelist);
+		mode = fb_find_nearest_mode(fb_display[i].mode, &info->modelist);
 		fb_videomode_to_var(&var, mode);
 
 		if (vc)

drivers/video/modedb.c +12 ~~-11~~

diff -Nurp fbdev-orig/drivers/video/modedb.c fbdev/drivers/video/modedb.c
@@ -663,6 +663,7 @@ void fb_var_to_videomode(struct fb_video
 {
 	u32 pixclock, hfreq, htotal, vtotal;
 
+	mode->refresh = 0;
 	mode->name = NULL;
 	mode->xres = var->xres;
 	mode->yres = var->yres;

@@ -785,39 +786,39 @@ struct fb_videomode *fb_find_best_mode(s
 }
 
 /**
- * fb_find_nearest_mode - find mode closest video mode
+ * fb_find_nearest_mode - find closest videomode
  *
- * @var: pointer to struct fb_var_screeninfo
+ * @mode: pointer to struct fb_videomode
  * @head: pointer to modelist
  *
  * Finds best matching videomode, smaller or greater in dimension.
  * If more than 1 videomode is found, will return the videomode with
- * the closest refresh rate
+ * the closest refresh rate.
  */
-struct fb_videomode *fb_find_nearest_mode(struct fb_var_screeninfo *var,
+struct fb_videomode *fb_find_nearest_mode(struct fb_videomode *mode,
 					  struct list_head *head)
 {
 	struct list_head *pos;
 	struct fb_modelist *modelist;
-	struct fb_videomode *mode, *best = NULL;
+	struct fb_videomode *cmode, *best = NULL;
 	u32 diff = -1, diff_refresh = -1;
 
 	list_for_each(pos, head) {
 		u32 d;
 
 		modelist = list_entry(pos, struct fb_modelist, list);
-		mode = &modelist->mode;
+		cmode = &modelist->mode;
 
-		d = abs(mode->xres - var->xres) +
-			abs(mode->yres - var->yres);
+		d = abs(cmode->xres - mode->xres) +
+			abs(cmode->yres - mode->yres);
 		if (diff > d) {
 			diff = d;
-			best = mode;
+			best = cmode;
 		} else if (diff == d) {
-			d = abs(mode->refresh - best->refresh);
+			d = abs(cmode->refresh - mode->refresh);
 			if (diff_refresh > d) {
 				diff_refresh = d;
-				best = mode;
+				best = cmode;
 			}
 		}
 	}

include/linux/fb.h +1 -1

diff -Nurp fbdev-orig/include/linux/fb.h fbdev/include/linux/fb.h
@@ -898,7 +898,7 @@ extern struct fb_videomode *fb_match_mod
 					  struct list_head *head);
 extern struct fb_videomode *fb_find_best_mode(struct fb_var_screeninfo *var,
 					      struct list_head *head);
-extern struct fb_videomode *fb_find_nearest_mode(struct fb_var_screeninfo *var,
+extern struct fb_videomode *fb_find_nearest_mode(struct fb_videomode *mode,
 						 struct list_head *head);
 extern void fb_destroy_modelist(struct list_head *head);
 extern void fb_videomode_to_modelist(struct fb_videomode *modedb, int num,

Antonino A. Daplas

2005-10-10 08:22 UTC #1 lore raw

Quoted 11 lines · Michal Januszewski

Currently the fb_find_nearest_mode() function finds a mode with screen resolution closest to that described by the 'var' argument and with some arbitrary refresh rate (eg. in the following sequence of refresh rates: 70 60 53 85 75, 53 is selected).
This patch fixes the function so that it looks for the closest mode as far as both resolution and refresh rate are concerned. The function's first argument is changed to fb_videomode so that the refresh rate can be specified by the caller, as fb_var_screeninfo doesn't have any fields that could directly hold this data.

I like it. I always wanted to fix this but I keep on forgetting :-)

Quoted 19 lines

Signed-off-by Michal Januszewski <[email protected]>
---
diff -Nurp fbdev-orig/drivers/video/console/fbcon.c fbdev/drivers/video/console/fbcon.c
drivers/video/console/fbcon.c +1 -1
@@ -2758,7 +2758,7 @@ static void fbcon_new_modelist(struct fb
 			continue;
 		vc = vc_cons[i].d;
 		display_to_var(&var, &fb_display[i]);
-		mode = fb_find_nearest_mode(&var, &info->modelist);
+		mode = fb_find_nearest_mode(fb_display[i].mode, &info->modelist);
 		fb_videomode_to_var(&var, mode);
 
 		if (vc)
diff -Nurp fbdev-orig/drivers/video/modedb.c fbdev/drivers/video/modedb.c

drivers/video/modedb.c void fb_var_to_videomode(struct fb_video · line 666

Quoted 4 lines

drivers/video/modedb.c +1 -0

@@ -663,6 +663,7 @@ void fb_var_to_videomode(struct fb_video
 {
 	u32 pixclock, hfreq, htotal, vtotal;
 
+	mode->refresh = 0;

Any reason why you need to set mode->refresh to 0?

Tony

-------------------------------------------------------
This SF.Net email is sponsored by:
Power Architecture Resource Center: Free content, downloads, discussions, and more. http://solutions.newsforge.com/ibmarch.tmpl

Michal Januszewski

author

2005-10-13 19:37 UTC #2 lore raw

Quoted 10 lines · Antonino A. Daplas

drivers/video/modedb.c void fb_var_to_videomode(struct fb_video · line 666
Quoted 8 lines
diff -Nurp fbdev-orig/drivers/video/modedb.c fbdev/drivers/video/modedb.c
drivers/video/modedb.c +1 -0
@@ -663,6 +663,7 @@ void fb_var_to_videomode(struct fb_video
 {
 	u32 pixclock, hfreq, htotal, vtotal;
 
+	mode->refresh = 0;
Any reason why you need to set mode->refresh to 0?

This is something I had to use in a driver I'm currently working on and I thought it might be a good idea to include it in the patch I sent.

The thing is, if the var struct has the pixclock field set to 0, mode->refresh is left untouched, which could mean that it's either set to a random value or to an invalid refresh rate coming from a previously processed videomode (in case the fb_videomode struct is reused).

I actually dealt with modes with var->pixclock == 0, and it always resulted in a getting U:<xres>x<yres>-<random> in /sys/class/graphics/fb0/modes.

In case you're wondering why would anyone ever need to have var->pixclock set to 0 -- I was using it to denote modes with a default refresh rate set by the hardware (well, the Video BIOS to be more specific). Please let me know is this is very wrong, but I just didn't see any other way of doing it.

Signature · 3 lines

Michal Januszewski                                Gentoo Linux Developer
cell: +48504917690                         http://dev.gentoo.org/~spock/
JID: [email protected]               freenode: #gentoo-dev, #gentoo-pl

Antonino A. Daplas

2005-10-14 00:18 UTC #3 lore raw

Quoted 12 lines · Michal Januszewski

Quoted 9 lines · Antonino A. Daplas
drivers/video/modedb.c void fb_var_to_videomode(struct fb_video · line 666
Quoted 8 lines
diff -Nurp fbdev-orig/drivers/video/modedb.c fbdev/drivers/video/modedb.c
drivers/video/modedb.c +1 -0
@@ -663,6 +663,7 @@ void fb_var_to_videomode(struct fb_video
 {
 	u32 pixclock, hfreq, htotal, vtotal;
 
+	mode->refresh = 0;
Any reason why you need to set mode->refresh to 0?

Quoted 8 lines

This is something I had to use in a driver I'm currently working on and I thought it might be a good idea to include it in the patch I sent.
The thing is, if the var struct has the pixclock field set to 0, mode->refresh is left untouched, which could mean that it's either set to a random value or to an invalid refresh rate coming from a previously processed videomode (in case the fb_videomode struct is reused).

Ok.

Quoted 9 lines

I actually dealt with modes with var->pixclock == 0, and it always resulted in a getting U:<xres>x<yres>-<random> in /sys/class/graphics/fb0/modes.
In case you're wondering why would anyone ever need to have var->pixclock set to 0 -- I was using it to denote modes with a default refresh rate set by the hardware (well, the Video BIOS to be more specific). Please let me know is this is very wrong, but I just didn't see any other way of doing it.

No, it's not wrong. But it is preferrable to have a nonzero value in var->pixclock (whether calculated or via a VBE function call).

Still, I agree with you to initialize mode->refresh to zero (or perhaps -1 to to denote a divide by zero) for these cases.

Tony

Michal Januszewski

author

2005-10-14 15:04 UTC #4 lore raw

Quoted 11 lines · Antonino A. Daplas

Quoted 5 lines
In case you're wondering why would anyone ever need to have var->pixclock set to 0 -- I was using it to denote modes with a default refresh rate set by the hardware (well, the Video BIOS to be more specific). Please let me know is this is very wrong, but I just didn't see any other way of doing it.
No, it's not wrong. But it is preferrable to have a nonzero value in var->pixclock (whether calculated or via a VBE function call).
Still, I agree with you to initialize mode->refresh to zero (or perhaps -1 to to denote a divide by zero) for these cases.

OK, nice. I'd say that 0 might be a little cleaner (we're avoiding <nn>x<nn>-4294967295 entries in the sysfs nodes without having to write any additional code). Also, no one should be dividing anything by mode->refresh without checking its contents first, so having a zero there doesn't seem like something dangerous. But, ultimately it's up to you to decide, of course.

Signature · 3 lines

Michal Januszewski                                Gentoo Linux Developer
cell: +48504917690                         http://dev.gentoo.org/~spock/
JID: [email protected]               freenode: #gentoo-dev, #gentoo-pl