[PATCH v3 0/4] media: rockchip: rga: PX30 support and YUV2YUV fix

[PATCH v3 0/4] media: rockchip: rga: PX30 support and YUV2YUV fix

[Paul Kocialkowski]

Hi,

This series adds support for the Rockchip PX30 SoC in the V4L2 M2M RGA driver.
It also contains a fix for the YUV2YUV case that was not properly handled.

Changes since v2:
- Used RK3288 compatible in PX30 dts, removed PX30 compatible from driver;
- Added cleanup patch with format macros;
- Added comment about CSC mode fix.

Changes since v1:
- Rebased on media tree master (changed dt binding to yaml);
- Removed spurious line removal.

Cheers,

Paul

Paul Kocialkowski (4):
  dt-bindings: rockchip-rga: Add PX30 compatible
  arm64: dts: rockchip: Add RGA support to the PX30
  media: rockchip: rga: Introduce color fmt macros and refactor CSC mode
    logic
  media: rockchip: rga: Only set output CSC mode for RGB input

 .../bindings/media/rockchip-rga.yaml          |  3 ++
 arch/arm64/boot/dts/rockchip/px30.dtsi        | 11 +++++++
 drivers/media/platform/rockchip/rga/rga-hw.c  | 29 ++++++++++---------
 drivers/media/platform/rockchip/rga/rga-hw.h  |  5 ++++
 4 files changed, 35 insertions(+), 13 deletions(-)

-- 
2.26.0

[PATCH v3 1/4] dt-bindings: rockchip-rga: Add PX30 compatible

[Paul Kocialkowski]

Add a new compatible for the PX30 Rockchip SoC, which also features
a RGA block. It is compatible with the RK3288 RGA block.

Signed-off-by: Paul Kocialkowski <paul.kocialkowski@bootlin.com>
---
 Documentation/devicetree/bindings/media/rockchip-rga.yaml | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/Documentation/devicetree/bindings/media/rockchip-rga.yaml b/Documentation/devicetree/bindings/media/rockchip-rga.yaml
index dd645ddccb07..740586299da9 100644
--- a/Documentation/devicetree/bindings/media/rockchip-rga.yaml
+++ b/Documentation/devicetree/bindings/media/rockchip-rga.yaml
@@ -23,6 +23,9 @@ properties:
       - items:
           - const: rockchip,rk3228-rga
           - const: rockchip,rk3288-rga
+      - items:
+          - const: rockchip,px30-rga
+          - const: rockchip,rk3288-rga
 
   reg:
     maxItems: 1
-- 
2.26.0

[PATCH v3 2/4] arm64: dts: rockchip: Add RGA support to the PX30

[Paul Kocialkowski]

The PX30 features a RGA block: add the necessary node to support it.

Signed-off-by: Paul Kocialkowski <paul.kocialkowski@bootlin.com>
---
 arch/arm64/boot/dts/rockchip/px30.dtsi | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/arch/arm64/boot/dts/rockchip/px30.dtsi b/arch/arm64/boot/dts/rockchip/px30.dtsi
index f809dd6d5dc3..3de70aa4f1ce 100644
--- a/arch/arm64/boot/dts/rockchip/px30.dtsi
+++ b/arch/arm64/boot/dts/rockchip/px30.dtsi
@@ -1102,6 +1102,17 @@ vopl_mmu: iommu@ff470f00 {
 		status = "disabled";
 	};
 
+	rga: rga@ff480000 {
+		compatible = "rockchip,px30-rga", "rockchip,rk3288-rga";
+		reg = <0x0 0xff480000 0x0 0x10000>;
+		interrupts = <GIC_SPI 76 IRQ_TYPE_LEVEL_HIGH 0>;
+		clocks = <&cru ACLK_RGA>, <&cru HCLK_RGA>, <&cru SCLK_RGA_CORE>;
+		clock-names = "aclk", "hclk", "sclk";
+		resets = <&cru SRST_RGA>, <&cru SRST_RGA_A>, <&cru SRST_RGA_H>;
+		reset-names = "core", "axi", "ahb";
+		power-domains = <&power PX30_PD_VO>;
+	};
+
 	qos_gmac: qos@ff518000 {
 		compatible = "syscon";
 		reg = <0x0 0xff518000 0x0 0x20>;
-- 
2.26.0

[PATCH v3 3/4] media: rockchip: rga: Introduce color fmt macros and refactor CSC mode logic

[Paul Kocialkowski]

This introduces two macros: RGA_COLOR_FMT_IS_YUV and RGA_COLOR_FMT_IS_RGB
which allow quick checking of the colorspace familily of a RGA color format.

These macros are then used to refactor the logic for CSC mode selection.
The two nested tests for input colorspace are simplified into a single one,
with a logical and, making the whole more readable.

Signed-off-by: Paul Kocialkowski <paul.kocialkowski@bootlin.com>
---
 drivers/media/platform/rockchip/rga/rga-hw.c | 23 +++++++++-----------
 drivers/media/platform/rockchip/rga/rga-hw.h |  5 +++++
 2 files changed, 15 insertions(+), 13 deletions(-)

diff --git a/drivers/media/platform/rockchip/rga/rga-hw.c b/drivers/media/platform/rockchip/rga/rga-hw.c
index 4be6dcf292ff..5607ee8d1917 100644
--- a/drivers/media/platform/rockchip/rga/rga-hw.c
+++ b/drivers/media/platform/rockchip/rga/rga-hw.c
@@ -200,22 +200,19 @@ static void rga_cmd_set_trans_info(struct rga_ctx *ctx)
 	dst_info.data.format = ctx->out.fmt->hw_format;
 	dst_info.data.swap = ctx->out.fmt->color_swap;
 
-	if (ctx->in.fmt->hw_format >= RGA_COLOR_FMT_YUV422SP) {
-		if (ctx->out.fmt->hw_format < RGA_COLOR_FMT_YUV422SP) {
-			switch (ctx->in.colorspace) {
-			case V4L2_COLORSPACE_REC709:
-				src_info.data.csc_mode =
-					RGA_SRC_CSC_MODE_BT709_R0;
-				break;
-			default:
-				src_info.data.csc_mode =
-					RGA_SRC_CSC_MODE_BT601_R0;
-				break;
-			}
+	if (RGA_COLOR_FMT_IS_YUV(ctx->in.fmt->hw_format) &&
+	    RGA_COLOR_FMT_IS_RGB(ctx->out.fmt->hw_format)) {
+		switch (ctx->in.colorspace) {
+		case V4L2_COLORSPACE_REC709:
+			src_info.data.csc_mode = RGA_SRC_CSC_MODE_BT709_R0;
+			break;
+		default:
+			src_info.data.csc_mode = RGA_SRC_CSC_MODE_BT601_R0;
+			break;
 		}
 	}
 
-	if (ctx->out.fmt->hw_format >= RGA_COLOR_FMT_YUV422SP) {
+	if (RGA_COLOR_FMT_IS_YUV(ctx->out.fmt->hw_format)) {
 		switch (ctx->out.colorspace) {
 		case V4L2_COLORSPACE_REC709:
 			dst_info.data.csc_mode = RGA_SRC_CSC_MODE_BT709_R0;
diff --git a/drivers/media/platform/rockchip/rga/rga-hw.h b/drivers/media/platform/rockchip/rga/rga-hw.h
index 96cb0314dfa7..e8917e5630a4 100644
--- a/drivers/media/platform/rockchip/rga/rga-hw.h
+++ b/drivers/media/platform/rockchip/rga/rga-hw.h
@@ -95,6 +95,11 @@
 #define RGA_COLOR_FMT_CP_8BPP 15
 #define RGA_COLOR_FMT_MASK 15
 
+#define RGA_COLOR_FMT_IS_YUV(fmt) \
+	(((fmt) >= RGA_COLOR_FMT_YUV422SP) && ((fmt) < RGA_COLOR_FMT_CP_1BPP))
+#define RGA_COLOR_FMT_IS_RGB(fmt) \
+	((fmt) < RGA_COLOR_FMT_YUV422SP)
+
 #define RGA_COLOR_NONE_SWAP 0
 #define RGA_COLOR_RB_SWAP 1
 #define RGA_COLOR_ALPHA_SWAP 2
-- 
2.26.0

[PATCH v3 4/4] media: rockchip: rga: Only set output CSC mode for RGB input

[Paul Kocialkowski]

Setting the output CSC mode is required for a YUV output, but must not
be set when the input is also YUV. Doing this (as tested with a YUV420P
to YUV420P conversion) results in wrong colors.

Adapt the logic to only set the output CSC mode when the output is YUV and
the input is RGB. Also add a comment to clarify the rationale.

Fixes: f7e7b48e6d79 ("[media] rockchip/rga: v4l2 m2m support")
Signed-off-by: Paul Kocialkowski <paul.kocialkowski@bootlin.com>
---
 drivers/media/platform/rockchip/rga/rga-hw.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/drivers/media/platform/rockchip/rga/rga-hw.c b/drivers/media/platform/rockchip/rga/rga-hw.c
index 5607ee8d1917..aaa96f256356 100644
--- a/drivers/media/platform/rockchip/rga/rga-hw.c
+++ b/drivers/media/platform/rockchip/rga/rga-hw.c
@@ -200,6 +200,11 @@ static void rga_cmd_set_trans_info(struct rga_ctx *ctx)
 	dst_info.data.format = ctx->out.fmt->hw_format;
 	dst_info.data.swap = ctx->out.fmt->color_swap;
 
+	/*
+	 * CSC mode must only be set when the colorspace families differ between
+	 * input and output. It must remain unset (zeroed) if both are the same.
+	 */
+
 	if (RGA_COLOR_FMT_IS_YUV(ctx->in.fmt->hw_format) &&
 	    RGA_COLOR_FMT_IS_RGB(ctx->out.fmt->hw_format)) {
 		switch (ctx->in.colorspace) {
@@ -212,7 +217,8 @@ static void rga_cmd_set_trans_info(struct rga_ctx *ctx)
 		}
 	}
 
-	if (RGA_COLOR_FMT_IS_YUV(ctx->out.fmt->hw_format)) {
+	if (RGA_COLOR_FMT_IS_RGB(ctx->in.fmt->hw_format) &&
+	    RGA_COLOR_FMT_IS_YUV(ctx->out.fmt->hw_format)) {
 		switch (ctx->out.colorspace) {
 		case V4L2_COLORSPACE_REC709:
 			dst_info.data.csc_mode = RGA_SRC_CSC_MODE_BT709_R0;
-- 
2.26.0

Re: [PATCH v3 1/4] dt-bindings: rockchip-rga: Add PX30 compatible

[Johan Jonker]

Hi Paul,

> 
> Add a new compatible for the PX30 Rockchip SoC, which also features
> a RGA block. It is compatible with the RK3288 RGA block.
> 
> Signed-off-by: Paul Kocialkowski <paul.kocialkowski@bootlin.com>
> ---
>  Documentation/devicetree/bindings/media/rockchip-rga.yaml | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/media/rockchip-rga.yaml b/Documentation/devicetree/bindings/media/rockchip-rga.yaml
> index dd645ddccb07..740586299da9 100644
> --- a/Documentation/devicetree/bindings/media/rockchip-rga.yaml
> +++ b/Documentation/devicetree/bindings/media/rockchip-rga.yaml
> @@ -23,6 +23,9 @@ properties:


>        - items:
>            - const: rockchip,rk3228-rga
>            - const: rockchip,rk3288-rga
> +      - items:
> +          - const: rockchip,px30-rga
> +          - const: rockchip,rk3288-rga

Use enum.

      - items:
          - enum:
            - rockchip,px30-rga
            - rockchip,rk3228-rga
          - const: rockchip,rk3288-rga

>  
>    reg:
>      maxItems: 1
> -- 
> 2.26.0

Re: [PATCH v3 2/4] arm64: dts: rockchip: Add RGA support to the PX30

[Johan Jonker]

Hi Paul,

> The PX30 features a RGA block: add the necessary node to support it.
> 
> Signed-off-by: Paul Kocialkowski <paul.kocialkowski@bootlin.com>
> ---
>  arch/arm64/boot/dts/rockchip/px30.dtsi | 11 +++++++++++
>  1 file changed, 11 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/rockchip/px30.dtsi b/arch/arm64/boot/dts/rockchip/px30.dtsi
> index f809dd6d5dc3..3de70aa4f1ce 100644
> --- a/arch/arm64/boot/dts/rockchip/px30.dtsi
> +++ b/arch/arm64/boot/dts/rockchip/px30.dtsi
> @@ -1102,6 +1102,17 @@ vopl_mmu: iommu@ff470f00 {
>  		status = "disabled";
>  	};
>  
> +	rga: rga@ff480000 {
> +		compatible = "rockchip,px30-rga", "rockchip,rk3288-rga";
> +		reg = <0x0 0xff480000 0x0 0x10000>;
> +		interrupts = <GIC_SPI 76 IRQ_TYPE_LEVEL_HIGH 0>;
> +		clocks = <&cru ACLK_RGA>, <&cru HCLK_RGA>, <&cru SCLK_RGA_CORE>;
> +		clock-names = "aclk", "hclk", "sclk";

> +		resets = <&cru SRST_RGA>, <&cru SRST_RGA_A>, <&cru SRST_RGA_H>;
> +		reset-names = "core", "axi", "ahb";
> +		power-domains = <&power PX30_PD_VO>;

sort

		power-domains = <&power PX30_PD_VO>;
		resets = <&cru SRST_RGA>, <&cru SRST_RGA_A>, <&cru SRST_RGA_H>;
		reset-names = "core", "axi", "ahb";



> +	};
> +
>  	qos_gmac: qos@ff518000 {
>  		compatible = "syscon";
>  		reg = <0x0 0xff518000 0x0 0x20>;
> -- 
> 2.26.0

Re: [PATCH v3 1/4] dt-bindings: rockchip-rga: Add PX30 compatible

[Paul Kocialkowski]

[-- Attachment #1: Type: text/plain, Size: 1568 bytes --]

Hi,

On Thu 30 Apr 20, 23:24, Johan Jonker wrote:
> Hi Paul,
> 
> > 
> > Add a new compatible for the PX30 Rockchip SoC, which also features
> > a RGA block. It is compatible with the RK3288 RGA block.
> > 
> > Signed-off-by: Paul Kocialkowski <paul.kocialkowski@bootlin.com>
> > ---
> >  Documentation/devicetree/bindings/media/rockchip-rga.yaml | 3 +++
> >  1 file changed, 3 insertions(+)
> > 
> > diff --git a/Documentation/devicetree/bindings/media/rockchip-rga.yaml b/Documentation/devicetree/bindings/media/rockchip-rga.yaml
> > index dd645ddccb07..740586299da9 100644
> > --- a/Documentation/devicetree/bindings/media/rockchip-rga.yaml
> > +++ b/Documentation/devicetree/bindings/media/rockchip-rga.yaml
> > @@ -23,6 +23,9 @@ properties:
> 
> 
> >        - items:
> >            - const: rockchip,rk3228-rga
> >            - const: rockchip,rk3288-rga
> > +      - items:
> > +          - const: rockchip,px30-rga
> > +          - const: rockchip,rk3288-rga
> 
> Use enum.
> 
>       - items:
>           - enum:
>             - rockchip,px30-rga
>             - rockchip,rk3228-rga
>           - const: rockchip,rk3288-rga

Are you sure about this? The rk3228 above does it as I did it and other examples
like allwinner,sun4i-a10-csi.yaml appear to be doing the same too.

The case with rockchip,rk3288-rga alone already seems covered.

Cheers,

Paul

> >  
> >    reg:
> >      maxItems: 1
> > -- 
> > 2.26.0
> 

-- 
Paul Kocialkowski, Bootlin
Embedded Linux and kernel engineering
https://bootlin.com

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v3 2/4] arm64: dts: rockchip: Add RGA support to the PX30

[Paul Kocialkowski]

[-- Attachment #1: Type: text/plain, Size: 1824 bytes --]

Hi,

On Fri 01 May 20, 00:05, Johan Jonker wrote:
> Hi Paul,
> 
> > The PX30 features a RGA block: add the necessary node to support it.
> > 
> > Signed-off-by: Paul Kocialkowski <paul.kocialkowski@bootlin.com>
> > ---
> >  arch/arm64/boot/dts/rockchip/px30.dtsi | 11 +++++++++++
> >  1 file changed, 11 insertions(+)
> > 
> > diff --git a/arch/arm64/boot/dts/rockchip/px30.dtsi b/arch/arm64/boot/dts/rockchip/px30.dtsi
> > index f809dd6d5dc3..3de70aa4f1ce 100644
> > --- a/arch/arm64/boot/dts/rockchip/px30.dtsi
> > +++ b/arch/arm64/boot/dts/rockchip/px30.dtsi
> > @@ -1102,6 +1102,17 @@ vopl_mmu: iommu@ff470f00 {
> >  		status = "disabled";
> >  	};
> >  
> > +	rga: rga@ff480000 {
> > +		compatible = "rockchip,px30-rga", "rockchip,rk3288-rga";
> > +		reg = <0x0 0xff480000 0x0 0x10000>;
> > +		interrupts = <GIC_SPI 76 IRQ_TYPE_LEVEL_HIGH 0>;
> > +		clocks = <&cru ACLK_RGA>, <&cru HCLK_RGA>, <&cru SCLK_RGA_CORE>;
> > +		clock-names = "aclk", "hclk", "sclk";
> 
> > +		resets = <&cru SRST_RGA>, <&cru SRST_RGA_A>, <&cru SRST_RGA_H>;
> > +		reset-names = "core", "axi", "ahb";
> > +		power-domains = <&power PX30_PD_VO>;
> 
> sort
> 
> 		power-domains = <&power PX30_PD_VO>;
> 		resets = <&cru SRST_RGA>, <&cru SRST_RGA_A>, <&cru SRST_RGA_H>;
> 		reset-names = "core", "axi", "ahb";

What's the rationale behind this (besides alphabetic sorting, which I don't
believe is a rule for dt properties)? Some nodes above in the file have it in
the same order that I do, and I like to see clocks followed by resets.

Cheers,

Paul

> 
> 
> > +	};
> > +
> >  	qos_gmac: qos@ff518000 {
> >  		compatible = "syscon";
> >  		reg = <0x0 0xff518000 0x0 0x20>;
> > -- 
> > 2.26.0
> 

-- 
Paul Kocialkowski, Bootlin
Embedded Linux and kernel engineering
https://bootlin.com

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v3 1/4] dt-bindings: rockchip-rga: Add PX30 compatible

[Johan Jonker]

Hi Paul,

With help of enum each additional compatibility string with fall back
'rockchip,rk3288-rga' adds only 1 extra line instead of 3.

See my and Heiko's response at the review of 'rockchip-saradc.yaml'.

Re: [PATCH v1 1/3] dt-bindings: iio: adc: convert rockchip saradc
bindings to yaml
https://lore.kernel.org/lkml/a35bdff4-601e-6186-584e-9a0b88cf3dbb@gmail.com/

The response of robh when I did something similar wrong as this patch.

Re: [PATCH 1/2] dt-bindings: usb: dwc2: add compatible property for
rk3328 usb
https://lore.kernel.org/lkml/20200310192933.GA15236@bogus/

Example of an approved patch with enum.

[PATCH v2 1/2] dt-bindings: usb: dwc2: add compatible property for
rk3328 usb
https://lore.kernel.org/lkml/20200311122121.8912-1-jbx6244@gmail.com/

Kind regards,

Johan

On 5/7/20 10:23 PM, Paul Kocialkowski wrote:
> Hi,
> 
> On Thu 30 Apr 20, 23:24, Johan Jonker wrote:
>> Hi Paul,
>>
>>>
>>> Add a new compatible for the PX30 Rockchip SoC, which also features
>>> a RGA block. It is compatible with the RK3288 RGA block.
>>>
>>> Signed-off-by: Paul Kocialkowski <paul.kocialkowski@bootlin.com>
>>> ---
>>>  Documentation/devicetree/bindings/media/rockchip-rga.yaml | 3 +++
>>>  1 file changed, 3 insertions(+)
>>>
>>> diff --git a/Documentation/devicetree/bindings/media/rockchip-rga.yaml b/Documentation/devicetree/bindings/media/rockchip-rga.yaml
>>> index dd645ddccb07..740586299da9 100644
>>> --- a/Documentation/devicetree/bindings/media/rockchip-rga.yaml
>>> +++ b/Documentation/devicetree/bindings/media/rockchip-rga.yaml
>>> @@ -23,6 +23,9 @@ properties:
>>
>>
>>>        - items:
>>>            - const: rockchip,rk3228-rga
>>>            - const: rockchip,rk3288-rga
>>> +      - items:
>>> +          - const: rockchip,px30-rga
>>> +          - const: rockchip,rk3288-rga
>>
>> Use enum.
>>
>>       - items:
>>           - enum:
>>             - rockchip,px30-rga
>>             - rockchip,rk3228-rga
>>           - const: rockchip,rk3288-rga
> 
> Are you sure about this? The rk3228 above does it as I did it and other examples
> like allwinner,sun4i-a10-csi.yaml appear to be doing the same too.

The use of enum starts from 2 or more identical fall back strings.
'allwinner,sun4i-a10-csi.yaml' has 2 different fall back strings.

properties:
  compatible:
    oneOf:
      - const: allwinner,sun4i-a10-csi1
      - const: allwinner,sun7i-a20-csi0
      - items:
        - const: allwinner,sun7i-a20-csi1
        - const: allwinner,sun4i-a10-csi1
      - items:
        - const: allwinner,sun8i-r40-csi0
        - const: allwinner,sun7i-a20-csi0

> 
> The case with rockchip,rk3288-rga alone already seems covered.
See yaml examples in the links above.

> 
> Cheers,
> 
> Paul
> 
>>>  
>>>    reg:
>>>      maxItems: 1
>>> -- 
>>> 2.26.0
>>
> 

Re: [PATCH v3 2/4] arm64: dts: rockchip: Add RGA support to the PX30

[Johan Jonker]

Hi Paul,

On 5/7/20 10:25 PM, Paul Kocialkowski wrote:
> Hi,
> 
> On Fri 01 May 20, 00:05, Johan Jonker wrote:
>> Hi Paul,
>>
>>> The PX30 features a RGA block: add the necessary node to support it.
>>>
>>> Signed-off-by: Paul Kocialkowski <paul.kocialkowski@bootlin.com>
>>> ---
>>>  arch/arm64/boot/dts/rockchip/px30.dtsi | 11 +++++++++++
>>>  1 file changed, 11 insertions(+)
>>>
>>> diff --git a/arch/arm64/boot/dts/rockchip/px30.dtsi b/arch/arm64/boot/dts/rockchip/px30.dtsi
>>> index f809dd6d5dc3..3de70aa4f1ce 100644
>>> --- a/arch/arm64/boot/dts/rockchip/px30.dtsi
>>> +++ b/arch/arm64/boot/dts/rockchip/px30.dtsi
>>> @@ -1102,6 +1102,17 @@ vopl_mmu: iommu@ff470f00 {
>>>  		status = "disabled";
>>>  	};
>>>  
>>> +	rga: rga@ff480000 {
>>> +		compatible = "rockchip,px30-rga", "rockchip,rk3288-rga";
>>> +		reg = <0x0 0xff480000 0x0 0x10000>;
>>> +		interrupts = <GIC_SPI 76 IRQ_TYPE_LEVEL_HIGH 0>;
>>> +		clocks = <&cru ACLK_RGA>, <&cru HCLK_RGA>, <&cru SCLK_RGA_CORE>;
>>> +		clock-names = "aclk", "hclk", "sclk";
>>
>>> +		resets = <&cru SRST_RGA>, <&cru SRST_RGA_A>, <&cru SRST_RGA_H>;
>>> +		reset-names = "core", "axi", "ahb";
>>> +		power-domains = <&power PX30_PD_VO>;
>>
>> sort
>>
>> 		power-domains = <&power PX30_PD_VO>;
>> 		resets = <&cru SRST_RGA>, <&cru SRST_RGA_A>, <&cru SRST_RGA_H>;
>> 		reset-names = "core", "axi", "ahb";
> 
> What's the rationale behind this (besides alphabetic sorting, which I don't
> believe is a rule for dt properties)? Some nodes above in the file have it in
> the same order that I do, and I like to see clocks followed by resets.

My short list.
There is no hard rule... It mostly depend on Heiko...

For nodes:
If exists on top: model, compatible and chosen.
Sort things without reg alphabetical first,
then sort the rest by reg address.

Inside nodes:
If exists on top: compatible, reg and interrupts.
In alphabetical order the required properties.
Then in alphabetical order the other properties.
And as last things that start with '#' in alphabetical order.
Add status below all other properties for soc internal components with
any board-specifics.
Keep an empty line between properties and nodes.

Exceptions:
Sort pinctrl-0 above pinctrl-names, so it stays in line with clock-names
and dma-names.
Sort simple-audio-card,name above other simple-audio-card properties.
Sort regulator-name above other regulator properties.
Sort regulator-min-microvolt above regulator-max-microvolt.

> 
> Cheers,
> 
> Paul
> 
>>
>>
>>> +	};
>>> +
>>>  	qos_gmac: qos@ff518000 {
>>>  		compatible = "syscon";
>>>  		reg = <0x0 0xff518000 0x0 0x20>;
>>> -- 
>>> 2.26.0
>>
> 

Re: [PATCH v3 3/4] media: rockchip: rga: Introduce color fmt macros and refactor CSC mode logic

[Ezequiel Garcia]

On Thu, 2020-04-30 at 18:42 +0200, Paul Kocialkowski wrote:
> This introduces two macros: RGA_COLOR_FMT_IS_YUV and RGA_COLOR_FMT_IS_RGB
> which allow quick checking of the colorspace familily of a RGA color format.
> 
> These macros are then used to refactor the logic for CSC mode selection.
> The two nested tests for input colorspace are simplified into a single one,
> with a logical and, making the whole more readable.
> 
> Signed-off-by: Paul Kocialkowski <paul.kocialkowski@bootlin.com>

Reviewed-by: Ezequiel Garcia <ezequiel@collabora.com>

Thanks a lot for the nice cleanup.

> ---
>  drivers/media/platform/rockchip/rga/rga-hw.c | 23 +++++++++-----------
>  drivers/media/platform/rockchip/rga/rga-hw.h |  5 +++++
>  2 files changed, 15 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/media/platform/rockchip/rga/rga-hw.c b/drivers/media/platform/rockchip/rga/rga-hw.c
> index 4be6dcf292ff..5607ee8d1917 100644
> --- a/drivers/media/platform/rockchip/rga/rga-hw.c
> +++ b/drivers/media/platform/rockchip/rga/rga-hw.c
> @@ -200,22 +200,19 @@ static void rga_cmd_set_trans_info(struct rga_ctx *ctx)
>  	dst_info.data.format = ctx->out.fmt->hw_format;
>  	dst_info.data.swap = ctx->out.fmt->color_swap;
>  
> -	if (ctx->in.fmt->hw_format >= RGA_COLOR_FMT_YUV422SP) {
> -		if (ctx->out.fmt->hw_format < RGA_COLOR_FMT_YUV422SP) {
> -			switch (ctx->in.colorspace) {
> -			case V4L2_COLORSPACE_REC709:
> -				src_info.data.csc_mode =
> -					RGA_SRC_CSC_MODE_BT709_R0;
> -				break;
> -			default:
> -				src_info.data.csc_mode =
> -					RGA_SRC_CSC_MODE_BT601_R0;
> -				break;
> -			}
> +	if (RGA_COLOR_FMT_IS_YUV(ctx->in.fmt->hw_format) &&
> +	    RGA_COLOR_FMT_IS_RGB(ctx->out.fmt->hw_format)) {
> +		switch (ctx->in.colorspace) {
> +		case V4L2_COLORSPACE_REC709:
> +			src_info.data.csc_mode = RGA_SRC_CSC_MODE_BT709_R0;
> +			break;
> +		default:
> +			src_info.data.csc_mode = RGA_SRC_CSC_MODE_BT601_R0;
> +			break;
>  		}
>  	}
>  
> -	if (ctx->out.fmt->hw_format >= RGA_COLOR_FMT_YUV422SP) {
> +	if (RGA_COLOR_FMT_IS_YUV(ctx->out.fmt->hw_format)) {
>  		switch (ctx->out.colorspace) {
>  		case V4L2_COLORSPACE_REC709:
>  			dst_info.data.csc_mode = RGA_SRC_CSC_MODE_BT709_R0;
> diff --git a/drivers/media/platform/rockchip/rga/rga-hw.h b/drivers/media/platform/rockchip/rga/rga-hw.h
> index 96cb0314dfa7..e8917e5630a4 100644
> --- a/drivers/media/platform/rockchip/rga/rga-hw.h
> +++ b/drivers/media/platform/rockchip/rga/rga-hw.h
> @@ -95,6 +95,11 @@
>  #define RGA_COLOR_FMT_CP_8BPP 15
>  #define RGA_COLOR_FMT_MASK 15
>  
> +#define RGA_COLOR_FMT_IS_YUV(fmt) \
> +	(((fmt) >= RGA_COLOR_FMT_YUV422SP) && ((fmt) < RGA_COLOR_FMT_CP_1BPP))
> +#define RGA_COLOR_FMT_IS_RGB(fmt) \
> +	((fmt) < RGA_COLOR_FMT_YUV422SP)
> +
>  #define RGA_COLOR_NONE_SWAP 0
>  #define RGA_COLOR_RB_SWAP 1
>  #define RGA_COLOR_ALPHA_SWAP 2

Re: [PATCH v3 4/4] media: rockchip: rga: Only set output CSC mode for RGB input

[Ezequiel Garcia]

On Thu, 2020-04-30 at 18:42 +0200, Paul Kocialkowski wrote:
> Setting the output CSC mode is required for a YUV output, but must not
> be set when the input is also YUV. Doing this (as tested with a YUV420P
> to YUV420P conversion) results in wrong colors.
> 
> Adapt the logic to only set the output CSC mode when the output is YUV and
> the input is RGB. Also add a comment to clarify the rationale.
> 
> Fixes: f7e7b48e6d79 ("[media] rockchip/rga: v4l2 m2m support")
> Signed-off-by: Paul Kocialkowski <paul.kocialkowski@bootlin.com>

Reviewed-by: Ezequiel Garcia <ezequiel@collabora.com>

Thanks!

> ---
>  drivers/media/platform/rockchip/rga/rga-hw.c | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/media/platform/rockchip/rga/rga-hw.c b/drivers/media/platform/rockchip/rga/rga-hw.c
> index 5607ee8d1917..aaa96f256356 100644
> --- a/drivers/media/platform/rockchip/rga/rga-hw.c
> +++ b/drivers/media/platform/rockchip/rga/rga-hw.c
> @@ -200,6 +200,11 @@ static void rga_cmd_set_trans_info(struct rga_ctx *ctx)
>  	dst_info.data.format = ctx->out.fmt->hw_format;
>  	dst_info.data.swap = ctx->out.fmt->color_swap;
>  
> +	/*
> +	 * CSC mode must only be set when the colorspace families differ between
> +	 * input and output. It must remain unset (zeroed) if both are the same.
> +	 */
> +
>  	if (RGA_COLOR_FMT_IS_YUV(ctx->in.fmt->hw_format) &&
>  	    RGA_COLOR_FMT_IS_RGB(ctx->out.fmt->hw_format)) {
>  		switch (ctx->in.colorspace) {
> @@ -212,7 +217,8 @@ static void rga_cmd_set_trans_info(struct rga_ctx *ctx)
>  		}
>  	}
>  
> -	if (RGA_COLOR_FMT_IS_YUV(ctx->out.fmt->hw_format)) {
> +	if (RGA_COLOR_FMT_IS_RGB(ctx->in.fmt->hw_format) &&
> +	    RGA_COLOR_FMT_IS_YUV(ctx->out.fmt->hw_format)) {
>  		switch (ctx->out.colorspace) {
>  		case V4L2_COLORSPACE_REC709:
>  			dst_info.data.csc_mode = RGA_SRC_CSC_MODE_BT709_R0;

Re: [PATCH v3 2/4] arm64: dts: rockchip: Add RGA support to the PX30

[Heiko Stuebner]

Am Freitag, 8. Mai 2020, 01:40:08 CEST schrieb Johan Jonker:
> Hi Paul,
> 
> On 5/7/20 10:25 PM, Paul Kocialkowski wrote:
> > Hi,
> > 
> > On Fri 01 May 20, 00:05, Johan Jonker wrote:
> >> Hi Paul,
> >>
> >>> The PX30 features a RGA block: add the necessary node to support it.
> >>>
> >>> Signed-off-by: Paul Kocialkowski <paul.kocialkowski@bootlin.com>
> >>> ---
> >>>  arch/arm64/boot/dts/rockchip/px30.dtsi | 11 +++++++++++
> >>>  1 file changed, 11 insertions(+)
> >>>
> >>> diff --git a/arch/arm64/boot/dts/rockchip/px30.dtsi b/arch/arm64/boot/dts/rockchip/px30.dtsi
> >>> index f809dd6d5dc3..3de70aa4f1ce 100644
> >>> --- a/arch/arm64/boot/dts/rockchip/px30.dtsi
> >>> +++ b/arch/arm64/boot/dts/rockchip/px30.dtsi
> >>> @@ -1102,6 +1102,17 @@ vopl_mmu: iommu@ff470f00 {
> >>>  		status = "disabled";
> >>>  	};
> >>>  
> >>> +	rga: rga@ff480000 {
> >>> +		compatible = "rockchip,px30-rga", "rockchip,rk3288-rga";
> >>> +		reg = <0x0 0xff480000 0x0 0x10000>;
> >>> +		interrupts = <GIC_SPI 76 IRQ_TYPE_LEVEL_HIGH 0>;
> >>> +		clocks = <&cru ACLK_RGA>, <&cru HCLK_RGA>, <&cru SCLK_RGA_CORE>;
> >>> +		clock-names = "aclk", "hclk", "sclk";
> >>
> >>> +		resets = <&cru SRST_RGA>, <&cru SRST_RGA_A>, <&cru SRST_RGA_H>;
> >>> +		reset-names = "core", "axi", "ahb";
> >>> +		power-domains = <&power PX30_PD_VO>;
> >>
> >> sort
> >>
> >> 		power-domains = <&power PX30_PD_VO>;
> >> 		resets = <&cru SRST_RGA>, <&cru SRST_RGA_A>, <&cru SRST_RGA_H>;
> >> 		reset-names = "core", "axi", "ahb";
> > 
> > What's the rationale behind this (besides alphabetic sorting, which I don't
> > believe is a rule for dt properties)? Some nodes above in the file have it in
> > the same order that I do, and I like to see clocks followed by resets.
> 
> My short list.
> There is no hard rule... It mostly depend on Heiko...

For the record, if needed I do any re-sorting myself normally, so there is
no need to respin patches just because nodes are sorted differently.

But yes, since the early Chromebook project in 2014 we agreed on
doing in Rockchip dts files:

----
compatible
reg
interrupts
[alphabetical]
status [if needed]
----

This works most of the time, but sometimes gets missed but is not _that_
big a deal if that happens ;-) .


Heiko


> For nodes:
> If exists on top: model, compatible and chosen.
> Sort things without reg alphabetical first,
> then sort the rest by reg address.
> 
> Inside nodes:
> If exists on top: compatible, reg and interrupts.
> In alphabetical order the required properties.
> Then in alphabetical order the other properties.
> And as last things that start with '#' in alphabetical order.
> Add status below all other properties for soc internal components with
> any board-specifics.
> Keep an empty line between properties and nodes.
> 
> Exceptions:
> Sort pinctrl-0 above pinctrl-names, so it stays in line with clock-names
> and dma-names.
> Sort simple-audio-card,name above other simple-audio-card properties.
> Sort regulator-name above other regulator properties.
> Sort regulator-min-microvolt above regulator-max-microvolt.
> 
> > 
> > Cheers,
> > 
> > Paul
> > 
> >>
> >>
> >>> +	};
> >>> +
> >>>  	qos_gmac: qos@ff518000 {
> >>>  		compatible = "syscon";
> >>>  		reg = <0x0 0xff518000 0x0 0x20>;
> >>
> > 
> 
>