drivers: display: stm32_ltdc: Add SMH attribute for LTDC buffer.

[iabdalkader]

Add a config option for the LTDC FB SMH attribute, and fix/update the board config.

Fixes #89233.

[JarmouniA]

Why not create a generic symbol in drivers/display/Kconfig instead?

[iabdalkader]

Because no other display drivers use SMH as far as I know.

[avolmat-st]

Maybe I missed something but in order to be able to use the SHARED_MULTI_HEAP, I would expect this to be first initialized via the function shared_multi_heap_pool_init and then an area added via the call to the function shared_multi_heap_add. Only then shared_multi_heap_aligned_alloc call would be done.
However I cannot find any such initialize / add call done in the stm32 context. Did I missed something ?
Of course such call could be done outside of the LTDC driver, however still I cannot find them.

[avolmat-st]

Discussion ongoing on #89233 seems to highlight that actually this doesn't work with SHM enabled, is that right ?

[avolmat-st]

Considering that finally the SHM usage is disabled, if it is not functionnal (to be confirmed), maybe the addition of the new option to mention the attribute is not really needed, at least for the time being.

[JarmouniA]

in order to be able to use the SHARED_MULTI_HEAP, I would expect this to be first initialized via the function shared_multi_heap_pool_init and then an area added via the call to the function shared_multi_heap_add. Only then shared_multi_heap_aligned_alloc call would be done.

Indeed.

[iabdalkader]

Discussion ongoing on #89233 seems to highlight that actually this doesn't work with SHM enabled, is that right ?
Considering that finally the SHM usage is disabled, if it is not functionnal (to be confirmed), maybe the addition of the new option to mention the attribute is not really needed, at least for the time being.

No, the LTDC driver definitely works with SMH, and we already use it in the Arduino core to share SDRAM with the video subsystem. The attribute is needed to control the display’s SMH allocations and to decouple it from the video’s SMH attribute (in fact, I believe using the video’s SMH attribute here was a copy&paste mistake).

The reason SMH isn’t enabled for the display by default is that it really depends on the application. If the user app uses video with SMH (to support more, bigger, buffers) , it definitely needs to enable SMH for both display too, otherwise, LTDC will just grab a chunk of SDRAM that could overlap with the heap.

As for SMH initialization, I believe it can be handled in the user app. Alternatively, we could add the initialization code to boards/arduino/giga_r1/board.c. In that case, the heap size (and location) would be fixed, but the user would only need to enable SMH and define the attributes. Either way works for me, I'm just trying to fix #89233.

[avolmat-st]

Thanks for the clarification.
I agree that SMH is a must whenever using the DCMI as well in order to avoid having the LTDC framebuffer overlapping with the video data.

So are you saying that currently even without initialization (aka calls to the shared_multi_heap_pool_init and shared_multi_heap_add) done prior to start actually using SMH (either from LTDC or DCMI), this is working ?

Indeed, performing initialization within the board code sounds like a good place. Knowing where we intend to make memory accessible via SMH is really board specific.
That's why, considering that currently I do not see any such initialization calls, I am wondering why it is working. (I might have missed something with the SMH code probably).

As a side note, while currently LTDC / DCMI (more generally video FW) are based on SMH usage, I am actually considering addition of the memory attribute heap API usage, I tend to find it more flexible than SMH since this is based on memory-attribute done in the DT. But that's another topic for which I will post a PR.

[iabdalkader]

So are you saying that currently even without initialization (aka calls to the shared_multi_heap_pool_init and shared_multi_heap_add) done prior to start actually using SMH (either from LTDC or DCMI), this is working ?

No, currently, if you enable SMH you need to initialize it in your app. For example, we do so in the Arduino core: if you build an Arduino sketch for Zephyr, that uses display and/or video, the SMH gets initialized.

Indeed, performing initialization within the board code sounds like a good place.

I can add this code to board.c in a follow up PR, as it doesn't really change anything in this PR. I think SMH for display should remain disabled by default, as for most standalone samples (such as lvgl etc..) it's not really needed.

[avolmat-st]

No, currently, if you enable SMH you need to initialize it in your app. For example, we do so in the Arduino core: if you build an Arduino sketch for Zephyr, that uses display and/or video, the SMH gets initialized.

Ok that make sense then.

I can add this code to board.c in a follow up PR, as it doesn't really change anything in this PR. I think SMH for display should remain disabled by default, as for most standalone samples (such as lvgl etc..) it's not really needed.

Ok thank you, and yes it should be kept disabled by default considering that most of boards do not initialize the SMH as it wouldn't work anymore.

[moosery]

The shield name seems to be wrong. The shield looks to be named "giga_display_shield" and not "arduino_giga_display_shield"?

[erwango]

Thanks for the comment, but this is unrelated to the content of this PR. Feel free to raise a dedicated issue so this can be discussed and not "defocus" discussions on current PR.

[moosery]

I am sorry. I do not mean to defocus anything. The contributor said it fixes #89233 as well in one comment above. So I incorrectly assumed he was trying to fix that as well which is why I added the comment. Thank you for your correction. Maybe the contributor can update his comment to say it addresses portions of that issue?

[iabdalkader]

I fixed the board's name, but in the case of the shield, I believe the documentation is correct: the shield should be called arduino_giga_display_shield, not only to match the board's naming convention, but also because other vendors use similar patterns. I can rename the file in this PR or in a separate one, either way works for me.

[iabdalkader]

@moosery @erwango I renamed the overlay to match the shield name, and the docs, so now the following command works:

west build -b arduino_giga_r1//m7 -p --shield arduino_giga_display_shield samples/subsys/display/lvgl

[dleach02]

@erwango my original bug report also highlighted the problem with the shield name in the example command line as @moosery is indicating.

I just checked and @iabdalkader addressed this by changing the name of the shield overlay file so now the reference command line does build correctly.

[dleach02]

sample command line now builds correctly, which was the original issue I submitted. I'll let others with actual hardware confirm that it works.