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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAA8EJpotBtz8Dw9QsLX5Li2ON=QuxWFGZkaiYw9Ac-XerNEEag@mail.gmail.com>
Date:   Wed, 29 Mar 2023 20:33:54 +0300
From:   Dmitry Baryshkov <dmitry.baryshkov@...aro.org>
To:     Vikash Garodia <quic_vgarodia@...cinc.com>
Cc:     Andy Gross <agross@...nel.org>,
        Mauro Carvalho Chehab <mchehab@...nel.org>,
        linux-media@...r.kernel.org, linux-arm-msm@...r.kernel.org,
        linux-kernel@...r.kernel.org, stanimir.k.varbanov@...il.com,
        andersson@...nel.org, konrad.dybcio@...aro.org,
        quic_dikshita@...cinc.com, Viswanath Boma <quic_vboma@...cinc.com>
Subject: Re: [PATCH v4] venus: Enable sufficient sequence change support for
 sc7180 and fix for Decoder STOP command issue.

On Wed, 29 Mar 2023 at 20:16, Vikash Garodia <quic_vgarodia@...cinc.com> wrote:
>
>
> On 3/29/2023 7:06 PM, Dmitry Baryshkov wrote:
> > 29 марта 2023 г. 10:48:23 GMT+03:00, Vikash Garodia <quic_vgarodia@...cinc.com> пишет:
> >> 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@....qualcomm.com>
> >>>> Signed-off-by: Viswanath Boma <quic_vboma@...cinc.com>
> >>>> Tested-by: Nathan Hebert <nhebert@...omium.org>
> >>>> ---
> >>>> 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.

> >>>> +        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;



-- 
With best wishes
Dmitry