From: "Keith Packard" <keithp@keithp.com> To: Sean Paul <seanpaul@chromium.org> Cc: linux-kernel@vger.kernel.org, Dave Airlie <airlied@redhat.com>, Daniel Vetter <daniel@ffwll.ch>, dri-devel@lists.freedesktop.org Subject: Re: [PATCH 5/5] drm: Add four ioctls for managing drm mode object leases [v6] Date: Mon, 16 Oct 2017 14:31:30 -0700 [thread overview] Message-ID: <878tgawzil.fsf@keithp.com> (raw) In-Reply-To: <20171016210315.stpau32hblgxfsgm@art_vandelay> [-- Attachment #1: Type: text/plain, Size: 1938 bytes --] Sean Paul <seanpaul@chromium.org> writes: > nit: space before , Thanks. >> + /* Clone the lessor file to create a new file for us */ >> + DRM_DEBUG_LEASE("Allocating lease file\n"); >> + path_get(&lessor_file->f_path); > > Please forgive the stupid question, but where is this reference given > up? That's not a stupid question, it's a very subtle one which took me quite a while to sort out. Here's path_get: void path_get(const struct path *path) { mntget(path->mnt); dget(path->dentry); } So, getting a reference on a 'path' actually gets a reference on two of the things it points to. alloc_file is passed the path and doesn't take an additional reference on either of these fields, presumably because the normal path has the caller taking a reference while looking up the object and handing that reference off to alloc_file. In our case, we're creating a new file that refers to the same path as an existing one, so we need another reference. When the file is finally freed in __fput, the two references are dropped at the end of the function: static void __fput(struct file *file) { struct dentry *dentry = file->f_path.dentry; struct vfsmount *mnt = file->f_path.mnt; ... dput(dentry); mntput(mnt); } This was probably the twistiest part of creating a lease. All of the DRM stuff was trivial; getting the core kernel object reference counts right was a pain. >> + if (lessee->lessor == NULL) >> + /* owner can use all objects */ >> + object_idr = &lessee->dev->mode_config.crtc_idr; > > What about other types of objects? If I understand your question correctly, the answer is that 'crtc_idr' is misnamed -- it holds all of the mode setting objects. Thanks for your review, let me know if you have more questions! -- -keith [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 832 bytes --]
WARNING: multiple messages have this Message-ID (diff)
From: "Keith Packard" <keithp@keithp.com> To: Sean Paul <seanpaul@chromium.org> Cc: Dave Airlie <airlied@redhat.com>, dri-devel@lists.freedesktop.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH 5/5] drm: Add four ioctls for managing drm mode object leases [v6] Date: Mon, 16 Oct 2017 14:31:30 -0700 [thread overview] Message-ID: <878tgawzil.fsf@keithp.com> (raw) In-Reply-To: <20171016210315.stpau32hblgxfsgm@art_vandelay> [-- Attachment #1.1: Type: text/plain, Size: 1938 bytes --] Sean Paul <seanpaul@chromium.org> writes: > nit: space before , Thanks. >> + /* Clone the lessor file to create a new file for us */ >> + DRM_DEBUG_LEASE("Allocating lease file\n"); >> + path_get(&lessor_file->f_path); > > Please forgive the stupid question, but where is this reference given > up? That's not a stupid question, it's a very subtle one which took me quite a while to sort out. Here's path_get: void path_get(const struct path *path) { mntget(path->mnt); dget(path->dentry); } So, getting a reference on a 'path' actually gets a reference on two of the things it points to. alloc_file is passed the path and doesn't take an additional reference on either of these fields, presumably because the normal path has the caller taking a reference while looking up the object and handing that reference off to alloc_file. In our case, we're creating a new file that refers to the same path as an existing one, so we need another reference. When the file is finally freed in __fput, the two references are dropped at the end of the function: static void __fput(struct file *file) { struct dentry *dentry = file->f_path.dentry; struct vfsmount *mnt = file->f_path.mnt; ... dput(dentry); mntput(mnt); } This was probably the twistiest part of creating a lease. All of the DRM stuff was trivial; getting the core kernel object reference counts right was a pain. >> + if (lessee->lessor == NULL) >> + /* owner can use all objects */ >> + object_idr = &lessee->dev->mode_config.crtc_idr; > > What about other types of objects? If I understand your question correctly, the answer is that 'crtc_idr' is misnamed -- it holds all of the mode setting objects. Thanks for your review, let me know if you have more questions! -- -keith [-- Attachment #1.2: signature.asc --] [-- Type: application/pgp-signature, Size: 832 bytes --] [-- Attachment #2: Type: text/plain, Size: 160 bytes --] _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
next prev parent reply other threads:[~2017-10-16 21:31 UTC|newest] Thread overview: 26+ messages / expand[flat|nested] mbox.gz Atom feed top 2017-10-13 1:56 [PATCH 0/5]: drm: Add drm mode object leases Keith Packard 2017-10-13 1:56 ` Keith Packard 2017-10-13 1:56 ` [PATCH 1/5] drm/plane: drop num_overlay_planes (v2) Keith Packard 2017-10-16 19:23 ` Sean Paul 2017-10-13 1:56 ` [PATCH 2/5] drm: Add new LEASE debug level Keith Packard 2017-10-13 1:56 ` Keith Packard 2017-10-16 19:24 ` Sean Paul 2017-10-13 1:56 ` [PATCH 3/5] drm: Add drm_object lease infrastructure [v4] Keith Packard 2017-10-13 1:56 ` Keith Packard 2017-10-16 19:44 ` Sean Paul 2017-10-16 20:42 ` Keith Packard 2017-10-16 21:05 ` Sean Paul 2017-10-16 21:05 ` Sean Paul 2017-10-13 1:56 ` [PATCH 4/5] drm: Check mode object lease status in all master ioctl paths [v3] Keith Packard 2017-10-13 1:56 ` Keith Packard 2017-10-16 20:34 ` Sean Paul 2017-10-13 1:56 ` [PATCH 5/5] drm: Add four ioctls for managing drm mode object leases [v6] Keith Packard 2017-10-13 1:56 ` Keith Packard 2017-10-16 21:03 ` Sean Paul 2017-10-16 21:03 ` Sean Paul 2017-10-16 21:31 ` Keith Packard [this message] 2017-10-16 21:31 ` Keith Packard 2017-10-16 9:13 ` [PATCH 0/5]: drm: Add drm mode object leases Daniel Vetter 2017-10-16 9:13 ` Daniel Vetter 2017-10-16 17:52 ` Keith Packard 2017-10-16 17:52 ` Keith Packard
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=878tgawzil.fsf@keithp.com \ --to=keithp@keithp.com \ --cc=airlied@redhat.com \ --cc=daniel@ffwll.ch \ --cc=dri-devel@lists.freedesktop.org \ --cc=linux-kernel@vger.kernel.org \ --cc=seanpaul@chromium.org \ /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.