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

[Stanimir Varbanov <stanimir.k.varbanov@gmail.com>]

Hi Martin,

On 3.05.23 г. 9:47 ч., Vikash Garodia wrote:
> 
> On 5/3/2023 11:23 AM, Stanimir Varbanov wrote:
>> Hi,
>>
>> On 3.05.23 г. 7:25 ч., Vikash Garodia wrote:
>>> On 5/3/2023 2:22 AM, Stanimir Varbanov wrote:
>>>>
>>>>
>>>> On 14.04.23 г. 13: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.
>>>>>
>>>>> Signed-off-by: Martin Dørum <dorum@noisolation.com>
>>>>>
>>>>> ---
>>>>>
>>>>> I have an APQ8016-based board. Before this patch, the Venus driver
>>>>> would simply fail with EINVAL when trying to request buffers
>>>>> (VIDIOC_REQBUFS). With this patch, encoding works
>>>>> (tested using gstreamer's v4l2h264enc).
>>>>>
>>>>>   drivers/media/platform/qcom/venus/venc.c | 21 +++++++++++----------
>>>>>   1 file changed, 11 insertions(+), 10 deletions(-)
>>>>>
>>>>> diff --git a/drivers/media/platform/qcom/venus/venc.c 
>>>>> b/drivers/media/platform/qcom/venus/venc.c
>>>>> index cdb12546c4fa..b3df805a8c9c 100644
>>>>> --- a/drivers/media/platform/qcom/venus/venc.c
>>>>> +++ b/drivers/media/platform/qcom/venus/venc.c
>>>>> @@ -672,16 +672,17 @@ static int venc_set_properties(struct 
>>>>> venus_inst *inst)
>>>>>           if (ret)
>>>>>               return ret;
>>>>>
>>>>> -        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;
>>>>> -
>>>>> +        if (!IS_V1(inst->core) && !IS_V3(inst->core)) {
>>>>
>>>> Instead of doing these checks here you could do:
>>>>
>>>> diff --git a/drivers/media/platform/qcom/venus/hfi_cmds.c 
>>>> b/drivers/media/platform/qcom/venus/hfi_cmds.c
>>>> index bc3f8ff05840..2453e5c3d244 100644
>>>> --- a/drivers/media/platform/qcom/venus/hfi_cmds.c
>>>> +++ b/drivers/media/platform/qcom/venus/hfi_cmds.c
>>>> @@ -1064,6 +1064,7 @@ static int pkt_session_set_property_1x(struct 
>>>> hfi_session_set_property_pkt *pkt,
>>>>                 break;
>>>>         }
>>>>         case HFI_PROPERTY_PARAM_VENC_HDR10_PQ_SEI:
>>>> +       case HFI_PROPERTY_PARAM_VENC_H264_TRANSFORM_8X8:
>>>>                 return -ENOTSUPP;
>>>>
>>> This may still deinit the session from [1] based on failure return 
>>> value.
>>>
>>> [1] 
>>> https://elixir.bootlin.com/linux/v6.3/source/drivers/media/platform/qcom/venus/venc.c#L963
>>
>> No, it will not fail because of:
>>
>> https://elixir.bootlin.com/linux/v6.3/source/drivers/media/platform/qcom/venus/hfi_venus.c#L1426
> 
> Thats correct, I missed to notice the explicit handling for -ENOTSUPP. 
> Above suggestion is better than keeping verison checks and
> 
> would also avoid deinit.
> 

Could you send v2 with suggested change?

-- 
regards,
Stan