Re: [Freedreno] [PATCH v2] drm/msm: Add syncobj support.
On Mon, Jan 20, 2020 at 5:06 PM Jordan Crouse wrote:
>
> On Fri, Jan 17, 2020 at 07:32:27PM +0100, Bas Nieuwenhuizen wrote:
> > On Fri, Jan 17, 2020 at 7:17 PM Jordan Crouse
> > wrote:
> > >
> > > On Fri, Jan 17, 2020 at 12:04:17AM +0100, Bas Nieuwenhuizen wrote:
> > > > This
> > > >
> > > > 1) Enables core DRM syncobj support.
> > > > 2) Adds options to the submission ioctl to wait/signal syncobjs.
> > > >
> > > > Just like the wait fence fd, this does inline waits. Using the
> > > > scheduler would be nice but I believe it is out of scope for
> > > > this work.
> > > >
> > > > Support for timeline syncobjs is implemented and the interface
> > > > is ready for it, but I'm not enabling it yet until there is
> > > > some code for turnip to use it.
> > > >
> > > > The reset is mostly in there because in the presence of waiting
> > > > and signalling the same semaphores, resetting them after
> > > > signalling can become very annoying.
> > > >
> > > > v2:
> > > > - Fixed style issues
> > > > - Removed a cleanup issue in a failure case
> > > > - Moved to a copy_from_user per syncobj
> > >
> > > A few nits, but nothing serious. This is looking good, thanks for the
> > > quick
> > > turn around.
> > >
> > > > Signed-off-by: Bas Nieuwenhuizen
>
>
>
> > > > +out_syncobjs:
> > > > + if (ret && *syncobjs) {
> > > > + for (j = 0; j < i; ++j) {
> > >
> > > You could also walk backwards from i and save having another iterator:
> > >
> > > for( ; i >=0; i--)
> >
> > I thought about that but the slight annoyance is that from the API
> > perspective they're all unsigned so a >= 0 doesn't really work.
>
>
> > So we have 3 alternatives:
> >
> > 1) check for INT32_MAX and then use signed
> > 2) keep the iterator
> > 3) do { ... } while (i-- != 0);
> >
> > I think 1 adds extra complexity for no good reason. (unless we want to
> > implicitly rely on that failing the kmalloc?) Happy to do 3 if we're
> > happy with a do while construct.
>
> Ah, good point on the unsigned-ness. Keeping the iterator is fine. No reason
> to
> try to be overly clever.
>
> >
> > >
> > > > + if ((*syncobjs)[j])
> > > > + drm_syncobj_put((*syncobjs)[j]);
> > > > + }
> > > > + kfree(*syncobjs);
> > > > + *syncobjs = NULL;
> > > > + }
> > > > + return ret;
> > >
> > > if you wanted to you could return syncobjs or ERR_PTR instead of passing
> > > it by
> > > reference. I would probably chose that option personally but it is up to
> > > you.
> >
> > How does this work wrt returning different error codes?
>
> For each error code, you would use the ERR_PTR() macro, so for example,
>
> return ERR_PTR(-ENOMEM);
>
> and then for the success path you would return the actual pointer,
>
> return syncobjs;
>
> The caller would use the IS_ERR() macro to check the return value. It can also
> get the error code back with PTR_ERR() to send to the upper levels.
>
> It is up to you. I personally like using ERR_PTR() but as long as the code
> works
> whichever method you choose is fine by me.
> > >
> > > > +}
> > > > +
> > > > +static void msm_reset_syncobjs(struct drm_syncobj **syncobjs,
> > > > + uint32_t nr_syncobjs)
> > > > +{
> > > > + uint32_t i;
> > > > +
> > > > + for (i = 0; syncobjs && i < nr_syncobjs; ++i) {
> > > > + if (syncobjs[i])
> > > > + drm_syncobj_replace_fence(syncobjs[i], NULL);
> > > > + }
> > > > +}
> > > > +
> > > > +static int msm_parse_post_deps(struct drm_device *dev,
> > > > + struct drm_file *file,
> > > > + uint64_t out_syncobjs_addr,
> > > > + uint32_t nr_out_syncobjs,
> > > > +uint32_t syncobj_stride,
> > > > + struct msm_submit_post_dep **post_deps)
> > > > +{
> > > > + int ret = 0;
> > > > + uint32_t i, j;
> > > > +
> > > > + *post_deps = kmalloc_array(nr_out_syncobjs, sizeof(**post_deps),
> > > > +GFP_KERNEL | __GFP_NOWARN |
> > > > __GFP_NORETRY);
> > > > + if (!*post_deps) {
> > > > + ret = -ENOMEM;
> > > > + goto out_syncobjs;
> > >
> > > return -ENOMEM works fine.
> > >
> > > > + }
> > > > +
> > > > + for (i = 0; i < nr_out_syncobjs; ++i) {
> > > > + uint64_t address = out_syncobjs_addr + i * syncobj_stride;
> > > > +
> > > > + if (copy_from_user(&syncobj_desc,
> > > > +u64_to_user_ptr(address),
> > > > +min(syncobj_stride,
> > > > sizeof(syncobj_desc {
> > > > + ret = -EFAULT;
> > > > + goto out_syncobjs;
> > >
> > > You can break here.
> > >
> > > > + }
> > > > +
> > > > + (*post_deps)[i].point = syncobj_desc.point;
> > > > + (*post_deps)[i].ch
Re: [Freedreno] [PATCH v2] drm/msm: Add syncobj support.
On Fri, Jan 17, 2020 at 07:32:27PM +0100, Bas Nieuwenhuizen wrote:
> On Fri, Jan 17, 2020 at 7:17 PM Jordan Crouse wrote:
> >
> > On Fri, Jan 17, 2020 at 12:04:17AM +0100, Bas Nieuwenhuizen wrote:
> > > This
> > >
> > > 1) Enables core DRM syncobj support.
> > > 2) Adds options to the submission ioctl to wait/signal syncobjs.
> > >
> > > Just like the wait fence fd, this does inline waits. Using the
> > > scheduler would be nice but I believe it is out of scope for
> > > this work.
> > >
> > > Support for timeline syncobjs is implemented and the interface
> > > is ready for it, but I'm not enabling it yet until there is
> > > some code for turnip to use it.
> > >
> > > The reset is mostly in there because in the presence of waiting
> > > and signalling the same semaphores, resetting them after
> > > signalling can become very annoying.
> > >
> > > v2:
> > > - Fixed style issues
> > > - Removed a cleanup issue in a failure case
> > > - Moved to a copy_from_user per syncobj
> >
> > A few nits, but nothing serious. This is looking good, thanks for the quick
> > turn around.
> >
> > > Signed-off-by: Bas Nieuwenhuizen
> > > +out_syncobjs:
> > > + if (ret && *syncobjs) {
> > > + for (j = 0; j < i; ++j) {
> >
> > You could also walk backwards from i and save having another iterator:
> >
> > for( ; i >=0; i--)
>
> I thought about that but the slight annoyance is that from the API
> perspective they're all unsigned so a >= 0 doesn't really work.
> So we have 3 alternatives:
>
> 1) check for INT32_MAX and then use signed
> 2) keep the iterator
> 3) do { ... } while (i-- != 0);
>
> I think 1 adds extra complexity for no good reason. (unless we want to
> implicitly rely on that failing the kmalloc?) Happy to do 3 if we're
> happy with a do while construct.
Ah, good point on the unsigned-ness. Keeping the iterator is fine. No reason to
try to be overly clever.
>
> >
> > > + if ((*syncobjs)[j])
> > > + drm_syncobj_put((*syncobjs)[j]);
> > > + }
> > > + kfree(*syncobjs);
> > > + *syncobjs = NULL;
> > > + }
> > > + return ret;
> >
> > if you wanted to you could return syncobjs or ERR_PTR instead of passing it
> > by
> > reference. I would probably chose that option personally but it is up to
> > you.
>
> How does this work wrt returning different error codes?
For each error code, you would use the ERR_PTR() macro, so for example,
return ERR_PTR(-ENOMEM);
and then for the success path you would return the actual pointer,
return syncobjs;
The caller would use the IS_ERR() macro to check the return value. It can also
get the error code back with PTR_ERR() to send to the upper levels.
It is up to you. I personally like using ERR_PTR() but as long as the code works
whichever method you choose is fine by me.
> >
> > > +}
> > > +
> > > +static void msm_reset_syncobjs(struct drm_syncobj **syncobjs,
> > > + uint32_t nr_syncobjs)
> > > +{
> > > + uint32_t i;
> > > +
> > > + for (i = 0; syncobjs && i < nr_syncobjs; ++i) {
> > > + if (syncobjs[i])
> > > + drm_syncobj_replace_fence(syncobjs[i], NULL);
> > > + }
> > > +}
> > > +
> > > +static int msm_parse_post_deps(struct drm_device *dev,
> > > + struct drm_file *file,
> > > + uint64_t out_syncobjs_addr,
> > > + uint32_t nr_out_syncobjs,
> > > +uint32_t syncobj_stride,
> > > + struct msm_submit_post_dep **post_deps)
> > > +{
> > > + int ret = 0;
> > > + uint32_t i, j;
> > > +
> > > + *post_deps = kmalloc_array(nr_out_syncobjs, sizeof(**post_deps),
> > > +GFP_KERNEL | __GFP_NOWARN |
> > > __GFP_NORETRY);
> > > + if (!*post_deps) {
> > > + ret = -ENOMEM;
> > > + goto out_syncobjs;
> >
> > return -ENOMEM works fine.
> >
> > > + }
> > > +
> > > + for (i = 0; i < nr_out_syncobjs; ++i) {
> > > + uint64_t address = out_syncobjs_addr + i * syncobj_stride;
> > > +
> > > + if (copy_from_user(&syncobj_desc,
> > > +u64_to_user_ptr(address),
> > > +min(syncobj_stride,
> > > sizeof(syncobj_desc {
> > > + ret = -EFAULT;
> > > + goto out_syncobjs;
> >
> > You can break here.
> >
> > > + }
> > > +
> > > + (*post_deps)[i].point = syncobj_desc.point;
> > > + (*post_deps)[i].chain = NULL;
> > > +
> > > + if (syncobj_desc.flags) {
> > > + ret = -EINVAL;
> > > + break;
> > > + }
> > > +
> > > + if (syncobj_desc.point) {
> > > + if (!drm_core_check_feature(dev,
> > > +
Re: [PATCH v2] drm/msm: Add syncobj support.
Hi Bas,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on tegra-drm/drm/tegra/for-next]
[also build test ERROR on drm-tip/drm-tip linus/master v5.5-rc6 next-20200117]
[cannot apply to drm-exynos/exynos-drm-next drm-intel/for-linux-next
drm/drm-next]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]
url:
https://github.com/0day-ci/linux/commits/Bas-Nieuwenhuizen/drm-msm-Add-syncobj-support/20200118-033342
base: git://anongit.freedesktop.org/tegra/linux.git drm/tegra/for-next
config: arm-defconfig (attached as .config)
compiler: arm-linux-gnueabi-gcc (GCC) 7.5.0
reproduce:
wget
https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O
~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
GCC_VERSION=7.5.0 make.cross ARCH=arm
If you fix the issue, kindly add following tag
Reported-by: kbuild test robot
All error/warnings (new ones prefixed by >>):
drivers/gpu/drm/msm/msm_gem_submit.c: In function 'msm_parse_post_deps':
>> drivers/gpu/drm/msm/msm_gem_submit.c:512:23: error: 'syncobj_desc'
>> undeclared (first use in this function); did you mean 'syncobj_stride'?
if (copy_from_user(&syncobj_desc,
^~~~
syncobj_stride
drivers/gpu/drm/msm/msm_gem_submit.c:512:23: note: each undeclared
identifier is reported only once for each function it appears in
In file included from include/linux/list.h:9:0,
from include/linux/preempt.h:11,
from include/linux/spinlock.h:51,
from include/linux/seqlock.h:36,
from include/linux/time.h:6,
from include/linux/ktime.h:24,
from include/linux/sync_file.h:17,
from drivers/gpu/drm/msm/msm_gem_submit.c:8:
>> include/linux/kernel.h:868:2: error: first argument to
>> '__builtin_choose_expr' not a constant
__builtin_choose_expr(__safe_cmp(x, y), \
^
include/linux/kernel.h:877:19: note: in expansion of macro '__careful_cmp'
#define min(x, y) __careful_cmp(x, y, <)
^
>> drivers/gpu/drm/msm/msm_gem_submit.c:514:15: note: in expansion of macro
>> 'min'
min(syncobj_stride, sizeof(syncobj_desc {
^~~
vim +512 drivers/gpu/drm/msm/msm_gem_submit.c
491
492 static int msm_parse_post_deps(struct drm_device *dev,
493 struct drm_file *file,
494 uint64_t out_syncobjs_addr,
495 uint32_t nr_out_syncobjs,
496 uint32_t syncobj_stride,
497 struct msm_submit_post_dep **post_deps)
498 {
499 int ret = 0;
500 uint32_t i, j;
501
502 *post_deps = kmalloc_array(nr_out_syncobjs, sizeof(**post_deps),
503 GFP_KERNEL | __GFP_NOWARN |
__GFP_NORETRY);
504 if (!*post_deps) {
505 ret = -ENOMEM;
506 goto out_syncobjs;
507 }
508
509 for (i = 0; i < nr_out_syncobjs; ++i) {
510 uint64_t address = out_syncobjs_addr + i *
syncobj_stride;
511
> 512 if (copy_from_user(&syncobj_desc,
513 u64_to_user_ptr(address),
> 514 min(syncobj_stride,
> sizeof(syncobj_desc {
515 ret = -EFAULT;
516 goto out_syncobjs;
517 }
518
519 (*post_deps)[i].point = syncobj_desc.point;
520 (*post_deps)[i].chain = NULL;
521
522 if (syncobj_desc.flags) {
523 ret = -EINVAL;
524 break;
525 }
526
527 if (syncobj_desc.point) {
528 if (!drm_core_check_feature(dev,
529
DRIVER_SYNCOBJ_TIMELINE)) {
530 ret = -EOPNOTSUPP;
531 break;
532 }
533
534 (*post_deps)[i].chain =
535 kmalloc(sizeof(*(*post_deps)[i].chain),
536 GFP_KERNEL);
537 if (!(*post_deps)[i].chain) {
538 ret = -ENOMEM;
539 break;
540 }
541 }
Re: [PATCH v2] drm/msm: Add syncobj support.
On Fri, Jan 17, 2020 at 7:17 PM Jordan Crouse wrote:
>
> On Fri, Jan 17, 2020 at 12:04:17AM +0100, Bas Nieuwenhuizen wrote:
> > This
> >
> > 1) Enables core DRM syncobj support.
> > 2) Adds options to the submission ioctl to wait/signal syncobjs.
> >
> > Just like the wait fence fd, this does inline waits. Using the
> > scheduler would be nice but I believe it is out of scope for
> > this work.
> >
> > Support for timeline syncobjs is implemented and the interface
> > is ready for it, but I'm not enabling it yet until there is
> > some code for turnip to use it.
> >
> > The reset is mostly in there because in the presence of waiting
> > and signalling the same semaphores, resetting them after
> > signalling can become very annoying.
> >
> > v2:
> > - Fixed style issues
> > - Removed a cleanup issue in a failure case
> > - Moved to a copy_from_user per syncobj
>
> A few nits, but nothing serious. This is looking good, thanks for the quick
> turn around.
>
> > Signed-off-by: Bas Nieuwenhuizen
> > ---
> > drivers/gpu/drm/msm/msm_drv.c| 6 +-
> > drivers/gpu/drm/msm/msm_gem_submit.c | 236 ++-
> > include/uapi/drm/msm_drm.h | 24 ++-
> > 3 files changed, 262 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c
> > index c84f0a8b3f2c..5246b41798df 100644
> > --- a/drivers/gpu/drm/msm/msm_drv.c
> > +++ b/drivers/gpu/drm/msm/msm_drv.c
> > @@ -37,9 +37,10 @@
> > * - 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.6.0 - Syncobj support
> > */
> > #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 = {
> > @@ -988,7 +989,8 @@ static struct drm_driver msm_driver = {
> > .driver_features= DRIVER_GEM |
> > DRIVER_RENDER |
> > DRIVER_ATOMIC |
> > - DRIVER_MODESET,
> > + DRIVER_MODESET |
> > + DRIVER_SYNCOBJ,
> > .open = msm_open,
> > .postclose = msm_postclose,
> > .lastclose = drm_fb_helper_lastclose,
> > diff --git a/drivers/gpu/drm/msm/msm_gem_submit.c
> > b/drivers/gpu/drm/msm/msm_gem_submit.c
> > index be5327af16fa..6c7f95fc5cfd 100644
> > --- a/drivers/gpu/drm/msm/msm_gem_submit.c
> > +++ b/drivers/gpu/drm/msm/msm_gem_submit.c
> > @@ -8,7 +8,9 @@
> > #include
> > #include
> >
> > +#include
> > #include
> > +#include
> >
> > #include "msm_drv.h"
> > #include "msm_gpu.h"
> > @@ -394,6 +396,191 @@ static void submit_cleanup(struct msm_gem_submit
> > *submit)
> > ww_acquire_fini(&submit->ticket);
> > }
> >
> > +
> > +struct msm_submit_post_dep {
> > + struct drm_syncobj *syncobj;
> > + uint64_t point;
> > + struct dma_fence_chain *chain;
> > +};
> > +
> > +static int msm_wait_deps(struct drm_device *dev,
> > + struct drm_file *file,
> > + uint64_t in_syncobjs_addr,
> > + uint32_t nr_in_syncobjs,
> > + uint32_t syncobj_stride,
> > + struct msm_ringbuffer *ring,
> > + struct drm_syncobj ***syncobjs)
> > +{
> > + struct drm_msm_gem_submit_syncobj syncobj_desc = {0};
> > + int ret = 0;
> > + uint32_t i, j;
> > +
> > + *syncobjs = kcalloc(nr_in_syncobjs, sizeof(**syncobjs),
> > + GFP_KERNEL | __GFP_NOWARN | __GFP_NORETRY);
>
> Perfect - I think this will do everything we want - protect us against OOM
> death
> but also not introduce artificial constraints on object counts.
>
> > + if (!syncobjs) {
>
> You should be able to just return -ENOMEM here.
>
> > + ret = -ENOMEM;
> > + goto out_syncobjs;
> > + }
>
> > +
> > + for (i = 0; i < nr_in_syncobjs; ++i) {
> > + uint64_t address = in_syncobjs_addr + i * syncobj_stride;
> > + struct dma_fence *fence;
> > +
> > + if (copy_from_user(&syncobj_desc,
> > +u64_to_user_ptr(address),
> > +min(syncobj_stride,
> > sizeof(syncobj_desc {
> > + ret = -EFAULT;
> > + goto out_syncobjs;
>
> You can just use break here.
>
> > + }
> > +
> > + if (syncobj_desc.point &&
> > + !drm_core_check_feature(dev, DRIVER_SYNCOBJ_TIMELINE)) {
> > + ret = -EOPNOTSUPP;
> > + break;
> > + }
> > +
> > + if (syncobj_desc.flags & ~MSM_SUBMIT_SYNCOBJ_FLAGS) {
> > + ret = -EINVAL;
> > +
Re: [PATCH v2] drm/msm: Add syncobj support.
On Fri, Jan 17, 2020 at 12:04:17AM +0100, Bas Nieuwenhuizen wrote:
> This
>
> 1) Enables core DRM syncobj support.
> 2) Adds options to the submission ioctl to wait/signal syncobjs.
>
> Just like the wait fence fd, this does inline waits. Using the
> scheduler would be nice but I believe it is out of scope for
> this work.
>
> Support for timeline syncobjs is implemented and the interface
> is ready for it, but I'm not enabling it yet until there is
> some code for turnip to use it.
>
> The reset is mostly in there because in the presence of waiting
> and signalling the same semaphores, resetting them after
> signalling can become very annoying.
>
> v2:
> - Fixed style issues
> - Removed a cleanup issue in a failure case
> - Moved to a copy_from_user per syncobj
A few nits, but nothing serious. This is looking good, thanks for the quick
turn around.
> Signed-off-by: Bas Nieuwenhuizen
> ---
> drivers/gpu/drm/msm/msm_drv.c| 6 +-
> drivers/gpu/drm/msm/msm_gem_submit.c | 236 ++-
> include/uapi/drm/msm_drm.h | 24 ++-
> 3 files changed, 262 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c
> index c84f0a8b3f2c..5246b41798df 100644
> --- a/drivers/gpu/drm/msm/msm_drv.c
> +++ b/drivers/gpu/drm/msm/msm_drv.c
> @@ -37,9 +37,10 @@
> * - 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.6.0 - Syncobj support
> */
> #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 = {
> @@ -988,7 +989,8 @@ static struct drm_driver msm_driver = {
> .driver_features= DRIVER_GEM |
> DRIVER_RENDER |
> DRIVER_ATOMIC |
> - DRIVER_MODESET,
> + DRIVER_MODESET |
> + DRIVER_SYNCOBJ,
> .open = msm_open,
> .postclose = msm_postclose,
> .lastclose = drm_fb_helper_lastclose,
> diff --git a/drivers/gpu/drm/msm/msm_gem_submit.c
> b/drivers/gpu/drm/msm/msm_gem_submit.c
> index be5327af16fa..6c7f95fc5cfd 100644
> --- a/drivers/gpu/drm/msm/msm_gem_submit.c
> +++ b/drivers/gpu/drm/msm/msm_gem_submit.c
> @@ -8,7 +8,9 @@
> #include
> #include
>
> +#include
> #include
> +#include
>
> #include "msm_drv.h"
> #include "msm_gpu.h"
> @@ -394,6 +396,191 @@ static void submit_cleanup(struct msm_gem_submit
> *submit)
> ww_acquire_fini(&submit->ticket);
> }
>
> +
> +struct msm_submit_post_dep {
> + struct drm_syncobj *syncobj;
> + uint64_t point;
> + struct dma_fence_chain *chain;
> +};
> +
> +static int msm_wait_deps(struct drm_device *dev,
> + struct drm_file *file,
> + uint64_t in_syncobjs_addr,
> + uint32_t nr_in_syncobjs,
> + uint32_t syncobj_stride,
> + struct msm_ringbuffer *ring,
> + struct drm_syncobj ***syncobjs)
> +{
> + struct drm_msm_gem_submit_syncobj syncobj_desc = {0};
> + int ret = 0;
> + uint32_t i, j;
> +
> + *syncobjs = kcalloc(nr_in_syncobjs, sizeof(**syncobjs),
> + GFP_KERNEL | __GFP_NOWARN | __GFP_NORETRY);
Perfect - I think this will do everything we want - protect us against OOM death
but also not introduce artificial constraints on object counts.
> + if (!syncobjs) {
You should be able to just return -ENOMEM here.
> + ret = -ENOMEM;
> + goto out_syncobjs;
> + }
> +
> + for (i = 0; i < nr_in_syncobjs; ++i) {
> + uint64_t address = in_syncobjs_addr + i * syncobj_stride;
> + struct dma_fence *fence;
> +
> + if (copy_from_user(&syncobj_desc,
> +u64_to_user_ptr(address),
> +min(syncobj_stride, sizeof(syncobj_desc {
> + ret = -EFAULT;
> + goto out_syncobjs;
You can just use break here.
> + }
> +
> + if (syncobj_desc.point &&
> + !drm_core_check_feature(dev, DRIVER_SYNCOBJ_TIMELINE)) {
> + ret = -EOPNOTSUPP;
> + break;
> + }
> +
> + if (syncobj_desc.flags & ~MSM_SUBMIT_SYNCOBJ_FLAGS) {
> + ret = -EINVAL;
> + break;
> + }
> +
> + ret = drm_syncobj_find_fence(file, syncobj_desc.handle,
> + syncobj_desc.point, 0, &fence);
> + if (ret)
> + break;
> +
> + if (!dma_fence_match_context(fence, ring->fctx->context))
[PATCH v2] drm/msm: Add syncobj support.
This
1) Enables core DRM syncobj support.
2) Adds options to the submission ioctl to wait/signal syncobjs.
Just like the wait fence fd, this does inline waits. Using the
scheduler would be nice but I believe it is out of scope for
this work.
Support for timeline syncobjs is implemented and the interface
is ready for it, but I'm not enabling it yet until there is
some code for turnip to use it.
The reset is mostly in there because in the presence of waiting
and signalling the same semaphores, resetting them after
signalling can become very annoying.
v2:
- Fixed style issues
- Removed a cleanup issue in a failure case
- Moved to a copy_from_user per syncobj
Signed-off-by: Bas Nieuwenhuizen
---
drivers/gpu/drm/msm/msm_drv.c| 6 +-
drivers/gpu/drm/msm/msm_gem_submit.c | 236 ++-
include/uapi/drm/msm_drm.h | 24 ++-
3 files changed, 262 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c
index c84f0a8b3f2c..5246b41798df 100644
--- a/drivers/gpu/drm/msm/msm_drv.c
+++ b/drivers/gpu/drm/msm/msm_drv.c
@@ -37,9 +37,10 @@
* - 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.6.0 - Syncobj support
*/
#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 = {
@@ -988,7 +989,8 @@ static struct drm_driver msm_driver = {
.driver_features= DRIVER_GEM |
DRIVER_RENDER |
DRIVER_ATOMIC |
- DRIVER_MODESET,
+ DRIVER_MODESET |
+ DRIVER_SYNCOBJ,
.open = msm_open,
.postclose = msm_postclose,
.lastclose = drm_fb_helper_lastclose,
diff --git a/drivers/gpu/drm/msm/msm_gem_submit.c
b/drivers/gpu/drm/msm/msm_gem_submit.c
index be5327af16fa..6c7f95fc5cfd 100644
--- a/drivers/gpu/drm/msm/msm_gem_submit.c
+++ b/drivers/gpu/drm/msm/msm_gem_submit.c
@@ -8,7 +8,9 @@
#include
#include
+#include
#include
+#include
#include "msm_drv.h"
#include "msm_gpu.h"
@@ -394,6 +396,191 @@ static void submit_cleanup(struct msm_gem_submit *submit)
ww_acquire_fini(&submit->ticket);
}
+
+struct msm_submit_post_dep {
+ struct drm_syncobj *syncobj;
+ uint64_t point;
+ struct dma_fence_chain *chain;
+};
+
+static int msm_wait_deps(struct drm_device *dev,
+ struct drm_file *file,
+ uint64_t in_syncobjs_addr,
+ uint32_t nr_in_syncobjs,
+ uint32_t syncobj_stride,
+ struct msm_ringbuffer *ring,
+ struct drm_syncobj ***syncobjs)
+{
+ struct drm_msm_gem_submit_syncobj syncobj_desc = {0};
+ int ret = 0;
+ uint32_t i, j;
+
+ *syncobjs = kcalloc(nr_in_syncobjs, sizeof(**syncobjs),
+ GFP_KERNEL | __GFP_NOWARN | __GFP_NORETRY);
+ if (!syncobjs) {
+ ret = -ENOMEM;
+ goto out_syncobjs;
+ }
+
+ for (i = 0; i < nr_in_syncobjs; ++i) {
+ uint64_t address = in_syncobjs_addr + i * syncobj_stride;
+ struct dma_fence *fence;
+
+ if (copy_from_user(&syncobj_desc,
+ u64_to_user_ptr(address),
+ min(syncobj_stride, sizeof(syncobj_desc {
+ ret = -EFAULT;
+ goto out_syncobjs;
+ }
+
+ if (syncobj_desc.point &&
+ !drm_core_check_feature(dev, DRIVER_SYNCOBJ_TIMELINE)) {
+ ret = -EOPNOTSUPP;
+ break;
+ }
+
+ if (syncobj_desc.flags & ~MSM_SUBMIT_SYNCOBJ_FLAGS) {
+ ret = -EINVAL;
+ break;
+ }
+
+ ret = drm_syncobj_find_fence(file, syncobj_desc.handle,
+syncobj_desc.point, 0, &fence);
+ if (ret)
+ break;
+
+ if (!dma_fence_match_context(fence, ring->fctx->context))
+ ret = dma_fence_wait(fence, true);
+
+ dma_fence_put(fence);
+ if (ret)
+ break;
+
+ if (syncobj_desc.flags & MSM_SUBMIT_SYNCOBJ_RESET) {
+ (*syncobjs)[i] =
+ drm_syncobj_find(file, syncobj_desc.handle);
+ if (!(*syncobjs)[i]) {
+ ret = -EINVAL;
+ break;
+ }
+ }
+ }
+
+out_syncobjs:
+
