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@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
