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

[Sui Jingfeng <sui.jingfeng@linux.dev>]

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);

-- 
Best regards,
Sui