Re: [v2] drm/etnaviv: Clear the __GFP_HIGHMEM bit in GFP_HIGHUSER with 32 address

Re: [v2] drm/etnaviv: Clear the __GFP_HIGHMEM bit in GFP_HIGHUSER with 32 address

[Sui Jingfeng]

Hi, Xiaolei


On 2024/8/16 09:55, Wang, Xiaolei wrote:
> Ping ...

I think, the more proper fix that Lucas hint
is to modify the 'priv->shm_gfp_mask' variable
in the|etnaviv_bind() function|. Say:
|Use "priv->shm_gfp_mask = GFP_USER | __GFP_RETRY_MAYFAIL | __GFP_NOWARN;"|

instead of

|"priv->shm_gfp_mask = ||GFP_HIGHUSER||| __GFP_RETRY_MAYFAIL | __GFP_NOWARN;|"


Right?

> thanks
> xiaolei
>
> diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gpu.c b/drivers/gpu/drm/etnaviv/etnaviv_gpu.c
> index 7c7f97793ddd..0e6bdf2d028b 100644
> --- a/drivers/gpu/drm/etnaviv/etnaviv_gpu.c
> +++ b/drivers/gpu/drm/etnaviv/etnaviv_gpu.c
> @@ -844,8 +844,10 @@ int etnaviv_gpu_init(struct etnaviv_gpu *gpu)
>            * request pages for our SHM backend buffers from the DMA32 zone to
>            * hopefully avoid performance killing SWIOTLB bounce buffering.
>            */
> -       if (dma_addressing_limited(gpu->dev))
> +       if (dma_addressing_limited(gpu->dev)) {
>                   priv->shm_gfp_mask |= GFP_DMA32;
> +               priv->shm_gfp_mask &= ~__GFP_HIGHMEM;
> +       }
>
>           /* Create buffer: */
>           ret = etnaviv_cmdbuf_init(priv->cmdbuf_suballoc, &gpu->buffer,

-- 
Best regards,
Sui

Re: [v2] drm/etnaviv: Clear the __GFP_HIGHMEM bit in GFP_HIGHUSER with 32 address

[Sui Jingfeng]

On 2024/8/31 03:40, Sui Jingfeng wrote:
> Hi, Xiaolei
>
>
> On 2024/8/16 09:55, Wang, Xiaolei wrote:
>> Ping ...
>
> I think, the more proper fix that Lucas hint
> is to modify the 'priv->shm_gfp_mask' variable
> in the|etnaviv_bind() function|. Say:
> |Use "priv->shm_gfp_mask = GFP_USER | __GFP_RETRY_MAYFAIL | 
> __GFP_NOWARN;"|
>
> instead of
>
> |"priv->shm_gfp_mask = ||GFP_HIGHUSER||| __GFP_RETRY_MAYFAIL | 
> __GFP_NOWARN;|"
>
>

Oops, please ignore the irrelevant(superfluous) "|" characters in my
reply, my Thunderbird mail client has some problem, generate them unreasonably.

Should be:

priv->shm_gfp_mask = GFP_USER | __GFP_RETRY_MAYFAIL | __GFP_NOWARN;



> Right?
>
>> thanks
>> xiaolei
>>
>> diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gpu.c 
>> b/drivers/gpu/drm/etnaviv/etnaviv_gpu.c
>> index 7c7f97793ddd..0e6bdf2d028b 100644
>> --- a/drivers/gpu/drm/etnaviv/etnaviv_gpu.c
>> +++ b/drivers/gpu/drm/etnaviv/etnaviv_gpu.c
>> @@ -844,8 +844,10 @@ int etnaviv_gpu_init(struct etnaviv_gpu *gpu)
>>            * request pages for our SHM backend buffers from the DMA32 
>> zone to
>>            * hopefully avoid performance killing SWIOTLB bounce 
>> buffering.
>>            */
>> -       if (dma_addressing_limited(gpu->dev))
>> +       if (dma_addressing_limited(gpu->dev)) {
>>                   priv->shm_gfp_mask |= GFP_DMA32;
>> +               priv->shm_gfp_mask &= ~__GFP_HIGHMEM;
>> +       }
>>
>>           /* Create buffer: */
>>           ret = etnaviv_cmdbuf_init(priv->cmdbuf_suballoc, &gpu->buffer,
>
-- 
Best regards,
Sui

Re: [v2] drm/etnaviv: Clear the __GFP_HIGHMEM bit in GFP_HIGHUSER with 32 address

[Sui Jingfeng]

Hi, Xiaolei


Thanks for your nice catch! I have more to say.

On 2024/8/16 09:55, Wang, Xiaolei wrote:
> Ping ...

32 address -> 32-bit address,

Perhaps, we could improve the commit title a little bit
by writing a more accurate sentence if possible, say:

drm/etnaviv: Properly request pages from DMA32 zone when needed

or

drm/etnaviv: Request pages from DMA32 zone on addressing_limited


> thanks
> xiaolei

Vivante GPU is a 32-bit GPU, it do can access 40-bit physical address via its MMU(IOMMU).
But this is only possible *after* the MMU has been setup(initialized). Before GPU page
table is setup(and flush-ed into the GPU's TLB), the device can only access 32-bit
physical addresses and the addresses has to be physical continues in ranges.

The GPU page tables (GART) and command buffer has to reside in low 4GB address.

> diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gpu.c b/drivers/gpu/drm/etnaviv/etnaviv_gpu.c
> index 7c7f97793ddd..0e6bdf2d028b 100644
> --- a/drivers/gpu/drm/etnaviv/etnaviv_gpu.c
> +++ b/drivers/gpu/drm/etnaviv/etnaviv_gpu.c
> @@ -844,8 +844,10 @@ int etnaviv_gpu_init(struct etnaviv_gpu *gpu)
>            * request pages for our SHM backend buffers from the DMA32 zone to
>            * hopefully avoid performance killing SWIOTLB bounce buffering.
>            */
> -       if (dma_addressing_limited(gpu->dev))
> +       if (dma_addressing_limited(gpu->dev)) {
>                   priv->shm_gfp_mask |= GFP_DMA32;
> +               priv->shm_gfp_mask &= ~__GFP_HIGHMEM;
> +       }

The code here  still looks itchy and risky,
because for a i.MX8 SoC with multiple vivante GPU core.
We will modify priv->shm_gfp_mask *multiple* time.

For the 2D core and the 3D core have different DMA addressing constraint.
Then, only the last(latest) modify will be effective. This lead to the
probe order dependent.

However this may not be a problem in practice, as usually, all vivante
GPUs in the system will share the same DMA constraints. And the driver
assume that.

But then, we probably still should not modify the global shared GFP
mask multiple time.

Now that we do assume that all vivante GPUs in the system share the
same DMA constraints. And the DMA constraints information has been
assigned to the virtual master. The right time to modify the
`priv->shm_gfp_mask` should be in the etnaviv_bind() function. as
this can eliminate overlap(repeat) stores.


Please consider move the entire if() {} to etnaviv_bind(), just below
where the 'priv->shm_gfp_mask' was initially initialized.

or alternatively we can just hard-code to use low 4GM memmory only:

priv->shm_gfp_mask = GFP_USER | GFP_DMA32 | __GFP_RETRY_MAYFAIL | __GFP_NOWARN;


Best regards,
Sui

>           /* Create buffer: */
>           ret = etnaviv_cmdbuf_init(priv->cmdbuf_suballoc, &gpu->buffer,

Re: [v2] drm/etnaviv: Clear the __GFP_HIGHMEM bit in GFP_HIGHUSER with 32 address

[wang xiaolei]

On 8/31/24 8:03 AM, Sui Jingfeng wrote:
> CAUTION: This email comes from a non Wind River email account!
> Do not click links or open attachments unless you recognize the sender 
> and know the content is safe.
>
> Hi, Xiaolei
>
>
> Thanks for your nice catch! I have more to say.
>
> On 2024/8/16 09:55, Wang, Xiaolei wrote:
>> Ping ...
>
> 32 address -> 32-bit address,
>
> Perhaps, we could improve the commit title a little bit
> by writing a more accurate sentence if possible, say:
>
> drm/etnaviv: Properly request pages from DMA32 zone when needed
>
> or
>
> drm/etnaviv: Request pages from DMA32 zone on addressing_limited
>
>
>> thanks
>> xiaolei
>
> Vivante GPU is a 32-bit GPU, it do can access 40-bit physical address 
> via its MMU(IOMMU).
> But this is only possible *after* the MMU has been setup(initialized). 
> Before GPU page
> table is setup(and flush-ed into the GPU's TLB), the device can only 
> access 32-bit
> physical addresses and the addresses has to be physical continues in 
> ranges.
>
> The GPU page tables (GART) and command buffer has to reside in low 4GB 
> address.
>
>> diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gpu.c 
>> b/drivers/gpu/drm/etnaviv/etnaviv_gpu.c
>> index 7c7f97793ddd..0e6bdf2d028b 100644
>> --- a/drivers/gpu/drm/etnaviv/etnaviv_gpu.c
>> +++ b/drivers/gpu/drm/etnaviv/etnaviv_gpu.c
>> @@ -844,8 +844,10 @@ int etnaviv_gpu_init(struct etnaviv_gpu *gpu)
>>            * request pages for our SHM backend buffers from the DMA32 
>> zone to
>>            * hopefully avoid performance killing SWIOTLB bounce 
>> buffering.
>>            */
>> -       if (dma_addressing_limited(gpu->dev))
>> +       if (dma_addressing_limited(gpu->dev)) {
>>                   priv->shm_gfp_mask |= GFP_DMA32;
>> +               priv->shm_gfp_mask &= ~__GFP_HIGHMEM;
>> +       }
>
> The code here  still looks itchy and risky,
> because for a i.MX8 SoC with multiple vivante GPU core.
> We will modify priv->shm_gfp_mask *multiple* time.
>
> For the 2D core and the 3D core have different DMA addressing constraint.
> Then, only the last(latest) modify will be effective. This lead to the
> probe order dependent.
>
> However this may not be a problem in practice, as usually, all vivante
> GPUs in the system will share the same DMA constraints. And the driver
> assume that.
>
> But then, we probably still should not modify the global shared GFP
> mask multiple time.
>
> Now that we do assume that all vivante GPUs in the system share the
> same DMA constraints. And the DMA constraints information has been
> assigned to the virtual master. The right time to modify the
> `priv->shm_gfp_mask` should be in the etnaviv_bind() function. as
> this can eliminate overlap(repeat) stores.
>
>
> Please consider move the entire if() {} to etnaviv_bind(), just below
> where the 'priv->shm_gfp_mask' was initially initialized.
>
> or alternatively we can just hard-code to use low 4GM memmory only:
>
> priv->shm_gfp_mask = GFP_USER | GFP_DMA32 | __GFP_RETRY_MAYFAIL | 
> __GFP_NOWARN;
Thank you very much for your review and suggestion. I will move the 
entire if() {} to etnaviv_bind()

thanks
xiaolei
>
>
> Best regards,
> Sui
>
>>           /* Create buffer: */
>>           ret = etnaviv_cmdbuf_init(priv->cmdbuf_suballoc, &gpu->buffer,