[Intel-gfx] [PATCH 7/7] drm/i915: fbdev restore mode needs to invalidate frontbuffer

[Paulo Zanoni]

2015-07-07 20:28 GMT-03:00 Rodrigo Vivi <rodrigo.vivi at intel.com>:
> This fbdev restore mode was another corner case that was now
> calling frontbuffer flip and flush and making we miss
> screen updates with PSR enabled.
>
> So let's also add the invalidate hack here while we don't have
> a reliable dirty fbdev op.

So when I saw patches 6 and 7 I decided to investigate how fbcon
interacts with frontbuffer tracking. One thing that I notice is that,
without this patch, if I kill the display manager, I'll have a bunch
of flushes without an invalidate when returning to fbcon. And I
suppose we don't want PSR/FBC enabled on fbcon, so this patch seems to
fix a bug.

By the way, I think we can try to simulate this "kill display manager"
bug on IGT. We could try to close the DRM device and then check if
FBC/PSR stopped. I guess it's probably easier to create a new IGT test
for that.

More below.

>
> Signed-off-by: Rodrigo Vivi <rodrigo.vivi at intel.com>
> ---
>  drivers/gpu/drm/i915/intel_fbdev.c | 11 +++++++++--
>  1 file changed, 9 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_fbdev.c b/drivers/gpu/drm/i915/intel_fbdev.c
> index a76cebc..ae9b809 100644
> --- a/drivers/gpu/drm/i915/intel_fbdev.c
> +++ b/drivers/gpu/drm/i915/intel_fbdev.c
> @@ -831,11 +831,18 @@ 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 = &ifbdev->helper;

Can't this ifbdev->helper assignment segfault? Shouldn't we assign
this pointer just after the !ifbdev check below?

>
> -       if (!dev_priv->fbdev)
> +       if (!ifbdev)
>                 return;
>
> -       ret = drm_fb_helper_restore_fbdev_mode_unlocked(&dev_priv->fbdev->helper);
> +       ret = drm_fb_helper_restore_fbdev_mode_unlocked(&ifbdev->helper);

You could have used fb_helper here :)

>         if (ret)
>                 DRM_DEBUG("failed to restore crtc mode\n");

My OCD tells me to request you to add the brackets on the "if" too.
Documentation/CodingStyle:168

> +       else {
> +               mutex_lock(&fb_helper->dev->struct_mutex);
> +               intel_fb_obj_invalidate(ifbdev->fb->obj, ORIGIN_GTT);
> +               mutex_unlock(&fb_helper->dev->struct_mutex);
> +       }
>  }
> --
> 2.1.0
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx



-- 
Paulo Zanoni