Re: [RESEND PATCH v2] remoteproc: qcom: Add venus rproc support on msm8996 platform.

[Bjorn Andersson <bjorn.andersson@linaro.org>]

On Fri 16 Dec 16:56 PST 2016, Stephen Boyd wrote:

> On 12/12, Bjorn Andersson wrote:
> > On Wed 30 Nov 01:22 PST 2016, Stanimir Varbanov wrote:
> > [..]
> > 
> > > I think using power domain in remoteproc driver will break power
> > > management of the v4l2 venus driver. Presently I use runtime pm to
> > > switch ON/OFF GDSC and enable/disable Venus clocks.
> > > 
> > > When the .probe of venus remoteproc driver is called the power domain
> > > will become ON and never switched OFF (except when platform driver
> > > .remove method is called). One possible solution would be to add runtime
> > > pm in venus remoreproc driver.
> > > 
> > > IMO we should think more about that.
> > > 
> > > Bjorn, Stephen what are your opinions?
> > > 
> > 
> > Sorry for not getting back to you on this after our earlier discussion
> > on the topic.
> > 
> > Modelling the remoteproc and codec drivers as completely separate
> > entities (with only the rproc phandle) gives us issues with the timing
> > between the two. As I pulled down and started playing with [1] I noticed
> > that I was not able to get the codec driver to probe unless I compiled
> > it as a module which I insmoded after my /lib/firmware became available.
> > 
> > In addition to this, there's no way for the remoteproc driver to inform
> > the codec driver when it's actually shutting down and booting back up
> > during a crash recovery (an async operation).
> > 
> > And lastly when I talked to Rob about the DT binding the obvious
> > question of why one piece of hardware have two disjunct nodes in the DT.
> > 
> > 
> > So I believe we should represent the codec driver as a child of the
> > remoteproc driver, utilizing the newly introduced "remoteproc
> > subdevices" to probe and remove the codec driver based on the state of
> > the remoteproc driver. This relationship will also allow us to tie
> > certain resources (e.g. the clocks and power-domain) to the remoteproc
> > driver and use pm_runtime in either driver to make sure the resources
> > are enabled appropriately.
> > 
> > I did backport some patches from my v4.10 remoteproc pull request into
> > [2] and merged this into [1] and made a prototype of this. You can find
> > it here [3], I did test that I can boot the remoteproc and run
> > v4l2-compliance, shut it down, boot it back up and re-run
> > v4l2-compliance, and it seems to work fine.
> > 
> 
> From a DT perspective this seems backwards. We shouldn't be
> putting something with a reg property underneath a "virtual" node
> that doesn't have a reg property. It's already causing pain,
> evident by these sorts of patches, so something seems wrong.
> 

I agree.

> Are we gaining anything by having a remoteproc driver and device
> for the video hardware here?

After discussing the matter with Avaneesh I realized that the downstream
driver powers the venus core up and down based on the presence of
clients. I expect that this means we have to do the same to be able to
meet our power KPIs.

With this in mind the remoteproc driver becomes a wrapper for a mdt
loader and the scm pas interface - and brings with it a bunch of other
things, expectations and challenges.

> Why can't the video driver load/unload firmware with the mdt loader
> code by itself when it wants to boot the processor?

Providing a saner api around the mdt-loader and pas would allow venus
and Adreno to reuse the code, without the remoteproc bloat.

> That seems like a much simpler design and it nicely avoids this DT
> confusion.
> 

Agreed.

I was under the impression that the venus core was always on, but after
re-reading the downstream venus code a few more times I think this would
be much cleaner to do so, both implementation and binding wise.

Regards,
Bjorn