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

[Ben Widawsky]

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.

> >  
> >     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

-- 
Ben Widawsky, Intel Open Source Technology Center