| Message ID | 5D1EB136-0839-44BF-9F9B-A937237C9C96@noisolation.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | media: venus: only set H264_TRANSFORM_8X8 on supported hfi versions | expand |
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>
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> >
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>
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
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 >
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
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.
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
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
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 --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 ||