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: Wed, 19 Jun 2019 07:49:18 +0100 [thread overview]
Message-ID: <a4292c39-51fc-9844-1a65-9dc24a8f15ac@linux.intel.com> (raw)
In-Reply-To: <20190618195421.31027-1-John.C.Harrison@Intel.com>
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)
Reminder to add a GEM_BUG_ON if invalid flags are passed in.
> 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;
I guess it doesn't matter what value you set since it is only used on
pr_info on ro_reg paths?
> + } 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)
>
This in fact looks fine to me.
Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Regards,
Tvrtko
_______________________________________________
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-19 6:49 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 [this message]
2019-06-20 15:43 ` Tvrtko Ursulin
2019-06-25 8:33 ` Tvrtko Ursulin
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=a4292c39-51fc-9844-1a65-9dc24a8f15ac@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.