lkml.org 
[lkml]   [2011]   [Jul]   [6]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    SubjectRe: [PATCH] drm/radeon: Fix Asus M2A-VM HDMI EDID error flooding problem
    From
    On Wed, Jul 6, 2011 at 6:09 AM,  <reimth@googlemail.com> wrote:
    > From: Thomas Reim <rdratlos@yahoo.co.uk>
    >
    >    Some integrated ATI Radeon chipset implementations
    >    with add-on HDMI card (e. g. Asus M2A-VM HDMI) indicate the availability
    >    of a DDC even when the add-on card is not plugged in or HDMI is disabled
    >    in BIOS setup. In this case, drm_get_edid() and drm_edid_block_valid()
    >    periodically dump data and kernel errors into system log files and onto
    >    terminals. For these chipsets DDC probing is extended by a check for a
    >    correct EDID header. Only in case a valid EDID header is also found, the
    >    (HDMI) connector will be used by the Radeon driver. This prevents the
    >    kernel driver from useless flooding of logs and terminal sessions with
    >    EDID dumps and error messages.
    >    This patch adds a flag 'requires_extended_probe' to the radeon_connector
    >    structure. In function radeon_connector_needs_extended_probe() this flag
    >    can be set on a chipset family/vendor/connector type specific basis.
    >    In addition, function drm_edid_header_is_valid() has been added for EDID
    >    header check and function radeon_ddc_probe() has been adapted to perform
    >    extended DDC probing if required by the connector's flag.
    >
    >    Tested for kernel 2.35, 2.38 and 3.0 on Asus M2A-VM HDMI board
    >

    Once it's ready, just add:
    Cc: stable@kernel.org
    to the commit message and it will go into the stable kernels as well.
    Might want to mention the bug report in your commit message as well.
    Just a couple comments below. With those fixed:

    Reviewed-by: Alex Deucher <alexdeucher@gmail.com>

    > Signed-off-by: Thomas Reim <rdratlos@yahoo.co.uk>
    > ---
    >  drivers/gpu/drm/drm_edid.c                 |   24 +++++++++++----
    >  drivers/gpu/drm/radeon/radeon_connectors.c |   45 ++++++++++++++++++++++++++--
    >  drivers/gpu/drm/radeon/radeon_device.c     |    7 ++++-
    >  drivers/gpu/drm/radeon/radeon_display.c    |    9 +++++
    >  drivers/gpu/drm/radeon/radeon_i2c.c        |   32 +++++++++++++++-----
    >  drivers/gpu/drm/radeon/radeon_mode.h       |    6 +++-
    >  include/drm/drm_crtc.h                     |    1 +
    >  7 files changed, 105 insertions(+), 19 deletions(-)
    >
    > diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
    > index 0929219..1bbb85b 100644
    > --- a/drivers/gpu/drm/drm_edid.c
    > +++ b/drivers/gpu/drm/drm_edid.c
    > @@ -127,6 +127,23 @@ static const u8 edid_header[] = {
    >        0x00, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0x00
    >  };
    >
    > + /*
    > + * Sanity check the header of the base EDID block.  Return 8 if the header
    > + * is perfect, down to 0 if it's totally wrong.
    > + */
    > +int drm_edid_header_is_valid(const u8 *raw_edid)
    > +{
    > +       int i, score = 0;
    > +
    > +       for (i = 0; i < sizeof(edid_header); i++)
    > +               if (raw_edid[i] == edid_header[i])
    > +                       score++;
    > +
    > +       return score;
    > +}
    > +EXPORT_SYMBOL(drm_edid_header_is_valid);
    > +
    > +
    >  /*
    >  * Sanity check the EDID block (base or extension).  Return 0 if the block
    >  * doesn't check out, or 1 if it's valid.
    > @@ -139,12 +156,7 @@ drm_edid_block_valid(u8 *raw_edid)
    >        struct edid *edid = (struct edid *)raw_edid;
    >
    >        if (raw_edid[0] == 0x00) {
    > -               int score = 0;
    > -
    > -               for (i = 0; i < sizeof(edid_header); i++)
    > -                       if (raw_edid[i] == edid_header[i])
    > -                               score++;
    > -
    > +               int score = drm_edid_header_is_valid(raw_edid);
    >                if (score == 8) ;
    >                else if (score >= 6) {
    >                        DRM_DEBUG("Fixing EDID header, your hardware may be failing\n");

    Might want to break this hunk out as a separate patch.

    > diff --git a/drivers/gpu/drm/radeon/radeon_connectors.c b/drivers/gpu/drm/radeon/radeon_connectors.c
    > index cbfca3a..2e70be2 100644
    > --- a/drivers/gpu/drm/radeon/radeon_connectors.c
    > +++ b/drivers/gpu/drm/radeon/radeon_connectors.c
    > @@ -424,6 +424,36 @@ int radeon_connector_set_property(struct drm_connector *connector, struct drm_pr
    >        return 0;
    >  }
    >
    > +/*
    > + * Some integrated ATI Radeon chipset implementations (e. g.
    > + * Asus M2A-VM HDMI) may indicate the availability of a DDC,
    > + * even when there's no monitor connected. For these connectors
    > + * following DDC probe extension will be applied: check also for the
    > + * availability of EDID with at least a correct EDID header. Only then,
    > + * DDC is assumed to be available. This prevents drm_get_edid() and
    > + * drm_edid_block_valid() from periodically dumping data and kernel
    > + * errors into the logs and onto the terminal.
    > + */
    > +static bool radeon_connector_needs_extended_probe(struct radeon_device *dev,
    > +                                    uint32_t supported_device,
    > +                                    int connector_type)
    > +{
    > +       /* Asus M2A-VM HDMI board sends data to i2c bus even,
    > +        * if HDMI add-on card is not plugged in or HDMI is disabled in
    > +        * BIOS. Valid DDC can only be assumed, if also a valid EDID header
    > +        * can be retrieved via i2c bus during DDC probe */
    > +       if ((dev->pdev->device == 0x791e) &&
    > +           (dev->pdev->subsystem_vendor == 0x1043) &&
    > +           (dev->pdev->subsystem_device == 0x826d)) {
    > +               if ((connector_type == DRM_MODE_CONNECTOR_HDMIA) &&
    > +                   (supported_device == ATOM_DEVICE_DFP2_SUPPORT))
    > +                       return true;
    > +       }
    > +
    > +       /* Default: no EDID header probe required for DDC probing */
    > +       return false;
    > +}
    > +
    >  static void radeon_fixup_lvds_native_mode(struct drm_encoder *encoder,
    >                                          struct drm_connector *connector)
    >  {
    > @@ -655,7 +685,8 @@ radeon_vga_detect(struct drm_connector *connector, bool force)
    >                ret = connector_status_disconnected;
    >
    >        if (radeon_connector->ddc_bus)
    > -               dret = radeon_ddc_probe(radeon_connector);
    > +               dret = radeon_ddc_probe(radeon_connector,
    > +                                       radeon_connector->requires_extended_probe);
    >        if (dret) {
    >                if (radeon_connector->edid) {
    >                        kfree(radeon_connector->edid);
    > @@ -827,7 +858,8 @@ radeon_dvi_detect(struct drm_connector *connector, bool force)
    >        bool dret = false;
    >
    >        if (radeon_connector->ddc_bus)
    > -               dret = radeon_ddc_probe(radeon_connector);
    > +               dret = radeon_ddc_probe(radeon_connector,
    > +                                       radeon_connector->requires_extended_probe);
    >        if (dret) {
    >                if (radeon_connector->edid) {
    >                        kfree(radeon_connector->edid);
    > @@ -1245,7 +1277,8 @@ radeon_dp_detect(struct drm_connector *connector, bool force)
    >                                if (radeon_dp_getdpcd(radeon_connector))
    >                                        ret = connector_status_connected;
    >                        } else {
    > -                               if (radeon_ddc_probe(radeon_connector))
    > +                               if (radeon_ddc_probe(radeon_connector,
    > +                                                    radeon_connector->requires_extended_probe))
    >                                        ret = connector_status_connected;
    >                        }
    >                }
    > @@ -1400,6 +1433,9 @@ radeon_add_atom_connector(struct drm_device *dev,
    >        radeon_connector->shared_ddc = shared_ddc;
    >        radeon_connector->connector_object_id = connector_object_id;
    >        radeon_connector->hpd = *hpd;
    > +       radeon_connector->requires_extended_probe =
    > +               radeon_connector_needs_extended_probe(rdev, supported_device,
    > +                                                       connector_type);
    >        radeon_connector->router = *router;
    >        if (router->ddc_valid || router->cd_valid) {
    >                radeon_connector->router_bus = radeon_i2c_lookup(rdev, &router->i2c_info);
    > @@ -1746,6 +1782,9 @@ radeon_add_legacy_connector(struct drm_device *dev,
    >        radeon_connector->devices = supported_device;
    >        radeon_connector->connector_object_id = connector_object_id;
    >        radeon_connector->hpd = *hpd;
    > +       radeon_connector->requires_extended_probe =
    > +               radeon_connector_needs_extended_probe(rdev, supported_device,
    > +                                                       connector_type);
    >        switch (connector_type) {
    >        case DRM_MODE_CONNECTOR_VGA:
    >                drm_connector_init(dev, &radeon_connector->base, &radeon_vga_connector_funcs, connector_type);
    > diff --git a/drivers/gpu/drm/radeon/radeon_device.c b/drivers/gpu/drm/radeon/radeon_device.c
    > index 7cfaa7e..cbdac3d 100644
    > --- a/drivers/gpu/drm/radeon/radeon_device.c
    > +++ b/drivers/gpu/drm/radeon/radeon_device.c
    > @@ -704,8 +704,13 @@ int radeon_device_init(struct radeon_device *rdev,
    >        rdev->gpu_lockup = false;
    >        rdev->accel_working = false;
    >
    > -       DRM_INFO("initializing kernel modesetting (%s 0x%04X:0x%04X).\n",
    > +       if (pdev->subsystem_vendor == 0)
    > +               DRM_INFO("initializing kernel modesetting (%s 0x%04X:0x%04X).\n",
    >                radeon_family_name[rdev->family], pdev->vendor, pdev->device);
    > +       else
    > +               DRM_INFO("initializing kernel modesetting (%s 0x%04X:0x%04X 0x%04X:0x%04X).\n",
    > +               radeon_family_name[rdev->family], pdev->vendor, pdev->device,
    > +               pdev->subsystem_vendor, pdev->subsystem_device);
    >

    No need for the if block. Just always print both the pci and
    subsystem ids. Also, I'd suggest making that a separate patch as it's
    not related to the actual fix.


    >        /* mutex initialization are all done here so we
    >         * can recall function without having locking issues */
    > diff --git a/drivers/gpu/drm/radeon/radeon_display.c b/drivers/gpu/drm/radeon/radeon_display.c
    > index 292f73f..ed085ce 100644
    > --- a/drivers/gpu/drm/radeon/radeon_display.c
    > +++ b/drivers/gpu/drm/radeon/radeon_display.c
    > @@ -777,8 +777,17 @@ static int radeon_ddc_dump(struct drm_connector *connector)
    >        if (!radeon_connector->ddc_bus)
    >                return -1;
    >        edid = drm_get_edid(connector, &radeon_connector->ddc_bus->adapter);
    > +       /* Log EDID retrieval status here. In particular with regard to
    > +        * connectors with requires_extended_probe flag set, that will prevent
    > +        * function radeon_dvi_detect() to fetch EDID on this connector,
    > +        * as long as there is no valid EDID header found */
    >        if (edid) {
    > +               DRM_INFO("Radeon display connector %s: Found valid EDID",
    > +                               drm_get_connector_name(connector));
    >                kfree(edid);
    > +       } else {
    > +               DRM_INFO("Radeon display connector %s: No monitor connected or invalid EDID",
    > +                               drm_get_connector_name(connector));
    >        }
    >        return ret;
    >  }
    > diff --git a/drivers/gpu/drm/radeon/radeon_i2c.c b/drivers/gpu/drm/radeon/radeon_i2c.c
    > index 781196d..6c111c1 100644
    > --- a/drivers/gpu/drm/radeon/radeon_i2c.c
    > +++ b/drivers/gpu/drm/radeon/radeon_i2c.c
    > @@ -32,17 +32,17 @@
    >  * radeon_ddc_probe
    >  *
    >  */
    > -bool radeon_ddc_probe(struct radeon_connector *radeon_connector)
    > +bool radeon_ddc_probe(struct radeon_connector *radeon_connector, bool requires_extended_probe)
    >  {
    > -       u8 out_buf[] = { 0x0, 0x0};
    > -       u8 buf[2];
    > +       u8 out = 0x0;
    > +       u8 buf[8];
    >        int ret;
    >        struct i2c_msg msgs[] = {
    >                {
    >                        .addr = 0x50,
    >                        .flags = 0,
    >                        .len = 1,
    > -                       .buf = out_buf,
    > +                       .buf = &out,
    >                },
    >                {
    >                        .addr = 0x50,
    > @@ -52,15 +52,31 @@ bool radeon_ddc_probe(struct radeon_connector *radeon_connector)
    >                }
    >        };
    >
    > +       /* Read 8 bytes from i2c for extended probe of EDID header */
    > +       if (requires_extended_probe)
    > +               msgs[1].len = 8;
    > +
    >        /* on hw with routers, select right port */
    >        if (radeon_connector->router.ddc_valid)
    >                radeon_router_select_ddc_port(radeon_connector);
    >
    >        ret = i2c_transfer(&radeon_connector->ddc_bus->adapter, msgs, 2);
    > -       if (ret == 2)
    > -               return true;
    > -
    > -       return false;
    > +       if (ret != 2)
    > +               /* Couldn't find an accessible DDC on this connector */
    > +               return false;
    > +       if (requires_extended_probe) {
    > +               /* 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
    > +                * drm_edid_block_valid() can fix the last 2 bytes */
    > +               if (drm_edid_header_is_valid(buf) < 6) {
    > +                       /* Couldn't find an accessible EDID on this
    > +                        * connector */
    > +                       return false;
    > +               }
    > +       }
    > +       return true;
    >  }
    >
    >  /* bit banging i2c */
    > diff --git a/drivers/gpu/drm/radeon/radeon_mode.h b/drivers/gpu/drm/radeon/radeon_mode.h
    > index 6df4e3c..d09031c 100644
    > --- a/drivers/gpu/drm/radeon/radeon_mode.h
    > +++ b/drivers/gpu/drm/radeon/radeon_mode.h
    > @@ -438,6 +438,9 @@ struct radeon_connector {
    >        struct radeon_i2c_chan *ddc_bus;
    >        /* some systems have an hdmi and vga port with a shared ddc line */
    >        bool shared_ddc;
    > +       /* for some Radeon chip families we apply an additional EDID header
    > +          check as part of the DDC probe */
    > +       bool requires_extended_probe;
    >        bool use_digital;
    >        /* we need to mind the EDID between detect
    >           and get modes due to analog/digital/tvencoder */
    > @@ -514,7 +517,8 @@ extern void radeon_i2c_put_byte(struct radeon_i2c_chan *i2c,
    >                                u8 val);
    >  extern void radeon_router_select_ddc_port(struct radeon_connector *radeon_connector);
    >  extern void radeon_router_select_cd_port(struct radeon_connector *radeon_connector);
    > -extern bool radeon_ddc_probe(struct radeon_connector *radeon_connector);
    > +extern bool radeon_ddc_probe(struct radeon_connector *radeon_connector,
    > +                       bool requires_extended_probe);
    >  extern int radeon_ddc_get_modes(struct radeon_connector *radeon_connector);
    >
    >  extern struct drm_encoder *radeon_best_encoder(struct drm_connector *connector);
    > diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
    > index 33d12f8..0ec3687 100644
    > --- a/include/drm/drm_crtc.h
    > +++ b/include/drm/drm_crtc.h
    > @@ -802,6 +802,7 @@ extern struct drm_display_mode *drm_gtf_mode_complex(struct drm_device *dev,
    >  extern int drm_add_modes_noedid(struct drm_connector *connector,
    >                                int hdisplay, int vdisplay);
    >
    > +extern int drm_edid_header_is_valid(const u8 *raw_edid);
    >  extern bool drm_edid_is_valid(struct edid *edid);
    >  struct drm_display_mode *drm_mode_find_dmt(struct drm_device *dev,
    >                                           int hsize, int vsize, int fresh);
    > --
    > 1.7.1
    >
    >
    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

    \
     
     \ /
      Last update: 2011-07-06 17:41    [W:0.065 / U:117.148 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site