Re: [PATCH v3 6/6] drm/radeon: Switch DDC when reading the EDID

From: Alex Deucher <alexdeucher@gmail.com>
To: Lukas Wunner <lukas@wunner.de>
Cc: William Brown <william@blackhats.net.au>,
	Maling list - DRI developers <dri-devel@lists.freedesktop.org>
Subject: Re: [PATCH v3 6/6] drm/radeon: Switch DDC when reading the EDID
Date: Wed, 7 Oct 2015 23:14:43 -0400	[thread overview]
Message-ID: <CADnq5_P3bfj_w+FAmpwbU3kkgS4g4bX=zP1cOhvt+aqRJhnBrw@mail.gmail.com> (raw)
In-Reply-To: <dd957202c7c113a09414a27c598daf358ee5d068.1443952353.git.lukas@wunner.de>

On Sat, Sep 12, 2015 at 3:44 AM, Lukas Wunner <lukas@wunner.de> wrote:
> The pre-retina MacBook Pro uses an LVDS panel and a gmux controller
> to switch the panel between its two GPUs. The panel mode in VBIOS
> is notoriously bogus on these machines.
>
> Use drm_get_edid_switcheroo() in lieu of drm_get_edid() on LVDS.
> This allows us to retrieve the EDID if the outputs are currently
> muxed to the integrated GPU by temporarily switching the panel's
> DDC lines to the discrete GPU.
>
> This only enables EDID probing on the pre-retina MBP (2008 - 2013).
> The retina MBP (2012 - present) uses eDP and gmux is apparently not
> capable of switching AUX separately from the main link on these models.
> This will be addressed in later patches.
>
> List of pre-retina MBPs with dual GPUs, one of them AMD:
>     [MBP  8,2 2011  intel SNB + amd turks     pre-retina  15"]
>     [MBP  8,3 2011  intel SNB + amd turks     pre-retina  17"]
>
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=61115
> Tested-by: William Brown <william@blackhats.net.au>
>     [MBP  8,2 2011  intel SNB + amd turks     pre-retina  15"]
>
> Signed-off-by: Lukas Wunner <lukas@wunner.de>

I'm not opposed to this series, but I have a few questions.

1.  Has any of this been tested on non-Apple hybrid laptops to make
sure nothing has regressed?  I think it would be good to verify that
since there are more of them in the wild.
2.  This only does the ddc switching for LVDS.  Newer systems almost
certainly use DP (either eDP or DP to LVDS bridges).  What about those
systems?
3.  Most muxed hybrid laptops also mux external displays connectors as
well (VGA, DVI, HDMI, DP, etc.).  Do you have any plans to extend this
to those cases?
4. Some desktop environments (GNOME IIRC, but possibly KDE as well)
rely on the fact that ddc doesn't work on one of the GPUs when it's
not selected.  They don't know how to deal with muxes and can't deal
with the same port showing up as connected on two GPUs.  I suspect
this patch set may break that.  radeon has always been able to report
the panel information on hybrid laptops even when the mux is not
switched since the info is also stored in the vbios.  At the behest of
those desktop environments there is actually code in the driver to not
report the edid for the unselected gpu on hybrid laptops even though
we could report it.  I suspect this patch may break that.

Alex

> ---
>  drivers/gpu/drm/radeon/radeon_connectors.c | 4 ++++
>  1 file changed, 4 insertions(+)
>
> diff --git a/drivers/gpu/drm/radeon/radeon_connectors.c b/drivers/gpu/drm/radeon/radeon_connectors.c
> index 5a2cafb..569f63c 100644
> --- a/drivers/gpu/drm/radeon/radeon_connectors.c
> +++ b/drivers/gpu/drm/radeon/radeon_connectors.c
> @@ -344,6 +344,10 @@ static void radeon_connector_get_edid(struct drm_connector *connector)
>                 else if (radeon_connector->ddc_bus)
>                         radeon_connector->edid = drm_get_edid(&radeon_connector->base,
>                                                               &radeon_connector->ddc_bus->adapter);
> +       } else if (connector->connector_type == DRM_MODE_CONNECTOR_LVDS &&
> +                  radeon_connector->ddc_bus) {
> +               radeon_connector->edid = drm_get_edid_switcheroo(&radeon_connector->base,
> +                                                                &radeon_connector->ddc_bus->adapter);
>         } else if (radeon_connector->ddc_bus) {
>                 radeon_connector->edid = drm_get_edid(&radeon_connector->base,
>                                                       &radeon_connector->ddc_bus->adapter);
> --
> 1.8.5.2 (Apple Git-48)
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel