Re: [Intel-gfx] [PATCH 10/13] drm/i915/lvds: switch to drm_edid_read_switcheroo()

[Lukas Wunner]

On Fri, Apr 21, 2023 at 02:47:48PM +0300, Jani Nikula wrote:
> --- a/drivers/gpu/drm/i915/display/intel_lvds.c
> +++ b/drivers/gpu/drm/i915/display/intel_lvds.c
> @@ -943,17 +943,8 @@ void intel_lvds_init(struct drm_i915_private *i915)
>*/
>   mutex_lock(>drm.mode_config.mutex);
>   if (vga_switcheroo_handler_flags() & VGA_SWITCHEROO_CAN_SWITCH_DDC) {
> - const struct edid *edid;
> -
> - /* FIXME: Make drm_get_edid_switcheroo() return drm_edid */
> - edid = drm_get_edid_switcheroo(>base,
> -intel_gmbus_get_adapter(i915, 
> pin));
> - if (edid) {
> - drm_edid = drm_edid_alloc(edid, (edid->extensions + 1) 
> * EDID_LENGTH);
> - kfree(edid);
> - } else {
> - drm_edid = NULL;
> - }
> + drm_edid = drm_edid_read_switcheroo(>base,
> + 
> intel_gmbus_get_adapter(i915, pin));
>   } else {
>   drm_edid = drm_edid_read_ddc(>base,
>intel_gmbus_get_adapter(i915, 
> pin));

No need for curly braces anymore, but regardless:

Reviewed-by: Lukas Wunner 

Re: [Intel-gfx] [PATCH v2 3/3] drm: Call vga_switcheroo_process_delayed_switch() in drm_lastclose

[Lukas Wunner]

On Thu, Jan 12, 2023 at 09:11:56PM +0100, Thomas Zimmermann wrote:
> Several lastclose helpers call vga_switcheroo_process_delayed_switch().
> It's better to call the helper from drm_lastclose() after the kernel
> client's screen has been restored. This way, all drivers can benefit
> without having to implement their own lastclose helper. For drivers
> without vga-switcheroo, vga_switcheroo_process_delayed_switch() does
> nothing.
[...]
> --- a/drivers/gpu/drm/drm_file.c
> +++ b/drivers/gpu/drm/drm_file.c
> @@ -38,6 +38,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>  
>  #include 
>  #include 
> @@ -460,6 +461,8 @@ void drm_lastclose(struct drm_device * dev)
>   drm_legacy_dev_reinit(dev);
>  
>   drm_client_dev_restore(dev);
> +
> + vga_switcheroo_process_delayed_switch();
>  }

Hm, this looks like a case of midlayer fallacy:

https://blog.ffwll.ch/2016/12/midlayers-once-more-with-feeling.html

It is a departure from the opt-in library approach we've had so far.

For switcheroo-aware EDID retrieval, there's a drm_get_edid_switcheroo()
helper.  How about introducing a switcheroo-aware lastclose helper which
drivers can reference?

Thanks,

Lukas

Re: [Intel-gfx] [PATCH] drm/i915: Switch TGL-H DP-IN to dGFX when it's supported

[Lukas Wunner]

On Tue, Aug 16, 2022 at 11:06:18AM +0300, Jani Nikula wrote:
> On Tue, 16 Aug 2022, Kai-Heng Feng  wrote:
> > On mobile workstations like HP ZBook Fury G8, iGFX's DP-IN can switch to
> > dGFX so external monitors are routed to dGFX, and more monitors can be
> > supported as result.
> >
> > To switch the DP-IN to dGFX, the driver needs to invoke _DSM function 20
> > on intel_dsm_guid2. This method is described in Intel document 632107.
> 
> Is this the policy decision that we want to unconditionally make,
> though?

In general, we handle switching of outputs between GPUs in vga_switcheroo.c
upon a request from user space via sysfs (well, debugfs currently).
It's up to users to decide which policy suits their needs best.

That said, we never grew support to allow different switching policies for
the built-in panel and external outputs.  Laptops supporting this are
rare.  Older MacBook Pros introduced between 2008 and 2010 are among them:
They have separate muxes for the panel and external DP port.  Our policy
is documented in a code comment in drivers/platform/x86/apple-gmux.c:

 * The external DP port is only fully switchable on the first two unibody
 * MacBook Pro generations, MBP5 2008/09 and MBP6 2010. This is done by an
 * `NXP CBTL06141`_ which is controlled by gmux.
 [...]
 * Our switching policy for the external port is that on those generations
 * which are able to switch it fully, the port is switched together with the
 * panel when IGD / DIS commands are issued to vga_switcheroo. It is thus
 * possible to drive e.g. a beamer on battery power with the integrated GPU.
 * The user may manually switch to the discrete GPU if more performance is
 * needed.
 *
 * On all newer generations, the external port can only be driven by the
 * discrete GPU. If a display is plugged in while the panel is switched to
 * the integrated GPU, *both* GPUs will be in use for maximum performance.
 * To decrease power consumption, the user may manually switch to the
 * discrete GPU, thereby suspending the integrated GPU.

In other words, on these older MacBook Pros, we switch the panel and
external DP port in unison, thus always allowing one of the two GPUs
to runtime suspend and save power.

Thanks,

Lukas

Re: [Intel-gfx] [PATCH v2 4/4] vgaswitcheroo: do not check for NULL debugfs dentry

[Lukas Wunner]

On Wed, Oct 13, 2021 at 08:36:01PM +0200, Nirmoy Das wrote:
> Debugfs APIs returns encoded error on failure so use
> debugfs_lookup() instead of checking for NULL.

The commit message no longer matches up with the patch itself
(debugfs_lookup() isn't called).

My suggestion would be something like:

  Retry creation of the vga_switcheroo debugfs if a previous
  invocation of debugfs_create_dir() returned an error code.

With that addressed,
Reviewed-by: Lukas Wunner 

Thanks,

Lukas

> Signed-off-by: Nirmoy Das 
> ---
>  drivers/gpu/vga/vga_switcheroo.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/vga/vga_switcheroo.c 
> b/drivers/gpu/vga/vga_switcheroo.c
> index 365e6ddbe90f..07ab8d85e899 100644
> --- a/drivers/gpu/vga/vga_switcheroo.c
> +++ b/drivers/gpu/vga/vga_switcheroo.c
> @@ -914,7 +914,7 @@ static void vga_switcheroo_debugfs_fini(struct vgasr_priv 
> *priv)
>  static void vga_switcheroo_debugfs_init(struct vgasr_priv *priv)
>  {
>   /* already initialised */
> - if (priv->debugfs_root)
> + if (priv->debugfs_root && !IS_ERR(priv->debugfs_root))
>   return;
> 
>   priv->debugfs_root = debugfs_create_dir("vgaswitcheroo", NULL);
> --
> 2.32.0

Re: [Intel-gfx] [PATCH 4/4] vgaswitcheroo: do not check for NULL debugfs dentry

[Lukas Wunner]

On Mon, Oct 11, 2021 at 10:24:29PM +0200, Lukas Wunner wrote:
> On Mon, Oct 11, 2021 at 09:06:07PM +0200, Nirmoy Das wrote:
> > Debugfs APIs returns encoded error on failure so use
> > debugfs_lookup() instead of checking for NULL.
> [...]
> > --- a/drivers/gpu/vga/vga_switcheroo.c
> > +++ b/drivers/gpu/vga/vga_switcheroo.c
> > @@ -914,7 +914,7 @@ static void vga_switcheroo_debugfs_fini(struct 
> > vgasr_priv *priv)
> >  static void vga_switcheroo_debugfs_init(struct vgasr_priv *priv)
> >  {
> > /* already initialised */
> > -   if (priv->debugfs_root)
> > +   if (debugfs_lookup("vgaswitcheroo", NULL))
> > return;
> > 
> > priv->debugfs_root = debugfs_create_dir("vgaswitcheroo", NULL);
> 
> If debugfs_create_dir() returns an error code, it does make sense to
> retry the call when another vga_switcheroo client registers later.
> I like that.
> 
> However I'd prefer simply changing this to explicitly check for NULL, i.e.:
> 
> - if (priv->debugfs_root)
> + if (priv->debugfs_root == NULL)

Apologies, I meant:

-   if (priv->debugfs_root)
+   if (priv->debugfs_root && !IS_ERR(priv->debugfs_root))

Thanks,

Lukas

> It's just as clear as calling debugfs_lookup() and it has way less
> overhead.  Granted, this isn't a hot path, but it's called on boot,
> and the less code we execute, the faster the machine boots.

Re: [Intel-gfx] [PATCH 4/4] vgaswitcheroo: do not check for NULL debugfs dentry

[Lukas Wunner]

On Mon, Oct 11, 2021 at 09:06:07PM +0200, Nirmoy Das wrote:
> Debugfs APIs returns encoded error on failure so use
> debugfs_lookup() instead of checking for NULL.
[...]
> --- a/drivers/gpu/vga/vga_switcheroo.c
> +++ b/drivers/gpu/vga/vga_switcheroo.c
> @@ -914,7 +914,7 @@ static void vga_switcheroo_debugfs_fini(struct vgasr_priv 
> *priv)
>  static void vga_switcheroo_debugfs_init(struct vgasr_priv *priv)
>  {
>   /* already initialised */
> - if (priv->debugfs_root)
> + if (debugfs_lookup("vgaswitcheroo", NULL))
>   return;
> 
>   priv->debugfs_root = debugfs_create_dir("vgaswitcheroo", NULL);

If debugfs_create_dir() returns an error code, it does make sense to
retry the call when another vga_switcheroo client registers later.
I like that.

However I'd prefer simply changing this to explicitly check for NULL, i.e.:

-   if (priv->debugfs_root)
+   if (priv->debugfs_root == NULL)

It's just as clear as calling debugfs_lookup() and it has way less
overhead.  Granted, this isn't a hot path, but it's called on boot,
and the less code we execute, the faster the machine boots.

Thanks,

Lukas

Re: [Intel-gfx] [PATCH RESEND] drm/i915: register vga switcheroo later, unregister earlier

[Lukas Wunner]

On Sun, Oct 06, 2019 at 12:46:43PM +0300, Jani Nikula wrote:
> Move vga switcheroo and dsm handler register later in
> i915_driver_register(), and unregister in i915_driver_unregister(). The
> dsm handler unregister is a nop, and is only added for completeness.
> 
> My unsubstantiated suspicion is that the vga switcheroo state change
> would not work as early as we register the hooks currently. In any case
> exposing the interfaces to the world only after we've got everything set
> up seems prudent.
> 
> Also replace the error handling in vga switcheroo register with a simple
> error message. This is done at the same time due to lack of error
> propagation from i915_driver_register().
> 
> Signed-off-by: Jani Nikula 

Acked-by: Lukas Wunner 

Seems like a good idea to limp on instead of bailing out if vga_switcheroo
registration fails.

Taking a brief look at intel_acpi.c, I notice intel_dsm_detect() checks
for presence of exactly 2 devices with class PCI_CLASS_DISPLAY_VGA.
This breaks if a third GPU is attached via Thunderbolt or CardBus.
Not sure if that can happen in reality, I suspect the DSM is only
present on older laptops?

I'm also confused that intel_acpi.c doesn't register a vga_switcheroo
handler.  Is the mux controlled by a separate ACPI method and the one
checked for in intel_acpi.c is only used to gather system information?

Thanks,

Lukas

> ---
>  drivers/gpu/drm/i915/i915_drv.c | 19 +--
>  1 file changed, 9 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index 9354924576c4..63d47d699305 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -289,12 +289,6 @@ static int i915_driver_modeset_probe(struct 
> drm_i915_private *i915)
>   if (ret)
>   goto out;
>  
> - intel_register_dsm_handler();
> -
> - ret = i915_switcheroo_register(i915);
> - if (ret)
> - goto cleanup_vga_client;
> -
>   /* must happen before intel_power_domains_init_hw() on VLV/CHV */
>   intel_update_rawclk(i915);
>  
> @@ -343,8 +337,6 @@ static int i915_driver_modeset_probe(struct 
> drm_i915_private *i915)
>  cleanup_csr:
>   intel_csr_ucode_fini(i915);
>   intel_power_domains_driver_remove(i915);
> - i915_switcheroo_unregister(i915);
> -cleanup_vga_client:
>   intel_vga_unregister(i915);
>  out:
>   return ret;
> @@ -356,8 +348,6 @@ static void i915_driver_modeset_remove(struct 
> drm_i915_private *i915)
>  
>   intel_bios_driver_remove(i915);
>  
> - i915_switcheroo_unregister(i915);
> -
>   intel_vga_unregister(i915);
>  
>   intel_csr_ucode_fini(i915);
> @@ -1344,6 +1334,11 @@ static void i915_driver_register(struct 
> drm_i915_private *dev_priv)
>  
>   intel_power_domains_enable(dev_priv);
>   intel_runtime_pm_enable(_priv->runtime_pm);
> +
> + intel_register_dsm_handler();
> +
> + if (i915_switcheroo_register(dev_priv))
> + DRM_ERROR("Failed to register vga switcheroo!\n");
>  }
>  
>  /**
> @@ -1352,6 +1347,10 @@ static void i915_driver_register(struct 
> drm_i915_private *dev_priv)
>   */
>  static void i915_driver_unregister(struct drm_i915_private *dev_priv)
>  {
> + i915_switcheroo_unregister(dev_priv);
> +
> + intel_unregister_dsm_handler();
> +
>   intel_runtime_pm_disable(_priv->runtime_pm);
>   intel_power_domains_disable(dev_priv);
>  
> -- 
> 2.20.1
> 
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 30/33] vgaswitcheroo: call fbcon_remap_all directly

[Lukas Wunner]

On Mon, May 20, 2019 at 10:22:13AM +0200, Daniel Vetter wrote:
> --- a/drivers/video/fbdev/core/fbcon.c
> +++ b/drivers/video/fbdev/core/fbcon.c
> @@ -3146,16 +3146,16 @@ void fbcon_fb_unregistered(struct fb_info *info)
>  }
>  
>  /* called with console_lock held */
> -static void fbcon_remap_all(int idx)

That comment needs to be removed as well.

Not an expert on fbcon code but this looks sane to me, so in case it helps:
Acked-by: Lukas Wunner 

Thanks,

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

Re: [Intel-gfx] [PATCH] drm/i915: Ensure proper HDA suspend/resume ordering with a device link

[Lukas Wunner]

On Thu, Oct 18, 2018 at 05:25:58PM +0300, Imre Deak wrote:
> In order to ensure that our system suspend and resume callbacks are
> called in the correct order wrt. those of the HDA driver add a device
> link to the HDA driver during audio component binding time. With i915 as
> the supplier and HDA as the consumer the PM framework will guarantee
> the HDA->i915 suspend (and shutdown) and i915->HDA resume order.

Thanks for doing this.  Can you also revert 7b3b61b62a58 ("drm/todo:
i915 could use device_link_add") while at it?
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [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.d...@intel.com>
> > > > Signed-off-by: Lukas Wunner <lu...@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
> int

Re: [Intel-gfx] [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.d...@intel.com>
> > Signed-off-by: Lukas Wunner <lu...@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

[Intel-gfx] [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.d...@intel.com>
Signed-off-by: Lukas Wunner <lu...@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, _switcheroo_ops, false);
+   ret = vga_switcheroo_register_client(pdev, _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: [Intel-gfx] [Nouveau] [PATCH 0/5] Fix deadlock on runtime suspend in DRM drivers

[Lukas Wunner]

On Mon, Feb 19, 2018 at 03:05:53PM +0100, Daniel Vetter wrote:
> On Mon, Feb 19, 2018 at 12:58:17PM +0100, Lukas Wunner wrote:
> > On Mon, Feb 19, 2018 at 12:34:43PM +0100, Daniel Vetter wrote:
> > > Well, userspace expects hotplug events, even when we runtime suspend
> > > stuff. Hence waking shit up with polling. Imo ignoring hotplug events is a
> > > pretty serious policy decision which is ok in the context of
> > > vga_switcheroo, but not really as an automatic thing. E.g. usb also wakes
> > > up if you plug something in, even with all the runtime pm stuff enabled.
> > > Same for sata and everything else.
> > 
> > On the MacBook Pro, the HPD pin of external DP and HDMI ports goes into
> > the gmux controller, which sends an interrupt on hotplug even if the GPU
> > is powered down.
> > 
> > Apparently Optimus has a similar functionality, cf. 3a6536c51d5d.
> 
> Yeah, for vga_switcheroo and gmux setups shutting down polling explicitly
> makes sense. I think ideally we'd stop polling in the gmux handler somehow
> (maybe by dropping the relevant DRM_CONNECTOR_POLL flags, or outright
> stopping it all). But not when runtime suspending the entire gpu (e.g.
> idle system that shuts down the screen and everything, before it decides
> a few minutes later to do a full system suspend).

nouveau, radeon and amdgpu currently use runtime PM *only* on hybrid
graphics laptops.

Should the drivers later be extended to also use runtime PM in other
scenarios (desktop machines, eGPUs), they can easily detect whether
to disable polling on runtime suspend by calling apple_gmux_present()
on Macs or the equivalent for Optimus/ATPX.

Thanks,

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

Re: [Intel-gfx] [PATCH 0/5] Fix deadlock on runtime suspend in DRM drivers

[Lukas Wunner]

On Mon, Feb 19, 2018 at 12:48:04PM +0100, Daniel Vetter wrote:
> On Thu, Feb 15, 2018 at 06:38:44AM +0100, Lukas Wunner wrote:
> > On Wed, Feb 14, 2018 at 09:58:43AM -0500, Sean Paul wrote:
> > > On Wed, Feb 14, 2018 at 03:43:56PM +0100, Michel Dänzer wrote:
> > > > On 2018-02-14 03:08 PM, Sean Paul wrote:
> > > > > On Wed, Feb 14, 2018 at 10:26:35AM +0100, Maarten Lankhorst wrote:
> > > > >> Op 14-02-18 om 09:46 schreef Lukas Wunner:
> > > > >>> On Sun, Feb 11, 2018 at 10:38:28AM +0100, Lukas Wunner wrote:
> > > > >>>> Fix a deadlock on hybrid graphics laptops that's been present 
> > > > >>>> since 2013:
> > > > >>> This series has been reviewed, consent has been expressed by the 
> > > > >>> most
> > > > >>> interested parties, patch [1/5] which touches files outside 
> > > > >>> drivers/gpu
> > > > >>> has been acked and I've just out a v2 addressing the only objection
> > > > >>> raised.  My plan is thus to wait another two days for comments and,
> > > > >>> barring further objections, push to drm-misc this weekend.
> > > > >>>
> > > > >>> However I'm struggling with the decision whether to push to next or
> > > > >>> fixes.  The series is marked for stable, however the number of
> > > > >>> affected machines is limited and for an issue that's been present
> > > > >>> for 5 years it probably doesn't matter if it soaks another two 
> > > > >>> months
> > > > >>> in linux-next befor it gets backported.  Hence I tend to err on the
> > > > >>> side of caution and push to next, however a case could be made that
> > > > >>> fixes is more appropriate.
> > > > >>>
> > > > >>> I'm lacking experience making such decisions and would be interested
> > > > >>> to learn how you'd handle this.
> > > > >>
> > > > >> I would say fixes, it doesn't look particularly scary. :)
> > > > > 
> > > > > Agreed. If it's good enough for stable, it's good enough for -fixes!
> > > > 
> > > > It's not that simple, is it? Fast-tracking patches (some of which appear
> > > > to be untested) to stable without an immediate cause for urgency seems
> > > > risky to me.
> > > 
> > > /me should be more careful what he says
> > > 
> > > Given where we are in the release cycle, it's barely a fast track.
> > > If these go in -fixes, they'll get in -rc2 and will have plenty of
> > > time to bake. If we were at rc5, it might be a different story.
> > 
> > The patches are marked for stable though, so if they go in through
> > drm-misc-fixes, they may appear in stable kernels before 4.16-final
> > is out.  Greg picks up patches once they're in Linus' tree, though
> > often with a delay of a few days or weeks.  If they go in through
> > drm-misc-next, they're guaranteed not to appear in *any* release
> > before 4.16-final is out.
> > 
> > This allows for differentiation between no-brainer stable fixes that
> > can be sent immediately and scarier, but similarly important stable
> > fixes that should soak for a while.  I'm not sure which category
> > this series belongs to, though it's true what Maarten says, it's
> > not *that* grave a change.
> 
> If you're this concerned about them, then pls do _not_ put cc: stable on
> the patches. Instead get them merged through -fixes (or maybe even -next),
> and once they're sufficiently tested, send a mail to stable@ asking for
> ane explicit backport.

I'm not concerned about them, but would have erred on the side of caution.
However consensus seems to have been that they're sufficiently unscary to
push to -fixes.  Do you disagree with that decision, if so, why?  Can we
amend the dim docs to codify guidelines whether to push to -fixes or -next?
I allowed 1 week for comments, now you're returning from vacation and seem
to be unhappy, was 1 week too short?

Thanks,

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

Re: [Intel-gfx] [Nouveau] [PATCH 0/5] Fix deadlock on runtime suspend in DRM drivers

[Lukas Wunner]

On Mon, Feb 19, 2018 at 12:34:43PM +0100, Daniel Vetter wrote:
> On Sun, Feb 11, 2018 at 10:38:28AM +0100, Lukas Wunner wrote:
> > Fix a deadlock on hybrid graphics laptops that's been present since 2013:
> > 
> > DRM drivers poll connectors in 10 sec intervals.  The poll worker is
> > stopped on ->runtime_suspend with cancel_delayed_work_sync().  However
> > the poll worker invokes the DRM drivers' ->detect callbacks, which call
> > pm_runtime_get_sync().  If the poll worker starts after runtime suspend
> > has begun, pm_runtime_get_sync() will wait for runtime suspend to finish
> > with the intention of runtime resuming the device afterwards.  The result
> > is a circular wait between poll worker and autosuspend worker.
> 
> Don't shut down the poll worker when runtime suspending, that' doesn't
> work. If you need the poll work, then that means waking up the gpu every
> few seconds. If you don't need it, then make sure the DRM_CONNECTOR_POLL
> flags are set correctly (you can update them at runtime, the poll worker
> will pick that up).
> 
> That should fix the deadlock, and it's how we do it in i915 (where igt in
> CI totally hammers the runtime pm support, and it seems to hold up).

It would fix the deadlock but it's not an option on dual GPU laptops.
Power consumption of the discrete GPU is massive (9 W on my machine).


> > i915, malidp and msm "solved" this issue by not stopping the poll worker
> > on runtime suspend.  But this results in the GPU bouncing back and forth
> > between D0 and D3 continuously.  That's a total no-go for GPUs which
> > runtime suspend to D3cold since every suspend/resume cycle costs a
> > significant amount of time and energy.  (i915 and malidp do not seem
> > to acquire a runtime PM ref in the ->detect callbacks, which seems
> > questionable.  msm however does and would also deadlock if it disabled
> > the poll worker on runtime suspend.  cc += Archit, Liviu, intel-gfx)
> 
> Well, userspace expects hotplug events, even when we runtime suspend
> stuff. Hence waking shit up with polling. Imo ignoring hotplug events is a
> pretty serious policy decision which is ok in the context of
> vga_switcheroo, but not really as an automatic thing. E.g. usb also wakes
> up if you plug something in, even with all the runtime pm stuff enabled.
> Same for sata and everything else.

On the MacBook Pro, the HPD pin of external DP and HDMI ports goes into
the gmux controller, which sends an interrupt on hotplug even if the GPU
is powered down.

Apparently Optimus has a similar functionality, cf. 3a6536c51d5d.

For the rare cases where an external VGA or DVI-A port is present, I guess
it's reasonable to have the user wake up the machine manually.

I'm not sure why nouveau polls ports on my laptop, the GK107 only has an
LVDS and three DP ports, need to investigate.

Thanks,

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

Re: [Intel-gfx] [PATCH 0/5] Fix deadlock on runtime suspend in DRM drivers

[Lukas Wunner]

On Sun, Feb 11, 2018 at 10:38:28AM +0100, Lukas Wunner wrote:
>   workqueue: Allow retrieval of current task's work struct
>   drm: Allow determining if current task is output poll worker
>   drm/nouveau: Fix deadlock on runtime suspend
>   drm/radeon: Fix deadlock on runtime suspend
>   drm/amdgpu: Fix deadlock on runtime suspend

Pushed to drm-misc-fixes, thanks a lot everyone for the acks,
reviews, testing and comments.

drm-misc maintainers, heads-up:

drm-misc-fixes is still based on 4.15-rc8.  The present series
applies cleanly to both 4.15 and 4.16, so I had no need to have
4.16-rc1 backmerged, but that may be necessary sooner or later.
I did a local test pull into drm-fixes, the shortlog looked sane
and it merged and compiled cleanly.

Please note two other commits are still pending in drm-misc-fixes:

commit 745fd50f3b044db6a3922e1718306555613164b0
Author: Daniel Vetter <daniel.vet...@ffwll.ch>
Date:   Wed Jan 31 12:04:50 2018 +0100

drm/cirrus: Load lut in crtc_commit

Gustavo sent a pull request for this one on Jan 31 but erroneously
based it on the wrong commit and it ended up not being pulled by Dave.

commit 54f809cfbd6b4a43959039f5d33596ed3297ce16
Author: Leo (Sunpeng) Li <sunpeng...@amd.com>
Date:   Wed Jan 17 12:51:08 2018 +0100

drm/atomic: Fix memleak on ERESTARTSYS during non-blocking commits

This one has already been pulled by Dave into drm-next for 4.17
as commit 1c6c6ebb but Maarten subsequently cherry-picked
it onto drm-misc-fixes.

Let me know if I've made any mistakes.

Thanks,

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

[Intel-gfx] i915 runtime PM (was: Re: [PATCH 0/5] Fix deadlock on runtime suspend in DRM drivers)

[Lukas Wunner]

[trimming cc: a little to avoid spamming folks not directly involved with i915]

On Mon, Feb 12, 2018 at 03:04:06PM +0200, Imre Deak wrote:
> On Sun, Feb 11, 2018 at 10:38:28AM +0100, Lukas Wunner wrote:
> > i915, malidp and msm "solved" this issue by not stopping the poll worker
> > on runtime suspend.  But this results in the GPU bouncing back and forth
> > between D0 and D3 continuously.  That's a total no-go for GPUs which
> > runtime suspend to D3cold since every suspend/resume cycle costs a
> > significant amount of time and energy.  (i915 and malidp do not seem
> > to acquire a runtime PM ref in the ->detect callbacks, which seems
> > questionable.  msm however does and would also deadlock if it disabled
> > the poll worker on runtime suspend.  cc += Archit, Liviu, intel-gfx)
> 
> In i915 polling is on during runtime suspend only if there are outputs
> without hotplug interrupt support. A special case is when an output has
> working HPD interrupts when in D0, but no interrupts when runtime
> suspended. For these we start polling (from a scheduled work) in the
> runtime suspend hook and stop it in the runtime resume hook (again from
> a scheduled work).

I'm assuming to poll outputs you need to access mmio, is that correct?
Since mmio is inaccessible in D3hot, you seem to leave the PCI device
in D0 during runtime suspend, right?  Aren't you leaving power saving
potential on the table that way?  Or are you able to achieve the same
low power consumption in D0 as in D3hot by turning off power wells etc?

When powering off the GPU via vga_switcheroo manual power control
(a legacy feature we'll hopefully drop once we have runtime PM
support on muxed laptops and in i915) the PCI device *is* put into
D3hot by i915_suspend_switcheroo().  If leaving the device in D0
in the runtime suspend code path results in less power reduction,
runtime PM wouldn't be a full replacement for vga_switcheroo manual
power control, which would be kind of a bummer. :-/

Another question, you're not calling pm_runtime_allow() when binding
the driver to the device, so users have to either manually allow it
via sysfs or use "powertop --auto-tune" or whatever.  What's the
rationale for that?  nouveau, radeon and amdgpu all allow it by
default.

Thanks,

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

Re: [Intel-gfx] [PATCH 0/5] Fix deadlock on runtime suspend in DRM drivers

[Lukas Wunner]

On Wed, Feb 14, 2018 at 09:58:43AM -0500, Sean Paul wrote:
> On Wed, Feb 14, 2018 at 03:43:56PM +0100, Michel Dänzer wrote:
> > On 2018-02-14 03:08 PM, Sean Paul wrote:
> > > On Wed, Feb 14, 2018 at 10:26:35AM +0100, Maarten Lankhorst wrote:
> > >> Op 14-02-18 om 09:46 schreef Lukas Wunner:
> > >>> On Sun, Feb 11, 2018 at 10:38:28AM +0100, Lukas Wunner wrote:
> > >>>> Fix a deadlock on hybrid graphics laptops that's been present since 
> > >>>> 2013:
> > >>> This series has been reviewed, consent has been expressed by the most
> > >>> interested parties, patch [1/5] which touches files outside drivers/gpu
> > >>> has been acked and I've just out a v2 addressing the only objection
> > >>> raised.  My plan is thus to wait another two days for comments and,
> > >>> barring further objections, push to drm-misc this weekend.
> > >>>
> > >>> However I'm struggling with the decision whether to push to next or
> > >>> fixes.  The series is marked for stable, however the number of
> > >>> affected machines is limited and for an issue that's been present
> > >>> for 5 years it probably doesn't matter if it soaks another two months
> > >>> in linux-next befor it gets backported.  Hence I tend to err on the
> > >>> side of caution and push to next, however a case could be made that
> > >>> fixes is more appropriate.
> > >>>
> > >>> I'm lacking experience making such decisions and would be interested
> > >>> to learn how you'd handle this.
> > >>
> > >> I would say fixes, it doesn't look particularly scary. :)
> > > 
> > > Agreed. If it's good enough for stable, it's good enough for -fixes!
> > 
> > It's not that simple, is it? Fast-tracking patches (some of which appear
> > to be untested) to stable without an immediate cause for urgency seems
> > risky to me.
> 
> /me should be more careful what he says
> 
> Given where we are in the release cycle, it's barely a fast track.
> If these go in -fixes, they'll get in -rc2 and will have plenty of
> time to bake. If we were at rc5, it might be a different story.

The patches are marked for stable though, so if they go in through
drm-misc-fixes, they may appear in stable kernels before 4.16-final
is out.  Greg picks up patches once they're in Linus' tree, though
often with a delay of a few days or weeks.  If they go in through
drm-misc-next, they're guaranteed not to appear in *any* release
before 4.16-final is out.

This allows for differentiation between no-brainer stable fixes that
can be sent immediately and scarier, but similarly important stable
fixes that should soak for a while.  I'm not sure which category
this series belongs to, though it's true what Maarten says, it's
not *that* grave a change.

Thanks,

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

Re: [Intel-gfx] [PATCH 0/5] Fix deadlock on runtime suspend in DRM drivers

[Lukas Wunner]

On Tue, Feb 13, 2018 at 03:46:08PM +, Liviu Dudau wrote:
> On Tue, Feb 13, 2018 at 12:52:06PM +0100, Lukas Wunner wrote:
> > On Tue, Feb 13, 2018 at 10:55:06AM +, Liviu Dudau wrote:
> > > On Sun, Feb 11, 2018 at 10:38:28AM +0100, Lukas Wunner wrote:
> > > > DRM drivers poll connectors in 10 sec intervals.  The poll worker is
> > > > stopped on ->runtime_suspend with cancel_delayed_work_sync().  However
> > > > the poll worker invokes the DRM drivers' ->detect callbacks, which call
> > > > pm_runtime_get_sync().  If the poll worker starts after runtime suspend
> > > > has begun, pm_runtime_get_sync() will wait for runtime suspend to finish
> > > > with the intention of runtime resuming the device afterwards.  The 
> > > > result
> > > > is a circular wait between poll worker and autosuspend worker.
> > > 
> > > I think I understand the problem you are trying to solve, but I'm
> > > struggling to understand where malidp makes any specific mistakes. First
> > > of all, malidp is only a display engine, so there is no GPU attached to
> > > it, but that is only a small clarification. Second, malidp doesn't use
> > > directly any of the callbacks that you are referring to, it uses the
> > > drm_cma_() API plus the generic drm_() call. So if there are any
> > > issues there (as they might well be) I think they would apply to a lot
> > > more drivers and the fix will involve more than just malidp, i915 and
> > > msm.
[snip]
> > There are no ->detect hooks declared
> > in drivers/gpu/drm/arm/, so it's unclear to me whether you're able to probe
> > during runtime suspend.
> 
> That's because the drivers in drivers/gpu/drm/arm do not have
> connectors, they are only the CRTC part of the driver. Both hdlcd and
> mali-dp use the component framework to locate an encoder in device tree
> that will then provide the connectors.
> 
> > 
> > hdlcd_drv.c and malidp_drv.c both enable output polling.  Output polling
> > is only necessary if you don't get HPD interrupts.
> 
> That's right, hdlcd and mali-dp don't receive HPD interrupts because
> they don't have any. And because we don't know ahead of time which
> encoder/connector will be paired with the driver, we enable polling as a
> safe fallback.
> 

Looking e.g. at inno_hdmi.c (used by rk3036.dtsi), this calls
drm_helper_hpd_irq_event() on receiving an HPD interrupt, and that
function returns immediately if polling is not enabled.  So you *have*
to enable polling to receive HPD events.

You seem to keep the crtc runtime active as long as it's bound to an
encoder.  If you do not ever intend to runtime suspend the crtc while
an encoder is attached, you don't need to keep polling enabled during
runtime suspend (because there's nothing to poll), but it shouldn't
hurt either.

If you would runtime suspend while an encoder is attached, then you
would only runtime resume every 10 sec (upon polling) if the encoder
was a child of the crtc and would support runtime suspend as well.
That's because the PM core wakes the parent by default when a child
runtime resumes.  However in the DT's I've looked at, the encoder
is never a child of the crtc and at least inno_hdmi.c doesn't use
runtime suspend.

So I think you're all green, I can't spot any grave issues here.
Just be aware of the above-mentioned constraints.

Thanks,

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

Re: [Intel-gfx] [PATCH 0/5] Fix deadlock on runtime suspend in DRM drivers

[Lukas Wunner]

Dear drm-misc maintainers,

On Sun, Feb 11, 2018 at 10:38:28AM +0100, Lukas Wunner wrote:
> Fix a deadlock on hybrid graphics laptops that's been present since 2013:

This series has been reviewed, consent has been expressed by the most
interested parties, patch [1/5] which touches files outside drivers/gpu
has been acked and I've just out a v2 addressing the only objection
raised.  My plan is thus to wait another two days for comments and,
barring further objections, push to drm-misc this weekend.

However I'm struggling with the decision whether to push to next or
fixes.  The series is marked for stable, however the number of
affected machines is limited and for an issue that's been present
for 5 years it probably doesn't matter if it soaks another two months
in linux-next befor it gets backported.  Hence I tend to err on the
side of caution and push to next, however a case could be made that
fixes is more appropriate.

I'm lacking experience making such decisions and would be interested
to learn how you'd handle this.

Thanks,

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

Re: [Intel-gfx] [PATCH 2/5] drm: Allow determining if current task is output poll worker

[Lukas Wunner]

On Mon, Feb 12, 2018 at 12:46:11PM -0500, Lyude Paul wrote:
> On Sun, 2018-02-11 at 10:38 +0100, Lukas Wunner wrote:
> > Introduce a helper to determine if the current task is an output poll
> > worker.
> > 
> > This allows us to fix a long-standing deadlock in several DRM drivers
> > wherein the ->runtime_suspend callback waits for the output poll worker
> > to finish and the worker in turn calls a ->detect callback which waits
> > for runtime suspend to finish.  The ->detect callback is invoked from
> > multiple call sites and waiting for runtime suspend to finish is the
> > correct thing to do except if it's executing in the context of the
> > worker.
[snip]
> > +/**
> > + * drm_kms_helper_is_poll_worker - is %current task an output poll worker?
> > + *
> > + * Determine if %current task is an output poll worker.  This can be used
> > + * to select distinct code paths for output polling versus other contexts.
> > + */
>
> For this, it would be worth explicitly noting in the comments herethat this
> should be called by DRM drivers in order to prevent racing with hotplug
> polling workers, so that new drivers in the future can avoid implementing this
> race condition in their driver.

Good point, I've just sent out a v2 to address your comment.  Let me know
if this isn't what you had in mind.  It may also be worth to expand the
DOC section at the top of drm_probe_helper.c to explain the interaction
between polling and runtime suspend in more detail, but I think this is
better done in a separate patch to keep the present patch small and thus
easily backportable to stable.

Thanks a lot for the review,

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

[Intel-gfx] [PATCH v2] drm: Allow determining if current task is output poll worker

[Lukas Wunner]

Introduce a helper to determine if the current task is an output poll
worker.

This allows us to fix a long-standing deadlock in several DRM drivers
wherein the ->runtime_suspend callback waits for the output poll worker
to finish and the worker in turn calls a ->detect callback which waits
for runtime suspend to finish.  The ->detect callback is invoked from
multiple call sites and waiting for runtime suspend to finish is the
correct thing to do except if it's executing in the context of the
worker.

v2: Expand kerneldoc to specifically mention deadlock between
output poll worker and autosuspend worker as use case. (Lyude)

Cc: Dave Airlie <airl...@redhat.com>
Cc: Ben Skeggs <bske...@redhat.com>
Cc: Alex Deucher <alexander.deuc...@amd.com>
Reviewed-by: Lyude Paul <ly...@redhat.com>
Signed-off-by: Lukas Wunner <lu...@wunner.de>
---
 drivers/gpu/drm/drm_probe_helper.c | 20 
 include/drm/drm_crtc_helper.h  |  1 +
 2 files changed, 21 insertions(+)

diff --git a/drivers/gpu/drm/drm_probe_helper.c 
b/drivers/gpu/drm/drm_probe_helper.c
index 6dc2dde5b672..7a6b2dc08913 100644
--- a/drivers/gpu/drm/drm_probe_helper.c
+++ b/drivers/gpu/drm/drm_probe_helper.c
@@ -654,6 +654,26 @@ static void output_poll_execute(struct work_struct *work)
schedule_delayed_work(delayed_work, DRM_OUTPUT_POLL_PERIOD);
 }
 
+/**
+ * drm_kms_helper_is_poll_worker - is %current task an output poll worker?
+ *
+ * Determine if %current task is an output poll worker.  This can be used
+ * to select distinct code paths for output polling versus other contexts.
+ *
+ * One use case is to avoid a deadlock between the output poll worker and
+ * the autosuspend worker wherein the latter waits for polling to finish
+ * upon calling drm_kms_helper_poll_disable(), while the former waits for
+ * runtime suspend to finish upon calling pm_runtime_get_sync() in a
+ * connector ->detect hook.
+ */
+bool drm_kms_helper_is_poll_worker(void)
+{
+   struct work_struct *work = current_work();
+
+   return work && work->func == output_poll_execute;
+}
+EXPORT_SYMBOL(drm_kms_helper_is_poll_worker);
+
 /**
  * drm_kms_helper_poll_disable - disable output polling
  * @dev: drm_device
diff --git a/include/drm/drm_crtc_helper.h b/include/drm/drm_crtc_helper.h
index 76e237bd989b..6914633037a5 100644
--- a/include/drm/drm_crtc_helper.h
+++ b/include/drm/drm_crtc_helper.h
@@ -77,5 +77,6 @@ void drm_kms_helper_hotplug_event(struct drm_device *dev);
 
 void drm_kms_helper_poll_disable(struct drm_device *dev);
 void drm_kms_helper_poll_enable(struct drm_device *dev);
+bool drm_kms_helper_is_poll_worker(void);
 
 #endif
-- 
2.15.1

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

Re: [Intel-gfx] [PATCH 0/5] Fix deadlock on runtime suspend in DRM drivers

[Lukas Wunner]

On Tue, Feb 13, 2018 at 10:55:06AM +, Liviu Dudau wrote:
> On Sun, Feb 11, 2018 at 10:38:28AM +0100, Lukas Wunner wrote:
> > DRM drivers poll connectors in 10 sec intervals.  The poll worker is
> > stopped on ->runtime_suspend with cancel_delayed_work_sync().  However
> > the poll worker invokes the DRM drivers' ->detect callbacks, which call
> > pm_runtime_get_sync().  If the poll worker starts after runtime suspend
> > has begun, pm_runtime_get_sync() will wait for runtime suspend to finish
> > with the intention of runtime resuming the device afterwards.  The result
> > is a circular wait between poll worker and autosuspend worker.
> 
> I think I understand the problem you are trying to solve, but I'm
> struggling to understand where malidp makes any specific mistakes. First
> of all, malidp is only a display engine, so there is no GPU attached to
> it, but that is only a small clarification. Second, malidp doesn't use
> directly any of the callbacks that you are referring to, it uses the
> drm_cma_() API plus the generic drm_() call. So if there are any
> issues there (as they might well be) I think they would apply to a lot
> more drivers and the fix will involve more than just malidp, i915 and
> msm.

I don't know if malidp makes any specific mistakes and didn't mean to
cast it in a negative light, sorry.

So a lot of DRM drivers acquire a runtime PM ref in the connector ->detect
hook because they can't probe connectors while runtime suspended.
E.g. for a PCI device, probing might require mmio access, which isn't
possible outside of power state D0.  There are no ->detect hooks declared
in drivers/gpu/drm/arm/, so it's unclear to me whether you're able to probe
during runtime suspend.

hdlcd_drv.c and malidp_drv.c both enable output polling.  Output polling
is only necessary if you don't get HPD interrupts.

You're not disabling polling upon runtime suspend.  Thus, if a runtime PM
ref is acquired during polling (such as in a ->detect hook), the GPU will
be runtime resumed every 10 secs.  You may want to verify that's not the
case.  If you decide that you do want to stop polling during runtime
suspend because it runtime resumes the GPU continuously, you'll need the
helper introduced in this series.  So by cc'ing you I just wanted to keep
you in the loop about an issue that may potentially affect your driver.

Let me know if there are any questions.

Thanks,

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

Re: [Intel-gfx] [PATCH 0/5] Fix deadlock on runtime suspend in DRM drivers

[Lukas Wunner]

On Mon, Feb 12, 2018 at 01:58:32PM -0500, Alex Deucher wrote:
> On Mon, Feb 12, 2018 at 4:45 AM, Lukas Wunner <lu...@wunner.de> wrote:
> > On Mon, Feb 12, 2018 at 09:03:26AM +, Mike Lothian wrote:
> >> On 12 February 2018 at 03:39, Lukas Wunner <lu...@wunner.de> wrote:
> >> > On Mon, Feb 12, 2018 at 12:35:51AM +, Mike Lothian wrote:
> >> > > I've not been able to reproduce the original problem you're trying to
> >> > > solve on amdgpu thats with or without your patch set and the above
> >> > > "trigger" too
> >
> > Okay the reason you're not seeing deadlocks is because the output poll
> > worker is not enabled.  And the output poll worker is not enabled
> > because your discrete GPU doesn't have any outputs:
> >
> > [0.265568] [drm:dc_create] *ERROR* DC: Number of connectors is zero!
> >
> > The outputs are only polled if there are connectors which have the
> > DRM_CONNECTOR_POLL_CONNECT or DRM_CONNECTOR_POLL_DISCONNECT flag set.
> > And that only ever seems to be the case for VGA and DVI.
> >
> > We know based on bugzilla reports that hybrid graphics laptops do exist
> > which poll outputs with radeon and nouveau.  If there are no laptops
> > supported by amdgpu whose discrete GPU has polled connectors, then
> > patch [5/5] would be unnecessary.  That is for Alex to decide.
> 
> Most hybrid laptops don't have display connectors on the dGPU and we
> only use polling on analog connectors, so you are not likely to run
> into this on recent laptops.  That said, I don't know if there is some
> OEM system out there with a VGA port on the dGPU in a hybrid laptop.
> I guess another option is to just disable polling on hybrid laptops.

If we don't know for sure, applying patch [5/5] would seem to be the
safest approach.  (Assuming it doesn't break anything else.)

Right now runtime PM is only used on hybrid graphics dGPUs by nouveau,
radeon and amdgpu.  Would it be conceivable that its use is expanded
beyond that in the future?  E.g. on a desktop machine, if DPMS is off
on all screens, why keep the GPU in D0?  If that is conceivable, chances
that analog connectors are present are higher, and then the patch would
be necessary again.  (Of course this would mean that analog screens
wouldn't light up automatically if they're attached while the GPU is
in D3hot, but the user may forbid runtime PM via sysfs if that is
unwanted.)

Thanks,

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

Re: [Intel-gfx] [PATCH 2/5] drm: Allow determining if current task is output poll worker

[Lukas Wunner]

On Mon, Feb 12, 2018 at 05:50:12PM +, Chris Wilson wrote:
> Quoting Lyude Paul (2018-02-12 17:46:11)
> > On Sun, 2018-02-11 at 10:38 +0100, Lukas Wunner wrote:
> > > Introduce a helper to determine if the current task is an output poll
> > > worker.
> > > 
> > > This allows us to fix a long-standing deadlock in several DRM drivers
> > > wherein the ->runtime_suspend callback waits for the output poll worker
> > > to finish and the worker in turn calls a ->detect callback which waits
> > > for runtime suspend to finish.  The ->detect callback is invoked from
> > > multiple call sites and waiting for runtime suspend to finish is the
> > > correct thing to do except if it's executing in the context of the
> > > worker.
> > > 
> > > Cc: Dave Airlie <airl...@redhat.com>
> > > Cc: Ben Skeggs <bske...@redhat.com>
> > > Cc: Alex Deucher <alexander.deuc...@amd.com>
> > > Signed-off-by: Lukas Wunner <lu...@wunner.de>
> > > ---
> > >  drivers/gpu/drm/drm_probe_helper.c | 14 ++
> > >  include/drm/drm_crtc_helper.h  |  1 +
> > >  2 files changed, 15 insertions(+)
> > > 
> > > diff --git a/drivers/gpu/drm/drm_probe_helper.c
> > > b/drivers/gpu/drm/drm_probe_helper.c
> > > index 555fbe54d6e2..019881d15ce1 100644
> > > --- a/drivers/gpu/drm/drm_probe_helper.c
> > > +++ b/drivers/gpu/drm/drm_probe_helper.c
> > > @@ -653,6 +653,20 @@ static void output_poll_execute(struct work_struct
> > > *work)
> > >   schedule_delayed_work(delayed_work,
> > > DRM_OUTPUT_POLL_PERIOD);
> > >  }
> > >  
> > > +/**
> > > + * drm_kms_helper_is_poll_worker - is %current task an output poll 
> > > worker?
> > > + *
> > > + * Determine if %current task is an output poll worker.  This can be used
> > > + * to select distinct code paths for output polling versus other 
> > > contexts.
> > > + */
> > > +bool drm_kms_helper_is_poll_worker(void)
> > > +{
> > > + struct work_struct *work = current_work();
> > > +
> > > + return work && work->func == output_poll_execute;
> 
> What ensures that work is accessible? Does this need rcu_read_lock
> protection or more?

The work_struct exists as long this kthread is working on it.
Since this is called by the kthread itself, it is guaranteed to exist.
So there's no protection needed.

Thanks,

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

Re: [Intel-gfx] [PATCH 0/5] Fix deadlock on runtime suspend in DRM drivers

[Lukas Wunner]

On Mon, Feb 12, 2018 at 09:03:26AM +, Mike Lothian wrote:
> On 12 February 2018 at 03:39, Lukas Wunner <lu...@wunner.de> wrote:
> > On Mon, Feb 12, 2018 at 12:35:51AM +, Mike Lothian wrote:
> > > I've not been able to reproduce the original problem you're trying to
> > > solve on amdgpu thats with or without your patch set and the above
> > > "trigger" too
> > >
> > > Is anything else required to trigger it, I started multiple DRI_PRIME
> > > glxgears, in parallel, serial waiting the 12 seconds and serial within
> > > the 12 seconds and I couldn't reproduce it
> >
> > The discrete GPU needs to runtime suspend, that's the trigger,
> > so no DRI_PRIME executables should be running.  Just let it
> > autosuspend after boot.  Do you see "waiting 12 sec" messages
> > in dmesg?  If not it's not autosuspending.
> 
> Yes I'm seeing those messages, I'm just not seeing the hangs
> 
> I've attached the dmesg in case you're interested

Okay the reason you're not seeing deadlocks is because the output poll
worker is not enabled.  And the output poll worker is not enabled
because your discrete GPU doesn't have any outputs:

[0.265568] [drm:dc_create] *ERROR* DC: Number of connectors is zero!

The outputs are only polled if there are connectors which have the
DRM_CONNECTOR_POLL_CONNECT or DRM_CONNECTOR_POLL_DISCONNECT flag set.
And that only ever seems to be the case for VGA and DVI.

We know based on bugzilla reports that hybrid graphics laptops do exist
which poll outputs with radeon and nouveau.  If there are no laptops
supported by amdgpu whose discrete GPU has polled connectors, then
patch [5/5] would be unnecessary.  That is for Alex to decide.

However that is very good to know, so thanks a lot for your testing
efforts, much appreciated!

Kind regards,

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

Re: [Intel-gfx] [PATCH 0/5] Fix deadlock on runtime suspend in DRM drivers

[Lukas Wunner]

On Mon, Feb 12, 2018 at 12:35:51AM +, Mike Lothian wrote:
> I've not been able to reproduce the original problem you're trying to
> solve on amdgpu thats with or without your patch set and the above
> "trigger" too
> 
> Is anything else required to trigger it, I started multiple DRI_PRIME
> glxgears, in parallel, serial waiting the 12 seconds and serial within
> the 12 seconds and I couldn't reproduce it

The discrete GPU needs to runtime suspend, that's the trigger,
so no DRI_PRIME executables should be running.  Just let it
autosuspend after boot.  Do you see "waiting 12 sec" messages
in dmesg?  If not it's not autosuspending.

Thanks,

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

Re: [Intel-gfx] [PATCH 0/5] Fix deadlock on runtime suspend in DRM drivers

[Lukas Wunner]

On Sun, Feb 11, 2018 at 08:23:14PM +0100, Lukas Wunner wrote:
> On Sun, Feb 11, 2018 at 06:58:11PM +, Mike Lothian wrote:
> > On 11 February 2018 at 09:38, Lukas Wunner <lu...@wunner.de> wrote:
> > > The patches for radeon and amdgpu are compile-tested only, I only have a
> > > MacBook Pro with an Nvidia GK107 to test.  To test the patches, add an
> > > "msleep(12*1000);" at the top of the driver's ->runtime_suspend hook.
> > > This ensures that the poll worker runs after ->runtime_suspend has begun.
> > > Wait 12 sec after the GPU has begun runtime suspend, then check
> > > /sys/bus/pci/devices/:01:00.0/power/runtime_status.  Without this
> > > series, the status will be stuck at "suspending" and you'll get hung task
> > > errors in dmesg after a few minutes.
> > 
> > I wasn't quite sure where to add that msleep. I've tested the patches
> > as is on top of agd5f's wip branch without ill effects
> > 
> > I've had a radeon and now a amdgpu PRIME setup and don't believe I've
> > ever seen this issue
> > 
> > If you could pop a patch together for the msleep I'll give it a test on
> > amdgpu
> 
> Here you go, this is for all 3 drivers.
> Should deadlock without the series.
> Thanks!

Sorry, I missed that amdgpu_drv.c and radeon_drv.c don't include delay.h,
rectified testing patch below:


diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c 
b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
index 50afcf6..beaaf2c 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
@@ -36,6 +36,7 @@
 
 #include 
 #include 
+#include 
 #include 
 #include 
 #include 
@@ -718,6 +719,9 @@ static int amdgpu_pmops_runtime_suspend(struct device *dev)
return -EBUSY;
}
 
+   printk("waiting 12 sec\n");
+   msleep(12*1000);
+   printk("done waiting 12 sec\n");
drm_dev->switch_power_state = DRM_SWITCH_POWER_CHANGING;
drm_kms_helper_poll_disable(drm_dev);
vga_switcheroo_set_dynamic_switch(pdev, VGA_SWITCHEROO_OFF);
diff --git a/drivers/gpu/drm/drm_probe_helper.c 
b/drivers/gpu/drm/drm_probe_helper.c
index 555fbe5..ee7cf0d 100644
--- a/drivers/gpu/drm/drm_probe_helper.c
+++ b/drivers/gpu/drm/drm_probe_helper.c
@@ -586,6 +586,7 @@ static void output_poll_execute(struct work_struct *work)
repoll = true;
goto out;
}
+   dev_info(>pdev->dev, "begin poll\n");
 
drm_connector_list_iter_begin(dev, _iter);
drm_for_each_connector_iter(connector, _iter) {
@@ -651,6 +652,7 @@ static void output_poll_execute(struct work_struct *work)
 
if (repoll)
schedule_delayed_work(delayed_work, DRM_OUTPUT_POLL_PERIOD);
+   dev_info(>pdev->dev, "end poll\n");
 }
 
 /**
diff --git a/drivers/gpu/drm/nouveau/nouveau_drm.c 
b/drivers/gpu/drm/nouveau/nouveau_drm.c
index 3e29302..f9da5bc 100644
--- a/drivers/gpu/drm/nouveau/nouveau_drm.c
+++ b/drivers/gpu/drm/nouveau/nouveau_drm.c
@@ -855,6 +855,9 @@ static int nouveau_drm_probe(struct pci_dev *pdev,
return -EBUSY;
}
 
+   printk("waiting 12 sec\n");
+   msleep(12*1000);
+   printk("done waiting 12 sec\n");
drm_kms_helper_poll_disable(drm_dev);
vga_switcheroo_set_dynamic_switch(pdev, VGA_SWITCHEROO_OFF);
nouveau_switcheroo_optimus_dsm();
diff --git a/drivers/gpu/drm/radeon/radeon_drv.c 
b/drivers/gpu/drm/radeon/radeon_drv.c
index 31dd04f..2b4e7e0 100644
--- a/drivers/gpu/drm/radeon/radeon_drv.c
+++ b/drivers/gpu/drm/radeon/radeon_drv.c
@@ -35,6 +35,7 @@
 
 #include 
 #include 
+#include 
 #include 
 #include 
 #include 
@@ -413,6 +414,9 @@ static int radeon_pmops_runtime_suspend(struct device *dev)
return -EBUSY;
}
 
+   printk("waiting 12 sec\n");
+   msleep(12*1000);
+   printk("done waiting 12 sec\n");
drm_dev->switch_power_state = DRM_SWITCH_POWER_CHANGING;
drm_kms_helper_poll_disable(drm_dev);
vga_switcheroo_set_dynamic_switch(pdev, VGA_SWITCHEROO_OFF);
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 0/5] Fix deadlock on runtime suspend in DRM drivers

[Lukas Wunner]

On Sun, Feb 11, 2018 at 06:58:11PM +, Mike Lothian wrote:
> On 11 February 2018 at 09:38, Lukas Wunner <lu...@wunner.de> wrote:
> > The patches for radeon and amdgpu are compile-tested only, I only have a
> > MacBook Pro with an Nvidia GK107 to test.  To test the patches, add an
> > "msleep(12*1000);" at the top of the driver's ->runtime_suspend hook.
> > This ensures that the poll worker runs after ->runtime_suspend has begun.
> > Wait 12 sec after the GPU has begun runtime suspend, then check
> > /sys/bus/pci/devices/:01:00.0/power/runtime_status.  Without this
> > series, the status will be stuck at "suspending" and you'll get hung task
> > errors in dmesg after a few minutes.
> 
> I wasn't quite sure where to add that msleep. I've tested the patches
> as is on top of agd5f's wip branch without ill effects
> 
> I've had a radeon and now a amdgpu PRIME setup and don't believe I've
> ever seen this issue
> 
> If you could pop a patch together for the msleep I'll give it a test on
> amdgpu

Here you go, this is for all 3 drivers.
Should deadlock without the series.
Thanks!


diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c 
b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
index 50afcf6..eefa0d4 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
@@ -718,6 +718,9 @@ static int amdgpu_pmops_runtime_suspend(struct device *dev)
return -EBUSY;
}
 
+   printk("waiting 12 sec\n");
+   msleep(12*1000);
+   printk("done waiting 12 sec\n");
drm_dev->switch_power_state = DRM_SWITCH_POWER_CHANGING;
drm_kms_helper_poll_disable(drm_dev);
vga_switcheroo_set_dynamic_switch(pdev, VGA_SWITCHEROO_OFF);
diff --git a/drivers/gpu/drm/drm_probe_helper.c 
b/drivers/gpu/drm/drm_probe_helper.c
index 555fbe5..ee7cf0d 100644
--- a/drivers/gpu/drm/drm_probe_helper.c
+++ b/drivers/gpu/drm/drm_probe_helper.c
@@ -586,6 +586,7 @@ static void output_poll_execute(struct work_struct *work)
repoll = true;
goto out;
}
+   dev_info(>pdev->dev, "begin poll\n");
 
drm_connector_list_iter_begin(dev, _iter);
drm_for_each_connector_iter(connector, _iter) {
@@ -651,6 +652,7 @@ static void output_poll_execute(struct work_struct *work)
 
if (repoll)
schedule_delayed_work(delayed_work, DRM_OUTPUT_POLL_PERIOD);
+   dev_info(>pdev->dev, "end poll\n");
 }
 
 /**
diff --git a/drivers/gpu/drm/nouveau/nouveau_drm.c 
b/drivers/gpu/drm/nouveau/nouveau_drm.c
index 3e29302..f9da5bc 100644
--- a/drivers/gpu/drm/nouveau/nouveau_drm.c
+++ b/drivers/gpu/drm/nouveau/nouveau_drm.c
@@ -855,6 +855,9 @@ static int nouveau_drm_probe(struct pci_dev *pdev,
return -EBUSY;
}
 
+   printk("waiting 12 sec\n");
+   msleep(12*1000);
+   printk("done waiting 12 sec\n");
drm_kms_helper_poll_disable(drm_dev);
vga_switcheroo_set_dynamic_switch(pdev, VGA_SWITCHEROO_OFF);
nouveau_switcheroo_optimus_dsm();
diff --git a/drivers/gpu/drm/radeon/radeon_drv.c 
b/drivers/gpu/drm/radeon/radeon_drv.c
index 31dd04f..707b8aa 100644
--- a/drivers/gpu/drm/radeon/radeon_drv.c
+++ b/drivers/gpu/drm/radeon/radeon_drv.c
@@ -413,6 +413,9 @@ static int radeon_pmops_runtime_suspend(struct device *dev)
return -EBUSY;
}
 
+   printk("waiting 12 sec\n");
+   msleep(12*1000);
+   printk("done waiting 12 sec\n");
drm_dev->switch_power_state = DRM_SWITCH_POWER_CHANGING;
drm_kms_helper_poll_disable(drm_dev);
vga_switcheroo_set_dynamic_switch(pdev, VGA_SWITCHEROO_OFF);
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Intel-gfx] [PATCH 0/5] Fix deadlock on runtime suspend in DRM drivers

[Lukas Wunner]

Fix a deadlock on hybrid graphics laptops that's been present since 2013:

DRM drivers poll connectors in 10 sec intervals.  The poll worker is
stopped on ->runtime_suspend with cancel_delayed_work_sync().  However
the poll worker invokes the DRM drivers' ->detect callbacks, which call
pm_runtime_get_sync().  If the poll worker starts after runtime suspend
has begun, pm_runtime_get_sync() will wait for runtime suspend to finish
with the intention of runtime resuming the device afterwards.  The result
is a circular wait between poll worker and autosuspend worker.

I'm seeing this deadlock so often it's not funny anymore. I've also found
3 nouveau bugzillas about it and 1 radeon bugzilla. (See patch [3/5] and
[4/5].)

One theoretical solution would be to add a flag to the ->detect callback
to tell it that it's running in the poll worker's context.  In that case
it doesn't need to call pm_runtime_get_sync() because the poll worker is
only enabled while runtime active and we know that ->runtime_suspend
waits for it to finish.  But this approach would require touching every
single DRM driver's ->detect hook.  Moreover the ->detect hook is called
from numerous other places, both in the DRM library as well as in each
driver, making it necessary to trace every possible call chain and check
if it's coming from the poll worker or not.  I've tried to do that for
nouveau (see the call sites listed in the commit message of patch [3/5])
and concluded that this approach is a nightmare to implement.

Another reason for the infeasibility of this approach is that ->detect
is called from within the DRM library without driver involvement, e.g.
via DRM's sysfs interface.  In those cases, pm_runtime_get_sync() would
have to be called by the DRM library, but the DRM library is supposed to
stay generic, wheras runtime PM is driver-specific.

So instead, I've come up with this much simpler solution which gleans
from the current task's flags if it's a workqueue worker, retrieves the
work_struct and checks if it's the poll worker.  All that's needed is
one small helper in the workqueue code and another small helper in the
DRM library.  This solution lends itself nicely to stable-backporting.

The patches for radeon and amdgpu are compile-tested only, I only have a
MacBook Pro with an Nvidia GK107 to test.  To test the patches, add an
"msleep(12*1000);" at the top of the driver's ->runtime_suspend hook.
This ensures that the poll worker runs after ->runtime_suspend has begun.
Wait 12 sec after the GPU has begun runtime suspend, then check
/sys/bus/pci/devices/:01:00.0/power/runtime_status.  Without this
series, the status will be stuck at "suspending" and you'll get hung task
errors in dmesg after a few minutes.

i915, malidp and msm "solved" this issue by not stopping the poll worker
on runtime suspend.  But this results in the GPU bouncing back and forth
between D0 and D3 continuously.  That's a total no-go for GPUs which
runtime suspend to D3cold since every suspend/resume cycle costs a
significant amount of time and energy.  (i915 and malidp do not seem
to acquire a runtime PM ref in the ->detect callbacks, which seems
questionable.  msm however does and would also deadlock if it disabled
the poll worker on runtime suspend.  cc += Archit, Liviu, intel-gfx)

Please review.  Thanks,

Lukas

Lukas Wunner (5):
  workqueue: Allow retrieval of current task's work struct
  drm: Allow determining if current task is output poll worker
  drm/nouveau: Fix deadlock on runtime suspend
  drm/radeon: Fix deadlock on runtime suspend
  drm/amdgpu: Fix deadlock on runtime suspend

 drivers/gpu/drm/amd/amdgpu/amdgpu_connectors.c | 58 +---
 drivers/gpu/drm/drm_probe_helper.c | 14 +
 drivers/gpu/drm/nouveau/nouveau_connector.c| 18 +--
 drivers/gpu/drm/radeon/radeon_connectors.c | 74 +-
 include/drm/drm_crtc_helper.h  |  1 +
 include/linux/workqueue.h  |  1 +
 kernel/workqueue.c | 16 ++
 7 files changed, 132 insertions(+), 50 deletions(-)

-- 
2.15.1

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

[Intel-gfx] [PATCH 5/5] drm/amdgpu: Fix deadlock on runtime suspend

[Lukas Wunner]

amdgpu's ->runtime_suspend hook calls drm_kms_helper_poll_disable(),
which waits for the output poll worker to finish if it's running.

The output poll worker meanwhile calls pm_runtime_get_sync() in
amdgpu's ->detect hooks, which waits for the ongoing suspend to finish,
causing a deadlock.

Fix by not acquiring a runtime PM ref if the ->detect hooks are called
in the output poll worker's context.  This is safe because the poll
worker is only enabled while runtime active and we know that
->runtime_suspend waits for it to finish.

Fixes: d38ceaf99ed0 ("drm/amdgpu: add core driver (v4)")
Cc: sta...@vger.kernel.org # v4.2+: 1234567890ab: workqueue: Allow retrieval of 
current task's work struct
Cc: sta...@vger.kernel.org # v4.2+: 1234567890ab: drm: Allow determining if 
current task is output poll worker
Cc: Alex Deucher <alexander.deuc...@amd.com>
Signed-off-by: Lukas Wunner <lu...@wunner.de>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_connectors.c | 58 +-
 1 file changed, 38 insertions(+), 20 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_connectors.c 
b/drivers/gpu/drm/amd/amdgpu/amdgpu_connectors.c
index 8ca3783f2deb..74d2efaec52f 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_connectors.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_connectors.c
@@ -736,9 +736,11 @@ amdgpu_connector_lvds_detect(struct drm_connector 
*connector, bool force)
enum drm_connector_status ret = connector_status_disconnected;
int r;
 
-   r = pm_runtime_get_sync(connector->dev->dev);
-   if (r < 0)
-   return connector_status_disconnected;
+   if (!drm_kms_helper_is_poll_worker()) {
+   r = pm_runtime_get_sync(connector->dev->dev);
+   if (r < 0)
+   return connector_status_disconnected;
+   }
 
if (encoder) {
struct amdgpu_encoder *amdgpu_encoder = 
to_amdgpu_encoder(encoder);
@@ -757,8 +759,12 @@ amdgpu_connector_lvds_detect(struct drm_connector 
*connector, bool force)
/* check acpi lid status ??? */
 
amdgpu_connector_update_scratch_regs(connector, ret);
-   pm_runtime_mark_last_busy(connector->dev->dev);
-   pm_runtime_put_autosuspend(connector->dev->dev);
+
+   if (!drm_kms_helper_is_poll_worker()) {
+   pm_runtime_mark_last_busy(connector->dev->dev);
+   pm_runtime_put_autosuspend(connector->dev->dev);
+   }
+
return ret;
 }
 
@@ -868,9 +874,11 @@ amdgpu_connector_vga_detect(struct drm_connector 
*connector, bool force)
enum drm_connector_status ret = connector_status_disconnected;
int r;
 
-   r = pm_runtime_get_sync(connector->dev->dev);
-   if (r < 0)
-   return connector_status_disconnected;
+   if (!drm_kms_helper_is_poll_worker()) {
+   r = pm_runtime_get_sync(connector->dev->dev);
+   if (r < 0)
+   return connector_status_disconnected;
+   }
 
encoder = amdgpu_connector_best_single_encoder(connector);
if (!encoder)
@@ -924,8 +932,10 @@ amdgpu_connector_vga_detect(struct drm_connector 
*connector, bool force)
amdgpu_connector_update_scratch_regs(connector, ret);
 
 out:
-   pm_runtime_mark_last_busy(connector->dev->dev);
-   pm_runtime_put_autosuspend(connector->dev->dev);
+   if (!drm_kms_helper_is_poll_worker()) {
+   pm_runtime_mark_last_busy(connector->dev->dev);
+   pm_runtime_put_autosuspend(connector->dev->dev);
+   }
 
return ret;
 }
@@ -988,9 +998,11 @@ amdgpu_connector_dvi_detect(struct drm_connector 
*connector, bool force)
enum drm_connector_status ret = connector_status_disconnected;
bool dret = false, broken_edid = false;
 
-   r = pm_runtime_get_sync(connector->dev->dev);
-   if (r < 0)
-   return connector_status_disconnected;
+   if (!drm_kms_helper_is_poll_worker()) {
+   r = pm_runtime_get_sync(connector->dev->dev);
+   if (r < 0)
+   return connector_status_disconnected;
+   }
 
if (!force && amdgpu_connector_check_hpd_status_unchanged(connector)) {
ret = connector->status;
@@ -1115,8 +1127,10 @@ amdgpu_connector_dvi_detect(struct drm_connector 
*connector, bool force)
amdgpu_connector_update_scratch_regs(connector, ret);
 
 exit:
-   pm_runtime_mark_last_busy(connector->dev->dev);
-   pm_runtime_put_autosuspend(connector->dev->dev);
+   if (!drm_kms_helper_is_poll_worker()) {
+   pm_runtime_mark_last_busy(connector->dev->dev);
+   pm_runtime_put_autosuspend(connector->dev->dev);
+   }
 
return ret;
 }
@@ -1359,9 +1373,11 @@ amdgpu_connector_dp_detect(struct drm_connector 
*connector, bool force)
 

[Intel-gfx] [PATCH 2/5] drm: Allow determining if current task is output poll worker

[Lukas Wunner]

Introduce a helper to determine if the current task is an output poll
worker.

This allows us to fix a long-standing deadlock in several DRM drivers
wherein the ->runtime_suspend callback waits for the output poll worker
to finish and the worker in turn calls a ->detect callback which waits
for runtime suspend to finish.  The ->detect callback is invoked from
multiple call sites and waiting for runtime suspend to finish is the
correct thing to do except if it's executing in the context of the
worker.

Cc: Dave Airlie <airl...@redhat.com>
Cc: Ben Skeggs <bske...@redhat.com>
Cc: Alex Deucher <alexander.deuc...@amd.com>
Signed-off-by: Lukas Wunner <lu...@wunner.de>
---
 drivers/gpu/drm/drm_probe_helper.c | 14 ++
 include/drm/drm_crtc_helper.h  |  1 +
 2 files changed, 15 insertions(+)

diff --git a/drivers/gpu/drm/drm_probe_helper.c 
b/drivers/gpu/drm/drm_probe_helper.c
index 555fbe54d6e2..019881d15ce1 100644
--- a/drivers/gpu/drm/drm_probe_helper.c
+++ b/drivers/gpu/drm/drm_probe_helper.c
@@ -653,6 +653,20 @@ static void output_poll_execute(struct work_struct *work)
schedule_delayed_work(delayed_work, DRM_OUTPUT_POLL_PERIOD);
 }
 
+/**
+ * drm_kms_helper_is_poll_worker - is %current task an output poll worker?
+ *
+ * Determine if %current task is an output poll worker.  This can be used
+ * to select distinct code paths for output polling versus other contexts.
+ */
+bool drm_kms_helper_is_poll_worker(void)
+{
+   struct work_struct *work = current_work();
+
+   return work && work->func == output_poll_execute;
+}
+EXPORT_SYMBOL(drm_kms_helper_is_poll_worker);
+
 /**
  * drm_kms_helper_poll_disable - disable output polling
  * @dev: drm_device
diff --git a/include/drm/drm_crtc_helper.h b/include/drm/drm_crtc_helper.h
index 76e237bd989b..6914633037a5 100644
--- a/include/drm/drm_crtc_helper.h
+++ b/include/drm/drm_crtc_helper.h
@@ -77,5 +77,6 @@ void drm_kms_helper_hotplug_event(struct drm_device *dev);
 
 void drm_kms_helper_poll_disable(struct drm_device *dev);
 void drm_kms_helper_poll_enable(struct drm_device *dev);
+bool drm_kms_helper_is_poll_worker(void);
 
 #endif
-- 
2.15.1

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

[Intel-gfx] [PATCH 4/5] drm/radeon: Fix deadlock on runtime suspend

[Lukas Wunner]

radeon's ->runtime_suspend hook calls drm_kms_helper_poll_disable(),
which waits for the output poll worker to finish if it's running.

The output poll worker meanwhile calls pm_runtime_get_sync() in
radeon's ->detect hooks, which waits for the ongoing suspend to finish,
causing a deadlock.

Fix by not acquiring a runtime PM ref if the ->detect hooks are called
in the output poll worker's context.  This is safe because the poll
worker is only enabled while runtime active and we know that
->runtime_suspend waits for it to finish.

Stack trace for posterity:

  INFO: task kworker/0:3:31847 blocked for more than 120 seconds
  Workqueue: events output_poll_execute [drm_kms_helper]
  Call Trace:
   schedule+0x3c/0x90
   rpm_resume+0x1e2/0x690
   __pm_runtime_resume+0x3f/0x60
   radeon_lvds_detect+0x39/0xf0 [radeon]
   output_poll_execute+0xda/0x1e0 [drm_kms_helper]
   process_one_work+0x14b/0x440
   worker_thread+0x48/0x4a0

  INFO: task kworker/2:0:10493 blocked for more than 120 seconds.
  Workqueue: pm pm_runtime_work
  Call Trace:
   schedule+0x3c/0x90
   schedule_timeout+0x1b3/0x240
   wait_for_common+0xc2/0x180
   wait_for_completion+0x1d/0x20
   flush_work+0xfc/0x1a0
   __cancel_work_timer+0xa5/0x1d0
   cancel_delayed_work_sync+0x13/0x20
   drm_kms_helper_poll_disable+0x1f/0x30 [drm_kms_helper]
   radeon_pmops_runtime_suspend+0x3d/0xa0 [radeon]
   pci_pm_runtime_suspend+0x61/0x1a0
   vga_switcheroo_runtime_suspend+0x21/0x70
   __rpm_callback+0x32/0x70
   rpm_callback+0x24/0x80
   rpm_suspend+0x12b/0x640
   pm_runtime_work+0x6f/0xb0
   process_one_work+0x14b/0x440
   worker_thread+0x48/0x4a0

Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=94147
Fixes: 10ebc0bc0934 ("drm/radeon: add runtime PM support (v2)")
Cc: sta...@vger.kernel.org # v3.13+: 1234567890ab: workqueue: Allow retrieval 
of current task's work struct
Cc: sta...@vger.kernel.org # v3.13+: 1234567890ab: drm: Allow determining if 
current task is output poll worker
Cc: Ismo Toijala <ismo.toij...@gmail.com>
Cc: Alex Deucher <alexander.deuc...@amd.com>
Cc: Dave Airlie <airl...@redhat.com>
Signed-off-by: Lukas Wunner <lu...@wunner.de>
---
 drivers/gpu/drm/radeon/radeon_connectors.c | 74 --
 1 file changed, 49 insertions(+), 25 deletions(-)

diff --git a/drivers/gpu/drm/radeon/radeon_connectors.c 
b/drivers/gpu/drm/radeon/radeon_connectors.c
index 5012f5e47a1e..2e2ca3c6b47d 100644
--- a/drivers/gpu/drm/radeon/radeon_connectors.c
+++ b/drivers/gpu/drm/radeon/radeon_connectors.c
@@ -899,9 +899,11 @@ radeon_lvds_detect(struct drm_connector *connector, bool 
force)
enum drm_connector_status ret = connector_status_disconnected;
int r;
 
-   r = pm_runtime_get_sync(connector->dev->dev);
-   if (r < 0)
-   return connector_status_disconnected;
+   if (!drm_kms_helper_is_poll_worker()) {
+   r = pm_runtime_get_sync(connector->dev->dev);
+   if (r < 0)
+   return connector_status_disconnected;
+   }
 
if (encoder) {
struct radeon_encoder *radeon_encoder = 
to_radeon_encoder(encoder);
@@ -924,8 +926,12 @@ radeon_lvds_detect(struct drm_connector *connector, bool 
force)
/* check acpi lid status ??? */
 
radeon_connector_update_scratch_regs(connector, ret);
-   pm_runtime_mark_last_busy(connector->dev->dev);
-   pm_runtime_put_autosuspend(connector->dev->dev);
+
+   if (!drm_kms_helper_is_poll_worker()) {
+   pm_runtime_mark_last_busy(connector->dev->dev);
+   pm_runtime_put_autosuspend(connector->dev->dev);
+   }
+
return ret;
 }
 
@@ -1039,9 +1045,11 @@ radeon_vga_detect(struct drm_connector *connector, bool 
force)
enum drm_connector_status ret = connector_status_disconnected;
int r;
 
-   r = pm_runtime_get_sync(connector->dev->dev);
-   if (r < 0)
-   return connector_status_disconnected;
+   if (!drm_kms_helper_is_poll_worker()) {
+   r = pm_runtime_get_sync(connector->dev->dev);
+   if (r < 0)
+   return connector_status_disconnected;
+   }
 
encoder = radeon_best_single_encoder(connector);
if (!encoder)
@@ -1108,8 +1116,10 @@ radeon_vga_detect(struct drm_connector *connector, bool 
force)
radeon_connector_update_scratch_regs(connector, ret);
 
 out:
-   pm_runtime_mark_last_busy(connector->dev->dev);
-   pm_runtime_put_autosuspend(connector->dev->dev);
+   if (!drm_kms_helper_is_poll_worker()) {
+   pm_runtime_mark_last_busy(connector->dev->dev);
+   pm_runtime_put_autosuspend(connector->dev->dev);
+   }
 
return ret;
 }
@@ -1173,9 +1183,11 @@ radeon_tv_detect(struct drm_connector *connector, bool 
force)
if (!radeon_connector->dac_load_

[Intel-gfx] [PATCH 1/5] workqueue: Allow retrieval of current task's work struct

[Lukas Wunner]

Introduce a helper to retrieve the current task's work struct if it is
a workqueue worker.

This allows us to fix a long-standing deadlock in several DRM drivers
wherein the ->runtime_suspend callback waits for a specific worker to
finish and that worker in turn calls a function which waits for runtime
suspend to finish.  That function is invoked from multiple call sites
and waiting for runtime suspend to finish is the correct thing to do
except if it's executing in the context of the worker.

Cc: Tejun Heo <t...@kernel.org>
Cc: Lai Jiangshan <jiangshan...@gmail.com>
Cc: Dave Airlie <airl...@redhat.com>
Cc: Ben Skeggs <bske...@redhat.com>
Cc: Alex Deucher <alexander.deuc...@amd.com>
Signed-off-by: Lukas Wunner <lu...@wunner.de>
---
 include/linux/workqueue.h |  1 +
 kernel/workqueue.c| 16 
 2 files changed, 17 insertions(+)

diff --git a/include/linux/workqueue.h b/include/linux/workqueue.h
index 4a54ef96aff5..bc0cda180c8b 100644
--- a/include/linux/workqueue.h
+++ b/include/linux/workqueue.h
@@ -465,6 +465,7 @@ extern bool cancel_delayed_work_sync(struct delayed_work 
*dwork);
 
 extern void workqueue_set_max_active(struct workqueue_struct *wq,
 int max_active);
+extern struct work_struct *current_work(void);
 extern bool current_is_workqueue_rescuer(void);
 extern bool workqueue_congested(int cpu, struct workqueue_struct *wq);
 extern unsigned int work_busy(struct work_struct *work);
diff --git a/kernel/workqueue.c b/kernel/workqueue.c
index 017044c26233..bb9a519cbf50 100644
--- a/kernel/workqueue.c
+++ b/kernel/workqueue.c
@@ -4179,6 +4179,22 @@ void workqueue_set_max_active(struct workqueue_struct 
*wq, int max_active)
 }
 EXPORT_SYMBOL_GPL(workqueue_set_max_active);
 
+/**
+ * current_work - retrieve %current task's work struct
+ *
+ * Determine if %current task is a workqueue worker and what it's working on.
+ * Useful to find out the context that the %current task is running in.
+ *
+ * Return: work struct if %current task is a workqueue worker, %NULL otherwise.
+ */
+struct work_struct *current_work(void)
+{
+   struct worker *worker = current_wq_worker();
+
+   return worker ? worker->current_work : NULL;
+}
+EXPORT_SYMBOL(current_work);
+
 /**
  * current_is_workqueue_rescuer - is %current workqueue rescuer?
  *
-- 
2.15.1

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

[Intel-gfx] [PATCH 3/5] drm/nouveau: Fix deadlock on runtime suspend

[Lukas Wunner]

nouveau's ->runtime_suspend hook calls drm_kms_helper_poll_disable(),
which waits for the output poll worker to finish if it's running.

The output poll worker meanwhile calls pm_runtime_get_sync() in
nouveau_connector_detect() which waits for the ongoing suspend to finish,
causing a deadlock.

Fix by not acquiring a runtime PM ref if nouveau_connector_detect() is
called in the output poll worker's context.  This is safe because
the poll worker is only enabled while runtime active and we know that
->runtime_suspend waits for it to finish.

Other contexts calling nouveau_connector_detect() do require a runtime
PM ref, these comprise:

  status_store() drm sysfs interface
  ->fill_modes drm callback
  drm_fb_helper_probe_connector_modes()
  drm_mode_getconnector()
  nouveau_connector_hotplug()
  nouveau_display_hpd_work()
  nv17_tv_set_property()

Stack trace for posterity:

  INFO: task kworker/0:1:58 blocked for more than 120 seconds.
  Workqueue: events output_poll_execute [drm_kms_helper]
  Call Trace:
   schedule+0x28/0x80
   rpm_resume+0x107/0x6e0
   __pm_runtime_resume+0x47/0x70
   nouveau_connector_detect+0x7e/0x4a0 [nouveau]
   nouveau_connector_detect_lvds+0x132/0x180 [nouveau]
   drm_helper_probe_detect_ctx+0x85/0xd0 [drm_kms_helper]
   output_poll_execute+0x11e/0x1c0 [drm_kms_helper]
   process_one_work+0x184/0x380
   worker_thread+0x2e/0x390

  INFO: task kworker/0:2:252 blocked for more than 120 seconds.
  Workqueue: pm pm_runtime_work
  Call Trace:
   schedule+0x28/0x80
   schedule_timeout+0x1e3/0x370
   wait_for_completion+0x123/0x190
   flush_work+0x142/0x1c0
   nouveau_pmops_runtime_suspend+0x7e/0xd0 [nouveau]
   pci_pm_runtime_suspend+0x5c/0x180
   vga_switcheroo_runtime_suspend+0x1e/0xa0
   __rpm_callback+0xc1/0x200
   rpm_callback+0x1f/0x70
   rpm_suspend+0x13c/0x640
   pm_runtime_work+0x6e/0x90
   process_one_work+0x184/0x380
   worker_thread+0x2e/0x390

Bugzilla: https://bugs.archlinux.org/task/53497
Bugzilla: https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=870523
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=70388#c33
Fixes: 5addcf0a5f0f ("nouveau: add runtime PM support (v0.9)")
Cc: sta...@vger.kernel.org # v3.12+: 1234567890ab: workqueue: Allow retrieval 
of current task's work struct
Cc: sta...@vger.kernel.org # v3.12+: 1234567890ab: drm: Allow determining if 
current task is output poll worker
Cc: Ben Skeggs <bske...@redhat.com>
Cc: Dave Airlie <airl...@redhat.com>
Signed-off-by: Lukas Wunner <lu...@wunner.de>
---
 drivers/gpu/drm/nouveau/nouveau_connector.c | 18 +-
 1 file changed, 13 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/nouveau/nouveau_connector.c 
b/drivers/gpu/drm/nouveau/nouveau_connector.c
index 69d6e61a01ec..6ed9cb053dfa 100644
--- a/drivers/gpu/drm/nouveau/nouveau_connector.c
+++ b/drivers/gpu/drm/nouveau/nouveau_connector.c
@@ -570,9 +570,15 @@ nouveau_connector_detect(struct drm_connector *connector, 
bool force)
nv_connector->edid = NULL;
}
 
-   ret = pm_runtime_get_sync(connector->dev->dev);
-   if (ret < 0 && ret != -EACCES)
-   return conn_status;
+   /* Outputs are only polled while runtime active, so acquiring a
+* runtime PM ref here is unnecessary (and would deadlock upon
+* runtime suspend because it waits for polling to finish).
+*/
+   if (!drm_kms_helper_is_poll_worker()) {
+   ret = pm_runtime_get_sync(connector->dev->dev);
+   if (ret < 0 && ret != -EACCES)
+   return conn_status;
+   }
 
nv_encoder = nouveau_connector_ddc_detect(connector);
if (nv_encoder && (i2c = nv_encoder->i2c) != NULL) {
@@ -647,8 +653,10 @@ nouveau_connector_detect(struct drm_connector *connector, 
bool force)
 
  out:
 
-   pm_runtime_mark_last_busy(connector->dev->dev);
-   pm_runtime_put_autosuspend(connector->dev->dev);
+   if (!drm_kms_helper_is_poll_worker()) {
+   pm_runtime_mark_last_busy(connector->dev->dev);
+   pm_runtime_put_autosuspend(connector->dev->dev);
+   }
 
return conn_status;
 }
-- 
2.15.1

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

Re: [Intel-gfx] [PATCH] Revert "drm/i915: mark all device info struct with __initconst"

[Lukas Wunner]

On Mon, Jan 29, 2018 at 09:27:39AM +, Lionel Landwerlin wrote:
> On 29/01/18 09:02, Chris Wilson wrote:
> > Quoting Lionel Landwerlin (2018-01-29 08:33:46)
> > > This reverts commit 5b54eddd3920e9f6f1a6d972454baf350cbae77e.
> > > 
> > >   Conflicts:
> > >  drivers/gpu/drm/i915/i915_pci.c
> > > 
> > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=104805
> > Fixes: 5b54eddd3920 ("drm/i915: mark all device info struct with 
> > __initconst")
> > 
> > So it ends up in the right place.
> > -Chris
> > 
> Thanks, pushed.

This is missing
Reported-by: Chris Murphy 

Also, someone please put on the todo list to write an igt test which
unbinds and rebinds the driver to prevent this regression from
reappearing:

echo PCI-ID > /sys/bus/pci/drivers/i915/unbind
echo PCI-ID > /sys/bus/pci/drivers/i915/bind

Thanks,

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

Re: [Intel-gfx] [Bug 104805] regression in 4.15 unable to reboot/poweroff, vgaswitcheroo doesn't work, RIP: 0010:i915_pci_probe+0x11/0x70 [i915]

[Lukas Wunner]

On Sat, Jan 27, 2018 at 10:42:45AM +, bugzilla-dae...@freedesktop.org wrote:
> https://bugs.freedesktop.org/show_bug.cgi?id=104805
> 
> --- Comment #6 from Chris Wilson  ---
> Sigh. Revert then solve the bloat another way. I think we can move it to a
> seperate module and only load it during pci_probe. (That module can then play
> some tricks to construct driver_info using private commands to allow feature
> enabling.)

The following might work:  Set i915_pci_driver->id_table = NULL.
Then in i915_init(), after calling pci_register_driver(), iterate
over pciidlist[] and call pci_add_dynid() for each entry.  Finally,
call pci_free_dynids().

The problem is that after manually unbinding the driver via sysfs,
you can't bind it again.

Also, as soon as Intel introduces a discrete GPU (e.g. in a dock,
hotplugged via Thunderbolt), the whole idea falls apart.

How many bytes are we talking about here anyway?  I imagine it's less
than a page.  Honestly I think it's not worth the effort.

I'm wondering why noone else is seeing this crash, myself included.
Is the driver somehow unbound and rebound on Chris Murphy's machine?

@Chris Murphy: You may have noticed an 8 second delay on reboot with
4.15.  Before you waste time bisecting it:  A fix is already queued
for 4.16 and marked for stable:
https://git.kernel.org/pub/scm/linux/kernel/git/helgaas/pci.git/commit/?id=493fb50e958c

Have a pleasant fin de weekend.

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

Re: [Intel-gfx] [PATCH] drm/i915/fbdev: Serialise early hotplug events with async fbdev config

[Lukas Wunner]

On Sun, Nov 26, 2017 at 11:58:43AM +, Chris Wilson wrote:
> Quoting Lukas Wunner (2017-11-26 11:49:19)
> > Hm, the race at hand would be solved by the intel_fbdev_sync() below,
> > or am I missing something?  Still wondering why it's necessary to
> > leave the fbdev around...
> 
> The race is solved, but if we do free ifbdev, we can't dereference
> ifbdev prior to the sync; and we store the async cookie inside ifbdev.
> Bleugh. Catch 22.

Right.  Oh dear god!  We could move the cookie into dev_priv, the
fbdev_suspend_work is also there, outside of struct intel_fbdev.


> What we might do then is just pull the struct into dev_priv under
> ifdef FBDEV.

I vaguely remember something that dev_priv deliberately only contains
a pointer to struct intel_fbdev, that it *was* embedded in dev_priv
in the past but moved out for some reason.


> > However the "if (ifbdev->vma)" looks a bit fishy, ifbdev could be NULL
> > (e.g. if BIOS fb was too small but intelfb_alloc() failed) so I think
> > this might lead to a null pointer deref.  Does it make a difference
> > if we check for ifbdev versus ifbdev->vma?  I also notice that you
> > added a check for ifbdev->vma with 15727ed0d944 but Daniel later
> > removed it with 88be58be886f.
> 
> We know that ifbdev is non-NULL and can't become NULL until fini. So
> after the sync point, we want to ask the question of whether the config
> was successful, for that I used to use ->fb which now replaced by ->vma.

Yes if the fbdev is kept around then obviously it's fine to deref it.

Thanks,

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

Re: [Intel-gfx] [PATCH] drm/i915/fbdev: Serialise early hotplug events with async fbdev config

[Lukas Wunner]

On Sat, Nov 25, 2017 at 07:41:55PM +, Chris Wilson wrote:
> As both the hotplug event and fbdev configuration run asynchronously, it
> is possible for them to run concurrently. If configuration fails, we were
> freeing the fbdev causing a use-after-free in the hotplug event.
> 
> <7>[ 3069.935211] [drm:intel_fb_initial_config [i915]] Not using firmware 
> configuration
> <7>[ 3069.935225] [drm:drm_setup_crtcs] looking for cmdline mode on connector 
> 77
> <7>[ 3069.935229] [drm:drm_setup_crtcs] looking for preferred mode on 
> connector 77 0
> <7>[ 3069.935233] [drm:drm_setup_crtcs] found mode 3200x1800
> <7>[ 3069.935236] [drm:drm_setup_crtcs] picking CRTCs for 8192x8192 config
> <7>[ 3069.935253] [drm:drm_setup_crtcs] desired mode 3200x1800 set on crtc 43 
> (0,0)
> <7>[ 3069.935323] [drm:intelfb_create [i915]] no BIOS fb, allocating a new one
> <4>[ 3069.967737] general protection fault:  [#1] PREEMPT SMP
> <0>[ 3069.977453] -
> <4>[ 3069.977457] Modules linked in: i915(+) vgem snd_hda_codec_hdmi 
> snd_hda_codec_realtek snd_hda_codec_generic x86_pkg_temp_thermal 
> intel_powerclamp coretemp crct10dif_pclmul crc32_pclmul ghash_clmulni_intel 
> snd_hda_codec snd_hwdep snd_hda_core snd_pcm r8169 mei_me mii prime_numbers 
> mei i2c_hid pinctrl_geminilake pinctrl_intel [last unloaded: i915]
> <4>[ 3069.977492] CPU: 1 PID: 15414 Comm: kworker/1:0 Tainted: G U
>   4.14.0-CI-CI_DRM_3388+ #1
> <4>[ 3069.977497] Hardware name: Intel Corp. Geminilake/GLK RVP1 DDR4 (05), 
> BIOS GELKRVPA.X64.0062.B30.1708222146 08/22/2017
> <4>[ 3069.977508] Workqueue: events output_poll_execute
> <4>[ 3069.977512] task: 880177734e40 task.stack: c90001fe4000
> <4>[ 3069.977519] RIP: 0010:__lock_acquire+0x109/0x1b60
> <4>[ 3069.977523] RSP: 0018:c90001fe7bb0 EFLAGS: 00010002
> <4>[ 3069.977526] RAX: 6b6b6b6b6b6b6b6b RBX: 0282 RCX: 
> 
> <4>[ 3069.977530] RDX:  RSI:  RDI: 
> 880170d4efd0
> <4>[ 3069.977534] RBP: c90001fe7c70 R08: 0001 R09: 
> 
> <4>[ 3069.977538] R10:  R11: 81899609 R12: 
> 880170d4efd0
> <4>[ 3069.977542] R13: 880177734e40 R14: 0001 R15: 
> 
> <4>[ 3069.977547] FS:  () GS:88017fc8() 
> knlGS:
> <4>[ 3069.977551] CS:  0010 DS:  ES:  CR0: 80050033
> <4>[ 3069.977555] CR2: 7f7e8b7bcf04 CR3: 03e0f000 CR4: 
> 003406e0
> <4>[ 3069.977559] Call Trace:
> <4>[ 3069.977565]  ? mark_held_locks+0x64/0x90
> <4>[ 3069.977571]  ? _raw_spin_unlock_irq+0x24/0x50
> <4>[ 3069.977575]  ? _raw_spin_unlock_irq+0x24/0x50
> <4>[ 3069.977579]  ? trace_hardirqs_on_caller+0xde/0x1c0
> <4>[ 3069.977583]  ? _raw_spin_unlock_irq+0x2f/0x50
> <4>[ 3069.977588]  ? finish_task_switch+0xa5/0x210
> <4>[ 3069.977592]  ? lock_acquire+0xaf/0x200
> <4>[ 3069.977596]  lock_acquire+0xaf/0x200
> <4>[ 3069.977600]  ? __mutex_lock+0x5e9/0x9b0
> <4>[ 3069.977604]  _raw_spin_lock+0x2a/0x40
> <4>[ 3069.977608]  ? __mutex_lock+0x5e9/0x9b0
> <4>[ 3069.977612]  __mutex_lock+0x5e9/0x9b0
> <4>[ 3069.977616]  ? drm_fb_helper_hotplug_event.part.19+0x16/0xa0
> <4>[ 3069.977621]  ? drm_fb_helper_hotplug_event.part.19+0x16/0xa0
> <4>[ 3069.977625]  drm_fb_helper_hotplug_event.part.19+0x16/0xa0
> <4>[ 3069.977630]  output_poll_execute+0x8d/0x180
> <4>[ 3069.977635]  process_one_work+0x22e/0x660
> <4>[ 3069.977640]  worker_thread+0x48/0x3a0
> <4>[ 3069.977644]  ? _raw_spin_unlock_irqrestore+0x4c/0x60
> <4>[ 3069.977649]  kthread+0x102/0x140
> <4>[ 3069.977653]  ? process_one_work+0x660/0x660
> <4>[ 3069.977657]  ? kthread_create_on_node+0x40/0x40
> <4>[ 3069.977662]  ret_from_fork+0x27/0x40
> <4>[ 3069.977666] Code: 8d 62 f8 c3 49 81 3c 24 e0 fa 3c 82 41 be 00 00 00 00 
> 45 0f 45 f0 83 fe 01 77 86 89 f0 49 8b 44 c4 08 48 85 c0 0f 84 76 ff ff ff 
>  ff 80 38 01 00 00 8b 1d 62 f9 e8 01 45 8b 85 b8 08 00 00 85
> <1>[ 3069.977707] RIP: __lock_acquire+0x109/0x1b60 RSP: c90001fe7bb0
> <4>[ 3069.977712] ---[ end trace 4ad012eb3af62df7 ]---
> 
> In order to keep the dev_priv->ifbdev alive after failure, we have to
> avoid the free and leave it empty until we unload the module. Then we
> can use intel_fbdev_sync() to serialise the hotplug event with the
> configuration. The serialisation between the two was removed in commit
> 934458c2c9

Re: [Intel-gfx] [PATCH] drm/i915/fbdev: Serialise early hotplug events with async fbdev config

[Lukas Wunner]

On Sat, Nov 25, 2017 at 07:41:55PM +, Chris Wilson wrote:
> @@ -697,10 +697,8 @@ static void intel_fbdev_initial_config(void *data, 
> async_cookie_t cookie)
>  
>   /* Due to peculiar init order wrt to hpd handling this is separate. */
>   if (drm_fb_helper_initial_config(>helper,
> -  ifbdev->preferred_bpp)) {
> +  ifbdev->preferred_bpp))
>   intel_fbdev_unregister(to_i915(ifbdev->helper.dev));
> - intel_fbdev_fini(to_i915(ifbdev->helper.dev));
> - }
>  }

Hm, the race at hand would be solved by the intel_fbdev_sync() below,
or am I missing something?  Still wondering why it's necessary to
leave the fbdev around...


> @@ -800,7 +798,11 @@ void intel_fbdev_output_poll_changed(struct drm_device 
> *dev)
>  {
>   struct intel_fbdev *ifbdev = to_i915(dev)->fbdev;
>  
> - if (ifbdev)
> + if (!ifbdev)
> + return;
> +
> + intel_fbdev_sync(ifbdev);
> + if (ifbdev->vma)
>   drm_fb_helper_hotplug_event(>helper);
>  }

This hunk looks good, as you note the synchronization was already there
but had to be reverted because I failed to notice that a "+ 1" needs to
be added to the cookie.  You did a much better job than me understanding
how the async API works with 43cee314345a.

However the "if (ifbdev->vma)" looks a bit fishy, ifbdev could be NULL
(e.g. if BIOS fb was too small but intelfb_alloc() failed) so I think
this might lead to a null pointer deref.  Does it make a difference
if we check for ifbdev versus ifbdev->vma?  I also notice that you
added a check for ifbdev->vma with 15727ed0d944 but Daniel later
removed it with 88be58be886f.

I guess a check *is* necessary here because fbdev initialization might
have failed, but I'd just check for "if (ifbdev)".

Thanks & have a pleasant Sunday afternoon.

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

Re: [Intel-gfx] [PATCH] drm/i915/fbdev: Serialise early hotplug events with async fbdev config

[Lukas Wunner]

On Sat, Nov 25, 2017 at 07:41:55PM +, Chris Wilson wrote:
> As both the hotplug event and fbdev configuration run asynchronously, it
> is possible for them to run concurrently. If configuration fails, we were
> freeing the fbdev causing a use-after-free in the hotplug event.

That'll teach me to muck around in this complicated driver. :-)

IIUC, the issue is that ifbdev is briefly non-NULL and the if clause
happens to be executed when it's non-NULL and it becomes NULL upon
or during execution of intel_fbdev_output_poll_changed(), is that
correct?

Wouldn't the proper solution be to set ifbdev only after configuration
was successful, i.e. somewhere at the end of intelfb_create()?
With a memory barrier in case intel_fbdev_output_poll_changed is running
on a different CPU?


> In order to keep the dev_priv->ifbdev alive after failure, we have to
> avoid the free and leave it empty until we unload the module.

Well, that seems to defeat the goal stated in the commit message of
366e39b4d2c5 to free up the memory if fbdev initialization failed.
Not that it's a big deal for me personally, just noting. :-)

Thanks,

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

Re: [Intel-gfx] [PULL] drm-misc-fixes

[Lukas Wunner]

On Fri, May 26, 2017 at 08:36:45AM +0200, Daniel Vetter wrote:
> On Thu, May 25, 2017 at 7:44 PM, Sean Paul <seanp...@chromium.org> wrote:
> > The pull is noisy because it includes -rc2.
> 
> dim has you covered for this, in case you've rolled forward but Dave
> hasn't yet, you can regenerate against linus upstream branch for a
> cleaner pull (but still warn Dave ofc):
> 
> $ dim pull-request drm-misc-next origin/master

Is it worth documenting this?  If so, below is a suggestion.
Feel free to rephrase as you see fit.

Thanks,

Lukas

-- >8 --
Subject: [PATCH] drm-misc: Document recommended base for -fixes pulls

Summarize the following discussion on dri-devel:

   "dim has you covered for this, in case you've rolled forward but Dave
hasn't yet, you can regenerate against linus upstream branch for a
cleaner pull (but still warn Dave ofc):
$ dim pull-request drm-misc-next origin/master" (Daniel)

   "FWIW this is what I've always done with drm-intel-fixes." (Jani)

   "As long as I'm warned in the pull request I often fast forward to
the base if my tree is clean, just to avoid the pull request having
noise in it." (Dave)

Cc: Daniel Vetter <daniel.vet...@ffwll.ch>
Cc: Jani Nikula <jani.nik...@linux.intel.com>
Cc: Sean Paul <seanp...@chromium.org>
Cc: Dave Airlie <airl...@redhat.com>
Signed-off-by: Lukas Wunner <lu...@wunner.de>
---
 drm-misc.rst | 8 
 1 file changed, 8 insertions(+)

diff --git a/drm-misc.rst b/drm-misc.rst
index d56c3c7..c66ac67 100644
--- a/drm-misc.rst
+++ b/drm-misc.rst
@@ -178,6 +178,14 @@ Maintainers mostly provide services to keep drm-misc 
running smoothly:
   keep it current. We try to avoid backmerges for bugfix branches, and rebasing
   isn't an option with multiple committers.
 
+* Pull requests become noisy if `-fixes` has been fast-forwarded to Linus'
+  latest -rc tag but drm-upstream hasn't done the same yet: The shortlog
+  will contain not just the queued fixes but also anything else that has
+  landed in Linus' tree in the meantime. The best practice is then to base
+  the pull request on Linus' master branch (rather than drm-upstream) by
+  setting the `upstream` argument for ``dim pull-request`` accordingly.
+  Upstream should be warned that they haven't fast-forwarded yet.
+
 * During the merge-windo blackout, i.e. from -rc6 on until the merge window
   closes with the release of -rc1, try to track `drm-next` with the
   `-next-fixes` branch. Do not advance past -rc1, otherwise the automagic in
-- 
2.11.0

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

Re: [Intel-gfx] [PULL] drm-misc-fixes

[Lukas Wunner]

On Thu, May 25, 2017 at 01:44:04PM -0400, Sean Paul wrote:
> The pull is noisy because it includes -rc2.

Looks like I've caused another fine mess by fast-forwarding -fixes. :-(

I followed the docs in drm-misc.rst which say:

* Fast-forward (when possible) `-fixes` to each released -rc kernel tag, to
  keep it current. We try to avoid backmerges for bugfix branches, and rebasing
  isn't an option with multiple committers.

How do you actually handle this?  Fast-forward only if Dave has already
done so himself?

Thanks,

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

Re: [Intel-gfx] [PATCH] dim: Enforce review requirements

[Lukas Wunner]

On Wed, May 24, 2017 at 11:28:12AM +0200, Daniel Vetter wrote:
> We can't check this when applying (since r-b/a-b tags often get added
> afterwards), but we can check this when pushing. This only looks at
> patches authored by the pusher.

BTW, can we also have a rule that large drivers (i.e. with a large user
base) are required to provide at least one person at all times (or at
least outside the merge window) who is able to review fixes, push them
to the driver's repo and send a timely pull to Dave?

On multiple occasions I've seen Dave send a fixes pull with an apology
along the lines of "a bit more than usual at this time of the cycle
because it includes two weeks worth of driver XYZ fixes".

Thanks,

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

Re: [Intel-gfx] [PATCH] dim: Enforce review requirements

[Lukas Wunner]

On Wed, May 24, 2017 at 11:28:12AM +0200, Daniel Vetter wrote:
> We can't check this when applying (since r-b/a-b tags often get added
> afterwards), but we can check this when pushing. This only looks at
> patches authored by the pusher.
> 
> Also update the docs to highlight that review requirements hold
> especially also for bugfixes.
> 
> Cc: Patrik Jakobsson <patrik.r.jakobs...@gmail.com>
> Cc: Lukas Wunner <lu...@wunner.de>
> Cc: Alex Deucher <alexander.deuc...@amd.com>
> Cc: Christian König <deathsim...@vodafone.de>
> Cc: Sean Paul <seanp...@chromium.org>
> Signed-off-by: Daniel Vetter <daniel.vet...@intel.com>

Reviewed-by: Lukas Wunner <lu...@wunner.de>

Thanks,

Lukas

> ---
>  dim  | 42 ++
>  drm-misc.rst |  4 +++-
>  2 files changed, 41 insertions(+), 5 deletions(-)
> 
> diff --git a/dim b/dim
> index baa0b3832314..79738a1b37a0 100755
> --- a/dim
> +++ b/dim
> @@ -340,6 +340,15 @@ function git_branch_exists # branch
>   fi
>  }
>  
> +function git_committer_email
> +{
> + if ! commiter_email=$(git config --get user.email) ; then
> + commiter_email=$EMAIL
> + fi
> +
> + echo $commiter_email
> +}
> +
>  # get message id from file
>  # $1 = file
>  message_get_id ()
> @@ -632,11 +641,32 @@ function dim_rebuild_tip
>   exit 1
>   fi
>  }
> +
> +# additional patch checks before pushing, e.g. for r-b tags
> +function checkpatch_commit_push
> +{
> + local sha1
> +
> + sha1=$1
> +
> + # check for a-b/r-b tag
> + if git show -s $sha1 | grep -qi '\(reviewed\|acked\)\S*-by:'  ; then
> + return
> + fi
> +
> + # check for commiter != author
> + if [[ $(git show -s $sha1 --format="format:%ce") != $(git show -s $sha1 
> --format="format:%ae") ]]; then
> + return
> + fi
> +
> + warn_or_fail "$sha1 is lacking mandatory review"
> +}
> +
>  # push branch $1, rebuild drm-tip. the rest of the arguments are passed to 
> git
>  # push.
>  function dim_push_branch
>  {
> - local branch remote
> + local branch remote commiter_email
>  
>   branch=${1:?$usage}
>   shift
> @@ -645,6 +675,12 @@ function dim_push_branch
>  
>   remote=$(branch_to_remote $branch)
>  
> + commiter_email=$(git_committer_email)
> +
> + for sha1 in $(git rev-list $branch@{u}..$branch 
> --committer="$commiter_email" --no-merges) ; do
> + checkpatch_commit_push $sha1
> + done
> +
>   git push $DRY_RUN $remote $branch "$@"
>  
>   update_linux_next $branch drm-intel-next-queued drm-intel-next-fixes 
> drm-intel-fixes
> @@ -690,9 +726,7 @@ function dim_apply_branch
>  
>   message_id=$(message_get_id $file)
>  
> - if ! commiter_email=$(git config --get user.email) ; then
> - commiter_email=$EMAIL
> - fi
> + commiter_email=$(git_committer_email)
>  
>   patch_from=$(grep "From:" "$file" | head -1)
>   if [[ "$patch_from" != *"$commiter_email"* ]] ; then
> diff --git a/drm-misc.rst b/drm-misc.rst
> index caba8dc77696..d56c3c7f92a3 100644
> --- a/drm-misc.rst
> +++ b/drm-misc.rst
> @@ -90,7 +90,9 @@ Merge Criteria
>  Right now the only hard merge criteria are:
>  
>  * Patch is properly reviewed or at least Ack, i.e. don't just push your own
> -  stuff directly.
> +  stuff directly. This rule holds even more for bugfix patches - it would be
> +  embarrassing if the bugfix contains a small gotcha that review would have
> +  caught.
>  
>  * drm-misc is for drm core (non-driver) patches, subsystem-wide refactorings,
>and small trivial patches all over (including drivers). For a detailed 
> list of
> -- 
> 2.11.0
> 
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH v2 1/2] PCI / PM: Add needs_resume flag to avoid suspend complete optimization

[Lukas Wunner]

On Tue, Apr 25, 2017 at 11:55:07AM +0300, Imre Deak wrote:
> On Tue, Apr 25, 2017 at 08:21:49AM +0200, Lukas Wunner wrote:
> > On Mon, Apr 24, 2017 at 10:56:40PM +0200, Rafael J. Wysocki wrote:
> > > On Monday, April 24, 2017 10:42:42 PM Lukas Wunner wrote:
> > > > On Mon, Apr 24, 2017 at 10:02:30PM +0200, Lukas Wunner wrote:
> > > > > On Mon, Apr 24, 2017 at 05:27:42PM +0300, Imre Deak wrote:
> > > > > > Some drivers - like i915 - may not support the system suspend direct
> > > > > > complete optimization due to differences in their runtime and system
> > > > > > suspend sequence. Add a flag that when set resumes the device before
> > > > > > calling the driver's system suspend handlers which effectively 
> > > > > > disables
> > > > > > the optimization.
> > > > > 
> > > > > FWIW, there are at least two alternative solutions to this problem 
> > > > > which
> > > > > do not require changes to the PCI core:
> > > > > 
> > > > > (1) Add a ->prepare hook to i915_pm_ops which calls 
> > > > > pm_runtime_get_sync()
> > > > > and a ->complete hook which calls pm_runtime_put().
> > > > 
> > > > Thinking a bit more about this, it's even simpler:  The PM core acquires
> > > > a runtime PM ref in device_prepare() and releases it in 
> > > > device_complete(),
> > > > so it's sufficient to just call pm_runtime_resume() in a ->prepare hook
> > > > that's newly added to i915.  No ->complete hook necessary.  Tentative
> > > > patch below, based on drm-intel-fixes, would replace both of your 
> > > > patches.
> > > 
> > > Calling it in ->prepare() means that everybody is now waiting for you to
> > > resume.
> > > 
> > > Not quite optimal IMO.
> > 
> > Okay, understood.
> > 
> > However in the absence of a device_link from the HDA device (consumer)
> > to the GPU (supplier), there's no guarantee that the GPU is resumed
> > when the HDA device needs it due to the asynchronous invocation of
> > the ->suspend hooks.  This is assuming that the HDA device already
> > needs the GPU during ->suspend phase.
> 
> i915 has ->resume_early and ->suspend_late hooks that provides the above
> guarantee.

Employing device links is preferable IMO:

* Less code.  Just find the HDA device in your ->probe hook and call
  device_link_add().  Alternatively do it from the HDA driver.  Even
  adding the link from both drivers should be fine.  I can provide
  you with an evaluation patch if you'd like.

* Avoidance of doing things behind the PM core's back.

* Takes care of more stuff than just system sleep (e.g. shutdown ordering).

Thanks,

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

Re: [Intel-gfx] [PATCH v2 1/2] PCI / PM: Add needs_resume flag to avoid suspend complete optimization

[Lukas Wunner]

On Mon, Apr 24, 2017 at 10:56:40PM +0200, Rafael J. Wysocki wrote:
> On Monday, April 24, 2017 10:42:42 PM Lukas Wunner wrote:
> > On Mon, Apr 24, 2017 at 10:02:30PM +0200, Lukas Wunner wrote:
> > > On Mon, Apr 24, 2017 at 05:27:42PM +0300, Imre Deak wrote:
> > > > Some drivers - like i915 - may not support the system suspend direct
> > > > complete optimization due to differences in their runtime and system
> > > > suspend sequence. Add a flag that when set resumes the device before
> > > > calling the driver's system suspend handlers which effectively disables
> > > > the optimization.
> > > 
> > > FWIW, there are at least two alternative solutions to this problem which
> > > do not require changes to the PCI core:
> > > 
> > > (1) Add a ->prepare hook to i915_pm_ops which calls pm_runtime_get_sync()
> > > and a ->complete hook which calls pm_runtime_put().
> > 
> > Thinking a bit more about this, it's even simpler:  The PM core acquires
> > a runtime PM ref in device_prepare() and releases it in device_complete(),
> > so it's sufficient to just call pm_runtime_resume() in a ->prepare hook
> > that's newly added to i915.  No ->complete hook necessary.  Tentative
> > patch below, based on drm-intel-fixes, would replace both of your patches.
> 
> Calling it in ->prepare() means that everybody is now waiting for you to
> resume.
> 
> Not quite optimal IMO.

Okay, understood.

However in the absence of a device_link from the HDA device (consumer)
to the GPU (supplier), there's no guarantee that the GPU is resumed
when the HDA device needs it due to the asynchronous invocation of
the ->suspend hooks.  This is assuming that the HDA device already
needs the GPU during ->suspend phase.

Thanks,

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

Re: [Intel-gfx] [PATCH v2 1/2] PCI / PM: Add needs_resume flag to avoid suspend complete optimization

[Lukas Wunner]

On Mon, Apr 24, 2017 at 10:02:30PM +0200, Lukas Wunner wrote:
> On Mon, Apr 24, 2017 at 05:27:42PM +0300, Imre Deak wrote:
> > Some drivers - like i915 - may not support the system suspend direct
> > complete optimization due to differences in their runtime and system
> > suspend sequence. Add a flag that when set resumes the device before
> > calling the driver's system suspend handlers which effectively disables
> > the optimization.
> 
> FWIW, there are at least two alternative solutions to this problem which
> do not require changes to the PCI core:
> 
> (1) Add a ->prepare hook to i915_pm_ops which calls pm_runtime_get_sync()
> and a ->complete hook which calls pm_runtime_put().

Thinking a bit more about this, it's even simpler:  The PM core acquires
a runtime PM ref in device_prepare() and releases it in device_complete(),
so it's sufficient to just call pm_runtime_resume() in a ->prepare hook
that's newly added to i915.  No ->complete hook necessary.  Tentative
patch below, based on drm-intel-fixes, would replace both of your patches.

Thanks,

Lukas

-- >8 --
diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 5c089b3..6ef156b 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -1820,6 +1820,11 @@ void i915_reset(struct drm_i915_private *dev_priv)
goto wakeup;
 }
 
+static int i915_pm_prepare(struct device *kdev)
+{
+   pm_runtime_resume(kdev);
+}
+
 static int i915_pm_suspend(struct device *kdev)
 {
struct pci_dev *pdev = to_pci_dev(kdev);
@@ -2451,6 +2456,7 @@ static int intel_runtime_resume(struct device *kdev)
 * S0ix (via system suspend) and S3 event handlers [PMSG_SUSPEND,
 * PMSG_RESUME]
 */
+   .prepare = i915_pm_prepare,
.suspend = i915_pm_suspend,
.suspend_late = i915_pm_suspend_late,
.resume_early = i915_pm_resume_early,
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH v2 2/2] drm/i915: Prevent the system suspend complete optimization

[Lukas Wunner]

On Mon, Apr 24, 2017 at 05:27:43PM +0300, Imre Deak wrote:
> Since
> 
> commit bac2a909a096c9110525c18cbb8ce73c660d5f71
> Author: Rafael J. Wysocki <rafael.j.wyso...@intel.com>
> Date:   Wed Jan 21 02:17:42 2015 +0100
> 
> PCI / PM: Avoid resuming PCI devices during system suspend

This is not the commit you are looking for. :-)  See below.


> PCI devices will default to allowing the system suspend complete
> optimization where devices are not woken up during system suspend if
> they were already runtime suspended. This however breaks the i915/HDA
> drivers for two reasons:
> 
> - The i915 driver has system suspend specific steps that it needs to
>   run, that bring the device to a different state than its runtime
>   suspended state.
> 
> - The HDA driver's suspend handler requires power that it will request
>   from the i915 driver's power domain handler. This in turn requires the
>   i915 driver to runtime resume itself, but this won't be possible if the
>   suspend complete optimization is in effect: in this case the i915
>   runtime PM is disabled and trying to get an RPM reference returns
>   -EACCESS.

Hm, sounds like something that needs to be solved with device_links.


> 
> Solve this by requiring the PCI/PM core to resume the device during
> system suspend which in effect disables the suspend complete optimization.
> 
> One possibility for this bug staying hidden for so long is that the
> optimization for a device is disabled if it's disabled for any of its
> children devices. i915 may have a backlight device as its child which
> doesn't support runtime PM and so doesn't allow the optimization either.
> So if this backlight device got registered the bug stayed hidden.

No, the reason this hasn't popped up earlier is because direct_complete
has only been enabled for DRM devices for a few months now, to be specific
since

commit d14d2a8453d650bea32a1c5271af1458cd283a0f
Author: Lukas Wunner <lu...@wunner.de>
Date:   Wed Jun 8 12:49:29 2016 +0200

drm: Remove dev_pm_ops from drm_class

which landed in v4.8.

(Sorry for not raising my voice earlier, this patch appeared on my radar
just now.)

Kind regards,

Lukas

> 
> Credits to Marta, Tomi and David who enabled pstore logging,
> that caught one instance of this issue across a suspend/
> resume-to-ram and Ville who rememberd that the optimization was enabled
> for some devices at one point.
> 
> The first WARN triggered by the problem:
> 
> [ 6250.746445] WARNING: CPU: 2 PID: 17384 at 
> drivers/gpu/drm/i915/intel_runtime_pm.c:2846 intel_runtime_pm_get+0x6b/0xd0 
> [i915]
> [ 6250.746448] pm_runtime_get_sync() failed: -13
> [ 6250.746451] Modules linked in: snd_hda_intel i915 vgem snd_hda_codec_hdmi 
> x86_pkg_temp_thermal intel_powerclamp coretemp crct10dif_pclmul crc32_pclmul
> snd_hda_codec_realtek snd_hda_codec_generic ghash_clmulni_intel e1000e 
> snd_hda_codec snd_hwdep snd_hda_core ptp mei_me pps_core snd_pcm lpc_ich mei 
> prime_
> numbers i2c_hid i2c_designware_platform i2c_designware_core [last unloaded: 
> i915]
> [ 6250.746512] CPU: 2 PID: 17384 Comm: kworker/u8:0 Tainted: G U  W   
> 4.11.0-rc5-CI-CI_DRM_334+ #1
> [ 6250.746515] Hardware name:  /NUC5i5RYB, BIOS 
> RYBDWi35.86A.0362.2017.0118.0940 01/18/2017
> [ 6250.746521] Workqueue: events_unbound async_run_entry_fn
> [ 6250.746525] Call Trace:
> [ 6250.746530]  dump_stack+0x67/0x92
> [ 6250.746536]  __warn+0xc6/0xe0
> [ 6250.746542]  ? pci_restore_standard_config+0x40/0x40
> [ 6250.746546]  warn_slowpath_fmt+0x46/0x50
> [ 6250.746553]  ? __pm_runtime_resume+0x56/0x80
> [ 6250.746584]  intel_runtime_pm_get+0x6b/0xd0 [i915]
> [ 6250.746610]  intel_display_power_get+0x1b/0x40 [i915]
> [ 6250.746646]  i915_audio_component_get_power+0x15/0x20 [i915]
> [ 6250.746654]  snd_hdac_display_power+0xc8/0x110 [snd_hda_core]
> [ 6250.746661]  azx_runtime_resume+0x218/0x280 [snd_hda_intel]
> [ 6250.746667]  pci_pm_runtime_resume+0x76/0xa0
> [ 6250.746672]  __rpm_callback+0xb4/0x1f0
> [ 6250.746677]  ? pci_restore_standard_config+0x40/0x40
> [ 6250.746682]  rpm_callback+0x1f/0x80
> [ 6250.746686]  ? pci_restore_standard_config+0x40/0x40
> [ 6250.746690]  rpm_resume+0x4ba/0x740
> [ 6250.746698]  __pm_runtime_resume+0x49/0x80
> [ 6250.746703]  pci_pm_suspend+0x57/0x140
> [ 6250.746709]  dpm_run_callback+0x6f/0x330
> [ 6250.746713]  ? pci_pm_freeze+0xe0/0xe0
> [ 6250.746718]  __device_suspend+0xf9/0x370
> [ 6250.746724]  ? dpm_watchdog_set+0x60/0x60
> [ 6250.746730]  async_suspend+0x1a/0x90
> [ 6250.746735]  async_run_entry_fn+0x34/0x160
> [ 6250.746741]  process_one_work+0x1f2/0x6d0
> [ 6250.746749]  worker_thread+0x49/0x4a0
> [ 6250.746755]  kthread+0x107/0x140
> [ 6250.746759]  ? process_one_work

Re: [Intel-gfx] [PATCH v2 1/2] PCI / PM: Add needs_resume flag to avoid suspend complete optimization

[Lukas Wunner]

On Mon, Apr 24, 2017 at 05:27:42PM +0300, Imre Deak wrote:
> Some drivers - like i915 - may not support the system suspend direct
> complete optimization due to differences in their runtime and system
> suspend sequence. Add a flag that when set resumes the device before
> calling the driver's system suspend handlers which effectively disables
> the optimization.

FWIW, there are at least two alternative solutions to this problem which
do not require changes to the PCI core:

(1) Add a ->prepare hook to i915_pm_ops which calls pm_runtime_get_sync()
and a ->complete hook which calls pm_runtime_put().

(2) Assign a struct dev_pm_domain to the GPU which overrides the PCI
->prepare hook to return 0 unconditionally.  See here for an example:
http://lxr.free-electrons.com/source/drivers/gpu/vga/vga_switcheroo.c#L1056

It may be worth considering approach (1) (which is the simpler of the two)
in favor of this patch.  Because I'm not sure there are many drivers that
will make use of this change to the PCI core.

It's unfortunate that the return value of PCI drivers' ->prepare hooks
has totally different semantics than that of bus types, thereby
necessitating such workarounds. :-(

Best regards,

Lukas

> 
> Needed by the next patch fixing suspend/resume on i915.
> 
> Suggested by Rafael.
> 
> Cc: Rafael J. Wysocki 
> Cc: Bjorn Helgaas 
> Cc: linux-...@vger.kernel.org
> Cc: sta...@vger.kernel.org
> Signed-off-by: Imre Deak 
> ---
>  drivers/pci/pci.c   | 3 ++-
>  include/linux/pci.h | 7 +++
>  2 files changed, 9 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index 7904d02..020f02d 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -2141,7 +2141,8 @@ bool pci_dev_keep_suspended(struct pci_dev *pci_dev)
>  
>   if (!pm_runtime_suspended(dev)
>   || pci_target_state(pci_dev) != pci_dev->current_state
> - || platform_pci_need_resume(pci_dev))
> + || platform_pci_need_resume(pci_dev)
> + || pci_dev->needs_resume)
>   return false;
>  
>   /*
> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index e2d1a12..3fe00a6 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -316,6 +316,9 @@ struct pci_dev {
>   unsigned inthotplug_user_indicators:1; /* SlotCtl indicators
> controlled exclusively by
> user sysfs */
> + unsigned intneeds_resume:1; /* Resume before calling the driver's
> +system suspend hooks, disabling the
> +direct_complete optimization. */
>   unsigned intd3_delay;   /* D3->D0 transition time in ms */
>   unsigned intd3cold_delay;   /* D3cold->D0 transition time in ms */
>  
> @@ -1113,6 +1116,10 @@ bool pci_check_pme_status(struct pci_dev *dev);
>  void pci_pme_wakeup_bus(struct pci_bus *bus);
>  void pci_d3cold_enable(struct pci_dev *dev);
>  void pci_d3cold_disable(struct pci_dev *dev);
> +static inline void pci_resume_before_suspend(struct pci_dev *dev, bool 
> enable)
> +{
> + dev->needs_resume = enable;
> +}
>  
>  static inline int pci_enable_wake(struct pci_dev *dev, pci_power_t state,
> bool enable)
> -- 
> 2.5.0
> 
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH v6 1/3] ACPI / bus: Introduce a list of ids for "always present" devices

[Lukas Wunner]

On Wed, Apr 19, 2017 at 12:28:50PM +0200, Rafael J. Wysocki wrote:
> On Wed, Apr 19, 2017 at 10:59 AM, Hans de Goede  wrote:
> > On 18-04-17 15:34, Rafael J. Wysocki wrote:
> >> On Tue, Apr 18, 2017 at 1:54 PM, Hans de Goede  wrote:
> >>>
> >>> Several Cherry Trail devices (all of which ship with Windows 10) hide the
> >>> LPSS PWM controller in ACPI, typically the _STA method looks like this:
> >>>
> >>> Method (_STA, 0, NotSerialized)  // _STA: Status
> >>> {
> >>> If (OSID == One)
> >>> {
> >>> Return (Zero)
> >>> }
> >>>
> >>> Return (0x0F)
> >>> }
> >>>
> >>> Where OSID is some dark magic seen in all Cherry Trail ACPI tables making
> >>> the machine behave differently depending on which OS it *thinks* it is
> >>> booting, this gets set in a number of ways which we cannot control, on
> >>> some newer machines it simple hardcoded to "One" aka win10.
> >>>
> >>> This causes the PWM controller to get hidden, which means Linux cannot
> >>> control the backlight level on cht based tablets / laptops.
> >>>
> >>> Since loading the driver for this does no harm (the only in kernel user
> >>> of it is the i915 driver, which will only use it when it needs it), this
> >>> commit makes acpi_bus_get_status() always set status to ACPI_STA_DEFAULT
> >>> for the 80862288 device, fixing the lack of backlight control.
> >>>
> >>> Signed-off-by: Hans de Goede 
> >>> ---
> >>>  drivers/acpi/bus.c  | 65
> >>> +
> >>>  include/acpi/acpi_bus.h |  6 +
> >>>  2 files changed, 66 insertions(+), 5 deletions(-)
> >>>
> >>> diff --git a/drivers/acpi/bus.c b/drivers/acpi/bus.c
> >>> index 34fbe02..eb30630 100644
> >>> --- a/drivers/acpi/bus.c
> >>> +++ b/drivers/acpi/bus.c
> >>> @@ -34,6 +34,8 @@
> >>>  #include 
> >>>  #include 
> >>>  #ifdef CONFIG_X86
> >>> +#include 
> >>> +#include 
> >>>  #include 
> >>>  #endif
> >>>  #include 
> >>> @@ -132,6 +134,69 @@ int acpi_bus_get_status(struct acpi_device *device)
> >>>  }
> >>>  EXPORT_SYMBOL(acpi_bus_get_status);
> >>>
> >>> +#ifdef CONFIG_X86
> >>> +/*
> >>> + * Some ACPI devices are hidden (status == 0x0) in recent BIOS-es
> >>> because
> >>> + * some recent Windows drivers bind to one device but poke at multiple
> >>> + * devices at the same time, so the others get hidden.
> >>> + * We work around this by always reporting ACPI_STA_DEFAULT for these
> >>> + * devices. Note this MUST only be done for devices where this is safe.
> >>> + *
> >>> + * This forcing of devices to be present is limited to specific CPU
> >>> (SoC)
> >>> + * models both to avoid potentially causing trouble on other models and
> >>> + * because some HIDs are re-used on different SoCs for completely
> >>> + * different devices.
> >>> + */
> >>> +struct always_present_device_id {
> >>> +   struct acpi_device_id hid[2];
> >>> +   struct x86_cpu_id cpu_ids[2];
> >>
> >> This really is x86-specific, so it should go into somewhere like
> >> arch/x86/kernel/acpi/ or drivers/acpi/x86/ (not present yet).
> >
> > Ok, but then how do you want to hook this up, before you said
> > that you wanted to deal with this in acpi_set_device_status(),
> > which belongs in drivers/acpi/bus.c, do you want the x86
> > code to provide something like a
> >
> > bool acpi_device_always_present(struct acpi_device *adev) {
> > }
> >
> > Helper function and use that in the drivers/acpi/bus.c
> > acpi_set_device_status() implementation ?
> 
> Yes, something like that.
> 
> In a header file you can make it depend on CONFIG_X86 and provide and
> empty static inline stub for the "not defined" case.

The PCI subsystem uses __weak stubs such as pcibios_add_bus() for arch-
specific behaviour, or quirks which can be declared in arch/x86 to
constrain them to this specific platform.

Perhaps it makes sense to introduce ACPI quirks which can be declared
the same way as PCI quirks to avoid cluttering the generic code with
arch- or device-specific special cases.  So in this case we'd have
something like:

DECLARE_ACPI_FIXUP_STATUS(const char *hid, const char *uid, hook);

And before calling _STA we'd check for presence of a fixup for the
device in question.  Any additional stuff such as _HRV, CPU ID,
DMI data could be checked in the fixup hook.  Thoughts?

By the way:

> >>> +void acpi_set_device_status(struct acpi_device *adev, u32 sta)
> >>> +{
> >>> +   u32 *status = (u32 *)>status;
> >>> +#ifdef CONFIG_X86
> >>> +   u32 old_status = *status;
> >>> +   int i;
> >>> +
> >>> +   /* acpi_match_device_ids checks status, so start with default */
> >>> +   *status = ACPI_STA_DEFAULT;
> >>> +   for (i = 0; i < ARRAY_SIZE(always_present_device_ids); i++) {
> >>> +   if (acpi_match_device_ids(adev,
> >>> +   always_present_device_ids[i].hid) == 0 &&
> >>> +   adev->pnp.unique_id &&
> >>> +   

Re: [Intel-gfx] [PATCH v3] ACPI / bus: Introduce a list of ids for "always present" devices

[Lukas Wunner]

On Sun, Apr 09, 2017 at 11:46:41AM +0200, Hans de Goede wrote:
> On 09-04-17 09:26, Lukas Wunner wrote:
> > On Sat, Apr 08, 2017 at 05:05:11PM +0200, Hans de Goede wrote:
> > > + pr_debug("Device [%s] is in always present list setting status 
> > > [%08x]\n",
> > > +  adev->pnp.bus_id, ACPI_STA_DEFAULT);
> > 
> > In a lot of places in drivers/acpi/, dev_warn(>dev, ...) is used.
> 
> Rafael asked me to use pr_debug here. But I agree that maybe always
> logging something when this trigger might be a good idea, although
> warning is too high a loglevel IMHO. So I would got with dev_info.

Sorry for not being clear, I just meant that existing code seems to
prefer the dev_*() macros to report device-specific stuff, I wasn't
referring to the severity level.

Thanks,

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

Re: [Intel-gfx] [PATCH v3] ACPI / bus: Introduce a list of ids for "always present" devices

[Lukas Wunner]

On Sat, Apr 08, 2017 at 05:05:11PM +0200, Hans de Goede wrote:
> Several cherrytrail devices (all of which ship with windows 10) hide the
> lpss pwm controller in ACPI, typically the _STA method looks like this:
> 
> Method (_STA, 0, NotSerialized)  // _STA: Status
> {
> If (OSID == One)
> {
> Return (Zero)
> }
> 
> Return (0x0F)
> }
> 
> Where OSID is some dark magic seen in all cherrytrail ACPI tables making
> the machine behave differently depending on which OS it *thinks* it is
> booting, this gets set in a number of ways which we cannot control, on
> some newer machines it simple hardcoded to "One" aka win10.

Would it be a viable alternative to respond differently to _OSI queries
for these devices by amending acpi_osi_dmi_table[] in drivers/acpi/osi.c?


> + pr_debug("Device [%s] is in always present list setting status 
> [%08x]\n",
> +  adev->pnp.bus_id, ACPI_STA_DEFAULT);

In a lot of places in drivers/acpi/, dev_warn(>dev, ...) is used.

Otherwise LGTM.

Thanks,

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

[Intel-gfx] [maintainer-tools PATCH] dim: Fix assorted documentation typos

[Lukas Wunner]

Just a bunch of trivial typos that caught my eye while perusing the
documentation.

Signed-off-by: Lukas Wunner <lu...@wunner.de>
---
 dim.rst   | 14 +++---
 drm-intel.rst | 10 +-
 drm-misc.rst  |  2 +-
 3 files changed, 13 insertions(+), 13 deletions(-)

diff --git a/dim.rst b/dim.rst
index aed79ca1d43c..3aa0eeae9a7a 100644
--- a/dim.rst
+++ b/dim.rst
@@ -31,7 +31,7 @@ Used to maintain drm-intel_ and drm-misc_ git repositories.
 QUICKSTART
 ==
 
-For getting started grab the latest drm (drm-intel-maintainer) script from::
+For getting started grab the latest dim (drm-intel-maintainer) script from::
 
 http://cgit.freedesktop.org/drm-intel/tree/dim?h=maintainer-tools
 
@@ -56,7 +56,7 @@ you have improvements for dim, please submit them to 
intel-gfx.
 
 If you have a freedesktop.org account and plan to push things on one of the
 drm-xxx repos, you should use the ssh://git.freedesktop.org/git/drm-xxx urls
-when adding a remote and, if it's not already done, you should add new entry in
+when adding a remote and, if it's not already done, you should add a new entry 
in
 ~/.ssh/config::
 
 $ printf '\nHost git.freedesktop.org\n\tUser ' >> ~/.ssh/config
@@ -200,7 +200,7 @@ apply-queued [*git am arguments*]
 
 extract-tags *branch* [*git-rangeish*]
 --
-This extracts various tags (eg. Reviwed-by:) from emails and applies them to 
the
+This extracts various tags (e.g. Reviewed-by:) from emails and applies them to 
the
 top commit on the given branch. You can give the command a rangeish to add the
 tags from the same email to multiple already applied patches.
 
@@ -277,12 +277,12 @@ add-link-queued
 magic-rebase-resolve
 
 Tries to resolve a rebase conflict by first resetting the tree
-and the using the magic patch tool. Then builds the tree, adds
+and then using the magic patch tool. Then builds the tree, adds
 any changes with git add -u and continues the rebase.
 
 apply-resolved
 --
-Compile-test the current tree and if successfully resolve a
+Compile-test the current tree and if successful resolve a
 conflicted git am. Also runs the patch checker afterwards. This fails to add 
the
 Link: tag, so you'll need to add it manually or use the **add-link** 
subcommand.
 
@@ -292,7 +292,7 @@ Create a new topic branch with the given name. Note that 
topic/ is not
 automatically prepended. The branch starts at HEAD or the given commit-ish. 
Note
 that by default the new branch is created in the drm-intel.git repository. If
 you want to create a branch somewhere else, then you need to prepend the remote
-name from nigthly.conf, e.g. "drm-misc/topic/branch".
+name from nightly.conf, e.g. "drm-misc/topic/branch".
 
 remove-branch *branch*
 --
@@ -413,7 +413,7 @@ Show this help. Install **rst2man(1)** for best results.
 
 usage
 -
-Short form usage help listening all subcommands. Run by default or if an 
unknown
+Short form usage help listing all subcommands. Run by default or if an unknown
 subcommand was passed on the cmdline.
 
 ENVIRONMENT
diff --git a/drm-intel.rst b/drm-intel.rst
index 9be1f086e925..c9c8812dc254 100644
--- a/drm-intel.rst
+++ b/drm-intel.rst
@@ -87,7 +87,7 @@ This is the branch where all new features, as well as any 
non-trivial or
 controversial fixes, are applied.
 
 This branch "hides" the merge window from the drm/i915 developers; patches are
-applied here regardless of the development phase of the Linus' upstream kernel.
+applied here regardless of the development phase of Linus' upstream kernel.
 
 drm-intel-next
 ~~
@@ -220,7 +220,7 @@ Signed-off-by: line in the commit message:
 Resolving Conflicts when Rebuilding drm-tip
 ===
 
-When you push patches with dim drm-tip always gets rebuild and this can
+When you push patches with dim drm-tip always gets rebuilt and this can
 sometimes fail, for example like this: ::
 
 Updating rerere cache and nightly.conf... Done.
@@ -263,7 +263,7 @@ when it's tricky or something fails in the below procedure.
 $ git commit -a
 
git will then store the conflict resolution internally (see git help rerere
-   for how this is implemented). Then re-run -nigthly generation to confirm the
+   for how this is implemented). Then re-run drm-tip generation to confirm the
resolution has been captured correctly by git (sometimes git rerere can't
match up your resolution with the conflict for odd reasons) and to make sure
there's no other conflict in later merges: ::
@@ -457,7 +457,7 @@ stakeholders. There's three components for that:
   domain experts, maybe maintainers. Also include maintainers and reviewers of
   the userspace component for new ABI, which often means non-Intel people. In
   case of doubt ask maintainers for a reasonable list of people. Make sure you
-  gather their input actively, don't expect 

Re: [Intel-gfx] [PATCH v2 08/40] drm: Add a simple prime number generator

[Lukas Wunner]

On Fri, Dec 16, 2016 at 09:43:54AM +, Chris Wilson wrote:
> On Fri, Dec 16, 2016 at 10:31:17AM +0100, Lukas Wunner wrote:
> > On Fri, Dec 16, 2016 at 07:46:46AM +, Chris Wilson wrote:
> > > Prime numbers are interesting for testing components that use multiplies
> > > and divides, such as testing struct drm_mm alignment computations.
> > > 
> > > Signed-off-by: Chris Wilson <ch...@chris-wilson.co.uk>
> > > ---
> > >  drivers/gpu/drm/Kconfig |   4 +
> > >  drivers/gpu/drm/Makefile|   1 +
> > >  drivers/gpu/drm/lib/drm_prime_numbers.c | 175 
> > > 
> > >  drivers/gpu/drm/lib/drm_prime_numbers.h |  10 ++
> > >  4 files changed, 190 insertions(+)
> > >  create mode 100644 drivers/gpu/drm/lib/drm_prime_numbers.c
> > >  create mode 100644 drivers/gpu/drm/lib/drm_prime_numbers.h
> > 
> > Hm, why not put this in lib/ ?  Don't see anything DRM-specific here
> > at first glance and this might be useful to others.  Or others might
> > come up with improvements and they'll be more likely to discover it
> > outside of DRM.
> 
> Because that is a 3+ month cycle before I can then apply the testcases,
> and without the testscases do you want the bugfixes?

Do patches for lib/ have to go through a different tree?
Don't think so, I've seen e.g. changes to lib/ucs2_string.c
go through the EFI tree.  It seems to me lib/ is sort of
free for all.


> If I put in in drm/lib then lift it, I can use it immediately and drop
> the local copy once merged.

That is also workable of course.  Anyway, it was just a suggestion.

Thanks,

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

Re: [Intel-gfx] [PATCH v2 08/40] drm: Add a simple prime number generator

[Lukas Wunner]

On Fri, Dec 16, 2016 at 07:46:46AM +, Chris Wilson wrote:
> Prime numbers are interesting for testing components that use multiplies
> and divides, such as testing struct drm_mm alignment computations.
> 
> Signed-off-by: Chris Wilson 
> ---
>  drivers/gpu/drm/Kconfig |   4 +
>  drivers/gpu/drm/Makefile|   1 +
>  drivers/gpu/drm/lib/drm_prime_numbers.c | 175 
> 
>  drivers/gpu/drm/lib/drm_prime_numbers.h |  10 ++
>  4 files changed, 190 insertions(+)
>  create mode 100644 drivers/gpu/drm/lib/drm_prime_numbers.c
>  create mode 100644 drivers/gpu/drm/lib/drm_prime_numbers.h

Hm, why not put this in lib/ ?  Don't see anything DRM-specific here
at first glance and this might be useful to others.  Or others might
come up with improvements and they'll be more likely to discover it
outside of DRM.

Same for the random permutations patch.

Thanks,

Lukas

> 
> diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
> index 2e6ae95459e4..93895898d596 100644
> --- a/drivers/gpu/drm/Kconfig
> +++ b/drivers/gpu/drm/Kconfig
> @@ -53,6 +53,7 @@ config DRM_DEBUG_MM_SELFTEST
>   depends on DRM
>   depends on DEBUG_KERNEL
>   select DRM_LIB_RANDOM
> + select DRM_LIB_PRIMES
>   default n
>   help
> This option provides a kernel module that can be used to test
> @@ -340,3 +341,6 @@ config DRM_LIB_RANDOM
>   bool
>   default n
>  
> +config DRM_LIB_PRIMES
> + bool
> + default n
> diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile
> index 0fa16275fdae..bbd390fa8914 100644
> --- a/drivers/gpu/drm/Makefile
> +++ b/drivers/gpu/drm/Makefile
> @@ -19,6 +19,7 @@ drm-y   :=  drm_auth.o drm_bufs.o drm_cache.o \
>   drm_dumb_buffers.o drm_mode_config.o
>  
>  drm-$(CONFIG_DRM_LIB_RANDOM) += lib/drm_random.o
> +obj-$(CONFIG_DRM_LIB_PRIMES) += lib/drm_prime_numbers.o
>  obj-$(CONFIG_DRM_DEBUG_MM_SELFTEST) += selftests/test-drm_mm.o
>  
>  drm-$(CONFIG_COMPAT) += drm_ioc32.o
> diff --git a/drivers/gpu/drm/lib/drm_prime_numbers.c 
> b/drivers/gpu/drm/lib/drm_prime_numbers.c
> new file mode 100644
> index ..839563d9b787
> --- /dev/null
> +++ b/drivers/gpu/drm/lib/drm_prime_numbers.c
> @@ -0,0 +1,175 @@
> +#include 
> +#include 
> +#include 
> +
> +#include "drm_prime_numbers.h"
> +
> +static DEFINE_MUTEX(lock);
> +
> +static struct primes {
> + struct rcu_head rcu;
> + unsigned long last, sz;
> + unsigned long primes[];
> +} __rcu *primes;
> +
> +static bool slow_is_prime_number(unsigned long x)
> +{
> + unsigned long y = int_sqrt(x) + 1;
> +
> + while (y > 1) {
> + if ((x % y) == 0)
> + break;
> + y--;
> + }
> +
> + return y == 1;
> +}
> +
> +static unsigned long slow_next_prime_number(unsigned long x)
> +{
> + for (;;) {
> + if (slow_is_prime_number(++x))
> + return x;
> + }
> +}
> +
> +static unsigned long mark_multiples(unsigned long x,
> + unsigned long *p,
> + unsigned long start,
> + unsigned long end)
> +{
> + unsigned long m;
> +
> + m = 2 * x;
> + if (m < start)
> + m = (start / x + 1) * x;
> +
> + while (m < end) {
> + __clear_bit(m, p);
> + m += x;
> + }
> +
> + return x;
> +}
> +
> +static struct primes *expand(unsigned long x)
> +{
> + unsigned long sz, y, prev;
> + struct primes *p, *new;
> +
> + sz = x * x;
> + if (sz < x)
> + return NULL;
> +
> + mutex_lock();
> + p = rcu_dereference_protected(primes, lockdep_is_held());
> + if (p && x < p->last)
> + goto unlock;
> +
> + sz = round_up(sz, BITS_PER_LONG);
> + new = kmalloc(sizeof(*new) + sz / sizeof(long), GFP_KERNEL);
> + if (!new) {
> + p = NULL;
> + goto unlock;
> + }
> +
> + /* Where memory permits, track the primes using the
> +  * Sieve of Eratosthenes.
> +  */
> + if (p) {
> + prev = p->sz;
> + memcpy(new->primes, p->primes, prev / BITS_PER_LONG);
> + } else {
> + prev = 0;
> + }
> + memset(new->primes + prev / BITS_PER_LONG,
> +0xff, (sz - prev) / sizeof(long));
> + for (y = 2UL; y < sz; y = find_next_bit(new->primes, sz, y + 1))
> + new->last = mark_multiples(y, new->primes, prev, sz);
> + new->sz = sz;
> +
> + rcu_assign_pointer(primes, new);
> + if (p)
> + kfree_rcu(p, rcu);
> + p = new;
> +
> +unlock:
> + mutex_unlock();
> + return p;
> +}
> +
> +unsigned long drm_next_prime_number(unsigned long x)
> +{
> + struct primes *p;
> +
> + if (x < 2)
> + return 2;
> +
> + rcu_read_lock();
> + p = rcu_dereference(primes);
> + if (!p || x >= p->last) {
> + 

Re: [Intel-gfx] S4 resume breakage with i915 driver

[Lukas Wunner]

On Mon, Aug 29, 2016 at 04:25:25PM +0100, Chris Wilson wrote:
> On Mon, Aug 29, 2016 at 05:54:45PM +0300, Imre Deak wrote:
> > On ma, 2016-08-29 at 16:24 +0200, Takashi Iwai wrote:
> > > Hmm, this always confuses me.  Is the freeze callback called to the
> > > loader kernel?
> > 
> > It's called both in loader and target kernel, before creating or
> > restoring the image.
> 
> So the right answer for hiberation is?

According to Documentation/power/devices.txt:

"After the image has been loaded, the devices managed by the boot kernel
need to be prepared for passing control back to the image kernel.  This
is very similar to the initial steps involved in creating a system image,
and it is accomplished in the same way, using prepare, freeze, and
freeze_noirq phases.  However the devices affected by these phases are
only those having drivers in the boot kernel; other devices will still be
in whatever state the boot loader left them."

"Most often the pre-hibernation memory contents are restored successfully
and control is passed to the image kernel, which then becomes responsible
for bringing the system back to the working state.
To achieve this, the image kernel must restore the devices' pre-hibernation
functionality.  The operation is much like waking up from the memory sleep
state, although it involves different phases:
restore_noirq, restore_early, restore, complete"


If a missing i915.ko in the boot kernel's initrd causes problems, perhaps
this can be solved by amending intel_graphics_quirks() to reset the GPU?

Best regards,

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

Re: [Intel-gfx] [PATCH] drm/i915: Shut down displays gracefully on reboot

[Lukas Wunner]

On Wed, Aug 03, 2016 at 04:36:18PM +0300, ville.syrj...@linux.intel.com wrote:
> From: Ville Syrjälä 
> 
> Dell UP2414Q doesn't like it when we're driving it in MST mode, and then
> reboot the machine. After reboot, the monitor is somehow confused and
> refuses to do the payload allocation:
>  [drm:drm_dp_mst_allocate_vcpi] initing vcpi for 282 8
>  [drm:drm_dp_dpcd_write_payload] status not set after read payload table 
> status 0
> and thus we're left with a black screen until the monitor power is
> toggled.
> 
> It seems that if we shut down things gracefully prior to rebooting, the
> monitor doesn't get confused like this. So let's try to shut down all
> the displays gracefully on reboot. The downside is that we will
> introduce the reboot notifier to all the modesetl locks. So if there's
> a locking bug around, we might not be able to reboot the machine
> gracefully. sysrq reboot will still work though, as it will not
> call the notifiers.

struct pci_driver has a ->shutdown hook which is currently not defined
in i915_pci_driver. How about using that instead of reboot_notifier
to avoid potential locking issues?

Best regards,

Lukas

> 
> While we do this, we can eliminate the eDP reboot notifier, which is
> there to shut down the panel power and make sure the firmware won't
> violate the panel power cycle delay post-reboot. Since we're now
> shutting eDP down properly, we can mostly just rip out the eDP notifier.
> We still need to do the panel power cycle wait though, as that would
> normally get postponed until the next modeset. So let's move that part
> into intel_panel so that other display types will get the same treatment
> as a bonus.
> 
> The Dell UP2414Q can often get even more confused, and sometimes what
> you have to do is: switch to another input on the monitor, toggle the
> monitor power, switch the input back to the original setting. And
> sometimes it seems you just have to yank the power cable entirely. I'm
> not sure if this reboot notifier might avoid some of these other
> failure modes as well, but I'm pretty sure it can't hurt at least.
> 
> Cc: Clint Taylor 
> Cc: Paulo Zanoni 
> Signed-off-by: Ville Syrjälä 
> ---
>  drivers/gpu/drm/i915/i915_drv.h  |  2 ++
>  drivers/gpu/drm/i915/intel_display.c | 24 ++
>  drivers/gpu/drm/i915/intel_dp.c  | 49 
> ++--
>  drivers/gpu/drm/i915/intel_drv.h |  7 +++---
>  drivers/gpu/drm/i915/intel_dsi.c |  3 ++-
>  drivers/gpu/drm/i915/intel_dvo.c |  2 +-
>  drivers/gpu/drm/i915/intel_lvds.c|  3 ++-
>  drivers/gpu/drm/i915/intel_panel.c   | 29 -
>  8 files changed, 65 insertions(+), 54 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 65ada5d2f88c..f8eb8c7de007 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -1727,6 +1727,8 @@ struct intel_wm_config {
>  struct drm_i915_private {
>   struct drm_device drm;
>  
> + struct notifier_block reboot_notifier;
> +
>   struct kmem_cache *objects;
>   struct kmem_cache *vmas;
>   struct kmem_cache *requests;
> diff --git a/drivers/gpu/drm/i915/intel_display.c 
> b/drivers/gpu/drm/i915/intel_display.c
> index a8e8cc8dfae9..2192a21aa6c9 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -31,6 +31,8 @@
>  #include 
>  #include 
>  #include 
> +#include 
> +#include 
>  #include 
>  #include 
>  #include "intel_drv.h"
> @@ -15578,6 +15580,23 @@ fail:
>   drm_modeset_acquire_fini();
>  }
>  
> +
> +static int intel_reboot_notifier(struct notifier_block *nb,
> +  unsigned long code, void *unused)
> +{
> + struct drm_i915_private *dev_priv =
> + container_of(nb, typeof(*dev_priv), reboot_notifier);
> +
> + if (code != SYS_RESTART)
> + return 0;
> +
> + intel_display_suspend(_priv->drm);
> +
> + intel_panel_reboot_notifier(dev_priv);
> +
> + return 0;
> +}
> +
>  void intel_modeset_init(struct drm_device *dev)
>  {
>   struct drm_i915_private *dev_priv = to_i915(dev);
> @@ -15706,6 +15725,9 @@ void intel_modeset_init(struct drm_device *dev)
>* since the watermark calculation done here will use pstate->fb.
>*/
>   sanitize_watermarks(dev);
> +
> + dev_priv->reboot_notifier.notifier_call = intel_reboot_notifier;
> + register_reboot_notifier(_priv->reboot_notifier);
>  }
>  
>  static void intel_enable_pipe_a(struct drm_device *dev)
> @@ -16291,6 +16313,8 @@ void intel_modeset_cleanup(struct drm_device *dev)
>  {
>   struct drm_i915_private *dev_priv = to_i915(dev);
>  
> + unregister_reboot_notifier(_priv->reboot_notifier);
> +
>   intel_disable_gt_powersave(dev_priv);
>  
>   /*
> diff --git 

Re: [Intel-gfx] [PATCH 0197/1285] Replace numeric parameter like 0444 with macro

[Lukas Wunner]

On Tue, Aug 02, 2016 at 02:37:37PM +0300, Ville Syrjälä wrote:
> On Tue, Aug 02, 2016 at 06:48:47PM +0800, Baole Ni wrote:
> > I find that the developers often just specified the numeric value
> > when calling a macro which is defined with a parameter for access 
> > permission.
> > As we know, these numeric value for access permission have had the 
> > corresponding macro,
> > and that using macro can improve the robustness and readability of the code,
> > thus, I suggest replacing the numeric parameter with the macro.
> > 
> > Signed-off-by: Chuansheng Liu 
> > Signed-off-by: Baole Ni 
> > ---
> >  drivers/gpu/drm/i915/i915_params.c | 64 
> > +++---
> >  1 file changed, 32 insertions(+), 32 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_params.c 
> > b/drivers/gpu/drm/i915/i915_params.c
> > index 1779f02..7184e06 100644
> > --- a/drivers/gpu/drm/i915/i915_params.c
> > +++ b/drivers/gpu/drm/i915/i915_params.c
> > @@ -60,22 +60,22 @@ struct i915_params i915 __read_mostly = {
> > .inject_load_failure = 0,
> >  };
> >  
> > -module_param_named(modeset, i915.modeset, int, 0400);
> > +module_param_named(modeset, i915.modeset, int, S_IRUSR);
> 
> At least I can't read those macros. Octal is much clearer IMO.

It's also easier to grep for, say, 644, rather than formulating a
regex with all possible ordering permutations of these macros.

Best regards,

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

Re: [Intel-gfx] [PATCH] drm/i915: intel_dp_check_link_status should only return status of link

[Lukas Wunner]

On Fri, Jul 01, 2016 at 03:47:56PM -0700, Manasi Navare wrote:
> Intel_dp_check_link_status() function reads the Link status registers
> and returns a boolean to indicate link is good or bad.
> If the link is bad, it is retrained outside the function based
> on the return value.
> 
> Signed-off-by: Manasi Navare 
> ---
>  drivers/gpu/drm/i915/intel_dp.c | 41 
> -
>  1 file changed, 24 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index 6d586b7..c795c9e 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -3863,7 +3863,7 @@ go_again:
>   return -EINVAL;
>  }
>  
> -static void
> +static bool
>  intel_dp_check_link_status(struct intel_dp *intel_dp)
>  {

A common pattern is to name bool functions like a yes/no question,
so you might want to adjust the name to something like
intel_dp_link_status_ok() or intel_dp_link_is_good() here.

Best regards,

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

Re: [Intel-gfx] [PATCH 3/3] drm/i915/fbdev: Flush mode configuration before lastclose

[Lukas Wunner]

On Fri, Jun 17, 2016 at 06:54:49PM +0100, Chris Wilson wrote:
> During lastclose, we call intel_fbdev_restore_mode() to switch back to
> the fbcon configuration on return to VT. However, if we have not yet
> finished the asynchronous fbdev initialisation, the current mode will be
> invalid and trigger WARNs upon application.
> 
> Serialise with the outstanding initialisation if the first application
> exits quickly. Note that to hit this in practice requires using an
> unregistered async_domain as otherwise modprobe will force a full
> synchronisation prior to init() completing.
> 
> Signed-off-by: Chris Wilson 

Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=93580
Reported-by: Gustav Fägerlind 
Reported-by: "Li, Weinan Z" 
Cc: sta...@vger.kernel.org

> ---
>  drivers/gpu/drm/i915/intel_fbdev.c | 5 +
>  1 file changed, 5 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/intel_fbdev.c 
> b/drivers/gpu/drm/i915/intel_fbdev.c
> index 638e420a59cb..a19d621d8815 100644
> --- a/drivers/gpu/drm/i915/intel_fbdev.c
> +++ b/drivers/gpu/drm/i915/intel_fbdev.c
> @@ -830,6 +830,11 @@ void intel_fbdev_restore_mode(struct drm_device *dev)
>   if (!ifbdev)
>   return;
>  
> + if (ifbdev->cookie) {
> + async_synchronize_cookie(ifbdev->cookie + 1);
> + ifbdev->cookie = 0;
> + }
> +
>   fb_helper = >helper;
>  
>   ret = drm_fb_helper_restore_fbdev_mode_unlocked(fb_helper);
> -- 
> 2.8.1
> 
> ___
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH] drm/i915: Add a module option for disabling use of stolen memory

[Lukas Wunner]

On Sat, Jun 11, 2016 at 09:21:11AM +0100, Chris Wilson wrote:
> One of the first steps in triaging memory corruption on module load is
> to disable stolen. (It helps reduce the impact of the BIOS clobbering
> our memory since we allocate our ringbuffers and initial objects from
> stolen, if they are clobbered we get an immediate hang and garbage on
> screen.) Rather than requesting bug reporters patch their kernel to test
> the theory, allow us to disable stolen through a module option.

Just a quick drive-by observation, intelfb_alloc() calls
i915_gem_object_create_stolen() and if I follow the call chain
in that function it seems to me that dev_priv->mm.stolen_base == 0
isn't checked anywhere, so this looks like something that might break
with i915.enable_stolen=0. But probably that's what you meant with
"secondary effects".

Best regards,

Lukas

> 
> Signed-off-by: Chris Wilson 
> ---
>  drivers/gpu/drm/i915/i915_gem_stolen.c | 5 +
>  drivers/gpu/drm/i915/i915_params.c | 6 ++
>  drivers/gpu/drm/i915/i915_params.h | 1 +
>  3 files changed, 12 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem_stolen.c 
> b/drivers/gpu/drm/i915/i915_gem_stolen.c
> index e9cd82290408..bb8567b1d6a4 100644
> --- a/drivers/gpu/drm/i915/i915_gem_stolen.c
> +++ b/drivers/gpu/drm/i915/i915_gem_stolen.c
> @@ -411,6 +411,11 @@ int i915_gem_init_stolen(struct drm_device *dev)
>   }
>  #endif
>  
> + if (!i915.enable_stolen) {
> + DRM_INFO("Disabling use of stolen memory at user request.\n");
> + return 0;
> + }
> +
>   if (ggtt->stolen_size == 0)
>   return 0;
>  
> diff --git a/drivers/gpu/drm/i915/i915_params.c 
> b/drivers/gpu/drm/i915/i915_params.c
> index 573e78723fc5..511528199aec 100644
> --- a/drivers/gpu/drm/i915/i915_params.c
> +++ b/drivers/gpu/drm/i915/i915_params.c
> @@ -60,6 +60,7 @@ struct i915_params i915 __read_mostly = {
>   .enable_dp_mst = true,
>   .inject_load_failure = 0,
>   .enable_dpcd_backlight = false,
> + .enable_stolen = true,
>  };
>  
>  module_param_named(modeset, i915.modeset, int, 0400);
> @@ -119,6 +120,11 @@ MODULE_PARM_DESC(enable_hangcheck,
>   "WARNING: Disabling this can cause system wide hangs. "
>   "(default: true)");
>  
> +module_param_named(enable_stolen, i915.enable_stolen, bool, 0400);
> +MODULE_PARM_DESC(enable_stolen,
> + "Enable use of memory reserved (stolen) by the BIOS for graphics. "
> + "(default: true)");
> +
>  module_param_named_unsafe(enable_ppgtt, i915.enable_ppgtt, int, 0400);
>  MODULE_PARM_DESC(enable_ppgtt,
>   "Override PPGTT usage. "
> diff --git a/drivers/gpu/drm/i915/i915_params.h 
> b/drivers/gpu/drm/i915/i915_params.h
> index 1323261a0cdd..f126decae848 100644
> --- a/drivers/gpu/drm/i915/i915_params.h
> +++ b/drivers/gpu/drm/i915/i915_params.h
> @@ -63,6 +63,7 @@ struct i915_params {
>   bool nuclear_pageflip;
>   bool enable_dp_mst;
>   bool enable_dpcd_backlight;
> + bool enable_stolen;
>  };
>  
>  extern struct i915_params i915 __read_mostly;
> -- 
> 2.8.1
> 
> ___
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH] drm/i915: Don't unregister fbdev twice

[Lukas Wunner]

On Wed, Jun 08, 2016 at 02:09:40PM +0200, Daniel Vetter wrote:
> On Wed, Jun 08, 2016 at 01:15:22PM +0200, Lukas Wunner wrote:
> > Calling drm_framebuffer_unregister_private() in intel_fbdev_destroy() is
> > superfluous because the framebuffer will subsequently be unregistered by
> > drm_framebuffer_free() when unreferenced in drm_framebuffer_remove().
> > The call is a leftover, when it was introduced by commit 362063619cf6
> > ("drm: revamp framebuffer cleanup interfaces"), struct intel_framebuffer
> > was still embedded in struct intel_fbdev rather than being a pointer as
> > it is today, and drm_framebuffer_remove() wasn't used yet.
> > 
> > As a bonus, the ID of the framebuffer is no longer 0 in the debug log:
> > 
> > Before:
> > [   39.680874] [drm:drm_mode_object_unreference] OBJ ID: 0 (3)
> > [   39.680878] [drm:drm_mode_object_unreference] OBJ ID: 0 (2)
> > [   39.680884] [drm:drm_mode_object_unreference] OBJ ID: 0 (1)
> > 
> > After:
> > [  102.504649] [drm:drm_mode_object_unreference] OBJ ID: 45 (3)
> > [  102.504651] [drm:drm_mode_object_unreference] OBJ ID: 45 (2)
> > [  102.504654] [drm:drm_mode_object_unreference] OBJ ID: 45 (1)
> > 
> > Signed-off-by: Lukas Wunner <lu...@wunner.de>
> 
> Hm yeah. But there's a pile of that particaluar cargo-culting copied all
> over the place, would be really good to audit all callers of
> unregister_private and fix them all up. A few older drivers still embed
> the fbdev fb, but most don't but still use the unregister_private +
> cleanup combo.

Yes, I noticed that but i915 was the only one that I could actually test,
the others I can only compile test. So fixing those up requires very
careful examination and takes more time, but I'll keep it on my todo list.


> Nitpick in your subject: s/fbdev/fbdev's fb/

Right, should I post a v2 or are you going to fix it up if/when merging?

Thanks,

Lukas

> 
> > ---
> >  drivers/gpu/drm/i915/intel_fbdev.c | 2 --
> >  1 file changed, 2 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_fbdev.c 
> > b/drivers/gpu/drm/i915/intel_fbdev.c
> > index ef8e676..4c7ea46 100644
> > --- a/drivers/gpu/drm/i915/intel_fbdev.c
> > +++ b/drivers/gpu/drm/i915/intel_fbdev.c
> > @@ -552,8 +552,6 @@ static void intel_fbdev_destroy(struct drm_device *dev,
> > drm_fb_helper_fini(>helper);
> >  
> > if (ifbdev->fb) {
> > -   drm_framebuffer_unregister_private(>fb->base);
> > -
> > mutex_lock(>struct_mutex);
> > intel_unpin_fb_obj(>fb->base, BIT(DRM_ROTATE_0));
> > mutex_unlock(>struct_mutex);
> > -- 
> > 2.8.1
> > 
> > ___
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [Nouveau] [PATCH 9/9] drm: Turn off crtc before tearing down its data structure

[Lukas Wunner]

On Fri, Jun 03, 2016 at 08:21:50PM +0200, Daniel Vetter wrote:
> On Fri, Jun 03, 2016 at 09:30:06AM +0200, Lukas Wunner wrote:
> > On Wed, Jun 01, 2016 at 04:40:12PM +0200, Daniel Vetter wrote:
> > > On Wed, Jun 01, 2016 at 02:36:41PM +0200, Lukas Wunner wrote:
> > > > On Wed, May 25, 2016 at 03:43:42PM +0200, Daniel Vetter wrote:
> > > > > On Wed, May 25, 2016 at 12:51 PM, Lukas Wunner <lu...@wunner.de> 
> > > > > wrote:
> > > > > > On Tue, May 24, 2016 at 11:30:42PM +0200, Daniel Vetter wrote:
> > > > > > > On Tue, May 24, 2016 at 06:03:27PM +0200, Lukas Wunner wrote:
> > > > > > > > When a drm_crtc structure is destroyed with drm_crtc_cleanup(), 
> > > > > > > > the DRM
> > > > > > > > core does not turn off the crtc first and neither do the 
> > > > > > > > drivers. With
> > > > > > > > nouveau, radeon and amdgpu, this causes a runtime pm ref to be 
> > > > > > > > leaked on
> > > > > > > > driver unload if at least one crtc was enabled.
> > > > > > > >
> > > > > > > > (See usage of have_disp_power_ref in nouveau_crtc_set_config(),
> > > > > > > > radeon_crtc_set_config() and amdgpu_crtc_set_config()).
> > > > > > > >
> > > > > > > > Fixes: 5addcf0a5f0f ("nouveau: add runtime PM support (v0.9)")
> > > > > > > > Cc: Dave Airlie <airl...@redhat.com>
> > > > > > > > Tested-by: Karol Herbst <nouv...@karolherbst.de>
> > > > > > > > Signed-off-by: Lukas Wunner <lu...@wunner.de>
> > > > > 
> > > > > With legacy kms the only way to keep a crtc enabled is to display a
> > > > > drm_framebuffer on it. And drm_mode_config_cleanup has a WARN_ON if
> > > > > framebuffers are left behind. There's a bunch of options:
> > > > > - nouveau somehow manages to keep the crtc on without a framebuffer
> > > > > - nouveau somehow leaks a drm_framebuffer, but removes it from the 
> > > > > fb_list
> > > > > - something else
> > > > 
> > > > Found it. nouveau_fbcon_destroy() doesn't call drm_framebuffer_remove().
> > > > If I add that, the crtc gets properly disabled on unload.
> > > > 
> > > > It does call drm_framebuffer_cleanup(). That's why there was no WARN,
> > > > drm_mode_config_cleanup() only WARNs if a framebuffer was left on the
> > > > mode_config.fb_list.
> > > > 
> > > > radeon and amdgpu have the same problem. In fact there are very few
> > > > drivers that call drm_framebuffer_remove(): tegra, msm, exynos, omapdrm
> > > > and i915 (since Imre Deak's 9d6612516da0).
> > > > 
> > > > Should we add a WARN to prevent this? How about WARN_ON(crtc->enabled)
> > > > in drm_crtc_cleanup()?
> > > > 
> > > > Also, i915 calls drm_framebuffer_unregister_private() before it calls
> > > > drm_framebuffer_remove(). This ordering has the unfortunate side effect
> > > > that the drm_framebuffer has ID 0 in log messages emitted by
> > > > drm_framebuffer_remove():
> > > > 
> > > > [   39.680874] [drm:drm_mode_object_unreference] OBJ ID: 0 (3)
> > > > [   39.680878] [drm:drm_mode_object_unreference] OBJ ID: 0 (2)
> > > > [   39.680884] [drm:drm_mode_object_unreference] OBJ ID: 0 (1)
> > > 
> > > Well we must first unregister it before we can remove it, so this is
> > > unavoidable.
> > 
> > Yes but drm_framebuffer_free() calls drm_mode_object_unregister()
> > and is invoked by drm_framebuffer_remove(), so the additional call to
> > drm_framebuffer_unregister_private() in intel_fbdev_destroy() seems
> > superfluous. Or is there some reason I'm missing that this needs to
> > be called before intel_unpin_fb_obj()?
> > 
> > 
> > > Wrt switching from _cleanup to _remove, iirc there was troubles with the
> > > later calling into the fb->funcs->destroy hook. But many drivers have
> > > their fbdev fb embedded into some struct (instead of a pointer like i915),
> > > and then things go sideways badly. That's why you can't just blindly
> > > replace them.
> > 
> > So the options seem to be:
> > 
> > (1) Refactor nouveau, radeon and amdgpu to not embed their framebuffer
> > struct in their fbdev struct, so t

[Intel-gfx] [PATCH] drm/i915: Don't unregister fbdev twice

[Lukas Wunner]

Calling drm_framebuffer_unregister_private() in intel_fbdev_destroy() is
superfluous because the framebuffer will subsequently be unregistered by
drm_framebuffer_free() when unreferenced in drm_framebuffer_remove().
The call is a leftover, when it was introduced by commit 362063619cf6
("drm: revamp framebuffer cleanup interfaces"), struct intel_framebuffer
was still embedded in struct intel_fbdev rather than being a pointer as
it is today, and drm_framebuffer_remove() wasn't used yet.

As a bonus, the ID of the framebuffer is no longer 0 in the debug log:

Before:
[   39.680874] [drm:drm_mode_object_unreference] OBJ ID: 0 (3)
[   39.680878] [drm:drm_mode_object_unreference] OBJ ID: 0 (2)
[   39.680884] [drm:drm_mode_object_unreference] OBJ ID: 0 (1)

After:
[  102.504649] [drm:drm_mode_object_unreference] OBJ ID: 45 (3)
[  102.504651] [drm:drm_mode_object_unreference] OBJ ID: 45 (2)
[  102.504654] [drm:drm_mode_object_unreference] OBJ ID: 45 (1)

Signed-off-by: Lukas Wunner <lu...@wunner.de>
---
 drivers/gpu/drm/i915/intel_fbdev.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_fbdev.c 
b/drivers/gpu/drm/i915/intel_fbdev.c
index ef8e676..4c7ea46 100644
--- a/drivers/gpu/drm/i915/intel_fbdev.c
+++ b/drivers/gpu/drm/i915/intel_fbdev.c
@@ -552,8 +552,6 @@ static void intel_fbdev_destroy(struct drm_device *dev,
drm_fb_helper_fini(>helper);
 
if (ifbdev->fb) {
-   drm_framebuffer_unregister_private(>fb->base);
-
mutex_lock(>struct_mutex);
intel_unpin_fb_obj(>fb->base, BIT(DRM_ROTATE_0));
mutex_unlock(>struct_mutex);
-- 
2.8.1

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

Re: [Intel-gfx] [Nouveau] [PATCH 9/9] drm: Turn off crtc before tearing down its data structure

[Lukas Wunner]

On Wed, Jun 01, 2016 at 04:40:12PM +0200, Daniel Vetter wrote:
> On Wed, Jun 01, 2016 at 02:36:41PM +0200, Lukas Wunner wrote:
> > On Wed, May 25, 2016 at 03:43:42PM +0200, Daniel Vetter wrote:
> > > On Wed, May 25, 2016 at 12:51 PM, Lukas Wunner <lu...@wunner.de> wrote:
> > > > On Tue, May 24, 2016 at 11:30:42PM +0200, Daniel Vetter wrote:
> > > > > On Tue, May 24, 2016 at 06:03:27PM +0200, Lukas Wunner wrote:
> > > > > > When a drm_crtc structure is destroyed with drm_crtc_cleanup(), the 
> > > > > > DRM
> > > > > > core does not turn off the crtc first and neither do the drivers. 
> > > > > > With
> > > > > > nouveau, radeon and amdgpu, this causes a runtime pm ref to be 
> > > > > > leaked on
> > > > > > driver unload if at least one crtc was enabled.
> > > > > >
> > > > > > (See usage of have_disp_power_ref in nouveau_crtc_set_config(),
> > > > > > radeon_crtc_set_config() and amdgpu_crtc_set_config()).
> > > > > >
> > > > > > Fixes: 5addcf0a5f0f ("nouveau: add runtime PM support (v0.9)")
> > > > > > Cc: Dave Airlie <airl...@redhat.com>
> > > > > > Tested-by: Karol Herbst <nouv...@karolherbst.de>
> > > > > > Signed-off-by: Lukas Wunner <lu...@wunner.de>
> > > 
> > > With legacy kms the only way to keep a crtc enabled is to display a
> > > drm_framebuffer on it. And drm_mode_config_cleanup has a WARN_ON if
> > > framebuffers are left behind. There's a bunch of options:
> > > - nouveau somehow manages to keep the crtc on without a framebuffer
> > > - nouveau somehow leaks a drm_framebuffer, but removes it from the fb_list
> > > - something else
> > 
> > Found it. nouveau_fbcon_destroy() doesn't call drm_framebuffer_remove().
> > If I add that, the crtc gets properly disabled on unload.
> > 
> > It does call drm_framebuffer_cleanup(). That's why there was no WARN,
> > drm_mode_config_cleanup() only WARNs if a framebuffer was left on the
> > mode_config.fb_list.
> > 
> > radeon and amdgpu have the same problem. In fact there are very few
> > drivers that call drm_framebuffer_remove(): tegra, msm, exynos, omapdrm
> > and i915 (since Imre Deak's 9d6612516da0).
> > 
> > Should we add a WARN to prevent this? How about WARN_ON(crtc->enabled)
> > in drm_crtc_cleanup()?
> > 
> > Also, i915 calls drm_framebuffer_unregister_private() before it calls
> > drm_framebuffer_remove(). This ordering has the unfortunate side effect
> > that the drm_framebuffer has ID 0 in log messages emitted by
> > drm_framebuffer_remove():
> > 
> > [   39.680874] [drm:drm_mode_object_unreference] OBJ ID: 0 (3)
> > [   39.680878] [drm:drm_mode_object_unreference] OBJ ID: 0 (2)
> > [   39.680884] [drm:drm_mode_object_unreference] OBJ ID: 0 (1)
> 
> Well we must first unregister it before we can remove it, so this is
> unavoidable.

Yes but drm_framebuffer_free() calls drm_mode_object_unregister()
and is invoked by drm_framebuffer_remove(), so the additional call to
drm_framebuffer_unregister_private() in intel_fbdev_destroy() seems
superfluous. Or is there some reason I'm missing that this needs to
be called before intel_unpin_fb_obj()?


> Wrt switching from _cleanup to _remove, iirc there was troubles with the
> later calling into the fb->funcs->destroy hook. But many drivers have
> their fbdev fb embedded into some struct (instead of a pointer like i915),
> and then things go sideways badly. That's why you can't just blindly
> replace them.

So the options seem to be:

(1) Refactor nouveau, radeon and amdgpu to not embed their framebuffer
struct in their fbdev struct, so that drm_framebuffer_remove() can
be used.

(2) Amend each of them to turn off crtcs which are using the fbdev
framebuffer, duplicating the code in drm_framebuffer_remove().

(3) Split drm_framebuffer_remove(), move the portion to turn off crtcs
into a separate helper, say, drm_framebuffer_deactivate(), call that
from nouveau, radeon and amdgpu.

(4) Go back to square one and use patch [9/9] of this series.

Which one would be most preferred? Is there another solution I've missed?

Thanks,

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

Re: [Intel-gfx] [Nouveau] [PATCH 9/9] drm: Turn off crtc before tearing down its data structure

[Lukas Wunner]

On Wed, May 25, 2016 at 03:43:42PM +0200, Daniel Vetter wrote:
> On Wed, May 25, 2016 at 12:51 PM, Lukas Wunner <lu...@wunner.de> wrote:
> > On Tue, May 24, 2016 at 11:30:42PM +0200, Daniel Vetter wrote:
> >> On Tue, May 24, 2016 at 06:03:27PM +0200, Lukas Wunner wrote:
> >> > When a drm_crtc structure is destroyed with drm_crtc_cleanup(), the DRM
> >> > core does not turn off the crtc first and neither do the drivers. With
> >> > nouveau, radeon and amdgpu, this causes a runtime pm ref to be leaked on
> >> > driver unload if at least one crtc was enabled.
> >> >
> >> > (See usage of have_disp_power_ref in nouveau_crtc_set_config(),
> >> > radeon_crtc_set_config() and amdgpu_crtc_set_config()).
> >> >
> >> > Fixes: 5addcf0a5f0f ("nouveau: add runtime PM support (v0.9)")
> >> > Cc: Dave Airlie <airl...@redhat.com>
> >> > Tested-by: Karol Herbst <nouv...@karolherbst.de>
> >> > Signed-off-by: Lukas Wunner <lu...@wunner.de>
> >>
> >> This is a core regression, we fixed it again. Previously when unreference
> >> drm_planes the core made sure that it's not longer in use, which had the
> >> side effect of shutting everything off in module unload.
> >>
> >> For a bunch of reasons we've stopped doing that, but that turned out to be
> >> a mistake. It's fixed since
> >>
> >> commit f2d580b9a8149735cbc4b59c4a8df60173658140
> >> Author: Maarten Lankhorst <maarten.lankho...@linux.intel.com>
> >> Date:   Wed May 4 14:38:26 2016 +0200
> >>
> >> drm/core: Do not preserve framebuffer on rmfb, v4.
> >>
> >> Your patch shouldn't be needed with that any more. If it still is it's
> >> most likely the fbdev cleanup done too late, but you /should/ get a big
> >> WARNING splat in that case from drm_mode_config_cleanup().
> >
> > I tested it and at least with nouveau, the above-mentioned commit does
> > *not* solve the issue, so patch [9/9] of this series is still needed.
> > I do not get a WARN splat when unloading nouveau.
> 
> With legacy kms the only way to keep a crtc enabled is to display a
> drm_framebuffer on it. And drm_mode_config_cleanup has a WARN_ON if
> framebuffers are left behind. There's a bunch of options:
> - nouveau somehow manages to keep the crtc on without a framebuffer
> - nouveau somehow leaks a drm_framebuffer, but removes it from the fb_list
> - something else

Found it. nouveau_fbcon_destroy() doesn't call drm_framebuffer_remove().
If I add that, the crtc gets properly disabled on unload.

It does call drm_framebuffer_cleanup(). That's why there was no WARN,
drm_mode_config_cleanup() only WARNs if a framebuffer was left on the
mode_config.fb_list.

radeon and amdgpu have the same problem. In fact there are very few
drivers that call drm_framebuffer_remove(): tegra, msm, exynos, omapdrm
and i915 (since Imre Deak's 9d6612516da0).

Should we add a WARN to prevent this? How about WARN_ON(crtc->enabled)
in drm_crtc_cleanup()?

Also, i915 calls drm_framebuffer_unregister_private() before it calls
drm_framebuffer_remove(). This ordering has the unfortunate side effect
that the drm_framebuffer has ID 0 in log messages emitted by
drm_framebuffer_remove():

[   39.680874] [drm:drm_mode_object_unreference] OBJ ID: 0 (3)
[   39.680878] [drm:drm_mode_object_unreference] OBJ ID: 0 (2)
[   39.680884] [drm:drm_mode_object_unreference] OBJ ID: 0 (1)

Best regards,

Lukas

> 
> There's still no need to forcefully shut down crtc at cleanup time in
> the core, this is still a driver bug. So yes your patch might be
> needed, but it's not the right fix.
> -Daniel
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> +41 (0) 79 365 57 48 - http://blog.ffwll.ch
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH v5] vga_switcheroo: Add helper for deferred probing

[Lukas Wunner]

On Mon, May 23, 2016 at 09:23:14AM +0200, Daniel Vetter wrote:
> On Sat, May 21, 2016 at 03:59:16PM +0100, Lukas Wunner wrote:
[snip]
> >  /**
> > + * vga_switcheroo_client_probe_defer() - whether to defer probing a given 
> > client
> > + * @pdev: client pci device
> > + *
> > + * Determine whether any prerequisites are not fulfilled to probe a given
> > + * client. Drivers shall invoke this early on in their ->probe callback
> > + * and return %-EPROBE_DEFER if it evaluates to %true. Thou shalt not
> > + * register the client ere thou hast called this.
> 
> I still think we should just smash this into the relevant _register
> functions(), for safety. But I forgot the actual result of the previous
> discussion ...

I've amended the commit message in v6 to summarize the result of the
previous discussion, which was:

One might be tempted to check deferred probing conditions in
vga_switcheroo_register_client(), but this is usually called fairly late
during driver load. The GPU is fully brought up and ready for switching
at that point. On boot the ->probe hook is potentially called dozens of
times until it finally succeeds, and each time we'd repeat bringup and
teardown of the GPU, lengthening boot time considerably and cluttering
logfiles. A separate helper is therefore needed which can be called
right at the beginning of the ->probe hook.

Thanks,

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

Re: [Intel-gfx] [Nouveau] [PATCH v4] vga_switcheroo: Add helper for deferred probing

[Lukas Wunner]

On Mon, May 23, 2016 at 09:40:59AM +0100, Emil Velikov wrote:
> On 21 May 2016 at 15:08, Lukas Wunner <lu...@wunner.de> wrote:
> > Daniel Vetter explicitly wanted the ability to use the helper in
> > vga_switcheroo audio clients, and those shouldn't run the apple-gmux
> > test. I think it makes sense to enclose it in the above-quoted if-block
> > *now* even though it's not needed. Once someone adds a check for an
> > audio client, chances are they'll forget to add this if-block.
> >
> Absolutely no arguments against any of that. Having a helper makes
> sense even without any of the above arguments - it's a 'nasty looking'
> if statement, duplicated multiple times. Speaking of which ...
> shouldn't there be a similar hunk for amdgpu ?

Currently vga_switcheroo_client_probe_defer() is only needed for the
MacBook Pro and the AMD GPUs used by Apple are only supported by radeon.
I've amended the commit message in v6 to explain that.


> Then again throwing everything into one patch does _not_ make sense.
> Don't know why people would insist on having re-factoring and
> functionality change as a single commit. Esp. in a place where hw
> combinations, and thus chances of things going wrong, are pretty high.

Noone insists, v1 of this patch didn't contain the functional change
you're taking exception to, I added it in v2 on Daniel's request and
didn't want to complicate things further, seemed like a trivial change
to me. Nevertheless I've spun this out into a separate commit now,
no problem at all.

Best regards,

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

[Intel-gfx] [PATCH v6 2/2] vga_switcheroo: Support deferred probing of audio clients

[Lukas Wunner]

Daniel Vetter pointed out that vga_switcheroo_client_probe_defer() could
be needed by audio clients as well. To avoid mistakes when someone adds
conditions for these in the future, constrain the single existing
condition to VGA clients by checking for PCI_BASE_CLASS_DISPLAY. This
encompasses both PCI_CLASS_DISPLAY_VGA as well as PCI_CLASS_DISPLAY_3D,
which is used by some Nvidia Optimus GPUs.

Any future checks for audio clients should then be constrained to
PCI_BASE_CLASS_MULTIMEDIA.

v6: Spun out from commit introducing vga_switcheroo_client_probe_defer()
to keep it a pure refactoring change. (Emil Velikov, Jani Nikula)

Cc: Daniel Vetter <daniel.vet...@ffwll.ch>
Cc: Emil Velikov <emil.l.veli...@gmail.com>
Cc: Jani Nikula <jani.nik...@linux.intel.com>
Signed-off-by: Lukas Wunner <lu...@wunner.de>
---
 drivers/gpu/vga/vga_switcheroo.c | 19 +++
 1 file changed, 11 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/vga/vga_switcheroo.c b/drivers/gpu/vga/vga_switcheroo.c
index d349bf9..2df216b3 100644
--- a/drivers/gpu/vga/vga_switcheroo.c
+++ b/drivers/gpu/vga/vga_switcheroo.c
@@ -331,7 +331,8 @@ EXPORT_SYMBOL(vga_switcheroo_register_client);
  * @id: client identifier
  *
  * Register audio client (audio device on a GPU). The power state of the
- * client is assumed to be ON.
+ * client is assumed to be ON. Beforehand, vga_switcheroo_client_probe_defer()
+ * shall be called to ensure that all prerequisites are met.
  *
  * Return: 0 on success, -ENOMEM on memory allocation error.
  */
@@ -390,13 +391,15 @@ find_active_client(struct list_head *head)
  */
 bool vga_switcheroo_client_probe_defer(struct pci_dev *pdev)
 {
-   /*
-* apple-gmux is needed on pre-retina MacBook Pro
-* to probe the panel if pdev is the inactive GPU.
-*/
-   if (apple_gmux_present() && pdev != vga_default_device() &&
-   !vgasr_priv.handler_flags)
-   return true;
+   if ((pdev->class >> 16) == PCI_BASE_CLASS_DISPLAY) {
+   /*
+* apple-gmux is needed on pre-retina MacBook Pro
+* to probe the panel if pdev is the inactive GPU.
+*/
+   if (apple_gmux_present() && pdev != vga_default_device() &&
+   !vgasr_priv.handler_flags)
+   return true;
+   }
 
return false;
 }
-- 
2.8.1

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

[Intel-gfx] [PATCH v6 1/2] vga_switcheroo: Add helper for deferred probing

[Lukas Wunner]

So far we've got one condition when DRM drivers need to defer probing
on a dual GPU system and it's coded separately into each of the relevant
drivers. As suggested by Daniel Vetter, deduplicate that code in the
drivers and move it to a new vga_switcheroo helper. This yields better
encapsulation of concepts and lets us add further checks in a central
place. (The existing check pertains to pre-retina MacBook Pros and an
additional check is expected to be needed for retinas.)

One might be tempted to check deferred probing conditions in
vga_switcheroo_register_client(), but this is usually called fairly late
during driver load. The GPU is fully brought up and ready for switching
at that point. On boot the ->probe hook is potentially called dozens of
times until it finally succeeds, and each time we'd repeat bringup and
teardown of the GPU, lengthening boot time considerably and cluttering
logfiles. A separate helper is therefore needed which can be called
right at the beginning of the ->probe hook.

Note that amdgpu currently does not call this helper as the AMD GPUs
built into MacBook Pros are only supported by radeon so far.

v2: This helper could eventually be used by audio clients as well,
so rephrase kerneldoc to refer to "client" instead of "GPU"
and move the single existing check in an if block specific
to PCI_CLASS_DISPLAY_VGA devices. Move documentation on
that check from kerneldoc to a comment. (Daniel Vetter)

v3: Mandate in kerneldoc that registration of client shall only
happen after calling this helper. (Daniel Vetter)

v4: Rebase on 412c8f7de011 ("drm/radeon: Return -EPROBE_DEFER when
amdkfd not loaded")

v5: Some Optimus GPUs use PCI_CLASS_DISPLAY_3D, make sure those are
matched as well. (Emil Velikov)

v6: The if-condition referring to PCI_BASE_CLASS_DISPLAY may be
considered a functional change. Move to a separate commit to
keep this a pure refactoring change. (Emil Velikov, Jani Nikula)

Cc: Daniel Vetter <daniel.vet...@ffwll.ch>
Cc: Ben Skeggs <bske...@redhat.com>
Cc: Alex Deucher <alexander.deuc...@amd.com>
Signed-off-by: Lukas Wunner <lu...@wunner.de>
---
 drivers/gpu/drm/i915/i915_drv.c   | 10 +-
 drivers/gpu/drm/nouveau/nouveau_drm.c | 10 +-
 drivers/gpu/drm/radeon/radeon_drv.c   | 10 +-
 drivers/gpu/vga/vga_switcheroo.c  | 29 -
 include/linux/vga_switcheroo.h|  2 ++
 5 files changed, 33 insertions(+), 28 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 943d7b2..872c6060 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -35,11 +35,9 @@
 #include "i915_trace.h"
 #include "intel_drv.h"
 
-#include 
 #include 
 #include 
 #include 
-#include 
 #include 
 #include 
 
@@ -1030,13 +1028,7 @@ static int i915_pci_probe(struct pci_dev *pdev, const 
struct pci_device_id *ent)
if (PCI_FUNC(pdev->devfn))
return -ENODEV;
 
-   /*
-* apple-gmux is needed on dual GPU MacBook Pro
-* to probe the panel if we're the inactive GPU.
-*/
-   if (IS_ENABLED(CONFIG_VGA_ARB) && IS_ENABLED(CONFIG_VGA_SWITCHEROO) &&
-   apple_gmux_present() && pdev != vga_default_device() &&
-   !vga_switcheroo_handler_flags())
+   if (vga_switcheroo_client_probe_defer(pdev))
return -EPROBE_DEFER;
 
return drm_get_pci_dev(pdev, ent, );
diff --git a/drivers/gpu/drm/nouveau/nouveau_drm.c 
b/drivers/gpu/drm/nouveau/nouveau_drm.c
index 11f8dd9..5c4d4df 100644
--- a/drivers/gpu/drm/nouveau/nouveau_drm.c
+++ b/drivers/gpu/drm/nouveau/nouveau_drm.c
@@ -22,13 +22,11 @@
  * Authors: Ben Skeggs
  */
 
-#include 
 #include 
 #include 
 #include 
 #include 
 #include 
-#include 
 #include 
 
 #include "drmP.h"
@@ -315,13 +313,7 @@ static int nouveau_drm_probe(struct pci_dev *pdev,
bool boot = false;
int ret;
 
-   /*
-* apple-gmux is needed on dual GPU MacBook Pro
-* to probe the panel if we're the inactive GPU.
-*/
-   if (IS_ENABLED(CONFIG_VGA_ARB) && IS_ENABLED(CONFIG_VGA_SWITCHEROO) &&
-   apple_gmux_present() && pdev != vga_default_device() &&
-   !vga_switcheroo_handler_flags())
+   if (vga_switcheroo_client_probe_defer(pdev))
return -EPROBE_DEFER;
 
/* remove conflicting drivers (vesafb, efifb etc) */
diff --git a/drivers/gpu/drm/radeon/radeon_drv.c 
b/drivers/gpu/drm/radeon/radeon_drv.c
index b55aa74..a455dc7 100644
--- a/drivers/gpu/drm/radeon/radeon_drv.c
+++ b/drivers/gpu/drm/radeon/radeon_drv.c
@@ -34,11 +34,9 @@
 #include "radeon_drv.h"
 
 #include 
-#include 
 #include 
 #include 
 #include 
-#include 
 #include 
 #include 
 
@@ -340,13 +338,7 @@ static int radeon_pci_probe(struc

Re: [Intel-gfx] [Nouveau] [PATCH v4] vga_switcheroo: Add helper for deferred probing

[Lukas Wunner]

Hi Emil,

On Fri, May 20, 2016 at 12:41:04AM +0100, Emil Velikov wrote:
> On 19 May 2016 at 15:39, Lukas Wunner <lu...@wunner.de> wrote:
> > +bool vga_switcheroo_client_probe_defer(struct pci_dev *pdev)
> > +{
> > +   if ((pdev->class >> 8) == PCI_CLASS_DISPLAY_VGA) {
> Not sure if we want/need this, yet at least. This changes behaviour
> which is not what refactoring patches should be doing, right ? if
> needed it ought to be a separate patch, imho.

Well, the commit message doesn't claim "no functional change", does it?

Daniel Vetter explicitly wanted the ability to use the helper in
vga_switcheroo audio clients, and those shouldn't run the apple-gmux
test. I think it makes sense to enclose it in the above-quoted if-block
*now* even though it's not needed. Once someone adds a check for an
audio client, chances are they'll forget to add this if-block.

> Furthermore on nouveau and AMD (iirc) front, some dual-gpu/optimus
> systems use PCI_CLASS_DISPLAY_3D. So if we add DISPLAY_VGA perhaps we
> should also check for DISPLAY_3D ?

Fair enough, I've changed it to match PCI_BASE_CLASS_DISPLAY
and sent it out as v5 a few minutes ago.

Thanks,

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

[Intel-gfx] [PATCH v5] vga_switcheroo: Add helper for deferred probing

[Lukas Wunner]

So far we've got one condition when DRM drivers need to defer probing
on a dual GPU system and it's coded separately into each of the relevant
drivers. As suggested by Daniel Vetter, deduplicate that code in the
drivers and move it to a new vga_switcheroo helper. This yields better
encapsulation of concepts and lets us add further checks in a central
place. (The existing check pertains to pre-retina MacBook Pros and an
additional check is expected to be needed for retinas.)

v2: This helper could eventually be used by audio clients as well,
so rephrase kerneldoc to refer to "client" instead of "GPU"
and move the single existing check in an if block specific
to PCI_CLASS_DISPLAY_VGA devices. Move documentation on
that check from kerneldoc to a comment. (Daniel Vetter)

v3: Mandate in kerneldoc that registration of client shall only
happen after calling this helper. (Daniel Vetter)

v4: Rebase on 412c8f7de011 ("drm/radeon: Return -EPROBE_DEFER when
amdkfd not loaded")

v5: Some Optimus GPUs use PCI_CLASS_DISPLAY_3D, make sure those are
matched as well. (Emil Velikov)

Cc: Daniel Vetter <daniel.vet...@ffwll.ch>
Cc: Ben Skeggs <bske...@redhat.com>
Cc: Alex Deucher <alexander.deuc...@amd.com>
Signed-off-by: Lukas Wunner <lu...@wunner.de>
---
 drivers/gpu/drm/i915/i915_drv.c   | 10 +-
 drivers/gpu/drm/nouveau/nouveau_drm.c | 10 +-
 drivers/gpu/drm/radeon/radeon_drv.c   | 10 +-
 drivers/gpu/vga/vga_switcheroo.c  | 34 --
 include/linux/vga_switcheroo.h|  2 ++
 5 files changed, 37 insertions(+), 29 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index d37c0a6..20d5898 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -35,11 +35,9 @@
 #include "i915_trace.h"
 #include "intel_drv.h"
 
-#include 
 #include 
 #include 
 #include 
-#include 
 #include 
 #include 
 
@@ -1025,13 +1023,7 @@ static int i915_pci_probe(struct pci_dev *pdev, const 
struct pci_device_id *ent)
if (PCI_FUNC(pdev->devfn))
return -ENODEV;
 
-   /*
-* apple-gmux is needed on dual GPU MacBook Pro
-* to probe the panel if we're the inactive GPU.
-*/
-   if (IS_ENABLED(CONFIG_VGA_ARB) && IS_ENABLED(CONFIG_VGA_SWITCHEROO) &&
-   apple_gmux_present() && pdev != vga_default_device() &&
-   !vga_switcheroo_handler_flags())
+   if (vga_switcheroo_client_probe_defer(pdev))
return -EPROBE_DEFER;
 
return drm_get_pci_dev(pdev, ent, );
diff --git a/drivers/gpu/drm/nouveau/nouveau_drm.c 
b/drivers/gpu/drm/nouveau/nouveau_drm.c
index 11f8dd9..5c4d4df 100644
--- a/drivers/gpu/drm/nouveau/nouveau_drm.c
+++ b/drivers/gpu/drm/nouveau/nouveau_drm.c
@@ -22,13 +22,11 @@
  * Authors: Ben Skeggs
  */
 
-#include 
 #include 
 #include 
 #include 
 #include 
 #include 
-#include 
 #include 
 
 #include "drmP.h"
@@ -315,13 +313,7 @@ static int nouveau_drm_probe(struct pci_dev *pdev,
bool boot = false;
int ret;
 
-   /*
-* apple-gmux is needed on dual GPU MacBook Pro
-* to probe the panel if we're the inactive GPU.
-*/
-   if (IS_ENABLED(CONFIG_VGA_ARB) && IS_ENABLED(CONFIG_VGA_SWITCHEROO) &&
-   apple_gmux_present() && pdev != vga_default_device() &&
-   !vga_switcheroo_handler_flags())
+   if (vga_switcheroo_client_probe_defer(pdev))
return -EPROBE_DEFER;
 
/* remove conflicting drivers (vesafb, efifb etc) */
diff --git a/drivers/gpu/drm/radeon/radeon_drv.c 
b/drivers/gpu/drm/radeon/radeon_drv.c
index b55aa74..a455dc7 100644
--- a/drivers/gpu/drm/radeon/radeon_drv.c
+++ b/drivers/gpu/drm/radeon/radeon_drv.c
@@ -34,11 +34,9 @@
 #include "radeon_drv.h"
 
 #include 
-#include 
 #include 
 #include 
 #include 
-#include 
 #include 
 #include 
 
@@ -340,13 +338,7 @@ static int radeon_pci_probe(struct pci_dev *pdev,
if (ret == -EPROBE_DEFER)
return ret;
 
-   /*
-* apple-gmux is needed on dual GPU MacBook Pro
-* to probe the panel if we're the inactive GPU.
-*/
-   if (IS_ENABLED(CONFIG_VGA_ARB) && IS_ENABLED(CONFIG_VGA_SWITCHEROO) &&
-   apple_gmux_present() && pdev != vga_default_device() &&
-   !vga_switcheroo_handler_flags())
+   if (vga_switcheroo_client_probe_defer(pdev))
return -EPROBE_DEFER;
 
/* Get rid of things like offb */
diff --git a/drivers/gpu/vga/vga_switcheroo.c b/drivers/gpu/vga/vga_switcheroo.c
index cbd7c98..2df216b3 100644
--- a/drivers/gpu/vga/vga_switcheroo.c
+++ b/drivers/gpu/vga/vga_switcheroo.c
@@ -30,6 +30,7 @@
 
 #define pr_fmt(fmt) "vga_switcheroo: " fmt
 
+#include 
 #include 
 

Re: [Intel-gfx] [PATCH v2] drm/i915/fbdev: Limit the global async-domain synchronization

[Lukas Wunner]

Hi Chris,

On Thu, May 19, 2016 at 09:28:10AM +0100, Chris Wilson wrote:
> During cleanup we have to synchronise with the async task we are using
> to initialise and register our fbdev. Currently, we are using a full
> synchronisation on the global domain, but we can restrict this to just
> synchronising up to our task if we remember our cookie.
> 
> v2: async_synchronize_cookie() takes an exclusive upper bound, to
> synchronize with our task we have to pass in the next cookie.

Oops, good catch, missed that in my own version of this patch:
https://lists.freedesktop.org/archives/intel-gfx/2016-March/091257.html

> 
> Signed-off-by: Chris Wilson 
> ---
>  drivers/gpu/drm/i915/intel_drv.h   |  1 +
>  drivers/gpu/drm/i915/intel_fbdev.c | 31 ++-
>  2 files changed, 19 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_drv.h 
> b/drivers/gpu/drm/i915/intel_drv.h
> index 3536292babe0..5bb045ba608e 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -159,6 +159,7 @@ struct intel_framebuffer {
>  struct intel_fbdev {
>   struct drm_fb_helper helper;
>   struct intel_framebuffer *fb;
> + async_cookie_t cookie;
>   int preferred_bpp;
>  };
>  
> diff --git a/drivers/gpu/drm/i915/intel_fbdev.c 
> b/drivers/gpu/drm/i915/intel_fbdev.c
> index 99e27530e264..2e9c3f38c023 100644
> --- a/drivers/gpu/drm/i915/intel_fbdev.c
> +++ b/drivers/gpu/drm/i915/intel_fbdev.c
> @@ -538,8 +538,7 @@ static const struct drm_fb_helper_funcs 
> intel_fb_helper_funcs = {
>   .fb_probe = intelfb_create,
>  };
>  
> -static void intel_fbdev_destroy(struct drm_device *dev,
> - struct intel_fbdev *ifbdev)
> +static void intel_fbdev_destroy(struct intel_fbdev *ifbdev)
>  {
>   /* We rely on the object-free to release the VMA pinning for
>* the info->screen_base mmaping. Leaking the VMA is simpler than
> @@ -554,12 +553,14 @@ static void intel_fbdev_destroy(struct drm_device *dev,
>   if (ifbdev->fb) {
>   drm_framebuffer_unregister_private(>fb->base);
>  
> - mutex_lock(>struct_mutex);
> + mutex_lock(>helper.dev->struct_mutex);
>   intel_unpin_fb_obj(>fb->base, BIT(DRM_ROTATE_0));
> - mutex_unlock(>struct_mutex);
> + mutex_unlock(>helper.dev->struct_mutex);
>  
>   drm_framebuffer_remove(>fb->base);
>   }
> +
> + kfree(ifbdev);
>  }
>  
>  /*
> @@ -736,32 +737,36 @@ int intel_fbdev_init(struct drm_device *dev)
>  
>  static void intel_fbdev_initial_config(void *data, async_cookie_t cookie)
>  {
> - struct drm_i915_private *dev_priv = data;
> - struct intel_fbdev *ifbdev = dev_priv->fbdev;
> + struct intel_fbdev *ifbdev = data;
> +
> + ifbdev->cookie = 0;

Hm, why are you setting this to 0 here? IIUC the effect is that
async_synchronize_cookie() will wait until intel_fbdev_initial_config()
has been *entered*, but isn't the desired effect that it has *finished*?

Best regards,

Lukas

>  
>   /* Due to peculiar init order wrt to hpd handling this is separate. */
>   if (drm_fb_helper_initial_config(>helper,
>ifbdev->preferred_bpp))
> - intel_fbdev_fini(dev_priv->dev);
> + intel_fbdev_fini(ifbdev->helper.dev);
>  }
>  
>  void intel_fbdev_initial_config_async(struct drm_device *dev)
>  {
> - async_schedule(intel_fbdev_initial_config, to_i915(dev));
> + struct intel_fbdev *ifbdev = to_i915(dev)->fbdev;
> +
> + ifbdev->cookie = async_schedule(intel_fbdev_initial_config, ifbdev);
>  }
>  
>  void intel_fbdev_fini(struct drm_device *dev)
>  {
>   struct drm_i915_private *dev_priv = dev->dev_private;
> - if (!dev_priv->fbdev)
> + struct intel_fbdev *ifbdev = dev_priv->fbdev;
> +
> + if (!ifbdev)
>   return;
>  
>   flush_work(_priv->fbdev_suspend_work);
> + if (ifbdev->cookie && !current_is_async())
> + async_synchronize_cookie(ifbdev->cookie + 1);
>  
> - if (!current_is_async())
> - async_synchronize_full();
> - intel_fbdev_destroy(dev, dev_priv->fbdev);
> - kfree(dev_priv->fbdev);
> + intel_fbdev_destroy(ifbdev);
>   dev_priv->fbdev = NULL;
>  }
>  
> -- 
> 2.8.1
> 
> ___
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Intel-gfx] [PATCH v4] vga_switcheroo: Add helper for deferred probing

[Lukas Wunner]

So far we've got one condition when DRM drivers need to defer probing
on a dual GPU system and it's coded separately into each of the relevant
drivers. As suggested by Daniel Vetter, deduplicate that code in the
drivers and move it to a new vga_switcheroo helper. This yields better
encapsulation of concepts and lets us add further checks in a central
place. (The existing check pertains to pre-retina MacBook Pros and an
additional check is expected to be needed for retinas.)

v2: This helper could eventually be used by audio clients as well,
so rephrase kerneldoc to refer to "client" instead of "GPU"
and move the single existing check in an if block specific
to PCI_CLASS_DISPLAY_VGA devices. Move documentation on
that check from kerneldoc to a comment. (Daniel Vetter)

v3: Mandate in kerneldoc that registration of client shall only
happen after calling this helper. (Daniel Vetter)

v4: Rebase on 412c8f7de011 ("drm/radeon: Return -EPROBE_DEFER when
amdkfd not loaded")

Cc: Daniel Vetter <daniel.vet...@ffwll.ch>
Cc: Ben Skeggs <bske...@redhat.com>
Cc: Alex Deucher <alexander.deuc...@amd.com>
Signed-off-by: Lukas Wunner <lu...@wunner.de>
---
 drivers/gpu/drm/i915/i915_drv.c   | 10 +-
 drivers/gpu/drm/nouveau/nouveau_drm.c | 10 +-
 drivers/gpu/drm/radeon/radeon_drv.c   | 10 +-
 drivers/gpu/vga/vga_switcheroo.c  | 34 --
 include/linux/vga_switcheroo.h|  2 ++
 5 files changed, 37 insertions(+), 29 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index dba03c0..61bf5a9 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -35,11 +35,9 @@
 #include "i915_trace.h"
 #include "intel_drv.h"
 
-#include 
 #include 
 #include 
 #include 
-#include 
 #include 
 #include 
 
@@ -1030,13 +1028,7 @@ static int i915_pci_probe(struct pci_dev *pdev, const 
struct pci_device_id *ent)
if (PCI_FUNC(pdev->devfn))
return -ENODEV;
 
-   /*
-* apple-gmux is needed on dual GPU MacBook Pro
-* to probe the panel if we're the inactive GPU.
-*/
-   if (IS_ENABLED(CONFIG_VGA_ARB) && IS_ENABLED(CONFIG_VGA_SWITCHEROO) &&
-   apple_gmux_present() && pdev != vga_default_device() &&
-   !vga_switcheroo_handler_flags())
+   if (vga_switcheroo_client_probe_defer(pdev))
return -EPROBE_DEFER;
 
return drm_get_pci_dev(pdev, ent, );
diff --git a/drivers/gpu/drm/nouveau/nouveau_drm.c 
b/drivers/gpu/drm/nouveau/nouveau_drm.c
index db5c7d0..a0bb1d0 100644
--- a/drivers/gpu/drm/nouveau/nouveau_drm.c
+++ b/drivers/gpu/drm/nouveau/nouveau_drm.c
@@ -22,13 +22,11 @@
  * Authors: Ben Skeggs
  */
 
-#include 
 #include 
 #include 
 #include 
 #include 
 #include 
-#include 
 #include 
 
 #include "drmP.h"
@@ -315,13 +313,7 @@ static int nouveau_drm_probe(struct pci_dev *pdev,
bool boot = false;
int ret;
 
-   /*
-* apple-gmux is needed on dual GPU MacBook Pro
-* to probe the panel if we're the inactive GPU.
-*/
-   if (IS_ENABLED(CONFIG_VGA_ARB) && IS_ENABLED(CONFIG_VGA_SWITCHEROO) &&
-   apple_gmux_present() && pdev != vga_default_device() &&
-   !vga_switcheroo_handler_flags())
+   if (vga_switcheroo_client_probe_defer(pdev))
return -EPROBE_DEFER;
 
/* remove conflicting drivers (vesafb, efifb etc) */
diff --git a/drivers/gpu/drm/radeon/radeon_drv.c 
b/drivers/gpu/drm/radeon/radeon_drv.c
index b55aa74..a455dc7 100644
--- a/drivers/gpu/drm/radeon/radeon_drv.c
+++ b/drivers/gpu/drm/radeon/radeon_drv.c
@@ -34,11 +34,9 @@
 #include "radeon_drv.h"
 
 #include 
-#include 
 #include 
 #include 
 #include 
-#include 
 #include 
 #include 
 
@@ -340,13 +338,7 @@ static int radeon_pci_probe(struct pci_dev *pdev,
if (ret == -EPROBE_DEFER)
return ret;
 
-   /*
-* apple-gmux is needed on dual GPU MacBook Pro
-* to probe the panel if we're the inactive GPU.
-*/
-   if (IS_ENABLED(CONFIG_VGA_ARB) && IS_ENABLED(CONFIG_VGA_SWITCHEROO) &&
-   apple_gmux_present() && pdev != vga_default_device() &&
-   !vga_switcheroo_handler_flags())
+   if (vga_switcheroo_client_probe_defer(pdev))
return -EPROBE_DEFER;
 
/* Get rid of things like offb */
diff --git a/drivers/gpu/vga/vga_switcheroo.c b/drivers/gpu/vga/vga_switcheroo.c
index cbd7c98..101e14c 100644
--- a/drivers/gpu/vga/vga_switcheroo.c
+++ b/drivers/gpu/vga/vga_switcheroo.c
@@ -30,6 +30,7 @@
 
 #define pr_fmt(fmt) "vga_switcheroo: " fmt
 
+#include 
 #include 
 #include 
 #include 
@@ -308,7 +309,8 @@ static int register_client(struct pci_de

Re: [Intel-gfx] [PATCH] i915 suspend/resume_noirq instead of suspend_late/resume_early

[Lukas Wunner]

Hi,

On Wed, Apr 27, 2016 at 11:46:22AM -0700, Todd Brandt wrote:
>  I'd like to propose that we push the i915 suspend_late/resume_early code
>  into suspend_noirq/resume_noirq in order to reduce the total suspend time
>  by ~15ms. According to the comments, when i915_pm_suspend_late was first
>  added to the kernel back in April 2014, it was done so to ensure that it
>  was called after the snd_hda_intel driver had finished its suspend.

Ordering issues like this one should be solved with device_pm_wait_for_dev(),
not by shuffling code around among the callbacks.

In any case it would be good if you would name the sha1 of the commit
that added i915_pm_suspend_late to spare readers the git blame / git log.

Thanks,

Lukas

> 
> The comments in i915_drv.c are here:
> 
> /*
>  * We have a suspedn ordering issue with the snd-hda driver also
>  * requiring our device to be power up. Due to the lack of a
>  * parent/child relationship we currently solve this with an late
>  * suspend hook.
>  *
>  * FIXME: This should be solved with a special hdmi sink device or
>  * similar so that power domains can be employed.
>  */
> 
> I believe we could achieve the same ordering by simply pushing it to
> suspend/resume_noirq. Thus we can effectively eliminate the suspend_late
> and resume_early phases altogether in most simple systems. Does anyone see
> a problem with this?
> 
> analyzesuspend outputs for freeze/suspend/hibernate (WITHOUT PATCH):
> https://01org.github.io/suspendresume/i915/freeze-160422-155931-ivybridge-dev-late/
> https://01org.github.io/suspendresume/i915/suspend-160422-155735-ivybridge-dev-late/
> https://01org.github.io/suspendresume/i915/hibernate-160422-163915-ivybridge-dev-late/
> 
> analyzesuspend outputs for freeze/suspend/hibernate (WITH PATCH):
> https://01org.github.io/suspendresume/i915/freeze-160422-162811-ivybridge-dev-noirq/
> https://01org.github.io/suspendresume/i915/suspend-160422-162700-ivybridge-dev-noirq/
> https://01org.github.io/suspendresume/i915/hibernate-160422-162952-ivybridge-dev-noirq/
> 
> Signed-off-by: Todd Brandt 
> ---
>  drivers/gpu/drm/i915/i915_drv.c | 12 ++--
>  1 file changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index 30798cb..759d93c 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -1628,8 +1628,8 @@ static const struct dev_pm_ops i915_pm_ops = {
>* PMSG_RESUME]
>*/
>   .suspend = i915_pm_suspend,
> - .suspend_late = i915_pm_suspend_late,
> - .resume_early = i915_pm_resume_early,
> + .suspend_noirq = i915_pm_suspend_late,
> + .resume_noirq = i915_pm_resume_early,
>   .resume = i915_pm_resume,
>  
>   /*
> @@ -1648,12 +1648,12 @@ static const struct dev_pm_ops i915_pm_ops = {
>*hibernation image [PMSG_RESTORE]
>*/
>   .freeze = i915_pm_suspend,
> - .freeze_late = i915_pm_suspend_late,
> - .thaw_early = i915_pm_resume_early,
> + .freeze_noirq = i915_pm_suspend_late,
> + .thaw_noirq = i915_pm_resume_early,
>   .thaw = i915_pm_resume,
>   .poweroff = i915_pm_suspend,
> - .poweroff_late = i915_pm_poweroff_late,
> - .restore_early = i915_pm_resume_early,
> + .poweroff_noirq = i915_pm_poweroff_late,
> + .restore_noirq = i915_pm_resume_early,
>   .restore = i915_pm_resume,
>  
>   /* S0ix (via runtime suspend) event handlers */
> -- 
> 2.1.4
> 
> ___
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 10/12] drm/i915: Disable use of stolen area by User when Intel RST is present

[Lukas Wunner]

Hi Ankitprasad,

just a quick heads-up, Rafael asked that the function name
acpi_dev_present() be changed, so there's now a commit queued
for Linux 4.7 to rename it to acpi_dev_found():

https://git.kernel.org/cgit/linux/kernel/git/next/linux-next.git/commit/?id=c68ae33e7fb4

It will therefore unfortunately be necessary that you respin
this patch with the new function name once the above-linked
commit has landed in Linus' tree (which I expect will happen
early in the merge window, sometime in mid-May).

My apologies for the inconvenience!

Lukas

On Wed, Apr 20, 2016 at 04:47:37PM +0530, ankitprasad.r.sha...@intel.com wrote:
> From: Ankitprasad Sharma <ankitprasad.r.sha...@intel.com>
> 
> The BIOS RapidStartTechnology may corrupt the stolen memory across S3
> suspend due to unalarmed hibernation, in which case we will not be able
> to preserve the User data stored in the stolen region. Hence this patch
> tries to identify presence of the RST device on the ACPI bus, and
> disables use of stolen memory (for persistent data) if found.
> 
> v2: Updated comment, updated/corrected new functions private to driver
> (Chris/Tvrtko)
> 
> v3: Disabling stolen by default, wait till required acpi changes to
> detect device presence are pulled in (Ankit)
> 
> v4: Enabled stolen by default as required acpi changes are merged
> (Ankit)
> 
> v5: renamed variable, is IS_ENABLED() in place of #ifdef, use char*
> instead of structures (Lukas)
> 
> Signed-off-by: Ankitprasad Sharma <ankitprasad.r.sha...@intel.com>
> Cc: Lukas Wunner <lu...@wunner.de>
> Reviewed-by: Tvrtko Ursulin <tvrtko.ursu...@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.h| 11 +++
>  drivers/gpu/drm/i915/i915_gem.c|  8 
>  drivers/gpu/drm/i915/i915_gem_stolen.c | 12 
>  drivers/gpu/drm/i915/intel_acpi.c  |  7 +++
>  4 files changed, 38 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index c7fe863..37f9de8 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -1326,6 +1326,16 @@ struct i915_gem_mm {
>*/
>   bool busy;
>  
> + /**
> +  * Stolen will be lost upon hibernate (as the memory is unpowered).
> +  * Across resume, we expect stolen to be intact - however, it may
> +  * also be utililised by third parties (e.g. Intel RapidStart
> +  * Technology) and if so we have to assume that any data stored in
> +  * stolen across resume is lost and we set this flag to indicate that
> +  * the stolen memory is volatile.
> +  */
> + bool volatile_stolen;
> +
>   /* the indicator for dispatch video commands on two BSD rings */
>   unsigned int bsd_ring_dispatch_index;
>  
> @@ -3559,6 +3569,7 @@ static inline int intel_opregion_get_panel_type(struct 
> drm_device *dev)
>  #endif
>  
>  /* intel_acpi.c */
> +bool intel_detect_acpi_rst(void);
>  #ifdef CONFIG_ACPI
>  extern void intel_register_dsm_handler(void);
>  extern void intel_unregister_dsm_handler(void);
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index 48959cf..45fd049 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -391,8 +391,16 @@ static struct drm_i915_gem_object *
>  i915_gem_alloc_object_stolen(struct drm_device *dev, size_t size)
>  {
>   struct drm_i915_gem_object *obj;
> + struct drm_i915_private *dev_priv = dev->dev_private;
>   int ret;
>  
> + if (dev_priv->mm.volatile_stolen) {
> + /* Stolen may be overwritten by external parties
> +  * so unsuitable for persistent user data.
> +  */
> + return ERR_PTR(-ENODEV);
> + }
> +
>   mutex_lock(>struct_mutex);
>   obj = i915_gem_object_create_stolen(dev, size);
>   if (IS_ERR(obj))
> diff --git a/drivers/gpu/drm/i915/i915_gem_stolen.c 
> b/drivers/gpu/drm/i915/i915_gem_stolen.c
> index d9756ee..ef28af6 100644
> --- a/drivers/gpu/drm/i915/i915_gem_stolen.c
> +++ b/drivers/gpu/drm/i915/i915_gem_stolen.c
> @@ -490,6 +490,18 @@ int i915_gem_init_stolen(struct drm_device *dev)
>*/
>   drm_mm_init(_priv->mm.stolen, 0, ggtt->stolen_usable_size);
>  
> + /* If the stolen region can be modified behind our backs upon suspend,
> +  * then we cannot use it to store nonvolatile contents (i.e user data)
> +  * as it will be corrupted upon resume.
> +  */
> + dev_priv->mm.volatile_stolen = false;
> + if (IS_ENABLED(CONFIG_SUSPEND)) {
> + /* BIOSes using RapidStart Technology have been reported
> +  * to overwrite stolen ac

Re: [Intel-gfx] [PATCH v5 00/12] Enable GPU switching on pre-retina?MacBook Pro

[Lukas Wunner]

Hi Bastien,

On Tue, Apr 05, 2016 at 06:59:40PM +0200, Bastien Nocera wrote:
> I tested the runtime patches for Radeon on top of 4.6.0-rc2, and
> writing DIGD failed. I also saw a number of messages with the
> vga_switcheroo core in the kernel trying to switch GPUs but failed
> because "client 1" was keeping it busy.
> 
> Is there any way to see what this "client 1" actually is? I'm guessing
> plymouth. In any case, once I'm logged in, the "DIS" is powered and
> used, and the IGD is powered as well.

Client 1 is the discrete GPU, see enum vga_switcheroo_client_id in
include/linux/vga_switcheroo.h:
https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/include/linux/vga_switcheroo.h#n75

The vga_switcheroo documentation explains how to find out which
user space process is blocking the switch:
https://01.org/linuxgraphics/gfx-docs/drm/modes_of_use.html

"Prerequisite is that no user space processes (e.g. Xorg, alsactl)
have opened device files of the GPUs or the audio client. If the
switch fails, the user may invoke lsof(8) or fuser(1) on /dev/dri/
and /dev/snd/controlC1 to identify processes blocking the switch."

HTH,

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

Re: [Intel-gfx] [REGRESSION] system hang on ILK/SNB/IVB

[Lukas Wunner]

Hi Tomi,

On Thu, Mar 31, 2016 at 10:21:16AM +0300, Tomi Sarvela wrote:
> The problem with the results in your link is that there is no HSW, ILK, IVB
> or SNB results. This might give the impression that everything is well.
> 
> Most damning is lack of HSW-gt2 and SNB-dellxps: those machines hang on to
> APC, and have run quite stably for every Patchwork run. The case isn't strong
> enough yet that series should fail if either of those won't run, but it might
> be so in future.

So my patch seeking to fix the hangs has passed Romania CI with "success":
https://patchwork.freedesktop.org/series/5125/

However I don't see HSW-gt2 and SNB-dellxps in their hardware lineup.
And I would still like to know what the actual cause of the hangs is
since they do not occur on my IVB laptop.

If you get the chance maybe you can repeat the test and include a
"dump_stack();" at the beginning of intel_fbdev_output_poll_changed()
and intel_fbdev_restore_mode(). This should show in the logs which of
the two functions is called during suspend/resume and from where it's
called. My guess is that this particular hardware causes a hotplug
signal to be generated upon waking up. If the hangs do not stop with
this patch, booting with "no_console_suspend" should at least show
what's going on.

Thank you & best regards,

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

Re: [Intel-gfx] [PATCH v4 1/2] drm/i915: Protect fbdev across slow or failed initialisation

[Lukas Wunner]

Hi Chris,

On Thu, Mar 31, 2016 at 05:13:55PM +0100, Chris Wilson wrote:
> On Thu, Mar 31, 2016 at 07:05:21PM +0300, Joonas Lahtinen wrote:
> > On to, 2016-03-31 at 14:57 +0100, Chris Wilson wrote:
> > >  void intel_fbdev_restore_mode(struct drm_device *dev)
> > >  {
> > > - int ret;
> > > - struct drm_i915_private *dev_priv = dev->dev_private;
> > > - struct intel_fbdev *ifbdev = dev_priv->fbdev;
> > > - struct drm_fb_helper *fb_helper;
> > > + struct intel_fbdev *ifbdev = intel_fbdev_get_if_active(dev);
> > >  
> > > - async_synchronize_full();
> > 
> > What's with the async_synchronize_full() begin removed completely?
> 
> Because it's not just wrong, but completely broken imo.
> 
> During suspend, we want to freeze the async task not flush.
> Then here and during resume we skip the restoration of the unregistered
> fbdev, and then when the task is woken it can complete the registration
> and present the vanilla ifbdev.

No. The fbdev initialization is guaranteed to have finished before
suspend. So "we want to freeze the async task" is wrong thinking.
There is no async task to freeze.

Please read the commit message of a7442b93cf32 ("drm/i915: Fix races
on fbdev"):

"a device is never suspended until its ->probe callback (and all
asynchronous tasks it scheduled) have finished. See dpm_prepare(),
which calls wait_for_device_probe(), which calls async_synchronize_full()."

Best regards,

Lukas

> During hibernation, we really just want to cancel the task and start
> from scratch on resume.
> -Chris
> 
> -- 
> Chris Wilson, Intel Open Source Technology Centre
> ___
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [REGRESSION] system hang on ILK/SNB/IVB

[Lukas Wunner]

Hi Gabriel,

On Thu, Mar 31, 2016 at 10:42:37AM +0300, Gabriel Feceoru wrote:
> On 31.03.2016 00:35, Lukas Wunner wrote:
> >On Wed, Mar 30, 2016 at 08:20:26PM +0300, Gabriel Feceoru wrote:
> >>This commit causes a hang while running kms suspend tests
> >>(kms_pipe_crc_basic@suspend-read-crc-pipe-*) on ILK/SNB/IVB, affecting CI.
> 
> Tomi already replied, meantime I also looked at the results.
> The current regression is for ILK/SNB/IVB only (v1 seemed to affect more
> platforms).
> Unfortunately these machines were not available when v2 was tested, so this
> couldn't be detected.

I dev on an IVB machine and cannot reproduce this. Suspend works fine.

All the patch does is call async_synchronize_full()
(1) when a hotplug event arrives or
(2) when the last DRM client closes the connection.
Either of these two things seems to be happening on your test machines
when running the suspend test.

The PM core suspends and resumes individual devices asynchronously and
calls async_synchronize_full() in a couple of places. If a device's PM
callbacks also call async_synchronize_full(), the machine deadlocks.

It is unnecessary that we call async_synchronize_full(), we only need
to synchronize up to a specific cookie (which represents initialization
of the fbdev). So I've just posted a patch to replace the calls to
async_synchronize_full() with async_synchronize_cookie(). This should
make things less fragile and hopefully also solve the hangs you're seeing.

Best regards,

Lukas

> >>
> >>Probably the same problem with the one in v2, but on older HW.
> >>
> >>
> >>commit a7442b93cf32c1e1ddb721a26cd1f92302e2a222
> >>Author: Lukas Wunner <lu...@wunner.de>
> >>Date:   Wed Mar 9 12:52:53 2016 +0100
> >>
> >> drm/i915: Fix races on fbdev
> >>
> >> The ->lastclose callback invokes intel_fbdev_restore_mode() and has
> >> been witnessed to run before intel_fbdev_initial_config_async()
> >> has finished.
> >>
> >> We might likewise receive hotplug events before we've had a chance to
> >> fully set up the fbdev.
> >>
> >> Fix by waiting for the asynchronous thread to finish.
> >>
> >> v2:
> >> An async_synchronize_full() was also added to intel_fbdev_set_suspend()
> >> in v1 which turned out to be entirely gratuitous. It caused a deadlock
> >> on suspend (discovered by CI, thanks to Damien Lespiau and Tomi Sarvela
> >> for CI support) and was unnecessary since a device is never suspended
> >> until its ->probe callback (and all asynchronous tasks it scheduled)
> >> have finished. See dpm_prepare(), which calls wait_for_device_probe(),
> >> which calls async_synchronize_full().
> >>
> >>     Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=93580
> >> Reported-by: Gustav Fägerlind <gustav.fagerl...@gmail.com>
> >> Reported-by: "Li, Weinan Z" <weinan.z...@intel.com>
> >> Cc: Chris Wilson <ch...@chris-wilson.co.uk>
> >> Cc: sta...@vger.kernel.org
> >> Signed-off-by: Lukas Wunner <lu...@wunner.de>
> >> Signed-off-by: Daniel Vetter <daniel.vet...@ffwll.ch>
> >> Link: 
> >> http://patchwork.freedesktop.org/patch/msgid/20160309115147.67b2b6e...@gabe.freedesktop.org
> >>
> >>
> >>Regards,
> >>Gabriel
> >v2 passed CI fine, save for one warning not caused by the patch:
> >https://patchwork.freedesktop.org/series/4068/
> >
> >For comparison, this was v1:
> >https://patchwork.freedesktop.org/patch/75840/
> >
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Intel-gfx] [PATCH] drm/i915: Avoid unnecessary full synchronize for fbdev

[Lukas Wunner]

This should make the code less fragile by synchronizing only up to the
relevant cookie. Otherwise we risk deadlocks particularly during suspend
and resume.

Signed-off-by: Lukas Wunner <lu...@wunner.de>
---
 drivers/gpu/drm/i915/intel_drv.h   |  1 +
 drivers/gpu/drm/i915/intel_fbdev.c | 14 +-
 2 files changed, 10 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 6ac46d9..5bc9606 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -158,6 +158,7 @@ struct intel_framebuffer {
 struct intel_fbdev {
struct drm_fb_helper helper;
struct intel_framebuffer *fb;
+   async_cookie_t init_cookie;
int preferred_bpp;
 };
 
diff --git a/drivers/gpu/drm/i915/intel_fbdev.c 
b/drivers/gpu/drm/i915/intel_fbdev.c
index 153ea7a..8254ac8d 100644
--- a/drivers/gpu/drm/i915/intel_fbdev.c
+++ b/drivers/gpu/drm/i915/intel_fbdev.c
@@ -725,7 +725,7 @@ int intel_fbdev_init(struct drm_device *dev)
return 0;
 }
 
-static void intel_fbdev_initial_config(void *data, async_cookie_t cookie)
+static void intel_fbdev_initial_config(void *data, async_cookie_t init_cookie)
 {
struct drm_i915_private *dev_priv = data;
struct intel_fbdev *ifbdev = dev_priv->fbdev;
@@ -738,7 +738,11 @@ static void intel_fbdev_initial_config(void *data, 
async_cookie_t cookie)
 
 void intel_fbdev_initial_config_async(struct drm_device *dev)
 {
-   async_schedule(intel_fbdev_initial_config, to_i915(dev));
+   struct drm_i915_private *dev_priv = dev->dev_private;
+   struct intel_fbdev *ifbdev = dev_priv->fbdev;
+
+   ifbdev->init_cookie = async_schedule(intel_fbdev_initial_config,
+dev_priv);
 }
 
 void intel_fbdev_fini(struct drm_device *dev)
@@ -750,7 +754,7 @@ void intel_fbdev_fini(struct drm_device *dev)
flush_work(_priv->fbdev_suspend_work);
 
if (!current_is_async())
-   async_synchronize_full();
+   async_synchronize_cookie(dev_priv->fbdev->init_cookie);
intel_fbdev_destroy(dev, dev_priv->fbdev);
kfree(dev_priv->fbdev);
dev_priv->fbdev = NULL;
@@ -809,7 +813,7 @@ void intel_fbdev_output_poll_changed(struct drm_device *dev)
 {
struct drm_i915_private *dev_priv = dev->dev_private;
 
-   async_synchronize_full();
+   async_synchronize_cookie(dev_priv->fbdev->init_cookie);
if (dev_priv->fbdev)
drm_fb_helper_hotplug_event(_priv->fbdev->helper);
 }
@@ -821,7 +825,7 @@ void intel_fbdev_restore_mode(struct drm_device *dev)
struct intel_fbdev *ifbdev = dev_priv->fbdev;
struct drm_fb_helper *fb_helper;
 
-   async_synchronize_full();
+   async_synchronize_cookie(dev_priv->fbdev->init_cookie);
if (!ifbdev)
return;
 
-- 
2.8.0.rc3

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

Re: [Intel-gfx] [REGRESSION] system hang on ILK/SNB/IVB

[Lukas Wunner]

Hi Gabriel,

On Wed, Mar 30, 2016 at 08:20:26PM +0300, Gabriel Feceoru wrote:
> This commit causes a hang while running kms suspend tests
> (kms_pipe_crc_basic@suspend-read-crc-pipe-*) on ILK/SNB/IVB, affecting CI.

This happened with v1 but not with v2 of the patch.
Please check if somehow v1 ended up in your tree.

v2 passed CI fine, save for one warning not caused by the patch:
https://patchwork.freedesktop.org/series/4068/

For comparison, this was v1:
https://patchwork.freedesktop.org/patch/75840/

Best regards,

Lukas

> 
> Probably the same problem with the one in v2, but on older HW.
> 
> 
> commit a7442b93cf32c1e1ddb721a26cd1f92302e2a222
> Author: Lukas Wunner <lu...@wunner.de>
> Date:   Wed Mar 9 12:52:53 2016 +0100
> 
> drm/i915: Fix races on fbdev
> 
> The ->lastclose callback invokes intel_fbdev_restore_mode() and has
> been witnessed to run before intel_fbdev_initial_config_async()
> has finished.
> 
> We might likewise receive hotplug events before we've had a chance to
> fully set up the fbdev.
> 
> Fix by waiting for the asynchronous thread to finish.
> 
> v2:
> An async_synchronize_full() was also added to intel_fbdev_set_suspend()
> in v1 which turned out to be entirely gratuitous. It caused a deadlock
> on suspend (discovered by CI, thanks to Damien Lespiau and Tomi Sarvela
> for CI support) and was unnecessary since a device is never suspended
> until its ->probe callback (and all asynchronous tasks it scheduled)
> have finished. See dpm_prepare(), which calls wait_for_device_probe(),
> which calls async_synchronize_full().
> 
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=93580
> Reported-by: Gustav Fägerlind <gustav.fagerl...@gmail.com>
> Reported-by: "Li, Weinan Z" <weinan.z...@intel.com>
> Cc: Chris Wilson <ch...@chris-wilson.co.uk>
> Cc: sta...@vger.kernel.org
> Signed-off-by: Lukas Wunner <lu...@wunner.de>
> Signed-off-by: Daniel Vetter <daniel.vet...@ffwll.ch>
> Link: 
> http://patchwork.freedesktop.org/patch/msgid/20160309115147.67b2b6e...@gabe.freedesktop.org
> 
> 
> Regards,
> Gabriel
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH v5 00/12] Enable GPU switching on pre-retina?MacBook Pro

[Lukas Wunner]

Hi Alex,

On Tue, Mar 15, 2016 at 02:33:56PM -0400, Alex Deucher wrote:
> On Tue, Mar 15, 2016 at 1:54 PM, Lukas Wunner <lu...@wunner.de> wrote:
> > On Sat, Mar 05, 2016 at 01:10:56PM -0500, Alex Deucher wrote:
> >> Is there any reason to make use of the mux?
> >
> > Performance (lower latency => no need for framebuffer writes over PCIe),
> > improved battery life (no need to use 2 GPUs simultaneously).
> >
> > Technically you can't just ignore that the mux is there on the MBP
> > because the kernel has no control over the GPU used on boot.
> > (It's determined by EFI).
> >
> Is GPU power switching also handled by the mux?  Is it independent of
> the display mux?

Yes and yes.

Best regards,

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

Re: [Intel-gfx] [PATCH v5 00/12] Enable GPU switching on pre-retina?MacBook Pro

[Lukas Wunner]

Hi Alex,

On Sat, Mar 05, 2016 at 01:10:56PM -0500, Alex Deucher wrote:
> Is there any reason to make use of the mux?

Performance (lower latency => no need for framebuffer writes over PCIe),
improved battery life (no need to use 2 GPUs simultaneously).

Technically you can't just ignore that the mux is there on the MBP
because the kernel has no control over the GPU used on boot.
(It's determined by EFI).


> > I've heard that the AMD GPU is picky about external monitors and
> > doesn't recognize them unless they're plugged in at exactly the
> > right moment, so you may need to retry a couple of times until it
> > works.
> 
> Are talking about some issue specific to these muxed apple systems or
> in general?

Feedback I got from William Brown of Red Hat who tested the GPU switching
patches on an MBP8,2 and reported that (independently of the patches),
a display connected with an original Apple DP-to-DVI adapter would only
be recognized if plugged in at exactly the right moment and in the correct
order (first adapter, then display). However it doesn't seem to work
better on OS X.


> If you are having issues, please file a bug.

I'm not having issues so can't file a bug. Besides, filing a bug is no
guarantee that things get fixed. He had opened a bug for GPU switching
3 years ago (https://bugs.freedesktop.org/show_bug.cgi?id=61115) and
nobody did a thing. Obviously whether something gets fixed is a function
of the perceived importance by maintainers, unless a volunteer comes
along and does the dirty work.

Best regards,

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

Re: [Intel-gfx] [PATCH v5 00/12] Enable GPU switching on pre-retina?MacBook Pro

[Lukas Wunner]

Hi Bastien,

sorry for the delay.

On Sat, Mar 05, 2016 at 05:31:55PM +0100, Bastien Nocera wrote:
> We could, on boot, force using the integrated GPU, turning off the
> discrete GPU that we're not interested in.

Yes, many people "solve" this by having grub write the requisite commands
to gmux' I/O ports, however this approach won't work with gummiboot.
Also, the commands that need to be sent to gmux differ between retinas
and pre-retinas.


> So I'd push DIGD to the switch sysfs entry on boot. But I'm guessing
> that won't turn off the other output we're not interested in.

IGD and DIGD switch to the integrated GPU and also turn off the discrete
GPU. However if the machine is already switched to the integrated GPU,
the commands are no-ops and the discrete GPU is not turned off.

In other words you need to check (by reading the switch file) which GPU
the machine is switched to. If it's the integrated GPU, write OFF to the
switch file. If it's the discrete GPU, write DIGD to the switch file.

Once runtime pm works, writing DIGD is sufficient.


> DIGD and DDIS should help (you need to log out/log in again), and if
> the current GPU is the integrated one, you could force running, say, a
> game on the discrete GPU using DRI_PRIME=1, correct?

If runtime pm works, then yes. Otherwise you'd have to manually power up
the GPU before using DRI_PRIME=1 and power it down afterwards.


> FWIW, using OFF at runtime made my machine hang, and without any ways
> for me to get debug output.

Which GPU were you switched to? Did you issue the command while X11 was
running? Since the machine is switched to the AMD on boot, I guess you
were powering down the Intel GPU. This works on my machine, I get a log
entry "i915: switched off". If this doesn't work on your machine it might
be an i915 bug on your Sandy Bridge GPU or it might be because X11 is
running.

Try booting with "drm.debug=0xf log_buf_len=10M" and see if this gets you
log output. If it doesn't, netconsole might help if the hang occurs while
the console is locked.


> Ok, so using GIGD or DDIS would just switch the output, but not turn
> off the unused GPU without runtime PM management.

No. They do switch off the other GPU if runtime pm is disabled.


> > http://wunner.de/mbp_switcheroo_v5-4.5.tar.gz   => gpu switching
> > for 4.5
> That's the same patch that's already applied to the kernel above (the
> ones from this patchset thread), right?

I'm not sure, the patches in the copr repository might be an earlier
test version.


> My concerns here would be:
> - Do we know how to turn off the integrated GPU automatically, if the
> user only used the internal screen and wanted to use the discrete GPU?

Runtime pm is currently disabled by default for i915. The Intel folks
are on it. Until that works, the integrated GPU will be powered down
when the user manually switches to the discrete GPU with DIS / DDIS
and powered up when switching back with IGD / DIGD.


> - If only the discrete GPU is powered on, do we know how to power on
> the integrated GPU so it can drive the external screen when plugged in?

On the MBP5 2008/09 and MBP6 2010, the external DP port can be switched
between GPUs and we switch it together with the panel. So if you switch
to the discrete GPU, you can also drive the external DP port on these
models and the integrated GPU can be turned off.

On the MBP8 2011 and newer, external ports are combined DP/Thunderbolt
ports which can only be driven by the discrete GPU. For some reason the
HPD/AUX pins of the ports can still be switched but not the other pins.
We should nail these ports to the discrete GPU and not switch those pins
together with the panel as we currently do. There's a patch in
mbp_switcheroo_v5-4.5-runpm.tar.gz which fixes that. The patch also wakes
up the discrete GPU on hotplug, which obviously needs runtime pm.


> - While plymouth is short-lived at boot, Wayland and X11 GNOME sessions
> use the GPU. The first user session will run on a VT that's separate
> from the greeter to implement fast-user switching. So, if we wanted to
> force using the other GPU for future sessions, we'd need to tell gdm to
> kill its graphical session and to respawn it so it doesn't hog the GPU
> and avoid the switch happening. Correct?

I think so, yes.


> On the 8,2, still, and with the kernel from the COPR repo[1].
> 
> I tried running:
> echo DIGD > switch
> 
> to (later) switch to the integrated GPU. Log out, get to gdm, log back
> in to a black screen with the cursor visible and nothing else.
> 
> I'm wondering if it's the gdm X11 session running in the background
> that makes this fail.

That's possible, I have no idea. I have zero issues with GPU switching
on my Nvidia-based MacBook Pro, though I only switch on the console with
no X11 running. On the AMD-based MBP8 that you're using, the patches have
only been tested by William Brown, a colleague of yours at Red Hat
Australia.

If you send me dmesg output with "drm.debug=0xf log_buf_len=10M"
I 

Re: [Intel-gfx] ??? Fi.CI.BAT: failure for drm/i915: Fix races on fbdev (rev2)

[Lukas Wunner]

On Wed, Mar 09, 2016 at 12:06:24PM -, Patchwork wrote:
> == Series Details ==
> 
> Series: drm/i915: Fix races on fbdev (rev2)
> URL   : https://patchwork.freedesktop.org/series/4068/
> State : failure
> 
> == Summary ==
> 
> Series 4068v2 drm/i915: Fix races on fbdev
> http://patchwork.freedesktop.org/api/1.0/series/4068/revisions/2/mbox/
> 
> Test gem_ringfill:
> Subgroup basic-default-s3:
> pass   -> DMESG-WARN (bsw-nuc-2)

Unrelated to this patch according to paste kindly provided by Tomi
Sarvela: http://paste.debian.net/413850/

> Test kms_flip:
> Subgroup basic-flip-vs-modeset:
> dmesg-warn -> PASS   (bdw-ultra)
> Test kms_pipe_crc_basic:
> Subgroup suspend-read-crc-pipe-c:
> dmesg-warn -> PASS   (bsw-nuc-2)
> 
> bdw-nuci7total:186  pass:174  dwarn:0   dfail:0   fail:0   skip:12 
> bdw-ultratotal:186  pass:167  dwarn:0   dfail:0   fail:0   skip:19 
> bsw-nuc-2total:186  pass:150  dwarn:1   dfail:0   fail:0   skip:35 
> byt-nuc  total:186  pass:154  dwarn:0   dfail:0   fail:0   skip:32 
> skl-i5k-2total:186  pass:165  dwarn:0   dfail:0   fail:0   skip:21 
> skl-i7k-2total:186  pass:165  dwarn:0   dfail:0   fail:0   skip:21 
> 
> Results at /archive/results/CI_IGT_test/Patchwork_1548/
> 
> ab403b26610034afe0e0c97d960782bad98b97d0 drm-intel-nightly: 
> 2016y-03m-09d-09h-25m-31s UTC integration manifest
> a06328bd06f956c0b41202589175e68665c09ab1 drm/i915: Fix races on fbdev
> 
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Intel-gfx] [PATCH v2] drm/i915: Fix races on fbdev

[Lukas Wunner]

The ->lastclose callback invokes intel_fbdev_restore_mode() and has
been witnessed to run before intel_fbdev_initial_config_async()
has finished.

We might likewise receive hotplug events before we've had a chance to
fully set up the fbdev.

Fix by waiting for the asynchronous thread to finish.

v2:
An async_synchronize_full() was also added to intel_fbdev_set_suspend()
in v1 which turned out to be entirely gratuitous. It caused a deadlock
on suspend (discovered by CI, thanks to Damien Lespiau and Tomi Sarvela
for CI support) and was unnecessary since a device is never suspended
until its ->probe callback (and all asynchronous tasks it scheduled)
have finished. See dpm_prepare(), which calls wait_for_device_probe(),
which calls async_synchronize_full().

Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=93580
Reported-by: Gustav Fägerlind <gustav.fagerl...@gmail.com>
Reported-by: "Li, Weinan Z" <weinan.z...@intel.com>
Cc: Chris Wilson <ch...@chris-wilson.co.uk>
Cc: sta...@vger.kernel.org
Signed-off-by: Lukas Wunner <lu...@wunner.de>
---
 drivers/gpu/drm/i915/i915_dma.c| 8 +++-
 drivers/gpu/drm/i915/intel_fbdev.c | 3 +++
 2 files changed, 6 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
index 4aa3db6..9d76dfb 100644
--- a/drivers/gpu/drm/i915/i915_dma.c
+++ b/drivers/gpu/drm/i915/i915_dma.c
@@ -430,11 +430,9 @@ static int i915_load_modeset_init(struct drm_device *dev)
 * Some ports require correctly set-up hpd registers for detection to
 * work properly (leading to ghost connected connector status), e.g. VGA
 * on gm45.  Hence we can only set up the initial fbdev config after hpd
-* irqs are fully enabled. Now we should scan for the initial config
-* only once hotplug handling is enabled, but due to screwed-up locking
-* around kms/fbdev init we can't protect the fdbev initial config
-* scanning against hotplug events. Hence do this first and ignore the
-* tiny window where we will loose hotplug notifactions.
+* irqs are fully enabled. We protect the fbdev initial config scanning
+* against hotplug events by waiting in intel_fbdev_output_poll_changed
+* until the asynchronous thread has finished.
 */
intel_fbdev_initial_config_async(dev);
 
diff --git a/drivers/gpu/drm/i915/intel_fbdev.c 
b/drivers/gpu/drm/i915/intel_fbdev.c
index ae9cf6f..7e73acc 100644
--- a/drivers/gpu/drm/i915/intel_fbdev.c
+++ b/drivers/gpu/drm/i915/intel_fbdev.c
@@ -800,6 +800,8 @@ void intel_fbdev_set_suspend(struct drm_device *dev, int 
state, bool synchronous
 void intel_fbdev_output_poll_changed(struct drm_device *dev)
 {
struct drm_i915_private *dev_priv = dev->dev_private;
+
+   async_synchronize_full();
if (dev_priv->fbdev)
drm_fb_helper_hotplug_event(_priv->fbdev->helper);
 }
@@ -811,6 +813,7 @@ void intel_fbdev_restore_mode(struct drm_device *dev)
struct intel_fbdev *ifbdev = dev_priv->fbdev;
struct drm_fb_helper *fb_helper;
 
+   async_synchronize_full();
if (!ifbdev)
return;
 
-- 
1.8.5.2 (Apple Git-48)

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

Re: [Intel-gfx] [PATCH v5 00/12] Enable GPU switching on pre-retina?MacBook Pro

[Lukas Wunner]

Hi Bastien,

On Fri, Mar 04, 2016 at 04:12:27PM +, Bastien Nocera wrote:
> Lukas Wunner  wunner.de> writes:
> > Enable GPU switching on the pre-retina MacBook Pro (2008 - 2013), v5.
> 
> I've tested your patchset on a MacBookPro8,1, with an integrated Intel and
> discrete AMD/ATI GPUs.

Hm, it must be either an 8,2 or 8,3. The 8,1 was a 13" machine and only
had an integrated GPU.


> I've used the COPR repository here to cut down on my compilation time:
> https://copr.fedorainfracloud.org/coprs/firstyear/kernel-mbp/
> 
> I'm not certain how to test out your changes, or what the consequences should
> be on a stock Fedora 23/GNOME 3.18 installation. After booting (note that I
> did not change any command-line options in grub), a gnome-shell/gdm X11
> session comes up (I disabled Wayland, to rule out behavioural changes), I'd
> log in to GNOME and gnome-shell (which starts another X11 session on
> another VT).

Switching and power control currently requires manual intervention
by echoing commands to /sys/kernel/debug/vgaswitcheroo/switch
as documented here:
https://01.org/linuxgraphics/gfx-docs/drm/modes_of_use.html

As you've correctly observed, the machine is initially switched to
the discrete GPU and both GPUs are turned on. By echoing "IGD" to
the sysfs file, you'll switch to the integrated GPU and turn off
the discrete GPU.

It's possible to let the EFI firmware switch to the integrated GPU
on boot by using this tool: https://github.com/0xbb/gpu-switch
However still both GPUs will be powered up, so you have to issue
the "OFF" command to sysfs to power the discrete GPU down. Also,
once you boot into OS X, the setting made by the gpu-switch tool
will be overwritten and the machine will be switched to the discrete
GPU again the next time you boot Linux.

Note that switching is only possible from the text console, with
X11/Wayland shut down. Obviously this is not great in terms of UX.
A few years ago there was a GSoC proposal to get hot GPU switching
to work on Linux (akin to what OS X does) but nothing ever came of it:
http://www.phoronix.com/scan.php?page=news_item=OTIyMQ
https://lists.x.org/archives/xorg/2011-March/052522.html

Unfortunately this seems to be a low priority item for kernel graphics
developers since nowadays most dual GPU notebooks no longer have a mux
and cannot switch. The MacBook Pro seems to be the last one supporting
this but I've witnessed a bit of an anti-Apple sentiment among kernel
graphics developers since everything is non-standard there. Which is
unfortunate because these machines have a large market share and Apple
software quality is deteriorating rapidly so a lot of Mac users are
ripe for converting to Linux.

Anyway, one short-term improvement will be to add runtime pm support
(called "Driver power control" in the vga_switcheroo documentation
linked above). That way it'll no longer be necessary to power the
discrete GPU up and down manually, this will happen automatically
as needed (when switching or using render offloading with DRI PRIME).
I have patches to enable this for radeon but they're completely untested:

http://wunner.de/mbp_switcheroo_v5-4.5.tar.gz   => gpu switching for 4.5
http://wunner.de/mbp_switcheroo_v5-4.5-runpm.tar.gz => runtime pm for radeon

I have an Nvidia based machine and runtime pm doesn't work there yet
because of bugs in nouveau that I haven't had the time to look into.


> I did not use any external screens to test this.

Since your machine has Thunderbolt, the external port is no longer
switchable between GPUs, it can only be driven by the discrete GPU.
So you need to power it up manually for this to work. You don't need
to switch to it, but it's probably recommendable to save energy.
(Otherwise both GPUs are on with the integrated GPU driving the panel
and the discrete GPU driving the DP port.)

The runpm tarball linked above contains a patch to automatically
wake the discrete GPU on hotplug.

I've heard that the AMD GPU is picky about external monitors and
doesn't recognize them unless they're plugged in at exactly the
right moment, so you may need to retry a couple of times until it
works.

Best regards,

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

Re: [Intel-gfx] [PATCH RESEND FOR CI] drm/i915: Fix races on fbdev

[Lukas Wunner]

Hi Damien, Hi Daniel,

I've submitted the patch below for the third time now in an attempt
to get CI to test it, again to no avail. This time I didn't set the
In-Reply-To header and only submitted it as a single patch instead
of as a series because I expected this might confuse CI.

Nevertheless CI misclassified it under "Series" instead of "Patches",
reports that the series consists of 0 patches and doesn't test it:
https://patchwork.freedesktop.org/series/4068/

Earlier attempts:
https://patchwork.freedesktop.org/series/3453/
https://patchwork.freedesktop.org/series/3126/

Sorry, I give up.

Best regards,

Lukas

On Thu, Mar 03, 2016 at 06:02:53PM +0100, Lukas Wunner wrote:
> The ->lastclose callback invokes intel_fbdev_restore_mode() and has
> been witnessed to run before intel_fbdev_initial_config_async()
> has finished.
> 
> We might likewise receive hotplug events or be suspended before
> we've had a chance to fully set up the fbdev.
> 
> Fix by waiting for the asynchronous thread to finish.
> 
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=93580
> Reported-by: Gustav Fägerlind <gustav.fagerl...@gmail.com>
> Reported-by: "Li, Weinan Z" <weinan.z...@intel.com>
> Cc: Chris Wilson <ch...@chris-wilson.co.uk>
> Cc: sta...@vger.kernel.org
> Signed-off-by: Lukas Wunner <lu...@wunner.de>
> ---
>  drivers/gpu/drm/i915/i915_dma.c| 8 +++-
>  drivers/gpu/drm/i915/intel_fbdev.c | 4 
>  2 files changed, 7 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
> index 4aa3db6..9d76dfb 100644
> --- a/drivers/gpu/drm/i915/i915_dma.c
> +++ b/drivers/gpu/drm/i915/i915_dma.c
> @@ -430,11 +430,9 @@ static int i915_load_modeset_init(struct drm_device *dev)
>* Some ports require correctly set-up hpd registers for detection to
>* work properly (leading to ghost connected connector status), e.g. VGA
>* on gm45.  Hence we can only set up the initial fbdev config after hpd
> -  * irqs are fully enabled. Now we should scan for the initial config
> -  * only once hotplug handling is enabled, but due to screwed-up locking
> -  * around kms/fbdev init we can't protect the fdbev initial config
> -  * scanning against hotplug events. Hence do this first and ignore the
> -  * tiny window where we will loose hotplug notifactions.
> +  * irqs are fully enabled. We protect the fbdev initial config scanning
> +  * against hotplug events by waiting in intel_fbdev_output_poll_changed
> +  * until the asynchronous thread has finished.
>*/
>   intel_fbdev_initial_config_async(dev);
>  
> diff --git a/drivers/gpu/drm/i915/intel_fbdev.c 
> b/drivers/gpu/drm/i915/intel_fbdev.c
> index ae9cf6f..936c3d7 100644
> --- a/drivers/gpu/drm/i915/intel_fbdev.c
> +++ b/drivers/gpu/drm/i915/intel_fbdev.c
> @@ -754,6 +754,7 @@ void intel_fbdev_set_suspend(struct drm_device *dev, int 
> state, bool synchronous
>   struct intel_fbdev *ifbdev = dev_priv->fbdev;
>   struct fb_info *info;
>  
> + async_synchronize_full();
>   if (!ifbdev)
>   return;
>  
> @@ -800,6 +801,8 @@ void intel_fbdev_set_suspend(struct drm_device *dev, int 
> state, bool synchronous
>  void intel_fbdev_output_poll_changed(struct drm_device *dev)
>  {
>   struct drm_i915_private *dev_priv = dev->dev_private;
> +
> + async_synchronize_full();
>   if (dev_priv->fbdev)
>   drm_fb_helper_hotplug_event(_priv->fbdev->helper);
>  }
> @@ -811,6 +814,7 @@ void intel_fbdev_restore_mode(struct drm_device *dev)
>   struct intel_fbdev *ifbdev = dev_priv->fbdev;
>   struct drm_fb_helper *fb_helper;
>  
> + async_synchronize_full();
>   if (!ifbdev)
>   return;
>  
> -- 
> 1.8.5.2 (Apple Git-48)
> 
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Intel-gfx] [PATCH RESEND FOR CI] drm/i915: Fix races on fbdev

[Lukas Wunner]

The ->lastclose callback invokes intel_fbdev_restore_mode() and has
been witnessed to run before intel_fbdev_initial_config_async()
has finished.

We might likewise receive hotplug events or be suspended before
we've had a chance to fully set up the fbdev.

Fix by waiting for the asynchronous thread to finish.

Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=93580
Reported-by: Gustav Fägerlind <gustav.fagerl...@gmail.com>
Reported-by: "Li, Weinan Z" <weinan.z...@intel.com>
Cc: Chris Wilson <ch...@chris-wilson.co.uk>
Cc: sta...@vger.kernel.org
Signed-off-by: Lukas Wunner <lu...@wunner.de>
---
 drivers/gpu/drm/i915/i915_dma.c| 8 +++-
 drivers/gpu/drm/i915/intel_fbdev.c | 4 
 2 files changed, 7 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
index 4aa3db6..9d76dfb 100644
--- a/drivers/gpu/drm/i915/i915_dma.c
+++ b/drivers/gpu/drm/i915/i915_dma.c
@@ -430,11 +430,9 @@ static int i915_load_modeset_init(struct drm_device *dev)
 * Some ports require correctly set-up hpd registers for detection to
 * work properly (leading to ghost connected connector status), e.g. VGA
 * on gm45.  Hence we can only set up the initial fbdev config after hpd
-* irqs are fully enabled. Now we should scan for the initial config
-* only once hotplug handling is enabled, but due to screwed-up locking
-* around kms/fbdev init we can't protect the fdbev initial config
-* scanning against hotplug events. Hence do this first and ignore the
-* tiny window where we will loose hotplug notifactions.
+* irqs are fully enabled. We protect the fbdev initial config scanning
+* against hotplug events by waiting in intel_fbdev_output_poll_changed
+* until the asynchronous thread has finished.
 */
intel_fbdev_initial_config_async(dev);
 
diff --git a/drivers/gpu/drm/i915/intel_fbdev.c 
b/drivers/gpu/drm/i915/intel_fbdev.c
index ae9cf6f..936c3d7 100644
--- a/drivers/gpu/drm/i915/intel_fbdev.c
+++ b/drivers/gpu/drm/i915/intel_fbdev.c
@@ -754,6 +754,7 @@ void intel_fbdev_set_suspend(struct drm_device *dev, int 
state, bool synchronous
struct intel_fbdev *ifbdev = dev_priv->fbdev;
struct fb_info *info;
 
+   async_synchronize_full();
if (!ifbdev)
return;
 
@@ -800,6 +801,8 @@ void intel_fbdev_set_suspend(struct drm_device *dev, int 
state, bool synchronous
 void intel_fbdev_output_poll_changed(struct drm_device *dev)
 {
struct drm_i915_private *dev_priv = dev->dev_private;
+
+   async_synchronize_full();
if (dev_priv->fbdev)
drm_fb_helper_hotplug_event(_priv->fbdev->helper);
 }
@@ -811,6 +814,7 @@ void intel_fbdev_restore_mode(struct drm_device *dev)
struct intel_fbdev *ifbdev = dev_priv->fbdev;
struct drm_fb_helper *fb_helper;
 
+   async_synchronize_full();
if (!ifbdev)
return;
 
-- 
1.8.5.2 (Apple Git-48)

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

Re: [Intel-gfx] [PATCH v5 10/12] drm/i915: Defer probe if gmux is present but its driver isn't

[Lukas Wunner]

Hi,

On Thu, Feb 18, 2016 at 10:39:05PM +0100, Daniel Vetter wrote:
> On Thu, Feb 18, 2016 at 9:34 PM, Lukas Wunner <lu...@wunner.de> wrote:
> >
> >> Ok, makes sense. I still think adding the check to the client_register
> >> function would be good, just as a safety measure.
> >
> > Hm, the idea of calling vga_switcheroo_client_probe_defer() twice
> > causes me pain in the stomach. It's surprising for drivers which
> > just don't need it at the moment (amdgpu and snd_hda_intel) and
> > it feels like overengineering and pampering driver developers
> > beyond reasonable measure. Also while the single existing check is
> > cheap, we might later on add checks that take more time and slow
> > things down.
> 
> It's motivated by Rusty's API Manifesto:
> 
> http://sweng.the-davies.net/Home/rustys-api-design-manifesto

Interesting, thank you.


> With the mandatory check in _register we reach level 5 - it'll blow up
> at runtime when we try to register it.

The manifesto says "5. Do it right or it will always break at runtime".

However even if we add a WARN_ON(vga_switcheroo_client_probe_defer(pdev))
to register_client(), it will not *always* spew a stacktrace but only on
the machines this concerns (MacBook Pros). Since failure to defer breaks
GPU switching, level 5 is already reached. Chances are this won't go
unnoticed by the user.


> For more context: We have tons of fun with EPROBE_DEFER handling
> between i915 and snd-hda

I don't understand, there is currently not a single occurrence of
EPROBE_DEFER in i915, apart from the one I added.

In sound/ there are 88 occurrences of EPROBE_DEFER in soc/, plus 1 in
ppc/ and that's it. So not a single one in pci/hda/ where hda_intel.c
resides.

Is the fun with EPROBE_DEFER handling caused by the lack thereof?


Best regards,

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

Re: [Intel-gfx] [PATCH v5 10/12] drm/i915: Defer probe if gmux is present but its driver isn't

[Lukas Wunner]

Hi,

On Tue, Feb 16, 2016 at 05:08:40PM +0100, Daniel Vetter wrote:
> On Tue, Feb 16, 2016 at 04:58:20PM +0100, Lukas Wunner wrote:
> > On Sun, Feb 14, 2016 at 01:46:28PM +0100, Daniel Vetter wrote:
> > > On Sun, Feb 14, 2016 at 1:10 PM, Lukas Wunner <lu...@wunner.de> wrote:
> > > > + * DRM drivers should invoke this early on in their ->probe callback 
> > > > and return
> > > > + * %-EPROBE_DEFER if it evaluates to %true. The GPU need not be 
> > > > registered with
> > > > + * vga_switcheroo_register_client() beforehand.
> > > 
> > > s/need not/must not/ ... is your native language German by any chance?
> > 
> > In principle there's no harm in registering the client first,
> > then checking if probing should be deferred, as long as the
> > client is unregistered before deferring. Thus the language
> > above is intentionally "need not" (muss nicht) rather than
> > "must not" (darf nicht). I didn't want to mandate something
> > that isn't actually required. The above sentence is merely
> > an aid for driver developers who might be confused in which
> > order to call what.
> 
> I'd reject any driver that does this, registering, then checking, then
> unregistering seems extermely unsafe. I'd really stick with mandatory
> language here to make this clear.

Ok, I've made it mandatory in the kerneldoc, updated patch follows below.


> Ok, makes sense. I still think adding the check to the client_register
> function would be good, just as a safety measure.

Hm, the idea of calling vga_switcheroo_client_probe_defer() twice
causes me pain in the stomach. It's surprising for drivers which
just don't need it at the moment (amdgpu and snd_hda_intel) and
it feels like overengineering and pampering driver developers
beyond reasonable measure. Also while the single existing check is
cheap, we might later on add checks that take more time and slow
things down.

Best regards,

Lukas

-- >8 --
Subject: [PATCH] vga_switcheroo: Add helper for deferred probing

So far we've got one condition when DRM drivers need to defer probing
on a dual GPU system and it's coded separately into each of the relevant
drivers. As suggested by Daniel Vetter, deduplicate that code in the
drivers and move it to a new vga_switcheroo helper. This yields better
encapsulation of concepts and lets us add further checks in a central
place. (The existing check pertains to pre-retina MacBook Pros and an
additional check is expected to be needed for retinas.)

v2: This helper could eventually be used by audio clients as well,
so rephrase kerneldoc to refer to "client" instead of "GPU"
and move the single existing check in an if block specific
to PCI_CLASS_DISPLAY_VGA devices. Move documentation on
that check from kerneldoc to a comment. (Daniel Vetter)

v3: Mandate in kerneldoc that registration of client shall only
happen after calling this helper. (Daniel Vetter)

Cc: Daniel Vetter <daniel.vet...@ffwll.ch>
Cc: Ben Skeggs <bske...@redhat.com>
Cc: Alex Deucher <alexander.deuc...@amd.com>
Signed-off-by: Lukas Wunner <lu...@wunner.de>
---
 drivers/gpu/drm/i915/i915_drv.c   | 10 +-
 drivers/gpu/drm/nouveau/nouveau_drm.c | 10 +-
 drivers/gpu/drm/radeon/radeon_drv.c   | 10 +-
 drivers/gpu/vga/vga_switcheroo.c  | 34 --
 include/linux/vga_switcheroo.h|  2 ++
 5 files changed, 37 insertions(+), 29 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 44912ec..80cfd73 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -35,11 +35,9 @@
 #include "i915_trace.h"
 #include "intel_drv.h"
 
-#include 
 #include 
 #include 
 #include 
-#include 
 #include 
 #include 
 
@@ -972,13 +970,7 @@ static int i915_pci_probe(struct pci_dev *pdev, const 
struct pci_device_id *ent)
if (PCI_FUNC(pdev->devfn))
return -ENODEV;
 
-   /*
-* apple-gmux is needed on dual GPU MacBook Pro
-* to probe the panel if we're the inactive GPU.
-*/
-   if (IS_ENABLED(CONFIG_VGA_ARB) && IS_ENABLED(CONFIG_VGA_SWITCHEROO) &&
-   apple_gmux_present() && pdev != vga_default_device() &&
-   !vga_switcheroo_handler_flags())
+   if (vga_switcheroo_client_probe_defer(pdev))
return -EPROBE_DEFER;
 
return drm_get_pci_dev(pdev, ent, );
diff --git a/drivers/gpu/drm/nouveau/nouveau_drm.c 
b/drivers/gpu/drm/nouveau/nouveau_drm.c
index bb8498c..9141bcd 100644
--- a/drivers/gpu/drm/nouveau/nouveau_drm.c
+++ b/drivers/gpu/drm/nouveau/nouveau_drm.c
@@ -22,13 +22,11 @@
  * Authors: Ben Skeggs
  */
 
-#include 
 #include 
 #include 
 #include 
 #incl

Re: [Intel-gfx] [PATCH v5 10/12] drm/i915: Defer probe if gmux is present but its driver isn't

[Lukas Wunner]

Hi,

On Sun, Feb 14, 2016 at 01:46:28PM +0100, Daniel Vetter wrote:
> On Sun, Feb 14, 2016 at 1:10 PM, Lukas Wunner <lu...@wunner.de> wrote:
> >  /**
> > + * vga_switcheroo_client_probe_defer() - whether to defer probing a given 
> > GPU
> > + * @pdev: pci device of GPU
> > + *
> > + * Determine whether any prerequisites are not fulfilled to probe a given 
> > GPU.
> 
> I'd phrase this as "Determine whether the vgaswitcheroor support is
> fully loaded" and drop the GPU part - it could be needed by snd
> drivers eventually too.

Ok, I've rephrased the kerneldoc to refer to "client" instead of "GPU"
and moved the single existing check in an if block for
PCI_CLASS_DISPLAY_VGA devices.


> > + * DRM drivers should invoke this early on in their ->probe callback and 
> > return
> > + * %-EPROBE_DEFER if it evaluates to %true. The GPU need not be registered 
> > with
> > + * vga_switcheroo_register_client() beforehand.
> 
> s/need not/must not/ ... is your native language German by any chance?

In principle there's no harm in registering the client first,
then checking if probing should be deferred, as long as the
client is unregistered before deferring. Thus the language
above is intentionally "need not" (muss nicht) rather than
"must not" (darf nicht). I didn't want to mandate something
that isn't actually required. The above sentence is merely
an aid for driver developers who might be confused in which
order to call what.


> Aside from that ... should vga_switcheroo_register_client call this
> helper instead directly and return the appropriate -EDEFER_PROBE
> error? I realize for some drivers it might be way too late to unrol
> from that point on, but e.g. i915 already uses -EDEFER_PROBE. And
> calling it unconditionally will make sure that it's easier to notice
> when people forget this. So maybe call it both from the register
> functions, and export as a separate hook? And for i915 we should be
> able to remove that early check entirely.

I'm afraid that wouldn't be a good idea. The ->probe hook is
potentially called dozens of times during boot until it finally
succeeds and vga_switcheroo_register_client() is usually called
in a fairly late driver load stage. In i915, we already have a
working GEM at that point and an almost fully brought up GPU.
Repeating bring up and tear down dozens of times is a nice
stress test but nothing I'd inflict on production systems.
I imagine it would also severely impact boot time and
clutter the kernel log.

So a separate helper seems to be the only sensible option.


> > + *
> > + * Return: %false unless one of the following applies:
> > + *
> > + * * On pre-retina MacBook Pros, the apple-gmux driver is needed to 
> > temporarily
> > + *   switch DDC to the inactive GPU so that it can probe the panel's EDID.
> > + *   Therefore return %true if gmux is built into the machine, @pdev is the
> > + *   inactive GPU and the apple-gmux driver has not registered its handler
> > + *   flags, signifying it has not yet loaded or has not finished 
> > initializing.
> 
> I think the apple-specific comment here should be a separate comment
> right above the if condition below - it doesn't explain the interface,
> only one specific case. And we might grow more of those.

Ok, I've moved that to a comment.

Updated patch follows after the scissors & perforation.

Thanks for the feedback!

Lukas

-- >8 --
Subject: [PATCH] vga_switcheroo: Allow clients to determine whether to defer
 probing

So far we've got one condition when DRM drivers need to defer probing
on a dual GPU system and it's coded separately into each of the relevant
drivers. As suggested by Daniel Vetter, deduplicate that code in the
drivers and move it to a new vga_switcheroo helper. This yields better
encapsulation of concepts and lets us add further checks in a central
place. (The existing check pertains to pre-retina MacBook Pros and an
additional check is expected to be needed for retinas.)

v2: This helper could eventually be used by audio clients as well,
so rephrase kerneldoc to refer to "client" instead of "GPU"
and move the single existing check in an if block specific
to PCI_CLASS_DISPLAY_VGA devices. Move documentation on
that check from kerneldoc to a comment. (Daniel Vetter)

Cc: Daniel Vetter <daniel.vet...@ffwll.ch>
Cc: Ben Skeggs <bske...@redhat.com>
Cc: Alex Deucher <alexander.deuc...@amd.com>
Signed-off-by: Lukas Wunner <lu...@wunner.de>
---
 drivers/gpu/drm/i915/i915_drv.c   | 10 +-
 drivers/gpu/drm/nouveau/nouveau_drm.c | 10 +-
 drivers/gpu/drm/radeon/radeon_drv.c   | 10 +-
 drivers/gpu/vga/vga_switcheroo.c  | 28 
 include/li

[Intel-gfx] [PATCH 0/1] drm/i915: Fix races on fbdev [RESEND FOR CI]

[Lukas Wunner]

Originally submitted inline with <20160205145831.ga14...@wunner.de>,
hereby resubmitted standalone for CI as requested by Daniel with
<20160215163251.GR11240@phenom.ffwll.local>.

Above-mentioned message contained the following important note:

> However AFAICT a corner case remains where we're still vulnerable to races:
> async_schedule() runs synchronously "if we're out of memory or if there's
> too much work pending already" (see __async_schedule() in kernel/async.c).
> In this case no entry is added to the pending list and
> async_synchronize_full() might theoretically return before the synchronously
> executed function has finished.
>
> The existing call to async_synchronize_full() in intel_fbdev_fini()
> seems to be susceptible to the same race condition, but it's probably
> very hard to trigger it. (Unload the module before the fbdev is fully
> initialized.)
>
> To make it bullet proof we could declare a completion in intel_fbdev.c
> and wait for that instead. Opinions?


Lukas Wunner (1):
  drm/i915: Fix races on fbdev

 drivers/gpu/drm/i915/i915_dma.c| 8 +++-
 drivers/gpu/drm/i915/intel_fbdev.c | 4 
 2 files changed, 7 insertions(+), 5 deletions(-)

-- 
1.8.5.2 (Apple Git-48)

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

[Intel-gfx] [PATCH 1/1] drm/i915: Fix races on fbdev

[Lukas Wunner]

The ->lastclose callback invokes intel_fbdev_restore_mode() and has
been witnessed to run before intel_fbdev_initial_config_async()
has finished.

We might likewise receive hotplug events or be suspended before
we've had a chance to fully set up the fbdev.

Fix by waiting for the asynchronous thread to finish.

Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=93580
Reported-by: Gustav Fägerlind <gustav.fagerl...@gmail.com>
Reported-by: "Li, Weinan Z" <weinan.z...@intel.com>
Cc: Chris Wilson <ch...@chris-wilson.co.uk>
Cc: sta...@vger.kernel.org
Signed-off-by: Lukas Wunner <lu...@wunner.de>
---
 drivers/gpu/drm/i915/i915_dma.c| 8 +++-
 drivers/gpu/drm/i915/intel_fbdev.c | 4 
 2 files changed, 7 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
index a0f5659..a76b528 100644
--- a/drivers/gpu/drm/i915/i915_dma.c
+++ b/drivers/gpu/drm/i915/i915_dma.c
@@ -437,11 +437,9 @@ static int i915_load_modeset_init(struct drm_device *dev)
 * Some ports require correctly set-up hpd registers for detection to
 * work properly (leading to ghost connected connector status), e.g. VGA
 * on gm45.  Hence we can only set up the initial fbdev config after hpd
-* irqs are fully enabled. Now we should scan for the initial config
-* only once hotplug handling is enabled, but due to screwed-up locking
-* around kms/fbdev init we can't protect the fdbev initial config
-* scanning against hotplug events. Hence do this first and ignore the
-* tiny window where we will loose hotplug notifactions.
+* irqs are fully enabled. We protect the fbdev initial config scanning
+* against hotplug events by waiting in intel_fbdev_output_poll_changed
+* until the asynchronous thread has finished.
 */
intel_fbdev_initial_config_async(dev);
 
diff --git a/drivers/gpu/drm/i915/intel_fbdev.c 
b/drivers/gpu/drm/i915/intel_fbdev.c
index 09840f4..2002b13 100644
--- a/drivers/gpu/drm/i915/intel_fbdev.c
+++ b/drivers/gpu/drm/i915/intel_fbdev.c
@@ -749,6 +749,7 @@ void intel_fbdev_set_suspend(struct drm_device *dev, int 
state, bool synchronous
struct intel_fbdev *ifbdev = dev_priv->fbdev;
struct fb_info *info;
 
+   async_synchronize_full();
if (!ifbdev)
return;
 
@@ -795,6 +796,8 @@ void intel_fbdev_set_suspend(struct drm_device *dev, int 
state, bool synchronous
 void intel_fbdev_output_poll_changed(struct drm_device *dev)
 {
struct drm_i915_private *dev_priv = dev->dev_private;
+
+   async_synchronize_full();
if (dev_priv->fbdev)
drm_fb_helper_hotplug_event(_priv->fbdev->helper);
 }
@@ -806,6 +809,7 @@ void intel_fbdev_restore_mode(struct drm_device *dev)
struct intel_fbdev *ifbdev = dev_priv->fbdev;
struct drm_fb_helper *fb_helper;
 
+   async_synchronize_full();
if (!ifbdev)
return;
 
-- 
1.8.5.2 (Apple Git-48)

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

Re: [Intel-gfx] [PATCH v5 10/12] drm/i915: Defer probe if gmux is present but its driver isn't

[Lukas Wunner]

Hi,

On Tue, Feb 09, 2016 at 10:04:01AM +0100, Daniel Vetter wrote:
> On Mon, Jan 11, 2016 at 08:09:20PM +0100, Lukas Wunner wrote:
[...]
> > @@ -967,6 +970,15 @@ static int i915_pci_probe(struct pci_dev *pdev, const 
> > struct pci_device_id *ent)
> > if (PCI_FUNC(pdev->devfn))
> > return -ENODEV;
> >  
> > +   /*
> > +* apple-gmux is needed on dual GPU MacBook Pro
> > +* to probe the panel if we're the inactive GPU.
> > +*/
> > +   if (IS_ENABLED(CONFIG_VGA_ARB) && IS_ENABLED(CONFIG_VGA_SWITCHEROO) &&
> > +   apple_gmux_present() && pdev != vga_default_device() &&
> > +   !vga_switcheroo_handler_flags())
> > +   return -EPROBE_DEFER;
> 
> I pulled in all patches to drm-misc, but this here is imo ugly and needs
> to be polished a bit. What about adding a vga_switcheroo_ready() function
> which contains this check (and might in the future contain even more
> checks)? Then i915/radeon/nouveau would just have a simple
> 
>   if (!vga_switcheroo_ready())
>   return -EPROBE_DEFER;
> 
> and instead of duplicating the same comment 3 times we could have it once
> in one place. Plus some neat kerneldoc for this new helper to describe how
> it's supposed to be used. Plus better encapsulation of concepts.
> 
> Can you pls follow up with a patch/series to do that?

You're right, this is indeed much better. It also allows me to drop the
IS_ENABLED(CONFIG_VGA_ARB) and IS_ENABLED(CONFIG_VGA_SWITCHEROO) checks.

A patch follows below after the scissors.

The name vga_switcheroo_ready() was already taken by a static function
in vga_switcheroo.c, so I've named it vga_switcheroo_client_probe_defer().
If anyone has a suggestion for a better name I'll be happy to amend the
patch.

I've switched all three drivers to the new helper within the same patch
but will gladly spin this out into one patch per driver if preferred.

Thank you!

Lukas

-- >8 --
Subject: [PATCH] vga_switcheroo: Allow clients to determine whether to defer
 probing

So far we've got one condition when DRM drivers need to defer probing
on a dual GPU system and it's coded separately into each of the relevant
drivers. As suggested by Daniel Vetter, deduplicate that code in the
drivers and move it to a new vga_switcheroo helper. This yields better
encapsulation of concepts and lets us add further checks in a central
place. (The existing check pertains to pre-retina MacBook Pros and an
additional check is expected to be needed for retinas.)

Cc: Daniel Vetter <daniel.vet...@ffwll.ch>
Cc: Ben Skeggs <bske...@redhat.com>
Cc: Alex Deucher <alexander.deuc...@amd.com>
Signed-off-by: Lukas Wunner <lu...@wunner.de>
---
 drivers/gpu/drm/i915/i915_drv.c   | 10 +-
 drivers/gpu/drm/nouveau/nouveau_drm.c | 10 +-
 drivers/gpu/drm/radeon/radeon_drv.c   | 10 +-
 drivers/gpu/vga/vga_switcheroo.c  | 28 
 include/linux/vga_switcheroo.h|  2 ++
 5 files changed, 33 insertions(+), 27 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 44912ec..80cfd73 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -35,11 +35,9 @@
 #include "i915_trace.h"
 #include "intel_drv.h"
 
-#include 
 #include 
 #include 
 #include 
-#include 
 #include 
 #include 
 
@@ -972,13 +970,7 @@ static int i915_pci_probe(struct pci_dev *pdev, const 
struct pci_device_id *ent)
if (PCI_FUNC(pdev->devfn))
return -ENODEV;
 
-   /*
-* apple-gmux is needed on dual GPU MacBook Pro
-* to probe the panel if we're the inactive GPU.
-*/
-   if (IS_ENABLED(CONFIG_VGA_ARB) && IS_ENABLED(CONFIG_VGA_SWITCHEROO) &&
-   apple_gmux_present() && pdev != vga_default_device() &&
-   !vga_switcheroo_handler_flags())
+   if (vga_switcheroo_client_probe_defer(pdev))
return -EPROBE_DEFER;
 
return drm_get_pci_dev(pdev, ent, );
diff --git a/drivers/gpu/drm/nouveau/nouveau_drm.c 
b/drivers/gpu/drm/nouveau/nouveau_drm.c
index bb8498c..9141bcd 100644
--- a/drivers/gpu/drm/nouveau/nouveau_drm.c
+++ b/drivers/gpu/drm/nouveau/nouveau_drm.c
@@ -22,13 +22,11 @@
  * Authors: Ben Skeggs
  */
 
-#include 
 #include 
 #include 
 #include 
 #include 
 #include 
-#include 
 #include 
 
 #include "drmP.h"
@@ -314,13 +312,7 @@ static int nouveau_drm_probe(struct pci_dev *pdev,
bool boot = false;
int ret;
 
-   /*
-* apple-gmux is needed on dual GPU MacBook Pro
-* to probe the panel if we're the inactive GPU.
-*/
-   if (IS_ENABLED(CONFIG_VGA_ARB) && IS_ENABLED(CONFIG_VGA_SWITCHEROO) &&
-   apple_gmux_present() && pdev != 

Re: [Intel-gfx] [PATCH 10/10] drm/i915: Disable use of stolen area by User when Intel RST is present

[Lukas Wunner]

Hi Ankitprasad,

On Thu, Feb 04, 2016 at 05:43:17PM +0100, Lukas Wunner wrote:
> On Thu, Feb 04, 2016 at 04:05:04PM +, Chris Wilson wrote:
> > We could #define INTEL_RAPID_START "INT3392" for
> > if (IS_ENABLED(CONFIG_SUSPEND) && acpi_dev_present(INTEL_RAPID_START))
> > but Anki wanted to keep the acpi details themselves out of stolen (hence
> > the current split).
> 
> Less code is almost always better, it's more work for a reader to
> follow the logic if things are split across multiple files.
> 
> If you absolutely positively want to keep the current split,
> the "static const struct acpi_device_id irst_ids[]" data structure
> should be replaced by a "static const char*" in order to not waste
> memory.

As I've learned the hard way yesterday, acpi_dev_present() is undefined
if the kernel is compiled without CONFIG_ACPI, so you made the right
call splitting that out into intel_acpi.c and my suggested simplification
was wrong. Sorry for the noise. :-/

I'd still suggest to invoke acpi_dev_present() with the string literal
"INT3392" in intel_acpi.c:intel_detect_acpi_rst() though, or at least
replace "static const struct acpi_device_id irst_ids[]" with
"static const char*".

Best regards,

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

Re: [Intel-gfx] linux-next: build failure after merge of the drm-misc tree

[Lukas Wunner]

[cc += Rafael J. Wysocki, linux-acpi]

Hi Stephen,

On Wed, Feb 10, 2016 at 12:24:51PM +1100, Stephen Rothwell wrote:
> Hi all,
> 
> After merging the drm-misc tree, today's linux-next build (arm
> multi_v7_defconfig) failed like this:
> 
> In file included from drivers/gpu/drm/nouveau/nouveau_drm.c:25:0:
> include/linux/apple-gmux.h: In function 'apple_gmux_present':
> include/linux/apple-gmux.h:36:42: error: implicit declaration of function 
> 'acpi_dev_present' [-Werror=implicit-function-declaration]
>   return IS_ENABLED(CONFIG_APPLE_GMUX) && acpi_dev_present(GMUX_ACPI_HID);
>   ^
> 
> Caused by commit
> 
>   2413306c2566 ("apple-gmux: Add helper for presence detect")
> 
> I have used the drm-misc tree from next-20160209 for today.

Ugh, apologies, I didn't have a non-ACPI platform available to test
this on.

Solution is to either add to include/linux/acpi.h

static inline bool acpi_dev_present(const char *hid)
{
return false;
}

somewhere below

#else   /* !CONFIG_ACPI */

or alternatively to add to include/linux/apple-gmux.h

IS_ENABLED(CONFIG_ACPI)

in apple_gmux_present().

I'll check the other users of acpi_dev_present() to see which of
these two solutions is more appropriate and will post a fix shortly.

Thanks a lot for reporting this.

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

Re: [Intel-gfx] linux-next: build failure after merge of the drm-misc tree

[Lukas Wunner]

Hi,

On Wed, Feb 10, 2016 at 09:41:38AM +0100, Lukas Wunner wrote:
> On Wed, Feb 10, 2016 at 12:24:51PM +1100, Stephen Rothwell wrote:
> > Hi all,
> > 
> > After merging the drm-misc tree, today's linux-next build (arm
> > multi_v7_defconfig) failed like this:
> > 
> > In file included from drivers/gpu/drm/nouveau/nouveau_drm.c:25:0:
> > include/linux/apple-gmux.h: In function 'apple_gmux_present':
> > include/linux/apple-gmux.h:36:42: error: implicit declaration of function 
> > 'acpi_dev_present' [-Werror=implicit-function-declaration]
> >   return IS_ENABLED(CONFIG_APPLE_GMUX) && acpi_dev_present(GMUX_ACPI_HID);
> >   ^
> > 
> > Caused by commit
> > 
> >   2413306c2566 ("apple-gmux: Add helper for presence detect")
> > 
> > I have used the drm-misc tree from next-20160209 for today.
> 
> Ugh, apologies, I didn't have a non-ACPI platform available to test
> this on.
> 
> Solution is to either add to include/linux/acpi.h
> 
> static inline bool acpi_dev_present(const char *hid)
> {
>   return false;
> }
> 
> somewhere below
> 
> #else /* !CONFIG_ACPI */
> 
> or alternatively to add to include/linux/apple-gmux.h
> 
> IS_ENABLED(CONFIG_ACPI)
> 
> in apple_gmux_present().
> 
> I'll check the other users of acpi_dev_present() to see which of
> these two solutions is more appropriate and will post a fix shortly.

The patch included below fixes the build if CONFIG_ACPI is not set.

@Daniel Vetter: Would it be possible to squash this with 2413306c2566
("apple-gmux: Add helper for presence detect") on topic/drm-misc so
as to avoid build breakage for anyone trying to bisect between that
commit and this fix?

I checked all other users of acpi_dev_present() and all of them are
only compiled if CONFIG_ACPI is set. Hence I opted to fix this in
 rather than in .

Thanks again Stephen for reporting this at such an early stage,
though doubtlessly it would have been better if I had thought of
this possibility when preparing the original patch, or if I had
compile-tested without CONFIG_ACPI. :-(

Lukas

-- >8 --
Subject: [PATCH] apple-gmux: Fix build breakage if !CONFIG_ACPI

The DRM drivers i915, nouveau and radeon may be compiled with
CONFIG_ACPI not set, in which case acpi_dev_present() is undefined.

Add a no-op stub for apple_gmux_present() which is used if
CONFIG_APPLE_GMUX is not enabled to avoid build breakage.
(CONFIG_APPLE_GMUX depends on CONFIG_ACPI.)

Reported-by: Stephen Rothwell <s...@canb.auug.org.au>
Signed-off-by: Lukas Wunner <lu...@wunner.de>
---
 include/linux/apple-gmux.h | 13 -
 1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/include/linux/apple-gmux.h b/include/linux/apple-gmux.h
index feebc28..b2d32e0 100644
--- a/include/linux/apple-gmux.h
+++ b/include/linux/apple-gmux.h
@@ -22,6 +22,8 @@
 
 #define GMUX_ACPI_HID "APP000B"
 
+#if IS_ENABLED(CONFIG_APPLE_GMUX)
+
 /**
  * apple_gmux_present() - detect if gmux is built into the machine
  *
@@ -33,7 +35,16 @@
  */
 static inline bool apple_gmux_present(void)
 {
-   return IS_ENABLED(CONFIG_APPLE_GMUX) && acpi_dev_present(GMUX_ACPI_HID);
+   return acpi_dev_present(GMUX_ACPI_HID);
 }
 
+#else  /* !CONFIG_APPLE_GMUX */
+
+static inline bool apple_gmux_present(void)
+{
+   return false;
+}
+
+#endif /* !CONFIG_APPLE_GMUX */
+
 #endif /* LINUX_APPLE_GMUX_H */
-- 
1.8.5.2 (Apple Git-48)

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