| Message ID | 1477639682-22520-3-git-send-email-zourongrong@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, Oct 28, 2016 at 3:27 AM, Rongrong Zou <zourongrong@gmail.com> wrote: > Hibmc have 32m video memory which can be accessed through PCIe by host, > we use ttm to manage these memory. > > Signed-off-by: Rongrong Zou <zourongrong@gmail.com> > --- > drivers/gpu/drm/hisilicon/hibmc/Kconfig | 1 + > drivers/gpu/drm/hisilicon/hibmc/Makefile | 2 +- > drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c | 12 + > drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.h | 46 +++ > drivers/gpu/drm/hisilicon/hibmc/hibmc_ttm.c | 490 ++++++++++++++++++++++++ > 5 files changed, 550 insertions(+), 1 deletion(-) > create mode 100644 drivers/gpu/drm/hisilicon/hibmc/hibmc_ttm.c > > diff --git a/drivers/gpu/drm/hisilicon/hibmc/Kconfig b/drivers/gpu/drm/hisilicon/hibmc/Kconfig > index a9af90d..bcb8c18 100644 > --- a/drivers/gpu/drm/hisilicon/hibmc/Kconfig > +++ b/drivers/gpu/drm/hisilicon/hibmc/Kconfig > @@ -1,6 +1,7 @@ > config DRM_HISI_HIBMC > tristate "DRM Support for Hisilicon Hibmc" > depends on DRM && PCI > + select DRM_TTM > > help > Choose this option if you have a Hisilicon Hibmc soc chipset. > diff --git a/drivers/gpu/drm/hisilicon/hibmc/Makefile b/drivers/gpu/drm/hisilicon/hibmc/Makefile > index 97cf4a0..d5c40b8 100644 > --- a/drivers/gpu/drm/hisilicon/hibmc/Makefile > +++ b/drivers/gpu/drm/hisilicon/hibmc/Makefile > @@ -1,5 +1,5 @@ > ccflags-y := -Iinclude/drm > -hibmc-drm-y := hibmc_drm_drv.o hibmc_drm_power.o > +hibmc-drm-y := hibmc_drm_drv.o hibmc_drm_power.o hibmc_ttm.o > > obj-$(CONFIG_DRM_HISI_HIBMC) +=hibmc-drm.o > #obj-y += hibmc-drm.o > diff --git a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c > index 4669d42..81f4301 100644 > --- a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c > +++ b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c > @@ -31,6 +31,7 @@ > #ifdef CONFIG_COMPAT > .compat_ioctl = drm_compat_ioctl, > #endif > + .mmap = hibmc_mmap, > .poll = drm_poll, > .read = drm_read, > .llseek = no_llseek, > @@ -46,6 +47,8 @@ static void hibmc_disable_vblank(struct drm_device *dev, unsigned int pipe) > } > > static struct drm_driver hibmc_driver = { > + .driver_features = DRIVER_GEM, > + nit: extra space > .fops = &hibmc_fops, > .name = "hibmc", > .date = "20160828", > @@ -55,6 +58,10 @@ static void hibmc_disable_vblank(struct drm_device *dev, unsigned int pipe) > .get_vblank_counter = drm_vblank_no_hw_counter, > .enable_vblank = hibmc_enable_vblank, > .disable_vblank = hibmc_disable_vblank, > + .gem_free_object_unlocked = hibmc_gem_free_object, > + .dumb_create = hibmc_dumb_create, > + .dumb_map_offset = hibmc_dumb_mmap_offset, > + .dumb_destroy = drm_gem_dumb_destroy, > }; > > static int hibmc_pm_suspend(struct device *dev) > @@ -163,6 +170,7 @@ static int hibmc_unload(struct drm_device *dev) > { > struct hibmc_drm_device *hidev = dev->dev_private; > > + hibmc_mm_fini(hidev); > hibmc_hw_fini(hidev); > dev->dev_private = NULL; > return 0; > @@ -183,6 +191,10 @@ static int hibmc_load(struct drm_device *dev, unsigned long flags) > if (ret) > goto err; > > + ret = hibmc_mm_init(hidev); > + if (ret) > + goto err; > + > return 0; > > err: > diff --git a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.h b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.h > index 0037341..db8d80e 100644 > --- a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.h > +++ b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.h > @@ -20,6 +20,8 @@ > #define HIBMC_DRM_DRV_H > > #include <drm/drmP.h> > +#include <drm/ttm/ttm_bo_driver.h> > +#include <drm/drm_gem.h> nit: alphabetize > > struct hibmc_drm_device { > /* hw */ > @@ -30,6 +32,50 @@ struct hibmc_drm_device { > > /* drm */ > struct drm_device *dev; > + > + /* ttm */ > + struct { > + struct drm_global_reference mem_global_ref; > + struct ttm_bo_global_ref bo_global_ref; > + struct ttm_bo_device bdev; > + bool initialized; > + } ttm; I don't think you gain anything other than keystrokes from the substruct > + > + bool mm_inited; > }; > > +struct hibmc_bo { > + struct ttm_buffer_object bo; > + struct ttm_placement placement; > + struct ttm_bo_kmap_obj kmap; > + struct drm_gem_object gem; > + struct ttm_place placements[3]; > + int pin_count; > +}; > + > +static inline struct hibmc_bo *hibmc_bo(struct ttm_buffer_object *bo) > +{ > + return container_of(bo, struct hibmc_bo, bo); > +} > + > +static inline struct hibmc_bo *gem_to_hibmc_bo(struct drm_gem_object *gem) > +{ > + return container_of(gem, struct hibmc_bo, gem); > +} > + > +#define DRM_FILE_PAGE_OFFSET (0x100000000ULL >> PAGE_SHIFT) Hide this in ttm.c > + > +int hibmc_gem_create(struct drm_device *dev, u32 size, bool iskernel, > + struct drm_gem_object **obj); > + > +int hibmc_mm_init(struct hibmc_drm_device *hibmc); > +void hibmc_mm_fini(struct hibmc_drm_device *hibmc); > +int hibmc_bo_pin(struct hibmc_bo *bo, u32 pl_flag, u64 *gpu_addr); > +void hibmc_gem_free_object(struct drm_gem_object *obj); > +int hibmc_dumb_create(struct drm_file *file, struct drm_device *dev, > + struct drm_mode_create_dumb *args); > +int hibmc_dumb_mmap_offset(struct drm_file *file, struct drm_device *dev, > + u32 handle, u64 *offset); > +int hibmc_mmap(struct file *filp, struct vm_area_struct *vma); > + > #endif > diff --git a/drivers/gpu/drm/hisilicon/hibmc/hibmc_ttm.c b/drivers/gpu/drm/hisilicon/hibmc/hibmc_ttm.c > new file mode 100644 > index 0000000..0802ebd > --- /dev/null > +++ b/drivers/gpu/drm/hisilicon/hibmc/hibmc_ttm.c > @@ -0,0 +1,490 @@ > +/* Hisilicon Hibmc SoC drm driver > + * > + * Based on the bochs drm driver. > + * > + * Copyright (c) 2016 Huawei Limited. > + * > + * Author: > + * Rongrong Zou <zourongrong@huawei.com> > + * Rongrong Zou <zourongrong@gmail.com> > + * Jianhua Li <lijianhua@huawei.com> > + * > + * This program is free software; you can redistribute it and/or modify > + * it under the terms of the GNU General Public License as published by > + * the Free Software Foundation; either version 2 of the License, or > + * (at your option) any later version. > + * > + */ > + > +#include "hibmc_drm_drv.h" > +#include <ttm/ttm_page_alloc.h> > +#include <drm/drm_crtc_helper.h> > +#include <drm/drm_atomic_helper.h> > + > +static inline struct hibmc_drm_device * > +hibmc_bdev(struct ttm_bo_device *bd) > +{ > + return container_of(bd, struct hibmc_drm_device, ttm.bdev); > +} > + > +static int > +hibmc_ttm_mem_global_init(struct drm_global_reference *ref) > +{ > + return ttm_mem_global_init(ref->object); > +} > + > +static void > +hibmc_ttm_mem_global_release(struct drm_global_reference *ref) > +{ > + ttm_mem_global_release(ref->object); > +} > + > +static int hibmc_ttm_global_init(struct hibmc_drm_device *hibmc) > +{ > + struct drm_global_reference *global_ref; > + int r; nit: try not to use one character variable names unless it's for the purpose of a loop (ie: i,j). You also use ret elsewhere in the driver, so it'd be nice to remain consistent > + > + global_ref = &hibmc->ttm.mem_global_ref; I think using the global_ref local obfuscates what you're doing here. It saves you 6 characters while typing, but adds a layer of indirection for all future readers. > + global_ref->global_type = DRM_GLOBAL_TTM_MEM; > + global_ref->size = sizeof(struct ttm_mem_global); > + global_ref->init = &hibmc_ttm_mem_global_init; > + global_ref->release = &hibmc_ttm_mem_global_release; > + r = drm_global_item_ref(global_ref); > + if (r != 0) { nit: if (r) > + DRM_ERROR("Failed setting up TTM memory accounting subsystem.\n" > + ); Breaking up the line for one character is probably not worthwhile, and you should really print the error. How about: DRM_ERROR("Could not get ref on ttm global ret=%d.\n", ret); > + return r; > + } > + > + hibmc->ttm.bo_global_ref.mem_glob = > + hibmc->ttm.mem_global_ref.object; > + global_ref = &hibmc->ttm.bo_global_ref.ref; > + global_ref->global_type = DRM_GLOBAL_TTM_BO; > + global_ref->size = sizeof(struct ttm_bo_global); > + global_ref->init = &ttm_bo_global_init; > + global_ref->release = &ttm_bo_global_release; > + r = drm_global_item_ref(global_ref); > + if (r != 0) { > + DRM_ERROR("Failed setting up TTM BO subsystem.\n"); > + drm_global_item_unref(&hibmc->ttm.mem_global_ref); > + return r; > + } > + return 0; > +} > + > +static void > +hibmc_ttm_global_release(struct hibmc_drm_device *hibmc) > +{ > + if (!hibmc->ttm.mem_global_ref.release) Are you actually hitting this condition? This seems like it's papering over something else. > + return; > + > + drm_global_item_unref(&hibmc->ttm.bo_global_ref.ref); > + drm_global_item_unref(&hibmc->ttm.mem_global_ref); > + hibmc->ttm.mem_global_ref.release = NULL; > +} > + > +static void hibmc_bo_ttm_destroy(struct ttm_buffer_object *tbo) > +{ > + struct hibmc_bo *bo; > + > + bo = container_of(tbo, struct hibmc_bo, bo); nit: No need to split this into a separate line. > + > + drm_gem_object_release(&bo->gem); > + kfree(bo); > +} > + > +static bool hibmc_ttm_bo_is_hibmc_bo(struct ttm_buffer_object *bo) > +{ > + if (bo->destroy == &hibmc_bo_ttm_destroy) > + return true; > + return false; return bo->destroy == &hibmc_bo_ttm_destroy; > +} > + > +static int > +hibmc_bo_init_mem_type(struct ttm_bo_device *bdev, u32 type, > + struct ttm_mem_type_manager *man) > +{ > + switch (type) { > + case TTM_PL_SYSTEM: > + man->flags = TTM_MEMTYPE_FLAG_MAPPABLE; > + man->available_caching = TTM_PL_MASK_CACHING; > + man->default_caching = TTM_PL_FLAG_CACHED; > + break; > + case TTM_PL_VRAM: > + man->func = &ttm_bo_manager_func; > + man->flags = TTM_MEMTYPE_FLAG_FIXED | > + TTM_MEMTYPE_FLAG_MAPPABLE; > + man->available_caching = TTM_PL_FLAG_UNCACHED | > + TTM_PL_FLAG_WC; > + man->default_caching = TTM_PL_FLAG_WC; > + break; > + default: > + DRM_ERROR("Unsupported memory type %u\n", type); > + return -EINVAL; > + } > + return 0; > +} > + > +void hibmc_ttm_placement(struct hibmc_bo *bo, int domain) > +{ > + u32 c = 0; Can you please use a more descriptive name than 'c'? > + u32 i; > + > + bo->placement.placement = bo->placements; > + bo->placement.busy_placement = bo->placements; > + if (domain & TTM_PL_FLAG_VRAM) > + bo->placements[c++].flags = TTM_PL_FLAG_WC | > + TTM_PL_FLAG_UNCACHED | TTM_PL_FLAG_VRAM; nit: you're alignment is off here and below > + if (domain & TTM_PL_FLAG_SYSTEM) > + bo->placements[c++].flags = TTM_PL_MASK_CACHING | > + TTM_PL_FLAG_SYSTEM; > + if (!c) > + bo->placements[c++].flags = TTM_PL_MASK_CACHING | > + TTM_PL_FLAG_SYSTEM; > + > + bo->placement.num_placement = c; > + bo->placement.num_busy_placement = c; > + for (i = 0; i < c; ++i) { nit: we tend towards post-increment in kernel > + bo->placements[i].fpfn = 0; > + bo->placements[i].lpfn = 0; > + } > +} > + > +static void > +hibmc_bo_evict_flags(struct ttm_buffer_object *bo, struct ttm_placement *pl) > +{ > + struct hibmc_bo *hibmcbo = hibmc_bo(bo); > + > + if (!hibmc_ttm_bo_is_hibmc_bo(bo)) > + return; > + > + hibmc_ttm_placement(hibmcbo, TTM_PL_FLAG_SYSTEM); > + *pl = hibmcbo->placement; > +} > + > +static int hibmc_bo_verify_access(struct ttm_buffer_object *bo, > + struct file *filp) > +{ > + struct hibmc_bo *hibmcbo = hibmc_bo(bo); > + > + return drm_vma_node_verify_access(&hibmcbo->gem.vma_node, > + filp->private_data); > +} > + > +static int hibmc_ttm_io_mem_reserve(struct ttm_bo_device *bdev, > + struct ttm_mem_reg *mem) > +{ > + struct ttm_mem_type_manager *man = &bdev->man[mem->mem_type]; > + struct hibmc_drm_device *hibmc = hibmc_bdev(bdev); > + > + mem->bus.addr = NULL; > + mem->bus.offset = 0; > + mem->bus.size = mem->num_pages << PAGE_SHIFT; > + mem->bus.base = 0; > + mem->bus.is_iomem = false; > + if (!(man->flags & TTM_MEMTYPE_FLAG_MAPPABLE)) > + return -EINVAL; > + switch (mem->mem_type) { > + case TTM_PL_SYSTEM: > + /* system memory */ > + return 0; > + case TTM_PL_VRAM: > + mem->bus.offset = mem->start << PAGE_SHIFT; > + mem->bus.base = pci_resource_start(hibmc->dev->pdev, 0); > + mem->bus.is_iomem = true; > + break; > + default: > + return -EINVAL; > + } > + return 0; > +} > + > +static void hibmc_ttm_io_mem_free(struct ttm_bo_device *bdev, > + struct ttm_mem_reg *mem) > +{ > +} No need to stub this, the caller does a NULL-check before invoking > + > +static void hibmc_ttm_backend_destroy(struct ttm_tt *tt) > +{ > + ttm_tt_fini(tt); > + kfree(tt); > +} > + > +static struct ttm_backend_func hibmc_tt_backend_func = { > + .destroy = &hibmc_ttm_backend_destroy, > +}; > + > +static struct ttm_tt *hibmc_ttm_tt_create(struct ttm_bo_device *bdev, > + unsigned long size, > + u32 page_flags, > + struct page *dummy_read_page) > +{ > + struct ttm_tt *tt; > + > + tt = kzalloc(sizeof(*tt), GFP_KERNEL); > + if (!tt) Print error > + return NULL; > + tt->func = &hibmc_tt_backend_func; > + if (ttm_tt_init(tt, bdev, size, page_flags, dummy_read_page)) { Here too? > + kfree(tt); > + return NULL; > + } > + return tt; > +} > + > +static int hibmc_ttm_tt_populate(struct ttm_tt *ttm) > +{ > + return ttm_pool_populate(ttm); > +} > + > +static void hibmc_ttm_tt_unpopulate(struct ttm_tt *ttm) > +{ > + ttm_pool_unpopulate(ttm); > +} > + > +struct ttm_bo_driver hibmc_bo_driver = { > + .ttm_tt_create = hibmc_ttm_tt_create, > + .ttm_tt_populate = hibmc_ttm_tt_populate, > + .ttm_tt_unpopulate = hibmc_ttm_tt_unpopulate, > + .init_mem_type = hibmc_bo_init_mem_type, > + .evict_flags = hibmc_bo_evict_flags, > + .move = NULL, > + .verify_access = hibmc_bo_verify_access, > + .io_mem_reserve = &hibmc_ttm_io_mem_reserve, > + .io_mem_free = &hibmc_ttm_io_mem_free, > + .lru_tail = &ttm_bo_default_lru_tail, > + .swap_lru_tail = &ttm_bo_default_swap_lru_tail, > +}; > + > +int hibmc_mm_init(struct hibmc_drm_device *hibmc) > +{ > + int ret; > + struct drm_device *dev = hibmc->dev; > + struct ttm_bo_device *bdev = &hibmc->ttm.bdev; > + > + ret = hibmc_ttm_global_init(hibmc); > + if (ret) > + return ret; > + > + ret = ttm_bo_device_init(&hibmc->ttm.bdev, > + hibmc->ttm.bo_global_ref.ref.object, > + &hibmc_bo_driver, > + dev->anon_inode->i_mapping, > + DRM_FILE_PAGE_OFFSET, > + true); > + if (ret) { Call hibmc_ttm_global_release here? > + DRM_ERROR("Error initialising bo driver; %d\n", ret); > + return ret; > + } > + > + ret = ttm_bo_init_mm(bdev, TTM_PL_VRAM, > + hibmc->fb_size >> PAGE_SHIFT); > + if (ret) { Clean up here as well? > + DRM_ERROR("Failed ttm VRAM init: %d\n", ret); > + return ret; > + } > + > + hibmc->mm_inited = true; > + return 0; > +} > + > +void hibmc_mm_fini(struct hibmc_drm_device *hibmc) > +{ > + if (!hibmc->mm_inited) > + return; > + > + ttm_bo_device_release(&hibmc->ttm.bdev); > + hibmc_ttm_global_release(hibmc); > + hibmc->mm_inited = false; > +} > + > +int hibmc_bo_create(struct drm_device *dev, int size, int align, > + u32 flags, struct hibmc_bo **phibmcbo) > +{ > + struct hibmc_drm_device *hibmc = dev->dev_private; > + struct hibmc_bo *hibmcbo; > + size_t acc_size; > + int ret; > + > + hibmcbo = kzalloc(sizeof(*hibmcbo), GFP_KERNEL); > + if (!hibmcbo) > + return -ENOMEM; > + > + ret = drm_gem_object_init(dev, &hibmcbo->gem, size); > + if (ret) { > + kfree(hibmcbo); > + return ret; > + } > + > + hibmcbo->bo.bdev = &hibmc->ttm.bdev; > + > + hibmc_ttm_placement(hibmcbo, TTM_PL_FLAG_VRAM | TTM_PL_FLAG_SYSTEM); > + > + acc_size = ttm_bo_dma_acc_size(&hibmc->ttm.bdev, size, > + sizeof(struct hibmc_bo)); > + > + ret = ttm_bo_init(&hibmc->ttm.bdev, &hibmcbo->bo, size, > + ttm_bo_type_device, &hibmcbo->placement, > + align >> PAGE_SHIFT, false, NULL, acc_size, > + NULL, NULL, hibmc_bo_ttm_destroy); > + if (ret) Missing hibmcbo clean up here > + return ret; > + > + *phibmcbo = hibmcbo; > + return 0; > +} > + > +static inline u64 hibmc_bo_gpu_offset(struct hibmc_bo *bo) > +{ > + return bo->bo.offset; > +} I don't think this function provides any value > + > +int hibmc_bo_pin(struct hibmc_bo *bo, u32 pl_flag, u64 *gpu_addr) > +{ > + int i, ret; > + > + if (bo->pin_count) { > + bo->pin_count++; > + if (gpu_addr) > + *gpu_addr = hibmc_bo_gpu_offset(bo); Are you missing a return here? > + } > + > + hibmc_ttm_placement(bo, pl_flag); > + for (i = 0; i < bo->placement.num_placement; i++) > + bo->placements[i].flags |= TTM_PL_FLAG_NO_EVICT; > + ret = ttm_bo_validate(&bo->bo, &bo->placement, false, false); > + if (ret) > + return ret; > + > + bo->pin_count = 1; > + if (gpu_addr) > + *gpu_addr = hibmc_bo_gpu_offset(bo); > + return 0; > +} > + > +int hibmc_bo_push_sysram(struct hibmc_bo *bo) > +{ > + int i, ret; > + > + if (!bo->pin_count) { > + DRM_ERROR("unpin bad %p\n", bo); > + return 0; > + } > + bo->pin_count--; > + if (bo->pin_count) > + return 0; > + > + if (bo->kmap.virtual) ttm_bo_kunmap already does this check so you don't have to > + ttm_bo_kunmap(&bo->kmap); > + > + hibmc_ttm_placement(bo, TTM_PL_FLAG_SYSTEM); > + for (i = 0; i < bo->placement.num_placement ; i++) > + bo->placements[i].flags |= TTM_PL_FLAG_NO_EVICT; > + > + ret = ttm_bo_validate(&bo->bo, &bo->placement, false, false); > + if (ret) { > + DRM_ERROR("pushing to VRAM failed\n"); Print ret > + return ret; > + } > + return 0; > +} > + > +int hibmc_mmap(struct file *filp, struct vm_area_struct *vma) > +{ > + struct drm_file *file_priv; > + struct hibmc_drm_device *hibmc; > + > + if (unlikely(vma->vm_pgoff < DRM_FILE_PAGE_OFFSET)) > + return -EINVAL; > + > + file_priv = filp->private_data; > + hibmc = file_priv->minor->dev->dev_private; > + return ttm_bo_mmap(filp, vma, &hibmc->ttm.bdev); > +} > + > +int hibmc_gem_create(struct drm_device *dev, u32 size, bool iskernel, > + struct drm_gem_object **obj) > +{ > + struct hibmc_bo *hibmcbo; > + int ret; > + > + *obj = NULL; > + > + size = PAGE_ALIGN(size); > + if (size == 0) Print error > + return -EINVAL; > + > + ret = hibmc_bo_create(dev, size, 0, 0, &hibmcbo); > + if (ret) { > + if (ret != -ERESTARTSYS) > + DRM_ERROR("failed to allocate GEM object\n"); Print ret > + return ret; > + } > + *obj = &hibmcbo->gem; > + return 0; > +} > + > +int hibmc_dumb_create(struct drm_file *file, struct drm_device *dev, > + struct drm_mode_create_dumb *args) > +{ > + struct drm_gem_object *gobj; > + u32 handle; > + int ret; > + > + args->pitch = ALIGN(args->width * ((args->bpp + 7) / 8), 16); What's up with the bpp + 7 here? Perhaps you're looking for DIV_ROUND_UP? > + args->size = args->pitch * args->height; > + > + ret = hibmc_gem_create(dev, args->size, false, > + &gobj); > + if (ret) > + return ret; > + > + ret = drm_gem_handle_create(file, gobj, &handle); > + drm_gem_object_unreference_unlocked(gobj); > + if (ret) Print error here > + return ret; > + > + args->handle = handle; > + return 0; > +} > + > +static void hibmc_bo_unref(struct hibmc_bo **bo) > +{ > + struct ttm_buffer_object *tbo; > + > + if ((*bo) == NULL) > + return; > + > + tbo = &((*bo)->bo); > + ttm_bo_unref(&tbo); > + *bo = NULL; > +} > + > +void hibmc_gem_free_object(struct drm_gem_object *obj) > +{ > + struct hibmc_bo *hibmcbo = gem_to_hibmc_bo(obj); > + > + hibmc_bo_unref(&hibmcbo); > +} > + > +static u64 hibmc_bo_mmap_offset(struct hibmc_bo *bo) > +{ > + return drm_vma_node_offset_addr(&bo->bo.vma_node); > +} > + > +int hibmc_dumb_mmap_offset(struct drm_file *file, struct drm_device *dev, > + u32 handle, u64 *offset) > +{ > + struct drm_gem_object *obj; > + struct hibmc_bo *bo; > + > + obj = drm_gem_object_lookup(file, handle); > + if (!obj) > + return -ENOENT; > + > + bo = gem_to_hibmc_bo(obj); > + *offset = hibmc_bo_mmap_offset(bo); > + > + drm_gem_object_unreference_unlocked(obj); > + return 0; > +} > -- > 1.9.1 > > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
在 2016/11/11 1:35, Sean Paul 写道: > On Fri, Oct 28, 2016 at 3:27 AM, Rongrong Zou <zourongrong@gmail.com> wrote: >> Hibmc have 32m video memory which can be accessed through PCIe by host, >> we use ttm to manage these memory. >> >> Signed-off-by: Rongrong Zou <zourongrong@gmail.com> >> --- >> drivers/gpu/drm/hisilicon/hibmc/Kconfig | 1 + >> drivers/gpu/drm/hisilicon/hibmc/Makefile | 2 +- >> drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c | 12 + >> drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.h | 46 +++ >> drivers/gpu/drm/hisilicon/hibmc/hibmc_ttm.c | 490 ++++++++++++++++++++++++ >> 5 files changed, 550 insertions(+), 1 deletion(-) >> create mode 100644 drivers/gpu/drm/hisilicon/hibmc/hibmc_ttm.c >> >> diff --git a/drivers/gpu/drm/hisilicon/hibmc/Kconfig b/drivers/gpu/drm/hisilicon/hibmc/Kconfig >> index a9af90d..bcb8c18 100644 >> --- a/drivers/gpu/drm/hisilicon/hibmc/Kconfig >> +++ b/drivers/gpu/drm/hisilicon/hibmc/Kconfig >> @@ -1,6 +1,7 @@ >> config DRM_HISI_HIBMC >> tristate "DRM Support for Hisilicon Hibmc" >> depends on DRM && PCI >> + select DRM_TTM >> >> help >> Choose this option if you have a Hisilicon Hibmc soc chipset. >> diff --git a/drivers/gpu/drm/hisilicon/hibmc/Makefile b/drivers/gpu/drm/hisilicon/hibmc/Makefile >> index 97cf4a0..d5c40b8 100644 >> --- a/drivers/gpu/drm/hisilicon/hibmc/Makefile >> +++ b/drivers/gpu/drm/hisilicon/hibmc/Makefile >> @@ -1,5 +1,5 @@ >> ccflags-y := -Iinclude/drm >> -hibmc-drm-y := hibmc_drm_drv.o hibmc_drm_power.o >> +hibmc-drm-y := hibmc_drm_drv.o hibmc_drm_power.o hibmc_ttm.o >> >> obj-$(CONFIG_DRM_HISI_HIBMC) +=hibmc-drm.o >> #obj-y += hibmc-drm.o >> diff --git a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c >> index 4669d42..81f4301 100644 >> --- a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c >> +++ b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c >> @@ -31,6 +31,7 @@ >> #ifdef CONFIG_COMPAT >> .compat_ioctl = drm_compat_ioctl, >> #endif >> + .mmap = hibmc_mmap, >> .poll = drm_poll, >> .read = drm_read, >> .llseek = no_llseek, >> @@ -46,6 +47,8 @@ static void hibmc_disable_vblank(struct drm_device *dev, unsigned int pipe) >> } >> >> static struct drm_driver hibmc_driver = { >> + .driver_features = DRIVER_GEM, >> + > > nit: extra space > >> .fops = &hibmc_fops, >> .name = "hibmc", >> .date = "20160828", >> @@ -55,6 +58,10 @@ static void hibmc_disable_vblank(struct drm_device *dev, unsigned int pipe) >> .get_vblank_counter = drm_vblank_no_hw_counter, >> .enable_vblank = hibmc_enable_vblank, >> .disable_vblank = hibmc_disable_vblank, >> + .gem_free_object_unlocked = hibmc_gem_free_object, >> + .dumb_create = hibmc_dumb_create, >> + .dumb_map_offset = hibmc_dumb_mmap_offset, >> + .dumb_destroy = drm_gem_dumb_destroy, >> }; >> >> static int hibmc_pm_suspend(struct device *dev) >> @@ -163,6 +170,7 @@ static int hibmc_unload(struct drm_device *dev) >> { >> struct hibmc_drm_device *hidev = dev->dev_private; >> >> + hibmc_mm_fini(hidev); >> hibmc_hw_fini(hidev); >> dev->dev_private = NULL; >> return 0; >> @@ -183,6 +191,10 @@ static int hibmc_load(struct drm_device *dev, unsigned long flags) >> if (ret) >> goto err; >> >> + ret = hibmc_mm_init(hidev); >> + if (ret) >> + goto err; >> + >> return 0; >> >> err: >> diff --git a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.h b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.h >> index 0037341..db8d80e 100644 >> --- a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.h >> +++ b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.h >> @@ -20,6 +20,8 @@ >> #define HIBMC_DRM_DRV_H >> >> #include <drm/drmP.h> >> +#include <drm/ttm/ttm_bo_driver.h> >> +#include <drm/drm_gem.h> > > nit: alphabetize will fix it, thanks. > >> >> struct hibmc_drm_device { >> /* hw */ >> @@ -30,6 +32,50 @@ struct hibmc_drm_device { >> >> /* drm */ >> struct drm_device *dev; >> + >> + /* ttm */ >> + struct { >> + struct drm_global_reference mem_global_ref; >> + struct ttm_bo_global_ref bo_global_ref; >> + struct ttm_bo_device bdev; >> + bool initialized; >> + } ttm; > > I don't think you gain anything other than keystrokes from the substruct I'm sorry i didn't catch you, i looked at the all drivers used ttm such as ast/bochs/cirrus/mgag200/qxl/virtio_gpu, they all embedded the ttm substruct into the driver-private struct. so do you mean struct hibmc_drm_device { /* hw */ void __iomem *mmio; void __iomem *fb_map; unsigned long fb_base; unsigned long fb_size; /* drm */ struct drm_device *dev; struct drm_plane plane; struct drm_crtc crtc; struct drm_encoder encoder; struct drm_connector connector; bool mode_config_initialized; /* ttm */ struct drm_global_reference mem_global_ref; struct ttm_bo_global_ref bo_global_ref; struct ttm_bo_device bdev; bool initialized; ... }; ? > >> + >> + bool mm_inited; >> }; >> >> +struct hibmc_bo { >> + struct ttm_buffer_object bo; >> + struct ttm_placement placement; >> + struct ttm_bo_kmap_obj kmap; >> + struct drm_gem_object gem; >> + struct ttm_place placements[3]; >> + int pin_count; >> +}; >> + >> +static inline struct hibmc_bo *hibmc_bo(struct ttm_buffer_object *bo) >> +{ >> + return container_of(bo, struct hibmc_bo, bo); >> +} >> + >> +static inline struct hibmc_bo *gem_to_hibmc_bo(struct drm_gem_object *gem) >> +{ >> + return container_of(gem, struct hibmc_bo, gem); >> +} >> + >> +#define DRM_FILE_PAGE_OFFSET (0x100000000ULL >> PAGE_SHIFT) > > Hide this in ttm.c ok, will do that. thanks for pointing it out. > >> + >> +int hibmc_gem_create(struct drm_device *dev, u32 size, bool iskernel, >> + struct drm_gem_object **obj); >> + >> +int hibmc_mm_init(struct hibmc_drm_device *hibmc); >> +void hibmc_mm_fini(struct hibmc_drm_device *hibmc); >> +int hibmc_bo_pin(struct hibmc_bo *bo, u32 pl_flag, u64 *gpu_addr); >> +void hibmc_gem_free_object(struct drm_gem_object *obj); >> +int hibmc_dumb_create(struct drm_file *file, struct drm_device *dev, >> + struct drm_mode_create_dumb *args); >> +int hibmc_dumb_mmap_offset(struct drm_file *file, struct drm_device *dev, >> + u32 handle, u64 *offset); >> +int hibmc_mmap(struct file *filp, struct vm_area_struct *vma); >> + >> #endif >> diff --git a/drivers/gpu/drm/hisilicon/hibmc/hibmc_ttm.c b/drivers/gpu/drm/hisilicon/hibmc/hibmc_ttm.c >> new file mode 100644 >> index 0000000..0802ebd >> --- /dev/null >> +++ b/drivers/gpu/drm/hisilicon/hibmc/hibmc_ttm.c >> @@ -0,0 +1,490 @@ >> +/* Hisilicon Hibmc SoC drm driver >> + * >> + * Based on the bochs drm driver. >> + * >> + * Copyright (c) 2016 Huawei Limited. >> + * >> + * Author: >> + * Rongrong Zou <zourongrong@huawei.com> >> + * Rongrong Zou <zourongrong@gmail.com> >> + * Jianhua Li <lijianhua@huawei.com> >> + * >> + * This program is free software; you can redistribute it and/or modify >> + * it under the terms of the GNU General Public License as published by >> + * the Free Software Foundation; either version 2 of the License, or >> + * (at your option) any later version. >> + * >> + */ >> + >> +#include "hibmc_drm_drv.h" >> +#include <ttm/ttm_page_alloc.h> >> +#include <drm/drm_crtc_helper.h> >> +#include <drm/drm_atomic_helper.h> >> + >> +static inline struct hibmc_drm_device * >> +hibmc_bdev(struct ttm_bo_device *bd) >> +{ >> + return container_of(bd, struct hibmc_drm_device, ttm.bdev); >> +} >> + >> +static int >> +hibmc_ttm_mem_global_init(struct drm_global_reference *ref) >> +{ >> + return ttm_mem_global_init(ref->object); >> +} >> + >> +static void >> +hibmc_ttm_mem_global_release(struct drm_global_reference *ref) >> +{ >> + ttm_mem_global_release(ref->object); >> +} >> + >> +static int hibmc_ttm_global_init(struct hibmc_drm_device *hibmc) >> +{ >> + struct drm_global_reference *global_ref; >> + int r; > > nit: try not to use one character variable names unless it's for the > purpose of a loop (ie: i,j). You also use ret elsewhere in the driver, > so it'd be nice to remain consistent the whole file is delivered from bochs ttm, i didn't modify anything except some checkpatch warnings and the 'hibmc_' prefix. Unfortunately, some problems were delivered too. > >> + >> + global_ref = &hibmc->ttm.mem_global_ref; > > I think using the global_ref local obfuscates what you're doing here. > It saves you 6 characters while typing, but adds a layer of > indirection for all future readers. > >> + global_ref->global_type = DRM_GLOBAL_TTM_MEM; >> + global_ref->size = sizeof(struct ttm_mem_global); >> + global_ref->init = &hibmc_ttm_mem_global_init; >> + global_ref->release = &hibmc_ttm_mem_global_release; >> + r = drm_global_item_ref(global_ref); >> + if (r != 0) { > > nit: if (r) will fix it, thanks. BTW, i wonder why checkpatch.pl didn't report it. > >> + DRM_ERROR("Failed setting up TTM memory accounting subsystem.\n" >> + ); > > Breaking up the line for one character is probably not worthwhile, and > you should really print the error. How about: > > DRM_ERROR("Could not get ref on ttm global ret=%d.\n", ret); i like your solution, thanks. > > >> + return r; >> + } >> + >> + hibmc->ttm.bo_global_ref.mem_glob = >> + hibmc->ttm.mem_global_ref.object; >> + global_ref = &hibmc->ttm.bo_global_ref.ref; >> + global_ref->global_type = DRM_GLOBAL_TTM_BO; >> + global_ref->size = sizeof(struct ttm_bo_global); >> + global_ref->init = &ttm_bo_global_init; >> + global_ref->release = &ttm_bo_global_release; >> + r = drm_global_item_ref(global_ref); >> + if (r != 0) { >> + DRM_ERROR("Failed setting up TTM BO subsystem.\n"); >> + drm_global_item_unref(&hibmc->ttm.mem_global_ref); >> + return r; >> + } >> + return 0; >> +} >> + >> +static void >> +hibmc_ttm_global_release(struct hibmc_drm_device *hibmc) >> +{ >> + if (!hibmc->ttm.mem_global_ref.release) > > Are you actually hitting this condition? This seems like it's papering > over something else. it was also delivered from others, i looked at the xxx_ttm_global_init function, 'mem_global_ref.release' is assigned unconditionally, so i think this condition never be hit, it may be hit when release twice, but this won't take place in my driver. > >> + return; >> + >> + drm_global_item_unref(&hibmc->ttm.bo_global_ref.ref); >> + drm_global_item_unref(&hibmc->ttm.mem_global_ref); >> + hibmc->ttm.mem_global_ref.release = NULL; >> +} >> + >> +static void hibmc_bo_ttm_destroy(struct ttm_buffer_object *tbo) >> +{ >> + struct hibmc_bo *bo; >> + >> + bo = container_of(tbo, struct hibmc_bo, bo); > > nit: No need to split this into a separate line. agreed, thanks. > >> + >> + drm_gem_object_release(&bo->gem); >> + kfree(bo); >> +} >> + >> +static bool hibmc_ttm_bo_is_hibmc_bo(struct ttm_buffer_object *bo) >> +{ >> + if (bo->destroy == &hibmc_bo_ttm_destroy) >> + return true; >> + return false; > > return bo->destroy == &hibmc_bo_ttm_destroy; looks better to me. > >> +} >> + >> +static int >> +hibmc_bo_init_mem_type(struct ttm_bo_device *bdev, u32 type, >> + struct ttm_mem_type_manager *man) >> +{ >> + switch (type) { >> + case TTM_PL_SYSTEM: >> + man->flags = TTM_MEMTYPE_FLAG_MAPPABLE; >> + man->available_caching = TTM_PL_MASK_CACHING; >> + man->default_caching = TTM_PL_FLAG_CACHED; >> + break; >> + case TTM_PL_VRAM: >> + man->func = &ttm_bo_manager_func; >> + man->flags = TTM_MEMTYPE_FLAG_FIXED | >> + TTM_MEMTYPE_FLAG_MAPPABLE; >> + man->available_caching = TTM_PL_FLAG_UNCACHED | >> + TTM_PL_FLAG_WC; >> + man->default_caching = TTM_PL_FLAG_WC; >> + break; >> + default: >> + DRM_ERROR("Unsupported memory type %u\n", type); >> + return -EINVAL; >> + } >> + return 0; >> +} >> + >> +void hibmc_ttm_placement(struct hibmc_bo *bo, int domain) >> +{ >> + u32 c = 0; > > Can you please use a more descriptive name than 'c'? ok, will do that. > >> + u32 i; >> + >> + bo->placement.placement = bo->placements; >> + bo->placement.busy_placement = bo->placements; >> + if (domain & TTM_PL_FLAG_VRAM) >> + bo->placements[c++].flags = TTM_PL_FLAG_WC | >> + TTM_PL_FLAG_UNCACHED | TTM_PL_FLAG_VRAM; > > nit: you're alignment is off here and below is it correct? if (domain & TTM_PL_FLAG_VRAM) bo->placements[c++].flags = TTM_PL_FLAG_WC | TTM_PL_FLAG_UNCACHED | TTM_PL_FLAG_VRAM; if (domain & TTM_PL_FLAG_SYSTEM) bo->placements[c++].flags = TTM_PL_MASK_CACHING | TTM_PL_FLAG_SYSTEM; if (!c) bo->placements[c++].flags = TTM_PL_MASK_CACHING | TTM_PL_FLAG_SYSTEM; > >> + if (domain & TTM_PL_FLAG_SYSTEM) >> + bo->placements[c++].flags = TTM_PL_MASK_CACHING | >> + TTM_PL_FLAG_SYSTEM; >> + if (!c) >> + bo->placements[c++].flags = TTM_PL_MASK_CACHING | >> + TTM_PL_FLAG_SYSTEM; >> + >> + bo->placement.num_placement = c; >> + bo->placement.num_busy_placement = c; >> + for (i = 0; i < c; ++i) { > > nit: we tend towards post-increment in kernel agreed, thanks. > >> + bo->placements[i].fpfn = 0; >> + bo->placements[i].lpfn = 0; >> + } >> +} >> + >> +static void >> +hibmc_bo_evict_flags(struct ttm_buffer_object *bo, struct ttm_placement *pl) >> +{ >> + struct hibmc_bo *hibmcbo = hibmc_bo(bo); >> + >> + if (!hibmc_ttm_bo_is_hibmc_bo(bo)) >> + return; >> + >> + hibmc_ttm_placement(hibmcbo, TTM_PL_FLAG_SYSTEM); >> + *pl = hibmcbo->placement; >> +} >> + >> +static int hibmc_bo_verify_access(struct ttm_buffer_object *bo, >> + struct file *filp) >> +{ >> + struct hibmc_bo *hibmcbo = hibmc_bo(bo); >> + >> + return drm_vma_node_verify_access(&hibmcbo->gem.vma_node, >> + filp->private_data); >> +} >> + >> +static int hibmc_ttm_io_mem_reserve(struct ttm_bo_device *bdev, >> + struct ttm_mem_reg *mem) >> +{ >> + struct ttm_mem_type_manager *man = &bdev->man[mem->mem_type]; >> + struct hibmc_drm_device *hibmc = hibmc_bdev(bdev); >> + >> + mem->bus.addr = NULL; >> + mem->bus.offset = 0; >> + mem->bus.size = mem->num_pages << PAGE_SHIFT; >> + mem->bus.base = 0; >> + mem->bus.is_iomem = false; >> + if (!(man->flags & TTM_MEMTYPE_FLAG_MAPPABLE)) >> + return -EINVAL; >> + switch (mem->mem_type) { >> + case TTM_PL_SYSTEM: >> + /* system memory */ >> + return 0; >> + case TTM_PL_VRAM: >> + mem->bus.offset = mem->start << PAGE_SHIFT; >> + mem->bus.base = pci_resource_start(hibmc->dev->pdev, 0); >> + mem->bus.is_iomem = true; >> + break; >> + default: >> + return -EINVAL; >> + } >> + return 0; >> +} >> + >> +static void hibmc_ttm_io_mem_free(struct ttm_bo_device *bdev, >> + struct ttm_mem_reg *mem) >> +{ >> +} > > No need to stub this, the caller does a NULL-check before invoking will delete it, thanks. > >> + >> +static void hibmc_ttm_backend_destroy(struct ttm_tt *tt) >> +{ >> + ttm_tt_fini(tt); >> + kfree(tt); >> +} >> + >> +static struct ttm_backend_func hibmc_tt_backend_func = { >> + .destroy = &hibmc_ttm_backend_destroy, >> +}; >> + >> +static struct ttm_tt *hibmc_ttm_tt_create(struct ttm_bo_device *bdev, >> + unsigned long size, >> + u32 page_flags, >> + struct page *dummy_read_page) >> +{ >> + struct ttm_tt *tt; >> + >> + tt = kzalloc(sizeof(*tt), GFP_KERNEL); >> + if (!tt) > > Print error ok, will do that, thanks. > >> + return NULL; >> + tt->func = &hibmc_tt_backend_func; >> + if (ttm_tt_init(tt, bdev, size, page_flags, dummy_read_page)) { > > Here too? ditto > >> + kfree(tt); >> + return NULL; >> + } >> + return tt; >> +} >> + >> +static int hibmc_ttm_tt_populate(struct ttm_tt *ttm) >> +{ >> + return ttm_pool_populate(ttm); >> +} >> + >> +static void hibmc_ttm_tt_unpopulate(struct ttm_tt *ttm) >> +{ >> + ttm_pool_unpopulate(ttm); >> +} >> + >> +struct ttm_bo_driver hibmc_bo_driver = { >> + .ttm_tt_create = hibmc_ttm_tt_create, >> + .ttm_tt_populate = hibmc_ttm_tt_populate, >> + .ttm_tt_unpopulate = hibmc_ttm_tt_unpopulate, >> + .init_mem_type = hibmc_bo_init_mem_type, >> + .evict_flags = hibmc_bo_evict_flags, >> + .move = NULL, >> + .verify_access = hibmc_bo_verify_access, >> + .io_mem_reserve = &hibmc_ttm_io_mem_reserve, >> + .io_mem_free = &hibmc_ttm_io_mem_free, >> + .lru_tail = &ttm_bo_default_lru_tail, >> + .swap_lru_tail = &ttm_bo_default_swap_lru_tail, >> +}; >> + >> +int hibmc_mm_init(struct hibmc_drm_device *hibmc) >> +{ >> + int ret; >> + struct drm_device *dev = hibmc->dev; >> + struct ttm_bo_device *bdev = &hibmc->ttm.bdev; >> + >> + ret = hibmc_ttm_global_init(hibmc); >> + if (ret) >> + return ret; >> + >> + ret = ttm_bo_device_init(&hibmc->ttm.bdev, >> + hibmc->ttm.bo_global_ref.ref.object, >> + &hibmc_bo_driver, >> + dev->anon_inode->i_mapping, >> + DRM_FILE_PAGE_OFFSET, >> + true); >> + if (ret) { > > Call hibmc_ttm_global_release here? agreed, thanks for pointing it out. > >> + DRM_ERROR("Error initialising bo driver; %d\n", ret); >> + return ret; >> + } >> + >> + ret = ttm_bo_init_mm(bdev, TTM_PL_VRAM, >> + hibmc->fb_size >> PAGE_SHIFT); >> + if (ret) { > > Clean up here as well? ditto > >> + DRM_ERROR("Failed ttm VRAM init: %d\n", ret); >> + return ret; >> + } >> + >> + hibmc->mm_inited = true; >> + return 0; >> +} >> + >> +void hibmc_mm_fini(struct hibmc_drm_device *hibmc) >> +{ >> + if (!hibmc->mm_inited) >> + return; >> + >> + ttm_bo_device_release(&hibmc->ttm.bdev); >> + hibmc_ttm_global_release(hibmc); >> + hibmc->mm_inited = false; >> +} >> + >> +int hibmc_bo_create(struct drm_device *dev, int size, int align, >> + u32 flags, struct hibmc_bo **phibmcbo) >> +{ >> + struct hibmc_drm_device *hibmc = dev->dev_private; >> + struct hibmc_bo *hibmcbo; >> + size_t acc_size; >> + int ret; >> + >> + hibmcbo = kzalloc(sizeof(*hibmcbo), GFP_KERNEL); >> + if (!hibmcbo) >> + return -ENOMEM; >> + >> + ret = drm_gem_object_init(dev, &hibmcbo->gem, size); >> + if (ret) { >> + kfree(hibmcbo); >> + return ret; >> + } >> + >> + hibmcbo->bo.bdev = &hibmc->ttm.bdev; >> + >> + hibmc_ttm_placement(hibmcbo, TTM_PL_FLAG_VRAM | TTM_PL_FLAG_SYSTEM); >> + >> + acc_size = ttm_bo_dma_acc_size(&hibmc->ttm.bdev, size, >> + sizeof(struct hibmc_bo)); >> + >> + ret = ttm_bo_init(&hibmc->ttm.bdev, &hibmcbo->bo, size, >> + ttm_bo_type_device, &hibmcbo->placement, >> + align >> PAGE_SHIFT, false, NULL, acc_size, >> + NULL, NULL, hibmc_bo_ttm_destroy); >> + if (ret) > > Missing hibmcbo clean up here i looked at all other ttm drivers and all of them return directly when ttm_bo_init failed, however, i think it is better to clean up here, should i call hibmc_bo_unref(&hibmc_bo) here ? > >> + return ret; >> + >> + *phibmcbo = hibmcbo; >> + return 0; >> +} >> + >> +static inline u64 hibmc_bo_gpu_offset(struct hibmc_bo *bo) >> +{ >> + return bo->bo.offset; >> +} > > I don't think this function provides any value do you nean i use bo->bo.offset instead of calling hibmc_bo_gpu_offset()? > >> + >> +int hibmc_bo_pin(struct hibmc_bo *bo, u32 pl_flag, u64 *gpu_addr) >> +{ >> + int i, ret; >> + >> + if (bo->pin_count) { >> + bo->pin_count++; >> + if (gpu_addr) >> + *gpu_addr = hibmc_bo_gpu_offset(bo); > > Are you missing a return here? Thanks for pointing it out! > >> + } >> + >> + hibmc_ttm_placement(bo, pl_flag); >> + for (i = 0; i < bo->placement.num_placement; i++) >> + bo->placements[i].flags |= TTM_PL_FLAG_NO_EVICT; >> + ret = ttm_bo_validate(&bo->bo, &bo->placement, false, false); >> + if (ret) >> + return ret; >> + >> + bo->pin_count = 1; >> + if (gpu_addr) >> + *gpu_addr = hibmc_bo_gpu_offset(bo); >> + return 0; >> +} >> + >> +int hibmc_bo_push_sysram(struct hibmc_bo *bo) >> +{ >> + int i, ret; >> + >> + if (!bo->pin_count) { >> + DRM_ERROR("unpin bad %p\n", bo); >> + return 0; >> + } >> + bo->pin_count--; >> + if (bo->pin_count) >> + return 0; >> + >> + if (bo->kmap.virtual) > > ttm_bo_kunmap already does this check so you don't have to agreed. will remove this condition. > >> + ttm_bo_kunmap(&bo->kmap); >> + >> + hibmc_ttm_placement(bo, TTM_PL_FLAG_SYSTEM); >> + for (i = 0; i < bo->placement.num_placement ; i++) >> + bo->placements[i].flags |= TTM_PL_FLAG_NO_EVICT; >> + >> + ret = ttm_bo_validate(&bo->bo, &bo->placement, false, false); >> + if (ret) { >> + DRM_ERROR("pushing to VRAM failed\n"); > > Print ret ok, thanks. > >> + return ret; >> + } >> + return 0; >> +} >> + >> +int hibmc_mmap(struct file *filp, struct vm_area_struct *vma) >> +{ >> + struct drm_file *file_priv; >> + struct hibmc_drm_device *hibmc; >> + >> + if (unlikely(vma->vm_pgoff < DRM_FILE_PAGE_OFFSET)) >> + return -EINVAL; >> + >> + file_priv = filp->private_data; >> + hibmc = file_priv->minor->dev->dev_private; >> + return ttm_bo_mmap(filp, vma, &hibmc->ttm.bdev); >> +} >> + >> +int hibmc_gem_create(struct drm_device *dev, u32 size, bool iskernel, >> + struct drm_gem_object **obj) >> +{ >> + struct hibmc_bo *hibmcbo; >> + int ret; >> + >> + *obj = NULL; >> + >> + size = PAGE_ALIGN(size); >> + if (size == 0) > > Print error ditto > >> + return -EINVAL; >> + >> + ret = hibmc_bo_create(dev, size, 0, 0, &hibmcbo); >> + if (ret) { >> + if (ret != -ERESTARTSYS) >> + DRM_ERROR("failed to allocate GEM object\n"); > > Print ret ditto > >> + return ret; >> + } >> + *obj = &hibmcbo->gem; >> + return 0; >> +} >> + >> +int hibmc_dumb_create(struct drm_file *file, struct drm_device *dev, >> + struct drm_mode_create_dumb *args) >> +{ >> + struct drm_gem_object *gobj; >> + u32 handle; >> + int ret; >> + >> + args->pitch = ALIGN(args->width * ((args->bpp + 7) / 8), 16); > > What's up with the bpp + 7 here? Perhaps you're looking for DIV_ROUND_UP? Yes, that sounds sane. > > >> + args->size = args->pitch * args->height; >> + >> + ret = hibmc_gem_create(dev, args->size, false, >> + &gobj); >> + if (ret) >> + return ret; >> + >> + ret = drm_gem_handle_create(file, gobj, &handle); >> + drm_gem_object_unreference_unlocked(gobj); >> + if (ret) > > Print error here agreed. > >> + return ret; >> + >> + args->handle = handle; >> + return 0; >> +} >> + >> +static void hibmc_bo_unref(struct hibmc_bo **bo) >> +{ >> + struct ttm_buffer_object *tbo; >> + >> + if ((*bo) == NULL) >> + return; >> + >> + tbo = &((*bo)->bo); >> + ttm_bo_unref(&tbo); >> + *bo = NULL; >> +} >> + >> +void hibmc_gem_free_object(struct drm_gem_object *obj) >> +{ >> + struct hibmc_bo *hibmcbo = gem_to_hibmc_bo(obj); >> + >> + hibmc_bo_unref(&hibmcbo); >> +} >> + >> +static u64 hibmc_bo_mmap_offset(struct hibmc_bo *bo) >> +{ >> + return drm_vma_node_offset_addr(&bo->bo.vma_node); >> +} >> + >> +int hibmc_dumb_mmap_offset(struct drm_file *file, struct drm_device *dev, >> + u32 handle, u64 *offset) >> +{ >> + struct drm_gem_object *obj; >> + struct hibmc_bo *bo; >> + >> + obj = drm_gem_object_lookup(file, handle); >> + if (!obj) >> + return -ENOENT; >> + >> + bo = gem_to_hibmc_bo(obj); >> + *offset = hibmc_bo_mmap_offset(bo); >> + >> + drm_gem_object_unreference_unlocked(obj); >> + return 0; >> +} Regards, Rongrong. >> -- >> 1.9.1 >> >> >> _______________________________________________ >> linux-arm-kernel mailing list >> linux-arm-kernel@lists.infradead.org >> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel > _______________________________________________ > linuxarm mailing list > linuxarm@huawei.com > http://rnd-openeuler.huawei.com/mailman/listinfo/linuxarm > > . >
On Fri, Nov 11, 2016 at 6:16 AM, Rongrong Zou <zourongrong@huawei.com> wrote: > 在 2016/11/11 1:35, Sean Paul 写道: >> >> On Fri, Oct 28, 2016 at 3:27 AM, Rongrong Zou <zourongrong@gmail.com> >> wrote: >>> >>> Hibmc have 32m video memory which can be accessed through PCIe by host, >>> we use ttm to manage these memory. >>> >>> Signed-off-by: Rongrong Zou <zourongrong@gmail.com> >>> --- >>> drivers/gpu/drm/hisilicon/hibmc/Kconfig | 1 + >>> drivers/gpu/drm/hisilicon/hibmc/Makefile | 2 +- >>> drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c | 12 + >>> drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.h | 46 +++ >>> drivers/gpu/drm/hisilicon/hibmc/hibmc_ttm.c | 490 >>> ++++++++++++++++++++++++ >>> 5 files changed, 550 insertions(+), 1 deletion(-) >>> create mode 100644 drivers/gpu/drm/hisilicon/hibmc/hibmc_ttm.c >>> >>> diff --git a/drivers/gpu/drm/hisilicon/hibmc/Kconfig >>> b/drivers/gpu/drm/hisilicon/hibmc/Kconfig >>> index a9af90d..bcb8c18 100644 >>> --- a/drivers/gpu/drm/hisilicon/hibmc/Kconfig >>> +++ b/drivers/gpu/drm/hisilicon/hibmc/Kconfig >>> @@ -1,6 +1,7 @@ >>> config DRM_HISI_HIBMC >>> tristate "DRM Support for Hisilicon Hibmc" >>> depends on DRM && PCI >>> + select DRM_TTM >>> >>> help >>> Choose this option if you have a Hisilicon Hibmc soc chipset. >>> diff --git a/drivers/gpu/drm/hisilicon/hibmc/Makefile >>> b/drivers/gpu/drm/hisilicon/hibmc/Makefile >>> index 97cf4a0..d5c40b8 100644 >>> --- a/drivers/gpu/drm/hisilicon/hibmc/Makefile >>> +++ b/drivers/gpu/drm/hisilicon/hibmc/Makefile >>> @@ -1,5 +1,5 @@ >>> ccflags-y := -Iinclude/drm >>> -hibmc-drm-y := hibmc_drm_drv.o hibmc_drm_power.o >>> +hibmc-drm-y := hibmc_drm_drv.o hibmc_drm_power.o hibmc_ttm.o >>> >>> obj-$(CONFIG_DRM_HISI_HIBMC) +=hibmc-drm.o >>> #obj-y += hibmc-drm.o >>> diff --git a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c >>> b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c >>> index 4669d42..81f4301 100644 >>> --- a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c >>> +++ b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c >>> @@ -31,6 +31,7 @@ >>> #ifdef CONFIG_COMPAT >>> .compat_ioctl = drm_compat_ioctl, >>> #endif >>> + .mmap = hibmc_mmap, >>> .poll = drm_poll, >>> .read = drm_read, >>> .llseek = no_llseek, >>> @@ -46,6 +47,8 @@ static void hibmc_disable_vblank(struct drm_device >>> *dev, unsigned int pipe) >>> } >>> >>> static struct drm_driver hibmc_driver = { >>> + .driver_features = DRIVER_GEM, >>> + >> >> >> nit: extra space >> >>> .fops = &hibmc_fops, >>> .name = "hibmc", >>> .date = "20160828", >>> @@ -55,6 +58,10 @@ static void hibmc_disable_vblank(struct drm_device >>> *dev, unsigned int pipe) >>> .get_vblank_counter = drm_vblank_no_hw_counter, >>> .enable_vblank = hibmc_enable_vblank, >>> .disable_vblank = hibmc_disable_vblank, >>> + .gem_free_object_unlocked = hibmc_gem_free_object, >>> + .dumb_create = hibmc_dumb_create, >>> + .dumb_map_offset = hibmc_dumb_mmap_offset, >>> + .dumb_destroy = drm_gem_dumb_destroy, >>> }; >>> >>> static int hibmc_pm_suspend(struct device *dev) >>> @@ -163,6 +170,7 @@ static int hibmc_unload(struct drm_device *dev) >>> { >>> struct hibmc_drm_device *hidev = dev->dev_private; >>> >>> + hibmc_mm_fini(hidev); >>> hibmc_hw_fini(hidev); >>> dev->dev_private = NULL; >>> return 0; >>> @@ -183,6 +191,10 @@ static int hibmc_load(struct drm_device *dev, >>> unsigned long flags) >>> if (ret) >>> goto err; >>> >>> + ret = hibmc_mm_init(hidev); >>> + if (ret) >>> + goto err; >>> + >>> return 0; >>> >>> err: >>> diff --git a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.h >>> b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.h >>> index 0037341..db8d80e 100644 >>> --- a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.h >>> +++ b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.h >>> @@ -20,6 +20,8 @@ >>> #define HIBMC_DRM_DRV_H >>> >>> #include <drm/drmP.h> >>> +#include <drm/ttm/ttm_bo_driver.h> >>> +#include <drm/drm_gem.h> >> >> >> nit: alphabetize > > > will fix it, thanks. > >> >>> >>> struct hibmc_drm_device { >>> /* hw */ >>> @@ -30,6 +32,50 @@ struct hibmc_drm_device { >>> >>> /* drm */ >>> struct drm_device *dev; >>> + >>> + /* ttm */ >>> + struct { >>> + struct drm_global_reference mem_global_ref; >>> + struct ttm_bo_global_ref bo_global_ref; >>> + struct ttm_bo_device bdev; >>> + bool initialized; >>> + } ttm; >> >> >> I don't think you gain anything other than keystrokes from the substruct > > > I'm sorry i didn't catch you, i looked at the all drivers used ttm such > as ast/bochs/cirrus/mgag200/qxl/virtio_gpu, they all embedded the ttm > substruct > into the driver-private struct. > > so do you mean > struct hibmc_drm_device { > /* hw */ > void __iomem *mmio; > void __iomem *fb_map; > unsigned long fb_base; > unsigned long fb_size; > > /* drm */ > struct drm_device *dev; > struct drm_plane plane; > struct drm_crtc crtc; > struct drm_encoder encoder; > struct drm_connector connector; > bool mode_config_initialized; > > /* ttm */ > struct drm_global_reference mem_global_ref; > struct ttm_bo_global_ref bo_global_ref; > struct ttm_bo_device bdev; > bool initialized; > ... > }; > ? Yeah, that's what I was thinking > >> >>> + >>> + bool mm_inited; >>> }; >>> >>> +struct hibmc_bo { >>> + struct ttm_buffer_object bo; >>> + struct ttm_placement placement; >>> + struct ttm_bo_kmap_obj kmap; >>> + struct drm_gem_object gem; >>> + struct ttm_place placements[3]; >>> + int pin_count; >>> +}; >>> + >>> +static inline struct hibmc_bo *hibmc_bo(struct ttm_buffer_object *bo) >>> +{ >>> + return container_of(bo, struct hibmc_bo, bo); >>> +} >>> + >>> +static inline struct hibmc_bo *gem_to_hibmc_bo(struct drm_gem_object >>> *gem) >>> +{ >>> + return container_of(gem, struct hibmc_bo, gem); >>> +} >>> + >>> +#define DRM_FILE_PAGE_OFFSET (0x100000000ULL >> PAGE_SHIFT) >> >> >> Hide this in ttm.c > > > ok, will do that. > thanks for pointing it out. > > >> >>> + >>> +int hibmc_gem_create(struct drm_device *dev, u32 size, bool iskernel, >>> + struct drm_gem_object **obj); >>> + >>> +int hibmc_mm_init(struct hibmc_drm_device *hibmc); >>> +void hibmc_mm_fini(struct hibmc_drm_device *hibmc); >>> +int hibmc_bo_pin(struct hibmc_bo *bo, u32 pl_flag, u64 *gpu_addr); >>> +void hibmc_gem_free_object(struct drm_gem_object *obj); >>> +int hibmc_dumb_create(struct drm_file *file, struct drm_device *dev, >>> + struct drm_mode_create_dumb *args); >>> +int hibmc_dumb_mmap_offset(struct drm_file *file, struct drm_device >>> *dev, >>> + u32 handle, u64 *offset); >>> +int hibmc_mmap(struct file *filp, struct vm_area_struct *vma); >>> + >>> #endif >>> diff --git a/drivers/gpu/drm/hisilicon/hibmc/hibmc_ttm.c >>> b/drivers/gpu/drm/hisilicon/hibmc/hibmc_ttm.c >>> new file mode 100644 >>> index 0000000..0802ebd >>> --- /dev/null >>> +++ b/drivers/gpu/drm/hisilicon/hibmc/hibmc_ttm.c >>> @@ -0,0 +1,490 @@ >>> +/* Hisilicon Hibmc SoC drm driver >>> + * >>> + * Based on the bochs drm driver. >>> + * >>> + * Copyright (c) 2016 Huawei Limited. >>> + * >>> + * Author: >>> + * Rongrong Zou <zourongrong@huawei.com> >>> + * Rongrong Zou <zourongrong@gmail.com> >>> + * Jianhua Li <lijianhua@huawei.com> >>> + * >>> + * This program is free software; you can redistribute it and/or modify >>> + * it under the terms of the GNU General Public License as published by >>> + * the Free Software Foundation; either version 2 of the License, or >>> + * (at your option) any later version. >>> + * >>> + */ >>> + >>> +#include "hibmc_drm_drv.h" >>> +#include <ttm/ttm_page_alloc.h> >>> +#include <drm/drm_crtc_helper.h> >>> +#include <drm/drm_atomic_helper.h> >>> + >>> +static inline struct hibmc_drm_device * >>> +hibmc_bdev(struct ttm_bo_device *bd) >>> +{ >>> + return container_of(bd, struct hibmc_drm_device, ttm.bdev); >>> +} >>> + >>> +static int >>> +hibmc_ttm_mem_global_init(struct drm_global_reference *ref) >>> +{ >>> + return ttm_mem_global_init(ref->object); >>> +} >>> + >>> +static void >>> +hibmc_ttm_mem_global_release(struct drm_global_reference *ref) >>> +{ >>> + ttm_mem_global_release(ref->object); >>> +} >>> + >>> +static int hibmc_ttm_global_init(struct hibmc_drm_device *hibmc) >>> +{ >>> + struct drm_global_reference *global_ref; >>> + int r; >> >> >> nit: try not to use one character variable names unless it's for the >> purpose of a loop (ie: i,j). You also use ret elsewhere in the driver, >> so it'd be nice to remain consistent > > > the whole file is delivered from bochs ttm, i didn't modify anything except > some checkpatch warnings and the 'hibmc_' prefix. Unfortunately, some > problems were delivered too. Yeah, seems like it. Perhaps you can post patches to fix these issues in the other drivers too :) > >> >>> + >>> + global_ref = &hibmc->ttm.mem_global_ref; >> >> >> I think using the global_ref local obfuscates what you're doing here. >> It saves you 6 characters while typing, but adds a layer of >> indirection for all future readers. >> >>> + global_ref->global_type = DRM_GLOBAL_TTM_MEM; >>> + global_ref->size = sizeof(struct ttm_mem_global); >>> + global_ref->init = &hibmc_ttm_mem_global_init; >>> + global_ref->release = &hibmc_ttm_mem_global_release; >>> + r = drm_global_item_ref(global_ref); >>> + if (r != 0) { >> >> >> nit: if (r) > > > will fix it, > thanks. > BTW, i wonder why checkpatch.pl didn't report it. > > >> >>> + DRM_ERROR("Failed setting up TTM memory accounting >>> subsystem.\n" >>> + ); >> >> >> Breaking up the line for one character is probably not worthwhile, and >> you should really print the error. How about: >> >> DRM_ERROR("Could not get ref on ttm global ret=%d.\n", ret); > > > i like your solution, thanks. > > >> >> >>> + return r; >>> + } >>> + >>> + hibmc->ttm.bo_global_ref.mem_glob = >>> + hibmc->ttm.mem_global_ref.object; >>> + global_ref = &hibmc->ttm.bo_global_ref.ref; >>> + global_ref->global_type = DRM_GLOBAL_TTM_BO; >>> + global_ref->size = sizeof(struct ttm_bo_global); >>> + global_ref->init = &ttm_bo_global_init; >>> + global_ref->release = &ttm_bo_global_release; >>> + r = drm_global_item_ref(global_ref); >>> + if (r != 0) { >>> + DRM_ERROR("Failed setting up TTM BO subsystem.\n"); >>> + drm_global_item_unref(&hibmc->ttm.mem_global_ref); >>> + return r; >>> + } >>> + return 0; >>> +} >>> + >>> +static void >>> +hibmc_ttm_global_release(struct hibmc_drm_device *hibmc) >>> +{ >>> + if (!hibmc->ttm.mem_global_ref.release) >> >> >> Are you actually hitting this condition? This seems like it's papering >> over something else. > > > it was also delivered from others, i looked at the xxx_ttm_global_init > function, 'mem_global_ref.release' is assigned unconditionally, so i > think this condition never be hit, it may be hit when release twice, > but this won't take place in my driver. > Yeah, that's what I was hoping for. So perhaps we can remove this? >> >>> + return; >>> + >>> + drm_global_item_unref(&hibmc->ttm.bo_global_ref.ref); >>> + drm_global_item_unref(&hibmc->ttm.mem_global_ref); >>> + hibmc->ttm.mem_global_ref.release = NULL; >>> +} >>> + >>> +static void hibmc_bo_ttm_destroy(struct ttm_buffer_object *tbo) >>> +{ >>> + struct hibmc_bo *bo; >>> + >>> + bo = container_of(tbo, struct hibmc_bo, bo); >> >> >> nit: No need to split this into a separate line. > > > agreed, thanks. > >> >>> + >>> + drm_gem_object_release(&bo->gem); >>> + kfree(bo); >>> +} >>> + >>> +static bool hibmc_ttm_bo_is_hibmc_bo(struct ttm_buffer_object *bo) >>> +{ >>> + if (bo->destroy == &hibmc_bo_ttm_destroy) >>> + return true; >>> + return false; >> >> >> return bo->destroy == &hibmc_bo_ttm_destroy; > > > looks better to me. > > >> >>> +} >>> + >>> +static int >>> +hibmc_bo_init_mem_type(struct ttm_bo_device *bdev, u32 type, >>> + struct ttm_mem_type_manager *man) >>> +{ >>> + switch (type) { >>> + case TTM_PL_SYSTEM: >>> + man->flags = TTM_MEMTYPE_FLAG_MAPPABLE; >>> + man->available_caching = TTM_PL_MASK_CACHING; >>> + man->default_caching = TTM_PL_FLAG_CACHED; >>> + break; >>> + case TTM_PL_VRAM: >>> + man->func = &ttm_bo_manager_func; >>> + man->flags = TTM_MEMTYPE_FLAG_FIXED | >>> + TTM_MEMTYPE_FLAG_MAPPABLE; >>> + man->available_caching = TTM_PL_FLAG_UNCACHED | >>> + TTM_PL_FLAG_WC; >>> + man->default_caching = TTM_PL_FLAG_WC; >>> + break; >>> + default: >>> + DRM_ERROR("Unsupported memory type %u\n", type); >>> + return -EINVAL; >>> + } >>> + return 0; >>> +} >>> + >>> +void hibmc_ttm_placement(struct hibmc_bo *bo, int domain) >>> +{ >>> + u32 c = 0; >> >> >> Can you please use a more descriptive name than 'c'? > > > ok, will do that. > >> >>> + u32 i; >>> + >>> + bo->placement.placement = bo->placements; >>> + bo->placement.busy_placement = bo->placements; >>> + if (domain & TTM_PL_FLAG_VRAM) >>> + bo->placements[c++].flags = TTM_PL_FLAG_WC | >>> + TTM_PL_FLAG_UNCACHED | TTM_PL_FLAG_VRAM; >> >> >> nit: you're alignment is off here and below > > > is it correct? > > if (domain & TTM_PL_FLAG_VRAM) > bo->placements[c++].flags = TTM_PL_FLAG_WC | > TTM_PL_FLAG_UNCACHED | TTM_PL_FLAG_VRAM; > if (domain & TTM_PL_FLAG_SYSTEM) > bo->placements[c++].flags = TTM_PL_MASK_CACHING | > TTM_PL_FLAG_SYSTEM; > if (!c) > bo->placements[c++].flags = TTM_PL_MASK_CACHING | > TTM_PL_FLAG_SYSTEM; > Pretty much anything other than lining them up one under the other is better >> >>> + if (domain & TTM_PL_FLAG_SYSTEM) >>> + bo->placements[c++].flags = TTM_PL_MASK_CACHING | >>> + TTM_PL_FLAG_SYSTEM; >>> + if (!c) >>> + bo->placements[c++].flags = TTM_PL_MASK_CACHING | >>> + TTM_PL_FLAG_SYSTEM; >>> + >>> + bo->placement.num_placement = c; >>> + bo->placement.num_busy_placement = c; >>> + for (i = 0; i < c; ++i) { >> >> >> nit: we tend towards post-increment in kernel > > > agreed, thanks. > > >> >>> + bo->placements[i].fpfn = 0; >>> + bo->placements[i].lpfn = 0; >>> + } >>> +} >>> + >>> +static void >>> +hibmc_bo_evict_flags(struct ttm_buffer_object *bo, struct ttm_placement >>> *pl) >>> +{ >>> + struct hibmc_bo *hibmcbo = hibmc_bo(bo); >>> + >>> + if (!hibmc_ttm_bo_is_hibmc_bo(bo)) >>> + return; >>> + >>> + hibmc_ttm_placement(hibmcbo, TTM_PL_FLAG_SYSTEM); >>> + *pl = hibmcbo->placement; >>> +} >>> + >>> +static int hibmc_bo_verify_access(struct ttm_buffer_object *bo, >>> + struct file *filp) >>> +{ >>> + struct hibmc_bo *hibmcbo = hibmc_bo(bo); >>> + >>> + return drm_vma_node_verify_access(&hibmcbo->gem.vma_node, >>> + filp->private_data); >>> +} >>> + >>> +static int hibmc_ttm_io_mem_reserve(struct ttm_bo_device *bdev, >>> + struct ttm_mem_reg *mem) >>> +{ >>> + struct ttm_mem_type_manager *man = &bdev->man[mem->mem_type]; >>> + struct hibmc_drm_device *hibmc = hibmc_bdev(bdev); >>> + >>> + mem->bus.addr = NULL; >>> + mem->bus.offset = 0; >>> + mem->bus.size = mem->num_pages << PAGE_SHIFT; >>> + mem->bus.base = 0; >>> + mem->bus.is_iomem = false; >>> + if (!(man->flags & TTM_MEMTYPE_FLAG_MAPPABLE)) >>> + return -EINVAL; >>> + switch (mem->mem_type) { >>> + case TTM_PL_SYSTEM: >>> + /* system memory */ >>> + return 0; >>> + case TTM_PL_VRAM: >>> + mem->bus.offset = mem->start << PAGE_SHIFT; >>> + mem->bus.base = pci_resource_start(hibmc->dev->pdev, 0); >>> + mem->bus.is_iomem = true; >>> + break; >>> + default: >>> + return -EINVAL; >>> + } >>> + return 0; >>> +} >>> + >>> +static void hibmc_ttm_io_mem_free(struct ttm_bo_device *bdev, >>> + struct ttm_mem_reg *mem) >>> +{ >>> +} >> >> >> No need to stub this, the caller does a NULL-check before invoking > > > will delete it, thanks. > >> >>> + >>> +static void hibmc_ttm_backend_destroy(struct ttm_tt *tt) >>> +{ >>> + ttm_tt_fini(tt); >>> + kfree(tt); >>> +} >>> + >>> +static struct ttm_backend_func hibmc_tt_backend_func = { >>> + .destroy = &hibmc_ttm_backend_destroy, >>> +}; >>> + >>> +static struct ttm_tt *hibmc_ttm_tt_create(struct ttm_bo_device *bdev, >>> + unsigned long size, >>> + u32 page_flags, >>> + struct page *dummy_read_page) >>> +{ >>> + struct ttm_tt *tt; >>> + >>> + tt = kzalloc(sizeof(*tt), GFP_KERNEL); >>> + if (!tt) >> >> >> Print error > > > ok, will do that, thanks. > >> >>> + return NULL; >>> + tt->func = &hibmc_tt_backend_func; >>> + if (ttm_tt_init(tt, bdev, size, page_flags, dummy_read_page)) { >> >> >> Here too? > > > ditto > > >> >>> + kfree(tt); >>> + return NULL; >>> + } >>> + return tt; >>> +} >>> + >>> +static int hibmc_ttm_tt_populate(struct ttm_tt *ttm) >>> +{ >>> + return ttm_pool_populate(ttm); >>> +} >>> + >>> +static void hibmc_ttm_tt_unpopulate(struct ttm_tt *ttm) >>> +{ >>> + ttm_pool_unpopulate(ttm); >>> +} >>> + >>> +struct ttm_bo_driver hibmc_bo_driver = { >>> + .ttm_tt_create = hibmc_ttm_tt_create, >>> + .ttm_tt_populate = hibmc_ttm_tt_populate, >>> + .ttm_tt_unpopulate = hibmc_ttm_tt_unpopulate, >>> + .init_mem_type = hibmc_bo_init_mem_type, >>> + .evict_flags = hibmc_bo_evict_flags, >>> + .move = NULL, >>> + .verify_access = hibmc_bo_verify_access, >>> + .io_mem_reserve = &hibmc_ttm_io_mem_reserve, >>> + .io_mem_free = &hibmc_ttm_io_mem_free, >>> + .lru_tail = &ttm_bo_default_lru_tail, >>> + .swap_lru_tail = &ttm_bo_default_swap_lru_tail, >>> +}; >>> + >>> +int hibmc_mm_init(struct hibmc_drm_device *hibmc) >>> +{ >>> + int ret; >>> + struct drm_device *dev = hibmc->dev; >>> + struct ttm_bo_device *bdev = &hibmc->ttm.bdev; >>> + >>> + ret = hibmc_ttm_global_init(hibmc); >>> + if (ret) >>> + return ret; >>> + >>> + ret = ttm_bo_device_init(&hibmc->ttm.bdev, >>> + hibmc->ttm.bo_global_ref.ref.object, >>> + &hibmc_bo_driver, >>> + dev->anon_inode->i_mapping, >>> + DRM_FILE_PAGE_OFFSET, >>> + true); >>> + if (ret) { >> >> >> Call hibmc_ttm_global_release here? > > > agreed, thanks for pointing it out. > >> >>> + DRM_ERROR("Error initialising bo driver; %d\n", ret); >>> + return ret; >>> + } >>> + >>> + ret = ttm_bo_init_mm(bdev, TTM_PL_VRAM, >>> + hibmc->fb_size >> PAGE_SHIFT); >>> + if (ret) { >> >> >> Clean up here as well? > > > ditto > > >> >>> + DRM_ERROR("Failed ttm VRAM init: %d\n", ret); >>> + return ret; >>> + } >>> + >>> + hibmc->mm_inited = true; >>> + return 0; >>> +} >>> + >>> +void hibmc_mm_fini(struct hibmc_drm_device *hibmc) >>> +{ >>> + if (!hibmc->mm_inited) >>> + return; >>> + >>> + ttm_bo_device_release(&hibmc->ttm.bdev); >>> + hibmc_ttm_global_release(hibmc); >>> + hibmc->mm_inited = false; >>> +} >>> + >>> +int hibmc_bo_create(struct drm_device *dev, int size, int align, >>> + u32 flags, struct hibmc_bo **phibmcbo) >>> +{ >>> + struct hibmc_drm_device *hibmc = dev->dev_private; >>> + struct hibmc_bo *hibmcbo; >>> + size_t acc_size; >>> + int ret; >>> + >>> + hibmcbo = kzalloc(sizeof(*hibmcbo), GFP_KERNEL); >>> + if (!hibmcbo) >>> + return -ENOMEM; >>> + >>> + ret = drm_gem_object_init(dev, &hibmcbo->gem, size); >>> + if (ret) { >>> + kfree(hibmcbo); >>> + return ret; >>> + } >>> + >>> + hibmcbo->bo.bdev = &hibmc->ttm.bdev; >>> + >>> + hibmc_ttm_placement(hibmcbo, TTM_PL_FLAG_VRAM | >>> TTM_PL_FLAG_SYSTEM); >>> + >>> + acc_size = ttm_bo_dma_acc_size(&hibmc->ttm.bdev, size, >>> + sizeof(struct hibmc_bo)); >>> + >>> + ret = ttm_bo_init(&hibmc->ttm.bdev, &hibmcbo->bo, size, >>> + ttm_bo_type_device, &hibmcbo->placement, >>> + align >> PAGE_SHIFT, false, NULL, acc_size, >>> + NULL, NULL, hibmc_bo_ttm_destroy); >>> + if (ret) >> >> >> Missing hibmcbo clean up here > > > i looked at all other ttm drivers and all of them return directly when > ttm_bo_init > failed, however, i think it is better to clean up here, should i call > hibmc_bo_unref(&hibmc_bo) here ? > Yeah, that should work (might want to test it, though ;) >> >>> + return ret; >>> + >>> + *phibmcbo = hibmcbo; >>> + return 0; >>> +} >>> + >>> +static inline u64 hibmc_bo_gpu_offset(struct hibmc_bo *bo) >>> +{ >>> + return bo->bo.offset; >>> +} >> >> >> I don't think this function provides any value > > > do you nean i use bo->bo.offset instead of calling hibmc_bo_gpu_offset()? > yes >> >>> + >>> +int hibmc_bo_pin(struct hibmc_bo *bo, u32 pl_flag, u64 *gpu_addr) >>> +{ >>> + int i, ret; >>> + >>> + if (bo->pin_count) { >>> + bo->pin_count++; >>> + if (gpu_addr) >>> + *gpu_addr = hibmc_bo_gpu_offset(bo); >> >> >> Are you missing a return here? > > > Thanks for pointing it out! > > >> >>> + } >>> + >>> + hibmc_ttm_placement(bo, pl_flag); >>> + for (i = 0; i < bo->placement.num_placement; i++) >>> + bo->placements[i].flags |= TTM_PL_FLAG_NO_EVICT; >>> + ret = ttm_bo_validate(&bo->bo, &bo->placement, false, false); >>> + if (ret) >>> + return ret; >>> + >>> + bo->pin_count = 1; >>> + if (gpu_addr) >>> + *gpu_addr = hibmc_bo_gpu_offset(bo); >>> + return 0; >>> +} >>> + >>> +int hibmc_bo_push_sysram(struct hibmc_bo *bo) >>> +{ >>> + int i, ret; >>> + >>> + if (!bo->pin_count) { >>> + DRM_ERROR("unpin bad %p\n", bo); >>> + return 0; >>> + } >>> + bo->pin_count--; >>> + if (bo->pin_count) >>> + return 0; >>> + >>> + if (bo->kmap.virtual) >> >> >> ttm_bo_kunmap already does this check so you don't have to > > > agreed. will remove this condition. > >> >>> + ttm_bo_kunmap(&bo->kmap); >>> + >>> + hibmc_ttm_placement(bo, TTM_PL_FLAG_SYSTEM); >>> + for (i = 0; i < bo->placement.num_placement ; i++) >>> + bo->placements[i].flags |= TTM_PL_FLAG_NO_EVICT; >>> + >>> + ret = ttm_bo_validate(&bo->bo, &bo->placement, false, false); >>> + if (ret) { >>> + DRM_ERROR("pushing to VRAM failed\n"); >> >> >> Print ret > > > ok, thanks. > > >> >>> + return ret; >>> + } >>> + return 0; >>> +} >>> + >>> +int hibmc_mmap(struct file *filp, struct vm_area_struct *vma) >>> +{ >>> + struct drm_file *file_priv; >>> + struct hibmc_drm_device *hibmc; >>> + >>> + if (unlikely(vma->vm_pgoff < DRM_FILE_PAGE_OFFSET)) >>> + return -EINVAL; >>> + >>> + file_priv = filp->private_data; >>> + hibmc = file_priv->minor->dev->dev_private; >>> + return ttm_bo_mmap(filp, vma, &hibmc->ttm.bdev); >>> +} >>> + >>> +int hibmc_gem_create(struct drm_device *dev, u32 size, bool iskernel, >>> + struct drm_gem_object **obj) >>> +{ >>> + struct hibmc_bo *hibmcbo; >>> + int ret; >>> + >>> + *obj = NULL; >>> + >>> + size = PAGE_ALIGN(size); >>> + if (size == 0) >> >> >> Print error > > > ditto > >> >>> + return -EINVAL; >>> + >>> + ret = hibmc_bo_create(dev, size, 0, 0, &hibmcbo); >>> + if (ret) { >>> + if (ret != -ERESTARTSYS) >>> + DRM_ERROR("failed to allocate GEM object\n"); >> >> >> Print ret > > > ditto > >> >>> + return ret; >>> + } >>> + *obj = &hibmcbo->gem; >>> + return 0; >>> +} >>> + >>> +int hibmc_dumb_create(struct drm_file *file, struct drm_device *dev, >>> + struct drm_mode_create_dumb *args) >>> +{ >>> + struct drm_gem_object *gobj; >>> + u32 handle; >>> + int ret; >>> + >>> + args->pitch = ALIGN(args->width * ((args->bpp + 7) / 8), 16); >> >> >> What's up with the bpp + 7 here? Perhaps you're looking for DIV_ROUND_UP? > > > Yes, that sounds sane. > sane is what i usually aim for :) Sean >> >> >>> + args->size = args->pitch * args->height; >>> + >>> + ret = hibmc_gem_create(dev, args->size, false, >>> + &gobj); >>> + if (ret) >>> + return ret; >>> + >>> + ret = drm_gem_handle_create(file, gobj, &handle); >>> + drm_gem_object_unreference_unlocked(gobj); >>> + if (ret) >> >> >> Print error here > > > agreed. > > >> >>> + return ret; >>> + >>> + args->handle = handle; >>> + return 0; >>> +} >>> + >>> +static void hibmc_bo_unref(struct hibmc_bo **bo) >>> +{ >>> + struct ttm_buffer_object *tbo; >>> + >>> + if ((*bo) == NULL) >>> + return; >>> + >>> + tbo = &((*bo)->bo); >>> + ttm_bo_unref(&tbo); >>> + *bo = NULL; >>> +} >>> + >>> +void hibmc_gem_free_object(struct drm_gem_object *obj) >>> +{ >>> + struct hibmc_bo *hibmcbo = gem_to_hibmc_bo(obj); >>> + >>> + hibmc_bo_unref(&hibmcbo); >>> +} >>> + >>> +static u64 hibmc_bo_mmap_offset(struct hibmc_bo *bo) >>> +{ >>> + return drm_vma_node_offset_addr(&bo->bo.vma_node); >>> +} >>> + >>> +int hibmc_dumb_mmap_offset(struct drm_file *file, struct drm_device >>> *dev, >>> + u32 handle, u64 *offset) >>> +{ >>> + struct drm_gem_object *obj; >>> + struct hibmc_bo *bo; >>> + >>> + obj = drm_gem_object_lookup(file, handle); >>> + if (!obj) >>> + return -ENOENT; >>> + >>> + bo = gem_to_hibmc_bo(obj); >>> + *offset = hibmc_bo_mmap_offset(bo); >>> + >>> + drm_gem_object_unreference_unlocked(obj); >>> + return 0; >>> +} > > > Regards, > Rongrong. > >>> -- >>> 1.9.1 >>> >>> >>> _______________________________________________ >>> linux-arm-kernel mailing list >>> linux-arm-kernel@lists.infradead.org >>> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel >> >> _______________________________________________ >> linuxarm mailing list >> linuxarm@huawei.com >> http://rnd-openeuler.huawei.com/mailman/listinfo/linuxarm >> >> . >> > > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
在 2016/11/11 21:25, Sean Paul 写道: > On Fri, Nov 11, 2016 at 6:16 AM, Rongrong Zou <zourongrong@huawei.com> wrote: >> 在 2016/11/11 1:35, Sean Paul 写道: >>> >>> On Fri, Oct 28, 2016 at 3:27 AM, Rongrong Zou <zourongrong@gmail.com> >>> wrote: >>>> >>>> Hibmc have 32m video memory which can be accessed through PCIe by host, >>>> we use ttm to manage these memory. >>>> >>>> Signed-off-by: Rongrong Zou <zourongrong@gmail.com> >>>> --- >>>> drivers/gpu/drm/hisilicon/hibmc/Kconfig | 1 + >>>> drivers/gpu/drm/hisilicon/hibmc/Makefile | 2 +- >>>> drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c | 12 + >>>> drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.h | 46 +++ >>>> drivers/gpu/drm/hisilicon/hibmc/hibmc_ttm.c | 490 >>>> ++++++++++++++++++++++++ >>>> 5 files changed, 550 insertions(+), 1 deletion(-) >>>> create mode 100644 drivers/gpu/drm/hisilicon/hibmc/hibmc_ttm.c >>>> >>>> diff --git a/drivers/gpu/drm/hisilicon/hibmc/Kconfig >>>> b/drivers/gpu/drm/hisilicon/hibmc/Kconfig >>>> index a9af90d..bcb8c18 100644 >>>> --- a/drivers/gpu/drm/hisilicon/hibmc/Kconfig >>>> +++ b/drivers/gpu/drm/hisilicon/hibmc/Kconfig >>>> @@ -1,6 +1,7 @@ >>>> config DRM_HISI_HIBMC >>>> tristate "DRM Support for Hisilicon Hibmc" >>>> depends on DRM && PCI >>>> + select DRM_TTM >>>> >>>> help >>>> Choose this option if you have a Hisilicon Hibmc soc chipset. >>>> diff --git a/drivers/gpu/drm/hisilicon/hibmc/Makefile >>>> b/drivers/gpu/drm/hisilicon/hibmc/Makefile >>>> index 97cf4a0..d5c40b8 100644 >>>> --- a/drivers/gpu/drm/hisilicon/hibmc/Makefile >>>> +++ b/drivers/gpu/drm/hisilicon/hibmc/Makefile >>>> @@ -1,5 +1,5 @@ >>>> ccflags-y := -Iinclude/drm >>>> -hibmc-drm-y := hibmc_drm_drv.o hibmc_drm_power.o >>>> +hibmc-drm-y := hibmc_drm_drv.o hibmc_drm_power.o hibmc_ttm.o >>>> >>>> obj-$(CONFIG_DRM_HISI_HIBMC) +=hibmc-drm.o >>>> #obj-y += hibmc-drm.o >>>> diff --git a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c >>>> b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c >>>> index 4669d42..81f4301 100644 >>>> --- a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c >>>> +++ b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c >>>> @@ -31,6 +31,7 @@ >>>> #ifdef CONFIG_COMPAT >>>> .compat_ioctl = drm_compat_ioctl, >>>> #endif >>>> + .mmap = hibmc_mmap, >>>> .poll = drm_poll, >>>> .read = drm_read, >>>> .llseek = no_llseek, >>>> @@ -46,6 +47,8 @@ static void hibmc_disable_vblank(struct drm_device >>>> *dev, unsigned int pipe) >>>> } >>>> >>>> static struct drm_driver hibmc_driver = { >>>> + .driver_features = DRIVER_GEM, >>>> + >>> >>> >>> nit: extra space >>> >>>> .fops = &hibmc_fops, >>>> .name = "hibmc", >>>> .date = "20160828", >>>> @@ -55,6 +58,10 @@ static void hibmc_disable_vblank(struct drm_device >>>> *dev, unsigned int pipe) >>>> .get_vblank_counter = drm_vblank_no_hw_counter, >>>> .enable_vblank = hibmc_enable_vblank, >>>> .disable_vblank = hibmc_disable_vblank, >>>> + .gem_free_object_unlocked = hibmc_gem_free_object, >>>> + .dumb_create = hibmc_dumb_create, >>>> + .dumb_map_offset = hibmc_dumb_mmap_offset, >>>> + .dumb_destroy = drm_gem_dumb_destroy, >>>> }; >>>> >>>> static int hibmc_pm_suspend(struct device *dev) >>>> @@ -163,6 +170,7 @@ static int hibmc_unload(struct drm_device *dev) >>>> { >>>> struct hibmc_drm_device *hidev = dev->dev_private; >>>> >>>> + hibmc_mm_fini(hidev); >>>> hibmc_hw_fini(hidev); >>>> dev->dev_private = NULL; >>>> return 0; >>>> @@ -183,6 +191,10 @@ static int hibmc_load(struct drm_device *dev, >>>> unsigned long flags) >>>> if (ret) >>>> goto err; >>>> >>>> + ret = hibmc_mm_init(hidev); >>>> + if (ret) >>>> + goto err; >>>> + >>>> return 0; >>>> >>>> err: >>>> diff --git a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.h >>>> b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.h >>>> index 0037341..db8d80e 100644 >>>> --- a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.h >>>> +++ b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.h >>>> @@ -20,6 +20,8 @@ >>>> #define HIBMC_DRM_DRV_H >>>> >>>> #include <drm/drmP.h> >>>> +#include <drm/ttm/ttm_bo_driver.h> >>>> +#include <drm/drm_gem.h> >>> >>> >>> nit: alphabetize >> >> >> will fix it, thanks. >> >>> >>>> >>>> struct hibmc_drm_device { >>>> /* hw */ >>>> @@ -30,6 +32,50 @@ struct hibmc_drm_device { >>>> >>>> /* drm */ >>>> struct drm_device *dev; >>>> + >>>> + /* ttm */ >>>> + struct { >>>> + struct drm_global_reference mem_global_ref; >>>> + struct ttm_bo_global_ref bo_global_ref; >>>> + struct ttm_bo_device bdev; >>>> + bool initialized; >>>> + } ttm; >>> >>> >>> I don't think you gain anything other than keystrokes from the substruct >> >> >> I'm sorry i didn't catch you, i looked at the all drivers used ttm such >> as ast/bochs/cirrus/mgag200/qxl/virtio_gpu, they all embedded the ttm >> substruct >> into the driver-private struct. >> >> so do you mean >> struct hibmc_drm_device { >> /* hw */ >> void __iomem *mmio; >> void __iomem *fb_map; >> unsigned long fb_base; >> unsigned long fb_size; >> >> /* drm */ >> struct drm_device *dev; >> struct drm_plane plane; >> struct drm_crtc crtc; >> struct drm_encoder encoder; >> struct drm_connector connector; >> bool mode_config_initialized; >> >> /* ttm */ >> struct drm_global_reference mem_global_ref; >> struct ttm_bo_global_ref bo_global_ref; >> struct ttm_bo_device bdev; >> bool initialized; >> ... >> }; >> ? > > Yeah, that's what I was thinking > >> >>> >>>> + >>>> + bool mm_inited; >>>> }; >>>> >>>> +struct hibmc_bo { >>>> + struct ttm_buffer_object bo; >>>> + struct ttm_placement placement; >>>> + struct ttm_bo_kmap_obj kmap; >>>> + struct drm_gem_object gem; >>>> + struct ttm_place placements[3]; >>>> + int pin_count; >>>> +}; >>>> + >>>> +static inline struct hibmc_bo *hibmc_bo(struct ttm_buffer_object *bo) >>>> +{ >>>> + return container_of(bo, struct hibmc_bo, bo); >>>> +} >>>> + >>>> +static inline struct hibmc_bo *gem_to_hibmc_bo(struct drm_gem_object >>>> *gem) >>>> +{ >>>> + return container_of(gem, struct hibmc_bo, gem); >>>> +} >>>> + >>>> +#define DRM_FILE_PAGE_OFFSET (0x100000000ULL >> PAGE_SHIFT) >>> >>> >>> Hide this in ttm.c >> >> >> ok, will do that. >> thanks for pointing it out. >> >> >>> >>>> + >>>> +int hibmc_gem_create(struct drm_device *dev, u32 size, bool iskernel, >>>> + struct drm_gem_object **obj); >>>> + >>>> +int hibmc_mm_init(struct hibmc_drm_device *hibmc); >>>> +void hibmc_mm_fini(struct hibmc_drm_device *hibmc); >>>> +int hibmc_bo_pin(struct hibmc_bo *bo, u32 pl_flag, u64 *gpu_addr); >>>> +void hibmc_gem_free_object(struct drm_gem_object *obj); >>>> +int hibmc_dumb_create(struct drm_file *file, struct drm_device *dev, >>>> + struct drm_mode_create_dumb *args); >>>> +int hibmc_dumb_mmap_offset(struct drm_file *file, struct drm_device >>>> *dev, >>>> + u32 handle, u64 *offset); >>>> +int hibmc_mmap(struct file *filp, struct vm_area_struct *vma); >>>> + >>>> #endif >>>> diff --git a/drivers/gpu/drm/hisilicon/hibmc/hibmc_ttm.c >>>> b/drivers/gpu/drm/hisilicon/hibmc/hibmc_ttm.c >>>> new file mode 100644 >>>> index 0000000..0802ebd >>>> --- /dev/null >>>> +++ b/drivers/gpu/drm/hisilicon/hibmc/hibmc_ttm.c >>>> @@ -0,0 +1,490 @@ >>>> +/* Hisilicon Hibmc SoC drm driver >>>> + * >>>> + * Based on the bochs drm driver. >>>> + * >>>> + * Copyright (c) 2016 Huawei Limited. >>>> + * >>>> + * Author: >>>> + * Rongrong Zou <zourongrong@huawei.com> >>>> + * Rongrong Zou <zourongrong@gmail.com> >>>> + * Jianhua Li <lijianhua@huawei.com> >>>> + * >>>> + * This program is free software; you can redistribute it and/or modify >>>> + * it under the terms of the GNU General Public License as published by >>>> + * the Free Software Foundation; either version 2 of the License, or >>>> + * (at your option) any later version. >>>> + * >>>> + */ >>>> + >>>> +#include "hibmc_drm_drv.h" >>>> +#include <ttm/ttm_page_alloc.h> >>>> +#include <drm/drm_crtc_helper.h> >>>> +#include <drm/drm_atomic_helper.h> >>>> + >>>> +static inline struct hibmc_drm_device * >>>> +hibmc_bdev(struct ttm_bo_device *bd) >>>> +{ >>>> + return container_of(bd, struct hibmc_drm_device, ttm.bdev); >>>> +} >>>> + >>>> +static int >>>> +hibmc_ttm_mem_global_init(struct drm_global_reference *ref) >>>> +{ >>>> + return ttm_mem_global_init(ref->object); >>>> +} >>>> + >>>> +static void >>>> +hibmc_ttm_mem_global_release(struct drm_global_reference *ref) >>>> +{ >>>> + ttm_mem_global_release(ref->object); >>>> +} >>>> + >>>> +static int hibmc_ttm_global_init(struct hibmc_drm_device *hibmc) >>>> +{ >>>> + struct drm_global_reference *global_ref; >>>> + int r; >>> >>> >>> nit: try not to use one character variable names unless it's for the >>> purpose of a loop (ie: i,j). You also use ret elsewhere in the driver, >>> so it'd be nice to remain consistent >> >> >> the whole file is delivered from bochs ttm, i didn't modify anything except >> some checkpatch warnings and the 'hibmc_' prefix. Unfortunately, some >> problems were delivered too. > > Yeah, seems like it. Perhaps you can post patches to fix these issues > in the other drivers too :) i will do after the this one get merged :) > >> >>> >>>> + >>>> + global_ref = &hibmc->ttm.mem_global_ref; >>> >>> >>> I think using the global_ref local obfuscates what you're doing here. >>> It saves you 6 characters while typing, but adds a layer of >>> indirection for all future readers. >>> >>>> + global_ref->global_type = DRM_GLOBAL_TTM_MEM; >>>> + global_ref->size = sizeof(struct ttm_mem_global); >>>> + global_ref->init = &hibmc_ttm_mem_global_init; >>>> + global_ref->release = &hibmc_ttm_mem_global_release; >>>> + r = drm_global_item_ref(global_ref); >>>> + if (r != 0) { >>> >>> >>> nit: if (r) >> >> >> will fix it, >> thanks. >> BTW, i wonder why checkpatch.pl didn't report it. >> >> >>> >>>> + DRM_ERROR("Failed setting up TTM memory accounting >>>> subsystem.\n" >>>> + ); >>> >>> >>> Breaking up the line for one character is probably not worthwhile, and >>> you should really print the error. How about: >>> >>> DRM_ERROR("Could not get ref on ttm global ret=%d.\n", ret); >> >> >> i like your solution, thanks. >> >> >>> >>> >>>> + return r; >>>> + } >>>> + >>>> + hibmc->ttm.bo_global_ref.mem_glob = >>>> + hibmc->ttm.mem_global_ref.object; >>>> + global_ref = &hibmc->ttm.bo_global_ref.ref; >>>> + global_ref->global_type = DRM_GLOBAL_TTM_BO; >>>> + global_ref->size = sizeof(struct ttm_bo_global); >>>> + global_ref->init = &ttm_bo_global_init; >>>> + global_ref->release = &ttm_bo_global_release; >>>> + r = drm_global_item_ref(global_ref); >>>> + if (r != 0) { >>>> + DRM_ERROR("Failed setting up TTM BO subsystem.\n"); >>>> + drm_global_item_unref(&hibmc->ttm.mem_global_ref); >>>> + return r; >>>> + } >>>> + return 0; >>>> +} >>>> + >>>> +static void >>>> +hibmc_ttm_global_release(struct hibmc_drm_device *hibmc) >>>> +{ >>>> + if (!hibmc->ttm.mem_global_ref.release) >>> >>> >>> Are you actually hitting this condition? This seems like it's papering >>> over something else. >> >> >> it was also delivered from others, i looked at the xxx_ttm_global_init >> function, 'mem_global_ref.release' is assigned unconditionally, so i >> think this condition never be hit, it may be hit when release twice, >> but this won't take place in my driver. >> > > Yeah, that's what I was hoping for. So perhaps we can remove this? yes, we can. Regards, Rongrong. > >>> >>>> + return; >>>> + >>>> + drm_global_item_unref(&hibmc->ttm.bo_global_ref.ref); >>>> + drm_global_item_unref(&hibmc->ttm.mem_global_ref); >>>> + hibmc->ttm.mem_global_ref.release = NULL; >>>> +} >>>> + >>>> +static void hibmc_bo_ttm_destroy(struct ttm_buffer_object *tbo) >>>> +{ >>>> + struct hibmc_bo *bo; >>>> + >>>> + bo = container_of(tbo, struct hibmc_bo, bo); >>> >>> >>> nit: No need to split this into a separate line. >> >> >> agreed, thanks. >> >>> >>>> + >>>> + drm_gem_object_release(&bo->gem); >>>> + kfree(bo); >>>> +} >>>> + >>>> +static bool hibmc_ttm_bo_is_hibmc_bo(struct ttm_buffer_object *bo) >>>> +{ >>>> + if (bo->destroy == &hibmc_bo_ttm_destroy) >>>> + return true; >>>> + return false; >>> >>> >>> return bo->destroy == &hibmc_bo_ttm_destroy; >> >> >> looks better to me. >> >> >>> >>>> +} >>>> + >>>> +static int >>>> +hibmc_bo_init_mem_type(struct ttm_bo_device *bdev, u32 type, >>>> + struct ttm_mem_type_manager *man) >>>> +{ >>>> + switch (type) { >>>> + case TTM_PL_SYSTEM: >>>> + man->flags = TTM_MEMTYPE_FLAG_MAPPABLE; >>>> + man->available_caching = TTM_PL_MASK_CACHING; >>>> + man->default_caching = TTM_PL_FLAG_CACHED; >>>> + break; >>>> + case TTM_PL_VRAM: >>>> + man->func = &ttm_bo_manager_func; >>>> + man->flags = TTM_MEMTYPE_FLAG_FIXED | >>>> + TTM_MEMTYPE_FLAG_MAPPABLE; >>>> + man->available_caching = TTM_PL_FLAG_UNCACHED | >>>> + TTM_PL_FLAG_WC; >>>> + man->default_caching = TTM_PL_FLAG_WC; >>>> + break; >>>> + default: >>>> + DRM_ERROR("Unsupported memory type %u\n", type); >>>> + return -EINVAL; >>>> + } >>>> + return 0; >>>> +} >>>> + >>>> +void hibmc_ttm_placement(struct hibmc_bo *bo, int domain) >>>> +{ >>>> + u32 c = 0; >>> >>> >>> Can you please use a more descriptive name than 'c'? >> >> >> ok, will do that. >> >>> >>>> + u32 i; >>>> + >>>> + bo->placement.placement = bo->placements; >>>> + bo->placement.busy_placement = bo->placements; >>>> + if (domain & TTM_PL_FLAG_VRAM) >>>> + bo->placements[c++].flags = TTM_PL_FLAG_WC | >>>> + TTM_PL_FLAG_UNCACHED | TTM_PL_FLAG_VRAM; >>> >>> >>> nit: you're alignment is off here and below >> >> >> is it correct? >> >> if (domain & TTM_PL_FLAG_VRAM) >> bo->placements[c++].flags = TTM_PL_FLAG_WC | >> TTM_PL_FLAG_UNCACHED | TTM_PL_FLAG_VRAM; >> if (domain & TTM_PL_FLAG_SYSTEM) >> bo->placements[c++].flags = TTM_PL_MASK_CACHING | >> TTM_PL_FLAG_SYSTEM; >> if (!c) >> bo->placements[c++].flags = TTM_PL_MASK_CACHING | >> TTM_PL_FLAG_SYSTEM; >> > > Pretty much anything other than lining them up one under the other is better > >>> >>>> + if (domain & TTM_PL_FLAG_SYSTEM) >>>> + bo->placements[c++].flags = TTM_PL_MASK_CACHING | >>>> + TTM_PL_FLAG_SYSTEM; >>>> + if (!c) >>>> + bo->placements[c++].flags = TTM_PL_MASK_CACHING | >>>> + TTM_PL_FLAG_SYSTEM; >>>> + >>>> + bo->placement.num_placement = c; >>>> + bo->placement.num_busy_placement = c; >>>> + for (i = 0; i < c; ++i) { >>> >>> >>> nit: we tend towards post-increment in kernel >> >> >> agreed, thanks. >> >> >>> >>>> + bo->placements[i].fpfn = 0; >>>> + bo->placements[i].lpfn = 0; >>>> + } >>>> +} >>>> + >>>> +static void >>>> +hibmc_bo_evict_flags(struct ttm_buffer_object *bo, struct ttm_placement >>>> *pl) >>>> +{ >>>> + struct hibmc_bo *hibmcbo = hibmc_bo(bo); >>>> + >>>> + if (!hibmc_ttm_bo_is_hibmc_bo(bo)) >>>> + return; >>>> + >>>> + hibmc_ttm_placement(hibmcbo, TTM_PL_FLAG_SYSTEM); >>>> + *pl = hibmcbo->placement; >>>> +} >>>> + >>>> +static int hibmc_bo_verify_access(struct ttm_buffer_object *bo, >>>> + struct file *filp) >>>> +{ >>>> + struct hibmc_bo *hibmcbo = hibmc_bo(bo); >>>> + >>>> + return drm_vma_node_verify_access(&hibmcbo->gem.vma_node, >>>> + filp->private_data); >>>> +} >>>> + >>>> +static int hibmc_ttm_io_mem_reserve(struct ttm_bo_device *bdev, >>>> + struct ttm_mem_reg *mem) >>>> +{ >>>> + struct ttm_mem_type_manager *man = &bdev->man[mem->mem_type]; >>>> + struct hibmc_drm_device *hibmc = hibmc_bdev(bdev); >>>> + >>>> + mem->bus.addr = NULL; >>>> + mem->bus.offset = 0; >>>> + mem->bus.size = mem->num_pages << PAGE_SHIFT; >>>> + mem->bus.base = 0; >>>> + mem->bus.is_iomem = false; >>>> + if (!(man->flags & TTM_MEMTYPE_FLAG_MAPPABLE)) >>>> + return -EINVAL; >>>> + switch (mem->mem_type) { >>>> + case TTM_PL_SYSTEM: >>>> + /* system memory */ >>>> + return 0; >>>> + case TTM_PL_VRAM: >>>> + mem->bus.offset = mem->start << PAGE_SHIFT; >>>> + mem->bus.base = pci_resource_start(hibmc->dev->pdev, 0); >>>> + mem->bus.is_iomem = true; >>>> + break; >>>> + default: >>>> + return -EINVAL; >>>> + } >>>> + return 0; >>>> +} >>>> + >>>> +static void hibmc_ttm_io_mem_free(struct ttm_bo_device *bdev, >>>> + struct ttm_mem_reg *mem) >>>> +{ >>>> +} >>> >>> >>> No need to stub this, the caller does a NULL-check before invoking >> >> >> will delete it, thanks. >> >>> >>>> + >>>> +static void hibmc_ttm_backend_destroy(struct ttm_tt *tt) >>>> +{ >>>> + ttm_tt_fini(tt); >>>> + kfree(tt); >>>> +} >>>> + >>>> +static struct ttm_backend_func hibmc_tt_backend_func = { >>>> + .destroy = &hibmc_ttm_backend_destroy, >>>> +}; >>>> + >>>> +static struct ttm_tt *hibmc_ttm_tt_create(struct ttm_bo_device *bdev, >>>> + unsigned long size, >>>> + u32 page_flags, >>>> + struct page *dummy_read_page) >>>> +{ >>>> + struct ttm_tt *tt; >>>> + >>>> + tt = kzalloc(sizeof(*tt), GFP_KERNEL); >>>> + if (!tt) >>> >>> >>> Print error >> >> >> ok, will do that, thanks. >> >>> >>>> + return NULL; >>>> + tt->func = &hibmc_tt_backend_func; >>>> + if (ttm_tt_init(tt, bdev, size, page_flags, dummy_read_page)) { >>> >>> >>> Here too? >> >> >> ditto >> >> >>> >>>> + kfree(tt); >>>> + return NULL; >>>> + } >>>> + return tt; >>>> +} >>>> + >>>> +static int hibmc_ttm_tt_populate(struct ttm_tt *ttm) >>>> +{ >>>> + return ttm_pool_populate(ttm); >>>> +} >>>> + >>>> +static void hibmc_ttm_tt_unpopulate(struct ttm_tt *ttm) >>>> +{ >>>> + ttm_pool_unpopulate(ttm); >>>> +} >>>> + >>>> +struct ttm_bo_driver hibmc_bo_driver = { >>>> + .ttm_tt_create = hibmc_ttm_tt_create, >>>> + .ttm_tt_populate = hibmc_ttm_tt_populate, >>>> + .ttm_tt_unpopulate = hibmc_ttm_tt_unpopulate, >>>> + .init_mem_type = hibmc_bo_init_mem_type, >>>> + .evict_flags = hibmc_bo_evict_flags, >>>> + .move = NULL, >>>> + .verify_access = hibmc_bo_verify_access, >>>> + .io_mem_reserve = &hibmc_ttm_io_mem_reserve, >>>> + .io_mem_free = &hibmc_ttm_io_mem_free, >>>> + .lru_tail = &ttm_bo_default_lru_tail, >>>> + .swap_lru_tail = &ttm_bo_default_swap_lru_tail, >>>> +}; >>>> + >>>> +int hibmc_mm_init(struct hibmc_drm_device *hibmc) >>>> +{ >>>> + int ret; >>>> + struct drm_device *dev = hibmc->dev; >>>> + struct ttm_bo_device *bdev = &hibmc->ttm.bdev; >>>> + >>>> + ret = hibmc_ttm_global_init(hibmc); >>>> + if (ret) >>>> + return ret; >>>> + >>>> + ret = ttm_bo_device_init(&hibmc->ttm.bdev, >>>> + hibmc->ttm.bo_global_ref.ref.object, >>>> + &hibmc_bo_driver, >>>> + dev->anon_inode->i_mapping, >>>> + DRM_FILE_PAGE_OFFSET, >>>> + true); >>>> + if (ret) { >>> >>> >>> Call hibmc_ttm_global_release here? >> >> >> agreed, thanks for pointing it out. >> >>> >>>> + DRM_ERROR("Error initialising bo driver; %d\n", ret); >>>> + return ret; >>>> + } >>>> + >>>> + ret = ttm_bo_init_mm(bdev, TTM_PL_VRAM, >>>> + hibmc->fb_size >> PAGE_SHIFT); >>>> + if (ret) { >>> >>> >>> Clean up here as well? >> >> >> ditto >> >> >>> >>>> + DRM_ERROR("Failed ttm VRAM init: %d\n", ret); >>>> + return ret; >>>> + } >>>> + >>>> + hibmc->mm_inited = true; >>>> + return 0; >>>> +} >>>> + >>>> +void hibmc_mm_fini(struct hibmc_drm_device *hibmc) >>>> +{ >>>> + if (!hibmc->mm_inited) >>>> + return; >>>> + >>>> + ttm_bo_device_release(&hibmc->ttm.bdev); >>>> + hibmc_ttm_global_release(hibmc); >>>> + hibmc->mm_inited = false; >>>> +} >>>> + >>>> +int hibmc_bo_create(struct drm_device *dev, int size, int align, >>>> + u32 flags, struct hibmc_bo **phibmcbo) >>>> +{ >>>> + struct hibmc_drm_device *hibmc = dev->dev_private; >>>> + struct hibmc_bo *hibmcbo; >>>> + size_t acc_size; >>>> + int ret; >>>> + >>>> + hibmcbo = kzalloc(sizeof(*hibmcbo), GFP_KERNEL); >>>> + if (!hibmcbo) >>>> + return -ENOMEM; >>>> + >>>> + ret = drm_gem_object_init(dev, &hibmcbo->gem, size); >>>> + if (ret) { >>>> + kfree(hibmcbo); >>>> + return ret; >>>> + } >>>> + >>>> + hibmcbo->bo.bdev = &hibmc->ttm.bdev; >>>> + >>>> + hibmc_ttm_placement(hibmcbo, TTM_PL_FLAG_VRAM | >>>> TTM_PL_FLAG_SYSTEM); >>>> + >>>> + acc_size = ttm_bo_dma_acc_size(&hibmc->ttm.bdev, size, >>>> + sizeof(struct hibmc_bo)); >>>> + >>>> + ret = ttm_bo_init(&hibmc->ttm.bdev, &hibmcbo->bo, size, >>>> + ttm_bo_type_device, &hibmcbo->placement, >>>> + align >> PAGE_SHIFT, false, NULL, acc_size, >>>> + NULL, NULL, hibmc_bo_ttm_destroy); >>>> + if (ret) >>> >>> >>> Missing hibmcbo clean up here >> >> >> i looked at all other ttm drivers and all of them return directly when >> ttm_bo_init >> failed, however, i think it is better to clean up here, should i call >> hibmc_bo_unref(&hibmc_bo) here ? >> > > Yeah, that should work (might want to test it, though ;) > > >>> >>>> + return ret; >>>> + >>>> + *phibmcbo = hibmcbo; >>>> + return 0; >>>> +} >>>> + >>>> +static inline u64 hibmc_bo_gpu_offset(struct hibmc_bo *bo) >>>> +{ >>>> + return bo->bo.offset; >>>> +} >>> >>> >>> I don't think this function provides any value >> >> >> do you nean i use bo->bo.offset instead of calling hibmc_bo_gpu_offset()? >> > > yes > >>> >>>> + >>>> +int hibmc_bo_pin(struct hibmc_bo *bo, u32 pl_flag, u64 *gpu_addr) >>>> +{ >>>> + int i, ret; >>>> + >>>> + if (bo->pin_count) { >>>> + bo->pin_count++; >>>> + if (gpu_addr) >>>> + *gpu_addr = hibmc_bo_gpu_offset(bo); >>> >>> >>> Are you missing a return here? >> >> >> Thanks for pointing it out! >> >> >>> >>>> + } >>>> + >>>> + hibmc_ttm_placement(bo, pl_flag); >>>> + for (i = 0; i < bo->placement.num_placement; i++) >>>> + bo->placements[i].flags |= TTM_PL_FLAG_NO_EVICT; >>>> + ret = ttm_bo_validate(&bo->bo, &bo->placement, false, false); >>>> + if (ret) >>>> + return ret; >>>> + >>>> + bo->pin_count = 1; >>>> + if (gpu_addr) >>>> + *gpu_addr = hibmc_bo_gpu_offset(bo); >>>> + return 0; >>>> +} >>>> + >>>> +int hibmc_bo_push_sysram(struct hibmc_bo *bo) >>>> +{ >>>> + int i, ret; >>>> + >>>> + if (!bo->pin_count) { >>>> + DRM_ERROR("unpin bad %p\n", bo); >>>> + return 0; >>>> + } >>>> + bo->pin_count--; >>>> + if (bo->pin_count) >>>> + return 0; >>>> + >>>> + if (bo->kmap.virtual) >>> >>> >>> ttm_bo_kunmap already does this check so you don't have to >> >> >> agreed. will remove this condition. >> >>> >>>> + ttm_bo_kunmap(&bo->kmap); >>>> + >>>> + hibmc_ttm_placement(bo, TTM_PL_FLAG_SYSTEM); >>>> + for (i = 0; i < bo->placement.num_placement ; i++) >>>> + bo->placements[i].flags |= TTM_PL_FLAG_NO_EVICT; >>>> + >>>> + ret = ttm_bo_validate(&bo->bo, &bo->placement, false, false); >>>> + if (ret) { >>>> + DRM_ERROR("pushing to VRAM failed\n"); >>> >>> >>> Print ret >> >> >> ok, thanks. >> >> >>> >>>> + return ret; >>>> + } >>>> + return 0; >>>> +} >>>> + >>>> +int hibmc_mmap(struct file *filp, struct vm_area_struct *vma) >>>> +{ >>>> + struct drm_file *file_priv; >>>> + struct hibmc_drm_device *hibmc; >>>> + >>>> + if (unlikely(vma->vm_pgoff < DRM_FILE_PAGE_OFFSET)) >>>> + return -EINVAL; >>>> + >>>> + file_priv = filp->private_data; >>>> + hibmc = file_priv->minor->dev->dev_private; >>>> + return ttm_bo_mmap(filp, vma, &hibmc->ttm.bdev); >>>> +} >>>> + >>>> +int hibmc_gem_create(struct drm_device *dev, u32 size, bool iskernel, >>>> + struct drm_gem_object **obj) >>>> +{ >>>> + struct hibmc_bo *hibmcbo; >>>> + int ret; >>>> + >>>> + *obj = NULL; >>>> + >>>> + size = PAGE_ALIGN(size); >>>> + if (size == 0) >>> >>> >>> Print error >> >> >> ditto >> >>> >>>> + return -EINVAL; >>>> + >>>> + ret = hibmc_bo_create(dev, size, 0, 0, &hibmcbo); >>>> + if (ret) { >>>> + if (ret != -ERESTARTSYS) >>>> + DRM_ERROR("failed to allocate GEM object\n"); >>> >>> >>> Print ret >> >> >> ditto >> >>> >>>> + return ret; >>>> + } >>>> + *obj = &hibmcbo->gem; >>>> + return 0; >>>> +} >>>> + >>>> +int hibmc_dumb_create(struct drm_file *file, struct drm_device *dev, >>>> + struct drm_mode_create_dumb *args) >>>> +{ >>>> + struct drm_gem_object *gobj; >>>> + u32 handle; >>>> + int ret; >>>> + >>>> + args->pitch = ALIGN(args->width * ((args->bpp + 7) / 8), 16); >>> >>> >>> What's up with the bpp + 7 here? Perhaps you're looking for DIV_ROUND_UP? >> >> >> Yes, that sounds sane. >> > > sane is what i usually aim for :) > > Sean > > >>> >>> >>>> + args->size = args->pitch * args->height; >>>> + >>>> + ret = hibmc_gem_create(dev, args->size, false, >>>> + &gobj); >>>> + if (ret) >>>> + return ret; >>>> + >>>> + ret = drm_gem_handle_create(file, gobj, &handle); >>>> + drm_gem_object_unreference_unlocked(gobj); >>>> + if (ret) >>> >>> >>> Print error here >> >> >> agreed. >> >> >>> >>>> + return ret; >>>> + >>>> + args->handle = handle; >>>> + return 0; >>>> +} >>>> + >>>> +static void hibmc_bo_unref(struct hibmc_bo **bo) >>>> +{ >>>> + struct ttm_buffer_object *tbo; >>>> + >>>> + if ((*bo) == NULL) >>>> + return; >>>> + >>>> + tbo = &((*bo)->bo); >>>> + ttm_bo_unref(&tbo); >>>> + *bo = NULL; >>>> +} >>>> + >>>> +void hibmc_gem_free_object(struct drm_gem_object *obj) >>>> +{ >>>> + struct hibmc_bo *hibmcbo = gem_to_hibmc_bo(obj); >>>> + >>>> + hibmc_bo_unref(&hibmcbo); >>>> +} >>>> + >>>> +static u64 hibmc_bo_mmap_offset(struct hibmc_bo *bo) >>>> +{ >>>> + return drm_vma_node_offset_addr(&bo->bo.vma_node); >>>> +} >>>> + >>>> +int hibmc_dumb_mmap_offset(struct drm_file *file, struct drm_device >>>> *dev, >>>> + u32 handle, u64 *offset) >>>> +{ >>>> + struct drm_gem_object *obj; >>>> + struct hibmc_bo *bo; >>>> + >>>> + obj = drm_gem_object_lookup(file, handle); >>>> + if (!obj) >>>> + return -ENOENT; >>>> + >>>> + bo = gem_to_hibmc_bo(obj); >>>> + *offset = hibmc_bo_mmap_offset(bo); >>>> + >>>> + drm_gem_object_unreference_unlocked(obj); >>>> + return 0; >>>> +} >> >> >> Regards, >> Rongrong. >> >>>> -- >>>> 1.9.1 >>>> >>>> >>>> _______________________________________________ >>>> linux-arm-kernel mailing list >>>> linux-arm-kernel@lists.infradead.org >>>> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel >>> >>> _______________________________________________ >>> linuxarm mailing list >>> linuxarm@huawei.com >>> http://rnd-openeuler.huawei.com/mailman/listinfo/linuxarm >>> >>> . >>> >> >> >> _______________________________________________ >> linux-arm-kernel mailing list >> linux-arm-kernel@lists.infradead.org >> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel > > . >
diff --git a/drivers/gpu/drm/hisilicon/hibmc/Kconfig b/drivers/gpu/drm/hisilicon/hibmc/Kconfig index a9af90d..bcb8c18 100644 --- a/drivers/gpu/drm/hisilicon/hibmc/Kconfig +++ b/drivers/gpu/drm/hisilicon/hibmc/Kconfig @@ -1,6 +1,7 @@ config DRM_HISI_HIBMC tristate "DRM Support for Hisilicon Hibmc" depends on DRM && PCI + select DRM_TTM help Choose this option if you have a Hisilicon Hibmc soc chipset. diff --git a/drivers/gpu/drm/hisilicon/hibmc/Makefile b/drivers/gpu/drm/hisilicon/hibmc/Makefile index 97cf4a0..d5c40b8 100644 --- a/drivers/gpu/drm/hisilicon/hibmc/Makefile +++ b/drivers/gpu/drm/hisilicon/hibmc/Makefile @@ -1,5 +1,5 @@ ccflags-y := -Iinclude/drm -hibmc-drm-y := hibmc_drm_drv.o hibmc_drm_power.o +hibmc-drm-y := hibmc_drm_drv.o hibmc_drm_power.o hibmc_ttm.o obj-$(CONFIG_DRM_HISI_HIBMC) +=hibmc-drm.o #obj-y += hibmc-drm.o diff --git a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c index 4669d42..81f4301 100644 --- a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c +++ b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c @@ -31,6 +31,7 @@ #ifdef CONFIG_COMPAT .compat_ioctl = drm_compat_ioctl, #endif + .mmap = hibmc_mmap, .poll = drm_poll, .read = drm_read, .llseek = no_llseek, @@ -46,6 +47,8 @@ static void hibmc_disable_vblank(struct drm_device *dev, unsigned int pipe) } static struct drm_driver hibmc_driver = { + .driver_features = DRIVER_GEM, + .fops = &hibmc_fops, .name = "hibmc", .date = "20160828", @@ -55,6 +58,10 @@ static void hibmc_disable_vblank(struct drm_device *dev, unsigned int pipe) .get_vblank_counter = drm_vblank_no_hw_counter, .enable_vblank = hibmc_enable_vblank, .disable_vblank = hibmc_disable_vblank, + .gem_free_object_unlocked = hibmc_gem_free_object, + .dumb_create = hibmc_dumb_create, + .dumb_map_offset = hibmc_dumb_mmap_offset, + .dumb_destroy = drm_gem_dumb_destroy, }; static int hibmc_pm_suspend(struct device *dev) @@ -163,6 +170,7 @@ static int hibmc_unload(struct drm_device *dev) { struct hibmc_drm_device *hidev = dev->dev_private; + hibmc_mm_fini(hidev); hibmc_hw_fini(hidev); dev->dev_private = NULL; return 0; @@ -183,6 +191,10 @@ static int hibmc_load(struct drm_device *dev, unsigned long flags) if (ret) goto err; + ret = hibmc_mm_init(hidev); + if (ret) + goto err; + return 0; err: diff --git a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.h b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.h index 0037341..db8d80e 100644 --- a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.h +++ b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.h @@ -20,6 +20,8 @@ #define HIBMC_DRM_DRV_H #include <drm/drmP.h> +#include <drm/ttm/ttm_bo_driver.h> +#include <drm/drm_gem.h> struct hibmc_drm_device { /* hw */ @@ -30,6 +32,50 @@ struct hibmc_drm_device { /* drm */ struct drm_device *dev; + + /* ttm */ + struct { + struct drm_global_reference mem_global_ref; + struct ttm_bo_global_ref bo_global_ref; + struct ttm_bo_device bdev; + bool initialized; + } ttm; + + bool mm_inited; }; +struct hibmc_bo { + struct ttm_buffer_object bo; + struct ttm_placement placement; + struct ttm_bo_kmap_obj kmap; + struct drm_gem_object gem; + struct ttm_place placements[3]; + int pin_count; +}; + +static inline struct hibmc_bo *hibmc_bo(struct ttm_buffer_object *bo) +{ + return container_of(bo, struct hibmc_bo, bo); +} + +static inline struct hibmc_bo *gem_to_hibmc_bo(struct drm_gem_object *gem) +{ + return container_of(gem, struct hibmc_bo, gem); +} + +#define DRM_FILE_PAGE_OFFSET (0x100000000ULL >> PAGE_SHIFT) + +int hibmc_gem_create(struct drm_device *dev, u32 size, bool iskernel, + struct drm_gem_object **obj); + +int hibmc_mm_init(struct hibmc_drm_device *hibmc); +void hibmc_mm_fini(struct hibmc_drm_device *hibmc); +int hibmc_bo_pin(struct hibmc_bo *bo, u32 pl_flag, u64 *gpu_addr); +void hibmc_gem_free_object(struct drm_gem_object *obj); +int hibmc_dumb_create(struct drm_file *file, struct drm_device *dev, + struct drm_mode_create_dumb *args); +int hibmc_dumb_mmap_offset(struct drm_file *file, struct drm_device *dev, + u32 handle, u64 *offset); +int hibmc_mmap(struct file *filp, struct vm_area_struct *vma); + #endif diff --git a/drivers/gpu/drm/hisilicon/hibmc/hibmc_ttm.c b/drivers/gpu/drm/hisilicon/hibmc/hibmc_ttm.c new file mode 100644 index 0000000..0802ebd --- /dev/null +++ b/drivers/gpu/drm/hisilicon/hibmc/hibmc_ttm.c @@ -0,0 +1,490 @@ +/* Hisilicon Hibmc SoC drm driver + * + * Based on the bochs drm driver. + * + * Copyright (c) 2016 Huawei Limited. + * + * Author: + * Rongrong Zou <zourongrong@huawei.com> + * Rongrong Zou <zourongrong@gmail.com> + * Jianhua Li <lijianhua@huawei.com> + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation; either version 2 of the License, or + * (at your option) any later version. + * + */ + +#include "hibmc_drm_drv.h" +#include <ttm/ttm_page_alloc.h> +#include <drm/drm_crtc_helper.h> +#include <drm/drm_atomic_helper.h> + +static inline struct hibmc_drm_device * +hibmc_bdev(struct ttm_bo_device *bd) +{ + return container_of(bd, struct hibmc_drm_device, ttm.bdev); +} + +static int +hibmc_ttm_mem_global_init(struct drm_global_reference *ref) +{ + return ttm_mem_global_init(ref->object); +} + +static void +hibmc_ttm_mem_global_release(struct drm_global_reference *ref) +{ + ttm_mem_global_release(ref->object); +} + +static int hibmc_ttm_global_init(struct hibmc_drm_device *hibmc) +{ + struct drm_global_reference *global_ref; + int r; + + global_ref = &hibmc->ttm.mem_global_ref; + global_ref->global_type = DRM_GLOBAL_TTM_MEM; + global_ref->size = sizeof(struct ttm_mem_global); + global_ref->init = &hibmc_ttm_mem_global_init; + global_ref->release = &hibmc_ttm_mem_global_release; + r = drm_global_item_ref(global_ref); + if (r != 0) { + DRM_ERROR("Failed setting up TTM memory accounting subsystem.\n" + ); + return r; + } + + hibmc->ttm.bo_global_ref.mem_glob = + hibmc->ttm.mem_global_ref.object; + global_ref = &hibmc->ttm.bo_global_ref.ref; + global_ref->global_type = DRM_GLOBAL_TTM_BO; + global_ref->size = sizeof(struct ttm_bo_global); + global_ref->init = &ttm_bo_global_init; + global_ref->release = &ttm_bo_global_release; + r = drm_global_item_ref(global_ref); + if (r != 0) { + DRM_ERROR("Failed setting up TTM BO subsystem.\n"); + drm_global_item_unref(&hibmc->ttm.mem_global_ref); + return r; + } + return 0; +} + +static void +hibmc_ttm_global_release(struct hibmc_drm_device *hibmc) +{ + if (!hibmc->ttm.mem_global_ref.release) + return; + + drm_global_item_unref(&hibmc->ttm.bo_global_ref.ref); + drm_global_item_unref(&hibmc->ttm.mem_global_ref); + hibmc->ttm.mem_global_ref.release = NULL; +} + +static void hibmc_bo_ttm_destroy(struct ttm_buffer_object *tbo) +{ + struct hibmc_bo *bo; + + bo = container_of(tbo, struct hibmc_bo, bo); + + drm_gem_object_release(&bo->gem); + kfree(bo); +} + +static bool hibmc_ttm_bo_is_hibmc_bo(struct ttm_buffer_object *bo) +{ + if (bo->destroy == &hibmc_bo_ttm_destroy) + return true; + return false; +} + +static int +hibmc_bo_init_mem_type(struct ttm_bo_device *bdev, u32 type, + struct ttm_mem_type_manager *man) +{ + switch (type) { + case TTM_PL_SYSTEM: + man->flags = TTM_MEMTYPE_FLAG_MAPPABLE; + man->available_caching = TTM_PL_MASK_CACHING; + man->default_caching = TTM_PL_FLAG_CACHED; + break; + case TTM_PL_VRAM: + man->func = &ttm_bo_manager_func; + man->flags = TTM_MEMTYPE_FLAG_FIXED | + TTM_MEMTYPE_FLAG_MAPPABLE; + man->available_caching = TTM_PL_FLAG_UNCACHED | + TTM_PL_FLAG_WC; + man->default_caching = TTM_PL_FLAG_WC; + break; + default: + DRM_ERROR("Unsupported memory type %u\n", type); + return -EINVAL; + } + return 0; +} + +void hibmc_ttm_placement(struct hibmc_bo *bo, int domain) +{ + u32 c = 0; + u32 i; + + bo->placement.placement = bo->placements; + bo->placement.busy_placement = bo->placements; + if (domain & TTM_PL_FLAG_VRAM) + bo->placements[c++].flags = TTM_PL_FLAG_WC | + TTM_PL_FLAG_UNCACHED | TTM_PL_FLAG_VRAM; + if (domain & TTM_PL_FLAG_SYSTEM) + bo->placements[c++].flags = TTM_PL_MASK_CACHING | + TTM_PL_FLAG_SYSTEM; + if (!c) + bo->placements[c++].flags = TTM_PL_MASK_CACHING | + TTM_PL_FLAG_SYSTEM; + + bo->placement.num_placement = c; + bo->placement.num_busy_placement = c; + for (i = 0; i < c; ++i) { + bo->placements[i].fpfn = 0; + bo->placements[i].lpfn = 0; + } +} + +static void +hibmc_bo_evict_flags(struct ttm_buffer_object *bo, struct ttm_placement *pl) +{ + struct hibmc_bo *hibmcbo = hibmc_bo(bo); + + if (!hibmc_ttm_bo_is_hibmc_bo(bo)) + return; + + hibmc_ttm_placement(hibmcbo, TTM_PL_FLAG_SYSTEM); + *pl = hibmcbo->placement; +} + +static int hibmc_bo_verify_access(struct ttm_buffer_object *bo, + struct file *filp) +{ + struct hibmc_bo *hibmcbo = hibmc_bo(bo); + + return drm_vma_node_verify_access(&hibmcbo->gem.vma_node, + filp->private_data); +} + +static int hibmc_ttm_io_mem_reserve(struct ttm_bo_device *bdev, + struct ttm_mem_reg *mem) +{ + struct ttm_mem_type_manager *man = &bdev->man[mem->mem_type]; + struct hibmc_drm_device *hibmc = hibmc_bdev(bdev); + + mem->bus.addr = NULL; + mem->bus.offset = 0; + mem->bus.size = mem->num_pages << PAGE_SHIFT; + mem->bus.base = 0; + mem->bus.is_iomem = false; + if (!(man->flags & TTM_MEMTYPE_FLAG_MAPPABLE)) + return -EINVAL; + switch (mem->mem_type) { + case TTM_PL_SYSTEM: + /* system memory */ + return 0; + case TTM_PL_VRAM: + mem->bus.offset = mem->start << PAGE_SHIFT; + mem->bus.base = pci_resource_start(hibmc->dev->pdev, 0); + mem->bus.is_iomem = true; + break; + default: + return -EINVAL; + } + return 0; +} + +static void hibmc_ttm_io_mem_free(struct ttm_bo_device *bdev, + struct ttm_mem_reg *mem) +{ +} + +static void hibmc_ttm_backend_destroy(struct ttm_tt *tt) +{ + ttm_tt_fini(tt); + kfree(tt); +} + +static struct ttm_backend_func hibmc_tt_backend_func = { + .destroy = &hibmc_ttm_backend_destroy, +}; + +static struct ttm_tt *hibmc_ttm_tt_create(struct ttm_bo_device *bdev, + unsigned long size, + u32 page_flags, + struct page *dummy_read_page) +{ + struct ttm_tt *tt; + + tt = kzalloc(sizeof(*tt), GFP_KERNEL); + if (!tt) + return NULL; + tt->func = &hibmc_tt_backend_func; + if (ttm_tt_init(tt, bdev, size, page_flags, dummy_read_page)) { + kfree(tt); + return NULL; + } + return tt; +} + +static int hibmc_ttm_tt_populate(struct ttm_tt *ttm) +{ + return ttm_pool_populate(ttm); +} + +static void hibmc_ttm_tt_unpopulate(struct ttm_tt *ttm) +{ + ttm_pool_unpopulate(ttm); +} + +struct ttm_bo_driver hibmc_bo_driver = { + .ttm_tt_create = hibmc_ttm_tt_create, + .ttm_tt_populate = hibmc_ttm_tt_populate, + .ttm_tt_unpopulate = hibmc_ttm_tt_unpopulate, + .init_mem_type = hibmc_bo_init_mem_type, + .evict_flags = hibmc_bo_evict_flags, + .move = NULL, + .verify_access = hibmc_bo_verify_access, + .io_mem_reserve = &hibmc_ttm_io_mem_reserve, + .io_mem_free = &hibmc_ttm_io_mem_free, + .lru_tail = &ttm_bo_default_lru_tail, + .swap_lru_tail = &ttm_bo_default_swap_lru_tail, +}; + +int hibmc_mm_init(struct hibmc_drm_device *hibmc) +{ + int ret; + struct drm_device *dev = hibmc->dev; + struct ttm_bo_device *bdev = &hibmc->ttm.bdev; + + ret = hibmc_ttm_global_init(hibmc); + if (ret) + return ret; + + ret = ttm_bo_device_init(&hibmc->ttm.bdev, + hibmc->ttm.bo_global_ref.ref.object, + &hibmc_bo_driver, + dev->anon_inode->i_mapping, + DRM_FILE_PAGE_OFFSET, + true); + if (ret) { + DRM_ERROR("Error initialising bo driver; %d\n", ret); + return ret; + } + + ret = ttm_bo_init_mm(bdev, TTM_PL_VRAM, + hibmc->fb_size >> PAGE_SHIFT); + if (ret) { + DRM_ERROR("Failed ttm VRAM init: %d\n", ret); + return ret; + } + + hibmc->mm_inited = true; + return 0; +} + +void hibmc_mm_fini(struct hibmc_drm_device *hibmc) +{ + if (!hibmc->mm_inited) + return; + + ttm_bo_device_release(&hibmc->ttm.bdev); + hibmc_ttm_global_release(hibmc); + hibmc->mm_inited = false; +} + +int hibmc_bo_create(struct drm_device *dev, int size, int align, + u32 flags, struct hibmc_bo **phibmcbo) +{ + struct hibmc_drm_device *hibmc = dev->dev_private; + struct hibmc_bo *hibmcbo; + size_t acc_size; + int ret; + + hibmcbo = kzalloc(sizeof(*hibmcbo), GFP_KERNEL); + if (!hibmcbo) + return -ENOMEM; + + ret = drm_gem_object_init(dev, &hibmcbo->gem, size); + if (ret) { + kfree(hibmcbo); + return ret; + } + + hibmcbo->bo.bdev = &hibmc->ttm.bdev; + + hibmc_ttm_placement(hibmcbo, TTM_PL_FLAG_VRAM | TTM_PL_FLAG_SYSTEM); + + acc_size = ttm_bo_dma_acc_size(&hibmc->ttm.bdev, size, + sizeof(struct hibmc_bo)); + + ret = ttm_bo_init(&hibmc->ttm.bdev, &hibmcbo->bo, size, + ttm_bo_type_device, &hibmcbo->placement, + align >> PAGE_SHIFT, false, NULL, acc_size, + NULL, NULL, hibmc_bo_ttm_destroy); + if (ret) + return ret; + + *phibmcbo = hibmcbo; + return 0; +} + +static inline u64 hibmc_bo_gpu_offset(struct hibmc_bo *bo) +{ + return bo->bo.offset; +} + +int hibmc_bo_pin(struct hibmc_bo *bo, u32 pl_flag, u64 *gpu_addr) +{ + int i, ret; + + if (bo->pin_count) { + bo->pin_count++; + if (gpu_addr) + *gpu_addr = hibmc_bo_gpu_offset(bo); + } + + hibmc_ttm_placement(bo, pl_flag); + for (i = 0; i < bo->placement.num_placement; i++) + bo->placements[i].flags |= TTM_PL_FLAG_NO_EVICT; + ret = ttm_bo_validate(&bo->bo, &bo->placement, false, false); + if (ret) + return ret; + + bo->pin_count = 1; + if (gpu_addr) + *gpu_addr = hibmc_bo_gpu_offset(bo); + return 0; +} + +int hibmc_bo_push_sysram(struct hibmc_bo *bo) +{ + int i, ret; + + if (!bo->pin_count) { + DRM_ERROR("unpin bad %p\n", bo); + return 0; + } + bo->pin_count--; + if (bo->pin_count) + return 0; + + if (bo->kmap.virtual) + ttm_bo_kunmap(&bo->kmap); + + hibmc_ttm_placement(bo, TTM_PL_FLAG_SYSTEM); + for (i = 0; i < bo->placement.num_placement ; i++) + bo->placements[i].flags |= TTM_PL_FLAG_NO_EVICT; + + ret = ttm_bo_validate(&bo->bo, &bo->placement, false, false); + if (ret) { + DRM_ERROR("pushing to VRAM failed\n"); + return ret; + } + return 0; +} + +int hibmc_mmap(struct file *filp, struct vm_area_struct *vma) +{ + struct drm_file *file_priv; + struct hibmc_drm_device *hibmc; + + if (unlikely(vma->vm_pgoff < DRM_FILE_PAGE_OFFSET)) + return -EINVAL; + + file_priv = filp->private_data; + hibmc = file_priv->minor->dev->dev_private; + return ttm_bo_mmap(filp, vma, &hibmc->ttm.bdev); +} + +int hibmc_gem_create(struct drm_device *dev, u32 size, bool iskernel, + struct drm_gem_object **obj) +{ + struct hibmc_bo *hibmcbo; + int ret; + + *obj = NULL; + + size = PAGE_ALIGN(size); + if (size == 0) + return -EINVAL; + + ret = hibmc_bo_create(dev, size, 0, 0, &hibmcbo); + if (ret) { + if (ret != -ERESTARTSYS) + DRM_ERROR("failed to allocate GEM object\n"); + return ret; + } + *obj = &hibmcbo->gem; + return 0; +} + +int hibmc_dumb_create(struct drm_file *file, struct drm_device *dev, + struct drm_mode_create_dumb *args) +{ + struct drm_gem_object *gobj; + u32 handle; + int ret; + + args->pitch = ALIGN(args->width * ((args->bpp + 7) / 8), 16); + args->size = args->pitch * args->height; + + ret = hibmc_gem_create(dev, args->size, false, + &gobj); + if (ret) + return ret; + + ret = drm_gem_handle_create(file, gobj, &handle); + drm_gem_object_unreference_unlocked(gobj); + if (ret) + return ret; + + args->handle = handle; + return 0; +} + +static void hibmc_bo_unref(struct hibmc_bo **bo) +{ + struct ttm_buffer_object *tbo; + + if ((*bo) == NULL) + return; + + tbo = &((*bo)->bo); + ttm_bo_unref(&tbo); + *bo = NULL; +} + +void hibmc_gem_free_object(struct drm_gem_object *obj) +{ + struct hibmc_bo *hibmcbo = gem_to_hibmc_bo(obj); + + hibmc_bo_unref(&hibmcbo); +} + +static u64 hibmc_bo_mmap_offset(struct hibmc_bo *bo) +{ + return drm_vma_node_offset_addr(&bo->bo.vma_node); +} + +int hibmc_dumb_mmap_offset(struct drm_file *file, struct drm_device *dev, + u32 handle, u64 *offset) +{ + struct drm_gem_object *obj; + struct hibmc_bo *bo; + + obj = drm_gem_object_lookup(file, handle); + if (!obj) + return -ENOENT; + + bo = gem_to_hibmc_bo(obj); + *offset = hibmc_bo_mmap_offset(bo); + + drm_gem_object_unreference_unlocked(obj); + return 0; +}
Hibmc have 32m video memory which can be accessed through PCIe by host, we use ttm to manage these memory. Signed-off-by: Rongrong Zou <zourongrong@gmail.com> --- drivers/gpu/drm/hisilicon/hibmc/Kconfig | 1 + drivers/gpu/drm/hisilicon/hibmc/Makefile | 2 +- drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c | 12 + drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.h | 46 +++ drivers/gpu/drm/hisilicon/hibmc/hibmc_ttm.c | 490 ++++++++++++++++++++++++ 5 files changed, 550 insertions(+), 1 deletion(-) create mode 100644 drivers/gpu/drm/hisilicon/hibmc/hibmc_ttm.c