[PATCH v2 0/2] drm/msm/adreno: Setup SMMU aparture

[PATCH v2 0/2] drm/msm/adreno: Setup SMMU aparture

[Bjorn Andersson]

Support for per-page tables requires the SMMU aparture to be setup, on
some targets this is done statically in firmware, on others it's
expected to be requested in runtime by the driver, through a SCM call.

Marking the series as RFT, as this has been tested on a few different
modern platforms, but only with Qualcomm presence in EL2.

I did receive Tested-by from Konrad and Jessica, not picking these up,
as I change the firmware call pattern in this version by introducing a
"is_available" check.

Signed-off-by: Bjorn Andersson <bjorn.andersson@oss.qualcomm.com>
---
Changes in v2:
- Added a check to see if the SCM call is available, to avoid possibly
  error prints.
- Link to v1: https://lore.kernel.org/r/20241002-adreno-smmu-aparture-v1-0-e9a63c9ccef5@oss.qualcomm.com

---
Bjorn Andersson (2):
      firmware: qcom: scm: Introduce CP_SMMU_APERTURE_ID
      drm/msm/adreno: Setup SMMU aparture for per-process page table

 drivers/firmware/qcom/qcom_scm.c        | 26 ++++++++++++++++++++++++++
 drivers/firmware/qcom/qcom_scm.h        |  1 +
 drivers/gpu/drm/msm/adreno/adreno_gpu.c | 11 +++++++++++
 include/linux/firmware/qcom/qcom_scm.h  |  2 ++
 4 files changed, 40 insertions(+)
---
base-commit: 929beafbe7acce3267c06115e13e03ff6e50548a
change-id: 20241002-adreno-smmu-aparture-fe7d5a1cb834

Best regards,
-- 
Bjorn Andersson <bjorn.andersson@oss.qualcomm.com>

[PATCH v2 1/2] firmware: qcom: scm: Introduce CP_SMMU_APERTURE_ID

[Bjorn Andersson]

The QCOM_SCM_SVC_MP service provides QCOM_SCM_MP_CP_SMMU_APERTURE_ID,
which is used to trigger the mapping of register banks into the SMMU
context for per-processes page tables to function (in case this isn't
statically setup by firmware).

This is necessary on e.g. QCS6490 Rb3Gen2, in order to avoid "CP | AHB
bus error"-errors from the GPU.

Introduce a function to allow the msm driver to invoke this call.

Signed-off-by: Bjorn Andersson <bjorn.andersson@oss.qualcomm.com>
---
 drivers/firmware/qcom/qcom_scm.c       | 26 ++++++++++++++++++++++++++
 drivers/firmware/qcom/qcom_scm.h       |  1 +
 include/linux/firmware/qcom/qcom_scm.h |  2 ++
 3 files changed, 29 insertions(+)

diff --git a/drivers/firmware/qcom/qcom_scm.c b/drivers/firmware/qcom/qcom_scm.c
index 95815e64e1e6..72bf87ddcd96 100644
--- a/drivers/firmware/qcom/qcom_scm.c
+++ b/drivers/firmware/qcom/qcom_scm.c
@@ -904,6 +904,32 @@ int qcom_scm_restore_sec_cfg(u32 device_id, u32 spare)
 }
 EXPORT_SYMBOL_GPL(qcom_scm_restore_sec_cfg);
 
+#define QCOM_SCM_CP_APERTURE_CONTEXT_MASK	GENMASK(7, 0)
+
+bool qcom_scm_set_gpu_smmu_aperture_is_available(void)
+{
+	return __qcom_scm_is_call_available(__scm->dev, QCOM_SCM_SVC_MP,
+					    QCOM_SCM_MP_CP_SMMU_APERTURE_ID);
+}
+EXPORT_SYMBOL_GPL(qcom_scm_set_gpu_smmu_aperture_is_available);
+
+int qcom_scm_set_gpu_smmu_aperture(unsigned int context_bank)
+{
+	struct qcom_scm_desc desc = {
+		.svc = QCOM_SCM_SVC_MP,
+		.cmd = QCOM_SCM_MP_CP_SMMU_APERTURE_ID,
+		.arginfo = QCOM_SCM_ARGS(4),
+		.args[0] = 0xffff0000 | FIELD_PREP(QCOM_SCM_CP_APERTURE_CONTEXT_MASK, context_bank),
+		.args[1] = 0xffffffff,
+		.args[2] = 0xffffffff,
+		.args[3] = 0xffffffff,
+		.owner = ARM_SMCCC_OWNER_SIP
+	};
+
+	return qcom_scm_call(__scm->dev, &desc, NULL);
+}
+EXPORT_SYMBOL_GPL(qcom_scm_set_gpu_smmu_aperture);
+
 int qcom_scm_iommu_secure_ptbl_size(u32 spare, size_t *size)
 {
 	struct qcom_scm_desc desc = {
diff --git a/drivers/firmware/qcom/qcom_scm.h b/drivers/firmware/qcom/qcom_scm.h
index 685b8f59e7a6..e36b2f67607f 100644
--- a/drivers/firmware/qcom/qcom_scm.h
+++ b/drivers/firmware/qcom/qcom_scm.h
@@ -116,6 +116,7 @@ struct qcom_tzmem_pool *qcom_scm_get_tzmem_pool(void);
 #define QCOM_SCM_MP_IOMMU_SET_CP_POOL_SIZE	0x05
 #define QCOM_SCM_MP_VIDEO_VAR			0x08
 #define QCOM_SCM_MP_ASSIGN			0x16
+#define QCOM_SCM_MP_CP_SMMU_APERTURE_ID		0x1b
 #define QCOM_SCM_MP_SHM_BRIDGE_ENABLE		0x1c
 #define QCOM_SCM_MP_SHM_BRIDGE_DELETE		0x1d
 #define QCOM_SCM_MP_SHM_BRIDGE_CREATE		0x1e
diff --git a/include/linux/firmware/qcom/qcom_scm.h b/include/linux/firmware/qcom/qcom_scm.h
index 9f14976399ab..4621aec0328c 100644
--- a/include/linux/firmware/qcom/qcom_scm.h
+++ b/include/linux/firmware/qcom/qcom_scm.h
@@ -85,6 +85,8 @@ int qcom_scm_io_writel(phys_addr_t addr, unsigned int val);
 
 bool qcom_scm_restore_sec_cfg_available(void);
 int qcom_scm_restore_sec_cfg(u32 device_id, u32 spare);
+int qcom_scm_set_gpu_smmu_aperture(unsigned int context_bank);
+bool qcom_scm_set_gpu_smmu_aperture_is_available(void);
 int qcom_scm_iommu_secure_ptbl_size(u32 spare, size_t *size);
 int qcom_scm_iommu_secure_ptbl_init(u64 addr, u32 size, u32 spare);
 int qcom_scm_iommu_set_cp_pool_size(u32 spare, u32 size);

-- 
2.45.2

[PATCH v2 2/2] drm/msm/adreno: Setup SMMU aparture for per-process page table

[Bjorn Andersson]

Support for per-process page tables requires the SMMU aparture to be
setup such that the GPU can make updates with the SMMU. On some targets
this is done statically in firmware, on others it's expected to be
requested in runtime by the driver, through a SCM call.

One place where configuration is expected to be done dynamically is the
QCS6490 rb3gen2.

The downstream driver does this unconditioanlly on any A6xx and newer,
so follow suite and make the call.

Signed-off-by: Bjorn Andersson <bjorn.andersson@oss.qualcomm.com>
---
 drivers/gpu/drm/msm/adreno/adreno_gpu.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/drivers/gpu/drm/msm/adreno/adreno_gpu.c b/drivers/gpu/drm/msm/adreno/adreno_gpu.c
index 076be0473eb5..75f5367e73ca 100644
--- a/drivers/gpu/drm/msm/adreno/adreno_gpu.c
+++ b/drivers/gpu/drm/msm/adreno/adreno_gpu.c
@@ -572,8 +572,19 @@ struct drm_gem_object *adreno_fw_create_bo(struct msm_gpu *gpu,
 
 int adreno_hw_init(struct msm_gpu *gpu)
 {
+	struct adreno_gpu *adreno_gpu = to_adreno_gpu(gpu);
+	int ret;
+
 	VERB("%s", gpu->name);
 
+	if (adreno_gpu->info->family >= ADRENO_6XX_GEN1 &&
+	    qcom_scm_set_gpu_smmu_aperture_is_available()) {
+		/* We currently always use context bank 0, so hard code this */
+		ret = qcom_scm_set_gpu_smmu_aperture(0);
+		if (ret)
+			DRM_DEV_ERROR(gpu->dev->dev, "unable to set SMMU aperture: %d\n", ret);
+	}
+
 	for (int i = 0; i < gpu->nr_rings; i++) {
 		struct msm_ringbuffer *ring = gpu->rb[i];
 

-- 
2.45.2

Re: [PATCH v2 1/2] firmware: qcom: scm: Introduce CP_SMMU_APERTURE_ID

[Rob Clark]

On Sun, Nov 10, 2024 at 9:31 AM Bjorn Andersson
<bjorn.andersson@oss.qualcomm.com> wrote:
>
> The QCOM_SCM_SVC_MP service provides QCOM_SCM_MP_CP_SMMU_APERTURE_ID,
> which is used to trigger the mapping of register banks into the SMMU
> context for per-processes page tables to function (in case this isn't
> statically setup by firmware).
>
> This is necessary on e.g. QCS6490 Rb3Gen2, in order to avoid "CP | AHB
> bus error"-errors from the GPU.
>
> Introduce a function to allow the msm driver to invoke this call.
>
> Signed-off-by: Bjorn Andersson <bjorn.andersson@oss.qualcomm.com>

Reviewed-by: Rob Clark <robdclark@gmail.com>

> ---
>  drivers/firmware/qcom/qcom_scm.c       | 26 ++++++++++++++++++++++++++
>  drivers/firmware/qcom/qcom_scm.h       |  1 +
>  include/linux/firmware/qcom/qcom_scm.h |  2 ++
>  3 files changed, 29 insertions(+)
>
> diff --git a/drivers/firmware/qcom/qcom_scm.c b/drivers/firmware/qcom/qcom_scm.c
> index 95815e64e1e6..72bf87ddcd96 100644
> --- a/drivers/firmware/qcom/qcom_scm.c
> +++ b/drivers/firmware/qcom/qcom_scm.c
> @@ -904,6 +904,32 @@ int qcom_scm_restore_sec_cfg(u32 device_id, u32 spare)
>  }
>  EXPORT_SYMBOL_GPL(qcom_scm_restore_sec_cfg);
>
> +#define QCOM_SCM_CP_APERTURE_CONTEXT_MASK      GENMASK(7, 0)
> +
> +bool qcom_scm_set_gpu_smmu_aperture_is_available(void)
> +{
> +       return __qcom_scm_is_call_available(__scm->dev, QCOM_SCM_SVC_MP,
> +                                           QCOM_SCM_MP_CP_SMMU_APERTURE_ID);
> +}
> +EXPORT_SYMBOL_GPL(qcom_scm_set_gpu_smmu_aperture_is_available);
> +
> +int qcom_scm_set_gpu_smmu_aperture(unsigned int context_bank)
> +{
> +       struct qcom_scm_desc desc = {
> +               .svc = QCOM_SCM_SVC_MP,
> +               .cmd = QCOM_SCM_MP_CP_SMMU_APERTURE_ID,
> +               .arginfo = QCOM_SCM_ARGS(4),
> +               .args[0] = 0xffff0000 | FIELD_PREP(QCOM_SCM_CP_APERTURE_CONTEXT_MASK, context_bank),
> +               .args[1] = 0xffffffff,
> +               .args[2] = 0xffffffff,
> +               .args[3] = 0xffffffff,
> +               .owner = ARM_SMCCC_OWNER_SIP
> +       };
> +
> +       return qcom_scm_call(__scm->dev, &desc, NULL);
> +}
> +EXPORT_SYMBOL_GPL(qcom_scm_set_gpu_smmu_aperture);
> +
>  int qcom_scm_iommu_secure_ptbl_size(u32 spare, size_t *size)
>  {
>         struct qcom_scm_desc desc = {
> diff --git a/drivers/firmware/qcom/qcom_scm.h b/drivers/firmware/qcom/qcom_scm.h
> index 685b8f59e7a6..e36b2f67607f 100644
> --- a/drivers/firmware/qcom/qcom_scm.h
> +++ b/drivers/firmware/qcom/qcom_scm.h
> @@ -116,6 +116,7 @@ struct qcom_tzmem_pool *qcom_scm_get_tzmem_pool(void);
>  #define QCOM_SCM_MP_IOMMU_SET_CP_POOL_SIZE     0x05
>  #define QCOM_SCM_MP_VIDEO_VAR                  0x08
>  #define QCOM_SCM_MP_ASSIGN                     0x16
> +#define QCOM_SCM_MP_CP_SMMU_APERTURE_ID                0x1b
>  #define QCOM_SCM_MP_SHM_BRIDGE_ENABLE          0x1c
>  #define QCOM_SCM_MP_SHM_BRIDGE_DELETE          0x1d
>  #define QCOM_SCM_MP_SHM_BRIDGE_CREATE          0x1e
> diff --git a/include/linux/firmware/qcom/qcom_scm.h b/include/linux/firmware/qcom/qcom_scm.h
> index 9f14976399ab..4621aec0328c 100644
> --- a/include/linux/firmware/qcom/qcom_scm.h
> +++ b/include/linux/firmware/qcom/qcom_scm.h
> @@ -85,6 +85,8 @@ int qcom_scm_io_writel(phys_addr_t addr, unsigned int val);
>
>  bool qcom_scm_restore_sec_cfg_available(void);
>  int qcom_scm_restore_sec_cfg(u32 device_id, u32 spare);
> +int qcom_scm_set_gpu_smmu_aperture(unsigned int context_bank);
> +bool qcom_scm_set_gpu_smmu_aperture_is_available(void);
>  int qcom_scm_iommu_secure_ptbl_size(u32 spare, size_t *size);
>  int qcom_scm_iommu_secure_ptbl_init(u64 addr, u32 size, u32 spare);
>  int qcom_scm_iommu_set_cp_pool_size(u32 spare, u32 size);
>
> --
> 2.45.2
>

Re: [PATCH v2 2/2] drm/msm/adreno: Setup SMMU aparture for per-process page table

[Rob Clark]

On Sun, Nov 10, 2024 at 9:31 AM Bjorn Andersson
<bjorn.andersson@oss.qualcomm.com> wrote:
>
> Support for per-process page tables requires the SMMU aparture to be
> setup such that the GPU can make updates with the SMMU. On some targets
> this is done statically in firmware, on others it's expected to be
> requested in runtime by the driver, through a SCM call.
>
> One place where configuration is expected to be done dynamically is the
> QCS6490 rb3gen2.
>
> The downstream driver does this unconditioanlly on any A6xx and newer,

nit, s/unconditioanlly/unconditionally/

> so follow suite and make the call.
>
> Signed-off-by: Bjorn Andersson <bjorn.andersson@oss.qualcomm.com>

Reviewed-by: Rob Clark <robdclark@gmail.com>


> ---
>  drivers/gpu/drm/msm/adreno/adreno_gpu.c | 11 +++++++++++
>  1 file changed, 11 insertions(+)
>
> diff --git a/drivers/gpu/drm/msm/adreno/adreno_gpu.c b/drivers/gpu/drm/msm/adreno/adreno_gpu.c
> index 076be0473eb5..75f5367e73ca 100644
> --- a/drivers/gpu/drm/msm/adreno/adreno_gpu.c
> +++ b/drivers/gpu/drm/msm/adreno/adreno_gpu.c
> @@ -572,8 +572,19 @@ struct drm_gem_object *adreno_fw_create_bo(struct msm_gpu *gpu,
>
>  int adreno_hw_init(struct msm_gpu *gpu)
>  {
> +       struct adreno_gpu *adreno_gpu = to_adreno_gpu(gpu);
> +       int ret;
> +
>         VERB("%s", gpu->name);
>
> +       if (adreno_gpu->info->family >= ADRENO_6XX_GEN1 &&
> +           qcom_scm_set_gpu_smmu_aperture_is_available()) {
> +               /* We currently always use context bank 0, so hard code this */
> +               ret = qcom_scm_set_gpu_smmu_aperture(0);
> +               if (ret)
> +                       DRM_DEV_ERROR(gpu->dev->dev, "unable to set SMMU aperture: %d\n", ret);
> +       }
> +
>         for (int i = 0; i < gpu->nr_rings; i++) {
>                 struct msm_ringbuffer *ring = gpu->rb[i];
>
>
> --
> 2.45.2
>

Re: [PATCH v2 0/2] drm/msm/adreno: Setup SMMU aparture

[Bjorn Andersson]

On Sun, 10 Nov 2024 09:33:39 -0800, Bjorn Andersson wrote:
> Support for per-page tables requires the SMMU aparture to be setup, on
> some targets this is done statically in firmware, on others it's
> expected to be requested in runtime by the driver, through a SCM call.
> 
> Marking the series as RFT, as this has been tested on a few different
> modern platforms, but only with Qualcomm presence in EL2.
> 
> [...]

Applied, thanks!

[1/2] firmware: qcom: scm: Introduce CP_SMMU_APERTURE_ID
      commit: 1af75b2ad08bd5977c51c2d0fc11741a4c0a48d9
[2/2] drm/msm/adreno: Setup SMMU aparture for per-process page table
      commit: 98e5b7f98356cef2f13b54862ca9ac016b71ff06

Best regards,
-- 
Bjorn Andersson <andersson@kernel.org>

Re: [PATCH v2 2/2] drm/msm/adreno: Setup SMMU aparture for per-process page table

[Akhil P Oommen]

On 11/11/2024 8:38 PM, Rob Clark wrote:
> On Sun, Nov 10, 2024 at 9:31 AM Bjorn Andersson
> <bjorn.andersson@oss.qualcomm.com> wrote:
>>
>> Support for per-process page tables requires the SMMU aparture to be
>> setup such that the GPU can make updates with the SMMU. On some targets
>> this is done statically in firmware, on others it's expected to be
>> requested in runtime by the driver, through a SCM call.
>>
>> One place where configuration is expected to be done dynamically is the
>> QCS6490 rb3gen2.
>>
>> The downstream driver does this unconditioanlly on any A6xx and newer,
> 
> nit, s/unconditioanlly/unconditionally/
> 
>> so follow suite and make the call.
>>
>> Signed-off-by: Bjorn Andersson <bjorn.andersson@oss.qualcomm.com>
> 
> Reviewed-by: Rob Clark <robdclark@gmail.com>
> 
> 
>> ---
>>  drivers/gpu/drm/msm/adreno/adreno_gpu.c | 11 +++++++++++
>>  1 file changed, 11 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/msm/adreno/adreno_gpu.c b/drivers/gpu/drm/msm/adreno/adreno_gpu.c
>> index 076be0473eb5..75f5367e73ca 100644
>> --- a/drivers/gpu/drm/msm/adreno/adreno_gpu.c
>> +++ b/drivers/gpu/drm/msm/adreno/adreno_gpu.c
>> @@ -572,8 +572,19 @@ struct drm_gem_object *adreno_fw_create_bo(struct msm_gpu *gpu,
>>
>>  int adreno_hw_init(struct msm_gpu *gpu)
>>  {

SCM calls into TZ can block for a very long time (seconds). It depends
on concurrent activities from other drivers like crypto for eg:. So we
should not do this in the gpu wake up path.

Practically, gpu probe is the better place to do this.

-Akhil

>> +       struct adreno_gpu *adreno_gpu = to_adreno_gpu(gpu);
>> +       int ret;
>> +
>>         VERB("%s", gpu->name);
>>
>> +       if (adreno_gpu->info->family >= ADRENO_6XX_GEN1 &&
>> +           qcom_scm_set_gpu_smmu_aperture_is_available()) {
>> +               /* We currently always use context bank 0, so hard code this */
>> +               ret = qcom_scm_set_gpu_smmu_aperture(0);
>> +               if (ret)
>> +                       DRM_DEV_ERROR(gpu->dev->dev, "unable to set SMMU aperture: %d\n", ret);
>> +       }
>> +
>>         for (int i = 0; i < gpu->nr_rings; i++) {
>>                 struct msm_ringbuffer *ring = gpu->rb[i];
>>
>>
>> --
>> 2.45.2
>>

Re: [PATCH v2 2/2] drm/msm/adreno: Setup SMMU aparture for per-process page table

[Konrad Dybcio]

On 12.11.2024 10:15 PM, Akhil P Oommen wrote:
> On 11/11/2024 8:38 PM, Rob Clark wrote:
>> On Sun, Nov 10, 2024 at 9:31 AM Bjorn Andersson
>> <bjorn.andersson@oss.qualcomm.com> wrote:
>>>
>>> Support for per-process page tables requires the SMMU aparture to be
>>> setup such that the GPU can make updates with the SMMU. On some targets
>>> this is done statically in firmware, on others it's expected to be
>>> requested in runtime by the driver, through a SCM call.
>>>
>>> One place where configuration is expected to be done dynamically is the
>>> QCS6490 rb3gen2.
>>>
>>> The downstream driver does this unconditioanlly on any A6xx and newer,
>>
>> nit, s/unconditioanlly/unconditionally/
>>
>>> so follow suite and make the call.
>>>
>>> Signed-off-by: Bjorn Andersson <bjorn.andersson@oss.qualcomm.com>
>>
>> Reviewed-by: Rob Clark <robdclark@gmail.com>
>>
>>
>>> ---
>>>  drivers/gpu/drm/msm/adreno/adreno_gpu.c | 11 +++++++++++
>>>  1 file changed, 11 insertions(+)
>>>
>>> diff --git a/drivers/gpu/drm/msm/adreno/adreno_gpu.c b/drivers/gpu/drm/msm/adreno/adreno_gpu.c
>>> index 076be0473eb5..75f5367e73ca 100644
>>> --- a/drivers/gpu/drm/msm/adreno/adreno_gpu.c
>>> +++ b/drivers/gpu/drm/msm/adreno/adreno_gpu.c
>>> @@ -572,8 +572,19 @@ struct drm_gem_object *adreno_fw_create_bo(struct msm_gpu *gpu,
>>>
>>>  int adreno_hw_init(struct msm_gpu *gpu)
>>>  {
> 
> SCM calls into TZ can block for a very long time (seconds). It depends
> on concurrent activities from other drivers like crypto for eg:. So we
> should not do this in the gpu wake up path.
> 
> Practically, gpu probe is the better place to do this.

Do we only have to do this once?

Do we have to redo it after CXPC?

Konrad

Re: [PATCH v2 2/2] drm/msm/adreno: Setup SMMU aparture for per-process page table

[Akhil P Oommen]

On 11/14/2024 8:57 PM, Konrad Dybcio wrote:
> On 12.11.2024 10:15 PM, Akhil P Oommen wrote:
>> On 11/11/2024 8:38 PM, Rob Clark wrote:
>>> On Sun, Nov 10, 2024 at 9:31 AM Bjorn Andersson
>>> <bjorn.andersson@oss.qualcomm.com> wrote:
>>>>
>>>> Support for per-process page tables requires the SMMU aparture to be
>>>> setup such that the GPU can make updates with the SMMU. On some targets
>>>> this is done statically in firmware, on others it's expected to be
>>>> requested in runtime by the driver, through a SCM call.
>>>>
>>>> One place where configuration is expected to be done dynamically is the
>>>> QCS6490 rb3gen2.
>>>>
>>>> The downstream driver does this unconditioanlly on any A6xx and newer,
>>>
>>> nit, s/unconditioanlly/unconditionally/
>>>
>>>> so follow suite and make the call.
>>>>
>>>> Signed-off-by: Bjorn Andersson <bjorn.andersson@oss.qualcomm.com>
>>>
>>> Reviewed-by: Rob Clark <robdclark@gmail.com>
>>>
>>>
>>>> ---
>>>>  drivers/gpu/drm/msm/adreno/adreno_gpu.c | 11 +++++++++++
>>>>  1 file changed, 11 insertions(+)
>>>>
>>>> diff --git a/drivers/gpu/drm/msm/adreno/adreno_gpu.c b/drivers/gpu/drm/msm/adreno/adreno_gpu.c
>>>> index 076be0473eb5..75f5367e73ca 100644
>>>> --- a/drivers/gpu/drm/msm/adreno/adreno_gpu.c
>>>> +++ b/drivers/gpu/drm/msm/adreno/adreno_gpu.c
>>>> @@ -572,8 +572,19 @@ struct drm_gem_object *adreno_fw_create_bo(struct msm_gpu *gpu,
>>>>
>>>>  int adreno_hw_init(struct msm_gpu *gpu)
>>>>  {
>>
>> SCM calls into TZ can block for a very long time (seconds). It depends
>> on concurrent activities from other drivers like crypto for eg:. So we
>> should not do this in the gpu wake up path.
>>
>> Practically, gpu probe is the better place to do this.
> 
> Do we only have to do this once?
> 
> Do we have to redo it after CXPC?

Only once. Those registers have retention.

-Akhil.

> 
> Konrad

Re: [PATCH v2 2/2] drm/msm/adreno: Setup SMMU aparture for per-process page table

[Bjorn Andersson]

On Tue, Nov 12, 2024 at 3:15 PM Akhil P Oommen <quic_akhilpo@quicinc.com> wrote:
>
> On 11/11/2024 8:38 PM, Rob Clark wrote:
> > On Sun, Nov 10, 2024 at 9:31 AM Bjorn Andersson
> > <bjorn.andersson@oss.qualcomm.com> wrote:
> >>
> >> Support for per-process page tables requires the SMMU aparture to be
> >> setup such that the GPU can make updates with the SMMU. On some targets
> >> this is done statically in firmware, on others it's expected to be
> >> requested in runtime by the driver, through a SCM call.
> >>
> >> One place where configuration is expected to be done dynamically is the
> >> QCS6490 rb3gen2.
> >>
> >> The downstream driver does this unconditioanlly on any A6xx and newer,
> >
> > nit, s/unconditioanlly/unconditionally/
> >
> >> so follow suite and make the call.
> >>
> >> Signed-off-by: Bjorn Andersson <bjorn.andersson@oss.qualcomm.com>
> >
> > Reviewed-by: Rob Clark <robdclark@gmail.com>
> >
> >
> >> ---
> >>  drivers/gpu/drm/msm/adreno/adreno_gpu.c | 11 +++++++++++
> >>  1 file changed, 11 insertions(+)
> >>
> >> diff --git a/drivers/gpu/drm/msm/adreno/adreno_gpu.c b/drivers/gpu/drm/msm/adreno/adreno_gpu.c
> >> index 076be0473eb5..75f5367e73ca 100644
> >> --- a/drivers/gpu/drm/msm/adreno/adreno_gpu.c
> >> +++ b/drivers/gpu/drm/msm/adreno/adreno_gpu.c
> >> @@ -572,8 +572,19 @@ struct drm_gem_object *adreno_fw_create_bo(struct msm_gpu *gpu,
> >>
> >>  int adreno_hw_init(struct msm_gpu *gpu)
> >>  {
>
> SCM calls into TZ can block for a very long time (seconds). It depends
> on concurrent activities from other drivers like crypto for eg:. So we
> should not do this in the gpu wake up path.
>
> Practically, gpu probe is the better place to do this.
>

Thanks for your feedback, Akhil!

I've yet to see SCM calls take that long, but we don't want that in
the wakeup path, so I have no concerns about moving this call to probe
time if that works.
Based on conversation with Rob I merged the two patches through the
qcom-soc tree, so they are expected to show up in v6.13-rc1.

Let's follow up with a patch that moves the call, once -rc1 is out.
That said, I don't have any means currently to test the retention part...

Thanks,
Bjorn