[v2] drm/msm/adreno: Setup SMMU aparture

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

Posted by Bjorn Andersson 2 weeks, 3 days ago

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 2/2] drm/msm/adreno: Setup SMMU aparture for per-process page table

Posted by Rob Clark 2 weeks, 2 days ago

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 2/2] drm/msm/adreno: Setup SMMU aparture for per-process page table

Posted by Akhil P Oommen 2 weeks, 1 day ago

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

Posted by Bjorn Andersson 1 week, 1 day ago

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

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

Posted by Konrad Dybcio 1 week, 6 days ago

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

Posted by Akhil P Oommen 1 week, 6 days ago

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