| Message ID | 20170308125328.28306-2-p.zabel@pengutronix.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Philipp, 2017-03-08 Philipp Zabel <p.zabel@pengutronix.de>: > The next patch will need the dma_fence to create the sync_file in > etnaviv_ioctl_gem_submit, in case an out_fence_fd is requested. > > Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de> > --- > drivers/gpu/drm/etnaviv/etnaviv_gem.h | 3 ++- > drivers/gpu/drm/etnaviv/etnaviv_gem_submit.c | 8 +++++++- > drivers/gpu/drm/etnaviv/etnaviv_gpu.c | 21 ++++++--------------- > drivers/gpu/drm/etnaviv/etnaviv_gpu.h | 1 + > 4 files changed, 16 insertions(+), 17 deletions(-) > > diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gem.h b/drivers/gpu/drm/etnaviv/etnaviv_gem.h > index 120410d67eb5b..c4a091e874269 100644 > --- a/drivers/gpu/drm/etnaviv/etnaviv_gem.h > +++ b/drivers/gpu/drm/etnaviv/etnaviv_gem.h > @@ -20,6 +20,7 @@ > #include <linux/reservation.h> > #include "etnaviv_drv.h" > > +struct dma_fence; Why not #include <linux/dma_fence.h> ? Other than that looks good to me: Reviewed-by: Gustavo Padovan <gustavo.padovan@collabora.com> Gustavo
On Wed, Mar 08, 2017 at 11:42:17AM -0300, Gustavo Padovan wrote: > Hi Philipp, > > 2017-03-08 Philipp Zabel <p.zabel@pengutronix.de>: > > > The next patch will need the dma_fence to create the sync_file in > > etnaviv_ioctl_gem_submit, in case an out_fence_fd is requested. > > > > Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de> > > --- > > drivers/gpu/drm/etnaviv/etnaviv_gem.h | 3 ++- > > drivers/gpu/drm/etnaviv/etnaviv_gem_submit.c | 8 +++++++- > > drivers/gpu/drm/etnaviv/etnaviv_gpu.c | 21 ++++++--------------- > > drivers/gpu/drm/etnaviv/etnaviv_gpu.h | 1 + > > 4 files changed, 16 insertions(+), 17 deletions(-) > > > > diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gem.h b/drivers/gpu/drm/etnaviv/etnaviv_gem.h > > index 120410d67eb5b..c4a091e874269 100644 > > --- a/drivers/gpu/drm/etnaviv/etnaviv_gem.h > > +++ b/drivers/gpu/drm/etnaviv/etnaviv_gem.h > > @@ -20,6 +20,7 @@ > > #include <linux/reservation.h> > > #include "etnaviv_drv.h" > > > > +struct dma_fence; > > Why not #include <linux/dma_fence.h> ? Adding needless includes when a struct prototype will do eventually creates headaches with circular dependencies and the like. Low probability in this case, but the principle is a good one to adhere to.
On Wed, 2017-03-08 at 11:42 -0300, Gustavo Padovan wrote: > Hi Philipp, > > 2017-03-08 Philipp Zabel <p.zabel@pengutronix.de>: > > > The next patch will need the dma_fence to create the sync_file in > > etnaviv_ioctl_gem_submit, in case an out_fence_fd is requested. > > > > Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de> > > --- > > drivers/gpu/drm/etnaviv/etnaviv_gem.h | 3 ++- > > drivers/gpu/drm/etnaviv/etnaviv_gem_submit.c | 8 +++++++- > > drivers/gpu/drm/etnaviv/etnaviv_gpu.c | 21 ++++++--------------- > > drivers/gpu/drm/etnaviv/etnaviv_gpu.h | 1 + > > 4 files changed, 16 insertions(+), 17 deletions(-) > > > > diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gem.h b/drivers/gpu/drm/etnaviv/etnaviv_gem.h > > index 120410d67eb5b..c4a091e874269 100644 > > --- a/drivers/gpu/drm/etnaviv/etnaviv_gem.h > > +++ b/drivers/gpu/drm/etnaviv/etnaviv_gem.h > > @@ -20,6 +20,7 @@ > > #include <linux/reservation.h> > > #include "etnaviv_drv.h" > > > > +struct dma_fence; > > Why not #include <linux/dma_fence.h> ? I don't see the need to include the header as long as the memory layout of struct dma_fence doesn't have to be known. Here we just need to pass pointers to the structure type as function arguments. > Other than that looks good to me: > > Reviewed-by: Gustavo Padovan <gustavo.padovan@collabora.com> May I keep the review tag without adding the #include? regards Philipp
2017-03-13 Philipp Zabel <p.zabel@pengutronix.de>: > On Wed, 2017-03-08 at 11:42 -0300, Gustavo Padovan wrote: > > Hi Philipp, > > > > 2017-03-08 Philipp Zabel <p.zabel@pengutronix.de>: > > > > > The next patch will need the dma_fence to create the sync_file in > > > etnaviv_ioctl_gem_submit, in case an out_fence_fd is requested. > > > > > > Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de> > > > --- > > > drivers/gpu/drm/etnaviv/etnaviv_gem.h | 3 ++- > > > drivers/gpu/drm/etnaviv/etnaviv_gem_submit.c | 8 +++++++- > > > drivers/gpu/drm/etnaviv/etnaviv_gpu.c | 21 ++++++--------------- > > > drivers/gpu/drm/etnaviv/etnaviv_gpu.h | 1 + > > > 4 files changed, 16 insertions(+), 17 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gem.h b/drivers/gpu/drm/etnaviv/etnaviv_gem.h > > > index 120410d67eb5b..c4a091e874269 100644 > > > --- a/drivers/gpu/drm/etnaviv/etnaviv_gem.h > > > +++ b/drivers/gpu/drm/etnaviv/etnaviv_gem.h > > > @@ -20,6 +20,7 @@ > > > #include <linux/reservation.h> > > > #include "etnaviv_drv.h" > > > > > > +struct dma_fence; > > > > Why not #include <linux/dma_fence.h> ? > > I don't see the need to include the header as long as the memory layout > of struct dma_fence doesn't have to be known. Here we just need to pass > pointers to the structure type as function arguments. > > > Other than that looks good to me: > > > > Reviewed-by: Gustavo Padovan <gustavo.padovan@collabora.com> > > May I keep the review tag without adding the #include? Sure. Please keep it. Gustavo
diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gem.h b/drivers/gpu/drm/etnaviv/etnaviv_gem.h index 120410d67eb5b..c4a091e874269 100644 --- a/drivers/gpu/drm/etnaviv/etnaviv_gem.h +++ b/drivers/gpu/drm/etnaviv/etnaviv_gem.h @@ -20,6 +20,7 @@ #include <linux/reservation.h> #include "etnaviv_drv.h" +struct dma_fence; struct etnaviv_gem_ops; struct etnaviv_gem_object; @@ -104,7 +105,7 @@ struct etnaviv_gem_submit { struct drm_device *dev; struct etnaviv_gpu *gpu; struct ww_acquire_ctx ticket; - u32 fence; + struct dma_fence *fence; unsigned int nr_bos; struct etnaviv_gem_submit_bo bos[0]; u32 flags; diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gem_submit.c b/drivers/gpu/drm/etnaviv/etnaviv_gem_submit.c index e67d83eac22dc..022fcc7d57f48 100644 --- a/drivers/gpu/drm/etnaviv/etnaviv_gem_submit.c +++ b/drivers/gpu/drm/etnaviv/etnaviv_gem_submit.c @@ -431,11 +431,17 @@ int etnaviv_ioctl_gem_submit(struct drm_device *dev, void *data, memcpy(cmdbuf->vaddr, stream, args->stream_size); cmdbuf->user_size = ALIGN(args->stream_size, 8); + submit->fence = etnaviv_gpu_fence_alloc(gpu); + if (!submit->fence) { + ret = -ENOMEM; + goto out; + } + ret = etnaviv_gpu_submit(gpu, submit, cmdbuf); if (ret == 0) cmdbuf = NULL; - args->fence = submit->fence; + args->fence = submit->fence->seqno; out: submit_unpin_objects(submit); diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gpu.c b/drivers/gpu/drm/etnaviv/etnaviv_gpu.c index 51d52c72aef17..a439700cc577f 100644 --- a/drivers/gpu/drm/etnaviv/etnaviv_gpu.c +++ b/drivers/gpu/drm/etnaviv/etnaviv_gpu.c @@ -1047,7 +1047,7 @@ static const struct dma_fence_ops etnaviv_fence_ops = { .release = etnaviv_fence_release, }; -static struct dma_fence *etnaviv_gpu_fence_alloc(struct etnaviv_gpu *gpu) +struct dma_fence *etnaviv_gpu_fence_alloc(struct etnaviv_gpu *gpu) { struct etnaviv_fence *f; @@ -1290,7 +1290,6 @@ void etnaviv_gpu_pm_put(struct etnaviv_gpu *gpu) int etnaviv_gpu_submit(struct etnaviv_gpu *gpu, struct etnaviv_gem_submit *submit, struct etnaviv_cmdbuf *cmdbuf) { - struct dma_fence *fence; unsigned int event, i; int ret; @@ -1314,18 +1313,10 @@ int etnaviv_gpu_submit(struct etnaviv_gpu *gpu, goto out_pm_put; } - fence = etnaviv_gpu_fence_alloc(gpu); - if (!fence) { - event_free(gpu, event); - ret = -ENOMEM; - goto out_pm_put; - } - mutex_lock(&gpu->lock); - gpu->event[event].fence = fence; - submit->fence = fence->seqno; - gpu->active_fence = submit->fence; + gpu->event[event].fence = submit->fence; + gpu->active_fence = submit->fence->seqno; if (gpu->lastctx != cmdbuf->ctx) { gpu->mmu->need_flush = true; @@ -1335,7 +1326,7 @@ int etnaviv_gpu_submit(struct etnaviv_gpu *gpu, etnaviv_buffer_queue(gpu, event, cmdbuf); - cmdbuf->fence = fence; + cmdbuf->fence = submit->fence; list_add_tail(&cmdbuf->node, &gpu->active_cmd_list); /* We're committed to adding this command buffer, hold a PM reference */ @@ -1351,10 +1342,10 @@ int etnaviv_gpu_submit(struct etnaviv_gpu *gpu, if (submit->bos[i].flags & ETNA_SUBMIT_BO_WRITE) reservation_object_add_excl_fence(etnaviv_obj->resv, - fence); + submit->fence); else reservation_object_add_shared_fence(etnaviv_obj->resv, - fence); + submit->fence); } cmdbuf->nr_bos = submit->nr_bos; hangcheck_timer_reset(gpu); diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gpu.h b/drivers/gpu/drm/etnaviv/etnaviv_gpu.h index dc27c7a039060..20f7191018daf 100644 --- a/drivers/gpu/drm/etnaviv/etnaviv_gpu.h +++ b/drivers/gpu/drm/etnaviv/etnaviv_gpu.h @@ -180,6 +180,7 @@ int etnaviv_gpu_init(struct etnaviv_gpu *gpu); int etnaviv_gpu_debugfs(struct etnaviv_gpu *gpu, struct seq_file *m); #endif +struct dma_fence *etnaviv_gpu_fence_alloc(struct etnaviv_gpu *gpu); int etnaviv_gpu_fence_sync_obj(struct etnaviv_gem_object *etnaviv_obj, unsigned int context, bool exclusive, bool implicit);
The next patch will need the dma_fence to create the sync_file in etnaviv_ioctl_gem_submit, in case an out_fence_fd is requested. Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de> --- drivers/gpu/drm/etnaviv/etnaviv_gem.h | 3 ++- drivers/gpu/drm/etnaviv/etnaviv_gem_submit.c | 8 +++++++- drivers/gpu/drm/etnaviv/etnaviv_gpu.c | 21 ++++++--------------- drivers/gpu/drm/etnaviv/etnaviv_gpu.h | 1 + 4 files changed, 16 insertions(+), 17 deletions(-)