| Message ID | 1471622000-1906-4-git-send-email-stanimir.varbanov@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
On Fri, Aug 19, 2016 at 06:53:19PM +0300, Stanimir Varbanov wrote: > Add devicetree binding document for Venus remote processor. > > Signed-off-by: Stanimir Varbanov <stanimir.varbanov@linaro.org> > --- > .../devicetree/bindings/remoteproc/qcom,venus.txt | 33 ++++++++++++++++++++++ > 1 file changed, 33 insertions(+) > create mode 100644 Documentation/devicetree/bindings/remoteproc/qcom,venus.txt > > diff --git a/Documentation/devicetree/bindings/remoteproc/qcom,venus.txt b/Documentation/devicetree/bindings/remoteproc/qcom,venus.txt > new file mode 100644 > index 000000000000..06a2db60fa38 > --- /dev/null > +++ b/Documentation/devicetree/bindings/remoteproc/qcom,venus.txt > @@ -0,0 +1,33 @@ > +Qualcomm Venus Peripheral Image Loader > + > +This document defines the binding for a component that loads and boots firmware > +on the Qualcomm Venus remote processor core. This does not make sense to me. Venus is the video encoder/decoder h/w, right? Why is the firmware loader separate from the codec block? Why rproc is used? Are there multiple clients? Naming it rproc_venus implies there aren't. And why does the firmware loading need 8MB of memory at a fixed address? > + > +- compatible: > + Usage: required > + Value type: <string> > + Definition: must contain "qcom,venus-pil" > + > +- memory-region: > + Usage: required > + Value type: <phandle> > + Definition: a phandle to a node describing reserved memory > + > +* An example > + reserved-memory { > + #address-cells = <2>; > + #size-cells = <2>; > + ranges; > + > + venus_mem: venus@89900000 { > + compatible = "shared-dma-pool"; > + reg = <0x0 0x89900000 0x0 0x800000>; > + alignment = <0x1000>; > + no-map; > + }; > + }; > + > + rproc_venus@0 { > + compatible = "qcom,venus-pil"; > + memory-region = <&venus_mem>; > + }; > \ No newline at end of file > -- > 2.7.4 > -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi, Cc: Marek On 08/30/2016 08:17 PM, Bjorn Andersson wrote: > 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. That would be interesting topic. > >>> >>> 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? yes, at least for Venus case, the secured buffers are tied to different iommu contexts (and stream IDs). But this needs to be verified more carefully. > >>> >>> >>> 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. For msm8916 we don't have iommu driver in mainline, I don't know what is the status with msm8996. > > 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). This sounds scary :) > >>> >>>>> >>>>>> 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 Just to be precise it should be dma_alloc_from_coherent(). Marek, what is your opinion on that, can we make dma_alloc_from_coherent able to allocate memory for sizes with bigger granularity. For your convenience here [1] is the mail thread. > allow us to use the existing infrastructure, for both fixed and > dynamically placed memory carveouts in remoteproc. [1] https://lkml.org/lkml/2016/8/19/570 -- regards, Stan -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Bjorn, On 09/02/2016 11:12 PM, Bjorn Andersson wrote: > On Fri 02 Sep 04:52 PDT 2016, Marek Szyprowski wrote: > >> Hi, >> >> >> On 2016-09-01 16:58, Stanimir Varbanov wrote: >>> Hi, >>> >>> Cc: Marek >>> >> >> ... >> >>>>>> 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 >>> Just to be precise it should be dma_alloc_from_coherent(). >>> >>> Marek, what is your opinion on that, can we make dma_alloc_from_coherent >>> able to allocate memory for sizes with bigger granularity. >>> >>> For your convenience here [1] is the mail thread. >> >> There should be no technical restrictions to add support for bigger >> granularity than power-of-2. dma_alloc_from_coherent uses standard >> bitmap based allocator, so it already support tracking allocations of >> arbitrary size. > > I believe we should be able to change the parameter of > bitmap_{find_free,release,allocate}_region() to take a size rather than > an order. > > The mask used in __reg_op() is an unsigned long, that is stamped over > the region to be masked or cleared, so there are some clear restrictions > in what parameters we can pass there - without having to break this > operation up in steps. > > But if drive the offset by taking the next power-of-two of the size and > then align the number of bits to min(count, BITS_PER_LONG) we should > retain the performance characteristics and requirements of __reg_op(). > >> However for the small allocations (smaller than 64KiB?, 512KiB?) it >> would make sense to keep nearest-power-of-2 round up to prevent memory >> fragmentation. > > But in our case each bit matches a single page, so by making sure the > mask always fills the unsigned long in the larger cases we would end up > with having to align things to 128kb (or 256kb if unsigned long is 64 > bit). > > > Does this sound reasonable? I haven't looked in bitmap details, but can't we reuse genalloc? -- regards, Stan -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/Documentation/devicetree/bindings/remoteproc/qcom,venus.txt b/Documentation/devicetree/bindings/remoteproc/qcom,venus.txt new file mode 100644 index 000000000000..06a2db60fa38 --- /dev/null +++ b/Documentation/devicetree/bindings/remoteproc/qcom,venus.txt @@ -0,0 +1,33 @@ +Qualcomm Venus Peripheral Image Loader + +This document defines the binding for a component that loads and boots firmware +on the Qualcomm Venus remote processor core. + +- compatible: + Usage: required + Value type: <string> + Definition: must contain "qcom,venus-pil" + +- memory-region: + Usage: required + Value type: <phandle> + Definition: a phandle to a node describing reserved memory + +* An example + reserved-memory { + #address-cells = <2>; + #size-cells = <2>; + ranges; + + venus_mem: venus@89900000 { + compatible = "shared-dma-pool"; + reg = <0x0 0x89900000 0x0 0x800000>; + alignment = <0x1000>; + no-map; + }; + }; + + rproc_venus@0 { + compatible = "qcom,venus-pil"; + memory-region = <&venus_mem>; + }; \ No newline at end of file
Add devicetree binding document for Venus remote processor. Signed-off-by: Stanimir Varbanov <stanimir.varbanov@linaro.org> --- .../devicetree/bindings/remoteproc/qcom,venus.txt | 33 ++++++++++++++++++++++ 1 file changed, 33 insertions(+) create mode 100644 Documentation/devicetree/bindings/remoteproc/qcom,venus.txt -- 2.7.4 -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html