On Tue, Feb 02, 2021 at 08:24:15AM +0000, Chris Wilson wrote:
Quoting Umesh Nerlige Ramappa (2021-02-02 07:54:15)
Validity of an OA format is checked by using a sparse array of formats
per gen. Instead maintain a mask of supported formats for a platform in
the perf object.
Signed-off-by: Umesh Nerlige Ramappa <umesh.nerlige.ramappa@xxxxxxxxx>
---
drivers/gpu/drm/i915/i915_perf.c | 64 +++++++++++++++++++++++++-
drivers/gpu/drm/i915/i915_perf_types.h | 16 +++++++
2 files changed, 79 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/i915/i915_perf.c b/drivers/gpu/drm/i915/i915_perf.c
index 112ba5f2ce90..973577fcad58 100644
--- a/drivers/gpu/drm/i915/i915_perf.c
+++ b/drivers/gpu/drm/i915/i915_perf.c
@@ -3524,6 +3524,19 @@ static u64 oa_exponent_to_ns(struct i915_perf *perf, int exponent)
2ULL << exponent);
}
+static __always_inline bool
+oa_format_valid(struct i915_perf *perf, enum drm_i915_oa_format format)
+{
+ return !!(perf->format_mask[__format_index(format)] &
+ __format_bit(format));
!! is already provided by the implicit cast to (bool)
+}
+
+static __always_inline void
+oa_format_add(struct i915_perf *perf, enum drm_i915_oa_format format)
+{
+ perf->format_mask[__format_index(format)] |= __format_bit(format);
+}
+
/**
* read_properties_unlocked - validate + copy userspace stream open properties
* @perf: i915 perf instance
@@ -3615,7 +3628,7 @@ static int read_properties_unlocked(struct i915_perf *perf,
value);
return -EINVAL;
}
- if (!perf->oa_formats[value].size) {
+ if (!oa_format_valid(perf, value)) {
DRM_DEBUG("Unsupported OA report format %llu\n",
value);
return -EINVAL;
@@ -4259,6 +4272,53 @@ static struct ctl_table dev_root[] = {
{}
};
+static void oa_init_supported_formats(struct i915_perf *perf)
+{
+ struct drm_i915_private *i915 = perf->i915;
+ enum intel_platform platform = INTEL_INFO(i915)->platform;
+
+ switch (platform) {
+ case INTEL_HASWELL:
+ oa_format_add(perf, I915_OA_FORMAT_A13);
+ oa_format_add(perf, I915_OA_FORMAT_A13);
+ oa_format_add(perf, I915_OA_FORMAT_A29);
+ oa_format_add(perf, I915_OA_FORMAT_A13_B8_C8);
+ oa_format_add(perf, I915_OA_FORMAT_B4_C8);
+ oa_format_add(perf, I915_OA_FORMAT_A45_B8_C8);
+ oa_format_add(perf, I915_OA_FORMAT_B4_C8_A16);
+ oa_format_add(perf, I915_OA_FORMAT_C4_B8);
+ break;
+
+ case INTEL_BROADWELL:
+ case INTEL_CHERRYVIEW:
+ case INTEL_SKYLAKE:
+ case INTEL_BROXTON:
+ case INTEL_KABYLAKE:
+ case INTEL_GEMINILAKE:
+ case INTEL_COFFEELAKE:
+ case INTEL_COMETLAKE:
+ case INTEL_CANNONLAKE:
+ case INTEL_ICELAKE:
+ case INTEL_ELKHARTLAKE:
+ case INTEL_JASPERLAKE:
+ oa_format_add(perf, I915_OA_FORMAT_A12);
+ oa_format_add(perf, I915_OA_FORMAT_A12_B8_C8);
+ oa_format_add(perf, I915_OA_FORMAT_A32u40_A4u32_B8_C8);
+ oa_format_add(perf, I915_OA_FORMAT_C4_B8);
+ break;
Ok, this looks as compact and readable as writing it as a bunch of
tables. I presume there's a reason you didn't just use generation rather
than platform.
switch (gen) {
case 7:
haswell();
break;
case 8 .. 11:
broadwell();
break;
case 12:
tigerlake();
break;
}
if you wanted to stick with a switch rather than an if-else tree for the
ranges.
only haswell is supported on gen7 and gen12 may define new formats that
are platform specific.
How about a mix? -
if (gen == 7 && haswell)
haswell();
else if (gen >= 8 && gen <= 11)
broadwell;
else
gen12_formats();
gen12_formats can choose to use the switch if formats vary between
platforms.
Thanks,
Umesh
Note you could equally do
case INTEL_BROADWELL .. INTEL_JASPERLAKE:
but I expect that to cause confusion for the reader.
+ /**
+ * Use a format mask to store the supported formats
+ * for a platform.
+ */
+#define __fbits (BITS_PER_TYPE(u32))
+#define __format_bit(__f) \
+ BIT((__f) & (__fbits - 1))
+
+#define __format_index_shift (5)
+#define __format_index(__f) \
+ (((__f) & ~(__fbits - 1)) >> __format_index_shift)
+
+#define FORMAT_MASK_SIZE (((I915_OA_FORMAT_MAX - 1) / __fbits) + 1)
+ u32 format_mask[FORMAT_MASK_SIZE];
This is just open-coding set_bit/test_bit
#define FORMAT_MASK_SIZE DIV_ROUND_UP(I915_OA_FORMAT_MAX - 1, BITS_PER_LONG)
unsigned long format_mask[FORMAT_MASK_SIZE];
static __always_inline bool
oa_format_valid(struct i915_perf *perf, enum drm_i915_oa_format format)
{
return test_bit(format, perf->format_mask);
}
static __always_inline void
oa_format_add(struct i915_perf *perf, enum drm_i915_oa_format format)
{
__set_bit(format, perf->format_mask);
}
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
