diff mbox

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

Message ID 20180103222110.45855-4-noralf@tronnes.org (mailing list archive)
State New, archived
Headers show

Commit Message

Noralf Trønnes Jan. 3, 2018, 10:21 p.m. UTC 
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

Comments

Daniel Vetter Jan. 9, 2018, 10:16 a.m. UTC | #1 
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
>
David Herrmann Jan. 9, 2018, 10:31 a.m. UTC | #2 
Hi

On Tue, Jan 9, 2018 at 11:16 AM, Daniel Vetter <daniel@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@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
Laurent Pinchart Jan. 9, 2018, 2:48 p.m. UTC | #3 
Hi Noralf,

Thank you for the patch.

On Thursday, 4 January 2018 00:21:05 EET 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>

I agree with Daniel's comments. Additionally please see below for one small 
comment.

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

[snip]

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

You should add an SPDX header to specify the file license.

> +#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 mbox

Patch

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);
 
 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);
+
 #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__ */