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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
