From: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
To: John.C.Harrison@Intel.com, Intel-GFX@Lists.FreeDesktop.Org
Subject: Re: [PATCH] drm/i915: Implement read-only support in whitelist selftest
Date: Tue, 25 Jun 2019 09:33:00 +0100 [thread overview]
Message-ID: <d1af8bf7-246b-a2e0-e719-1a282fb1341f@linux.intel.com> (raw)
In-Reply-To: <e46beed0-0851-b57f-f092-4f0c8bf064a7@linux.intel.com>
Ping.
We agreed to follow up with a test ASAP after merging.
Here's another feature request for you: Add engine->name logging to
wa_init_start in intel_engine_init_whitelist. Because with the change to
add whitelist on other engines there are now multiple identical lines in
dmesg.
To sum up that's three todo items:
1. Resend the selftest for CI.
2. Add GEM_BUG_ON for reg->flags checking invalid flag usage.
3. Improve dmesg so we know which engine got how many whitelist entries.
Thanks,
Tvrtko
On 20/06/2019 16:43, Tvrtko Ursulin wrote:
>
> Hi,
>
> You will need to send this not as reply to this thread so it is picked
> up by CI and then can be merged.
>
> But please also add a patch which adds that GEM_BUG_ON reg->flags check
> we talked about.
>
> Regards,
>
> Tvrtko
>
> On 18/06/2019 20:54, John.C.Harrison@Intel.com wrote:
>> From: John Harrison <John.C.Harrison@Intel.com>
>>
>> Newer hardware supports extra feature in the whitelist registers. This
>> patch updates the selftest to test that entries marked as read only
>> are actually read only.
>>
>> Also updated the read/write access definitions to make it clearer that
>> they are an enum field not a set of single bit flags.
>>
>> Signed-off-by: John Harrison <John.C.Harrison@Intel.com>
>> CC: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>> ---
>> drivers/gpu/drm/i915/gt/intel_workarounds.c | 8 +--
>> .../gpu/drm/i915/gt/selftest_workarounds.c | 53 +++++++++++++------
>> drivers/gpu/drm/i915/i915_reg.h | 9 ++--
>> 3 files changed, 48 insertions(+), 22 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/gt/intel_workarounds.c
>> b/drivers/gpu/drm/i915/gt/intel_workarounds.c
>> index 93caa7b6d7a9..4429ee08b20e 100644
>> --- a/drivers/gpu/drm/i915/gt/intel_workarounds.c
>> +++ b/drivers/gpu/drm/i915/gt/intel_workarounds.c
>> @@ -1028,7 +1028,7 @@ whitelist_reg_ext(struct i915_wa_list *wal,
>> i915_reg_t reg, u32 flags)
>> static void
>> whitelist_reg(struct i915_wa_list *wal, i915_reg_t reg)
>> {
>> - whitelist_reg_ext(wal, reg, RING_FORCE_TO_NONPRIV_RW);
>> + whitelist_reg_ext(wal, reg, RING_FORCE_TO_NONPRIV_ACCESS_RW);
>> }
>> static void gen9_whitelist_build(struct i915_wa_list *w)
>> @@ -1134,13 +1134,13 @@ printk(KERN_INFO "%-32s:%4d> Boo! [engine =
>> %s, instance = %d, base = 0x%X, reg
>> /* hucStatusRegOffset */
>> whitelist_reg_ext(w, _MMIO(0x2000 + engine->mmio_base),
>> - RING_FORCE_TO_NONPRIV_RD);
>> + RING_FORCE_TO_NONPRIV_ACCESS_RD);
>> /* hucUKernelHdrInfoRegOffset */
>> whitelist_reg_ext(w, _MMIO(0x2014 + engine->mmio_base),
>> - RING_FORCE_TO_NONPRIV_RD);
>> + RING_FORCE_TO_NONPRIV_ACCESS_RD);
>> /* hucStatus2RegOffset */
>> whitelist_reg_ext(w, _MMIO(0x23B0 + engine->mmio_base),
>> - RING_FORCE_TO_NONPRIV_RD);
>> + RING_FORCE_TO_NONPRIV_ACCESS_RD);
>> break;
>> default:
>> diff --git a/drivers/gpu/drm/i915/gt/selftest_workarounds.c
>> b/drivers/gpu/drm/i915/gt/selftest_workarounds.c
>> index eb6d3aa2c8cc..a0a88ec66861 100644
>> --- a/drivers/gpu/drm/i915/gt/selftest_workarounds.c
>> +++ b/drivers/gpu/drm/i915/gt/selftest_workarounds.c
>> @@ -399,6 +399,10 @@ static bool wo_register(struct intel_engine_cs
>> *engine, u32 reg)
>> enum intel_platform platform = INTEL_INFO(engine->i915)->platform;
>> int i;
>> + if ((reg & RING_FORCE_TO_NONPRIV_ACCESS_MASK) ==
>> + RING_FORCE_TO_NONPRIV_ACCESS_WR)
>> + return true;
>> +
>> for (i = 0; i < ARRAY_SIZE(wo_registers); i++) {
>> if (wo_registers[i].platform == platform &&
>> wo_registers[i].reg == reg)
>> @@ -410,7 +414,8 @@ static bool wo_register(struct intel_engine_cs
>> *engine, u32 reg)
>> static bool ro_register(u32 reg)
>> {
>> - if (reg & RING_FORCE_TO_NONPRIV_RD)
>> + if ((reg & RING_FORCE_TO_NONPRIV_ACCESS_MASK) ==
>> + RING_FORCE_TO_NONPRIV_ACCESS_RD)
>> return true;
>> return false;
>> @@ -482,12 +487,12 @@ static int check_dirty_whitelist(struct
>> i915_gem_context *ctx,
>> u32 srm, lrm, rsvd;
>> u32 expect;
>> int idx;
>> + bool ro_reg;
>> if (wo_register(engine, reg))
>> continue;
>> - if (ro_register(reg))
>> - continue;
>> + ro_reg = ro_register(reg);
>> srm = MI_STORE_REGISTER_MEM;
>> lrm = MI_LOAD_REGISTER_MEM;
>> @@ -588,24 +593,36 @@ static int check_dirty_whitelist(struct
>> i915_gem_context *ctx,
>> }
>> GEM_BUG_ON(values[ARRAY_SIZE(values) - 1] != 0xffffffff);
>> - rsvd = results[ARRAY_SIZE(values)]; /* detect write masking */
>> - if (!rsvd) {
>> - pr_err("%s: Unable to write to whitelisted register %x\n",
>> - engine->name, reg);
>> - err = -EINVAL;
>> - goto out_unpin;
>> + if (ro_reg) {
>> + rsvd = 0xFFFFFFFF;
>> + } else {
>> + rsvd = results[ARRAY_SIZE(values)]; /* detect write
>> masking */
>> + if (!rsvd) {
>> + pr_err("%s: Unable to write to whitelisted register
>> %x\n",
>> + engine->name, reg);
>> + err = -EINVAL;
>> + goto out_unpin;
>> + }
>> }
>> expect = results[0];
>> idx = 1;
>> for (v = 0; v < ARRAY_SIZE(values); v++) {
>> - expect = reg_write(expect, values[v], rsvd);
>> + if (ro_reg)
>> + expect = results[0];
>> + else
>> + expect = reg_write(expect, values[v], rsvd);
>> +
>> if (results[idx] != expect)
>> err++;
>> idx++;
>> }
>> for (v = 0; v < ARRAY_SIZE(values); v++) {
>> - expect = reg_write(expect, ~values[v], rsvd);
>> + if (ro_reg)
>> + expect = results[0];
>> + else
>> + expect = reg_write(expect, ~values[v], rsvd);
>> +
>> if (results[idx] != expect)
>> err++;
>> idx++;
>> @@ -622,7 +639,10 @@ static int check_dirty_whitelist(struct
>> i915_gem_context *ctx,
>> for (v = 0; v < ARRAY_SIZE(values); v++) {
>> u32 w = values[v];
>> - expect = reg_write(expect, w, rsvd);
>> + if (ro_reg)
>> + expect = results[0];
>> + else
>> + expect = reg_write(expect, w, rsvd);
>> pr_info("Wrote %08x, read %08x, expect %08x\n",
>> w, results[idx], expect);
>> idx++;
>> @@ -630,7 +650,10 @@ static int check_dirty_whitelist(struct
>> i915_gem_context *ctx,
>> for (v = 0; v < ARRAY_SIZE(values); v++) {
>> u32 w = ~values[v];
>> - expect = reg_write(expect, w, rsvd);
>> + if (ro_reg)
>> + expect = results[0];
>> + else
>> + expect = reg_write(expect, w, rsvd);
>> pr_info("Wrote %08x, read %08x, expect %08x\n",
>> w, results[idx], expect);
>> idx++;
>> @@ -762,8 +785,8 @@ static int read_whitelisted_registers(struct
>> i915_gem_context *ctx,
>> u64 offset = results->node.start + sizeof(u32) * i;
>> u32 reg = i915_mmio_reg_offset(engine->whitelist.list[i].reg);
>> - /* Clear RD only and WR only flags */
>> - reg &= ~(RING_FORCE_TO_NONPRIV_RD | RING_FORCE_TO_NONPRIV_WR);
>> + /* Clear access permission field */
>> + reg &= ~RING_FORCE_TO_NONPRIV_ACCESS_MASK;
>> *cs++ = srm;
>> *cs++ = reg;
>> diff --git a/drivers/gpu/drm/i915/i915_reg.h
>> b/drivers/gpu/drm/i915/i915_reg.h
>> index cc295a4f6e92..ba22e3b8f77e 100644
>> --- a/drivers/gpu/drm/i915/i915_reg.h
>> +++ b/drivers/gpu/drm/i915/i915_reg.h
>> @@ -2513,13 +2513,16 @@ enum i915_power_well_id {
>> #define RING_WAIT_SEMAPHORE (1 << 10) /* gen6+ */
>> #define RING_FORCE_TO_NONPRIV(base, i) _MMIO(((base) + 0x4D0) + (i)
>> * 4)
>> -#define RING_FORCE_TO_NONPRIV_RW (0 << 28) /* CFL+ &
>> Gen11+ */
>> -#define RING_FORCE_TO_NONPRIV_RD (1 << 28)
>> -#define RING_FORCE_TO_NONPRIV_WR (2 << 28)
>> +#define RING_FORCE_TO_NONPRIV_ACCESS_RW (0 << 28) /* CFL+ &
>> Gen11+ */
>> +#define RING_FORCE_TO_NONPRIV_ACCESS_RD (1 << 28)
>> +#define RING_FORCE_TO_NONPRIV_ACCESS_WR (2 << 28)
>> +#define RING_FORCE_TO_NONPRIV_ACCESS_INVALID (3 << 28)
>> +#define RING_FORCE_TO_NONPRIV_ACCESS_MASK (3 << 28)
>> #define RING_FORCE_TO_NONPRIV_RANGE_1 (0 << 0) /* CFL+
>> & Gen11+ */
>> #define RING_FORCE_TO_NONPRIV_RANGE_4 (1 << 0)
>> #define RING_FORCE_TO_NONPRIV_RANGE_16 (2 << 0)
>> #define RING_FORCE_TO_NONPRIV_RANGE_64 (3 << 0)
>> +#define RING_FORCE_TO_NONPRIV_RANGE_MASK (3 << 0)
>> #define RING_MAX_NONPRIV_SLOTS 12
>> #define GEN7_TLB_RD_ADDR _MMIO(0x4700)
>>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
next prev parent reply other threads:[~2019-06-25 8:33 UTC|newest]
Thread overview: 25+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-06-18 1:01 [PATCH 0/4] Update whitelist support for new hardware John.C.Harrison
2019-06-18 1:01 ` [PATCH 1/4] drm/i915: Support flags in whitlist WAs John.C.Harrison
2019-06-18 6:27 ` Tvrtko Ursulin
2019-06-18 6:35 ` Tvrtko Ursulin
2019-06-18 13:48 ` John Harrison
2019-06-18 16:10 ` Tvrtko Ursulin
2019-06-18 1:01 ` [PATCH 2/4] drm/i915: Support whitelist workarounds on all engines John.C.Harrison
2019-06-18 6:29 ` Tvrtko Ursulin
2019-06-18 1:01 ` [PATCH 3/4] drm/i915: Add whitelist workarounds for ICL John.C.Harrison
2019-06-18 6:30 ` Tvrtko Ursulin
2019-06-18 1:01 ` [PATCH 4/4] drm/i915: Update workarounds selftest for read only regs John.C.Harrison
2019-06-18 6:42 ` Tvrtko Ursulin
2019-06-18 13:43 ` John Harrison
2019-06-18 16:14 ` Tvrtko Ursulin
2019-06-18 1:50 ` ✓ Fi.CI.BAT: success for Update whitelist support for new hardware (rev2) Patchwork
2019-06-18 16:33 ` Tvrtko Ursulin
2019-06-18 19:54 ` [PATCH] drm/i915: Implement read-only support in whitelist selftest John.C.Harrison
2019-06-18 20:08 ` John Harrison
2019-06-19 6:41 ` Tvrtko Ursulin
2019-06-19 6:49 ` Tvrtko Ursulin
2019-06-20 15:43 ` Tvrtko Ursulin
2019-06-25 8:33 ` Tvrtko Ursulin [this message]
2019-07-03 2:20 ` John Harrison
2019-06-18 20:51 ` ✗ Fi.CI.BAT: failure for " Patchwork
2019-06-18 16:22 ` ✓ Fi.CI.IGT: success for Update whitelist support for new hardware (rev2) Patchwork
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=d1af8bf7-246b-a2e0-e719-1a282fb1341f@linux.intel.com \
--to=tvrtko.ursulin@linux.intel.com \
--cc=Intel-GFX@Lists.FreeDesktop.Org \
--cc=John.C.Harrison@Intel.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.