[PATCH] drm/amdgpu: exclude domain start when calucales offset for AGP aperture BOs

Christian König christian.koenig at amd.com
Fri Nov 10 14:30:44 UTC 2023 

Just call amdgpu_gmc_agp_addr() and check the return value for != 
AMDGPU_BO_INVALID_OFFSET;

The problem is simply that we can't cache that result anywhere because 
bo->resource->start is essentially the offset into the GART and not the 
MC address.

That must have been sneaked in years ago when we removed the MC address 
in the TTM BO.

Christian.

Am 10.11.23 um 15:27 schrieb Deucher, Alexander:
>
> [Public]
>
>
> In that case, how do we know we can skip the gart setup in 
> amdgpu_ttm_alloc_gart()?
>
> Alex
> ------------------------------------------------------------------------
> *From:* Koenig, Christian <Christian.Koenig at amd.com>
> *Sent:* Friday, November 10, 2023 9:20 AM
> *To:* Deucher, Alexander <Alexander.Deucher at amd.com>; Zhang, Yifan 
> <Yifan1.Zhang at amd.com>; amd-gfx at lists.freedesktop.org 
> <amd-gfx at lists.freedesktop.org>
> *Cc:* Zhang, Jesse(Jie) <Jesse.Zhang at amd.com>
> *Subject:* Re: [PATCH] drm/amdgpu: exclude domain start when calucales 
> offset for AGP aperture BOs
> No, that's broken as well.
>
> The problem is in amdgpu_ttm_alloc_gart():
>
>         if (addr != AMDGPU_BO_INVALID_OFFSET) {
>                 bo->resource->start = addr >> PAGE_SHIFT;
>                 return 0;
>         }
>
> bo->resource->start is relative to the GART address, so we can't 
> assign the AGP address here in the first place.
>
> What we need to do is to drop this and call amdgpu_gmc_agp_addr() 
> fromamdgpu_bo_gpu_offset_no_check().
>
> Regards,
> Christian.
>
> Am 10.11.23 um 15:17 schrieb Deucher, Alexander:
>>
>> [Public]
>>
>>
>> I think the proper fix is probably to just drop the addition of 
>> agp_start in amdgpu_gmc_agp_addr().
>>
>> Alex
>> ------------------------------------------------------------------------
>> *From:* Deucher, Alexander <Alexander.Deucher at amd.com> 
>> <mailto:Alexander.Deucher at amd.com>
>> *Sent:* Friday, November 10, 2023 9:16 AM
>> *To:* Koenig, Christian <Christian.Koenig at amd.com> 
>> <mailto:Christian.Koenig at amd.com>; Zhang, Yifan 
>> <Yifan1.Zhang at amd.com> <mailto:Yifan1.Zhang at amd.com>; 
>> amd-gfx at lists.freedesktop.org <mailto:amd-gfx at lists.freedesktop.org> 
>> <amd-gfx at lists.freedesktop.org> <mailto:amd-gfx at lists.freedesktop.org>
>> *Cc:* Zhang, Jesse(Jie) <Jesse.Zhang at amd.com> 
>> <mailto:Jesse.Zhang at amd.com>
>> *Subject:* Re: [PATCH] drm/amdgpu: exclude domain start when 
>> calucales offset for AGP aperture BOs
>> It happens in amdgpu_gmc_agp_addr() which is called from 
>> amdgpu_ttm_alloc_gart().
>>
>> Alex
>> ------------------------------------------------------------------------
>> *From:* Koenig, Christian <Christian.Koenig at amd.com> 
>> <mailto:Christian.Koenig at amd.com>
>> *Sent:* Friday, November 10, 2023 9:14 AM
>> *To:* Zhang, Yifan <Yifan1.Zhang at amd.com> 
>> <mailto:Yifan1.Zhang at amd.com>; amd-gfx at lists.freedesktop.org 
>> <mailto:amd-gfx at lists.freedesktop.org> 
>> <amd-gfx at lists.freedesktop.org> <mailto:amd-gfx at lists.freedesktop.org>
>> *Cc:* Deucher, Alexander <Alexander.Deucher at amd.com> 
>> <mailto:Alexander.Deucher at amd.com>; Zhang, Jesse(Jie) 
>> <Jesse.Zhang at amd.com> <mailto:Jesse.Zhang at amd.com>
>> *Subject:* Re: [PATCH] drm/amdgpu: exclude domain start when 
>> calucales offset for AGP aperture BOs
>> Am 10.11.23 um 13:52 schrieb Yifan Zhang:
>> > For BOs in AGP aperture, tbo.resource->start includes AGP aperture 
>> start.
>>
>>
>> Well big NAK to that. tbo.resource->start should never ever include the
>> AGP aperture start in the first place.
>>
>> How did that happen?
>>
>> Regards,
>> Christian.
>>
>> > Don't add it again in amdgpu_bo_gpu_offset. This issue was 
>> mitigated due to
>> > GART aperture start was 0 until this patch ("a013c94d5aca 
>> drm/amdgpu/gmc11:
>> > set gart placement GC11") changes GART start to a non-zero value.
>> >
>> > Reported-by: Jesse Zhang <Jesse.Zhang at amd.com> 
>> <mailto:Jesse.Zhang at amd.com>
>> > Signed-off-by: Yifan Zhang <yifan1.zhang at amd.com> 
>> <mailto:yifan1.zhang at amd.com>
>> > ---
>> > drivers/gpu/drm/amd/amdgpu/amdgpu_gmc.c    |  7 +++++++
>> > drivers/gpu/drm/amd/amdgpu/amdgpu_gmc.h    |  1 +
>> > drivers/gpu/drm/amd/amdgpu/amdgpu_object.c | 10 ++++++++--
>> >   3 files changed, 16 insertions(+), 2 deletions(-)
>> >
>> > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gmc.c 
>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_gmc.c
>> > index 5f71414190e9..00e940eb69ab 100644
>> > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gmc.c
>> > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gmc.c
>> > @@ -169,6 +169,13 @@ int amdgpu_gmc_set_pte_pde(struct 
>> amdgpu_device *adev, void *cpu_pt_addr,
>> >        return 0;
>> >   }
>> >
>> > +bool bo_in_agp_aperture(struct amdgpu_bo *bo)
>> > +{
>> > +     struct ttm_buffer_object *tbo = &(bo->tbo);
>> > +     struct amdgpu_device *adev = amdgpu_ttm_adev(tbo->bdev);
>> > +
>> > +     return (tbo->resource->start << PAGE_SHIFT) > 
>> adev->gmc.agp_start;
>> > +}
>> >   /**
>> >    * amdgpu_gmc_agp_addr - return the address in the AGP address space
>> >    *
>> > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gmc.h 
>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_gmc.h
>> > index e699d1ca8deb..448dc08e83de 100644
>> > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gmc.h
>> > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gmc.h
>> > @@ -393,6 +393,7 @@ int amdgpu_gmc_set_pte_pde(struct amdgpu_device 
>> *adev, void *cpu_pt_addr,
>> >                                uint64_t flags);
>> >   uint64_t amdgpu_gmc_pd_addr(struct amdgpu_bo *bo);
>> >   uint64_t amdgpu_gmc_agp_addr(struct ttm_buffer_object *bo);
>> > +bool bo_in_agp_aperture(struct amdgpu_bo *bo);
>> >   void amdgpu_gmc_sysvm_location(struct amdgpu_device *adev, struct 
>> amdgpu_gmc *mc);
>> >   void amdgpu_gmc_vram_location(struct amdgpu_device *adev, struct 
>> amdgpu_gmc *mc,
>> >                              u64 base);
>> > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c 
>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
>> > index cef920a93924..91a011d63ab4 100644
>> > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
>> > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
>> > @@ -39,6 +39,7 @@
>> >   #include "amdgpu.h"
>> >   #include "amdgpu_trace.h"
>> >   #include "amdgpu_amdkfd.h"
>> > +#include "amdgpu_gmc.h"
>> >
>> >   /**
>> >    * DOC: amdgpu_object
>> > @@ -1529,8 +1530,13 @@ u64 amdgpu_bo_gpu_offset_no_check(struct 
>> amdgpu_bo *bo)
>> >        struct amdgpu_device *adev = amdgpu_ttm_adev(bo->tbo.bdev);
>> >        uint64_t offset;
>> >
>> > -     offset = (bo->tbo.resource->start << PAGE_SHIFT) +
>> > - amdgpu_ttm_domain_start(adev, bo->tbo.resource->mem_type);
>> > +     /* tbo.resource->start includes agp_start for AGP BOs */
>> > +     if (bo_in_agp_aperture(bo)) {
>> > +             offset = (bo->tbo.resource->start << PAGE_SHIFT);
>> > +     } else {
>> > +             offset = (bo->tbo.resource->start << PAGE_SHIFT) +
>> > + amdgpu_ttm_domain_start(adev, bo->tbo.resource->mem_type);
>> > +     }
>> >
>> >        return amdgpu_gmc_sign_extend(offset);
>> >   }
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/amd-gfx/attachments/20231110/5394dc97/attachment-0001.htm>

More information about the amd-gfx mailing list