Re: [PATCH 07/10] intel-gpu-tools: register range handling for forcewake hooks

[Chris Wilson <chris@chris-wilson.co.uk>]

On Wed, 13 Jul 2011 13:51:49 -0700, Ben Widawsky <ben@bwidawsk.net> wrote:
Non-text part: multipart/mixed
> We can deprecate the old code by using the non-safe flag in the new API.
> The safe flag should allow the previous behavior to continue.
> 
> The code also adds some range checking on register access. This code is
> gives hooks to prevent tools from doing bad things.
> 
> Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
> ---
>  lib/Makefile.am       |    1 +
>  lib/intel_chipset.h   |    8 ++
>  lib/intel_gpu_tools.h |   27 ++++++++
>  lib/intel_mmio.c      |  178 +++++++++++++++++++++++++++++++++++++++++++++++++
>  lib/intel_reg_map.c   |  174 +++++++++++++++++++++++++++++++++++++++++++++++
>  5 files changed, 388 insertions(+), 0 deletions(-)
> 
> diff --git a/lib/Makefile.am b/lib/Makefile.am
> index 0c9380d..4612cd5 100644
> --- a/lib/Makefile.am
> +++ b/lib/Makefile.am
> @@ -8,6 +8,7 @@ libintel_tools_la_SOURCES = \
>  	intel_mmio.c \
>  	intel_pci.c \
>  	intel_reg.h \
> +	intel_reg_map.c \
>  	instdone.c \
>  	instdone.h \
>  	drmtest.h
> diff --git a/lib/intel_chipset.h b/lib/intel_chipset.h
> index c3db3ab..a38f661 100755
> --- a/lib/intel_chipset.h
> +++ b/lib/intel_chipset.h
> @@ -168,3 +168,11 @@
>  
>  #define HAS_BLT_RING(devid)	(IS_GEN6(devid) || \
>  				 IS_GEN7(devid))
> +
> +#define IS_BROADWATER(devid)	(devid == PCI_CHIP_I946_GZ || \
> +				 devid == PCI_CHIP_I965_G_1 || \
> +				 devid == PCI_CHIP_I965_Q || \
> +				 devid == PCI_CHIP_I965_G)
> +
> +#define IS_CRESTLINE(devid)	(devid == PCI_CHIP_I965_GM || \
> +				 devid == PCI_CHIP_I965_GME)
> diff --git a/lib/intel_gpu_tools.h b/lib/intel_gpu_tools.h
> index acee657..a145fb9 100644
> --- a/lib/intel_gpu_tools.h
> +++ b/lib/intel_gpu_tools.h
> @@ -37,6 +37,33 @@
>  extern void *mmio;
>  void intel_get_mmio(struct pci_device *pci_dev);
>  
> +/* New style register access API */
> +int intel_register_access_init(struct pci_device *pci_dev, int safe);
> +void intel_register_access_fini(void);
> +uint32_t intel_register_read(uint32_t reg);
> +void intel_register_write(uint32_t reg, uint32_t val);
> +
> +#define INTEL_RANGE_RSVD	(0<<0) /*  Shouldn't be read or written */
> +#define INTEL_RANGE_READ	(1<<0)
> +#define INTEL_RANGE_WRITE	(1<<1)
> +#define INTEL_RANGE_RW		(INTEL_RANGE_READ | INTEL_RANGE_WRITE)
> +#define INTEL_RANGE_END		(1<<31)
> +
> +struct intel_register_range {
> +	uint32_t base;
> +	uint32_t size;
> +	uint32_t flags;
> +};
> +
> +struct intel_register_map {
> +	struct intel_register_range *map;
> +	uint32_t top;
> +	uint32_t alignment_mask;
> +};
> +struct intel_register_map intel_get_register_map(uint32_t devid);
> +struct intel_register_range *intel_get_register_range(struct intel_register_map map, uint32_t offset, int mode);
> +
> +
>  static inline uint32_t
>  INREG(uint32_t reg)
>  {
> diff --git a/lib/intel_mmio.c b/lib/intel_mmio.c
> index 0228a87..05cde4f 100644
> --- a/lib/intel_mmio.c
> +++ b/lib/intel_mmio.c
> @@ -22,12 +22,15 @@
>   *
>   * Authors:
>   *    Eric Anholt <eric@anholt.net>
> + *    Ben Widawsky <ben@bwidawsk.net>
>   *
>   */
>  
>  #include <unistd.h>
>  #include <stdlib.h>
>  #include <stdio.h>
> +#include <stdint.h>
> +#include <stdbool.h>
>  #include <string.h>
>  #include <errno.h>
>  #include <err.h>
> @@ -41,6 +44,16 @@
>  
>  void *mmio;
>  
> +static struct _mmio_data {
> +	int inited;
> +	bool safe;
> +	char debugfs_path[FILENAME_MAX];
> +	char debugfs_forcewake_path[FILENAME_MAX];
> +	uint32_t i915_devid;
> +	struct intel_register_map map;
> +	int key;
> +} mmio_data;
> +
>  void
>  intel_map_file(char *file)
>  {
> @@ -89,3 +102,168 @@ intel_get_mmio(struct pci_device *pci_dev)
>  	}
>  }
>  
> +/*
> + * If successful, i915_debugfs_path and i915_debugfs_forcewake_path are both
> + * updated with the correct path.
> + */
> +static int
> +find_debugfs_path(char *dri_base)
> +{
> +	int i;
> +	char buf[FILENAME_MAX];
> +	char name[256];
> +	char pciid[256];
> +	FILE *file;
> +
> +	for (i = 0; i < 16; i++) {
> +		int n;
> +		snprintf(buf, FILENAME_MAX, "%s/%i/name", dri_base, i);
> +
> +		file = fopen(buf, "r");
> +		if (file == NULL)
> +			continue;
> +
> +		/*
> +		 *if (master->unique) {
> +		 *	seq_printf(m, "%s %s %s\n",
> +		 *		   bus_name,
> +		 *		   dev_name(dev->dev), master->unique);
> +		 *} else {
> +		 *	seq_printf(m, "%s %s\n",
> +		 *	bus_name, dev_name(dev->dev));
> +		 *}
> +		 */
> +		n = fscanf(file, "%s %s ", name, pciid);
> +		fclose (file);
> +
> +		if (n != 2)
> +			continue;
> +
> +		if (strncmp("0000:00:02.0", pciid, strlen("0000:00:02.0")))
> +			continue;

We'd like to maintain the option of someday handling more than one
adaptor. We have the pciid, that should be enough to identify it as a
supported Intel gfx chipset?

> +		snprintf(mmio_data.debugfs_path, FILENAME_MAX,
> +			 "%s/%i/", dri_base, i);
> +		snprintf(mmio_data.debugfs_forcewake_path, FILENAME_MAX,
> +			 "%s/%i/i915_forcewake_user", dri_base, i);
> +
And this file won't even exist otherwise... You need a stat here to
confirm that it does.

> +		return 0;
> +	}
> +
> +	return -1;
> +}
> +
> +/* force wake locking is stubbed out until accepted upstream */
> +static int
> +get_forcewake_lock(void)
> +{
> +	return open(mmio_data.debugfs_forcewake_path, 0);
> +}
> +
> +static void
> +release_forcewake_lock(int fd)
> +{
> +	close(fd);
> +}
> +
> +/*
> + * Initialize register access library.
> + *
> + * @pci_dev: pci device we're mucking with
> + * @safe: use safe register access tables
> + */
> +int
> +intel_register_access_init(struct pci_device *pci_dev, int safe)
> +{
> +	int ret;
> +
> +	/* after old API is deprecated, remove this */
> +	if (mmio == NULL)
> +		intel_get_mmio(pci_dev);
> +
> +	assert(mmio != NULL);
> +
> +	if (mmio_data.inited)
> +		return -1;
> +
> +	mmio_data.safe = safe != 0 ? true : false;
> +
> +	/* Find where the forcewake lock is */
> +	ret = find_debugfs_path("/sys/kernel/debug/dri");
Need to try /debug afterwards, just because that's the other convention.

> +	if (ret) {
> +		fprintf(stderr, "Couldn't find path to dri/debugfs entry\n");
> +		return ret;
> +	}
> +
> +	mmio_data.i915_devid = pci_dev->device_id;
> +	if (mmio_data.safe)
> +		mmio_data.map = intel_get_register_map(mmio_data.i915_devid);
> +
> +	mmio_data.key = get_forcewake_lock();
> +	mmio_data.inited++;
> +	return 0;
> +}
> +
> +void
> +intel_register_access_fini(void)
> +{
> +	release_forcewake_lock(mmio_data.key);
> +	mmio_data.inited--;
> +}
> +
> +uint32_t
> +intel_register_read(uint32_t reg)
> +{
> +	struct intel_register_range *range;
> +	uint32_t ret;
> +
> +	assert(mmio_data.inited);
> +
> +	if (IS_GEN6(mmio_data.i915_devid))
> +		assert(mmio_data.key != -1);
> +
> +	if (!mmio_data.safe)
> +		goto read_out;
> +
> +	range = intel_get_register_range(mmio_data.map,
> +					 reg,
> +					 INTEL_RANGE_READ);
> +
> +	if(!range) {
        if (!range) {


> +struct intel_register_map
> +intel_get_register_map(uint32_t devid)
> +{
> +	struct intel_register_map map;
> +
> +	if (IS_GEN6(devid)) {
> +		map.map = gen6_gt_register_map;
> +		map.top = 0x180000;
> +	} else if (IS_BROADWATER(devid) || IS_CRESTLINE(devid)) {
> +		map.map = gen_bwcl_register_map;
> +		map.top = 0x80000;
> +	} else if (IS_GEN4(devid) || IS_GEN5(devid)) {
> +		map.map = gen4_register_map;
> +		map.top = 0x80000;
> +	} else {
> +		abort();
Give us some warning that this function will die horribly on gen2/3!
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre