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

[Mayuresh Kulkarni <mkulkarni-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>]

On Tuesday 28 May 2013 02:40 PM, Thierry Reding wrote:
> * PGP Signed by an unknown key
>
> On Tue, May 28, 2013 at 08:45:03AM +0300, Terje Bergström wrote:
>> On 27.05.2013 18:45, Thierry Reding wrote:
>>> On Mon, May 27, 2013 at 07:19:28PM +0530, Mayuresh Kulkarni wrote:
>>>> +#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.
>>
>> True, we should avoid IS_ERR_OR_NULL() like plague. We always know if
>> the called API returns a NULL on error or an error code. In case of
>> error code we should just propagate that.
>
> Yes, that's the case in general. In this specific case the value
> obtained by dev_get_drvdata() should either be a valid pointer or NULL,
> never an error code. We can easily make sure by only setting the data
> (using platform_set_drvdata()) when the pointer is valid.
>
> Thinking about it some more, I don't think we can ever get NULL here. A
> device's .runtime_suspend() cannot be called when the device has been
> removed, right? That's the only case where the value returned might be
> NULL. It would be NULL too if host1x wasn't initialized yet, but that's
> already dealt with by the proper ordering in .probe().
>
>>> Same comments apply here. Also I think it might be a good idea to split
>>> the host1x and gr2d changes into separate patches.
>>
>> That's a bit tricky, but doable. We just need to enable it for 2D first,
>> and then host1x to keep bisectability.
>
> Right, there's a dependency. But I'd still prefer to have them separate.
> Unless it gets really messy.
>
>>>>   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?
>>
>> Yeah, it's a bit awkward. action_submit_complete() actually does handle
>> completion of multiple jobs, and we do one pm_runtime_get() per job.
>>
>> We could do pm_runtime_put() in host1x_cdma_update(). It anyway goes
>> through each job that is completed, so while freeing the job it could as
>> well call runtime PM. That way we could even remove the waiter->count
>> variable altogether as it's not needed anymore.
>
> That sounds a lot better. We could add a helper (host1x_job_finish()
> perhaps) with the following from update_cdma_locked():
>
> 	/* Unpin the memory */
> 	host1x_job_unpin(job);
>
> 	/* Pop push buffer slots */
> 	if (job->num_slots) {
> 		struct push_buffer *pb = &cdma->push_buffer;
> 		host1x_pushbuffer_pop(pb, job->num_slots);
> 		if (cdma->event == CDMA_EVENT_PUSH_BUFFER_SPACE)
> 			signal = true;
> 	}
>
> 	list_del(&job->list);
>
> And add pm_runtime_put() (as well as potentially other stuff) in there.
> That'll prevent update_cdma_unlocked() from growing too much. It isn't
> too bad right now, so maybe a helper isn't warranted yet, but I don't
> think it'll hurt.
>
>> The not-so-beautiful aspect is that we do pm_runtime_get() in
>> host1x_channel.c and pm_runtime_put() in host1x_cdma.c. For code
>> readability it's be great to have them in the same file. I actually get
>> questions every now and then because in downstream because of doing
>> these operations in different files.
>
> With the above helper in place, we could move host1x_job_submit() to
> job.c instead and have all the code in one file.
>
> Thierry
>
> * Unknown Key
> * 0x7F3EB3A1
>

In downstream, we have 2 APIs which are wrapper over runtime PM calls. 
We call those from _submit and job complete.

I wonder if we should follow the same here?