[PATCH v4 0/4] drm/vc4: Binner BO management improvements

[PATCH v4 0/4] drm/vc4: Binner BO management improvements

[Paul Kocialkowski]

Changes since v3:
* Split changes into more commits when possible;
* Reworked binner bo alloc condition as discussed.

Changes since v2:
* Removed deprecated sentence about fristopen;
* Added collected Reviewed-By tags.

Changes since v1:
* Squashed the two final patches into one.

Paul Kocialkowski (4):
  drm/vc4: Reformat and export binner bo allocation helper
  drm/vc4: Check for V3D before binner bo alloc
  drm/vc4: Check for the binner bo before handling OOM interrupt
  drm/vc4: Allocate binner bo when starting to use the V3D

 drivers/gpu/drm/vc4/vc4_bo.c  | 32 ++++++++++++++++++++++++++++++++
 drivers/gpu/drm/vc4/vc4_drv.c |  9 +++++++++
 drivers/gpu/drm/vc4/vc4_drv.h |  5 +++++
 drivers/gpu/drm/vc4/vc4_irq.c |  3 +++
 drivers/gpu/drm/vc4/vc4_v3d.c | 23 ++++++-----------------
 5 files changed, 55 insertions(+), 17 deletions(-)

-- 
2.21.0

[PATCH v4 0/4] drm/vc4: Binner BO management improvements

[Paul Kocialkowski]

Changes since v3:
* Split changes into more commits when possible;
* Reworked binner bo alloc condition as discussed.

Changes since v2:
* Removed deprecated sentence about fristopen;
* Added collected Reviewed-By tags.

Changes since v1:
* Squashed the two final patches into one.

Paul Kocialkowski (4):
  drm/vc4: Reformat and export binner bo allocation helper
  drm/vc4: Check for V3D before binner bo alloc
  drm/vc4: Check for the binner bo before handling OOM interrupt
  drm/vc4: Allocate binner bo when starting to use the V3D

 drivers/gpu/drm/vc4/vc4_bo.c  | 32 ++++++++++++++++++++++++++++++++
 drivers/gpu/drm/vc4/vc4_drv.c |  9 +++++++++
 drivers/gpu/drm/vc4/vc4_drv.h |  5 +++++
 drivers/gpu/drm/vc4/vc4_irq.c |  3 +++
 drivers/gpu/drm/vc4/vc4_v3d.c | 23 ++++++-----------------
 5 files changed, 55 insertions(+), 17 deletions(-)

-- 
2.21.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v4 1/4] drm/vc4: Reformat and export binner bo allocation helper

[Paul Kocialkowski]

Since we'll be using the binner bo allocation helper in other parts
of the driver, reformat it with a vc4_v3d prefix and pass the vc4 dev
directly to match other functions.

Make the function visible to the whole driver too.

Signed-off-by: Paul Kocialkowski <paul.kocialkowski@bootlin.com>
---
 drivers/gpu/drm/vc4/vc4_v3d.c | 11 +++++------
 1 file changed, 5 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/vc4/vc4_v3d.c b/drivers/gpu/drm/vc4/vc4_v3d.c
index a4b6859e3af6..fe091256800d 100644
--- a/drivers/gpu/drm/vc4/vc4_v3d.c
+++ b/drivers/gpu/drm/vc4/vc4_v3d.c
@@ -213,7 +213,7 @@ int vc4_v3d_get_bin_slot(struct vc4_dev *vc4)
 }
 
 /**
- * vc4_allocate_bin_bo() - allocates the memory that will be used for
+ * vc4_v3d_allocate_bin_bo() - allocates the memory that will be used for
  * tile binning.
  *
  * The binner has a limitation that the addresses in the tile state
@@ -234,9 +234,8 @@ int vc4_v3d_get_bin_slot(struct vc4_dev *vc4)
  * overall CMA pool before they make scenes complicated enough to run
  * out of bin space.
  */
-static int vc4_allocate_bin_bo(struct drm_device *drm)
+int vc4_v3d_allocate_bin_bo(struct vc4_dev *vc4)
 {
-	struct vc4_dev *vc4 = to_vc4_dev(drm);
 	struct vc4_v3d *v3d = vc4->v3d;
 	uint32_t size = 16 * 1024 * 1024;
 	int ret = 0;
@@ -251,7 +250,7 @@ static int vc4_allocate_bin_bo(struct drm_device *drm)
 	INIT_LIST_HEAD(&list);
 
 	while (true) {
-		struct vc4_bo *bo = vc4_bo_create(drm, size, true,
+		struct vc4_bo *bo = vc4_bo_create(vc4->dev, size, true,
 						  VC4_BO_TYPE_BIN);
 
 		if (IS_ERR(bo)) {
@@ -333,7 +332,7 @@ static int vc4_v3d_runtime_resume(struct device *dev)
 	struct vc4_dev *vc4 = v3d->vc4;
 	int ret;
 
-	ret = vc4_allocate_bin_bo(vc4->dev);
+	ret = vc4_v3d_allocate_bin_bo(vc4);
 	if (ret)
 		return ret;
 
@@ -403,7 +402,7 @@ static int vc4_v3d_bind(struct device *dev, struct device *master, void *data)
 	if (ret != 0)
 		return ret;
 
-	ret = vc4_allocate_bin_bo(drm);
+	ret = vc4_v3d_allocate_bin_bo(vc4);
 	if (ret) {
 		clk_disable_unprepare(v3d->clk);
 		return ret;
-- 
2.21.0

[PATCH v4 2/4] drm/vc4: Check for V3D before binner bo alloc

[Paul Kocialkowski]

Check that we have a V3D device registered before attempting to
allocate a binner buffer object.

Signed-off-by: Paul Kocialkowski <paul.kocialkowski@bootlin.com>
---
 drivers/gpu/drm/vc4/vc4_drv.h | 1 +
 drivers/gpu/drm/vc4/vc4_v3d.c | 3 +++
 2 files changed, 4 insertions(+)

diff --git a/drivers/gpu/drm/vc4/vc4_drv.h b/drivers/gpu/drm/vc4/vc4_drv.h
index e61734af059b..37941f0e212a 100644
--- a/drivers/gpu/drm/vc4/vc4_drv.h
+++ b/drivers/gpu/drm/vc4/vc4_drv.h
@@ -833,6 +833,7 @@ void vc4_plane_async_set_fb(struct drm_plane *plane,
 extern struct platform_driver vc4_v3d_driver;
 extern const struct of_device_id vc4_v3d_dt_match[];
 int vc4_v3d_get_bin_slot(struct vc4_dev *vc4);
+int vc4_v3d_allocate_bin_bo(struct vc4_dev *vc4);
 int vc4_v3d_pm_get(struct vc4_dev *vc4);
 void vc4_v3d_pm_put(struct vc4_dev *vc4);
 
diff --git a/drivers/gpu/drm/vc4/vc4_v3d.c b/drivers/gpu/drm/vc4/vc4_v3d.c
index fe091256800d..70204c38074a 100644
--- a/drivers/gpu/drm/vc4/vc4_v3d.c
+++ b/drivers/gpu/drm/vc4/vc4_v3d.c
@@ -241,6 +241,9 @@ int vc4_v3d_allocate_bin_bo(struct vc4_dev *vc4)
 	int ret = 0;
 	struct list_head list;
 
+	if (!v3d)
+		return -ENODEV;
+
 	/* We may need to try allocating more than once to get a BO
 	 * that doesn't cross 256MB.  Track the ones we've allocated
 	 * that failed so far, so that we can free them when we've got
-- 
2.21.0

[PATCH v4 2/4] drm/vc4: Check for V3D before binner bo alloc

[Paul Kocialkowski]

Check that we have a V3D device registered before attempting to
allocate a binner buffer object.

Signed-off-by: Paul Kocialkowski <paul.kocialkowski@bootlin.com>
---
 drivers/gpu/drm/vc4/vc4_drv.h | 1 +
 drivers/gpu/drm/vc4/vc4_v3d.c | 3 +++
 2 files changed, 4 insertions(+)

diff --git a/drivers/gpu/drm/vc4/vc4_drv.h b/drivers/gpu/drm/vc4/vc4_drv.h
index e61734af059b..37941f0e212a 100644
--- a/drivers/gpu/drm/vc4/vc4_drv.h
+++ b/drivers/gpu/drm/vc4/vc4_drv.h
@@ -833,6 +833,7 @@ void vc4_plane_async_set_fb(struct drm_plane *plane,
 extern struct platform_driver vc4_v3d_driver;
 extern const struct of_device_id vc4_v3d_dt_match[];
 int vc4_v3d_get_bin_slot(struct vc4_dev *vc4);
+int vc4_v3d_allocate_bin_bo(struct vc4_dev *vc4);
 int vc4_v3d_pm_get(struct vc4_dev *vc4);
 void vc4_v3d_pm_put(struct vc4_dev *vc4);
 
diff --git a/drivers/gpu/drm/vc4/vc4_v3d.c b/drivers/gpu/drm/vc4/vc4_v3d.c
index fe091256800d..70204c38074a 100644
--- a/drivers/gpu/drm/vc4/vc4_v3d.c
+++ b/drivers/gpu/drm/vc4/vc4_v3d.c
@@ -241,6 +241,9 @@ int vc4_v3d_allocate_bin_bo(struct vc4_dev *vc4)
 	int ret = 0;
 	struct list_head list;
 
+	if (!v3d)
+		return -ENODEV;
+
 	/* We may need to try allocating more than once to get a BO
 	 * that doesn't cross 256MB.  Track the ones we've allocated
 	 * that failed so far, so that we can free them when we've got
-- 
2.21.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v4 3/4] drm/vc4: Check for the binner bo before handling OOM interrupt

[Paul Kocialkowski]

Since the OOM interrupt directly deals with the binner bo, it doesn't
make sense to try and handle it without a binner buffer registered.
The interrupt will kick again in due time, so we can safely ignore it
without a binner bo allocated.

Signed-off-by: Paul Kocialkowski <paul.kocialkowski@bootlin.com>
---
 drivers/gpu/drm/vc4/vc4_irq.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/gpu/drm/vc4/vc4_irq.c b/drivers/gpu/drm/vc4/vc4_irq.c
index ffd0a4388752..723dc86b4511 100644
--- a/drivers/gpu/drm/vc4/vc4_irq.c
+++ b/drivers/gpu/drm/vc4/vc4_irq.c
@@ -64,6 +64,9 @@ vc4_overflow_mem_work(struct work_struct *work)
 	struct vc4_exec_info *exec;
 	unsigned long irqflags;
 
+	if (!bo)
+		return;
+
 	bin_bo_slot = vc4_v3d_get_bin_slot(vc4);
 	if (bin_bo_slot < 0) {
 		DRM_ERROR("Couldn't allocate binner overflow mem\n");
-- 
2.21.0

[PATCH v4 4/4] drm/vc4: Allocate binner bo when starting to use the V3D

[Paul Kocialkowski]

The binner bo is not required until the V3D is in use, so avoid
allocating it at probe and do it on the first non-dumb BO allocation.
Keep track of which clients are using the V3D and liberate the buffer
when there is none left.

We also want to keep it alive during runtime suspend/resume to avoid
failing to allocate it at resume. This happens when the CMA pool is
full at that point and results in a hard crash.

Signed-off-by: Paul Kocialkowski <paul.kocialkowski@bootlin.com>
---
 drivers/gpu/drm/vc4/vc4_bo.c  | 32 ++++++++++++++++++++++++++++++++
 drivers/gpu/drm/vc4/vc4_drv.c |  9 +++++++++
 drivers/gpu/drm/vc4/vc4_drv.h |  4 ++++
 drivers/gpu/drm/vc4/vc4_v3d.c | 13 -------------
 4 files changed, 45 insertions(+), 13 deletions(-)

diff --git a/drivers/gpu/drm/vc4/vc4_bo.c b/drivers/gpu/drm/vc4/vc4_bo.c
index 88ebd681d7eb..b941f09b9378 100644
--- a/drivers/gpu/drm/vc4/vc4_bo.c
+++ b/drivers/gpu/drm/vc4/vc4_bo.c
@@ -799,6 +799,30 @@ vc4_prime_import_sg_table(struct drm_device *dev,
 	return obj;
 }
 
+static int vc4_prepare_bin_bo(struct drm_device *dev,
+			      struct drm_file *file_priv)
+{
+	struct vc4_file *vc4file = file_priv->driver_priv;
+	struct vc4_dev *vc4 = to_vc4_dev(dev);
+	int ret;
+
+	if (!vc4->v3d)
+		return -ENODEV;
+
+	if (!vc4file->needs_bin_bo) {
+		atomic_inc(&vc4->bin_bo_usecnt);
+		vc4file->needs_bin_bo = true;
+	}
+
+	if (!vc4->bin_bo) {
+		ret = vc4_v3d_allocate_bin_bo(vc4);
+		if (ret)
+			return ret;
+	}
+
+	return 0;
+}
+
 int vc4_create_bo_ioctl(struct drm_device *dev, void *data,
 			struct drm_file *file_priv)
 {
@@ -806,6 +830,10 @@ int vc4_create_bo_ioctl(struct drm_device *dev, void *data,
 	struct vc4_bo *bo = NULL;
 	int ret;
 
+	ret = vc4_prepare_bin_bo(dev, file_priv);
+	if (ret)
+		return ret;
+
 	/*
 	 * We can't allocate from the BO cache, because the BOs don't
 	 * get zeroed, and that might leak data between users.
@@ -865,6 +893,10 @@ vc4_create_shader_bo_ioctl(struct drm_device *dev, void *data,
 		return -EINVAL;
 	}
 
+	ret = vc4_prepare_bin_bo(dev, file_priv);
+	if (ret)
+		return ret;
+
 	bo = vc4_bo_create(dev, args->size, true, VC4_BO_TYPE_V3D_SHADER);
 	if (IS_ERR(bo))
 		return PTR_ERR(bo);
diff --git a/drivers/gpu/drm/vc4/vc4_drv.c b/drivers/gpu/drm/vc4/vc4_drv.c
index d840b52b9805..4db937601638 100644
--- a/drivers/gpu/drm/vc4/vc4_drv.c
+++ b/drivers/gpu/drm/vc4/vc4_drv.c
@@ -128,8 +128,15 @@ static int vc4_open(struct drm_device *dev, struct drm_file *file)
 
 static void vc4_close(struct drm_device *dev, struct drm_file *file)
 {
+	struct vc4_dev *vc4 = to_vc4_dev(dev);
 	struct vc4_file *vc4file = file->driver_priv;
 
+	if (vc4file->needs_bin_bo && atomic_dec_and_test(&vc4->bin_bo_usecnt) &&
+	    vc4->bin_bo) {
+		drm_gem_object_put_unlocked(&vc4->bin_bo->base.base);
+		vc4->bin_bo = NULL;
+	}
+
 	vc4_perfmon_close_file(vc4file);
 	kfree(vc4file);
 }
@@ -274,6 +281,8 @@ static int vc4_drm_bind(struct device *dev)
 	drm->dev_private = vc4;
 	INIT_LIST_HEAD(&vc4->debugfs_list);
 
+	atomic_set(&vc4->bin_bo_usecnt, 0);
+
 	ret = vc4_bo_cache_init(drm);
 	if (ret)
 		goto dev_put;
diff --git a/drivers/gpu/drm/vc4/vc4_drv.h b/drivers/gpu/drm/vc4/vc4_drv.h
index 37941f0e212a..7d49452f04fe 100644
--- a/drivers/gpu/drm/vc4/vc4_drv.h
+++ b/drivers/gpu/drm/vc4/vc4_drv.h
@@ -216,6 +216,8 @@ struct vc4_dev {
 	 * the minor is available (after drm_dev_register()).
 	 */
 	struct list_head debugfs_list;
+
+	atomic_t bin_bo_usecnt;
 };
 
 static inline struct vc4_dev *
@@ -594,6 +596,8 @@ struct vc4_file {
 		struct idr idr;
 		struct mutex lock;
 	} perfmon;
+
+	bool needs_bin_bo;
 };
 
 static inline struct vc4_exec_info *
diff --git a/drivers/gpu/drm/vc4/vc4_v3d.c b/drivers/gpu/drm/vc4/vc4_v3d.c
index 70204c38074a..1cc366d76f2a 100644
--- a/drivers/gpu/drm/vc4/vc4_v3d.c
+++ b/drivers/gpu/drm/vc4/vc4_v3d.c
@@ -321,9 +321,6 @@ static int vc4_v3d_runtime_suspend(struct device *dev)
 
 	vc4_irq_uninstall(vc4->dev);
 
-	drm_gem_object_put_unlocked(&vc4->bin_bo->base.base);
-	vc4->bin_bo = NULL;
-
 	clk_disable_unprepare(v3d->clk);
 
 	return 0;
@@ -335,10 +332,6 @@ static int vc4_v3d_runtime_resume(struct device *dev)
 	struct vc4_dev *vc4 = v3d->vc4;
 	int ret;
 
-	ret = vc4_v3d_allocate_bin_bo(vc4);
-	if (ret)
-		return ret;
-
 	ret = clk_prepare_enable(v3d->clk);
 	if (ret != 0)
 		return ret;
@@ -405,12 +398,6 @@ static int vc4_v3d_bind(struct device *dev, struct device *master, void *data)
 	if (ret != 0)
 		return ret;
 
-	ret = vc4_v3d_allocate_bin_bo(vc4);
-	if (ret) {
-		clk_disable_unprepare(v3d->clk);
-		return ret;
-	}
-
 	/* Reset the binner overflow address/size at setup, to be sure
 	 * we don't reuse an old one.
 	 */
-- 
2.21.0

Re: [PATCH v4 4/4] drm/vc4: Allocate binner bo when starting to use the V3D

[Eric Anholt]

[-- Attachment #1: Type: text/plain, Size: 2227 bytes --]

Paul Kocialkowski <paul.kocialkowski@bootlin.com> writes:

> The binner bo is not required until the V3D is in use, so avoid
> allocating it at probe and do it on the first non-dumb BO allocation.
> Keep track of which clients are using the V3D and liberate the buffer
> when there is none left.
>
> We also want to keep it alive during runtime suspend/resume to avoid
> failing to allocate it at resume. This happens when the CMA pool is
> full at that point and results in a hard crash.
>
> Signed-off-by: Paul Kocialkowski <paul.kocialkowski@bootlin.com>
> ---
>  drivers/gpu/drm/vc4/vc4_bo.c  | 32 ++++++++++++++++++++++++++++++++
>  drivers/gpu/drm/vc4/vc4_drv.c |  9 +++++++++
>  drivers/gpu/drm/vc4/vc4_drv.h |  4 ++++
>  drivers/gpu/drm/vc4/vc4_v3d.c | 13 -------------
>  4 files changed, 45 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/gpu/drm/vc4/vc4_bo.c b/drivers/gpu/drm/vc4/vc4_bo.c
> index 88ebd681d7eb..b941f09b9378 100644
> --- a/drivers/gpu/drm/vc4/vc4_bo.c
> +++ b/drivers/gpu/drm/vc4/vc4_bo.c
> @@ -799,6 +799,30 @@ vc4_prime_import_sg_table(struct drm_device *dev,
>  	return obj;
>  }
>  
> +static int vc4_prepare_bin_bo(struct drm_device *dev,
> +			      struct drm_file *file_priv)
> +{
> +	struct vc4_file *vc4file = file_priv->driver_priv;
> +	struct vc4_dev *vc4 = to_vc4_dev(dev);
> +	int ret;
> +
> +	if (!vc4->v3d)
> +		return -ENODEV;
> +
> +	if (!vc4file->needs_bin_bo) {
> +		atomic_inc(&vc4->bin_bo_usecnt);
> +		vc4file->needs_bin_bo = true;
> +	}
> +
> +	if (!vc4->bin_bo) {
> +		ret = vc4_v3d_allocate_bin_bo(vc4);
> +		if (ret)
> +			return ret;
> +	}
> +

This atomic usage looks really racy.  For example, multiple clients
could call allocate at the same time and leak one.  Or this timeline:

us           them
             dec count to 0
inc count
check bin_bo
             free bin_bo

vc4_v3d_allocate_bin_bo should probably be a vc4_v3d_bin_bo_get()
returning a kref on the BO, called under a lock protecting both one
file_priv being dereferenced by multiple threads in the kernel at the
same time (so file_priv doesn't try to double-get its ref) and multiple
file_privs trying to get the bin_bo at once.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

Re: [PATCH v4 4/4] drm/vc4: Allocate binner bo when starting to use the V3D

[Eric Anholt]

[-- Attachment #1: Type: text/plain, Size: 2227 bytes --]

Paul Kocialkowski <paul.kocialkowski@bootlin.com> writes:

> The binner bo is not required until the V3D is in use, so avoid
> allocating it at probe and do it on the first non-dumb BO allocation.
> Keep track of which clients are using the V3D and liberate the buffer
> when there is none left.
>
> We also want to keep it alive during runtime suspend/resume to avoid
> failing to allocate it at resume. This happens when the CMA pool is
> full at that point and results in a hard crash.
>
> Signed-off-by: Paul Kocialkowski <paul.kocialkowski@bootlin.com>
> ---
>  drivers/gpu/drm/vc4/vc4_bo.c  | 32 ++++++++++++++++++++++++++++++++
>  drivers/gpu/drm/vc4/vc4_drv.c |  9 +++++++++
>  drivers/gpu/drm/vc4/vc4_drv.h |  4 ++++
>  drivers/gpu/drm/vc4/vc4_v3d.c | 13 -------------
>  4 files changed, 45 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/gpu/drm/vc4/vc4_bo.c b/drivers/gpu/drm/vc4/vc4_bo.c
> index 88ebd681d7eb..b941f09b9378 100644
> --- a/drivers/gpu/drm/vc4/vc4_bo.c
> +++ b/drivers/gpu/drm/vc4/vc4_bo.c
> @@ -799,6 +799,30 @@ vc4_prime_import_sg_table(struct drm_device *dev,
>  	return obj;
>  }
>  
> +static int vc4_prepare_bin_bo(struct drm_device *dev,
> +			      struct drm_file *file_priv)
> +{
> +	struct vc4_file *vc4file = file_priv->driver_priv;
> +	struct vc4_dev *vc4 = to_vc4_dev(dev);
> +	int ret;
> +
> +	if (!vc4->v3d)
> +		return -ENODEV;
> +
> +	if (!vc4file->needs_bin_bo) {
> +		atomic_inc(&vc4->bin_bo_usecnt);
> +		vc4file->needs_bin_bo = true;
> +	}
> +
> +	if (!vc4->bin_bo) {
> +		ret = vc4_v3d_allocate_bin_bo(vc4);
> +		if (ret)
> +			return ret;
> +	}
> +

This atomic usage looks really racy.  For example, multiple clients
could call allocate at the same time and leak one.  Or this timeline:

us           them
             dec count to 0
inc count
check bin_bo
             free bin_bo

vc4_v3d_allocate_bin_bo should probably be a vc4_v3d_bin_bo_get()
returning a kref on the BO, called under a lock protecting both one
file_priv being dereferenced by multiple threads in the kernel at the
same time (so file_priv doesn't try to double-get its ref) and multiple
file_privs trying to get the bin_bo at once.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

Re: [PATCH v4 3/4] drm/vc4: Check for the binner bo before handling OOM interrupt

[Eric Anholt]

[-- Attachment #1: Type: text/plain, Size: 1360 bytes --]

Paul Kocialkowski <paul.kocialkowski@bootlin.com> writes:

> Since the OOM interrupt directly deals with the binner bo, it doesn't
> make sense to try and handle it without a binner buffer registered.
> The interrupt will kick again in due time, so we can safely ignore it
> without a binner bo allocated.
>
> Signed-off-by: Paul Kocialkowski <paul.kocialkowski@bootlin.com>
> ---
>  drivers/gpu/drm/vc4/vc4_irq.c | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/drivers/gpu/drm/vc4/vc4_irq.c b/drivers/gpu/drm/vc4/vc4_irq.c
> index ffd0a4388752..723dc86b4511 100644
> --- a/drivers/gpu/drm/vc4/vc4_irq.c
> +++ b/drivers/gpu/drm/vc4/vc4_irq.c
> @@ -64,6 +64,9 @@ vc4_overflow_mem_work(struct work_struct *work)
>  	struct vc4_exec_info *exec;
>  	unsigned long irqflags;

Since OOM handling is tricky, could we add a comment to help the next
person try to understand it:

/* The OOM IRQ is level-triggered, so we'll see one at power-on before
 * any jobs are submitted.  The OOM IRQ is masked when this work is
 * scheduled, so we can safely return if there's no binner memory
 * (because no client is currently using 3D).  When a bin job is
 * later submitted, its tile memory allocation will end up bringing us
 * back to a non-OOM state so the OOM can be triggered again.
 */

But, actually, I don't see how the OOM IRQ will ever get re-enabled.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

Re: [PATCH v4 3/4] drm/vc4: Check for the binner bo before handling OOM interrupt

[Eric Anholt]

[-- Attachment #1: Type: text/plain, Size: 1360 bytes --]

Paul Kocialkowski <paul.kocialkowski@bootlin.com> writes:

> Since the OOM interrupt directly deals with the binner bo, it doesn't
> make sense to try and handle it without a binner buffer registered.
> The interrupt will kick again in due time, so we can safely ignore it
> without a binner bo allocated.
>
> Signed-off-by: Paul Kocialkowski <paul.kocialkowski@bootlin.com>
> ---
>  drivers/gpu/drm/vc4/vc4_irq.c | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/drivers/gpu/drm/vc4/vc4_irq.c b/drivers/gpu/drm/vc4/vc4_irq.c
> index ffd0a4388752..723dc86b4511 100644
> --- a/drivers/gpu/drm/vc4/vc4_irq.c
> +++ b/drivers/gpu/drm/vc4/vc4_irq.c
> @@ -64,6 +64,9 @@ vc4_overflow_mem_work(struct work_struct *work)
>  	struct vc4_exec_info *exec;
>  	unsigned long irqflags;

Since OOM handling is tricky, could we add a comment to help the next
person try to understand it:

/* The OOM IRQ is level-triggered, so we'll see one at power-on before
 * any jobs are submitted.  The OOM IRQ is masked when this work is
 * scheduled, so we can safely return if there's no binner memory
 * (because no client is currently using 3D).  When a bin job is
 * later submitted, its tile memory allocation will end up bringing us
 * back to a non-OOM state so the OOM can be triggered again.
 */

But, actually, I don't see how the OOM IRQ will ever get re-enabled.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

Re: [PATCH v4 4/4] drm/vc4: Allocate binner bo when starting to use the V3D

[Paul Kocialkowski]

Hi,

Le mercredi 03 avril 2019 à 11:53 -0700, Eric Anholt a écrit :
> Paul Kocialkowski <paul.kocialkowski@bootlin.com> writes:
> 
> > The binner bo is not required until the V3D is in use, so avoid
> > allocating it at probe and do it on the first non-dumb BO allocation.
> > Keep track of which clients are using the V3D and liberate the buffer
> > when there is none left.
> > 
> > We also want to keep it alive during runtime suspend/resume to avoid
> > failing to allocate it at resume. This happens when the CMA pool is
> > full at that point and results in a hard crash.
> > 
> > Signed-off-by: Paul Kocialkowski <paul.kocialkowski@bootlin.com>
> > ---
> >  drivers/gpu/drm/vc4/vc4_bo.c  | 32 ++++++++++++++++++++++++++++++++
> >  drivers/gpu/drm/vc4/vc4_drv.c |  9 +++++++++
> >  drivers/gpu/drm/vc4/vc4_drv.h |  4 ++++
> >  drivers/gpu/drm/vc4/vc4_v3d.c | 13 -------------
> >  4 files changed, 45 insertions(+), 13 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/vc4/vc4_bo.c b/drivers/gpu/drm/vc4/vc4_bo.c
> > index 88ebd681d7eb..b941f09b9378 100644
> > --- a/drivers/gpu/drm/vc4/vc4_bo.c
> > +++ b/drivers/gpu/drm/vc4/vc4_bo.c
> > @@ -799,6 +799,30 @@ vc4_prime_import_sg_table(struct drm_device *dev,
> >  	return obj;
> >  }
> >  
> > +static int vc4_prepare_bin_bo(struct drm_device *dev,
> > +			      struct drm_file *file_priv)
> > +{
> > +	struct vc4_file *vc4file = file_priv->driver_priv;
> > +	struct vc4_dev *vc4 = to_vc4_dev(dev);
> > +	int ret;
> > +
> > +	if (!vc4->v3d)
> > +		return -ENODEV;
> > +
> > +	if (!vc4file->needs_bin_bo) {
> > +		atomic_inc(&vc4->bin_bo_usecnt);
> > +		vc4file->needs_bin_bo = true;
> > +	}
> > +
> > +	if (!vc4->bin_bo) {
> > +		ret = vc4_v3d_allocate_bin_bo(vc4);
> > +		if (ret)
> > +			return ret;
> > +	}
> > +
> 
> This atomic usage looks really racy.  For example, multiple clients
> could call allocate at the same time and leak one.  Or this timeline:
> 
> us           them
>              dec count to 0
> inc count
> check bin_bo
>              free bin_bo

Oh, you're definitely right. Sorry I missed that.

> vc4_v3d_allocate_bin_bo should probably be a vc4_v3d_bin_bo_get()
> returning a kref on the BO, called under a lock protecting both one
> file_priv being dereferenced by multiple threads in the kernel at the
> same time (so file_priv doesn't try to double-get its ref) and multiple
> file_privs trying to get the bin_bo at once.

Sounds good, I'll look into it and spin up a new revision soon.

Cheers,

Paul

-- 
Paul Kocialkowski, Bootlin
Embedded Linux and kernel engineering
https://bootlin.com

Re: [PATCH v4 3/4] drm/vc4: Check for the binner bo before handling OOM interrupt

[Paul Kocialkowski]

Hey,

Le mercredi 03 avril 2019 à 11:58 -0700, Eric Anholt a écrit :
> Paul Kocialkowski <paul.kocialkowski@bootlin.com> writes:
> 
> > Since the OOM interrupt directly deals with the binner bo, it doesn't
> > make sense to try and handle it without a binner buffer registered.
> > The interrupt will kick again in due time, so we can safely ignore it
> > without a binner bo allocated.
> > 
> > Signed-off-by: Paul Kocialkowski <paul.kocialkowski@bootlin.com>
> > ---
> >  drivers/gpu/drm/vc4/vc4_irq.c | 3 +++
> >  1 file changed, 3 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/vc4/vc4_irq.c b/drivers/gpu/drm/vc4/vc4_irq.c
> > index ffd0a4388752..723dc86b4511 100644
> > --- a/drivers/gpu/drm/vc4/vc4_irq.c
> > +++ b/drivers/gpu/drm/vc4/vc4_irq.c
> > @@ -64,6 +64,9 @@ vc4_overflow_mem_work(struct work_struct *work)
> >  	struct vc4_exec_info *exec;
> >  	unsigned long irqflags;
> 
> Since OOM handling is tricky, could we add a comment to help the next
> person try to understand it:
> 
> /* The OOM IRQ is level-triggered, so we'll see one at power-on before
>  * any jobs are submitted.  The OOM IRQ is masked when this work is
>  * scheduled, so we can safely return if there's no binner memory
>  * (because no client is currently using 3D).  When a bin job is
>  * later submitted, its tile memory allocation will end up bringing us
>  * back to a non-OOM state so the OOM can be triggered again.
>  */
> 
> But, actually, I don't see how the OOM IRQ will ever get re-enabled.

Okay so I investigated that to try and understand what's going on.
We are definitely writing the OUTOMEM bit to V3D_INTDIS just before
scheduling the workqueue, and never re-enable the IRQ when leaving
early in the workqueue because !vc4->bin_bo.

It turns out that what saves us here is vc4_irq_postinstall being
called from runtime resume at "the right time". Obviously this is more
than fragile, so we should really be re-enabling the IRQ as soon as we
have the binner bo allocated.

Since we're now allocating at the first non-dumb bo alloc, I think we
need to make sure that we did in fact get the irq and registered the
allocated BO with the workqueue before submitting the rcl. Or does the
hardware provide any mechanism to take that off our hands somehow?

What do you think?

Cheers,

Paul

-- 
Paul Kocialkowski, Bootlin
Embedded Linux and kernel engineering
https://bootlin.com

Re: [PATCH v4 3/4] drm/vc4: Check for the binner bo before handling OOM interrupt

[Eric Anholt]

[-- Attachment #1: Type: text/plain, Size: 2679 bytes --]

Paul Kocialkowski <paul.kocialkowski@bootlin.com> writes:

> Hey,
>
> Le mercredi 03 avril 2019 à 11:58 -0700, Eric Anholt a écrit :
>> Paul Kocialkowski <paul.kocialkowski@bootlin.com> writes:
>> 
>> > Since the OOM interrupt directly deals with the binner bo, it doesn't
>> > make sense to try and handle it without a binner buffer registered.
>> > The interrupt will kick again in due time, so we can safely ignore it
>> > without a binner bo allocated.
>> > 
>> > Signed-off-by: Paul Kocialkowski <paul.kocialkowski@bootlin.com>
>> > ---
>> >  drivers/gpu/drm/vc4/vc4_irq.c | 3 +++
>> >  1 file changed, 3 insertions(+)
>> > 
>> > diff --git a/drivers/gpu/drm/vc4/vc4_irq.c b/drivers/gpu/drm/vc4/vc4_irq.c
>> > index ffd0a4388752..723dc86b4511 100644
>> > --- a/drivers/gpu/drm/vc4/vc4_irq.c
>> > +++ b/drivers/gpu/drm/vc4/vc4_irq.c
>> > @@ -64,6 +64,9 @@ vc4_overflow_mem_work(struct work_struct *work)
>> >  	struct vc4_exec_info *exec;
>> >  	unsigned long irqflags;
>> 
>> Since OOM handling is tricky, could we add a comment to help the next
>> person try to understand it:
>> 
>> /* The OOM IRQ is level-triggered, so we'll see one at power-on before
>>  * any jobs are submitted.  The OOM IRQ is masked when this work is
>>  * scheduled, so we can safely return if there's no binner memory
>>  * (because no client is currently using 3D).  When a bin job is
>>  * later submitted, its tile memory allocation will end up bringing us
>>  * back to a non-OOM state so the OOM can be triggered again.
>>  */
>> 
>> But, actually, I don't see how the OOM IRQ will ever get re-enabled.
>
> Okay so I investigated that to try and understand what's going on.
> We are definitely writing the OUTOMEM bit to V3D_INTDIS just before
> scheduling the workqueue, and never re-enable the IRQ when leaving
> early in the workqueue because !vc4->bin_bo.
>
> It turns out that what saves us here is vc4_irq_postinstall being
> called from runtime resume at "the right time". Obviously this is more
> than fragile, so we should really be re-enabling the IRQ as soon as we
> have the binner bo allocated.
>
> Since we're now allocating at the first non-dumb bo alloc, I think we
> need to make sure that we did in fact get the irq and registered the
> allocated BO with the workqueue before submitting the rcl. Or does the
> hardware provide any mechanism to take that off our hands somehow?

Maybe just enable the OOM interrupt using INTENA in the bin BO
allocation's success case?  That feels race-free, since it's a level
interrupt and even if we were racing the !bin_bo check in the work, we'd
end up re-scheduling the work?

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]