| Message ID | 1638501230-13417-4-git-send-email-kyrie.wu@mediatek.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Support multi-hardware jpeg encoding using of_platform_populate | expand |
kyrie.wu wrote: > Generalizes jpegenc timeout func interfaces to handle HW timeout. Where do you call schedule_delayed_work() and init hw_param? > > Signed-off-by: kyrie.wu <kyrie.wu@mediatek.com> > --- > V6: no change > --- > drivers/media/platform/mtk-jpeg/mtk_jpeg_core.h | 8 ++++++++ > drivers/media/platform/mtk-jpeg/mtk_jpeg_enc_hw.c | 23 +++++++++++++++++++++++ > 2 files changed, 31 insertions(+) > > diff --git a/drivers/media/platform/mtk-jpeg/mtk_jpeg_core.h b/drivers/media/platform/mtk-jpeg/mtk_jpeg_core.h > index 7d013de..baab305 100644 > --- a/drivers/media/platform/mtk-jpeg/mtk_jpeg_core.h > +++ b/drivers/media/platform/mtk-jpeg/mtk_jpeg_core.h > @@ -75,6 +75,12 @@ struct mtk_jpeg_variant { > u32 cap_q_default_fourcc; > }; > > +struct mtk_jpeg_hw_param { > + struct vb2_v4l2_buffer *src_buffer; > + struct vb2_v4l2_buffer *dst_buffer; > + struct mtk_jpeg_ctx *curr_ctx; > +}; > + > enum mtk_jpegenc_hw_id { > MTK_JPEGENC_HW0, > MTK_JPEGENC_HW1, > @@ -122,6 +128,8 @@ struct mtk_jpegenc_comp_dev { > struct mtk_jpeg_dev *master_dev; > struct mtk_jpegenc_pm pm; > int jpegenc_irq; > + struct delayed_work job_timeout_work; > + struct mtk_jpeg_hw_param hw_param; > }; > > /** > diff --git a/drivers/media/platform/mtk-jpeg/mtk_jpeg_enc_hw.c b/drivers/media/platform/mtk-jpeg/mtk_jpeg_enc_hw.c > index 4ccda1d..e62b875 100644 > --- a/drivers/media/platform/mtk-jpeg/mtk_jpeg_enc_hw.c > +++ b/drivers/media/platform/mtk-jpeg/mtk_jpeg_enc_hw.c > @@ -183,6 +183,24 @@ void mtk_jpeg_set_enc_params(struct mtk_jpeg_ctx *ctx, void __iomem *base) > writel(ctx->restart_interval, base + JPEG_ENC_RST_MCU_NUM); > } > > +static void mtk_jpegenc_timeout_work(struct work_struct *work) > +{ > + struct delayed_work *Pwork = > + container_of(work, struct delayed_work, work); Please use to_delayed_work() > + struct mtk_jpegenc_comp_dev *cjpeg = > + container_of(Pwork, struct mtk_jpegenc_comp_dev, > + job_timeout_work); > + enum vb2_buffer_state buf_state = VB2_BUF_STATE_ERROR; > + struct vb2_v4l2_buffer *src_buf; > + > + src_buf = cjpeg->hw_param.src_buffer; > + > + mtk_jpeg_enc_reset(cjpeg->reg_base); > + clk_disable_unprepare(cjpeg->pm.venc_clk.clk_info->jpegenc_clk); > + pm_runtime_put(cjpeg->pm.dev); > + v4l2_m2m_buf_done(src_buf, buf_state); > +} > + > static irqreturn_t mtk_jpegenc_hw_irq_handler(int irq, void *priv) > { > struct vb2_v4l2_buffer *src_buf, *dst_buf; > @@ -194,6 +212,8 @@ static irqreturn_t mtk_jpegenc_hw_irq_handler(int irq, void *priv) > struct mtk_jpegenc_comp_dev *jpeg = priv; > struct mtk_jpeg_dev *master_jpeg = jpeg->master_dev; > > + cancel_delayed_work(&jpeg->job_timeout_work); Isn't here a race condition is the delayed work is being exectuted? > + > irq_status = readl(jpeg->reg_base + JPEG_ENC_INT_STS) & > JPEG_ENC_INT_STATUS_MASK_ALLIRQ; > if (irq_status) > @@ -322,6 +342,9 @@ static int mtk_jpegenc_hw_probe(struct platform_device *pdev) > > dev->plat_dev = pdev; > > + INIT_DELAYED_WORK(&dev->job_timeout_work, > + mtk_jpegenc_timeout_work); > + > ret = mtk_jpegenc_init_pm(dev); > if (ret) { > dev_err(&pdev->dev, "Failed to get jpeg enc clock source"); > -- > 2.6.4 > > > _______________________________________________ > Linux-mediatek mailing list > Linux-mediatek@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-mediatek >
On Fri, 2021-12-03 at 16:07 +0100, Ricardo Ribalda wrote: > kyrie.wu wrote: > > > Generalizes jpegenc timeout func interfaces to handle HW timeout. > > Where do you call schedule_delayed_work() and init hw_param? the work of call schedule_delayed_work() and init hw_param would execute in the function of mtk_jpegenc_worker in [PATCH V6, 4/5] > > > > Signed-off-by: kyrie.wu <kyrie.wu@mediatek.com> > > --- > > V6: no change > > --- > > drivers/media/platform/mtk-jpeg/mtk_jpeg_core.h | 8 ++++++++ > > drivers/media/platform/mtk-jpeg/mtk_jpeg_enc_hw.c | 23 > > +++++++++++++++++++++++ > > 2 files changed, 31 insertions(+) > > > > diff --git a/drivers/media/platform/mtk-jpeg/mtk_jpeg_core.h > > b/drivers/media/platform/mtk-jpeg/mtk_jpeg_core.h > > index 7d013de..baab305 100644 > > --- a/drivers/media/platform/mtk-jpeg/mtk_jpeg_core.h > > +++ b/drivers/media/platform/mtk-jpeg/mtk_jpeg_core.h > > @@ -75,6 +75,12 @@ struct mtk_jpeg_variant { > > u32 cap_q_default_fourcc; > > }; > > > > +struct mtk_jpeg_hw_param { > > + struct vb2_v4l2_buffer *src_buffer; > > + struct vb2_v4l2_buffer *dst_buffer; > > + struct mtk_jpeg_ctx *curr_ctx; > > +}; > > + > > enum mtk_jpegenc_hw_id { > > MTK_JPEGENC_HW0, > > MTK_JPEGENC_HW1, > > @@ -122,6 +128,8 @@ struct mtk_jpegenc_comp_dev { > > struct mtk_jpeg_dev *master_dev; > > struct mtk_jpegenc_pm pm; > > int jpegenc_irq; > > + struct delayed_work job_timeout_work; > > + struct mtk_jpeg_hw_param hw_param; > > }; > > > > /** > > diff --git a/drivers/media/platform/mtk-jpeg/mtk_jpeg_enc_hw.c > > b/drivers/media/platform/mtk-jpeg/mtk_jpeg_enc_hw.c > > index 4ccda1d..e62b875 100644 > > --- a/drivers/media/platform/mtk-jpeg/mtk_jpeg_enc_hw.c > > +++ b/drivers/media/platform/mtk-jpeg/mtk_jpeg_enc_hw.c > > @@ -183,6 +183,24 @@ void mtk_jpeg_set_enc_params(struct > > mtk_jpeg_ctx *ctx, void __iomem *base) > > writel(ctx->restart_interval, base + JPEG_ENC_RST_MCU_NUM); > > } > > > > +static void mtk_jpegenc_timeout_work(struct work_struct *work) > > +{ > > + struct delayed_work *Pwork = > > + container_of(work, struct delayed_work, work); > > Please use to_delayed_work() It would be fixed in the next code version. Thanks. > > + struct mtk_jpegenc_comp_dev *cjpeg = > > + container_of(Pwork, struct mtk_jpegenc_comp_dev, > > + job_timeout_work); > > + enum vb2_buffer_state buf_state = VB2_BUF_STATE_ERROR; > > + struct vb2_v4l2_buffer *src_buf; > > + > > + src_buf = cjpeg->hw_param.src_buffer; > > + > > + mtk_jpeg_enc_reset(cjpeg->reg_base); > > + clk_disable_unprepare(cjpeg->pm.venc_clk.clk_info- > > >jpegenc_clk); > > + pm_runtime_put(cjpeg->pm.dev); > > + v4l2_m2m_buf_done(src_buf, buf_state); > > +} > > + > > static irqreturn_t mtk_jpegenc_hw_irq_handler(int irq, void *priv) > > { > > struct vb2_v4l2_buffer *src_buf, *dst_buf; > > @@ -194,6 +212,8 @@ static irqreturn_t > > mtk_jpegenc_hw_irq_handler(int irq, void *priv) > > struct mtk_jpegenc_comp_dev *jpeg = priv; > > struct mtk_jpeg_dev *master_jpeg = jpeg->master_dev; > > > > + cancel_delayed_work(&jpeg->job_timeout_work); > > Isn't here a race condition is the delayed work is being exectuted? If hw worked beyond 1 second, the delayed work is called. This setting is shown in the function of mtk_jpegenc_worker in [PATCH V6, 4/5]. > > + > > irq_status = readl(jpeg->reg_base + JPEG_ENC_INT_STS) & > > JPEG_ENC_INT_STATUS_MASK_ALLIRQ; > > if (irq_status) > > @@ -322,6 +342,9 @@ static int mtk_jpegenc_hw_probe(struct > > platform_device *pdev) > > > > dev->plat_dev = pdev; > > > > + INIT_DELAYED_WORK(&dev->job_timeout_work, > > + mtk_jpegenc_timeout_work); > > + > > ret = mtk_jpegenc_init_pm(dev); > > if (ret) { > > dev_err(&pdev->dev, "Failed to get jpeg enc clock > > source"); > > -- > > 2.6.4 > > > > > > _______________________________________________ > > Linux-mediatek mailing list > > Linux-mediatek@lists.infradead.org > > http://lists.infradead.org/mailman/listinfo/linux-mediatek > >
Il 03/12/21 04:13, kyrie.wu ha scritto: > Generalizes jpegenc timeout func interfaces to handle HW timeout. > > Signed-off-by: kyrie.wu <kyrie.wu@mediatek.com> > --- > V6: no change > --- > drivers/media/platform/mtk-jpeg/mtk_jpeg_core.h | 8 ++++++++ > drivers/media/platform/mtk-jpeg/mtk_jpeg_enc_hw.c | 23 +++++++++++++++++++++++ > 2 files changed, 31 insertions(+) > > diff --git a/drivers/media/platform/mtk-jpeg/mtk_jpeg_core.h b/drivers/media/platform/mtk-jpeg/mtk_jpeg_core.h > index 7d013de..baab305 100644 > --- a/drivers/media/platform/mtk-jpeg/mtk_jpeg_core.h > +++ b/drivers/media/platform/mtk-jpeg/mtk_jpeg_core.h > @@ -75,6 +75,12 @@ struct mtk_jpeg_variant { > u32 cap_q_default_fourcc; > }; > > +struct mtk_jpeg_hw_param { > + struct vb2_v4l2_buffer *src_buffer; > + struct vb2_v4l2_buffer *dst_buffer; > + struct mtk_jpeg_ctx *curr_ctx; > +}; > + > enum mtk_jpegenc_hw_id { > MTK_JPEGENC_HW0, > MTK_JPEGENC_HW1, > @@ -122,6 +128,8 @@ struct mtk_jpegenc_comp_dev { > struct mtk_jpeg_dev *master_dev; > struct mtk_jpegenc_pm pm; > int jpegenc_irq; > + struct delayed_work job_timeout_work; > + struct mtk_jpeg_hw_param hw_param; > }; > > /** > diff --git a/drivers/media/platform/mtk-jpeg/mtk_jpeg_enc_hw.c b/drivers/media/platform/mtk-jpeg/mtk_jpeg_enc_hw.c > index 4ccda1d..e62b875 100644 > --- a/drivers/media/platform/mtk-jpeg/mtk_jpeg_enc_hw.c > +++ b/drivers/media/platform/mtk-jpeg/mtk_jpeg_enc_hw.c > @@ -183,6 +183,24 @@ void mtk_jpeg_set_enc_params(struct mtk_jpeg_ctx *ctx, void __iomem *base) > writel(ctx->restart_interval, base + JPEG_ENC_RST_MCU_NUM); > } > > +static void mtk_jpegenc_timeout_work(struct work_struct *work) > +{ > + struct delayed_work *Pwork = > + container_of(work, struct delayed_work, work); > + struct mtk_jpegenc_comp_dev *cjpeg = > + container_of(Pwork, struct mtk_jpegenc_comp_dev, > + job_timeout_work); > + enum vb2_buffer_state buf_state = VB2_BUF_STATE_ERROR; > + struct vb2_v4l2_buffer *src_buf; > + > + src_buf = cjpeg->hw_param.src_buffer; > + > + mtk_jpeg_enc_reset(cjpeg->reg_base); > + clk_disable_unprepare(cjpeg->pm.venc_clk.clk_info->jpegenc_clk); You disable and unprepare the clock, but you never turn it back on? This will lead to unbalanced disable on module removal but, more importantly, will lead to unclocked access after a timeout, for which the platform may or may not crash, but the jpgenc hardware will be unrecoverable until performing a full system reboot. Please fix this. > + pm_runtime_put(cjpeg->pm.dev); > + v4l2_m2m_buf_done(src_buf, buf_state); > +} > + > static irqreturn_t mtk_jpegenc_hw_irq_handler(int irq, void *priv) > { > struct vb2_v4l2_buffer *src_buf, *dst_buf; > @@ -194,6 +212,8 @@ static irqreturn_t mtk_jpegenc_hw_irq_handler(int irq, void *priv) > struct mtk_jpegenc_comp_dev *jpeg = priv; > struct mtk_jpeg_dev *master_jpeg = jpeg->master_dev; > > + cancel_delayed_work(&jpeg->job_timeout_work); > + > irq_status = readl(jpeg->reg_base + JPEG_ENC_INT_STS) & > JPEG_ENC_INT_STATUS_MASK_ALLIRQ; > if (irq_status) > @@ -322,6 +342,9 @@ static int mtk_jpegenc_hw_probe(struct platform_device *pdev) > > dev->plat_dev = pdev; > > + INIT_DELAYED_WORK(&dev->job_timeout_work, > + mtk_jpegenc_timeout_work); > + > ret = mtk_jpegenc_init_pm(dev); > if (ret) { > dev_err(&pdev->dev, "Failed to get jpeg enc clock source");
On Mon, 2022-02-07 at 15:50 +0100, AngeloGioacchino Del Regno wrote: > Il 03/12/21 04:13, kyrie.wu ha scritto: > > Generalizes jpegenc timeout func interfaces to handle HW timeout. > > > > Signed-off-by: kyrie.wu <kyrie.wu@mediatek.com> > > --- > > V6: no change > > --- > > drivers/media/platform/mtk-jpeg/mtk_jpeg_core.h | 8 ++++++++ > > drivers/media/platform/mtk-jpeg/mtk_jpeg_enc_hw.c | 23 > > +++++++++++++++++++++++ > > 2 files changed, 31 insertions(+) > > > > diff --git a/drivers/media/platform/mtk-jpeg/mtk_jpeg_core.h > > b/drivers/media/platform/mtk-jpeg/mtk_jpeg_core.h > > index 7d013de..baab305 100644 > > --- a/drivers/media/platform/mtk-jpeg/mtk_jpeg_core.h > > +++ b/drivers/media/platform/mtk-jpeg/mtk_jpeg_core.h > > @@ -75,6 +75,12 @@ struct mtk_jpeg_variant { > > u32 cap_q_default_fourcc; > > }; > > > > +struct mtk_jpeg_hw_param { > > + struct vb2_v4l2_buffer *src_buffer; > > + struct vb2_v4l2_buffer *dst_buffer; > > + struct mtk_jpeg_ctx *curr_ctx; > > +}; > > + > > enum mtk_jpegenc_hw_id { > > MTK_JPEGENC_HW0, > > MTK_JPEGENC_HW1, > > @@ -122,6 +128,8 @@ struct mtk_jpegenc_comp_dev { > > struct mtk_jpeg_dev *master_dev; > > struct mtk_jpegenc_pm pm; > > int jpegenc_irq; > > + struct delayed_work job_timeout_work; > > + struct mtk_jpeg_hw_param hw_param; > > }; > > > > /** > > diff --git a/drivers/media/platform/mtk-jpeg/mtk_jpeg_enc_hw.c > > b/drivers/media/platform/mtk-jpeg/mtk_jpeg_enc_hw.c > > index 4ccda1d..e62b875 100644 > > --- a/drivers/media/platform/mtk-jpeg/mtk_jpeg_enc_hw.c > > +++ b/drivers/media/platform/mtk-jpeg/mtk_jpeg_enc_hw.c > > @@ -183,6 +183,24 @@ void mtk_jpeg_set_enc_params(struct > > mtk_jpeg_ctx *ctx, void __iomem *base) > > writel(ctx->restart_interval, base + JPEG_ENC_RST_MCU_NUM); > > } > > > > +static void mtk_jpegenc_timeout_work(struct work_struct *work) > > +{ > > + struct delayed_work *Pwork = > > + container_of(work, struct delayed_work, work); > > + struct mtk_jpegenc_comp_dev *cjpeg = > > + container_of(Pwork, struct mtk_jpegenc_comp_dev, > > + job_timeout_work); > > + enum vb2_buffer_state buf_state = VB2_BUF_STATE_ERROR; > > + struct vb2_v4l2_buffer *src_buf; > > + > > + src_buf = cjpeg->hw_param.src_buffer; > > + > > + mtk_jpeg_enc_reset(cjpeg->reg_base); > > + clk_disable_unprepare(cjpeg->pm.venc_clk.clk_info- > > >jpegenc_clk); > > You disable and unprepare the clock, but you never turn it back on? > > This will lead to unbalanced disable on module removal but, more > importantly, > will lead to unclocked access after a timeout, for which the platform > may or > may not crash, but the jpgenc hardware will be unrecoverable until > performing > a full system reboot. > > Please fix this. Dear AngeloGioacchino, The colck enabled in the function of mtk_jpegenc_worker in [V6,4/5] media: mtk-jpegenc: add jpeg encode worker interface. And it disabled in mtk_jpegenc_timeout_work if encode error, or in mtk_jpegenc_hw_irq_handler if encode success. > > > + pm_runtime_put(cjpeg->pm.dev); > > + v4l2_m2m_buf_done(src_buf, buf_state); > > +} > > + > > static irqreturn_t mtk_jpegenc_hw_irq_handler(int irq, void > > *priv) > > { > > struct vb2_v4l2_buffer *src_buf, *dst_buf; > > @@ -194,6 +212,8 @@ static irqreturn_t > > mtk_jpegenc_hw_irq_handler(int irq, void *priv) > > struct mtk_jpegenc_comp_dev *jpeg = priv; > > struct mtk_jpeg_dev *master_jpeg = jpeg->master_dev; > > > > + cancel_delayed_work(&jpeg->job_timeout_work); > > + > > irq_status = readl(jpeg->reg_base + JPEG_ENC_INT_STS) & > > JPEG_ENC_INT_STATUS_MASK_ALLIRQ; > > if (irq_status) > > @@ -322,6 +342,9 @@ static int mtk_jpegenc_hw_probe(struct > > platform_device *pdev) > > > > dev->plat_dev = pdev; > > > > + INIT_DELAYED_WORK(&dev->job_timeout_work, > > + mtk_jpegenc_timeout_work); > > + > > ret = mtk_jpegenc_init_pm(dev); > > if (ret) { > > dev_err(&pdev->dev, "Failed to get jpeg enc clock > > source"); > >
diff --git a/drivers/media/platform/mtk-jpeg/mtk_jpeg_core.h b/drivers/media/platform/mtk-jpeg/mtk_jpeg_core.h index 7d013de..baab305 100644 --- a/drivers/media/platform/mtk-jpeg/mtk_jpeg_core.h +++ b/drivers/media/platform/mtk-jpeg/mtk_jpeg_core.h @@ -75,6 +75,12 @@ struct mtk_jpeg_variant { u32 cap_q_default_fourcc; }; +struct mtk_jpeg_hw_param { + struct vb2_v4l2_buffer *src_buffer; + struct vb2_v4l2_buffer *dst_buffer; + struct mtk_jpeg_ctx *curr_ctx; +}; + enum mtk_jpegenc_hw_id { MTK_JPEGENC_HW0, MTK_JPEGENC_HW1, @@ -122,6 +128,8 @@ struct mtk_jpegenc_comp_dev { struct mtk_jpeg_dev *master_dev; struct mtk_jpegenc_pm pm; int jpegenc_irq; + struct delayed_work job_timeout_work; + struct mtk_jpeg_hw_param hw_param; }; /** diff --git a/drivers/media/platform/mtk-jpeg/mtk_jpeg_enc_hw.c b/drivers/media/platform/mtk-jpeg/mtk_jpeg_enc_hw.c index 4ccda1d..e62b875 100644 --- a/drivers/media/platform/mtk-jpeg/mtk_jpeg_enc_hw.c +++ b/drivers/media/platform/mtk-jpeg/mtk_jpeg_enc_hw.c @@ -183,6 +183,24 @@ void mtk_jpeg_set_enc_params(struct mtk_jpeg_ctx *ctx, void __iomem *base) writel(ctx->restart_interval, base + JPEG_ENC_RST_MCU_NUM); } +static void mtk_jpegenc_timeout_work(struct work_struct *work) +{ + struct delayed_work *Pwork = + container_of(work, struct delayed_work, work); + struct mtk_jpegenc_comp_dev *cjpeg = + container_of(Pwork, struct mtk_jpegenc_comp_dev, + job_timeout_work); + enum vb2_buffer_state buf_state = VB2_BUF_STATE_ERROR; + struct vb2_v4l2_buffer *src_buf; + + src_buf = cjpeg->hw_param.src_buffer; + + mtk_jpeg_enc_reset(cjpeg->reg_base); + clk_disable_unprepare(cjpeg->pm.venc_clk.clk_info->jpegenc_clk); + pm_runtime_put(cjpeg->pm.dev); + v4l2_m2m_buf_done(src_buf, buf_state); +} + static irqreturn_t mtk_jpegenc_hw_irq_handler(int irq, void *priv) { struct vb2_v4l2_buffer *src_buf, *dst_buf; @@ -194,6 +212,8 @@ static irqreturn_t mtk_jpegenc_hw_irq_handler(int irq, void *priv) struct mtk_jpegenc_comp_dev *jpeg = priv; struct mtk_jpeg_dev *master_jpeg = jpeg->master_dev; + cancel_delayed_work(&jpeg->job_timeout_work); + irq_status = readl(jpeg->reg_base + JPEG_ENC_INT_STS) & JPEG_ENC_INT_STATUS_MASK_ALLIRQ; if (irq_status) @@ -322,6 +342,9 @@ static int mtk_jpegenc_hw_probe(struct platform_device *pdev) dev->plat_dev = pdev; + INIT_DELAYED_WORK(&dev->job_timeout_work, + mtk_jpegenc_timeout_work); + ret = mtk_jpegenc_init_pm(dev); if (ret) { dev_err(&pdev->dev, "Failed to get jpeg enc clock source");
Generalizes jpegenc timeout func interfaces to handle HW timeout. Signed-off-by: kyrie.wu <kyrie.wu@mediatek.com> --- V6: no change --- drivers/media/platform/mtk-jpeg/mtk_jpeg_core.h | 8 ++++++++ drivers/media/platform/mtk-jpeg/mtk_jpeg_enc_hw.c | 23 +++++++++++++++++++++++ 2 files changed, 31 insertions(+)