media: cedrus: Propagate error code from cedrus_h265_skip_bits()

PATCH In review Dmitry Osipenko 3 participants 7 messages Sep 14, 2022 – Oct 25, 2022

Subsystems (4)

Media (V4L/DVB) Staging Arm/Allwinner sunXi SoC support Allwinner VPU driver

lore

Share & tools

lore

Dmitry Osipenko

author

2022-09-14 15:03 UTC #0 lore raw

PATCH 1/1 media: cedrus: Propagate error code from cedrus_h265_skip_bits()

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.

Signed-off-by Dmitry Osipenko <[email protected]>

---
 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(-)

drivers/staging/media/sunxi/cedrus/cedrus_dec.c +2 -0

index e7f7602a5ab4..ae5df3dc01c0 100644
@@ -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);

drivers/staging/media/sunxi/cedrus/cedrus_h265.c +12 -3

index 4952fc17f3e6..f409f59452d8 100644
@@ -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. */

drivers/staging/media/sunxi/cedrus/cedrus_hw.c +6 -1

index a6470a89851e..e9ceca332062 100644
@@ -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);

drivers/staging/media/sunxi/cedrus/cedrus_hw.h +2 -0

index 7c92f00e36da..919c4475f0d7 100644
@@ -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

Signature · 1 line

2.37.3

Hans Verkuil

2022-10-24 11:38 UTC #1 lore raw

Hi Dmitry,

This patch has a conflict with this patch from Jernej:

https://patchwork.linuxtv.org/project/linux-media/patch/[email protected]/

I decided to take Jernej's patch first. Can you make a v2 that sits on top of that patch?

Thanks!

Hans

Quoted 116 lines · Dmitry Osipenko

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.

Signed-off-by Dmitry Osipenko <[email protected]>

---
 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(-)

drivers/staging/media/sunxi/cedrus/cedrus_dec.c +2 -0

index e7f7602a5ab4..ae5df3dc01c0 100644
@@ -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);

drivers/staging/media/sunxi/cedrus/cedrus_h265.c +12 -3

index 4952fc17f3e6..f409f59452d8 100644
@@ -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. */

drivers/staging/media/sunxi/cedrus/cedrus_hw.c +6 -1

index a6470a89851e..e9ceca332062 100644
@@ -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);

drivers/staging/media/sunxi/cedrus/cedrus_hw.h +2 -0

index 7c92f00e36da..919c4475f0d7 100644
@@ -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

Jernej Škrabec

maintainer

2022-10-25 04:59 UTC #2 lore raw

Hi Hans,

Dne ponedeljek, 24. oktober 2022 ob 13:38:36 CEST je Hans Verkuil napisal(a):

Quoted 9 lines

Hi Dmitry,
This patch has a conflict with this patch from Jernej:
https://patchwork.linuxtv.org/project/linux-media/patch/20221017194413.11983
[email protected]/
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

Quoted 35 lines

Thanks!
Hans
Quoted 29 lines · Dmitry Osipenko
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 <[email protected]>
Signed-off-by Dmitry Osipenko <[email protected]>
---
 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(-)
(quoted diff) +0 -0
b/drivers/staging/media/sunxi/cedrus/cedrus_dec.c index e7f7602a5ab4..ae5df3dc01c0 100644
drivers/staging/media/sunxi/cedrus/cedrus_dec.c +0 -0
@@ -112,6 +112,8 @@ void cedrus_device_run(void *priv)
 
 		dev->dec_ops[ctx->current_codec]->trigger(ctx);
 
 	} else {
 

Quoted 8 lines

drivers/staging/media/sunxi/cedrus/cedrus_dec.c
Quoted 6 lines
drivers/staging/media/sunxi/cedrus/cedrus_dec.c +2 -0
+		cedrus_hw_reset(dev);
+
 
drivers/staging/media/sunxi/cedrus/cedrus_dec.c +0 -0
 		v4l2_m2m_buf_done_and_job_finish(ctx->dev->m2m_dev,
 
 						 ctx-
fh.m2m_ctx,
Quoted 1 line

VB2_BUF_STATE_ERROR);

Quoted 25 lines

Quoted 25 lines
(quoted diff) +0 -0
b/drivers/staging/media/sunxi/cedrus/cedrus_h265.c index 4952fc17f3e6..f409f59452d8 100644
drivers/staging/media/sunxi/cedrus/cedrus_h265.c +0 -0
@@ -224,9 +224,10 @@ static void cedrus_h265_pred_weight_write(struct
cedrus_dev *dev,>
drivers/staging/media/sunxi/cedrus/cedrus_h265.c +2 -1
 	}
 
 }
 
-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);

Quoted 8 lines

drivers/staging/media/sunxi/cedrus/cedrus_h265.c +0 -1

@@ -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) {

Quoted 2 lines

Quoted 2 lines
dev_err_ratelimited(dev->dev, "timed out

waiting to skip bits\n");

Quoted 25 lines

drivers/staging/media/sunxi/cedrus/cedrus_h265.c +4 -0

+			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)>

drivers/staging/media/sunxi/cedrus/cedrus_h265.c +1 -0

 	u32 pic_order_cnt[2];
 	u8 *padding;
 	int count;
 
+	int err;
 
 	u32 reg;

Quoted 8 lines

drivers/staging/media/sunxi/cedrus/cedrus_h265.c static int cedrus_h265_setup(struct cedrus_ctx *ctx, struct cedrus_run *run)
Quoted 8 lines
drivers/staging/media/sunxi/cedrus/cedrus_h265.c +0 -0
 	sps = run->h265.sps;
@@ -534,7 +541,9 @@ static int cedrus_h265_setup(struct cedrus_ctx *ctx,
struct cedrus_run *run)>
drivers/staging/media/sunxi/cedrus/cedrus_h265.c +0 -1
 	/* Include the one bit. */
 	count++;
 
-	cedrus_h265_skip_bits(dev, slice_params->data_byte_offset * 8 - 

count);

Quoted 1 line

drivers/staging/media/sunxi/cedrus/cedrus_h265.c
Quoted 1 line
drivers/staging/media/sunxi/cedrus/cedrus_h265.c +1 -0
+	err = cedrus_h265_skip_bits(dev, slice_params->data_byte_offset * 8 

Quoted 5 lines

Quoted 5 lines
count); + if (err)
drivers/staging/media/sunxi/cedrus/cedrus_h265.c +1 -0
+		return err;
 
 	/* Bitstream parameters. */

Quoted 8 lines

drivers/staging/media/sunxi/cedrus/cedrus_hw.c void cedrus_watchdog(struct work_struct *work) · line 169
Quoted 8 lines
(quoted diff) +0 -0
b/drivers/staging/media/sunxi/cedrus/cedrus_hw.c index a6470a89851e..e9ceca332062 100644
drivers/staging/media/sunxi/cedrus/cedrus_hw.c +0 -0
@@ -168,11 +168,16 @@ void cedrus_watchdog(struct work_struct *work)
 
 

VB2_BUF_STATE_ERROR);

Quoted 33 lines

drivers/staging/media/sunxi/cedrus/cedrus_hw.c +6 -1

 }
 
+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);

drivers/staging/media/sunxi/cedrus/cedrus_hw.c +0 -0

 	clk_disable_unprepare(dev->ram_clk);
 	clk_disable_unprepare(dev->mod_clk);

(quoted diff) +0 -0

b/drivers/staging/media/sunxi/cedrus/cedrus_hw.h index 7c92f00e36da..919c4475f0d7 100644

drivers/staging/media/sunxi/cedrus/cedrus_hw.h +2 -0

@@ -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

Dmitry Osipenko

author

2022-10-25 09:35 UTC #3 lore raw

Quoted 5 lines · Jernej Škrabec

Dne ponedeljek, 24. oktober 2022 ob 13:38:36 CEST je Hans Verkuil napisal(a):
Quoted 4 lines
Hi Dmitry,
This patch has a conflict with this patch from Jernej:

Quoted 7 lines

Quoted 5 lines
https://patchwork.linuxtv.org/project/linux-media/patch/20221017194413.11983
[email protected]/
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.

Signature · 2 lines

Best regards,
Dmitry

Hans Verkuil

2022-10-25 10:23 UTC #4 lore raw

Quoted 9 lines · Dmitry Osipenko

Quoted 8 lines · Jernej Škrabec
Dne ponedeljek, 24. oktober 2022 ob 13:38:36 CEST je Hans Verkuil napisal(a):
Quoted 7 lines
Hi Dmitry,
This patch has a conflict with this patch from Jernej:
https://patchwork.linuxtv.org/project/linux-media/patch/20221017194413.11983
[email protected]/

Quoted 7 lines

Quoted 4 lines
Quoted 2 lines
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

Dmitry Osipenko

author

2022-10-25 10:36 UTC #5 lore raw

Quoted 15 lines · Hans Verkuil

Quoted 14 lines · Dmitry Osipenko
Quoted 12 lines · Jernej Škrabec
Dne ponedeljek, 24. oktober 2022 ob 13:38:36 CEST je Hans Verkuil napisal(a):
Quoted 9 lines
Hi Dmitry,
This patch has a conflict with this patch from Jernej:
https://patchwork.linuxtv.org/project/linux-media/patch/20221017194413.11983
[email protected]/
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.

Quoted 7 lines

Quoted 2 lines
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.

Signature · 2 lines

Best regards,
Dmitry

Hans Verkuil

2022-10-25 10:41 UTC #6 lore raw

Quoted 27 lines · Dmitry Osipenko

Quoted 15 lines · Hans Verkuil
Quoted 14 lines · Dmitry Osipenko
Quoted 12 lines · Jernej Škrabec
Dne ponedeljek, 24. oktober 2022 ob 13:38:36 CEST je Hans Verkuil napisal(a):
Quoted 9 lines
Hi Dmitry,
This patch has a conflict with this patch from Jernej:
https://patchwork.linuxtv.org/project/linux-media/patch/20221017194413.11983
[email protected]/
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.
Quoted 7 lines
Quoted 2 lines
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].

Quoted 7 lines

[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