EDID vs. fbmon & radeon

EDID vs. fbmon & radeon

[Benjamin Herrenschmidt]

Hi James !

I'm working on radeonfb to implement full DDC2 support among other,
however, I'm having a problem with the way you currently put the EDID
code in fbmon.c

Right now, most of the actual EDID parsing is done within fbmon with
static functions, all I can do from the "outside" is to either use
parse_edid which will get me the first detailed timing block or
build a modedb, which will cause me to lose the information of what
comes from the detailed block, and what is a strandard VESA timing.

For proper flat panel setup, I need to actually know what is a detailed
timing block and iterate over those specifically to figure out the
proper panel informations that I'll need later on to configure the
RMX (scaler engine), among others. For now, I'll be using parse_edid()
but I would prefer iterating them all 4 so I can pick the largest one,
in case the panel does HW scaling and expose that some way...

We could either export low-level parsing routines from fbmon.c so I
can do my own parsing in special cases like this one, we could just
extend the modedb created with a few flags indicating for each mode
where it comes from (detailed block, vesa block), or we could output
some different EDID-specific data structure containing the parsed
EDID in various arrays like XFree does....

What do you think ?

(BTW. What about those fb_client patches ? Any chance we get that in
Linus tree soon ? )

Ben.
 



-------------------------------------------------------
This sf.net email is sponsored by:ThinkGeek
Welcome to geek heaven.
http://thinkgeek.com/sf

Re: EDID vs. fbmon & radeon

[James Simmons]

> I'm working on radeonfb to implement full DDC2 support among other,
> however, I'm having a problem with the way you currently put the EDID
> code in fbmon.c

I have been watching. Really cool stuff. 
 
> Right now, most of the actual EDID parsing is done within fbmon with
> static functions, all I can do from the "outside" is to either use
> parse_edid which will get me the first detailed timing block or
> build a modedb, which will cause me to lose the information of what
> comes from the detailed block, and what is a strandard VESA timing.
> 
> For proper flat panel setup, I need to actually know what is a detailed
> timing block and iterate over those specifically to figure out the
> proper panel informations that I'll need later on to configure the
> RMX (scaler engine), among others. For now, I'll be using parse_edid()
> but I would prefer iterating them all 4 so I can pick the largest one,
> in case the panel does HW scaling and expose that some way...
> 
> We could either export low-level parsing routines from fbmon.c so I
> can do my own parsing in special cases like this one, we could just
> extend the modedb created with a few flags indicating for each mode
> where it comes from (detailed block, vesa block), or we could output
> some different EDID-specific data structure containing the parsed
> EDID in various arrays like XFree does....
> 
> What do you think ?

Hm. The most important goal I think should be that as much as possible 
code sharing goes on. The way XFree86 handles it is that th edrivers only 
have to deal with 2 functions for writing the i2c register. Everything 
else is handled by a upper layer. I like to see fbdev have the same thing.

> (BTW. What about those fb_client patches ? Any chance we get that in
> Linus tree soon ? )

Yes. I have viewed the code and I like to go over some ideas with you on 
it. I will post my comments tomorrow. 




-------------------------------------------------------
This sf.net email is sponsored by:ThinkGeek
Welcome to geek heaven.
http://thinkgeek.com/sf

Re: EDID vs. fbmon & radeon

[Benjamin Herrenschmidt]

> Hm. The most important goal I think should be that as much as possible 
> code sharing goes on. The way XFree86 handles it is that th edrivers only 
> have to deal with 2 functions for writing the i2c register. Everything 
> else is handled by a upper layer. I like to see fbdev have the same thing.

If you look at my radeonfb work, you'll notice that I do pretty much like
XFree: that is I provide low level i2c routines to the kernel i2c layer,
and later one use these along with chip specific code (actually copied from
XFree) to retreive the EDID. It can't be all factored due to some HW gory
details though.

What XFree does though is to parse the EDID and expose structures containing
the decoded timing infos so that the driver can actually pick individual
modes in there knowing which ones come from detailed timing blocks and
chich ones are calculated VESA modes. We could acheive the same by adding
some flags to the modedb we generate.

Ben.




-------------------------------------------------------
This sf.net email is sponsored by:ThinkGeek
Welcome to geek heaven.
http://thinkgeek.com/sf

Re: EDID vs. fbmon & radeon

[James Simmons]

> > Hm. The most important goal I think should be that as much as possible 
> > code sharing goes on. The way XFree86 handles it is that th edrivers only 
> > have to deal with 2 functions for writing the i2c register. Everything 
> > else is handled by a upper layer. I like to see fbdev have the same thing.
> 
> If you look at my radeonfb work, you'll notice that I do pretty much like
> XFree: that is I provide low level i2c routines to the kernel i2c layer,
> and later one use these along with chip specific code (actually copied from
> XFree) to retreive the EDID. It can't be all factored due to some HW gory
> details though.

:-( I just want to make the drivers as simple as possible to create. 
 
> What XFree does though is to parse the EDID and expose structures containing
> the decoded timing infos so that the driver can actually pick individual
> modes in there knowing which ones come from detailed timing blocks and
> chich ones are calculated VESA modes. We could acheive the same by adding
> some flags to the modedb we generate.

Make it so.....

P.S
	So I got swarmed with emails. I had some reading to do. BTW where 
is this new code. I like to commit to fbdev BK tree. 

P.S.S
	I did some work on your fbdev pm stuff. I will send another email 
about this.




-------------------------------------------------------
This sf.net email is sponsored by:ThinkGeek
Welcome to geek heaven.
http://thinkgeek.com/sf

Fbdev and PM patch.

[James Simmons]

Hi!

    I looked at your PM patch and I have a few ideas. The first thing I 
did was create a dummy struct fb_ops instead of dummy functions for each 
function for fb_ops. Plus you only had dummy functions for the basic 
functions. So in the style of dummycon.c I created a dummy struct fb_ops.
    I like to seperate out the client management code out of fbmem.c into 
a seperate file. Especially with the device handling stuff it is quite 
ugly looking. Here is what I suggest:

    struct fb_client_ops {
	struct module *owner;
	void (*state_manager)(struct fb_info *info, void *data, u32 state);
    }

The idea is instead of a bunch of functions we have one function for 
state management. In struct fb_info we keep track of the state flag to see
what the states are. We have something like this:

int
fb_change_state(struct fb_info *info, u32 state)
{
	if (info->state & state)
		return;

	Your fb_client_call_##name code except it is now only one 
	function.
}			

What do you think?




-------------------------------------------------------
This sf.net email is sponsored by:ThinkGeek
Welcome to geek heaven.
http://thinkgeek.com/sf

Re: Fbdev and PM patch.

[Benjamin Herrenschmidt]

On Fri, 2003-09-12 at 20:08, James Simmons wrote:
> Hi!
> 
>     I looked at your PM patch and I have a few ideas. The first thing I 
> did was create a dummy struct fb_ops instead of dummy functions for each 
> function for fb_ops. Plus you only had dummy functions for the basic 
> functions. So in the style of dummycon.c I created a dummy struct fb_ops.
>     I like to seperate out the client management code out of fbmem.c into 
> a seperate file. Especially with the device handling stuff it is quite 
> ugly looking. Here is what I suggest:

I kept the fb_ops on purpose. If you look at my implementation in
radeonfb, you'll see for example that the check_var/set_par fully
works during sleep, except that the HW isn't touched, but on wakeup,
the mode that was set during sleep will be applied properly.

If you put dummy fb_ops, you'll either let var be changed without
control from the driver, or make any set_var fail, which is a policy
decision I didn't want to take (but acceptable).

>     struct fb_client_ops {
> 	struct module *owner;
> 	void (*state_manager)(struct fb_info *info, void *data, u32 state);
>     }
> 
> The idea is instead of a bunch of functions we have one function for 
> state management. In struct fb_info we keep track of the state flag to see
> what the states are. We have something like this:
> 
> int
> fb_change_state(struct fb_info *info, u32 state)
> {
> 	if (info->state & state)
> 		return;
> 
> 	Your fb_client_call_##name code except it is now only one 
> 	function.
> }			
> 
> What do you think?

Well... The mode change notification isn't a state per-se and would
benefit from beeing a separate function. For the Power Management
callbacks, I don't mind either way

Ben.




-------------------------------------------------------
This sf.net email is sponsored by:ThinkGeek
Welcome to geek heaven.
http://thinkgeek.com/sf