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

[Joonas Lahtinen <joonas.lahtinen@linux.intel.com>]

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
> 
-- 
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