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

[Daniel Vetter <daniel@ffwll.ch>]

On Mon, Mar 02, 2015 at 06:35:26PM +0000, Vivi, Rodrigo wrote:
> 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

so fbdev_blank returns _before_ the below enable_planes/frontbuf_flush?
Can you please attach full backtraces for steps 5&6?

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

Yeah generally fbcon starts out with drawing a bit black rectangle for the
entire screen, so this should generally work. But first I really want to
understand where that enable plane is coming from, before I give up and
apply this.

Thanks, Daniel

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

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx