Re: [PATCH 1/2] drm: remove usage of drm_pci_alloc/free

From: Daniel Vetter
Date: Tue Apr 27 2021 - 03:18:04 EST


On Thu, Apr 22, 2021 at 07:02:43PM -0700, Joseph Kogut wrote:
> Remove usage of legacy dma-api abstraction in preparation for removal
>
> Signed-off-by: Joseph Kogut <joseph.kogut@xxxxxxxxx>
> ---
> Checkpatch warns here that r128 is marked obsolete, and asks for no
> unnecessary modifications.
>
> This series aims to address the FIXME in drivers/gpu/drm/drm_pci.c
> explaining that drm_pci_alloc/free is a needless abstraction of the
> dma-api, and it should be removed. Unfortunately, doing this requires
> removing the usage from an obsolete driver as well.
>
> If this patch is rejected for modifying an obsolete driver, would it be
> appropriate to follow up removing the FIXME from drm_pci?

Feels like a good enough reason, both patches queued up in drm-misc-next
for 5.14. Thanks a lot for doing them.
-Daniel

>
> drivers/gpu/drm/drm_bufs.c | 19 ++++++++++++++++---
> drivers/gpu/drm/drm_dma.c | 8 +++++++-
> drivers/gpu/drm/r128/ati_pcigart.c | 22 ++++++++++++++++++----
> 3 files changed, 41 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_bufs.c b/drivers/gpu/drm/drm_bufs.c
> index e3d77dfefb0a..94bc1f6049c9 100644
> --- a/drivers/gpu/drm/drm_bufs.c
> +++ b/drivers/gpu/drm/drm_bufs.c
> @@ -674,12 +674,17 @@ int drm_legacy_rmmap_ioctl(struct drm_device *dev, void *data,
> static void drm_cleanup_buf_error(struct drm_device *dev,
> struct drm_buf_entry *entry)
> {
> + drm_dma_handle_t *dmah;
> int i;
>
> if (entry->seg_count) {
> for (i = 0; i < entry->seg_count; i++) {
> if (entry->seglist[i]) {
> - drm_pci_free(dev, entry->seglist[i]);
> + dmah = entry->seglist[i];
> + dma_free_coherent(dev->dev,
> + dmah->size,
> + dmah->vaddr,
> + dmah->busaddr);
> }
> }
> kfree(entry->seglist);
> @@ -978,10 +983,18 @@ int drm_legacy_addbufs_pci(struct drm_device *dev,
> page_count = 0;
>
> while (entry->buf_count < count) {
> + dmah = kmalloc(sizeof(drm_dma_handle_t), GFP_KERNEL);
> + if (!dmah)
> + return -ENOMEM;
>
> - dmah = drm_pci_alloc(dev, PAGE_SIZE << page_order, 0x1000);
> + dmah->size = total;
> + dmah->vaddr = dma_alloc_coherent(dev->dev,
> + dmah->size,
> + &dmah->busaddr,
> + GFP_KERNEL);
> + if (!dmah->vaddr) {
> + kfree(dmah);
>
> - if (!dmah) {
> /* Set count correctly so we free the proper amount. */
> entry->buf_count = count;
> entry->seg_count = count;
> diff --git a/drivers/gpu/drm/drm_dma.c b/drivers/gpu/drm/drm_dma.c
> index d07ba54ec945..eb6b741a6f99 100644
> --- a/drivers/gpu/drm/drm_dma.c
> +++ b/drivers/gpu/drm/drm_dma.c
> @@ -81,6 +81,7 @@ int drm_legacy_dma_setup(struct drm_device *dev)
> void drm_legacy_dma_takedown(struct drm_device *dev)
> {
> struct drm_device_dma *dma = dev->dma;
> + drm_dma_handle_t *dmah;
> int i, j;
>
> if (!drm_core_check_feature(dev, DRIVER_HAVE_DMA) ||
> @@ -100,7 +101,12 @@ void drm_legacy_dma_takedown(struct drm_device *dev)
> dma->bufs[i].seg_count);
> for (j = 0; j < dma->bufs[i].seg_count; j++) {
> if (dma->bufs[i].seglist[j]) {
> - drm_pci_free(dev, dma->bufs[i].seglist[j]);
> + dmah = dma->bufs[i].seglist[j];
> + dma_free_coherent(dev->dev,
> + dmah->size,
> + dmah->vaddr,
> + dmah->busaddr);
> + kfree(dmah);
> }
> }
> kfree(dma->bufs[i].seglist);
> diff --git a/drivers/gpu/drm/r128/ati_pcigart.c b/drivers/gpu/drm/r128/ati_pcigart.c
> index 1234ec60c0af..fbb0cfd79758 100644
> --- a/drivers/gpu/drm/r128/ati_pcigart.c
> +++ b/drivers/gpu/drm/r128/ati_pcigart.c
> @@ -45,18 +45,32 @@
> static int drm_ati_alloc_pcigart_table(struct drm_device *dev,
> struct drm_ati_pcigart_info *gart_info)
> {
> - gart_info->table_handle = drm_pci_alloc(dev, gart_info->table_size,
> - PAGE_SIZE);
> - if (gart_info->table_handle == NULL)
> + drm_dma_handle_t *dmah = kmalloc(sizeof(drm_dma_handle_t), GFP_KERNEL);
> +
> + if (!dmah)
> + return -ENOMEM;
> +
> + dmah->size = gart_info->table_size;
> + dmah->vaddr = dma_alloc_coherent(dev->dev,
> + dmah->size,
> + &dmah->busaddr,
> + GFP_KERNEL);
> +
> + if (!dmah->vaddr) {
> + kfree(dmah);
> return -ENOMEM;
> + }
>
> + gart_info->table_handle = dmah;
> return 0;
> }
>
> static void drm_ati_free_pcigart_table(struct drm_device *dev,
> struct drm_ati_pcigart_info *gart_info)
> {
> - drm_pci_free(dev, gart_info->table_handle);
> + drm_dma_handle_t *dmah = gart_info->table_handle;
> +
> + dma_free_coherent(dev->dev, dmah->size, dmah->vaddr, dmah->busaddr);
> gart_info->table_handle = NULL;
> }
>
> --
> 2.31.1
>

--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch