Re: [Intel-gfx] [PATCH 03/11] drm/panfrost: Use xarray and helpers for depedency tracking
On 02/06/2021 19:51, Daniel Vetter wrote: > On Wed, Jun 02, 2021 at 03:06:50PM +0100, Steven Price wrote: >> On 21/05/2021 10:09, Daniel Vetter wrote: [...] >>> + if (!xa_empty(>deps)) >>> + return xa_erase(>deps, job->last_dep++); >> >> Rather than tracking last_dep separately this could be written using >> xa_find(): >> >> if (xa_find(>deps, , ULONG_MAX, XA_PRESENT)) >> return xa_erase(>deps, ); > > I copypasted this from other drivers, imo consistency is better than > looking pretty. I think eventually we should stuff this as optional > helpers into drm/scheduler. > > Also yours walks the xa twice. Agreed this isn't as efficient (I was somewhat disappointed xarray doesn't expose a "get and remove the first element" API). Moving this into drm/scheduler seems like the right long term solution - so matching other drivers first is a good move. Thanks, Steve ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 03/11] drm/panfrost: Use xarray and helpers for depedency tracking
On Wed, Jun 02, 2021 at 03:06:50PM +0100, Steven Price wrote:
> On 21/05/2021 10:09, Daniel Vetter wrote:
> > More consistency and prep work for the next patch.
> >
> > Aside: I wonder whether we shouldn't just move this entire xarray
> > business into the scheduler so that not everyone has to reinvent the
> > same wheels. Cc'ing some scheduler people for this too.
> >
> > Cc: "Christian König"
> > Cc: Luben Tuikov
> > Cc: Alex Deucher
> > Cc: Lee Jones
> > Cc: Steven Price
> > Cc: Rob Herring
> > Cc: Tomeu Vizoso
> > Cc: Alyssa Rosenzweig
> > Cc: Sumit Semwal
> > Cc: linux-me...@vger.kernel.org
> > Cc: linaro-mm-...@lists.linaro.org
> > Signed-off-by: Daniel Vetter
>
> Two comments below, but otherwise looks like a nice cleanup.
Thanks for taking a look.
> > ---
> > drivers/gpu/drm/panfrost/panfrost_drv.c | 41 -
> > drivers/gpu/drm/panfrost/panfrost_job.c | 61 ++---
> > drivers/gpu/drm/panfrost/panfrost_job.h | 8 ++--
> > 3 files changed, 46 insertions(+), 64 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/panfrost/panfrost_drv.c
> > b/drivers/gpu/drm/panfrost/panfrost_drv.c
> > index ca07098a6141..7977b4752b5c 100644
> > --- a/drivers/gpu/drm/panfrost/panfrost_drv.c
> > +++ b/drivers/gpu/drm/panfrost/panfrost_drv.c
> > @@ -137,12 +137,6 @@ panfrost_lookup_bos(struct drm_device *dev,
> > if (!job->bo_count)
> > return 0;
> >
> > - job->implicit_fences = kvmalloc_array(job->bo_count,
> > - sizeof(struct dma_fence *),
> > - GFP_KERNEL | __GFP_ZERO);
> > - if (!job->implicit_fences)
> > - return -ENOMEM;
> > -
> > ret = drm_gem_objects_lookup(file_priv,
> > (void __user *)(uintptr_t)args->bo_handles,
> > job->bo_count, >bos);
> > @@ -173,7 +167,7 @@ panfrost_lookup_bos(struct drm_device *dev,
> > }
> >
> > /**
> > - * panfrost_copy_in_sync() - Sets up job->in_fences[] with the sync objects
> > + * panfrost_copy_in_sync() - Sets up job->deps with the sync objects
> > * referenced by the job.
> > * @dev: DRM device
> > * @file_priv: DRM file for this fd
> > @@ -193,22 +187,14 @@ panfrost_copy_in_sync(struct drm_device *dev,
> > {
> > u32 *handles;
> > int ret = 0;
> > - int i;
> > + int i, in_fence_count;
> >
> > - job->in_fence_count = args->in_sync_count;
> > + in_fence_count = args->in_sync_count;
> >
> > - if (!job->in_fence_count)
> > + if (!in_fence_count)
> > return 0;
> >
> > - job->in_fences = kvmalloc_array(job->in_fence_count,
> > - sizeof(struct dma_fence *),
> > - GFP_KERNEL | __GFP_ZERO);
> > - if (!job->in_fences) {
> > - DRM_DEBUG("Failed to allocate job in fences\n");
> > - return -ENOMEM;
> > - }
> > -
> > - handles = kvmalloc_array(job->in_fence_count, sizeof(u32), GFP_KERNEL);
> > + handles = kvmalloc_array(in_fence_count, sizeof(u32), GFP_KERNEL);
> > if (!handles) {
> > ret = -ENOMEM;
> > DRM_DEBUG("Failed to allocate incoming syncobj handles\n");
> > @@ -217,16 +203,23 @@ panfrost_copy_in_sync(struct drm_device *dev,
> >
> > if (copy_from_user(handles,
> >(void __user *)(uintptr_t)args->in_syncs,
> > - job->in_fence_count * sizeof(u32))) {
> > + in_fence_count * sizeof(u32))) {
> > ret = -EFAULT;
> > DRM_DEBUG("Failed to copy in syncobj handles\n");
> > goto fail;
> > }
> >
> > - for (i = 0; i < job->in_fence_count; i++) {
> > + for (i = 0; i < in_fence_count; i++) {
> > + struct dma_fence *fence;
> > +
> > ret = drm_syncobj_find_fence(file_priv, handles[i], 0, 0,
> > ->in_fences[i]);
> > - if (ret == -EINVAL)
> > +);
> > + if (ret)
> > + goto fail;
> > +
> > + ret = drm_gem_fence_array_add(>deps, fence);
> > +
> > + if (ret)
> > goto fail;
> > }
> >
> > @@ -264,6 +257,8 @@ static int panfrost_ioctl_submit(struct drm_device
> > *dev, void *data,
> >
> > kref_init(>refcount);
> >
> > + xa_init_flags(>deps, XA_FLAGS_ALLOC);
> > +
> > job->pfdev = pfdev;
> > job->jc = args->jc;
> > job->requirements = args->requirements;
> > diff --git a/drivers/gpu/drm/panfrost/panfrost_job.c
> > b/drivers/gpu/drm/panfrost/panfrost_job.c
> > index f5d39ee14ab5..707d912ff64a 100644
> > --- a/drivers/gpu/drm/panfrost/panfrost_job.c
> > +++ b/drivers/gpu/drm/panfrost/panfrost_job.c
> > @@ -196,14 +196,21 @@ static void panfrost_job_hw_submit(struct
> > panfrost_job *job, int js)
> > job_write(pfdev, JS_COMMAND_NEXT(js), JS_COMMAND_START);
> > }
> >
> > -static void
Re: [Intel-gfx] [PATCH 03/11] drm/panfrost: Use xarray and helpers for depedency tracking
On 21/05/2021 10:09, Daniel Vetter wrote:
> More consistency and prep work for the next patch.
>
> Aside: I wonder whether we shouldn't just move this entire xarray
> business into the scheduler so that not everyone has to reinvent the
> same wheels. Cc'ing some scheduler people for this too.
>
> Cc: "Christian König"
> Cc: Luben Tuikov
> Cc: Alex Deucher
> Cc: Lee Jones
> Cc: Steven Price
> Cc: Rob Herring
> Cc: Tomeu Vizoso
> Cc: Alyssa Rosenzweig
> Cc: Sumit Semwal
> Cc: linux-me...@vger.kernel.org
> Cc: linaro-mm-...@lists.linaro.org
> Signed-off-by: Daniel Vetter
Two comments below, but otherwise looks like a nice cleanup.
> ---
> drivers/gpu/drm/panfrost/panfrost_drv.c | 41 -
> drivers/gpu/drm/panfrost/panfrost_job.c | 61 ++---
> drivers/gpu/drm/panfrost/panfrost_job.h | 8 ++--
> 3 files changed, 46 insertions(+), 64 deletions(-)
>
> diff --git a/drivers/gpu/drm/panfrost/panfrost_drv.c
> b/drivers/gpu/drm/panfrost/panfrost_drv.c
> index ca07098a6141..7977b4752b5c 100644
> --- a/drivers/gpu/drm/panfrost/panfrost_drv.c
> +++ b/drivers/gpu/drm/panfrost/panfrost_drv.c
> @@ -137,12 +137,6 @@ panfrost_lookup_bos(struct drm_device *dev,
> if (!job->bo_count)
> return 0;
>
> - job->implicit_fences = kvmalloc_array(job->bo_count,
> - sizeof(struct dma_fence *),
> - GFP_KERNEL | __GFP_ZERO);
> - if (!job->implicit_fences)
> - return -ENOMEM;
> -
> ret = drm_gem_objects_lookup(file_priv,
>(void __user *)(uintptr_t)args->bo_handles,
>job->bo_count, >bos);
> @@ -173,7 +167,7 @@ panfrost_lookup_bos(struct drm_device *dev,
> }
>
> /**
> - * panfrost_copy_in_sync() - Sets up job->in_fences[] with the sync objects
> + * panfrost_copy_in_sync() - Sets up job->deps with the sync objects
> * referenced by the job.
> * @dev: DRM device
> * @file_priv: DRM file for this fd
> @@ -193,22 +187,14 @@ panfrost_copy_in_sync(struct drm_device *dev,
> {
> u32 *handles;
> int ret = 0;
> - int i;
> + int i, in_fence_count;
>
> - job->in_fence_count = args->in_sync_count;
> + in_fence_count = args->in_sync_count;
>
> - if (!job->in_fence_count)
> + if (!in_fence_count)
> return 0;
>
> - job->in_fences = kvmalloc_array(job->in_fence_count,
> - sizeof(struct dma_fence *),
> - GFP_KERNEL | __GFP_ZERO);
> - if (!job->in_fences) {
> - DRM_DEBUG("Failed to allocate job in fences\n");
> - return -ENOMEM;
> - }
> -
> - handles = kvmalloc_array(job->in_fence_count, sizeof(u32), GFP_KERNEL);
> + handles = kvmalloc_array(in_fence_count, sizeof(u32), GFP_KERNEL);
> if (!handles) {
> ret = -ENOMEM;
> DRM_DEBUG("Failed to allocate incoming syncobj handles\n");
> @@ -217,16 +203,23 @@ panfrost_copy_in_sync(struct drm_device *dev,
>
> if (copy_from_user(handles,
> (void __user *)(uintptr_t)args->in_syncs,
> -job->in_fence_count * sizeof(u32))) {
> +in_fence_count * sizeof(u32))) {
> ret = -EFAULT;
> DRM_DEBUG("Failed to copy in syncobj handles\n");
> goto fail;
> }
>
> - for (i = 0; i < job->in_fence_count; i++) {
> + for (i = 0; i < in_fence_count; i++) {
> + struct dma_fence *fence;
> +
> ret = drm_syncobj_find_fence(file_priv, handles[i], 0, 0,
> - >in_fences[i]);
> - if (ret == -EINVAL)
> + );
> + if (ret)
> + goto fail;
> +
> + ret = drm_gem_fence_array_add(>deps, fence);
> +
> + if (ret)
> goto fail;
> }
>
> @@ -264,6 +257,8 @@ static int panfrost_ioctl_submit(struct drm_device *dev,
> void *data,
>
> kref_init(>refcount);
>
> + xa_init_flags(>deps, XA_FLAGS_ALLOC);
> +
> job->pfdev = pfdev;
> job->jc = args->jc;
> job->requirements = args->requirements;
> diff --git a/drivers/gpu/drm/panfrost/panfrost_job.c
> b/drivers/gpu/drm/panfrost/panfrost_job.c
> index f5d39ee14ab5..707d912ff64a 100644
> --- a/drivers/gpu/drm/panfrost/panfrost_job.c
> +++ b/drivers/gpu/drm/panfrost/panfrost_job.c
> @@ -196,14 +196,21 @@ static void panfrost_job_hw_submit(struct panfrost_job
> *job, int js)
> job_write(pfdev, JS_COMMAND_NEXT(js), JS_COMMAND_START);
> }
>
> -static void panfrost_acquire_object_fences(struct drm_gem_object **bos,
> -int bo_count,
> -struct dma_fence **implicit_fences)
> +static int panfrost_acquire_object_fences(struct
[Intel-gfx] [PATCH 03/11] drm/panfrost: Use xarray and helpers for depedency tracking
More consistency and prep work for the next patch.
Aside: I wonder whether we shouldn't just move this entire xarray
business into the scheduler so that not everyone has to reinvent the
same wheels. Cc'ing some scheduler people for this too.
Cc: "Christian König"
Cc: Luben Tuikov
Cc: Alex Deucher
Cc: Lee Jones
Cc: Steven Price
Cc: Rob Herring
Cc: Tomeu Vizoso
Cc: Alyssa Rosenzweig
Cc: Sumit Semwal
Cc: linux-me...@vger.kernel.org
Cc: linaro-mm-...@lists.linaro.org
Signed-off-by: Daniel Vetter
---
drivers/gpu/drm/panfrost/panfrost_drv.c | 41 -
drivers/gpu/drm/panfrost/panfrost_job.c | 61 ++---
drivers/gpu/drm/panfrost/panfrost_job.h | 8 ++--
3 files changed, 46 insertions(+), 64 deletions(-)
diff --git a/drivers/gpu/drm/panfrost/panfrost_drv.c
b/drivers/gpu/drm/panfrost/panfrost_drv.c
index ca07098a6141..7977b4752b5c 100644
--- a/drivers/gpu/drm/panfrost/panfrost_drv.c
+++ b/drivers/gpu/drm/panfrost/panfrost_drv.c
@@ -137,12 +137,6 @@ panfrost_lookup_bos(struct drm_device *dev,
if (!job->bo_count)
return 0;
- job->implicit_fences = kvmalloc_array(job->bo_count,
- sizeof(struct dma_fence *),
- GFP_KERNEL | __GFP_ZERO);
- if (!job->implicit_fences)
- return -ENOMEM;
-
ret = drm_gem_objects_lookup(file_priv,
(void __user *)(uintptr_t)args->bo_handles,
job->bo_count, >bos);
@@ -173,7 +167,7 @@ panfrost_lookup_bos(struct drm_device *dev,
}
/**
- * panfrost_copy_in_sync() - Sets up job->in_fences[] with the sync objects
+ * panfrost_copy_in_sync() - Sets up job->deps with the sync objects
* referenced by the job.
* @dev: DRM device
* @file_priv: DRM file for this fd
@@ -193,22 +187,14 @@ panfrost_copy_in_sync(struct drm_device *dev,
{
u32 *handles;
int ret = 0;
- int i;
+ int i, in_fence_count;
- job->in_fence_count = args->in_sync_count;
+ in_fence_count = args->in_sync_count;
- if (!job->in_fence_count)
+ if (!in_fence_count)
return 0;
- job->in_fences = kvmalloc_array(job->in_fence_count,
- sizeof(struct dma_fence *),
- GFP_KERNEL | __GFP_ZERO);
- if (!job->in_fences) {
- DRM_DEBUG("Failed to allocate job in fences\n");
- return -ENOMEM;
- }
-
- handles = kvmalloc_array(job->in_fence_count, sizeof(u32), GFP_KERNEL);
+ handles = kvmalloc_array(in_fence_count, sizeof(u32), GFP_KERNEL);
if (!handles) {
ret = -ENOMEM;
DRM_DEBUG("Failed to allocate incoming syncobj handles\n");
@@ -217,16 +203,23 @@ panfrost_copy_in_sync(struct drm_device *dev,
if (copy_from_user(handles,
(void __user *)(uintptr_t)args->in_syncs,
- job->in_fence_count * sizeof(u32))) {
+ in_fence_count * sizeof(u32))) {
ret = -EFAULT;
DRM_DEBUG("Failed to copy in syncobj handles\n");
goto fail;
}
- for (i = 0; i < job->in_fence_count; i++) {
+ for (i = 0; i < in_fence_count; i++) {
+ struct dma_fence *fence;
+
ret = drm_syncobj_find_fence(file_priv, handles[i], 0, 0,
->in_fences[i]);
- if (ret == -EINVAL)
+);
+ if (ret)
+ goto fail;
+
+ ret = drm_gem_fence_array_add(>deps, fence);
+
+ if (ret)
goto fail;
}
@@ -264,6 +257,8 @@ static int panfrost_ioctl_submit(struct drm_device *dev,
void *data,
kref_init(>refcount);
+ xa_init_flags(>deps, XA_FLAGS_ALLOC);
+
job->pfdev = pfdev;
job->jc = args->jc;
job->requirements = args->requirements;
diff --git a/drivers/gpu/drm/panfrost/panfrost_job.c
b/drivers/gpu/drm/panfrost/panfrost_job.c
index f5d39ee14ab5..707d912ff64a 100644
--- a/drivers/gpu/drm/panfrost/panfrost_job.c
+++ b/drivers/gpu/drm/panfrost/panfrost_job.c
@@ -196,14 +196,21 @@ static void panfrost_job_hw_submit(struct panfrost_job
*job, int js)
job_write(pfdev, JS_COMMAND_NEXT(js), JS_COMMAND_START);
}
-static void panfrost_acquire_object_fences(struct drm_gem_object **bos,
- int bo_count,
- struct dma_fence **implicit_fences)
+static int panfrost_acquire_object_fences(struct drm_gem_object **bos,
+ int bo_count,
+ struct xarray *deps)
{
- int i;
+ int i, ret;
- for (i = 0; i < bo_count; i++)
- implicit_fences[i] =
