Re: [PATCH 5/5] amdgpu: use drm sync objects for shared semaphores (v5)

[Christian König <deathsimple@xxxxxxxxxxx>]

Am 24.05.2017 um 09:25 schrieb zhoucm1:
> On 2017年05月24日 15:06, Dave Airlie wrote:
> > From: Dave Airlie <airlied@xxxxxxxxxx>
> >
> > This creates a new command submission chunk for amdgpu
> > to add in and out sync objects around the submission.
> >
> > Sync objects are managed via the drm syncobj ioctls.
> >
> > The command submission interface is enhanced with two new
> > chunks, one for syncobj pre submission dependencies,
> > and one for post submission sync obj signalling,
> > and just takes a list of handles for each.
> >
> > This is based on work originally done by David Zhou at AMD,
> > with input from Christian Konig on what things should look like.
> >
> > In theory VkFences could be backed with sync objects and
> > just get passed into the cs as syncobj handles as well.
> >
> > NOTE: this interface addition needs a version bump to expose
> > it to userspace.
> >
> > v1.1: keep file reference on import.
> > v2: move to using syncobjs
> > v2.1: change some APIs to just use p pointer.
> > v3: make more robust against CS failures, we now add the
> > wait sems but only remove them once the CS job has been
> > submitted.
> > v4: rewrite names of API and base on new syncobj code.
> > v5: move post deps earlier, rename some apis
> >
> > Signed-off-by: Dave Airlie <airlied@xxxxxxxxxx>
> > ---
> >   drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c  | 87 
> > ++++++++++++++++++++++++++++++++-
> >   drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c |  2 +-
> >   include/uapi/drm/amdgpu_drm.h           |  6 +++
> >   3 files changed, 93 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c 
> > b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
> > index 30225d7..3cbd547 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
> > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
> > @@ -27,6 +27,7 @@
> >   #include <linux/pagemap.h>
> >   #include <drm/drmP.h>
> >   #include <drm/amdgpu_drm.h>
> > +#include <drm/drm_syncobj.h>
> >   #include "amdgpu.h"
> >   #include "amdgpu_trace.h"
> >   @@ -226,6 +227,8 @@ int amdgpu_cs_parser_init(struct 
> > amdgpu_cs_parser *p, void *data)
> >               break;
> >             case AMDGPU_CHUNK_ID_DEPENDENCIES:
> > +        case AMDGPU_CHUNK_ID_SYNCOBJ_IN:
> > +        case AMDGPU_CHUNK_ID_SYNCOBJ_OUT:
> >               break;
> >             default:
> > @@ -1040,6 +1043,40 @@ static int amdgpu_cs_process_fence_dep(struct 
> > amdgpu_cs_parser *p,
> >       return 0;
> >   }
> >   +static int amdgpu_syncobj_lookup_and_add_to_sync(struct 
> > amdgpu_cs_parser *p,
> > +                         uint32_t handle)
> > +{
> > +    int r;
> > +    struct dma_fence *fence;
> > +    r = drm_syncobj_fence_get(p->filp, handle, &fence);
> > +    if (r)
> > +        return r;
> > +
> > +    r = amdgpu_sync_fence(p->adev, &p->job->sync, fence);
> We just fixed a sched fence issue by adding new job->dep_sync member, 
> so here should use &p->job->dep_sync instead of &p->job->sync.

To be precise job->sync is now for the implicit fences from the BOs 
(e.g. buffers moves and such) and job->dep_sync is for all explicit 
fences from dependencies/semaphores.

The handling is now slightly different to fix a bug with two consecutive 
jobs scheduled to the same ring.

But apart from that the patch now looks good to me, so with that fixed 
it is Reviewed-by: Christian König <christian.koenig@xxxxxxx>.

Regards,
Christian.

> Regards,
> David Zhou
> > +    dma_fence_put(fence);
> > +
> > +    return r;
> > +}
> > +
> > +static int amdgpu_cs_process_syncobj_in_dep(struct amdgpu_cs_parser *p,
> > +                        struct amdgpu_cs_chunk *chunk)
> > +{
> > +    unsigned num_deps;
> > +    int i, r;
> > +    struct drm_amdgpu_cs_chunk_sem *deps;
> > +
> > +    deps = (struct drm_amdgpu_cs_chunk_sem *)chunk->kdata;
> > +    num_deps = chunk->length_dw * 4 /
> > +        sizeof(struct drm_amdgpu_cs_chunk_sem);
> > +
> > +    for (i = 0; i < num_deps; ++i) {
> > +        r = amdgpu_syncobj_lookup_and_add_to_sync(p, deps[i].handle);
> > +        if (r)
> > +            return r;
> > +    }
> > +    return 0;
> > +}
> > +
> >   static int amdgpu_cs_dependencies(struct amdgpu_device *adev,
> >                     struct amdgpu_cs_parser *p)
> >   {
> > @@ -1054,12 +1091,54 @@ static int amdgpu_cs_dependencies(struct 
> > amdgpu_device *adev,
> >               r = amdgpu_cs_process_fence_dep(p, chunk);
> >               if (r)
> >                   return r;
> > +        } else if (chunk->chunk_id == AMDGPU_CHUNK_ID_SYNCOBJ_IN) {
> > +            r = amdgpu_cs_process_syncobj_in_dep(p, chunk);
> > +            if (r)
> > +                return r;
> >           }
> >       }
> >         return 0;
> >   }
> >   +static int amdgpu_cs_process_syncobj_out_dep(struct 
> > amdgpu_cs_parser *p,
> > +                         struct amdgpu_cs_chunk *chunk)
> > +{
> > +    unsigned num_deps;
> > +    int i, r;
> > +    struct drm_amdgpu_cs_chunk_sem *deps;
> > +
> > +    deps = (struct drm_amdgpu_cs_chunk_sem *)chunk->kdata;
> > +    num_deps = chunk->length_dw * 4 /
> > +        sizeof(struct drm_amdgpu_cs_chunk_sem);
> > +
> > +    for (i = 0; i < num_deps; ++i) {
> > +        r = drm_syncobj_replace_fence(p->filp, deps[i].handle,
> > +                          p->fence);
> > +        if (r)
> > +            return r;
> > +    }
> > +    return 0;
> > +}
> > +
> > +static int amdgpu_cs_post_dependencies(struct amdgpu_cs_parser *p)
> > +{
> > +    int i, r;
> > +
> > +    for (i = 0; i < p->nchunks; ++i) {
> > +        struct amdgpu_cs_chunk *chunk;
> > +
> > +        chunk = &p->chunks[i];
> > +
> > +        if (chunk->chunk_id == AMDGPU_CHUNK_ID_SYNCOBJ_OUT) {
> > +            r = amdgpu_cs_process_syncobj_out_dep(p, chunk);
> > +            if (r)
> > +                return r;
> > +        }
> > +    }
> > +    return 0;
> > +}
> > +
> >   static int amdgpu_cs_submit(struct amdgpu_cs_parser *p,
> >                   union drm_amdgpu_cs *cs)
> >   {
> > @@ -1080,6 +1159,13 @@ static int amdgpu_cs_submit(struct 
> > amdgpu_cs_parser *p,
> >       job->owner = p->filp;
> >       job->fence_ctx = entity->fence_context;
> >       p->fence = dma_fence_get(&job->base.s_fence->finished);
> > +
> > +    r = amdgpu_cs_post_dependencies(p);
> > +    if (r) {
> > +        amdgpu_job_free(job);
> > +        return r;
> > +    }
> > +
> >       cs->out.handle = amdgpu_ctx_add_fence(p->ctx, ring, p->fence);
> >       job->uf_sequence = cs->out.handle;
> >       amdgpu_job_free_resources(job);
> > @@ -1087,7 +1173,6 @@ static int amdgpu_cs_submit(struct 
> > amdgpu_cs_parser *p,
> >         trace_amdgpu_cs_ioctl(job);
> >       amd_sched_entity_push_job(&job->base);
> > -
> >       return 0;
> >   }
> >   diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c 
> > b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> > index f2d705e..5329e7f 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> > @@ -719,7 +719,7 @@ static struct drm_driver kms_driver = {
> >       .driver_features =
> >           DRIVER_USE_AGP |
> >           DRIVER_HAVE_IRQ | DRIVER_IRQ_SHARED | DRIVER_GEM |
> > -        DRIVER_PRIME | DRIVER_RENDER | DRIVER_MODESET,
> > +        DRIVER_PRIME | DRIVER_RENDER | DRIVER_MODESET | DRIVER_SYNCOBJ,
> >       .load = amdgpu_driver_load_kms,
> >       .open = amdgpu_driver_open_kms,
> >       .postclose = amdgpu_driver_postclose_kms,
> > diff --git a/include/uapi/drm/amdgpu_drm.h 
> > b/include/uapi/drm/amdgpu_drm.h
> > index 6c249e5..cc702ca 100644
> > --- a/include/uapi/drm/amdgpu_drm.h
> > +++ b/include/uapi/drm/amdgpu_drm.h
> > @@ -416,6 +416,8 @@ struct drm_amdgpu_gem_va {
> >   #define AMDGPU_CHUNK_ID_IB        0x01
> >   #define AMDGPU_CHUNK_ID_FENCE        0x02
> >   #define AMDGPU_CHUNK_ID_DEPENDENCIES    0x03
> > +#define AMDGPU_CHUNK_ID_SYNCOBJ_IN      0x04
> > +#define AMDGPU_CHUNK_ID_SYNCOBJ_OUT     0x05
> >     struct drm_amdgpu_cs_chunk {
> >       __u32        chunk_id;
> > @@ -483,6 +485,10 @@ struct drm_amdgpu_cs_chunk_fence {
> >       __u32 offset;
> >   };
> >   +struct drm_amdgpu_cs_chunk_sem {
> > +    __u32 handle;
> > +};
> > +
> >   struct drm_amdgpu_cs_chunk_data {
> >       union {
> >           struct drm_amdgpu_cs_chunk_ib        ib_data;
>
> _______________________________________________
> amd-gfx mailing list
> amd-gfx@xxxxxxxxxxxxxxxxxxxxx
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx


_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel