[Mesa-dev] [PATCH] i965: Make intel_get_param return an int

[Alejandro Piñeiro]

On 13/04/16 01:01, Ben Widawsky wrote:
> On Tue, Apr 12, 2016 at 10:10:35AM +0200, Alejandro Piñeiro wrote:
>>
>> On 11/04/16 18:49, Ben Widawsky wrote:
>>> This will fix the spurious error message: "Failed to query GPU properties."
>>> that was unintentionally added in cc01b63d730.
>>>
>>> This patch changes the function to return an int so that the caller is able to
>>> do stuff based on the return value.
>>>
>>> The equivalent of this patch was in the original series that fixed up the
>>> warning, but I dropped it at the last moment. It is required to make the desired
>>> behavior of not warning when trying to query GPU properties from the kernel
>>> unless there is something the user can do about it.
>>>
>>> NOTE: Broadwell appears to actually have some issue where the kernel returns
>>> ENODEV when it shouldn't be. I will investigate this separately.
>>>
>>> Reported-by: Chris Forbes <chrisf at ijw.co.nz>
>>> Signed-off-by: Ben Widawsky <ben at bwidawsk.net>
>>> ---
>>>  src/mesa/drivers/dri/i965/intel_screen.c | 13 ++++++-------
>>>  1 file changed, 6 insertions(+), 7 deletions(-)
>>>
>>> diff --git a/src/mesa/drivers/dri/i965/intel_screen.c b/src/mesa/drivers/dri/i965/intel_screen.c
>>> index 03e6852..d1e8b68 100644
>>> --- a/src/mesa/drivers/dri/i965/intel_screen.c
>>> +++ b/src/mesa/drivers/dri/i965/intel_screen.c
>>> @@ -932,7 +932,7 @@ static const __DRIextension *intelRobustScreenExtensions[] = {
>>>      NULL
>>>  };
>>>  
>>> -static bool
>>> +static int
>>>  intel_get_param(__DRIscreen *psp, int param, int *value)
>>>  {
>>>     int ret;
>>> @@ -946,17 +946,16 @@ intel_get_param(__DRIscreen *psp, int param, int *value)
>>>     if (ret) {
>>>        if (ret != -EINVAL)
>>>  	 _mesa_warning(NULL, "drm_i915_getparam: %d", ret);
>>> -      return false;
>>>     }
>>>  
>>> -   return true;
>>> +   return ret;
>>>  }
>>>  
>>>  static bool
>>>  intel_get_boolean(__DRIscreen *psp, int param)
>>>  {
>>>     int value = 0;
>>> -   return intel_get_param(psp, param, &value) && value;
>>> +   return (intel_get_param(psp, param, &value) == 0) && value;
>>>  }
>>>  
>>>  static void
>>> @@ -1089,12 +1088,12 @@ intel_detect_sseu(struct intel_screen *intelScreen)
>>>  
>>>     ret = intel_get_param(intelScreen->driScrnPriv, I915_PARAM_SUBSLICE_TOTAL,
>>>                           &intelScreen->subslice_total);
>>> -   if (ret != -EINVAL)
>>> +   if (ret < 0 && ret != -EINVAL)
>>>        goto err_out;
>> Probably Im missing something, as it was the previous behaviour, but as
>> far as I understand drmCommandWriteRead [1] returns negative values on
>> error. So -errno is something goes wrong. The second check goes to
>> err_out if the returned value is not -EINVAL. Why -EINVAL is considered
>> a valid/expected return value?
>>
> -EINVAL is the return value if the getparam doesn't exist, ie. the kernel is too
> old. Everything else is an actual runtime error that is worth reporting ENODEV
> and EFAULT in particular.

Ok, thanks for the explanation. Although Jason already gave the RB with
a suggestion, fwiw:

Reviewed-by: Alejandro Piñeiro <apinheiro at igalia.com>

>
>>>  
>>>     ret = intel_get_param(intelScreen->driScrnPriv,
>>>                           I915_PARAM_EU_TOTAL, &intelScreen->eu_total);
>>> -   if (ret != -EINVAL)
>>> +   if (ret < 0 && ret != -EINVAL)
>>>        goto err_out;
>>>  
>>>     /* Without this information, we cannot get the right Braswell brandstrings,
>>> @@ -1110,7 +1109,7 @@ intel_detect_sseu(struct intel_screen *intelScreen)
>>>  err_out:
>>>     intelScreen->subslice_total = -1;
>>>     intelScreen->eu_total = -1;
>>> -   _mesa_warning(NULL, "Failed to query GPU properties.\n");
>>> +   _mesa_warning(NULL, "Failed to query GPU properties (%d).\n", ret);
>>>  }
>>>  
>>>  static bool
>> [1] https://cgit.freedesktop.org/mesa/drm/tree/xf86drm.c#n2591