| Message ID | 1459432663-990-2-git-send-email-chris@chris-wilson.co.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On to, 2016-03-31 at 14:57 +0100, Chris Wilson wrote: > Since the suspend_work is entirely internal to intel_fbdev.c, move it > from the top level drm_i915_private and into struct intel_fbdev. This > requires splitting the internal interface for the suspend worker from > the external interface used by the higher layers to initiate suspend. > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > --- > drivers/gpu/drm/i915/i915_drv.c | 4 +- > drivers/gpu/drm/i915/i915_drv.h | 1 - > drivers/gpu/drm/i915/intel_drv.h | 5 +- > drivers/gpu/drm/i915/intel_fbdev.c | 117 ++++++++++++++++++++----------------- > 4 files changed, 68 insertions(+), 59 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c > index 020a31c5e2bb..100d0d92b1e6 100644 > --- a/drivers/gpu/drm/i915/i915_drv.c > +++ b/drivers/gpu/drm/i915/i915_drv.c > @@ -634,7 +634,7 @@ static int i915_drm_suspend(struct drm_device *dev) > intel_uncore_forcewake_reset(dev, false); > intel_opregion_fini(dev); > > - intel_fbdev_set_suspend(dev, FBINFO_STATE_SUSPENDED, true); > + intel_fbdev_set_suspend(dev, FBINFO_STATE_SUSPENDED); > > dev_priv->suspend_count++; > > @@ -784,7 +784,7 @@ static int i915_drm_resume(struct drm_device *dev) > > intel_opregion_init(dev); > > - intel_fbdev_set_suspend(dev, FBINFO_STATE_RUNNING, false); > + intel_fbdev_set_suspend(dev, FBINFO_STATE_RUNNING); > > mutex_lock(&dev_priv->modeset_restore_lock); > dev_priv->modeset_restore = MODESET_DONE; > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h > index 820c91f551ba..973a602c5077 100644 > --- a/drivers/gpu/drm/i915/i915_drv.h > +++ b/drivers/gpu/drm/i915/i915_drv.h > @@ -1880,7 +1880,6 @@ struct drm_i915_private { > #ifdef CONFIG_DRM_FBDEV_EMULATION > /* list of fbdev register on this device */ > struct intel_fbdev *fbdev; > - struct work_struct fbdev_suspend_work; > #endif > > struct drm_property *broadcast_rgb_property; > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h > index 6ac46d921cde..00b6c60c1cb8 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; > + struct work_struct suspend_work; > int preferred_bpp; > }; > > @@ -1335,7 +1336,7 @@ void intel_dvo_init(struct drm_device *dev); > extern int intel_fbdev_init(struct drm_device *dev); > extern void intel_fbdev_initial_config_async(struct drm_device *dev); > extern void intel_fbdev_fini(struct drm_device *dev); > -extern void intel_fbdev_set_suspend(struct drm_device *dev, int state, bool synchronous); > +extern void intel_fbdev_set_suspend(struct drm_device *dev, int state); > extern void intel_fbdev_output_poll_changed(struct drm_device *dev); > extern void intel_fbdev_restore_mode(struct drm_device *dev); > #else > @@ -1352,7 +1353,7 @@ static inline void intel_fbdev_fini(struct drm_device *dev) > { > } > > -static inline void intel_fbdev_set_suspend(struct drm_device *dev, int state, bool synchronous) > +static inline void intel_fbdev_set_suspend(struct drm_device *dev, int state) > { > } > > diff --git a/drivers/gpu/drm/i915/intel_fbdev.c b/drivers/gpu/drm/i915/intel_fbdev.c > index 5d4be71bdf22..66bb79613660 100644 > --- a/drivers/gpu/drm/i915/intel_fbdev.c > +++ b/drivers/gpu/drm/i915/intel_fbdev.c > @@ -534,8 +534,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 > @@ -681,13 +680,56 @@ out: > return false; > } > > +static void __intel_fbdev_set_suspend(struct intel_fbdev *ifbdev, > + int state, bool synchronous) > +{ > + 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 > + * about to suspend or unload the driver. > + * > + * Note that from within the work-handler, we cannot flush > + * ourselves, so only flush outstanding work upon suspend! > + */ > + if (state != FBINFO_STATE_RUNNING) > + flush_work(&ifbdev->suspend_work); > + console_lock(); > + } else { > + /* > + * The console lock can be pretty contented on resume due > + * to all the printk activity. Try to keep it out of the hot > + * path of resume if possible. > + */ > + WARN_ON(state != FBINFO_STATE_RUNNING); > + if (!console_trylock()) { > + /* Don't block our own workqueue as this can > + * be run in parallel with other i915.ko tasks. > + */ > + schedule_work(&ifbdev->suspend_work); > + return; > + } > + } > + > + /* On resume from hibernation: If the object is shmemfs backed, it has > + * 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) { > + struct fb_info *info = ifbdev->helper.fbdev; > + memset_io(info->screen_base, 0, info->screen_size); > + } > + > + drm_fb_helper_set_suspend(&ifbdev->helper, state); > + console_unlock(); > +} > + > static void intel_fbdev_suspend_worker(struct work_struct *work) > { > - intel_fbdev_set_suspend(container_of(work, > - struct drm_i915_private, > - fbdev_suspend_work)->dev, > - FBINFO_STATE_RUNNING, > - true); > + __intel_fbdev_set_suspend(container_of(work, > + struct intel_fbdev, > + suspend_work), Have the container_of on separate line at least, maybe even with a macro. > + FBINFO_STATE_RUNNING, > + true); > } > > int intel_fbdev_init(struct drm_device *dev) > @@ -716,9 +758,9 @@ int intel_fbdev_init(struct drm_device *dev) > } > > ifbdev->helper.atomic = true; > + INIT_WORK(&ifbdev->suspend_work, intel_fbdev_suspend_worker); > > dev_priv->fbdev = ifbdev; > - INIT_WORK(&dev_priv->fbdev_suspend_work, intel_fbdev_suspend_worker); > > drm_fb_helper_single_add_all_connectors(&ifbdev->helper); > > @@ -743,17 +785,21 @@ void intel_fbdev_initial_config_async(struct drm_device *dev) > > void intel_fbdev_fini(struct drm_device *dev) > { > - struct drm_i915_private *dev_priv = dev->dev_private; > - if (!dev_priv->fbdev) > + struct drm_i915_private *dev_priv = to_i915(dev); > + struct intel_fbdev *ifbdev; > + > + ifbdev = dev_priv->fbdev; Straight assignment. > + if (ifbdev == NULL) > return; > > - flush_work(&dev_priv->fbdev_suspend_work); > + dev_priv->fbdev = NULL; > > if (!current_is_async()) > async_synchronize_full(); > - intel_fbdev_destroy(dev, dev_priv->fbdev); > - kfree(dev_priv->fbdev); > - dev_priv->fbdev = NULL; > + flush_work(&ifbdev->suspend_work); > + > + intel_fbdev_destroy(ifbdev); > + kfree(ifbdev); > } > > static struct intel_fbdev *intel_fbdev_get(struct drm_device *dev) > @@ -788,53 +834,16 @@ static struct intel_fbdev *intel_fbdev_get_if_active(struct drm_device *dev) > return ifbdev; > } > > -void intel_fbdev_set_suspend(struct drm_device *dev, int state, bool synchronous) > +void intel_fbdev_set_suspend(struct drm_device *dev, int state) > { > - struct drm_i915_private *dev_priv = dev->dev_private; > struct intel_fbdev *ifbdev; > > ifbdev = intel_fbdev_get(dev); > if (ifbdev == NULL) > return; > > - 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 > - * about to suspend or unload the driver. > - * > - * Note that from within the work-handler, we cannot flush > - * ourselves, so only flush outstanding work upon suspend! > - */ > - if (state != FBINFO_STATE_RUNNING) > - flush_work(&dev_priv->fbdev_suspend_work); > - console_lock(); > - } else { > - /* > - * The console lock can be pretty contented on resume due > - * to all the printk activity. Try to keep it out of the hot > - * path of resume if possible. > - */ > - WARN_ON(state != FBINFO_STATE_RUNNING); > - if (!console_trylock()) { > - /* Don't block our own workqueue as this can > - * be run in parallel with other i915.ko tasks. > - */ > - schedule_work(&dev_priv->fbdev_suspend_work); > - return; > - } > - } > - > - /* On resume from hibernation: If the object is shmemfs backed, it has > - * 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) { > - struct fb_info *info = ifbdev->helper.fbdev; > - memset_io(info->screen_base, 0, info->screen_size); > - } > - > - drm_fb_helper_set_suspend(&ifbdev->helper, state); > - console_unlock(); > + __intel_fbdev_set_suspend(ifbdev, state, > + state != FBINFO_STATE_RUNNING); Could have changed this function name to make it easier to spot what you changed or not. Code motion as separate patches. With above addressed, Reviewed-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> > } > > void intel_fbdev_output_poll_changed(struct drm_device *dev)
On Thu, Mar 31, 2016 at 06:22:01PM +0300, Joonas Lahtinen wrote: > On to, 2016-03-31 at 14:57 +0100, Chris Wilson wrote: > > static void intel_fbdev_suspend_worker(struct work_struct *work) > > { > > - intel_fbdev_set_suspend(container_of(work, > > - struct drm_i915_private, > > - fbdev_suspend_work)->dev, > > - FBINFO_STATE_RUNNING, > > - true); > > + __intel_fbdev_set_suspend(container_of(work, > > + struct intel_fbdev, > > + suspend_work), > > Have the container_of on separate line at least, maybe even with a > macro. Sure. > > + FBINFO_STATE_RUNNING, > > + true); > > } > > > > int intel_fbdev_init(struct drm_device *dev) > > @@ -716,9 +758,9 @@ int intel_fbdev_init(struct drm_device *dev) > > } > > > > ifbdev->helper.atomic = true; > > + INIT_WORK(&ifbdev->suspend_work, intel_fbdev_suspend_worker); > > > > dev_priv->fbdev = ifbdev; > > - INIT_WORK(&dev_priv->fbdev_suspend_work, intel_fbdev_suspend_worker); > > > > drm_fb_helper_single_add_all_connectors(&ifbdev->helper); > > > > @@ -743,17 +785,21 @@ void intel_fbdev_initial_config_async(struct drm_device *dev) > > > > void intel_fbdev_fini(struct drm_device *dev) > > { > > - struct drm_i915_private *dev_priv = dev->dev_private; > > - if (!dev_priv->fbdev) > > + struct drm_i915_private *dev_priv = to_i915(dev); > > + struct intel_fbdev *ifbdev; > > + > > + ifbdev = dev_priv->fbdev; > > Straight assignment. > > > + if (ifbdev == NULL) I was trying to group the local assignment with the if, and I like to leave whitespace after the locals. By placing these two lines together this function looks like the majority of the other functions. > > return; > > > > - flush_work(&dev_priv->fbdev_suspend_work); > > + dev_priv->fbdev = NULL; > > > > if (!current_is_async()) > > async_synchronize_full(); > > - intel_fbdev_destroy(dev, dev_priv->fbdev); > > - kfree(dev_priv->fbdev); > > - dev_priv->fbdev = NULL; > > + flush_work(&ifbdev->suspend_work); > > + > > + intel_fbdev_destroy(ifbdev); > > + kfree(ifbdev); > > } > > > > static struct intel_fbdev *intel_fbdev_get(struct drm_device *dev) > > @@ -788,53 +834,16 @@ static struct intel_fbdev *intel_fbdev_get_if_active(struct drm_device *dev) > > return ifbdev; > > } > > > > -void intel_fbdev_set_suspend(struct drm_device *dev, int state, bool synchronous) > > +void intel_fbdev_set_suspend(struct drm_device *dev, int state) > > { > > - struct drm_i915_private *dev_priv = dev->dev_private; > > struct intel_fbdev *ifbdev; > > > > ifbdev = intel_fbdev_get(dev); > > if (ifbdev == NULL) > > return; > > > > - 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 > > - * about to suspend or unload the driver. > > - * > > - * Note that from within the work-handler, we cannot flush > > - * ourselves, so only flush outstanding work upon suspend! > > - */ > > - if (state != FBINFO_STATE_RUNNING) > > - flush_work(&dev_priv->fbdev_suspend_work); > > - console_lock(); > > - } else { > > - /* > > - * The console lock can be pretty contented on resume due > > - * to all the printk activity. Try to keep it out of the hot > > - * path of resume if possible. > > - */ > > - WARN_ON(state != FBINFO_STATE_RUNNING); > > - if (!console_trylock()) { > > - /* Don't block our own workqueue as this can > > - * be run in parallel with other i915.ko tasks. > > - */ > > - schedule_work(&dev_priv->fbdev_suspend_work); > > - return; > > - } > > - } > > - > > - /* On resume from hibernation: If the object is shmemfs backed, it has > > - * 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) { > > - struct fb_info *info = ifbdev->helper.fbdev; > > - memset_io(info->screen_base, 0, info->screen_size); > > - } > > - > > - drm_fb_helper_set_suspend(&ifbdev->helper, state); > > - console_unlock(); > > + __intel_fbdev_set_suspend(ifbdev, state, > > + state != FBINFO_STATE_RUNNING); > > Could have changed this function name to make it easier to spot what > you changed or not. Code motion as separate patches. This was a separate patch! The purpose of this patch was to move code/data about :) -Chris
On to, 2016-03-31 at 16:30 +0100, Chris Wilson wrote: > On Thu, Mar 31, 2016 at 06:22:01PM +0300, Joonas Lahtinen wrote: > > > > On to, 2016-03-31 at 14:57 +0100, Chris Wilson wrote: > > > > > > static void intel_fbdev_suspend_worker(struct work_struct *work) > > > { > > > - intel_fbdev_set_suspend(container_of(work, > > > - struct drm_i915_private, > > > - fbdev_suspend_work)->dev, > > > - FBINFO_STATE_RUNNING, > > > - true); > > > + __intel_fbdev_set_suspend(container_of(work, > > > + struct intel_fbdev, > > > + suspend_work), > > Have the container_of on separate line at least, maybe even with a > > macro. > Sure. > > > > > > > > > + FBINFO_STATE_RUNNING, > > > + true); > > > } > > > > > > int intel_fbdev_init(struct drm_device *dev) > > > @@ -716,9 +758,9 @@ int intel_fbdev_init(struct drm_device *dev) > > > } > > > > > > ifbdev->helper.atomic = true; > > > + INIT_WORK(&ifbdev->suspend_work, intel_fbdev_suspend_worker); > > > > > > dev_priv->fbdev = ifbdev; > > > - INIT_WORK(&dev_priv->fbdev_suspend_work, intel_fbdev_suspend_worker); > > > > > > drm_fb_helper_single_add_all_connectors(&ifbdev->helper); > > > > > > @@ -743,17 +785,21 @@ void intel_fbdev_initial_config_async(struct drm_device *dev) > > > > > > void intel_fbdev_fini(struct drm_device *dev) > > > { > > > - struct drm_i915_private *dev_priv = dev->dev_private; > > > - if (!dev_priv->fbdev) > > > + struct drm_i915_private *dev_priv = to_i915(dev); > > > + struct intel_fbdev *ifbdev; > > > + > > > + ifbdev = dev_priv->fbdev; > > Straight assignment. > > > > > > > > + if (ifbdev == NULL) > I was trying to group the local assignment with the if, and I like to leave > whitespace after the locals. By placing these two lines together this > function looks like the majority of the other functions. > > > > > > > > > return; > > > > > > - flush_work(&dev_priv->fbdev_suspend_work); > > > + dev_priv->fbdev = NULL; > > > > > > if (!current_is_async()) > > > async_synchronize_full(); > > > - intel_fbdev_destroy(dev, dev_priv->fbdev); > > > - kfree(dev_priv->fbdev); > > > - dev_priv->fbdev = NULL; > > > + flush_work(&ifbdev->suspend_work); > > > + > > > + intel_fbdev_destroy(ifbdev); > > > + kfree(ifbdev); > > > } > > > > > > static struct intel_fbdev *intel_fbdev_get(struct drm_device *dev) > > > @@ -788,53 +834,16 @@ static struct intel_fbdev *intel_fbdev_get_if_active(struct drm_device *dev) > > > return ifbdev; > > > } > > > > > > -void intel_fbdev_set_suspend(struct drm_device *dev, int state, bool synchronous) > > > +void intel_fbdev_set_suspend(struct drm_device *dev, int state) > > > { > > > - struct drm_i915_private *dev_priv = dev->dev_private; > > > struct intel_fbdev *ifbdev; > > > > > > ifbdev = intel_fbdev_get(dev); > > > if (ifbdev == NULL) > > > return; > > > > > > - 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 > > > - * about to suspend or unload the driver. > > > - * > > > - * Note that from within the work-handler, we cannot flush > > > - * ourselves, so only flush outstanding work upon suspend! > > > - */ > > > - if (state != FBINFO_STATE_RUNNING) > > > - flush_work(&dev_priv->fbdev_suspend_work); > > > - console_lock(); > > > - } else { > > > - /* > > > - * The console lock can be pretty contented on resume due > > > - * to all the printk activity. Try to keep it out of the hot > > > - * path of resume if possible. > > > - */ > > > - WARN_ON(state != FBINFO_STATE_RUNNING); > > > - if (!console_trylock()) { > > > - /* Don't block our own workqueue as this can > > > - * be run in parallel with other i915.ko tasks. > > > - */ > > > - schedule_work(&dev_priv->fbdev_suspend_work); > > > - return; > > > - } > > > - } > > > - > > > - /* On resume from hibernation: If the object is shmemfs backed, it has > > > - * 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) { > > > - struct fb_info *info = ifbdev->helper.fbdev; > > > - memset_io(info->screen_base, 0, info->screen_size); > > > - } > > > - > > > - drm_fb_helper_set_suspend(&ifbdev->helper, state); > > > - console_unlock(); > > > + __intel_fbdev_set_suspend(ifbdev, state, > > > + state != FBINFO_STATE_RUNNING); > > Could have changed this function name to make it easier to spot what > > you changed or not. Code motion as separate patches. > This was a separate patch! The purpose of this patch was to move > code/data about :) It also changed order of some stuff, so I got suspicious ;) But looks like the function stayed the same, so OK for me. > -Chris >
diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c index 020a31c5e2bb..100d0d92b1e6 100644 --- a/drivers/gpu/drm/i915/i915_drv.c +++ b/drivers/gpu/drm/i915/i915_drv.c @@ -634,7 +634,7 @@ static int i915_drm_suspend(struct drm_device *dev) intel_uncore_forcewake_reset(dev, false); intel_opregion_fini(dev); - intel_fbdev_set_suspend(dev, FBINFO_STATE_SUSPENDED, true); + intel_fbdev_set_suspend(dev, FBINFO_STATE_SUSPENDED); dev_priv->suspend_count++; @@ -784,7 +784,7 @@ static int i915_drm_resume(struct drm_device *dev) intel_opregion_init(dev); - intel_fbdev_set_suspend(dev, FBINFO_STATE_RUNNING, false); + intel_fbdev_set_suspend(dev, FBINFO_STATE_RUNNING); mutex_lock(&dev_priv->modeset_restore_lock); dev_priv->modeset_restore = MODESET_DONE; diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index 820c91f551ba..973a602c5077 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -1880,7 +1880,6 @@ struct drm_i915_private { #ifdef CONFIG_DRM_FBDEV_EMULATION /* list of fbdev register on this device */ struct intel_fbdev *fbdev; - struct work_struct fbdev_suspend_work; #endif struct drm_property *broadcast_rgb_property; diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h index 6ac46d921cde..00b6c60c1cb8 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; + struct work_struct suspend_work; int preferred_bpp; }; @@ -1335,7 +1336,7 @@ void intel_dvo_init(struct drm_device *dev); extern int intel_fbdev_init(struct drm_device *dev); extern void intel_fbdev_initial_config_async(struct drm_device *dev); extern void intel_fbdev_fini(struct drm_device *dev); -extern void intel_fbdev_set_suspend(struct drm_device *dev, int state, bool synchronous); +extern void intel_fbdev_set_suspend(struct drm_device *dev, int state); extern void intel_fbdev_output_poll_changed(struct drm_device *dev); extern void intel_fbdev_restore_mode(struct drm_device *dev); #else @@ -1352,7 +1353,7 @@ static inline void intel_fbdev_fini(struct drm_device *dev) { } -static inline void intel_fbdev_set_suspend(struct drm_device *dev, int state, bool synchronous) +static inline void intel_fbdev_set_suspend(struct drm_device *dev, int state) { } diff --git a/drivers/gpu/drm/i915/intel_fbdev.c b/drivers/gpu/drm/i915/intel_fbdev.c index 5d4be71bdf22..66bb79613660 100644 --- a/drivers/gpu/drm/i915/intel_fbdev.c +++ b/drivers/gpu/drm/i915/intel_fbdev.c @@ -534,8 +534,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 @@ -681,13 +680,56 @@ out: return false; } +static void __intel_fbdev_set_suspend(struct intel_fbdev *ifbdev, + int state, bool synchronous) +{ + 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 + * about to suspend or unload the driver. + * + * Note that from within the work-handler, we cannot flush + * ourselves, so only flush outstanding work upon suspend! + */ + if (state != FBINFO_STATE_RUNNING) + flush_work(&ifbdev->suspend_work); + console_lock(); + } else { + /* + * The console lock can be pretty contented on resume due + * to all the printk activity. Try to keep it out of the hot + * path of resume if possible. + */ + WARN_ON(state != FBINFO_STATE_RUNNING); + if (!console_trylock()) { + /* Don't block our own workqueue as this can + * be run in parallel with other i915.ko tasks. + */ + schedule_work(&ifbdev->suspend_work); + return; + } + } + + /* On resume from hibernation: If the object is shmemfs backed, it has + * 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) { + struct fb_info *info = ifbdev->helper.fbdev; + memset_io(info->screen_base, 0, info->screen_size); + } + + drm_fb_helper_set_suspend(&ifbdev->helper, state); + console_unlock(); +} + static void intel_fbdev_suspend_worker(struct work_struct *work) { - intel_fbdev_set_suspend(container_of(work, - struct drm_i915_private, - fbdev_suspend_work)->dev, - FBINFO_STATE_RUNNING, - true); + __intel_fbdev_set_suspend(container_of(work, + struct intel_fbdev, + suspend_work), + FBINFO_STATE_RUNNING, + true); } int intel_fbdev_init(struct drm_device *dev) @@ -716,9 +758,9 @@ int intel_fbdev_init(struct drm_device *dev) } ifbdev->helper.atomic = true; + INIT_WORK(&ifbdev->suspend_work, intel_fbdev_suspend_worker); dev_priv->fbdev = ifbdev; - INIT_WORK(&dev_priv->fbdev_suspend_work, intel_fbdev_suspend_worker); drm_fb_helper_single_add_all_connectors(&ifbdev->helper); @@ -743,17 +785,21 @@ void intel_fbdev_initial_config_async(struct drm_device *dev) void intel_fbdev_fini(struct drm_device *dev) { - struct drm_i915_private *dev_priv = dev->dev_private; - if (!dev_priv->fbdev) + struct drm_i915_private *dev_priv = to_i915(dev); + struct intel_fbdev *ifbdev; + + ifbdev = dev_priv->fbdev; + if (ifbdev == NULL) return; - flush_work(&dev_priv->fbdev_suspend_work); + dev_priv->fbdev = NULL; if (!current_is_async()) async_synchronize_full(); - intel_fbdev_destroy(dev, dev_priv->fbdev); - kfree(dev_priv->fbdev); - dev_priv->fbdev = NULL; + flush_work(&ifbdev->suspend_work); + + intel_fbdev_destroy(ifbdev); + kfree(ifbdev); } static struct intel_fbdev *intel_fbdev_get(struct drm_device *dev) @@ -788,53 +834,16 @@ static struct intel_fbdev *intel_fbdev_get_if_active(struct drm_device *dev) return ifbdev; } -void intel_fbdev_set_suspend(struct drm_device *dev, int state, bool synchronous) +void intel_fbdev_set_suspend(struct drm_device *dev, int state) { - struct drm_i915_private *dev_priv = dev->dev_private; struct intel_fbdev *ifbdev; ifbdev = intel_fbdev_get(dev); if (ifbdev == NULL) return; - 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 - * about to suspend or unload the driver. - * - * Note that from within the work-handler, we cannot flush - * ourselves, so only flush outstanding work upon suspend! - */ - if (state != FBINFO_STATE_RUNNING) - flush_work(&dev_priv->fbdev_suspend_work); - console_lock(); - } else { - /* - * The console lock can be pretty contented on resume due - * to all the printk activity. Try to keep it out of the hot - * path of resume if possible. - */ - WARN_ON(state != FBINFO_STATE_RUNNING); - if (!console_trylock()) { - /* Don't block our own workqueue as this can - * be run in parallel with other i915.ko tasks. - */ - schedule_work(&dev_priv->fbdev_suspend_work); - return; - } - } - - /* On resume from hibernation: If the object is shmemfs backed, it has - * 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) { - struct fb_info *info = ifbdev->helper.fbdev; - memset_io(info->screen_base, 0, info->screen_size); - } - - drm_fb_helper_set_suspend(&ifbdev->helper, state); - console_unlock(); + __intel_fbdev_set_suspend(ifbdev, state, + state != FBINFO_STATE_RUNNING); } void intel_fbdev_output_poll_changed(struct drm_device *dev)
Since the suspend_work is entirely internal to intel_fbdev.c, move it from the top level drm_i915_private and into struct intel_fbdev. This requires splitting the internal interface for the suspend worker from the external interface used by the higher layers to initiate suspend. Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> --- drivers/gpu/drm/i915/i915_drv.c | 4 +- drivers/gpu/drm/i915/i915_drv.h | 1 - drivers/gpu/drm/i915/intel_drv.h | 5 +- drivers/gpu/drm/i915/intel_fbdev.c | 117 ++++++++++++++++++++----------------- 4 files changed, 68 insertions(+), 59 deletions(-)