[PATCH v5+] viafb: I2C/DDC LCD detection for VIA framebuffer

[PATCH v5+] viafb: I2C/DDC LCD detection for VIA framebuffer

[Dzianis Kahanovich]

I2C/DDC LCD detection for VIA framebuffer

Adding legacy I2C/DDC support for VIA framebuffer, used to fix LCD
panel size and (if "via_active_dev" is empty default) force LCD
detection. This solving at least defaults on Chrome9 video
on HP mini 2133 notebook, but must be good in other cases,
include double-LCD. Also related bugfixes.

v3 patch: original viafb_find_i2c_adapt() code is unsafe (nowere used).

v4 patch: viafbinfo1 is not initilalized if !viafb_dual_fb
and may cause problems with dual separated LCD. Now dual & SAMM
code looks safe (will no generate errors).

v5 patch: handle undefined resolution, overhead, cleanup.

Signed-off-by: Dzianis Kahanovich <mahatma@eu.by>
diff -pruN a/drivers/video/via/via-core.c b/drivers/video/via/via-core.c
--- a/drivers/video/via/via-core.c	2010-12-03 13:44:48.000000000 +0200
+++ b/drivers/video/via/via-core.c	2010-12-03 13:16:14.000000000 +0200
@@ -2,6 +2,7 @@
  * Copyright 1998-2009 VIA Technologies, Inc. All Rights Reserved.
  * Copyright 2001-2008 S3 Graphics, Inc. All Rights Reserved.
  * Copyright 2009 Jonathan Corbet <corbet@lwn.net>
+ * Copyright 2010 Dzianis Kahanovich <mahatma@eu.by>
  */

 /*
@@ -559,6 +560,69 @@ static void via_teardown_subdevs(void)
 		}
 }

+static void via_i2c_detect(int stage)
+{
+	u8 *edid;
+	struct fb_var_screeninfo var;
+	int i, n = 0;
+	struct i2c_adapter *adapter;
+	struct lvds_setting_information *inf;
+
+	for (i = 0; i < VIAFB_NUM_PORTS; i++) {
+		adapter = viafb_find_i2c_adapter(i);
+		if (!adapter || !adapter->algo_data ||
+		    !(edid = fb_ddc_read(adapter)))
+			continue;
+		memset(&var, 0, sizeof(var));
+		if (fb_parse_edid(edid, &var))
+			goto free_edid;
+		if (!stage) {
+			if (n && !viafb_active_dev)
+				viafb_dual_fb = STATE_ON;
+			inf = NULL;
+		} else if (!n || !viafb_LCD_ON) {
+			fb_edid_to_monspecs(edid, &viafbinfo->monspecs);
+			inf = viaparinfo->lvds_setting_info;
+			if (!viafb_active_dev) {
+				viafb_DVI_ON +				    viaparinfo->tmds_setting_info->max_hres ?
+				    STATE_ON : STATE_OFF;
+				if (viafbinfo->monspecs.input & FB_DISP_DDI) {
+					viafb_LCD_ON = STATE_ON;
+					viafb_DeviceStatus = LCD_Device;
+					if (!n)
+						viafb_primary_dev = LCD_Device;
+				}
+			}
+		} else if (viafb_dual_fb) {
+			fb_edid_to_monspecs(edid, &viafbinfo1->monspecs);
+			inf = viaparinfo1->lvds_setting_info;
+			if (!viafb_active_dev &&
+			    (viafbinfo1->monspecs.input & FB_DISP_DDI))
+				viafb_LCD2_ON = STATE_ON;
+		} else {
+			fb_edid_to_monspecs(edid, &viafbinfo->monspecs);
+			inf = viaparinfo->lvds_setting_info2;
+			if (!viafb_active_dev &&
+			    (viafbinfo->monspecs.input & FB_DISP_DDI))
+				viafb_LCD2_ON = STATE_ON;
+		}
+		if (inf) {
+			if (var.xres)
+			    inf->lcd_panel_hres = var.xres;
+			if (var.yres)
+			    inf->lcd_panel_vres = var.yres;
+		}
+		n++;
+free_edid:
+		kfree(edid);
+		if (n > 1)
+			break;
+	}
+	if (stage && n && !viafb_active_dev)
+		viafb_set_iga_path();
+}
+

 static int __devinit via_pci_probe(struct pci_dev *pdev,
 		const struct pci_device_id *ent)
@@ -585,12 +649,16 @@ static int __devinit via_pci_probe(struc
 	 */
 	viafb_int_init();
 	via_setup_subdevs(&global_dev);
+	/* dual? */
+	via_i2c_detect(0);
 	/*
 	 * Set up the framebuffer device
 	 */
 	ret = via_fb_pci_probe(&global_dev);
 	if (ret)
 		goto out_subdevs;
+	/* LCD size & present */
+	via_i2c_detect(1);
 	return 0;

 out_subdevs:
diff -pruN a/drivers/video/via/viafbdev.h b/drivers/video/via/viafbdev.h
--- a/drivers/video/via/viafbdev.h	2010-12-03 13:44:48.000000000 +0200
+++ b/drivers/video/via/viafbdev.h	2010-12-03 13:13:50.000000000 +0200
@@ -88,6 +88,7 @@ extern int viafb_LCD2_ON;
 extern int viafb_LCD_ON;
 extern int viafb_DVI_ON;
 extern int viafb_hotplug;
+extern char *viafb_active_dev;

 extern int strict_strtoul(const char *cp, unsigned int base,
 	unsigned long *res);
diff -pruN a/drivers/video/via/via_i2c.c b/drivers/video/via/via_i2c.c
--- a/drivers/video/via/via_i2c.c	2010-12-03 13:44:48.000000000 +0200
+++ b/drivers/video/via/via_i2c.c	2010-12-03 13:10:26.000000000 +0200
@@ -167,7 +167,7 @@ struct i2c_adapter *viafb_find_i2c_adapt
 {
 	struct via_i2c_stuff *stuff = &via_i2c_par[which];

-	return &stuff->adapter;
+	return stuff->is_active ? &stuff->adapter : NULL;
 }
 EXPORT_SYMBOL_GPL(viafb_find_i2c_adapter);

--

no patch without bug. (c) & PS:
PS Todo: find where initilized mode (while no) and try to [un]detect CRT too
(stage 3?). CRT plugging unaffected in module init stage on my chip, but CRT
must have own DDC.

Re: [PATCH v5+] viafb: I2C/DDC LCD detection for VIA framebuffer

[Florian Tobias Schandinat]

Hi,

thanks a lot for working on the EDID code.
[and sorry for not responding sooner, but I'm busy at the moment]

Dzianis Kahanovich schrieb:
> I2C/DDC LCD detection for VIA framebuffer
> 
> Adding legacy I2C/DDC support for VIA framebuffer, used to fix LCD
> panel size and (if "via_active_dev" is empty default) force LCD
> detection. This solving at least defaults on Chrome9 video
> on HP mini 2133 notebook, but must be good in other cases,
> include double-LCD. Also related bugfixes.
> 
> v3 patch: original viafb_find_i2c_adapt() code is unsafe (nowere used).
> 
> v4 patch: viafbinfo1 is not initilalized if !viafb_dual_fb
> and may cause problems with dual separated LCD. Now dual & SAMM
> code looks safe (will no generate errors).
> 
> v5 patch: handle undefined resolution, overhead, cleanup.
> 
> Signed-off-by: Dzianis Kahanovich <mahatma@eu.by>

The description of changes (from above) in your patch version should go here 
like this (as it should not show up in "git log" later on):
---
v3 patch: original viafb_find_i2c_adapt() code is unsafe (nowere used).

v4 patch: viafbinfo1 is not initilalized if !viafb_dual_fb and may cause 
problems with dual separated LCD. Now dual & SAMM code looks safe (will no 
generate errors).

v5 patch: handle undefined resolution, overhead, cleanup.

> diff -pruN a/drivers/video/via/via-core.c b/drivers/video/via/via-core.c
> --- a/drivers/video/via/via-core.c	2010-12-03 13:44:48.000000000 +0200
> +++ b/drivers/video/via/via-core.c	2010-12-03 13:16:14.000000000 +0200

As Jon already mentioned, via-core.c is the wrong file for such things because 
via-core is meant for managing shared resources (between video capture and 
framebuffer at the moment) and EDID is only interesting for the framebuffer. 
Probably viafbdev.c would be the best choice.

> @@ -2,6 +2,7 @@
>   * Copyright 1998-2009 VIA Technologies, Inc. All Rights Reserved.
>   * Copyright 2001-2008 S3 Graphics, Inc. All Rights Reserved.
>   * Copyright 2009 Jonathan Corbet <corbet@lwn.net>
> + * Copyright 2010 Dzianis Kahanovich <mahatma@eu.by>
>   */
> 
>  /*
> @@ -559,6 +560,69 @@ static void via_teardown_subdevs(void)
>  		}
>  }
> 
> +static void via_i2c_detect(int stage)
> +{
> +	u8 *edid;
> +	struct fb_var_screeninfo var;
> +	int i, n = 0;
> +	struct i2c_adapter *adapter;
> +	struct lvds_setting_information *inf;
> +
> +	for (i = 0; i < VIAFB_NUM_PORTS; i++) {

I think you should do it a bit more target oriented. At least on newer IGPs it 
looks like the following ports are fixed
0x26 : CRT
0x31 : DVP1
which allows easier matching of the EDID data to the output device.

> +		adapter = viafb_find_i2c_adapter(i);
> +		if (!adapter || !adapter->algo_data ||
> +		    !(edid = fb_ddc_read(adapter)))
> +			continue;
> +		memset(&var, 0, sizeof(var));
> +		if (fb_parse_edid(edid, &var))
> +			goto free_edid;
> +		if (!stage) {
> +			if (n && !viafb_active_dev)
> +				viafb_dual_fb = STATE_ON;
> +			inf = NULL;
> +		} else if (!n || !viafb_LCD_ON) {
> +			fb_edid_to_monspecs(edid, &viafbinfo->monspecs);
> +			inf = viaparinfo->lvds_setting_info;
> +			if (!viafb_active_dev) {
> +				viafb_DVI_ON > +				    viaparinfo->tmds_setting_info->max_hres ?
> +				    STATE_ON : STATE_OFF;

Doing such thing here looks wrong as this loop does not influence 
viaparinfo->tmds_setting_info

> +				if (viafbinfo->monspecs.input & FB_DISP_DDI) {
> +					viafb_LCD_ON = STATE_ON;

It can happen that we enable LCD and DVI for the same device? Well nearly the 
only difference between LCD and DVI is backlight handling, so it should work, 
but it is ugly.

> +					viafb_DeviceStatus = LCD_Device;
> +					if (!n)
> +						viafb_primary_dev = LCD_Device;
> +				}
> +			}
> +		} else if (viafb_dual_fb) {
> +			fb_edid_to_monspecs(edid, &viafbinfo1->monspecs);
> +			inf = viaparinfo1->lvds_setting_info;
> +			if (!viafb_active_dev &&
> +			    (viafbinfo1->monspecs.input & FB_DISP_DDI))
> +				viafb_LCD2_ON = STATE_ON;
> +		} else {
> +			fb_edid_to_monspecs(edid, &viafbinfo->monspecs);
> +			inf = viaparinfo->lvds_setting_info2;
> +			if (!viafb_active_dev &&
> +			    (viafbinfo->monspecs.input & FB_DISP_DDI))
> +				viafb_LCD2_ON = STATE_ON;
> +		}
> +		if (inf) {
> +			if (var.xres)
> +			    inf->lcd_panel_hres = var.xres;
> +			if (var.yres)
> +			    inf->lcd_panel_vres = var.yres;
> +		}
> +		n++;
> +free_edid:
> +		kfree(edid);
> +		if (n > 1)
> +			break;
> +	}
> +	if (stage && n && !viafb_active_dev)
> +		viafb_set_iga_path();
> +}
> +
> 
>  static int __devinit via_pci_probe(struct pci_dev *pdev,
>  		const struct pci_device_id *ent)
> @@ -585,12 +649,16 @@ static int __devinit via_pci_probe(struc
>  	 */
>  	viafb_int_init();
>  	via_setup_subdevs(&global_dev);
> +	/* dual? */
> +	via_i2c_detect(0);
>  	/*
>  	 * Set up the framebuffer device
>  	 */
>  	ret = via_fb_pci_probe(&global_dev);
>  	if (ret)
>  		goto out_subdevs;
> +	/* LCD size & present */
> +	via_i2c_detect(1);
>  	return 0;
> 
>  out_subdevs:
> diff -pruN a/drivers/video/via/viafbdev.h b/drivers/video/via/viafbdev.h
> --- a/drivers/video/via/viafbdev.h	2010-12-03 13:44:48.000000000 +0200
> +++ b/drivers/video/via/viafbdev.h	2010-12-03 13:13:50.000000000 +0200
> @@ -88,6 +88,7 @@ extern int viafb_LCD2_ON;
>  extern int viafb_LCD_ON;
>  extern int viafb_DVI_ON;
>  extern int viafb_hotplug;
> +extern char *viafb_active_dev;
> 
>  extern int strict_strtoul(const char *cp, unsigned int base,
>  	unsigned long *res);
> diff -pruN a/drivers/video/via/via_i2c.c b/drivers/video/via/via_i2c.c
> --- a/drivers/video/via/via_i2c.c	2010-12-03 13:44:48.000000000 +0200
> +++ b/drivers/video/via/via_i2c.c	2010-12-03 13:10:26.000000000 +0200
> @@ -167,7 +167,7 @@ struct i2c_adapter *viafb_find_i2c_adapt
>  {
>  	struct via_i2c_stuff *stuff = &via_i2c_par[which];
> 
> -	return &stuff->adapter;
> +	return stuff->is_active ? &stuff->adapter : NULL;
>  }
>  EXPORT_SYMBOL_GPL(viafb_find_i2c_adapter);
> 
> --
> 
> no patch without bug. (c) & PS:
> PS Todo: find where initilized mode (while no) and try to [un]detect CRT too
> (stage 3?). CRT plugging unaffected in module init stage on my chip, but CRT
> must have own DDC.

CRT DDC is at 0x26.

I like your EDID parsing code (the fact that you are using existing functions). 
But as we already have a poor open-coded EDID parser and as it would be very 
ugly to have two different methods of EDID parsing in the same driver you really 
should investigate whether your parser can't fully replace the one in dvi.c

And you should care about where the EDID information comes from:
0x21 => CRT, do not set lvds or tmds information
0x31 => DVP1:
if lcd/dvi_port was not specified : lvds/tmds_chip_information output interface 
should be set to INTERFACE_DVP1 if you set the matching 
lvds/tmds_setting_information
if lcd/dvi_port was specified : set the lvds/tmds_setting_information only if 
the output interface is INTERFACE_DVP1


Thanks,

Florian Tobias Schandinat

Re: [PATCH v5+] viafb: I2C/DDC LCD detection for VIA framebuffer

[Dzianis Kahanovich]

Florian Tobias Schandinat wrote:

> As Jon already mentioned, via-core.c is the wrong file for such things because
> via-core is meant for managing shared resources (between video capture and
> framebuffer at the moment) and EDID is only interesting for the framebuffer.
> Probably viafbdev.c would be the best choice.

I think about moving code, but another sense was - this code mix (eclectic)
calls from various places and called from here, then...

But OK.

>> +            if (!viafb_active_dev) {
>> +                viafb_DVI_ON >> +                    viaparinfo->tmds_setting_info->max_hres ?
>> +                    STATE_ON : STATE_OFF;
> 
> Doing such thing here looks wrong as this loop does not influence
> viaparinfo->tmds_setting_info

I have only card, incompatible with current tmds code - Chrome9 (0x3371). But
while I send patch, I think "legacy" DDC code must at least be compatible with
current code and not brake it. Really I have no tmds identifyed and this place
only disabling DVI if so (default enabled).

>> +                if (viafbinfo->monspecs.input & FB_DISP_DDI) {
>> +                    viafb_LCD_ON = STATE_ON;
> 
> It can happen that we enable LCD and DVI for the same device? Well nearly the
> only difference between LCD and DVI is backlight handling, so it should work,
> but it is ugly.

Then I must not enable LCD if DVI/tmds detected?
if ((viafbinfo->monspecs.input & FB_DISP_DDI) && !viafb_DVI_ON) {

- right? If there are only change I may make - do it if you will use this code.

Or just on start of function:
if (viaparinfo->tmds_setting_info->max_hres)
	return;

What is better?

> I like your EDID parsing code (the fact that you are using existing functions).
> But as we already have a poor open-coded EDID parser and as it would be very
> ugly to have two different methods of EDID parsing in the same driver you really
> should investigate whether your parser can't fully replace the one in dvi.c

Current parser just don't work with my card. But this - at least found good
resolution and whole LCD. Default I have dead DVI and CRT. And LCD resolution
not detected. I must change type to "17" (1024x600), default - "2" (1024x768).
This code is just working for me.

> And you should care about where the EDID information comes from:
> 0x21 => CRT, do not set lvds or tmds information
> 0x31 => DVP1:
[...]

OK. I will play with this.


-- 
WBR, Dzianis Kahanovich AKA Denis Kaganovich, http://mahatma.bspu.unibel.by/

[PATCH v5] viafb: I2C/DDC LCD detection for VIA framebuffer

[Dzianis Kahanovich]

I2C/DDC LCD detection for VIA framebuffer

Adding legacy I2C/DDC support for VIA framebuffer, used to fix LCD
panel size and (if "via_active_dev" is empty default) force LCD
detection. This solving at least defaults on Chrome9 video
on HP mini 2133 notebook, but must be good in other cases,
include double-LCD. Also related bugfixes.

v3 patch: original viafb_find_i2c_adapt() code is unsafe (nowere used).

v4 patch: viafbinfo1 is not initilalized if !viafb_dual_fb
and may cause problems with dual separated LCD. Now dual & SAMM
code looks safe (will no generate errors).

v5 patch: handle undefined resolution, overhead, cleanup.

Signed-off-by: Dzianis Kahanovich <mahatma@eu.by>
---
diff -pruN a/drivers/video/via/via-core.c b/drivers/video/via/via-core.c
--- a/drivers/video/via/via-core.c	2010-12-03 13:06:06.000000000 +0200
+++ b/drivers/video/via/via-core.c	2010-12-03 13:16:14.000000000 +0200
@@ -559,6 +560,69 @@ static void via_teardown_subdevs(void)
 		}
 }

+static void via_i2c_detect(int stage)
+{
+	u8 *edid;
+	struct fb_var_screeninfo var;
+	int i, n = 0;
+	struct i2c_adapter *adapter;
+	struct lvds_setting_information *inf;
+
+	for (i = 0; i < VIAFB_NUM_PORTS; i++) {
+		adapter = viafb_find_i2c_adapter(i);
+		if (!adapter || !adapter->algo_data ||
+		    !(edid = fb_ddc_read(adapter)))
+			continue;
+		memset(&var, 0, sizeof(var));
+		if (fb_parse_edid(edid, &var))
+			goto free_edid;
+		if (!stage) {
+			if (n && !viafb_active_dev)
+				viafb_dual_fb = STATE_ON;
+			inf = NULL;
+		} else if (!n || !viafb_LCD_ON) {
+			fb_edid_to_monspecs(edid, &viafbinfo->monspecs);
+			inf = viaparinfo->lvds_setting_info;
+			if (!viafb_active_dev) {
+				viafb_DVI_ON +				    viaparinfo->tmds_setting_info->max_hres ?
+				    STATE_ON : STATE_OFF;
+				if (viafbinfo->monspecs.input & FB_DISP_DDI) {
+					viafb_LCD_ON = STATE_ON;
+					viafb_DeviceStatus = LCD_Device;
+					if (!n)
+						viafb_primary_dev = LCD_Device;
+				}
+			}
+		} else if (viafb_dual_fb) {
+			fb_edid_to_monspecs(edid, &viafbinfo1->monspecs);
+			inf = viaparinfo1->lvds_setting_info;
+			if (!viafb_active_dev &&
+			    (viafbinfo1->monspecs.input & FB_DISP_DDI))
+				viafb_LCD2_ON = STATE_ON;
+		} else {
+			fb_edid_to_monspecs(edid, &viafbinfo->monspecs);
+			inf = viaparinfo->lvds_setting_info2;
+			if (!viafb_active_dev &&
+			    (viafbinfo->monspecs.input & FB_DISP_DDI))
+				viafb_LCD2_ON = STATE_ON;
+		}
+		if (inf) {
+			if (var.xres)
+			    inf->lcd_panel_hres = var.xres;
+			if (var.yres)
+			    inf->lcd_panel_vres = var.yres;
+		}
+		n++;
+free_edid:
+		kfree(edid);
+		if (n > 1)
+			break;
+	}
+	if (stage && n && !viafb_active_dev)
+		viafb_set_iga_path();
+}
+

 static int __devinit via_pci_probe(struct pci_dev *pdev,
 		const struct pci_device_id *ent)
@@ -585,12 +649,16 @@ static int __devinit via_pci_probe(struc
 	 */
 	viafb_int_init();
 	via_setup_subdevs(&global_dev);
+	/* dual? */
+	via_i2c_detect(0);
 	/*
 	 * Set up the framebuffer device
 	 */
 	ret = via_fb_pci_probe(&global_dev);
 	if (ret)
 		goto out_subdevs;
+	/* LCD size & present */
+	via_i2c_detect(1);
 	return 0;

 out_subdevs:
diff -pruN a/drivers/video/via/viafbdev.h b/drivers/video/via/viafbdev.h
--- a/drivers/video/via/viafbdev.h	2010-08-02 01:11:14.000000000 +0300
+++ b/drivers/video/via/viafbdev.h	2010-12-03 13:13:50.000000000 +0200
@@ -88,6 +88,7 @@ extern int viafb_LCD2_ON;
 extern int viafb_LCD_ON;
 extern int viafb_DVI_ON;
 extern int viafb_hotplug;
+extern char *viafb_active_dev;

 extern int strict_strtoul(const char *cp, unsigned int base,
 	unsigned long *res);
diff -pruN a/drivers/video/via/via_i2c.c b/drivers/video/via/via_i2c.c
--- a/drivers/video/via/via_i2c.c	2010-12-03 13:06:06.000000000 +0200
+++ b/drivers/video/via/via_i2c.c	2010-12-03 13:10:26.000000000 +0200
@@ -167,7 +167,7 @@ struct i2c_adapter *viafb_find_i2c_adapt
 {
 	struct via_i2c_stuff *stuff = &via_i2c_par[which];

-	return &stuff->adapter;
+	return stuff->is_active ? &stuff->adapter : NULL;
 }
 EXPORT_SYMBOL_GPL(viafb_find_i2c_adapter);

--