On Wed, 3 Apr 2019 09:04:03 +0200
Daniel Vetter <dan...@ffwll.ch> wrote:
> On Thu, Mar 14, 2019 at 09:07:25AM +0100, Boris Brezillon wrote:
> > On Thu, 28 Feb 2019 15:49:07 +0100
> > Daniel Vetter <daniel.vet...@ffwll.ch> wrote:
> >
> > > The lessor is invariant over a lifetime of a lease, we don't have to
> > > grab any locks for that. Speeds up the common case of not being a lease.
> > >
> > > Cc: Keith Packard <kei...@keithp.com>
> > > Signed-off-by: Daniel Vetter <daniel.vet...@ffwll.ch>
> > > ---
> > > drivers/gpu/drm/drm_lease.c | 6 +++---
> > > 1 file changed, 3 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/drm_lease.c b/drivers/gpu/drm/drm_lease.c
> > > index cce5d9dd52ff..694ff363a90b 100644
> > > --- a/drivers/gpu/drm/drm_lease.c
> > > +++ b/drivers/gpu/drm/drm_lease.c
> > > @@ -111,7 +111,7 @@ static bool _drm_has_leased(struct drm_master
> > > *master, int id)
> > > */
> > > bool _drm_lease_held(struct drm_file *file_priv, int id)
> > > {
> > > - if (file_priv == NULL || file_priv->master == NULL)
> > > + if (!file_priv || !file_priv->master)
> >
> > Looks like you're doing unrelated cosmetic changes in the same patch.
> > Maybe mention that in the commit message, or move that to a separate
> > patch.
>
> The next hunk would have broken the 80 limit with the == NULL check, so I
> changed those for consistency too because ocd. I can mention that in the
> commit message.
Okay.
> >
> > > return true;
> > >
> > > return _drm_lease_held_master(file_priv->master, id);
> > > @@ -133,7 +133,7 @@ bool drm_lease_held(struct drm_file *file_priv, int
> > > id)
> > > struct drm_master *master;
> > > bool ret;
> > >
> > > - if (file_priv == NULL || file_priv->master == NULL)
> > > + if (!file_priv || !file_priv->master || !file_priv->master->lessor)
> > > return true;
> > >
> > > master = file_priv->master;
> > > @@ -159,7 +159,7 @@ uint32_t drm_lease_filter_crtcs(struct drm_file
> > > *file_priv, uint32_t crtcs_in)
> > > int count_in, count_out;
> > > uint32_t crtcs_out = 0;
> > >
> > > - if (file_priv == NULL || file_priv->master == NULL)
> > > + if (!file_priv || !file_priv->master || !file_priv->master->lessor)
> > > return crtcs_in;
> > >
> > > master = file_priv->master;
> >
> > Couldn't we also remove the if (master->lessor) check done in
> > _drm_lease_held_master()?
>
> How? For !master->lessor (which is the case for all top-level masters,
> i.e. one created by opening the /dev node and not through the create lease
> ioctl) there's no lease idr. These are handled by the unconditional return
> true. And there's some call chains leading to this which don't first check
> for master->lessor (the object find stuff through _drm_lease_held).
I had the impression that all callers of _drm_lease_held_master() were
now doing the necessary checks to guarantee that master->lessor == NULL
cannot happen in _drm_lease_held_master(). But it seems that
_drm_lease_held() and drm_lease_create() do not check the value of
master->lessor.
>
> Also confused by "also remove", this patch doesn't drop any checks, just
> adds them outside of the lock to extend existing fastpaths.
Yes, I meant "remove" not "also remove". Sorry for the confusion.
FWIW, here is my
Reviewed-by: Boris Brezillon <boris.brezil...@collabora.com>
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
