Re: [PATCH] drm/i915: Fix frontbuffer false positve.

[Matt Roper <matthew.d.roper@intel.com>]

On Tue, Feb 03, 2015 at 10:46:49AM -0800, Rodrigo Vivi wrote:
> On Tue, Feb 3, 2015 at 8:21 AM, Matt Roper <matthew.d.roper@intel.com> wrote:
> > On Tue, Feb 03, 2015 at 12:57:31PM +0100, Daniel Vetter wrote:
> >> On Mon, Feb 02, 2015 at 03:38:16PM -0800, Rodrigo Vivi wrote:
> >> > frontbuffer bits must be updated during commit times not on atomica prepare
> >> > one, otherwise we have a risk of false positive.
> >> >
> >> > Cc Daniel Vetter <daniel.vetter@ffwll.ch>
> >> > Cc: Sonika Jindal <sonika.jindal@intel.com>
> >> > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> >>
> >> atomic.fb_bits isn't used at all right now, instead the
> >> begin_crtc_commit function recomputes them. That looks wrong.
> >
> > We build up the collection of bits in atomic.fb_bits while going through
> > the atomic pipeline for each plane, then do a single call to
> >
> >         intel_frontbuffer_flip(dev, intel_crtc->atomic.fb_bits);
> >
> > in intel_finish_crtc_commit to flush them all out together (and if
> > check/prepare fail, we never actually get to that flush).
> >
> >> Also for async commits we need
> >> to do the proper 2-stage flip stuff that current page_flip code does using
> >> frontbuffer_flip_prepare/complete.
> >
> > I need to look at the frontbuffer stuff again...maybe we don't need
> > atomic.fb_bits at all and should just use intel_frontbuffer_flip_prepare
> > in the places we set the bits now and intel_frontbuffer_flip_complete
> > where we're calling the flip mentioned above?
> >
> >
> > Matt
> >
> >>
> >> This patch here should have 0 effect (presuming I'm reading code
> >> correctly), so what kinf of bug exactly are you seeing?
> 
> Yeah, after I sent the patch I was thinking about that: that it should
> have 0 effect,
> but the symptom that this apparently fixed here is that PSR wasn't
> starting at all without
> doing something like going to fbcon and come back to X.
> Something similar what Sonika had told on that psr-skl thread where
> she couldn't get psr working on login screen.
> After this patch I didn't' have to change back and forth to fbcon to
> make psr work.
> 
> Maybe just a coincidence, but anyway I believe the way it is nowadays
> it is wrong. I believe frontbuffer bits and calls should be done at
> commit step, not under prepare.

I may be misunderstanding what you're saying, but I think this is the
way things already work?  We plan out which bits we're going to update
in the 'check' step (and record those bits in atomic.fb_bits), but then
we only actually set them with intel_frontbuffer_flip() in commit if
everything looks good.  If we wind up rejecting the update because we
fail to check/prepare, those bits in atomic.fb_bits should just get
thrown away, unless I'm missing something.


Matt

> 
> >>
> >> Adding Matt&Ander.
> >> -Daniel
> >> --
> >> Daniel Vetter
> >> Software Engineer, Intel Corporation
> >> +41 (0) 79 365 57 48 - http://blog.ffwll.ch
> >
> > --
> > Matt Roper
> > Graphics Software Engineer
> > IoTG Platform Enabling & Development
> > Intel Corporation
> > (916) 356-2795
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> 
> 
> -- 
> Rodrigo Vivi
> Blog: http://blog.vivi.eng.br

-- 
Matt Roper
Graphics Software Engineer
IoTG Platform Enabling & Development
Intel Corporation
(916) 356-2795
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx