Re: [PATCH 5/6] drm/i915: add query uAPI

[Lionel Landwerlin <lionel.g.landwerlin@intel.com>]

Applied all of your comments here.

On 11/01/18 12:19, Tvrtko Ursulin wrote:
>
> On 18/12/2017 15:35, Lionel Landwerlin wrote:
>> There are a number of information that are readable from hardware
>> registers and that we would like to make accessible to userspace. One
>> particular example is the topology of the execution units (how are
>> execution units grouped in subslices and slices and also which ones
>> have been fused off for die recovery).
>>
>> At the moment the GET_PARAM ioctl covers some basic needs, but
>> generally is only able to return a single value for each defined
>> parameter. This is a bit problematic with topology descriptions which
>> are array/maps of available units.
>>
>> This change introduces a new ioctl that can deal with requests to fill
>> structures of potentially variable lengths. The user is expected fill
>> a query with length fields set at 0 on the first call, the kernel then
>> sets the length fields to the their expected values. A second call to
>> the kernel with length fields at their expected values will trigger a
>> copy of the data to the pointed memory locations.
>>
>> The scope of this uAPI is only to provide information to userspace,
>> not to allow configuration of the device.
>>
>> Signed-off-by: Lionel Landwerlin <lionel.g.landwerlin@intel.com>
>> ---
>>   drivers/gpu/drm/i915/Makefile     |  1 +
>>   drivers/gpu/drm/i915/i915_drv.c   |  1 +
>>   drivers/gpu/drm/i915/i915_drv.h   |  3 +++
>>   drivers/gpu/drm/i915/i915_query.c | 52 
>> +++++++++++++++++++++++++++++++++++++++
>>   include/uapi/drm/i915_drm.h       | 31 +++++++++++++++++++++++
>>   5 files changed, 88 insertions(+)
>>   create mode 100644 drivers/gpu/drm/i915/i915_query.c
>>
>> diff --git a/drivers/gpu/drm/i915/Makefile 
>> b/drivers/gpu/drm/i915/Makefile
>> index 091aef281963..9627e7e309dc 100644
>> --- a/drivers/gpu/drm/i915/Makefile
>> +++ b/drivers/gpu/drm/i915/Makefile
>> @@ -69,6 +69,7 @@ i915-y += i915_cmd_parser.o \
>>         i915_gem_timeline.o \
>>         i915_gem_userptr.o \
>>         i915_gemfs.o \
>> +      i915_query.o \
>>         i915_trace_points.o \
>>         i915_vma.o \
>>         intel_breadcrumbs.o \
>> diff --git a/drivers/gpu/drm/i915/i915_drv.c 
>> b/drivers/gpu/drm/i915/i915_drv.c
>> index 8b99e415c345..a90ed9f2b759 100644
>> --- a/drivers/gpu/drm/i915/i915_drv.c
>> +++ b/drivers/gpu/drm/i915/i915_drv.c
>> @@ -2814,6 +2814,7 @@ static const struct drm_ioctl_desc 
>> i915_ioctls[] = {
>>       DRM_IOCTL_DEF_DRV(I915_PERF_OPEN, i915_perf_open_ioctl, 
>> DRM_RENDER_ALLOW),
>>       DRM_IOCTL_DEF_DRV(I915_PERF_ADD_CONFIG, 
>> i915_perf_add_config_ioctl, DRM_UNLOCKED|DRM_RENDER_ALLOW),
>>       DRM_IOCTL_DEF_DRV(I915_PERF_REMOVE_CONFIG, 
>> i915_perf_remove_config_ioctl, DRM_UNLOCKED|DRM_RENDER_ALLOW),
>> +    DRM_IOCTL_DEF_DRV(I915_QUERY, i915_query_ioctl, 
>> DRM_UNLOCKED|DRM_RENDER_ALLOW),
>>   };
>>     static struct drm_driver driver = {
>> diff --git a/drivers/gpu/drm/i915/i915_drv.h 
>> b/drivers/gpu/drm/i915/i915_drv.h
>> index 82fc59078c6a..3415a3d2399c 100644
>> --- a/drivers/gpu/drm/i915/i915_drv.h
>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>> @@ -4084,6 +4084,9 @@ extern void i915_perf_fini(struct 
>> drm_i915_private *dev_priv);
>>   extern void i915_perf_register(struct drm_i915_private *dev_priv);
>>   extern void i915_perf_unregister(struct drm_i915_private *dev_priv);
>>   +/* i915_query.c */
>> +int i915_query_ioctl(struct drm_device *dev, void *data, struct 
>> drm_file *file);
>> +
>>   /* i915_suspend.c */
>>   extern int i915_save_state(struct drm_i915_private *dev_priv);
>>   extern int i915_restore_state(struct drm_i915_private *dev_priv);
>> diff --git a/drivers/gpu/drm/i915/i915_query.c 
>> b/drivers/gpu/drm/i915/i915_query.c
>> new file mode 100644
>> index 000000000000..227a28978190
>> --- /dev/null
>> +++ b/drivers/gpu/drm/i915/i915_query.c
>> @@ -0,0 +1,52 @@
>> +/*
>> + * Copyright © 2017 Intel Corporation
>> + *
>> + * Permission is hereby granted, free of charge, to any person 
>> obtaining a
>> + * copy of this software and associated documentation files (the 
>> "Software"),
>> + * to deal in the Software without restriction, including without 
>> limitation
>> + * the rights to use, copy, modify, merge, publish, distribute, 
>> sublicense,
>> + * and/or sell copies of the Software, and to permit persons to whom 
>> the
>> + * Software is furnished to do so, subject to the following conditions:
>> + *
>> + * The above copyright notice and this permission notice (including 
>> the next
>> + * paragraph) shall be included in all copies or substantial 
>> portions of the
>> + * Software.
>> + *
>> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, 
>> EXPRESS OR
>> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF 
>> MERCHANTABILITY,
>> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO 
>> EVENT SHALL
>> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES 
>> OR OTHER
>> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, 
>> ARISING
>> + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR 
>> OTHER DEALINGS
>> + * IN THE SOFTWARE.
>> + *
>> + */
>> +
>> +#include "i915_drv.h"
>> +#include <uapi/drm/i915_drm.h>
>> +
>> +int i915_query_ioctl(struct drm_device *dev, void *data, struct 
>> drm_file *file)
>> +{
>> +    struct drm_i915_query *args = data;
>
> An alternative I think could be:
>
> struct drm_i915_query_item __user *user_item_ptr = 
> u64_to_user_ptr(args->items_ptr);
>
>> +    u32 i;
>> +
>> +    for (i = 0; i < args->num_items; i++) {
>
> i++, user_item_ptr++
>
>> +        struct drm_i915_query_item item;
>> +        u64 item_user_ptr = args->items_ptr + sizeof(item) * i;
> So you can drop this line.
>
>> +
>> +        if (copy_from_user(&item, u64_to_user_ptr(item_user_ptr),
>> +                   sizeof(item)))
>> +            return -EFAULT;
>
> And drop u64_to_user_ptr from here...
>
>> +
>> +        switch (item.query_id) {
>> +        default:
>> +            return -EINVAL;
>> +        }
>> +
>> +        if (copy_to_user(u64_to_user_ptr(item_user_ptr), &item,
>
> ... and here.
>
>> +                 sizeof(item)))
>> +            return -EFAULT;
>> +    }
>> +
>> +    return 0;
>> +}
>> diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h
>> index 536ee4febd74..87dd8b15548c 100644
>> --- a/include/uapi/drm/i915_drm.h
>> +++ b/include/uapi/drm/i915_drm.h
>> @@ -318,6 +318,7 @@ typedef struct _drm_i915_sarea {
>>   #define DRM_I915_PERF_OPEN        0x36
>>   #define DRM_I915_PERF_ADD_CONFIG    0x37
>>   #define DRM_I915_PERF_REMOVE_CONFIG    0x38
>> +#define DRM_I915_QUERY            0x39
>>     #define DRM_IOCTL_I915_INIT        DRM_IOW( DRM_COMMAND_BASE + 
>> DRM_I915_INIT, drm_i915_init_t)
>>   #define DRM_IOCTL_I915_FLUSH        DRM_IO ( DRM_COMMAND_BASE + 
>> DRM_I915_FLUSH)
>> @@ -375,6 +376,7 @@ typedef struct _drm_i915_sarea {
>>   #define DRM_IOCTL_I915_PERF_OPEN    DRM_IOW(DRM_COMMAND_BASE + 
>> DRM_I915_PERF_OPEN, struct drm_i915_perf_open_param)
>>   #define DRM_IOCTL_I915_PERF_ADD_CONFIG DRM_IOW(DRM_COMMAND_BASE + 
>> DRM_I915_PERF_ADD_CONFIG, struct drm_i915_perf_oa_config)
>>   #define DRM_IOCTL_I915_PERF_REMOVE_CONFIG DRM_IOW(DRM_COMMAND_BASE 
>> + DRM_I915_PERF_REMOVE_CONFIG, __u64)
>> +#define DRM_IOCTL_I915_QUERY DRM_IOW(DRM_COMMAND_BASE + 
>> DRM_I915_QUERY, struct drm_i915_query)
>
> DRM_IOWR I think.
>
>>     /* Allow drivers to submit batchbuffers directly to hardware, 
>> relying
>>    * on the security mechanisms provided by hardware.
>> @@ -1613,6 +1615,35 @@ struct drm_i915_perf_oa_config {
>>       __u64 flex_regs_ptr;
>>   };
>>   +
>> +struct drm_i915_query_item {
>> +    __u32 query_id;
>
> Maybe make it 64-bit, just in case we get a need to reserve blocks of 
> bits so we have some more space then.
>
>> +
>> +    /*
>> +     * When left to 0 by userspace, this is filled with the size the 
>> data
>
> s/left to 0/set to zero/?
> s/the data/of the data/
>
>> +     * to be written at the query_data pointer.
>
> s/query_data/data_ptr/ ? Or rename the field to query_data - I don't 
> have a case for one versus the other.
>
>> +     */
>> +    __u32 length;
>> +
>> +    /*
>> +     * Data will be written at the location pointed by data_ptr when 
>> the
>> +     * value of length matches the length of the data to be written 
>> by the
>> +     * kernel.
>> +     */
>> +    __u64 data_ptr; > +};
>> +
>> +struct drm_i915_query {
>> +    __u32 num_items;
>> +    __u32 _pad;
>> +
>> +    /*
>> +     * This points an array of struct drm_i915_query_item of length
>
> "This point to an array of num_items drm_i915_query_item structures" ?
>
>> +     * num_items.
>> +     */
>> +    __u64 items_ptr;
>> +};
>> +
>>   #if defined(__cplusplus)
>>   }
>>   #endif
>>
>
> But in general this works for me.
>
> Can't remember who last time raised a complaint about dispatcher, 
> within a dispatcher, within.., but we can't have it all. It's either 
> nested dispatchers, or multiple single purpose ioctls, or sysfs. 
> Executive decision time. :)
>
> Again, this approach looks quite easy to extend, and with low 
> boilerplate code requirements so it looks good to me.
>
> Regards,
>
> Tvrtko
>

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx