[Mesa-dev] i965: Properly return *RESET* status in glGetGraphicsResetStatusARB

Submitted by Pavel Popov on May 16, 2014, 5 a.m.

Details

Message ID 1400216402-18791-1-git-send-email-pavel.e.popov@intel.com
State New
Headers show

Not browsing as part of any series.

Commit Message

Pavel Popov May 16, 2014, 5 a.m.
The glGetGraphicsResetStatusARB from ARB_robustness extension always returns GUILTY_CONTEXT_RESET_ARB
and never returns NO_ERROR for guilty context with LOSE_CONTEXT_ON_RESET_ARB strategy.
This is because Mesa returns GUILTY_CONTEXT_RESET_ARB if batch_active !=0 whereas kernel driver
never reset batch_active and this variable always > 0 for guilty context.
The same behaviour also can be observed for batch_pending and INNOCENT_CONTEXT_RESET_ARB.

But spec says the following (http://www.opengl.org/registry/specs/ARB/robustness.txt):

  If a reset status other than NO_ERROR is returned and subsequent
  calls return NO_ERROR, the context reset was encountered and
  completed. If a reset status is repeatedly returned, the context may
  be in the process of resetting.

  8. How should the application react to a reset context event?
  RESOLVED: For this extension, the application is expected to query
  the reset status until NO_ERROR is returned. If a reset is encountered,
  at least one *RESET* status will be returned. Once NO_ERROR is
  encountered, the application can safely destroy the old context and
  create a new one.

The main problem is the context may be in the process of resetting and in this case
a reset status should be repeatedly returned.
But looks like the kernel driver returns nonzero active/pending only if the context
reset has already been encountered and completed.
For this reason the *RESET* status cannot be repeatedly returned and should be returned only once.

The reset_count and brw->reset_count variables can be used to control that glGetGraphicsResetStatusARB
returns *RESET* status only once for each context.
Note the i915 triggers reset_count twice which allows to return correct reset count immediately after
active/pending have been incremented.

Signed-off-by: Pavel Popov <pavel.e.popov@intel.com>
---
 src/mesa/drivers/dri/i965/brw_reset.c | 19 ++++++++++++++-----
 1 file changed, 14 insertions(+), 5 deletions(-)

Patch hide | download patch | download mbox

diff --git a/src/mesa/drivers/dri/i965/brw_reset.c b/src/mesa/drivers/dri/i965/brw_reset.c
index 7eca1bc..d2d051e 100644
--- a/src/mesa/drivers/dri/i965/brw_reset.c
+++ b/src/mesa/drivers/dri/i965/brw_reset.c
@@ -42,6 +42,14 @@  brw_get_graphics_reset_status(struct gl_context *ctx)
     */
    assert(brw->hw_ctx != NULL);
 
+   /*
+   * A reset status other than NO_ERROR was returned last time. I915 returns
+   * nonzero active/pending only if reset has been encountered and completed.
+   * Return NO_ERROR from now on.
+   */
+   if (brw->reset_count != 0)
+      return GL_NO_ERROR;
+
    err = drm_intel_get_reset_stats(brw->hw_ctx, &reset_count, &active,
                                    &pending);
    if (err)
@@ -50,18 +58,19 @@  brw_get_graphics_reset_status(struct gl_context *ctx)
    /* A reset was observed while a batch from this context was executing.
     * Assume that this context was at fault.
     */
-   if (active != 0)
+   if (active != 0) {
+      brw->reset_count = reset_count;
       return GL_GUILTY_CONTEXT_RESET_ARB;
+   }
 
    /* A reset was observed while a batch from this context was in progress,
     * but the batch was not executing.  In this case, assume that the context
     * was not at fault.
     */
-   if (pending != 0)
+   if (pending != 0) {
+      brw->reset_count = reset_count;
       return GL_INNOCENT_CONTEXT_RESET_ARB;
-
-   /* FINISHME: Should we report anything if reset_count > brw->reset_count?
-    */
+   }
 
    return GL_NO_ERROR;
 }

Comments

I made a couple tweaks to the formatting and pushed.  Thanks!

This is also queued for 10.2.  It should be in release candidate 4 later
today.

On 05/15/2014 10:00 PM, Pavel Popov wrote:
> The glGetGraphicsResetStatusARB from ARB_robustness extension always returns GUILTY_CONTEXT_RESET_ARB
> and never returns NO_ERROR for guilty context with LOSE_CONTEXT_ON_RESET_ARB strategy.
> This is because Mesa returns GUILTY_CONTEXT_RESET_ARB if batch_active !=0 whereas kernel driver
> never reset batch_active and this variable always > 0 for guilty context.
> The same behaviour also can be observed for batch_pending and INNOCENT_CONTEXT_RESET_ARB.
> 
> But spec says the following (http://www.opengl.org/registry/specs/ARB/robustness.txt):
> 
>   If a reset status other than NO_ERROR is returned and subsequent
>   calls return NO_ERROR, the context reset was encountered and
>   completed. If a reset status is repeatedly returned, the context may
>   be in the process of resetting.
> 
>   8. How should the application react to a reset context event?
>   RESOLVED: For this extension, the application is expected to query
>   the reset status until NO_ERROR is returned. If a reset is encountered,
>   at least one *RESET* status will be returned. Once NO_ERROR is
>   encountered, the application can safely destroy the old context and
>   create a new one.
> 
> The main problem is the context may be in the process of resetting and in this case
> a reset status should be repeatedly returned.
> But looks like the kernel driver returns nonzero active/pending only if the context
> reset has already been encountered and completed.
> For this reason the *RESET* status cannot be repeatedly returned and should be returned only once.
> 
> The reset_count and brw->reset_count variables can be used to control that glGetGraphicsResetStatusARB
> returns *RESET* status only once for each context.
> Note the i915 triggers reset_count twice which allows to return correct reset count immediately after
> active/pending have been incremented.
> 
> Signed-off-by: Pavel Popov <pavel.e.popov@intel.com>
> ---
>  src/mesa/drivers/dri/i965/brw_reset.c | 19 ++++++++++++++-----
>  1 file changed, 14 insertions(+), 5 deletions(-)
> 
> diff --git a/src/mesa/drivers/dri/i965/brw_reset.c b/src/mesa/drivers/dri/i965/brw_reset.c
> index 7eca1bc..d2d051e 100644
> --- a/src/mesa/drivers/dri/i965/brw_reset.c
> +++ b/src/mesa/drivers/dri/i965/brw_reset.c
> @@ -42,6 +42,14 @@ brw_get_graphics_reset_status(struct gl_context *ctx)
>      */
>     assert(brw->hw_ctx != NULL);
>  
> +   /*
> +   * A reset status other than NO_ERROR was returned last time. I915 returns
> +   * nonzero active/pending only if reset has been encountered and completed.
> +   * Return NO_ERROR from now on.
> +   */
> +   if (brw->reset_count != 0)
> +      return GL_NO_ERROR;
> +
>     err = drm_intel_get_reset_stats(brw->hw_ctx, &reset_count, &active,
>                                     &pending);
>     if (err)
> @@ -50,18 +58,19 @@ brw_get_graphics_reset_status(struct gl_context *ctx)
>     /* A reset was observed while a batch from this context was executing.
>      * Assume that this context was at fault.
>      */
> -   if (active != 0)
> +   if (active != 0) {
> +      brw->reset_count = reset_count;
>        return GL_GUILTY_CONTEXT_RESET_ARB;
> +   }
>  
>     /* A reset was observed while a batch from this context was in progress,
>      * but the batch was not executing.  In this case, assume that the context
>      * was not at fault.
>      */
> -   if (pending != 0)
> +   if (pending != 0) {
> +      brw->reset_count = reset_count;
>        return GL_INNOCENT_CONTEXT_RESET_ARB;
> -
> -   /* FINISHME: Should we report anything if reset_count > brw->reset_count?
> -    */
> +   }
>  
>     return GL_NO_ERROR;
>  }
>