Improve nxp display drivers

[ngphibang]

This PR adds support for the ARGB8888 pixel format to the NXP PxP and eLCDIF drivers. It is needed because the camera pipeline on i.MX RT11xx outputs images in this format.
The PR also adds implementation for set_format API to eLCDIF driver so that application can change to a format other than the predefined one in the devicetree.

[danieldegrasse]

Thanks for the contribution- please see my comments about dynamic allocation of the framebuffer

[danieldegrasse]

I am very concerned about allocating these framebuffers dynamically. This will require a very large heap for this driver to work- Instead, could we reuse the statically allocated framebuffer from init, and only support changing pixel format to formats that require a smaller framebuffer than the default format?

Currently, this PR is breaking the display driver sample for me on the RT1060 EVK- this is because the call to k_aligned_alloc is returning NULL (presumably because we are out of memory on the heap)

[ngphibang]

Yes, we can use static buffers with a predefined size, e.g. for 4-bytes format. But we will waste half of them if app changes to a 2-bytes format. The problem here with dynamic buffer is the driver and the app use the same system heap and the app often limits the heap size with CONFIG_HEAP_MEM_POOL_SIZE. We can extend this size but you are right, apps are not aware of this and it's not their responsibility to care about what the driver needs.

So, I think the best way is either

• defining a seperate heap for the driver, or
• defining a Kconfig option with the prefix HEAP_MEM_POOL_ADD_SIZE_

I prefer option 1 (unless you have another idea ?) as we already had a similar example : the video_buffer_pool heap. The option 2 is rather for board / SoC level.

[ngphibang]

By defining a separate heap, with K_HEAP_DEFINE(name, bytes), we still need to reserve a fixed size for the driver heap and still waste of memory with formats requiring smaller buffers. So, I used option 2.
HEAP_MEM_POOL_ADD_SIZE_ (introduced recently by this patch "Johan Hedberg - kernel: Introduce a way to specify minimum system heap size") allows each subsystem (board, driver, library, etc) to specify their own system heap size requirement.

[danieldegrasse]

I think a driver level heap is an ok solution here- however I would note that we will still waste memory if the user switches to a smaller format size. I would recommend we define the driver heap size based on the size we would need for the default format set in devicetree- this way, there are no Kconfig changes needed at the user level.

[ngphibang]

Hi @danieldegrasse ,
Perhaps I missunderstood, when you say "I think a driver level heap is an ok solution here" do you mean the current shared system heap solution that I just pushed or a separate driver heap solution that I mentioned in my first comment but not opted ?

In the current implementation, the driver and the app share the same system heap, each specifies the heap size it needs and we don't waste memory :-). For example, on a 8MB-memory system, driver say by default "I need 6MB" and app says "I needs 2MB" via CONFIG_HEAP_MEM_POOL_ADD_SIZE_XXX , then if user sets a smaller format, the driver actually uses only 3MB, the app can use all the rest if it wants by reducing the CONFIG_HEAP_MEM_POOL_ADD_SIZE_ of the driver.

But if you think this is so complicated for the app to change these two configs, it's no problem for me to implement the "driver heap solution" :-).

Otherwise, I don't really understand about the point "define the driver heap size based on the size we would need for the default format set in devicetree". Currently, the default format in devicetree is BGR_565 (2 bytes), my camera pipeline on RT11xx outputs XRGB8888 (4 bytes), so I need to change the default display format in all devicetrees to ARGB8888 ? Would it be better removing this default format in devicetree and set it implicitely in the driver init code, this way user will not have intention to change it ?

[danieldegrasse]

Perhaps I missunderstood, when you say "I think a driver level heap is an ok solution here" do you mean the current shared system heap solution that I just pushed or a separate driver heap solution that I mentioned in my first comment but not opted

I meant a separate driver heap in this case, defined with K_HEAP_DEFINE.

But if you think this is so complicated for the app to change these two configs, it's no problem for me to implement the "driver heap solution" :-).

I want to avoid changes that will cause a lot of downstream churn for users. It seems like this solution will only require changes for users that are utilizing a heap in their application, and the warning would be pretty clear, right?: https://github.com/zephyrproject-rtos/zephyr/blob/main/kernel/CMakeLists.txt#L137.

That being said, a few other comments:

• I don't think we need CONFIG_HEAP_MEM_POOL_ADD_SIZE_APP. The app can just set CONFIG_HEAP_MEM_POOL_SIZE to the (larger) size they need, right?
• We should have some logic to (at a minimum) not set CONFIG_HEAP_MEM_POOL_ADD_SIZE_ELCDIF if CONFIG_MCUX_ELCDIF_FB_NUM is 0. Otherwise we will be allocating heap memory we do not need.

[ngphibang]

I want to avoid changes that will cause a lot of downstream churn for users.

The only change I see is applications (only who use eLCDIF driver) will use CONFIG_HEAP_MEM_POOL_ADD_APP
to specify the system heap size it needs for itself instead of using CONFIG_HEAP_MEM_POOL_SIZE to decide the whole system heap size that it does not has the entire knowdlege. Applications using system heap memory but not using eLCDIF driver are not impacted.

And even if we opt the driver heap solution, applications still need to deal with a new config CONFIG_ELCDIF_BUFFER_SIZE_MAX (explained at the end of this comment).

It seems like this solution will only require changes for users that are utilizing a heap in their application, and the warning would be pretty clear, right?: https://github.com/zephyrproject-rtos/zephyr/blob/main/kernel/CMakeLists.txt#L137.

The warning you pointed out just supports this solution, not against it :-). We will have a warning at compile time if the app tries to set the whole system heap size (with CONFIG_HEAP_MEM_POOL_SIZE) that’s less than the minimum value required by the whole system which is the sum of all CONFIG_HEAP_MEM_POOL_ADD_XXX. And the value set by the app will be ignored to favorize this minimum value : https://docs.zephyrproject.org/latest/kernel/memory_management/heap.html

So, the question is:

• Why does the app want to set the whole system heap size why it does not have any knowledge about how much system heap memory other subsystems / libraries / drivers need ?

And what Zephyr does now : "the value set by the app with CONFIG_HEAP_MEM_POOL_SIZE will be ignored to favorize this minimum value set by all CONFIG_HEAP_MEM_POOL_ADD_XXX " is not enough as the app itself also needs to use heap memory, so developers must always look at the minimum value required by other subsystems (pointed out in the warning), add the amount the apps needs then reset CONFIG_HEAP_MEM_POOL_SIZE with the new value.

So, I think applications should stop deciding the whole system heap size with CONFIG_HEAP_MEM_POOL_SIZE, instead just specifies how much it needs using a new config like CONFIG_HEAP_MEM_POOL_ADD_APP as I did but this should be defined uniquely and globally by Zephyr (in the future ?), not by a specific driver. This is also partly pointed out in the commit message of "Johan Hedberg - kernel: Introduce a way to specify minimum system heap size" that

The vast majority of values set by current sample or test applications is much too small ...

That being said, a few other comments:

• I don't think we need CONFIG_HEAP_MEM_POOL_ADD_SIZE_APP. The app can just set CONFIG_HEAP_MEM_POOL_SIZE to the (larger) size they need, right?
• We should have some logic to (at a minimum) not set CONFIG_HEAP_MEM_POOL_ADD_SIZE_ELCDIF if CONFIG_MCUX_ELCDIF_FB_NUM is 0. Otherwise we will be allocating heap memory we do not need.

It seems you mixed up the two solutions (?). If we choose the driver separate heap solution, the size of this heap is defined by K_HEAP_DEFINE and is not impacted by all the CONFIG_HEAP_MEM_POOL_ADD_XXX. These configs only have impact on the system heap. And we will do exactly the same as the video buffer pool heap:
https://github.com/zephyrproject-rtos/zephyr/blob/main/drivers/video/video_common.c

K_HEAP_DEFINE(elcdif_heap, CONFIG_MCUX_ELCDIF_FB_NUM * CONFIG_ELCDIF_BUFFER_SIZE_MAX);

Applications and samples using eLCDIF drivers will need to adjust CONFIG_ELCDIF_BUFFER_SIZE_MAX to optimize memory usage, or in case the chosen format exceeds the preset default value, exactly the same as in the video examples, though most of the cases, apps will forget to touch this value when it just "works" and we will waste memory. You can see even the video capture sample does not adjust this value:
https://github.com/zephyrproject-rtos/zephyr/blob/main/samples/subsys/video/capture/prj.conf

But again as being said, the app does not know in details how much this value should be as the buffers are allocated by the elcdif driver, not by the app. It is not the case of the video buffers, as in the video subsystem, the app allocates the video buffers on the video driver heap using the video_buffer_alloc API so it knows the buffer size
That's why I think the driver heap solution is approriate for the video subsystem, but not for our case. I hope it is clearer now, but as being said, it's ok for me to switch to the driver heap solution (The change is minor and I already had it).

[ngphibang]

Hi @danieldegrasse ,
I updated the PR with the driver heap solution as your suggestion.

[danieldegrasse]

So, I think applications should stop deciding the whole system heap size with CONFIG_HEAP_MEM_POOL_SIZE, instead just specifies how much it needs using a new config like CONFIG_HEAP_MEM_POOL_ADD_APP as I did but this should be defined uniquely and globally by Zephyr (in the future ?), not by a specific driver.

I agree, this makes sense. I think if we wanted to take this path though, it would be a Kconfig we should introduce globally as a separate PR, and then leverage it in this PR. If we are going to make this transition (which I agree, we should) it shouldn't be something we initially implement as part of a driver, we should make the transition at the global level.

But again as being said, the app does not know in details how much this value should be as the buffers are allocated by the elcdif driver, not by the app. It is not the case of the video buffers, as in the video subsystem, the app allocates the video buffers on the video driver heap using the video_buffer_alloc API so it knows the buffer size
That's why I think the driver heap solution is approriate for the video subsystem, but not for our case. I hope it is clearer now, but as being said, it's ok for me to switch to the driver heap solution (The change is minor and I already had it).

I think I see the point you are raising here. Essentially, if we use the system heap, the memory that would otherwise be wasted can be used by the application to allocate memory? I guess I can see use cases where the developer would change the pixel format at runtime to reduce memory consumption, and want to use that memory for something else. Either way, it is harmless to make this possible :)

So maybe we can use the Kconfig CONFIG_HEAP_MEM_POOL_ADD_SIZE_ELCDIF, and set this to zero if CONFIG_MCUX_ELCDIF_FB_NUM==0? Then in the display samples we will need to raise the value of CONFIG_HEAP_MEM_POOL_SIZE. For downstream users, they will get a warning directing them to change CONFIG_HEAP_MEM_POOL_SIZE once they use this code, so this change should be pretty easy for them to handle.

Does this approach work for you? Sorry if I am missing something here

[ngphibang]

Thank @danieldegrasse for your feedbacks ! My responses are below.

[ngphibang]

Yes, we can use static buffers with a predefined size, e.g. for 4-bytes format. But we will waste half of them if app changes to a 2-bytes format. The problem here with dynamic buffer is the driver and the app use the same system heap and the app often limits the heap size with CONFIG_HEAP_MEM_POOL_SIZE. We can extend this size but you are right, apps are not aware of this and it's not their responsibility to care about what the driver needs.

So, I think the best way is either

• defining a seperate heap for the driver, or
• defining a Kconfig option with the prefix HEAP_MEM_POOL_ADD_SIZE_

I prefer option 1 (unless you have another idea ?) as we already had a similar example : the video_buffer_pool heap. The option 2 is rather for board / SoC level.

[danieldegrasse]

Current implementation is fine with me, but I am also happy to switch to CONFIG_HEAP_MEM_POOL_ADD_SIZE_ELCDIF provided we do not add a Kconfig like CONFIG_HEAP_MEM_POOL_ADD_SIZE_APP in the scope of this PR (as mentioned in my comment, I'm ok with adding this Kconfig in another PR if we do so more generically)