Re: [v2] media: mediatek: vcodec: fix AV1 decode fail for 36bit iova
Hi,
Le mardi 04 juillet 2023 à 09:51 +0800, Xiaoyong Lu a écrit :
> Fix av1 decode fail when iova is 36bit.
I'd change the subject to "media: mediatek: vcodec: fix AV1 decoding on MT8188"
And rephrase this one to:
Fix AV1 decoding failure when the iova is 36bit.
>
> Decoder hardware will access incorrect iova address when tile buffer is
> 36bit, it will lead to iommu fault when hardware access dram data.
Suggest to rephrase this:
Before this fix, the decoder was accessing incorrect addresses with 36bit
iova tile buffer, leading to iommu faults.
>
> Fixes: 2f5d0aef37c6 ("media: mediatek: vcodec: support stateless AV1 decoder")
> Signed-off-by: Xiaoyong Lu
With some rework of the commit message, see my suggestions above:
Reviewed-by: Nicolas Dufresne
> ---
> Changes from v1
>
> - prefer '|' rather than '+'
> - prefer '&' rather than shift operation
> - add comments for address operations
>
> v1:
> - VDEC HW can access tile buffer and decode normally.
> - Test ok by mt8195 32bit and mt8188 36bit iova.
>
> ---
> .../mediatek/vcodec/vdec/vdec_av1_req_lat_if.c | 12
> 1 file changed, 8 insertions(+), 4 deletions(-)
>
> diff --git
> a/drivers/media/platform/mediatek/vcodec/vdec/vdec_av1_req_lat_if.c
> b/drivers/media/platform/mediatek/vcodec/vdec/vdec_av1_req_lat_if.c
> index 404a1a23fd402..e9f2393f6a883 100644
> --- a/drivers/media/platform/mediatek/vcodec/vdec/vdec_av1_req_lat_if.c
> +++ b/drivers/media/platform/mediatek/vcodec/vdec/vdec_av1_req_lat_if.c
> @@ -1658,9 +1658,9 @@ static void vdec_av1_slice_setup_tile_buffer(struct
> vdec_av1_slice_instance *ins
> u32 allow_update_cdf = 0;
> u32 sb_boundary_x_m1 = 0, sb_boundary_y_m1 = 0;
> int tile_info_base;
> - u32 tile_buf_pa;
> + u64 tile_buf_pa;
> u32 *tile_info_buf = instance->tile.va;
> - u32 pa = (u32)bs->dma_addr;
> + u64 pa = (u64)bs->dma_addr;
>
> if (uh->disable_cdf_update == 0)
> allow_update_cdf = 1;
> @@ -1673,8 +1673,12 @@ static void vdec_av1_slice_setup_tile_buffer(struct
> vdec_av1_slice_instance *ins
> tile_info_buf[tile_info_base + 0] =
> (tile_group->tile_size[tile_num] << 3);
> tile_buf_pa = pa + tile_group->tile_start_offset[tile_num];
>
> - tile_info_buf[tile_info_base + 1] = (tile_buf_pa >> 4) << 4;
> - tile_info_buf[tile_info_base + 2] = (tile_buf_pa % 16) << 3;
> + /* save av1 tile high 4bits(bit 32-35) address in lower 4 bits
> position
> + * and clear original for hw requirement.
> + */
> + tile_info_buf[tile_info_base + 1] = (tile_buf_pa &
> 0xFFF0ull) |
> + ((tile_buf_pa & 0xFull) >> 32);
> + tile_info_buf[tile_info_base + 2] = (tile_buf_pa & 0xFull) << 3;
>
> sb_boundary_x_m1 =
> (tile->mi_col_starts[tile_col + 1] -
> tile->mi_col_starts[tile_col] - 1) &
[v2] media: mediatek: vcodec: fix AV1 decode fail for 36bit iova
Fix av1 decode fail when iova is 36bit.
Decoder hardware will access incorrect iova address when tile buffer is
36bit, it will lead to iommu fault when hardware access dram data.
Fixes: 2f5d0aef37c6 ("media: mediatek: vcodec: support stateless AV1 decoder")
Signed-off-by: Xiaoyong Lu
---
Changes from v1
- prefer '|' rather than '+'
- prefer '&' rather than shift operation
- add comments for address operations
v1:
- VDEC HW can access tile buffer and decode normally.
- Test ok by mt8195 32bit and mt8188 36bit iova.
---
.../mediatek/vcodec/vdec/vdec_av1_req_lat_if.c | 12
1 file changed, 8 insertions(+), 4 deletions(-)
diff --git a/drivers/media/platform/mediatek/vcodec/vdec/vdec_av1_req_lat_if.c
b/drivers/media/platform/mediatek/vcodec/vdec/vdec_av1_req_lat_if.c
index 404a1a23fd402..e9f2393f6a883 100644
--- a/drivers/media/platform/mediatek/vcodec/vdec/vdec_av1_req_lat_if.c
+++ b/drivers/media/platform/mediatek/vcodec/vdec/vdec_av1_req_lat_if.c
@@ -1658,9 +1658,9 @@ static void vdec_av1_slice_setup_tile_buffer(struct
vdec_av1_slice_instance *ins
u32 allow_update_cdf = 0;
u32 sb_boundary_x_m1 = 0, sb_boundary_y_m1 = 0;
int tile_info_base;
- u32 tile_buf_pa;
+ u64 tile_buf_pa;
u32 *tile_info_buf = instance->tile.va;
- u32 pa = (u32)bs->dma_addr;
+ u64 pa = (u64)bs->dma_addr;
if (uh->disable_cdf_update == 0)
allow_update_cdf = 1;
@@ -1673,8 +1673,12 @@ static void vdec_av1_slice_setup_tile_buffer(struct
vdec_av1_slice_instance *ins
tile_info_buf[tile_info_base + 0] =
(tile_group->tile_size[tile_num] << 3);
tile_buf_pa = pa + tile_group->tile_start_offset[tile_num];
- tile_info_buf[tile_info_base + 1] = (tile_buf_pa >> 4) << 4;
- tile_info_buf[tile_info_base + 2] = (tile_buf_pa % 16) << 3;
+ /* save av1 tile high 4bits(bit 32-35) address in lower 4 bits
position
+* and clear original for hw requirement.
+*/
+ tile_info_buf[tile_info_base + 1] = (tile_buf_pa &
0xFFF0ull) |
+ ((tile_buf_pa & 0xFull) >> 32);
+ tile_info_buf[tile_info_base + 2] = (tile_buf_pa & 0xFull) << 3;
sb_boundary_x_m1 =
(tile->mi_col_starts[tile_col + 1] -
tile->mi_col_starts[tile_col] - 1) &
--
2.18.0
Re: media: mediatek: vcodec: fix AV1 decode fail for 36bit iova
Hi Xiaoyong,
kernel test robot noticed the following build warnings:
[auto build test WARNING on next-20230627]
[cannot apply to media-tree/master v6.4 v6.4-rc7 v6.4-rc6 linus/master v6.4]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url:
https://github.com/intel-lab-lkp/linux/commits/Xiaoyong-Lu/media-mediatek-vcodec-fix-AV1-decode-fail-for-36bit-iova/20230628-134327
base: next-20230627
patch link:
https://lore.kernel.org/r/20230628054111.8967-1-xiaoyong.lu%40mediatek.com
patch subject: media: mediatek: vcodec: fix AV1 decode fail for 36bit iova
config: arm-allmodconfig
(https://download.01.org/0day-ci/archive/20230629/202306291250.o5amgfic-...@intel.com/config)
compiler: arm-linux-gnueabi-gcc (GCC) 12.3.0
reproduce:
(https://download.01.org/0day-ci/archive/20230629/202306291250.o5amgfic-...@intel.com/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot
| Closes:
https://lore.kernel.org/oe-kbuild-all/202306291250.o5amgfic-...@intel.com/
All warnings (new ones prefixed by >>):
drivers/media/platform/mediatek/vcodec/vdec/vdec_av1_req_lat_if.c: In
function 'vdec_av1_slice_setup_tile_buffer':
>> drivers/media/platform/mediatek/vcodec/vdec/vdec_av1_req_lat_if.c:1676:91:
>> warning: suggest parentheses around '+' inside '<<' [-Wparentheses]
1676 | tile_info_buf[tile_info_base + 1] = (unsigned
int)(tile_buf_pa >> 4) << 4 +
|
~~^
1677 | ((unsigned int)(tile_buf_pa >> 32) & 0xf);
| ~
Kconfig warnings: (for reference only)
WARNING: unmet direct dependencies detected for SM_GCC_8350
Depends on [n]: COMMON_CLK [=y] && COMMON_CLK_QCOM [=m] && (ARM64 ||
COMPILE_TEST [=n])
Selected by [m]:
- SM_VIDEOCC_8350 [=m] && COMMON_CLK [=y] && COMMON_CLK_QCOM [=m]
WARNING: unmet direct dependencies detected for SM_GCC_8450
Depends on [n]: COMMON_CLK [=y] && COMMON_CLK_QCOM [=m] && (ARM64 ||
COMPILE_TEST [=n])
Selected by [m]:
- SM_GPUCC_8450 [=m] && COMMON_CLK [=y] && COMMON_CLK_QCOM [=m]
- SM_VIDEOCC_8450 [=m] && COMMON_CLK [=y] && COMMON_CLK_QCOM [=m]
WARNING: unmet direct dependencies detected for SM_GCC_8550
Depends on [n]: COMMON_CLK [=y] && COMMON_CLK_QCOM [=m] && (ARM64 ||
COMPILE_TEST [=n])
Selected by [m]:
- SM_GPUCC_8550 [=m] && COMMON_CLK [=y] && COMMON_CLK_QCOM [=m]
- SM_VIDEOCC_8550 [=m] && COMMON_CLK [=y] && COMMON_CLK_QCOM [=m]
vim +1676 drivers/media/platform/mediatek/vcodec/vdec/vdec_av1_req_lat_if.c
1649
1650 static void vdec_av1_slice_setup_tile_buffer(struct
vdec_av1_slice_instance *instance,
1651 struct vdec_av1_slice_vsi
*vsi,
1652 struct mtk_vcodec_mem *bs)
1653 {
1654 struct vdec_av1_slice_tile_group *tile_group =
>tile_group;
1655 struct vdec_av1_slice_uncompressed_header *uh = >frame.uh;
1656 struct vdec_av1_slice_tile *tile = >tile;
1657 u32 tile_num, tile_row, tile_col;
1658 u32 allow_update_cdf = 0;
1659 u32 sb_boundary_x_m1 = 0, sb_boundary_y_m1 = 0;
1660 int tile_info_base;
1661 u64 tile_buf_pa;
1662 u32 *tile_info_buf = instance->tile.va;
1663 u64 pa = (u64)bs->dma_addr;
1664
1665 if (uh->disable_cdf_update == 0)
1666 allow_update_cdf = 1;
1667
1668 for (tile_num = 0; tile_num < tile_group->num_tiles;
tile_num++) {
1669 /* each uint32 takes place of 4 bytes */
1670 tile_info_base = (AV1_TILE_BUF_SIZE * tile_num) >> 2;
1671 tile_row = tile_num / tile->tile_cols;
1672 tile_col = tile_num % tile->tile_cols;
1673 tile_info_buf[tile_info_base + 0] =
(tile_group->tile_size[tile_num] << 3);
1674 tile_buf_pa = pa +
tile_group->tile_start_offset[tile_num];
1675
> 1676 tile_info_buf[tile_info_base + 1] = (unsigned
> int)(tile_buf_pa >> 4) << 4 +
1677 ((unsigned int)(tile_buf_pa >> 32) & 0xf);
1678 tile_info_buf[tile_info_base + 2] = (tile_buf_pa % 16)
<< 3;
1679
1680 sb_boundary_x_m1 =
1681
Re: media: mediatek: vcodec: fix AV1 decode fail for 36bit iova
Hi,
Le mercredi 28 juin 2023 à 13:41 +0800, Xiaoyong Lu a écrit :
> Decoder hardware will access incorrect iova address when tile buffer is
> 36bit, leading to iommu fault when hardware access dram data.
>
> Fixes: 2f5d0aef37c6 ("media: mediatek: vcodec: support stateless AV1 decoder")
> Signed-off-by: Xiaoyong Lu
> ---
> - Test ok: mt8195 32bit and mt8188 36bit iova.
> ---
> .../platform/mediatek/vcodec/vdec/vdec_av1_req_lat_if.c| 7 ---
> 1 file changed, 4 insertions(+), 3 deletions(-)
>
> diff --git
> a/drivers/media/platform/mediatek/vcodec/vdec/vdec_av1_req_lat_if.c
> b/drivers/media/platform/mediatek/vcodec/vdec/vdec_av1_req_lat_if.c
> index 404a1a23fd40..420222c8a56d 100644
> --- a/drivers/media/platform/mediatek/vcodec/vdec/vdec_av1_req_lat_if.c
> +++ b/drivers/media/platform/mediatek/vcodec/vdec/vdec_av1_req_lat_if.c
> @@ -1658,9 +1658,9 @@ static void vdec_av1_slice_setup_tile_buffer(struct
> vdec_av1_slice_instance *ins
> u32 allow_update_cdf = 0;
> u32 sb_boundary_x_m1 = 0, sb_boundary_y_m1 = 0;
> int tile_info_base;
> - u32 tile_buf_pa;
> + u64 tile_buf_pa;
> u32 *tile_info_buf = instance->tile.va;
> - u32 pa = (u32)bs->dma_addr;
> + u64 pa = (u64)bs->dma_addr;
>
> if (uh->disable_cdf_update == 0)
> allow_update_cdf = 1;
> @@ -1673,7 +1673,8 @@ static void vdec_av1_slice_setup_tile_buffer(struct
> vdec_av1_slice_instance *ins
> tile_info_buf[tile_info_base + 0] =
> (tile_group->tile_size[tile_num] << 3);
> tile_buf_pa = pa + tile_group->tile_start_offset[tile_num];
>
> - tile_info_buf[tile_info_base + 1] = (tile_buf_pa >> 4) << 4;
> + tile_info_buf[tile_info_base + 1] = (unsigned int)(tile_buf_pa
> >> 4) << 4 +
> + ((unsigned int)(tile_buf_pa >> 32) & 0xf);
I'm not clear on how this works. In the original code, it was a complicated way
to ignore the 4 least significant bits. Something like this would avoid the cast
and clarify it:
tile_info_buf[tile_info_base + 1] = tile_buf_pa &
0xFF00ull;
But in the updated code, if you have 36 bit, you store these 2 bits in the lower
part, which was originally cleared. Can you confirm this is exactly what you
wanted ? And if so add a comment ? It could also be written has (but this is
just me considering this more readable, I also prefer | (or) rather then +, and
hates casting):
tile_info_buf[tile_info_base + 1] = (tile_buf_pa &
0xFF00ull) |
(tile_buf_pa & 0x000Full) >> 32;
> tile_info_buf[tile_info_base + 2] = (tile_buf_pa % 16) << 3;
Is this the same as ?
tile_info_buf[tile_info_base + 2] = (tile_buf_pa & 0x00FFull)
<< 3;
>
>
> sb_boundary_x_m1 =
media: mediatek: vcodec: fix AV1 decode fail for 36bit iova
Decoder hardware will access incorrect iova address when tile buffer is
36bit, leading to iommu fault when hardware access dram data.
Fixes: 2f5d0aef37c6 ("media: mediatek: vcodec: support stateless AV1 decoder")
Signed-off-by: Xiaoyong Lu
---
- Test ok: mt8195 32bit and mt8188 36bit iova.
---
.../platform/mediatek/vcodec/vdec/vdec_av1_req_lat_if.c| 7 ---
1 file changed, 4 insertions(+), 3 deletions(-)
diff --git a/drivers/media/platform/mediatek/vcodec/vdec/vdec_av1_req_lat_if.c
b/drivers/media/platform/mediatek/vcodec/vdec/vdec_av1_req_lat_if.c
index 404a1a23fd40..420222c8a56d 100644
--- a/drivers/media/platform/mediatek/vcodec/vdec/vdec_av1_req_lat_if.c
+++ b/drivers/media/platform/mediatek/vcodec/vdec/vdec_av1_req_lat_if.c
@@ -1658,9 +1658,9 @@ static void vdec_av1_slice_setup_tile_buffer(struct
vdec_av1_slice_instance *ins
u32 allow_update_cdf = 0;
u32 sb_boundary_x_m1 = 0, sb_boundary_y_m1 = 0;
int tile_info_base;
- u32 tile_buf_pa;
+ u64 tile_buf_pa;
u32 *tile_info_buf = instance->tile.va;
- u32 pa = (u32)bs->dma_addr;
+ u64 pa = (u64)bs->dma_addr;
if (uh->disable_cdf_update == 0)
allow_update_cdf = 1;
@@ -1673,7 +1673,8 @@ static void vdec_av1_slice_setup_tile_buffer(struct
vdec_av1_slice_instance *ins
tile_info_buf[tile_info_base + 0] =
(tile_group->tile_size[tile_num] << 3);
tile_buf_pa = pa + tile_group->tile_start_offset[tile_num];
- tile_info_buf[tile_info_base + 1] = (tile_buf_pa >> 4) << 4;
+ tile_info_buf[tile_info_base + 1] = (unsigned int)(tile_buf_pa
>> 4) << 4 +
+ ((unsigned int)(tile_buf_pa >> 32) & 0xf);
tile_info_buf[tile_info_base + 2] = (tile_buf_pa % 16) << 3;
sb_boundary_x_m1 =
--
2.25.1
