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 <[email protected]> >>> Signed-off-by: Ben Widawsky <[email protected]> >>> --- >>> 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 <[email protected]> > >>> >>> 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 _______________________________________________ mesa-dev mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/mesa-dev
