Re: [Freedreno] [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  wrote:
> Hi Jordan,
>
> On 6 February 2017 at 17:39, Jordan Crouse  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 
>> ---
>>  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, );
>> +   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-de...@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
___
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno

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

[Emil Velikov]

Hi Jordan,

On 6 February 2017 at 17:39, Jordan Crouse  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 
> ---
>  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, );
> +   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
___
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno

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

[Jordan Crouse]

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 
---
 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;
+
+   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;
-
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) {
+   uint64_t iova;
+
+   ret = msm_ioctl_gem_info_iova(dev, obj, );
+   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 */
+   __u64 offset; /* out, mmap() offset if hint is 0, iova if 1 */
 };
 
 #define MSM_PREP_READ0x01
-- 
1.9.1

___
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno