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

[Daniel Vetter]

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