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

[Emil Velikov <emil.l.velikov@gmail.com>]

Hi Jordan,

On 6 February 2017 at 17:39, Jordan Crouse <jcrouse@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@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 ?

> +       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).

>         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 */
        };
};


Thanks
Emil