diff mbox series

[v2,2/3] arm64: dts: qcom: sc7280: Move video-firmware to chrome-common

Message ID 20231002-sc7280-venus-pas-v2-2-bd2408891317@fairphone.com
State Superseded
Headers show
Series [v2,1/3] media: venus: core: Set up secure memory ranges for SC7280 | expand

Commit Message

Luca Weiss Oct. 2, 2023, 2:20 p.m. UTC 
If the video-firmware node is present, the venus driver assumes we're on
a system that doesn't use TZ for starting venus, like on ChromeOS
devices.

Move the video-firmware node to chrome-common.dtsi so we can use venus
on a non-ChromeOS devices.

At the same time also disable the venus node by default in the dtsi,
like it's done on other SoCs.

Reviewed-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
Signed-off-by: Luca Weiss <luca.weiss@fairphone.com>
---
 arch/arm64/boot/dts/qcom/sc7280-chrome-common.dtsi | 8 ++++++++
 arch/arm64/boot/dts/qcom/sc7280.dtsi               | 6 ++----
 2 files changed, 10 insertions(+), 4 deletions(-)

Comments

Vikash Garodia Nov. 22, 2023, 1:17 p.m. UTC | #1 
On 10/2/2023 7:50 PM, Luca Weiss wrote:
> If the video-firmware node is present, the venus driver assumes we're on
> a system that doesn't use TZ for starting venus, like on ChromeOS
> devices.
> 
> Move the video-firmware node to chrome-common.dtsi so we can use venus
> on a non-ChromeOS devices.
> 
> At the same time also disable the venus node by default in the dtsi,
> like it's done on other SoCs.
> 
> Reviewed-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
> Signed-off-by: Luca Weiss <luca.weiss@fairphone.com>
> ---
>  arch/arm64/boot/dts/qcom/sc7280-chrome-common.dtsi | 8 ++++++++
>  arch/arm64/boot/dts/qcom/sc7280.dtsi               | 6 ++----
>  2 files changed, 10 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/arm64/boot/dts/qcom/sc7280-chrome-common.dtsi b/arch/arm64/boot/dts/qcom/sc7280-chrome-common.dtsi
> index 5d462ae14ba1..cd491e46666d 100644
> --- a/arch/arm64/boot/dts/qcom/sc7280-chrome-common.dtsi
> +++ b/arch/arm64/boot/dts/qcom/sc7280-chrome-common.dtsi
> @@ -104,6 +104,14 @@ &scm {
>  	dma-coherent;
>  };
>  
> +&venus {
> +	status = "okay";
> +
> +	video-firmware {
> +		iommus = <&apps_smmu 0x21a2 0x0>;
> +	};
> +};
> +
>  &watchdog {
>  	status = "okay";
>  };
> diff --git a/arch/arm64/boot/dts/qcom/sc7280.dtsi b/arch/arm64/boot/dts/qcom/sc7280.dtsi
> index 66f1eb83cca7..fa53f54d4675 100644
> --- a/arch/arm64/boot/dts/qcom/sc7280.dtsi
> +++ b/arch/arm64/boot/dts/qcom/sc7280.dtsi
> @@ -3740,6 +3740,8 @@ venus: video-codec@aa00000 {
>  				 <&apps_smmu 0x2184 0x20>;
>  			memory-region = <&video_mem>;
>  
> +			status = "disabled";
> +
>  			video-decoder {
>  				compatible = "venus-decoder";
>  			};
> @@ -3748,10 +3750,6 @@ video-encoder {
>  				compatible = "venus-encoder";
>  			};
>  
> -			video-firmware {
> -				iommus = <&apps_smmu 0x21a2 0x0>;
> -			};
> -
>  			venus_opp_table: opp-table {
>  				compatible = "operating-points-v2";
>  
> 
Changes look good. Is this tested on SC7280 ?

Regards,
Vikash
Luca Weiss Nov. 24, 2023, 11:35 a.m. UTC | #2 
On Fri Nov 24, 2023 at 7:38 AM CET, Vikash Garodia wrote:
>
> On 11/22/2023 7:50 PM, Luca Weiss wrote:
> > On Wed Nov 22, 2023 at 2:17 PM CET, Vikash Garodia wrote:
> >>
> >> On 10/2/2023 7:50 PM, Luca Weiss wrote:
> >>> If the video-firmware node is present, the venus driver assumes we're on
> >>> a system that doesn't use TZ for starting venus, like on ChromeOS
> >>> devices.
> >>>
> >>> Move the video-firmware node to chrome-common.dtsi so we can use venus
> >>> on a non-ChromeOS devices.
> >>>
> >>> At the same time also disable the venus node by default in the dtsi,
> >>> like it's done on other SoCs.
> >>>
> >>> Reviewed-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
> >>> Signed-off-by: Luca Weiss <luca.weiss@fairphone.com>
> >>> ---
> >>>  arch/arm64/boot/dts/qcom/sc7280-chrome-common.dtsi | 8 ++++++++
> >>>  arch/arm64/boot/dts/qcom/sc7280.dtsi               | 6 ++----
> >>>  2 files changed, 10 insertions(+), 4 deletions(-)
> >>>
> >>> diff --git a/arch/arm64/boot/dts/qcom/sc7280-chrome-common.dtsi b/arch/arm64/boot/dts/qcom/sc7280-chrome-common.dtsi
> >>> index 5d462ae14ba1..cd491e46666d 100644
> >>> --- a/arch/arm64/boot/dts/qcom/sc7280-chrome-common.dtsi
> >>> +++ b/arch/arm64/boot/dts/qcom/sc7280-chrome-common.dtsi
> >>> @@ -104,6 +104,14 @@ &scm {
> >>>  	dma-coherent;
> >>>  };
> >>>  
> >>> +&venus {
> >>> +	status = "okay";
> >>> +
> >>> +	video-firmware {
> >>> +		iommus = <&apps_smmu 0x21a2 0x0>;
> >>> +	};
> >>> +};
> >>> +
> >>>  &watchdog {
> >>>  	status = "okay";
> >>>  };
> >>> diff --git a/arch/arm64/boot/dts/qcom/sc7280.dtsi b/arch/arm64/boot/dts/qcom/sc7280.dtsi
> >>> index 66f1eb83cca7..fa53f54d4675 100644
> >>> --- a/arch/arm64/boot/dts/qcom/sc7280.dtsi
> >>> +++ b/arch/arm64/boot/dts/qcom/sc7280.dtsi
> >>> @@ -3740,6 +3740,8 @@ venus: video-codec@aa00000 {
> >>>  				 <&apps_smmu 0x2184 0x20>;
> 0x2184 is a secure SID. I think qcm6490-fairphone-fp5.dts needs to override the
> iommus property as well to retain only the non secure SID i.e 0x2180 ? I am
> seeing below crash
>
> Call trace:
> [   47.663593]  qcom_smmu_write_s2cr+0x64/0xa4
> [   47.663616]  arm_smmu_attach_dev+0x120/0x284
> [   47.663647]  __iommu_attach_device+0x24/0xf8
> [   47.676845]  __iommu_device_set_domain+0x70/0xd0
> [   47.681632]  __iommu_group_set_domain_internal+0x60/0x1b4
> [   47.687218]  iommu_setup_default_domain+0x358/0x418
> [   47.692258]  __iommu_probe_device+0x3e4/0x404
>
> Could you please reconfirm if Video SID 0x2184 (and mask) is allowed by the
> qcm6490-fairphone-fp5 hardware having TZ ?

Hi,

On FP5 it seems it's no problem to have both SIDs in there, probe and
using venus appears to work fine.

Are you using different firmware than QCM6490.LA.3.0 on the device where
you tested this?

>
> >>>  			memory-region = <&video_mem>;
> >>>  
> >>> +			status = "disabled";
> >>> +
> >>>  			video-decoder {
> >>>  				compatible = "venus-decoder";
> >>>  			};
> >>> @@ -3748,10 +3750,6 @@ video-encoder {
> >>>  				compatible = "venus-encoder";
> >>>  			};
> >>>  
> >>> -			video-firmware {
> >>> -				iommus = <&apps_smmu 0x21a2 0x0>;
> >>> -			};
> >>> -
> >>>  			venus_opp_table: opp-table {
> >>>  				compatible = "operating-points-v2";
> >>>  
> >>>
> >> Changes look good. Is this tested on SC7280 ?
> > 
> > Hi Vikash,
> > 
> > I didn't test it myself on sc7280 (just qcm6490-fp5) but dtx_diff
> > reports no differences except for status = okay property being added, so
> > there should be no change on those boards. See below.
> > 
> > Regards
> > Luca
>
> I tested on SC7280 (herobrine) and all good.

Great, thanks!

Regards
Luca

>
> Regards,
> Vikash
Vikash Garodia Nov. 24, 2023, 12:29 p.m. UTC | #3 
On 11/24/2023 5:05 PM, Luca Weiss wrote:
> On Fri Nov 24, 2023 at 7:38 AM CET, Vikash Garodia wrote:
>>
>> On 11/22/2023 7:50 PM, Luca Weiss wrote:
>>> On Wed Nov 22, 2023 at 2:17 PM CET, Vikash Garodia wrote:
>>>>
>>>> On 10/2/2023 7:50 PM, Luca Weiss wrote:
>>>>> If the video-firmware node is present, the venus driver assumes we're on
>>>>> a system that doesn't use TZ for starting venus, like on ChromeOS
>>>>> devices.
>>>>>
>>>>> Move the video-firmware node to chrome-common.dtsi so we can use venus
>>>>> on a non-ChromeOS devices.
>>>>>
>>>>> At the same time also disable the venus node by default in the dtsi,
>>>>> like it's done on other SoCs.
>>>>>
>>>>> Reviewed-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
>>>>> Signed-off-by: Luca Weiss <luca.weiss@fairphone.com>
>>>>> ---
>>>>>  arch/arm64/boot/dts/qcom/sc7280-chrome-common.dtsi | 8 ++++++++
>>>>>  arch/arm64/boot/dts/qcom/sc7280.dtsi               | 6 ++----
>>>>>  2 files changed, 10 insertions(+), 4 deletions(-)
>>>>>
>>>>> diff --git a/arch/arm64/boot/dts/qcom/sc7280-chrome-common.dtsi b/arch/arm64/boot/dts/qcom/sc7280-chrome-common.dtsi
>>>>> index 5d462ae14ba1..cd491e46666d 100644
>>>>> --- a/arch/arm64/boot/dts/qcom/sc7280-chrome-common.dtsi
>>>>> +++ b/arch/arm64/boot/dts/qcom/sc7280-chrome-common.dtsi
>>>>> @@ -104,6 +104,14 @@ &scm {
>>>>>  	dma-coherent;
>>>>>  };
>>>>>  
>>>>> +&venus {
>>>>> +	status = "okay";
>>>>> +
>>>>> +	video-firmware {
>>>>> +		iommus = <&apps_smmu 0x21a2 0x0>;
>>>>> +	};
>>>>> +};
>>>>> +
>>>>>  &watchdog {
>>>>>  	status = "okay";
>>>>>  };
>>>>> diff --git a/arch/arm64/boot/dts/qcom/sc7280.dtsi b/arch/arm64/boot/dts/qcom/sc7280.dtsi
>>>>> index 66f1eb83cca7..fa53f54d4675 100644
>>>>> --- a/arch/arm64/boot/dts/qcom/sc7280.dtsi
>>>>> +++ b/arch/arm64/boot/dts/qcom/sc7280.dtsi
>>>>> @@ -3740,6 +3740,8 @@ venus: video-codec@aa00000 {
>>>>>  				 <&apps_smmu 0x2184 0x20>;
>> 0x2184 is a secure SID. I think qcm6490-fairphone-fp5.dts needs to override the
>> iommus property as well to retain only the non secure SID i.e 0x2180 ? I am
>> seeing below crash
>>
>> Call trace:
>> [   47.663593]  qcom_smmu_write_s2cr+0x64/0xa4
>> [   47.663616]  arm_smmu_attach_dev+0x120/0x284
>> [   47.663647]  __iommu_attach_device+0x24/0xf8
>> [   47.676845]  __iommu_device_set_domain+0x70/0xd0
>> [   47.681632]  __iommu_group_set_domain_internal+0x60/0x1b4
>> [   47.687218]  iommu_setup_default_domain+0x358/0x418
>> [   47.692258]  __iommu_probe_device+0x3e4/0x404
>>
>> Could you please reconfirm if Video SID 0x2184 (and mask) is allowed by the
>> qcm6490-fairphone-fp5 hardware having TZ ?
> 
> Hi,
> 
> On FP5 it seems it's no problem to have both SIDs in there, probe and
> using venus appears to work fine.
> 
> Are you using different firmware than QCM6490.LA.3.0 on the device where
> you tested this?
I was testing this on RB3 board which uses firmware [1].

Regards,
Vikash

[1]
https://git.kernel.org/pub/scm/linux/kernel/git/firmware/linux-firmware.git/tree/qcom/vpu-2.0

>>
>>>>>  			memory-region = <&video_mem>;
>>>>>  
>>>>> +			status = "disabled";
>>>>> +
>>>>>  			video-decoder {
>>>>>  				compatible = "venus-decoder";
>>>>>  			};
>>>>> @@ -3748,10 +3750,6 @@ video-encoder {
>>>>>  				compatible = "venus-encoder";
>>>>>  			};
>>>>>  
>>>>> -			video-firmware {
>>>>> -				iommus = <&apps_smmu 0x21a2 0x0>;
>>>>> -			};
>>>>> -
>>>>>  			venus_opp_table: opp-table {
>>>>>  				compatible = "operating-points-v2";
>>>>>  
>>>>>
>>>> Changes look good. Is this tested on SC7280 ?
>>>
>>> Hi Vikash,
>>>
>>> I didn't test it myself on sc7280 (just qcm6490-fp5) but dtx_diff
>>> reports no differences except for status = okay property being added, so
>>> there should be no change on those boards. See below.
>>>
>>> Regards
>>> Luca
>>
>> I tested on SC7280 (herobrine) and all good.
> 
> Great, thanks!
> 
> Regards
> Luca
> 
>>
>> Regards,
>> Vikash
>
Dmitry Baryshkov Nov. 24, 2023, 12:53 p.m. UTC | #4 
On Fri, 24 Nov 2023 at 14:30, Vikash Garodia <quic_vgarodia@quicinc.com> wrote:
>
> On 11/24/2023 5:05 PM, Luca Weiss wrote:
> > On Fri Nov 24, 2023 at 7:38 AM CET, Vikash Garodia wrote:
> >>
> >> On 11/22/2023 7:50 PM, Luca Weiss wrote:
> >>> On Wed Nov 22, 2023 at 2:17 PM CET, Vikash Garodia wrote:
> >>>>
> >>>> On 10/2/2023 7:50 PM, Luca Weiss wrote:
> >>>>> If the video-firmware node is present, the venus driver assumes we're on
> >>>>> a system that doesn't use TZ for starting venus, like on ChromeOS
> >>>>> devices.
> >>>>>
> >>>>> Move the video-firmware node to chrome-common.dtsi so we can use venus
> >>>>> on a non-ChromeOS devices.
> >>>>>
> >>>>> At the same time also disable the venus node by default in the dtsi,
> >>>>> like it's done on other SoCs.
> >>>>>
> >>>>> Reviewed-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
> >>>>> Signed-off-by: Luca Weiss <luca.weiss@fairphone.com>
> >>>>> ---
> >>>>>  arch/arm64/boot/dts/qcom/sc7280-chrome-common.dtsi | 8 ++++++++
> >>>>>  arch/arm64/boot/dts/qcom/sc7280.dtsi               | 6 ++----
> >>>>>  2 files changed, 10 insertions(+), 4 deletions(-)
> >>>>>
> >>>>> diff --git a/arch/arm64/boot/dts/qcom/sc7280-chrome-common.dtsi b/arch/arm64/boot/dts/qcom/sc7280-chrome-common.dtsi
> >>>>> index 5d462ae14ba1..cd491e46666d 100644
> >>>>> --- a/arch/arm64/boot/dts/qcom/sc7280-chrome-common.dtsi
> >>>>> +++ b/arch/arm64/boot/dts/qcom/sc7280-chrome-common.dtsi
> >>>>> @@ -104,6 +104,14 @@ &scm {
> >>>>>   dma-coherent;
> >>>>>  };
> >>>>>
> >>>>> +&venus {
> >>>>> + status = "okay";
> >>>>> +
> >>>>> + video-firmware {
> >>>>> +         iommus = <&apps_smmu 0x21a2 0x0>;
> >>>>> + };
> >>>>> +};
> >>>>> +
> >>>>>  &watchdog {
> >>>>>   status = "okay";
> >>>>>  };
> >>>>> diff --git a/arch/arm64/boot/dts/qcom/sc7280.dtsi b/arch/arm64/boot/dts/qcom/sc7280.dtsi
> >>>>> index 66f1eb83cca7..fa53f54d4675 100644
> >>>>> --- a/arch/arm64/boot/dts/qcom/sc7280.dtsi
> >>>>> +++ b/arch/arm64/boot/dts/qcom/sc7280.dtsi
> >>>>> @@ -3740,6 +3740,8 @@ venus: video-codec@aa00000 {
> >>>>>                            <&apps_smmu 0x2184 0x20>;
> >> 0x2184 is a secure SID. I think qcm6490-fairphone-fp5.dts needs to override the
> >> iommus property as well to retain only the non secure SID i.e 0x2180 ? I am
> >> seeing below crash
> >>
> >> Call trace:
> >> [   47.663593]  qcom_smmu_write_s2cr+0x64/0xa4
> >> [   47.663616]  arm_smmu_attach_dev+0x120/0x284
> >> [   47.663647]  __iommu_attach_device+0x24/0xf8
> >> [   47.676845]  __iommu_device_set_domain+0x70/0xd0
> >> [   47.681632]  __iommu_group_set_domain_internal+0x60/0x1b4
> >> [   47.687218]  iommu_setup_default_domain+0x358/0x418
> >> [   47.692258]  __iommu_probe_device+0x3e4/0x404
> >>
> >> Could you please reconfirm if Video SID 0x2184 (and mask) is allowed by the
> >> qcm6490-fairphone-fp5 hardware having TZ ?
> >
> > Hi,
> >
> > On FP5 it seems it's no problem to have both SIDs in there, probe and
> > using venus appears to work fine.
> >
> > Are you using different firmware than QCM6490.LA.3.0 on the device where
> > you tested this?
> I was testing this on RB3 board which uses firmware [1].

There is something wrong here.

RB3 board uses venus-5.2
RB5 board uses vpu-1.0
Only sc7280 uses vpu-2.0

>
> Regards,
> Vikash
>
> [1]
> https://git.kernel.org/pub/scm/linux/kernel/git/firmware/linux-firmware.git/tree/qcom/vpu-2.0
>
> >>
> >>>>>                   memory-region = <&video_mem>;
> >>>>>
> >>>>> +                 status = "disabled";
> >>>>> +
> >>>>>                   video-decoder {
> >>>>>                           compatible = "venus-decoder";
> >>>>>                   };
> >>>>> @@ -3748,10 +3750,6 @@ video-encoder {
> >>>>>                           compatible = "venus-encoder";
> >>>>>                   };
> >>>>>
> >>>>> -                 video-firmware {
> >>>>> -                         iommus = <&apps_smmu 0x21a2 0x0>;
> >>>>> -                 };
> >>>>> -
> >>>>>                   venus_opp_table: opp-table {
> >>>>>                           compatible = "operating-points-v2";
> >>>>>
> >>>>>
> >>>> Changes look good. Is this tested on SC7280 ?
> >>>
> >>> Hi Vikash,
> >>>
> >>> I didn't test it myself on sc7280 (just qcm6490-fp5) but dtx_diff
> >>> reports no differences except for status = okay property being added, so
> >>> there should be no change on those boards. See below.
> >>>
> >>> Regards
> >>> Luca
> >>
> >> I tested on SC7280 (herobrine) and all good.
> >
> > Great, thanks!
> >
> > Regards
> > Luca
> >
> >>
> >> Regards,
> >> Vikash
> >
>
Vikash Garodia Nov. 24, 2023, 1:35 p.m. UTC | #5 
On 11/24/2023 6:23 PM, Dmitry Baryshkov wrote:
> On Fri, 24 Nov 2023 at 14:30, Vikash Garodia <quic_vgarodia@quicinc.com> wrote:
>>
>> On 11/24/2023 5:05 PM, Luca Weiss wrote:
>>> On Fri Nov 24, 2023 at 7:38 AM CET, Vikash Garodia wrote:
>>>>
>>>> On 11/22/2023 7:50 PM, Luca Weiss wrote:
>>>>> On Wed Nov 22, 2023 at 2:17 PM CET, Vikash Garodia wrote:
>>>>>>
>>>>>> On 10/2/2023 7:50 PM, Luca Weiss wrote:
>>>>>>> If the video-firmware node is present, the venus driver assumes we're on
>>>>>>> a system that doesn't use TZ for starting venus, like on ChromeOS
>>>>>>> devices.
>>>>>>>
>>>>>>> Move the video-firmware node to chrome-common.dtsi so we can use venus
>>>>>>> on a non-ChromeOS devices.
>>>>>>>
>>>>>>> At the same time also disable the venus node by default in the dtsi,
>>>>>>> like it's done on other SoCs.
>>>>>>>
>>>>>>> Reviewed-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
>>>>>>> Signed-off-by: Luca Weiss <luca.weiss@fairphone.com>
>>>>>>> ---
>>>>>>>  arch/arm64/boot/dts/qcom/sc7280-chrome-common.dtsi | 8 ++++++++
>>>>>>>  arch/arm64/boot/dts/qcom/sc7280.dtsi               | 6 ++----
>>>>>>>  2 files changed, 10 insertions(+), 4 deletions(-)
>>>>>>>
>>>>>>> diff --git a/arch/arm64/boot/dts/qcom/sc7280-chrome-common.dtsi b/arch/arm64/boot/dts/qcom/sc7280-chrome-common.dtsi
>>>>>>> index 5d462ae14ba1..cd491e46666d 100644
>>>>>>> --- a/arch/arm64/boot/dts/qcom/sc7280-chrome-common.dtsi
>>>>>>> +++ b/arch/arm64/boot/dts/qcom/sc7280-chrome-common.dtsi
>>>>>>> @@ -104,6 +104,14 @@ &scm {
>>>>>>>   dma-coherent;
>>>>>>>  };
>>>>>>>
>>>>>>> +&venus {
>>>>>>> + status = "okay";
>>>>>>> +
>>>>>>> + video-firmware {
>>>>>>> +         iommus = <&apps_smmu 0x21a2 0x0>;
>>>>>>> + };
>>>>>>> +};
>>>>>>> +
>>>>>>>  &watchdog {
>>>>>>>   status = "okay";
>>>>>>>  };
>>>>>>> diff --git a/arch/arm64/boot/dts/qcom/sc7280.dtsi b/arch/arm64/boot/dts/qcom/sc7280.dtsi
>>>>>>> index 66f1eb83cca7..fa53f54d4675 100644
>>>>>>> --- a/arch/arm64/boot/dts/qcom/sc7280.dtsi
>>>>>>> +++ b/arch/arm64/boot/dts/qcom/sc7280.dtsi
>>>>>>> @@ -3740,6 +3740,8 @@ venus: video-codec@aa00000 {
>>>>>>>                            <&apps_smmu 0x2184 0x20>;
>>>> 0x2184 is a secure SID. I think qcm6490-fairphone-fp5.dts needs to override the
>>>> iommus property as well to retain only the non secure SID i.e 0x2180 ? I am
>>>> seeing below crash
>>>>
>>>> Call trace:
>>>> [   47.663593]  qcom_smmu_write_s2cr+0x64/0xa4
>>>> [   47.663616]  arm_smmu_attach_dev+0x120/0x284
>>>> [   47.663647]  __iommu_attach_device+0x24/0xf8
>>>> [   47.676845]  __iommu_device_set_domain+0x70/0xd0
>>>> [   47.681632]  __iommu_group_set_domain_internal+0x60/0x1b4
>>>> [   47.687218]  iommu_setup_default_domain+0x358/0x418
>>>> [   47.692258]  __iommu_probe_device+0x3e4/0x404
>>>>
>>>> Could you please reconfirm if Video SID 0x2184 (and mask) is allowed by the
>>>> qcm6490-fairphone-fp5 hardware having TZ ?
>>>
>>> Hi,
>>>
>>> On FP5 it seems it's no problem to have both SIDs in there, probe and
>>> using venus appears to work fine.
>>>
>>> Are you using different firmware than QCM6490.LA.3.0 on the device where
>>> you tested this?
>> I was testing this on RB3 board which uses firmware [1].
> 
> There is something wrong here.
> 
> RB3 board uses venus-5.2
> RB5 board uses vpu-1.0
> Only sc7280 uses vpu-2.0

Tested on QCM6490 IDP board, which is QCOM internal board similar to RB3 gen2.

>>
>> Regards,
>> Vikash
>>
>> [1]
>> https://git.kernel.org/pub/scm/linux/kernel/git/firmware/linux-firmware.git/tree/qcom/vpu-2.0
>>
>>>>
>>>>>>>                   memory-region = <&video_mem>;
>>>>>>>
>>>>>>> +                 status = "disabled";
>>>>>>> +
>>>>>>>                   video-decoder {
>>>>>>>                           compatible = "venus-decoder";
>>>>>>>                   };
>>>>>>> @@ -3748,10 +3750,6 @@ video-encoder {
>>>>>>>                           compatible = "venus-encoder";
>>>>>>>                   };
>>>>>>>
>>>>>>> -                 video-firmware {
>>>>>>> -                         iommus = <&apps_smmu 0x21a2 0x0>;
>>>>>>> -                 };
>>>>>>> -
>>>>>>>                   venus_opp_table: opp-table {
>>>>>>>                           compatible = "operating-points-v2";
>>>>>>>
>>>>>>>
>>>>>> Changes look good. Is this tested on SC7280 ?
>>>>>
>>>>> Hi Vikash,
>>>>>
>>>>> I didn't test it myself on sc7280 (just qcm6490-fp5) but dtx_diff
>>>>> reports no differences except for status = okay property being added, so
>>>>> there should be no change on those boards. See below.
>>>>>
>>>>> Regards
>>>>> Luca
>>>>
>>>> I tested on SC7280 (herobrine) and all good.
>>>
>>> Great, thanks!
>>>
>>> Regards
>>> Luca
>>>
>>>>
>>>> Regards,
>>>> Vikash
>>>
>>
> 
>
Luca Weiss Nov. 24, 2023, 3:56 p.m. UTC | #6 
On Fri Nov 24, 2023 at 2:35 PM CET, Vikash Garodia wrote:
>
>
> On 11/24/2023 6:23 PM, Dmitry Baryshkov wrote:
> > On Fri, 24 Nov 2023 at 14:30, Vikash Garodia <quic_vgarodia@quicinc.com> wrote:
> >>
> >> On 11/24/2023 5:05 PM, Luca Weiss wrote:
> >>> On Fri Nov 24, 2023 at 7:38 AM CET, Vikash Garodia wrote:
> >>>>
> >>>> On 11/22/2023 7:50 PM, Luca Weiss wrote:
> >>>>> On Wed Nov 22, 2023 at 2:17 PM CET, Vikash Garodia wrote:
> >>>>>>
> >>>>>> On 10/2/2023 7:50 PM, Luca Weiss wrote:
> >>>>>>> If the video-firmware node is present, the venus driver assumes we're on
> >>>>>>> a system that doesn't use TZ for starting venus, like on ChromeOS
> >>>>>>> devices.
> >>>>>>>
> >>>>>>> Move the video-firmware node to chrome-common.dtsi so we can use venus
> >>>>>>> on a non-ChromeOS devices.
> >>>>>>>
> >>>>>>> At the same time also disable the venus node by default in the dtsi,
> >>>>>>> like it's done on other SoCs.
> >>>>>>>
> >>>>>>> Reviewed-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
> >>>>>>> Signed-off-by: Luca Weiss <luca.weiss@fairphone.com>
> >>>>>>> ---
> >>>>>>>  arch/arm64/boot/dts/qcom/sc7280-chrome-common.dtsi | 8 ++++++++
> >>>>>>>  arch/arm64/boot/dts/qcom/sc7280.dtsi               | 6 ++----
> >>>>>>>  2 files changed, 10 insertions(+), 4 deletions(-)
> >>>>>>>
> >>>>>>> diff --git a/arch/arm64/boot/dts/qcom/sc7280-chrome-common.dtsi b/arch/arm64/boot/dts/qcom/sc7280-chrome-common.dtsi
> >>>>>>> index 5d462ae14ba1..cd491e46666d 100644
> >>>>>>> --- a/arch/arm64/boot/dts/qcom/sc7280-chrome-common.dtsi
> >>>>>>> +++ b/arch/arm64/boot/dts/qcom/sc7280-chrome-common.dtsi
> >>>>>>> @@ -104,6 +104,14 @@ &scm {
> >>>>>>>   dma-coherent;
> >>>>>>>  };
> >>>>>>>
> >>>>>>> +&venus {
> >>>>>>> + status = "okay";
> >>>>>>> +
> >>>>>>> + video-firmware {
> >>>>>>> +         iommus = <&apps_smmu 0x21a2 0x0>;
> >>>>>>> + };
> >>>>>>> +};
> >>>>>>> +
> >>>>>>>  &watchdog {
> >>>>>>>   status = "okay";
> >>>>>>>  };
> >>>>>>> diff --git a/arch/arm64/boot/dts/qcom/sc7280.dtsi b/arch/arm64/boot/dts/qcom/sc7280.dtsi
> >>>>>>> index 66f1eb83cca7..fa53f54d4675 100644
> >>>>>>> --- a/arch/arm64/boot/dts/qcom/sc7280.dtsi
> >>>>>>> +++ b/arch/arm64/boot/dts/qcom/sc7280.dtsi
> >>>>>>> @@ -3740,6 +3740,8 @@ venus: video-codec@aa00000 {
> >>>>>>>                            <&apps_smmu 0x2184 0x20>;
> >>>> 0x2184 is a secure SID. I think qcm6490-fairphone-fp5.dts needs to override the
> >>>> iommus property as well to retain only the non secure SID i.e 0x2180 ? I am
> >>>> seeing below crash
> >>>>
> >>>> Call trace:
> >>>> [   47.663593]  qcom_smmu_write_s2cr+0x64/0xa4
> >>>> [   47.663616]  arm_smmu_attach_dev+0x120/0x284
> >>>> [   47.663647]  __iommu_attach_device+0x24/0xf8
> >>>> [   47.676845]  __iommu_device_set_domain+0x70/0xd0
> >>>> [   47.681632]  __iommu_group_set_domain_internal+0x60/0x1b4
> >>>> [   47.687218]  iommu_setup_default_domain+0x358/0x418
> >>>> [   47.692258]  __iommu_probe_device+0x3e4/0x404
> >>>>
> >>>> Could you please reconfirm if Video SID 0x2184 (and mask) is allowed by the
> >>>> qcm6490-fairphone-fp5 hardware having TZ ?
> >>>
> >>> Hi,
> >>>
> >>> On FP5 it seems it's no problem to have both SIDs in there, probe and
> >>> using venus appears to work fine.
> >>>
> >>> Are you using different firmware than QCM6490.LA.3.0 on the device where
> >>> you tested this?
> >> I was testing this on RB3 board which uses firmware [1].
> > 
> > There is something wrong here.
> > 
> > RB3 board uses venus-5.2
> > RB5 board uses vpu-1.0
> > Only sc7280 uses vpu-2.0
>
> Tested on QCM6490 IDP board, which is QCOM internal board similar to RB3 gen2.

In any case, I don't know much about the venus & iommu setup here. I can
try removing the 0x2184 SID and test if venus still works on FP5.
Also should the chromebooks keep that iommu entry or not?

Regards
Luca

>
> >>
> >> Regards,
> >> Vikash
> >>
> >> [1]
> >> https://git.kernel.org/pub/scm/linux/kernel/git/firmware/linux-firmware.git/tree/qcom/vpu-2.0
> >>
> >>>>
> >>>>>>>                   memory-region = <&video_mem>;
> >>>>>>>
> >>>>>>> +                 status = "disabled";
> >>>>>>> +
> >>>>>>>                   video-decoder {
> >>>>>>>                           compatible = "venus-decoder";
> >>>>>>>                   };
> >>>>>>> @@ -3748,10 +3750,6 @@ video-encoder {
> >>>>>>>                           compatible = "venus-encoder";
> >>>>>>>                   };
> >>>>>>>
> >>>>>>> -                 video-firmware {
> >>>>>>> -                         iommus = <&apps_smmu 0x21a2 0x0>;
> >>>>>>> -                 };
> >>>>>>> -
> >>>>>>>                   venus_opp_table: opp-table {
> >>>>>>>                           compatible = "operating-points-v2";
> >>>>>>>
> >>>>>>>
> >>>>>> Changes look good. Is this tested on SC7280 ?
> >>>>>
> >>>>> Hi Vikash,
> >>>>>
> >>>>> I didn't test it myself on sc7280 (just qcm6490-fp5) but dtx_diff
> >>>>> reports no differences except for status = okay property being added, so
> >>>>> there should be no change on those boards. See below.
> >>>>>
> >>>>> Regards
> >>>>> Luca
> >>>>
> >>>> I tested on SC7280 (herobrine) and all good.
> >>>
> >>> Great, thanks!
> >>>
> >>> Regards
> >>> Luca
> >>>
> >>>>
> >>>> Regards,
> >>>> Vikash
> >>>
> >>
> > 
> >
diff mbox series

Patch

diff --git a/arch/arm64/boot/dts/qcom/sc7280-chrome-common.dtsi b/arch/arm64/boot/dts/qcom/sc7280-chrome-common.dtsi
index 5d462ae14ba1..cd491e46666d 100644
--- a/arch/arm64/boot/dts/qcom/sc7280-chrome-common.dtsi
+++ b/arch/arm64/boot/dts/qcom/sc7280-chrome-common.dtsi
@@ -104,6 +104,14 @@  &scm {
 	dma-coherent;
 };
 
+&venus {
+	status = "okay";
+
+	video-firmware {
+		iommus = <&apps_smmu 0x21a2 0x0>;
+	};
+};
+
 &watchdog {
 	status = "okay";
 };
diff --git a/arch/arm64/boot/dts/qcom/sc7280.dtsi b/arch/arm64/boot/dts/qcom/sc7280.dtsi
index 66f1eb83cca7..fa53f54d4675 100644
--- a/arch/arm64/boot/dts/qcom/sc7280.dtsi
+++ b/arch/arm64/boot/dts/qcom/sc7280.dtsi
@@ -3740,6 +3740,8 @@  venus: video-codec@aa00000 {
 				 <&apps_smmu 0x2184 0x20>;
 			memory-region = <&video_mem>;
 
+			status = "disabled";
+
 			video-decoder {
 				compatible = "venus-decoder";
 			};
@@ -3748,10 +3750,6 @@  video-encoder {
 				compatible = "venus-encoder";
 			};
 
-			video-firmware {
-				iommus = <&apps_smmu 0x21a2 0x0>;
-			};
-
 			venus_opp_table: opp-table {
 				compatible = "operating-points-v2";