[PATCH 1/2] drm/xe/dma_buf: stop relying on placement in unmap

[PATCH 1/2] drm/xe/dma_buf: stop relying on placement in unmap

[Matthew Auld]

The is_vram() is checking the current placement, however if we consider
exported VRAM with dynamic dma-buf, it looks possible for the xe driver
to async evict the memory, notifying the importer, however importer does
not have to call unmap_attachment() immediately, but rather just as
"soon as possible", like when the dma-resv idles. Following from this we
would then pipeline the move, attaching the fence to the manager, and
then update the current placement. But when the unmap_attachment() runs
at some later point we might see that is_vram() is now false, and take
the complete wrong path when dma-unmapping the sg, leading to
explosions.

To fix this check if the sgl was mapping a struct page.

v2:
  - The attachment can be mapped multiple times it seems, so we can't
    really rely on encoding something in the attachment->priv. Instead
    see if the page_link has an encoded struct page. For vram we expect
    this to be NULL.

Link: https://gitlab.freedesktop.org/drm/xe/kernel/-/issues/4563
Fixes: dd08ebf6c352 ("drm/xe: Introduce a new DRM driver for Intel GPUs")
Signed-off-by: Matthew Auld <matthew.auld@intel.com>
Cc: Thomas Hellström <thomas.hellstrom@linux.intel.com>
Cc: Matthew Brost <matthew.brost@intel.com>
Cc: <stable@vger.kernel.org> # v6.8+
---
 drivers/gpu/drm/xe/xe_dma_buf.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/xe/xe_dma_buf.c b/drivers/gpu/drm/xe/xe_dma_buf.c
index f67803e15a0e..f7a20264ea33 100644
--- a/drivers/gpu/drm/xe/xe_dma_buf.c
+++ b/drivers/gpu/drm/xe/xe_dma_buf.c
@@ -145,10 +145,7 @@ static void xe_dma_buf_unmap(struct dma_buf_attachment *attach,
 			     struct sg_table *sgt,
 			     enum dma_data_direction dir)
 {
-	struct dma_buf *dma_buf = attach->dmabuf;
-	struct xe_bo *bo = gem_to_xe_bo(dma_buf->priv);
-
-	if (!xe_bo_is_vram(bo)) {
+	if (sg_page(sgt->sgl)) {
 		dma_unmap_sgtable(attach->dev, sgt, dir, 0);
 		sg_free_table(sgt);
 		kfree(sgt);
-- 
2.49.0

[PATCH 2/2] drm/amdgpu/dma_buf: fix page_link check

[Matthew Auld]

The page_link lower bits of the first sg could contain something like
SG_END, if we are mapping a single VRAM page or contiguous blob which
fits into one sg entry. Rather pull out the struct page, and use that in
our check to know if we mapped struct pages vs VRAM.

Fixes: f44ffd677fb3 ("drm/amdgpu: add support for exporting VRAM using DMA-buf v3")
Signed-off-by: Matthew Auld <matthew.auld@intel.com>
Cc: Christian König <christian.koenig@amd.com>
Cc: amd-gfx@lists.freedesktop.org
Cc: <stable@vger.kernel.org> # v5.8+
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c
index 9f627caedc3f..c9842a0e2a1c 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c
@@ -184,7 +184,7 @@ static void amdgpu_dma_buf_unmap(struct dma_buf_attachment *attach,
 				 struct sg_table *sgt,
 				 enum dma_data_direction dir)
 {
-	if (sgt->sgl->page_link) {
+	if (sg_page(sgt->sgl)) {
 		dma_unmap_sgtable(attach->dev, sgt, dir, 0);
 		sg_free_table(sgt);
 		kfree(sgt);
-- 
2.49.0

Re: [PATCH 2/2] drm/amdgpu/dma_buf: fix page_link check

[Christian König]

Am 07.04.25 um 16:18 schrieb Matthew Auld:
> The page_link lower bits of the first sg could contain something like
> SG_END, if we are mapping a single VRAM page or contiguous blob which
> fits into one sg entry. Rather pull out the struct page, and use that in
> our check to know if we mapped struct pages vs VRAM.
>
> Fixes: f44ffd677fb3 ("drm/amdgpu: add support for exporting VRAM using DMA-buf v3")
> Signed-off-by: Matthew Auld <matthew.auld@intel.com>
> Cc: Christian König <christian.koenig@amd.com>
> Cc: amd-gfx@lists.freedesktop.org
> Cc: <stable@vger.kernel.org> # v5.8+

Good point, haven't thought about that at all since we only abuse the sg table as DMA addr container.

Reviewed-by: Christian König <christian.koenig@amd.com>

Were is patch #1 from this series?

Thanks,
Christian.

> ---
>  drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c
> index 9f627caedc3f..c9842a0e2a1c 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c
> @@ -184,7 +184,7 @@ static void amdgpu_dma_buf_unmap(struct dma_buf_attachment *attach,
>  				 struct sg_table *sgt,
>  				 enum dma_data_direction dir)
>  {
> -	if (sgt->sgl->page_link) {
> +	if (sg_page(sgt->sgl)) {
>  		dma_unmap_sgtable(attach->dev, sgt, dir, 0);
>  		sg_free_table(sgt);
>  		kfree(sgt);

Re: [PATCH 2/2] drm/amdgpu/dma_buf: fix page_link check

[Matthew Auld]

On 07/04/2025 15:32, Christian König wrote:
> Am 07.04.25 um 16:18 schrieb Matthew Auld:
>> The page_link lower bits of the first sg could contain something like
>> SG_END, if we are mapping a single VRAM page or contiguous blob which
>> fits into one sg entry. Rather pull out the struct page, and use that in
>> our check to know if we mapped struct pages vs VRAM.
>>
>> Fixes: f44ffd677fb3 ("drm/amdgpu: add support for exporting VRAM using DMA-buf v3")
>> Signed-off-by: Matthew Auld <matthew.auld@intel.com>
>> Cc: Christian König <christian.koenig@amd.com>
>> Cc: amd-gfx@lists.freedesktop.org
>> Cc: <stable@vger.kernel.org> # v5.8+
> 
> Good point, haven't thought about that at all since we only abuse the sg table as DMA addr container.
> 
> Reviewed-by: Christian König <christian.koenig@amd.com>
> 
> Were is patch #1 from this series?

That one is xe specific:
https://lore.kernel.org/intel-xe/20250407141823.44504-3-matthew.auld@intel.com/T/#m4ef16e478cfc8853d4518448dd345a66d5a7f6d9

I copied your approach with using page_link here, but with added sg_page().

> 
> Thanks,
> Christian.
> 
>> ---
>>   drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c
>> index 9f627caedc3f..c9842a0e2a1c 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c
>> @@ -184,7 +184,7 @@ static void amdgpu_dma_buf_unmap(struct dma_buf_attachment *attach,
>>   				 struct sg_table *sgt,
>>   				 enum dma_data_direction dir)
>>   {
>> -	if (sgt->sgl->page_link) {
>> +	if (sg_page(sgt->sgl)) {
>>   		dma_unmap_sgtable(attach->dev, sgt, dir, 0);
>>   		sg_free_table(sgt);
>>   		kfree(sgt);
> 

Re: [PATCH 2/2] drm/amdgpu/dma_buf: fix page_link check

[Christian König]

Am 07.04.25 um 16:44 schrieb Matthew Auld:
> On 07/04/2025 15:32, Christian König wrote:
>> Am 07.04.25 um 16:18 schrieb Matthew Auld:
>>> The page_link lower bits of the first sg could contain something like
>>> SG_END, if we are mapping a single VRAM page or contiguous blob which
>>> fits into one sg entry. Rather pull out the struct page, and use that in
>>> our check to know if we mapped struct pages vs VRAM.
>>>
>>> Fixes: f44ffd677fb3 ("drm/amdgpu: add support for exporting VRAM using DMA-buf v3")
>>> Signed-off-by: Matthew Auld <matthew.auld@intel.com>
>>> Cc: Christian König <christian.koenig@amd.com>
>>> Cc: amd-gfx@lists.freedesktop.org
>>> Cc: <stable@vger.kernel.org> # v5.8+
>>
>> Good point, haven't thought about that at all since we only abuse the sg table as DMA addr container.
>>
>> Reviewed-by: Christian König <christian.koenig@amd.com>
>>
>> Were is patch #1 from this series?
>
> That one is xe specific:
> https://lore.kernel.org/intel-xe/20250407141823.44504-3-matthew.auld@intel.com/T/#m4ef16e478cfc8853d4518448dd345a66d5a7f6d9
>
> I copied your approach with using page_link here, but with added sg_page().

Feel free to add my Acked-by to that one as well.

I just wanted to double check if we need to push the patches upstream together, but that looks like we can take each through individual branches.

Thanks,
Christian.

>
>>
>> Thanks,
>> Christian.
>>
>>> ---
>>>   drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c | 2 +-
>>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c
>>> index 9f627caedc3f..c9842a0e2a1c 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c
>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c
>>> @@ -184,7 +184,7 @@ static void amdgpu_dma_buf_unmap(struct dma_buf_attachment *attach,
>>>                    struct sg_table *sgt,
>>>                    enum dma_data_direction dir)
>>>   {
>>> -    if (sgt->sgl->page_link) {
>>> +    if (sg_page(sgt->sgl)) {
>>>           dma_unmap_sgtable(attach->dev, sgt, dir, 0);
>>>           sg_free_table(sgt);
>>>           kfree(sgt);
>>
>

Re: [PATCH 2/2] drm/amdgpu/dma_buf: fix page_link check

[Matthew Auld]

On 07/04/2025 15:46, Christian König wrote:
> Am 07.04.25 um 16:44 schrieb Matthew Auld:
>> On 07/04/2025 15:32, Christian König wrote:
>>> Am 07.04.25 um 16:18 schrieb Matthew Auld:
>>>> The page_link lower bits of the first sg could contain something like
>>>> SG_END, if we are mapping a single VRAM page or contiguous blob which
>>>> fits into one sg entry. Rather pull out the struct page, and use that in
>>>> our check to know if we mapped struct pages vs VRAM.
>>>>
>>>> Fixes: f44ffd677fb3 ("drm/amdgpu: add support for exporting VRAM using DMA-buf v3")
>>>> Signed-off-by: Matthew Auld <matthew.auld@intel.com>
>>>> Cc: Christian König <christian.koenig@amd.com>
>>>> Cc: amd-gfx@lists.freedesktop.org
>>>> Cc: <stable@vger.kernel.org> # v5.8+
>>>
>>> Good point, haven't thought about that at all since we only abuse the sg table as DMA addr container.
>>>
>>> Reviewed-by: Christian König <christian.koenig@amd.com>
>>>
>>> Were is patch #1 from this series?
>>
>> That one is xe specific:
>> https://lore.kernel.org/intel-xe/20250407141823.44504-3-matthew.auld@intel.com/T/#m4ef16e478cfc8853d4518448dd345a66d5a7f6d9
>>
>> I copied your approach with using page_link here, but with added sg_page().
> 
> Feel free to add my Acked-by to that one as well.

Thanks.

> 
> I just wanted to double check if we need to push the patches upstream together, but that looks like we can take each through individual branches.

Sounds good.

> 
> Thanks,
> Christian.
> 
>>
>>>
>>> Thanks,
>>> Christian.
>>>
>>>> ---
>>>>    drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c | 2 +-
>>>>    1 file changed, 1 insertion(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c
>>>> index 9f627caedc3f..c9842a0e2a1c 100644
>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c
>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c
>>>> @@ -184,7 +184,7 @@ static void amdgpu_dma_buf_unmap(struct dma_buf_attachment *attach,
>>>>                     struct sg_table *sgt,
>>>>                     enum dma_data_direction dir)
>>>>    {
>>>> -    if (sgt->sgl->page_link) {
>>>> +    if (sg_page(sgt->sgl)) {
>>>>            dma_unmap_sgtable(attach->dev, sgt, dir, 0);
>>>>            sg_free_table(sgt);
>>>>            kfree(sgt);
>>>
>>
> 

Re: [PATCH 2/2] drm/amdgpu/dma_buf: fix page_link check

[Alex Deucher]

Applied.  Thanks!

On Mon, Apr 7, 2025 at 10:42 AM Christian König
<ckoenig.leichtzumerken@gmail.com> wrote:
>
> Am 07.04.25 um 16:18 schrieb Matthew Auld:
> > The page_link lower bits of the first sg could contain something like
> > SG_END, if we are mapping a single VRAM page or contiguous blob which
> > fits into one sg entry. Rather pull out the struct page, and use that in
> > our check to know if we mapped struct pages vs VRAM.
> >
> > Fixes: f44ffd677fb3 ("drm/amdgpu: add support for exporting VRAM using DMA-buf v3")
> > Signed-off-by: Matthew Auld <matthew.auld@intel.com>
> > Cc: Christian König <christian.koenig@amd.com>
> > Cc: amd-gfx@lists.freedesktop.org
> > Cc: <stable@vger.kernel.org> # v5.8+
>
> Good point, haven't thought about that at all since we only abuse the sg table as DMA addr container.
>
> Reviewed-by: Christian König <christian.koenig@amd.com>
>
> Were is patch #1 from this series?
>
> Thanks,
> Christian.
>
> > ---
> >  drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c
> > index 9f627caedc3f..c9842a0e2a1c 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c
> > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c
> > @@ -184,7 +184,7 @@ static void amdgpu_dma_buf_unmap(struct dma_buf_attachment *attach,
> >                                struct sg_table *sgt,
> >                                enum dma_data_direction dir)
> >  {
> > -     if (sgt->sgl->page_link) {
> > +     if (sg_page(sgt->sgl)) {
> >               dma_unmap_sgtable(attach->dev, sgt, dir, 0);
> >               sg_free_table(sgt);
> >               kfree(sgt);
>