On Mon, May 27, 2013 at 07:19:28PM +0530, Mayuresh Kulkarni wrote:
> - as of now host1x and gr2d module's clocks are enabled
> in their driver's .probe and never disabled
> - this commit adds support for runtime pm in host1x and
> gr2d drivers
> - during boot-up clocks are enabled in .probe and disabled
> on its exit
> - when new work is submitted, clocks are enabled in submit
> and disabled when submit completes
> - parent->child relation between host1x and gr2d ensures
> that host1x clock is also enabled before enabling gr2d clock
> and it is turned off after gr2d clock is turned off
I think this format is a little odd and hard to follow for a commit
message. You can easily turn these into proper sentences and hence make
it much easier to read.
> diff --git a/drivers/gpu/host1x/channel.c b/drivers/gpu/host1x/channel.c
[...]
> @@ -41,6 +42,10 @@ int host1x_job_submit(struct host1x_job *job)
> {
> struct host1x *host = dev_get_drvdata(job->channel->dev->parent);
>
> + /* enable clocks
> + * they will be disabled in action_submit_complete */
> + pm_runtime_get_sync(job->channel->dev);
This comment is misleading. Runtime PM could be used for more than just
enabling clocks. Also it should be following the format defined in the
CodingStyle document, like so:
/*
* Enable clocks...
* ...
*/
> diff --git a/drivers/gpu/host1x/dev.c b/drivers/gpu/host1x/dev.c
[...]
> @@ -143,11 +144,11 @@ static int host1x_probe(struct platform_device *pdev)
> return err;
> }
>
> - err = clk_prepare_enable(host->clk);
> - if (err < 0) {
> - dev_err(&pdev->dev, "failed to enable clock\n");
> - return err;
> - }
> + /* enable runtime pm */
> + pm_runtime_enable(&pdev->dev);
> +
> + /* enable clocks */
> + pm_runtime_get_sync(&pdev->dev);
I think the comments can be dropped. They don't add any useful
information.
> @@ -165,6 +166,9 @@ static int host1x_probe(struct platform_device *pdev)
>
> host1x_drm_alloc(pdev);
>
> + /* disable clocks */
> + pm_runtime_put(&pdev->dev);
Same here.
> +#ifdef CONFIG_PM_RUNTIME
> +static int host1x_runtime_suspend(struct device *dev)
> +{
> + struct host1x *host;
> +
> + host = dev_get_drvdata(dev);
> + if (IS_ERR_OR_NULL(host))
I think a simple
if (!host)
return -EINVAL;
would be enough here. The driver-data of the device should never be an
ERR_PTR()-encoded value, but either a valid pointer to a host1x object
or NULL.
> +#ifdef CONFIG_PM
> +static const struct dev_pm_ops host1x_pm_ops = {
> +#ifdef CONFIG_PM_RUNTIME
> + .runtime_suspend = host1x_runtime_suspend,
> + .runtime_resume = host1x_runtime_resume,
> +#endif
SET_RUNTIME_PM_OPS?
> diff --git a/drivers/gpu/host1x/drm/gr2d.c b/drivers/gpu/host1x/drm/gr2d.c
[...]
Same comments apply here. Also I think it might be a good idea to split
the host1x and gr2d changes into separate patches.
> diff --git a/drivers/gpu/host1x/intr.c b/drivers/gpu/host1x/intr.c
> index 2491bf8..c23fb64 100644
> --- a/drivers/gpu/host1x/intr.c
> +++ b/drivers/gpu/host1x/intr.c
> @@ -20,6 +20,7 @@
> #include <linux/interrupt.h>
> #include <linux/slab.h>
> #include <linux/irq.h>
> +#include <linux/pm_runtime.h>
>
> #include <trace/events/host1x.h>
> #include "channel.h"
> @@ -109,14 +110,18 @@ static void reset_threshold_interrupt(struct host1x *host,
>
> static void action_submit_complete(struct host1x_waitlist *waiter)
> {
> + int completed = waiter->count;
> struct host1x_channel *channel = waiter->data;
>
> + /* disable clocks for all the submits that got completed in this lot */
> + while (completed--)
> + pm_runtime_put(channel->dev);
> +
> host1x_cdma_update(&channel->cdma);
>
> - /* Add nr_completed to trace */
> + /* Add nr_completed to trace */
> trace_host1x_channel_submit_complete(dev_name(channel->dev),
> waiter->count, waiter->thresh);
> -
> }
This feels hackish. But I can't see any better place to do this. Terje,
Arto: any ideas how we can do this in a cleaner way? If there's nothing
better then maybe moving the code into a separate function, say
host1x_waitlist_complete(), might make this less awkward?
Thierry
Attachment:
pgpsWFFi7sOua.pgp
Description: PGP signature
