Re: [Intel-gfx] [PATCH i-g-t v4 2/2] i915/gem_ctx_isolation: Check engine relative registers (revised)
On 2/14/20 6:33 PM, Dale B Stimson wrote:
From: Chris Wilson
Some of the non-privileged registers are at the same offset on each
engine. We can improve our coverage for unknown HW layout by using the
reported engine->mmio_base for relative offsets.
Subsequent to sign-off by Chris Wilson, added by Dale B Stimson:
Modify previous "i915/gem_ctx_isolation: Check engine relative registers"
to support alternative mmio_base infrastructure API.
Signed-off-by: Chris Wilson
Signed-off-by: Dale B Stimson
Acked-by: Robert M. Fosha
---
lib/i915/gem_mmio_base.c | 7 +-
tests/i915/gem_ctx_isolation.c | 229 -
2 files changed, 141 insertions(+), 95 deletions(-)
diff --git a/lib/i915/gem_mmio_base.c b/lib/i915/gem_mmio_base.c
index d1b83221a..c712b4431 100644
--- a/lib/i915/gem_mmio_base.c
+++ b/lib/i915/gem_mmio_base.c
@@ -286,13 +286,14 @@ struct eng_mmio_base_table_s
*gem_engine_mmio_base_info_get(int fd_dev)
{
struct eng_mmio_base_table_s *mbp = NULL;
- /* If and when better ways are provided to find the mmio_base
-* information, they may be added them here in order of preference.
+ /* If and when more desirable ways exist to find the mmio_base
+* information, they may be added here, in order of consideration.
*/
#if 0
+ /* Anticipating a future method: */
if (!mbp)
- mbp = _mmio_base_info_get_via_sysfs(fd_dev);
+ mbp = _gem_engine_mmio_base_info_get_sysfs(fd_dev);
#endif
if (!mbp)
diff --git a/tests/i915/gem_ctx_isolation.c b/tests/i915/gem_ctx_isolation.c
index 1b66fec11..07ffbb84a 100644
--- a/tests/i915/gem_ctx_isolation.c
+++ b/tests/i915/gem_ctx_isolation.c
@@ -70,6 +70,7 @@ static const struct named_register {
uint32_t ignore_bits;
uint32_t write_mask; /* some registers bits do not exist */
bool masked;
+ bool relative;
} nonpriv_registers[] = {
{ "NOPID", NOCTX, RCS0, 0x2094 },
{ "MI_PREDICATE_RESULT_2", NOCTX, RCS0, 0x23bc },
@@ -109,7 +110,6 @@ static const struct named_register {
{ "PS_DEPTH_COUNT_1", GEN8, RCS0, 0x22f8, 2 },
{ "BB_OFFSET", GEN8, RCS0, 0x2158, .ignore_bits = 0x7 },
{ "MI_PREDICATE_RESULT_1", GEN8, RCS0, 0x241c },
- { "CS_GPR", GEN8, RCS0, 0x2600, 32 },
{ "OA_CTX_CONTROL", GEN8, RCS0, 0x2360 },
{ "OACTXID", GEN8, RCS0, 0x2364 },
{ "PS_INVOCATION_COUNT_2", GEN8, RCS0, 0x2448, 2, .write_mask = ~0x3 },
@@ -138,79 +138,56 @@ static const struct named_register {
{ "CTX_PREEMPT", NOCTX /* GEN10 */, RCS0, 0x2248 },
{ "CS_CHICKEN1", GEN11, RCS0, 0x2580, .masked = true },
- { "HDC_CHICKEN1", GEN_RANGE(10, 10), RCS0, 0x7304, .masked = true },
/* Privileged (enabled by w/a + FORCE_TO_NONPRIV) */
{ "CTX_PREEMPT", NOCTX /* GEN9 */, RCS0, 0x2248 },
{ "CS_CHICKEN1", GEN_RANGE(9, 10), RCS0, 0x2580, .masked = true },
{ "COMMON_SLICE_CHICKEN2", GEN_RANGE(9, 9), RCS0, 0x7014, .masked =
true },
- { "HDC_CHICKEN1", GEN_RANGE(9, 9), RCS0, 0x7304, .masked = true },
+ { "HDC_CHICKEN1", GEN_RANGE(9, 10), RCS0, 0x7304, .masked = true },
{ "SLICE_COMMON_ECO_CHICKEN1", GEN_RANGE(11, 11) /* + glk */, RCS0,
0x731c, .masked = true },
{ "L3SQREG4", NOCTX /* GEN9:skl,kbl */, RCS0, 0xb118, .write_mask =
~0x10 },
{ "HALF_SLICE_CHICKEN7", GEN_RANGE(11, 11), RCS0, 0xe194, .masked =
true },
{ "SAMPLER_MODE", GEN_RANGE(11, 11), RCS0, 0xe18c, .masked = true },
- { "BCS_GPR", GEN9, BCS0, 0x22600, 32 },
{ "BCS_SWCTRL", GEN8, BCS0, 0x22200, .write_mask = 0x3, .masked = true
},
{ "MFC_VDBOX1", NOCTX, VCS0, 0x12800, 64 },
{ "MFC_VDBOX2", NOCTX, VCS1, 0x1c800, 64 },
- { "VCS0_GPR", GEN_RANGE(9, 10), VCS0, 0x12600, 32 },
- { "VCS1_GPR", GEN_RANGE(9, 10), VCS1, 0x1c600, 32 },
- { "VECS_GPR", GEN_RANGE(9, 10), VECS0, 0x1a600, 32 },
-
- { "VCS0_GPR", GEN11, VCS0, 0x1c0600, 32 },
- { "VCS1_GPR", GEN11, VCS1, 0x1c4600, 32 },
- { "VCS2_GPR", GEN11, VCS2, 0x1d0600, 32 },
- { "VCS3_GPR", GEN11, VCS3, 0x1d4600, 32 },
- { "VECS_GPR", GEN11, VECS0, 0x1c8600, 32 },
+ { "xCS_GPR", GEN9, ALL, 0x600, 32, .relative = true },
{}
}, ignore_registers[] = {
{ "RCS timestamp", GEN6, ~0u, 0x2358 },
{ "BCS timestamp", GEN7, ~0u, 0x22358 },
- { "VCS0 timestamp", GEN_RANGE(7, 10), ~0u, 0x12358 },
- { "VCS1 timestamp", GEN_RANGE(7, 10), ~0u, 0x1c358 },
- { "VECS timestamp", GEN_RANGE(8, 10), ~0u, 0x1a358 },
-
- { "VCS0 timestamp", GEN11, ~0u, 0x1c0358 },
- { "VCS1 timestamp", GEN11, ~0u, 0x1c4358 },
- { "VCS2 timestamp", GEN11, ~0u, 0x1d0358 },
- { "VCS3 timestamp", GEN11, ~0u, 0x1d4358 },
- { "VECS timestamp", GEN11, ~0u, 0x1c8358 },
+ { "xCS timestamp", GEN8, ALL, 0x358, .relative = true },
/* huc read only */
- { "BSD0 0x2000", G
Re: [Intel-gfx] [PATCH i-g-t v4 1/2] i915/gem_mmio_base.c - get mmio_base from debugfs (if possible)
On 2/14/20 6:33 PM, Dale B Stimson wrote:
Signed-off-by: Dale B Stimson
Acked-by: Robert M. Fosha
---
lib/Makefile.sources | 2 +
lib/i915/gem_mmio_base.c | 353 +++
lib/i915/gem_mmio_base.h | 19 +++
lib/igt.h| 1 +
lib/meson.build | 1 +
5 files changed, 376 insertions(+)
create mode 100644 lib/i915/gem_mmio_base.c
create mode 100644 lib/i915/gem_mmio_base.h
diff --git a/lib/Makefile.sources b/lib/Makefile.sources
index 3e573f267..4c5d50d5d 100644
--- a/lib/Makefile.sources
+++ b/lib/Makefile.sources
@@ -7,6 +7,8 @@ lib_source_list = \
i915/gem_context.h \
i915/gem_engine_topology.c \
i915/gem_engine_topology.h \
+ i915/gem_mmio_base.c\
+ i915/gem_mmio_base.h\
i915/gem_scheduler.c\
i915/gem_scheduler.h\
i915/gem_submission.c \
diff --git a/lib/i915/gem_mmio_base.c b/lib/i915/gem_mmio_base.c
new file mode 100644
index 0..d1b83221a
--- /dev/null
+++ b/lib/i915/gem_mmio_base.c
@@ -0,0 +1,353 @@
+// Copyright (C) 2020 Intel Corporation
+//
+// SPDX-License-Identifier: MIT
+
+#include
+
+#include
+
+#include "igt.h"
+
+struct eng_mmio_base_s {
+ char name[8];
+ uint32_t mmio_base;
+};
+
+struct eng_mmio_base_table_s {
+ unsigned int mb_cnt;
+ struct eng_mmio_base_s mb_tab[GEM_MAX_ENGINES];
+};
+
+
+static struct eng_mmio_base_table_s *_gem_engine_mmio_info_dup(
+ const struct eng_mmio_base_table_s *mbpi)
+{
+ struct eng_mmio_base_table_s *mbpo;
+ size_t nbytes;
+
+ nbytes = offsetof(typeof(struct eng_mmio_base_table_s),
mb_tab[mbpi->mb_cnt]);
+ mbpo = malloc(nbytes);
+ igt_assert(mbpo);
+ memcpy(mbpo, mbpi, nbytes);
+
+ return mbpo;
+}
+
+void gem_engine_mmio_base_info_free(struct eng_mmio_base_table_s *mbp)
+{
+ if (mbp)
+ free(mbp);
+}
+
+static void _gem_engine_mmio_info_legacy_add(struct eng_mmio_base_table_s *mbp,
+ const char *eng_name, uint32_t mmio_base)
+{
+ if (mmio_base) {
+ strncpy(mbp->mb_tab[mbp->mb_cnt].name, eng_name,
+ sizeof(mbp->mb_tab[0].name));
+ mbp->mb_tab[mbp->mb_cnt].mmio_base = mmio_base;
+ mbp->mb_cnt++;
+ }
+}
+
+/**
+ * _gem_engine_mmio_base_info_get_legacy:
+ * @fd_dev: file descriptor upon which device is open or -1 to use defaults.
+ *
+ * Provides per-engine mmio_base information from legacy built-in values
+ * for the case when the information is not otherwise available.
+ *
+ * Returns:
+ * Pointer to dynamically allocated struct eng_mmio_base_table_s describing
+ * engine config or NULL.
+ * The allocated size does not include unused engine entries.
+ * If non-NULL, it is caller's responsibility to free.
+ */
+static struct eng_mmio_base_table_s *_gem_engine_mmio_base_info_get_legacy(int
fd_dev)
+{
+ int gen;
+ uint32_t mmio_base;
+ struct eng_mmio_base_table_s mbt;
+ struct eng_mmio_base_table_s *mbp;
+
+ memset(&mbt, 0, sizeof(mbt));
+
+ gen = intel_gen(intel_get_drm_devid(fd_dev));
+
+ /* The mmio_base values for engine instances 1 and higher cannot
+* be reliability determinated a priori. */
+
+ _gem_engine_mmio_info_legacy_add(&mbt, "rcs0", 0x2000);
+ _gem_engine_mmio_info_legacy_add(&mbt, "bcs0", 0x22000);
+
+ if (gen < 6)
+ mmio_base = 0x4000;
+ else if (gen < 11)
+ mmio_base = 0x12000;
+ else
+ mmio_base = 0x1c;
+ _gem_engine_mmio_info_legacy_add(&mbt, "vcs0", mmio_base);
+
+ if (gen < 11)
+ mmio_base = 0x1a000;
+ else
+ mmio_base = 0x1c8000;
+ _gem_engine_mmio_info_legacy_add(&mbt, "vecs0", mmio_base);
+
+ if (mbt.mb_cnt <= 0)
+ return NULL;
+
+ mbp = _gem_engine_mmio_info_dup(&mbt);
+
+ return mbp;
+}
+
+
+/**
+ * _gem_engine_mmio_base_info_get_debugfs:
+ * @fd_dev: file descriptor upon which device is open or -1 to use defaults.
+ *
+ * Obtains per-engine mmio_base information from debugfs.
+ *
+ * Returns:
+ * Pointer to dynamically allocated struct eng_mmio_base_table_s describing
+ * engine config or NULL.
+ * The allocated size does not include unused engine entries.
+ * If non-NULL, it is caller's responsibility to free.
+ *
+ * Looking in debugfs for per-engine instances of:
+ *
+ * ...
+ * MMIO base:
+ *
+ * Example of relevant lines from debugfs:
+ * vcs0
+ * MMIO base: 0x001c
+ * vcs1
+ * MMIO base: 0x001d
+ *
+ * In order to qualify as the introduction of a new per-engine section, an
+ * input line must consist solely of an engine name. An engine name must
+ * be 7 or fewer characters in length and must consist of an engine class
+ * name of 3 or more lower case characters f
Re: [Intel-gfx] [PATCH] drm/i915/huc: Fix error reported by I915_PARAM_HUC_STATUS
On 2/11/20 11:57 AM, Michal Wajdeczko wrote:
On Tue, 11 Feb 2020 18:53:05 +0100, Fosha, Robert M
wrote:
On 2/4/20 4:43 PM, Ye, Tony wrote:
On 1/27/2020 1:41 AM, Michal Wajdeczko wrote:
On Thu, 23 Jan 2020 16:51:58 +0100, Chris Wilson
wrote:
Quoting Michal Wajdeczko (2020-01-23 15:38:52)
On Thu, 23 Jan 2020 16:02:17 +0100, Chris Wilson
wrote:
> Quoting Daniele Ceraolo Spurio (2020-01-22 23:52:33)
>>
>>
>> On 1/22/20 11:48 AM, Michal Wajdeczko wrote:
>> > From commit 84b1ca2f0e68 ("drm/i915/uc: prefer intel_gt
over i915
>> > in GuC/HuC paths") we stopped using HUC_STATUS error -ENODEV
only
>> > to indicate lack of HuC hardware and we started to use this
error
>> > also for all other cases when HuC was not in use or supported.
>> >
>> > Fix that by relying again on HAS_GT_UC macro, since currently
>> > used function intel_huc_is_supported() is based on HuC firmware
>> > support which could be unsupported also due to force disabled
>> > GuC firmware.
>> >
>> > Signed-off-by: Michal Wajdeczko
>> > Cc: Daniele Ceraolo Spurio
>> > Cc: Michal Wajdeczko
>> > Cc: Tony Ye
>>
>> Reviewed-by: Daniele Ceraolo Spurio
>
> Once upon a time did you (Michal) not argue we should indicate
the lack
> of firmware in the error code? Something like
>
> if (!HAS_GT_UC(gt->i915))
> return -ENODEV;
>
> if (!intel_huc_is_supported(huc))
> return -ENOEXEC;
Yes, we discussed this here [1] together with [2] but we didn't
conclude our discussion due to different opinions on how represent
some states, in particular "manually disabled" state.
In this patch I just wanted to restore old notation.
But we can start new discussion, here is summary:
--+--+--+--
HuC state | today* | option A | option B
--+--+--+--
no HuC hardware | -ENODEV | -ENODEV | -ENODEV
GuC fw disabled | 0 | 0 | -EOPNOTSUPP
HuC fw disabled | 0 | 0 | -EOPNOTSUPP
HuC fw missing | 0 | -ENOPKG | -ENOEXEC
HuC fw error | 0 | -ENOEXEC | -ENOEXEC
HuC fw fail | 0 | -EACCES | 0
HuC authenticated | 1 | 1 | 1
--+--+--+--
By fw fail, you mean we loaded the firmware (to our knowledge)
correctly, but HUC_STATUS is not reported as valid?
If so, I support option B. I like the idea of saying
"no HuC" (machine too old)
"no firmware" (user action, or lack thereof)
0 (fw unhappy)
1 (fw reports success)
In between states for failures in fw loading? Not so sure. But I
can see
the nicety in distinguishing between lack of firmware and some random
failure in loading the firmware (the former being user action
required
to rectify, command line parameter whatever and the latter being the
firmware file is either invalid or a stray neutrino prevented
loading).
Imo the error messages should be about why we cannot probe/trust the
HUC_STATUS register. If everything is setup correctly then the
returned
value should be from reading the register. I dislike only
returning 1 if
supported, and converting a valid read of 0 into another error.
So Option B :)
But I'm not sure that option B is consistent in error reporting, as
"fw unhappy" is definitely an serious error but is represented as
plain
non-error "0" status, while "fw disabled" (user action) is treated
as error
--+--
HuC state | option B
--+--
no HuC hardware | -ENODEV
GuC fw disabled | -EOPNOTSUPP -> user decision, why error?
HuC fw disabled | -EOPNOTSUPP -> user decision, why error?
HuC fw missing | -ENOEXEC
HuC fw error | -ENOEXEC
HuC fw fail | 0 -> unlikely, but still fw/hw error
HuC authenticated | 1
--+--
On other hand, option A treats all error conditions as errors, leaving
status codes only for normal operations: disabled(0)/authenticated(1):
--+--
HuC state | option A
--+--
no HuC hardware | -ENODEV -> you shouldn't ask
GuC fw disabled | 0 -> user decision, not an error
HuC fw disabled | 0 -> user decision, not an error
HuC fw missing | -ENOPKG -> fw not installed correctly
HuC fw error | -ENOEXEC -> bad/wrong fw
HuC fw fail | -EACCES -> fw/hw error
HuC authenticated | 1
--+--
Vote for Option A.
Regards,
Tony
Are we ok to move forward on this? Michal, are you working on
updating the patch?
I can update the patch, but I don't know which option to implement:
Tony votes for Option A, Chris for Option B, what's y
Re: [Intel-gfx] [PATCH] drm/i915/huc: Fix error reported by I915_PARAM_HUC_STATUS
On 2/4/20 4:43 PM, Ye, Tony wrote:
On 1/27/2020 1:41 AM, Michal Wajdeczko wrote:
On Thu, 23 Jan 2020 16:51:58 +0100, Chris Wilson
wrote:
Quoting Michal Wajdeczko (2020-01-23 15:38:52)
On Thu, 23 Jan 2020 16:02:17 +0100, Chris Wilson
wrote:
> Quoting Daniele Ceraolo Spurio (2020-01-22 23:52:33)
>>
>>
>> On 1/22/20 11:48 AM, Michal Wajdeczko wrote:
>> > From commit 84b1ca2f0e68 ("drm/i915/uc: prefer intel_gt over
i915
>> > in GuC/HuC paths") we stopped using HUC_STATUS error -ENODEV only
>> > to indicate lack of HuC hardware and we started to use this error
>> > also for all other cases when HuC was not in use or supported.
>> >
>> > Fix that by relying again on HAS_GT_UC macro, since currently
>> > used function intel_huc_is_supported() is based on HuC firmware
>> > support which could be unsupported also due to force disabled
>> > GuC firmware.
>> >
>> > Signed-off-by: Michal Wajdeczko
>> > Cc: Daniele Ceraolo Spurio
>> > Cc: Michal Wajdeczko
>> > Cc: Tony Ye
>>
>> Reviewed-by: Daniele Ceraolo Spurio
>
> Once upon a time did you (Michal) not argue we should indicate
the lack
> of firmware in the error code? Something like
>
> if (!HAS_GT_UC(gt->i915))
> return -ENODEV;
>
> if (!intel_huc_is_supported(huc))
> return -ENOEXEC;
Yes, we discussed this here [1] together with [2] but we didn't
conclude our discussion due to different opinions on how represent
some states, in particular "manually disabled" state.
In this patch I just wanted to restore old notation.
But we can start new discussion, here is summary:
--+--+--+--
HuC state | today* | option A | option B
--+--+--+--
no HuC hardware | -ENODEV | -ENODEV | -ENODEV
GuC fw disabled | 0 | 0 | -EOPNOTSUPP
HuC fw disabled | 0 | 0 | -EOPNOTSUPP
HuC fw missing | 0 | -ENOPKG | -ENOEXEC
HuC fw error | 0 | -ENOEXEC | -ENOEXEC
HuC fw fail | 0 | -EACCES | 0
HuC authenticated | 1 | 1 | 1
--+--+--+--
By fw fail, you mean we loaded the firmware (to our knowledge)
correctly, but HUC_STATUS is not reported as valid?
If so, I support option B. I like the idea of saying
"no HuC" (machine too old)
"no firmware" (user action, or lack thereof)
0 (fw unhappy)
1 (fw reports success)
In between states for failures in fw loading? Not so sure. But I can
see
the nicety in distinguishing between lack of firmware and some random
failure in loading the firmware (the former being user action required
to rectify, command line parameter whatever and the latter being the
firmware file is either invalid or a stray neutrino prevented loading).
Imo the error messages should be about why we cannot probe/trust the
HUC_STATUS register. If everything is setup correctly then the returned
value should be from reading the register. I dislike only returning
1 if
supported, and converting a valid read of 0 into another error.
So Option B :)
But I'm not sure that option B is consistent in error reporting, as
"fw unhappy" is definitely an serious error but is represented as plain
non-error "0" status, while "fw disabled" (user action) is treated as
error
--+--
HuC state | option B
--+--
no HuC hardware | -ENODEV
GuC fw disabled | -EOPNOTSUPP -> user decision, why error?
HuC fw disabled | -EOPNOTSUPP -> user decision, why error?
HuC fw missing | -ENOEXEC
HuC fw error | -ENOEXEC
HuC fw fail | 0 -> unlikely, but still fw/hw error
HuC authenticated | 1
--+--
On other hand, option A treats all error conditions as errors, leaving
status codes only for normal operations: disabled(0)/authenticated(1):
--+--
HuC state | option A
--+--
no HuC hardware | -ENODEV -> you shouldn't ask
GuC fw disabled | 0 -> user decision, not an error
HuC fw disabled | 0 -> user decision, not an error
HuC fw missing | -ENOPKG -> fw not installed correctly
HuC fw error | -ENOEXEC -> bad/wrong fw
HuC fw fail | -EACCES -> fw/hw error
HuC authenticated | 1
--+--
Vote for Option A.
Regards,
Tony
Are we ok to move forward on this? Michal, are you working on updating
the patch?
-Rob
But since I'm not an active HuC user, will leave final decision to
others.
/Michal
Note that all above should be compatible with media driver,
which explicitly looks for no error and value 1
Cool.
-Chris
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [igt-dev] [PATCH i-g-t] i915/perf: Find the associated perf-type for a particular device
On 1/7/20 2:32 AM, Chris Wilson wrote:
Quoting Tvrtko Ursulin (2020-01-07 09:53:39)
+Arek, Saurabhg
On 05/01/2020 01:06, Chris Wilson wrote:
Since with multiple devices, we may have multiple different perf_pmu
each with their own type, we want to find the right one for the job.
The tests are run with a specific fd, from which we can extract the
appropriate bus-id and find the associated perf-type. The performance
monitoring tools are a little more general and not yet ready to probe
all device or bind to one in particular, so we just assume the default
igfx for the time being.
v2: Extract the bus address from out of sysfs
Signed-off-by: Chris Wilson
Cc: "Robert M. Fosha"
Cc: Tvrtko Ursulin
Cc: Michal Wajdeczko
Tested-by: Robert M. Fosha
---
benchmarks/gem_wsim.c | 4 +-
lib/igt_perf.c | 84 +++---
lib/igt_perf.h | 13 --
overlay/gem-interrupts.c | 2 +-
overlay/gpu-freq.c | 4 +-
overlay/gpu-top.c | 12 ++---
overlay/rc6.c | 2 +-
tests/i915/gem_ctx_freq.c | 2 +-
tests/i915/gem_ctx_sseu.c | 2 +-
tests/i915/gem_exec_balancer.c | 18 +---
tests/perf_pmu.c | 84 ++
tools/intel_gpu_top.c | 2 +-
12 files changed, 159 insertions(+), 70 deletions(-)
diff --git a/benchmarks/gem_wsim.c b/benchmarks/gem_wsim.c
index 6305e0d7a..9156fdc90 100644
--- a/benchmarks/gem_wsim.c
+++ b/benchmarks/gem_wsim.c
@@ -2268,8 +2268,8 @@ busy_init(const struct workload_balancer *balancer,
struct workload *wrk)
for (d = &engines[0]; d->id != VCS; d++) {
int pfd;
- pfd = perf_i915_open_group(I915_PMU_ENGINE_BUSY(d->class,
- d->inst),
+ pfd = perf_igfx_open_group(I915_PMU_ENGINE_BUSY(d->class,
+ d->inst),
bb->fd);
if (pfd < 0) {
if (d->id != VCS2)
diff --git a/lib/igt_perf.c b/lib/igt_perf.c
index e3dec2cc2..840add043 100644
--- a/lib/igt_perf.c
+++ b/lib/igt_perf.c
@@ -4,17 +4,77 @@
#include
#include
#include
+#include
#include
+#include
#include "igt_perf.h"
-uint64_t i915_type_id(void)
+static char *bus_address(int i915, char *path, int pathlen)
+{
+ struct stat st;
+ int len = -1;
+ int dir;
+ char *s;
+
+ if (fstat(i915, &st) || !S_ISCHR(st.st_mode))
+ return NULL;
+
+ snprintf(path, pathlen, "/sys/dev/char/%d:%d",
+ major(st.st_rdev), minor(st.st_rdev));
+
+ dir = open(path, O_RDONLY);
+ if (dir != -1) {
+ len = readlinkat(dir, "device", path, pathlen - 1);
+ close(dir);
+ }
+ if (len < 0)
+ return NULL;
+
+ path[len] = '\0';
+
+ /* strip off the relative path */
+ s = strrchr(path, '/');
+ if (s)
+ memmove(path, s + 1, len - (s - path) + 1);
+
+ return path;
+}
+
+const char *i915_perf_device(int i915, char *buf, int buflen)
+{
+#define prefix "i915-"
+#define plen strlen(prefix)
+
+ if (!buf || buflen < plen)
+ return "i915";
+
+ memcpy(buf, prefix, plen);
+
+ if (!bus_address(i915, buf + plen, buflen - plen) ||
+ strcmp(buf + plen, ":00:02.0") == 0) /* legacy name for igfx */
+ buf[plen - 1] = '\0';
+
+ return buf;
+}
So DRM fd -> PCI string conversion, yes? On a glance it looks okay.
However Arek probably has this data as part of "[PATCH i-g-t 0/4] device
selection && lsgpu" (https://patchwork.freedesktop.org/series/70285/).
If the string is known, we can use it. This simple routine is *simple*
yet effective :)
Also:
https://gitlab.freedesktop.org/drm/igt-gpu-tools/issues/52
https://gitlab.freedesktop.org/drm/igt-gpu-tools/issues/51
How lightweight are they aiming to be?
And VLK-5588.
This patch is overlap with #52 and then #51/VLK-5588 is about allowing
card selection for tools.
How to meld the two with minimum effort? We could put this in and then
later replace the PCI name resolve with a library routine and re-adjust
tools to allow card selection via some mechanism.
Exactly. All we need here is a name to lookup the perf type id. One
routine to provide an introspection method for a given fd and assumption
of i915, does not prevent better methods :)
I do wonder though if we should have perf_name in our sysfs.
-Chris
Agree with idea of adding this change now and re-adjusting if other
mechanism is added for other tests/tools. If no other concerns from
Tvrtko or Arek
Reviewed-by: Robert M. Fosha
-Rob
___
igt-dev mailing list
igt-...@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev
___
Inte
Re: [Intel-gfx] [PATCH i-g-t] i915/perf: Find the associated perf-type for a particular device
On 1/3/20 9:41 AM, Chris Wilson wrote:
Since with multiple devices, we may have multiple different perf_pmu
each with their own type, we want to find the right one for the job.
The tests are run with a specific fd, from which we can extract the
appropriate bus-id and find the associated perf-type. The performance
monitoring tools are a little more general and not yet ready to probe
all device or bind to one in particular, so we just assume the default
igfx for the time being.
Signed-off-by: Chris Wilson
Cc: "Robert M. Fosha"
Cc: Tvrtko Ursulin
Cc: Michal Wajdeczko
---
benchmarks/gem_wsim.c | 4 +-
lib/igt_perf.c | 66 +++---
lib/igt_perf.h | 13 --
overlay/gem-interrupts.c | 2 +-
overlay/gpu-freq.c | 4 +-
overlay/gpu-top.c | 12 ++---
overlay/rc6.c | 2 +-
tests/i915/gem_ctx_freq.c | 2 +-
tests/i915/gem_ctx_sseu.c | 2 +-
tests/i915/gem_exec_balancer.c | 18 +---
tests/perf_pmu.c | 84 ++
tools/intel_gpu_top.c | 2 +-
12 files changed, 141 insertions(+), 70 deletions(-)
diff --git a/benchmarks/gem_wsim.c b/benchmarks/gem_wsim.c
index 6305e0d7a..9156fdc90 100644
--- a/benchmarks/gem_wsim.c
+++ b/benchmarks/gem_wsim.c
@@ -2268,8 +2268,8 @@ busy_init(const struct workload_balancer *balancer,
struct workload *wrk)
for (d = &engines[0]; d->id != VCS; d++) {
int pfd;
- pfd = perf_i915_open_group(I915_PMU_ENGINE_BUSY(d->class,
- d->inst),
+ pfd = perf_igfx_open_group(I915_PMU_ENGINE_BUSY(d->class,
+ d->inst),
bb->fd);
if (pfd < 0) {
if (d->id != VCS2)
diff --git a/lib/igt_perf.c b/lib/igt_perf.c
index e3dec2cc2..4922a2df7 100644
--- a/lib/igt_perf.c
+++ b/lib/igt_perf.c
@@ -4,17 +4,59 @@
#include
#include
#include
+#include
#include
#include "igt_perf.h"
-uint64_t i915_type_id(void)
+const char *i915_perf_device(int i915, char *buf, int buflen)
+{
+ drm_unique_t u = {
+ .unique = buf + 1,
+ .unique_len = buflen - 1,
+ };
+ drm_set_version_t sv = {
+ .drm_di_major = 1,
+ .drm_di_minor = 4,
+ .drm_dd_major = -1,/* Don't care */
+ .drm_dd_minor = -1,/* Don't care */
+ };
+
+ if (ioctl(i915, DRM_IOCTL_SET_VERSION, &sv))
+ return "i915";
perf_pmu has test cases that use drm_open_driver_render() and pass the
render_fd. What about this cases?
+
+ memset(buf, 0, buflen);
+ ioctl(i915, DRM_IOCTL_GET_UNIQUE, &u);
+
+ if (u.unique_len >= buflen)
+ return NULL;
+
+ if (strncmp(buf + 1, "pci:", 4))
+ return NULL;
+
+ if (strcmp(buf + 1, "pci::00:02.0") == 0)
+ return "i915";
+
+ return memcpy(buf, "i915-", strlen("i915-"));
+}
+
+uint64_t i915_perf_type_id(int i915)
+{
+ char buf[80];
+
+ return igt_perf_type_id(i915_perf_device(i915, buf, sizeof(buf)));
+}
+
+uint64_t igt_perf_type_id(const char *device)
{
char buf[64];
ssize_t ret;
int fd;
- fd = open("/sys/bus/event_source/devices/i915/type", O_RDONLY);
+ snprintf(buf, sizeof(buf),
+"/sys/bus/event_source/devices/%s/type", device);
+
+ fd = open(buf, O_RDONLY);
if (fd < 0)
return 0;
@@ -52,15 +94,27 @@ _perf_open(uint64_t type, uint64_t config, int group, uint64_t format)
return ret;
}
-int perf_i915_open(uint64_t config)
+int perf_igfx_open(uint64_t config)
+{
+ return _perf_open(igt_perf_type_id("i915"), config, -1,
+ PERF_FORMAT_TOTAL_TIME_ENABLED);
+}
+
+int perf_igfx_open_group(uint64_t config, int group)
+{
+ return _perf_open(igt_perf_type_id("i915"), config, group,
+ PERF_FORMAT_TOTAL_TIME_ENABLED | PERF_FORMAT_GROUP);
+}
+
+int perf_i915_open(int i915, uint64_t config)
{
- return _perf_open(i915_type_id(), config, -1,
+ return _perf_open(i915_perf_type_id(i915), config, -1,
PERF_FORMAT_TOTAL_TIME_ENABLED);
}
-int perf_i915_open_group(uint64_t config, int group)
+int perf_i915_open_group(int i915, uint64_t config, int group)
{
- return _perf_open(i915_type_id(), config, group,
+ return _perf_open(i915_perf_type_id(i915), config, group,
PERF_FORMAT_TOTAL_TIME_ENABLED | PERF_FORMAT_GROUP);
}
diff --git a/lib/igt_perf.h b/lib/igt_perf.h
index e00718f47..a8328c70c 100644
--- a/lib/igt_perf.h
+++ b/lib/igt_perf.h
@@ -51,10 +51,17 @@ perf_event_open(struct perf_event_attr *attr,
return syscall(__N
Re: [Intel-gfx] ✗ Fi.CI.IGT: failure for drm/i915/guc: Update H2G enable logging action definition
On 9/28/19 3:36 AM, Patchwork wrote: == Series Details == Series: drm/i915/guc: Update H2G enable logging action definition URL : https://patchwork.freedesktop.org/series/67351/ State : failure == Summary == CI Bug Log - changes from CI_DRM_6970_full -> Patchwork_14570_full Summary --- **FAILURE** Serious unknown changes coming with Patchwork_14570_full absolutely need to be verified manually. If you think the reported changes have nothing to do with the changes introduced in Patchwork_14570_full, please notify your bug team to allow them to document this new failure mode, which will reduce false positives in CI. Possible new issues --- Here are the unknown changes that may have been introduced in Patchwork_14570_full: ### IGT changes ### Possible regressions * igt@i915_pm_rpm@system-suspend-execbuf: - shard-iclb: [PASS][1] -> [DMESG-WARN][2] [1]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6970/shard-iclb8/igt@i915_pm_...@system-suspend-execbuf.html [2]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14570/shard-iclb1/igt@i915_pm_...@system-suspend-execbuf.html Dmesg warnings are for thunderbolt and do not look to be related to this patch. It looks to be related to fdo#111764. I think patch should be safe to merge. New tests - New tests have been introduced between CI_DRM_6970_full and Patchwork_14570_full: ### New Piglit tests (7) ### * spec@arb_gpu_shader5@texturegather@vs-rgba-2-float-2darray: - Statuses : 1 incomplete(s) - Exec time: [0.0] s * spec@arb_gpu_shader5@texturegather@vs-rgba-3-float-2darray: - Statuses : 1 incomplete(s) - Exec time: [0.0] s * spec@arb_gpu_shader5@texturegatheroffset@vs-rgba-0-float-2darray: - Statuses : 1 incomplete(s) - Exec time: [0.0] s * spec@arb_gpu_shader5@texturegatheroffsets@vs-rgba-0-float-2d: - Statuses : 1 incomplete(s) - Exec time: [0.0] s * spec@arb_gpu_shader5@texturegatheroffsets@vs-rgba-1-float-2d: - Statuses : 1 incomplete(s) - Exec time: [0.0] s * spec@arb_gpu_shader5@texturegatheroffsets@vs-rgba-2-float-2d: - Statuses : 1 incomplete(s) - Exec time: [0.0] s * spec@arb_gpu_shader5@texturegatheroffsets@vs-rgba-3-float-2d: - Statuses : 1 incomplete(s) - Exec time: [0.0] s Known issues Here are the changes found in Patchwork_14570_full that come from known issues: ### IGT changes ### Issues hit * igt@gem_exec_async@concurrent-writes-bsd: - shard-iclb: [PASS][3] -> [SKIP][4] ([fdo#111325]) +3 similar issues [3]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6970/shard-iclb7/igt@gem_exec_as...@concurrent-writes-bsd.html [4]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14570/shard-iclb1/igt@gem_exec_as...@concurrent-writes-bsd.html * igt@gem_exec_schedule@preempt-queue-bsd1: - shard-iclb: [PASS][5] -> [SKIP][6] ([fdo#109276]) +17 similar issues [5]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6970/shard-iclb2/igt@gem_exec_sched...@preempt-queue-bsd1.html [6]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14570/shard-iclb3/igt@gem_exec_sched...@preempt-queue-bsd1.html * igt@i915_pm_rpm@cursor-dpms: - shard-iclb: [PASS][7] -> [INCOMPLETE][8] ([fdo#107713] / [fdo#108840]) [7]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6970/shard-iclb7/igt@i915_pm_...@cursor-dpms.html [8]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14570/shard-iclb7/igt@i915_pm_...@cursor-dpms.html * igt@kms_busy@basic-modeset-a: - shard-iclb: [PASS][9] -> [INCOMPLETE][10] ([fdo#107713]) [9]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6970/shard-iclb4/igt@kms_b...@basic-modeset-a.html [10]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14570/shard-iclb1/igt@kms_b...@basic-modeset-a.html * igt@kms_cursor_legacy@cursor-vs-flip-varying-size: - shard-apl: [PASS][11] -> [INCOMPLETE][12] ([fdo#103927]) +3 similar issues [11]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6970/shard-apl7/igt@kms_cursor_leg...@cursor-vs-flip-varying-size.html [12]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14570/shard-apl1/igt@kms_cursor_leg...@cursor-vs-flip-varying-size.html * igt@kms_flip@flip-vs-suspend-interruptible: - shard-hsw: [PASS][13] -> [INCOMPLETE][14] ([fdo#103540]) [13]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6970/shard-hsw1/igt@kms_f...@flip-vs-suspend-interruptible.html [14]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14570/shard-hsw1/igt@kms_f...@flip-vs-suspend-interruptible.html * igt@kms_flip@plain-flip-fb-recreate-interruptible: - shard-skl: [PASS][15] -> [FAIL][16] ([fdo#100368]) [15]: https://intel-gfx-ci.01.org/tree/drm-ti
Re: [Intel-gfx] ✗ Fi.CI.IGT: failure for drm/i915/guc: Update H2G enable logging action definition
On 9/28/19 3:36 AM, Patchwork wrote: == Series Details == Series: drm/i915/guc: Update H2G enable logging action definition URL : https://patchwork.freedesktop.org/series/67351/ State : failure == Summary == CI Bug Log - changes from CI_DRM_6970_full -> Patchwork_14570_full Summary --- **FAILURE** Serious unknown changes coming with Patchwork_14570_full absolutely need to be verified manually. If you think the reported changes have nothing to do with the changes introduced in Patchwork_14570_full, please notify your bug team to allow them to document this new failure mode, which will reduce false positives in CI. Possible new issues --- Here are the unknown changes that may have been introduced in Patchwork_14570_full: ### IGT changes ### Possible regressions * igt@i915_pm_rpm@system-suspend-execbuf: - shard-iclb: [PASS][1] -> [DMESG-WARN][2] [1]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6970/shard-iclb8/igt@i915_pm_...@system-suspend-execbuf.html [2]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14570/shard-iclb1/igt@i915_pm_...@system-suspend-execbuf.html Dmesg warnings are for thunderbolt and do not look to be related to this patch. It looks to be related to fdo#111764. I think patch should be safe to merge. New tests - New tests have been introduced between CI_DRM_6970_full and Patchwork_14570_full: ### New Piglit tests (7) ### * spec@arb_gpu_shader5@texturegather@vs-rgba-2-float-2darray: - Statuses : 1 incomplete(s) - Exec time: [0.0] s * spec@arb_gpu_shader5@texturegather@vs-rgba-3-float-2darray: - Statuses : 1 incomplete(s) - Exec time: [0.0] s * spec@arb_gpu_shader5@texturegatheroffset@vs-rgba-0-float-2darray: - Statuses : 1 incomplete(s) - Exec time: [0.0] s * spec@arb_gpu_shader5@texturegatheroffsets@vs-rgba-0-float-2d: - Statuses : 1 incomplete(s) - Exec time: [0.0] s * spec@arb_gpu_shader5@texturegatheroffsets@vs-rgba-1-float-2d: - Statuses : 1 incomplete(s) - Exec time: [0.0] s * spec@arb_gpu_shader5@texturegatheroffsets@vs-rgba-2-float-2d: - Statuses : 1 incomplete(s) - Exec time: [0.0] s * spec@arb_gpu_shader5@texturegatheroffsets@vs-rgba-3-float-2d: - Statuses : 1 incomplete(s) - Exec time: [0.0] s Known issues Here are the changes found in Patchwork_14570_full that come from known issues: ### IGT changes ### Issues hit * igt@gem_exec_async@concurrent-writes-bsd: - shard-iclb: [PASS][3] -> [SKIP][4] ([fdo#111325]) +3 similar issues [3]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6970/shard-iclb7/igt@gem_exec_as...@concurrent-writes-bsd.html [4]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14570/shard-iclb1/igt@gem_exec_as...@concurrent-writes-bsd.html * igt@gem_exec_schedule@preempt-queue-bsd1: - shard-iclb: [PASS][5] -> [SKIP][6] ([fdo#109276]) +17 similar issues [5]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6970/shard-iclb2/igt@gem_exec_sched...@preempt-queue-bsd1.html [6]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14570/shard-iclb3/igt@gem_exec_sched...@preempt-queue-bsd1.html * igt@i915_pm_rpm@cursor-dpms: - shard-iclb: [PASS][7] -> [INCOMPLETE][8] ([fdo#107713] / [fdo#108840]) [7]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6970/shard-iclb7/igt@i915_pm_...@cursor-dpms.html [8]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14570/shard-iclb7/igt@i915_pm_...@cursor-dpms.html * igt@kms_busy@basic-modeset-a: - shard-iclb: [PASS][9] -> [INCOMPLETE][10] ([fdo#107713]) [9]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6970/shard-iclb4/igt@kms_b...@basic-modeset-a.html [10]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14570/shard-iclb1/igt@kms_b...@basic-modeset-a.html * igt@kms_cursor_legacy@cursor-vs-flip-varying-size: - shard-apl: [PASS][11] -> [INCOMPLETE][12] ([fdo#103927]) +3 similar issues [11]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6970/shard-apl7/igt@kms_cursor_leg...@cursor-vs-flip-varying-size.html [12]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14570/shard-apl1/igt@kms_cursor_leg...@cursor-vs-flip-varying-size.html * igt@kms_flip@flip-vs-suspend-interruptible: - shard-hsw: [PASS][13] -> [INCOMPLETE][14] ([fdo#103540]) [13]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6970/shard-hsw1/igt@kms_f...@flip-vs-suspend-interruptible.html [14]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14570/shard-hsw1/igt@kms_f...@flip-vs-suspend-interruptible.html * igt@kms_flip@plain-flip-fb-recreate-interruptible: - shard-skl: [PASS][15] -> [FAIL][16] ([fdo#100368]) [15]: https://intel-gfx-ci.01.org/tree/drm-tip/
Re: [Intel-gfx] ✗ Fi.CI.IGT: failure for series starting with [CI,1/2] drm/i915/guc: Enable guc logging on guc log relay write
On 9/21/19 4:00 PM, Patchwork wrote:
== Series Details ==
Series: series starting with [CI,1/2] drm/i915/guc: Enable guc logging on guc
log relay write
URL : https://patchwork.freedesktop.org/series/67009/
State : failure
== Summary ==
CI Bug Log - changes from CI_DRM_6929_full -> Patchwork_14480_full
Summary
---
**FAILURE**
Serious unknown changes coming with Patchwork_14480_full absolutely need to
be
verified manually.
If you think the reported changes have nothing to do with the changes
introduced in Patchwork_14480_full, please notify your bug team to allow them
to document this new failure mode, which will reduce false positives in CI.
Possible new issues
---
Here are the unknown changes that may have been introduced in
Patchwork_14480_full:
### IGT changes ###
Possible regressions
* igt@i915_pm_rc6_residency@rc6-accuracy:
- shard-iclb: [PASS][1] -> [SKIP][2]
[1]:
https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6929/shard-iclb8/igt@i915_pm_rc6_reside...@rc6-accuracy.html
[2]:
https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14480/shard-iclb3/igt@i915_pm_rc6_reside...@rc6-accuracy.html
Have looked and seen that this behavior (test is skipped) has occurred
for other patches.
Appears to be sporadic and unrelated to this patch, so I think this
patch should be safe to merge.
Suppressed
The following results come from untrusted machines, tests, or statuses.
They do not affect the overall result.
* {igt@i915_pm_dc@dc6-dpms}:
- shard-iclb: [PASS][3] -> [FAIL][4]
[3]:
https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6929/shard-iclb2/igt@i915_pm...@dc6-dpms.html
[4]:
https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14480/shard-iclb3/igt@i915_pm...@dc6-dpms.html
Known issues
Here are the changes found in Patchwork_14480_full that come from known
issues:
### IGT changes ###
Issues hit
* igt@gem_ctx_isolation@bcs0-s3:
- shard-apl: [PASS][5] -> [DMESG-WARN][6] ([fdo#108566]) +8
similar issues
[5]:
https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6929/shard-apl2/igt@gem_ctx_isolat...@bcs0-s3.html
[6]:
https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14480/shard-apl6/igt@gem_ctx_isolat...@bcs0-s3.html
* igt@gem_eio@in-flight-contexts-immediate:
- shard-hsw: [PASS][7] -> [FAIL][8] ([fdo#105957])
[7]:
https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6929/shard-hsw5/igt@gem_...@in-flight-contexts-immediate.html
[8]:
https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14480/shard-hsw1/igt@gem_...@in-flight-contexts-immediate.html
* igt@gem_exec_balancer@smoke:
- shard-iclb: [PASS][9] -> [SKIP][10] ([fdo#110854])
[9]:
https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6929/shard-iclb1/igt@gem_exec_balan...@smoke.html
[10]:
https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14480/shard-iclb3/igt@gem_exec_balan...@smoke.html
* igt@gem_exec_schedule@wide-bsd:
- shard-iclb: [PASS][11] -> [SKIP][12] ([fdo#111325]) +6 similar
issues
[11]:
https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6929/shard-iclb6/igt@gem_exec_sched...@wide-bsd.html
[12]:
https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14480/shard-iclb2/igt@gem_exec_sched...@wide-bsd.html
* igt@i915_pm_rc6_residency@rc6-accuracy:
- shard-apl: [PASS][13] -> [SKIP][14] ([fdo#109271])
[13]:
https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6929/shard-apl2/igt@i915_pm_rc6_reside...@rc6-accuracy.html
[14]:
https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14480/shard-apl6/igt@i915_pm_rc6_reside...@rc6-accuracy.html
- shard-glk: [PASS][15] -> [SKIP][16] ([fdo#109271])
[15]:
https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6929/shard-glk1/igt@i915_pm_rc6_reside...@rc6-accuracy.html
[16]:
https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14480/shard-glk1/igt@i915_pm_rc6_reside...@rc6-accuracy.html
- shard-kbl: [PASS][17] -> [SKIP][18] ([fdo#109271])
[17]:
https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6929/shard-kbl7/igt@i915_pm_rc6_reside...@rc6-accuracy.html
[18]:
https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14480/shard-kbl1/igt@i915_pm_rc6_reside...@rc6-accuracy.html
- shard-skl: [PASS][19] -> [SKIP][20] ([fdo#109271])
[19]:
https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6929/shard-skl8/igt@i915_pm_rc6_reside...@rc6-accuracy.html
[20]:
https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14480/shard-skl5/igt@i915_pm_rc6_reside...@rc6-accuracy.html
* igt@i915_suspend@fence-restore-untiled:
- shard-kbl: [PASS][21] -> [DMESG-WARN][22] ([fdo#103313])
[21]:
https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6929/shard-kbl7/igt@i915_susp...@fence-restore-untiled.html
[22]:
https://intel-gfx-ci.01.or
Re: [Intel-gfx] [RFC] drm/i915/guc: Enable guc logging on guc log relay write
On 9/10/19 5:48 PM, Daniele Ceraolo Spurio wrote:
On 9/10/19 3:46 PM, Robert M. Fosha wrote:
Creating and opening the GuC log relay file enables and starts
the relay potentially before the caller is ready to consume logs.
Change the behavior so that relay starts only on an explicit call
to the write function (with a value of '1'). Other values flush
the log relay as before.
Cc: Matthew Brost
Cc: Daniele Ceraolo Spurio
Cc: Michal Wajdeczko
Signed-off-by: Robert M. Fosha
---
drivers/gpu/drm/i915/gt/uc/intel_guc_log.c | 38 +-
drivers/gpu/drm/i915/gt/uc/intel_guc_log.h | 2 ++
drivers/gpu/drm/i915/i915_debugfs.c | 27 +--
3 files changed, 56 insertions(+), 11 deletions(-)
diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_log.c
b/drivers/gpu/drm/i915/gt/uc/intel_guc_log.c
index 36332064de9c..9a98270d05b6 100644
--- a/drivers/gpu/drm/i915/gt/uc/intel_guc_log.c
+++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_log.c
@@ -361,6 +361,7 @@ void intel_guc_log_init_early(struct
intel_guc_log *log)
{
mutex_init(&log->relay.lock);
INIT_WORK(&log->relay.flush_work, capture_logs_work);
+ log->relay_started = false;
}
static int guc_log_relay_create(struct intel_guc_log *log)
@@ -585,15 +586,6 @@ int intel_guc_log_relay_open(struct
intel_guc_log *log)
mutex_unlock(&log->relay.lock);
- guc_log_enable_flush_events(log);
-
- /*
- * When GuC is logging without us relaying to userspace, we're
ignoring
- * the flush notification. This means that we need to
unconditionally
- * flush on relay enabling, since GuC only notifies us once.
- */
- queue_work(system_highpri_wq, &log->relay.flush_work);
-
return 0;
out_relay:
@@ -604,12 +596,38 @@ int intel_guc_log_relay_open(struct
intel_guc_log *log)
return ret;
}
+int intel_guc_log_relay_start(struct intel_guc_log *log)
+{
+ int ret = 0;
+
+ if (log->relay_started) {
+ ret = -EEXIST;
+ } else {
style: for this kind of checks, we usually just return early instead
of using an if-else, i.e.:
if (log->relay_started)
return -EEXIST;
[...] /* code */
return 0;
+ guc_log_enable_flush_events(log);
+
+ /*
+ * When GuC is logging without us relaying to userspace, we're
+ * ignoring the flush notification. This means that we need to
+ * unconditionally * flush on relay enabling, since GuC only
stray "*"
+ * notifies us once.
+ */
+ queue_work(system_highpri_wq, &log->relay.flush_work);
+
+ log->relay_started = false;
s/false/true/
+ }
+
+ return ret;
+}
+
void intel_guc_log_relay_flush(struct intel_guc_log *log)
{
struct intel_guc *guc = log_to_guc(log);
struct drm_i915_private *i915 = guc_to_gt(guc)->i915;
intel_wakeref_t wakeref;
+ if (!log->relay_started)
+ return;
+
/*
* Before initiating the forceful flush, wait for any
pending/ongoing
* flush to complete otherwise forceful flush may not actually
happen.
@@ -638,6 +656,8 @@ void intel_guc_log_relay_close(struct
intel_guc_log *log)
guc_log_unmap(log);
guc_log_relay_destroy(log);
mutex_unlock(&log->relay.lock);
+
+ log->relay_started = false;
For symmetry, it might be worth adding a guc_log_relay_stop:
Should it be intel_guc_log_relay_stop to be consistent with naming of
other functions?
static void guc_log_relay_stop(...)
{
if (!log->relay_started)
return;
guc_log_disable_flush_events(log);
intel_synchronize_irq(i915);
flush_work(&log->relay.flush_work);
log->relay_started = false;
}
and call it from intel_guc_log_relay_close().
Also, it should be impossible to race the start/flush with the stop
because the relay_write can't race the relay_close, but it might be
worth a comment in case we decide to have guc_log_relay_stop()
callable from the debugfs in the future.
How about this comment just above the relay_stop function:
/*
* Stops the relay log. Called from intel_guc_log_relay_close(), so no
* possibility of race with start/flush since relay_write cannot race
* relay_close.
*/
}
void intel_guc_log_handle_flush_event(struct intel_guc_log *log)
diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_log.h
b/drivers/gpu/drm/i915/gt/uc/intel_guc_log.h
index 6f764879acb1..ecf7a49416b4 100644
--- a/drivers/gpu/drm/i915/gt/uc/intel_guc_log.h
+++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_log.h
@@ -44,6 +44,7 @@ struct intel_guc;
struct intel_guc_log {
u32 level;
+ bool relay_started;
this should move inside the relay structure below. Just "started" will
be enough as a name at that point because the structure is already
called relay.
struct i915_vma *vma;
struct {
void *buf_addr;
@@ -67,6 +68,7 @@ void intel_guc_log_destroy(struct intel_guc_log *log);
int intel_guc_log_set_level(struct intel_guc_log *log, u32 le
