Re: [PATCH 5/6] drm/mediatek: Convert to use CMA helpers

[Matthias Brugger <matthias.bgg@gmail.com>]

Hi Rob,

On 21/10/2019 23:45, Rob Herring wrote:
> The only reason the Mediatek driver doesn't use the CMA helpers is it
> sets DMA_ATTR_NO_KERNEL_MAPPING and does a vmap() on demand. Using
> vmap() is not even guaranteed to work as DMA buffers may not have a
> struct page. Now that the CMA helpers support setting
> DMA_ATTR_NO_KERNEL_MAPPING as needed or not, convert Mediatek driver to
> use CMA helpers.
> 
> Cc: CK Hu <ck.hu@mediatek.com>
> Cc: Philipp Zabel <p.zabel@pengutronix.de>
> Cc: David Airlie <airlied@linux.ie>
> Cc: Daniel Vetter <daniel@ffwll.ch>
> Cc: Matthias Brugger <matthias.bgg@gmail.com>
> Cc: linux-arm-kernel@lists.infradead.org
> Cc: linux-mediatek@lists.infradead.org
> Signed-off-by: Rob Herring <robh@kernel.org>
> ---

I tested this on my Chromebook with some patches on top of v5.4-rc1 [1], which
work. If I add your patches on top of that, the system does not boot up.
Unfortunately I don't have a serial console, so I wasn't able to see if there is
any error message.

I checked the config and didn't see any suspicious

I added Uli and some guys from chromium, maybe someone can provide some
logs/insights.

Regards,
Matthias

[1] https://github.com/uli/kernel/tree/elm-working-5.4

>  drivers/gpu/drm/mediatek/Makefile        |   1 -
>  drivers/gpu/drm/mediatek/mtk_drm_crtc.c  |   2 +-
>  drivers/gpu/drm/mediatek/mtk_drm_drv.c   |  28 +--
>  drivers/gpu/drm/mediatek/mtk_drm_fb.c    |   1 -
>  drivers/gpu/drm/mediatek/mtk_drm_gem.c   | 289 -----------------------
>  drivers/gpu/drm/mediatek/mtk_drm_gem.h   |  51 ----
>  drivers/gpu/drm/mediatek/mtk_drm_plane.c |   7 +-
>  7 files changed, 15 insertions(+), 364 deletions(-)
>  delete mode 100644 drivers/gpu/drm/mediatek/mtk_drm_gem.c
>  delete mode 100644 drivers/gpu/drm/mediatek/mtk_drm_gem.h
> 
> diff --git a/drivers/gpu/drm/mediatek/Makefile b/drivers/gpu/drm/mediatek/Makefile
> index 82ae49c64221..50a50e86738f 100644
> --- a/drivers/gpu/drm/mediatek/Makefile
> +++ b/drivers/gpu/drm/mediatek/Makefile
> @@ -8,7 +8,6 @@ mediatek-drm-y := mtk_disp_color.o \
>  		  mtk_drm_ddp_comp.o \
>  		  mtk_drm_drv.o \
>  		  mtk_drm_fb.o \
> -		  mtk_drm_gem.o \
>  		  mtk_drm_plane.o \
>  		  mtk_dsi.o \
>  		  mtk_mipi_tx.o \
> diff --git a/drivers/gpu/drm/mediatek/mtk_drm_crtc.c b/drivers/gpu/drm/mediatek/mtk_drm_crtc.c
> index 34a731755791..638d57e8ac12 100644
> --- a/drivers/gpu/drm/mediatek/mtk_drm_crtc.c
> +++ b/drivers/gpu/drm/mediatek/mtk_drm_crtc.c
> @@ -11,6 +11,7 @@
>  
>  #include <drm/drm_atomic_helper.h>
>  #include <drm/drm_plane_helper.h>
> +#include <drm/drm_print.h>
>  #include <drm/drm_probe_helper.h>
>  #include <drm/drm_vblank.h>
>  
> @@ -18,7 +19,6 @@
>  #include "mtk_drm_crtc.h"
>  #include "mtk_drm_ddp.h"
>  #include "mtk_drm_ddp_comp.h"
> -#include "mtk_drm_gem.h"
>  #include "mtk_drm_plane.h"
>  
>  /**
> diff --git a/drivers/gpu/drm/mediatek/mtk_drm_drv.c b/drivers/gpu/drm/mediatek/mtk_drm_drv.c
> index 352b81a7a670..36f32507e5fb 100644
> --- a/drivers/gpu/drm/mediatek/mtk_drm_drv.c
> +++ b/drivers/gpu/drm/mediatek/mtk_drm_drv.c
> @@ -28,7 +28,6 @@
>  #include "mtk_drm_ddp_comp.h"
>  #include "mtk_drm_drv.h"
>  #include "mtk_drm_fb.h"
> -#include "mtk_drm_gem.h"
>  
>  #define DRIVER_NAME "mediatek"
>  #define DRIVER_DESC "Mediatek SoC DRM"
> @@ -335,16 +334,14 @@ static void mtk_drm_kms_deinit(struct drm_device *drm)
>  	drm_mode_config_cleanup(drm);
>  }
>  
> -static const struct file_operations mtk_drm_fops = {
> -	.owner = THIS_MODULE,
> -	.open = drm_open,
> -	.release = drm_release,
> -	.unlocked_ioctl = drm_ioctl,
> -	.mmap = mtk_drm_gem_mmap,
> -	.poll = drm_poll,
> -	.read = drm_read,
> -	.compat_ioctl = drm_compat_ioctl,
> -};
> +DEFINE_DRM_GEM_CMA_FOPS(mtk_drm_fops);
> +
> +static int mtk_drm_gem_dumb_create(struct drm_file *file_priv,
> +				   struct drm_device *dev,
> +				   struct drm_mode_create_dumb *args)
> +{
> +	return drm_gem_cma_dumb_create_internal(file_priv, dev, args);
> +}
>  
>  /*
>   * We need to override this because the device used to import the memory is
> @@ -361,18 +358,15 @@ struct drm_gem_object *mtk_drm_gem_prime_import(struct drm_device *dev,
>  static struct drm_driver mtk_drm_driver = {
>  	.driver_features = DRIVER_MODESET | DRIVER_GEM | DRIVER_ATOMIC,
>  
> -	.gem_free_object_unlocked = mtk_drm_gem_free_object,
>  	.gem_vm_ops = &drm_gem_cma_vm_ops,
> +	.gem_create_object = drm_cma_gem_create_object_default_funcs,
>  	.dumb_create = mtk_drm_gem_dumb_create,
>  
>  	.prime_handle_to_fd = drm_gem_prime_handle_to_fd,
>  	.prime_fd_to_handle = drm_gem_prime_fd_to_handle,
>  	.gem_prime_import = mtk_drm_gem_prime_import,
> -	.gem_prime_get_sg_table = mtk_gem_prime_get_sg_table,
> -	.gem_prime_import_sg_table = mtk_gem_prime_import_sg_table,
> -	.gem_prime_mmap = mtk_drm_gem_mmap_buf,
> -	.gem_prime_vmap = mtk_drm_gem_prime_vmap,
> -	.gem_prime_vunmap = mtk_drm_gem_prime_vunmap,
> +	.gem_prime_import_sg_table = drm_gem_cma_prime_import_sg_table_vmap,
> +	.gem_prime_mmap = drm_gem_prime_mmap,
>  	.fops = &mtk_drm_fops,
>  
>  	.name = DRIVER_NAME,
> diff --git a/drivers/gpu/drm/mediatek/mtk_drm_fb.c b/drivers/gpu/drm/mediatek/mtk_drm_fb.c
> index 3f230a28a2dc..596b4d5ed002 100644
> --- a/drivers/gpu/drm/mediatek/mtk_drm_fb.c
> +++ b/drivers/gpu/drm/mediatek/mtk_drm_fb.c
> @@ -14,7 +14,6 @@
>  
>  #include "mtk_drm_drv.h"
>  #include "mtk_drm_fb.h"
> -#include "mtk_drm_gem.h"
>  
>  static const struct drm_framebuffer_funcs mtk_drm_fb_funcs = {
>  	.create_handle = drm_gem_fb_create_handle,
> diff --git a/drivers/gpu/drm/mediatek/mtk_drm_gem.c b/drivers/gpu/drm/mediatek/mtk_drm_gem.c
> deleted file mode 100644
> index ca672f1d140d..000000000000
> --- a/drivers/gpu/drm/mediatek/mtk_drm_gem.c
> +++ /dev/null
> @@ -1,289 +0,0 @@
> -// SPDX-License-Identifier: GPL-2.0-only
> -/*
> - * Copyright (c) 2015 MediaTek Inc.
> - */
> -
> -#include <linux/dma-buf.h>
> -
> -#include <drm/drm.h>
> -#include <drm/drm_device.h>
> -#include <drm/drm_gem.h>
> -#include <drm/drm_prime.h>
> -
> -#include "mtk_drm_drv.h"
> -#include "mtk_drm_gem.h"
> -
> -static struct mtk_drm_gem_obj *mtk_drm_gem_init(struct drm_device *dev,
> -						unsigned long size)
> -{
> -	struct mtk_drm_gem_obj *mtk_gem_obj;
> -	int ret;
> -
> -	size = round_up(size, PAGE_SIZE);
> -
> -	mtk_gem_obj = kzalloc(sizeof(*mtk_gem_obj), GFP_KERNEL);
> -	if (!mtk_gem_obj)
> -		return ERR_PTR(-ENOMEM);
> -
> -	ret = drm_gem_object_init(dev, &mtk_gem_obj->base, size);
> -	if (ret < 0) {
> -		DRM_ERROR("failed to initialize gem object\n");
> -		kfree(mtk_gem_obj);
> -		return ERR_PTR(ret);
> -	}
> -
> -	return mtk_gem_obj;
> -}
> -
> -struct mtk_drm_gem_obj *mtk_drm_gem_create(struct drm_device *dev,
> -					   size_t size, bool alloc_kmap)
> -{
> -	struct mtk_drm_private *priv = dev->dev_private;
> -	struct mtk_drm_gem_obj *mtk_gem;
> -	struct drm_gem_object *obj;
> -	int ret;
> -
> -	mtk_gem = mtk_drm_gem_init(dev, size);
> -	if (IS_ERR(mtk_gem))
> -		return ERR_CAST(mtk_gem);
> -
> -	obj = &mtk_gem->base;
> -
> -	mtk_gem->dma_attrs = DMA_ATTR_WRITE_COMBINE;
> -
> -	if (!alloc_kmap)
> -		mtk_gem->dma_attrs |= DMA_ATTR_NO_KERNEL_MAPPING;
> -
> -	mtk_gem->cookie = dma_alloc_attrs(priv->dma_dev, obj->size,
> -					  &mtk_gem->dma_addr, GFP_KERNEL,
> -					  mtk_gem->dma_attrs);
> -	if (!mtk_gem->cookie) {
> -		DRM_ERROR("failed to allocate %zx byte dma buffer", obj->size);
> -		ret = -ENOMEM;
> -		goto err_gem_free;
> -	}
> -
> -	if (alloc_kmap)
> -		mtk_gem->kvaddr = mtk_gem->cookie;
> -
> -	DRM_DEBUG_DRIVER("cookie = %p dma_addr = %pad size = %zu\n",
> -			 mtk_gem->cookie, &mtk_gem->dma_addr,
> -			 size);
> -
> -	return mtk_gem;
> -
> -err_gem_free:
> -	drm_gem_object_release(obj);
> -	kfree(mtk_gem);
> -	return ERR_PTR(ret);
> -}
> -
> -void mtk_drm_gem_free_object(struct drm_gem_object *obj)
> -{
> -	struct mtk_drm_gem_obj *mtk_gem = to_mtk_gem_obj(obj);
> -	struct mtk_drm_private *priv = obj->dev->dev_private;
> -
> -	if (mtk_gem->sg)
> -		drm_prime_gem_destroy(obj, mtk_gem->sg);
> -	else
> -		dma_free_attrs(priv->dma_dev, obj->size, mtk_gem->cookie,
> -			       mtk_gem->dma_addr, mtk_gem->dma_attrs);
> -
> -	/* release file pointer to gem object. */
> -	drm_gem_object_release(obj);
> -
> -	kfree(mtk_gem);
> -}
> -
> -int mtk_drm_gem_dumb_create(struct drm_file *file_priv, struct drm_device *dev,
> -			    struct drm_mode_create_dumb *args)
> -{
> -	struct mtk_drm_gem_obj *mtk_gem;
> -	int ret;
> -
> -	args->pitch = DIV_ROUND_UP(args->width * args->bpp, 8);
> -	args->size = args->pitch * args->height;
> -
> -	mtk_gem = mtk_drm_gem_create(dev, args->size, false);
> -	if (IS_ERR(mtk_gem))
> -		return PTR_ERR(mtk_gem);
> -
> -	/*
> -	 * allocate a id of idr table where the obj is registered
> -	 * and handle has the id what user can see.
> -	 */
> -	ret = drm_gem_handle_create(file_priv, &mtk_gem->base, &args->handle);
> -	if (ret)
> -		goto err_handle_create;
> -
> -	/* drop reference from allocate - handle holds it now. */
> -	drm_gem_object_put_unlocked(&mtk_gem->base);
> -
> -	return 0;
> -
> -err_handle_create:
> -	mtk_drm_gem_free_object(&mtk_gem->base);
> -	return ret;
> -}
> -
> -static int mtk_drm_gem_object_mmap(struct drm_gem_object *obj,
> -				   struct vm_area_struct *vma)
> -
> -{
> -	int ret;
> -	struct mtk_drm_gem_obj *mtk_gem = to_mtk_gem_obj(obj);
> -	struct mtk_drm_private *priv = obj->dev->dev_private;
> -
> -	/*
> -	 * dma_alloc_attrs() allocated a struct page table for mtk_gem, so clear
> -	 * VM_PFNMAP flag that was set by drm_gem_mmap_obj()/drm_gem_mmap().
> -	 */
> -	vma->vm_flags &= ~VM_PFNMAP;
> -
> -	ret = dma_mmap_attrs(priv->dma_dev, vma, mtk_gem->cookie,
> -			     mtk_gem->dma_addr, obj->size, mtk_gem->dma_attrs);
> -	if (ret)
> -		drm_gem_vm_close(vma);
> -
> -	return ret;
> -}
> -
> -int mtk_drm_gem_mmap_buf(struct drm_gem_object *obj, struct vm_area_struct *vma)
> -{
> -	int ret;
> -
> -	ret = drm_gem_mmap_obj(obj, obj->size, vma);
> -	if (ret)
> -		return ret;
> -
> -	return mtk_drm_gem_object_mmap(obj, vma);
> -}
> -
> -int mtk_drm_gem_mmap(struct file *filp, struct vm_area_struct *vma)
> -{
> -	struct drm_gem_object *obj;
> -	int ret;
> -
> -	ret = drm_gem_mmap(filp, vma);
> -	if (ret)
> -		return ret;
> -
> -	obj = vma->vm_private_data;
> -
> -	/*
> -	 * Set vm_pgoff (used as a fake buffer offset by DRM) to 0 and map the
> -	 * whole buffer from the start.
> -	 */
> -	vma->vm_pgoff = 0;
> -
> -	return mtk_drm_gem_object_mmap(obj, vma);
> -}
> -
> -/*
> - * Allocate a sg_table for this GEM object.
> - * Note: Both the table's contents, and the sg_table itself must be freed by
> - *       the caller.
> - * Returns a pointer to the newly allocated sg_table, or an ERR_PTR() error.
> - */
> -struct sg_table *mtk_gem_prime_get_sg_table(struct drm_gem_object *obj)
> -{
> -	struct mtk_drm_gem_obj *mtk_gem = to_mtk_gem_obj(obj);
> -	struct mtk_drm_private *priv = obj->dev->dev_private;
> -	struct sg_table *sgt;
> -	int ret;
> -
> -	sgt = kzalloc(sizeof(*sgt), GFP_KERNEL);
> -	if (!sgt)
> -		return ERR_PTR(-ENOMEM);
> -
> -	ret = dma_get_sgtable_attrs(priv->dma_dev, sgt, mtk_gem->cookie,
> -				    mtk_gem->dma_addr, obj->size,
> -				    mtk_gem->dma_attrs);
> -	if (ret) {
> -		DRM_ERROR("failed to allocate sgt, %d\n", ret);
> -		kfree(sgt);
> -		return ERR_PTR(ret);
> -	}
> -
> -	return sgt;
> -}
> -
> -struct drm_gem_object *mtk_gem_prime_import_sg_table(struct drm_device *dev,
> -			struct dma_buf_attachment *attach, struct sg_table *sg)
> -{
> -	struct mtk_drm_gem_obj *mtk_gem;
> -	int ret;
> -	struct scatterlist *s;
> -	unsigned int i;
> -	dma_addr_t expected;
> -
> -	mtk_gem = mtk_drm_gem_init(dev, attach->dmabuf->size);
> -
> -	if (IS_ERR(mtk_gem))
> -		return ERR_CAST(mtk_gem);
> -
> -	expected = sg_dma_address(sg->sgl);
> -	for_each_sg(sg->sgl, s, sg->nents, i) {
> -		if (sg_dma_address(s) != expected) {
> -			DRM_ERROR("sg_table is not contiguous");
> -			ret = -EINVAL;
> -			goto err_gem_free;
> -		}
> -		expected = sg_dma_address(s) + sg_dma_len(s);
> -	}
> -
> -	mtk_gem->dma_addr = sg_dma_address(sg->sgl);
> -	mtk_gem->sg = sg;
> -
> -	return &mtk_gem->base;
> -
> -err_gem_free:
> -	kfree(mtk_gem);
> -	return ERR_PTR(ret);
> -}
> -
> -void *mtk_drm_gem_prime_vmap(struct drm_gem_object *obj)
> -{
> -	struct mtk_drm_gem_obj *mtk_gem = to_mtk_gem_obj(obj);
> -	struct sg_table *sgt;
> -	struct sg_page_iter iter;
> -	unsigned int npages;
> -	unsigned int i = 0;
> -
> -	if (mtk_gem->kvaddr)
> -		return mtk_gem->kvaddr;
> -
> -	sgt = mtk_gem_prime_get_sg_table(obj);
> -	if (IS_ERR(sgt))
> -		return NULL;
> -
> -	npages = obj->size >> PAGE_SHIFT;
> -	mtk_gem->pages = kcalloc(npages, sizeof(*mtk_gem->pages), GFP_KERNEL);
> -	if (!mtk_gem->pages)
> -		goto out;
> -
> -	for_each_sg_page(sgt->sgl, &iter, sgt->orig_nents, 0) {
> -		mtk_gem->pages[i++] = sg_page_iter_page(&iter);
> -		if (i > npages)
> -			break;
> -	}
> -	mtk_gem->kvaddr = vmap(mtk_gem->pages, npages, VM_MAP,
> -			       pgprot_writecombine(PAGE_KERNEL));
> -
> -out:
> -	kfree((void *)sgt);
> -
> -	return mtk_gem->kvaddr;
> -}
> -
> -void mtk_drm_gem_prime_vunmap(struct drm_gem_object *obj, void *vaddr)
> -{
> -	struct mtk_drm_gem_obj *mtk_gem = to_mtk_gem_obj(obj);
> -
> -	if (!mtk_gem->pages)
> -		return;
> -
> -	vunmap(vaddr);
> -	mtk_gem->kvaddr = 0;
> -	kfree((void *)mtk_gem->pages);
> -}
> diff --git a/drivers/gpu/drm/mediatek/mtk_drm_gem.h b/drivers/gpu/drm/mediatek/mtk_drm_gem.h
> deleted file mode 100644
> index ff9f976d9807..000000000000
> --- a/drivers/gpu/drm/mediatek/mtk_drm_gem.h
> +++ /dev/null
> @@ -1,51 +0,0 @@
> -/* SPDX-License-Identifier: GPL-2.0-only */
> -/*
> - * Copyright (c) 2015 MediaTek Inc.
> - */
> -
> -#ifndef _MTK_DRM_GEM_H_
> -#define _MTK_DRM_GEM_H_
> -
> -#include <drm/drm_gem.h>
> -
> -/*
> - * mtk drm buffer structure.
> - *
> - * @base: a gem object.
> - *	- a new handle to this gem object would be created
> - *	by drm_gem_handle_create().
> - * @cookie: the return value of dma_alloc_attrs(), keep it for dma_free_attrs()
> - * @kvaddr: kernel virtual address of gem buffer.
> - * @dma_addr: dma address of gem buffer.
> - * @dma_attrs: dma attributes of gem buffer.
> - *
> - * P.S. this object would be transferred to user as kms_bo.handle so
> - *	user can access the buffer through kms_bo.handle.
> - */
> -struct mtk_drm_gem_obj {
> -	struct drm_gem_object	base;
> -	void			*cookie;
> -	void			*kvaddr;
> -	dma_addr_t		dma_addr;
> -	unsigned long		dma_attrs;
> -	struct sg_table		*sg;
> -	struct page		**pages;
> -};
> -
> -#define to_mtk_gem_obj(x)	container_of(x, struct mtk_drm_gem_obj, base)
> -
> -void mtk_drm_gem_free_object(struct drm_gem_object *gem);
> -struct mtk_drm_gem_obj *mtk_drm_gem_create(struct drm_device *dev, size_t size,
> -					   bool alloc_kmap);
> -int mtk_drm_gem_dumb_create(struct drm_file *file_priv, struct drm_device *dev,
> -			    struct drm_mode_create_dumb *args);
> -int mtk_drm_gem_mmap(struct file *filp, struct vm_area_struct *vma);
> -int mtk_drm_gem_mmap_buf(struct drm_gem_object *obj,
> -			 struct vm_area_struct *vma);
> -struct sg_table *mtk_gem_prime_get_sg_table(struct drm_gem_object *obj);
> -struct drm_gem_object *mtk_gem_prime_import_sg_table(struct drm_device *dev,
> -			struct dma_buf_attachment *attach, struct sg_table *sg);
> -void *mtk_drm_gem_prime_vmap(struct drm_gem_object *obj);
> -void mtk_drm_gem_prime_vunmap(struct drm_gem_object *obj, void *vaddr);
> -
> -#endif
> diff --git a/drivers/gpu/drm/mediatek/mtk_drm_plane.c b/drivers/gpu/drm/mediatek/mtk_drm_plane.c
> index 584a9ecadce6..8f256602f075 100644
> --- a/drivers/gpu/drm/mediatek/mtk_drm_plane.c
> +++ b/drivers/gpu/drm/mediatek/mtk_drm_plane.c
> @@ -8,13 +8,14 @@
>  #include <drm/drm_atomic_helper.h>
>  #include <drm/drm_fourcc.h>
>  #include <drm/drm_plane_helper.h>
> +#include <drm/drm_print.h>
> +#include <drm/drm_gem_cma_helper.h>
>  #include <drm/drm_gem_framebuffer_helper.h>
>  
>  #include "mtk_drm_crtc.h"
>  #include "mtk_drm_ddp_comp.h"
>  #include "mtk_drm_drv.h"
>  #include "mtk_drm_fb.h"
> -#include "mtk_drm_gem.h"
>  #include "mtk_drm_plane.h"
>  
>  static const u32 formats[] = {
> @@ -108,7 +109,6 @@ static void mtk_plane_atomic_update(struct drm_plane *plane,
>  	struct drm_crtc *crtc = plane->state->crtc;
>  	struct drm_framebuffer *fb = plane->state->fb;
>  	struct drm_gem_object *gem;
> -	struct mtk_drm_gem_obj *mtk_gem;
>  	unsigned int pitch, format;
>  	dma_addr_t addr;
>  
> @@ -116,8 +116,7 @@ static void mtk_plane_atomic_update(struct drm_plane *plane,
>  		return;
>  
>  	gem = fb->obj[0];
> -	mtk_gem = to_mtk_gem_obj(gem);
> -	addr = mtk_gem->dma_addr;
> +	addr = to_drm_gem_cma_obj(gem)->paddr;
>  	pitch = fb->pitches[0];
>  	format = fb->format->format;
>  
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel