From work_struct to tasklet_struct

From work_struct to tasklet_struct

[Chris Wilson]

Just a small set to convert our irq bottom-halves to use a tasklet
instead of a work_struct. In theory, the tasklet has lower latency
from the interrupt (or lower upper bound at least), but in practice I
think it is clearer to be consistent in using tasklets for irq bh.
-Chris

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 1/5] drm/i915: Convert RPS irq worker into a tasklet

[Chris Wilson]

Using a tasklet for an irq bottom-half is the preferred form as it
should ensure minimal latency from the irq to execution of the tasklet.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h |  2 +-
 drivers/gpu/drm/i915/i915_irq.c | 13 +++++++------
 drivers/gpu/drm/i915/intel_pm.c |  2 +-
 3 files changed, 9 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 72f0b02a8372..b3b81c60bbb3 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1128,7 +1128,7 @@ struct intel_gen6_power_mgmt {
 	 * work, interrupts_enabled and pm_iir are protected by
 	 * dev_priv->irq_lock
 	 */
-	struct work_struct work;
+	struct tasklet_struct task;
 	bool interrupts_enabled;
 	u32 pm_iir;
 
diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index f0d941455bed..a4ca9471f3f4 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -385,7 +385,7 @@ void gen6_disable_rps_interrupts(struct drm_i915_private *dev_priv)
 	dev_priv->rps.interrupts_enabled = false;
 	spin_unlock_irq(&dev_priv->irq_lock);
 
-	cancel_work_sync(&dev_priv->rps.work);
+	tasklet_kill(&dev_priv->rps.task);
 
 	spin_lock_irq(&dev_priv->irq_lock);
 
@@ -1081,10 +1081,9 @@ static bool any_waiters(struct drm_i915_private *dev_priv)
 	return false;
 }
 
-static void gen6_pm_rps_work(struct work_struct *work)
+static void gen6_pm_rps_work(unsigned long data)
 {
-	struct drm_i915_private *dev_priv =
-		container_of(work, struct drm_i915_private, rps.work);
+	struct drm_i915_private *dev_priv = (struct drm_i915_private *)data;
 	bool client_boost;
 	int new_delay, adj, min, max;
 	u32 pm_iir;
@@ -1614,7 +1613,7 @@ static void gen6_rps_irq_handler(struct drm_i915_private *dev_priv, u32 pm_iir)
 		gen6_disable_pm_irq(dev_priv, pm_iir & dev_priv->pm_rps_events);
 		if (dev_priv->rps.interrupts_enabled) {
 			dev_priv->rps.pm_iir |= pm_iir & dev_priv->pm_rps_events;
-			queue_work(dev_priv->wq, &dev_priv->rps.work);
+			tasklet_schedule(&dev_priv->rps.task);
 		}
 		spin_unlock(&dev_priv->irq_lock);
 	}
@@ -4570,7 +4569,9 @@ void intel_irq_init(struct drm_i915_private *dev_priv)
 
 	intel_hpd_init_work(dev_priv);
 
-	INIT_WORK(&dev_priv->rps.work, gen6_pm_rps_work);
+	tasklet_init(&dev_priv->rps.task,
+		     gen6_pm_rps_work,
+		     (unsigned long)dev_priv);
 	INIT_WORK(&dev_priv->l3_parity.error_work, ivybridge_parity_work);
 
 	/* Let's track the enabled rps events */
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index adb64638f595..0e4f783b5da9 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -4748,7 +4748,7 @@ void gen6_rps_boost(struct drm_i915_private *dev_priv,
 		spin_lock_irq(&dev_priv->irq_lock);
 		if (dev_priv->rps.interrupts_enabled) {
 			dev_priv->rps.client_boost = true;
-			queue_work(dev_priv->wq, &dev_priv->rps.work);
+			tasklet_schedule(&dev_priv->rps.task);
 		}
 		spin_unlock_irq(&dev_priv->irq_lock);
 
-- 
2.8.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 2/5] drm/i915: Convert L3 parity irq worker into a tasklet

[Chris Wilson]

Using a tasklet for an irq bottom-half is the preferred form as it
should ensure minimal latency from the irq to execution of the tasklet.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h |  2 +-
 drivers/gpu/drm/i915/i915_irq.c | 16 ++++++++--------
 2 files changed, 9 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index b3b81c60bbb3..3a991bfce067 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1263,7 +1263,7 @@ struct i915_power_domains {
 #define MAX_L3_SLICES 2
 struct intel_l3_parity {
 	u32 *remap_info[MAX_L3_SLICES];
-	struct work_struct error_work;
+	struct tasklet_struct task;
 	int which_slice;
 };
 
diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index a4ca9471f3f4..56f0410cf8e8 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -1175,18 +1175,16 @@ out:
 
 
 /**
- * ivybridge_parity_work - Workqueue called when a parity error interrupt
- * occurred.
- * @work: workqueue struct
+ * ivybridge_parity_task - tasklet for the parity error interrupt
+ * @data: i915 device
  *
  * Doesn't actually do anything except notify userspace. As a consequence of
  * this event, userspace should try to remap the bad rows since statistically
  * it is likely the same row is more likely to go bad again.
  */
-static void ivybridge_parity_work(struct work_struct *work)
+static void ivybridge_parity_task(unsigned long data)
 {
-	struct drm_i915_private *dev_priv =
-		container_of(work, struct drm_i915_private, l3_parity.error_work);
+	struct drm_i915_private *dev_priv = (struct drm_i915_private *)data;
 	u32 error_status, row, bank, subbank;
 	char *parity_event[6];
 	uint32_t misccpctl;
@@ -1272,7 +1270,7 @@ static void ivybridge_parity_error_irq_handler(struct drm_i915_private *dev_priv
 	if (iir & GT_RENDER_L3_PARITY_ERROR_INTERRUPT)
 		dev_priv->l3_parity.which_slice |= 1 << 0;
 
-	queue_work(dev_priv->wq, &dev_priv->l3_parity.error_work);
+	tasklet_schedule(&dev_priv->l3_parity.task);
 }
 
 static void ilk_gt_irq_handler(struct drm_i915_private *dev_priv,
@@ -4572,7 +4570,9 @@ void intel_irq_init(struct drm_i915_private *dev_priv)
 	tasklet_init(&dev_priv->rps.task,
 		     gen6_pm_rps_work,
 		     (unsigned long)dev_priv);
-	INIT_WORK(&dev_priv->l3_parity.error_work, ivybridge_parity_work);
+	tasklet_init(&dev_priv->l3_parity.task,
+		     ivybridge_parity_task,
+		     (unsigned long)dev_priv);
 
 	/* Let's track the enabled rps events */
 	if (IS_VALLEYVIEW(dev_priv))
-- 
2.8.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 3/5] drm/i915: Convert OpRegion ASLE irq worker into a tasklet

[Chris Wilson]

Using a tasklet for an irq bottom-half is the preferred form as it
should ensure minimal latency from the irq to execution of the tasklet.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h       |  2 +-
 drivers/gpu/drm/i915/intel_opregion.c | 19 ++++++-------------
 2 files changed, 7 insertions(+), 14 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 3a991bfce067..ce7f30cecb1f 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -438,7 +438,7 @@ struct intel_opregion {
 	const void *vbt;
 	u32 vbt_size;
 	u32 *lid_state;
-	struct work_struct asle_work;
+	struct tasklet_struct asle_task;
 };
 #define OPREGION_SIZE            (8*1024)
 
diff --git a/drivers/gpu/drm/i915/intel_opregion.c b/drivers/gpu/drm/i915/intel_opregion.c
index 8347fd8af8e4..727ca017f5b0 100644
--- a/drivers/gpu/drm/i915/intel_opregion.c
+++ b/drivers/gpu/drm/i915/intel_opregion.c
@@ -521,22 +521,15 @@ static u32 asle_isct_state(struct drm_device *dev)
 	return ASLC_ISCT_STATE_FAILED;
 }
 
-static void asle_work(struct work_struct *work)
+static void asle_task(unsigned long data)
 {
-	struct intel_opregion *opregion =
-		container_of(work, struct intel_opregion, asle_work);
-	struct drm_i915_private *dev_priv =
-		container_of(opregion, struct drm_i915_private, opregion);
+	struct drm_i915_private *dev_priv = (struct drm_i915_private *)data;
 	struct drm_device *dev = dev_priv->dev;
 	struct opregion_asle *asle = dev_priv->opregion.asle;
 	u32 aslc_stat = 0;
 	u32 aslc_req;
 
-	if (!asle)
-		return;
-
 	aslc_req = asle->aslc;
-
 	if (!(aslc_req & ASLC_REQ_MSK)) {
 		DRM_DEBUG_DRIVER("No request on ASLC interrupt 0x%08x\n",
 				 aslc_req);
@@ -577,7 +570,7 @@ static void asle_work(struct work_struct *work)
 void intel_opregion_asle_intr(struct drm_i915_private *dev_priv)
 {
 	if (dev_priv->opregion.asle)
-		schedule_work(&dev_priv->opregion.asle_work);
+		tasklet_schedule(&dev_priv->opregion.asle_task);
 }
 
 #define ACPI_EV_DISPLAY_SWITCH (1<<0)
@@ -814,11 +807,11 @@ void intel_opregion_fini(struct drm_device *dev)
 	if (!opregion->header)
 		return;
 
+	tasklet_kill(&dev_priv->opregion.asle_task);
+
 	if (opregion->asle)
 		opregion->asle->ardy = ASLE_ARDY_NOT_READY;
 
-	cancel_work_sync(&dev_priv->opregion.asle_work);
-
 	if (opregion->acpi) {
 		opregion->acpi->drdy = 0;
 
@@ -938,7 +931,7 @@ int intel_opregion_setup(struct drm_device *dev)
 		return -ENOTSUPP;
 	}
 
-	INIT_WORK(&opregion->asle_work, asle_work);
+	tasklet_init(&opregion->asle_task, asle_task, (unsigned long)dev_priv);
 
 	base = memremap(asls, OPREGION_SIZE, MEMREMAP_WB);
 	if (!base)
-- 
2.8.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 4/5] drm/i915: Convert display-port irq worker into a tasklet

[Chris Wilson]

Using a tasklet for an irq bottom-half is the preferred form as it
should ensure minimal latency from the irq to execution of the tasklet.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_dma.c      |  9 +--------
 drivers/gpu/drm/i915/i915_drv.h      | 11 +----------
 drivers/gpu/drm/i915/intel_hotplug.c | 17 +++++++++--------
 3 files changed, 11 insertions(+), 26 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
index a8c79f6512a4..466d2cd4f656 100644
--- a/drivers/gpu/drm/i915/i915_dma.c
+++ b/drivers/gpu/drm/i915/i915_dma.c
@@ -1022,19 +1022,13 @@ static int i915_workqueues_init(struct drm_i915_private *dev_priv)
 	if (dev_priv->wq == NULL)
 		goto out_err;
 
-	dev_priv->hotplug.dp_wq = alloc_ordered_workqueue("i915-dp", 0);
-	if (dev_priv->hotplug.dp_wq == NULL)
-		goto out_free_wq;
-
 	dev_priv->gpu_error.hangcheck_wq =
 		alloc_ordered_workqueue("i915-hangcheck", 0);
 	if (dev_priv->gpu_error.hangcheck_wq == NULL)
-		goto out_free_dp_wq;
+		goto out_free_wq;
 
 	return 0;
 
-out_free_dp_wq:
-	destroy_workqueue(dev_priv->hotplug.dp_wq);
 out_free_wq:
 	destroy_workqueue(dev_priv->wq);
 out_err:
@@ -1046,7 +1040,6 @@ out_err:
 static void i915_workqueues_cleanup(struct drm_i915_private *dev_priv)
 {
 	destroy_workqueue(dev_priv->gpu_error.hangcheck_wq);
-	destroy_workqueue(dev_priv->hotplug.dp_wq);
 	destroy_workqueue(dev_priv->wq);
 }
 
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index ce7f30cecb1f..fdb19e2610eb 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -279,16 +279,7 @@ struct i915_hotplug {
 	struct intel_digital_port *irq_port[I915_MAX_PORTS];
 	u32 long_port_mask;
 	u32 short_port_mask;
-	struct work_struct dig_port_work;
-
-	/*
-	 * if we get a HPD irq from DP and a HPD irq from non-DP
-	 * the non-DP HPD could block the workqueue on a mode config
-	 * mutex getting, that userspace may have taken. However
-	 * userspace is waiting on the DP workqueue to run which is
-	 * blocked behind the non-DP one.
-	 */
-	struct workqueue_struct *dp_wq;
+	struct tasklet_struct dig_port_task;
 };
 
 #define I915_GEM_GPU_DOMAINS \
diff --git a/drivers/gpu/drm/i915/intel_hotplug.c b/drivers/gpu/drm/i915/intel_hotplug.c
index 38eeca7a6e72..3c74c2b944cf 100644
--- a/drivers/gpu/drm/i915/intel_hotplug.c
+++ b/drivers/gpu/drm/i915/intel_hotplug.c
@@ -48,7 +48,7 @@
  * further processing to appropriate bottom halves (Display Port specific and
  * regular hotplug).
  *
- * The Display Port work function i915_digport_work_func() calls into
+ * The Display Port work function i915_digport_task() calls into
  * intel_dp_hpd_pulse() via hooks, which handles DP short pulses and DP MST long
  * pulses, with failures and non-MST long pulses triggering regular hotplug
  * processing on the connector.
@@ -69,7 +69,7 @@
  *
  * Current implementation expects that hotplug interrupt storm will not be
  * seen when display port sink is connected, hence on platforms whose DP
- * callback is handled by i915_digport_work_func reenabling of hpd is not
+ * callback is handled by i915_digport_task reenabling of hpd is not
  * performed (it was never expected to be disabled in the first place ;) )
  * this is specific to DP sinks handled by this routine and any other display
  * such as HDMI or DVI enabled on the same port will have proper logic since
@@ -247,10 +247,9 @@ static bool intel_hpd_irq_event(struct drm_device *dev,
 	return true;
 }
 
-static void i915_digport_work_func(struct work_struct *work)
+static void i915_digport_task(unsigned long data)
 {
-	struct drm_i915_private *dev_priv =
-		container_of(work, struct drm_i915_private, hotplug.dig_port_work);
+	struct drm_i915_private *dev_priv = (struct drm_i915_private *)data;
 	u32 long_port_mask, short_port_mask;
 	struct intel_digital_port *intel_dig_port;
 	int i;
@@ -436,7 +435,7 @@ void intel_hpd_irq_handler(struct drm_i915_private *dev_priv,
 	 * deadlock.
 	 */
 	if (queue_dig)
-		queue_work(dev_priv->hotplug.dp_wq, &dev_priv->hotplug.dig_port_work);
+		tasklet_schedule(&dev_priv->hotplug.dig_port_task);
 	if (queue_hp)
 		schedule_work(&dev_priv->hotplug.hotplug_work);
 }
@@ -491,7 +490,9 @@ void intel_hpd_init(struct drm_i915_private *dev_priv)
 void intel_hpd_init_work(struct drm_i915_private *dev_priv)
 {
 	INIT_WORK(&dev_priv->hotplug.hotplug_work, i915_hotplug_work_func);
-	INIT_WORK(&dev_priv->hotplug.dig_port_work, i915_digport_work_func);
+	tasklet_init(&dev_priv->hotplug.dig_port_task,
+		     i915_digport_task,
+		     (unsigned long)dev_priv);
 	INIT_DELAYED_WORK(&dev_priv->hotplug.reenable_work,
 			  intel_hpd_irq_storm_reenable_work);
 }
@@ -506,7 +507,7 @@ void intel_hpd_cancel_work(struct drm_i915_private *dev_priv)
 
 	spin_unlock_irq(&dev_priv->irq_lock);
 
-	cancel_work_sync(&dev_priv->hotplug.dig_port_work);
+	tasklet_kill(&dev_priv->hotplug.dig_port_task);
 	cancel_work_sync(&dev_priv->hotplug.hotplug_work);
 	cancel_delayed_work_sync(&dev_priv->hotplug.reenable_work);
 }
-- 
2.8.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 5/5] drm/i915: Convert hotplug irq worker into a tasklet

[Chris Wilson]

Using a tasklet for an irq bottom-half is the preferred form as it
should ensure minimal latency from the irq to execution of the tasklet.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h      |  2 +-
 drivers/gpu/drm/i915/intel_hotplug.c | 16 +++++++++-------
 2 files changed, 10 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index fdb19e2610eb..5e541b4dbfb9 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -262,7 +262,7 @@ enum hpd_pin {
 	for ((__pin) = (HPD_NONE + 1); (__pin) < HPD_NUM_PINS; (__pin)++)
 
 struct i915_hotplug {
-	struct work_struct hotplug_work;
+	struct tasklet_struct hpd_task;
 
 	struct {
 		unsigned long last_jiffies;
diff --git a/drivers/gpu/drm/i915/intel_hotplug.c b/drivers/gpu/drm/i915/intel_hotplug.c
index 3c74c2b944cf..1ab6fd298f51 100644
--- a/drivers/gpu/drm/i915/intel_hotplug.c
+++ b/drivers/gpu/drm/i915/intel_hotplug.c
@@ -290,17 +290,17 @@ static void i915_digport_task(unsigned long data)
 		spin_lock_irq(&dev_priv->irq_lock);
 		dev_priv->hotplug.event_bits |= old_bits;
 		spin_unlock_irq(&dev_priv->irq_lock);
-		schedule_work(&dev_priv->hotplug.hotplug_work);
+
+		tasklet_schedule(&dev_priv->hotplug.hpd_task);
 	}
 }
 
 /*
  * Handle hotplug events outside the interrupt handler proper.
  */
-static void i915_hotplug_work_func(struct work_struct *work)
+static void i915_hotplug_task(unsigned long data)
 {
-	struct drm_i915_private *dev_priv =
-		container_of(work, struct drm_i915_private, hotplug.hotplug_work);
+	struct drm_i915_private *dev_priv = (struct drm_i915_private *)data;
 	struct drm_device *dev = dev_priv->dev;
 	struct drm_mode_config *mode_config = &dev->mode_config;
 	struct intel_connector *intel_connector;
@@ -437,7 +437,7 @@ void intel_hpd_irq_handler(struct drm_i915_private *dev_priv,
 	if (queue_dig)
 		tasklet_schedule(&dev_priv->hotplug.dig_port_task);
 	if (queue_hp)
-		schedule_work(&dev_priv->hotplug.hotplug_work);
+		tasklet_schedule(&dev_priv->hotplug.hpd_task);
 }
 
 /**
@@ -489,7 +489,9 @@ void intel_hpd_init(struct drm_i915_private *dev_priv)
 
 void intel_hpd_init_work(struct drm_i915_private *dev_priv)
 {
-	INIT_WORK(&dev_priv->hotplug.hotplug_work, i915_hotplug_work_func);
+	tasklet_init(&dev_priv->hotplug.hpd_task,
+		     i915_hotplug_task,
+		     (unsigned long)dev_priv);
 	tasklet_init(&dev_priv->hotplug.dig_port_task,
 		     i915_digport_task,
 		     (unsigned long)dev_priv);
@@ -508,6 +510,6 @@ void intel_hpd_cancel_work(struct drm_i915_private *dev_priv)
 	spin_unlock_irq(&dev_priv->irq_lock);
 
 	tasklet_kill(&dev_priv->hotplug.dig_port_task);
-	cancel_work_sync(&dev_priv->hotplug.hotplug_work);
+	tasklet_kill(&dev_priv->hotplug.hpd_task);
 	cancel_delayed_work_sync(&dev_priv->hotplug.reenable_work);
 }
-- 
2.8.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: From work_struct to tasklet_struct

[Chris Wilson]

On Thu, May 19, 2016 at 08:48:29AM +0100, Chris Wilson wrote:
> Just a small set to convert our irq bottom-halves to use a tasklet
> instead of a work_struct. In theory, the tasklet has lower latency
> from the interrupt (or lower upper bound at least), but in practice I
> think it is clearer to be consistent in using tasklets for irq bh.

Forget this since as Ville pointed out, tasklets are softirq and cannot
sleep and we have far too many sleeps for slow hw.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✗ Ro.CI.BAT: failure for series starting with [1/5] drm/i915: Convert RPS irq worker into a tasklet

[Patchwork]

== Series Details ==

Series: series starting with [1/5] drm/i915: Convert RPS irq worker into a tasklet
URL   : https://patchwork.freedesktop.org/series/7386/
State : failure

== Summary ==

Series 7386v1 Series without cover letter
http://patchwork.freedesktop.org/api/1.0/series/7386/revisions/1/mbox

Test drv_module_reload_basic:
                pass       -> DMESG-WARN (ro-hsw-i7-4770r)
Test gem_cpu_reloc:
        Subgroup basic:
                pass       -> DMESG-WARN (ro-ivb-i7-3770)
                pass       -> DMESG-WARN (ro-ivb2-i7-3770)
                pass       -> DMESG-WARN (fi-snb-i7-2600)
Test gem_ctx_basic:
                pass       -> DMESG-WARN (ro-skl-i7-6700hq)
Test gem_ctx_create:
        Subgroup basic:
                pass       -> DMESG-WARN (fi-bdw-i7-5557u)
        Subgroup basic-files:
                pass       -> INCOMPLETE (ro-skl-i7-6700hq)
                pass       -> DMESG-WARN (fi-skl-i7-6700k)
Test gem_ctx_switch:
        Subgroup basic-default:
                pass       -> DMESG-WARN (ro-bdw-i7-5557U)
                pass       -> DMESG-WARN (ro-bdw-i5-5250u)
Test gem_exec_flush:
        Subgroup basic-wb-set-default:
                pass       -> DMESG-WARN (ro-bdw-i5-5250u)
Test gem_exec_parallel:
        Subgroup basic:
                pass       -> DMESG-WARN (ro-bdw-i5-5250u)
Test gem_exec_suspend:
        Subgroup basic-s3:
                pass       -> INCOMPLETE (ro-ivb-i7-3770)
                pass       -> DMESG-WARN (ro-bdw-i5-5250u)
Test gem_exec_whisper:
        Subgroup basic:
                pass       -> DMESG-WARN (ro-bdw-i5-5250u)
Test gem_mmap:
        Subgroup basic:
                pass       -> INCOMPLETE (ro-hsw-i7-4770r)
        Subgroup basic-small-bo:
                pass       -> INCOMPLETE (ro-bdw-i5-5250u)
Test gem_render_tiled_blits:
        Subgroup basic:
                pass       -> INCOMPLETE (fi-snb-i7-2600)
Test gem_storedw_loop:
        Subgroup basic-default:
                pass       -> INCOMPLETE (fi-hsw-i7-4770k)
                pass       -> DMESG-WARN (fi-bdw-i7-5557u)
                pass       -> INCOMPLETE (ro-snb-i7-2620M)
                pass       -> INCOMPLETE (ro-hsw-i3-4010u)
        Subgroup basic-render:
                pass       -> INCOMPLETE (fi-bdw-i7-5557u)
                pass       -> INCOMPLETE (fi-skl-i7-6700k)
Test gem_tiled_pread_basic:
                pass       -> INCOMPLETE (ro-bdw-i7-5557U)
Test kms_flip:
        Subgroup basic-flip-vs-modeset:
                pass       -> INCOMPLETE (ro-ivb2-i7-3770)

fi-bdw-i7-5557u  total:12   pass:8    dwarn:2   dfail:0   fail:0   skip:1  
fi-bsw-n3050     total:9    pass:6    dwarn:1   dfail:0   fail:0   skip:1  
fi-hsw-i7-4770k  total:11   pass:10   dwarn:0   dfail:0   fail:0   skip:0  
fi-hsw-i7-4770r  total:108  pass:84   dwarn:1   dfail:0   fail:0   skip:22 
fi-skl-i7-6700k  total:12   pass:9    dwarn:1   dfail:0   fail:0   skip:1  
fi-snb-i7-2600   total:10   pass:7    dwarn:1   dfail:0   fail:0   skip:1  
ro-bdw-i5-5250u  total:121  pass:105  dwarn:5   dfail:0   fail:0   skip:10 
ro-bdw-i7-5557U  total:28   pass:25   dwarn:1   dfail:0   fail:0   skip:1  
ro-bsw-n3050     total:10   pass:7    dwarn:1   dfail:0   fail:0   skip:1  
ro-hsw-i3-4010u  total:11   pass:10   dwarn:0   dfail:0   fail:0   skip:0  
ro-hsw-i7-4770r  total:120  pass:96   dwarn:1   dfail:0   fail:0   skip:22 
ro-ilk-i7-620lm  total:200  pass:138  dwarn:3   dfail:0   fail:1   skip:57 
ro-ilk1-i5-650   total:214  pass:151  dwarn:0   dfail:0   fail:2   skip:61 
ro-ivb-i7-3770   total:111  pass:79   dwarn:1   dfail:0   fail:0   skip:30 
ro-ivb2-i7-3770  total:183  pass:153  dwarn:1   dfail:0   fail:0   skip:28 
ro-skl-i7-6700hq total:7    pass:4    dwarn:1   dfail:0   fail:0   skip:1  
ro-snb-i7-2620M  total:11   pass:10   dwarn:0   dfail:0   fail:0   skip:0  
fi-byt-n2820 failed to connect after reboot
ro-bdw-i7-5600u failed to connect after reboot
ro-byt-n2820 failed to connect after reboot

Results at /archive/results/CI_IGT_test/RO_Patchwork_935/

a2499a0 drm-intel-nightly: 2016y-05m-18d-17h-17m-55s UTC integration manifest
205504b drm/i915: Convert hotplug irq worker into a tasklet
9cde61c drm/i915: Convert display-port irq worker into a tasklet
a35a5e5 drm/i915: Convert OpRegion ASLE irq worker into a tasklet
76cdef4 drm/i915: Convert L3 parity irq worker into a tasklet
0bc8257 drm/i915: Convert RPS irq worker into a tasklet

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 3/5] drm/i915: Convert OpRegion ASLE irq worker into a tasklet

[Jani Nikula]

On Thu, 19 May 2016, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> Using a tasklet for an irq bottom-half is the preferred form as it
> should ensure minimal latency from the irq to execution of the tasklet.
>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.h       |  2 +-
>  drivers/gpu/drm/i915/intel_opregion.c | 19 ++++++-------------
>  2 files changed, 7 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 3a991bfce067..ce7f30cecb1f 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -438,7 +438,7 @@ struct intel_opregion {
>  	const void *vbt;
>  	u32 vbt_size;
>  	u32 *lid_state;
> -	struct work_struct asle_work;
> +	struct tasklet_struct asle_task;
>  };
>  #define OPREGION_SIZE            (8*1024)
>  
> diff --git a/drivers/gpu/drm/i915/intel_opregion.c b/drivers/gpu/drm/i915/intel_opregion.c
> index 8347fd8af8e4..727ca017f5b0 100644
> --- a/drivers/gpu/drm/i915/intel_opregion.c
> +++ b/drivers/gpu/drm/i915/intel_opregion.c
> @@ -521,22 +521,15 @@ static u32 asle_isct_state(struct drm_device *dev)
>  	return ASLC_ISCT_STATE_FAILED;
>  }
>  
> -static void asle_work(struct work_struct *work)
> +static void asle_task(unsigned long data)
>  {
> -	struct intel_opregion *opregion =
> -		container_of(work, struct intel_opregion, asle_work);
> -	struct drm_i915_private *dev_priv =
> -		container_of(opregion, struct drm_i915_private, opregion);
> +	struct drm_i915_private *dev_priv = (struct drm_i915_private *)data;
>  	struct drm_device *dev = dev_priv->dev;
>  	struct opregion_asle *asle = dev_priv->opregion.asle;
>  	u32 aslc_stat = 0;
>  	u32 aslc_req;
>  
> -	if (!asle)
> -		return;
> -

Mmkay.

>  	aslc_req = asle->aslc;
> -
>  	if (!(aslc_req & ASLC_REQ_MSK)) {
>  		DRM_DEBUG_DRIVER("No request on ASLC interrupt 0x%08x\n",
>  				 aslc_req);
> @@ -577,7 +570,7 @@ static void asle_work(struct work_struct *work)
>  void intel_opregion_asle_intr(struct drm_i915_private *dev_priv)
>  {
>  	if (dev_priv->opregion.asle)
> -		schedule_work(&dev_priv->opregion.asle_work);
> +		tasklet_schedule(&dev_priv->opregion.asle_task);
>  }
>  
>  #define ACPI_EV_DISPLAY_SWITCH (1<<0)
> @@ -814,11 +807,11 @@ void intel_opregion_fini(struct drm_device *dev)
>  	if (!opregion->header)
>  		return;
>  
> +	tasklet_kill(&dev_priv->opregion.asle_task);
> +

So what if you got a new asle interrupt right here?

>  	if (opregion->asle)
>  		opregion->asle->ardy = ASLE_ARDY_NOT_READY;

This is supposed to signal we're not ready to handle said interrupts
anymore. Not that we should rely on it either.

It wasn't pretty before, but I think this patch widens the window for a
race. If you kept the *other* code as it were, and just changed the work
to tasklets, I'd be willing to look in the other direction...

BR,
Jani.

>  
> -	cancel_work_sync(&dev_priv->opregion.asle_work);
> -
>  	if (opregion->acpi) {
>  		opregion->acpi->drdy = 0;
>  
> @@ -938,7 +931,7 @@ int intel_opregion_setup(struct drm_device *dev)
>  		return -ENOTSUPP;
>  	}
>  
> -	INIT_WORK(&opregion->asle_work, asle_work);
> +	tasklet_init(&opregion->asle_task, asle_task, (unsigned long)dev_priv);
>  
>  	base = memremap(asls, OPREGION_SIZE, MEMREMAP_WB);
>  	if (!base)

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 3/5] drm/i915: Convert OpRegion ASLE irq worker into a tasklet

[Chris Wilson]

On Mon, May 23, 2016 at 12:12:30PM +0300, Jani Nikula wrote:
> >  #define ACPI_EV_DISPLAY_SWITCH (1<<0)
> > @@ -814,11 +807,11 @@ void intel_opregion_fini(struct drm_device *dev)
> >  	if (!opregion->header)
> >  		return;
> >  
> > +	tasklet_kill(&dev_priv->opregion.asle_task);
> > +
> 
> So what if you got a new asle interrupt right here?

Before we call fini, we should have de-installed the irq and done
synchronize_irq, so we only have to worry about the residual task.
(At least that is what I expect!)

> >  	if (opregion->asle)
> >  		opregion->asle->ardy = ASLE_ARDY_NOT_READY;
> 
> This is supposed to signal we're not ready to handle said interrupts
> anymore. Not that we should rely on it either.
> 
> It wasn't pretty before, but I think this patch widens the window for a
> race. If you kept the *other* code as it were, and just changed the work
> to tasklets, I'd be willing to look in the other direction...

Considering the recent discussion about the negatives of
tasklets/ksoftirqd, I think I was being too cavalier in this conversion,
and we should only think about using tasklet where the post-interrupt
latency is critical.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 3/5] drm/i915: Convert OpRegion ASLE irq worker into a tasklet

[Jani Nikula]

On Mon, 23 May 2016, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> On Mon, May 23, 2016 at 12:12:30PM +0300, Jani Nikula wrote:
>> >  #define ACPI_EV_DISPLAY_SWITCH (1<<0)
>> > @@ -814,11 +807,11 @@ void intel_opregion_fini(struct drm_device *dev)
>> >  	if (!opregion->header)
>> >  		return;
>> >  
>> > +	tasklet_kill(&dev_priv->opregion.asle_task);
>> > +
>> 
>> So what if you got a new asle interrupt right here?
>
> Before we call fini, we should have de-installed the irq and done
> synchronize_irq, so we only have to worry about the residual task.
> (At least that is what I expect!)

I'd expect that too, but looks like

i915_driver_unload -> i915_driver_unregister -> intel_opregion_fini

happens *before*, not after

i915_driver_unload -> intel_modeset_cleanup -> intel_irq_uninstall

J.


>
>> >  	if (opregion->asle)
>> >  		opregion->asle->ardy = ASLE_ARDY_NOT_READY;
>> 
>> This is supposed to signal we're not ready to handle said interrupts
>> anymore. Not that we should rely on it either.
>> 
>> It wasn't pretty before, but I think this patch widens the window for a
>> race. If you kept the *other* code as it were, and just changed the work
>> to tasklets, I'd be willing to look in the other direction...
>
> Considering the recent discussion about the negatives of
> tasklets/ksoftirqd, I think I was being too cavalier in this conversion,
> and we should only think about using tasklet where the post-interrupt
> latency is critical.
> -Chris

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx