Updated H265 V4L2 decoder plus V4L2 stability fixes by jc-kynesim · Pull Request #4343 · raspberrypi/linux

jc-kynesim · 2021-05-12T15:56:14Z

This is a mix of my improvements to the decoder and fixes that should be making their way to master.
The decoder is faster with multiple slices and more stable than the previous version.

popcornmix · 2021-05-13T11:26:36Z

I've tested this (and @HiassofT has too) and hevc through V4l2 is working. @6by9 any objections?

6by9 · 2021-05-13T13:47:57Z

I was making my way through them, but can claim no expertise on the actual code.
I'll try to finish up this afternoon.

jc-kynesim · 2021-05-13T13:57:59Z

On the assumption that I need to change some stuff - what do I need to do to update the PR? Do I rework the branch and force push it with the same name or is there some other thing to do?

6by9 · 2021-05-13T14:00:13Z

what do I need to do to update the PR? Do I rework the branch and force push it with the same name

Yes. Force push the existing branch and the PR magically gets updated.

6by9

Mainly style issues.

6by9 · 2021-05-13T14:04:36Z

 		return;

-	if (!hdl->req_obj.req && !list_empty(&hdl->requests)) {
+	/*


Commit text

(cherry picked from commit be7e8af)

Is that a partial cherry-pick, or complete?

Generally Phil will request for backports that the first line of the body is
Commit be7e8af98f3af729aa9f08b1053f9533a5cceb91 upstream
or similar, so that it is obvious when rebasing on later trees that already include the patch.

(The patch does look very much like torvalds/linux@be7e8af)

This patch is a full cherry-pick from Hans' tree from before it made it to upstream. I think it was backported to 5.10 last night so may be redundant.

"media: v4l2-ctrls: fix reference to freed memory" is in 5.10.36. https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/log/?h=linux-5.10.y as https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/commit/?h=linux-5.10.y&id=5d0f6f5251a360f97dad0221ca60cf622caf17d2

"media: v4l2-ctrls: fix race condition in hdl->requests lists" doesn't appear to be there. Seeing as the commit text didn't cc stable, it would have had to be a manual poke to get it backported.

I got an (automated) email last night (presumably as I am listed as reporter and tester) giving as "[PATCH 5.10 315/530] media: v4l2-ctrls.c: fix race condition in hdl->requests list" and also patches against 5.11, 5.12, 5.4

So should I just remove these two patches or what? The code will work without them but it will be crashy

6by9 · 2021-05-13T14:08:07Z

 	if (!ref)
 		return;
 	ptr_to_ptr(ref->ctrl, ref->ctrl->p_new, ref->p_req);
-	ref->req = ref;


Commit text again. Looks like torvalds/linux@ac34b79

Seeing as it has
Cc: <stable@vger.kernel.org> # for v5.9 and up
I'm surprised it hasn't made it back to 5.10 already.

Again a cherry-pick from Hans' tree from before being upstreamed. I'm going to guess it will turn up soon.

6by9 · 2021-05-13T14:10:41Z

+		(s->sh->slice_type << 12);
+
+	if (s->sh->flags & V4L2_HEVC_SLICE_PARAMS_FLAG_SLICE_SAO_LUMA)
+		x |= BIT(14);


Do we not have defines for the bit usage in these registers?

No we don't - I got as far as creating defines for the registers but not individual bits. There are a lot of bits.

6by9 · 2021-05-13T14:20:32Z


 	/* Reject format types we don't support */
-	if (f->type != V4L2_BUF_TYPE_VIDEO_CAPTURE)
+	if (f->type != V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE)


This check should be totally redundant as the framework won't allow it.

6by9 · 2021-05-13T14:21:00Z

 				  struct v4l2_format *f)
 {
-	if (f->type != V4L2_BUF_TYPE_VIDEO_OUTPUT)
+	if (f->type != V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE)


Another redundant check.

6by9 · 2021-05-13T14:34:42Z

 		media_request_put(req);
 }
 EXPORT_SYMBOL_GPL(media_request_object_complete);
+


A link to the discussion on linux-media for this patch wouldn't go amiss.

Discussion was on IRC

6by9 · 2021-05-13T14:35:32Z


 #define DEBUG_TRACE_P1_CMD 0
-#define DEBUG_TRACE_EXECUTION 1
+#define DEBUG_TRACE_EXECUTION 0


Flopped back again to no tracing.

6by9 · 2021-05-13T14:42:42Z

+
+	atomic_t stream_state;
+	struct clk_request		*clk_req;
+	int src_stream_on;


Indentation.

6by9 · 2021-05-13T14:44:31Z

 	{
 		.cfg = {
 			.id	= V4L2_CID_MPEG_VIDEO_HEVC_SPS,
+			.ops = &rpivid_hevc_sps_ctrl_ops,


Indentation? Or is it Github messing about?

6by9 · 2021-05-13T14:44:40Z

 	{
 		.cfg = {
 			.id	= V4L2_CID_MPEG_VIDEO_HEVC_PPS,
+			.ops = &rpivid_hevc_pps_ctrl_ops,


Indentation? Or is it Github messing about?

jc-kynesim · 2021-05-13T16:21:54Z

I've left the initial two patches intact as I'm not sure what to do with them

6by9 · 2021-05-13T16:30:38Z

I've left the initial two patches intact as I'm not sure what to do with them

Up to Dom.
5.10.36 is available with one of the patches, so that will deal with the first patch.
I'd guess 5.10.37 will only be a few days away, and by the sounds of it it will have the second patch. If the patch exactly matches then it'll be a nice clean merge, or just wait for it to hit.

jc-kynesim · 2021-05-13T16:42:24Z

Well the email contained the exact patch that is going to be applied for "[PATCH 5.10 315/530] media: v4l2-ctrls.c: fix race condition in hdl->requests list" and what I've got looks exactly the same as was in the email with the exception of one more signed-off-by line in the commit message

When a request is re-inited it will release all control handler objects that are still in the request. It does that by unbinding and putting all those objects. When the object is unbound the obj->req pointer is set to NULL, and the object's unbind op is called. When the object it put the object's release op is called to free the memory. For a request object that contains a control handler that means that v4l2_ctrl_handler_free() is called in the release op. A control handler used in a request has a pointer to the main control handler that is created by the driver and contains the current state of all controls. If the device is unbound (due to rmmod or a forced unbind), then that main handler is freed, again by calling v4l2_ctrl_handler_free(), and any outstanding request objects that refer to that main handler have to be unbound and put as well. It does that by this test: if (!hdl->req_obj.req && !list_empty(&hdl->requests)) { I.e. the handler has no pointer to a request, so is the main handler, and one or more request objects refer to this main handler. However, this test is wrong since hdl->req_obj.req is actually NULL when re-initing a request (the object unbind will set req to NULL), and the only reason this seemingly worked is that the requests list is typically empty since the request's unbind op will remove the handler from the requests list. But if another thread is at the same time adding a new control to a request, then there is a race condition where one thread is removing a control handler object from the requests list and another thread is adding one. The result is that hdl->requests is no longer empty and the code thinks that a main handler is being freed instead of a control handler that is part of a request. There are two bugs here: first the test for hdl->req_obj.req: this should be hdl->req_obj.ops since only the main control handler will have a NULL pointer there. The second is that adding or deleting request objects from the requests list of the main handler isn't protected by taking the main handler's lock. Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl> Reported-by: John Cox <jc@kynesim.co.uk> Fixes: 6fa6f83 ("media: v4l2-ctrls: add core request support") Tested-by: John Cox <jc@kynesim.co.uk> Reported-by: John Cox <jc@kynesim.co.uk> Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org> (cherry picked from commit be7e8af)

VAAPI H265 has num entry points but never sets it. Allow a VAAPI shim to work without requiring rewriting the VAAPI driver. num_entry_points can be calculated from the slice_segment_addr of the next slice so delay processing until we have that. Also includes some minor cosmetics. Signed-off-by: John Cox <jc@kynesim.co.uk>

Use multi-planar interface rather than single plane interface. This allows dmabufs holding compressed data to be resized. Signed-off-by: John Cox <jc@kynesim.co.uk>

Add an enable count to the irq Q structures to allow the irq logic to block further callbacks if resources associated with the irq are not yet available. Signed-off-by: John Cox <jc@kynesim.co.uk>

This is probably not the API we will want to add, but it should show what semantics are needed by drivers. The goal is to allow the OUTPUT (aka source) buffer and the controls associated to a request to be released from the request, and in particular return the OUTPUT buffer back to userspace, without signalling the media request fd. This is useful for devices that are able to pre-process the OUTPUT buffer, therefore able to release it before the decoding is finished. These drivers should signal the media request fd only after the CAPTURE buffer is done. Tested-by: John Cox <jc@kynesim.co.uk> Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com>

Due to overheads in assembling controls and requests it is worth having the slice assembly phase separate from the h/w pass1 processing. Create a queue to service pass1 rather than have the pass1 finished callback trigger the next slice job. This requires a rework of the logic that splits up the buffer and request done events. This code contains two ways of doing that, we use Ezequiel Garcias <ezequiel@collabora.com> solution, but expect that in the future this will be handled by the framework in a cleaner manner. Fix up the handling of some of the memory exhaustion crashes uncovered in the process of writing this code. Signed-off-by: John Cox <jc@kynesim.co.uk>

…r job finish" This reverts commit 40ce826. Signed-off-by: John Cox <jc@kynesim.co.uk>

It is unnecessary to have a separate dmabuf to hold the cmd buffer. Map it directly from the kmalloc. Signed-off-by: John Cox <jc@kynesim.co.uk>

Guess a better value for the compressed bitstream buffer size Signed-off-by: John Cox <jc@kynesim.co.uk>

Fix stream on & off such that failures leave the driver in the correct state. Ensure that the clock is on when we are streaming and off when all contexts attached to this device have stopped streaming. Signed-off-by: John Cox <jc@kynesim.co.uk>

Move size and width checking from bitstream processing to control validation Signed-off-by: John Cox <jc@kynesim.co.uk>

pelwell · 2021-05-13T16:48:38Z

The first two commits disappear if cherry-picked onto linux-next, so I'm happy to merge them as is. The third commit doesn't apply cleanly to linux-next, but that's a problem for another day.

popcornmix · 2021-05-13T16:55:05Z

I've bumped raspberrypi:rpi-10.y to 5.10.36. That hopefully means one of the commits disappears.
I'm okay with this being merged before 5.10.37 appears if all other issues are resolved, assuming we are expecting the upstream commit to be near identical.

jc-kynesim · 2021-05-13T16:57:18Z

The 2nd patch disappears if rebased onto 5.10.36 (I've just done that). I'd expect the 1st to vanish with .37. I'm just recompiling to check nothing unexpected happens and then I'll push again

jc-kynesim · 2021-05-14T10:00:45Z

So - does anyone think I have more to do on this PR before it can be pushed?

pelwell · 2021-05-14T10:16:56Z

Other than making GitHub happy (it claims it can't rebase), or perhaps waiting for 5.10.37 (which I don't think is necessary), no.

jc-kynesim · 2021-05-14T10:20:33Z

Huh? This page says no conflicts & when I last saw it was sitting directly on 5.10.y...

pelwell · 2021-05-14T10:22:42Z

I don't understand it either, but GitHub is claiming otherwise.

Alternatively I can merge manually.

jc-kynesim · 2021-05-14T10:28:29Z

I'd fix it if I had any idea how to. Is it possible that the rebase fails 'cos it is already on the point that you want to rebase onto? It looks like a fast-forward to me.

pelwell · 2021-05-14T10:32:32Z

Merged offline.

jc-kynesim · 2021-05-14T10:33:32Z

Ta - any ideas what was confused so I can avoid in the future?

pelwell · 2021-05-14T10:44:17Z

Nope, sorry - I'm as puzzled as you.

kernel: Add CAN_J1939 as module to default build configs See: raspberrypi/linux#4346 kernel: add sensirion sgp30 to i2c-sensor overlay See: raspberrypi/linux#4355 kernel: Updated H265 V4L2 decoder plus V4L2 stability fixes See: raspberrypi/linux#4343 firmware: platform: Remove build-time constant for MICROVOLTS_PER_PIP

6by9 reviewed May 13, 2021

View reviewed changes

jc-kynesim force-pushed the test/5.10/rpivid_10 branch from ab5ea6a to a78e9b8 Compare May 13, 2021 16:20

Hans Verkuil and others added 11 commits May 13, 2021 17:47

media: rpivid: Convert to MPLANE

6992876

Use multi-planar interface rather than single plane interface. This allows dmabufs holding compressed data to be resized. Signed-off-by: John Cox <jc@kynesim.co.uk>

media: rpivid: Add an enable count to irq claim Qs

3cac2c6

Add an enable count to the irq Q structures to allow the irq logic to block further callbacks if resources associated with the irq are not yet available. Signed-off-by: John Cox <jc@kynesim.co.uk>

Revert "media: v4l2-mem2mem: allow request job buffer processing afte…

0b0791a

…r job finish" This reverts commit 40ce826. Signed-off-by: John Cox <jc@kynesim.co.uk>

media: rpivid: Map cmd buffer directly

5621d90

It is unnecessary to have a separate dmabuf to hold the cmd buffer. Map it directly from the kmalloc. Signed-off-by: John Cox <jc@kynesim.co.uk>

media: rpivid: Improve values returned when setting output format

5147ae4

Guess a better value for the compressed bitstream buffer size Signed-off-by: John Cox <jc@kynesim.co.uk>

media: rpivid: Improve SPS/PPS error handling/validation

ccdface

Move size and width checking from bitstream processing to control validation Signed-off-by: John Cox <jc@kynesim.co.uk>

jc-kynesim force-pushed the test/5.10/rpivid_10 branch from a78e9b8 to ccdface Compare May 13, 2021 17:01

pelwell merged commit ccdface into raspberrypi:rpi-5.10.y May 14, 2021

Conversation

jc-kynesim commented May 12, 2021

Uh oh!

popcornmix commented May 13, 2021

Uh oh!

6by9 commented May 13, 2021

Uh oh!

jc-kynesim commented May 13, 2021

Uh oh!

6by9 commented May 13, 2021

Uh oh!

6by9 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jc-kynesim commented May 13, 2021

Uh oh!

6by9 commented May 13, 2021

Uh oh!

jc-kynesim commented May 13, 2021

Uh oh!

pelwell commented May 13, 2021

Uh oh!

popcornmix commented May 13, 2021

Uh oh!

jc-kynesim commented May 13, 2021

Uh oh!

jc-kynesim commented May 14, 2021

Uh oh!

pelwell commented May 14, 2021

Uh oh!

jc-kynesim commented May 14, 2021

Uh oh!

pelwell commented May 14, 2021

Uh oh!