[Intel-gfx] [PATCH] drm/i915: Tell vga_switcheroo whether runtime PM is used

[Lukas Wunner]

On Mon, Mar 05, 2018 at 05:37:11PM +0200, Imre Deak wrote:
> On Mon, Feb 26, 2018 at 04:57:11PM +0100, Lukas Wunner wrote:
> > On Mon, Feb 26, 2018 at 04:41:09PM +0200, Imre Deak wrote:
> > > On Sun, Feb 25, 2018 at 12:42:30AM +0100, Lukas Wunner wrote:
> > > > DRM drivers need to tell vga_switcheroo whether they use runtime PM.
> > > > If they do use it, vga_switcheroo lets them autosuspend at their own
> > > > discretion.  If on the other hand they do not use it, vga_switcheroo
> > > > allows the user to suspend and resume the GPU manually via the
> > > > ->set_gpu_state hook.
> > > > 
> > > > i915 currently tells vga_switcheroo that it never uses runtime PM, even
> > > > though it does use it on HSW and newer.  The result is that users may
> > > > interfere with the driver's runtime PM on those platforms.  Avoid by
> > > > reporting runtime PM support correctly to vga_switcheroo.
> > > > 
> > > > Cc: Imre Deak <imre.deak at intel.com>
> > > > Signed-off-by: Lukas Wunner <lukas at wunner.de>
> > > 
> > > Also after this we can remove i915_switcheroo_set_state() ?
> > 
> > Not yet.  That's still needed for manual power control on chips
> > where you're not supporting runtime PM yet and which are known to
> > be built into hybrid graphics laptops.  (On the MacBook Pro, that's
> > ILK, SNB, IVB, can't speak for non-Macs.)
> 
> Err, forgot about the old i915 platforms w/o runtime PM support. So ok,
> I see why we still do need i915_switcheroo_set_state().

Imre, sorry for the delay, this is a "submit a seemingly simple patch,
then realize you've opened a can of worms" kind of thing.

Actually I agree that we should probably hold off on this patch for
the moment.  On top of the issues you've mentioned there's also the
problem that switching the panel between GPUs currently only works
with manual power control.  I'm working on fixing that but it'll
take more time and if you apply this patch and then add runtime PM
for pre-HSW, switching would no longer work on LVDS MacBook Pros.


> > > It's probably worth mentioning in the commit message that this changes
> > > the semantics of the switching: while atm you can't open the the DRM
> > > file for an inactive device (switched off from with IGD/DIS/DIGD/DDIS)
> > > after this change you can. I suppose that's not a problem, it just means
> > > display probing will fail on inactive devices (the same way it's with
> > > MIGD/MDIS currently).
> > 
> > Sorry, I don't understand the last sentence in that paragraph at all.
> 
> I meant that before this change if i915 was not the active device (since
> the discrete card was made active for instance by 'echo DIS >
> /sys/kernel/debug/vgaswitcheroo') then trying to open the i915
> /dev/dri/cardX device file failed due to the corresponding check in
> drm_open_helper() and the i915 drm_device::switch_power_state being now
> DRM_SWITCH_POWER_OFF.
> 
> After this change if i915 is not active opening the i915 /dev/dri/cardX
> will succeed, since drm_device::switch_power_state will be permanently
> kept at DRM_SWITCH_POWER_ON.  But now since the display signals
> (including the DDC and DP AUX pins) could have been switched over to the
> discrete card doing display probing on i915 with
> DRM_IOCTL_MODE_GETCONNECTOR will fail.

Hm, do you always reprobe the panel resolution on ->runtime_resume?  Why?
Or are you referring to e.g. a reprobe triggered via sysfs?

Anyway, it's a little more complicated than that:
- On MacBook Pros with LVDS, the DDC pins are switchable between GPUs
  and this is taken advantage of in intel_lvds.c by calling
  drm_get_edid_switcheroo().
- On MacBook Pros with eDP, the AUX channel is not switchable and that's
  one of the reasons why we don't support switching the panel on those yet.
- On older AMD PowerXpress laptops with LVDS, DDC is likewise switchable
  but we don't make use of it because I don't have such a machine and it
  wasn't a priority for anyone else.
- On Optimus/PowerXpress laptops we therefore rely on correct VBT data.
  That's not an option on the MacBook Pro where VBT always contains
  bogus information.


> This is a change in semantics
> that's worth mentioning in the commit message.

If the machine is booted with the panel switched to the discrete GPU,
i915 would likewise have trouble probing the panel.  Apparently it
doesn't, so it seems relying on DDC switching or, where that's not
available, on VBT data, seems to work.

Or maybe the muxed Optimus/PowerXpress laptops were always booted with
the panel switched to the Intel GPU, I'm not sure.


> I'm not sure how this patch affects the workaround in
> intel_panel_disable_backlight(). Atm during switching we keep the
> backlight enabled since the discrete card depends on this. That won't
> work after this patch, since we won't call i915_switcheroo_set_state
> (except on old platforms) and so won't set
> drm_device::switch_power_state. Not sure what happens even now if i915
> disabled the panel before or after the switcheroo switch to the discrete
> card, but would be good to resolve this issue before your change. Maybe
> i915 would still need a notification about the switch and enable/disable
> the backlight accordingly? Adding Jani.

Thanks for making me aware of that, it wasn't on my radar.

So on the MacBook Pro, backlight brightness is always controlled by gmux,
not by the Intel GPU.  Based on the commit Jani mentioned, and the
bugzillas linked therein, it seems some PowerXpress laptops let the
integrated GPU handle backlight regardless whether the panel is switched
to the discrete GPU.

You could detect presence of the ATPX _DSM to find out if you're on a
PowerXpress laptop, but that would also match machines where the discrete
GPU has no outputs.

A better solution might be to check if dev_priv->drm.pdev !=
vga_default_device().  That will tell you if the panel is currently
not switched to the Intel GPU.  The default device is updated in
vga_switchto_stage1(), i.e. before actually making the switch.

If the Intel GPU is not the vga_default_device(), you know for certain
that you're dealing with a muxed laptop and that the panel is not
switched to the Intel GPU.

I'm wondering if it's at all allowed to runtime suspend the Intel GPU
if it continues to be responsible for backlight brightness after
switching.  At the very least, it has to be resumed when brightness
is changed, but perhaps it has to be kept resumed permanently to drive
the PWM signal?

Unfortunately to actually test this, you'd need one of these older
PowerXpress machines and they may be hard to come by.  I think the
newer ones are universally muxless.

Adding callbacks to be invoked pre and post switching would also be
an option (something like ->become_active and ->become_inactive,
where the latter would keep the backlight enabled unless
apple_gmux_present() returns true).

Thanks,

Lukas