Re: [PATCH 05/18] media: venus: hfi_venus: Sanitize venus_boot_core() per-VPU-version

[Dikshita Agarwal <quic_dikshita@xxxxxxxxxxx>]

On 3/2/2023 4:40 PM, Konrad Dybcio wrote:
> On 2.03.2023 12:00, Dikshita Agarwal wrote:
> > On 2/28/2023 8:54 PM, Konrad Dybcio wrote:
> > > The current assumption of IS_V6 is overgeneralized. Adjust the logic
> > > to take the VPU hardware version into account.
> > >
> > > Signed-off-by: Konrad Dybcio <konrad.dybcio@xxxxxxxxxx>
> > > ---
> > >    drivers/media/platform/qcom/venus/hfi_venus.c | 10 ++++++----
> > >    1 file changed, 6 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/drivers/media/platform/qcom/venus/hfi_venus.c b/drivers/media/platform/qcom/venus/hfi_venus.c
> > > index 4ccf31147c2a..772e5e9cf127 100644
> > > --- a/drivers/media/platform/qcom/venus/hfi_venus.c
> > > +++ b/drivers/media/platform/qcom/venus/hfi_venus.c
> > > @@ -448,20 +448,21 @@ static int venus_boot_core(struct venus_hfi_device *hdev)
> > >    {
> > >        struct device *dev = hdev->core->dev;
> > >        static const unsigned int max_tries = 100;
> > > -    u32 ctrl_status = 0, mask_val;
> > > +    u32 ctrl_status = 0, mask_val = 0;
> > >        unsigned int count = 0;
> > >        void __iomem *cpu_cs_base = hdev->core->cpu_cs_base;
> > >        void __iomem *wrapper_base = hdev->core->wrapper_base;
> > >        int ret = 0;
> > >          writel(BIT(VIDC_CTRL_INIT_CTRL_SHIFT), cpu_cs_base + VIDC_CTRL_INIT);
> > > -    if (IS_V6(hdev->core)) {
> > > +    if (IS_IRIS1(hdev->core) || IS_IRIS2(hdev->core) || IS_IRIS2_1(hdev->core)) {
> > I think the IRIS1 check can be removed from here as we are not handling IRIS1 related things at other places.
> >
> > we can add the required checks for IRIS1 when we add support for any IRIS1 based chipset.
> Up to you really, I plan on getting IRIS1 (SM8150) supported and have
> some mumbling going on for that on my local branch. FWIW these checks
> are logically correct and I would personally prefer not to have to go
> through each one of them and remove them just to bring them back soon.
>
> Konrad

oh, I see. I wasn't aware about the plan for IRIS1.

if you plan to add these checks on all relevant places then it should be 
fine.

Thanks,

Dikshita

> > Thanks,
> >
> > Dikshita
> >
> > >            mask_val = readl(wrapper_base + WRAPPER_INTR_MASK);
> > >            mask_val &= ~(WRAPPER_INTR_MASK_A2HWD_BASK_V6 |
> > >                      WRAPPER_INTR_MASK_A2HCPU_MASK);
> > >        } else {
> > >            mask_val = WRAPPER_INTR_MASK_A2HVCODEC_MASK;
> > >        }
> > > +
> > >        writel(mask_val, wrapper_base + WRAPPER_INTR_MASK);
> > >        writel(1, cpu_cs_base + CPU_CS_SCIACMDARG3);
> > >    @@ -480,10 +481,11 @@ static int venus_boot_core(struct venus_hfi_device *hdev)
> > >        if (count >= max_tries)
> > >            ret = -ETIMEDOUT;
> > >    -    if (IS_V6(hdev->core)) {
> > > +    if (IS_AR50_LITE(hdev->core) || IS_IRIS2(hdev->core) || IS_IRIS2_1(hdev->core))
> > >            writel(0x1, cpu_cs_base + CPU_CS_H2XSOFTINTEN_V6);
> > > +
> > > +    if (IS_IRIS2(hdev->core) || IS_IRIS2_1(hdev->core))
> > >            writel(0x0, cpu_cs_base + CPU_CS_X2RPMH_V6);
> > > -    }
> > >          return ret;
> > >    }