| Message ID | 20250328063056.762-3-ming.qian@oss.nxp.com |
|---|---|
| State | New |
| Headers | show |
| Series | media: imx-jpeg: Fix some motion-jpeg decoding issues | expand |
Hey Ming, On 28.03.2025 14:30, ming.qian@oss.nxp.com wrote: >From: Ming Qian <ming.qian@oss.nxp.com> > >To support decoding motion-jpeg without DHT, driver will try to decode a >pattern jpeg before actual jpeg frame by use of linked descriptors >(This is called "repeat mode"), then the DHT in the pattern jpeg can be >used for decoding the motion-jpeg. > >To avoid performance loss, use the smallest supported resolution 64x64 >as the pattern jpeg size. > >But there is a hardware issue: when the JPEG decoded frame with a >resolution that is no larger than 64x64 and it is followed by a next >decoded frame with a larger resolution but not 64 aligned, then this >next decoded frame may be corrupted. > >To avoid corruption of the decoded image, we change the pattern jpeg >size to 128x64, as we confirmed with the hardware designer that this is >a safe size. > >Besides, we also need to allocate a dma buffer to store the decoded >picture for the pattern image. Why is that related to the change of the pattern size? Like why wasn't that needed for 64x64? And if this solves a different issue, can you put that into an extra patch? This is a bit hard to understand, maybe this is better: In order to decode a motion-jpeg bitstream, which doesn't provide a DHT, the driver will first decode a pattern jpeg and use the DHT found in the pattern to decode the first actual frame. This mode is called "repeat-mode" and it utilizes linked descriptors. The smallest supported resolution of 64x64 was used for that pattern to not cause unneeded performance delay. This choice, however, can cause a corrupted decoded picture of the first frame after the pattern, when the resolution of that frame is larger than the pattern and is not aligned to 64. By altering the pattern size to 128x64, this corruption can be avoided. That size has been confirmed to be safe by the hardware designers. Additionally, a DMA buffer needs to be allocated to store the decoded picture of the pattern image. The rest looks good. Regards, Sebastian > >Signed-off-by: Ming Qian <ming.qian@oss.nxp.com> >--- > .../media/platform/nxp/imx-jpeg/mxc-jpeg.c | 42 +++++++++++++++---- > .../media/platform/nxp/imx-jpeg/mxc-jpeg.h | 5 +++ > 2 files changed, 39 insertions(+), 8 deletions(-) > >diff --git a/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.c b/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.c >index 12661c177f5a..45705c606769 100644 >--- a/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.c >+++ b/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.c >@@ -535,7 +535,18 @@ static const unsigned char jpeg_sos_maximal[] = { > }; > > static const unsigned char jpeg_image_red[] = { >- 0xFC, 0x5F, 0xA2, 0xBF, 0xCA, 0x73, 0xFE, 0xFE, >+ 0xF9, 0xFE, 0x8A, 0xFC, 0x34, 0xFD, 0xC4, 0x28, >+ 0xA0, 0x02, 0x8A, 0x00, 0x28, 0xA0, 0x02, 0x8A, >+ 0x00, 0x28, 0xA0, 0x02, 0x8A, 0x00, 0x28, 0xA0, >+ 0x02, 0x8A, 0x00, 0x28, 0xA0, 0x02, 0x8A, 0x00, >+ 0x28, 0xA0, 0x02, 0x8A, 0x00, 0x28, 0xA0, 0x02, >+ 0x8A, 0x00, 0x28, 0xA0, 0x02, 0x8A, 0x00, 0x28, >+ 0xA0, 0x02, 0x8A, 0x00, 0x28, 0xA0, 0x02, 0x8A, >+ 0x00, 0x28, 0xA0, 0x02, 0x8A, 0x00, 0x28, 0xA0, >+ 0x02, 0x8A, 0x00, 0x28, 0xA0, 0x02, 0x8A, 0x00, >+ 0x28, 0xA0, 0x02, 0x8A, 0x00, 0x28, 0xA0, 0x02, >+ 0x8A, 0x00, 0x28, 0xA0, 0x0F, 0xFF, 0xD0, 0xF9, >+ 0xFE, 0x8A, 0xFC, 0x34, 0xFD, 0xC4, 0x28, 0xA0, > 0x02, 0x8A, 0x00, 0x28, 0xA0, 0x02, 0x8A, 0x00, > 0x28, 0xA0, 0x02, 0x8A, 0x00, 0x28, 0xA0, 0x02, > 0x8A, 0x00, 0x28, 0xA0, 0x02, 0x8A, 0x00, 0x28, >@@ -545,7 +556,7 @@ static const unsigned char jpeg_image_red[] = { > 0x28, 0xA0, 0x02, 0x8A, 0x00, 0x28, 0xA0, 0x02, > 0x8A, 0x00, 0x28, 0xA0, 0x02, 0x8A, 0x00, 0x28, > 0xA0, 0x02, 0x8A, 0x00, 0x28, 0xA0, 0x02, 0x8A, >- 0x00, 0x28, 0xA0, 0x02, 0x8A, 0x00 >+ 0x00, 0x28, 0xA0, 0x0F > }; > > static const unsigned char jpeg_eoi[] = { >@@ -775,6 +786,13 @@ static void mxc_jpeg_free_slot_data(struct mxc_jpeg_dev *jpeg) > jpeg->slot_data.cfg_stream_vaddr = NULL; > jpeg->slot_data.cfg_stream_handle = 0; > >+ dma_free_coherent(jpeg->dev, jpeg->slot_data.cfg_dec_size, >+ jpeg->slot_data.cfg_dec_vaddr, >+ jpeg->slot_data.cfg_dec_daddr); >+ jpeg->slot_data.cfg_dec_size = 0; >+ jpeg->slot_data.cfg_dec_vaddr = NULL; >+ jpeg->slot_data.cfg_dec_daddr = 0; >+ > jpeg->slot_data.used = false; > } > >@@ -814,6 +832,14 @@ static bool mxc_jpeg_alloc_slot_data(struct mxc_jpeg_dev *jpeg) > goto err; > jpeg->slot_data.cfg_stream_vaddr = cfg_stm; > >+ jpeg->slot_data.cfg_dec_size = MXC_JPEG_PATTERN_WIDTH * MXC_JPEG_PATTERN_HEIGHT * 2; >+ jpeg->slot_data.cfg_dec_vaddr = dma_alloc_coherent(jpeg->dev, >+ jpeg->slot_data.cfg_dec_size, >+ &jpeg->slot_data.cfg_dec_daddr, >+ GFP_ATOMIC); >+ if (!jpeg->slot_data.cfg_dec_vaddr) >+ goto err; >+ > skip_alloc: > jpeg->slot_data.used = true; > >@@ -1216,14 +1242,14 @@ static void mxc_jpeg_config_dec_desc(struct vb2_buffer *out_buf, > */ > *cfg_size = mxc_jpeg_setup_cfg_stream(cfg_stream_vaddr, > V4L2_PIX_FMT_YUYV, >- MXC_JPEG_MIN_WIDTH, >- MXC_JPEG_MIN_HEIGHT); >+ MXC_JPEG_PATTERN_WIDTH, >+ MXC_JPEG_PATTERN_HEIGHT); > cfg_desc->next_descpt_ptr = desc_handle | MXC_NXT_DESCPT_EN; >- cfg_desc->buf_base0 = vb2_dma_contig_plane_dma_addr(dst_buf, 0); >+ cfg_desc->buf_base0 = jpeg->slot_data.cfg_dec_daddr; > cfg_desc->buf_base1 = 0; >- cfg_desc->imgsize = MXC_JPEG_MIN_WIDTH << 16; >- cfg_desc->imgsize |= MXC_JPEG_MIN_HEIGHT; >- cfg_desc->line_pitch = MXC_JPEG_MIN_WIDTH * 2; >+ cfg_desc->imgsize = MXC_JPEG_PATTERN_WIDTH << 16; >+ cfg_desc->imgsize |= MXC_JPEG_PATTERN_HEIGHT; >+ cfg_desc->line_pitch = MXC_JPEG_PATTERN_WIDTH * 2; > cfg_desc->stm_ctrl = STM_CTRL_IMAGE_FORMAT(MXC_JPEG_YUV422); > cfg_desc->stm_ctrl |= STM_CTRL_BITBUF_PTR_CLR(1); > cfg_desc->stm_bufbase = cfg_stream_handle; >diff --git a/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.h b/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.h >index 86e324b21aed..fdde45f7e163 100644 >--- a/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.h >+++ b/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.h >@@ -28,6 +28,8 @@ > #define MXC_JPEG_W_ALIGN 3 > #define MXC_JPEG_MAX_SIZEIMAGE 0xFFFFFC00 > #define MXC_JPEG_MAX_PLANES 2 >+#define MXC_JPEG_PATTERN_WIDTH 128 >+#define MXC_JPEG_PATTERN_HEIGHT 64 > > enum mxc_jpeg_enc_state { > MXC_JPEG_ENCODING = 0, /* jpeg encode phase */ >@@ -117,6 +119,9 @@ struct mxc_jpeg_slot_data { > dma_addr_t desc_handle; > dma_addr_t cfg_desc_handle; // configuration descriptor dma address > dma_addr_t cfg_stream_handle; // configuration bitstream dma address >+ dma_addr_t cfg_dec_size; >+ void *cfg_dec_vaddr; >+ dma_addr_t cfg_dec_daddr; > }; > > struct mxc_jpeg_dev { >-- >2.43.0-rc1 > >
diff --git a/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.c b/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.c index 12661c177f5a..45705c606769 100644 --- a/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.c +++ b/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.c @@ -535,7 +535,18 @@ static const unsigned char jpeg_sos_maximal[] = { }; static const unsigned char jpeg_image_red[] = { - 0xFC, 0x5F, 0xA2, 0xBF, 0xCA, 0x73, 0xFE, 0xFE, + 0xF9, 0xFE, 0x8A, 0xFC, 0x34, 0xFD, 0xC4, 0x28, + 0xA0, 0x02, 0x8A, 0x00, 0x28, 0xA0, 0x02, 0x8A, + 0x00, 0x28, 0xA0, 0x02, 0x8A, 0x00, 0x28, 0xA0, + 0x02, 0x8A, 0x00, 0x28, 0xA0, 0x02, 0x8A, 0x00, + 0x28, 0xA0, 0x02, 0x8A, 0x00, 0x28, 0xA0, 0x02, + 0x8A, 0x00, 0x28, 0xA0, 0x02, 0x8A, 0x00, 0x28, + 0xA0, 0x02, 0x8A, 0x00, 0x28, 0xA0, 0x02, 0x8A, + 0x00, 0x28, 0xA0, 0x02, 0x8A, 0x00, 0x28, 0xA0, + 0x02, 0x8A, 0x00, 0x28, 0xA0, 0x02, 0x8A, 0x00, + 0x28, 0xA0, 0x02, 0x8A, 0x00, 0x28, 0xA0, 0x02, + 0x8A, 0x00, 0x28, 0xA0, 0x0F, 0xFF, 0xD0, 0xF9, + 0xFE, 0x8A, 0xFC, 0x34, 0xFD, 0xC4, 0x28, 0xA0, 0x02, 0x8A, 0x00, 0x28, 0xA0, 0x02, 0x8A, 0x00, 0x28, 0xA0, 0x02, 0x8A, 0x00, 0x28, 0xA0, 0x02, 0x8A, 0x00, 0x28, 0xA0, 0x02, 0x8A, 0x00, 0x28, @@ -545,7 +556,7 @@ static const unsigned char jpeg_image_red[] = { 0x28, 0xA0, 0x02, 0x8A, 0x00, 0x28, 0xA0, 0x02, 0x8A, 0x00, 0x28, 0xA0, 0x02, 0x8A, 0x00, 0x28, 0xA0, 0x02, 0x8A, 0x00, 0x28, 0xA0, 0x02, 0x8A, - 0x00, 0x28, 0xA0, 0x02, 0x8A, 0x00 + 0x00, 0x28, 0xA0, 0x0F }; static const unsigned char jpeg_eoi[] = { @@ -775,6 +786,13 @@ static void mxc_jpeg_free_slot_data(struct mxc_jpeg_dev *jpeg) jpeg->slot_data.cfg_stream_vaddr = NULL; jpeg->slot_data.cfg_stream_handle = 0; + dma_free_coherent(jpeg->dev, jpeg->slot_data.cfg_dec_size, + jpeg->slot_data.cfg_dec_vaddr, + jpeg->slot_data.cfg_dec_daddr); + jpeg->slot_data.cfg_dec_size = 0; + jpeg->slot_data.cfg_dec_vaddr = NULL; + jpeg->slot_data.cfg_dec_daddr = 0; + jpeg->slot_data.used = false; } @@ -814,6 +832,14 @@ static bool mxc_jpeg_alloc_slot_data(struct mxc_jpeg_dev *jpeg) goto err; jpeg->slot_data.cfg_stream_vaddr = cfg_stm; + jpeg->slot_data.cfg_dec_size = MXC_JPEG_PATTERN_WIDTH * MXC_JPEG_PATTERN_HEIGHT * 2; + jpeg->slot_data.cfg_dec_vaddr = dma_alloc_coherent(jpeg->dev, + jpeg->slot_data.cfg_dec_size, + &jpeg->slot_data.cfg_dec_daddr, + GFP_ATOMIC); + if (!jpeg->slot_data.cfg_dec_vaddr) + goto err; + skip_alloc: jpeg->slot_data.used = true; @@ -1216,14 +1242,14 @@ static void mxc_jpeg_config_dec_desc(struct vb2_buffer *out_buf, */ *cfg_size = mxc_jpeg_setup_cfg_stream(cfg_stream_vaddr, V4L2_PIX_FMT_YUYV, - MXC_JPEG_MIN_WIDTH, - MXC_JPEG_MIN_HEIGHT); + MXC_JPEG_PATTERN_WIDTH, + MXC_JPEG_PATTERN_HEIGHT); cfg_desc->next_descpt_ptr = desc_handle | MXC_NXT_DESCPT_EN; - cfg_desc->buf_base0 = vb2_dma_contig_plane_dma_addr(dst_buf, 0); + cfg_desc->buf_base0 = jpeg->slot_data.cfg_dec_daddr; cfg_desc->buf_base1 = 0; - cfg_desc->imgsize = MXC_JPEG_MIN_WIDTH << 16; - cfg_desc->imgsize |= MXC_JPEG_MIN_HEIGHT; - cfg_desc->line_pitch = MXC_JPEG_MIN_WIDTH * 2; + cfg_desc->imgsize = MXC_JPEG_PATTERN_WIDTH << 16; + cfg_desc->imgsize |= MXC_JPEG_PATTERN_HEIGHT; + cfg_desc->line_pitch = MXC_JPEG_PATTERN_WIDTH * 2; cfg_desc->stm_ctrl = STM_CTRL_IMAGE_FORMAT(MXC_JPEG_YUV422); cfg_desc->stm_ctrl |= STM_CTRL_BITBUF_PTR_CLR(1); cfg_desc->stm_bufbase = cfg_stream_handle; diff --git a/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.h b/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.h index 86e324b21aed..fdde45f7e163 100644 --- a/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.h +++ b/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.h @@ -28,6 +28,8 @@ #define MXC_JPEG_W_ALIGN 3 #define MXC_JPEG_MAX_SIZEIMAGE 0xFFFFFC00 #define MXC_JPEG_MAX_PLANES 2 +#define MXC_JPEG_PATTERN_WIDTH 128 +#define MXC_JPEG_PATTERN_HEIGHT 64 enum mxc_jpeg_enc_state { MXC_JPEG_ENCODING = 0, /* jpeg encode phase */ @@ -117,6 +119,9 @@ struct mxc_jpeg_slot_data { dma_addr_t desc_handle; dma_addr_t cfg_desc_handle; // configuration descriptor dma address dma_addr_t cfg_stream_handle; // configuration bitstream dma address + dma_addr_t cfg_dec_size; + void *cfg_dec_vaddr; + dma_addr_t cfg_dec_daddr; }; struct mxc_jpeg_dev {