| Message ID | 1506638439-6903-1-git-send-email-oscar.mateo@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 28/09/17 15:40, Oscar Mateo wrote: > RING_FORCE_TO_NONPRIV registers do not live in the logical context. They are simply > global privileged MMIO registers that happen to be powercontext saved and restored > (meaning only they can survive RC6). Therefore, there is absolutely no need to save > them so that they can be restored everytime we create a new logical context. > > Suggested-by: Chris Wilson <chris@chris-wilson.co.uk> > Signed-off-by: Oscar Mateo <oscar.mateo@intel.com> > Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com> > --- > drivers/gpu/drm/i915/intel_engine_cs.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_engine_cs.c b/drivers/gpu/drm/i915/intel_engine_cs.c > index a28e2a8..a75f5e8 100644 > --- a/drivers/gpu/drm/i915/intel_engine_cs.c > +++ b/drivers/gpu/drm/i915/intel_engine_cs.c > @@ -845,8 +845,8 @@ static int wa_ring_whitelist_reg(struct intel_engine_cs *engine, > if (WARN_ON(index >= RING_MAX_NONPRIV_SLOTS)) > return -EINVAL; > > - WA_WRITE(RING_FORCE_TO_NONPRIV(engine->mmio_base, index), > - i915_mmio_reg_offset(reg)); > + I915_WRITE(RING_FORCE_TO_NONPRIV(engine->mmio_base, index), > + i915_mmio_reg_offset(reg)); > wa->hw_whitelist_count[engine->id]++; > > return 0; > -- > 1.9.1 > I see RCS_FORCE_TO_NONPRIV in "Render Engine *Power* Context" and not in the "Register State Context", so Acked-by: Michel Thierry <michel.thierry@intel.com>
Quoting Oscar Mateo (2017-09-28 23:40:39) > RING_FORCE_TO_NONPRIV registers do not live in the logical context. They are simply > global privileged MMIO registers that happen to be powercontext saved and restored > (meaning only they can survive RC6). Therefore, there is absolutely no need to save > them so that they can be restored everytime we create a new logical context. > > Suggested-by: Chris Wilson <chris@chris-wilson.co.uk> > Signed-off-by: Oscar Mateo <oscar.mateo@intel.com> > Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com> Tested-by: Chris Wilson <chris@chris-wilson.co.uk> #bxt Now we just need Mika for the full set of tags! :) -Chris
On Thu, 2017-09-28 at 16:47 -0700, Michel Thierry wrote: > On 28/09/17 15:40, Oscar Mateo wrote: > > RING_FORCE_TO_NONPRIV registers do not live in the logical context. They are simply > > global privileged MMIO registers that happen to be powercontext saved and restored > > (meaning only they can survive RC6). Therefore, there is absolutely no need to save > > them so that they can be restored everytime we create a new logical context. > > > > Suggested-by: Chris Wilson <chris@chris-wilson.co.uk> > > Signed-off-by: Oscar Mateo <oscar.mateo@intel.com> > > Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com> > > --- > > drivers/gpu/drm/i915/intel_engine_cs.c | 4 ++-- > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/intel_engine_cs.c b/drivers/gpu/drm/i915/intel_engine_cs.c > > index a28e2a8..a75f5e8 100644 > > --- a/drivers/gpu/drm/i915/intel_engine_cs.c > > +++ b/drivers/gpu/drm/i915/intel_engine_cs.c > > @@ -845,8 +845,8 @@ static int wa_ring_whitelist_reg(struct intel_engine_cs *engine, > > if (WARN_ON(index >= RING_MAX_NONPRIV_SLOTS)) > > return -EINVAL; > > > > - WA_WRITE(RING_FORCE_TO_NONPRIV(engine->mmio_base, index), > > - i915_mmio_reg_offset(reg)); > > + I915_WRITE(RING_FORCE_TO_NONPRIV(engine->mmio_base, index), > > + i915_mmio_reg_offset(reg)); > > wa->hw_whitelist_count[engine->id]++; > > > > return 0; > > -- > > 1.9.1 > > > > I see RCS_FORCE_TO_NONPRIV in "Render Engine *Power* Context" and not in > the "Register State Context", so > > Acked-by: Michel Thierry <michel.thierry@intel.com> You reviewed the spec and the code, so should be Reviewed-by :) Then this could be merged, too. Regards, Joonas
On 9/29/2017 3:25 AM, Joonas Lahtinen wrote: > On Thu, 2017-09-28 at 16:47 -0700, Michel Thierry wrote: >> On 28/09/17 15:40, Oscar Mateo wrote: >>> RING_FORCE_TO_NONPRIV registers do not live in the logical context. They are simply >>> global privileged MMIO registers that happen to be powercontext saved and restored >>> (meaning only they can survive RC6). Therefore, there is absolutely no need to save >>> them so that they can be restored everytime we create a new logical context. >>> >>> Suggested-by: Chris Wilson <chris@chris-wilson.co.uk> >>> Signed-off-by: Oscar Mateo <oscar.mateo@intel.com> >>> Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com> >>> --- >>> drivers/gpu/drm/i915/intel_engine_cs.c | 4 ++-- >>> 1 file changed, 2 insertions(+), 2 deletions(-) >>> >>> diff --git a/drivers/gpu/drm/i915/intel_engine_cs.c b/drivers/gpu/drm/i915/intel_engine_cs.c >>> index a28e2a8..a75f5e8 100644 >>> --- a/drivers/gpu/drm/i915/intel_engine_cs.c >>> +++ b/drivers/gpu/drm/i915/intel_engine_cs.c >>> @@ -845,8 +845,8 @@ static int wa_ring_whitelist_reg(struct intel_engine_cs *engine, >>> if (WARN_ON(index >= RING_MAX_NONPRIV_SLOTS)) >>> return -EINVAL; >>> >>> - WA_WRITE(RING_FORCE_TO_NONPRIV(engine->mmio_base, index), >>> - i915_mmio_reg_offset(reg)); >>> + I915_WRITE(RING_FORCE_TO_NONPRIV(engine->mmio_base, index), >>> + i915_mmio_reg_offset(reg)); >>> wa->hw_whitelist_count[engine->id]++; >>> >>> return 0; >>> -- >>> 1.9.1 >>> >> >> I see RCS_FORCE_TO_NONPRIV in "Render Engine *Power* Context" and not in >> the "Register State Context", so >> >> Acked-by: Michel Thierry <michel.thierry@intel.com> > > You reviewed the spec and the code, so should be Reviewed-by :) > > Then this could be merged, too. > > Regards, Joonas > I thought Chris would be adding his. Swap my acked-by to, Reviewed-by: Michel Thierry <michel.thierry@intel.com>
On Fri, 2017-09-29 at 09:38 +0100, Chris Wilson wrote: > Quoting Oscar Mateo (2017-09-28 23:40:39) > > RING_FORCE_TO_NONPRIV registers do not live in the logical context. They are simply > > global privileged MMIO registers that happen to be powercontext saved and restored > > (meaning only they can survive RC6). Therefore, there is absolutely no need to save > > them so that they can be restored everytime we create a new logical context. > > > > Suggested-by: Chris Wilson <chris@chris-wilson.co.uk> > > Signed-off-by: Oscar Mateo <oscar.mateo@intel.com> > > Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com> > > Tested-by: Chris Wilson <chris@chris-wilson.co.uk> #bxt > > Now we just need Mika for the full set of tags! :) Mika, ping? Regards, Joonas
Oscar Mateo <oscar.mateo@intel.com> writes: > RING_FORCE_TO_NONPRIV registers do not live in the logical context. They are simply > global privileged MMIO registers that happen to be powercontext saved and restored > (meaning only they can survive RC6). Therefore, there is absolutely no need to save > them so that they can be restored everytime we create a new logical context. > > Suggested-by: Chris Wilson <chris@chris-wilson.co.uk> > Signed-off-by: Oscar Mateo <oscar.mateo@intel.com> > Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com> > --- > drivers/gpu/drm/i915/intel_engine_cs.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_engine_cs.c b/drivers/gpu/drm/i915/intel_engine_cs.c > index a28e2a8..a75f5e8 100644 > --- a/drivers/gpu/drm/i915/intel_engine_cs.c > +++ b/drivers/gpu/drm/i915/intel_engine_cs.c > @@ -845,8 +845,8 @@ static int wa_ring_whitelist_reg(struct intel_engine_cs *engine, > if (WARN_ON(index >= RING_MAX_NONPRIV_SLOTS)) > return -EINVAL; > > - WA_WRITE(RING_FORCE_TO_NONPRIV(engine->mmio_base, index), > - i915_mmio_reg_offset(reg)); > + I915_WRITE(RING_FORCE_TO_NONPRIV(engine->mmio_base, index), > + i915_mmio_reg_offset(reg)); #define WA_WRITE should also been removed as it is clearly dangerous. Chris pointed out that anything with nonmasked access is not part of context image, and this seems to hold true in atleast with current cases. But removing of define can be a followup. Reviewed-by: Mika Kuoppala <mika.kuoppala@linux.intel.com> > wa->hw_whitelist_count[engine->id]++; > > return 0; > -- > 1.9.1
Quoting Mika Kuoppala (2017-10-04 13:39:13) > Oscar Mateo <oscar.mateo@intel.com> writes: > > > RING_FORCE_TO_NONPRIV registers do not live in the logical context. They are simply > > global privileged MMIO registers that happen to be powercontext saved and restored > > (meaning only they can survive RC6). Therefore, there is absolutely no need to save > > them so that they can be restored everytime we create a new logical context. > > > > Suggested-by: Chris Wilson <chris@chris-wilson.co.uk> > > Signed-off-by: Oscar Mateo <oscar.mateo@intel.com> > > Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com> > > --- > > drivers/gpu/drm/i915/intel_engine_cs.c | 4 ++-- > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/intel_engine_cs.c b/drivers/gpu/drm/i915/intel_engine_cs.c > > index a28e2a8..a75f5e8 100644 > > --- a/drivers/gpu/drm/i915/intel_engine_cs.c > > +++ b/drivers/gpu/drm/i915/intel_engine_cs.c > > @@ -845,8 +845,8 @@ static int wa_ring_whitelist_reg(struct intel_engine_cs *engine, > > if (WARN_ON(index >= RING_MAX_NONPRIV_SLOTS)) > > return -EINVAL; > > > > - WA_WRITE(RING_FORCE_TO_NONPRIV(engine->mmio_base, index), > > - i915_mmio_reg_offset(reg)); > > + I915_WRITE(RING_FORCE_TO_NONPRIV(engine->mmio_base, index), > > + i915_mmio_reg_offset(reg)); > > #define WA_WRITE should also been removed as it is clearly > dangerous. Chris pointed out that anything with nonmasked access > is not part of context image, and this seems to hold true in > atleast with current cases. > > But removing of define can be a followup. I've picked up this patch and I'll squash in the -WA_WRITE into the removal of WA_SET_BIT and push all 3 patches at once (when the shards report back). Thanks for the patch, review, testing, debate and keep having fun. -Chris
On 10/04/2017 06:17 AM, Chris Wilson wrote: > Quoting Mika Kuoppala (2017-10-04 13:39:13) >> Oscar Mateo <oscar.mateo@intel.com> writes: >> >>> RING_FORCE_TO_NONPRIV registers do not live in the logical context. They are simply >>> global privileged MMIO registers that happen to be powercontext saved and restored >>> (meaning only they can survive RC6). Therefore, there is absolutely no need to save >>> them so that they can be restored everytime we create a new logical context. >>> >>> Suggested-by: Chris Wilson <chris@chris-wilson.co.uk> >>> Signed-off-by: Oscar Mateo <oscar.mateo@intel.com> >>> Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com> >>> --- >>> drivers/gpu/drm/i915/intel_engine_cs.c | 4 ++-- >>> 1 file changed, 2 insertions(+), 2 deletions(-) >>> >>> diff --git a/drivers/gpu/drm/i915/intel_engine_cs.c b/drivers/gpu/drm/i915/intel_engine_cs.c >>> index a28e2a8..a75f5e8 100644 >>> --- a/drivers/gpu/drm/i915/intel_engine_cs.c >>> +++ b/drivers/gpu/drm/i915/intel_engine_cs.c >>> @@ -845,8 +845,8 @@ static int wa_ring_whitelist_reg(struct intel_engine_cs *engine, >>> if (WARN_ON(index >= RING_MAX_NONPRIV_SLOTS)) >>> return -EINVAL; >>> >>> - WA_WRITE(RING_FORCE_TO_NONPRIV(engine->mmio_base, index), >>> - i915_mmio_reg_offset(reg)); >>> + I915_WRITE(RING_FORCE_TO_NONPRIV(engine->mmio_base, index), >>> + i915_mmio_reg_offset(reg)); >> #define WA_WRITE should also been removed as it is clearly >> dangerous. Chris pointed out that anything with nonmasked access >> is not part of context image, and this seems to hold true in >> atleast with current cases. >> >> But removing of define can be a followup. > I've picked up this patch and I'll squash in the -WA_WRITE into the > removal of WA_SET_BIT and push all 3 patches at once (when the shards > report back). > > Thanks for the patch, review, testing, debate and keep having fun. > -Chris We can also remove RING_MAX_NONPRIV_SLOTS from here: #define I915_MAX_WA_REGS (16 + RING_MAX_NONPRIV_SLOTS)
diff --git a/drivers/gpu/drm/i915/intel_engine_cs.c b/drivers/gpu/drm/i915/intel_engine_cs.c index a28e2a8..a75f5e8 100644 --- a/drivers/gpu/drm/i915/intel_engine_cs.c +++ b/drivers/gpu/drm/i915/intel_engine_cs.c @@ -845,8 +845,8 @@ static int wa_ring_whitelist_reg(struct intel_engine_cs *engine, if (WARN_ON(index >= RING_MAX_NONPRIV_SLOTS)) return -EINVAL; - WA_WRITE(RING_FORCE_TO_NONPRIV(engine->mmio_base, index), - i915_mmio_reg_offset(reg)); + I915_WRITE(RING_FORCE_TO_NONPRIV(engine->mmio_base, index), + i915_mmio_reg_offset(reg)); wa->hw_whitelist_count[engine->id]++; return 0;
RING_FORCE_TO_NONPRIV registers do not live in the logical context. They are simply global privileged MMIO registers that happen to be powercontext saved and restored (meaning only they can survive RC6). Therefore, there is absolutely no need to save them so that they can be restored everytime we create a new logical context. Suggested-by: Chris Wilson <chris@chris-wilson.co.uk> Signed-off-by: Oscar Mateo <oscar.mateo@intel.com> Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com> --- drivers/gpu/drm/i915/intel_engine_cs.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)