iommu/arm-smmu-qcom: Work around SDM845 Adreno SMMU w/ 16K pages

Commit Message

Konrad Dybcio July 29, 2024, 8:37 a.m. UTC

From: Konrad Dybcio <[e-mail]>

SDM845's Adreno SMMU is unique in that it actually advertizes support
for 16K (and 32M) pages, which doesn't hold for newer SoCs.

This however, seems either broken in the hardware implementation, the
hypervisor middleware that abstracts the SMMU, or there's a bug in the
Linux kernel somewhere down the line that nobody managed to track down.

Booting SDM845 with 16K page sizes and drm/msm results in:

*** gpu fault: ttbr0=0000000000000000 iova=000100000000c000 dir=READ
type=TRANSLATION source=CP (0,0,0,0)

right after loading the firmware. The GPU then starts spitting out
illegal intstruction errors, as it's quite obvious that it got a
bogus pointer.

Hide 16K support on SDM845's Adreno SMMU to work around this.

Reported-by: Sumit Semwal <[e-mail]>
Signed-off-by: Konrad Dybcio <[e-mail]>
---
There's a mismatch in sender/committer addresses but that's "fine":
https://lore.kernel.org/linux-usb/2024072734-scenic-unwilling-71ea@gregkh/
---
 drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c | 10 ++++++++++
 1 file changed, 10 insertions(+)


---
base-commit: 931a3b3bccc96e7708c82b30b2b5fa82dfd04890
change-id: 20240726-topic-845_gpu_smmu-ab738f7a013c

Best regards,

Comments

Dmitry Baryshkov July 29, 2024, 8:14 p.m. UTC | #1

On Mon, Jul 29, 2024 at 10:37:48AM GMT, Konrad Dybcio wrote:
> From: Konrad Dybcio <[e-mail]>
> 
> SDM845's Adreno SMMU is unique in that it actually advertizes support
> for 16K (and 32M) pages, which doesn't hold for newer SoCs.
> 
> This however, seems either broken in the hardware implementation, the
> hypervisor middleware that abstracts the SMMU, or there's a bug in the
> Linux kernel somewhere down the line that nobody managed to track down.
> 
> Booting SDM845 with 16K page sizes and drm/msm results in:
> 
> *** gpu fault: ttbr0=0000000000000000 iova=000100000000c000 dir=READ
> type=TRANSLATION source=CP (0,0,0,0)
> 
> right after loading the firmware. The GPU then starts spitting out
> illegal intstruction errors, as it's quite obvious that it got a
> bogus pointer.
> 
> Hide 16K support on SDM845's Adreno SMMU to work around this.
> 
> Reported-by: Sumit Semwal <[e-mail]>
> Signed-off-by: Konrad Dybcio <[e-mail]>
> ---
> There's a mismatch in sender/committer addresses but that's "fine":
> https://lore.kernel.org/linux-usb/2024072734-scenic-unwilling-71ea@gregkh/
> ---
>  drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
> index 36c6b36ad4ff..d25825c05817 100644
> --- a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
> +++ b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
> @@ -338,6 +338,15 @@ static int qcom_smmu_cfg_probe(struct arm_smmu_device *smmu)
>  	return 0;
>  }
>  
> +static int qcom_adreno_smmuv2_cfg_probe(struct arm_smmu_device *smmu)
> +{
> +	/* SDM845 Adreno SMMU advertizes 16K pages support, but something is broken */
> +	if (of_device_is_compatible(smmu->dev->of_node, "qcom,sdm845-smmu-v2"))
> +		smmu->features &= ~ARM_SMMU_FEAT_FMT_AARCH64_16K;

Shouldn't we hide that uncoditionally as it's likely that none of v2
Adreno SMMUs support 16k pages?

> +
> +	return 0;
> +}
> +
>  static void qcom_smmu_write_s2cr(struct arm_smmu_device *smmu, int idx)
>  {
>  	struct arm_smmu_s2cr *s2cr = smmu->s2crs + idx;
> @@ -436,6 +445,7 @@ static const struct arm_smmu_impl sdm845_smmu_500_impl = {
>  
>  static const struct arm_smmu_impl qcom_adreno_smmu_v2_impl = {
>  	.init_context = qcom_adreno_smmu_init_context,
> +	.cfg_probe = qcom_adreno_smmuv2_cfg_probe,
>  	.def_domain_type = qcom_smmu_def_domain_type,
>  	.alloc_context_bank = qcom_adreno_smmu_alloc_context_bank,
>  	.write_sctlr = qcom_adreno_smmu_write_sctlr,
> 
> ---
> base-commit: 931a3b3bccc96e7708c82b30b2b5fa82dfd04890
> change-id: 20240726-topic-845_gpu_smmu-ab738f7a013c
> 
> Best regards,
> -- 
> Konrad Dybcio <[e-mail]>
>

Rob Clark July 29, 2024, 9:07 p.m. UTC | #2

On Mon, Jul 29, 2024 at 1:14 PM Dmitry Baryshkov
<[e-mail]> wrote:
>
> On Mon, Jul 29, 2024 at 10:37:48AM GMT, Konrad Dybcio wrote:
> > From: Konrad Dybcio <[e-mail]>
> >
> > SDM845's Adreno SMMU is unique in that it actually advertizes support
> > for 16K (and 32M) pages, which doesn't hold for newer SoCs.
> >
> > This however, seems either broken in the hardware implementation, the
> > hypervisor middleware that abstracts the SMMU, or there's a bug in the
> > Linux kernel somewhere down the line that nobody managed to track down.
> >
> > Booting SDM845 with 16K page sizes and drm/msm results in:
> >
> > *** gpu fault: ttbr0=0000000000000000 iova=000100000000c000 dir=READ
> > type=TRANSLATION source=CP (0,0,0,0)
> >
> > right after loading the firmware. The GPU then starts spitting out
> > illegal intstruction errors, as it's quite obvious that it got a
> > bogus pointer.
> >
> > Hide 16K support on SDM845's Adreno SMMU to work around this.
> >
> > Reported-by: Sumit Semwal <[e-mail]>
> > Signed-off-by: Konrad Dybcio <[e-mail]>
> > ---
> > There's a mismatch in sender/committer addresses but that's "fine":
> > https://lore.kernel.org/linux-usb/2024072734-scenic-unwilling-71ea@gregkh/
> > ---
> >  drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c | 10 ++++++++++
> >  1 file changed, 10 insertions(+)
> >
> > diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
> > index 36c6b36ad4ff..d25825c05817 100644
> > --- a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
> > +++ b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
> > @@ -338,6 +338,15 @@ static int qcom_smmu_cfg_probe(struct arm_smmu_device *smmu)
> >       return 0;
> >  }
> >
> > +static int qcom_adreno_smmuv2_cfg_probe(struct arm_smmu_device *smmu)
> > +{
> > +     /* SDM845 Adreno SMMU advertizes 16K pages support, but something is broken */
> > +     if (of_device_is_compatible(smmu->dev->of_node, "qcom,sdm845-smmu-v2"))
> > +             smmu->features &= ~ARM_SMMU_FEAT_FMT_AARCH64_16K;
>
> Shouldn't we hide that uncoditionally as it's likely that none of v2
> Adreno SMMUs support 16k pages?

Hmm, that would be unfortunate to have the GPU not supporting the CPU
page size.  I guess we could still map 16k pages as multiple 4k pages,
but that is a bit sad..

BR,
-R

>
> > +
> > +     return 0;
> > +}
> > +
> >  static void qcom_smmu_write_s2cr(struct arm_smmu_device *smmu, int idx)
> >  {
> >       struct arm_smmu_s2cr *s2cr = smmu->s2crs + idx;
> > @@ -436,6 +445,7 @@ static const struct arm_smmu_impl sdm845_smmu_500_impl = {
> >
> >  static const struct arm_smmu_impl qcom_adreno_smmu_v2_impl = {
> >       .init_context = qcom_adreno_smmu_init_context,
> > +     .cfg_probe = qcom_adreno_smmuv2_cfg_probe,
> >       .def_domain_type = qcom_smmu_def_domain_type,
> >       .alloc_context_bank = qcom_adreno_smmu_alloc_context_bank,
> >       .write_sctlr = qcom_adreno_smmu_write_sctlr,
> >
> > ---
> > base-commit: 931a3b3bccc96e7708c82b30b2b5fa82dfd04890
> > change-id: 20240726-topic-845_gpu_smmu-ab738f7a013c
> >
> > Best regards,
> > --
> > Konrad Dybcio <[e-mail]>
> >
>
> --
> With best wishes
> Dmitry

Dmitry Baryshkov July 29, 2024, 9:21 p.m. UTC | #3

On Tue, 30 Jul 2024 at 00:08, Rob Clark <[e-mail]> wrote:
>
> On Mon, Jul 29, 2024 at 1:14 PM Dmitry Baryshkov
> <[e-mail]> wrote:
> >
> > On Mon, Jul 29, 2024 at 10:37:48AM GMT, Konrad Dybcio wrote:
> > > From: Konrad Dybcio <[e-mail]>
> > >
> > > SDM845's Adreno SMMU is unique in that it actually advertizes support
> > > for 16K (and 32M) pages, which doesn't hold for newer SoCs.
> > >
> > > This however, seems either broken in the hardware implementation, the
> > > hypervisor middleware that abstracts the SMMU, or there's a bug in the
> > > Linux kernel somewhere down the line that nobody managed to track down.
> > >
> > > Booting SDM845 with 16K page sizes and drm/msm results in:
> > >
> > > *** gpu fault: ttbr0=0000000000000000 iova=000100000000c000 dir=READ
> > > type=TRANSLATION source=CP (0,0,0,0)
> > >
> > > right after loading the firmware. The GPU then starts spitting out
> > > illegal intstruction errors, as it's quite obvious that it got a
> > > bogus pointer.
> > >
> > > Hide 16K support on SDM845's Adreno SMMU to work around this.
> > >
> > > Reported-by: Sumit Semwal <[e-mail]>
> > > Signed-off-by: Konrad Dybcio <[e-mail]>
> > > ---
> > > There's a mismatch in sender/committer addresses but that's "fine":
> > > https://lore.kernel.org/linux-usb/2024072734-scenic-unwilling-71ea@gregkh/
> > > ---
> > >  drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c | 10 ++++++++++
> > >  1 file changed, 10 insertions(+)
> > >
> > > diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
> > > index 36c6b36ad4ff..d25825c05817 100644
> > > --- a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
> > > +++ b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
> > > @@ -338,6 +338,15 @@ static int qcom_smmu_cfg_probe(struct arm_smmu_device *smmu)
> > >       return 0;
> > >  }
> > >
> > > +static int qcom_adreno_smmuv2_cfg_probe(struct arm_smmu_device *smmu)
> > > +{
> > > +     /* SDM845 Adreno SMMU advertizes 16K pages support, but something is broken */
> > > +     if (of_device_is_compatible(smmu->dev->of_node, "qcom,sdm845-smmu-v2"))
> > > +             smmu->features &= ~ARM_SMMU_FEAT_FMT_AARCH64_16K;
> >
> > Shouldn't we hide that uncoditionally as it's likely that none of v2
> > Adreno SMMUs support 16k pages?
>
> Hmm, that would be unfortunate to have the GPU not supporting the CPU
> page size.  I guess we could still map 16k pages as multiple 4k pages,
> but that is a bit sad..

For now this might be limited to older platforms (v2 vs -500)

>
> BR,
> -R
>
> >
> > > +
> > > +     return 0;
> > > +}
> > > +
> > >  static void qcom_smmu_write_s2cr(struct arm_smmu_device *smmu, int idx)
> > >  {
> > >       struct arm_smmu_s2cr *s2cr = smmu->s2crs + idx;
> > > @@ -436,6 +445,7 @@ static const struct arm_smmu_impl sdm845_smmu_500_impl = {
> > >
> > >  static const struct arm_smmu_impl qcom_adreno_smmu_v2_impl = {
> > >       .init_context = qcom_adreno_smmu_init_context,
> > > +     .cfg_probe = qcom_adreno_smmuv2_cfg_probe,
> > >       .def_domain_type = qcom_smmu_def_domain_type,
> > >       .alloc_context_bank = qcom_adreno_smmu_alloc_context_bank,
> > >       .write_sctlr = qcom_adreno_smmu_write_sctlr,
> > >
> > > ---
> > > base-commit: 931a3b3bccc96e7708c82b30b2b5fa82dfd04890
> > > change-id: 20240726-topic-845_gpu_smmu-ab738f7a013c
> > >
> > > Best regards,
> > > --
> > > Konrad Dybcio <[e-mail]>
> > >
> >
> > --
> > With best wishes
> > Dmitry

Konrad Dybcio July 30, 2024, 8:08 a.m. UTC | #4

On 29.07.2024 11:21 PM, Dmitry Baryshkov wrote:
> On Tue, 30 Jul 2024 at 00:08, Rob Clark <[e-mail]> wrote:
>>
>> On Mon, Jul 29, 2024 at 1:14 PM Dmitry Baryshkov
>> <[e-mail]> wrote:
>>>
>>> On Mon, Jul 29, 2024 at 10:37:48AM GMT, Konrad Dybcio wrote:
>>>> From: Konrad Dybcio <[e-mail]>
>>>>
>>>> SDM845's Adreno SMMU is unique in that it actually advertizes support
>>>> for 16K (and 32M) pages, which doesn't hold for newer SoCs.
>>>>
>>>> This however, seems either broken in the hardware implementation, the
>>>> hypervisor middleware that abstracts the SMMU, or there's a bug in the
>>>> Linux kernel somewhere down the line that nobody managed to track down.
>>>>
>>>> Booting SDM845 with 16K page sizes and drm/msm results in:
>>>>
>>>> *** gpu fault: ttbr0=0000000000000000 iova=000100000000c000 dir=READ
>>>> type=TRANSLATION source=CP (0,0,0,0)
>>>>
>>>> right after loading the firmware. The GPU then starts spitting out
>>>> illegal intstruction errors, as it's quite obvious that it got a
>>>> bogus pointer.
>>>>
>>>> Hide 16K support on SDM845's Adreno SMMU to work around this.
>>>>
>>>> Reported-by: Sumit Semwal <[e-mail]>
>>>> Signed-off-by: Konrad Dybcio <[e-mail]>
>>>> ---
>>>> There's a mismatch in sender/committer addresses but that's "fine":
>>>> https://lore.kernel.org/linux-usb/2024072734-scenic-unwilling-71ea@gregkh/
>>>> ---
>>>>  drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c | 10 ++++++++++
>>>>  1 file changed, 10 insertions(+)
>>>>
>>>> diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
>>>> index 36c6b36ad4ff..d25825c05817 100644
>>>> --- a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
>>>> +++ b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
>>>> @@ -338,6 +338,15 @@ static int qcom_smmu_cfg_probe(struct arm_smmu_device *smmu)
>>>>       return 0;
>>>>  }
>>>>
>>>> +static int qcom_adreno_smmuv2_cfg_probe(struct arm_smmu_device *smmu)
>>>> +{
>>>> +     /* SDM845 Adreno SMMU advertizes 16K pages support, but something is broken */
>>>> +     if (of_device_is_compatible(smmu->dev->of_node, "qcom,sdm845-smmu-v2"))
>>>> +             smmu->features &= ~ARM_SMMU_FEAT_FMT_AARCH64_16K;
>>>
>>> Shouldn't we hide that uncoditionally as it's likely that none of v2
>>> Adreno SMMUs support 16k pages?
>>
>> Hmm, that would be unfortunate to have the GPU not supporting the CPU
>> page size.  I guess we could still map 16k pages as multiple 4k pages,
>> but that is a bit sad..
> 
> For now this might be limited to older platforms (v2 vs -500)

In the commit message:

>>>> SDM845's Adreno SMMU is unique in that it actually advertizes support
>>>> for 16K (and 32M) pages, which doesn't hold for newer SoCs.

Konrad

Dmitry Baryshkov July 30, 2024, 8:50 a.m. UTC | #5

On Tue, 30 Jul 2024 at 11:08, Konrad Dybcio <[e-mail]> wrote:
>
> On 29.07.2024 11:21 PM, Dmitry Baryshkov wrote:
> > On Tue, 30 Jul 2024 at 00:08, Rob Clark <[e-mail]> wrote:
> >>
> >> On Mon, Jul 29, 2024 at 1:14 PM Dmitry Baryshkov
> >> <[e-mail]> wrote:
> >>>
> >>> On Mon, Jul 29, 2024 at 10:37:48AM GMT, Konrad Dybcio wrote:
> >>>> From: Konrad Dybcio <[e-mail]>
> >>>>
> >>>> SDM845's Adreno SMMU is unique in that it actually advertizes support
> >>>> for 16K (and 32M) pages, which doesn't hold for newer SoCs.
> >>>>
> >>>> This however, seems either broken in the hardware implementation, the
> >>>> hypervisor middleware that abstracts the SMMU, or there's a bug in the
> >>>> Linux kernel somewhere down the line that nobody managed to track down.
> >>>>
> >>>> Booting SDM845 with 16K page sizes and drm/msm results in:
> >>>>
> >>>> *** gpu fault: ttbr0=0000000000000000 iova=000100000000c000 dir=READ
> >>>> type=TRANSLATION source=CP (0,0,0,0)
> >>>>
> >>>> right after loading the firmware. The GPU then starts spitting out
> >>>> illegal intstruction errors, as it's quite obvious that it got a
> >>>> bogus pointer.
> >>>>
> >>>> Hide 16K support on SDM845's Adreno SMMU to work around this.
> >>>>
> >>>> Reported-by: Sumit Semwal <[e-mail]>
> >>>> Signed-off-by: Konrad Dybcio <[e-mail]>
> >>>> ---
> >>>> There's a mismatch in sender/committer addresses but that's "fine":
> >>>> https://lore.kernel.org/linux-usb/2024072734-scenic-unwilling-71ea@gregkh/
> >>>> ---
> >>>>  drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c | 10 ++++++++++
> >>>>  1 file changed, 10 insertions(+)
> >>>>
> >>>> diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
> >>>> index 36c6b36ad4ff..d25825c05817 100644
> >>>> --- a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
> >>>> +++ b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
> >>>> @@ -338,6 +338,15 @@ static int qcom_smmu_cfg_probe(struct arm_smmu_device *smmu)
> >>>>       return 0;
> >>>>  }
> >>>>
> >>>> +static int qcom_adreno_smmuv2_cfg_probe(struct arm_smmu_device *smmu)
> >>>> +{
> >>>> +     /* SDM845 Adreno SMMU advertizes 16K pages support, but something is broken */
> >>>> +     if (of_device_is_compatible(smmu->dev->of_node, "qcom,sdm845-smmu-v2"))
> >>>> +             smmu->features &= ~ARM_SMMU_FEAT_FMT_AARCH64_16K;
> >>>
> >>> Shouldn't we hide that uncoditionally as it's likely that none of v2
> >>> Adreno SMMUs support 16k pages?
> >>
> >> Hmm, that would be unfortunate to have the GPU not supporting the CPU
> >> page size.  I guess we could still map 16k pages as multiple 4k pages,
> >> but that is a bit sad..
> >
> > For now this might be limited to older platforms (v2 vs -500)
>
> In the commit message:
>
> >>>> SDM845's Adreno SMMU is unique in that it actually advertizes support
> >>>> for 16K (and 32M) pages, which doesn't hold for newer SoCs.

My question is about forbidding 16k pages for sdm845 only or for other
chips too. I'd assume that it shouldn't also work for other smmu-v2
platforms.

Konrad Dybcio July 30, 2024, 10:12 a.m. UTC | #6

On 30.07.2024 10:50 AM, Dmitry Baryshkov wrote:
> On Tue, 30 Jul 2024 at 11:08, Konrad Dybcio <[e-mail]> wrote:
>>
>> On 29.07.2024 11:21 PM, Dmitry Baryshkov wrote:
>>> On Tue, 30 Jul 2024 at 00:08, Rob Clark <[e-mail]> wrote:
>>>>
>>>> On Mon, Jul 29, 2024 at 1:14 PM Dmitry Baryshkov
>>>> <[e-mail]> wrote:
>>>>>
>>>>> On Mon, Jul 29, 2024 at 10:37:48AM GMT, Konrad Dybcio wrote:
>>>>>> From: Konrad Dybcio <[e-mail]>
>>>>>>
>>>>>> SDM845's Adreno SMMU is unique in that it actually advertizes support
>>>>>> for 16K (and 32M) pages, which doesn't hold for newer SoCs.
>>>>>>
>>>>>> This however, seems either broken in the hardware implementation, the
>>>>>> hypervisor middleware that abstracts the SMMU, or there's a bug in the
>>>>>> Linux kernel somewhere down the line that nobody managed to track down.
>>>>>>
>>>>>> Booting SDM845 with 16K page sizes and drm/msm results in:
>>>>>>
>>>>>> *** gpu fault: ttbr0=0000000000000000 iova=000100000000c000 dir=READ
>>>>>> type=TRANSLATION source=CP (0,0,0,0)
>>>>>>
>>>>>> right after loading the firmware. The GPU then starts spitting out
>>>>>> illegal intstruction errors, as it's quite obvious that it got a
>>>>>> bogus pointer.
>>>>>>
>>>>>> Hide 16K support on SDM845's Adreno SMMU to work around this.
>>>>>>
>>>>>> Reported-by: Sumit Semwal <[e-mail]>
>>>>>> Signed-off-by: Konrad Dybcio <[e-mail]>
>>>>>> ---
>>>>>> There's a mismatch in sender/committer addresses but that's "fine":
>>>>>> https://lore.kernel.org/linux-usb/2024072734-scenic-unwilling-71ea@gregkh/
>>>>>> ---
>>>>>>  drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c | 10 ++++++++++
>>>>>>  1 file changed, 10 insertions(+)
>>>>>>
>>>>>> diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
>>>>>> index 36c6b36ad4ff..d25825c05817 100644
>>>>>> --- a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
>>>>>> +++ b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
>>>>>> @@ -338,6 +338,15 @@ static int qcom_smmu_cfg_probe(struct arm_smmu_device *smmu)
>>>>>>       return 0;
>>>>>>  }
>>>>>>
>>>>>> +static int qcom_adreno_smmuv2_cfg_probe(struct arm_smmu_device *smmu)
>>>>>> +{
>>>>>> +     /* SDM845 Adreno SMMU advertizes 16K pages support, but something is broken */
>>>>>> +     if (of_device_is_compatible(smmu->dev->of_node, "qcom,sdm845-smmu-v2"))
>>>>>> +             smmu->features &= ~ARM_SMMU_FEAT_FMT_AARCH64_16K;
>>>>>
>>>>> Shouldn't we hide that uncoditionally as it's likely that none of v2
>>>>> Adreno SMMUs support 16k pages?
>>>>
>>>> Hmm, that would be unfortunate to have the GPU not supporting the CPU
>>>> page size.  I guess we could still map 16k pages as multiple 4k pages,
>>>> but that is a bit sad..
>>>
>>> For now this might be limited to older platforms (v2 vs -500)
>>
>> In the commit message:
>>
>>>>>> SDM845's Adreno SMMU is unique in that it actually advertizes support
>>>>>> for 16K (and 32M) pages, which doesn't hold for newer SoCs.
> 
> My question is about forbidding 16k pages for sdm845 only or for other
> chips too. I'd assume that it shouldn't also work for other smmu-v2
> platforms.

I'd say we shouldn't cause trouble unless we know it's an issue

Konrad

Rob Clark July 30, 2024, 3:57 p.m. UTC | #7

On Tue, Jul 30, 2024 at 3:12 AM Konrad Dybcio <[e-mail]> wrote:
>
> On 30.07.2024 10:50 AM, Dmitry Baryshkov wrote:
> > On Tue, 30 Jul 2024 at 11:08, Konrad Dybcio <[e-mail]> wrote:
> >>
> >> On 29.07.2024 11:21 PM, Dmitry Baryshkov wrote:
> >>> On Tue, 30 Jul 2024 at 00:08, Rob Clark <[e-mail]> wrote:
> >>>>
> >>>> On Mon, Jul 29, 2024 at 1:14 PM Dmitry Baryshkov
> >>>> <[e-mail]> wrote:
> >>>>>
> >>>>> On Mon, Jul 29, 2024 at 10:37:48AM GMT, Konrad Dybcio wrote:
> >>>>>> From: Konrad Dybcio <[e-mail]>
> >>>>>>
> >>>>>> SDM845's Adreno SMMU is unique in that it actually advertizes support
> >>>>>> for 16K (and 32M) pages, which doesn't hold for newer SoCs.
> >>>>>>
> >>>>>> This however, seems either broken in the hardware implementation, the
> >>>>>> hypervisor middleware that abstracts the SMMU, or there's a bug in the
> >>>>>> Linux kernel somewhere down the line that nobody managed to track down.
> >>>>>>
> >>>>>> Booting SDM845 with 16K page sizes and drm/msm results in:
> >>>>>>
> >>>>>> *** gpu fault: ttbr0=0000000000000000 iova=000100000000c000 dir=READ
> >>>>>> type=TRANSLATION source=CP (0,0,0,0)
> >>>>>>
> >>>>>> right after loading the firmware. The GPU then starts spitting out
> >>>>>> illegal intstruction errors, as it's quite obvious that it got a
> >>>>>> bogus pointer.
> >>>>>>
> >>>>>> Hide 16K support on SDM845's Adreno SMMU to work around this.
> >>>>>>
> >>>>>> Reported-by: Sumit Semwal <[e-mail]>
> >>>>>> Signed-off-by: Konrad Dybcio <[e-mail]>
> >>>>>> ---
> >>>>>> There's a mismatch in sender/committer addresses but that's "fine":
> >>>>>> https://lore.kernel.org/linux-usb/2024072734-scenic-unwilling-71ea@gregkh/
> >>>>>> ---
> >>>>>>  drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c | 10 ++++++++++
> >>>>>>  1 file changed, 10 insertions(+)
> >>>>>>
> >>>>>> diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
> >>>>>> index 36c6b36ad4ff..d25825c05817 100644
> >>>>>> --- a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
> >>>>>> +++ b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
> >>>>>> @@ -338,6 +338,15 @@ static int qcom_smmu_cfg_probe(struct arm_smmu_device *smmu)
> >>>>>>       return 0;
> >>>>>>  }
> >>>>>>
> >>>>>> +static int qcom_adreno_smmuv2_cfg_probe(struct arm_smmu_device *smmu)
> >>>>>> +{
> >>>>>> +     /* SDM845 Adreno SMMU advertizes 16K pages support, but something is broken */
> >>>>>> +     if (of_device_is_compatible(smmu->dev->of_node, "qcom,sdm845-smmu-v2"))
> >>>>>> +             smmu->features &= ~ARM_SMMU_FEAT_FMT_AARCH64_16K;
> >>>>>
> >>>>> Shouldn't we hide that uncoditionally as it's likely that none of v2
> >>>>> Adreno SMMUs support 16k pages?
> >>>>
> >>>> Hmm, that would be unfortunate to have the GPU not supporting the CPU
> >>>> page size.  I guess we could still map 16k pages as multiple 4k pages,
> >>>> but that is a bit sad..
> >>>
> >>> For now this might be limited to older platforms (v2 vs -500)
> >>
> >> In the commit message:
> >>
> >>>>>> SDM845's Adreno SMMU is unique in that it actually advertizes support
> >>>>>> for 16K (and 32M) pages, which doesn't hold for newer SoCs.
> >
> > My question is about forbidding 16k pages for sdm845 only or for other
> > chips too. I'd assume that it shouldn't also work for other smmu-v2
> > platforms.
>
> I'd say we shouldn't cause trouble unless we know it's an issue

I guess the other direction (gpu only supporting _larger_ pgsize as
cpu) would be more problematic

What is the next larger pg size above 4k which is supported by both gpu and cpu?

BR,
-R

Rob Clark July 30, 2024, 4:39 p.m. UTC | #8

On Mon, Jul 29, 2024 at 1:37 AM Konrad Dybcio <[e-mail]> wrote:
>
> From: Konrad Dybcio <[e-mail]>
>
> SDM845's Adreno SMMU is unique in that it actually advertizes support
> for 16K (and 32M) pages, which doesn't hold for newer SoCs.
>
> This however, seems either broken in the hardware implementation, the
> hypervisor middleware that abstracts the SMMU, or there's a bug in the
> Linux kernel somewhere down the line that nobody managed to track down.
>
> Booting SDM845 with 16K page sizes and drm/msm results in:
>
> *** gpu fault: ttbr0=0000000000000000 iova=000100000000c000 dir=READ
> type=TRANSLATION source=CP (0,0,0,0)
>
> right after loading the firmware. The GPU then starts spitting out
> illegal intstruction errors, as it's quite obvious that it got a
> bogus pointer.
>
> Hide 16K support on SDM845's Adreno SMMU to work around this.
>
> Reported-by: Sumit Semwal <[e-mail]>
> Signed-off-by: Konrad Dybcio <[e-mail]>
> ---
> There's a mismatch in sender/committer addresses but that's "fine":
> https://lore.kernel.org/linux-usb/2024072734-scenic-unwilling-71ea@gregkh/
> ---
>  drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)
>
> diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
> index 36c6b36ad4ff..d25825c05817 100644
> --- a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
> +++ b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
> @@ -338,6 +338,15 @@ static int qcom_smmu_cfg_probe(struct arm_smmu_device *smmu)
>         return 0;
>  }
>
> +static int qcom_adreno_smmuv2_cfg_probe(struct arm_smmu_device *smmu)
> +{
> +       /* SDM845 Adreno SMMU advertizes 16K pages support, but something is broken */
> +       if (of_device_is_compatible(smmu->dev->of_node, "qcom,sdm845-smmu-v2"))
> +               smmu->features &= ~ARM_SMMU_FEAT_FMT_AARCH64_16K;

jfwiw, it looks like we need this for sc7180 as well.. probably just
remove the "if" and mask out 16K for all smmu-v2, until we know that
there is somewhere that it _does_ work?

BR,
-R


> +
> +       return 0;
> +}
> +
>  static void qcom_smmu_write_s2cr(struct arm_smmu_device *smmu, int idx)
>  {
>         struct arm_smmu_s2cr *s2cr = smmu->s2crs + idx;
> @@ -436,6 +445,7 @@ static const struct arm_smmu_impl sdm845_smmu_500_impl = {
>
>  static const struct arm_smmu_impl qcom_adreno_smmu_v2_impl = {
>         .init_context = qcom_adreno_smmu_init_context,
> +       .cfg_probe = qcom_adreno_smmuv2_cfg_probe,
>         .def_domain_type = qcom_smmu_def_domain_type,
>         .alloc_context_bank = qcom_adreno_smmu_alloc_context_bank,
>         .write_sctlr = qcom_adreno_smmu_write_sctlr,
>
> ---
> base-commit: 931a3b3bccc96e7708c82b30b2b5fa82dfd04890
> change-id: 20240726-topic-845_gpu_smmu-ab738f7a013c
>
> Best regards,
> --
> Konrad Dybcio <[e-mail]>
>

Trilok Soni July 30, 2024, 5:16 p.m. UTC | #9

On 7/30/2024 1:50 AM, Dmitry Baryshkov wrote:
>>>>>> SDM845's Adreno SMMU is unique in that it actually advertizes support
>>>>>> for 16K (and 32M) pages, which doesn't hold for newer SoCs.
> My question is about forbidding 16k pages for sdm845 only or for other
> chips too. I'd assume that it shouldn't also work for other smmu-v2
> platforms.

Yes, my understanding was that SMMUv2 based IPs doesn't have 16k support
and it is only starting from SMMUv3.

Will Deacon Aug. 23, 2024, 3:21 p.m. UTC | #10

On Tue, Jul 30, 2024 at 10:16:00AM -0700, Trilok Soni wrote:
> On 7/30/2024 1:50 AM, Dmitry Baryshkov wrote:
> >>>>>> SDM845's Adreno SMMU is unique in that it actually advertizes support
> >>>>>> for 16K (and 32M) pages, which doesn't hold for newer SoCs.
> > My question is about forbidding 16k pages for sdm845 only or for other
> > chips too. I'd assume that it shouldn't also work for other smmu-v2
> > platforms.
> 
> Yes, my understanding was that SMMUv2 based IPs doesn't have 16k support
> and it is only starting from SMMUv3. 

I'm not sure about that. The architecture doc for SMMUv2 talks about the
AArch64 translation regime in section 1.5 and bit 13 of SMMU_IDR2 says:

  | PTFSv8_16kB, bit[13]
  | Support for 16KB translation granule size. The possible values of this bit are:
  | 0	The 16KB translation granule is not supported.
  | 1	The 16KB translation granule is supported.
  | In SMMUv1, this bit is reserved.

so I think Konrad's patch is about right, but if you want to extend it
to cover other implementations then that's fine too.

Will

Rob Clark Aug. 23, 2024, 5:15 p.m. UTC | #11

On Fri, Aug 23, 2024 at 8:21 AM Will Deacon <[e-mail]> wrote:
>
> On Tue, Jul 30, 2024 at 10:16:00AM -0700, Trilok Soni wrote:
> > On 7/30/2024 1:50 AM, Dmitry Baryshkov wrote:
> > >>>>>> SDM845's Adreno SMMU is unique in that it actually advertizes support
> > >>>>>> for 16K (and 32M) pages, which doesn't hold for newer SoCs.
> > > My question is about forbidding 16k pages for sdm845 only or for other
> > > chips too. I'd assume that it shouldn't also work for other smmu-v2
> > > platforms.
> >
> > Yes, my understanding was that SMMUv2 based IPs doesn't have 16k support
> > and it is only starting from SMMUv3.
>
> I'm not sure about that. The architecture doc for SMMUv2 talks about the
> AArch64 translation regime in section 1.5 and bit 13 of SMMU_IDR2 says:
>
>   | PTFSv8_16kB, bit[13]
>   | Support for 16KB translation granule size. The possible values of this bit are:
>   | 0   The 16KB translation granule is not supported.
>   | 1   The 16KB translation granule is supported.
>   | In SMMUv1, this bit is reserved.
>
> so I think Konrad's patch is about right, but if you want to extend it
> to cover other implementations then that's fine too.
>

Perhaps that should have been "qcom's SMMUv2 based IPs doesn't have
16k support"?

At any rate, 16k sizes don't appear to work on sc7180 as well.  I
don't really have any other data points but it wouldn't really
surprise me if this applied to all qc smmu-v2

BR,
-R

Patch

diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
index 36c6b36ad4ff..d25825c05817 100644
--- a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
+++ b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
@@ -338,6 +338,15 @@  static int qcom_smmu_cfg_probe(struct arm_smmu_device *smmu)
 	return 0;
 }
 
+static int qcom_adreno_smmuv2_cfg_probe(struct arm_smmu_device *smmu)
+{
+	/* SDM845 Adreno SMMU advertizes 16K pages support, but something is broken */
+	if (of_device_is_compatible(smmu->dev->of_node, "qcom,sdm845-smmu-v2"))
+		smmu->features &= ~ARM_SMMU_FEAT_FMT_AARCH64_16K;
+
+	return 0;
+}
+
 static void qcom_smmu_write_s2cr(struct arm_smmu_device *smmu, int idx)
 {
 	struct arm_smmu_s2cr *s2cr = smmu->s2crs + idx;
@@ -436,6 +445,7 @@  static const struct arm_smmu_impl sdm845_smmu_500_impl = {
 
 static const struct arm_smmu_impl qcom_adreno_smmu_v2_impl = {
 	.init_context = qcom_adreno_smmu_init_context,
+	.cfg_probe = qcom_adreno_smmuv2_cfg_probe,
 	.def_domain_type = qcom_smmu_def_domain_type,
 	.alloc_context_bank = qcom_adreno_smmu_alloc_context_bank,
 	.write_sctlr = qcom_adreno_smmu_write_sctlr,