Re: [RFC 07/12] drm/i915/config: Get workaround information from configuration.

[Daniel Vetter <daniel@ffwll.ch>]

On Thu, Feb 12, 2015 at 03:41:33PM -0800, Bob Paauwe wrote:
> Add ability to parse a list of workarounds from the ACPI table.
> Initially, this expects all workarounds listed to be valid and
> they replace the hard coded list initialized in init_workarounds_ring().
> 
> The main benefit of this is the ability to add/remove workarounds
> at runtime.  It may also be useful to "adjust" the workaround
> list for a new GPU prior to fixed support being available in the
> driver.
> 
> Signed-off-by: Bob Paauwe <bob.j.paauwe@intel.com>

This one here freaks me out. We /should/ be fast enough with rolling out
workarounds through backports to stable kernels and product trees that
this shouldn't ever matter. And for a lot of workarounds this is too
limited since we also need to emit some of them to the ring on each engine
init or sometimes even context load.

As-is it looks a lot more like a backdoor to the driver to enable some
features no one wants to talk about in public ;-)
-Daniel

> ---
>  drivers/gpu/drm/i915/i915_drv.h     |   1 +
>  drivers/gpu/drm/i915/intel_config.c | 101 +++++++++++++++++++++++++++++++++++-
>  drivers/gpu/drm/i915/intel_drv.h    |   4 +-
>  3 files changed, 104 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 165091c..1580702 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -1656,6 +1656,7 @@ struct intel_config_info {
>  	struct list_head crtc_list;
>  	struct list_head connector_list;
>  	struct list_head plane_list;
> +	struct list_head wa_list;
>  };
>  
>  
> diff --git a/drivers/gpu/drm/i915/intel_config.c b/drivers/gpu/drm/i915/intel_config.c
> index fb495ed..ab14928 100644
> --- a/drivers/gpu/drm/i915/intel_config.c
> +++ b/drivers/gpu/drm/i915/intel_config.c
> @@ -214,6 +214,7 @@ void intel_config_init(struct drm_device *dev)
>  	INIT_LIST_HEAD(&info->crtc_list);
>  	INIT_LIST_HEAD(&info->connector_list);
>  	INIT_LIST_HEAD(&info->plane_list);
> +	INIT_LIST_HEAD(&info->wa_list);
>  
>  	handle = ACPI_HANDLE(dev->dev);
>  	if (!handle) {
> @@ -239,6 +240,7 @@ void intel_config_init(struct drm_device *dev)
>  #define i915_COMPONENT_CRTC "CRTC"
>  #define i915_COMPONENT_CONNECTOR "CNCT"
>  #define i915_COMPONENT_PLANE "PLNS"
> +#define i915_COMPONENT_WORKAROUND "WRKS"
>  
>  	/* Build lists */
>  	list_for_each_entry(component, &info->base.adev->children, node) {
> @@ -262,6 +264,9 @@ void intel_config_init(struct drm_device *dev)
>  				if (!alloc_new_node(cl, &info->crtc_list))
>  					goto bail;
>  			}
> +		} else if (strcmp(cname, i915_COMPONENT_WORKAROUND) == 0) {
> +			if (!alloc_new_node(cl, &info->wa_list))
> +				goto bail;
>  		}
>  	}
>  
> @@ -277,6 +282,8 @@ bail:
>  		kfree(new_node);
>  	list_for_each_entry_safe(new_node, tmp, &info->plane_list, node)
>  		kfree(new_node);
> +	list_for_each_entry_safe(new_node, tmp, &info->wa_list, node)
> +		kfree(new_node);
>  
>  	kfree(info);
>  	dev_priv->config_info = NULL;
> @@ -309,6 +316,8 @@ void intel_config_shutdown(struct drm_device *dev)
>  		kfree(n);
>  	list_for_each_entry_safe(n, tmp, &info->plane_list, node)
>  		kfree(n);
> +	list_for_each_entry_safe(n, tmp, &info->wa_list, node)
> +		kfree(n);
>  
>  	/* Unload any dynamically loaded ACPI property table */
>  	handle = ACPI_HANDLE(dev->dev);
> @@ -384,6 +393,8 @@ static struct list_head *cfg_type_to_list(struct intel_config_info *info,
>  		return &info->connector_list;
>  	case CFG_PLANE:
>  		return &info->plane_list;
> +	case CFG_WORKAROUND:
> +		return &info->wa_list;
>  	}
>  	return NULL;
>  }
> @@ -404,7 +415,6 @@ bool intel_config_get_integer(struct drm_i915_private *dev_priv,
>  	struct intel_config_info *info = dev_priv->config_info;
>  	struct intel_config_node *n;
>  	struct list_head *list;
> -	bool ret = false;
>  
>  	if (!info)
>  		return false;
> @@ -423,6 +433,95 @@ bool intel_config_get_integer(struct drm_i915_private *dev_priv,
>  	return false;
>  }
>  
> +
> +/*
> + * Use ACPI methods to enumerate the items in the WA list.
> + *
> + * The list from the ACPI table is inserted into the ring_init
> + * workaround table.  This could either over-wright any hard coded
> + * table or append to an existing hard coded table. Need to understand
> + * which way would make more sense.
> + */
> +bool intel_config_wa_add(struct drm_i915_private *dev_priv)
> +{
> +	struct intel_config_info *info = dev_priv->config_info;
> +	const union acpi_object *properties;
> +	const union acpi_object *prop, *name, *value;
> +	struct list_head *list;
> +	struct intel_config_node *n;
> +	int i;
> +	u32 idx;
> +	bool ret = false;
> +
> +	list = cfg_type_to_list(info, CFG_WORKAROUND);
> +	if (!list)
> +		return false;
> +
> +	list_for_each_entry(n, list, node) {
> +		properties = n->adev->data.properties;
> +		if (!properties)
> +			continue;
> +
> +		DRM_DEBUG_DRIVER("i915/config/wa: Found %d workarounds\n",
> +					properties->package.count);
> +
> +		for (i = 0; i < properties->package.count; i++) {
> +			prop = &properties->package.elements[i];
> +			name = &prop->package.elements[0];
> +			value = &prop->package.elements[1];
> +			ret = true;
> +
> +			/*
> +			 * Right now, it's assumed that any workarounds
> +			 * listed in the table should be added to the
> +			 * workaround array and applied. This could be a
> +			 * bad assumption.
> +			 *
> +			 * Might be better if the workaround table includes
> +			 * the PCI ID and check that before applying.
> +			 */
> +
> +			if ((value->package.count >= 2) &&
> +			    (value->type == ACPI_TYPE_PACKAGE)) {
> +				/*
> +				 * Package elements:
> +				 *   0 = workaround register
> +				 *   1 = register value
> +				 *   2 = mask
> +				 *   3 = mask enable vs disable bit (flags)
> +				 */
> +				idx = dev_priv->workarounds.count;
> +				if (WARN_ON(idx >= I915_MAX_WA_REGS))
> +					return ret;
> +
> +				dev_priv->workarounds.reg[idx].addr =
> +					(u32)value->package.elements[0].integer.value;
> +				dev_priv->workarounds.reg[idx].value =
> +					(u32)value->package.elements[1].integer.value;
> +
> +				if ((u32)value->package.elements[3].integer.value)
> +					dev_priv->workarounds.reg[idx].mask =
> +						_MASKED_BIT_ENABLE(
> +								(u32)value->package.elements[2].integer.value);
> +				else
> +					dev_priv->workarounds.reg[idx].mask =
> +						_MASKED_BIT_DISABLE(
> +								(u32)value->package.elements[2].integer.value);
> +
> +				dev_priv->workarounds.count++;
> +
> +				DRM_DEBUG_DRIVER("i915/config: WA %s = 0x%x, 0x%x, 0x%x\n",
> +					   name->string.pointer,
> +					   dev_priv->workarounds.reg[idx].addr,
> +					   dev_priv->workarounds.reg[idx].value,
> +					   dev_priv->workarounds.reg[idx].mask);
> +			}
> +		}
> +	}
> +
> +	return ret;
> +}
> +
>  /*
>   * Look up a drm property name in the configuration store and if
>   * found, return the value.
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 4c81ee9..e2b4e09 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -1265,11 +1265,13 @@ enum cfg_type {
>  	CFG_DRV,
>  	CFG_CRTC,
>  	CFG_CONNECTOR,
> -	CFG_PLANE
> +	CFG_PLANE,
> +	CFG_WORKAROUND
>  };
>  
>  void intel_config_init(struct drm_device *dev);
>  void intel_config_shutdown(struct drm_device *dev);
> +bool intel_config_wa_add(struct drm_i915_private *dev_priv);
>  bool intel_config_get_integer(struct drm_i915_private *dev_priv,
>  			  enum cfg_type cfg_type,
>  			  const char *name,
> -- 
> 2.1.0
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx