[Intel-gfx] [RFC v2 3/8] drm: Export some ioctl functions

David Herrmann dh.herrmann at gmail.com
Tue Jan 9 10:31:15 UTC 2018


Hi

On Tue, Jan 9, 2018 at 11:16 AM, Daniel Vetter <daniel at ffwll.ch> wrote:
> 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 at 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.

I agree. Feel free to amend my patch there.

>>
>>  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.

I agree to all points. Provide a sane exported API along the lines of:

    int drm_master_drop(struct drm_file *file, struct
foobar_ioctl_params *params);

..and then make drm_dropmaster_ioctl() call this. It might even be
nicer to open-code the ioctl parameters to that function, rather than
re-using the ioctl-struct.

I think drm_file should be a superset of drm_device, so no need to
pass both (unless it does not have a back-pointer..).

Thanks
David

> 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


More information about the Intel-gfx mailing list