| Message ID | 1486402779-9024-4-git-send-email-jcrouse@codeaurora.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
On Mon, Feb 6, 2017 at 2:20 PM, Emil Velikov <emil.l.velikov@gmail.com> wrote: > 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 ? 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@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel
On 6 February 2017 at 19:57, Rob Clark <robdclark@gmail.com> wrote: > On Mon, Feb 6, 2017 at 2:20 PM, Emil Velikov <emil.l.velikov@gmail.com> wrote: >> 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 ? > > if gpu has not loaded (for example, missing firmware, or kernel does > not have iommu, etc) > Thanks Rob. I was under the impression that in such cases the driver will/should fail to load. >>> + __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. > Haven't seen any wording against it and we do have a few instances in DRM UABI land. Either way it was just an idea. Regards, Emil
On Mon, Feb 6, 2017 at 3:24 PM, Emil Velikov <emil.l.velikov@gmail.com> wrote: > On 6 February 2017 at 19:57, Rob Clark <robdclark@gmail.com> wrote: >> On Mon, Feb 6, 2017 at 2:20 PM, Emil Velikov <emil.l.velikov@gmail.com> wrote: >>> 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 ? >> >> if gpu has not loaded (for example, missing firmware, or kernel does >> not have iommu, etc) >> > Thanks Rob. I was under the impression that in such cases the driver > will/should fail to load. radeon/nouveau/i915 will all, iirc, fail to load. I made drm/msm defer to loading gpu until first open() since having to constantly rebuild initrd seemed annoying ;-) >>>> + __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. >> > Haven't seen any wording against it and we do have a few instances in > DRM UABI land. > Either way it was just an idea. hmm, ok, if we are already using it in uabi (and not just ancient ioctls) then maybe I am wrong.. BR, -R > Regards, > Emil
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, &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 */ + __u64 offset; /* out, mmap() offset if hint is 0, iova if 1 */ }; #define MSM_PREP_READ 0x01
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(-)