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