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

Tue Mar 3 12:03:13 PST 2015

On Tue, 2015-03-03 at 09:28 +0100, Daniel Vetter wrote:
> 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?

[  156.665517] [drm:intel_fbdev_set_par] PSR FBDEV modeset
[  759.232969] [drm:intel_fbdev_blank] PSR FBDEV blank normal
[  759.232987] [drm:intel_crtc_disable_planes] PSR FBDEV crtc disable
planes flush fb bits
[  897.313095] [drm:intel_fbdev_blank] PSR FBDEV unblank
[  897.313112] [drm:intel_crtc_control] PSR FBDEV crtc enable planes
[  897.527818] [drm:haswell_crtc_enable] PSR FBDEV crtc enable planes
[  897.542925] [drm:intel_crtc_enable_planes] PSR FBDEV crtc enable
planes flush fb bits

(full attached)

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

fair enough! thanks for all help here.

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

-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: dmesg.full
URL: <http://lists.freedesktop.org/archives/intel-gfx/attachments/20150303/50441c46/attachment-0001.ksh>