Messages in this thread | | | Subject | Re: [PATCH 0/2] media: meson: vdec: Add compliant H264 support | From | Maxime Jourdan <> | Date | Fri, 18 Oct 2019 11:23:47 +0200 |
| |
On 18/10/2019 09:50, Maxime Jourdan wrote: > On Wed, Oct 9, 2019 at 2:01 PM Hans Verkuil <hverkuil@xs4all.nl> wrote: >> >> On 10/8/19 3:40 PM, Maxime Jourdan wrote: >>> On 07/10/2019 18:39, Hans Verkuil wrote: >>>> On 10/7/19 6:24 PM, Maxime Jourdan wrote: >>>>> On 07/10/2019 17:12, Hans Verkuil wrote: >>>>>> On 10/7/19 4:59 PM, Maxime Jourdan wrote: >>>>>>> Hello, >>>>>>> >>>>>>> This patch series aims to bring H.264 support as well as compliance update >>>>>>> to the amlogic stateful video decoder driver. >>>>>>> >>>>>>> There is 1 issue that remains currently: >>>>>>> >>>>>>> - The following codepath had to be commented out from v4l2-compliance as >>>>>>> it led to stalling: >>>>>>> >>>>>>> if (node->codec_mask & STATEFUL_DECODER) { >>>>>>> struct v4l2_decoder_cmd cmd; >>>>>>> buffer buf_cap(m2m_q); >>>>>>> >>>>>>> memset(&cmd, 0, sizeof(cmd)); >>>>>>> cmd.cmd = V4L2_DEC_CMD_STOP; >>>>>>> >>>>>>> /* No buffers are queued, call STREAMON, then STOP */ >>>>>>> fail_on_test(node->streamon(q.g_type())); >>>>>>> fail_on_test(node->streamon(m2m_q.g_type())); >>>>>>> fail_on_test(doioctl(node, VIDIOC_DECODER_CMD, &cmd)); >>>>>>> >>>>>>> fail_on_test(buf_cap.querybuf(node, 0)); >>>>>>> fail_on_test(buf_cap.qbuf(node)); >>>>>>> fail_on_test(buf_cap.dqbuf(node)); >>>>>>> fail_on_test(!(buf_cap.g_flags() & V4L2_BUF_FLAG_LAST)); >>>>>>> for (unsigned p = 0; p < buf_cap.g_num_planes(); p++) >>>>>>> fail_on_test(buf_cap.g_bytesused(p)); >>>>>>> fail_on_test(node->streamoff(q.g_type())); >>>>>>> fail_on_test(node->streamoff(m2m_q.g_type())); >>>>>>> >>>>>>> /* Call STREAMON, queue one CAPTURE buffer, then STOP */ >>>>>>> fail_on_test(node->streamon(q.g_type())); >>>>>>> fail_on_test(node->streamon(m2m_q.g_type())); >>>>>>> fail_on_test(buf_cap.querybuf(node, 0)); >>>>>>> fail_on_test(buf_cap.qbuf(node)); >>>>>>> fail_on_test(doioctl(node, VIDIOC_DECODER_CMD, &cmd)); >>>>>>> >>>>>>> fail_on_test(buf_cap.dqbuf(node)); >>>>>>> fail_on_test(!(buf_cap.g_flags() & V4L2_BUF_FLAG_LAST)); >>>>>>> for (unsigned p = 0; p < buf_cap.g_num_planes(); p++) >>>>>>> fail_on_test(buf_cap.g_bytesused(p)); >>>>>>> fail_on_test(node->streamoff(q.g_type())); >>>>>>> fail_on_test(node->streamoff(m2m_q.g_type())); >>>>>>> } >>>>>>> >>>>>>> The reason for this is because the driver has a limitation where all >>>>>>> capturebuffers must be queued to the driver before STREAMON is effective. >>>>>>> The firmware needs to know in advance what all the buffers are before >>>>>>> starting to decode. >>>>>>> This limitation is enforced via q->min_buffers_needed. >>>>>>> As such, in this compliance codepath, STREAMON is never actually called >>>>>>> driver-side and there is a stall on fail_on_test(buf_cap.dqbuf(node)); >>>>>> >>>>>> That's interesting. I will have to look more closely at this. >> >> This requires a helper function in videobuf2-v4l2.c. >> >> In vdec_decoder_cmd you would need code like this: >> >> if (!vb2_start_streaming_called(&capture_queue)) { >> vb2_dequeue_empty_last_buf(&capture_queue); >> return 0; >> } >> >> The vb2_dequeue_empty_last_buf (function name can probably be improved upon!) >> does nothing if no capture buffers were queued, otherwise it takes the first >> buffer, sets the LAST flag and sets bytesused to 0 and marks it as DONE. >> >> The driver cannot do this directly, since the buffers were never queued to the >> driver and are owned by vb2. >> >> This is something that needs to be done for any codec driver that sets >> min_buffers_needed to a value > 1. >> >> The vb2 function would look something like this: >> >> void vb2_dqbuf_empty_last_buf(struct vb2_queue *q) >> { >> struct vb2_buffer *vb; >> struct vb2_v4l2_buffer *vbuf; >> unsigned int i; >> >> if (WARN_ON(q->is_output)) >> return; >> if (list_empty(&q->queued_list)) >> return; >> vb = list_first_entry(&q->queued_list, struct vb2_buffer, queued_entry); >> list_del(&vb->queued_entry); >> for (i = 0; i < vb->num_planes; i++) >> vb2_set_plane_payload(vb, i, 0) >> vbuf = to_vb2_v4l2_buffer(vb); >> vbuf->flags |= V4L2_BUF_FLAG_LAST; >> vb2_buffer_done(vb, VB2_BUF_STATE_DONE); >> } >> EXPORT_SYMBOL_GPL(vb2_dqbuf_empty_last_buf); >> >> Neither compiled, nor tested, and I think this should be in v4l2-mem2mem.c instead of >> in videobuf2-v4l2.c since this is very m2m specific. >> >> So see this as a suggestion :-) >> >> Anyway, the key take-away from this is that userspace does not know if your driver >> behaves the way it does, so STOP should still produce a sane expected result. >> >> Which in this is just a single empty capture buffer marked LAST. > > Thanks, this makes sense. It doesn't quite fit the current usage > unfortunately as the test in v4l2-compliance goes like this: > > fail_on_test(doioctl(node, VIDIOC_DECODER_CMD, &cmd)); > fail_on_test(buf_cap.querybuf(node, 0)); > fail_on_test(buf_cap.qbuf(node)); > fail_on_test(buf_cap.dqbuf(node)); > fail_on_test(!(buf_cap.g_flags() & V4L2_BUF_FLAG_LAST)); > > Since the buffer is queued after issuing the stop cmd, it is not > possible to flag it as DONE in vdec_decoder_cmd. > > A solution would be to hijack vidioc_qbuf and flag the buffer if a > stop has been issued previously and the capture queue is not > streaming. Would that be okay ? >
I was able to pass the vanilla v4l2-compliance tests by hacking in the following 2 things:
1) Adding your function that I modified a bit (buffer had to be tagged as VB2_BUF_STATE_ACTIVE, it shouldn't be removed from the list, and q->owned_by_drv_count needs to be atomic_inc'd)
void vb2_dqbuf_empty_last_buf(struct vb2_queue *q) { struct vb2_buffer *vb; struct vb2_v4l2_buffer *vbuf; unsigned int i;
if (WARN_ON(q->is_output)) return; if (list_empty(&q->queued_list)) return;
vb = list_first_entry(&q->queued_list, struct vb2_buffer, queued_entry); for (i = 0; i < vb->num_planes; i++) vb2_set_plane_payload(vb, i, 0);
vb->state = VB2_BUF_STATE_ACTIVE; atomic_inc(&q->owned_by_drv_count); vbuf = to_vb2_v4l2_buffer(vb); vbuf->flags |= V4L2_BUF_FLAG_LAST; vb2_buffer_done(vb, VB2_BUF_STATE_DONE); }
2) Hijacking vidioc_qbuf to DONE the buffer if a stop was previously asked for, and if the capture queue isn't streaming:
static int vdec_qbuf(struct file *file, void *priv, struct v4l2_buffer *buf) { struct v4l2_fh *fh = file->private_data; struct amvdec_session *sess; struct vb2_queue *vq; int ret;
ret = v4l2_m2m_ioctl_qbuf(file, priv, buf); if (ret) return ret;
sess = container_of(file->private_data, struct amvdec_session, fh); vq = v4l2_m2m_get_vq(fh->m2m_ctx, buf->type); /* * If the capture queue isn't streaming and we were asked to * stop, DONE the buffer instantly */ if (!V4L2_TYPE_IS_OUTPUT(vq->type) && !sess->streamon_cap && sess->should_stop) vb2_dqbuf_empty_last_buf(vq);
return 0; }
Overall it feels quite messy :( . It doesn't look like a buffer is supposed to be dequeued if streaming hasn't started (they are tagged as VB2_BUF_STATE_ACTIVE only when streaming starts, and this flag is a requirement for vb2_buffer_done).
All this could be built in more properly into v4l2-mem2mem.c, though it would require the same hacks around VB2_BUF_STATE_ACTIVE and q->owned_by_drv_count. Or it would need a vb2 function specifically for this case, which would be very similar to vb2_buffer_done but allowing the state being VB2_BUF_STATE_QUEUED and not changing q->owned_by_drv_count.
>> >> Regards, >> >> Hans
| |