Re: [Freedreno] [PATCH 2/2] drm/msm: Add MSM_WAIT_IOVA ioctl
On Tue, Jan 14, 2020 at 09:30:00AM -0800, Kristian Kristensen wrote: > On Tue, Jan 14, 2020 at 9:23 AM Jordan Crouse wrote: > > > > On Tue, Jan 14, 2020 at 08:52:43AM -0800, Rob Clark wrote: > > > On Mon, Jan 13, 2020 at 9:51 AM Jordan Crouse > > > wrote: > > > > > > > > On Mon, Jan 13, 2020 at 10:36:05AM -0500, Brian Ho wrote: > > > > > + > > > > > + vaddr = base_vaddr + args->offset; > > > > > + > > > > > + /* Assumes WC mapping */ > > > > > + ret = wait_event_interruptible_timeout( > > > > > + gpu->event, *vaddr >= args->value, > > > > > remaining_jiffies); > > > > > > > > I feel like a barrier might be needed before checking *vaddr just in > > > > case you > > > > get the interrupt and wake up the queue before the write posts from the > > > > hardware. > > > > > > > > > > if the gpu is doing posted (or cached) writes, I don't think there is > > > even a CPU side barrier primitive that could wait for that? I think > > > we rely on the GPU not interrupting the CPU until the write is posted > > > > Once the GPU puts the write on the bus then it is up to the whims of the CPU > > architecture. If the writes are being done out of order you run a chance of > > firing the interrupt and making it all the way to your handler before the > > writes > > catch up. > > > > Since you are scheduling and doing a bunch of things in between you probably > > don't need to worry but if you start missing events and you don't know why > > then > > this could be why. A rmb() would give you piece of mind at the cost of being > > Yet Another Barrier (TM). > > Doesn't the fence case do the same thing without a barrier? We get a free __iormb() and dma_rmb() from every gpu_read() so I think that is enough to catch everything up when the interrupt handler reads the status register and in most cases we don't check the fence until after that. I'm not sure that covers all the possible error cases. Maybe something to look into? Also that field is marked as volatile in the struct, but I'm not sure that does anything useful. Jordan > > > BR, > > > -R > > > ___ > > > Freedreno mailing list > > > Freedreno@lists.freedesktop.org > > > https://lists.freedesktop.org/mailman/listinfo/freedreno > > > > -- > > The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, > > a Linux Foundation Collaborative Project > ___ > Freedreno mailing list > Freedreno@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/freedreno -- The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project ___ Freedreno mailing list Freedreno@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/freedreno
Re: [Freedreno] [PATCH 2/2] drm/msm: Add MSM_WAIT_IOVA ioctl
On Tue, Jan 14, 2020 at 08:48:48AM -0800, Rob Clark wrote:
> On Tue, Jan 14, 2020 at 8:40 AM Brian Ho wrote:
> >
> > On Mon, Jan 13, 2020 at 03:17:38PM -0800, Rob Clark wrote:
> > > On Mon, Jan 13, 2020 at 2:55 PM Brian Ho wrote:
> > > >
> > > > On Mon, Jan 13, 2020 at 09:57:43AM -0800, Kristian Kristensen wrote:
> > > > > On Mon, Jan 13, 2020 at 8:25 AM Rob Clark
> > > > > wrote:
> > > > >
> > > > > > On Mon, Jan 13, 2020 at 7:37 AM Brian Ho wrote:
> > > > > > >
> > > > > > > Implements an ioctl to wait until a value at a given iova is
> > > > > > > greater
> > > > > > > than or equal to a supplied value.
> > > > > > >
> > > > > > > This will initially be used by turnip (open-source Vulkan driver
> > > > > > > for
> > > > > > > QC in mesa) for occlusion queries where the userspace driver can
> > > > > > > block on a query becoming available before continuing via
> > > > > > > vkGetQueryPoolResults.
> > > > > > >
> > > > > > > Signed-off-by: Brian Ho
> > > > > > > ---
> > > > > > > drivers/gpu/drm/msm/msm_drv.c | 63
> > > > > > > +--
> > > > > > > include/uapi/drm/msm_drm.h| 13
> > > > > > > 2 files changed, 74 insertions(+), 2 deletions(-)
> > > > > > >
> > > > > > > diff --git a/drivers/gpu/drm/msm/msm_drv.c
> > > > > > b/drivers/gpu/drm/msm/msm_drv.c
> > > > > > > index c84f0a8b3f2c..dcc46874a5a2 100644
> > > > > > > --- a/drivers/gpu/drm/msm/msm_drv.c
> > > > > > > +++ b/drivers/gpu/drm/msm/msm_drv.c
> > > > > > > @@ -36,10 +36,11 @@
> > > > > > > * MSM_GEM_INFO ioctl.
> > > > > > > * - 1.4.0 - softpin, MSM_RELOC_BO_DUMP, and GEM_INFO support to
> > > > > > > set/get
> > > > > > > * GEM object's debug name
> > > > > > > - * - 1.5.0 - Add SUBMITQUERY_QUERY ioctl
> > > > > > > + * - 1.5.0 - Add SUBMITQUEUE_QUERY ioctl
> > > > > > > + * - 1.6.0 - Add WAIT_IOVA ioctl
> > > > > > > */
> > > > > > > #define MSM_VERSION_MAJOR 1
> > > > > > > -#define MSM_VERSION_MINOR 5
> > > > > > > +#define MSM_VERSION_MINOR 6
> > > > > > > #define MSM_VERSION_PATCHLEVEL 0
> > > > > > >
> > > > > > > static const struct drm_mode_config_funcs mode_config_funcs = {
> > > > > > > @@ -952,6 +953,63 @@ static int msm_ioctl_submitqueue_close(struct
> > > > > > drm_device *dev, void *data,
> > > > > > > return msm_submitqueue_remove(file->driver_priv, id);
> > > > > > > }
> > > > > > >
> > > > > > > +static int msm_ioctl_wait_iova(struct drm_device *dev, void
> > > > > > > *data,
> > > > > > > + struct drm_file *file)
> > > > > > > +{
> > > > > > > + struct msm_drm_private *priv = dev->dev_private;
> > > > > > > + struct drm_gem_object *obj;
> > > > > > > + struct drm_msm_wait_iova *args = data;
> > > > > > > + ktime_t timeout = to_ktime(args->timeout);
> > > > > > > + unsigned long remaining_jiffies =
> > > > > > > timeout_to_jiffies(&timeout);
> > > > > > > + struct msm_gpu *gpu = priv->gpu;
> > > > > > > + void *base_vaddr;
> > > > > > > + uint64_t *vaddr;
> > > > > > > + int ret;
> > > > > > > +
> > > > > > > + if (args->pad)
> > > > > > > + return -EINVAL;
> > > > > > > +
> > > > > > > + if (!gpu)
> > > > > > > + return 0;
> > > > > >
> > > > > > hmm, I'm not sure we should return zero in this case.. maybe
> > > > > > -ENODEV?
> > > > > >
> > > > > > > +
> > > > > > > + obj = drm_gem_object_lookup(file, args->handle);
> > > > > > > + if (!obj)
> > > > > > > + return -ENOENT;
> > > > > > > +
> > > > > > > + base_vaddr = msm_gem_get_vaddr(obj);
> > > > > > > + if (IS_ERR(base_vaddr)) {
> > > > > > > + ret = PTR_ERR(base_vaddr);
> > > > > > > + goto err_put_gem_object;
> > > > > > > + }
> > > > > > > + if (args->offset + sizeof(*vaddr) > obj->size) {
> > > > > > > + ret = -EINVAL;
> > > > > > > + goto err_put_vaddr;
> > > > > > > + }
> > > > > > > +
> > > > > > > + vaddr = base_vaddr + args->offset;
> > > > > > > +
> > > > > > > + /* Assumes WC mapping */
> > > > > > > + ret = wait_event_interruptible_timeout(
> > > > > > > + gpu->event, *vaddr >= args->value,
> > > > > > remaining_jiffies);
> > > > > >
> > > > >
> > > > > This needs to do the awkward looking
> > > > >
> > > > > (int64_t)(*data - value) >= 0
> > > > >
> > > > > to properly handle the wraparound case.
> > > > >
> > > >
> > > > I think this comparison will run into issues if we allow for 64-bit
> > > > reference values. For example, if value is ULLONG_MAX, and *data
> > > > starts at 0 on the first comparison, we'll immediately return.
> > > >
> > > > It's not too much of an issue in fence_completed (msm_fence.c), but
> > > > in this ioctl, *data can grow at an arbitrary rate. Are we concerned
> > > > about this?
> > > >
> > > > > > +
> > > > > > > + if (ret == 0) {
> > > > > > > +
Re: [Freedreno] [PATCH 2/2] drm/msm: Add MSM_WAIT_IOVA ioctl
On Tue, Jan 14, 2020 at 08:52:43AM -0800, Rob Clark wrote: > On Mon, Jan 13, 2020 at 9:51 AM Jordan Crouse wrote: > > > > On Mon, Jan 13, 2020 at 10:36:05AM -0500, Brian Ho wrote: > > > + > > > + vaddr = base_vaddr + args->offset; > > > + > > > + /* Assumes WC mapping */ > > > + ret = wait_event_interruptible_timeout( > > > + gpu->event, *vaddr >= args->value, > > > remaining_jiffies); > > > > I feel like a barrier might be needed before checking *vaddr just in case > > you > > get the interrupt and wake up the queue before the write posts from the > > hardware. > > > > if the gpu is doing posted (or cached) writes, I don't think there is > even a CPU side barrier primitive that could wait for that? I think > we rely on the GPU not interrupting the CPU until the write is posted Once the GPU puts the write on the bus then it is up to the whims of the CPU architecture. If the writes are being done out of order you run a chance of firing the interrupt and making it all the way to your handler before the writes catch up. Since you are scheduling and doing a bunch of things in between you probably don't need to worry but if you start missing events and you don't know why then this could be why. A rmb() would give you piece of mind at the cost of being Yet Another Barrier (TM). Jordan > BR, > -R > ___ > Freedreno mailing list > Freedreno@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/freedreno -- The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project ___ Freedreno mailing list Freedreno@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/freedreno
Re: [Freedreno] [PATCH 2/2] drm/msm: Add MSM_WAIT_IOVA ioctl
On Mon, Jan 13, 2020 at 9:51 AM Jordan Crouse wrote: > > On Mon, Jan 13, 2020 at 10:36:05AM -0500, Brian Ho wrote: > > + > > + vaddr = base_vaddr + args->offset; > > + > > + /* Assumes WC mapping */ > > + ret = wait_event_interruptible_timeout( > > + gpu->event, *vaddr >= args->value, remaining_jiffies); > > I feel like a barrier might be needed before checking *vaddr just in case you > get the interrupt and wake up the queue before the write posts from the > hardware. > if the gpu is doing posted (or cached) writes, I don't think there is even a CPU side barrier primitive that could wait for that? I think we rely on the GPU not interrupting the CPU until the write is posted BR, -R ___ Freedreno mailing list Freedreno@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/freedreno
Re: [Freedreno] [PATCH 2/2] drm/msm: Add MSM_WAIT_IOVA ioctl
On Tue, Jan 14, 2020 at 8:40 AM Brian Ho wrote:
>
> On Mon, Jan 13, 2020 at 03:17:38PM -0800, Rob Clark wrote:
> > On Mon, Jan 13, 2020 at 2:55 PM Brian Ho wrote:
> > >
> > > On Mon, Jan 13, 2020 at 09:57:43AM -0800, Kristian Kristensen wrote:
> > > > On Mon, Jan 13, 2020 at 8:25 AM Rob Clark
> > > > wrote:
> > > >
> > > > > On Mon, Jan 13, 2020 at 7:37 AM Brian Ho wrote:
> > > > > >
> > > > > > Implements an ioctl to wait until a value at a given iova is greater
> > > > > > than or equal to a supplied value.
> > > > > >
> > > > > > This will initially be used by turnip (open-source Vulkan driver for
> > > > > > QC in mesa) for occlusion queries where the userspace driver can
> > > > > > block on a query becoming available before continuing via
> > > > > > vkGetQueryPoolResults.
> > > > > >
> > > > > > Signed-off-by: Brian Ho
> > > > > > ---
> > > > > > drivers/gpu/drm/msm/msm_drv.c | 63
> > > > > > +--
> > > > > > include/uapi/drm/msm_drm.h| 13
> > > > > > 2 files changed, 74 insertions(+), 2 deletions(-)
> > > > > >
> > > > > > diff --git a/drivers/gpu/drm/msm/msm_drv.c
> > > > > b/drivers/gpu/drm/msm/msm_drv.c
> > > > > > index c84f0a8b3f2c..dcc46874a5a2 100644
> > > > > > --- a/drivers/gpu/drm/msm/msm_drv.c
> > > > > > +++ b/drivers/gpu/drm/msm/msm_drv.c
> > > > > > @@ -36,10 +36,11 @@
> > > > > > * MSM_GEM_INFO ioctl.
> > > > > > * - 1.4.0 - softpin, MSM_RELOC_BO_DUMP, and GEM_INFO support to
> > > > > > set/get
> > > > > > * GEM object's debug name
> > > > > > - * - 1.5.0 - Add SUBMITQUERY_QUERY ioctl
> > > > > > + * - 1.5.0 - Add SUBMITQUEUE_QUERY ioctl
> > > > > > + * - 1.6.0 - Add WAIT_IOVA ioctl
> > > > > > */
> > > > > > #define MSM_VERSION_MAJOR 1
> > > > > > -#define MSM_VERSION_MINOR 5
> > > > > > +#define MSM_VERSION_MINOR 6
> > > > > > #define MSM_VERSION_PATCHLEVEL 0
> > > > > >
> > > > > > static const struct drm_mode_config_funcs mode_config_funcs = {
> > > > > > @@ -952,6 +953,63 @@ static int msm_ioctl_submitqueue_close(struct
> > > > > drm_device *dev, void *data,
> > > > > > return msm_submitqueue_remove(file->driver_priv, id);
> > > > > > }
> > > > > >
> > > > > > +static int msm_ioctl_wait_iova(struct drm_device *dev, void *data,
> > > > > > + struct drm_file *file)
> > > > > > +{
> > > > > > + struct msm_drm_private *priv = dev->dev_private;
> > > > > > + struct drm_gem_object *obj;
> > > > > > + struct drm_msm_wait_iova *args = data;
> > > > > > + ktime_t timeout = to_ktime(args->timeout);
> > > > > > + unsigned long remaining_jiffies =
> > > > > > timeout_to_jiffies(&timeout);
> > > > > > + struct msm_gpu *gpu = priv->gpu;
> > > > > > + void *base_vaddr;
> > > > > > + uint64_t *vaddr;
> > > > > > + int ret;
> > > > > > +
> > > > > > + if (args->pad)
> > > > > > + return -EINVAL;
> > > > > > +
> > > > > > + if (!gpu)
> > > > > > + return 0;
> > > > >
> > > > > hmm, I'm not sure we should return zero in this case.. maybe -ENODEV?
> > > > >
> > > > > > +
> > > > > > + obj = drm_gem_object_lookup(file, args->handle);
> > > > > > + if (!obj)
> > > > > > + return -ENOENT;
> > > > > > +
> > > > > > + base_vaddr = msm_gem_get_vaddr(obj);
> > > > > > + if (IS_ERR(base_vaddr)) {
> > > > > > + ret = PTR_ERR(base_vaddr);
> > > > > > + goto err_put_gem_object;
> > > > > > + }
> > > > > > + if (args->offset + sizeof(*vaddr) > obj->size) {
> > > > > > + ret = -EINVAL;
> > > > > > + goto err_put_vaddr;
> > > > > > + }
> > > > > > +
> > > > > > + vaddr = base_vaddr + args->offset;
> > > > > > +
> > > > > > + /* Assumes WC mapping */
> > > > > > + ret = wait_event_interruptible_timeout(
> > > > > > + gpu->event, *vaddr >= args->value,
> > > > > remaining_jiffies);
> > > > >
> > > >
> > > > This needs to do the awkward looking
> > > >
> > > > (int64_t)(*data - value) >= 0
> > > >
> > > > to properly handle the wraparound case.
> > > >
> > >
> > > I think this comparison will run into issues if we allow for 64-bit
> > > reference values. For example, if value is ULLONG_MAX, and *data
> > > starts at 0 on the first comparison, we'll immediately return.
> > >
> > > It's not too much of an issue in fence_completed (msm_fence.c), but
> > > in this ioctl, *data can grow at an arbitrary rate. Are we concerned
> > > about this?
> > >
> > > > > +
> > > > > > + if (ret == 0) {
> > > > > > + ret = -ETIMEDOUT;
> > > > > > + goto err_put_vaddr;
> > > > > > + } else if (ret == -ERESTARTSYS) {
> > > > > > + goto err_put_vaddr;
> > > > > > + }
> > > > >
> > > > > maybe:
> > > > >
> > > > > } else {
> > > > >ret = 0;
> > > > > }
> > > > >
> > > > > and then drop the next three lines
Re: [Freedreno] [PATCH 2/2] drm/msm: Add MSM_WAIT_IOVA ioctl
On Mon, Jan 13, 2020 at 03:17:38PM -0800, Rob Clark wrote:
> On Mon, Jan 13, 2020 at 2:55 PM Brian Ho wrote:
> >
> > On Mon, Jan 13, 2020 at 09:57:43AM -0800, Kristian Kristensen wrote:
> > > On Mon, Jan 13, 2020 at 8:25 AM Rob Clark wrote:
> > >
> > > > On Mon, Jan 13, 2020 at 7:37 AM Brian Ho wrote:
> > > > >
> > > > > Implements an ioctl to wait until a value at a given iova is greater
> > > > > than or equal to a supplied value.
> > > > >
> > > > > This will initially be used by turnip (open-source Vulkan driver for
> > > > > QC in mesa) for occlusion queries where the userspace driver can
> > > > > block on a query becoming available before continuing via
> > > > > vkGetQueryPoolResults.
> > > > >
> > > > > Signed-off-by: Brian Ho
> > > > > ---
> > > > > drivers/gpu/drm/msm/msm_drv.c | 63
> > > > > +--
> > > > > include/uapi/drm/msm_drm.h| 13
> > > > > 2 files changed, 74 insertions(+), 2 deletions(-)
> > > > >
> > > > > diff --git a/drivers/gpu/drm/msm/msm_drv.c
> > > > b/drivers/gpu/drm/msm/msm_drv.c
> > > > > index c84f0a8b3f2c..dcc46874a5a2 100644
> > > > > --- a/drivers/gpu/drm/msm/msm_drv.c
> > > > > +++ b/drivers/gpu/drm/msm/msm_drv.c
> > > > > @@ -36,10 +36,11 @@
> > > > > * MSM_GEM_INFO ioctl.
> > > > > * - 1.4.0 - softpin, MSM_RELOC_BO_DUMP, and GEM_INFO support to
> > > > > set/get
> > > > > * GEM object's debug name
> > > > > - * - 1.5.0 - Add SUBMITQUERY_QUERY ioctl
> > > > > + * - 1.5.0 - Add SUBMITQUEUE_QUERY ioctl
> > > > > + * - 1.6.0 - Add WAIT_IOVA ioctl
> > > > > */
> > > > > #define MSM_VERSION_MAJOR 1
> > > > > -#define MSM_VERSION_MINOR 5
> > > > > +#define MSM_VERSION_MINOR 6
> > > > > #define MSM_VERSION_PATCHLEVEL 0
> > > > >
> > > > > static const struct drm_mode_config_funcs mode_config_funcs = {
> > > > > @@ -952,6 +953,63 @@ static int msm_ioctl_submitqueue_close(struct
> > > > drm_device *dev, void *data,
> > > > > return msm_submitqueue_remove(file->driver_priv, id);
> > > > > }
> > > > >
> > > > > +static int msm_ioctl_wait_iova(struct drm_device *dev, void *data,
> > > > > + struct drm_file *file)
> > > > > +{
> > > > > + struct msm_drm_private *priv = dev->dev_private;
> > > > > + struct drm_gem_object *obj;
> > > > > + struct drm_msm_wait_iova *args = data;
> > > > > + ktime_t timeout = to_ktime(args->timeout);
> > > > > + unsigned long remaining_jiffies =
> > > > > timeout_to_jiffies(&timeout);
> > > > > + struct msm_gpu *gpu = priv->gpu;
> > > > > + void *base_vaddr;
> > > > > + uint64_t *vaddr;
> > > > > + int ret;
> > > > > +
> > > > > + if (args->pad)
> > > > > + return -EINVAL;
> > > > > +
> > > > > + if (!gpu)
> > > > > + return 0;
> > > >
> > > > hmm, I'm not sure we should return zero in this case.. maybe -ENODEV?
> > > >
> > > > > +
> > > > > + obj = drm_gem_object_lookup(file, args->handle);
> > > > > + if (!obj)
> > > > > + return -ENOENT;
> > > > > +
> > > > > + base_vaddr = msm_gem_get_vaddr(obj);
> > > > > + if (IS_ERR(base_vaddr)) {
> > > > > + ret = PTR_ERR(base_vaddr);
> > > > > + goto err_put_gem_object;
> > > > > + }
> > > > > + if (args->offset + sizeof(*vaddr) > obj->size) {
> > > > > + ret = -EINVAL;
> > > > > + goto err_put_vaddr;
> > > > > + }
> > > > > +
> > > > > + vaddr = base_vaddr + args->offset;
> > > > > +
> > > > > + /* Assumes WC mapping */
> > > > > + ret = wait_event_interruptible_timeout(
> > > > > + gpu->event, *vaddr >= args->value,
> > > > remaining_jiffies);
> > > >
> > >
> > > This needs to do the awkward looking
> > >
> > > (int64_t)(*data - value) >= 0
> > >
> > > to properly handle the wraparound case.
> > >
> >
> > I think this comparison will run into issues if we allow for 64-bit
> > reference values. For example, if value is ULLONG_MAX, and *data
> > starts at 0 on the first comparison, we'll immediately return.
> >
> > It's not too much of an issue in fence_completed (msm_fence.c), but
> > in this ioctl, *data can grow at an arbitrary rate. Are we concerned
> > about this?
> >
> > > > +
> > > > > + if (ret == 0) {
> > > > > + ret = -ETIMEDOUT;
> > > > > + goto err_put_vaddr;
> > > > > + } else if (ret == -ERESTARTSYS) {
> > > > > + goto err_put_vaddr;
> > > > > + }
> > > >
> > > > maybe:
> > > >
> > > > } else {
> > > >ret = 0;
> > > > }
> > > >
> > > > and then drop the next three lines?
> > > >
> > > > > +
> > > > > + msm_gem_put_vaddr(obj);
> > > > > + drm_gem_object_put_unlocked(obj);
> > > > > + return 0;
> > > > > +
> > > > > +err_put_vaddr:
> > > > > + msm_gem_put_vaddr(obj);
> > > > > +err_put_gem_object:
> > > > > + drm_gem_object_put_unlocked(obj);
Re: [Freedreno] [PATCH 2/2] drm/msm: Add MSM_WAIT_IOVA ioctl
On Mon, Jan 13, 2020 at 3:17 PM Rob Clark wrote:
>
> On Mon, Jan 13, 2020 at 2:55 PM Brian Ho wrote:
> >
> > On Mon, Jan 13, 2020 at 09:57:43AM -0800, Kristian Kristensen wrote:
> > > On Mon, Jan 13, 2020 at 8:25 AM Rob Clark wrote:
> > >
> > > > On Mon, Jan 13, 2020 at 7:37 AM Brian Ho wrote:
> > > > >
> > > > > Implements an ioctl to wait until a value at a given iova is greater
> > > > > than or equal to a supplied value.
> > > > >
> > > > > This will initially be used by turnip (open-source Vulkan driver for
> > > > > QC in mesa) for occlusion queries where the userspace driver can
> > > > > block on a query becoming available before continuing via
> > > > > vkGetQueryPoolResults.
> > > > >
> > > > > Signed-off-by: Brian Ho
> > > > > ---
> > > > > drivers/gpu/drm/msm/msm_drv.c | 63
> > > > > +--
> > > > > include/uapi/drm/msm_drm.h| 13
> > > > > 2 files changed, 74 insertions(+), 2 deletions(-)
> > > > >
> > > > > diff --git a/drivers/gpu/drm/msm/msm_drv.c
> > > > b/drivers/gpu/drm/msm/msm_drv.c
> > > > > index c84f0a8b3f2c..dcc46874a5a2 100644
> > > > > --- a/drivers/gpu/drm/msm/msm_drv.c
> > > > > +++ b/drivers/gpu/drm/msm/msm_drv.c
> > > > > @@ -36,10 +36,11 @@
> > > > > * MSM_GEM_INFO ioctl.
> > > > > * - 1.4.0 - softpin, MSM_RELOC_BO_DUMP, and GEM_INFO support to
> > > > > set/get
> > > > > * GEM object's debug name
> > > > > - * - 1.5.0 - Add SUBMITQUERY_QUERY ioctl
> > > > > + * - 1.5.0 - Add SUBMITQUEUE_QUERY ioctl
> > > > > + * - 1.6.0 - Add WAIT_IOVA ioctl
> > > > > */
> > > > > #define MSM_VERSION_MAJOR 1
> > > > > -#define MSM_VERSION_MINOR 5
> > > > > +#define MSM_VERSION_MINOR 6
> > > > > #define MSM_VERSION_PATCHLEVEL 0
> > > > >
> > > > > static const struct drm_mode_config_funcs mode_config_funcs = {
> > > > > @@ -952,6 +953,63 @@ static int msm_ioctl_submitqueue_close(struct
> > > > drm_device *dev, void *data,
> > > > > return msm_submitqueue_remove(file->driver_priv, id);
> > > > > }
> > > > >
> > > > > +static int msm_ioctl_wait_iova(struct drm_device *dev, void *data,
> > > > > + struct drm_file *file)
> > > > > +{
> > > > > + struct msm_drm_private *priv = dev->dev_private;
> > > > > + struct drm_gem_object *obj;
> > > > > + struct drm_msm_wait_iova *args = data;
> > > > > + ktime_t timeout = to_ktime(args->timeout);
> > > > > + unsigned long remaining_jiffies =
> > > > > timeout_to_jiffies(&timeout);
> > > > > + struct msm_gpu *gpu = priv->gpu;
> > > > > + void *base_vaddr;
> > > > > + uint64_t *vaddr;
> > > > > + int ret;
> > > > > +
> > > > > + if (args->pad)
> > > > > + return -EINVAL;
> > > > > +
> > > > > + if (!gpu)
> > > > > + return 0;
> > > >
> > > > hmm, I'm not sure we should return zero in this case.. maybe -ENODEV?
> > > >
> > > > > +
> > > > > + obj = drm_gem_object_lookup(file, args->handle);
> > > > > + if (!obj)
> > > > > + return -ENOENT;
> > > > > +
> > > > > + base_vaddr = msm_gem_get_vaddr(obj);
> > > > > + if (IS_ERR(base_vaddr)) {
> > > > > + ret = PTR_ERR(base_vaddr);
> > > > > + goto err_put_gem_object;
> > > > > + }
> > > > > + if (args->offset + sizeof(*vaddr) > obj->size) {
> > > > > + ret = -EINVAL;
> > > > > + goto err_put_vaddr;
> > > > > + }
> > > > > +
> > > > > + vaddr = base_vaddr + args->offset;
> > > > > +
> > > > > + /* Assumes WC mapping */
> > > > > + ret = wait_event_interruptible_timeout(
> > > > > + gpu->event, *vaddr >= args->value,
> > > > remaining_jiffies);
> > > >
> > >
> > > This needs to do the awkward looking
> > >
> > > (int64_t)(*data - value) >= 0
> > >
> > > to properly handle the wraparound case.
> > >
> >
> > I think this comparison will run into issues if we allow for 64-bit
> > reference values. For example, if value is ULLONG_MAX, and *data
> > starts at 0 on the first comparison, we'll immediately return.
The comparison will have to account of the number of bits in the
serial number. The (int64_t) (*data - value) works for 64 bit unsigned
serial numbers, but for 32 bit serials as suggested below, we need to
cast to int32_t. It does work though, in the case where value is
ULLONG_MAX and and *data is 0, 0 - ULLONG_MAX is one, which is
correct. The serial numbers wrap around and the distance is computed
modulo 2^64. See
https://en.wikipedia.org/wiki/Serial_number_arithmetic.
> >
> > It's not too much of an issue in fence_completed (msm_fence.c), but
> > in this ioctl, *data can grow at an arbitrary rate. Are we concerned
> > about this?
> >
> > > > +
> > > > > + if (ret == 0) {
> > > > > + ret = -ETIMEDOUT;
> > > > > + goto err_put_vaddr;
> > > > > + } else if (ret == -ERESTARTSYS) {
> > > > > + goto
Re: [Freedreno] [PATCH 2/2] drm/msm: Add MSM_WAIT_IOVA ioctl
On Mon, Jan 13, 2020 at 2:55 PM Brian Ho wrote:
>
> On Mon, Jan 13, 2020 at 09:57:43AM -0800, Kristian Kristensen wrote:
> > On Mon, Jan 13, 2020 at 8:25 AM Rob Clark wrote:
> >
> > > On Mon, Jan 13, 2020 at 7:37 AM Brian Ho wrote:
> > > >
> > > > Implements an ioctl to wait until a value at a given iova is greater
> > > > than or equal to a supplied value.
> > > >
> > > > This will initially be used by turnip (open-source Vulkan driver for
> > > > QC in mesa) for occlusion queries where the userspace driver can
> > > > block on a query becoming available before continuing via
> > > > vkGetQueryPoolResults.
> > > >
> > > > Signed-off-by: Brian Ho
> > > > ---
> > > > drivers/gpu/drm/msm/msm_drv.c | 63 +--
> > > > include/uapi/drm/msm_drm.h| 13
> > > > 2 files changed, 74 insertions(+), 2 deletions(-)
> > > >
> > > > diff --git a/drivers/gpu/drm/msm/msm_drv.c
> > > b/drivers/gpu/drm/msm/msm_drv.c
> > > > index c84f0a8b3f2c..dcc46874a5a2 100644
> > > > --- a/drivers/gpu/drm/msm/msm_drv.c
> > > > +++ b/drivers/gpu/drm/msm/msm_drv.c
> > > > @@ -36,10 +36,11 @@
> > > > * MSM_GEM_INFO ioctl.
> > > > * - 1.4.0 - softpin, MSM_RELOC_BO_DUMP, and GEM_INFO support to
> > > > set/get
> > > > * GEM object's debug name
> > > > - * - 1.5.0 - Add SUBMITQUERY_QUERY ioctl
> > > > + * - 1.5.0 - Add SUBMITQUEUE_QUERY ioctl
> > > > + * - 1.6.0 - Add WAIT_IOVA ioctl
> > > > */
> > > > #define MSM_VERSION_MAJOR 1
> > > > -#define MSM_VERSION_MINOR 5
> > > > +#define MSM_VERSION_MINOR 6
> > > > #define MSM_VERSION_PATCHLEVEL 0
> > > >
> > > > static const struct drm_mode_config_funcs mode_config_funcs = {
> > > > @@ -952,6 +953,63 @@ static int msm_ioctl_submitqueue_close(struct
> > > drm_device *dev, void *data,
> > > > return msm_submitqueue_remove(file->driver_priv, id);
> > > > }
> > > >
> > > > +static int msm_ioctl_wait_iova(struct drm_device *dev, void *data,
> > > > + struct drm_file *file)
> > > > +{
> > > > + struct msm_drm_private *priv = dev->dev_private;
> > > > + struct drm_gem_object *obj;
> > > > + struct drm_msm_wait_iova *args = data;
> > > > + ktime_t timeout = to_ktime(args->timeout);
> > > > + unsigned long remaining_jiffies = timeout_to_jiffies(&timeout);
> > > > + struct msm_gpu *gpu = priv->gpu;
> > > > + void *base_vaddr;
> > > > + uint64_t *vaddr;
> > > > + int ret;
> > > > +
> > > > + if (args->pad)
> > > > + return -EINVAL;
> > > > +
> > > > + if (!gpu)
> > > > + return 0;
> > >
> > > hmm, I'm not sure we should return zero in this case.. maybe -ENODEV?
> > >
> > > > +
> > > > + obj = drm_gem_object_lookup(file, args->handle);
> > > > + if (!obj)
> > > > + return -ENOENT;
> > > > +
> > > > + base_vaddr = msm_gem_get_vaddr(obj);
> > > > + if (IS_ERR(base_vaddr)) {
> > > > + ret = PTR_ERR(base_vaddr);
> > > > + goto err_put_gem_object;
> > > > + }
> > > > + if (args->offset + sizeof(*vaddr) > obj->size) {
> > > > + ret = -EINVAL;
> > > > + goto err_put_vaddr;
> > > > + }
> > > > +
> > > > + vaddr = base_vaddr + args->offset;
> > > > +
> > > > + /* Assumes WC mapping */
> > > > + ret = wait_event_interruptible_timeout(
> > > > + gpu->event, *vaddr >= args->value,
> > > remaining_jiffies);
> > >
> >
> > This needs to do the awkward looking
> >
> > (int64_t)(*data - value) >= 0
> >
> > to properly handle the wraparound case.
> >
>
> I think this comparison will run into issues if we allow for 64-bit
> reference values. For example, if value is ULLONG_MAX, and *data
> starts at 0 on the first comparison, we'll immediately return.
>
> It's not too much of an issue in fence_completed (msm_fence.c), but
> in this ioctl, *data can grow at an arbitrary rate. Are we concerned
> about this?
>
> > > +
> > > > + if (ret == 0) {
> > > > + ret = -ETIMEDOUT;
> > > > + goto err_put_vaddr;
> > > > + } else if (ret == -ERESTARTSYS) {
> > > > + goto err_put_vaddr;
> > > > + }
> > >
> > > maybe:
> > >
> > > } else {
> > >ret = 0;
> > > }
> > >
> > > and then drop the next three lines?
> > >
> > > > +
> > > > + msm_gem_put_vaddr(obj);
> > > > + drm_gem_object_put_unlocked(obj);
> > > > + return 0;
> > > > +
> > > > +err_put_vaddr:
> > > > + msm_gem_put_vaddr(obj);
> > > > +err_put_gem_object:
> > > > + drm_gem_object_put_unlocked(obj);
> > > > + return ret;
> > > > +}
> > > > +
> > > > static const struct drm_ioctl_desc msm_ioctls[] = {
> > > > DRM_IOCTL_DEF_DRV(MSM_GET_PARAM,msm_ioctl_get_param,
> > > DRM_RENDER_ALLOW),
> > > > DRM_IOCTL_DEF_DRV(MSM_GEM_NEW, msm_ioctl_gem_new,
> > > DRM_RENDER_ALLOW),
> > > > @@ -964,6 +1022,7 @@ static
Re: [Freedreno] [PATCH 2/2] drm/msm: Add MSM_WAIT_IOVA ioctl
On Mon, Jan 13, 2020 at 09:57:43AM -0800, Kristian Kristensen wrote:
> On Mon, Jan 13, 2020 at 8:25 AM Rob Clark wrote:
>
> > On Mon, Jan 13, 2020 at 7:37 AM Brian Ho wrote:
> > >
> > > Implements an ioctl to wait until a value at a given iova is greater
> > > than or equal to a supplied value.
> > >
> > > This will initially be used by turnip (open-source Vulkan driver for
> > > QC in mesa) for occlusion queries where the userspace driver can
> > > block on a query becoming available before continuing via
> > > vkGetQueryPoolResults.
> > >
> > > Signed-off-by: Brian Ho
> > > ---
> > > drivers/gpu/drm/msm/msm_drv.c | 63 +--
> > > include/uapi/drm/msm_drm.h| 13
> > > 2 files changed, 74 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/msm/msm_drv.c
> > b/drivers/gpu/drm/msm/msm_drv.c
> > > index c84f0a8b3f2c..dcc46874a5a2 100644
> > > --- a/drivers/gpu/drm/msm/msm_drv.c
> > > +++ b/drivers/gpu/drm/msm/msm_drv.c
> > > @@ -36,10 +36,11 @@
> > > * MSM_GEM_INFO ioctl.
> > > * - 1.4.0 - softpin, MSM_RELOC_BO_DUMP, and GEM_INFO support to set/get
> > > * GEM object's debug name
> > > - * - 1.5.0 - Add SUBMITQUERY_QUERY ioctl
> > > + * - 1.5.0 - Add SUBMITQUEUE_QUERY ioctl
> > > + * - 1.6.0 - Add WAIT_IOVA ioctl
> > > */
> > > #define MSM_VERSION_MAJOR 1
> > > -#define MSM_VERSION_MINOR 5
> > > +#define MSM_VERSION_MINOR 6
> > > #define MSM_VERSION_PATCHLEVEL 0
> > >
> > > static const struct drm_mode_config_funcs mode_config_funcs = {
> > > @@ -952,6 +953,63 @@ static int msm_ioctl_submitqueue_close(struct
> > drm_device *dev, void *data,
> > > return msm_submitqueue_remove(file->driver_priv, id);
> > > }
> > >
> > > +static int msm_ioctl_wait_iova(struct drm_device *dev, void *data,
> > > + struct drm_file *file)
> > > +{
> > > + struct msm_drm_private *priv = dev->dev_private;
> > > + struct drm_gem_object *obj;
> > > + struct drm_msm_wait_iova *args = data;
> > > + ktime_t timeout = to_ktime(args->timeout);
> > > + unsigned long remaining_jiffies = timeout_to_jiffies(&timeout);
> > > + struct msm_gpu *gpu = priv->gpu;
> > > + void *base_vaddr;
> > > + uint64_t *vaddr;
> > > + int ret;
> > > +
> > > + if (args->pad)
> > > + return -EINVAL;
> > > +
> > > + if (!gpu)
> > > + return 0;
> >
> > hmm, I'm not sure we should return zero in this case.. maybe -ENODEV?
> >
> > > +
> > > + obj = drm_gem_object_lookup(file, args->handle);
> > > + if (!obj)
> > > + return -ENOENT;
> > > +
> > > + base_vaddr = msm_gem_get_vaddr(obj);
> > > + if (IS_ERR(base_vaddr)) {
> > > + ret = PTR_ERR(base_vaddr);
> > > + goto err_put_gem_object;
> > > + }
> > > + if (args->offset + sizeof(*vaddr) > obj->size) {
> > > + ret = -EINVAL;
> > > + goto err_put_vaddr;
> > > + }
> > > +
> > > + vaddr = base_vaddr + args->offset;
> > > +
> > > + /* Assumes WC mapping */
> > > + ret = wait_event_interruptible_timeout(
> > > + gpu->event, *vaddr >= args->value,
> > remaining_jiffies);
> >
>
> This needs to do the awkward looking
>
> (int64_t)(*data - value) >= 0
>
> to properly handle the wraparound case.
>
I think this comparison will run into issues if we allow for 64-bit
reference values. For example, if value is ULLONG_MAX, and *data
starts at 0 on the first comparison, we'll immediately return.
It's not too much of an issue in fence_completed (msm_fence.c), but
in this ioctl, *data can grow at an arbitrary rate. Are we concerned
about this?
> > +
> > > + if (ret == 0) {
> > > + ret = -ETIMEDOUT;
> > > + goto err_put_vaddr;
> > > + } else if (ret == -ERESTARTSYS) {
> > > + goto err_put_vaddr;
> > > + }
> >
> > maybe:
> >
> > } else {
> >ret = 0;
> > }
> >
> > and then drop the next three lines?
> >
> > > +
> > > + msm_gem_put_vaddr(obj);
> > > + drm_gem_object_put_unlocked(obj);
> > > + return 0;
> > > +
> > > +err_put_vaddr:
> > > + msm_gem_put_vaddr(obj);
> > > +err_put_gem_object:
> > > + drm_gem_object_put_unlocked(obj);
> > > + return ret;
> > > +}
> > > +
> > > static const struct drm_ioctl_desc msm_ioctls[] = {
> > > DRM_IOCTL_DEF_DRV(MSM_GET_PARAM,msm_ioctl_get_param,
> > DRM_RENDER_ALLOW),
> > > DRM_IOCTL_DEF_DRV(MSM_GEM_NEW, msm_ioctl_gem_new,
> > DRM_RENDER_ALLOW),
> > > @@ -964,6 +1022,7 @@ static const struct drm_ioctl_desc msm_ioctls[] = {
> > > DRM_IOCTL_DEF_DRV(MSM_SUBMITQUEUE_NEW,
> > msm_ioctl_submitqueue_new, DRM_RENDER_ALLOW),
> > > DRM_IOCTL_DEF_DRV(MSM_SUBMITQUEUE_CLOSE,
> > msm_ioctl_submitqueue_close, DRM_RENDER_ALLOW),
> > > DRM_IOCTL_DEF_DRV(MSM_SUBMITQUEUE_QUERY,
> > msm_ioct
Re: [Freedreno] [PATCH 2/2] drm/msm: Add MSM_WAIT_IOVA ioctl
On Mon, Jan 13, 2020 at 9:51 AM Jordan Crouse wrote:
>
> On Mon, Jan 13, 2020 at 10:36:05AM -0500, Brian Ho wrote:
> > Implements an ioctl to wait until a value at a given iova is greater
> > than or equal to a supplied value.
> >
> > This will initially be used by turnip (open-source Vulkan driver for
> > QC in mesa) for occlusion queries where the userspace driver can
> > block on a query becoming available before continuing via
> > vkGetQueryPoolResults.
> >
> > Signed-off-by: Brian Ho
> > ---
> > drivers/gpu/drm/msm/msm_drv.c | 63 +--
> > include/uapi/drm/msm_drm.h| 13
> > 2 files changed, 74 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c
> > index c84f0a8b3f2c..dcc46874a5a2 100644
> > --- a/drivers/gpu/drm/msm/msm_drv.c
> > +++ b/drivers/gpu/drm/msm/msm_drv.c
> > @@ -36,10 +36,11 @@
> > * MSM_GEM_INFO ioctl.
> > * - 1.4.0 - softpin, MSM_RELOC_BO_DUMP, and GEM_INFO support to set/get
> > * GEM object's debug name
> > - * - 1.5.0 - Add SUBMITQUERY_QUERY ioctl
> > + * - 1.5.0 - Add SUBMITQUEUE_QUERY ioctl
> > + * - 1.6.0 - Add WAIT_IOVA ioctl
> > */
> > #define MSM_VERSION_MAJOR1
> > -#define MSM_VERSION_MINOR5
> > +#define MSM_VERSION_MINOR6
> > #define MSM_VERSION_PATCHLEVEL 0
> >
> > static const struct drm_mode_config_funcs mode_config_funcs = {
> > @@ -952,6 +953,63 @@ static int msm_ioctl_submitqueue_close(struct
> > drm_device *dev, void *data,
> > return msm_submitqueue_remove(file->driver_priv, id);
> > }
> >
> > +static int msm_ioctl_wait_iova(struct drm_device *dev, void *data,
> > + struct drm_file *file)
> > +{
> > + struct msm_drm_private *priv = dev->dev_private;
> > + struct drm_gem_object *obj;
> > + struct drm_msm_wait_iova *args = data;
> > + ktime_t timeout = to_ktime(args->timeout);
> > + unsigned long remaining_jiffies = timeout_to_jiffies(&timeout);
> > + struct msm_gpu *gpu = priv->gpu;
> > + void *base_vaddr;
> > + uint64_t *vaddr;
> > + int ret;
> > +
> > + if (args->pad)
> > + return -EINVAL;
> > +
> > + if (!gpu)
> > + return 0;
>
> If the GPU isn't up, it should be an error since this macro is specifically
> designed for just the GPU (though, I suppose the display *COULD* use it to
> watch
> a memory mapped register or something).
>
> > +
> > + obj = drm_gem_object_lookup(file, args->handle);
> > + if (!obj)
> > + return -ENOENT;
> > +
> > + base_vaddr = msm_gem_get_vaddr(obj);
> > + if (IS_ERR(base_vaddr)) {
> > + ret = PTR_ERR(base_vaddr);
> > + goto err_put_gem_object;
> > + }
> > + if (args->offset + sizeof(*vaddr) > obj->size) {
>
> There is a chance to trigger a u64 overflow here resulting in an arbitrary
> (ish)
> vaddr two lines below.
>
>
> > + ret = -EINVAL;
> > + goto err_put_vaddr;
> > + }
>
> You can check this before getting the vaddr which would save you a clean up
> step.
>
> > +
> > + vaddr = base_vaddr + args->offset;
> > +
> > + /* Assumes WC mapping */
> > + ret = wait_event_interruptible_timeout(
> > + gpu->event, *vaddr >= args->value, remaining_jiffies);
>
> I feel like a barrier might be needed before checking *vaddr just in case you
> get the interrupt and wake up the queue before the write posts from the
> hardware.
>
> > +
>
> > + if (ret == 0) {
> > + ret = -ETIMEDOUT;
> > + goto err_put_vaddr;
> > + } else if (ret == -ERESTARTSYS) {
> > + goto err_put_vaddr;
> > + }
>
> You don't need either goto here because both paths execute the following
> cleanup
> steps. I'm also not sure you need to worry about explicitly checking the
> ERESTARTSYS value, I think that this would be sufficient:
>
> if (ret == 0)
> ret = -ETIMEDOUT;
> else if (ret > 0)
> ret = 0;
>
> > +
>
> Put your err_put_vaddr: label here, but looking up, if you move the bounds
> check
> before the msm_gem_get_vaddr, I don't think you need a label.
>
> > + msm_gem_put_vaddr(obj);
>
> Put the err_put_gem_object: label here.
>
> > + drm_gem_object_put_unlocked(obj);
> > + return 0;
>
> return ret;
>
> > +
> > +err_put_vaddr:
> > + msm_gem_put_vaddr(obj);
> > +err_put_gem_object:
> > + drm_gem_object_put_unlocked(obj);
> > + return ret;
> > +}
>
> And then these guys aren't needed.
>
> > +
> > static const struct drm_ioctl_desc msm_ioctls[] = {
> > DRM_IOCTL_DEF_DRV(MSM_GET_PARAM,msm_ioctl_get_param,
> > DRM_RENDER_ALLOW),
> > DRM_IOCTL_DEF_DRV(MSM_GEM_NEW, msm_ioctl_gem_new,
> > DRM_RENDER_ALLOW),
> > @@ -964,6 +1022,7 @@ static const struct drm_ioctl_desc msm_ioctls[] = {
> > DRM_IOCTL_DEF_DRV(MSM_SUBMITQUEUE_NEW, msm_ioctl_submitqueue_new,
> > DRM_RENDER_ALLOW),
> > DRM_IOCTL_DEF_DRV(MSM_
Re: [Freedreno] [PATCH 2/2] drm/msm: Add MSM_WAIT_IOVA ioctl
On Mon, Jan 13, 2020 at 8:25 AM Rob Clark wrote:
> On Mon, Jan 13, 2020 at 7:37 AM Brian Ho wrote:
> >
> > Implements an ioctl to wait until a value at a given iova is greater
> > than or equal to a supplied value.
> >
> > This will initially be used by turnip (open-source Vulkan driver for
> > QC in mesa) for occlusion queries where the userspace driver can
> > block on a query becoming available before continuing via
> > vkGetQueryPoolResults.
> >
> > Signed-off-by: Brian Ho
> > ---
> > drivers/gpu/drm/msm/msm_drv.c | 63 +--
> > include/uapi/drm/msm_drm.h| 13
> > 2 files changed, 74 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/msm/msm_drv.c
> b/drivers/gpu/drm/msm/msm_drv.c
> > index c84f0a8b3f2c..dcc46874a5a2 100644
> > --- a/drivers/gpu/drm/msm/msm_drv.c
> > +++ b/drivers/gpu/drm/msm/msm_drv.c
> > @@ -36,10 +36,11 @@
> > * MSM_GEM_INFO ioctl.
> > * - 1.4.0 - softpin, MSM_RELOC_BO_DUMP, and GEM_INFO support to set/get
> > * GEM object's debug name
> > - * - 1.5.0 - Add SUBMITQUERY_QUERY ioctl
> > + * - 1.5.0 - Add SUBMITQUEUE_QUERY ioctl
> > + * - 1.6.0 - Add WAIT_IOVA ioctl
> > */
> > #define MSM_VERSION_MAJOR 1
> > -#define MSM_VERSION_MINOR 5
> > +#define MSM_VERSION_MINOR 6
> > #define MSM_VERSION_PATCHLEVEL 0
> >
> > static const struct drm_mode_config_funcs mode_config_funcs = {
> > @@ -952,6 +953,63 @@ static int msm_ioctl_submitqueue_close(struct
> drm_device *dev, void *data,
> > return msm_submitqueue_remove(file->driver_priv, id);
> > }
> >
> > +static int msm_ioctl_wait_iova(struct drm_device *dev, void *data,
> > + struct drm_file *file)
> > +{
> > + struct msm_drm_private *priv = dev->dev_private;
> > + struct drm_gem_object *obj;
> > + struct drm_msm_wait_iova *args = data;
> > + ktime_t timeout = to_ktime(args->timeout);
> > + unsigned long remaining_jiffies = timeout_to_jiffies(&timeout);
> > + struct msm_gpu *gpu = priv->gpu;
> > + void *base_vaddr;
> > + uint64_t *vaddr;
> > + int ret;
> > +
> > + if (args->pad)
> > + return -EINVAL;
> > +
> > + if (!gpu)
> > + return 0;
>
> hmm, I'm not sure we should return zero in this case.. maybe -ENODEV?
>
> > +
> > + obj = drm_gem_object_lookup(file, args->handle);
> > + if (!obj)
> > + return -ENOENT;
> > +
> > + base_vaddr = msm_gem_get_vaddr(obj);
> > + if (IS_ERR(base_vaddr)) {
> > + ret = PTR_ERR(base_vaddr);
> > + goto err_put_gem_object;
> > + }
> > + if (args->offset + sizeof(*vaddr) > obj->size) {
> > + ret = -EINVAL;
> > + goto err_put_vaddr;
> > + }
> > +
> > + vaddr = base_vaddr + args->offset;
> > +
> > + /* Assumes WC mapping */
> > + ret = wait_event_interruptible_timeout(
> > + gpu->event, *vaddr >= args->value,
> remaining_jiffies);
>
This needs to do the awkward looking
(int64_t)(*data - value) >= 0
to properly handle the wraparound case.
> +
> > + if (ret == 0) {
> > + ret = -ETIMEDOUT;
> > + goto err_put_vaddr;
> > + } else if (ret == -ERESTARTSYS) {
> > + goto err_put_vaddr;
> > + }
>
> maybe:
>
> } else {
>ret = 0;
> }
>
> and then drop the next three lines?
>
> > +
> > + msm_gem_put_vaddr(obj);
> > + drm_gem_object_put_unlocked(obj);
> > + return 0;
> > +
> > +err_put_vaddr:
> > + msm_gem_put_vaddr(obj);
> > +err_put_gem_object:
> > + drm_gem_object_put_unlocked(obj);
> > + return ret;
> > +}
> > +
> > static const struct drm_ioctl_desc msm_ioctls[] = {
> > DRM_IOCTL_DEF_DRV(MSM_GET_PARAM,msm_ioctl_get_param,
> DRM_RENDER_ALLOW),
> > DRM_IOCTL_DEF_DRV(MSM_GEM_NEW, msm_ioctl_gem_new,
> DRM_RENDER_ALLOW),
> > @@ -964,6 +1022,7 @@ static const struct drm_ioctl_desc msm_ioctls[] = {
> > DRM_IOCTL_DEF_DRV(MSM_SUBMITQUEUE_NEW,
> msm_ioctl_submitqueue_new, DRM_RENDER_ALLOW),
> > DRM_IOCTL_DEF_DRV(MSM_SUBMITQUEUE_CLOSE,
> msm_ioctl_submitqueue_close, DRM_RENDER_ALLOW),
> > DRM_IOCTL_DEF_DRV(MSM_SUBMITQUEUE_QUERY,
> msm_ioctl_submitqueue_query, DRM_RENDER_ALLOW),
> > + DRM_IOCTL_DEF_DRV(MSM_WAIT_IOVA, msm_ioctl_wait_iova,
> DRM_RENDER_ALLOW),
> > };
> >
> > static const struct vm_operations_struct vm_ops = {
> > diff --git a/include/uapi/drm/msm_drm.h b/include/uapi/drm/msm_drm.h
> > index 0b85ed6a3710..8477f28a4ee1 100644
> > --- a/include/uapi/drm/msm_drm.h
> > +++ b/include/uapi/drm/msm_drm.h
> > @@ -298,6 +298,17 @@ struct drm_msm_submitqueue_query {
> > __u32 pad;
> > };
> >
> > +/* This ioctl blocks until the u64 value at bo + offset is greater than
> or
> > + * equal to the reference value.
> > + */
> > +struct drm_msm_wait_iova {
> > + __u32 handle; /*
Re: [Freedreno] [PATCH 2/2] drm/msm: Add MSM_WAIT_IOVA ioctl
On Mon, Jan 13, 2020 at 10:36:05AM -0500, Brian Ho wrote:
> Implements an ioctl to wait until a value at a given iova is greater
> than or equal to a supplied value.
>
> This will initially be used by turnip (open-source Vulkan driver for
> QC in mesa) for occlusion queries where the userspace driver can
> block on a query becoming available before continuing via
> vkGetQueryPoolResults.
>
> Signed-off-by: Brian Ho
> ---
> drivers/gpu/drm/msm/msm_drv.c | 63 +--
> include/uapi/drm/msm_drm.h| 13
> 2 files changed, 74 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c
> index c84f0a8b3f2c..dcc46874a5a2 100644
> --- a/drivers/gpu/drm/msm/msm_drv.c
> +++ b/drivers/gpu/drm/msm/msm_drv.c
> @@ -36,10 +36,11 @@
> * MSM_GEM_INFO ioctl.
> * - 1.4.0 - softpin, MSM_RELOC_BO_DUMP, and GEM_INFO support to set/get
> * GEM object's debug name
> - * - 1.5.0 - Add SUBMITQUERY_QUERY ioctl
> + * - 1.5.0 - Add SUBMITQUEUE_QUERY ioctl
> + * - 1.6.0 - Add WAIT_IOVA ioctl
> */
> #define MSM_VERSION_MAJOR1
> -#define MSM_VERSION_MINOR5
> +#define MSM_VERSION_MINOR6
> #define MSM_VERSION_PATCHLEVEL 0
>
> static const struct drm_mode_config_funcs mode_config_funcs = {
> @@ -952,6 +953,63 @@ static int msm_ioctl_submitqueue_close(struct drm_device
> *dev, void *data,
> return msm_submitqueue_remove(file->driver_priv, id);
> }
>
> +static int msm_ioctl_wait_iova(struct drm_device *dev, void *data,
> + struct drm_file *file)
> +{
> + struct msm_drm_private *priv = dev->dev_private;
> + struct drm_gem_object *obj;
> + struct drm_msm_wait_iova *args = data;
> + ktime_t timeout = to_ktime(args->timeout);
> + unsigned long remaining_jiffies = timeout_to_jiffies(&timeout);
> + struct msm_gpu *gpu = priv->gpu;
> + void *base_vaddr;
> + uint64_t *vaddr;
> + int ret;
> +
> + if (args->pad)
> + return -EINVAL;
> +
> + if (!gpu)
> + return 0;
If the GPU isn't up, it should be an error since this macro is specifically
designed for just the GPU (though, I suppose the display *COULD* use it to watch
a memory mapped register or something).
> +
> + obj = drm_gem_object_lookup(file, args->handle);
> + if (!obj)
> + return -ENOENT;
> +
> + base_vaddr = msm_gem_get_vaddr(obj);
> + if (IS_ERR(base_vaddr)) {
> + ret = PTR_ERR(base_vaddr);
> + goto err_put_gem_object;
> + }
> + if (args->offset + sizeof(*vaddr) > obj->size) {
There is a chance to trigger a u64 overflow here resulting in an arbitrary (ish)
vaddr two lines below.
> + ret = -EINVAL;
> + goto err_put_vaddr;
> + }
You can check this before getting the vaddr which would save you a clean up
step.
> +
> + vaddr = base_vaddr + args->offset;
> +
> + /* Assumes WC mapping */
> + ret = wait_event_interruptible_timeout(
> + gpu->event, *vaddr >= args->value, remaining_jiffies);
I feel like a barrier might be needed before checking *vaddr just in case you
get the interrupt and wake up the queue before the write posts from the
hardware.
> +
> + if (ret == 0) {
> + ret = -ETIMEDOUT;
> + goto err_put_vaddr;
> + } else if (ret == -ERESTARTSYS) {
> + goto err_put_vaddr;
> + }
You don't need either goto here because both paths execute the following cleanup
steps. I'm also not sure you need to worry about explicitly checking the
ERESTARTSYS value, I think that this would be sufficient:
if (ret == 0)
ret = -ETIMEDOUT;
else if (ret > 0)
ret = 0;
> +
Put your err_put_vaddr: label here, but looking up, if you move the bounds check
before the msm_gem_get_vaddr, I don't think you need a label.
> + msm_gem_put_vaddr(obj);
Put the err_put_gem_object: label here.
> + drm_gem_object_put_unlocked(obj);
> + return 0;
return ret;
> +
> +err_put_vaddr:
> + msm_gem_put_vaddr(obj);
> +err_put_gem_object:
> + drm_gem_object_put_unlocked(obj);
> + return ret;
> +}
And then these guys aren't needed.
> +
> static const struct drm_ioctl_desc msm_ioctls[] = {
> DRM_IOCTL_DEF_DRV(MSM_GET_PARAM,msm_ioctl_get_param,
> DRM_RENDER_ALLOW),
> DRM_IOCTL_DEF_DRV(MSM_GEM_NEW, msm_ioctl_gem_new,
> DRM_RENDER_ALLOW),
> @@ -964,6 +1022,7 @@ static const struct drm_ioctl_desc msm_ioctls[] = {
> DRM_IOCTL_DEF_DRV(MSM_SUBMITQUEUE_NEW, msm_ioctl_submitqueue_new,
> DRM_RENDER_ALLOW),
> DRM_IOCTL_DEF_DRV(MSM_SUBMITQUEUE_CLOSE, msm_ioctl_submitqueue_close,
> DRM_RENDER_ALLOW),
> DRM_IOCTL_DEF_DRV(MSM_SUBMITQUEUE_QUERY, msm_ioctl_submitqueue_query,
> DRM_RENDER_ALLOW),
> + DRM_IOCTL_DEF_DRV(MSM_WAIT_IOVA, msm_ioctl_wait_iova, DRM_RENDER_ALLOW),
> };
>
> static const struct vm_operations_struct vm_ops = {
> diff --git a/inc
Re: [Freedreno] [PATCH 2/2] drm/msm: Add MSM_WAIT_IOVA ioctl
On Mon, Jan 13, 2020 at 7:37 AM Brian Ho wrote:
>
> Implements an ioctl to wait until a value at a given iova is greater
> than or equal to a supplied value.
>
> This will initially be used by turnip (open-source Vulkan driver for
> QC in mesa) for occlusion queries where the userspace driver can
> block on a query becoming available before continuing via
> vkGetQueryPoolResults.
>
> Signed-off-by: Brian Ho
> ---
> drivers/gpu/drm/msm/msm_drv.c | 63 +--
> include/uapi/drm/msm_drm.h| 13
> 2 files changed, 74 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c
> index c84f0a8b3f2c..dcc46874a5a2 100644
> --- a/drivers/gpu/drm/msm/msm_drv.c
> +++ b/drivers/gpu/drm/msm/msm_drv.c
> @@ -36,10 +36,11 @@
> * MSM_GEM_INFO ioctl.
> * - 1.4.0 - softpin, MSM_RELOC_BO_DUMP, and GEM_INFO support to set/get
> * GEM object's debug name
> - * - 1.5.0 - Add SUBMITQUERY_QUERY ioctl
> + * - 1.5.0 - Add SUBMITQUEUE_QUERY ioctl
> + * - 1.6.0 - Add WAIT_IOVA ioctl
> */
> #define MSM_VERSION_MAJOR 1
> -#define MSM_VERSION_MINOR 5
> +#define MSM_VERSION_MINOR 6
> #define MSM_VERSION_PATCHLEVEL 0
>
> static const struct drm_mode_config_funcs mode_config_funcs = {
> @@ -952,6 +953,63 @@ static int msm_ioctl_submitqueue_close(struct drm_device
> *dev, void *data,
> return msm_submitqueue_remove(file->driver_priv, id);
> }
>
> +static int msm_ioctl_wait_iova(struct drm_device *dev, void *data,
> + struct drm_file *file)
> +{
> + struct msm_drm_private *priv = dev->dev_private;
> + struct drm_gem_object *obj;
> + struct drm_msm_wait_iova *args = data;
> + ktime_t timeout = to_ktime(args->timeout);
> + unsigned long remaining_jiffies = timeout_to_jiffies(&timeout);
> + struct msm_gpu *gpu = priv->gpu;
> + void *base_vaddr;
> + uint64_t *vaddr;
> + int ret;
> +
> + if (args->pad)
> + return -EINVAL;
> +
> + if (!gpu)
> + return 0;
hmm, I'm not sure we should return zero in this case.. maybe -ENODEV?
> +
> + obj = drm_gem_object_lookup(file, args->handle);
> + if (!obj)
> + return -ENOENT;
> +
> + base_vaddr = msm_gem_get_vaddr(obj);
> + if (IS_ERR(base_vaddr)) {
> + ret = PTR_ERR(base_vaddr);
> + goto err_put_gem_object;
> + }
> + if (args->offset + sizeof(*vaddr) > obj->size) {
> + ret = -EINVAL;
> + goto err_put_vaddr;
> + }
> +
> + vaddr = base_vaddr + args->offset;
> +
> + /* Assumes WC mapping */
> + ret = wait_event_interruptible_timeout(
> + gpu->event, *vaddr >= args->value, remaining_jiffies);
> +
> + if (ret == 0) {
> + ret = -ETIMEDOUT;
> + goto err_put_vaddr;
> + } else if (ret == -ERESTARTSYS) {
> + goto err_put_vaddr;
> + }
maybe:
} else {
ret = 0;
}
and then drop the next three lines?
> +
> + msm_gem_put_vaddr(obj);
> + drm_gem_object_put_unlocked(obj);
> + return 0;
> +
> +err_put_vaddr:
> + msm_gem_put_vaddr(obj);
> +err_put_gem_object:
> + drm_gem_object_put_unlocked(obj);
> + return ret;
> +}
> +
> static const struct drm_ioctl_desc msm_ioctls[] = {
> DRM_IOCTL_DEF_DRV(MSM_GET_PARAM,msm_ioctl_get_param,
> DRM_RENDER_ALLOW),
> DRM_IOCTL_DEF_DRV(MSM_GEM_NEW, msm_ioctl_gem_new,
> DRM_RENDER_ALLOW),
> @@ -964,6 +1022,7 @@ static const struct drm_ioctl_desc msm_ioctls[] = {
> DRM_IOCTL_DEF_DRV(MSM_SUBMITQUEUE_NEW, msm_ioctl_submitqueue_new,
> DRM_RENDER_ALLOW),
> DRM_IOCTL_DEF_DRV(MSM_SUBMITQUEUE_CLOSE, msm_ioctl_submitqueue_close,
> DRM_RENDER_ALLOW),
> DRM_IOCTL_DEF_DRV(MSM_SUBMITQUEUE_QUERY, msm_ioctl_submitqueue_query,
> DRM_RENDER_ALLOW),
> + DRM_IOCTL_DEF_DRV(MSM_WAIT_IOVA, msm_ioctl_wait_iova,
> DRM_RENDER_ALLOW),
> };
>
> static const struct vm_operations_struct vm_ops = {
> diff --git a/include/uapi/drm/msm_drm.h b/include/uapi/drm/msm_drm.h
> index 0b85ed6a3710..8477f28a4ee1 100644
> --- a/include/uapi/drm/msm_drm.h
> +++ b/include/uapi/drm/msm_drm.h
> @@ -298,6 +298,17 @@ struct drm_msm_submitqueue_query {
> __u32 pad;
> };
>
> +/* This ioctl blocks until the u64 value at bo + offset is greater than or
> + * equal to the reference value.
> + */
> +struct drm_msm_wait_iova {
> + __u32 handle; /* in, GEM handle */
> + __u32 pad;
> + struct drm_msm_timespec timeout; /* in */
> + __u64 offset; /* offset into bo */
> + __u64 value; /* reference value */
Maybe we should go ahead and add a __u64 mask;
that would let us wait for 32b values as well, and wait for bits in a bitmask
Other than those minor comments, it looks pretty good to me
BR,
-R
> +};
> +
[Freedreno] [PATCH 2/2] drm/msm: Add MSM_WAIT_IOVA ioctl
Implements an ioctl to wait until a value at a given iova is greater
than or equal to a supplied value.
This will initially be used by turnip (open-source Vulkan driver for
QC in mesa) for occlusion queries where the userspace driver can
block on a query becoming available before continuing via
vkGetQueryPoolResults.
Signed-off-by: Brian Ho
---
drivers/gpu/drm/msm/msm_drv.c | 63 +--
include/uapi/drm/msm_drm.h| 13
2 files changed, 74 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c
index c84f0a8b3f2c..dcc46874a5a2 100644
--- a/drivers/gpu/drm/msm/msm_drv.c
+++ b/drivers/gpu/drm/msm/msm_drv.c
@@ -36,10 +36,11 @@
* MSM_GEM_INFO ioctl.
* - 1.4.0 - softpin, MSM_RELOC_BO_DUMP, and GEM_INFO support to set/get
* GEM object's debug name
- * - 1.5.0 - Add SUBMITQUERY_QUERY ioctl
+ * - 1.5.0 - Add SUBMITQUEUE_QUERY ioctl
+ * - 1.6.0 - Add WAIT_IOVA ioctl
*/
#define MSM_VERSION_MAJOR 1
-#define MSM_VERSION_MINOR 5
+#define MSM_VERSION_MINOR 6
#define MSM_VERSION_PATCHLEVEL 0
static const struct drm_mode_config_funcs mode_config_funcs = {
@@ -952,6 +953,63 @@ static int msm_ioctl_submitqueue_close(struct drm_device
*dev, void *data,
return msm_submitqueue_remove(file->driver_priv, id);
}
+static int msm_ioctl_wait_iova(struct drm_device *dev, void *data,
+ struct drm_file *file)
+{
+ struct msm_drm_private *priv = dev->dev_private;
+ struct drm_gem_object *obj;
+ struct drm_msm_wait_iova *args = data;
+ ktime_t timeout = to_ktime(args->timeout);
+ unsigned long remaining_jiffies = timeout_to_jiffies(&timeout);
+ struct msm_gpu *gpu = priv->gpu;
+ void *base_vaddr;
+ uint64_t *vaddr;
+ int ret;
+
+ if (args->pad)
+ return -EINVAL;
+
+ if (!gpu)
+ return 0;
+
+ obj = drm_gem_object_lookup(file, args->handle);
+ if (!obj)
+ return -ENOENT;
+
+ base_vaddr = msm_gem_get_vaddr(obj);
+ if (IS_ERR(base_vaddr)) {
+ ret = PTR_ERR(base_vaddr);
+ goto err_put_gem_object;
+ }
+ if (args->offset + sizeof(*vaddr) > obj->size) {
+ ret = -EINVAL;
+ goto err_put_vaddr;
+ }
+
+ vaddr = base_vaddr + args->offset;
+
+ /* Assumes WC mapping */
+ ret = wait_event_interruptible_timeout(
+ gpu->event, *vaddr >= args->value, remaining_jiffies);
+
+ if (ret == 0) {
+ ret = -ETIMEDOUT;
+ goto err_put_vaddr;
+ } else if (ret == -ERESTARTSYS) {
+ goto err_put_vaddr;
+ }
+
+ msm_gem_put_vaddr(obj);
+ drm_gem_object_put_unlocked(obj);
+ return 0;
+
+err_put_vaddr:
+ msm_gem_put_vaddr(obj);
+err_put_gem_object:
+ drm_gem_object_put_unlocked(obj);
+ return ret;
+}
+
static const struct drm_ioctl_desc msm_ioctls[] = {
DRM_IOCTL_DEF_DRV(MSM_GET_PARAM,msm_ioctl_get_param,
DRM_RENDER_ALLOW),
DRM_IOCTL_DEF_DRV(MSM_GEM_NEW, msm_ioctl_gem_new,
DRM_RENDER_ALLOW),
@@ -964,6 +1022,7 @@ static const struct drm_ioctl_desc msm_ioctls[] = {
DRM_IOCTL_DEF_DRV(MSM_SUBMITQUEUE_NEW, msm_ioctl_submitqueue_new,
DRM_RENDER_ALLOW),
DRM_IOCTL_DEF_DRV(MSM_SUBMITQUEUE_CLOSE, msm_ioctl_submitqueue_close,
DRM_RENDER_ALLOW),
DRM_IOCTL_DEF_DRV(MSM_SUBMITQUEUE_QUERY, msm_ioctl_submitqueue_query,
DRM_RENDER_ALLOW),
+ DRM_IOCTL_DEF_DRV(MSM_WAIT_IOVA, msm_ioctl_wait_iova, DRM_RENDER_ALLOW),
};
static const struct vm_operations_struct vm_ops = {
diff --git a/include/uapi/drm/msm_drm.h b/include/uapi/drm/msm_drm.h
index 0b85ed6a3710..8477f28a4ee1 100644
--- a/include/uapi/drm/msm_drm.h
+++ b/include/uapi/drm/msm_drm.h
@@ -298,6 +298,17 @@ struct drm_msm_submitqueue_query {
__u32 pad;
};
+/* This ioctl blocks until the u64 value at bo + offset is greater than or
+ * equal to the reference value.
+ */
+struct drm_msm_wait_iova {
+ __u32 handle; /* in, GEM handle */
+ __u32 pad;
+ struct drm_msm_timespec timeout; /* in */
+ __u64 offset; /* offset into bo */
+ __u64 value; /* reference value */
+};
+
#define DRM_MSM_GET_PARAM 0x00
/* placeholder:
#define DRM_MSM_SET_PARAM 0x01
@@ -315,6 +326,7 @@ struct drm_msm_submitqueue_query {
#define DRM_MSM_SUBMITQUEUE_NEW0x0A
#define DRM_MSM_SUBMITQUEUE_CLOSE 0x0B
#define DRM_MSM_SUBMITQUEUE_QUERY 0x0C
+#define DRM_MSM_WAIT_IOVA 0x0D
#define DRM_IOCTL_MSM_GET_PARAMDRM_IOWR(DRM_COMMAND_BASE +
DRM_MSM_GET_PARAM, struct drm_msm_param)
#define DRM_IOCTL_MSM_GEM_NEW DRM_IOWR(DRM_COMMAND_BASE +
DRM_MSM_GEM_NEW, struct drm_msm_gem_new)
@@ -327,6 +339,7 @@ struct drm_msm_submitqueue_query {
#define DRM_IOCTL_MSM_SUBMITQU
[Freedreno] [PATCH 2/2] drm/msm: Add MSM_WAIT_IOVA ioctl
Implements an ioctl to wait until a value at a given iova is greater
than or equal to a supplied value.
This will initially be used by turnip (open-source Vulkan driver for
QC in mesa) for occlusion queries where the userspace driver can
block on a query becoming available before continuing via
vkGetQueryPoolResults.
Signed-off-by: Brian Ho
---
drivers/gpu/drm/msm/msm_drv.c | 60 +--
include/uapi/drm/msm_drm.h| 12 +++
2 files changed, 70 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c
index c84f0a8b3f2c..57b3f12182fe 100644
--- a/drivers/gpu/drm/msm/msm_drv.c
+++ b/drivers/gpu/drm/msm/msm_drv.c
@@ -36,10 +36,11 @@
* MSM_GEM_INFO ioctl.
* - 1.4.0 - softpin, MSM_RELOC_BO_DUMP, and GEM_INFO support to set/get
* GEM object's debug name
- * - 1.5.0 - Add SUBMITQUERY_QUERY ioctl
+ * - 1.5.0 - Add SUBMITQUEUE_QUERY ioctl
+ * - 1.6.0 - Add WAIT_IOVA ioctl
*/
#define MSM_VERSION_MAJOR 1
-#define MSM_VERSION_MINOR 5
+#define MSM_VERSION_MINOR 6
#define MSM_VERSION_PATCHLEVEL 0
static const struct drm_mode_config_funcs mode_config_funcs = {
@@ -952,6 +953,60 @@ static int msm_ioctl_submitqueue_close(struct drm_device
*dev, void *data,
return msm_submitqueue_remove(file->driver_priv, id);
}
+static int msm_ioctl_wait_iova(struct drm_device *dev, void *data,
+ struct drm_file *file)
+{
+ struct msm_drm_private *priv = dev->dev_private;
+ struct drm_gem_object *obj;
+ struct drm_msm_wait_iova *args = data;
+ ktime_t timeout = to_ktime(args->timeout);
+ unsigned long remaining_jiffies = timeout_to_jiffies(&timeout);
+ struct msm_gpu *gpu = priv->gpu;
+ void *base_vaddr;
+ uint64_t *vaddr;
+ int ret;
+
+ if (!gpu)
+ return 0;
+
+ obj = drm_gem_object_lookup(file, args->handle);
+ if (!obj)
+ return -ENOENT;
+
+ base_vaddr = msm_gem_get_vaddr(obj);
+ if (IS_ERR(base_vaddr)) {
+ ret = PTR_ERR(base_vaddr);
+ goto err_put_gem_object;
+ }
+ if (args->offset + sizeof(*vaddr) > obj->size) {
+ ret = -EINVAL;
+ goto err_put_vaddr;
+ }
+
+ vaddr = base_vaddr + args->offset;
+
+ /* Assumes WC mapping */
+ ret = wait_event_interruptible_timeout(
+ gpu->event, *vaddr >= args->value, remaining_jiffies);
+
+ if (ret == 0) {
+ ret = -ETIMEDOUT;
+ goto err_put_vaddr;
+ } else if (ret == -ERESTARTSYS) {
+ goto err_put_vaddr;
+ }
+
+ msm_gem_put_vaddr(obj);
+ drm_gem_object_put_unlocked(obj);
+ return 0;
+
+err_put_vaddr:
+ msm_gem_put_vaddr(obj);
+err_put_gem_object:
+ drm_gem_object_put_unlocked(obj);
+ return ret;
+}
+
static const struct drm_ioctl_desc msm_ioctls[] = {
DRM_IOCTL_DEF_DRV(MSM_GET_PARAM,msm_ioctl_get_param,
DRM_RENDER_ALLOW),
DRM_IOCTL_DEF_DRV(MSM_GEM_NEW, msm_ioctl_gem_new,
DRM_RENDER_ALLOW),
@@ -964,6 +1019,7 @@ static const struct drm_ioctl_desc msm_ioctls[] = {
DRM_IOCTL_DEF_DRV(MSM_SUBMITQUEUE_NEW, msm_ioctl_submitqueue_new,
DRM_RENDER_ALLOW),
DRM_IOCTL_DEF_DRV(MSM_SUBMITQUEUE_CLOSE, msm_ioctl_submitqueue_close,
DRM_RENDER_ALLOW),
DRM_IOCTL_DEF_DRV(MSM_SUBMITQUEUE_QUERY, msm_ioctl_submitqueue_query,
DRM_RENDER_ALLOW),
+ DRM_IOCTL_DEF_DRV(MSM_WAIT_IOVA, msm_ioctl_wait_iova, DRM_RENDER_ALLOW),
};
static const struct vm_operations_struct vm_ops = {
diff --git a/include/uapi/drm/msm_drm.h b/include/uapi/drm/msm_drm.h
index 0b85ed6a3710..8069e5628c5e 100644
--- a/include/uapi/drm/msm_drm.h
+++ b/include/uapi/drm/msm_drm.h
@@ -298,6 +298,16 @@ struct drm_msm_submitqueue_query {
__u32 pad;
};
+/* This ioctl blocks until the u64 value at bo + offset is greater than or
+ * equal to the reference value.
+ */
+struct drm_msm_wait_iova {
+ __u32 handle; /* in, GEM handle */
+ struct drm_msm_timespec timeout; /* in */
+ __u64 offset; /* offset into bo */
+ __u64 value; /* reference value */
+};
+
#define DRM_MSM_GET_PARAM 0x00
/* placeholder:
#define DRM_MSM_SET_PARAM 0x01
@@ -315,6 +325,7 @@ struct drm_msm_submitqueue_query {
#define DRM_MSM_SUBMITQUEUE_NEW0x0A
#define DRM_MSM_SUBMITQUEUE_CLOSE 0x0B
#define DRM_MSM_SUBMITQUEUE_QUERY 0x0C
+#define DRM_MSM_WAIT_IOVA 0x0D
#define DRM_IOCTL_MSM_GET_PARAMDRM_IOWR(DRM_COMMAND_BASE +
DRM_MSM_GET_PARAM, struct drm_msm_param)
#define DRM_IOCTL_MSM_GEM_NEW DRM_IOWR(DRM_COMMAND_BASE +
DRM_MSM_GEM_NEW, struct drm_msm_gem_new)
@@ -327,6 +338,7 @@ struct drm_msm_submitqueue_query {
#define DRM_IOCTL_MSM_SUBMITQUEUE_NEWDRM_IOWR(DRM_COMMAND_BASE +
DRM_MSM_SUBMITQUEUE_NEW, struct drm_m
