| Message ID | 20221007124946.406808-5-thierry.reding@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | drm/simpledrm: Support system memory framebuffers | expand |
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
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
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
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");