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@xxxxxxxxx>
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@xxxxxxxxxxxxxxxxxxxxx
> 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@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
