Re: [RFC v2 3/8] drm: Export some ioctl functions

Daniel Vetter
2018-01-09T10:16:29Z

On Wed, Jan 03, 2018 at 11:21:05PM +0100, Noralf Trønnes wrote:
> Export the following functions so in-kernel users can allocate
> dumb buffers:
> - drm_file_alloc
> - drm_file_free
> - drm_prime_handle_to_fd_ioctl
> - drm_mode_addfb2
> - drm_mode_create_dumb_ioctl
> - drm_dropmaster_ioctl
>
> Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
> ---
> drivers/gpu/drm/drm_auth.c | 1 +
> drivers/gpu/drm/drm_crtc_internal.h | 4 ----
> drivers/gpu/drm/drm_dumb_buffers.c | 1 +
> drivers/gpu/drm/drm_file.c | 2 ++
> drivers/gpu/drm/drm_framebuffer.c | 1 +
> drivers/gpu/drm/drm_internal.h | 6 ------
> drivers/gpu/drm/drm_ioctl.c | 1 +
> drivers/gpu/drm/drm_prime.c | 1 +
> include/drm/drm_auth.h | 3 +++
> include/drm/drm_dumb_buffers.h | 10 ++++++++++
> include/drm/drm_file.h | 2 ++
> include/drm/drm_framebuffer.h | 3 +++
> include/drm/drm_prime.h | 2 ++
> 13 files changed, 27 insertions(+), 10 deletions(-)
> create mode 100644 include/drm/drm_dumb_buffers.h
>
> diff --git a/drivers/gpu/drm/drm_auth.c b/drivers/gpu/drm/drm_auth.c
> index aad468d170a7..e35ed9ee0c5a 100644
> --- a/drivers/gpu/drm/drm_auth.c
> +++ b/drivers/gpu/drm/drm_auth.c
> @@ -236,6 +236,7 @@ int drm_dropmaster_ioctl(struct drm_device *dev, void *data,
> mutex_unlock(&dev->master_mutex);
> return ret;
> }
> +EXPORT_SYMBOL(drm_dropmaster_ioctl);
>
> int drm_master_open(struct drm_file *file_priv)
> {
> diff --git a/drivers/gpu/drm/drm_crtc_internal.h b/drivers/gpu/drm/drm_crtc_internal.h
> index 9ebb8841778c..86422492ad00 100644
> --- a/drivers/gpu/drm/drm_crtc_internal.h
> +++ b/drivers/gpu/drm/drm_crtc_internal.h
> @@ -63,8 +63,6 @@ int drm_mode_getresources(struct drm_device *dev,
>
> /* drm_dumb_buffers.c */
> /* IOCTLs */
> -int drm_mode_create_dumb_ioctl(struct drm_device *dev,
> - void *data, struct drm_file *file_priv);
> int drm_mode_mmap_dumb_ioctl(struct drm_device *dev,
> void *data, struct drm_file *file_priv);
> int drm_mode_destroy_dumb_ioctl(struct drm_device *dev,
> @@ -164,8 +162,6 @@ void drm_fb_release(struct drm_file *file_priv);
> /* IOCTL */
> int drm_mode_addfb(struct drm_device *dev,
> void *data, struct drm_file *file_priv);
> -int drm_mode_addfb2(struct drm_device *dev,
> - void *data, struct drm_file *file_priv);
> int drm_mode_rmfb(struct drm_device *dev,
> void *data, struct drm_file *file_priv);
> int drm_mode_getfb(struct drm_device *dev,
> diff --git a/drivers/gpu/drm/drm_dumb_buffers.c b/drivers/gpu/drm/drm_dumb_buffers.c
> index 39ac15ce4702..199b279f7650 100644
> --- a/drivers/gpu/drm/drm_dumb_buffers.c
> +++ b/drivers/gpu/drm/drm_dumb_buffers.c
> @@ -90,6 +90,7 @@ int drm_mode_create_dumb_ioctl(struct drm_device *dev,
>
> return dev->driver->dumb_create(file_priv, dev, args);
> }
> +EXPORT_SYMBOL(drm_mode_create_dumb_ioctl);
>
> /**
> * drm_mode_mmap_dumb_ioctl - create an mmap offset for a dumb backing storage buffer
> diff --git a/drivers/gpu/drm/drm_file.c b/drivers/gpu/drm/drm_file.c
> index d208faade27e..400d44437e93 100644
> --- a/drivers/gpu/drm/drm_file.c
> +++ b/drivers/gpu/drm/drm_file.c
> @@ -180,6 +180,7 @@ struct drm_file *drm_file_alloc(struct drm_minor *minor)
>
> return ERR_PTR(ret);
> }
> +EXPORT_SYMBOL(drm_file_alloc);
>
> static void drm_events_release(struct drm_file *file_priv)
> {
> @@ -273,6 +274,7 @@ void drm_file_free(struct drm_file *file)
> put_pid(file->pid);
> kfree(file);
> }
> +EXPORT_SYMBOL(drm_file_free);

I'd put the EXPORT_SYMBOL for the drm_file stuff into the first patch.
That way it's next to the kerneldoc and code all in the same patch.

>
> static int drm_setup(struct drm_device * dev)
> {
> diff --git a/drivers/gpu/drm/drm_framebuffer.c b/drivers/gpu/drm/drm_framebuffer.c
> index 5a13ff29f4f0..0493977e6848 100644
> --- a/drivers/gpu/drm/drm_framebuffer.c
> +++ b/drivers/gpu/drm/drm_framebuffer.c
> @@ -341,6 +341,7 @@ int drm_mode_addfb2(struct drm_device *dev,
>
> return 0;
> }
> +EXPORT_SYMBOL(drm_mode_addfb2);
>
> struct drm_mode_rmfb_work {
> struct work_struct work;
> diff --git a/drivers/gpu/drm/drm_internal.h b/drivers/gpu/drm/drm_internal.h
> index 40179c5fc6b8..7d62e412fbb8 100644
> --- a/drivers/gpu/drm/drm_internal.h
> +++ b/drivers/gpu/drm/drm_internal.h
> @@ -26,8 +26,6 @@
>
> /* drm_file.c */
> extern struct mutex drm_global_mutex;
> -struct drm_file *drm_file_alloc(struct drm_minor *minor);
> -void drm_file_free(struct drm_file *file);
> void drm_lastclose(struct drm_device *dev);
>
> /* drm_pci.c */
> @@ -37,8 +35,6 @@ void drm_pci_agp_destroy(struct drm_device *dev);
> int drm_pci_set_busid(struct drm_device *dev, struct drm_master *master);
>
> /* drm_prime.c */
> -int drm_prime_handle_to_fd_ioctl(struct drm_device *dev, void *data,
> - struct drm_file *file_priv);
> int drm_prime_fd_to_handle_ioctl(struct drm_device *dev, void *data,
> struct drm_file *file_priv);
>
> @@ -85,8 +81,6 @@ int drm_authmagic(struct drm_device *dev, void *data,
> struct drm_file *file_priv);
> int drm_setmaster_ioctl(struct drm_device *dev, void *data,
> struct drm_file *file_priv);
> -int drm_dropmaster_ioctl(struct drm_device *dev, void *data,
> - struct drm_file *file_priv);
> int drm_master_open(struct drm_file *file_priv);
> void drm_master_release(struct drm_file *file_priv);
>
> diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c
> index b1e96fb68ea8..ea8f6ca8b449 100644
> --- a/drivers/gpu/drm/drm_ioctl.c
> +++ b/drivers/gpu/drm/drm_ioctl.c
> @@ -31,6 +31,7 @@
> #include <drm/drm_ioctl.h>
> #include <drm/drmP.h>
> #include <drm/drm_auth.h>
> +#include <drm/drm_dumb_buffers.h>
> #include "drm_legacy.h"
> #include "drm_internal.h"
> #include "drm_crtc_internal.h"
> diff --git a/drivers/gpu/drm/drm_prime.c b/drivers/gpu/drm/drm_prime.c
> index 9a17725b0f7a..1bfc7c9d6127 100644
> --- a/drivers/gpu/drm/drm_prime.c
> +++ b/drivers/gpu/drm/drm_prime.c
> @@ -774,6 +774,7 @@ int drm_prime_handle_to_fd_ioctl(struct drm_device *dev, void *data,
> return dev->driver->prime_handle_to_fd(dev, file_priv,
> args->handle, args->flags, &args->fd);
> }
> +EXPORT_SYMBOL(drm_prime_handle_to_fd_ioctl);
>
> int drm_prime_fd_to_handle_ioctl(struct drm_device *dev, void *data,
> struct drm_file *file_priv)
> diff --git a/include/drm/drm_auth.h b/include/drm/drm_auth.h
> index 86bff9841b54..fdc3fed50daf 100644
> --- a/include/drm/drm_auth.h
> +++ b/include/drm/drm_auth.h
> @@ -103,4 +103,7 @@ bool drm_is_current_master(struct drm_file *fpriv);
>
> struct drm_master *drm_master_create(struct drm_device *dev);
>
> +int drm_dropmaster_ioctl(struct drm_device *dev, void *data,
> + struct drm_file *file_priv);

Hm, so this looks reasonable as an internal function except for the void
*data. I think for making ioctl calls available internally we should:

- have the _ioctl variant for the ioctl table, which takes void *data.
That one stays internal to drm.ko
- Move the real implementation into a function without the _ioctl suffix,
which takes the casted pointer (and has kerneldoc and all the neat
things).
- Also maybe let's drop the uapi version suffix internall too, so
drm_addfb instead of drm_addfb2.
- I wonder whether we should drop the drm_device *dev too while at it
(from the internal callback), it's kinda redundant.

All in the name of a neat&tidy internal api.

Besides this polish, looks all reasonable.
-Daniel

> +
> #endif
> diff --git a/include/drm/drm_dumb_buffers.h b/include/drm/drm_dumb_buffers.h
> new file mode 100644
> index 000000000000..c1138c1c06ab
> --- /dev/null
> +++ b/include/drm/drm_dumb_buffers.h
> @@ -0,0 +1,10 @@
> +#ifndef _DRM_DUMB_BUFFERS_H_
> +#define _DRM_DUMB_BUFFERS_H_
> +
> +struct drm_device;
> +struct drm_file;
> +
> +int drm_mode_create_dumb_ioctl(struct drm_device *dev,
> + void *data, struct drm_file *file_priv);
> +
> +#endif
> diff --git a/include/drm/drm_file.h b/include/drm/drm_file.h
> index 0e0c868451a5..23d90744c3d9 100644
> --- a/include/drm/drm_file.h
> +++ b/include/drm/drm_file.h
> @@ -360,6 +360,8 @@ static inline bool drm_is_control_client(const struct drm_file *file_priv)
> return file_priv->minor->type == DRM_MINOR_CONTROL;
> }
>
> +struct drm_file *drm_file_alloc(struct drm_minor *minor);
> +void drm_file_free(struct drm_file *file);
> int drm_open(struct inode *inode, struct file *filp);
> ssize_t drm_read(struct file *filp, char __user *buffer,
> size_t count, loff_t *offset);
> diff --git a/include/drm/drm_framebuffer.h b/include/drm/drm_framebuffer.h
> index c50502c656e5..6c54db1c23ae 100644
> --- a/include/drm/drm_framebuffer.h
> +++ b/include/drm/drm_framebuffer.h
> @@ -313,4 +313,7 @@ int drm_framebuffer_plane_width(int width,
> int drm_framebuffer_plane_height(int height,
> const struct drm_framebuffer *fb, int plane);
>
> +int drm_mode_addfb2(struct drm_device *dev,
> + void *data, struct drm_file *file_priv);
> +
> #endif
> diff --git a/include/drm/drm_prime.h b/include/drm/drm_prime.h
> index 9cd9e36f77b5..ca7bc50af2d7 100644
> --- a/include/drm/drm_prime.h
> +++ b/include/drm/drm_prime.h
> @@ -85,5 +85,7 @@ int drm_prime_sg_to_page_addr_arrays(struct sg_table *sgt, struct page **pages,
> struct sg_table *drm_prime_pages_to_sg(struct page **pages, unsigned int nr_pages);
> void drm_prime_gem_destroy(struct drm_gem_object *obj, struct sg_table *sg);
>
> +int drm_prime_handle_to_fd_ioctl(struct drm_device *dev, void *data,
> + struct drm_file *file_priv);
>
> #endif /* __DRM_PRIME_H__ */
> --
> 2.14.2
>

--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org