Re: [igt-dev] [PATCH 4/7] drm/lease: Check for lessor outside of locks

[Daniel Vetter <daniel@ffwll.ch>]

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.vetter@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 <keithp@keithp.com>
> > Signed-off-by: Daniel Vetter <daniel.vetter@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.
> 
> >  		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).

Also confused by "also remove", this patch doesn't drop any checks, just
adds them outside of the lock to extend existing fastpaths.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev