| Message ID | 1308705639-4215-1-git-send-email-reimth@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Dear Alex, > This is going to enable your quirk on every board with an HDMI port. > If we need to add a quirk for your board, let's make it board > specific. It might be better to just disable edid polling for certain > connectors on certain boards if they cause problems when no monitor is > detected. > > Alex Initially, I made the quirk only for the Asus board, because I only have this one to test. But the quirk applies of course to all boards that use the RS690 type chipset. I went through a couple of the many and various bugs that reported the EDID error/dump flooding problem. Beside of RS690, also at least RS630, RV770 and R350 are affected by this problem. In addition, I also found a bug report on Intel 945 GM that seems to have the same root cause. The majority of the bugs (ca. 80%) report a problem with the HDMI-A connector. 15% have a problem with the DVI-I connector and 5% have a problem with the LVDS connector. The proposed patch currently concentrates on the HDMI-A connector, only. Based on Jean's proposal I updated the patch, so that radeon_ddc_edid_probe() now replaces radeon_ddc_probe() for all HDMI-A connectors. According to the VESA standards we could also do this for all connector types in general. DDC is i2c plus EDID. Without the patch radeon_ddc_probe() probes DDC by checking, if data at all can be retrieved from i2c port 0x50 (where the EDID is provided). New radeon_ddc_edid_probe() just checks in addition, if the EDID header and version/revision information is available and according to the VESA standards. Then and only then (for the HDMI-A connectors) we let drm_get_edid() and drm_edid_block_valid() check the EDID checksum and probe the validity of the EDID. According to VESA's implementation guide this is the right approach: Checking the exact match of the EDID header and that the revision number is in valid range is mandatory. Based on Jean's comments, I believe that it makes sense to have EDID polling enabled as there might be temporary communication problems between graphics card and monitor. These should be logged. This also applies for the buggy HDMI-A connectors, as soon as a monitor is connected to them. Hope this helps you. Best regards Thomas
On Wed, Jun 22, 2011 at 11:17 AM, Thomas Reim <thomas.reim@nepomuc.de> wrote: > Dear Alex, > >> This is going to enable your quirk on every board with an HDMI port. >> If we need to add a quirk for your board, let's make it board >> specific. It might be better to just disable edid polling for certain >> connectors on certain boards if they cause problems when no monitor is >> detected. >> >> Alex > > Initially, I made the quirk only for the Asus board, because I only have > this one to test. But the quirk applies of course to all boards that use > the RS690 type chipset. > > I went through a couple of the many and various bugs that reported the > EDID error/dump flooding problem. Beside of RS690, also at least RS630, > RV770 and R350 are affected by this problem. In addition, I also found a > bug report on Intel 945 GM that seems to have the same root cause. > > The majority of the bugs (ca. 80%) report a problem with the HDMI-A > connector. 15% have a problem with the DVI-I connector and 5% have a > problem with the LVDS connector. Got links? I'm not sure this is the same root cause. A lot of hdmi monitors have broken edid checksums and other edid problems due to hdmi switches and receivers messing with the edid or just plain broken edids (tv's are particularly bad). These problems are reported the same way via log spam, but are not the same issue. In the particular case of your board (and probably a few other similar rs690/rs740 boards), the oem provides an optional hdmi add-in card that uses pcie lanes for display. The oem usually has the same connector table regardless of whether the add-in card is installed or not so we have no way of knowing whether there is actually a connector there or not. Even when there is a add-in board present it sounds like perhaps the i2c lines are not terminated properly when no monitor is attached. > > The proposed patch currently concentrates on the HDMI-A connector, only. > > Based on Jean's proposal I updated the patch, so that > radeon_ddc_edid_probe() now replaces radeon_ddc_probe() for all HDMI-A > connectors. > > According to the VESA standards we could also do this for all connector > types in general. DDC is i2c plus EDID. Without the patch > radeon_ddc_probe() probes DDC by checking, if data at all can be > retrieved from i2c port 0x50 (where the EDID is provided). New > radeon_ddc_edid_probe() just checks in addition, if the EDID header and > version/revision information is available and according to the VESA > standards. > > Then and only then (for the HDMI-A connectors) we let drm_get_edid() and > drm_edid_block_valid() check the EDID checksum and probe the validity of > the EDID. > > According to VESA's implementation guide this is the right approach: > Checking the exact match of the EDID header and that the revision number > is in valid range is mandatory. > radeon_ddc_probe() is supposed to be lightweight. It's used in the detect() functions just to probe if something is attached or not. There is a separate function to fetch the full edid and check if it's valid. For connectors without HPD (hot plug detect), the polling code probes the edid every 10 seconds. > Based on Jean's comments, I believe that it makes sense to have EDID > polling enabled as there might be temporary communication problems > between graphics card and monitor. These should be logged. This also > applies for the buggy HDMI-A connectors, as soon as a monitor is > connected to them. > edid polling was only added to deal with analog connectors that do not support hotplug interrupts and digital connectors that don't have HPD. You can still manually probe the monitor, you just won't get an event like you would with HPD or edid polling. Alex > Hope this helps you. > > Best regards > > Thomas > > > >
On Wed, Jun 22, 2011 at 11:17 AM, Thomas Reim <thomas.reim@nepomuc.de> wrote: > Dear Alex, > >> This is going to enable your quirk on every board with an HDMI port. >> If we need to add a quirk for your board, let's make it board >> specific. It might be better to just disable edid polling for certain >> connectors on certain boards if they cause problems when no monitor is >> detected. >> >> Alex > > Initially, I made the quirk only for the Asus board, because I only have > this one to test. But the quirk applies of course to all boards that use > the RS690 type chipset. > > I went through a couple of the many and various bugs that reported the > EDID error/dump flooding problem. Beside of RS690, also at least RS630, > RV770 and R350 are affected by this problem. In addition, I also found a > bug report on Intel 945 GM that seems to have the same root cause. > > The majority of the bugs (ca. 80%) report a problem with the HDMI-A > connector. 15% have a problem with the DVI-I connector and 5% have a > problem with the LVDS connector. > > The proposed patch currently concentrates on the HDMI-A connector, only. > I think it would be better to add a quirk flag and only do the extended probe on specific boards where it is required. E.g., if (radeon_connector->requires_extended_probe) radeon_ddc_probe_extended(); else radeon_ddc_probe(); Alex > Based on Jean's proposal I updated the patch, so that > radeon_ddc_edid_probe() now replaces radeon_ddc_probe() for all HDMI-A > connectors. > > According to the VESA standards we could also do this for all connector > types in general. DDC is i2c plus EDID. Without the patch > radeon_ddc_probe() probes DDC by checking, if data at all can be > retrieved from i2c port 0x50 (where the EDID is provided). New > radeon_ddc_edid_probe() just checks in addition, if the EDID header and > version/revision information is available and according to the VESA > standards. > > Then and only then (for the HDMI-A connectors) we let drm_get_edid() and > drm_edid_block_valid() check the EDID checksum and probe the validity of > the EDID. > > According to VESA's implementation guide this is the right approach: > Checking the exact match of the EDID header and that the revision number > is in valid range is mandatory. > > Based on Jean's comments, I believe that it makes sense to have EDID > polling enabled as there might be temporary communication problems > between graphics card and monitor. These should be logged. This also > applies for the buggy HDMI-A connectors, as soon as a monitor is > connected to them. > > Hope this helps you. > > Best regards > > Thomas > > > >
Dear Alex, > I think it would be better to add a quirk flag and only do the > extended probe on specific boards where it is required. E.g., > > if (radeon_connector->requires_extended_probe) > radeon_ddc_probe_extended(); > else > radeon_ddc_probe(); > > Alex Today, I updated the patch according to your proposal and the proposals of Jean. I'll send it with my next mail. Best regards Thomas
diff --git a/drivers/gpu/drm/radeon/radeon_connectors.c b/drivers/gpu/drm/radeon/radeon_connectors.c index cbfca3a..497e32a 100644 --- a/drivers/gpu/drm/radeon/radeon_connectors.c +++ b/drivers/gpu/drm/radeon/radeon_connectors.c @@ -825,9 +825,24 @@ radeon_dvi_detect(struct drm_connector *connector, bool force) int i; enum drm_connector_status ret = connector_status_disconnected; bool dret = false; - - if (radeon_connector->ddc_bus) - dret = radeon_ddc_probe(radeon_connector); + + if (radeon_connector->ddc_bus) { + /* AMD 690 chipset series HDMI DDC quirk: + * Some integrated ATI Radeon chipset implementations (e. g. + * Asus M2A-VM HDMI) indicate the availability of a DDC even + * when there's no monitor connected to HDMI. For HDMI + * connectors we check for the availability of EDID with + * at least a correct EDID header and EDID version/revision + * information. Only then, DDC is assumed to be available. + * This prevents drm_get_edid() and drm_edid_block_valid() of + * periodically dumping data and kernel errors into the logs + * and onto the terminal, which would lead to an unacceptable + * system behaviour */ + if (connector->connector_type == DRM_MODE_CONNECTOR_HDMIA) + dret = radeon_ddc_edid_probe(radeon_connector); + else + dret = radeon_ddc_probe(radeon_connector); + } if (dret) { if (radeon_connector->edid) { kfree(radeon_connector->edid); diff --git a/drivers/gpu/drm/radeon/radeon_display.c b/drivers/gpu/drm/radeon/radeon_display.c index 292f73f..550f143 100644 --- a/drivers/gpu/drm/radeon/radeon_display.c +++ b/drivers/gpu/drm/radeon/radeon_display.c @@ -715,6 +715,9 @@ static bool radeon_setup_enc_conn(struct drm_device *dev) if (ret) { radeon_setup_encoder_clones(dev); radeon_print_display_setup(dev); + /* Is this really required here? + Seems to just force drm to dump EDID errors + to kernel logs */ list_for_each_entry(drm_connector, &dev->mode_config.connector_list, head) radeon_ddc_dump(drm_connector); } @@ -777,8 +780,16 @@ static int radeon_ddc_dump(struct drm_connector *connector) if (!radeon_connector->ddc_bus) return -1; edid = drm_get_edid(connector, &radeon_connector->ddc_bus->adapter); + /* Asus M2A-VM HDMI DDC quirk: Log EDID retrieval status here once, + * instead of periodically dumping data and kernel errors into the + * logs, if a monitor is not connected to HDMI */ if (edid) { + DRM_INFO("Radeon display connector %s: Found valid EDID", + drm_get_connector_name(connector)); kfree(edid); + } else { + DRM_INFO("Radeon display connector %s: No display connected or invalid EDID", + drm_get_connector_name(connector)); } return ret; } diff --git a/drivers/gpu/drm/radeon/radeon_i2c.c b/drivers/gpu/drm/radeon/radeon_i2c.c index 781196d..80b871f 100644 --- a/drivers/gpu/drm/radeon/radeon_i2c.c +++ b/drivers/gpu/drm/radeon/radeon_i2c.c @@ -63,6 +63,61 @@ bool radeon_ddc_probe(struct radeon_connector *radeon_connector) return false; } +/* + * Probe EDID information via I2C: + * Try to fetch EDID information by calling i2c_transfer driver function and + * probe for valid EDID header and version information. + * + * AMD 690 chipset series HDMI DDC quirk: + * Some integrated ATI Radeon chipset implementations (e. g. Asus M2A-VM HDMI + * indicate the availability of a DDC even when there's no monitor connected. + * Invalid EDID leads to flooding of logs and terminals with error messages + */ +bool radeon_ddc_edid_probe(struct radeon_connector *radeon_connector) +{ + u8 out_buf[] = {0x0, 0x0}; + u8 block[20]; + int ret; + struct i2c_msg msgs[] = { + { + .addr = 0x50, + .flags = 0, + .len = 1, + .buf = out_buf, + }, { + .addr = 0x50, + .flags = I2C_M_RD, + .len = 20, + .buf = block, + } + }; + + /* on hw with routers, select right port */ + if (radeon_connector->router.ddc_valid) + radeon_router_select_ddc_port(radeon_connector); + + ret = i2c_transfer(&radeon_connector->ddc_bus->adapter, msgs, 2); + if (ret == 2) + if (block[0] == 0x00 && + block[7] == 0x00 && + block[1] == 0xff && + block[2] == 0xff && + block[3] == 0xff && + block[4] == 0xff && + block[5] == 0xff && + block[6] == 0xff) + /* EDID header starts with: + * 0x00,0xFF,0xFF,0xFF,0xFF,0xFF,0xFF,0x00 + */ + if (block[18] != 0x00 || block[19] != 0x00) + /* EDID header ends with EDID version and + * revision number <> 0.0 + */ + return true; + /* Couldn't find an accessible EDID on this connector */ + return false; +} + /* bit banging i2c */ static void radeon_i2c_do_lock(struct radeon_i2c_chan *i2c, int lock_state) diff --git a/drivers/gpu/drm/radeon/radeon_mode.h b/drivers/gpu/drm/radeon/radeon_mode.h index 6df4e3c..14710fc 100644 --- a/drivers/gpu/drm/radeon/radeon_mode.h +++ b/drivers/gpu/drm/radeon/radeon_mode.h @@ -515,6 +515,7 @@ extern void radeon_i2c_put_byte(struct radeon_i2c_chan *i2c, extern void radeon_router_select_ddc_port(struct radeon_connector *radeon_connector); extern void radeon_router_select_cd_port(struct radeon_connector *radeon_connector); extern bool radeon_ddc_probe(struct radeon_connector *radeon_connector); +extern bool radeon_ddc_edid_probe(struct radeon_connector *radeon_connector); extern int radeon_ddc_get_modes(struct radeon_connector *radeon_connector); extern struct drm_encoder *radeon_best_encoder(struct drm_connector *connector);