| Message ID | 1486473024-21705-3-git-send-email-stanimir.varbanov@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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? Rob
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.
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
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?
On Thu, Feb 23, 2017 at 7:08 AM, Stanimir Varbanov <stanimir.varbanov@linaro.org> wrote: > > > 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? Yes. With that change: Acked-by: Rob Herring <robh@kernel.org>
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