| Message ID | 20250328063056.762-4-ming.qian@oss.nxp.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | media: imx-jpeg: Fix some motion-jpeg decoding issues | expand |
On Fri, Mar 28, 2025 at 02:30:52PM +0800, 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. > > In other words, 2 frame done interrupts will be triggered, driver will > ignore the first interrupt, Does any field in linked descriptors to control if issue irq? Generally you needn't enable first descriptors's irq and only enable second one. > and wait for the second interrupt. > If the resolution is small, and the 2 interrupts may be too close, It also possible two irqs combine to 1 irqs. If I understand correct, your irq handle only handle one descriptors per irq. Another words, If second irq already happen just before 1, 1. dec_ret = readl(reg + MXC_SLOT_OFFSET(slot, SLOT_STATUS)); 2. writel(dec_ret, reg + MXC_SLOT_OFFSET(slot, SLOT_STATUS)); /* w1c */ Does your driver wait forever because no second irq happen? Frank > > when driver is handling the first interrupt, two frames are done, then > driver will fail to wait for the second interrupt. > > In such case, driver can check whether the decoding is still ongoing, > if not, just done the current decoding. > > Signed-off-by: Ming Qian <ming.qian@oss.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 >
Hi Frank, On 2025/3/28 22:45, Frank Li wrote: > On Fri, Mar 28, 2025 at 02:30:52PM +0800, 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. >> >> In other words, 2 frame done interrupts will be triggered, driver will >> ignore the first interrupt, > > Does any field in linked descriptors to control if issue irq? Generally > you needn't enable first descriptors's irq and only enable second one. > Unfortunately, we cannot configure interrupts for each descriptor. So we can't only enable the second irq. >> and wait for the second interrupt. >> If the resolution is small, and the 2 interrupts may be too close, > > It also possible two irqs combine to 1 irqs. If I understand correct, your > irq handle only handle one descriptors per irq. > > Another words, > > If second irq already happen just before 1, > > 1. dec_ret = readl(reg + MXC_SLOT_OFFSET(slot, SLOT_STATUS)); > 2. writel(dec_ret, reg + MXC_SLOT_OFFSET(slot, SLOT_STATUS)); /* w1c */ > > Does your driver wait forever because no second irq happen? > > Frank Before this patch, the answer is yes, the driver will wait 2 seconds until timeout. In fact, this is the problem that this patch wants to avoid. Now I think there are 3 cases for motion-jpeg decoding: 1. The second irq happens before the first irq status check, the on-going check help to hanle this case. 2. The second irq happens after the first irq status check, but before on-going check, this on-going check can help handle it, fisnish the current decoding and reset the jpeg decoder. 3. The second irq happens after the on-going check, this is the normal process before. No additional processing required. Thanks, Ming >> >> when driver is handling the first interrupt, two frames are done, then >> driver will fail to wait for the second interrupt. >> >> In such case, driver can check whether the decoding is still ongoing, >> if not, just done the current decoding. >> >> Signed-off-by: Ming Qian <ming.qian@oss.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 >>
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;