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] [PATCH v4 1/2] drm/i915: Protect fbdev across slow or failed initialisation

[Joonas Lahtinen]

On to, 2016-03-31 at 17:13 +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.
> 
> During hibernation, we really just want to cancel the task and start
> from scratch on resume.

Maybe Ack from Lukas with those? As this is effectively a revert of :

commit a7442b93cf32c1e1ddb721a26cd1f92302e2a222
Author: Lukas Wunner 
Date:   Wed Mar 9 12:52:53 2016 +0100

drm/i915: Fix races on fbdev

Committed without R-b by Daniel, so maybe he has a comment on it too.

Regards, Joonas

> -Chris
> 
-- 
Joonas Lahtinen
Open Source Technology Center
Intel Corporation
___
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

[Chris Wilson]

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.

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

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

[Joonas Lahtinen]

On to, 2016-03-31 at 14:57 +0100, Chris Wilson wrote:
> If the initialisation fails, we may be left with a dangling pointer with
> an incomplete fbdev structure. Here we want to disable internal calls
> into fbdev. Similarly, the initialisation may be slow and we haven't yet
> enabled the fbdev (e.g. quick suspend or last-close before the async init
> completes).
> 
> v3: To create a typo introduced when retyping
> v4: Prevent info==NULL dereference in early boot
> 
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=93580
> Reported-by: "Li, Weinan Z" 
> Tested-by: Gabriel Feceoru 
> Signed-off-by: Chris Wilson 
> ---
>  drivers/gpu/drm/i915/intel_fbdev.c | 72 
> +-
>  1 file changed, 48 insertions(+), 24 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_fbdev.c 
> b/drivers/gpu/drm/i915/intel_fbdev.c
> index 153ea7a3fcf6..5d4be71bdf22 100644
> --- a/drivers/gpu/drm/i915/intel_fbdev.c
> +++ b/drivers/gpu/drm/i915/intel_fbdev.c
> @@ -756,17 +756,47 @@ void intel_fbdev_fini(struct drm_device *dev)
>   dev_priv->fbdev = NULL;
>  }
>  
> +static struct intel_fbdev *intel_fbdev_get(struct drm_device *dev)
> +{
> + struct drm_i915_private *dev_priv = to_i915(dev);
> + struct fb_info *info;
> +
> + if (dev_priv->fbdev == NULL)
> + return NULL;
> +
> + info = dev_priv->fbdev->helper.fbdev;
> + if (info == NULL)
> + return NULL;
> +
> + if (info->screen_base == NULL)
> + return NULL;
> +

This is rather verbose to my liking, I'd rather be dropping those '==
NULL' and convert to !. But either way to me.

> + return dev_priv->fbdev;
> +}
> +
> +static struct intel_fbdev *intel_fbdev_get_if_active(struct drm_device *dev)
> +{
> + struct intel_fbdev *ifbdev;
> +
> + ifbdev = intel_fbdev_get(dev);
> + if (ifbdev == NULL)
> + return NULL;
> +
> + if (ifbdev->helper.fbdev->state != FBINFO_STATE_RUNNING)
> + return NULL;
> +
> + return ifbdev;
> +}
> +
>  void intel_fbdev_set_suspend(struct drm_device *dev, int state, bool 
> synchronous)
>  {
>   struct drm_i915_private *dev_priv = dev->dev_private;
> - struct intel_fbdev *ifbdev = dev_priv->fbdev;
> - struct fb_info *info;
> + struct intel_fbdev *ifbdev;
>  
> - if (!ifbdev)
> + ifbdev = intel_fbdev_get(dev);
> + if (ifbdev == NULL)
>   return;
>  
> - info = ifbdev->helper.fbdev;
> -
>   if (synchronous) {
>   /* Flush any pending work to turn the console on, and then
>    * wait to turn it off. It must be synchronous as we are
> @@ -798,8 +828,10 @@ void intel_fbdev_set_suspend(struct drm_device *dev, int 
> state, bool synchronous
>    * been restored from swap. If the object is stolen however, it will be
>    * full of whatever garbage was left in there.
>    */
> - if (state == FBINFO_STATE_RUNNING && ifbdev->fb->obj->stolen)
> + if (state == FBINFO_STATE_RUNNING && ifbdev->fb->obj->stolen) {
> + struct fb_info *info = ifbdev->helper.fbdev;
>   memset_io(info->screen_base, 0, info->screen_size);
> + }
>  
>   drm_fb_helper_set_suspend(>helper, state);
>   console_unlock();
> @@ -807,32 +839,24 @@ 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;
> + struct intel_fbdev *ifbdev = intel_fbdev_get_if_active(dev);
> +
> + if (ifbdev == NULL)
> + return;
>  
> - async_synchronize_full();
> - if (dev_priv->fbdev)
> - drm_fb_helper_hotplug_event(_priv->fbdev->helper);
> + drm_fb_helper_hotplug_event(>helper);
>  }
>  
>  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?

> - if (!ifbdev)
> + if (ifbdev == NULL)

Argh.

>   return;
>  
> - fb_helper = >helper;
> -
> - ret = drm_fb_helper_restore_fbdev_mode_unlocked(fb_helper);
> - if (ret) {
> - DRM_DEBUG("failed to restore crtc mode\n");
> - } else {
> - mutex_lock(_helper->dev->struct_mutex);
> + if (drm_fb_helper_restore_fbdev_mode_unlocked(>helper) == 0) {
> + mutex_lock(>struct_mutex);
>   intel_fb_obj_invalidate(ifbdev->fb->obj, ORIGIN_GTT);
> - mutex_unlock(_helper->dev->struct_mutex);
> + mutex_unlock(>struct_mutex);

Above addressed,

Reviewed-by: Joonas Lahtinen 

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

[Chris Wilson]

If the initialisation fails, we may be left with a dangling pointer with
an incomplete fbdev structure. Here we want to disable internal calls
into fbdev. Similarly, the initialisation may be slow and we haven't yet
enabled the fbdev (e.g. quick suspend or last-close before the async init
completes).

v3: To create a typo introduced when retyping
v4: Prevent info==NULL dereference in early boot

Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=93580
Reported-by: "Li, Weinan Z" 
Tested-by: Gabriel Feceoru 
Signed-off-by: Chris Wilson 
---
 drivers/gpu/drm/i915/intel_fbdev.c | 72 +-
 1 file changed, 48 insertions(+), 24 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_fbdev.c 
b/drivers/gpu/drm/i915/intel_fbdev.c
index 153ea7a3fcf6..5d4be71bdf22 100644
--- a/drivers/gpu/drm/i915/intel_fbdev.c
+++ b/drivers/gpu/drm/i915/intel_fbdev.c
@@ -756,17 +756,47 @@ void intel_fbdev_fini(struct drm_device *dev)
dev_priv->fbdev = NULL;
 }
 
+static struct intel_fbdev *intel_fbdev_get(struct drm_device *dev)
+{
+   struct drm_i915_private *dev_priv = to_i915(dev);
+   struct fb_info *info;
+
+   if (dev_priv->fbdev == NULL)
+   return NULL;
+
+   info = dev_priv->fbdev->helper.fbdev;
+   if (info == NULL)
+   return NULL;
+
+   if (info->screen_base == NULL)
+   return NULL;
+
+   return dev_priv->fbdev;
+}
+
+static struct intel_fbdev *intel_fbdev_get_if_active(struct drm_device *dev)
+{
+   struct intel_fbdev *ifbdev;
+
+   ifbdev = intel_fbdev_get(dev);
+   if (ifbdev == NULL)
+   return NULL;
+
+   if (ifbdev->helper.fbdev->state != FBINFO_STATE_RUNNING)
+   return NULL;
+
+   return ifbdev;
+}
+
 void intel_fbdev_set_suspend(struct drm_device *dev, int state, bool 
synchronous)
 {
struct drm_i915_private *dev_priv = dev->dev_private;
-   struct intel_fbdev *ifbdev = dev_priv->fbdev;
-   struct fb_info *info;
+   struct intel_fbdev *ifbdev;
 
-   if (!ifbdev)
+   ifbdev = intel_fbdev_get(dev);
+   if (ifbdev == NULL)
return;
 
-   info = ifbdev->helper.fbdev;
-
if (synchronous) {
/* Flush any pending work to turn the console on, and then
 * wait to turn it off. It must be synchronous as we are
@@ -798,8 +828,10 @@ void intel_fbdev_set_suspend(struct drm_device *dev, int 
state, bool synchronous
 * been restored from swap. If the object is stolen however, it will be
 * full of whatever garbage was left in there.
 */
-   if (state == FBINFO_STATE_RUNNING && ifbdev->fb->obj->stolen)
+   if (state == FBINFO_STATE_RUNNING && ifbdev->fb->obj->stolen) {
+   struct fb_info *info = ifbdev->helper.fbdev;
memset_io(info->screen_base, 0, info->screen_size);
+   }
 
drm_fb_helper_set_suspend(>helper, state);
console_unlock();
@@ -807,32 +839,24 @@ 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;
+   struct intel_fbdev *ifbdev = intel_fbdev_get_if_active(dev);
+
+   if (ifbdev == NULL)
+   return;
 
-   async_synchronize_full();
-   if (dev_priv->fbdev)
-   drm_fb_helper_hotplug_event(_priv->fbdev->helper);
+   drm_fb_helper_hotplug_event(>helper);
 }
 
 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();
-   if (!ifbdev)
+   if (ifbdev == NULL)
return;
 
-   fb_helper = >helper;
-
-   ret = drm_fb_helper_restore_fbdev_mode_unlocked(fb_helper);
-   if (ret) {
-   DRM_DEBUG("failed to restore crtc mode\n");
-   } else {
-   mutex_lock(_helper->dev->struct_mutex);
+   if (drm_fb_helper_restore_fbdev_mode_unlocked(>helper) == 0) {
+   mutex_lock(>struct_mutex);
intel_fb_obj_invalidate(ifbdev->fb->obj, ORIGIN_GTT);
-   mutex_unlock(_helper->dev->struct_mutex);
+   mutex_unlock(>struct_mutex);
}
 }
-- 
2.8.0.rc3

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