[PATCH v2 0/4] soc: amlogic: add meson-canvas

[PATCH v2 0/4] soc: amlogic: add meson-canvas

[Maxime Jourdan]

Amlogic SoCs feature a set of 256 canvas that act as pixel buffer
descriptors. Some IPs like the display and video decoders access those
pixels by using canvas IDs rather than direct phy addresses.

As such, allocating/manipulating canvases can be done concurrently and
there is a need for a standalone, lock-aware canvas provider module.

Currently, canvas code lies in the drm/meson module as it is the sole
user.

This patchset adds such canvas provider module and converts drm/meson to
using it, stripping/moving the current canvas code.

Changes since v1: [0]
 - Convert ops struct to a public API
 - Added comments
 - Hid the of-node probe code behind meson_canvas_get
 - Changed device lock to a spinlock with irqsave

[0]: https://lkml.org/lkml/2018/8/1/1512

Maxime Jourdan (4):
  soc: amlogic: add meson-canvas driver
  dt-bindings: soc: amlogic: add meson-canvas documentation
  ARM64: dts: meson-gx: add dmcbus and canvas nodes.
  drm/meson: convert to the new canvas module

 .../bindings/display/amlogic,meson-vpu.txt    |   9 +-
 .../soc/amlogic/amlogic,meson-canvas.txt      |  36 ++++
 arch/arm64/boot/dts/amlogic/meson-gx.dtsi     |  24 ++-
 drivers/gpu/drm/meson/Kconfig                 |   1 +
 drivers/gpu/drm/meson/Makefile                |   2 +-
 drivers/gpu/drm/meson/meson_canvas.c          |  70 -------
 drivers/gpu/drm/meson/meson_canvas.h          |  42 ----
 drivers/gpu/drm/meson/meson_crtc.c            |   9 +-
 drivers/gpu/drm/meson/meson_drv.c             |  22 +--
 drivers/gpu/drm/meson/meson_drv.h             |   5 +-
 drivers/gpu/drm/meson/meson_plane.c           |   3 +-
 drivers/gpu/drm/meson/meson_viu.c             |   1 -
 drivers/soc/amlogic/Kconfig                   |   7 +
 drivers/soc/amlogic/Makefile                  |   1 +
 drivers/soc/amlogic/meson-canvas.c            | 185 ++++++++++++++++++
 include/linux/soc/amlogic/meson-canvas.h      |  65 ++++++
 16 files changed, 337 insertions(+), 145 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/soc/amlogic/amlogic,meson-canvas.txt
 delete mode 100644 drivers/gpu/drm/meson/meson_canvas.c
 delete mode 100644 drivers/gpu/drm/meson/meson_canvas.h
 create mode 100644 drivers/soc/amlogic/meson-canvas.c
 create mode 100644 include/linux/soc/amlogic/meson-canvas.h

-- 
2.18.0

[PATCH v2 2/4] dt-bindings: soc: amlogic: add meson-canvas documentation

[Maxime Jourdan]

DT bindings doc for amlogic,meson-canvas

Signed-off-by: Maxime Jourdan <maxi.jourdan@wanadoo.fr>
---
 .../soc/amlogic/amlogic,meson-canvas.txt      | 36 +++++++++++++++++++
 1 file changed, 36 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/soc/amlogic/amlogic,meson-canvas.txt

diff --git a/Documentation/devicetree/bindings/soc/amlogic/amlogic,meson-canvas.txt b/Documentation/devicetree/bindings/soc/amlogic/amlogic,meson-canvas.txt
new file mode 100644
index 000000000000..5f0351717bee
--- /dev/null
+++ b/Documentation/devicetree/bindings/soc/amlogic/amlogic,meson-canvas.txt
@@ -0,0 +1,36 @@
+Amlogic Canvas
+================================
+
+A canvas is a collection of metadata that describes a pixel buffer.
+Those metadata include: width, height, phyaddr, wrapping, block mode
+and endianness.
+
+Many IPs within Amlogic SoCs rely on canvas indexes to read/write pixel data
+rather than use the phy addresses directly. For instance, this is the case for
+the video decoders and the display.
+
+Amlogic SoCs have 256 canvas.
+
+Device Tree Bindings:
+---------------------
+
+Canvas Provider
+--------------------------
+
+Required properties:
+- compatible: "amlogic,canvas"
+
+Parent node should have the following properties :
+- compatible: "amlogic,gx-dmc-sysctrl", "syscon", "simple-mfd"
+- reg: base address and size of the DMC system control register space.
+
+Example:
+
+sysctrl_DMC: system-controller@0 {
+	compatible = "amlogic,gx-dmc-sysctrl", "syscon", "simple-mfd";
+	reg = <0x0 0x0 0x0 0x1000>;
+
+	canvas: canvas-provider@0 {
+		compatible = "amlogic,canvas";
+	};
+};
-- 
2.18.0

[PATCH v2 3/4] ARM64: dts: meson-gx: add dmcbus and canvas nodes.

[Maxime Jourdan]

Wrap the canvas node in a syscon node.

Signed-off-by: Maxime Jourdan <maxi.jourdan@wanadoo.fr>
---
 arch/arm64/boot/dts/amlogic/meson-gx.dtsi | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/arch/arm64/boot/dts/amlogic/meson-gx.dtsi b/arch/arm64/boot/dts/amlogic/meson-gx.dtsi
index b8dc4dbb391b..c98198662ae2 100644
--- a/arch/arm64/boot/dts/amlogic/meson-gx.dtsi
+++ b/arch/arm64/boot/dts/amlogic/meson-gx.dtsi
@@ -423,6 +423,23 @@
 			};
 		};
 
+		dmcbus: bus@c8838000 {
+			compatible = "simple-bus";
+			reg = <0x0 0xc8838000 0x0 0x1000>;
+			#address-cells = <2>;
+			#size-cells = <2>;
+			ranges = <0x0 0x0 0x0 0xc8838000 0x0 0x1000>;
+
+			sysctrl_DMC: system-controller@0 {
+				compatible = "amlogic,gx-dmc-sysctrl", "syscon", "simple-mfd";
+				reg = <0x0 0x0 0x0 0x1000>;
+
+				canvas: canvas-provider@0 {
+					compatible = "amlogic,canvas";
+				};
+			};
+		};
+
 		hiubus: bus@c883c000 {
 			compatible = "simple-bus";
 			reg = <0x0 0xc883c000 0x0 0x2000>;
-- 
2.18.0

Re: [PATCH v2 3/4] ARM64: dts: meson-gx: add dmcbus and canvas nodes.

[Jerome Brunet]

On Wed, 2018-08-08 at 00:00 +0200, Maxime Jourdan wrote:
> Wrap the canvas node in a syscon node.
> 
> Signed-off-by: Maxime Jourdan <maxi.jourdan@wanadoo.fr>
> ---
>  arch/arm64/boot/dts/amlogic/meson-gx.dtsi | 17 +++++++++++++++++
>  1 file changed, 17 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/amlogic/meson-gx.dtsi b/arch/arm64/boot/dts/amlogic/meson-gx.dtsi
> index b8dc4dbb391b..c98198662ae2 100644
> --- a/arch/arm64/boot/dts/amlogic/meson-gx.dtsi
> +++ b/arch/arm64/boot/dts/amlogic/meson-gx.dtsi
> @@ -423,6 +423,23 @@
>  			};
>  		};
>  
> +		dmcbus: bus@c8838000 {
> +			compatible = "simple-bus";
> +			reg = <0x0 0xc8838000 0x0 0x1000>;
> +			#address-cells = <2>;
> +			#size-cells = <2>;
> +			ranges = <0x0 0x0 0x0 0xc8838000 0x0 0x1000>;
> +
> +			sysctrl_DMC: system-controller@0 {
> +				compatible = "amlogic,gx-dmc-sysctrl", "syscon", "simple-mfd";
> +				reg = <0x0 0x0 0x0 0x1000>;
> +
> +				canvas: canvas-provider@0 {
> +					compatible = "amlogic,canvas";

If there is only one canvas provider under "sysctrl_DMC" and it has no reg
property , you should not put a unit-address (@0) here. (same for the
documentation patch)

You may have seen unit-address without a reg property used elsewhere (ASoC
simple-card, my recent axg-sound-card), when there is multiple node with the
same node-name (ex: dai-link).

As Martin pointed out, the DT spec says we should not use unit-address unless
there is a reg property. We did not get Rob's view on this and we might have to
update this later on. In your case, unless I missed something, you should
definitely not have it

nitpick regarding the node-name (canvas-provider). If appropriate, we should try
to stick to one of the generic names proposed in the spec. I wonder if the
canvas provider could be viewed as a "memory" or "memory-controller"

So, what about this ? Just a proposition, feel free to comment ;)

	sysctrl_DMC: system-controller@0 {							compatib
le = "amlogic,gx-dmc-sysctrl", "syscon", "simple-mfd";> 		reg =
<0x0 0x0 0x0 0x1000>;
	
		canvas: memory-controller {
			compatible = "amlogic,canvas";
		}

[...]


> +				};
> +			};
> +		};
> +
>  		hiubus: bus@c883c000 {
>  			compatible = "simple-bus";
>  			reg = <0x0 0xc883c000 0x0 0x2000>;

Re: [PATCH v2 3/4] ARM64: dts: meson-gx: add dmcbus and canvas nodes.

[Neil Armstrong]

On 08/08/2018 10:41, Jerome Brunet wrote:
> On Wed, 2018-08-08 at 00:00 +0200, Maxime Jourdan wrote:
>> Wrap the canvas node in a syscon node.
>>
>> Signed-off-by: Maxime Jourdan <maxi.jourdan@wanadoo.fr>
>> ---
>>  arch/arm64/boot/dts/amlogic/meson-gx.dtsi | 17 +++++++++++++++++
>>  1 file changed, 17 insertions(+)
>>
>> diff --git a/arch/arm64/boot/dts/amlogic/meson-gx.dtsi b/arch/arm64/boot/dts/amlogic/meson-gx.dtsi
>> index b8dc4dbb391b..c98198662ae2 100644
>> --- a/arch/arm64/boot/dts/amlogic/meson-gx.dtsi
>> +++ b/arch/arm64/boot/dts/amlogic/meson-gx.dtsi
>> @@ -423,6 +423,23 @@
>>  			};
>>  		};
>>  
>> +		dmcbus: bus@c8838000 {
>> +			compatible = "simple-bus";
>> +			reg = <0x0 0xc8838000 0x0 0x1000>;
>> +			#address-cells = <2>;
>> +			#size-cells = <2>;
>> +			ranges = <0x0 0x0 0x0 0xc8838000 0x0 0x1000>;
>> +
>> +			sysctrl_DMC: system-controller@0 {
>> +				compatible = "amlogic,gx-dmc-sysctrl", "syscon", "simple-mfd";
>> +				reg = <0x0 0x0 0x0 0x1000>;
>> +
>> +				canvas: canvas-provider@0 {
>> +					compatible = "amlogic,canvas";
> 
> If there is only one canvas provider under "sysctrl_DMC" and it has no reg
> property , you should not put a unit-address (@0) here. (same for the
> documentation patch)
> 
> You may have seen unit-address without a reg property used elsewhere (ASoC
> simple-card, my recent axg-sound-card), when there is multiple node with the
> same node-name (ex: dai-link).
> 
> As Martin pointed out, the DT spec says we should not use unit-address unless
> there is a reg property. We did not get Rob's view on this and we might have to
> update this later on. In your case, unless I missed something, you should
> definitely not have it
> 
> nitpick regarding the node-name (canvas-provider). If appropriate, we should try
> to stick to one of the generic names proposed in the spec. I wonder if the
> canvas provider could be viewed as a "memory" or "memory-controller"

It's not really a memory or memory-controller, it's a Lookup Table on top of the memory controller,
nothing really matches here...

> 
> So, what about this ? Just a proposition, feel free to comment ;)
> 
> 	sysctrl_DMC: system-controller@0 {							compatib
> le = "amlogic,gx-dmc-sysctrl", "syscon", "simple-mfd";> 		reg =
> <0x0 0x0 0x0 0x1000>;
> 	
> 		canvas: memory-controller {
> 			compatible = "amlogic,canvas";
> 		}
> 
> [...]
> 
> 
>> +				};
>> +			};
>> +		};
>> +
>>  		hiubus: bus@c883c000 {
>>  			compatible = "simple-bus";
>>  			reg = <0x0 0xc883c000 0x0 0x2000>;
> 
> 

Re: [PATCH v2 3/4] ARM64: dts: meson-gx: add dmcbus and canvas nodes.

[Maxime Jourdan]

2018-08-09 9:53 GMT+02:00 Neil Armstrong <narmstrong@baylibre.com>:
> On 08/08/2018 10:41, Jerome Brunet wrote:
>> On Wed, 2018-08-08 at 00:00 +0200, Maxime Jourdan wrote:
>>> Wrap the canvas node in a syscon node.
>>>
>>> Signed-off-by: Maxime Jourdan <maxi.jourdan@wanadoo.fr>
>>> ---
>>>  arch/arm64/boot/dts/amlogic/meson-gx.dtsi | 17 +++++++++++++++++
>>>  1 file changed, 17 insertions(+)
>>>
>>> diff --git a/arch/arm64/boot/dts/amlogic/meson-gx.dtsi b/arch/arm64/boot/dts/amlogic/meson-gx.dtsi
>>> index b8dc4dbb391b..c98198662ae2 100644
>>> --- a/arch/arm64/boot/dts/amlogic/meson-gx.dtsi
>>> +++ b/arch/arm64/boot/dts/amlogic/meson-gx.dtsi
>>> @@ -423,6 +423,23 @@
>>>                      };
>>>              };
>>>
>>> +            dmcbus: bus@c8838000 {
>>> +                    compatible = "simple-bus";
>>> +                    reg = <0x0 0xc8838000 0x0 0x1000>;
>>> +                    #address-cells = <2>;
>>> +                    #size-cells = <2>;
>>> +                    ranges = <0x0 0x0 0x0 0xc8838000 0x0 0x1000>;
>>> +
>>> +                    sysctrl_DMC: system-controller@0 {
>>> +                            compatible = "amlogic,gx-dmc-sysctrl", "syscon", "simple-mfd";
>>> +                            reg = <0x0 0x0 0x0 0x1000>;
>>> +
>>> +                            canvas: canvas-provider@0 {
>>> +                                    compatible = "amlogic,canvas";
>>
>> If there is only one canvas provider under "sysctrl_DMC" and it has no reg
>> property , you should not put a unit-address (@0) here. (same for the
>> documentation patch)
>>
>> You may have seen unit-address without a reg property used elsewhere (ASoC
>> simple-card, my recent axg-sound-card), when there is multiple node with the
>> same node-name (ex: dai-link).

Ack.

>> As Martin pointed out, the DT spec says we should not use unit-address unless
>> there is a reg property. We did not get Rob's view on this and we might have to
>> update this later on. In your case, unless I missed something, you should
>> definitely not have it
>>
>> nitpick regarding the node-name (canvas-provider). If appropriate, we should try
>> to stick to one of the generic names proposed in the spec. I wonder if the
>> canvas provider could be viewed as a "memory" or "memory-controller"
>
> It's not really a memory or memory-controller, it's a Lookup Table on top of the memory controller,
> nothing really matches here...

Thanks for the comments. It will be "video-lut" as suggested.

>>
>> So, what about this ? Just a proposition, feel free to comment ;)
>>
>>       sysctrl_DMC: system-controller@0 {                                                      compatib
>> le = "amlogic,gx-dmc-sysctrl", "syscon", "simple-mfd";>               reg =
>> <0x0 0x0 0x0 0x1000>;
>>
>>               canvas: memory-controller {
>>                       compatible = "amlogic,canvas";
>>               }
>>
>> [...]
>>
>>
>>> +                            };
>>> +                    };
>>> +            };
>>> +
>>>              hiubus: bus@c883c000 {
>>>                      compatible = "simple-bus";
>>>                      reg = <0x0 0xc883c000 0x0 0x2000>;
>>
>>
>

Re: [PATCH v2 2/4] dt-bindings: soc: amlogic: add meson-canvas documentation

[Rob Herring]

On Wed, Aug 08, 2018 at 12:00:09AM +0200, Maxime Jourdan wrote:
> DT bindings doc for amlogic,meson-canvas
> 
> Signed-off-by: Maxime Jourdan <maxi.jourdan@wanadoo.fr>
> ---
>  .../soc/amlogic/amlogic,meson-canvas.txt      | 36 +++++++++++++++++++
>  1 file changed, 36 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/soc/amlogic/amlogic,meson-canvas.txt
> 
> diff --git a/Documentation/devicetree/bindings/soc/amlogic/amlogic,meson-canvas.txt b/Documentation/devicetree/bindings/soc/amlogic/amlogic,meson-canvas.txt
> new file mode 100644
> index 000000000000..5f0351717bee
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/soc/amlogic/amlogic,meson-canvas.txt
> @@ -0,0 +1,36 @@
> +Amlogic Canvas
> +================================
> +
> +A canvas is a collection of metadata that describes a pixel buffer.
> +Those metadata include: width, height, phyaddr, wrapping, block mode
> +and endianness.
> +
> +Many IPs within Amlogic SoCs rely on canvas indexes to read/write pixel data
> +rather than use the phy addresses directly. For instance, this is the case for
> +the video decoders and the display.
> +
> +Amlogic SoCs have 256 canvas.
> +
> +Device Tree Bindings:
> +---------------------
> +
> +Canvas Provider
> +--------------------------
> +
> +Required properties:
> +- compatible: "amlogic,canvas"
> +
> +Parent node should have the following properties :
> +- compatible: "amlogic,gx-dmc-sysctrl", "syscon", "simple-mfd"

Is this documented somewhere? One child function is not a reason for an 
MFD and child nodes. And child nodes like this with no resources are 
unnecessary.

> +- reg: base address and size of the DMC system control register space.
> +
> +Example:
> +
> +sysctrl_DMC: system-controller@0 {
> +	compatible = "amlogic,gx-dmc-sysctrl", "syscon", "simple-mfd";
> +	reg = <0x0 0x0 0x0 0x1000>;
> +
> +	canvas: canvas-provider@0 {
> +		compatible = "amlogic,canvas";
> +	};
> +};
> -- 
> 2.18.0
> 
> --
> To unsubscribe from this list: send the line "unsubscribe devicetree" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 2/4] dt-bindings: soc: amlogic: add meson-canvas documentation

[Maxime Jourdan]

2018-08-13 21:07 GMT+02:00 Rob Herring <robh@kernel.org>:
> On Wed, Aug 08, 2018 at 12:00:09AM +0200, Maxime Jourdan wrote:
>> DT bindings doc for amlogic,meson-canvas
>>
>> Signed-off-by: Maxime Jourdan <maxi.jourdan@wanadoo.fr>
>> ---
>>  .../soc/amlogic/amlogic,meson-canvas.txt      | 36 +++++++++++++++++++
>>  1 file changed, 36 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/soc/amlogic/amlogic,meson-canvas.txt
>>
>> diff --git a/Documentation/devicetree/bindings/soc/amlogic/amlogic,meson-canvas.txt b/Documentation/devicetree/bindings/soc/amlogic/amlogic,meson-canvas.txt
>> new file mode 100644
>> index 000000000000..5f0351717bee
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/soc/amlogic/amlogic,meson-canvas.txt
>> @@ -0,0 +1,36 @@
>> +Amlogic Canvas
>> +================================
>> +
>> +A canvas is a collection of metadata that describes a pixel buffer.
>> +Those metadata include: width, height, phyaddr, wrapping, block mode
>> +and endianness.
>> +
>> +Many IPs within Amlogic SoCs rely on canvas indexes to read/write pixel data
>> +rather than use the phy addresses directly. For instance, this is the case for
>> +the video decoders and the display.
>> +
>> +Amlogic SoCs have 256 canvas.
>> +
>> +Device Tree Bindings:
>> +---------------------
>> +
>> +Canvas Provider
>> +--------------------------
>> +
>> +Required properties:
>> +- compatible: "amlogic,canvas"
>> +
>> +Parent node should have the following properties :
>> +- compatible: "amlogic,gx-dmc-sysctrl", "syscon", "simple-mfd"
>
> Is this documented somewhere? One child function is not a reason for an
> MFD and child nodes. And child nodes like this with no resources are
> unnecessary.
>

Hi Rob, this was done to follow the same path as other buses on the
platform that have sysctrls in order to provide regmaps to the
devices.

I can see how it's not really necessary here though, would it be okay
with you if I turned "canvas" into a simple bus subnode, using __iomem
in the device ?

>> +- reg: base address and size of the DMC system control register space.
>> +
>> +Example:
>> +
>> +sysctrl_DMC: system-controller@0 {
>> +     compatible = "amlogic,gx-dmc-sysctrl", "syscon", "simple-mfd";
>> +     reg = <0x0 0x0 0x0 0x1000>;
>> +
>> +     canvas: canvas-provider@0 {
>> +             compatible = "amlogic,canvas";
>> +     };
>> +};
>> --
>> 2.18.0
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe devicetree" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 2/4] dt-bindings: soc: amlogic: add meson-canvas documentation

[Rob Herring]

On Mon, Aug 13, 2018 at 8:42 PM Maxime Jourdan <maxi.jourdan@wanadoo.fr> wrote:
>
> 2018-08-13 21:07 GMT+02:00 Rob Herring <robh@kernel.org>:
> > On Wed, Aug 08, 2018 at 12:00:09AM +0200, Maxime Jourdan wrote:
> >> DT bindings doc for amlogic,meson-canvas
> >>
> >> Signed-off-by: Maxime Jourdan <maxi.jourdan@wanadoo.fr>
> >> ---
> >>  .../soc/amlogic/amlogic,meson-canvas.txt      | 36 +++++++++++++++++++
> >>  1 file changed, 36 insertions(+)
> >>  create mode 100644 Documentation/devicetree/bindings/soc/amlogic/amlogic,meson-canvas.txt
> >>
> >> diff --git a/Documentation/devicetree/bindings/soc/amlogic/amlogic,meson-canvas.txt b/Documentation/devicetree/bindings/soc/amlogic/amlogic,meson-canvas.txt
> >> new file mode 100644
> >> index 000000000000..5f0351717bee
> >> --- /dev/null
> >> +++ b/Documentation/devicetree/bindings/soc/amlogic/amlogic,meson-canvas.txt
> >> @@ -0,0 +1,36 @@
> >> +Amlogic Canvas
> >> +================================
> >> +
> >> +A canvas is a collection of metadata that describes a pixel buffer.
> >> +Those metadata include: width, height, phyaddr, wrapping, block mode
> >> +and endianness.
> >> +
> >> +Many IPs within Amlogic SoCs rely on canvas indexes to read/write pixel data
> >> +rather than use the phy addresses directly. For instance, this is the case for
> >> +the video decoders and the display.
> >> +
> >> +Amlogic SoCs have 256 canvas.
> >> +
> >> +Device Tree Bindings:
> >> +---------------------
> >> +
> >> +Canvas Provider
> >> +--------------------------
> >> +
> >> +Required properties:
> >> +- compatible: "amlogic,canvas"
> >> +
> >> +Parent node should have the following properties :
> >> +- compatible: "amlogic,gx-dmc-sysctrl", "syscon", "simple-mfd"
> >
> > Is this documented somewhere? One child function is not a reason for an
> > MFD and child nodes. And child nodes like this with no resources are
> > unnecessary.
> >
>
> Hi Rob, this was done to follow the same path as other buses on the
> platform that have sysctrls in order to provide regmaps to the
> devices.
>
> I can see how it's not really necessary here though, would it be okay
> with you if I turned "canvas" into a simple bus subnode, using __iomem
> in the device ?

That's a driver issue that has little to do with the binding. You can
create a regmap for any node, "syscon" just does that automagically
for you.

Rob