On Tue, 24 Feb 2015 14:51:31 +0100
Daniel Vetter <daniel@xxxxxxxx> wrote:
> 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
Yeah, I don't know that we have any valid use-case for this. It
really is just an idea. Maybe a bad idea. It was something that looked
like it could map into the configuration space so I coded it up.
I did think it might be useful if we could disable workarounds for early
steppings that may not be applicable for later steppings without the
need to release a new version of the driver. As it sits, it doesn't
support this use-case so would need a bit more work. For some embedded
appliance type devices, customers are sometimes more willing to take a
configuration change than a new driver.
The patches for this should be easy to drop without effecting anything
else.
>
> > ---
> > 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
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
