Changes since v1:
* Squashed the two final patches into one.
Paul Kocialkowski (2):
drm/file: Rehabilitate the firstopen hook for non-legacy drivers
drm/vc4: Allocated/liberate the binner BO at firstopen/lastclose
drivers/gpu/drm/drm_file.c | 3 +--
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 | 15 +--------------
include/drm/drm_drv.h | 2 +-
6 files changed, 33 insertions(+), 17 deletions(-)
--
2.21.0
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 <[email protected]>
---
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
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. Currently, we are
allocating it at probe time and liberating/allocating it during runtime
pm cycles.
First, 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 instead to
only allocate it when userspace has opened the device and adapt the IRQ
handler to return early when no binner BO was allocated yet.
Second, 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.
As a result, keep the buffer alive during runtime pm.
Signed-off-by: Paul Kocialkowski <[email protected]>
---
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 | 15 +--------------
4 files changed, 31 insertions(+), 14 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..e04a51a75f01 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;
@@ -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;
@@ -384,12 +377,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
Paul Kocialkowski <[email protected]> writes:
> 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 <[email protected]>
> ---
> 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 *);
Our usage in vc4 is not very different from what we called "hardware
initialization" in other devices. I would rather just delete this
sentence entirely.
The only alternative I can think of to using a firstopen/lastclose-style
allocation for this would be to allocate the bin bo on the first
(non-dumb?) V3D BO allocation and refcount those to free the binner.
Paul Kocialkowski <[email protected]> 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. Currently, we are
> allocating it at probe time and liberating/allocating it during runtime
> pm cycles.
>
> First, 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 instead to
> only allocate it when userspace has opened the device and adapt the IRQ
> handler to return early when no binner BO was allocated yet.
>
> Second, 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.
>
> As a result, keep the buffer alive during runtime pm.
>
> Signed-off-by: Paul Kocialkowski <[email protected]>
> ---
> 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");
Hmm. We take the OOM IRQ on poweron, have no bin BO since nobody's
opened yet, and leave it. Do we ever get the OOM IRQ again after that?
Seems like vc4_allocate_bin_bo() might need to kick something so that we
can fill an OOM request.
Hi,
Le mercredi 20 mars 2019 à 09:56 -0700, Eric Anholt a écrit :
> Paul Kocialkowski <[email protected]> writes:
>
> > 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 <[email protected]>
> > ---
> > 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 *);
>
> Our usage in vc4 is not very different from what we called "hardware
> initialization" in other devices. I would rather just delete this
> sentence entirely.
Sounds good to me!
> The only alternative I can think of to using a firstopen/lastclose-style
> allocation for this would be to allocate the bin bo on the first
> (non-dumb?) V3D BO allocation and refcount those to free the binner.
I don't see other options either, and using firstclose/lastopen feels
overall more readable in the driver code.
I'm not sure there is such a big overhead associated with allocating
the binner BO (it seems that the current implementation tries to alloc
until the specific memory constraints for the buffer are met, so
perhaps that can take time). But if there is, I suppose it's best to
have that when starting up rather than delaying the first render
operation.
Cheers,
Paul
--
Paul Kocialkowski, Bootlin
Embedded Linux and kernel engineering
https://bootlin.com
Hi,
Le mercredi 20 mars 2019 à 09:58 -0700, Eric Anholt a écrit :
> Paul Kocialkowski <[email protected]> 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. Currently, we are
> > allocating it at probe time and liberating/allocating it during runtime
> > pm cycles.
> >
> > First, 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 instead to
> > only allocate it when userspace has opened the device and adapt the IRQ
> > handler to return early when no binner BO was allocated yet.
> >
> > Second, 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.
> >
> > As a result, keep the buffer alive during runtime pm.
> >
> > Signed-off-by: Paul Kocialkowski <[email protected]>
> > ---
> > 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");
>
> Hmm. We take the OOM IRQ on poweron, have no bin BO since nobody's
> opened yet, and leave it. Do we ever get the OOM IRQ again after that?
> Seems like vc4_allocate_bin_bo() might need to kick something so that we
> can fill an OOM request.
I just had a look and it seems that we do get the OOM interrupt again
after the bin BO is allocated. Actually, I can see it kicking from time
to time when using X with glamor.
From what I understood, this looks fairly legitimate. Should we be
worried about this?
Cheers,
Paul
--
Paul Kocialkowski, Bootlin
Embedded Linux and kernel engineering
https://bootlin.com
Paul Kocialkowski <[email protected]> writes:
> Hi,
>
> Le mercredi 20 mars 2019 à 09:58 -0700, Eric Anholt a écrit :
>> Paul Kocialkowski <[email protected]> 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. Currently, we are
>> > allocating it at probe time and liberating/allocating it during runtime
>> > pm cycles.
>> >
>> > First, 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 instead to
>> > only allocate it when userspace has opened the device and adapt the IRQ
>> > handler to return early when no binner BO was allocated yet.
>> >
>> > Second, 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.
>> >
>> > As a result, keep the buffer alive during runtime pm.
>> >
>> > Signed-off-by: Paul Kocialkowski <[email protected]>
>> > ---
>> > 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");
>>
>> Hmm. We take the OOM IRQ on poweron, have no bin BO since nobody's
>> opened yet, and leave it. Do we ever get the OOM IRQ again after that?
>> Seems like vc4_allocate_bin_bo() might need to kick something so that we
>> can fill an OOM request.
>
> I just had a look and it seems that we do get the OOM interrupt again
> after the bin BO is allocated. Actually, I can see it kicking from time
> to time when using X with glamor.
>
> From what I understood, this looks fairly legitimate. Should we be
> worried about this?
Great. I think how it ends up working is that when the job is
submitted, the bin allocation it supplies internally gets us out of the
OOM condition, so OOM can edge trigger again later.
Paul Kocialkowski <[email protected]> writes:
> Hi,
>
> Le mercredi 20 mars 2019 à 09:56 -0700, Eric Anholt a écrit :
>> Paul Kocialkowski <[email protected]> writes:
>>
>> > 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 <[email protected]>
>> > ---
>> > 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 *);
>>
>> Our usage in vc4 is not very different from what we called "hardware
>> initialization" in other devices. I would rather just delete this
>> sentence entirely.
>
> Sounds good to me!
With the delete, the series is:
Reviewed-by: Eric Anholt <[email protected]>
but hopefully someone other than a vc4 developer (danvet?) can make the
call on whether undeprecating firstopen/lastclose for this is
reasonable.
On Thu, Mar 21, 2019 at 04:27:06PM +0100, Paul Kocialkowski wrote:
> Hi,
>
> Le mercredi 20 mars 2019 ? 09:56 -0700, Eric Anholt a ?crit :
> > Paul Kocialkowski <[email protected]> writes:
> >
> > > 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 <[email protected]>
> > > ---
> > > 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 *);
> >
> > Our usage in vc4 is not very different from what we called "hardware
> > initialization" in other devices. I would rather just delete this
> > sentence entirely.
>
> Sounds good to me!
>
> > The only alternative I can think of to using a firstopen/lastclose-style
> > allocation for this would be to allocate the bin bo on the first
> > (non-dumb?) V3D BO allocation and refcount those to free the binner.
>
> I don't see other options either, and using firstclose/lastopen feels
> overall more readable in the driver code.
>
> I'm not sure there is such a big overhead associated with allocating
> the binner BO (it seems that the current implementation tries to alloc
> until the specific memory constraints for the buffer are met, so
> perhaps that can take time). But if there is, I suppose it's best to
> have that when starting up rather than delaying the first render
> operation.
I'm not entirely buying the "we don't need this for fbcon only" argument -
there's plenty of dumb kms clients too (boot splash and whatever else
there might be). If you don't want to keep this around I think allocating
on first non-dumb bo allocation and dropping it when the last such fd
closes sounds like a much better idea. Needs a bit more state, you need to
track per drm_file whether you've already allocated a non-dumb bo, and a
drm_device refcount, but that's not much. Firstopen feels like the wrong
thing.
Another option would be first_renderopen or something like that, except
you can also render on the legacy node and I'm not sure how much there's a
demand for this in other drivers. In the end you have open/close
callbacks, you can do all the driver specific things you want to do in
there.
Aside: I kinda also want to ditch lastclose usage. There's fbdev (we have a
better solution with Noralf's drm_client for those) and runtime pm (which
frankly is just a cheap hack, I want my gpu to susepend also when it's not
in use when all the screens are off, not only when I killed X and
everything).
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
Hi,
On Thu, 28 Mar 2019 at 18:53, Daniel Vetter <[email protected]> wrote:
> On Thu, Mar 21, 2019 at 04:27:06PM +0100, Paul Kocialkowski wrote:
> > I don't see other options either, and using firstclose/lastopen feels
> > overall more readable in the driver code.
> >
> > I'm not sure there is such a big overhead associated with allocating
> > the binner BO (it seems that the current implementation tries to alloc
> > until the specific memory constraints for the buffer are met, so
> > perhaps that can take time). But if there is, I suppose it's best to
> > have that when starting up rather than delaying the first render
> > operation.
>
> I'm not entirely buying the "we don't need this for fbcon only" argument -
> there's plenty of dumb kms clients too (boot splash and whatever else
> there might be). If you don't want to keep this around I think allocating
> on first non-dumb bo allocation and dropping it when the last such fd
> closes sounds like a much better idea. Needs a bit more state, you need to
> track per drm_file whether you've already allocated a non-dumb bo, and a
> drm_device refcount, but that's not much. Firstopen feels like the wrong
> thing.
>
> Another option would be first_renderopen or something like that, except
> you can also render on the legacy node and I'm not sure how much there's a
> demand for this in other drivers. In the end you have open/close
> callbacks, you can do all the driver specific things you want to do in
> there.
I'd like to avoid doing it in open where possible, since that hurts
device enumeration from userspace.
Cheers,
Daniel
Hi,
On Fri, 2019-03-29 at 09:09 +0000, Daniel Stone wrote:
> Hi,
>
> On Thu, 28 Mar 2019 at 18:53, Daniel Vetter <[email protected]> wrote:
> > On Thu, Mar 21, 2019 at 04:27:06PM +0100, Paul Kocialkowski wrote:
> > > I don't see other options either, and using firstclose/lastopen feels
> > > overall more readable in the driver code.
> > >
> > > I'm not sure there is such a big overhead associated with allocating
> > > the binner BO (it seems that the current implementation tries to alloc
> > > until the specific memory constraints for the buffer are met, so
> > > perhaps that can take time). But if there is, I suppose it's best to
> > > have that when starting up rather than delaying the first render
> > > operation.
> >
> > I'm not entirely buying the "we don't need this for fbcon only" argument -
> > there's plenty of dumb kms clients too (boot splash and whatever else
> > there might be). If you don't want to keep this around I think allocating
> > on first non-dumb bo allocation and dropping it when the last such fd
> > closes sounds like a much better idea. Needs a bit more state, you need to
> > track per drm_file whether you've already allocated a non-dumb bo, and a
> > drm_device refcount, but that's not much. Firstopen feels like the wrong
> > thing.
> >
> > Another option would be first_renderopen or something like that, except
> > you can also render on the legacy node and I'm not sure how much there's a
> > demand for this in other drivers. In the end you have open/close
> > callbacks, you can do all the driver specific things you want to do in
> > there.
>
> I'd like to avoid doing it in open where possible, since that hurts
> device enumeration from userspace.
I've noticed the same issue with firstopen, where our buffer is
allocated/liberated a couple of times during enumeration, before the
final open that stays alive during use.
I'm not sure what is preferable between that and allocating when the
GPU is first used. Slowing down the first GPU operation with the
allocation does not sound too great either and it feels like the buffer
should have been allocated earlier.
To me, it feels I think it's better to have delays due to allocation at
enumeration / startup rather than later on, but I might be missing some
elements to have a clear idea.
What do you think?
Cheers,
Paul
--
Paul Kocialkowski, Bootlin
Embedded Linux and kernel engineering
https://bootlin.com
Hi,
Le jeudi 28 mars 2019 à 19:53 +0100, Daniel Vetter a écrit :
> On Thu, Mar 21, 2019 at 04:27:06PM +0100, Paul Kocialkowski wrote:
> > Hi,
> >
> > Le mercredi 20 mars 2019 à 09:56 -0700, Eric Anholt a écrit :
> > > Paul Kocialkowski <[email protected]> writes:
> > >
> > > > 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 <[email protected]>
> > > > ---
> > > > 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 *);
> > >
> > > Our usage in vc4 is not very different from what we called "hardware
> > > initialization" in other devices. I would rather just delete this
> > > sentence entirely.
> >
> > Sounds good to me!
> >
> > > The only alternative I can think of to using a firstopen/lastclose-style
> > > allocation for this would be to allocate the bin bo on the first
> > > (non-dumb?) V3D BO allocation and refcount those to free the binner.
> >
> > I don't see other options either, and using firstclose/lastopen feels
> > overall more readable in the driver code.
> >
> > I'm not sure there is such a big overhead associated with allocating
> > the binner BO (it seems that the current implementation tries to alloc
> > until the specific memory constraints for the buffer are met, so
> > perhaps that can take time). But if there is, I suppose it's best to
> > have that when starting up rather than delaying the first render
> > operation.
>
> I'm not entirely buying the "we don't need this for fbcon only" argument -
> there's plenty of dumb kms clients too (boot splash and whatever else
> there might be). If you don't want to keep this around I think allocating
> on first non-dumb bo allocation and dropping it when the last such fd
> closes sounds like a much better idea. Needs a bit more state, you need to
> track per drm_file whether you've already allocated a non-dumb bo, and a
> drm_device refcount, but that's not much. Firstopen feels like the wrong
> thing.
This is quite a valid point, but we decided to go for a middle-ground
between always allocating and allocating at the first GPU op, so that
the operation is not significantly delayed by allocating the buffer.
> Another option would be first_renderopen or something like that, except
> you can also render on the legacy node and I'm not sure how much there's a
> demand for this in other drivers. In the end you have open/close
> callbacks, you can do all the driver specific things you want to do in
> there.
Yes I've initially tried playing with that and reached the same
conclusion: there can be render ops on the legacy node too so this is
just not reliable for what we're trying to do.
> Aside: I kinda also want to ditch lastclose usage. There's fbdev (we have a
> better solution with Noralf's drm_client for those) and runtime pm (which
> frankly is just a cheap hack, I want my gpu to susepend also when it's not
> in use when all the screens are off, not only when I killed X and
> everything).
I understand that, yes. I don't see any issue with a implementation
tracking the state in open if we really want firstopen/lastclose to
disappear.
Cheers,
Paul
--
Paul Kocialkowski, Bootlin
Embedded Linux and kernel engineering
https://bootlin.com
On Fri, Mar 29, 2019 at 04:02:23PM +0100, Paul Kocialkowski wrote:
> Hi,
>
> On Fri, 2019-03-29 at 09:09 +0000, Daniel Stone wrote:
> > Hi,
> >
> > On Thu, 28 Mar 2019 at 18:53, Daniel Vetter <[email protected]> wrote:
> > > On Thu, Mar 21, 2019 at 04:27:06PM +0100, Paul Kocialkowski wrote:
> > > > I don't see other options either, and using firstclose/lastopen feels
> > > > overall more readable in the driver code.
> > > >
> > > > I'm not sure there is such a big overhead associated with allocating
> > > > the binner BO (it seems that the current implementation tries to alloc
> > > > until the specific memory constraints for the buffer are met, so
> > > > perhaps that can take time). But if there is, I suppose it's best to
> > > > have that when starting up rather than delaying the first render
> > > > operation.
> > >
> > > I'm not entirely buying the "we don't need this for fbcon only" argument -
> > > there's plenty of dumb kms clients too (boot splash and whatever else
> > > there might be). If you don't want to keep this around I think allocating
> > > on first non-dumb bo allocation and dropping it when the last such fd
> > > closes sounds like a much better idea. Needs a bit more state, you need to
> > > track per drm_file whether you've already allocated a non-dumb bo, and a
> > > drm_device refcount, but that's not much. Firstopen feels like the wrong
> > > thing.
> > >
> > > Another option would be first_renderopen or something like that, except
> > > you can also render on the legacy node and I'm not sure how much there's a
> > > demand for this in other drivers. In the end you have open/close
> > > callbacks, you can do all the driver specific things you want to do in
> > > there.
> >
> > I'd like to avoid doing it in open where possible, since that hurts
> > device enumeration from userspace.
>
> I've noticed the same issue with firstopen, where our buffer is
> allocated/liberated a couple of times during enumeration, before the
> final open that stays alive during use.
>
> I'm not sure what is preferable between that and allocating when the
> GPU is first used. Slowing down the first GPU operation with the
> allocation does not sound too great either and it feels like the buffer
> should have been allocated earlier.
>
> To me, it feels I think it's better to have delays due to allocation at
> enumeration / startup rather than later on, but I might be missing some
> elements to have a clear idea.
>
> What do you think?
We'll have the delay somewhere on driver load. Better to have it only once
(when the driver starts using gem for real), than a bunch of time, at
least once while enumerating and then once more while actually
initializing. I think if you allocat this on first non-dumb gem_create,
and on first command submission (just so evil userspace can't screw up the
hw too badly), that should be perfectly fine.
Only way to avoid that is to allocate at driver load and pin it, but
that's what we're trying to avoid here.
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
Hi,
Le vendredi 29 mars 2019 à 16:25 +0100, Daniel Vetter a écrit :
> On Fri, Mar 29, 2019 at 04:02:23PM +0100, Paul Kocialkowski wrote:
> > Hi,
> >
> > On Fri, 2019-03-29 at 09:09 +0000, Daniel Stone wrote:
> > > Hi,
> > >
> > > On Thu, 28 Mar 2019 at 18:53, Daniel Vetter <[email protected]> wrote:
> > > > On Thu, Mar 21, 2019 at 04:27:06PM +0100, Paul Kocialkowski wrote:
> > > > > I don't see other options either, and using firstclose/lastopen feels
> > > > > overall more readable in the driver code.
> > > > >
> > > > > I'm not sure there is such a big overhead associated with allocating
> > > > > the binner BO (it seems that the current implementation tries to alloc
> > > > > until the specific memory constraints for the buffer are met, so
> > > > > perhaps that can take time). But if there is, I suppose it's best to
> > > > > have that when starting up rather than delaying the first render
> > > > > operation.
> > > >
> > > > I'm not entirely buying the "we don't need this for fbcon only" argument -
> > > > there's plenty of dumb kms clients too (boot splash and whatever else
> > > > there might be). If you don't want to keep this around I think allocating
> > > > on first non-dumb bo allocation and dropping it when the last such fd
> > > > closes sounds like a much better idea. Needs a bit more state, you need to
> > > > track per drm_file whether you've already allocated a non-dumb bo, and a
> > > > drm_device refcount, but that's not much. Firstopen feels like the wrong
> > > > thing.
> > > >
> > > > Another option would be first_renderopen or something like that, except
> > > > you can also render on the legacy node and I'm not sure how much there's a
> > > > demand for this in other drivers. In the end you have open/close
> > > > callbacks, you can do all the driver specific things you want to do in
> > > > there.
> > >
> > > I'd like to avoid doing it in open where possible, since that hurts
> > > device enumeration from userspace.
> >
> > I've noticed the same issue with firstopen, where our buffer is
> > allocated/liberated a couple of times during enumeration, before the
> > final open that stays alive during use.
> >
> > I'm not sure what is preferable between that and allocating when the
> > GPU is first used. Slowing down the first GPU operation with the
> > allocation does not sound too great either and it feels like the buffer
> > should have been allocated earlier.
> >
> > To me, it feels I think it's better to have delays due to allocation at
> > enumeration / startup rather than later on, but I might be missing some
> > elements to have a clear idea.
> >
> > What do you think?
>
> We'll have the delay somewhere on driver load. Better to have it only once
> (when the driver starts using gem for real), than a bunch of time, at
> least once while enumerating and then once more while actually
> initializing. I think if you allocat this on first non-dumb gem_create,
> and on first command submission (just so evil userspace can't screw up the
> hw too badly), that should be perfectly fine.
I'm not totally convinced that it's okay to have a delay outside of
init/enumeration, even if it's a smaller delay.
Eric, do you have an opinion on what is the preference with VC4?
> Only way to avoid that is to allocate at driver load and pin it, but
> that's what we're trying to avoid here.
Exactly!
Cheers,
Paul
--
Paul Kocialkowski, Bootlin
Embedded Linux and kernel engineering
https://bootlin.com
Paul Kocialkowski <[email protected]> writes:
> Hi,
>
> Le vendredi 29 mars 2019 à 16:25 +0100, Daniel Vetter a écrit :
>> On Fri, Mar 29, 2019 at 04:02:23PM +0100, Paul Kocialkowski wrote:
>> > Hi,
>> >
>> > On Fri, 2019-03-29 at 09:09 +0000, Daniel Stone wrote:
>> > > Hi,
>> > >
>> > > On Thu, 28 Mar 2019 at 18:53, Daniel Vetter <[email protected]> wrote:
>> > > > On Thu, Mar 21, 2019 at 04:27:06PM +0100, Paul Kocialkowski wrote:
>> > > > > I don't see other options either, and using firstclose/lastopen feels
>> > > > > overall more readable in the driver code.
>> > > > >
>> > > > > I'm not sure there is such a big overhead associated with allocating
>> > > > > the binner BO (it seems that the current implementation tries to alloc
>> > > > > until the specific memory constraints for the buffer are met, so
>> > > > > perhaps that can take time). But if there is, I suppose it's best to
>> > > > > have that when starting up rather than delaying the first render
>> > > > > operation.
>> > > >
>> > > > I'm not entirely buying the "we don't need this for fbcon only" argument -
>> > > > there's plenty of dumb kms clients too (boot splash and whatever else
>> > > > there might be). If you don't want to keep this around I think allocating
>> > > > on first non-dumb bo allocation and dropping it when the last such fd
>> > > > closes sounds like a much better idea. Needs a bit more state, you need to
>> > > > track per drm_file whether you've already allocated a non-dumb bo, and a
>> > > > drm_device refcount, but that's not much. Firstopen feels like the wrong
>> > > > thing.
>> > > >
>> > > > Another option would be first_renderopen or something like that, except
>> > > > you can also render on the legacy node and I'm not sure how much there's a
>> > > > demand for this in other drivers. In the end you have open/close
>> > > > callbacks, you can do all the driver specific things you want to do in
>> > > > there.
>> > >
>> > > I'd like to avoid doing it in open where possible, since that hurts
>> > > device enumeration from userspace.
>> >
>> > I've noticed the same issue with firstopen, where our buffer is
>> > allocated/liberated a couple of times during enumeration, before the
>> > final open that stays alive during use.
>> >
>> > I'm not sure what is preferable between that and allocating when the
>> > GPU is first used. Slowing down the first GPU operation with the
>> > allocation does not sound too great either and it feels like the buffer
>> > should have been allocated earlier.
>> >
>> > To me, it feels I think it's better to have delays due to allocation at
>> > enumeration / startup rather than later on, but I might be missing some
>> > elements to have a clear idea.
>> >
>> > What do you think?
>>
>> We'll have the delay somewhere on driver load. Better to have it only once
>> (when the driver starts using gem for real), than a bunch of time, at
>> least once while enumerating and then once more while actually
>> initializing. I think if you allocat this on first non-dumb gem_create,
>> and on first command submission (just so evil userspace can't screw up the
>> hw too badly), that should be perfectly fine.
>
> I'm not totally convinced that it's okay to have a delay outside of
> init/enumeration, even if it's a smaller delay.
You'll have non-dumb buffers created during GL context creation, so
early in xserver or other KMS-and-GL-using application init anyway.
Seems like a perfectly fine plan to me.
Hi,
On Fri, 29 Mar 2019 at 18:14, Eric Anholt <[email protected]> wrote:
> Paul Kocialkowski <[email protected]> writes:
> > I'm not totally convinced that it's okay to have a delay outside of
> > init/enumeration, even if it's a smaller delay.
>
> You'll have non-dumb buffers created during GL context creation, so
> early in xserver or other KMS-and-GL-using application init anyway.
> Seems like a perfectly fine plan to me.
Yeah. The alternative is doing it once when Plymouth starts, and then
maybe again when Weston/GNOME/Xorg/... starts, which isn't really
ideal (or maybe even udev helpers). Doing it on probe also complicates
profiling startup for those: if GL context or surface creation takes a
long time, that's easy to reason about. If opening an FD takes ages,
that makes figuring out why stuff is slow a lot more complicated. This
used to happen with RPM resume for PCI devices to read the device ID &
revision, which is why we now have an API that persists that to avoid
the delay.
Sorry this feedback is coming quite late into development.
Cheers,
Daniel
Hi,
On Fri, 2019-03-29 at 18:42 +0000, Daniel Stone wrote:
> Hi,
>
> On Fri, 29 Mar 2019 at 18:14, Eric Anholt <[email protected]> wrote:
> > Paul Kocialkowski <[email protected]> writes:
> > > I'm not totally convinced that it's okay to have a delay outside of
> > > init/enumeration, even if it's a smaller delay.
> >
> > You'll have non-dumb buffers created during GL context creation, so
> > early in xserver or other KMS-and-GL-using application init anyway.
> > Seems like a perfectly fine plan to me.
>
> Yeah. The alternative is doing it once when Plymouth starts, and then
> maybe again when Weston/GNOME/Xorg/... starts, which isn't really
> ideal (or maybe even udev helpers). Doing it on probe also complicates
> profiling startup for those: if GL context or surface creation takes a
> long time, that's easy to reason about. If opening an FD takes ages,
> that makes figuring out why stuff is slow a lot more complicated. This
> used to happen with RPM resume for PCI devices to read the device ID &
> revision, which is why we now have an API that persists that to avoid
> the delay.
Sounds like we have a plan then, I'll spin up a new series allocating
the binner BO at the first non-dumb BO alloc. Thanks for your input!
> Sorry this feedback is coming quite late into development.
The feedback is definitely quite welcome! I tried a few things that
didn't pan out before using firstopen/lastclose and it's really
interesting to refine the implementation based on the shortcomings of
previous ideas.
Cheers,
Paul
--
Paul Kocialkowski, Bootlin
Embedded Linux and kernel engineering
https://bootlin.com