| Message ID | c7995af1f91733626ff4e86f0575dea5d2ff0bb8.1697597713.git.ming.qian@nxp.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | media: imx-jpeg: Assign a single slot | expand |
On 19/10/2023 04:48, Ming Qian (OSS) wrote: > From: Ming Qian <ming.qian@nxp.com> > > This IP includes a jpeg wrapper and a jpeg engine, the wrapper is > working on descriptor based manner. It supports up to 4 slots, each slot > can have its own chained descriptors. Host won't configure the engine > directly, but fill some descriptors to encode or decode one jpeg > picture. Then configure the descriptors to certain slot register. The > jpeg wrapper will schedule between different slots. When some slot is > finished, the slot interrupt will be triggered. The purpose of slot is > that engine can be shared across multiple VMS and os. > > Currently, power domains and interrupts are enabled for all 4 slots, but > only one slot is used. There is no benefit in using more that one slot > from within the same OS, as the slots are scheduled in round-robin > manner and not executed in parallel. > > Use the property "nxp,slot" to assign a single slot, and just expose the > parts of the h/w for the assigned slot. For example, only put slot 1's > power-domains entry in the DT when slot 1 is assigned. If not specified, > 0 is used by default. > > Signed-off-by: Ming Qian <ming.qian@nxp.com> > --- > v5 > - improve commit message > - improve property description > > v4 > - improve commit message > - drop line making the property required, to avoid ABI break > > v3 > - add vender prefix, change property slot to nxp,slot > - add type for property slot > > v2 > - add a new property in bindings document > > .../bindings/media/nxp,imx8-jpeg.yaml | 46 +++++++++---------- > 1 file changed, 22 insertions(+), 24 deletions(-) > > diff --git a/Documentation/devicetree/bindings/media/nxp,imx8-jpeg.yaml b/Documentation/devicetree/bindings/media/nxp,imx8-jpeg.yaml > index 3d9d1db37040..0961856bdcab 100644 > --- a/Documentation/devicetree/bindings/media/nxp,imx8-jpeg.yaml > +++ b/Documentation/devicetree/bindings/media/nxp,imx8-jpeg.yaml > @@ -32,19 +32,27 @@ properties: > maxItems: 1 > > interrupts: > - description: | > - There are 4 slots available in the IP, which the driver may use > - If a certain slot is used, it should have an associated interrupt > - The interrupt with index i is assumed to be for slot i > - minItems: 1 # At least one slot is needed by the driver > - maxItems: 4 # The IP has 4 slots available for use > + description: > + Interrupt number for slot > + maxItems: 1 The device still has four interrupts, so we should allow up to four of them. One given OS might want to use two or all four slots. > > power-domains: > description: > List of phandle and PM domain specifier as documented in > Documentation/devicetree/bindings/power/power_domain.txt > - minItems: 2 # Wrapper and 1 slot > - maxItems: 5 # Wrapper and 4 slots > + minItems: 1 # Mixed power domain > + maxItems: 2 # Wrapper and 1 slot > + > + nxp,slot: > + $ref: /schemas/types.yaml#/definitions/uint32 > + description: > + Integer number of slot index used. This IP includes a jpeg wrapper, the > + wrapper is working on descriptor based manner. It supports up to 4 slots, > + each slot can have its own chained descriptors. The purpose is to share > + the jpeg engine across multiple VMS and os. We use this property to > + assign a single slot. If not specified, 0 is used by default. > + minimum: 0 > + maximum: 3 default: 0 Best regards, Krzysztof
Hi Krzysztof, >> From: Ming Qian <ming.qian@nxp.com> >> >> This IP includes a jpeg wrapper and a jpeg engine, the wrapper is >> working on descriptor based manner. It supports up to 4 slots, each slot >> can have its own chained descriptors. Host won't configure the engine >> directly, but fill some descriptors to encode or decode one jpeg >> picture. Then configure the descriptors to certain slot register. The >> jpeg wrapper will schedule between different slots. When some slot is >> finished, the slot interrupt will be triggered. The purpose of slot is >> that engine can be shared across multiple VMS and os. >> >> Currently, power domains and interrupts are enabled for all 4 slots, but >> only one slot is used. There is no benefit in using more that one slot >> from within the same OS, as the slots are scheduled in round-robin >> manner and not executed in parallel. >> >> Use the property "nxp,slot" to assign a single slot, and just expose the >> parts of the h/w for the assigned slot. For example, only put slot 1's >> power-domains entry in the DT when slot 1 is assigned. If not specified, >> 0 is used by default. >> >> Signed-off-by: Ming Qian <ming.qian@nxp.com> >> --- >> v5 >> - improve commit message >> - improve property description >> >> v4 >> - improve commit message >> - drop line making the property required, to avoid ABI break >> >> v3 >> - add vender prefix, change property slot to nxp,slot >> - add type for property slot >> >> v2 >> - add a new property in bindings document >> >> .../bindings/media/nxp,imx8-jpeg.yaml | 46 +++++++++---------- >> 1 file changed, 22 insertions(+), 24 deletions(-) >> >> diff --git a/Documentation/devicetree/bindings/media/nxp,imx8-jpeg.yaml b/Documentation/devicetree/bindings/media/nxp,imx8-jpeg.yaml >> index 3d9d1db37040..0961856bdcab 100644 >> --- a/Documentation/devicetree/bindings/media/nxp,imx8-jpeg.yaml >> +++ b/Documentation/devicetree/bindings/media/nxp,imx8-jpeg.yaml >> @@ -32,19 +32,27 @@ properties: >> maxItems: 1 >> >> interrupts: >> - description: | >> - There are 4 slots available in the IP, which the driver may use >> - If a certain slot is used, it should have an associated interrupt >> - The interrupt with index i is assumed to be for slot i >> - minItems: 1 # At least one slot is needed by the driver >> - maxItems: 4 # The IP has 4 slots available for use >> + description: >> + Interrupt number for slot >> + maxItems: 1 > > The device still has four interrupts, so we should allow up to four of > them. One given OS might want to use two or all four slots. > > Got it, I will fix it in v6 patch. >> >> power-domains: >> description: >> List of phandle and PM domain specifier as documented in >> Documentation/devicetree/bindings/power/power_domain.txt >> - minItems: 2 # Wrapper and 1 slot >> - maxItems: 5 # Wrapper and 4 slots >> + minItems: 1 # Mixed power domain >> + maxItems: 2 # Wrapper and 1 slot >> + >> + nxp,slot: >> + $ref: /schemas/types.yaml#/definitions/uint32 >> + description: >> + Integer number of slot index used. This IP includes a jpeg wrapper, the >> + wrapper is working on descriptor based manner. It supports up to 4 slots, >> + each slot can have its own chained descriptors. The purpose is to share >> + the jpeg engine across multiple VMS and os. We use this property to >> + assign a single slot. If not specified, 0 is used by default. >> + minimum: 0 >> + maximum: 3 > > default: 0 > Got it. Best regards, Ming > > > Best regards, > Krzysztof >
Hi Krzysztof, > >>> From: Ming Qian <ming.qian@nxp.com> >>> >>> This IP includes a jpeg wrapper and a jpeg engine, the wrapper is >>> working on descriptor based manner. It supports up to 4 slots, each slot >>> can have its own chained descriptors. Host won't configure the engine >>> directly, but fill some descriptors to encode or decode one jpeg >>> picture. Then configure the descriptors to certain slot register. The >>> jpeg wrapper will schedule between different slots. When some slot is >>> finished, the slot interrupt will be triggered. The purpose of slot is >>> that engine can be shared across multiple VMS and os. >>> >>> Currently, power domains and interrupts are enabled for all 4 slots, but >>> only one slot is used. There is no benefit in using more that one slot >>> from within the same OS, as the slots are scheduled in round-robin >>> manner and not executed in parallel. >>> >>> Use the property "nxp,slot" to assign a single slot, and just expose the >>> parts of the h/w for the assigned slot. For example, only put slot 1's >>> power-domains entry in the DT when slot 1 is assigned. If not specified, >>> 0 is used by default. >>> >>> Signed-off-by: Ming Qian <ming.qian@nxp.com> >>> --- >>> v5 >>> - improve commit message >>> - improve property description >>> >>> v4 >>> - improve commit message >>> - drop line making the property required, to avoid ABI break >>> >>> v3 >>> - add vender prefix, change property slot to nxp,slot >>> - add type for property slot >>> >>> v2 >>> - add a new property in bindings document >>> >>> .../bindings/media/nxp,imx8-jpeg.yaml | 46 +++++++++---------- >>> 1 file changed, 22 insertions(+), 24 deletions(-) >>> >>> diff --git >>> a/Documentation/devicetree/bindings/media/nxp,imx8-jpeg.yaml >>> b/Documentation/devicetree/bindings/media/nxp,imx8-jpeg.yaml >>> index 3d9d1db37040..0961856bdcab 100644 >>> --- a/Documentation/devicetree/bindings/media/nxp,imx8-jpeg.yaml >>> +++ b/Documentation/devicetree/bindings/media/nxp,imx8-jpeg.yaml >>> @@ -32,19 +32,27 @@ properties: >>> maxItems: 1 >>> interrupts: >>> - description: | >>> - There are 4 slots available in the IP, which the driver may use >>> - If a certain slot is used, it should have an associated interrupt >>> - The interrupt with index i is assumed to be for slot i >>> - minItems: 1 # At least one slot is needed by the >>> driver >>> - maxItems: 4 # The IP has 4 slots available for use >>> + description: >>> + Interrupt number for slot >>> + maxItems: 1 >> >> The device still has four interrupts, so we should allow up to four of >> them. One given OS might want to use two or all four slots. >> >> > Got it, I will fix it in v6 patch. > We made an internal discussion about this, current approach of the bindings implies that one dts node is for one slot only, and the slot property is integer, not a list of slots. Whoever wants to use 2 or more slots, should put more nodes in the dts, one for each slot. Therefor, there is no point in allowing more than one interrupt in one node. Or, if we do, for the sake of allowing one OS to use more slots within the same dts node, we must also allow more power domains, in any case, we do not have multiple slots functionality in the driver anymore. Best regards, Ming >>> power-domains: >>> description: >>> List of phandle and PM domain specifier as documented in >>> Documentation/devicetree/bindings/power/power_domain.txt >>> - minItems: 2 # Wrapper and 1 slot >>> - maxItems: 5 # Wrapper and 4 slots >>> + minItems: 1 # Mixed power domain >>> + maxItems: 2 # Wrapper and 1 slot >>> + >>> + nxp,slot: >>> + $ref: /schemas/types.yaml#/definitions/uint32 >>> + description: >>> + Integer number of slot index used. This IP includes a jpeg >>> wrapper, the >>> + wrapper is working on descriptor based manner. It supports up >>> to 4 slots, >>> + each slot can have its own chained descriptors. The purpose is >>> to share >>> + the jpeg engine across multiple VMS and os. We use this >>> property to >>> + assign a single slot. If not specified, 0 is used by default. >>> + minimum: 0 >>> + maximum: 3 >> >> default: 0 >> > Got it. > > Best regards, > Ming >> >> >> Best regards, >> Krzysztof >>
On 20/10/2023 07:38, ming qian wrote: > Hi Krzysztof, > >> >>>> From: Ming Qian <ming.qian@nxp.com> >>>> >>>> This IP includes a jpeg wrapper and a jpeg engine, the wrapper is >>>> working on descriptor based manner. It supports up to 4 slots, each slot >>>> can have its own chained descriptors. Host won't configure the engine >>>> directly, but fill some descriptors to encode or decode one jpeg >>>> picture. Then configure the descriptors to certain slot register. The >>>> jpeg wrapper will schedule between different slots. When some slot is >>>> finished, the slot interrupt will be triggered. The purpose of slot is >>>> that engine can be shared across multiple VMS and os. >>>> >>>> Currently, power domains and interrupts are enabled for all 4 slots, but >>>> only one slot is used. There is no benefit in using more that one slot >>>> from within the same OS, as the slots are scheduled in round-robin >>>> manner and not executed in parallel. >>>> >>>> Use the property "nxp,slot" to assign a single slot, and just expose the >>>> parts of the h/w for the assigned slot. For example, only put slot 1's >>>> power-domains entry in the DT when slot 1 is assigned. If not specified, >>>> 0 is used by default. >>>> >>>> Signed-off-by: Ming Qian <ming.qian@nxp.com> >>>> --- >>>> v5 >>>> - improve commit message >>>> - improve property description >>>> >>>> v4 >>>> - improve commit message >>>> - drop line making the property required, to avoid ABI break >>>> >>>> v3 >>>> - add vender prefix, change property slot to nxp,slot >>>> - add type for property slot >>>> >>>> v2 >>>> - add a new property in bindings document >>>> >>>> .../bindings/media/nxp,imx8-jpeg.yaml | 46 +++++++++---------- >>>> 1 file changed, 22 insertions(+), 24 deletions(-) >>>> >>>> diff --git >>>> a/Documentation/devicetree/bindings/media/nxp,imx8-jpeg.yaml >>>> b/Documentation/devicetree/bindings/media/nxp,imx8-jpeg.yaml >>>> index 3d9d1db37040..0961856bdcab 100644 >>>> --- a/Documentation/devicetree/bindings/media/nxp,imx8-jpeg.yaml >>>> +++ b/Documentation/devicetree/bindings/media/nxp,imx8-jpeg.yaml >>>> @@ -32,19 +32,27 @@ properties: >>>> maxItems: 1 >>>> interrupts: >>>> - description: | >>>> - There are 4 slots available in the IP, which the driver may use >>>> - If a certain slot is used, it should have an associated interrupt >>>> - The interrupt with index i is assumed to be for slot i >>>> - minItems: 1 # At least one slot is needed by the >>>> driver >>>> - maxItems: 4 # The IP has 4 slots available for use >>>> + description: >>>> + Interrupt number for slot >>>> + maxItems: 1 >>> >>> The device still has four interrupts, so we should allow up to four of >>> them. One given OS might want to use two or all four slots. >>> >>> >> Got it, I will fix it in v6 patch. >> > > We made an internal discussion about this, current approach of the How does it help me? Why private discussion should matter to us? > bindings implies that one dts node is for one slot only, and the slot > property is integer, not a list of slots. Whoever wants to use 2 or more > slots, should put more nodes in the dts, one for each slot. Therefor, Really? Under the same IO address? Please show how it works and then run `dtbs_check W=1`.. > there is no point in allowing more than one interrupt in one node. Or, > if we do, for the sake of allowing one OS to use more slots within the > same dts node, we must also allow more power domains, in any case, we do power-domains, interrupts, everything. > not have multiple slots functionality in the driver anymore. Why your driver should limit bindings? My driver supports multiple slots... Best regards, Krzysztof
diff --git a/Documentation/devicetree/bindings/media/nxp,imx8-jpeg.yaml b/Documentation/devicetree/bindings/media/nxp,imx8-jpeg.yaml index 3d9d1db37040..0961856bdcab 100644 --- a/Documentation/devicetree/bindings/media/nxp,imx8-jpeg.yaml +++ b/Documentation/devicetree/bindings/media/nxp,imx8-jpeg.yaml @@ -32,19 +32,27 @@ properties: maxItems: 1 interrupts: - description: | - There are 4 slots available in the IP, which the driver may use - If a certain slot is used, it should have an associated interrupt - The interrupt with index i is assumed to be for slot i - minItems: 1 # At least one slot is needed by the driver - maxItems: 4 # The IP has 4 slots available for use + description: + Interrupt number for slot + maxItems: 1 power-domains: description: List of phandle and PM domain specifier as documented in Documentation/devicetree/bindings/power/power_domain.txt - minItems: 2 # Wrapper and 1 slot - maxItems: 5 # Wrapper and 4 slots + minItems: 1 # Mixed power domain + maxItems: 2 # Wrapper and 1 slot + + nxp,slot: + $ref: /schemas/types.yaml#/definitions/uint32 + description: + Integer number of slot index used. This IP includes a jpeg wrapper, the + wrapper is working on descriptor based manner. It supports up to 4 slots, + each slot can have its own chained descriptors. The purpose is to share + the jpeg engine across multiple VMS and os. We use this property to + assign a single slot. If not specified, 0 is used by default. + minimum: 0 + maximum: 3 required: - compatible @@ -62,28 +70,18 @@ examples: jpegdec: jpegdec@58400000 { compatible = "nxp,imx8qxp-jpgdec"; reg = <0x58400000 0x00050000 >; - interrupts = <GIC_SPI 309 IRQ_TYPE_LEVEL_HIGH>, - <GIC_SPI 310 IRQ_TYPE_LEVEL_HIGH>, - <GIC_SPI 311 IRQ_TYPE_LEVEL_HIGH>, - <GIC_SPI 312 IRQ_TYPE_LEVEL_HIGH>; + interrupts = <GIC_SPI 309 IRQ_TYPE_LEVEL_HIGH>; power-domains = <&pd IMX_SC_R_MJPEG_DEC_MP>, - <&pd IMX_SC_R_MJPEG_DEC_S0>, - <&pd IMX_SC_R_MJPEG_DEC_S1>, - <&pd IMX_SC_R_MJPEG_DEC_S2>, - <&pd IMX_SC_R_MJPEG_DEC_S3>; + <&pd IMX_SC_R_MJPEG_DEC_S0>; + nxp,slot = <0>; }; jpegenc: jpegenc@58450000 { compatible = "nxp,imx8qm-jpgenc", "nxp,imx8qxp-jpgenc"; reg = <0x58450000 0x00050000 >; - interrupts = <GIC_SPI 305 IRQ_TYPE_LEVEL_HIGH>, - <GIC_SPI 306 IRQ_TYPE_LEVEL_HIGH>, - <GIC_SPI 307 IRQ_TYPE_LEVEL_HIGH>, - <GIC_SPI 308 IRQ_TYPE_LEVEL_HIGH>; + interrupts = <GIC_SPI 305 IRQ_TYPE_LEVEL_HIGH>; power-domains = <&pd IMX_SC_R_MJPEG_ENC_MP>, - <&pd IMX_SC_R_MJPEG_ENC_S0>, - <&pd IMX_SC_R_MJPEG_ENC_S1>, - <&pd IMX_SC_R_MJPEG_ENC_S2>, - <&pd IMX_SC_R_MJPEG_ENC_S3>; + <&pd IMX_SC_R_MJPEG_ENC_S0>; + nxp,slot = <0>; }; ...