The host1x driver uses currently syncpoints statically from host1x point of
view. If we do a wait inside a job, it always has a constant value to wait.
host1x supports also doing relative syncpoint waits with respect to syncpoint
bases. This allows doing multiple operations inside a single submit and
waiting an operation to complete before moving to next one.
This set of patches adds support for syncpoint bases to host1x driver and
enables the support for gr2d client. The series is based on 3.12-rc4.
I have tested the series using the host1x test application (available at [0],
function test_wait_base() in tests/tegra/host1x/tegra_host1x_test.c) on cardhu.
I would appreciate help in reviewing the series and testing the patches
on other boards.
[0] https://gitorious.org/linux-host1x/libdrm-host1x
Arto Merilainen (3):
gpu: host1x: Add syncpoint base support
drm/tegra: Deliver syncpoint base to user space
drm/tegra: Reserve base for gr2d
drivers/gpu/host1x/dev.h | 2 ++
drivers/gpu/host1x/drm/drm.c | 2 ++
drivers/gpu/host1x/drm/gr2d.c | 2 +-
drivers/gpu/host1x/hw/channel_hw.c | 19 +++++++++++
drivers/gpu/host1x/hw/hw_host1x01_uclass.h | 6 ++++
drivers/gpu/host1x/syncpt.c | 55 +++++++++++++++++++++++++++---
drivers/gpu/host1x/syncpt.h | 10 ++++++
include/uapi/drm/tegra_drm.h | 4 ++-
8 files changed, 93 insertions(+), 7 deletions(-)
--
1.8.1.5
This patch makes the necessary additions to deliver syncpoint base
to the user space.
This patch splits the index field in the drm_tegra_get_syncpt structure
into three separate fields (index, support_base, base_id). This allows
to keep compatibility over kernel versions:
- The placing of index field stays constant and therefore the interface
should stay compatible with earlier userspace versions.
- The old index field was input-only and it was not supposed to be read
afterwards. Delivering data back in the same field should not introduce
regressions to any old user space applications.
- Old kernels left support_base and base_id fields intact (zero) and
hence the new user space applications get the correct response from
the kernel (=syncpoint does not have a base).
Signed-off-by: Arto Merilainen <[email protected]>
---
drivers/gpu/host1x/drm/drm.c | 2 ++
include/uapi/drm/tegra_drm.h | 4 +++-
2 files changed, 5 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/host1x/drm/drm.c b/drivers/gpu/host1x/drm/drm.c
index 8c61cee..4251563 100644
--- a/drivers/gpu/host1x/drm/drm.c
+++ b/drivers/gpu/host1x/drm/drm.c
@@ -468,6 +468,8 @@ static int tegra_get_syncpt(struct drm_device *drm, void *data,
syncpt = context->client->syncpts[args->index];
args->id = host1x_syncpt_id(syncpt);
+ args->support_base = syncpt->base ? 1 : 0;
+ args->base_id = syncpt->base ? syncpt->base->id : 0;
return 0;
}
diff --git a/include/uapi/drm/tegra_drm.h b/include/uapi/drm/tegra_drm.h
index 73bde4e..7a79ca5 100644
--- a/include/uapi/drm/tegra_drm.h
+++ b/include/uapi/drm/tegra_drm.h
@@ -61,7 +61,9 @@ struct drm_tegra_close_channel {
struct drm_tegra_get_syncpt {
__u64 context;
- __u32 index;
+ __u16 index;
+ __u8 support_base;
+ __u8 base_id;
__u32 id;
};
--
1.8.1.5
This patch modifies the gr2d to reserve a base for syncpoint.
Signed-off-by: Arto Merilainen <[email protected]>
---
drivers/gpu/host1x/drm/gr2d.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/host1x/drm/gr2d.c b/drivers/gpu/host1x/drm/gr2d.c
index 27ffcf1..f0c3fd5 100644
--- a/drivers/gpu/host1x/drm/gr2d.c
+++ b/drivers/gpu/host1x/drm/gr2d.c
@@ -285,7 +285,7 @@ static int gr2d_probe(struct platform_device *pdev)
if (!gr2d->channel)
return -ENOMEM;
- *syncpts = host1x_syncpt_request(dev, false);
+ *syncpts = host1x_syncpt_request_based(dev, false);
if (!(*syncpts)) {
host1x_channel_free(gr2d->channel);
return -ENOMEM;
--
1.8.1.5
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.
Signed-off-by: Arto Merilainen <[email protected]>
---
drivers/gpu/host1x/dev.h | 2 ++
drivers/gpu/host1x/hw/channel_hw.c | 19 +++++++++++
drivers/gpu/host1x/hw/hw_host1x01_uclass.h | 6 ++++
drivers/gpu/host1x/syncpt.c | 55 +++++++++++++++++++++++++++---
drivers/gpu/host1x/syncpt.h | 10 ++++++
5 files changed, 87 insertions(+), 5 deletions(-)
diff --git a/drivers/gpu/host1x/dev.h b/drivers/gpu/host1x/dev.h
index bed90a8..89a3c1e 100644
--- 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;
struct host1x_syncpt;
struct host1x_channel;
struct host1x_cdma;
@@ -102,6 +103,7 @@ struct host1x {
void __iomem *regs;
struct host1x_syncpt *syncpt;
+ struct host1x_base *bases;
struct device *dev;
struct clk *clk;
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));
+}
+
static int channel_submit(struct host1x_job *job)
{
struct host1x_channel *ch = job->channel;
@@ -118,6 +133,10 @@ static int channel_submit(struct host1x_job *job)
host1x_syncpt_read_max(sp)));
}
+ /* Synchronize base register to allow using it for relative waiting */
+ if (sp->base)
+ synchronize_syncpt_base(job);
+
syncval = host1x_syncpt_incr_max(sp, user_syncpt_incrs);
job->syncpt_end = syncval;
diff --git a/drivers/gpu/host1x/hw/hw_host1x01_uclass.h b/drivers/gpu/host1x/hw/hw_host1x01_uclass.h
index 42f3ce1..f755359 100644
--- a/drivers/gpu/host1x/hw/hw_host1x01_uclass.h
+++ b/drivers/gpu/host1x/hw/hw_host1x01_uclass.h
@@ -111,6 +111,12 @@ static inline u32 host1x_uclass_wait_syncpt_base_offset_f(u32 v)
}
#define HOST1X_UCLASS_WAIT_SYNCPT_BASE_OFFSET_F(v) \
host1x_uclass_wait_syncpt_base_offset_f(v)
+static inline u32 host1x_uclass_load_syncpt_base_r(void)
+{
+ return 0xb;
+}
+#define HOST1X_UCLASS_LOAD_SYNCPT_BASE \
+ host1x_uclass_load_syncpt_base_r()
static inline u32 host1x_uclass_load_syncpt_base_base_indx_f(u32 v)
{
return (v & 0xff) << 24;
diff --git a/drivers/gpu/host1x/syncpt.c b/drivers/gpu/host1x/syncpt.c
index 409745b..48927b1 100644
--- a/drivers/gpu/host1x/syncpt.c
+++ b/drivers/gpu/host1x/syncpt.c
@@ -30,9 +30,32 @@
#define SYNCPT_CHECK_PERIOD (2 * HZ)
#define MAX_STUCK_CHECK_COUNT 15
+static struct host1x_base *host1x_base_alloc(struct host1x *host)
+{
+ struct host1x_base *base = host->bases;
+ int i;
+
+ for (i = 0; i < host->info->nb_bases && base->reserved; i++, base++)
+ ;
+
+ if (i >= host->info->nb_bases)
+ return NULL;
+
+ base->reserved = true;
+ return base;
+}
+
+static void host1x_base_free(struct host1x_base *base)
+{
+ if (!base)
+ return;
+ 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)
{
int i;
struct host1x_syncpt *sp = host->syncpt;
@@ -44,6 +67,12 @@ static struct host1x_syncpt *_host1x_syncpt_alloc(struct host1x *host,
if (i >= host->info->nb_pts)
return NULL;
+ if (support_base) {
+ sp->base = host1x_base_alloc(host);
+ if (!sp->base)
+ return NULL;
+ }
+
name = kasprintf(GFP_KERNEL, "%02d-%s", sp->id,
dev ? dev_name(dev) : NULL);
if (!name)
@@ -304,24 +333,31 @@ int host1x_syncpt_patch_wait(struct host1x_syncpt *sp, void *patch_addr)
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);
- 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;
}
host->syncpt = syncpt;
+ host->bases = bases;
host1x_syncpt_restore(host);
/* Allocate sync point to use for clearing waits for expired fences */
- host->nop_sp = _host1x_syncpt_alloc(host, NULL, false);
+ host->nop_sp = _host1x_syncpt_alloc(host, NULL, false, false);
if (!host->nop_sp)
return -ENOMEM;
@@ -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);
}
void host1x_syncpt_free(struct host1x_syncpt *sp)
@@ -340,7 +383,9 @@ void host1x_syncpt_free(struct host1x_syncpt *sp)
if (!sp)
return;
+ host1x_base_free(sp->base);
kfree(sp->name);
+ sp->base = NULL;
sp->dev = NULL;
sp->name = NULL;
sp->client_managed = false;
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;
+};
+
struct host1x_syncpt {
int id;
atomic_t min_val;
@@ -39,6 +44,7 @@ struct host1x_syncpt {
bool client_managed;
struct host1x *host;
struct device *dev;
+ struct host1x_base *base;
/* interrupt data */
struct host1x_syncpt_intr intr;
@@ -156,6 +162,10 @@ u32 host1x_syncpt_id(struct host1x_syncpt *sp);
struct host1x_syncpt *host1x_syncpt_request(struct device *dev,
bool client_managed);
+/* Allocate a sync point and a base for a device */
+struct host1x_syncpt *host1x_syncpt_request_based(struct device *dev,
+ bool client_managed);
+
/* Free a sync point. */
void host1x_syncpt_free(struct host1x_syncpt *sp);
--
1.8.1.5
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
On Wed, Oct 09, 2013 at 02:54:09PM +0300, Arto Merilainen wrote:
> This patch makes the necessary additions to deliver syncpoint base
> to the user space.
>
> This patch splits the index field in the drm_tegra_get_syncpt structure
> into three separate fields (index, support_base, base_id). This allows
> to keep compatibility over kernel versions:
> - The placing of index field stays constant and therefore the interface
> should stay compatible with earlier userspace versions.
> - The old index field was input-only and it was not supposed to be read
> afterwards. Delivering data back in the same field should not introduce
> regressions to any old user space applications.
> - Old kernels left support_base and base_id fields intact (zero) and
> hence the new user space applications get the correct response from
> the kernel (=syncpoint does not have a base).
>
> Signed-off-by: Arto Merilainen <[email protected]>
> ---
> drivers/gpu/host1x/drm/drm.c | 2 ++
> include/uapi/drm/tegra_drm.h | 4 +++-
> 2 files changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/host1x/drm/drm.c b/drivers/gpu/host1x/drm/drm.c
> index 8c61cee..4251563 100644
> --- a/drivers/gpu/host1x/drm/drm.c
> +++ b/drivers/gpu/host1x/drm/drm.c
> @@ -468,6 +468,8 @@ static int tegra_get_syncpt(struct drm_device *drm, void *data,
>
> syncpt = context->client->syncpts[args->index];
> args->id = host1x_syncpt_id(syncpt);
> + args->support_base = syncpt->base ? 1 : 0;
> + args->base_id = syncpt->base ? syncpt->base->id : 0;
>
> return 0;
> }
> diff --git a/include/uapi/drm/tegra_drm.h b/include/uapi/drm/tegra_drm.h
> index 73bde4e..7a79ca5 100644
> --- a/include/uapi/drm/tegra_drm.h
> +++ b/include/uapi/drm/tegra_drm.h
> @@ -61,7 +61,9 @@ struct drm_tegra_close_channel {
>
> struct drm_tegra_get_syncpt {
> __u64 context;
> - __u32 index;
> + __u16 index;
> + __u8 support_base;
> + __u8 base_id;
> __u32 id;
I don't like this. Can't we just add a separate IOCTL? Something like:
struct drm_tegra_get_syncpt_base {
__u64 context;
__u32 syncpt;
__u32 base;
};
Which could be used somewhat like this:
struct drm_tegra_get_syncpt_base args;
memset(&args, 0, sizeof(args));
args.context = context;
args.syncpt = syncpt;
err = ioctl(fd, DRM_IOCTL_TEGRA_GET_SYNCPT_BASE, &args);
if (err < 0) {
/*
* perhaps errno == ENOTSUP if the syncpoint has no
* associated base?
*/
}
base = args.base;
Thierry
On Wed, Oct 09, 2013 at 02:54:07PM +0300, Arto Merilainen wrote:
> The host1x driver uses currently syncpoints statically from host1x point of
> view. If we do a wait inside a job, it always has a constant value to wait.
> host1x supports also doing relative syncpoint waits with respect to syncpoint
> bases. This allows doing multiple operations inside a single submit and
> waiting an operation to complete before moving to next one.
>
> This set of patches adds support for syncpoint bases to host1x driver and
> enables the support for gr2d client. The series is based on 3.12-rc4.
>
> I have tested the series using the host1x test application (available at [0],
> function test_wait_base() in tests/tegra/host1x/tegra_host1x_test.c) on cardhu.
> I would appreciate help in reviewing the series and testing the patches
> on other boards.
>
> [0] https://gitorious.org/linux-host1x/libdrm-host1x
>
> Arto Merilainen (3):
> gpu: host1x: Add syncpoint base support
> drm/tegra: Deliver syncpoint base to user space
> drm/tegra: Reserve base for gr2d
>
> drivers/gpu/host1x/dev.h | 2 ++
> drivers/gpu/host1x/drm/drm.c | 2 ++
> drivers/gpu/host1x/drm/gr2d.c | 2 +-
> drivers/gpu/host1x/hw/channel_hw.c | 19 +++++++++++
> drivers/gpu/host1x/hw/hw_host1x01_uclass.h | 6 ++++
> drivers/gpu/host1x/syncpt.c | 55 +++++++++++++++++++++++++++---
> drivers/gpu/host1x/syncpt.h | 10 ++++++
> include/uapi/drm/tegra_drm.h | 4 ++-
> 8 files changed, 93 insertions(+), 7 deletions(-)
Generally this looks pretty good. It does conflict with some of the
changes in my tree, but I think I can fix that up when applying.
Thanks,
Thierry
On 10/11/2013 12:43 PM, Thierry Reding wrote:
> * PGP Signed by an unknown key
>
> On Wed, Oct 09, 2013 at 02:54:09PM +0300, Arto Merilainen wrote:
>> This patch makes the necessary additions to deliver syncpoint base
>> to the user space.
>>
>> This patch splits the index field in the drm_tegra_get_syncpt structure
>> into three separate fields (index, support_base, base_id). This allows
>> to keep compatibility over kernel versions:
>> - The placing of index field stays constant and therefore the interface
>> should stay compatible with earlier userspace versions.
>> - The old index field was input-only and it was not supposed to be read
>> afterwards. Delivering data back in the same field should not introduce
>> regressions to any old user space applications.
>> - Old kernels left support_base and base_id fields intact (zero) and
>> hence the new user space applications get the correct response from
>> the kernel (=syncpoint does not have a base).
>>
>> Signed-off-by: Arto Merilainen <[email protected]>
>> ---
>> drivers/gpu/host1x/drm/drm.c | 2 ++
>> include/uapi/drm/tegra_drm.h | 4 +++-
>> 2 files changed, 5 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/host1x/drm/drm.c b/drivers/gpu/host1x/drm/drm.c
>> index 8c61cee..4251563 100644
>> --- a/drivers/gpu/host1x/drm/drm.c
>> +++ b/drivers/gpu/host1x/drm/drm.c
>> @@ -468,6 +468,8 @@ static int tegra_get_syncpt(struct drm_device *drm, void *data,
>>
>> syncpt = context->client->syncpts[args->index];
>> args->id = host1x_syncpt_id(syncpt);
>> + args->support_base = syncpt->base ? 1 : 0;
>> + args->base_id = syncpt->base ? syncpt->base->id : 0;
>>
>> return 0;
>> }
>> diff --git a/include/uapi/drm/tegra_drm.h b/include/uapi/drm/tegra_drm.h
>> index 73bde4e..7a79ca5 100644
>> --- a/include/uapi/drm/tegra_drm.h
>> +++ b/include/uapi/drm/tegra_drm.h
>> @@ -61,7 +61,9 @@ struct drm_tegra_close_channel {
>>
>> struct drm_tegra_get_syncpt {
>> __u64 context;
>> - __u32 index;
>> + __u16 index;
>> + __u8 support_base;
>> + __u8 base_id;
>> __u32 id;
>
> I don't like this. Can't we just add a separate IOCTL? Something like:
Sure. This felt like a good idea, but I must admit I had a bit mixed
feelings after looking my own explanation why this would be ok...
- Arto
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
>