Re: [PATCH 0/2] media: meson: vdec: Add compliant H264 support

[Maxime Jourdan]

On Wed, Oct 9, 2019 at 2:01 PM Hans Verkuil <hverkuil@xxxxxxxxx> 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 ?

Maxime

>
> Regards,
>
>         Hans