Re: [PATCH 1/3] gpu: host1x: Add syncpoint base support

[Thierry Reding]

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: pgp00000.pgp
Description: PGP signature