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

[Thierry Reding]

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: pgp00000.pgp
Description: PGP signature