LKML Archive on lore.kernel.org
 help / color / Atom feed
From: Hans Verkuil <hverkuil@xs4all.nl>
To: Maxime Jourdan <mjourdan@baylibre.com>,
	Mauro Carvalho Chehab <mchehab@kernel.org>,
	Hans Verkuil <hans.verkuil@cisco.com>
Cc: Kevin Hilman <khilman@baylibre.com>,
	Jerome Brunet <jbrunet@baylibre.com>,
	Neil Armstrong <narmstrong@baylibre.com>,
	Martin Blumenstingl <martin.blumenstingl@googlemail.com>,
	linux-media@vger.kernel.org, linux-kernel@vger.kernel.org,
	linux-arm-kernel@lists.infradead.org,
	linux-amlogic@lists.infradead.org
Subject: Re: [PATCH 0/2] media: meson: vdec: Add compliant H264 support
Date: Wed, 9 Oct 2019 14:01:32 +0200
Message-ID: <fb6edb95-069e-abeb-416e-2327da0a87ab@xs4all.nl> (raw)
In-Reply-To: <acef4f1e-0b59-30f5-f31f-9fc22f393072@baylibre.com>

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.

Regards,

	Hans

  parent reply index

Thread overview: 18+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-10-07 14:59 Maxime Jourdan
2019-10-07 14:59 ` [PATCH 1/2] media: meson: vdec: bring up to compliance Maxime Jourdan
2019-10-16 12:38   ` Hans Verkuil
2019-10-07 14:59 ` [PATCH 2/2] media: meson: vdec: add H.264 decoding support Maxime Jourdan
2019-10-08 20:27   ` Nicolas Dufresne
2019-10-08 20:44     ` Nicolas Dufresne
2019-10-08 21:23       ` Maxime Jourdan
2019-10-07 15:12 ` [PATCH 0/2] media: meson: vdec: Add compliant H264 support Hans Verkuil
2019-10-07 16:24   ` Maxime Jourdan
2019-10-07 16:39     ` Hans Verkuil
2019-10-08 13:40       ` Maxime Jourdan
2019-10-08 13:45         ` Hans Verkuil
2019-10-09 12:01         ` Hans Verkuil [this message]
2019-10-18  7:50           ` Maxime Jourdan
2019-10-18  9:23             ` Maxime Jourdan
2019-10-21 13:13             ` Hans Verkuil
2019-10-13  1:08 ` Nicolas Dufresne
2019-10-13  7:30   ` Maxime Jourdan

Reply instructions:

You may reply publically to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=fb6edb95-069e-abeb-416e-2327da0a87ab@xs4all.nl \
    --to=hverkuil@xs4all.nl \
    --cc=hans.verkuil@cisco.com \
    --cc=jbrunet@baylibre.com \
    --cc=khilman@baylibre.com \
    --cc=linux-amlogic@lists.infradead.org \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-media@vger.kernel.org \
    --cc=martin.blumenstingl@googlemail.com \
    --cc=mchehab@kernel.org \
    --cc=mjourdan@baylibre.com \
    --cc=narmstrong@baylibre.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

LKML Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/lkml/0 lkml/git/0.git
	git clone --mirror https://lore.kernel.org/lkml/1 lkml/git/1.git
	git clone --mirror https://lore.kernel.org/lkml/2 lkml/git/2.git
	git clone --mirror https://lore.kernel.org/lkml/3 lkml/git/3.git
	git clone --mirror https://lore.kernel.org/lkml/4 lkml/git/4.git
	git clone --mirror https://lore.kernel.org/lkml/5 lkml/git/5.git
	git clone --mirror https://lore.kernel.org/lkml/6 lkml/git/6.git
	git clone --mirror https://lore.kernel.org/lkml/7 lkml/git/7.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 lkml lkml/ https://lore.kernel.org/lkml \
		linux-kernel@vger.kernel.org
	public-inbox-index lkml

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-kernel


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git