[PATCH] qxl: fix framebuffer unpinning

[PATCH] qxl: fix framebuffer unpinning

[Gerd Hoffmann]

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;
 	user_bo = gem_to_qxl_bo(obj);
 	qxl_bo_unpin(user_bo);
 }
-- 
2.9.3

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] qxl: fix framebuffer unpinning

[Gabriel Krisman Bertazi]

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,

-- 
Gabriel Krisman Bertazi
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] qxl: fix framebuffer unpinning

[Gerd Hoffmann]

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

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] qxl: fix framebuffer unpinning

[Gabriel Krisman Bertazi]

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
>

-- 
Gabriel Krisman Bertazi
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] qxl: fix framebuffer unpinning

[Gerd Hoffmann]

  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

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] qxl: fix framebuffer unpinning

[Gabriel Krisman Bertazi]

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.

-- 
Gabriel Krisman Bertazi
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] qxl: fix framebuffer unpinning

[Gerd Hoffmann]

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

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] qxl: fix framebuffer unpinning

[Gabriel Krisman Bertazi]

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,

-- 
Gabriel Krisman Bertazi
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] qxl: fix framebuffer unpinning

[Gerd Hoffmann]

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
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel