From: Philipp Zabel <p.zabel@pengutronix.de>
To: Lucas Stach <l.stach@pengutronix.de>, etnaviv@lists.freedesktop.org
Cc: Russell King <linux+etnaviv@armlinux.org.uk>,
dri-devel@lists.freedesktop.org, kernel@pengutronix.de,
patchwork-lst@pengutronix.de
Subject: Re: [PATCH v2 2/8] drm/etnaviv: share a single cmdbuf suballoc region across all GPUs
Date: Tue, 23 Apr 2019 19:04:24 +0200 [thread overview]
Message-ID: <1556039064.3043.11.camel@pengutronix.de> (raw)
In-Reply-To: <20190417135023.26977-3-l.stach@pengutronix.de>
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@pengutronix.de>
> ---
> 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@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
next prev parent reply other threads:[~2019-04-23 17:04 UTC|newest]
Thread overview: 17+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-04-17 13:50 [PATCH v2 0/8] per-process address spaces for MMUv2 Lucas Stach
2019-04-17 13:50 ` [PATCH v2 1/8] drm/etnaviv: split out cmdbuf mapping into address space Lucas Stach
2019-04-23 16:44 ` Philipp Zabel
2019-04-17 13:50 ` [PATCH v2 2/8] drm/etnaviv: share a single cmdbuf suballoc region across all GPUs Lucas Stach
2019-04-23 17:04 ` Philipp Zabel [this message]
2019-04-17 13:50 ` [PATCH v2 3/8] drm/etnaviv: replace MMU flush marker with flush sequence Lucas Stach
2019-04-23 17:13 ` Philipp Zabel
2019-04-17 13:50 ` [PATCH v2 4/8] drm/etnaviv: rework MMU handling Lucas Stach
2019-05-03 10:29 ` Guido Günther
2019-04-17 13:50 ` [PATCH v2 5/8] drm/etnaviv: split out starting of FE idle loop Lucas Stach
2019-04-25 9:30 ` Philipp Zabel
2019-04-17 13:50 ` [PATCH v2 6/8] drm/etnaviv: provide MMU context to etnaviv_gem_mapping_get Lucas Stach
2019-04-25 9:35 ` Philipp Zabel
2019-04-17 13:50 ` [PATCH v2 7/8] drm/etnaviv: implement per-process address spaces on MMUv2 Lucas Stach
2019-04-17 13:50 ` [PATCH v2 8/8] drm/etnaviv: dump only failing submit Lucas Stach
2019-05-03 11:10 ` [PATCH v2 0/8] per-process address spaces for MMUv2 Guido Günther
2019-05-10 15:07 ` Guido Günther
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=1556039064.3043.11.camel@pengutronix.de \
--to=p.zabel@pengutronix.de \
--cc=dri-devel@lists.freedesktop.org \
--cc=etnaviv@lists.freedesktop.org \
--cc=kernel@pengutronix.de \
--cc=l.stach@pengutronix.de \
--cc=linux+etnaviv@armlinux.org.uk \
--cc=patchwork-lst@pengutronix.de \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.