Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com>

Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com>

Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com>

Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com>

Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com>

The post-processing feature is meant to be used by decoding
only. Prevent encoding jobs from enabling it.

Fixes: 8c2d66b036c77 ("media: hantro: Support color conversion via post-processing")
Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com>
Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>

Philipp Zabel authored 5 years ago and Ezequiel Garcia committed 5 years ago

Add space for MVs and MC sync data to the capture buffers depending on
whether the post processor will be enabled for the new capture format
passed to TRY_FMT, not the currently set capture format.

Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
Reviewed-by: Ezequiel Garcia <ezequiel@collabora.com>
Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>

Jonas Karlman authored 5 years ago and Ezequiel Garcia committed 5 years ago

The RK3399 variant does not have postproc_regs declared,
this can cause a NULL pointer dereference trying to decode:

[   89.331359] Unable to handle kernel NULL pointer dereference at virtual address 0000000000000000

[   89.352804] Call trace:
[   89.353191]  hantro_postproc_disable+0x20/0xe8 [hantro_vpu]
[   89.354056]  hantro_start_prepare_run+0x58/0x68 [hantro_vpu]
[   89.354923]  hantro_h264_dec_prepare_run+0x30/0x6f0 [hantro_vpu]
[   89.355846]  rk3399_vpu_h264_dec_run+0x1c/0x14a8 [hantro_vpu]
[   89.356748]  device_run+0xa4/0xb8 [hantro_vpu]

Fix this by adding a NULL check in hantro_postproc_enable/disable.

Fixes: 8c2d66b036c7 ("media: hantro: Support color conversion via post-processing")
Signed-off-by: Jonas Karlman <jonas@kwiboo.se>
Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>

Dan Carpenter authored 5 years ago and Ezequiel Garcia committed 5 years ago

This can't be NULL and we've already dereferenced it so let's remove
the check.

Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>

The Hantro G1 decoder is able to enable a post-processor
on the decoding pipeline, which can be used to perform
scaling and color conversion.

The post-processor is integrated to the decoder, and it's
possible to use it in a way that is completely transparent
to the user.

This commit enables color conversion via post-processing,
which means the driver now exposes YUV packed, in addition to NV12.

Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com>
Reviewed-by: Boris Brezillon <boris.brezillon@collabora.com>
Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>

hantro_prepare_run() and hantro_finish_run() are
slightly misleading, so let's rename it to something
a bit more clear.

Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com>
Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>

Format negotiation helpers, hantro_find_format()
and hantro_get_default_fmt() can be simplified,
making the code a little bit clearer.

More importantly, this change is preparation work
for the post-processor usage.

Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com>
Reviewed-by: Philipp Zabel <p.zabel@pengutronix.de>
Reviewed-by: Boris Brezillon <boris.brezillon@collabora.com>
Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>

Jonas Karlman authored 5 years ago and Ezequiel Garcia committed 5 years ago

The FIELDPIC_FLAG_E bit should be set when field_pic_flag exists in stream,
it is currently set based on field_pic_flag of current frame.
The PIC_FIELDMODE_E bit is correctly set based on the field_pic_flag.

Fix this by setting the FIELDPIC_FLAG_E bit when frame_mbs_only is not set.

Fixes: dea0a82f ("media: hantro: Add support for H264 decoding on G1")
Signed-off-by: Jonas Karlman <jonas@kwiboo.se>
Reviewed-by: Boris Brezillon <boris.brezillon@collabora.com>
Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
Signed-off-by: Mauro Carvalho Chehab <mchehab@kernel.org>

Jonas Karlman authored 5 years ago and Ezequiel Garcia committed 5 years ago

pic_size in hantro_h264_dec_hw_ctx struct is no longer used,
lets remove it.

Signed-off-by: Jonas Karlman <jonas@kwiboo.se>
Reviewed-by: Boris Brezillon <boris.brezillon@collabora.com>
Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
Signed-off-by: Mauro Carvalho Chehab <mchehab@kernel.org>

Jonas Karlman authored 5 years ago and Ezequiel Garcia committed 5 years ago

Calculations for motion vector buffer offset is based on width and height
from the configured output format, lets use the same values for macroblock
width and height hw regs.

Signed-off-by: Jonas Karlman <jonas@kwiboo.se>
Reviewed-by: Boris Brezillon <boris.brezillon@collabora.com>
Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
Signed-off-by: Mauro Carvalho Chehab <mchehab@kernel.org>

Jonas Karlman authored 5 years ago and Ezequiel Garcia committed 5 years ago

A decoded 8-bit 4:2:0 frame need memory for up to 448 bytes per
macroblock with additional 32 bytes on multi-core variants.

Memory layout is as follow:

+---------------------------+
| Y-plane   256 bytes x MBs |
+---------------------------+
| UV-plane  128 bytes x MBs |
+---------------------------+
| MV buffer  64 bytes x MBs |
+---------------------------+
| MC sync          32 bytes |
+---------------------------+

Reduce the extra space allocated now that motion vector buffer offset no
longer is based on the extra space.

Only allocate extra space for 64 bytes x MBs of motion vector buffer
and 32 bytes for multi-core sync.

Fixes: a9471e25 ("media: hantro: Add core bits to support H264 decoding")
Signed-off-by: Jonas Karlman <jonas@kwiboo.se>
Reviewed-by: Boris Brezillon <boris.brezillon@collabora.com>
Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
Signed-off-by: Mauro Carvalho Chehab <mchehab@kernel.org>

Jonas Karlman authored 5 years ago and Ezequiel Garcia committed 5 years ago

A decoded 8-bit 4:2:0 frame need memory for up to 448 bytes per
macroblock and is laid out in memory as follow:

+---------------------------+
| Y-plane   256 bytes x MBs |
+---------------------------+
| UV-plane  128 bytes x MBs |
+---------------------------+
| MV buffer  64 bytes x MBs |
+---------------------------+

The motion vector buffer offset is currently correct for 4:2:0 because
the extra space for motion vectors is overallocated with an extra
64 bytes x MBs.

Wrong offset for both destination and motion vector buffer are used
for the bottom field of field encoded content, wrong offset is
also used for 4:0:0 (monochrome) content.

Fix this by setting the motion vector address to the expected 384 bytes
x MBs offset for 4:2:0 and 256 bytes x MBs offset for 4:0:0 content.

Also use correct destination and motion vector buffer offset
for the bottom field of field encoded content.

While at it also extend the check for 4:0:0 (monochrome) to include an
additional check for High Profile (100).

Fixes: dea0a82f ("media: hantro: Add support for H264 decoding on G1")
Signed-off-by: Jonas Karlman <jonas@kwiboo.se>
Reviewed-by: Boris Brezillon <boris.brezillon@collabora.com>
Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
Signed-off-by: Mauro Carvalho Chehab <mchehab@kernel.org>

Commit 953aaa14 ("media: rockchip/vpu: Prepare things to support decoders")
changed the conditions under S_FMT was allowed for OUTPUT
CAPTURE buffers.

However, and according to the mem-to-mem stateless decoder specification,
in order to support dynamic resolution changes, S_FMT should be allowed
even if OUTPUT buffers have been allocated.

Relax decoder S_FMT restrictions on OUTPUT buffers, allowing a
resolution modification, provided the pixel format stays the same.

Tested on RK3288 platforms using ChromiumOS Video Decode/Encode
Accelerator Unittests.

[hverkuil: fix typo: In other -> In order]

Fixes: 953aaa14 ("media: rockchip/vpu: Prepare things to support decoders")
Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com>
Reviewed-by: Boris Brezillon <boris.brezillon@collabora.com>
Cc: <stable@vger.kernel.org>      # for v5.4 and up
Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
Signed-off-by: Mauro Carvalho Chehab <mchehab@kernel.org>

Francois Buergisser authored 5 years ago and Ezequiel Garcia committed 5 years ago

The picture order count table only makes sense for profiles
higher than Baseline. This is confirmed by the H.264 specification
(See 8.2.1 Decoding process for picture order count), which
clarifies how POC are used for features not present in Baseline.

"""
Picture order counts are used to determine initial picture orderings
for reference pictures in the decoding of B slices, to represent picture
order differences between frames or fields for motion vector derivation
in temporal direct mode, for implicit mode weighted prediction in B slices,
and for decoder conformance checking.
"""

As a side note, this change matches various vendors downstream codebases,
including ChromiumOS and IMX VPU libraries.

Fixes: dea0a82f ("media: hantro: Add support for H264 decoding on G1")
Signed-off-by: Francois Buergisser <fbuergisser@chromium.org>
Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com>
Signed-off-by: Jonas Karlman <jonas@kwiboo.se>
Reviewed-by: Boris Brezillon <boris.brezillon@collabora.com>
Tested-by: Boris Brezillon <boris.brezillon@collabora.com>
Cc: <stable@vger.kernel.org>      # for v5.4 and up
Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
Signed-off-by: Mauro Carvalho Chehab <mchehab@kernel.org>

Francois Buergisser authored 5 years ago and Ezequiel Garcia committed 5 years ago

The setting of the motion vectors usage and the setting of motion
vectors address are currently done under different conditions.

When decoding pre-recorded videos, this results of leaving the motion
vectors address unset, resulting in faulty memory accesses. Fix it
by using the same condition everywhere, which matches the profiles
that support motion vectors.

Fixes: dea0a82f ("media: hantro: Add support for H264 decoding on G1")
Signed-off-by: Francois Buergisser <fbuergisser@chromium.org>
Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com>
Signed-off-by: Jonas Karlman <jonas@kwiboo.se>
Reviewed-by: Boris Brezillon <boris.brezillon@collabora.com>
Tested-by: Boris Brezillon <boris.brezillon@collabora.com>
Cc: <stable@vger.kernel.org>      # for v5.4 and up
Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
Signed-off-by: Mauro Carvalho Chehab <mchehab@kernel.org>

Jonas Karlman authored 5 years ago and Ezequiel Garcia committed 5 years ago

TRM specify supported image size 48x48 to 4096x2304 at step size 16 pixels,
change frmsize max_width/max_height to match TRM at [1].

This patch makes it possible to decode the 4096x2304 sample at [2].

[1] http://www.t-firefly.com/download/firefly-rk3288/docs/TRM/rk3288-chapter-25-video-encoder-decoder-unit-(vcodec).pdf
[2] https://4ksamples.com/puppies-bath-in-4k/



Fixes: 76032793 ("media: hantro: Enable H264 decoding on rk3288")
Signed-off-by: Jonas Karlman <jonas@kwiboo.se>
Reviewed-by: Boris Brezillon <boris.brezillon@collabora.com>
Tested-by: Boris Brezillon <boris.brezillon@collabora.com>
Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
Signed-off-by: Mauro Carvalho Chehab <mchehab@kernel.org>

Jonas Karlman authored 5 years ago and Ezequiel Garcia committed 5 years ago

Scaling list supplied from userspace should be in matrix order
and can be used without applying the inverse scanning process.

The HW also only support 8x8 scaling list for the Y component, indices 0
and 1 in the scaling list supplied from userspace.

Remove reordering and write the scaling matrix in an order expected by
the VPU, also only allocate memory for the two 8x8 lists supported.

Fixes: a9471e25 ("media: hantro: Add core bits to support H264 decoding")
Signed-off-by: Jonas Karlman <jonas@kwiboo.se>
Reviewed-by: Philipp Zabel <p.zabel@pengutronix.de>
Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
Signed-off-by: Mauro Carvalho Chehab <mchehab@kernel.org>

Boris Brezillon authored 5 years ago and Ezequiel Garcia committed 5 years ago

Step '8.2.4.1 Decoding process for picture numbers' was missing in the
reflist creation logic, leading to invalid P reflists when a
->frame_num wraparound happens.

Fixes: a9471e25 ("media: hantro: Add core bits to support H264 decoding")
Reported-by: Francois Buergisser <fbuergisser@google.com>
Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>
Reviewed-by: Philipp Zabel <p.zabel@pengutronix.de>
Tested-by: Philipp Zabel <p.zabel@pengutronix.de>
Tested-by: Francois Buergisser <fbuergisser@chromium.org>
Reviewed-by: Tomasz Figa <tfiga@chromium.org>
Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
Signed-off-by: Mauro Carvalho Chehab <mchehab+samsung@kernel.org>

Boris Brezillon authored 5 years ago and Ezequiel Garcia committed 5 years ago

And use it for all native type comparisons, even if it's not strictly
required. By doing that we make the code more consistent and prevent
from potential incorrect results in case of overflow or when the the
values being compared are both negative.

Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>
Tested-by: Francois Buergisser <fbuergisser@chromium.org>
Reviewed-by: Tomasz Figa <tfiga@chromium.org>
Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
Signed-off-by: Mauro Carvalho Chehab <mchehab+samsung@kernel.org>

Boris Brezillon authored 5 years ago and Ezequiel Garcia committed 5 years ago

So it matches the code and the spec.

Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>
Tested-by: Francois Buergisser <fbuergisser@chromium.org>
Reviewed-by: Tomasz Figa <tfiga@chromium.org>
Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
Signed-off-by: Mauro Carvalho Chehab <mchehab+samsung@kernel.org>

Modify hantro_get_ref() and hantro_h264_get_ref_buf() helpers
to return the buffer DMA address, this makes the code simpler
and at the same time will allow easier enablement of the
post-processing feature.

Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com>
Reviewed-by: Philipp Zabel <p.zabel@pengutronix.de>
Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
Signed-off-by: Mauro Carvalho Chehab <mchehab+samsung@kernel.org>

It seems all codecs are using a 16x16 size macroblock,
and so it's possible to have just one set of macroblock macros.

Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com>
Reviewed-by: Philipp Zabel <p.zabel@pengutronix.de>
Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
Signed-off-by: Mauro Carvalho Chehab <mchehab+samsung@kernel.org>

Philipp Zabel authored 5 years ago and Ezequiel Garcia committed 5 years ago

This adds a label to the error path to avoid calling
v4l2_m2m_ctx_init() and kfree(ctx) in two places each.

Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
Signed-off-by: Mauro Carvalho Chehab <mchehab+samsung@kernel.org>

Suggested-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>
Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com>

TODO: Add bloat-o-meter comment and explain why.

This goes upstream.

Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com>

Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com>

Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com>

Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com>

Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com>

Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com>

Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com>

The driver currently creates a broken topology,
with a source-to-source link and a sink-to-sink
link instead of two source-to-sink links.

Reported-by: Nicolas Dufresne <nicolas@ndufresne.ca>
Cc: <stable@vger.kernel.org>      # for v5.3 and up
Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com>

Hans Verkuil authored 5 years ago and Ezequiel Garcia committed 5 years ago

These are bits so to test if a pad is a sink you use & but not ==.

It looks like the only reason this hasn't caused problems before is that
media_get_pad_index() is currently only used with pads that do not set the
MEDIA_PAD_FL_MUST_CONNECT flag. So a pad really had only the SINK or SOURCE
flag set and nothing else.

Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
Cc: <stable@vger.kernel.org>      # for v5.3 and up

Hans Verkuil authored 5 years ago and Ezequiel Garcia committed 5 years ago

Use WARN_ON instead of BUG_ON.

Add two new WARN_ONs to verify that the source pad is really a source
and that the sink pad is really a sink.

Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>

Hans Verkuil authored 5 years ago and Ezequiel Garcia committed 5 years ago

The topology that v4l2_m2m_register_media_controller() creates for a
processing block actually created a source-to-source link and a sink-to-sink
link instead of two source-to-sink links.

Unfortunately v4l2-compliance never checked for such bad links, so this
went unreported for quite some time.

Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
Reported-by: Nicolas Dufresne <nicolas@ndufresne.ca>
Cc: <stable@vger.kernel.org>      # for v4.19 and up