Re: [PATCH] drm/tegra: add support for runtime pm

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