Re: [PATCH v4 2/2] drm/i915: Move fbdev_suspend_work to intel_fbdev

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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




[Index of Archives]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]
  Powered by Linux