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?
--
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
