Dear Alex,
> On Mon, Nov 28, 2011 at 11:20 AM, Thomas Reim <reimth@xxxxxxxxxxxxxx> 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@xxxxxxxxx>
> > ---
> > 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
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/dri-devel
