On Wed, 2019-04-17 at 15:50 +0200, Lucas Stach wrote:
> There is no need for each GPU to have it's own cmdbuf suballocation
> region. Only allocate a single one for the the etnaviv virtual device
> and share it across all GPUs.
>
> Signed-off-by: Lucas Stach <l.stach@xxxxxxxxxxxxxx>
> ---
> drivers/gpu/drm/etnaviv/etnaviv_cmdbuf.c | 14 ++++++------
> drivers/gpu/drm/etnaviv/etnaviv_cmdbuf.h | 4 ++--
> drivers/gpu/drm/etnaviv/etnaviv_drv.c | 11 +++++++++-
> drivers/gpu/drm/etnaviv/etnaviv_drv.h | 2 ++
> drivers/gpu/drm/etnaviv/etnaviv_gem_submit.c | 2 +-
> drivers/gpu/drm/etnaviv/etnaviv_gpu.c | 23 ++++----------------
> drivers/gpu/drm/etnaviv/etnaviv_gpu.h | 3 +--
> 7 files changed, 27 insertions(+), 32 deletions(-)
>
> diff --git a/drivers/gpu/drm/etnaviv/etnaviv_cmdbuf.c b/drivers/gpu/drm/etnaviv/etnaviv_cmdbuf.c
> index 1bc529399783..a01ae32dcd88 100644
> --- a/drivers/gpu/drm/etnaviv/etnaviv_cmdbuf.c
> +++ b/drivers/gpu/drm/etnaviv/etnaviv_cmdbuf.c
> @@ -10,13 +10,13 @@
> #include "etnaviv_gpu.h"
> #include "etnaviv_mmu.h"
>
> -#define SUBALLOC_SIZE SZ_256K
> +#define SUBALLOC_SIZE SZ_512K
> #define SUBALLOC_GRANULE SZ_4K
> #define SUBALLOC_GRANULES (SUBALLOC_SIZE / SUBALLOC_GRANULE)
>
> struct etnaviv_cmdbuf_suballoc {
> /* suballocated dma buffer properties */
> - struct etnaviv_gpu *gpu;
> + struct device *dev;
> void *vaddr;
> dma_addr_t paddr;
>
> @@ -28,7 +28,7 @@ struct etnaviv_cmdbuf_suballoc {
> };
>
> struct etnaviv_cmdbuf_suballoc *
> -etnaviv_cmdbuf_suballoc_new(struct etnaviv_gpu * gpu)
> +etnaviv_cmdbuf_suballoc_new(struct device *dev)
> {
> struct etnaviv_cmdbuf_suballoc *suballoc;
> int ret;
> @@ -37,11 +37,11 @@ etnaviv_cmdbuf_suballoc_new(struct etnaviv_gpu * gpu)
> if (!suballoc)
> return ERR_PTR(-ENOMEM);
>
> - suballoc->gpu = gpu;
> + suballoc->dev = dev;
> mutex_init(&suballoc->lock);
> init_waitqueue_head(&suballoc->free_event);
>
> - suballoc->vaddr = dma_alloc_wc(gpu->dev, SUBALLOC_SIZE,
> + suballoc->vaddr = dma_alloc_wc(dev, SUBALLOC_SIZE,
> &suballoc->paddr, GFP_KERNEL);
> if (!suballoc->vaddr) {
> ret = -ENOMEM;
> @@ -73,7 +73,7 @@ void etnaviv_cmdbuf_suballoc_unmap(struct etnaviv_iommu *mmu,
>
> void etnaviv_cmdbuf_suballoc_destroy(struct etnaviv_cmdbuf_suballoc *suballoc)
> {
> - dma_free_wc(suballoc->gpu->dev, SUBALLOC_SIZE, suballoc->vaddr,
> + dma_free_wc(suballoc->dev, SUBALLOC_SIZE, suballoc->vaddr,
> suballoc->paddr);
> kfree(suballoc);
> }
> @@ -98,7 +98,7 @@ int etnaviv_cmdbuf_init(struct etnaviv_cmdbuf_suballoc *suballoc,
> suballoc->free_space,
> msecs_to_jiffies(10 * 1000));
> if (!ret) {
> - dev_err(suballoc->gpu->dev,
> + dev_err(suballoc->dev,
> "Timeout waiting for cmdbuf space\n");
> return -ETIMEDOUT;
> }
> diff --git a/drivers/gpu/drm/etnaviv/etnaviv_cmdbuf.h b/drivers/gpu/drm/etnaviv/etnaviv_cmdbuf.h
> index 11d95f05c017..7fdc2e3fea5f 100644
> --- a/drivers/gpu/drm/etnaviv/etnaviv_cmdbuf.h
> +++ b/drivers/gpu/drm/etnaviv/etnaviv_cmdbuf.h
> @@ -8,7 +8,7 @@
>
> #include <linux/types.h>
>
> -struct etnaviv_gpu;
> +struct device;
> struct etnaviv_iommu;
> struct etnaviv_vram_mapping;
> struct etnaviv_cmdbuf_suballoc;
> @@ -24,7 +24,7 @@ struct etnaviv_cmdbuf {
> };
>
> struct etnaviv_cmdbuf_suballoc *
> -etnaviv_cmdbuf_suballoc_new(struct etnaviv_gpu * gpu);
> +etnaviv_cmdbuf_suballoc_new(struct device *dev);
> void etnaviv_cmdbuf_suballoc_destroy(struct etnaviv_cmdbuf_suballoc *suballoc);
> int etnaviv_cmdbuf_suballoc_map(struct etnaviv_cmdbuf_suballoc *suballoc,
> struct etnaviv_iommu *mmu,
> diff --git a/drivers/gpu/drm/etnaviv/etnaviv_drv.c b/drivers/gpu/drm/etnaviv/etnaviv_drv.c
> index 3156450723ba..138025bc5376 100644
> --- a/drivers/gpu/drm/etnaviv/etnaviv_drv.c
> +++ b/drivers/gpu/drm/etnaviv/etnaviv_drv.c
> @@ -30,7 +30,7 @@ static void load_gpu(struct drm_device *dev)
> if (g) {
> int ret;
>
> - ret = etnaviv_gpu_init(g);
> + ret = etnaviv_gpu_init(priv, g);
> if (ret)
> priv->gpu[i] = NULL;
> }
> @@ -522,6 +522,13 @@ static int etnaviv_bind(struct device *dev)
> INIT_LIST_HEAD(&priv->gem_list);
> priv->num_gpus = 0;
>
> + priv->cmdbuf_suballoc = etnaviv_cmdbuf_suballoc_new(drm->dev);
> + if (IS_ERR(priv->cmdbuf_suballoc)) {
> + dev_err(drm->dev, "Failed to create cmdbuf suballocator\n");
> + ret = PTR_ERR(priv->cmdbuf_suballoc);
> + goto out_put;
> + }
> +
> dev_set_drvdata(dev, drm);
>
> ret = component_bind_all(dev, drm);
This looks like it is missing an etnaviv_cmdbuf_suballoc_destroy in the
error path for component_bind_all and drm_dev_register.
> @@ -555,6 +562,8 @@ static void etnaviv_unbind(struct device *dev)
>
> component_unbind_all(dev, drm);
>
> + etnaviv_cmdbuf_suballoc_destroy(priv->cmdbuf_suballoc);
> +
> dev->dma_parms = NULL;
>
> drm->dev_private = NULL;
> diff --git a/drivers/gpu/drm/etnaviv/etnaviv_drv.h b/drivers/gpu/drm/etnaviv/etnaviv_drv.h
> index 6a4ea127c4f1..0291771e72fa 100644
> --- a/drivers/gpu/drm/etnaviv/etnaviv_drv.h
> +++ b/drivers/gpu/drm/etnaviv/etnaviv_drv.h
> @@ -45,6 +45,8 @@ struct etnaviv_drm_private {
> struct device_dma_parameters dma_parms;
> struct etnaviv_gpu *gpu[ETNA_MAX_PIPES];
>
> + struct etnaviv_cmdbuf_suballoc *cmdbuf_suballoc;
> +
> /* list of GEM objects: */
> struct mutex gem_lock;
> struct list_head gem_list;
> diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gem_submit.c b/drivers/gpu/drm/etnaviv/etnaviv_gem_submit.c
> index b2fe3446bfbc..f5b3dfa312b7 100644
> --- a/drivers/gpu/drm/etnaviv/etnaviv_gem_submit.c
> +++ b/drivers/gpu/drm/etnaviv/etnaviv_gem_submit.c
> @@ -501,7 +501,7 @@ int etnaviv_ioctl_gem_submit(struct drm_device *dev, void *data,
> goto err_submit_ww_acquire;
> }
>
> - ret = etnaviv_cmdbuf_init(gpu->cmdbuf_suballoc, &submit->cmdbuf,
> + ret = etnaviv_cmdbuf_init(priv->cmdbuf_suballoc, &submit->cmdbuf,
> ALIGN(args->stream_size, 8) + 8);
> if (ret)
> goto err_submit_objects;
> diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gpu.c b/drivers/gpu/drm/etnaviv/etnaviv_gpu.c
> index a70ff4c77a8a..a5eed14cec8d 100644
> --- a/drivers/gpu/drm/etnaviv/etnaviv_gpu.c
> +++ b/drivers/gpu/drm/etnaviv/etnaviv_gpu.c
> @@ -687,7 +687,7 @@ static void etnaviv_gpu_hw_init(struct etnaviv_gpu *gpu)
> &gpu->cmdbuf_mapping), prefetch);
> }
>
> -int etnaviv_gpu_init(struct etnaviv_gpu *gpu)
> +int etnaviv_gpu_init(struct etnaviv_drm_private *priv, struct etnaviv_gpu *gpu)
Isn't passing etnaviv_drm_private into etnaviv_gpu_init kind of a layer
violation? I understand this isn't only about cmdbuf_suballoc, and
mmu_global will be added later, but without context this looks a bit
weird.
> {
> int ret, i;
>
> @@ -756,23 +756,16 @@ int etnaviv_gpu_init(struct etnaviv_gpu *gpu)
> goto fail;
> }
>
> - gpu->cmdbuf_suballoc = etnaviv_cmdbuf_suballoc_new(gpu);
> - if (IS_ERR(gpu->cmdbuf_suballoc)) {
> - dev_err(gpu->dev, "Failed to create cmdbuf suballocator\n");
> - ret = PTR_ERR(gpu->cmdbuf_suballoc);
> - goto destroy_iommu;
> - }
> -
> - ret = etnaviv_cmdbuf_suballoc_map(gpu->cmdbuf_suballoc, gpu->mmu,
> + ret = etnaviv_cmdbuf_suballoc_map(priv->cmdbuf_suballoc, gpu->mmu,
> &gpu->cmdbuf_mapping,
> gpu->memory_base);
> if (ret) {
> dev_err(gpu->dev, "failed to map cmdbuf suballoc\n");
> - goto destroy_suballoc;
> + goto destroy_iommu;
> }
>
> /* Create buffer: */
> - ret = etnaviv_cmdbuf_init(gpu->cmdbuf_suballoc, &gpu->buffer,
> + ret = etnaviv_cmdbuf_init(priv->cmdbuf_suballoc, &gpu->buffer,
> PAGE_SIZE);
> if (ret) {
> dev_err(gpu->dev, "could not create command buffer\n");
> @@ -810,9 +803,6 @@ int etnaviv_gpu_init(struct etnaviv_gpu *gpu)
> gpu->buffer.suballoc = NULL;
> unmap_suballoc:
> etnaviv_cmdbuf_suballoc_unmap(gpu->mmu, &gpu->cmdbuf_mapping);
> -destroy_suballoc:
> - etnaviv_cmdbuf_suballoc_destroy(gpu->cmdbuf_suballoc);
> - gpu->cmdbuf_suballoc = NULL;
> destroy_iommu:
> etnaviv_iommu_destroy(gpu->mmu);
> gpu->mmu = NULL;
> @@ -1692,11 +1682,6 @@ static void etnaviv_gpu_unbind(struct device *dev, struct device *master,
> if (gpu->cmdbuf_mapping.use == 1)
> etnaviv_cmdbuf_suballoc_unmap(gpu->mmu, &gpu->cmdbuf_mapping);
>
> - if (gpu->cmdbuf_suballoc) {
> - etnaviv_cmdbuf_suballoc_destroy(gpu->cmdbuf_suballoc);
> - gpu->cmdbuf_suballoc = NULL;
> - }
> -
> if (gpu->mmu) {
> etnaviv_iommu_destroy(gpu->mmu);
> gpu->mmu = NULL;
> diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gpu.h b/drivers/gpu/drm/etnaviv/etnaviv_gpu.h
> index 8c68869ba180..004f2cdfb4e0 100644
> --- a/drivers/gpu/drm/etnaviv/etnaviv_gpu.h
> +++ b/drivers/gpu/drm/etnaviv/etnaviv_gpu.h
> @@ -136,7 +136,6 @@ struct etnaviv_gpu {
> int irq;
>
> struct etnaviv_iommu *mmu;
> - struct etnaviv_cmdbuf_suballoc *cmdbuf_suballoc;
>
> /* Power Control: */
> struct clk *clk_bus;
> @@ -161,7 +160,7 @@ static inline u32 gpu_read(struct etnaviv_gpu *gpu, u32 reg)
>
> int etnaviv_gpu_get_param(struct etnaviv_gpu *gpu, u32 param, u64 *value);
>
> -int etnaviv_gpu_init(struct etnaviv_gpu *gpu);
> +int etnaviv_gpu_init(struct etnaviv_drm_private *priv, struct etnaviv_gpu *gpu);
> bool etnaviv_fill_identity_from_hwdb(struct etnaviv_gpu *gpu);
>
> #ifdef CONFIG_DEBUG_FS
regards
Philipp
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
