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

[Jani Nikula <jani.nikula@linux.intel.com>]

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