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

[Jani Nikula <jani.nikula@intel.com>]

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