| Message ID | 1398808360-3674-14-git-send-email-benjamin.widawsky@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, Apr 29, 2014 at 02:52:40PM -0700, Ben Widawsky wrote: > This appears to not actually be needed on the current code. Just putting > it on the ML so we can point bug reports at it later. > > As pointed out by Ville, the current code is "broken" since we do > FORCE_RESTORE, and RESTORE_INHIBIT on the same dword. Anecdotally, this > seems fine. > > Signed-off-by: Ben Widawsky <ben@bwidawsk.net> > --- > drivers/gpu/drm/i915/i915_gem_context.c | 7 +++++++ > 1 file changed, 7 insertions(+) > > diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c > index f77b4c1..aa82fb4 100644 > --- a/drivers/gpu/drm/i915/i915_gem_context.c > +++ b/drivers/gpu/drm/i915/i915_gem_context.c > @@ -661,6 +661,13 @@ static int do_switch(struct intel_ring_buffer *ring, > if (!to->is_initialized || i915_gem_context_is_default(to)) > hw_flags |= MI_RESTORE_INHIBIT; > > + /* When SW intends to use semaphore signaling between Command streamers, > + * it must avoid lite restores in HW by programming "Force Restore" bit > + * to ‘1’ in context descriptor during context submission > + */ > + if (IS_GEN8(ring->dev) && i915_semaphore_is_enabled(ring->dev)) > + hw_flags |= MI_FORCE_RESTORE; Is it not an error to set both FORCE and INHIBIT? -Chris
On Wed, Apr 30, 2014 at 08:13:25AM +0100, Chris Wilson wrote: > On Tue, Apr 29, 2014 at 02:52:40PM -0700, Ben Widawsky wrote: > > This appears to not actually be needed on the current code. Just putting > > it on the ML so we can point bug reports at it later. > > > > As pointed out by Ville, the current code is "broken" since we do > > FORCE_RESTORE, and RESTORE_INHIBIT on the same dword. Anecdotally, this > > seems fine. > > > > Signed-off-by: Ben Widawsky <ben@bwidawsk.net> > > --- > > drivers/gpu/drm/i915/i915_gem_context.c | 7 +++++++ > > 1 file changed, 7 insertions(+) > > > > diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c > > index f77b4c1..aa82fb4 100644 > > --- a/drivers/gpu/drm/i915/i915_gem_context.c > > +++ b/drivers/gpu/drm/i915/i915_gem_context.c > > @@ -661,6 +661,13 @@ static int do_switch(struct intel_ring_buffer *ring, > > if (!to->is_initialized || i915_gem_context_is_default(to)) > > hw_flags |= MI_RESTORE_INHIBIT; > > > > + /* When SW intends to use semaphore signaling between Command streamers, > > + * it must avoid lite restores in HW by programming "Force Restore" bit > > + * to ‘1’ in context descriptor during context submission > > + */ > > + if (IS_GEN8(ring->dev) && i915_semaphore_is_enabled(ring->dev)) > > + hw_flags |= MI_FORCE_RESTORE; > > Is it not an error to set both FORCE and INHIBIT? > -Chris Read the commit message.
On Wed, Apr 30, 2014 at 11:44:47AM -0700, Ben Widawsky wrote: > On Wed, Apr 30, 2014 at 08:13:25AM +0100, Chris Wilson wrote: > > On Tue, Apr 29, 2014 at 02:52:40PM -0700, Ben Widawsky wrote: > > > This appears to not actually be needed on the current code. Just putting > > > it on the ML so we can point bug reports at it later. > > > > > > As pointed out by Ville, the current code is "broken" since we do > > > FORCE_RESTORE, and RESTORE_INHIBIT on the same dword. Anecdotally, this > > > seems fine. > > > > > > Signed-off-by: Ben Widawsky <ben@bwidawsk.net> > > > --- > > > drivers/gpu/drm/i915/i915_gem_context.c | 7 +++++++ > > > 1 file changed, 7 insertions(+) > > > > > > diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c > > > index f77b4c1..aa82fb4 100644 > > > --- a/drivers/gpu/drm/i915/i915_gem_context.c > > > +++ b/drivers/gpu/drm/i915/i915_gem_context.c > > > @@ -661,6 +661,13 @@ static int do_switch(struct intel_ring_buffer *ring, > > > if (!to->is_initialized || i915_gem_context_is_default(to)) > > > hw_flags |= MI_RESTORE_INHIBIT; > > > > > > + /* When SW intends to use semaphore signaling between Command streamers, > > > + * it must avoid lite restores in HW by programming "Force Restore" bit > > > + * to ‘1’ in context descriptor during context submission > > > + */ > > > + if (IS_GEN8(ring->dev) && i915_semaphore_is_enabled(ring->dev)) > > > + hw_flags |= MI_FORCE_RESTORE; > > > > Is it not an error to set both FORCE and INHIBIT? > > -Chris > > Read the commit message. And for once I was reading the code. Wouldn't it be worth to clear MI_RESTORE_INHIBIT any way to silence the critics? -Chris
On Wed, Apr 30, 2014 at 08:03:27PM +0100, Chris Wilson wrote: > On Wed, Apr 30, 2014 at 11:44:47AM -0700, Ben Widawsky wrote: > > On Wed, Apr 30, 2014 at 08:13:25AM +0100, Chris Wilson wrote: > > > On Tue, Apr 29, 2014 at 02:52:40PM -0700, Ben Widawsky wrote: > > > > This appears to not actually be needed on the current code. Just putting > > > > it on the ML so we can point bug reports at it later. > > > > > > > > As pointed out by Ville, the current code is "broken" since we do > > > > FORCE_RESTORE, and RESTORE_INHIBIT on the same dword. Anecdotally, this > > > > seems fine. > > > > > > > > Signed-off-by: Ben Widawsky <ben@bwidawsk.net> > > > > --- > > > > drivers/gpu/drm/i915/i915_gem_context.c | 7 +++++++ > > > > 1 file changed, 7 insertions(+) > > > > > > > > diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c > > > > index f77b4c1..aa82fb4 100644 > > > > --- a/drivers/gpu/drm/i915/i915_gem_context.c > > > > +++ b/drivers/gpu/drm/i915/i915_gem_context.c > > > > @@ -661,6 +661,13 @@ static int do_switch(struct intel_ring_buffer *ring, > > > > if (!to->is_initialized || i915_gem_context_is_default(to)) > > > > hw_flags |= MI_RESTORE_INHIBIT; > > > > > > > > + /* When SW intends to use semaphore signaling between Command streamers, > > > > + * it must avoid lite restores in HW by programming "Force Restore" bit > > > > + * to ‘1’ in context descriptor during context submission > > > > + */ > > > > + if (IS_GEN8(ring->dev) && i915_semaphore_is_enabled(ring->dev)) > > > > + hw_flags |= MI_FORCE_RESTORE; > > > > > > Is it not an error to set both FORCE and INHIBIT? > > > -Chris > > > > Read the commit message. > > And for once I was reading the code. Wouldn't it be worth to clear > MI_RESTORE_INHIBIT any way to silence the critics? > -Chris > If we had interest in merging the patch, I can certainly fix it.
diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c index f77b4c1..aa82fb4 100644 --- a/drivers/gpu/drm/i915/i915_gem_context.c +++ b/drivers/gpu/drm/i915/i915_gem_context.c @@ -661,6 +661,13 @@ static int do_switch(struct intel_ring_buffer *ring, if (!to->is_initialized || i915_gem_context_is_default(to)) hw_flags |= MI_RESTORE_INHIBIT; + /* When SW intends to use semaphore signaling between Command streamers, + * it must avoid lite restores in HW by programming "Force Restore" bit + * to ‘1’ in context descriptor during context submission + */ + if (IS_GEN8(ring->dev) && i915_semaphore_is_enabled(ring->dev)) + hw_flags |= MI_FORCE_RESTORE; + ret = mi_set_context(ring, to, hw_flags); if (ret) goto unpin_out;
This appears to not actually be needed on the current code. Just putting it on the ML so we can point bug reports at it later. As pointed out by Ville, the current code is "broken" since we do FORCE_RESTORE, and RESTORE_INHIBIT on the same dword. Anecdotally, this seems fine. Signed-off-by: Ben Widawsky <ben@bwidawsk.net> --- drivers/gpu/drm/i915/i915_gem_context.c | 7 +++++++ 1 file changed, 7 insertions(+)