Hi Xia, On Fri, Apr 03, 2020 at 05:40:25PM +0800, Xia Jiang wrote: > Cancel reset hw operation in suspend and resume function because this > will be done in device_run().
This and... > Add spin_lock and unlock operation in irq and resume function to make > sure that the current frame is processed completely before suspend. ...this are two separate changes. Please split. > > Signed-off-by: Xia Jiang <[email protected]> > --- > drivers/media/platform/mtk-jpeg/mtk_jpeg_core.c | 11 +++++++++-- > 1 file changed, 9 insertions(+), 2 deletions(-) > > diff --git a/drivers/media/platform/mtk-jpeg/mtk_jpeg_core.c > b/drivers/media/platform/mtk-jpeg/mtk_jpeg_core.c > index dd5cadd101ef..2fa3711fdc9b 100644 > --- a/drivers/media/platform/mtk-jpeg/mtk_jpeg_core.c > +++ b/drivers/media/platform/mtk-jpeg/mtk_jpeg_core.c > @@ -911,6 +911,8 @@ static irqreturn_t mtk_jpeg_dec_irq(int irq, void *priv) > u32 dec_ret; > int i; > > + spin_lock(&jpeg->hw_lock); > + nit: For consistency, it is recommended to always use the same, i.e. the strongest, spin_(un)lock_ primitives when operating on the same spinlock. In this case it would be the irqsave(restore) variants. > dec_ret = mtk_jpeg_dec_get_int_status(jpeg->dec_reg_base); > dec_irq_ret = mtk_jpeg_dec_enum_result(dec_ret); > ctx = v4l2_m2m_get_curr_priv(jpeg->m2m_dev); > @@ -941,6 +943,7 @@ static irqreturn_t mtk_jpeg_dec_irq(int irq, void *priv) > v4l2_m2m_buf_done(src_buf, buf_state); > v4l2_m2m_buf_done(dst_buf, buf_state); > v4l2_m2m_job_finish(jpeg->m2m_dev, ctx->fh.m2m_ctx); > + spin_unlock(&jpeg->hw_lock); > pm_runtime_put_sync(ctx->jpeg->dev); > return IRQ_HANDLED; > } > @@ -1191,7 +1194,6 @@ static __maybe_unused int mtk_jpeg_pm_suspend(struct > device *dev) > { > struct mtk_jpeg_dev *jpeg = dev_get_drvdata(dev); > > - mtk_jpeg_dec_reset(jpeg->dec_reg_base); > mtk_jpeg_clk_off(jpeg); > > return 0; > @@ -1202,19 +1204,24 @@ static __maybe_unused int mtk_jpeg_pm_resume(struct > device *dev) > struct mtk_jpeg_dev *jpeg = dev_get_drvdata(dev); > > mtk_jpeg_clk_on(jpeg); > - mtk_jpeg_dec_reset(jpeg->dec_reg_base); > > return 0; > } > > static __maybe_unused int mtk_jpeg_suspend(struct device *dev) > { > + struct mtk_jpeg_dev *jpeg = dev_get_drvdata(dev); > + unsigned long flags; > int ret; > > if (pm_runtime_suspended(dev)) > return 0; > > + spin_lock_irqsave(&jpeg->hw_lock, flags); What does this spinlock protect us from? I can see that it would prevent the interrupt handler from being called, but is it okay to suspend the system without handling the interrupt? > + > ret = mtk_jpeg_pm_suspend(dev); > + Looking at the implementation of mtk_jpeg_pm_suspend(), all it does is disabling the clock. How do we make sure that there is no frame currently being processed by the hardware? Best regards, Tomasz
