diff mbox series

[v3] drm/etnaviv: Request pages from DMA32 zone on addressing_limited

Message ID 20240903020857.3250038-1-xiaolei.wang@windriver.com (mailing list archive)
State New, archived
Headers show
Series [v3] drm/etnaviv: Request pages from DMA32 zone on addressing_limited | expand

Commit Message

Xiaolei Wang Sept. 3, 2024, 2:08 a.m. UTC
Remove __GFP_HIGHMEM when requesting a page from DMA32 zone,
and since all vivante GPUs in the system will share the same
DMA constraints, move the check of whether to get a page from
DMA32 to etnaviv_bind().

Fixes: b72af445cd38 ("drm/etnaviv: request pages from DMA32 zone when needed")
Suggested-by: Sui Jingfeng <sui.jingfeng@linux.dev>
Signed-off-by: Xiaolei Wang <xiaolei.wang@windriver.com>
---

change log

v1:
  https://patchwork.kernel.org/project/dri-devel/patch/20240806104733.2018783-1-xiaolei.wang@windriver.com/

v2:
  Modify the issue of not retaining GFP_USER in v1 and update the commit log.

v3:
  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;"
and move the check of whether to get a page from DMA32 to etnaviv_bind().

 drivers/gpu/drm/etnaviv/etnaviv_drv.c | 10 +++++++++-
 drivers/gpu/drm/etnaviv/etnaviv_gpu.c |  8 --------
 2 files changed, 9 insertions(+), 9 deletions(-)

Comments

Sui Jingfeng Sept. 11, 2024, 9:37 a.m. UTC | #1
Humble ping ?

On 9/3/24 10:08, Xiaolei Wang wrote:
> Remove __GFP_HIGHMEM when requesting a page from DMA32 zone,
> and since all vivante GPUs in the system will share the same
> DMA constraints, move the check of whether to get a page from
> DMA32 to etnaviv_bind().
> 
> Fixes: b72af445cd38 ("drm/etnaviv: request pages from DMA32 zone when needed")
> Suggested-by: Sui Jingfeng <sui.jingfeng@linux.dev>
> Signed-off-by: Xiaolei Wang <xiaolei.wang@windriver.com>
> ---
> 
> change log
> 
> v1:
>    https://patchwork.kernel.org/project/dri-devel/patch/20240806104733.2018783-1-xiaolei.wang@windriver.com/
> 
> v2:
>    Modify the issue of not retaining GFP_USER in v1 and update the commit log.
> 
> v3:
>    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;"
> and move the check of whether to get a page from DMA32 to etnaviv_bind().
> 
>   drivers/gpu/drm/etnaviv/etnaviv_drv.c | 10 +++++++++-
>   drivers/gpu/drm/etnaviv/etnaviv_gpu.c |  8 --------
>   2 files changed, 9 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/gpu/drm/etnaviv/etnaviv_drv.c b/drivers/gpu/drm/etnaviv/etnaviv_drv.c
> index 6500f3999c5f..8cb2c3ec8e5d 100644
> --- a/drivers/gpu/drm/etnaviv/etnaviv_drv.c
> +++ b/drivers/gpu/drm/etnaviv/etnaviv_drv.c
> @@ -536,7 +536,15 @@ static int etnaviv_bind(struct device *dev)
>   	mutex_init(&priv->gem_lock);
>   	INIT_LIST_HEAD(&priv->gem_list);
>   	priv->num_gpus = 0;
> -	priv->shm_gfp_mask = GFP_HIGHUSER | __GFP_RETRY_MAYFAIL | __GFP_NOWARN;
> +	priv->shm_gfp_mask = GFP_USER | __GFP_RETRY_MAYFAIL | __GFP_NOWARN;
> +
> +	/*
> +	 * If the GPU is part of a system with DMA addressing limitations,
> +	 * request pages for our SHM backend buffers from the DMA32 zone to
> +	 * hopefully avoid performance killing SWIOTLB bounce buffering.
> +	 */
> +	if (dma_addressing_limited(dev))
> +		priv->shm_gfp_mask |= GFP_DMA32;
>   
>   	priv->cmdbuf_suballoc = etnaviv_cmdbuf_suballoc_new(drm->dev);
>   	if (IS_ERR(priv->cmdbuf_suballoc)) {
> diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gpu.c b/drivers/gpu/drm/etnaviv/etnaviv_gpu.c
> index 7c7f97793ddd..5e753dd42f72 100644
> --- a/drivers/gpu/drm/etnaviv/etnaviv_gpu.c
> +++ b/drivers/gpu/drm/etnaviv/etnaviv_gpu.c
> @@ -839,14 +839,6 @@ int etnaviv_gpu_init(struct etnaviv_gpu *gpu)
>   	if (ret)
>   		goto fail;
>   
> -	/*
> -	 * If the GPU is part of a system with DMA addressing limitations,
> -	 * 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))
> -		priv->shm_gfp_mask |= GFP_DMA32;
> -
>   	/* Create buffer: */
>   	ret = etnaviv_cmdbuf_init(priv->cmdbuf_suballoc, &gpu->buffer,
>   				  PAGE_SIZE);
Christian Gmeiner Sept. 11, 2024, 5:35 p.m. UTC | #2
Am Di., 3. Sept. 2024 um 04:09 Uhr schrieb Xiaolei Wang
<xiaolei.wang@windriver.com>:
>
> Remove __GFP_HIGHMEM when requesting a page from DMA32 zone,
> and since all vivante GPUs in the system will share the same
> DMA constraints, move the check of whether to get a page from
> DMA32 to etnaviv_bind().
>
> Fixes: b72af445cd38 ("drm/etnaviv: request pages from DMA32 zone when needed")
> Suggested-by: Sui Jingfeng <sui.jingfeng@linux.dev>
> Signed-off-by: Xiaolei Wang <xiaolei.wang@windriver.com>

Reviewed-by: Christian Gmeiner <cgmeiner@igalia.com>

> ---
>
> change log
>
> v1:
>   https://patchwork.kernel.org/project/dri-devel/patch/20240806104733.2018783-1-xiaolei.wang@windriver.com/
>
> v2:
>   Modify the issue of not retaining GFP_USER in v1 and update the commit log.
>
> v3:
>   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;"
> and move the check of whether to get a page from DMA32 to etnaviv_bind().
>
>  drivers/gpu/drm/etnaviv/etnaviv_drv.c | 10 +++++++++-
>  drivers/gpu/drm/etnaviv/etnaviv_gpu.c |  8 --------
>  2 files changed, 9 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/gpu/drm/etnaviv/etnaviv_drv.c b/drivers/gpu/drm/etnaviv/etnaviv_drv.c
> index 6500f3999c5f..8cb2c3ec8e5d 100644
> --- a/drivers/gpu/drm/etnaviv/etnaviv_drv.c
> +++ b/drivers/gpu/drm/etnaviv/etnaviv_drv.c
> @@ -536,7 +536,15 @@ static int etnaviv_bind(struct device *dev)
>         mutex_init(&priv->gem_lock);
>         INIT_LIST_HEAD(&priv->gem_list);
>         priv->num_gpus = 0;
> -       priv->shm_gfp_mask = GFP_HIGHUSER | __GFP_RETRY_MAYFAIL | __GFP_NOWARN;
> +       priv->shm_gfp_mask = GFP_USER | __GFP_RETRY_MAYFAIL | __GFP_NOWARN;
> +
> +       /*
> +        * If the GPU is part of a system with DMA addressing limitations,
> +        * request pages for our SHM backend buffers from the DMA32 zone to
> +        * hopefully avoid performance killing SWIOTLB bounce buffering.
> +        */
> +       if (dma_addressing_limited(dev))
> +               priv->shm_gfp_mask |= GFP_DMA32;
>
>         priv->cmdbuf_suballoc = etnaviv_cmdbuf_suballoc_new(drm->dev);
>         if (IS_ERR(priv->cmdbuf_suballoc)) {
> diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gpu.c b/drivers/gpu/drm/etnaviv/etnaviv_gpu.c
> index 7c7f97793ddd..5e753dd42f72 100644
> --- a/drivers/gpu/drm/etnaviv/etnaviv_gpu.c
> +++ b/drivers/gpu/drm/etnaviv/etnaviv_gpu.c
> @@ -839,14 +839,6 @@ int etnaviv_gpu_init(struct etnaviv_gpu *gpu)
>         if (ret)
>                 goto fail;
>
> -       /*
> -        * If the GPU is part of a system with DMA addressing limitations,
> -        * 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))
> -               priv->shm_gfp_mask |= GFP_DMA32;
> -
>         /* Create buffer: */
>         ret = etnaviv_cmdbuf_init(priv->cmdbuf_suballoc, &gpu->buffer,
>                                   PAGE_SIZE);
> --
> 2.25.1
>
Lucas Stach Oct. 1, 2024, 12:17 p.m. UTC | #3
Hi Xiaolei,

Am Dienstag, dem 03.09.2024 um 10:08 +0800 schrieb Xiaolei Wang:
> Remove __GFP_HIGHMEM when requesting a page from DMA32 zone,
> and since all vivante GPUs in the system will share the same
> DMA constraints, move the check of whether to get a page from
> DMA32 to etnaviv_bind().
> 
> Fixes: b72af445cd38 ("drm/etnaviv: request pages from DMA32 zone when needed")
> Suggested-by: Sui Jingfeng <sui.jingfeng@linux.dev>
> Signed-off-by: Xiaolei Wang <xiaolei.wang@windriver.com>
> ---
> 
> change log
> 
> v1:
>   https://patchwork.kernel.org/project/dri-devel/patch/20240806104733.2018783-1-xiaolei.wang@windriver.com/
> 
> v2:
>   Modify the issue of not retaining GFP_USER in v1 and update the commit log.
> 
> v3:
>   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;"

I don't understand this part of the changes in the new version. Why
should we drop the HIGHMEM bit always and not only in the case where
dma addressing is limited? This seems overly restrictive.

Regards,
Lucas

> and move the check of whether to get a page from DMA32 to etnaviv_bind().
> 
>  drivers/gpu/drm/etnaviv/etnaviv_drv.c | 10 +++++++++-
>  drivers/gpu/drm/etnaviv/etnaviv_gpu.c |  8 --------
>  2 files changed, 9 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/gpu/drm/etnaviv/etnaviv_drv.c b/drivers/gpu/drm/etnaviv/etnaviv_drv.c
> index 6500f3999c5f..8cb2c3ec8e5d 100644
> --- a/drivers/gpu/drm/etnaviv/etnaviv_drv.c
> +++ b/drivers/gpu/drm/etnaviv/etnaviv_drv.c
> @@ -536,7 +536,15 @@ static int etnaviv_bind(struct device *dev)
>  	mutex_init(&priv->gem_lock);
>  	INIT_LIST_HEAD(&priv->gem_list);
>  	priv->num_gpus = 0;
> -	priv->shm_gfp_mask = GFP_HIGHUSER | __GFP_RETRY_MAYFAIL | __GFP_NOWARN;
> +	priv->shm_gfp_mask = GFP_USER | __GFP_RETRY_MAYFAIL | __GFP_NOWARN;
> +
> +	/*
> +	 * If the GPU is part of a system with DMA addressing limitations,
> +	 * request pages for our SHM backend buffers from the DMA32 zone to
> +	 * hopefully avoid performance killing SWIOTLB bounce buffering.
> +	 */
> +	if (dma_addressing_limited(dev))
> +		priv->shm_gfp_mask |= GFP_DMA32;
>  
>  	priv->cmdbuf_suballoc = etnaviv_cmdbuf_suballoc_new(drm->dev);
>  	if (IS_ERR(priv->cmdbuf_suballoc)) {
> diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gpu.c b/drivers/gpu/drm/etnaviv/etnaviv_gpu.c
> index 7c7f97793ddd..5e753dd42f72 100644
> --- a/drivers/gpu/drm/etnaviv/etnaviv_gpu.c
> +++ b/drivers/gpu/drm/etnaviv/etnaviv_gpu.c
> @@ -839,14 +839,6 @@ int etnaviv_gpu_init(struct etnaviv_gpu *gpu)
>  	if (ret)
>  		goto fail;
>  
> -	/*
> -	 * If the GPU is part of a system with DMA addressing limitations,
> -	 * 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))
> -		priv->shm_gfp_mask |= GFP_DMA32;
> -
>  	/* Create buffer: */
>  	ret = etnaviv_cmdbuf_init(priv->cmdbuf_suballoc, &gpu->buffer,
>  				  PAGE_SIZE);
Xiaolei Wang Oct. 1, 2024, 12:32 p.m. UTC | #4
On 10/1/24 20:17, Lucas Stach 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,
>
> Am Dienstag, dem 03.09.2024 um 10:08 +0800 schrieb Xiaolei Wang:
>> Remove __GFP_HIGHMEM when requesting a page from DMA32 zone,
>> and since all vivante GPUs in the system will share the same
>> DMA constraints, move the check of whether to get a page from
>> DMA32 to etnaviv_bind().
>>
>> Fixes: b72af445cd38 ("drm/etnaviv: request pages from DMA32 zone when needed")
>> Suggested-by: Sui Jingfeng<sui.jingfeng@linux.dev>
>> Signed-off-by: Xiaolei Wang<xiaolei.wang@windriver.com>
>> ---
>>
>> change log
>>
>> v1:
>>    https://patchwork.kernel.org/project/dri-devel/patch/20240806104733.2018783-1-xiaolei.wang@windriver.com/
>>
>> v2:
>>    Modify the issue of not retaining GFP_USER in v1 and update the commit log.
>>
>> v3:
>>    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;"
> I don't understand this part of the changes in the new version. Why
> should we drop the HIGHMEM bit always and not only in the case where
> dma addressing is limited? This seems overly restrictive.

Makes sense, thanks for your reminder, I will drop the HIGHMEM bit when 
the next version has address limit

     if (dma_addressing_limited(gpu->dev)) {
         priv->shm_gfp_mask |= GFP_DMA32;
         priv->shm_gfp_mask &= ~__GFP_HIGHMEM;
     }

thanks

xiaolei

>
> Regards,
> Lucas
>
>> and move the check of whether to get a page from DMA32 to etnaviv_bind().
>>
>>   drivers/gpu/drm/etnaviv/etnaviv_drv.c | 10 +++++++++-
>>   drivers/gpu/drm/etnaviv/etnaviv_gpu.c |  8 --------
>>   2 files changed, 9 insertions(+), 9 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/etnaviv/etnaviv_drv.c b/drivers/gpu/drm/etnaviv/etnaviv_drv.c
>> index 6500f3999c5f..8cb2c3ec8e5d 100644
>> --- a/drivers/gpu/drm/etnaviv/etnaviv_drv.c
>> +++ b/drivers/gpu/drm/etnaviv/etnaviv_drv.c
>> @@ -536,7 +536,15 @@ static int etnaviv_bind(struct device *dev)
>>        mutex_init(&priv->gem_lock);
>>        INIT_LIST_HEAD(&priv->gem_list);
>>        priv->num_gpus = 0;
>> -     priv->shm_gfp_mask = GFP_HIGHUSER | __GFP_RETRY_MAYFAIL | __GFP_NOWARN;
>> +     priv->shm_gfp_mask = GFP_USER | __GFP_RETRY_MAYFAIL | __GFP_NOWARN;
>> +
>> +     /*
>> +      * If the GPU is part of a system with DMA addressing limitations,
>> +      * request pages for our SHM backend buffers from the DMA32 zone to
>> +      * hopefully avoid performance killing SWIOTLB bounce buffering.
>> +      */
>> +     if (dma_addressing_limited(dev))
>> +             priv->shm_gfp_mask |= GFP_DMA32;
>>
>>        priv->cmdbuf_suballoc = etnaviv_cmdbuf_suballoc_new(drm->dev);
>>        if (IS_ERR(priv->cmdbuf_suballoc)) {
>> diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gpu.c b/drivers/gpu/drm/etnaviv/etnaviv_gpu.c
>> index 7c7f97793ddd..5e753dd42f72 100644
>> --- a/drivers/gpu/drm/etnaviv/etnaviv_gpu.c
>> +++ b/drivers/gpu/drm/etnaviv/etnaviv_gpu.c
>> @@ -839,14 +839,6 @@ int etnaviv_gpu_init(struct etnaviv_gpu *gpu)
>>        if (ret)
>>                goto fail;
>>
>> -     /*
>> -      * If the GPU is part of a system with DMA addressing limitations,
>> -      * 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))
>> -             priv->shm_gfp_mask |= GFP_DMA32;
>> -
>>        /* Create buffer: */
>>        ret = etnaviv_cmdbuf_init(priv->cmdbuf_suballoc, &gpu->buffer,
>>                                  PAGE_SIZE);
Sui Jingfeng Nov. 2, 2024, 3:24 p.m. UTC | #5
Hi,

I forget to mention that  the commit title should be "on addressing limited devices",
the underscore between the last two words is not necessary. It's a typo when reply
email.


On 2024/10/1 20:32, xiaolei wang wrote:
>
>
> On 10/1/24 20:17, Lucas Stach 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,
>>
>> Am Dienstag, dem 03.09.2024 um 10:08 +0800 schrieb Xiaolei Wang:
>>> Remove __GFP_HIGHMEM when requesting a page from DMA32 zone,
>>> and since all vivante GPUs in the system will share the same
>>> DMA constraints, move the check of whether to get a page from
>>> DMA32 to etnaviv_bind().
>>>
>>> Fixes: b72af445cd38 ("drm/etnaviv: request pages from DMA32 zone when needed")
>>> Suggested-by: Sui Jingfeng<sui.jingfeng@linux.dev>
>>> Signed-off-by: Xiaolei Wang<xiaolei.wang@windriver.com>
>>> ---
>>>
>>> change log
>>>
>>> v1:
>>>    https://patchwork.kernel.org/project/dri-devel/patch/20240806104733.2018783-1-xiaolei.wang@windriver.com/
>>>
>>> v2:
>>>    Modify the issue of not retaining GFP_USER in v1 and update the commit log.
>>>
>>> v3:
>>>    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;"
>> I don't understand this part of the changes in the new version. Why
>> should we drop the HIGHMEM bit always and not only in the case where
>> dma addressing is limited? This seems overly restrictive.
>
> Makes sense, thanks for your reminder, I will drop the HIGHMEM bit 
> when the next version has address limit
>
>     if (dma_addressing_limited(gpu->dev)) {
>         priv->shm_gfp_mask |= GFP_DMA32;
>         priv->shm_gfp_mask &= ~__GFP_HIGHMEM;
>     }
>
> thanks
>
> xiaolei
>
>> Regards,
>> Lucas
>>
>>> and move the check of whether to get a page from DMA32 to etnaviv_bind().
>>>
>>>   drivers/gpu/drm/etnaviv/etnaviv_drv.c | 10 +++++++++-
>>>   drivers/gpu/drm/etnaviv/etnaviv_gpu.c |  8 --------
>>>   2 files changed, 9 insertions(+), 9 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/etnaviv/etnaviv_drv.c b/drivers/gpu/drm/etnaviv/etnaviv_drv.c
>>> index 6500f3999c5f..8cb2c3ec8e5d 100644
>>> --- a/drivers/gpu/drm/etnaviv/etnaviv_drv.c
>>> +++ b/drivers/gpu/drm/etnaviv/etnaviv_drv.c
>>> @@ -536,7 +536,15 @@ static int etnaviv_bind(struct device *dev)
>>>        mutex_init(&priv->gem_lock);
>>>        INIT_LIST_HEAD(&priv->gem_list);
>>>        priv->num_gpus = 0;
>>> -     priv->shm_gfp_mask = GFP_HIGHUSER | __GFP_RETRY_MAYFAIL | __GFP_NOWARN;
>>> +     priv->shm_gfp_mask = GFP_USER | __GFP_RETRY_MAYFAIL | __GFP_NOWARN;
>>> +
>>> +     /*
>>> +      * If the GPU is part of a system with DMA addressing limitations,
>>> +      * request pages for our SHM backend buffers from the DMA32 zone to
>>> +      * hopefully avoid performance killing SWIOTLB bounce buffering.
>>> +      */
>>> +     if (dma_addressing_limited(dev))
>>> +             priv->shm_gfp_mask |= GFP_DMA32;
>>>
>>>        priv->cmdbuf_suballoc = etnaviv_cmdbuf_suballoc_new(drm->dev);
>>>        if (IS_ERR(priv->cmdbuf_suballoc)) {
>>> diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gpu.c b/drivers/gpu/drm/etnaviv/etnaviv_gpu.c
>>> index 7c7f97793ddd..5e753dd42f72 100644
>>> --- a/drivers/gpu/drm/etnaviv/etnaviv_gpu.c
>>> +++ b/drivers/gpu/drm/etnaviv/etnaviv_gpu.c
>>> @@ -839,14 +839,6 @@ int etnaviv_gpu_init(struct etnaviv_gpu *gpu)
>>>        if (ret)
>>>                goto fail;
>>>
>>> -     /*
>>> -      * If the GPU is part of a system with DMA addressing limitations,
>>> -      * 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))
>>> -             priv->shm_gfp_mask |= GFP_DMA32;
>>> -
>>>        /* Create buffer: */
>>>        ret = etnaviv_cmdbuf_init(priv->cmdbuf_suballoc, &gpu->buffer,
>>>                                  PAGE_SIZE);
Sui Jingfeng Nov. 2, 2024, 3:31 p.m. UTC | #6
Hi,


On 2024/10/1 20:17, Lucas Stach wrote:
> Hi Xiaolei,
>
> Am Dienstag, dem 03.09.2024 um 10:08 +0800 schrieb Xiaolei Wang:
>> Remove __GFP_HIGHMEM when requesting a page from DMA32 zone,
>> and since all vivante GPUs in the system will share the same
>> DMA constraints, move the check of whether to get a page from
>> DMA32 to etnaviv_bind().
>>
>> Fixes: b72af445cd38 ("drm/etnaviv: request pages from DMA32 zone when needed")
>> Suggested-by: Sui Jingfeng <sui.jingfeng@linux.dev>
>> Signed-off-by: Xiaolei Wang <xiaolei.wang@windriver.com>
>> ---
>>
>> change log
>>
>> v1:
>>    https://patchwork.kernel.org/project/dri-devel/patch/20240806104733.2018783-1-xiaolei.wang@windriver.com/
>>
>> v2:
>>    Modify the issue of not retaining GFP_USER in v1 and update the commit log.
>>
>> v3:
>>    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;"
> I don't understand this part of the changes in the new version. Why
> should we drop the HIGHMEM bit always and not only in the case where
> dma addressing is limited? This seems overly restrictive.

While reading the implementation of the dma_alloc_attrs() function,
except allocate memory from device coherent pool, the rest implementation
just mask out __GFP_DMA, __GFP_DMA32 and __GFP_HIGHMEM anyway.

So ?


```
void *dma_alloc_attrs(struct device *dev, size_t size, dma_addr_t *dma_handle,
		gfp_t flag, unsigned long attrs)
{
	const struct dma_map_ops *ops = get_dma_ops(dev);
	void *cpu_addr;

	WARN_ON_ONCE(!dev->coherent_dma_mask);

	/*
	 * DMA allocations can never be turned back into a page pointer, so
	 * requesting compound pages doesn't make sense (and can't even be
	 * supported at all by various backends).
	 */
	if (WARN_ON_ONCE(flag & __GFP_COMP))
		return NULL;

	if (dma_alloc_from_dev_coherent(dev, size, dma_handle, &cpu_addr))
		return cpu_addr;

	/* let the implementation decide on the zone to allocate from: */
	flag &= ~(__GFP_DMA | __GFP_DMA32 | __GFP_HIGHMEM);

	if (dma_alloc_direct(dev, ops))
		cpu_addr = dma_direct_alloc(dev, size, dma_handle, flag, attrs);
	else if (use_dma_iommu(dev))
		cpu_addr = iommu_dma_alloc(dev, size, dma_handle, flag, attrs);
	else if (ops->alloc)
		cpu_addr = ops->alloc(dev, size, dma_handle, flag, attrs);
	else
		return NULL;


	return cpu_addr;
}
```


> Regards,
> Lucas
>
>> and move the check of whether to get a page from DMA32 to etnaviv_bind().
>>
>>   drivers/gpu/drm/etnaviv/etnaviv_drv.c | 10 +++++++++-
>>   drivers/gpu/drm/etnaviv/etnaviv_gpu.c |  8 --------
>>   2 files changed, 9 insertions(+), 9 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/etnaviv/etnaviv_drv.c b/drivers/gpu/drm/etnaviv/etnaviv_drv.c
>> index 6500f3999c5f..8cb2c3ec8e5d 100644
>> --- a/drivers/gpu/drm/etnaviv/etnaviv_drv.c
>> +++ b/drivers/gpu/drm/etnaviv/etnaviv_drv.c
>> @@ -536,7 +536,15 @@ static int etnaviv_bind(struct device *dev)
>>   	mutex_init(&priv->gem_lock);
>>   	INIT_LIST_HEAD(&priv->gem_list);
>>   	priv->num_gpus = 0;
>> -	priv->shm_gfp_mask = GFP_HIGHUSER | __GFP_RETRY_MAYFAIL | __GFP_NOWARN;
>> +	priv->shm_gfp_mask = GFP_USER | __GFP_RETRY_MAYFAIL | __GFP_NOWARN;
>> +
>> +	/*
>> +	 * If the GPU is part of a system with DMA addressing limitations,
>> +	 * request pages for our SHM backend buffers from the DMA32 zone to
>> +	 * hopefully avoid performance killing SWIOTLB bounce buffering.
>> +	 */
>> +	if (dma_addressing_limited(dev))
>> +		priv->shm_gfp_mask |= GFP_DMA32;
>>   
>>   	priv->cmdbuf_suballoc = etnaviv_cmdbuf_suballoc_new(drm->dev);
>>   	if (IS_ERR(priv->cmdbuf_suballoc)) {
>> diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gpu.c b/drivers/gpu/drm/etnaviv/etnaviv_gpu.c
>> index 7c7f97793ddd..5e753dd42f72 100644
>> --- a/drivers/gpu/drm/etnaviv/etnaviv_gpu.c
>> +++ b/drivers/gpu/drm/etnaviv/etnaviv_gpu.c
>> @@ -839,14 +839,6 @@ int etnaviv_gpu_init(struct etnaviv_gpu *gpu)
>>   	if (ret)
>>   		goto fail;
>>   
>> -	/*
>> -	 * If the GPU is part of a system with DMA addressing limitations,
>> -	 * 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))
>> -		priv->shm_gfp_mask |= GFP_DMA32;
>> -
>>   	/* Create buffer: */
>>   	ret = etnaviv_cmdbuf_init(priv->cmdbuf_suballoc, &gpu->buffer,
>>   				  PAGE_SIZE);
diff mbox series

Patch

diff --git a/drivers/gpu/drm/etnaviv/etnaviv_drv.c b/drivers/gpu/drm/etnaviv/etnaviv_drv.c
index 6500f3999c5f..8cb2c3ec8e5d 100644
--- a/drivers/gpu/drm/etnaviv/etnaviv_drv.c
+++ b/drivers/gpu/drm/etnaviv/etnaviv_drv.c
@@ -536,7 +536,15 @@  static int etnaviv_bind(struct device *dev)
 	mutex_init(&priv->gem_lock);
 	INIT_LIST_HEAD(&priv->gem_list);
 	priv->num_gpus = 0;
-	priv->shm_gfp_mask = GFP_HIGHUSER | __GFP_RETRY_MAYFAIL | __GFP_NOWARN;
+	priv->shm_gfp_mask = GFP_USER | __GFP_RETRY_MAYFAIL | __GFP_NOWARN;
+
+	/*
+	 * If the GPU is part of a system with DMA addressing limitations,
+	 * request pages for our SHM backend buffers from the DMA32 zone to
+	 * hopefully avoid performance killing SWIOTLB bounce buffering.
+	 */
+	if (dma_addressing_limited(dev))
+		priv->shm_gfp_mask |= GFP_DMA32;
 
 	priv->cmdbuf_suballoc = etnaviv_cmdbuf_suballoc_new(drm->dev);
 	if (IS_ERR(priv->cmdbuf_suballoc)) {
diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gpu.c b/drivers/gpu/drm/etnaviv/etnaviv_gpu.c
index 7c7f97793ddd..5e753dd42f72 100644
--- a/drivers/gpu/drm/etnaviv/etnaviv_gpu.c
+++ b/drivers/gpu/drm/etnaviv/etnaviv_gpu.c
@@ -839,14 +839,6 @@  int etnaviv_gpu_init(struct etnaviv_gpu *gpu)
 	if (ret)
 		goto fail;
 
-	/*
-	 * If the GPU is part of a system with DMA addressing limitations,
-	 * 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))
-		priv->shm_gfp_mask |= GFP_DMA32;
-
 	/* Create buffer: */
 	ret = etnaviv_cmdbuf_init(priv->cmdbuf_suballoc, &gpu->buffer,
 				  PAGE_SIZE);