| 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 >>
On Mon, Mar 31, 2025 at 11:10:20AM +0800, Ming Qian(OSS) wrote: > > 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. Okay, not sure if hardware provide current_descript position. Generally descriptor queue irq handle is like cur = queue_header; while(cur != read_hardware_currunt_pos()) { handle(cur); cur = cur->next; queue_header = cur; } with above logic, even you queue new request during irq handler, it should work correctly. But it is depend on if hardware can indicate current working queue position, some time, hardware stop last queue posistion when handle last one. Frank > > 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 > > >
Hi Frank, On 2025/3/31 22:32, Frank Li wrote: > On Mon, Mar 31, 2025 at 11:10:20AM +0800, Ming Qian(OSS) wrote: >> >> 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. > > Okay, not sure if hardware provide current_descript position. Generally > descriptor queue irq handle is like > > cur = queue_header; > while(cur != read_hardware_currunt_pos()) > { > handle(cur); > cur = cur->next; > queue_header = cur; > } > > with above logic, even you queue new request during irq handler, it should > work correctly. > > But it is depend on if hardware can indicate current working queue > position, some time, hardware stop last queue posistion when handle last > one. > > Frank > I think it doesn't matter, the 2 descriptors are the cfg descriptor and then the image descriptor. If the current descriptor register remains the last image descriptor, the ongoing check works. And I guess your concern is as below. If the current descriptor register is still the cfg descriptor, but the hardware has finished decoding the next image descriptor. I confirmed with our hardware engineer. This can't happen. The first cfg decriptor has a next_descpt_ptr that is pointing to the image descriptor, when the hardware read tne next_descpt_ptr, the current descriptor register is updated, before the actual decoding. Thanks, Ming >> >> 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 >>>>
On Tue, Apr 01, 2025 at 11:17:36AM +0800, Ming Qian(OSS) wrote: > > Hi Frank, > > On 2025/3/31 22:32, Frank Li wrote: > > On Mon, Mar 31, 2025 at 11:10:20AM +0800, Ming Qian(OSS) wrote: > > > > > > 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. > > > > Okay, not sure if hardware provide current_descript position. Generally > > descriptor queue irq handle is like > > > > cur = queue_header; > > while(cur != read_hardware_currunt_pos()) > > { > > handle(cur); > > cur = cur->next; > > queue_header = cur; > > } > > > > with above logic, even you queue new request during irq handler, it should > > work correctly. > > > > But it is depend on if hardware can indicate current working queue > > position, some time, hardware stop last queue posistion when handle last > > one. > > > > Frank > > > > I think it doesn't matter, the 2 descriptors are the cfg descriptor and > then the image descriptor. > If the current descriptor register remains the last image descriptor, > the ongoing check works. > > And I guess your concern is as below. > If the current descriptor register is still the cfg descriptor, but the > hardware has finished decoding the next image descriptor. > > I confirmed with our hardware engineer. This can't happen. > The first cfg decriptor has a next_descpt_ptr that is pointing to the > image descriptor, when the hardware read tne next_descpt_ptr, the > current descriptor register is updated, before the actual decoding. Maybe off topic, CFG->next = Image Image->next = NULL; If hardware finish image descriptior, current descriptor is 'Image' or 'NULL' If it is 'Image', need extra status bit show 'done' 1: slot_status = readl(jpeg->base_reg + MXC_SLOT_OFFSET(ctx->slot, SLOT_STATUS)); I suppose it should be DONE status if just finish CFG description. 2: curr_desc = readl(jpeg->base_reg + MXC_SLOT_OFFSET(ctx->slot, SLOT_CUR_DESCPT_PTR)); It is possible curr_desc already was 'Image' after 1. if (curr_desc == jpeg->slot_data.cfg_desc_handle) //not hit this return true; if (slot_status & SLOT_STATUS_ONGOING) // not hit this return true; fake false may return. check two aync condition "slot_status" and "curr_desc" always be risk. But I don't know what's happen if fake false return here. for this type check do { slot_status = readl(); curr_desc = readl(); } while (slot_status != read()); to make sure slot_status and cur_desc indicate the hardware status correctly. Frank > > Thanks, > Ming > > > > > > > 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;