-
Notifications
You must be signed in to change notification settings - Fork 105
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feature(lvgl_port): Initial support for ppa rendering in lvgl (BSP-563) #409
base: master
Are you sure you want to change the base?
feature(lvgl_port): Initial support for ppa rendering in lvgl (BSP-563) #409
Conversation
ThunderDai
commented
Oct 15, 2024
0984d25
to
9723801
Compare
@peter-marcisovsky @espzav hi, PTAL, this is my first commit for esp-bsp. Therefore, some code related rules are not familiar enough |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you consider adding some tests to your feature? If you take a look at my #357 , there are functionality and benchmark unit tests. Would it be possible to do the same for this feature?
Also, can you take a look at the failing CI?
@@ -55,6 +55,21 @@ typedef struct { | |||
|
|||
extern int lv_color_blend_to_argb8888_esp(asm_dsc_t *asm_dsc); | |||
|
|||
// Just for compatibility with v9.2.0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would not recommend this approach of solving compatibility issues with LVGL 9.2.
I tried to build lvlgl_port test app on your branch, with the HW accelerating enabled and the test app would not build, neither with LVGL9.1, nor with LVGL9.2.
peter@peter ➜ ~/esp/esp-bsp/components/esp_lvgl_port/test_apps/lvgl_port git:(pr/409) ✗ idf.py -DSDKCONFIG_DEFAULTS='sdkconfig.defaults;sdkconfig.ci.asm_render' set-target esp32s3 build
Is your feature working with LVGL9.1? If yes, could you consider using just LVGL9.1 in this MR?
We have agreed to make the HW acceleration support working with LVGL9.1, for now here, until we figure out what to do with this brekaing changes which was introduced in LVGL9.2.
yes, can add some test. I have already included the newly submitted lvgl_port component as part of my project and have run the benchmark without any issues. I will also try running the existing test_apps. It is worth noting that because PPA has implemented some of your current functions, it conflicts with the current header file. I have added a new header file myself. The following configuration is required: CONFIG_LV_DRAW_SW_ASM_CUSTOM=y |
24e762a
to
0b1609e
Compare
865eada
to
461a0f8
Compare
Mainly supports the following features: - color blend (simple fill) - color blend with opa - blend normal (blend with argb8888) - blend normal to color (color convert / memcpy)
461a0f8
to
47b925e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ThunderDai I'm sorry for the late start of review. I left few comments for start
espressif/esp32_p4_function_ev_board: | ||
version: "*" | ||
override_path: "../../../../../bsp/esp32_p4_function_ev_board" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For esp_lvgl_port, we try to avoid dependencies on any BSP. Can we removed it?
If not, we can place the example in different location,(not directly into esp_lvgl_port component)
@@ -286,7 +296,11 @@ static lv_display_t *lvgl_port_add_disp_priv(const lvgl_port_display_cfg_t *disp | |||
buff_caps |= MALLOC_CAP_DEFAULT; | |||
} | |||
|
|||
/* Use RGB internal buffers for avoid tearing effect */ | |||
#if CONFIG_IDF_TARGET_ESP32P4 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#if CONFIG_IDF_TARGET_ESP32P4 | |
#if SOC_CACHE_INTERNAL_MEM_VIA_L1CACHE |
Would this be better for forward compatibility?
if (buf1) { | ||
free(buf1); | ||
if (disp_ctx->draw_buffs[0]) { | ||
free(disp_ctx->draw_buffs[0]); | ||
} | ||
if (buf2) { | ||
free(buf2); | ||
if (disp_ctx->draw_buffs[1]) { | ||
free(disp_ctx->draw_buffs[1]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for this fix! It was already fixed in the master branch, so you might have to resolve some conflicts...
@@ -77,12 +77,12 @@ if("usb_host_hid" IN_LIST build_components) | |||
endif() | |||
|
|||
# Include SIMD assembly source code for rendering, only for (9.1.0 <= LVG_version < 9.2.0) and only for esp32 and esp32s3 | |||
if((lvgl_ver VERSION_GREATER_EQUAL "9.1.0") AND (lvgl_ver VERSION_LESS "9.2.0")) | |||
if(CONFIG_IDF_TARGET_ESP32 OR CONFIG_IDF_TARGET_ESP32S3) | |||
if((lvgl_ver VERSION_GREATER_EQUAL "9.1.0") AND (lvgl_ver VERSION_LESS_EQUAL "9.2.0")) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does it really work with 9.2.0? we had some compatibility problems with this version. If yes, why can't support 9.2.2 too?
#define LVGL_VERSION_MAJOR 9 | ||
#define LVGL_VERSION_MINOR 1 | ||
#define LVGL_VERSION_PATCH 0 | ||
#define LVGL_VERSION_INFO "" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why can't we use lv_version.h
from the lvgl component?
dma2d_descriptor_t *tx_link_buffer = (dma2d_descriptor_t *)heap_caps_aligned_calloc(64, 1, 64, MALLOC_CAP_SPIRAM); | ||
dma2d_descriptor_t *rx_link_buffer = (dma2d_descriptor_t *)heap_caps_aligned_calloc(64, 1, 64, MALLOC_CAP_SPIRAM); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The cache line size is configurable. Can be 64 or 128. Can we take the value from sdkconfig.h?
esp_cache_msync((void *)tx_dsc, 64, ESP_CACHE_MSYNC_FLAG_DIR_C2M | ESP_CACHE_MSYNC_FLAG_UNALIGNED); | ||
esp_cache_msync((void *)rx_dsc, 64, ESP_CACHE_MSYNC_FLAG_DIR_C2M | ESP_CACHE_MSYNC_FLAG_UNALIGNED); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here, you assume cache line size == 64. But it can be 128 too.
Is the unaligned sync sage here? If yes, please provide better comment