Message ID | 20220530074756.14585-1-ming.qian@nxp.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | [v2] media: imx-jpeg: Disable some unused interrupt | expand |
Hi Ming, On 30.05.2022 10:47, Ming Qian wrote: > The interrupt STMBUF_HALF may be triggered after frame done. > It may led to system hang if driver try to access the register after > power off. > And interrupt STMBUF_HALF and STMBUF_RTND have no other effect. > So disable them and the unused interrupts. > > Fixes: 2db16c6ed72ce ("media: imx-jpeg: Add V4L2 driver for i.MX8 JPEG Encoder/Decoder") > Signed-off-by: Ming Qian <ming.qian@nxp.com> > --- > v2 > - add Fixes tag > drivers/media/platform/nxp/imx-jpeg/mxc-jpeg-hw.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg-hw.c b/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg-hw.c > index c482228262a3..258fbee7ab66 100644 > --- a/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg-hw.c > +++ b/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg-hw.c > @@ -76,7 +76,7 @@ void print_wrapper_info(struct device *dev, void __iomem *reg) > > void mxc_jpeg_enable_irq(void __iomem *reg, int slot) > { > - writel(0xFFFFFFFF, reg + MXC_SLOT_OFFSET(slot, SLOT_IRQ_EN)); > + writel(0xF0C, reg + MXC_SLOT_OFFSET(slot, SLOT_IRQ_EN)); There is another way, less aggressive, to go around this, disable all the interrupts once FRMDONE is received (or some other error condition), interrupts will get re-enabled at the next device_run. I checked this works, in mxc_jpeg_dec_irq: buffers_done: writel(0x0, reg + MXC_SLOT_OFFSET(slot, SLOT_IRQ_EN)); Either way, I would also replace this: if (!ctx) { dev_err(dev, "Instance released before the end of transaction.\n"); /* soft reset only resets internal state, not registers */ mxc_jpeg_sw_reset(reg); /* clear all interrupts */ writel(0xFFFFFFFF, reg + MXC_SLOT_OFFSET(slot, SLOT_STATUS)); goto job_unlock; } With something like: BUG_ON(!ctx) The initial intent of this code was to cope with the same problem, STMBUF_HALF interrupt received after FRMDONE, which could not be cleared, but it was not done right, I can see the hang in some rare cases. We should not run into it anymore, with interrupts disabled, either the way you proposed, or mine. Regards, Mirela > } > > void mxc_jpeg_sw_reset(void __iomem *reg)
> From: Mirela Rabulea (OSS) <mirela.rabulea@oss.nxp.com> > Sent: 2022年6月6日 0:43 > To: Ming Qian <ming.qian@nxp.com>; mchehab@kernel.org; > hverkuil-cisco@xs4all.nl > Cc: shawnguo@kernel.org; s.hauer@pengutronix.de; kernel@pengutronix.de; > festevam@gmail.com; dl-linux-imx <linux-imx@nxp.com>; > linux-media@vger.kernel.org; linux-kernel@vger.kernel.org; > devicetree@vger.kernel.org; linux-arm-kernel@lists.infradead.org > Subject: Re: [PATCH v2] media: imx-jpeg: Disable some unused interrupt > > Hi Ming, > > On 30.05.2022 10:47, Ming Qian wrote: > > The interrupt STMBUF_HALF may be triggered after frame done. > > It may led to system hang if driver try to access the register after > > power off. > > And interrupt STMBUF_HALF and STMBUF_RTND have no other effect. > > So disable them and the unused interrupts. > > > > Fixes: 2db16c6ed72ce ("media: imx-jpeg: Add V4L2 driver for i.MX8 JPEG > > Encoder/Decoder") > > Signed-off-by: Ming Qian <ming.qian@nxp.com> > > --- > > v2 > > - add Fixes tag > > drivers/media/platform/nxp/imx-jpeg/mxc-jpeg-hw.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg-hw.c > > b/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg-hw.c > > index c482228262a3..258fbee7ab66 100644 > > --- a/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg-hw.c > > +++ b/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg-hw.c > > @@ -76,7 +76,7 @@ void print_wrapper_info(struct device *dev, void > > __iomem *reg) > > > > void mxc_jpeg_enable_irq(void __iomem *reg, int slot) > > { > > - writel(0xFFFFFFFF, reg + MXC_SLOT_OFFSET(slot, SLOT_IRQ_EN)); > > + writel(0xF0C, reg + MXC_SLOT_OFFSET(slot, SLOT_IRQ_EN)); > > There is another way, less aggressive, to go around this, disable all the > interrupts once FRMDONE is received (or some other error condition), > interrupts will get re-enabled at the next device_run. I checked this works, in > mxc_jpeg_dec_irq: > buffers_done: > writel(0x0, reg + MXC_SLOT_OFFSET(slot, SLOT_IRQ_EN)); > Hi Mirela, Yes, I think it should work if we disable slot interrupt when frame done. And which solution do you prefer? Ming > Either way, I would also replace this: > if (!ctx) { > dev_err(dev, > "Instance released before the end of transaction.\n"); > /* soft reset only resets internal state, not registers */ > mxc_jpeg_sw_reset(reg); > /* clear all interrupts */ > writel(0xFFFFFFFF, reg + MXC_SLOT_OFFSET(slot, SLOT_STATUS)); > goto job_unlock; > } > > With something like: > BUG_ON(!ctx) > > The initial intent of this code was to cope with the same problem, > STMBUF_HALF interrupt received after FRMDONE, which could not be cleared, > but it was not done right, I can see the hang in some rare cases. We should not > run into it anymore, with interrupts disabled, either the way you proposed, or > mine. > > Regards, > Mirela > > > } > > > > void mxc_jpeg_sw_reset(void __iomem *reg)
On 06.06.2022 05:33, Ming Qian wrote: >> From: Mirela Rabulea (OSS) <mirela.rabulea@oss.nxp.com> >> Sent: 2022年6月6日 0:43 >> To: Ming Qian <ming.qian@nxp.com>; mchehab@kernel.org; >> hverkuil-cisco@xs4all.nl >> Cc: shawnguo@kernel.org; s.hauer@pengutronix.de; kernel@pengutronix.de; >> festevam@gmail.com; dl-linux-imx <linux-imx@nxp.com>; >> linux-media@vger.kernel.org; linux-kernel@vger.kernel.org; >> devicetree@vger.kernel.org; linux-arm-kernel@lists.infradead.org >> Subject: Re: [PATCH v2] media: imx-jpeg: Disable some unused interrupt >> >> Hi Ming, >> >> On 30.05.2022 10:47, Ming Qian wrote: >>> The interrupt STMBUF_HALF may be triggered after frame done. >>> It may led to system hang if driver try to access the register after >>> power off. >>> And interrupt STMBUF_HALF and STMBUF_RTND have no other effect. >>> So disable them and the unused interrupts. >>> >>> Fixes: 2db16c6ed72ce ("media: imx-jpeg: Add V4L2 driver for i.MX8 JPEG >>> Encoder/Decoder") >>> Signed-off-by: Ming Qian <ming.qian@nxp.com> >>> --- >>> v2 >>> - add Fixes tag >>> drivers/media/platform/nxp/imx-jpeg/mxc-jpeg-hw.c | 2 +- >>> 1 file changed, 1 insertion(+), 1 deletion(-) >>> >>> diff --git a/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg-hw.c >>> b/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg-hw.c >>> index c482228262a3..258fbee7ab66 100644 >>> --- a/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg-hw.c >>> +++ b/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg-hw.c >>> @@ -76,7 +76,7 @@ void print_wrapper_info(struct device *dev, void >>> __iomem *reg) >>> >>> void mxc_jpeg_enable_irq(void __iomem *reg, int slot) >>> { >>> - writel(0xFFFFFFFF, reg + MXC_SLOT_OFFSET(slot, SLOT_IRQ_EN)); >>> + writel(0xF0C, reg + MXC_SLOT_OFFSET(slot, SLOT_IRQ_EN)); >> >> There is another way, less aggressive, to go around this, disable all the >> interrupts once FRMDONE is received (or some other error condition), >> interrupts will get re-enabled at the next device_run. I checked this works, in >> mxc_jpeg_dec_irq: >> buffers_done: >> writel(0x0, reg + MXC_SLOT_OFFSET(slot, SLOT_IRQ_EN)); >> > > Hi Mirela, > > Yes, I think it should work if we disable slot interrupt when frame done. > And which solution do you prefer? Hi Ming, I don't have a strong preference, but just in case we decide to later use some of these interrupts, I would suggest to go with the disabling after FRMDONE. Please do not forget to clean the other code related to "Instance released before the end of transaction". Thanks, Mirela > > Ming > >> Either way, I would also replace this: >> if (!ctx) { >> dev_err(dev, >> "Instance released before the end of transaction.\n"); >> /* soft reset only resets internal state, not registers */ >> mxc_jpeg_sw_reset(reg); >> /* clear all interrupts */ >> writel(0xFFFFFFFF, reg + MXC_SLOT_OFFSET(slot, SLOT_STATUS)); >> goto job_unlock; >> } >> >> With something like: >> BUG_ON(!ctx) >> >> The initial intent of this code was to cope with the same problem, >> STMBUF_HALF interrupt received after FRMDONE, which could not be cleared, >> but it was not done right, I can see the hang in some rare cases. We should not >> run into it anymore, with interrupts disabled, either the way you proposed, or >> mine. >> >> Regards, >> Mirela >> >>> } >>> >>> void mxc_jpeg_sw_reset(void __iomem *reg)
> From: Mirela Rabulea (OSS) <mirela.rabulea@oss.nxp.com> > Sent: 2022年6月6日 17:41 > To: Ming Qian <ming.qian@nxp.com>; mchehab@kernel.org; > hverkuil-cisco@xs4all.nl > Cc: shawnguo@kernel.org; s.hauer@pengutronix.de; kernel@pengutronix.de; > festevam@gmail.com; dl-linux-imx <linux-imx@nxp.com>; > linux-media@vger.kernel.org; linux-kernel@vger.kernel.org; > devicetree@vger.kernel.org; linux-arm-kernel@lists.infradead.org > Subject: Re: [PATCH v2] media: imx-jpeg: Disable some unused interrupt > > > > On 06.06.2022 05:33, Ming Qian wrote: > >> From: Mirela Rabulea (OSS) <mirela.rabulea@oss.nxp.com> > >> Sent: 2022年6月6日 0:43 > >> To: Ming Qian <ming.qian@nxp.com>; mchehab@kernel.org; > >> hverkuil-cisco@xs4all.nl > >> Cc: shawnguo@kernel.org; s.hauer@pengutronix.de; > >> kernel@pengutronix.de; festevam@gmail.com; dl-linux-imx > >> <linux-imx@nxp.com>; linux-media@vger.kernel.org; > >> linux-kernel@vger.kernel.org; devicetree@vger.kernel.org; > >> linux-arm-kernel@lists.infradead.org > >> Subject: Re: [PATCH v2] media: imx-jpeg: Disable some unused > >> interrupt > >> > >> Hi Ming, > >> > >> On 30.05.2022 10:47, Ming Qian wrote: > >>> The interrupt STMBUF_HALF may be triggered after frame done. > >>> It may led to system hang if driver try to access the register after > >>> power off. > >>> And interrupt STMBUF_HALF and STMBUF_RTND have no other effect. > >>> So disable them and the unused interrupts. > >>> > >>> Fixes: 2db16c6ed72ce ("media: imx-jpeg: Add V4L2 driver for i.MX8 > >>> JPEG > >>> Encoder/Decoder") > >>> Signed-off-by: Ming Qian <ming.qian@nxp.com> > >>> --- > >>> v2 > >>> - add Fixes tag > >>> drivers/media/platform/nxp/imx-jpeg/mxc-jpeg-hw.c | 2 +- > >>> 1 file changed, 1 insertion(+), 1 deletion(-) > >>> > >>> diff --git a/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg-hw.c > >>> b/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg-hw.c > >>> index c482228262a3..258fbee7ab66 100644 > >>> --- a/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg-hw.c > >>> +++ b/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg-hw.c > >>> @@ -76,7 +76,7 @@ void print_wrapper_info(struct device *dev, void > >>> __iomem *reg) > >>> > >>> void mxc_jpeg_enable_irq(void __iomem *reg, int slot) > >>> { > >>> - writel(0xFFFFFFFF, reg + MXC_SLOT_OFFSET(slot, SLOT_IRQ_EN)); > >>> + writel(0xF0C, reg + MXC_SLOT_OFFSET(slot, SLOT_IRQ_EN)); > >> > >> There is another way, less aggressive, to go around this, disable all > >> the interrupts once FRMDONE is received (or some other error > >> condition), interrupts will get re-enabled at the next device_run. I > >> checked this works, in > >> mxc_jpeg_dec_irq: > >> buffers_done: > >> writel(0x0, reg + MXC_SLOT_OFFSET(slot, SLOT_IRQ_EN)); > >> > > > > Hi Mirela, > > > > Yes, I think it should work if we disable slot interrupt when frame done. > > And which solution do you prefer? > > Hi Ming, I don't have a strong preference, but just in case we decide to later > use some of these interrupts, I would suggest to go with the disabling after > FRMDONE. > > Please do not forget to clean the other code related to "Instance released > before the end of transaction". > > Thanks, > Mirela > OK, I'll make another patch that disable slot interrupt when frame done, and clean the code. > > > > Ming > > > >> Either way, I would also replace this: > >> if (!ctx) { > >> dev_err(dev, > >> "Instance released before the end of transaction.\n"); > >> /* soft reset only resets internal state, not registers */ > >> mxc_jpeg_sw_reset(reg); > >> /* clear all interrupts */ > >> writel(0xFFFFFFFF, reg + MXC_SLOT_OFFSET(slot, SLOT_STATUS)); > >> goto job_unlock; > >> } > >> > >> With something like: > >> BUG_ON(!ctx) > >> > >> The initial intent of this code was to cope with the same problem, > >> STMBUF_HALF interrupt received after FRMDONE, which could not be > >> cleared, but it was not done right, I can see the hang in some rare > >> cases. We should not run into it anymore, with interrupts disabled, > >> either the way you proposed, or mine. > >> > >> Regards, > >> Mirela > >> > >>> } > >>> > >>> void mxc_jpeg_sw_reset(void __iomem *reg)
This patch can be superseded now. It was replaced by another patch https://patchwork.linuxtv.org/project/linux-media/patch/20220607072315.23209-1-ming.qian@nxp.com/ > From: Ming Qian > Sent: 2022年6月7日 9:49 > To: Mirela Rabulea (OSS) <mirela.rabulea@oss.nxp.com>; > mchehab@kernel.org; hverkuil-cisco@xs4all.nl > Cc: shawnguo@kernel.org; s.hauer@pengutronix.de; kernel@pengutronix.de; > festevam@gmail.com; dl-linux-imx <linux-imx@nxp.com>; > linux-media@vger.kernel.org; linux-kernel@vger.kernel.org; > devicetree@vger.kernel.org; linux-arm-kernel@lists.infradead.org > Subject: RE: [PATCH v2] media: imx-jpeg: Disable some unused interrupt > > > From: Mirela Rabulea (OSS) <mirela.rabulea@oss.nxp.com> > > Sent: 2022年6月6日 17:41 > > To: Ming Qian <ming.qian@nxp.com>; mchehab@kernel.org; > > hverkuil-cisco@xs4all.nl > > Cc: shawnguo@kernel.org; s.hauer@pengutronix.de; > > kernel@pengutronix.de; festevam@gmail.com; dl-linux-imx > > <linux-imx@nxp.com>; linux-media@vger.kernel.org; > > linux-kernel@vger.kernel.org; devicetree@vger.kernel.org; > > linux-arm-kernel@lists.infradead.org > > Subject: Re: [PATCH v2] media: imx-jpeg: Disable some unused interrupt > > > > > > > > On 06.06.2022 05:33, Ming Qian wrote: > > >> From: Mirela Rabulea (OSS) <mirela.rabulea@oss.nxp.com> > > >> Sent: 2022年6月6日 0:43 > > >> To: Ming Qian <ming.qian@nxp.com>; mchehab@kernel.org; > > >> hverkuil-cisco@xs4all.nl > > >> Cc: shawnguo@kernel.org; s.hauer@pengutronix.de; > > >> kernel@pengutronix.de; festevam@gmail.com; dl-linux-imx > > >> <linux-imx@nxp.com>; linux-media@vger.kernel.org; > > >> linux-kernel@vger.kernel.org; devicetree@vger.kernel.org; > > >> linux-arm-kernel@lists.infradead.org > > >> Subject: Re: [PATCH v2] media: imx-jpeg: Disable some unused > > >> interrupt > > >> > > >> Hi Ming, > > >> > > >> On 30.05.2022 10:47, Ming Qian wrote: > > >>> The interrupt STMBUF_HALF may be triggered after frame done. > > >>> It may led to system hang if driver try to access the register > > >>> after power off. > > >>> And interrupt STMBUF_HALF and STMBUF_RTND have no other effect. > > >>> So disable them and the unused interrupts. > > >>> > > >>> Fixes: 2db16c6ed72ce ("media: imx-jpeg: Add V4L2 driver for i.MX8 > > >>> JPEG > > >>> Encoder/Decoder") > > >>> Signed-off-by: Ming Qian <ming.qian@nxp.com> > > >>> --- > > >>> v2 > > >>> - add Fixes tag > > >>> drivers/media/platform/nxp/imx-jpeg/mxc-jpeg-hw.c | 2 +- > > >>> 1 file changed, 1 insertion(+), 1 deletion(-) > > >>> > > >>> diff --git a/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg-hw.c > > >>> b/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg-hw.c > > >>> index c482228262a3..258fbee7ab66 100644 > > >>> --- a/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg-hw.c > > >>> +++ b/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg-hw.c > > >>> @@ -76,7 +76,7 @@ void print_wrapper_info(struct device *dev, void > > >>> __iomem *reg) > > >>> > > >>> void mxc_jpeg_enable_irq(void __iomem *reg, int slot) > > >>> { > > >>> - writel(0xFFFFFFFF, reg + MXC_SLOT_OFFSET(slot, SLOT_IRQ_EN)); > > >>> + writel(0xF0C, reg + MXC_SLOT_OFFSET(slot, SLOT_IRQ_EN)); > > >> > > >> There is another way, less aggressive, to go around this, disable > > >> all the interrupts once FRMDONE is received (or some other error > > >> condition), interrupts will get re-enabled at the next device_run. > > >> I checked this works, in > > >> mxc_jpeg_dec_irq: > > >> buffers_done: > > >> writel(0x0, reg + MXC_SLOT_OFFSET(slot, SLOT_IRQ_EN)); > > >> > > > > > > Hi Mirela, > > > > > > Yes, I think it should work if we disable slot interrupt when frame done. > > > And which solution do you prefer? > > > > Hi Ming, I don't have a strong preference, but just in case we decide > > to later use some of these interrupts, I would suggest to go with the > > disabling after FRMDONE. > > > > Please do not forget to clean the other code related to "Instance > > released before the end of transaction". > > > > Thanks, > > Mirela > > > > OK, I'll make another patch that disable slot interrupt when frame done, and > clean the code. > > > > > > > Ming > > > > > >> Either way, I would also replace this: > > >> if (!ctx) { > > >> dev_err(dev, > > >> "Instance released before the end of transaction.\n"); > > >> /* soft reset only resets internal state, not registers */ > > >> mxc_jpeg_sw_reset(reg); > > >> /* clear all interrupts */ > > >> writel(0xFFFFFFFF, reg + MXC_SLOT_OFFSET(slot, > SLOT_STATUS)); > > >> goto job_unlock; > > >> } > > >> > > >> With something like: > > >> BUG_ON(!ctx) > > >> > > >> The initial intent of this code was to cope with the same problem, > > >> STMBUF_HALF interrupt received after FRMDONE, which could not be > > >> cleared, but it was not done right, I can see the hang in some rare > > >> cases. We should not run into it anymore, with interrupts disabled, > > >> either the way you proposed, or mine. > > >> > > >> Regards, > > >> Mirela > > >> > > >>> } > > >>> > > >>> void mxc_jpeg_sw_reset(void __iomem *reg)
diff --git a/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg-hw.c b/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg-hw.c index c482228262a3..258fbee7ab66 100644 --- a/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg-hw.c +++ b/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg-hw.c @@ -76,7 +76,7 @@ void print_wrapper_info(struct device *dev, void __iomem *reg) void mxc_jpeg_enable_irq(void __iomem *reg, int slot) { - writel(0xFFFFFFFF, reg + MXC_SLOT_OFFSET(slot, SLOT_IRQ_EN)); + writel(0xF0C, reg + MXC_SLOT_OFFSET(slot, SLOT_IRQ_EN)); } void mxc_jpeg_sw_reset(void __iomem *reg)
The interrupt STMBUF_HALF may be triggered after frame done. It may led to system hang if driver try to access the register after power off. And interrupt STMBUF_HALF and STMBUF_RTND have no other effect. So disable them and the unused interrupts. Fixes: 2db16c6ed72ce ("media: imx-jpeg: Add V4L2 driver for i.MX8 JPEG Encoder/Decoder") Signed-off-by: Ming Qian <ming.qian@nxp.com> --- v2 - add Fixes tag drivers/media/platform/nxp/imx-jpeg/mxc-jpeg-hw.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)