Message-ID: <20130610204316.GC26036@mithrandir>
Date:	Mon, 10 Jun 2013 22:43:17 +0200
From:	Thierry Reding <thierry.reding@...il.com>
To:	Mayuresh Kulkarni <mkulkarni@...dia.com>
Cc:	Terje Bergstrom <tbergstrom@...dia.com>,
	Arto Merilainen <amerilainen@...dia.com>,
	"airlied@...hat.com" <airlied@...hat.com>,
	"dri-devel@...ts.freedesktop.org" <dri-devel@...ts.freedesktop.org>,
	"linux-tegra@...r.kernel.org" <linux-tegra@...r.kernel.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] drm/tegra: add support for runtime pm

On Tue, Jun 04, 2013 at 02:11:27PM +0530, Mayuresh Kulkarni wrote:
> 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?

Can you post the corresponding wrappers to make it easier to discuss
them? If they just wrap runtime PM calls then they don't solve the
locality problems that Terje brought up.

Thierry

Content of type "application/pgp-signature" skipped

Powered by blists - more mailing lists