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

[PATCH v2 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 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: Add support for the PX30 compatible
  media: rockchip: rga: Only set output CSC mode for RGB input

 .../bindings/media/rockchip-rga.yaml           |  1 +
 arch/arm64/boot/dts/rockchip/px30.dtsi         | 11 +++++++++++
 drivers/media/platform/rockchip/rga/rga-hw.c   | 18 +++++++++++-------
 drivers/media/platform/rockchip/rga/rga.c      |  3 +++
 4 files changed, 26 insertions(+), 7 deletions(-)

-- 
2.26.0

[PATCH v2 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.

Signed-off-by: Paul Kocialkowski <paul.kocialkowski-LDxbnhwyfcJBDgjK7y7TUQ@public.gmane.org>
---
 Documentation/devicetree/bindings/media/rockchip-rga.yaml | 1 +
 1 file changed, 1 insertion(+)

diff --git a/Documentation/devicetree/bindings/media/rockchip-rga.yaml b/Documentation/devicetree/bindings/media/rockchip-rga.yaml
index dd645ddccb07..841b70d787f9 100644
--- a/Documentation/devicetree/bindings/media/rockchip-rga.yaml
+++ b/Documentation/devicetree/bindings/media/rockchip-rga.yaml
@@ -18,6 +18,7 @@ maintainers:
 properties:
   compatible:
     oneOf:
+      - const: rockchip,px30-rga
       - const: rockchip,rk3288-rga
       - const: rockchip,rk3399-rga
       - items:
-- 
2.26.0

[PATCH v2 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..613703e3d0df 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";
+		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 v2 3/4] media: rockchip: rga: Add support for the PX30 compatible

[Paul Kocialkowski]

The PX30 SoC has a RGA block, so add the associated compatible to
support it.

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

diff --git a/drivers/media/platform/rockchip/rga/rga.c b/drivers/media/platform/rockchip/rga/rga.c
index 9d122429706e..4fb4615662b7 100644
--- a/drivers/media/platform/rockchip/rga/rga.c
+++ b/drivers/media/platform/rockchip/rga/rga.c
@@ -955,6 +955,9 @@ static const struct dev_pm_ops rga_pm = {
 };
 
 static const struct of_device_id rockchip_rga_match[] = {
+	{
+		.compatible = "rockchip,px30-rga",
+	},
 	{
 		.compatible = "rockchip,rk3288-rga",
 	},
-- 
2.26.0

Re: [PATCH v2 3/4] media: rockchip: rga: Add support for the PX30 compatible

[Ezequiel Garcia]

Hey Paul,

Thanks for the patch!

On Thu, 2020-04-23 at 22:09 +0200, Paul Kocialkowski wrote:
> The PX30 SoC has a RGA block, so add the associated compatible to
> support it.
> 
> Signed-off-by: Paul Kocialkowski <paul.kocialkowski-LDxbnhwyfcJBDgjK7y7TUQ@public.gmane.org>
> ---
>  drivers/media/platform/rockchip/rga/rga.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/media/platform/rockchip/rga/rga.c b/drivers/media/platform/rockchip/rga/rga.c
> index 9d122429706e..4fb4615662b7 100644
> --- a/drivers/media/platform/rockchip/rga/rga.c
> +++ b/drivers/media/platform/rockchip/rga/rga.c
> @@ -955,6 +955,9 @@ static const struct dev_pm_ops rga_pm = {
>  };
>  
>  static const struct of_device_id rockchip_rga_match[] = {
> +	{
> +		.compatible = "rockchip,px30-rga",
> +	},

Please note that if you don't have anything px30-specific,
then you don't need the compatible in the driver.

You can have something like:

compatible = "rockchip,px30-rga", "rockchip,rk3288-rga"

so you need to add it to the bindings. See Justin Swartz
recent patches for rk3228.

Down the road, if you find something specific for px30,
you can make the driver aware. 

Cheers,
Ezequiel

Re: [PATCH v2 3/4] media: rockchip: rga: Add support for the PX30 compatible

[Paul Kocialkowski]

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

Hi Ezequiel,

On Fri 24 Apr 20, 09:54, Ezequiel Garcia wrote:
> Hey Paul,
> 
> Thanks for the patch!
> 
> On Thu, 2020-04-23 at 22:09 +0200, Paul Kocialkowski wrote:
> > The PX30 SoC has a RGA block, so add the associated compatible to
> > support it.
> > 
> > Signed-off-by: Paul Kocialkowski <paul.kocialkowski-LDxbnhwyfcJBDgjK7y7TUQ@public.gmane.org>
> > ---
> >  drivers/media/platform/rockchip/rga/rga.c | 3 +++
> >  1 file changed, 3 insertions(+)
> > 
> > diff --git a/drivers/media/platform/rockchip/rga/rga.c b/drivers/media/platform/rockchip/rga/rga.c
> > index 9d122429706e..4fb4615662b7 100644
> > --- a/drivers/media/platform/rockchip/rga/rga.c
> > +++ b/drivers/media/platform/rockchip/rga/rga.c
> > @@ -955,6 +955,9 @@ static const struct dev_pm_ops rga_pm = {
> >  };
> >  
> >  static const struct of_device_id rockchip_rga_match[] = {
> > +	{
> > +		.compatible = "rockchip,px30-rga",
> > +	},
> 
> Please note that if you don't have anything px30-specific,
> then you don't need the compatible in the driver.
> 
> You can have something like:
> 
> compatible = "rockchip,px30-rga", "rockchip,rk3288-rga"
> 
> so you need to add it to the bindings. See Justin Swartz
> recent patches for rk3228.

Thanks for the instruction!

I've been a bit confused about that because RK3399 has its own compatible
(without a 2nd rk3288 compatible) although there's nothing different with it
either. All of these rockchip platforms come with what they call "RGA2", that
seems to have no variation across platforms (downstream rockchip even has a
single compatible for it).

Should we add the rk3288 compatible to the rk3399 dtsi? I guess we
can't remove it from the driver at this point, for backward compatibility
with previous dts (what a strange idea...).

> Down the road, if you find something specific for px30,
> you can make the driver aware. 

Makes sense, yes.

Cheers,

Paul

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

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

Re: [PATCH v2 3/4] media: rockchip: rga: Add support for the PX30 compatible

[Ezequiel Garcia]

On Fri, 2020-04-24 at 15:55 +0200, Paul Kocialkowski wrote:
> Hi Ezequiel,
> 
> On Fri 24 Apr 20, 09:54, Ezequiel Garcia wrote:
> > Hey Paul,
> > 
> > Thanks for the patch!
> > 
> > On Thu, 2020-04-23 at 22:09 +0200, Paul Kocialkowski wrote:
> > > The PX30 SoC has a RGA block, so add the associated compatible to
> > > support it.
> > > 
> > > Signed-off-by: Paul Kocialkowski <paul.kocialkowski-LDxbnhwyfcJBDgjK7y7TUQ@public.gmane.org>
> > > ---
> > >  drivers/media/platform/rockchip/rga/rga.c | 3 +++
> > >  1 file changed, 3 insertions(+)
> > > 
> > > diff --git a/drivers/media/platform/rockchip/rga/rga.c b/drivers/media/platform/rockchip/rga/rga.c
> > > index 9d122429706e..4fb4615662b7 100644
> > > --- a/drivers/media/platform/rockchip/rga/rga.c
> > > +++ b/drivers/media/platform/rockchip/rga/rga.c
> > > @@ -955,6 +955,9 @@ static const struct dev_pm_ops rga_pm = {
> > >  };
> > >  
> > >  static const struct of_device_id rockchip_rga_match[] = {
> > > +	{
> > > +		.compatible = "rockchip,px30-rga",
> > > +	},
> > 
> > Please note that if you don't have anything px30-specific,
> > then you don't need the compatible in the driver.
> > 
> > You can have something like:
> > 
> > compatible = "rockchip,px30-rga", "rockchip,rk3288-rga"
> > 
> > so you need to add it to the bindings. See Justin Swartz
> > recent patches for rk3228.
> 
> Thanks for the instruction!
> 
> I've been a bit confused about that because RK3399 has its own compatible
> (without a 2nd rk3288 compatible) although there's nothing different with it
> either. All of these rockchip platforms come with what they call "RGA2", that
> seems to have no variation across platforms (downstream rockchip even has a
> single compatible for it).
> 

Yep, and that's an anti pattern, so you can expect to see that elsewhere.

> Should we add the rk3288 compatible to the rk3399 dtsi? I guess we
> can't remove it from the driver at this point, for backward compatibility
> with previous dts (what a strange idea...).
> 

No, we don't necesarily need/have to do anything with rk3399,
not sure if there's any gain at this point.

> > Down the road, if you find something specific for px30,
> > you can make the driver aware. 
> 
> Makes sense, yes.
> 

Cool.

Thanks,
Ezequiel

[PATCH v2 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 CSC mode when the output is YUV and the
input is RGB.

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 | 18 +++++++++++-------
 1 file changed, 11 insertions(+), 7 deletions(-)

diff --git a/drivers/media/platform/rockchip/rga/rga-hw.c b/drivers/media/platform/rockchip/rga/rga-hw.c
index 4be6dcf292ff..cbffcf986ccf 100644
--- a/drivers/media/platform/rockchip/rga/rga-hw.c
+++ b/drivers/media/platform/rockchip/rga/rga-hw.c
@@ -216,13 +216,17 @@ static void rga_cmd_set_trans_info(struct rga_ctx *ctx)
 	}
 
 	if (ctx->out.fmt->hw_format >= RGA_COLOR_FMT_YUV422SP) {
-		switch (ctx->out.colorspace) {
-		case V4L2_COLORSPACE_REC709:
-			dst_info.data.csc_mode = RGA_SRC_CSC_MODE_BT709_R0;
-			break;
-		default:
-			dst_info.data.csc_mode = RGA_DST_CSC_MODE_BT601_R0;
-			break;
+		if (ctx->in.fmt->hw_format < RGA_COLOR_FMT_YUV422SP) {
+			switch (ctx->out.colorspace) {
+			case V4L2_COLORSPACE_REC709:
+				dst_info.data.csc_mode =
+					RGA_SRC_CSC_MODE_BT709_R0;
+				break;
+			default:
+				dst_info.data.csc_mode =
+					RGA_DST_CSC_MODE_BT601_R0;
+				break;
+			}
 		}
 	}
 
-- 
2.26.0

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

[Ezequiel Garcia]

Hi Paul,

Thanks a lot for the patch.

I haven't had the chance to test this,
but I'd say you are fixing a long time issue here.

I really appreciate that.

On Thu, 2020-04-23 at 22:09 +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 CSC mode when the output is YUV and the
> input is RGB.
> 
> 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 | 18 +++++++++++-------
>  1 file changed, 11 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/media/platform/rockchip/rga/rga-hw.c b/drivers/media/platform/rockchip/rga/rga-hw.c
> index 4be6dcf292ff..cbffcf986ccf 100644
> --- a/drivers/media/platform/rockchip/rga/rga-hw.c
> +++ b/drivers/media/platform/rockchip/rga/rga-hw.c
> @@ -216,13 +216,17 @@ static void rga_cmd_set_trans_info(struct rga_ctx *ctx)
>  	}
>  
>  	if (ctx->out.fmt->hw_format >= RGA_COLOR_FMT_YUV422SP) {

Since we are already here touching this code, would you mind
adding another patch, to do some cleaning first?

First, replace the nested ifs with a boolean operator.
Then, introduce some IS_YUV (or IS_RGB) macro, making the above test
more like IS_YUV(out_hw_format).

Finally, perhaps a comment along the lines of your commit message:

"""
Setting the output CSC mode is required for a YUV output,
but must not be set when the input is also YUV.
"""

Details up to you :-)

After the clean-up patch, which would be just cosmetics,
your fix should be cleaner and more clear.

Thanks,
Ezequiel
 
> -		switch (ctx->out.colorspace) {
> -		case V4L2_COLORSPACE_REC709:
> -			dst_info.data.csc_mode = RGA_SRC_CSC_MODE_BT709_R0;
> -			break;
> -		default:
> -			dst_info.data.csc_mode = RGA_DST_CSC_MODE_BT601_R0;
> -			break;
> +		if (ctx->in.fmt->hw_format < RGA_COLOR_FMT_YUV422SP) {
> +			switch (ctx->out.colorspace) {
> +			case V4L2_COLORSPACE_REC709:
> +				dst_info.data.csc_mode =
> +					RGA_SRC_CSC_MODE_BT709_R0;
> +				break;
> +			default:
> +				dst_info.data.csc_mode =
> +					RGA_DST_CSC_MODE_BT601_R0;
> +				break;
> +			}
>  		}
>  	}
>  

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

[Paul Kocialkowski]

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

Hi Ezequiel,

On Sat 25 Apr 20, 10:46, Ezequiel Garcia wrote:
> Hi Paul,
> 
> Thanks a lot for the patch.
> 
> I haven't had the chance to test this,
> but I'd say you are fixing a long time issue here.
> 
> I really appreciate that.
> 
> On Thu, 2020-04-23 at 22:09 +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 CSC mode when the output is YUV and the
> > input is RGB.
> > 
> > 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 | 18 +++++++++++-------
> >  1 file changed, 11 insertions(+), 7 deletions(-)
> > 
> > diff --git a/drivers/media/platform/rockchip/rga/rga-hw.c b/drivers/media/platform/rockchip/rga/rga-hw.c
> > index 4be6dcf292ff..cbffcf986ccf 100644
> > --- a/drivers/media/platform/rockchip/rga/rga-hw.c
> > +++ b/drivers/media/platform/rockchip/rga/rga-hw.c
> > @@ -216,13 +216,17 @@ static void rga_cmd_set_trans_info(struct rga_ctx *ctx)
> >  	}
> >  
> >  	if (ctx->out.fmt->hw_format >= RGA_COLOR_FMT_YUV422SP) {
> 
> Since we are already here touching this code, would you mind
> adding another patch, to do some cleaning first?
> 
> First, replace the nested ifs with a boolean operator.
> Then, introduce some IS_YUV (or IS_RGB) macro, making the above test
> more like IS_YUV(out_hw_format).
> 
> Finally, perhaps a comment along the lines of your commit message:
> 
> """
> Setting the output CSC mode is required for a YUV output,
> but must not be set when the input is also YUV.
> """
> 
> Details up to you :-)
> 
> After the clean-up patch, which would be just cosmetics,
> your fix should be cleaner and more clear.

All done in v3, thanks for the feedback :)

Cheers,

Paul

> Thanks,
> Ezequiel
>  
> > -		switch (ctx->out.colorspace) {
> > -		case V4L2_COLORSPACE_REC709:
> > -			dst_info.data.csc_mode = RGA_SRC_CSC_MODE_BT709_R0;
> > -			break;
> > -		default:
> > -			dst_info.data.csc_mode = RGA_DST_CSC_MODE_BT601_R0;
> > -			break;
> > +		if (ctx->in.fmt->hw_format < RGA_COLOR_FMT_YUV422SP) {
> > +			switch (ctx->out.colorspace) {
> > +			case V4L2_COLORSPACE_REC709:
> > +				dst_info.data.csc_mode =
> > +					RGA_SRC_CSC_MODE_BT709_R0;
> > +				break;
> > +			default:
> > +				dst_info.data.csc_mode =
> > +					RGA_DST_CSC_MODE_BT601_R0;
> > +				break;
> > +			}
> >  		}
> >  	}
> >  
> 
> 

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

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