Hi,
This is the second series that prepares i915 to host multitile platforms. It introduces the for_each_gt() macro that loops over the tiles to perform per gt actions.
This patch is a combination of two patches developed originally by Abdiel, who introduced some refactoring during probe, and then Tvrtko has added the necessary tools to start using the various tiles.
The second patch re-organises the sysfs interface to expose the API for each of the GTs. I decided to prioritise this patch over others to unblock Sujaritha for further development.
A third series will still follow this.
Thanks, Andi
Patchwork: https://patchwork.freedesktop.org/series/98741/
Changelog ========= v4 -> v5 - fixed Michal's reviews. - the sysfs patches have been split in 3 parts to make reviews easier. - Sujaritha's patch on pm throttle has been queued. - INTEL_REGION_LMEM has been renamed to INTEL_REGION_LMEM_0 - added the gt_is_root() helper - the sysfs files will be called intel_gt_sysfs_* instead of sysfs_gt_*
v3 -> v4 - fixed Tvrtko's review: - remove the SYSFS_DEPRECATED_V2 mention from the commit log - reworded the error message when accessing deprecated files - errors in sysfs are printed as warnings as they are not fatal - the inline functions are moved to be out of line. and some other minor refactoring.
v2 -> v3 - Added Matt and Sujaritha's r-b for patch 1 and 2. - Reworded the commit of patch 2 to underline the fact that the interface is useful also when used manually.
v1 -> v2 - fixed a couple of coding style issues in patch 2.
Andi Shyti (5): drm/i915: Rename INTEL_REGION_LMEM with INTEL_REGION_LMEM_0 drm/i915/gt: add gt_is_root() helper drm/i915/gt: create per-tile sysfs interface drm/i915/gt: Create per-tile RC6 sysfs interface drm/i915/gt: Create per-tile RPS sysfs interfaces
Sujaritha Sundaresan (1): RFC : drm/i915: Adding new sysfs frequency attributes
Tvrtko Ursulin (1): drm/i915: Prepare for multiple GTs
drivers/gpu/drm/i915/Makefile | 2 + drivers/gpu/drm/i915/display/intel_fb.c | 2 +- drivers/gpu/drm/i915/display/intel_fb_pin.c | 2 +- drivers/gpu/drm/i915/gem/i915_gem_lmem.c | 4 +- .../drm/i915/gem/selftests/i915_gem_dmabuf.c | 6 +- .../drm/i915/gem/selftests/i915_gem_migrate.c | 8 +- drivers/gpu/drm/i915/gt/intel_gt.c | 137 +++- drivers/gpu/drm/i915/gt/intel_gt.h | 21 +- drivers/gpu/drm/i915/gt/intel_gt_pm.c | 9 +- drivers/gpu/drm/i915/gt/intel_gt_sysfs.c | 137 ++++ drivers/gpu/drm/i915/gt/intel_gt_sysfs.h | 34 + drivers/gpu/drm/i915/gt/intel_gt_sysfs_pm.c | 638 ++++++++++++++++++ drivers/gpu/drm/i915/gt/intel_gt_sysfs_pm.h | 15 + drivers/gpu/drm/i915/gt/intel_gt_types.h | 7 + drivers/gpu/drm/i915/gt/intel_rps.c | 83 +++ drivers/gpu/drm/i915/gt/intel_rps.h | 10 + drivers/gpu/drm/i915/i915_driver.c | 29 +- drivers/gpu/drm/i915/i915_drv.h | 8 + drivers/gpu/drm/i915/i915_reg.h | 11 + drivers/gpu/drm/i915/i915_sysfs.c | 315 +-------- drivers/gpu/drm/i915/i915_sysfs.h | 3 + drivers/gpu/drm/i915/intel_memory_region.c | 2 +- drivers/gpu/drm/i915/intel_memory_region.h | 7 +- drivers/gpu/drm/i915/intel_uncore.c | 12 +- drivers/gpu/drm/i915/intel_uncore.h | 3 +- .../gpu/drm/i915/selftests/mock_gem_device.c | 7 +- 26 files changed, 1148 insertions(+), 364 deletions(-) create mode 100644 drivers/gpu/drm/i915/gt/intel_gt_sysfs.c create mode 100644 drivers/gpu/drm/i915/gt/intel_gt_sysfs.h create mode 100644 drivers/gpu/drm/i915/gt/intel_gt_sysfs_pm.c create mode 100644 drivers/gpu/drm/i915/gt/intel_gt_sysfs_pm.h
With the upcoming multitile support each tile will have its own local memory. Mark the current LMEM with the suffix '0' to emphasise that it belongs to the root tile.
Suggested-by: Michal Wajdeczko michal.wajdeczko@intel.com Signed-off-by: Andi Shyti andi.shyti@linux.intel.com --- drivers/gpu/drm/i915/display/intel_fb.c | 2 +- drivers/gpu/drm/i915/display/intel_fb_pin.c | 2 +- drivers/gpu/drm/i915/gem/i915_gem_lmem.c | 4 ++-- drivers/gpu/drm/i915/gem/selftests/i915_gem_dmabuf.c | 6 +++--- drivers/gpu/drm/i915/gem/selftests/i915_gem_migrate.c | 8 ++++---- drivers/gpu/drm/i915/gt/intel_gt.c | 2 +- drivers/gpu/drm/i915/intel_memory_region.c | 2 +- drivers/gpu/drm/i915/intel_memory_region.h | 4 ++-- 8 files changed, 15 insertions(+), 15 deletions(-)
diff --git a/drivers/gpu/drm/i915/display/intel_fb.c b/drivers/gpu/drm/i915/display/intel_fb.c index 23cfe2e5ce2a..421f7238da05 100644 --- a/drivers/gpu/drm/i915/display/intel_fb.c +++ b/drivers/gpu/drm/i915/display/intel_fb.c @@ -1981,7 +1981,7 @@ intel_user_framebuffer_create(struct drm_device *dev,
/* object is backed with LMEM for discrete */ i915 = to_i915(obj->base.dev); - if (HAS_LMEM(i915) && !i915_gem_object_can_migrate(obj, INTEL_REGION_LMEM)) { + if (HAS_LMEM(i915) && !i915_gem_object_can_migrate(obj, INTEL_REGION_LMEM_0)) { /* object is "remote", not in local memory */ i915_gem_object_put(obj); return ERR_PTR(-EREMOTE); diff --git a/drivers/gpu/drm/i915/display/intel_fb_pin.c b/drivers/gpu/drm/i915/display/intel_fb_pin.c index a307b4993bcf..bd6e7c98e751 100644 --- a/drivers/gpu/drm/i915/display/intel_fb_pin.c +++ b/drivers/gpu/drm/i915/display/intel_fb_pin.c @@ -140,7 +140,7 @@ intel_pin_and_fence_fb_obj(struct drm_framebuffer *fb, if (!ret && phys_cursor) ret = i915_gem_object_attach_phys(obj, alignment); else if (!ret && HAS_LMEM(dev_priv)) - ret = i915_gem_object_migrate(obj, &ww, INTEL_REGION_LMEM); + ret = i915_gem_object_migrate(obj, &ww, INTEL_REGION_LMEM_0); /* TODO: Do we need to sync when migration becomes async? */ if (!ret) ret = i915_gem_object_pin_pages(obj); diff --git a/drivers/gpu/drm/i915/gem/i915_gem_lmem.c b/drivers/gpu/drm/i915/gem/i915_gem_lmem.c index 444f8268b9c5..47e43dc3a174 100644 --- a/drivers/gpu/drm/i915/gem/i915_gem_lmem.c +++ b/drivers/gpu/drm/i915/gem/i915_gem_lmem.c @@ -100,7 +100,7 @@ __i915_gem_object_create_lmem_with_ps(struct drm_i915_private *i915, resource_size_t page_size, unsigned int flags) { - return i915_gem_object_create_region(i915->mm.regions[INTEL_REGION_LMEM], + return i915_gem_object_create_region(i915->mm.regions[INTEL_REGION_LMEM_0], size, page_size, flags); }
@@ -135,6 +135,6 @@ i915_gem_object_create_lmem(struct drm_i915_private *i915, resource_size_t size, unsigned int flags) { - return i915_gem_object_create_region(i915->mm.regions[INTEL_REGION_LMEM], + return i915_gem_object_create_region(i915->mm.regions[INTEL_REGION_LMEM_0], size, 0, flags); } diff --git a/drivers/gpu/drm/i915/gem/selftests/i915_gem_dmabuf.c b/drivers/gpu/drm/i915/gem/selftests/i915_gem_dmabuf.c index b071a58dd6da..a342fd387d4e 100644 --- a/drivers/gpu/drm/i915/gem/selftests/i915_gem_dmabuf.c +++ b/drivers/gpu/drm/i915/gem/selftests/i915_gem_dmabuf.c @@ -88,7 +88,7 @@ static int igt_dmabuf_import_self(void *arg) static int igt_dmabuf_import_same_driver_lmem(void *arg) { struct drm_i915_private *i915 = arg; - struct intel_memory_region *lmem = i915->mm.regions[INTEL_REGION_LMEM]; + struct intel_memory_region *lmem = i915->mm.regions[INTEL_REGION_LMEM_0]; struct drm_i915_gem_object *obj; struct drm_gem_object *import; struct dma_buf *dmabuf; @@ -252,10 +252,10 @@ static int igt_dmabuf_import_same_driver_lmem_smem(void *arg) struct drm_i915_private *i915 = arg; struct intel_memory_region *regions[2];
- if (!i915->mm.regions[INTEL_REGION_LMEM]) + if (!i915->mm.regions[INTEL_REGION_LMEM_0]) return 0;
- regions[0] = i915->mm.regions[INTEL_REGION_LMEM]; + regions[0] = i915->mm.regions[INTEL_REGION_LMEM_0]; regions[1] = i915->mm.regions[INTEL_REGION_SMEM]; return igt_dmabuf_import_same_driver(i915, regions, 2); } diff --git a/drivers/gpu/drm/i915/gem/selftests/i915_gem_migrate.c b/drivers/gpu/drm/i915/gem/selftests/i915_gem_migrate.c index d534141b2cf7..2c63daf932de 100644 --- a/drivers/gpu/drm/i915/gem/selftests/i915_gem_migrate.c +++ b/drivers/gpu/drm/i915/gem/selftests/i915_gem_migrate.c @@ -92,17 +92,17 @@ static int igt_create_migrate(struct intel_gt *gt, enum intel_region_id src,
static int igt_smem_create_migrate(void *arg) { - return igt_create_migrate(arg, INTEL_REGION_LMEM, INTEL_REGION_SMEM); + return igt_create_migrate(arg, INTEL_REGION_LMEM_0, INTEL_REGION_SMEM); }
static int igt_lmem_create_migrate(void *arg) { - return igt_create_migrate(arg, INTEL_REGION_SMEM, INTEL_REGION_LMEM); + return igt_create_migrate(arg, INTEL_REGION_SMEM, INTEL_REGION_LMEM_0); }
static int igt_same_create_migrate(void *arg) { - return igt_create_migrate(arg, INTEL_REGION_LMEM, INTEL_REGION_LMEM); + return igt_create_migrate(arg, INTEL_REGION_LMEM_0, INTEL_REGION_LMEM_0); }
static int lmem_pages_migrate_one(struct i915_gem_ww_ctx *ww, @@ -152,7 +152,7 @@ static int lmem_pages_migrate_one(struct i915_gem_ww_ctx *ww, }
} else { - err = i915_gem_object_migrate(obj, ww, INTEL_REGION_LMEM); + err = i915_gem_object_migrate(obj, ww, INTEL_REGION_LMEM_0); if (err) { pr_err("Object failed migration to lmem\n"); if (err) diff --git a/drivers/gpu/drm/i915/gt/intel_gt.c b/drivers/gpu/drm/i915/gt/intel_gt.c index e8403fa53909..db171e85f4df 100644 --- a/drivers/gpu/drm/i915/gt/intel_gt.c +++ b/drivers/gpu/drm/i915/gt/intel_gt.c @@ -78,7 +78,7 @@ int intel_gt_probe_lmem(struct intel_gt *gt) return err; }
- id = INTEL_REGION_LMEM; + id = INTEL_REGION_LMEM_0;
mem->id = id;
diff --git a/drivers/gpu/drm/i915/intel_memory_region.c b/drivers/gpu/drm/i915/intel_memory_region.c index c70d7e286a51..18d7f0aa314e 100644 --- a/drivers/gpu/drm/i915/intel_memory_region.c +++ b/drivers/gpu/drm/i915/intel_memory_region.c @@ -17,7 +17,7 @@ static const struct { .class = INTEL_MEMORY_SYSTEM, .instance = 0, }, - [INTEL_REGION_LMEM] = { + [INTEL_REGION_LMEM_0] = { .class = INTEL_MEMORY_LOCAL, .instance = 0, }, diff --git a/drivers/gpu/drm/i915/intel_memory_region.h b/drivers/gpu/drm/i915/intel_memory_region.h index 5625c9c38993..95db0a8029e2 100644 --- a/drivers/gpu/drm/i915/intel_memory_region.h +++ b/drivers/gpu/drm/i915/intel_memory_region.h @@ -29,14 +29,14 @@ enum intel_memory_type {
enum intel_region_id { INTEL_REGION_SMEM = 0, - INTEL_REGION_LMEM, + INTEL_REGION_LMEM_0, 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_LMEM BIT(INTEL_REGION_LMEM_0) #define REGION_STOLEN_SMEM BIT(INTEL_REGION_STOLEN_SMEM) #define REGION_STOLEN_LMEM BIT(INTEL_REGION_STOLEN_LMEM)
On 17.02.2022 15:41, Andi Shyti wrote:
With the upcoming multitile support each tile will have its own local memory. Mark the current LMEM with the suffix '0' to emphasise that it belongs to the root tile.
Suggested-by: Michal Wajdeczko michal.wajdeczko@intel.com Signed-off-by: Andi Shyti andi.shyti@linux.intel.com
drivers/gpu/drm/i915/display/intel_fb.c | 2 +- drivers/gpu/drm/i915/display/intel_fb_pin.c | 2 +- drivers/gpu/drm/i915/gem/i915_gem_lmem.c | 4 ++-- drivers/gpu/drm/i915/gem/selftests/i915_gem_dmabuf.c | 6 +++--- drivers/gpu/drm/i915/gem/selftests/i915_gem_migrate.c | 8 ++++---- drivers/gpu/drm/i915/gt/intel_gt.c | 2 +- drivers/gpu/drm/i915/intel_memory_region.c | 2 +- drivers/gpu/drm/i915/intel_memory_region.h | 4 ++-- 8 files changed, 15 insertions(+), 15 deletions(-)
diff --git a/drivers/gpu/drm/i915/display/intel_fb.c b/drivers/gpu/drm/i915/display/intel_fb.c index 23cfe2e5ce2a..421f7238da05 100644 --- a/drivers/gpu/drm/i915/display/intel_fb.c +++ b/drivers/gpu/drm/i915/display/intel_fb.c @@ -1981,7 +1981,7 @@ intel_user_framebuffer_create(struct drm_device *dev,
/* object is backed with LMEM for discrete */ i915 = to_i915(obj->base.dev);
- if (HAS_LMEM(i915) && !i915_gem_object_can_migrate(obj, INTEL_REGION_LMEM)) {
- if (HAS_LMEM(i915) && !i915_gem_object_can_migrate(obj, INTEL_REGION_LMEM_0)) { /* object is "remote", not in local memory */ i915_gem_object_put(obj); return ERR_PTR(-EREMOTE);
diff --git a/drivers/gpu/drm/i915/display/intel_fb_pin.c b/drivers/gpu/drm/i915/display/intel_fb_pin.c index a307b4993bcf..bd6e7c98e751 100644 --- a/drivers/gpu/drm/i915/display/intel_fb_pin.c +++ b/drivers/gpu/drm/i915/display/intel_fb_pin.c @@ -140,7 +140,7 @@ intel_pin_and_fence_fb_obj(struct drm_framebuffer *fb, if (!ret && phys_cursor) ret = i915_gem_object_attach_phys(obj, alignment); else if (!ret && HAS_LMEM(dev_priv))
ret = i915_gem_object_migrate(obj, &ww, INTEL_REGION_LMEM);
ret = i915_gem_object_migrate(obj, &ww, INTEL_REGION_LMEM_0);diff --git a/drivers/gpu/drm/i915/gem/i915_gem_lmem.c b/drivers/gpu/drm/i915/gem/i915_gem_lmem.c index 444f8268b9c5..47e43dc3a174 100644 --- a/drivers/gpu/drm/i915/gem/i915_gem_lmem.c +++ b/drivers/gpu/drm/i915/gem/i915_gem_lmem.c @@ -100,7 +100,7 @@ __i915_gem_object_create_lmem_with_ps(struct drm_i915_private *i915, resource_size_t page_size, unsigned int flags) {
- return i915_gem_object_create_region(i915->mm.regions[INTEL_REGION_LMEM],
- return i915_gem_object_create_region(i915->mm.regions[INTEL_REGION_LMEM_0], size, page_size, flags);
}
@@ -135,6 +135,6 @@ i915_gem_object_create_lmem(struct drm_i915_private *i915, resource_size_t size, unsigned int flags) {
- return i915_gem_object_create_region(i915->mm.regions[INTEL_REGION_LMEM],
- return i915_gem_object_create_region(i915->mm.regions[INTEL_REGION_LMEM_0], size, 0, flags);
} diff --git a/drivers/gpu/drm/i915/gem/selftests/i915_gem_dmabuf.c b/drivers/gpu/drm/i915/gem/selftests/i915_gem_dmabuf.c index b071a58dd6da..a342fd387d4e 100644 --- a/drivers/gpu/drm/i915/gem/selftests/i915_gem_dmabuf.c +++ b/drivers/gpu/drm/i915/gem/selftests/i915_gem_dmabuf.c @@ -88,7 +88,7 @@ static int igt_dmabuf_import_self(void *arg) static int igt_dmabuf_import_same_driver_lmem(void *arg) { struct drm_i915_private *i915 = arg;
- struct intel_memory_region *lmem = i915->mm.regions[INTEL_REGION_LMEM];
- struct intel_memory_region *lmem = i915->mm.regions[INTEL_REGION_LMEM_0]; struct drm_i915_gem_object *obj; struct drm_gem_object *import; struct dma_buf *dmabuf;
@@ -252,10 +252,10 @@ static int igt_dmabuf_import_same_driver_lmem_smem(void *arg) struct drm_i915_private *i915 = arg; struct intel_memory_region *regions[2];
- if (!i915->mm.regions[INTEL_REGION_LMEM])
- if (!i915->mm.regions[INTEL_REGION_LMEM_0]) return 0;
- regions[0] = i915->mm.regions[INTEL_REGION_LMEM];
- regions[0] = i915->mm.regions[INTEL_REGION_LMEM_0]; regions[1] = i915->mm.regions[INTEL_REGION_SMEM]; return igt_dmabuf_import_same_driver(i915, regions, 2);
} diff --git a/drivers/gpu/drm/i915/gem/selftests/i915_gem_migrate.c b/drivers/gpu/drm/i915/gem/selftests/i915_gem_migrate.c index d534141b2cf7..2c63daf932de 100644 --- a/drivers/gpu/drm/i915/gem/selftests/i915_gem_migrate.c +++ b/drivers/gpu/drm/i915/gem/selftests/i915_gem_migrate.c @@ -92,17 +92,17 @@ static int igt_create_migrate(struct intel_gt *gt, enum intel_region_id src,
static int igt_smem_create_migrate(void *arg) {
- return igt_create_migrate(arg, INTEL_REGION_LMEM, INTEL_REGION_SMEM);
- return igt_create_migrate(arg, INTEL_REGION_LMEM_0, INTEL_REGION_SMEM);
}
static int igt_lmem_create_migrate(void *arg) {
- return igt_create_migrate(arg, INTEL_REGION_SMEM, INTEL_REGION_LMEM);
- return igt_create_migrate(arg, INTEL_REGION_SMEM, INTEL_REGION_LMEM_0);
}
static int igt_same_create_migrate(void *arg) {
- return igt_create_migrate(arg, INTEL_REGION_LMEM, INTEL_REGION_LMEM);
- return igt_create_migrate(arg, INTEL_REGION_LMEM_0, INTEL_REGION_LMEM_0);
}
static int lmem_pages_migrate_one(struct i915_gem_ww_ctx *ww, @@ -152,7 +152,7 @@ static int lmem_pages_migrate_one(struct i915_gem_ww_ctx *ww, }
} else {
err = i915_gem_object_migrate(obj, ww, INTEL_REGION_LMEM);
err = i915_gem_object_migrate(obj, ww, INTEL_REGION_LMEM_0);diff --git a/drivers/gpu/drm/i915/gt/intel_gt.c b/drivers/gpu/drm/i915/gt/intel_gt.c index e8403fa53909..db171e85f4df 100644 --- a/drivers/gpu/drm/i915/gt/intel_gt.c +++ b/drivers/gpu/drm/i915/gt/intel_gt.c @@ -78,7 +78,7 @@ int intel_gt_probe_lmem(struct intel_gt *gt) return err; }
- id = INTEL_REGION_LMEM;
id = INTEL_REGION_LMEM_0;
mem->id = id;
diff --git a/drivers/gpu/drm/i915/intel_memory_region.c b/drivers/gpu/drm/i915/intel_memory_region.c index c70d7e286a51..18d7f0aa314e 100644 --- a/drivers/gpu/drm/i915/intel_memory_region.c +++ b/drivers/gpu/drm/i915/intel_memory_region.c @@ -17,7 +17,7 @@ static const struct { .class = INTEL_MEMORY_SYSTEM, .instance = 0, },
- [INTEL_REGION_LMEM] = {
- [INTEL_REGION_LMEM_0] = { .class = INTEL_MEMORY_LOCAL, .instance = 0, },
diff --git a/drivers/gpu/drm/i915/intel_memory_region.h b/drivers/gpu/drm/i915/intel_memory_region.h index 5625c9c38993..95db0a8029e2 100644 --- a/drivers/gpu/drm/i915/intel_memory_region.h +++ b/drivers/gpu/drm/i915/intel_memory_region.h @@ -29,14 +29,14 @@ enum intel_memory_type {
enum intel_region_id { INTEL_REGION_SMEM = 0,
- INTEL_REGION_LMEM,
- INTEL_REGION_LMEM_0,
wasn't sure how big this change will be, but after looking at next patch, it was worth doing that, so
Reviewed-by: Michal Wajdeczko michal.wajdeczko@intel.com
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_LMEM BIT(INTEL_REGION_LMEM_0) #define REGION_STOLEN_SMEM BIT(INTEL_REGION_STOLEN_SMEM) #define REGION_STOLEN_LMEM BIT(INTEL_REGION_STOLEN_LMEM)
On 17.02.2022 15:41, Andi Shyti wrote:
With the upcoming multitile support each tile will have its own local memory. Mark the current LMEM with the suffix '0' to emphasise that it belongs to the root tile.
Suggested-by: Michal Wajdeczko michal.wajdeczko@intel.com Signed-off-by: Andi Shyti andi.shyti@linux.intel.com
Reviewed-by: Andrzej Hajda andrzej.hajda@intel.com
Regards Andrzej
From: Tvrtko Ursulin tvrtko.ursulin@intel.com
On a multi-tile platform, each tile has its own registers + GGTT space, and BAR 0 is extended to cover all of them.
Up to four GTs are supported in i915->gt[], with slot zero shadowing the existing i915->gt0 to enable source compatibility with legacy driver paths. A for_each_gt macro is added to iterate over the GTs and will be used by upcoming patches that convert various parts of the driver to be multi-gt aware.
Only the primary/root tile is initialized for now; the other tiles will be detected and plugged in by future patches once the necessary infrastructure is in place to handle them.
Signed-off-by: Abdiel Janulgue abdiel.janulgue@gmail.com Signed-off-by: Daniele Ceraolo Spurio daniele.ceraolospurio@intel.com Signed-off-by: Tvrtko Ursulin tvrtko.ursulin@intel.com Signed-off-by: Matt Roper matthew.d.roper@intel.com Signed-off-by: Andi Shyti andi.shyti@linux.intel.com Cc: Daniele Ceraolo Spurio daniele.ceraolospurio@intel.com Cc: Joonas Lahtinen joonas.lahtinen@linux.intel.com Cc: Matthew Auld matthew.auld@intel.com Reviewed-by: Matt Roper matthew.d.roper@intel.com --- drivers/gpu/drm/i915/gt/intel_gt.c | 135 ++++++++++++++++-- drivers/gpu/drm/i915/gt/intel_gt.h | 16 ++- drivers/gpu/drm/i915/gt/intel_gt_pm.c | 9 +- drivers/gpu/drm/i915/gt/intel_gt_types.h | 7 + drivers/gpu/drm/i915/i915_driver.c | 29 ++-- drivers/gpu/drm/i915/i915_drv.h | 6 + drivers/gpu/drm/i915/intel_memory_region.h | 3 + drivers/gpu/drm/i915/intel_uncore.c | 12 +- drivers/gpu/drm/i915/intel_uncore.h | 3 +- .../gpu/drm/i915/selftests/mock_gem_device.c | 7 +- 10 files changed, 182 insertions(+), 45 deletions(-)
diff --git a/drivers/gpu/drm/i915/gt/intel_gt.c b/drivers/gpu/drm/i915/gt/intel_gt.c index db171e85f4df..8c64b81e9ec9 100644 --- a/drivers/gpu/drm/i915/gt/intel_gt.c +++ b/drivers/gpu/drm/i915/gt/intel_gt.c @@ -29,7 +29,7 @@ #include "intel_uncore.h" #include "shmem_utils.h"
-void __intel_gt_init_early(struct intel_gt *gt, struct drm_i915_private *i915) +static void __intel_gt_init_early(struct intel_gt *gt) { spin_lock_init(>->irq_lock);
@@ -51,19 +51,29 @@ void __intel_gt_init_early(struct intel_gt *gt, struct drm_i915_private *i915) intel_rps_init_early(>->rps); }
-void intel_gt_init_early(struct intel_gt *gt, struct drm_i915_private *i915) +/* Preliminary initialization of Tile 0 */ +void intel_root_gt_init_early(struct drm_i915_private *i915) { + struct intel_gt *gt = to_gt(i915); + gt->i915 = i915; gt->uncore = &i915->uncore; + + __intel_gt_init_early(gt); }
-int intel_gt_probe_lmem(struct intel_gt *gt) +static int intel_gt_probe_lmem(struct intel_gt *gt) { struct drm_i915_private *i915 = gt->i915; + unsigned int instance = gt->info.id; struct intel_memory_region *mem; int id; int err;
+ id = INTEL_REGION_LMEM_0 + instance; + if (drm_WARN_ON(&i915->drm, id >= INTEL_REGION_STOLEN_SMEM)) + return -ENODEV; + mem = intel_gt_setup_lmem(gt); if (mem == ERR_PTR(-ENODEV)) mem = intel_gt_setup_fake_lmem(gt); @@ -78,9 +88,8 @@ int intel_gt_probe_lmem(struct intel_gt *gt) return err; }
- id = INTEL_REGION_LMEM_0; - mem->id = id; + mem->instance = instance;
intel_memory_region_set_name(mem, "local%u", mem->instance);
@@ -795,16 +804,21 @@ void intel_gt_driver_release(struct intel_gt *gt) intel_gt_fini_buffer_pool(gt); }
-void intel_gt_driver_late_release(struct intel_gt *gt) +void intel_gt_driver_late_release(struct drm_i915_private *i915) { + struct intel_gt *gt; + unsigned int id; + /* We need to wait for inflight RCU frees to release their grip */ rcu_barrier();
- intel_uc_driver_late_release(>->uc); - intel_gt_fini_requests(gt); - intel_gt_fini_reset(gt); - intel_gt_fini_timelines(gt); - intel_engines_free(gt); + for_each_gt(gt, i915, id) { + intel_uc_driver_late_release(>->uc); + intel_gt_fini_requests(gt); + intel_gt_fini_reset(gt); + intel_gt_fini_timelines(gt); + intel_engines_free(gt); + } }
/** @@ -913,6 +927,105 @@ u32 intel_gt_read_register_fw(struct intel_gt *gt, i915_reg_t reg) return intel_uncore_read_fw(gt->uncore, reg); }
+static int intel_gt_tile_setup(struct intel_gt *gt, phys_addr_t phys_addr) +{ + unsigned int id = gt->info.id; + int ret; + + if (id) { + struct intel_uncore_mmio_debug *mmio_debug; + struct intel_uncore *uncore; + + uncore = kzalloc(sizeof(*uncore), GFP_KERNEL); + if (!gt->uncore) + return -ENOMEM; + + mmio_debug = kzalloc(sizeof(*mmio_debug), GFP_KERNEL); + if (!mmio_debug) { + kfree(uncore); + return -ENOMEM; + } + + gt->uncore = uncore; + gt->uncore->debug = mmio_debug; + + __intel_gt_init_early(gt); + } + + intel_uncore_init_early(gt->uncore, gt); + + ret = intel_uncore_setup_mmio(gt->uncore, phys_addr); + if (ret) + return ret; + + gt->phys_addr = phys_addr; + + return 0; +} + +static void +intel_gt_tile_cleanup(struct intel_gt *gt) +{ + intel_uncore_cleanup_mmio(gt->uncore); + + if (gt->info.id) { + kfree(gt->uncore); + kfree(gt); + } +} + +int intel_gt_probe_all(struct drm_i915_private *i915) +{ + struct pci_dev *pdev = to_pci_dev(i915->drm.dev); + struct intel_gt *gt = &i915->gt0; + phys_addr_t phys_addr; + unsigned int mmio_bar; + int ret; + + mmio_bar = GRAPHICS_VER(i915) == 2 ? 1 : 0; + phys_addr = pci_resource_start(pdev, mmio_bar); + + /* + * We always have at least one primary GT on any device + * and it has been already initialized early during probe + * in i915_driver_probe() + */ + ret = intel_gt_tile_setup(gt, phys_addr); + if (ret) + return ret; + + i915->gt[0] = gt; + + /* TODO: add more tiles */ + return 0; +} + +int intel_gt_tiles_init(struct drm_i915_private *i915) +{ + struct intel_gt *gt; + unsigned int id; + int ret; + + for_each_gt(gt, i915, id) { + ret = intel_gt_probe_lmem(gt); + if (ret) + return ret; + } + + return 0; +} + +void intel_gt_release_all(struct drm_i915_private *i915) +{ + struct intel_gt *gt; + unsigned int id; + + for_each_gt(gt, i915, id) { + intel_gt_tile_cleanup(gt); + i915->gt[id] = NULL; + } +} + void intel_gt_info_print(const struct intel_gt_info *info, struct drm_printer *p) { diff --git a/drivers/gpu/drm/i915/gt/intel_gt.h b/drivers/gpu/drm/i915/gt/intel_gt.h index 2dad46c3eff2..915d6192079b 100644 --- a/drivers/gpu/drm/i915/gt/intel_gt.h +++ b/drivers/gpu/drm/i915/gt/intel_gt.h @@ -34,10 +34,8 @@ static inline struct intel_gt *huc_to_gt(struct intel_huc *huc) return container_of(huc, struct intel_gt, uc.huc); }
-void intel_gt_init_early(struct intel_gt *gt, struct drm_i915_private *i915); -void __intel_gt_init_early(struct intel_gt *gt, struct drm_i915_private *i915); +void intel_root_gt_init_early(struct drm_i915_private *i915); int intel_gt_assign_ggtt(struct intel_gt *gt); -int intel_gt_probe_lmem(struct intel_gt *gt); int intel_gt_init_mmio(struct intel_gt *gt); int __must_check intel_gt_init_hw(struct intel_gt *gt); int intel_gt_init(struct intel_gt *gt); @@ -47,7 +45,7 @@ void intel_gt_driver_unregister(struct intel_gt *gt); void intel_gt_driver_remove(struct intel_gt *gt); void intel_gt_driver_release(struct intel_gt *gt);
-void intel_gt_driver_late_release(struct intel_gt *gt); +void intel_gt_driver_late_release(struct drm_i915_private *i915);
int intel_gt_wait_for_idle(struct intel_gt *gt, long timeout);
@@ -86,6 +84,16 @@ static inline bool intel_gt_needs_read_steering(struct intel_gt *gt,
u32 intel_gt_read_register_fw(struct intel_gt *gt, i915_reg_t reg);
+int intel_gt_probe_all(struct drm_i915_private *i915); +int intel_gt_tiles_init(struct drm_i915_private *i915); +void intel_gt_release_all(struct drm_i915_private *i915); + +#define for_each_gt(gt__, i915__, id__) \ + for ((id__) = 0; \ + (id__) < I915_MAX_GT; \ + (id__)++) \ + for_each_if(((gt__) = (i915__)->gt[(id__)])) + void intel_gt_info_print(const struct intel_gt_info *info, struct drm_printer *p);
diff --git a/drivers/gpu/drm/i915/gt/intel_gt_pm.c b/drivers/gpu/drm/i915/gt/intel_gt_pm.c index c0fa41e4c803..e66479d33bc3 100644 --- a/drivers/gpu/drm/i915/gt/intel_gt_pm.c +++ b/drivers/gpu/drm/i915/gt/intel_gt_pm.c @@ -128,7 +128,14 @@ static const struct intel_wakeref_ops wf_ops = {
void intel_gt_pm_init_early(struct intel_gt *gt) { - intel_wakeref_init(>->wakeref, gt->uncore->rpm, &wf_ops); + /* + * We access the runtime_pm structure via gt->i915 here rather than + * gt->uncore as we do elsewhere in the file because gt->uncore is not + * yet initialized for all tiles at this point in the driver startup. + * runtime_pm is per-device rather than per-tile, so this is still the + * correct structure. + */ + intel_wakeref_init(>->wakeref, >->i915->runtime_pm, &wf_ops); seqcount_mutex_init(>->stats.lock, >->wakeref.mutex); }
diff --git a/drivers/gpu/drm/i915/gt/intel_gt_types.h b/drivers/gpu/drm/i915/gt/intel_gt_types.h index f20687796490..89fad770b2d4 100644 --- a/drivers/gpu/drm/i915/gt/intel_gt_types.h +++ b/drivers/gpu/drm/i915/gt/intel_gt_types.h @@ -182,7 +182,14 @@ struct intel_gt {
const struct intel_mmio_range *steering_table[NUM_STEERING_TYPES];
+ /* + * Base of per-tile GTTMMADR where we can derive the MMIO and the GGTT. + */ + phys_addr_t phys_addr; + struct intel_gt_info { + unsigned int id; + intel_engine_mask_t engine_mask;
u32 l3bank_mask; diff --git a/drivers/gpu/drm/i915/i915_driver.c b/drivers/gpu/drm/i915/i915_driver.c index 1c67ff735f18..144f989e4fef 100644 --- a/drivers/gpu/drm/i915/i915_driver.c +++ b/drivers/gpu/drm/i915/i915_driver.c @@ -320,9 +320,8 @@ static int i915_driver_early_probe(struct drm_i915_private *dev_priv) intel_device_info_subplatform_init(dev_priv); intel_step_init(dev_priv);
- intel_gt_init_early(to_gt(dev_priv), dev_priv); + /* All tiles share a single mmio_debug */ intel_uncore_mmio_debug_init_early(&dev_priv->mmio_debug); - intel_uncore_init_early(&dev_priv->uncore, to_gt(dev_priv));
spin_lock_init(&dev_priv->irq_lock); spin_lock_init(&dev_priv->gpu_error.lock); @@ -353,7 +352,7 @@ static int i915_driver_early_probe(struct drm_i915_private *dev_priv)
intel_wopcm_init_early(&dev_priv->wopcm);
- __intel_gt_init_early(to_gt(dev_priv), dev_priv); + intel_root_gt_init_early(dev_priv);
i915_gem_init_early(dev_priv);
@@ -374,7 +373,7 @@ static int i915_driver_early_probe(struct drm_i915_private *dev_priv)
err_gem: i915_gem_cleanup_early(dev_priv); - intel_gt_driver_late_release(to_gt(dev_priv)); + intel_gt_driver_late_release(dev_priv); intel_region_ttm_device_fini(dev_priv); err_ttm: vlv_suspend_cleanup(dev_priv); @@ -393,7 +392,7 @@ static void i915_driver_late_release(struct drm_i915_private *dev_priv) intel_irq_fini(dev_priv); intel_power_domains_cleanup(dev_priv); i915_gem_cleanup_early(dev_priv); - intel_gt_driver_late_release(to_gt(dev_priv)); + intel_gt_driver_late_release(dev_priv); intel_region_ttm_device_fini(dev_priv); vlv_suspend_cleanup(dev_priv); i915_workqueues_cleanup(dev_priv); @@ -424,13 +423,9 @@ static int i915_driver_mmio_probe(struct drm_i915_private *dev_priv) if (ret < 0) return ret;
- ret = intel_uncore_setup_mmio(&dev_priv->uncore); - if (ret < 0) - goto err_bridge; - ret = intel_uncore_init_mmio(&dev_priv->uncore); if (ret) - goto err_mmio; + return ret;
/* Try to make sure MCHBAR is enabled before poking at it */ intel_setup_mchbar(dev_priv); @@ -448,9 +443,6 @@ static int i915_driver_mmio_probe(struct drm_i915_private *dev_priv) err_uncore: intel_teardown_mchbar(dev_priv); intel_uncore_fini_mmio(&dev_priv->uncore); -err_mmio: - intel_uncore_cleanup_mmio(&dev_priv->uncore); -err_bridge: pci_dev_put(dev_priv->bridge_dev);
return ret; @@ -464,7 +456,6 @@ static void i915_driver_mmio_release(struct drm_i915_private *dev_priv) { intel_teardown_mchbar(dev_priv); intel_uncore_fini_mmio(&dev_priv->uncore); - intel_uncore_cleanup_mmio(&dev_priv->uncore); pci_dev_put(dev_priv->bridge_dev); }
@@ -597,7 +588,7 @@ static int i915_driver_hw_probe(struct drm_i915_private *dev_priv) if (ret) goto err_ggtt;
- ret = intel_gt_probe_lmem(to_gt(dev_priv)); + ret = intel_gt_tiles_init(dev_priv); if (ret) goto err_mem_regions;
@@ -862,10 +853,14 @@ int i915_driver_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
intel_vgpu_detect(i915);
- ret = i915_driver_mmio_probe(i915); + ret = intel_gt_probe_all(i915); if (ret < 0) goto out_runtime_pm_put;
+ ret = i915_driver_mmio_probe(i915); + if (ret < 0) + goto out_tiles_cleanup; + ret = i915_driver_hw_probe(i915); if (ret < 0) goto out_cleanup_mmio; @@ -922,6 +917,8 @@ int i915_driver_probe(struct pci_dev *pdev, const struct pci_device_id *ent) i915_ggtt_driver_late_release(i915); out_cleanup_mmio: i915_driver_mmio_release(i915); +out_tiles_cleanup: + intel_gt_release_all(i915); out_runtime_pm_put: enable_rpm_wakeref_asserts(&i915->runtime_pm); i915_driver_late_release(i915); diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index 418091484e02..88a83cd81ddd 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -805,6 +805,12 @@ struct drm_i915_private { /* Abstract the submission mechanism (legacy ringbuffer or execlists) away */ struct intel_gt gt0;
+ /* + * i915->gt[0] == &i915->gt0 + */ +#define I915_MAX_GT 4 + struct intel_gt *gt[I915_MAX_GT]; + struct { struct i915_gem_contexts { spinlock_t lock; /* locks list */ diff --git a/drivers/gpu/drm/i915/intel_memory_region.h b/drivers/gpu/drm/i915/intel_memory_region.h index 95db0a8029e2..9548335344be 100644 --- a/drivers/gpu/drm/i915/intel_memory_region.h +++ b/drivers/gpu/drm/i915/intel_memory_region.h @@ -30,6 +30,9 @@ enum intel_memory_type { enum intel_region_id { INTEL_REGION_SMEM = 0, INTEL_REGION_LMEM_0, + INTEL_REGION_LMEM_1, + INTEL_REGION_LMEM_2, + INTEL_REGION_LMEM_3, INTEL_REGION_STOLEN_SMEM, INTEL_REGION_STOLEN_LMEM, INTEL_REGION_UNKNOWN, /* Should be last */ diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c index dd8fdd5863de..75d9fa21923e 100644 --- a/drivers/gpu/drm/i915/intel_uncore.c +++ b/drivers/gpu/drm/i915/intel_uncore.c @@ -2039,14 +2039,11 @@ static int i915_pmic_bus_access_notifier(struct notifier_block *nb, return NOTIFY_OK; }
-int intel_uncore_setup_mmio(struct intel_uncore *uncore) +int intel_uncore_setup_mmio(struct intel_uncore *uncore, phys_addr_t phys_addr) { struct drm_i915_private *i915 = uncore->i915; - struct pci_dev *pdev = to_pci_dev(i915->drm.dev); - int mmio_bar; int mmio_size;
- mmio_bar = GRAPHICS_VER(i915) == 2 ? 1 : 0; /* * Before gen4, the registers and the GTT are behind different BARs. * However, from gen4 onwards, the registers and the GTT are shared @@ -2063,7 +2060,7 @@ int intel_uncore_setup_mmio(struct intel_uncore *uncore) else mmio_size = 2 * 1024 * 1024;
- uncore->regs = pci_iomap(pdev, mmio_bar, mmio_size); + uncore->regs = ioremap(phys_addr, mmio_size); if (uncore->regs == NULL) { drm_err(&i915->drm, "failed to map registers\n"); return -EIO; @@ -2074,9 +2071,8 @@ int intel_uncore_setup_mmio(struct intel_uncore *uncore)
void intel_uncore_cleanup_mmio(struct intel_uncore *uncore) { - struct pci_dev *pdev = to_pci_dev(uncore->i915->drm.dev); - - pci_iounmap(pdev, uncore->regs); + if (uncore->regs) + iounmap(uncore->regs); }
void intel_uncore_init_early(struct intel_uncore *uncore, diff --git a/drivers/gpu/drm/i915/intel_uncore.h b/drivers/gpu/drm/i915/intel_uncore.h index 6ff56d673e2b..de53c961fadb 100644 --- a/drivers/gpu/drm/i915/intel_uncore.h +++ b/drivers/gpu/drm/i915/intel_uncore.h @@ -29,6 +29,7 @@ #include <linux/notifier.h> #include <linux/hrtimer.h> #include <linux/io-64-nonatomic-lo-hi.h> +#include <linux/types.h>
#include "i915_reg_defs.h"
@@ -219,7 +220,7 @@ void intel_uncore_mmio_debug_init_early(struct intel_uncore_mmio_debug *mmio_debug); void intel_uncore_init_early(struct intel_uncore *uncore, struct intel_gt *gt); -int intel_uncore_setup_mmio(struct intel_uncore *uncore); +int intel_uncore_setup_mmio(struct intel_uncore *uncore, phys_addr_t phys_addr); int intel_uncore_init_mmio(struct intel_uncore *uncore); void intel_uncore_prune_engine_fw_domains(struct intel_uncore *uncore, struct intel_gt *gt); diff --git a/drivers/gpu/drm/i915/selftests/mock_gem_device.c b/drivers/gpu/drm/i915/selftests/mock_gem_device.c index 573d9b2e1a4a..f72436c5d596 100644 --- a/drivers/gpu/drm/i915/selftests/mock_gem_device.c +++ b/drivers/gpu/drm/i915/selftests/mock_gem_device.c @@ -73,7 +73,7 @@ static void mock_device_release(struct drm_device *dev) destroy_workqueue(i915->wq);
intel_region_ttm_device_fini(i915); - intel_gt_driver_late_release(to_gt(i915)); + intel_gt_driver_late_release(i915); intel_memory_regions_driver_release(i915);
drm_mode_config_cleanup(&i915->drm); @@ -180,8 +180,7 @@ struct drm_i915_private *mock_gem_device(void) spin_lock_init(&i915->gpu_error.lock);
i915_gem_init__mm(i915); - intel_gt_init_early(to_gt(i915), i915); - __intel_gt_init_early(to_gt(i915), i915); + intel_root_gt_init_early(i915); mock_uncore_init(&i915->uncore, i915); atomic_inc(&to_gt(i915)->wakeref.count); /* disable; no hw support */ to_gt(i915)->awake = -ENODEV; @@ -229,7 +228,7 @@ struct drm_i915_private *mock_gem_device(void) err_drv: intel_region_ttm_device_fini(i915); err_ttm: - intel_gt_driver_late_release(to_gt(i915)); + intel_gt_driver_late_release(i915); intel_memory_regions_driver_release(i915); drm_mode_config_cleanup(&i915->drm); mock_destroy_device(i915);
On 17.02.2022 15:41, Andi Shyti wrote:
From: Tvrtko Ursulin tvrtko.ursulin@intel.com
On a multi-tile platform, each tile has its own registers + GGTT space, and BAR 0 is extended to cover all of them.
Up to four GTs are supported in i915->gt[], with slot zero shadowing the existing i915->gt0 to enable source compatibility with legacy driver paths. A for_each_gt macro is added to iterate over the GTs and will be used by upcoming patches that convert various parts of the driver to be multi-gt aware.
Only the primary/root tile is initialized for now; the other tiles will be detected and plugged in by future patches once the necessary infrastructure is in place to handle them.
Signed-off-by: Abdiel Janulgue abdiel.janulgue@gmail.com Signed-off-by: Daniele Ceraolo Spurio daniele.ceraolospurio@intel.com Signed-off-by: Tvrtko Ursulin tvrtko.ursulin@intel.com Signed-off-by: Matt Roper matthew.d.roper@intel.com Signed-off-by: Andi Shyti andi.shyti@linux.intel.com Cc: Daniele Ceraolo Spurio daniele.ceraolospurio@intel.com Cc: Joonas Lahtinen joonas.lahtinen@linux.intel.com Cc: Matthew Auld matthew.auld@intel.com Reviewed-by: Matt Roper matthew.d.roper@intel.com
drivers/gpu/drm/i915/gt/intel_gt.c | 135 ++++++++++++++++-- drivers/gpu/drm/i915/gt/intel_gt.h | 16 ++- drivers/gpu/drm/i915/gt/intel_gt_pm.c | 9 +- drivers/gpu/drm/i915/gt/intel_gt_types.h | 7 + drivers/gpu/drm/i915/i915_driver.c | 29 ++-- drivers/gpu/drm/i915/i915_drv.h | 6 + drivers/gpu/drm/i915/intel_memory_region.h | 3 + drivers/gpu/drm/i915/intel_uncore.c | 12 +- drivers/gpu/drm/i915/intel_uncore.h | 3 +- .../gpu/drm/i915/selftests/mock_gem_device.c | 7 +- 10 files changed, 182 insertions(+), 45 deletions(-)
diff --git a/drivers/gpu/drm/i915/gt/intel_gt.c b/drivers/gpu/drm/i915/gt/intel_gt.c index db171e85f4df..8c64b81e9ec9 100644 --- a/drivers/gpu/drm/i915/gt/intel_gt.c +++ b/drivers/gpu/drm/i915/gt/intel_gt.c @@ -29,7 +29,7 @@ #include "intel_uncore.h" #include "shmem_utils.h"
-void __intel_gt_init_early(struct intel_gt *gt, struct drm_i915_private *i915) +static void __intel_gt_init_early(struct intel_gt *gt) { spin_lock_init(>->irq_lock);
@@ -51,19 +51,29 @@ void __intel_gt_init_early(struct intel_gt *gt, struct drm_i915_private *i915) intel_rps_init_early(>->rps); }
-void intel_gt_init_early(struct intel_gt *gt, struct drm_i915_private *i915) +/* Preliminary initialization of Tile 0 */ +void intel_root_gt_init_early(struct drm_i915_private *i915) {
- struct intel_gt *gt = to_gt(i915);
- gt->i915 = i915; gt->uncore = &i915->uncore;
- __intel_gt_init_early(gt); }
-int intel_gt_probe_lmem(struct intel_gt *gt) +static int intel_gt_probe_lmem(struct intel_gt *gt) { struct drm_i915_private *i915 = gt->i915;
unsigned int instance = gt->info.id; struct intel_memory_region *mem; int id; int err;
id = INTEL_REGION_LMEM_0 + instance;
if (drm_WARN_ON(&i915->drm, id >= INTEL_REGION_STOLEN_SMEM))
Do we need to check id correctness? wouldn't be enough to check it on initialization of gt->info.id. If yes, maybe (id > INTEL_REGION_LMEM_3) would be more readable, or (info.id < MAX_GT), up to you.
return -ENODEV;- mem = intel_gt_setup_lmem(gt); if (mem == ERR_PTR(-ENODEV)) mem = intel_gt_setup_fake_lmem(gt);
@@ -78,9 +88,8 @@ int intel_gt_probe_lmem(struct intel_gt *gt) return err; }
- id = INTEL_REGION_LMEM_0;
- mem->id = id;
mem->instance = instance;
intel_memory_region_set_name(mem, "local%u", mem->instance);
@@ -795,16 +804,21 @@ void intel_gt_driver_release(struct intel_gt *gt) intel_gt_fini_buffer_pool(gt); }
-void intel_gt_driver_late_release(struct intel_gt *gt) +void intel_gt_driver_late_release(struct drm_i915_private *i915) {
- struct intel_gt *gt;
- unsigned int id;
- /* We need to wait for inflight RCU frees to release their grip */ rcu_barrier();
- intel_uc_driver_late_release(>->uc);
- intel_gt_fini_requests(gt);
- intel_gt_fini_reset(gt);
- intel_gt_fini_timelines(gt);
- intel_engines_free(gt);
for_each_gt(gt, i915, id) {
intel_uc_driver_late_release(>->uc);intel_gt_fini_requests(gt);intel_gt_fini_reset(gt);intel_gt_fini_timelines(gt);intel_engines_free(gt);} }
/**
@@ -913,6 +927,105 @@ u32 intel_gt_read_register_fw(struct intel_gt *gt, i915_reg_t reg) return intel_uncore_read_fw(gt->uncore, reg); }
+static int intel_gt_tile_setup(struct intel_gt *gt, phys_addr_t phys_addr) +{
- unsigned int id = gt->info.id;
- int ret;
- if (id) {
struct intel_uncore_mmio_debug *mmio_debug;struct intel_uncore *uncore;uncore = kzalloc(sizeof(*uncore), GFP_KERNEL);if (!gt->uncore)return -ENOMEM;
s/gt->uncore/uncore/
mmio_debug = kzalloc(sizeof(*mmio_debug), GFP_KERNEL);if (!mmio_debug) {kfree(uncore);return -ENOMEM;}gt->uncore = uncore;gt->uncore->debug = mmio_debug;__intel_gt_init_early(gt);- }
- intel_uncore_init_early(gt->uncore, gt);
- ret = intel_uncore_setup_mmio(gt->uncore, phys_addr);
- if (ret)
return ret;- gt->phys_addr = phys_addr;
- return 0;
+}
+static void +intel_gt_tile_cleanup(struct intel_gt *gt) +{
- intel_uncore_cleanup_mmio(gt->uncore);
- if (gt->info.id) {
kfree(gt->uncore);kfree(gt);
What about gt->uncore->debug ?
- }
+}
+int intel_gt_probe_all(struct drm_i915_private *i915) +{
- struct pci_dev *pdev = to_pci_dev(i915->drm.dev);
- struct intel_gt *gt = &i915->gt0;
- phys_addr_t phys_addr;
- unsigned int mmio_bar;
- int ret;
- mmio_bar = GRAPHICS_VER(i915) == 2 ? 1 : 0;
- phys_addr = pci_resource_start(pdev, mmio_bar);
- /*
* We always have at least one primary GT on any device* and it has been already initialized early during probe* in i915_driver_probe()*/- ret = intel_gt_tile_setup(gt, phys_addr);
- if (ret)
return ret;- i915->gt[0] = gt;
- /* TODO: add more tiles */
- return 0;
+}
+int intel_gt_tiles_init(struct drm_i915_private *i915) +{
- struct intel_gt *gt;
- unsigned int id;
- int ret;
- for_each_gt(gt, i915, id) {
ret = intel_gt_probe_lmem(gt);if (ret)return ret;- }
- return 0;
+}
+void intel_gt_release_all(struct drm_i915_private *i915) +{
- struct intel_gt *gt;
- unsigned int id;
- for_each_gt(gt, i915, id) {
intel_gt_tile_cleanup(gt);i915->gt[id] = NULL;- }
+}
- void intel_gt_info_print(const struct intel_gt_info *info, struct drm_printer *p) {
diff --git a/drivers/gpu/drm/i915/gt/intel_gt.h b/drivers/gpu/drm/i915/gt/intel_gt.h index 2dad46c3eff2..915d6192079b 100644 --- a/drivers/gpu/drm/i915/gt/intel_gt.h +++ b/drivers/gpu/drm/i915/gt/intel_gt.h @@ -34,10 +34,8 @@ static inline struct intel_gt *huc_to_gt(struct intel_huc *huc) return container_of(huc, struct intel_gt, uc.huc); }
-void intel_gt_init_early(struct intel_gt *gt, struct drm_i915_private *i915); -void __intel_gt_init_early(struct intel_gt *gt, struct drm_i915_private *i915); +void intel_root_gt_init_early(struct drm_i915_private *i915); int intel_gt_assign_ggtt(struct intel_gt *gt); -int intel_gt_probe_lmem(struct intel_gt *gt); int intel_gt_init_mmio(struct intel_gt *gt); int __must_check intel_gt_init_hw(struct intel_gt *gt); int intel_gt_init(struct intel_gt *gt); @@ -47,7 +45,7 @@ void intel_gt_driver_unregister(struct intel_gt *gt); void intel_gt_driver_remove(struct intel_gt *gt); void intel_gt_driver_release(struct intel_gt *gt);
-void intel_gt_driver_late_release(struct intel_gt *gt); +void intel_gt_driver_late_release(struct drm_i915_private *i915);
int intel_gt_wait_for_idle(struct intel_gt *gt, long timeout);
@@ -86,6 +84,16 @@ static inline bool intel_gt_needs_read_steering(struct intel_gt *gt,
u32 intel_gt_read_register_fw(struct intel_gt *gt, i915_reg_t reg);
+int intel_gt_probe_all(struct drm_i915_private *i915); +int intel_gt_tiles_init(struct drm_i915_private *i915); +void intel_gt_release_all(struct drm_i915_private *i915);
+#define for_each_gt(gt__, i915__, id__) \
- for ((id__) = 0; \
(id__) < I915_MAX_GT; \(id__)++) \for_each_if(((gt__) = (i915__)->gt[(id__)]))- void intel_gt_info_print(const struct intel_gt_info *info, struct drm_printer *p);
diff --git a/drivers/gpu/drm/i915/gt/intel_gt_pm.c b/drivers/gpu/drm/i915/gt/intel_gt_pm.c index c0fa41e4c803..e66479d33bc3 100644 --- a/drivers/gpu/drm/i915/gt/intel_gt_pm.c +++ b/drivers/gpu/drm/i915/gt/intel_gt_pm.c @@ -128,7 +128,14 @@ static const struct intel_wakeref_ops wf_ops = {
void intel_gt_pm_init_early(struct intel_gt *gt) {
- intel_wakeref_init(>->wakeref, gt->uncore->rpm, &wf_ops);
- /*
* We access the runtime_pm structure via gt->i915 here rather than* gt->uncore as we do elsewhere in the file because gt->uncore is not* yet initialized for all tiles at this point in the driver startup.* runtime_pm is per-device rather than per-tile, so this is still the* correct structure.*/- intel_wakeref_init(>->wakeref, >->i915->runtime_pm, &wf_ops); seqcount_mutex_init(>->stats.lock, >->wakeref.mutex); }
diff --git a/drivers/gpu/drm/i915/gt/intel_gt_types.h b/drivers/gpu/drm/i915/gt/intel_gt_types.h index f20687796490..89fad770b2d4 100644 --- a/drivers/gpu/drm/i915/gt/intel_gt_types.h +++ b/drivers/gpu/drm/i915/gt/intel_gt_types.h @@ -182,7 +182,14 @@ struct intel_gt {
const struct intel_mmio_range *steering_table[NUM_STEERING_TYPES];
/*
* Base of per-tile GTTMMADR where we can derive the MMIO and the GGTT.*/phys_addr_t phys_addr;
struct intel_gt_info {
unsigned int id;intel_engine_mask_t engine_mask;
u32 l3bank_mask;
diff --git a/drivers/gpu/drm/i915/i915_driver.c b/drivers/gpu/drm/i915/i915_driver.c index 1c67ff735f18..144f989e4fef 100644 --- a/drivers/gpu/drm/i915/i915_driver.c +++ b/drivers/gpu/drm/i915/i915_driver.c @@ -320,9 +320,8 @@ static int i915_driver_early_probe(struct drm_i915_private *dev_priv) intel_device_info_subplatform_init(dev_priv); intel_step_init(dev_priv);
- intel_gt_init_early(to_gt(dev_priv), dev_priv);
- /* All tiles share a single mmio_debug */
So why are we allocating mmio_debug in intel_gt_tile_setup ?
intel_uncore_mmio_debug_init_early(&dev_priv->mmio_debug);
intel_uncore_init_early(&dev_priv->uncore, to_gt(dev_priv));
spin_lock_init(&dev_priv->irq_lock); spin_lock_init(&dev_priv->gpu_error.lock);
@@ -353,7 +352,7 @@ static int i915_driver_early_probe(struct drm_i915_private *dev_priv)
intel_wopcm_init_early(&dev_priv->wopcm);
- __intel_gt_init_early(to_gt(dev_priv), dev_priv);
intel_root_gt_init_early(dev_priv);
i915_gem_init_early(dev_priv);
@@ -374,7 +373,7 @@ static int i915_driver_early_probe(struct drm_i915_private *dev_priv)
err_gem: i915_gem_cleanup_early(dev_priv);
- intel_gt_driver_late_release(to_gt(dev_priv));
- intel_gt_driver_late_release(dev_priv); intel_region_ttm_device_fini(dev_priv); err_ttm: vlv_suspend_cleanup(dev_priv);
@@ -393,7 +392,7 @@ static void i915_driver_late_release(struct drm_i915_private *dev_priv) intel_irq_fini(dev_priv); intel_power_domains_cleanup(dev_priv); i915_gem_cleanup_early(dev_priv);
- intel_gt_driver_late_release(to_gt(dev_priv));
- intel_gt_driver_late_release(dev_priv); intel_region_ttm_device_fini(dev_priv); vlv_suspend_cleanup(dev_priv); i915_workqueues_cleanup(dev_priv);
@@ -424,13 +423,9 @@ static int i915_driver_mmio_probe(struct drm_i915_private *dev_priv) if (ret < 0) return ret;
- ret = intel_uncore_setup_mmio(&dev_priv->uncore);
- if (ret < 0)
goto err_bridge;- ret = intel_uncore_init_mmio(&dev_priv->uncore); if (ret)
goto err_mmio;
return ret;/* Try to make sure MCHBAR is enabled before poking at it */ intel_setup_mchbar(dev_priv);
@@ -448,9 +443,6 @@ static int i915_driver_mmio_probe(struct drm_i915_private *dev_priv) err_uncore: intel_teardown_mchbar(dev_priv); intel_uncore_fini_mmio(&dev_priv->uncore); -err_mmio:
- intel_uncore_cleanup_mmio(&dev_priv->uncore);
-err_bridge: pci_dev_put(dev_priv->bridge_dev);
return ret; @@ -464,7 +456,6 @@ static void i915_driver_mmio_release(struct drm_i915_private *dev_priv) { intel_teardown_mchbar(dev_priv); intel_uncore_fini_mmio(&dev_priv->uncore);
- intel_uncore_cleanup_mmio(&dev_priv->uncore); pci_dev_put(dev_priv->bridge_dev); }
@@ -597,7 +588,7 @@ static int i915_driver_hw_probe(struct drm_i915_private *dev_priv) if (ret) goto err_ggtt;
- ret = intel_gt_probe_lmem(to_gt(dev_priv));
- ret = intel_gt_tiles_init(dev_priv); if (ret) goto err_mem_regions;
@@ -862,10 +853,14 @@ int i915_driver_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
intel_vgpu_detect(i915);
- ret = i915_driver_mmio_probe(i915);
ret = intel_gt_probe_all(i915); if (ret < 0) goto out_runtime_pm_put;
ret = i915_driver_mmio_probe(i915);
if (ret < 0)
goto out_tiles_cleanup;ret = i915_driver_hw_probe(i915); if (ret < 0) goto out_cleanup_mmio;
@@ -922,6 +917,8 @@ int i915_driver_probe(struct pci_dev *pdev, const struct pci_device_id *ent) i915_ggtt_driver_late_release(i915); out_cleanup_mmio: i915_driver_mmio_release(i915); +out_tiles_cleanup:
- intel_gt_release_all(i915); out_runtime_pm_put: enable_rpm_wakeref_asserts(&i915->runtime_pm); i915_driver_late_release(i915);
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index 418091484e02..88a83cd81ddd 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -805,6 +805,12 @@ struct drm_i915_private { /* Abstract the submission mechanism (legacy ringbuffer or execlists) away */ struct intel_gt gt0;
- /*
* i915->gt[0] == &i915->gt0*/+#define I915_MAX_GT 4
- struct intel_gt *gt[I915_MAX_GT];
- struct { struct i915_gem_contexts { spinlock_t lock; /* locks list */
diff --git a/drivers/gpu/drm/i915/intel_memory_region.h b/drivers/gpu/drm/i915/intel_memory_region.h index 95db0a8029e2..9548335344be 100644 --- a/drivers/gpu/drm/i915/intel_memory_region.h +++ b/drivers/gpu/drm/i915/intel_memory_region.h @@ -30,6 +30,9 @@ enum intel_memory_type { enum intel_region_id { INTEL_REGION_SMEM = 0, INTEL_REGION_LMEM_0,
- INTEL_REGION_LMEM_1,
- INTEL_REGION_LMEM_2,
- INTEL_REGION_LMEM_3, INTEL_REGION_STOLEN_SMEM, INTEL_REGION_STOLEN_LMEM, INTEL_REGION_UNKNOWN, /* Should be last */
diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c index dd8fdd5863de..75d9fa21923e 100644 --- a/drivers/gpu/drm/i915/intel_uncore.c +++ b/drivers/gpu/drm/i915/intel_uncore.c @@ -2039,14 +2039,11 @@ static int i915_pmic_bus_access_notifier(struct notifier_block *nb, return NOTIFY_OK; }
-int intel_uncore_setup_mmio(struct intel_uncore *uncore) +int intel_uncore_setup_mmio(struct intel_uncore *uncore, phys_addr_t phys_addr) { struct drm_i915_private *i915 = uncore->i915;
struct pci_dev *pdev = to_pci_dev(i915->drm.dev);
int mmio_bar; int mmio_size;
mmio_bar = GRAPHICS_VER(i915) == 2 ? 1 : 0; /*
- Before gen4, the registers and the GTT are behind different BARs.
- However, from gen4 onwards, the registers and the GTT are shared
@@ -2063,7 +2060,7 @@ int intel_uncore_setup_mmio(struct intel_uncore *uncore) else mmio_size = 2 * 1024 * 1024;
- uncore->regs = pci_iomap(pdev, mmio_bar, mmio_size);
- uncore->regs = ioremap(phys_addr, mmio_size); if (uncore->regs == NULL) { drm_err(&i915->drm, "failed to map registers\n"); return -EIO;
@@ -2074,9 +2071,8 @@ int intel_uncore_setup_mmio(struct intel_uncore *uncore)
void intel_uncore_cleanup_mmio(struct intel_uncore *uncore) {
- struct pci_dev *pdev = to_pci_dev(uncore->i915->drm.dev);
- pci_iounmap(pdev, uncore->regs);
- if (uncore->regs)
iounmap(uncore->regs);
'if' is not necessary, up to you.
Regards Andrzej
}
void intel_uncore_init_early(struct intel_uncore *uncore, diff --git a/drivers/gpu/drm/i915/intel_uncore.h b/drivers/gpu/drm/i915/intel_uncore.h index 6ff56d673e2b..de53c961fadb 100644 --- a/drivers/gpu/drm/i915/intel_uncore.h +++ b/drivers/gpu/drm/i915/intel_uncore.h @@ -29,6 +29,7 @@ #include <linux/notifier.h> #include <linux/hrtimer.h> #include <linux/io-64-nonatomic-lo-hi.h> +#include <linux/types.h>
#include "i915_reg_defs.h"
@@ -219,7 +220,7 @@ void intel_uncore_mmio_debug_init_early(struct intel_uncore_mmio_debug *mmio_debug); void intel_uncore_init_early(struct intel_uncore *uncore, struct intel_gt *gt); -int intel_uncore_setup_mmio(struct intel_uncore *uncore); +int intel_uncore_setup_mmio(struct intel_uncore *uncore, phys_addr_t phys_addr); int intel_uncore_init_mmio(struct intel_uncore *uncore); void intel_uncore_prune_engine_fw_domains(struct intel_uncore *uncore, struct intel_gt *gt); diff --git a/drivers/gpu/drm/i915/selftests/mock_gem_device.c b/drivers/gpu/drm/i915/selftests/mock_gem_device.c index 573d9b2e1a4a..f72436c5d596 100644 --- a/drivers/gpu/drm/i915/selftests/mock_gem_device.c +++ b/drivers/gpu/drm/i915/selftests/mock_gem_device.c @@ -73,7 +73,7 @@ static void mock_device_release(struct drm_device *dev) destroy_workqueue(i915->wq);
intel_region_ttm_device_fini(i915);
- intel_gt_driver_late_release(to_gt(i915));
intel_gt_driver_late_release(i915); intel_memory_regions_driver_release(i915);
drm_mode_config_cleanup(&i915->drm);
@@ -180,8 +180,7 @@ struct drm_i915_private *mock_gem_device(void) spin_lock_init(&i915->gpu_error.lock);
i915_gem_init__mm(i915);
- intel_gt_init_early(to_gt(i915), i915);
- __intel_gt_init_early(to_gt(i915), i915);
- intel_root_gt_init_early(i915); mock_uncore_init(&i915->uncore, i915); atomic_inc(&to_gt(i915)->wakeref.count); /* disable; no hw support */ to_gt(i915)->awake = -ENODEV;
@@ -229,7 +228,7 @@ struct drm_i915_private *mock_gem_device(void) err_drv: intel_region_ttm_device_fini(i915); err_ttm:
- intel_gt_driver_late_release(to_gt(i915));
- intel_gt_driver_late_release(i915); intel_memory_regions_driver_release(i915); drm_mode_config_cleanup(&i915->drm); mock_destroy_device(i915);
Hi Andrzej,
[...]
-int intel_gt_probe_lmem(struct intel_gt *gt) +static int intel_gt_probe_lmem(struct intel_gt *gt) { struct drm_i915_private *i915 = gt->i915;
- unsigned int instance = gt->info.id; struct intel_memory_region *mem; int id; int err;
- id = INTEL_REGION_LMEM_0 + instance;
- if (drm_WARN_ON(&i915->drm, id >= INTEL_REGION_STOLEN_SMEM))
Do we need to check id correctness? wouldn't be enough to check it on initialization of gt->info.id. If yes, maybe (id > INTEL_REGION_LMEM_3) would be more readable, or (info.id < MAX_GT), up to you.
yes, it's indeed redundant. Also because if that 'if' was true it would be a bit more catastrophic than a simple warning. I will remove it completely.
[...]
- if (id) {
struct intel_uncore_mmio_debug *mmio_debug;struct intel_uncore *uncore;uncore = kzalloc(sizeof(*uncore), GFP_KERNEL);if (!gt->uncore)return -ENOMEM;s/gt->uncore/uncore/
thanks!
[...]
+static void +intel_gt_tile_cleanup(struct intel_gt *gt) +{
- intel_uncore_cleanup_mmio(gt->uncore);
- if (gt->info.id) {
kfree(gt->uncore);kfree(gt);What about gt->uncore->debug ?
you don't want to leak anything? :)
will add it, nice catch! Thanks!
[...]
diff --git a/drivers/gpu/drm/i915/i915_driver.c b/drivers/gpu/drm/i915/i915_driver.c index 1c67ff735f18..144f989e4fef 100644 --- a/drivers/gpu/drm/i915/i915_driver.c +++ b/drivers/gpu/drm/i915/i915_driver.c @@ -320,9 +320,8 @@ static int i915_driver_early_probe(struct drm_i915_private *dev_priv) intel_device_info_subplatform_init(dev_priv); intel_step_init(dev_priv);
- intel_gt_init_early(to_gt(dev_priv), dev_priv);
- /* All tiles share a single mmio_debug */
So why are we allocating mmio_debug in intel_gt_tile_setup ?
yes... this is a leftover from previous development cycles... I will remove the comment. Indeed this goes only to tile 0.
[...]
void intel_uncore_cleanup_mmio(struct intel_uncore *uncore) {
- struct pci_dev *pdev = to_pci_dev(uncore->i915->drm.dev);
- pci_iounmap(pdev, uncore->regs);
- if (uncore->regs)
iounmap(uncore->regs);'if' is not necessary, up to you.
will remove, thanks!
[...]
Thank you for the review! Andi
The "gt_is_root(struct intel_gt *gt)" helper return true if the gt is the root gt, which means that its id is 0. Return false otherwise.
Suggested-by: Michal Wajdeczko michal.wajdeczko@intel.com Signed-off-by: Andi Shyti andi.shyti@linux.intel.com --- drivers/gpu/drm/i915/gt/intel_gt.h | 5 +++++ 1 file changed, 5 insertions(+)
diff --git a/drivers/gpu/drm/i915/gt/intel_gt.h b/drivers/gpu/drm/i915/gt/intel_gt.h index 915d6192079b..f17f51e2d394 100644 --- a/drivers/gpu/drm/i915/gt/intel_gt.h +++ b/drivers/gpu/drm/i915/gt/intel_gt.h @@ -19,6 +19,11 @@ struct drm_printer; ##__VA_ARGS__); \ } while (0)
+static inline bool gt_is_root(struct intel_gt *gt) +{ + return !gt->info.id; +} + static inline struct intel_gt *uc_to_gt(struct intel_uc *uc) { return container_of(uc, struct intel_gt, uc);
On 17.02.2022 15:41, Andi Shyti wrote:
The "gt_is_root(struct intel_gt *gt)" helper return true if the gt is the root gt, which means that its id is 0. Return false otherwise.
Suggested-by: Michal Wajdeczko michal.wajdeczko@intel.com Signed-off-by: Andi Shyti andi.shyti@linux.intel.com
drivers/gpu/drm/i915/gt/intel_gt.h | 5 +++++ 1 file changed, 5 insertions(+)
diff --git a/drivers/gpu/drm/i915/gt/intel_gt.h b/drivers/gpu/drm/i915/gt/intel_gt.h index 915d6192079b..f17f51e2d394 100644 --- a/drivers/gpu/drm/i915/gt/intel_gt.h +++ b/drivers/gpu/drm/i915/gt/intel_gt.h @@ -19,6 +19,11 @@ struct drm_printer; ##__VA_ARGS__); \ } while (0)
+static inline bool gt_is_root(struct intel_gt *gt) +{
- return !gt->info.id;
+}
we could squash this patch with prev one, where it can be used in:
intel_gt_tile_cleanup(struct intel_gt *gt) { intel_uncore_cleanup_mmio(gt->uncore);
- if (gt->info.id) { + if (!gt_is_root(gt)) { kfree(gt->uncore); kfree(gt); } }
or just use it this way in this patch, with that:
Reviewed-by: Michal Wajdeczko michal.wajdeczko@intel.com
static inline struct intel_gt *uc_to_gt(struct intel_uc *uc) { return container_of(uc, struct intel_gt, uc);
On 28.02.2022 21:02, Michal Wajdeczko wrote:
On 17.02.2022 15:41, Andi Shyti wrote:
The "gt_is_root(struct intel_gt *gt)" helper return true if the gt is the root gt, which means that its id is 0. Return false otherwise.
Suggested-by: Michal Wajdeczko michal.wajdeczko@intel.com Signed-off-by: Andi Shyti andi.shyti@linux.intel.com
drivers/gpu/drm/i915/gt/intel_gt.h | 5 +++++ 1 file changed, 5 insertions(+)
diff --git a/drivers/gpu/drm/i915/gt/intel_gt.h b/drivers/gpu/drm/i915/gt/intel_gt.h index 915d6192079b..f17f51e2d394 100644 --- a/drivers/gpu/drm/i915/gt/intel_gt.h +++ b/drivers/gpu/drm/i915/gt/intel_gt.h @@ -19,6 +19,11 @@ struct drm_printer; ##__VA_ARGS__); \ } while (0)
+static inline bool gt_is_root(struct intel_gt *gt) +{
- return !gt->info.id;
+}
we could squash this patch with prev one, where it can be used in:
intel_gt_tile_cleanup(struct intel_gt *gt) { intel_uncore_cleanup_mmio(gt->uncore);
- if (gt->info.id) {
- if (!gt_is_root(gt)) { kfree(gt->uncore); kfree(gt); } }
It can be used in intel_gt_tile_setup as well, and then you can remove id var.
or just use it this way in this patch, with that:
Reviewed-by: Michal Wajdeczko michal.wajdeczko@intel.com
Accordingly:
Reviewed-by: Andrzej Hajda andrzej.hajda@intel.com
Regards Andrzej
static inline struct intel_gt *uc_to_gt(struct intel_uc *uc) { return container_of(uc, struct intel_gt, uc);
Hi Andrzej and Michal,
The "gt_is_root(struct intel_gt *gt)" helper return true if the gt is the root gt, which means that its id is 0. Return false otherwise.
Suggested-by: Michal Wajdeczko michal.wajdeczko@intel.com Signed-off-by: Andi Shyti andi.shyti@linux.intel.com
drivers/gpu/drm/i915/gt/intel_gt.h | 5 +++++ 1 file changed, 5 insertions(+)
diff --git a/drivers/gpu/drm/i915/gt/intel_gt.h b/drivers/gpu/drm/i915/gt/intel_gt.h index 915d6192079b..f17f51e2d394 100644 --- a/drivers/gpu/drm/i915/gt/intel_gt.h +++ b/drivers/gpu/drm/i915/gt/intel_gt.h @@ -19,6 +19,11 @@ struct drm_printer; ##__VA_ARGS__); \ } while (0) +static inline bool gt_is_root(struct intel_gt *gt) +{
- return !gt->info.id;
+}
we could squash this patch with prev one, where it can be used in:
intel_gt_tile_cleanup(struct intel_gt *gt) { intel_uncore_cleanup_mmio(gt->uncore);
- if (gt->info.id) {
- if (!gt_is_root(gt)) { kfree(gt->uncore); kfree(gt); } }
It can be used in intel_gt_tile_setup as well, and then you can remove id var.
I will move this before, then before patch 2 and use it where you suggested, as well.
or just use it this way in this patch, with that:
Reviewed-by: Michal Wajdeczko michal.wajdeczko@intel.com
Accordingly:
Reviewed-by: Andrzej Hajda andrzej.hajda@intel.com
Thank you!
Andi
Now that we have tiles we want each of them to have its own interface. A directory "gt/" is created under "cardN/" that will contain as many diroctories as the tiles.
In the coming patches tile related interfaces will be added. For now the sysfs gt structure simply has an id interface related to the current tile count.
The directory structure will follow this scheme:
/sys/.../card0 └── gt ├── gt0 │ └── id : : └─- gtN └── id
This new set of interfaces will be a basic tool for system managers and administrators when using i915.
Signed-off-by: Andi Shyti andi.shyti@linux.intel.com Cc: Matt Roper matthew.d.roper@intel.com Cc: Sujaritha Sundaresan sujaritha.sundaresan@intel.com Cc: Tvrtko Ursulin tvrtko.ursulin@intel.com Reviewed-by: Sujaritha Sundaresan sujaritha.sundaresan@intel.com --- drivers/gpu/drm/i915/Makefile | 1 + drivers/gpu/drm/i915/gt/intel_gt.c | 2 + drivers/gpu/drm/i915/gt/intel_gt_sysfs.c | 118 +++++++++++++++++++++++ drivers/gpu/drm/i915/gt/intel_gt_sysfs.h | 34 +++++++ drivers/gpu/drm/i915/i915_drv.h | 2 + drivers/gpu/drm/i915/i915_sysfs.c | 12 ++- drivers/gpu/drm/i915/i915_sysfs.h | 3 + 7 files changed, 171 insertions(+), 1 deletion(-) create mode 100644 drivers/gpu/drm/i915/gt/intel_gt_sysfs.c create mode 100644 drivers/gpu/drm/i915/gt/intel_gt_sysfs.h
diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile index 9d588d936e3d..277064b00afd 100644 --- a/drivers/gpu/drm/i915/Makefile +++ b/drivers/gpu/drm/i915/Makefile @@ -105,6 +105,7 @@ gt-y += \ gt/intel_gt_pm_debugfs.o \ gt/intel_gt_pm_irq.o \ gt/intel_gt_requests.o \ + gt/intel_gt_sysfs.o \ gt/intel_gtt.o \ gt/intel_llc.o \ gt/intel_lrc.o \ diff --git a/drivers/gpu/drm/i915/gt/intel_gt.c b/drivers/gpu/drm/i915/gt/intel_gt.c index 8c64b81e9ec9..0f080bbad043 100644 --- a/drivers/gpu/drm/i915/gt/intel_gt.c +++ b/drivers/gpu/drm/i915/gt/intel_gt.c @@ -26,6 +26,7 @@ #include "intel_rc6.h" #include "intel_renderstate.h" #include "intel_rps.h" +#include "intel_gt_sysfs.h" #include "intel_uncore.h" #include "shmem_utils.h"
@@ -458,6 +459,7 @@ void intel_gt_driver_register(struct intel_gt *gt) intel_rps_driver_register(>->rps);
intel_gt_debugfs_register(gt); + intel_gt_sysfs_register(gt); }
static int intel_gt_init_scratch(struct intel_gt *gt, unsigned int size) diff --git a/drivers/gpu/drm/i915/gt/intel_gt_sysfs.c b/drivers/gpu/drm/i915/gt/intel_gt_sysfs.c new file mode 100644 index 000000000000..0206e9aa4867 --- /dev/null +++ b/drivers/gpu/drm/i915/gt/intel_gt_sysfs.c @@ -0,0 +1,118 @@ +// SPDX-License-Identifier: MIT +/* + * Copyright © 2022 Intel Corporation + */ + +#include <drm/drm_device.h> +#include <linux/device.h> +#include <linux/kobject.h> +#include <linux/printk.h> +#include <linux/sysfs.h> + +#include "i915_drv.h" +#include "i915_sysfs.h" +#include "intel_gt.h" +#include "intel_gt_sysfs.h" +#include "intel_gt_types.h" +#include "intel_rc6.h" + +bool is_object_gt(struct kobject *kobj) +{ + return !strncmp(kobj->name, "gt", 2); +} + +static struct intel_gt *kobj_to_gt(struct kobject *kobj) +{ + return container_of(kobj, struct kobj_gt, base)->gt; +} + +struct intel_gt *intel_gt_sysfs_get_drvdata(struct device *dev, + const char *name) +{ + struct kobject *kobj = &dev->kobj; + + /* + * We are interested at knowing from where the interface + * has been called, whether it's called from gt/ or from + * the parent directory. + * From the interface position it depends also the value of + * the private data. + * If the interface is called from gt/ then private data is + * of the "struct intel_gt *" type, otherwise it's * a + * "struct drm_i915_private *" type. + */ + if (!is_object_gt(kobj)) { + struct drm_i915_private *i915 = kdev_minor_to_i915(dev); + + pr_devel_ratelimited(DEPRECATED + "%s (pid %d) is accessing deprecated %s " + "sysfs control, please use gt/gt<n>/%s instead\n", + current->comm, task_pid_nr(current), name, name); + return to_gt(i915); + } + + return kobj_to_gt(kobj); +} + +static ssize_t id_show(struct device *dev, + struct device_attribute *attr, + char *buf) +{ + struct intel_gt *gt = intel_gt_sysfs_get_drvdata(dev, attr->attr.name); + + return sysfs_emit(buf, "%u\n", gt->info.id); +} +static DEVICE_ATTR_RO(id); + +static struct attribute *id_attrs[] = { + &dev_attr_id.attr, + NULL, +}; +ATTRIBUTE_GROUPS(id); + +static void kobj_gt_release(struct kobject *kobj) +{ + kfree(kobj); +} + +static struct kobj_type kobj_gt_type = { + .release = kobj_gt_release, + .sysfs_ops = &kobj_sysfs_ops, + .default_groups = id_groups, +}; + +struct kobject * +intel_gt_create_kobj(struct intel_gt *gt, struct kobject *dir, const char *name) +{ + struct kobj_gt *kg; + + kg = kzalloc(sizeof(*kg), GFP_KERNEL); + if (!kg) + return NULL; + + kobject_init(&kg->base, &kobj_gt_type); + kg->gt = gt; + + /* xfer ownership to sysfs tree */ + if (kobject_add(&kg->base, dir, "%s", name)) { + kobject_put(&kg->base); + return NULL; + } + + return &kg->base; /* borrowed ref */ +} + +void intel_gt_sysfs_register(struct intel_gt *gt) +{ + struct kobject *dir; + char name[80]; + + snprintf(name, sizeof(name), "gt%d", gt->info.id); + + dir = intel_gt_create_kobj(gt, gt->i915->sysfs_gt, name); + if (!dir) { + drm_warn(>->i915->drm, + "failed to initialize %s sysfs root\n", name); + return; + } +} diff --git a/drivers/gpu/drm/i915/gt/intel_gt_sysfs.h b/drivers/gpu/drm/i915/gt/intel_gt_sysfs.h new file mode 100644 index 000000000000..9471b26752cf --- /dev/null +++ b/drivers/gpu/drm/i915/gt/intel_gt_sysfs.h @@ -0,0 +1,34 @@ +/* SPDX-License-Identifier: MIT */ +/* + * Copyright © 2022 Intel Corporation + */ + +#ifndef __SYSFS_GT_H__ +#define __SYSFS_GT_H__ + +#include <linux/ctype.h> +#include <linux/kobject.h> + +#include "i915_gem.h" /* GEM_BUG_ON() */ + +struct intel_gt; + +struct kobj_gt { + struct kobject base; + struct intel_gt *gt; +}; + +bool is_object_gt(struct kobject *kobj); + +struct drm_i915_private *kobj_to_i915(struct kobject *kobj); + +struct kobject * +intel_gt_create_kobj(struct intel_gt *gt, + struct kobject *dir, + const char *name); + +void intel_gt_sysfs_register(struct intel_gt *gt); +struct intel_gt *intel_gt_sysfs_get_drvdata(struct device *dev, + const char *name); + +#endif /* SYSFS_GT_H */ diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index 88a83cd81ddd..e8c729f2ae00 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -811,6 +811,8 @@ struct drm_i915_private { #define I915_MAX_GT 4 struct intel_gt *gt[I915_MAX_GT];
+ struct kobject *sysfs_gt; + struct { struct i915_gem_contexts { spinlock_t lock; /* locks list */ diff --git a/drivers/gpu/drm/i915/i915_sysfs.c b/drivers/gpu/drm/i915/i915_sysfs.c index a4d1759375b9..3643efde52ca 100644 --- a/drivers/gpu/drm/i915/i915_sysfs.c +++ b/drivers/gpu/drm/i915/i915_sysfs.c @@ -39,7 +39,7 @@ #include "i915_sysfs.h" #include "intel_pm.h"
-static inline struct drm_i915_private *kdev_minor_to_i915(struct device *kdev) +struct drm_i915_private *kdev_minor_to_i915(struct device *kdev) { struct drm_minor *minor = dev_get_drvdata(kdev); return to_i915(minor->dev); @@ -487,6 +487,11 @@ static void i915_setup_error_capture(struct device *kdev) {} static void i915_teardown_error_capture(struct device *kdev) {} #endif
+static struct kobject *i915_setup_gt_sysfs(struct kobject *parent) +{ + return kobject_create_and_add("gt", parent); +} + void i915_setup_sysfs(struct drm_i915_private *dev_priv) { struct device *kdev = dev_priv->drm.primary->kdev; @@ -538,6 +543,11 @@ void i915_setup_sysfs(struct drm_i915_private *dev_priv) if (ret) drm_err(&dev_priv->drm, "RPS sysfs setup failed\n");
+ dev_priv->sysfs_gt = i915_setup_gt_sysfs(&kdev->kobj); + if (!dev_priv->sysfs_gt) + drm_warn(&dev_priv->drm, + "failed to register GT sysfs directory\n"); + i915_setup_error_capture(kdev);
intel_engines_add_sysfs(dev_priv); diff --git a/drivers/gpu/drm/i915/i915_sysfs.h b/drivers/gpu/drm/i915/i915_sysfs.h index 41afd4366416..243a17741e3f 100644 --- a/drivers/gpu/drm/i915/i915_sysfs.h +++ b/drivers/gpu/drm/i915/i915_sysfs.h @@ -6,8 +6,11 @@ #ifndef __I915_SYSFS_H__ #define __I915_SYSFS_H__
+struct device; struct drm_i915_private;
+struct drm_i915_private *kdev_minor_to_i915(struct device *kdev); + void i915_setup_sysfs(struct drm_i915_private *i915); void i915_teardown_sysfs(struct drm_i915_private *i915);
On 17.02.2022 15:41, Andi Shyti wrote:
Now that we have tiles we want each of them to have its own interface. A directory "gt/" is created under "cardN/" that will contain as many diroctories as the tiles.
In the coming patches tile related interfaces will be added. For now the sysfs gt structure simply has an id interface related to the current tile count.
The directory structure will follow this scheme:
/sys/.../card0 └── gt ├── gt0 │ └── id : : └─- gtN └── idThis new set of interfaces will be a basic tool for system managers and administrators when using i915.
Signed-off-by: Andi Shyti andi.shyti@linux.intel.com Cc: Matt Roper matthew.d.roper@intel.com Cc: Sujaritha Sundaresan sujaritha.sundaresan@intel.com Cc: Tvrtko Ursulin tvrtko.ursulin@intel.com Reviewed-by: Sujaritha Sundaresan sujaritha.sundaresan@intel.com
drivers/gpu/drm/i915/Makefile | 1 + drivers/gpu/drm/i915/gt/intel_gt.c | 2 + drivers/gpu/drm/i915/gt/intel_gt_sysfs.c | 118 +++++++++++++++++++++++ drivers/gpu/drm/i915/gt/intel_gt_sysfs.h | 34 +++++++ drivers/gpu/drm/i915/i915_drv.h | 2 + drivers/gpu/drm/i915/i915_sysfs.c | 12 ++- drivers/gpu/drm/i915/i915_sysfs.h | 3 + 7 files changed, 171 insertions(+), 1 deletion(-) create mode 100644 drivers/gpu/drm/i915/gt/intel_gt_sysfs.c create mode 100644 drivers/gpu/drm/i915/gt/intel_gt_sysfs.h
diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile index 9d588d936e3d..277064b00afd 100644 --- a/drivers/gpu/drm/i915/Makefile +++ b/drivers/gpu/drm/i915/Makefile @@ -105,6 +105,7 @@ gt-y += \ gt/intel_gt_pm_debugfs.o \ gt/intel_gt_pm_irq.o \ gt/intel_gt_requests.o \
- gt/intel_gt_sysfs.o \ gt/intel_gtt.o \ gt/intel_llc.o \ gt/intel_lrc.o \
diff --git a/drivers/gpu/drm/i915/gt/intel_gt.c b/drivers/gpu/drm/i915/gt/intel_gt.c index 8c64b81e9ec9..0f080bbad043 100644 --- a/drivers/gpu/drm/i915/gt/intel_gt.c +++ b/drivers/gpu/drm/i915/gt/intel_gt.c @@ -26,6 +26,7 @@ #include "intel_rc6.h" #include "intel_renderstate.h" #include "intel_rps.h" +#include "intel_gt_sysfs.h" #include "intel_uncore.h" #include "shmem_utils.h"
@@ -458,6 +459,7 @@ void intel_gt_driver_register(struct intel_gt *gt) intel_rps_driver_register(>->rps);
intel_gt_debugfs_register(gt);
intel_gt_sysfs_register(gt); }
static int intel_gt_init_scratch(struct intel_gt *gt, unsigned int size)
diff --git a/drivers/gpu/drm/i915/gt/intel_gt_sysfs.c b/drivers/gpu/drm/i915/gt/intel_gt_sysfs.c new file mode 100644 index 000000000000..0206e9aa4867 --- /dev/null +++ b/drivers/gpu/drm/i915/gt/intel_gt_sysfs.c @@ -0,0 +1,118 @@ +// SPDX-License-Identifier: MIT +/*
- Copyright © 2022 Intel Corporation
- */
+#include <drm/drm_device.h> +#include <linux/device.h> +#include <linux/kobject.h> +#include <linux/printk.h> +#include <linux/sysfs.h>
+#include "i915_drv.h" +#include "i915_sysfs.h" +#include "intel_gt.h" +#include "intel_gt_sysfs.h" +#include "intel_gt_types.h" +#include "intel_rc6.h"
+bool is_object_gt(struct kobject *kobj) +{
- return !strncmp(kobj->name, "gt", 2);
+}
It looks quite fragile, at the moment I do not have better idea:) maybe after reviewing the rest of the patches.
+static struct intel_gt *kobj_to_gt(struct kobject *kobj) +{
- return container_of(kobj, struct kobj_gt, base)->gt;
+}
+struct intel_gt *intel_gt_sysfs_get_drvdata(struct device *dev,
const char *name)+{
- struct kobject *kobj = &dev->kobj;
- /*
* We are interested at knowing from where the interface* has been called, whether it's called from gt/ or from* the parent directory.* From the interface position it depends also the value of* the private data.* If the interface is called from gt/ then private data is* of the "struct intel_gt *" type, otherwise it's * a* "struct drm_i915_private *" type.*/- if (!is_object_gt(kobj)) {
struct drm_i915_private *i915 = kdev_minor_to_i915(dev);pr_devel_ratelimited(DEPRECATED"%s (pid %d) is accessing deprecated %s ""sysfs control, please use gt/gt<n>/%s instead\n",current->comm, task_pid_nr(current), name, name);return to_gt(i915);- }
- return kobj_to_gt(kobj);
It took some time for me to understand what is going on here. We have dev argument which sometimes can point to "struct device", sometimes to "struct kobj_gt", but it's type suggests differently, quite ugly. I wonder if wouldn't be better to use __ATTR instead of DEVICE_ATTR* as in case of intel_engines_add_sysfs. This way abstractions would look better, hopefully.
+}
+static ssize_t id_show(struct device *dev,
struct device_attribute *attr,char *buf)+{
- struct intel_gt *gt = intel_gt_sysfs_get_drvdata(dev, attr->attr.name);
- return sysfs_emit(buf, "%u\n", gt->info.id);
+} +static DEVICE_ATTR_RO(id);
+static struct attribute *id_attrs[] = {
- &dev_attr_id.attr,
- NULL,
+}; +ATTRIBUTE_GROUPS(id);
+static void kobj_gt_release(struct kobject *kobj) +{
- kfree(kobj);
+}
+static struct kobj_type kobj_gt_type = {
- .release = kobj_gt_release,
- .sysfs_ops = &kobj_sysfs_ops,
- .default_groups = id_groups,
+};
+struct kobject * +intel_gt_create_kobj(struct intel_gt *gt, struct kobject *dir, const char *name) +{
- struct kobj_gt *kg;
- kg = kzalloc(sizeof(*kg), GFP_KERNEL);
- if (!kg)
return NULL;- kobject_init(&kg->base, &kobj_gt_type);
- kg->gt = gt;
- /* xfer ownership to sysfs tree */
- if (kobject_add(&kg->base, dir, "%s", name)) {
kobject_put(&kg->base);return NULL;- }
- return &kg->base; /* borrowed ref */
+}
+void intel_gt_sysfs_register(struct intel_gt *gt) +{
- struct kobject *dir;
- char name[80];
- snprintf(name, sizeof(name), "gt%d", gt->info.id);
- dir = intel_gt_create_kobj(gt, gt->i915->sysfs_gt, name);
- if (!dir) {
drm_warn(>->i915->drm,"failed to initialize %s sysfs root\n", name);return;- }
+}
Squashing intel_gt_create_kobj into intel_gt_sysfs_register would simplify code and allows drop snprintf to local array.
diff --git a/drivers/gpu/drm/i915/gt/intel_gt_sysfs.h b/drivers/gpu/drm/i915/gt/intel_gt_sysfs.h new file mode 100644 index 000000000000..9471b26752cf --- /dev/null +++ b/drivers/gpu/drm/i915/gt/intel_gt_sysfs.h @@ -0,0 +1,34 @@ +/* SPDX-License-Identifier: MIT */ +/*
- Copyright © 2022 Intel Corporation
- */
+#ifndef __SYSFS_GT_H__ +#define __SYSFS_GT_H__
+#include <linux/ctype.h> +#include <linux/kobject.h>
+#include "i915_gem.h" /* GEM_BUG_ON() */
+struct intel_gt;
+struct kobj_gt {
- struct kobject base;
- struct intel_gt *gt;
+};
+bool is_object_gt(struct kobject *kobj);
+struct drm_i915_private *kobj_to_i915(struct kobject *kobj);
+struct kobject * +intel_gt_create_kobj(struct intel_gt *gt,
struct kobject *dir,const char *name);+void intel_gt_sysfs_register(struct intel_gt *gt); +struct intel_gt *intel_gt_sysfs_get_drvdata(struct device *dev,
const char *name);+#endif /* SYSFS_GT_H */ diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index 88a83cd81ddd..e8c729f2ae00 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -811,6 +811,8 @@ struct drm_i915_private { #define I915_MAX_GT 4 struct intel_gt *gt[I915_MAX_GT];
- struct kobject *sysfs_gt;
- struct { struct i915_gem_contexts { spinlock_t lock; /* locks list */
diff --git a/drivers/gpu/drm/i915/i915_sysfs.c b/drivers/gpu/drm/i915/i915_sysfs.c index a4d1759375b9..3643efde52ca 100644 --- a/drivers/gpu/drm/i915/i915_sysfs.c +++ b/drivers/gpu/drm/i915/i915_sysfs.c @@ -39,7 +39,7 @@ #include "i915_sysfs.h" #include "intel_pm.h"
-static inline struct drm_i915_private *kdev_minor_to_i915(struct device *kdev) +struct drm_i915_private *kdev_minor_to_i915(struct device *kdev) { struct drm_minor *minor = dev_get_drvdata(kdev); return to_i915(minor->dev); @@ -487,6 +487,11 @@ static void i915_setup_error_capture(struct device *kdev) {} static void i915_teardown_error_capture(struct device *kdev) {} #endif
+static struct kobject *i915_setup_gt_sysfs(struct kobject *parent) +{
- return kobject_create_and_add("gt", parent);
+}
- void i915_setup_sysfs(struct drm_i915_private *dev_priv) { struct device *kdev = dev_priv->drm.primary->kdev;
@@ -538,6 +543,11 @@ void i915_setup_sysfs(struct drm_i915_private *dev_priv) if (ret) drm_err(&dev_priv->drm, "RPS sysfs setup failed\n");
- dev_priv->sysfs_gt = i915_setup_gt_sysfs(&kdev->kobj);
Why not directly kobject_create_and_add("gt", parent) ? up to you.
Regards Andrzej
if (!dev_priv->sysfs_gt)
drm_warn(&dev_priv->drm,"failed to register GT sysfs directory\n");i915_setup_error_capture(kdev);
intel_engines_add_sysfs(dev_priv);
diff --git a/drivers/gpu/drm/i915/i915_sysfs.h b/drivers/gpu/drm/i915/i915_sysfs.h index 41afd4366416..243a17741e3f 100644 --- a/drivers/gpu/drm/i915/i915_sysfs.h +++ b/drivers/gpu/drm/i915/i915_sysfs.h @@ -6,8 +6,11 @@ #ifndef __I915_SYSFS_H__ #define __I915_SYSFS_H__
+struct device; struct drm_i915_private;
+struct drm_i915_private *kdev_minor_to_i915(struct device *kdev);
- void i915_setup_sysfs(struct drm_i915_private *i915); void i915_teardown_sysfs(struct drm_i915_private *i915);
Hi Andrzej,
[...]
+bool is_object_gt(struct kobject *kobj) +{
- return !strncmp(kobj->name, "gt", 2);
+}
It looks quite fragile, at the moment I do not have better idea:) maybe after reviewing the rest of the patches.
yeah... it's not pretty, I agree, but I couldn't come up with a better way of doing it.
+static struct intel_gt *kobj_to_gt(struct kobject *kobj) +{
- return container_of(kobj, struct kobj_gt, base)->gt;
+}
+struct intel_gt *intel_gt_sysfs_get_drvdata(struct device *dev,
const char *name)+{
- struct kobject *kobj = &dev->kobj;
- /*
* We are interested at knowing from where the interface* has been called, whether it's called from gt/ or from* the parent directory.* From the interface position it depends also the value of* the private data.* If the interface is called from gt/ then private data is* of the "struct intel_gt *" type, otherwise it's * a* "struct drm_i915_private *" type.*/- if (!is_object_gt(kobj)) {
struct drm_i915_private *i915 = kdev_minor_to_i915(dev);pr_devel_ratelimited(DEPRECATED"%s (pid %d) is accessing deprecated %s ""sysfs control, please use gt/gt<n>/%s instead\n",current->comm, task_pid_nr(current), name, name);return to_gt(i915);- }
- return kobj_to_gt(kobj);
It took some time for me to understand what is going on here. We have dev argument which sometimes can point to "struct device", sometimes to "struct kobj_gt", but it's type suggests differently, quite ugly. I wonder if wouldn't be better to use __ATTR instead of DEVICE_ATTR* as in case of intel_engines_add_sysfs. This way abstractions would look better, hopefully.
How would it help?
The difference is that I'm adding twice different interfaces with the same name and different location (i.e. different object). The legacy intrefaces inherit the object from drm and I'm preserving that reference.
While the new objects would derive from the previous and they are pretty much like intel_engines_add_sysfs().
[...]
+struct kobject * +intel_gt_create_kobj(struct intel_gt *gt, struct kobject *dir, const char *name) +{
- struct kobj_gt *kg;
- kg = kzalloc(sizeof(*kg), GFP_KERNEL);
- if (!kg)
return NULL;- kobject_init(&kg->base, &kobj_gt_type);
- kg->gt = gt;
- /* xfer ownership to sysfs tree */
- if (kobject_add(&kg->base, dir, "%s", name)) {
kobject_put(&kg->base);return NULL;- }
- return &kg->base; /* borrowed ref */
+}
+void intel_gt_sysfs_register(struct intel_gt *gt) +{
- struct kobject *dir;
- char name[80];
- snprintf(name, sizeof(name), "gt%d", gt->info.id);
- dir = intel_gt_create_kobj(gt, gt->i915->sysfs_gt, name);
- if (!dir) {
drm_warn(>->i915->drm,"failed to initialize %s sysfs root\n", name);return;- }
+}
Squashing intel_gt_create_kobj into intel_gt_sysfs_register would simplify code and allows drop snprintf to local array.
right!
+static struct kobject *i915_setup_gt_sysfs(struct kobject *parent) +{
- return kobject_create_and_add("gt", parent);
+}
- void i915_setup_sysfs(struct drm_i915_private *dev_priv) { struct device *kdev = dev_priv->drm.primary->kdev;
@@ -538,6 +543,11 @@ void i915_setup_sysfs(struct drm_i915_private *dev_priv) if (ret) drm_err(&dev_priv->drm, "RPS sysfs setup failed\n");
- dev_priv->sysfs_gt = i915_setup_gt_sysfs(&kdev->kobj);
Why not directly kobject_create_and_add("gt", parent) ? up to you.
of course!
[...]
Thanks a lot for the review, Andi
On 07.03.2022 00:04, Andi Shyti wrote:
Hi Andrzej,
[...]
+bool is_object_gt(struct kobject *kobj) +{
- return !strncmp(kobj->name, "gt", 2);
+}
It looks quite fragile, at the moment I do not have better idea:) maybe after reviewing the rest of the patches.
yeah... it's not pretty, I agree, but I couldn't come up with a better way of doing it.
+static struct intel_gt *kobj_to_gt(struct kobject *kobj) +{
- return container_of(kobj, struct kobj_gt, base)->gt;
+}
+struct intel_gt *intel_gt_sysfs_get_drvdata(struct device *dev,
const char *name)+{
- struct kobject *kobj = &dev->kobj;
- /*
* We are interested at knowing from where the interface* has been called, whether it's called from gt/ or from* the parent directory.* From the interface position it depends also the value of* the private data.* If the interface is called from gt/ then private data is* of the "struct intel_gt *" type, otherwise it's * a* "struct drm_i915_private *" type.*/- if (!is_object_gt(kobj)) {
struct drm_i915_private *i915 = kdev_minor_to_i915(dev);pr_devel_ratelimited(DEPRECATED"%s (pid %d) is accessing deprecated %s ""sysfs control, please use gt/gt<n>/%s instead\n",current->comm, task_pid_nr(current), name, name);return to_gt(i915);- }
- return kobj_to_gt(kobj);
It took some time for me to understand what is going on here. We have dev argument which sometimes can point to "struct device", sometimes to "struct kobj_gt", but it's type suggests differently, quite ugly. I wonder if wouldn't be better to use __ATTR instead of DEVICE_ATTR* as in case of intel_engines_add_sysfs. This way abstractions would look better, hopefully.
How would it help?
The difference is that I'm adding twice different interfaces with the same name and different location (i.e. different object). The legacy intrefaces inherit the object from drm and I'm preserving that reference.
While the new objects would derive from the previous and they are pretty much like intel_engines_add_sysfs().
I was not clear on the issue. Here in case of 'id' attribute it is defined as device_attribute, but in kobj_type.sysfs_ops you assign formally incompatible &kobj_sysfs_ops. kobj_sysfs_ops expects kobj_attribute! Fortunately kobj_attribute is 'binary compatible' with device_attribute and kobj is at beginning of struct device as well, so it does not blow up, but I wouldn't say it is clean solution :) If you look at intel_engines_add_sysfs you can see that all attributes are defined as kobj_attribute.
Regards Andrzej
[...]
+struct kobject * +intel_gt_create_kobj(struct intel_gt *gt, struct kobject *dir, const char *name) +{
- struct kobj_gt *kg;
- kg = kzalloc(sizeof(*kg), GFP_KERNEL);
- if (!kg)
return NULL;- kobject_init(&kg->base, &kobj_gt_type);
- kg->gt = gt;
- /* xfer ownership to sysfs tree */
- if (kobject_add(&kg->base, dir, "%s", name)) {
kobject_put(&kg->base);return NULL;- }
- return &kg->base; /* borrowed ref */
+}
+void intel_gt_sysfs_register(struct intel_gt *gt) +{
- struct kobject *dir;
- char name[80];
- snprintf(name, sizeof(name), "gt%d", gt->info.id);
- dir = intel_gt_create_kobj(gt, gt->i915->sysfs_gt, name);
- if (!dir) {
drm_warn(>->i915->drm,"failed to initialize %s sysfs root\n", name);return;- }
+}
Squashing intel_gt_create_kobj into intel_gt_sysfs_register would simplify code and allows drop snprintf to local array.
right!
+static struct kobject *i915_setup_gt_sysfs(struct kobject *parent) +{
- return kobject_create_and_add("gt", parent);
+}
- void i915_setup_sysfs(struct drm_i915_private *dev_priv) { struct device *kdev = dev_priv->drm.primary->kdev;
@@ -538,6 +543,11 @@ void i915_setup_sysfs(struct drm_i915_private *dev_priv) if (ret) drm_err(&dev_priv->drm, "RPS sysfs setup failed\n");
- dev_priv->sysfs_gt = i915_setup_gt_sysfs(&kdev->kobj);
Why not directly kobject_create_and_add("gt", parent) ? up to you.
of course!
[...]
Thanks a lot for the review, Andi
Hi Andrzej,
I'm sorry, but I'm not fully understanding,
+struct intel_gt *intel_gt_sysfs_get_drvdata(struct device *dev,
const char *name)+{
- struct kobject *kobj = &dev->kobj;
- /*
* We are interested at knowing from where the interface* has been called, whether it's called from gt/ or from* the parent directory.* From the interface position it depends also the value of* the private data.* If the interface is called from gt/ then private data is* of the "struct intel_gt *" type, otherwise it's * a* "struct drm_i915_private *" type.*/- if (!is_object_gt(kobj)) {
struct drm_i915_private *i915 = kdev_minor_to_i915(dev);pr_devel_ratelimited(DEPRECATED"%s (pid %d) is accessing deprecated %s ""sysfs control, please use gt/gt<n>/%s instead\n",current->comm, task_pid_nr(current), name, name);return to_gt(i915);- }
- return kobj_to_gt(kobj);
It took some time for me to understand what is going on here. We have dev argument which sometimes can point to "struct device", sometimes to "struct kobj_gt", but it's type suggests differently, quite ugly. I wonder if wouldn't be better to use __ATTR instead of DEVICE_ATTR* as in case of intel_engines_add_sysfs. This way abstractions would look better, hopefully.
How would it help?
The difference is that I'm adding twice different interfaces with the same name and different location (i.e. different object). The legacy intrefaces inherit the object from drm and I'm preserving that reference.
While the new objects would derive from the previous and they are pretty much like intel_engines_add_sysfs().
I was not clear on the issue. Here in case of 'id' attribute it is defined as device_attribute, but in kobj_type.sysfs_ops you assign formally incompatible &kobj_sysfs_ops.
'kobj_sysfs_ops' is of the type 'kobj_type'.
kobj_sysfs_ops expects kobj_attribute! Fortunately kobj_attribute is 'binary compatible' with device_attribute and kobj is at beginning of struct device as well, so it does not blow up, but I wouldn't say it is clean solution :) If you look at intel_engines_add_sysfs you can see that all attributes are defined as kobj_attribute.
That's exactly the approach I use in the next patches for the power management files, I use "struct kobj_gt" wrapped around "struct kobject". But I'm using that only for the GT files.
Are you, btw, suggesting to use this same approache also for the legacy files that for now have a pointer to the drm kobject? This way I would need to add more information, like the pointer to i915 and gt_id. This way I wouldn't need the files above that look hacky to you. Is this what you mean?
Andi
+struct intel_gt *intel_gt_sysfs_get_drvdata(struct device *dev,
const char *name)+{
- struct kobject *kobj = &dev->kobj;
- /*
* We are interested at knowing from where the interface* has been called, whether it's called from gt/ or from* the parent directory.* From the interface position it depends also the value of* the private data.* If the interface is called from gt/ then private data is* of the "struct intel_gt *" type, otherwise it's * a* "struct drm_i915_private *" type.*/- if (!is_object_gt(kobj)) {
struct drm_i915_private *i915 = kdev_minor_to_i915(dev);pr_devel_ratelimited(DEPRECATED"%s (pid %d) is accessing deprecated %s ""sysfs control, please use gt/gt<n>/%s instead\n",current->comm, task_pid_nr(current), name, name);return to_gt(i915);- }
- return kobj_to_gt(kobj);
It took some time for me to understand what is going on here. We have dev argument which sometimes can point to "struct device", sometimes to "struct kobj_gt", but it's type suggests differently, quite ugly. I wonder if wouldn't be better to use __ATTR instead of DEVICE_ATTR* as in case of intel_engines_add_sysfs. This way abstractions would look better, hopefully.
How would it help?
The difference is that I'm adding twice different interfaces with the same name and different location (i.e. different object). The legacy intrefaces inherit the object from drm and I'm preserving that reference.
While the new objects would derive from the previous and they are pretty much like intel_engines_add_sysfs().
I was not clear on the issue. Here in case of 'id' attribute it is defined as device_attribute, but in kobj_type.sysfs_ops you assign formally incompatible &kobj_sysfs_ops.
'kobj_sysfs_ops' is of the type 'kobj_type'.
kobj_sysfs_ops expects kobj_attribute! Fortunately kobj_attribute is 'binary compatible' with device_attribute and kobj is at beginning of struct device as well, so it does not blow up, but I wouldn't say it is clean solution :) If you look at intel_engines_add_sysfs you can see that all attributes are defined as kobj_attribute.
That's exactly the approach I use in the next patches for the power management files, I use "struct kobj_gt" wrapped around "struct kobject". But I'm using that only for the GT files.
Are you, btw, suggesting to use this same approache also for the legacy files that for now have a pointer to the drm kobject? This way I would need to add more information, like the pointer to i915 and gt_id. This way I wouldn't need the files above that look hacky to you. Is this what you mean?
Still this wouldn't solve it because I am merging the legacy interfaces to an existing kobject and creating new kobjects for the new interfaces that go under gt. I would need some other ugly hack to have things coming around.
Unless I misunderstood you.
Andi
On 13.03.2022 20:45, Andi Shyti wrote:
Hi Andrzej,
I'm sorry, but I'm not fully understanding,
+struct intel_gt *intel_gt_sysfs_get_drvdata(struct device *dev,
const char *name)+{
- struct kobject *kobj = &dev->kobj;
- /*
* We are interested at knowing from where the interface* has been called, whether it's called from gt/ or from* the parent directory.* From the interface position it depends also the value of* the private data.* If the interface is called from gt/ then private data is* of the "struct intel_gt *" type, otherwise it's * a* "struct drm_i915_private *" type.*/- if (!is_object_gt(kobj)) {
struct drm_i915_private *i915 = kdev_minor_to_i915(dev);pr_devel_ratelimited(DEPRECATED"%s (pid %d) is accessing deprecated %s ""sysfs control, please use gt/gt<n>/%s instead\n",current->comm, task_pid_nr(current), name, name);return to_gt(i915);- }
- return kobj_to_gt(kobj);
It took some time for me to understand what is going on here. We have dev argument which sometimes can point to "struct device", sometimes to "struct kobj_gt", but it's type suggests differently, quite ugly. I wonder if wouldn't be better to use __ATTR instead of DEVICE_ATTR* as in case of intel_engines_add_sysfs. This way abstractions would look better, hopefully.
How would it help?
The difference is that I'm adding twice different interfaces with the same name and different location (i.e. different object). The legacy intrefaces inherit the object from drm and I'm preserving that reference.
While the new objects would derive from the previous and they are pretty much like intel_engines_add_sysfs().
I was not clear on the issue. Here in case of 'id' attribute it is defined as device_attribute, but in kobj_type.sysfs_ops you assign formally incompatible &kobj_sysfs_ops.
'kobj_sysfs_ops' is of the type 'kobj_type'.
Yes, but for example kobj_sysfs_ops.show points to function kobj_attr_show, and kobj_attr_show expects that it's attr argument is embedded in kobj_attribute[1], but this is not true in case of 'id' attribute - it is embedded in device_attribute. In short kobj_sysfs_ops should be used only with attrs embeded in kobj_attribute, unless I missed sth.
[1]: https://elixir.bootlin.com/linux/latest/source/lib/kobject.c#L836
kobj_sysfs_ops expects kobj_attribute! Fortunately kobj_attribute is 'binary compatible' with device_attribute and kobj is at beginning of struct device as well, so it does not blow up, but I wouldn't say it is clean solution :) If you look at intel_engines_add_sysfs you can see that all attributes are defined as kobj_attribute.
That's exactly the approach I use in the next patches for the power management files, I use "struct kobj_gt" wrapped around "struct kobject". But I'm using that only for the GT files.
But attributes are still defined using DEVICE_ATTR* macros, ie they are embedded in device_attribute, so the problem is the same - you are using kobj_sysfs_ops with device_attribute.
Are you, btw, suggesting to use this same approache also for the legacy files that for now have a pointer to the drm kobject? This way I would need to add more information, like the pointer to i915 and gt_id. This way I wouldn't need the files above that look hacky to you. Is this what you mean?
Positive feedback is more difficult :) I am little bit lost in possible solutions, after grepping other drivers I have not good advice about proper handling of such situation, *beside splitting the interface*. For sure attrs used in device/power must be embedded in device_attribute. So if you do not want to split interface, then it implies GTs attrs must be also in device_attribute. Then maybe creating custom sysfs_ops would help??? I am not sure.
Regards Andrzej
Andi
Now tiles have their own sysfs interfaces under the gt/ directory. Because RC6 is a property that can be configured on a tile basis, then each tile should have its own interface
The new sysfs structure will have a similar layout for the 4 tile case:
/sys/.../card0 ├── gt │ ├── gt0 │ │ ├── id │ │ ├── rc6_enable │ │ ├── rc6_residency_ms . . . . . . . . │ └── gtN │ ├── id │ ├── rc6_enable │ ├── rc6_residency_ms │ . │ . │ └── power/ -+ ├── rc6_enable | Original interface ├── rc6_residency_ms +-> kept as existing ABI; . | it multiplexes over . | the GTs -+
The existing interfaces have been kept in their original location to preserve the existing ABI. They act on all the GTs: when reading they provide the average value from all the GTs.
This patch is not really adding exposing new interfaces (new ABI) other than adapting the existing one to more tiles. In any case this new set of interfaces will be a basic tool for system managers and administrators when using i915.
Signed-off-by: Andi Shyti andi.shyti@linux.intel.com Signed-off-by: Lucas De Marchi lucas.demarchi@intel.com Cc: Chris Wilson chris@chris-wilson.co.uk Cc: Joonas Lahtinen joonas.lahtinen@linux.intel.com Cc: Matt Roper matthew.d.roper@intel.com Cc: Sujaritha Sundaresan sujaritha.sundaresan@intel.com Cc: Tvrtko Ursulin tvrtko.ursulin@intel.com --- drivers/gpu/drm/i915/Makefile | 1 + drivers/gpu/drm/i915/gt/intel_gt_sysfs.c | 19 ++ drivers/gpu/drm/i915/gt/intel_gt_sysfs_pm.c | 220 ++++++++++++++++++++ drivers/gpu/drm/i915/gt/intel_gt_sysfs_pm.h | 15 ++ drivers/gpu/drm/i915/i915_sysfs.c | 128 ------------ 5 files changed, 255 insertions(+), 128 deletions(-) create mode 100644 drivers/gpu/drm/i915/gt/intel_gt_sysfs_pm.c create mode 100644 drivers/gpu/drm/i915/gt/intel_gt_sysfs_pm.h
diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile index 277064b00afd..7104558b81d5 100644 --- a/drivers/gpu/drm/i915/Makefile +++ b/drivers/gpu/drm/i915/Makefile @@ -106,6 +106,7 @@ gt-y += \ gt/intel_gt_pm_irq.o \ gt/intel_gt_requests.o \ gt/intel_gt_sysfs.o \ + gt/intel_gt_sysfs_pm.o \ gt/intel_gtt.o \ gt/intel_llc.o \ gt/intel_lrc.o \ diff --git a/drivers/gpu/drm/i915/gt/intel_gt_sysfs.c b/drivers/gpu/drm/i915/gt/intel_gt_sysfs.c index 0206e9aa4867..132b90247a41 100644 --- a/drivers/gpu/drm/i915/gt/intel_gt_sysfs.c +++ b/drivers/gpu/drm/i915/gt/intel_gt_sysfs.c @@ -13,6 +13,7 @@ #include "i915_sysfs.h" #include "intel_gt.h" #include "intel_gt_sysfs.h" +#include "intel_gt_sysfs_pm.h" #include "intel_gt_types.h" #include "intel_rc6.h"
@@ -54,6 +55,11 @@ struct intel_gt *intel_gt_sysfs_get_drvdata(struct device *dev, return kobj_to_gt(kobj); }
+static struct kobject *gt_get_parent_obj(struct intel_gt *gt) +{ + return >->i915->drm.primary->kdev->kobj; +} + static ssize_t id_show(struct device *dev, struct device_attribute *attr, char *buf) @@ -107,6 +113,17 @@ void intel_gt_sysfs_register(struct intel_gt *gt) struct kobject *dir; char name[80];
+ /* + * We need to make things right with the + * ABI compatibility. The files were originally + * generated under the parent directory. + * + * We generate the files only for gt 0 + * to avoid duplicates. + */ + if (gt_is_root(gt)) + intel_gt_sysfs_pm_init(gt, gt_get_parent_obj(gt)); + snprintf(name, sizeof(name), "gt%d", gt->info.id);
dir = intel_gt_create_kobj(gt, gt->i915->sysfs_gt, name); @@ -115,4 +132,6 @@ void intel_gt_sysfs_register(struct intel_gt *gt) "failed to initialize %s sysfs root\n", name); return; } + + intel_gt_sysfs_pm_init(gt, dir); } diff --git a/drivers/gpu/drm/i915/gt/intel_gt_sysfs_pm.c b/drivers/gpu/drm/i915/gt/intel_gt_sysfs_pm.c new file mode 100644 index 000000000000..27d93a36894a --- /dev/null +++ b/drivers/gpu/drm/i915/gt/intel_gt_sysfs_pm.c @@ -0,0 +1,220 @@ +// SPDX-License-Identifier: MIT +/* + * Copyright © 2022 Intel Corporation + */ + +#include <drm/drm_device.h> +#include <linux/sysfs.h> +#include <linux/printk.h> + +#include "i915_drv.h" +#include "i915_sysfs.h" +#include "intel_gt.h" +#include "intel_gt_regs.h" +#include "intel_gt_sysfs.h" +#include "intel_gt_sysfs_pm.h" +#include "intel_rc6.h" + +#ifdef CONFIG_PM +static s64 +sysfs_gt_attribute_r_func(struct device *dev, struct device_attribute *attr, + s64 (func)(struct intel_gt *gt)) +{ + struct intel_gt *gt; + s64 ret = 0; + + if (!is_object_gt(&dev->kobj)) { + int i, num_gt = 0; + struct drm_i915_private *i915 = kdev_minor_to_i915(dev); + + for_each_gt(gt, i915, i) { + ret += func(gt); + num_gt++; + } + + ret /= num_gt; + } else { + gt = intel_gt_sysfs_get_drvdata(dev, attr->attr.name); + ret = func(gt); + } + + return ret; +} + +static u32 get_residency(struct intel_gt *gt, i915_reg_t reg) +{ + intel_wakeref_t wakeref; + u64 res = 0; + + with_intel_runtime_pm(gt->uncore->rpm, wakeref) + res = intel_rc6_residency_us(>->rc6, reg); + + return DIV_ROUND_CLOSEST_ULL(res, 1000); +} + +static ssize_t rc6_enable_show(struct device *dev, + struct device_attribute *attr, + char *buff) +{ + struct intel_gt *gt = intel_gt_sysfs_get_drvdata(dev, attr->attr.name); + u8 mask = 0; + + if (HAS_RC6(gt->i915)) + mask |= BIT(0); + if (HAS_RC6p(gt->i915)) + mask |= BIT(1); + if (HAS_RC6pp(gt->i915)) + mask |= BIT(2); + + return sysfs_emit(buff, "%x\n", mask); +} + +static s64 __rc6_residency_ms_show(struct intel_gt *gt) +{ + return get_residency(gt, GEN6_GT_GFX_RC6); +} + +static ssize_t rc6_residency_ms_show(struct device *dev, + struct device_attribute *attr, + char *buff) +{ + s64 rc6_residency = sysfs_gt_attribute_r_func(dev, attr, + __rc6_residency_ms_show); + + return sysfs_emit(buff, "%u\n", (u32) rc6_residency); +} + +static s64 __rc6p_residency_ms_show(struct intel_gt *gt) +{ + return get_residency(gt, GEN6_GT_GFX_RC6p); +} + +static ssize_t rc6p_residency_ms_show(struct device *dev, + struct device_attribute *attr, + char *buff) +{ + s64 rc6p_residency = sysfs_gt_attribute_r_func(dev, attr, + __rc6p_residency_ms_show); + + return sysfs_emit(buff, "%u\n", (u32) rc6p_residency); +} + +static s64 __rc6pp_residency_ms_show(struct intel_gt *gt) +{ + return get_residency(gt, GEN6_GT_GFX_RC6pp); +} + +static ssize_t rc6pp_residency_ms_show(struct device *dev, + struct device_attribute *attr, + char *buff) +{ + s64 rc6pp_residency = sysfs_gt_attribute_r_func(dev, attr, + __rc6pp_residency_ms_show); + + return sysfs_emit(buff, "%u\n", (u32) rc6pp_residency); +} + +static s64 __media_rc6_residency_ms_show(struct intel_gt *gt) +{ + return get_residency(gt, VLV_GT_MEDIA_RC6); +} + +static ssize_t media_rc6_residency_ms_show(struct device *dev, + struct device_attribute *attr, + char *buff) +{ + s64 rc6_residency = sysfs_gt_attribute_r_func(dev, attr, + __media_rc6_residency_ms_show); + + return sysfs_emit(buff, "%u\n", (u32) rc6_residency); +} + +static DEVICE_ATTR_RO(rc6_enable); +static DEVICE_ATTR_RO(rc6_residency_ms); +static DEVICE_ATTR_RO(rc6p_residency_ms); +static DEVICE_ATTR_RO(rc6pp_residency_ms); +static DEVICE_ATTR_RO(media_rc6_residency_ms); + +static struct attribute *rc6_attrs[] = { + &dev_attr_rc6_enable.attr, + &dev_attr_rc6_residency_ms.attr, + NULL +}; + +static struct attribute *rc6p_attrs[] = { + &dev_attr_rc6p_residency_ms.attr, + &dev_attr_rc6pp_residency_ms.attr, + NULL +}; + +static struct attribute *media_rc6_attrs[] = { + &dev_attr_media_rc6_residency_ms.attr, + NULL +}; + +static const struct attribute_group rc6_attr_group[] = { + { .attrs = rc6_attrs, }, + { .name = power_group_name, .attrs = rc6_attrs, }, +}; + +static const struct attribute_group rc6p_attr_group[] = { + { .attrs = rc6p_attrs, }, + { .name = power_group_name, .attrs = rc6p_attrs, }, +}; + +static const struct attribute_group media_rc6_attr_group[] = { + { .attrs = media_rc6_attrs, }, + { .name = power_group_name, .attrs = media_rc6_attrs, }, +}; + +static int __intel_gt_sysfs_create_group(struct kobject *kobj, + const struct attribute_group *grp) +{ + return is_object_gt(kobj) ? + sysfs_create_group(kobj, &grp[0]) : + sysfs_merge_group(kobj, &grp[1]); +} + +static void intel_sysfs_rc6_init(struct intel_gt *gt, struct kobject *kobj) +{ + int ret; + + if (!HAS_RC6(gt->i915)) + return; + + ret = __intel_gt_sysfs_create_group(kobj, rc6_attr_group); + if (ret) + drm_warn(>->i915->drm, + "failed to create gt%u RC6 sysfs files\n", + gt->info.id); + + /* + * cannot use the is_visible() attribute because + * the upper object inherits from the parent group. + */ + if (HAS_RC6p(gt->i915)) { + ret = __intel_gt_sysfs_create_group(kobj, rc6p_attr_group); + if (ret) + drm_warn(>->i915->drm, + "failed to create gt%u RC6p sysfs files\n", + gt->info.id); + } + + if (IS_VALLEYVIEW(gt->i915) || IS_CHERRYVIEW(gt->i915)) { + ret = __intel_gt_sysfs_create_group(kobj, media_rc6_attr_group); + if (ret) + drm_warn(>->i915->drm, + "failed to create media %u RC6 sysfs files\n", + gt->info.id); + } +} +#else +static void intel_sysfs_rc6_init(struct intel_gt *gt, struct kobject *kobj) +{ +} +#endif /* CONFIG_PM */ + +void intel_gt_sysfs_pm_init(struct intel_gt *gt, struct kobject *kobj) +{ + intel_sysfs_rc6_init(gt, kobj); +} diff --git a/drivers/gpu/drm/i915/gt/intel_gt_sysfs_pm.h b/drivers/gpu/drm/i915/gt/intel_gt_sysfs_pm.h new file mode 100644 index 000000000000..f567105a4a89 --- /dev/null +++ b/drivers/gpu/drm/i915/gt/intel_gt_sysfs_pm.h @@ -0,0 +1,15 @@ +/* SPDX-License-Identifier: MIT */ +/* + * Copyright © 2022 Intel Corporation + */ + +#ifndef __SYSFS_GT_PM_H__ +#define __SYSFS_GT_PM_H__ + +#include <linux/kobject.h> + +#include "intel_gt_types.h" + +void intel_gt_sysfs_pm_init(struct intel_gt *gt, struct kobject *kobj); + +#endif /* SYSFS_RC6_H */ diff --git a/drivers/gpu/drm/i915/i915_sysfs.c b/drivers/gpu/drm/i915/i915_sysfs.c index 3643efde52ca..b08a959e5ccc 100644 --- a/drivers/gpu/drm/i915/i915_sysfs.c +++ b/drivers/gpu/drm/i915/i915_sysfs.c @@ -45,107 +45,6 @@ struct drm_i915_private *kdev_minor_to_i915(struct device *kdev) return to_i915(minor->dev); }
-#ifdef CONFIG_PM -static u32 calc_residency(struct drm_i915_private *dev_priv, - i915_reg_t reg) -{ - intel_wakeref_t wakeref; - u64 res = 0; - - with_intel_runtime_pm(&dev_priv->runtime_pm, wakeref) - res = intel_rc6_residency_us(&to_gt(dev_priv)->rc6, reg); - - return DIV_ROUND_CLOSEST_ULL(res, 1000); -} - -static ssize_t rc6_enable_show(struct device *kdev, - struct device_attribute *attr, char *buf) -{ - struct drm_i915_private *dev_priv = kdev_minor_to_i915(kdev); - unsigned int mask; - - mask = 0; - if (HAS_RC6(dev_priv)) - mask |= BIT(0); - if (HAS_RC6p(dev_priv)) - mask |= BIT(1); - if (HAS_RC6pp(dev_priv)) - mask |= BIT(2); - - return sysfs_emit(buf, "%x\n", mask); -} - -static ssize_t rc6_residency_ms_show(struct device *kdev, - struct device_attribute *attr, char *buf) -{ - struct drm_i915_private *dev_priv = kdev_minor_to_i915(kdev); - u32 rc6_residency = calc_residency(dev_priv, GEN6_GT_GFX_RC6); - return sysfs_emit(buf, "%u\n", rc6_residency); -} - -static ssize_t rc6p_residency_ms_show(struct device *kdev, - struct device_attribute *attr, char *buf) -{ - struct drm_i915_private *dev_priv = kdev_minor_to_i915(kdev); - u32 rc6p_residency = calc_residency(dev_priv, GEN6_GT_GFX_RC6p); - return sysfs_emit(buf, "%u\n", rc6p_residency); -} - -static ssize_t rc6pp_residency_ms_show(struct device *kdev, - struct device_attribute *attr, char *buf) -{ - struct drm_i915_private *dev_priv = kdev_minor_to_i915(kdev); - u32 rc6pp_residency = calc_residency(dev_priv, GEN6_GT_GFX_RC6pp); - return sysfs_emit(buf, "%u\n", rc6pp_residency); -} - -static ssize_t media_rc6_residency_ms_show(struct device *kdev, - struct device_attribute *attr, char *buf) -{ - struct drm_i915_private *dev_priv = kdev_minor_to_i915(kdev); - u32 rc6_residency = calc_residency(dev_priv, VLV_GT_MEDIA_RC6); - return sysfs_emit(buf, "%u\n", rc6_residency); -} - -static DEVICE_ATTR_RO(rc6_enable); -static DEVICE_ATTR_RO(rc6_residency_ms); -static DEVICE_ATTR_RO(rc6p_residency_ms); -static DEVICE_ATTR_RO(rc6pp_residency_ms); -static DEVICE_ATTR_RO(media_rc6_residency_ms); - -static struct attribute *rc6_attrs[] = { - &dev_attr_rc6_enable.attr, - &dev_attr_rc6_residency_ms.attr, - NULL -}; - -static const struct attribute_group rc6_attr_group = { - .name = power_group_name, - .attrs = rc6_attrs -}; - -static struct attribute *rc6p_attrs[] = { - &dev_attr_rc6p_residency_ms.attr, - &dev_attr_rc6pp_residency_ms.attr, - NULL -}; - -static const struct attribute_group rc6p_attr_group = { - .name = power_group_name, - .attrs = rc6p_attrs -}; - -static struct attribute *media_rc6_attrs[] = { - &dev_attr_media_rc6_residency_ms.attr, - NULL -}; - -static const struct attribute_group media_rc6_attr_group = { - .name = power_group_name, - .attrs = media_rc6_attrs -}; -#endif - static int l3_access_valid(struct drm_i915_private *i915, loff_t offset) { if (!HAS_L3_DPF(i915)) @@ -497,29 +396,6 @@ void i915_setup_sysfs(struct drm_i915_private *dev_priv) struct device *kdev = dev_priv->drm.primary->kdev; int ret;
-#ifdef CONFIG_PM - if (HAS_RC6(dev_priv)) { - ret = sysfs_merge_group(&kdev->kobj, - &rc6_attr_group); - if (ret) - drm_err(&dev_priv->drm, - "RC6 residency sysfs setup failed\n"); - } - if (HAS_RC6p(dev_priv)) { - ret = sysfs_merge_group(&kdev->kobj, - &rc6p_attr_group); - if (ret) - drm_err(&dev_priv->drm, - "RC6p residency sysfs setup failed\n"); - } - if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv)) { - ret = sysfs_merge_group(&kdev->kobj, - &media_rc6_attr_group); - if (ret) - drm_err(&dev_priv->drm, - "Media RC6 residency sysfs setup failed\n"); - } -#endif if (HAS_L3_DPF(dev_priv)) { ret = device_create_bin_file(kdev, &dpf_attrs); if (ret) @@ -565,8 +441,4 @@ void i915_teardown_sysfs(struct drm_i915_private *dev_priv) sysfs_remove_files(&kdev->kobj, gen6_attrs); device_remove_bin_file(kdev, &dpf_attrs_1); device_remove_bin_file(kdev, &dpf_attrs); -#ifdef CONFIG_PM - sysfs_unmerge_group(&kdev->kobj, &rc6_attr_group); - sysfs_unmerge_group(&kdev->kobj, &rc6p_attr_group); -#endif }
On 17/02/2022 14:41, Andi Shyti wrote:
Now tiles have their own sysfs interfaces under the gt/ directory. Because RC6 is a property that can be configured on a tile basis, then each tile should have its own interface
The new sysfs structure will have a similar layout for the 4 tile case:
/sys/.../card0 ├── gt │ ├── gt0 │ │ ├── id │ │ ├── rc6_enable │ │ ├── rc6_residency_ms . . . . . . . . │ └── gtN │ ├── id │ ├── rc6_enable │ ├── rc6_residency_ms │ . │ . │ └── power/ -+ ├── rc6_enable | Original interface ├── rc6_residency_ms +-> kept as existing ABI; . | it multiplexes over . | the GTs -+
The existing interfaces have been kept in their original location to preserve the existing ABI. They act on all the GTs: when reading they provide the average value from all the GTs.
Average feels very odd to me. I'd ask if we can get away providing an errno instead? Or tile zero data?
Case in point, and please correct me if I am wrong, legacy rc6_enable returns tile zero, while residency returns average.
Even the deprecated message gets logged with every access right?
Btw is the deperecated message limited to multi-tile platforms (can't see that it is) and what is the plan for that?
It's a tough problem, no easy answers even after all this time. :D
Regards,
Tvrtko
This patch is not really adding exposing new interfaces (new ABI) other than adapting the existing one to more tiles. In any case this new set of interfaces will be a basic tool for system managers and administrators when using i915.
Signed-off-by: Andi Shyti andi.shyti@linux.intel.com Signed-off-by: Lucas De Marchi lucas.demarchi@intel.com Cc: Chris Wilson chris@chris-wilson.co.uk Cc: Joonas Lahtinen joonas.lahtinen@linux.intel.com Cc: Matt Roper matthew.d.roper@intel.com Cc: Sujaritha Sundaresan sujaritha.sundaresan@intel.com Cc: Tvrtko Ursulin tvrtko.ursulin@intel.com
drivers/gpu/drm/i915/Makefile | 1 + drivers/gpu/drm/i915/gt/intel_gt_sysfs.c | 19 ++ drivers/gpu/drm/i915/gt/intel_gt_sysfs_pm.c | 220 ++++++++++++++++++++ drivers/gpu/drm/i915/gt/intel_gt_sysfs_pm.h | 15 ++ drivers/gpu/drm/i915/i915_sysfs.c | 128 ------------ 5 files changed, 255 insertions(+), 128 deletions(-) create mode 100644 drivers/gpu/drm/i915/gt/intel_gt_sysfs_pm.c create mode 100644 drivers/gpu/drm/i915/gt/intel_gt_sysfs_pm.h
diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile index 277064b00afd..7104558b81d5 100644 --- a/drivers/gpu/drm/i915/Makefile +++ b/drivers/gpu/drm/i915/Makefile @@ -106,6 +106,7 @@ gt-y += \ gt/intel_gt_pm_irq.o \ gt/intel_gt_requests.o \ gt/intel_gt_sysfs.o \
- gt/intel_gt_sysfs_pm.o \ gt/intel_gtt.o \ gt/intel_llc.o \ gt/intel_lrc.o \
diff --git a/drivers/gpu/drm/i915/gt/intel_gt_sysfs.c b/drivers/gpu/drm/i915/gt/intel_gt_sysfs.c index 0206e9aa4867..132b90247a41 100644 --- a/drivers/gpu/drm/i915/gt/intel_gt_sysfs.c +++ b/drivers/gpu/drm/i915/gt/intel_gt_sysfs.c @@ -13,6 +13,7 @@ #include "i915_sysfs.h" #include "intel_gt.h" #include "intel_gt_sysfs.h" +#include "intel_gt_sysfs_pm.h" #include "intel_gt_types.h" #include "intel_rc6.h"
@@ -54,6 +55,11 @@ struct intel_gt *intel_gt_sysfs_get_drvdata(struct device *dev, return kobj_to_gt(kobj); }
+static struct kobject *gt_get_parent_obj(struct intel_gt *gt) +{
- return >->i915->drm.primary->kdev->kobj;
+}
- static ssize_t id_show(struct device *dev, struct device_attribute *attr, char *buf)
@@ -107,6 +113,17 @@ void intel_gt_sysfs_register(struct intel_gt *gt) struct kobject *dir; char name[80];
/*
* We need to make things right with the* ABI compatibility. The files were originally* generated under the parent directory.** We generate the files only for gt 0* to avoid duplicates.*/if (gt_is_root(gt))
intel_gt_sysfs_pm_init(gt, gt_get_parent_obj(gt));snprintf(name, sizeof(name), "gt%d", gt->info.id);
dir = intel_gt_create_kobj(gt, gt->i915->sysfs_gt, name);
@@ -115,4 +132,6 @@ void intel_gt_sysfs_register(struct intel_gt *gt) "failed to initialize %s sysfs root\n", name); return; }
- intel_gt_sysfs_pm_init(gt, dir); }
diff --git a/drivers/gpu/drm/i915/gt/intel_gt_sysfs_pm.c b/drivers/gpu/drm/i915/gt/intel_gt_sysfs_pm.c new file mode 100644 index 000000000000..27d93a36894a --- /dev/null +++ b/drivers/gpu/drm/i915/gt/intel_gt_sysfs_pm.c @@ -0,0 +1,220 @@ +// SPDX-License-Identifier: MIT +/*
- Copyright © 2022 Intel Corporation
- */
+#include <drm/drm_device.h> +#include <linux/sysfs.h> +#include <linux/printk.h>
+#include "i915_drv.h" +#include "i915_sysfs.h" +#include "intel_gt.h" +#include "intel_gt_regs.h" +#include "intel_gt_sysfs.h" +#include "intel_gt_sysfs_pm.h" +#include "intel_rc6.h"
+#ifdef CONFIG_PM +static s64 +sysfs_gt_attribute_r_func(struct device *dev, struct device_attribute *attr,
s64 (func)(struct intel_gt *gt))+{
- struct intel_gt *gt;
- s64 ret = 0;
- if (!is_object_gt(&dev->kobj)) {
int i, num_gt = 0;struct drm_i915_private *i915 = kdev_minor_to_i915(dev);for_each_gt(gt, i915, i) {ret += func(gt);num_gt++;}ret /= num_gt;- } else {
gt = intel_gt_sysfs_get_drvdata(dev, attr->attr.name);ret = func(gt);- }
- return ret;
+}
+static u32 get_residency(struct intel_gt *gt, i915_reg_t reg) +{
- intel_wakeref_t wakeref;
- u64 res = 0;
- with_intel_runtime_pm(gt->uncore->rpm, wakeref)
res = intel_rc6_residency_us(>->rc6, reg);- return DIV_ROUND_CLOSEST_ULL(res, 1000);
+}
+static ssize_t rc6_enable_show(struct device *dev,
struct device_attribute *attr,char *buff)+{
- struct intel_gt *gt = intel_gt_sysfs_get_drvdata(dev, attr->attr.name);
- u8 mask = 0;
- if (HAS_RC6(gt->i915))
mask |= BIT(0);- if (HAS_RC6p(gt->i915))
mask |= BIT(1);- if (HAS_RC6pp(gt->i915))
mask |= BIT(2);- return sysfs_emit(buff, "%x\n", mask);
+}
+static s64 __rc6_residency_ms_show(struct intel_gt *gt) +{
- return get_residency(gt, GEN6_GT_GFX_RC6);
+}
+static ssize_t rc6_residency_ms_show(struct device *dev,
struct device_attribute *attr,char *buff)+{
- s64 rc6_residency = sysfs_gt_attribute_r_func(dev, attr,
__rc6_residency_ms_show);- return sysfs_emit(buff, "%u\n", (u32) rc6_residency);
+}
+static s64 __rc6p_residency_ms_show(struct intel_gt *gt) +{
- return get_residency(gt, GEN6_GT_GFX_RC6p);
+}
+static ssize_t rc6p_residency_ms_show(struct device *dev,
struct device_attribute *attr,char *buff)+{
- s64 rc6p_residency = sysfs_gt_attribute_r_func(dev, attr,
__rc6p_residency_ms_show);- return sysfs_emit(buff, "%u\n", (u32) rc6p_residency);
+}
+static s64 __rc6pp_residency_ms_show(struct intel_gt *gt) +{
- return get_residency(gt, GEN6_GT_GFX_RC6pp);
+}
+static ssize_t rc6pp_residency_ms_show(struct device *dev,
struct device_attribute *attr,char *buff)+{
- s64 rc6pp_residency = sysfs_gt_attribute_r_func(dev, attr,
__rc6pp_residency_ms_show);- return sysfs_emit(buff, "%u\n", (u32) rc6pp_residency);
+}
+static s64 __media_rc6_residency_ms_show(struct intel_gt *gt) +{
- return get_residency(gt, VLV_GT_MEDIA_RC6);
+}
+static ssize_t media_rc6_residency_ms_show(struct device *dev,
struct device_attribute *attr,char *buff)+{
- s64 rc6_residency = sysfs_gt_attribute_r_func(dev, attr,
__media_rc6_residency_ms_show);- return sysfs_emit(buff, "%u\n", (u32) rc6_residency);
+}
+static DEVICE_ATTR_RO(rc6_enable); +static DEVICE_ATTR_RO(rc6_residency_ms); +static DEVICE_ATTR_RO(rc6p_residency_ms); +static DEVICE_ATTR_RO(rc6pp_residency_ms); +static DEVICE_ATTR_RO(media_rc6_residency_ms);
+static struct attribute *rc6_attrs[] = {
- &dev_attr_rc6_enable.attr,
- &dev_attr_rc6_residency_ms.attr,
- NULL
+};
+static struct attribute *rc6p_attrs[] = {
- &dev_attr_rc6p_residency_ms.attr,
- &dev_attr_rc6pp_residency_ms.attr,
- NULL
+};
+static struct attribute *media_rc6_attrs[] = {
- &dev_attr_media_rc6_residency_ms.attr,
- NULL
+};
+static const struct attribute_group rc6_attr_group[] = {
- { .attrs = rc6_attrs, },
- { .name = power_group_name, .attrs = rc6_attrs, },
+};
+static const struct attribute_group rc6p_attr_group[] = {
- { .attrs = rc6p_attrs, },
- { .name = power_group_name, .attrs = rc6p_attrs, },
+};
+static const struct attribute_group media_rc6_attr_group[] = {
- { .attrs = media_rc6_attrs, },
- { .name = power_group_name, .attrs = media_rc6_attrs, },
+};
+static int __intel_gt_sysfs_create_group(struct kobject *kobj,
const struct attribute_group *grp)+{
- return is_object_gt(kobj) ?
sysfs_create_group(kobj, &grp[0]) :sysfs_merge_group(kobj, &grp[1]);+}
+static void intel_sysfs_rc6_init(struct intel_gt *gt, struct kobject *kobj) +{
- int ret;
- if (!HAS_RC6(gt->i915))
return;- ret = __intel_gt_sysfs_create_group(kobj, rc6_attr_group);
- if (ret)
drm_warn(>->i915->drm,"failed to create gt%u RC6 sysfs files\n",gt->info.id);- /*
* cannot use the is_visible() attribute because* the upper object inherits from the parent group.*/- if (HAS_RC6p(gt->i915)) {
ret = __intel_gt_sysfs_create_group(kobj, rc6p_attr_group);if (ret)drm_warn(>->i915->drm,"failed to create gt%u RC6p sysfs files\n",gt->info.id);- }
- if (IS_VALLEYVIEW(gt->i915) || IS_CHERRYVIEW(gt->i915)) {
ret = __intel_gt_sysfs_create_group(kobj, media_rc6_attr_group);if (ret)drm_warn(>->i915->drm,"failed to create media %u RC6 sysfs files\n",gt->info.id);- }
+} +#else +static void intel_sysfs_rc6_init(struct intel_gt *gt, struct kobject *kobj) +{ +} +#endif /* CONFIG_PM */
+void intel_gt_sysfs_pm_init(struct intel_gt *gt, struct kobject *kobj) +{
- intel_sysfs_rc6_init(gt, kobj);
+} diff --git a/drivers/gpu/drm/i915/gt/intel_gt_sysfs_pm.h b/drivers/gpu/drm/i915/gt/intel_gt_sysfs_pm.h new file mode 100644 index 000000000000..f567105a4a89 --- /dev/null +++ b/drivers/gpu/drm/i915/gt/intel_gt_sysfs_pm.h @@ -0,0 +1,15 @@ +/* SPDX-License-Identifier: MIT */ +/*
- Copyright © 2022 Intel Corporation
- */
+#ifndef __SYSFS_GT_PM_H__ +#define __SYSFS_GT_PM_H__
+#include <linux/kobject.h>
+#include "intel_gt_types.h"
+void intel_gt_sysfs_pm_init(struct intel_gt *gt, struct kobject *kobj);
+#endif /* SYSFS_RC6_H */ diff --git a/drivers/gpu/drm/i915/i915_sysfs.c b/drivers/gpu/drm/i915/i915_sysfs.c index 3643efde52ca..b08a959e5ccc 100644 --- a/drivers/gpu/drm/i915/i915_sysfs.c +++ b/drivers/gpu/drm/i915/i915_sysfs.c @@ -45,107 +45,6 @@ struct drm_i915_private *kdev_minor_to_i915(struct device *kdev) return to_i915(minor->dev); }
-#ifdef CONFIG_PM -static u32 calc_residency(struct drm_i915_private *dev_priv,
i915_reg_t reg)-{
- intel_wakeref_t wakeref;
- u64 res = 0;
- with_intel_runtime_pm(&dev_priv->runtime_pm, wakeref)
res = intel_rc6_residency_us(&to_gt(dev_priv)->rc6, reg);- return DIV_ROUND_CLOSEST_ULL(res, 1000);
-}
-static ssize_t rc6_enable_show(struct device *kdev,
struct device_attribute *attr, char *buf)-{
- struct drm_i915_private *dev_priv = kdev_minor_to_i915(kdev);
- unsigned int mask;
- mask = 0;
- if (HAS_RC6(dev_priv))
mask |= BIT(0);- if (HAS_RC6p(dev_priv))
mask |= BIT(1);- if (HAS_RC6pp(dev_priv))
mask |= BIT(2);- return sysfs_emit(buf, "%x\n", mask);
-}
-static ssize_t rc6_residency_ms_show(struct device *kdev,
struct device_attribute *attr, char *buf)-{
- struct drm_i915_private *dev_priv = kdev_minor_to_i915(kdev);
- u32 rc6_residency = calc_residency(dev_priv, GEN6_GT_GFX_RC6);
- return sysfs_emit(buf, "%u\n", rc6_residency);
-}
-static ssize_t rc6p_residency_ms_show(struct device *kdev,
struct device_attribute *attr, char *buf)-{
- struct drm_i915_private *dev_priv = kdev_minor_to_i915(kdev);
- u32 rc6p_residency = calc_residency(dev_priv, GEN6_GT_GFX_RC6p);
- return sysfs_emit(buf, "%u\n", rc6p_residency);
-}
-static ssize_t rc6pp_residency_ms_show(struct device *kdev,
struct device_attribute *attr, char *buf)-{
- struct drm_i915_private *dev_priv = kdev_minor_to_i915(kdev);
- u32 rc6pp_residency = calc_residency(dev_priv, GEN6_GT_GFX_RC6pp);
- return sysfs_emit(buf, "%u\n", rc6pp_residency);
-}
-static ssize_t media_rc6_residency_ms_show(struct device *kdev,
struct device_attribute *attr, char *buf)-{
- struct drm_i915_private *dev_priv = kdev_minor_to_i915(kdev);
- u32 rc6_residency = calc_residency(dev_priv, VLV_GT_MEDIA_RC6);
- return sysfs_emit(buf, "%u\n", rc6_residency);
-}
-static DEVICE_ATTR_RO(rc6_enable); -static DEVICE_ATTR_RO(rc6_residency_ms); -static DEVICE_ATTR_RO(rc6p_residency_ms); -static DEVICE_ATTR_RO(rc6pp_residency_ms); -static DEVICE_ATTR_RO(media_rc6_residency_ms);
-static struct attribute *rc6_attrs[] = {
- &dev_attr_rc6_enable.attr,
- &dev_attr_rc6_residency_ms.attr,
- NULL
-};
-static const struct attribute_group rc6_attr_group = {
- .name = power_group_name,
- .attrs = rc6_attrs
-};
-static struct attribute *rc6p_attrs[] = {
- &dev_attr_rc6p_residency_ms.attr,
- &dev_attr_rc6pp_residency_ms.attr,
- NULL
-};
-static const struct attribute_group rc6p_attr_group = {
- .name = power_group_name,
- .attrs = rc6p_attrs
-};
-static struct attribute *media_rc6_attrs[] = {
- &dev_attr_media_rc6_residency_ms.attr,
- NULL
-};
-static const struct attribute_group media_rc6_attr_group = {
- .name = power_group_name,
- .attrs = media_rc6_attrs
-}; -#endif
- static int l3_access_valid(struct drm_i915_private *i915, loff_t offset) { if (!HAS_L3_DPF(i915))
@@ -497,29 +396,6 @@ void i915_setup_sysfs(struct drm_i915_private *dev_priv) struct device *kdev = dev_priv->drm.primary->kdev; int ret;
-#ifdef CONFIG_PM
- if (HAS_RC6(dev_priv)) {
ret = sysfs_merge_group(&kdev->kobj,&rc6_attr_group);if (ret)drm_err(&dev_priv->drm,"RC6 residency sysfs setup failed\n");- }
- if (HAS_RC6p(dev_priv)) {
ret = sysfs_merge_group(&kdev->kobj,&rc6p_attr_group);if (ret)drm_err(&dev_priv->drm,"RC6p residency sysfs setup failed\n");- }
- if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv)) {
ret = sysfs_merge_group(&kdev->kobj,&media_rc6_attr_group);if (ret)drm_err(&dev_priv->drm,"Media RC6 residency sysfs setup failed\n");- }
-#endif if (HAS_L3_DPF(dev_priv)) { ret = device_create_bin_file(kdev, &dpf_attrs); if (ret) @@ -565,8 +441,4 @@ void i915_teardown_sysfs(struct drm_i915_private *dev_priv) sysfs_remove_files(&kdev->kobj, gen6_attrs); device_remove_bin_file(kdev, &dpf_attrs_1); device_remove_bin_file(kdev, &dpf_attrs); -#ifdef CONFIG_PM
- sysfs_unmerge_group(&kdev->kobj, &rc6_attr_group);
- sysfs_unmerge_group(&kdev->kobj, &rc6p_attr_group);
-#endif }
Hi Tvrtko,
Now tiles have their own sysfs interfaces under the gt/ directory. Because RC6 is a property that can be configured on a tile basis, then each tile should have its own interface
The new sysfs structure will have a similar layout for the 4 tile case:
/sys/.../card0 ├── gt │ ├── gt0 │ │ ├── id │ │ ├── rc6_enable │ │ ├── rc6_residency_ms . . . . . . . . │ └── gtN │ ├── id │ ├── rc6_enable │ ├── rc6_residency_ms │ . │ . │ └── power/ -+ ├── rc6_enable | Original interface ├── rc6_residency_ms +-> kept as existing ABI; . | it multiplexes over . | the GTs -+
The existing interfaces have been kept in their original location to preserve the existing ABI. They act on all the GTs: when reading they provide the average value from all the GTs.
Average feels very odd to me. I'd ask if we can get away providing an errno instead? Or tile zero data?
Real multiplexing would be providing something when reading and when writing. The idea of average came while revieweing with Chris the write multiplexing. Indeed it makes sense to provide some common value, but I don't know how useful it can be to the user (still if the user needs any average).
Joonas, Chris... any idea?
Case in point, and please correct me if I am wrong, legacy rc6_enable returns tile zero, while residency returns average.
As the interface is done now, the rc6_enable is just returning whether the gpu (i.e. i915, not gt) supports RC6 or not. I think there is a patch later.
Even the deprecated message gets logged with every access right?
Btw is the deperecated message limited to multi-tile platforms (can't see that it is) and what is the plan for that?
yes, at this point the message would need to be removed and I forgot to do it.
It's a tough problem, no easy answers even after all this time. :D
yeah! quite hard to get it conceptually right!
Thanks Tvrtko, Andi
On 17/02/2022 15:53, Andi Shyti wrote:
Hi Tvrtko,
Now tiles have their own sysfs interfaces under the gt/ directory. Because RC6 is a property that can be configured on a tile basis, then each tile should have its own interface
The new sysfs structure will have a similar layout for the 4 tile case:
/sys/.../card0 ├── gt │ ├── gt0 │ │ ├── id │ │ ├── rc6_enable │ │ ├── rc6_residency_ms . . . . . . . . │ └── gtN │ ├── id │ ├── rc6_enable │ ├── rc6_residency_ms │ . │ . │ └── power/ -+ ├── rc6_enable | Original interface ├── rc6_residency_ms +-> kept as existing ABI; . | it multiplexes over . | the GTs -+
The existing interfaces have been kept in their original location to preserve the existing ABI. They act on all the GTs: when reading they provide the average value from all the GTs.
Average feels very odd to me. I'd ask if we can get away providing an errno instead? Or tile zero data?
Real multiplexing would be providing something when reading and when writing. The idea of average came while revieweing with Chris the write multiplexing. Indeed it makes sense to provide some common value, but I don't know how useful it can be to the user (still if the user needs any average).
Joonas, Chris... any idea?
Case in point, and please correct me if I am wrong, legacy rc6_enable returns tile zero, while residency returns average.
As the interface is done now, the rc6_enable is just returning whether the gpu (i.e. i915, not gt) supports RC6 or not. I think there is a patch later.
Even the deprecated message gets logged with every access right?
Btw is the deperecated message limited to multi-tile platforms (can't see that it is) and what is the plan for that?
yes, at this point the message would need to be removed and I forgot to do it.
Maybe it is correct to have it, I don't know at this point. Is the plan to remove the warning everywhere, or only have it on multi-tile platforms, or new platforms? And/or remove legacy files after a while on all platforms, or just new ones?
Regards,
Tvrtko
Hi Tvrtko,
Now tiles have their own sysfs interfaces under the gt/ directory. Because RC6 is a property that can be configured on a tile basis, then each tile should have its own interface
The new sysfs structure will have a similar layout for the 4 tile case:
/sys/.../card0 ├── gt │ ├── gt0 │ │ ├── id │ │ ├── rc6_enable │ │ ├── rc6_residency_ms . . . . . . . . │ └── gtN │ ├── id │ ├── rc6_enable │ ├── rc6_residency_ms │ . │ . │ └── power/ -+ ├── rc6_enable | Original interface ├── rc6_residency_ms +-> kept as existing ABI; . | it multiplexes over . | the GTs -+
The existing interfaces have been kept in their original location to preserve the existing ABI. They act on all the GTs: when reading they provide the average value from all the GTs.
Average feels very odd to me. I'd ask if we can get away providing an errno instead? Or tile zero data?
Real multiplexing would be providing something when reading and when writing. The idea of average came while revieweing with Chris the write multiplexing. Indeed it makes sense to provide some common value, but I don't know how useful it can be to the user (still if the user needs any average).
Joonas, Chris... any idea?
Case in point, and please correct me if I am wrong, legacy rc6_enable returns tile zero, while residency returns average.
As the interface is done now, the rc6_enable is just returning whether the gpu (i.e. i915, not gt) supports RC6 or not. I think there is a patch later.
Even the deprecated message gets logged with every access right?
Btw is the deperecated message limited to multi-tile platforms (can't see that it is) and what is the plan for that?
yes, at this point the message would need to be removed and I forgot to do it.
Maybe it is correct to have it, I don't know at this point. Is the plan to remove the warning everywhere, or only have it on multi-tile platforms, or new platforms? And/or remove legacy files after a while on all platforms, or just new ones?
At this point I guess the warning should be removed from everywhere (i.e. only those RC6 and RPS interfaces that are duplicated/multiplexed).
We shouldn't be supposed to need more usage of multiplexed interfaces in the future (maybe just rc6 enable, but I don't see it really necessary).
Thanks, Andi
Quoting Andi Shyti (2022-02-17 17:53:58)
Hi Tvrtko,
Now tiles have their own sysfs interfaces under the gt/ directory. Because RC6 is a property that can be configured on a tile basis, then each tile should have its own interface
The new sysfs structure will have a similar layout for the 4 tile case:
/sys/.../card0 \u251c\u2500\u2500 gt \u2502 \u251c\u2500\u2500 gt0 \u2502 \u2502 \u251c\u2500\u2500 id \u2502 \u2502 \u251c\u2500\u2500 rc6_enable \u2502 \u2502 \u251c\u2500\u2500 rc6_residency_ms . . . . . . . . \u2502 \u2514\u2500\u2500 gtN \u2502 \u251c\u2500\u2500 id \u2502 \u251c\u2500\u2500 rc6_enable \u2502 \u251c\u2500\u2500 rc6_residency_ms \u2502 . \u2502 . \u2502 \u2514\u2500\u2500 power/ -+ \u251c\u2500\u2500 rc6_enable | Original interface \u251c\u2500\u2500 rc6_residency_ms +-> kept as existing ABI; . | it multiplexes over . | the GTs -+
The existing interfaces have been kept in their original location to preserve the existing ABI. They act on all the GTs: when reading they provide the average value from all the GTs.
Average feels very odd to me. I'd ask if we can get away providing an errno instead? Or tile zero data?
Tile zero data is always wrong, in my opinion. If we have round-robin scaling workloads like some media cases, part of the system load might just disappear when it goes to tile 1.
Real multiplexing would be providing something when reading and when writing. The idea of average came while revieweing with Chris the write multiplexing. Indeed it makes sense to provide some common value, but I don't know how useful it can be to the user (still if the user needs any average).
I think all read/write controls like min/max/boost_freq should return an error from the global interface if all the tiles don't return same value. Write will always overwrite per-tile values.
When we have frequency readbacks without control, returning MAX() across tiles would be the logical thing. The fact that parts of the hardware can be clocked lower when one part is fully utilized is the "new feature".
After that we're only really left with the rc6_residency_ms. And that is the tough one. I'm inclined that MIN() across tiles would be the right answer. If you are fully utilizing a single tile, you should be able to see it.
This all would be what feels natural for an user who has their setup tuned for single-tile device. And would allow simple round-robing balancing across the tiles in somewhat coherent manner.
Regards, Joonas
Joonas, Chris... any idea?
Case in point, and please correct me if I am wrong, legacy rc6_enable returns tile zero, while residency returns average.
As the interface is done now, the rc6_enable is just returning whether the gpu (i.e. i915, not gt) supports RC6 or not. I think there is a patch later.
Even the deprecated message gets logged with every access right?
Btw is the deperecated message limited to multi-tile platforms (can't see that it is) and what is the plan for that?
yes, at this point the message would need to be removed and I forgot to do it.
It's a tough problem, no easy answers even after all this time. :D
yeah! quite hard to get it conceptually right!
Thanks Tvrtko, Andi
On 18/02/2022 10:46, Joonas Lahtinen wrote:
Quoting Andi Shyti (2022-02-17 17:53:58)
Hi Tvrtko,
Now tiles have their own sysfs interfaces under the gt/ directory. Because RC6 is a property that can be configured on a tile basis, then each tile should have its own interface
The new sysfs structure will have a similar layout for the 4 tile case:
/sys/.../card0 \u251c\u2500\u2500 gt \u2502 \u251c\u2500\u2500 gt0 \u2502 \u2502 \u251c\u2500\u2500 id \u2502 \u2502 \u251c\u2500\u2500 rc6_enable \u2502 \u2502 \u251c\u2500\u2500 rc6_residency_ms . . . . . . . . \u2502 \u2514\u2500\u2500 gtN \u2502 \u251c\u2500\u2500 id \u2502 \u251c\u2500\u2500 rc6_enable \u2502 \u251c\u2500\u2500 rc6_residency_ms \u2502 . \u2502 . \u2502 \u2514\u2500\u2500 power/ -+ \u251c\u2500\u2500 rc6_enable | Original interface \u251c\u2500\u2500 rc6_residency_ms +-> kept as existing ABI; . | it multiplexes over . | the GTs -+
The existing interfaces have been kept in their original location to preserve the existing ABI. They act on all the GTs: when reading they provide the average value from all the GTs.
Average feels very odd to me. I'd ask if we can get away providing an errno instead? Or tile zero data?
Tile zero data is always wrong, in my opinion. If we have round-robin scaling workloads like some media cases, part of the system load might just disappear when it goes to tile 1.
I was thinking that in conjunction with deprecated log message it wouldn't be wrong - I mean if the route take was to eventually retire the legacy files altogether.
Real multiplexing would be providing something when reading and when writing. The idea of average came while revieweing with Chris the write multiplexing. Indeed it makes sense to provide some common value, but I don't know how useful it can be to the user (still if the user needs any average).
I think all read/write controls like min/max/boost_freq should return an error from the global interface if all the tiles don't return same value. Write will always overwrite per-tile values.
That would work I think, if the option chosen was not to retire the legacy files.
When we have frequency readbacks without control, returning MAX() across tiles would be the logical thing. The fact that parts of the hardware can be clocked lower when one part is fully utilized is the "new feature".
After that we're only really left with the rc6_residency_ms. And that is the tough one. I'm inclined that MIN() across tiles would be the right answer. If you are fully utilizing a single tile, you should be able to see it.
So we have MIN, AVG or SUM, or errno, or remove the file (which is just a different kind of errno?) to choose from. :)
Regards,
Tvrtko
This all would be what feels natural for an user who has their setup tuned for single-tile device. And would allow simple round-robing balancing across the tiles in somewhat coherent manner.
Hi Tvrtko and Joonas,
Now tiles have their own sysfs interfaces under the gt/ directory. Because RC6 is a property that can be configured on a tile basis, then each tile should have its own interface
The new sysfs structure will have a similar layout for the 4 tile case:
/sys/.../card0 \u251c\u2500\u2500 gt \u2502 \u251c\u2500\u2500 gt0 \u2502 \u2502 \u251c\u2500\u2500 id \u2502 \u2502 \u251c\u2500\u2500 rc6_enable \u2502 \u2502 \u251c\u2500\u2500 rc6_residency_ms . . . . . . . . \u2502 \u2514\u2500\u2500 gtN \u2502 \u251c\u2500\u2500 id \u2502 \u251c\u2500\u2500 rc6_enable \u2502 \u251c\u2500\u2500 rc6_residency_ms \u2502 . \u2502 . \u2502 \u2514\u2500\u2500 power/ -+ \u251c\u2500\u2500 rc6_enable | Original interface \u251c\u2500\u2500 rc6_residency_ms +-> kept as existing ABI; . | it multiplexes over . | the GTs -+
The existing interfaces have been kept in their original location to preserve the existing ABI. They act on all the GTs: when reading they provide the average value from all the GTs.
Average feels very odd to me. I'd ask if we can get away providing an errno instead? Or tile zero data?
Tile zero data is always wrong, in my opinion. If we have round-robin scaling workloads like some media cases, part of the system load might just disappear when it goes to tile 1.
I was thinking that in conjunction with deprecated log message it wouldn't be wrong - I mean if the route take was to eventually retire the legacy files altogether.
that's a good point... do we want to treat the legacy interfaces as an error or do we want to make them a feature? As the discussion is turning those interfaces are becoming a feature. But what are we going to do with the coming interfaces?
E.g. in the future we will have the rc6_enable/disable that can be a command, so that we will add the "_store" interface per tile. What are we going to do with the above interfaces? Are we going to add a multiplexed command as well?
When we have frequency readbacks without control, returning MAX() across tiles would be the logical thing. The fact that parts of the hardware can be clocked lower when one part is fully utilized is the "new feature".
After that we're only really left with the rc6_residency_ms. And that is the tough one. I'm inclined that MIN() across tiles would be the right answer. If you are fully utilizing a single tile, you should be able to see it.
So we have MIN, AVG or SUM, or errno, or remove the file (which is just a different kind of errno?) to choose from. :)
in this case it would just be MIN and MAX. At the end we have here only two types of interface: frequencies and residency_ms. For the first type we would use 'max', for the second 'min'.
But the question holds in case we want keep adding interfaces to the above directories.
Andi
Hi Andi,
I love your patch! Yet something to improve:
[auto build test ERROR on drm-tip/drm-tip] [cannot apply to drm-intel/for-linux-next drm-exynos/exynos-drm-next drm/drm-next tegra-drm/drm/tegra/for-next airlied/drm-next v5.17-rc4 next-20220217] [If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use '--base' as documented in https://git-scm.com/docs/git-format-patch]
url: https://github.com/0day-ci/linux/commits/Andi-Shyti/Introduce-multitile-supp... base: git://anongit.freedesktop.org/drm/drm-tip drm-tip config: i386-defconfig (https://download.01.org/0day-ci/archive/20220218/202202180400.pPkEh3Z4-lkp@i...) compiler: gcc-9 (Debian 9.3.0-22) 9.3.0 reproduce (this is a W=1 build): # https://github.com/0day-ci/linux/commit/b358d991c154dc27fa4ef2fc99f8819f4f3e... git remote add linux-review https://github.com/0day-ci/linux git fetch --no-tags linux-review Andi-Shyti/Introduce-multitile-support/20220217-224547 git checkout b358d991c154dc27fa4ef2fc99f8819f4f3e97e7 # save the config file to linux build tree mkdir build_dir make W=1 O=build_dir ARCH=i386 SHELL=/bin/bash
If you fix the issue, kindly add following tag as appropriate Reported-by: kernel test robot lkp@intel.com
All errors (new ones prefixed by >>):
ld: drivers/gpu/drm/i915/gt/intel_gt_sysfs_pm.o: in function `sysfs_gt_attribute_r_func.isra.0':
intel_gt_sysfs_pm.c:(.text+0x1b2): undefined reference to `__divdi3'
--- 0-DAY CI Kernel Test Service, Intel Corporation https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
Hi Andi,
I love your patch! Yet something to improve:
[auto build test ERROR on drm-tip/drm-tip] [cannot apply to drm-intel/for-linux-next drm-exynos/exynos-drm-next drm/drm-next tegra-drm/drm/tegra/for-next airlied/drm-next v5.17-rc4 next-20220217] [If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use '--base' as documented in https://git-scm.com/docs/git-format-patch]
url: https://github.com/0day-ci/linux/commits/Andi-Shyti/Introduce-multitile-supp... base: git://anongit.freedesktop.org/drm/drm-tip drm-tip config: i386-randconfig-a004 (https://download.01.org/0day-ci/archive/20220218/202202180713.xhySMQW4-lkp@i...) compiler: clang version 15.0.0 (https://github.com/llvm/llvm-project d271fc04d5b97b12e6b797c6067d3c96a8d7470e) reproduce (this is a W=1 build): wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross chmod +x ~/bin/make.cross # https://github.com/0day-ci/linux/commit/b358d991c154dc27fa4ef2fc99f8819f4f3e... git remote add linux-review https://github.com/0day-ci/linux git fetch --no-tags linux-review Andi-Shyti/Introduce-multitile-support/20220217-224547 git checkout b358d991c154dc27fa4ef2fc99f8819f4f3e97e7 # save the config file to linux build tree mkdir build_dir COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=i386 SHELL=/bin/bash
If you fix the issue, kindly add following tag as appropriate Reported-by: kernel test robot lkp@intel.com
All errors (new ones prefixed by >>):
ld.lld: error: undefined symbol: __divdi3
referenced by intel_gt_sysfs_pm.c:35 (drivers/gpu/drm/i915/gt/intel_gt_sysfs_pm.c:35) gpu/drm/i915/gt/intel_gt_sysfs_pm.o:(rc6_residency_ms_show) in archive drivers/built-in.a referenced by intel_gt_sysfs_pm.c:35 (drivers/gpu/drm/i915/gt/intel_gt_sysfs_pm.c:35) gpu/drm/i915/gt/intel_gt_sysfs_pm.o:(rc6p_residency_ms_show) in archive drivers/built-in.a referenced by intel_gt_sysfs_pm.c:35 (drivers/gpu/drm/i915/gt/intel_gt_sysfs_pm.c:35) gpu/drm/i915/gt/intel_gt_sysfs_pm.o:(rc6pp_residency_ms_show) in archive drivers/built-in.a referenced 1 more times
--- 0-DAY CI Kernel Test Service, Intel Corporation https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
On 17.02.2022 15:41, Andi Shyti wrote:
Now tiles have their own sysfs interfaces under the gt/ directory. Because RC6 is a property that can be configured on a tile basis, then each tile should have its own interface
The new sysfs structure will have a similar layout for the 4 tile case:
/sys/.../card0 ├── gt │ ├── gt0 │ │ ├── id │ │ ├── rc6_enable │ │ ├── rc6_residency_ms . . . . . . . . │ └── gtN │ ├── id │ ├── rc6_enable │ ├── rc6_residency_ms │ . │ . │ └── power/ -+ ├── rc6_enable | Original interface ├── rc6_residency_ms +-> kept as existing ABI; . | it multiplexes over . | the GTs -+
The existing interfaces have been kept in their original location to preserve the existing ABI. They act on all the GTs: when reading they provide the average value from all the GTs.
If ABI should be kept forever, depreciation msg should be removed from intel_gt_sysfs_get_drvdata.
This patch is not really adding exposing new interfaces (new ABI) other than adapting the existing one to more tiles. In any case this new set of interfaces will be a basic tool for system managers and administrators when using i915. Signed-off-by: Andi Shyti andi.shyti@linux.intel.com Signed-off-by: Lucas De Marchi lucas.demarchi@intel.com Cc: Chris Wilson chris@chris-wilson.co.uk Cc: Joonas Lahtinen joonas.lahtinen@linux.intel.com Cc: Matt Roper matthew.d.roper@intel.com Cc: Sujaritha Sundaresan sujaritha.sundaresan@intel.com Cc: Tvrtko Ursulin tvrtko.ursulin@intel.com
drivers/gpu/drm/i915/Makefile | 1 + drivers/gpu/drm/i915/gt/intel_gt_sysfs.c | 19 ++ drivers/gpu/drm/i915/gt/intel_gt_sysfs_pm.c | 220 ++++++++++++++++++++ drivers/gpu/drm/i915/gt/intel_gt_sysfs_pm.h | 15 ++ drivers/gpu/drm/i915/i915_sysfs.c | 128 ------------ 5 files changed, 255 insertions(+), 128 deletions(-) create mode 100644 drivers/gpu/drm/i915/gt/intel_gt_sysfs_pm.c create mode 100644 drivers/gpu/drm/i915/gt/intel_gt_sysfs_pm.h
diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile index 277064b00afd..7104558b81d5 100644 --- a/drivers/gpu/drm/i915/Makefile +++ b/drivers/gpu/drm/i915/Makefile @@ -106,6 +106,7 @@ gt-y += \ gt/intel_gt_pm_irq.o \ gt/intel_gt_requests.o \ gt/intel_gt_sysfs.o \
- gt/intel_gt_sysfs_pm.o \ gt/intel_gtt.o \ gt/intel_llc.o \ gt/intel_lrc.o \
diff --git a/drivers/gpu/drm/i915/gt/intel_gt_sysfs.c b/drivers/gpu/drm/i915/gt/intel_gt_sysfs.c index 0206e9aa4867..132b90247a41 100644 --- a/drivers/gpu/drm/i915/gt/intel_gt_sysfs.c +++ b/drivers/gpu/drm/i915/gt/intel_gt_sysfs.c @@ -13,6 +13,7 @@ #include "i915_sysfs.h" #include "intel_gt.h" #include "intel_gt_sysfs.h" +#include "intel_gt_sysfs_pm.h" #include "intel_gt_types.h" #include "intel_rc6.h"
@@ -54,6 +55,11 @@ struct intel_gt *intel_gt_sysfs_get_drvdata(struct device *dev, return kobj_to_gt(kobj); }
+static struct kobject *gt_get_parent_obj(struct intel_gt *gt) +{
- return >->i915->drm.primary->kdev->kobj;
+}
- static ssize_t id_show(struct device *dev, struct device_attribute *attr, char *buf)
@@ -107,6 +113,17 @@ void intel_gt_sysfs_register(struct intel_gt *gt) struct kobject *dir; char name[80];
/*
* We need to make things right with the* ABI compatibility. The files were originally* generated under the parent directory.** We generate the files only for gt 0* to avoid duplicates.*/if (gt_is_root(gt))
intel_gt_sysfs_pm_init(gt, gt_get_parent_obj(gt));snprintf(name, sizeof(name), "gt%d", gt->info.id);
dir = intel_gt_create_kobj(gt, gt->i915->sysfs_gt, name);
@@ -115,4 +132,6 @@ void intel_gt_sysfs_register(struct intel_gt *gt) "failed to initialize %s sysfs root\n", name); return; }
- intel_gt_sysfs_pm_init(gt, dir); }
diff --git a/drivers/gpu/drm/i915/gt/intel_gt_sysfs_pm.c b/drivers/gpu/drm/i915/gt/intel_gt_sysfs_pm.c new file mode 100644 index 000000000000..27d93a36894a --- /dev/null +++ b/drivers/gpu/drm/i915/gt/intel_gt_sysfs_pm.c @@ -0,0 +1,220 @@ +// SPDX-License-Identifier: MIT +/*
- Copyright © 2022 Intel Corporation
- */
+#include <drm/drm_device.h> +#include <linux/sysfs.h> +#include <linux/printk.h>
+#include "i915_drv.h" +#include "i915_sysfs.h" +#include "intel_gt.h" +#include "intel_gt_regs.h" +#include "intel_gt_sysfs.h" +#include "intel_gt_sysfs_pm.h" +#include "intel_rc6.h"
+#ifdef CONFIG_PM +static s64 +sysfs_gt_attribute_r_func(struct device *dev, struct device_attribute *attr,
s64 (func)(struct intel_gt *gt))+{
- struct intel_gt *gt;
- s64 ret = 0;
- if (!is_object_gt(&dev->kobj)) {
int i, num_gt = 0;struct drm_i915_private *i915 = kdev_minor_to_i915(dev);for_each_gt(gt, i915, i) {ret += func(gt);num_gt++;}ret /= num_gt;- } else {
gt = intel_gt_sysfs_get_drvdata(dev, attr->attr.name);ret = func(gt);- }
- return ret;
+}
Return value is always converted to u32 or int, maybe we could use just int ?
+static u32 get_residency(struct intel_gt *gt, i915_reg_t reg) +{
- intel_wakeref_t wakeref;
- u64 res = 0;
- with_intel_runtime_pm(gt->uncore->rpm, wakeref)
res = intel_rc6_residency_us(>->rc6, reg);- return DIV_ROUND_CLOSEST_ULL(res, 1000);
+}
+static ssize_t rc6_enable_show(struct device *dev,
struct device_attribute *attr,char *buff)+{
- struct intel_gt *gt = intel_gt_sysfs_get_drvdata(dev, attr->attr.name);
- u8 mask = 0;
- if (HAS_RC6(gt->i915))
mask |= BIT(0);- if (HAS_RC6p(gt->i915))
mask |= BIT(1);- if (HAS_RC6pp(gt->i915))
mask |= BIT(2);- return sysfs_emit(buff, "%x\n", mask);
+}
+static s64 __rc6_residency_ms_show(struct intel_gt *gt) +{
- return get_residency(gt, GEN6_GT_GFX_RC6);
+}
+static ssize_t rc6_residency_ms_show(struct device *dev,
struct device_attribute *attr,char *buff)+{
- s64 rc6_residency = sysfs_gt_attribute_r_func(dev, attr,
__rc6_residency_ms_show);- return sysfs_emit(buff, "%u\n", (u32) rc6_residency);
Here and below (where applicable) variable should be just u32, no need to conversion in sysfs_emit.
+}
+static s64 __rc6p_residency_ms_show(struct intel_gt *gt) +{
- return get_residency(gt, GEN6_GT_GFX_RC6p);
+}
+static ssize_t rc6p_residency_ms_show(struct device *dev,
struct device_attribute *attr,char *buff)+{
- s64 rc6p_residency = sysfs_gt_attribute_r_func(dev, attr,
__rc6p_residency_ms_show);- return sysfs_emit(buff, "%u\n", (u32) rc6p_residency);
+}
+static s64 __rc6pp_residency_ms_show(struct intel_gt *gt) +{
- return get_residency(gt, GEN6_GT_GFX_RC6pp);
+}
+static ssize_t rc6pp_residency_ms_show(struct device *dev,
struct device_attribute *attr,char *buff)+{
- s64 rc6pp_residency = sysfs_gt_attribute_r_func(dev, attr,
__rc6pp_residency_ms_show);- return sysfs_emit(buff, "%u\n", (u32) rc6pp_residency);
+}
+static s64 __media_rc6_residency_ms_show(struct intel_gt *gt) +{
- return get_residency(gt, VLV_GT_MEDIA_RC6);
+}
+static ssize_t media_rc6_residency_ms_show(struct device *dev,
struct device_attribute *attr,char *buff)+{
- s64 rc6_residency = sysfs_gt_attribute_r_func(dev, attr,
__media_rc6_residency_ms_show);- return sysfs_emit(buff, "%u\n", (u32) rc6_residency);
+}
+static DEVICE_ATTR_RO(rc6_enable); +static DEVICE_ATTR_RO(rc6_residency_ms); +static DEVICE_ATTR_RO(rc6p_residency_ms); +static DEVICE_ATTR_RO(rc6pp_residency_ms); +static DEVICE_ATTR_RO(media_rc6_residency_ms);
+static struct attribute *rc6_attrs[] = {
- &dev_attr_rc6_enable.attr,
- &dev_attr_rc6_residency_ms.attr,
- NULL
+};
+static struct attribute *rc6p_attrs[] = {
- &dev_attr_rc6p_residency_ms.attr,
- &dev_attr_rc6pp_residency_ms.attr,
- NULL
+};
+static struct attribute *media_rc6_attrs[] = {
- &dev_attr_media_rc6_residency_ms.attr,
- NULL
+};
+static const struct attribute_group rc6_attr_group[] = {
- { .attrs = rc6_attrs, },
- { .name = power_group_name, .attrs = rc6_attrs, },
+};
+static const struct attribute_group rc6p_attr_group[] = {
- { .attrs = rc6p_attrs, },
- { .name = power_group_name, .attrs = rc6p_attrs, },
+};
+static const struct attribute_group media_rc6_attr_group[] = {
- { .attrs = media_rc6_attrs, },
- { .name = power_group_name, .attrs = media_rc6_attrs, },
+};
+static int __intel_gt_sysfs_create_group(struct kobject *kobj,
const struct attribute_group *grp)+{
- return is_object_gt(kobj) ?
sysfs_create_group(kobj, &grp[0]) :sysfs_merge_group(kobj, &grp[1]);+}
Merging handling "gt/gt#/*" and "power/*" attributes into the same helpers seems unnatural to me, in many functions we have two branches based on value of is_object_gt, with the most hacky intel_gt_sysfs_get_drvdata. Splitting handling these attributes would allow to drop fragile is_object_gt helper and make functions more straightforward, probably at the cost of few lines more. On the other side I am not sure if it is worth to change everything to just address code purity concerns :)
So with variable type adjustement: Reviewed-by: Andrzej Hajda andrzej.hajda@intel.com
Regards Andrzej
Regards Andrzej
+static void intel_sysfs_rc6_init(struct intel_gt *gt, struct kobject *kobj) +{
- int ret;
- if (!HAS_RC6(gt->i915))
return;- ret = __intel_gt_sysfs_create_group(kobj, rc6_attr_group);
- if (ret)
drm_warn(>->i915->drm,"failed to create gt%u RC6 sysfs files\n",gt->info.id);- /*
* cannot use the is_visible() attribute because* the upper object inherits from the parent group.*/- if (HAS_RC6p(gt->i915)) {
ret = __intel_gt_sysfs_create_group(kobj, rc6p_attr_group);if (ret)drm_warn(>->i915->drm,"failed to create gt%u RC6p sysfs files\n",gt->info.id);- }
- if (IS_VALLEYVIEW(gt->i915) || IS_CHERRYVIEW(gt->i915)) {
ret = __intel_gt_sysfs_create_group(kobj, media_rc6_attr_group);if (ret)drm_warn(>->i915->drm,"failed to create media %u RC6 sysfs files\n",gt->info.id);- }
+} +#else +static void intel_sysfs_rc6_init(struct intel_gt *gt, struct kobject *kobj) +{ +} +#endif /* CONFIG_PM */
+void intel_gt_sysfs_pm_init(struct intel_gt *gt, struct kobject *kobj) +{
- intel_sysfs_rc6_init(gt, kobj);
+} diff --git a/drivers/gpu/drm/i915/gt/intel_gt_sysfs_pm.h b/drivers/gpu/drm/i915/gt/intel_gt_sysfs_pm.h new file mode 100644 index 000000000000..f567105a4a89 --- /dev/null +++ b/drivers/gpu/drm/i915/gt/intel_gt_sysfs_pm.h @@ -0,0 +1,15 @@ +/* SPDX-License-Identifier: MIT */ +/*
- Copyright © 2022 Intel Corporation
- */
+#ifndef __SYSFS_GT_PM_H__ +#define __SYSFS_GT_PM_H__
+#include <linux/kobject.h>
+#include "intel_gt_types.h"
+void intel_gt_sysfs_pm_init(struct intel_gt *gt, struct kobject *kobj);
+#endif /* SYSFS_RC6_H */ diff --git a/drivers/gpu/drm/i915/i915_sysfs.c b/drivers/gpu/drm/i915/i915_sysfs.c index 3643efde52ca..b08a959e5ccc 100644 --- a/drivers/gpu/drm/i915/i915_sysfs.c +++ b/drivers/gpu/drm/i915/i915_sysfs.c @@ -45,107 +45,6 @@ struct drm_i915_private *kdev_minor_to_i915(struct device *kdev) return to_i915(minor->dev); }
-#ifdef CONFIG_PM -static u32 calc_residency(struct drm_i915_private *dev_priv,
i915_reg_t reg)-{
- intel_wakeref_t wakeref;
- u64 res = 0;
- with_intel_runtime_pm(&dev_priv->runtime_pm, wakeref)
res = intel_rc6_residency_us(&to_gt(dev_priv)->rc6, reg);- return DIV_ROUND_CLOSEST_ULL(res, 1000);
-}
-static ssize_t rc6_enable_show(struct device *kdev,
struct device_attribute *attr, char *buf)-{
- struct drm_i915_private *dev_priv = kdev_minor_to_i915(kdev);
- unsigned int mask;
- mask = 0;
- if (HAS_RC6(dev_priv))
mask |= BIT(0);- if (HAS_RC6p(dev_priv))
mask |= BIT(1);- if (HAS_RC6pp(dev_priv))
mask |= BIT(2);- return sysfs_emit(buf, "%x\n", mask);
-}
-static ssize_t rc6_residency_ms_show(struct device *kdev,
struct device_attribute *attr, char *buf)-{
- struct drm_i915_private *dev_priv = kdev_minor_to_i915(kdev);
- u32 rc6_residency = calc_residency(dev_priv, GEN6_GT_GFX_RC6);
- return sysfs_emit(buf, "%u\n", rc6_residency);
-}
-static ssize_t rc6p_residency_ms_show(struct device *kdev,
struct device_attribute *attr, char *buf)-{
- struct drm_i915_private *dev_priv = kdev_minor_to_i915(kdev);
- u32 rc6p_residency = calc_residency(dev_priv, GEN6_GT_GFX_RC6p);
- return sysfs_emit(buf, "%u\n", rc6p_residency);
-}
-static ssize_t rc6pp_residency_ms_show(struct device *kdev,
struct device_attribute *attr, char *buf)-{
- struct drm_i915_private *dev_priv = kdev_minor_to_i915(kdev);
- u32 rc6pp_residency = calc_residency(dev_priv, GEN6_GT_GFX_RC6pp);
- return sysfs_emit(buf, "%u\n", rc6pp_residency);
-}
-static ssize_t media_rc6_residency_ms_show(struct device *kdev,
struct device_attribute *attr, char *buf)-{
- struct drm_i915_private *dev_priv = kdev_minor_to_i915(kdev);
- u32 rc6_residency = calc_residency(dev_priv, VLV_GT_MEDIA_RC6);
- return sysfs_emit(buf, "%u\n", rc6_residency);
-}
-static DEVICE_ATTR_RO(rc6_enable); -static DEVICE_ATTR_RO(rc6_residency_ms); -static DEVICE_ATTR_RO(rc6p_residency_ms); -static DEVICE_ATTR_RO(rc6pp_residency_ms); -static DEVICE_ATTR_RO(media_rc6_residency_ms);
-static struct attribute *rc6_attrs[] = {
- &dev_attr_rc6_enable.attr,
- &dev_attr_rc6_residency_ms.attr,
- NULL
-};
-static const struct attribute_group rc6_attr_group = {
- .name = power_group_name,
- .attrs = rc6_attrs
-};
-static struct attribute *rc6p_attrs[] = {
- &dev_attr_rc6p_residency_ms.attr,
- &dev_attr_rc6pp_residency_ms.attr,
- NULL
-};
-static const struct attribute_group rc6p_attr_group = {
- .name = power_group_name,
- .attrs = rc6p_attrs
-};
-static struct attribute *media_rc6_attrs[] = {
- &dev_attr_media_rc6_residency_ms.attr,
- NULL
-};
-static const struct attribute_group media_rc6_attr_group = {
- .name = power_group_name,
- .attrs = media_rc6_attrs
-}; -#endif
- static int l3_access_valid(struct drm_i915_private *i915, loff_t offset) { if (!HAS_L3_DPF(i915))
@@ -497,29 +396,6 @@ void i915_setup_sysfs(struct drm_i915_private *dev_priv) struct device *kdev = dev_priv->drm.primary->kdev; int ret;
-#ifdef CONFIG_PM
- if (HAS_RC6(dev_priv)) {
ret = sysfs_merge_group(&kdev->kobj,&rc6_attr_group);if (ret)drm_err(&dev_priv->drm,"RC6 residency sysfs setup failed\n");- }
- if (HAS_RC6p(dev_priv)) {
ret = sysfs_merge_group(&kdev->kobj,&rc6p_attr_group);if (ret)drm_err(&dev_priv->drm,"RC6p residency sysfs setup failed\n");- }
- if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv)) {
ret = sysfs_merge_group(&kdev->kobj,&media_rc6_attr_group);if (ret)drm_err(&dev_priv->drm,"Media RC6 residency sysfs setup failed\n");- }
-#endif if (HAS_L3_DPF(dev_priv)) { ret = device_create_bin_file(kdev, &dpf_attrs); if (ret) @@ -565,8 +441,4 @@ void i915_teardown_sysfs(struct drm_i915_private *dev_priv) sysfs_remove_files(&kdev->kobj, gen6_attrs); device_remove_bin_file(kdev, &dpf_attrs_1); device_remove_bin_file(kdev, &dpf_attrs); -#ifdef CONFIG_PM
- sysfs_unmerge_group(&kdev->kobj, &rc6_attr_group);
- sysfs_unmerge_group(&kdev->kobj, &rc6p_attr_group);
-#endif }
Hi Andrzej,
Now tiles have their own sysfs interfaces under the gt/ directory. Because RC6 is a property that can be configured on a tile basis, then each tile should have its own interface
The new sysfs structure will have a similar layout for the 4 tile case:
/sys/.../card0 ├── gt │ ├── gt0 │ │ ├── id │ │ ├── rc6_enable │ │ ├── rc6_residency_ms . . . . . . . . │ └── gtN │ ├── id │ ├── rc6_enable │ ├── rc6_residency_ms │ . │ . │ └── power/ -+ ├── rc6_enable | Original interface ├── rc6_residency_ms +-> kept as existing ABI; . | it multiplexes over . | the GTs -+
The existing interfaces have been kept in their original location to preserve the existing ABI. They act on all the GTs: when reading they provide the average value from all the GTs.
If ABI should be kept forever, depreciation msg should be removed from intel_gt_sysfs_get_drvdata.
yes... to be removed!
+#ifdef CONFIG_PM +static s64 +sysfs_gt_attribute_r_func(struct device *dev, struct device_attribute *attr,
s64 (func)(struct intel_gt *gt))+{
- struct intel_gt *gt;
- s64 ret = 0;
- if (!is_object_gt(&dev->kobj)) {
int i, num_gt = 0;struct drm_i915_private *i915 = kdev_minor_to_i915(dev);for_each_gt(gt, i915, i) {ret += func(gt);num_gt++;}ret /= num_gt;- } else {
gt = intel_gt_sysfs_get_drvdata(dev, attr->attr.name);ret = func(gt);- }
- return ret;
+}
Return value is always converted to u32 or int, maybe we could use just int ?
there is one case when the value to be returned is an 'int' and that is the "int intel_gpu_freq()". That's why I supposed that s64 was the right size. But I don't see how it can be negative so that I will make it u32. Perhaps we need to change intel_gpu_freq to be u32.
(Didn't want to take it too far, but I was also thinking that in the future we might need to return error values)
+static ssize_t rc6_residency_ms_show(struct device *dev,
struct device_attribute *attr,char *buff)+{
- s64 rc6_residency = sysfs_gt_attribute_r_func(dev, attr,
__rc6_residency_ms_show);- return sysfs_emit(buff, "%u\n", (u32) rc6_residency);
Here and below (where applicable) variable should be just u32, no need to conversion in sysfs_emit.
yep! same comment as above.
+static int __intel_gt_sysfs_create_group(struct kobject *kobj,
const struct attribute_group *grp)+{
- return is_object_gt(kobj) ?
sysfs_create_group(kobj, &grp[0]) :sysfs_merge_group(kobj, &grp[1]);+}
Merging handling "gt/gt#/*" and "power/*" attributes into the same helpers seems unnatural to me, in many functions we have two branches based on value of is_object_gt, with the most hacky intel_gt_sysfs_get_drvdata. Splitting handling these attributes would allow to drop fragile is_object_gt helper and make functions more straightforward, probably at the cost of few lines more. On the other side I am not sure if it is worth to change everything to just address code purity concerns :)
I the amount of duplicated code would be too high and there have been already complaints some times in the past (e.g. we have had same discussion in the case of the debugfs files).
But it's true that is quite hard to find the correct balance between duplicated code and compact code.
So with variable type adjustement: Reviewed-by: Andrzej Hajda andrzej.hajda@intel.com
Thanks!
Now tiles have their own sysfs interfaces under the gt/ directory. Because RPS is a property that can be configured on a tile basis, then each tile should have its own interface
The new sysfs structure will have a similar layout for the 4 tile case:
/sys/.../card0 ├── gt │ ├── gt0 │ │ ├── id │ │ ├── rc6_enable │ │ ├── rc6_residency_ms │ │ ├── rps_act_freq_mhz │ │ ├── rps_boost_freq_mhz │ │ ├── rps_cur_freq_mhz │ │ ├── rps_max_freq_mhz │ │ ├── rps_min_freq_mhz │ │ ├── rps_RP0_freq_mhz │ │ ├── rps_RP1_freq_mhz │ │ └── rps_RPn_freq_mhz . . . . . . │ └── gtN │ ├── id │ ├── rc6_enable │ ├── rc6_residency_ms │ ├── rps_act_freq_mhz │ ├── rps_boost_freq_mhz │ ├── rps_cur_freq_mhz │ ├── rps_max_freq_mhz │ ├── rps_min_freq_mhz │ ├── rps_RP0_freq_mhz │ ├── rps_RP1_freq_mhz │ └── rps_RPn_freq_mhz ├── gt_act_freq_mhz -+ ├── gt_boost_freq_mhz | ├── gt_cur_freq_mhz | Original interface ├── gt_max_freq_mhz +─-> kept as existing ABI; ├── gt_min_freq_mhz | it points to gt0/ ├── gt_RP0_freq_mhz | ├── gt_RP1_freq_mhz | └── gt_RPn_freq_mhz -+
The existing interfaces have been kept in their original location to preserve the existing ABI. They act on all the GTs: when writing they loop through all the GTs and write the information on each interface. When reading they provide the average value from all the GTs.
This patch is not really adding exposing new interfaces (new ABI) other than adapting the existing one to more tiles. In any case this new set of interfaces will be a basic tool for system managers and administrators when using i915.
Signed-off-by: Andi Shyti andi.shyti@linux.intel.com Signed-off-by: Lucas De Marchi lucas.demarchi@intel.com Cc: Chris Wilson chris@chris-wilson.co.uk Cc: Joonas Lahtinen joonas.lahtinen@linux.intel.com Cc: Matt Roper matthew.d.roper@intel.com Cc: Sujaritha Sundaresan sujaritha.sundaresan@intel.com Cc: Tvrtko Ursulin tvrtko.ursulin@intel.com --- drivers/gpu/drm/i915/gt/intel_gt_sysfs_pm.c | 276 ++++++++++++++++++++ drivers/gpu/drm/i915/i915_sysfs.c | 177 ------------- 2 files changed, 276 insertions(+), 177 deletions(-)
diff --git a/drivers/gpu/drm/i915/gt/intel_gt_sysfs_pm.c b/drivers/gpu/drm/i915/gt/intel_gt_sysfs_pm.c index 27d93a36894a..8e86b8f675f1 100644 --- a/drivers/gpu/drm/i915/gt/intel_gt_sysfs_pm.c +++ b/drivers/gpu/drm/i915/gt/intel_gt_sysfs_pm.c @@ -14,8 +14,33 @@ #include "intel_gt_sysfs.h" #include "intel_gt_sysfs_pm.h" #include "intel_rc6.h" +#include "intel_rps.h"
#ifdef CONFIG_PM +static int +sysfs_gt_attribute_w_func(struct device *dev, struct device_attribute *attr, + int (func)(struct intel_gt *gt, u32 val), u32 val) +{ + struct intel_gt *gt; + int ret; + + if (!is_object_gt(&dev->kobj)) { + int i; + struct drm_i915_private *i915 = kdev_minor_to_i915(dev); + + for_each_gt(gt, i915, i) { + ret = func(gt, val); + if (ret) + break; + } + } else { + gt = intel_gt_sysfs_get_drvdata(dev, attr->attr.name); + ret = func(gt, val); + } + + return ret; +} + static s64 sysfs_gt_attribute_r_func(struct device *dev, struct device_attribute *attr, s64 (func)(struct intel_gt *gt)) @@ -214,7 +239,258 @@ static void intel_sysfs_rc6_init(struct intel_gt *gt, struct kobject *kobj) } #endif /* CONFIG_PM */
+static s64 __act_freq_mhz_show(struct intel_gt *gt) +{ + return intel_rps_read_actual_frequency(>->rps); +} + +static ssize_t act_freq_mhz_show(struct device *dev, + struct device_attribute *attr, char *buff) +{ + s64 actual_freq = sysfs_gt_attribute_r_func(dev, attr, + __act_freq_mhz_show); + + return sysfs_emit(buff, "%u\n", (u32) actual_freq); +} + +static s64 __cur_freq_mhz_show(struct intel_gt *gt) +{ + return intel_rps_get_requested_frequency(>->rps); +} + +static ssize_t cur_freq_mhz_show(struct device *dev, + struct device_attribute *attr, char *buff) +{ + s64 cur_freq = sysfs_gt_attribute_r_func(dev, attr, + __cur_freq_mhz_show); + + return sysfs_emit(buff, "%u\n", (u32) cur_freq); +} + +static s64 __boost_freq_mhz_show(struct intel_gt *gt) +{ + return intel_rps_get_boost_frequency(>->rps); +} + +static ssize_t boost_freq_mhz_show(struct device *dev, + struct device_attribute *attr, + char *buff) +{ + s64 boost_freq = sysfs_gt_attribute_r_func(dev, attr, + __boost_freq_mhz_show); + + return sysfs_emit(buff, "%u\n", (u32) boost_freq); +} + +static int __boost_freq_mhz_store(struct intel_gt *gt, u32 val) +{ + struct intel_rps *rps = >->rps; + bool boost = false; + + /* Validate against (static) hardware limits */ + val = intel_freq_opcode(rps, val); + if (val < rps->min_freq || val > rps->max_freq) + return -EINVAL; + + mutex_lock(&rps->lock); + if (val != rps->boost_freq) { + rps->boost_freq = val; + boost = atomic_read(&rps->num_waiters); + } + mutex_unlock(&rps->lock); + if (boost) + schedule_work(&rps->work); + + return 0; +} + +static ssize_t boost_freq_mhz_store(struct device *dev, + struct device_attribute *attr, + const char *buff, size_t count) +{ + ssize_t ret; + u32 val; + + ret = kstrtou32(buff, 0, &val); + if (ret) + return ret; + + return sysfs_gt_attribute_w_func(dev, attr, + __boost_freq_mhz_store, val) ?: count; +} + +static s64 __vlv_rpe_freq_mhz_show(struct intel_gt *gt) +{ + struct intel_rps *rps = >->rps; + + return intel_gpu_freq(rps, rps->efficient_freq); +} + +static ssize_t vlv_rpe_freq_mhz_show(struct device *dev, + struct device_attribute *attr, char *buff) +{ + s64 rpe_freq = sysfs_gt_attribute_r_func(dev, attr, + __vlv_rpe_freq_mhz_show); + + return sysfs_emit(buff, "%d\n", (int) rpe_freq); +} + +static s64 __max_freq_mhz_show(struct intel_gt *gt) +{ + return intel_rps_get_max_frequency(>->rps); +} + +static int __set_max_freq(struct intel_gt *gt, u32 val) +{ + return intel_rps_set_max_frequency(>->rps, val); +} + +static s64 __min_freq_mhz_show(struct intel_gt *gt) +{ + return intel_rps_get_min_frequency(>->rps); +} + +static int __set_min_freq(struct intel_gt *gt, u32 val) +{ + return intel_rps_set_min_frequency(>->rps, val); +} + +static ssize_t min_max_freq_mhz_store(struct device *dev, + struct device_attribute *attr, + const char *buff, size_t count); +static ssize_t min_max_freq_mhz_show(struct device *dev, + struct device_attribute *attr, char *buff); + +#define INTEL_GT_RPS_SYSFS_ATTR(_name, _mode, _show, _store) \ + struct device_attribute dev_attr_gt_##_name = __ATTR(gt_##_name, _mode, _show, _store); \ + struct device_attribute dev_attr_rps_##_name = __ATTR(rps_##_name, _mode, _show, _store) + +#define INTEL_GT_RPS_SYSFS_ATTR_RO(_name) \ + INTEL_GT_RPS_SYSFS_ATTR(_name, 0444, _name##_show, NULL) +#define INTEL_GT_RPS_SYSFS_ATTR_RW(_name) \ + INTEL_GT_RPS_SYSFS_ATTR(_name, 0644, _name##_show, _name##_store) + +static INTEL_GT_RPS_SYSFS_ATTR_RO(act_freq_mhz); +static INTEL_GT_RPS_SYSFS_ATTR_RO(cur_freq_mhz); +static INTEL_GT_RPS_SYSFS_ATTR_RW(boost_freq_mhz); + +static DEVICE_ATTR_RO(vlv_rpe_freq_mhz); + +static ssize_t rps_rp_mhz_show(struct device *dev, + struct device_attribute *attr, + char *buff); + +static INTEL_GT_RPS_SYSFS_ATTR(RP0_freq_mhz, 0444, rps_rp_mhz_show, NULL); +static INTEL_GT_RPS_SYSFS_ATTR(RP1_freq_mhz, 0444, rps_rp_mhz_show, NULL); +static INTEL_GT_RPS_SYSFS_ATTR(RPn_freq_mhz, 0444, rps_rp_mhz_show, NULL); + +INTEL_GT_RPS_SYSFS_ATTR(max_freq_mhz, 0644, min_max_freq_mhz_show, min_max_freq_mhz_store); +INTEL_GT_RPS_SYSFS_ATTR(min_freq_mhz, 0644, min_max_freq_mhz_show, min_max_freq_mhz_store); + +#define GEN6_ATTR(s) { \ + &dev_attr_##s##_act_freq_mhz.attr, \ + &dev_attr_##s##_cur_freq_mhz.attr, \ + &dev_attr_##s##_boost_freq_mhz.attr, \ + &dev_attr_##s##_max_freq_mhz.attr, \ + &dev_attr_##s##_min_freq_mhz.attr, \ + &dev_attr_##s##_RP0_freq_mhz.attr, \ + &dev_attr_##s##_RP1_freq_mhz.attr, \ + &dev_attr_##s##_RPn_freq_mhz.attr, \ + NULL, \ + } + +#define GEN6_RPS_ATTR GEN6_ATTR(rps) +#define GEN6_GT_ATTR GEN6_ATTR(gt) + +static ssize_t min_max_freq_mhz_store(struct device *dev, + struct device_attribute *attr, + const char *buff, size_t count) +{ + long ret; + u32 val; + + ret = kstrtou32(buff, 0, &val); + if (ret) + return ret; + + ret = (attr == &dev_attr_gt_min_freq_mhz || + attr == &dev_attr_rps_min_freq_mhz) ? + sysfs_gt_attribute_w_func(dev, attr, __set_min_freq, val) : + sysfs_gt_attribute_w_func(dev, attr, __set_max_freq, val); + + return ret ?: count; +} + +static ssize_t min_max_freq_mhz_show(struct device *dev, + struct device_attribute *attr, char *buff) +{ + s64 val; + + val = (attr == &dev_attr_gt_min_freq_mhz || + attr == &dev_attr_rps_min_freq_mhz) ? + sysfs_gt_attribute_r_func(dev, attr, __min_freq_mhz_show) : + sysfs_gt_attribute_r_func(dev, attr, __max_freq_mhz_show); + + return sysfs_emit(buff, "%u\n", (u32) val); +} + +/* For now we have a static number of RP states */ +static ssize_t rps_rp_mhz_show(struct device *dev, + struct device_attribute *attr, + char *buff) +{ + struct intel_gt *gt = intel_gt_sysfs_get_drvdata(dev, attr->attr.name); + struct intel_rps *rps = >->rps; + u32 val; + + if (attr == &dev_attr_gt_RP0_freq_mhz || + attr == &dev_attr_rps_RP0_freq_mhz) { + val = intel_rps_get_rp0_frequency(rps); + } else if (attr == &dev_attr_gt_RP1_freq_mhz || + attr == &dev_attr_rps_RP1_freq_mhz) { + val = intel_rps_get_rp1_frequency(rps); + } else if (attr == &dev_attr_gt_RPn_freq_mhz || + attr == &dev_attr_rps_RPn_freq_mhz) { + val = intel_rps_get_rpn_frequency(rps); + } else { + GEM_WARN_ON(1); + return -ENODEV; + } + + return sysfs_emit(buff, "%d\n", val); +} + +static const struct attribute * const gen6_rps_attrs[] = GEN6_RPS_ATTR; +static const struct attribute * const gen6_gt_attrs[] = GEN6_GT_ATTR; + +static int intel_sysfs_rps_init(struct intel_gt *gt, struct kobject *kobj, + const struct attribute * const *attrs) +{ + int ret; + + if (GRAPHICS_VER(gt->i915) < 6) + return 0; + + ret = sysfs_create_files(kobj, attrs); + if (ret) + return ret; + + if (IS_VALLEYVIEW(gt->i915) || IS_CHERRYVIEW(gt->i915)) + ret = sysfs_create_file(kobj, &dev_attr_vlv_rpe_freq_mhz.attr); + + return ret; +} + void intel_gt_sysfs_pm_init(struct intel_gt *gt, struct kobject *kobj) { + int ret; + intel_sysfs_rc6_init(gt, kobj); + + ret = is_object_gt(kobj) ? + intel_sysfs_rps_init(gt, kobj, gen6_rps_attrs) : + intel_sysfs_rps_init(gt, kobj, gen6_gt_attrs); + if (ret) + drm_warn(>->i915->drm, + "failed to create gt%u RPS sysfs files", gt->info.id); } diff --git a/drivers/gpu/drm/i915/i915_sysfs.c b/drivers/gpu/drm/i915/i915_sysfs.c index b08a959e5ccc..0a0057940718 100644 --- a/drivers/gpu/drm/i915/i915_sysfs.c +++ b/drivers/gpu/drm/i915/i915_sysfs.c @@ -156,171 +156,6 @@ static const struct bin_attribute dpf_attrs_1 = { .private = (void *)1 };
-static ssize_t gt_act_freq_mhz_show(struct device *kdev, - struct device_attribute *attr, char *buf) -{ - struct drm_i915_private *i915 = kdev_minor_to_i915(kdev); - struct intel_rps *rps = &to_gt(i915)->rps; - - return sysfs_emit(buf, "%d\n", intel_rps_read_actual_frequency(rps)); -} - -static ssize_t gt_cur_freq_mhz_show(struct device *kdev, - struct device_attribute *attr, char *buf) -{ - struct drm_i915_private *i915 = kdev_minor_to_i915(kdev); - struct intel_rps *rps = &to_gt(i915)->rps; - - return sysfs_emit(buf, "%d\n", intel_rps_get_requested_frequency(rps)); -} - -static ssize_t gt_boost_freq_mhz_show(struct device *kdev, struct device_attribute *attr, char *buf) -{ - struct drm_i915_private *i915 = kdev_minor_to_i915(kdev); - struct intel_rps *rps = &to_gt(i915)->rps; - - return sysfs_emit(buf, "%d\n", intel_rps_get_boost_frequency(rps)); -} - -static ssize_t gt_boost_freq_mhz_store(struct device *kdev, - struct device_attribute *attr, - const char *buf, size_t count) -{ - struct drm_i915_private *dev_priv = kdev_minor_to_i915(kdev); - struct intel_rps *rps = &to_gt(dev_priv)->rps; - ssize_t ret; - u32 val; - - ret = kstrtou32(buf, 0, &val); - if (ret) - return ret; - - ret = intel_rps_set_boost_frequency(rps, val); - - return ret ?: count; -} - -static ssize_t vlv_rpe_freq_mhz_show(struct device *kdev, - struct device_attribute *attr, char *buf) -{ - struct drm_i915_private *dev_priv = kdev_minor_to_i915(kdev); - struct intel_rps *rps = &to_gt(dev_priv)->rps; - - return sysfs_emit(buf, "%d\n", intel_gpu_freq(rps, rps->efficient_freq)); -} - -static ssize_t gt_max_freq_mhz_show(struct device *kdev, struct device_attribute *attr, char *buf) -{ - struct drm_i915_private *dev_priv = kdev_minor_to_i915(kdev); - struct intel_gt *gt = to_gt(dev_priv); - struct intel_rps *rps = >->rps; - - return sysfs_emit(buf, "%d\n", intel_rps_get_max_frequency(rps)); -} - -static ssize_t gt_max_freq_mhz_store(struct device *kdev, - struct device_attribute *attr, - const char *buf, size_t count) -{ - struct drm_i915_private *dev_priv = kdev_minor_to_i915(kdev); - struct intel_gt *gt = to_gt(dev_priv); - struct intel_rps *rps = >->rps; - ssize_t ret; - u32 val; - - ret = kstrtou32(buf, 0, &val); - if (ret) - return ret; - - ret = intel_rps_set_max_frequency(rps, val); - - return ret ?: count; -} - -static ssize_t gt_min_freq_mhz_show(struct device *kdev, struct device_attribute *attr, char *buf) -{ - struct drm_i915_private *i915 = kdev_minor_to_i915(kdev); - struct intel_gt *gt = to_gt(i915); - struct intel_rps *rps = >->rps; - - return sysfs_emit(buf, "%d\n", intel_rps_get_min_frequency(rps)); -} - -static ssize_t gt_min_freq_mhz_store(struct device *kdev, - struct device_attribute *attr, - const char *buf, size_t count) -{ - struct drm_i915_private *i915 = kdev_minor_to_i915(kdev); - struct intel_rps *rps = &to_gt(i915)->rps; - ssize_t ret; - u32 val; - - ret = kstrtou32(buf, 0, &val); - if (ret) - return ret; - - ret = intel_rps_set_min_frequency(rps, val); - - return ret ?: count; -} - -static DEVICE_ATTR_RO(gt_act_freq_mhz); -static DEVICE_ATTR_RO(gt_cur_freq_mhz); -static DEVICE_ATTR_RW(gt_boost_freq_mhz); -static DEVICE_ATTR_RW(gt_max_freq_mhz); -static DEVICE_ATTR_RW(gt_min_freq_mhz); - -static DEVICE_ATTR_RO(vlv_rpe_freq_mhz); - -static ssize_t gt_rp_mhz_show(struct device *kdev, struct device_attribute *attr, char *buf); -static DEVICE_ATTR(gt_RP0_freq_mhz, S_IRUGO, gt_rp_mhz_show, NULL); -static DEVICE_ATTR(gt_RP1_freq_mhz, S_IRUGO, gt_rp_mhz_show, NULL); -static DEVICE_ATTR(gt_RPn_freq_mhz, S_IRUGO, gt_rp_mhz_show, NULL); - -/* For now we have a static number of RP states */ -static ssize_t gt_rp_mhz_show(struct device *kdev, struct device_attribute *attr, char *buf) -{ - struct drm_i915_private *dev_priv = kdev_minor_to_i915(kdev); - struct intel_rps *rps = &to_gt(dev_priv)->rps; - u32 val; - - if (attr == &dev_attr_gt_RP0_freq_mhz) - val = intel_rps_get_rp0_frequency(rps); - else if (attr == &dev_attr_gt_RP1_freq_mhz) - val = intel_rps_get_rp1_frequency(rps); - else if (attr == &dev_attr_gt_RPn_freq_mhz) - val = intel_rps_get_rpn_frequency(rps); - else - BUG(); - - return sysfs_emit(buf, "%d\n", val); -} - -static const struct attribute * const gen6_attrs[] = { - &dev_attr_gt_act_freq_mhz.attr, - &dev_attr_gt_cur_freq_mhz.attr, - &dev_attr_gt_boost_freq_mhz.attr, - &dev_attr_gt_max_freq_mhz.attr, - &dev_attr_gt_min_freq_mhz.attr, - &dev_attr_gt_RP0_freq_mhz.attr, - &dev_attr_gt_RP1_freq_mhz.attr, - &dev_attr_gt_RPn_freq_mhz.attr, - NULL, -}; - -static const struct attribute * const vlv_attrs[] = { - &dev_attr_gt_act_freq_mhz.attr, - &dev_attr_gt_cur_freq_mhz.attr, - &dev_attr_gt_boost_freq_mhz.attr, - &dev_attr_gt_max_freq_mhz.attr, - &dev_attr_gt_min_freq_mhz.attr, - &dev_attr_gt_RP0_freq_mhz.attr, - &dev_attr_gt_RP1_freq_mhz.attr, - &dev_attr_gt_RPn_freq_mhz.attr, - &dev_attr_vlv_rpe_freq_mhz.attr, - NULL, -}; - #if IS_ENABLED(CONFIG_DRM_I915_CAPTURE_ERROR)
static ssize_t error_state_read(struct file *filp, struct kobject *kobj, @@ -411,14 +246,6 @@ void i915_setup_sysfs(struct drm_i915_private *dev_priv) } }
- ret = 0; - if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv)) - ret = sysfs_create_files(&kdev->kobj, vlv_attrs); - else if (GRAPHICS_VER(dev_priv) >= 6) - ret = sysfs_create_files(&kdev->kobj, gen6_attrs); - if (ret) - drm_err(&dev_priv->drm, "RPS sysfs setup failed\n"); - dev_priv->sysfs_gt = i915_setup_gt_sysfs(&kdev->kobj); if (!dev_priv->sysfs_gt) drm_warn(&dev_priv->drm, @@ -435,10 +262,6 @@ void i915_teardown_sysfs(struct drm_i915_private *dev_priv)
i915_teardown_error_capture(kdev);
- if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv)) - sysfs_remove_files(&kdev->kobj, vlv_attrs); - else - sysfs_remove_files(&kdev->kobj, gen6_attrs); device_remove_bin_file(kdev, &dpf_attrs_1); device_remove_bin_file(kdev, &dpf_attrs); }
Hi Andi,
I love your patch! Yet something to improve:
[auto build test ERROR on drm-tip/drm-tip] [cannot apply to drm-intel/for-linux-next drm-exynos/exynos-drm-next drm/drm-next tegra-drm/drm/tegra/for-next airlied/drm-next v5.17-rc4 next-20220217] [If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use '--base' as documented in https://git-scm.com/docs/git-format-patch]
url: https://github.com/0day-ci/linux/commits/Andi-Shyti/Introduce-multitile-supp... base: git://anongit.freedesktop.org/drm/drm-tip drm-tip config: x86_64-randconfig-a011 (https://download.01.org/0day-ci/archive/20220218/202202180224.l042viYj-lkp@i...) compiler: gcc-9 (Debian 9.3.0-22) 9.3.0 reproduce (this is a W=1 build): # https://github.com/0day-ci/linux/commit/f1802e7224006bf4801fe56193bf5eb223a3... git remote add linux-review https://github.com/0day-ci/linux git fetch --no-tags linux-review Andi-Shyti/Introduce-multitile-support/20220217-224547 git checkout f1802e7224006bf4801fe56193bf5eb223a3f4d0 # save the config file to linux build tree mkdir build_dir make W=1 O=build_dir ARCH=x86_64 SHELL=/bin/bash
If you fix the issue, kindly add following tag as appropriate Reported-by: kernel test robot lkp@intel.com
All errors (new ones prefixed by >>):
drivers/gpu/drm/i915/gt/intel_gt_sysfs_pm.c: In function 'act_freq_mhz_show':
drivers/gpu/drm/i915/gt/intel_gt_sysfs_pm.c:250:20: error: implicit declaration of function 'sysfs_gt_attribute_r_func' [-Werror=implicit-function-declaration]
250 | s64 actual_freq = sysfs_gt_attribute_r_func(dev, attr, | ^~~~~~~~~~~~~~~~~~~~~~~~~ drivers/gpu/drm/i915/gt/intel_gt_sysfs_pm.c: In function 'boost_freq_mhz_store':
drivers/gpu/drm/i915/gt/intel_gt_sysfs_pm.c:318:9: error: implicit declaration of function 'sysfs_gt_attribute_w_func' [-Werror=implicit-function-declaration]
318 | return sysfs_gt_attribute_w_func(dev, attr, | ^~~~~~~~~~~~~~~~~~~~~~~~~ cc1: some warnings being treated as errors
vim +/sysfs_gt_attribute_r_func +250 drivers/gpu/drm/i915/gt/intel_gt_sysfs_pm.c
246 247 static ssize_t act_freq_mhz_show(struct device *dev, 248 struct device_attribute *attr, char *buff) 249 {
250 s64 actual_freq = sysfs_gt_attribute_r_func(dev, attr,
251 __act_freq_mhz_show); 252 253 return sysfs_emit(buff, "%u\n", (u32) actual_freq); 254 } 255 256 static s64 __cur_freq_mhz_show(struct intel_gt *gt) 257 { 258 return intel_rps_get_requested_frequency(>->rps); 259 } 260 261 static ssize_t cur_freq_mhz_show(struct device *dev, 262 struct device_attribute *attr, char *buff) 263 { 264 s64 cur_freq = sysfs_gt_attribute_r_func(dev, attr, 265 __cur_freq_mhz_show); 266 267 return sysfs_emit(buff, "%u\n", (u32) cur_freq); 268 } 269 270 static s64 __boost_freq_mhz_show(struct intel_gt *gt) 271 { 272 return intel_rps_get_boost_frequency(>->rps); 273 } 274 275 static ssize_t boost_freq_mhz_show(struct device *dev, 276 struct device_attribute *attr, 277 char *buff) 278 { 279 s64 boost_freq = sysfs_gt_attribute_r_func(dev, attr, 280 __boost_freq_mhz_show); 281 282 return sysfs_emit(buff, "%u\n", (u32) boost_freq); 283 } 284 285 static int __boost_freq_mhz_store(struct intel_gt *gt, u32 val) 286 { 287 struct intel_rps *rps = >->rps; 288 bool boost = false; 289 290 /* Validate against (static) hardware limits */ 291 val = intel_freq_opcode(rps, val); 292 if (val < rps->min_freq || val > rps->max_freq) 293 return -EINVAL; 294 295 mutex_lock(&rps->lock); 296 if (val != rps->boost_freq) { 297 rps->boost_freq = val; 298 boost = atomic_read(&rps->num_waiters); 299 } 300 mutex_unlock(&rps->lock); 301 if (boost) 302 schedule_work(&rps->work); 303 304 return 0; 305 } 306 307 static ssize_t boost_freq_mhz_store(struct device *dev, 308 struct device_attribute *attr, 309 const char *buff, size_t count) 310 { 311 ssize_t ret; 312 u32 val; 313 314 ret = kstrtou32(buff, 0, &val); 315 if (ret) 316 return ret; 317
318 return sysfs_gt_attribute_w_func(dev, attr,
319 __boost_freq_mhz_store, val) ?: count; 320 } 321
--- 0-DAY CI Kernel Test Service, Intel Corporation https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
On 17.02.2022 15:41, Andi Shyti wrote:
Now tiles have their own sysfs interfaces under the gt/ directory. Because RPS is a property that can be configured on a tile basis, then each tile should have its own interface
The new sysfs structure will have a similar layout for the 4 tile case:
/sys/.../card0 ├── gt │ ├── gt0 │ │ ├── id │ │ ├── rc6_enable │ │ ├── rc6_residency_ms │ │ ├── rps_act_freq_mhz │ │ ├── rps_boost_freq_mhz │ │ ├── rps_cur_freq_mhz │ │ ├── rps_max_freq_mhz │ │ ├── rps_min_freq_mhz │ │ ├── rps_RP0_freq_mhz │ │ ├── rps_RP1_freq_mhz │ │ └── rps_RPn_freq_mhz . . . . . . │ └── gtN │ ├── id │ ├── rc6_enable │ ├── rc6_residency_ms │ ├── rps_act_freq_mhz │ ├── rps_boost_freq_mhz │ ├── rps_cur_freq_mhz │ ├── rps_max_freq_mhz │ ├── rps_min_freq_mhz │ ├── rps_RP0_freq_mhz │ ├── rps_RP1_freq_mhz │ └── rps_RPn_freq_mhz ├── gt_act_freq_mhz -+ ├── gt_boost_freq_mhz | ├── gt_cur_freq_mhz | Original interface ├── gt_max_freq_mhz +─-> kept as existing ABI; ├── gt_min_freq_mhz | it points to gt0/ ├── gt_RP0_freq_mhz | ├── gt_RP1_freq_mhz | └── gt_RPn_freq_mhz -+
The existing interfaces have been kept in their original location to preserve the existing ABI. They act on all the GTs: when writing they loop through all the GTs and write the information on each interface. When reading they provide the average value from all the GTs.
This patch is not really adding exposing new interfaces (new ABI) other than adapting the existing one to more tiles. In any case this new set of interfaces will be a basic tool for system managers and administrators when using i915.
Signed-off-by: Andi Shyti andi.shyti@linux.intel.com Signed-off-by: Lucas De Marchi lucas.demarchi@intel.com Cc: Chris Wilson chris@chris-wilson.co.uk Cc: Joonas Lahtinen joonas.lahtinen@linux.intel.com Cc: Matt Roper matthew.d.roper@intel.com Cc: Sujaritha Sundaresan sujaritha.sundaresan@intel.com Cc: Tvrtko Ursulin tvrtko.ursulin@intel.com
drivers/gpu/drm/i915/gt/intel_gt_sysfs_pm.c | 276 ++++++++++++++++++++ drivers/gpu/drm/i915/i915_sysfs.c | 177 ------------- 2 files changed, 276 insertions(+), 177 deletions(-)
diff --git a/drivers/gpu/drm/i915/gt/intel_gt_sysfs_pm.c b/drivers/gpu/drm/i915/gt/intel_gt_sysfs_pm.c index 27d93a36894a..8e86b8f675f1 100644 --- a/drivers/gpu/drm/i915/gt/intel_gt_sysfs_pm.c +++ b/drivers/gpu/drm/i915/gt/intel_gt_sysfs_pm.c @@ -14,8 +14,33 @@ #include "intel_gt_sysfs.h" #include "intel_gt_sysfs_pm.h" #include "intel_rc6.h" +#include "intel_rps.h"
#ifdef CONFIG_PM +static int +sysfs_gt_attribute_w_func(struct device *dev, struct device_attribute *attr,
int (func)(struct intel_gt *gt, u32 val), u32 val)+{
- struct intel_gt *gt;
- int ret;
- if (!is_object_gt(&dev->kobj)) {
int i;struct drm_i915_private *i915 = kdev_minor_to_i915(dev);for_each_gt(gt, i915, i) {ret = func(gt, val);if (ret)break;}- } else {
gt = intel_gt_sysfs_get_drvdata(dev, attr->attr.name);ret = func(gt, val);- }
- return ret;
+}
- static s64 sysfs_gt_attribute_r_func(struct device *dev, struct device_attribute *attr, s64 (func)(struct intel_gt *gt))
@@ -214,7 +239,258 @@ static void intel_sysfs_rc6_init(struct intel_gt *gt, struct kobject *kobj) } #endif /* CONFIG_PM */
+static s64 __act_freq_mhz_show(struct intel_gt *gt) +{
- return intel_rps_read_actual_frequency(>->rps);
+}
+static ssize_t act_freq_mhz_show(struct device *dev,
struct device_attribute *attr, char *buff)+{
- s64 actual_freq = sysfs_gt_attribute_r_func(dev, attr,
__act_freq_mhz_show);- return sysfs_emit(buff, "%u\n", (u32) actual_freq);
Again, variable can be just u32.
+}
+static s64 __cur_freq_mhz_show(struct intel_gt *gt) +{
- return intel_rps_get_requested_frequency(>->rps);
+}
+static ssize_t cur_freq_mhz_show(struct device *dev,
struct device_attribute *attr, char *buff)+{
- s64 cur_freq = sysfs_gt_attribute_r_func(dev, attr,
__cur_freq_mhz_show);- return sysfs_emit(buff, "%u\n", (u32) cur_freq);
+}
+static s64 __boost_freq_mhz_show(struct intel_gt *gt) +{
- return intel_rps_get_boost_frequency(>->rps);
+}
+static ssize_t boost_freq_mhz_show(struct device *dev,
struct device_attribute *attr,char *buff)+{
- s64 boost_freq = sysfs_gt_attribute_r_func(dev, attr,
__boost_freq_mhz_show);- return sysfs_emit(buff, "%u\n", (u32) boost_freq);
+}
+static int __boost_freq_mhz_store(struct intel_gt *gt, u32 val) +{
- struct intel_rps *rps = >->rps;
- bool boost = false;
- /* Validate against (static) hardware limits */
- val = intel_freq_opcode(rps, val);
- if (val < rps->min_freq || val > rps->max_freq)
return -EINVAL;- mutex_lock(&rps->lock);
- if (val != rps->boost_freq) {
rps->boost_freq = val;boost = atomic_read(&rps->num_waiters);- }
- mutex_unlock(&rps->lock);
- if (boost)
schedule_work(&rps->work);- return 0;
Why not intel_rps_set_boost_frequency? Why copy/paste from rps_set_boost_freq?
+}
+static ssize_t boost_freq_mhz_store(struct device *dev,
struct device_attribute *attr,const char *buff, size_t count)+{
- ssize_t ret;
- u32 val;
- ret = kstrtou32(buff, 0, &val);
- if (ret)
return ret;- return sysfs_gt_attribute_w_func(dev, attr,
__boost_freq_mhz_store, val) ?: count;+}
+static s64 __vlv_rpe_freq_mhz_show(struct intel_gt *gt) +{
- struct intel_rps *rps = >->rps;
- return intel_gpu_freq(rps, rps->efficient_freq);
+}
+static ssize_t vlv_rpe_freq_mhz_show(struct device *dev,
struct device_attribute *attr, char *buff)+{
- s64 rpe_freq = sysfs_gt_attribute_r_func(dev, attr,
__vlv_rpe_freq_mhz_show);- return sysfs_emit(buff, "%d\n", (int) rpe_freq);
+}
+static s64 __max_freq_mhz_show(struct intel_gt *gt) +{
- return intel_rps_get_max_frequency(>->rps);
+}
+static int __set_max_freq(struct intel_gt *gt, u32 val) +{
- return intel_rps_set_max_frequency(>->rps, val);
+}
+static s64 __min_freq_mhz_show(struct intel_gt *gt) +{
- return intel_rps_get_min_frequency(>->rps);
+}
+static int __set_min_freq(struct intel_gt *gt, u32 val) +{
- return intel_rps_set_min_frequency(>->rps, val);
+}
+static ssize_t min_max_freq_mhz_store(struct device *dev,
struct device_attribute *attr,const char *buff, size_t count);+static ssize_t min_max_freq_mhz_show(struct device *dev,
struct device_attribute *attr, char *buff);+#define INTEL_GT_RPS_SYSFS_ATTR(_name, _mode, _show, _store) \
- struct device_attribute dev_attr_gt_##_name = __ATTR(gt_##_name, _mode, _show, _store); \
- struct device_attribute dev_attr_rps_##_name = __ATTR(rps_##_name, _mode, _show, _store)
+#define INTEL_GT_RPS_SYSFS_ATTR_RO(_name) \
INTEL_GT_RPS_SYSFS_ATTR(_name, 0444, _name##_show, NULL)+#define INTEL_GT_RPS_SYSFS_ATTR_RW(_name) \
INTEL_GT_RPS_SYSFS_ATTR(_name, 0644, _name##_show, _name##_store)+static INTEL_GT_RPS_SYSFS_ATTR_RO(act_freq_mhz); +static INTEL_GT_RPS_SYSFS_ATTR_RO(cur_freq_mhz); +static INTEL_GT_RPS_SYSFS_ATTR_RW(boost_freq_mhz);
+static DEVICE_ATTR_RO(vlv_rpe_freq_mhz);
+static ssize_t rps_rp_mhz_show(struct device *dev,
struct device_attribute *attr,char *buff);+static INTEL_GT_RPS_SYSFS_ATTR(RP0_freq_mhz, 0444, rps_rp_mhz_show, NULL); +static INTEL_GT_RPS_SYSFS_ATTR(RP1_freq_mhz, 0444, rps_rp_mhz_show, NULL); +static INTEL_GT_RPS_SYSFS_ATTR(RPn_freq_mhz, 0444, rps_rp_mhz_show, NULL);
+INTEL_GT_RPS_SYSFS_ATTR(max_freq_mhz, 0644, min_max_freq_mhz_show, min_max_freq_mhz_store); +INTEL_GT_RPS_SYSFS_ATTR(min_freq_mhz, 0644, min_max_freq_mhz_show, min_max_freq_mhz_store);
+#define GEN6_ATTR(s) { \
&dev_attr_##s##_act_freq_mhz.attr, \&dev_attr_##s##_cur_freq_mhz.attr, \&dev_attr_##s##_boost_freq_mhz.attr, \&dev_attr_##s##_max_freq_mhz.attr, \&dev_attr_##s##_min_freq_mhz.attr, \&dev_attr_##s##_RP0_freq_mhz.attr, \&dev_attr_##s##_RP1_freq_mhz.attr, \&dev_attr_##s##_RPn_freq_mhz.attr, \NULL, \- }
+#define GEN6_RPS_ATTR GEN6_ATTR(rps) +#define GEN6_GT_ATTR GEN6_ATTR(gt)
+static ssize_t min_max_freq_mhz_store(struct device *dev,
struct device_attribute *attr,const char *buff, size_t count)+{
- long ret;
- u32 val;
- ret = kstrtou32(buff, 0, &val);
- if (ret)
return ret;- ret = (attr == &dev_attr_gt_min_freq_mhz ||
attr == &dev_attr_rps_min_freq_mhz) ?sysfs_gt_attribute_w_func(dev, attr, __set_min_freq, val) :sysfs_gt_attribute_w_func(dev, attr, __set_max_freq, val);- return ret ?: count;
+}
+static ssize_t min_max_freq_mhz_show(struct device *dev,
struct device_attribute *attr, char *buff)+{
- s64 val;
- val = (attr == &dev_attr_gt_min_freq_mhz ||
attr == &dev_attr_rps_min_freq_mhz) ?sysfs_gt_attribute_r_func(dev, attr, __min_freq_mhz_show) :sysfs_gt_attribute_r_func(dev, attr, __max_freq_mhz_show);- return sysfs_emit(buff, "%u\n", (u32) val);
+}
+/* For now we have a static number of RP states */ +static ssize_t rps_rp_mhz_show(struct device *dev,
struct device_attribute *attr,char *buff)+{
- struct intel_gt *gt = intel_gt_sysfs_get_drvdata(dev, attr->attr.name);
- struct intel_rps *rps = >->rps;
- u32 val;
- if (attr == &dev_attr_gt_RP0_freq_mhz ||
attr == &dev_attr_rps_RP0_freq_mhz) {val = intel_rps_get_rp0_frequency(rps);- } else if (attr == &dev_attr_gt_RP1_freq_mhz ||
attr == &dev_attr_rps_RP1_freq_mhz) {val = intel_rps_get_rp1_frequency(rps);- } else if (attr == &dev_attr_gt_RPn_freq_mhz ||
attr == &dev_attr_rps_RPn_freq_mhz) {val = intel_rps_get_rpn_frequency(rps);- } else {
GEM_WARN_ON(1);return -ENODEV;
I guess this is not necessary, otherwise similar path should be in other helpers.
Regards Andrzej
- }
- return sysfs_emit(buff, "%d\n", val);
+}
+static const struct attribute * const gen6_rps_attrs[] = GEN6_RPS_ATTR; +static const struct attribute * const gen6_gt_attrs[] = GEN6_GT_ATTR;
+static int intel_sysfs_rps_init(struct intel_gt *gt, struct kobject *kobj,
const struct attribute * const *attrs)+{
- int ret;
- if (GRAPHICS_VER(gt->i915) < 6)
return 0;- ret = sysfs_create_files(kobj, attrs);
- if (ret)
return ret;- if (IS_VALLEYVIEW(gt->i915) || IS_CHERRYVIEW(gt->i915))
ret = sysfs_create_file(kobj, &dev_attr_vlv_rpe_freq_mhz.attr);- return ret;
+}
- void intel_gt_sysfs_pm_init(struct intel_gt *gt, struct kobject *kobj) {
- int ret;
- intel_sysfs_rc6_init(gt, kobj);
- ret = is_object_gt(kobj) ?
intel_sysfs_rps_init(gt, kobj, gen6_rps_attrs) :intel_sysfs_rps_init(gt, kobj, gen6_gt_attrs);- if (ret)
drm_warn(>->i915->drm,"failed to create gt%u RPS sysfs files", gt->info.id);diff --git a/drivers/gpu/drm/i915/i915_sysfs.c b/drivers/gpu/drm/i915/i915_sysfs.c index b08a959e5ccc..0a0057940718 100644 --- a/drivers/gpu/drm/i915/i915_sysfs.c +++ b/drivers/gpu/drm/i915/i915_sysfs.c @@ -156,171 +156,6 @@ static const struct bin_attribute dpf_attrs_1 = { .private = (void *)1 };
-static ssize_t gt_act_freq_mhz_show(struct device *kdev,
struct device_attribute *attr, char *buf)-{
- struct drm_i915_private *i915 = kdev_minor_to_i915(kdev);
- struct intel_rps *rps = &to_gt(i915)->rps;
- return sysfs_emit(buf, "%d\n", intel_rps_read_actual_frequency(rps));
-}
-static ssize_t gt_cur_freq_mhz_show(struct device *kdev,
struct device_attribute *attr, char *buf)-{
- struct drm_i915_private *i915 = kdev_minor_to_i915(kdev);
- struct intel_rps *rps = &to_gt(i915)->rps;
- return sysfs_emit(buf, "%d\n", intel_rps_get_requested_frequency(rps));
-}
-static ssize_t gt_boost_freq_mhz_show(struct device *kdev, struct device_attribute *attr, char *buf) -{
- struct drm_i915_private *i915 = kdev_minor_to_i915(kdev);
- struct intel_rps *rps = &to_gt(i915)->rps;
- return sysfs_emit(buf, "%d\n", intel_rps_get_boost_frequency(rps));
-}
-static ssize_t gt_boost_freq_mhz_store(struct device *kdev,
struct device_attribute *attr,const char *buf, size_t count)-{
- struct drm_i915_private *dev_priv = kdev_minor_to_i915(kdev);
- struct intel_rps *rps = &to_gt(dev_priv)->rps;
- ssize_t ret;
- u32 val;
- ret = kstrtou32(buf, 0, &val);
- if (ret)
return ret;- ret = intel_rps_set_boost_frequency(rps, val);
- return ret ?: count;
-}
-static ssize_t vlv_rpe_freq_mhz_show(struct device *kdev,
struct device_attribute *attr, char *buf)-{
- struct drm_i915_private *dev_priv = kdev_minor_to_i915(kdev);
- struct intel_rps *rps = &to_gt(dev_priv)->rps;
- return sysfs_emit(buf, "%d\n", intel_gpu_freq(rps, rps->efficient_freq));
-}
-static ssize_t gt_max_freq_mhz_show(struct device *kdev, struct device_attribute *attr, char *buf) -{
- struct drm_i915_private *dev_priv = kdev_minor_to_i915(kdev);
- struct intel_gt *gt = to_gt(dev_priv);
- struct intel_rps *rps = >->rps;
- return sysfs_emit(buf, "%d\n", intel_rps_get_max_frequency(rps));
-}
-static ssize_t gt_max_freq_mhz_store(struct device *kdev,
struct device_attribute *attr,const char *buf, size_t count)-{
- struct drm_i915_private *dev_priv = kdev_minor_to_i915(kdev);
- struct intel_gt *gt = to_gt(dev_priv);
- struct intel_rps *rps = >->rps;
- ssize_t ret;
- u32 val;
- ret = kstrtou32(buf, 0, &val);
- if (ret)
return ret;- ret = intel_rps_set_max_frequency(rps, val);
- return ret ?: count;
-}
-static ssize_t gt_min_freq_mhz_show(struct device *kdev, struct device_attribute *attr, char *buf) -{
- struct drm_i915_private *i915 = kdev_minor_to_i915(kdev);
- struct intel_gt *gt = to_gt(i915);
- struct intel_rps *rps = >->rps;
- return sysfs_emit(buf, "%d\n", intel_rps_get_min_frequency(rps));
-}
-static ssize_t gt_min_freq_mhz_store(struct device *kdev,
struct device_attribute *attr,const char *buf, size_t count)-{
- struct drm_i915_private *i915 = kdev_minor_to_i915(kdev);
- struct intel_rps *rps = &to_gt(i915)->rps;
- ssize_t ret;
- u32 val;
- ret = kstrtou32(buf, 0, &val);
- if (ret)
return ret;- ret = intel_rps_set_min_frequency(rps, val);
- return ret ?: count;
-}
-static DEVICE_ATTR_RO(gt_act_freq_mhz); -static DEVICE_ATTR_RO(gt_cur_freq_mhz); -static DEVICE_ATTR_RW(gt_boost_freq_mhz); -static DEVICE_ATTR_RW(gt_max_freq_mhz); -static DEVICE_ATTR_RW(gt_min_freq_mhz);
-static DEVICE_ATTR_RO(vlv_rpe_freq_mhz);
-static ssize_t gt_rp_mhz_show(struct device *kdev, struct device_attribute *attr, char *buf); -static DEVICE_ATTR(gt_RP0_freq_mhz, S_IRUGO, gt_rp_mhz_show, NULL); -static DEVICE_ATTR(gt_RP1_freq_mhz, S_IRUGO, gt_rp_mhz_show, NULL); -static DEVICE_ATTR(gt_RPn_freq_mhz, S_IRUGO, gt_rp_mhz_show, NULL);
-/* For now we have a static number of RP states */ -static ssize_t gt_rp_mhz_show(struct device *kdev, struct device_attribute *attr, char *buf) -{
- struct drm_i915_private *dev_priv = kdev_minor_to_i915(kdev);
- struct intel_rps *rps = &to_gt(dev_priv)->rps;
- u32 val;
- if (attr == &dev_attr_gt_RP0_freq_mhz)
val = intel_rps_get_rp0_frequency(rps);- else if (attr == &dev_attr_gt_RP1_freq_mhz)
val = intel_rps_get_rp1_frequency(rps);- else if (attr == &dev_attr_gt_RPn_freq_mhz)
val = intel_rps_get_rpn_frequency(rps);- else
BUG();- return sysfs_emit(buf, "%d\n", val);
-}
-static const struct attribute * const gen6_attrs[] = {
- &dev_attr_gt_act_freq_mhz.attr,
- &dev_attr_gt_cur_freq_mhz.attr,
- &dev_attr_gt_boost_freq_mhz.attr,
- &dev_attr_gt_max_freq_mhz.attr,
- &dev_attr_gt_min_freq_mhz.attr,
- &dev_attr_gt_RP0_freq_mhz.attr,
- &dev_attr_gt_RP1_freq_mhz.attr,
- &dev_attr_gt_RPn_freq_mhz.attr,
- NULL,
-};
-static const struct attribute * const vlv_attrs[] = {
- &dev_attr_gt_act_freq_mhz.attr,
- &dev_attr_gt_cur_freq_mhz.attr,
- &dev_attr_gt_boost_freq_mhz.attr,
- &dev_attr_gt_max_freq_mhz.attr,
- &dev_attr_gt_min_freq_mhz.attr,
- &dev_attr_gt_RP0_freq_mhz.attr,
- &dev_attr_gt_RP1_freq_mhz.attr,
- &dev_attr_gt_RPn_freq_mhz.attr,
- &dev_attr_vlv_rpe_freq_mhz.attr,
- NULL,
-};
#if IS_ENABLED(CONFIG_DRM_I915_CAPTURE_ERROR)
static ssize_t error_state_read(struct file *filp, struct kobject *kobj,
@@ -411,14 +246,6 @@ void i915_setup_sysfs(struct drm_i915_private *dev_priv) } }
- ret = 0;
- if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv))
ret = sysfs_create_files(&kdev->kobj, vlv_attrs);- else if (GRAPHICS_VER(dev_priv) >= 6)
ret = sysfs_create_files(&kdev->kobj, gen6_attrs);- if (ret)
drm_err(&dev_priv->drm, "RPS sysfs setup failed\n");- dev_priv->sysfs_gt = i915_setup_gt_sysfs(&kdev->kobj); if (!dev_priv->sysfs_gt) drm_warn(&dev_priv->drm,
@@ -435,10 +262,6 @@ void i915_teardown_sysfs(struct drm_i915_private *dev_priv)
i915_teardown_error_capture(kdev);
- if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv))
sysfs_remove_files(&kdev->kobj, vlv_attrs);- else
sysfs_remove_files(&kdev->kobj, gen6_attrs);
Hi Andrzej,
[...]
+static ssize_t act_freq_mhz_show(struct device *dev,
struct device_attribute *attr, char *buff)+{
- s64 actual_freq = sysfs_gt_attribute_r_func(dev, attr,
__act_freq_mhz_show);- return sysfs_emit(buff, "%u\n", (u32) actual_freq);
Again, variable can be just u32.
yes
[...]
+static int __boost_freq_mhz_store(struct intel_gt *gt, u32 val) +{
- struct intel_rps *rps = >->rps;
- bool boost = false;
- /* Validate against (static) hardware limits */
- val = intel_freq_opcode(rps, val);
- if (val < rps->min_freq || val > rps->max_freq)
return -EINVAL;- mutex_lock(&rps->lock);
- if (val != rps->boost_freq) {
rps->boost_freq = val;boost = atomic_read(&rps->num_waiters);- }
- mutex_unlock(&rps->lock);
- if (boost)
schedule_work(&rps->work);- return 0;
Why not intel_rps_set_boost_frequency? Why copy/paste from rps_set_boost_freq?
you are right... this must be some ugly leftover. Thanks!
[...]
+/* For now we have a static number of RP states */ +static ssize_t rps_rp_mhz_show(struct device *dev,
struct device_attribute *attr,char *buff)+{
- struct intel_gt *gt = intel_gt_sysfs_get_drvdata(dev, attr->attr.name);
- struct intel_rps *rps = >->rps;
- u32 val;
- if (attr == &dev_attr_gt_RP0_freq_mhz ||
attr == &dev_attr_rps_RP0_freq_mhz) {val = intel_rps_get_rp0_frequency(rps);- } else if (attr == &dev_attr_gt_RP1_freq_mhz ||
attr == &dev_attr_rps_RP1_freq_mhz) {val = intel_rps_get_rp1_frequency(rps);- } else if (attr == &dev_attr_gt_RPn_freq_mhz ||
attr == &dev_attr_rps_RPn_freq_mhz) {val = intel_rps_get_rpn_frequency(rps);- } else {
GEM_WARN_ON(1);return -ENODEV;I guess this is not necessary, otherwise similar path should be in other helpers.
yeah... it's a bit hacky, I must agree... will split it properly.
[...]
Thanks, Andi
From: Sujaritha Sundaresan sujaritha.sundaresan@intel.com
This patch adds the following new sysfs frequency attributes; - punit_req_freq_mhz - throttle_reason_status - throttle_reason_pl1 - throttle_reason_pl2 - throttle_reason_pl4 - throttle_reason_thermal - throttle_reason_prochot - throttle_reason_ratl - throttle_reason_vr_thermalert - throttle_reason_vr_tdc
Signed-off-by: Sujaritha Sundaresan sujaritha.sundaresan@intel.com Signed-off-by: Andi Shyti andi.shyti@linux.intel.com Cc: Dale B Stimson dale.b.stimson@intel.com --- drivers/gpu/drm/i915/gt/intel_gt_sysfs_pm.c | 142 ++++++++++++++++++++ drivers/gpu/drm/i915/gt/intel_rps.c | 83 ++++++++++++ drivers/gpu/drm/i915/gt/intel_rps.h | 10 ++ drivers/gpu/drm/i915/i915_reg.h | 11 ++ 4 files changed, 246 insertions(+)
diff --git a/drivers/gpu/drm/i915/gt/intel_gt_sysfs_pm.c b/drivers/gpu/drm/i915/gt/intel_gt_sysfs_pm.c index 8e86b8f675f1..8be676cd1607 100644 --- a/drivers/gpu/drm/i915/gt/intel_gt_sysfs_pm.c +++ b/drivers/gpu/drm/i915/gt/intel_gt_sysfs_pm.c @@ -463,6 +463,141 @@ static ssize_t rps_rp_mhz_show(struct device *dev, static const struct attribute * const gen6_rps_attrs[] = GEN6_RPS_ATTR; static const struct attribute * const gen6_gt_attrs[] = GEN6_GT_ATTR;
+static ssize_t punit_req_freq_mhz_show(struct device *dev, + struct device_attribute *attr, + char *buff) +{ + struct intel_gt *gt = intel_gt_sysfs_get_drvdata(dev, attr->attr.name); + struct intel_rps *rps = >->rps; + u32 preq = intel_rps_read_punit_req_frequency(rps); + + return scnprintf(buff, PAGE_SIZE, "%d\n", preq); +} + +static ssize_t throttle_reason_status_show(struct device *dev, + struct device_attribute *attr, + char *buff) +{ + struct intel_gt *gt = intel_gt_sysfs_get_drvdata(dev, attr->attr.name); + struct intel_rps *rps = >->rps; + bool status = !!intel_rps_read_throttle_reason_status(rps); + + return scnprintf(buff, PAGE_SIZE, "%u\n", status); +} + +static ssize_t throttle_reason_pl1_show(struct device *dev, + struct device_attribute *attr, + char *buff) +{ + struct intel_gt *gt = intel_gt_sysfs_get_drvdata(dev, attr->attr.name); + struct intel_rps *rps = >->rps; + bool pl1 = !!intel_rps_read_throttle_reason_pl1(rps); + + return scnprintf(buff, PAGE_SIZE, "%u\n", pl1); +} + +static ssize_t throttle_reason_pl2_show(struct device *dev, + struct device_attribute *attr, + char *buff) +{ + struct intel_gt *gt = intel_gt_sysfs_get_drvdata(dev, attr->attr.name); + struct intel_rps *rps = >->rps; + bool pl2 = !!intel_rps_read_throttle_reason_pl2(rps); + + return scnprintf(buff, PAGE_SIZE, "%u\n", pl2); +} + +static ssize_t throttle_reason_pl4_show(struct device *dev, + struct device_attribute *attr, + char *buff) +{ + struct intel_gt *gt = intel_gt_sysfs_get_drvdata(dev, attr->attr.name); + struct intel_rps *rps = >->rps; + bool pl4 = !!intel_rps_read_throttle_reason_pl4(rps); + + return scnprintf(buff, PAGE_SIZE, "%u\n", pl4); +} + +static ssize_t throttle_reason_thermal_show(struct device *dev, + struct device_attribute *attr, + char *buff) +{ + struct intel_gt *gt = intel_gt_sysfs_get_drvdata(dev, attr->attr.name); + struct intel_rps *rps = >->rps; + bool thermal = !!intel_rps_read_throttle_reason_thermal(rps); + + return scnprintf(buff, PAGE_SIZE, "%u\n", thermal); +} + +static ssize_t throttle_reason_prochot_show(struct device *dev, + struct device_attribute *attr, + char *buff) +{ + struct intel_gt *gt = intel_gt_sysfs_get_drvdata(dev, attr->attr.name); + struct intel_rps *rps = >->rps; + bool prochot = !!intel_rps_read_throttle_reason_prochot(rps); + + return scnprintf(buff, PAGE_SIZE, "%u\n", prochot); +} + +static ssize_t throttle_reason_ratl_show(struct device *dev, + struct device_attribute *attr, + char *buff) +{ + struct intel_gt *gt = intel_gt_sysfs_get_drvdata(dev, attr->attr.name); + struct intel_rps *rps = >->rps; + bool ratl = !!intel_rps_read_throttle_reason_ratl(rps); + + return scnprintf(buff, PAGE_SIZE, "%u\n", ratl); +} + +static ssize_t throttle_reason_vr_thermalert_show(struct device *dev, + struct device_attribute *attr, + char *buff) +{ + struct intel_gt *gt = intel_gt_sysfs_get_drvdata(dev, attr->attr.name); + struct intel_rps *rps = >->rps; + bool thermalert = !!intel_rps_read_throttle_reason_vr_thermalert(rps); + + return scnprintf(buff, PAGE_SIZE, "%u\n", thermalert); +} + +static ssize_t throttle_reason_vr_tdc_show(struct device *dev, + struct device_attribute *attr, + char *buff) +{ + struct intel_gt *gt = intel_gt_sysfs_get_drvdata(dev, attr->attr.name); + struct intel_rps *rps = >->rps; + bool tdc = !!intel_rps_read_throttle_reason_vr_tdc(rps); + + return scnprintf(buff, PAGE_SIZE, "%u\n", tdc); +} + +static DEVICE_ATTR_RO(punit_req_freq_mhz); +static DEVICE_ATTR_RO(throttle_reason_status); +static DEVICE_ATTR_RO(throttle_reason_pl1); +static DEVICE_ATTR_RO(throttle_reason_pl2); +static DEVICE_ATTR_RO(throttle_reason_pl4); +static DEVICE_ATTR_RO(throttle_reason_thermal); +static DEVICE_ATTR_RO(throttle_reason_prochot); +static DEVICE_ATTR_RO(throttle_reason_ratl); +static DEVICE_ATTR_RO(throttle_reason_vr_thermalert); +static DEVICE_ATTR_RO(throttle_reason_vr_tdc); + +static const struct attribute *freq_attrs[] = { + &dev_attr_punit_req_freq_mhz.attr, + &dev_attr_throttle_reason_status.attr, + &dev_attr_throttle_reason_pl1.attr, + &dev_attr_throttle_reason_pl2.attr, + &dev_attr_throttle_reason_pl4.attr, + &dev_attr_throttle_reason_thermal.attr, + &dev_attr_throttle_reason_prochot.attr, + &dev_attr_throttle_reason_ratl.attr, + &dev_attr_throttle_reason_vr_thermalert.attr, + &dev_attr_throttle_reason_vr_tdc.attr, + NULL +}; + static int intel_sysfs_rps_init(struct intel_gt *gt, struct kobject *kobj, const struct attribute * const *attrs) { @@ -493,4 +628,11 @@ void intel_gt_sysfs_pm_init(struct intel_gt *gt, struct kobject *kobj) if (ret) drm_warn(>->i915->drm, "failed to create gt%u RPS sysfs files", gt->info.id); + + ret = sysfs_create_files(kobj, freq_attrs); + if (ret) + drm_warn(>->i915->drm, + "failed to create gt%u throttle sysfs files", + gt->info.id); + } diff --git a/drivers/gpu/drm/i915/gt/intel_rps.c b/drivers/gpu/drm/i915/gt/intel_rps.c index fd95449ed46d..94c78cfaf9c9 100644 --- a/drivers/gpu/drm/i915/gt/intel_rps.c +++ b/drivers/gpu/drm/i915/gt/intel_rps.c @@ -2286,6 +2286,89 @@ void intel_rps_lower_unslice(struct intel_rps *rps) mutex_unlock(&rps->lock); }
+static u32 __rps_read_mmio(struct intel_gt *gt, i915_reg_t reg32) +{ + intel_wakeref_t wakeref; + u32 val; + + with_intel_runtime_pm(gt->uncore->rpm, wakeref) + val = intel_uncore_read(gt->uncore, reg32); + + return val; +} + +u32 intel_rps_read_throttle_reason_status(struct intel_rps *rps) +{ + struct intel_gt *gt = rps_to_gt(rps); + u32 status = __rps_read_mmio(gt, GT0_PERF_LIMIT_REASONS) & GT0_PERF_LIMIT_REASONS_MASK; + + return status; +} + +u32 intel_rps_read_throttle_reason_pl1(struct intel_rps *rps) +{ + struct intel_gt *gt = rps_to_gt(rps); + u32 pl1 = __rps_read_mmio(gt, GT0_PERF_LIMIT_REASONS) & POWER_LIMIT_1_MASK; + + return pl1; +} + +u32 intel_rps_read_throttle_reason_pl2(struct intel_rps *rps) +{ + struct intel_gt *gt = rps_to_gt(rps); + u32 pl2 = __rps_read_mmio(gt, GT0_PERF_LIMIT_REASONS) & POWER_LIMIT_2_MASK; + + return pl2; +} + +u32 intel_rps_read_throttle_reason_pl4(struct intel_rps *rps) +{ + struct intel_gt *gt = rps_to_gt(rps); + u32 pl4 = __rps_read_mmio(gt, GT0_PERF_LIMIT_REASONS) & POWER_LIMIT_4_MASK; + + return pl4; +} + +u32 intel_rps_read_throttle_reason_thermal(struct intel_rps *rps) +{ + struct intel_gt *gt = rps_to_gt(rps); + u32 thermal = __rps_read_mmio(gt, GT0_PERF_LIMIT_REASONS) & THERMAL_LIMIT_MASK; + + return thermal; +} + +u32 intel_rps_read_throttle_reason_prochot(struct intel_rps *rps) +{ + struct intel_gt *gt = rps_to_gt(rps); + u32 prochot = __rps_read_mmio(gt, GT0_PERF_LIMIT_REASONS) & PROCHOT_MASK; + + return prochot; +} + +u32 intel_rps_read_throttle_reason_ratl(struct intel_rps *rps) +{ + struct intel_gt *gt = rps_to_gt(rps); + u32 ratl = __rps_read_mmio(gt, GT0_PERF_LIMIT_REASONS) & RATL_MASK; + + return ratl; +} + +u32 intel_rps_read_throttle_reason_vr_thermalert(struct intel_rps *rps) +{ + struct intel_gt *gt = rps_to_gt(rps); + u32 thermalert = __rps_read_mmio(gt, GT0_PERF_LIMIT_REASONS) & VR_THERMALERT_MASK; + + return thermalert; +} + +u32 intel_rps_read_throttle_reason_vr_tdc(struct intel_rps *rps) +{ + struct intel_gt *gt = rps_to_gt(rps); + u32 tdc = __rps_read_mmio(gt, GT0_PERF_LIMIT_REASONS) & VR_TDC_MASK; + + return tdc; +} + /* External interface for intel_ips.ko */
static struct drm_i915_private __rcu *ips_mchdev; diff --git a/drivers/gpu/drm/i915/gt/intel_rps.h b/drivers/gpu/drm/i915/gt/intel_rps.h index c6d76a3d1331..b45ab983895c 100644 --- a/drivers/gpu/drm/i915/gt/intel_rps.h +++ b/drivers/gpu/drm/i915/gt/intel_rps.h @@ -47,6 +47,16 @@ u32 intel_rps_read_punit_req_frequency(struct intel_rps *rps); u32 intel_rps_read_state_cap(struct intel_rps *rps); void intel_rps_raise_unslice(struct intel_rps *rps); void intel_rps_lower_unslice(struct intel_rps *rps); +u32 intel_rps_read_throttle_reason(struct intel_rps *rps); +u32 intel_rps_read_throttle_reason_status(struct intel_rps *rps); +u32 intel_rps_read_throttle_reason_pl1(struct intel_rps *rps); +u32 intel_rps_read_throttle_reason_pl2(struct intel_rps *rps); +u32 intel_rps_read_throttle_reason_pl4(struct intel_rps *rps); +u32 intel_rps_read_throttle_reason_thermal(struct intel_rps *rps); +u32 intel_rps_read_throttle_reason_prochot(struct intel_rps *rps); +u32 intel_rps_read_throttle_reason_ratl(struct intel_rps *rps); +u32 intel_rps_read_throttle_reason_vr_thermalert(struct intel_rps *rps); +u32 intel_rps_read_throttle_reason_vr_tdc(struct intel_rps *rps);
void gen5_rps_irq_handler(struct intel_rps *rps); void gen6_rps_irq_handler(struct intel_rps *rps, u32 pm_iir); diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h index 2243d9d1d941..c4f53e5404cd 100644 --- a/drivers/gpu/drm/i915/i915_reg.h +++ b/drivers/gpu/drm/i915/i915_reg.h @@ -1836,6 +1836,17 @@ #define GEN9_RP_STATE_LIMITS _MMIO(0x138148) #define XEHPSDV_RP_STATE_CAP _MMIO(0x250014)
+#define GT0_PERF_LIMIT_REASONS _MMIO(0x1381A8) +#define GT0_PERF_LIMIT_REASONS_MASK 0x00000de3 +#define PROCHOT_MASK BIT(1) +#define THERMAL_LIMIT_MASK BIT(2) +#define RATL_MASK BIT(6) +#define VR_THERMALERT_MASK BIT(7) +#define VR_TDC_MASK BIT(8) +#define POWER_LIMIT_4_MASK BIT(9) +#define POWER_LIMIT_1_MASK BIT(11) +#define POWER_LIMIT_2_MASK BIT(12) + #define CHV_CLK_CTL1 _MMIO(0x101100) #define VLV_CLK_CTL2 _MMIO(0x101104) #define CLK_CTL2_CZCOUNT_30NS_SHIFT 28
Hi,
I forgot to add some note to this patch...
[...]
+static ssize_t throttle_reason_status_show(struct device *dev,
struct device_attribute *attr,char *buff)+{
- struct intel_gt *gt = intel_gt_sysfs_get_drvdata(dev, attr->attr.name);
- struct intel_rps *rps = >->rps;
- bool status = !!intel_rps_read_throttle_reason_status(rps);
why are these boolean? Can't we send whatever we read from the register?
[...]
+#define GT0_PERF_LIMIT_REASONS _MMIO(0x1381A8) +#define GT0_PERF_LIMIT_REASONS_MASK 0x00000de3
This mask is really weird! Sujaritha, can you please explain it?
It looks something like this,
REG_GENMASK(11, 6) | REG_GENMASK(2, 0)
But I don't know if it improves any readability, in any case, the mask is not clear.
+#define PROCHOT_MASK BIT(1) +#define THERMAL_LIMIT_MASK BIT(2) +#define RATL_MASK BIT(6) +#define VR_THERMALERT_MASK BIT(7) +#define VR_TDC_MASK BIT(8) +#define POWER_LIMIT_4_MASK BIT(9) +#define POWER_LIMIT_1_MASK BIT(11) +#define POWER_LIMIT_2_MASK BIT(12)
I hope I got these right. Sujaritha, can you please check?
Andi
On 2/17/2022 7:45 AM, Andi Shyti wrote:
Hi,
I forgot to add some note to this patch...
[...]
+static ssize_t throttle_reason_status_show(struct device *dev,
struct device_attribute *attr,char *buff)+{
- struct intel_gt *gt = intel_gt_sysfs_get_drvdata(dev, attr->attr.name);
- struct intel_rps *rps = >->rps;
- bool status = !!intel_rps_read_throttle_reason_status(rps);
why are these boolean? Can't we send whatever we read from the register?
Didn't want to report out the register mask value since the sysfs is supposed to a 0/1 status.
[...]
+#define GT0_PERF_LIMIT_REASONS _MMIO(0x1381A8) +#define GT0_PERF_LIMIT_REASONS_MASK 0x00000de3
This mask is really weird! Sujaritha, can you please explain it?
It looks something like this,
REG_GENMASK(11, 6) | REG_GENMASK(2, 0)
But I don't know if it improves any readability, in any case, the mask is not clear.
This is meant to be an overall status flag as a one stop shop check for any kind of throttling.
+#define PROCHOT_MASK BIT(1) +#define THERMAL_LIMIT_MASK BIT(2) +#define RATL_MASK BIT(6) +#define VR_THERMALERT_MASK BIT(7) +#define VR_TDC_MASK BIT(8) +#define POWER_LIMIT_4_MASK BIT(9) +#define POWER_LIMIT_1_MASK BIT(11) +#define POWER_LIMIT_2_MASK BIT(12)
I hope I got these right. Sujaritha, can you please check?
Andi
Yes these are correct.
Thanks,
Suja
On 17.02.2022 15:41, Andi Shyti wrote:
From: Sujaritha Sundaresan sujaritha.sundaresan@intel.com
This patch adds the following new sysfs frequency attributes;
- punit_req_freq_mhz
- throttle_reason_status
- throttle_reason_pl1
- throttle_reason_pl2
- throttle_reason_pl4
- throttle_reason_thermal
- throttle_reason_prochot
- throttle_reason_ratl
- throttle_reason_vr_thermalert
- throttle_reason_vr_tdc
Signed-off-by: Sujaritha Sundaresan sujaritha.sundaresan@intel.com Signed-off-by: Andi Shyti andi.shyti@linux.intel.com Cc: Dale B Stimson dale.b.stimson@intel.com
drivers/gpu/drm/i915/gt/intel_gt_sysfs_pm.c | 142 ++++++++++++++++++++ drivers/gpu/drm/i915/gt/intel_rps.c | 83 ++++++++++++ drivers/gpu/drm/i915/gt/intel_rps.h | 10 ++ drivers/gpu/drm/i915/i915_reg.h | 11 ++ 4 files changed, 246 insertions(+)
diff --git a/drivers/gpu/drm/i915/gt/intel_gt_sysfs_pm.c b/drivers/gpu/drm/i915/gt/intel_gt_sysfs_pm.c index 8e86b8f675f1..8be676cd1607 100644 --- a/drivers/gpu/drm/i915/gt/intel_gt_sysfs_pm.c +++ b/drivers/gpu/drm/i915/gt/intel_gt_sysfs_pm.c @@ -463,6 +463,141 @@ static ssize_t rps_rp_mhz_show(struct device *dev, static const struct attribute * const gen6_rps_attrs[] = GEN6_RPS_ATTR; static const struct attribute * const gen6_gt_attrs[] = GEN6_GT_ATTR;
+static ssize_t punit_req_freq_mhz_show(struct device *dev,
struct device_attribute *attr,char *buff)+{
- struct intel_gt *gt = intel_gt_sysfs_get_drvdata(dev, attr->attr.name);
- struct intel_rps *rps = >->rps;
- u32 preq = intel_rps_read_punit_req_frequency(rps);
- return scnprintf(buff, PAGE_SIZE, "%d\n", preq);
%u since preq is u32
and use sysfs_emit (also in below show functions)
+}
+static ssize_t throttle_reason_status_show(struct device *dev,
struct device_attribute *attr,char *buff)+{
- struct intel_gt *gt = intel_gt_sysfs_get_drvdata(dev, attr->attr.name);
- struct intel_rps *rps = >->rps;
- bool status = !!intel_rps_read_throttle_reason_status(rps);
- return scnprintf(buff, PAGE_SIZE, "%u\n", status);
+}
+static ssize_t throttle_reason_pl1_show(struct device *dev,
struct device_attribute *attr,char *buff)+{
- struct intel_gt *gt = intel_gt_sysfs_get_drvdata(dev, attr->attr.name);
- struct intel_rps *rps = >->rps;
- bool pl1 = !!intel_rps_read_throttle_reason_pl1(rps);
- return scnprintf(buff, PAGE_SIZE, "%u\n", pl1);
+}
+static ssize_t throttle_reason_pl2_show(struct device *dev,
struct device_attribute *attr,char *buff)+{
- struct intel_gt *gt = intel_gt_sysfs_get_drvdata(dev, attr->attr.name);
- struct intel_rps *rps = >->rps;
- bool pl2 = !!intel_rps_read_throttle_reason_pl2(rps);
- return scnprintf(buff, PAGE_SIZE, "%u\n", pl2);
+}
+static ssize_t throttle_reason_pl4_show(struct device *dev,
struct device_attribute *attr,char *buff)+{
- struct intel_gt *gt = intel_gt_sysfs_get_drvdata(dev, attr->attr.name);
- struct intel_rps *rps = >->rps;
- bool pl4 = !!intel_rps_read_throttle_reason_pl4(rps);
- return scnprintf(buff, PAGE_SIZE, "%u\n", pl4);
+}
+static ssize_t throttle_reason_thermal_show(struct device *dev,
struct device_attribute *attr,char *buff)+{
- struct intel_gt *gt = intel_gt_sysfs_get_drvdata(dev, attr->attr.name);
- struct intel_rps *rps = >->rps;
- bool thermal = !!intel_rps_read_throttle_reason_thermal(rps);
- return scnprintf(buff, PAGE_SIZE, "%u\n", thermal);
+}
+static ssize_t throttle_reason_prochot_show(struct device *dev,
struct device_attribute *attr,char *buff)+{
- struct intel_gt *gt = intel_gt_sysfs_get_drvdata(dev, attr->attr.name);
- struct intel_rps *rps = >->rps;
- bool prochot = !!intel_rps_read_throttle_reason_prochot(rps);
- return scnprintf(buff, PAGE_SIZE, "%u\n", prochot);
+}
+static ssize_t throttle_reason_ratl_show(struct device *dev,
struct device_attribute *attr,char *buff)+{
- struct intel_gt *gt = intel_gt_sysfs_get_drvdata(dev, attr->attr.name);
- struct intel_rps *rps = >->rps;
- bool ratl = !!intel_rps_read_throttle_reason_ratl(rps);
- return scnprintf(buff, PAGE_SIZE, "%u\n", ratl);
+}
+static ssize_t throttle_reason_vr_thermalert_show(struct device *dev,
struct device_attribute *attr,char *buff)+{
- struct intel_gt *gt = intel_gt_sysfs_get_drvdata(dev, attr->attr.name);
- struct intel_rps *rps = >->rps;
- bool thermalert = !!intel_rps_read_throttle_reason_vr_thermalert(rps);
- return scnprintf(buff, PAGE_SIZE, "%u\n", thermalert);
+}
+static ssize_t throttle_reason_vr_tdc_show(struct device *dev,
struct device_attribute *attr,char *buff)+{
- struct intel_gt *gt = intel_gt_sysfs_get_drvdata(dev, attr->attr.name);
- struct intel_rps *rps = >->rps;
- bool tdc = !!intel_rps_read_throttle_reason_vr_tdc(rps);
- return scnprintf(buff, PAGE_SIZE, "%u\n", tdc);
+}
+static DEVICE_ATTR_RO(punit_req_freq_mhz); +static DEVICE_ATTR_RO(throttle_reason_status); +static DEVICE_ATTR_RO(throttle_reason_pl1); +static DEVICE_ATTR_RO(throttle_reason_pl2); +static DEVICE_ATTR_RO(throttle_reason_pl4); +static DEVICE_ATTR_RO(throttle_reason_thermal); +static DEVICE_ATTR_RO(throttle_reason_prochot); +static DEVICE_ATTR_RO(throttle_reason_ratl); +static DEVICE_ATTR_RO(throttle_reason_vr_thermalert); +static DEVICE_ATTR_RO(throttle_reason_vr_tdc);
+static const struct attribute *freq_attrs[] = {
- &dev_attr_punit_req_freq_mhz.attr,
- &dev_attr_throttle_reason_status.attr,
- &dev_attr_throttle_reason_pl1.attr,
- &dev_attr_throttle_reason_pl2.attr,
- &dev_attr_throttle_reason_pl4.attr,
- &dev_attr_throttle_reason_thermal.attr,
- &dev_attr_throttle_reason_prochot.attr,
- &dev_attr_throttle_reason_ratl.attr,
- &dev_attr_throttle_reason_vr_thermalert.attr,
- &dev_attr_throttle_reason_vr_tdc.attr,
- NULL
+};
static int intel_sysfs_rps_init(struct intel_gt *gt, struct kobject *kobj, const struct attribute * const *attrs) { @@ -493,4 +628,11 @@ void intel_gt_sysfs_pm_init(struct intel_gt *gt, struct kobject *kobj) if (ret) drm_warn(>->i915->drm, "failed to create gt%u RPS sysfs files", gt->info.id);
- ret = sysfs_create_files(kobj, freq_attrs);
- if (ret)
drm_warn(>->i915->drm,"failed to create gt%u throttle sysfs files",gt->info.id);
nit: would be nice to see %pe why it failed
} diff --git a/drivers/gpu/drm/i915/gt/intel_rps.c b/drivers/gpu/drm/i915/gt/intel_rps.c index fd95449ed46d..94c78cfaf9c9 100644 --- a/drivers/gpu/drm/i915/gt/intel_rps.c +++ b/drivers/gpu/drm/i915/gt/intel_rps.c @@ -2286,6 +2286,89 @@ void intel_rps_lower_unslice(struct intel_rps *rps) mutex_unlock(&rps->lock); }
+static u32 __rps_read_mmio(struct intel_gt *gt, i915_reg_t reg32)
this doesn't look like "rps" helper, rather like "gt" so it should have different prefix and maybe even be exported by the gt or uncore ?
unless you wanted:
static u32 __rps_read_mmio(struct intel_rps *rps, i915_reg_t reg32) { struct intel_gt *gt = rps_to_gt(rps);
+{
- intel_wakeref_t wakeref;
- u32 val;
- with_intel_runtime_pm(gt->uncore->rpm, wakeref)
val = intel_uncore_read(gt->uncore, reg32);- return val;
+}
+u32 intel_rps_read_throttle_reason_status(struct intel_rps *rps) +{
- struct intel_gt *gt = rps_to_gt(rps);
- u32 status = __rps_read_mmio(gt, GT0_PERF_LIMIT_REASONS) & GT0_PERF_LIMIT_REASONS_MASK;
- return status;
+}
+u32 intel_rps_read_throttle_reason_pl1(struct intel_rps *rps) +{
- struct intel_gt *gt = rps_to_gt(rps);
- u32 pl1 = __rps_read_mmio(gt, GT0_PERF_LIMIT_REASONS) & POWER_LIMIT_1_MASK;
- return pl1;
+}
+u32 intel_rps_read_throttle_reason_pl2(struct intel_rps *rps) +{
- struct intel_gt *gt = rps_to_gt(rps);
- u32 pl2 = __rps_read_mmio(gt, GT0_PERF_LIMIT_REASONS) & POWER_LIMIT_2_MASK;
- return pl2;
+}
+u32 intel_rps_read_throttle_reason_pl4(struct intel_rps *rps) +{
- struct intel_gt *gt = rps_to_gt(rps);
- u32 pl4 = __rps_read_mmio(gt, GT0_PERF_LIMIT_REASONS) & POWER_LIMIT_4_MASK;
- return pl4;
+}
+u32 intel_rps_read_throttle_reason_thermal(struct intel_rps *rps) +{
- struct intel_gt *gt = rps_to_gt(rps);
- u32 thermal = __rps_read_mmio(gt, GT0_PERF_LIMIT_REASONS) & THERMAL_LIMIT_MASK;
- return thermal;
+}
+u32 intel_rps_read_throttle_reason_prochot(struct intel_rps *rps) +{
- struct intel_gt *gt = rps_to_gt(rps);
- u32 prochot = __rps_read_mmio(gt, GT0_PERF_LIMIT_REASONS) & PROCHOT_MASK;
- return prochot;
+}
+u32 intel_rps_read_throttle_reason_ratl(struct intel_rps *rps) +{
- struct intel_gt *gt = rps_to_gt(rps);
- u32 ratl = __rps_read_mmio(gt, GT0_PERF_LIMIT_REASONS) & RATL_MASK;
- return ratl;
+}
+u32 intel_rps_read_throttle_reason_vr_thermalert(struct intel_rps *rps) +{
- struct intel_gt *gt = rps_to_gt(rps);
- u32 thermalert = __rps_read_mmio(gt, GT0_PERF_LIMIT_REASONS) & VR_THERMALERT_MASK;
- return thermalert;
+}
shouldn't we return bool by all of these functions as used/expected in show() counterparts ?
+u32 intel_rps_read_throttle_reason_vr_tdc(struct intel_rps *rps) +{
- struct intel_gt *gt = rps_to_gt(rps);
- u32 tdc = __rps_read_mmio(gt, GT0_PERF_LIMIT_REASONS) & VR_TDC_MASK;
- return tdc;
+}
/* External interface for intel_ips.ko */
static struct drm_i915_private __rcu *ips_mchdev; diff --git a/drivers/gpu/drm/i915/gt/intel_rps.h b/drivers/gpu/drm/i915/gt/intel_rps.h index c6d76a3d1331..b45ab983895c 100644 --- a/drivers/gpu/drm/i915/gt/intel_rps.h +++ b/drivers/gpu/drm/i915/gt/intel_rps.h @@ -47,6 +47,16 @@ u32 intel_rps_read_punit_req_frequency(struct intel_rps *rps); u32 intel_rps_read_state_cap(struct intel_rps *rps); void intel_rps_raise_unslice(struct intel_rps *rps); void intel_rps_lower_unslice(struct intel_rps *rps); +u32 intel_rps_read_throttle_reason(struct intel_rps *rps); +u32 intel_rps_read_throttle_reason_status(struct intel_rps *rps); +u32 intel_rps_read_throttle_reason_pl1(struct intel_rps *rps); +u32 intel_rps_read_throttle_reason_pl2(struct intel_rps *rps); +u32 intel_rps_read_throttle_reason_pl4(struct intel_rps *rps); +u32 intel_rps_read_throttle_reason_thermal(struct intel_rps *rps); +u32 intel_rps_read_throttle_reason_prochot(struct intel_rps *rps); +u32 intel_rps_read_throttle_reason_ratl(struct intel_rps *rps); +u32 intel_rps_read_throttle_reason_vr_thermalert(struct intel_rps *rps); +u32 intel_rps_read_throttle_reason_vr_tdc(struct intel_rps *rps);
void gen5_rps_irq_handler(struct intel_rps *rps); void gen6_rps_irq_handler(struct intel_rps *rps, u32 pm_iir); diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h index 2243d9d1d941..c4f53e5404cd 100644 --- a/drivers/gpu/drm/i915/i915_reg.h +++ b/drivers/gpu/drm/i915/i915_reg.h @@ -1836,6 +1836,17 @@ #define GEN9_RP_STATE_LIMITS _MMIO(0x138148) #define XEHPSDV_RP_STATE_CAP _MMIO(0x250014)
+#define GT0_PERF_LIMIT_REASONS _MMIO(0x1381A8) +#define GT0_PERF_LIMIT_REASONS_MASK 0x00000de3
this mask is different that other (FIELD_PREP/GET wont work) so maybe we should name it in special way ?
+#define PROCHOT_MASK BIT(1) +#define THERMAL_LIMIT_MASK BIT(2) +#define RATL_MASK BIT(6) +#define VR_THERMALERT_MASK BIT(7) +#define VR_TDC_MASK BIT(8) +#define POWER_LIMIT_4_MASK BIT(9) +#define POWER_LIMIT_1_MASK BIT(11) +#define POWER_LIMIT_2_MASK BIT(12)
REG_BIT ?
Michal
#define CHV_CLK_CTL1 _MMIO(0x101100) #define VLV_CLK_CTL2 _MMIO(0x101104) #define CLK_CTL2_CZCOUNT_30NS_SHIFT 28
Hi Michal,
[...]
+static ssize_t punit_req_freq_mhz_show(struct device *dev,
struct device_attribute *attr,char *buff)+{
- struct intel_gt *gt = intel_gt_sysfs_get_drvdata(dev, attr->attr.name);
- struct intel_rps *rps = >->rps;
- u32 preq = intel_rps_read_punit_req_frequency(rps);
- return scnprintf(buff, PAGE_SIZE, "%d\n", preq);
%u since preq is u32
and use sysfs_emit (also in below show functions)
sure! I'll change them.
[...]
static int intel_sysfs_rps_init(struct intel_gt *gt, struct kobject *kobj, const struct attribute * const *attrs) { @@ -493,4 +628,11 @@ void intel_gt_sysfs_pm_init(struct intel_gt *gt, struct kobject *kobj) if (ret) drm_warn(>->i915->drm, "failed to create gt%u RPS sysfs files", gt->info.id);
- ret = sysfs_create_files(kobj, freq_attrs);
- if (ret)
drm_warn(>->i915->drm,"failed to create gt%u throttle sysfs files",gt->info.id);nit: would be nice to see %pe why it failed
[...]
I will add it to the other cases as well.
+static u32 __rps_read_mmio(struct intel_gt *gt, i915_reg_t reg32)
this doesn't look like "rps" helper, rather like "gt" so it should have different prefix and maybe even be exported by the gt or uncore ?
unless you wanted:
static u32 __rps_read_mmio(struct intel_rps *rps, i915_reg_t reg32) { struct intel_gt *gt = rps_to_gt(rps);
+{
- intel_wakeref_t wakeref;
- u32 val;
- with_intel_runtime_pm(gt->uncore->rpm, wakeref)
val = intel_uncore_read(gt->uncore, reg32);- return val;
+}
Yes, you are right!
@Sujaritha: shall I move "__rps_read_mmio()" in intel_gt.c and call it intel_gt_read_mmio()?
[...]
+u32 intel_rps_read_throttle_reason_vr_thermalert(struct intel_rps *rps) +{
- struct intel_gt *gt = rps_to_gt(rps);
- u32 thermalert = __rps_read_mmio(gt, GT0_PERF_LIMIT_REASONS) & VR_THERMALERT_MASK;
- return thermalert;
+}
shouldn't we return bool by all of these functions as used/expected in show() counterparts ?
Suja?
[...]
+#define GT0_PERF_LIMIT_REASONS _MMIO(0x1381A8) +#define GT0_PERF_LIMIT_REASONS_MASK 0x00000de3
this mask is different that other (FIELD_PREP/GET wont work) so maybe we should name it in special way ?
As far as I understood this is still a mask and used as such. This mask is actually telling that there is some throttling going on.
It looks weird because there are some unwanted bits in between the interesting bits.
+#define PROCHOT_MASK BIT(1) +#define THERMAL_LIMIT_MASK BIT(2) +#define RATL_MASK BIT(6) +#define VR_THERMALERT_MASK BIT(7) +#define VR_TDC_MASK BIT(8) +#define POWER_LIMIT_4_MASK BIT(9) +#define POWER_LIMIT_1_MASK BIT(11) +#define POWER_LIMIT_2_MASK BIT(12)
REG_BIT ?
yes!
Thanks, Michal! Andi
On 3/13/2022 5:38 PM, Andi Shyti wrote:
Hi Michal,
[...]
+static ssize_t punit_req_freq_mhz_show(struct device *dev,
struct device_attribute *attr,char *buff)+{
- struct intel_gt *gt = intel_gt_sysfs_get_drvdata(dev, attr->attr.name);
- struct intel_rps *rps = >->rps;
- u32 preq = intel_rps_read_punit_req_frequency(rps);
- return scnprintf(buff, PAGE_SIZE, "%d\n", preq);
%u since preq is u32
and use sysfs_emit (also in below show functions)
sure! I'll change them.
[...]
static int intel_sysfs_rps_init(struct intel_gt *gt, struct kobject *kobj, const struct attribute * const *attrs) { @@ -493,4 +628,11 @@ void intel_gt_sysfs_pm_init(struct intel_gt *gt, struct kobject *kobj) if (ret) drm_warn(>->i915->drm, "failed to create gt%u RPS sysfs files", gt->info.id);
- ret = sysfs_create_files(kobj, freq_attrs);
- if (ret)
drm_warn(>->i915->drm,"failed to create gt%u throttle sysfs files",gt->info.id);nit: would be nice to see %pe why it failed
[...]
I will add it to the other cases as well.
+static u32 __rps_read_mmio(struct intel_gt *gt, i915_reg_t reg32)
this doesn't look like "rps" helper, rather like "gt" so it should have different prefix and maybe even be exported by the gt or uncore ?
unless you wanted:
static u32 __rps_read_mmio(struct intel_rps *rps, i915_reg_t reg32) { struct intel_gt *gt = rps_to_gt(rps);
+{
- intel_wakeref_t wakeref;
- u32 val;
- with_intel_runtime_pm(gt->uncore->rpm, wakeref)
val = intel_uncore_read(gt->uncore, reg32);- return val;
+}
Yes, you are right!
@Sujaritha: shall I move "__rps_read_mmio()" in intel_gt.c and call it intel_gt_read_mmio()?
[...]
Sure since it is kind of a gt helper, makes sense to have gt prefix.
+u32 intel_rps_read_throttle_reason_vr_thermalert(struct intel_rps *rps) +{
- struct intel_gt *gt = rps_to_gt(rps);
- u32 thermalert = __rps_read_mmio(gt, GT0_PERF_LIMIT_REASONS) & VR_THERMALERT_MASK;
- return thermalert;
+}
shouldn't we return bool by all of these functions as used/expected in show() counterparts ?
Suja?
[...]
Yes we can make this bool as well.
+#define GT0_PERF_LIMIT_REASONS _MMIO(0x1381A8) +#define GT0_PERF_LIMIT_REASONS_MASK 0x00000de3
this mask is different that other (FIELD_PREP/GET wont work) so maybe we should name it in special way ?
As far as I understood this is still a mask and used as such. This mask is actually telling that there is some throttling going on.
It looks weird because there are some unwanted bits in between the interesting bits.
+#define PROCHOT_MASK BIT(1) +#define THERMAL_LIMIT_MASK BIT(2) +#define RATL_MASK BIT(6) +#define VR_THERMALERT_MASK BIT(7) +#define VR_TDC_MASK BIT(8) +#define POWER_LIMIT_4_MASK BIT(9) +#define POWER_LIMIT_1_MASK BIT(11) +#define POWER_LIMIT_2_MASK BIT(12)
REG_BIT ?
yes!
Thanks, Michal! Andi
On 17.02.2022 15:41, Andi Shyti wrote:
From: Sujaritha Sundaresan sujaritha.sundaresan@intel.com
This patch adds the following new sysfs frequency attributes;
- punit_req_freq_mhz
- throttle_reason_status
- throttle_reason_pl1
- throttle_reason_pl2
- throttle_reason_pl4
- throttle_reason_thermal
- throttle_reason_prochot
- throttle_reason_ratl
- throttle_reason_vr_thermalert
- throttle_reason_vr_tdc
Signed-off-by: Sujaritha Sundaresan sujaritha.sundaresan@intel.com Signed-off-by: Andi Shyti andi.shyti@linux.intel.com Cc: Dale B Stimson dale.b.stimson@intel.com
drivers/gpu/drm/i915/gt/intel_gt_sysfs_pm.c | 142 ++++++++++++++++++++ drivers/gpu/drm/i915/gt/intel_rps.c | 83 ++++++++++++ drivers/gpu/drm/i915/gt/intel_rps.h | 10 ++ drivers/gpu/drm/i915/i915_reg.h | 11 ++ 4 files changed, 246 insertions(+)
diff --git a/drivers/gpu/drm/i915/gt/intel_gt_sysfs_pm.c b/drivers/gpu/drm/i915/gt/intel_gt_sysfs_pm.c index 8e86b8f675f1..8be676cd1607 100644 --- a/drivers/gpu/drm/i915/gt/intel_gt_sysfs_pm.c +++ b/drivers/gpu/drm/i915/gt/intel_gt_sysfs_pm.c @@ -463,6 +463,141 @@ static ssize_t rps_rp_mhz_show(struct device *dev, static const struct attribute * const gen6_rps_attrs[] = GEN6_RPS_ATTR; static const struct attribute * const gen6_gt_attrs[] = GEN6_GT_ATTR;
+static ssize_t punit_req_freq_mhz_show(struct device *dev,
struct device_attribute *attr,char *buff)+{
- struct intel_gt *gt = intel_gt_sysfs_get_drvdata(dev, attr->attr.name);
- struct intel_rps *rps = >->rps;
- u32 preq = intel_rps_read_punit_req_frequency(rps);
- return scnprintf(buff, PAGE_SIZE, "%d\n", preq);
+}
+static ssize_t throttle_reason_status_show(struct device *dev,
struct device_attribute *attr,char *buff)+{
- struct intel_gt *gt = intel_gt_sysfs_get_drvdata(dev, attr->attr.name);
- struct intel_rps *rps = >->rps;
- bool status = !!intel_rps_read_throttle_reason_status(rps);
!! is not necessary if you assign to bool variable, here and below.
- return scnprintf(buff, PAGE_SIZE, "%u\n", status);
+}
+static ssize_t throttle_reason_pl1_show(struct device *dev,
struct device_attribute *attr,char *buff)+{
- struct intel_gt *gt = intel_gt_sysfs_get_drvdata(dev, attr->attr.name);
- struct intel_rps *rps = >->rps;
- bool pl1 = !!intel_rps_read_throttle_reason_pl1(rps);
- return scnprintf(buff, PAGE_SIZE, "%u\n", pl1);
+}
+static ssize_t throttle_reason_pl2_show(struct device *dev,
struct device_attribute *attr,char *buff)+{
- struct intel_gt *gt = intel_gt_sysfs_get_drvdata(dev, attr->attr.name);
- struct intel_rps *rps = >->rps;
- bool pl2 = !!intel_rps_read_throttle_reason_pl2(rps);
- return scnprintf(buff, PAGE_SIZE, "%u\n", pl2);
+}
+static ssize_t throttle_reason_pl4_show(struct device *dev,
struct device_attribute *attr,char *buff)+{
- struct intel_gt *gt = intel_gt_sysfs_get_drvdata(dev, attr->attr.name);
- struct intel_rps *rps = >->rps;
- bool pl4 = !!intel_rps_read_throttle_reason_pl4(rps);
- return scnprintf(buff, PAGE_SIZE, "%u\n", pl4);
+}
+static ssize_t throttle_reason_thermal_show(struct device *dev,
struct device_attribute *attr,char *buff)+{
- struct intel_gt *gt = intel_gt_sysfs_get_drvdata(dev, attr->attr.name);
- struct intel_rps *rps = >->rps;
- bool thermal = !!intel_rps_read_throttle_reason_thermal(rps);
- return scnprintf(buff, PAGE_SIZE, "%u\n", thermal);
+}
+static ssize_t throttle_reason_prochot_show(struct device *dev,
struct device_attribute *attr,char *buff)+{
- struct intel_gt *gt = intel_gt_sysfs_get_drvdata(dev, attr->attr.name);
- struct intel_rps *rps = >->rps;
- bool prochot = !!intel_rps_read_throttle_reason_prochot(rps);
- return scnprintf(buff, PAGE_SIZE, "%u\n", prochot);
+}
+static ssize_t throttle_reason_ratl_show(struct device *dev,
struct device_attribute *attr,char *buff)+{
- struct intel_gt *gt = intel_gt_sysfs_get_drvdata(dev, attr->attr.name);
- struct intel_rps *rps = >->rps;
- bool ratl = !!intel_rps_read_throttle_reason_ratl(rps);
- return scnprintf(buff, PAGE_SIZE, "%u\n", ratl);
+}
+static ssize_t throttle_reason_vr_thermalert_show(struct device *dev,
struct device_attribute *attr,char *buff)+{
- struct intel_gt *gt = intel_gt_sysfs_get_drvdata(dev, attr->attr.name);
- struct intel_rps *rps = >->rps;
- bool thermalert = !!intel_rps_read_throttle_reason_vr_thermalert(rps);
- return scnprintf(buff, PAGE_SIZE, "%u\n", thermalert);
+}
+static ssize_t throttle_reason_vr_tdc_show(struct device *dev,
struct device_attribute *attr,char *buff)+{
- struct intel_gt *gt = intel_gt_sysfs_get_drvdata(dev, attr->attr.name);
- struct intel_rps *rps = >->rps;
- bool tdc = !!intel_rps_read_throttle_reason_vr_tdc(rps);
- return scnprintf(buff, PAGE_SIZE, "%u\n", tdc);
+}
Nine, almost the same functions are asking for proper abstraction, a'la: struct intel_rps_bool_attribute { struct device_attribute attr; u32 (*func)(struct intel_rps *rps); } And for example:
static INTEL_RPS_BOOL_ATTR_RO(throttle_reason_status, intel_rps_read_throttle_reason_status);
Or at least some macro magic to hide this code redundancy.
Regards Andrzej
+static DEVICE_ATTR_RO(punit_req_freq_mhz); +static DEVICE_ATTR_RO(throttle_reason_status); +static DEVICE_ATTR_RO(throttle_reason_pl1); +static DEVICE_ATTR_RO(throttle_reason_pl2); +static DEVICE_ATTR_RO(throttle_reason_pl4); +static DEVICE_ATTR_RO(throttle_reason_thermal); +static DEVICE_ATTR_RO(throttle_reason_prochot); +static DEVICE_ATTR_RO(throttle_reason_ratl); +static DEVICE_ATTR_RO(throttle_reason_vr_thermalert); +static DEVICE_ATTR_RO(throttle_reason_vr_tdc);
+static const struct attribute *freq_attrs[] = {
- &dev_attr_punit_req_freq_mhz.attr,
- &dev_attr_throttle_reason_status.attr,
- &dev_attr_throttle_reason_pl1.attr,
- &dev_attr_throttle_reason_pl2.attr,
- &dev_attr_throttle_reason_pl4.attr,
- &dev_attr_throttle_reason_thermal.attr,
- &dev_attr_throttle_reason_prochot.attr,
- &dev_attr_throttle_reason_ratl.attr,
- &dev_attr_throttle_reason_vr_thermalert.attr,
- &dev_attr_throttle_reason_vr_tdc.attr,
- NULL
+};
- static int intel_sysfs_rps_init(struct intel_gt *gt, struct kobject *kobj, const struct attribute * const *attrs) {
@@ -493,4 +628,11 @@ void intel_gt_sysfs_pm_init(struct intel_gt *gt, struct kobject *kobj) if (ret) drm_warn(>->i915->drm, "failed to create gt%u RPS sysfs files", gt->info.id);
- ret = sysfs_create_files(kobj, freq_attrs);
- if (ret)
drm_warn(>->i915->drm,"failed to create gt%u throttle sysfs files",gt->info.id);- }
diff --git a/drivers/gpu/drm/i915/gt/intel_rps.c b/drivers/gpu/drm/i915/gt/intel_rps.c index fd95449ed46d..94c78cfaf9c9 100644 --- a/drivers/gpu/drm/i915/gt/intel_rps.c +++ b/drivers/gpu/drm/i915/gt/intel_rps.c @@ -2286,6 +2286,89 @@ void intel_rps_lower_unslice(struct intel_rps *rps) mutex_unlock(&rps->lock); }
+static u32 __rps_read_mmio(struct intel_gt *gt, i915_reg_t reg32) +{
- intel_wakeref_t wakeref;
- u32 val;
- with_intel_runtime_pm(gt->uncore->rpm, wakeref)
val = intel_uncore_read(gt->uncore, reg32);- return val;
+}
+u32 intel_rps_read_throttle_reason_status(struct intel_rps *rps) +{
- struct intel_gt *gt = rps_to_gt(rps);
- u32 status = __rps_read_mmio(gt, GT0_PERF_LIMIT_REASONS) & GT0_PERF_LIMIT_REASONS_MASK;
- return status;
+}
+u32 intel_rps_read_throttle_reason_pl1(struct intel_rps *rps) +{
- struct intel_gt *gt = rps_to_gt(rps);
- u32 pl1 = __rps_read_mmio(gt, GT0_PERF_LIMIT_REASONS) & POWER_LIMIT_1_MASK;
- return pl1;
+}
+u32 intel_rps_read_throttle_reason_pl2(struct intel_rps *rps) +{
- struct intel_gt *gt = rps_to_gt(rps);
- u32 pl2 = __rps_read_mmio(gt, GT0_PERF_LIMIT_REASONS) & POWER_LIMIT_2_MASK;
- return pl2;
+}
+u32 intel_rps_read_throttle_reason_pl4(struct intel_rps *rps) +{
- struct intel_gt *gt = rps_to_gt(rps);
- u32 pl4 = __rps_read_mmio(gt, GT0_PERF_LIMIT_REASONS) & POWER_LIMIT_4_MASK;
- return pl4;
+}
+u32 intel_rps_read_throttle_reason_thermal(struct intel_rps *rps) +{
- struct intel_gt *gt = rps_to_gt(rps);
- u32 thermal = __rps_read_mmio(gt, GT0_PERF_LIMIT_REASONS) & THERMAL_LIMIT_MASK;
- return thermal;
+}
+u32 intel_rps_read_throttle_reason_prochot(struct intel_rps *rps) +{
- struct intel_gt *gt = rps_to_gt(rps);
- u32 prochot = __rps_read_mmio(gt, GT0_PERF_LIMIT_REASONS) & PROCHOT_MASK;
- return prochot;
+}
+u32 intel_rps_read_throttle_reason_ratl(struct intel_rps *rps) +{
- struct intel_gt *gt = rps_to_gt(rps);
- u32 ratl = __rps_read_mmio(gt, GT0_PERF_LIMIT_REASONS) & RATL_MASK;
- return ratl;
+}
+u32 intel_rps_read_throttle_reason_vr_thermalert(struct intel_rps *rps) +{
- struct intel_gt *gt = rps_to_gt(rps);
- u32 thermalert = __rps_read_mmio(gt, GT0_PERF_LIMIT_REASONS) & VR_THERMALERT_MASK;
- return thermalert;
+}
+u32 intel_rps_read_throttle_reason_vr_tdc(struct intel_rps *rps) +{
- struct intel_gt *gt = rps_to_gt(rps);
- u32 tdc = __rps_read_mmio(gt, GT0_PERF_LIMIT_REASONS) & VR_TDC_MASK;
- return tdc;
+}
/* External interface for intel_ips.ko */
static struct drm_i915_private __rcu *ips_mchdev;
diff --git a/drivers/gpu/drm/i915/gt/intel_rps.h b/drivers/gpu/drm/i915/gt/intel_rps.h index c6d76a3d1331..b45ab983895c 100644 --- a/drivers/gpu/drm/i915/gt/intel_rps.h +++ b/drivers/gpu/drm/i915/gt/intel_rps.h @@ -47,6 +47,16 @@ u32 intel_rps_read_punit_req_frequency(struct intel_rps *rps); u32 intel_rps_read_state_cap(struct intel_rps *rps); void intel_rps_raise_unslice(struct intel_rps *rps); void intel_rps_lower_unslice(struct intel_rps *rps); +u32 intel_rps_read_throttle_reason(struct intel_rps *rps); +u32 intel_rps_read_throttle_reason_status(struct intel_rps *rps); +u32 intel_rps_read_throttle_reason_pl1(struct intel_rps *rps); +u32 intel_rps_read_throttle_reason_pl2(struct intel_rps *rps); +u32 intel_rps_read_throttle_reason_pl4(struct intel_rps *rps); +u32 intel_rps_read_throttle_reason_thermal(struct intel_rps *rps); +u32 intel_rps_read_throttle_reason_prochot(struct intel_rps *rps); +u32 intel_rps_read_throttle_reason_ratl(struct intel_rps *rps); +u32 intel_rps_read_throttle_reason_vr_thermalert(struct intel_rps *rps); +u32 intel_rps_read_throttle_reason_vr_tdc(struct intel_rps *rps);
void gen5_rps_irq_handler(struct intel_rps *rps); void gen6_rps_irq_handler(struct intel_rps *rps, u32 pm_iir); diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h index 2243d9d1d941..c4f53e5404cd 100644 --- a/drivers/gpu/drm/i915/i915_reg.h +++ b/drivers/gpu/drm/i915/i915_reg.h @@ -1836,6 +1836,17 @@ #define GEN9_RP_STATE_LIMITS _MMIO(0x138148) #define XEHPSDV_RP_STATE_CAP _MMIO(0x250014)
+#define GT0_PERF_LIMIT_REASONS _MMIO(0x1381A8) +#define GT0_PERF_LIMIT_REASONS_MASK 0x00000de3 +#define PROCHOT_MASK BIT(1) +#define THERMAL_LIMIT_MASK BIT(2) +#define RATL_MASK BIT(6) +#define VR_THERMALERT_MASK BIT(7) +#define VR_TDC_MASK BIT(8) +#define POWER_LIMIT_4_MASK BIT(9) +#define POWER_LIMIT_1_MASK BIT(11) +#define POWER_LIMIT_2_MASK BIT(12)
- #define CHV_CLK_CTL1 _MMIO(0x101100) #define VLV_CLK_CTL2 _MMIO(0x101104) #define CLK_CTL2_CZCOUNT_30NS_SHIFT 28
dri-devel@lists.freedesktop.org
-
Andi Shyti -
Andi Shyti -
Andrzej Hajda -
Joonas Lahtinen -
kernel test robot -
Michal Wajdeczko -
Sundaresan, Sujaritha -
Tvrtko Ursulin