On Wed, Oct 09, 2013 at 02:54:08PM +0300, Arto Merilainen wrote:
> This patch adds support for hardware syncpoint bases. This creates
> a simple mechanism for waiting an operation to complete in the middle
> of the command buffer.
Perhaps "... simple mechanism to stall the command FIFO until an
operation is completed." That's what the TRM contains and more
accurately describes the hardware functionality.
> diff --git a/drivers/gpu/host1x/dev.h b/drivers/gpu/host1x/dev.h
[...]
> @@ -26,6 +26,7 @@
> #include "cdma.h"
> #include "job.h"
>
> +struct host1x_base;
host1x_syncpt_base is more explicit, host1x_base sounds very generic.
> diff --git a/drivers/gpu/host1x/hw/channel_hw.c b/drivers/gpu/host1x/hw/channel_hw.c
> index ee19962..5f9f735 100644
> --- a/drivers/gpu/host1x/hw/channel_hw.c
> +++ b/drivers/gpu/host1x/hw/channel_hw.c
> @@ -67,6 +67,21 @@ static void submit_gathers(struct host1x_job *job)
> }
> }
>
> +static inline void synchronize_syncpt_base(struct host1x_job *job)
> +{
> + struct host1x_channel *ch = job->channel;
> + struct host1x *host = dev_get_drvdata(ch->dev->parent);
> + struct host1x_syncpt *sp = host->syncpt + job->syncpt_id;
> + u32 base_id = sp->base->id;
> + u32 base_val = host1x_syncpt_read_max(sp);
> +
> + host1x_cdma_push(&ch->cdma,
> + host1x_opcode_setclass(HOST1X_CLASS_HOST1X,
> + host1x_uclass_load_syncpt_base_r(), 1),
> + host1x_uclass_load_syncpt_base_base_indx_f(base_id) |
> + host1x_uclass_load_syncpt_base_value_f(base_val));
Please use the all-caps version of the register definitions. The
lowercase variants were only introduced to allow profiling and coverage
testing, which I think nobody's been doing and I'm in fact thinking
about removing them.
> diff --git a/drivers/gpu/host1x/syncpt.c b/drivers/gpu/host1x/syncpt.c
[...]
> +static struct host1x_base *host1x_base_alloc(struct host1x *host)
> +{
> + struct host1x_base *base = host->bases;
> + int i;
unsigned int
> +
> + for (i = 0; i < host->info->nb_bases && base->reserved; i++, base++)
> + ;
I'd like to see this rewritten as:
for (i = 0; i < host->info->nb_bases; i++) {
if (!host->bases[i].reserved)
break;
}
> +static void host1x_base_free(struct host1x_base *base)
> +{
> + if (!base)
> + return;
> + base->reserved = false;
> +}
The following would be somewhat shorter:
if (base)
base->reserved = false;
> static struct host1x_syncpt *_host1x_syncpt_alloc(struct host1x *host,
> struct device *dev,
> - bool client_managed)
> + bool client_managed,
> + bool support_base)
I think at this point we probably want to introduce a flags argument
instead of adding all those boolean parameters. Something like:
#define HOST1X_SYNCPT_CLIENT_MANAGED (1 << 0)
#define HOST1X_SYNCPT_HAS_BASE (1 << 1)
struct host1x_syncpt *host1x_syncpt_alloc(struct host1x *host,
struct device *dev,
unsigned long flags);
> int host1x_syncpt_init(struct host1x *host)
> {
> struct host1x_syncpt *syncpt;
> + struct host1x_base *bases;
> int i;
>
> + bases = devm_kzalloc(host->dev, sizeof(*bases) * host->info->nb_bases,
> + GFP_KERNEL);
> syncpt = devm_kzalloc(host->dev, sizeof(*syncpt) * host->info->nb_pts,
> GFP_KERNEL);
I'd prefer these to be checked separately. Also the argument alignment
is wrong here. Align with the first function argument. And for
consistency, allocate bases after syncpoints...
> - if (!syncpt)
> + if (!syncpt || !bases)
> return -ENOMEM;
>
> - for (i = 0; i < host->info->nb_pts; ++i) {
> + for (i = 0; i < host->info->nb_bases; i++)
> + bases[i].id = i;
> +
> + for (i = 0; i < host->info->nb_pts; i++) {
> syncpt[i].id = i;
> syncpt[i].host = host;
> }
... and initialize them after the syncpoints...
>
> host->syncpt = syncpt;
> + host->bases = bases;
... to match the assignment order.
> @@ -332,7 +368,14 @@ struct host1x_syncpt *host1x_syncpt_request(struct device *dev,
> bool client_managed)
> {
> struct host1x *host = dev_get_drvdata(dev->parent);
> - return _host1x_syncpt_alloc(host, dev, client_managed);
> + return _host1x_syncpt_alloc(host, dev, client_managed, false);
> +}
> +
> +struct host1x_syncpt *host1x_syncpt_request_based(struct device *dev,
> + bool client_managed)
> +{
> + struct host1x *host = dev_get_drvdata(dev->parent);
> + return _host1x_syncpt_alloc(host, dev, client_managed, true);
> }
If we add a flags parameter to host1x_syncpt_request() (and
host1x_syncpt_alloc()) then we don't need the separate function.
> diff --git a/drivers/gpu/host1x/syncpt.h b/drivers/gpu/host1x/syncpt.h
> index 267c0b9..852dc76 100644
> --- a/drivers/gpu/host1x/syncpt.h
> +++ b/drivers/gpu/host1x/syncpt.h
> @@ -30,6 +30,11 @@ struct host1x;
> /* Reserved for replacing an expired wait with a NOP */
> #define HOST1X_SYNCPT_RESERVED 0
>
> +struct host1x_base {
> + u8 id;
> + bool reserved;
Perhaps name this to something like "requested". "reserved" makes it
sound like it's reserved for some special use.
Thierry
Attachment:
pgpCcp4AYELcN.pgp
Description: PGP signature
