Hi
On Tue, Jan 9, 2018 at 11:16 AM, Daniel Vetter <daniel@xxxxxxxx> 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@xxxxxxxxxxx>
>> ---
>> 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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
