| Message ID | 1486473024-21705-3-git-send-email-stanimir.varbanov@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | None | expand |
Hi Rob, On 02/22/2017 02:09 AM, Rob Herring wrote: > On Tue, Feb 07, 2017 at 03:10:17PM +0200, Stanimir Varbanov wrote: >> Add binding document for Venus video encoder/decoder driver >> >> Cc: Rob Herring <robh+dt@kernel.org> >> Cc: devicetree@vger.kernel.org >> Signed-off-by: Stanimir Varbanov <stanimir.varbanov@linaro.org> >> --- >> Changes since previous v5: >> * dropped rproc phandle (remoteproc is not used anymore) >> * added subnodes paragraph with descrition of three subnodes: >> - video-decoder and video-encoder - describes decoder (core0) and >> encoder (core1) power-domains and clocks (applicable for msm8996 >> Venus core). >> - video-firmware - needed to get reserved memory region where the >> firmware is stored. >> >> .../devicetree/bindings/media/qcom,venus.txt | 112 +++++++++++++++++++++ >> 1 file changed, 112 insertions(+) >> create mode 100644 Documentation/devicetree/bindings/media/qcom,venus.txt >> >> diff --git a/Documentation/devicetree/bindings/media/qcom,venus.txt b/Documentation/devicetree/bindings/media/qcom,venus.txt >> new file mode 100644 >> index 000000000000..4427af3ca5a5 >> --- /dev/null >> +++ b/Documentation/devicetree/bindings/media/qcom,venus.txt >> @@ -0,0 +1,112 @@ > > [...] > >> +* Subnodes >> +The Venus node must contain three subnodes representing video-decoder, >> +video-encoder and video-firmware. > > [...] > >> +The video-firmware subnode should contain: >> + >> +- memory-region: >> + Usage: required >> + Value type: <phandle> >> + Definition: reference to the reserved-memory for the memory region >> + >> +* An Example >> + video-codec@1d00000 { >> + compatible = "qcom,msm8916-venus"; >> + reg = <0x01d00000 0xff000>; >> + interrupts = <GIC_SPI 44 IRQ_TYPE_LEVEL_HIGH>; >> + clocks = <&gcc GCC_VENUS0_VCODEC0_CLK>, >> + <&gcc GCC_VENUS0_AHB_CLK>, >> + <&gcc GCC_VENUS0_AXI_CLK>; >> + clock-names = "core", "iface", "bus"; >> + power-domains = <&gcc VENUS_GDSC>; >> + iommus = <&apps_iommu 5>; >> + >> + video-decoder { >> + compatible = "venus-decoder"; >> + clocks = <&mmcc VIDEO_SUBCORE0_CLK>; >> + clock-names = "core"; >> + power-domains = <&mmcc VENUS_CORE0_GDSC>; >> + }; >> + >> + video-encoder { >> + compatible = "venus-encoder"; >> + clocks = <&mmcc VIDEO_SUBCORE1_CLK>; >> + clock-names = "core"; >> + power-domains = <&mmcc VENUS_CORE1_GDSC>; >> + }; >> + >> + video-firmware { >> + memory-region = <&venus_mem>; > > Why does this need to be a sub node? Because firmware reserved memory region must have separate struct device, otherwise allocating video buffers (and map them through iommu) for video-codec will fail because dma_alloc_coherent trying to allocate from per-device coherent area. -- 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
On Wed, Feb 22, 2017 at 3:25 AM, Stanimir Varbanov <stanimir.varbanov@linaro.org> wrote: > Hi Rob, > > On 02/22/2017 02:09 AM, Rob Herring wrote: >> On Tue, Feb 07, 2017 at 03:10:17PM +0200, Stanimir Varbanov wrote: >>> Add binding document for Venus video encoder/decoder driver >>> >>> Cc: Rob Herring <robh+dt@kernel.org> >>> Cc: devicetree@vger.kernel.org >>> Signed-off-by: Stanimir Varbanov <stanimir.varbanov@linaro.org> >>> --- >>> Changes since previous v5: >>> * dropped rproc phandle (remoteproc is not used anymore) >>> * added subnodes paragraph with descrition of three subnodes: >>> - video-decoder and video-encoder - describes decoder (core0) and >>> encoder (core1) power-domains and clocks (applicable for msm8996 >>> Venus core). >>> - video-firmware - needed to get reserved memory region where the >>> firmware is stored. >>> >>> .../devicetree/bindings/media/qcom,venus.txt | 112 +++++++++++++++++++++ >>> 1 file changed, 112 insertions(+) >>> create mode 100644 Documentation/devicetree/bindings/media/qcom,venus.txt >>> >>> diff --git a/Documentation/devicetree/bindings/media/qcom,venus.txt b/Documentation/devicetree/bindings/media/qcom,venus.txt >>> new file mode 100644 >>> index 000000000000..4427af3ca5a5 >>> --- /dev/null >>> +++ b/Documentation/devicetree/bindings/media/qcom,venus.txt >>> @@ -0,0 +1,112 @@ >> >> [...] >> >>> +* Subnodes >>> +The Venus node must contain three subnodes representing video-decoder, >>> +video-encoder and video-firmware. >> >> [...] >> >>> +The video-firmware subnode should contain: >>> + >>> +- memory-region: >>> + Usage: required >>> + Value type: <phandle> >>> + Definition: reference to the reserved-memory for the memory region >>> + >>> +* An Example >>> + video-codec@1d00000 { >>> + compatible = "qcom,msm8916-venus"; >>> + reg = <0x01d00000 0xff000>; >>> + interrupts = <GIC_SPI 44 IRQ_TYPE_LEVEL_HIGH>; >>> + clocks = <&gcc GCC_VENUS0_VCODEC0_CLK>, >>> + <&gcc GCC_VENUS0_AHB_CLK>, >>> + <&gcc GCC_VENUS0_AXI_CLK>; >>> + clock-names = "core", "iface", "bus"; >>> + power-domains = <&gcc VENUS_GDSC>; >>> + iommus = <&apps_iommu 5>; >>> + >>> + video-decoder { >>> + compatible = "venus-decoder"; >>> + clocks = <&mmcc VIDEO_SUBCORE0_CLK>; >>> + clock-names = "core"; >>> + power-domains = <&mmcc VENUS_CORE0_GDSC>; >>> + }; >>> + >>> + video-encoder { >>> + compatible = "venus-encoder"; >>> + clocks = <&mmcc VIDEO_SUBCORE1_CLK>; >>> + clock-names = "core"; >>> + power-domains = <&mmcc VENUS_CORE1_GDSC>; >>> + }; >>> + >>> + video-firmware { >>> + memory-region = <&venus_mem>; >> >> Why does this need to be a sub node? > > Because firmware reserved memory region must have separate struct > device, otherwise allocating video buffers (and map them through iommu) > for video-codec will fail because dma_alloc_coherent trying to allocate > from per-device coherent area. Why can't the struct device be the video-codec device? Looking at the code, I don't see why you need the 2nd struct device. In any case, this is letting the driver design the binding which is wrong. From a binding perspective, there's no reason to have this node. Rob -- 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 02/22/2017 04:17 PM, Rob Herring wrote: > On Wed, Feb 22, 2017 at 3:25 AM, Stanimir Varbanov > <stanimir.varbanov@linaro.org> wrote: >> Hi Rob, >> >> On 02/22/2017 02:09 AM, Rob Herring wrote: >>> On Tue, Feb 07, 2017 at 03:10:17PM +0200, Stanimir Varbanov wrote: >>>> Add binding document for Venus video encoder/decoder driver >>>> >>>> Cc: Rob Herring <robh+dt@kernel.org> >>>> Cc: devicetree@vger.kernel.org >>>> Signed-off-by: Stanimir Varbanov <stanimir.varbanov@linaro.org> >>>> --- >>>> Changes since previous v5: >>>> * dropped rproc phandle (remoteproc is not used anymore) >>>> * added subnodes paragraph with descrition of three subnodes: >>>> - video-decoder and video-encoder - describes decoder (core0) and >>>> encoder (core1) power-domains and clocks (applicable for msm8996 >>>> Venus core). >>>> - video-firmware - needed to get reserved memory region where the >>>> firmware is stored. >>>> >>>> .../devicetree/bindings/media/qcom,venus.txt | 112 +++++++++++++++++++++ >>>> 1 file changed, 112 insertions(+) >>>> create mode 100644 Documentation/devicetree/bindings/media/qcom,venus.txt >>>> >>>> diff --git a/Documentation/devicetree/bindings/media/qcom,venus.txt b/Documentation/devicetree/bindings/media/qcom,venus.txt >>>> new file mode 100644 >>>> index 000000000000..4427af3ca5a5 >>>> --- /dev/null >>>> +++ b/Documentation/devicetree/bindings/media/qcom,venus.txt >>>> @@ -0,0 +1,112 @@ >>> >>> [...] >>> >>>> +* Subnodes >>>> +The Venus node must contain three subnodes representing video-decoder, >>>> +video-encoder and video-firmware. >>> >>> [...] >>> >>>> +The video-firmware subnode should contain: >>>> + >>>> +- memory-region: >>>> + Usage: required >>>> + Value type: <phandle> >>>> + Definition: reference to the reserved-memory for the memory region >>>> + >>>> +* An Example >>>> + video-codec@1d00000 { >>>> + compatible = "qcom,msm8916-venus"; >>>> + reg = <0x01d00000 0xff000>; >>>> + interrupts = <GIC_SPI 44 IRQ_TYPE_LEVEL_HIGH>; >>>> + clocks = <&gcc GCC_VENUS0_VCODEC0_CLK>, >>>> + <&gcc GCC_VENUS0_AHB_CLK>, >>>> + <&gcc GCC_VENUS0_AXI_CLK>; >>>> + clock-names = "core", "iface", "bus"; >>>> + power-domains = <&gcc VENUS_GDSC>; >>>> + iommus = <&apps_iommu 5>; >>>> + >>>> + video-decoder { >>>> + compatible = "venus-decoder"; >>>> + clocks = <&mmcc VIDEO_SUBCORE0_CLK>; >>>> + clock-names = "core"; >>>> + power-domains = <&mmcc VENUS_CORE0_GDSC>; >>>> + }; >>>> + >>>> + video-encoder { >>>> + compatible = "venus-encoder"; >>>> + clocks = <&mmcc VIDEO_SUBCORE1_CLK>; >>>> + clock-names = "core"; >>>> + power-domains = <&mmcc VENUS_CORE1_GDSC>; >>>> + }; >>>> + >>>> + video-firmware { >>>> + memory-region = <&venus_mem>; >>> >>> Why does this need to be a sub node? >> >> Because firmware reserved memory region must have separate struct >> device, otherwise allocating video buffers (and map them through iommu) >> for video-codec will fail because dma_alloc_coherent trying to allocate >> from per-device coherent area. > > Why can't the struct device be the video-codec device? Looking at the > code, I don't see why you need the 2nd struct device. The issue when using one device (video-codec device) is coming from dma_alloc_from_coherent() which is called by dma_alloc_attrs(). The firmware memory-region is parsed and initialize coherent memory such a region with flag DMA_MEMORY_EXCLUSIVE which means that following invocations of dma_alloc_coherent (for example to allocate video buffers and map them through IOMMU) will no fallback to generic memory allocator i.e. dma_ops. Because Venus is behind IOMMU the dma_ops are pointing to iommu_dma_ops for ARM64. > > In any case, this is letting the driver design the binding which is > wrong. From a binding perspective, there's no reason to have this > node. OK, thanks for the comments. I have few changes in the firmware memory allocation part of the driver which makes possible to remove video-firmware subnode and move memory-region to video-codec DT node. Is that fine for you? -- 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/media/qcom,venus.txt b/Documentation/devicetree/bindings/media/qcom,venus.txt new file mode 100644 index 000000000000..4427af3ca5a5 --- /dev/null +++ b/Documentation/devicetree/bindings/media/qcom,venus.txt @@ -0,0 +1,112 @@ +* Qualcomm Venus video encode/decode accelerator + +- compatible: + Usage: required + Value type: <stringlist> + Definition: Value should contain one of: + - "qcom,msm8916-venus" + - "qcom,msm8996-venus" +- reg: + Usage: required + Value type: <prop-encoded-array> + Definition: Register base address and length of the register map. +- interrupts: + Usage: required + Value type: <prop-encoded-array> + Definition: Should contain interrupt line number. +- clocks: + Usage: required + Value type: <prop-encoded-array> + Definition: A List of phandle and clock specifier pairs as listed + in clock-names property. +- clock-names: + Usage: required for msm8916 + Value type: <stringlist> + Definition: Should contain the following entries: + - "core" Core video accelerator clock + - "iface" Video accelerator AHB clock + - "bus" Video accelerator AXI clock +- clock-names: + Usage: required for msm8996 + Value type: <stringlist> + Definition: Should contain the following entries: + - "core" Core video accelerator clock + - "iface" Video accelerator AHB clock + - "bus" Video accelerator AXI clock + - "mbus" Video MAXI clock +- power-domains: + Usage: required + Value type: <prop-encoded-array> + Definition: A phandle and power domain specifier pairs to the + power domain which is responsible for collapsing + and restoring power to the peripheral. +- iommus: + Usage: required + Value type: <prop-encoded-array> + Definition: A list of phandle and IOMMU specifier pairs. + +* Subnodes +The Venus node must contain three subnodes representing video-decoder, +video-encoder and video-firmware. + +Every of video-encoder or video-decoder subnode should have: + +- compatible: + Usage: required + Value type: <stringlist> + Definition: Value should contain "venus-decoder" or "venus-encoder" +- clocks: + Usage: required for msm8996 + Value type: <prop-encoded-array> + Definition: A List of phandle and clock specifier pairs as listed + in clock-names property. +- clock-names: + Usage: required for msm8996 + Value type: <stringlist> + Definition: Should contain the following entries: + - "core" Subcore video accelerator clock + +- power-domains: + Usage: required for msm8996 + Value type: <prop-encoded-array> + Definition: A phandle and power domain specifier pairs to the + power domain which is responsible for collapsing + and restoring power to the subcore. + +The video-firmware subnode should contain: + +- memory-region: + Usage: required + Value type: <phandle> + Definition: reference to the reserved-memory for the memory region + +* An Example + video-codec@1d00000 { + compatible = "qcom,msm8916-venus"; + reg = <0x01d00000 0xff000>; + interrupts = <GIC_SPI 44 IRQ_TYPE_LEVEL_HIGH>; + clocks = <&gcc GCC_VENUS0_VCODEC0_CLK>, + <&gcc GCC_VENUS0_AHB_CLK>, + <&gcc GCC_VENUS0_AXI_CLK>; + clock-names = "core", "iface", "bus"; + power-domains = <&gcc VENUS_GDSC>; + iommus = <&apps_iommu 5>; + + video-decoder { + compatible = "venus-decoder"; + clocks = <&mmcc VIDEO_SUBCORE0_CLK>; + clock-names = "core"; + power-domains = <&mmcc VENUS_CORE0_GDSC>; + }; + + video-encoder { + compatible = "venus-encoder"; + clocks = <&mmcc VIDEO_SUBCORE1_CLK>; + clock-names = "core"; + power-domains = <&mmcc VENUS_CORE1_GDSC>; + }; + + video-firmware { + memory-region = <&venus_mem>; + }; + };
Add binding document for Venus video encoder/decoder driver Cc: Rob Herring <robh+dt@kernel.org> Cc: devicetree@vger.kernel.org Signed-off-by: Stanimir Varbanov <stanimir.varbanov@linaro.org> --- Changes since previous v5: * dropped rproc phandle (remoteproc is not used anymore) * added subnodes paragraph with descrition of three subnodes: - video-decoder and video-encoder - describes decoder (core0) and encoder (core1) power-domains and clocks (applicable for msm8996 Venus core). - video-firmware - needed to get reserved memory region where the firmware is stored. .../devicetree/bindings/media/qcom,venus.txt | 112 +++++++++++++++++++++ 1 file changed, 112 insertions(+) create mode 100644 Documentation/devicetree/bindings/media/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