Two AST driver fixes

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(+)

drivers/gpu/drm/ast/ast_fb.c +0 -0

index 0cd827e11fa2..655372ea81e9 100644

Quoted 18 lines · Sam Bobroff

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(+)

drivers/gpu/drm/ast/ast_fb.c +0 -0

index 0cd827e11fa2..655372ea81e9 100644

drivers/gpu/drm/ast/ast_fb.c static void ast_fbdev_destroy(struct drm_device *dev, · line 269

Quoted 7 lines

drivers/gpu/drm/ast/ast_fb.c +4 -0

@@ -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.

Quoted 30 lines · Dave Airlie

Quoted 18 lines · Sam Bobroff

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(+)

drivers/gpu/drm/ast/ast_fb.c +0 -0

index 0cd827e11fa2..655372ea81e9 100644

drivers/gpu/drm/ast/ast_fb.c static void ast_fbdev_destroy(struct drm_device *dev, · line 269

Quoted 7 lines

drivers/gpu/drm/ast/ast_fb.c +4 -0

@@ -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.

Quoted 1 line

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?

Quoted 47 lines · Sam Bobroff

Quoted 30 lines · Dave Airlie

Quoted 18 lines · Sam Bobroff

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(+)

drivers/gpu/drm/ast/ast_fb.c +0 -0

index 0cd827e11fa2..655372ea81e9 100644

drivers/gpu/drm/ast/ast_fb.c static void ast_fbdev_destroy(struct drm_device *dev, · line 269

Quoted 7 lines

drivers/gpu/drm/ast/ast_fb.c +4 -0

@@ -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.

Quoted 1 line

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.)

Quoted 4 lines

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

Signature · 3 lines

Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Quoted 59 lines · Daniel Vetter

Quoted 47 lines · Sam Bobroff

Quoted 30 lines · Dave Airlie

Quoted 18 lines · Sam Bobroff

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(+)

drivers/gpu/drm/ast/ast_fb.c +0 -0

index 0cd827e11fa2..655372ea81e9 100644

drivers/gpu/drm/ast/ast_fb.c static void ast_fbdev_destroy(struct drm_device *dev, · line 269

Quoted 7 lines

drivers/gpu/drm/ast/ast_fb.c +4 -0

@@ -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.

Quoted 1 line

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.)

Quoted 4 lines

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.

Quoted 3 lines

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?)

Quoted 60 lines · Sam Bobroff

Quoted 59 lines · Daniel Vetter

Quoted 47 lines · Sam Bobroff

Quoted 30 lines · Dave Airlie

Quoted 18 lines · Sam Bobroff

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(+)

drivers/gpu/drm/ast/ast_fb.c +0 -0

index 0cd827e11fa2..655372ea81e9 100644

drivers/gpu/drm/ast/ast_fb.c static void ast_fbdev_destroy(struct drm_device *dev, · line 269

Quoted 7 lines

drivers/gpu/drm/ast/ast_fb.c +4 -0

@@ -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.

Quoted 1 line

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.)

Quoted 4 lines

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.

Quoted 7 lines

Quoted 3 lines

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.

Quoted 3 lines

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