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

On 26.12.2012 11:42, Mark Zhang wrote:

diff --git a/drivers/gpu/drm/tegra/gr2d.c b/drivers/gpu/drm/tegra/gr2d.c index a936902..28954b3 100644 --- a/drivers/gpu/drm/tegra/gr2d.c +++ b/drivers/gpu/drm/tegra/gr2d.c @@ -247,6 +247,10 @@ static int __devinit gr2d_probe(struct platform_device *dev) { int err; struct gr2d *gr2d = NULL;

•   /* Cause drm_device is created in host1x driver. So
  

•      if host1x drivers loads after tegradrm, then in this
  

•      gr2d_probe function, this "drm_device" will be NULL.
  

•      How can we handle this? Defer driver probe? */
  

I will push a new proposal about how the devices & drivers get probed.

    struct platform_device *drm_device =
            host1x_drm_device(to_platform_device(dev->dev.parent));
    struct tegradrm *tegradrm = platform_get_drvdata(drm_device);

@@ -273,6 +277,11 @@ static int __devinit gr2d_probe(struct platform_device *dev)

    gr2d->syncpt = host1x_syncpt_alloc(dev, 0);
    if (!gr2d->syncpt) {

•           /* Do we really need this?
  

•              After "host1x_channel_alloc", the refcount of this
  

•              channel is 0. So calling host1x_channel_put here will
  

•              make the refcount going to negative.
  

•              I suppose we should call "host1x_free_channel" here. */
  

True. I need to also export host1x_channel_free (I will change the name, too).

            host1x_channel_put(gr2d->channel);
            return -ENOMEM;
    }

@@ -284,6 +293,7 @@ static int __devinit gr2d_probe(struct platform_device *dev)

    err = tegra_drm_register_client(tegradrm, &gr2d->client);
    if (err)

•           /* Add "host1x_free_channel" */
  

Will do.

            return err;

    platform_set_drvdata(dev, gr2d);

diff --git a/drivers/gpu/host1x/channel.c b/drivers/gpu/host1x/channel.c index 3705cae..ed83b9e 100644 --- a/drivers/gpu/host1x/channel.c +++ b/drivers/gpu/host1x/channel.c @@ -95,6 +95,9 @@ struct host1x_channel *host1x_channel_alloc(struct platform_device *pdev) int max_channels = host1x->info.nb_channels; int err;

•   /* Is it necessary to add mutex protection here?
  

•      I'm just wondering in a smp system, multiple host1x clients
  

•      may try to alloc their channels simultaneously... */
  

I'll add locking.

    if (chindex > max_channels)
            return NULL;

diff --git a/drivers/gpu/host1x/debug.c b/drivers/gpu/host1x/debug.c index 86d5c70..f2bd5aa 100644 --- a/drivers/gpu/host1x/debug.c +++ b/drivers/gpu/host1x/debug.c @@ -164,6 +164,10 @@ static const struct file_operations host1x_debug_fops = {

void host1x_debug_init(struct host1x *host1x) {

•   /* I think it's better to set this directory name the same with
  

•      the driver's name -- defined in dev.c:
  

•      #define DRIVER_NAME  "tegra-host1x"
  

•      */
    struct dentry *de = debugfs_create_dir("tegra_host", NULL);
  

Will do.

    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.

{ 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.

    if (!host->nop_sp)
            goto fail;

@@ -191,6 +203,7 @@ static int host1x_probe(struct platform_device *dev) }

    err = clk_prepare_enable(host->clk);

•   /* Add a dev_err here */
    if (err < 0)
            goto fail;
  
  

@@ -209,9 +222,12 @@ static int host1x_probe(struct platform_device *dev) return 0;

fail:

•   /* host1x->syncpt isn't released here... */
  

•   /* host1x->intr isn't release here... */
  

•   /* Remove debugfs stuffs? */
    host1x_syncpt_free(host->nop_sp);
    host1x_unregister_drm_device(host);
  

•   kfree(host);
  

•   kfree(host); /* not necessary*/
    return err;
  

}

I added a few other dev_err()'s and checked the deinitialization on error, too.

@@ -222,6 +238,7 @@ static int __exit host1x_remove(struct platform_device *dev) host1x_syncpt_deinit(host); host1x_unregister_drm_device(host); clk_disable_unprepare(host->clk);

•   /* Remove debugfs stuffs? */
    return 0;
  

}

Will do.

diff --git a/drivers/gpu/host1x/dev.h b/drivers/gpu/host1x/dev.h index 05f8544..58f4c71 100644 --- a/drivers/gpu/host1x/dev.h +++ b/drivers/gpu/host1x/dev.h @@ -36,6 +36,7 @@ struct platform_device; struct output;

struct host1x_channel_ops {

•   /* Seems no one uses this member. Remove it. */
    const char *soc_name;
    int (*init)(struct host1x_channel *,
                struct host1x *,
  

@@ -125,12 +126,18 @@ struct host1x { struct host1x_syncpt *syncpt; struct host1x_intr intr; struct platform_device *dev;

•   /* Seems no one uses this. Remove it. */
    atomic_t clientid;
    struct host1x_device_info info;
    struct clk *clk;
  
  

•   /* Put some comments for this member.
  

•      For guys who're not familiar with nop sp, I think they'll
  

•      definitely get confused about this. */
    struct host1x_syncpt *nop_sp;
  
  

•   /* Seems no one uses this member. Remove it. */
    const char *soc_name;
    struct host1x_channel_ops channel_op;
    struct host1x_cdma_ops cdma_op;
  

@@ -140,6 +147,7 @@ struct host1x { struct host1x_intr_ops intr_op;

    struct host1x_channel chlist;

•   int allocated_channels;
  
    struct dentry *debugfs;
  

I removed the extra entries, and I added a short comment about nop_sp.

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().

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