lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  linux-cve-announce  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <f488bfca-f50a-4eb4-afea-babcd114fc35@oss.nxp.com>
Date: Mon, 7 Apr 2025 10:54:31 +0800
From: "Ming Qian(OSS)" <ming.qian@....nxp.com>
To: Sebastian Fricke <sebastian.fricke@...labora.com>
Cc: mchehab@...nel.org, hverkuil-cisco@...all.nl, mirela.rabulea@....nxp.com,
 shawnguo@...nel.org, s.hauer@...gutronix.de, kernel@...gutronix.de,
 festevam@...il.com, xiahong.bao@....com, eagle.zhou@....com,
 linux-imx@....com, imx@...ts.linux.dev, linux-media@...r.kernel.org,
 linux-kernel@...r.kernel.org, linux-arm-kernel@...ts.infradead.org
Subject: Re: [PATCH v2 2/3] media: imx-jpeg: Change the pattern size to 128x64


Hi Sebastian,

On 2025/4/5 23:26, Sebastian Fricke wrote:
> Hey Ming,
> 
> On 28.03.2025 14:30, ming.qian@....nxp.com wrote:
>> From: Ming Qian <ming.qian@....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
> 

You're right, and your description is clearer, I'll apply it.
Thanks very much.

Regards,
Ming

>>
>> Signed-off-by: Ming Qian <ming.qian@....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
>>
>>

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ