(documenting what we discussed on IRC)
2014-06-20 13:29 GMT-03:00 Jesse Barnes <jbarnes@xxxxxxxxxxxxxxxx>:
> This was always the case on our suspend path, but it was recently
> exposed by the change to use our runtime IRQ disable routine rather than
> the full DRM IRQ disable. Keep the warning on the enable side, as that
> really would indicate a bug.
While I understand the patch and think it's a reasonable thing to do,
I feel the need to spend some time persuading you in replacing it with
something that doesn't involve removing WARNs from our driver. While
the driver is runtime suspended, no one should really be manipulating
IRQs, even if they're disabling stuff that is already disabled: this
reflects there's probably a problem somewhere. These WARNs are
extremely helpful for the runtime PM feature because almost nobody
actually uses runtime PM to notice any bugs with it, so the WARNs can
make QA report bugs and bisect things for us.
Also, it seems S3 suspend is currently a little disaster on our
driver. Your 6 patches just solve some of the WARNs, not all of them.
And last week I even solved another WARN on the S3 path. I just did
some investigation, and the current bad commits are:
8abdc17941c71b37311bb93876ac83dce58160c8,
e11aa362308f5de467ce355a2a2471321b15a35c and
85e90679335f56d162f4a0ff525573818e17ce44. If I just revert these 3
commits, S3 doesn't give me any WARNs.
Instead of the change you proposed, can't we think of another solution
that would maybe address all the 3 regressions we have? Since we're
always submitting patches to change the order we do things at S3
suspend/resume, shouldn't we add something like "dev_priv->suspending"
that could be used to avoid the strict ordering that is required
during runtime?
>
> Signed-off-by: Jesse Barnes <jbarnes@xxxxxxxxxxxxxxxx>
> ---
> drivers/gpu/drm/i915/i915_irq.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index 1c1ec22..fe3b309 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -151,7 +151,7 @@ ironlake_disable_display_irq(struct drm_i915_private *dev_priv, u32 mask)
> {
> assert_spin_locked(&dev_priv->irq_lock);
>
> - if (WARN_ON(dev_priv->pm.irqs_disabled))
> + if (dev_priv->pm.irqs_disabled)
> return;
>
> if ((dev_priv->irq_mask & mask) != mask) {
> --
> 1.8.3.2
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Paulo Zanoni
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
