* [PATCH v3 0/2] drm/vc4: Binner BO management improvements @ 2019-03-28 16:13 Paul Kocialkowski 2019-03-28 16:13 ` [PATCH v3 1/2] drm/file: Rehabilitate the firstopen hook for non-legacy drivers Paul Kocialkowski 2019-03-28 16:13 ` [PATCH v3 2/2] drm/vc4: Allocated/liberate the binner BO at firstopen/lastclose Paul Kocialkowski 0 siblings, 2 replies; 6+ messages in thread From: Paul Kocialkowski @ 2019-03-28 16:13 UTC (permalink / raw) To: dri-devel, linux-kernel Cc: Maarten Lankhorst, Maxime Ripard, Sean Paul, David Airlie, Daniel Vetter, Eric Anholt, Eben Upton, Thomas Petazzoni, Paul Kocialkowski 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 (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 | 3 --- 6 files changed, 32 insertions(+), 19 deletions(-) -- 2.21.0 ^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH v3 1/2] drm/file: Rehabilitate the firstopen hook for non-legacy drivers 2019-03-28 16:13 [PATCH v3 0/2] drm/vc4: Binner BO management improvements Paul Kocialkowski @ 2019-03-28 16:13 ` Paul Kocialkowski 2019-03-29 8:35 ` Daniel Vetter 2019-03-28 16:13 ` [PATCH v3 2/2] drm/vc4: Allocated/liberate the binner BO at firstopen/lastclose Paul Kocialkowski 1 sibling, 1 reply; 6+ messages in thread From: Paul Kocialkowski @ 2019-03-28 16:13 UTC (permalink / raw) To: dri-devel, linux-kernel Cc: Thomas Petazzoni, Maxime Ripard, Eben Upton, Paul Kocialkowski, David Airlie, Sean Paul 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> Reviewed-by: Eric Anholt <eric@anholt.net> --- drivers/gpu/drm/drm_file.c | 3 +-- include/drm/drm_drv.h | 3 --- 2 files changed, 1 insertion(+), 5 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..de5637494503 100644 --- a/include/drm/drm_drv.h +++ b/include/drm/drm_drv.h @@ -234,9 +234,6 @@ struct drm_driver { * kernel driver does not really own the hardware. Instead ownershipe is * handled with the help of userspace through an inheritedly racy dance * 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. */ void (*lastclose) (struct drm_device *); -- 2.21.0 _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH v3 1/2] drm/file: Rehabilitate the firstopen hook for non-legacy drivers 2019-03-28 16:13 ` [PATCH v3 1/2] drm/file: Rehabilitate the firstopen hook for non-legacy drivers Paul Kocialkowski @ 2019-03-29 8:35 ` Daniel Vetter 2019-03-29 8:37 ` Daniel Vetter 2019-03-29 8:38 ` Paul Kocialkowski 0 siblings, 2 replies; 6+ messages in thread From: Daniel Vetter @ 2019-03-29 8:35 UTC (permalink / raw) To: Paul Kocialkowski Cc: dri-devel, linux-kernel, Maarten Lankhorst, Maxime Ripard, Sean Paul, David Airlie, Daniel Vetter, Eric Anholt, Eben Upton, Thomas Petazzoni On Thu, Mar 28, 2019 at 05:13:05PM +0100, Paul Kocialkowski wrote: > 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> > Reviewed-by: Eric Anholt <eric@anholt.net> > --- > drivers/gpu/drm/drm_file.c | 3 +-- > include/drm/drm_drv.h | 3 --- > 2 files changed, 1 insertion(+), 5 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) { If you guys insist on this (still not a fan myself) pls pull it out of drm_setup - that's all just legacy hw setup in there. I still think tracking this a bit more accurately (if you want to bother with this) would be better. -Daniel > 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..de5637494503 100644 > --- a/include/drm/drm_drv.h > +++ b/include/drm/drm_drv.h > @@ -234,9 +234,6 @@ struct drm_driver { > * kernel driver does not really own the hardware. Instead ownershipe is > * handled with the help of userspace through an inheritedly racy dance > * 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. > */ > void (*lastclose) (struct drm_device *); > > -- > 2.21.0 > -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v3 1/2] drm/file: Rehabilitate the firstopen hook for non-legacy drivers 2019-03-29 8:35 ` Daniel Vetter @ 2019-03-29 8:37 ` Daniel Vetter 2019-03-29 8:38 ` Paul Kocialkowski 1 sibling, 0 replies; 6+ messages in thread From: Daniel Vetter @ 2019-03-29 8:37 UTC (permalink / raw) To: Paul Kocialkowski Cc: dri-devel, linux-kernel, Maarten Lankhorst, Maxime Ripard, Sean Paul, David Airlie, Daniel Vetter, Eric Anholt, Eben Upton, Thomas Petazzoni On Fri, Mar 29, 2019 at 09:35:34AM +0100, Daniel Vetter wrote: > On Thu, Mar 28, 2019 at 05:13:05PM +0100, Paul Kocialkowski wrote: > > 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> > > Reviewed-by: Eric Anholt <eric@anholt.net> > > --- > > drivers/gpu/drm/drm_file.c | 3 +-- > > include/drm/drm_drv.h | 3 --- > > 2 files changed, 1 insertion(+), 5 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) { > > If you guys insist on this (still not a fan myself) pls pull it out of > drm_setup - that's all just legacy hw setup in there. I still think > tracking this a bit more accurately (if you want to bother with this) > would be better. Maybe another one: Firstopen is called after ->open, which might be confusing. But we can't change that ever because legacy drivers surely rely on that somewhere (like the backwards load/unload calling sequence). -Daniel -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v3 1/2] drm/file: Rehabilitate the firstopen hook for non-legacy drivers 2019-03-29 8:35 ` Daniel Vetter 2019-03-29 8:37 ` Daniel Vetter @ 2019-03-29 8:38 ` Paul Kocialkowski 1 sibling, 0 replies; 6+ messages in thread From: Paul Kocialkowski @ 2019-03-29 8:38 UTC (permalink / raw) To: Daniel Vetter Cc: dri-devel, linux-kernel, Maarten Lankhorst, Maxime Ripard, Sean Paul, David Airlie, Eric Anholt, Eben Upton, Thomas Petazzoni Hi Daniel, On Fri, 2019-03-29 at 09:35 +0100, Daniel Vetter wrote: > On Thu, Mar 28, 2019 at 05:13:05PM +0100, Paul Kocialkowski wrote: > > 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> > > Reviewed-by: Eric Anholt <eric@anholt.net> > > --- > > drivers/gpu/drm/drm_file.c | 3 +-- > > include/drm/drm_drv.h | 3 --- > > 2 files changed, 1 insertion(+), 5 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) { > > If you guys insist on this (still not a fan myself) pls pull it out of > drm_setup - that's all just legacy hw setup in there. I still think > tracking this a bit more accurately (if you want to bother with this) > would be better. I sent out this v3 right before receiving your comments on v2, so this new iteration only takes Eric's comments into account. I think the points you made on v2 are very legitimate and I'll be working to prepare a new series that takes them into consideration :) Cheers, Paul > -Daniel > > > 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..de5637494503 100644 > > --- a/include/drm/drm_drv.h > > +++ b/include/drm/drm_drv.h > > @@ -234,9 +234,6 @@ struct drm_driver { > > * kernel driver does not really own the hardware. Instead ownershipe is > > * handled with the help of userspace through an inheritedly racy dance > > * 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. > > */ > > void (*lastclose) (struct drm_device *); > > > > -- > > 2.21.0 > > -- Paul Kocialkowski, Bootlin Embedded Linux and kernel engineering https://bootlin.com ^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH v3 2/2] drm/vc4: Allocated/liberate the binner BO at firstopen/lastclose 2019-03-28 16:13 [PATCH v3 0/2] drm/vc4: Binner BO management improvements Paul Kocialkowski 2019-03-28 16:13 ` [PATCH v3 1/2] drm/file: Rehabilitate the firstopen hook for non-legacy drivers Paul Kocialkowski @ 2019-03-28 16:13 ` Paul Kocialkowski 1 sibling, 0 replies; 6+ messages in thread From: Paul Kocialkowski @ 2019-03-28 16:13 UTC (permalink / raw) To: dri-devel, linux-kernel Cc: Maarten Lankhorst, Maxime Ripard, Sean Paul, David Airlie, Daniel Vetter, Eric Anholt, Eben Upton, Thomas Petazzoni, 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. 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 <paul.kocialkowski@bootlin.com> Reviewed-by: Eric Anholt <eric@anholt.net> --- 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 ^ permalink raw reply related [flat|nested] 6+ messages in thread
end of thread, other threads:[~2019-03-29 8:38 UTC | newest] Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-03-28 16:13 [PATCH v3 0/2] drm/vc4: Binner BO management improvements Paul Kocialkowski 2019-03-28 16:13 ` [PATCH v3 1/2] drm/file: Rehabilitate the firstopen hook for non-legacy drivers Paul Kocialkowski 2019-03-29 8:35 ` Daniel Vetter 2019-03-29 8:37 ` Daniel Vetter 2019-03-29 8:38 ` Paul Kocialkowski 2019-03-28 16:13 ` [PATCH v3 2/2] drm/vc4: Allocated/liberate the binner BO at firstopen/lastclose Paul Kocialkowski
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).