[PATCH 1/3] drm/file: Rehabilitate the firstopen hook for non-legacy drivers

[PATCH 1/3] drm/file: Rehabilitate the firstopen hook for non-legacy drivers

[Paul Kocialkowski]

The firstopen DRM driver hook was initially used to perform hardware
initialization, which is now considered legacy. Only a single user of
firstopen remains at this point (savage).

In some specific cases, non-legacy drivers may also need to implement
these hooks. For instance on VC4, we need to allocate a 16 MiB buffer
for the GPU. Because it's not required for fbcon, it's a waste to
allocate it before userspace starts using the DRM device.

Using firstopen and lastclose for this allocation seems like the best
fit, so re-habilitate the hook to allow it to be called for non-legacy
drivers.

Signed-off-by: Paul Kocialkowski <paul.kocialkowski@bootlin.com>
---
 drivers/gpu/drm/drm_file.c | 3 +--
 include/drm/drm_drv.h      | 2 +-
 2 files changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/drm_file.c b/drivers/gpu/drm/drm_file.c
index b1838a41ad43..c011b5cbfb6b 100644
--- a/drivers/gpu/drm/drm_file.c
+++ b/drivers/gpu/drm/drm_file.c
@@ -266,8 +266,7 @@ static int drm_setup(struct drm_device * dev)
 {
 	int ret;
 
-	if (dev->driver->firstopen &&
-	    drm_core_check_feature(dev, DRIVER_LEGACY)) {
+	if (dev->driver->firstopen) {
 		ret = dev->driver->firstopen(dev);
 		if (ret != 0)
 			return ret;
diff --git a/include/drm/drm_drv.h b/include/drm/drm_drv.h
index ca46a45a9cce..aa14607e54d4 100644
--- a/include/drm/drm_drv.h
+++ b/include/drm/drm_drv.h
@@ -236,7 +236,7 @@ struct drm_driver {
 	 * to set/unset the VT into raw mode.
 	 *
 	 * Legacy drivers initialize the hardware in the @firstopen callback,
-	 * which isn't even called for modern drivers.
+	 * modern drivers can use it for other purposes only.
 	 */
 	void (*lastclose) (struct drm_device *);
 
-- 
2.21.0

[PATCH 2/3] drm/vc4: Allocate/liberate the binner BO at firstopen/lastclose

[Paul Kocialkowski]

Since the binner buffer is only required for GPU rendering, it's a waste
to allocate it when the driver probes since internal users of the driver
(such as fbcon) won't try to use the GPU.

Move the allocation/liberation to the firstopen/lastclose to only
allocate it when userspace has opened the device.

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

diff --git a/drivers/gpu/drm/vc4/vc4_drv.c b/drivers/gpu/drm/vc4/vc4_drv.c
index 3227706700f9..605dc50613e3 100644
--- a/drivers/gpu/drm/vc4/vc4_drv.c
+++ b/drivers/gpu/drm/vc4/vc4_drv.c
@@ -134,6 +134,30 @@ static void vc4_close(struct drm_device *dev, struct drm_file *file)
 	kfree(vc4file);
 }
 
+static int vc4_firstopen(struct drm_device *drm)
+{
+	struct vc4_dev *vc4 = to_vc4_dev(drm);
+	int ret;
+
+	if (!vc4->bin_bo) {
+		ret = vc4_allocate_bin_bo(drm);
+		if (ret)
+			return ret;
+	}
+
+	return 0;
+}
+
+static void vc4_lastclose(struct drm_device *drm)
+{
+	struct vc4_dev *vc4 = to_vc4_dev(drm);
+
+	if (vc4->bin_bo) {
+		drm_gem_object_put_unlocked(&vc4->bin_bo->base.base);
+		vc4->bin_bo = NULL;
+	}
+}
+
 static const struct vm_operations_struct vc4_vm_ops = {
 	.fault = vc4_fault,
 	.open = drm_gem_vm_open,
@@ -180,6 +204,8 @@ static struct drm_driver vc4_drm_driver = {
 			    DRIVER_SYNCOBJ),
 	.open = vc4_open,
 	.postclose = vc4_close,
+	.firstopen = vc4_firstopen,
+	.lastclose = vc4_lastclose,
 	.irq_handler = vc4_irq,
 	.irq_preinstall = vc4_irq_preinstall,
 	.irq_postinstall = vc4_irq_postinstall,
diff --git a/drivers/gpu/drm/vc4/vc4_drv.h b/drivers/gpu/drm/vc4/vc4_drv.h
index 7a3c093e7443..f52bb21e9885 100644
--- a/drivers/gpu/drm/vc4/vc4_drv.h
+++ b/drivers/gpu/drm/vc4/vc4_drv.h
@@ -808,6 +808,7 @@ extern struct platform_driver vc4_v3d_driver;
 int vc4_v3d_debugfs_ident(struct seq_file *m, void *unused);
 int vc4_v3d_debugfs_regs(struct seq_file *m, void *unused);
 int vc4_v3d_get_bin_slot(struct vc4_dev *vc4);
+int vc4_allocate_bin_bo(struct drm_device *drm);
 
 /* vc4_validate.c */
 int
diff --git a/drivers/gpu/drm/vc4/vc4_irq.c b/drivers/gpu/drm/vc4/vc4_irq.c
index 4cd2ccfe15f4..efaba2b02f6c 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");
diff --git a/drivers/gpu/drm/vc4/vc4_v3d.c b/drivers/gpu/drm/vc4/vc4_v3d.c
index e47e29426078..ab15e71df001 100644
--- a/drivers/gpu/drm/vc4/vc4_v3d.c
+++ b/drivers/gpu/drm/vc4/vc4_v3d.c
@@ -218,7 +218,7 @@ 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_allocate_bin_bo(struct drm_device *drm)
 {
 	struct vc4_dev *vc4 = to_vc4_dev(drm);
 	struct vc4_v3d *v3d = vc4->v3d;
@@ -384,12 +384,6 @@ static int vc4_v3d_bind(struct device *dev, struct device *master, void *data)
 	if (ret != 0)
 		return ret;
 
-	ret = vc4_allocate_bin_bo(drm);
-	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

[PATCH 3/3] drm/vc4: Don't liberate the binner BO at runtime suspend

[Paul Kocialkowski]

The binner BO is a pre-requisite to GPU operations, so we must ensure
that it is always allocated when the GPU is in use.

Because the buffer is allocated from the same pool as other GPU buffers,
we might run into a situation where we are out of memory at runtime
resume. This causes the binner BO allocation to fail and results in all
subsequent operations to fail, resulting in a major hang in userspace.

Now that we allocate the buffer at firstopen and liberate it at
lastclose, we can just keep it alive during runtime suspend.

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

diff --git a/drivers/gpu/drm/vc4/vc4_v3d.c b/drivers/gpu/drm/vc4/vc4_v3d.c
index ab15e71df001..e04a51a75f01 100644
--- a/drivers/gpu/drm/vc4/vc4_v3d.c
+++ b/drivers/gpu/drm/vc4/vc4_v3d.c
@@ -303,9 +303,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;
@@ -317,10 +314,6 @@ static int vc4_v3d_runtime_resume(struct device *dev)
 	struct vc4_dev *vc4 = v3d->vc4;
 	int ret;
 
-	ret = vc4_allocate_bin_bo(vc4->dev);
-	if (ret)
-		return ret;
-
 	ret = clk_prepare_enable(v3d->clk);
 	if (ret != 0)
 		return ret;
-- 
2.21.0

Re: [PATCH 3/3] drm/vc4: Don't liberate the binner BO at runtime suspend

[Eric Anholt]

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

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

> The binner BO is a pre-requisite to GPU operations, so we must ensure
> that it is always allocated when the GPU is in use.
>
> Because the buffer is allocated from the same pool as other GPU buffers,
> we might run into a situation where we are out of memory at runtime
> resume. This causes the binner BO allocation to fail and results in all
> subsequent operations to fail, resulting in a major hang in userspace.
>
> Now that we allocate the buffer at firstopen and liberate it at
> lastclose, we can just keep it alive during runtime suspend.

I think this needs to be squashed into the previous patch, as otherwise
coming from suspended, a firstopen -> resume -> render will leak a copy
of the bin BO.

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

Re: [PATCH 3/3] drm/vc4: Don't liberate the binner BO at runtime suspend

[Paul Kocialkowski]

Hi,

Le samedi 16 mars 2019 à 11:58 -0700, Eric Anholt a écrit :
> Paul Kocialkowski <paul.kocialkowski@bootlin.com> writes:
> 
> > The binner BO is a pre-requisite to GPU operations, so we must ensure
> > that it is always allocated when the GPU is in use.
> > 
> > Because the buffer is allocated from the same pool as other GPU buffers,
> > we might run into a situation where we are out of memory at runtime
> > resume. This causes the binner BO allocation to fail and results in all
> > subsequent operations to fail, resulting in a major hang in userspace.
> > 
> > Now that we allocate the buffer at firstopen and liberate it at
> > lastclose, we can just keep it alive during runtime suspend.
> 
> I think this needs to be squashed into the previous patch, as otherwise
> coming from suspended, a firstopen -> resume -> render will leak a copy
> of the bin BO.

Woops, you're definitely right: vc4_allocate_bin_bo won't check whether
we already have allocated it or not. I'll send a new version with both
patches squashed.

Cheers,

Paul

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