On to, 2016-02-18 at 08:56 -0800, Rodrigo Vivi wrote:
> Imre, Patrik, do you know if I'm missing something or what I'm doing
> wrong with this power domain handler for vblanks to avoid DC states
> when we need a reliable frame counter in place.
The WARN is due to the spin_lock() in drm_vblank_enable(), you can't
call power domain functions in atomic context, due to the mutex the
power domain and runtime PM fw uses.
--Imre
>
> Do you have better ideas?
>
> Thanks,
> Rodrigo.
>
> ---------- Forwarded message ----------
> From: Rodrigo Vivi <rodrigo.vivi@xxxxxxxxx>
> Date: Wed, Feb 17, 2016 at 3:14 PM
> Subject: Re: [PATCH] drm/i915: Avoid vblank counter for
> gen9+
> To: Daniel Vetter <daniel@xxxxxxxx>, Patrik Jakobsson
> <patrik.jakobsson@xxxxxxxxxxxxxxx>
> Cc: Rodrigo Vivi <rodrigo.vivi@xxxxxxxxx>, intel-gfx
> <intel-gfx@xxxxxxxxxxxxxxxxxxxxx>
>
>
> On Tue, Feb 16, 2016 at 7:50 AM, Daniel Vetter <daniel@xxxxxxxx>
> wrote:
> > On Thu, Feb 11, 2016 at 09:00:47AM -0800, Rodrigo Vivi wrote:
> > > Framecounter register is read-only so DMC cannot restore it
> > > after exiting DC5 and DC6.
> > >
> > > Easiest way to go is to avoid the counter and use vblank
> > > interruptions for this platform and for all the following
> > > ones since DMC came to stay. At least while we can't change
> > > this register to read-write.
> > >
> > > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@xxxxxxxxx>
> >
> > Now my comments also in public:
> > - Do we still get reasonable dc5 residency with this - it means
> > we'll keep
> > vblank irq running forever.
> >
> > - I'm a bit unclear on what exactly this fixes - have you tested
> > that
> > long-lasting vblank waits are still accurate? Just want to make
> > sure we
> > don't just paper over the issue and desktops can still get stuck
> > waiting
> > for a vblank.
>
> apparently no... so please just ignore this patch for now... after a
> while with that patch I was seeing the issue again...
>
> >
> > Just a bit suprised that the only problem is the framecounter, and
> > not
> > that vblanks stop happening too.
> >
> > We need to also know these details for the proper fix, which will
> > involve
> > grabbing power well references (might need a new one for vblank
> > interrupts) to make sure.
>
> Yeap, I liked this idea... so combining a power domain reference with
> a vblank count restore once we know the dc off is blocked we could
> workaround this case... something like:
>
> diff --git a/drivers/gpu/drm/i915/i915_irq.c
> b/drivers/gpu/drm/i915/i915_irq.c
> index 25a8937..2b18778 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -2743,7 +2743,10 @@ static int gen8_enable_vblank(struct
> drm_device
> *dev, unsigned int pipe)
> struct drm_i915_private *dev_priv = dev->dev_private;
> unsigned long irqflags;
>
> + intel_display_power_get(dev_priv, POWER_DOMAIN_VBLANK);
> +
> spin_lock_irqsave(&dev_priv->irq_lock, irqflags);
> + dev->vblank[pipe].last = g4x_get_vblank_counter(dev, pipe);
> bdw_enable_pipe_irq(dev_priv, pipe, GEN8_PIPE_VBLANK);
> spin_unlock_irqrestore(&dev_priv->irq_lock, irqflags);
>
> @@ -2796,6 +2799,8 @@ static void gen8_disable_vblank(struct
> drm_device *dev, unsigned int pipe)
> spin_lock_irqsave(&dev_priv->irq_lock, irqflags);
> bdw_disable_pipe_irq(dev_priv, pipe, GEN8_PIPE_VBLANK);
> spin_unlock_irqrestore(&dev_priv->irq_lock, irqflags);
> +
> + intel_display_power_put(dev_priv, POWER_DOMAIN_VBLANK);
> }
>
> where POWER_DOMAIN_VBLANK is part of:
> #define SKL_DISPLAY_DC_OFF_POWER_DOMAINS ( \
> BIT(POWER_DOMAIN_VBLANK) | \
>
>
> However I have my dmesg flooded by:
>
>
> [ 69.025562] BUG: sleeping function called from invalid context at
> drivers/base/power/runtime.c:955
> [ 69.025576] in_atomic(): 1, irqs_disabled(): 1, pid: 995, name:
> Xorg
> [ 69.025582] Preemption disabled at:[<ffffffff815a1fee>]
> drm_vblank_get+0x4e/0xd0
>
> [ 69.025619] CPU: 3 PID: 995 Comm: Xorg Tainted: G U W
> 4.5.0-rc4+ #11
> [ 69.025628] Hardware name: Intel Corporation Kabylake Client
> platform/Skylake U DDR3L RVP7, BIOS KBLSE2R1.R00.X019.B01.1512230743
> 12/23/2015
> [ 69.025637] 0000000000000000 ffff88003f0bfbb0 ffffffff8148e983
> 0000000000000000
> [ 69.025653] ffff880085b04200 ffff88003f0bfbd0 ffffffff81133ece
> ffffffff81d77f23
> [ 69.025667] 00000000000003bb ffff88003f0bfbf8 ffffffff81133f89
> ffff88016913a098
> [ 69.025680] Call Trace:
> [ 69.025697] [<ffffffff8148e983>] dump_stack+0x65/0x92
> [ 69.025711] [<ffffffff81133ece>] ___might_sleep+0x10e/0x180
> [ 69.025722] [<ffffffff81133f89>] __might_sleep+0x49/0x80
> [ 69.025739] [<ffffffff815d21d9>] __pm_runtime_resume+0x79/0x80
> [ 69.025841] [<ffffffffa005ebc8>] intel_runtime_pm_get+0x28/0x90
> [i915]
> [ 69.025924] [<ffffffffa005ec49>]
> intel_display_power_get+0x19/0x50 [i915]
> [ 69.025995] [<ffffffffa004aea4>] gen8_enable_vblank+0x34/0xc0
> [i915]
> [ 69.026016] [<ffffffff815a1f46>] drm_vblank_enable+0x76/0xd0
>
>
>
>
> Another thing that I search in the spec was for an Interrupt to know
> when we came back from DC5 or DC6 or got power well re-enabled, so we
> would be able to restore the drm last counter... but I couldn't find
> any...
>
>
> Any other idea?
>
>
> >
> > Cheers, Daniel
> >
> > > ---
> > > drivers/gpu/drm/i915/i915_irq.c | 7 +++++--
> > > 1 file changed, 5 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/i915/i915_irq.c
> > > b/drivers/gpu/drm/i915/i915_irq.c
> > > index 25a8937..c294a4b 100644
> > > --- a/drivers/gpu/drm/i915/i915_irq.c
> > > +++ b/drivers/gpu/drm/i915/i915_irq.c
> > > @@ -4556,7 +4556,10 @@ void intel_irq_init(struct
> > > drm_i915_private *dev_priv)
> > >
> > > pm_qos_add_request(&dev_priv->pm_qos,
> > > PM_QOS_CPU_DMA_LATENCY, PM_QOS_DEFAULT_VALUE);
> > >
> > > - if (IS_GEN2(dev_priv)) {
> > > + if (INTEL_INFO(dev_priv)->gen >= 9) {
> > > + dev->max_vblank_count = 0;
> > > + dev->driver->get_vblank_counter =
> > > g4x_get_vblank_counter;
> > > + } else if (IS_GEN2(dev_priv)) {
> > > dev->max_vblank_count = 0;
> > > dev->driver->get_vblank_counter =
> > > i8xx_get_vblank_counter;
> > > } else if (IS_G4X(dev_priv) || INTEL_INFO(dev_priv)->gen >=
> > > 5) {
> > > @@ -4572,7 +4575,7 @@ void intel_irq_init(struct drm_i915_private
> > > *dev_priv)
> > > * Gen2 doesn't have a hardware frame counter and so
> > > depends on
> > > * vblank interrupts to produce sane vblank seuquence
> > > numbers.
> > > */
> > > - if (!IS_GEN2(dev_priv))
> > > + if (!IS_GEN2(dev_priv) && !INTEL_INFO(dev_priv)->gen >= 9)
> > > dev->vblank_disable_immediate = true;
> > >
> > > dev->driver->get_vblank_timestamp =
> > > i915_get_vblank_timestamp;
> > > --
> > > 2.4.3
> > >
> > > _______________________________________________
> > > Intel-gfx mailing list
> > > Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
> > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> >
> > --
> > Daniel Vetter
> > Software Engineer, Intel Corporation
> > http://blog.ffwll.ch
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
>
>
> --
> Rodrigo Vivi
> Blog: http://blog.vivi.eng.br
>
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
