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@xxxxxxxxxxxxxxxxxx>
> ---
> 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@xxxxxxxxxxxxxxx>
> }
>
> void intel_fbdev_output_poll_changed(struct drm_device *dev)
--
Joonas Lahtinen
Open Source Technology Center
Intel Corporation
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
