Re: [PATCH] drm/i915: fbdev_set_par reliably invalidating frontbuffer

From: "Vivi, Rodrigo" <rodrigo.vivi@intel.com>
To: "przanoni@gmail.com" <przanoni@gmail.com>
Cc: "daniel.vetter@ffwll.ch" <daniel.vetter@ffwll.ch>,
	"intel-gfx@lists.freedesktop.org"
	<intel-gfx@lists.freedesktop.org>
Subject: Re: [PATCH] drm/i915: fbdev_set_par reliably invalidating frontbuffer
Date: Thu, 9 Jul 2015 16:47:21 +0000	[thread overview]
Message-ID: <1436460782.2893.1.camel@intel.com> (raw)
In-Reply-To: <CA+gsUGQJ5tkDfA-J7iY5v3mSyFcVmkhOccTmnaCmcZtbSAGrBA@mail.gmail.com>

On Thu, 2015-07-09 at 10:10 -0300, Paulo Zanoni wrote:
> 2015-07-08 20:24 GMT-03:00 Rodrigo Vivi <rodrigo.vivi@intel.com>:
> > fbdev_set_par is called when fbcon is taking over control.
> > In the past frontbuffer was being invalidated on
> > set_to_gtt_domain, but it moved to set_domain fixing that case,
> > but left this behind.
> >
> > Commit that changed this fixing set_domain case was:
> >
> > commit 031b698a77a70a6c394568034437b5486a44e868
> > Author: Daniel Vetter <daniel.vetter@ffwll.ch>
> > Date:   Fri Jun 26 19:35:16 2015 +0200
> >
> >     drm/i915: Unconditionally do fb tracking invalidate in set_domain
> >
> > Since we are also invalidating in other fbdev cases this one
> > was masked here. At least until now that I found this corner
> > case: On boot with plymouth doing a splash screen
> > when returning to the console frontbuffer wans't being invalidated
> > causing missed screen updates with PSR enabled.
> >
> > So this patch fixes this issue.
> >
> > v2: Make invalidate directly and unconditionally and
> >     fix commit message indicating the set_domain fix
> >     as pointed out by Daniel.
> >
> > Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_fbdev.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_fbdev.c b/drivers/gpu/drm/i915/intel_fbdev.c
> > index 2a1724e..44c9ccc 100644
> > --- a/drivers/gpu/drm/i915/intel_fbdev.c
> > +++ b/drivers/gpu/drm/i915/intel_fbdev.c
> > @@ -63,8 +63,8 @@ static int intel_fbdev_set_par(struct fb_info *info)
> >                  * now until we solve this for real.
> >                  */
> >                 mutex_lock(&fb_helper->dev->struct_mutex);
> > -               ret = i915_gem_object_set_to_gtt_domain(ifbdev->fb->obj,
> > -                                                       true);
> > +               if (ifbdev->fb->obj)
> 
> I'm confused. Why are we doing this check now? If this is actually
> needed, then the current set_to_gtt_domain() call will blow up too. So
> this would be 2 different patches: one preventing the explosion, the
> other doing obj_invalidate. Unless we don't need the obj check.

I'm confused as well. I could swear I was just putting back an "if" that
was already here. No idea where this came from and don't see a need for
it...  v++ on the way... 

> 
> > +                       intel_fb_obj_invalidate(ifbdev->fb->obj, ORIGIN_GTT);
> >                 mutex_unlock(&fb_helper->dev->struct_mutex);
> >         }
> >
> > --
> > 2.1.0
> >
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> 
> 

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx