Re: [Intel-gfx] [PATCH v2 00/22] Enable gpu switching on the MacBook Pro

[Lukas Wunner]

Hi Jani,

On Mon, Aug 31, 2015 at 10:15:07PM +0300, Jani Nikula wrote:
> On Sat, 29 Aug 2015, Lukas Wunner  wrote:
> > the patch set I've posted August 12 included 3 commits which fix bugs
> > in i915. These bugs should be fixed independently of MacBook Pro GPU
> > switching, please consider merging them:
> > [...]
> > drm/i915: Fix failure paths around initial fbdev allocation
> > http://patchwork.freedesktop.org/patch/53673/
> > drm/i915: On fb alloc failure, unref gem object where it gets refed
> > http://patchwork.freedesktop.org/patch/53674/
> 
> Sorry about that. Unfortunately the target is moving fast, and they no
> longer apply. Please resend on top of current nightly.

Alright, coming up in separate e-mails are the above 2 patches rebased on
drm-intel-nightly as of this morning. I didn't have to make any changes to
the code, if they didn't apply cleanly to your tree it was probably just
because of changed diff context.

To ease reviewing I've also pushed them to GitHub:
https://github.com/l1k/linux/commit/f0cd66427039ce1bdc61460a9d833e6d858cff3e
https://github.com/l1k/linux/commit/521e48fc5fc8d211ed2847070120ff4032b7a383

Briefly, the story of the 2 patches is this:
- I had originally reported the issue on June 3:
  http://lists.freedesktop.org/archives/intel-gfx/2015-June/067965.html
- Tvrtko came up with a patch which I've tested successfully:
  https://patchwork.freedesktop.org/patch/53207/
- However Ville responded to Tvrtko's patch: "I find it rather unexpected
  that the function drops the passed reference on error. My usual rule is:
  do nothing on error, if possible." (see comment section of patchwork link)
  Tvrtko answered that he didn't have time to look into this further and
  I wanted it fixed, so I submitted a set of 2 patches, consisting of an
  adjusted version of Tvrtko's patch, plus another one by me to address
  Ville's remarks.

So you can either merge Tvrtko's single patch or the 2 patches from me,
whichever you prefer. Or request something completely different.

Thanks,

Lukas
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH v2 00/22] Enable gpu switching on the MacBook Pro

[Jani Nikula]

On Mon, 31 Aug 2015, Jani Nikula  wrote:
> On Sat, 29 Aug 2015, Lukas Wunner  wrote:
>> Hi Daniel, Hi Jani,
>>
>> the patch set I've posted August 12 included 3 commits which fix bugs
>> in i915. These bugs should be fixed independently of MacBook Pro GPU
>> switching, please consider merging them:
>>
>> drm/i915: Preserve SSC earlier
>> http://patchwork.freedesktop.org/patch/56921/

Pushed this one.

Jani.

>>
>> drm/i915: Fix failure paths around initial fbdev allocation
>> http://patchwork.freedesktop.org/patch/53673/
>> drm/i915: On fb alloc failure, unref gem object where it gets refed
>> http://patchwork.freedesktop.org/patch/53674/
>>
>> The latter two commits relate to a bug Jani was tracking before his
>> holidays which has unfortunately fallen by the wayside.
>
> Sorry about that. Unfortunately the target is moving fast, and they no
> longer apply. Please resend on top of current nightly.
>
> BR,
> Jani.
>
>
>>
>> Thanks,
>>
>> Lukas
>
> -- 
> Jani Nikula, Intel Open Source Technology Center

-- 
Jani Nikula, Intel Open Source Technology Center
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH v2 00/22] Enable gpu switching on the MacBook Pro

[Jani Nikula]

On Sat, 29 Aug 2015, Lukas Wunner  wrote:
> Hi Daniel, Hi Jani,
>
> the patch set I've posted August 12 included 3 commits which fix bugs
> in i915. These bugs should be fixed independently of MacBook Pro GPU
> switching, please consider merging them:
>
> drm/i915: Preserve SSC earlier
> http://patchwork.freedesktop.org/patch/56921/
>
> drm/i915: Fix failure paths around initial fbdev allocation
> http://patchwork.freedesktop.org/patch/53673/
> drm/i915: On fb alloc failure, unref gem object where it gets refed
> http://patchwork.freedesktop.org/patch/53674/
>
> The latter two commits relate to a bug Jani was tracking before his
> holidays which has unfortunately fallen by the wayside.

Sorry about that. Unfortunately the target is moving fast, and they no
longer apply. Please resend on top of current nightly.

BR,
Jani.


>
> Thanks,
>
> Lukas

-- 
Jani Nikula, Intel Open Source Technology Center
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH v2 00/22] Enable gpu switching on the MacBook Pro

[Lukas Wunner]

Hi Daniel, Hi Jani,

the patch set I've posted August 12 included 3 commits which fix bugs
in i915. These bugs should be fixed independently of MacBook Pro GPU
switching, please consider merging them:

drm/i915: Preserve SSC earlier
http://patchwork.freedesktop.org/patch/56921/

drm/i915: Fix failure paths around initial fbdev allocation
http://patchwork.freedesktop.org/patch/53673/
drm/i915: On fb alloc failure, unref gem object where it gets refed
http://patchwork.freedesktop.org/patch/53674/

The latter two commits relate to a bug Jani was tracking before his
holidays which has unfortunately fallen by the wayside.

Thanks,

Lukas
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH v2 00/22] Enable gpu switching on the MacBook Pro

[Daniel Vetter]

On Thu, Aug 13, 2015 at 1:37 AM, Lukas Wunner lu...@wunner.de wrote:
 Hi Daniel,

 On Wed, Aug 12, 2015 at 04:16:25PM +0200, Daniel Vetter wrote:
  * Reprobing if the inactive GPU initializes before the apple-gmux module:
v1 used Matthew Garrett's approach of adding a driver callback.
v2 simply generates a hotplug event instead. nouveau polls its outputs
every 10 seconds so we want it to poll immediately once apple-gmux
registers. That is achieved by the hotplug event. The i915 driver is
changed to behave identically to nouveau. (Right now it deletes LVDS
and eDP connectors from the mode configuration if they can't be probed,
deeming them to be ghosts.)

 I thought -EDEFERREDPROBE is what we should be using if drivers don't get
 loaded in the right order? Hand-rolling depency avoidance stuff is imo a
 horrible idea.
 [...]
 I think just reading edid and the relevant dp aux data in apple-gmux or
 somewhere like that and stalling driver load until that's ready is the
 only clean option.

 I'm afraid we can't stall initialization of a driver like that because
 even though the GPU may not be switched to the panel, it may still have
 an external monitor attached.

 All MacBook Pros have external DP and/or HDMI ports and these are
 either soldered to the discrete GPU (model year 2011 and onwards)
 or switchable between the discrete and integrated GPU (until 2010;
 I think they are even switchable separately from the panel).

 So basically we'd have to initialize the driver normally, and if
 intel_lvds_init() or intel_edp_init_connector() fail we'd have to
 somehow pass that up the call chain so that i915_pci_probe() can
 return -EPROBE_DEFER. And whenever we're asked to reprobe we'd
 repeat initialization of the LVDS or eDP connector.

 I'm wondering what the benefit is compared to just keeping the
 connector in the mode configuration, but with status disconnected,
 and reprobing it when the -output_poll_changed callback gets invoked?
 Because that's what nouveau already does, and what I've changed i915
 to do with patch 13.

 vga_switcheroo calls drm_kms_helper_hotplug_event() when the handler
 registers (patch 11), which will invoke -output_poll_changed.
 So we're talking about the Official DRM Callback [tm] to probe
 outputs, not hand-rolling depency avoidance. :-)

Oh I didn't spot that one. This kind of layering inversions generally
leads to deadlocks and fun stuff. Also reprobing lvds/edp is just a
side-effect when you have fbdev emulation enabled. If we go with this
re-probing approach then we definitely need a new hook in
vga-switcheroo, and even then there's still the locking problem.

  * Framebuffer recreation if the inactive GPU initializes before the
apple-gmux module (i.e. discarding the default 1024x768 fb and replacing
with a new one with the actual panel resolution): v1 only supported this
for i915, v2 has a generic solution which works with nouveau and radeon
as well. (Necessary if the discrete GPU is forced to be the inactive one
on boot via the EFI variable.)

 Would completely remove the need for this ;-)

 Unfortunately not: We'd still have to initialize the driver to be able
 to drive external displays. If there are initially no connectors with
 modes, we'll once again end up with the 1024x768 fb.

EDEFERREDPROBE isn't something that gets returned to userspace, it's
just internal handling so that the kernel knows there's a depency
issue and it needs to retry probing once other drivers have finished
loading. It is the generic means linux has to handle cross-driver
depencies which aren't reflected in the bus hierarchy.

I.e. it's just something to make sure that apple-gmux is fully loaded
before i915/nouveau. The driver _will_ be initialized eventually.

 You can't share the dp aux like that. It's true that we need a bit more
 data (there's a few eDP related feature blocsk we need), but sharing the
 aux channel entirely is no-go. If you do you get drivers trying to link
 train and at best this fails and at worst you'll upset the configuration
 of the other driver and piss of the panel enough to need a hard reset
 until it works again.

 Yes, so far proxying of the AUX channel is read-only. In those cases
 when writing is necessary for setting up the output, I'm adding code
 to check if the current content of the DPCD is identical to what's
 being written and if so, skip the write. We'll see if this stategy is
 sufficient for the drivers to set up their outputs.

You need to block anything that would write _much_ earlier. By the
time we're doing link-training it's way too late.

 I think the real tricky bit here with vgaswitcheroo is locking, I need to
 take a separate lock at the patches for that.

 Locking when switching only the DDC lines is facilitated by the ddc_lock
 attribute of struct vgasr_priv. This is all local to vga_switcheroo.c
 and contained in patches 5 and 6.

 Locking when proxying the AUX channel is facilitated by the 

Re: [Intel-gfx] [PATCH v2 00/22] Enable gpu switching on the MacBook Pro

[Lukas Wunner]

Hi Daniel,

On Wed, Aug 12, 2015 at 04:16:25PM +0200, Daniel Vetter wrote:
  * Reprobing if the inactive GPU initializes before the apple-gmux module:
v1 used Matthew Garrett's approach of adding a driver callback.
v2 simply generates a hotplug event instead. nouveau polls its outputs
every 10 seconds so we want it to poll immediately once apple-gmux
registers. That is achieved by the hotplug event. The i915 driver is
changed to behave identically to nouveau. (Right now it deletes LVDS
and eDP connectors from the mode configuration if they can't be probed,
deeming them to be ghosts.)
 
 I thought -EDEFERREDPROBE is what we should be using if drivers don't get
 loaded in the right order? Hand-rolling depency avoidance stuff is imo a
 horrible idea.
[...]
 I think just reading edid and the relevant dp aux data in apple-gmux or
 somewhere like that and stalling driver load until that's ready is the
 only clean option.

I'm afraid we can't stall initialization of a driver like that because
even though the GPU may not be switched to the panel, it may still have
an external monitor attached.

All MacBook Pros have external DP and/or HDMI ports and these are
either soldered to the discrete GPU (model year 2011 and onwards)
or switchable between the discrete and integrated GPU (until 2010;
I think they are even switchable separately from the panel).

So basically we'd have to initialize the driver normally, and if
intel_lvds_init() or intel_edp_init_connector() fail we'd have to
somehow pass that up the call chain so that i915_pci_probe() can
return -EPROBE_DEFER. And whenever we're asked to reprobe we'd
repeat initialization of the LVDS or eDP connector.

I'm wondering what the benefit is compared to just keeping the
connector in the mode configuration, but with status disconnected,
and reprobing it when the -output_poll_changed callback gets invoked?
Because that's what nouveau already does, and what I've changed i915
to do with patch 13.

vga_switcheroo calls drm_kms_helper_hotplug_event() when the handler
registers (patch 11), which will invoke -output_poll_changed.
So we're talking about the Official DRM Callback [tm] to probe
outputs, not hand-rolling depency avoidance. :-)


  * Framebuffer recreation if the inactive GPU initializes before the
apple-gmux module (i.e. discarding the default 1024x768 fb and replacing
with a new one with the actual panel resolution): v1 only supported this
for i915, v2 has a generic solution which works with nouveau and radeon
as well. (Necessary if the discrete GPU is forced to be the inactive one
on boot via the EFI variable.)
 
 Would completely remove the need for this ;-)

Unfortunately not: We'd still have to initialize the driver to be able
to drive external displays. If there are initially no connectors with
modes, we'll once again end up with the 1024x768 fb.


 You can't share the dp aux like that. It's true that we need a bit more
 data (there's a few eDP related feature blocsk we need), but sharing the
 aux channel entirely is no-go. If you do you get drivers trying to link
 train and at best this fails and at worst you'll upset the configuration
 of the other driver and piss of the panel enough to need a hard reset
 until it works again.

Yes, so far proxying of the AUX channel is read-only. In those cases
when writing is necessary for setting up the output, I'm adding code
to check if the current content of the DPCD is identical to what's
being written and if so, skip the write. We'll see if this stategy is
sufficient for the drivers to set up their outputs.


 I think the real tricky bit here with vgaswitcheroo is locking, I need to
 take a separate lock at the patches for that.

Locking when switching only the DDC lines is facilitated by the ddc_lock
attribute of struct vgasr_priv. This is all local to vga_switcheroo.c
and contained in patches 5 and 6.

Locking when proxying the AUX channel is facilitated by the hw_mutex
attribute of struct drm_dp_aux. nouveau has its own locking mechanism
contained in drivers/gpu/drm/nouveau/nvkm/subdev/i2c/pad*.c. Thus,
when proxying via nouveau, there are two locking mechanisms at work
(drm_dp_aux hw_mutex as outer lock + pad as inner lock). This is
nothing introduced by this patch set, all existing code.

Locking of access to the struct vgasr_priv is facilitated by the
vgasr_mutex in vga_switcheroo.c. Also existing code.


Best regards,

Lukas
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Intel-gfx] [PATCH v2 00/22] Enable gpu switching on the MacBook Pro

[Lukas Wunner]

This is a follow-up to the v1 posted in April:
http://lists.freedesktop.org/archives/dri-devel/2015-April/081515.html


Patches 1 - 17 enable GPU switching on the pre-retina MacBook Pro.
These were tested successfully by multiple people and solve two
tickets in Bugzilla:
https://bugzilla.kernel.org/show_bug.cgi?id=88861
https://bugs.freedesktop.org/show_bug.cgi?id=61115

Patches 18 - 22 are a preview of how we're tackling retina support.
Those are marked experimental and are NOT ready to be merged yet.
Feedback on them is welcome.

The patches are based on drm-next.

They were tested on the following hardware (thanks a lot everyone!):
Tested-by: Paul Hordiienko pvt.g...@gmail.com
[MBP  6,2 2010  intel ILK + nvidia GT216  pre-retina]
Tested-by: William Brown will...@blackhats.net.au
[MBP  8,2 2011  intel SNB + amd turks pre-retina]
Tested-by: Lukas Wunner lu...@wunner.de
[MBP  9,1 2012  intel IVB + nvidia GK107  pre-retina]
Tested-by: Bruno Bierbaumer br...@bierbaumer.net
[MBP 11,3 2013  intel HSW + nvidia GK107  retina -- work in progress]


What's new:

* By default the MBP boots with the display switched to the discrete GPU
  but it can be forced to the integrated GPU with an EFI boot variable.
  Here's a handy tool for that: https://github.com/0xbb/gpu-switch
  v1 didn't work in this configuration, v2 does.

* Reprobing if the inactive GPU initializes before the apple-gmux module:
  v1 used Matthew Garrett's approach of adding a driver callback.
  v2 simply generates a hotplug event instead. nouveau polls its outputs
  every 10 seconds so we want it to poll immediately once apple-gmux
  registers. That is achieved by the hotplug event. The i915 driver is
  changed to behave identically to nouveau. (Right now it deletes LVDS
  and eDP connectors from the mode configuration if they can't be probed,
  deeming them to be ghosts.)

* Framebuffer recreation if the inactive GPU initializes before the
  apple-gmux module (i.e. discarding the default 1024x768 fb and replacing
  with a new one with the actual panel resolution): v1 only supported this
  for i915, v2 has a generic solution which works with nouveau and radeon
  as well. (Necessary if the discrete GPU is forced to be the inactive one
  on boot via the EFI variable.)

* Generally lots of rough edges were smoothed to hopefully make the
  patches more suitable for merging. E.g. there's a bug in i915 where
  the SSC status set by BIOS is preserved too late and v1 only contained
  a workaround, whereas v2 contains a proper fix in a separate commit.


The long journey towards retina support:

The pixel clock required for retina resolution is not supported by LVDS
(which was used on pre-retinas), necessitating eDP instead. Problem is,
the gmux register which switches the DDC lines on pre-retinas doesn't
switch the AUX channel on retinas. Disassembling the OS X driver revealed
that the gmux in retina MBPs gained an additional register 0x7f which gets
written to when setting up the eDP configuration. There was some hope that
this might switch the AUX channel. Alas, we tried writing various values
to that register but were unable to get the inactive GPU to talk to the
panel. The purpose of register 0x7f remains a mystery.

Teardowns of the first generation retina MBP name the NXP CBTL06142 and
TI HD3SS212 as multiplexers and according to the data sheets I've found,
neither supports switching the AUX channel separately from the main link.

Matthew Garrett had the idea of having the active GPU stash the EDID and
the first 8 bytes of the DPCD (receiver capabilities) and letting the
inactive GPU retrieve that data. I rebased and rewrote his patches and
got everything working, only to discover that the drivers are unhappy
with just 8 bytes of DPCD. They need full access to the DPCD to set up
their outputs. We could stash the entire DPCD but some parts of it are
mutable so the stashed data may become stale when the active GPU performs
writes to the DPCD.

So I had the idea of using the active GPU as a proxy to talk to the panel,
thus emulating switching of the AUX channel in software. We can leverage
the struct drm_dp_aux and i2c_adapter (for DDC) to achieve this, swapping
the inactive GPU's structs with those of the active GPU on the fly.
That approach is implemented in patches 18 - 22 but there are still some
driver issues that I'm debugging. The current status as per the latest
logs Bruno sent me is that i915 rejects the mode retrieved via proxying
with CLOCK_HIGH and nouveau aborts link training halfway through.
Bottom line is that it's not yet working but we're getting closer.

As a side effect, the pre-retinas gain a second way to initialize their
outputs: They can either use gmux to switch the DDC lines, or use the
active GPU as a proxy for the DDC communication. Which method gets used
depends on the order in which the drivers initialize, the inactive GPU
will happily use whatever is available and it automatically receives
a hotplug event 

Re: [Intel-gfx] [PATCH v2 00/22] Enable gpu switching on the MacBook Pro

[Daniel Vetter]

On Tue, Aug 11, 2015 at 12:29:17PM +0200, Lukas Wunner wrote:
 This is a follow-up to the v1 posted in April:
 http://lists.freedesktop.org/archives/dri-devel/2015-April/081515.html
 
 
 Patches 1 - 17 enable GPU switching on the pre-retina MacBook Pro.
 These were tested successfully by multiple people and solve two
 tickets in Bugzilla:
 https://bugzilla.kernel.org/show_bug.cgi?id=88861
 https://bugs.freedesktop.org/show_bug.cgi?id=61115
 
 Patches 18 - 22 are a preview of how we're tackling retina support.
 Those are marked experimental and are NOT ready to be merged yet.
 Feedback on them is welcome.
 
 The patches are based on drm-next.
 
 They were tested on the following hardware (thanks a lot everyone!):
 Tested-by: Paul Hordiienko pvt.g...@gmail.com
 [MBP  6,2 2010  intel ILK + nvidia GT216  pre-retina]
 Tested-by: William Brown will...@blackhats.net.au
 [MBP  8,2 2011  intel SNB + amd turks pre-retina]
 Tested-by: Lukas Wunner lu...@wunner.de
 [MBP  9,1 2012  intel IVB + nvidia GK107  pre-retina]
 Tested-by: Bruno Bierbaumer br...@bierbaumer.net
 [MBP 11,3 2013  intel HSW + nvidia GK107  retina -- work in progress]
 
 
 What's new:
 
 * By default the MBP boots with the display switched to the discrete GPU
   but it can be forced to the integrated GPU with an EFI boot variable.
   Here's a handy tool for that: https://github.com/0xbb/gpu-switch
   v1 didn't work in this configuration, v2 does.
 
 * Reprobing if the inactive GPU initializes before the apple-gmux module:
   v1 used Matthew Garrett's approach of adding a driver callback.
   v2 simply generates a hotplug event instead. nouveau polls its outputs
   every 10 seconds so we want it to poll immediately once apple-gmux
   registers. That is achieved by the hotplug event. The i915 driver is
   changed to behave identically to nouveau. (Right now it deletes LVDS
   and eDP connectors from the mode configuration if they can't be probed,
   deeming them to be ghosts.)

I thought -EDEFERREDPROBE is what we should be using if drivers don't get
loaded in the right order? Hand-rolling depency avoidance stuff is imo a
horrible idea.

 * Framebuffer recreation if the inactive GPU initializes before the
   apple-gmux module (i.e. discarding the default 1024x768 fb and replacing
   with a new one with the actual panel resolution): v1 only supported this
   for i915, v2 has a generic solution which works with nouveau and radeon
   as well. (Necessary if the discrete GPU is forced to be the inactive one
   on boot via the EFI variable.)

Would completely remove the need for this ;-)

 * Generally lots of rough edges were smoothed to hopefully make the
   patches more suitable for merging. E.g. there's a bug in i915 where
   the SSC status set by BIOS is preserved too late and v1 only contained
   a workaround, whereas v2 contains a proper fix in a separate commit.
 
 
 The long journey towards retina support:
 
 The pixel clock required for retina resolution is not supported by LVDS
 (which was used on pre-retinas), necessitating eDP instead. Problem is,
 the gmux register which switches the DDC lines on pre-retinas doesn't
 switch the AUX channel on retinas. Disassembling the OS X driver revealed
 that the gmux in retina MBPs gained an additional register 0x7f which gets
 written to when setting up the eDP configuration. There was some hope that
 this might switch the AUX channel. Alas, we tried writing various values
 to that register but were unable to get the inactive GPU to talk to the
 panel. The purpose of register 0x7f remains a mystery.
 
 Teardowns of the first generation retina MBP name the NXP CBTL06142 and
 TI HD3SS212 as multiplexers and according to the data sheets I've found,
 neither supports switching the AUX channel separately from the main link.
 
 Matthew Garrett had the idea of having the active GPU stash the EDID and
 the first 8 bytes of the DPCD (receiver capabilities) and letting the
 inactive GPU retrieve that data. I rebased and rewrote his patches and
 got everything working, only to discover that the drivers are unhappy
 with just 8 bytes of DPCD. They need full access to the DPCD to set up
 their outputs. We could stash the entire DPCD but some parts of it are
 mutable so the stashed data may become stale when the active GPU performs
 writes to the DPCD.
 
 So I had the idea of using the active GPU as a proxy to talk to the panel,
 thus emulating switching of the AUX channel in software. We can leverage
 the struct drm_dp_aux and i2c_adapter (for DDC) to achieve this, swapping
 the inactive GPU's structs with those of the active GPU on the fly.
 That approach is implemented in patches 18 - 22 but there are still some
 driver issues that I'm debugging. The current status as per the latest
 logs Bruno sent me is that i915 rejects the mode retrieved via proxying
 with CLOCK_HIGH and nouveau aborts link training halfway through.
 Bottom line is that it's not yet working but we're getting closer.
 
 As a