Re: [PATCHv4 0/8] Support for Tegra 2D hardware

On 01/02/2013 05:25 PM, Terje Bergström wrote:

On 26.12.2012 11:42, Mark Zhang wrote:

[...]

...

    if (!de)

diff --git a/drivers/gpu/host1x/dev.c b/drivers/gpu/host1x/dev.c index 07e8813..01ed10d 100644 --- a/drivers/gpu/host1x/dev.c +++ b/drivers/gpu/host1x/dev.c @@ -38,6 +38,7 @@

struct host1x *host1x;

+/* Called by drm unlocked ioctl function. So do we need a lock here? */ void host1x_syncpt_incr_byid(u32 id)

No, host1x_syncpt_incr_byid is SMP safe.

Correct. Lock is unnecessary.

...

{ struct host1x_syncpt *sp = host1x->syncpt + id; @@ -52,6 +53,7 @@ u32 host1x_syncpt_read_byid(u32 id) } EXPORT_SYMBOL(host1x_syncpt_read_byid);

+/* Called by drm unlocked ioctl function. So do we need a lock here? */ int host1x_syncpt_wait_byid(u32 id, u32 thresh, long timeout, u32 *value)

Same here, SMP safe.

...

{ struct host1x_syncpt *sp = host1x->syncpt + id; @@ -161,6 +163,8 @@ static int host1x_probe(struct platform_device *dev)

    err = host1x_alloc_resources(host);
    if (err) {

•           /* We don't have chip_ops right now, so here the
  

•              error message is somewhat improper */
            dev_err(&dev->dev, "failed to init chip support\n");
            goto fail;
    }
  

Actually, alloc_resources only allocates intr->syncpt, so I the code to host1x_intr_init().

...

@@ -175,6 +179,14 @@ static int host1x_probe(struct platform_device *dev) if (!host->syncpt) goto fail;

•   /* I suggest create a dedicate function for initializing nop sp.
  

•      First this "_host1x_syncpt_alloc" looks like an internal/static
  

•      function.
  

•      Then actually "host1x_syncpt_alloc" & "_host1x_syncpt_alloc" all
  

•      serve host1x client(e.g: gr2d) so it's not suitable to use them
  

•      for nop sp.
  

•      Just create a wrapper function to call _host1x_syncpt_alloc is OK.
  

•      This will make the code more readable. */
    host->nop_sp = _host1x_syncpt_alloc(host, NULL, 0);
  

_host1x_syncpt_alloc is an internal function, not exported. host1x_syncpt_alloc is exported. I think it's even better if I just move allocation of nop_sp to happen in host1x_syncpt_init.

Agree.

...

    if (!host->nop_sp)
            goto fail;

[...]

...

diff --git a/drivers/gpu/host1x/intr.c b/drivers/gpu/host1x/intr.c index efcb9be..e112001 100644 --- a/drivers/gpu/host1x/intr.c +++ b/drivers/gpu/host1x/intr.c @@ -329,8 +329,13 @@ void host1x_intr_deinit(struct host1x_intr *intr) void host1x_intr_start(struct host1x_intr *intr, u32 hz) { struct host1x *host1x = intr_to_host1x(intr);

•   /* Why we need to lock here? Seems like this function is
  

•      called by host1x's probe only. */
    mutex_lock(&intr->mutex);
  
  

•   /* "init_host_sync" has already been called in function
  

•      host1x_intr_init. Remove this line. */
    host1x->intr_op.init_host_sync(intr);
    host1x->intr_op.set_host_clocks_per_usec(intr,
                    DIV_ROUND_UP(hz, 1000000));
  

In future, we'll call host1x_intr_start() whenever host1x is turned on. Thus we need locking.

init_host_sync() should actually be called from host1x_intr_start(), not _init().

Got it. Thanks for explanation.

...

diff --git a/drivers/gpu/host1x/syncpt.c b/drivers/gpu/host1x/syncpt.c index 07cbca5..9a234ad 100644 --- a/drivers/gpu/host1x/syncpt.c +++ b/drivers/gpu/host1x/syncpt.c @@ -309,6 +309,8 @@ struct host1x_syncpt *host1x_syncpt_init(struct host1x *host) struct host1x_syncpt *syncpt, *sp; int i;

•   /* Consider devm_kzalloc here. Then you can forget the release
  

•      stuffs about this "syncpt". */
    syncpt = sp = kzalloc(sizeof(struct host1x_syncpt) * host->info.nb_pts,
            GFP_KERNEL);
    if (!syncpt)
  

Will do.

Thanks!

Terje