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

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

[Lukas Wunner]

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

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

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Tell vga_switcheroo whether runtime PM is used

[Imre Deak]

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
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Tell vga_switcheroo whether runtime PM is used

[Lukas Wunner]

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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [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@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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Tell vga_switcheroo whether runtime PM is used

[Jani Nikula]

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.

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

Re: [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@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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx