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.
> 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.
>> 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.
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.
Terje
--
To unsubscribe from this list: send the line "unsubscribe linux-tegra" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
