From: Boris Brezillon <boris.brezillon@collabora.com> To: Daniel Vetter <daniel@ffwll.ch> Cc: IGT development <igt-dev@lists.freedesktop.org>, Daniel Vetter <daniel.vetter@ffwll.ch>, Keith Packard <keithp@keithp.com>, DRI Development <dri-devel@lists.freedesktop.org> Subject: Re: [PATCH 4/7] drm/lease: Check for lessor outside of locks Date: Wed, 3 Apr 2019 09:50:14 +0200 [thread overview] Message-ID: <20190403095014.52fe11e9@collabora.com> (raw) In-Reply-To: <20190403070403.GN2665@phenom.ffwll.local> On Wed, 3 Apr 2019 09:04:03 +0200 Daniel Vetter <daniel@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.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. 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.brezillon@collabora.com> _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
WARNING: multiple messages have this Message-ID (diff)
From: Boris Brezillon <boris.brezillon@collabora.com> To: Daniel Vetter <daniel@ffwll.ch> Cc: IGT development <igt-dev@lists.freedesktop.org>, Keith Packard <keithp@keithp.com>, DRI Development <dri-devel@lists.freedesktop.org> Subject: Re: [igt-dev] [PATCH 4/7] drm/lease: Check for lessor outside of locks Date: Wed, 3 Apr 2019 09:50:14 +0200 [thread overview] Message-ID: <20190403095014.52fe11e9@collabora.com> (raw) In-Reply-To: <20190403070403.GN2665@phenom.ffwll.local> On Wed, 3 Apr 2019 09:04:03 +0200 Daniel Vetter <daniel@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.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. 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.brezillon@collabora.com> _______________________________________________ igt-dev mailing list igt-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/igt-dev
next prev parent reply other threads:[~2019-04-03 7:50 UTC|newest] Thread overview: 45+ messages / expand[flat|nested] mbox.gz Atom feed top 2019-02-28 14:49 [PATCH 0/7] some cleanups and uapi clarification for leases Daniel Vetter 2019-02-28 14:49 ` [igt-dev] " Daniel Vetter 2019-02-28 14:49 ` [PATCH 1/7] drm/leases: Drop object_id validation for negative ids Daniel Vetter 2019-02-28 14:49 ` [igt-dev] " Daniel Vetter 2019-03-14 7:54 ` Boris Brezillon 2019-03-14 7:54 ` [igt-dev] " Boris Brezillon 2019-03-29 4:46 ` Dave Airlie 2019-03-29 4:46 ` [igt-dev] " Dave Airlie 2019-03-29 8:28 ` Daniel Vetter 2019-03-29 8:28 ` [igt-dev] " Daniel Vetter 2019-02-28 14:49 ` [PATCH 2/7] drm/lease: Drop recursive leads checks Daniel Vetter 2019-02-28 14:49 ` [igt-dev] " Daniel Vetter 2019-03-14 8:44 ` Boris Brezillon 2019-03-14 8:44 ` [igt-dev] " Boris Brezillon 2019-02-28 14:49 ` [PATCH 3/7] drm/leases: Don't init to 0 in drm_master_create Daniel Vetter 2019-02-28 14:49 ` [igt-dev] " Daniel Vetter 2019-03-14 7:56 ` Boris Brezillon 2019-03-14 7:56 ` [igt-dev] " Boris Brezillon 2019-03-29 4:47 ` Dave Airlie 2019-03-29 4:47 ` [igt-dev] " Dave Airlie 2019-02-28 14:49 ` [PATCH 4/7] drm/lease: Check for lessor outside of locks Daniel Vetter 2019-02-28 14:49 ` [igt-dev] " Daniel Vetter 2019-03-14 8:07 ` Boris Brezillon 2019-03-14 8:07 ` [igt-dev] " Boris Brezillon 2019-04-03 1:33 ` Dave Airlie 2019-04-03 1:33 ` [igt-dev] " Dave Airlie 2019-04-03 7:04 ` Daniel Vetter 2019-04-03 7:04 ` [igt-dev] " Daniel Vetter 2019-04-03 7:50 ` Boris Brezillon [this message] 2019-04-03 7:50 ` Boris Brezillon 2019-02-28 14:49 ` [PATCH 5/7] drm/lease: Make sure implicit planes are leased Daniel Vetter 2019-02-28 14:49 ` [igt-dev] " Daniel Vetter 2019-03-05 13:35 ` Sasha Levin 2019-03-05 13:35 ` Sasha Levin 2019-02-28 14:49 ` [PATCH 6/7] drm/atomic: Wire file_priv through for property changes Daniel Vetter 2019-02-28 14:49 ` [igt-dev] " Daniel Vetter 2019-02-28 14:49 ` [PATCH 7/7] drm/atomic: -EACCESS for lease-denied crtc lookup Daniel Vetter 2019-02-28 14:49 ` [igt-dev] " Daniel Vetter 2019-03-14 8:58 ` [PATCH 0/7] some cleanups and uapi clarification for leases Boris Brezillon 2019-03-14 8:58 ` [igt-dev] " Boris Brezillon 2019-04-05 2:40 ` Dave Airlie 2019-04-05 2:40 ` [igt-dev] " Dave Airlie 2019-04-24 9:31 ` Daniel Vetter 2019-04-24 9:31 ` [igt-dev] " Daniel Vetter 2019-02-28 17:00 Daniel Vetter 2019-02-28 17:00 ` [PATCH 4/7] drm/lease: Check for lessor outside of locks Daniel Vetter
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=20190403095014.52fe11e9@collabora.com \ --to=boris.brezillon@collabora.com \ --cc=daniel.vetter@ffwll.ch \ --cc=daniel@ffwll.ch \ --cc=dri-devel@lists.freedesktop.org \ --cc=igt-dev@lists.freedesktop.org \ --cc=keithp@keithp.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.