[PATCH] Do set var even if no fb_check_var() provided.

[PATCH] Do set var even if no fb_check_var() provided.

[Takashi Yoshii]

Hi,

The behavior of FBIOSET_VSCREENINFO seems to be weired on FB which does not
 provides fbops->fb_check_var.
It doesn't set anything, but gets info->var instead, and returns no error.
It is just same as FBIOGET_VSCREENINFO does.

IMHO, this should be one of the following candidates,
 1. set var without check.
 2. do nothing but return error (setting var is not supported).
or
 3. it's a bug (fb_check_var should always be provided).

The patch at the bottom implements "1".

Because I don't know API specification, nor the history of the code, 
I would like people who knows well to discuss this.

Regards,
/yoshii
---
drivers/video/fbmem.c:fb_set_var()
 Do set_var even if fbops->fb_check_var == NULL.
 Possible API change of ioctl(FBIOSET_VSCREENINFO).

Signed-off-by: Takashi YOSHII <yoshii.takashi@renesas.com>

diff --git a/drivers/video/fbmem.c b/drivers/video/fbmem.c
index 776f7fc..6fc2ba6 100644
--- a/drivers/video/fbmem.c
+++ b/drivers/video/fbmem.c
@@ -939,16 +939,12 @@ fb_set_var(struct fb_info *info, struct fb_var_screeninfo *var)
 	    memcmp(&info->var, var, sizeof(struct fb_var_screeninfo))) {
 		u32 activate = var->activate;
 
-		if (!info->fbops->fb_check_var) {
-			*var = info->var;
-			goto done;
+		if (info->fbops->fb_check_var) {
+			ret = info->fbops->fb_check_var(var, info);
+			if (ret)
+				goto done;
 		}
 
-		ret = info->fbops->fb_check_var(var, info);
-
-		if (ret)
-			goto done;
-
 		if ((var->activate & FB_ACTIVATE_MASK) == FB_ACTIVATE_NOW) {
 			struct fb_videomode mode;
 
-- 
1.5.4.5


-------------------------------------------------------------------------
This SF.Net email is sponsored by the Moblin Your Move Developer's challenge
Build the coolest Linux based applications with Moblin SDK & win great prizes
Grand prize is a trip for two to an Open Source event anywhere in the world
http://moblin-contest.org/redirect.php?banner_id=100&url=/

Re: [PATCH] Do set var even if no fb_check_var() provided.

[Geert Uytterhoeven]

On Wed, 27 Aug 2008, Takashi Yoshii wrote:
> The behavior of FBIOSET_VSCREENINFO seems to be weired on FB which does not
>  provides fbops->fb_check_var.
> It doesn't set anything, but gets info->var instead, and returns no error.
> It is just same as FBIOGET_VSCREENINFO does.
> 
> IMHO, this should be one of the following candidates,
>  1. set var without check.
>  2. do nothing but return error (setting var is not supported).
> or
>  3. it's a bug (fb_check_var should always be provided).
> 
> The patch at the bottom implements "1".
> 
> Because I don't know API specification, nor the history of the code, 
> I would like people who knows well to discuss this.

If the driver doesn't provide a fb_check_var(), it means it cannot
change video mode. Hence this rules out #1.

#2 is not acceptable, as it will break existing applications. It's also
incorrect, as FBIOPUT_VSCREENINFO should succeed for supported modes.
For unsupported modes, the mode should be rounded up to a supported mode, if
possible. In the case of drivers that support one fixed mode only, this
rounding up is relaxed to `rounding' to the sole supported mode.

#3 is also wrong, as fb_check_var() has been deliberately made optional to
simplify drivers that support one fixed mode only.

Conclusion: nothing should be changed?

Gr{oetje,eeting}s,

						Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
							    -- Linus Torvalds

-------------------------------------------------------------------------
This SF.Net email is sponsored by the Moblin Your Move Developer's challenge
Build the coolest Linux based applications with Moblin SDK & win great prizes
Grand prize is a trip for two to an Open Source event anywhere in the world
http://moblin-contest.org/redirect.php?banner_id=100&url=/

Re: [PATCH] Do set var even if no fb_check_var() provided.

[Helge Deller]

Geert Uytterhoeven wrote:
> On Wed, 27 Aug 2008, Takashi Yoshii wrote:
>> The behavior of FBIOSET_VSCREENINFO seems to be weired on FB which does
>> not
>>  provides fbops->fb_check_var.
>> It doesn't set anything, but gets info->var instead, and returns no
>> error. It is just same as FBIOGET_VSCREENINFO does.
>> 
>> IMHO, this should be one of the following candidates,
>>  1. set var without check.
>>  2. do nothing but return error (setting var is not supported).
>> or
>>  3. it's a bug (fb_check_var should always be provided).
>> 
>> The patch at the bottom implements "1".
>> 
>> Because I don't know API specification, nor the history of the code,
>> I would like people who knows well to discuss this.
> 
> If the driver doesn't provide a fb_check_var(), it means it cannot
> change video mode. Hence this rules out #1.
> 
> #2 is not acceptable, as it will break existing applications. It's also
> incorrect, as FBIOPUT_VSCREENINFO should succeed for supported modes.
> For unsupported modes, the mode should be rounded up to a supported mode,
> if possible. In the case of drivers that support one fixed mode only, this
> rounding up is relaxed to `rounding' to the sole supported mode.
> 
> #3 is also wrong, as fb_check_var() has been deliberately made optional to
> simplify drivers that support one fixed mode only.
> 
> Conclusion: nothing should be changed?

I'm not sure.
On parisc we just stumbled over exactly this problem where I think Takashi's
proposal "1." is probably the right solution.

Please see my Xorg bugzilla entry:
https://bugs.freedesktop.org/show_bug.cgi?id=17153 
and this problem description/thread ("X won't start with VisEG and
2.6.22.19"):
http://thread.gmane.org/gmane.linux.ports.parisc/564/focus=592

Helge


-------------------------------------------------------------------------
This SF.Net email is sponsored by the Moblin Your Move Developer's challenge
Build the coolest Linux based applications with Moblin SDK & win great prizes
Grand prize is a trip for two to an Open Source event anywhere in the world
http://moblin-contest.org/redirect.php?banner_id=100&url=/

Re: [PATCH] Do set var even if no fb_check_var() provided.

[Michel Dänzer]

On Wed, 2008-08-27 at 23:49 +0200, Helge Deller wrote:
> Geert Uytterhoeven wrote:
> > On Wed, 27 Aug 2008, Takashi Yoshii wrote:
> >> The behavior of FBIOSET_VSCREENINFO seems to be weired on FB which does
> >> not
> >>  provides fbops->fb_check_var.
> >> It doesn't set anything, but gets info->var instead, and returns no
> >> error. It is just same as FBIOGET_VSCREENINFO does.
> >> 
> >> IMHO, this should be one of the following candidates,
> >>  1. set var without check.
> >>  2. do nothing but return error (setting var is not supported).
> >> or
> >>  3. it's a bug (fb_check_var should always be provided).
> >> 
> >> The patch at the bottom implements "1".
> >> 
> >> Because I don't know API specification, nor the history of the code,
> >> I would like people who knows well to discuss this.
> > 
> > If the driver doesn't provide a fb_check_var(), it means it cannot
> > change video mode. Hence this rules out #1.
> > 
> > #2 is not acceptable, as it will break existing applications. It's also
> > incorrect, as FBIOPUT_VSCREENINFO should succeed for supported modes.
> > For unsupported modes, the mode should be rounded up to a supported mode,
> > if possible. In the case of drivers that support one fixed mode only, this
> > rounding up is relaxed to `rounding' to the sole supported mode.
> > 
> > #3 is also wrong, as fb_check_var() has been deliberately made optional to
> > simplify drivers that support one fixed mode only.
> > 
> > Conclusion: nothing should be changed?
> 
> I'm not sure.
> On parisc we just stumbled over exactly this problem where I think Takashi's
> proposal "1." is probably the right solution.
> 
> Please see my Xorg bugzilla entry:
> https://bugs.freedesktop.org/show_bug.cgi?id=17153 

I believe (bugs.freedesktop.org is currently down) that's currently
waiting for information from you. Namely, have you tried not specifying
a Modes line in the xorg.conf SubSection "Display"? I think the Xorg
fbdev driver should come up with the currently active mode in that case.
If that doesn't work, please attach the corresponding Xorg.0.log to the
bug report once bugzilla is back online.


-- 
Earthling Michel Dänzer           |          http://tungstengraphics.com
Libre software enthusiast         |          Debian, X and DRI developer


-------------------------------------------------------------------------
This SF.Net email is sponsored by the Moblin Your Move Developer's challenge
Build the coolest Linux based applications with Moblin SDK & win great prizes
Grand prize is a trip for two to an Open Source event anywhere in the world
http://moblin-contest.org/redirect.php?banner_id=100&url=/
_______________________________________________
Linux-fbdev-devel mailing list
Linux-fbdev-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-fbdev-devel

Re: [PATCH] Do set var even if no fb_check_var() provided.

[Helge Deller]

Michel Dänzer wrote:
> On Wed, 2008-08-27 at 23:49 +0200, Helge Deller wrote:
>> Geert Uytterhoeven wrote:
>>> On Wed, 27 Aug 2008, Takashi Yoshii wrote:
>>>> The behavior of FBIOSET_VSCREENINFO seems to be weired on FB which does
>>>> not
>>>>  provides fbops->fb_check_var.
>>>> It doesn't set anything, but gets info->var instead, and returns no
>>>> error. It is just same as FBIOGET_VSCREENINFO does.
>>>>
>>>> IMHO, this should be one of the following candidates,
>>>>  1. set var without check.
>>>>  2. do nothing but return error (setting var is not supported).
>>>> or
>>>>  3. it's a bug (fb_check_var should always be provided).
>>>>
>>>> The patch at the bottom implements "1".
>>>>
>>>> Because I don't know API specification, nor the history of the code,
>>>> I would like people who knows well to discuss this.
>>> If the driver doesn't provide a fb_check_var(), it means it cannot
>>> change video mode. Hence this rules out #1.
>>>
>>> #2 is not acceptable, as it will break existing applications. It's also
>>> incorrect, as FBIOPUT_VSCREENINFO should succeed for supported modes.
>>> For unsupported modes, the mode should be rounded up to a supported mode,
>>> if possible. In the case of drivers that support one fixed mode only, this
>>> rounding up is relaxed to `rounding' to the sole supported mode.
>>>
>>> #3 is also wrong, as fb_check_var() has been deliberately made optional to
>>> simplify drivers that support one fixed mode only.
>>>
>>> Conclusion: nothing should be changed?
>> I'm not sure.
>> On parisc we just stumbled over exactly this problem where I think Takashi's
>> proposal "1." is probably the right solution.
>>
>> Please see my Xorg bugzilla entry:
>> https://bugs.freedesktop.org/show_bug.cgi?id=17153 
> 
> I believe (bugs.freedesktop.org is currently down) that's currently
> waiting for information from you. Namely, have you tried not specifying
> a Modes line in the xorg.conf SubSection "Display"? I think the Xorg
> fbdev driver should come up with the currently active mode in that case.
> If that doesn't work, please attach the corresponding Xorg.0.log to the
> bug report once bugzilla is back online.

Bugzilla (https://bugs.freedesktop.org/show_bug.cgi?id=17153 ) is 
available again, and I've uploaded the xorg.conf file, as well as the 
Xorg.0.log log file as requested.

Your idea about testing a few mode lines is normally a good idea.
On a PC I would do exactly this.

But on a pure framebuffer driver it's only necessary to know (and to 
check), that it runs (for example) at 1024x768 pixels with 8bpp.  The 
monitor sync values shouldn't really matter and that is the problem in 
this thread. X should not try to force a specific monitor frequency. It 
won't suceed anyway. It's for some cards just not possible to switch 
monitor frequencies or to know the current ones (and this is why those 
drivers don't implement fb_check_var())...

Helge

-------------------------------------------------------------------------
This SF.Net email is sponsored by the Moblin Your Move Developer's challenge
Build the coolest Linux based applications with Moblin SDK & win great prizes
Grand prize is a trip for two to an Open Source event anywhere in the world
http://moblin-contest.org/redirect.php?banner_id=100&url=/
_______________________________________________
Linux-fbdev-devel mailing list
Linux-fbdev-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-fbdev-devel

Re: [PATCH] Do set var even if no fb_check_var() provided.

[Michel Dänzer]

On Thu, 2008-08-28 at 23:43 +0200, Helge Deller wrote:
> 
> Your idea about testing a few mode lines is normally a good idea.

My idea is quite the opposite - to try *without* even the Modes line in
SubSection "Display". Can you try that please?


-- 
Earthling Michel Dänzer           |          http://tungstengraphics.com
Libre software enthusiast         |          Debian, X and DRI developer


-------------------------------------------------------------------------
This SF.Net email is sponsored by the Moblin Your Move Developer's challenge
Build the coolest Linux based applications with Moblin SDK & win great prizes
Grand prize is a trip for two to an Open Source event anywhere in the world
http://moblin-contest.org/redirect.php?banner_id=100&url=/
_______________________________________________
Linux-fbdev-devel mailing list
Linux-fbdev-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-fbdev-devel

Re: [PATCH] Do set var even if no fb_check_var() provided.

[Helge Deller]

Michel Dänzer wrote:
> On Thu, 2008-08-28 at 23:43 +0200, Helge Deller wrote:
>> Your idea about testing a few mode lines is normally a good idea.
> 
> My idea is quite the opposite - to try *without* even the Modes line in
> SubSection "Display". Can you try that please?


Doesn't work either.

a) If I drop the "monitor" line in the Screen section it chooses a 
"default monitor" and fails:
(==) Using config file: "/etc/X11/xorg.conf"
(==) ServerLayout "Default Layout"
(**) |-->Screen "STI" (0)
(**) |   |-->Monitor "<default monitor>"
(**) |   |-->Device "Generic Video Card"
(==) No monitor specified for screen "STI".
         Using a default monitor configuration.
-> same failure.


b) If I just drop the HorizSync and VertRefresh entries, it fails as well:
(II) Running in FRAMEBUFFER Mode
(**) FBDEV(0): Depth 8, (--) framebuffer bpp 8
(==) FBDEV(0): Default visual is PseudoColor
(==) FBDEV(0): Using gamma correction (1.0, 1.0, 1.0)
(II) FBDEV(0): hardware: stifb (video memory: 1536kB)
(**) FBDEV(0): Option "fbdev" "/dev/fb0"
(II) FBDEV(0): checking modes against framebuffer device...
(II) FBDEV(0):  mode "1024x768" test failed
(II) FBDEV(0): checking modes against monitor...
(--) FBDEV(0): Virtual size is 1024x768 (pitch 1024)
(**) FBDEV(0):  Built-in mode "current": 28000.0 MHz, 27343.8 kHz, 
35603.8 Hz
(II) FBDEV(0): Modeline "current"x0.0  28000.00  1024 1024 1024 1024 
768 768 768 768 -hsync -vsync -csync (27343.8 kHz)
(==) FBDEV(0): DPI set to (96, 96)

Helge

-------------------------------------------------------------------------
This SF.Net email is sponsored by the Moblin Your Move Developer's challenge
Build the coolest Linux based applications with Moblin SDK & win great prizes
Grand prize is a trip for two to an Open Source event anywhere in the world
http://moblin-contest.org/redirect.php?banner_id=100&url=/
_______________________________________________
Linux-fbdev-devel mailing list
Linux-fbdev-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-fbdev-devel

Re: [PATCH] Do set var even if no fb_check_var() provided.

[Michel Dänzer]

On Fri, 2008-08-29 at 00:16 +0200, Helge Deller wrote:
> Michel Dänzer wrote:
> > On Thu, 2008-08-28 at 23:43 +0200, Helge Deller wrote:
> >> Your idea about testing a few mode lines is normally a good idea.
> > 
> > My idea is quite the opposite - to try *without* even the Modes line in
> > SubSection "Display". Can you try that please?
> 
> 
> Doesn't work either.
> 
> a) If I drop the "monitor" line in the Screen section it chooses a 

[...]

> b) If I just drop the HorizSync and VertRefresh entries, it fails as well:

[...]

Which part of 'the Modes line in SubSection "Display"' is so hard to
understand? *shrug*


-- 
Earthling Michel Dänzer           |          http://tungstengraphics.com
Libre software enthusiast         |          Debian, X and DRI developer


-------------------------------------------------------------------------
This SF.Net email is sponsored by the Moblin Your Move Developer's challenge
Build the coolest Linux based applications with Moblin SDK & win great prizes
Grand prize is a trip for two to an Open Source event anywhere in the world
http://moblin-contest.org/redirect.php?banner_id=100&url=/
_______________________________________________
Linux-fbdev-devel mailing list
Linux-fbdev-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-fbdev-devel

Re: [PATCH] Do set var even if no fb_check_var() provided.

[Helge Deller]

Michel Dänzer wrote:
> On Fri, 2008-08-29 at 00:16 +0200, Helge Deller wrote:
>> Michel Dänzer wrote:
>>> On Thu, 2008-08-28 at 23:43 +0200, Helge Deller wrote:
>>>> Your idea about testing a few mode lines is normally a good idea.
>>> My idea is quite the opposite - to try *without* even the Modes line in
>>> SubSection "Display". Can you try that please?
>>
>> Doesn't work either.
>>
>> a) If I drop the "monitor" line in the Screen section it chooses a 
> 
> [...]
> 
>> b) If I just drop the HorizSync and VertRefresh entries, it fails as well:
> 
> [...]
> 
> Which part of 'the Modes line in SubSection "Display"' is so hard to
> understand? *shrug*

Do you want me to change this (1024x768): ?
	SubSection "Display"
                 Depth           8
                 Modes           "1024x768"
         EndSubSection

Why?
There is no reason.
The hardware can only run at 1024x768 at 8bpp.
If I change this, it will clearly fail.
No need to try as the answer is clear.

Reason:
root@c3000:/usr/lib/xorg/modules/linux# fbset -i

mmode "1024x768"
     geometry 1024 768 1024 768 8
     timings 0 0 0 0 0 0 0
     rgba 8/0,8/0,8/0,0/0
endmode

Frame buffer device information:
     Name        : stifb
     Address     : 0xf9000000
     Size        : 1572864
     Type        : PACKED PIXELS
     Visual      : PSEUDOCOLOR
     XPanStep    : 0
     YPanStep    : 0
     YWrapStep   : 0
     LineLength  : 2048
     MMIO Address: 0xf8100000
     MMIO Size   : 4194304
     Accelerator : No

Helge

-------------------------------------------------------------------------
This SF.Net email is sponsored by the Moblin Your Move Developer's challenge
Build the coolest Linux based applications with Moblin SDK & win great prizes
Grand prize is a trip for two to an Open Source event anywhere in the world
http://moblin-contest.org/redirect.php?banner_id=100&url=/
_______________________________________________
Linux-fbdev-devel mailing list
Linux-fbdev-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-fbdev-devel

Re: [PATCH] Do set var even if no fb_check_var() provided.

[Michel Dänzer]

On Fri, 2008-08-29 at 00:23 +0200, Helge Deller wrote:
> Michel Dänzer wrote:
> > On Fri, 2008-08-29 at 00:16 +0200, Helge Deller wrote:
> >> Michel Dänzer wrote:
> >>> On Thu, 2008-08-28 at 23:43 +0200, Helge Deller wrote:
> >>>> Your idea about testing a few mode lines is normally a good idea.
> >>> My idea is quite the opposite - to try *without* even the Modes line in
> >>> SubSection "Display". Can you try that please?
> >>
> >> Doesn't work either.
> >>
> >> a) If I drop the "monitor" line in the Screen section it chooses a 
> > 
> > [...]
> > 
> >> b) If I just drop the HorizSync and VertRefresh entries, it fails as well:
> > 
> > [...]
> > 
> > Which part of 'the Modes line in SubSection "Display"' is so hard to
> > understand? *shrug*
> 
> Do you want me to change this (1024x768): ?
> 	SubSection "Display"
>                  Depth           8
>                  Modes           "1024x768"
>          EndSubSection

Not change it, remove it (hence 'without').

I suspect the problem is that you're misunderstanding the meaning of
this line. It tells the X server to try all the 1024x768 modes from its
internal mode database, none of which match the mode of your framebuffer
device unsurprisingly. If you omit the line, the fbdev driver will get
the current mode from the framebuffer device and try that. Note that the
pixclock 0 can still cause trouble with X servers before the upcoming
1.5 release.


-- 
Earthling Michel Dänzer           |          http://tungstengraphics.com
Libre software enthusiast         |          Debian, X and DRI developer


-------------------------------------------------------------------------
This SF.Net email is sponsored by the Moblin Your Move Developer's challenge
Build the coolest Linux based applications with Moblin SDK & win great prizes
Grand prize is a trip for two to an Open Source event anywhere in the world
http://moblin-contest.org/redirect.php?banner_id=100&url=/
_______________________________________________
Linux-fbdev-devel mailing list
Linux-fbdev-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-fbdev-devel

Re: [PATCH] Do set var even if no fb_check_var() provided.

[Helge Deller]

Michel Dänzer wrote:
> On Fri, 2008-08-29 at 00:23 +0200, Helge Deller wrote:
>> Michel Dänzer wrote:
>>> On Fri, 2008-08-29 at 00:16 +0200, Helge Deller wrote:
>>>> Michel Dänzer wrote:
>>>>> On Thu, 2008-08-28 at 23:43 +0200, Helge Deller wrote:
>>>>>> Your idea about testing a few mode lines is normally a good idea.
>>>>> My idea is quite the opposite - to try *without* even the Modes line in
>>>>> SubSection "Display". Can you try that please?
>>>> Doesn't work either.
>>>>
>>>> a) If I drop the "monitor" line in the Screen section it chooses a 
>>> [...]
>>>
>>>> b) If I just drop the HorizSync and VertRefresh entries, it fails as well:
>>> [...]
>>>
>>> Which part of 'the Modes line in SubSection "Display"' is so hard to
>>> understand? *shrug*
>> Do you want me to change this (1024x768): ?
>> 	SubSection "Display"
>>                  Depth           8
>>                  Modes           "1024x768"
>>          EndSubSection
> 
> Not change it, remove it (hence 'without').
> 
> I suspect the problem is that you're misunderstanding the meaning of
> this line. It tells the X server to try all the 1024x768 modes from its
> internal mode database, none of which match the mode of your framebuffer
> device unsurprisingly. If you omit the line, the fbdev driver will get
> the current mode from the framebuffer device and try that. Note that the
> pixclock 0 can still cause trouble with X servers before the upcoming
> 1.5 release.

No, doesn't work either (see below).
Seems to be the "pixclock 0" problem you mention.
What changed in 1.5? Any link to a patch?


Helge

X.Org X Server 1.4.2
Release Date: 11 June 2008
X Protocol Version 11, Revision 0
Build Operating System: Linux Debian (xorg-server 2:1.4.2-4)
Current Operating System: Linux c3000 2.6.27-rc4 #7 SMP Thu Aug 28 
23:15:29 CEST 2008 parisc
Build Date: 15 August 2008  06:24:13PM
...
(II) FBDEV(0): using /dev/fb0
(II) Running in FRAMEBUFFER Mode
(II) FBDEV(0): Creating default Display subsection in Screen section
         "STI" for depth/fbbpp 8/8
(==) FBDEV(0): Depth 8, (==) framebuffer bpp 8
(==) FBDEV(0): Default visual is PseudoColor
(==) FBDEV(0): Using gamma correction (1.0, 1.0, 1.0)
(II) FBDEV(0): hardware: stifb (video memory: 1536kB)
(**) FBDEV(0): Option "fbdev" "/dev/fb0"
(II) FBDEV(0): checking modes against framebuffer device...
(II) FBDEV(0): checking modes against monitor...
(--) FBDEV(0): Virtual size is 1024x768 (pitch 1024)
(**) FBDEV(0):  Built-in mode "current": 28000.0 MHz, 27343.8 kHz, 
35603.8 Hz
(II) FBDEV(0): Modeline "current"x0.0  28000.00  1024 1024 1024 1024 
768 768 768 768 -hsync -vsync -csync (27343.8 kHz)
(==) FBDEV(0): DPI set to (96, 96)
...
(EE) FBDEV(0): FBIOPUT_VSCREENINFO succeeded but modified mode
(EE) FBDEV(0): mode initialization failed

Fatal server error:
AddScreen/ScreenInit failed for driver 0





-------------------------------------------------------------------------
This SF.Net email is sponsored by the Moblin Your Move Developer's challenge
Build the coolest Linux based applications with Moblin SDK & win great prizes
Grand prize is a trip for two to an Open Source event anywhere in the world
http://moblin-contest.org/redirect.php?banner_id=100&url=/
_______________________________________________
Linux-fbdev-devel mailing list
Linux-fbdev-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-fbdev-devel

Re: [PATCH] Do set var even if no fb_check_var() provided.

[Michel Dänzer]

On Fri, 2008-08-29 at 00:35 +0200, Helge Deller wrote:
> Michel D채nzer wrote:
> > On Fri, 2008-08-29 at 00:23 +0200, Helge Deller wrote:
> >> Michel D채nzer wrote:
> >>> On Fri, 2008-08-29 at 00:16 +0200, Helge Deller wrote:
> >>>> Michel D채nzer wrote:
> >>>>> On Thu, 2008-08-28 at 23:43 +0200, Helge Deller wrote:
> >>>>>> Your idea about testing a few mode lines is normally a good idea.
> >>>>> My idea is quite the opposite - to try *without* even the Modes line in
> >>>>> SubSection "Display". Can you try that please?
> >>>> Doesn't work either.
> >>>>
> >>>> a) If I drop the "monitor" line in the Screen section it chooses a 
> >>> [...]
> >>>
> >>>> b) If I just drop the HorizSync and VertRefresh entries, it fails as well:
> >>> [...]
> >>>
> >>> Which part of 'the Modes line in SubSection "Display"' is so hard to
> >>> understand? *shrug*
> >> Do you want me to change this (1024x768): ?
> >> 	SubSection "Display"
> >>                  Depth           8
> >>                  Modes           "1024x768"
> >>          EndSubSection
> > 
> > Not change it, remove it (hence 'without').
> > 
> > I suspect the problem is that you're misunderstanding the meaning of
> > this line. It tells the X server to try all the 1024x768 modes from its
> > internal mode database, none of which match the mode of your framebuffer
> > device unsurprisingly. If you omit the line, the fbdev driver will get
> > the current mode from the framebuffer device and try that. Note that the
> > pixclock 0 can still cause trouble with X servers before the upcoming
> > 1.5 release.
> 
> No, doesn't work either (see below).
> Seems to be the "pixclock 0" problem you mention.
> What changed in 1.5? Any link to a patch?

http://cgit.freedesktop.org/xorg/xserver/commit/?id=c095da04fe7c73b6503ef5b93549b13796c51b22


-- 
Earthling Michel D채nzer           |          http://tungstengraphics.com
Libre software enthusiast         |          Debian, X and DRI developer


-------------------------------------------------------------------------
This SF.Net email is sponsored by the Moblin Your Move Developer's challenge
Build the coolest Linux based applications with Moblin SDK & win great prizes
Grand prize is a trip for two to an Open Source event anywhere in the world
http://moblin-contest.org/redirect.php?banner_id=100&url=/
_______________________________________________
Linux-fbdev-devel mailing list
Linux-fbdev-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-fbdev-devel

Re: [PATCH] Do set var even if no fb_check_var() provided.

[Ville Syrjälä]

On Wed, Aug 27, 2008 at 11:49:15PM +0200, Helge Deller wrote:
> Geert Uytterhoeven wrote:
> > On Wed, 27 Aug 2008, Takashi Yoshii wrote:
> >> The behavior of FBIOSET_VSCREENINFO seems to be weired on FB which does
> >> not
> >>  provides fbops->fb_check_var.
> >> It doesn't set anything, but gets info->var instead, and returns no
> >> error. It is just same as FBIOGET_VSCREENINFO does.
> >> 
> >> IMHO, this should be one of the following candidates,
> >>  1. set var without check.
> >>  2. do nothing but return error (setting var is not supported).
> >> or
> >>  3. it's a bug (fb_check_var should always be provided).
> >> 
> >> The patch at the bottom implements "1".
> >> 
> >> Because I don't know API specification, nor the history of the code,
> >> I would like people who knows well to discuss this.
> > 
> > If the driver doesn't provide a fb_check_var(), it means it cannot
> > change video mode. Hence this rules out #1.
> > 
> > #2 is not acceptable, as it will break existing applications. It's also
> > incorrect, as FBIOPUT_VSCREENINFO should succeed for supported modes.
> > For unsupported modes, the mode should be rounded up to a supported mode,
> > if possible. In the case of drivers that support one fixed mode only, this
> > rounding up is relaxed to `rounding' to the sole supported mode.
> > 
> > #3 is also wrong, as fb_check_var() has been deliberately made optional to
> > simplify drivers that support one fixed mode only.
> > 
> > Conclusion: nothing should be changed?
> 
> I'm not sure.
> On parisc we just stumbled over exactly this problem where I think Takashi's
> proposal "1." is probably the right solution.

What about this?

4. Provide a generic check_var() that does some basic sanity checking
against info->var (eg. check xres, yres and bits_per_pixel).

-- 
Ville Syrjälä
syrjala@sci.fi
http://www.sci.fi/~syrjala/

-------------------------------------------------------------------------
This SF.Net email is sponsored by the Moblin Your Move Developer's challenge
Build the coolest Linux based applications with Moblin SDK & win great prizes
Grand prize is a trip for two to an Open Source event anywhere in the world
http://moblin-contest.org/redirect.php?banner_id=100&url=/

Re: [PATCH] Do set var even if no fb_check_var() provided.

[Helge Deller]

Ville Syrjälä wrote:
> On Wed, Aug 27, 2008 at 11:49:15PM +0200, Helge Deller wrote:
>> Geert Uytterhoeven wrote:
>>> On Wed, 27 Aug 2008, Takashi Yoshii wrote:
>>>> The behavior of FBIOSET_VSCREENINFO seems to be weired on FB which does
>>>> not
>>>>  provides fbops->fb_check_var.
>>>> It doesn't set anything, but gets info->var instead, and returns no
>>>> error. It is just same as FBIOGET_VSCREENINFO does.
>>>>
>>>> IMHO, this should be one of the following candidates,
>>>>  1. set var without check.
>>>>  2. do nothing but return error (setting var is not supported).
>>>> or
>>>>  3. it's a bug (fb_check_var should always be provided).
>>>>
>>>> The patch at the bottom implements "1".
>>>>
>>>> Because I don't know API specification, nor the history of the code,
>>>> I would like people who knows well to discuss this.
>>> If the driver doesn't provide a fb_check_var(), it means it cannot
>>> change video mode. Hence this rules out #1.
>>>
>>> #2 is not acceptable, as it will break existing applications. It's also
>>> incorrect, as FBIOPUT_VSCREENINFO should succeed for supported modes.
>>> For unsupported modes, the mode should be rounded up to a supported mode,
>>> if possible. In the case of drivers that support one fixed mode only, this
>>> rounding up is relaxed to `rounding' to the sole supported mode.
>>>
>>> #3 is also wrong, as fb_check_var() has been deliberately made optional to
>>> simplify drivers that support one fixed mode only.
>>>
>>> Conclusion: nothing should be changed?
>> I'm not sure.
>> On parisc we just stumbled over exactly this problem where I think Takashi's
>> proposal "1." is probably the right solution.
> 
> What about this?
> 
> 4. Provide a generic check_var() that does some basic sanity checking
> against info->var (eg. check xres, yres and bits_per_pixel).

Yes, possible.
But that wouldn't solve issues with Xorg's current implementation, as 
Xorg is currently checking the monitor sync values as well (see 
https://bugs.freedesktop.org/show_bug.cgi?id=17153)

Helge

-------------------------------------------------------------------------
This SF.Net email is sponsored by the Moblin Your Move Developer's challenge
Build the coolest Linux based applications with Moblin SDK & win great prizes
Grand prize is a trip for two to an Open Source event anywhere in the world
http://moblin-contest.org/redirect.php?banner_id=100&url=/

[PATCH] Add fb_check_var() for fixed mode device.

[Takashi Yoshii]

Sounds good.
> 4. Provide a generic check_var() that does some basic sanity checking
> against info->var (eg. check xres, yres and bits_per_pixel).
How about this.
Any comments? Expecially about what to check, what to restore.

Cheers,
/yoshii # Restoring pixclock would be a workaround for Xorg issue :)

drivers/video/fbmem.c:fb_check_var()
 New function for fixed mode device which doesn't provide its own
 check_var function.

Signed-off-by: Takashi YOSHII <yoshii.takashi@renesas.com>

diff --git a/drivers/video/fbmem.c b/drivers/video/fbmem.c
index c6b8e92..ff94eaf 100644
--- a/drivers/video/fbmem.c
+++ b/drivers/video/fbmem.c
@@ -904,6 +904,56 @@ static int fb_check_caps(struct fb_info *info, struct fb_var_screeninfo *var,
 	return err;
 }
 
+/* Sanity check for drivers which can't change video mode */
+static int
+fb_check_var(struct fb_var_screeninfo *var, struct fb_info *info)
+{
+	struct fb_var_screeninfo *constant = &info->var;
+        __u32 xoffset = var->xoffset;
+	__u32 yoffset = var->yoffset;
+	__u32 activate = var->activate;
+	__u32 yres = (constant->vmode & FB_VMODE_YWRAP)? 0: var->yres; 
+
+	/* do round _up_ */
+	if (var->xres_virtual < xoffset + var->xres)
+		var->xres_virtual = xoffset + var->xres;
+	if (var->yres_virtual < yoffset + yres)
+		var->yres_virtual = yoffset + yres;
+
+	/* bigger is error, smaller is OK */
+	if( ( var->xres > constant->xres )
+	  ||( var->yres > constant->yres )
+	  ||( var->xres_virtual > constant->xres_virtual )
+	  ||( var->yres_virtual > constant->yres_virtual ))
+		return -EINVAL;
+
+	/* only length are checked for bitfields */
+	if( ( var->bits_per_pixel > constant->bits_per_pixel )
+	  ||( var->red.length > constant->red.length )
+	  ||( var->green.length > constant->green.length )
+	  ||( var->blue.length > constant->blue.length ))
+		return -EINVAL;
+
+	/* boolean parameters can't be rounded, should be equal */
+	if( ( !var->grayscale != !constant->grayscale )
+	  ||( !var->nonstd != !constant->nonstd )
+	  ||( var->vmode != constant->vmode ))
+		return -EINVAL;
+
+	/* pan is acceptable only if we have fb_pan_display) */
+	if ( (var->yoffset || var->xoffset) && !info->fbops->fb_pan_display )
+		return -EINVAL;
+
+	/* copy most */
+	*var = *constant;
+	/* resotore some that are not a part of viede mode */
+	var->xoffset = xoffset;
+	var->yoffset = yoffset;
+	var->activate = activate;
+
+	return 0;
+}
+
 int
 fb_set_var(struct fb_info *info, struct fb_var_screeninfo *var)
 {
@@ -940,9 +990,11 @@ fb_set_var(struct fb_info *info, struct fb_var_screeninfo *var)
 
 		if (info->fbops->fb_check_var) {
 			ret = info->fbops->fb_check_var(var, info);
-			if (ret)
-				goto done;
-		}
+		} else
+			ret = fb_check_var(var, info);
+
+		if (ret)
+			goto done;
 
 		if ((var->activate & FB_ACTIVATE_MASK) == FB_ACTIVATE_NOW) {
 			struct fb_videomode mode;


-------------------------------------------------------------------------
This SF.Net email is sponsored by the Moblin Your Move Developer's challenge
Build the coolest Linux based applications with Moblin SDK & win great prizes
Grand prize is a trip for two to an Open Source event anywhere in the world
http://moblin-contest.org/redirect.php?banner_id=100&url=/

Re: [PATCH] Add fb_check_var() for fixed mode device.

[Michel Dänzer]

On Fri, 2008-08-29 at 14:15 +0900, Takashi Yoshii wrote:
> 
> /yoshii # Restoring pixclock would be a workaround for Xorg issue :)

What 'Xorg issue'? I've only seen a misconfiguration and a pixclock 0
issue which has been fixed in Xorg.


-- 
Earthling Michel Dänzer           |          http://tungstengraphics.com
Libre software enthusiast         |          Debian, X and DRI developer


-------------------------------------------------------------------------
This SF.Net email is sponsored by the Moblin Your Move Developer's challenge
Build the coolest Linux based applications with Moblin SDK & win great prizes
Grand prize is a trip for two to an Open Source event anywhere in the world
http://moblin-contest.org/redirect.php?banner_id=100&url=/
_______________________________________________
Linux-fbdev-devel mailing list
Linux-fbdev-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-fbdev-devel

Re: [PATCH] Add fb_check_var() for fixed mode device.

[Takashi Yoshii]

> What 'Xorg issue'? I've only seen a misconfiguration and a pixclock 0
> issue which has been fixed in Xorg.
pixclock 0 one. Which has beed fixed in git. But I don't expect all people
 do/can build latest Xorg source. And there might be people who think modifying
 kernel is easier, especially because this is kernel related ML.
/yoshii

-------------------------------------------------------------------------
This SF.Net email is sponsored by the Moblin Your Move Developer's challenge
Build the coolest Linux based applications with Moblin SDK & win great prizes
Grand prize is a trip for two to an Open Source event anywhere in the world
http://moblin-contest.org/redirect.php?banner_id=100&url=/

Re: [PATCH] Add fb_check_var() for fixed mode device.

[Geert Uytterhoeven]

On Fri, 29 Aug 2008, Takashi Yoshii wrote:
> Sounds good.
> > 4. Provide a generic check_var() that does some basic sanity checking
> > against info->var (eg. check xres, yres and bits_per_pixel).
> How about this.
> Any comments? Expecially about what to check, what to restore.
> 
> Cheers,
> /yoshii # Restoring pixclock would be a workaround for Xorg issue :)
> 
> drivers/video/fbmem.c:fb_check_var()
>  New function for fixed mode device which doesn't provide its own
>  check_var function.
> 
> Signed-off-by: Takashi YOSHII <yoshii.takashi@renesas.com>
> 
> diff --git a/drivers/video/fbmem.c b/drivers/video/fbmem.c
> index c6b8e92..ff94eaf 100644
> --- a/drivers/video/fbmem.c
> +++ b/drivers/video/fbmem.c
> @@ -904,6 +904,56 @@ static int fb_check_caps(struct fb_info *info, struct fb_var_screeninfo *var,
>  	return err;
>  }
>  
> +/* Sanity check for drivers which can't change video mode */
> +static int
> +fb_check_var(struct fb_var_screeninfo *var, struct fb_info *info)
> +{
> +	struct fb_var_screeninfo *constant = &info->var;
> +        __u32 xoffset = var->xoffset;
> +	__u32 yoffset = var->yoffset;
> +	__u32 activate = var->activate;
> +	__u32 yres = (constant->vmode & FB_VMODE_YWRAP)? 0: var->yres; 
> +
> +	/* do round _up_ */
> +	if (var->xres_virtual < xoffset + var->xres)
> +		var->xres_virtual = xoffset + var->xres;
> +	if (var->yres_virtual < yoffset + yres)
> +		var->yres_virtual = yoffset + yres;

Why this part? var->[xy]res{,_virtual} will be overwritten by the
correct values later anyway.

> +	/* pan is acceptable only if we have fb_pan_display) */
> +	if ( (var->yoffset || var->xoffset) && !info->fbops->fb_pan_display )
> +		return -EINVAL;

You should validate var->[xy]offset against constant->[xy]res_virtual
and info->fix.[xy]{pan,wrap}step.

> +	/* copy most */
> +	*var = *constant;
> +	/* resotore some that are not a part of viede mode */
           ^^^^^^^^
	   restore
> +	var->xoffset = xoffset;
> +	var->yoffset = yoffset;
> +	var->activate = activate;
> +
> +	return 0;
> +}

Gr{oetje,eeting}s,

						Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
							    -- Linus Torvalds

-------------------------------------------------------------------------
This SF.Net email is sponsored by the Moblin Your Move Developer's challenge
Build the coolest Linux based applications with Moblin SDK & win great prizes
Grand prize is a trip for two to an Open Source event anywhere in the world
http://moblin-contest.org/redirect.php?banner_id=100&url=/

[PATCH] Add fb_check_var() for fixed mode device.

[Takashi Yoshii]

Take 2.

This adds sanitizing. But main effect is, as a result, adding
 FBIOPUT_VSCREENINFO on fixed mode device following functionality.
. return error when request screen is bigger than the fixed mode.
. round pan values if invalid
. do set pan
same as drivers which has own check_var.

>> > > +	/* do round _up_ */
>> > > +	if (var->xres_virtual < xoffset + var->xres)
>> > > +		var->xres_virtual = xoffset + var->xres;
>> > > +	if (var->yres_virtual < yoffset + yres)
>> > > +		var->yres_virtual = yoffset + yres;
> > 
> > Why this part? var->[xy]res{,_virtual} will be overwritten by the
> > correct values later anyway.
This was to a part of pan check rounding [xy]res_virtual up instead of
 rounding [xy]offset down, and check them in lines below there.
But as you pointed, it's weired for this(checking against constants) case.

> > +	/* pan is acceptable only if we have fb_pan_display) */
> > +	if ( (var->yoffset || var->xoffset) && !info->fbops->fb_pan_display )
> > +		return -EINVAL;
I changed this non-error, but resetting pan values with info->var.
This supports HW which can't change pan but have initial fixed offsets.
# So far, not in kernel source, though.

> > You should validate var->[xy]offset against constant->[xy]res_virtual
> > and info->fix.[xy]{pan,wrap}step.
I add explicit rounding down code this time, instead of returning error.
I am not sure which is right. But because I found 10 drivers round them down,
 6 return error, 2 set const value. 
# and 80 (20(with pan) + 60(no pan)) ignore them.

/yoshii

drivers/video/fbmem.c:fb_check_var(): New function
 for fixed mode device which doesn't provide its own check_var function.
   
Signed-off-by: Takashi YOSHII <yoshii.takashi@renesas.com>
diff --git a/drivers/video/fbmem.c b/drivers/video/fbmem.c
index c6b8e92..243e5f4 100644
--- a/drivers/video/fbmem.c
+++ b/drivers/video/fbmem.c
@@ -904,6 +904,68 @@ static int fb_check_caps(struct fb_info *info, struct fb_var_screeninfo *var,
 	return err;
 }
 
+/* Sanity check for drivers which can't change video mode */
+static int
+fb_check_var(struct fb_var_screeninfo *var, struct fb_info *info)
+{
+	struct fb_var_screeninfo *constant = &info->var;
+	struct fb_fix_screeninfo *fix = &info->fix;
+	__u32 xoffset = var->xoffset;
+	__u32 yoffset = var->yoffset;
+	__u32 activate = var->activate;
+	__u32 ywrap = var->vmode & FB_VMODE_YWRAP;
+
+	/* Bigger is error, smaller is OK */
+	if ((var->xres > constant->xres) ||
+	    (var->yres > constant->yres) ||
+	    (var->xres_virtual > constant->xres_virtual) ||
+	    (var->yres_virtual > constant->yres_virtual))
+		return -EINVAL;
+
+	/* Ohly length are checked for bitfields */
+	if ((var->bits_per_pixel > constant->bits_per_pixel) ||
+	    (var->red.length > constant->red.length) ||
+	    (var->green.length > constant->green.length) ||
+	    (var->blue.length > constant->blue.length))
+		return -EINVAL;
+
+	/* Boolean parameters can't be rounded, should be equal */
+	if ((!var->grayscale != !constant->grayscale) ||
+	    (!var->nonstd != !constant->nonstd) ||
+	    ((var->vmode ^ constant->vmode) &~FB_VMODE_YWRAP))
+		return -EINVAL;
+
+	/* round offsets */
+	if (xoffset > constant->xres_virtual - constant->xres)
+		xoffset = constant->xres_virtual - constant->xres;
+	if (fix->xpanstep)
+		xoffset -= (var->xoffset % fix->xpanstep);
+
+	if (ywrap) {
+		if (yoffset > constant->yres_virtual -1)
+			yoffset = constant->yres_virtual -1;
+		if (fix->ywrapstep)
+			yoffset -= (yoffset % fix->ywrapstep);
+	} else {
+		if (yoffset > constant->yres_virtual - constant->yres)
+			yoffset = constant->yres_virtual - constant->yres;
+		if (fix->ypanstep)
+			yoffset -= (yoffset % fix->ypanstep);
+	}
+
+	/* Copy most */
+	*var = *constant;
+	/* Restore some that are not a part of video mode */
+	var->activate = activate;
+	/*  You can't alter pan parameters without fb_pan_display() */
+	if ( info->fbops->fb_pan_display ) {
+		var->xoffset = xoffset;
+		var->yoffset = yoffset;
+		var->vmode = (constant->vmode &~FB_VMODE_YWRAP) | ywrap;
+	}
+	return 0;
+}
+
 int
 fb_set_var(struct fb_info *info, struct fb_var_screeninfo *var)
 {
@@ -940,9 +1002,11 @@ fb_set_var(struct fb_info *info, struct fb_var_screeninfo *var)
 
 		if (info->fbops->fb_check_var) {
 			ret = info->fbops->fb_check_var(var, info);
-			if (ret)
-				goto done;
-		}
+		} else
+			ret = fb_check_var(var, info);
+
+		if (ret)
+			goto done;
 
 		if ((var->activate & FB_ACTIVATE_MASK) == FB_ACTIVATE_NOW) {
 			struct fb_videomode mode;



-------------------------------------------------------------------------
This SF.Net email is sponsored by the Moblin Your Move Developer's challenge
Build the coolest Linux based applications with Moblin SDK & win great prizes
Grand prize is a trip for two to an Open Source event anywhere in the world
http://moblin-contest.org/redirect.php?banner_id=100&url=/

Re: [PATCH] Add fb_check_var() for fixed mode device.

[Michel Dänzer]

On Fri, 2008-08-29 at 14:15 +0900, Takashi Yoshii wrote:
> 
> +	/* bigger is error, smaller is OK */
> +	if( ( var->xres > constant->xres )
> +	  ||( var->yres > constant->yres )

The resolution must match, otherwise userspace thinks it can e.g. set
800x600 when the fixed mode is 1024x768.

I think this is just a bad idea in general though, as it reintroduces
the problem that was fixed with the check in the X server: the X server
will be mislead into thinking it can set modes that it really can't.


-- 
Earthling Michel Dänzer           |          http://tungstengraphics.com
Libre software enthusiast         |          Debian, X and DRI developer


-------------------------------------------------------------------------
This SF.Net email is sponsored by the Moblin Your Move Developer's challenge
Build the coolest Linux based applications with Moblin SDK & win great prizes
Grand prize is a trip for two to an Open Source event anywhere in the world
http://moblin-contest.org/redirect.php?banner_id=100&url=/
_______________________________________________
Linux-fbdev-devel mailing list
Linux-fbdev-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-fbdev-devel

Re: [PATCH] Add fb_check_var() for fixed mode device.

[Geert Uytterhoeven]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: TEXT/PLAIN; CHARSET=UTF-8, Size: 3591 bytes --]

On Fri, 29 Aug 2008, Michel Dänzer wrote:
> On Fri, 2008-08-29 at 14:15 +0900, Takashi Yoshii wrote:
> > +	/* bigger is error, smaller is OK */
> > +	if( ( var->xres > constant->xres )
> > +	  ||( var->yres > constant->yres )
> 
> The resolution must match, otherwise userspace thinks it can e.g. set
> 800x600 when the fixed mode is 1024x768.

It will be set later to the correct resolution. The above hunk just taks
care of the rounding rules.

Anyway, I'm still wondering whether this check is really needed. If your
application doesn't look at how fb_var_screeninfo was changed by calling
FBIOPUT_VSCREENINFO, you're in deep trouble anyway, due to the rounding
rules.

As I don't have access to a PA-RISC machine with the mentioned hardware,
I modified ps3fb by commenting out its fb_check_var() and fb_set_par()
routines (this also gave a black screen, but as far as the fbdev subsystem
is concerned, it has a working frame buffer device that supports one single
video mode only). Surprisingly, X indeed didn't like it:

| X.Org X Server 1.4.2
| Release Date: 11 June 2008
| X Protocol Version 11, Revision 0
| Build Operating System: Linux Debian (xorg-server 2:1.4.2-3)
| Current Operating System: Linux ps3 2.6.27-rc4-00176-gb8e6c91-dirty #1815 SMP Fri Aug 29 13:54:57 CEST 2008 ppc64
| Build Date: 03 August 2008  03:08:04AM
|  
|         Before reporting problems, check http://wiki.x.org
|         to make sure that you have the latest version.
| Module Loader present
| Markers: (--) probed, (**) from config file, (==) default setting,
|         (++) from command line, (!!) notice, (II) informational,
|         (WW) warning, (EE) error, (NI) not implemented, (??) unknown.
| (==) Log file: "/var/log/Xorg.0.log", Time: Fri Aug 29 13:57:26 2008
| (==) Using config file: "/etc/X11/xorg.conf"
| (EE) FBDEV(0): FBIOPUT_VSCREENINFO succeeded but modified mode
                 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
| (EE) FBDEV(0): mode initialization failed
| 
| Fatal server error:
| AddScreen/ScreenInit failed for driver 0

The "Screen" / "Display" (sub)section has `Modes "1920x1200"'. Removing
this indeed doesn't make a difference.

As I had added debug code to drivers/video/fbmem.c:fb_set_var(), I could
see what happened in the working version with mode-setting vs. the non-working
version without modesetting:
  - working, with mode-setting:
      o fb_set_var() fails for the standard 1920x1200 mode in the Xorg
        database,
      o X fallbacks to the current 1920x1200 mode.
  - non-working, without modesetting:
      o fb_set_var() doesn't fail for the standard 1920x1200 mode in the
        Xorg database, but it returns the current 1920x1200 mode,
      o X complains that the mode got modified.

Hence Xorg is broken on all embedded devices with a frame buffer driver
that supports a single fixed video mode only?

BTW, I know current Xorg is broken on several m68k platforms with non-chunky
frame buffer layouts, due to some serious bugs in the way some uncommon values
of frame buffer parameters are handled, but IIRC they shouldn't affect
`common' chunky frame buffers. Still have to look into it to fix it properly
(anyone who can donate me some spare time? ;-)

Gr{oetje,eeting}s,

						Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
							    -- Linus Torvalds

[-- Attachment #2: Type: text/plain, Size: 363 bytes --]

-------------------------------------------------------------------------
This SF.Net email is sponsored by the Moblin Your Move Developer's challenge
Build the coolest Linux based applications with Moblin SDK & win great prizes
Grand prize is a trip for two to an Open Source event anywhere in the world
http://moblin-contest.org/redirect.php?banner_id=100&url=/

[-- Attachment #3: Type: text/plain, Size: 182 bytes --]

_______________________________________________
Linux-fbdev-devel mailing list
Linux-fbdev-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-fbdev-devel

Re: [PATCH] Add fb_check_var() for fixed mode device.

[Michel Dänzer]

On Fri, 2008-08-29 at 14:16 +0200, Geert Uytterhoeven wrote:
> 
> | X.Org X Server 1.4.2

[...]

> | (EE) FBDEV(0): FBIOPUT_VSCREENINFO succeeded but modified mode

[...]

> Hence Xorg is broken on all embedded devices with a frame buffer driver
> that supports a single fixed video mode only?

No. As discussed before, you're probably missing

http://cgit.freedesktop.org/xorg/xserver/commit/?id=c095da04fe7c73b6503ef5b93549b13796c51b22

which is in the upcoming xserver 1.5 release.

(BTW, would you happen to remember what the magic pixclock value was
there for in the first place? :)


-- 
Earthling Michel Dänzer           |          http://tungstengraphics.com
Libre software enthusiast         |          Debian, X and DRI developer


-------------------------------------------------------------------------
This SF.Net email is sponsored by the Moblin Your Move Developer's challenge
Build the coolest Linux based applications with Moblin SDK & win great prizes
Grand prize is a trip for two to an Open Source event anywhere in the world
http://moblin-contest.org/redirect.php?banner_id=100&url=/
_______________________________________________
Linux-fbdev-devel mailing list
Linux-fbdev-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-fbdev-devel

Re: [PATCH] Add fb_check_var() for fixed mode device.

[Geert Uytterhoeven]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: TEXT/PLAIN; charset=UTF-8, Size: 1151 bytes --]

On Fri, 29 Aug 2008, Michel Dänzer wrote:
> On Fri, 2008-08-29 at 14:16 +0200, Geert Uytterhoeven wrote:
> > 
> > | X.Org X Server 1.4.2
> 
> [...]
> 
> > | (EE) FBDEV(0): FBIOPUT_VSCREENINFO succeeded but modified mode
> 
> [...]
> 
> > Hence Xorg is broken on all embedded devices with a frame buffer driver
> > that supports a single fixed video mode only?
> 
> No. As discussed before, you're probably missing
> 
> http://cgit.freedesktop.org/xorg/xserver/commit/?id=c095da04fe7c73b6503ef5b93549b13796c51b22
> 
> which is in the upcoming xserver 1.5 release.

Ah, upcoming. That's not current/previous/... ;-)

> (BTW, would you happen to remember what the magic pixclock value was
> there for in the first place? :)

To avoid a division by zero exception? I guess I just took a value that
looked sensible.

Gr{oetje,eeting}s,

						Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
							    -- Linus Torvalds

[-- Attachment #2: Type: text/plain, Size: 363 bytes --]

-------------------------------------------------------------------------
This SF.Net email is sponsored by the Moblin Your Move Developer's challenge
Build the coolest Linux based applications with Moblin SDK & win great prizes
Grand prize is a trip for two to an Open Source event anywhere in the world
http://moblin-contest.org/redirect.php?banner_id=100&url=/

[-- Attachment #3: Type: text/plain, Size: 182 bytes --]

_______________________________________________
Linux-fbdev-devel mailing list
Linux-fbdev-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-fbdev-devel

Re: [PATCH] Add fb_check_var() for fixed mode device.

[Michel Dänzer]

On Fri, 2008-08-29 at 16:14 +0200, Geert Uytterhoeven wrote:
> On Fri, 29 Aug 2008, Michel Dnzer wrote:
> > On Fri, 2008-08-29 at 14:16 +0200, Geert Uytterhoeven wrote:
> > > 
> > > | X.Org X Server 1.4.2
> > 
> > [...]
> > 
> > > | (EE) FBDEV(0): FBIOPUT_VSCREENINFO succeeded but modified mode
> > 
> > [...]
> > 
> > > Hence Xorg is broken on all embedded devices with a frame buffer driver
> > > that supports a single fixed video mode only?
> > 
> > No. As discussed before, you're probably missing
> > 
> > http://cgit.freedesktop.org/xorg/xserver/commit/?id=c095da04fe7c73b6503ef5b93549b13796c51b22
> > 
> > which is in the upcoming xserver 1.5 release.
> 
> Ah, upcoming. That's not current/previous/... ;-)

Yeah, it might have been nice to backport this to the 1.4 branch, but
it's kind of too late now. Of course, you can always bug your
distributor to backport it. :)


-- 
Earthling Michel Dänzer           |          http://tungstengraphics.com
Libre software enthusiast         |          Debian, X and DRI developer


-------------------------------------------------------------------------
This SF.Net email is sponsored by the Moblin Your Move Developer's challenge
Build the coolest Linux based applications with Moblin SDK & win great prizes
Grand prize is a trip for two to an Open Source event anywhere in the world
http://moblin-contest.org/redirect.php?banner_id=100&url=/
_______________________________________________
Linux-fbdev-devel mailing list
Linux-fbdev-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-fbdev-devel

Re: [PATCH] Add fb_check_var() for fixed mode device.

[Helge Deller]

[-- Attachment #1: Type: text/plain, Size: 2256 bytes --]

Michel Dänzer wrote:
> On Fri, 2008-08-29 at 16:14 +0200, Geert Uytterhoeven wrote:
>> On Fri, 29 Aug 2008, Michel Dnzer wrote:
>>> On Fri, 2008-08-29 at 14:16 +0200, Geert Uytterhoeven wrote:
>>>> | X.Org X Server 1.4.2
>>> [...]
>>>
>>>> | (EE) FBDEV(0): FBIOPUT_VSCREENINFO succeeded but modified mode
>>> [...]
>>>
>>>> Hence Xorg is broken on all embedded devices with a frame buffer driver
>>>> that supports a single fixed video mode only?
>>> No. As discussed before, you're probably missing
>>>
>>> http://cgit.freedesktop.org/xorg/xserver/commit/?id=c095da04fe7c73b6503ef5b93549b13796c51b22
>>>
>>> which is in the upcoming xserver 1.5 release.
>> Ah, upcoming. That's not current/previous/... ;-)
> 
> Yeah, it might have been nice to backport this to the 1.4 branch, but
> it's kind of too late now. Of course, you can always bug your
> distributor to backport it. :)

The attached patch is a completely different approach to solve the 
issue. I've tested it, and it works even with the buggy version of X 
(1.4.2).

Basic idea is, that when we have fixed mode devices (aka drivers which 
don't implement fb_check_var) we just simply fill in a "emulated" valid 
monitor/modeline into the var struct during the register_framebuffer() 
initialization call. Since the modeline is valid, X does not has any 
problems with it.

The patch has a few benefits:
a) We still keep binary compatability/behavior for FBIOGET_VSCREENINFO 
and FBIOPUT_VSCREENINFO.
b) In Xorg.conf files, users can simply delete all monitor and display 
sections -> Xorg will autoconfigure itself to the only valid 
resolution/modeline itself on all devices (I really like this!!!)

Thus, the patch basically changes the fixed mode drivers to have some 
monitor timings by default. As an example, before I got:
root@c3000:~# fbset -i
mode "1024x768"
     geometry 1024 768 1024 768 8
     timings 0 0 0 0 0 0 0
     rgba 8/0,8/0,8/0,0/0
endmode

Now I see:
root@c3000:~# fbset -i
mode "1024x768-60"
     # D: 65.003 MHz, H: 48.365 kHz, V: 60.006 Hz
     geometry 1024 768 1024 768 8
     timings 15384 168 8 29 3 144 6
     rgba 8/0,8/0,8/0,0/0
endmode

Opinions ?

Signed-off-by: Helge Deller <deller@gmx.de>



[-- Attachment #2: fb_check_var_fix.patch --]
[-- Type: text/x-patch, Size: 1005 bytes --]

diff --git a/drivers/video/fbmem.c b/drivers/video/fbmem.c
index 98843c2..72b3113 100644
--- a/drivers/video/fbmem.c
+++ b/drivers/video/fbmem.c
@@ -1470,9 +1470,23 @@ register_framebuffer(struct fb_info *fb_info)
 	if (!fb_info->pixmap.blit_y)
 		fb_info->pixmap.blit_y = ~(u32)0;
 
-	if (!fb_info->modelist.prev || !fb_info->modelist.next)
+	if (!fb_info->modelist.prev || !fb_info->modelist.next) {
 		INIT_LIST_HEAD(&fb_info->modelist);
 
+		/* For drivers which does not support mode setting
+		 * initialize an emulated modeline */
+		if (!fb_info->fbops->fb_check_var &&
+		     fb_info->var.pixclock == 0) {
+			char modestr[32];
+			snprintf(modestr, sizeof(modestr), "%dx%d-%d",
+				fb_info->var.xres,
+				fb_info->var.yres,
+				fb_info->var.bits_per_pixel);
+			fb_find_mode(&fb_info->var, fb_info, modestr,
+				NULL, 0, NULL, fb_info->var.bits_per_pixel);
+		}
+	}
+
 	fb_var_to_videomode(&mode, &fb_info->var);
 	fb_add_videomode(&mode, &fb_info->modelist);
 	registered_fb[i] = fb_info;

[-- Attachment #3: Type: text/plain, Size: 363 bytes --]

-------------------------------------------------------------------------
This SF.Net email is sponsored by the Moblin Your Move Developer's challenge
Build the coolest Linux based applications with Moblin SDK & win great prizes
Grand prize is a trip for two to an Open Source event anywhere in the world
http://moblin-contest.org/redirect.php?banner_id=100&url=/

[-- Attachment #4: Type: text/plain, Size: 182 bytes --]

_______________________________________________
Linux-fbdev-devel mailing list
Linux-fbdev-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-fbdev-devel

Re: [PATCH] Add fb_check_var() for fixed mode device.

[Helge Deller]

Helge Deller wrote:
> The attached patch is a completely different approach to solve the 
> issue. I've tested it, and it works even with the buggy version of X 
> (1.4.2).
> 
> Basic idea is, that when we have fixed mode devices (aka drivers which 
> don't implement fb_check_var) we just simply fill in a "emulated" valid 
> monitor/modeline into the var struct during the register_framebuffer() 
> initialization call. Since the modeline is valid, X does not has any 
> problems with it.
> 
> The patch has a few benefits:
> a) We still keep binary compatability/behavior for FBIOGET_VSCREENINFO 
> and FBIOPUT_VSCREENINFO.
> b) In Xorg.conf files, users can simply delete all monitor and display 
> sections -> Xorg will autoconfigure itself to the only valid 
> resolution/modeline itself on all devices (I really like this!!!)
> 
> Thus, the patch basically changes the fixed mode drivers to have some 
> monitor timings by default. As an example, before I got:
> root@c3000:~# fbset -i
> mode "1024x768"
>     geometry 1024 768 1024 768 8
>     timings 0 0 0 0 0 0 0
>     rgba 8/0,8/0,8/0,0/0
> endmode
> 
> Now I see:
> root@c3000:~# fbset -i
> mode "1024x768-60"
>     # D: 65.003 MHz, H: 48.365 kHz, V: 60.006 Hz
>     geometry 1024 768 1024 768 8
>     timings 15384 168 8 29 3 144 6
>     rgba 8/0,8/0,8/0,0/0
> endmode
> 
> Opinions ?

Sadly nobody answered yet....

Helge

-------------------------------------------------------------------------
This SF.Net email is sponsored by the Moblin Your Move Developer's challenge
Build the coolest Linux based applications with Moblin SDK & win great prizes
Grand prize is a trip for two to an Open Source event anywhere in the world
http://moblin-contest.org/redirect.php?banner_id=100&url=/

Re: [PATCH] Add fb_check_var() for fixed mode device.

[Michel Dänzer]

On Tue, 2008-09-02 at 21:11 +0200, Helge Deller wrote:
> Helge Deller wrote:
> > The attached patch is a completely different approach to solve the 
> > issue. I've tested it, and it works even with the buggy version of X 
> > (1.4.2).
> > 
> > Basic idea is, that when we have fixed mode devices (aka drivers which 
> > don't implement fb_check_var) we just simply fill in a "emulated" valid 
> > monitor/modeline into the var struct during the register_framebuffer() 
> > initialization call. Since the modeline is valid, X does not has any 
> > problems with it.
> > 
> > The patch has a few benefits:
> > a) We still keep binary compatability/behavior for FBIOGET_VSCREENINFO 
> > and FBIOPUT_VSCREENINFO.
> > b) In Xorg.conf files, users can simply delete all monitor and display 
> > sections -> Xorg will autoconfigure itself to the only valid 
> > resolution/modeline itself on all devices (I really like this!!!)
> > 
> > Thus, the patch basically changes the fixed mode drivers to have some 
> > monitor timings by default. As an example, before I got:
> > root@c3000:~# fbset -i
> > mode "1024x768"
> >     geometry 1024 768 1024 768 8
> >     timings 0 0 0 0 0 0 0
> >     rgba 8/0,8/0,8/0,0/0
> > endmode
> > 
> > Now I see:
> > root@c3000:~# fbset -i
> > mode "1024x768-60"
> >     # D: 65.003 MHz, H: 48.365 kHz, V: 60.006 Hz
> >     geometry 1024 768 1024 768 8
> >     timings 15384 168 8 29 3 144 6
> >     rgba 8/0,8/0,8/0,0/0
> > endmode
> > 
> > Opinions ?
> 
> Sadly nobody answered yet....

FWIW, I like this approach much better.


-- 
Earthling Michel Dänzer           |          http://tungstengraphics.com
Libre software enthusiast         |          Debian, X and DRI developer


-------------------------------------------------------------------------
This SF.Net email is sponsored by the Moblin Your Move Developer's challenge
Build the coolest Linux based applications with Moblin SDK & win great prizes
Grand prize is a trip for two to an Open Source event anywhere in the world
http://moblin-contest.org/redirect.php?banner_id=100&url=/
_______________________________________________
Linux-fbdev-devel mailing list
Linux-fbdev-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-fbdev-devel

Re: [PATCH] Add fb_check_var() for fixed mode device.

[Helge Deller]

Michel Dänzer wrote:
> On Tue, 2008-09-02 at 21:11 +0200, Helge Deller wrote:
>> Helge Deller wrote:
>>> The attached patch is a completely different approach to solve the 
>>> issue. I've tested it, and it works even with the buggy version of X 
>>> (1.4.2).
>>>
>>> Basic idea is, that when we have fixed mode devices (aka drivers which 
>>> don't implement fb_check_var) we just simply fill in a "emulated" valid 
>>> monitor/modeline into the var struct during the register_framebuffer() 
>>> initialization call. Since the modeline is valid, X does not has any 
>>> problems with it.
>>>
>>> The patch has a few benefits:
>>> a) We still keep binary compatability/behavior for FBIOGET_VSCREENINFO 
>>> and FBIOPUT_VSCREENINFO.
>>> b) In Xorg.conf files, users can simply delete all monitor and display 
>>> sections -> Xorg will autoconfigure itself to the only valid 
>>> resolution/modeline itself on all devices (I really like this!!!)
>>>
>>> Thus, the patch basically changes the fixed mode drivers to have some 
>>> monitor timings by default. As an example, before I got:
>>> root@c3000:~# fbset -i
>>> mode "1024x768"
>>>     geometry 1024 768 1024 768 8
>>>     timings 0 0 0 0 0 0 0
>>>     rgba 8/0,8/0,8/0,0/0
>>> endmode
>>>
>>> Now I see:
>>> root@c3000:~# fbset -i
>>> mode "1024x768-60"
>>>     # D: 65.003 MHz, H: 48.365 kHz, V: 60.006 Hz
>>>     geometry 1024 768 1024 768 8
>>>     timings 15384 168 8 29 3 144 6
>>>     rgba 8/0,8/0,8/0,0/0
>>> endmode
>>>
>>> Opinions ?
>> Sadly nobody answered yet....
> 
> FWIW, I like this approach much better.

Thanks Michel,

I'll send an updated patch shortly (in a new thread) which
fixes the english comment as well.

Helge


-------------------------------------------------------------------------
This SF.Net email is sponsored by the Moblin Your Move Developer's challenge
Build the coolest Linux based applications with Moblin SDK & win great prizes
Grand prize is a trip for two to an Open Source event anywhere in the world
http://moblin-contest.org/redirect.php?banner_id=100&url=/
_______________________________________________
Linux-fbdev-devel mailing list
Linux-fbdev-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-fbdev-devel

Re: [PATCH] Add fb_check_var() for fixed mode device.

[Takashi Yoshii]

Hi,

>> Basic idea is, that when we have fixed mode devices (aka drivers which 
>> don't implement fb_check_var) we just simply fill in a "emulated" valid 
>> monitor/modeline into the var struct during the register_framebuffer() 
>> initialization call. Since the modeline is valid, X does not has any 
>> problems with it.
It works nicely, and simple. but,
IMHO, It is not bad for workaround, but not for merging into mainline.
If it is only for workaround, setting info->var->pixclock(and some others)
 in your driver, is simpler (assuming we have source code).

Generally, pixclock is 0 in purpose.
It means there really is no clock, or author didn't have enough HW spec.
Anyway, it indicate there is no information about pixclock.
I think nothing other than 0 is suitable for this, because 
this is T of clock, and T=0 is logically invalid (f=INF?).
.... I think.

If it is true as fbdev specification, that means applications must ignore
 timings when pixclock==0.
/yoshii

-------------------------------------------------------------------------
This SF.Net email is sponsored by the Moblin Your Move Developer's challenge
Build the coolest Linux based applications with Moblin SDK & win great prizes
Grand prize is a trip for two to an Open Source event anywhere in the world
http://moblin-contest.org/redirect.php?banner_id=100&url=/

Re: [PATCH] Add fb_check_var() for fixed mode device.

[Ville Syrjälä]

On Fri, Aug 29, 2008 at 02:16:50PM +0200, Geert Uytterhoeven wrote:
> On Fri, 29 Aug 2008, Michel D?nzer wrote:
> > On Fri, 2008-08-29 at 14:15 +0900, Takashi Yoshii wrote:
> > > +	/* bigger is error, smaller is OK */
> > > +	if( ( var->xres > constant->xres )
> > > +	  ||( var->yres > constant->yres )
> > 
> > The resolution must match, otherwise userspace thinks it can e.g. set
> > 800x600 when the fixed mode is 1024x768.
> 
> It will be set later to the correct resolution. The above hunk just taks
> care of the rounding rules.
> 
> Anyway, I'm still wondering whether this check is really needed. If your
> application doesn't look at how fb_var_screeninfo was changed by calling
> FBIOPUT_VSCREENINFO, you're in deep trouble anyway, due to the rounding
> rules.

It can be used to to quickly check a bunch of modes for with 
FB_ACTIVATE_TEST. In fact FB_ACTIVATE_TEST's comment says that rounding
up should not happen here but no driver implements that. It could be
implemented in the common code by keeping a copy of the original var and
comparing them after the driver has done the rounding.

But what are the rounding rules anyway?
My take on the matter:
- xres_virtual/yres_virtual can be rounded up.
- xres/yres shouldn't be rounded since this is the first thing users
  usually care about.
- bits_per_pixel/red/gren/blue/transp shouldn't be rounded since
  it's the second thing users care about. I think most driver do round
  these though. In fact most drivers don't really even look at anything
  besides bits_per_pixel and green.length. Perhaps if the user has set
  some of them to zero the driver could pick freely.
- Timing values can be rounded. Which way or by how much I'm not sure.
  Refresh rate is the third thing users care about so huge rounding here
  could be problematic but some rounding is probably needed due to
  hardware constraints. In the "hardware supports only one mode" case
  the rounding should probably be unlimited though.
- xoffset/yoffset should be checked against panstep/wrap.
  FBIOPAN_DISPLAY doesn't allow any rounding but should
  FBIOPUT_VSCREENINFO?

Maybe some of the constraints (like limiting the refresh rate rounding to
some %) should only be enforced with FB_ACTIVATE_TEST. That would
prevent it breaking some stupid application that just sets some
randomish mode and expects the driver to hand out something that's supported.

-- 
Ville Syrjälä
syrjala@sci.fi
http://www.sci.fi/~syrjala/

-------------------------------------------------------------------------
This SF.Net email is sponsored by the Moblin Your Move Developer's challenge
Build the coolest Linux based applications with Moblin SDK & win great prizes
Grand prize is a trip for two to an Open Source event anywhere in the world
http://moblin-contest.org/redirect.php?banner_id=100&url=/

Re: [PATCH] Do set var even if no fb_check_var() provided.

[Takashi Yoshii]

Thank you for your explanation.
Sorry for slow, long response.
Summary:
 1. I understand your strategy (no check == fix video).
 2. pan should be fixed state after set var.
 3. pixclock can be any, OK?

---
1. 
> > If the driver doesn't provide a fb_check_var(), it means it cannot
> > change video mode. Hence this rules out #1.
Well, this logic looks like "A is B because A is B".
But, anyway this should be this because you say so.
Accepting this as a fixed rule, things becomes simple.

I guess this is based on the idea
 var should always be corresponding to HW state.
My patch was based on the idea
 var can be any, because HW accept all (by just ignoring all).

> > #2 is not acceptable, as it will break existing applications. It's also
> > incorrect, as FBIOPUT_VSCREENINFO should succeed for supported modes.
Right. It's important not to break existing application. I found Xorg server
 does set var then check it to see if the operation was successful.
Perhaps that is the usage of this API. So, returning error seems bad.

> > #3 is also wrong, as fb_check_var() has been deliberately made optional to
> > simplify drivers that support one fixed mode only.
OK. I don't think it is bad idea.

---
2. pan should be fixed state after set var.

# This is becoming another topic, though...
One small problem of current code is that
 FBIOPUT_VSCREENINFO sometimes set PAN but sometimes does not.
# assuming pan is not a part of "video mode".

It will be 1.unchanged, or 2.info->var, or 3.var, or 4.var+rounding?,
but generally pan state after FBIOPUT_VSCREENINFO should be considered
 as unknown(but something valid), even if passing valid value(say, (0,0)).

It's ok if set or not. But, I believe it should be deterministic.

---
3. pixclock can be any, OK?

The real problem(for me;) might be
. Habit of applications that check the var by themselves.
This is  not actually a topic on this ML. But I want to confirm that
  pixclock can be any. Even 0 is a kind of "Valid" value.
OK?

I'm asking because, there are drivers return timing parameters as 0.
- HW has clock, known, but set 0 (sh_mobile_lcdfb).
- HW must have clock, but unknown (hitfb, stifb, xilinxfb).
- No clock (xenfb).
There are drivers return false value.
- No clock but return some (vfb).

If 0 is valid, problem is Xorg's issue.
X(at least 1.4) doesn't accept clock=0. (and the reason looks be buggy)

Regards,
/yoshii



-------------------------------------------------------------------------
This SF.Net email is sponsored by the Moblin Your Move Developer's challenge
Build the coolest Linux based applications with Moblin SDK & win great prizes
Grand prize is a trip for two to an Open Source event anywhere in the world
http://moblin-contest.org/redirect.php?banner_id=100&url=/

Re: [PATCH] Do set var even if no fb_check_var() provided.

[Geert Uytterhoeven]

On Fri, 29 Aug 2008, Takashi Yoshii wrote:
> Thank you for your explanation.
> Sorry for slow, long response.
> Summary:
> 1. I understand your strategy (no check == fix video).
> 2. pan should be fixed state after set var.
> 3. pixclock can be any, OK?
> 
> ---
> 1. 
> > > If the driver doesn't provide a fb_check_var(), it means it cannot
> > > change video mode. Hence this rules out #1.
> Well, this logic looks like "A is B because A is B".
> But, anyway this should be this because you say so.
> Accepting this as a fixed rule, things becomes simple.
> 
> I guess this is based on the idea
> var should always be corresponding to HW state.
> My patch was based on the idea
> var can be any, because HW accept all (by just ignoring all).

No, var cannot be any. Var can only be the single tuple of values the
hardware supports.

> 2. pan should be fixed state after set var.
> 
> # This is becoming another topic, though...
> One small problem of current code is that
> FBIOPUT_VSCREENINFO sometimes set PAN but sometimes does not.
> # assuming pan is not a part of "video mode".
> 
> It will be 1.unchanged, or 2.info->var, or 3.var, or 4.var+rounding?,
> but generally pan state after FBIOPUT_VSCREENINFO should be considered
> as unknown(but something valid), even if passing valid value(say, (0,0)).

Panning state is not unknown. It's in var.

> 3. pixclock can be any, OK?
> 
> The real problem(for me;) might be
> . Habit of applications that check the var by themselves.
> This is  not actually a topic on this ML. But I want to confirm that
>  pixclock can be any. Even 0 is a kind of "Valid" value.
> OK?
> 
> I'm asking because, there are drivers return timing parameters as 0.
> - HW has clock, known, but set 0 (sh_mobile_lcdfb).
> - HW must have clock, but unknown (hitfb, stifb, xilinxfb).
> - No clock (xenfb).
> There are drivers return false value.
> - No clock but return some (vfb).
> 
> If 0 is valid, problem is Xorg's issue.
> X(at least 1.4) doesn't accept clock=0. (and the reason looks be buggy)

If you don't know the pixclock, set it to 0.

Gr{oetje,eeting}s,

						Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
							    -- Linus Torvalds

-------------------------------------------------------------------------
This SF.Net email is sponsored by the Moblin Your Move Developer's challenge
Build the coolest Linux based applications with Moblin SDK & win great prizes
Grand prize is a trip for two to an Open Source event anywhere in the world
http://moblin-contest.org/redirect.php?banner_id=100&url=/

Re: [PATCH] Do set var even if no fb_check_var() provided.

[Takashi Yoshii]

Geert Uytterhoeven wrote:
> Panning state is not unknown. It's in var.
I don't think it is not always in var.
I found there are following three types of drivers.

1. ideal(?) driver
FBIOPUT_VSCREENINFO: pan (rounded a_set_varnd) done, no error, pan in returned var
FBIOGET_VSCREENINFO: returns pan

2. driver which doesn't provide own check_var (fixed mode drivers)
FBIOPUT_VSCREENINFO: no pan done, no error, false pan in returned var
FBIOGET_VSCREENINFO: returns pan

3. driver which doesn't provede pan_display nor check pan in check_var
FBIOPUT_VSCREENINFO: no pan done, no error, false pan in returned var
FBIOGET_VSCREENINFO: returns false pan

2 will be trouble if people expect they can initialize pan by
FBIOPUT_VSCREENINFO. Actually, you need FBIOPAN_DISPLAY afterwards.
Or when they believe returned var have valid value for pan.
Actually, you need FBIOGET_VSCREENINFO (anyway, you need FBIOPAN_DISPLAY as
 described above, this one is not so big problem).

3 is weired. info->var continuously have value which can not be set to HW.
Many drivers which doesn't support pan seems to be this type
(sstfb is one of exceptions).
Possibly, we can fix it at the middle of fb_set_var().
But because of the rule(?) that only *_check_var() can freely modify var, 
In this case, *_check_var() in each driver should do return error, or set
 them 0. Right?

> If you don't know the pixclock, set it to 0.
OK. Thank you.
/yoshii


-------------------------------------------------------------------------
This SF.Net email is sponsored by the Moblin Your Move Developer's challenge
Build the coolest Linux based applications with Moblin SDK & win great prizes
Grand prize is a trip for two to an Open Source event anywhere in the world
http://moblin-contest.org/redirect.php?banner_id=100&url=/