On Thu, 19 May 2016, Chris Wilson <chris@xxxxxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxxxxx>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxx>
> Cc: Joonas Lahtinen <joonas.lahtinen@xxxxxxxxxxxxxxx>
> ---
> 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@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
