[Mesa-dev] [PATCH] i965: Don't enable reset notification support on Gen4-5.

[Ian Romanick]

On 03/13/2014 08:15 AM, Mika Kuoppala wrote:
> Ian Romanick <idr at freedesktop.org> writes:
> 
>> On 03/12/2014 01:43 AM, Kenneth Graunke wrote:
>>> arekm reported that using Chrome with GPU acceleration enabled on GM45
>>> triggered the hw_ctx != NULL assertion in brw_get_graphics_reset_status.
>>>
>>> We definitely do not want to advertise reset notification support on
>>> Gen4-5 systems, since it needs hardware contexts, and we never even
>>> request a hardware context on those systems.
>>>
>>> Cc: Ian Romanick <idr at freedesktop.org>
>>> Signed-off-by: Kenneth Graunke <kenneth at whitecape.org>
>>> ---
>>>  src/mesa/drivers/dri/i965/intel_screen.c | 13 +++++++++----
>>>  1 file changed, 9 insertions(+), 4 deletions(-)
>>>
>>> This may not be the best solution; if someone would like to send patches for
>>> a better one, I'm more than happy to go with something else.  Checking for
>>> hardware contexts is a bit awkward - we need to decide whether to advertise
>>> support here in the screen, but we don't create hardware contexts until
>>> context creation time, which is a fair bit later.  So, I'm not sure how to
>>> do better than the generation check, for now.
>>
>> I'm confused.  I thought the ioctl was was supposed to fail with EINVAL
>> in that case.  Mika, what's your suggestion?
> 
> I think two ioctl queries are needed to catch the combinations,
> unfortunately :(
> 
> For gens without hw_contexts, we keep statistics per file
> descriptor and thus we can ban batch submission through
> file desc if has been sending hanging batches. This is
> tied to reset stats so that if there is no hw_contexts,
> you get statistics for your fd.
> 
> Here is how I do it in tests:

So... in the reset tests, you do something like:

    if (gem_has_hw_contexts(fd) && gem_has_reset_stats(fd))
        do_reset_with_context_test();

I guess that wouldn't be a terrible way to handle it in Mesa...

> static bool gem_has_hw_contexts(int fd)
> {
> 	struct local_drm_i915_gem_context_create create;
> 	int ret;
> 
> 	memset(&create, 0, sizeof(create));
> 	ret = drmIoctl(fd, CONTEXT_CREATE_IOCTL, &create);
> 
> 	if (ret == 0) {
> 		drmIoctl(fd, CONTEXT_DESTROY_IOCTL, &create);
> 		return true;
> 	}
> 
> 	return false;
> }
> 
> static bool gem_has_reset_stats(int fd)
> {
> 	struct local_drm_i915_reset_stats rs;
> 	int ret;
> 
> 	/* Carefully set flags and pad to zero, otherwise
> 	   we get -EINVAL
> 	*/
> 	memset(&rs, 0, sizeof(rs));
> 
> 	ret = drmIoctl(fd, GET_RESET_STATS_IOCTL, &rs);
> 	if (ret == 0)
> 		return true;
> 
> 	/* If we get EPERM, we have support but did not
> 	   have CAP_SYSADM */
> 	if (ret == -1 && errno == EPERM)
> 		return true;
> 
> 	return false;
> }
> 
> -Mika
> 
> 
>>> diff --git a/src/mesa/drivers/dri/i965/intel_screen.c b/src/mesa/drivers/dri/i965/intel_screen.c
>>> index 464cebf..12006be 100644
>>> --- a/src/mesa/drivers/dri/i965/intel_screen.c
>>> +++ b/src/mesa/drivers/dri/i965/intel_screen.c
>>> @@ -1342,13 +1342,18 @@ __DRIconfig **intelInitScreen2(__DRIscreen *psp)
>>>      * no error.  If the ioctl is not supported, it always generate EINVAL.
>>>      * Use this to determine whether to advertise the __DRI2_ROBUSTNESS
>>>      * extension to the loader.
>>> +    *
>>> +    * Don't even try on pre-Gen6, since we don't attempt to use contexts there.
>>>      */
>>> -   struct drm_i915_reset_stats stats;
>>> -   memset(&stats, 0, sizeof(stats));
>>> +   if (intelScreen->devinfo->gen >= 6) {
>>> +      struct drm_i915_reset_stats stats;
>>> +      memset(&stats, 0, sizeof(stats));
>>>  
>>> -   const int ret = drmIoctl(psp->fd, DRM_IOCTL_I915_GET_RESET_STATS, &stats);
>>> +      const int ret = drmIoctl(psp->fd, DRM_IOCTL_I915_GET_RESET_STATS, &stats);
>>>  
>>> -   intelScreen->has_context_reset_notification = (ret != -1 || errno != EINVAL);
>>> +      intelScreen->has_context_reset_notification =
>>> +         (ret != -1 || errno != EINVAL);
>>> +   }
>>>  
>>>     psp->extensions = !intelScreen->has_context_reset_notification
>>>        ? intelScreenExtensions : intelRobustScreenExtensions;