[PATCH] DDC2/I2C Support

[PATCH] DDC2/I2C Support

[adaplas]

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

Hi all,

I had the Christmas to work a little bit more on EDID/DDC/I2C for fb
drivers. I know they can be implemented in userspace, however, we do need
some sane way of setting video modes since the framebuffer system is
initialized quite early in the boot process.  Still, setting modes using
stty is still a bit problematic.  We need a way to tell fbdev whether to
accept or ignore the timings in fb_var_screeninfo.  These cases are
manifested when changing resolution using stty or using fbset.

Anyway, attached is a patch (gzipped) against linux-2.6.1-rc1. Sorry for
lumping everything into one big patch since I don't have time to separate
them into different patches.

1. [fbi2c.c]:  Added preliminary generic DDC2/i2c support (derived from
xf86DDC.c and xf86i2.c)

	This is a configurable option (CONFIG_FB_I2C). 	For now, I only used
"default" timeout values which may not work for all devices.

	Most drivers need to implement only two functions --

		void xxxfb_i2c_getbits(struct fb_info *info, signed long *clock, signed
long *data)
		void xxxfb_i2c_putbits(struct fb_info *info, signed long clock, signed
long data)

	The functionality is exposed in the following:

		unsigned char *fb_get_edid_from_ddc2(struct fb_info *info)

2. [fbmon.c]: Consolidated different ways of getting EDID with the
following function:

	unsigned char *fb_get_edid(struct fb_info *info, struct pci_dev *pdev,
int what)

		the parameter "what" can be:
			EDID_DDC2 - get EDID via DDC2/I2C fb_get_edid_from_ddc2()
			EDID_ARCH - get	EDID via BIOS or OF get_EDID_from_{OF|BIOS}()

	2.a getting EDID from the BIOS is now configurable (CONFIG_EDID_BIOS)
since some people reported problems during booting or it takes a long
time for the process to finish.

3. [modedb.c]: Added additional field (edid_src) to struct fb_videomode.

	The idea came from Ben H. He suggested that the mode database built by
fb_create_modedb() does not specify where each came from.  So
fb_videomode.edid_src can be:

	MODE_IS_UNKNOWN - defined as (0) -- all entries of modedb[] in fbmon.c
	MODE_IS_DETAILED -  mode came from detailed timings block of EDID.  They
are the most preferred mode since all timings are specified in the block.
	MODE_IS_STANDARD - mode came from standard timings block usually in the
form of [xres]x[yres]@refresh
	MODE_IS_VESA - mode is VESA standard
	MODE_IS_CALCULATED - mode is calculated using GTF (xres, yres, refresh).
	MODE_IS_FIRST - if set, mode is the first entry in the detailed timing
block and is the preferred mode of the display.

	Flags can be combined in different ways such as:

	MODE_IS_STANDARD | MODE_IS_CALCULATED - ie, mode came from standard
timing block, but mode was not found in VESA database, and instead
calculated using GTF.

4. [fbmon.c]: Extended struct fb_monspecs to include display information
specified in EDID.

	Use function
		void fb_get_monspecs(unsigned char *edid, struct fb_monspecs *specs)

------------------------------------------
struct fb_chroma {
	__u32 redx;         /* divide value by 1024 */
	__u32 greenx;
	__u32 bluex;
	__u32 whitex;
	__u32 redy;
	__u32 greeny;
	__u32 bluey;
	__u32 whitey;
};

struct fb_monspecs {
        struct fb_chroma chroma;
	__u32 hfmin;			/* hfreq lower limit (Hz) */
	__u32 hfmax; 			/* hfreq upper limit (Hz) */
	__u16 vfmin;			/* vfreq lower limit (Hz) */
	__u16 vfmax;			/* vfreq upper limit (Hz) */
	__u32 dclkmin;                  /* pixelclock lower limit (Hz) */
	__u32 dclkmax;                  /* pixelclock upper limit (Hz) */
	unsigned input;                 /* display type - see FB_DISP_* */
	unsigned dpms;  		/* DPMS support - see FB_DPMS_ */
	unsigned signal;                /* Signal Type - see FB_SIGNAL_* */
	unsigned char max_x;            /* Maximum horizontal size (cm) */
	unsigned char max_y;            /* Maximum vertical size (cm) */
	unsigned gamma;                 /* Gamma - divide value by 100 */
	unsigned gtf  : 1;              /* supports GTF */
	unsigned misc;                  /* Misc flags - see FB_MISC_* */
};

--------------------------------------------------------

5. [fbcon.c]: Minor fixes on the cursor and scrolling.

6. [RIVAFB]:

	a. Implemented I2C/DDC2 support.  Changing video modes should be more
robust even when using stty.
	b. fixed various hardware cursor corruption
	c. fixed maximum yres_virtual.  Hardware accel engine will crash if blit
rectangle dimensions/origin exceeds 0x7fff.
	d. added new boot parameter, "strictmode", to force driver to use only
modes specified in the EDID block.


Comments?

Tony


[-- Attachment #2: patch-fblinux-2.6.0-rc1.diff.gz --]
[-- Type: application/x-gzip, Size: 16946 bytes --]

Re: [PATCH] DDC2/I2C Support

[Antonino A. Daplas]

On Tuesday 06 January 2004 01:16, Kronos wrote:
> Il Mon, Jan 05, 2004 at 09:31:02AM -0500, adaplas@pol.net ha scritto:
> > 1. [fbi2c.c]:  Added preliminary generic DDC2/i2c support (derived from
> > xf86DDC.c and xf86i2.c)
> >
> > 	This is a configurable option (CONFIG_FB_I2C). 	For now, I only used
> > "default" timeout values which may not work for all devices.
> >
> > 	Most drivers need to implement only two functions --
> >
> > 		void xxxfb_i2c_getbits(struct fb_info *info, signed long *clock, signed
> > long *data)
> > 		void xxxfb_i2c_putbits(struct fb_info *info, signed long clock, signed
> > long data)
>
> In fbi2c.c you are duplicating the i2c layer that's already there in the
> kernel. Each driver should  have its set of function to  control SDA and
> SCL and then it's only a matter of calling i2c_transfer.
>
> I wrote the i2c stuff for the new radeon driver, look here:
> bk://ppc.bkbits.net/linuxppc-2.5-benh
> (drivers/video/aty/radeon_i2c.c)

Ok.  See my reply to Ben H.

>
> > 2. [fbmon.c]: Consolidated different ways of getting EDID with the
> > following function:
> >
> > 	unsigned char *fb_get_edid(struct fb_info *info, struct pci_dev *pdev,
> > int what)
> >
> > 		the parameter "what" can be:
> > 			EDID_DDC2 - get EDID via DDC2/I2C fb_get_edid_from_ddc2()
> > 			EDID_ARCH - get	EDID via BIOS or OF get_EDID_from_{OF|BIOS}()
>
> Hum, I don't like this kind of "multiplexing". I think that EDID
> reading (if possibile) should be done inside the driver which knows how
> to do it.
>
> Also, why did you remove all the printk's? Ok, show_edid is a bit too
> verbose but it's usefull for debugging.

The first time I wrote the edid parser, show_edid() was never meant to have 
all that printk's :-).  It's only preliminary code in preparation for 
extending struct fb_monspecs.  But I'll bring it back, possibly when DEBUG is 
defined.

Tony


-------------------------------------------------------
This SF.net email is sponsored by: IBM Linux Tutorials.
Become an expert in LINUX or just sharpen your skills.  Sign up for IBM's
Free Linux Tutorials.  Learn everything from the bash shell to sys admin.
Click now! http://ads.osdn.com/?ad_id\x1278&alloc_id371&op=click

Re: [PATCH] DDC2/I2C Support

[Antonino A. Daplas]

On Tuesday 06 January 2004 05:59, Benjamin Herrenschmidt wrote:

>
> So you choose to get away from the linux i2c infrastructure ?

I already looked at your tree.  After some thought, I agree with both of you 
(Kronos) that i2c/ddc2 should be driver specific.  I'll remove the generic 
fbi2c interface.

>
> > 2. [fbmon.c]: Consolidated different ways of getting EDID with the
> > following function:
> >
> > 	unsigned char *fb_get_edid(struct fb_info *info, struct pci_dev *pdev,
> > int what)
> >
> > 		the parameter "what" can be:
> > 			EDID_DDC2 - get EDID via DDC2/I2C fb_get_edid_from_ddc2()
> >			EDID_ARCH - get	EDID via BIOS or OF get_EDID_from_{OF|BIOS}()
>
> The later is very card-specific... I don't think it can be abstracted.
> Also, if you look at the new radeonfb, you'll notice that the BIOS
> search has actually more than one way of getting to the EDID and
> may gather more than just the EDID (panel power delay, panel fixed
> dividers, etc...) Also, the OF parsing code cannot easily be abstracted
> as the OF driver for different type of cards do things differently.
>
> Hrm... reading your actual patch, I see that you are actually doing
> this BIOS call to get it, not parsing the BIOS image. Ok, that may
> be interesting...
>

There was the patch I submitted some 8 months ago and has been in the kernel 
since linux-2.5.x.  For X86, it will get the EDID block via VBE calls before 
the kernel goes to protected mode.  This is used by get_EDID_from_BIOS() in 
fbmon.c. Actual code is in boot/arch/i386/boot/video.S[store_edid]

Currently, it's always compiled in and some people reported problems with it 
(like taking 5 minutes to boot - probably BIOS using DDC1). I just added a 
kernel config for it so people can choose to compile it out.  However, with 
most drivers still not having DDC/I2C support, it's an easy way of getting 
the EDID block.

> Also, the former need a way to tell on which port we want the EDID,
> and the EDID code may need a bit more than juts that "get EDID" (look
> the other bit banging done to the i2c registers in radeonfb).
>
> I'd actually keep all that in the driver...
>
> > 	2.a getting EDID from the BIOS is now configurable (CONFIG_EDID_BIOS)
> > since some people reported problems during booting or it takes a long
> > time for the process to finish.
>
> This is about using the VESA BIOS at boot or scanning the ROM image ?

This is about getting the EDID using VBE calls while in real mode. 

>
> > 4. [fbmon.c]: Extended struct fb_monspecs to include display information
> > specified in EDID.
> >
> > 	Use function
> > 		void fb_get_monspecs(unsigned char *edid, struct fb_monspecs *specs)
>
>  .../...
>
> Can we define a "parsed_EDID" structure or whatever we call it that
> contains the list of modes, the monspecs, and other informations
> obtained from the EDID all at once ? I also need the display model
> name and vendor for example (to deal with some "fixups"). it would
> be convenient to have one single parsing pass extracting all the
> informations to one structure...
>

Ok, this should not be hard.

> > --------------------------------------------------------
> >
> > 5. [fbcon.c]: Minor fixes on the cursor and scrolling.
>
> That pops up something else in my mind... Shouldn't we take the console
> sempahore when doing cursor ops in the workqueue ?

I believe so.  I'm not sure where to start though. info->fb_cursor is called 
by at least two functons (fbcon_cursor, fb_flash_cursor), and I don't see any 
form of locking, especially for the data in info->cursor which is constantly 
being modified by one and read by another.

>
> In my tree, I have moved the VT blanking code out of the timer to a
> workqueue as well and fixed other VT stuffs to make sure we always
> have the console semaphore when getting to fbdev's. The current
> "lock-less" state of the whole thing is scary...
>

Yes :-(.

> > 6. [RIVAFB]:
> >
> > 	a. Implemented I2C/DDC2 support.  Changing video modes should be more
> > robust even when using stty.
>
> I strongly suggest that you look at what I've done for radeonfb and stty
> in my tree with FB_ACTIVATE_FIND and my mode search algorithm....

Ok. 

>
> > 	b. fixed various hardware cursor corruption
> > 	c. fixed maximum yres_virtual.  Hardware accel engine will crash if blit
> > rectangle dimensions/origin exceeds 0x7fff.
> > 	d. added new boot parameter, "strictmode", to force driver to use only
> > modes specified in the EDID block.
>
> Boot parameters are evil... we should default to the "right" way whenever
> possible. Let's make sure we properly deal with either complete modes

Evil, yes, but for now, necessary. Until the stty fix (and the 
FB_ACTIVATE_FIND) is accepted by James, I will need this new boot parameter. 
 
I'll submit a new patch (without the fbi2c code) by next week.  Still too busy 
to work full time with fbdev.

Tony


-------------------------------------------------------
This SF.net email is sponsored by: IBM Linux Tutorials.
Become an expert in LINUX or just sharpen your skills.  Sign up for IBM's
Free Linux Tutorials.  Learn everything from the bash shell to sys admin.
Click now! http://ads.osdn.com/?ad_id\x1278&alloc_id371&op=click

Re: [PATCH] DDC2/I2C Support

[Benjamin Herrenschmidt]

> There was the patch I submitted some 8 months ago and has been in the kernel 
> since linux-2.5.x.  For X86, it will get the EDID block via VBE calls before 
> the kernel goes to protected mode.  This is used by get_EDID_from_BIOS() in 
> fbmon.c. Actual code is in boot/arch/i386/boot/video.S[store_edid]
> 
> Currently, it's always compiled in and some people reported problems with it 
> (like taking 5 minutes to boot - probably BIOS using DDC1). I just added a 
> kernel config for it so people can choose to compile it out.  However, with 
> most drivers still not having DDC/I2C support, it's an easy way of getting 
> the EDID block.

Ok, if we go that way, we also need to store somewhere the PCI ID of
the primary display for which the EDID was "captured" or we'll do
weird things with several cards. I need to know which card is the
primary one anyway in radeonfb for other reasons (I need to fallback
to the RAM image of the BIOS with some laptops for the on-board
display)...

> > Can we define a "parsed_EDID" structure or whatever we call it that
> > contains the list of modes, the monspecs, and other informations
> > obtained from the EDID all at once ? I also need the display model
> > name and vendor for example (to deal with some "fixups"). it would
> > be convenient to have one single parsing pass extracting all the
> > informations to one structure...
> >
> 
> Ok, this should not be hard.

Good ;)

> > > --------------------------------------------------------
> > >
> > > 5. [fbcon.c]: Minor fixes on the cursor and scrolling.
> >
> > That pops up something else in my mind... Shouldn't we take the console
> > sempahore when doing cursor ops in the workqueue ?
> 
> I believe so.  I'm not sure where to start though. info->fb_cursor is called 
> by at least two functons (fbcon_cursor, fb_flash_cursor), and I don't see any 
> form of locking, especially for the data in info->cursor which is constantly 
> being modified by one and read by another.

In the work queue code itself. I'll do a patch for that after testing
here. (See my other race fixes).

> > I strongly suggest that you look at what I've done for radeonfb and stty
> > in my tree with FB_ACTIVATE_FIND and my mode search algorithm....
> 
> Ok. 
> > Boot parameters are evil... we should default to the "right" way whenever
> > possible. Let's make sure we properly deal with either complete modes
> 
> Evil, yes, but for now, necessary. Until the stty fix (and the 
> FB_ACTIVATE_FIND) is accepted by James, I will need this new boot parameter. 
>  
> I'll submit a new patch (without the fbi2c code) by next week.  Still too busy 
> to work full time with fbdev.

No problem, we can always extract the good stuff from your current
one anyway :)

Ben.




-------------------------------------------------------
This SF.net email is sponsored by: IBM Linux Tutorials.
Become an expert in LINUX or just sharpen your skills.  Sign up for IBM's
Free Linux Tutorials.  Learn everything from the bash shell to sys admin.
Click now! http://ads.osdn.com/?ad_id=1278&alloc_id=3371&op=click

Re: [PATCH] DDC2/I2C Support

[Benjamin Herrenschmidt]

(RESENT because of stupid problem with my mailer, it wouldn't reach
the list)

On Tue, 2004-01-06 at 01:31, adaplas@pol.net wrote:
> Hi all,
> 
> I had the Christmas to work a little bit more on EDID/DDC/I2C for fb
> drivers. I know they can be implemented in userspace, however, we do need
> some sane way of setting video modes since the framebuffer system is
> initialized quite early in the boot process.  Still, setting modes using
> stty is still a bit problematic.  We need a way to tell fbdev whether to
> accept or ignore the timings in fb_var_screeninfo.  These cases are
> manifested when changing resolution using stty or using fbset.

Did you look at what I did in my new raeondb ? I basically fixed all
these problems (I also reworked the way the console mode is changed
on fbset).

> Anyway, attached is a patch (gzipped) against linux-2.6.1-rc1. Sorry for
> lumping everything into one big patch since I don't have time to separate
> them into different patches.
> 
> 1. [fbi2c.c]:  Added preliminary generic DDC2/i2c support (derived from
> xf86DDC.c and xf86i2.c)
> 
> 	This is a configurable option (CONFIG_FB_I2C). 	For now, I only used
> "default" timeout values which may not work for all devices.
> 
> 	Most drivers need to implement only two functions --
> 
> 		void xxxfb_i2c_getbits(struct fb_info *info, signed long *clock, signed
> long *data)
> 		void xxxfb_i2c_putbits(struct fb_info *info, signed long clock, signed
> long data)
> 
> 	The functionality is exposed in the following:
> 
> 		unsigned char *fb_get_edid_from_ddc2(struct fb_info *info)

So you choose to get away from the linux i2c infrastructure ?

> 2. [fbmon.c]: Consolidated different ways of getting EDID with the
> following function:
> 
> 	unsigned char *fb_get_edid(struct fb_info *info, struct pci_dev *pdev,
> int what)
> 
> 		the parameter "what" can be:
> 			EDID_DDC2 - get EDID via DDC2/I2C fb_get_edid_from_ddc2()
>			EDID_ARCH - get	EDID via BIOS or OF get_EDID_from_{OF|BIOS}()

The later is very card-specific... I don't think it can be abstracted.
Also, if you look at the new radeonfb, you'll notice that the BIOS
search has actually more than one way of getting to the EDID and
may gather more than just the EDID (panel power delay, panel fixed
dividers, etc...) Also, the OF parsing code cannot easily be abstracted
as the OF driver for different type of cards do things differently.

Hrm... reading your actual patch, I see that you are actually doing
this BIOS call to get it, not parsing the BIOS image. Ok, that may
be interesting...

Also, the former need a way to tell on which port we want the EDID,
and the EDID code may need a bit more than juts that "get EDID" (look
the other bit banging done to the i2c registers in radeonfb).

I'd actually keep all that in the driver...

> 	2.a getting EDID from the BIOS is now configurable (CONFIG_EDID_BIOS)
> since some people reported problems during booting or it takes a long
> time for the process to finish.

This is about using the VESA BIOS at boot or scanning the ROM image ?

> 3. [modedb.c]: Added additional field (edid_src) to struct fb_videomode.
> 
> 	The idea came from Ben H. He suggested that the mode database built by
> fb_create_modedb() does not specify where each came from.  So
> fb_videomode.edid_src can be:

Ok. I'd rather call that more generically "flags" though...

> 	MODE_IS_UNKNOWN - defined as (0) -- all entries of modedb[] in fbmon.c
> 	MODE_IS_DETAILED -  mode came from detailed timings block of EDID.  They
> are the most preferred mode since all timings are specified in the block.
> 	MODE_IS_STANDARD - mode came from standard timings block usually in the
> form of [xres]x[yres]@refresh
> 	MODE_IS_VESA - mode is VESA standard
> 	MODE_IS_CALCULATED - mode is calculated using GTF (xres, yres, refresh).
> 	MODE_IS_FIRST - if set, mode is the first entry in the detailed timing
> block and is the preferred mode of the display.
> 
> 	Flags can be combined in different ways such as:
> 
> 	MODE_IS_STANDARD | MODE_IS_CALCULATED - ie, mode came from standard
> timing block, but mode was not found in VESA database, and instead
> calculated using GTF.

Sounds good.

> 4. [fbmon.c]: Extended struct fb_monspecs to include display information
> specified in EDID.
> 
> 	Use function
> 		void fb_get_monspecs(unsigned char *edid, struct fb_monspecs *specs)

 .../...

Can we define a "parsed_EDID" structure or whatever we call it that
contains the list of modes, the monspecs, and other informations
obtained from the EDID all at once ? I also need the display model
name and vendor for example (to deal with some "fixups"). it would
be convenient to have one single parsing pass extracting all the
informations to one structure...

> --------------------------------------------------------
> 
> 5. [fbcon.c]: Minor fixes on the cursor and scrolling.

That pops up something else in my mind... Shouldn't we take the console
sempahore when doing cursor ops in the workqueue ?

In my tree, I have moved the VT blanking code out of the timer to a
workqueue as well and fixed other VT stuffs to make sure we always
have the console semaphore when getting to fbdev's. The current
"lock-less" state of the whole thing is scary...

> 6. [RIVAFB]:
> 
> 	a. Implemented I2C/DDC2 support.  Changing video modes should be more
> robust even when using stty.

I strongly suggest that you look at what I've done for radeonfb and stty
in my tree with FB_ACTIVATE_FIND and my mode search algorithm....

> 	b. fixed various hardware cursor corruption
> 	c. fixed maximum yres_virtual.  Hardware accel engine will crash if blit
> rectangle dimensions/origin exceeds 0x7fff.
> 	d. added new boot parameter, "strictmode", to force driver to use only
> modes specified in the EDID block.

Boot parameters are evil... we should default to the "right" way whenever
possible. Let's make sure we properly deal with either complete modes
passed from userland, or just xres/yres/bpp passed by fbcon on console
resize (that is not only stty but console switch). Again, I got that
working properly with radeonfb (both stty and fbset resizing the console)
in my tree (that required some fbcon and VT changes), I really suggest you
look at this.

Ben.
-- 
Benjamin Herrenschmidt <benh@kernel.crashing.org>



-------------------------------------------------------
This SF.net email is sponsored by: IBM Linux Tutorials.
Become an expert in LINUX or just sharpen your skills.  Sign up for IBM's
Free Linux Tutorials.  Learn everything from the bash shell to sys admin.
Click now! http://ads.osdn.com/?ad_id=1278&alloc_id=3371&op=click