Re: [PATCH 3/4] dt-binding: remoteproc: venus rproc dt binding document

[Bjorn Andersson]

On Mon 29 Aug 04:48 PDT 2016, Stanimir Varbanov wrote:

[..]
> > Trying to wrap my head around how the iommu part works here. The
> > downstream code seems to indicate that this is a "generic" secure iommu
> > interface - used by venus, camera and kgsl; likely for dealing with DRM
> > protected buffers.
> 
> The secure iommu interface is for content protected buffers. But these
> secure iommu contexts aren't used by msm DRM nor Venus in mainline. In
> Venus case I use non-secure iommu context for data buffers.
> 

We must consider the case when DRM, VFE and Venus handles protected
buffers.

> > 
> > As such the iommu tables are not part of the venus rproc; I believe they
> > should either be tied into the msm-iommu driver or perhaps exposed as
> > its own iommu(?).
> 
> The page tables are in msm-iommu driver.
> 

So, just to verify your answer, the msm-iommu driver will handle both
protected and unprotected?

> > 
> > 
> > But I presume from your inclusion that you've concluded that the venus
> > firmware we have refuses to execute without these tables at least
> > initialized, is this correct?
> 
> Yes, the SMC call for PAS memory-setup will fail if this page table is
> not initialized.
> 

If the msm-iommu driver will handle the protected buffers (or if there
will be a separate iommu driver for protected buffers) it should issue
these calls, to not be dependant on the rproc-venus driver.

With that I think we should make the rproc-venus driver depend on this
being setup (even if this means creating a "dummy" driver for the
protected iommu handling for now).

> > 
> >>>
> >>>> The address is not really fixed, cause the firmware could support
> >>>> relocation. In this example I just picked up the next free memory region
> >>>> in memory-reserved from msm8916.dtsi.
> >>>>
> >>>
> >>> In 8974 we do have a physical region where it's expected to be loaded.
> >>>
> >>> So in line with upcoming remoteproc work we should support referencing a
> >>> reserved-memory node with either reg or size.
> >>>
> >>> In the case of spotting a "reg" we're currently better off using
> >>> ioremap. We're looking at getting the remoteproc core to deal with this
> >>> mess.
> >>
> >> You mean that remoteproc core will parse memory-region property?
> >>
> > 
> > It has to, because it's a quite common scenario for remoteproc drivers
> > to either get its backing memory from a static region or be restricted
> > to part of system ram - properties that reserved-memory and
> > memory-region captures already.
> 
> OK, I have no issues with that. My concern is the manual parsing of
> 'memory-region' and 'reg' properties in remoteproc core.
> 
> So that idea is to have generic binding for rproc, that would be good.
> 

I do share your concerns here. But it's a recurring issue with
remoteproc drivers.

[..]
> > But I presume we have the implementation issue of dma_alloc_coherent()
> > failing in either case with the 5MB size. I think we need to look into
> 
> I'd be good to include Marek Szyprowski? At least he will know what
> design restrictions there are.
> 

Please do. The more I look at this the more I think we must use the
existing infrastructure for allocating "dma memory". Getting
dma_alloc_coherent() supporting non-power-of-2 memory regions would
allow us to use the existing infrastructure, for both fixed and
dynamically placed memory carveouts in remoteproc.

Regards,
Bjorn