[Intel-gfx] [PATCH 5/7] drm/i915: Make sure we invalidate frontbuffer on fbcon.

[Vivi, Rodrigo]

On Mon, 2015-03-02 at 18:59 +0100, Daniel Vetter wrote:
> On Fri, Feb 27, 2015 at 08:26:05PM -0500, Rodrigo Vivi wrote:
> > There are some cases like suspend/resume or dpms off/on sequences
> > that can flush frontbuffer bits. In these cases features that relies
> > on frontbuffer tracking can start working and user can stop getting
> > screen updates on fbcon having impression the system is frozen.
> > 
> > So, let's make sure on fbcon write operation we also invalidate
> > frontbuffer bits so we will be on the safest side with fbcon.
> 
> This is just a bandaid since you can always just directly access the
> fbdev framebuffer. We really need to figure out why we have frontbuffer
> bit flushes after we've invalidated them for fbcon and catch them all.

yeah, an ugly bandaid... Just to make PSR a bit more reliable without
breaking fbcon environment when it gets enabled by default.

The issue is that on the logs I see:

1.fbdev_blank dpms off
2. disable planes
3. flush frontbuffer bits
--- blank stage ---
4. fbdev_blank dpms on
5. enable planes
6. flush frontbuffer bits

So even if we put the invalidate there it will still get flushed.

Along with this sequence I see bunch of fillrect, cursor, imageblt,
copyarea so what ever happens first right after the "6." will invalidate
the frontbuffer_bits again so any direct write thought fbdev framebuffer
will be safe enough.

So yeah, with this bandaid for now I believe we are safe to enable psr
by default while we continue the investigation to come up with a proper
fix.

> -Daniel
> 
> > 
> > Signed-off-by: Rodrigo Vivi <rodrigo.vivi at intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_fbdev.c | 120 ++++++++++++++++++++++++++++++++++++-
> >  1 file changed, 117 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_fbdev.c b/drivers/gpu/drm/i915/intel_fbdev.c
> > index 234a699..1b512f2 100644
> > --- a/drivers/gpu/drm/i915/intel_fbdev.c
> > +++ b/drivers/gpu/drm/i915/intel_fbdev.c
> > @@ -71,13 +71,127 @@ static int intel_fbdev_set_par(struct fb_info *info)
> >  	return ret;
> >  }
> >  
> > +void intel_fbdev_fillrect(struct fb_info *info, const struct fb_fillrect *rect)
> > +{
> > +	struct drm_fb_helper *fb_helper = info->par;
> > +	struct intel_fbdev *ifbdev =
> > +		container_of(fb_helper, struct intel_fbdev, helper);
> > +
> > +	cfb_fillrect(info, rect);
> > +
> > +	/*
> > +	 * FIXME: fbdev presumes that all callbacks also work from
> > +	 * atomic contexts and relies on that for emergency oops
> > +	 * printing. KMS totally doesn't do that and the locking here is
> > +	 * by far not the only place this goes wrong.  Ignore this for
> > +	 * now until we solve this for real.
> > +	 */
> > +	mutex_lock(&fb_helper->dev->struct_mutex);
> > +
> > +	/*
> > +	 * There are some cases that can flush frontbuffer bits
> > +	 * while we are still on console. So, let's make sure the fb obj
> > +	 * gets invalidated on this write op so we don't have any risk
> > +	 * of missing screen updates when PSR, FBC or any other power saving
> > +	 * feature is enabled.
> > +	 */
> > +	intel_fb_obj_invalidate(ifbdev->fb->obj, NULL);
> > +	mutex_unlock(&fb_helper->dev->struct_mutex);
> > +}
> > +
> > +void intel_fbdev_copyarea(struct fb_info *info,
> > +			  const struct fb_copyarea *region)\
> > +{
> > +	struct drm_fb_helper *fb_helper = info->par;
> > +	struct intel_fbdev *ifbdev =
> > +		container_of(fb_helper, struct intel_fbdev, helper);
> > +
> > +	cfb_copyarea(info, region);
> > +
> > +	/*
> > +	 * FIXME: fbdev presumes that all callbacks also work from
> > +	 * atomic contexts and relies on that for emergency oops
> > +	 * printing. KMS totally doesn't do that and the locking here is
> > +	 * by far not the only place this goes wrong.  Ignore this for
> > +	 * now until we solve this for real.
> > +	 */
> > +	mutex_lock(&fb_helper->dev->struct_mutex);
> > +
> > +	/*
> > +	 * There are some cases that can flush frontbuffer bits
> > +	 * while we are still on console. So, let's make sure the fb obj
> > +	 * gets invalidated on this write op so we don't have any risk
> > +	 * of missing screen updates when PSR, FBC or any other power saving
> > +	 * feature is enabled.
> > +	 */
> > +	intel_fb_obj_invalidate(ifbdev->fb->obj, NULL);
> > +	mutex_unlock(&fb_helper->dev->struct_mutex);
> > +}
> > +
> > +void intel_fbdev_imageblit(struct fb_info *info, const struct fb_image *image)
> > +{
> > +	struct drm_fb_helper *fb_helper = info->par;
> > +	struct intel_fbdev *ifbdev =
> > +		container_of(fb_helper, struct intel_fbdev, helper);
> > +
> > +	cfb_imageblit(info, image);
> > +
> > +	/*
> > +	 * FIXME: fbdev presumes that all callbacks also work from
> > +	 * atomic contexts and relies on that for emergency oops
> > +	 * printing. KMS totally doesn't do that and the locking here is
> > +	 * by far not the only place this goes wrong.  Ignore this for
> > +	 * now until we solve this for real.
> > +	 */
> > +	mutex_lock(&fb_helper->dev->struct_mutex);
> > +
> > +	/*
> > +	 * There are some cases that can flush frontbuffer bits
> > +	 * while we are still on console. So, let's make sure the fb obj
> > +	 * gets invalidated on this write op so we don't have any risk
> > +	 * of missing screen updates when PSR, FBC or any other power saving
> > +	 * feature is enabled.
> > +	 */
> > +	intel_fb_obj_invalidate(ifbdev->fb->obj, NULL);
> > +	mutex_unlock(&fb_helper->dev->struct_mutex);
> > +}
> > +
> > +int intel_fbdev_cursor(struct fb_info *info, struct fb_cursor *cursor)
> > +{
> > +	struct drm_fb_helper *fb_helper = info->par;
> > +	struct intel_fbdev *ifbdev =
> > +		container_of(fb_helper, struct intel_fbdev, helper);
> > +
> > +	/*
> > +	 * FIXME: fbdev presumes that all callbacks also work from
> > +	 * atomic contexts and relies on that for emergency oops
> > +	 * printing. KMS totally doesn't do that and the locking here is
> > +	 * by far not the only place this goes wrong.  Ignore this for
> > +	 * now until we solve this for real.
> > +	 */
> > +	mutex_lock(&fb_helper->dev->struct_mutex);
> > +
> > +	/*
> > +	 * There are some cases that can flush frontbuffer bits
> > +	 * while we are still on console. So, let's make sure the fb obj
> > +	 * gets invalidated on this write op so we don't have any risk
> > +	 * of missing screen updates when PSR, FBC or any other power saving
> > +	 * feature is enabled.
> > +	 */
> > +	intel_fb_obj_invalidate(ifbdev->fb->obj, NULL);
> > +	mutex_unlock(&fb_helper->dev->struct_mutex);
> > +
> > +	return 0;
> > +}
> > +
> >  static struct fb_ops intelfb_ops = {
> >  	.owner = THIS_MODULE,
> >  	.fb_check_var = drm_fb_helper_check_var,
> >  	.fb_set_par = intel_fbdev_set_par,
> > -	.fb_fillrect = cfb_fillrect,
> > -	.fb_copyarea = cfb_copyarea,
> > -	.fb_imageblit = cfb_imageblit,
> > +	.fb_fillrect = intel_fbdev_fillrect,
> > +	.fb_copyarea = intel_fbdev_copyarea,
> > +	.fb_imageblit = intel_fbdev_imageblit,
> > +	.fb_cursor = intel_fbdev_cursor,
> >  	.fb_pan_display = drm_fb_helper_pan_display,
> >  	.fb_blank = drm_fb_helper_blank,
> >  	.fb_setcmap = drm_fb_helper_setcmap,
> > -- 
> > 1.9.3
> > 
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx at lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>