[Intel-gfx] Fwd: [PATCH] drm/i915: Avoid vblank counter for gen9+
Rodrigo Vivi
rodrigo.vivi at gmail.com
Fri Feb 26 18:02:20 UTC 2016
On Mon, Feb 22, 2016 at 6:33 AM, Imre Deak <imre.deak at intel.com> wrote:
> 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.
>>
>> Do you have better ideas?
>
> Would it be possible to check the DC5/6 entry counters whenever you try
> to access to vblank counter and consider them corrupted if they
> increased since the last access? In this case you could adjust the
> counter based on some timestamp difference.
Imre, thanks a lot for the idea and reviving this topic. I'd like to
know your opinion on the approach with patches 1,2,3 that I wrote
below:
Unfortunately we cannot use those counters for anything.
But anyways, they are just entry counters and for every enter it would
take vblank number to zero.
so, if we waited 100 vblanks and dc entered 100->0
and if we waited 1 vblank and dc entered 1->0
so seems unpredictable...
Well, I have this tree:
https://cgit.freedesktop.org/~vivijim/drm-intel/log/?h=rpm-domains-psr-vblank-counter-full
with mainly:
1 - vblank domain on pre-enable post-disable vblanks hooks as Ville
had suggested
2 - psr domain so we just enable dc state when screen is really in idle.
3 - restore counter on vblank enable.
>From what I understood so far of this problem, only the patch 1 should
be enough, but with only this one I don't get the screen frozen but
the typying is so slow that is visible that we have something
wrong.... Maybe dc state transition with mutexes there are slow?
Patch 2 by iitself also doesn't solve this and I still have frozen
screens, but when combined to patch 1 everything works really well...
In the point that I believe we really don't need patch 3.
But I didn't submit any series yet because I'd like to understand more
of what is happening here and probably with a good i-g-t coverage for
this case.
Please let me know your thoughts.
Thanks,
Rodrigo.
>
> --Imre
>
>> Thanks,
>> Rodrigo.
>>
>> ---------- Forwarded message ----------
>> From: Rodrigo Vivi <rodrigo.vivi at gmail.com>
>> Date: Wed, Feb 17, 2016 at 3:14 PM
>> Subject: Re: [Intel-gfx] [PATCH] drm/i915: Avoid vblank counter for
>> gen9+
>> To: Daniel Vetter <daniel at ffwll.ch>, Patrik Jakobsson
>> <patrik.jakobsson at linux.intel.com>
>> Cc: Rodrigo Vivi <rodrigo.vivi at intel.com>, intel-gfx
>> <intel-gfx at lists.freedesktop.org>
>>
>>
>> On Tue, Feb 16, 2016 at 7:50 AM, Daniel Vetter <daniel at ffwll.ch>
>> 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 at intel.com>
>> >
>> > 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 at lists.freedesktop.org
>> > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
>> >
>> > --
>> > Daniel Vetter
>> > Software Engineer, Intel Corporation
>> > http://blog.ffwll.ch
>> > _______________________________________________
>> > Intel-gfx mailing list
>> > Intel-gfx at lists.freedesktop.org
>> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
>>
>>
>>
>> --
>> Rodrigo Vivi
>> Blog: http://blog.vivi.eng.br
>>
>>
--
Rodrigo Vivi
Blog: http://blog.vivi.eng.br
More information about the Intel-gfx
mailing list