Re: [PATCH v4] venus: Enable sufficient sequence change support for sc7180 and fix for Decoder STOP command issue.

From: Vikash Garodia
Date: Thu Mar 30 2023 - 00:35:17 EST


On 3/29/2023 11:03 PM, Dmitry Baryshkov wrote:
On Wed, 29 Mar 2023 at 20:16, Vikash Garodia <quic_vgarodia@xxxxxxxxxxx> wrote:

On 3/29/2023 7:06 PM, Dmitry Baryshkov wrote:
29 марта 2023 г. 10:48:23 GMT+03:00, Vikash Garodia <quic_vgarodia@xxxxxxxxxxx> пишет:
On 3/29/2023 3:49 AM, Dmitry Baryshkov wrote:
On 23/03/2023 15:01, Viswanath Boma wrote:
For VP9 bitstreams, there could be a change in resolution at interframe,
for driver to get notified of such resolution change,
enable the property in video firmware.
Also, EOS handling is now made same in video firmware across all V6 SOCs,
hence above a certain firmware version, the driver handling is
made generic for all V6s
Having "Do abc. Also do defgh." is a clear sign that this patch should be split into two.
I agree, it could have split into patches. The patch introduces way to store venus firmware

version and take some decision for various version. For ex. here STOP handling and enabling

DRC event for specific firmware revision and onwards. Since both the handling was primarily

dependent of firmware version, and since the handlings were smaller, it was combined as single

patch. Let me know, if you have any further review comments, else, will raise a new version with

2 patches probably.
Thanks!

Signed-off-by: Vikash Garodia <vgarodia@xxxxxxxxxxxxxxxx>
Signed-off-by: Viswanath Boma <quic_vboma@xxxxxxxxxxx>
Tested-by: Nathan Hebert <nhebert@xxxxxxxxxxxx>
---
Since v3 : Addressed comments to rectify email address.

drivers/media/platform/qcom/venus/core.h | 18 ++++++++++++++++++
drivers/media/platform/qcom/venus/hfi_cmds.c | 1 +
drivers/media/platform/qcom/venus/hfi_helper.h | 2 ++
drivers/media/platform/qcom/venus/hfi_msgs.c | 11 +++++++++--
drivers/media/platform/qcom/venus/vdec.c | 12 +++++++++++-
5 files changed, 41 insertions(+), 3 deletions(-)

(Skipped)



@@ -671,6 +671,16 @@ static int vdec_set_properties(struct venus_inst *inst)
return ret;
}
+ /* Enabling sufficient sequence change support for VP9 */
+ if (of_device_is_compatible(inst->core->dev->of_node, "qcom,sc7180-venus")) {
Let me repeat my question from v3:

Is it really specific just to sc7180 or will it be applicable to any
other platform using venus-5.4 firmware?
The HFI "HFI_PROPERTY_PARAM_VDEC_ENABLE_SUFFICIENT_SEQCHANGE_EVENT" is implemented

only for sc7180. Calling this for any other venus-5.4 would error out the session with error as

unsupported property from firmware.
How can we be sure that other platforms do not end up using sc7180 firmware? Or that sc7180 didn't end up using some other firmware?

I see generic qcom/venus-5.4/venus.mbn in Linux firmware. It's version is VIDEO.VE.5.4-00053-PROD-1. It can be used with any unfused device which uses firmware 5.4
Driver defines resources for every platforms and there it specifies the
firmware to be used for that platform. For ex, for sc7180, the firmware
is specified at [1].
And note that the firmware doesn't have an SoC name in it. This file
will be used by all unfused devices that use 5.4 firmware family.

The various firmware supported by different platforms are also available
in linux firmware.

[1]
https://elixir.bootlin.com/linux/v6.3-rc4/source/drivers/media/platform/qcom/venus/core.c#L765
And in that file sc7180 is the only platform having firmware 5.4.

I think that the check for sc7180 is redundant. Just check that the
firmware is from 5.4 family and it is 5.4.51 or newer.
I agree. sc7180 check can be removed as the feature is applicable for all 5.4 firmwares, irrespective of platform.

+ if (is_fw_rev_or_newer(inst->core, 5, 4, 51)) {
+ ptype = HFI_PROPERTY_PARAM_VDEC_ENABLE_SUFFICIENT_SEQCHANGE_EVENT;
+ ret = hfi_session_set_property(inst, ptype, &en);
+ if (ret)
+ return ret;
+ }
+ }
+
ptype = HFI_PROPERTY_PARAM_VDEC_CONCEAL_COLOR;
conceal = ctr->conceal_color & 0xffff;
conceal |= ((ctr->conceal_color >> 16) & 0xffff) << 10;