diff mbox

[RFC,01/12] drm/i915/config: Initial framework

Message ID 1423784498-11272-2-git-send-email-bob.j.paauwe@intel.com (mailing list archive)
State New, archived
Headers show

Commit Message

Paauwe, Bob J Feb. 12, 2015, 11:41 p.m. UTC
This adds an init-time configuration framework that parses configuration
data from an ACPI property table. The table is assumed to have well
defined sub-device property tables that correspond to the various
driver components.  Initially the following sub-device tables are
defined:

CRTC (CRTC)
   The CRTC sub-device table contains additional sub-device tables
   where each one corresponds to a CRTC.  Each CRTC sub-device must
   include a property called "id" whose value matches the driver's
   crtc id. Additional properties for the CRTC are used to configure
   the crtc.

Connector (CNCT)
   The CNCT sub-device table contains additional sub-device tables
   where each one corresponds to a connector. Each of the connector
   sub-device tables must include a property called "name" whose value
   matches a connector name assigned by the driver (see later patch
   for output name function). Additional connector properties can
   be set through these tables.

Plane (PLNS)
   The PLNS sub-device table contains additional sub-device tables
   where each one corresponds to a plane.  [this needs additional work]

In addition, the main device property table for the device may
contain configuration information that applies to general driver
configuration.

The framework includes a couple of helper functions to access the
configuration data.

   intel_config_get_integer() will look up a configuration property
   and return the integer value associated with it.

   intel_config_init_<component>_property() will look up a
   configuration property and assign the value to a drm
   property of the same name.  These functions are used to
   initialize drm property instances to specific values.

Signed-off-by: Bob Paauwe <bob.j.paauwe@intel.com>
---
 drivers/gpu/drm/i915/Makefile       |   3 +-
 drivers/gpu/drm/i915/i915_dma.c     |   4 +
 drivers/gpu/drm/i915/i915_drv.h     |  16 ++
 drivers/gpu/drm/i915/i915_params.c  |   6 +
 drivers/gpu/drm/i915/intel_config.c | 542 ++++++++++++++++++++++++++++++++++++
 drivers/gpu/drm/i915/intel_drv.h    |  28 ++
 6 files changed, 598 insertions(+), 1 deletion(-)
 create mode 100644 drivers/gpu/drm/i915/intel_config.c

Comments

Jani Nikula Feb. 13, 2015, 8:19 a.m. UTC | #1
On Fri, 13 Feb 2015, Bob Paauwe <bob.j.paauwe@intel.com> wrote:
> This adds an init-time configuration framework that parses configuration
> data from an ACPI property table. The table is assumed to have well
> defined sub-device property tables that correspond to the various
> driver components.  Initially the following sub-device tables are
> defined:
>
> CRTC (CRTC)
>    The CRTC sub-device table contains additional sub-device tables
>    where each one corresponds to a CRTC.  Each CRTC sub-device must
>    include a property called "id" whose value matches the driver's
>    crtc id. Additional properties for the CRTC are used to configure
>    the crtc.
>
> Connector (CNCT)
>    The CNCT sub-device table contains additional sub-device tables
>    where each one corresponds to a connector. Each of the connector
>    sub-device tables must include a property called "name" whose value
>    matches a connector name assigned by the driver (see later patch
>    for output name function). Additional connector properties can
>    be set through these tables.
>
> Plane (PLNS)
>    The PLNS sub-device table contains additional sub-device tables
>    where each one corresponds to a plane.  [this needs additional work]
>
> In addition, the main device property table for the device may
> contain configuration information that applies to general driver
> configuration.
>
> The framework includes a couple of helper functions to access the
> configuration data.
>
>    intel_config_get_integer() will look up a configuration property
>    and return the integer value associated with it.
>
>    intel_config_init_<component>_property() will look up a
>    configuration property and assign the value to a drm
>    property of the same name.  These functions are used to
>    initialize drm property instances to specific values.
>
> Signed-off-by: Bob Paauwe <bob.j.paauwe@intel.com>
> ---
>  drivers/gpu/drm/i915/Makefile       |   3 +-
>  drivers/gpu/drm/i915/i915_dma.c     |   4 +
>  drivers/gpu/drm/i915/i915_drv.h     |  16 ++
>  drivers/gpu/drm/i915/i915_params.c  |   6 +
>  drivers/gpu/drm/i915/intel_config.c | 542 ++++++++++++++++++++++++++++++++++++
>  drivers/gpu/drm/i915/intel_drv.h    |  28 ++
>  6 files changed, 598 insertions(+), 1 deletion(-)
>  create mode 100644 drivers/gpu/drm/i915/intel_config.c
>
> diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile
> index f025e7f..462de19 100644
> --- a/drivers/gpu/drm/i915/Makefile
> +++ b/drivers/gpu/drm/i915/Makefile
> @@ -12,7 +12,8 @@ i915-y := i915_drv.o \
>            i915_suspend.o \
>  	  i915_sysfs.o \
>  	  intel_pm.o \
> -	  intel_runtime_pm.o
> +	  intel_runtime_pm.o \
> +	  intel_config.o
>  
>  i915-$(CONFIG_COMPAT)   += i915_ioc32.o
>  i915-$(CONFIG_DEBUG_FS) += i915_debugfs.o
> diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
> index 5804aa5..9501360 100644
> --- a/drivers/gpu/drm/i915/i915_dma.c
> +++ b/drivers/gpu/drm/i915/i915_dma.c
> @@ -656,6 +656,8 @@ int i915_driver_load(struct drm_device *dev, unsigned long flags)
>  	dev->dev_private = dev_priv;
>  	dev_priv->dev = dev;
>  
> +	intel_config_init(dev);
> +
>  	/* Setup the write-once "constant" device info */
>  	device_info = (struct intel_device_info *)&dev_priv->info;
>  	memcpy(device_info, info, sizeof(dev_priv->info));
> @@ -929,6 +931,8 @@ int i915_driver_unload(struct drm_device *dev)
>  
>  	acpi_video_unregister();
>  
> +	intel_config_shutdown(dev);
> +
>  	if (drm_core_check_feature(dev, DRIVER_MODESET))
>  		intel_fbdev_fini(dev);
>  
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 2dedd43..165091c 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -1645,6 +1645,20 @@ struct i915_virtual_gpu {
>  	bool active;
>  };
>  
> +struct intel_config_node {
> +	struct acpi_device *adev;
> +	struct list_head node;
> +	struct list_head list;
> +};
> +
> +struct intel_config_info {
> +	struct intel_config_node base;
> +	struct list_head crtc_list;
> +	struct list_head connector_list;
> +	struct list_head plane_list;
> +};
> +
> +
>  struct drm_i915_private {
>  	struct drm_device *dev;
>  	struct kmem_cache *slab;
> @@ -1886,6 +1900,7 @@ struct drm_i915_private {
>  	u32 long_hpd_port_mask;
>  	u32 short_hpd_port_mask;
>  	struct work_struct dig_port_work;
> +	struct intel_config_info *config_info;
>  
>  	/*
>  	 * if we get a HPD irq from DP and a HPD irq from non-DP
> @@ -2528,6 +2543,7 @@ struct i915_params {
>  	int enable_ips;
>  	int invert_brightness;
>  	int enable_cmd_parser;
> +	char cfg_firmware[PATH_MAX];
>  	/* leave bools at the end to not create holes */
>  	bool enable_hangcheck;
>  	bool fastboot;
> diff --git a/drivers/gpu/drm/i915/i915_params.c b/drivers/gpu/drm/i915/i915_params.c
> index 44f2262..f92621c 100644
> --- a/drivers/gpu/drm/i915/i915_params.c
> +++ b/drivers/gpu/drm/i915/i915_params.c
> @@ -53,6 +53,7 @@ struct i915_params i915 __read_mostly = {
>  	.mmio_debug = 0,
>  	.verbose_state_checks = 1,
>  	.nuclear_pageflip = 0,
> +	.cfg_firmware = "",
>  };
>  
>  module_param_named(modeset, i915.modeset, int, 0400);
> @@ -183,3 +184,8 @@ MODULE_PARM_DESC(verbose_state_checks,
>  module_param_named_unsafe(nuclear_pageflip, i915.nuclear_pageflip, bool, 0600);
>  MODULE_PARM_DESC(nuclear_pageflip,
>  		 "Force atomic modeset functionality; only planes work for now (default: false).");
> +
> +module_param_string(cfg_firmware, i915.cfg_firmware, sizeof(i915.cfg_firmware), 0444);
> +MODULE_PARM_DESC(cfg_firmware,
> +		 "Load configuration firmware from built-in data or /lib/firmware. ");

This isn't used now.

> +
> diff --git a/drivers/gpu/drm/i915/intel_config.c b/drivers/gpu/drm/i915/intel_config.c
> new file mode 100644
> index 0000000..cf7da93
> --- /dev/null
> +++ b/drivers/gpu/drm/i915/intel_config.c
> @@ -0,0 +1,542 @@
> +/*
> + * i915 configuration via ACPI device properties.
> + *
> + * Copyright (C) 2014, 2015 Intel Corporation
> + *
> + * Permission to use, copy, modify, distribute, and sell this software and its
> + * documentation for any purpose is hereby granted without fee, provided that
> + * the above copyright notice appear in all copies and that both that copyright
> + * notice and this permission notice appear in supporting documentation, and
> + * that the name of the copyright holders not be used in advertising or
> + * publicity pertaining to distribution of the software without specific,
> + * written prior permission.  The copyright holders make no representations
> + * about the suitability of this software for any purpose.  It is provided "as
> + * is" without express or implied warranty.
> + *
> + * THE COPYRIGHT HOLDERS DISCLAIM ALL WARRANTIES WITH REGARD TO THIS SOFTWARE,
> + * INCLUDING ALL IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS, IN NO
> + * EVENT SHALL THE COPYRIGHT HOLDERS BE LIABLE FOR ANY SPECIAL, INDIRECT OR
> + * CONSEQUENTIAL DAMAGES OR ANY DAMAGES WHATSOEVER RESULTING FROM LOSS OF USE,
> + * DATA OR PROFITS, WHETHER IN AN ACTION OF CONTRACT, NEGLIGENCE OR OTHER
> + * TORTIOUS ACTION, ARISING OUT OF OR IN CONNECTION WITH THE USE OR PERFORMANCE
> + * OF THIS SOFTWARE.
> + */
> +#include <linux/acpi.h>
> +#include <linux/init.h>
> +#include <linux/module.h>
> +#include <linux/firmware.h>
> +#include <linux/ctype.h>
> +#include <acpi/acpi_bus.h>
> +#include "intel_drv.h"
> +#include <drm/i915_drm.h>
> +#include "i915_drv.h"
> +#include "i915_trace.h"
> +#include <drm/drmP.h>
> +#include <drm/drm_crtc.h>
> +#include <drm/drm_crtc_helper.h>
> +
> +
> +#define i915_ACPI_PRP_ROOT "\\_SB.PRP.GFX0"
> +
> +/*
> + * Load an ACPI property table into the ACPI subsystem.
> + *
> + * First, verify that a table isn't already loaded.  The table may
> + * be part of the ACPI firmware and thus loaded by the ACPI sub-system
> + * during boot.  It is also possible for ACPI sub-system to load tables
> + * to override those supplied by the firmware.
> + *
> + * If a property table for the GFX device has not been loaded, attempt
> + * to load one from /lib/firmware here.  The name will default to
> + * drm_i915.aml but the name be overridden by the cfg_firmware module
> + * parameter.
> + *
> + * The order of precidence for table loading is:
> + *   - dyanamic table loaded by ACPI driver
> + *   - table built into firmware
> + *   - dynamic table loaded based on driver name or module parameter
> + *
> + * If the table is loaded by the driver, it will be unloaded when the
> + * driver module is unloaded.  Tables that are part of the firmware or
> + * overridden by the ACPI driver are not unloaded and cannot be replaced
> + * by tables loaded by the driver.
> + */
> +static int intel_acpi_load_table(struct drm_device *dev, char *firmware)
> +{
> +	struct acpi_table_header *table;
> +	acpi_status status;
> +	acpi_handle handle;
> +	struct acpi_device *acpi_dev = NULL;
> +	const struct firmware *fw;
> +	int ret = 0;
> +
> +	/* make sure the table isn't already loaded before loading it */
> +	status = acpi_get_handle(ACPI_ROOT_OBJECT, i915_ACPI_PRP_ROOT, &handle);
> +	if (ACPI_FAILURE(status)) {
> +
> +		/* Try to dynamically load a table.... */
> +		DRM_DEBUG_DRIVER("Requesting configuration table: %s\n",
> +				 firmware);
> +		ret = request_firmware(&fw, firmware, dev->dev);
> +		if (ret != 0) {
> +			DRM_ERROR("Failed to find ACPI table %s: %d\n",
> +				  firmware, ret);
> +			fw = NULL;
> +			goto bad_table;
> +		} else {
> +			table = (struct acpi_table_header *)fw->data;
> +		}
> +
> +		/* Load the table into the acpi device. */
> +		status = acpi_load_table(table);
> +		if (ACPI_FAILURE(status))
> +			goto bad_table;
> +
> +		add_taint(TAINT_OVERRIDDEN_ACPI_TABLE, true);
> +
> +		/* Get a handle and scan the bus */
> +		status = acpi_get_handle(ACPI_ROOT_OBJECT, i915_ACPI_PRP_ROOT,
> +					 &handle);
> +		acpi_bus_scan(handle);
> +
> +		if (acpi_bus_get_device(handle, &acpi_dev))
> +			goto bad_table;
> +
> +		acpi_bind_one(dev->dev, acpi_dev);
> +
> +	} else {
> +		DRM_DEBUG_DRIVER("ACPI Property table previously loaded for _SB.PRP.GFX0\n");
> +	}
> +
> +	return 0;
> +
> +bad_table:
> +	release_firmware(fw);
> +	return -ENODEV;
> +}
> +
> +/*
> + * Use ACPI methods to get the property.
> + *
> + * This isn't using the generic device_property_read because that
> + * can only access properties associated with the actual device. It
> + * doesn't understand our sub-component property tree.
> + */
> +static bool node_property(struct intel_config_node *n,
> +			  const char *prop,
> +			  void *value)
> +{
> +	int ret = 0;
> +	const union acpi_object *obj;
> +
> +	ret = acpi_dev_get_property(n->adev, prop, ACPI_TYPE_ANY, &obj);
> +	if (ret == -ENODATA) {
> +		/*
> +		 * This isn't really a failure, it's ok if the property
> +		 * doesn't exist. The message is for debug purposes only.
> +		 */
> +		DRM_DEBUG_DRIVER("Property \"%s\" not found in %s\n",
> +				 prop, acpi_device_bid(n->adev));
> +	} else if (ret == -EINVAL) {
> +		DRM_ERROR("Invalid acpi device or property name.\n");
> +	} else if (ret) {
> +		/* This is a failure. */
> +		DRM_ERROR("Property request failed for %s: %d\n", prop, ret);
> +	} else {
> +		switch (obj->type) {
> +		case ACPI_TYPE_INTEGER:
> +			*(u32 *)value = obj->integer.value;
> +			break;
> +		case ACPI_TYPE_STRING:
> +			*(char **)value = obj->string.pointer;
> +			break;
> +		default:
> +			DRM_ERROR("Unsupported property type, only support integer and string.\n");
> +			ret = -1;
> +			break;
> +		}
> +	}
> +
> +	return ret == 0;
> +}
> +
> +
> +/**
> + * intel_config_init -
> + *
> + * Initialize the configuration framework by attempting to load a
> + * property table and parse the contents into component lists.
> + *
> + * @dev: The drm driver device.
> + */
> +void intel_config_init(struct drm_device *dev)
> +{
> +	struct intel_config_info *info;
> +	struct intel_config_node *new_node, *tmp;
> +	acpi_handle handle;
> +	struct acpi_device *cl, *component;
> +	char *cname;
> +	struct drm_i915_private *dev_priv = dev->dev_private;
> +
> +	/* Load an ACPI table from /lib/firmware.  */
> +	snprintf(i915.cfg_firmware, PATH_MAX, "drm_%s.aml", dev->driver->name);
> +
> +	if (intel_acpi_load_table(dev, i915.cfg_firmware) != 0) {
> +		DRM_ERROR("Failed to load ACPI device property table.\n");
> +		return;
> +	}
> +
> +	DRM_DEBUG_DRIVER("Loaded ACPI configuration for %s\n",
> +			 dev->driver->name);
> +
> +	info = kzalloc(sizeof(*info), GFP_KERNEL);
> +	if (!info) {
> +		DRM_ERROR("Failed to allocate memory for configuration.\n");
> +		return;
> +	}
> +
> +	INIT_LIST_HEAD(&info->base.list);
> +	INIT_LIST_HEAD(&info->crtc_list);
> +	INIT_LIST_HEAD(&info->connector_list);
> +	INIT_LIST_HEAD(&info->plane_list);
> +
> +	handle = ACPI_HANDLE(dev->dev);
> +	if (!handle) {
> +		DRM_DEBUG_DRIVER("No associated ACPI handle.\n");
> +		kfree(info);
> +		return;
> +	} else {
> +		acpi_bus_get_device(handle, &info->base.adev);
> +	}
> +
> +	/*
> +	 * Create a list of one for the top level driver config.
> +	 *
> +	 * We don't really need a full config_info structure for this but
> +	 * it simplifies the handling of driver general config settings
> +	 * as we don't have to have a special case and unique structure
> +	 * just for this.
> +	 */
> +	INIT_LIST_HEAD(&info->base.node);
> +	list_add_tail(&info->base.node, &info->base.list);
> +
> +/* Component sub-device ACPI names */
> +#define i915_COMPONENT_CRTC "CRTC"
> +#define i915_COMPONENT_CONNECTOR "CNCT"
> +#define i915_COMPONENT_PLANE "PLNS"
> +
> +	/* Build lists */
> +	list_for_each_entry(component, &info->base.adev->children, node) {
> +		if (!component)
> +			continue;
> +
> +		cname = acpi_device_bid(component);
> +
> +		list_for_each_entry(cl, &component->children, node) {
> +			new_node = kzalloc(sizeof(*new_node), GFP_KERNEL);
> +			if (!new_node)
> +				goto bail;
> +			new_node->adev = cl;
> +			INIT_LIST_HEAD(&new_node->node);
> +
> +			/* Add to the appropriate list */
> +			if (strcmp(cname, i915_COMPONENT_CRTC) == 0) {
> +				list_add_tail(&new_node->node,
> +					      &info->crtc_list);
> +			} else if (strcmp(cname, i915_COMPONENT_CONNECTOR) == 0) {
> +				list_add_tail(&new_node->node,
> +					      &info->connector_list);
> +			} else if (strcmp(cname, i915_COMPONENT_PLANE) == 0) {
> +				list_add_tail(&new_node->node,
> +					      &info->plane_list);
> +			} else {
> +				/* unknown component, ignore it */
> +				kfree(new_node);
> +			}
> +		}
> +	}
> +
> +	dev_priv->config_info = info;
> +	DRM_DEBUG_DRIVER("i915 Configuration loaded.\n");
> +	return;
> +
> +bail:
> +	DRM_DEBUG_DRIVER("i915 Configuration aborted, insufficient memory.\n");
> +	list_for_each_entry_safe(new_node, tmp, &info->crtc_list, node)
> +		kfree(new_node);
> +	list_for_each_entry_safe(new_node, tmp, &info->connector_list, node)
> +		kfree(new_node);
> +	list_for_each_entry_safe(new_node, tmp, &info->plane_list, node)
> +		kfree(new_node);
> +
> +	kfree(info);
> +	dev_priv->config_info = NULL;

You should probably abstract this part to reduce duplication with
intel_config_shutdown. You also leak dev_priv->config_info there.

> +}
> +
> +
> +/**
> + * Clean up the configuration infrastructure as the driver is
> + * shuting down.  Don't leak memory!
> + *
> + * @dev: The drm driver device.
> + */
> +void intel_config_shutdown(struct drm_device *dev)
> +{
> +	struct drm_i915_private *dev_priv = dev->dev_private;
> +	acpi_handle handle;
> +	acpi_status status;
> +	struct acpi_device *adev;
> +	struct intel_config_info *info;
> +	struct intel_config_node *n, *tmp;
> +
> +	if (dev_priv->config_info == NULL)
> +		return;
> +
> +	/* Free the component lists.  */
> +	info = dev_priv->config_info;
> +	list_for_each_entry_safe(n, tmp, &info->crtc_list, node)
> +		kfree(n);
> +	list_for_each_entry_safe(n, tmp, &info->connector_list, node)
> +		kfree(n);
> +	list_for_each_entry_safe(n, tmp, &info->plane_list, node)
> +		kfree(n);
> +
> +	/* Unload any dynamically loaded ACPI property table */
> +	handle = ACPI_HANDLE(dev->dev);
> +
> +	DRM_DEBUG_DRIVER("Unloading dynamic i915 Configuration ACPI table.\n");
> +	if (handle) {
> +
> +		acpi_unbind_one(dev->dev);
> +
> +		if (acpi_bus_get_device(handle, &adev))
> +			DRM_ERROR("Failed to get ACPI bus device.\n");
> +		else
> +			acpi_bus_trim(adev);
> +
> +		status = acpi_unload_parent_table(handle);
> +		if (ACPI_FAILURE(status))
> +			DRM_ERROR("Failed to unload the i915 Configuration"
> +				  "ACPI table. %d\n", status);
> +	}
> +}
> +
> +
> +
> +/*
> + * does_name_match
> + *
> + * The various drm components have names assocated with them. To link
> + * a component in the ACPI property tree, use a "special" property
> + * called "name".
> + *
> + * The exception is the general driver properties.  There is no "name"
> + * property associated with those.  We could force one, but that may
> + * be less intuitive than leaving the name empty.
> + *
> + * This function look for a property called "name" and compares the
> + * value of that property with the passed in name parameter.
> + */
> +static bool does_name_match(struct intel_config_node *node, const char *name)
> +{
> +	char *p_name;
> +
> +	/*
> +	 * General driver properties aren't in a section with a "name"
> +	 * property. Thus this should just return true in that case.
> +	 */
> +	if (!name || strlen(name) == 0)
> +		return true;
> +
> +
> +	/* Look up a name property and see if it matches */
> +	if (node_property(node, "name", &p_name)) {
> +		if (strcmp(name, p_name) == 0)
> +			return true;
> +	}
> +
> +	return false;
> +}
> +
> +
> +/*
> + * Map the configuration sub component enum to the cached
> + * sub component list.
> + */
> +static struct list_head *cfg_type_to_list(struct intel_config_info *info,
> +					  enum cfg_type cfg_type)
> +{
> +	switch (cfg_type) {
> +	case CFG_DRV:
> +		return &info->base.list;
> +	case CFG_CRTC:
> +		return &info->crtc_list;
> +	case CFG_CONNECTOR:
> +		return &info->connector_list;
> +	case CFG_PLANE:
> +		return &info->plane_list;
> +	}
> +	return NULL;
> +}
> +
> +/*
> + * Integer property.
> + *
> + * Look up a property and set the value pointer to the property value.
> + *
> + * returns true if the property was found, false if not.
> + */
> +bool intel_config_get_integer(struct drm_i915_private *dev_priv,
> +			      enum cfg_type cfg_type,
> +			      const char *name,
> +			      const char *property,
> +			      uint32_t *value)
> +{
> +	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;
> +
> +	list = cfg_type_to_list(info, cfg_type);
> +	if (!list)
> +		return false;
> +
> +	list_for_each_entry(n, list, node) {
> +		if (does_name_match(n, name)) {
> +			if (node_property(n, property, value))
> +				return true;
> +		}
> +	}
> +
> +	return false;
> +}
> +
> +/*
> + * Look up a drm property name in the configuration store and if
> + * found, return the value.
> + *
> + * A default value is included in the parameter list so that something
> + * reasonable is returned if the lookup fails to find a matching
> + * configuration key.
> + */
> +static uint64_t lookup_property(struct drm_i915_private *dev_priv,
> +				const char *name,
> +				struct drm_property *property,
> +				enum cfg_type cfg_type,
> +				uint64_t dflt)
> +{
> +	struct intel_config_info *info = dev_priv->config_info;
> +	struct drm_property_enum *prop_enum;
> +	const char *value;
> +	uint64_t retval = dflt;
> +	struct intel_config_node *n;
> +	struct list_head *list;
> +
> +	list = cfg_type_to_list(info, cfg_type);
> +	if (!list)
> +		goto out;
> +
> +	list_for_each_entry(n, list, node) {
> +		if (!does_name_match(n, name))
> +			continue;
> +		/*
> +		 * FIXME: This is assuming that the type is string
> +		 * for enun drm properties.  This should be more
> +		 * generic and map drm property types to ACPI property
> +		 * types.
> +		 */
> +		if (!node_property(n, property->name, &value))
> +			continue;
> +
> +		/* Look for a matching enum */
> +		list_for_each_entry(prop_enum, &property->enum_list, head) {
> +			if (strcmp(value, prop_enum->name) == 0) {
> +				retval = prop_enum->value;
> +				goto out;
> +			}
> +		}
> +	}
> +
> +out:
> +	return retval;
> +}
> +
> +/*
> + * Connector properties.
> + *
> + * If a connector drm property has a corresponding configuration property,
> + * use the configuration property value to initialize the drm property.
> + */
> +uint64_t intel_config_init_connector_property(struct drm_connector *connector,
> +					      const char *name,
> +					      struct drm_property *property,
> +					      uint64_t dflt)
> +{
> +	struct drm_i915_private *dev_priv = connector->dev->dev_private;
> +	struct intel_config_info *info = dev_priv->config_info;
> +	uint64_t retval;
> +
> +	if (!info)
> +		goto out;
> +
> +	retval = lookup_property(dev_priv, name, property, CFG_CONNECTOR, dflt);
> +
> +out:
> +	drm_object_attach_property(&connector->base, property, retval);
> +	return retval;
> +}
> +
> +
> +/*
> + * Plane properties.
> + *
> + * If a plane drm property has a corresponding configuration property,
> + * use the configuration property value to initialize the drm property.
> + */
> +uint64_t intel_config_init_plane_property(struct drm_plane *plane,
> +					  const char *name,
> +					  struct drm_property *property,
> +					  uint64_t dflt)
> +{
> +	struct drm_i915_private *dev_priv = plane->dev->dev_private;
> +	struct intel_config_info *info = dev_priv->config_info;
> +	uint64_t retval;
> +
> +	if (!info)
> +		goto out;
> +
> +	retval = lookup_property(dev_priv, name, property, CFG_PLANE, dflt);
> +
> +out:
> +	drm_object_attach_property(&plane->base, property, retval);
> +	return retval;
> +}
> +
> +
> +/*
> + * CRTC properties.
> + *
> + * If a crtc drm property has a corresponding configuration property,
> + * use the configuration property value to initialize the drm property.
> + */
> +uint64_t intel_config_init_crtc_property(struct drm_crtc *crtc,
> +					 const char *name,
> +					 struct drm_property *property,
> +					 uint64_t dflt)
> +{
> +	struct drm_i915_private *dev_priv = crtc->dev->dev_private;
> +	struct intel_config_info *info = dev_priv->config_info;
> +	uint64_t retval;
> +
> +	if (!info)
> +		goto out;
> +
> +	retval = lookup_property(dev_priv, name, property, CFG_CRTC, dflt);
> +
> +out:
> +	drm_object_attach_property(&crtc->base, property, retval);
> +	return retval;
> +}
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 1de8e20..aefd95e 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -1259,4 +1259,32 @@ void intel_plane_destroy_state(struct drm_plane *plane,
>  			       struct drm_plane_state *state);
>  extern const struct drm_plane_helper_funcs intel_plane_helper_funcs;
>  
> +/* intel_config.c */
> +enum cfg_type {
> +	CFG_DRV,
> +	CFG_CRTC,
> +	CFG_CONNECTOR,
> +	CFG_PLANE
> +};
> +
> +void intel_config_init(struct drm_device *dev);
> +void intel_config_shutdown(struct drm_device *dev);
> +bool intel_config_get_integer(struct drm_i915_private *dev_priv,
> +			  enum cfg_type cfg_type,
> +			  const char *name,
> +			  const char *property,
> +			  uint32_t *value);
> +uint64_t intel_config_init_connector_property(struct drm_connector *connector,
> +			  const char *name,
> +			  struct drm_property *property,
> +			  uint64_t dflt);
> +uint64_t intel_config_init_plane_property(struct drm_plane *plane,
> +			  const char *name,
> +			  struct drm_property *property,
> +			  uint64_t dflt);
> +uint64_t intel_config_init_crtc_property(struct drm_crtc *crtc,
> +			  const char *name,
> +			  struct drm_property *property,
> +			  uint64_t dflt);
> +
>  #endif /* __INTEL_DRV_H__ */
> -- 
> 2.1.0
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Daniel Vetter Feb. 24, 2015, 4:17 p.m. UTC | #2
On Thu, Feb 12, 2015 at 03:41:27PM -0800, Bob Paauwe wrote:
> This adds an init-time configuration framework that parses configuration
> data from an ACPI property table. The table is assumed to have well
> defined sub-device property tables that correspond to the various
> driver components.  Initially the following sub-device tables are
> defined:
> 
> CRTC (CRTC)
>    The CRTC sub-device table contains additional sub-device tables
>    where each one corresponds to a CRTC.  Each CRTC sub-device must
>    include a property called "id" whose value matches the driver's
>    crtc id. Additional properties for the CRTC are used to configure
>    the crtc.
> 
> Connector (CNCT)
>    The CNCT sub-device table contains additional sub-device tables
>    where each one corresponds to a connector. Each of the connector
>    sub-device tables must include a property called "name" whose value
>    matches a connector name assigned by the driver (see later patch
>    for output name function). Additional connector properties can
>    be set through these tables.
> 
> Plane (PLNS)
>    The PLNS sub-device table contains additional sub-device tables
>    where each one corresponds to a plane.  [this needs additional work]
> 
> In addition, the main device property table for the device may
> contain configuration information that applies to general driver
> configuration.
> 
> The framework includes a couple of helper functions to access the
> configuration data.
> 
>    intel_config_get_integer() will look up a configuration property
>    and return the integer value associated with it.
> 
>    intel_config_init_<component>_property() will look up a
>    configuration property and assign the value to a drm
>    property of the same name.  These functions are used to
>    initialize drm property instances to specific values.
> 
> Signed-off-by: Bob Paauwe <bob.j.paauwe@intel.com>

Like Jani I'm mostly concerned about maintaining yet another slightly
different definition of paramters and settings here - we already have (or
will soon grow) per crtc/plane/connector properties for atomic.

My high-level idea for this was that we reuse the atomic support (once we
have it) and just take the acpi tables as a slightly different source for
an atomic update. Since the design of a name/value pair for objects is
very similar to what we do with the atomic ioctl this hopefully should map
fairly well.

Internally in the driver the only work needed to do would be to add any
missing properties. All the acpi atomic update code should be fully i915
agnostic and only use generic atomic update interfaces.

Would this work or do I overlook something important here.
-Daniel

> ---
>  drivers/gpu/drm/i915/Makefile       |   3 +-
>  drivers/gpu/drm/i915/i915_dma.c     |   4 +
>  drivers/gpu/drm/i915/i915_drv.h     |  16 ++
>  drivers/gpu/drm/i915/i915_params.c  |   6 +
>  drivers/gpu/drm/i915/intel_config.c | 542 ++++++++++++++++++++++++++++++++++++
>  drivers/gpu/drm/i915/intel_drv.h    |  28 ++
>  6 files changed, 598 insertions(+), 1 deletion(-)
>  create mode 100644 drivers/gpu/drm/i915/intel_config.c
> 
> diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile
> index f025e7f..462de19 100644
> --- a/drivers/gpu/drm/i915/Makefile
> +++ b/drivers/gpu/drm/i915/Makefile
> @@ -12,7 +12,8 @@ i915-y := i915_drv.o \
>            i915_suspend.o \
>  	  i915_sysfs.o \
>  	  intel_pm.o \
> -	  intel_runtime_pm.o
> +	  intel_runtime_pm.o \
> +	  intel_config.o
>  
>  i915-$(CONFIG_COMPAT)   += i915_ioc32.o
>  i915-$(CONFIG_DEBUG_FS) += i915_debugfs.o
> diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
> index 5804aa5..9501360 100644
> --- a/drivers/gpu/drm/i915/i915_dma.c
> +++ b/drivers/gpu/drm/i915/i915_dma.c
> @@ -656,6 +656,8 @@ int i915_driver_load(struct drm_device *dev, unsigned long flags)
>  	dev->dev_private = dev_priv;
>  	dev_priv->dev = dev;
>  
> +	intel_config_init(dev);
> +
>  	/* Setup the write-once "constant" device info */
>  	device_info = (struct intel_device_info *)&dev_priv->info;
>  	memcpy(device_info, info, sizeof(dev_priv->info));
> @@ -929,6 +931,8 @@ int i915_driver_unload(struct drm_device *dev)
>  
>  	acpi_video_unregister();
>  
> +	intel_config_shutdown(dev);
> +
>  	if (drm_core_check_feature(dev, DRIVER_MODESET))
>  		intel_fbdev_fini(dev);
>  
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 2dedd43..165091c 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -1645,6 +1645,20 @@ struct i915_virtual_gpu {
>  	bool active;
>  };
>  
> +struct intel_config_node {
> +	struct acpi_device *adev;
> +	struct list_head node;
> +	struct list_head list;
> +};
> +
> +struct intel_config_info {
> +	struct intel_config_node base;
> +	struct list_head crtc_list;
> +	struct list_head connector_list;
> +	struct list_head plane_list;
> +};
> +
> +
>  struct drm_i915_private {
>  	struct drm_device *dev;
>  	struct kmem_cache *slab;
> @@ -1886,6 +1900,7 @@ struct drm_i915_private {
>  	u32 long_hpd_port_mask;
>  	u32 short_hpd_port_mask;
>  	struct work_struct dig_port_work;
> +	struct intel_config_info *config_info;
>  
>  	/*
>  	 * if we get a HPD irq from DP and a HPD irq from non-DP
> @@ -2528,6 +2543,7 @@ struct i915_params {
>  	int enable_ips;
>  	int invert_brightness;
>  	int enable_cmd_parser;
> +	char cfg_firmware[PATH_MAX];
>  	/* leave bools at the end to not create holes */
>  	bool enable_hangcheck;
>  	bool fastboot;
> diff --git a/drivers/gpu/drm/i915/i915_params.c b/drivers/gpu/drm/i915/i915_params.c
> index 44f2262..f92621c 100644
> --- a/drivers/gpu/drm/i915/i915_params.c
> +++ b/drivers/gpu/drm/i915/i915_params.c
> @@ -53,6 +53,7 @@ struct i915_params i915 __read_mostly = {
>  	.mmio_debug = 0,
>  	.verbose_state_checks = 1,
>  	.nuclear_pageflip = 0,
> +	.cfg_firmware = "",
>  };
>  
>  module_param_named(modeset, i915.modeset, int, 0400);
> @@ -183,3 +184,8 @@ MODULE_PARM_DESC(verbose_state_checks,
>  module_param_named_unsafe(nuclear_pageflip, i915.nuclear_pageflip, bool, 0600);
>  MODULE_PARM_DESC(nuclear_pageflip,
>  		 "Force atomic modeset functionality; only planes work for now (default: false).");
> +
> +module_param_string(cfg_firmware, i915.cfg_firmware, sizeof(i915.cfg_firmware), 0444);
> +MODULE_PARM_DESC(cfg_firmware,
> +		 "Load configuration firmware from built-in data or /lib/firmware. ");
> +
> diff --git a/drivers/gpu/drm/i915/intel_config.c b/drivers/gpu/drm/i915/intel_config.c
> new file mode 100644
> index 0000000..cf7da93
> --- /dev/null
> +++ b/drivers/gpu/drm/i915/intel_config.c
> @@ -0,0 +1,542 @@
> +/*
> + * i915 configuration via ACPI device properties.
> + *
> + * Copyright (C) 2014, 2015 Intel Corporation
> + *
> + * Permission to use, copy, modify, distribute, and sell this software and its
> + * documentation for any purpose is hereby granted without fee, provided that
> + * the above copyright notice appear in all copies and that both that copyright
> + * notice and this permission notice appear in supporting documentation, and
> + * that the name of the copyright holders not be used in advertising or
> + * publicity pertaining to distribution of the software without specific,
> + * written prior permission.  The copyright holders make no representations
> + * about the suitability of this software for any purpose.  It is provided "as
> + * is" without express or implied warranty.
> + *
> + * THE COPYRIGHT HOLDERS DISCLAIM ALL WARRANTIES WITH REGARD TO THIS SOFTWARE,
> + * INCLUDING ALL IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS, IN NO
> + * EVENT SHALL THE COPYRIGHT HOLDERS BE LIABLE FOR ANY SPECIAL, INDIRECT OR
> + * CONSEQUENTIAL DAMAGES OR ANY DAMAGES WHATSOEVER RESULTING FROM LOSS OF USE,
> + * DATA OR PROFITS, WHETHER IN AN ACTION OF CONTRACT, NEGLIGENCE OR OTHER
> + * TORTIOUS ACTION, ARISING OUT OF OR IN CONNECTION WITH THE USE OR PERFORMANCE
> + * OF THIS SOFTWARE.
> + */
> +#include <linux/acpi.h>
> +#include <linux/init.h>
> +#include <linux/module.h>
> +#include <linux/firmware.h>
> +#include <linux/ctype.h>
> +#include <acpi/acpi_bus.h>
> +#include "intel_drv.h"
> +#include <drm/i915_drm.h>
> +#include "i915_drv.h"
> +#include "i915_trace.h"
> +#include <drm/drmP.h>
> +#include <drm/drm_crtc.h>
> +#include <drm/drm_crtc_helper.h>
> +
> +
> +#define i915_ACPI_PRP_ROOT "\\_SB.PRP.GFX0"
> +
> +/*
> + * Load an ACPI property table into the ACPI subsystem.
> + *
> + * First, verify that a table isn't already loaded.  The table may
> + * be part of the ACPI firmware and thus loaded by the ACPI sub-system
> + * during boot.  It is also possible for ACPI sub-system to load tables
> + * to override those supplied by the firmware.
> + *
> + * If a property table for the GFX device has not been loaded, attempt
> + * to load one from /lib/firmware here.  The name will default to
> + * drm_i915.aml but the name be overridden by the cfg_firmware module
> + * parameter.
> + *
> + * The order of precidence for table loading is:
> + *   - dyanamic table loaded by ACPI driver
> + *   - table built into firmware
> + *   - dynamic table loaded based on driver name or module parameter
> + *
> + * If the table is loaded by the driver, it will be unloaded when the
> + * driver module is unloaded.  Tables that are part of the firmware or
> + * overridden by the ACPI driver are not unloaded and cannot be replaced
> + * by tables loaded by the driver.
> + */
> +static int intel_acpi_load_table(struct drm_device *dev, char *firmware)
> +{
> +	struct acpi_table_header *table;
> +	acpi_status status;
> +	acpi_handle handle;
> +	struct acpi_device *acpi_dev = NULL;
> +	const struct firmware *fw;
> +	int ret = 0;
> +
> +	/* make sure the table isn't already loaded before loading it */
> +	status = acpi_get_handle(ACPI_ROOT_OBJECT, i915_ACPI_PRP_ROOT, &handle);
> +	if (ACPI_FAILURE(status)) {
> +
> +		/* Try to dynamically load a table.... */
> +		DRM_DEBUG_DRIVER("Requesting configuration table: %s\n",
> +				 firmware);
> +		ret = request_firmware(&fw, firmware, dev->dev);
> +		if (ret != 0) {
> +			DRM_ERROR("Failed to find ACPI table %s: %d\n",
> +				  firmware, ret);
> +			fw = NULL;
> +			goto bad_table;
> +		} else {
> +			table = (struct acpi_table_header *)fw->data;
> +		}
> +
> +		/* Load the table into the acpi device. */
> +		status = acpi_load_table(table);
> +		if (ACPI_FAILURE(status))
> +			goto bad_table;
> +
> +		add_taint(TAINT_OVERRIDDEN_ACPI_TABLE, true);
> +
> +		/* Get a handle and scan the bus */
> +		status = acpi_get_handle(ACPI_ROOT_OBJECT, i915_ACPI_PRP_ROOT,
> +					 &handle);
> +		acpi_bus_scan(handle);
> +
> +		if (acpi_bus_get_device(handle, &acpi_dev))
> +			goto bad_table;
> +
> +		acpi_bind_one(dev->dev, acpi_dev);
> +
> +	} else {
> +		DRM_DEBUG_DRIVER("ACPI Property table previously loaded for _SB.PRP.GFX0\n");
> +	}
> +
> +	return 0;
> +
> +bad_table:
> +	release_firmware(fw);
> +	return -ENODEV;
> +}
> +
> +/*
> + * Use ACPI methods to get the property.
> + *
> + * This isn't using the generic device_property_read because that
> + * can only access properties associated with the actual device. It
> + * doesn't understand our sub-component property tree.
> + */
> +static bool node_property(struct intel_config_node *n,
> +			  const char *prop,
> +			  void *value)
> +{
> +	int ret = 0;
> +	const union acpi_object *obj;
> +
> +	ret = acpi_dev_get_property(n->adev, prop, ACPI_TYPE_ANY, &obj);
> +	if (ret == -ENODATA) {
> +		/*
> +		 * This isn't really a failure, it's ok if the property
> +		 * doesn't exist. The message is for debug purposes only.
> +		 */
> +		DRM_DEBUG_DRIVER("Property \"%s\" not found in %s\n",
> +				 prop, acpi_device_bid(n->adev));
> +	} else if (ret == -EINVAL) {
> +		DRM_ERROR("Invalid acpi device or property name.\n");
> +	} else if (ret) {
> +		/* This is a failure. */
> +		DRM_ERROR("Property request failed for %s: %d\n", prop, ret);
> +	} else {
> +		switch (obj->type) {
> +		case ACPI_TYPE_INTEGER:
> +			*(u32 *)value = obj->integer.value;
> +			break;
> +		case ACPI_TYPE_STRING:
> +			*(char **)value = obj->string.pointer;
> +			break;
> +		default:
> +			DRM_ERROR("Unsupported property type, only support integer and string.\n");
> +			ret = -1;
> +			break;
> +		}
> +	}
> +
> +	return ret == 0;
> +}
> +
> +
> +/**
> + * intel_config_init -
> + *
> + * Initialize the configuration framework by attempting to load a
> + * property table and parse the contents into component lists.
> + *
> + * @dev: The drm driver device.
> + */
> +void intel_config_init(struct drm_device *dev)
> +{
> +	struct intel_config_info *info;
> +	struct intel_config_node *new_node, *tmp;
> +	acpi_handle handle;
> +	struct acpi_device *cl, *component;
> +	char *cname;
> +	struct drm_i915_private *dev_priv = dev->dev_private;
> +
> +	/* Load an ACPI table from /lib/firmware.  */
> +	snprintf(i915.cfg_firmware, PATH_MAX, "drm_%s.aml", dev->driver->name);
> +
> +	if (intel_acpi_load_table(dev, i915.cfg_firmware) != 0) {
> +		DRM_ERROR("Failed to load ACPI device property table.\n");
> +		return;
> +	}
> +
> +	DRM_DEBUG_DRIVER("Loaded ACPI configuration for %s\n",
> +			 dev->driver->name);
> +
> +	info = kzalloc(sizeof(*info), GFP_KERNEL);
> +	if (!info) {
> +		DRM_ERROR("Failed to allocate memory for configuration.\n");
> +		return;
> +	}
> +
> +	INIT_LIST_HEAD(&info->base.list);
> +	INIT_LIST_HEAD(&info->crtc_list);
> +	INIT_LIST_HEAD(&info->connector_list);
> +	INIT_LIST_HEAD(&info->plane_list);
> +
> +	handle = ACPI_HANDLE(dev->dev);
> +	if (!handle) {
> +		DRM_DEBUG_DRIVER("No associated ACPI handle.\n");
> +		kfree(info);
> +		return;
> +	} else {
> +		acpi_bus_get_device(handle, &info->base.adev);
> +	}
> +
> +	/*
> +	 * Create a list of one for the top level driver config.
> +	 *
> +	 * We don't really need a full config_info structure for this but
> +	 * it simplifies the handling of driver general config settings
> +	 * as we don't have to have a special case and unique structure
> +	 * just for this.
> +	 */
> +	INIT_LIST_HEAD(&info->base.node);
> +	list_add_tail(&info->base.node, &info->base.list);
> +
> +/* Component sub-device ACPI names */
> +#define i915_COMPONENT_CRTC "CRTC"
> +#define i915_COMPONENT_CONNECTOR "CNCT"
> +#define i915_COMPONENT_PLANE "PLNS"
> +
> +	/* Build lists */
> +	list_for_each_entry(component, &info->base.adev->children, node) {
> +		if (!component)
> +			continue;
> +
> +		cname = acpi_device_bid(component);
> +
> +		list_for_each_entry(cl, &component->children, node) {
> +			new_node = kzalloc(sizeof(*new_node), GFP_KERNEL);
> +			if (!new_node)
> +				goto bail;
> +			new_node->adev = cl;
> +			INIT_LIST_HEAD(&new_node->node);
> +
> +			/* Add to the appropriate list */
> +			if (strcmp(cname, i915_COMPONENT_CRTC) == 0) {
> +				list_add_tail(&new_node->node,
> +					      &info->crtc_list);
> +			} else if (strcmp(cname, i915_COMPONENT_CONNECTOR) == 0) {
> +				list_add_tail(&new_node->node,
> +					      &info->connector_list);
> +			} else if (strcmp(cname, i915_COMPONENT_PLANE) == 0) {
> +				list_add_tail(&new_node->node,
> +					      &info->plane_list);
> +			} else {
> +				/* unknown component, ignore it */
> +				kfree(new_node);
> +			}
> +		}
> +	}
> +
> +	dev_priv->config_info = info;
> +	DRM_DEBUG_DRIVER("i915 Configuration loaded.\n");
> +	return;
> +
> +bail:
> +	DRM_DEBUG_DRIVER("i915 Configuration aborted, insufficient memory.\n");
> +	list_for_each_entry_safe(new_node, tmp, &info->crtc_list, node)
> +		kfree(new_node);
> +	list_for_each_entry_safe(new_node, tmp, &info->connector_list, node)
> +		kfree(new_node);
> +	list_for_each_entry_safe(new_node, tmp, &info->plane_list, node)
> +		kfree(new_node);
> +
> +	kfree(info);
> +	dev_priv->config_info = NULL;
> +}
> +
> +
> +/**
> + * Clean up the configuration infrastructure as the driver is
> + * shuting down.  Don't leak memory!
> + *
> + * @dev: The drm driver device.
> + */
> +void intel_config_shutdown(struct drm_device *dev)
> +{
> +	struct drm_i915_private *dev_priv = dev->dev_private;
> +	acpi_handle handle;
> +	acpi_status status;
> +	struct acpi_device *adev;
> +	struct intel_config_info *info;
> +	struct intel_config_node *n, *tmp;
> +
> +	if (dev_priv->config_info == NULL)
> +		return;
> +
> +	/* Free the component lists.  */
> +	info = dev_priv->config_info;
> +	list_for_each_entry_safe(n, tmp, &info->crtc_list, node)
> +		kfree(n);
> +	list_for_each_entry_safe(n, tmp, &info->connector_list, node)
> +		kfree(n);
> +	list_for_each_entry_safe(n, tmp, &info->plane_list, node)
> +		kfree(n);
> +
> +	/* Unload any dynamically loaded ACPI property table */
> +	handle = ACPI_HANDLE(dev->dev);
> +
> +	DRM_DEBUG_DRIVER("Unloading dynamic i915 Configuration ACPI table.\n");
> +	if (handle) {
> +
> +		acpi_unbind_one(dev->dev);
> +
> +		if (acpi_bus_get_device(handle, &adev))
> +			DRM_ERROR("Failed to get ACPI bus device.\n");
> +		else
> +			acpi_bus_trim(adev);
> +
> +		status = acpi_unload_parent_table(handle);
> +		if (ACPI_FAILURE(status))
> +			DRM_ERROR("Failed to unload the i915 Configuration"
> +				  "ACPI table. %d\n", status);
> +	}
> +}
> +
> +
> +
> +/*
> + * does_name_match
> + *
> + * The various drm components have names assocated with them. To link
> + * a component in the ACPI property tree, use a "special" property
> + * called "name".
> + *
> + * The exception is the general driver properties.  There is no "name"
> + * property associated with those.  We could force one, but that may
> + * be less intuitive than leaving the name empty.
> + *
> + * This function look for a property called "name" and compares the
> + * value of that property with the passed in name parameter.
> + */
> +static bool does_name_match(struct intel_config_node *node, const char *name)
> +{
> +	char *p_name;
> +
> +	/*
> +	 * General driver properties aren't in a section with a "name"
> +	 * property. Thus this should just return true in that case.
> +	 */
> +	if (!name || strlen(name) == 0)
> +		return true;
> +
> +
> +	/* Look up a name property and see if it matches */
> +	if (node_property(node, "name", &p_name)) {
> +		if (strcmp(name, p_name) == 0)
> +			return true;
> +	}
> +
> +	return false;
> +}
> +
> +
> +/*
> + * Map the configuration sub component enum to the cached
> + * sub component list.
> + */
> +static struct list_head *cfg_type_to_list(struct intel_config_info *info,
> +					  enum cfg_type cfg_type)
> +{
> +	switch (cfg_type) {
> +	case CFG_DRV:
> +		return &info->base.list;
> +	case CFG_CRTC:
> +		return &info->crtc_list;
> +	case CFG_CONNECTOR:
> +		return &info->connector_list;
> +	case CFG_PLANE:
> +		return &info->plane_list;
> +	}
> +	return NULL;
> +}
> +
> +/*
> + * Integer property.
> + *
> + * Look up a property and set the value pointer to the property value.
> + *
> + * returns true if the property was found, false if not.
> + */
> +bool intel_config_get_integer(struct drm_i915_private *dev_priv,
> +			      enum cfg_type cfg_type,
> +			      const char *name,
> +			      const char *property,
> +			      uint32_t *value)
> +{
> +	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;
> +
> +	list = cfg_type_to_list(info, cfg_type);
> +	if (!list)
> +		return false;
> +
> +	list_for_each_entry(n, list, node) {
> +		if (does_name_match(n, name)) {
> +			if (node_property(n, property, value))
> +				return true;
> +		}
> +	}
> +
> +	return false;
> +}
> +
> +/*
> + * Look up a drm property name in the configuration store and if
> + * found, return the value.
> + *
> + * A default value is included in the parameter list so that something
> + * reasonable is returned if the lookup fails to find a matching
> + * configuration key.
> + */
> +static uint64_t lookup_property(struct drm_i915_private *dev_priv,
> +				const char *name,
> +				struct drm_property *property,
> +				enum cfg_type cfg_type,
> +				uint64_t dflt)
> +{
> +	struct intel_config_info *info = dev_priv->config_info;
> +	struct drm_property_enum *prop_enum;
> +	const char *value;
> +	uint64_t retval = dflt;
> +	struct intel_config_node *n;
> +	struct list_head *list;
> +
> +	list = cfg_type_to_list(info, cfg_type);
> +	if (!list)
> +		goto out;
> +
> +	list_for_each_entry(n, list, node) {
> +		if (!does_name_match(n, name))
> +			continue;
> +		/*
> +		 * FIXME: This is assuming that the type is string
> +		 * for enun drm properties.  This should be more
> +		 * generic and map drm property types to ACPI property
> +		 * types.
> +		 */
> +		if (!node_property(n, property->name, &value))
> +			continue;
> +
> +		/* Look for a matching enum */
> +		list_for_each_entry(prop_enum, &property->enum_list, head) {
> +			if (strcmp(value, prop_enum->name) == 0) {
> +				retval = prop_enum->value;
> +				goto out;
> +			}
> +		}
> +	}
> +
> +out:
> +	return retval;
> +}
> +
> +/*
> + * Connector properties.
> + *
> + * If a connector drm property has a corresponding configuration property,
> + * use the configuration property value to initialize the drm property.
> + */
> +uint64_t intel_config_init_connector_property(struct drm_connector *connector,
> +					      const char *name,
> +					      struct drm_property *property,
> +					      uint64_t dflt)
> +{
> +	struct drm_i915_private *dev_priv = connector->dev->dev_private;
> +	struct intel_config_info *info = dev_priv->config_info;
> +	uint64_t retval;
> +
> +	if (!info)
> +		goto out;
> +
> +	retval = lookup_property(dev_priv, name, property, CFG_CONNECTOR, dflt);
> +
> +out:
> +	drm_object_attach_property(&connector->base, property, retval);
> +	return retval;
> +}
> +
> +
> +/*
> + * Plane properties.
> + *
> + * If a plane drm property has a corresponding configuration property,
> + * use the configuration property value to initialize the drm property.
> + */
> +uint64_t intel_config_init_plane_property(struct drm_plane *plane,
> +					  const char *name,
> +					  struct drm_property *property,
> +					  uint64_t dflt)
> +{
> +	struct drm_i915_private *dev_priv = plane->dev->dev_private;
> +	struct intel_config_info *info = dev_priv->config_info;
> +	uint64_t retval;
> +
> +	if (!info)
> +		goto out;
> +
> +	retval = lookup_property(dev_priv, name, property, CFG_PLANE, dflt);
> +
> +out:
> +	drm_object_attach_property(&plane->base, property, retval);
> +	return retval;
> +}
> +
> +
> +/*
> + * CRTC properties.
> + *
> + * If a crtc drm property has a corresponding configuration property,
> + * use the configuration property value to initialize the drm property.
> + */
> +uint64_t intel_config_init_crtc_property(struct drm_crtc *crtc,
> +					 const char *name,
> +					 struct drm_property *property,
> +					 uint64_t dflt)
> +{
> +	struct drm_i915_private *dev_priv = crtc->dev->dev_private;
> +	struct intel_config_info *info = dev_priv->config_info;
> +	uint64_t retval;
> +
> +	if (!info)
> +		goto out;
> +
> +	retval = lookup_property(dev_priv, name, property, CFG_CRTC, dflt);
> +
> +out:
> +	drm_object_attach_property(&crtc->base, property, retval);
> +	return retval;
> +}
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 1de8e20..aefd95e 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -1259,4 +1259,32 @@ void intel_plane_destroy_state(struct drm_plane *plane,
>  			       struct drm_plane_state *state);
>  extern const struct drm_plane_helper_funcs intel_plane_helper_funcs;
>  
> +/* intel_config.c */
> +enum cfg_type {
> +	CFG_DRV,
> +	CFG_CRTC,
> +	CFG_CONNECTOR,
> +	CFG_PLANE
> +};
> +
> +void intel_config_init(struct drm_device *dev);
> +void intel_config_shutdown(struct drm_device *dev);
> +bool intel_config_get_integer(struct drm_i915_private *dev_priv,
> +			  enum cfg_type cfg_type,
> +			  const char *name,
> +			  const char *property,
> +			  uint32_t *value);
> +uint64_t intel_config_init_connector_property(struct drm_connector *connector,
> +			  const char *name,
> +			  struct drm_property *property,
> +			  uint64_t dflt);
> +uint64_t intel_config_init_plane_property(struct drm_plane *plane,
> +			  const char *name,
> +			  struct drm_property *property,
> +			  uint64_t dflt);
> +uint64_t intel_config_init_crtc_property(struct drm_crtc *crtc,
> +			  const char *name,
> +			  struct drm_property *property,
> +			  uint64_t dflt);
> +
>  #endif /* __INTEL_DRV_H__ */
> -- 
> 2.1.0
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Paauwe, Bob J Feb. 24, 2015, 5:41 p.m. UTC | #3
On Tue, 24 Feb 2015 17:17:18 +0100
Daniel Vetter <daniel@ffwll.ch> wrote:

> On Thu, Feb 12, 2015 at 03:41:27PM -0800, Bob Paauwe wrote:
> > This adds an init-time configuration framework that parses configuration
> > data from an ACPI property table. The table is assumed to have well
> > defined sub-device property tables that correspond to the various
> > driver components.  Initially the following sub-device tables are
> > defined:
> > 
> > CRTC (CRTC)
> >    The CRTC sub-device table contains additional sub-device tables
> >    where each one corresponds to a CRTC.  Each CRTC sub-device must
> >    include a property called "id" whose value matches the driver's
> >    crtc id. Additional properties for the CRTC are used to configure
> >    the crtc.
> > 
> > Connector (CNCT)
> >    The CNCT sub-device table contains additional sub-device tables
> >    where each one corresponds to a connector. Each of the connector
> >    sub-device tables must include a property called "name" whose value
> >    matches a connector name assigned by the driver (see later patch
> >    for output name function). Additional connector properties can
> >    be set through these tables.
> > 
> > Plane (PLNS)
> >    The PLNS sub-device table contains additional sub-device tables
> >    where each one corresponds to a plane.  [this needs additional work]
> > 
> > In addition, the main device property table for the device may
> > contain configuration information that applies to general driver
> > configuration.
> > 
> > The framework includes a couple of helper functions to access the
> > configuration data.
> > 
> >    intel_config_get_integer() will look up a configuration property
> >    and return the integer value associated with it.
> > 
> >    intel_config_init_<component>_property() will look up a
> >    configuration property and assign the value to a drm
> >    property of the same name.  These functions are used to
> >    initialize drm property instances to specific values.
> > 
> > Signed-off-by: Bob Paauwe <bob.j.paauwe@intel.com>
> 
> Like Jani I'm mostly concerned about maintaining yet another slightly
> different definition of paramters and settings here - we already have (or
> will soon grow) per crtc/plane/connector properties for atomic.

For drm properties, the idea was to expand the existing hard coded
default values with configurable defaults.  For the pre-atomic
connector properties, when the property instance was created, they were
created with a specific default value.  

I haven't looked closely at the atomic property sets yet to see if this
interface also makes sense with them, but that is on my near-term todo
list.  Most of this code had been developed prior to the atomic patches.

> 
> My high-level idea for this was that we reuse the atomic support (once we
> have it) and just take the acpi tables as a slightly different source for
> an atomic update. Since the design of a name/value pair for objects is
> very similar to what we do with the atomic ioctl this hopefully should map
> fairly well.

Yes, I believe it will map easily also, and plan to look at this next. 

> 
> Internally in the driver the only work needed to do would be to add any
> missing properties. All the acpi atomic update code should be fully i915
> agnostic and only use generic atomic update interfaces.

ACPI isn't really driver agnostic since it's only available to IA based
drivers. So I'm not sure I follow your thinking here.  Maybe it will
make more sense to me as I look at the atomic properties. 

I needed to get some fresh eyes looking at this which is why I posted
it. I appreciate the feedback and will be incorporating it into the
next version.

> 
> Would this work or do I overlook something important here.
> -Daniel
> 
> > ---
> >  drivers/gpu/drm/i915/Makefile       |   3 +-
> >  drivers/gpu/drm/i915/i915_dma.c     |   4 +
> >  drivers/gpu/drm/i915/i915_drv.h     |  16 ++
> >  drivers/gpu/drm/i915/i915_params.c  |   6 +
> >  drivers/gpu/drm/i915/intel_config.c | 542 ++++++++++++++++++++++++++++++++++++
> >  drivers/gpu/drm/i915/intel_drv.h    |  28 ++
> >  6 files changed, 598 insertions(+), 1 deletion(-)
> >  create mode 100644 drivers/gpu/drm/i915/intel_config.c
> > 
> > diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile
> > index f025e7f..462de19 100644
> > --- a/drivers/gpu/drm/i915/Makefile
> > +++ b/drivers/gpu/drm/i915/Makefile
> > @@ -12,7 +12,8 @@ i915-y := i915_drv.o \
> >            i915_suspend.o \
> >  	  i915_sysfs.o \
> >  	  intel_pm.o \
> > -	  intel_runtime_pm.o
> > +	  intel_runtime_pm.o \
> > +	  intel_config.o
> >  
> >  i915-$(CONFIG_COMPAT)   += i915_ioc32.o
> >  i915-$(CONFIG_DEBUG_FS) += i915_debugfs.o
> > diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
> > index 5804aa5..9501360 100644
> > --- a/drivers/gpu/drm/i915/i915_dma.c
> > +++ b/drivers/gpu/drm/i915/i915_dma.c
> > @@ -656,6 +656,8 @@ int i915_driver_load(struct drm_device *dev, unsigned long flags)
> >  	dev->dev_private = dev_priv;
> >  	dev_priv->dev = dev;
> >  
> > +	intel_config_init(dev);
> > +
> >  	/* Setup the write-once "constant" device info */
> >  	device_info = (struct intel_device_info *)&dev_priv->info;
> >  	memcpy(device_info, info, sizeof(dev_priv->info));
> > @@ -929,6 +931,8 @@ int i915_driver_unload(struct drm_device *dev)
> >  
> >  	acpi_video_unregister();
> >  
> > +	intel_config_shutdown(dev);
> > +
> >  	if (drm_core_check_feature(dev, DRIVER_MODESET))
> >  		intel_fbdev_fini(dev);
> >  
> > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> > index 2dedd43..165091c 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.h
> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > @@ -1645,6 +1645,20 @@ struct i915_virtual_gpu {
> >  	bool active;
> >  };
> >  
> > +struct intel_config_node {
> > +	struct acpi_device *adev;
> > +	struct list_head node;
> > +	struct list_head list;
> > +};
> > +
> > +struct intel_config_info {
> > +	struct intel_config_node base;
> > +	struct list_head crtc_list;
> > +	struct list_head connector_list;
> > +	struct list_head plane_list;
> > +};
> > +
> > +
> >  struct drm_i915_private {
> >  	struct drm_device *dev;
> >  	struct kmem_cache *slab;
> > @@ -1886,6 +1900,7 @@ struct drm_i915_private {
> >  	u32 long_hpd_port_mask;
> >  	u32 short_hpd_port_mask;
> >  	struct work_struct dig_port_work;
> > +	struct intel_config_info *config_info;
> >  
> >  	/*
> >  	 * if we get a HPD irq from DP and a HPD irq from non-DP
> > @@ -2528,6 +2543,7 @@ struct i915_params {
> >  	int enable_ips;
> >  	int invert_brightness;
> >  	int enable_cmd_parser;
> > +	char cfg_firmware[PATH_MAX];
> >  	/* leave bools at the end to not create holes */
> >  	bool enable_hangcheck;
> >  	bool fastboot;
> > diff --git a/drivers/gpu/drm/i915/i915_params.c b/drivers/gpu/drm/i915/i915_params.c
> > index 44f2262..f92621c 100644
> > --- a/drivers/gpu/drm/i915/i915_params.c
> > +++ b/drivers/gpu/drm/i915/i915_params.c
> > @@ -53,6 +53,7 @@ struct i915_params i915 __read_mostly = {
> >  	.mmio_debug = 0,
> >  	.verbose_state_checks = 1,
> >  	.nuclear_pageflip = 0,
> > +	.cfg_firmware = "",
> >  };
> >  
> >  module_param_named(modeset, i915.modeset, int, 0400);
> > @@ -183,3 +184,8 @@ MODULE_PARM_DESC(verbose_state_checks,
> >  module_param_named_unsafe(nuclear_pageflip, i915.nuclear_pageflip, bool, 0600);
> >  MODULE_PARM_DESC(nuclear_pageflip,
> >  		 "Force atomic modeset functionality; only planes work for now (default: false).");
> > +
> > +module_param_string(cfg_firmware, i915.cfg_firmware, sizeof(i915.cfg_firmware), 0444);
> > +MODULE_PARM_DESC(cfg_firmware,
> > +		 "Load configuration firmware from built-in data or /lib/firmware. ");
> > +
> > diff --git a/drivers/gpu/drm/i915/intel_config.c b/drivers/gpu/drm/i915/intel_config.c
> > new file mode 100644
> > index 0000000..cf7da93
> > --- /dev/null
> > +++ b/drivers/gpu/drm/i915/intel_config.c
> > @@ -0,0 +1,542 @@
> > +/*
> > + * i915 configuration via ACPI device properties.
> > + *
> > + * Copyright (C) 2014, 2015 Intel Corporation
> > + *
> > + * Permission to use, copy, modify, distribute, and sell this software and its
> > + * documentation for any purpose is hereby granted without fee, provided that
> > + * the above copyright notice appear in all copies and that both that copyright
> > + * notice and this permission notice appear in supporting documentation, and
> > + * that the name of the copyright holders not be used in advertising or
> > + * publicity pertaining to distribution of the software without specific,
> > + * written prior permission.  The copyright holders make no representations
> > + * about the suitability of this software for any purpose.  It is provided "as
> > + * is" without express or implied warranty.
> > + *
> > + * THE COPYRIGHT HOLDERS DISCLAIM ALL WARRANTIES WITH REGARD TO THIS SOFTWARE,
> > + * INCLUDING ALL IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS, IN NO
> > + * EVENT SHALL THE COPYRIGHT HOLDERS BE LIABLE FOR ANY SPECIAL, INDIRECT OR
> > + * CONSEQUENTIAL DAMAGES OR ANY DAMAGES WHATSOEVER RESULTING FROM LOSS OF USE,
> > + * DATA OR PROFITS, WHETHER IN AN ACTION OF CONTRACT, NEGLIGENCE OR OTHER
> > + * TORTIOUS ACTION, ARISING OUT OF OR IN CONNECTION WITH THE USE OR PERFORMANCE
> > + * OF THIS SOFTWARE.
> > + */
> > +#include <linux/acpi.h>
> > +#include <linux/init.h>
> > +#include <linux/module.h>
> > +#include <linux/firmware.h>
> > +#include <linux/ctype.h>
> > +#include <acpi/acpi_bus.h>
> > +#include "intel_drv.h"
> > +#include <drm/i915_drm.h>
> > +#include "i915_drv.h"
> > +#include "i915_trace.h"
> > +#include <drm/drmP.h>
> > +#include <drm/drm_crtc.h>
> > +#include <drm/drm_crtc_helper.h>
> > +
> > +
> > +#define i915_ACPI_PRP_ROOT "\\_SB.PRP.GFX0"
> > +
> > +/*
> > + * Load an ACPI property table into the ACPI subsystem.
> > + *
> > + * First, verify that a table isn't already loaded.  The table may
> > + * be part of the ACPI firmware and thus loaded by the ACPI sub-system
> > + * during boot.  It is also possible for ACPI sub-system to load tables
> > + * to override those supplied by the firmware.
> > + *
> > + * If a property table for the GFX device has not been loaded, attempt
> > + * to load one from /lib/firmware here.  The name will default to
> > + * drm_i915.aml but the name be overridden by the cfg_firmware module
> > + * parameter.
> > + *
> > + * The order of precidence for table loading is:
> > + *   - dyanamic table loaded by ACPI driver
> > + *   - table built into firmware
> > + *   - dynamic table loaded based on driver name or module parameter
> > + *
> > + * If the table is loaded by the driver, it will be unloaded when the
> > + * driver module is unloaded.  Tables that are part of the firmware or
> > + * overridden by the ACPI driver are not unloaded and cannot be replaced
> > + * by tables loaded by the driver.
> > + */
> > +static int intel_acpi_load_table(struct drm_device *dev, char *firmware)
> > +{
> > +	struct acpi_table_header *table;
> > +	acpi_status status;
> > +	acpi_handle handle;
> > +	struct acpi_device *acpi_dev = NULL;
> > +	const struct firmware *fw;
> > +	int ret = 0;
> > +
> > +	/* make sure the table isn't already loaded before loading it */
> > +	status = acpi_get_handle(ACPI_ROOT_OBJECT, i915_ACPI_PRP_ROOT, &handle);
> > +	if (ACPI_FAILURE(status)) {
> > +
> > +		/* Try to dynamically load a table.... */
> > +		DRM_DEBUG_DRIVER("Requesting configuration table: %s\n",
> > +				 firmware);
> > +		ret = request_firmware(&fw, firmware, dev->dev);
> > +		if (ret != 0) {
> > +			DRM_ERROR("Failed to find ACPI table %s: %d\n",
> > +				  firmware, ret);
> > +			fw = NULL;
> > +			goto bad_table;
> > +		} else {
> > +			table = (struct acpi_table_header *)fw->data;
> > +		}
> > +
> > +		/* Load the table into the acpi device. */
> > +		status = acpi_load_table(table);
> > +		if (ACPI_FAILURE(status))
> > +			goto bad_table;
> > +
> > +		add_taint(TAINT_OVERRIDDEN_ACPI_TABLE, true);
> > +
> > +		/* Get a handle and scan the bus */
> > +		status = acpi_get_handle(ACPI_ROOT_OBJECT, i915_ACPI_PRP_ROOT,
> > +					 &handle);
> > +		acpi_bus_scan(handle);
> > +
> > +		if (acpi_bus_get_device(handle, &acpi_dev))
> > +			goto bad_table;
> > +
> > +		acpi_bind_one(dev->dev, acpi_dev);
> > +
> > +	} else {
> > +		DRM_DEBUG_DRIVER("ACPI Property table previously loaded for _SB.PRP.GFX0\n");
> > +	}
> > +
> > +	return 0;
> > +
> > +bad_table:
> > +	release_firmware(fw);
> > +	return -ENODEV;
> > +}
> > +
> > +/*
> > + * Use ACPI methods to get the property.
> > + *
> > + * This isn't using the generic device_property_read because that
> > + * can only access properties associated with the actual device. It
> > + * doesn't understand our sub-component property tree.
> > + */
> > +static bool node_property(struct intel_config_node *n,
> > +			  const char *prop,
> > +			  void *value)
> > +{
> > +	int ret = 0;
> > +	const union acpi_object *obj;
> > +
> > +	ret = acpi_dev_get_property(n->adev, prop, ACPI_TYPE_ANY, &obj);
> > +	if (ret == -ENODATA) {
> > +		/*
> > +		 * This isn't really a failure, it's ok if the property
> > +		 * doesn't exist. The message is for debug purposes only.
> > +		 */
> > +		DRM_DEBUG_DRIVER("Property \"%s\" not found in %s\n",
> > +				 prop, acpi_device_bid(n->adev));
> > +	} else if (ret == -EINVAL) {
> > +		DRM_ERROR("Invalid acpi device or property name.\n");
> > +	} else if (ret) {
> > +		/* This is a failure. */
> > +		DRM_ERROR("Property request failed for %s: %d\n", prop, ret);
> > +	} else {
> > +		switch (obj->type) {
> > +		case ACPI_TYPE_INTEGER:
> > +			*(u32 *)value = obj->integer.value;
> > +			break;
> > +		case ACPI_TYPE_STRING:
> > +			*(char **)value = obj->string.pointer;
> > +			break;
> > +		default:
> > +			DRM_ERROR("Unsupported property type, only support integer and string.\n");
> > +			ret = -1;
> > +			break;
> > +		}
> > +	}
> > +
> > +	return ret == 0;
> > +}
> > +
> > +
> > +/**
> > + * intel_config_init -
> > + *
> > + * Initialize the configuration framework by attempting to load a
> > + * property table and parse the contents into component lists.
> > + *
> > + * @dev: The drm driver device.
> > + */
> > +void intel_config_init(struct drm_device *dev)
> > +{
> > +	struct intel_config_info *info;
> > +	struct intel_config_node *new_node, *tmp;
> > +	acpi_handle handle;
> > +	struct acpi_device *cl, *component;
> > +	char *cname;
> > +	struct drm_i915_private *dev_priv = dev->dev_private;
> > +
> > +	/* Load an ACPI table from /lib/firmware.  */
> > +	snprintf(i915.cfg_firmware, PATH_MAX, "drm_%s.aml", dev->driver->name);
> > +
> > +	if (intel_acpi_load_table(dev, i915.cfg_firmware) != 0) {
> > +		DRM_ERROR("Failed to load ACPI device property table.\n");
> > +		return;
> > +	}
> > +
> > +	DRM_DEBUG_DRIVER("Loaded ACPI configuration for %s\n",
> > +			 dev->driver->name);
> > +
> > +	info = kzalloc(sizeof(*info), GFP_KERNEL);
> > +	if (!info) {
> > +		DRM_ERROR("Failed to allocate memory for configuration.\n");
> > +		return;
> > +	}
> > +
> > +	INIT_LIST_HEAD(&info->base.list);
> > +	INIT_LIST_HEAD(&info->crtc_list);
> > +	INIT_LIST_HEAD(&info->connector_list);
> > +	INIT_LIST_HEAD(&info->plane_list);
> > +
> > +	handle = ACPI_HANDLE(dev->dev);
> > +	if (!handle) {
> > +		DRM_DEBUG_DRIVER("No associated ACPI handle.\n");
> > +		kfree(info);
> > +		return;
> > +	} else {
> > +		acpi_bus_get_device(handle, &info->base.adev);
> > +	}
> > +
> > +	/*
> > +	 * Create a list of one for the top level driver config.
> > +	 *
> > +	 * We don't really need a full config_info structure for this but
> > +	 * it simplifies the handling of driver general config settings
> > +	 * as we don't have to have a special case and unique structure
> > +	 * just for this.
> > +	 */
> > +	INIT_LIST_HEAD(&info->base.node);
> > +	list_add_tail(&info->base.node, &info->base.list);
> > +
> > +/* Component sub-device ACPI names */
> > +#define i915_COMPONENT_CRTC "CRTC"
> > +#define i915_COMPONENT_CONNECTOR "CNCT"
> > +#define i915_COMPONENT_PLANE "PLNS"
> > +
> > +	/* Build lists */
> > +	list_for_each_entry(component, &info->base.adev->children, node) {
> > +		if (!component)
> > +			continue;
> > +
> > +		cname = acpi_device_bid(component);
> > +
> > +		list_for_each_entry(cl, &component->children, node) {
> > +			new_node = kzalloc(sizeof(*new_node), GFP_KERNEL);
> > +			if (!new_node)
> > +				goto bail;
> > +			new_node->adev = cl;
> > +			INIT_LIST_HEAD(&new_node->node);
> > +
> > +			/* Add to the appropriate list */
> > +			if (strcmp(cname, i915_COMPONENT_CRTC) == 0) {
> > +				list_add_tail(&new_node->node,
> > +					      &info->crtc_list);
> > +			} else if (strcmp(cname, i915_COMPONENT_CONNECTOR) == 0) {
> > +				list_add_tail(&new_node->node,
> > +					      &info->connector_list);
> > +			} else if (strcmp(cname, i915_COMPONENT_PLANE) == 0) {
> > +				list_add_tail(&new_node->node,
> > +					      &info->plane_list);
> > +			} else {
> > +				/* unknown component, ignore it */
> > +				kfree(new_node);
> > +			}
> > +		}
> > +	}
> > +
> > +	dev_priv->config_info = info;
> > +	DRM_DEBUG_DRIVER("i915 Configuration loaded.\n");
> > +	return;
> > +
> > +bail:
> > +	DRM_DEBUG_DRIVER("i915 Configuration aborted, insufficient memory.\n");
> > +	list_for_each_entry_safe(new_node, tmp, &info->crtc_list, node)
> > +		kfree(new_node);
> > +	list_for_each_entry_safe(new_node, tmp, &info->connector_list, node)
> > +		kfree(new_node);
> > +	list_for_each_entry_safe(new_node, tmp, &info->plane_list, node)
> > +		kfree(new_node);
> > +
> > +	kfree(info);
> > +	dev_priv->config_info = NULL;
> > +}
> > +
> > +
> > +/**
> > + * Clean up the configuration infrastructure as the driver is
> > + * shuting down.  Don't leak memory!
> > + *
> > + * @dev: The drm driver device.
> > + */
> > +void intel_config_shutdown(struct drm_device *dev)
> > +{
> > +	struct drm_i915_private *dev_priv = dev->dev_private;
> > +	acpi_handle handle;
> > +	acpi_status status;
> > +	struct acpi_device *adev;
> > +	struct intel_config_info *info;
> > +	struct intel_config_node *n, *tmp;
> > +
> > +	if (dev_priv->config_info == NULL)
> > +		return;
> > +
> > +	/* Free the component lists.  */
> > +	info = dev_priv->config_info;
> > +	list_for_each_entry_safe(n, tmp, &info->crtc_list, node)
> > +		kfree(n);
> > +	list_for_each_entry_safe(n, tmp, &info->connector_list, node)
> > +		kfree(n);
> > +	list_for_each_entry_safe(n, tmp, &info->plane_list, node)
> > +		kfree(n);
> > +
> > +	/* Unload any dynamically loaded ACPI property table */
> > +	handle = ACPI_HANDLE(dev->dev);
> > +
> > +	DRM_DEBUG_DRIVER("Unloading dynamic i915 Configuration ACPI table.\n");
> > +	if (handle) {
> > +
> > +		acpi_unbind_one(dev->dev);
> > +
> > +		if (acpi_bus_get_device(handle, &adev))
> > +			DRM_ERROR("Failed to get ACPI bus device.\n");
> > +		else
> > +			acpi_bus_trim(adev);
> > +
> > +		status = acpi_unload_parent_table(handle);
> > +		if (ACPI_FAILURE(status))
> > +			DRM_ERROR("Failed to unload the i915 Configuration"
> > +				  "ACPI table. %d\n", status);
> > +	}
> > +}
> > +
> > +
> > +
> > +/*
> > + * does_name_match
> > + *
> > + * The various drm components have names assocated with them. To link
> > + * a component in the ACPI property tree, use a "special" property
> > + * called "name".
> > + *
> > + * The exception is the general driver properties.  There is no "name"
> > + * property associated with those.  We could force one, but that may
> > + * be less intuitive than leaving the name empty.
> > + *
> > + * This function look for a property called "name" and compares the
> > + * value of that property with the passed in name parameter.
> > + */
> > +static bool does_name_match(struct intel_config_node *node, const char *name)
> > +{
> > +	char *p_name;
> > +
> > +	/*
> > +	 * General driver properties aren't in a section with a "name"
> > +	 * property. Thus this should just return true in that case.
> > +	 */
> > +	if (!name || strlen(name) == 0)
> > +		return true;
> > +
> > +
> > +	/* Look up a name property and see if it matches */
> > +	if (node_property(node, "name", &p_name)) {
> > +		if (strcmp(name, p_name) == 0)
> > +			return true;
> > +	}
> > +
> > +	return false;
> > +}
> > +
> > +
> > +/*
> > + * Map the configuration sub component enum to the cached
> > + * sub component list.
> > + */
> > +static struct list_head *cfg_type_to_list(struct intel_config_info *info,
> > +					  enum cfg_type cfg_type)
> > +{
> > +	switch (cfg_type) {
> > +	case CFG_DRV:
> > +		return &info->base.list;
> > +	case CFG_CRTC:
> > +		return &info->crtc_list;
> > +	case CFG_CONNECTOR:
> > +		return &info->connector_list;
> > +	case CFG_PLANE:
> > +		return &info->plane_list;
> > +	}
> > +	return NULL;
> > +}
> > +
> > +/*
> > + * Integer property.
> > + *
> > + * Look up a property and set the value pointer to the property value.
> > + *
> > + * returns true if the property was found, false if not.
> > + */
> > +bool intel_config_get_integer(struct drm_i915_private *dev_priv,
> > +			      enum cfg_type cfg_type,
> > +			      const char *name,
> > +			      const char *property,
> > +			      uint32_t *value)
> > +{
> > +	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;
> > +
> > +	list = cfg_type_to_list(info, cfg_type);
> > +	if (!list)
> > +		return false;
> > +
> > +	list_for_each_entry(n, list, node) {
> > +		if (does_name_match(n, name)) {
> > +			if (node_property(n, property, value))
> > +				return true;
> > +		}
> > +	}
> > +
> > +	return false;
> > +}
> > +
> > +/*
> > + * Look up a drm property name in the configuration store and if
> > + * found, return the value.
> > + *
> > + * A default value is included in the parameter list so that something
> > + * reasonable is returned if the lookup fails to find a matching
> > + * configuration key.
> > + */
> > +static uint64_t lookup_property(struct drm_i915_private *dev_priv,
> > +				const char *name,
> > +				struct drm_property *property,
> > +				enum cfg_type cfg_type,
> > +				uint64_t dflt)
> > +{
> > +	struct intel_config_info *info = dev_priv->config_info;
> > +	struct drm_property_enum *prop_enum;
> > +	const char *value;
> > +	uint64_t retval = dflt;
> > +	struct intel_config_node *n;
> > +	struct list_head *list;
> > +
> > +	list = cfg_type_to_list(info, cfg_type);
> > +	if (!list)
> > +		goto out;
> > +
> > +	list_for_each_entry(n, list, node) {
> > +		if (!does_name_match(n, name))
> > +			continue;
> > +		/*
> > +		 * FIXME: This is assuming that the type is string
> > +		 * for enun drm properties.  This should be more
> > +		 * generic and map drm property types to ACPI property
> > +		 * types.
> > +		 */
> > +		if (!node_property(n, property->name, &value))
> > +			continue;
> > +
> > +		/* Look for a matching enum */
> > +		list_for_each_entry(prop_enum, &property->enum_list, head) {
> > +			if (strcmp(value, prop_enum->name) == 0) {
> > +				retval = prop_enum->value;
> > +				goto out;
> > +			}
> > +		}
> > +	}
> > +
> > +out:
> > +	return retval;
> > +}
> > +
> > +/*
> > + * Connector properties.
> > + *
> > + * If a connector drm property has a corresponding configuration property,
> > + * use the configuration property value to initialize the drm property.
> > + */
> > +uint64_t intel_config_init_connector_property(struct drm_connector *connector,
> > +					      const char *name,
> > +					      struct drm_property *property,
> > +					      uint64_t dflt)
> > +{
> > +	struct drm_i915_private *dev_priv = connector->dev->dev_private;
> > +	struct intel_config_info *info = dev_priv->config_info;
> > +	uint64_t retval;
> > +
> > +	if (!info)
> > +		goto out;
> > +
> > +	retval = lookup_property(dev_priv, name, property, CFG_CONNECTOR, dflt);
> > +
> > +out:
> > +	drm_object_attach_property(&connector->base, property, retval);
> > +	return retval;
> > +}
> > +
> > +
> > +/*
> > + * Plane properties.
> > + *
> > + * If a plane drm property has a corresponding configuration property,
> > + * use the configuration property value to initialize the drm property.
> > + */
> > +uint64_t intel_config_init_plane_property(struct drm_plane *plane,
> > +					  const char *name,
> > +					  struct drm_property *property,
> > +					  uint64_t dflt)
> > +{
> > +	struct drm_i915_private *dev_priv = plane->dev->dev_private;
> > +	struct intel_config_info *info = dev_priv->config_info;
> > +	uint64_t retval;
> > +
> > +	if (!info)
> > +		goto out;
> > +
> > +	retval = lookup_property(dev_priv, name, property, CFG_PLANE, dflt);
> > +
> > +out:
> > +	drm_object_attach_property(&plane->base, property, retval);
> > +	return retval;
> > +}
> > +
> > +
> > +/*
> > + * CRTC properties.
> > + *
> > + * If a crtc drm property has a corresponding configuration property,
> > + * use the configuration property value to initialize the drm property.
> > + */
> > +uint64_t intel_config_init_crtc_property(struct drm_crtc *crtc,
> > +					 const char *name,
> > +					 struct drm_property *property,
> > +					 uint64_t dflt)
> > +{
> > +	struct drm_i915_private *dev_priv = crtc->dev->dev_private;
> > +	struct intel_config_info *info = dev_priv->config_info;
> > +	uint64_t retval;
> > +
> > +	if (!info)
> > +		goto out;
> > +
> > +	retval = lookup_property(dev_priv, name, property, CFG_CRTC, dflt);
> > +
> > +out:
> > +	drm_object_attach_property(&crtc->base, property, retval);
> > +	return retval;
> > +}
> > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> > index 1de8e20..aefd95e 100644
> > --- a/drivers/gpu/drm/i915/intel_drv.h
> > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > @@ -1259,4 +1259,32 @@ void intel_plane_destroy_state(struct drm_plane *plane,
> >  			       struct drm_plane_state *state);
> >  extern const struct drm_plane_helper_funcs intel_plane_helper_funcs;
> >  
> > +/* intel_config.c */
> > +enum cfg_type {
> > +	CFG_DRV,
> > +	CFG_CRTC,
> > +	CFG_CONNECTOR,
> > +	CFG_PLANE
> > +};
> > +
> > +void intel_config_init(struct drm_device *dev);
> > +void intel_config_shutdown(struct drm_device *dev);
> > +bool intel_config_get_integer(struct drm_i915_private *dev_priv,
> > +			  enum cfg_type cfg_type,
> > +			  const char *name,
> > +			  const char *property,
> > +			  uint32_t *value);
> > +uint64_t intel_config_init_connector_property(struct drm_connector *connector,
> > +			  const char *name,
> > +			  struct drm_property *property,
> > +			  uint64_t dflt);
> > +uint64_t intel_config_init_plane_property(struct drm_plane *plane,
> > +			  const char *name,
> > +			  struct drm_property *property,
> > +			  uint64_t dflt);
> > +uint64_t intel_config_init_crtc_property(struct drm_crtc *crtc,
> > +			  const char *name,
> > +			  struct drm_property *property,
> > +			  uint64_t dflt);
> > +
> >  #endif /* __INTEL_DRV_H__ */
> > -- 
> > 2.1.0
> > 
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
Daniel Vetter Feb. 24, 2015, 8:47 p.m. UTC | #4
On Tue, Feb 24, 2015 at 09:41:41AM -0800, Bob Paauwe wrote:
> On Tue, 24 Feb 2015 17:17:18 +0100
> Daniel Vetter <daniel@ffwll.ch> wrote:
> > Internally in the driver the only work needed to do would be to add any
> > missing properties. All the acpi atomic update code should be fully i915
> > agnostic and only use generic atomic update interfaces.
> 
> ACPI isn't really driver agnostic since it's only available to IA based
> drivers. So I'm not sure I follow your thinking here.  Maybe it will
> make more sense to me as I look at the atomic properties. 

Yeah I don't expect anyone else but i915 to use this, but reusing the
atomic interface (which is already abi towards userspace) means we don't
have another slightly different interface where abi compatibility matters.
Hence why I want to reuse the atomic interface.

ARM ofc has the cross-vendor dt, so there even code reuse would be
possible. And there's still rumours that at least some arm64 platforms
will only ship with acpi, so maybe there's even some room to share that
code. But really my main goal is to not create another ABI layer but
instead try to fully reuse the atomic one by using a mechanical mapping
for acpi.

> I needed to get some fresh eyes looking at this which is why I posted
> it. I appreciate the feedback and will be incorporating it into the
> next version.

Well I haven't looked at it in details. And I dont think anyone but your
group has a need for this, so not sure who could do a more in-depth review
if you want that.

Cheers, Daniel
diff mbox

Patch

diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile
index f025e7f..462de19 100644
--- a/drivers/gpu/drm/i915/Makefile
+++ b/drivers/gpu/drm/i915/Makefile
@@ -12,7 +12,8 @@  i915-y := i915_drv.o \
           i915_suspend.o \
 	  i915_sysfs.o \
 	  intel_pm.o \
-	  intel_runtime_pm.o
+	  intel_runtime_pm.o \
+	  intel_config.o
 
 i915-$(CONFIG_COMPAT)   += i915_ioc32.o
 i915-$(CONFIG_DEBUG_FS) += i915_debugfs.o
diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
index 5804aa5..9501360 100644
--- a/drivers/gpu/drm/i915/i915_dma.c
+++ b/drivers/gpu/drm/i915/i915_dma.c
@@ -656,6 +656,8 @@  int i915_driver_load(struct drm_device *dev, unsigned long flags)
 	dev->dev_private = dev_priv;
 	dev_priv->dev = dev;
 
+	intel_config_init(dev);
+
 	/* Setup the write-once "constant" device info */
 	device_info = (struct intel_device_info *)&dev_priv->info;
 	memcpy(device_info, info, sizeof(dev_priv->info));
@@ -929,6 +931,8 @@  int i915_driver_unload(struct drm_device *dev)
 
 	acpi_video_unregister();
 
+	intel_config_shutdown(dev);
+
 	if (drm_core_check_feature(dev, DRIVER_MODESET))
 		intel_fbdev_fini(dev);
 
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 2dedd43..165091c 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1645,6 +1645,20 @@  struct i915_virtual_gpu {
 	bool active;
 };
 
+struct intel_config_node {
+	struct acpi_device *adev;
+	struct list_head node;
+	struct list_head list;
+};
+
+struct intel_config_info {
+	struct intel_config_node base;
+	struct list_head crtc_list;
+	struct list_head connector_list;
+	struct list_head plane_list;
+};
+
+
 struct drm_i915_private {
 	struct drm_device *dev;
 	struct kmem_cache *slab;
@@ -1886,6 +1900,7 @@  struct drm_i915_private {
 	u32 long_hpd_port_mask;
 	u32 short_hpd_port_mask;
 	struct work_struct dig_port_work;
+	struct intel_config_info *config_info;
 
 	/*
 	 * if we get a HPD irq from DP and a HPD irq from non-DP
@@ -2528,6 +2543,7 @@  struct i915_params {
 	int enable_ips;
 	int invert_brightness;
 	int enable_cmd_parser;
+	char cfg_firmware[PATH_MAX];
 	/* leave bools at the end to not create holes */
 	bool enable_hangcheck;
 	bool fastboot;
diff --git a/drivers/gpu/drm/i915/i915_params.c b/drivers/gpu/drm/i915/i915_params.c
index 44f2262..f92621c 100644
--- a/drivers/gpu/drm/i915/i915_params.c
+++ b/drivers/gpu/drm/i915/i915_params.c
@@ -53,6 +53,7 @@  struct i915_params i915 __read_mostly = {
 	.mmio_debug = 0,
 	.verbose_state_checks = 1,
 	.nuclear_pageflip = 0,
+	.cfg_firmware = "",
 };
 
 module_param_named(modeset, i915.modeset, int, 0400);
@@ -183,3 +184,8 @@  MODULE_PARM_DESC(verbose_state_checks,
 module_param_named_unsafe(nuclear_pageflip, i915.nuclear_pageflip, bool, 0600);
 MODULE_PARM_DESC(nuclear_pageflip,
 		 "Force atomic modeset functionality; only planes work for now (default: false).");
+
+module_param_string(cfg_firmware, i915.cfg_firmware, sizeof(i915.cfg_firmware), 0444);
+MODULE_PARM_DESC(cfg_firmware,
+		 "Load configuration firmware from built-in data or /lib/firmware. ");
+
diff --git a/drivers/gpu/drm/i915/intel_config.c b/drivers/gpu/drm/i915/intel_config.c
new file mode 100644
index 0000000..cf7da93
--- /dev/null
+++ b/drivers/gpu/drm/i915/intel_config.c
@@ -0,0 +1,542 @@ 
+/*
+ * i915 configuration via ACPI device properties.
+ *
+ * Copyright (C) 2014, 2015 Intel Corporation
+ *
+ * Permission to use, copy, modify, distribute, and sell this software and its
+ * documentation for any purpose is hereby granted without fee, provided that
+ * the above copyright notice appear in all copies and that both that copyright
+ * notice and this permission notice appear in supporting documentation, and
+ * that the name of the copyright holders not be used in advertising or
+ * publicity pertaining to distribution of the software without specific,
+ * written prior permission.  The copyright holders make no representations
+ * about the suitability of this software for any purpose.  It is provided "as
+ * is" without express or implied warranty.
+ *
+ * THE COPYRIGHT HOLDERS DISCLAIM ALL WARRANTIES WITH REGARD TO THIS SOFTWARE,
+ * INCLUDING ALL IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS, IN NO
+ * EVENT SHALL THE COPYRIGHT HOLDERS BE LIABLE FOR ANY SPECIAL, INDIRECT OR
+ * CONSEQUENTIAL DAMAGES OR ANY DAMAGES WHATSOEVER RESULTING FROM LOSS OF USE,
+ * DATA OR PROFITS, WHETHER IN AN ACTION OF CONTRACT, NEGLIGENCE OR OTHER
+ * TORTIOUS ACTION, ARISING OUT OF OR IN CONNECTION WITH THE USE OR PERFORMANCE
+ * OF THIS SOFTWARE.
+ */
+#include <linux/acpi.h>
+#include <linux/init.h>
+#include <linux/module.h>
+#include <linux/firmware.h>
+#include <linux/ctype.h>
+#include <acpi/acpi_bus.h>
+#include "intel_drv.h"
+#include <drm/i915_drm.h>
+#include "i915_drv.h"
+#include "i915_trace.h"
+#include <drm/drmP.h>
+#include <drm/drm_crtc.h>
+#include <drm/drm_crtc_helper.h>
+
+
+#define i915_ACPI_PRP_ROOT "\\_SB.PRP.GFX0"
+
+/*
+ * Load an ACPI property table into the ACPI subsystem.
+ *
+ * First, verify that a table isn't already loaded.  The table may
+ * be part of the ACPI firmware and thus loaded by the ACPI sub-system
+ * during boot.  It is also possible for ACPI sub-system to load tables
+ * to override those supplied by the firmware.
+ *
+ * If a property table for the GFX device has not been loaded, attempt
+ * to load one from /lib/firmware here.  The name will default to
+ * drm_i915.aml but the name be overridden by the cfg_firmware module
+ * parameter.
+ *
+ * The order of precidence for table loading is:
+ *   - dyanamic table loaded by ACPI driver
+ *   - table built into firmware
+ *   - dynamic table loaded based on driver name or module parameter
+ *
+ * If the table is loaded by the driver, it will be unloaded when the
+ * driver module is unloaded.  Tables that are part of the firmware or
+ * overridden by the ACPI driver are not unloaded and cannot be replaced
+ * by tables loaded by the driver.
+ */
+static int intel_acpi_load_table(struct drm_device *dev, char *firmware)
+{
+	struct acpi_table_header *table;
+	acpi_status status;
+	acpi_handle handle;
+	struct acpi_device *acpi_dev = NULL;
+	const struct firmware *fw;
+	int ret = 0;
+
+	/* make sure the table isn't already loaded before loading it */
+	status = acpi_get_handle(ACPI_ROOT_OBJECT, i915_ACPI_PRP_ROOT, &handle);
+	if (ACPI_FAILURE(status)) {
+
+		/* Try to dynamically load a table.... */
+		DRM_DEBUG_DRIVER("Requesting configuration table: %s\n",
+				 firmware);
+		ret = request_firmware(&fw, firmware, dev->dev);
+		if (ret != 0) {
+			DRM_ERROR("Failed to find ACPI table %s: %d\n",
+				  firmware, ret);
+			fw = NULL;
+			goto bad_table;
+		} else {
+			table = (struct acpi_table_header *)fw->data;
+		}
+
+		/* Load the table into the acpi device. */
+		status = acpi_load_table(table);
+		if (ACPI_FAILURE(status))
+			goto bad_table;
+
+		add_taint(TAINT_OVERRIDDEN_ACPI_TABLE, true);
+
+		/* Get a handle and scan the bus */
+		status = acpi_get_handle(ACPI_ROOT_OBJECT, i915_ACPI_PRP_ROOT,
+					 &handle);
+		acpi_bus_scan(handle);
+
+		if (acpi_bus_get_device(handle, &acpi_dev))
+			goto bad_table;
+
+		acpi_bind_one(dev->dev, acpi_dev);
+
+	} else {
+		DRM_DEBUG_DRIVER("ACPI Property table previously loaded for _SB.PRP.GFX0\n");
+	}
+
+	return 0;
+
+bad_table:
+	release_firmware(fw);
+	return -ENODEV;
+}
+
+/*
+ * Use ACPI methods to get the property.
+ *
+ * This isn't using the generic device_property_read because that
+ * can only access properties associated with the actual device. It
+ * doesn't understand our sub-component property tree.
+ */
+static bool node_property(struct intel_config_node *n,
+			  const char *prop,
+			  void *value)
+{
+	int ret = 0;
+	const union acpi_object *obj;
+
+	ret = acpi_dev_get_property(n->adev, prop, ACPI_TYPE_ANY, &obj);
+	if (ret == -ENODATA) {
+		/*
+		 * This isn't really a failure, it's ok if the property
+		 * doesn't exist. The message is for debug purposes only.
+		 */
+		DRM_DEBUG_DRIVER("Property \"%s\" not found in %s\n",
+				 prop, acpi_device_bid(n->adev));
+	} else if (ret == -EINVAL) {
+		DRM_ERROR("Invalid acpi device or property name.\n");
+	} else if (ret) {
+		/* This is a failure. */
+		DRM_ERROR("Property request failed for %s: %d\n", prop, ret);
+	} else {
+		switch (obj->type) {
+		case ACPI_TYPE_INTEGER:
+			*(u32 *)value = obj->integer.value;
+			break;
+		case ACPI_TYPE_STRING:
+			*(char **)value = obj->string.pointer;
+			break;
+		default:
+			DRM_ERROR("Unsupported property type, only support integer and string.\n");
+			ret = -1;
+			break;
+		}
+	}
+
+	return ret == 0;
+}
+
+
+/**
+ * intel_config_init -
+ *
+ * Initialize the configuration framework by attempting to load a
+ * property table and parse the contents into component lists.
+ *
+ * @dev: The drm driver device.
+ */
+void intel_config_init(struct drm_device *dev)
+{
+	struct intel_config_info *info;
+	struct intel_config_node *new_node, *tmp;
+	acpi_handle handle;
+	struct acpi_device *cl, *component;
+	char *cname;
+	struct drm_i915_private *dev_priv = dev->dev_private;
+
+	/* Load an ACPI table from /lib/firmware.  */
+	snprintf(i915.cfg_firmware, PATH_MAX, "drm_%s.aml", dev->driver->name);
+
+	if (intel_acpi_load_table(dev, i915.cfg_firmware) != 0) {
+		DRM_ERROR("Failed to load ACPI device property table.\n");
+		return;
+	}
+
+	DRM_DEBUG_DRIVER("Loaded ACPI configuration for %s\n",
+			 dev->driver->name);
+
+	info = kzalloc(sizeof(*info), GFP_KERNEL);
+	if (!info) {
+		DRM_ERROR("Failed to allocate memory for configuration.\n");
+		return;
+	}
+
+	INIT_LIST_HEAD(&info->base.list);
+	INIT_LIST_HEAD(&info->crtc_list);
+	INIT_LIST_HEAD(&info->connector_list);
+	INIT_LIST_HEAD(&info->plane_list);
+
+	handle = ACPI_HANDLE(dev->dev);
+	if (!handle) {
+		DRM_DEBUG_DRIVER("No associated ACPI handle.\n");
+		kfree(info);
+		return;
+	} else {
+		acpi_bus_get_device(handle, &info->base.adev);
+	}
+
+	/*
+	 * Create a list of one for the top level driver config.
+	 *
+	 * We don't really need a full config_info structure for this but
+	 * it simplifies the handling of driver general config settings
+	 * as we don't have to have a special case and unique structure
+	 * just for this.
+	 */
+	INIT_LIST_HEAD(&info->base.node);
+	list_add_tail(&info->base.node, &info->base.list);
+
+/* Component sub-device ACPI names */
+#define i915_COMPONENT_CRTC "CRTC"
+#define i915_COMPONENT_CONNECTOR "CNCT"
+#define i915_COMPONENT_PLANE "PLNS"
+
+	/* Build lists */
+	list_for_each_entry(component, &info->base.adev->children, node) {
+		if (!component)
+			continue;
+
+		cname = acpi_device_bid(component);
+
+		list_for_each_entry(cl, &component->children, node) {
+			new_node = kzalloc(sizeof(*new_node), GFP_KERNEL);
+			if (!new_node)
+				goto bail;
+			new_node->adev = cl;
+			INIT_LIST_HEAD(&new_node->node);
+
+			/* Add to the appropriate list */
+			if (strcmp(cname, i915_COMPONENT_CRTC) == 0) {
+				list_add_tail(&new_node->node,
+					      &info->crtc_list);
+			} else if (strcmp(cname, i915_COMPONENT_CONNECTOR) == 0) {
+				list_add_tail(&new_node->node,
+					      &info->connector_list);
+			} else if (strcmp(cname, i915_COMPONENT_PLANE) == 0) {
+				list_add_tail(&new_node->node,
+					      &info->plane_list);
+			} else {
+				/* unknown component, ignore it */
+				kfree(new_node);
+			}
+		}
+	}
+
+	dev_priv->config_info = info;
+	DRM_DEBUG_DRIVER("i915 Configuration loaded.\n");
+	return;
+
+bail:
+	DRM_DEBUG_DRIVER("i915 Configuration aborted, insufficient memory.\n");
+	list_for_each_entry_safe(new_node, tmp, &info->crtc_list, node)
+		kfree(new_node);
+	list_for_each_entry_safe(new_node, tmp, &info->connector_list, node)
+		kfree(new_node);
+	list_for_each_entry_safe(new_node, tmp, &info->plane_list, node)
+		kfree(new_node);
+
+	kfree(info);
+	dev_priv->config_info = NULL;
+}
+
+
+/**
+ * Clean up the configuration infrastructure as the driver is
+ * shuting down.  Don't leak memory!
+ *
+ * @dev: The drm driver device.
+ */
+void intel_config_shutdown(struct drm_device *dev)
+{
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	acpi_handle handle;
+	acpi_status status;
+	struct acpi_device *adev;
+	struct intel_config_info *info;
+	struct intel_config_node *n, *tmp;
+
+	if (dev_priv->config_info == NULL)
+		return;
+
+	/* Free the component lists.  */
+	info = dev_priv->config_info;
+	list_for_each_entry_safe(n, tmp, &info->crtc_list, node)
+		kfree(n);
+	list_for_each_entry_safe(n, tmp, &info->connector_list, node)
+		kfree(n);
+	list_for_each_entry_safe(n, tmp, &info->plane_list, node)
+		kfree(n);
+
+	/* Unload any dynamically loaded ACPI property table */
+	handle = ACPI_HANDLE(dev->dev);
+
+	DRM_DEBUG_DRIVER("Unloading dynamic i915 Configuration ACPI table.\n");
+	if (handle) {
+
+		acpi_unbind_one(dev->dev);
+
+		if (acpi_bus_get_device(handle, &adev))
+			DRM_ERROR("Failed to get ACPI bus device.\n");
+		else
+			acpi_bus_trim(adev);
+
+		status = acpi_unload_parent_table(handle);
+		if (ACPI_FAILURE(status))
+			DRM_ERROR("Failed to unload the i915 Configuration"
+				  "ACPI table. %d\n", status);
+	}
+}
+
+
+
+/*
+ * does_name_match
+ *
+ * The various drm components have names assocated with them. To link
+ * a component in the ACPI property tree, use a "special" property
+ * called "name".
+ *
+ * The exception is the general driver properties.  There is no "name"
+ * property associated with those.  We could force one, but that may
+ * be less intuitive than leaving the name empty.
+ *
+ * This function look for a property called "name" and compares the
+ * value of that property with the passed in name parameter.
+ */
+static bool does_name_match(struct intel_config_node *node, const char *name)
+{
+	char *p_name;
+
+	/*
+	 * General driver properties aren't in a section with a "name"
+	 * property. Thus this should just return true in that case.
+	 */
+	if (!name || strlen(name) == 0)
+		return true;
+
+
+	/* Look up a name property and see if it matches */
+	if (node_property(node, "name", &p_name)) {
+		if (strcmp(name, p_name) == 0)
+			return true;
+	}
+
+	return false;
+}
+
+
+/*
+ * Map the configuration sub component enum to the cached
+ * sub component list.
+ */
+static struct list_head *cfg_type_to_list(struct intel_config_info *info,
+					  enum cfg_type cfg_type)
+{
+	switch (cfg_type) {
+	case CFG_DRV:
+		return &info->base.list;
+	case CFG_CRTC:
+		return &info->crtc_list;
+	case CFG_CONNECTOR:
+		return &info->connector_list;
+	case CFG_PLANE:
+		return &info->plane_list;
+	}
+	return NULL;
+}
+
+/*
+ * Integer property.
+ *
+ * Look up a property and set the value pointer to the property value.
+ *
+ * returns true if the property was found, false if not.
+ */
+bool intel_config_get_integer(struct drm_i915_private *dev_priv,
+			      enum cfg_type cfg_type,
+			      const char *name,
+			      const char *property,
+			      uint32_t *value)
+{
+	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;
+
+	list = cfg_type_to_list(info, cfg_type);
+	if (!list)
+		return false;
+
+	list_for_each_entry(n, list, node) {
+		if (does_name_match(n, name)) {
+			if (node_property(n, property, value))
+				return true;
+		}
+	}
+
+	return false;
+}
+
+/*
+ * Look up a drm property name in the configuration store and if
+ * found, return the value.
+ *
+ * A default value is included in the parameter list so that something
+ * reasonable is returned if the lookup fails to find a matching
+ * configuration key.
+ */
+static uint64_t lookup_property(struct drm_i915_private *dev_priv,
+				const char *name,
+				struct drm_property *property,
+				enum cfg_type cfg_type,
+				uint64_t dflt)
+{
+	struct intel_config_info *info = dev_priv->config_info;
+	struct drm_property_enum *prop_enum;
+	const char *value;
+	uint64_t retval = dflt;
+	struct intel_config_node *n;
+	struct list_head *list;
+
+	list = cfg_type_to_list(info, cfg_type);
+	if (!list)
+		goto out;
+
+	list_for_each_entry(n, list, node) {
+		if (!does_name_match(n, name))
+			continue;
+		/*
+		 * FIXME: This is assuming that the type is string
+		 * for enun drm properties.  This should be more
+		 * generic and map drm property types to ACPI property
+		 * types.
+		 */
+		if (!node_property(n, property->name, &value))
+			continue;
+
+		/* Look for a matching enum */
+		list_for_each_entry(prop_enum, &property->enum_list, head) {
+			if (strcmp(value, prop_enum->name) == 0) {
+				retval = prop_enum->value;
+				goto out;
+			}
+		}
+	}
+
+out:
+	return retval;
+}
+
+/*
+ * Connector properties.
+ *
+ * If a connector drm property has a corresponding configuration property,
+ * use the configuration property value to initialize the drm property.
+ */
+uint64_t intel_config_init_connector_property(struct drm_connector *connector,
+					      const char *name,
+					      struct drm_property *property,
+					      uint64_t dflt)
+{
+	struct drm_i915_private *dev_priv = connector->dev->dev_private;
+	struct intel_config_info *info = dev_priv->config_info;
+	uint64_t retval;
+
+	if (!info)
+		goto out;
+
+	retval = lookup_property(dev_priv, name, property, CFG_CONNECTOR, dflt);
+
+out:
+	drm_object_attach_property(&connector->base, property, retval);
+	return retval;
+}
+
+
+/*
+ * Plane properties.
+ *
+ * If a plane drm property has a corresponding configuration property,
+ * use the configuration property value to initialize the drm property.
+ */
+uint64_t intel_config_init_plane_property(struct drm_plane *plane,
+					  const char *name,
+					  struct drm_property *property,
+					  uint64_t dflt)
+{
+	struct drm_i915_private *dev_priv = plane->dev->dev_private;
+	struct intel_config_info *info = dev_priv->config_info;
+	uint64_t retval;
+
+	if (!info)
+		goto out;
+
+	retval = lookup_property(dev_priv, name, property, CFG_PLANE, dflt);
+
+out:
+	drm_object_attach_property(&plane->base, property, retval);
+	return retval;
+}
+
+
+/*
+ * CRTC properties.
+ *
+ * If a crtc drm property has a corresponding configuration property,
+ * use the configuration property value to initialize the drm property.
+ */
+uint64_t intel_config_init_crtc_property(struct drm_crtc *crtc,
+					 const char *name,
+					 struct drm_property *property,
+					 uint64_t dflt)
+{
+	struct drm_i915_private *dev_priv = crtc->dev->dev_private;
+	struct intel_config_info *info = dev_priv->config_info;
+	uint64_t retval;
+
+	if (!info)
+		goto out;
+
+	retval = lookup_property(dev_priv, name, property, CFG_CRTC, dflt);
+
+out:
+	drm_object_attach_property(&crtc->base, property, retval);
+	return retval;
+}
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 1de8e20..aefd95e 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -1259,4 +1259,32 @@  void intel_plane_destroy_state(struct drm_plane *plane,
 			       struct drm_plane_state *state);
 extern const struct drm_plane_helper_funcs intel_plane_helper_funcs;
 
+/* intel_config.c */
+enum cfg_type {
+	CFG_DRV,
+	CFG_CRTC,
+	CFG_CONNECTOR,
+	CFG_PLANE
+};
+
+void intel_config_init(struct drm_device *dev);
+void intel_config_shutdown(struct drm_device *dev);
+bool intel_config_get_integer(struct drm_i915_private *dev_priv,
+			  enum cfg_type cfg_type,
+			  const char *name,
+			  const char *property,
+			  uint32_t *value);
+uint64_t intel_config_init_connector_property(struct drm_connector *connector,
+			  const char *name,
+			  struct drm_property *property,
+			  uint64_t dflt);
+uint64_t intel_config_init_plane_property(struct drm_plane *plane,
+			  const char *name,
+			  struct drm_property *property,
+			  uint64_t dflt);
+uint64_t intel_config_init_crtc_property(struct drm_crtc *crtc,
+			  const char *name,
+			  struct drm_property *property,
+			  uint64_t dflt);
+
 #endif /* __INTEL_DRV_H__ */