[Intel-gfx] [PATCH 1/3] drm/i915: Skip the ERR_PTR error state

[Chris Wilson]

Quoting Tvrtko Ursulin (2018-12-06 12:57:23)
> 
> On 06/12/2018 08:44, Chris Wilson wrote:
> > diff --git a/drivers/gpu/drm/i915/i915_gpu_error.c b/drivers/gpu/drm/i915/i915_gpu_error.c
> > index 07465123c166..7ca6f3f31d41 100644
> > --- a/drivers/gpu/drm/i915/i915_gpu_error.c
> > +++ b/drivers/gpu/drm/i915/i915_gpu_error.c
> > @@ -1907,6 +1907,11 @@ i915_capture_gpu_state(struct drm_i915_private *i915)
> >   {
> >       struct i915_gpu_state *error;
> >   
> > +     /* Check if GPU capture has been disabled */
> > +     error = READ_ONCE(i915->gpu_error.first_error);
> > +     if (IS_ERR(error))
> > +             return error;
> > +
> >       error = kzalloc(sizeof(*error), GFP_ATOMIC);
> >       if (!error)
> >               return NULL;
> 
> Looks like it would be cleaner to return ERR_PTR(-ENOMEM) here. At least 
> the i915_gpu_info_open hunk would be.
> 
> Then in drm-tip I see i915_capture_error_state as a caller which needs 
> to handle ERR_PTR now as well, no?

It shouldn't due to the earlier check, but it would work better if it
tool the ERR_PTR pointer from here.

> > @@ -1987,7 +1992,7 @@ i915_first_error_state(struct drm_i915_private *i915)
> >   
> >       spin_lock_irq(&i915->gpu_error.lock);
> >       error = i915->gpu_error.first_error;
> > -     if (error)
> > +     if (!IS_ERR_OR_NULL(error))
> >               i915_gpu_state_get(error);
> >       spin_unlock_irq(&i915->gpu_error.lock);
> >   
> > @@ -2000,10 +2005,11 @@ void i915_reset_error_state(struct drm_i915_private *i915)
> >   
> >       spin_lock_irq(&i915->gpu_error.lock);
> >       error = i915->gpu_error.first_error;
> > -     i915->gpu_error.first_error = NULL;
> > +     if (!IS_ERR_OR_NULL(error)) /* once disabled, always disabled */
> 
> Hm, this will apply on transient -ENOMEM as well.

Yeah. The best compromise is to say only ENODEV is special.

> > +             i915->gpu_error.first_error = NULL;
> >       spin_unlock_irq(&i915->gpu_error.lock);
> >   
> > -     if (!IS_ERR(error))
> > +     if (!IS_ERR_OR_NULL(error))
> >               i915_gpu_state_put(error);
> >   }
> >   
> > diff --git a/drivers/gpu/drm/i915/i915_sysfs.c b/drivers/gpu/drm/i915/i915_sysfs.c
> > index 535caebd9813..370b7497e9df 100644
> > --- a/drivers/gpu/drm/i915/i915_sysfs.c
> > +++ b/drivers/gpu/drm/i915/i915_sysfs.c
> > @@ -521,6 +521,9 @@ static ssize_t error_state_read(struct file *filp, struct kobject *kobj,
> >       ssize_t ret;
> >   
> >       gpu = i915_first_error_state(i915);
> > +     if (IS_ERR(gpu))
> > +             return PTR_ERR(gpu);
> > +
> >       if (gpu) {
> >               ret = i915_gpu_state_copy_to_buffer(gpu, buf, off, count);
> >               i915_gpu_state_put(gpu);#
> > 
> 
> A single control block:
> 
> if (!IS_ERR_OR_NULL) {
>         ...
> } else {
>         ...
> }
> 
> seems like it would do the trick.

Not quite unless you were thinking of if { } else if { } else { };
-Chris