| Message ID | 20230228-topic-venus-v1-0-58c2c88384e9@linaro.org |
|---|---|
| Headers | show |
| Series | Venus QoL / maintainability fixes | expand |
On 2/28/2023 10:23 PM, Bryan O'Donoghue wrote: > On 28/02/2023 15:41, Konrad Dybcio wrote: >>> Can you test it and make sure ? >> As I mentioned in the cover letter, 8250 still seems to work with this >> patchset. I have no idea how one would go about validating the >> functionality enabled through this call. > > We offlined about this. > > I think it is correct to say you don't have access to a display to > test this on sm8250. > > I do so, I will try this out for you, though I'll wait for your V2 for > this series. > > --- > bod Hi Konrad, I understand from your commit text, setting this indicator for AR50L is causing issue with suspend. Ideally it shouldn't cause any such issue. I checked with FW team and got to know that this property is not supported on AR50LT so if you set it there should be some property not supported error. In my opinion it would be good to replace these versions checks with VPU version check you have introduced in your other patch and keep this setting for current targets and not set wherever not needed eg AR50LT. Thanks, Dikshita
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@linaro.org> > --- > 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. 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; > } >
On 2.03.2023 07:39, Dikshita Agarwal wrote: > > On 2/28/2023 10:23 PM, Bryan O'Donoghue wrote: >> On 28/02/2023 15:41, Konrad Dybcio wrote: >>>> Can you test it and make sure ? >>> As I mentioned in the cover letter, 8250 still seems to work with this >>> patchset. I have no idea how one would go about validating the >>> functionality enabled through this call. >> >> We offlined about this. >> >> I think it is correct to say you don't have access to a display to test this on sm8250. >> >> I do so, I will try this out for you, though I'll wait for your V2 for this series. >> >> --- >> bod > > Hi Konrad, > > I understand from your commit text, setting this indicator for AR50L is causing issue with suspend. > > Ideally it shouldn't cause any such issue. I checked with FW team and got to know that this property is not supported on AR50LT so if you set it there should be some property not supported error. > > In my opinion it would be good to replace these versions checks with VPU version check you have introduced in your other patch and keep this setting for current targets and not set wherever not needed eg AR50LT. Okay, I have two questions then: 1. Can the firmware team confirm it shouldn't crash on a fw tag that's close to VIDEO.VE.6.0-00042-PROD-1? 2. Are there any other SoCs that SYS_IDLE is not supported on? It hasn't been sent to the hardware by the vidc driver for any SoC using the downstream vidc (NOT the legacy vidc_3x) driver starting with msm-4.14, AFAICS.. check out this link: https://github.com/sonyxperiadev/kernel/commit/8889a7d26943e96eae2f318f87b15efa4b907f28 Konrad > > Thanks, > > Dikshita >
On 3/2/2023 5:03 PM, Konrad Dybcio wrote: > > On 2.03.2023 07:39, Dikshita Agarwal wrote: >> On 2/28/2023 10:23 PM, Bryan O'Donoghue wrote: >>> On 28/02/2023 15:41, Konrad Dybcio wrote: >>>>> Can you test it and make sure ? >>>> As I mentioned in the cover letter, 8250 still seems to work with this >>>> patchset. I have no idea how one would go about validating the >>>> functionality enabled through this call. >>> We offlined about this. >>> >>> I think it is correct to say you don't have access to a display to test this on sm8250. >>> >>> I do so, I will try this out for you, though I'll wait for your V2 for this series. >>> >>> --- >>> bod >> Hi Konrad, >> >> I understand from your commit text, setting this indicator for AR50L is causing issue with suspend. >> >> Ideally it shouldn't cause any such issue. I checked with FW team and got to know that this property is not supported on AR50LT so if you set it there should be some property not supported error. >> >> In my opinion it would be good to replace these versions checks with VPU version check you have introduced in your other patch and keep this setting for current targets and not set wherever not needed eg AR50LT. > Okay, I have two questions then: > > 1. Can the firmware team confirm it shouldn't crash on a fw tag > that's close to VIDEO.VE.6.0-00042-PROD-1? > > 2. Are there any other SoCs that SYS_IDLE is not supported on? > It hasn't been sent to the hardware by the vidc driver for > any SoC using the downstream vidc (NOT the legacy vidc_3x) > driver starting with msm-4.14, AFAICS.. check out this link: > > https://github.com/sonyxperiadev/kernel/commit/8889a7d26943e96eae2f318f87b15efa4b907f28 > > Konrad Yes, that's correct, I have already confirmed from FW team that this idle check is always enabled in FW now. so driver doesn't have to set itexplicitly, that's why it works for you on 8250 I believe. So removing this setting seems fine. My only concern is that why we see a crash when setting it on AR50LT. Thanks, Dikshita >> Thanks, >> >> Dikshita >>
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@linaro.org> >>> --- >>> 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; >>> } >>>
On 2/28/2023 8:54 PM, Konrad Dybcio wrote: > IRIS2(_1) has a different register map compared to other HFI6XX- > using VPUs. Take care of it. > > Signed-off-by: Konrad Dybcio<konrad.dybcio@linaro.org> > --- > drivers/media/platform/qcom/venus/core.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/media/platform/qcom/venus/core.c b/drivers/media/platform/qcom/venus/core.c > index c13436d58ed3..bdc14acc8399 100644 > --- a/drivers/media/platform/qcom/venus/core.c > +++ b/drivers/media/platform/qcom/venus/core.c > @@ -246,7 +246,7 @@ static int venus_enumerate_codecs(struct venus_core *core, u32 type) > > static void venus_assign_register_offsets(struct venus_core *core) > { > - if (IS_V6(core)) { > + if (IS_IRIS2(core) || IS_IRIS2_1(core)) { > core->vbif_base = core->base + VBIF_BASE; > core->cpu_base = core->base + CPU_BASE_V6; > core->cpu_cs_base = core->base + CPU_CS_BASE_V6; AR50_LITE also should be added here, as I see you have added the same to places where we are using V6 based registers. if the base addresses are not assigned here properly. the register writing at other places will be wrong, ex: patch 05/18 Thanks, Dikshita
On 2/28/2023 8:54 PM, Konrad Dybcio wrote: > IS_V6 was used there IS_IRIS2(_1) should have been and the !IS_V6 > condition was only correct by luck and for now. Replace them both > with VPU version checks. > > Fixes: 24fcc0522d87 ("media: venus: hfi: Add 6xx interrupt support") > Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org> > --- > drivers/media/platform/qcom/venus/hfi_venus.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/media/platform/qcom/venus/hfi_venus.c b/drivers/media/platform/qcom/venus/hfi_venus.c > index 0d137e070407..ecfbac36de20 100644 > --- a/drivers/media/platform/qcom/venus/hfi_venus.c > +++ b/drivers/media/platform/qcom/venus/hfi_venus.c > @@ -1136,7 +1136,7 @@ static irqreturn_t venus_isr(struct venus_core *core) > wrapper_base = hdev->core->wrapper_base; > > status = readl(wrapper_base + WRAPPER_INTR_STATUS); > - if (IS_V6(core)) { > + if (IS_IRIS2(core) || IS_IRIS2_1(core)) { > if (status & WRAPPER_INTR_STATUS_A2H_MASK || > status & WRAPPER_INTR_STATUS_A2HWD_MASK_V6 || > status & CPU_CS_SCIACMDARG0_INIT_IDLE_MSG_MASK) > @@ -1148,7 +1148,7 @@ static irqreturn_t venus_isr(struct venus_core *core) > hdev->irq_status = status; > } > writel(1, cpu_cs_base + CPU_CS_A2HSOFTINTCLR); > - if (!IS_V6(core)) > + if (!(IS_AR50_LITE(core) || IS_IRIS2(core) || IS_IRIS2_1(core))) > writel(status, wrapper_base + WRAPPER_INTR_CLEAR); > > return IRQ_WAKE_THREAD; reviewed-by: Dikshita Agarwal <quic_dikshita@quicinc.com>
On 3/7/2023 11:42 AM, Dikshita Agarwal wrote: > > On 2/28/2023 8:54 PM, Konrad Dybcio wrote: >> IS_V6 was used there IS_IRIS2(_1) should have been and the !IS_V6 >> condition was only correct by luck and for now. Replace them both >> with VPU version checks. >> >> Fixes: 24fcc0522d87 ("media: venus: hfi: Add 6xx interrupt support") >> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org> >> --- >> drivers/media/platform/qcom/venus/hfi_venus.c | 4 ++-- >> 1 file changed, 2 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/media/platform/qcom/venus/hfi_venus.c >> b/drivers/media/platform/qcom/venus/hfi_venus.c >> index 0d137e070407..ecfbac36de20 100644 >> --- a/drivers/media/platform/qcom/venus/hfi_venus.c >> +++ b/drivers/media/platform/qcom/venus/hfi_venus.c >> @@ -1136,7 +1136,7 @@ static irqreturn_t venus_isr(struct venus_core >> *core) >> wrapper_base = hdev->core->wrapper_base; >> status = readl(wrapper_base + WRAPPER_INTR_STATUS); >> - if (IS_V6(core)) { >> + if (IS_IRIS2(core) || IS_IRIS2_1(core)) { >> if (status & WRAPPER_INTR_STATUS_A2H_MASK || >> status & WRAPPER_INTR_STATUS_A2HWD_MASK_V6 || >> status & CPU_CS_SCIACMDARG0_INIT_IDLE_MSG_MASK) >> @@ -1148,7 +1148,7 @@ static irqreturn_t venus_isr(struct venus_core >> *core) >> hdev->irq_status = status; >> } >> writel(1, cpu_cs_base + CPU_CS_A2HSOFTINTCLR); >> - if (!IS_V6(core)) >> + if (!(IS_AR50_LITE(core) || IS_IRIS2(core) || IS_IRIS2_1(core))) >> writel(status, wrapper_base + WRAPPER_INTR_CLEAR); >> return IRQ_WAKE_THREAD; this change looks good to me , once base register values are fixed in other patch. reviewed-by: Dikshita Agarwal <quic_dikshita@quicinc.com>
On 7.03.2023 05:57, Dikshita Agarwal wrote: > > On 2/28/2023 8:54 PM, Konrad Dybcio wrote: >> IRIS2(_1) has a different register map compared to other HFI6XX- >> using VPUs. Take care of it. >> >> Signed-off-by: Konrad Dybcio<konrad.dybcio@linaro.org> >> --- >> drivers/media/platform/qcom/venus/core.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/drivers/media/platform/qcom/venus/core.c b/drivers/media/platform/qcom/venus/core.c >> index c13436d58ed3..bdc14acc8399 100644 >> --- a/drivers/media/platform/qcom/venus/core.c >> +++ b/drivers/media/platform/qcom/venus/core.c >> @@ -246,7 +246,7 @@ static int venus_enumerate_codecs(struct venus_core *core, u32 type) >> static void venus_assign_register_offsets(struct venus_core *core) >> { >> - if (IS_V6(core)) { >> + if (IS_IRIS2(core) || IS_IRIS2_1(core)) { >> core->vbif_base = core->base + VBIF_BASE; >> core->cpu_base = core->base + CPU_BASE_V6; >> core->cpu_cs_base = core->base + CPU_CS_BASE_V6; > > AR50_LITE also should be added here, as I see you have added the same to places where we are using V6 based registers. > > if the base addresses are not assigned here properly. the register writing at other places will be wrong, ex: patch 05/18 I have a separate patch set which specifically adds AR50L data, and they're not 1:1, vbif_base and aon_base are gone (at least according to techpack/video). I intend to push it when I get it all working, but here's what it looks like right now: diff --git a/drivers/media/platform/qcom/venus/core.c b/drivers/media/platform/qcom/venus/core.c index fd9ecb1f7a05..f88b4781c5d0 100644 --- a/drivers/media/platform/qcom/venus/core.c +++ b/drivers/media/platform/qcom/venus/core.c @@ -254,6 +254,14 @@ static void venus_assign_register_offsets(struct venus_core *core) core->wrapper_base = core->base + WRAPPER_BASE_V6; core->wrapper_tz_base = core->base + WRAPPER_TZ_BASE_V6; core->aon_base = core->base + AON_BASE_V6; + } else if (IS_AR50_LITE(core)) { + core->vbif_base = NULL; + core->cpu_base = core->base + CPU_BASE_V6; + core->cpu_cs_base = core->base + CPU_CS_BASE_V6; + core->cpu_ic_base = core->base + CPU_IC_BASE_V6; + core->wrapper_base = core->base + WRAPPER_BASE_V6; + core->wrapper_tz_base = core->base + WRAPPER_TZ_BASE_V6; + core->aon_base = NULL; } else { core->vbif_base = core->base + VBIF_BASE; core->cpu_base = core->base + CPU_BASE;
On 2.03.2023 07:39, Dikshita Agarwal wrote: > > On 2/28/2023 10:23 PM, Bryan O'Donoghue wrote: >> On 28/02/2023 15:41, Konrad Dybcio wrote: >>>> Can you test it and make sure ? >>> As I mentioned in the cover letter, 8250 still seems to work with this >>> patchset. I have no idea how one would go about validating the >>> functionality enabled through this call. >> >> We offlined about this. >> >> I think it is correct to say you don't have access to a display to test this on sm8250. >> >> I do so, I will try this out for you, though I'll wait for your V2 for this series. >> >> --- >> bod > > Hi Konrad, > > I understand from your commit text, setting this indicator for AR50L is causing issue with suspend. > > Ideally it shouldn't cause any such issue. I checked with FW team and got to know that this property is not supported on AR50LT so if you set it there should be some property not supported error. > > In my opinion it would be good to replace these versions checks with VPU version check you have introduced in your other patch and keep this setting for current targets and not set wherever not needed eg AR50LT. So.. I did *something* and I'm no longer getting a jump to EDL. The *something* being knocking off hfi_core_suspend(). If I send a sys_idle_indicator = true, I get (reformatted for better legibility): [ 0.576543] qcom-venus 5a00000.video-codec: VenusFW : <VFW_H:HostDr:unkn:--------:-> IMAGE_VARIANT_STRING=PROD [ 0.603818] qcom-venus 5a00000.video-codec: VenusFW : <VFW_H:HostDr:unkn:--------:-> OEM_IMAGE_VERSION_STRING=CRM [ 0.608633] qcom-venus 5a00000.video-codec: VenusFW : <VFW_H:HostDr:unkn:--------:-> BUILD_TIME: Mar 15 2021 04:24:58 [ 0.608644] qcom-venus 5a00000.video-codec: VenusFW : <VFW_L:HostDr:unkn:--------:-> Host cmd 0x10005 [ 0.608655] qcom-venus 5a00000.video-codec: VenusFW : <VFW_E:HostDr:unkn:--------:-> VenusHostDriver_SetSysProperty(1019): HostDriver: VenusHostDriver_SetSysProperty unsupport property! [ 0.608667] qcom-venus 5a00000.video-codec: VenusFW : <VFW_E:HostDr:unkn:--------:-> WaitForHWidle(408): VENUS is idle, no HW is running [ 0.650759] qcom-venus 5a00000.video-codec: VenusFW : <VFW_E:HostDr:unkn:--------:-> assert_loop(433): FW Assertion - Z:/b/venus_proc/venus/drivers/src/VenusHostDriver.c:1020:5ab9a Which then crashes Venus for good (perhaps we're missing a handler for such errors that would hard reset the hw), meaning trying to access it through ffmpeg will result in it never firing any IRQs, so no submitted commands ever complete. With this information, after uncommenting the hfi_core_suspend call and changing: [1] --- hfi_venus.c : venus_suspend_3xx() -- - venus_prepare_power_collapse(hdev, true); + venus_prepare_power_collapse(hdev, false); ---------------------------------------- I was able to test further. Turning the ARM9 core off messes with the sys_idle things. Perhaps some power sequencing is wrong. The diff I just mentioned comes from the fact that AR50L will never ever ever send a PC_PREP_DONE ack, or at least downstream never expects it (or any other HFI6XX target FWIW) to do so. Now, I also realized the adjacent set_power_control doesn't seem to be used at all on msm-4.19 techpack/video. Testing all the possible combinations, I get (to make it extra clear, with all the powerdown stuff in place and only diff [1] in place atop what I already had before): [set_idle_message] [set_power_control] [result] 0 0 - no crash at boot, venus doesn't work -> "Too many packets buffered for output stream 0:1." 0 1 - no crash at boot, ffmpeg hangs near vdec session init -> jump to EDL shortly after 1 0 - hang at boot, even before display subsys initializes -> platform totally hangs 1 1 - same as (1, 0), probably due to sys_idle_indicator being on -> platform totally hangs as well Perhaps (0, 0) is "good" and things can be worked up from there? Can you recheck with the firmware team if this is expected? Konrad > > Thanks, > > Dikshita >
On 30.03.2023 12:44, Vikash Garodia wrote: > On 3/24/2023 2:46 PM, Dikshita Agarwal wrote: >> >> >> On 3/20/2023 8:24 PM, Konrad Dybcio wrote: >>> On 2.03.2023 07:39, Dikshita Agarwal wrote: >>>> On 2/28/2023 10:23 PM, Bryan O'Donoghue wrote: >>>>> On 28/02/2023 15:41, Konrad Dybcio wrote: >>>>>>> Can you test it and make sure ? >>>>>> As I mentioned in the cover letter, 8250 still seems to work with this >>>>>> patchset. I have no idea how one would go about validating the >>>>>> functionality enabled through this call. >>>>> We offlined about this. >>>>> >>>>> I think it is correct to say you don't have access to a display to test this on sm8250. >>>>> >>>>> I do so, I will try this out for you, though I'll wait for your V2 for this series. >>>>> >>>>> --- >>>>> bod >>>> Hi Konrad, >>>> >>>> I understand from your commit text, setting this indicator for AR50L is causing issue with suspend. >>>> >>>> Ideally it shouldn't cause any such issue. I checked with FW team and got to know that this property is not supported on AR50LT so if you set it there should be some property not supported error. >>>> >>>> In my opinion it would be good to replace these versions checks with VPU version check you have introduced in your other patch and keep this setting for current targets and not set wherever not needed eg AR50LT. >>> So.. I did *something* and I'm no longer getting a jump to EDL. >>> >>> The *something* being knocking off hfi_core_suspend(). >>> >>> If I send a sys_idle_indicator = true, I get (reformatted for >>> better legibility): >>> >>> >>> [ 0.576543] qcom-venus 5a00000.video-codec: VenusFW : >>> <VFW_H:HostDr:unkn:--------:-> IMAGE_VARIANT_STRING=PROD >>> >>> [ 0.603818] qcom-venus 5a00000.video-codec: VenusFW : >>> <VFW_H:HostDr:unkn:--------:-> OEM_IMAGE_VERSION_STRING=CRM >>> >>> [ 0.608633] qcom-venus 5a00000.video-codec: VenusFW : >>> <VFW_H:HostDr:unkn:--------:-> BUILD_TIME: Mar 15 2021 04:24:58 >>> >>> [ 0.608644] qcom-venus 5a00000.video-codec: VenusFW : >>> <VFW_L:HostDr:unkn:--------:-> Host cmd 0x10005 >>> >>> [ 0.608655] qcom-venus 5a00000.video-codec: VenusFW : >>> <VFW_E:HostDr:unkn:--------:-> VenusHostDriver_SetSysProperty(1019): HostDriver: VenusHostDriver_SetSysProperty unsupport property! >>> >>> [ 0.608667] qcom-venus 5a00000.video-codec: VenusFW : >>> <VFW_E:HostDr:unkn:--------:-> WaitForHWidle(408): VENUS is idle, no HW is running >>> >>> [ 0.650759] qcom-venus 5a00000.video-codec: VenusFW : >>> <VFW_E:HostDr:unkn:--------:-> assert_loop(433): >>> FW Assertion - Z:/b/venus_proc/venus/drivers/src/VenusHostDriver.c:1020:5ab9a >> >> this "unsupported property" error and then the assert from FW is expected on AR50LT if driver sets HFI_PROPERTY_SYS_IDLE_INDICATOR to FW. >> >> As I mentioned in my other reply, this property doesn't need to be set by driver now, FW internally always enables it. >> >>> Which then crashes Venus for good (perhaps we're missing a >>> handler for such errors that would hard reset the hw), meaning >>> trying to access it through ffmpeg will result in it never firing >>> any IRQs, so no submitted commands ever complete. >>> >>> With this information, after uncommenting the hfi_core_suspend >>> call and changing: >>> >>> [1] >>> --- hfi_venus.c : venus_suspend_3xx() -- >>> >>> - venus_prepare_power_collapse(hdev, true); >>> + venus_prepare_power_collapse(hdev, false); >>> >>> ---------------------------------------- >>> >>> I was able to test further. Turning the ARM9 core off messes >>> with the sys_idle things. Perhaps some power sequencing is >>> wrong. The diff I just mentioned comes from the fact that >>> AR50L will never ever ever send a PC_PREP_DONE ack, or at >>> least downstream never expects it (or any other HFI6XX >>> target FWIW) to do so. >>> >>> >>> Now, I also realized the adjacent set_power_control doesn't seem to be used at >>> all on msm-4.19 techpack/video. Testing all the possible combinations, I get >>> (to make it extra clear, with all the powerdown stuff in place and only diff >>> [1] in place atop what I already had before): >>> >>> >>> [set_idle_message] [set_power_control] [result] >>> 0 0 - no crash at boot, venus doesn't work -> >>> "Too many packets buffered for output stream 0:1." >>> >>> 0 1 - no crash at boot, ffmpeg hangs near vdec session init -> >>> jump to EDL shortly after >>> >>> 1 0 - hang at boot, even before display subsys initializes -> >>> platform totally hangs >>> >>> 1 1 - same as (1, 0), probably due to sys_idle_indicator being on -> >>> platform totally hangs as well >>> >>> Perhaps (0, 0) is "good" and things can be worked up from there? >>> Can you recheck with the firmware team if this is expected? >> >> I will check regarding set_power_control(HFI_PROPERTY_SYS_CODEC_POWER_PLANE_CTRL) with FW team and get back. >> > HFI_PROPERTY_SYS_IDLE_INDICATOR is not supported beyond 8916 (which is versioned as V1 in video driver). This can be dropped. > > Since the property is not functionally active, it is upto firmware when they might decide to start error out as unsupported property. > > SYS_CODEC_POWER_PLANE_CTRL is supported for AR50/AR50L/IRIS1/2. It is a mandatory HFI to get the required power benefits. > > vcodec0 GDSC should be also configured as HW_CTRL while setting POWER_PLANE_CTRL to firmware. > Okay that's very good to know. To sum it up, the outcome you would expect is (more or less): - static bool venus_sys_idle_indicator = true; [...] - if(IS_V4(hdev->core) || IS_V6(hdev->core)) - venus_sys_idle_indicator = true; + venus_sys_idle_indicator = IS_V1(hdev->core); ? Konrad >> Thanks, >> >> Dikshita >> >>> Konrad >>>> Thanks, >>>> >>>> Dikshita >>>>
Currently upstream assumes all (well, almost all - see 7280 or CrOS specific checks) Venus implementations using the same version of the Hardware Firmware Interface can be treated the same way. This is however not the case. This series tries to introduce the groundwork to start differentiating them based on the VPU (Video Processing Unit) hardware type, fixes a couple of issues that were an effect of that generalized assumption and lays the foundation for supporting 8150 (IRIS1) and SM6115/QCM2290 (AR50 Lite), which will hopefully come soon. Tested on 8250, but pretty please test it on your boards too! Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org> --- Konrad Dybcio (18): media: venus: hfi_venus: Set venus_sys_idle_indicator to false on V6 media: venus: Introduce VPU version distinction media: venus: Add vpu_version to most SoCs media: venus: firmware: Leave a clue for homegrown porters media: venus: hfi_venus: Sanitize venus_boot_core() per-VPU-version media: venus: hfi_venus: Write to VIDC_CTRL_INIT after unmasking interrupts media: venus: core: Assign registers based on VPU version media: venus: hfi_venus: Fix version checks in venus_halt_axi() media: venus: hfi_venus: Fix version checks in venus_isr() media: venus: hfi_venus: Fix version check in venus_cpu_and_video_core_idle() media: venus: hfi_venus: Fix version check in venus_cpu_idle_and_pc_ready() media: venus: firmware: Correct IS_V6() checks media: venus: hfi_platform: Check vpu_version instead of device compatible media: venus: vdec: Fix version check in vdec_set_work_route() media: venus: Remap bufreq fields on HFI6XX media: venus: Introduce accessors for remapped hfi_buffer_reqs members media: venus: Use newly-introduced hfi_buffer_requirements accessors media: venus: hfi_venus: Restrict writing SCIACMDARG3 to Venus V1/V2 drivers/media/platform/qcom/venus/core.c | 7 ++- drivers/media/platform/qcom/venus/core.h | 15 ++++++ drivers/media/platform/qcom/venus/firmware.c | 20 +++++-- drivers/media/platform/qcom/venus/helpers.c | 7 +-- drivers/media/platform/qcom/venus/hfi_helper.h | 61 +++++++++++++++++++--- drivers/media/platform/qcom/venus/hfi_msgs.c | 2 +- .../media/platform/qcom/venus/hfi_plat_bufs_v6.c | 22 ++++---- drivers/media/platform/qcom/venus/hfi_platform.c | 2 +- drivers/media/platform/qcom/venus/hfi_venus.c | 29 +++++----- drivers/media/platform/qcom/venus/vdec.c | 10 ++-- drivers/media/platform/qcom/venus/vdec_ctrls.c | 2 +- drivers/media/platform/qcom/venus/venc.c | 4 +- drivers/media/platform/qcom/venus/venc_ctrls.c | 2 +- 13 files changed, 132 insertions(+), 51 deletions(-) --- base-commit: 058f4df42121baadbb8a980c06011e912784dbd2 change-id: 20230228-topic-venus-70ea3bc76688 Best regards,