On Mon, Feb 6, 2017 at 2:20 PM, Emil Velikov <emil.l.velikov@xxxxxxxxx> wrote:
> Hi Jordan,
>
> On 6 February 2017 at 17:39, Jordan Crouse <jcrouse@xxxxxxxxxxxxxx> wrote:
>> Modify the 'pad' member of struct drm_msm_gem_info to 'hint'. If the
>> user sets 'hint' to non-zero it means that they want a IOVA for the
>> GEM object instead of a mmap() offset. Return the iova in the 'offset'
>> member.
>>
>> Signed-off-by: Jordan Crouse <jcrouse@xxxxxxxxxxxxxx>
>> ---
>> drivers/gpu/drm/msm/msm_drv.c | 29 +++++++++++++++++++++++++----
>> include/uapi/drm/msm_drm.h | 4 ++--
>> 2 files changed, 27 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c
>> index e29bb66..1e4e022 100644
>> --- a/drivers/gpu/drm/msm/msm_drv.c
>> +++ b/drivers/gpu/drm/msm/msm_drv.c
>> @@ -677,6 +677,17 @@ static int msm_ioctl_gem_cpu_fini(struct drm_device *dev, void *data,
>> return ret;
>> }
>>
>> +static int msm_ioctl_gem_info_iova(struct drm_device *dev,
>> + struct drm_gem_object *obj, uint64_t *iova)
>> +{
>> + struct msm_drm_private *priv = dev->dev_private;
>> +
>> + if (!priv->gpu)
>> + return -EINVAL;
>> +
> Not too familiar with msm so perhaps a silly question: how can we trigger this ?
if gpu has not loaded (for example, missing firmware, or kernel does
not have iommu, etc)
>> + return msm_gem_get_iova(obj, priv->gpu->aspace, iova);
>> +}
>> +
>> static int msm_ioctl_gem_info(struct drm_device *dev, void *data,
>> struct drm_file *file)
>> {
>> @@ -684,14 +695,24 @@ static int msm_ioctl_gem_info(struct drm_device *dev, void *data,
>> struct drm_gem_object *obj;
>> int ret = 0;
>>
>> - if (args->pad)
>> - return -EINVAL;
>> -
> Please keep the input validation before doing any work (the lookup below).
+1 for making this args->flags and checking against
GEM_INFO_VALID_FLAGS up front. We may want to use some of those other
bits some day
>> obj = drm_gem_object_lookup(file, args->handle);
>> if (!obj)
>> return -ENOENT;
>>
>> - args->offset = msm_gem_mmap_offset(obj);
>> + /*
>> + * If the hint variable is set, it means that the user wants a IOVA for
>> + * this buffer. Return the address from the GPU because that is
>> + * probably what it is looking for
>> + */
>> + if (args->hint) {
> One could also rename hint to flags. Regardless of the name you can
> use hint/flags as a bitmask.
>
>> + uint64_t iova;
>> +
>> + ret = msm_ioctl_gem_info_iova(dev, obj, &iova);
>> + if (!ret)
>> + args->offset = iova;
>> + } else {
>> + args->offset = msm_gem_mmap_offset(obj);
>> + }
>>
>> drm_gem_object_unreference_unlocked(obj);
>>
>> diff --git a/include/uapi/drm/msm_drm.h b/include/uapi/drm/msm_drm.h
>> index 4d5d6a2..045ad20 100644
>> --- a/include/uapi/drm/msm_drm.h
>> +++ b/include/uapi/drm/msm_drm.h
>> @@ -105,8 +105,8 @@ struct drm_msm_gem_new {
>>
>> struct drm_msm_gem_info {
>> __u32 handle; /* in */
>> - __u32 pad;
>> - __u64 offset; /* out, offset to pass to mmap() */
>> + __u32 hint; /* in */
> Please add explicit #define for the currently valid hints/flags.
>
>> + __u64 offset; /* out, mmap() offset if hint is 0, iova if 1 */
> Other drivers have used anonymous unions to improve the naming, in
> such situations.
>
> struct drm_msm_gem_info {
> __u32 handle; /* in */
> __u32 hint; /* in */
> union { /* out */
> __u64 offset; /* offset if hint is FOO */
> __u64 iova; /* iova if hint is BAR */
> };
> };
is anon union legit for uabi? I was under the impression that for
some reason it was not. But I could be wrong.
BR,
-R
>
> Thanks
> Emil
> _______________________________________________
> dri-devel mailing list
> dri-devel@xxxxxxxxxxxxxxxxxxxxx
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
