[PATCH 03/11] drm/msm: Add hint to DRM_IOCTL_MSM_GEM_INFO to return an object IOVA

[Rob Clark]

On Mon, Feb 6, 2017 at 2:20 PM, Emil Velikov <emil.l.velikov at gmail.com> wrote:
> Hi Jordan,
>
> On 6 February 2017 at 17:39, Jordan Crouse <jcrouse at codeaurora.org> 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 at codeaurora.org>
>> ---
>>  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 at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel