Message ID | 5D1EB136-0839-44BF-9F9B-A937237C9C96@noisolation.com (mailing list archive) |
---|---|
State | Changes Requested |
Delegated to: | Stanimir Varbanov |
Headers |
Received: from vger.kernel.org ([23.128.96.18]) by www.linuxtv.org with esmtp (Exim 4.92) (envelope-from <linux-media-owner@vger.kernel.org>) id 1pnGQy-001DAo-S8; Fri, 14 Apr 2023 10:13:37 +0000 Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S229591AbjDNKNe (ORCPT <rfc822;mkrufky@linuxtv.org> + 1 other); Fri, 14 Apr 2023 06:13:34 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:51358 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S229546AbjDNKNd (ORCPT <rfc822;linux-media@vger.kernel.org>); Fri, 14 Apr 2023 06:13:33 -0400 Received: from mail-lf1-x132.google.com (mail-lf1-x132.google.com [IPv6:2a00:1450:4864:20::132]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id E979A83E3 for <linux-media@vger.kernel.org>; Fri, 14 Apr 2023 03:12:55 -0700 (PDT) Received: by mail-lf1-x132.google.com with SMTP id i26so23077803lfc.6 for <linux-media@vger.kernel.org>; Fri, 14 Apr 2023 03:12:55 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=noisolation.com; s=google; t=1681467171; x=1684059171; h=to:cc:date:message-id:subject:mime-version :content-transfer-encoding:from:from:to:cc:subject:date:message-id :reply-to; bh=FpMSL9mpptWBMKkZ5mf+l7zqDIp0JFzhLiaDWfzh9X8=; b=Ymj89xA5rhr9u1CAAAtLQR4lXawVfThjnSc1hXbKeDOg2ysvxPY5hXgYHQ+ttOIbKu NZoBZyB54oo56clyFo0NUaiS80hkSWeL5s2+HQQEHMBpDxKhuWp3+ZyOYhr3iWLiWAPY Tvochj8EYsgj8EvK552VBeqD7qo5xRfeUPspbWHBGqesz6k/Nw/cQO+KUFb9cNT6wGEV jUHotE//fXDRwT4H/4KdBxaqZCePltM/EswSlceRxpmd9KInnYOiXmOZPh2J491FfqVz 4071meA6KKrWv1dgT1vojbqD75DDiqub6C6aiMazg+5cbXL/Hr2U7JJmcBlPAbhb+l9E kp1g== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20221208; t=1681467171; x=1684059171; h=to:cc:date:message-id:subject:mime-version :content-transfer-encoding:from:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=FpMSL9mpptWBMKkZ5mf+l7zqDIp0JFzhLiaDWfzh9X8=; b=T9A2HAL8FCe+C2NnRU2hckTmIEND7THka75EfBXe6ilk1dwcKwBm0erjzOTbGswrXG KwBJF1AOvv1K1kgADn/oCrMv1VDDUFjjuCOaYOlnLXEqoLgYcOEXNOappQNiGyzCm7R8 ydIkEMxNXX3aqMoxqi6nX55jnEPRo7/o6gEu3R8mUW70I5jFEHg2NM1Fdm9N6nbe7RQF wjMAK2mK11+kCyPtPMg5wtfPbIBN0eOqIlGwJVbnNvc+OS8Rt8FByi/W5TQlhiZlCMOY kN7uYaakpBW6ifSSYK8RlF+7MF2aDclpK+t7mDa4Ev8ryoSTv0G+8jfkXTRxQl++8Bgx Xv0A== X-Gm-Message-State: AAQBX9dDWZYamQJWM6t6UWuD/VBtqxuDprr1U34phQEw6PVwM/QVCjJz 2nFuUvK3nVLSxcQAbQnf5McPTfuBzkSWt5JuyjI= X-Google-Smtp-Source: AKy350ZLCEd53qnU6FCgK6pZDFtBParuu+MbnDYFygCjCg1tVm3sCOWUtXd12mUZOL9rAX+gNp9HqQ== X-Received: by 2002:a05:6512:21cd:b0:4ec:9c2e:7edb with SMTP id d13-20020a05651221cd00b004ec9c2e7edbmr1936996lft.42.1681467171024; Fri, 14 Apr 2023 03:12:51 -0700 (PDT) Received: from smtpclient.apple (108.224.92.62.static.cust.telenor.com. [62.92.224.108]) by smtp.gmail.com with ESMTPSA id d15-20020ac24c8f000000b004ec83665aeasm731886lfl.89.2023.04.14.03.12.50 (version=TLS1_2 cipher=ECDHE-ECDSA-AES128-GCM-SHA256 bits=128/128); Fri, 14 Apr 2023 03:12:50 -0700 (PDT) From: =?utf-8?q?Martin_D=C3=B8rum?= <dorum@noisolation.com> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable Mime-Version: 1.0 (Mac OS X Mail 16.0 \(3731.500.231\)) Subject: [PATCH] media: venus: only set H264_TRANSFORM_8X8 on supported hfi versions Message-Id: <5D1EB136-0839-44BF-9F9B-A937237C9C96@noisolation.com> Date: Fri, 14 Apr 2023 12:12:40 +0200 Cc: linux-media@vger.kernel.org, linux-arm-msm@vger.kernel.org To: stanimir.k.varbanov@gmail.com, quic_vgarodia@quicinc.com X-Mailer: Apple Mail (2.3731.500.231) X-Spam-Status: No, score=-2.1 required=5.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,RCVD_IN_DNSWL_NONE, SPF_HELO_NONE,SPF_PASS,T_SCC_BODY_TEXT_LINE autolearn=unavailable autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on lindbergh.monkeyblade.net Precedence: bulk List-ID: <linux-media.vger.kernel.org> X-Mailing-List: linux-media@vger.kernel.org X-LSpam-Score: -2.5 (--) X-LSpam-Report: No, score=-2.5 required=5.0 tests=BAYES_00=-1.9,DKIM_SIGNED=0.1,DKIM_VALID=-0.1,DKIM_VALID_AU=-0.1,HEADER_FROM_DIFFERENT_DOMAINS=0.5,MAILING_LIST_MULTI=-1 autolearn=ham autolearn_force=no |
Series |
media: venus: only set H264_TRANSFORM_8X8 on supported hfi versions
|
|
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
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 ||