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@xxxxxxxxxx>
> >
> > 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@xxxxxxxxxx>
> > ---
> > 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:
signature.asc
Description: PGP signature
