diff mbox series

[07/27] drm/i915/pxp: Add PXP-related registers into allowlist

Message ID 20201115210815.5272-7-sean.z.huang@intel.com (mailing list archive)
State New, archived
Headers show
Series [01/27] drm/i915/pxp: Introduce Intel PXP component | expand

Commit Message

Huang, Sean Z Nov. 15, 2020, 9:07 p.m. UTC 
Add several PXP-related reg into allowlist to allow
ring3 driver to read the those register values.

Signed-off-by: Huang, Sean Z <sean.z.huang@intel.com>
---
 drivers/gpu/drm/i915/i915_reg.h     |  8 ++++
 drivers/gpu/drm/i915/intel_uncore.c | 57 +++++++++++++++++++++--------
 2 files changed, 50 insertions(+), 15 deletions(-)

Comments

Joonas Lahtinen Nov. 16, 2020, 10:33 a.m. UTC | #1 
Quoting Huang, Sean Z (2020-11-15 23:07:55)
> Add several PXP-related reg into allowlist to allow
> ring3 driver to read the those register values.

The individual registers need to be spelled out and their usage on the
UMD side needs to be documented.

There needs to be a link to the Open Source userspace which requires
these registers.

> Signed-off-by: Huang, Sean Z <sean.z.huang@intel.com>

<SNIP>

> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -12419,4 +12419,12 @@ enum skl_power_gate {
>  #define TGL_ROOT_DEVICE_SKU_ULX                0x2
>  #define TGL_ROOT_DEVICE_SKU_ULT                0x4
>  
> +/* Registers for passlist check */
> +#define PXP_REG_01_LOWERBOUND          _MMIO(0x32260)
> +#define PXP_REG_01_UPPERBOUND          _MMIO(0x32268)
> +#define PXP_REG_02_LOWERBOUND          _MMIO(0x32670)
> +#define PXP_REG_02_UPPERBOUND          _MMIO(0x32678)
> +#define PXP_REG_03_LOWERBOUND          _MMIO(0x32860)
> +#define PXP_REG_03_UPPERBOUND          _MMIO(0x32c7c)

This is not any more informative than embedding magical values in the code.

Regards, Joonas
Chris Wilson Nov. 16, 2020, 11:10 a.m. UTC | #2 
Quoting Joonas Lahtinen (2020-11-16 10:33:07)
> Quoting Huang, Sean Z (2020-11-15 23:07:55)
> > Add several PXP-related reg into allowlist to allow
> > ring3 driver to read the those register values.
> 
> The individual registers need to be spelled out and their usage on the
> UMD side needs to be documented.

It's a huge block of registers that we need to manually verify for
mistakes. It also does not bode well for the userspace design as probing
so many through a single ioctl is going to be ratelimiting, nor why do
they need to check a entire kilobyte register file. Hence why the
userspace portion is essential to justify this as being the preferred
design.
-Chris
diff mbox series

Patch

diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index faf6b06145fa..5c51c9df8b28 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -12419,4 +12419,12 @@  enum skl_power_gate {
 #define TGL_ROOT_DEVICE_SKU_ULX		0x2
 #define TGL_ROOT_DEVICE_SKU_ULT		0x4
 
+/* Registers for passlist check */
+#define PXP_REG_01_LOWERBOUND		_MMIO(0x32260)
+#define PXP_REG_01_UPPERBOUND		_MMIO(0x32268)
+#define PXP_REG_02_LOWERBOUND		_MMIO(0x32670)
+#define PXP_REG_02_UPPERBOUND		_MMIO(0x32678)
+#define PXP_REG_03_LOWERBOUND		_MMIO(0x32860)
+#define PXP_REG_03_UPPERBOUND		_MMIO(0x32c7c)
+
 #endif /* _I915_REG_H_ */
diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
index c9ef0025c60e..670856e095c4 100644
--- a/drivers/gpu/drm/i915/intel_uncore.c
+++ b/drivers/gpu/drm/i915/intel_uncore.c
@@ -1990,16 +1990,41 @@  void intel_uncore_fini_mmio(struct intel_uncore *uncore)
 }
 
 static const struct reg_allowlist {
-	i915_reg_t offset_ldw;
+	i915_reg_t offset_ldw_lowerbound;
+	i915_reg_t offset_ldw_upperbound;
 	i915_reg_t offset_udw;
 	u16 gen_mask;
 	u8 size;
-} reg_read_allowlist[] = { {
-	.offset_ldw = RING_TIMESTAMP(RENDER_RING_BASE),
+} reg_read_allowlist[] = {
+	{
+	.offset_ldw_lowerbound = RING_TIMESTAMP(RENDER_RING_BASE),
+	.offset_ldw_upperbound = RING_TIMESTAMP(RENDER_RING_BASE),
 	.offset_udw = RING_TIMESTAMP_UDW(RENDER_RING_BASE),
 	.gen_mask = INTEL_GEN_MASK(4, 12),
 	.size = 8
-} };
+	},
+	{
+	.offset_ldw_lowerbound = PXP_REG_01_LOWERBOUND,
+	.offset_ldw_upperbound = PXP_REG_01_UPPERBOUND,
+	.offset_udw = {0},
+	.gen_mask = INTEL_GEN_MASK(4, 12),
+	.size = 4
+	},
+	{
+	.offset_ldw_lowerbound = PXP_REG_02_LOWERBOUND,
+	.offset_ldw_upperbound = PXP_REG_02_UPPERBOUND,
+	.offset_udw = {0},
+	.gen_mask = INTEL_GEN_MASK(4, 12),
+	.size = 4
+	},
+	{
+	.offset_ldw_lowerbound = PXP_REG_03_LOWERBOUND,
+	.offset_ldw_upperbound = PXP_REG_03_UPPERBOUND,
+	.offset_udw = {0},
+	.gen_mask = INTEL_GEN_MASK(4, 12),
+	.size = 4
+	}
+};
 
 int i915_reg_read_ioctl(struct drm_device *dev,
 			void *data, struct drm_file *file)
@@ -2012,18 +2037,22 @@  int i915_reg_read_ioctl(struct drm_device *dev,
 	unsigned int flags;
 	int remain;
 	int ret = 0;
+	i915_reg_t offset_ldw;
 
 	entry = reg_read_allowlist;
 	remain = ARRAY_SIZE(reg_read_allowlist);
 	while (remain) {
-		u32 entry_offset = i915_mmio_reg_offset(entry->offset_ldw);
+		u32 entry_offset_lb = i915_mmio_reg_offset(entry->offset_ldw_lowerbound);
+		u32 entry_offset_ub = i915_mmio_reg_offset(entry->offset_ldw_upperbound);
 
 		GEM_BUG_ON(!is_power_of_2(entry->size));
 		GEM_BUG_ON(entry->size > 8);
-		GEM_BUG_ON(entry_offset & (entry->size - 1));
+		GEM_BUG_ON(entry_offset_lb & (entry->size - 1));
+		GEM_BUG_ON(entry_offset_ub & (entry->size - 1));
 
 		if (INTEL_INFO(i915)->gen_mask & entry->gen_mask &&
-		    entry_offset == (reg->offset & -entry->size))
+		    entry_offset_lb <= (reg->offset & -entry->size) &&
+		    (reg->offset & -entry->size) <= entry_offset_ub)
 			break;
 		entry++;
 		remain--;
@@ -2033,23 +2062,21 @@  int i915_reg_read_ioctl(struct drm_device *dev,
 		return -EINVAL;
 
 	flags = reg->offset & (entry->size - 1);
+	offset_ldw = _MMIO(reg->offset - flags);
 
 	with_intel_runtime_pm(&i915->runtime_pm, wakeref) {
 		if (entry->size == 8 && flags == I915_REG_READ_8B_WA)
 			reg->val = intel_uncore_read64_2x32(uncore,
-							    entry->offset_ldw,
+							    offset_ldw,
 							    entry->offset_udw);
 		else if (entry->size == 8 && flags == 0)
-			reg->val = intel_uncore_read64(uncore,
-						       entry->offset_ldw);
+			reg->val = intel_uncore_read64(uncore, offset_ldw);
 		else if (entry->size == 4 && flags == 0)
-			reg->val = intel_uncore_read(uncore, entry->offset_ldw);
+			reg->val = intel_uncore_read(uncore, offset_ldw);
 		else if (entry->size == 2 && flags == 0)
-			reg->val = intel_uncore_read16(uncore,
-						       entry->offset_ldw);
+			reg->val = intel_uncore_read16(uncore, offset_ldw);
 		else if (entry->size == 1 && flags == 0)
-			reg->val = intel_uncore_read8(uncore,
-						      entry->offset_ldw);
+			reg->val = intel_uncore_read8(uncore, offset_ldw);
 		else
 			ret = -EINVAL;
 	}