On Mon, 05 Mar 2018, Imre Deak <imre.deak@xxxxxxxxx> 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@xxxxxxxxx> >> > > Signed-off-by: Lukas Wunner <lukas@xxxxxxxxx> >> > >> > 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. I guess the reference is 3f577573cd54 ("drm/i915: do not disable backlight on vgaswitcheroo switch off") which I had happily forgotten about. Not sure we would've added it like that had it not been a regression fix way back when. BR, Jani. -- Jani Nikula, Intel Open Source Technology Center _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/intel-gfx
