From d8b02f5d76133c5465c98af0315a9a415b436705 Mon Sep 17 00:00:00 2001 From: Song Ruo Jing Date: Tue, 9 Jul 2024 16:06:42 +0800 Subject: [PATCH 1/2] fix(ppa): fix insufficient writeback/invalidate data length --- components/esp_driver_ppa/src/ppa_blend.c | 3 ++- components/esp_driver_ppa/src/ppa_fill.c | 3 ++- components/esp_driver_ppa/src/ppa_srm.c | 3 ++- 3 files changed, 6 insertions(+), 3 deletions(-) diff --git a/components/esp_driver_ppa/src/ppa_blend.c b/components/esp_driver_ppa/src/ppa_blend.c index 08c3e93e8c..b5ebaa7f69 100644 --- a/components/esp_driver_ppa/src/ppa_blend.c +++ b/components/esp_driver_ppa/src/ppa_blend.c @@ -246,8 +246,9 @@ esp_err_t ppa_do_blend(ppa_client_handle_t ppa_client, const ppa_blend_oper_conf esp_cache_msync((void *)in_fg_ext_window, in_fg_ext_window_len, ESP_CACHE_MSYNC_FLAG_DIR_C2M | ESP_CACHE_MSYNC_FLAG_UNALIGNED); // Invalidate out_buffer extended window (alignment strict on M2C direction) uint32_t out_ext_window = (uint32_t)config->out.buffer + config->out.block_offset_y * config->out.pic_w * out_pixel_depth / 8; + uint32_t out_ext_window_aligned = PPA_ALIGN_DOWN(out_ext_window, buf_alignment_size); uint32_t out_ext_window_len = config->out.pic_w * config->in_bg.block_h * out_pixel_depth / 8; - esp_cache_msync((void *)PPA_ALIGN_DOWN(out_ext_window, buf_alignment_size), PPA_ALIGN_UP(out_ext_window_len, buf_alignment_size), ESP_CACHE_MSYNC_FLAG_DIR_M2C); + esp_cache_msync((void *)out_ext_window_aligned, PPA_ALIGN_UP(out_ext_window_len + (out_ext_window - out_ext_window_aligned), buf_alignment_size), ESP_CACHE_MSYNC_FLAG_DIR_M2C); esp_err_t ret = ESP_OK; ppa_trans_t *trans_elm = NULL; diff --git a/components/esp_driver_ppa/src/ppa_fill.c b/components/esp_driver_ppa/src/ppa_fill.c index d34c338dca..1983ea3f01 100644 --- a/components/esp_driver_ppa/src/ppa_fill.c +++ b/components/esp_driver_ppa/src/ppa_fill.c @@ -107,8 +107,9 @@ esp_err_t ppa_do_fill(ppa_client_handle_t ppa_client, const ppa_fill_oper_config // Write back and invalidate necessary data (note that the window content is not continuous in the buffer) // Write back and invalidate buffer extended window (alignment not necessary on C2M direction, but alignment strict on M2C direction) uint32_t out_ext_window = (uint32_t)config->out.buffer + config->out.block_offset_y * config->out.pic_w * out_pixel_depth / 8; + uint32_t out_ext_window_aligned = PPA_ALIGN_DOWN(out_ext_window, buf_alignment_size); uint32_t out_ext_window_len = config->out.pic_w * config->fill_block_h * out_pixel_depth / 8; - esp_cache_msync((void *)PPA_ALIGN_DOWN(out_ext_window, buf_alignment_size), PPA_ALIGN_UP(out_ext_window_len, buf_alignment_size), ESP_CACHE_MSYNC_FLAG_DIR_C2M | ESP_CACHE_MSYNC_FLAG_INVALIDATE); + esp_cache_msync((void *)out_ext_window_aligned, PPA_ALIGN_UP(out_ext_window_len + (out_ext_window - out_ext_window_aligned), buf_alignment_size), ESP_CACHE_MSYNC_FLAG_DIR_C2M | ESP_CACHE_MSYNC_FLAG_INVALIDATE); esp_err_t ret = ESP_OK; ppa_trans_t *trans_elm = NULL; diff --git a/components/esp_driver_ppa/src/ppa_srm.c b/components/esp_driver_ppa/src/ppa_srm.c index 6fbcc4f7ed..f59746e866 100644 --- a/components/esp_driver_ppa/src/ppa_srm.c +++ b/components/esp_driver_ppa/src/ppa_srm.c @@ -242,8 +242,9 @@ esp_err_t ppa_do_scale_rotate_mirror(ppa_client_handle_t ppa_client, const ppa_s esp_cache_msync((void *)in_ext_window, in_ext_window_len, ESP_CACHE_MSYNC_FLAG_DIR_C2M | ESP_CACHE_MSYNC_FLAG_UNALIGNED); // Invalidate out_buffer extended window (alignment strict on M2C direction) uint32_t out_ext_window = (uint32_t)config->out.buffer + config->out.block_offset_y * config->out.pic_w * out_pixel_depth / 8; + uint32_t out_ext_window_aligned = PPA_ALIGN_DOWN(out_ext_window, buf_alignment_size); uint32_t out_ext_window_len = config->out.pic_w * config->in.block_h * out_pixel_depth / 8; - esp_cache_msync((void *)PPA_ALIGN_DOWN(out_ext_window, buf_alignment_size), PPA_ALIGN_UP(out_ext_window_len, buf_alignment_size), ESP_CACHE_MSYNC_FLAG_DIR_M2C); + esp_cache_msync((void *)out_ext_window_aligned, PPA_ALIGN_UP(out_ext_window_len + (out_ext_window - out_ext_window_aligned), buf_alignment_size), ESP_CACHE_MSYNC_FLAG_DIR_M2C); esp_err_t ret = ESP_OK; ppa_trans_t *trans_elm = NULL; From 5a0eb906daf8afbb9278424ac53ba627a89592f6 Mon Sep 17 00:00:00 2001 From: Song Ruo Jing Date: Fri, 18 Oct 2024 21:05:34 +0800 Subject: [PATCH 2/2] fix(ppa): fix a few minor issues for ppa srm and blend driver 1. The smallest scale size can be 1/16, not 1/15 2. Fix potential heap corruption if scale to a smaller size (OOB) 3. Fix mismatching writeback and invalidate data size if in_bg/fg_buffer and out_buffer are the same one and L2 cacheline size is larger than L1 cacheline size --- components/esp_driver_ppa/src/ppa_blend.c | 10 ++++++++-- components/esp_driver_ppa/src/ppa_srm.c | 14 +++++++++----- components/hal/esp32p4/include/hal/ppa_ll.h | 4 ++-- 3 files changed, 19 insertions(+), 9 deletions(-) diff --git a/components/esp_driver_ppa/src/ppa_blend.c b/components/esp_driver_ppa/src/ppa_blend.c index b5ebaa7f69..f8ffab01ee 100644 --- a/components/esp_driver_ppa/src/ppa_blend.c +++ b/components/esp_driver_ppa/src/ppa_blend.c @@ -234,16 +234,22 @@ esp_err_t ppa_do_blend(ppa_client_handle_t ppa_client, const ppa_blend_oper_conf .color_type_id = config->in_bg.blend_cm, }; uint32_t in_bg_pixel_depth = color_hal_pixel_format_get_bit_depth(in_bg_pixel_format); // bits + // Usually C2M can let the msync do alignment internally, however, it only do L1-cacheline-size alignment for L1->L2, and then L2-cacheline-size alignment for L2->mem + // While M2C direction manual alignment is L2-cacheline-size alignment for mem->L2->L1 + // Mismatching writeback and invalidate data size could cause synchronization error if in_bg/fg_buffer and out_buffer are the same one uint32_t in_bg_ext_window = (uint32_t)config->in_bg.buffer + config->in_bg.block_offset_y * config->in_bg.pic_w * in_bg_pixel_depth / 8; + uint32_t in_bg_ext_window_aligned = PPA_ALIGN_DOWN(in_bg_ext_window, buf_alignment_size); uint32_t in_bg_ext_window_len = config->in_bg.pic_w * config->in_bg.block_h * in_bg_pixel_depth / 8; - esp_cache_msync((void *)in_bg_ext_window, in_bg_ext_window_len, ESP_CACHE_MSYNC_FLAG_DIR_C2M | ESP_CACHE_MSYNC_FLAG_UNALIGNED); + esp_cache_msync((void *)in_bg_ext_window_aligned, PPA_ALIGN_UP(in_bg_ext_window_len + (in_bg_ext_window - in_bg_ext_window_aligned), buf_alignment_size), ESP_CACHE_MSYNC_FLAG_DIR_C2M); color_space_pixel_format_t in_fg_pixel_format = { .color_type_id = config->in_fg.blend_cm, }; uint32_t in_fg_pixel_depth = color_hal_pixel_format_get_bit_depth(in_fg_pixel_format); // bits uint32_t in_fg_ext_window = (uint32_t)config->in_fg.buffer + config->in_fg.block_offset_y * config->in_fg.pic_w * in_fg_pixel_depth / 8; + // Same for fg_buffer msync, do manual alignment + uint32_t in_fg_ext_window_aligned = PPA_ALIGN_DOWN(in_fg_ext_window, buf_alignment_size); uint32_t in_fg_ext_window_len = config->in_fg.pic_w * config->in_fg.block_h * in_fg_pixel_depth / 8; - esp_cache_msync((void *)in_fg_ext_window, in_fg_ext_window_len, ESP_CACHE_MSYNC_FLAG_DIR_C2M | ESP_CACHE_MSYNC_FLAG_UNALIGNED); + esp_cache_msync((void *)in_fg_ext_window_aligned, PPA_ALIGN_UP(in_fg_ext_window_len + (in_fg_ext_window - in_fg_ext_window_aligned), buf_alignment_size), ESP_CACHE_MSYNC_FLAG_DIR_C2M); // Invalidate out_buffer extended window (alignment strict on M2C direction) uint32_t out_ext_window = (uint32_t)config->out.buffer + config->out.block_offset_y * config->out.pic_w * out_pixel_depth / 8; uint32_t out_ext_window_aligned = PPA_ALIGN_DOWN(out_ext_window, buf_alignment_size); diff --git a/components/esp_driver_ppa/src/ppa_srm.c b/components/esp_driver_ppa/src/ppa_srm.c index f59746e866..0242ef37ee 100644 --- a/components/esp_driver_ppa/src/ppa_srm.c +++ b/components/esp_driver_ppa/src/ppa_srm.c @@ -194,14 +194,17 @@ esp_err_t ppa_do_scale_rotate_mirror(ppa_client_handle_t ppa_client, const ppa_s config->out.block_offset_x % 2 == 0 && config->out.block_offset_y % 2 == 0, ESP_ERR_INVALID_ARG, TAG, "YUV420 output does not support odd h/w/offset_x/offset_y"); } + ESP_RETURN_ON_FALSE(config->in.block_w <= (config->in.pic_w - config->in.block_offset_x) && + config->in.block_h <= (config->in.pic_h - config->in.block_offset_y), + ESP_ERR_INVALID_ARG, TAG, "in.block_w/h + in.block_offset_x/y does not fit in the in pic"); color_space_pixel_format_t out_pixel_format = { .color_type_id = config->out.srm_cm, }; uint32_t out_pixel_depth = color_hal_pixel_format_get_bit_depth(out_pixel_format); // bits uint32_t out_pic_len = config->out.pic_w * config->out.pic_h * out_pixel_depth / 8; ESP_RETURN_ON_FALSE(out_pic_len <= config->out.buffer_size, ESP_ERR_INVALID_ARG, TAG, "out.pic_w/h mismatch with out.buffer_size"); - ESP_RETURN_ON_FALSE(config->scale_x < (PPA_LL_SRM_SCALING_INT_MAX + 1) && config->scale_x >= (1.0 / PPA_LL_SRM_SCALING_FRAG_MAX) && - config->scale_y < (PPA_LL_SRM_SCALING_INT_MAX + 1) && config->scale_y >= (1.0 / PPA_LL_SRM_SCALING_FRAG_MAX), + ESP_RETURN_ON_FALSE(config->scale_x < PPA_LL_SRM_SCALING_INT_MAX && config->scale_x >= (1.0 / PPA_LL_SRM_SCALING_FRAG_MAX) && + config->scale_y < PPA_LL_SRM_SCALING_INT_MAX && config->scale_y >= (1.0 / PPA_LL_SRM_SCALING_FRAG_MAX), ESP_ERR_INVALID_ARG, TAG, "invalid scale"); uint32_t new_block_w = 0; uint32_t new_block_h = 0; @@ -243,7 +246,8 @@ esp_err_t ppa_do_scale_rotate_mirror(ppa_client_handle_t ppa_client, const ppa_s // Invalidate out_buffer extended window (alignment strict on M2C direction) uint32_t out_ext_window = (uint32_t)config->out.buffer + config->out.block_offset_y * config->out.pic_w * out_pixel_depth / 8; uint32_t out_ext_window_aligned = PPA_ALIGN_DOWN(out_ext_window, buf_alignment_size); - uint32_t out_ext_window_len = config->out.pic_w * config->in.block_h * out_pixel_depth / 8; + uint32_t out_ext_window_len = config->out.pic_w * new_block_h * out_pixel_depth / 8; // actual ext_window_len must be less than or equal to this, since actual block_h <= new_block_h (may round down) + assert(out_ext_window + out_ext_window_len <= (uint32_t)config->out.buffer + config->out.buffer_size); esp_cache_msync((void *)out_ext_window_aligned, PPA_ALIGN_UP(out_ext_window_len + (out_ext_window - out_ext_window_aligned), buf_alignment_size), ESP_CACHE_MSYNC_FLAG_DIR_M2C); esp_err_t ret = ESP_OK; @@ -256,9 +260,9 @@ esp_err_t ppa_do_scale_rotate_mirror(ppa_client_handle_t ppa_client, const ppa_s ppa_srm_oper_t *srm_trans_desc = (ppa_srm_oper_t *)trans_on_picked_desc->srm_desc; memcpy(srm_trans_desc, config, sizeof(ppa_srm_oper_config_t)); srm_trans_desc->scale_x_int = (uint32_t)srm_trans_desc->scale_x; - srm_trans_desc->scale_x_frag = (uint32_t)(srm_trans_desc->scale_x * (PPA_LL_SRM_SCALING_FRAG_MAX + 1)) & PPA_LL_SRM_SCALING_FRAG_MAX; + srm_trans_desc->scale_x_frag = (uint32_t)(srm_trans_desc->scale_x * PPA_LL_SRM_SCALING_FRAG_MAX) & (PPA_LL_SRM_SCALING_FRAG_MAX - 1); srm_trans_desc->scale_y_int = (uint32_t)srm_trans_desc->scale_y; - srm_trans_desc->scale_y_frag = (uint32_t)(srm_trans_desc->scale_y * (PPA_LL_SRM_SCALING_FRAG_MAX + 1)) & PPA_LL_SRM_SCALING_FRAG_MAX; + srm_trans_desc->scale_y_frag = (uint32_t)(srm_trans_desc->scale_y * PPA_LL_SRM_SCALING_FRAG_MAX) & (PPA_LL_SRM_SCALING_FRAG_MAX - 1); srm_trans_desc->alpha_value = new_alpha_value; srm_trans_desc->data_burst_length = ppa_client->data_burst_length; diff --git a/components/hal/esp32p4/include/hal/ppa_ll.h b/components/hal/esp32p4/include/hal/ppa_ll.h index 60facd94fe..1d82fdce92 100644 --- a/components/hal/esp32p4/include/hal/ppa_ll.h +++ b/components/hal/esp32p4/include/hal/ppa_ll.h @@ -24,8 +24,8 @@ extern "C" { #define PPA_LL_BLEND0_CLUT_MEM_ADDR_OFFSET 0x400 #define PPA_LL_BLEND1_CLUT_MEM_ADDR_OFFSET 0x800 -#define PPA_LL_SRM_SCALING_INT_MAX PPA_SR_SCAL_X_INT_V -#define PPA_LL_SRM_SCALING_FRAG_MAX PPA_SR_SCAL_X_FRAG_V +#define PPA_LL_SRM_SCALING_INT_MAX (PPA_SR_SCAL_X_INT_V + 1) +#define PPA_LL_SRM_SCALING_FRAG_MAX (PPA_SR_SCAL_X_FRAG_V + 1) // TODO: On P4 ECO2, SRM block size needs update #define PPA_LL_SRM_DEFAULT_BLOCK_SIZE 18 // 18 x 18 block size