[PATCH v2 4/7] drm/simpledrm: Add support for system memory framebuffers
Thomas Zimmermann
tzimmermann at suse.de
Tue Oct 18 11:58:53 UTC 2022
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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: OpenPGP_signature
Type: application/pgp-signature
Size: 840 bytes
Desc: OpenPGP digital signature
URL: <https://lists.freedesktop.org/archives/dri-devel/attachments/20221018/d2709010/attachment.sig>
More information about the dri-devel
mailing list