| Message ID | 20220914150105.209484-1-dmitry.osipenko@collabora.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v1] media: cedrus: Propagate error code from cedrus_h265_skip_bits() | expand |
Hi Dmitry, This patch has a conflict with this patch from Jernej: https://patchwork.linuxtv.org/project/linux-media/patch/20221017194413.1198301-1-jernej.skrabec@gmail.com/ I decided to take Jernej's patch first. Can you make a v2 that sits on top of that patch? Thanks! Hans On 9/14/22 17:01, Dmitry Osipenko wrote: > The cedrus_h265_skip_bits() may get into infinite loop if decoding > parameters are incorrect. In this case we detect the loop and print > a error message, continuing the decoding that is fated to fail. > > Will be cleaner to abort the decoding early. Propagate the error code > to cedrus_device_run() and reset hardware on the cedrus_h265_skip_bits() > failure. > > Suggested-by: Jernej Škrabec <jernej.skrabec@gmail.com> > Signed-off-by: Dmitry Osipenko <dmitry.osipenko@collabora.com> > --- > drivers/staging/media/sunxi/cedrus/cedrus_dec.c | 2 ++ > drivers/staging/media/sunxi/cedrus/cedrus_h265.c | 15 ++++++++++++--- > drivers/staging/media/sunxi/cedrus/cedrus_hw.c | 7 ++++++- > drivers/staging/media/sunxi/cedrus/cedrus_hw.h | 2 ++ > 4 files changed, 22 insertions(+), 4 deletions(-) > > diff --git a/drivers/staging/media/sunxi/cedrus/cedrus_dec.c b/drivers/staging/media/sunxi/cedrus/cedrus_dec.c > index e7f7602a5ab4..ae5df3dc01c0 100644 > --- a/drivers/staging/media/sunxi/cedrus/cedrus_dec.c > +++ b/drivers/staging/media/sunxi/cedrus/cedrus_dec.c > @@ -112,6 +112,8 @@ void cedrus_device_run(void *priv) > > dev->dec_ops[ctx->current_codec]->trigger(ctx); > } else { > + cedrus_hw_reset(dev); > + > v4l2_m2m_buf_done_and_job_finish(ctx->dev->m2m_dev, > ctx->fh.m2m_ctx, > VB2_BUF_STATE_ERROR); > diff --git a/drivers/staging/media/sunxi/cedrus/cedrus_h265.c b/drivers/staging/media/sunxi/cedrus/cedrus_h265.c > index 4952fc17f3e6..f409f59452d8 100644 > --- a/drivers/staging/media/sunxi/cedrus/cedrus_h265.c > +++ b/drivers/staging/media/sunxi/cedrus/cedrus_h265.c > @@ -224,9 +224,10 @@ static void cedrus_h265_pred_weight_write(struct cedrus_dev *dev, > } > } > > -static void cedrus_h265_skip_bits(struct cedrus_dev *dev, int num) > +static int cedrus_h265_skip_bits(struct cedrus_dev *dev, int num) > { > int count = 0; > + int err; > > while (count < num) { > int tmp = min(num - count, 32); > @@ -235,11 +236,16 @@ static void cedrus_h265_skip_bits(struct cedrus_dev *dev, int num) > VE_DEC_H265_TRIGGER_FLUSH_BITS | > VE_DEC_H265_TRIGGER_TYPE_N_BITS(tmp)); > > - if (cedrus_wait_for(dev, VE_DEC_H265_STATUS, VE_DEC_H265_STATUS_VLD_BUSY)) > + err = cedrus_wait_for(dev, VE_DEC_H265_STATUS, VE_DEC_H265_STATUS_VLD_BUSY); > + if (err) { > dev_err_ratelimited(dev->dev, "timed out waiting to skip bits\n"); > + return err; > + } > > count += tmp; > } > + > + return 0; > } > > static void cedrus_h265_write_scaling_list(struct cedrus_ctx *ctx, > @@ -408,6 +414,7 @@ static int cedrus_h265_setup(struct cedrus_ctx *ctx, struct cedrus_run *run) > u32 pic_order_cnt[2]; > u8 *padding; > int count; > + int err; > u32 reg; > > sps = run->h265.sps; > @@ -534,7 +541,9 @@ static int cedrus_h265_setup(struct cedrus_ctx *ctx, struct cedrus_run *run) > /* Include the one bit. */ > count++; > > - cedrus_h265_skip_bits(dev, slice_params->data_byte_offset * 8 - count); > + err = cedrus_h265_skip_bits(dev, slice_params->data_byte_offset * 8 - count); > + if (err) > + return err; > > /* Bitstream parameters. */ > > diff --git a/drivers/staging/media/sunxi/cedrus/cedrus_hw.c b/drivers/staging/media/sunxi/cedrus/cedrus_hw.c > index a6470a89851e..e9ceca332062 100644 > --- a/drivers/staging/media/sunxi/cedrus/cedrus_hw.c > +++ b/drivers/staging/media/sunxi/cedrus/cedrus_hw.c > @@ -168,11 +168,16 @@ void cedrus_watchdog(struct work_struct *work) > VB2_BUF_STATE_ERROR); > } > > +void cedrus_hw_reset(struct cedrus_dev *dev) > +{ > + reset_control_reset(dev->rstc); > +} > + > int cedrus_hw_suspend(struct device *device) > { > struct cedrus_dev *dev = dev_get_drvdata(device); > > - reset_control_assert(dev->rstc); > + cedrus_hw_reset(dev); > > clk_disable_unprepare(dev->ram_clk); > clk_disable_unprepare(dev->mod_clk); > diff --git a/drivers/staging/media/sunxi/cedrus/cedrus_hw.h b/drivers/staging/media/sunxi/cedrus/cedrus_hw.h > index 7c92f00e36da..919c4475f0d7 100644 > --- a/drivers/staging/media/sunxi/cedrus/cedrus_hw.h > +++ b/drivers/staging/media/sunxi/cedrus/cedrus_hw.h > @@ -30,4 +30,6 @@ void cedrus_hw_remove(struct cedrus_dev *dev); > > void cedrus_watchdog(struct work_struct *work); > > +void cedrus_hw_reset(struct cedrus_dev *dev); > + > #endif
Hi Hans, Dne ponedeljek, 24. oktober 2022 ob 13:38:36 CEST je Hans Verkuil napisal(a): > Hi Dmitry, > > This patch has a conflict with this patch from Jernej: > > https://patchwork.linuxtv.org/project/linux-media/patch/20221017194413.11983 > 01-1-jernej.skrabec@gmail.com/ > > I decided to take Jernej's patch first. Can you make a v2 that sits on top > of that patch? I believe you already merged first version of the patch for 6.1. No need for this version though, first version already solves main issue. Best regards, Jernej > > Thanks! > > Hans > > On 9/14/22 17:01, Dmitry Osipenko wrote: > > The cedrus_h265_skip_bits() may get into infinite loop if decoding > > parameters are incorrect. In this case we detect the loop and print > > a error message, continuing the decoding that is fated to fail. > > > > Will be cleaner to abort the decoding early. Propagate the error code > > to cedrus_device_run() and reset hardware on the cedrus_h265_skip_bits() > > failure. > > > > Suggested-by: Jernej Škrabec <jernej.skrabec@gmail.com> > > Signed-off-by: Dmitry Osipenko <dmitry.osipenko@collabora.com> > > --- > > > > drivers/staging/media/sunxi/cedrus/cedrus_dec.c | 2 ++ > > drivers/staging/media/sunxi/cedrus/cedrus_h265.c | 15 ++++++++++++--- > > drivers/staging/media/sunxi/cedrus/cedrus_hw.c | 7 ++++++- > > drivers/staging/media/sunxi/cedrus/cedrus_hw.h | 2 ++ > > 4 files changed, 22 insertions(+), 4 deletions(-) > > > > diff --git a/drivers/staging/media/sunxi/cedrus/cedrus_dec.c > > b/drivers/staging/media/sunxi/cedrus/cedrus_dec.c index > > e7f7602a5ab4..ae5df3dc01c0 100644 > > --- a/drivers/staging/media/sunxi/cedrus/cedrus_dec.c > > +++ b/drivers/staging/media/sunxi/cedrus/cedrus_dec.c > > @@ -112,6 +112,8 @@ void cedrus_device_run(void *priv) > > > > dev->dec_ops[ctx->current_codec]->trigger(ctx); > > > > } else { > > > > + cedrus_hw_reset(dev); > > + > > > > v4l2_m2m_buf_done_and_job_finish(ctx->dev->m2m_dev, > > > > ctx- >fh.m2m_ctx, > > VB2_BUF_STATE_ERROR); > > > > diff --git a/drivers/staging/media/sunxi/cedrus/cedrus_h265.c > > b/drivers/staging/media/sunxi/cedrus/cedrus_h265.c index > > 4952fc17f3e6..f409f59452d8 100644 > > --- a/drivers/staging/media/sunxi/cedrus/cedrus_h265.c > > +++ b/drivers/staging/media/sunxi/cedrus/cedrus_h265.c > > @@ -224,9 +224,10 @@ static void cedrus_h265_pred_weight_write(struct > > cedrus_dev *dev,> > > } > > > > } > > > > -static void cedrus_h265_skip_bits(struct cedrus_dev *dev, int num) > > +static int cedrus_h265_skip_bits(struct cedrus_dev *dev, int num) > > > > { > > > > int count = 0; > > > > + int err; > > > > while (count < num) { > > > > int tmp = min(num - count, 32); > > > > @@ -235,11 +236,16 @@ static void cedrus_h265_skip_bits(struct cedrus_dev > > *dev, int num)> > > VE_DEC_H265_TRIGGER_FLUSH_BITS | > > VE_DEC_H265_TRIGGER_TYPE_N_BITS(tmp)); > > > > - if (cedrus_wait_for(dev, VE_DEC_H265_STATUS, > > VE_DEC_H265_STATUS_VLD_BUSY)) + err = cedrus_wait_for(dev, > > VE_DEC_H265_STATUS, VE_DEC_H265_STATUS_VLD_BUSY); + if (err) { > > > > dev_err_ratelimited(dev->dev, "timed out waiting to skip bits\n"); > > > > + return err; > > + } > > > > count += tmp; > > > > } > > > > + > > + return 0; > > > > } > > > > static void cedrus_h265_write_scaling_list(struct cedrus_ctx *ctx, > > > > @@ -408,6 +414,7 @@ static int cedrus_h265_setup(struct cedrus_ctx *ctx, > > struct cedrus_run *run)> > > u32 pic_order_cnt[2]; > > u8 *padding; > > int count; > > > > + int err; > > > > u32 reg; > > > > sps = run->h265.sps; > > > > @@ -534,7 +541,9 @@ static int cedrus_h265_setup(struct cedrus_ctx *ctx, > > struct cedrus_run *run)> > > /* Include the one bit. */ > > count++; > > > > - cedrus_h265_skip_bits(dev, slice_params->data_byte_offset * 8 - count); > > + err = cedrus_h265_skip_bits(dev, slice_params->data_byte_offset * 8 - > > count); + if (err) > > + return err; > > > > /* Bitstream parameters. */ > > > > diff --git a/drivers/staging/media/sunxi/cedrus/cedrus_hw.c > > b/drivers/staging/media/sunxi/cedrus/cedrus_hw.c index > > a6470a89851e..e9ceca332062 100644 > > --- a/drivers/staging/media/sunxi/cedrus/cedrus_hw.c > > +++ b/drivers/staging/media/sunxi/cedrus/cedrus_hw.c > > @@ -168,11 +168,16 @@ void cedrus_watchdog(struct work_struct *work) > > > > VB2_BUF_STATE_ERROR); > > > > } > > > > +void cedrus_hw_reset(struct cedrus_dev *dev) > > +{ > > + reset_control_reset(dev->rstc); > > +} > > + > > > > int cedrus_hw_suspend(struct device *device) > > { > > > > struct cedrus_dev *dev = dev_get_drvdata(device); > > > > - reset_control_assert(dev->rstc); > > + cedrus_hw_reset(dev); > > > > clk_disable_unprepare(dev->ram_clk); > > clk_disable_unprepare(dev->mod_clk); > > > > diff --git a/drivers/staging/media/sunxi/cedrus/cedrus_hw.h > > b/drivers/staging/media/sunxi/cedrus/cedrus_hw.h index > > 7c92f00e36da..919c4475f0d7 100644 > > --- a/drivers/staging/media/sunxi/cedrus/cedrus_hw.h > > +++ b/drivers/staging/media/sunxi/cedrus/cedrus_hw.h > > @@ -30,4 +30,6 @@ void cedrus_hw_remove(struct cedrus_dev *dev); > > > > void cedrus_watchdog(struct work_struct *work); > > > > +void cedrus_hw_reset(struct cedrus_dev *dev); > > + > > > > #endif
On 10/25/22 07:59, Jernej Škrabec wrote: > Dne ponedeljek, 24. oktober 2022 ob 13:38:36 CEST je Hans Verkuil napisal(a): >> Hi Dmitry, >> >> This patch has a conflict with this patch from Jernej: >> >> https://patchwork.linuxtv.org/project/linux-media/patch/20221017194413.11983 >> 01-1-jernej.skrabec@gmail.com/ >> >> I decided to take Jernej's patch first. Can you make a v2 that sits on top >> of that patch? > I believe you already merged first version of the patch for 6.1. No need for > this version though, first version already solves main issue. That was a followup to the first version.
On 10/25/22 11:34, Dmitry Osipenko wrote: > On 10/25/22 07:59, Jernej Škrabec wrote: >> Dne ponedeljek, 24. oktober 2022 ob 13:38:36 CEST je Hans Verkuil napisal(a): >>> Hi Dmitry, >>> >>> This patch has a conflict with this patch from Jernej: >>> >>> https://patchwork.linuxtv.org/project/linux-media/patch/20221017194413.11983 >>> 01-1-jernej.skrabec@gmail.com/ >>> >>> I decided to take Jernej's patch first. Can you make a v2 that sits on top >>> of that patch? >> I believe you already merged first version of the patch for 6.1. No need for >> this version though, first version already solves main issue. > > That was a followup to the first version. > I have no idea what you are both talking about :-) "I believe you already merged first version of the patch for 6.1" Which patch are you referring to? Regards, Hans
On 10/25/22 13:23, Hans Verkuil wrote: > On 10/25/22 11:34, Dmitry Osipenko wrote: >> On 10/25/22 07:59, Jernej Škrabec wrote: >>> Dne ponedeljek, 24. oktober 2022 ob 13:38:36 CEST je Hans Verkuil napisal(a): >>>> Hi Dmitry, >>>> >>>> This patch has a conflict with this patch from Jernej: >>>> >>>> https://patchwork.linuxtv.org/project/linux-media/patch/20221017194413.11983 >>>> 01-1-jernej.skrabec@gmail.com/ >>>> >>>> I decided to take Jernej's patch first. Can you make a v2 that sits on top >>>> of that patch? >>> I believe you already merged first version of the patch for 6.1. No need for >>> this version though, first version already solves main issue. >> >> That was a followup to the first version. >> > > I have no idea what you are both talking about :-) > > "I believe you already merged first version of the patch for 6.1" Which patch > are you referring to? This is about [1] that was already merged. This patch implements the error propagation that Jernej asked [2] when he reviewed [1]. [1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=91db7a3fc7fe670cf1770a398a43bb4a1f776bf1 [2] https://lore.kernel.org/lkml/1733932.VLH7GnMWUR@kista/ I'll prepare the v2 if this change is still desired by Jernej.
On 10/25/22 12:36, Dmitry Osipenko wrote: > On 10/25/22 13:23, Hans Verkuil wrote: >> On 10/25/22 11:34, Dmitry Osipenko wrote: >>> On 10/25/22 07:59, Jernej Škrabec wrote: >>>> Dne ponedeljek, 24. oktober 2022 ob 13:38:36 CEST je Hans Verkuil napisal(a): >>>>> Hi Dmitry, >>>>> >>>>> This patch has a conflict with this patch from Jernej: >>>>> >>>>> https://patchwork.linuxtv.org/project/linux-media/patch/20221017194413.11983 >>>>> 01-1-jernej.skrabec@gmail.com/ >>>>> >>>>> I decided to take Jernej's patch first. Can you make a v2 that sits on top >>>>> of that patch? >>>> I believe you already merged first version of the patch for 6.1. No need for >>>> this version though, first version already solves main issue. >>> >>> That was a followup to the first version. >>> >> >> I have no idea what you are both talking about :-) >> >> "I believe you already merged first version of the patch for 6.1" Which patch >> are you referring to? > > This is about [1] that was already merged. This patch implements the > error propagation that Jernej asked [2] when he reviewed [1]. > > [1] > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=91db7a3fc7fe670cf1770a398a43bb4a1f776bf1 > > [2] https://lore.kernel.org/lkml/1733932.VLH7GnMWUR@kista/ > > I'll prepare the v2 if this change is still desired by Jernej. > Ah, OK. That was not obvious to me :-) I'll mark this v1 patch as 'Changes Requested' in patchwork. Thank you for the explanation! Regards, Hans
diff --git a/drivers/staging/media/sunxi/cedrus/cedrus_dec.c b/drivers/staging/media/sunxi/cedrus/cedrus_dec.c index e7f7602a5ab4..ae5df3dc01c0 100644 --- a/drivers/staging/media/sunxi/cedrus/cedrus_dec.c +++ b/drivers/staging/media/sunxi/cedrus/cedrus_dec.c @@ -112,6 +112,8 @@ void cedrus_device_run(void *priv) dev->dec_ops[ctx->current_codec]->trigger(ctx); } else { + cedrus_hw_reset(dev); + v4l2_m2m_buf_done_and_job_finish(ctx->dev->m2m_dev, ctx->fh.m2m_ctx, VB2_BUF_STATE_ERROR); diff --git a/drivers/staging/media/sunxi/cedrus/cedrus_h265.c b/drivers/staging/media/sunxi/cedrus/cedrus_h265.c index 4952fc17f3e6..f409f59452d8 100644 --- a/drivers/staging/media/sunxi/cedrus/cedrus_h265.c +++ b/drivers/staging/media/sunxi/cedrus/cedrus_h265.c @@ -224,9 +224,10 @@ static void cedrus_h265_pred_weight_write(struct cedrus_dev *dev, } } -static void cedrus_h265_skip_bits(struct cedrus_dev *dev, int num) +static int cedrus_h265_skip_bits(struct cedrus_dev *dev, int num) { int count = 0; + int err; while (count < num) { int tmp = min(num - count, 32); @@ -235,11 +236,16 @@ static void cedrus_h265_skip_bits(struct cedrus_dev *dev, int num) VE_DEC_H265_TRIGGER_FLUSH_BITS | VE_DEC_H265_TRIGGER_TYPE_N_BITS(tmp)); - if (cedrus_wait_for(dev, VE_DEC_H265_STATUS, VE_DEC_H265_STATUS_VLD_BUSY)) + err = cedrus_wait_for(dev, VE_DEC_H265_STATUS, VE_DEC_H265_STATUS_VLD_BUSY); + if (err) { dev_err_ratelimited(dev->dev, "timed out waiting to skip bits\n"); + return err; + } count += tmp; } + + return 0; } static void cedrus_h265_write_scaling_list(struct cedrus_ctx *ctx, @@ -408,6 +414,7 @@ static int cedrus_h265_setup(struct cedrus_ctx *ctx, struct cedrus_run *run) u32 pic_order_cnt[2]; u8 *padding; int count; + int err; u32 reg; sps = run->h265.sps; @@ -534,7 +541,9 @@ static int cedrus_h265_setup(struct cedrus_ctx *ctx, struct cedrus_run *run) /* Include the one bit. */ count++; - cedrus_h265_skip_bits(dev, slice_params->data_byte_offset * 8 - count); + err = cedrus_h265_skip_bits(dev, slice_params->data_byte_offset * 8 - count); + if (err) + return err; /* Bitstream parameters. */ diff --git a/drivers/staging/media/sunxi/cedrus/cedrus_hw.c b/drivers/staging/media/sunxi/cedrus/cedrus_hw.c index a6470a89851e..e9ceca332062 100644 --- a/drivers/staging/media/sunxi/cedrus/cedrus_hw.c +++ b/drivers/staging/media/sunxi/cedrus/cedrus_hw.c @@ -168,11 +168,16 @@ void cedrus_watchdog(struct work_struct *work) VB2_BUF_STATE_ERROR); } +void cedrus_hw_reset(struct cedrus_dev *dev) +{ + reset_control_reset(dev->rstc); +} + int cedrus_hw_suspend(struct device *device) { struct cedrus_dev *dev = dev_get_drvdata(device); - reset_control_assert(dev->rstc); + cedrus_hw_reset(dev); clk_disable_unprepare(dev->ram_clk); clk_disable_unprepare(dev->mod_clk); diff --git a/drivers/staging/media/sunxi/cedrus/cedrus_hw.h b/drivers/staging/media/sunxi/cedrus/cedrus_hw.h index 7c92f00e36da..919c4475f0d7 100644 --- a/drivers/staging/media/sunxi/cedrus/cedrus_hw.h +++ b/drivers/staging/media/sunxi/cedrus/cedrus_hw.h @@ -30,4 +30,6 @@ void cedrus_hw_remove(struct cedrus_dev *dev); void cedrus_watchdog(struct work_struct *work); +void cedrus_hw_reset(struct cedrus_dev *dev); + #endif
The cedrus_h265_skip_bits() may get into infinite loop if decoding parameters are incorrect. In this case we detect the loop and print a error message, continuing the decoding that is fated to fail. Will be cleaner to abort the decoding early. Propagate the error code to cedrus_device_run() and reset hardware on the cedrus_h265_skip_bits() failure. Suggested-by: Jernej Škrabec <jernej.skrabec@gmail.com> Signed-off-by: Dmitry Osipenko <dmitry.osipenko@collabora.com> --- drivers/staging/media/sunxi/cedrus/cedrus_dec.c | 2 ++ drivers/staging/media/sunxi/cedrus/cedrus_h265.c | 15 ++++++++++++--- drivers/staging/media/sunxi/cedrus/cedrus_hw.c | 7 ++++++- drivers/staging/media/sunxi/cedrus/cedrus_hw.h | 2 ++ 4 files changed, 22 insertions(+), 4 deletions(-)