Re: [Intel-gfx] [RFC 35/60] drm/i915/query: Expose memory regions through the query uAPI

[Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>]

On 10/07/2020 12:57, Matthew Auld wrote:
> From: Abdiel Janulgue <abdiel.janulgue@linux.intel.com>
> 
> Returns the available memory region areas supported by the HW.
> 
> Signed-off-by: Abdiel Janulgue <abdiel.janulgue@linux.intel.com>
> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> ---
>   drivers/gpu/drm/i915/gem/i915_gem_stolen.c | 12 ++++-
>   drivers/gpu/drm/i915/gem/i915_gem_stolen.h |  3 ++
>   drivers/gpu/drm/i915/i915_drv.c            |  2 +-
>   drivers/gpu/drm/i915/i915_pci.c            |  2 +-
>   drivers/gpu/drm/i915/i915_query.c          | 62 ++++++++++++++++++++++
>   drivers/gpu/drm/i915/intel_memory_region.c | 32 ++++++-----
>   drivers/gpu/drm/i915/intel_memory_region.h | 38 +++++++------
>   include/uapi/drm/i915_drm.h                | 58 ++++++++++++++++++++
>   8 files changed, 172 insertions(+), 37 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_stolen.c b/drivers/gpu/drm/i915/gem/i915_gem_stolen.c
> index e0f21f12d3ce..6704877fbda8 100644
> --- a/drivers/gpu/drm/i915/gem/i915_gem_stolen.c
> +++ b/drivers/gpu/drm/i915/gem/i915_gem_stolen.c
> @@ -646,11 +646,19 @@ _i915_gem_object_create_stolen(struct intel_memory_region *mem,
>   	return obj;
>   }
>   
> +struct intel_memory_region *i915_stolen_region(struct drm_i915_private *i915)
> +{
> +	if (HAS_LMEM(i915))
> +		return i915->mm.regions[INTEL_REGION_STOLEN_LMEM];
> +
> +	return i915->mm.regions[INTEL_REGION_STOLEN_SMEM];
> +}

The split of the stolen region to lmem and smem should be a separate 
patch I think.

> +
>   struct drm_i915_gem_object *
>   i915_gem_object_create_stolen(struct drm_i915_private *i915,
>   			      resource_size_t size)
>   {
> -	return i915_gem_object_create_region(i915->mm.regions[INTEL_REGION_STOLEN],
> +	return i915_gem_object_create_region(i915_stolen_region(i915),
>   					     size, I915_BO_ALLOC_CONTIGUOUS);
>   }
>   
> @@ -690,7 +698,7 @@ i915_gem_object_create_stolen_for_preallocated(struct drm_i915_private *i915,
>   					       resource_size_t stolen_offset,
>   					       resource_size_t size)
>   {
> -	struct intel_memory_region *mem = i915->mm.regions[INTEL_REGION_STOLEN];
> +	struct intel_memory_region *mem = i915_stolen_region(i915);
>   	struct drm_i915_gem_object *obj;
>   	struct drm_mm_node *stolen;
>   	int ret;
> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_stolen.h b/drivers/gpu/drm/i915/gem/i915_gem_stolen.h
> index e15c0adad8af..a19110a1b75a 100644
> --- a/drivers/gpu/drm/i915/gem/i915_gem_stolen.h
> +++ b/drivers/gpu/drm/i915/gem/i915_gem_stolen.h
> @@ -22,6 +22,9 @@ int i915_gem_stolen_insert_node_in_range(struct drm_i915_private *dev_priv,
>   void i915_gem_stolen_remove_node(struct drm_i915_private *dev_priv,
>   				 struct drm_mm_node *node);
>   struct intel_memory_region *i915_gem_stolen_setup(struct drm_i915_private *i915);
> +
> +struct intel_memory_region *i915_stolen_region(struct drm_i915_private *i915);
> +
>   struct drm_i915_gem_object *
>   i915_gem_object_create_stolen(struct drm_i915_private *dev_priv,
>   			      resource_size_t size);
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index 5473bfe9126c..39826b98fac2 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -963,7 +963,7 @@ int i915_driver_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
>   		if (INTEL_GEN(i915) >= 9 && i915_selftest.live < 0 &&
>   		    i915->params.fake_lmem_start) {
>   			mkwrite_device_info(i915)->memory_regions =
> -				REGION_SMEM | REGION_LMEM | REGION_STOLEN;
> +				REGION_SMEM | REGION_LMEM | REGION_STOLEN_SMEM;
>   			mkwrite_device_info(i915)->is_dgfx = true;
>   			GEM_BUG_ON(!HAS_LMEM(i915));
>   			GEM_BUG_ON(!IS_DGFX(i915));
> diff --git a/drivers/gpu/drm/i915/i915_pci.c b/drivers/gpu/drm/i915/i915_pci.c
> index d5e27202d150..e132fdffa432 100644
> --- a/drivers/gpu/drm/i915/i915_pci.c
> +++ b/drivers/gpu/drm/i915/i915_pci.c
> @@ -154,7 +154,7 @@
>   	.page_sizes = I915_GTT_PAGE_SIZE_4K
>   
>   #define GEN_DEFAULT_REGIONS \
> -	.memory_regions = REGION_SMEM | REGION_STOLEN
> +	.memory_regions = REGION_SMEM | REGION_STOLEN_SMEM
>   
>   #define I830_FEATURES \
>   	GEN(2), \
> diff --git a/drivers/gpu/drm/i915/i915_query.c b/drivers/gpu/drm/i915/i915_query.c
> index fed337ad7b68..d4ca040c528b 100644
> --- a/drivers/gpu/drm/i915/i915_query.c
> +++ b/drivers/gpu/drm/i915/i915_query.c
> @@ -419,11 +419,73 @@ static int query_perf_config(struct drm_i915_private *i915,
>   	}
>   }
>   
> +static int query_memregion_info(struct drm_i915_private *dev_priv,
> +				struct drm_i915_query_item *query_item)
> +{
> +	struct drm_i915_query_memory_regions __user *query_ptr =
> +		u64_to_user_ptr(query_item->data_ptr);
> +	struct drm_i915_memory_region_info __user *info_ptr =
> +		&query_ptr->regions[0];
> +	struct drm_i915_memory_region_info info = { };
> +	struct drm_i915_query_memory_regions query;
> +	u32 total_length;
> +	int ret, i;
> +
> +	if (query_item->flags != 0)
> +		return -EINVAL;
> +
> +	total_length = sizeof(query);
> +	for (i = 0; i < ARRAY_SIZE(dev_priv->mm.regions); ++i) {
> +		struct intel_memory_region *region = dev_priv->mm.regions[i];
> +
> +		if (!region)
> +			continue;
> +
> +		total_length += sizeof(info);
> +	}
> +
> +	ret = copy_query_item(&query, sizeof(query), total_length, query_item);
> +	if (ret != 0)
> +		return ret;
> +
> +	if (query.num_regions)
> +		return -EINVAL;
> +
> +	for (i = 0; i < ARRAY_SIZE(query.rsvd); ++i) {
> +		if (query.rsvd[i])
> +			return  -EINVAL;
> +	}
> +
> +	for (i = 0; i < ARRAY_SIZE(dev_priv->mm.regions); ++i) {
> +		struct intel_memory_region *region = dev_priv->mm.regions[i];
> +
> +		if (!region)
> +			continue;
> +
> +		info.region.memory_class = region->type;
> +		info.region.memory_instance = region->instance;
> +		info.probed_size = region->total;
> +		info.unallocated_size = region->avail;
> +
> +		if (__copy_to_user(info_ptr, &info, sizeof(info)))
> +			return -EFAULT;
> +
> +		query.num_regions++;
> +		info_ptr++;
> +	}
> +
> +	if (__copy_to_user(query_ptr, &query, sizeof(query)))
> +		return -EFAULT;
> +
> +	return total_length;
> +}
> +
>   static int (* const i915_query_funcs[])(struct drm_i915_private *dev_priv,
>   					struct drm_i915_query_item *query_item) = {
>   	query_topology_info,
>   	query_engine_info,
>   	query_perf_config,
> +	query_memregion_info,
>   };
>   
>   int i915_query_ioctl(struct drm_device *dev, void *data, struct drm_file *file)
> diff --git a/drivers/gpu/drm/i915/intel_memory_region.c b/drivers/gpu/drm/i915/intel_memory_region.c
> index 2943c7778d5e..b9eb1a42dd3a 100644
> --- a/drivers/gpu/drm/i915/intel_memory_region.c
> +++ b/drivers/gpu/drm/i915/intel_memory_region.c
> @@ -6,14 +6,19 @@
>   #include "intel_memory_region.h"
>   #include "i915_drv.h"
>   
> -/* XXX: Hysterical raisins. BIT(inst) needs to just be (inst) at some point. */
> -#define REGION_MAP(type, inst) \
> -	BIT((type) + INTEL_MEMORY_TYPE_SHIFT) | BIT(inst)
> -
> -const u32 intel_region_map[] = {
> -	[INTEL_REGION_SMEM] = REGION_MAP(INTEL_MEMORY_SYSTEM, 0),
> -	[INTEL_REGION_LMEM] = REGION_MAP(INTEL_MEMORY_LOCAL, 0),
> -	[INTEL_REGION_STOLEN] = REGION_MAP(INTEL_MEMORY_STOLEN, 0),
> +const struct intel_memory_region_info intel_region_map[] = {
> +       [INTEL_REGION_SMEM] = {
> +               .class = INTEL_MEMORY_SYSTEM,
> +               .instance = 0,
> +       },
> +       [INTEL_REGION_LMEM] = {
> +               .class = INTEL_MEMORY_LOCAL,
> +               .instance = 0,
> +       },
> +       [INTEL_REGION_STOLEN_SMEM] = {
> +               .class = INTEL_MEMORY_STOLEN_SYSTEM,
> +               .instance = 0,
> +       },
>   };
>   
>   struct intel_memory_region *
> @@ -257,17 +262,18 @@ int intel_memory_regions_hw_probe(struct drm_i915_private *i915)
>   
>   	for (i = 0; i < ARRAY_SIZE(i915->mm.regions); i++) {
>   		struct intel_memory_region *mem = ERR_PTR(-ENODEV);
> -		u32 type;
> +		u16 type, instance;
>   
>   		if (!HAS_REGION(i915, BIT(i)))
>   			continue;
>   
> -		type = MEMORY_TYPE_FROM_REGION(intel_region_map[i]);
> +		type = intel_region_map[i].class;
> +		instance = intel_region_map[i].instance;
>   		switch (type) {
>   		case INTEL_MEMORY_SYSTEM:
>   			mem = i915_gem_shmem_setup(i915);
>   			break;
> -		case INTEL_MEMORY_STOLEN:
> +		case INTEL_MEMORY_STOLEN_SYSTEM:
>   			mem = i915_gem_stolen_setup(i915);
>   			break;
>   		case INTEL_MEMORY_LOCAL:
> @@ -283,9 +289,9 @@ int intel_memory_regions_hw_probe(struct drm_i915_private *i915)
>   			goto out_cleanup;
>   		}
>   
> -		mem->id = intel_region_map[i];
> +		mem->id = i;
>   		mem->type = type;
> -		mem->instance = MEMORY_INSTANCE_FROM_REGION(intel_region_map[i]);
> +		mem->instance = instance;
>   
>   		i915->mm.regions[i] = mem;
>   	}
> diff --git a/drivers/gpu/drm/i915/intel_memory_region.h b/drivers/gpu/drm/i915/intel_memory_region.h
> index 232490d89a83..c047cf7c5e7c 100644
> --- a/drivers/gpu/drm/i915/intel_memory_region.h
> +++ b/drivers/gpu/drm/i915/intel_memory_region.h
> @@ -11,6 +11,7 @@
>   #include <linux/mutex.h>
>   #include <linux/io-mapping.h>
>   #include <drm/drm_mm.h>
> +#include <drm/i915_drm.h>
>   
>   #include "i915_buddy.h"
>   
> @@ -19,30 +20,25 @@ struct drm_i915_gem_object;
>   struct intel_memory_region;
>   struct sg_table;
>   
> -/**
> - *  Base memory type
> - */
>   enum intel_memory_type {
> -	INTEL_MEMORY_SYSTEM = 0,
> -	INTEL_MEMORY_LOCAL,
> -	INTEL_MEMORY_STOLEN,
> +	INTEL_MEMORY_SYSTEM = I915_MEMORY_CLASS_SYSTEM,
> +	INTEL_MEMORY_LOCAL = I915_MEMORY_CLASS_DEVICE,
> +	INTEL_MEMORY_STOLEN_SYSTEM = I915_MEMORY_CLASS_STOLEN_SYSTEM,
> +	INTEL_MEMORY_STOLEN_LOCAL = I915_MEMORY_CLASS_STOLEN_DEVICE,
>   };
>   
>   enum intel_region_id {
>   	INTEL_REGION_SMEM = 0,
>   	INTEL_REGION_LMEM,
> -	INTEL_REGION_STOLEN,
> +	INTEL_REGION_STOLEN_SMEM,
> +	INTEL_REGION_STOLEN_LMEM,
>   	INTEL_REGION_UNKNOWN, /* Should be last */
>   };
>   
>   #define REGION_SMEM     BIT(INTEL_REGION_SMEM)
>   #define REGION_LMEM     BIT(INTEL_REGION_LMEM)
> -#define REGION_STOLEN   BIT(INTEL_REGION_STOLEN)
> -
> -#define INTEL_MEMORY_TYPE_SHIFT 16
> -
> -#define MEMORY_TYPE_FROM_REGION(r) (ilog2((r) >> INTEL_MEMORY_TYPE_SHIFT))
> -#define MEMORY_INSTANCE_FROM_REGION(r) (ilog2((r) & 0xffff))
> +#define REGION_STOLEN_SMEM   BIT(INTEL_REGION_STOLEN_SMEM)
> +#define REGION_STOLEN_LMEM   BIT(INTEL_REGION_STOLEN_LMEM)
>   
>   #define I915_ALLOC_MIN_PAGE_SIZE  BIT(0)
>   #define I915_ALLOC_CONTIGUOUS     BIT(1)
> @@ -51,10 +47,12 @@ enum intel_region_id {
>   	for (id = 0; id < ARRAY_SIZE((i915)->mm.regions); id++) \
>   		for_each_if((mr) = (i915)->mm.regions[id])
>   
> -/**
> - * Memory regions encoded as type | instance
> - */
> -extern const u32 intel_region_map[];
> +struct intel_memory_region_info {
> +       u16 class;
> +       u16 instance;
> +};
> +
> +extern const struct intel_memory_region_info intel_region_map[];
>   
>   struct intel_memory_region_ops {
>   	unsigned int flags;
> @@ -89,9 +87,9 @@ struct intel_memory_region {
>   	resource_size_t total;
>   	resource_size_t avail;
>   
> -	unsigned int type;
> -	unsigned int instance;
> -	unsigned int id;
> +	u16 type;
> +	u16 instance;

Class and type could be tidied to just one.

struct intel_memory_region_info could then be used here. Or even struct 
drm_i915_gem_memory_class_instance but granted perhaps it is better to 
keep uapi and implementation separate.

> +	enum intel_region_id id;
>   	char name[8];
>   
>   	dma_addr_t remap_addr;
> diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h
> index 14b67cd6b54b..46baedf71cb1 100644
> --- a/include/uapi/drm/i915_drm.h
> +++ b/include/uapi/drm/i915_drm.h
> @@ -2122,6 +2122,7 @@ struct drm_i915_query_item {
>   #define DRM_I915_QUERY_TOPOLOGY_INFO    1
>   #define DRM_I915_QUERY_ENGINE_INFO	2
>   #define DRM_I915_QUERY_PERF_CONFIG      3
> +#define DRM_I915_QUERY_MEMORY_REGIONS   4
>   /* Must be kept compact -- no holes and well documented */
>   
>   	/*
> @@ -2322,6 +2323,63 @@ struct drm_i915_query_perf_config {
>   	__u8 data[];
>   };
>   
> +enum drm_i915_gem_memory_class {
> +	I915_MEMORY_CLASS_SYSTEM = 0,
> +	I915_MEMORY_CLASS_DEVICE,
> +	I915_MEMORY_CLASS_STOLEN_SYSTEM,
> +	I915_MEMORY_CLASS_STOLEN_DEVICE,
> +};
> +
> +struct drm_i915_gem_memory_class_instance {
> +	__u16 memory_class; /* see enum drm_i915_gem_memory_class */
> +	__u16 memory_instance;
> +};
> +
> +/**
> + * struct drm_i915_memory_region_info
> + *
> + * Describes one region as known to the driver.
> + */
> +struct drm_i915_memory_region_info {
> +	/** class:instance pair encoding */
> +	struct drm_i915_gem_memory_class_instance region;
> +
> +	/** MBZ */
> +	__u32 rsvd0;
> +
> +	/** MBZ */
> +	__u64 caps;
> +
> +	/** MBZ */
> +	__u64 flags;
> +
> +	/** Memory probed by the driver (-1 = unknown) */
> +	__u64 probed_size;
> +
> +	/** Estimate of memory remaining (-1 = unknown) */
> +	__u64 unallocated_size;
> +
> +	/** MBZ */
> +	__u64 rsvd1[8];
> +};
> +
> +/**
> + * struct drm_i915_query_memory_regions
> + *
> + * Region info query enumerates all regions known to the driver by filling in
> + * an array of struct drm_i915_memory_region_info structures.
> + */
> +struct drm_i915_query_memory_regions {
> +	/** Number of supported regions */
> +	__u32 num_regions;
> +
> +	/** MBZ */
> +	__u32 rsvd[3];
> +
> +	/* Info about each supported region */
> +	struct drm_i915_memory_region_info regions[];
> +};
> +
>   #if defined(__cplusplus)
>   }
>   #endif
> 

The rest looks good to me.

Regards,

Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx