Message ID | 20170918074145.2257-1-kraxel@redhat.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Gerd Hoffmann <kraxel@redhat.com> writes: > qxl_plane_cleanup_fb() unpins the just activated framebuffer > instead of the old one. Oops. Fix it. > > Cc: Gabriel Krisman Bertazi <krisman@collabora.co.uk> > Fixes: 1277eed5fecb8830c8cc414ad70c1ef640464bc0 > Signed-off-by: Gerd Hoffmann <kraxel@redhat.com> > --- > drivers/gpu/drm/qxl/qxl_display.c | 7 ++++--- > 1 file changed, 4 insertions(+), 3 deletions(-) > > diff --git a/drivers/gpu/drm/qxl/qxl_display.c b/drivers/gpu/drm/qxl/qxl_display.c > index e1dd05423e..afbf50d0c0 100644 > --- a/drivers/gpu/drm/qxl/qxl_display.c > +++ b/drivers/gpu/drm/qxl/qxl_display.c > @@ -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; 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,
On Tue, 2017-09-19 at 11:35 -0300, Gabriel Krisman Bertazi wrote: > Gerd Hoffmann <kraxel@redhat.com> writes: > > > qxl_plane_cleanup_fb() unpins the just activated framebuffer > > instead of the old one. Oops. Fix it. > > > > Cc: Gabriel Krisman Bertazi <krisman@collabora.co.uk> > > Fixes: 1277eed5fecb8830c8cc414ad70c1ef640464bc0 > > Signed-off-by: Gerd Hoffmann <kraxel@redhat.com> > > --- > > drivers/gpu/drm/qxl/qxl_display.c | 7 ++++--- > > 1 file changed, 4 insertions(+), 3 deletions(-) > > > > diff --git a/drivers/gpu/drm/qxl/qxl_display.c > > b/drivers/gpu/drm/qxl/qxl_display.c > > index e1dd05423e..afbf50d0c0 100644 > > --- a/drivers/gpu/drm/qxl/qxl_display.c > > +++ b/drivers/gpu/drm/qxl/qxl_display.c > > @@ -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; > > 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. > 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
Gerd Hoffmann <kraxel@redhat.com> writes: > On Tue, 2017-09-19 at 11:35 -0300, Gabriel Krisman Bertazi wrote: >> Gerd Hoffmann <kraxel@redhat.com> writes: >> >> > qxl_plane_cleanup_fb() unpins the just activated framebuffer >> > instead of the old one. Oops. Fix it. >> > >> > Cc: Gabriel Krisman Bertazi <krisman@collabora.co.uk> >> > Fixes: 1277eed5fecb8830c8cc414ad70c1ef640464bc0 >> > Signed-off-by: Gerd Hoffmann <kraxel@redhat.com> >> > --- >> > drivers/gpu/drm/qxl/qxl_display.c | 7 ++++--- >> > 1 file changed, 4 insertions(+), 3 deletions(-) >> > >> > diff --git a/drivers/gpu/drm/qxl/qxl_display.c >> > b/drivers/gpu/drm/qxl/qxl_display.c >> > index e1dd05423e..afbf50d0c0 100644 >> > --- a/drivers/gpu/drm/qxl/qxl_display.c >> > +++ b/drivers/gpu/drm/qxl/qxl_display.c >> > @@ -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; >> >> 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. > ok, then it makes sense indeed :) >> 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 > > cheers, > Gerd >
Hi, > > "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
Gerd Hoffmann <kraxel@redhat.com> writes: > Hi, > >> > "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.
On Fri, 2017-09-22 at 02:03 -0300, Gabriel Krisman Bertazi wrote: > Gerd Hoffmann <kraxel@redhat.com> writes: > > > Hi, > > > > > > "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. 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
Gerd Hoffmann <kraxel@redhat.com> writes: >> 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: Reviewed-by: Gabriel Krisman Bertazi <krisman@collabora.co.uk> Thanks,
On Fri, 2017-09-22 at 14:37 -0300, Gabriel Krisman Bertazi wrote: > Gerd Hoffmann <kraxel@redhat.com> writes: > > > > 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: > > Reviewed-by: Gabriel Krisman Bertazi <krisman@collabora.co.uk> Thanks. Pushed to misc-fixes. cheers, Gerd
diff --git a/drivers/gpu/drm/qxl/qxl_display.c b/drivers/gpu/drm/qxl/qxl_display.c index e1dd05423e..afbf50d0c0 100644 --- a/drivers/gpu/drm/qxl/qxl_display.c +++ b/drivers/gpu/drm/qxl/qxl_display.c @@ -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); }
qxl_plane_cleanup_fb() unpins the just activated framebuffer instead of the old one. Oops. Fix it. Cc: Gabriel Krisman Bertazi <krisman@collabora.co.uk> Fixes: 1277eed5fecb8830c8cc414ad70c1ef640464bc0 Signed-off-by: Gerd Hoffmann <kraxel@redhat.com> --- drivers/gpu/drm/qxl/qxl_display.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-)