* [PATCH 0/2] drm_radeon_kms: Some Regression Fixes for Extended DDC Probe
@ 2011-11-28 16:20 Thomas Reim
2011-11-28 16:20 ` [PATCH 1/2] drm: Improve detection of floating connectors Thomas Reim
` (2 more replies)
0 siblings, 3 replies; 10+ messages in thread
From: Thomas Reim @ 2011-11-28 16:20 UTC (permalink / raw)
To: Dave Airlie, Alex Deucher; +Cc: Thomas Reim, dri-devel
Since Linux 3.2-rc2 the following two patches have changed the DDC detect and
and probe behaviour of the radeon driver:
- drm/radeon/kms: always do extended edid probe
- drm/radeon/kms: remove useless radeon_ddc_dump()
The first patch makes an EDID header check mandatory during DDC detection/probe.
The second patch removes the DDC check during Radeon device setup, that was done
before the actual connector detection. The log output of this check confused
users with DP, eDP, or DP bridge connectors.
Found regression:
(1) The stop of DDC detection for floating connectors, i. e. connectors with
improperly wired/terminated i2c bus, of RS690/RS740 family chipsets does
not work anymore.
(2) HW bugs that result in the DDC being available, but unusable EDID (header)
do now lead to connectors silently probed via i2c but not being used. There
is no notification to the user (via kernel logs). Even with drm debugging
enabled at kernel boot time, there is no information. No chance for a user
to identify the root cause of the connector being probed but not being
used.
The attached two fixes restore the RS690/RS740 floating connectors quirk,
i. e. stop of DDC detection. In addition, the detection of such floating
connectors is now more comprehensive: Also floating connectors with a few (<= 8)
random byte values other than zero reported during i2c transaction are now
properly detected.
During discussion of patch "drm/radeon/kms: remove useless radeon_ddc_dump()"
(see http://www.spinics.net/lists/dri-devel/msg15523.html) the proposal was
made to include the log information of the removed function radeon_ddc_dump()
into function drm_fb_helper_initial_config(). Further investigations showed,
that during intial framebuffer configuration the connector (type) specific
detect function will be called surrounded by kernel debug log message output.
In the here important radeon DVI connector case function radeon_dvi_detect()
is called. As this function is anyway touched by the present patch,
I decided to move the unusable EDID logging there. Counter radeon_connector->
broken_edid_header_counter will ensure, that kernel logs are not flooded, as
it was the case before implementation of extended DDC/EDID probe.
Thomas Reim (2):
drm: Improve detection of floating connectors
drm/radeon/kms: wrap-up handling of floating connectors and connector
unavailability status logging
drivers/gpu/drm/drm_edid.c | 13 ++++++-
drivers/gpu/drm/radeon/radeon_connectors.c | 54 +++++++++++++++++++++++-----
drivers/gpu/drm/radeon/radeon_i2c.c | 7 +++-
drivers/gpu/drm/radeon/radeon_mode.h | 3 ++
4 files changed, 65 insertions(+), 12 deletions(-)
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH 1/2] drm: Improve detection of floating connectors
2011-11-28 16:20 [PATCH 0/2] drm_radeon_kms: Some Regression Fixes for Extended DDC Probe Thomas Reim
@ 2011-11-28 16:20 ` Thomas Reim
2011-11-28 16:20 ` [PATCH 2/2] drm/radeon/kms: wrap-up handling of floating connectors and connector unavailability status logging Thomas Reim
2011-11-28 16:34 ` [PATCH 0/2] drm_radeon_kms: Some Regression Fixes for Extended DDC Probe Thomas Reim
2 siblings, 0 replies; 10+ messages in thread
From: Thomas Reim @ 2011-11-28 16:20 UTC (permalink / raw)
To: Dave Airlie, Alex Deucher; +Cc: Thomas Reim, dri-devel
Some RS690 chipsets seem to end up with floating connectors, either
a DVI connector isn't actually populated, or an add-in HDMI card
is available but not installed. In this case we seem to get a NULL byte
response for each byte of the i2c transaction.
Function drm_edid_is_zero has been introduced to handle this. But this
function detects only all-0 EDIDs. Testing showed that there are floating
RS690 connectors that responds also few random value bytes via i2c transaction.
So we detect also this case.
I've tested this on my RS690 without the HDMI card installed and
it seems to work fine.
Signed-off-by: Thomas Reim <reimth@gmail.com>
---
drivers/gpu/drm/drm_edid.c | 13 ++++++++++++-
1 files changed, 12 insertions(+), 1 deletions(-)
diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
index 3e927ce..0e9be64 100644
--- a/drivers/gpu/drm/drm_edid.c
+++ b/drivers/gpu/drm/drm_edid.c
@@ -271,14 +271,25 @@ drm_do_probe_ddc_edid(struct i2c_adapter *adapter, unsigned char *buf,
return ret == 2 ? 0 : -1;
}
+/*
+ * drm_edid_is_zero - zero value EDID records
+ * @edid: EDID data
+ *
+ * Check if an EDID record mainly consists of zero bytes
+ */
static bool drm_edid_is_zero(u8 *in_edid, int length)
{
+ int non_zero_counter = 0;
int i;
u32 *raw_edid = (u32 *)in_edid;
- for (i = 0; i < length / 4; i++)
+ for (i = 0; i < length / 4; i++) {
if (*(raw_edid + i) != 0)
+ non_zero_counter++;
+ /* ignore random non-zero bytes */
+ if (non_zero_counter > 8)
return false;
+ }
return true;
}
--
1.7.1
^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH 2/2] drm/radeon/kms: wrap-up handling of floating connectors and connector unavailability status logging
2011-11-28 16:20 [PATCH 0/2] drm_radeon_kms: Some Regression Fixes for Extended DDC Probe Thomas Reim
2011-11-28 16:20 ` [PATCH 1/2] drm: Improve detection of floating connectors Thomas Reim
@ 2011-11-28 16:20 ` Thomas Reim
2011-11-29 14:50 ` Alex Deucher
2011-11-29 18:33 ` Thomas Reim
2011-11-28 16:34 ` [PATCH 0/2] drm_radeon_kms: Some Regression Fixes for Extended DDC Probe Thomas Reim
2 siblings, 2 replies; 10+ messages in thread
From: Thomas Reim @ 2011-11-28 16:20 UTC (permalink / raw)
To: Dave Airlie, Alex Deucher; +Cc: Thomas Reim, dri-devel
Extended DDC probe is now default for RADEON chipsets. In case of
HW bugs (e. g. floating connectors), the affected connectors will
not be used, as a valid EDID header can not be detected. Another
patch removed DDC detection and connector status logging during
device setup. So the user is not informed anymore about HW bugs
leading to connectors being unavailable.
Reintroduce one-time logging of connector unavailability status
when probing (single) connector modes during framebuffer
initialisation.
For RS690 chipsets DDC detection shall be stopped, if the i2c bus
receives all-0 EDIDs (floating connectors). The introduction of
extended DDC probing prevents the driver from doing this. Correctly
relocate the related code.
Signed-off-by: Thomas Reim <reimth@gmail.com>
---
drivers/gpu/drm/radeon/radeon_connectors.c | 54 +++++++++++++++++++++++-----
drivers/gpu/drm/radeon/radeon_i2c.c | 7 +++-
drivers/gpu/drm/radeon/radeon_mode.h | 3 ++
3 files changed, 53 insertions(+), 11 deletions(-)
diff --git a/drivers/gpu/drm/radeon/radeon_connectors.c b/drivers/gpu/drm/radeon/radeon_connectors.c
index e7cb3ab..a4c0f59 100644
--- a/drivers/gpu/drm/radeon/radeon_connectors.c
+++ b/drivers/gpu/drm/radeon/radeon_connectors.c
@@ -662,6 +662,7 @@ radeon_vga_detect(struct drm_connector *connector, bool force)
struct radeon_connector *radeon_connector = to_radeon_connector(connector);
struct drm_encoder *encoder;
struct drm_encoder_helper_funcs *encoder_funcs;
+ struct edid *edid = NULL;
bool dret = false;
enum drm_connector_status ret = connector_status_disconnected;
@@ -669,8 +670,24 @@ radeon_vga_detect(struct drm_connector *connector, bool force)
if (!encoder)
ret = connector_status_disconnected;
- if (radeon_connector->ddc_bus)
+ if (radeon_connector->ddc_bus) {
dret = radeon_ddc_probe(radeon_connector);
+ if (!dret && radeon_connector->broken_edid_header_counter == 1) {
+ /* Found a broken DDC when probing (single) connector
+ modes during framebuffer initialisation.
+ Clean-up and log the HW bug. */
+ edid = drm_get_edid(&radeon_connector->base, &radeon_connector->ddc_bus->adapter);
+ if (!edid) {
+ radeon_connector->broken_edid_header_counter++;
+ DRM_INFO("%s: probed a monitor, DDC responded but no|invalid EDID\n",
+ drm_get_connector_name(connector));
+ } else {
+ /* We should not get here, as the EDID header is broken */
+ connector->display_info.raw_edid = NULL;
+ kfree(edid);
+ }
+ }
+ }
if (dret) {
radeon_connector->detected_by_load = false;
if (radeon_connector->edid) {
@@ -847,12 +864,38 @@ radeon_dvi_detect(struct drm_connector *connector, bool force)
struct drm_encoder *encoder = NULL;
struct drm_encoder_helper_funcs *encoder_funcs;
struct drm_mode_object *obj;
+ struct edid *edid = NULL;
int i;
enum drm_connector_status ret = connector_status_disconnected;
bool dret = false;
- if (radeon_connector->ddc_bus)
+ if (radeon_connector->ddc_bus) {
dret = radeon_ddc_probe(radeon_connector);
+ if (!dret && radeon_connector->broken_edid_header_counter == 1) {
+ /* Found a broken DDC when probing (single) connector
+ modes during framebuffer initialisation.
+ Clean-up and log the HW bug. */
+ edid = drm_get_edid(&radeon_connector->base, &radeon_connector->ddc_bus->adapter);
+ if (!edid) {
+ radeon_connector->broken_edid_header_counter++;
+ DRM_INFO("%s: probed a monitor, DDC responded but no|invalid EDID\n",
+ drm_get_connector_name(connector));
+ /* rs690 seems to have a problem with connectors not existing and return
+ * random EDIDs mainly with blocks of 0's. If we see this, just stop
+ * polling on this output */
+ if ((rdev->family == CHIP_RS690 || rdev->family == CHIP_RS740)) {
+ ret = connector_status_disconnected;
+ DRM_INFO("%s: detected RS690 floating bus bug, stopping ddc detect\n",
+ drm_get_connector_name(connector));
+ radeon_connector->ddc_bus = NULL;
+ }
+ } else {
+ /* We should not get here, as the EDID header is broken */
+ connector->display_info.raw_edid = NULL;
+ kfree(edid);
+ }
+ }
+ }
if (dret) {
radeon_connector->detected_by_load = false;
if (radeon_connector->edid) {
@@ -864,13 +907,6 @@ radeon_dvi_detect(struct drm_connector *connector, bool force)
if (!radeon_connector->edid) {
DRM_ERROR("%s: probed a monitor but no|invalid EDID\n",
drm_get_connector_name(connector));
- /* rs690 seems to have a problem with connectors not existing and always
- * return a block of 0's. If we see this just stop polling on this output */
- if ((rdev->family == CHIP_RS690 || rdev->family == CHIP_RS740) && radeon_connector->base.null_edid_counter) {
- ret = connector_status_disconnected;
- DRM_ERROR("%s: detected RS690 floating bus bug, stopping ddc detect\n", drm_get_connector_name(connector));
- radeon_connector->ddc_bus = NULL;
- }
} else {
radeon_connector->use_digital = !!(radeon_connector->edid->input & DRM_EDID_INPUT_DIGITAL);
diff --git a/drivers/gpu/drm/radeon/radeon_i2c.c b/drivers/gpu/drm/radeon/radeon_i2c.c
index 7bb1b07..ac6aa02 100644
--- a/drivers/gpu/drm/radeon/radeon_i2c.c
+++ b/drivers/gpu/drm/radeon/radeon_i2c.c
@@ -59,10 +59,12 @@ bool radeon_ddc_probe(struct radeon_connector *radeon_connector)
radeon_router_select_ddc_port(radeon_connector);
ret = i2c_transfer(&radeon_connector->ddc_bus->adapter, msgs, 2);
- if (ret != 2)
+ if (ret != 2) {
/* Couldn't find an accessible DDC on this connector */
+ radeon_connector->broken_edid_header_counter = 0;
return false;
- /* Probe also for valid EDID header
+ }
+ /* DDC is available; probe also for valid EDID header
* EDID header starts with:
* 0x00,0xFF,0xFF,0xFF,0xFF,0xFF,0xFF,0x00.
* Only the first 6 bytes must be valid as
@@ -70,6 +72,7 @@ bool radeon_ddc_probe(struct radeon_connector *radeon_connector)
if (drm_edid_header_is_valid(buf) < 6) {
/* Couldn't find an accessible EDID on this
* connector */
+ radeon_connector->broken_edid_header_counter++;
return false;
}
return true;
diff --git a/drivers/gpu/drm/radeon/radeon_mode.h b/drivers/gpu/drm/radeon/radeon_mode.h
index 2c2e75e..4af4583 100644
--- a/drivers/gpu/drm/radeon/radeon_mode.h
+++ b/drivers/gpu/drm/radeon/radeon_mode.h
@@ -442,6 +442,9 @@ struct radeon_connector {
/* we need to mind the EDID between detect
and get modes due to analog/digital/tvencoder */
struct edid *edid;
+ /* extended DDC probing is now default for radeon connectors
+ count the number of failed EDID header probes */
+ int broken_edid_header_counter;
void *con_priv;
bool dac_load_detect;
bool detected_by_load; /* if the connection status was determined by load */
--
1.7.1
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH 0/2] drm_radeon_kms: Some Regression Fixes for Extended DDC Probe
2011-11-28 16:20 [PATCH 0/2] drm_radeon_kms: Some Regression Fixes for Extended DDC Probe Thomas Reim
2011-11-28 16:20 ` [PATCH 1/2] drm: Improve detection of floating connectors Thomas Reim
2011-11-28 16:20 ` [PATCH 2/2] drm/radeon/kms: wrap-up handling of floating connectors and connector unavailability status logging Thomas Reim
@ 2011-11-28 16:34 ` Thomas Reim
2 siblings, 0 replies; 10+ messages in thread
From: Thomas Reim @ 2011-11-28 16:34 UTC (permalink / raw)
To: Dave Airlie, Alex Deucher; +Cc: dri-devel
Here are the logs without and with drm debug option set:
[0.000000] Initializing cgroup subsys cpuset
[0.000000] Initializing cgroup subsys cpu
[0.000000] Linux version 2.6.35-31-generic (buildd@pluot) (gcc version
4.4.5 (Ubuntu/Linaro 4.4.4-14ubuntu5) ) #62+lp722806thor4-Ubuntu SMP Fri
Nov 25 11:50:56 UTC 2011 (Ubuntu 2.6.35-31.62+lp722806thor4-generic
2.6.35.13)
[0.000000] Command line: BOOT_IMAGE=/vmlinuz-2.6.35-31-generic
root=UUID=bd9acbc2-0ada-404f-9864-158f93de9ed5 ro
crashkernel=384M-2G:64M,2G-:128M quiet splash
...
[8.459216] [drm] Radeon Display Connectors
[8.459218] [drm] Connector 0:
[8.459220] [drm] VGA
[8.459223] [drm] DDC: 0x7e50 0x7e40 0x7e54 0x7e44 0x7e58 0x7e48 0x7e5c
0x7e4c
[8.459225] [drm] Encoders:
[8.459227] [drm] CRT1: INTERNAL_KLDSCP_DAC1
[8.459229] [drm] Connector 1:
[8.459231] [drm] S-video
[8.459232] [drm] Encoders:
[8.459234] [drm] TV1: INTERNAL_KLDSCP_DAC1
[8.459236] [drm] Connector 2:
[8.459238] [drm] HDMI-A
[8.459239] [drm] HPD2
[8.459242] [drm] DDC: 0x7e40 0x7e60 0x7e44 0x7e64 0x7e48 0x7e68 0x7e4c
0x7e6c
[8.459244] [drm] Encoders:
[8.459246] [drm] DFP2: INTERNAL_DDI
[8.459248] [drm] Connector 3:
[8.459249] [drm] DVI-D
[8.459252] [drm] DDC: 0x7e40 0x7e50 0x7e44 0x7e54 0x7e48 0x7e58 0x7e4c
0x7e5c
[8.459254] [drm] Encoders:
[8.459256] [drm] DFP3: INTERNAL_LVTM1
[8.593100] i2c /dev entries driver
[8.721280] Raw EDID:
[8.721324] <3>00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
00 ................
[8.721327] <3>00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
00 ................
[8.721330] <3>00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
00 ................
[8.721332] <3>00 00 00 00 00 00 00 00 07 00 00 00 00 00 00
00 ................
[8.721335] <3>00 00 00 1f 03 00 00 00 00 07 00 00 00 01 00
00 ................
[8.721338] <3>00 00 0f 00 00 00 00 00 00 00 00 00 00 00 00
00 ................
[8.721340] <3>00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
00 ................
[8.721343] <3>00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
00 ................
[8.721345]
[8.721383] radeon 0000:01:05.0: HDMI-A-1: EDID block 0 invalid.
[8.721387] [drm:radeon_dvi_detect] HDMI-A-1: probed a monitor, DDC
responded but no|invalid EDID
[8.721428] [drm:radeon_dvi_detect] HDMI-A-1: detected RS690 floating bus
bug, stopping ddc detect
[8.871708] [drm] fb mappable at 0xF0040000
[8.871713] [drm] vram apper at 0xF0000000
[8.871715] [drm] size 5242880
[8.871717] [drm] fb depth is 24
[8.871719] [drm] pitch is 5120
[8.912957] Console: switching to colour frame buffer device 160x64
[8.925225] fb0: radeondrmfb frame buffer device
[8.925228] drm: registered panic notifier
[8.964597] [drm] Initialized radeon 2.5.0 20080528 for 0000:01:05.0 on
minor 0
[0.000000] Initializing cgroup subsys cpuset
[0.000000] Initializing cgroup subsys cpu
[0.000000] Linux version 2.6.35-31-generic (buildd@pluot) (gcc version
4.4.5 (Ubuntu/Linaro 4.4.4-14ubuntu5) ) #62+lp722806thor4-Ubuntu SMP Fri
Nov 25 11:50:56 UTC 2011 (Ubuntu 2.6.35-31.62+lp722806thor4-generic
2.6.35.13)
[0.000000] Command line: BOOT_IMAGE=/vmlinuz-2.6.35-31-generic
root=UUID=bd9acbc2-0ada-404f-9864-158f93de9ed5 ro
crashkernel=384M-2G:64M,2G-:128M quiet splash drm.debug=14
log_buf_len=16M
...
[8.320490] [drm] Radeon Display Connectors
[8.320492] [drm] Connector 0:
[8.320494] [drm] VGA
[8.320498] [drm] DDC: 0x7e50 0x7e40 0x7e54 0x7e44 0x7e58 0x7e48 0x7e5c
0x7e4c
[8.320500] [drm] Encoders:
[8.320502] [drm] CRT1: INTERNAL_KLDSCP_DAC1
[8.320504] [drm] Connector 1:
[8.320506] [drm] S-video
[8.320508] [drm] Encoders:
[8.320510] [drm] TV1: INTERNAL_KLDSCP_DAC1
[8.320512] [drm] Connector 2:
[8.320514] [drm] HDMI-A
[8.320515] [drm] HPD2
[8.320518] [drm] DDC: 0x7e40 0x7e60 0x7e44 0x7e64 0x7e48 0x7e68 0x7e4c
0x7e6c
[8.320521] [drm] Encoders:
[8.320523] [drm] DFP2: INTERNAL_DDI
[8.320525] [drm] Connector 3:
[8.320526] [drm] DVI-D
[8.320529] [drm] DDC: 0x7e40 0x7e50 0x7e44 0x7e54 0x7e48 0x7e58 0x7e4c
0x7e5c
[8.320532] [drm] Encoders:
[8.320534] [drm] DFP3: INTERNAL_LVTM1
[8.320598] [drm:drm_helper_probe_single_connector_modes], VGA-1
[8.410041] [drm:drm_helper_probe_single_connector_modes], VGA-1 is
disconnected
[8.410049] [drm:drm_helper_probe_single_connector_modes], SVIDEO-1
[8.479510] i2c /dev entries driver
[8.500081] [drm:drm_helper_probe_single_connector_modes], SVIDEO-1 is
disconnected
[8.500090] [drm:drm_helper_probe_single_connector_modes], HDMI-A-1
[8.563878] Raw EDID:
[8.563922] <3>00 00 07 00 00 00 00 00 00 00 00 00 00 00 00
00 ................
[8.563926] <3>00 00 ff 00 00 00 00 00 00 00 00 00 00 00 00
00 ................
[8.563929] <3>00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
00 ................
[8.563932] <3>00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
00 ................
[8.563934] <3>00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
00 ................
[8.563937] <3>00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
00 ................
[8.563940] <3>00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
00 ................
[8.563943] <3>00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
00 ................
[8.563945]
[8.563983] radeon 0000:01:05.0: HDMI-A-1: EDID block 0 invalid.
[8.563987] [drm:radeon_dvi_detect] HDMI-A-1: probed a monitor, DDC
responded but no|invalid EDID
[8.564028] [drm:radeon_dvi_detect] HDMI-A-1: detected RS690 floating bus
bug, stopping ddc detect
[8.564074] [drm:drm_helper_probe_single_connector_modes], HDMI-A-1 is
disconnected
[8.564080] [drm:drm_helper_probe_single_connector_modes], DVI-D-1
[8.624077] [drm:drm_helper_probe_single_connector_modes], Probed modes
for DVI-D-1
[8.624083] [drm:drm_mode_debug_printmodeline], Modeline 17:"1280x1024"
60 108000 1280 1328 1440 1688 1024 1025 1028 1066 0x48 0x5
...
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 2/2] drm/radeon/kms: wrap-up handling of floating connectors and connector unavailability status logging
2011-11-28 16:20 ` [PATCH 2/2] drm/radeon/kms: wrap-up handling of floating connectors and connector unavailability status logging Thomas Reim
@ 2011-11-29 14:50 ` Alex Deucher
2011-11-29 18:06 ` Thomas Reim
2011-11-29 18:33 ` Thomas Reim
1 sibling, 1 reply; 10+ messages in thread
From: Alex Deucher @ 2011-11-29 14:50 UTC (permalink / raw)
To: Thomas Reim; +Cc: Dave Airlie, dri-devel, Thomas Reim
On Mon, Nov 28, 2011 at 11:20 AM, Thomas Reim <reimth@googlemail.com> wrote:
> Extended DDC probe is now default for RADEON chipsets. In case of
> HW bugs (e. g. floating connectors), the affected connectors will
> not be used, as a valid EDID header can not be detected. Another
> patch removed DDC detection and connector status logging during
> device setup. So the user is not informed anymore about HW bugs
> leading to connectors being unavailable.
> Reintroduce one-time logging of connector unavailability status
> when probing (single) connector modes during framebuffer
> initialisation.
>
> For RS690 chipsets DDC detection shall be stopped, if the i2c bus
> receives all-0 EDIDs (floating connectors). The introduction of
> extended DDC probing prevents the driver from doing this. Correctly
> relocate the related code.
Is any of this needed anymore now that we do an extended probe? I
think the original null edid patch was just papering over the actual
issue which was the need for a full edid header probe.
>
> Signed-off-by: Thomas Reim <reimth@gmail.com>
> ---
> drivers/gpu/drm/radeon/radeon_connectors.c | 54 +++++++++++++++++++++++-----
> drivers/gpu/drm/radeon/radeon_i2c.c | 7 +++-
> drivers/gpu/drm/radeon/radeon_mode.h | 3 ++
> 3 files changed, 53 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/gpu/drm/radeon/radeon_connectors.c b/drivers/gpu/drm/radeon/radeon_connectors.c
> index e7cb3ab..a4c0f59 100644
> --- a/drivers/gpu/drm/radeon/radeon_connectors.c
> +++ b/drivers/gpu/drm/radeon/radeon_connectors.c
> @@ -662,6 +662,7 @@ radeon_vga_detect(struct drm_connector *connector, bool force)
> struct radeon_connector *radeon_connector = to_radeon_connector(connector);
> struct drm_encoder *encoder;
> struct drm_encoder_helper_funcs *encoder_funcs;
> + struct edid *edid = NULL;
> bool dret = false;
> enum drm_connector_status ret = connector_status_disconnected;
>
> @@ -669,8 +670,24 @@ radeon_vga_detect(struct drm_connector *connector, bool force)
> if (!encoder)
> ret = connector_status_disconnected;
>
> - if (radeon_connector->ddc_bus)
> + if (radeon_connector->ddc_bus) {
> dret = radeon_ddc_probe(radeon_connector);
> + if (!dret && radeon_connector->broken_edid_header_counter == 1) {
> + /* Found a broken DDC when probing (single) connector
> + modes during framebuffer initialisation.
> + Clean-up and log the HW bug. */
> + edid = drm_get_edid(&radeon_connector->base, &radeon_connector->ddc_bus->adapter);
> + if (!edid) {
> + radeon_connector->broken_edid_header_counter++;
> + DRM_INFO("%s: probed a monitor, DDC responded but no|invalid EDID\n",
> + drm_get_connector_name(connector));
> + } else {
> + /* We should not get here, as the EDID header is broken */
> + connector->display_info.raw_edid = NULL;
> + kfree(edid);
> + }
> + }
> + }
What is this hunk for? It's not related to the problematic DDIA
interface which is digital only. It seems like it will just spam the
log for VGA monitors with faulty EDIDs.
> if (dret) {
> radeon_connector->detected_by_load = false;
> if (radeon_connector->edid) {
> @@ -847,12 +864,38 @@ radeon_dvi_detect(struct drm_connector *connector, bool force)
> struct drm_encoder *encoder = NULL;
> struct drm_encoder_helper_funcs *encoder_funcs;
> struct drm_mode_object *obj;
> + struct edid *edid = NULL;
> int i;
> enum drm_connector_status ret = connector_status_disconnected;
> bool dret = false;
>
> - if (radeon_connector->ddc_bus)
> + if (radeon_connector->ddc_bus) {
> dret = radeon_ddc_probe(radeon_connector);
> + if (!dret && radeon_connector->broken_edid_header_counter == 1) {
> + /* Found a broken DDC when probing (single) connector
> + modes during framebuffer initialisation.
> + Clean-up and log the HW bug. */
> + edid = drm_get_edid(&radeon_connector->base, &radeon_connector->ddc_bus->adapter);
> + if (!edid) {
> + radeon_connector->broken_edid_header_counter++;
> + DRM_INFO("%s: probed a monitor, DDC responded but no|invalid EDID\n",
> + drm_get_connector_name(connector));
> + /* rs690 seems to have a problem with connectors not existing and return
> + * random EDIDs mainly with blocks of 0's. If we see this, just stop
> + * polling on this output */
> + if ((rdev->family == CHIP_RS690 || rdev->family == CHIP_RS740)) {
> + ret = connector_status_disconnected;
> + DRM_INFO("%s: detected RS690 floating bus bug, stopping ddc detect\n",
> + drm_get_connector_name(connector));
> + radeon_connector->ddc_bus = NULL;
> + }
> + } else {
> + /* We should not get here, as the EDID header is broken */
> + connector->display_info.raw_edid = NULL;
> + kfree(edid);
> + }
> + }
> + }
Is this really needed? What is it preventing? Doesn't the full EDID
header probe alleviate the need for this? My concern is that we will
remove the ddc bus on connectors that are valid just because the user
probed with no monitor connected or a monitor with a faulty edid.
Then all future probes will fail to get an EDID since the bus is gone.
> if (dret) {
> radeon_connector->detected_by_load = false;
> if (radeon_connector->edid) {
> @@ -864,13 +907,6 @@ radeon_dvi_detect(struct drm_connector *connector, bool force)
> if (!radeon_connector->edid) {
> DRM_ERROR("%s: probed a monitor but no|invalid EDID\n",
> drm_get_connector_name(connector));
> - /* rs690 seems to have a problem with connectors not existing and always
> - * return a block of 0's. If we see this just stop polling on this output */
> - if ((rdev->family == CHIP_RS690 || rdev->family == CHIP_RS740) && radeon_connector->base.null_edid_counter) {
> - ret = connector_status_disconnected;
> - DRM_ERROR("%s: detected RS690 floating bus bug, stopping ddc detect\n", drm_get_connector_name(connector));
> - radeon_connector->ddc_bus = NULL;
> - }
This part can probably be removed now.
> } else {
> radeon_connector->use_digital = !!(radeon_connector->edid->input & DRM_EDID_INPUT_DIGITAL);
>
> diff --git a/drivers/gpu/drm/radeon/radeon_i2c.c b/drivers/gpu/drm/radeon/radeon_i2c.c
> index 7bb1b07..ac6aa02 100644
> --- a/drivers/gpu/drm/radeon/radeon_i2c.c
> +++ b/drivers/gpu/drm/radeon/radeon_i2c.c
> @@ -59,10 +59,12 @@ bool radeon_ddc_probe(struct radeon_connector *radeon_connector)
> radeon_router_select_ddc_port(radeon_connector);
>
> ret = i2c_transfer(&radeon_connector->ddc_bus->adapter, msgs, 2);
> - if (ret != 2)
> + if (ret != 2) {
> /* Couldn't find an accessible DDC on this connector */
> + radeon_connector->broken_edid_header_counter = 0;
> return false;
> - /* Probe also for valid EDID header
> + }
> + /* DDC is available; probe also for valid EDID header
> * EDID header starts with:
> * 0x00,0xFF,0xFF,0xFF,0xFF,0xFF,0xFF,0x00.
> * Only the first 6 bytes must be valid as
> @@ -70,6 +72,7 @@ bool radeon_ddc_probe(struct radeon_connector *radeon_connector)
> if (drm_edid_header_is_valid(buf) < 6) {
> /* Couldn't find an accessible EDID on this
> * connector */
> + radeon_connector->broken_edid_header_counter++;
> return false;
> }
> return true;
> diff --git a/drivers/gpu/drm/radeon/radeon_mode.h b/drivers/gpu/drm/radeon/radeon_mode.h
> index 2c2e75e..4af4583 100644
> --- a/drivers/gpu/drm/radeon/radeon_mode.h
> +++ b/drivers/gpu/drm/radeon/radeon_mode.h
> @@ -442,6 +442,9 @@ struct radeon_connector {
> /* we need to mind the EDID between detect
> and get modes due to analog/digital/tvencoder */
> struct edid *edid;
> + /* extended DDC probing is now default for radeon connectors
> + count the number of failed EDID header probes */
> + int broken_edid_header_counter;
> void *con_priv;
> bool dac_load_detect;
> bool detected_by_load; /* if the connection status was determined by load */
> --
> 1.7.1
>
>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 2/2] drm/radeon/kms: wrap-up handling of floating connectors and connector unavailability status logging
2011-11-29 14:50 ` Alex Deucher
@ 2011-11-29 18:06 ` Thomas Reim
2011-11-29 20:53 ` Alex Deucher
0 siblings, 1 reply; 10+ messages in thread
From: Thomas Reim @ 2011-11-29 18:06 UTC (permalink / raw)
To: Alex Deucher; +Cc: Dave Airlie, dri-devel
Dear Alex,
> On Mon, Nov 28, 2011 at 11:20 AM, Thomas Reim <reimth@googlemail.com> wrote:
> > Extended DDC probe is now default for RADEON chipsets. In case of
> > HW bugs (e. g. floating connectors), the affected connectors will
> > not be used, as a valid EDID header can not be detected. Another
> > patch removed DDC detection and connector status logging during
> > device setup. So the user is not informed anymore about HW bugs
> > leading to connectors being unavailable.
> > Reintroduce one-time logging of connector unavailability status
> > when probing (single) connector modes during framebuffer
> > initialisation.
> >
> > For RS690 chipsets DDC detection shall be stopped, if the i2c bus
> > receives all-0 EDIDs (floating connectors). The introduction of
> > extended DDC probing prevents the driver from doing this. Correctly
> > relocate the related code.
>
> Is any of this needed anymore now that we do an extended probe? I
> think the original null edid patch was just papering over the actual
> issue which was the need for a full edid header probe.
The difference, at least this is my understanding of Dave's patch, is
that we completely stop DDC detection for the famous RS690/RS740 chips
with missing or disabled HDMI add-on cards. We decrease latency, as we
do not retrieve data from i2c bus anymore. Therefore, I would keep it.
>
> >
> > Signed-off-by: Thomas Reim <reimth@gmail.com>
> > ---
> > drivers/gpu/drm/radeon/radeon_connectors.c | 54 +++++++++++++++++++++++-----
> > drivers/gpu/drm/radeon/radeon_i2c.c | 7 +++-
> > drivers/gpu/drm/radeon/radeon_mode.h | 3 ++
> > 3 files changed, 53 insertions(+), 11 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/radeon/radeon_connectors.c b/drivers/gpu/drm/radeon/radeon_connectors.c
> > index e7cb3ab..a4c0f59 100644
> > --- a/drivers/gpu/drm/radeon/radeon_connectors.c
> > +++ b/drivers/gpu/drm/radeon/radeon_connectors.c
> > @@ -662,6 +662,7 @@ radeon_vga_detect(struct drm_connector *connector, bool force)
> > struct radeon_connector *radeon_connector = to_radeon_connector(connector);
> > struct drm_encoder *encoder;
> > struct drm_encoder_helper_funcs *encoder_funcs;
> > + struct edid *edid = NULL;
> > bool dret = false;
> > enum drm_connector_status ret = connector_status_disconnected;
> >
> > @@ -669,8 +670,24 @@ radeon_vga_detect(struct drm_connector *connector, bool force)
> > if (!encoder)
> > ret = connector_status_disconnected;
> >
> > - if (radeon_connector->ddc_bus)
> > + if (radeon_connector->ddc_bus) {
> > dret = radeon_ddc_probe(radeon_connector);
> > + if (!dret && radeon_connector->broken_edid_header_counter == 1) {
> > + /* Found a broken DDC when probing (single) connector
> > + modes during framebuffer initialisation.
> > + Clean-up and log the HW bug. */
> > + edid = drm_get_edid(&radeon_connector->base, &radeon_connector->ddc_bus->adapter);
> > + if (!edid) {
> > + radeon_connector->broken_edid_header_counter++;
> > + DRM_INFO("%s: probed a monitor, DDC responded but no|invalid EDID\n",
> > + drm_get_connector_name(connector));
> > + } else {
> > + /* We should not get here, as the EDID header is broken */
> > + connector->display_info.raw_edid = NULL;
> > + kfree(edid);
> > + }
> > + }
> > + }
>
> What is this hunk for? It's not related to the problematic DDIA
> interface which is digital only. It seems like it will just spam the
> log for VGA monitors with faulty EDIDs.
The broken_edid_header_counter prevents from spamming. Only the first
time, the extended DDC probe fails due to a broken EDID header (which
may happen also to VGA connectors with a buggy monitor connected; at
least I remember to have seen such dmesg logs in the past) we would
inform users. As the EDID is broken, the screen will stay dark, but
users would have at least the required information in the logs. They can
post them, once they've connected a working monitor.
My ASUS RS690 system suffered from such log spamming. So please believe
me, that I would never ask for a solution that leads us back to this.
>
> > if (dret) {
> > radeon_connector->detected_by_load = false;
> > if (radeon_connector->edid) {
> > @@ -847,12 +864,38 @@ radeon_dvi_detect(struct drm_connector *connector, bool force)
> > struct drm_encoder *encoder = NULL;
> > struct drm_encoder_helper_funcs *encoder_funcs;
> > struct drm_mode_object *obj;
> > + struct edid *edid = NULL;
> > int i;
> > enum drm_connector_status ret = connector_status_disconnected;
> > bool dret = false;
> >
> > - if (radeon_connector->ddc_bus)
> > + if (radeon_connector->ddc_bus) {
> > dret = radeon_ddc_probe(radeon_connector);
> > + if (!dret && radeon_connector->broken_edid_header_counter == 1) {
> > + /* Found a broken DDC when probing (single) connector
> > + modes during framebuffer initialisation.
> > + Clean-up and log the HW bug. */
> > + edid = drm_get_edid(&radeon_connector->base, &radeon_connector->ddc_bus->adapter);
> > + if (!edid) {
> > + radeon_connector->broken_edid_header_counter++;
> > + DRM_INFO("%s: probed a monitor, DDC responded but no|invalid EDID\n",
> > + drm_get_connector_name(connector));
> > + /* rs690 seems to have a problem with connectors not existing and return
> > + * random EDIDs mainly with blocks of 0's. If we see this, just stop
> > + * polling on this output */
> > + if ((rdev->family == CHIP_RS690 || rdev->family == CHIP_RS740)) {
> > + ret = connector_status_disconnected;
> > + DRM_INFO("%s: detected RS690 floating bus bug, stopping ddc detect\n",
> > + drm_get_connector_name(connector));
> > + radeon_connector->ddc_bus = NULL;
> > + }
> > + } else {
> > + /* We should not get here, as the EDID header is broken */
> > + connector->display_info.raw_edid = NULL;
> > + kfree(edid);
> > + }
> > + }
> > + }
>
> Is this really needed? What is it preventing? Doesn't the full EDID
> header probe alleviate the need for this? My concern is that we will
> remove the ddc bus on connectors that are valid just because the user
> probed with no monitor connected or a monitor with a faulty edid.
> Then all future probes will fail to get an EDID since the bus is gone.
You're right. This is a bug! Somehow I lost the third if condition: &&
radeon_connector->base.null_edid_counter when updating the patches.
Thanks for checking the code. I'll post the corrected one after sending
this mail.
>
> > if (dret) {
> > radeon_connector->detected_by_load = false;
> > if (radeon_connector->edid) {
> > @@ -864,13 +907,6 @@ radeon_dvi_detect(struct drm_connector *connector, bool force)
> > if (!radeon_connector->edid) {
> > DRM_ERROR("%s: probed a monitor but no|invalid EDID\n",
> > drm_get_connector_name(connector));
> > - /* rs690 seems to have a problem with connectors not existing and always
> > - * return a block of 0's. If we see this just stop polling on this output */
> > - if ((rdev->family == CHIP_RS690 || rdev->family == CHIP_RS740) && radeon_connector->base.null_edid_counter) {
> > - ret = connector_status_disconnected;
> > - DRM_ERROR("%s: detected RS690 floating bus bug, stopping ddc detect\n", drm_get_connector_name(connector));
> > - radeon_connector->ddc_bus = NULL;
> > - }
>
> This part can probably be removed now.
>
> > } else {
> > radeon_connector->use_digital = !!(radeon_connector->edid->input & DRM_EDID_INPUT_DIGITAL);
> >
> > diff --git a/drivers/gpu/drm/radeon/radeon_i2c.c b/drivers/gpu/drm/radeon/radeon_i2c.c
> > index 7bb1b07..ac6aa02 100644
> > --- a/drivers/gpu/drm/radeon/radeon_i2c.c
> > +++ b/drivers/gpu/drm/radeon/radeon_i2c.c
> > @@ -59,10 +59,12 @@ bool radeon_ddc_probe(struct radeon_connector *radeon_connector)
> > radeon_router_select_ddc_port(radeon_connector);
> >
> > ret = i2c_transfer(&radeon_connector->ddc_bus->adapter, msgs, 2);
> > - if (ret != 2)
> > + if (ret != 2) {
> > /* Couldn't find an accessible DDC on this connector */
> > + radeon_connector->broken_edid_header_counter = 0;
> > return false;
> > - /* Probe also for valid EDID header
> > + }
> > + /* DDC is available; probe also for valid EDID header
> > * EDID header starts with:
> > * 0x00,0xFF,0xFF,0xFF,0xFF,0xFF,0xFF,0x00.
> > * Only the first 6 bytes must be valid as
> > @@ -70,6 +72,7 @@ bool radeon_ddc_probe(struct radeon_connector *radeon_connector)
> > if (drm_edid_header_is_valid(buf) < 6) {
> > /* Couldn't find an accessible EDID on this
> > * connector */
> > + radeon_connector->broken_edid_header_counter++;
> > return false;
> > }
> > return true;
> > diff --git a/drivers/gpu/drm/radeon/radeon_mode.h b/drivers/gpu/drm/radeon/radeon_mode.h
> > index 2c2e75e..4af4583 100644
> > --- a/drivers/gpu/drm/radeon/radeon_mode.h
> > +++ b/drivers/gpu/drm/radeon/radeon_mode.h
> > @@ -442,6 +442,9 @@ struct radeon_connector {
> > /* we need to mind the EDID between detect
> > and get modes due to analog/digital/tvencoder */
> > struct edid *edid;
> > + /* extended DDC probing is now default for radeon connectors
> > + count the number of failed EDID header probes */
> > + int broken_edid_header_counter;
> > void *con_priv;
> > bool dac_load_detect;
> > bool detected_by_load; /* if the connection status was determined by load */
> > --
> > 1.7.1
> >
> >
Best regards
Thomas
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH 2/2] drm/radeon/kms: wrap-up handling of floating connectors and connector unavailability status logging
2011-11-28 16:20 ` [PATCH 2/2] drm/radeon/kms: wrap-up handling of floating connectors and connector unavailability status logging Thomas Reim
2011-11-29 14:50 ` Alex Deucher
@ 2011-11-29 18:33 ` Thomas Reim
1 sibling, 0 replies; 10+ messages in thread
From: Thomas Reim @ 2011-11-29 18:33 UTC (permalink / raw)
To: Alex Deucher; +Cc: Dave Airlie, dri-devel, Thomas Reim
Extended DDC probe is now default for RADEON chipsets. In case of
HW bugs (e. g. floating connectors), the affected connectors will
not be used, as a valid EDID header can not be detected. Another
patch removed DDC detection and connector status logging during
device setup. So the user is not informed anymore about HW bugs
leading to connectors being unavailable.
Reintroduce one-time logging of connector unavailability status
when probing (single) connector modes during framebuffer
initialisation.
For RS690 chipsets DDC detection shall be stopped, if the i2c bus
receives all-0 EDIDs (floating connectors). The introduction of
extended DDC probing prevents the driver from doing this. Correctly
relocate the related code.
Signed-off-by: Thomas Reim <reimth@gmail.com>
---
drivers/gpu/drm/radeon/radeon_connectors.c | 54 +++++++++++++++++++++++-----
drivers/gpu/drm/radeon/radeon_i2c.c | 7 +++-
drivers/gpu/drm/radeon/radeon_mode.h | 3 ++
3 files changed, 53 insertions(+), 11 deletions(-)
diff --git a/drivers/gpu/drm/radeon/radeon_connectors.c b/drivers/gpu/drm/radeon/radeon_connectors.c
index e7cb3ab..b06480c 100644
--- a/drivers/gpu/drm/radeon/radeon_connectors.c
+++ b/drivers/gpu/drm/radeon/radeon_connectors.c
@@ -662,6 +662,7 @@ radeon_vga_detect(struct drm_connector *connector, bool force)
struct radeon_connector *radeon_connector = to_radeon_connector(connector);
struct drm_encoder *encoder;
struct drm_encoder_helper_funcs *encoder_funcs;
+ struct edid *edid = NULL;
bool dret = false;
enum drm_connector_status ret = connector_status_disconnected;
@@ -669,8 +670,24 @@ radeon_vga_detect(struct drm_connector *connector, bool force)
if (!encoder)
ret = connector_status_disconnected;
- if (radeon_connector->ddc_bus)
+ if (radeon_connector->ddc_bus) {
dret = radeon_ddc_probe(radeon_connector);
+ if (!dret && radeon_connector->broken_edid_header_counter == 1) {
+ /* Found a broken DDC when probing (single) connector
+ modes during framebuffer initialisation.
+ Clean-up and log the HW bug. */
+ edid = drm_get_edid(&radeon_connector->base, &radeon_connector->ddc_bus->adapter);
+ if (!edid) {
+ radeon_connector->broken_edid_header_counter++;
+ DRM_INFO("%s: probed a monitor, DDC responded but no|invalid EDID\n",
+ drm_get_connector_name(connector));
+ } else {
+ /* We should not get here, as the EDID header is broken */
+ connector->display_info.raw_edid = NULL;
+ kfree(edid);
+ }
+ }
+ }
if (dret) {
radeon_connector->detected_by_load = false;
if (radeon_connector->edid) {
@@ -847,12 +864,38 @@ radeon_dvi_detect(struct drm_connector *connector, bool force)
struct drm_encoder *encoder = NULL;
struct drm_encoder_helper_funcs *encoder_funcs;
struct drm_mode_object *obj;
+ struct edid *edid = NULL;
int i;
enum drm_connector_status ret = connector_status_disconnected;
bool dret = false;
- if (radeon_connector->ddc_bus)
+ if (radeon_connector->ddc_bus) {
dret = radeon_ddc_probe(radeon_connector);
+ if (!dret && radeon_connector->broken_edid_header_counter == 1) {
+ /* Found a broken DDC when probing (single) connector
+ modes during framebuffer initialisation.
+ Clean-up and log the HW bug. */
+ edid = drm_get_edid(&radeon_connector->base, &radeon_connector->ddc_bus->adapter);
+ if (!edid) {
+ radeon_connector->broken_edid_header_counter++;
+ DRM_INFO("%s: probed a monitor, DDC responded but no|invalid EDID\n",
+ drm_get_connector_name(connector));
+ /* rs690 seems to have a problem with connectors not existing and return
+ * random EDIDs mainly with blocks of 0's. If we see this, just stop
+ * polling on this output */
+ if ((rdev->family == CHIP_RS690 || rdev->family == CHIP_RS740) && radeon_connector->base.null_edid_counter) {
+ ret = connector_status_disconnected;
+ DRM_INFO("%s: detected RS690 floating bus bug, stopping ddc detect\n",
+ drm_get_connector_name(connector));
+ radeon_connector->ddc_bus = NULL;
+ }
+ } else {
+ /* We should not get here, as the EDID header is broken */
+ connector->display_info.raw_edid = NULL;
+ kfree(edid);
+ }
+ }
+ }
if (dret) {
radeon_connector->detected_by_load = false;
if (radeon_connector->edid) {
@@ -864,13 +907,6 @@ radeon_dvi_detect(struct drm_connector *connector, bool force)
if (!radeon_connector->edid) {
DRM_ERROR("%s: probed a monitor but no|invalid EDID\n",
drm_get_connector_name(connector));
- /* rs690 seems to have a problem with connectors not existing and always
- * return a block of 0's. If we see this just stop polling on this output */
- if ((rdev->family == CHIP_RS690 || rdev->family == CHIP_RS740) && radeon_connector->base.null_edid_counter) {
- ret = connector_status_disconnected;
- DRM_ERROR("%s: detected RS690 floating bus bug, stopping ddc detect\n", drm_get_connector_name(connector));
- radeon_connector->ddc_bus = NULL;
- }
} else {
radeon_connector->use_digital = !!(radeon_connector->edid->input & DRM_EDID_INPUT_DIGITAL);
diff --git a/drivers/gpu/drm/radeon/radeon_i2c.c b/drivers/gpu/drm/radeon/radeon_i2c.c
index 7bb1b07..ac6aa02 100644
--- a/drivers/gpu/drm/radeon/radeon_i2c.c
+++ b/drivers/gpu/drm/radeon/radeon_i2c.c
@@ -59,10 +59,12 @@ bool radeon_ddc_probe(struct radeon_connector *radeon_connector)
radeon_router_select_ddc_port(radeon_connector);
ret = i2c_transfer(&radeon_connector->ddc_bus->adapter, msgs, 2);
- if (ret != 2)
+ if (ret != 2) {
/* Couldn't find an accessible DDC on this connector */
+ radeon_connector->broken_edid_header_counter = 0;
return false;
- /* Probe also for valid EDID header
+ }
+ /* DDC is available; probe also for valid EDID header
* EDID header starts with:
* 0x00,0xFF,0xFF,0xFF,0xFF,0xFF,0xFF,0x00.
* Only the first 6 bytes must be valid as
@@ -70,6 +72,7 @@ bool radeon_ddc_probe(struct radeon_connector *radeon_connector)
if (drm_edid_header_is_valid(buf) < 6) {
/* Couldn't find an accessible EDID on this
* connector */
+ radeon_connector->broken_edid_header_counter++;
return false;
}
return true;
diff --git a/drivers/gpu/drm/radeon/radeon_mode.h b/drivers/gpu/drm/radeon/radeon_mode.h
index 2c2e75e..4af4583 100644
--- a/drivers/gpu/drm/radeon/radeon_mode.h
+++ b/drivers/gpu/drm/radeon/radeon_mode.h
@@ -442,6 +442,9 @@ struct radeon_connector {
/* we need to mind the EDID between detect
and get modes due to analog/digital/tvencoder */
struct edid *edid;
+ /* extended DDC probing is now default for radeon connectors
+ count the number of failed EDID header probes */
+ int broken_edid_header_counter;
void *con_priv;
bool dac_load_detect;
bool detected_by_load; /* if the connection status was determined by load */
--
1.7.1
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH 2/2] drm/radeon/kms: wrap-up handling of floating connectors and connector unavailability status logging
2011-11-29 18:06 ` Thomas Reim
@ 2011-11-29 20:53 ` Alex Deucher
2011-11-30 10:54 ` Thomas Reim
0 siblings, 1 reply; 10+ messages in thread
From: Alex Deucher @ 2011-11-29 20:53 UTC (permalink / raw)
To: Thomas Reim; +Cc: Dave Airlie, dri-devel
On Tue, Nov 29, 2011 at 1:06 PM, Thomas Reim <reimth@googlemail.com> wrote:
> Dear Alex,
>
>> On Mon, Nov 28, 2011 at 11:20 AM, Thomas Reim <reimth@googlemail.com> wrote:
>> > Extended DDC probe is now default for RADEON chipsets. In case of
>> > HW bugs (e. g. floating connectors), the affected connectors will
>> > not be used, as a valid EDID header can not be detected. Another
>> > patch removed DDC detection and connector status logging during
>> > device setup. So the user is not informed anymore about HW bugs
>> > leading to connectors being unavailable.
>> > Reintroduce one-time logging of connector unavailability status
>> > when probing (single) connector modes during framebuffer
>> > initialisation.
>> >
>> > For RS690 chipsets DDC detection shall be stopped, if the i2c bus
>> > receives all-0 EDIDs (floating connectors). The introduction of
>> > extended DDC probing prevents the driver from doing this. Correctly
>> > relocate the related code.
>>
>> Is any of this needed anymore now that we do an extended probe? I
>> think the original null edid patch was just papering over the actual
>> issue which was the need for a full edid header probe.
>
> The difference, at least this is my understanding of Dave's patch, is
> that we completely stop DDC detection for the famous RS690/RS740 chips
> with missing or disabled HDMI add-on cards. We decrease latency, as we
> do not retrieve data from i2c bus anymore. Therefore, I would keep it.
>
My only concern is that we may disable the i2c bus if there is no
monitor connected which would prevent edid fetching from working in
the future. Does the problematic ddia i2c line produce the same error
in the following cases:
1. ddia hdmi connector present, no monitor attached
2. ddia hdmi connector present, monitor with faulty edid attached
3. ddia hdmi connector absent
In the third case it makes sense to remove the i2c bus, but in the
first two, we may remove the i2c bus which would prevent future edid
fetches from working.
>>
>> >
>> > Signed-off-by: Thomas Reim <reimth@gmail.com>
>> > ---
>> > drivers/gpu/drm/radeon/radeon_connectors.c | 54 +++++++++++++++++++++++-----
>> > drivers/gpu/drm/radeon/radeon_i2c.c | 7 +++-
>> > drivers/gpu/drm/radeon/radeon_mode.h | 3 ++
>> > 3 files changed, 53 insertions(+), 11 deletions(-)
>> >
>> > diff --git a/drivers/gpu/drm/radeon/radeon_connectors.c b/drivers/gpu/drm/radeon/radeon_connectors.c
>> > index e7cb3ab..a4c0f59 100644
>> > --- a/drivers/gpu/drm/radeon/radeon_connectors.c
>> > +++ b/drivers/gpu/drm/radeon/radeon_connectors.c
>> > @@ -662,6 +662,7 @@ radeon_vga_detect(struct drm_connector *connector, bool force)
>> > struct radeon_connector *radeon_connector = to_radeon_connector(connector);
>> > struct drm_encoder *encoder;
>> > struct drm_encoder_helper_funcs *encoder_funcs;
>> > + struct edid *edid = NULL;
>> > bool dret = false;
>> > enum drm_connector_status ret = connector_status_disconnected;
>> >
>> > @@ -669,8 +670,24 @@ radeon_vga_detect(struct drm_connector *connector, bool force)
>> > if (!encoder)
>> > ret = connector_status_disconnected;
>> >
>> > - if (radeon_connector->ddc_bus)
>> > + if (radeon_connector->ddc_bus) {
>> > dret = radeon_ddc_probe(radeon_connector);
>> > + if (!dret && radeon_connector->broken_edid_header_counter == 1) {
>> > + /* Found a broken DDC when probing (single) connector
>> > + modes during framebuffer initialisation.
>> > + Clean-up and log the HW bug. */
>> > + edid = drm_get_edid(&radeon_connector->base, &radeon_connector->ddc_bus->adapter);
>> > + if (!edid) {
>> > + radeon_connector->broken_edid_header_counter++;
>> > + DRM_INFO("%s: probed a monitor, DDC responded but no|invalid EDID\n",
>> > + drm_get_connector_name(connector));
>> > + } else {
>> > + /* We should not get here, as the EDID header is broken */
>> > + connector->display_info.raw_edid = NULL;
>> > + kfree(edid);
>> > + }
>> > + }
>> > + }
>>
>> What is this hunk for? It's not related to the problematic DDIA
>> interface which is digital only. It seems like it will just spam the
>> log for VGA monitors with faulty EDIDs.
>
> The broken_edid_header_counter prevents from spamming. Only the first
> time, the extended DDC probe fails due to a broken EDID header (which
> may happen also to VGA connectors with a buggy monitor connected; at
> least I remember to have seen such dmesg logs in the past) we would
> inform users. As the EDID is broken, the screen will stay dark, but
> users would have at least the required information in the logs. They can
> post them, once they've connected a working monitor.
> My ASUS RS690 system suffered from such log spamming. So please believe
> me, that I would never ask for a solution that leads us back to this.
>
I think I would skip this part for now. radeon_vga_detect() gets
called anytime you request a probe of a vga connector. The full
header check in the radeon_ddc_probe should make this a moot point in
most cases. It's also inconsistent to have this behaviour for only
certain connectors.
>>
>> > if (dret) {
>> > radeon_connector->detected_by_load = false;
>> > if (radeon_connector->edid) {
>> > @@ -847,12 +864,38 @@ radeon_dvi_detect(struct drm_connector *connector, bool force)
>> > struct drm_encoder *encoder = NULL;
>> > struct drm_encoder_helper_funcs *encoder_funcs;
>> > struct drm_mode_object *obj;
>> > + struct edid *edid = NULL;
>> > int i;
>> > enum drm_connector_status ret = connector_status_disconnected;
>> > bool dret = false;
>> >
>> > - if (radeon_connector->ddc_bus)
>> > + if (radeon_connector->ddc_bus) {
>> > dret = radeon_ddc_probe(radeon_connector);
>> > + if (!dret && radeon_connector->broken_edid_header_counter == 1) {
>> > + /* Found a broken DDC when probing (single) connector
>> > + modes during framebuffer initialisation.
>> > + Clean-up and log the HW bug. */
>> > + edid = drm_get_edid(&radeon_connector->base, &radeon_connector->ddc_bus->adapter);
>> > + if (!edid) {
>> > + radeon_connector->broken_edid_header_counter++;
>> > + DRM_INFO("%s: probed a monitor, DDC responded but no|invalid EDID\n",
>> > + drm_get_connector_name(connector));
>> > + /* rs690 seems to have a problem with connectors not existing and return
>> > + * random EDIDs mainly with blocks of 0's. If we see this, just stop
>> > + * polling on this output */
>> > + if ((rdev->family == CHIP_RS690 || rdev->family == CHIP_RS740)) {
>> > + ret = connector_status_disconnected;
>> > + DRM_INFO("%s: detected RS690 floating bus bug, stopping ddc detect\n",
>> > + drm_get_connector_name(connector));
>> > + radeon_connector->ddc_bus = NULL;
>> > + }
>> > + } else {
>> > + /* We should not get here, as the EDID header is broken */
>> > + connector->display_info.raw_edid = NULL;
>> > + kfree(edid);
>> > + }
>> > + }
>> > + }
>>
>> Is this really needed? What is it preventing? Doesn't the full EDID
>> header probe alleviate the need for this? My concern is that we will
>> remove the ddc bus on connectors that are valid just because the user
>> probed with no monitor connected or a monitor with a faulty edid.
>> Then all future probes will fail to get an EDID since the bus is gone.
>
> You're right. This is a bug! Somehow I lost the third if condition: &&
> radeon_connector->base.null_edid_counter when updating the patches.
> Thanks for checking the code. I'll post the corrected one after sending
> this mail.
>
>>
>> > if (dret) {
>> > radeon_connector->detected_by_load = false;
>> > if (radeon_connector->edid) {
>> > @@ -864,13 +907,6 @@ radeon_dvi_detect(struct drm_connector *connector, bool force)
>> > if (!radeon_connector->edid) {
>> > DRM_ERROR("%s: probed a monitor but no|invalid EDID\n",
>> > drm_get_connector_name(connector));
>> > - /* rs690 seems to have a problem with connectors not existing and always
>> > - * return a block of 0's. If we see this just stop polling on this output */
>> > - if ((rdev->family == CHIP_RS690 || rdev->family == CHIP_RS740) && radeon_connector->base.null_edid_counter) {
>> > - ret = connector_status_disconnected;
>> > - DRM_ERROR("%s: detected RS690 floating bus bug, stopping ddc detect\n", drm_get_connector_name(connector));
>> > - radeon_connector->ddc_bus = NULL;
>> > - }
>>
>> This part can probably be removed now.
>>
>> > } else {
>> > radeon_connector->use_digital = !!(radeon_connector->edid->input & DRM_EDID_INPUT_DIGITAL);
>> >
>> > diff --git a/drivers/gpu/drm/radeon/radeon_i2c.c b/drivers/gpu/drm/radeon/radeon_i2c.c
>> > index 7bb1b07..ac6aa02 100644
>> > --- a/drivers/gpu/drm/radeon/radeon_i2c.c
>> > +++ b/drivers/gpu/drm/radeon/radeon_i2c.c
>> > @@ -59,10 +59,12 @@ bool radeon_ddc_probe(struct radeon_connector *radeon_connector)
>> > radeon_router_select_ddc_port(radeon_connector);
>> >
>> > ret = i2c_transfer(&radeon_connector->ddc_bus->adapter, msgs, 2);
>> > - if (ret != 2)
>> > + if (ret != 2) {
>> > /* Couldn't find an accessible DDC on this connector */
>> > + radeon_connector->broken_edid_header_counter = 0;
>> > return false;
>> > - /* Probe also for valid EDID header
>> > + }
>> > + /* DDC is available; probe also for valid EDID header
>> > * EDID header starts with:
>> > * 0x00,0xFF,0xFF,0xFF,0xFF,0xFF,0xFF,0x00.
>> > * Only the first 6 bytes must be valid as
>> > @@ -70,6 +72,7 @@ bool radeon_ddc_probe(struct radeon_connector *radeon_connector)
>> > if (drm_edid_header_is_valid(buf) < 6) {
>> > /* Couldn't find an accessible EDID on this
>> > * connector */
>> > + radeon_connector->broken_edid_header_counter++;
>> > return false;
>> > }
>> > return true;
>> > diff --git a/drivers/gpu/drm/radeon/radeon_mode.h b/drivers/gpu/drm/radeon/radeon_mode.h
>> > index 2c2e75e..4af4583 100644
>> > --- a/drivers/gpu/drm/radeon/radeon_mode.h
>> > +++ b/drivers/gpu/drm/radeon/radeon_mode.h
>> > @@ -442,6 +442,9 @@ struct radeon_connector {
>> > /* we need to mind the EDID between detect
>> > and get modes due to analog/digital/tvencoder */
>> > struct edid *edid;
>> > + /* extended DDC probing is now default for radeon connectors
>> > + count the number of failed EDID header probes */
>> > + int broken_edid_header_counter;
>> > void *con_priv;
>> > bool dac_load_detect;
>> > bool detected_by_load; /* if the connection status was determined by load */
>> > --
>> > 1.7.1
>> >
>> >
>
> Best regards
>
> Thomas
>
>
>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 2/2] drm/radeon/kms: wrap-up handling of floating connectors and connector unavailability status logging
2011-11-29 20:53 ` Alex Deucher
@ 2011-11-30 10:54 ` Thomas Reim
2011-12-01 14:10 ` Alex Deucher
0 siblings, 1 reply; 10+ messages in thread
From: Thomas Reim @ 2011-11-30 10:54 UTC (permalink / raw)
To: Alex Deucher; +Cc: Dave Airlie, dri-devel, Thomas Reim
Am Dienstag, den 29.11.2011, 15:53 -0500 schrieb Alex Deucher:
> On Tue, Nov 29, 2011 at 1:06 PM, Thomas Reim <reimth@googlemail.com> wrote:
> > Dear Alex,
> >
> >> On Mon, Nov 28, 2011 at 11:20 AM, Thomas Reim <reimth@googlemail.com> wrote:
> >> > Extended DDC probe is now default for RADEON chipsets. In case of
> >> > HW bugs (e. g. floating connectors), the affected connectors will
> >> > not be used, as a valid EDID header can not be detected. Another
> >> > patch removed DDC detection and connector status logging during
> >> > device setup. So the user is not informed anymore about HW bugs
> >> > leading to connectors being unavailable.
> >> > Reintroduce one-time logging of connector unavailability status
> >> > when probing (single) connector modes during framebuffer
> >> > initialisation.
> >> >
> >> > For RS690 chipsets DDC detection shall be stopped, if the i2c bus
> >> > receives all-0 EDIDs (floating connectors). The introduction of
> >> > extended DDC probing prevents the driver from doing this. Correctly
> >> > relocate the related code.
> >>
> >> Is any of this needed anymore now that we do an extended probe? I
> >> think the original null edid patch was just papering over the actual
> >> issue which was the need for a full edid header probe.
> >
> > The difference, at least this is my understanding of Dave's patch, is
> > that we completely stop DDC detection for the famous RS690/RS740 chips
> > with missing or disabled HDMI add-on cards. We decrease latency, as we
> > do not retrieve data from i2c bus anymore. Therefore, I would keep it.
> >
>
> My only concern is that we may disable the i2c bus if there is no
> monitor connected which would prevent edid fetching from working in
> the future. Does the problematic ddia i2c line produce the same error
> in the following cases:
>
The proposed solution differentiates between i2c bus responds, i. e. a
DDC is available, and EDID (header) is valid.
> 1. ddia hdmi connector present, no monitor attached
DDC is not available: The connector is regarded as being disconnected;
function drm_helper_probe_single_connector_modes() records a debug
message in the logs, i. e. "HDMI-A-1 is disconnected".
The connector will be probed again, when probing single connector modes
(using function drm_helper_probe_single_connector_modes() about every
second).
> 2. ddia hdmi connector present, monitor with faulty edid attached
>
DDC is available, invalid EDID (header): The connector is regarded as
being disconnected; If it's the first time, that this connector has been
probed, usually during framebuffer initialisation, dump the faulty EDID
to the logs and add an info message, that there is a problem with the
monitor: e. g. "HDMI-A-1: probed a monitor, DDC responded but no|invalid
EDID". In addition, the debug message of case 1 is logged.
Then we have two subcases:
a) General
The connector will be probed again, when probing single connector modes
(using function drm_helper_probe_single_connector_modes() about every
second). But there will be only the debug message logged. No info
message and no EDID dump.
b) EDID is all zero, i. e. not more than 8 non-zero bytes have been
received (RS690/RS740 chips only)
Floating connector is assumed and an info message is logged: "HDMI-A-1:
detected RS690 floating bus bug, stopping ddc detect". Such a connector
will not be probed again, reducing latency. I believe that this is
rather a theoretical case, as it requires a monitor with an erased
EEPROM.
> 3. ddia hdmi connector absent
a) Correctly implemented HW
would handle this in the same way as for case 1. The connector is part
of the chipset, i2c bus is correctly terminated and will never be used,
but being probed. This is a drawback of implmentations, that use a
chipset wich provides more connectors than being used by the
implementation.
b) Buggy HW
The connector's i2c has not been terminated correctly. It can't be used,
but is detected. The EDID information will mainly consist of zero bytes,
except of some random byte values.
DDC is available, invalid EDID (header): The connector is regarded as
being disconnected; If it's the first time, that this connector has been
probed, usually during framebuffer initialisation, dump the received information
of the not available EDID to the logs and add an info message, that
there is a problem with the connector: e. g. "HDMI-A-1: probed a
monitor, DDC responded but no|invalid EDID". In addition, the debug
message of the first case is logged.
Then we have two subcases:
(1) RS690/RS740 chips
Floating connector is assumed and an info message is logged: "HDMI-A-1:
detected RS690 floating bus bug, stopping ddc detect". Such a connector
will not be probed again, reducing latency.
(2) Other chipsets
The connector will be probed again, when probing single connector modes
(using function drm_helper_probe_single_connector_modes() about every
second). But there will be only a debug message (see case 1) logged. No
info message and EDID dump.
>
> In the third case it makes sense to remove the i2c bus, but in the
> first two, we may remove the i2c bus which would prevent future edid
> fetches from working.
The there is a 4th use case:
4. ddia hdmi connector present, monitor with faulty edid connected, then
removed and re-connected again
The Radeon driver will behave in the same way as in case 2. But each
time the faulty monitor is attached again to the connector, the info
message extended by a dump of the faulty EDID will be recorded once (!)
in the logs.
>
> >>
> >> >
> >> > Signed-off-by: Thomas Reim <reimth@gmail.com>
> >> > ---
> >> > drivers/gpu/drm/radeon/radeon_connectors.c | 54 +++++++++++++++++++++++-----
> >> > drivers/gpu/drm/radeon/radeon_i2c.c | 7 +++-
> >> > drivers/gpu/drm/radeon/radeon_mode.h | 3 ++
> >> > 3 files changed, 53 insertions(+), 11 deletions(-)
> >> >
> >> > diff --git a/drivers/gpu/drm/radeon/radeon_connectors.c b/drivers/gpu/drm/radeon/radeon_connectors.c
> >> > index e7cb3ab..a4c0f59 100644
> >> > --- a/drivers/gpu/drm/radeon/radeon_connectors.c
> >> > +++ b/drivers/gpu/drm/radeon/radeon_connectors.c
> >> > @@ -662,6 +662,7 @@ radeon_vga_detect(struct drm_connector *connector, bool force)
> >> > struct radeon_connector *radeon_connector = to_radeon_connector(connector);
> >> > struct drm_encoder *encoder;
> >> > struct drm_encoder_helper_funcs *encoder_funcs;
> >> > + struct edid *edid = NULL;
> >> > bool dret = false;
> >> > enum drm_connector_status ret = connector_status_disconnected;
> >> >
> >> > @@ -669,8 +670,24 @@ radeon_vga_detect(struct drm_connector *connector, bool force)
> >> > if (!encoder)
> >> > ret = connector_status_disconnected;
> >> >
> >> > - if (radeon_connector->ddc_bus)
> >> > + if (radeon_connector->ddc_bus) {
> >> > dret = radeon_ddc_probe(radeon_connector);
> >> > + if (!dret && radeon_connector->broken_edid_header_counter == 1) {
> >> > + /* Found a broken DDC when probing (single) connector
> >> > + modes during framebuffer initialisation.
> >> > + Clean-up and log the HW bug. */
> >> > + edid = drm_get_edid(&radeon_connector->base, &radeon_connector->ddc_bus->adapter);
> >> > + if (!edid) {
> >> > + radeon_connector->broken_edid_header_counter++;
> >> > + DRM_INFO("%s: probed a monitor, DDC responded but no|invalid EDID\n",
> >> > + drm_get_connector_name(connector));
> >> > + } else {
> >> > + /* We should not get here, as the EDID header is broken */
> >> > + connector->display_info.raw_edid = NULL;
> >> > + kfree(edid);
> >> > + }
> >> > + }
> >> > + }
> >>
> >> What is this hunk for? It's not related to the problematic DDIA
> >> interface which is digital only. It seems like it will just spam the
> >> log for VGA monitors with faulty EDIDs.
> >
> > The broken_edid_header_counter prevents from spamming. Only the first
> > time, the extended DDC probe fails due to a broken EDID header (which
> > may happen also to VGA connectors with a buggy monitor connected; at
> > least I remember to have seen such dmesg logs in the past) we would
> > inform users. As the EDID is broken, the screen will stay dark, but
> > users would have at least the required information in the logs. They can
> > post them, once they've connected a working monitor.
> > My ASUS RS690 system suffered from such log spamming. So please believe
> > me, that I would never ask for a solution that leads us back to this.
> >
>
> I think I would skip this part for now. radeon_vga_detect() gets
> called anytime you request a probe of a vga connector. The full
> header check in the radeon_ddc_probe should make this a moot point in
> most cases. It's also inconsistent to have this behaviour for only
> certain connectors.
For VGA connectors we focus on case 1 and 2:
1. no monitor attached
DDC is not available: The connector is regarded as being disconnected;
function drm_helper_probe_single_connector_modes() records a debug
message in the logs, i. e. "VGA-1 is disconnected".
The connector will be probed again, when probing single connector modes
(using function drm_helper_probe_single_connector_modes() about every
second).
2. monitor with faulty edid attached
DDC is available, invalid EDID (header): The connector is regarded as
being disconnected; If it's the first time, that this connector has been
probed, usually during framebuffer initialisation, dump the faulty EDID
to the logs and add an info message, that there is a problem with the
monitor: e. g. "VGA-1: probed a monitor, DDC responded but no|invalid
EDID". In addition, the debug message of case 1 is logged.
The connector will be probed again, when probing single connector modes
(using function drm_helper_probe_single_connector_modes() about every
second). But there will be only the debug message logged. No info
message and no EDID dump.
We could skip this part of the patch. But if we did this, this scenario
(http://askubuntu.com/questions/14299/my-monitor-is-plugged-into-vga-0-why-it-is-giving-me-errors-about-vga-1) would lead to a dark screen with some meaningless debug only information in the dmesg log:
...
[drm:drm_helper_probe_single_connector_modes], VGA-1
[drm:drm_helper_probe_single_connector_modes], VGA-1 is disconnected
...
As a non-expert user I would prefer a more informative log, that helps
me to request support:
...
[drm:drm_helper_probe_single_connector_modes], VGA-1
[drm:drm_edid_block_valid] *ERROR* EDID checksum is invalid, remainder
is 126
[drm:drm_edid_block_valid] *ERROR* Raw EDID:
<3>ee 8b c5 a4 5a 48 9b 25 1a 50 54 bf ef 00 81 80 ....ZH.%.PT.....
<3>10 10 01 03 68 22 1b 78 ee 8b c5 a4 5a 48 9b 25 ....h".x....ZH.%
<3>1a 50 54 bf ef 00 81 80 71 4f 01 01 01 01 01 01 .PT.....qO......
<3>01 01 01 01 01 01 30 2a 00 98 51 00 2a 40 30 70 ......0*..Q.*@0p
<3>13 00 52 0e 11 00 00 1e 00 00 00 fd 00 38 4c 1e ..R..........8L.
<3>52 0e 00 0a 20 20 20 20 20 20 00 00 00 fc 00 43 R... .....C
<3>4d 43 20 31 37 0a 20 20 20 20 20 20 00 00 00 ff MC 17. ....
<3>00 30 0a 20 20 20 20 20 20 20 20 20 20 20 00 9d .0. ..
[drm:drm_edid_block_valid] *ERROR* EDID checksum is invalid, remainder is 126
[drm:drm_edid_block_valid] *ERROR* Raw EDID:
<3>ee 8b c5 a4 5a 48 9b 25 1a 50 54 bf ef 00 81 80 ....ZH.%.PT.....
<3>10 10 01 03 68 22 1b 78 ee 8b c5 a4 5a 48 9b 25 ....h".x....ZH.%
<3>1a 50 54 bf ef 00 81 80 71 4f 01 01 01 01 01 01 .PT.....qO......
<3>01 01 01 01 01 01 30 2a 00 98 51 00 2a 40 30 70 ......0*..Q.*@0p
<3>13 00 52 0e 11 00 00 1e 00 00 00 fd 00 38 4c 1e ..R..........8L.
<3>52 0e 00 0a 20 20 20 20 20 20 00 00 00 fc 00 43 R... .....C
<3>4d 43 20 31 37 0a 20 20 20 20 20 20 00 00 00 ff MC 17. ....
<3>00 30 0a 20 20 20 20 20 20 20 20 20 20 20 00 9d .0. ..
[drm:drm_edid_block_valid] *ERROR* EDID checksum is invalid, remainder is 126
[drm:drm_edid_block_valid] *ERROR* Raw EDID:
<3>ee 8b c5 a4 5a 48 9b 25 1a 50 54 bf ef 00 81 80 ....ZH.%.PT.....
<3>10 10 01 03 68 22 1b 78 ee 8b c5 a4 5a 48 9b 25 ....h".x....ZH.%
<3>1a 50 54 bf ef 00 81 80 71 4f 01 01 01 01 01 01 .PT.....qO......
<3>01 01 01 01 01 01 30 2a 00 98 51 00 2a 40 30 70 ......0*..Q.*@0p
<3>13 00 52 0e 11 00 00 1e 00 00 00 fd 00 38 4c 1e ..R..........8L.
<3>52 0e 00 0a 20 20 20 20 20 20 00 00 00 fc 00 43 R... .....C
<3>4d 43 20 31 37 0a 20 20 20 20 20 20 00 00 00 ff MC 17. ....
<3>00 30 0a 20 20 20 20 20 20 20 20 20 20 20 00 9d .0. ..
[drm:drm_edid_block_valid] *ERROR* EDID checksum is invalid, remainder is 126
[drm:drm_edid_block_valid] *ERROR* Raw EDID:
<3>ee 8b c5 a4 5a 48 9b 25 1a 50 54 bf ef 00 81 80 ....ZH.%.PT.....
<3>10 10 01 03 68 22 1b 78 ee 8b c5 a4 5a 48 9b 25 ....h".x....ZH.%
<3>1a 50 54 bf ef 00 81 80 71 4f 01 01 01 01 01 01 .PT.....qO......
<3>01 01 01 01 01 01 30 2a 00 98 51 00 2a 40 30 70 ......0*..Q.*@0p
<3>13 00 52 0e 11 00 00 1e 00 00 00 fd 00 38 4c 1e ..R..........8L.
<3>52 0e 00 0a 20 20 20 20 20 20 00 00 00 fc 00 43 R... .....C
<3>4d 43 20 31 37 0a 20 20 20 20 20 20 00 00 00 ff MC 17. ....
<3>00 30 0a 20 20 20 20 20 20 20 20 20 20 20 00 9d .0. ..
radeon 0000:01:05.0: VGA-1: EDID block 0 invalid.
[drm:radeon_vga_detect] VGA-1: probed a monitor, DDC responded but no|invalid EDID
[drm:drm_helper_probe_single_connector_modes], VGA-1 is disconnected
...
We see, that the EDID information is stable, but wrong, i. e. no EDID
header. The monitor seems to be defect. Therefore, the system does not
use this connector/monitor.
Furtheron, only the followuing two debug messages will be in the log
(about every second). This is also true, if the user disconnects the
buggy monitor:
...
[drm:drm_helper_probe_single_connector_modes], VGA-1
[drm:drm_helper_probe_single_connector_modes], VGA-1 is disconnected
...
[drm:drm_helper_probe_single_connector_modes], VGA-1
[drm:drm_helper_probe_single_connector_modes], VGA-1 is disconnected
...
[drm:drm_helper_probe_single_connector_modes], VGA-1
[drm:drm_helper_probe_single_connector_modes], VGA-1 is disconnected
...
If the debug option is disabled, no more information will be recorded in
the logs.
If the user attaches a working monitor, the screen will be back and
following debug messages will be in the logs (every second):
...
[drm:drm_helper_probe_single_connector_modes], VGA-1
[drm:drm_helper_probe_single_connector_modes], Probed modes for VGA-1
[drm:drm_mode_debug_printmodeline], Modeline 17:"1280x1024" 60 108000
1280 1328 1440 1688 1024 1025 1028 1066 0x48 0x5
...
[drm:drm_helper_probe_single_connector_modes], VGA-1
[drm:drm_helper_probe_single_connector_modes], Probed modes for VGA-1
[drm:drm_mode_debug_printmodeline], Modeline 17:"1280x1024" 60 108000
1280 1328 1440 1688 1024 1025 1028 1066 0x48 0x5
...
[drm:drm_helper_probe_single_connector_modes], VGA-1
[drm:drm_helper_probe_single_connector_modes], Probed modes for VGA-1
[drm:drm_mode_debug_printmodeline], Modeline 17:"1280x1024" 60 108000
1280 1328 1440 1688 1024 1025 1028 1066 0x48 0x5
...
This behaviour of the Radeon driver is consistent for VGA and DVI
connectors. Current LVDS implementation will still flood the logs with
EDID dumps in case of faulty EDIDs, as there is no DDC probe
implemented. In case of TV connectors, we don't have EDIDs. Only
exception are DP connectors, where we still have to rely on the
drm_helper_probe_single_connector_modes() debug log messages.
>
> >>
> >> > if (dret) {
> >> > radeon_connector->detected_by_load = false;
> >> > if (radeon_connector->edid) {
> >> > @@ -847,12 +864,38 @@ radeon_dvi_detect(struct drm_connector *connector, bool force)
> >> > struct drm_encoder *encoder = NULL;
> >> > struct drm_encoder_helper_funcs *encoder_funcs;
> >> > struct drm_mode_object *obj;
> >> > + struct edid *edid = NULL;
> >> > int i;
> >> > enum drm_connector_status ret = connector_status_disconnected;
> >> > bool dret = false;
> >> >
> >> > - if (radeon_connector->ddc_bus)
> >> > + if (radeon_connector->ddc_bus) {
> >> > dret = radeon_ddc_probe(radeon_connector);
> >> > + if (!dret && radeon_connector->broken_edid_header_counter == 1) {
> >> > + /* Found a broken DDC when probing (single) connector
> >> > + modes during framebuffer initialisation.
> >> > + Clean-up and log the HW bug. */
> >> > + edid = drm_get_edid(&radeon_connector->base, &radeon_connector->ddc_bus->adapter);
> >> > + if (!edid) {
> >> > + radeon_connector->broken_edid_header_counter++;
> >> > + DRM_INFO("%s: probed a monitor, DDC responded but no|invalid EDID\n",
> >> > + drm_get_connector_name(connector));
> >> > + /* rs690 seems to have a problem with connectors not existing and return
> >> > + * random EDIDs mainly with blocks of 0's. If we see this, just stop
> >> > + * polling on this output */
> >> > + if ((rdev->family == CHIP_RS690 || rdev->family == CHIP_RS740)) {
> >> > + ret = connector_status_disconnected;
> >> > + DRM_INFO("%s: detected RS690 floating bus bug, stopping ddc detect\n",
> >> > + drm_get_connector_name(connector));
> >> > + radeon_connector->ddc_bus = NULL;
> >> > + }
> >> > + } else {
> >> > + /* We should not get here, as the EDID header is broken */
> >> > + connector->display_info.raw_edid = NULL;
> >> > + kfree(edid);
> >> > + }
> >> > + }
> >> > + }
> >>
> >> Is this really needed? What is it preventing? Doesn't the full EDID
> >> header probe alleviate the need for this? My concern is that we will
> >> remove the ddc bus on connectors that are valid just because the user
> >> probed with no monitor connected or a monitor with a faulty edid.
> >> Then all future probes will fail to get an EDID since the bus is gone.
> >
> > You're right. This is a bug! Somehow I lost the third if condition: &&
> > radeon_connector->base.null_edid_counter when updating the patches.
> > Thanks for checking the code. I'll post the corrected one after sending
> > this mail.
> >
> >>
> >> > if (dret) {
> >> > radeon_connector->detected_by_load = false;
> >> > if (radeon_connector->edid) {
> >> > @@ -864,13 +907,6 @@ radeon_dvi_detect(struct drm_connector *connector, bool force)
> >> > if (!radeon_connector->edid) {
> >> > DRM_ERROR("%s: probed a monitor but no|invalid EDID\n",
> >> > drm_get_connector_name(connector));
> >> > - /* rs690 seems to have a problem with connectors not existing and always
> >> > - * return a block of 0's. If we see this just stop polling on this output */
> >> > - if ((rdev->family == CHIP_RS690 || rdev->family == CHIP_RS740) && radeon_connector->base.null_edid_counter) {
> >> > - ret = connector_status_disconnected;
> >> > - DRM_ERROR("%s: detected RS690 floating bus bug, stopping ddc detect\n", drm_get_connector_name(connector));
> >> > - radeon_connector->ddc_bus = NULL;
> >> > - }
> >>
> >> This part can probably be removed now.
> >>
> >> > } else {
> >> > radeon_connector->use_digital = !!(radeon_connector->edid->input & DRM_EDID_INPUT_DIGITAL);
> >> >
> >> > diff --git a/drivers/gpu/drm/radeon/radeon_i2c.c b/drivers/gpu/drm/radeon/radeon_i2c.c
> >> > index 7bb1b07..ac6aa02 100644
> >> > --- a/drivers/gpu/drm/radeon/radeon_i2c.c
> >> > +++ b/drivers/gpu/drm/radeon/radeon_i2c.c
> >> > @@ -59,10 +59,12 @@ bool radeon_ddc_probe(struct radeon_connector *radeon_connector)
> >> > radeon_router_select_ddc_port(radeon_connector);
> >> >
> >> > ret = i2c_transfer(&radeon_connector->ddc_bus->adapter, msgs, 2);
> >> > - if (ret != 2)
> >> > + if (ret != 2) {
> >> > /* Couldn't find an accessible DDC on this connector */
> >> > + radeon_connector->broken_edid_header_counter = 0;
> >> > return false;
> >> > - /* Probe also for valid EDID header
> >> > + }
> >> > + /* DDC is available; probe also for valid EDID header
> >> > * EDID header starts with:
> >> > * 0x00,0xFF,0xFF,0xFF,0xFF,0xFF,0xFF,0x00.
> >> > * Only the first 6 bytes must be valid as
> >> > @@ -70,6 +72,7 @@ bool radeon_ddc_probe(struct radeon_connector *radeon_connector)
> >> > if (drm_edid_header_is_valid(buf) < 6) {
> >> > /* Couldn't find an accessible EDID on this
> >> > * connector */
> >> > + radeon_connector->broken_edid_header_counter++;
> >> > return false;
> >> > }
> >> > return true;
> >> > diff --git a/drivers/gpu/drm/radeon/radeon_mode.h b/drivers/gpu/drm/radeon/radeon_mode.h
> >> > index 2c2e75e..4af4583 100644
> >> > --- a/drivers/gpu/drm/radeon/radeon_mode.h
> >> > +++ b/drivers/gpu/drm/radeon/radeon_mode.h
> >> > @@ -442,6 +442,9 @@ struct radeon_connector {
> >> > /* we need to mind the EDID between detect
> >> > and get modes due to analog/digital/tvencoder */
> >> > struct edid *edid;
> >> > + /* extended DDC probing is now default for radeon connectors
> >> > + count the number of failed EDID header probes */
> >> > + int broken_edid_header_counter;
> >> > void *con_priv;
> >> > bool dac_load_detect;
> >> > bool detected_by_load; /* if the connection status was determined by load */
> >> > --
> >> > 1.7.1
> >> >
> >> >
> >
> > Best regards
> >
> > Thomas
> >
> >
> >
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 2/2] drm/radeon/kms: wrap-up handling of floating connectors and connector unavailability status logging
2011-11-30 10:54 ` Thomas Reim
@ 2011-12-01 14:10 ` Alex Deucher
0 siblings, 0 replies; 10+ messages in thread
From: Alex Deucher @ 2011-12-01 14:10 UTC (permalink / raw)
To: Thomas Reim; +Cc: Dave Airlie, dri-devel
On Wed, Nov 30, 2011 at 5:54 AM, Thomas Reim <reimth@googlemail.com> wrote:
> Am Dienstag, den 29.11.2011, 15:53 -0500 schrieb Alex Deucher:
>> On Tue, Nov 29, 2011 at 1:06 PM, Thomas Reim <reimth@googlemail.com> wrote:
>> > Dear Alex,
>> >
>> >> On Mon, Nov 28, 2011 at 11:20 AM, Thomas Reim <reimth@googlemail.com> wrote:
>> >> > Extended DDC probe is now default for RADEON chipsets. In case of
>> >> > HW bugs (e. g. floating connectors), the affected connectors will
>> >> > not be used, as a valid EDID header can not be detected. Another
>> >> > patch removed DDC detection and connector status logging during
>> >> > device setup. So the user is not informed anymore about HW bugs
>> >> > leading to connectors being unavailable.
>> >> > Reintroduce one-time logging of connector unavailability status
>> >> > when probing (single) connector modes during framebuffer
>> >> > initialisation.
>> >> >
>> >> > For RS690 chipsets DDC detection shall be stopped, if the i2c bus
>> >> > receives all-0 EDIDs (floating connectors). The introduction of
>> >> > extended DDC probing prevents the driver from doing this. Correctly
>> >> > relocate the related code.
>> >>
>> >> Is any of this needed anymore now that we do an extended probe? I
>> >> think the original null edid patch was just papering over the actual
>> >> issue which was the need for a full edid header probe.
>> >
>> > The difference, at least this is my understanding of Dave's patch, is
>> > that we completely stop DDC detection for the famous RS690/RS740 chips
>> > with missing or disabled HDMI add-on cards. We decrease latency, as we
>> > do not retrieve data from i2c bus anymore. Therefore, I would keep it.
>> >
>>
>> My only concern is that we may disable the i2c bus if there is no
>> monitor connected which would prevent edid fetching from working in
>> the future. Does the problematic ddia i2c line produce the same error
>> in the following cases:
>>
>
> The proposed solution differentiates between i2c bus responds, i. e. a
> DDC is available, and EDID (header) is valid.
Right, but my concern is that if we remove the i2c bus, we no longer get DDC.
>
>> 1. ddia hdmi connector present, no monitor attached
>
> DDC is not available: The connector is regarded as being disconnected;
> function drm_helper_probe_single_connector_modes() records a debug
> message in the logs, i. e. "HDMI-A-1 is disconnected".
>
> The connector will be probed again, when probing single connector modes
> (using function drm_helper_probe_single_connector_modes() about every
> second).
It's every 5 seconds for polled connectors. If the OEM implements an
HPD pin for the connector, it's interrupt driven, so no polling
occurs. We get an interrupt when a monitor is connected.
>
>> 2. ddia hdmi connector present, monitor with faulty edid attached
>>
>
> DDC is available, invalid EDID (header): The connector is regarded as
> being disconnected; If it's the first time, that this connector has been
> probed, usually during framebuffer initialisation, dump the faulty EDID
> to the logs and add an info message, that there is a problem with the
> monitor: e. g. "HDMI-A-1: probed a monitor, DDC responded but no|invalid
> EDID". In addition, the debug message of case 1 is logged.
>
> Then we have two subcases:
> a) General
> The connector will be probed again, when probing single connector modes
> (using function drm_helper_probe_single_connector_modes() about every
> second). But there will be only the debug message logged. No info
> message and no EDID dump.
> b) EDID is all zero, i. e. not more than 8 non-zero bytes have been
> received (RS690/RS740 chips only)
> Floating connector is assumed and an info message is logged: "HDMI-A-1:
> detected RS690 floating bus bug, stopping ddc detect". Such a connector
> will not be probed again, reducing latency. I believe that this is
> rather a theoretical case, as it requires a monitor with an erased
> EEPROM.
>
>> 3. ddia hdmi connector absent
> a) Correctly implemented HW
> would handle this in the same way as for case 1. The connector is part
> of the chipset, i2c bus is correctly terminated and will never be used,
> but being probed. This is a drawback of implmentations, that use a
> chipset wich provides more connectors than being used by the
> implementation.
> b) Buggy HW
> The connector's i2c has not been terminated correctly. It can't be used,
> but is detected. The EDID information will mainly consist of zero bytes,
> except of some random byte values.
>
> DDC is available, invalid EDID (header): The connector is regarded as
> being disconnected; If it's the first time, that this connector has been
> probed, usually during framebuffer initialisation, dump the received information
> of the not available EDID to the logs and add an info message, that
> there is a problem with the connector: e. g. "HDMI-A-1: probed a
> monitor, DDC responded but no|invalid EDID". In addition, the debug
> message of the first case is logged.
>
> Then we have two subcases:
> (1) RS690/RS740 chips
> Floating connector is assumed and an info message is logged: "HDMI-A-1:
> detected RS690 floating bus bug, stopping ddc detect". Such a connector
> will not be probed again, reducing latency.
> (2) Other chipsets
> The connector will be probed again, when probing single connector modes
> (using function drm_helper_probe_single_connector_modes() about every
> second). But there will be only a debug message (see case 1) logged. No
> info message and EDID dump.
>
>>
>> In the third case it makes sense to remove the i2c bus, but in the
>> first two, we may remove the i2c bus which would prevent future edid
>> fetches from working.
>
> The there is a 4th use case:
> 4. ddia hdmi connector present, monitor with faulty edid connected, then
> removed and re-connected again
> The Radeon driver will behave in the same way as in case 2. But each
> time the faulty monitor is attached again to the connector, the info
> message extended by a dump of the faulty EDID will be recorded once (!)
> in the logs.
>
Right. My concern is the following. A user boots his system with a
ddia connector present, but no monitor connected to it. The driver
sees a faulty EDID header and removes the DDC bus. Later the user
connects a monitor with a working EDID and tries to enable it. Now
since the i2c bus is gone we no longer have DDC so we can't get an
EDID for the monitor and will probably not detect anything connected.
If we leave things as is, everything should just work.
>>
>> >>
>> >> >
>> >> > Signed-off-by: Thomas Reim <reimth@gmail.com>
>> >> > ---
>> >> > drivers/gpu/drm/radeon/radeon_connectors.c | 54 +++++++++++++++++++++++-----
>> >> > drivers/gpu/drm/radeon/radeon_i2c.c | 7 +++-
>> >> > drivers/gpu/drm/radeon/radeon_mode.h | 3 ++
>> >> > 3 files changed, 53 insertions(+), 11 deletions(-)
>> >> >
>> >> > diff --git a/drivers/gpu/drm/radeon/radeon_connectors.c b/drivers/gpu/drm/radeon/radeon_connectors.c
>> >> > index e7cb3ab..a4c0f59 100644
>> >> > --- a/drivers/gpu/drm/radeon/radeon_connectors.c
>> >> > +++ b/drivers/gpu/drm/radeon/radeon_connectors.c
>> >> > @@ -662,6 +662,7 @@ radeon_vga_detect(struct drm_connector *connector, bool force)
>> >> > struct radeon_connector *radeon_connector = to_radeon_connector(connector);
>> >> > struct drm_encoder *encoder;
>> >> > struct drm_encoder_helper_funcs *encoder_funcs;
>> >> > + struct edid *edid = NULL;
>> >> > bool dret = false;
>> >> > enum drm_connector_status ret = connector_status_disconnected;
>> >> >
>> >> > @@ -669,8 +670,24 @@ radeon_vga_detect(struct drm_connector *connector, bool force)
>> >> > if (!encoder)
>> >> > ret = connector_status_disconnected;
>> >> >
>> >> > - if (radeon_connector->ddc_bus)
>> >> > + if (radeon_connector->ddc_bus) {
>> >> > dret = radeon_ddc_probe(radeon_connector);
>> >> > + if (!dret && radeon_connector->broken_edid_header_counter == 1) {
>> >> > + /* Found a broken DDC when probing (single) connector
>> >> > + modes during framebuffer initialisation.
>> >> > + Clean-up and log the HW bug. */
>> >> > + edid = drm_get_edid(&radeon_connector->base, &radeon_connector->ddc_bus->adapter);
>> >> > + if (!edid) {
>> >> > + radeon_connector->broken_edid_header_counter++;
>> >> > + DRM_INFO("%s: probed a monitor, DDC responded but no|invalid EDID\n",
>> >> > + drm_get_connector_name(connector));
>> >> > + } else {
>> >> > + /* We should not get here, as the EDID header is broken */
>> >> > + connector->display_info.raw_edid = NULL;
>> >> > + kfree(edid);
>> >> > + }
>> >> > + }
>> >> > + }
>> >>
>> >> What is this hunk for? It's not related to the problematic DDIA
>> >> interface which is digital only. It seems like it will just spam the
>> >> log for VGA monitors with faulty EDIDs.
>> >
>> > The broken_edid_header_counter prevents from spamming. Only the first
>> > time, the extended DDC probe fails due to a broken EDID header (which
>> > may happen also to VGA connectors with a buggy monitor connected; at
>> > least I remember to have seen such dmesg logs in the past) we would
>> > inform users. As the EDID is broken, the screen will stay dark, but
>> > users would have at least the required information in the logs. They can
>> > post them, once they've connected a working monitor.
>> > My ASUS RS690 system suffered from such log spamming. So please believe
>> > me, that I would never ask for a solution that leads us back to this.
>> >
>>
>> I think I would skip this part for now. radeon_vga_detect() gets
>> called anytime you request a probe of a vga connector. The full
>> header check in the radeon_ddc_probe should make this a moot point in
>> most cases. It's also inconsistent to have this behaviour for only
>> certain connectors.
>
> For VGA connectors we focus on case 1 and 2:
> 1. no monitor attached
>
> DDC is not available: The connector is regarded as being disconnected;
> function drm_helper_probe_single_connector_modes() records a debug
> message in the logs, i. e. "VGA-1 is disconnected".
For VGA we can still use load detection. Plus we now introduce a full
EDID fetch.
>
> The connector will be probed again, when probing single connector modes
> (using function drm_helper_probe_single_connector_modes() about every
> second).
>
> 2. monitor with faulty edid attached
>
> DDC is available, invalid EDID (header): The connector is regarded as
> being disconnected; If it's the first time, that this connector has been
> probed, usually during framebuffer initialisation, dump the faulty EDID
> to the logs and add an info message, that there is a problem with the
> monitor: e. g. "VGA-1: probed a monitor, DDC responded but no|invalid
> EDID". In addition, the debug message of case 1 is logged.
>
> The connector will be probed again, when probing single connector modes
> (using function drm_helper_probe_single_connector_modes() about every
> second). But there will be only the debug message logged. No info
> message and no EDID dump.
>
> We could skip this part of the patch. But if we did this, this scenario
> (http://askubuntu.com/questions/14299/my-monitor-is-plugged-into-vga-0-why-it-is-giving-me-errors-about-vga-1) would lead to a dark screen with some meaningless debug only information in the dmesg log:
> ...
> [drm:drm_helper_probe_single_connector_modes], VGA-1
> [drm:drm_helper_probe_single_connector_modes], VGA-1 is disconnected
> ...
>
With VGA load detection will mostly likely detect a monitor even if
and EDID is not available.
> As a non-expert user I would prefer a more informative log, that helps
> me to request support:
If we want more complex detection logic I'd prefer we do it in the
common drm code so it's consistent across cards and connectors. Also,
I think we should be more forgiving about wonky EDIDs but that's
another issue.
> ...
> [drm:drm_helper_probe_single_connector_modes], VGA-1
> [drm:drm_edid_block_valid] *ERROR* EDID checksum is invalid, remainder
> is 126
> [drm:drm_edid_block_valid] *ERROR* Raw EDID:
> <3>ee 8b c5 a4 5a 48 9b 25 1a 50 54 bf ef 00 81 80 ....ZH.%.PT.....
> <3>10 10 01 03 68 22 1b 78 ee 8b c5 a4 5a 48 9b 25 ....h".x....ZH.%
> <3>1a 50 54 bf ef 00 81 80 71 4f 01 01 01 01 01 01 .PT.....qO......
> <3>01 01 01 01 01 01 30 2a 00 98 51 00 2a 40 30 70 ......0*..Q.*@0p
> <3>13 00 52 0e 11 00 00 1e 00 00 00 fd 00 38 4c 1e ..R..........8L.
> <3>52 0e 00 0a 20 20 20 20 20 20 00 00 00 fc 00 43 R... .....C
> <3>4d 43 20 31 37 0a 20 20 20 20 20 20 00 00 00 ff MC 17. ....
> <3>00 30 0a 20 20 20 20 20 20 20 20 20 20 20 00 9d .0. ..
>
> [drm:drm_edid_block_valid] *ERROR* EDID checksum is invalid, remainder is 126
> [drm:drm_edid_block_valid] *ERROR* Raw EDID:
> <3>ee 8b c5 a4 5a 48 9b 25 1a 50 54 bf ef 00 81 80 ....ZH.%.PT.....
> <3>10 10 01 03 68 22 1b 78 ee 8b c5 a4 5a 48 9b 25 ....h".x....ZH.%
> <3>1a 50 54 bf ef 00 81 80 71 4f 01 01 01 01 01 01 .PT.....qO......
> <3>01 01 01 01 01 01 30 2a 00 98 51 00 2a 40 30 70 ......0*..Q.*@0p
> <3>13 00 52 0e 11 00 00 1e 00 00 00 fd 00 38 4c 1e ..R..........8L.
> <3>52 0e 00 0a 20 20 20 20 20 20 00 00 00 fc 00 43 R... .....C
> <3>4d 43 20 31 37 0a 20 20 20 20 20 20 00 00 00 ff MC 17. ....
> <3>00 30 0a 20 20 20 20 20 20 20 20 20 20 20 00 9d .0. ..
>
> [drm:drm_edid_block_valid] *ERROR* EDID checksum is invalid, remainder is 126
> [drm:drm_edid_block_valid] *ERROR* Raw EDID:
> <3>ee 8b c5 a4 5a 48 9b 25 1a 50 54 bf ef 00 81 80 ....ZH.%.PT.....
> <3>10 10 01 03 68 22 1b 78 ee 8b c5 a4 5a 48 9b 25 ....h".x....ZH.%
> <3>1a 50 54 bf ef 00 81 80 71 4f 01 01 01 01 01 01 .PT.....qO......
> <3>01 01 01 01 01 01 30 2a 00 98 51 00 2a 40 30 70 ......0*..Q.*@0p
> <3>13 00 52 0e 11 00 00 1e 00 00 00 fd 00 38 4c 1e ..R..........8L.
> <3>52 0e 00 0a 20 20 20 20 20 20 00 00 00 fc 00 43 R... .....C
> <3>4d 43 20 31 37 0a 20 20 20 20 20 20 00 00 00 ff MC 17. ....
> <3>00 30 0a 20 20 20 20 20 20 20 20 20 20 20 00 9d .0. ..
>
> [drm:drm_edid_block_valid] *ERROR* EDID checksum is invalid, remainder is 126
> [drm:drm_edid_block_valid] *ERROR* Raw EDID:
> <3>ee 8b c5 a4 5a 48 9b 25 1a 50 54 bf ef 00 81 80 ....ZH.%.PT.....
> <3>10 10 01 03 68 22 1b 78 ee 8b c5 a4 5a 48 9b 25 ....h".x....ZH.%
> <3>1a 50 54 bf ef 00 81 80 71 4f 01 01 01 01 01 01 .PT.....qO......
> <3>01 01 01 01 01 01 30 2a 00 98 51 00 2a 40 30 70 ......0*..Q.*@0p
> <3>13 00 52 0e 11 00 00 1e 00 00 00 fd 00 38 4c 1e ..R..........8L.
> <3>52 0e 00 0a 20 20 20 20 20 20 00 00 00 fc 00 43 R... .....C
> <3>4d 43 20 31 37 0a 20 20 20 20 20 20 00 00 00 ff MC 17. ....
> <3>00 30 0a 20 20 20 20 20 20 20 20 20 20 20 00 9d .0. ..
>
> radeon 0000:01:05.0: VGA-1: EDID block 0 invalid.
> [drm:radeon_vga_detect] VGA-1: probed a monitor, DDC responded but no|invalid EDID
> [drm:drm_helper_probe_single_connector_modes], VGA-1 is disconnected
> ...
>
> We see, that the EDID information is stable, but wrong, i. e. no EDID
> header. The monitor seems to be defect. Therefore, the system does not
> use this connector/monitor.
The user just sees a blank screen whether there is a bunch of EDID
junk or not doesn't really change things. It's not like we can fix
their monitor.
>
> Furtheron, only the followuing two debug messages will be in the log
> (about every second). This is also true, if the user disconnects the
> buggy monitor:
> ...
> [drm:drm_helper_probe_single_connector_modes], VGA-1
> [drm:drm_helper_probe_single_connector_modes], VGA-1 is disconnected
> ...
> [drm:drm_helper_probe_single_connector_modes], VGA-1
> [drm:drm_helper_probe_single_connector_modes], VGA-1 is disconnected
> ...
> [drm:drm_helper_probe_single_connector_modes], VGA-1
> [drm:drm_helper_probe_single_connector_modes], VGA-1 is disconnected
> ...
> If the debug option is disabled, no more information will be recorded in
> the logs.
>
> If the user attaches a working monitor, the screen will be back and
> following debug messages will be in the logs (every second):
> ...
> [drm:drm_helper_probe_single_connector_modes], VGA-1
> [drm:drm_helper_probe_single_connector_modes], Probed modes for VGA-1
> [drm:drm_mode_debug_printmodeline], Modeline 17:"1280x1024" 60 108000
> 1280 1328 1440 1688 1024 1025 1028 1066 0x48 0x5
> ...
> [drm:drm_helper_probe_single_connector_modes], VGA-1
> [drm:drm_helper_probe_single_connector_modes], Probed modes for VGA-1
> [drm:drm_mode_debug_printmodeline], Modeline 17:"1280x1024" 60 108000
> 1280 1328 1440 1688 1024 1025 1028 1066 0x48 0x5
> ...
> [drm:drm_helper_probe_single_connector_modes], VGA-1
> [drm:drm_helper_probe_single_connector_modes], Probed modes for VGA-1
> [drm:drm_mode_debug_printmodeline], Modeline 17:"1280x1024" 60 108000
> 1280 1328 1440 1688 1024 1025 1028 1066 0x48 0x5
> ...
>
> This behaviour of the Radeon driver is consistent for VGA and DVI
> connectors. Current LVDS implementation will still flood the logs with
> EDID dumps in case of faulty EDIDs, as there is no DDC probe
> implemented. In case of TV connectors, we don't have EDIDs. Only
> exception are DP connectors, where we still have to rely on the
> drm_helper_probe_single_connector_modes() debug log messages.
>
>>
>> >>
>> >> > if (dret) {
>> >> > radeon_connector->detected_by_load = false;
>> >> > if (radeon_connector->edid) {
>> >> > @@ -847,12 +864,38 @@ radeon_dvi_detect(struct drm_connector *connector, bool force)
>> >> > struct drm_encoder *encoder = NULL;
>> >> > struct drm_encoder_helper_funcs *encoder_funcs;
>> >> > struct drm_mode_object *obj;
>> >> > + struct edid *edid = NULL;
>> >> > int i;
>> >> > enum drm_connector_status ret = connector_status_disconnected;
>> >> > bool dret = false;
>> >> >
>> >> > - if (radeon_connector->ddc_bus)
>> >> > + if (radeon_connector->ddc_bus) {
>> >> > dret = radeon_ddc_probe(radeon_connector);
>> >> > + if (!dret && radeon_connector->broken_edid_header_counter == 1) {
>> >> > + /* Found a broken DDC when probing (single) connector
>> >> > + modes during framebuffer initialisation.
>> >> > + Clean-up and log the HW bug. */
>> >> > + edid = drm_get_edid(&radeon_connector->base, &radeon_connector->ddc_bus->adapter);
>> >> > + if (!edid) {
>> >> > + radeon_connector->broken_edid_header_counter++;
>> >> > + DRM_INFO("%s: probed a monitor, DDC responded but no|invalid EDID\n",
>> >> > + drm_get_connector_name(connector));
>> >> > + /* rs690 seems to have a problem with connectors not existing and return
>> >> > + * random EDIDs mainly with blocks of 0's. If we see this, just stop
>> >> > + * polling on this output */
>> >> > + if ((rdev->family == CHIP_RS690 || rdev->family == CHIP_RS740)) {
>> >> > + ret = connector_status_disconnected;
>> >> > + DRM_INFO("%s: detected RS690 floating bus bug, stopping ddc detect\n",
>> >> > + drm_get_connector_name(connector));
>> >> > + radeon_connector->ddc_bus = NULL;
>> >> > + }
>> >> > + } else {
>> >> > + /* We should not get here, as the EDID header is broken */
>> >> > + connector->display_info.raw_edid = NULL;
>> >> > + kfree(edid);
>> >> > + }
>> >> > + }
>> >> > + }
>> >>
>> >> Is this really needed? What is it preventing? Doesn't the full EDID
>> >> header probe alleviate the need for this? My concern is that we will
>> >> remove the ddc bus on connectors that are valid just because the user
>> >> probed with no monitor connected or a monitor with a faulty edid.
>> >> Then all future probes will fail to get an EDID since the bus is gone.
>> >
>> > You're right. This is a bug! Somehow I lost the third if condition: &&
>> > radeon_connector->base.null_edid_counter when updating the patches.
>> > Thanks for checking the code. I'll post the corrected one after sending
>> > this mail.
>> >
>> >>
>> >> > if (dret) {
>> >> > radeon_connector->detected_by_load = false;
>> >> > if (radeon_connector->edid) {
>> >> > @@ -864,13 +907,6 @@ radeon_dvi_detect(struct drm_connector *connector, bool force)
>> >> > if (!radeon_connector->edid) {
>> >> > DRM_ERROR("%s: probed a monitor but no|invalid EDID\n",
>> >> > drm_get_connector_name(connector));
>> >> > - /* rs690 seems to have a problem with connectors not existing and always
>> >> > - * return a block of 0's. If we see this just stop polling on this output */
>> >> > - if ((rdev->family == CHIP_RS690 || rdev->family == CHIP_RS740) && radeon_connector->base.null_edid_counter) {
>> >> > - ret = connector_status_disconnected;
>> >> > - DRM_ERROR("%s: detected RS690 floating bus bug, stopping ddc detect\n", drm_get_connector_name(connector));
>> >> > - radeon_connector->ddc_bus = NULL;
>> >> > - }
>> >>
>> >> This part can probably be removed now.
>> >>
>> >> > } else {
>> >> > radeon_connector->use_digital = !!(radeon_connector->edid->input & DRM_EDID_INPUT_DIGITAL);
>> >> >
>> >> > diff --git a/drivers/gpu/drm/radeon/radeon_i2c.c b/drivers/gpu/drm/radeon/radeon_i2c.c
>> >> > index 7bb1b07..ac6aa02 100644
>> >> > --- a/drivers/gpu/drm/radeon/radeon_i2c.c
>> >> > +++ b/drivers/gpu/drm/radeon/radeon_i2c.c
>> >> > @@ -59,10 +59,12 @@ bool radeon_ddc_probe(struct radeon_connector *radeon_connector)
>> >> > radeon_router_select_ddc_port(radeon_connector);
>> >> >
>> >> > ret = i2c_transfer(&radeon_connector->ddc_bus->adapter, msgs, 2);
>> >> > - if (ret != 2)
>> >> > + if (ret != 2) {
>> >> > /* Couldn't find an accessible DDC on this connector */
>> >> > + radeon_connector->broken_edid_header_counter = 0;
>> >> > return false;
>> >> > - /* Probe also for valid EDID header
>> >> > + }
>> >> > + /* DDC is available; probe also for valid EDID header
>> >> > * EDID header starts with:
>> >> > * 0x00,0xFF,0xFF,0xFF,0xFF,0xFF,0xFF,0x00.
>> >> > * Only the first 6 bytes must be valid as
>> >> > @@ -70,6 +72,7 @@ bool radeon_ddc_probe(struct radeon_connector *radeon_connector)
>> >> > if (drm_edid_header_is_valid(buf) < 6) {
>> >> > /* Couldn't find an accessible EDID on this
>> >> > * connector */
>> >> > + radeon_connector->broken_edid_header_counter++;
>> >> > return false;
>> >> > }
>> >> > return true;
>> >> > diff --git a/drivers/gpu/drm/radeon/radeon_mode.h b/drivers/gpu/drm/radeon/radeon_mode.h
>> >> > index 2c2e75e..4af4583 100644
>> >> > --- a/drivers/gpu/drm/radeon/radeon_mode.h
>> >> > +++ b/drivers/gpu/drm/radeon/radeon_mode.h
>> >> > @@ -442,6 +442,9 @@ struct radeon_connector {
>> >> > /* we need to mind the EDID between detect
>> >> > and get modes due to analog/digital/tvencoder */
>> >> > struct edid *edid;
>> >> > + /* extended DDC probing is now default for radeon connectors
>> >> > + count the number of failed EDID header probes */
>> >> > + int broken_edid_header_counter;
>> >> > void *con_priv;
>> >> > bool dac_load_detect;
>> >> > bool detected_by_load; /* if the connection status was determined by load */
>> >> > --
>> >> > 1.7.1
>> >> >
>> >> >
>> >
>> > Best regards
>> >
>> > Thomas
>> >
>> >
>> >
>
>
>
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2011-12-01 14:10 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2011-11-28 16:20 [PATCH 0/2] drm_radeon_kms: Some Regression Fixes for Extended DDC Probe Thomas Reim
2011-11-28 16:20 ` [PATCH 1/2] drm: Improve detection of floating connectors Thomas Reim
2011-11-28 16:20 ` [PATCH 2/2] drm/radeon/kms: wrap-up handling of floating connectors and connector unavailability status logging Thomas Reim
2011-11-29 14:50 ` Alex Deucher
2011-11-29 18:06 ` Thomas Reim
2011-11-29 20:53 ` Alex Deucher
2011-11-30 10:54 ` Thomas Reim
2011-12-01 14:10 ` Alex Deucher
2011-11-29 18:33 ` Thomas Reim
2011-11-28 16:34 ` [PATCH 0/2] drm_radeon_kms: Some Regression Fixes for Extended DDC Probe Thomas Reim
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.