On Wed, May 02, 2018 at 03:31:15PM -0700, Tarun Vyas wrote:
> On Wed, May 02, 2018 at 01:04:06PM -0700, Vivi, Rodrigo wrote:
> > On Wed, May 02, 2018 at 09:51:43PM +0300, Ville Syrjälä wrote:
> > > On Wed, May 02, 2018 at 11:19:14AM -0700, Tarun Vyas wrote:
> > > > On Mon, Apr 30, 2018 at 10:19:33AM -0700, Rodrigo Vivi wrote:
> > > > > On Sun, Apr 29, 2018 at 09:00:18PM -0700, Tarun Vyas wrote:
> > > > > > From: Tarun <tarun.v...@intel.com>
> > > > > >
> > > > > > The PIPEDSL freezes on PSR entry and if PSR hasn't fully exited,
> > > > > > then
> > > > > > the pipe_update_start call schedules itself out to check back later.
> > > > > >
> > > > > > On ChromeOS-4.4 kernel, which is fairly up-to-date w.r.t drm/i915
> > > > > > but
> > > > > > lags w.r.t core kernel code, hot plugging an external display
> > > > > > triggers
> > > > > > tons of "potential atomic update errors" in the dmesg, on *pipe A*.
> > > > > > A
> > > > > > closer analysis reveals that we try to read the scanline 3 times and
> > > > > > eventually timeout, b/c PSR hasn't exited fully leading to a PIPEDSL
> > > > > > stuck @ 1599. This issue is not seen on upstream kernels, b/c for
> > > > > > *some*
> > > > > > reason we loop inside intel_pipe_update start for ~2+ msec which in
> > > > > > this
> > > > > > case is more than enough to exit PSR fully, hence an *unstuck*
> > > > > > PIPEDSL
> > > > > > counter, hence no error. On the other hand, the ChromeOS kernel
> > > > > > spends
> > > > > > ~1.1 msec looping inside intel_pipe_update_start and hence errors
> > > > > > out
> > > > > > b/c the source is still in PSR.
> > > > > >
> > > > > > Regardless, we should wait for PSR exit (if PSR is supported and
> > > > > > active
> > > > > > on the current pipe) before reading the PIPEDSL, b/c if we haven't
> > > > > > fully exited PSR, then checking for vblank evasion isn't actually
> > > > > > applicable.
> > > > > >
> > > > > > This scenario applies to a configuration with an additional pipe,
> > > > > > as of now.
> > > > >
> > > > > I honestly believe you picking the wrong culprit here. By
> > > > > "coincidence".
> > > > > PSR will allow DC state with screen on and DC state will mess up with
> > > > > all
> > > > > registers reads....
> > > > >
> > > > > probably what you are missing you your kernel is some power domain
> > > > > grab that would keep DC_OFF and consequently a sane read of these
> > > > > registers.
> > > > >
> > > > > Maybe Imre has a quick idea of what you could be missing on your
> > > > > kernel
> > > > > that we already have on upstream one.
> > > > >
> > > > > Thanks,
> > > > > Rodrigo.
> > > > >
> > > > Thanks for the quick response Rodrigo !
> > > > Some key observations based on my experiments so far:
> > > > for (;;) {
> > > > /*
> > > > * prepare_to_wait() has a memory barrier, which
> > > > guarantees
> > > > * other CPUs can see the task state update by the time
> > > > we
> > > > * read the scanline.
> > > > */
> > > > prepare_to_wait(wq, &wait, TASK_UNINTERRUPTIBLE);
> > > >
> > > > scanline = intel_get_crtc_scanline(crtc);
> > > > if (scanline < min || scanline > max)
> > > > break;
> > > >
> > > > if (timeout <= 0) {
> > > > DRM_ERROR("Potential atomic update failure on
> > > > pipe %c\n",
> > > > pipe_name(crtc->pipe));
> > > > break;
> > > > }
> > > >
> > > > local_irq_enable();
> > > >
> > > > timeout = schedule_timeout(timeout);
> > > >
> > > > local_irq_disable();
> > > > }
> > > > 1. In the above loop inside pipe_update_start, the *first time*, we
> > > > read the PIPEDSL, with PSR1 and external display connected, it always
> > > > reads 1599, for *both* the kernels(upstream and ChromeOS-4.4) . The
> > > > PSR_STATUS also reads the exact same for *both* kernels and shows that
> > > > we haven't *fully* exited PSR.
> > > >
> > > > 2. The difference between the two kernels comes after this first read
> > > > of the PIPEDSL. ChromeOS-4.4 spends ~1 msec inside that loop and
> > > > upstream spends ~2msec. I suspect that it is because of the scheduling
> > > > changes between the two kernels, b/c I can't find any i915 specific
> > > > code running in that loop, except for vblank processing.
> > > >
> > > > 3. So to summarize it, both the kernels are in the same state w.r.t PSR
> > > > and PIPEDSL value when they read the PIPEDSL for the first time inside
> > > > the loop. *When* the kernels *transition* to a *full PSR exit* is what
> > > > is differing.
> >
> > Oh! So you really are getting reliable counters....
> >
> > > >
> > > > My rationale for this patch is that, the pipe_update_start function is
> > > > meant to evade 100 usec before a vblank, but, *if* we haven't *fully*
> > > > exited PSR (which is true for both the kernels for the first PIPEDSL
> > > > read), then vblank evasion is *not applicable* b/c the PIPEDSL will be
> > > > messed up. So we shouldn't bother evading vblank until we have fully
> > > > exited PSR.
> > >
> > > Yeah, I think this is the right direction. The problem really is the
> > > extra vblank pulse that the hardware generates (or at least can
> > > generate depending on a chicken bit) when it exits PSR. We have no
> > > control over when that happens and hence we have no control over when
> > > the registers get latched. And yet we still have to somehow prevent
> > > the register latching from occurring while we're in middle of
> > > reprogramming them.
> >
> > I see the problem now. Thanks for the explanation.
> >
> > >
> > > There are a couple of ways to avoid this:
> > > 1) Set the chicken bit so that we don't get the vblank pulse. The
> > > pipe should restart from the vblank start, so we would have one
> > > full frame to reprogam the registers. Howver IIRC DK told me
> > > there is no way to fully eliminate it in all cases so this
> > > option is probably out. There was also some implication for FBC
> > > which I already forgot.
> > > 2) Make sure we've exited PSR before repgrogamming the registers
> > > (ie. what you do).
> > > 3) Use the DOUBLE_BUFFER_CTL to prevent the extra vblank pulse from
> > > latching the registers while we're still reprogramming them.
> > > This feature only exists on SKL+ so is not a solution for
> > > HSW/BDW. But maybe HSW/BDW didn't even have the extra vblank
> > > pulse?
> > >
> > > Option 2) does provide a consistent behaviour on all platforms, so I
> > > do kinda like it. It also avoids a bigger reword on account of the
> > > DOUBLE_BUFFER_CTL. I do think we'll have to start using
> > > DOUBLE_BUFFER_CTL anyway due to other issues, but at least this way
> > > we don't block PSR progress on that work.
> >
> > My vote is for the option 2. Seems more straighforward and more broad.
> >
> > DK?
> >
> > My only request on the patch itself would be to create a function
> > on intel_psr.c intel_psr_wait_for_idle... or something like this
> > and put the register wait logic inside it instead of spreading
> > the psr code around.
> >
> > Thanks,
> > Rodrigo.
> >
> > >
> > > --
> > > Ville Syrjälä
> > > Intel
> Thanks for the comments, Ville and Rodrigo. I'll rework this to move the wait
> to intel_psr.c. There is a psr_wait_for_idle() in there, but there are some
> PSR locks being passed around inside it (eventually released by the caller).
> Also,the max timeout specified there is 50 msec which might be way too much ?
ouch! that function is ugly.... unlock than lock back again...
(specially unlock without any assert locked... :/)
If you can improve that or split in a way that we reuse some code it would be
nice...
>
> Best,
> Tarun
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
