| Message ID | e64c6f8ae447c243947988c1c07d471955ee5042.1519515110.git.lukas@wunner.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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@intel.com> > Signed-off-by: Lukas Wunner <lukas@wunner.de> AFAICS this also needs calling vga_switcheroo_set_dynamic_switch() from the i915 runtime suspend/resume handlers. Also after this we can remove i915_switcheroo_set_state() ? 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). --Imre > --- > drivers/gpu/drm/i915/i915_drv.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c > index aaa861b51024..519a1b9568df 100644 > --- a/drivers/gpu/drm/i915/i915_drv.c > +++ b/drivers/gpu/drm/i915/i915_drv.c > @@ -668,7 +668,8 @@ static int i915_load_modeset_init(struct drm_device *dev) > > intel_register_dsm_handler(); > > - ret = vga_switcheroo_register_client(pdev, &i915_switcheroo_ops, false); > + ret = vga_switcheroo_register_client(pdev, &i915_switcheroo_ops, > + HAS_RUNTIME_PM(dev_priv)); > if (ret) > goto cleanup_vga_client; > > -- > 2.15.1 >
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@intel.com> > > Signed-off-by: Lukas Wunner <lukas@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 If you have an Optimus/ATPX machine handy, please consider testing the series. > 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.) 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. Thanks, Lukas
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@intel.com> > > > Signed-off-by: Lukas Wunner <lukas@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
On Mon, 05 Mar 2018, Imre Deak <imre.deak@intel.com> 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@intel.com> >> > > Signed-off-by: Lukas Wunner <lukas@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. 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.
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@intel.com> > > > > Signed-off-by: Lukas Wunner <lukas@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
diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c index aaa861b51024..519a1b9568df 100644 --- a/drivers/gpu/drm/i915/i915_drv.c +++ b/drivers/gpu/drm/i915/i915_drv.c @@ -668,7 +668,8 @@ static int i915_load_modeset_init(struct drm_device *dev) intel_register_dsm_handler(); - ret = vga_switcheroo_register_client(pdev, &i915_switcheroo_ops, false); + ret = vga_switcheroo_register_client(pdev, &i915_switcheroo_ops, + HAS_RUNTIME_PM(dev_priv)); if (ret) goto cleanup_vga_client;
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@intel.com> Signed-off-by: Lukas Wunner <lukas@wunner.de> --- drivers/gpu/drm/i915/i915_drv.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)