qxl: fix framebuffer unpinning
Subsystems (3)
Gerd Hoffmann
maintainerPATCH 1/1 qxl: fix framebuffer unpinning
qxl_plane_cleanup_fb() unpins the just activated framebuffer instead of the old one. Oops. Fix it.
--- drivers/gpu/drm/qxl/qxl_display.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-)
drivers/gpu/drm/qxl/qxl_display.c +4 -3
@@ -702,14 +702,15 @@ static void qxl_plane_cleanup_fb(struct drm_plane *plane,struct drm_gem_object *obj;struct qxl_bo *user_bo; -if (!plane->state->fb) { -/* we never executed prepare_fb, so there's nothing to +if (!old_state->fb) { +/* +* we never executed prepare_fb, so there's nothing to * unpin. */ return ; } - obj =to_qxl_framebuffer (plane->state->fb)->obj; + obj =to_qxl_framebuffer (old_state->fb)->obj; user_bo =gem_to_qxl_bo (obj);qxl_bo_unpin (user_bo); }
Signature · 6 lines
2.9.3 _______________________________________________ dri-devel mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/dri-devel
Quoted 20 lines · Gerd Hoffmann
qxl_plane_cleanup_fb()unpins the just activated framebuffer instead of the old one. Oops. Fix it.Cc Gabriel Krisman Bertazi <[email protected]>Fixes 1277eed5fecb8830c8cc414ad70c1ef640464bc0Signed-off-by Gerd Hoffmann <[email protected]>--- drivers/gpu/drm/qxl/qxl_display.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-)drivers/gpu/drm/qxl/qxl_display.c +0
-2@@ -702,14 +702,15 @@ static void qxl_plane_cleanup_fb(struct drm_plane *plane,struct drm_gem_object *obj;struct qxl_bo *user_bo; -if (!plane->state->fb) { -/* we never executed prepare_fb, so there's nothing to
drivers/gpu/drm/qxl/qxl_display.c static void qxl_plane_cleanup_fb(struct drm_plane *plane, · line 713
Quoted 10 lines
drivers/gpu/drm/qxl/qxl_display.c +4
-1+if (!old_state->fb) { +/* +* we never executed prepare_fb, so there's nothing to * unpin. */ return ; } - obj =to_qxl_framebuffer (plane->state->fb)->obj; + obj =to_qxl_framebuffer (old_state->fb)->obj;
I am not sure about this patch. My idea was to pin/unpin the current buffer on prepare/cleanup, and only keep it pinned during the atomic commit. This was the goal of 37235451c6990683a8, for instance.
Anyway, if that goal is wrong, I think that, with your change, we forget to unpin the final plane before removing the device.
Thanks,
Signature · 5 lines
Gabriel Krisman Bertazi _______________________________________________ dri-devel mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/dri-devel
Gerd Hoffmann
maintainerQuoted 32 lines · Gabriel Krisman Bertazi
Quoted 30 lines · Gerd Hoffmann
qxl_plane_cleanup_fb()unpins the just activated framebuffer instead of the old one. Oops. Fix it.Cc Gabriel Krisman Bertazi <[email protected]>Fixes 1277eed5fecb8830c8cc414ad70c1ef640464bc0Signed-off-by Gerd Hoffmann <[email protected]>---
drivers/gpu/drm/qxl/qxl_display.c | 7 ++++---1 file changed, 4 insertions(+), 3 deletions(-)drivers/gpu/drm/qxl/qxl_display.c +0
-0@@ -702,14 +702,15 @@ static void qxl_plane_cleanup_fb(struct drm_plane *plane, struct drm_gem_object *obj; struct qxl_bo *user_bo;drivers/gpu/drm/qxl/qxl_display.c +0
-2-if (!plane->state->fb) { -/* we never executed prepare_fb, so there's nothing to
drivers/gpu/drm/qxl/qxl_display.c +3
-0+if (!old_state->fb) { +/* +* we never executed prepare_fb, so there's nothing to
* unpin.
*/
return;
Quoted 8 lines
drivers/gpu/drm/qxl/qxl_display.c static void qxl_plane_cleanup_fb(struct drm_plane *plane, · line 713
Quoted 4 lines
}
drivers/gpu/drm/qxl/qxl_display.c +1
-1- obj =to_qxl_framebuffer (plane->state->fb)->obj; + obj =to_qxl_framebuffer (old_state->fb)->obj;I am not sure about this patch. My idea was to pin/unpin the current buffer on prepare/cleanup, and only keep it pinned during the atomic commit. This was the goal of
37235451c6990683a8, for instance.
I'm pretty sure the buffer must remain pinned while it is displayed.
Quoted 3 lines
Anyway, if that goal is wrong, I think that, with your change, we forget to
unpin the final plane before removing the device.
"removing the device"? qxl can't be hotplugged ... Or do you mean "rmmod qxl"?
cheers,
Gerd
_______________________________________________
dri-devel mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Quoted 42 lines · Gerd Hoffmann
Quoted 32 lines · Gabriel Krisman Bertazi
Quoted 30 lines · Gerd Hoffmann
qxl_plane_cleanup_fb()unpins the just activated framebuffer instead of the old one. Oops. Fix it.Cc Gabriel Krisman Bertazi <[email protected]>Fixes 1277eed5fecb8830c8cc414ad70c1ef640464bc0Signed-off-by Gerd Hoffmann <[email protected]>---
drivers/gpu/drm/qxl/qxl_display.c | 7 ++++---1 file changed, 4 insertions(+), 3 deletions(-)drivers/gpu/drm/qxl/qxl_display.c +0
-0@@ -702,14 +702,15 @@ static void qxl_plane_cleanup_fb(struct drm_plane *plane, struct drm_gem_object *obj; struct qxl_bo *user_bo;drivers/gpu/drm/qxl/qxl_display.c +0
-2-if (!plane->state->fb) { -/* we never executed prepare_fb, so there's nothing to
drivers/gpu/drm/qxl/qxl_display.c +3
-0+if (!old_state->fb) { +/* +* we never executed prepare_fb, so there's nothing to
* unpin.
*/
return;Quoted 8 lines
drivers/gpu/drm/qxl/qxl_display.c static void qxl_plane_cleanup_fb(struct drm_plane *plane, · line 713
Quoted 4 lines
}
drivers/gpu/drm/qxl/qxl_display.c +1
-1- obj =to_qxl_framebuffer (plane->state->fb)->obj; + obj =to_qxl_framebuffer (old_state->fb)->obj;I am not sure about this patch. My idea was to pin/unpin the current buffer on prepare/cleanup, and only keep it pinned during the atomic commit. This was the goal of
37235451c6990683a8, for instance.
Quoted 2 lines
I'm pretty sure the buffer must remain pinned while it is displayed.
ok, then it makes sense indeed :)
Quoted 6 lines
Quoted 3 lines
Anyway, if that goal is wrong, I think that, with your change, we forget to
unpin the final plane before removing the device."removing the device"? qxl can't be hotplugged ... Or do you mean "rmmod qxl"?
rmmod qxl
Quoted 4 lines
cheers,
Gerd
Signature · 5 lines
Gabriel Krisman Bertazi _______________________________________________ dri-devel mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/dri-devel
Gerd Hoffmann
maintainerHi,
Quoted 4 lines
Quoted 2 lines
"removing the device"? qxl can't be hotplugged ... Or do you mean "rmmod qxl"?
rmmod qxl
rmmod: ERROR: Module qxl is in use.
How do you do that? CONFIG_FBCON=n?
cheers,
Gerd
_______________________________________________
dri-devel mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Quoted 10 lines · Gerd Hoffmann
Hi,
Quoted 4 lines
Quoted 2 lines
"removing the device"? qxl can't be hotplugged ... Or do you mean "rmmod qxl"?
rmmod qxl
rmmod: ERROR: Module qxl is in use.
How do you do that?
CONFIG_FBCON=n?
I can simply disable X, lightdm or whatever is using it and then rmmod.
Signature · 5 lines
Gabriel Krisman Bertazi _______________________________________________ dri-devel mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/dri-devel
Gerd Hoffmann
maintainerQuoted 7 lines · Gabriel Krisman Bertazi
Quoted 5 lines · Gerd Hoffmann
Hi,
Quoted 3 lines
Quoted 2 lines
"removing the device"? qxl can't be hotplugged ... Or do you mean "rmmod qxl"?
Quoted 8 lines
Quoted 5 lines
Quoted 1 line
rmmod qxl
rmmod: ERROR: Module qxl is in use.
How do you do that? CONFIG_FBCON=n?
I can simply disable X, lightdm or whatever is using it and then rmmod.
Works for me only with CONFIG_FBCON=n, otherwise the fbcon framebuffer keeps qxl busy. So is most cases (typical distro kernel build with FBCON=y) you simply can't rmmod qxl.
I agree that "rmmod qxl" should be fixed, but IMHO this is independent from this bugfix, and I'd like to get it out of the door ASAP. Can I get your reviewed-by?
cheers,
Gerd
_______________________________________________
dri-devel mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Gabriel Krisman Bertazi
Reviewed-byQuoted 10 lines · Gerd Hoffmann
Quoted 2 lines
I can simply disable X, lightdm or whatever is using it and then rmmod.
Works for me only with
CONFIG_FBCON=n, otherwise the fbcon framebuffer keeps qxl busy. So is most cases (typical distro kernel build with FBCON=y) you simply can't rmmod qxl.I agree that "rmmod qxl" should be fixed, but IMHO this is independent from this bugfix, and I'd like to get it out of the door ASAP. Can I get your reviewed-by?
Yes, getting this out of the door asap makes total sense to me. Please add my reviewed-by:
Thanks,
Signature · 5 lines
Gabriel Krisman Bertazi _______________________________________________ dri-devel mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/dri-devel
Gerd Hoffmann
maintainerQuoted 16 lines · Gabriel Krisman Bertazi
Quoted 8 lines · Gerd Hoffmann
Quoted 2 lines
I can simply disable X, lightdm or whatever is using it and then rmmod.
Works for me only with
CONFIG_FBCON=n, otherwise the fbcon framebuffer
keeps qxl busy. So is most cases (typical distro kernel build with FBCON=y) you simply can't rmmod qxl.Quoted 5 lines
I agree that "rmmod qxl" should be fixed, but IMHO this is independent
from this bugfix, and I'd like to get it out of the door ASAP. Can I
get your reviewed-by?
Quoted 5 lines
Yes, getting this out of the door asap makes total sense to me. Please
add my reviewed-by:Reviewed-by Gabriel Krisman Bertazi <[email protected]>
Thanks. Pushed to misc-fixes.
cheers,
Gerd
_______________________________________________
dri-devel mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/dri-devel