[PATCH v2 0/7] drm/simpledrm: Support system memory framebuffers

[PATCH v2 0/7] drm/simpledrm: Support system memory framebuffers

[Thierry Reding]

From: Thierry Reding <treding@nvidia.com>

Hi,

this series of patches adds support for framebuffers residing in system
memory to the simple-framebuffer DRM driver. To do this, the DT bindings
are extended do accept the memory-region property in addition to the reg
property for specifying the framebuffer memory. This is done because the
framebuffer memory will typically also need to be marked as reserved so
that the operating system will not reuse it and the memory-region
property is the standard property to reference reserved memory regions.

A new compatible string is documented to annotate the framebuffer memory
regions and the simpledrm driver has code added to bind such annotated
regions to the simple-framebuffer device.

The second half of the series then adds support for the XB24 and AB24
formats and ties it all together to provide a simple-framebuffer on
Jetson Xavier NX. It should be noted, though, that the Jetson Xavier NX
device tree nodes are placeholders only and it is expected that firmware
or a bootloader will fill these in at runtime, due to the variable
nature of the values that they contain.

This example also uses (but doesn't depend on) the iommu-addresses
property that has been proposed and which will hopefully be merged soon.

Version 1 of these patches can be found here:

	https://lore.kernel.org/all/20220905163300.391692-1-thierry.reding@gmail.com/

Changes in v2:
- DT fields are now cleared so that they can be filled in at runtime
- add XB24 support and treat AB24 the same (alpha bits are unused)
- consistently use struct iosys_map
- fix issues with DT bindings

I've tested these with a simple UEFI implementation that will fill in
the placeholder values and set the simple-framebuffer's status property
to "okay".

Thierry

Thierry Reding (7):
  dt-bindings: display: simple-framebuffer: Support system memory
    framebuffers
  dt-bindings: display: simple-framebuffer: Document 32-bit BGR format
  dt-bindings: reserved-memory: Support framebuffer reserved memory
  drm/simpledrm: Add support for system memory framebuffers
  drm/format-helper: Support the XB24 format
  drm/simpledrm: Support the XB24/AB24 format
  arm64: tegra: Add simple framebuffer on Jetson Xavier NX

 .../bindings/display/simple-framebuffer.yaml  |   7 +
 .../bindings/reserved-memory/framebuffer.yaml |  52 +++++
 .../nvidia/tegra194-p3509-0000+p3668-0001.dts |  33 ++++
 arch/arm64/boot/dts/nvidia/tegra194.dtsi      |   2 +-
 drivers/gpu/drm/drm_format_helper.c           |  37 ++++
 drivers/gpu/drm/tiny/simpledrm.c              | 179 ++++++++++++++----
 include/linux/platform_data/simplefb.h        |   1 +
 7 files changed, 274 insertions(+), 37 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/reserved-memory/framebuffer.yaml

-- 
2.37.3

[PATCH v2 0/7] drm/simpledrm: Support system memory framebuffers

[Thierry Reding]

From: Thierry Reding <treding@nvidia.com>

Hi,

this series of patches adds support for framebuffers residing in system
memory to the simple-framebuffer DRM driver. To do this, the DT bindings
are extended do accept the memory-region property in addition to the reg
property for specifying the framebuffer memory. This is done because the
framebuffer memory will typically also need to be marked as reserved so
that the operating system will not reuse it and the memory-region
property is the standard property to reference reserved memory regions.

A new compatible string is documented to annotate the framebuffer memory
regions and the simpledrm driver has code added to bind such annotated
regions to the simple-framebuffer device.

The second half of the series then adds support for the XB24 and AB24
formats and ties it all together to provide a simple-framebuffer on
Jetson Xavier NX. It should be noted, though, that the Jetson Xavier NX
device tree nodes are placeholders only and it is expected that firmware
or a bootloader will fill these in at runtime, due to the variable
nature of the values that they contain.

This example also uses (but doesn't depend on) the iommu-addresses
property that has been proposed and which will hopefully be merged soon.

Version 1 of these patches can be found here:

	https://lore.kernel.org/all/20220905163300.391692-1-thierry.reding@gmail.com/

Changes in v2:
- DT fields are now cleared so that they can be filled in at runtime
- add XB24 support and treat AB24 the same (alpha bits are unused)
- consistently use struct iosys_map
- fix issues with DT bindings

I've tested these with a simple UEFI implementation that will fill in
the placeholder values and set the simple-framebuffer's status property
to "okay".

Thierry

Thierry Reding (7):
  dt-bindings: display: simple-framebuffer: Support system memory
    framebuffers
  dt-bindings: display: simple-framebuffer: Document 32-bit BGR format
  dt-bindings: reserved-memory: Support framebuffer reserved memory
  drm/simpledrm: Add support for system memory framebuffers
  drm/format-helper: Support the XB24 format
  drm/simpledrm: Support the XB24/AB24 format
  arm64: tegra: Add simple framebuffer on Jetson Xavier NX

 .../bindings/display/simple-framebuffer.yaml  |   7 +
 .../bindings/reserved-memory/framebuffer.yaml |  52 +++++
 .../nvidia/tegra194-p3509-0000+p3668-0001.dts |  33 ++++
 arch/arm64/boot/dts/nvidia/tegra194.dtsi      |   2 +-
 drivers/gpu/drm/drm_format_helper.c           |  37 ++++
 drivers/gpu/drm/tiny/simpledrm.c              | 179 ++++++++++++++----
 include/linux/platform_data/simplefb.h        |   1 +
 7 files changed, 274 insertions(+), 37 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/reserved-memory/framebuffer.yaml

-- 
2.37.3

[PATCH v2 1/7] dt-bindings: display: simple-framebuffer: Support system memory framebuffers

[Thierry Reding]

From: Thierry Reding <treding@nvidia.com>

In order to support framebuffers residing in system memory, allow the
memory-region property to override the framebuffer memory specification
in the "reg" property.

Signed-off-by: Thierry Reding <treding@nvidia.com>
---
 .../devicetree/bindings/display/simple-framebuffer.yaml      | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/Documentation/devicetree/bindings/display/simple-framebuffer.yaml b/Documentation/devicetree/bindings/display/simple-framebuffer.yaml
index dd64f70b5014..3e9857eb002e 100644
--- a/Documentation/devicetree/bindings/display/simple-framebuffer.yaml
+++ b/Documentation/devicetree/bindings/display/simple-framebuffer.yaml
@@ -63,6 +63,11 @@ properties:
   reg:
     description: Location and size of the framebuffer memory
 
+  memory-region:
+    maxItems: 1
+    description: Phandle to a node describing the memory to be used for the
+      framebuffer. If present, overrides the "reg" property (if one exists).
+
   clocks:
     description: List of clocks used by the framebuffer.
 
-- 
2.37.3

[PATCH v2 1/7] dt-bindings: display: simple-framebuffer: Support system memory framebuffers

[Thierry Reding]

From: Thierry Reding <treding@nvidia.com>

In order to support framebuffers residing in system memory, allow the
memory-region property to override the framebuffer memory specification
in the "reg" property.

Signed-off-by: Thierry Reding <treding@nvidia.com>
---
 .../devicetree/bindings/display/simple-framebuffer.yaml      | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/Documentation/devicetree/bindings/display/simple-framebuffer.yaml b/Documentation/devicetree/bindings/display/simple-framebuffer.yaml
index dd64f70b5014..3e9857eb002e 100644
--- a/Documentation/devicetree/bindings/display/simple-framebuffer.yaml
+++ b/Documentation/devicetree/bindings/display/simple-framebuffer.yaml
@@ -63,6 +63,11 @@ properties:
   reg:
     description: Location and size of the framebuffer memory
 
+  memory-region:
+    maxItems: 1
+    description: Phandle to a node describing the memory to be used for the
+      framebuffer. If present, overrides the "reg" property (if one exists).
+
   clocks:
     description: List of clocks used by the framebuffer.
 
-- 
2.37.3

[PATCH v2 2/7] dt-bindings: display: simple-framebuffer: Document 32-bit BGR format

[Thierry Reding]

From: Thierry Reding <treding@nvidia.com>

This is a variant of the 32-bit RGB format where the red and blue
components are swapped.

Signed-off-by: Thierry Reding <treding@nvidia.com>
---
 .../devicetree/bindings/display/simple-framebuffer.yaml         | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/Documentation/devicetree/bindings/display/simple-framebuffer.yaml b/Documentation/devicetree/bindings/display/simple-framebuffer.yaml
index 3e9857eb002e..3c9f29e428a4 100644
--- a/Documentation/devicetree/bindings/display/simple-framebuffer.yaml
+++ b/Documentation/devicetree/bindings/display/simple-framebuffer.yaml
@@ -99,6 +99,7 @@ properties:
         * `x1r5g5b5` - 16-bit pixels, d[14:10]=r, d[9:5]=g, d[4:0]=b
         * `x2r10g10b10` - 32-bit pixels, d[29:20]=r, d[19:10]=g, d[9:0]=b
         * `x8r8g8b8` - 32-bit pixels, d[23:16]=r, d[15:8]=g, d[7:0]=b
+        * `x8b8g8r8` - 32-bit pixels, d[23:16]=b, d[15:8]=g, d[7:0]=r
     enum:
       - a1r5g5b5
       - a2r10g10b10
@@ -110,6 +111,7 @@ properties:
       - x1r5g5b5
       - x2r10g10b10
       - x8r8g8b8
+      - x8b8g8r8
 
   display:
     $ref: /schemas/types.yaml#/definitions/phandle
-- 
2.37.3

[PATCH v2 2/7] dt-bindings: display: simple-framebuffer: Document 32-bit BGR format

[Thierry Reding]

From: Thierry Reding <treding@nvidia.com>

This is a variant of the 32-bit RGB format where the red and blue
components are swapped.

Signed-off-by: Thierry Reding <treding@nvidia.com>
---
 .../devicetree/bindings/display/simple-framebuffer.yaml         | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/Documentation/devicetree/bindings/display/simple-framebuffer.yaml b/Documentation/devicetree/bindings/display/simple-framebuffer.yaml
index 3e9857eb002e..3c9f29e428a4 100644
--- a/Documentation/devicetree/bindings/display/simple-framebuffer.yaml
+++ b/Documentation/devicetree/bindings/display/simple-framebuffer.yaml
@@ -99,6 +99,7 @@ properties:
         * `x1r5g5b5` - 16-bit pixels, d[14:10]=r, d[9:5]=g, d[4:0]=b
         * `x2r10g10b10` - 32-bit pixels, d[29:20]=r, d[19:10]=g, d[9:0]=b
         * `x8r8g8b8` - 32-bit pixels, d[23:16]=r, d[15:8]=g, d[7:0]=b
+        * `x8b8g8r8` - 32-bit pixels, d[23:16]=b, d[15:8]=g, d[7:0]=r
     enum:
       - a1r5g5b5
       - a2r10g10b10
@@ -110,6 +111,7 @@ properties:
       - x1r5g5b5
       - x2r10g10b10
       - x8r8g8b8
+      - x8b8g8r8
 
   display:
     $ref: /schemas/types.yaml#/definitions/phandle
-- 
2.37.3

[PATCH v2 3/7] dt-bindings: reserved-memory: Support framebuffer reserved memory

[Thierry Reding]

From: Thierry Reding <treding@nvidia.com>

Document the "framebuffer" compatible string for reserved memory nodes
to annotate reserved memory regions used for framebuffer carveouts.

Signed-off-by: Thierry Reding <treding@nvidia.com>
---
Changes in v2:
- use four spaces for indentation in example (as recommended elsewhere)
- add explicit root node
- drop unneeded quotes

 .../bindings/reserved-memory/framebuffer.yaml | 52 +++++++++++++++++++
 1 file changed, 52 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/reserved-memory/framebuffer.yaml

diff --git a/Documentation/devicetree/bindings/reserved-memory/framebuffer.yaml b/Documentation/devicetree/bindings/reserved-memory/framebuffer.yaml
new file mode 100644
index 000000000000..05b6648b3458
--- /dev/null
+++ b/Documentation/devicetree/bindings/reserved-memory/framebuffer.yaml
@@ -0,0 +1,52 @@
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/reserved-memory/framebuffer.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: /reserved-memory framebuffer node bindings
+
+maintainers:
+  - devicetree-spec@vger.kernel.org
+
+allOf:
+  - $ref: reserved-memory.yaml
+
+properties:
+  compatible:
+    const: framebuffer
+    description: >
+      This indicates a region of memory meant to be used as a framebuffer for
+      a set of display devices. It can be used by an operating system to keep
+      the framebuffer from being overwritten and use it as the backing memory
+      for a display device (such as simple-framebuffer).
+
+unevaluatedProperties: false
+
+examples:
+  - |
+    / {
+        compatible = "foo";
+        model = "foo";
+        #address-cells = <1>;
+        #size-cells = <1>;
+
+        chosen {
+            framebuffer {
+                compatible = "simple-framebuffer";
+                memory-region = <&fb>;
+            };
+        };
+
+        reserved-memory {
+            #address-cells = <1>;
+            #size-cells = <1>;
+            ranges;
+
+            fb: framebuffer@80000000 {
+                compatible = "framebuffer";
+                reg = <0x80000000 0x007e9000>;
+            };
+        };
+    };
+...
-- 
2.37.3

[PATCH v2 3/7] dt-bindings: reserved-memory: Support framebuffer reserved memory

[Thierry Reding]

From: Thierry Reding <treding@nvidia.com>

Document the "framebuffer" compatible string for reserved memory nodes
to annotate reserved memory regions used for framebuffer carveouts.

Signed-off-by: Thierry Reding <treding@nvidia.com>
---
Changes in v2:
- use four spaces for indentation in example (as recommended elsewhere)
- add explicit root node
- drop unneeded quotes

 .../bindings/reserved-memory/framebuffer.yaml | 52 +++++++++++++++++++
 1 file changed, 52 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/reserved-memory/framebuffer.yaml

diff --git a/Documentation/devicetree/bindings/reserved-memory/framebuffer.yaml b/Documentation/devicetree/bindings/reserved-memory/framebuffer.yaml
new file mode 100644
index 000000000000..05b6648b3458
--- /dev/null
+++ b/Documentation/devicetree/bindings/reserved-memory/framebuffer.yaml
@@ -0,0 +1,52 @@
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/reserved-memory/framebuffer.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: /reserved-memory framebuffer node bindings
+
+maintainers:
+  - devicetree-spec@vger.kernel.org
+
+allOf:
+  - $ref: reserved-memory.yaml
+
+properties:
+  compatible:
+    const: framebuffer
+    description: >
+      This indicates a region of memory meant to be used as a framebuffer for
+      a set of display devices. It can be used by an operating system to keep
+      the framebuffer from being overwritten and use it as the backing memory
+      for a display device (such as simple-framebuffer).
+
+unevaluatedProperties: false
+
+examples:
+  - |
+    / {
+        compatible = "foo";
+        model = "foo";
+        #address-cells = <1>;
+        #size-cells = <1>;
+
+        chosen {
+            framebuffer {
+                compatible = "simple-framebuffer";
+                memory-region = <&fb>;
+            };
+        };
+
+        reserved-memory {
+            #address-cells = <1>;
+            #size-cells = <1>;
+            ranges;
+
+            fb: framebuffer@80000000 {
+                compatible = "framebuffer";
+                reg = <0x80000000 0x007e9000>;
+            };
+        };
+    };
+...
-- 
2.37.3

[PATCH v2 4/7] drm/simpledrm: Add support for system memory framebuffers

[Thierry Reding]

From: Thierry Reding <treding@nvidia.com>

Simple framebuffers can be set up in system memory, which cannot be
requested and/or I/O remapped using the I/O resource helpers. Add a
separate code path that obtains system memory framebuffers from the
reserved memory region referenced in the memory-region property.

v2: make screen base a struct iosys_map to avoid sparse warnings

Signed-off-by: Thierry Reding <treding@nvidia.com>
---
 drivers/gpu/drm/tiny/simpledrm.c | 177 ++++++++++++++++++++++++-------
 1 file changed, 141 insertions(+), 36 deletions(-)

diff --git a/drivers/gpu/drm/tiny/simpledrm.c b/drivers/gpu/drm/tiny/simpledrm.c
index 18489779fb8a..cf36f67d22e4 100644
--- a/drivers/gpu/drm/tiny/simpledrm.c
+++ b/drivers/gpu/drm/tiny/simpledrm.c
@@ -2,6 +2,7 @@
 
 #include <linux/clk.h>
 #include <linux/of_clk.h>
+#include <linux/of_reserved_mem.h>
 #include <linux/minmax.h>
 #include <linux/platform_data/simplefb.h>
 #include <linux/platform_device.h>
@@ -207,7 +208,9 @@ struct simpledrm_device {
 	unsigned int pitch;
 
 	/* memory management */
-	void __iomem *screen_base;
+	struct iosys_map screen_base;
+	phys_addr_t sysmem_start;
+	size_t sysmem_size;
 
 	/* modesetting */
 	uint32_t formats[8];
@@ -441,6 +444,106 @@ static int simpledrm_device_init_regulators(struct simpledrm_device *sdev)
 }
 #endif
 
+/*
+ * Memory management
+ */
+
+static int simpledrm_device_init_mm_sysmem(struct simpledrm_device *sdev, phys_addr_t start,
+					   size_t size)
+{
+	struct drm_device *dev = &sdev->dev;
+	phys_addr_t end = start + size - 1;
+	void *screen_base;
+
+	drm_info(dev, "using system memory framebuffer at [%pa-%pa]\n", &start, &end);
+
+	screen_base = devm_memremap(dev->dev, start, size, MEMREMAP_WC);
+	if (!screen_base)
+		return -ENOMEM;
+
+	iosys_map_set_vaddr(&sdev->screen_base, screen_base);
+
+	return 0;
+}
+
+static int simpledrm_device_init_mm_io(struct simpledrm_device *sdev, phys_addr_t start,
+				       size_t size)
+{
+	struct drm_device *dev = &sdev->dev;
+	phys_addr_t end = start + size - 1;
+	void __iomem *screen_base;
+	struct resource *mem;
+
+	drm_info(dev, "using I/O memory framebuffer at [%pa-%pa]\n", &start, &end);
+
+	mem = devm_request_mem_region(dev->dev, start, size, sdev->dev.driver->name);
+	if (!mem) {
+		/*
+		 * We cannot make this fatal. Sometimes this comes from magic
+		 * spaces our resource handlers simply don't know about. Use
+		 * the I/O-memory resource as-is and try to map that instead.
+		 */
+		drm_warn(dev, "could not acquire memory region [%pa-%pa]\n", &start, &end);
+	} else {
+		size = resource_size(mem);
+		start = mem->start;
+	}
+
+	screen_base = devm_ioremap_wc(dev->dev, start, size);
+	if (!screen_base)
+		return -ENOMEM;
+
+	iosys_map_set_vaddr_iomem(&sdev->screen_base, screen_base);
+
+	return 0;
+}
+
+static void simpledrm_device_exit_mm(void *data)
+{
+	struct simpledrm_device *sdev = data;
+	struct drm_device *dev = &sdev->dev;
+
+	of_reserved_mem_device_release(dev->dev);
+}
+
+static int simpledrm_device_init_mm(struct simpledrm_device *sdev)
+{
+	int (*init)(struct simpledrm_device *sdev, phys_addr_t start, size_t size);
+	struct drm_device *dev = &sdev->dev;
+	struct platform_device *pdev = to_platform_device(dev->dev);
+	phys_addr_t start, end;
+	size_t size;
+	int ret;
+
+	ret = of_reserved_mem_device_init_by_idx(dev->dev, dev->dev->of_node, 0);
+	if (ret) {
+		struct resource *res;
+
+		res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+		if (!res)
+			return -EINVAL;
+
+		init = simpledrm_device_init_mm_io;
+		size = resource_size(res);
+		start = res->start;
+	} else {
+		devm_add_action_or_reset(dev->dev, simpledrm_device_exit_mm, sdev);
+		init = simpledrm_device_init_mm_sysmem;
+		start = sdev->sysmem_start;
+		size = sdev->sysmem_size;
+	}
+
+	end = start + size - 1;
+
+	ret = devm_aperture_acquire_from_firmware(dev, start, size);
+	if (ret) {
+		drm_err(dev, "could not acquire memory range [%pa-%pa]: %d\n", &start, &end, ret);
+		return ret;
+	}
+
+	return init(sdev, start, size);
+}
+
 /*
  * Modesetting
  */
@@ -491,15 +594,15 @@ static void simpledrm_primary_plane_helper_atomic_update(struct drm_plane *plane
 
 	drm_atomic_helper_damage_iter_init(&iter, old_plane_state, plane_state);
 	drm_atomic_for_each_plane_damage(&iter, &damage) {
-		struct iosys_map dst = IOSYS_MAP_INIT_VADDR(sdev->screen_base);
 		struct drm_rect dst_clip = plane_state->dst;
 
 		if (!drm_rect_intersect(&dst_clip, &damage))
 			continue;
 
-		iosys_map_incr(&dst, drm_fb_clip_offset(sdev->pitch, sdev->format, &dst_clip));
-		drm_fb_blit(&dst, &sdev->pitch, sdev->format->format, shadow_plane_state->data, fb,
-			    &damage);
+		iosys_map_incr(&sdev->screen_base, drm_fb_clip_offset(sdev->pitch, sdev->format,
+								      &dst_clip));
+		drm_fb_blit(&sdev->screen_base, &sdev->pitch, sdev->format->format,
+			    shadow_plane_state->data, fb, &damage);
 	}
 
 	drm_dev_exit(idx);
@@ -518,7 +621,7 @@ static void simpledrm_primary_plane_helper_atomic_disable(struct drm_plane *plan
 		return;
 
 	/* Clear screen to black if disabled */
-	memset_io(sdev->screen_base, 0, sdev->pitch * sdev->mode.vdisplay);
+	iosys_map_memset(&sdev->screen_base, 0, 0, sdev->pitch * sdev->mode.vdisplay);
 
 	drm_dev_exit(idx);
 }
@@ -635,8 +738,6 @@ static struct simpledrm_device *simpledrm_device_create(struct drm_driver *drv,
 	struct drm_device *dev;
 	int width, height, stride;
 	const struct drm_format_info *format;
-	struct resource *res, *mem;
-	void __iomem *screen_base;
 	struct drm_plane *primary_plane;
 	struct drm_crtc *crtc;
 	struct drm_encoder *encoder;
@@ -706,35 +807,9 @@ static struct simpledrm_device *simpledrm_device_create(struct drm_driver *drv,
 	drm_dbg(dev, "framebuffer format=%p4cc, size=%dx%d, stride=%d byte\n",
 		&format->format, width, height, stride);
 
-	/*
-	 * Memory management
-	 */
-
-	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
-	if (!res)
-		return ERR_PTR(-EINVAL);
-
-	ret = devm_aperture_acquire_from_firmware(dev, res->start, resource_size(res));
-	if (ret) {
-		drm_err(dev, "could not acquire memory range %pr: error %d\n", res, ret);
+	ret = simpledrm_device_init_mm(sdev);
+	if (ret)
 		return ERR_PTR(ret);
-	}
-
-	mem = devm_request_mem_region(&pdev->dev, res->start, resource_size(res), drv->name);
-	if (!mem) {
-		/*
-		 * We cannot make this fatal. Sometimes this comes from magic
-		 * spaces our resource handlers simply don't know about. Use
-		 * the I/O-memory resource as-is and try to map that instead.
-		 */
-		drm_warn(dev, "could not acquire memory region %pr\n", res);
-		mem = res;
-	}
-
-	screen_base = devm_ioremap_wc(&pdev->dev, mem->start, resource_size(mem));
-	if (!screen_base)
-		return ERR_PTR(-ENOMEM);
-	sdev->screen_base = screen_base;
 
 	/*
 	 * Modesetting
@@ -878,5 +953,35 @@ static struct platform_driver simpledrm_platform_driver = {
 
 module_platform_driver(simpledrm_platform_driver);
 
+static int simple_framebuffer_device_init(struct reserved_mem *rmem, struct device *dev)
+{
+	struct simpledrm_device *sdev = dev_get_drvdata(dev);
+
+	sdev->sysmem_start = rmem->base;
+	sdev->sysmem_size = rmem->size;
+
+	return 0;
+}
+
+static void simple_framebuffer_device_release(struct reserved_mem *rmem, struct device *dev)
+{
+}
+
+static const struct reserved_mem_ops simple_framebuffer_ops = {
+	.device_init = simple_framebuffer_device_init,
+	.device_release = simple_framebuffer_device_release,
+};
+
+static int simple_framebuffer_init(struct reserved_mem *rmem)
+{
+	pr_info("framebuffer memory at %pa, size %lu bytes\n", &rmem->base,
+		(unsigned long)rmem->size);
+
+	rmem->ops = &simple_framebuffer_ops;
+
+	return 0;
+}
+RESERVEDMEM_OF_DECLARE(simple_framebuffer, "framebuffer", simple_framebuffer_init);
+
 MODULE_DESCRIPTION(DRIVER_DESC);
 MODULE_LICENSE("GPL v2");
-- 
2.37.3

[PATCH v2 4/7] drm/simpledrm: Add support for system memory framebuffers

[Thierry Reding]

From: Thierry Reding <treding@nvidia.com>

Simple framebuffers can be set up in system memory, which cannot be
requested and/or I/O remapped using the I/O resource helpers. Add a
separate code path that obtains system memory framebuffers from the
reserved memory region referenced in the memory-region property.

v2: make screen base a struct iosys_map to avoid sparse warnings

Signed-off-by: Thierry Reding <treding@nvidia.com>
---
 drivers/gpu/drm/tiny/simpledrm.c | 177 ++++++++++++++++++++++++-------
 1 file changed, 141 insertions(+), 36 deletions(-)

diff --git a/drivers/gpu/drm/tiny/simpledrm.c b/drivers/gpu/drm/tiny/simpledrm.c
index 18489779fb8a..cf36f67d22e4 100644
--- a/drivers/gpu/drm/tiny/simpledrm.c
+++ b/drivers/gpu/drm/tiny/simpledrm.c
@@ -2,6 +2,7 @@
 
 #include <linux/clk.h>
 #include <linux/of_clk.h>
+#include <linux/of_reserved_mem.h>
 #include <linux/minmax.h>
 #include <linux/platform_data/simplefb.h>
 #include <linux/platform_device.h>
@@ -207,7 +208,9 @@ struct simpledrm_device {
 	unsigned int pitch;
 
 	/* memory management */
-	void __iomem *screen_base;
+	struct iosys_map screen_base;
+	phys_addr_t sysmem_start;
+	size_t sysmem_size;
 
 	/* modesetting */
 	uint32_t formats[8];
@@ -441,6 +444,106 @@ static int simpledrm_device_init_regulators(struct simpledrm_device *sdev)
 }
 #endif
 
+/*
+ * Memory management
+ */
+
+static int simpledrm_device_init_mm_sysmem(struct simpledrm_device *sdev, phys_addr_t start,
+					   size_t size)
+{
+	struct drm_device *dev = &sdev->dev;
+	phys_addr_t end = start + size - 1;
+	void *screen_base;
+
+	drm_info(dev, "using system memory framebuffer at [%pa-%pa]\n", &start, &end);
+
+	screen_base = devm_memremap(dev->dev, start, size, MEMREMAP_WC);
+	if (!screen_base)
+		return -ENOMEM;
+
+	iosys_map_set_vaddr(&sdev->screen_base, screen_base);
+
+	return 0;
+}
+
+static int simpledrm_device_init_mm_io(struct simpledrm_device *sdev, phys_addr_t start,
+				       size_t size)
+{
+	struct drm_device *dev = &sdev->dev;
+	phys_addr_t end = start + size - 1;
+	void __iomem *screen_base;
+	struct resource *mem;
+
+	drm_info(dev, "using I/O memory framebuffer at [%pa-%pa]\n", &start, &end);
+
+	mem = devm_request_mem_region(dev->dev, start, size, sdev->dev.driver->name);
+	if (!mem) {
+		/*
+		 * We cannot make this fatal. Sometimes this comes from magic
+		 * spaces our resource handlers simply don't know about. Use
+		 * the I/O-memory resource as-is and try to map that instead.
+		 */
+		drm_warn(dev, "could not acquire memory region [%pa-%pa]\n", &start, &end);
+	} else {
+		size = resource_size(mem);
+		start = mem->start;
+	}
+
+	screen_base = devm_ioremap_wc(dev->dev, start, size);
+	if (!screen_base)
+		return -ENOMEM;
+
+	iosys_map_set_vaddr_iomem(&sdev->screen_base, screen_base);
+
+	return 0;
+}
+
+static void simpledrm_device_exit_mm(void *data)
+{
+	struct simpledrm_device *sdev = data;
+	struct drm_device *dev = &sdev->dev;
+
+	of_reserved_mem_device_release(dev->dev);
+}
+
+static int simpledrm_device_init_mm(struct simpledrm_device *sdev)
+{
+	int (*init)(struct simpledrm_device *sdev, phys_addr_t start, size_t size);
+	struct drm_device *dev = &sdev->dev;
+	struct platform_device *pdev = to_platform_device(dev->dev);
+	phys_addr_t start, end;
+	size_t size;
+	int ret;
+
+	ret = of_reserved_mem_device_init_by_idx(dev->dev, dev->dev->of_node, 0);
+	if (ret) {
+		struct resource *res;
+
+		res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+		if (!res)
+			return -EINVAL;
+
+		init = simpledrm_device_init_mm_io;
+		size = resource_size(res);
+		start = res->start;
+	} else {
+		devm_add_action_or_reset(dev->dev, simpledrm_device_exit_mm, sdev);
+		init = simpledrm_device_init_mm_sysmem;
+		start = sdev->sysmem_start;
+		size = sdev->sysmem_size;
+	}
+
+	end = start + size - 1;
+
+	ret = devm_aperture_acquire_from_firmware(dev, start, size);
+	if (ret) {
+		drm_err(dev, "could not acquire memory range [%pa-%pa]: %d\n", &start, &end, ret);
+		return ret;
+	}
+
+	return init(sdev, start, size);
+}
+
 /*
  * Modesetting
  */
@@ -491,15 +594,15 @@ static void simpledrm_primary_plane_helper_atomic_update(struct drm_plane *plane
 
 	drm_atomic_helper_damage_iter_init(&iter, old_plane_state, plane_state);
 	drm_atomic_for_each_plane_damage(&iter, &damage) {
-		struct iosys_map dst = IOSYS_MAP_INIT_VADDR(sdev->screen_base);
 		struct drm_rect dst_clip = plane_state->dst;
 
 		if (!drm_rect_intersect(&dst_clip, &damage))
 			continue;
 
-		iosys_map_incr(&dst, drm_fb_clip_offset(sdev->pitch, sdev->format, &dst_clip));
-		drm_fb_blit(&dst, &sdev->pitch, sdev->format->format, shadow_plane_state->data, fb,
-			    &damage);
+		iosys_map_incr(&sdev->screen_base, drm_fb_clip_offset(sdev->pitch, sdev->format,
+								      &dst_clip));
+		drm_fb_blit(&sdev->screen_base, &sdev->pitch, sdev->format->format,
+			    shadow_plane_state->data, fb, &damage);
 	}
 
 	drm_dev_exit(idx);
@@ -518,7 +621,7 @@ static void simpledrm_primary_plane_helper_atomic_disable(struct drm_plane *plan
 		return;
 
 	/* Clear screen to black if disabled */
-	memset_io(sdev->screen_base, 0, sdev->pitch * sdev->mode.vdisplay);
+	iosys_map_memset(&sdev->screen_base, 0, 0, sdev->pitch * sdev->mode.vdisplay);
 
 	drm_dev_exit(idx);
 }
@@ -635,8 +738,6 @@ static struct simpledrm_device *simpledrm_device_create(struct drm_driver *drv,
 	struct drm_device *dev;
 	int width, height, stride;
 	const struct drm_format_info *format;
-	struct resource *res, *mem;
-	void __iomem *screen_base;
 	struct drm_plane *primary_plane;
 	struct drm_crtc *crtc;
 	struct drm_encoder *encoder;
@@ -706,35 +807,9 @@ static struct simpledrm_device *simpledrm_device_create(struct drm_driver *drv,
 	drm_dbg(dev, "framebuffer format=%p4cc, size=%dx%d, stride=%d byte\n",
 		&format->format, width, height, stride);
 
-	/*
-	 * Memory management
-	 */
-
-	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
-	if (!res)
-		return ERR_PTR(-EINVAL);
-
-	ret = devm_aperture_acquire_from_firmware(dev, res->start, resource_size(res));
-	if (ret) {
-		drm_err(dev, "could not acquire memory range %pr: error %d\n", res, ret);
+	ret = simpledrm_device_init_mm(sdev);
+	if (ret)
 		return ERR_PTR(ret);
-	}
-
-	mem = devm_request_mem_region(&pdev->dev, res->start, resource_size(res), drv->name);
-	if (!mem) {
-		/*
-		 * We cannot make this fatal. Sometimes this comes from magic
-		 * spaces our resource handlers simply don't know about. Use
-		 * the I/O-memory resource as-is and try to map that instead.
-		 */
-		drm_warn(dev, "could not acquire memory region %pr\n", res);
-		mem = res;
-	}
-
-	screen_base = devm_ioremap_wc(&pdev->dev, mem->start, resource_size(mem));
-	if (!screen_base)
-		return ERR_PTR(-ENOMEM);
-	sdev->screen_base = screen_base;
 
 	/*
 	 * Modesetting
@@ -878,5 +953,35 @@ static struct platform_driver simpledrm_platform_driver = {
 
 module_platform_driver(simpledrm_platform_driver);
 
+static int simple_framebuffer_device_init(struct reserved_mem *rmem, struct device *dev)
+{
+	struct simpledrm_device *sdev = dev_get_drvdata(dev);
+
+	sdev->sysmem_start = rmem->base;
+	sdev->sysmem_size = rmem->size;
+
+	return 0;
+}
+
+static void simple_framebuffer_device_release(struct reserved_mem *rmem, struct device *dev)
+{
+}
+
+static const struct reserved_mem_ops simple_framebuffer_ops = {
+	.device_init = simple_framebuffer_device_init,
+	.device_release = simple_framebuffer_device_release,
+};
+
+static int simple_framebuffer_init(struct reserved_mem *rmem)
+{
+	pr_info("framebuffer memory at %pa, size %lu bytes\n", &rmem->base,
+		(unsigned long)rmem->size);
+
+	rmem->ops = &simple_framebuffer_ops;
+
+	return 0;
+}
+RESERVEDMEM_OF_DECLARE(simple_framebuffer, "framebuffer", simple_framebuffer_init);
+
 MODULE_DESCRIPTION(DRIVER_DESC);
 MODULE_LICENSE("GPL v2");
-- 
2.37.3

[PATCH v2 5/7] drm/format-helper: Support the XB24 format

[Thierry Reding]

From: Thierry Reding <treding@nvidia.com>

Add a conversion helper for the XB24 format to use in drm_fb_blit().

v2: support XB24 format instead of AB24

Signed-off-by: Thierry Reding <treding@nvidia.com>
---
 drivers/gpu/drm/drm_format_helper.c | 37 +++++++++++++++++++++++++++++
 1 file changed, 37 insertions(+)

diff --git a/drivers/gpu/drm/drm_format_helper.c b/drivers/gpu/drm/drm_format_helper.c
index e2f76621453c..8a7c200ecba9 100644
--- a/drivers/gpu/drm/drm_format_helper.c
+++ b/drivers/gpu/drm/drm_format_helper.c
@@ -503,6 +503,36 @@ static void drm_fb_rgb888_to_xrgb8888(struct iosys_map *dst, const unsigned int
 		    drm_fb_rgb888_to_xrgb8888_line);
 }
 
+static void drm_fb_xrgb8888_to_xbgr8888_line(void *dbuf, const void *sbuf, unsigned int pixels)
+{
+	__le32 *dbuf32 = dbuf;
+	const __le32 *sbuf32 = sbuf;
+	unsigned int x;
+	u32 pix;
+
+	for (x = 0; x < pixels; x++) {
+		pix = le32_to_cpu(sbuf32[x]);
+		pix = ((pix & 0x00ff0000) >> 16) <<  0 |
+		      ((pix & 0x0000ff00) >>  8) <<  8 |
+		      ((pix & 0x000000ff) >>  0) << 16 |
+		      0xff << 24;
+		*dbuf32++ = cpu_to_le32(pix);
+	}
+}
+
+static void drm_fb_xrgb8888_to_xbgr8888(struct iosys_map *dst, const unsigned int *dst_pitch,
+					const struct iosys_map *src,
+					const struct drm_framebuffer *fb,
+					const struct drm_rect *clip)
+{
+	static const u8 dst_pixsize[DRM_FORMAT_MAX_PLANES] = {
+		4,
+	};
+
+	drm_fb_xfrm(dst, dst_pitch, dst_pixsize, src, fb, clip, false,
+		    drm_fb_xrgb8888_to_xbgr8888_line);
+}
+
 static void drm_fb_xrgb8888_to_xrgb2101010_line(void *dbuf, const void *sbuf, unsigned int pixels)
 {
 	__le32 *dbuf32 = dbuf;
@@ -646,6 +676,8 @@ int drm_fb_blit(struct iosys_map *dst, const unsigned int *dst_pitch, uint32_t d
 		fb_format = DRM_FORMAT_XRGB8888;
 	if (dst_format == DRM_FORMAT_ARGB8888)
 		dst_format = DRM_FORMAT_XRGB8888;
+	if (dst_format == DRM_FORMAT_ABGR8888)
+		dst_format = DRM_FORMAT_XBGR8888;
 	if (fb_format == DRM_FORMAT_ARGB2101010)
 		fb_format = DRM_FORMAT_XRGB2101010;
 	if (dst_format == DRM_FORMAT_ARGB2101010)
@@ -673,6 +705,11 @@ int drm_fb_blit(struct iosys_map *dst, const unsigned int *dst_pitch, uint32_t d
 			drm_fb_rgb565_to_xrgb8888(dst, dst_pitch, src, fb, clip);
 			return 0;
 		}
+	} else if (dst_format == DRM_FORMAT_XBGR8888) {
+		if (fb_format == DRM_FORMAT_XRGB8888) {
+			drm_fb_xrgb8888_to_xbgr8888(dst, dst_pitch, src, fb, clip);
+			return 0;
+		}
 	} else if (dst_format == DRM_FORMAT_XRGB2101010) {
 		if (fb_format == DRM_FORMAT_XRGB8888) {
 			drm_fb_xrgb8888_to_xrgb2101010(dst, dst_pitch, src, fb, clip);
-- 
2.37.3

[PATCH v2 5/7] drm/format-helper: Support the XB24 format

[Thierry Reding]

From: Thierry Reding <treding@nvidia.com>

Add a conversion helper for the XB24 format to use in drm_fb_blit().

v2: support XB24 format instead of AB24

Signed-off-by: Thierry Reding <treding@nvidia.com>
---
 drivers/gpu/drm/drm_format_helper.c | 37 +++++++++++++++++++++++++++++
 1 file changed, 37 insertions(+)

diff --git a/drivers/gpu/drm/drm_format_helper.c b/drivers/gpu/drm/drm_format_helper.c
index e2f76621453c..8a7c200ecba9 100644
--- a/drivers/gpu/drm/drm_format_helper.c
+++ b/drivers/gpu/drm/drm_format_helper.c
@@ -503,6 +503,36 @@ static void drm_fb_rgb888_to_xrgb8888(struct iosys_map *dst, const unsigned int
 		    drm_fb_rgb888_to_xrgb8888_line);
 }
 
+static void drm_fb_xrgb8888_to_xbgr8888_line(void *dbuf, const void *sbuf, unsigned int pixels)
+{
+	__le32 *dbuf32 = dbuf;
+	const __le32 *sbuf32 = sbuf;
+	unsigned int x;
+	u32 pix;
+
+	for (x = 0; x < pixels; x++) {
+		pix = le32_to_cpu(sbuf32[x]);
+		pix = ((pix & 0x00ff0000) >> 16) <<  0 |
+		      ((pix & 0x0000ff00) >>  8) <<  8 |
+		      ((pix & 0x000000ff) >>  0) << 16 |
+		      0xff << 24;
+		*dbuf32++ = cpu_to_le32(pix);
+	}
+}
+
+static void drm_fb_xrgb8888_to_xbgr8888(struct iosys_map *dst, const unsigned int *dst_pitch,
+					const struct iosys_map *src,
+					const struct drm_framebuffer *fb,
+					const struct drm_rect *clip)
+{
+	static const u8 dst_pixsize[DRM_FORMAT_MAX_PLANES] = {
+		4,
+	};
+
+	drm_fb_xfrm(dst, dst_pitch, dst_pixsize, src, fb, clip, false,
+		    drm_fb_xrgb8888_to_xbgr8888_line);
+}
+
 static void drm_fb_xrgb8888_to_xrgb2101010_line(void *dbuf, const void *sbuf, unsigned int pixels)
 {
 	__le32 *dbuf32 = dbuf;
@@ -646,6 +676,8 @@ int drm_fb_blit(struct iosys_map *dst, const unsigned int *dst_pitch, uint32_t d
 		fb_format = DRM_FORMAT_XRGB8888;
 	if (dst_format == DRM_FORMAT_ARGB8888)
 		dst_format = DRM_FORMAT_XRGB8888;
+	if (dst_format == DRM_FORMAT_ABGR8888)
+		dst_format = DRM_FORMAT_XBGR8888;
 	if (fb_format == DRM_FORMAT_ARGB2101010)
 		fb_format = DRM_FORMAT_XRGB2101010;
 	if (dst_format == DRM_FORMAT_ARGB2101010)
@@ -673,6 +705,11 @@ int drm_fb_blit(struct iosys_map *dst, const unsigned int *dst_pitch, uint32_t d
 			drm_fb_rgb565_to_xrgb8888(dst, dst_pitch, src, fb, clip);
 			return 0;
 		}
+	} else if (dst_format == DRM_FORMAT_XBGR8888) {
+		if (fb_format == DRM_FORMAT_XRGB8888) {
+			drm_fb_xrgb8888_to_xbgr8888(dst, dst_pitch, src, fb, clip);
+			return 0;
+		}
 	} else if (dst_format == DRM_FORMAT_XRGB2101010) {
 		if (fb_format == DRM_FORMAT_XRGB8888) {
 			drm_fb_xrgb8888_to_xrgb2101010(dst, dst_pitch, src, fb, clip);
-- 
2.37.3

[PATCH v2 6/7] drm/simpledrm: Support the XB24/AB24 format

[Thierry Reding]

From: Thierry Reding <treding@nvidia.com>

Add XB24 and AB24 to the list of supported formats. The format helpers
support conversion to these formats and they are documented in the
simple-framebuffer device tree bindings.

v2: treat AB24 as XB24 and support both at the same time

Signed-off-by: Thierry Reding <treding@nvidia.com>
---
 drivers/gpu/drm/tiny/simpledrm.c       | 2 ++
 include/linux/platform_data/simplefb.h | 1 +
 2 files changed, 3 insertions(+)

diff --git a/drivers/gpu/drm/tiny/simpledrm.c b/drivers/gpu/drm/tiny/simpledrm.c
index cf36f67d22e4..ecb303c89320 100644
--- a/drivers/gpu/drm/tiny/simpledrm.c
+++ b/drivers/gpu/drm/tiny/simpledrm.c
@@ -559,6 +559,8 @@ static int simpledrm_device_init_mm(struct simpledrm_device *sdev)
 static const uint32_t simpledrm_primary_plane_formats[] = {
 	DRM_FORMAT_XRGB8888,
 	DRM_FORMAT_ARGB8888,
+	DRM_FORMAT_XBGR8888,
+	DRM_FORMAT_ABGR8888,
 	DRM_FORMAT_RGB565,
 	//DRM_FORMAT_XRGB1555,
 	//DRM_FORMAT_ARGB1555,
diff --git a/include/linux/platform_data/simplefb.h b/include/linux/platform_data/simplefb.h
index 27ea99af6e1d..4f94d52ac99f 100644
--- a/include/linux/platform_data/simplefb.h
+++ b/include/linux/platform_data/simplefb.h
@@ -22,6 +22,7 @@
 	{ "r8g8b8", 24, {16, 8}, {8, 8}, {0, 8}, {0, 0}, DRM_FORMAT_RGB888 }, \
 	{ "x8r8g8b8", 32, {16, 8}, {8, 8}, {0, 8}, {0, 0}, DRM_FORMAT_XRGB8888 }, \
 	{ "a8r8g8b8", 32, {16, 8}, {8, 8}, {0, 8}, {24, 8}, DRM_FORMAT_ARGB8888 }, \
+	{ "x8b8g8r8", 32, {0, 8}, {8, 8}, {16, 8}, {0, 0}, DRM_FORMAT_XBGR8888 }, \
 	{ "a8b8g8r8", 32, {0, 8}, {8, 8}, {16, 8}, {24, 8}, DRM_FORMAT_ABGR8888 }, \
 	{ "x2r10g10b10", 32, {20, 10}, {10, 10}, {0, 10}, {0, 0}, DRM_FORMAT_XRGB2101010 }, \
 	{ "a2r10g10b10", 32, {20, 10}, {10, 10}, {0, 10}, {30, 2}, DRM_FORMAT_ARGB2101010 }, \
-- 
2.37.3

[PATCH v2 6/7] drm/simpledrm: Support the XB24/AB24 format

[Thierry Reding]

From: Thierry Reding <treding@nvidia.com>

Add XB24 and AB24 to the list of supported formats. The format helpers
support conversion to these formats and they are documented in the
simple-framebuffer device tree bindings.

v2: treat AB24 as XB24 and support both at the same time

Signed-off-by: Thierry Reding <treding@nvidia.com>
---
 drivers/gpu/drm/tiny/simpledrm.c       | 2 ++
 include/linux/platform_data/simplefb.h | 1 +
 2 files changed, 3 insertions(+)

diff --git a/drivers/gpu/drm/tiny/simpledrm.c b/drivers/gpu/drm/tiny/simpledrm.c
index cf36f67d22e4..ecb303c89320 100644
--- a/drivers/gpu/drm/tiny/simpledrm.c
+++ b/drivers/gpu/drm/tiny/simpledrm.c
@@ -559,6 +559,8 @@ static int simpledrm_device_init_mm(struct simpledrm_device *sdev)
 static const uint32_t simpledrm_primary_plane_formats[] = {
 	DRM_FORMAT_XRGB8888,
 	DRM_FORMAT_ARGB8888,
+	DRM_FORMAT_XBGR8888,
+	DRM_FORMAT_ABGR8888,
 	DRM_FORMAT_RGB565,
 	//DRM_FORMAT_XRGB1555,
 	//DRM_FORMAT_ARGB1555,
diff --git a/include/linux/platform_data/simplefb.h b/include/linux/platform_data/simplefb.h
index 27ea99af6e1d..4f94d52ac99f 100644
--- a/include/linux/platform_data/simplefb.h
+++ b/include/linux/platform_data/simplefb.h
@@ -22,6 +22,7 @@
 	{ "r8g8b8", 24, {16, 8}, {8, 8}, {0, 8}, {0, 0}, DRM_FORMAT_RGB888 }, \
 	{ "x8r8g8b8", 32, {16, 8}, {8, 8}, {0, 8}, {0, 0}, DRM_FORMAT_XRGB8888 }, \
 	{ "a8r8g8b8", 32, {16, 8}, {8, 8}, {0, 8}, {24, 8}, DRM_FORMAT_ARGB8888 }, \
+	{ "x8b8g8r8", 32, {0, 8}, {8, 8}, {16, 8}, {0, 0}, DRM_FORMAT_XBGR8888 }, \
 	{ "a8b8g8r8", 32, {0, 8}, {8, 8}, {16, 8}, {24, 8}, DRM_FORMAT_ABGR8888 }, \
 	{ "x2r10g10b10", 32, {20, 10}, {10, 10}, {0, 10}, {0, 0}, DRM_FORMAT_XRGB2101010 }, \
 	{ "a2r10g10b10", 32, {20, 10}, {10, 10}, {0, 10}, {30, 2}, DRM_FORMAT_ARGB2101010 }, \
-- 
2.37.3

[PATCH v2 7/7] arm64: tegra: Add simple framebuffer on Jetson Xavier NX

[Thierry Reding]

From: Thierry Reding <treding@nvidia.com>

Add the framebuffer carveout reserved memory node as well as a simple-
framebuffer node that is used to bind to the framebuffer that the
bootloader has set up.

Signed-off-by: Thierry Reding <treding@nvidia.com>
---
Changes in v2:
- clear out dynamic fields and leave it up to firmware to fill them in
- mark simple-framebuffer node as disabled by default

 .../nvidia/tegra194-p3509-0000+p3668-0001.dts | 33 +++++++++++++++++++
 arch/arm64/boot/dts/nvidia/tegra194.dtsi      |  2 +-
 2 files changed, 34 insertions(+), 1 deletion(-)

diff --git a/arch/arm64/boot/dts/nvidia/tegra194-p3509-0000+p3668-0001.dts b/arch/arm64/boot/dts/nvidia/tegra194-p3509-0000+p3668-0001.dts
index 238fd98e8e45..44600479ea5f 100644
--- a/arch/arm64/boot/dts/nvidia/tegra194-p3509-0000+p3668-0001.dts
+++ b/arch/arm64/boot/dts/nvidia/tegra194-p3509-0000+p3668-0001.dts
@@ -7,4 +7,37 @@
 / {
 	model = "NVIDIA Jetson Xavier NX Developer Kit (eMMC)";
 	compatible = "nvidia,p3509-0000+p3668-0001", "nvidia,tegra194";
+
+	chosen {
+		framebuffer {
+			compatible = "simple-framebuffer";
+			status = "disabled";
+			memory-region = <&fb>;
+			power-domains = <&bpmp TEGRA194_POWER_DOMAIN_DISP>;
+			clocks = <&bpmp TEGRA194_CLK_SOR1_REF>,
+				 <&bpmp TEGRA194_CLK_SOR1_OUT>,
+				 <&bpmp TEGRA194_CLK_SOR1_PAD_CLKOUT>,
+				 <&bpmp TEGRA194_CLK_PLLD2>,
+				 <&bpmp TEGRA194_CLK_PLLDP>,
+				 <&bpmp TEGRA194_CLK_NVDISPLAY_DISP>,
+				 <&bpmp TEGRA194_CLK_NVDISPLAYHUB>,
+				 <&bpmp TEGRA194_CLK_NVDISPLAY_P0>;
+			width = <0>;
+			height = <0>;
+			stride = <0>;
+			format = "";
+		};
+	};
+
+	reserved-memory {
+		#address-cells = <2>;
+		#size-cells = <2>;
+		ranges;
+
+		fb: framebuffer@0,0 {
+			compatible = "framebuffer";
+			reg = <0x0 0x0 0x0 0x0>;
+			iommu-addresses = <&dc0 0x0 0x0 0x0 0x0>;
+		};
+	};
 };
diff --git a/arch/arm64/boot/dts/nvidia/tegra194.dtsi b/arch/arm64/boot/dts/nvidia/tegra194.dtsi
index 4d10b6b5324d..a961d42c81d6 100644
--- a/arch/arm64/boot/dts/nvidia/tegra194.dtsi
+++ b/arch/arm64/boot/dts/nvidia/tegra194.dtsi
@@ -1960,7 +1960,7 @@ display-hub@15200000 {
 
 				ranges = <0x15200000 0x15200000 0x40000>;
 
-				display@15200000 {
+				dc0: display@15200000 {
 					compatible = "nvidia,tegra194-dc";
 					reg = <0x15200000 0x10000>;
 					interrupts = <GIC_SPI 153 IRQ_TYPE_LEVEL_HIGH>;
-- 
2.37.3

[PATCH v2 7/7] arm64: tegra: Add simple framebuffer on Jetson Xavier NX

[Thierry Reding]

From: Thierry Reding <treding@nvidia.com>

Add the framebuffer carveout reserved memory node as well as a simple-
framebuffer node that is used to bind to the framebuffer that the
bootloader has set up.

Signed-off-by: Thierry Reding <treding@nvidia.com>
---
Changes in v2:
- clear out dynamic fields and leave it up to firmware to fill them in
- mark simple-framebuffer node as disabled by default

 .../nvidia/tegra194-p3509-0000+p3668-0001.dts | 33 +++++++++++++++++++
 arch/arm64/boot/dts/nvidia/tegra194.dtsi      |  2 +-
 2 files changed, 34 insertions(+), 1 deletion(-)

diff --git a/arch/arm64/boot/dts/nvidia/tegra194-p3509-0000+p3668-0001.dts b/arch/arm64/boot/dts/nvidia/tegra194-p3509-0000+p3668-0001.dts
index 238fd98e8e45..44600479ea5f 100644
--- a/arch/arm64/boot/dts/nvidia/tegra194-p3509-0000+p3668-0001.dts
+++ b/arch/arm64/boot/dts/nvidia/tegra194-p3509-0000+p3668-0001.dts
@@ -7,4 +7,37 @@
 / {
 	model = "NVIDIA Jetson Xavier NX Developer Kit (eMMC)";
 	compatible = "nvidia,p3509-0000+p3668-0001", "nvidia,tegra194";
+
+	chosen {
+		framebuffer {
+			compatible = "simple-framebuffer";
+			status = "disabled";
+			memory-region = <&fb>;
+			power-domains = <&bpmp TEGRA194_POWER_DOMAIN_DISP>;
+			clocks = <&bpmp TEGRA194_CLK_SOR1_REF>,
+				 <&bpmp TEGRA194_CLK_SOR1_OUT>,
+				 <&bpmp TEGRA194_CLK_SOR1_PAD_CLKOUT>,
+				 <&bpmp TEGRA194_CLK_PLLD2>,
+				 <&bpmp TEGRA194_CLK_PLLDP>,
+				 <&bpmp TEGRA194_CLK_NVDISPLAY_DISP>,
+				 <&bpmp TEGRA194_CLK_NVDISPLAYHUB>,
+				 <&bpmp TEGRA194_CLK_NVDISPLAY_P0>;
+			width = <0>;
+			height = <0>;
+			stride = <0>;
+			format = "";
+		};
+	};
+
+	reserved-memory {
+		#address-cells = <2>;
+		#size-cells = <2>;
+		ranges;
+
+		fb: framebuffer@0,0 {
+			compatible = "framebuffer";
+			reg = <0x0 0x0 0x0 0x0>;
+			iommu-addresses = <&dc0 0x0 0x0 0x0 0x0>;
+		};
+	};
 };
diff --git a/arch/arm64/boot/dts/nvidia/tegra194.dtsi b/arch/arm64/boot/dts/nvidia/tegra194.dtsi
index 4d10b6b5324d..a961d42c81d6 100644
--- a/arch/arm64/boot/dts/nvidia/tegra194.dtsi
+++ b/arch/arm64/boot/dts/nvidia/tegra194.dtsi
@@ -1960,7 +1960,7 @@ display-hub@15200000 {
 
 				ranges = <0x15200000 0x15200000 0x40000>;
 
-				display@15200000 {
+				dc0: display@15200000 {
 					compatible = "nvidia,tegra194-dc";
 					reg = <0x15200000 0x10000>;
 					interrupts = <GIC_SPI 153 IRQ_TYPE_LEVEL_HIGH>;
-- 
2.37.3

Re: [PATCH v2 1/7] dt-bindings: display: simple-framebuffer: Support system memory framebuffers

[Rob Herring]

On Fri, 07 Oct 2022 14:49:40 +0200, Thierry Reding wrote:
> From: Thierry Reding <treding@nvidia.com>
> 
> In order to support framebuffers residing in system memory, allow the
> memory-region property to override the framebuffer memory specification
> in the "reg" property.
> 
> Signed-off-by: Thierry Reding <treding@nvidia.com>
> ---
>  .../devicetree/bindings/display/simple-framebuffer.yaml      | 5 +++++
>  1 file changed, 5 insertions(+)
> 

Reviewed-by: Rob Herring <robh@kernel.org>

Re: [PATCH v2 1/7] dt-bindings: display: simple-framebuffer: Support system memory framebuffers

[Rob Herring]

On Fri, 07 Oct 2022 14:49:40 +0200, Thierry Reding wrote:
> From: Thierry Reding <treding@nvidia.com>
> 
> In order to support framebuffers residing in system memory, allow the
> memory-region property to override the framebuffer memory specification
> in the "reg" property.
> 
> Signed-off-by: Thierry Reding <treding@nvidia.com>
> ---
>  .../devicetree/bindings/display/simple-framebuffer.yaml      | 5 +++++
>  1 file changed, 5 insertions(+)
> 

Reviewed-by: Rob Herring <robh@kernel.org>

Re: [PATCH v2 2/7] dt-bindings: display: simple-framebuffer: Document 32-bit BGR format

[Rob Herring]

On Fri, 07 Oct 2022 14:49:41 +0200, Thierry Reding wrote:
> From: Thierry Reding <treding@nvidia.com>
> 
> This is a variant of the 32-bit RGB format where the red and blue
> components are swapped.
> 
> Signed-off-by: Thierry Reding <treding@nvidia.com>
> ---
>  .../devicetree/bindings/display/simple-framebuffer.yaml         | 2 ++
>  1 file changed, 2 insertions(+)
> 

Reviewed-by: Rob Herring <robh@kernel.org>

Re: [PATCH v2 2/7] dt-bindings: display: simple-framebuffer: Document 32-bit BGR format

[Rob Herring]

On Fri, 07 Oct 2022 14:49:41 +0200, Thierry Reding wrote:
> From: Thierry Reding <treding@nvidia.com>
> 
> This is a variant of the 32-bit RGB format where the red and blue
> components are swapped.
> 
> Signed-off-by: Thierry Reding <treding@nvidia.com>
> ---
>  .../devicetree/bindings/display/simple-framebuffer.yaml         | 2 ++
>  1 file changed, 2 insertions(+)
> 

Reviewed-by: Rob Herring <robh@kernel.org>

Re: [PATCH v2 3/7] dt-bindings: reserved-memory: Support framebuffer reserved memory

[Rob Herring]

On Fri, 07 Oct 2022 14:49:42 +0200, Thierry Reding wrote:
> From: Thierry Reding <treding@nvidia.com>
> 
> Document the "framebuffer" compatible string for reserved memory nodes
> to annotate reserved memory regions used for framebuffer carveouts.
> 
> Signed-off-by: Thierry Reding <treding@nvidia.com>
> ---
> Changes in v2:
> - use four spaces for indentation in example (as recommended elsewhere)
> - add explicit root node
> - drop unneeded quotes
> 
>  .../bindings/reserved-memory/framebuffer.yaml | 52 +++++++++++++++++++
>  1 file changed, 52 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/reserved-memory/framebuffer.yaml
> 

Reviewed-by: Rob Herring <robh@kernel.org>

Re: [PATCH v2 3/7] dt-bindings: reserved-memory: Support framebuffer reserved memory

[Rob Herring]

On Fri, 07 Oct 2022 14:49:42 +0200, Thierry Reding wrote:
> From: Thierry Reding <treding@nvidia.com>
> 
> Document the "framebuffer" compatible string for reserved memory nodes
> to annotate reserved memory regions used for framebuffer carveouts.
> 
> Signed-off-by: Thierry Reding <treding@nvidia.com>
> ---
> Changes in v2:
> - use four spaces for indentation in example (as recommended elsewhere)
> - add explicit root node
> - drop unneeded quotes
> 
>  .../bindings/reserved-memory/framebuffer.yaml | 52 +++++++++++++++++++
>  1 file changed, 52 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/reserved-memory/framebuffer.yaml
> 

Reviewed-by: Rob Herring <robh@kernel.org>

Re: [PATCH v2 4/7] drm/simpledrm: Add support for system memory framebuffers

[Thomas Zimmermann]

[-- Attachment #1.1: Type: text/plain, Size: 9571 bytes --]

Hi

Am 07.10.22 um 14:49 schrieb Thierry Reding:
> From: Thierry Reding <treding@nvidia.com>
> 
> Simple framebuffers can be set up in system memory, which cannot be
> requested and/or I/O remapped using the I/O resource helpers. Add a
> separate code path that obtains system memory framebuffers from the
> reserved memory region referenced in the memory-region property.
> 
> v2: make screen base a struct iosys_map to avoid sparse warnings

Conversion to iosys_map has to be a separate patch.

> 
> Signed-off-by: Thierry Reding <treding@nvidia.com>
> ---
>   drivers/gpu/drm/tiny/simpledrm.c | 177 ++++++++++++++++++++++++-------
>   1 file changed, 141 insertions(+), 36 deletions(-)
> 
> diff --git a/drivers/gpu/drm/tiny/simpledrm.c b/drivers/gpu/drm/tiny/simpledrm.c
> index 18489779fb8a..cf36f67d22e4 100644
> --- a/drivers/gpu/drm/tiny/simpledrm.c
> +++ b/drivers/gpu/drm/tiny/simpledrm.c
> @@ -2,6 +2,7 @@
>   
>   #include <linux/clk.h>
>   #include <linux/of_clk.h>
> +#include <linux/of_reserved_mem.h>
>   #include <linux/minmax.h>
>   #include <linux/platform_data/simplefb.h>
>   #include <linux/platform_device.h>
> @@ -207,7 +208,9 @@ struct simpledrm_device {
>   	unsigned int pitch;
>   
>   	/* memory management */
> -	void __iomem *screen_base;
> +	struct iosys_map screen_base;
> +	phys_addr_t sysmem_start;
> +	size_t sysmem_size;
>   
>   	/* modesetting */
>   	uint32_t formats[8];
> @@ -441,6 +444,106 @@ static int simpledrm_device_init_regulators(struct simpledrm_device *sdev)
>   }
>   #endif
>   
> +/*
> + * Memory management
> + */
> +
> +static int simpledrm_device_init_mm_sysmem(struct simpledrm_device *sdev, phys_addr_t start,
> +					   size_t size)
> +{
> +	struct drm_device *dev = &sdev->dev;
> +	phys_addr_t end = start + size - 1;
> +	void *screen_base;
> +
> +	drm_info(dev, "using system memory framebuffer at [%pa-%pa]\n", &start, &end);
> +
> +	screen_base = devm_memremap(dev->dev, start, size, MEMREMAP_WC);
> +	if (!screen_base)
> +		return -ENOMEM;
> +
> +	iosys_map_set_vaddr(&sdev->screen_base, screen_base);
> +
> +	return 0;
> +}
> +
> +static int simpledrm_device_init_mm_io(struct simpledrm_device *sdev, phys_addr_t start,
> +				       size_t size)
> +{
> +	struct drm_device *dev = &sdev->dev;
> +	phys_addr_t end = start + size - 1;
> +	void __iomem *screen_base;
> +	struct resource *mem;
> +
> +	drm_info(dev, "using I/O memory framebuffer at [%pa-%pa]\n", &start, &end);
> +
> +	mem = devm_request_mem_region(dev->dev, start, size, sdev->dev.driver->name);
> +	if (!mem) {
> +		/*
> +		 * We cannot make this fatal. Sometimes this comes from magic
> +		 * spaces our resource handlers simply don't know about. Use
> +		 * the I/O-memory resource as-is and try to map that instead.
> +		 */
> +		drm_warn(dev, "could not acquire memory region [%pa-%pa]\n", &start, &end);
> +	} else {
> +		size = resource_size(mem);
> +		start = mem->start;
> +	}
> +
> +	screen_base = devm_ioremap_wc(dev->dev, start, size);
> +	if (!screen_base)
> +		return -ENOMEM;
> +
> +	iosys_map_set_vaddr_iomem(&sdev->screen_base, screen_base);
> +
> +	return 0;
> +}
> +
> +static void simpledrm_device_exit_mm(void *data)
> +{
> +	struct simpledrm_device *sdev = data;
> +	struct drm_device *dev = &sdev->dev;
> +
> +	of_reserved_mem_device_release(dev->dev);
> +}
> +
> +static int simpledrm_device_init_mm(struct simpledrm_device *sdev)
> +{
> +	int (*init)(struct simpledrm_device *sdev, phys_addr_t start, size_t size);
> +	struct drm_device *dev = &sdev->dev;
> +	struct platform_device *pdev = to_platform_device(dev->dev);
> +	phys_addr_t start, end;
> +	size_t size;
> +	int ret;
> +
> +	ret = of_reserved_mem_device_init_by_idx(dev->dev, dev->dev->of_node, 0);
> +	if (ret) {
> +		struct resource *res;
> +
> +		res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +		if (!res)
> +			return -EINVAL;
> +
> +		init = simpledrm_device_init_mm_io;
> +		size = resource_size(res);
> +		start = res->start;
> +	} else {
> +		devm_add_action_or_reset(dev->dev, simpledrm_device_exit_mm, sdev);
> +		init = simpledrm_device_init_mm_sysmem;
> +		start = sdev->sysmem_start;
> +		size = sdev->sysmem_size;
> +	}
> +
> +	end = start + size - 1;
> +
> +	ret = devm_aperture_acquire_from_firmware(dev, start, size);
> +	if (ret) {
> +		drm_err(dev, "could not acquire memory range [%pa-%pa]: %d\n", &start, &end, ret);
> +		return ret;
> +	}
> +
> +	return init(sdev, start, size);
> +}
> +

That whole 'Memory Manangement' block is will be unmaintainable. Before 
I go into a detailed review, please see my questions about the 
reservedmem code at the end of the patch.

>   /*
>    * Modesetting
>    */
> @@ -491,15 +594,15 @@ static void simpledrm_primary_plane_helper_atomic_update(struct drm_plane *plane
>   
>   	drm_atomic_helper_damage_iter_init(&iter, old_plane_state, plane_state);
>   	drm_atomic_for_each_plane_damage(&iter, &damage) {
> -		struct iosys_map dst = IOSYS_MAP_INIT_VADDR(sdev->screen_base);
>   		struct drm_rect dst_clip = plane_state->dst;
>   
>   		if (!drm_rect_intersect(&dst_clip, &damage))
>   			continue;
>   
> -		iosys_map_incr(&dst, drm_fb_clip_offset(sdev->pitch, sdev->format, &dst_clip));
> -		drm_fb_blit(&dst, &sdev->pitch, sdev->format->format, shadow_plane_state->data, fb,
> -			    &damage);
> +		iosys_map_incr(&sdev->screen_base, drm_fb_clip_offset(sdev->pitch, sdev->format,
> +								      &dst_clip));

You'll modify screen_base and it'll eventually blow up. You have to keep 
initializing the dst variable within the loop.

> +		drm_fb_blit(&sdev->screen_base, &sdev->pitch, sdev->format->format,
> +			    shadow_plane_state->data, fb, &damage);
>   	}
>   
>   	drm_dev_exit(idx);
> @@ -518,7 +621,7 @@ static void simpledrm_primary_plane_helper_atomic_disable(struct drm_plane *plan
>   		return;
>   
>   	/* Clear screen to black if disabled */
> -	memset_io(sdev->screen_base, 0, sdev->pitch * sdev->mode.vdisplay);
> +	iosys_map_memset(&sdev->screen_base, 0, 0, sdev->pitch * sdev->mode.vdisplay);
>   
>   	drm_dev_exit(idx);
>   }
> @@ -635,8 +738,6 @@ static struct simpledrm_device *simpledrm_device_create(struct drm_driver *drv,
>   	struct drm_device *dev;
>   	int width, height, stride;
>   	const struct drm_format_info *format;
> -	struct resource *res, *mem;
> -	void __iomem *screen_base;
>   	struct drm_plane *primary_plane;
>   	struct drm_crtc *crtc;
>   	struct drm_encoder *encoder;
> @@ -706,35 +807,9 @@ static struct simpledrm_device *simpledrm_device_create(struct drm_driver *drv,
>   	drm_dbg(dev, "framebuffer format=%p4cc, size=%dx%d, stride=%d byte\n",
>   		&format->format, width, height, stride);
>   
> -	/*
> -	 * Memory management
> -	 */
> -
> -	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> -	if (!res)
> -		return ERR_PTR(-EINVAL);
> -
> -	ret = devm_aperture_acquire_from_firmware(dev, res->start, resource_size(res));
> -	if (ret) {
> -		drm_err(dev, "could not acquire memory range %pr: error %d\n", res, ret);
> +	ret = simpledrm_device_init_mm(sdev);
> +	if (ret)
>   		return ERR_PTR(ret);
> -	}
> -
> -	mem = devm_request_mem_region(&pdev->dev, res->start, resource_size(res), drv->name);
> -	if (!mem) {
> -		/*
> -		 * We cannot make this fatal. Sometimes this comes from magic
> -		 * spaces our resource handlers simply don't know about. Use
> -		 * the I/O-memory resource as-is and try to map that instead.
> -		 */
> -		drm_warn(dev, "could not acquire memory region %pr\n", res);
> -		mem = res;
> -	}
> -
> -	screen_base = devm_ioremap_wc(&pdev->dev, mem->start, resource_size(mem));
> -	if (!screen_base)
> -		return ERR_PTR(-ENOMEM);
> -	sdev->screen_base = screen_base;
>   
>   	/*
>   	 * Modesetting
> @@ -878,5 +953,35 @@ static struct platform_driver simpledrm_platform_driver = {
>   
>   module_platform_driver(simpledrm_platform_driver);
>   
> +static int simple_framebuffer_device_init(struct reserved_mem *rmem, struct device *dev)
> +{
> +	struct simpledrm_device *sdev = dev_get_drvdata(dev);
> +
> +	sdev->sysmem_start = rmem->base;
> +	sdev->sysmem_size = rmem->size;

 From what I understand, you use the sysmem_ variables in the same code 
that allocates the simpledrm_device, which creates a chicken-egg 
problem. When does this code run?


> +
> +	return 0;
> +}
> +
> +static void simple_framebuffer_device_release(struct reserved_mem *rmem, struct device *dev)
> +{
> +}
> +
> +static const struct reserved_mem_ops simple_framebuffer_ops = {
> +	.device_init = simple_framebuffer_device_init,
> +	.device_release = simple_framebuffer_device_release,
> +};
> +
> +static int simple_framebuffer_init(struct reserved_mem *rmem)
> +{
> +	pr_info("framebuffer memory at %pa, size %lu bytes\n", &rmem->base,
> +		(unsigned long)rmem->size);
> +
> +	rmem->ops = &simple_framebuffer_ops;
> +
> +	return 0;
> +}
> +RESERVEDMEM_OF_DECLARE(simple_framebuffer, "framebuffer", simple_framebuffer_init);

What's the prupose of these code at all?  I looked through the kernel, 
but there aren't many other examples of it.

Best regards
Thomas

> +
>   MODULE_DESCRIPTION(DRIVER_DESC);
>   MODULE_LICENSE("GPL v2");

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Ivo Totev

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]

Re: [PATCH v2 4/7] drm/simpledrm: Add support for system memory framebuffers

[Thomas Zimmermann]

[-- Attachment #1.1: Type: text/plain, Size: 9571 bytes --]

Hi

Am 07.10.22 um 14:49 schrieb Thierry Reding:
> From: Thierry Reding <treding@nvidia.com>
> 
> Simple framebuffers can be set up in system memory, which cannot be
> requested and/or I/O remapped using the I/O resource helpers. Add a
> separate code path that obtains system memory framebuffers from the
> reserved memory region referenced in the memory-region property.
> 
> v2: make screen base a struct iosys_map to avoid sparse warnings

Conversion to iosys_map has to be a separate patch.

> 
> Signed-off-by: Thierry Reding <treding@nvidia.com>
> ---
>   drivers/gpu/drm/tiny/simpledrm.c | 177 ++++++++++++++++++++++++-------
>   1 file changed, 141 insertions(+), 36 deletions(-)
> 
> diff --git a/drivers/gpu/drm/tiny/simpledrm.c b/drivers/gpu/drm/tiny/simpledrm.c
> index 18489779fb8a..cf36f67d22e4 100644
> --- a/drivers/gpu/drm/tiny/simpledrm.c
> +++ b/drivers/gpu/drm/tiny/simpledrm.c
> @@ -2,6 +2,7 @@
>   
>   #include <linux/clk.h>
>   #include <linux/of_clk.h>
> +#include <linux/of_reserved_mem.h>
>   #include <linux/minmax.h>
>   #include <linux/platform_data/simplefb.h>
>   #include <linux/platform_device.h>
> @@ -207,7 +208,9 @@ struct simpledrm_device {
>   	unsigned int pitch;
>   
>   	/* memory management */
> -	void __iomem *screen_base;
> +	struct iosys_map screen_base;
> +	phys_addr_t sysmem_start;
> +	size_t sysmem_size;
>   
>   	/* modesetting */
>   	uint32_t formats[8];
> @@ -441,6 +444,106 @@ static int simpledrm_device_init_regulators(struct simpledrm_device *sdev)
>   }
>   #endif
>   
> +/*
> + * Memory management
> + */
> +
> +static int simpledrm_device_init_mm_sysmem(struct simpledrm_device *sdev, phys_addr_t start,
> +					   size_t size)
> +{
> +	struct drm_device *dev = &sdev->dev;
> +	phys_addr_t end = start + size - 1;
> +	void *screen_base;
> +
> +	drm_info(dev, "using system memory framebuffer at [%pa-%pa]\n", &start, &end);
> +
> +	screen_base = devm_memremap(dev->dev, start, size, MEMREMAP_WC);
> +	if (!screen_base)
> +		return -ENOMEM;
> +
> +	iosys_map_set_vaddr(&sdev->screen_base, screen_base);
> +
> +	return 0;
> +}
> +
> +static int simpledrm_device_init_mm_io(struct simpledrm_device *sdev, phys_addr_t start,
> +				       size_t size)
> +{
> +	struct drm_device *dev = &sdev->dev;
> +	phys_addr_t end = start + size - 1;
> +	void __iomem *screen_base;
> +	struct resource *mem;
> +
> +	drm_info(dev, "using I/O memory framebuffer at [%pa-%pa]\n", &start, &end);
> +
> +	mem = devm_request_mem_region(dev->dev, start, size, sdev->dev.driver->name);
> +	if (!mem) {
> +		/*
> +		 * We cannot make this fatal. Sometimes this comes from magic
> +		 * spaces our resource handlers simply don't know about. Use
> +		 * the I/O-memory resource as-is and try to map that instead.
> +		 */
> +		drm_warn(dev, "could not acquire memory region [%pa-%pa]\n", &start, &end);
> +	} else {
> +		size = resource_size(mem);
> +		start = mem->start;
> +	}
> +
> +	screen_base = devm_ioremap_wc(dev->dev, start, size);
> +	if (!screen_base)
> +		return -ENOMEM;
> +
> +	iosys_map_set_vaddr_iomem(&sdev->screen_base, screen_base);
> +
> +	return 0;
> +}
> +
> +static void simpledrm_device_exit_mm(void *data)
> +{
> +	struct simpledrm_device *sdev = data;
> +	struct drm_device *dev = &sdev->dev;
> +
> +	of_reserved_mem_device_release(dev->dev);
> +}
> +
> +static int simpledrm_device_init_mm(struct simpledrm_device *sdev)
> +{
> +	int (*init)(struct simpledrm_device *sdev, phys_addr_t start, size_t size);
> +	struct drm_device *dev = &sdev->dev;
> +	struct platform_device *pdev = to_platform_device(dev->dev);
> +	phys_addr_t start, end;
> +	size_t size;
> +	int ret;
> +
> +	ret = of_reserved_mem_device_init_by_idx(dev->dev, dev->dev->of_node, 0);
> +	if (ret) {
> +		struct resource *res;
> +
> +		res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +		if (!res)
> +			return -EINVAL;
> +
> +		init = simpledrm_device_init_mm_io;
> +		size = resource_size(res);
> +		start = res->start;
> +	} else {
> +		devm_add_action_or_reset(dev->dev, simpledrm_device_exit_mm, sdev);
> +		init = simpledrm_device_init_mm_sysmem;
> +		start = sdev->sysmem_start;
> +		size = sdev->sysmem_size;
> +	}
> +
> +	end = start + size - 1;
> +
> +	ret = devm_aperture_acquire_from_firmware(dev, start, size);
> +	if (ret) {
> +		drm_err(dev, "could not acquire memory range [%pa-%pa]: %d\n", &start, &end, ret);
> +		return ret;
> +	}
> +
> +	return init(sdev, start, size);
> +}
> +

That whole 'Memory Manangement' block is will be unmaintainable. Before 
I go into a detailed review, please see my questions about the 
reservedmem code at the end of the patch.

>   /*
>    * Modesetting
>    */
> @@ -491,15 +594,15 @@ static void simpledrm_primary_plane_helper_atomic_update(struct drm_plane *plane
>   
>   	drm_atomic_helper_damage_iter_init(&iter, old_plane_state, plane_state);
>   	drm_atomic_for_each_plane_damage(&iter, &damage) {
> -		struct iosys_map dst = IOSYS_MAP_INIT_VADDR(sdev->screen_base);
>   		struct drm_rect dst_clip = plane_state->dst;
>   
>   		if (!drm_rect_intersect(&dst_clip, &damage))
>   			continue;
>   
> -		iosys_map_incr(&dst, drm_fb_clip_offset(sdev->pitch, sdev->format, &dst_clip));
> -		drm_fb_blit(&dst, &sdev->pitch, sdev->format->format, shadow_plane_state->data, fb,
> -			    &damage);
> +		iosys_map_incr(&sdev->screen_base, drm_fb_clip_offset(sdev->pitch, sdev->format,
> +								      &dst_clip));

You'll modify screen_base and it'll eventually blow up. You have to keep 
initializing the dst variable within the loop.

> +		drm_fb_blit(&sdev->screen_base, &sdev->pitch, sdev->format->format,
> +			    shadow_plane_state->data, fb, &damage);
>   	}
>   
>   	drm_dev_exit(idx);
> @@ -518,7 +621,7 @@ static void simpledrm_primary_plane_helper_atomic_disable(struct drm_plane *plan
>   		return;
>   
>   	/* Clear screen to black if disabled */
> -	memset_io(sdev->screen_base, 0, sdev->pitch * sdev->mode.vdisplay);
> +	iosys_map_memset(&sdev->screen_base, 0, 0, sdev->pitch * sdev->mode.vdisplay);
>   
>   	drm_dev_exit(idx);
>   }
> @@ -635,8 +738,6 @@ static struct simpledrm_device *simpledrm_device_create(struct drm_driver *drv,
>   	struct drm_device *dev;
>   	int width, height, stride;
>   	const struct drm_format_info *format;
> -	struct resource *res, *mem;
> -	void __iomem *screen_base;
>   	struct drm_plane *primary_plane;
>   	struct drm_crtc *crtc;
>   	struct drm_encoder *encoder;
> @@ -706,35 +807,9 @@ static struct simpledrm_device *simpledrm_device_create(struct drm_driver *drv,
>   	drm_dbg(dev, "framebuffer format=%p4cc, size=%dx%d, stride=%d byte\n",
>   		&format->format, width, height, stride);
>   
> -	/*
> -	 * Memory management
> -	 */
> -
> -	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> -	if (!res)
> -		return ERR_PTR(-EINVAL);
> -
> -	ret = devm_aperture_acquire_from_firmware(dev, res->start, resource_size(res));
> -	if (ret) {
> -		drm_err(dev, "could not acquire memory range %pr: error %d\n", res, ret);
> +	ret = simpledrm_device_init_mm(sdev);
> +	if (ret)
>   		return ERR_PTR(ret);
> -	}
> -
> -	mem = devm_request_mem_region(&pdev->dev, res->start, resource_size(res), drv->name);
> -	if (!mem) {
> -		/*
> -		 * We cannot make this fatal. Sometimes this comes from magic
> -		 * spaces our resource handlers simply don't know about. Use
> -		 * the I/O-memory resource as-is and try to map that instead.
> -		 */
> -		drm_warn(dev, "could not acquire memory region %pr\n", res);
> -		mem = res;
> -	}
> -
> -	screen_base = devm_ioremap_wc(&pdev->dev, mem->start, resource_size(mem));
> -	if (!screen_base)
> -		return ERR_PTR(-ENOMEM);
> -	sdev->screen_base = screen_base;
>   
>   	/*
>   	 * Modesetting
> @@ -878,5 +953,35 @@ static struct platform_driver simpledrm_platform_driver = {
>   
>   module_platform_driver(simpledrm_platform_driver);
>   
> +static int simple_framebuffer_device_init(struct reserved_mem *rmem, struct device *dev)
> +{
> +	struct simpledrm_device *sdev = dev_get_drvdata(dev);
> +
> +	sdev->sysmem_start = rmem->base;
> +	sdev->sysmem_size = rmem->size;

 From what I understand, you use the sysmem_ variables in the same code 
that allocates the simpledrm_device, which creates a chicken-egg 
problem. When does this code run?


> +
> +	return 0;
> +}
> +
> +static void simple_framebuffer_device_release(struct reserved_mem *rmem, struct device *dev)
> +{
> +}
> +
> +static const struct reserved_mem_ops simple_framebuffer_ops = {
> +	.device_init = simple_framebuffer_device_init,
> +	.device_release = simple_framebuffer_device_release,
> +};
> +
> +static int simple_framebuffer_init(struct reserved_mem *rmem)
> +{
> +	pr_info("framebuffer memory at %pa, size %lu bytes\n", &rmem->base,
> +		(unsigned long)rmem->size);
> +
> +	rmem->ops = &simple_framebuffer_ops;
> +
> +	return 0;
> +}
> +RESERVEDMEM_OF_DECLARE(simple_framebuffer, "framebuffer", simple_framebuffer_init);

What's the prupose of these code at all?  I looked through the kernel, 
but there aren't many other examples of it.

Best regards
Thomas

> +
>   MODULE_DESCRIPTION(DRIVER_DESC);
>   MODULE_LICENSE("GPL v2");

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Ivo Totev

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]

Re: [PATCH v2 1/7] dt-bindings: display: simple-framebuffer: Support system memory framebuffers

[Thomas Zimmermann]

[-- Attachment #1.1: Type: text/plain, Size: 1676 bytes --]

Hi

Am 07.10.22 um 14:49 schrieb Thierry Reding:
> From: Thierry Reding <treding@nvidia.com>
> 
> In order to support framebuffers residing in system memory, allow the
> memory-region property to override the framebuffer memory specification
> in the "reg" property.

What happens if both properties are present and they disagree with each 
other?

I understand that the framebuffer is behind 'memory-region', but does 
'reg' still contain device memory?  Do we need to acquire ownership from 
within the driver?

Best regards
Thomas

> 
> Signed-off-by: Thierry Reding <treding@nvidia.com>
> ---
>   .../devicetree/bindings/display/simple-framebuffer.yaml      | 5 +++++
>   1 file changed, 5 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/display/simple-framebuffer.yaml b/Documentation/devicetree/bindings/display/simple-framebuffer.yaml
> index dd64f70b5014..3e9857eb002e 100644
> --- a/Documentation/devicetree/bindings/display/simple-framebuffer.yaml
> +++ b/Documentation/devicetree/bindings/display/simple-framebuffer.yaml
> @@ -63,6 +63,11 @@ properties:
>     reg:
>       description: Location and size of the framebuffer memory
>   
> +  memory-region:
> +    maxItems: 1
> +    description: Phandle to a node describing the memory to be used for the
> +      framebuffer. If present, overrides the "reg" property (if one exists).
> +
>     clocks:
>       description: List of clocks used by the framebuffer.
>   

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Ivo Totev

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]

Re: [PATCH v2 1/7] dt-bindings: display: simple-framebuffer: Support system memory framebuffers

[Thomas Zimmermann]

[-- Attachment #1.1: Type: text/plain, Size: 1676 bytes --]

Hi

Am 07.10.22 um 14:49 schrieb Thierry Reding:
> From: Thierry Reding <treding@nvidia.com>
> 
> In order to support framebuffers residing in system memory, allow the
> memory-region property to override the framebuffer memory specification
> in the "reg" property.

What happens if both properties are present and they disagree with each 
other?

I understand that the framebuffer is behind 'memory-region', but does 
'reg' still contain device memory?  Do we need to acquire ownership from 
within the driver?

Best regards
Thomas

> 
> Signed-off-by: Thierry Reding <treding@nvidia.com>
> ---
>   .../devicetree/bindings/display/simple-framebuffer.yaml      | 5 +++++
>   1 file changed, 5 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/display/simple-framebuffer.yaml b/Documentation/devicetree/bindings/display/simple-framebuffer.yaml
> index dd64f70b5014..3e9857eb002e 100644
> --- a/Documentation/devicetree/bindings/display/simple-framebuffer.yaml
> +++ b/Documentation/devicetree/bindings/display/simple-framebuffer.yaml
> @@ -63,6 +63,11 @@ properties:
>     reg:
>       description: Location and size of the framebuffer memory
>   
> +  memory-region:
> +    maxItems: 1
> +    description: Phandle to a node describing the memory to be used for the
> +      framebuffer. If present, overrides the "reg" property (if one exists).
> +
>     clocks:
>       description: List of clocks used by the framebuffer.
>   

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Ivo Totev

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]

Re: [PATCH v2 1/7] dt-bindings: display: simple-framebuffer: Support system memory framebuffers

[Thierry Reding]

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

On Mon, Oct 10, 2022 at 11:37:37AM +0200, Thomas Zimmermann wrote:
> Hi
> 
> Am 07.10.22 um 14:49 schrieb Thierry Reding:
> > From: Thierry Reding <treding@nvidia.com>
> > 
> > In order to support framebuffers residing in system memory, allow the
> > memory-region property to override the framebuffer memory specification
> > in the "reg" property.
> 
> What happens if both properties are present and they disagree with each
> other?
> 
> I understand that the framebuffer is behind 'memory-region', but does 'reg'
> still contain device memory?  Do we need to acquire ownership from within
> the driver?

The intention is for both memory-region and reg properties to be
mutually exclusive. I can't think of a scenario where you would need or
want both.

Note also the documentation for the memory-region property:

|  memory-region:
|    maxItems: 1
|    description: Phandle to a node describing the memory to be used for the
|      framebuffer. If present, overrides the "reg" property (if one exists).

Thierry

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

Re: [PATCH v2 1/7] dt-bindings: display: simple-framebuffer: Support system memory framebuffers

[Thierry Reding]

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

On Mon, Oct 10, 2022 at 11:37:37AM +0200, Thomas Zimmermann wrote:
> Hi
> 
> Am 07.10.22 um 14:49 schrieb Thierry Reding:
> > From: Thierry Reding <treding@nvidia.com>
> > 
> > In order to support framebuffers residing in system memory, allow the
> > memory-region property to override the framebuffer memory specification
> > in the "reg" property.
> 
> What happens if both properties are present and they disagree with each
> other?
> 
> I understand that the framebuffer is behind 'memory-region', but does 'reg'
> still contain device memory?  Do we need to acquire ownership from within
> the driver?

The intention is for both memory-region and reg properties to be
mutually exclusive. I can't think of a scenario where you would need or
want both.

Note also the documentation for the memory-region property:

|  memory-region:
|    maxItems: 1
|    description: Phandle to a node describing the memory to be used for the
|      framebuffer. If present, overrides the "reg" property (if one exists).

Thierry

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

Re: [PATCH v2 4/7] drm/simpledrm: Add support for system memory framebuffers

[Thierry Reding]

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

On Mon, Oct 10, 2022 at 10:12:34AM +0200, Thomas Zimmermann wrote:
> Hi
> 
> Am 07.10.22 um 14:49 schrieb Thierry Reding:
> > From: Thierry Reding <treding@nvidia.com>
> > 
> > Simple framebuffers can be set up in system memory, which cannot be
> > requested and/or I/O remapped using the I/O resource helpers. Add a
> > separate code path that obtains system memory framebuffers from the
> > reserved memory region referenced in the memory-region property.
> > 
> > v2: make screen base a struct iosys_map to avoid sparse warnings
> 
> Conversion to iosys_map has to be a separate patch.

Okay. It seemed to make sense to put it into this patch because only
the other changes in this patch make this necessary. The non-__iomem
path was not previously used, so without this patch there's nothing
that needs fixing. Well, unless perhaps for semantic correctness.

> > Signed-off-by: Thierry Reding <treding@nvidia.com>
> > ---
> >   drivers/gpu/drm/tiny/simpledrm.c | 177 ++++++++++++++++++++++++-------
> >   1 file changed, 141 insertions(+), 36 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/tiny/simpledrm.c b/drivers/gpu/drm/tiny/simpledrm.c
> > index 18489779fb8a..cf36f67d22e4 100644
> > --- a/drivers/gpu/drm/tiny/simpledrm.c
> > +++ b/drivers/gpu/drm/tiny/simpledrm.c
> > @@ -2,6 +2,7 @@
> >   #include <linux/clk.h>
> >   #include <linux/of_clk.h>
> > +#include <linux/of_reserved_mem.h>
> >   #include <linux/minmax.h>
> >   #include <linux/platform_data/simplefb.h>
> >   #include <linux/platform_device.h>
> > @@ -207,7 +208,9 @@ struct simpledrm_device {
> >   	unsigned int pitch;
> >   	/* memory management */
> > -	void __iomem *screen_base;
> > +	struct iosys_map screen_base;
> > +	phys_addr_t sysmem_start;
> > +	size_t sysmem_size;
> >   	/* modesetting */
> >   	uint32_t formats[8];
> > @@ -441,6 +444,106 @@ static int simpledrm_device_init_regulators(struct simpledrm_device *sdev)
> >   }
> >   #endif
> > +/*
> > + * Memory management
> > + */
> > +
> > +static int simpledrm_device_init_mm_sysmem(struct simpledrm_device *sdev, phys_addr_t start,
> > +					   size_t size)
> > +{
> > +	struct drm_device *dev = &sdev->dev;
> > +	phys_addr_t end = start + size - 1;
> > +	void *screen_base;
> > +
> > +	drm_info(dev, "using system memory framebuffer at [%pa-%pa]\n", &start, &end);
> > +
> > +	screen_base = devm_memremap(dev->dev, start, size, MEMREMAP_WC);
> > +	if (!screen_base)
> > +		return -ENOMEM;
> > +
> > +	iosys_map_set_vaddr(&sdev->screen_base, screen_base);
> > +
> > +	return 0;
> > +}
> > +
> > +static int simpledrm_device_init_mm_io(struct simpledrm_device *sdev, phys_addr_t start,
> > +				       size_t size)
> > +{
> > +	struct drm_device *dev = &sdev->dev;
> > +	phys_addr_t end = start + size - 1;
> > +	void __iomem *screen_base;
> > +	struct resource *mem;
> > +
> > +	drm_info(dev, "using I/O memory framebuffer at [%pa-%pa]\n", &start, &end);
> > +
> > +	mem = devm_request_mem_region(dev->dev, start, size, sdev->dev.driver->name);
> > +	if (!mem) {
> > +		/*
> > +		 * We cannot make this fatal. Sometimes this comes from magic
> > +		 * spaces our resource handlers simply don't know about. Use
> > +		 * the I/O-memory resource as-is and try to map that instead.
> > +		 */
> > +		drm_warn(dev, "could not acquire memory region [%pa-%pa]\n", &start, &end);
> > +	} else {
> > +		size = resource_size(mem);
> > +		start = mem->start;
> > +	}
> > +
> > +	screen_base = devm_ioremap_wc(dev->dev, start, size);
> > +	if (!screen_base)
> > +		return -ENOMEM;
> > +
> > +	iosys_map_set_vaddr_iomem(&sdev->screen_base, screen_base);
> > +
> > +	return 0;
> > +}
> > +
> > +static void simpledrm_device_exit_mm(void *data)
> > +{
> > +	struct simpledrm_device *sdev = data;
> > +	struct drm_device *dev = &sdev->dev;
> > +
> > +	of_reserved_mem_device_release(dev->dev);
> > +}
> > +
> > +static int simpledrm_device_init_mm(struct simpledrm_device *sdev)
> > +{
> > +	int (*init)(struct simpledrm_device *sdev, phys_addr_t start, size_t size);
> > +	struct drm_device *dev = &sdev->dev;
> > +	struct platform_device *pdev = to_platform_device(dev->dev);
> > +	phys_addr_t start, end;
> > +	size_t size;
> > +	int ret;
> > +
> > +	ret = of_reserved_mem_device_init_by_idx(dev->dev, dev->dev->of_node, 0);
> > +	if (ret) {
> > +		struct resource *res;
> > +
> > +		res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > +		if (!res)
> > +			return -EINVAL;
> > +
> > +		init = simpledrm_device_init_mm_io;
> > +		size = resource_size(res);
> > +		start = res->start;
> > +	} else {
> > +		devm_add_action_or_reset(dev->dev, simpledrm_device_exit_mm, sdev);
> > +		init = simpledrm_device_init_mm_sysmem;
> > +		start = sdev->sysmem_start;
> > +		size = sdev->sysmem_size;
> > +	}
> > +
> > +	end = start + size - 1;
> > +
> > +	ret = devm_aperture_acquire_from_firmware(dev, start, size);
> > +	if (ret) {
> > +		drm_err(dev, "could not acquire memory range [%pa-%pa]: %d\n", &start, &end, ret);
> > +		return ret;
> > +	}
> > +
> > +	return init(sdev, start, size);
> > +}
> > +
> 
> That whole 'Memory Manangement' block is will be unmaintainable. Before I go
> into a detailed review, please see my questions about the reservedmem code
> at the end of the patch.

It looks reasonably maintainable to me. Given that we only have __iomem
and non-__iomem cases, this is about the extent of the complexity that
could ever be added.

> 
> >   /*
> >    * Modesetting
> >    */
> > @@ -491,15 +594,15 @@ static void simpledrm_primary_plane_helper_atomic_update(struct drm_plane *plane
> >   	drm_atomic_helper_damage_iter_init(&iter, old_plane_state, plane_state);
> >   	drm_atomic_for_each_plane_damage(&iter, &damage) {
> > -		struct iosys_map dst = IOSYS_MAP_INIT_VADDR(sdev->screen_base);
> >   		struct drm_rect dst_clip = plane_state->dst;
> >   		if (!drm_rect_intersect(&dst_clip, &damage))
> >   			continue;
> > -		iosys_map_incr(&dst, drm_fb_clip_offset(sdev->pitch, sdev->format, &dst_clip));
> > -		drm_fb_blit(&dst, &sdev->pitch, sdev->format->format, shadow_plane_state->data, fb,
> > -			    &damage);
> > +		iosys_map_incr(&sdev->screen_base, drm_fb_clip_offset(sdev->pitch, sdev->format,
> > +								      &dst_clip));
> 
> You'll modify screen_base and it'll eventually blow up. You have to keep
> initializing the dst variable within the loop.
> 
> > +		drm_fb_blit(&sdev->screen_base, &sdev->pitch, sdev->format->format,
> > +			    shadow_plane_state->data, fb, &damage);
> >   	}
> >   	drm_dev_exit(idx);
> > @@ -518,7 +621,7 @@ static void simpledrm_primary_plane_helper_atomic_disable(struct drm_plane *plan
> >   		return;
> >   	/* Clear screen to black if disabled */
> > -	memset_io(sdev->screen_base, 0, sdev->pitch * sdev->mode.vdisplay);
> > +	iosys_map_memset(&sdev->screen_base, 0, 0, sdev->pitch * sdev->mode.vdisplay);
> >   	drm_dev_exit(idx);
> >   }
> > @@ -635,8 +738,6 @@ static struct simpledrm_device *simpledrm_device_create(struct drm_driver *drv,
> >   	struct drm_device *dev;
> >   	int width, height, stride;
> >   	const struct drm_format_info *format;
> > -	struct resource *res, *mem;
> > -	void __iomem *screen_base;
> >   	struct drm_plane *primary_plane;
> >   	struct drm_crtc *crtc;
> >   	struct drm_encoder *encoder;
> > @@ -706,35 +807,9 @@ static struct simpledrm_device *simpledrm_device_create(struct drm_driver *drv,
> >   	drm_dbg(dev, "framebuffer format=%p4cc, size=%dx%d, stride=%d byte\n",
> >   		&format->format, width, height, stride);
> > -	/*
> > -	 * Memory management
> > -	 */
> > -
> > -	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > -	if (!res)
> > -		return ERR_PTR(-EINVAL);
> > -
> > -	ret = devm_aperture_acquire_from_firmware(dev, res->start, resource_size(res));
> > -	if (ret) {
> > -		drm_err(dev, "could not acquire memory range %pr: error %d\n", res, ret);
> > +	ret = simpledrm_device_init_mm(sdev);
> > +	if (ret)
> >   		return ERR_PTR(ret);
> > -	}
> > -
> > -	mem = devm_request_mem_region(&pdev->dev, res->start, resource_size(res), drv->name);
> > -	if (!mem) {
> > -		/*
> > -		 * We cannot make this fatal. Sometimes this comes from magic
> > -		 * spaces our resource handlers simply don't know about. Use
> > -		 * the I/O-memory resource as-is and try to map that instead.
> > -		 */
> > -		drm_warn(dev, "could not acquire memory region %pr\n", res);
> > -		mem = res;
> > -	}
> > -
> > -	screen_base = devm_ioremap_wc(&pdev->dev, mem->start, resource_size(mem));
> > -	if (!screen_base)
> > -		return ERR_PTR(-ENOMEM);
> > -	sdev->screen_base = screen_base;
> >   	/*
> >   	 * Modesetting
> > @@ -878,5 +953,35 @@ static struct platform_driver simpledrm_platform_driver = {
> >   module_platform_driver(simpledrm_platform_driver);
> > +static int simple_framebuffer_device_init(struct reserved_mem *rmem, struct device *dev)
> > +{
> > +	struct simpledrm_device *sdev = dev_get_drvdata(dev);
> > +
> > +	sdev->sysmem_start = rmem->base;
> > +	sdev->sysmem_size = rmem->size;
> 
> From what I understand, you use the sysmem_ variables in the same code that
> allocates the simpledrm_device, which creates a chicken-egg problem. When
> does this code run?

This will run as a result of the of_reserved_mem_device_init_by_idx()
call earlier. It might be possible to push more code from the sysmem
initialization code path above into this function. That may also make
the somewhat clunky sysmem_start/size fields unnecessary.

Alternatively, we may be able to skip the whole RESERVEDMEM_OF_DECLARE
bits here and directly resolve the memory-region property and read its
"reg" property. However it seemed more appropriate to use the existing
infrastructure for this, even if it's rather minimal.

> 
> 
> > +
> > +	return 0;
> > +}
> > +
> > +static void simple_framebuffer_device_release(struct reserved_mem *rmem, struct device *dev)
> > +{
> > +}
> > +
> > +static const struct reserved_mem_ops simple_framebuffer_ops = {
> > +	.device_init = simple_framebuffer_device_init,
> > +	.device_release = simple_framebuffer_device_release,
> > +};
> > +
> > +static int simple_framebuffer_init(struct reserved_mem *rmem)
> > +{
> > +	pr_info("framebuffer memory at %pa, size %lu bytes\n", &rmem->base,
> > +		(unsigned long)rmem->size);
> > +
> > +	rmem->ops = &simple_framebuffer_ops;
> > +
> > +	return 0;
> > +}
> > +RESERVEDMEM_OF_DECLARE(simple_framebuffer, "framebuffer", simple_framebuffer_init);
> 
> What's the prupose of these code at all?  I looked through the kernel, but
> there aren't many other examples of it.

This is a fairly standard construct to deal with early memory
reservations. What happens is roughly this: during early kernel boot,
the reserved-memory core code will iterate over all children of the top-
level reserved-memory node and see if they have a compatible string that
matches one of the entries in the table created by these
RESERVEDMEM_OF_DECLARE entries. It will then call the init function for
a matched entry and register a struct reserved_mem for these. The init
function in this case just dumps an informational message to the boot
log to provide some information about the framebuffer region that was
reserved (which can be used for example for troubleshooting purposes)
and sets the device init/release operations (which will be called when a
device is associated with the reserved memory region, i.e. when the
of_reserved_mem_device_init_by_idx() function is called).

The reason why there aren't many examples of this is because these are
special memory regions that (at least upstream) kernels seldom support.
Perhaps the most common use-cases are the shared DMA pools (such as
CMA).

Thierry

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

Re: [PATCH v2 4/7] drm/simpledrm: Add support for system memory framebuffers

[Thierry Reding]

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

On Mon, Oct 10, 2022 at 10:12:34AM +0200, Thomas Zimmermann wrote:
> Hi
> 
> Am 07.10.22 um 14:49 schrieb Thierry Reding:
> > From: Thierry Reding <treding@nvidia.com>
> > 
> > Simple framebuffers can be set up in system memory, which cannot be
> > requested and/or I/O remapped using the I/O resource helpers. Add a
> > separate code path that obtains system memory framebuffers from the
> > reserved memory region referenced in the memory-region property.
> > 
> > v2: make screen base a struct iosys_map to avoid sparse warnings
> 
> Conversion to iosys_map has to be a separate patch.

Okay. It seemed to make sense to put it into this patch because only
the other changes in this patch make this necessary. The non-__iomem
path was not previously used, so without this patch there's nothing
that needs fixing. Well, unless perhaps for semantic correctness.

> > Signed-off-by: Thierry Reding <treding@nvidia.com>
> > ---
> >   drivers/gpu/drm/tiny/simpledrm.c | 177 ++++++++++++++++++++++++-------
> >   1 file changed, 141 insertions(+), 36 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/tiny/simpledrm.c b/drivers/gpu/drm/tiny/simpledrm.c
> > index 18489779fb8a..cf36f67d22e4 100644
> > --- a/drivers/gpu/drm/tiny/simpledrm.c
> > +++ b/drivers/gpu/drm/tiny/simpledrm.c
> > @@ -2,6 +2,7 @@
> >   #include <linux/clk.h>
> >   #include <linux/of_clk.h>
> > +#include <linux/of_reserved_mem.h>
> >   #include <linux/minmax.h>
> >   #include <linux/platform_data/simplefb.h>
> >   #include <linux/platform_device.h>
> > @@ -207,7 +208,9 @@ struct simpledrm_device {
> >   	unsigned int pitch;
> >   	/* memory management */
> > -	void __iomem *screen_base;
> > +	struct iosys_map screen_base;
> > +	phys_addr_t sysmem_start;
> > +	size_t sysmem_size;
> >   	/* modesetting */
> >   	uint32_t formats[8];
> > @@ -441,6 +444,106 @@ static int simpledrm_device_init_regulators(struct simpledrm_device *sdev)
> >   }
> >   #endif
> > +/*
> > + * Memory management
> > + */
> > +
> > +static int simpledrm_device_init_mm_sysmem(struct simpledrm_device *sdev, phys_addr_t start,
> > +					   size_t size)
> > +{
> > +	struct drm_device *dev = &sdev->dev;
> > +	phys_addr_t end = start + size - 1;
> > +	void *screen_base;
> > +
> > +	drm_info(dev, "using system memory framebuffer at [%pa-%pa]\n", &start, &end);
> > +
> > +	screen_base = devm_memremap(dev->dev, start, size, MEMREMAP_WC);
> > +	if (!screen_base)
> > +		return -ENOMEM;
> > +
> > +	iosys_map_set_vaddr(&sdev->screen_base, screen_base);
> > +
> > +	return 0;
> > +}
> > +
> > +static int simpledrm_device_init_mm_io(struct simpledrm_device *sdev, phys_addr_t start,
> > +				       size_t size)
> > +{
> > +	struct drm_device *dev = &sdev->dev;
> > +	phys_addr_t end = start + size - 1;
> > +	void __iomem *screen_base;
> > +	struct resource *mem;
> > +
> > +	drm_info(dev, "using I/O memory framebuffer at [%pa-%pa]\n", &start, &end);
> > +
> > +	mem = devm_request_mem_region(dev->dev, start, size, sdev->dev.driver->name);
> > +	if (!mem) {
> > +		/*
> > +		 * We cannot make this fatal. Sometimes this comes from magic
> > +		 * spaces our resource handlers simply don't know about. Use
> > +		 * the I/O-memory resource as-is and try to map that instead.
> > +		 */
> > +		drm_warn(dev, "could not acquire memory region [%pa-%pa]\n", &start, &end);
> > +	} else {
> > +		size = resource_size(mem);
> > +		start = mem->start;
> > +	}
> > +
> > +	screen_base = devm_ioremap_wc(dev->dev, start, size);
> > +	if (!screen_base)
> > +		return -ENOMEM;
> > +
> > +	iosys_map_set_vaddr_iomem(&sdev->screen_base, screen_base);
> > +
> > +	return 0;
> > +}
> > +
> > +static void simpledrm_device_exit_mm(void *data)
> > +{
> > +	struct simpledrm_device *sdev = data;
> > +	struct drm_device *dev = &sdev->dev;
> > +
> > +	of_reserved_mem_device_release(dev->dev);
> > +}
> > +
> > +static int simpledrm_device_init_mm(struct simpledrm_device *sdev)
> > +{
> > +	int (*init)(struct simpledrm_device *sdev, phys_addr_t start, size_t size);
> > +	struct drm_device *dev = &sdev->dev;
> > +	struct platform_device *pdev = to_platform_device(dev->dev);
> > +	phys_addr_t start, end;
> > +	size_t size;
> > +	int ret;
> > +
> > +	ret = of_reserved_mem_device_init_by_idx(dev->dev, dev->dev->of_node, 0);
> > +	if (ret) {
> > +		struct resource *res;
> > +
> > +		res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > +		if (!res)
> > +			return -EINVAL;
> > +
> > +		init = simpledrm_device_init_mm_io;
> > +		size = resource_size(res);
> > +		start = res->start;
> > +	} else {
> > +		devm_add_action_or_reset(dev->dev, simpledrm_device_exit_mm, sdev);
> > +		init = simpledrm_device_init_mm_sysmem;
> > +		start = sdev->sysmem_start;
> > +		size = sdev->sysmem_size;
> > +	}
> > +
> > +	end = start + size - 1;
> > +
> > +	ret = devm_aperture_acquire_from_firmware(dev, start, size);
> > +	if (ret) {
> > +		drm_err(dev, "could not acquire memory range [%pa-%pa]: %d\n", &start, &end, ret);
> > +		return ret;
> > +	}
> > +
> > +	return init(sdev, start, size);
> > +}
> > +
> 
> That whole 'Memory Manangement' block is will be unmaintainable. Before I go
> into a detailed review, please see my questions about the reservedmem code
> at the end of the patch.

It looks reasonably maintainable to me. Given that we only have __iomem
and non-__iomem cases, this is about the extent of the complexity that
could ever be added.

> 
> >   /*
> >    * Modesetting
> >    */
> > @@ -491,15 +594,15 @@ static void simpledrm_primary_plane_helper_atomic_update(struct drm_plane *plane
> >   	drm_atomic_helper_damage_iter_init(&iter, old_plane_state, plane_state);
> >   	drm_atomic_for_each_plane_damage(&iter, &damage) {
> > -		struct iosys_map dst = IOSYS_MAP_INIT_VADDR(sdev->screen_base);
> >   		struct drm_rect dst_clip = plane_state->dst;
> >   		if (!drm_rect_intersect(&dst_clip, &damage))
> >   			continue;
> > -		iosys_map_incr(&dst, drm_fb_clip_offset(sdev->pitch, sdev->format, &dst_clip));
> > -		drm_fb_blit(&dst, &sdev->pitch, sdev->format->format, shadow_plane_state->data, fb,
> > -			    &damage);
> > +		iosys_map_incr(&sdev->screen_base, drm_fb_clip_offset(sdev->pitch, sdev->format,
> > +								      &dst_clip));
> 
> You'll modify screen_base and it'll eventually blow up. You have to keep
> initializing the dst variable within the loop.
> 
> > +		drm_fb_blit(&sdev->screen_base, &sdev->pitch, sdev->format->format,
> > +			    shadow_plane_state->data, fb, &damage);
> >   	}
> >   	drm_dev_exit(idx);
> > @@ -518,7 +621,7 @@ static void simpledrm_primary_plane_helper_atomic_disable(struct drm_plane *plan
> >   		return;
> >   	/* Clear screen to black if disabled */
> > -	memset_io(sdev->screen_base, 0, sdev->pitch * sdev->mode.vdisplay);
> > +	iosys_map_memset(&sdev->screen_base, 0, 0, sdev->pitch * sdev->mode.vdisplay);
> >   	drm_dev_exit(idx);
> >   }
> > @@ -635,8 +738,6 @@ static struct simpledrm_device *simpledrm_device_create(struct drm_driver *drv,
> >   	struct drm_device *dev;
> >   	int width, height, stride;
> >   	const struct drm_format_info *format;
> > -	struct resource *res, *mem;
> > -	void __iomem *screen_base;
> >   	struct drm_plane *primary_plane;
> >   	struct drm_crtc *crtc;
> >   	struct drm_encoder *encoder;
> > @@ -706,35 +807,9 @@ static struct simpledrm_device *simpledrm_device_create(struct drm_driver *drv,
> >   	drm_dbg(dev, "framebuffer format=%p4cc, size=%dx%d, stride=%d byte\n",
> >   		&format->format, width, height, stride);
> > -	/*
> > -	 * Memory management
> > -	 */
> > -
> > -	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > -	if (!res)
> > -		return ERR_PTR(-EINVAL);
> > -
> > -	ret = devm_aperture_acquire_from_firmware(dev, res->start, resource_size(res));
> > -	if (ret) {
> > -		drm_err(dev, "could not acquire memory range %pr: error %d\n", res, ret);
> > +	ret = simpledrm_device_init_mm(sdev);
> > +	if (ret)
> >   		return ERR_PTR(ret);
> > -	}
> > -
> > -	mem = devm_request_mem_region(&pdev->dev, res->start, resource_size(res), drv->name);
> > -	if (!mem) {
> > -		/*
> > -		 * We cannot make this fatal. Sometimes this comes from magic
> > -		 * spaces our resource handlers simply don't know about. Use
> > -		 * the I/O-memory resource as-is and try to map that instead.
> > -		 */
> > -		drm_warn(dev, "could not acquire memory region %pr\n", res);
> > -		mem = res;
> > -	}
> > -
> > -	screen_base = devm_ioremap_wc(&pdev->dev, mem->start, resource_size(mem));
> > -	if (!screen_base)
> > -		return ERR_PTR(-ENOMEM);
> > -	sdev->screen_base = screen_base;
> >   	/*
> >   	 * Modesetting
> > @@ -878,5 +953,35 @@ static struct platform_driver simpledrm_platform_driver = {
> >   module_platform_driver(simpledrm_platform_driver);
> > +static int simple_framebuffer_device_init(struct reserved_mem *rmem, struct device *dev)
> > +{
> > +	struct simpledrm_device *sdev = dev_get_drvdata(dev);
> > +
> > +	sdev->sysmem_start = rmem->base;
> > +	sdev->sysmem_size = rmem->size;
> 
> From what I understand, you use the sysmem_ variables in the same code that
> allocates the simpledrm_device, which creates a chicken-egg problem. When
> does this code run?

This will run as a result of the of_reserved_mem_device_init_by_idx()
call earlier. It might be possible to push more code from the sysmem
initialization code path above into this function. That may also make
the somewhat clunky sysmem_start/size fields unnecessary.

Alternatively, we may be able to skip the whole RESERVEDMEM_OF_DECLARE
bits here and directly resolve the memory-region property and read its
"reg" property. However it seemed more appropriate to use the existing
infrastructure for this, even if it's rather minimal.

> 
> 
> > +
> > +	return 0;
> > +}
> > +
> > +static void simple_framebuffer_device_release(struct reserved_mem *rmem, struct device *dev)
> > +{
> > +}
> > +
> > +static const struct reserved_mem_ops simple_framebuffer_ops = {
> > +	.device_init = simple_framebuffer_device_init,
> > +	.device_release = simple_framebuffer_device_release,
> > +};
> > +
> > +static int simple_framebuffer_init(struct reserved_mem *rmem)
> > +{
> > +	pr_info("framebuffer memory at %pa, size %lu bytes\n", &rmem->base,
> > +		(unsigned long)rmem->size);
> > +
> > +	rmem->ops = &simple_framebuffer_ops;
> > +
> > +	return 0;
> > +}
> > +RESERVEDMEM_OF_DECLARE(simple_framebuffer, "framebuffer", simple_framebuffer_init);
> 
> What's the prupose of these code at all?  I looked through the kernel, but
> there aren't many other examples of it.

This is a fairly standard construct to deal with early memory
reservations. What happens is roughly this: during early kernel boot,
the reserved-memory core code will iterate over all children of the top-
level reserved-memory node and see if they have a compatible string that
matches one of the entries in the table created by these
RESERVEDMEM_OF_DECLARE entries. It will then call the init function for
a matched entry and register a struct reserved_mem for these. The init
function in this case just dumps an informational message to the boot
log to provide some information about the framebuffer region that was
reserved (which can be used for example for troubleshooting purposes)
and sets the device init/release operations (which will be called when a
device is associated with the reserved memory region, i.e. when the
of_reserved_mem_device_init_by_idx() function is called).

The reason why there aren't many examples of this is because these are
special memory regions that (at least upstream) kernels seldom support.
Perhaps the most common use-cases are the shared DMA pools (such as
CMA).

Thierry

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

Re: [PATCH v2 4/7] drm/simpledrm: Add support for system memory framebuffers

[Rob Herring]

On Mon, Oct 17, 2022 at 9:54 AM Thierry Reding <thierry.reding@gmail.com> wrote:
>
> On Mon, Oct 10, 2022 at 10:12:34AM +0200, Thomas Zimmermann wrote:
> > Hi
> >
> > Am 07.10.22 um 14:49 schrieb Thierry Reding:
> > > From: Thierry Reding <treding@nvidia.com>
> > >
> > > Simple framebuffers can be set up in system memory, which cannot be
> > > requested and/or I/O remapped using the I/O resource helpers. Add a
> > > separate code path that obtains system memory framebuffers from the
> > > reserved memory region referenced in the memory-region property.
> > >
> > > v2: make screen base a struct iosys_map to avoid sparse warnings

[...]

> > > +static int simple_framebuffer_init(struct reserved_mem *rmem)
> > > +{
> > > +   pr_info("framebuffer memory at %pa, size %lu bytes\n", &rmem->base,
> > > +           (unsigned long)rmem->size);
> > > +
> > > +   rmem->ops = &simple_framebuffer_ops;
> > > +
> > > +   return 0;
> > > +}
> > > +RESERVEDMEM_OF_DECLARE(simple_framebuffer, "framebuffer", simple_framebuffer_init);
> >
> > What's the prupose of these code at all?  I looked through the kernel, but
> > there aren't many other examples of it.
>
> This is a fairly standard construct to deal with early memory
> reservations. What happens is roughly this: during early kernel boot,
> the reserved-memory core code will iterate over all children of the top-
> level reserved-memory node and see if they have a compatible string that
> matches one of the entries in the table created by these
> RESERVEDMEM_OF_DECLARE entries. It will then call the init function for
> a matched entry and register a struct reserved_mem for these. The init
> function in this case just dumps an informational message to the boot
> log to provide some information about the framebuffer region that was
> reserved (which can be used for example for troubleshooting purposes)
> and sets the device init/release operations (which will be called when a
> device is associated with the reserved memory region, i.e. when the
> of_reserved_mem_device_init_by_idx() function is called).
>
> The reason why there aren't many examples of this is because these are
> special memory regions that (at least upstream) kernels seldom support.
> Perhaps the most common use-cases are the shared DMA pools (such as
> CMA).

Also, not all regions need to be handled 'early' before slab allocator
or drivers are probed. Do you need early handling here? I can't see
why other than if fbcon is up early.

Rob

Re: [PATCH v2 4/7] drm/simpledrm: Add support for system memory framebuffers

[Rob Herring]

On Mon, Oct 17, 2022 at 9:54 AM Thierry Reding <thierry.reding@gmail.com> wrote:
>
> On Mon, Oct 10, 2022 at 10:12:34AM +0200, Thomas Zimmermann wrote:
> > Hi
> >
> > Am 07.10.22 um 14:49 schrieb Thierry Reding:
> > > From: Thierry Reding <treding@nvidia.com>
> > >
> > > Simple framebuffers can be set up in system memory, which cannot be
> > > requested and/or I/O remapped using the I/O resource helpers. Add a
> > > separate code path that obtains system memory framebuffers from the
> > > reserved memory region referenced in the memory-region property.
> > >
> > > v2: make screen base a struct iosys_map to avoid sparse warnings

[...]

> > > +static int simple_framebuffer_init(struct reserved_mem *rmem)
> > > +{
> > > +   pr_info("framebuffer memory at %pa, size %lu bytes\n", &rmem->base,
> > > +           (unsigned long)rmem->size);
> > > +
> > > +   rmem->ops = &simple_framebuffer_ops;
> > > +
> > > +   return 0;
> > > +}
> > > +RESERVEDMEM_OF_DECLARE(simple_framebuffer, "framebuffer", simple_framebuffer_init);
> >
> > What's the prupose of these code at all?  I looked through the kernel, but
> > there aren't many other examples of it.
>
> This is a fairly standard construct to deal with early memory
> reservations. What happens is roughly this: during early kernel boot,
> the reserved-memory core code will iterate over all children of the top-
> level reserved-memory node and see if they have a compatible string that
> matches one of the entries in the table created by these
> RESERVEDMEM_OF_DECLARE entries. It will then call the init function for
> a matched entry and register a struct reserved_mem for these. The init
> function in this case just dumps an informational message to the boot
> log to provide some information about the framebuffer region that was
> reserved (which can be used for example for troubleshooting purposes)
> and sets the device init/release operations (which will be called when a
> device is associated with the reserved memory region, i.e. when the
> of_reserved_mem_device_init_by_idx() function is called).
>
> The reason why there aren't many examples of this is because these are
> special memory regions that (at least upstream) kernels seldom support.
> Perhaps the most common use-cases are the shared DMA pools (such as
> CMA).

Also, not all regions need to be handled 'early' before slab allocator
or drivers are probed. Do you need early handling here? I can't see
why other than if fbcon is up early.

Rob

Re: [PATCH v2 4/7] drm/simpledrm: Add support for system memory framebuffers

[Thierry Reding]

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

On Mon, Oct 17, 2022 at 01:15:59PM -0500, Rob Herring wrote:
> On Mon, Oct 17, 2022 at 9:54 AM Thierry Reding <thierry.reding@gmail.com> wrote:
> >
> > On Mon, Oct 10, 2022 at 10:12:34AM +0200, Thomas Zimmermann wrote:
> > > Hi
> > >
> > > Am 07.10.22 um 14:49 schrieb Thierry Reding:
> > > > From: Thierry Reding <treding@nvidia.com>
> > > >
> > > > Simple framebuffers can be set up in system memory, which cannot be
> > > > requested and/or I/O remapped using the I/O resource helpers. Add a
> > > > separate code path that obtains system memory framebuffers from the
> > > > reserved memory region referenced in the memory-region property.
> > > >
> > > > v2: make screen base a struct iosys_map to avoid sparse warnings
> 
> [...]
> 
> > > > +static int simple_framebuffer_init(struct reserved_mem *rmem)
> > > > +{
> > > > +   pr_info("framebuffer memory at %pa, size %lu bytes\n", &rmem->base,
> > > > +           (unsigned long)rmem->size);
> > > > +
> > > > +   rmem->ops = &simple_framebuffer_ops;
> > > > +
> > > > +   return 0;
> > > > +}
> > > > +RESERVEDMEM_OF_DECLARE(simple_framebuffer, "framebuffer", simple_framebuffer_init);
> > >
> > > What's the prupose of these code at all?  I looked through the kernel, but
> > > there aren't many other examples of it.
> >
> > This is a fairly standard construct to deal with early memory
> > reservations. What happens is roughly this: during early kernel boot,
> > the reserved-memory core code will iterate over all children of the top-
> > level reserved-memory node and see if they have a compatible string that
> > matches one of the entries in the table created by these
> > RESERVEDMEM_OF_DECLARE entries. It will then call the init function for
> > a matched entry and register a struct reserved_mem for these. The init
> > function in this case just dumps an informational message to the boot
> > log to provide some information about the framebuffer region that was
> > reserved (which can be used for example for troubleshooting purposes)
> > and sets the device init/release operations (which will be called when a
> > device is associated with the reserved memory region, i.e. when the
> > of_reserved_mem_device_init_by_idx() function is called).
> >
> > The reason why there aren't many examples of this is because these are
> > special memory regions that (at least upstream) kernels seldom support.
> > Perhaps the most common use-cases are the shared DMA pools (such as
> > CMA).
> 
> Also, not all regions need to be handled 'early' before slab allocator
> or drivers are probed. Do you need early handling here? I can't see
> why other than if fbcon is up early.

No, I don't think this needs early handling. Obviously we want this to
be available as soon as possible, but since the framebuffer driver is
built on top of DRM and that all becomes available fairly late, I don't
think this could ever run *that* early.

So are you saying that in general if we don't need early handling we
should avoid RESERVEDMEM_OF_DECLARE and instead manually resolve the
memory regions and inspect them? In other words, RESERVEDMEM_OF_DECLARE
should only ever be used when this early handling is needed?

Thierry

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

Re: [PATCH v2 4/7] drm/simpledrm: Add support for system memory framebuffers

[Thierry Reding]

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

On Mon, Oct 17, 2022 at 01:15:59PM -0500, Rob Herring wrote:
> On Mon, Oct 17, 2022 at 9:54 AM Thierry Reding <thierry.reding@gmail.com> wrote:
> >
> > On Mon, Oct 10, 2022 at 10:12:34AM +0200, Thomas Zimmermann wrote:
> > > Hi
> > >
> > > Am 07.10.22 um 14:49 schrieb Thierry Reding:
> > > > From: Thierry Reding <treding@nvidia.com>
> > > >
> > > > Simple framebuffers can be set up in system memory, which cannot be
> > > > requested and/or I/O remapped using the I/O resource helpers. Add a
> > > > separate code path that obtains system memory framebuffers from the
> > > > reserved memory region referenced in the memory-region property.
> > > >
> > > > v2: make screen base a struct iosys_map to avoid sparse warnings
> 
> [...]
> 
> > > > +static int simple_framebuffer_init(struct reserved_mem *rmem)
> > > > +{
> > > > +   pr_info("framebuffer memory at %pa, size %lu bytes\n", &rmem->base,
> > > > +           (unsigned long)rmem->size);
> > > > +
> > > > +   rmem->ops = &simple_framebuffer_ops;
> > > > +
> > > > +   return 0;
> > > > +}
> > > > +RESERVEDMEM_OF_DECLARE(simple_framebuffer, "framebuffer", simple_framebuffer_init);
> > >
> > > What's the prupose of these code at all?  I looked through the kernel, but
> > > there aren't many other examples of it.
> >
> > This is a fairly standard construct to deal with early memory
> > reservations. What happens is roughly this: during early kernel boot,
> > the reserved-memory core code will iterate over all children of the top-
> > level reserved-memory node and see if they have a compatible string that
> > matches one of the entries in the table created by these
> > RESERVEDMEM_OF_DECLARE entries. It will then call the init function for
> > a matched entry and register a struct reserved_mem for these. The init
> > function in this case just dumps an informational message to the boot
> > log to provide some information about the framebuffer region that was
> > reserved (which can be used for example for troubleshooting purposes)
> > and sets the device init/release operations (which will be called when a
> > device is associated with the reserved memory region, i.e. when the
> > of_reserved_mem_device_init_by_idx() function is called).
> >
> > The reason why there aren't many examples of this is because these are
> > special memory regions that (at least upstream) kernels seldom support.
> > Perhaps the most common use-cases are the shared DMA pools (such as
> > CMA).
> 
> Also, not all regions need to be handled 'early' before slab allocator
> or drivers are probed. Do you need early handling here? I can't see
> why other than if fbcon is up early.

No, I don't think this needs early handling. Obviously we want this to
be available as soon as possible, but since the framebuffer driver is
built on top of DRM and that all becomes available fairly late, I don't
think this could ever run *that* early.

So are you saying that in general if we don't need early handling we
should avoid RESERVEDMEM_OF_DECLARE and instead manually resolve the
memory regions and inspect them? In other words, RESERVEDMEM_OF_DECLARE
should only ever be used when this early handling is needed?

Thierry

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

Re: [PATCH v2 4/7] drm/simpledrm: Add support for system memory framebuffers

[Thomas Zimmermann]

[-- Attachment #1.1: Type: text/plain, Size: 7658 bytes --]

Hi Thierry

Am 17.10.22 um 16:54 schrieb Thierry Reding:
> On Mon, Oct 10, 2022 at 10:12:34AM +0200, Thomas Zimmermann wrote:
[...]
>>
>> That whole 'Memory Manangement' block is will be unmaintainable. Before I go
>> into a detailed review, please see my questions about the reservedmem code
>> at the end of the patch.
> 
> It looks reasonably maintainable to me. Given that we only have __iomem
> and non-__iomem cases, this is about the extent of the complexity that
> could ever be added.

I think we should split the detection and usage, as the driver does with 
other properties. It would fit better into the driver's overall design. 
I'll send out another email with a review to illustrate what I have in mind.

> 
>>
>>>    /*
>>>     * Modesetting
>>>     */
>>> @@ -491,15 +594,15 @@ static void simpledrm_primary_plane_helper_atomic_update(struct drm_plane *plane
>>>    	drm_atomic_helper_damage_iter_init(&iter, old_plane_state, plane_state);
>>>    	drm_atomic_for_each_plane_damage(&iter, &damage) {
>>> -		struct iosys_map dst = IOSYS_MAP_INIT_VADDR(sdev->screen_base);
>>>    		struct drm_rect dst_clip = plane_state->dst;
>>>    		if (!drm_rect_intersect(&dst_clip, &damage))
>>>    			continue;
>>> -		iosys_map_incr(&dst, drm_fb_clip_offset(sdev->pitch, sdev->format, &dst_clip));
>>> -		drm_fb_blit(&dst, &sdev->pitch, sdev->format->format, shadow_plane_state->data, fb,
>>> -			    &damage);
>>> +		iosys_map_incr(&sdev->screen_base, drm_fb_clip_offset(sdev->pitch, sdev->format,
>>> +								      &dst_clip));
>>
>> You'll modify screen_base and it'll eventually blow up. You have to keep
>> initializing the dst variable within the loop.
>>
>>> +		drm_fb_blit(&sdev->screen_base, &sdev->pitch, sdev->format->format,
>>> +			    shadow_plane_state->data, fb, &damage);
>>>    	}
>>>    	drm_dev_exit(idx);
>>> @@ -518,7 +621,7 @@ static void simpledrm_primary_plane_helper_atomic_disable(struct drm_plane *plan
>>>    		return;
>>>    	/* Clear screen to black if disabled */
>>> -	memset_io(sdev->screen_base, 0, sdev->pitch * sdev->mode.vdisplay);
>>> +	iosys_map_memset(&sdev->screen_base, 0, 0, sdev->pitch * sdev->mode.vdisplay);
>>>    	drm_dev_exit(idx);
>>>    }
>>> @@ -635,8 +738,6 @@ static struct simpledrm_device *simpledrm_device_create(struct drm_driver *drv,
>>>    	struct drm_device *dev;
>>>    	int width, height, stride;
>>>    	const struct drm_format_info *format;
>>> -	struct resource *res, *mem;
>>> -	void __iomem *screen_base;
>>>    	struct drm_plane *primary_plane;
>>>    	struct drm_crtc *crtc;
>>>    	struct drm_encoder *encoder;
>>> @@ -706,35 +807,9 @@ static struct simpledrm_device *simpledrm_device_create(struct drm_driver *drv,
>>>    	drm_dbg(dev, "framebuffer format=%p4cc, size=%dx%d, stride=%d byte\n",
>>>    		&format->format, width, height, stride);
>>> -	/*
>>> -	 * Memory management
>>> -	 */
>>> -
>>> -	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>>> -	if (!res)
>>> -		return ERR_PTR(-EINVAL);
>>> -
>>> -	ret = devm_aperture_acquire_from_firmware(dev, res->start, resource_size(res));
>>> -	if (ret) {
>>> -		drm_err(dev, "could not acquire memory range %pr: error %d\n", res, ret);
>>> +	ret = simpledrm_device_init_mm(sdev);
>>> +	if (ret)
>>>    		return ERR_PTR(ret);
>>> -	}
>>> -
>>> -	mem = devm_request_mem_region(&pdev->dev, res->start, resource_size(res), drv->name);
>>> -	if (!mem) {
>>> -		/*
>>> -		 * We cannot make this fatal. Sometimes this comes from magic
>>> -		 * spaces our resource handlers simply don't know about. Use
>>> -		 * the I/O-memory resource as-is and try to map that instead.
>>> -		 */
>>> -		drm_warn(dev, "could not acquire memory region %pr\n", res);
>>> -		mem = res;
>>> -	}
>>> -
>>> -	screen_base = devm_ioremap_wc(&pdev->dev, mem->start, resource_size(mem));
>>> -	if (!screen_base)
>>> -		return ERR_PTR(-ENOMEM);
>>> -	sdev->screen_base = screen_base;
>>>    	/*
>>>    	 * Modesetting
>>> @@ -878,5 +953,35 @@ static struct platform_driver simpledrm_platform_driver = {
>>>    module_platform_driver(simpledrm_platform_driver);
>>> +static int simple_framebuffer_device_init(struct reserved_mem *rmem, struct device *dev)
>>> +{
>>> +	struct simpledrm_device *sdev = dev_get_drvdata(dev);
>>> +
>>> +	sdev->sysmem_start = rmem->base;
>>> +	sdev->sysmem_size = rmem->size;
>>
>>  From what I understand, you use the sysmem_ variables in the same code that
>> allocates the simpledrm_device, which creates a chicken-egg problem. When
>> does this code run?
> 
> This will run as a result of the of_reserved_mem_device_init_by_idx()
> call earlier. It might be possible to push more code from the sysmem
> initialization code path above into this function. That may also make
> the somewhat clunky sysmem_start/size fields unnecessary.
> 
> Alternatively, we may be able to skip the whole RESERVEDMEM_OF_DECLARE
> bits here and directly resolve the memory-region property and read its
> "reg" property. However it seemed more appropriate to use the existing
> infrastructure for this, even if it's rather minimal.

I agree. It would still be nicer if there was a version of 
of_reserved_mem_device_init_by_idx that returns the instance of 
reserved_mem instead of setting it in the device structure behind our back.

> 
>>
>>
>>> +
>>> +	return 0;
>>> +}
>>> +
>>> +static void simple_framebuffer_device_release(struct reserved_mem *rmem, struct device *dev)
>>> +{
>>> +}
>>> +
>>> +static const struct reserved_mem_ops simple_framebuffer_ops = {
>>> +	.device_init = simple_framebuffer_device_init,
>>> +	.device_release = simple_framebuffer_device_release,
>>> +};
>>> +
>>> +static int simple_framebuffer_init(struct reserved_mem *rmem)
>>> +{
>>> +	pr_info("framebuffer memory at %pa, size %lu bytes\n", &rmem->base,
>>> +		(unsigned long)rmem->size);
>>> +
>>> +	rmem->ops = &simple_framebuffer_ops;
>>> +
>>> +	return 0;
>>> +}
>>> +RESERVEDMEM_OF_DECLARE(simple_framebuffer, "framebuffer", simple_framebuffer_init);
>>
>> What's the prupose of these code at all?  I looked through the kernel, but
>> there aren't many other examples of it.
> 
> This is a fairly standard construct to deal with early memory
> reservations. What happens is roughly this: during early kernel boot,
> the reserved-memory core code will iterate over all children of the top-
> level reserved-memory node and see if they have a compatible string that
> matches one of the entries in the table created by these
> RESERVEDMEM_OF_DECLARE entries. It will then call the init function for
> a matched entry and register a struct reserved_mem for these. The init
> function in this case just dumps an informational message to the boot
> log to provide some information about the framebuffer region that was
> reserved (which can be used for example for troubleshooting purposes)
> and sets the device init/release operations (which will be called when a
> device is associated with the reserved memory region, i.e. when the
> of_reserved_mem_device_init_by_idx() function is called).
> 
> The reason why there aren't many examples of this is because these are
> special memory regions that (at least upstream) kernels seldom support.
> Perhaps the most common use-cases are the shared DMA pools (such as
> CMA).

Thanks for the information.

Best regards
Thomas

> 
> Thierry

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Ivo Totev

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]

Re: [PATCH v2 4/7] drm/simpledrm: Add support for system memory framebuffers

[Thomas Zimmermann]

[-- Attachment #1.1: Type: text/plain, Size: 7658 bytes --]

Hi Thierry

Am 17.10.22 um 16:54 schrieb Thierry Reding:
> On Mon, Oct 10, 2022 at 10:12:34AM +0200, Thomas Zimmermann wrote:
[...]
>>
>> That whole 'Memory Manangement' block is will be unmaintainable. Before I go
>> into a detailed review, please see my questions about the reservedmem code
>> at the end of the patch.
> 
> It looks reasonably maintainable to me. Given that we only have __iomem
> and non-__iomem cases, this is about the extent of the complexity that
> could ever be added.

I think we should split the detection and usage, as the driver does with 
other properties. It would fit better into the driver's overall design. 
I'll send out another email with a review to illustrate what I have in mind.

> 
>>
>>>    /*
>>>     * Modesetting
>>>     */
>>> @@ -491,15 +594,15 @@ static void simpledrm_primary_plane_helper_atomic_update(struct drm_plane *plane
>>>    	drm_atomic_helper_damage_iter_init(&iter, old_plane_state, plane_state);
>>>    	drm_atomic_for_each_plane_damage(&iter, &damage) {
>>> -		struct iosys_map dst = IOSYS_MAP_INIT_VADDR(sdev->screen_base);
>>>    		struct drm_rect dst_clip = plane_state->dst;
>>>    		if (!drm_rect_intersect(&dst_clip, &damage))
>>>    			continue;
>>> -		iosys_map_incr(&dst, drm_fb_clip_offset(sdev->pitch, sdev->format, &dst_clip));
>>> -		drm_fb_blit(&dst, &sdev->pitch, sdev->format->format, shadow_plane_state->data, fb,
>>> -			    &damage);
>>> +		iosys_map_incr(&sdev->screen_base, drm_fb_clip_offset(sdev->pitch, sdev->format,
>>> +								      &dst_clip));
>>
>> You'll modify screen_base and it'll eventually blow up. You have to keep
>> initializing the dst variable within the loop.
>>
>>> +		drm_fb_blit(&sdev->screen_base, &sdev->pitch, sdev->format->format,
>>> +			    shadow_plane_state->data, fb, &damage);
>>>    	}
>>>    	drm_dev_exit(idx);
>>> @@ -518,7 +621,7 @@ static void simpledrm_primary_plane_helper_atomic_disable(struct drm_plane *plan
>>>    		return;
>>>    	/* Clear screen to black if disabled */
>>> -	memset_io(sdev->screen_base, 0, sdev->pitch * sdev->mode.vdisplay);
>>> +	iosys_map_memset(&sdev->screen_base, 0, 0, sdev->pitch * sdev->mode.vdisplay);
>>>    	drm_dev_exit(idx);
>>>    }
>>> @@ -635,8 +738,6 @@ static struct simpledrm_device *simpledrm_device_create(struct drm_driver *drv,
>>>    	struct drm_device *dev;
>>>    	int width, height, stride;
>>>    	const struct drm_format_info *format;
>>> -	struct resource *res, *mem;
>>> -	void __iomem *screen_base;
>>>    	struct drm_plane *primary_plane;
>>>    	struct drm_crtc *crtc;
>>>    	struct drm_encoder *encoder;
>>> @@ -706,35 +807,9 @@ static struct simpledrm_device *simpledrm_device_create(struct drm_driver *drv,
>>>    	drm_dbg(dev, "framebuffer format=%p4cc, size=%dx%d, stride=%d byte\n",
>>>    		&format->format, width, height, stride);
>>> -	/*
>>> -	 * Memory management
>>> -	 */
>>> -
>>> -	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>>> -	if (!res)
>>> -		return ERR_PTR(-EINVAL);
>>> -
>>> -	ret = devm_aperture_acquire_from_firmware(dev, res->start, resource_size(res));
>>> -	if (ret) {
>>> -		drm_err(dev, "could not acquire memory range %pr: error %d\n", res, ret);
>>> +	ret = simpledrm_device_init_mm(sdev);
>>> +	if (ret)
>>>    		return ERR_PTR(ret);
>>> -	}
>>> -
>>> -	mem = devm_request_mem_region(&pdev->dev, res->start, resource_size(res), drv->name);
>>> -	if (!mem) {
>>> -		/*
>>> -		 * We cannot make this fatal. Sometimes this comes from magic
>>> -		 * spaces our resource handlers simply don't know about. Use
>>> -		 * the I/O-memory resource as-is and try to map that instead.
>>> -		 */
>>> -		drm_warn(dev, "could not acquire memory region %pr\n", res);
>>> -		mem = res;
>>> -	}
>>> -
>>> -	screen_base = devm_ioremap_wc(&pdev->dev, mem->start, resource_size(mem));
>>> -	if (!screen_base)
>>> -		return ERR_PTR(-ENOMEM);
>>> -	sdev->screen_base = screen_base;
>>>    	/*
>>>    	 * Modesetting
>>> @@ -878,5 +953,35 @@ static struct platform_driver simpledrm_platform_driver = {
>>>    module_platform_driver(simpledrm_platform_driver);
>>> +static int simple_framebuffer_device_init(struct reserved_mem *rmem, struct device *dev)
>>> +{
>>> +	struct simpledrm_device *sdev = dev_get_drvdata(dev);
>>> +
>>> +	sdev->sysmem_start = rmem->base;
>>> +	sdev->sysmem_size = rmem->size;
>>
>>  From what I understand, you use the sysmem_ variables in the same code that
>> allocates the simpledrm_device, which creates a chicken-egg problem. When
>> does this code run?
> 
> This will run as a result of the of_reserved_mem_device_init_by_idx()
> call earlier. It might be possible to push more code from the sysmem
> initialization code path above into this function. That may also make
> the somewhat clunky sysmem_start/size fields unnecessary.
> 
> Alternatively, we may be able to skip the whole RESERVEDMEM_OF_DECLARE
> bits here and directly resolve the memory-region property and read its
> "reg" property. However it seemed more appropriate to use the existing
> infrastructure for this, even if it's rather minimal.

I agree. It would still be nicer if there was a version of 
of_reserved_mem_device_init_by_idx that returns the instance of 
reserved_mem instead of setting it in the device structure behind our back.

> 
>>
>>
>>> +
>>> +	return 0;
>>> +}
>>> +
>>> +static void simple_framebuffer_device_release(struct reserved_mem *rmem, struct device *dev)
>>> +{
>>> +}
>>> +
>>> +static const struct reserved_mem_ops simple_framebuffer_ops = {
>>> +	.device_init = simple_framebuffer_device_init,
>>> +	.device_release = simple_framebuffer_device_release,
>>> +};
>>> +
>>> +static int simple_framebuffer_init(struct reserved_mem *rmem)
>>> +{
>>> +	pr_info("framebuffer memory at %pa, size %lu bytes\n", &rmem->base,
>>> +		(unsigned long)rmem->size);
>>> +
>>> +	rmem->ops = &simple_framebuffer_ops;
>>> +
>>> +	return 0;
>>> +}
>>> +RESERVEDMEM_OF_DECLARE(simple_framebuffer, "framebuffer", simple_framebuffer_init);
>>
>> What's the prupose of these code at all?  I looked through the kernel, but
>> there aren't many other examples of it.
> 
> This is a fairly standard construct to deal with early memory
> reservations. What happens is roughly this: during early kernel boot,
> the reserved-memory core code will iterate over all children of the top-
> level reserved-memory node and see if they have a compatible string that
> matches one of the entries in the table created by these
> RESERVEDMEM_OF_DECLARE entries. It will then call the init function for
> a matched entry and register a struct reserved_mem for these. The init
> function in this case just dumps an informational message to the boot
> log to provide some information about the framebuffer region that was
> reserved (which can be used for example for troubleshooting purposes)
> and sets the device init/release operations (which will be called when a
> device is associated with the reserved memory region, i.e. when the
> of_reserved_mem_device_init_by_idx() function is called).
> 
> The reason why there aren't many examples of this is because these are
> special memory regions that (at least upstream) kernels seldom support.
> Perhaps the most common use-cases are the shared DMA pools (such as
> CMA).

Thanks for the information.

Best regards
Thomas

> 
> Thierry

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Ivo Totev

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]

Re: [PATCH v2 4/7] drm/simpledrm: Add support for system memory framebuffers

[Thierry Reding]

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

On Tue, Oct 18, 2022 at 01:58:53PM +0200, Thomas Zimmermann wrote:
> Hi Thierry
> 
> Am 17.10.22 um 16:54 schrieb Thierry Reding:
> > On Mon, Oct 10, 2022 at 10:12:34AM +0200, Thomas Zimmermann wrote:
> [...]
> > > 
> > > That whole 'Memory Manangement' block is will be unmaintainable. Before I go
> > > into a detailed review, please see my questions about the reservedmem code
> > > at the end of the patch.
> > 
> > It looks reasonably maintainable to me. Given that we only have __iomem
> > and non-__iomem cases, this is about the extent of the complexity that
> > could ever be added.
> 
> I think we should split the detection and usage, as the driver does with
> other properties. It would fit better into the driver's overall design. I'll
> send out another email with a review to illustrate what I have in mind.

Okay, great.

> > > >    /*
> > > >     * Modesetting
> > > >     */
> > > > @@ -491,15 +594,15 @@ static void simpledrm_primary_plane_helper_atomic_update(struct drm_plane *plane
> > > >    	drm_atomic_helper_damage_iter_init(&iter, old_plane_state, plane_state);
> > > >    	drm_atomic_for_each_plane_damage(&iter, &damage) {
> > > > -		struct iosys_map dst = IOSYS_MAP_INIT_VADDR(sdev->screen_base);
> > > >    		struct drm_rect dst_clip = plane_state->dst;
> > > >    		if (!drm_rect_intersect(&dst_clip, &damage))
> > > >    			continue;
> > > > -		iosys_map_incr(&dst, drm_fb_clip_offset(sdev->pitch, sdev->format, &dst_clip));
> > > > -		drm_fb_blit(&dst, &sdev->pitch, sdev->format->format, shadow_plane_state->data, fb,
> > > > -			    &damage);
> > > > +		iosys_map_incr(&sdev->screen_base, drm_fb_clip_offset(sdev->pitch, sdev->format,
> > > > +								      &dst_clip));
> > > 
> > > You'll modify screen_base and it'll eventually blow up. You have to keep
> > > initializing the dst variable within the loop.
> > > 
> > > > +		drm_fb_blit(&sdev->screen_base, &sdev->pitch, sdev->format->format,
> > > > +			    shadow_plane_state->data, fb, &damage);
> > > >    	}
> > > >    	drm_dev_exit(idx);
> > > > @@ -518,7 +621,7 @@ static void simpledrm_primary_plane_helper_atomic_disable(struct drm_plane *plan
> > > >    		return;
> > > >    	/* Clear screen to black if disabled */
> > > > -	memset_io(sdev->screen_base, 0, sdev->pitch * sdev->mode.vdisplay);
> > > > +	iosys_map_memset(&sdev->screen_base, 0, 0, sdev->pitch * sdev->mode.vdisplay);
> > > >    	drm_dev_exit(idx);
> > > >    }
> > > > @@ -635,8 +738,6 @@ static struct simpledrm_device *simpledrm_device_create(struct drm_driver *drv,
> > > >    	struct drm_device *dev;
> > > >    	int width, height, stride;
> > > >    	const struct drm_format_info *format;
> > > > -	struct resource *res, *mem;
> > > > -	void __iomem *screen_base;
> > > >    	struct drm_plane *primary_plane;
> > > >    	struct drm_crtc *crtc;
> > > >    	struct drm_encoder *encoder;
> > > > @@ -706,35 +807,9 @@ static struct simpledrm_device *simpledrm_device_create(struct drm_driver *drv,
> > > >    	drm_dbg(dev, "framebuffer format=%p4cc, size=%dx%d, stride=%d byte\n",
> > > >    		&format->format, width, height, stride);
> > > > -	/*
> > > > -	 * Memory management
> > > > -	 */
> > > > -
> > > > -	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > > > -	if (!res)
> > > > -		return ERR_PTR(-EINVAL);
> > > > -
> > > > -	ret = devm_aperture_acquire_from_firmware(dev, res->start, resource_size(res));
> > > > -	if (ret) {
> > > > -		drm_err(dev, "could not acquire memory range %pr: error %d\n", res, ret);
> > > > +	ret = simpledrm_device_init_mm(sdev);
> > > > +	if (ret)
> > > >    		return ERR_PTR(ret);
> > > > -	}
> > > > -
> > > > -	mem = devm_request_mem_region(&pdev->dev, res->start, resource_size(res), drv->name);
> > > > -	if (!mem) {
> > > > -		/*
> > > > -		 * We cannot make this fatal. Sometimes this comes from magic
> > > > -		 * spaces our resource handlers simply don't know about. Use
> > > > -		 * the I/O-memory resource as-is and try to map that instead.
> > > > -		 */
> > > > -		drm_warn(dev, "could not acquire memory region %pr\n", res);
> > > > -		mem = res;
> > > > -	}
> > > > -
> > > > -	screen_base = devm_ioremap_wc(&pdev->dev, mem->start, resource_size(mem));
> > > > -	if (!screen_base)
> > > > -		return ERR_PTR(-ENOMEM);
> > > > -	sdev->screen_base = screen_base;
> > > >    	/*
> > > >    	 * Modesetting
> > > > @@ -878,5 +953,35 @@ static struct platform_driver simpledrm_platform_driver = {
> > > >    module_platform_driver(simpledrm_platform_driver);
> > > > +static int simple_framebuffer_device_init(struct reserved_mem *rmem, struct device *dev)
> > > > +{
> > > > +	struct simpledrm_device *sdev = dev_get_drvdata(dev);
> > > > +
> > > > +	sdev->sysmem_start = rmem->base;
> > > > +	sdev->sysmem_size = rmem->size;
> > > 
> > >  From what I understand, you use the sysmem_ variables in the same code that
> > > allocates the simpledrm_device, which creates a chicken-egg problem. When
> > > does this code run?
> > 
> > This will run as a result of the of_reserved_mem_device_init_by_idx()
> > call earlier. It might be possible to push more code from the sysmem
> > initialization code path above into this function. That may also make
> > the somewhat clunky sysmem_start/size fields unnecessary.
> > 
> > Alternatively, we may be able to skip the whole RESERVEDMEM_OF_DECLARE
> > bits here and directly resolve the memory-region property and read its
> > "reg" property. However it seemed more appropriate to use the existing
> > infrastructure for this, even if it's rather minimal.
> 
> I agree. It would still be nicer if there was a version of
> of_reserved_mem_device_init_by_idx that returns the instance of reserved_mem
> instead of setting it in the device structure behind our back.

There's of_reserved_mem_lookup() which does that, or at least something
close to that. Ultimately, as Rob mentioned, we may not need any of this
infrastructure and can just directly parse the node from the driver.
This should allow us to avoid any of this infrastructure (i.e. the extra
indirection) and encapsulate the handling of this better.

I have a couple of ideas, but I'll wait for your feedback to work that
in as well.

Thierry

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

Re: [PATCH v2 4/7] drm/simpledrm: Add support for system memory framebuffers

[Thierry Reding]

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

On Tue, Oct 18, 2022 at 01:58:53PM +0200, Thomas Zimmermann wrote:
> Hi Thierry
> 
> Am 17.10.22 um 16:54 schrieb Thierry Reding:
> > On Mon, Oct 10, 2022 at 10:12:34AM +0200, Thomas Zimmermann wrote:
> [...]
> > > 
> > > That whole 'Memory Manangement' block is will be unmaintainable. Before I go
> > > into a detailed review, please see my questions about the reservedmem code
> > > at the end of the patch.
> > 
> > It looks reasonably maintainable to me. Given that we only have __iomem
> > and non-__iomem cases, this is about the extent of the complexity that
> > could ever be added.
> 
> I think we should split the detection and usage, as the driver does with
> other properties. It would fit better into the driver's overall design. I'll
> send out another email with a review to illustrate what I have in mind.

Okay, great.

> > > >    /*
> > > >     * Modesetting
> > > >     */
> > > > @@ -491,15 +594,15 @@ static void simpledrm_primary_plane_helper_atomic_update(struct drm_plane *plane
> > > >    	drm_atomic_helper_damage_iter_init(&iter, old_plane_state, plane_state);
> > > >    	drm_atomic_for_each_plane_damage(&iter, &damage) {
> > > > -		struct iosys_map dst = IOSYS_MAP_INIT_VADDR(sdev->screen_base);
> > > >    		struct drm_rect dst_clip = plane_state->dst;
> > > >    		if (!drm_rect_intersect(&dst_clip, &damage))
> > > >    			continue;
> > > > -		iosys_map_incr(&dst, drm_fb_clip_offset(sdev->pitch, sdev->format, &dst_clip));
> > > > -		drm_fb_blit(&dst, &sdev->pitch, sdev->format->format, shadow_plane_state->data, fb,
> > > > -			    &damage);
> > > > +		iosys_map_incr(&sdev->screen_base, drm_fb_clip_offset(sdev->pitch, sdev->format,
> > > > +								      &dst_clip));
> > > 
> > > You'll modify screen_base and it'll eventually blow up. You have to keep
> > > initializing the dst variable within the loop.
> > > 
> > > > +		drm_fb_blit(&sdev->screen_base, &sdev->pitch, sdev->format->format,
> > > > +			    shadow_plane_state->data, fb, &damage);
> > > >    	}
> > > >    	drm_dev_exit(idx);
> > > > @@ -518,7 +621,7 @@ static void simpledrm_primary_plane_helper_atomic_disable(struct drm_plane *plan
> > > >    		return;
> > > >    	/* Clear screen to black if disabled */
> > > > -	memset_io(sdev->screen_base, 0, sdev->pitch * sdev->mode.vdisplay);
> > > > +	iosys_map_memset(&sdev->screen_base, 0, 0, sdev->pitch * sdev->mode.vdisplay);
> > > >    	drm_dev_exit(idx);
> > > >    }
> > > > @@ -635,8 +738,6 @@ static struct simpledrm_device *simpledrm_device_create(struct drm_driver *drv,
> > > >    	struct drm_device *dev;
> > > >    	int width, height, stride;
> > > >    	const struct drm_format_info *format;
> > > > -	struct resource *res, *mem;
> > > > -	void __iomem *screen_base;
> > > >    	struct drm_plane *primary_plane;
> > > >    	struct drm_crtc *crtc;
> > > >    	struct drm_encoder *encoder;
> > > > @@ -706,35 +807,9 @@ static struct simpledrm_device *simpledrm_device_create(struct drm_driver *drv,
> > > >    	drm_dbg(dev, "framebuffer format=%p4cc, size=%dx%d, stride=%d byte\n",
> > > >    		&format->format, width, height, stride);
> > > > -	/*
> > > > -	 * Memory management
> > > > -	 */
> > > > -
> > > > -	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > > > -	if (!res)
> > > > -		return ERR_PTR(-EINVAL);
> > > > -
> > > > -	ret = devm_aperture_acquire_from_firmware(dev, res->start, resource_size(res));
> > > > -	if (ret) {
> > > > -		drm_err(dev, "could not acquire memory range %pr: error %d\n", res, ret);
> > > > +	ret = simpledrm_device_init_mm(sdev);
> > > > +	if (ret)
> > > >    		return ERR_PTR(ret);
> > > > -	}
> > > > -
> > > > -	mem = devm_request_mem_region(&pdev->dev, res->start, resource_size(res), drv->name);
> > > > -	if (!mem) {
> > > > -		/*
> > > > -		 * We cannot make this fatal. Sometimes this comes from magic
> > > > -		 * spaces our resource handlers simply don't know about. Use
> > > > -		 * the I/O-memory resource as-is and try to map that instead.
> > > > -		 */
> > > > -		drm_warn(dev, "could not acquire memory region %pr\n", res);
> > > > -		mem = res;
> > > > -	}
> > > > -
> > > > -	screen_base = devm_ioremap_wc(&pdev->dev, mem->start, resource_size(mem));
> > > > -	if (!screen_base)
> > > > -		return ERR_PTR(-ENOMEM);
> > > > -	sdev->screen_base = screen_base;
> > > >    	/*
> > > >    	 * Modesetting
> > > > @@ -878,5 +953,35 @@ static struct platform_driver simpledrm_platform_driver = {
> > > >    module_platform_driver(simpledrm_platform_driver);
> > > > +static int simple_framebuffer_device_init(struct reserved_mem *rmem, struct device *dev)
> > > > +{
> > > > +	struct simpledrm_device *sdev = dev_get_drvdata(dev);
> > > > +
> > > > +	sdev->sysmem_start = rmem->base;
> > > > +	sdev->sysmem_size = rmem->size;
> > > 
> > >  From what I understand, you use the sysmem_ variables in the same code that
> > > allocates the simpledrm_device, which creates a chicken-egg problem. When
> > > does this code run?
> > 
> > This will run as a result of the of_reserved_mem_device_init_by_idx()
> > call earlier. It might be possible to push more code from the sysmem
> > initialization code path above into this function. That may also make
> > the somewhat clunky sysmem_start/size fields unnecessary.
> > 
> > Alternatively, we may be able to skip the whole RESERVEDMEM_OF_DECLARE
> > bits here and directly resolve the memory-region property and read its
> > "reg" property. However it seemed more appropriate to use the existing
> > infrastructure for this, even if it's rather minimal.
> 
> I agree. It would still be nicer if there was a version of
> of_reserved_mem_device_init_by_idx that returns the instance of reserved_mem
> instead of setting it in the device structure behind our back.

There's of_reserved_mem_lookup() which does that, or at least something
close to that. Ultimately, as Rob mentioned, we may not need any of this
infrastructure and can just directly parse the node from the driver.
This should allow us to avoid any of this infrastructure (i.e. the extra
indirection) and encapsulate the handling of this better.

I have a couple of ideas, but I'll wait for your feedback to work that
in as well.

Thierry

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

Re: [PATCH v2 4/7] drm/simpledrm: Add support for system memory framebuffers

[Rob Herring]

On Tue, Oct 18, 2022 at 5:47 AM Thierry Reding <thierry.reding@gmail.com> wrote:
>
> On Mon, Oct 17, 2022 at 01:15:59PM -0500, Rob Herring wrote:
> > On Mon, Oct 17, 2022 at 9:54 AM Thierry Reding <thierry.reding@gmail.com> wrote:
> > >
> > > On Mon, Oct 10, 2022 at 10:12:34AM +0200, Thomas Zimmermann wrote:
> > > > Hi
> > > >
> > > > Am 07.10.22 um 14:49 schrieb Thierry Reding:
> > > > > From: Thierry Reding <treding@nvidia.com>
> > > > >
> > > > > Simple framebuffers can be set up in system memory, which cannot be
> > > > > requested and/or I/O remapped using the I/O resource helpers. Add a
> > > > > separate code path that obtains system memory framebuffers from the
> > > > > reserved memory region referenced in the memory-region property.
> > > > >
> > > > > v2: make screen base a struct iosys_map to avoid sparse warnings
> >
> > [...]
> >
> > > > > +static int simple_framebuffer_init(struct reserved_mem *rmem)
> > > > > +{
> > > > > +   pr_info("framebuffer memory at %pa, size %lu bytes\n", &rmem->base,
> > > > > +           (unsigned long)rmem->size);
> > > > > +
> > > > > +   rmem->ops = &simple_framebuffer_ops;
> > > > > +
> > > > > +   return 0;
> > > > > +}
> > > > > +RESERVEDMEM_OF_DECLARE(simple_framebuffer, "framebuffer", simple_framebuffer_init);
> > > >
> > > > What's the prupose of these code at all?  I looked through the kernel, but
> > > > there aren't many other examples of it.
> > >
> > > This is a fairly standard construct to deal with early memory
> > > reservations. What happens is roughly this: during early kernel boot,
> > > the reserved-memory core code will iterate over all children of the top-
> > > level reserved-memory node and see if they have a compatible string that
> > > matches one of the entries in the table created by these
> > > RESERVEDMEM_OF_DECLARE entries. It will then call the init function for
> > > a matched entry and register a struct reserved_mem for these. The init
> > > function in this case just dumps an informational message to the boot
> > > log to provide some information about the framebuffer region that was
> > > reserved (which can be used for example for troubleshooting purposes)
> > > and sets the device init/release operations (which will be called when a
> > > device is associated with the reserved memory region, i.e. when the
> > > of_reserved_mem_device_init_by_idx() function is called).
> > >
> > > The reason why there aren't many examples of this is because these are
> > > special memory regions that (at least upstream) kernels seldom support.
> > > Perhaps the most common use-cases are the shared DMA pools (such as
> > > CMA).
> >
> > Also, not all regions need to be handled 'early' before slab allocator
> > or drivers are probed. Do you need early handling here? I can't see
> > why other than if fbcon is up early.
>
> No, I don't think this needs early handling. Obviously we want this to
> be available as soon as possible, but since the framebuffer driver is
> built on top of DRM and that all becomes available fairly late, I don't
> think this could ever run *that* early.
>
> So are you saying that in general if we don't need early handling we
> should avoid RESERVEDMEM_OF_DECLARE and instead manually resolve the
> memory regions and inspect them? In other words, RESERVEDMEM_OF_DECLARE
> should only ever be used when this early handling is needed?

Right. Like all the other *_OF_DECLARE() macros, they are only for
when needed before driver probe time. Lots of shared memory mailbox
drivers use reserved-memory regions if you need examples.

Rob

Re: [PATCH v2 4/7] drm/simpledrm: Add support for system memory framebuffers

[Rob Herring]

On Tue, Oct 18, 2022 at 5:47 AM Thierry Reding <thierry.reding@gmail.com> wrote:
>
> On Mon, Oct 17, 2022 at 01:15:59PM -0500, Rob Herring wrote:
> > On Mon, Oct 17, 2022 at 9:54 AM Thierry Reding <thierry.reding@gmail.com> wrote:
> > >
> > > On Mon, Oct 10, 2022 at 10:12:34AM +0200, Thomas Zimmermann wrote:
> > > > Hi
> > > >
> > > > Am 07.10.22 um 14:49 schrieb Thierry Reding:
> > > > > From: Thierry Reding <treding@nvidia.com>
> > > > >
> > > > > Simple framebuffers can be set up in system memory, which cannot be
> > > > > requested and/or I/O remapped using the I/O resource helpers. Add a
> > > > > separate code path that obtains system memory framebuffers from the
> > > > > reserved memory region referenced in the memory-region property.
> > > > >
> > > > > v2: make screen base a struct iosys_map to avoid sparse warnings
> >
> > [...]
> >
> > > > > +static int simple_framebuffer_init(struct reserved_mem *rmem)
> > > > > +{
> > > > > +   pr_info("framebuffer memory at %pa, size %lu bytes\n", &rmem->base,
> > > > > +           (unsigned long)rmem->size);
> > > > > +
> > > > > +   rmem->ops = &simple_framebuffer_ops;
> > > > > +
> > > > > +   return 0;
> > > > > +}
> > > > > +RESERVEDMEM_OF_DECLARE(simple_framebuffer, "framebuffer", simple_framebuffer_init);
> > > >
> > > > What's the prupose of these code at all?  I looked through the kernel, but
> > > > there aren't many other examples of it.
> > >
> > > This is a fairly standard construct to deal with early memory
> > > reservations. What happens is roughly this: during early kernel boot,
> > > the reserved-memory core code will iterate over all children of the top-
> > > level reserved-memory node and see if they have a compatible string that
> > > matches one of the entries in the table created by these
> > > RESERVEDMEM_OF_DECLARE entries. It will then call the init function for
> > > a matched entry and register a struct reserved_mem for these. The init
> > > function in this case just dumps an informational message to the boot
> > > log to provide some information about the framebuffer region that was
> > > reserved (which can be used for example for troubleshooting purposes)
> > > and sets the device init/release operations (which will be called when a
> > > device is associated with the reserved memory region, i.e. when the
> > > of_reserved_mem_device_init_by_idx() function is called).
> > >
> > > The reason why there aren't many examples of this is because these are
> > > special memory regions that (at least upstream) kernels seldom support.
> > > Perhaps the most common use-cases are the shared DMA pools (such as
> > > CMA).
> >
> > Also, not all regions need to be handled 'early' before slab allocator
> > or drivers are probed. Do you need early handling here? I can't see
> > why other than if fbcon is up early.
>
> No, I don't think this needs early handling. Obviously we want this to
> be available as soon as possible, but since the framebuffer driver is
> built on top of DRM and that all becomes available fairly late, I don't
> think this could ever run *that* early.
>
> So are you saying that in general if we don't need early handling we
> should avoid RESERVEDMEM_OF_DECLARE and instead manually resolve the
> memory regions and inspect them? In other words, RESERVEDMEM_OF_DECLARE
> should only ever be used when this early handling is needed?

Right. Like all the other *_OF_DECLARE() macros, they are only for
when needed before driver probe time. Lots of shared memory mailbox
drivers use reserved-memory regions if you need examples.

Rob

Re: [PATCH v2 4/7] drm/simpledrm: Add support for system memory framebuffers

[Thomas Zimmermann]

[-- Attachment #1.1: Type: text/plain, Size: 11389 bytes --]

Hi,

please see my review below.

Am 07.10.22 um 14:49 schrieb Thierry Reding:
> From: Thierry Reding <treding@nvidia.com>
> 
> Simple framebuffers can be set up in system memory, which cannot be
> requested and/or I/O remapped using the I/O resource helpers. Add a
> separate code path that obtains system memory framebuffers from the
> reserved memory region referenced in the memory-region property.
> 
> v2: make screen base a struct iosys_map to avoid sparse warnings
> 
> Signed-off-by: Thierry Reding <treding@nvidia.com>
> ---
>   drivers/gpu/drm/tiny/simpledrm.c | 177 ++++++++++++++++++++++++-------
>   1 file changed, 141 insertions(+), 36 deletions(-)
> 
> diff --git a/drivers/gpu/drm/tiny/simpledrm.c b/drivers/gpu/drm/tiny/simpledrm.c
> index 18489779fb8a..cf36f67d22e4 100644
> --- a/drivers/gpu/drm/tiny/simpledrm.c
> +++ b/drivers/gpu/drm/tiny/simpledrm.c
> @@ -2,6 +2,7 @@
>   
>   #include <linux/clk.h>
>   #include <linux/of_clk.h>
> +#include <linux/of_reserved_mem.h>
>   #include <linux/minmax.h>
>   #include <linux/platform_data/simplefb.h>
>   #include <linux/platform_device.h>
> @@ -207,7 +208,9 @@ struct simpledrm_device {
>   	unsigned int pitch;
>   
>   	/* memory management */
> -	void __iomem *screen_base;
> +	struct iosys_map screen_base;
> +	phys_addr_t sysmem_start;
> +	size_t sysmem_size;

Please store a pointer to the struct reserved_mem here. If set, we use 
it; if NULL, we use the memory region as before.

>   
>   	/* modesetting */
>   	uint32_t formats[8];
> @@ -441,6 +444,106 @@ static int simpledrm_device_init_regulators(struct simpledrm_device *sdev)
>   }
>   #endif
>   
> +/*
> + * Memory management
> + */
> +
> +static int simpledrm_device_init_mm_sysmem(struct simpledrm_device *sdev, phys_addr_t start,
> +					   size_t size)
> +{
> +	struct drm_device *dev = &sdev->dev;
> +	phys_addr_t end = start + size - 1;
> +	void *screen_base;
> +
> +	drm_info(dev, "using system memory framebuffer at [%pa-%pa]\n", &start, &end);
> +
> +	screen_base = devm_memremap(dev->dev, start, size, MEMREMAP_WC);
> +	if (!screen_base)
> +		return -ENOMEM;
> +
> +	iosys_map_set_vaddr(&sdev->screen_base, screen_base);
> +
> +	return 0;
> +}
> +
> +static int simpledrm_device_init_mm_io(struct simpledrm_device *sdev, phys_addr_t start,
> +				       size_t size)
> +{
> +	struct drm_device *dev = &sdev->dev;
> +	phys_addr_t end = start + size - 1;
> +	void __iomem *screen_base;
> +	struct resource *mem;
> +
> +	drm_info(dev, "using I/O memory framebuffer at [%pa-%pa]\n", &start, &end);
> +
> +	mem = devm_request_mem_region(dev->dev, start, size, sdev->dev.driver->name);
> +	if (!mem) {
> +		/*
> +		 * We cannot make this fatal. Sometimes this comes from magic
> +		 * spaces our resource handlers simply don't know about. Use
> +		 * the I/O-memory resource as-is and try to map that instead.
> +		 */
> +		drm_warn(dev, "could not acquire memory region [%pa-%pa]\n", &start, &end);
> +	} else {
> +		size = resource_size(mem);
> +		start = mem->start;
> +	}
> +
> +	screen_base = devm_ioremap_wc(dev->dev, start, size);
> +	if (!screen_base)
> +		return -ENOMEM;
> +
> +	iosys_map_set_vaddr_iomem(&sdev->screen_base, screen_base);
> +
> +	return 0;
> +}
> +
> +static void simpledrm_device_exit_mm(void *data)
> +{
> +	struct simpledrm_device *sdev = data;
> +	struct drm_device *dev = &sdev->dev;
> +
> +	of_reserved_mem_device_release(dev->dev);
> +}
> +
> +static int simpledrm_device_init_mm(struct simpledrm_device *sdev)
> +{

No such _init_mm() helper please. Simply do everything in the _create() 
function.


> +	int (*init)(struct simpledrm_device *sdev, phys_addr_t start, size_t size);
> +	struct drm_device *dev = &sdev->dev;
> +	struct platform_device *pdev = to_platform_device(dev->dev);
> +	phys_addr_t start, end;
> +	size_t size;
> +	int ret;
> +
> +	ret = of_reserved_mem_device_init_by_idx(dev->dev, dev->dev->of_node, 0);

Instead of doing our own lookup of the memory node, I'd prefer to keep 
this function. If we encasulate it, it's better than reinventing the 
wheel within this driver.

A number of helpers look-up the various properties from the device tree. 
They are at [1] and below. We should add a new helper that reads the 
memory node via of_reserved_mem_device_init_by_idx(). Here's some 
example code

struct reserved_mem *
simplefb_get_memory_of(drm_device dev, of_node node)
{
	sdev = simpledrm_device_of_dev(dev)

	ret = of_reserved_mem_device_init_by_idx()
	if (ret)
		return ERR_PTR()

	ret = devm_add_action_or_reset(dev->dev,
		simpledrm_device_exit_mm, sdev)
	if (ret)
		return ERR_PTR()

	return sdev->rmem;
}

That wraps the rmem instance quite nicely and is similar to the rest of 
the code. You can call this function within the else branch at [2].

Later in _create() where we map the I/O memory, branch depending on the 
returned value of rmem.

if (rmem) {

   devm_aperture_acquire_from_firmware()
   devm_memremap()

} else {

   platform_get_resource()
   devm_aperture_acquire_from_firmware()
   devm_request_mem_region()
   devm_ioremap_wc()

}

Don't bother with reducing the code size. The only call that is sharable 
is devm_aperture_acquire_from_firmware(), which isn't worth the effort.


[1] 
https://elixir.bootlin.com/linux/v6.1-rc1/source/drivers/gpu/drm/tiny/simpledrm.c#L141
[2] 
https://elixir.bootlin.com/linux/v6.1-rc1/source/drivers/gpu/drm/tiny/simpledrm.c#L678


> +	if (ret) {
> +		struct resource *res;
> +
> +		res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +		if (!res)
> +			return -EINVAL;
> +
> +		init = simpledrm_device_init_mm_io;
> +		size = resource_size(res);
> +		start = res->start;
> +	} else {
> +		devm_add_action_or_reset(dev->dev, simpledrm_device_exit_mm, sdev);
> +		init = simpledrm_device_init_mm_sysmem;
> +		start = sdev->sysmem_start;
> +		size = sdev->sysmem_size;
> +	}
> +
> +	end = start + size - 1;
> +
> +	ret = devm_aperture_acquire_from_firmware(dev, start, size);
> +	if (ret) {
> +		drm_err(dev, "could not acquire memory range [%pa-%pa]: %d\n", &start, &end, ret);
> +		return ret;
> +	}
> +
> +	return init(sdev, start, size);
> +}
> +
>   /*
>    * Modesetting
>    */
> @@ -491,15 +594,15 @@ static void simpledrm_primary_plane_helper_atomic_update(struct drm_plane *plane
>   
>   	drm_atomic_helper_damage_iter_init(&iter, old_plane_state, plane_state);
>   	drm_atomic_for_each_plane_damage(&iter, &damage) {
> -		struct iosys_map dst = IOSYS_MAP_INIT_VADDR(sdev->screen_base);
>   		struct drm_rect dst_clip = plane_state->dst;
>   
>   		if (!drm_rect_intersect(&dst_clip, &damage))
>   			continue;
>   
> -		iosys_map_incr(&dst, drm_fb_clip_offset(sdev->pitch, sdev->format, &dst_clip));
> -		drm_fb_blit(&dst, &sdev->pitch, sdev->format->format, shadow_plane_state->data, fb,
> -			    &damage);
> +		iosys_map_incr(&sdev->screen_base, drm_fb_clip_offset(sdev->pitch, sdev->format,
> +								      &dst_clip));
> +		drm_fb_blit(&sdev->screen_base, &sdev->pitch, sdev->format->format,
> +			    shadow_plane_state->data, fb, &damage);
>   	}
>   
>   	drm_dev_exit(idx);
> @@ -518,7 +621,7 @@ static void simpledrm_primary_plane_helper_atomic_disable(struct drm_plane *plan
>   		return;
>   
>   	/* Clear screen to black if disabled */
> -	memset_io(sdev->screen_base, 0, sdev->pitch * sdev->mode.vdisplay);
> +	iosys_map_memset(&sdev->screen_base, 0, 0, sdev->pitch * sdev->mode.vdisplay);
>   
>   	drm_dev_exit(idx);
>   }
> @@ -635,8 +738,6 @@ static struct simpledrm_device *simpledrm_device_create(struct drm_driver *drv,
>   	struct drm_device *dev;
>   	int width, height, stride;
>   	const struct drm_format_info *format;
> -	struct resource *res, *mem;
> -	void __iomem *screen_base;
>   	struct drm_plane *primary_plane;
>   	struct drm_crtc *crtc;
>   	struct drm_encoder *encoder;
> @@ -706,35 +807,9 @@ static struct simpledrm_device *simpledrm_device_create(struct drm_driver *drv,
>   	drm_dbg(dev, "framebuffer format=%p4cc, size=%dx%d, stride=%d byte\n",
>   		&format->format, width, height, stride);
>   
> -	/*
> -	 * Memory management
> -	 */
> -
> -	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> -	if (!res)
> -		return ERR_PTR(-EINVAL);
> -
> -	ret = devm_aperture_acquire_from_firmware(dev, res->start, resource_size(res));
> -	if (ret) {
> -		drm_err(dev, "could not acquire memory range %pr: error %d\n", res, ret);
> +	ret = simpledrm_device_init_mm(sdev);
> +	if (ret)
>   		return ERR_PTR(ret);
> -	}
> -
> -	mem = devm_request_mem_region(&pdev->dev, res->start, resource_size(res), drv->name);
> -	if (!mem) {
> -		/*
> -		 * We cannot make this fatal. Sometimes this comes from magic
> -		 * spaces our resource handlers simply don't know about. Use
> -		 * the I/O-memory resource as-is and try to map that instead.
> -		 */
> -		drm_warn(dev, "could not acquire memory region %pr\n", res);
> -		mem = res;
> -	}
> -
> -	screen_base = devm_ioremap_wc(&pdev->dev, mem->start, resource_size(mem));
> -	if (!screen_base)
> -		return ERR_PTR(-ENOMEM);
> -	sdev->screen_base = screen_base;
>   
>   	/*
>   	 * Modesetting
> @@ -878,5 +953,35 @@ static struct platform_driver simpledrm_platform_driver = {
>   
>   module_platform_driver(simpledrm_platform_driver);
>   
> +static int simple_framebuffer_device_init(struct reserved_mem *rmem, struct device *dev)

'Simple-framebuffer' is the mechanism that provides the Linux platform 
device. And there's also a simplefb driver in fbdev.

The better name would be simpledrm_reserved_mem_device_init(). It makes 
it clear what this code belongs to.

> +{
> +	struct simpledrm_device *sdev = dev_get_drvdata(dev);
> +
> +	sdev->sysmem_start = rmem->base;
> +	sdev->sysmem_size = rmem->size;

As mentioned, assign rmem directly. Should we warn here if

   platform_get_resource(pdev, IORESOURCE_MEM, 0)

returns anything but NULL?

> +
> +	return 0;
> +}
> +
> +static void simple_framebuffer_device_release(struct reserved_mem *rmem, struct device *dev)

simpledrm_reserved_mem_device_release()

> +{
> +}

I think the caller should be updated to support callbacks of NULL here.

> +
> +static const struct reserved_mem_ops simple_framebuffer_ops = {

simpledrm_reserved_mem_ops

> +	.device_init = simple_framebuffer_device_init,
> +	.device_release = simple_framebuffer_device_release,
> +};
> +
> +static int simple_framebuffer_init(struct reserved_mem *rmem)

simpledrm_reserved_mem_init()

> +{
> +	pr_info("framebuffer memory at %pa, size %lu bytes\n", &rmem->base,

pr_debug() please.

> +		(unsigned long)rmem->size);
> +
> +	rmem->ops = &simple_framebuffer_ops;
> +
> +	return 0;
> +}
> +RESERVEDMEM_OF_DECLARE(simple_framebuffer, "framebuffer", simple_framebuffer_init);

  (simpledrm_reserved_mem, framebuffer, simpledrm_reserved_mem_init)

, I guess.

Best regards
Thomas

> +
>   MODULE_DESCRIPTION(DRIVER_DESC);
>   MODULE_LICENSE("GPL v2");

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Ivo Totev

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]

Re: [PATCH v2 4/7] drm/simpledrm: Add support for system memory framebuffers

[Thomas Zimmermann]

[-- Attachment #1.1: Type: text/plain, Size: 11389 bytes --]

Hi,

please see my review below.

Am 07.10.22 um 14:49 schrieb Thierry Reding:
> From: Thierry Reding <treding@nvidia.com>
> 
> Simple framebuffers can be set up in system memory, which cannot be
> requested and/or I/O remapped using the I/O resource helpers. Add a
> separate code path that obtains system memory framebuffers from the
> reserved memory region referenced in the memory-region property.
> 
> v2: make screen base a struct iosys_map to avoid sparse warnings
> 
> Signed-off-by: Thierry Reding <treding@nvidia.com>
> ---
>   drivers/gpu/drm/tiny/simpledrm.c | 177 ++++++++++++++++++++++++-------
>   1 file changed, 141 insertions(+), 36 deletions(-)
> 
> diff --git a/drivers/gpu/drm/tiny/simpledrm.c b/drivers/gpu/drm/tiny/simpledrm.c
> index 18489779fb8a..cf36f67d22e4 100644
> --- a/drivers/gpu/drm/tiny/simpledrm.c
> +++ b/drivers/gpu/drm/tiny/simpledrm.c
> @@ -2,6 +2,7 @@
>   
>   #include <linux/clk.h>
>   #include <linux/of_clk.h>
> +#include <linux/of_reserved_mem.h>
>   #include <linux/minmax.h>
>   #include <linux/platform_data/simplefb.h>
>   #include <linux/platform_device.h>
> @@ -207,7 +208,9 @@ struct simpledrm_device {
>   	unsigned int pitch;
>   
>   	/* memory management */
> -	void __iomem *screen_base;
> +	struct iosys_map screen_base;
> +	phys_addr_t sysmem_start;
> +	size_t sysmem_size;

Please store a pointer to the struct reserved_mem here. If set, we use 
it; if NULL, we use the memory region as before.

>   
>   	/* modesetting */
>   	uint32_t formats[8];
> @@ -441,6 +444,106 @@ static int simpledrm_device_init_regulators(struct simpledrm_device *sdev)
>   }
>   #endif
>   
> +/*
> + * Memory management
> + */
> +
> +static int simpledrm_device_init_mm_sysmem(struct simpledrm_device *sdev, phys_addr_t start,
> +					   size_t size)
> +{
> +	struct drm_device *dev = &sdev->dev;
> +	phys_addr_t end = start + size - 1;
> +	void *screen_base;
> +
> +	drm_info(dev, "using system memory framebuffer at [%pa-%pa]\n", &start, &end);
> +
> +	screen_base = devm_memremap(dev->dev, start, size, MEMREMAP_WC);
> +	if (!screen_base)
> +		return -ENOMEM;
> +
> +	iosys_map_set_vaddr(&sdev->screen_base, screen_base);
> +
> +	return 0;
> +}
> +
> +static int simpledrm_device_init_mm_io(struct simpledrm_device *sdev, phys_addr_t start,
> +				       size_t size)
> +{
> +	struct drm_device *dev = &sdev->dev;
> +	phys_addr_t end = start + size - 1;
> +	void __iomem *screen_base;
> +	struct resource *mem;
> +
> +	drm_info(dev, "using I/O memory framebuffer at [%pa-%pa]\n", &start, &end);
> +
> +	mem = devm_request_mem_region(dev->dev, start, size, sdev->dev.driver->name);
> +	if (!mem) {
> +		/*
> +		 * We cannot make this fatal. Sometimes this comes from magic
> +		 * spaces our resource handlers simply don't know about. Use
> +		 * the I/O-memory resource as-is and try to map that instead.
> +		 */
> +		drm_warn(dev, "could not acquire memory region [%pa-%pa]\n", &start, &end);
> +	} else {
> +		size = resource_size(mem);
> +		start = mem->start;
> +	}
> +
> +	screen_base = devm_ioremap_wc(dev->dev, start, size);
> +	if (!screen_base)
> +		return -ENOMEM;
> +
> +	iosys_map_set_vaddr_iomem(&sdev->screen_base, screen_base);
> +
> +	return 0;
> +}
> +
> +static void simpledrm_device_exit_mm(void *data)
> +{
> +	struct simpledrm_device *sdev = data;
> +	struct drm_device *dev = &sdev->dev;
> +
> +	of_reserved_mem_device_release(dev->dev);
> +}
> +
> +static int simpledrm_device_init_mm(struct simpledrm_device *sdev)
> +{

No such _init_mm() helper please. Simply do everything in the _create() 
function.


> +	int (*init)(struct simpledrm_device *sdev, phys_addr_t start, size_t size);
> +	struct drm_device *dev = &sdev->dev;
> +	struct platform_device *pdev = to_platform_device(dev->dev);
> +	phys_addr_t start, end;
> +	size_t size;
> +	int ret;
> +
> +	ret = of_reserved_mem_device_init_by_idx(dev->dev, dev->dev->of_node, 0);

Instead of doing our own lookup of the memory node, I'd prefer to keep 
this function. If we encasulate it, it's better than reinventing the 
wheel within this driver.

A number of helpers look-up the various properties from the device tree. 
They are at [1] and below. We should add a new helper that reads the 
memory node via of_reserved_mem_device_init_by_idx(). Here's some 
example code

struct reserved_mem *
simplefb_get_memory_of(drm_device dev, of_node node)
{
	sdev = simpledrm_device_of_dev(dev)

	ret = of_reserved_mem_device_init_by_idx()
	if (ret)
		return ERR_PTR()

	ret = devm_add_action_or_reset(dev->dev,
		simpledrm_device_exit_mm, sdev)
	if (ret)
		return ERR_PTR()

	return sdev->rmem;
}

That wraps the rmem instance quite nicely and is similar to the rest of 
the code. You can call this function within the else branch at [2].

Later in _create() where we map the I/O memory, branch depending on the 
returned value of rmem.

if (rmem) {

   devm_aperture_acquire_from_firmware()
   devm_memremap()

} else {

   platform_get_resource()
   devm_aperture_acquire_from_firmware()
   devm_request_mem_region()
   devm_ioremap_wc()

}

Don't bother with reducing the code size. The only call that is sharable 
is devm_aperture_acquire_from_firmware(), which isn't worth the effort.


[1] 
https://elixir.bootlin.com/linux/v6.1-rc1/source/drivers/gpu/drm/tiny/simpledrm.c#L141
[2] 
https://elixir.bootlin.com/linux/v6.1-rc1/source/drivers/gpu/drm/tiny/simpledrm.c#L678


> +	if (ret) {
> +		struct resource *res;
> +
> +		res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +		if (!res)
> +			return -EINVAL;
> +
> +		init = simpledrm_device_init_mm_io;
> +		size = resource_size(res);
> +		start = res->start;
> +	} else {
> +		devm_add_action_or_reset(dev->dev, simpledrm_device_exit_mm, sdev);
> +		init = simpledrm_device_init_mm_sysmem;
> +		start = sdev->sysmem_start;
> +		size = sdev->sysmem_size;
> +	}
> +
> +	end = start + size - 1;
> +
> +	ret = devm_aperture_acquire_from_firmware(dev, start, size);
> +	if (ret) {
> +		drm_err(dev, "could not acquire memory range [%pa-%pa]: %d\n", &start, &end, ret);
> +		return ret;
> +	}
> +
> +	return init(sdev, start, size);
> +}
> +
>   /*
>    * Modesetting
>    */
> @@ -491,15 +594,15 @@ static void simpledrm_primary_plane_helper_atomic_update(struct drm_plane *plane
>   
>   	drm_atomic_helper_damage_iter_init(&iter, old_plane_state, plane_state);
>   	drm_atomic_for_each_plane_damage(&iter, &damage) {
> -		struct iosys_map dst = IOSYS_MAP_INIT_VADDR(sdev->screen_base);
>   		struct drm_rect dst_clip = plane_state->dst;
>   
>   		if (!drm_rect_intersect(&dst_clip, &damage))
>   			continue;
>   
> -		iosys_map_incr(&dst, drm_fb_clip_offset(sdev->pitch, sdev->format, &dst_clip));
> -		drm_fb_blit(&dst, &sdev->pitch, sdev->format->format, shadow_plane_state->data, fb,
> -			    &damage);
> +		iosys_map_incr(&sdev->screen_base, drm_fb_clip_offset(sdev->pitch, sdev->format,
> +								      &dst_clip));
> +		drm_fb_blit(&sdev->screen_base, &sdev->pitch, sdev->format->format,
> +			    shadow_plane_state->data, fb, &damage);
>   	}
>   
>   	drm_dev_exit(idx);
> @@ -518,7 +621,7 @@ static void simpledrm_primary_plane_helper_atomic_disable(struct drm_plane *plan
>   		return;
>   
>   	/* Clear screen to black if disabled */
> -	memset_io(sdev->screen_base, 0, sdev->pitch * sdev->mode.vdisplay);
> +	iosys_map_memset(&sdev->screen_base, 0, 0, sdev->pitch * sdev->mode.vdisplay);
>   
>   	drm_dev_exit(idx);
>   }
> @@ -635,8 +738,6 @@ static struct simpledrm_device *simpledrm_device_create(struct drm_driver *drv,
>   	struct drm_device *dev;
>   	int width, height, stride;
>   	const struct drm_format_info *format;
> -	struct resource *res, *mem;
> -	void __iomem *screen_base;
>   	struct drm_plane *primary_plane;
>   	struct drm_crtc *crtc;
>   	struct drm_encoder *encoder;
> @@ -706,35 +807,9 @@ static struct simpledrm_device *simpledrm_device_create(struct drm_driver *drv,
>   	drm_dbg(dev, "framebuffer format=%p4cc, size=%dx%d, stride=%d byte\n",
>   		&format->format, width, height, stride);
>   
> -	/*
> -	 * Memory management
> -	 */
> -
> -	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> -	if (!res)
> -		return ERR_PTR(-EINVAL);
> -
> -	ret = devm_aperture_acquire_from_firmware(dev, res->start, resource_size(res));
> -	if (ret) {
> -		drm_err(dev, "could not acquire memory range %pr: error %d\n", res, ret);
> +	ret = simpledrm_device_init_mm(sdev);
> +	if (ret)
>   		return ERR_PTR(ret);
> -	}
> -
> -	mem = devm_request_mem_region(&pdev->dev, res->start, resource_size(res), drv->name);
> -	if (!mem) {
> -		/*
> -		 * We cannot make this fatal. Sometimes this comes from magic
> -		 * spaces our resource handlers simply don't know about. Use
> -		 * the I/O-memory resource as-is and try to map that instead.
> -		 */
> -		drm_warn(dev, "could not acquire memory region %pr\n", res);
> -		mem = res;
> -	}
> -
> -	screen_base = devm_ioremap_wc(&pdev->dev, mem->start, resource_size(mem));
> -	if (!screen_base)
> -		return ERR_PTR(-ENOMEM);
> -	sdev->screen_base = screen_base;
>   
>   	/*
>   	 * Modesetting
> @@ -878,5 +953,35 @@ static struct platform_driver simpledrm_platform_driver = {
>   
>   module_platform_driver(simpledrm_platform_driver);
>   
> +static int simple_framebuffer_device_init(struct reserved_mem *rmem, struct device *dev)

'Simple-framebuffer' is the mechanism that provides the Linux platform 
device. And there's also a simplefb driver in fbdev.

The better name would be simpledrm_reserved_mem_device_init(). It makes 
it clear what this code belongs to.

> +{
> +	struct simpledrm_device *sdev = dev_get_drvdata(dev);
> +
> +	sdev->sysmem_start = rmem->base;
> +	sdev->sysmem_size = rmem->size;

As mentioned, assign rmem directly. Should we warn here if

   platform_get_resource(pdev, IORESOURCE_MEM, 0)

returns anything but NULL?

> +
> +	return 0;
> +}
> +
> +static void simple_framebuffer_device_release(struct reserved_mem *rmem, struct device *dev)

simpledrm_reserved_mem_device_release()

> +{
> +}

I think the caller should be updated to support callbacks of NULL here.

> +
> +static const struct reserved_mem_ops simple_framebuffer_ops = {

simpledrm_reserved_mem_ops

> +	.device_init = simple_framebuffer_device_init,
> +	.device_release = simple_framebuffer_device_release,
> +};
> +
> +static int simple_framebuffer_init(struct reserved_mem *rmem)

simpledrm_reserved_mem_init()

> +{
> +	pr_info("framebuffer memory at %pa, size %lu bytes\n", &rmem->base,

pr_debug() please.

> +		(unsigned long)rmem->size);
> +
> +	rmem->ops = &simple_framebuffer_ops;
> +
> +	return 0;
> +}
> +RESERVEDMEM_OF_DECLARE(simple_framebuffer, "framebuffer", simple_framebuffer_init);

  (simpledrm_reserved_mem, framebuffer, simpledrm_reserved_mem_init)

, I guess.

Best regards
Thomas

> +
>   MODULE_DESCRIPTION(DRIVER_DESC);
>   MODULE_LICENSE("GPL v2");

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Ivo Totev

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]