Message-ID: <e4011f8c-2221-4350-b14f-52b5bb9c49f6@oss.nxp.com>
Date: Mon, 7 Apr 2025 13:14:34 +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 3/3] media: imx-jpeg: Check decoding is ongoing for
 motion-jpeg


Hi Sebastian,

On 2025/4/5 23:37, 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.
> 
> Hmm do we need to repeat the description from the previous patch?

This issue is also caused by the repeat mode, I thought I should explain
it, but as you said, this does make it redundant.

>>
>> In other words, 2 frame done interrupts will be triggered, driver will
>> ignore the first interrupt, and wait for the second interrupt.
>> If the resolution is small, and the 2 interrupts may be too close,
>> when driver is handling the first interrupt, two frames are done, then
>> driver will fail to wait for the second interrupt.
> 
> Okay this first part is a bit hard to understand, how about:
> 
> As the first frame in "repeat-mode" is the pattern, the first interrupt
> is ignored by the driver. With small resolution bitstreams, the
> interrupts might fire too quickly and thus the driver might miss the
> second interrupt from the first actual frame.
> 
> Is that what you mean?
Yes, you're right, and it's better now.

> 
>>
>> In such case, driver can check whether the decoding is still ongoing,
>> if not, just done the current decoding.
> 
> That doesn't answer to me why this solves the issue of missing the
> second interrupt, can you elaborate your solution a bit so that the
> reader of the commit description understands why this is needed?
> 

When the first frame-done interrupt is received, we need to figure out
if we can get the second interrupt.
So we check the curr_desc register first,
if it is still pointing to the pattern descripor, the second actual
frame is not started, we can wait for its frame-doen interrupt.
if the curr_desc has pointed to the frame descriptor, then we check the
ongoing bit of slot_status register.
if the ongoing bit is set to 1, the decoding of the actual frame is not
finished, we can wait for its frame-doen interrupt.
if the ongoing bit is set o 0, the decoding of the actual frame is
finished, we can't wait for the second interrupt, but mark it as done.

But there is still a small problem, that the curr_desc and slot_status
registers are not synchronous. curr_desc is updated when the
next_descpt_ptr is loaded, but the ongoing bit of slot_status is set
after the 32 bytes descriptor is loaded, there will be a short time
interval in between, which may cause fake faluse. Reading slot_status
twice can basically reduce the probability of fake false to 0.

Regards,
Ming

> Regards,
> Sebastian
> 
>>
>> Signed-off-by: Ming Qian <ming.qian@....nxp.com>
>> ---
>> .../media/platform/nxp/imx-jpeg/mxc-jpeg-hw.h |  1 +
>> .../media/platform/nxp/imx-jpeg/mxc-jpeg.c    | 20 ++++++++++++++++++-
>> 2 files changed, 20 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg-hw.h 
>> b/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg-hw.h
>> index d579c804b047..adb93e977be9 100644
>> --- a/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg-hw.h
>> +++ b/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg-hw.h
>> @@ -89,6 +89,7 @@
>> /* SLOT_STATUS fields for slots 0..3 */
>> #define SLOT_STATUS_FRMDONE            (0x1 << 3)
>> #define SLOT_STATUS_ENC_CONFIG_ERR        (0x1 << 8)
>> +#define SLOT_STATUS_ONGOING            (0x1 << 31)
>>
>> /* SLOT_IRQ_EN fields TBD */
>>
>> diff --git a/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.c 
>> b/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.c
>> index 45705c606769..e6bb45633a19 100644
>> --- a/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.c
>> +++ b/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.c
>> @@ -910,6 +910,23 @@ static u32 mxc_jpeg_get_plane_size(struct 
>> mxc_jpeg_q_data *q_data, u32 plane_no)
>>     return size;
>> }
>>
>> +static bool mxc_dec_is_ongoing(struct mxc_jpeg_ctx *ctx)
>> +{
>> +    struct mxc_jpeg_dev *jpeg = ctx->mxc_jpeg;
>> +    u32 curr_desc;
>> +    u32 slot_status;
>> +
>> +    slot_status = readl(jpeg->base_reg + MXC_SLOT_OFFSET(ctx->slot, 
>> SLOT_STATUS));
>> +    curr_desc = readl(jpeg->base_reg + MXC_SLOT_OFFSET(ctx->slot, 
>> SLOT_CUR_DESCPT_PTR));
>> +
>> +    if (curr_desc == jpeg->slot_data.cfg_desc_handle)
>> +        return true;
>> +    if (slot_status & SLOT_STATUS_ONGOING)
>> +        return true;
>> +
>> +    return false;
>> +}
>> +
>> static irqreturn_t mxc_jpeg_dec_irq(int irq, void *priv)
>> {
>>     struct mxc_jpeg_dev *jpeg = priv;
>> @@ -979,7 +996,8 @@ static irqreturn_t mxc_jpeg_dec_irq(int irq, void 
>> *priv)
>>         mxc_jpeg_enc_mode_go(dev, reg, 
>> mxc_jpeg_is_extended_sequential(q_data->fmt));
>>         goto job_unlock;
>>     }
>> -    if (jpeg->mode == MXC_JPEG_DECODE && jpeg_src_buf->dht_needed) {
>> +    if (jpeg->mode == MXC_JPEG_DECODE && jpeg_src_buf->dht_needed &&
>> +        mxc_dec_is_ongoing(ctx)) {
>>         jpeg_src_buf->dht_needed = false;
>>         dev_dbg(dev, "Decoder DHT cfg finished. Start decoding...\n");
>>         goto job_unlock;
>> -- 
>> 2.43.0-rc1
>>
>>

Powered by blists - more mailing lists