Messages in this thread | | | From | Ming Qian <> | Subject | RE: [PATCH v2] media: imx-jpeg: Disable some unused interrupt | Date | Tue, 7 Jun 2022 01:49:15 +0000 |
| |
> 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)
| |