On to, 2016-03-31 at 14:57 +0100, Chris Wilson wrote:
> If the initialisation fails, we may be left with a dangling pointer with
> an incomplete fbdev structure. Here we want to disable internal calls
> into fbdev. Similarly, the initialisation may be slow and we haven't yet
> enabled the fbdev (e.g. quick suspend or last-close before the async init
> completes).
>
> v3: To create a typo introduced when retyping
> v4: Prevent info==NULL dereference in early boot
>
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=93580
> Reported-by: "Li, Weinan Z" <[email protected]>
> Tested-by: Gabriel Feceoru <[email protected]>
> Signed-off-by: Chris Wilson <[email protected]>
> ---
> drivers/gpu/drm/i915/intel_fbdev.c | 72
> +++++++++++++++++++++++++-------------
> 1 file changed, 48 insertions(+), 24 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_fbdev.c
> b/drivers/gpu/drm/i915/intel_fbdev.c
> index 153ea7a3fcf6..5d4be71bdf22 100644
> --- a/drivers/gpu/drm/i915/intel_fbdev.c
> +++ b/drivers/gpu/drm/i915/intel_fbdev.c
> @@ -756,17 +756,47 @@ void intel_fbdev_fini(struct drm_device *dev)
> dev_priv->fbdev = NULL;
> }
>
> +static struct intel_fbdev *intel_fbdev_get(struct drm_device *dev)
> +{
> + struct drm_i915_private *dev_priv = to_i915(dev);
> + struct fb_info *info;
> +
> + if (dev_priv->fbdev == NULL)
> + return NULL;
> +
> + info = dev_priv->fbdev->helper.fbdev;
> + if (info == NULL)
> + return NULL;
> +
> + if (info->screen_base == NULL)
> + return NULL;
> +
This is rather verbose to my liking, I'd rather be dropping those '==
NULL' and convert to !. But either way to me.
> + return dev_priv->fbdev;
> +}
> +
> +static struct intel_fbdev *intel_fbdev_get_if_active(struct drm_device *dev)
> +{
> + struct intel_fbdev *ifbdev;
> +
> + ifbdev = intel_fbdev_get(dev);
> + if (ifbdev == NULL)
> + return NULL;
> +
> + if (ifbdev->helper.fbdev->state != FBINFO_STATE_RUNNING)
> + return NULL;
> +
> + return ifbdev;
> +}
> +
> void intel_fbdev_set_suspend(struct drm_device *dev, int state, bool
> synchronous)
> {
> struct drm_i915_private *dev_priv = dev->dev_private;
> - struct intel_fbdev *ifbdev = dev_priv->fbdev;
> - struct fb_info *info;
> + struct intel_fbdev *ifbdev;
>
> - if (!ifbdev)
> + ifbdev = intel_fbdev_get(dev);
> + if (ifbdev == NULL)
> return;
>
> - info = ifbdev->helper.fbdev;
> -
> 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
> @@ -798,8 +828,10 @@ void intel_fbdev_set_suspend(struct drm_device *dev, int
> state, bool synchronous
> * 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)
> + 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();
> @@ -807,32 +839,24 @@ void intel_fbdev_set_suspend(struct drm_device *dev,
> int state, bool synchronous
>
> void intel_fbdev_output_poll_changed(struct drm_device *dev)
> {
> - struct drm_i915_private *dev_priv = dev->dev_private;
> + struct intel_fbdev *ifbdev = intel_fbdev_get_if_active(dev);
> +
> + if (ifbdev == NULL)
> + return;
>
> - async_synchronize_full();
> - if (dev_priv->fbdev)
> - drm_fb_helper_hotplug_event(&dev_priv->fbdev->helper);
> + drm_fb_helper_hotplug_event(&ifbdev->helper);
> }
>
> void intel_fbdev_restore_mode(struct drm_device *dev)
> {
> - int ret;
> - struct drm_i915_private *dev_priv = dev->dev_private;
> - struct intel_fbdev *ifbdev = dev_priv->fbdev;
> - struct drm_fb_helper *fb_helper;
> + struct intel_fbdev *ifbdev = intel_fbdev_get_if_active(dev);
>
> - async_synchronize_full();
What's with the async_synchronize_full() begin removed completely?
> - if (!ifbdev)
> + if (ifbdev == NULL)
Argh.
> return;
>
> - fb_helper = &ifbdev->helper;
> -
> - ret = drm_fb_helper_restore_fbdev_mode_unlocked(fb_helper);
> - if (ret) {
> - DRM_DEBUG("failed to restore crtc mode\n");
> - } else {
> - mutex_lock(&fb_helper->dev->struct_mutex);
> + if (drm_fb_helper_restore_fbdev_mode_unlocked(&ifbdev->helper) == 0) {
> + mutex_lock(&dev->struct_mutex);
> intel_fb_obj_invalidate(ifbdev->fb->obj, ORIGIN_GTT);
> - mutex_unlock(&fb_helper->dev->struct_mutex);
> + mutex_unlock(&dev->struct_mutex);
Above addressed,
Reviewed-by: Joonas Lahtinen <[email protected]>
> }
> }
--
Joonas Lahtinen
Open Source Technology Center
Intel Corporation
_______________________________________________
Intel-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
