From 27a64f2e239b69a5c6377b121dd2306d258e0975 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Konsta=20H=C3=B6ltt=C3=A4?= Date: Tue, 4 Aug 2020 16:17:52 +0300 Subject: [PATCH] gpu: nvgpu: enforce priv usage of fence MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Add a "priv" fence struct type and use that in the fence type to emphasize that the inner data is not meant to be seen. The fence unit needs to have an outside-visible fence type so that fences can be allocated directly as a struct field in job metadata for performance and simplicity, so hiding the type entirely wouldn't work. A couple of places need to touch the priv data directly in channel code. Those can be thought to be technically fence unit's code scattered outside the fence files, but they mean that the architecture is not perfect yet. Jira NVGPU-5773 Change-Id: Ifa3c95757ae31eef0e32f2605293e23e210b065f Signed-off-by: Konsta Hölttä Reviewed-on: https://git-master.nvidia.com/r/c/linux-nvgpu/+/2395071 Tested-by: mobile promotions Reviewed-by: mobile promotions --- drivers/gpu/nvgpu/common/fence/fence.c | 45 ++++++++++++------- drivers/gpu/nvgpu/common/fence/fence_sema.c | 24 ++++++---- drivers/gpu/nvgpu/common/fence/fence_syncpt.c | 26 ++++++----- drivers/gpu/nvgpu/common/fifo/submit.c | 8 +++- drivers/gpu/nvgpu/include/nvgpu/fence.h | 12 ++++- drivers/gpu/nvgpu/os/linux/linux-channel.c | 6 ++- 6 files changed, 83 insertions(+), 38 deletions(-) diff --git a/drivers/gpu/nvgpu/common/fence/fence.c b/drivers/gpu/nvgpu/common/fence/fence.c index 608f29b22..d5edf76d5 100644 --- a/drivers/gpu/nvgpu/common/fence/fence.c +++ b/drivers/gpu/nvgpu/common/fence/fence.c @@ -30,28 +30,33 @@ static struct nvgpu_fence_type *nvgpu_fence_from_ref(struct nvgpu_ref *ref) { return (struct nvgpu_fence_type *)((uintptr_t)ref - - offsetof(struct nvgpu_fence_type, ref)); + offsetof(struct nvgpu_fence_type, priv.ref)); } static void nvgpu_fence_free(struct nvgpu_ref *ref) { struct nvgpu_fence_type *f = nvgpu_fence_from_ref(ref); + struct nvgpu_fence_type_priv *pf = &f->priv; - if (nvgpu_os_fence_is_initialized(&f->os_fence)) { - f->os_fence.ops->drop_ref(&f->os_fence); + if (nvgpu_os_fence_is_initialized(&pf->os_fence)) { + pf->os_fence.ops->drop_ref(&pf->os_fence); } - f->ops->free(f); + pf->ops->free(f); } void nvgpu_fence_put(struct nvgpu_fence_type *f) { - nvgpu_ref_put(&f->ref, nvgpu_fence_free); + struct nvgpu_fence_type_priv *pf = &f->priv; + + nvgpu_ref_put(&pf->ref, nvgpu_fence_free); } struct nvgpu_fence_type *nvgpu_fence_get(struct nvgpu_fence_type *f) { - nvgpu_ref_get(&f->ref); + struct nvgpu_fence_type_priv *pf = &f->priv; + + nvgpu_ref_get(&pf->ref); return f; } @@ -61,12 +66,14 @@ struct nvgpu_fence_type *nvgpu_fence_get(struct nvgpu_fence_type *f) */ struct nvgpu_user_fence nvgpu_fence_extract_user(struct nvgpu_fence_type *f) { + struct nvgpu_fence_type_priv *pf = &f->priv; + struct nvgpu_user_fence uf = (struct nvgpu_user_fence) { #ifdef CONFIG_TEGRA_GK20A_NVHOST - .syncpt_id = f->syncpt_id, - .syncpt_value = f->syncpt_value, + .syncpt_id = pf->syncpt_id, + .syncpt_value = pf->syncpt_value, #endif - .os_fence = f->os_fence, + .os_fence = pf->os_fence, }; /* @@ -76,8 +83,8 @@ struct nvgpu_user_fence nvgpu_fence_extract_user(struct nvgpu_fence_type *f) * submission ioctl finishes, or if it's stored for cde job state * tracking. */ - if (nvgpu_os_fence_is_initialized(&f->os_fence)) { - f->os_fence.ops->dup(&f->os_fence); + if (nvgpu_os_fence_is_initialized(&pf->os_fence)) { + pf->os_fence.ops->dup(&pf->os_fence); } return uf; @@ -86,22 +93,28 @@ struct nvgpu_user_fence nvgpu_fence_extract_user(struct nvgpu_fence_type *f) int nvgpu_fence_wait(struct gk20a *g, struct nvgpu_fence_type *f, u32 timeout) { + struct nvgpu_fence_type_priv *pf = &f->priv; + if (!nvgpu_platform_is_silicon(g)) { timeout = U32_MAX; } - return f->ops->wait(f, timeout); + return pf->ops->wait(f, timeout); } bool nvgpu_fence_is_expired(struct nvgpu_fence_type *f) { - return f->ops->is_expired(f); + struct nvgpu_fence_type_priv *pf = &f->priv; + + return pf->ops->is_expired(f); } void nvgpu_fence_init(struct nvgpu_fence_type *f, const struct nvgpu_fence_ops *ops, struct nvgpu_os_fence os_fence) { - nvgpu_ref_init(&f->ref); - f->ops = ops; - f->os_fence = os_fence; + struct nvgpu_fence_type_priv *pf = &f->priv; + + nvgpu_ref_init(&pf->ref); + pf->ops = ops; + pf->os_fence = os_fence; } diff --git a/drivers/gpu/nvgpu/common/fence/fence_sema.c b/drivers/gpu/nvgpu/common/fence/fence_sema.c index 521076a04..30f587919 100644 --- a/drivers/gpu/nvgpu/common/fence/fence_sema.c +++ b/drivers/gpu/nvgpu/common/fence/fence_sema.c @@ -28,25 +28,31 @@ static int nvgpu_fence_semaphore_wait(struct nvgpu_fence_type *f, u32 timeout) { - if (!nvgpu_semaphore_is_acquired(f->semaphore)) { + struct nvgpu_fence_type_priv *pf = &f->priv; + + if (!nvgpu_semaphore_is_acquired(pf->semaphore)) { return 0; } return NVGPU_COND_WAIT_INTERRUPTIBLE( - f->semaphore_wq, - !nvgpu_semaphore_is_acquired(f->semaphore), + pf->semaphore_wq, + !nvgpu_semaphore_is_acquired(pf->semaphore), timeout); } static bool nvgpu_fence_semaphore_is_expired(struct nvgpu_fence_type *f) { - return !nvgpu_semaphore_is_acquired(f->semaphore); + struct nvgpu_fence_type_priv *pf = &f->priv; + + return !nvgpu_semaphore_is_acquired(pf->semaphore); } static void nvgpu_fence_semaphore_free(struct nvgpu_fence_type *f) { - if (f->semaphore != NULL) { - nvgpu_semaphore_put(f->semaphore); + struct nvgpu_fence_type_priv *pf = &f->priv; + + if (pf->semaphore != NULL) { + nvgpu_semaphore_put(pf->semaphore); } } @@ -63,8 +69,10 @@ void nvgpu_fence_from_semaphore( struct nvgpu_cond *semaphore_wq, struct nvgpu_os_fence os_fence) { + struct nvgpu_fence_type_priv *pf = &f->priv; + nvgpu_fence_init(f, &nvgpu_fence_semaphore_ops, os_fence); - f->semaphore = semaphore; - f->semaphore_wq = semaphore_wq; + pf->semaphore = semaphore; + pf->semaphore_wq = semaphore_wq; } diff --git a/drivers/gpu/nvgpu/common/fence/fence_syncpt.c b/drivers/gpu/nvgpu/common/fence/fence_syncpt.c index 015cfc7f1..a3da9b310 100644 --- a/drivers/gpu/nvgpu/common/fence/fence_syncpt.c +++ b/drivers/gpu/nvgpu/common/fence/fence_syncpt.c @@ -27,27 +27,31 @@ static int nvgpu_fence_syncpt_wait(struct nvgpu_fence_type *f, u32 timeout) { + struct nvgpu_fence_type_priv *pf = &f->priv; + return nvgpu_nvhost_syncpt_wait_timeout_ext( - f->nvhost_dev, f->syncpt_id, f->syncpt_value, + pf->nvhost_dev, pf->syncpt_id, pf->syncpt_value, timeout, NVGPU_NVHOST_DEFAULT_WAITER); } static bool nvgpu_fence_syncpt_is_expired(struct nvgpu_fence_type *f) { + struct nvgpu_fence_type_priv *pf = &f->priv; + /* * In cases we don't register a notifier, we can't expect the * syncpt value to be updated. For this case, we force a read * of the value from HW, and then check for expiration. */ - if (!nvgpu_nvhost_syncpt_is_expired_ext(f->nvhost_dev, f->syncpt_id, - f->syncpt_value)) { + if (!nvgpu_nvhost_syncpt_is_expired_ext(pf->nvhost_dev, pf->syncpt_id, + pf->syncpt_value)) { u32 val; - if (!nvgpu_nvhost_syncpt_read_ext_check(f->nvhost_dev, - f->syncpt_id, &val)) { + if (!nvgpu_nvhost_syncpt_read_ext_check(pf->nvhost_dev, + pf->syncpt_id, &val)) { return nvgpu_nvhost_syncpt_is_expired_ext( - f->nvhost_dev, - f->syncpt_id, f->syncpt_value); + pf->nvhost_dev, + pf->syncpt_id, pf->syncpt_value); } } @@ -70,9 +74,11 @@ void nvgpu_fence_from_syncpt( struct nvgpu_nvhost_dev *nvhost_dev, u32 id, u32 value, struct nvgpu_os_fence os_fence) { + struct nvgpu_fence_type_priv *pf = &f->priv; + nvgpu_fence_init(f, &nvgpu_fence_syncpt_ops, os_fence); - f->nvhost_dev = nvhost_dev; - f->syncpt_id = id; - f->syncpt_value = value; + pf->nvhost_dev = nvhost_dev; + pf->syncpt_id = id; + pf->syncpt_value = value; } diff --git a/drivers/gpu/nvgpu/common/fifo/submit.c b/drivers/gpu/nvgpu/common/fifo/submit.c index a1d08da8d..5b057c26f 100644 --- a/drivers/gpu/nvgpu/common/fifo/submit.c +++ b/drivers/gpu/nvgpu/common/fifo/submit.c @@ -773,11 +773,15 @@ static int nvgpu_submit_channel_gpfifo(struct nvgpu_channel *c, #ifdef CONFIG_NVGPU_TRACE if (fence_out != NULL && *fence_out != NULL) { + /* + * This is not a good example on how to use the fence type. + * Don't touch the priv data. The debug trace is special. + */ #ifdef CONFIG_TEGRA_GK20A_NVHOST trace_gk20a_channel_submitted_gpfifo(g->name, c->chid, num_entries, flags, - (*fence_out)->syncpt_id, - (*fence_out)->syncpt_value); + (*fence_out)->priv.syncpt_id, + (*fence_out)->priv.syncpt_value); #else trace_gk20a_channel_submitted_gpfifo(g->name, c->chid, num_entries, flags, diff --git a/drivers/gpu/nvgpu/include/nvgpu/fence.h b/drivers/gpu/nvgpu/include/nvgpu/fence.h index d85eb266f..440dd4549 100644 --- a/drivers/gpu/nvgpu/include/nvgpu/fence.h +++ b/drivers/gpu/nvgpu/include/nvgpu/fence.h @@ -36,7 +36,7 @@ struct nvgpu_os_fence; struct nvgpu_user_fence; struct nvgpu_fence_ops; -struct nvgpu_fence_type { +struct nvgpu_fence_type_priv { /* Valid for all fence types: */ struct nvgpu_ref ref; const struct nvgpu_fence_ops *ops; @@ -57,6 +57,16 @@ struct nvgpu_fence_type { #endif }; +struct nvgpu_fence_type { + /* + * struct nvgpu_fence_type needs to be allocated outside fence code for + * performance. It's technically possible to peek inside this priv + * data, but it's called priv for a reason. Don't touch it; use the + * public API (nvgpu_fence_*()). + */ + struct nvgpu_fence_type_priv priv; +}; + /* Fence operations */ void nvgpu_fence_put(struct nvgpu_fence_type *f); struct nvgpu_fence_type *nvgpu_fence_get(struct nvgpu_fence_type *f); diff --git a/drivers/gpu/nvgpu/os/linux/linux-channel.c b/drivers/gpu/nvgpu/os/linux/linux-channel.c index 4a39077a8..534b9dbce 100644 --- a/drivers/gpu/nvgpu/os/linux/linux-channel.c +++ b/drivers/gpu/nvgpu/os/linux/linux-channel.c @@ -358,7 +358,11 @@ static void nvgpu_channel_signal_os_fence_framework(struct nvgpu_channel *ch, #if defined(CONFIG_NVGPU_SYNCFD_ANDROID) gk20a_sync_timeline_signal(fence_framework->timeline); #elif defined(CONFIG_NVGPU_SYNCFD_STABLE) - f = nvgpu_get_dma_fence(&fence->os_fence); + /* + * This is not a good example on how to use the fence type. Don't touch + * the priv data. This is os-specific code for the fence unit. + */ + f = nvgpu_get_dma_fence(&fence->priv.os_fence); /* * Sometimes the post fence of a job isn't a file. It can be a raw * semaphore for kernel-internal tracking, or a raw syncpoint for