Hello,
Here are two (attempted) fixes for the AST DRM driver. The issues they fix are
both seen when the ast driver is unloaded (tested on Power9, although it looks
like the second one is architecture independent).
I'm fairly confident about the first fix, as it looks pretty straight forward.
I'm not sure if the second patch uses the right approach, so I'd appreciate some
feedback. I've had a good look around, and it does seem to fix the problem but
I don't know this area at all.
Note that while it does seem odd to take a reference just before calling
drm_framebuffer_remove(), that seems to be be necessary because
drm_framebuffer_remove() is expecting the framebuffer to be dynamically
allocated (but the AST driver directly contains it) (I saw some references to
this being legacy behaviour).
Cheers,
Sam.
Sam Bobroff (2):
drm/ast: Fix incorrect free on ioregs
drm/ast: Fix connector leak during driver unload
drivers/gpu/drm/ast/ast_fb.c | 4 ++++
drivers/gpu/drm/ast/ast_main.c | 3 ++-
2 files changed, 6 insertions(+), 1 deletion(-)
--
2.19.0.2.gcad72f5712
If the platform has no IO space, ioregs is placed next to the already
allocated regs. In this case, it should not be separately freed.
This prevents a kernel warning from __vunmap "Trying to vfree()
nonexistent vm area" when unloading the driver.
Fixes: 0dd68309b9c5 ("drm/ast: Try to use MMIO registers when PIO isn't supported")
Signed-off-by: Sam Bobroff <[email protected]>
---
drivers/gpu/drm/ast/ast_main.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/ast/ast_main.c b/drivers/gpu/drm/ast/ast_main.c
index dac355812adc..373700c05a00 100644
--- a/drivers/gpu/drm/ast/ast_main.c
+++ b/drivers/gpu/drm/ast/ast_main.c
@@ -583,7 +583,8 @@ void ast_driver_unload(struct drm_device *dev)
drm_mode_config_cleanup(dev);
ast_mm_fini(ast);
- pci_iounmap(dev->pdev, ast->ioregs);
+ if (ast->ioregs != ast->regs + AST_IO_MM_OFFSET)
+ pci_iounmap(dev->pdev, ast->ioregs);
pci_iounmap(dev->pdev, ast->regs);
kfree(ast);
}
--
2.19.0.2.gcad72f5712
When unloading the ast driver, a warning message is printed by
drm_mode_config_cleanup() because a reference is still held to one of
the drm_connector structs.
Correct this by calling drm_framebuffer_remove() in
ast_fbdev_destroy().
Signed-off-by: Sam Bobroff <[email protected]>
---
drivers/gpu/drm/ast/ast_fb.c | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/drivers/gpu/drm/ast/ast_fb.c b/drivers/gpu/drm/ast/ast_fb.c
index 0cd827e11fa2..655372ea81e9 100644
--- a/drivers/gpu/drm/ast/ast_fb.c
+++ b/drivers/gpu/drm/ast/ast_fb.c
@@ -263,6 +263,10 @@ static void ast_fbdev_destroy(struct drm_device *dev,
{
struct ast_framebuffer *afb = &afbdev->afb;
+ /* drm_framebuffer_remove() expects us to hold a ref, which it
+ * will drop, so take one: */
+ drm_framebuffer_get(&afb->base);
+ drm_framebuffer_remove(&afb->base);
drm_fb_helper_unregister_fbi(&afbdev->helper);
if (afb->obj) {
--
2.19.0.2.gcad72f5712
On Mon, 5 Nov 2018 at 15:59, Sam Bobroff <[email protected]> wrote:
>
> When unloading the ast driver, a warning message is printed by
> drm_mode_config_cleanup() because a reference is still held to one of
> the drm_connector structs.
>
> Correct this by calling drm_framebuffer_remove() in
> ast_fbdev_destroy().
>
> Signed-off-by: Sam Bobroff <[email protected]>
> ---
> drivers/gpu/drm/ast/ast_fb.c | 4 ++++
> 1 file changed, 4 insertions(+)
>
> diff --git a/drivers/gpu/drm/ast/ast_fb.c b/drivers/gpu/drm/ast/ast_fb.c
> index 0cd827e11fa2..655372ea81e9 100644
> --- a/drivers/gpu/drm/ast/ast_fb.c
> +++ b/drivers/gpu/drm/ast/ast_fb.c
> @@ -263,6 +263,10 @@ static void ast_fbdev_destroy(struct drm_device *dev,
> {
> struct ast_framebuffer *afb = &afbdev->afb;
>
> + /* drm_framebuffer_remove() expects us to hold a ref, which it
> + * will drop, so take one: */
> + drm_framebuffer_get(&afb->base);
> + drm_framebuffer_remove(&afb->base);
This doesn't seem corret, no other driver does this pattern, and I
can't believe ast is special here.
The get just doesn't make sense.
Dave.
On Thu, Nov 29, 2018 at 09:40:53AM +1000, Dave Airlie wrote:
> On Mon, 5 Nov 2018 at 15:59, Sam Bobroff <[email protected]> wrote:
> >
> > When unloading the ast driver, a warning message is printed by
> > drm_mode_config_cleanup() because a reference is still held to one of
> > the drm_connector structs.
> >
> > Correct this by calling drm_framebuffer_remove() in
> > ast_fbdev_destroy().
> >
> > Signed-off-by: Sam Bobroff <[email protected]>
> > ---
> > drivers/gpu/drm/ast/ast_fb.c | 4 ++++
> > 1 file changed, 4 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/ast/ast_fb.c b/drivers/gpu/drm/ast/ast_fb.c
> > index 0cd827e11fa2..655372ea81e9 100644
> > --- a/drivers/gpu/drm/ast/ast_fb.c
> > +++ b/drivers/gpu/drm/ast/ast_fb.c
> > @@ -263,6 +263,10 @@ static void ast_fbdev_destroy(struct drm_device *dev,
> > {
> > struct ast_framebuffer *afb = &afbdev->afb;
> >
> > + /* drm_framebuffer_remove() expects us to hold a ref, which it
> > + * will drop, so take one: */
> > + drm_framebuffer_get(&afb->base);
> > + drm_framebuffer_remove(&afb->base);
>
> This doesn't seem corret, no other driver does this pattern, and I
> can't believe ast is special here.
>
> The get just doesn't make sense.
Thanks for having a look at this, as I said in the cover letter I was
concerned that it might not be a good fix.
But the AST driver does seem to be special (or just old?) because it
embeds the drm_framebuffer directly into ast_fbdev and (almost all)
other drivers dynamically allocate and reference count theirs.
The drm_framebuffer_get() certainly looks weird but it is there in order
to cause drm_framebuffer_remove() to call legacy_remove_fb(), which it
won't do unless the refcount is at least 2. (And because the
drm_framebuffer isn't dynamically allocated in this case we don't really
care about the reference count anyway.)
An alternative might be to call legacy_remove_fb() directly, but it's
declared static. Do you think it would be better to expose it and call
it directly from the AST driver code? Or is there some other better way
to put the drm_connectors?
> Dave.
Cheers,
Sam.
On Thu, Nov 29, 2018 at 9:05 AM Sam Bobroff <[email protected]> wrote:
>
> On Thu, Nov 29, 2018 at 09:40:53AM +1000, Dave Airlie wrote:
> > On Mon, 5 Nov 2018 at 15:59, Sam Bobroff <[email protected]> wrote:
> > >
> > > When unloading the ast driver, a warning message is printed by
> > > drm_mode_config_cleanup() because a reference is still held to one of
> > > the drm_connector structs.
> > >
> > > Correct this by calling drm_framebuffer_remove() in
> > > ast_fbdev_destroy().
> > >
> > > Signed-off-by: Sam Bobroff <[email protected]>
> > > ---
> > > drivers/gpu/drm/ast/ast_fb.c | 4 ++++
> > > 1 file changed, 4 insertions(+)
> > >
> > > diff --git a/drivers/gpu/drm/ast/ast_fb.c b/drivers/gpu/drm/ast/ast_fb.c
> > > index 0cd827e11fa2..655372ea81e9 100644
> > > --- a/drivers/gpu/drm/ast/ast_fb.c
> > > +++ b/drivers/gpu/drm/ast/ast_fb.c
> > > @@ -263,6 +263,10 @@ static void ast_fbdev_destroy(struct drm_device *dev,
> > > {
> > > struct ast_framebuffer *afb = &afbdev->afb;
> > >
> > > + /* drm_framebuffer_remove() expects us to hold a ref, which it
> > > + * will drop, so take one: */
> > > + drm_framebuffer_get(&afb->base);
> > > + drm_framebuffer_remove(&afb->base);
> >
> > This doesn't seem corret, no other driver does this pattern, and I
> > can't believe ast is special here.
> >
> > The get just doesn't make sense.
>
> Thanks for having a look at this, as I said in the cover letter I was
> concerned that it might not be a good fix.
>
> But the AST driver does seem to be special (or just old?) because it
> embeds the drm_framebuffer directly into ast_fbdev and (almost all)
> other drivers dynamically allocate and reference count theirs.
>
> The drm_framebuffer_get() certainly looks weird but it is there in order
> to cause drm_framebuffer_remove() to call legacy_remove_fb(), which it
> won't do unless the refcount is at least 2. (And because the
> drm_framebuffer isn't dynamically allocated in this case we don't really
> care about the reference count anyway.)
>
> An alternative might be to call legacy_remove_fb() directly, but it's
> declared static. Do you think it would be better to expose it and call
> it directly from the AST driver code? Or is there some other better way
> to put the drm_connectors?
Your problem isn't the dynamic fb vs. embedded fb for fbdev (you're
already using drm_framebuffer_unregister_private to handle that). Your
problem is you're not shutting down stuff on driver unload, which
means the fb is still in use. drm_atomic_helper_shutdown() takes care
of that for atomic drivers.
No idea anymore what to do for legacy code, probably need to open code
a shutdown sequence. Definitely not the above.
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
On Thu, Nov 29, 2018 at 09:56:53AM +0100, Daniel Vetter wrote:
> On Thu, Nov 29, 2018 at 9:05 AM Sam Bobroff <[email protected]> wrote:
> >
> > On Thu, Nov 29, 2018 at 09:40:53AM +1000, Dave Airlie wrote:
> > > On Mon, 5 Nov 2018 at 15:59, Sam Bobroff <[email protected]> wrote:
> > > >
> > > > When unloading the ast driver, a warning message is printed by
> > > > drm_mode_config_cleanup() because a reference is still held to one of
> > > > the drm_connector structs.
> > > >
> > > > Correct this by calling drm_framebuffer_remove() in
> > > > ast_fbdev_destroy().
> > > >
> > > > Signed-off-by: Sam Bobroff <[email protected]>
> > > > ---
> > > > drivers/gpu/drm/ast/ast_fb.c | 4 ++++
> > > > 1 file changed, 4 insertions(+)
> > > >
> > > > diff --git a/drivers/gpu/drm/ast/ast_fb.c b/drivers/gpu/drm/ast/ast_fb.c
> > > > index 0cd827e11fa2..655372ea81e9 100644
> > > > --- a/drivers/gpu/drm/ast/ast_fb.c
> > > > +++ b/drivers/gpu/drm/ast/ast_fb.c
> > > > @@ -263,6 +263,10 @@ static void ast_fbdev_destroy(struct drm_device *dev,
> > > > {
> > > > struct ast_framebuffer *afb = &afbdev->afb;
> > > >
> > > > + /* drm_framebuffer_remove() expects us to hold a ref, which it
> > > > + * will drop, so take one: */
> > > > + drm_framebuffer_get(&afb->base);
> > > > + drm_framebuffer_remove(&afb->base);
> > >
> > > This doesn't seem corret, no other driver does this pattern, and I
> > > can't believe ast is special here.
> > >
> > > The get just doesn't make sense.
> >
> > Thanks for having a look at this, as I said in the cover letter I was
> > concerned that it might not be a good fix.
> >
> > But the AST driver does seem to be special (or just old?) because it
> > embeds the drm_framebuffer directly into ast_fbdev and (almost all)
> > other drivers dynamically allocate and reference count theirs.
> >
> > The drm_framebuffer_get() certainly looks weird but it is there in order
> > to cause drm_framebuffer_remove() to call legacy_remove_fb(), which it
> > won't do unless the refcount is at least 2. (And because the
> > drm_framebuffer isn't dynamically allocated in this case we don't really
> > care about the reference count anyway.)
> >
> > An alternative might be to call legacy_remove_fb() directly, but it's
> > declared static. Do you think it would be better to expose it and call
> > it directly from the AST driver code? Or is there some other better way
> > to put the drm_connectors?
>
> Your problem isn't the dynamic fb vs. embedded fb for fbdev (you're
> already using drm_framebuffer_unregister_private to handle that). Your
> problem is you're not shutting down stuff on driver unload, which
> means the fb is still in use. drm_atomic_helper_shutdown() takes care
> of that for atomic drivers.
>
> No idea anymore what to do for legacy code, probably need to open code
> a shutdown sequence. Definitely not the above.
> -Daniel
Well, it looks like drm_crtc_force_disable_all() would also do the job,
and from looking at nouveau_display_fini() it's used there as an
alternative to drm_atomic_helper_shutdown().
Would it be reasonable to call that at the start of
ast_fbdev_destroy() instead? (Testing shows that it does allow the
drm_connector to be released. Is it enough/correct though?)
Cheers,
Sam.
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> +41 (0) 79 365 57 48 - http://blog.ffwll.ch
>
On Fri, Nov 30, 2018 at 11:17:51AM +1100, Sam Bobroff wrote:
> On Thu, Nov 29, 2018 at 09:56:53AM +0100, Daniel Vetter wrote:
> > On Thu, Nov 29, 2018 at 9:05 AM Sam Bobroff <[email protected]> wrote:
> > >
> > > On Thu, Nov 29, 2018 at 09:40:53AM +1000, Dave Airlie wrote:
> > > > On Mon, 5 Nov 2018 at 15:59, Sam Bobroff <[email protected]> wrote:
> > > > >
> > > > > When unloading the ast driver, a warning message is printed by
> > > > > drm_mode_config_cleanup() because a reference is still held to one of
> > > > > the drm_connector structs.
> > > > >
> > > > > Correct this by calling drm_framebuffer_remove() in
> > > > > ast_fbdev_destroy().
> > > > >
> > > > > Signed-off-by: Sam Bobroff <[email protected]>
> > > > > ---
> > > > > drivers/gpu/drm/ast/ast_fb.c | 4 ++++
> > > > > 1 file changed, 4 insertions(+)
> > > > >
> > > > > diff --git a/drivers/gpu/drm/ast/ast_fb.c b/drivers/gpu/drm/ast/ast_fb.c
> > > > > index 0cd827e11fa2..655372ea81e9 100644
> > > > > --- a/drivers/gpu/drm/ast/ast_fb.c
> > > > > +++ b/drivers/gpu/drm/ast/ast_fb.c
> > > > > @@ -263,6 +263,10 @@ static void ast_fbdev_destroy(struct drm_device *dev,
> > > > > {
> > > > > struct ast_framebuffer *afb = &afbdev->afb;
> > > > >
> > > > > + /* drm_framebuffer_remove() expects us to hold a ref, which it
> > > > > + * will drop, so take one: */
> > > > > + drm_framebuffer_get(&afb->base);
> > > > > + drm_framebuffer_remove(&afb->base);
> > > >
> > > > This doesn't seem corret, no other driver does this pattern, and I
> > > > can't believe ast is special here.
> > > >
> > > > The get just doesn't make sense.
> > >
> > > Thanks for having a look at this, as I said in the cover letter I was
> > > concerned that it might not be a good fix.
> > >
> > > But the AST driver does seem to be special (or just old?) because it
> > > embeds the drm_framebuffer directly into ast_fbdev and (almost all)
> > > other drivers dynamically allocate and reference count theirs.
> > >
> > > The drm_framebuffer_get() certainly looks weird but it is there in order
> > > to cause drm_framebuffer_remove() to call legacy_remove_fb(), which it
> > > won't do unless the refcount is at least 2. (And because the
> > > drm_framebuffer isn't dynamically allocated in this case we don't really
> > > care about the reference count anyway.)
> > >
> > > An alternative might be to call legacy_remove_fb() directly, but it's
> > > declared static. Do you think it would be better to expose it and call
> > > it directly from the AST driver code? Or is there some other better way
> > > to put the drm_connectors?
> >
> > Your problem isn't the dynamic fb vs. embedded fb for fbdev (you're
> > already using drm_framebuffer_unregister_private to handle that). Your
> > problem is you're not shutting down stuff on driver unload, which
> > means the fb is still in use. drm_atomic_helper_shutdown() takes care
> > of that for atomic drivers.
> >
> > No idea anymore what to do for legacy code, probably need to open code
> > a shutdown sequence. Definitely not the above.
> > -Daniel
>
> Well, it looks like drm_crtc_force_disable_all() would also do the job,
> and from looking at nouveau_display_fini() it's used there as an
> alternative to drm_atomic_helper_shutdown().
Ah right, I tried looking for that one but didn't find it with a quick
scan.
> Would it be reasonable to call that at the start of
> ast_fbdev_destroy() instead? (Testing shows that it does allow the
> drm_connector to be released. Is it enough/correct though?)
Yes.
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
On Fri, Nov 30, 2018 at 10:41:08AM +0100, Daniel Vetter wrote:
> On Fri, Nov 30, 2018 at 11:17:51AM +1100, Sam Bobroff wrote:
> > On Thu, Nov 29, 2018 at 09:56:53AM +0100, Daniel Vetter wrote:
> > > On Thu, Nov 29, 2018 at 9:05 AM Sam Bobroff <[email protected]> wrote:
> > > >
> > > > On Thu, Nov 29, 2018 at 09:40:53AM +1000, Dave Airlie wrote:
> > > > > On Mon, 5 Nov 2018 at 15:59, Sam Bobroff <[email protected]> wrote:
> > > > > >
> > > > > > When unloading the ast driver, a warning message is printed by
> > > > > > drm_mode_config_cleanup() because a reference is still held to one of
> > > > > > the drm_connector structs.
> > > > > >
> > > > > > Correct this by calling drm_framebuffer_remove() in
> > > > > > ast_fbdev_destroy().
> > > > > >
> > > > > > Signed-off-by: Sam Bobroff <[email protected]>
> > > > > > ---
> > > > > > drivers/gpu/drm/ast/ast_fb.c | 4 ++++
> > > > > > 1 file changed, 4 insertions(+)
> > > > > >
> > > > > > diff --git a/drivers/gpu/drm/ast/ast_fb.c b/drivers/gpu/drm/ast/ast_fb.c
> > > > > > index 0cd827e11fa2..655372ea81e9 100644
> > > > > > --- a/drivers/gpu/drm/ast/ast_fb.c
> > > > > > +++ b/drivers/gpu/drm/ast/ast_fb.c
> > > > > > @@ -263,6 +263,10 @@ static void ast_fbdev_destroy(struct drm_device *dev,
> > > > > > {
> > > > > > struct ast_framebuffer *afb = &afbdev->afb;
> > > > > >
> > > > > > + /* drm_framebuffer_remove() expects us to hold a ref, which it
> > > > > > + * will drop, so take one: */
> > > > > > + drm_framebuffer_get(&afb->base);
> > > > > > + drm_framebuffer_remove(&afb->base);
> > > > >
> > > > > This doesn't seem corret, no other driver does this pattern, and I
> > > > > can't believe ast is special here.
> > > > >
> > > > > The get just doesn't make sense.
> > > >
> > > > Thanks for having a look at this, as I said in the cover letter I was
> > > > concerned that it might not be a good fix.
> > > >
> > > > But the AST driver does seem to be special (or just old?) because it
> > > > embeds the drm_framebuffer directly into ast_fbdev and (almost all)
> > > > other drivers dynamically allocate and reference count theirs.
> > > >
> > > > The drm_framebuffer_get() certainly looks weird but it is there in order
> > > > to cause drm_framebuffer_remove() to call legacy_remove_fb(), which it
> > > > won't do unless the refcount is at least 2. (And because the
> > > > drm_framebuffer isn't dynamically allocated in this case we don't really
> > > > care about the reference count anyway.)
> > > >
> > > > An alternative might be to call legacy_remove_fb() directly, but it's
> > > > declared static. Do you think it would be better to expose it and call
> > > > it directly from the AST driver code? Or is there some other better way
> > > > to put the drm_connectors?
> > >
> > > Your problem isn't the dynamic fb vs. embedded fb for fbdev (you're
> > > already using drm_framebuffer_unregister_private to handle that). Your
> > > problem is you're not shutting down stuff on driver unload, which
> > > means the fb is still in use. drm_atomic_helper_shutdown() takes care
> > > of that for atomic drivers.
> > >
> > > No idea anymore what to do for legacy code, probably need to open code
> > > a shutdown sequence. Definitely not the above.
> > > -Daniel
> >
> > Well, it looks like drm_crtc_force_disable_all() would also do the job,
> > and from looking at nouveau_display_fini() it's used there as an
> > alternative to drm_atomic_helper_shutdown().
>
> Ah right, I tried looking for that one but didn't find it with a quick
> scan.
>
> > Would it be reasonable to call that at the start of
> > ast_fbdev_destroy() instead? (Testing shows that it does allow the
> > drm_connector to be released. Is it enough/correct though?)
>
> Yes.
> -Daniel
Great, I'll post a v2 with that change.
Cheers,
Sam.