| Message ID | 20170320184402.32723-1-mperttunen@nvidia.com |
|---|---|
| State | Superseded |
| Headers | show |
On 20.03.2017 21:44, Mikko Perttunen wrote: > This is largely a rewrite of the Host1x channel allocation > code in channel.c, bringing several changes: > > - The previous code could deadlock due to an interaction > between the 'reflock' mutex and CDMA timeout handling. > This gets rid of the mutex. > - In the future, per-user channel allocation will be > useful to have. This paves the way for that by allowing > host1x_channel_request to wait for a free channel, and > freeing channels when their refcount drops to zero. > - Support for more than 32 channels, required for Tegra186 > - General refactoring, including better encapsulation > of channel ownership handling into channel.c > > Signed-off-by: Mikko Perttunen <mperttunen@nvidia.com> > --- > Ran though test suite at github.com/cyndis/host1x_test. > Fixes the timeout test there. > > drivers/gpu/drm/tegra/gr2d.c | 6 +- > drivers/gpu/drm/tegra/gr3d.c | 6 +- > drivers/gpu/drm/tegra/vic.c | 6 +- > drivers/gpu/host1x/cdma.c | 2 +- > drivers/gpu/host1x/channel.c | 154 +++++++++++++++++++++++-------------- > drivers/gpu/host1x/channel.h | 28 ++++--- > drivers/gpu/host1x/debug.c | 49 +++++------- > drivers/gpu/host1x/dev.c | 6 +- > drivers/gpu/host1x/dev.h | 8 +- > drivers/gpu/host1x/hw/channel_hw.c | 12 +-- > include/linux/host1x.h | 1 - > 11 files changed, 155 insertions(+), 123 deletions(-) > > diff --git a/drivers/gpu/drm/tegra/gr2d.c b/drivers/gpu/drm/tegra/gr2d.c > index 02cd3e37a6ec..936ce1a55987 100644 > --- a/drivers/gpu/drm/tegra/gr2d.c > +++ b/drivers/gpu/drm/tegra/gr2d.c > @@ -1,5 +1,5 @@ > /* > - * Copyright (c) 2012-2013, NVIDIA Corporation. > + * Copyright (c) 2012-2017, NVIDIA Corporation. > * > * This program is free software; you can redistribute it and/or modify > * it under the terms of the GNU General Public License version 2 as > @@ -38,7 +38,7 @@ static int gr2d_init(struct host1x_client *client) > > client->syncpts[0] = host1x_syncpt_request(client->dev, flags); > if (!client->syncpts[0]) { > - host1x_channel_free(gr2d->channel); > + host1x_channel_put(gr2d->channel); > return -ENOMEM; > } > > @@ -57,7 +57,7 @@ static int gr2d_exit(struct host1x_client *client) > return err; > > host1x_syncpt_free(client->syncpts[0]); > - host1x_channel_free(gr2d->channel); > + host1x_channel_put(gr2d->channel); > > return 0; > } > diff --git a/drivers/gpu/drm/tegra/gr3d.c b/drivers/gpu/drm/tegra/gr3d.c > index 13f0d1b7cd98..da02230c58ef 100644 > --- a/drivers/gpu/drm/tegra/gr3d.c > +++ b/drivers/gpu/drm/tegra/gr3d.c > @@ -1,6 +1,6 @@ > /* > * Copyright (C) 2013 Avionic Design GmbH > - * Copyright (C) 2013 NVIDIA Corporation > + * Copyright (C) 2013-2017 NVIDIA Corporation > * > * This program is free software; you can redistribute it and/or modify > * it under the terms of the GNU General Public License version 2 as > @@ -48,7 +48,7 @@ static int gr3d_init(struct host1x_client *client) > > client->syncpts[0] = host1x_syncpt_request(client->dev, flags); > if (!client->syncpts[0]) { > - host1x_channel_free(gr3d->channel); > + host1x_channel_put(gr3d->channel); > return -ENOMEM; > } > > @@ -67,7 +67,7 @@ static int gr3d_exit(struct host1x_client *client) > return err; > > host1x_syncpt_free(client->syncpts[0]); > - host1x_channel_free(gr3d->channel); > + host1x_channel_put(gr3d->channel); > > return 0; > } > diff --git a/drivers/gpu/drm/tegra/vic.c b/drivers/gpu/drm/tegra/vic.c > index cd804e404a11..5a36adfc01f6 100644 > --- a/drivers/gpu/drm/tegra/vic.c > +++ b/drivers/gpu/drm/tegra/vic.c > @@ -1,5 +1,5 @@ > /* > - * Copyright (c) 2015, NVIDIA Corporation. > + * Copyright (c) 2015-2017, NVIDIA Corporation. > * > * This program is free software; you can redistribute it and/or modify > * it under the terms of the GNU General Public License version 2 as > @@ -182,7 +182,7 @@ static int vic_init(struct host1x_client *client) > free_syncpt: > host1x_syncpt_free(client->syncpts[0]); > free_channel: > - host1x_channel_free(vic->channel); > + host1x_channel_put(vic->channel); > detach_device: > if (tegra->domain) > iommu_detach_device(tegra->domain, vic->dev); > @@ -203,7 +203,7 @@ static int vic_exit(struct host1x_client *client) > return err; > > host1x_syncpt_free(client->syncpts[0]); > - host1x_channel_free(vic->channel); > + host1x_channel_put(vic->channel); > > if (vic->domain) { > iommu_detach_device(vic->domain, vic->dev); > diff --git a/drivers/gpu/host1x/cdma.c b/drivers/gpu/host1x/cdma.c > index 28541b280739..d7c1e269ed67 100644 > --- a/drivers/gpu/host1x/cdma.c > +++ b/drivers/gpu/host1x/cdma.c > @@ -1,7 +1,7 @@ > /* > * Tegra host1x Command DMA > * > - * Copyright (c) 2010-2013, NVIDIA Corporation. > + * Copyright (c) 2010-2017, NVIDIA Corporation. > * > * This program is free software; you can redistribute it and/or modify it > * under the terms and conditions of the GNU General Public License, > diff --git a/drivers/gpu/host1x/channel.c b/drivers/gpu/host1x/channel.c > index 8f437d924c10..819e97ccb00d 100644 > --- a/drivers/gpu/host1x/channel.c > +++ b/drivers/gpu/host1x/channel.c > @@ -1,7 +1,7 @@ > /* > * Tegra host1x Channel > * > - * Copyright (c) 2010-2013, NVIDIA Corporation. > + * Copyright (c) 2010-2017, NVIDIA Corporation. > * > * This program is free software; you can redistribute it and/or modify it > * under the terms and conditions of the GNU General Public License, > @@ -24,19 +24,36 @@ > #include "job.h" > > /* Constructor for the host1x device list */ > -int host1x_channel_list_init(struct host1x *host) > +int host1x_channel_list_init(struct host1x_channel_list *chlist, > + unsigned int num_channels) > { > - INIT_LIST_HEAD(&host->chlist.list); > - mutex_init(&host->chlist_mutex); > - > - if (host->info->nb_channels > BITS_PER_LONG) { > - WARN(1, "host1x hardware has more channels than supported by the driver\n"); > - return -ENOSYS; > + chlist->channels = kcalloc(num_channels, sizeof(struct host1x_channel), > + GFP_KERNEL); > + if (!chlist->channels) > + return -ENOMEM; > + > + chlist->allocated_channels = > + kcalloc(BITS_TO_LONGS(num_channels), sizeof(unsigned long), > + GFP_KERNEL); > + if (!chlist->allocated_channels) { > + kfree(chlist->channels); > + return -ENOMEM; > } > > + bitmap_zero(chlist->allocated_channels, num_channels); > + > + mutex_init(&chlist->alloc_lock); > + sema_init(&chlist->alloc_sema, num_channels); > + > return 0; > } > > +void host1x_channel_list_free(struct host1x_channel_list *chlist) > +{ > + kfree(chlist->allocated_channels); > + kfree(chlist->channels); > +} > + > int host1x_job_submit(struct host1x_job *job) > { > struct host1x *host = dev_get_drvdata(job->channel->dev->parent); > @@ -47,86 +64,107 @@ EXPORT_SYMBOL(host1x_job_submit); > > struct host1x_channel *host1x_channel_get(struct host1x_channel *channel) > { > - int err = 0; > - > - mutex_lock(&channel->reflock); > - > - if (channel->refcount == 0) > - err = host1x_cdma_init(&channel->cdma); > + kref_get(&channel->refcount); > > - if (!err) > - channel->refcount++; > + return channel; > +} > +EXPORT_SYMBOL(host1x_channel_get); > > - mutex_unlock(&channel->reflock); > +/** > + * host1x_channel_get_index() - Attempt to get channel reference by index > + * @host: Host1x device object > + * @index: Index of channel > + * > + * If channel number @index is currently allocated, increase its refcount > + * and return a pointer to it. Otherwise, return NULL. > + */ > +struct host1x_channel *host1x_channel_get_index(struct host1x *host, > + unsigned int index) > +{ > + struct host1x_channel *ch = &host->channel_list.channels[index]; > > - return err ? NULL : channel; > + if (kref_get_unless_zero(&ch->refcount)) > + return ch; > + else > + return NULL; > } > -EXPORT_SYMBOL(host1x_channel_get); > > -void host1x_channel_put(struct host1x_channel *channel) > +static void release_channel(struct kref *kref) > { > - mutex_lock(&channel->reflock); > + struct host1x_channel *channel = > + container_of(kref, struct host1x_channel, refcount); > + struct host1x *host = dev_get_drvdata(channel->dev->parent); > + struct host1x_channel_list *chlist = &host->channel_list; > > - if (channel->refcount == 1) { > - struct host1x *host = dev_get_drvdata(channel->dev->parent); > + host1x_hw_cdma_stop(host, &channel->cdma); > + host1x_cdma_deinit(&channel->cdma); > > - host1x_hw_cdma_stop(host, &channel->cdma); > - host1x_cdma_deinit(&channel->cdma); > - } > + clear_bit(channel->id, chlist->allocated_channels); > > - channel->refcount--; > + up(&chlist->alloc_sema); > +} > > - mutex_unlock(&channel->reflock); > +void host1x_channel_put(struct host1x_channel *channel) > +{ > + kref_put(&channel->refcount, release_channel); > } > EXPORT_SYMBOL(host1x_channel_put); > > +/** > + * host1x_channel_request() - Allocate a channel > + * @device: Host1x unit this channel will be used to send commands to > + * > + * Allocates a new host1x channel for @device. If there are no free channels, > + * this will sleep until one becomes available. May return NULL if CDMA > + * initialization fails. > + */ > struct host1x_channel *host1x_channel_request(struct device *dev) > { > struct host1x *host = dev_get_drvdata(dev->parent); > + struct host1x_channel_list *chlist = &host->channel_list; > unsigned int max_channels = host->info->nb_channels; > struct host1x_channel *channel = NULL; > - unsigned long index; > + unsigned int index; > int err; > > - mutex_lock(&host->chlist_mutex); > + down(&chlist->alloc_sema); > + It's a bit hard to follow lockings in the code, could you please explain why you've added this semaphore? What problem it solves? > + mutex_lock(&chlist->alloc_lock); > > - index = find_first_zero_bit(&host->allocated_channels, max_channels); > - if (index >= max_channels) > - goto fail; > + index = find_first_zero_bit(chlist->allocated_channels, max_channels); > + if (index >= max_channels) { > + mutex_unlock(&chlist->alloc_lock); > + dev_err(dev, "failed to find free channel\n"); > + return NULL; > + } > > - channel = kzalloc(sizeof(*channel), GFP_KERNEL); > - if (!channel) > - goto fail; > + set_bit(index, chlist->allocated_channels); > > - err = host1x_hw_channel_init(host, channel, index); > - if (err < 0) > - goto fail; > + mutex_unlock(&chlist->alloc_lock); > > - /* Link device to host1x_channel */ > + channel = &chlist->channels[index]; > + > + kref_init(&channel->refcount); > + mutex_init(&channel->submit_lock); > + channel->id = index; > channel->dev = dev; > > - /* Add to channel list */ > - list_add_tail(&channel->list, &host->chlist.list); > + err = host1x_hw_channel_init(host, channel, index); > + if (err < 0) > + goto free; > > - host->allocated_channels |= BIT(index); > + err = host1x_cdma_init(&channel->cdma); > + if (err < 0) > + goto free; > > - mutex_unlock(&host->chlist_mutex); > return channel; > > -fail: > - dev_err(dev, "failed to init channel\n"); > - kfree(channel); > - mutex_unlock(&host->chlist_mutex); > - return NULL; > -} > -EXPORT_SYMBOL(host1x_channel_request); > +free: > + clear_bit(channel->id, chlist->allocated_channels); > + up(&chlist->alloc_sema); > > -void host1x_channel_free(struct host1x_channel *channel) > -{ > - struct host1x *host = dev_get_drvdata(channel->dev->parent); > + dev_err(dev, "failed to initialize channel\n"); > > - host->allocated_channels &= ~BIT(channel->id); > - list_del(&channel->list); > - kfree(channel); > + return NULL; > } > -EXPORT_SYMBOL(host1x_channel_free); > +EXPORT_SYMBOL(host1x_channel_request); > diff --git a/drivers/gpu/host1x/channel.h b/drivers/gpu/host1x/channel.h > index df767cf90d51..91a40f221415 100644 > --- a/drivers/gpu/host1x/channel.h > +++ b/drivers/gpu/host1x/channel.h > @@ -1,7 +1,7 @@ > /* > * Tegra host1x Channel > * > - * Copyright (c) 2010-2013, NVIDIA Corporation. > + * Copyright (c) 2010-2017, NVIDIA Corporation. > * > * This program is free software; you can redistribute it and/or modify it > * under the terms and conditions of the GNU General Public License, > @@ -20,27 +20,37 @@ > #define __HOST1X_CHANNEL_H > > #include <linux/io.h> > +#include <linux/kref.h> > > #include "cdma.h" > > struct host1x; > +struct host1x_channel; > + > +struct host1x_channel_list { > + struct host1x_channel *channels; > + > + /* Allows clients to wait for a free channel */ > + struct semaphore alloc_sema; > + struct mutex alloc_lock; > + unsigned long *allocated_channels; > +}; > > struct host1x_channel { > - struct list_head list; > + struct kref refcount; > > - unsigned int refcount; > unsigned int id; > - struct mutex reflock; > - struct mutex submitlock; > + struct mutex submit_lock; > void __iomem *regs; > struct device *dev; > struct host1x_cdma cdma; > }; > > /* channel list operations */ > -int host1x_channel_list_init(struct host1x *host); > - > -#define host1x_for_each_channel(host, channel) \ > - list_for_each_entry(channel, &host->chlist.list, list) > +int host1x_channel_list_init(struct host1x_channel_list *chlist, > + unsigned int num_channels); > +void host1x_channel_list_free(struct host1x_channel_list *chlist); > +struct host1x_channel *host1x_channel_get_index(struct host1x *host, > + unsigned int index); > > #endif > diff --git a/drivers/gpu/host1x/debug.c b/drivers/gpu/host1x/debug.c > index d9330fcc62ad..0e8b66fea29f 100644 > --- a/drivers/gpu/host1x/debug.c > +++ b/drivers/gpu/host1x/debug.c > @@ -2,7 +2,7 @@ > * Copyright (C) 2010 Google, Inc. > * Author: Erik Gilling <konkers@android.com> > * > - * Copyright (C) 2011-2013 NVIDIA Corporation > + * Copyright (C) 2011-2017 NVIDIA Corporation > * > * This software is licensed under the terms of the GNU General Public > * License version 2, as published by the Free Software Foundation, and > @@ -43,24 +43,19 @@ void host1x_debug_output(struct output *o, const char *fmt, ...) > o->fn(o->ctx, o->buf, len); > } > > -static int show_channels(struct host1x_channel *ch, void *data, bool show_fifo) > +static int show_channel(struct host1x_channel *ch, void *data, bool show_fifo) > { > struct host1x *m = dev_get_drvdata(ch->dev->parent); > struct output *o = data; > > - mutex_lock(&ch->reflock); > + mutex_lock(&ch->cdma.lock); > > - if (ch->refcount) { > - mutex_lock(&ch->cdma.lock); > + if (show_fifo) > + host1x_hw_show_channel_fifo(m, ch, o); > > - if (show_fifo) > - host1x_hw_show_channel_fifo(m, ch, o); > + host1x_hw_show_channel_cdma(m, ch, o); > > - host1x_hw_show_channel_cdma(m, ch, o); > - mutex_unlock(&ch->cdma.lock); > - } > - > - mutex_unlock(&ch->reflock); > + mutex_unlock(&ch->cdma.lock); > > return 0; > } > @@ -94,28 +89,22 @@ static void show_syncpts(struct host1x *m, struct output *o) > host1x_debug_output(o, "\n"); > } > > -static void show_all(struct host1x *m, struct output *o) > +static void show_all(struct host1x *m, struct output *o, bool show_fifo) > { > - struct host1x_channel *ch; > + int i; > > host1x_hw_show_mlocks(m, o); > show_syncpts(m, o); > host1x_debug_output(o, "---- channels ----\n"); > > - host1x_for_each_channel(m, ch) > - show_channels(ch, o, true); > -} > - > -static void show_all_no_fifo(struct host1x *host1x, struct output *o) > -{ > - struct host1x_channel *ch; > - > - host1x_hw_show_mlocks(host1x, o); > - show_syncpts(host1x, o); > - host1x_debug_output(o, "---- channels ----\n"); > + for (i = 0; i < m->info->nb_channels; ++i) { > + struct host1x_channel *ch = host1x_channel_get_index(m, i); > > - host1x_for_each_channel(host1x, ch) > - show_channels(ch, o, false); > + if (ch) { > + show_channel(ch, o, show_fifo); > + host1x_channel_put(ch); > + } > + } > } > > static int host1x_debug_show_all(struct seq_file *s, void *unused) > @@ -125,7 +114,7 @@ static int host1x_debug_show_all(struct seq_file *s, void *unused) > .ctx = s > }; > > - show_all(s->private, &o); > + show_all(s->private, &o, true); > > return 0; > } > @@ -137,7 +126,7 @@ static int host1x_debug_show(struct seq_file *s, void *unused) > .ctx = s > }; > > - show_all_no_fifo(s->private, &o); > + show_all(s->private, &o, false); > > return 0; > } > @@ -216,7 +205,7 @@ void host1x_debug_dump(struct host1x *host1x) > .fn = write_to_printk > }; > > - show_all(host1x, &o); > + show_all(host1x, &o, true); > } > > void host1x_debug_dump_syncpts(struct host1x *host1x) > diff --git a/drivers/gpu/host1x/dev.c b/drivers/gpu/host1x/dev.c > index a8234bb49403..f57ef45f75c9 100644 > --- a/drivers/gpu/host1x/dev.c > +++ b/drivers/gpu/host1x/dev.c > @@ -1,7 +1,7 @@ > /* > * Tegra host1x driver > * > - * Copyright (c) 2010-2013, NVIDIA Corporation. > + * Copyright (c) 2010-2017, NVIDIA Corporation. > * > * This program is free software; you can redistribute it and/or modify it > * under the terms and conditions of the GNU General Public License, > @@ -189,7 +189,8 @@ static int host1x_probe(struct platform_device *pdev) > host->iova_end = geometry->aperture_end; > } > > - err = host1x_channel_list_init(host); > + err = host1x_channel_list_init(&host->channel_list, > + host->info->nb_channels); > if (err) { > dev_err(&pdev->dev, "failed to initialize channel list\n"); > goto fail_detach_device; > @@ -247,6 +248,7 @@ static int host1x_remove(struct platform_device *pdev) > host1x_intr_deinit(host); > host1x_syncpt_deinit(host); > clk_disable_unprepare(host->clk); > + host1x_channel_list_free(&host->channel_list); > > if (host->domain) { > put_iova_domain(&host->iova); > diff --git a/drivers/gpu/host1x/dev.h b/drivers/gpu/host1x/dev.h > index 1ee79dbd1882..a558e39bf7f5 100644 > --- a/drivers/gpu/host1x/dev.h > +++ b/drivers/gpu/host1x/dev.h > @@ -1,5 +1,5 @@ > /* > - * Copyright (C) 2012-2016 NVIDIA CORPORATION. All rights reserved. > + * Copyright (C) 2012-2017 NVIDIA CORPORATION. All rights reserved. > * > * This program is free software; you can redistribute it and/or modify it > * under the terms and conditions of the GNU General Public License, > @@ -130,10 +130,8 @@ struct host1x { > struct host1x_syncpt *nop_sp; > > struct mutex syncpt_mutex; > - struct mutex chlist_mutex; > - struct host1x_channel chlist; > - unsigned long allocated_channels; > - unsigned int num_allocated_channels; > + > + struct host1x_channel_list channel_list; > > struct dentry *debugfs; > > diff --git a/drivers/gpu/host1x/hw/channel_hw.c b/drivers/gpu/host1x/hw/channel_hw.c > index 27dc78f4c400..286c81aad308 100644 > --- a/drivers/gpu/host1x/hw/channel_hw.c > +++ b/drivers/gpu/host1x/hw/channel_hw.c > @@ -138,13 +138,13 @@ static int channel_submit(struct host1x_job *job) > } > > /* get submit lock */ > - err = mutex_lock_interruptible(&ch->submitlock); > + err = mutex_lock_interruptible(&ch->submit_lock); > if (err) > goto error; > > completed_waiter = kzalloc(sizeof(*completed_waiter), GFP_KERNEL); > if (!completed_waiter) { > - mutex_unlock(&ch->submitlock); > + mutex_unlock(&ch->submit_lock); > err = -ENOMEM; > goto error; > } > @@ -152,7 +152,7 @@ static int channel_submit(struct host1x_job *job) > /* begin a CDMA submit */ > err = host1x_cdma_begin(&ch->cdma, job); > if (err) { > - mutex_unlock(&ch->submitlock); > + mutex_unlock(&ch->submit_lock); > goto error; > } > > @@ -190,7 +190,7 @@ static int channel_submit(struct host1x_job *job) > completed_waiter = NULL; > WARN(err, "Failed to set submit complete interrupt"); > > - mutex_unlock(&ch->submitlock); > + mutex_unlock(&ch->submit_lock); > > return 0; > > @@ -202,10 +202,6 @@ static int channel_submit(struct host1x_job *job) > static int host1x_channel_init(struct host1x_channel *ch, struct host1x *dev, > unsigned int index) > { > - ch->id = index; > - mutex_init(&ch->reflock); > - mutex_init(&ch->submitlock); > - > ch->regs = dev->regs + index * HOST1X_CHANNEL_SIZE; > return 0; > } > diff --git a/include/linux/host1x.h b/include/linux/host1x.h > index fd9b526486e0..921ec6465952 100644 > --- a/include/linux/host1x.h > +++ b/include/linux/host1x.h > @@ -157,7 +157,6 @@ struct host1x_channel; > struct host1x_job; > > struct host1x_channel *host1x_channel_request(struct device *dev); > -void host1x_channel_free(struct host1x_channel *channel); > struct host1x_channel *host1x_channel_get(struct host1x_channel *channel); > void host1x_channel_put(struct host1x_channel *channel); > int host1x_job_submit(struct host1x_job *job); >
On 18.05.2017 14:42, Dmitry Osipenko wrote: > On 20.03.2017 21:44, Mikko Perttunen wrote: >> ... >> struct host1x_channel *host1x_channel_request(struct device *dev) >> { >> struct host1x *host = dev_get_drvdata(dev->parent); >> + struct host1x_channel_list *chlist = &host->channel_list; >> unsigned int max_channels = host->info->nb_channels; >> struct host1x_channel *channel = NULL; >> - unsigned long index; >> + unsigned int index; >> int err; >> >> - mutex_lock(&host->chlist_mutex); >> + down(&chlist->alloc_sema); >> + > > It's a bit hard to follow lockings in the code, could you please explain why > you've added this semaphore? What problem it solves? Sure. The role of the semaphore is to allow the caller to sleep waiting for a free channel, and also to be sure that a channel is available once the down() call finishes. Currently the channel allocation model is such that each unit allocates a single channel for itself, in which case this is not useful, but with later chips with more available channels we can move to a model where a channel is allocated for each userspace client. In this case we can allow the application to sleep if there are no available channels. Of course, there is also the possibility of just returning -EBUSY if there are no channels - that's probably fine too.. Cheers, Mikko. -- To unsubscribe from this list: send the line "unsubscribe linux-tegra" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 18.05.2017 14:55, Mikko Perttunen wrote: > On 18.05.2017 14:42, Dmitry Osipenko wrote: >> On 20.03.2017 21:44, Mikko Perttunen wrote: >>> ... >>> struct host1x_channel *host1x_channel_request(struct device *dev) >>> { >>> struct host1x *host = dev_get_drvdata(dev->parent); >>> + struct host1x_channel_list *chlist = &host->channel_list; >>> unsigned int max_channels = host->info->nb_channels; >>> struct host1x_channel *channel = NULL; >>> - unsigned long index; >>> + unsigned int index; >>> int err; >>> >>> - mutex_lock(&host->chlist_mutex); >>> + down(&chlist->alloc_sema); >>> + >> >> It's a bit hard to follow lockings in the code, could you please explain why >> you've added this semaphore? What problem it solves? > > Sure. The role of the semaphore is to allow the caller to sleep waiting for a > free channel, and also to be sure that a channel is available once the down() > call finishes. Currently the channel allocation model is such that each unit > allocates a single channel for itself, in which case this is not useful, but > with later chips with more available channels we can move to a model where a > channel is allocated for each userspace client. In this case we can allow the > application to sleep if there are no available channels. > > Of course, there is also the possibility of just returning -EBUSY if there are > no channels - that's probably fine too.. > Currently, the host1x_channel_request() is only invoked during the drivers probe routine, it has nothing to do with userspace. I'd suggest to drop that semaphore from the patch since it's unrelated to the current channel allocation model.
diff --git a/drivers/gpu/drm/tegra/gr2d.c b/drivers/gpu/drm/tegra/gr2d.c index 02cd3e37a6ec..936ce1a55987 100644 --- a/drivers/gpu/drm/tegra/gr2d.c +++ b/drivers/gpu/drm/tegra/gr2d.c @@ -1,5 +1,5 @@ /* - * Copyright (c) 2012-2013, NVIDIA Corporation. + * Copyright (c) 2012-2017, NVIDIA Corporation. * * This program is free software; you can redistribute it and/or modify * it under the terms of the GNU General Public License version 2 as @@ -38,7 +38,7 @@ static int gr2d_init(struct host1x_client *client) client->syncpts[0] = host1x_syncpt_request(client->dev, flags); if (!client->syncpts[0]) { - host1x_channel_free(gr2d->channel); + host1x_channel_put(gr2d->channel); return -ENOMEM; } @@ -57,7 +57,7 @@ static int gr2d_exit(struct host1x_client *client) return err; host1x_syncpt_free(client->syncpts[0]); - host1x_channel_free(gr2d->channel); + host1x_channel_put(gr2d->channel); return 0; } diff --git a/drivers/gpu/drm/tegra/gr3d.c b/drivers/gpu/drm/tegra/gr3d.c index 13f0d1b7cd98..da02230c58ef 100644 --- a/drivers/gpu/drm/tegra/gr3d.c +++ b/drivers/gpu/drm/tegra/gr3d.c @@ -1,6 +1,6 @@ /* * Copyright (C) 2013 Avionic Design GmbH - * Copyright (C) 2013 NVIDIA Corporation + * Copyright (C) 2013-2017 NVIDIA Corporation * * This program is free software; you can redistribute it and/or modify * it under the terms of the GNU General Public License version 2 as @@ -48,7 +48,7 @@ static int gr3d_init(struct host1x_client *client) client->syncpts[0] = host1x_syncpt_request(client->dev, flags); if (!client->syncpts[0]) { - host1x_channel_free(gr3d->channel); + host1x_channel_put(gr3d->channel); return -ENOMEM; } @@ -67,7 +67,7 @@ static int gr3d_exit(struct host1x_client *client) return err; host1x_syncpt_free(client->syncpts[0]); - host1x_channel_free(gr3d->channel); + host1x_channel_put(gr3d->channel); return 0; } diff --git a/drivers/gpu/drm/tegra/vic.c b/drivers/gpu/drm/tegra/vic.c index cd804e404a11..5a36adfc01f6 100644 --- a/drivers/gpu/drm/tegra/vic.c +++ b/drivers/gpu/drm/tegra/vic.c @@ -1,5 +1,5 @@ /* - * Copyright (c) 2015, NVIDIA Corporation. + * Copyright (c) 2015-2017, NVIDIA Corporation. * * This program is free software; you can redistribute it and/or modify * it under the terms of the GNU General Public License version 2 as @@ -182,7 +182,7 @@ static int vic_init(struct host1x_client *client) free_syncpt: host1x_syncpt_free(client->syncpts[0]); free_channel: - host1x_channel_free(vic->channel); + host1x_channel_put(vic->channel); detach_device: if (tegra->domain) iommu_detach_device(tegra->domain, vic->dev); @@ -203,7 +203,7 @@ static int vic_exit(struct host1x_client *client) return err; host1x_syncpt_free(client->syncpts[0]); - host1x_channel_free(vic->channel); + host1x_channel_put(vic->channel); if (vic->domain) { iommu_detach_device(vic->domain, vic->dev); diff --git a/drivers/gpu/host1x/cdma.c b/drivers/gpu/host1x/cdma.c index 28541b280739..d7c1e269ed67 100644 --- a/drivers/gpu/host1x/cdma.c +++ b/drivers/gpu/host1x/cdma.c @@ -1,7 +1,7 @@ /* * Tegra host1x Command DMA * - * Copyright (c) 2010-2013, NVIDIA Corporation. + * Copyright (c) 2010-2017, NVIDIA Corporation. * * This program is free software; you can redistribute it and/or modify it * under the terms and conditions of the GNU General Public License, diff --git a/drivers/gpu/host1x/channel.c b/drivers/gpu/host1x/channel.c index 8f437d924c10..819e97ccb00d 100644 --- a/drivers/gpu/host1x/channel.c +++ b/drivers/gpu/host1x/channel.c @@ -1,7 +1,7 @@ /* * Tegra host1x Channel * - * Copyright (c) 2010-2013, NVIDIA Corporation. + * Copyright (c) 2010-2017, NVIDIA Corporation. * * This program is free software; you can redistribute it and/or modify it * under the terms and conditions of the GNU General Public License, @@ -24,19 +24,36 @@ #include "job.h" /* Constructor for the host1x device list */ -int host1x_channel_list_init(struct host1x *host) +int host1x_channel_list_init(struct host1x_channel_list *chlist, + unsigned int num_channels) { - INIT_LIST_HEAD(&host->chlist.list); - mutex_init(&host->chlist_mutex); - - if (host->info->nb_channels > BITS_PER_LONG) { - WARN(1, "host1x hardware has more channels than supported by the driver\n"); - return -ENOSYS; + chlist->channels = kcalloc(num_channels, sizeof(struct host1x_channel), + GFP_KERNEL); + if (!chlist->channels) + return -ENOMEM; + + chlist->allocated_channels = + kcalloc(BITS_TO_LONGS(num_channels), sizeof(unsigned long), + GFP_KERNEL); + if (!chlist->allocated_channels) { + kfree(chlist->channels); + return -ENOMEM; } + bitmap_zero(chlist->allocated_channels, num_channels); + + mutex_init(&chlist->alloc_lock); + sema_init(&chlist->alloc_sema, num_channels); + return 0; } +void host1x_channel_list_free(struct host1x_channel_list *chlist) +{ + kfree(chlist->allocated_channels); + kfree(chlist->channels); +} + int host1x_job_submit(struct host1x_job *job) { struct host1x *host = dev_get_drvdata(job->channel->dev->parent); @@ -47,86 +64,107 @@ EXPORT_SYMBOL(host1x_job_submit); struct host1x_channel *host1x_channel_get(struct host1x_channel *channel) { - int err = 0; - - mutex_lock(&channel->reflock); - - if (channel->refcount == 0) - err = host1x_cdma_init(&channel->cdma); + kref_get(&channel->refcount); - if (!err) - channel->refcount++; + return channel; +} +EXPORT_SYMBOL(host1x_channel_get); - mutex_unlock(&channel->reflock); +/** + * host1x_channel_get_index() - Attempt to get channel reference by index + * @host: Host1x device object + * @index: Index of channel + * + * If channel number @index is currently allocated, increase its refcount + * and return a pointer to it. Otherwise, return NULL. + */ +struct host1x_channel *host1x_channel_get_index(struct host1x *host, + unsigned int index) +{ + struct host1x_channel *ch = &host->channel_list.channels[index]; - return err ? NULL : channel; + if (kref_get_unless_zero(&ch->refcount)) + return ch; + else + return NULL; } -EXPORT_SYMBOL(host1x_channel_get); -void host1x_channel_put(struct host1x_channel *channel) +static void release_channel(struct kref *kref) { - mutex_lock(&channel->reflock); + struct host1x_channel *channel = + container_of(kref, struct host1x_channel, refcount); + struct host1x *host = dev_get_drvdata(channel->dev->parent); + struct host1x_channel_list *chlist = &host->channel_list; - if (channel->refcount == 1) { - struct host1x *host = dev_get_drvdata(channel->dev->parent); + host1x_hw_cdma_stop(host, &channel->cdma); + host1x_cdma_deinit(&channel->cdma); - host1x_hw_cdma_stop(host, &channel->cdma); - host1x_cdma_deinit(&channel->cdma); - } + clear_bit(channel->id, chlist->allocated_channels); - channel->refcount--; + up(&chlist->alloc_sema); +} - mutex_unlock(&channel->reflock); +void host1x_channel_put(struct host1x_channel *channel) +{ + kref_put(&channel->refcount, release_channel); } EXPORT_SYMBOL(host1x_channel_put); +/** + * host1x_channel_request() - Allocate a channel + * @device: Host1x unit this channel will be used to send commands to + * + * Allocates a new host1x channel for @device. If there are no free channels, + * this will sleep until one becomes available. May return NULL if CDMA + * initialization fails. + */ struct host1x_channel *host1x_channel_request(struct device *dev) { struct host1x *host = dev_get_drvdata(dev->parent); + struct host1x_channel_list *chlist = &host->channel_list; unsigned int max_channels = host->info->nb_channels; struct host1x_channel *channel = NULL; - unsigned long index; + unsigned int index; int err; - mutex_lock(&host->chlist_mutex); + down(&chlist->alloc_sema); + + mutex_lock(&chlist->alloc_lock); - index = find_first_zero_bit(&host->allocated_channels, max_channels); - if (index >= max_channels) - goto fail; + index = find_first_zero_bit(chlist->allocated_channels, max_channels); + if (index >= max_channels) { + mutex_unlock(&chlist->alloc_lock); + dev_err(dev, "failed to find free channel\n"); + return NULL; + } - channel = kzalloc(sizeof(*channel), GFP_KERNEL); - if (!channel) - goto fail; + set_bit(index, chlist->allocated_channels); - err = host1x_hw_channel_init(host, channel, index); - if (err < 0) - goto fail; + mutex_unlock(&chlist->alloc_lock); - /* Link device to host1x_channel */ + channel = &chlist->channels[index]; + + kref_init(&channel->refcount); + mutex_init(&channel->submit_lock); + channel->id = index; channel->dev = dev; - /* Add to channel list */ - list_add_tail(&channel->list, &host->chlist.list); + err = host1x_hw_channel_init(host, channel, index); + if (err < 0) + goto free; - host->allocated_channels |= BIT(index); + err = host1x_cdma_init(&channel->cdma); + if (err < 0) + goto free; - mutex_unlock(&host->chlist_mutex); return channel; -fail: - dev_err(dev, "failed to init channel\n"); - kfree(channel); - mutex_unlock(&host->chlist_mutex); - return NULL; -} -EXPORT_SYMBOL(host1x_channel_request); +free: + clear_bit(channel->id, chlist->allocated_channels); + up(&chlist->alloc_sema); -void host1x_channel_free(struct host1x_channel *channel) -{ - struct host1x *host = dev_get_drvdata(channel->dev->parent); + dev_err(dev, "failed to initialize channel\n"); - host->allocated_channels &= ~BIT(channel->id); - list_del(&channel->list); - kfree(channel); + return NULL; } -EXPORT_SYMBOL(host1x_channel_free); +EXPORT_SYMBOL(host1x_channel_request); diff --git a/drivers/gpu/host1x/channel.h b/drivers/gpu/host1x/channel.h index df767cf90d51..91a40f221415 100644 --- a/drivers/gpu/host1x/channel.h +++ b/drivers/gpu/host1x/channel.h @@ -1,7 +1,7 @@ /* * Tegra host1x Channel * - * Copyright (c) 2010-2013, NVIDIA Corporation. + * Copyright (c) 2010-2017, NVIDIA Corporation. * * This program is free software; you can redistribute it and/or modify it * under the terms and conditions of the GNU General Public License, @@ -20,27 +20,37 @@ #define __HOST1X_CHANNEL_H #include <linux/io.h> +#include <linux/kref.h> #include "cdma.h" struct host1x; +struct host1x_channel; + +struct host1x_channel_list { + struct host1x_channel *channels; + + /* Allows clients to wait for a free channel */ + struct semaphore alloc_sema; + struct mutex alloc_lock; + unsigned long *allocated_channels; +}; struct host1x_channel { - struct list_head list; + struct kref refcount; - unsigned int refcount; unsigned int id; - struct mutex reflock; - struct mutex submitlock; + struct mutex submit_lock; void __iomem *regs; struct device *dev; struct host1x_cdma cdma; }; /* channel list operations */ -int host1x_channel_list_init(struct host1x *host); - -#define host1x_for_each_channel(host, channel) \ - list_for_each_entry(channel, &host->chlist.list, list) +int host1x_channel_list_init(struct host1x_channel_list *chlist, + unsigned int num_channels); +void host1x_channel_list_free(struct host1x_channel_list *chlist); +struct host1x_channel *host1x_channel_get_index(struct host1x *host, + unsigned int index); #endif diff --git a/drivers/gpu/host1x/debug.c b/drivers/gpu/host1x/debug.c index d9330fcc62ad..0e8b66fea29f 100644 --- a/drivers/gpu/host1x/debug.c +++ b/drivers/gpu/host1x/debug.c @@ -2,7 +2,7 @@ * Copyright (C) 2010 Google, Inc. * Author: Erik Gilling <konkers@android.com> * - * Copyright (C) 2011-2013 NVIDIA Corporation + * Copyright (C) 2011-2017 NVIDIA Corporation * * This software is licensed under the terms of the GNU General Public * License version 2, as published by the Free Software Foundation, and @@ -43,24 +43,19 @@ void host1x_debug_output(struct output *o, const char *fmt, ...) o->fn(o->ctx, o->buf, len); } -static int show_channels(struct host1x_channel *ch, void *data, bool show_fifo) +static int show_channel(struct host1x_channel *ch, void *data, bool show_fifo) { struct host1x *m = dev_get_drvdata(ch->dev->parent); struct output *o = data; - mutex_lock(&ch->reflock); + mutex_lock(&ch->cdma.lock); - if (ch->refcount) { - mutex_lock(&ch->cdma.lock); + if (show_fifo) + host1x_hw_show_channel_fifo(m, ch, o); - if (show_fifo) - host1x_hw_show_channel_fifo(m, ch, o); + host1x_hw_show_channel_cdma(m, ch, o); - host1x_hw_show_channel_cdma(m, ch, o); - mutex_unlock(&ch->cdma.lock); - } - - mutex_unlock(&ch->reflock); + mutex_unlock(&ch->cdma.lock); return 0; } @@ -94,28 +89,22 @@ static void show_syncpts(struct host1x *m, struct output *o) host1x_debug_output(o, "\n"); } -static void show_all(struct host1x *m, struct output *o) +static void show_all(struct host1x *m, struct output *o, bool show_fifo) { - struct host1x_channel *ch; + int i; host1x_hw_show_mlocks(m, o); show_syncpts(m, o); host1x_debug_output(o, "---- channels ----\n"); - host1x_for_each_channel(m, ch) - show_channels(ch, o, true); -} - -static void show_all_no_fifo(struct host1x *host1x, struct output *o) -{ - struct host1x_channel *ch; - - host1x_hw_show_mlocks(host1x, o); - show_syncpts(host1x, o); - host1x_debug_output(o, "---- channels ----\n"); + for (i = 0; i < m->info->nb_channels; ++i) { + struct host1x_channel *ch = host1x_channel_get_index(m, i); - host1x_for_each_channel(host1x, ch) - show_channels(ch, o, false); + if (ch) { + show_channel(ch, o, show_fifo); + host1x_channel_put(ch); + } + } } static int host1x_debug_show_all(struct seq_file *s, void *unused) @@ -125,7 +114,7 @@ static int host1x_debug_show_all(struct seq_file *s, void *unused) .ctx = s }; - show_all(s->private, &o); + show_all(s->private, &o, true); return 0; } @@ -137,7 +126,7 @@ static int host1x_debug_show(struct seq_file *s, void *unused) .ctx = s }; - show_all_no_fifo(s->private, &o); + show_all(s->private, &o, false); return 0; } @@ -216,7 +205,7 @@ void host1x_debug_dump(struct host1x *host1x) .fn = write_to_printk }; - show_all(host1x, &o); + show_all(host1x, &o, true); } void host1x_debug_dump_syncpts(struct host1x *host1x) diff --git a/drivers/gpu/host1x/dev.c b/drivers/gpu/host1x/dev.c index a8234bb49403..f57ef45f75c9 100644 --- a/drivers/gpu/host1x/dev.c +++ b/drivers/gpu/host1x/dev.c @@ -1,7 +1,7 @@ /* * Tegra host1x driver * - * Copyright (c) 2010-2013, NVIDIA Corporation. + * Copyright (c) 2010-2017, NVIDIA Corporation. * * This program is free software; you can redistribute it and/or modify it * under the terms and conditions of the GNU General Public License, @@ -189,7 +189,8 @@ static int host1x_probe(struct platform_device *pdev) host->iova_end = geometry->aperture_end; } - err = host1x_channel_list_init(host); + err = host1x_channel_list_init(&host->channel_list, + host->info->nb_channels); if (err) { dev_err(&pdev->dev, "failed to initialize channel list\n"); goto fail_detach_device; @@ -247,6 +248,7 @@ static int host1x_remove(struct platform_device *pdev) host1x_intr_deinit(host); host1x_syncpt_deinit(host); clk_disable_unprepare(host->clk); + host1x_channel_list_free(&host->channel_list); if (host->domain) { put_iova_domain(&host->iova); diff --git a/drivers/gpu/host1x/dev.h b/drivers/gpu/host1x/dev.h index 1ee79dbd1882..a558e39bf7f5 100644 --- a/drivers/gpu/host1x/dev.h +++ b/drivers/gpu/host1x/dev.h @@ -1,5 +1,5 @@ /* - * Copyright (C) 2012-2016 NVIDIA CORPORATION. All rights reserved. + * Copyright (C) 2012-2017 NVIDIA CORPORATION. All rights reserved. * * This program is free software; you can redistribute it and/or modify it * under the terms and conditions of the GNU General Public License, @@ -130,10 +130,8 @@ struct host1x { struct host1x_syncpt *nop_sp; struct mutex syncpt_mutex; - struct mutex chlist_mutex; - struct host1x_channel chlist; - unsigned long allocated_channels; - unsigned int num_allocated_channels; + + struct host1x_channel_list channel_list; struct dentry *debugfs; diff --git a/drivers/gpu/host1x/hw/channel_hw.c b/drivers/gpu/host1x/hw/channel_hw.c index 27dc78f4c400..286c81aad308 100644 --- a/drivers/gpu/host1x/hw/channel_hw.c +++ b/drivers/gpu/host1x/hw/channel_hw.c @@ -138,13 +138,13 @@ static int channel_submit(struct host1x_job *job) } /* get submit lock */ - err = mutex_lock_interruptible(&ch->submitlock); + err = mutex_lock_interruptible(&ch->submit_lock); if (err) goto error; completed_waiter = kzalloc(sizeof(*completed_waiter), GFP_KERNEL); if (!completed_waiter) { - mutex_unlock(&ch->submitlock); + mutex_unlock(&ch->submit_lock); err = -ENOMEM; goto error; } @@ -152,7 +152,7 @@ static int channel_submit(struct host1x_job *job) /* begin a CDMA submit */ err = host1x_cdma_begin(&ch->cdma, job); if (err) { - mutex_unlock(&ch->submitlock); + mutex_unlock(&ch->submit_lock); goto error; } @@ -190,7 +190,7 @@ static int channel_submit(struct host1x_job *job) completed_waiter = NULL; WARN(err, "Failed to set submit complete interrupt"); - mutex_unlock(&ch->submitlock); + mutex_unlock(&ch->submit_lock); return 0; @@ -202,10 +202,6 @@ static int channel_submit(struct host1x_job *job) static int host1x_channel_init(struct host1x_channel *ch, struct host1x *dev, unsigned int index) { - ch->id = index; - mutex_init(&ch->reflock); - mutex_init(&ch->submitlock); - ch->regs = dev->regs + index * HOST1X_CHANNEL_SIZE; return 0; } diff --git a/include/linux/host1x.h b/include/linux/host1x.h index fd9b526486e0..921ec6465952 100644 --- a/include/linux/host1x.h +++ b/include/linux/host1x.h @@ -157,7 +157,6 @@ struct host1x_channel; struct host1x_job; struct host1x_channel *host1x_channel_request(struct device *dev); -void host1x_channel_free(struct host1x_channel *channel); struct host1x_channel *host1x_channel_get(struct host1x_channel *channel); void host1x_channel_put(struct host1x_channel *channel); int host1x_job_submit(struct host1x_job *job);
This is largely a rewrite of the Host1x channel allocation code in channel.c, bringing several changes: - The previous code could deadlock due to an interaction between the 'reflock' mutex and CDMA timeout handling. This gets rid of the mutex. - In the future, per-user channel allocation will be useful to have. This paves the way for that by allowing host1x_channel_request to wait for a free channel, and freeing channels when their refcount drops to zero. - Support for more than 32 channels, required for Tegra186 - General refactoring, including better encapsulation of channel ownership handling into channel.c Signed-off-by: Mikko Perttunen <mperttunen@nvidia.com> --- Ran though test suite at github.com/cyndis/host1x_test. Fixes the timeout test there. drivers/gpu/drm/tegra/gr2d.c | 6 +- drivers/gpu/drm/tegra/gr3d.c | 6 +- drivers/gpu/drm/tegra/vic.c | 6 +- drivers/gpu/host1x/cdma.c | 2 +- drivers/gpu/host1x/channel.c | 154 +++++++++++++++++++++++-------------- drivers/gpu/host1x/channel.h | 28 ++++--- drivers/gpu/host1x/debug.c | 49 +++++------- drivers/gpu/host1x/dev.c | 6 +- drivers/gpu/host1x/dev.h | 8 +- drivers/gpu/host1x/hw/channel_hw.c | 12 +-- include/linux/host1x.h | 1 - 11 files changed, 155 insertions(+), 123 deletions(-)