[PATCH] drm/radeon: Fix screen corruption (v2)

[Alex Deucher]

On Wed, Dec 14, 2022 at 4:54 PM Robin Murphy <robin.murphy at arm.com> wrote:
>
> On 2022-12-12 02:08, Luben Tuikov wrote:
> > Fix screen corruption on older 32-bit systems using AGP chips.
> >
> > On older systems with little memory, for instance 1.5 GiB, using an AGP chip,
> > the device's DMA mask is 0xFFFFFFFF, but the memory mask is 0x7FFFFFF, and
> > subsequently dma_addressing_limited() returns 0xFFFFFFFF < 0x7FFFFFFF,
> > false. As such the result of this static inline isn't suitable for the last
> > argument to ttm_device_init()--it simply needs to now whether to use GFP_DMA32
> > when allocating DMA buffers.
>
> This sounds wrong to me. If the issues happen on systems without PAE it
> clearly can't have anything to with the actual DMA address size. Not to
> mention that AFAICS 32-bit x86 doesn't even have ZONE_DMA32, so
> GFP_DMA32 would be functionally meaningless anyway. Although the
> reported symptoms initially sounded like they could be caused by DMA
> going to the wrong place, that is also equally consistent with a loss of
> cache coherency.
>
> My (limited) understanding of AGP is that the GART can effectively alias
> memory to a second physical address, so I could well believe that
> something somewhere in the driver stack needs to perform some cache
> maintenance to avoid coherency issues, and that in these particular
> setups whatever that is might be assuming the memory is direct-mapped
> and thus going wrong for highmem pages.
>
> So as I said before, I really think this is not about using GFP_DMA32 at
> all, but about *not* using GFP_HIGHUSER.

One of the wonderful features of AGP is that it has to be used with
uncached memory.  The aperture basically just provides a remapping of
physical pages into a linear aperture that you point the GPU at.  TTM
has to jump through quite a few hoops to get uncached memory in the
first place, so it's likely that that somehow isn't compatible with
HIGHMEM.  Can you get uncached HIGHMEM?

Alex

>
> Thanks,
> Robin.
>
> > Partially reverts commit 33b3ad3788aba846fc8b9a065fe2685a0b64f713.
> >
> > v2: Amend the commit description.
> >
> > Cc: Mikhail Krylov <sqarert at gmail.com>
> > Cc: Alex Deucher <Alexander.Deucher at amd.com>
> > Cc: Robin Murphy <robin.murphy at arm.com>
> > Cc: Direct Rendering Infrastructure - Development <dri-devel at lists.freedesktop.org>
> > Cc: AMD Graphics <amd-gfx at lists.freedesktop.org>
> > Fixes: 33b3ad3788aba8 ("drm/radeon: handle PCIe root ports with addressing limitations")
> > Signed-off-by: Luben Tuikov <luben.tuikov at amd.com>
> > ---
> >   drivers/gpu/drm/radeon/radeon.h        | 1 +
> >   drivers/gpu/drm/radeon/radeon_device.c | 2 +-
> >   drivers/gpu/drm/radeon/radeon_ttm.c    | 2 +-
> >   3 files changed, 3 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/radeon/radeon.h b/drivers/gpu/drm/radeon/radeon.h
> > index 37dec92339b16a..4fe38fd9be3267 100644
> > --- a/drivers/gpu/drm/radeon/radeon.h
> > +++ b/drivers/gpu/drm/radeon/radeon.h
> > @@ -2426,6 +2426,7 @@ struct radeon_device {
> >       struct radeon_wb                wb;
> >       struct radeon_dummy_page        dummy_page;
> >       bool                            shutdown;
> > +     bool                            need_dma32;
> >       bool                            need_swiotlb;
> >       bool                            accel_working;
> >       bool                            fastfb_working; /* IGP feature*/
> > diff --git a/drivers/gpu/drm/radeon/radeon_device.c b/drivers/gpu/drm/radeon/radeon_device.c
> > index 6344454a772172..3643a3cfe061bd 100644
> > --- a/drivers/gpu/drm/radeon/radeon_device.c
> > +++ b/drivers/gpu/drm/radeon/radeon_device.c
> > @@ -1370,7 +1370,7 @@ int radeon_device_init(struct radeon_device *rdev,
> >       if (rdev->family == CHIP_CEDAR)
> >               dma_bits = 32;
> >   #endif
> > -
> > +     rdev->need_dma32 = dma_bits == 32;
> >       r = dma_set_mask_and_coherent(&rdev->pdev->dev, DMA_BIT_MASK(dma_bits));
> >       if (r) {
> >               pr_warn("radeon: No suitable DMA available\n");
> > diff --git a/drivers/gpu/drm/radeon/radeon_ttm.c b/drivers/gpu/drm/radeon/radeon_ttm.c
> > index bdb4c0e0736ba2..3debaeb720d173 100644
> > --- a/drivers/gpu/drm/radeon/radeon_ttm.c
> > +++ b/drivers/gpu/drm/radeon/radeon_ttm.c
> > @@ -696,7 +696,7 @@ int radeon_ttm_init(struct radeon_device *rdev)
> >                              rdev->ddev->anon_inode->i_mapping,
> >                              rdev->ddev->vma_offset_manager,
> >                              rdev->need_swiotlb,
> > -                            dma_addressing_limited(&rdev->pdev->dev));
> > +                            rdev->need_dma32);
> >       if (r) {
> >               DRM_ERROR("failed initializing buffer object driver(%d).\n", r);
> >               return r;
> >
> > base-commit: 20e03e7f6e8efd42168db6d3fe044b804e0ede8f