On 4/27/2023 8:13 PM, Javier Martinez Canillas wrote:
Hello Martin,
On 4/14/23 12:12, Martin Dørum wrote:
Setting the H264_TRANSFORM_8X8 property only works on HFI versions
=4xx. The code used to unconditionally set the property in
venc_set_properties, which meant that initializing the encoder would
always fail unless the hfi_version was >=4xx.
This patch changes venc_set_properties to only set the
H264_TRANSFORM_8X8 property if the hfi version is >=4xx.
I would add a
Fixes: bfee75f73c37 ("media: venus: venc: add support for V4L2_CID_MPEG_VIDEO_H264_8X8_TRANSFORM control")
since that is the commit that added this property and expected it to
be used unconditionally in the common venus venc part.
Signed-off-by: Martin Dørum <dorum@xxxxxxxxxxxxxxx>
---
I'm not familiar with the venus encoder driver but I had fixed a couple
of bugs on the venus decoder so I've spent some time looking at the code.
[...]
+ if (!IS_V1(inst->core) && !IS_V3(inst->core)) {
+ ptype = HFI_PROPERTY_PARAM_VENC_H264_TRANSFORM_8X8;
+ h264_transform.enable_type = 0;
+ if (ctr->profile.h264 == V4L2_MPEG_VIDEO_H264_PROFILE_HIGH ||
+ ctr->profile.h264 == V4L2_MPEG_VIDEO_H264_PROFILE_CONSTRAINED_HIGH)
+ h264_transform.enable_type = ctr->h264_8x8_transform;
+
+ ret = hfi_session_set_property(inst, ptype, &h264_transform);
+ if (ret)
+ return ret;
+ }
Is true that HFI_PROPERTY_PARAM_VENC_H264_TRANSFORM_8X8 isn't wired for
older HFI versions, but I wonder if that's something that was forgotten
when the property was added in that commit or instead should be ignored
as you do in your patch.
This HFI is supported on earlier version (atleast for V3). The code [1]
returns from packetization layer
with -EINVAL as there was no need to set the same. Infact, 8x8 transform
is auto applied in video firmware
for High/Constrained high profile encoding. Later there were request
from clients as they wanted to disable
this by setting false with this HFI. So it was added later for V4 and later.
[1]
https://elixir.bootlin.com/linux/v6.3/source/drivers/media/platform/qcom/venus/hfi_cmds.c#L1090
Reviewed-by: Vikash Garodia <quic_vgarodia@xxxxxxxxxxx>
In any case, this fixes a regression that you are experiencing so your
patch should land in my opinion and later can be added to older versions
if that is the correct thing to do.
Acked-by: Javier Martinez Canillas <javierm@xxxxxxxxxx>
