| Message ID | 1411640220-32696-1-git-send-email-tvrtko.ursulin@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, Sep 25, 2014 at 11:17:00AM +0100, Tvrtko Ursulin wrote: > From: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > > Write and reads following the block changed use engine specific use counters > and unless that is matched here force wake use counting goes bad. Same > force wake is attempted to be taken twice which leads to at least time outs. > > Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com> Is it worth a v2 to have gen >= 9 here?
Damien Lespiau <damien.lespiau@intel.com> writes: > On Thu, Sep 25, 2014 at 11:17:00AM +0100, Tvrtko Ursulin wrote: >> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com> >> >> Write and reads following the block changed use engine specific use counters >> and unless that is matched here force wake use counting goes bad. Same >> force wake is attempted to be taken twice which leads to at least time outs. >> >> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > > Is it worth a v2 to have gen >= 9 here? I think we should have gen >= 8 here. Shadowed ELSP's seems not to work on gen8. And the posting read will need fw anyways. Assuming the shadowing works on skl and we can get rid of the posting read, we could run this part without taking forcewake. -Mika > -- > Damien > >> --- >> drivers/gpu/drm/i915/intel_lrc.c | 4 ++-- >> 1 file changed, 2 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c >> index 3048d78..4b33767 100644 >> --- a/drivers/gpu/drm/i915/intel_lrc.c >> +++ b/drivers/gpu/drm/i915/intel_lrc.c >> @@ -301,7 +301,7 @@ static void execlists_elsp_write(struct intel_engine_cs *ring, >> * Instead, we do the runtime_pm_get/put when creating/destroying requests. >> */ >> spin_lock_irqsave(&dev_priv->uncore.lock, flags); >> - if (IS_CHERRYVIEW(dev_priv->dev)) { >> + if (IS_CHERRYVIEW(dev_priv->dev) || IS_GEN9(dev_priv->dev)) { >> if (dev_priv->uncore.fw_rendercount++ == 0) >> dev_priv->uncore.funcs.force_wake_get(dev_priv, >> FORCEWAKE_RENDER); >> @@ -326,7 +326,7 @@ static void execlists_elsp_write(struct intel_engine_cs *ring, >> >> /* Release Force Wakeup (see the big comment above). */ >> spin_lock_irqsave(&dev_priv->uncore.lock, flags); >> - if (IS_CHERRYVIEW(dev_priv->dev)) { >> + if (IS_CHERRYVIEW(dev_priv->dev) || IS_GEN9(dev_priv->dev)) { >> if (--dev_priv->uncore.fw_rendercount == 0) >> dev_priv->uncore.funcs.force_wake_put(dev_priv, >> FORCEWAKE_RENDER); >> -- >> 2.1.0 >> > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
On 09/25/2014 01:05 PM, Mika Kuoppala wrote: > Damien Lespiau <damien.lespiau@intel.com> writes: > >> On Thu, Sep 25, 2014 at 11:17:00AM +0100, Tvrtko Ursulin wrote: >>> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com> >>> >>> Write and reads following the block changed use engine specific use counters >>> and unless that is matched here force wake use counting goes bad. Same >>> force wake is attempted to be taken twice which leads to at least time outs. >>> >>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com> >> >> Is it worth a v2 to have gen >= 9 here? > > I think we should have gen >= 8 here. But that would not match against the current implementation of GEN8 vs CHV read/write functions. > Shadowed ELSP's seems not to work on gen8. And the posting read will > need fw anyways. > > Assuming the shadowing works on skl and we can get rid of the posting > read, we could run this part without taking forcewake. I don't know what criteria would need to be satisfied to get rid of the posting read. On GEN9 only you are saying? 2nd part, how to test if shadowing works? Just remove force wakes and see what happens? Thanks, Tvrtko
On Thu, Sep 25, 2014 at 01:43:31PM +0100, Tvrtko Ursulin wrote: > > On 09/25/2014 01:05 PM, Mika Kuoppala wrote: > >Damien Lespiau <damien.lespiau@intel.com> writes: > > > >>On Thu, Sep 25, 2014 at 11:17:00AM +0100, Tvrtko Ursulin wrote: > >>>From: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > >>> > >>>Write and reads following the block changed use engine specific use counters > >>>and unless that is matched here force wake use counting goes bad. Same > >>>force wake is attempted to be taken twice which leads to at least time outs. > >>> > >>>Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > >> > >>Is it worth a v2 to have gen >= 9 here? > > > >I think we should have gen >= 8 here. > > But that would not match against the current implementation of GEN8 > vs CHV read/write functions. The first problem here is that we have two sets of reference counts, one for FORCEWAKE_ALL (uncore.forcewake_count) and one for each forcewake engine on CHV/SKL. CHV has 2 fw engines, SKL 3. So, in that regard, maybe the fw engine abstraction needs work to unify the reference counts. The least intrusive change to make SKL not error out would be to have a separate code path for gen >= 9 with the 3 engines. The thread leading to that patch is great and already has all the right questions. We just ignored them. The most immediate one is, why are we waking up all the fw engines when writing a specific ring ELSP, but the whole thread is worth reading. > >Shadowed ELSP's seems not to work on gen8. And the posting read will > >need fw anyways. > > > >Assuming the shadowing works on skl and we can get rid of the posting > >read, we could run this part without taking forcewake. > > I don't know what criteria would need to be satisfied to get rid of > the posting read. On GEN9 only you are saying? > > 2nd part, how to test if shadowing works? Just remove force wakes > and see what happens? And that's a separate issue, can we use the shadow register mechanism to ensure the write lands and is that enough? Let's try to ask people internally about that. HTH,
diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c index 3048d78..4b33767 100644 --- a/drivers/gpu/drm/i915/intel_lrc.c +++ b/drivers/gpu/drm/i915/intel_lrc.c @@ -301,7 +301,7 @@ static void execlists_elsp_write(struct intel_engine_cs *ring, * Instead, we do the runtime_pm_get/put when creating/destroying requests. */ spin_lock_irqsave(&dev_priv->uncore.lock, flags); - if (IS_CHERRYVIEW(dev_priv->dev)) { + if (IS_CHERRYVIEW(dev_priv->dev) || IS_GEN9(dev_priv->dev)) { if (dev_priv->uncore.fw_rendercount++ == 0) dev_priv->uncore.funcs.force_wake_get(dev_priv, FORCEWAKE_RENDER); @@ -326,7 +326,7 @@ static void execlists_elsp_write(struct intel_engine_cs *ring, /* Release Force Wakeup (see the big comment above). */ spin_lock_irqsave(&dev_priv->uncore.lock, flags); - if (IS_CHERRYVIEW(dev_priv->dev)) { + if (IS_CHERRYVIEW(dev_priv->dev) || IS_GEN9(dev_priv->dev)) { if (--dev_priv->uncore.fw_rendercount == 0) dev_priv->uncore.funcs.force_wake_put(dev_priv, FORCEWAKE_RENDER);