| Message ID | 20220218113952.3077606-10-mperttunen@nvidia.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Host1x context isolation support | expand |
18.02.2022 14:39, Mikko Perttunen пишет: > + if (context->memory_context && context->client->ops->get_streamid_offset) { ^^^ > + int offset = context->client->ops->get_streamid_offset(context->client); > + > + if (offset >= 0) { > + job->context = context->memory_context; > + job->engine_streamid_offset = offset; > + host1x_context_get(job->context); > + } You should bump refcount unconditionally or you'll get refcnt underflow on put, when offset < 0. > + } > + > /* > * job_data is now part of job reference counting, so don't release > * it from here. > diff --git a/drivers/gpu/drm/tegra/uapi.c b/drivers/gpu/drm/tegra/uapi.c > index 9ab9179d2026..be33da54d12c 100644 > --- a/drivers/gpu/drm/tegra/uapi.c > +++ b/drivers/gpu/drm/tegra/uapi.c > @@ -33,6 +33,9 @@ static void tegra_drm_channel_context_close(struct tegra_drm_context *context) > struct tegra_drm_mapping *mapping; > unsigned long id; > > + if (context->memory_context) > + host1x_context_put(context->memory_context); The "if (context->memory_context && context->client->ops->get_streamid_offset)" above doesn't match the "if (context->memory_context)". You'll get refcount underflow.
On 2/19/22 20:35, Dmitry Osipenko wrote: > 18.02.2022 14:39, Mikko Perttunen пишет: >> + if (context->memory_context && context->client->ops->get_streamid_offset) { > ^^^ >> + int offset = context->client->ops->get_streamid_offset(context->client); >> + >> + if (offset >= 0) { >> + job->context = context->memory_context; >> + job->engine_streamid_offset = offset; >> + host1x_context_get(job->context); >> + } > > You should bump refcount unconditionally or you'll get refcnt underflow > on put, when offset < 0. This refcount is intended to be dropped from 'release_job', where it's dropped if job->context is set, which it is from this path. > >> + } >> + >> /* >> * job_data is now part of job reference counting, so don't release >> * it from here. >> diff --git a/drivers/gpu/drm/tegra/uapi.c b/drivers/gpu/drm/tegra/uapi.c >> index 9ab9179d2026..be33da54d12c 100644 >> --- a/drivers/gpu/drm/tegra/uapi.c >> +++ b/drivers/gpu/drm/tegra/uapi.c >> @@ -33,6 +33,9 @@ static void tegra_drm_channel_context_close(struct tegra_drm_context *context) >> struct tegra_drm_mapping *mapping; >> unsigned long id; >> >> + if (context->memory_context) >> + host1x_context_put(context->memory_context); > > The "if (context->memory_context && > context->client->ops->get_streamid_offset)" above doesn't match the "if > (context->memory_context)". You'll get refcount underflow. And this drop is for the refcount implicitly added when allocating the memory context through host1x_context_alloc; so these two places should be independent. Please elaborate if I missed something. Thanks, Mikko
21.02.2022 15:06, Mikko Perttunen пишет: > On 2/19/22 20:35, Dmitry Osipenko wrote: >> 18.02.2022 14:39, Mikko Perttunen пишет: >>> + if (context->memory_context && >>> context->client->ops->get_streamid_offset) { >> ^^^ >>> + int offset = >>> context->client->ops->get_streamid_offset(context->client); >>> + >>> + if (offset >= 0) { >>> + job->context = context->memory_context; >>> + job->engine_streamid_offset = offset; >>> + host1x_context_get(job->context); >>> + } >> >> You should bump refcount unconditionally or you'll get refcnt underflow >> on put, when offset < 0. > > This refcount is intended to be dropped from 'release_job', where it's > dropped if job->context is set, which it is from this path. > >> >>> + } >>> + >>> /* >>> * job_data is now part of job reference counting, so don't >>> release >>> * it from here. >>> diff --git a/drivers/gpu/drm/tegra/uapi.c b/drivers/gpu/drm/tegra/uapi.c >>> index 9ab9179d2026..be33da54d12c 100644 >>> --- a/drivers/gpu/drm/tegra/uapi.c >>> +++ b/drivers/gpu/drm/tegra/uapi.c >>> @@ -33,6 +33,9 @@ static void tegra_drm_channel_context_close(struct >>> tegra_drm_context *context) >>> struct tegra_drm_mapping *mapping; >>> unsigned long id; >>> + if (context->memory_context) >>> + host1x_context_put(context->memory_context); >> >> The "if (context->memory_context && >> context->client->ops->get_streamid_offset)" above doesn't match the "if >> (context->memory_context)". You'll get refcount underflow. > > And this drop is for the refcount implicitly added when allocating the > memory context through host1x_context_alloc; so these two places should > be independent. > > Please elaborate if I missed something. You named context as memory_context and then have context=context->memory_context. Please try to improve the variable names, like drm_ctx->host1x_ctx for example. I'm also not a big fan of the "if (ref) put(ref)" pattern. Won't be better to move all the "if (!NULL)" checks inside of get()/put() and make the invocations unconditional?
On 2/21/22 22:02, Dmitry Osipenko wrote: > 21.02.2022 15:06, Mikko Perttunen пишет: >> On 2/19/22 20:35, Dmitry Osipenko wrote: >>> 18.02.2022 14:39, Mikko Perttunen пишет: >>>> + if (context->memory_context && >>>> context->client->ops->get_streamid_offset) { >>> ^^^ >>>> + int offset = >>>> context->client->ops->get_streamid_offset(context->client); >>>> + >>>> + if (offset >= 0) { >>>> + job->context = context->memory_context; >>>> + job->engine_streamid_offset = offset; >>>> + host1x_context_get(job->context); >>>> + } >>> >>> You should bump refcount unconditionally or you'll get refcnt underflow >>> on put, when offset < 0. >> >> This refcount is intended to be dropped from 'release_job', where it's >> dropped if job->context is set, which it is from this path. >> >>> >>>> + } >>>> + >>>> /* >>>> * job_data is now part of job reference counting, so don't >>>> release >>>> * it from here. >>>> diff --git a/drivers/gpu/drm/tegra/uapi.c b/drivers/gpu/drm/tegra/uapi.c >>>> index 9ab9179d2026..be33da54d12c 100644 >>>> --- a/drivers/gpu/drm/tegra/uapi.c >>>> +++ b/drivers/gpu/drm/tegra/uapi.c >>>> @@ -33,6 +33,9 @@ static void tegra_drm_channel_context_close(struct >>>> tegra_drm_context *context) >>>> struct tegra_drm_mapping *mapping; >>>> unsigned long id; >>>> + if (context->memory_context) >>>> + host1x_context_put(context->memory_context); >>> >>> The "if (context->memory_context && >>> context->client->ops->get_streamid_offset)" above doesn't match the "if >>> (context->memory_context)". You'll get refcount underflow. >> >> And this drop is for the refcount implicitly added when allocating the >> memory context through host1x_context_alloc; so these two places should >> be independent. >> >> Please elaborate if I missed something. > > You named context as memory_context and then have > context=context->memory_context. Please try to improve the variable > names, like drm_ctx->host1x_ctx for example. > > I'm also not a big fan of the "if (ref) put(ref)" pattern. Won't be > better to move all the "if (!NULL)" checks inside of get()/put() and > make the invocations unconditional? I agree that the naming here is confusing with different kinds of contexts flying around, though I would prefer not to change the original 'struct tegra_drm_context *context' since it's used all around the code. But I'll try to make it clearer. Regarding moving NULL checks inside get/put, I personally dislike that pattern (also with e.g. kfree) since when reading the code, it makes it more difficult to see that the pointer can be NULL. Mikko
diff --git a/drivers/gpu/drm/tegra/drm.h b/drivers/gpu/drm/tegra/drm.h index fc0a19554eac..717e9f81ee1f 100644 --- a/drivers/gpu/drm/tegra/drm.h +++ b/drivers/gpu/drm/tegra/drm.h @@ -80,6 +80,7 @@ struct tegra_drm_context { /* Only used by new UAPI. */ struct xarray mappings; + struct host1x_context *memory_context; }; struct tegra_drm_client_ops { @@ -91,6 +92,7 @@ struct tegra_drm_client_ops { int (*submit)(struct tegra_drm_context *context, struct drm_tegra_submit *args, struct drm_device *drm, struct drm_file *file); + int (*get_streamid_offset)(struct tegra_drm_client *client); }; int tegra_drm_submit(struct tegra_drm_context *context, diff --git a/drivers/gpu/drm/tegra/submit.c b/drivers/gpu/drm/tegra/submit.c index 6d6dd8c35475..8d74b82b83a5 100644 --- a/drivers/gpu/drm/tegra/submit.c +++ b/drivers/gpu/drm/tegra/submit.c @@ -498,6 +498,9 @@ static void release_job(struct host1x_job *job) struct tegra_drm_submit_data *job_data = job->user_data; u32 i; + if (job->context) + host1x_context_put(job->context); + for (i = 0; i < job_data->num_used_mappings; i++) tegra_drm_mapping_put(job_data->used_mappings[i].mapping); @@ -599,6 +602,16 @@ int tegra_drm_ioctl_channel_submit(struct drm_device *drm, void *data, job->release = release_job; job->timeout = 10000; + if (context->memory_context && context->client->ops->get_streamid_offset) { + int offset = context->client->ops->get_streamid_offset(context->client); + + if (offset >= 0) { + job->context = context->memory_context; + job->engine_streamid_offset = offset; + host1x_context_get(job->context); + } + } + /* * job_data is now part of job reference counting, so don't release * it from here. diff --git a/drivers/gpu/drm/tegra/uapi.c b/drivers/gpu/drm/tegra/uapi.c index 9ab9179d2026..be33da54d12c 100644 --- a/drivers/gpu/drm/tegra/uapi.c +++ b/drivers/gpu/drm/tegra/uapi.c @@ -33,6 +33,9 @@ static void tegra_drm_channel_context_close(struct tegra_drm_context *context) struct tegra_drm_mapping *mapping; unsigned long id; + if (context->memory_context) + host1x_context_put(context->memory_context); + xa_for_each(&context->mappings, id, mapping) tegra_drm_mapping_put(mapping); @@ -72,6 +75,7 @@ static struct tegra_drm_client *tegra_drm_find_client(struct tegra_drm *tegra, u int tegra_drm_ioctl_channel_open(struct drm_device *drm, void *data, struct drm_file *file) { + struct host1x *host = tegra_drm_to_host1x(drm->dev_private); struct tegra_drm_file *fpriv = file->driver_priv; struct tegra_drm *tegra = drm->dev_private; struct drm_tegra_channel_open *args = data; @@ -102,10 +106,29 @@ int tegra_drm_ioctl_channel_open(struct drm_device *drm, void *data, struct drm_ } } + /* Only allocate context if the engine supports context isolation. */ + if (client->ops->get_streamid_offset && + client->ops->get_streamid_offset(client) >= 0) { + context->memory_context = + host1x_context_alloc(host, get_task_pid(current, PIDTYPE_TGID)); + if (IS_ERR(context->memory_context)) { + if (PTR_ERR(context->memory_context) != -EOPNOTSUPP) { + err = PTR_ERR(context->memory_context); + goto put_channel; + } else { + /* + * OK, HW does not support contexts or contexts + * are disabled. + */ + context->memory_context = NULL; + } + } + } + err = xa_alloc(&fpriv->contexts, &args->context, context, XA_LIMIT(1, U32_MAX), GFP_KERNEL); if (err < 0) - goto put_channel; + goto put_memctx; context->client = client; xa_init_flags(&context->mappings, XA_FLAGS_ALLOC1); @@ -118,6 +141,9 @@ int tegra_drm_ioctl_channel_open(struct drm_device *drm, void *data, struct drm_ return 0; +put_memctx: + if (context->memory_context) + host1x_context_put(context->memory_context); put_channel: host1x_channel_put(context->channel); free: @@ -156,6 +182,7 @@ int tegra_drm_ioctl_channel_map(struct drm_device *drm, void *data, struct drm_f struct tegra_drm_mapping *mapping; struct tegra_drm_context *context; enum dma_data_direction direction; + struct device *mapping_dev; int err = 0; if (args->flags & ~DRM_TEGRA_CHANNEL_MAP_READ_WRITE) @@ -177,6 +204,11 @@ int tegra_drm_ioctl_channel_map(struct drm_device *drm, void *data, struct drm_f kref_init(&mapping->ref); + if (context->memory_context) + mapping_dev = &context->memory_context->dev; + else + mapping_dev = context->client->base.dev; + mapping->bo = tegra_gem_lookup(file, args->handle); if (!mapping->bo) { err = -EINVAL; @@ -201,7 +233,7 @@ int tegra_drm_ioctl_channel_map(struct drm_device *drm, void *data, struct drm_f goto put_gem; } - mapping->map = host1x_bo_pin(context->client->base.dev, mapping->bo, direction, NULL); + mapping->map = host1x_bo_pin(mapping_dev, mapping->bo, direction, NULL); if (IS_ERR(mapping->map)) { err = PTR_ERR(mapping->map); goto put_gem;
For engines that support context isolation, allocate a context when opening a channel, and set up stream ID offset and context fields when submitting a job. Signed-off-by: Mikko Perttunen <mperttunen@nvidia.com> --- drivers/gpu/drm/tegra/drm.h | 2 ++ drivers/gpu/drm/tegra/submit.c | 13 ++++++++++++ drivers/gpu/drm/tegra/uapi.c | 36 ++++++++++++++++++++++++++++++++-- 3 files changed, 49 insertions(+), 2 deletions(-)