[PATCH 00/11] Spinlock use clarification in i915

List overview All Threads
Download

newer

older

PATCH: radeondrm x86_64 and...

DRM encoder/bridge architecture...

Daniel Vetter

15 Sep 2014 15 Sep '14

12:35 p.m.

Hi all,

So I've tried to figure out a way how to clear up our irq setup mess, but instead only managed to get sidetracked on a spinlock usage review.

Review highly welcome.

Cheers, Daniel

Daniel Vetter (11): drm/i915: Clarify event_lock locking, process context drm/i915: Clarify event_lock locking, irq&mixed context drm/i915: Clarify gpu_error.lock locking drm/i915: Clarify irq_lock locking, intel_tv_detect drm/i915: Clarify irq_lock locking, work functions drm/i915: Clarify irq_lock locking, interrupt install/uninstall drm/i915: Clarify irq_lock locking, irq handlers drm/i915: Clarify irq_lock locking, special cases drm/i915: Convert backlight_lock to a mutex drm/i915: Clarify uncore.lock locking drm/i915: Clarify mmio_flip_lock locking

drivers/gpu/drm/i915/i915_debugfs.c | 5 +- drivers/gpu/drm/i915/i915_dma.c | 2 +- drivers/gpu/drm/i915/i915_drv.c | 5 +- drivers/gpu/drm/i915/i915_drv.h | 2 +- drivers/gpu/drm/i915/i915_gpu_error.c | 10 ++-- drivers/gpu/drm/i915/i915_irq.c | 101 ++++++++++++++-------------------- drivers/gpu/drm/i915/intel_display.c | 67 +++++++++++----------- drivers/gpu/drm/i915/intel_panel.c | 30 ++++------ drivers/gpu/drm/i915/intel_tv.c | 9 ++- 9 files changed, 103 insertions(+), 128 deletions(-)

-- 1.9.3

Show replies by date

Daniel Vetter

15 Sep 15 Sep

12:35 p.m.

New subject: [PATCH 01/11] drm/i915: Clarify event_lock locking, process context

It's good practice to use the more specific versions for irq save spinlocks both as executable documentation and to enforce saner design. The _irqsave version really should only be used if the calling context is unknown and there's a good reason to call a function from all kinds of places.

This is the first step whice replaces all occurances of _irqsave in process context with the simpler irq disable/enable variants. We don't have any funky spinlock nesting going on, especially since the event_lock is the outermost of the irq/vblank related spinlocks.

Signed-off-by: Daniel Vetter daniel.vetter@ffwll.ch --- drivers/gpu/drm/i915/i915_debugfs.c | 5 ++--- drivers/gpu/drm/i915/intel_display.c | 35 +++++++++++++++-------------------- 2 files changed, 17 insertions(+), 23 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c index 063b44817e08..0ba5c7145240 100644 --- a/drivers/gpu/drm/i915/i915_debugfs.c +++ b/drivers/gpu/drm/i915/i915_debugfs.c @@ -516,7 +516,6 @@ static int i915_gem_pageflip_info(struct seq_file *m, void *data) struct drm_info_node *node = m->private; struct drm_device *dev = node->minor->dev; struct drm_i915_private *dev_priv = dev->dev_private; - unsigned long flags; struct intel_crtc *crtc; int ret;

@@ -529,7 +528,7 @@ static int i915_gem_pageflip_info(struct seq_file *m, void *data) const char plane = plane_name(crtc->plane); struct intel_unpin_work *work;

- spin_lock_irqsave(&dev->event_lock, flags); + spin_lock_irq(&dev->event_lock); work = crtc->unpin_work; if (work == NULL) { seq_printf(m, "No flip due on pipe %c (plane %c)\n", @@ -575,7 +574,7 @@ static int i915_gem_pageflip_info(struct seq_file *m, void *data) seq_printf(m, "MMIO update completed? %d\n", addr == work->gtt_offset); } } - spin_unlock_irqrestore(&dev->event_lock, flags); + spin_unlock_irq(&dev->event_lock); }

mutex_unlock(&dev->struct_mutex); diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index 82c0ad1f6a2e..dcbefe510a2a 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -2765,16 +2765,15 @@ static bool intel_crtc_has_pending_flip(struct drm_crtc *crtc) struct drm_device *dev = crtc->dev; struct drm_i915_private *dev_priv = dev->dev_private; struct intel_crtc *intel_crtc = to_intel_crtc(crtc); - unsigned long flags; bool pending;

if (i915_reset_in_progress(&dev_priv->gpu_error) || intel_crtc->reset_counter != atomic_read(&dev_priv->gpu_error.reset_counter)) return false;

- spin_lock_irqsave(&dev->event_lock, flags); + spin_lock_irq(&dev->event_lock); pending = to_intel_crtc(crtc)->unpin_work != NULL; - spin_unlock_irqrestore(&dev->event_lock, flags); + spin_unlock_irq(&dev->event_lock);

return pending; } @@ -3485,14 +3484,13 @@ void intel_crtc_wait_for_pending_flips(struct drm_crtc *crtc) !intel_crtc_has_pending_flip(crtc), 60*HZ) == 0)) { struct intel_crtc *intel_crtc = to_intel_crtc(crtc); - unsigned long flags;

- spin_lock_irqsave(&dev->event_lock, flags); + spin_lock_irq(&dev->event_lock); if (intel_crtc->unpin_work) { WARN_ONCE(1, "Removing stuck page flip\n"); page_flip_completed(intel_crtc); } - spin_unlock_irqrestore(&dev->event_lock, flags); + spin_unlock_irq(&dev->event_lock); }

if (crtc->primary->fb) { @@ -9337,12 +9335,11 @@ static void intel_crtc_destroy(struct drm_crtc *crtc) struct intel_crtc *intel_crtc = to_intel_crtc(crtc); struct drm_device *dev = crtc->dev; struct intel_unpin_work *work; - unsigned long flags;

- spin_lock_irqsave(&dev->event_lock, flags); + spin_lock_irq(&dev->event_lock); work = intel_crtc->unpin_work; intel_crtc->unpin_work = NULL; - spin_unlock_irqrestore(&dev->event_lock, flags); + spin_unlock_irq(&dev->event_lock);

if (work) { cancel_work_sync(&work->work); @@ -9953,7 +9950,6 @@ static int intel_crtc_page_flip(struct drm_crtc *crtc, enum pipe pipe = intel_crtc->pipe; struct intel_unpin_work *work; struct intel_engine_cs *ring; - unsigned long flags; int ret;

//trigger software GT busyness calculation @@ -9997,7 +9993,7 @@ static int intel_crtc_page_flip(struct drm_crtc *crtc, goto free_work;

/* We borrow the event spin lock for protecting unpin_work */ - spin_lock_irqsave(&dev->event_lock, flags); + spin_lock_irq(&dev->event_lock); if (intel_crtc->unpin_work) { /* Before declaring the flip queue wedged, check if * the hardware completed the operation behind our backs. @@ -10007,7 +10003,7 @@ static int intel_crtc_page_flip(struct drm_crtc *crtc, page_flip_completed(intel_crtc); } else { DRM_DEBUG_DRIVER("flip queue: crtc already busy\n"); - spin_unlock_irqrestore(&dev->event_lock, flags); + spin_unlock_irq(&dev->event_lock);

drm_crtc_vblank_put(crtc); kfree(work); @@ -10015,7 +10011,7 @@ static int intel_crtc_page_flip(struct drm_crtc *crtc, } } intel_crtc->unpin_work = work; - spin_unlock_irqrestore(&dev->event_lock, flags); + spin_unlock_irq(&dev->event_lock);

if (atomic_read(&intel_crtc->unpin_work_count) >= 2) flush_workqueue(dev_priv->wq); @@ -10102,9 +10098,9 @@ cleanup_pending: mutex_unlock(&dev->struct_mutex);

cleanup: - spin_lock_irqsave(&dev->event_lock, flags); + spin_lock_irq(&dev->event_lock); intel_crtc->unpin_work = NULL; - spin_unlock_irqrestore(&dev->event_lock, flags); + spin_unlock_irq(&dev->event_lock);

drm_crtc_vblank_put(crtc); free_work: @@ -10115,9 +10111,9 @@ out_hang: intel_crtc_wait_for_pending_flips(crtc); ret = intel_pipe_set_base(crtc, crtc->x, crtc->y, fb); if (ret == 0 && event) { - spin_lock_irqsave(&dev->event_lock, flags); + spin_lock_irq(&dev->event_lock); drm_send_vblank_event(dev, pipe, event); - spin_unlock_irqrestore(&dev->event_lock, flags); + spin_unlock_irq(&dev->event_lock); } } return ret; @@ -13826,9 +13822,8 @@ void intel_modeset_preclose(struct drm_device *dev, struct drm_file *file)

for_each_intel_crtc(dev, crtc) { struct intel_unpin_work *work; - unsigned long irqflags;

- spin_lock_irqsave(&dev->event_lock, irqflags); + spin_lock_irq(&dev->event_lock);

work = crtc->unpin_work;

@@ -13838,6 +13833,6 @@ void intel_modeset_preclose(struct drm_device *dev, struct drm_file *file) work->event = NULL; }

- spin_unlock_irqrestore(&dev->event_lock, irqflags); + spin_unlock_irq(&dev->event_lock); } }

-- 1.9.3

Daniel Vetter

12:35 p.m.

New subject: [PATCH 02/11] drm/i915: Clarify event_lock locking, irq&mixed context

Now we tackle the functions also called from interrupt handlers.

- intel_check_page_flip is exclusively called from irq handlers, so a plain spin_lock is all we need. In i915_irq.c we have the convention to give all such functions an _irq_handler postfix, but that would look strange and als be a bit a misleading name. I've opted for a WARN_ON(!in_irq()) instead.

- The other two places left are called both from interrupt handlers and from our reset work, so need the full irqsave dance. Annotate them with a short comment.

Signed-off-by: Daniel Vetter daniel.vetter@ffwll.ch --- drivers/gpu/drm/i915/intel_display.c | 18 ++++++++++++++---- 1 file changed, 14 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index dcbefe510a2a..d120dfff3841 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -9385,6 +9385,10 @@ static void do_intel_finish_page_flip(struct drm_device *dev, if (intel_crtc == NULL) return;

+ /* + * This is called both by irq handlers and the reset code (to complete + * lost pageflips) so needs the full irqsave spinlocks. + */ spin_lock_irqsave(&dev->event_lock, flags); work = intel_crtc->unpin_work;

@@ -9466,7 +9470,12 @@ void intel_prepare_page_flip(struct drm_device *dev, int plane) to_intel_crtc(dev_priv->plane_to_crtc_mapping[plane]); unsigned long flags;

- /* NB: An MMIO update of the plane base pointer will also + + /* + * This is called both by irq handlers and the reset code (to complete + * lost pageflips) so needs the full irqsave spinlocks. + * + * NB: An MMIO update of the plane base pointer will also * generate a page-flip completion irq, i.e. every modeset * is also accompanied by a spurious intel_prepare_page_flip(). */ @@ -9923,18 +9932,19 @@ void intel_check_page_flip(struct drm_device *dev, int pipe) struct drm_i915_private *dev_priv = dev->dev_private; struct drm_crtc *crtc = dev_priv->pipe_to_crtc_mapping[pipe]; struct intel_crtc *intel_crtc = to_intel_crtc(crtc); - unsigned long flags; + + WARN_ON(!in_irq());

if (crtc == NULL) return;

- spin_lock_irqsave(&dev->event_lock, flags); + spin_lock(&dev->event_lock); if (intel_crtc->unpin_work && __intel_pageflip_stall_check(dev, crtc)) { WARN_ONCE(1, "Kicking stuck page flip: queued at %d, now %d\n", intel_crtc->unpin_work->flip_queued_vblank, drm_vblank_count(dev, pipe)); page_flip_completed(intel_crtc); } - spin_unlock_irqrestore(&dev->event_lock, flags); + spin_unlock(&dev->event_lock); }

static int intel_crtc_page_flip(struct drm_crtc *crtc,

-- 1.9.3

Daniel Vetter

12:35 p.m.

New subject: [PATCH 03/11] drm/i915: Clarify gpu_error.lock locking

i915_capture_error_state can be called from all kinds of contexts, so needs the full irqsave dance. But the other two places to grab and release the error state are only called from process context. So simplify them to the plaine _irq spinlock versions to clarify the locking semantics.

Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com Signed-off-by: Daniel Vetter daniel.vetter@ffwll.ch --- drivers/gpu/drm/i915/i915_gpu_error.c | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gpu_error.c b/drivers/gpu/drm/i915/i915_gpu_error.c index 2c87a797213f..386e45dbeff1 100644 --- a/drivers/gpu/drm/i915/i915_gpu_error.c +++ b/drivers/gpu/drm/i915/i915_gpu_error.c @@ -1326,13 +1326,12 @@ void i915_error_state_get(struct drm_device *dev, struct i915_error_state_file_priv *error_priv) { struct drm_i915_private *dev_priv = dev->dev_private; - unsigned long flags;

- spin_lock_irqsave(&dev_priv->gpu_error.lock, flags); + spin_lock_irq(&dev_priv->gpu_error.lock); error_priv->error = dev_priv->gpu_error.first_error; if (error_priv->error) kref_get(&error_priv->error->ref); - spin_unlock_irqrestore(&dev_priv->gpu_error.lock, flags); + spin_unlock_irq(&dev_priv->gpu_error.lock);

}

@@ -1346,12 +1345,11 @@ void i915_destroy_error_state(struct drm_device *dev) { struct drm_i915_private *dev_priv = dev->dev_private; struct drm_i915_error_state *error; - unsigned long flags;

- spin_lock_irqsave(&dev_priv->gpu_error.lock, flags); + spin_lock_irq(&dev_priv->gpu_error.lock); error = dev_priv->gpu_error.first_error; dev_priv->gpu_error.first_error = NULL; - spin_unlock_irqrestore(&dev_priv->gpu_error.lock, flags); + spin_unlock_irq(&dev_priv->gpu_error.lock);

if (error) kref_put(&error->ref, i915_error_state_free);

-- 1.9.3

Daniel Vetter

12:35 p.m.

New subject: [PATCH 04/11] drm/i915: Clarify irq_lock locking, intel_tv_detect

->detect callbacks are only ever called from process context, and there's no fancy nesting going on here. So plain _irq spinlock variants is what we want.

Signed-off-by: Daniel Vetter daniel.vetter@ffwll.ch --- drivers/gpu/drm/i915/intel_tv.c | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_tv.c b/drivers/gpu/drm/i915/intel_tv.c index c14341ca3ef9..6f5f59b880f5 100644 --- a/drivers/gpu/drm/i915/intel_tv.c +++ b/drivers/gpu/drm/i915/intel_tv.c @@ -1182,18 +1182,17 @@ intel_tv_detect_type(struct intel_tv *intel_tv, struct intel_crtc *intel_crtc = to_intel_crtc(crtc); struct drm_device *dev = encoder->dev; struct drm_i915_private *dev_priv = dev->dev_private; - unsigned long irqflags; u32 tv_ctl, save_tv_ctl; u32 tv_dac, save_tv_dac; int type;

/* Disable TV interrupts around load detect or we'll recurse */ if (connector->polled & DRM_CONNECTOR_POLL_HPD) { - spin_lock_irqsave(&dev_priv->irq_lock, irqflags); + spin_lock_irq(&dev_priv->irq_lock); i915_disable_pipestat(dev_priv, 0, PIPE_HOTPLUG_INTERRUPT_STATUS | PIPE_HOTPLUG_TV_INTERRUPT_STATUS); - spin_unlock_irqrestore(&dev_priv->irq_lock, irqflags); + spin_unlock_irq(&dev_priv->irq_lock); }

save_tv_dac = tv_dac = I915_READ(TV_DAC); @@ -1266,11 +1265,11 @@ intel_tv_detect_type(struct intel_tv *intel_tv,

/* Restore interrupt config */ if (connector->polled & DRM_CONNECTOR_POLL_HPD) { - spin_lock_irqsave(&dev_priv->irq_lock, irqflags); + spin_lock_irq(&dev_priv->irq_lock); i915_enable_pipestat(dev_priv, 0, PIPE_HOTPLUG_INTERRUPT_STATUS | PIPE_HOTPLUG_TV_INTERRUPT_STATUS); - spin_unlock_irqrestore(&dev_priv->irq_lock, irqflags); + spin_unlock_irq(&dev_priv->irq_lock); }

return type;

-- 1.9.3

Daniel Vetter

12:35 p.m.

New subject: [PATCH 05/11] drm/i915: Clarify irq_lock locking, work functions

Work functions are in process context, so plain _irq spinlock variants is all we need.

The hpd reenable work didn't follow the _work/_work_func postfix naming scheme, so adjust that while at it.

Signed-off-by: Daniel Vetter daniel.vetter@ffwll.ch --- drivers/gpu/drm/i915/i915_irq.c | 28 ++++++++++++---------------- 1 file changed, 12 insertions(+), 16 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c index d22f87020aee..4906823baa11 100644 --- a/drivers/gpu/drm/i915/i915_irq.c +++ b/drivers/gpu/drm/i915/i915_irq.c @@ -1096,18 +1096,17 @@ static void i915_digport_work_func(struct work_struct *work) { struct drm_i915_private *dev_priv = container_of(work, struct drm_i915_private, dig_port_work); - unsigned long irqflags; u32 long_port_mask, short_port_mask; struct intel_digital_port *intel_dig_port; int i, ret; u32 old_bits = 0;

- spin_lock_irqsave(&dev_priv->irq_lock, irqflags); + spin_lock_irq(&dev_priv->irq_lock); long_port_mask = dev_priv->long_hpd_port_mask; dev_priv->long_hpd_port_mask = 0; short_port_mask = dev_priv->short_hpd_port_mask; dev_priv->short_hpd_port_mask = 0; - spin_unlock_irqrestore(&dev_priv->irq_lock, irqflags); + spin_unlock_irq(&dev_priv->irq_lock);

for (i = 0; i < I915_MAX_PORTS; i++) { bool valid = false; @@ -1132,9 +1131,9 @@ static void i915_digport_work_func(struct work_struct *work) }

if (old_bits) { - spin_lock_irqsave(&dev_priv->irq_lock, irqflags); + spin_lock_irq(&dev_priv->irq_lock); dev_priv->hpd_event_bits |= old_bits; - spin_unlock_irqrestore(&dev_priv->irq_lock, irqflags); + spin_unlock_irq(&dev_priv->irq_lock); schedule_work(&dev_priv->hotplug_work); } } @@ -1153,7 +1152,6 @@ static void i915_hotplug_work_func(struct work_struct *work) struct intel_connector *intel_connector; struct intel_encoder *intel_encoder; struct drm_connector *connector; - unsigned long irqflags; bool hpd_disabled = false; bool changed = false; u32 hpd_event_bits; @@ -1161,7 +1159,7 @@ static void i915_hotplug_work_func(struct work_struct *work) mutex_lock(&mode_config->mutex); DRM_DEBUG_KMS("running encoder hotplug functions\n");

- spin_lock_irqsave(&dev_priv->irq_lock, irqflags); + spin_lock_irq(&dev_priv->irq_lock);

hpd_event_bits = dev_priv->hpd_event_bits; dev_priv->hpd_event_bits = 0; @@ -1195,7 +1193,7 @@ static void i915_hotplug_work_func(struct work_struct *work) msecs_to_jiffies(I915_REENABLE_HOTPLUG_DELAY)); }

- spin_unlock_irqrestore(&dev_priv->irq_lock, irqflags); + spin_unlock_irq(&dev_priv->irq_lock);

list_for_each_entry(connector, &mode_config->connector_list, head) { intel_connector = to_intel_connector(connector); @@ -1490,7 +1488,6 @@ static void ivybridge_parity_work(struct work_struct *work) u32 error_status, row, bank, subbank; char *parity_event[6]; uint32_t misccpctl; - unsigned long flags; uint8_t slice = 0;

/* We must turn off DOP level clock gating to access the L3 registers. @@ -1549,9 +1546,9 @@ static void ivybridge_parity_work(struct work_struct *work)

out: WARN_ON(dev_priv->l3_parity.which_slice); - spin_lock_irqsave(&dev_priv->irq_lock, flags); + spin_lock_irq(&dev_priv->irq_lock); gen5_enable_gt_irq(dev_priv, GT_PARITY_ERROR(dev_priv->dev)); - spin_unlock_irqrestore(&dev_priv->irq_lock, flags); + spin_unlock_irq(&dev_priv->irq_lock);

mutex_unlock(&dev_priv->dev->struct_mutex); } @@ -4606,19 +4603,18 @@ static void i965_irq_uninstall(struct drm_device * dev) I915_WRITE(IIR, I915_READ(IIR)); }

-static void intel_hpd_irq_reenable(struct work_struct *work) +static void intel_hpd_irq_reenable_work(struct work_struct *work) { struct drm_i915_private *dev_priv = container_of(work, typeof(*dev_priv), hotplug_reenable_work.work); struct drm_device *dev = dev_priv->dev; struct drm_mode_config *mode_config = &dev->mode_config; - unsigned long irqflags; int i;

intel_runtime_pm_get(dev_priv);

- spin_lock_irqsave(&dev_priv->irq_lock, irqflags); + spin_lock_irq(&dev_priv->irq_lock); for (i = (HPD_NONE + 1); i < HPD_NUM_PINS; i++) { struct drm_connector *connector;

@@ -4642,7 +4638,7 @@ static void intel_hpd_irq_reenable(struct work_struct *work) } if (dev_priv->display.hpd_irq_setup) dev_priv->display.hpd_irq_setup(dev); - spin_unlock_irqrestore(&dev_priv->irq_lock, irqflags); + spin_unlock_irq(&dev_priv->irq_lock);

intel_runtime_pm_put(dev_priv); } @@ -4668,7 +4664,7 @@ void intel_irq_init(struct drm_device *dev) i915_hangcheck_elapsed, (unsigned long) dev); INIT_DELAYED_WORK(&dev_priv->hotplug_reenable_work, - intel_hpd_irq_reenable); + intel_hpd_irq_reenable_work);

pm_qos_add_request(&dev_priv->pm_qos, PM_QOS_CPU_DMA_LATENCY, PM_QOS_DEFAULT_VALUE);

-- 1.9.3

Daniel Vetter

12:35 p.m.

New subject: [PATCH 06/11] drm/i915: Clarify irq_lock locking, interrupt install/uninstall

All the interrupt setup/teardown hooks are always run from plain process context. So again just the _irq variant is good enough.

Signed-off-by: Daniel Vetter daniel.vetter@ffwll.ch --- drivers/gpu/drm/i915/i915_irq.c | 42 ++++++++++++++++++----------------------- 1 file changed, 18 insertions(+), 24 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c index 4906823baa11..a829619aa111 100644 --- a/drivers/gpu/drm/i915/i915_irq.c +++ b/drivers/gpu/drm/i915/i915_irq.c @@ -3603,7 +3603,6 @@ static void gen5_gt_irq_postinstall(struct drm_device *dev)

static int ironlake_irq_postinstall(struct drm_device *dev) { - unsigned long irqflags; struct drm_i915_private *dev_priv = dev->dev_private; u32 display_mask, extra_mask;

@@ -3642,9 +3641,9 @@ static int ironlake_irq_postinstall(struct drm_device *dev) * spinlocking not required here for correctness since interrupt * setup is guaranteed to run in single-threaded context. But we * need it to make the assert_spin_locked happy. */ - spin_lock_irqsave(&dev_priv->irq_lock, irqflags); + spin_lock_irq(&dev_priv->irq_lock); ironlake_enable_display_irq(dev_priv, DE_PCU_EVENT); - spin_unlock_irqrestore(&dev_priv->irq_lock, irqflags); + spin_unlock_irq(&dev_priv->irq_lock); }

return 0; @@ -3740,7 +3739,6 @@ void valleyview_disable_display_irqs(struct drm_i915_private *dev_priv) static int valleyview_irq_postinstall(struct drm_device *dev) { struct drm_i915_private *dev_priv = dev->dev_private; - unsigned long irqflags;

dev_priv->irq_mask = ~0;

@@ -3754,10 +3752,10 @@ static int valleyview_irq_postinstall(struct drm_device *dev)

/* Interrupt setup is already guaranteed to be single-threaded, this is * just to make the assert_spin_locked check happy. */ - spin_lock_irqsave(&dev_priv->irq_lock, irqflags); + spin_lock_irq(&dev_priv->irq_lock); if (dev_priv->display_irqs_enabled) valleyview_display_irqs_install(dev_priv); - spin_unlock_irqrestore(&dev_priv->irq_lock, irqflags); + spin_unlock_irq(&dev_priv->irq_lock);

I915_WRITE(VLV_IIR, 0xffffffff); I915_WRITE(VLV_IIR, 0xffffffff); @@ -3848,7 +3846,6 @@ static int cherryview_irq_postinstall(struct drm_device *dev) I915_DISPLAY_PIPE_C_EVENT_INTERRUPT; u32 pipestat_enable = PLANE_FLIP_DONE_INT_STATUS_VLV | PIPE_CRC_DONE_INTERRUPT_STATUS; - unsigned long irqflags; int pipe;

/* @@ -3860,11 +3857,11 @@ static int cherryview_irq_postinstall(struct drm_device *dev) for_each_pipe(dev_priv, pipe) I915_WRITE(PIPESTAT(pipe), 0xffff);

- spin_lock_irqsave(&dev_priv->irq_lock, irqflags); + spin_lock_irq(&dev_priv->irq_lock); i915_enable_pipestat(dev_priv, PIPE_A, PIPE_GMBUS_INTERRUPT_STATUS); for_each_pipe(dev_priv, pipe) i915_enable_pipestat(dev_priv, pipe, pipestat_enable); - spin_unlock_irqrestore(&dev_priv->irq_lock, irqflags); + spin_unlock_irq(&dev_priv->irq_lock);

I915_WRITE(VLV_IIR, 0xffffffff); I915_WRITE(VLV_IMR, dev_priv->irq_mask); @@ -3891,7 +3888,6 @@ static void gen8_irq_uninstall(struct drm_device *dev) static void valleyview_irq_uninstall(struct drm_device *dev) { struct drm_i915_private *dev_priv = dev->dev_private; - unsigned long irqflags; int pipe;

if (!dev_priv) @@ -3906,10 +3902,12 @@ static void valleyview_irq_uninstall(struct drm_device *dev) I915_WRITE(PORT_HOTPLUG_EN, 0); I915_WRITE(PORT_HOTPLUG_STAT, I915_READ(PORT_HOTPLUG_STAT));

- spin_lock_irqsave(&dev_priv->irq_lock, irqflags); + /* Interrupt setup is already guaranteed to be single-threaded, this is + * just to make the assert_spin_locked check happy. */ + spin_lock_irq(&dev_priv->irq_lock); if (dev_priv->display_irqs_enabled) valleyview_display_irqs_uninstall(dev_priv); - spin_unlock_irqrestore(&dev_priv->irq_lock, irqflags); + spin_unlock_irq(&dev_priv->irq_lock);

dev_priv->irq_mask = 0;

@@ -3995,7 +3993,6 @@ static void i8xx_irq_preinstall(struct drm_device * dev) static int i8xx_irq_postinstall(struct drm_device *dev) { struct drm_i915_private *dev_priv = dev->dev_private; - unsigned long irqflags;

I915_WRITE16(EMR, ~(I915_ERROR_PAGE_TABLE | I915_ERROR_MEMORY_REFRESH)); @@ -4018,10 +4015,10 @@ static int i8xx_irq_postinstall(struct drm_device *dev)

/* Interrupt setup is already guaranteed to be single-threaded, this is * just to make the assert_spin_locked check happy. */ - spin_lock_irqsave(&dev_priv->irq_lock, irqflags); + spin_lock_irq(&dev_priv->irq_lock); i915_enable_pipestat(dev_priv, PIPE_A, PIPE_CRC_DONE_INTERRUPT_STATUS); i915_enable_pipestat(dev_priv, PIPE_B, PIPE_CRC_DONE_INTERRUPT_STATUS); - spin_unlock_irqrestore(&dev_priv->irq_lock, irqflags); + spin_unlock_irq(&dev_priv->irq_lock);

return 0; } @@ -4168,7 +4165,6 @@ static int i915_irq_postinstall(struct drm_device *dev) { struct drm_i915_private *dev_priv = dev->dev_private; u32 enable_mask; - unsigned long irqflags;

I915_WRITE(EMR, ~(I915_ERROR_PAGE_TABLE | I915_ERROR_MEMORY_REFRESH));

@@ -4206,10 +4202,10 @@ static int i915_irq_postinstall(struct drm_device *dev)

/* Interrupt setup is already guaranteed to be single-threaded, this is * just to make the assert_spin_locked check happy. */ - spin_lock_irqsave(&dev_priv->irq_lock, irqflags); + spin_lock_irq(&dev_priv->irq_lock); i915_enable_pipestat(dev_priv, PIPE_A, PIPE_CRC_DONE_INTERRUPT_STATUS); i915_enable_pipestat(dev_priv, PIPE_B, PIPE_CRC_DONE_INTERRUPT_STATUS); - spin_unlock_irqrestore(&dev_priv->irq_lock, irqflags); + spin_unlock_irq(&dev_priv->irq_lock);

return 0; } @@ -4391,7 +4387,6 @@ static int i965_irq_postinstall(struct drm_device *dev) struct drm_i915_private *dev_priv = dev->dev_private; u32 enable_mask; u32 error_mask; - unsigned long irqflags;

/* Unmask the interrupts that we always want on. */ dev_priv->irq_mask = ~(I915_ASLE_INTERRUPT | @@ -4412,11 +4407,11 @@ static int i965_irq_postinstall(struct drm_device *dev)

/* Interrupt setup is already guaranteed to be single-threaded, this is * just to make the assert_spin_locked check happy. */ - spin_lock_irqsave(&dev_priv->irq_lock, irqflags); + spin_lock_irq(&dev_priv->irq_lock); i915_enable_pipestat(dev_priv, PIPE_A, PIPE_GMBUS_INTERRUPT_STATUS); i915_enable_pipestat(dev_priv, PIPE_A, PIPE_CRC_DONE_INTERRUPT_STATUS); i915_enable_pipestat(dev_priv, PIPE_B, PIPE_CRC_DONE_INTERRUPT_STATUS); - spin_unlock_irqrestore(&dev_priv->irq_lock, irqflags); + spin_unlock_irq(&dev_priv->irq_lock);

/* * Enable some error detection, note the instruction error mask @@ -4756,7 +4751,6 @@ void intel_hpd_init(struct drm_device *dev) struct drm_i915_private *dev_priv = dev->dev_private; struct drm_mode_config *mode_config = &dev->mode_config; struct drm_connector *connector; - unsigned long irqflags; int i;

for (i = 1; i < HPD_NUM_PINS; i++) { @@ -4774,10 +4768,10 @@ void intel_hpd_init(struct drm_device *dev)

/* Interrupt setup is already guaranteed to be single-threaded, this is * just to make the assert_spin_locked checks happy. */ - spin_lock_irqsave(&dev_priv->irq_lock, irqflags); + spin_lock_irq(&dev_priv->irq_lock); if (dev_priv->display.hpd_irq_setup) dev_priv->display.hpd_irq_setup(dev); - spin_unlock_irqrestore(&dev_priv->irq_lock, irqflags); + spin_unlock_irq(&dev_priv->irq_lock); }

/* Disable interrupts so we can allow runtime PM. */

-- 1.9.3

Daniel Vetter

12:35 p.m.

New subject: [PATCH 07/11] drm/i915: Clarify irq_lock locking, irq handlers

irq handlers always run with interrupts locally disabled, so plain spinlocks is all we need. I've also reviewed again that they all follow the _irq_handler postfix convention.

Signed-off-by: Daniel Vetter daniel.vetter@ffwll.ch --- drivers/gpu/drm/i915/i915_irq.c | 15 ++++++--------- 1 file changed, 6 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c index a829619aa111..6a4f389ff2f5 100644 --- a/drivers/gpu/drm/i915/i915_irq.c +++ b/drivers/gpu/drm/i915/i915_irq.c @@ -4063,7 +4063,6 @@ static irqreturn_t i8xx_irq_handler(int irq, void *arg) struct drm_i915_private *dev_priv = dev->dev_private; u16 iir, new_iir; u32 pipe_stats[2]; - unsigned long irqflags; int pipe; u16 flip_mask = I915_DISPLAY_PLANE_A_FLIP_PENDING_INTERRUPT | @@ -4079,7 +4078,7 @@ static irqreturn_t i8xx_irq_handler(int irq, void *arg) * It doesn't set the bit in iir again, but it still produces * interrupts (for non-MSI). */ - spin_lock_irqsave(&dev_priv->irq_lock, irqflags); + spin_lock(&dev_priv->irq_lock); if (iir & I915_RENDER_COMMAND_PARSER_ERROR_INTERRUPT) i915_handle_error(dev, false, "Command parser error, iir 0x%08x", @@ -4095,7 +4094,7 @@ static irqreturn_t i8xx_irq_handler(int irq, void *arg) if (pipe_stats[pipe] & 0x8000ffff) I915_WRITE(reg, pipe_stats[pipe]); } - spin_unlock_irqrestore(&dev_priv->irq_lock, irqflags); + spin_unlock(&dev_priv->irq_lock);

I915_WRITE16(IIR, iir & ~flip_mask); new_iir = I915_READ16(IIR); /* Flush posted writes */ @@ -4249,7 +4248,6 @@ static irqreturn_t i915_irq_handler(int irq, void *arg) struct drm_device *dev = arg; struct drm_i915_private *dev_priv = dev->dev_private; u32 iir, new_iir, pipe_stats[I915_MAX_PIPES]; - unsigned long irqflags; u32 flip_mask = I915_DISPLAY_PLANE_A_FLIP_PENDING_INTERRUPT | I915_DISPLAY_PLANE_B_FLIP_PENDING_INTERRUPT; @@ -4265,7 +4263,7 @@ static irqreturn_t i915_irq_handler(int irq, void *arg) * It doesn't set the bit in iir again, but it still produces * interrupts (for non-MSI). */ - spin_lock_irqsave(&dev_priv->irq_lock, irqflags); + spin_lock(&dev_priv->irq_lock); if (iir & I915_RENDER_COMMAND_PARSER_ERROR_INTERRUPT) i915_handle_error(dev, false, "Command parser error, iir 0x%08x", @@ -4281,7 +4279,7 @@ static irqreturn_t i915_irq_handler(int irq, void *arg) irq_received = true; } } - spin_unlock_irqrestore(&dev_priv->irq_lock, irqflags); + spin_unlock(&dev_priv->irq_lock);

if (!irq_received) break; @@ -4476,7 +4474,6 @@ static irqreturn_t i965_irq_handler(int irq, void *arg) struct drm_i915_private *dev_priv = dev->dev_private; u32 iir, new_iir; u32 pipe_stats[I915_MAX_PIPES]; - unsigned long irqflags; int ret = IRQ_NONE, pipe; u32 flip_mask = I915_DISPLAY_PLANE_A_FLIP_PENDING_INTERRUPT | @@ -4493,7 +4490,7 @@ static irqreturn_t i965_irq_handler(int irq, void *arg) * It doesn't set the bit in iir again, but it still produces * interrupts (for non-MSI). */ - spin_lock_irqsave(&dev_priv->irq_lock, irqflags); + spin_lock(&dev_priv->irq_lock); if (iir & I915_RENDER_COMMAND_PARSER_ERROR_INTERRUPT) i915_handle_error(dev, false, "Command parser error, iir 0x%08x", @@ -4511,7 +4508,7 @@ static irqreturn_t i965_irq_handler(int irq, void *arg) irq_received = true; } } - spin_unlock_irqrestore(&dev_priv->irq_lock, irqflags); + spin_unlock(&dev_priv->irq_lock);

if (!irq_received) break;

-- 1.9.3

Chris Wilson

16 Sep 16 Sep

10:21 a.m.

New subject: [PATCH 07/11] drm/i915: Clarify irq_lock locking, irq handlers

On Mon, Sep 15, 2014 at 02:35:07PM +0200, Daniel Vetter wrote:

...

irq handlers always run with interrupts locally disabled, so plain spinlocks is all we need. I've also reviewed again that they all follow the _irq_handler postfix convention.

Hmm, we still have the full irq dance inside the reg read/write macros, which themselves should never be used from inside the irq handlers.

(Modulo the misgivings in execlists_irq_handler). -Chris

-- Chris Wilson, Intel Open Source Technology Centre

Daniel Vetter

10:28 a.m.

New subject: [PATCH 07/11] drm/i915: Clarify irq_lock locking, irq handlers

On Tue, Sep 16, 2014 at 11:21:52AM +0100, Chris Wilson wrote:

...

On Mon, Sep 15, 2014 at 02:35:07PM +0200, Daniel Vetter wrote:

...
irq handlers always run with interrupts locally disabled, so plain spinlocks is all we need. I've also reviewed again that they all follow the _irq_handler postfix convention.

Hmm, we still have the full irq dance inside the reg read/write macros, which themselves should never be used from inside the irq handlers.

(Modulo the misgivings in execlists_irq_handler).

Hm, we still have the ACTHEAD hack to read somewhat coherent-ish seqnos. Dunno whether open-coding that would make sense really, and whether it's really beneficial to force register writes to never acquire forcewake from irq context. Definitely material for different patches though. -Daniel

-- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch

Chris Wilson

10:53 a.m.

New subject: [PATCH 07/11] drm/i915: Clarify irq_lock locking, irq handlers

On Tue, Sep 16, 2014 at 12:28:18PM +0200, Daniel Vetter wrote:

...

On Tue, Sep 16, 2014 at 11:21:52AM +0100, Chris Wilson wrote:

...
On Mon, Sep 15, 2014 at 02:35:07PM +0200, Daniel Vetter wrote:

...
irq handlers always run with interrupts locally disabled, so plain spinlocks is all we need. I've also reviewed again that they all follow the _irq_handler postfix convention.

Hmm, we still have the full irq dance inside the reg read/write macros, which themselves should never be used from inside the irq handlers.

(Modulo the misgivings in execlists_irq_handler).

Hm, we still have the ACTHEAD hack to read somewhat coherent-ish seqnos. Dunno whether open-coding that would make sense really, and whether it's really beneficial to force register writes to never acquire forcewake from irq context. Definitely material for different patches though.

ATCHD inside the irq handler? I am pretty sure that even -nightly doesn't have the irq-barrier inside the irq handler. And yes, we do want to avoid having a massive delay whilst we apply the forcewake workarounds from inside irq handlers. -Chris

-- Chris Wilson, Intel Open Source Technology Centre

Daniel Vetter

15 Sep 15 Sep

12:35 p.m.

New subject: [PATCH 08/11] drm/i915: Clarify irq_lock locking, special cases

Grab bag for all the special cases: - i9xx_check_fifo_underruns is only called from crtc_enable hooks, i.e. process context. - i915_enable_asle_pipestat is only called from interrupt postinstall hooks. So again process context. - gen8_irq_power_well_post_enable is called from the runtime pm code, which again means process context. - The open-coded hpd_irq_setup loop in _thaw is also running in process context.

So for all of them the plain _irq variant is sufficient.

Signed-off-by: Daniel Vetter daniel.vetter@ffwll.ch --- drivers/gpu/drm/i915/i915_drv.c | 5 ++--- drivers/gpu/drm/i915/i915_irq.c | 16 ++++++---------- 2 files changed, 8 insertions(+), 13 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c index b8bd0080603e..8ce1b13ad97e 100644 --- a/drivers/gpu/drm/i915/i915_drv.c +++ b/drivers/gpu/drm/i915/i915_drv.c @@ -686,11 +686,10 @@ static int __i915_drm_thaw(struct drm_device *dev, bool restore_gtt_mappings) intel_modeset_init_hw(dev);

{ - unsigned long irqflags; - spin_lock_irqsave(&dev_priv->irq_lock, irqflags); + spin_lock_irq(&dev_priv->irq_lock); if (dev_priv->display.hpd_irq_setup) dev_priv->display.hpd_irq_setup(dev); - spin_unlock_irqrestore(&dev_priv->irq_lock, irqflags); + spin_unlock_irq(&dev_priv->irq_lock); }

intel_dp_mst_resume(dev); diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c index 6a4f389ff2f5..a08cdc62f841 100644 --- a/drivers/gpu/drm/i915/i915_irq.c +++ b/drivers/gpu/drm/i915/i915_irq.c @@ -310,9 +310,8 @@ void i9xx_check_fifo_underruns(struct drm_device *dev) { struct drm_i915_private *dev_priv = dev->dev_private; struct intel_crtc *crtc; - unsigned long flags;

- spin_lock_irqsave(&dev_priv->irq_lock, flags); + spin_lock_irq(&dev_priv->irq_lock);

for_each_intel_crtc(dev, crtc) { u32 reg = PIPESTAT(crtc->pipe); @@ -331,7 +330,7 @@ void i9xx_check_fifo_underruns(struct drm_device *dev) DRM_ERROR("pipe %c underrun\n", pipe_name(crtc->pipe)); }

- spin_unlock_irqrestore(&dev_priv->irq_lock, flags); + spin_unlock_irq(&dev_priv->irq_lock); }

static void i9xx_set_fifo_underrun_reporting(struct drm_device *dev, @@ -696,19 +695,18 @@ i915_disable_pipestat(struct drm_i915_private *dev_priv, enum pipe pipe, static void i915_enable_asle_pipestat(struct drm_device *dev) { struct drm_i915_private *dev_priv = dev->dev_private; - unsigned long irqflags;

if (!dev_priv->opregion.asle || !IS_MOBILE(dev)) return;

- spin_lock_irqsave(&dev_priv->irq_lock, irqflags); + spin_lock_irq(&dev_priv->irq_lock);

i915_enable_pipestat(dev_priv, PIPE_B, PIPE_LEGACY_BLC_EVENT_STATUS); if (INTEL_INFO(dev)->gen >= 4) i915_enable_pipestat(dev_priv, PIPE_A, PIPE_LEGACY_BLC_EVENT_STATUS);

- spin_unlock_irqrestore(&dev_priv->irq_lock, irqflags); + spin_unlock_irq(&dev_priv->irq_lock); }

/** @@ -3477,14 +3475,12 @@ static void gen8_irq_reset(struct drm_device *dev)

void gen8_irq_power_well_post_enable(struct drm_i915_private *dev_priv) { - unsigned long irqflags; - - spin_lock_irqsave(&dev_priv->irq_lock, irqflags); + spin_lock_irq(&dev_priv->irq_lock); GEN8_IRQ_INIT_NDX(DE_PIPE, PIPE_B, dev_priv->de_irq_mask[PIPE_B], ~dev_priv->de_irq_mask[PIPE_B]); GEN8_IRQ_INIT_NDX(DE_PIPE, PIPE_C, dev_priv->de_irq_mask[PIPE_C], ~dev_priv->de_irq_mask[PIPE_C]); - spin_unlock_irqrestore(&dev_priv->irq_lock, irqflags); + spin_unlock_irq(&dev_priv->irq_lock); }

static void cherryview_irq_preinstall(struct drm_device *dev)

-- 1.9.3

Daniel Vetter

12:35 p.m.

New subject: [PATCH 09/11] drm/i915: Convert backlight_lock to a mutex

Originally the irq safe spinlock was required because of asle interrupts. But since

commit 7bd688cd66db93f6430f6e2b3145ee5686daa315 Author: Jani Nikula jani.nikula@intel.com Date: Fri Nov 8 16:48:56 2013 +0200

drm/i915: handle backlight through chip specific functions

there's no need for this any more. So switch to the simpler mutex.

Cc: Jani Nikula jani.nikula@intel.com Signed-off-by: Daniel Vetter daniel.vetter@ffwll.ch --- drivers/gpu/drm/i915/i915_dma.c | 2 +- drivers/gpu/drm/i915/i915_drv.h | 2 +- drivers/gpu/drm/i915/intel_panel.c | 30 ++++++++++++------------------ 3 files changed, 14 insertions(+), 20 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c index 1403b01e8216..0bc1583114e7 100644 --- a/drivers/gpu/drm/i915/i915_dma.c +++ b/drivers/gpu/drm/i915/i915_dma.c @@ -1614,7 +1614,7 @@ int i915_driver_load(struct drm_device *dev, unsigned long flags)

spin_lock_init(&dev_priv->irq_lock); spin_lock_init(&dev_priv->gpu_error.lock); - spin_lock_init(&dev_priv->backlight_lock); + mutex_init(&dev_priv->backlight_lock); spin_lock_init(&dev_priv->uncore.lock); spin_lock_init(&dev_priv->mm.object_stat_lock); spin_lock_init(&dev_priv->mmio_flip_lock); diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index 17dfce0f4e68..07dafa2c2d8c 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -1528,7 +1528,7 @@ struct drm_i915_private { struct intel_overlay *overlay;

/* backlight registers and fields in struct intel_panel */ - spinlock_t backlight_lock; + struct mutex backlight_lock;

/* LVDS info */ bool no_aux_handshake; diff --git a/drivers/gpu/drm/i915/intel_panel.c b/drivers/gpu/drm/i915/intel_panel.c index 18784470a760..f17ada3742de 100644 --- a/drivers/gpu/drm/i915/intel_panel.c +++ b/drivers/gpu/drm/i915/intel_panel.c @@ -538,14 +538,13 @@ static u32 intel_panel_get_backlight(struct intel_connector *connector) struct drm_device *dev = connector->base.dev; struct drm_i915_private *dev_priv = dev->dev_private; u32 val; - unsigned long flags;