| Message ID | 1308866743-8066-1-git-send-email-reimth@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, Jun 23, 2011 at 6:05 PM, <reimth@googlemail.com> wrote: > From: Thomas Reim <rdratlos@yahoo.co.uk> > > Some integrated ATI Radeon chipset implementations > (e. g. Asus M2A-VM HDMI, RS690) indicate the availability > of a DDC even when there's no monitor connected. > In this case, drm_get_edid() and drm_edid_block_valid() > periodically dump data and kernel errors into system > log files and onto terminals, which lead to an unacceptable > system behaviour. For this purpose function drm_edid_header_is_valid() > has been added to drm and function drm_edid_block_valid() > has been adapted to also use drm_edid_header_is_valid(). > > Tested since kernel 2.35 on Asus M2A-VM HDMI board See comments below. Alex > > Signed-off-by: Thomas Reim <rdratlos@yahoo.co.uk> > --- > drivers/gpu/drm/drm_edid.c | 23 +++++++++++--- > drivers/gpu/drm/radeon/radeon_connectors.c | 29 ++++++++++++++++- > drivers/gpu/drm/radeon/radeon_display.c | 12 +++++++ > drivers/gpu/drm/radeon/radeon_i2c.c | 45 ++++++++++++++++++++++++++- > drivers/gpu/drm/radeon/radeon_mode.h | 4 ++ > include/drm/drm_crtc.h | 1 + > 6 files changed, 105 insertions(+), 9 deletions(-) > > diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c > index 0929219..1daaf81 100644 > --- a/drivers/gpu/drm/drm_edid.c > +++ b/drivers/gpu/drm/drm_edid.c > @@ -128,6 +128,23 @@ static const u8 edid_header[] = { > }; > > /* > + * Sanity check the header of the base EDID block. Return 8 if the header > + * is perfect, down to 0 if it's totally wrong. > + */ > +int drm_edid_header_is_valid(const u8 *raw_edid) > +{ > + int i, score = 0; > + > + for (i = 0; i < sizeof(edid_header); i++) > + if (raw_edid[i] == edid_header[i]) > + score++; > + > + return score; > +} > +EXPORT_SYMBOL(drm_edid_header_is_valid); > + > + > +/* > * Sanity check the EDID block (base or extension). Return 0 if the block > * doesn't check out, or 1 if it's valid. > */ > @@ -139,11 +156,7 @@ drm_edid_block_valid(u8 *raw_edid) > struct edid *edid = (struct edid *)raw_edid; > > if (raw_edid[0] == 0x00) { > - int score = 0; > - > - for (i = 0; i < sizeof(edid_header); i++) > - if (raw_edid[i] == edid_header[i]) > - score++; > + int score = drm_edid_header_is_valid(raw_edid); > > if (score == 8) ; > else if (score >= 6) { > diff --git a/drivers/gpu/drm/radeon/radeon_connectors.c b/drivers/gpu/drm/radeon/radeon_connectors.c > index cbfca3a..bcd2380 100644 > --- a/drivers/gpu/drm/radeon/radeon_connectors.c > +++ b/drivers/gpu/drm/radeon/radeon_connectors.c > @@ -826,8 +826,12 @@ radeon_dvi_detect(struct drm_connector *connector, bool force) > 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) { > + if (radeon_connector->requires_extended_probe) > + dret = radeon_ddc_probe_extended(radeon_connector); > + else > + dret = radeon_ddc_probe(radeon_connector); > + } Thinking about it more, it might be cleaner to just make the extended mode a flag, e.g., dret = radeon_ddc_probe_extended(radeon_connector, radeon_connector->requires_extended_probe); and handle the extended fetch in the same probe function. > if (dret) { > if (radeon_connector->edid) { > kfree(radeon_connector->edid); > @@ -1342,6 +1346,7 @@ radeon_add_atom_connector(struct drm_device *dev, > struct radeon_encoder *radeon_encoder; > uint32_t subpixel_order = SubPixelNone; > bool shared_ddc = false; > + bool requires_extended_probe = false; > bool is_dp_bridge = false; > > if (connector_type == DRM_MODE_CONNECTOR_Unknown) > @@ -1400,6 +1405,7 @@ radeon_add_atom_connector(struct drm_device *dev, > radeon_connector->shared_ddc = shared_ddc; > radeon_connector->connector_object_id = connector_object_id; > radeon_connector->hpd = *hpd; > + radeon_connector->requires_extended_probe = requires_extended_probe; > radeon_connector->router = *router; > if (router->ddc_valid || router->cd_valid) { > radeon_connector->router_bus = radeon_i2c_lookup(rdev, &router->i2c_info); > @@ -1688,6 +1694,23 @@ radeon_add_atom_connector(struct drm_device *dev, > } > } > > + /* RS690 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_type == DRM_MODE_CONNECTOR_HDMIA && > + (rdev->family == CHIP_RS690 || > + rdev->family == CHIP_RS740 || > + rdev->family == CHIP_RV630)) This seems like an arbitrary selection of chips. I haven't heard of any problems related to ddc on rv630. Also I think we should limit it to the specific connector that is problematic rather than all hdmi ports. In the case of your board, it seems the hdmi port on the add-in card is the only problematic one. Lots of rs690 motherboards have hdmi ports on the motherboard itself that work fine. I'd prefer to match based on the pci device and subsytem ids and the supported_device and connector type. See radeon_atom_apply_quirks() in radeon_atombios.c for an example. Something like: radeon_connector->requires_extended_probe = radeon_connector_needs_extended_probe(rdev, supported_dev, connector_type); > + requires_extended_probe = true; > + radeon_connector->requires_extended_probe = requires_extended_probe; > if (radeon_connector->hpd.hpd == RADEON_HPD_NONE) { > if (i2c_bus->valid) > connector->polled = DRM_CONNECTOR_POLL_CONNECT; > @@ -1716,6 +1739,7 @@ radeon_add_legacy_connector(struct drm_device *dev, > struct drm_connector *connector; > struct radeon_connector *radeon_connector; > uint32_t subpixel_order = SubPixelNone; > + bool requires_extended_probe = false; > > if (connector_type == DRM_MODE_CONNECTOR_Unknown) > return; > @@ -1746,6 +1770,7 @@ radeon_add_legacy_connector(struct drm_device *dev, > radeon_connector->devices = supported_device; > radeon_connector->connector_object_id = connector_object_id; > radeon_connector->hpd = *hpd; > + radeon_connector->requires_extended_probe = requires_extended_probe; > switch (connector_type) { > case DRM_MODE_CONNECTOR_VGA: > drm_connector_init(dev, &radeon_connector->base, &radeon_vga_connector_funcs, connector_type); > diff --git a/drivers/gpu/drm/radeon/radeon_display.c b/drivers/gpu/drm/radeon/radeon_display.c > index 292f73f..27e6840 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); Both the code and the comment can probably be removed, but let's make that a separate patch. > } > @@ -777,8 +780,17 @@ 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); > + /* Log EDID retrieval status here. In particular with regard to > + * connectors with requires_extended_probe flag set, that will prevent > + * function radeon_dvi_detect() to fetch EDID on this connector, > + * as long as there is no valid EDID header found */ > 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 monitor connected or invalid EDID", > + drm_get_connector_name(connector)); These will just spam the log as well. > } > return ret; > } > diff --git a/drivers/gpu/drm/radeon/radeon_i2c.c b/drivers/gpu/drm/radeon/radeon_i2c.c > index 781196d..7e93cf9 100644 > --- a/drivers/gpu/drm/radeon/radeon_i2c.c > +++ b/drivers/gpu/drm/radeon/radeon_i2c.c > @@ -34,7 +34,7 @@ > */ > bool radeon_ddc_probe(struct radeon_connector *radeon_connector) > { > - u8 out_buf[] = { 0x0, 0x0}; > + u8 out = 0x0; > u8 buf[2]; > int ret; > struct i2c_msg msgs[] = { > @@ -42,7 +42,7 @@ bool radeon_ddc_probe(struct radeon_connector *radeon_connector) > .addr = 0x50, > .flags = 0, > .len = 1, > - .buf = out_buf, > + .buf = &out, > }, > { > .addr = 0x50, The change above doesn't seem to be related. > @@ -63,6 +63,47 @@ bool radeon_ddc_probe(struct radeon_connector *radeon_connector) > return false; > } > > +/* > + * Probe EDID header information via I2C: > + * Try to fetch EDID information by calling i2c_transfer driver function and > + * probe for a valid EDID header > + */ > +bool radeon_ddc_probe_extended(struct radeon_connector *radeon_connector) > +{ > + u8 out = 0x0; > + u8 block[8]; > + int ret; > + struct i2c_msg msgs[] = { > + { > + .addr = 0x50, > + .flags = 0, > + .len = 1, > + .buf = &out, > + }, { > + .addr = 0x50, > + .flags = I2C_M_RD, > + .len = 8, > + .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 (drm_edid_header_is_valid(block) >= 6) > + /* EDID header starts with: > + * 0x00,0xFF,0xFF,0xFF,0xFF,0xFF,0xFF,0x00 > + * Only the first 6 bytes must be valid as > + * drm_edid_block_valid() can fix the last 2 bytes > + */ > + 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..b834f27 100644 > --- a/drivers/gpu/drm/radeon/radeon_mode.h > +++ b/drivers/gpu/drm/radeon/radeon_mode.h > @@ -438,6 +438,9 @@ struct radeon_connector { > struct radeon_i2c_chan *ddc_bus; > /* some systems have an hdmi and vga port with a shared ddc line */ > bool shared_ddc; > + /* for some Radeon chip families we apply an additional EDID header > + check as part of the DDC probe */ > + bool requires_extended_probe; > bool use_digital; > /* we need to mind the EDID between detect > and get modes due to analog/digital/tvencoder */ > @@ -515,6 +518,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_probe_extended(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); > diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h > index 33d12f8..0ec3687 100644 > --- a/include/drm/drm_crtc.h > +++ b/include/drm/drm_crtc.h > @@ -802,6 +802,7 @@ extern struct drm_display_mode *drm_gtf_mode_complex(struct drm_device *dev, > extern int drm_add_modes_noedid(struct drm_connector *connector, > int hdisplay, int vdisplay); > > +extern int drm_edid_header_is_valid(const u8 *raw_edid); > extern bool drm_edid_is_valid(struct edid *edid); > struct drm_display_mode *drm_mode_find_dmt(struct drm_device *dev, > int hsize, int vsize, int fresh); > -- > 1.7.1 > >
Hi Alex, thank you for your feedback. > Thinking about it more, it might be cleaner to just make the extended > mode a flag, e.g., > dret = radeon_ddc_probe_extended(radeon_connector, > radeon_connector->requires_extended_probe); > and handle the extended fetch in the same probe function. > OK. That makes also sense. I will adapt again the patch. > > + /* RS690 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_type == DRM_MODE_CONNECTOR_HDMIA && > > + (rdev->family == CHIP_RS690 || > > + rdev->family == CHIP_RS740 || > > + rdev->family == CHIP_RV630)) > This seems like an arbitrary selection of chips. I haven't heard of > any problems related to ddc on rv630. Also I think we should limit it > to the specific connector that is problematic rather than all hdmi > ports. In the case of your board, it seems the hdmi port on the > add-in card is the only problematic one. Lots of rs690 motherboards > have hdmi ports on the motherboard itself that work fine. I'd prefer > to match based on the pci device and subsytem ids and the > supported_device and connector type. See radeon_atom_apply_quirks() > in radeon_atombios.c for an example. Something like: > > radeon_connector->requires_extended_probe = > radeon_connector_needs_extended_probe(rdev, supported_dev, > connector_type); I've added RS740 because linux uses the same firmware and this chip was also part of the other patch you mentioned in your first e-mail. RV630 was added because I checked freedesktop bug 31943. The problem described there is different from the one here, but I saw logs, when no monitor was connected, and for this situation the patch would help. With regard to moving away from connector type and chip family to pci devices I'm not really sure. I remember complaints from people a year ago, that used the RS690 on a laptop and had the same problem. I just can't find the related messages. Don't you believe that this could be to focused? Especially, if we compare patch http://git.kernel.org/?p=linux/kernel/git/airlied/drm-2.6.git;a=commitdiff;h=4a9a8b71e12d41abb71c4e741bff524f016cfef4? I felt rather safe with the above approach, as nothing will go wrong, if we check the HDMIA type connectors also RS690 of another manufacturers. We just check for a valid first six bytes set of the EDID header now. > > @@ -777,8 +780,17 @@ 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); > > + /* Log EDID retrieval status here. In particular with regard to > > + * connectors with requires_extended_probe flag set, that will prevent > > + * function radeon_dvi_detect() to fetch EDID on this connector, > > + * as long as there is no valid EDID header found */ > > 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 monitor connected or invalid EDID", > > + drm_get_connector_name(connector)); > > These will just spam the log as well. Here's the log: kernel: [ 14.912386] [drm] Radeon Display Connectors kernel: [ 14.912389] [drm] Connector 0: kernel: [ 14.912391] [drm] VGA kernel: [ 14.912394] [drm] DDC: 0x7e50 0x7e40 0x7e54 0x7e44 0x7e58 0x7e48 0x7e5c 0x7e4c kernel: [ 14.912397] [drm] Encoders: kernel: [ 14.912398] [drm] CRT1: INTERNAL_KLDSCP_DAC1 kernel: [ 14.912401] [drm] Connector 1: kernel: [ 14.912402] [drm] S-video kernel: [ 14.912404] [drm] Encoders: kernel: [ 14.912405] [drm] TV1: INTERNAL_KLDSCP_DAC1 kernel: [ 14.912407] [drm] Connector 2: kernel: [ 14.912409] [drm] HDMI-A kernel: [ 14.912410] [drm] HPD2 kernel: [ 14.912413] [drm] DDC: 0x7e40 0x7e60 0x7e44 0x7e64 0x7e48 0x7e68 0x7e4c 0x7e6c kernel: [ 14.912415] [drm] Encoders: kernel: [ 14.912417] [drm] DFP2: INTERNAL_DDI kernel: [ 14.912419] [drm] Connector 3: kernel: [ 14.912421] [drm] DVI-D kernel: [ 14.912423] [drm] DDC: 0x7e40 0x7e50 0x7e44 0x7e54 0x7e48 0x7e58 0x7e4c 0x7e5c kernel: [ 14.912425] [drm] Encoders: kernel: [ 14.912427] [drm] DFP3: INTERNAL_LVTM1 kernel: [ 15.071566] HDA Intel 0000:00:14.2: PCI INT A -> GSI 16 (level, low) -> IRQ 16 kernel: [ 15.071645] HDA Intel 0000:00:14.2: irq 42 for MSI/MSI-X kernel: [ 15.072678] [drm] Radeon display connector VGA-1: No display connected or invalid EDID kernel: [ 15.470734] Raw EDID: kernel: [ 15.470745] <3>00 00 00 00 00 00 00 00 00 00 00 07 00 00 00 00 ................ kernel: [ 15.470748] <3>00 00 00 00 00 00 00 00 00 00 07 00 00 00 00 00 ................ kernel: [ 15.470751] <3>00 00 00 00 00 00 00 00 00 00 7f 00 00 00 00 00 ................ kernel: [ 15.470754] <3>00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................ kernel: [ 15.470757] <3>00 00 00 00 1f 00 00 00 00 00 00 00 00 00 00 00 ................ kernel: [ 15.470760] <3>00 00 00 00 00 07 00 00 00 00 00 7f 00 00 00 00 ................ kernel: [ 15.470762] <3>00 00 00 00 00 00 07 00 00 00 00 00 00 00 00 00 ................ kernel: [ 15.470765] <3>00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 01 ................ kernel: [ 15.470767] kernel: [ 15.779568] Raw EDID: kernel: [ 15.779578] <3>00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................ kernel: [ 15.779581] <3>00 00 00 00 00 7f 00 00 00 00 03 00 00 00 00 00 ................ kernel: [ 15.779584] <3>00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................ kernel: [ 15.779587] <3>00 00 00 00 ff 00 00 00 00 00 00 00 00 00 00 00 ................ kernel: [ 15.779590] <3>00 00 00 00 00 00 00 00 ff 00 00 00 00 00 00 00 ................ kernel: [ 15.779593] <3>00 00 00 00 00 00 00 00 00 01 00 00 00 00 00 00 ................ kernel: [ 15.779596] <3>00 00 00 7f 00 00 00 00 00 03 07 00 00 00 00 00 ................ kernel: [ 15.779598] <3>00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................ kernel: [ 15.779600] kernel: [ 16.151817] Raw EDID: kernel: [ 16.151827] <3>00 00 00 00 00 00 1f 00 00 00 00 00 00 00 00 00 ................ kernel: [ 16.151830] <3>00 00 00 00 00 00 00 00 00 01 00 00 00 00 00 00 ................ kernel: [ 16.151833] <3>00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................ kernel: [ 16.151836] <3>00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................ kernel: [ 16.151839] <3>00 00 1f 00 00 00 00 00 00 00 00 00 00 00 0f 00 ................ kernel: [ 16.151842] <3>01 00 00 00 00 00 00 00 01 00 00 00 00 07 00 ff ................ kernel: [ 16.151844] <3>00 00 00 00 00 00 ff 00 00 00 00 00 00 00 7f 00 ................ kernel: [ 16.151847] <3>00 0f 00 00 00 00 00 00 00 3f 00 00 00 00 00 00 .........?...... kernel: [ 16.151849] kernel: [ 16.444209] Raw EDID: kernel: [ 16.444220] <3>00 07 00 00 00 00 00 00 ff 00 00 00 00 ff 00 00 ................ kernel: [ 16.444223] <3>00 00 3f 00 00 00 00 00 00 00 00 00 00 ff 00 00 ..?............. kernel: [ 16.444226] <3>00 00 00 00 00 00 00 00 00 07 00 00 00 01 0f 00 ................ kernel: [ 16.444229] <3>00 07 00 00 00 00 00 00 00 00 01 07 00 00 00 00 ................ kernel: [ 16.444231] <3>00 00 00 00 00 00 00 00 7f 00 00 00 00 1f 00 00 ................ kernel: [ 16.444234] <3>00 00 03 00 00 00 00 3f 00 03 00 00 00 00 00 00 .......?........ kernel: [ 16.444237] <3>7f 00 00 1f 00 00 00 00 00 00 00 00 0f 07 00 00 ................ kernel: [ 16.444240] <3>00 00 00 00 00 00 00 00 00 00 03 00 00 00 00 00 ................ kernel: [ 16.444242] kernel: [ 16.444248] radeon 0000:01:05.0: HDMI-A-1: EDID block 0 invalid. kernel: [ 16.444252] [drm] Radeon display connector HDMI-A-1: No display connected or invalid EDID kernel: [ 16.762734] [drm] Radeon display connector DVI-D-1: Found valid EDID Just one info entry per connector will be added during connector setup. The EDID dump was already there and comes from the DDC probing during connector setup. After that, there are no more entries in the log with regard to the buggy HDMI connector. From a user point of view I would prefer to have the two log entries: radeon 0000:01:05.0: HDMI-A-1: EDID block 0 invalid. [drm] Radeon display connector HDMI-A-1: No display connected or invalid EDID. One from the probing, which is a little bit cryptic, and the explaining one which prelude the silence for this connector. > > diff --git a/drivers/gpu/drm/radeon/radeon_i2c.c b/drivers/gpu/drm/radeon/radeon_i2c.c > > index 781196d..7e93cf9 100644 > > --- a/drivers/gpu/drm/radeon/radeon_i2c.c > > +++ b/drivers/gpu/drm/radeon/radeon_i2c.c > > @@ -34,7 +34,7 @@ > > */ > > bool radeon_ddc_probe(struct radeon_connector *radeon_connector) > > { > > - u8 out_buf[] = { 0x0, 0x0}; > > + u8 out = 0x0; > > u8 buf[2]; > > int ret; > > struct i2c_msg msgs[] = { > > @@ -42,7 +42,7 @@ bool radeon_ddc_probe(struct radeon_connector *radeon_connector) > > .addr = 0x50, > > .flags = 0, > > .len = 1, > > - .buf = out_buf, > > + .buf = &out, > > }, > > { > > .addr = 0x50, > > > The change above doesn't seem to be related. This was a comment from Jean who complained about the ineffective usage of the i2c bus. But I can also restore the old code. What's your preference? Best regards Thomas
On Fri, Jun 24, 2011 at 12:02 AM, Thomas Reim <thomas.reim@nepomuc.de> wrote: > Hi Alex, > > thank you for your feedback. > >> Thinking about it more, it might be cleaner to just make the extended >> mode a flag, e.g., >> dret = radeon_ddc_probe_extended(radeon_connector, >> radeon_connector->requires_extended_probe); >> and handle the extended fetch in the same probe function. >> > > OK. That makes also sense. I will adapt again the patch. > >> > + /* RS690 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_type == DRM_MODE_CONNECTOR_HDMIA && >> > + (rdev->family == CHIP_RS690 || >> > + rdev->family == CHIP_RS740 || >> > + rdev->family == CHIP_RV630)) >> This seems like an arbitrary selection of chips. I haven't heard of >> any problems related to ddc on rv630. Also I think we should limit it >> to the specific connector that is problematic rather than all hdmi >> ports. In the case of your board, it seems the hdmi port on the >> add-in card is the only problematic one. Lots of rs690 motherboards >> have hdmi ports on the motherboard itself that work fine. I'd prefer >> to match based on the pci device and subsytem ids and the >> supported_device and connector type. See radeon_atom_apply_quirks() >> in radeon_atombios.c for an example. Something like: >> >> radeon_connector->requires_extended_probe = >> radeon_connector_needs_extended_probe(rdev, supported_dev, >> connector_type); > > I've added RS740 because linux uses the same firmware and this chip was > also part of the other patch you mentioned in your first e-mail. RV630 > was added because I checked freedesktop bug 31943. The problem described > there is different from the one here, but I saw logs, when no monitor > was connected, and for this situation the patch would help. I'd rather add quirks on a case by case bases rather than speculating on mailing list reports. > > With regard to moving away from connector type and chip family to pci > devices I'm not really sure. I remember complaints from people a year > ago, that used the RS690 on a laptop and had the same problem. I just > can't find the related messages. Don't you believe that this could be to > focused? Especially, if we compare patch > http://git.kernel.org/?p=linux/kernel/git/airlied/drm-2.6.git;a=commitdiff;h=4a9a8b71e12d41abb71c4e741bff524f016cfef4? > We should probably revert or rework that patch when we apply yours otherwise we may end up removing i2c buses unnecessarily in some cases. I think this is a better approach. > I felt rather safe with the above approach, as nothing will go wrong, if > we check the HDMIA type connectors also RS690 of another manufacturers. > We just check for a valid first six bytes set of the EDID header now. As I said above, lets just apply this to the specific board and connector that is problematic. I seems to only affect the hdmi port on the add-in cards, so there's no need to penalize all hdmi ports. If we get enough quirks down the road, we can make it a general rule. > >> > @@ -777,8 +780,17 @@ 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); >> > + /* Log EDID retrieval status here. In particular with regard to >> > + * connectors with requires_extended_probe flag set, that will prevent >> > + * function radeon_dvi_detect() to fetch EDID on this connector, >> > + * as long as there is no valid EDID header found */ >> > 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 monitor connected or invalid EDID", >> > + drm_get_connector_name(connector)); >> >> These will just spam the log as well. > > Here's the log: > kernel: [ 14.912386] [drm] Radeon Display Connectors > kernel: [ 14.912389] [drm] Connector 0: > kernel: [ 14.912391] [drm] VGA > kernel: [ 14.912394] [drm] DDC: 0x7e50 0x7e40 0x7e54 0x7e44 0x7e58 > 0x7e48 0x7e5c 0x7e4c > kernel: [ 14.912397] [drm] Encoders: > kernel: [ 14.912398] [drm] CRT1: INTERNAL_KLDSCP_DAC1 > kernel: [ 14.912401] [drm] Connector 1: > kernel: [ 14.912402] [drm] S-video > kernel: [ 14.912404] [drm] Encoders: > kernel: [ 14.912405] [drm] TV1: INTERNAL_KLDSCP_DAC1 > kernel: [ 14.912407] [drm] Connector 2: > kernel: [ 14.912409] [drm] HDMI-A > kernel: [ 14.912410] [drm] HPD2 > kernel: [ 14.912413] [drm] DDC: 0x7e40 0x7e60 0x7e44 0x7e64 0x7e48 > 0x7e68 0x7e4c 0x7e6c > kernel: [ 14.912415] [drm] Encoders: > kernel: [ 14.912417] [drm] DFP2: INTERNAL_DDI > kernel: [ 14.912419] [drm] Connector 3: > kernel: [ 14.912421] [drm] DVI-D > kernel: [ 14.912423] [drm] DDC: 0x7e40 0x7e50 0x7e44 0x7e54 0x7e48 > 0x7e58 0x7e4c 0x7e5c > kernel: [ 14.912425] [drm] Encoders: > kernel: [ 14.912427] [drm] DFP3: INTERNAL_LVTM1 > kernel: [ 15.071566] HDA Intel 0000:00:14.2: PCI INT A -> GSI 16 > (level, low) -> IRQ 16 > kernel: [ 15.071645] HDA Intel 0000:00:14.2: irq 42 for MSI/MSI-X > kernel: [ 15.072678] [drm] Radeon display connector VGA-1: No display > connected or invalid EDID > kernel: [ 15.470734] Raw EDID: > kernel: [ 15.470745] <3>00 00 00 00 00 00 00 00 00 00 00 07 00 00 00 > 00 ................ > kernel: [ 15.470748] <3>00 00 00 00 00 00 00 00 00 00 07 00 00 00 00 > 00 ................ > kernel: [ 15.470751] <3>00 00 00 00 00 00 00 00 00 00 7f 00 00 00 00 > 00 ................ > kernel: [ 15.470754] <3>00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 > 00 ................ > kernel: [ 15.470757] <3>00 00 00 00 1f 00 00 00 00 00 00 00 00 00 00 > 00 ................ > kernel: [ 15.470760] <3>00 00 00 00 00 07 00 00 00 00 00 7f 00 00 00 > 00 ................ > kernel: [ 15.470762] <3>00 00 00 00 00 00 07 00 00 00 00 00 00 00 00 > 00 ................ > kernel: [ 15.470765] <3>00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 > 01 ................ > kernel: [ 15.470767] > kernel: [ 15.779568] Raw EDID: > kernel: [ 15.779578] <3>00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 > 00 ................ > kernel: [ 15.779581] <3>00 00 00 00 00 7f 00 00 00 00 03 00 00 00 00 > 00 ................ > kernel: [ 15.779584] <3>00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 > 00 ................ > kernel: [ 15.779587] <3>00 00 00 00 ff 00 00 00 00 00 00 00 00 00 00 > 00 ................ > kernel: [ 15.779590] <3>00 00 00 00 00 00 00 00 ff 00 00 00 00 00 00 > 00 ................ > kernel: [ 15.779593] <3>00 00 00 00 00 00 00 00 00 01 00 00 00 00 00 > 00 ................ > kernel: [ 15.779596] <3>00 00 00 7f 00 00 00 00 00 03 07 00 00 00 00 > 00 ................ > kernel: [ 15.779598] <3>00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 > 00 ................ > kernel: [ 15.779600] > kernel: [ 16.151817] Raw EDID: > kernel: [ 16.151827] <3>00 00 00 00 00 00 1f 00 00 00 00 00 00 00 00 > 00 ................ > kernel: [ 16.151830] <3>00 00 00 00 00 00 00 00 00 01 00 00 00 00 00 > 00 ................ > kernel: [ 16.151833] <3>00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 > 00 ................ > kernel: [ 16.151836] <3>00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 > 00 ................ > kernel: [ 16.151839] <3>00 00 1f 00 00 00 00 00 00 00 00 00 00 00 0f > 00 ................ > kernel: [ 16.151842] <3>01 00 00 00 00 00 00 00 01 00 00 00 00 07 00 > ff ................ > kernel: [ 16.151844] <3>00 00 00 00 00 00 ff 00 00 00 00 00 00 00 7f > 00 ................ > kernel: [ 16.151847] <3>00 0f 00 00 00 00 00 00 00 3f 00 00 00 00 00 > 00 .........?...... > kernel: [ 16.151849] > kernel: [ 16.444209] Raw EDID: > kernel: [ 16.444220] <3>00 07 00 00 00 00 00 00 ff 00 00 00 00 ff 00 > 00 ................ > kernel: [ 16.444223] <3>00 00 3f 00 00 00 00 00 00 00 00 00 00 ff 00 > 00 ..?............. > kernel: [ 16.444226] <3>00 00 00 00 00 00 00 00 00 07 00 00 00 01 0f > 00 ................ > kernel: [ 16.444229] <3>00 07 00 00 00 00 00 00 00 00 01 07 00 00 00 > 00 ................ > kernel: [ 16.444231] <3>00 00 00 00 00 00 00 00 7f 00 00 00 00 1f 00 > 00 ................ > kernel: [ 16.444234] <3>00 00 03 00 00 00 00 3f 00 03 00 00 00 00 00 > 00 .......?........ > kernel: [ 16.444237] <3>7f 00 00 1f 00 00 00 00 00 00 00 00 0f 07 00 > 00 ................ > kernel: [ 16.444240] <3>00 00 00 00 00 00 00 00 00 00 03 00 00 00 00 > 00 ................ > kernel: [ 16.444242] > kernel: [ 16.444248] radeon 0000:01:05.0: HDMI-A-1: EDID block 0 > invalid. > kernel: [ 16.444252] [drm] Radeon display connector HDMI-A-1: No > display connected or invalid EDID > kernel: [ 16.762734] [drm] Radeon display connector DVI-D-1: Found > valid EDID > > Just one info entry per connector will be added during connector setup. > The EDID dump was already there and comes from the DDC probing during > connector setup. After that, there are no more entries in the log with > regard to the buggy HDMI connector. From a user point of view I would > prefer to have the two log entries: > radeon 0000:01:05.0: HDMI-A-1: EDID block 0 invalid. > [drm] Radeon display connector HDMI-A-1: No display connected or invalid > EDID. > One from the probing, which is a little bit cryptic, and the explaining > one which prelude the silence for this connector. ok, that makes sense. > >> > diff --git a/drivers/gpu/drm/radeon/radeon_i2c.c b/drivers/gpu/drm/radeon/radeon_i2c.c >> > index 781196d..7e93cf9 100644 >> > --- a/drivers/gpu/drm/radeon/radeon_i2c.c >> > +++ b/drivers/gpu/drm/radeon/radeon_i2c.c >> > @@ -34,7 +34,7 @@ >> > */ >> > bool radeon_ddc_probe(struct radeon_connector *radeon_connector) >> > { >> > - u8 out_buf[] = { 0x0, 0x0}; >> > + u8 out = 0x0; >> > u8 buf[2]; >> > int ret; >> > struct i2c_msg msgs[] = { >> > @@ -42,7 +42,7 @@ bool radeon_ddc_probe(struct radeon_connector *radeon_connector) >> > .addr = 0x50, >> > .flags = 0, >> > .len = 1, >> > - .buf = out_buf, >> > + .buf = &out, >> > }, >> > { >> > .addr = 0x50, >> >> >> The change above doesn't seem to be related. > > This was a comment from Jean who complained about the ineffective usage > of the i2c bus. But I can also restore the old code. What's your > preference? Ah, I missed that. Let's make that a separate patch, or fix it when you add support for the extended edid check. Thanks for fixing this up. Alex > > Best regards > > Thomas > >
On Fri, 24 Jun 2011 09:36:56 -0400, Alex Deucher wrote: > On Fri, Jun 24, 2011 at 12:02 AM, Thomas Reim <thomas.reim@nepomuc.de> wrote: > >> > diff --git a/drivers/gpu/drm/radeon/radeon_i2c.c b/drivers/gpu/drm/radeon/radeon_i2c.c > >> > index 781196d..7e93cf9 100644 > >> > --- a/drivers/gpu/drm/radeon/radeon_i2c.c > >> > +++ b/drivers/gpu/drm/radeon/radeon_i2c.c > >> > @@ -34,7 +34,7 @@ > >> > */ > >> > bool radeon_ddc_probe(struct radeon_connector *radeon_connector) > >> > { > >> > - u8 out_buf[] = { 0x0, 0x0}; > >> > + u8 out = 0x0; > >> > u8 buf[2]; > >> > int ret; > >> > struct i2c_msg msgs[] = { > >> > @@ -42,7 +42,7 @@ bool radeon_ddc_probe(struct radeon_connector *radeon_connector) > >> > .addr = 0x50, > >> > .flags = 0, > >> > .len = 1, > >> > - .buf = out_buf, > >> > + .buf = &out, > >> > }, > >> > { > >> > .addr = 0x50, > >> > >> > >> The change above doesn't seem to be related. > > > > This was a comment from Jean who complained about the ineffective usage > > of the i2c bus. But I can also restore the old code. What's your > > preference? > > Ah, I missed that. Let's make that a separate patch, or fix it when > you add support for the extended edid check. > > Thanks for fixing this up. I'll send a patch for that one, as I found it. It is indeed unrelated to the problem, I mentioned it only to avoid the same mistake in a newly added function. It's a very minor cleanup / optimization, BTW.
Dear Alex, sorry for the long delay. I have updated the patch according to the comments in your last e-mail (see below). I've added function radeon_connector_needs_extended_probe() and flag radeon_connector->requires_extended_probe that provide the means to force on a board,connector basis to check for a valid EDID header during DDC probe. I've adapted function radeon_ddc_probe() to perform an additional EDID header check if required by the connector. Finally, I've adapted function radeon_device_init() to also log subsystem vendor and device information, if available. This allows to add further quirks based on the dmesg output of drm/radeon. > > I've added RS740 because linux uses the same firmware and this chip was > > also part of the other patch you mentioned in your first e-mail. RV630 > > was added because I checked freedesktop bug 31943. The problem described > > there is different from the one here, but I saw logs, when no monitor > > was connected, and for this situation the patch would help. > > I'd rather add quirks on a case by case bases rather than speculating > on mailing list reports. Done. See function radeon_connector_needs_extended_probe() > > > > > With regard to moving away from connector type and chip family to pci > > devices I'm not really sure. I remember complaints from people a year > > ago, that used the RS690 on a laptop and had the same problem. I just > > can't find the related messages. Don't you believe that this could be to > > focused? Especially, if we compare patch > > http://git.kernel.org/?p=linux/kernel/git/airlied/drm-2.6.git;a=commitdiff;h=4a9a8b71e12d41abb71c4e741bff524f016cfef4? > > > > We should probably revert or rework that patch when we apply yours > otherwise we may end up removing i2c buses unnecessarily in some > cases. I think this is a better approach. Understood. > > > I felt rather safe with the above approach, as nothing will go wrong, if > > we check the HDMIA type connectors also RS690 of another manufacturers. > > We just check for a valid first six bytes set of the EDID header now. > > As I said above, lets just apply this to the specific board and > connector that is problematic. I seems to only affect the hdmi port > on the add-in cards, so there's no need to penalize all hdmi ports. > If we get enough quirks down the road, we can make it a general rule. > You're right. The patch currently fixes only the ASUS M2A-VM device: + if ((dev->pdev->device == 0x791e) && + (dev->pdev->subsystem_vendor == 0x1043) && + (dev->pdev->subsystem_device == 0x826d)) { + if ((connector_type == DRM_MODE_CONNECTOR_HDMIA) && + (supported_device == ATOM_DEVICE_DFP2_SUPPORT)) This will solve bug https://bugs.launchpad.net/ubuntu/+source/linux/+bug/722806. > > > > Just one info entry per connector will be added during connector setup. > > The EDID dump was already there and comes from the DDC probing during > > connector setup. After that, there are no more entries in the log with > > regard to the buggy HDMI connector. From a user point of view I would > > prefer to have the two log entries: > > radeon 0000:01:05.0: HDMI-A-1: EDID block 0 invalid. > > [drm] Radeon display connector HDMI-A-1: No display connected or invalid > > EDID. > > One from the probing, which is a little bit cryptic, and the explaining > > one which prelude the silence for this connector. > > ok, that makes sense. > > > > >> > diff --git a/drivers/gpu/drm/radeon/radeon_i2c.c b/drivers/gpu/drm/radeon/radeon_i2c.c > >> > index 781196d..7e93cf9 100644 > >> > --- a/drivers/gpu/drm/radeon/radeon_i2c.c > >> > +++ b/drivers/gpu/drm/radeon/radeon_i2c.c > >> > @@ -34,7 +34,7 @@ > >> > */ > >> > bool radeon_ddc_probe(struct radeon_connector *radeon_connector) > >> > { > >> > - u8 out_buf[] = { 0x0, 0x0}; > >> > + u8 out = 0x0; > >> > u8 buf[2]; > >> > int ret; > >> > struct i2c_msg msgs[] = { > >> > @@ -42,7 +42,7 @@ bool radeon_ddc_probe(struct radeon_connector *radeon_connector) > >> > .addr = 0x50, > >> > .flags = 0, > >> > .len = 1, > >> > - .buf = out_buf, > >> > + .buf = &out, > >> > }, > >> > { > >> > .addr = 0x50, > >> > >> > >> The change above doesn't seem to be related. > > > > This was a comment from Jean who complained about the ineffective usage > > of the i2c bus. But I can also restore the old code. What's your > > preference? > > Ah, I missed that. Let's make that a separate patch, or fix it when > you add support for the extended edid check. > Done. The patch is for the latest git version of the linux kernel. Do you think it would be possible to provide this patch already starting from the most recent 2.35 kernel update? Ubuntu uses this kernel version for the long-term stable Maverick release. I will send the patch in my next e-mail. Hope this helps you. Best regards Thomas
diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c index 0929219..1daaf81 100644 --- a/drivers/gpu/drm/drm_edid.c +++ b/drivers/gpu/drm/drm_edid.c @@ -128,6 +128,23 @@ static const u8 edid_header[] = { }; /* + * Sanity check the header of the base EDID block. Return 8 if the header + * is perfect, down to 0 if it's totally wrong. + */ +int drm_edid_header_is_valid(const u8 *raw_edid) +{ + int i, score = 0; + + for (i = 0; i < sizeof(edid_header); i++) + if (raw_edid[i] == edid_header[i]) + score++; + + return score; +} +EXPORT_SYMBOL(drm_edid_header_is_valid); + + +/* * Sanity check the EDID block (base or extension). Return 0 if the block * doesn't check out, or 1 if it's valid. */ @@ -139,11 +156,7 @@ drm_edid_block_valid(u8 *raw_edid) struct edid *edid = (struct edid *)raw_edid; if (raw_edid[0] == 0x00) { - int score = 0; - - for (i = 0; i < sizeof(edid_header); i++) - if (raw_edid[i] == edid_header[i]) - score++; + int score = drm_edid_header_is_valid(raw_edid); if (score == 8) ; else if (score >= 6) { diff --git a/drivers/gpu/drm/radeon/radeon_connectors.c b/drivers/gpu/drm/radeon/radeon_connectors.c index cbfca3a..bcd2380 100644 --- a/drivers/gpu/drm/radeon/radeon_connectors.c +++ b/drivers/gpu/drm/radeon/radeon_connectors.c @@ -826,8 +826,12 @@ radeon_dvi_detect(struct drm_connector *connector, bool force) 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) { + if (radeon_connector->requires_extended_probe) + dret = radeon_ddc_probe_extended(radeon_connector); + else + dret = radeon_ddc_probe(radeon_connector); + } if (dret) { if (radeon_connector->edid) { kfree(radeon_connector->edid); @@ -1342,6 +1346,7 @@ radeon_add_atom_connector(struct drm_device *dev, struct radeon_encoder *radeon_encoder; uint32_t subpixel_order = SubPixelNone; bool shared_ddc = false; + bool requires_extended_probe = false; bool is_dp_bridge = false; if (connector_type == DRM_MODE_CONNECTOR_Unknown) @@ -1400,6 +1405,7 @@ radeon_add_atom_connector(struct drm_device *dev, radeon_connector->shared_ddc = shared_ddc; radeon_connector->connector_object_id = connector_object_id; radeon_connector->hpd = *hpd; + radeon_connector->requires_extended_probe = requires_extended_probe; radeon_connector->router = *router; if (router->ddc_valid || router->cd_valid) { radeon_connector->router_bus = radeon_i2c_lookup(rdev, &router->i2c_info); @@ -1688,6 +1694,23 @@ radeon_add_atom_connector(struct drm_device *dev, } } + /* RS690 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_type == DRM_MODE_CONNECTOR_HDMIA && + (rdev->family == CHIP_RS690 || + rdev->family == CHIP_RS740 || + rdev->family == CHIP_RV630)) + requires_extended_probe = true; + radeon_connector->requires_extended_probe = requires_extended_probe; if (radeon_connector->hpd.hpd == RADEON_HPD_NONE) { if (i2c_bus->valid) connector->polled = DRM_CONNECTOR_POLL_CONNECT; @@ -1716,6 +1739,7 @@ radeon_add_legacy_connector(struct drm_device *dev, struct drm_connector *connector; struct radeon_connector *radeon_connector; uint32_t subpixel_order = SubPixelNone; + bool requires_extended_probe = false; if (connector_type == DRM_MODE_CONNECTOR_Unknown) return; @@ -1746,6 +1770,7 @@ radeon_add_legacy_connector(struct drm_device *dev, radeon_connector->devices = supported_device; radeon_connector->connector_object_id = connector_object_id; radeon_connector->hpd = *hpd; + radeon_connector->requires_extended_probe = requires_extended_probe; switch (connector_type) { case DRM_MODE_CONNECTOR_VGA: drm_connector_init(dev, &radeon_connector->base, &radeon_vga_connector_funcs, connector_type); diff --git a/drivers/gpu/drm/radeon/radeon_display.c b/drivers/gpu/drm/radeon/radeon_display.c index 292f73f..27e6840 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,17 @@ 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); + /* Log EDID retrieval status here. In particular with regard to + * connectors with requires_extended_probe flag set, that will prevent + * function radeon_dvi_detect() to fetch EDID on this connector, + * as long as there is no valid EDID header found */ 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 monitor 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..7e93cf9 100644 --- a/drivers/gpu/drm/radeon/radeon_i2c.c +++ b/drivers/gpu/drm/radeon/radeon_i2c.c @@ -34,7 +34,7 @@ */ bool radeon_ddc_probe(struct radeon_connector *radeon_connector) { - u8 out_buf[] = { 0x0, 0x0}; + u8 out = 0x0; u8 buf[2]; int ret; struct i2c_msg msgs[] = { @@ -42,7 +42,7 @@ bool radeon_ddc_probe(struct radeon_connector *radeon_connector) .addr = 0x50, .flags = 0, .len = 1, - .buf = out_buf, + .buf = &out, }, { .addr = 0x50, @@ -63,6 +63,47 @@ bool radeon_ddc_probe(struct radeon_connector *radeon_connector) return false; } +/* + * Probe EDID header information via I2C: + * Try to fetch EDID information by calling i2c_transfer driver function and + * probe for a valid EDID header + */ +bool radeon_ddc_probe_extended(struct radeon_connector *radeon_connector) +{ + u8 out = 0x0; + u8 block[8]; + int ret; + struct i2c_msg msgs[] = { + { + .addr = 0x50, + .flags = 0, + .len = 1, + .buf = &out, + }, { + .addr = 0x50, + .flags = I2C_M_RD, + .len = 8, + .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 (drm_edid_header_is_valid(block) >= 6) + /* EDID header starts with: + * 0x00,0xFF,0xFF,0xFF,0xFF,0xFF,0xFF,0x00 + * Only the first 6 bytes must be valid as + * drm_edid_block_valid() can fix the last 2 bytes + */ + 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..b834f27 100644 --- a/drivers/gpu/drm/radeon/radeon_mode.h +++ b/drivers/gpu/drm/radeon/radeon_mode.h @@ -438,6 +438,9 @@ struct radeon_connector { struct radeon_i2c_chan *ddc_bus; /* some systems have an hdmi and vga port with a shared ddc line */ bool shared_ddc; + /* for some Radeon chip families we apply an additional EDID header + check as part of the DDC probe */ + bool requires_extended_probe; bool use_digital; /* we need to mind the EDID between detect and get modes due to analog/digital/tvencoder */ @@ -515,6 +518,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_probe_extended(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); diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h index 33d12f8..0ec3687 100644 --- a/include/drm/drm_crtc.h +++ b/include/drm/drm_crtc.h @@ -802,6 +802,7 @@ extern struct drm_display_mode *drm_gtf_mode_complex(struct drm_device *dev, extern int drm_add_modes_noedid(struct drm_connector *connector, int hdisplay, int vdisplay); +extern int drm_edid_header_is_valid(const u8 *raw_edid); extern bool drm_edid_is_valid(struct edid *edid); struct drm_display_mode *drm_mode_find_dmt(struct drm_device *dev, int hsize, int vsize, int fresh);