Re: [PATCH] media: venus: only set H264_TRANSFORM_8X8 on supported hfi versions

[Javier Martinez Canillas <javierm@redhat.com>]

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@noisolation.com>
> ---

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.

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@redhat.com>

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat