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