> On Sep 3, 2019, at 1:03 AM, Sebastian Andrzej Siewior <bigeasy@xxxxxxxxxxxxx> wrote:
>
> On 2019-08-19 19:33:18 [-0500], Clark Williams wrote:
>> From: Clark Williams <williams@xxxxxxxxxx>
>>
>> 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@xxxxxxxxxxxxxxx>
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
