On 10/11/2013 12:39 PM, Thierry Reding wrote:
> * PGP Signed by an unknown key
>
> 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.
I agree.
>
>> 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.
Will fix.
>
>> 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
Ops. Will fix.
>
>> +
>> + 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;
> }
I agree, that looks less obfuscated.
>
>> +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);
>
This is not a bad idea... I will write a patch for that.
>> 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...
Oh. Will fix.
>
>> - 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.
>
Will fix.
>> @@ -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.
>
Will fix. (my original idea was to avoid changes in the interface but it
is likely just a minor inconvenience..)
>> 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.
Sounds good.
- Arto
>
> Thierry
>
> * Unknown Key
> * 0x7F3EB3A1
>