Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Content-Type: text/plain;
        charset=utf-8
Mime-Version: 1.0 (Mac OS X Mail 12.4 \(3445.104.11\))
Subject: Re: [PREEMPT_RT PATCH 2/3] i915: convert all irq_locks spinlocks to
 raw spinlocks
From:   Sean V Kelley <sean.v.kelley@linux.intel.com>
In-Reply-To: <20190903080335.pe45dmgmjvdvbyd4@linutronix.de>
Date:   Mon, 16 Sep 2019 14:21:22 -0700
Cc:     Clark Williams <clark.williams@gmail.com>, bigeasy@linutronix.com,
        tglx@linutronix.com, linux-rt-users@vger.kernel.org,
        linux-kernel@vger.kernel.org
Content-Transfer-Encoding: 8BIT
Message-Id: <9EF2695D-3FBD-40E0-BE8A-EB71AF4155A5@linux.intel.com>
References: <20190820003319.24135-1-clark.williams@gmail.com>
 <20190820003319.24135-3-clark.williams@gmail.com>
 <20190903080335.pe45dmgmjvdvbyd4@linutronix.de>
To:     Sebastian Andrzej Siewior <bigeasy@linutronix.de>
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk


> On Sep 3, 2019, at 1:03 AM, Sebastian Andrzej Siewior <bigeasy@linutronix.de> wrote:
> 
> On 2019-08-19 19:33:18 [-0500], Clark Williams wrote:
>> From: Clark Williams <williams@redhat.com>
>> 
>> The following structures contain a member named 'irq_lock'.
>> These three locks are of type spinlock_t and are used in
>> multiple contexts including atomic:
>> 
>>    struct drm_i915_private
>>    struct intel_breadcrumbs
>>    strict intel_guc
>> 
>> Convert them all to be raw_spinlock_t so that lockdep and the lock
>> debugging code will be happy.
> 
> What is your motivation to make the lock raw?
> I did the following:
> 
> void intel_engine_signal_breadcrumbs(struct intel_engine_cs *engine)
> {
> -       local_irq_disable();
> -       intel_engine_breadcrumbs_irq(engine);
> -       local_irq_enable();
> +       if (IS_ENABLED(CONFIG_PREEMPT_RT_FULL)) {
> +               intel_engine_breadcrumbs_irq(engine);
> +       } else {
> +               local_irq_disable();
> +               intel_engine_breadcrumbs_irq(engine);
> +               local_irq_enable();
> +       }
> }
> 
> and lockdep was quiet (+ ignoring/patching the lockdep-irq-off-asserts).
> The local_irq_disable() is here (my interpretation of the situation)
> because that function is called from process context while the remaining
> callers invoke intel_engine_breadcrumbs_irq() from the interrupt
> handler and it acquires irq_lock via a plain spin_lock().  That
> local_irq_disable() would be required if everyone did a _irqsave().

I’ve tested this also on the v5.2.14-rt7 and can confirm that it avoids the need for making the locks raw.

Tested-by: Sean V Kelley <sean.v.kelley@linux.intel.com>

Thanks,

Sean

> 
> I tried to check how much worse the latency gets here but I didn't see
> anything in a brief test. What I saw however is that switching to
> fullscreen while playing a video gives me ~0.5 to ~2ms latency. This is
> has nothing to do with this change, I have to dig deeper… It might be
> one of the preempt_disable() section I just noticed.
> I would prefer to keep the lock non-raw unless there is actual need for
> it.
> 
> Sebastian