Re: [PATCH 4/5] drm: v3d: Switch to use drm_gem_object reservation_object

From: Rob Herring <robh@kernel.org>
To: Eric Anholt <eric@anholt.net>
Cc: Sean Paul <sean@poorly.run>, David Airlie <airlied@linux.ie>,
	linux-arm-msm <linux-arm-msm@vger.kernel.org>,
	etnaviv@lists.freedesktop.org,
	dri-devel <dri-devel@lists.freedesktop.org>,
	Maxime Ripard <maxime.ripard@bootlin.com>,
	Russell King <linux+etnaviv@armlinux.org.uk>,
	freedreno <freedreno@lists.freedesktop.org>
Subject: Re: [PATCH 4/5] drm: v3d: Switch to use drm_gem_object reservation_object
Date: Fri, 1 Feb 2019 11:38:25 -0600	[thread overview]
Message-ID: <CAL_JsqKxnr7k+U-6XOiwoATSzUA=+dzPQO-O12j0tcuBYVjicA@mail.gmail.com> (raw)
In-Reply-To: <87tvhn4emy.fsf@anholt.net>

On Fri, Feb 1, 2019 at 11:12 AM Eric Anholt <eric@anholt.net> wrote:
>
> Rob Herring <robh@kernel.org> writes:
>
> > Now that the base struct drm_gem_object has a reservation_object, use it
> > and remove the private BO one.
> >
> > Cc: Eric Anholt <eric@anholt.net>
> > Cc: Daniel Vetter <daniel@ffwll.ch>
> > Cc: David Airlie <airlied@linux.ie>
> > Cc: dri-devel@lists.freedesktop.org
> > Signed-off-by: Rob Herring <robh@kernel.org>
>
> > @@ -453,8 +453,6 @@ v3d_wait_bo_ioctl(struct drm_device *dev, void *data,
> >  {
> >       int ret;
> >       struct drm_v3d_wait_bo *args = data;
> > -     struct drm_gem_object *gem_obj;
> > -     struct v3d_bo *bo;
> >       ktime_t start = ktime_get();
> >       u64 delta_ns;
> >       unsigned long timeout_jiffies =
> > @@ -463,21 +461,8 @@ v3d_wait_bo_ioctl(struct drm_device *dev, void *data,
> >       if (args->pad != 0)
> >               return -EINVAL;
> >
> > -     gem_obj = drm_gem_object_lookup(file_priv, args->handle);
> > -     if (!gem_obj) {
> > -             DRM_DEBUG("Failed to look up GEM BO %d\n", args->handle);
> > -             return -EINVAL;
> > -     }
> > -     bo = to_v3d_bo(gem_obj);
> > -
> > -     ret = reservation_object_wait_timeout_rcu(bo->resv,
> > -                                               true, true,
> > -                                               timeout_jiffies);
> > -
> > -     if (ret == 0)
> > -             ret = -ETIME;
> > -     else if (ret > 0)
> > -             ret = 0;
> > +     ret = drm_gem_reservation_object_wait(file_priv, args->handle,
> > +                                           true, timeout_jiffies);
>
> Looks like you lost my ret handling in the change.

Indeed as initially I had moved that internal.

> Honestly, I'd love to see drm_gem_reservation_object_wait return
> sensible values like this (0 for success, -ETIME for timeouts) instead
> of drivers having to stubmle over this API.

Me too, but then I found some drivers use ETIMEDOUT instead. I didn't
really want to have to fixup one value to the other and gave up. They
also pass an absolute timeout instead.

Rob
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel