lkml.org 
[lkml]   [2013]   [Jan]   [2]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [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
    >
    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

    \
     
     \ /
      Last update: 2013-01-03 04:01    [W:3.585 / U:27.344 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site