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: Mon, 12 Oct 2015 14:59:08 -0400	[thread overview]
Message-ID: <CADnq5_NmF4DuzFqGK5e1FNDCTb4TnqV+OEn+BnhB3Hz=KWQTpg@mail.gmail.com> (raw)
In-Reply-To: <20151008145345.GA15763@wunner.de>

On Thu, Oct 8, 2015 at 10:53 AM, Lukas Wunner <lukas@wunner.de> wrote:
> Hi Alex,
>
> On Wed, Oct 07, 2015 at 11:14:43PM -0400, Alex Deucher wrote:
>> 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.
>
> It hasn't been tested on such a machine because I don't have one and
> don't know anyone who has one.
>
> However I would be surprised if the series broke anything on non-Apple
> laptops since apple-gmux is the only handler declaring a ->switch_ddc
> callback. If that callback is not defined, the behaviour should be
> identical to just calling drm_get_edid().

Ah, I missed that part.  There is an ATPX method to switch ddc
separately from the encoder data path.  That could probably be hooked
up as well at some point if someone wants to test it.

>
> (With the only exception that EDID reads are serialized on LVDS connectors
> due to the locking in vga_switcheroo. And that should be irrelevant since
> laptops usually have only *one* LVDS panel.)
>
>
>> 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?
>
> As noted in the commit message above, the *retina* MacBook Pro uses eDP
> instead of LVDS and is not capable of switching AUX separately from the
> main link. I've gotten this to work by proxying the AUX communication
> via the currently active GPU, but the patches for this are still in a
> very experimental state. They will be submitted in a future series.
>
> The drivers will then be amended to call drm_get_edid_switcheroo()
> for eDP connectors, and drm_get_edid_switcheroo() will be amended
> by the proxying code.
>
> There will probably also be separate proxying-enabled helpers for
> drm_dp_dpcd_read() and _write() which the drivers need to call for
> eDP connectors (right now the proxying code is hacked directly into
> those two functions).

OK.

>
>
>> 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?
>
> As far as the MacBook Pro is concerned, only the earliest two generations
> support this (MacBookPro5 2008/09 with dual Nvidia GPUs and MacBookPro6
> 2010 with Intel/Nvidia). They have a single external switchable DP port.
>
> I was thinking about hardcoding the DMIs of these few laptops in the
> drivers and call drm_get_edid_switcheroo() for the DP port. But this
> is not a high priority item. I don't have a clear plan yet.
>

OK.

>
>> 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.
>
> I assume you're referring to:
> http://lists.freedesktop.org/archives/dri-devel/2014-November/072032.html
> https://bugzilla.opensuse.org/show_bug.cgi?id=904417
>
> After perusing the bug report at length I'm under the impression that
> it's not GNOME getting confused by two LVDS outputs, rather this seems
> like a runtime pm issue since radeon.runpm=0 seemed to fix it.
>
> Maybe gdm was triggering an ioctl() that didn't wake up the GPU?
> I've briefly looked at the code and noticed that radeon_compat_ioctl()
> doesn't call pm_runtime_get_sync(). Maybe the reporter of the bug had
> a 32 bit GNOME installed? Or maybe the GPU failed to wake up from D0
> for some reason? There's no dmesg output attached to the bug report
> so it's impossible to tell.

I'm guessing it was an issue with gnome attempting to decide if it
should start at all.  We've had a lot of problems with gnome with how
it mis-detects hw vs sw rendering and seems to fail to start even
though everything works.

>
> It's unfortunate that the actual root cause was not clearly identified.
> In my opinion 134857943356 should be reverted and replaced with a proper
> fix. I don't understand why this got into mainline in the first place,
> you've (astutely) described this solution as a hack in the above-linked
> thread.

We'll it's not really nice to regress users if you can avoid it.  I
never really had the time to dig into issues with muxed hybrid laptops
especially since pretty much everything went muxless a while ago.

Alex

>
> Thank you and best regards,
>
> Lukas
>
>>
>> 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