[01/18] media: venus: hfi_venus: Set venus_sys_idle_indicator to false on V6

  This call does not seem to have been cast on any kernel with support
for VPU-1.0 or newer (and by extension, HFI6 and newer). Restrict it
to V4 only, as it seems to have been enabled by mistake and causes a
hang & reboot to EDL on at least one occasion with SM6115 / AR50L

Fixes: 7ed9e0b3393c ("media: venus: hfi, vdec: v6 Add IS_V6() to existing IS_V4() if locations")
Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
---
 drivers/media/platform/qcom/venus/hfi_venus.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
  

On 28/02/2023 15:24, Konrad Dybcio wrote:
> This call does not seem to have been cast on any kernel with support
> for VPU-1.0 or newer (and by extension, HFI6 and newer). 

We tested this on sm8250

Restrict it
> to V4 only, as it seems to have been enabled by mistake and causes a
> hang & reboot to EDL on at least one occasion with SM6115 / AR50L
> 
> Fixes: 7ed9e0b3393c ("media: venus: hfi, vdec: v6 Add IS_V6() to existing IS_V4() if locations")
> Signed-off-by: Konrad Dybcio<konrad.dybcio@linaro.org>

Right. This may indeed fix it for you on SM6115, could you test it on 
RB5 and verify the above statement ?

---
bod
  

On 28/02/2023 15:26, Bryan O'Donoghue wrote:
> On 28/02/2023 15:24, Konrad Dybcio wrote:
>> This call does not seem to have been cast on any kernel with support
>> for VPU-1.0 or newer (and by extension, HFI6 and newer). 
> 
> We tested this on sm8250
> 
> Restrict it
>> to V4 only, as it seems to have been enabled by mistake and causes a
>> hang & reboot to EDL on at least one occasion with SM6115 / AR50L
>>
>> Fixes: 7ed9e0b3393c ("media: venus: hfi, vdec: v6 Add IS_V6() to 
>> existing IS_V4() if locations")
>> Signed-off-by: Konrad Dybcio<konrad.dybcio@linaro.org>
> 
> Right. This may indeed fix it for you on SM6115, could you test it on 
> RB5 and verify the above statement ?
> 
> ---
> bod

For example.

Doesn't your later patch take account of VPU h/w version ? IRIS_1, 
IRIS_2 etc.

When we added for V6 here, we meant for current tested V6 hardware at 
that point - at least sm8250.

Can you not differentiate sm6115 based on VPU hardware identifier ? We 
want to retain this logic for 8250 and then assuming your patch is 
correct, not do this for sm6115.

---
bod
  

On 28.02.2023 16:31, Bryan O'Donoghue wrote:
> On 28/02/2023 15:26, Bryan O'Donoghue wrote:
>> On 28/02/2023 15:24, Konrad Dybcio wrote:
>>> This call does not seem to have been cast on any kernel with support
>>> for VPU-1.0 or newer (and by extension, HFI6 and newer). 
>>
>> We tested this on sm8250
>>
>> Restrict it
>>> to V4 only, as it seems to have been enabled by mistake and causes a
>>> hang & reboot to EDL on at least one occasion with SM6115 / AR50L
>>>
>>> Fixes: 7ed9e0b3393c ("media: venus: hfi, vdec: v6 Add IS_V6() to existing IS_V4() if locations")
>>> Signed-off-by: Konrad Dybcio<konrad.dybcio@linaro.org>
>>
>> Right. This may indeed fix it for you on SM6115, could you test it on RB5 and verify the above statement ?
>>
>> ---
>> bod
> 
> For example.
> 
> Doesn't your later patch take account of VPU h/w version ? IRIS_1, IRIS_2 etc.
> 
> When we added for V6 here, we meant for current tested V6 hardware at that point - at least sm8250.
> 
> Can you not differentiate sm6115 based on VPU hardware identifier ? We want to retain this logic for 8250 and then assuming your patch is correct, not do this for sm6115.
As far as my only source of information (msm-4.19 techpack) goes, this is
unnecessary/incorrect on 8250 as well. I doubt downstream would ship Venus
with no/broken low-power modes..

Konrad
> 
> ---
> bod
  

On 28/02/2023 15:37, Konrad Dybcio wrote:
> 
> 
> On 28.02.2023 16:31, Bryan O'Donoghue wrote:
>> On 28/02/2023 15:26, Bryan O'Donoghue wrote:
>>> On 28/02/2023 15:24, Konrad Dybcio wrote:
>>>> This call does not seem to have been cast on any kernel with support
>>>> for VPU-1.0 or newer (and by extension, HFI6 and newer).
>>>
>>> We tested this on sm8250
>>>
>>> Restrict it
>>>> to V4 only, as it seems to have been enabled by mistake and causes a
>>>> hang & reboot to EDL on at least one occasion with SM6115 / AR50L
>>>>
>>>> Fixes: 7ed9e0b3393c ("media: venus: hfi, vdec: v6 Add IS_V6() to existing IS_V4() if locations")
>>>> Signed-off-by: Konrad Dybcio<konrad.dybcio@linaro.org>
>>>
>>> Right. This may indeed fix it for you on SM6115, could you test it on RB5 and verify the above statement ?
>>>
>>> ---
>>> bod
>>
>> For example.
>>
>> Doesn't your later patch take account of VPU h/w version ? IRIS_1, IRIS_2 etc.
>>
>> When we added for V6 here, we meant for current tested V6 hardware at that point - at least sm8250.
>>
>> Can you not differentiate sm6115 based on VPU hardware identifier ? We want to retain this logic for 8250 and then assuming your patch is correct, not do this for sm6115.
> As far as my only source of information (msm-4.19 techpack) goes, this is
> unnecessary/incorrect on 8250 as well. I doubt downstream would ship Venus
> with no/broken low-power modes..

Can you test it and make sure ?

---
bod
  

On 28.02.2023 16:38, Bryan O'Donoghue wrote:
> On 28/02/2023 15:37, Konrad Dybcio wrote:
>>
>>
>> On 28.02.2023 16:31, Bryan O'Donoghue wrote:
>>> On 28/02/2023 15:26, Bryan O'Donoghue wrote:
>>>> On 28/02/2023 15:24, Konrad Dybcio wrote:
>>>>> This call does not seem to have been cast on any kernel with support
>>>>> for VPU-1.0 or newer (and by extension, HFI6 and newer).
>>>>
>>>> We tested this on sm8250
>>>>
>>>> Restrict it
>>>>> to V4 only, as it seems to have been enabled by mistake and causes a
>>>>> hang & reboot to EDL on at least one occasion with SM6115 / AR50L
>>>>>
>>>>> Fixes: 7ed9e0b3393c ("media: venus: hfi, vdec: v6 Add IS_V6() to existing IS_V4() if locations")
>>>>> Signed-off-by: Konrad Dybcio<konrad.dybcio@linaro.org>
>>>>
>>>> Right. This may indeed fix it for you on SM6115, could you test it on RB5 and verify the above statement ?
>>>>
>>>> ---
>>>> bod
>>>
>>> For example.
>>>
>>> Doesn't your later patch take account of VPU h/w version ? IRIS_1, IRIS_2 etc.
>>>
>>> When we added for V6 here, we meant for current tested V6 hardware at that point - at least sm8250.
>>>
>>> Can you not differentiate sm6115 based on VPU hardware identifier ? We want to retain this logic for 8250 and then assuming your patch is correct, not do this for sm6115.
>> As far as my only source of information (msm-4.19 techpack) goes, this is
>> unnecessary/incorrect on 8250 as well. I doubt downstream would ship Venus
>> with no/broken low-power modes..
> 
> 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.

Konrad
> 
> ---
> bod
>
  

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
  

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.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 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
>>>>