* [PATCH v2 4/7] drm/simpledrm: Add support for system memory framebuffers
@ 2022-10-07 12:49 ` Thierry Reding
0 siblings, 0 replies; 42+ messages in thread
From: Thierry Reding @ 2022-10-07 12:49 UTC (permalink / raw)
To: David Airlie, Daniel Vetter, Rob Herring, Krzysztof Kozlowski,
Thierry Reding
Cc: devicetree, dri-devel, Jon Hunter, Thomas Zimmermann, linux-tegra,
Robin Murphy
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
^ permalink raw reply related [flat|nested] 42+ messages in thread* Re: [PATCH v2 4/7] drm/simpledrm: Add support for system memory framebuffers
2022-10-07 12:49 ` Thierry Reding
@ 2022-10-10 8:12 ` Thomas Zimmermann
-1 siblings, 0 replies; 42+ messages in thread
From: Thomas Zimmermann @ 2022-10-10 8:12 UTC (permalink / raw)
To: Thierry Reding, David Airlie, Daniel Vetter, Rob Herring,
Krzysztof Kozlowski
Cc: Jon Hunter, Robin Murphy, dri-devel, linux-tegra, devicetree
[-- 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 --]
^ permalink raw reply [flat|nested] 42+ messages in thread* Re: [PATCH v2 4/7] drm/simpledrm: Add support for system memory framebuffers
@ 2022-10-10 8:12 ` Thomas Zimmermann
0 siblings, 0 replies; 42+ messages in thread
From: Thomas Zimmermann @ 2022-10-10 8:12 UTC (permalink / raw)
To: Thierry Reding, David Airlie, Daniel Vetter, Rob Herring,
Krzysztof Kozlowski
Cc: linux-tegra, devicetree, Robin Murphy, dri-devel, Jon Hunter
[-- 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 --]
^ permalink raw reply [flat|nested] 42+ messages in thread* Re: [PATCH v2 4/7] drm/simpledrm: Add support for system memory framebuffers
2022-10-10 8:12 ` Thomas Zimmermann
@ 2022-10-17 14:54 ` Thierry Reding
-1 siblings, 0 replies; 42+ messages in thread
From: Thierry Reding @ 2022-10-17 14:54 UTC (permalink / raw)
To: Thomas Zimmermann
Cc: David Airlie, Daniel Vetter, Rob Herring, Krzysztof Kozlowski,
Jon Hunter, Robin Murphy, dri-devel, linux-tegra, devicetree
[-- 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 --]
^ permalink raw reply [flat|nested] 42+ messages in thread* Re: [PATCH v2 4/7] drm/simpledrm: Add support for system memory framebuffers
@ 2022-10-17 14:54 ` Thierry Reding
0 siblings, 0 replies; 42+ messages in thread
From: Thierry Reding @ 2022-10-17 14:54 UTC (permalink / raw)
To: Thomas Zimmermann
Cc: devicetree, David Airlie, dri-devel, Jon Hunter, Rob Herring,
Krzysztof Kozlowski, linux-tegra, Robin Murphy
[-- 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 --]
^ permalink raw reply [flat|nested] 42+ messages in thread* Re: [PATCH v2 4/7] drm/simpledrm: Add support for system memory framebuffers
2022-10-17 14:54 ` Thierry Reding
@ 2022-10-17 18:15 ` Rob Herring
-1 siblings, 0 replies; 42+ messages in thread
From: Rob Herring @ 2022-10-17 18:15 UTC (permalink / raw)
To: Thierry Reding
Cc: Thomas Zimmermann, David Airlie, Daniel Vetter,
Krzysztof Kozlowski, Jon Hunter, Robin Murphy, dri-devel,
linux-tegra, devicetree
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
^ permalink raw reply [flat|nested] 42+ messages in thread* Re: [PATCH v2 4/7] drm/simpledrm: Add support for system memory framebuffers
@ 2022-10-17 18:15 ` Rob Herring
0 siblings, 0 replies; 42+ messages in thread
From: Rob Herring @ 2022-10-17 18:15 UTC (permalink / raw)
To: Thierry Reding
Cc: devicetree, David Airlie, Thomas Zimmermann, dri-devel,
Jon Hunter, Krzysztof Kozlowski, linux-tegra, Robin Murphy
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
^ permalink raw reply [flat|nested] 42+ messages in thread* Re: [PATCH v2 4/7] drm/simpledrm: Add support for system memory framebuffers
2022-10-17 18:15 ` Rob Herring
@ 2022-10-18 10:46 ` Thierry Reding
-1 siblings, 0 replies; 42+ messages in thread
From: Thierry Reding @ 2022-10-18 10:46 UTC (permalink / raw)
To: Rob Herring
Cc: Thomas Zimmermann, David Airlie, Daniel Vetter,
Krzysztof Kozlowski, Jon Hunter, Robin Murphy, dri-devel,
linux-tegra, devicetree
[-- 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 --]
^ permalink raw reply [flat|nested] 42+ messages in thread* Re: [PATCH v2 4/7] drm/simpledrm: Add support for system memory framebuffers
@ 2022-10-18 10:46 ` Thierry Reding
0 siblings, 0 replies; 42+ messages in thread
From: Thierry Reding @ 2022-10-18 10:46 UTC (permalink / raw)
To: Rob Herring
Cc: devicetree, David Airlie, Thomas Zimmermann, dri-devel,
Jon Hunter, Krzysztof Kozlowski, linux-tegra, Robin Murphy
[-- 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 --]
^ permalink raw reply [flat|nested] 42+ messages in thread* Re: [PATCH v2 4/7] drm/simpledrm: Add support for system memory framebuffers
2022-10-18 10:46 ` Thierry Reding
@ 2022-10-18 15:32 ` Rob Herring
-1 siblings, 0 replies; 42+ messages in thread
From: Rob Herring @ 2022-10-18 15:32 UTC (permalink / raw)
To: Thierry Reding
Cc: Thomas Zimmermann, David Airlie, Daniel Vetter,
Krzysztof Kozlowski, Jon Hunter, Robin Murphy, dri-devel,
linux-tegra, devicetree
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
^ permalink raw reply [flat|nested] 42+ messages in thread* Re: [PATCH v2 4/7] drm/simpledrm: Add support for system memory framebuffers
@ 2022-10-18 15:32 ` Rob Herring
0 siblings, 0 replies; 42+ messages in thread
From: Rob Herring @ 2022-10-18 15:32 UTC (permalink / raw)
To: Thierry Reding
Cc: devicetree, David Airlie, Thomas Zimmermann, dri-devel,
Jon Hunter, Krzysztof Kozlowski, linux-tegra, Robin Murphy
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
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH v2 4/7] drm/simpledrm: Add support for system memory framebuffers
2022-10-17 14:54 ` Thierry Reding
@ 2022-10-18 11:58 ` Thomas Zimmermann
-1 siblings, 0 replies; 42+ messages in thread
From: Thomas Zimmermann @ 2022-10-18 11:58 UTC (permalink / raw)
To: Thierry Reding
Cc: David Airlie, Daniel Vetter, Rob Herring, Krzysztof Kozlowski,
Jon Hunter, Robin Murphy, dri-devel, linux-tegra, devicetree
[-- 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 --]
^ permalink raw reply [flat|nested] 42+ messages in thread* Re: [PATCH v2 4/7] drm/simpledrm: Add support for system memory framebuffers
@ 2022-10-18 11:58 ` Thomas Zimmermann
0 siblings, 0 replies; 42+ messages in thread
From: Thomas Zimmermann @ 2022-10-18 11:58 UTC (permalink / raw)
To: Thierry Reding
Cc: devicetree, David Airlie, dri-devel, Jon Hunter, Rob Herring,
Krzysztof Kozlowski, linux-tegra, Robin Murphy
[-- 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 --]
^ permalink raw reply [flat|nested] 42+ messages in thread* Re: [PATCH v2 4/7] drm/simpledrm: Add support for system memory framebuffers
2022-10-18 11:58 ` Thomas Zimmermann
@ 2022-10-18 15:13 ` Thierry Reding
-1 siblings, 0 replies; 42+ messages in thread
From: Thierry Reding @ 2022-10-18 15:13 UTC (permalink / raw)
To: Thomas Zimmermann
Cc: David Airlie, Daniel Vetter, Rob Herring, Krzysztof Kozlowski,
Jon Hunter, Robin Murphy, dri-devel, linux-tegra, devicetree
[-- 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 --]
^ permalink raw reply [flat|nested] 42+ messages in thread* Re: [PATCH v2 4/7] drm/simpledrm: Add support for system memory framebuffers
@ 2022-10-18 15:13 ` Thierry Reding
0 siblings, 0 replies; 42+ messages in thread
From: Thierry Reding @ 2022-10-18 15:13 UTC (permalink / raw)
To: Thomas Zimmermann
Cc: devicetree, David Airlie, dri-devel, Jon Hunter, Rob Herring,
Krzysztof Kozlowski, linux-tegra, Robin Murphy
[-- 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 --]
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH v2 4/7] drm/simpledrm: Add support for system memory framebuffers
2022-10-07 12:49 ` Thierry Reding
@ 2022-10-19 12:25 ` Thomas Zimmermann
-1 siblings, 0 replies; 42+ messages in thread
From: Thomas Zimmermann @ 2022-10-19 12:25 UTC (permalink / raw)
To: Thierry Reding, David Airlie, Daniel Vetter, Rob Herring,
Krzysztof Kozlowski
Cc: devicetree, dri-devel, Jon Hunter, linux-tegra, Robin Murphy
[-- 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 --]
^ permalink raw reply [flat|nested] 42+ messages in thread* Re: [PATCH v2 4/7] drm/simpledrm: Add support for system memory framebuffers
@ 2022-10-19 12:25 ` Thomas Zimmermann
0 siblings, 0 replies; 42+ messages in thread
From: Thomas Zimmermann @ 2022-10-19 12:25 UTC (permalink / raw)
To: Thierry Reding, David Airlie, Daniel Vetter, Rob Herring,
Krzysztof Kozlowski
Cc: linux-tegra, devicetree, Robin Murphy, dri-devel, Jon Hunter
[-- 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 --]
^ permalink raw reply [flat|nested] 42+ messages in thread