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

[Imre Deak]

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>
> > 
> > AFAICS this also needs calling vga_switcheroo_set_dynamic_switch() from
> > the i915 runtime suspend/resume handlers.
> 
> I've posted a series a week ago which removes that call and haven't seen
> any major objections.  Assuming that series goes into 4.17, there's no
> point in adding calls to vga_switcheroo_set_dynamic_switch() now:
> https://lists.freedesktop.org/archives/nouveau/2018-February/029851.html

Ok, read through it and not adding the call to i915 makes sense then.

> 
> If you have an Optimus/ATPX machine handy, please consider testing the
> series.

I don't have one.

> > 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().

> Manual power control was a stopgap according to Dave Airlie that
> he implemented before he got runtime PM up and running:
> http://lkml.iu.edu/hypermail/linux/kernel/1603.1/01935.html
> 
> It will be removed eventually, but right now it can't because runtime PM
> on i915 doesn't yet cover all platforms and isn't yet working on muxed
> laptops such as the MacBook Pro.  (I'm working on fixing the latter,
> the series I've linked above gets us one step closer.)
> 
> 
> > 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. This is a change in semantics
that's worth mentioning in the commit message.

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.

--Imre