On Wed, Aug 28, 2019 at 03:09:07PM +0800, Brad Campbell wrote:
> G'day All,
Hi,
> 5.2 is the first kernel that has allowed me to use 2 Apple Thunderbolt
> display on a 2011 vintage iMac. Awesome work and many thanks!
>
> I boot this machine in BIOS (Bootcamp) mode as that gave me brightness
> control over the internal panel.
>
> I've been using it with a Thunderbolt display and a standard DVI display
> since 2012 as the Apple firmware would only set up a single Thunderbolt
> display at bootup. Didn't find that out until I'd bought a second one and
> kept it around as a spare.
>
> When I saw the changelog for 5.2 I thought I'd see if it would run 2
> Thnunderbolt displays, and it does after a minor fiddle. I have tried this
> with the Kanex adapter unplugged just for completeness. No change.
>
> When I boot the machine it picks up both displays :
>
> [ 1.072080] thunderbolt 0-1: new device found, vendor=0x1 device=0x8002
> [ 1.072086] thunderbolt 0-1: Apple, Inc. Thunderbolt Display
> [ 1.392076] random: crng init done
> [ 1.595609] thunderbolt 0-3: new device found, vendor=0x1 device=0x8002
> [ 1.595615] thunderbolt 0-3: Apple, Inc. Thunderbolt Display
> [ 1.905117] thunderbolt 0-303: new device found, vendor=0xa7 device=0x4
> [ 1.905121] thunderbolt 0-303: Kanex Thunderbolt to eSATA+USB3.0 Adapter
> [ 1.910098] thunderbolt 0000:07:00.0: 0:c: path does not end on a DP
> adapter, cleaning up
>
> Display 0-3 works. Display 0-1 remains blank.
>
> If I unplug 0-1 and re-plug it, it is detected and comes up perfectly.
Can you add "thunderbolt.dyndbg" to the kernel command line and
reproduce this? That gives bit more information what might be happening.
I'm suspecting that the boot firmware does configure second DP path also
and we either fail to discover it properly or the boot firmware fails to
set it up.
Also if you boot with one monitor connected and then connect another
(when the system is up) does it work then?
On 28/8/19 3:33 pm, Mika Westerberg wrote:
> On Wed, Aug 28, 2019 at 03:09:07PM +0800, Brad Campbell wrote:
>> G'day All,
>
> Hi,
>
>> 5.2 is the first kernel that ha/thunderbolt
s allowed me to use 2 Apple Thunderbolt
>> display on a 2011 vintage iMac. Awesome work and many thanks!
>>
>> I boot this machine in BIOS (Bootcamp) mode as that gave me brightness
>> control over the internal panel.
>>
>> I've been using it with a Thunderbolt display and a standard DVI display
>> since 2012 as the Apple firmware would only set up a single Thunderbolt
>> display at bootup. Didn't find that out until I'd bought a second one and
>> kept it around as a spare.
>>
>> When I saw the changelog for 5.2 I thought I'd see if it would run 2
>> Thnunderbolt displays, and it does after a minor fiddle. I have tried this
>> with the Kanex adapter unplugged just for completeness. No change.
>>
>> When I boot the machine it picks up both displays :
>>
>> [ 1.072080] thunderbolt 0-1: new device found, vendor=0x1 device=0x8002
>> [ 1.072086] thunderbolt 0-1: Apple, Inc. Thunderbolt Display
>> [ 1.392076] random: crng init done
>> [ 1.595609] thunderbolt 0-3: new device found, vendor=0x1 device=0x8002
>> [ 1.595615] thunderbolt 0-3: Apple, Inc. Thunderbolt Display
>> [ 1.905117] thunderbolt 0-303: new device found, vendor=0xa7 device=0x4
>> [ 1.905121] thunderbolt 0-303: Kanex Thunderbolt to eSATA+USB3.0 Adapter
>> [ 1.910098] thunderbolt 0000:07:00.0: 0:c: path does not end on a DP
>> adapter, cleaning up
>>
>> Display 0-3 works. Display 0-1 remains blank.
>>
>> If I unplug 0-1 and re-plug it, it is detected and comes up perfectly.
>
> Can you add "thunderbolt.dyndbg" to the kernel command line and
> reproduce this? That gives bit more information what might be happening.
dmesg.03 attached.
> I'm suspecting that the boot firmware does configure second DP path also
> and we either fail to discover it properly or the boot firmware fails to
> set it up.
>
> Also if you boot with one monitor connected and then connect another
> (when the system is up) does it work then?
Umm.. so this is where it gets weird. No it doesn't. Apparently it fails
to configure the first monitor it finds. This is the one the Apple
bootcamp firmware configures at boot.
So if I disconnect 0-1 and boot up, it detects 0-3 but it doesn't work
(same way 0-1 doesn't work). A plug-unplug brings that up and then
plugging in 0-1 brings that up too.
I then did a few swapsies trying to get the heads in the right order and
gave up and rebooted.
This is in dmesg.4 attached.
dmesg.3 is the straight boot with dyndbg enabled and both heads plugged
in. dmesg.4 is with 0-3 plugged in only. It comes up dead and then I
play swapsies getting things working.
Took me a while to figure out I didn't have CONFIG_DYNAMIC_DEBUG in the
kernel.
Now it makes sense. I used to have a Thunderbolt display and a DVI
monitor with an adapter. < v5.2 both of those worked just fine. When I
tried 5.2 I had to unplug/replug the Thunderbolt display to make it
work. When I added the second Thunderbolt display, it took the place of
the DVI display and is the first one the firmware tries to configure.
I wonder if I boot it up using EFI the firmware won't try and configure
either of them, but then I'll lose my backlight control. I'll give that
a go anyway.
Regards,
Brad
On 28/8/19 5:12 pm, Brad Campbell wrote:
> On 28/8/19 3:33 pm, Mika Westerberg wrote:
>> I'm suspecting that the boot firmware does configure second DP path also
>> and we either fail to discover it properly or the boot firmware fails to
>> set it up.
>>
>> Also if you boot with one monitor connected and then connect another
>> (when the system is up) does it work then?
>
> Umm.. so this is where it gets weird. No it doesn't. Apparently it fails
> to configure the first monitor it finds. This is the one the Apple
> bootcamp firmware configures at boot.
>
Ok just to clarify it appears I've been booting EFI and bypassing the
Bootcamp BIOS emulation for a few years. So, that made no difference.
On Wed, Aug 28, 2019 at 05:12:00PM +0800, Brad Campbell wrote:
> On 28/8/19 3:33 pm, Mika Westerberg wrote:
> > On Wed, Aug 28, 2019 at 03:09:07PM +0800, Brad Campbell wrote:
> > > G'day All,
> >
> > Hi,
> >
> > > 5.2 is the first kernel that ha/thunderbolt
>
> s allowed me to use 2 Apple Thunderbolt
> > > display on a 2011 vintage iMac. Awesome work and many thanks!
> > >
> > > I boot this machine in BIOS (Bootcamp) mode as that gave me brightness
> > > control over the internal panel.
> > >
> > > I've been using it with a Thunderbolt display and a standard DVI display
> > > since 2012 as the Apple firmware would only set up a single Thunderbolt
> > > display at bootup. Didn't find that out until I'd bought a second one and
> > > kept it around as a spare.
> > >
> > > When I saw the changelog for 5.2 I thought I'd see if it would run 2
> > > Thnunderbolt displays, and it does after a minor fiddle. I have tried this
> > > with the Kanex adapter unplugged just for completeness. No change.
> > >
> > > When I boot the machine it picks up both displays :
> > >
> > > [ 1.072080] thunderbolt 0-1: new device found, vendor=0x1 device=0x8002
> > > [ 1.072086] thunderbolt 0-1: Apple, Inc. Thunderbolt Display
> > > [ 1.392076] random: crng init done
> > > [ 1.595609] thunderbolt 0-3: new device found, vendor=0x1 device=0x8002
> > > [ 1.595615] thunderbolt 0-3: Apple, Inc. Thunderbolt Display
> > > [ 1.905117] thunderbolt 0-303: new device found, vendor=0xa7 device=0x4
> > > [ 1.905121] thunderbolt 0-303: Kanex Thunderbolt to eSATA+USB3.0 Adapter
> > > [ 1.910098] thunderbolt 0000:07:00.0: 0:c: path does not end on a DP
> > > adapter, cleaning up
> > >
> > > Display 0-3 works. Display 0-1 remains blank.
> > >
> > > If I unplug 0-1 and re-plug it, it is detected and comes up perfectly.
> >
> > Can you add "thunderbolt.dyndbg" to the kernel command line and
> > reproduce this? That gives bit more information what might be happening.
>
> dmesg.03 attached.
>
> > I'm suspecting that the boot firmware does configure second DP path also
> > and we either fail to discover it properly or the boot firmware fails to
> > set it up.
> >
> > Also if you boot with one monitor connected and then connect another
> > (when the system is up) does it work then?
>
> Umm.. so this is where it gets weird. No it doesn't. Apparently it fails to
> configure the first monitor it finds. This is the one the Apple bootcamp
> firmware configures at boot.
>
> So if I disconnect 0-1 and boot up, it detects 0-3 but it doesn't work (same
> way 0-1 doesn't work). A plug-unplug brings that up and then plugging in 0-1
> brings that up too.
>
> I then did a few swapsies trying to get the heads in the right order and
> gave up and rebooted.
>
> This is in dmesg.4 attached.
>
> dmesg.3 is the straight boot with dyndbg enabled and both heads plugged in.
> dmesg.4 is with 0-3 plugged in only. It comes up dead and then I play
> swapsies getting things working.
>
> Took me a while to figure out I didn't have CONFIG_DYNAMIC_DEBUG in the
> kernel.
Right, I should have mentioned that.
Apart from the warning in the log (which is not fatal, I'll look into
it) to me the second path setup looks fine.
Can you do one more experiment? Boot the system up without anything
connected and then plug both monitors. Does it work?
On 28/8/19 6:23 pm, Mika Westerberg wrote:
> On Wed, Aug 28, 2019 at 05:12:00PM +0800, Brad Campbell wrote:
>
> Apart from the warning in the log (which is not fatal, I'll look into
> it) to me the second path setup looks fine.
>
> Can you do one more experiment? Boot the system up without anything
> connected and then plug both monitors. Does it work?
>
Aside from head ordering issues in X it works just fine.
I've attached the dmesg. Boot with nothing plugged in, then plug in 0-1
and 0-3 in that order.
Regards,
Brad
On Wed, Aug 28, 2019 at 06:43:35PM +0800, Brad Campbell wrote:
> On 28/8/19 6:23 pm, Mika Westerberg wrote:
>
> > On Wed, Aug 28, 2019 at 05:12:00PM +0800, Brad Campbell wrote:
> >
> > Apart from the warning in the log (which is not fatal, I'll look into
> > it) to me the second path setup looks fine.
> >
> > Can you do one more experiment? Boot the system up without anything
> > connected and then plug both monitors. Does it work?
> >
>
> Aside from head ordering issues in X it works just fine.
> I've attached the dmesg. Boot with nothing plugged in, then plug in 0-1 and
> 0-3 in that order.
OK, thanks for checking. So when Linux is in complete control both DP
tunnels get created properly. I suspect there is something different
what the firmware does compared to other Macs I've been using and that
causes the driver to fail to discover all the paths. I will take a look
at this but I'm away tomorrow and friday so it goes to next week.
BTW, have you tried to chain the two monitors?
On 28/8/19 21:19, Mika Westerberg wrote:
> On Wed, Aug 28, 2019 at 06:43:35PM +0800, Brad Campbell wrote:
>> On 28/8/19 6:23 pm, Mika Westerberg wrote:
>>
>>> On Wed, Aug 28, 2019 at 05:12:00PM +0800, Brad Campbell wrote:
>>>
>>> Apart from the warning in the log (which is not fatal, I'll look into
>>> it) to me the second path setup looks fine.
>>>
>>> Can you do one more experiment? Boot the system up without anything
>>> connected and then plug both monitors. Does it work?
>>>
>> Aside from head ordering issues in X it works just fine.
>> I've attached the dmesg. Boot with nothing plugged in, then plug in 0-1 and
>> 0-3 in that order.
> OK, thanks for checking. So when Linux is in complete control both DP
> tunnels get created properly. I suspect there is something different
> what the firmware does compared to other Macs I've been using and that
> causes the driver to fail to discover all the paths. I will take a look
> at this but I'm away tomorrow and friday so it goes to next week.
It wouldn't surprise me if the firmware was doing something funky. It
was one of the first Thunderbolt equipped models and the support docs
explicitly say only one Thunderbolt display in Windows and two in later
versions of OSX. It almost needs a quirk to say "firmware does something
we don't like, reset the controller and re-discover from scratch".
Anyway, I'm not in any hurry. It doesn't get rebooted often and it's not
in any way preventing me using the machine. In fact, upgrading the third
head from an old 24" 1920x1200 to the second Thunderbolt display has
been invaluable.
> BTW, have you tried to chain the two monitors?
Not yet, but it was something I've been considering. I'll give it a go
tomorrow. Due to cable length vs display dimensions it'll require me
cleaning my desk, and that takes some forward planning.
--
An expert is a person who has found out by his own painful
experience all the mistakes that one can make in a very
narrow field. - Niels Bohr
Hi Brad,
On Thu, Aug 29, 2019 at 12:27:08AM +0800, Brad Campbell wrote:
> It wouldn't surprise me if the firmware was doing something funky. It was
> one of the first Thunderbolt equipped models and the support docs explicitly
> say only one Thunderbolt display in Windows and two in later versions of
> OSX. It almost needs a quirk to say "firmware does something we don't like,
> reset the controller and re-discover from scratch".
>
> Anyway, I'm not in any hurry. It doesn't get rebooted often and it's not in
> any way preventing me using the machine. In fact, upgrading the third head
> from an old 24" 1920x1200 to the second Thunderbolt display has been
> invaluable.
Can you apply the below patch and then boot with two monitors connected?
Then send me the dmesg. It does not fix anything but should log a bit
more.
diff --git a/drivers/thunderbolt/tb.c b/drivers/thunderbolt/tb.c
index 1f7a9e1cc09c..28a72336558a 100644
--- a/drivers/thunderbolt/tb.c
+++ b/drivers/thunderbolt/tb.c
@@ -313,8 +313,10 @@ static struct tb_port *tb_find_unused_port(struct tb_switch *sw,
continue;
if (!sw->ports[i].cap_adap)
continue;
- if (tb_port_is_enabled(&sw->ports[i]))
+ if (tb_port_is_enabled(&sw->ports[i])) {
+ tb_port_dbg(&sw->ports[i], "this already enabled\n");
continue;
+ }
return &sw->ports[i];
}
return NULL;
@@ -365,16 +367,25 @@ static int tb_tunnel_dp(struct tb *tb, struct tb_port *out)
struct tb_tunnel *tunnel;
struct tb_port *in;
- if (tb_port_is_enabled(out))
+ tb_port_dbg(out, "trying to tunnel DP\n");
+
+ if (tb_port_is_enabled(out)) {
+ tb_port_dbg(out, "DP OUT port already enabled\n");
return 0;
+ }
+
+ tb_port_dbg(out, "finding free DP IN port\n");
do {
sw = tb_to_switch(sw->dev.parent);
if (!sw)
return 0;
+ tb_sw_dbg(sw, "finding available DP IN\n");
in = tb_find_unused_port(sw, TB_TYPE_DP_HDMI_IN);
} while (!in);
+ tb_port_dbg(in, "found DP IN\n");
+
tunnel = tb_tunnel_alloc_dp(tb, in, out);
if (!tunnel) {
tb_port_dbg(out, "DP tunnel allocation failed\n");
diff --git a/drivers/thunderbolt/tunnel.c b/drivers/thunderbolt/tunnel.c
index 5a99234826e7..93c2c965bdde 100644
--- a/drivers/thunderbolt/tunnel.c
+++ b/drivers/thunderbolt/tunnel.c
@@ -351,9 +351,23 @@ struct tb_tunnel *tb_tunnel_discover_dp(struct tb *tb, struct tb_port *in)
struct tb_tunnel *tunnel;
struct tb_port *port;
struct tb_path *path;
+ u32 data[2];
+ int ret;
+
+ tb_port_dbg(in, "start DP discover\n");
- if (!tb_dp_port_is_enabled(in))
+ if (!tb_dp_port_is_enabled(in)) {
+ tb_port_dbg(in, "DP port enabled\n");
return NULL;
+ }
+
+ ret = tb_port_read(in, data, TB_CFG_PORT, in->cap_adap,
+ ARRAY_SIZE(data));
+ if (ret)
+ return NULL;
+
+ tb_port_dbg(in, "data[0]=0x%08x\n", data[0]);
+ tb_port_dbg(in, "data[1]=0x%08x\n", data[1]);
tunnel = tb_tunnel_alloc(tb, 3, TB_TUNNEL_DP);
if (!tunnel)
On Tue, Sep 03, 2019 at 01:13:28PM +0300, Mika Westerberg wrote:
> Hi Brad,
>
> On Thu, Aug 29, 2019 at 12:27:08AM +0800, Brad Campbell wrote:
> > It wouldn't surprise me if the firmware was doing something funky. It was
> > one of the first Thunderbolt equipped models and the support docs explicitly
> > say only one Thunderbolt display in Windows and two in later versions of
> > OSX. It almost needs a quirk to say "firmware does something we don't like,
> > reset the controller and re-discover from scratch".
> >
> > Anyway, I'm not in any hurry. It doesn't get rebooted often and it's not in
> > any way preventing me using the machine. In fact, upgrading the third head
> > from an old 24" 1920x1200 to the second Thunderbolt display has been
> > invaluable.
>
> Can you apply the below patch and then boot with two monitors connected?
> Then send me the dmesg. It does not fix anything but should log a bit
> more.
>
> diff --git a/drivers/thunderbolt/tb.c b/drivers/thunderbolt/tb.c
> index 1f7a9e1cc09c..28a72336558a 100644
> --- a/drivers/thunderbolt/tb.c
> +++ b/drivers/thunderbolt/tb.c
> @@ -313,8 +313,10 @@ static struct tb_port *tb_find_unused_port(struct tb_switch *sw,
> continue;
> if (!sw->ports[i].cap_adap)
> continue;
> - if (tb_port_is_enabled(&sw->ports[i]))
> + if (tb_port_is_enabled(&sw->ports[i])) {
> + tb_port_dbg(&sw->ports[i], "this already enabled\n");
> continue;
> + }
> return &sw->ports[i];
> }
> return NULL;
> @@ -365,16 +367,25 @@ static int tb_tunnel_dp(struct tb *tb, struct tb_port *out)
> struct tb_tunnel *tunnel;
> struct tb_port *in;
>
> - if (tb_port_is_enabled(out))
> + tb_port_dbg(out, "trying to tunnel DP\n");
> +
> + if (tb_port_is_enabled(out)) {
> + tb_port_dbg(out, "DP OUT port already enabled\n");
> return 0;
> + }
> +
> + tb_port_dbg(out, "finding free DP IN port\n");
>
> do {
> sw = tb_to_switch(sw->dev.parent);
> if (!sw)
> return 0;
> + tb_sw_dbg(sw, "finding available DP IN\n");
> in = tb_find_unused_port(sw, TB_TYPE_DP_HDMI_IN);
> } while (!in);
>
> + tb_port_dbg(in, "found DP IN\n");
> +
> tunnel = tb_tunnel_alloc_dp(tb, in, out);
> if (!tunnel) {
> tb_port_dbg(out, "DP tunnel allocation failed\n");
> diff --git a/drivers/thunderbolt/tunnel.c b/drivers/thunderbolt/tunnel.c
> index 5a99234826e7..93c2c965bdde 100644
> --- a/drivers/thunderbolt/tunnel.c
> +++ b/drivers/thunderbolt/tunnel.c
> @@ -351,9 +351,23 @@ struct tb_tunnel *tb_tunnel_discover_dp(struct tb *tb, struct tb_port *in)
> struct tb_tunnel *tunnel;
> struct tb_port *port;
> struct tb_path *path;
> + u32 data[2];
> + int ret;
> +
> + tb_port_dbg(in, "start DP discover\n");
>
> - if (!tb_dp_port_is_enabled(in))
> + if (!tb_dp_port_is_enabled(in)) {
> + tb_port_dbg(in, "DP port enabled\n");
I did typo here so while you try apply the patch, please change this one
to say "DP port not enabled" instead.
G'day Mika,
I'm sending you two dmesg because with that patch applied, a warm boot
(as in reboot) comes up with all 3 heads, but a cold boot only has the
two. I thought that was odd, so I reproduced it a couple of times just
to check.
So cold boot is dmesg.07 and warm boot is dmesg.06. If I cold boot I get
2 displays. A reboot from there results in all 3.
I compiled, installed and rebooted into all 3 heads (which somewhat
threw me, so I tried it a couple of times).
So that patch certainly made a difference. Timing/race issue?
Regards,
Brad
On 3/9/19 6:13 pm, Mika Westerberg wrote:
> Hi Brad,
>
> On Thu, Aug 29, 2019 at 12:27:08AM +0800, Brad Campbell wrote:
>> It wouldn't surprise me if the firmware was doing something funky. It was
>> one of the first Thunderbolt equipped models and the support docs explicitly
>> say only one Thunderbolt display in Windows and two in later versions of
>> OSX. It almost needs a quirk to say "firmware does something we don't like,
>> reset the controller and re-discover from scratch".
>>
>> Anyway, I'm not in any hurry. It doesn't get rebooted often and it's not in
>> any way preventing me using the machine. In fact, upgrading the third head
>> from an old 24" 1920x1200 to the second Thunderbolt display has been
>> invaluable.
>
> Can you apply the below patch and then boot with two monitors connected?
> Then send me the dmesg. It does not fix anything but should log a bit
> more.
>
> diff --git a/drivers/thunderbolt/tb.c b/drivers/thunderbolt/tb.c
> index 1f7a9e1cc09c..28a72336558a 100644
> --- a/drivers/thunderbolt/tb.c
> +++ b/drivers/thunderbolt/tb.c
> @@ -313,8 +313,10 @@ static struct tb_port *tb_find_unused_port(struct tb_switch *sw,
> continue;
> if (!sw->ports[i].cap_adap)
> continue;
> - if (tb_port_is_enabled(&sw->ports[i]))
> + if (tb_port_is_enabled(&sw->ports[i])) {
> + tb_port_dbg(&sw->ports[i], "this already enabled\n");
> continue;
> + }
> return &sw->ports[i];
> }
> return NULL;
> @@ -365,16 +367,25 @@ static int tb_tunnel_dp(struct tb *tb, struct tb_port *out)
> struct tb_tunnel *tunnel;
> struct tb_port *in;
>
> - if (tb_port_is_enabled(out))
> + tb_port_dbg(out, "trying to tunnel DP\n");
> +
> + if (tb_port_is_enabled(out)) {
> + tb_port_dbg(out, "DP OUT port already enabled\n");
> return 0;
> + }
> +
> + tb_port_dbg(out, "finding free DP IN port\n");
>
> do {
> sw = tb_to_switch(sw->dev.parent);
> if (!sw)
> return 0;
> + tb_sw_dbg(sw, "finding available DP IN\n");
> in = tb_find_unused_port(sw, TB_TYPE_DP_HDMI_IN);
> } while (!in);
>
> + tb_port_dbg(in, "found DP IN\n");
> +
> tunnel = tb_tunnel_alloc_dp(tb, in, out);
> if (!tunnel) {
> tb_port_dbg(out, "DP tunnel allocation failed\n");
> diff --git a/drivers/thunderbolt/tunnel.c b/drivers/thunderbolt/tunnel.c
> index 5a99234826e7..93c2c965bdde 100644
> --- a/drivers/thunderbolt/tunnel.c
> +++ b/drivers/thunderbolt/tunnel.c
> @@ -351,9 +351,23 @@ struct tb_tunnel *tb_tunnel_discover_dp(struct tb *tb, struct tb_port *in)
> struct tb_tunnel *tunnel;
> struct tb_port *port;
> struct tb_path *path;
> + u32 data[2];
> + int ret;
> +
> + tb_port_dbg(in, "start DP discover\n");
>
> - if (!tb_dp_port_is_enabled(in))
> + if (!tb_dp_port_is_enabled(in)) {
> + tb_port_dbg(in, "DP port enabled\n");
> return NULL;
> + }
> +
> + ret = tb_port_read(in, data, TB_CFG_PORT, in->cap_adap,
> + ARRAY_SIZE(data));
> + if (ret)
> + return NULL;
> +
> + tb_port_dbg(in, "data[0]=0x%08x\n", data[0]);
> + tb_port_dbg(in, "data[1]=0x%08x\n", data[1]);
>
> tunnel = tb_tunnel_alloc(tb, 3, TB_TUNNEL_DP);
> if (!tunnel)
>
On Tue, Sep 03, 2019 at 07:11:32PM +0800, Brad Campbell wrote:
> G'day Mika,
>
> I'm sending you two dmesg because with that patch applied, a warm boot (as
> in reboot) comes up with all 3 heads, but a cold boot only has the two. I
> thought that was odd, so I reproduced it a couple of times just to check.
>
> So cold boot is dmesg.07 and warm boot is dmesg.06. If I cold boot I get 2
> displays. A reboot from there results in all 3.
>
> I compiled, installed and rebooted into all 3 heads (which somewhat threw
> me, so I tried it a couple of times).
>
> So that patch certainly made a difference. Timing/race issue?
I think the problem is that for some reason, probably because this is
first generation hardware with all the bugs included, you cannot read
the second dword from DP adapter path config space (you can write it
though).
I've updated the patch so that it reads only the first dword when it
discovers paths and also when it disables them. Can you try it out and
see if it makes a difference? This should also get rid of the warnings
you get.
diff --git a/drivers/thunderbolt/path.c b/drivers/thunderbolt/path.c
index afe5f8391ebf..efda9f0467ad 100644
--- a/drivers/thunderbolt/path.c
+++ b/drivers/thunderbolt/path.c
@@ -45,7 +45,7 @@ static struct tb_port *tb_path_find_dst_port(struct tb_port *src, int src_hopid,
for (i = 0; port && i < TB_PATH_MAX_HOPS; i++) {
sw = port->sw;
- ret = tb_port_read(port, &hop, TB_CFG_HOPS, 2 * hopid, 2);
+ ret = tb_port_read(port, &hop, TB_CFG_HOPS, 2 * hopid, 1);
if (ret) {
tb_port_warn(port, "failed to read path at %d\n", hopid);
return NULL;
@@ -129,7 +129,7 @@ struct tb_path *tb_path_discover(struct tb_port *src, int src_hopid,
for (i = 0; p && i < TB_PATH_MAX_HOPS; i++) {
sw = p->sw;
- ret = tb_port_read(p, &hop, TB_CFG_HOPS, 2 * h, 2);
+ ret = tb_port_read(p, &hop, TB_CFG_HOPS, 2 * h, 1);
if (ret) {
tb_port_warn(p, "failed to read path at %d\n", h);
return NULL;
@@ -171,7 +171,7 @@ struct tb_path *tb_path_discover(struct tb_port *src, int src_hopid,
sw = p->sw;
- ret = tb_port_read(p, &hop, TB_CFG_HOPS, 2 * h, 2);
+ ret = tb_port_read(p, &hop, TB_CFG_HOPS, 2 * h, 1);
if (ret) {
tb_port_warn(p, "failed to read path at %d\n", h);
goto err;
@@ -349,8 +349,10 @@ static int __tb_path_deactivate_hop(struct tb_port *port, int hop_index,
ktime_t timeout;
int ret;
+ /* Use only first dword of hop when reading */
+
/* Disable the path */
- ret = tb_port_read(port, &hop, TB_CFG_HOPS, 2 * hop_index, 2);
+ ret = tb_port_read(port, &hop, TB_CFG_HOPS, 2 * hop_index, 1);
if (ret)
return ret;
@@ -360,7 +362,7 @@ static int __tb_path_deactivate_hop(struct tb_port *port, int hop_index,
hop.enable = 0;
- ret = tb_port_write(port, &hop, TB_CFG_HOPS, 2 * hop_index, 2);
+ ret = tb_port_write(port, &hop, TB_CFG_HOPS, 2 * hop_index, 1);
if (ret)
return ret;
diff --git a/drivers/thunderbolt/tb.c b/drivers/thunderbolt/tb.c
index 1f7a9e1cc09c..28a72336558a 100644
--- a/drivers/thunderbolt/tb.c
+++ b/drivers/thunderbolt/tb.c
@@ -313,8 +313,10 @@ static struct tb_port *tb_find_unused_port(struct tb_switch *sw,
continue;
if (!sw->ports[i].cap_adap)
continue;
- if (tb_port_is_enabled(&sw->ports[i]))
+ if (tb_port_is_enabled(&sw->ports[i])) {
+ tb_port_dbg(&sw->ports[i], "this already enabled\n");
continue;
+ }
return &sw->ports[i];
}
return NULL;
@@ -365,16 +367,25 @@ static int tb_tunnel_dp(struct tb *tb, struct tb_port *out)
struct tb_tunnel *tunnel;
struct tb_port *in;
- if (tb_port_is_enabled(out))
+ tb_port_dbg(out, "trying to tunnel DP\n");
+
+ if (tb_port_is_enabled(out)) {
+ tb_port_dbg(out, "DP OUT port already enabled\n");
return 0;
+ }
+
+ tb_port_dbg(out, "finding free DP IN port\n");
do {
sw = tb_to_switch(sw->dev.parent);
if (!sw)
return 0;
+ tb_sw_dbg(sw, "finding available DP IN\n");
in = tb_find_unused_port(sw, TB_TYPE_DP_HDMI_IN);
} while (!in);
+ tb_port_dbg(in, "found DP IN\n");
+
tunnel = tb_tunnel_alloc_dp(tb, in, out);
if (!tunnel) {
tb_port_dbg(out, "DP tunnel allocation failed\n");
diff --git a/drivers/thunderbolt/tunnel.c b/drivers/thunderbolt/tunnel.c
index 5a99234826e7..934a1978741c 100644
--- a/drivers/thunderbolt/tunnel.c
+++ b/drivers/thunderbolt/tunnel.c
@@ -351,9 +351,23 @@ struct tb_tunnel *tb_tunnel_discover_dp(struct tb *tb, struct tb_port *in)
struct tb_tunnel *tunnel;
struct tb_port *port;
struct tb_path *path;
+ u32 data[2];
+ int ret;
+
+ tb_port_dbg(in, "start DP discover\n");
- if (!tb_dp_port_is_enabled(in))
+ if (!tb_dp_port_is_enabled(in)) {
+ tb_port_dbg(in, "DP port NOT enabled\n");
return NULL;
+ }
+
+ ret = tb_port_read(in, data, TB_CFG_PORT, in->cap_adap,
+ ARRAY_SIZE(data));
+ if (ret)
+ return NULL;
+
+ tb_port_dbg(in, "data[0]=0x%08x\n", data[0]);
+ tb_port_dbg(in, "data[1]=0x%08x\n", data[1]);
tunnel = tb_tunnel_alloc(tb, 3, TB_TUNNEL_DP);
if (!tunnel)
On 3/9/19 7:55 pm, Mika Westerberg wrote:
> On Tue, Sep 03, 2019 at 07:11:32PM +0800, Brad Campbell wrote:
>
> I think the problem is that for some reason, probably because this is
> first generation hardware with all the bugs included, you cannot read
> the second dword from DP adapter path config space (you can write it
> though).
>
> I've updated the patch so that it reads only the first dword when it
> discovers paths and also when it disables them. Can you try it out and
> see if it makes a difference? This should also get rid of the warnings
> you get.
>
It would seem so. Now I get 3 heads on a cold or warm boot and no
warnings in the log, so it looks like that did the job. I've attached
the dmesg from the last cold boot for reference.
I forgot to mention I did try the chained monitors over the weekend, but
that resulted in some carnage (corrupted displays and some form of hard
lock when starting X). I've not had a chance to get netconsole
configured up to catch any wreckage when it explodes as it didn't leave
anything on disk.
I will do that in the next couple of days just for completeness.
I really appreciate you looking into this given it's probably one of
those .001% corner cases.
Regards,
Brad
On Tue, Sep 03, 2019 at 08:54:02PM +0800, Brad Campbell wrote:
> On 3/9/19 7:55 pm, Mika Westerberg wrote:
> > On Tue, Sep 03, 2019 at 07:11:32PM +0800, Brad Campbell wrote:
> >
> > I think the problem is that for some reason, probably because this is
> > first generation hardware with all the bugs included, you cannot read
> > the second dword from DP adapter path config space (you can write it
> > though).
> >
> > I've updated the patch so that it reads only the first dword when it
> > discovers paths and also when it disables them. Can you try it out and
> > see if it makes a difference? This should also get rid of the warnings
> > you get.
> >
>
> It would seem so. Now I get 3 heads on a cold or warm boot and no warnings
> in the log, so it looks like that did the job. I've attached the dmesg from
> the last cold boot for reference.
Great, thanks for checking. I will create a proper patch and submit
upstream hopefully later this week or early next.
> I forgot to mention I did try the chained monitors over the weekend, but
> that resulted in some carnage (corrupted displays and some form of hard lock
> when starting X). I've not had a chance to get netconsole configured up to
> catch any wreckage when it explodes as it didn't leave anything on disk.
>
> I will do that in the next couple of days just for completeness.
Yeah, it would be interesting to see where it blows up. From graphics
perspective the two cases should not make any difference.
Hi Brad,
On Tue, Sep 03, 2019 at 04:33:23PM +0300, Mika Westerberg wrote:
> On Tue, Sep 03, 2019 at 08:54:02PM +0800, Brad Campbell wrote:
> > On 3/9/19 7:55 pm, Mika Westerberg wrote:
> > > On Tue, Sep 03, 2019 at 07:11:32PM +0800, Brad Campbell wrote:
> > >
> > > I think the problem is that for some reason, probably because this is
> > > first generation hardware with all the bugs included, you cannot read
> > > the second dword from DP adapter path config space (you can write it
> > > though).
> > >
> > > I've updated the patch so that it reads only the first dword when it
> > > discovers paths and also when it disables them. Can you try it out and
> > > see if it makes a difference? This should also get rid of the warnings
> > > you get.
> > >
> >
> > It would seem so. Now I get 3 heads on a cold or warm boot and no warnings
> > in the log, so it looks like that did the job. I've attached the dmesg from
> > the last cold boot for reference.
>
> Great, thanks for checking. I will create a proper patch and submit
> upstream hopefully later this week or early next.
Unfortunately I did not have time to create a proper patch yet. I've
been busy with other things currently. Since merge window opens next
week this would have to wait until it closes anyway. I'm trying to get
this as part of a patch series I plan to submit after v5.4-rc1 is
released.