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:
OpenPGP_signature
Description: OpenPGP digital signature
