| Message ID | 20191106100716.18181-1-chris@chris-wilson.co.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/3] drm: Expose a method for creating anonymous struct file around drm_minor | expand |
On Wed, Nov 06, 2019 at 10:07:14AM +0000, Chris Wilson wrote: > Sometimes we need to create a struct file to wrap a drm_device, as it > the user were to have opened /dev/dri/card0 but to do so anonymously > (i.e. for internal use). Provide a utility method to create a struct > file with the drm_device->driver.fops, that wrap the drm_device. > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> For proper internal access we already have drm_client_open, so I think this has limited (but good use) in selftests only. So EXPORT_SYMBOL_FOR_TESTS_ONLY plus maybe a clearer name for the intended use like drm_file_mock_open? Aside from this I like. -Daniel > --- > drivers/gpu/drm/drm_file.c | 39 ++++++++++++++++++++++++++++++++++++++ > include/drm/drm_file.h | 3 +++ > 2 files changed, 42 insertions(+) > > diff --git a/drivers/gpu/drm/drm_file.c b/drivers/gpu/drm/drm_file.c > index ea34bc991858..cc55c0cd5826 100644 > --- a/drivers/gpu/drm/drm_file.c > +++ b/drivers/gpu/drm/drm_file.c > @@ -31,7 +31,9 @@ > * OTHER DEALINGS IN THE SOFTWARE. > */ > > +#include <linux/anon_inodes.h> > #include <linux/dma-fence.h> > +#include <linux/file.h> > #include <linux/module.h> > #include <linux/pci.h> > #include <linux/poll.h> > @@ -754,3 +756,40 @@ void drm_send_event(struct drm_device *dev, struct drm_pending_event *e) > spin_unlock_irqrestore(&dev->event_lock, irqflags); > } > EXPORT_SYMBOL(drm_send_event); > + > +/** > + * anon_drm_getfile - Create a new struct file for the drm device > + * @minor: drm minor to wrap (e.g. drm_device->primary) > + * @flags: file creation mode (O_RDWR etc) > + * > + * This create a new struct file that wraps a DRM file context around a > + * DRM minor. This mimicks userspace opening e.g. /dev/dri/card0, but without > + * invoking userspace. The struct file may be operated on using its f_op > + * (the drm_device.driver.fops) to mimick userspace operations, or be supplied > + * to userspace facing functions as an internal/anonymous client. > + * > + * RETURNS: > + * Pointer to newly created struct file, ERR_PTR on failure. > + */ > +struct file *anon_drm_getfile(struct drm_minor *minor, unsigned int flags) > +{ > + struct drm_device *dev = minor->dev; > + struct drm_file *priv; > + struct file *file; > + > + priv = drm_file_alloc(minor); > + if (IS_ERR(priv)) > + return ERR_CAST(priv); > + > + file = anon_inode_getfile("drm", dev->driver->fops, priv, flags); > + if (IS_ERR(file)) { > + drm_file_free(priv); > + return file; > + } > + > + drm_dev_get(dev); > + priv->filp = file; > + > + return file; > +} > +EXPORT_SYMBOL(anon_drm_getfile); > diff --git a/include/drm/drm_file.h b/include/drm/drm_file.h > index 67af60bb527a..b963535964f7 100644 > --- a/include/drm/drm_file.h > +++ b/include/drm/drm_file.h > @@ -42,6 +42,7 @@ struct dma_fence; > struct drm_file; > struct drm_device; > struct device; > +struct file; > > /* > * FIXME: Not sure we want to have drm_minor here in the end, but to avoid > @@ -387,4 +388,6 @@ void drm_event_cancel_free(struct drm_device *dev, > void drm_send_event_locked(struct drm_device *dev, struct drm_pending_event *e); > void drm_send_event(struct drm_device *dev, struct drm_pending_event *e); > > +struct file *anon_drm_getfile(struct drm_minor *minor, unsigned int flags); > + > #endif /* _DRM_FILE_H_ */ > -- > 2.24.0 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Quoting Daniel Vetter (2019-11-06 10:19:50) > On Wed, Nov 06, 2019 at 10:07:14AM +0000, Chris Wilson wrote: > > Sometimes we need to create a struct file to wrap a drm_device, as it > > the user were to have opened /dev/dri/card0 but to do so anonymously > > (i.e. for internal use). Provide a utility method to create a struct > > file with the drm_device->driver.fops, that wrap the drm_device. > > > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > > For proper internal access we already have drm_client_open, so I think > this has limited (but good use) in selftests only. So > EXPORT_SYMBOL_FOR_TESTS_ONLY plus maybe a clearer name for the intended > use like drm_file_mock_open? I found the example in drm_gem_prime_mmap() that was doing the same trick, and the trick of being able to instantiate new struct file and install a fd whenever seems like it will come in handy... Just lacking the third user at the moment to claim generality. -Chris
Quoting Chris Wilson (2019-11-06 10:26:48) > Quoting Daniel Vetter (2019-11-06 10:19:50) > > On Wed, Nov 06, 2019 at 10:07:14AM +0000, Chris Wilson wrote: > > > Sometimes we need to create a struct file to wrap a drm_device, as it > > > the user were to have opened /dev/dri/card0 but to do so anonymously > > > (i.e. for internal use). Provide a utility method to create a struct > > > file with the drm_device->driver.fops, that wrap the drm_device. > > > > > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > > > > For proper internal access we already have drm_client_open, so I think > > this has limited (but good use) in selftests only. So > > EXPORT_SYMBOL_FOR_TESTS_ONLY plus maybe a clearer name for the intended > > use like drm_file_mock_open? > > I found the example in drm_gem_prime_mmap() that was doing the same trick, > and the trick of being able to instantiate new struct file and install a > fd whenever seems like it will come in handy... Just lacking the third > user at the moment to claim generality. The closest example I found in the spirit of creating a new drm_device struct file and installing it is drm_mode_create_lease_ioctl() that uses file_clone_open() for this purpose. The argument there would be whether cloning the (file->f_path, file->f_flags, file->f_cred) is appropriate versus an anonymous inode. I think cloning the credentials seems correct for leasing. -Chris
On Wed, Nov 6, 2019 at 11:43 AM Chris Wilson <chris@chris-wilson.co.uk> wrote: > > Quoting Chris Wilson (2019-11-06 10:26:48) > > Quoting Daniel Vetter (2019-11-06 10:19:50) > > > On Wed, Nov 06, 2019 at 10:07:14AM +0000, Chris Wilson wrote: > > > > Sometimes we need to create a struct file to wrap a drm_device, as it > > > > the user were to have opened /dev/dri/card0 but to do so anonymously > > > > (i.e. for internal use). Provide a utility method to create a struct > > > > file with the drm_device->driver.fops, that wrap the drm_device. > > > > > > > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > > > > > > For proper internal access we already have drm_client_open, so I think > > > this has limited (but good use) in selftests only. So > > > EXPORT_SYMBOL_FOR_TESTS_ONLY plus maybe a clearer name for the intended > > > use like drm_file_mock_open? > > > > I found the example in drm_gem_prime_mmap() that was doing the same trick, > > and the trick of being able to instantiate new struct file and install a > > fd whenever seems like it will come in handy... Just lacking the third > > user at the moment to claim generality. > > The closest example I found in the spirit of creating a new drm_device > struct file and installing it is drm_mode_create_lease_ioctl() that uses > file_clone_open() for this purpose. The argument there would be whether > cloning the (file->f_path, file->f_flags, file->f_cred) is appropriate > versus an anonymous inode. I think cloning the credentials seems correct > for leasing. Hm ... I think we want the clone for this one here too. Otherwise we get the wrong inode, and then unmap_mapping_range wont work correctly, and we cant use this for selftests. That's the only case where I think we do actually need the file/inode to be functional. For anything else the drm_client internal api gets away without the file/inode stuff. -Daniel
diff --git a/drivers/gpu/drm/drm_file.c b/drivers/gpu/drm/drm_file.c index ea34bc991858..cc55c0cd5826 100644 --- a/drivers/gpu/drm/drm_file.c +++ b/drivers/gpu/drm/drm_file.c @@ -31,7 +31,9 @@ * OTHER DEALINGS IN THE SOFTWARE. */ +#include <linux/anon_inodes.h> #include <linux/dma-fence.h> +#include <linux/file.h> #include <linux/module.h> #include <linux/pci.h> #include <linux/poll.h> @@ -754,3 +756,40 @@ void drm_send_event(struct drm_device *dev, struct drm_pending_event *e) spin_unlock_irqrestore(&dev->event_lock, irqflags); } EXPORT_SYMBOL(drm_send_event); + +/** + * anon_drm_getfile - Create a new struct file for the drm device + * @minor: drm minor to wrap (e.g. drm_device->primary) + * @flags: file creation mode (O_RDWR etc) + * + * This create a new struct file that wraps a DRM file context around a + * DRM minor. This mimicks userspace opening e.g. /dev/dri/card0, but without + * invoking userspace. The struct file may be operated on using its f_op + * (the drm_device.driver.fops) to mimick userspace operations, or be supplied + * to userspace facing functions as an internal/anonymous client. + * + * RETURNS: + * Pointer to newly created struct file, ERR_PTR on failure. + */ +struct file *anon_drm_getfile(struct drm_minor *minor, unsigned int flags) +{ + struct drm_device *dev = minor->dev; + struct drm_file *priv; + struct file *file; + + priv = drm_file_alloc(minor); + if (IS_ERR(priv)) + return ERR_CAST(priv); + + file = anon_inode_getfile("drm", dev->driver->fops, priv, flags); + if (IS_ERR(file)) { + drm_file_free(priv); + return file; + } + + drm_dev_get(dev); + priv->filp = file; + + return file; +} +EXPORT_SYMBOL(anon_drm_getfile); diff --git a/include/drm/drm_file.h b/include/drm/drm_file.h index 67af60bb527a..b963535964f7 100644 --- a/include/drm/drm_file.h +++ b/include/drm/drm_file.h @@ -42,6 +42,7 @@ struct dma_fence; struct drm_file; struct drm_device; struct device; +struct file; /* * FIXME: Not sure we want to have drm_minor here in the end, but to avoid @@ -387,4 +388,6 @@ void drm_event_cancel_free(struct drm_device *dev, void drm_send_event_locked(struct drm_device *dev, struct drm_pending_event *e); void drm_send_event(struct drm_device *dev, struct drm_pending_event *e); +struct file *anon_drm_getfile(struct drm_minor *minor, unsigned int flags); + #endif /* _DRM_FILE_H_ */
Sometimes we need to create a struct file to wrap a drm_device, as it the user were to have opened /dev/dri/card0 but to do so anonymously (i.e. for internal use). Provide a utility method to create a struct file with the drm_device->driver.fops, that wrap the drm_device. Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> --- drivers/gpu/drm/drm_file.c | 39 ++++++++++++++++++++++++++++++++++++++ include/drm/drm_file.h | 3 +++ 2 files changed, 42 insertions(+)