diff mbox series

media: venus: only set H264_TRANSFORM_8X8 on supported hfi versions

Message ID 5D1EB136-0839-44BF-9F9B-A937237C9C96@noisolation.com
State New
Headers show
Series media: venus: only set H264_TRANSFORM_8X8 on supported hfi versions | expand

Commit Message

Martin Dørum April 14, 2023, 10:12 a.m. UTC 
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(-)

--
2.34.1

Comments

Vikash Garodia May 2, 2023, 12:49 p.m. UTC | #1 
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@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.

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

>
> 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>
>
Bryan O'Donoghue May 2, 2023, 4:37 p.m. UTC | #2 
On 14/04/2023 11: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)) {
> +			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 (inst->fmt_cap->pixfmt == V4L2_PIX_FMT_H264 ||
> --
> 2.34.1

I agree that a Fixes should be added.

Fixes: bfee75f73c37 ("media: venus: venc: add support for 
V4L2_CID_MPEG_VIDEO_H264_8X8_TRANSFORM control")

When sending out your V2, please remember to cc -> Hans Verkuil 
<hverkuil-cisco@xs4all.nl>

Reviewed-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
Stanimir Varbanov May 2, 2023, 8:52 p.m. UTC | #3 
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;

         /* FOLLOWING PROPERTIES ARE NOT IMPLEMENTED IN CORE YET */

> +			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 (inst->fmt_cap->pixfmt == V4L2_PIX_FMT_H264 ||
> --
> 2.34.1
Vikash Garodia May 3, 2023, 4:25 a.m. UTC | #4 
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

> /* FOLLOWING PROPERTIES ARE NOT IMPLEMENTED IN CORE YET */
>
>> +            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 (inst->fmt_cap->pixfmt == V4L2_PIX_FMT_H264 ||
>> -- 
>> 2.34.1
>
Stanimir Varbanov May 3, 2023, 5:53 a.m. UTC | #5 
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
Vikash Garodia May 3, 2023, 6:47 a.m. UTC | #6 
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.
Bryan O'Donoghue May 12, 2023, 10:32 a.m. UTC | #7 
On 02/05/2023 17:37, Bryan O'Donoghue wrote:
> On 14/04/2023 11: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)) {
>> +            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 (inst->fmt_cap->pixfmt == V4L2_PIX_FMT_H264 ||
>> -- 
>> 2.34.1
> 
> I agree that a Fixes should be added.
> 
> Fixes: bfee75f73c37 ("media: venus: venc: add support for 
> V4L2_CID_MPEG_VIDEO_H264_8X8_TRANSFORM control")
> 
> When sending out your V2, please remember to cc -> Hans Verkuil 
> <hverkuil-cisco@xs4all.nl>
> 
> Reviewed-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>

Hey Martin.

I tried verifying the before/after of your patch last night on db410c as 
@ 
https://git.codelinaro.org/bryan.odonoghue/kernel/-/tree/linux-next-23-05-11-venus-check 


I don't see any difference with h264 playback with or without your patch.

Could you share a command to verify the bug against ?

---
bod
Vikash Garodia May 16, 2023, 12:33 p.m. UTC | #8 
On 5/12/2023 4:02 PM, Bryan O'Donoghue wrote:
> On 02/05/2023 17:37, Bryan O'Donoghue wrote:
>> On 14/04/2023 11: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)) {
>>> +            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 (inst->fmt_cap->pixfmt == V4L2_PIX_FMT_H264 ||
>>> -- 
>>> 2.34.1
>>
>> I agree that a Fixes should be added.
>>
>> Fixes: bfee75f73c37 ("media: venus: venc: add support for
>> V4L2_CID_MPEG_VIDEO_H264_8X8_TRANSFORM control")
>>
>> When sending out your V2, please remember to cc -> Hans Verkuil
>> <hverkuil-cisco@xs4all.nl>
>>
>> Reviewed-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
> 
> Hey Martin.
> 
> I tried verifying the before/after of your patch last night on db410c as @
> https://git.codelinaro.org/bryan.odonoghue/kernel/-/tree/linux-next-23-05-11-venus-check
> 
> I don't see any difference with h264 playback with or without your patch.
> 
> Could you share a command to verify the bug against ?

Probably try an encode session.

-Vikash
> 
> ---
> bod
Stanimir Varbanov May 25, 2023, 10:03 p.m. UTC | #9 
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?
diff mbox series

Patch

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)) {
+			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 (inst->fmt_cap->pixfmt == V4L2_PIX_FMT_H264 ||