[PATCH] drm/core: Do not preserve framebuffer on rmfb.

[PATCH] drm/core: Do not preserve framebuffer on rmfb.

[Maarten Lankhorst]

It turns out that preserving framebuffers after the rmfb call breaks
vmwgfx userspace. This was originally introduced because it was thought
nobody relied on the behavior, but unfortunately it seems there are
exceptions.

drm_framebuffer_remove may fail with -EINTR now, so a straight revert
is impossible. There is no way to remove the framebuffer from the lists
and active planes without introducing a race because of the different
locking requirements. Instead call drm_framebuffer_remove from a
workqueue, which is unaffected by signals.

Cc: stable@vger.kernel.org #v4.4+
Fixes: 13803132818c ("drm/core: Preserve the framebuffer after removing it.")
Testcase: kms_flip.flip-vs-rmfb-interruptible
References: https://lists.freedesktop.org/archives/dri-devel/2016-March/102876.html
Cc: Thomas Hellstrom <thellstrom@vmware.com>
Cc: David Herrmann <dh.herrmann@gmail.com>
---
 drivers/gpu/drm/drm_crtc.c | 20 +++++++++++++++++++-
 1 file changed, 19 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
index e08f962288d9..b7d0b959f088 100644
--- a/drivers/gpu/drm/drm_crtc.c
+++ b/drivers/gpu/drm/drm_crtc.c
@@ -3434,6 +3434,18 @@ int drm_mode_addfb2(struct drm_device *dev,
 	return 0;
 }
 
+struct drm_mode_rmfb_work {
+	struct work_struct work;
+	struct drm_framebuffer *fb;
+};
+
+static void drm_mode_rmfb_work_fn(struct work_struct *w)
+{
+	struct drm_mode_rmfb_work *arg = container_of(w, typeof(*arg), work);
+
+	drm_framebuffer_remove(arg->fb);
+}
+
 /**
  * drm_mode_rmfb - remove an FB from the configuration
  * @dev: drm device for the ioctl
@@ -3454,6 +3466,7 @@ int drm_mode_rmfb(struct drm_device *dev,
 	struct drm_framebuffer *fbl = NULL;
 	uint32_t *id = data;
 	int found = 0;
+	struct drm_mode_rmfb_work arg;
 
 	if (!drm_core_check_feature(dev, DRIVER_MODESET))
 		return -EINVAL;
@@ -3474,7 +3487,12 @@ int drm_mode_rmfb(struct drm_device *dev,
 	mutex_unlock(&dev->mode_config.fb_lock);
 	mutex_unlock(&file_priv->fbs_lock);
 
-	drm_framebuffer_unreference(fb);
+	INIT_WORK_ONSTACK(&arg.work, drm_mode_rmfb_work_fn);
+	arg.fb = fb;
+
+	schedule_work(&arg.work);
+	flush_work(&arg.work);
+	destroy_work_on_stack(&arg.work);
 
 	return 0;
 
-- 
2.1.0

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

✗ Fi.CI.BAT: warning for drm/core: Do not preserve framebuffer on rmfb.

[Patchwork]

== Series Details ==

Series: drm/core: Do not preserve framebuffer on rmfb.
URL   : https://patchwork.freedesktop.org/series/4707/
State : warning

== Summary ==

Series 4707v1 drm/core: Do not preserve framebuffer on rmfb.
http://patchwork.freedesktop.org/api/1.0/series/4707/revisions/1/mbox/

Test kms_flip:
        Subgroup basic-flip-vs-wf_vblank:
                fail       -> PASS       (bdw-ultra)
        Subgroup basic-plain-flip:
                dmesg-warn -> PASS       (hsw-gt2)
                dmesg-warn -> PASS       (hsw-brixbox)
Test kms_force_connector_basic:
        Subgroup force-load-detect:
                pass       -> DMESG-WARN (snb-x220t)
Test kms_frontbuffer_tracking:
        Subgroup basic:
                pass       -> DMESG-WARN (snb-x220t)
Test pm_rpm:
        Subgroup basic-pci-d3-state:
                dmesg-warn -> PASS       (hsw-brixbox)
        Subgroup basic-rte:
                dmesg-warn -> PASS       (snb-x220t)
                pass       -> DMESG-WARN (bsw-nuc-2)

bdw-nuci7        total:194  pass:182  dwarn:0   dfail:0   fail:0   skip:12 
bdw-ultra        total:194  pass:173  dwarn:0   dfail:0   fail:0   skip:21 
bsw-nuc-2        total:194  pass:156  dwarn:1   dfail:0   fail:0   skip:37 
byt-nuc          total:194  pass:159  dwarn:0   dfail:0   fail:0   skip:35 
hsw-brixbox      total:194  pass:172  dwarn:0   dfail:0   fail:0   skip:22 
hsw-gt2          total:194  pass:177  dwarn:0   dfail:0   fail:0   skip:17 
ilk-hp8440p      total:194  pass:131  dwarn:0   dfail:0   fail:0   skip:63 
ivb-t430s        total:194  pass:169  dwarn:0   dfail:0   fail:0   skip:25 
skl-i5k-2        total:194  pass:171  dwarn:0   dfail:0   fail:0   skip:23 
skl-i7k-2        total:194  pass:171  dwarn:0   dfail:0   fail:0   skip:23 
skl-nuci5        total:194  pass:183  dwarn:0   dfail:0   fail:0   skip:11 
snb-dellxps      total:194  pass:159  dwarn:1   dfail:0   fail:0   skip:34 
snb-x220t        total:194  pass:158  dwarn:2   dfail:0   fail:1   skip:33 

Results at /archive/results/CI_IGT_test/Patchwork_1662/

3b8b9e2deb9f23e1a841d0f9d80296a9759ff8f8 drm-intel-nightly: 2016y-03m-21d-13h-03m-53s UTC integration manifest
ef6e18ed713cbdc31969373ac0ebcd85f2a8a803 drm/core: Do not preserve framebuffer on rmfb.

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/core: Do not preserve framebuffer on rmfb.

[Daniel Vetter]

On Mon, Mar 21, 2016 at 03:11:17PM +0100, Maarten Lankhorst wrote:
> It turns out that preserving framebuffers after the rmfb call breaks
> vmwgfx userspace. This was originally introduced because it was thought
> nobody relied on the behavior, but unfortunately it seems there are
> exceptions.
>
> drm_framebuffer_remove may fail with -EINTR now, so a straight revert
> is impossible. There is no way to remove the framebuffer from the lists
> and active planes without introducing a race because of the different
> locking requirements. Instead call drm_framebuffer_remove from a
> workqueue, which is unaffected by signals.
>
> Cc: stable@vger.kernel.org #v4.4+
> Fixes: 13803132818c ("drm/core: Preserve the framebuffer after removing it.")
> Testcase: kms_flip.flip-vs-rmfb-interruptible
> References: https://lists.freedesktop.org/archives/dri-devel/2016-March/102876.html
> Cc: Thomas Hellstrom <thellstrom@vmware.com>
> Cc: David Herrmann <dh.herrmann@gmail.com>
> ---
>  drivers/gpu/drm/drm_crtc.c | 20 +++++++++++++++++++-
>  1 file changed, 19 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
> index e08f962288d9..b7d0b959f088 100644
> --- a/drivers/gpu/drm/drm_crtc.c
> +++ b/drivers/gpu/drm/drm_crtc.c
> @@ -3434,6 +3434,18 @@ int drm_mode_addfb2(struct drm_device *dev,
>   return 0;
>  }
>
> +struct drm_mode_rmfb_work {
> + struct work_struct work;
> + struct drm_framebuffer *fb;
> +};
> +
> +static void drm_mode_rmfb_work_fn(struct work_struct *w)
> +{
> + struct drm_mode_rmfb_work *arg = container_of(w, typeof(*arg), work);
> +
> + drm_framebuffer_remove(arg->fb);

drm_framebuffer_remove still has the problem of not working correctly with
atomic since atomic commit will complain if we try to do more than 1
commit per ww_acquire_ctx. I think we still need an atomic version of
this. Also probably a more nasty igt testcase which uses the same fb on
more than one plane to be able to hit this case.

> +}
> +
>  /**
>   * drm_mode_rmfb - remove an FB from the configuration
>   * @dev: drm device for the ioctl
> @@ -3454,6 +3466,7 @@ int drm_mode_rmfb(struct drm_device *dev,
>   struct drm_framebuffer *fbl = NULL;
>   uint32_t *id = data;
>   int found = 0;
> + struct drm_mode_rmfb_work arg;
>
>   if (!drm_core_check_feature(dev, DRIVER_MODESET))
>   return -EINVAL;
> @@ -3474,7 +3487,12 @@ int drm_mode_rmfb(struct drm_device *dev,
>   mutex_unlock(&dev->mode_config.fb_lock);
>   mutex_unlock(&file_priv->fbs_lock);
>
> - drm_framebuffer_unreference(fb);

Needs a comment here to explain that we evade EINTR/signals, and that it's
not a trick to hide a locking inversion from lockdep.

Otherwise I think this patch here is the best fix of all the approaches
discussed on irc, under the constraint that we need some obviously
save&small for cc: stable.
-Daniel


> + INIT_WORK_ONSTACK(&arg.work, drm_mode_rmfb_work_fn);
> + arg.fb = fb;
> +
> + schedule_work(&arg.work);
> + flush_work(&arg.work);
> + destroy_work_on_stack(&arg.work);
>
>   return 0;
>
> --
> 2.1.0
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [PATCH] drm/core: Do not preserve framebuffer on rmfb.

[Maarten Lankhorst]

Op 21-03-16 om 18:37 schreef Daniel Vetter:
> On Mon, Mar 21, 2016 at 03:11:17PM +0100, Maarten Lankhorst wrote:
>> It turns out that preserving framebuffers after the rmfb call breaks
>> vmwgfx userspace. This was originally introduced because it was thought
>> nobody relied on the behavior, but unfortunately it seems there are
>> exceptions.
>>
>> drm_framebuffer_remove may fail with -EINTR now, so a straight revert
>> is impossible. There is no way to remove the framebuffer from the lists
>> and active planes without introducing a race because of the different
>> locking requirements. Instead call drm_framebuffer_remove from a
>> workqueue, which is unaffected by signals.
>>
>> Cc: stable@vger.kernel.org #v4.4+
>> Fixes: 13803132818c ("drm/core: Preserve the framebuffer after removing it.")
>> Testcase: kms_flip.flip-vs-rmfb-interruptible
>> References: https://lists.freedesktop.org/archives/dri-devel/2016-March/102876.html
>> Cc: Thomas Hellstrom <thellstrom@vmware.com>
>> Cc: David Herrmann <dh.herrmann@gmail.com>
>> ---
>>  drivers/gpu/drm/drm_crtc.c | 20 +++++++++++++++++++-
>>  1 file changed, 19 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
>> index e08f962288d9..b7d0b959f088 100644
>> --- a/drivers/gpu/drm/drm_crtc.c
>> +++ b/drivers/gpu/drm/drm_crtc.c
>> @@ -3434,6 +3434,18 @@ int drm_mode_addfb2(struct drm_device *dev,
>>   return 0;
>>  }
>>
>> +struct drm_mode_rmfb_work {
>> + struct work_struct work;
>> + struct drm_framebuffer *fb;
>> +};
>> +
>> +static void drm_mode_rmfb_work_fn(struct work_struct *w)
>> +{
>> + struct drm_mode_rmfb_work *arg = container_of(w, typeof(*arg), work);
>> +
>> + drm_framebuffer_remove(arg->fb);
> drm_framebuffer_remove still has the problem of not working correctly with
> atomic since atomic commit will complain if we try to do more than 1
> commit per ww_acquire_ctx. I think we still need an atomic version of
> this. Also probably a more nasty igt testcase which uses the same fb on
> more than one plane to be able to hit this case.
That's true, but a separate bug. :)
>> +}
>> +
>>  /**
>>   * drm_mode_rmfb - remove an FB from the configuration
>>   * @dev: drm device for the ioctl
>> @@ -3454,6 +3466,7 @@ int drm_mode_rmfb(struct drm_device *dev,
>>   struct drm_framebuffer *fbl = NULL;
>>   uint32_t *id = data;
>>   int found = 0;
>> + struct drm_mode_rmfb_work arg;
>>
>>   if (!drm_core_check_feature(dev, DRIVER_MODESET))
>>   return -EINVAL;
>> @@ -3474,7 +3487,12 @@ int drm_mode_rmfb(struct drm_device *dev,
>>   mutex_unlock(&dev->mode_config.fb_lock);
>>   mutex_unlock(&file_priv->fbs_lock);
>>
>> - drm_framebuffer_unreference(fb);
> Needs a comment here to explain that we evade EINTR/signals, and that it's
> not a trick to hide a locking inversion from lockdep.
>
> Otherwise I think this patch here is the best fix of all the approaches
> discussed on irc, under the constraint that we need some obviously
> save&small for cc: stable.
>
Indeed, will add a comment.

Re: [PATCH] drm/core: Do not preserve framebuffer on rmfb.

[Maarten Lankhorst]

Op 21-03-16 om 18:37 schreef Daniel Vetter:
> On Mon, Mar 21, 2016 at 03:11:17PM +0100, Maarten Lankhorst wrote:
>> It turns out that preserving framebuffers after the rmfb call breaks
>> vmwgfx userspace. This was originally introduced because it was thought
>> nobody relied on the behavior, but unfortunately it seems there are
>> exceptions.
>>
>> drm_framebuffer_remove may fail with -EINTR now, so a straight revert
>> is impossible. There is no way to remove the framebuffer from the lists
>> and active planes without introducing a race because of the different
>> locking requirements. Instead call drm_framebuffer_remove from a
>> workqueue, which is unaffected by signals.
>>
>> Cc: stable@vger.kernel.org #v4.4+
>> Fixes: 13803132818c ("drm/core: Preserve the framebuffer after removing it.")
>> Testcase: kms_flip.flip-vs-rmfb-interruptible
>> References: https://lists.freedesktop.org/archives/dri-devel/2016-March/102876.html
>> Cc: Thomas Hellstrom <thellstrom@vmware.com>
>> Cc: David Herrmann <dh.herrmann@gmail.com>
>> ---
>>  drivers/gpu/drm/drm_crtc.c | 20 +++++++++++++++++++-
>>  1 file changed, 19 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
>> index e08f962288d9..b7d0b959f088 100644
>> --- a/drivers/gpu/drm/drm_crtc.c
>> +++ b/drivers/gpu/drm/drm_crtc.c
>> @@ -3434,6 +3434,18 @@ int drm_mode_addfb2(struct drm_device *dev,
>>   return 0;
>>  }
>>
>> +struct drm_mode_rmfb_work {
>> + struct work_struct work;
>> + struct drm_framebuffer *fb;
>> +};
>> +
>> +static void drm_mode_rmfb_work_fn(struct work_struct *w)
>> +{
>> + struct drm_mode_rmfb_work *arg = container_of(w, typeof(*arg), work);
>> +
>> + drm_framebuffer_remove(arg->fb);
> drm_framebuffer_remove still has the problem of not working correctly with
> atomic since atomic commit will complain if we try to do more than 1
> commit per ww_acquire_ctx. I think we still need an atomic version of
> this. Also probably a more nasty igt testcase which uses the same fb on
> more than one plane to be able to hit this case.
That's true, but a separate bug. :)
>> +}
>> +
>>  /**
>>   * drm_mode_rmfb - remove an FB from the configuration
>>   * @dev: drm device for the ioctl
>> @@ -3454,6 +3466,7 @@ int drm_mode_rmfb(struct drm_device *dev,
>>   struct drm_framebuffer *fbl = NULL;
>>   uint32_t *id = data;
>>   int found = 0;
>> + struct drm_mode_rmfb_work arg;
>>
>>   if (!drm_core_check_feature(dev, DRIVER_MODESET))
>>   return -EINVAL;
>> @@ -3474,7 +3487,12 @@ int drm_mode_rmfb(struct drm_device *dev,
>>   mutex_unlock(&dev->mode_config.fb_lock);
>>   mutex_unlock(&file_priv->fbs_lock);
>>
>> - drm_framebuffer_unreference(fb);
> Needs a comment here to explain that we evade EINTR/signals, and that it's
> not a trick to hide a locking inversion from lockdep.
>
> Otherwise I think this patch here is the best fix of all the approaches
> discussed on irc, under the constraint that we need some obviously
> save&small for cc: stable.
>
Indeed, will add a comment.
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/core: Do not preserve framebuffer on rmfb.

[Daniel Vetter]

On Tue, Mar 22, 2016 at 10:32:32AM +0100, Maarten Lankhorst wrote:
> Op 21-03-16 om 18:37 schreef Daniel Vetter:
> > On Mon, Mar 21, 2016 at 03:11:17PM +0100, Maarten Lankhorst wrote:
> >> It turns out that preserving framebuffers after the rmfb call breaks
> >> vmwgfx userspace. This was originally introduced because it was thought
> >> nobody relied on the behavior, but unfortunately it seems there are
> >> exceptions.
> >>
> >> drm_framebuffer_remove may fail with -EINTR now, so a straight revert
> >> is impossible. There is no way to remove the framebuffer from the lists
> >> and active planes without introducing a race because of the different
> >> locking requirements. Instead call drm_framebuffer_remove from a
> >> workqueue, which is unaffected by signals.
> >>
> >> Cc: stable@vger.kernel.org #v4.4+
> >> Fixes: 13803132818c ("drm/core: Preserve the framebuffer after removing it.")
> >> Testcase: kms_flip.flip-vs-rmfb-interruptible
> >> References: https://lists.freedesktop.org/archives/dri-devel/2016-March/102876.html
> >> Cc: Thomas Hellstrom <thellstrom@vmware.com>
> >> Cc: David Herrmann <dh.herrmann@gmail.com>
> >> ---
> >>  drivers/gpu/drm/drm_crtc.c | 20 +++++++++++++++++++-
> >>  1 file changed, 19 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
> >> index e08f962288d9..b7d0b959f088 100644
> >> --- a/drivers/gpu/drm/drm_crtc.c
> >> +++ b/drivers/gpu/drm/drm_crtc.c
> >> @@ -3434,6 +3434,18 @@ int drm_mode_addfb2(struct drm_device *dev,
> >>   return 0;
> >>  }
> >>
> >> +struct drm_mode_rmfb_work {
> >> + struct work_struct work;
> >> + struct drm_framebuffer *fb;
> >> +};
> >> +
> >> +static void drm_mode_rmfb_work_fn(struct work_struct *w)
> >> +{
> >> + struct drm_mode_rmfb_work *arg = container_of(w, typeof(*arg), work);
> >> +
> >> + drm_framebuffer_remove(arg->fb);
> > drm_framebuffer_remove still has the problem of not working correctly with
> > atomic since atomic commit will complain if we try to do more than 1
> > commit per ww_acquire_ctx. I think we still need an atomic version of
> > this. Also probably a more nasty igt testcase which uses the same fb on
> > more than one plane to be able to hit this case.
> That's true, but a separate bug. :)

Atm we only use drm_framebuffer_remove in atomic drivers to nuke the fbdev
one at unload. With your patch userspace can't get there easily, and hence
it must be fixed. Maybe separate prep patch (also cc: stable) upfront?
-Daniel

> >> +}
> >> +
> >>  /**
> >>   * drm_mode_rmfb - remove an FB from the configuration
> >>   * @dev: drm device for the ioctl
> >> @@ -3454,6 +3466,7 @@ int drm_mode_rmfb(struct drm_device *dev,
> >>   struct drm_framebuffer *fbl = NULL;
> >>   uint32_t *id = data;
> >>   int found = 0;
> >> + struct drm_mode_rmfb_work arg;
> >>
> >>   if (!drm_core_check_feature(dev, DRIVER_MODESET))
> >>   return -EINVAL;
> >> @@ -3474,7 +3487,12 @@ int drm_mode_rmfb(struct drm_device *dev,
> >>   mutex_unlock(&dev->mode_config.fb_lock);
> >>   mutex_unlock(&file_priv->fbs_lock);
> >>
> >> - drm_framebuffer_unreference(fb);
> > Needs a comment here to explain that we evade EINTR/signals, and that it's
> > not a trick to hide a locking inversion from lockdep.
> >
> > Otherwise I think this patch here is the best fix of all the approaches
> > discussed on irc, under the constraint that we need some obviously
> > save&small for cc: stable.
> >
> Indeed, will add a comment.

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [PATCH] drm/core: Do not preserve framebuffer on rmfb.

[Daniel Vetter]

On Tue, Mar 22, 2016 at 10:32:32AM +0100, Maarten Lankhorst wrote:
> Op 21-03-16 om 18:37 schreef Daniel Vetter:
> > On Mon, Mar 21, 2016 at 03:11:17PM +0100, Maarten Lankhorst wrote:
> >> It turns out that preserving framebuffers after the rmfb call breaks
> >> vmwgfx userspace. This was originally introduced because it was thought
> >> nobody relied on the behavior, but unfortunately it seems there are
> >> exceptions.
> >>
> >> drm_framebuffer_remove may fail with -EINTR now, so a straight revert
> >> is impossible. There is no way to remove the framebuffer from the lists
> >> and active planes without introducing a race because of the different
> >> locking requirements. Instead call drm_framebuffer_remove from a
> >> workqueue, which is unaffected by signals.
> >>
> >> Cc: stable@vger.kernel.org #v4.4+
> >> Fixes: 13803132818c ("drm/core: Preserve the framebuffer after removing it.")
> >> Testcase: kms_flip.flip-vs-rmfb-interruptible
> >> References: https://lists.freedesktop.org/archives/dri-devel/2016-March/102876.html
> >> Cc: Thomas Hellstrom <thellstrom@vmware.com>
> >> Cc: David Herrmann <dh.herrmann@gmail.com>
> >> ---
> >>  drivers/gpu/drm/drm_crtc.c | 20 +++++++++++++++++++-
> >>  1 file changed, 19 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
> >> index e08f962288d9..b7d0b959f088 100644
> >> --- a/drivers/gpu/drm/drm_crtc.c
> >> +++ b/drivers/gpu/drm/drm_crtc.c
> >> @@ -3434,6 +3434,18 @@ int drm_mode_addfb2(struct drm_device *dev,
> >>   return 0;
> >>  }
> >>
> >> +struct drm_mode_rmfb_work {
> >> + struct work_struct work;
> >> + struct drm_framebuffer *fb;
> >> +};
> >> +
> >> +static void drm_mode_rmfb_work_fn(struct work_struct *w)
> >> +{
> >> + struct drm_mode_rmfb_work *arg = container_of(w, typeof(*arg), work);
> >> +
> >> + drm_framebuffer_remove(arg->fb);
> > drm_framebuffer_remove still has the problem of not working correctly with
> > atomic since atomic commit will complain if we try to do more than 1
> > commit per ww_acquire_ctx. I think we still need an atomic version of
> > this. Also probably a more nasty igt testcase which uses the same fb on
> > more than one plane to be able to hit this case.
> That's true, but a separate bug. :)

Atm we only use drm_framebuffer_remove in atomic drivers to nuke the fbdev
one at unload. With your patch userspace can't get there easily, and hence
it must be fixed. Maybe separate prep patch (also cc: stable) upfront?
-Daniel

> >> +}
> >> +
> >>  /**
> >>   * drm_mode_rmfb - remove an FB from the configuration
> >>   * @dev: drm device for the ioctl
> >> @@ -3454,6 +3466,7 @@ int drm_mode_rmfb(struct drm_device *dev,
> >>   struct drm_framebuffer *fbl = NULL;
> >>   uint32_t *id = data;
> >>   int found = 0;
> >> + struct drm_mode_rmfb_work arg;
> >>
> >>   if (!drm_core_check_feature(dev, DRIVER_MODESET))
> >>   return -EINVAL;
> >> @@ -3474,7 +3487,12 @@ int drm_mode_rmfb(struct drm_device *dev,
> >>   mutex_unlock(&dev->mode_config.fb_lock);
> >>   mutex_unlock(&file_priv->fbs_lock);
> >>
> >> - drm_framebuffer_unreference(fb);
> > Needs a comment here to explain that we evade EINTR/signals, and that it's
> > not a trick to hide a locking inversion from lockdep.
> >
> > Otherwise I think this patch here is the best fix of all the approaches
> > discussed on irc, under the constraint that we need some obviously
> > save&small for cc: stable.
> >
> Indeed, will add a comment.

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/core: Do not preserve framebuffer on rmfb.

[Maarten Lankhorst]

Op 22-03-16 om 11:50 schreef Daniel Vetter:
> On Tue, Mar 22, 2016 at 10:32:32AM +0100, Maarten Lankhorst wrote:
>> Op 21-03-16 om 18:37 schreef Daniel Vetter:
>>> On Mon, Mar 21, 2016 at 03:11:17PM +0100, Maarten Lankhorst wrote:
>>>> It turns out that preserving framebuffers after the rmfb call breaks
>>>> vmwgfx userspace. This was originally introduced because it was thought
>>>> nobody relied on the behavior, but unfortunately it seems there are
>>>> exceptions.
>>>>
>>>> drm_framebuffer_remove may fail with -EINTR now, so a straight revert
>>>> is impossible. There is no way to remove the framebuffer from the lists
>>>> and active planes without introducing a race because of the different
>>>> locking requirements. Instead call drm_framebuffer_remove from a
>>>> workqueue, which is unaffected by signals.
>>>>
>>>> Cc: stable@vger.kernel.org #v4.4+
>>>> Fixes: 13803132818c ("drm/core: Preserve the framebuffer after removing it.")
>>>> Testcase: kms_flip.flip-vs-rmfb-interruptible
>>>> References: https://lists.freedesktop.org/archives/dri-devel/2016-March/102876.html
>>>> Cc: Thomas Hellstrom <thellstrom@vmware.com>
>>>> Cc: David Herrmann <dh.herrmann@gmail.com>
>>>> ---
>>>>  drivers/gpu/drm/drm_crtc.c | 20 +++++++++++++++++++-
>>>>  1 file changed, 19 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
>>>> index e08f962288d9..b7d0b959f088 100644
>>>> --- a/drivers/gpu/drm/drm_crtc.c
>>>> +++ b/drivers/gpu/drm/drm_crtc.c
>>>> @@ -3434,6 +3434,18 @@ int drm_mode_addfb2(struct drm_device *dev,
>>>>   return 0;
>>>>  }
>>>>
>>>> +struct drm_mode_rmfb_work {
>>>> + struct work_struct work;
>>>> + struct drm_framebuffer *fb;
>>>> +};
>>>> +
>>>> +static void drm_mode_rmfb_work_fn(struct work_struct *w)
>>>> +{
>>>> + struct drm_mode_rmfb_work *arg = container_of(w, typeof(*arg), work);
>>>> +
>>>> + drm_framebuffer_remove(arg->fb);
>>> drm_framebuffer_remove still has the problem of not working correctly with
>>> atomic since atomic commit will complain if we try to do more than 1
>>> commit per ww_acquire_ctx. I think we still need an atomic version of
>>> this. Also probably a more nasty igt testcase which uses the same fb on
>>> more than one plane to be able to hit this case.
>> That's true, but a separate bug. :)
> Atm we only use drm_framebuffer_remove in atomic drivers to nuke the fbdev
> one at unload. With your patch userspace can't get there easily, and hence
> it must be fixed. Maybe separate prep patch (also cc: stable) upfront?
>
Something like this?

Unfortunately I need to collect acks first.

commit ed242f92c2e7571a6a5f649c2a67031debc73e44
Author: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Date:   Thu Mar 17 13:42:08 2016 +0100

    drm/atomic: Add remove_fb function pointer.
    
    Use this in drm_framebuffer_remove, this is to remove the fb in an atomic way.
    
    Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>

diff --git a/drivers/gpu/drm/arm/hdlcd_drv.c b/drivers/gpu/drm/arm/hdlcd_drv.c
index 56b829f97699..50ba6adb74e8 100644
--- a/drivers/gpu/drm/arm/hdlcd_drv.c
+++ b/drivers/gpu/drm/arm/hdlcd_drv.c
@@ -324,6 +324,7 @@ static struct drm_driver hdlcd_driver = {
 	.dumb_create = drm_gem_cma_dumb_create,
 	.dumb_map_offset = drm_gem_cma_dumb_map_offset,
 	.dumb_destroy = drm_gem_dumb_destroy,
+	.remove_fb = drm_atomic_helper_remove_fb,
 	.prime_handle_to_fd = drm_gem_prime_handle_to_fd,
 	.prime_fd_to_handle = drm_gem_prime_fd_to_handle,
 	.gem_prime_export = drm_gem_prime_export,
diff --git a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c
index 3d8d16402d07..5d357f729114 100644
--- a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c
+++ b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c
@@ -711,6 +711,7 @@ static struct drm_driver atmel_hlcdc_dc_driver = {
 	.dumb_create = drm_gem_cma_dumb_create,
 	.dumb_map_offset = drm_gem_cma_dumb_map_offset,
 	.dumb_destroy = drm_gem_dumb_destroy,
+	.remove_fb = drm_atomic_helper_remove_fb,
 	.fops = &fops,
 	.name = "atmel-hlcdc",
 	.desc = "Atmel HLCD Controller DRM",
diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
index 4befe25c81c7..eb3b413560df 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -1960,6 +1960,72 @@ commit:
 	return 0;
 }
 
+int drm_atomic_helper_remove_fb(struct drm_framebuffer *fb)
+{
+	struct drm_modeset_acquire_ctx ctx;
+	struct drm_device *dev = fb->dev;
+	struct drm_atomic_state *state;
+	struct drm_plane *plane;
+	int ret = 0;
+	unsigned plane_mask;
+
+	state = drm_atomic_state_alloc(dev);
+	if (!state)
+		return -ENOMEM;
+
+	drm_modeset_acquire_init(&ctx, 0);
+
+retry:
+	plane_mask = 0;
+	ret = drm_modeset_lock_all_ctx(dev, &ctx);
+	if (ret)
+		goto unlock;
+
+	drm_for_each_plane(plane, dev) {
+		struct drm_plane_state *plane_state;
+
+		if (plane->state->fb != fb)
+			continue;
+
+		plane_state = drm_atomic_get_plane_state(state, plane);
+		if (IS_ERR(plane_state)) {
+			ret = PTR_ERR(plane_state);
+			goto unlock;
+		}
+
+		drm_atomic_set_fb_for_plane(plane_state, NULL);
+		ret = drm_atomic_set_crtc_for_plane(plane_state, NULL);
+		if (ret)
+			goto unlock;
+
+		plane_mask |= BIT(drm_plane_index(plane));
+
+		plane->old_fb = plane->fb;
+		plane->fb = NULL;
+	}
+
+	if (plane_mask)
+		ret = drm_atomic_commit(state);
+
+unlock:
+	if (plane_mask)
+		drm_atomic_clean_old_fb(dev, plane_mask, ret);
+
+	if (ret == -EDEADLK) {
+		drm_modeset_backoff(&ctx);
+		goto retry;
+	}
+
+	if (ret || !plane_mask)
+		drm_atomic_state_free(state);
+
+	drm_modeset_drop_locks(&ctx);
+	drm_modeset_acquire_fini(&ctx);
+
+	return ret;
+}
+EXPORT_SYMBOL(drm_atomic_helper_remove_fb);
+
 /**
  * drm_atomic_helper_disable_all - disable all currently active outputs
  * @dev: DRM device
diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
index aaf6ab42f2c1..51c5a00ffdff 100644
--- a/drivers/gpu/drm/drm_crtc.c
+++ b/drivers/gpu/drm/drm_crtc.c
@@ -577,6 +577,42 @@ void drm_framebuffer_cleanup(struct drm_framebuffer *fb)
 }
 EXPORT_SYMBOL(drm_framebuffer_cleanup);
 
+static int
+legacy_remove_fb(struct drm_framebuffer *fb)
+{
+	struct drm_device *dev = fb->dev;
+	struct drm_crtc *crtc;
+	struct drm_plane *plane;
+	struct drm_mode_set set;
+	int ret = 0;
+
+	/* atomic drivers must use drm_atomic_helper_remove_fb */
+	WARN_ON(drm_core_check_feature(dev, DRIVER_ATOMIC));
+
+	drm_modeset_lock_all(dev);
+	/* remove from any CRTC */
+	drm_for_each_crtc(crtc, dev) {
+		if (crtc->primary->fb == fb) {
+			/* should turn off the crtc */
+			memset(&set, 0, sizeof(struct drm_mode_set));
+			set.crtc = crtc;
+			set.fb = NULL;
+			ret = drm_mode_set_config_internal(&set);
+			if (ret)
+				goto out;
+		}
+	}
+
+	drm_for_each_plane(plane, dev) {
+		if (plane->fb == fb)
+			drm_plane_force_disable(plane);
+	}
+
+out:
+	drm_modeset_unlock_all(dev);
+	return ret;
+}
+
 /**
  * drm_framebuffer_remove - remove and unreference a framebuffer object
  * @fb: framebuffer to remove
@@ -592,9 +628,6 @@ EXPORT_SYMBOL(drm_framebuffer_cleanup);
 void drm_framebuffer_remove(struct drm_framebuffer *fb)
 {
 	struct drm_device *dev;
-	struct drm_crtc *crtc;
-	struct drm_plane *plane;
-	struct drm_mode_set set;
 	int ret;
 
 	if (!fb)
@@ -620,25 +653,13 @@ void drm_framebuffer_remove(struct drm_framebuffer *fb)
 	 * in this manner.
 	 */
 	if (atomic_read(&fb->refcount.refcount) > 1) {
-		drm_modeset_lock_all(dev);
-		/* remove from any CRTC */
-		drm_for_each_crtc(crtc, dev) {
-			if (crtc->primary->fb == fb) {
-				/* should turn off the crtc */
-				memset(&set, 0, sizeof(struct drm_mode_set));
-				set.crtc = crtc;
-				set.fb = NULL;
-				ret = drm_mode_set_config_internal(&set);
-				if (ret)
-					DRM_ERROR("failed to reset crtc %p when fb was deleted\n", crtc);
-			}
-		}
+		if (dev->driver->remove_fb)
+			ret = dev->driver->remove_fb(fb);
+		else
+			ret = legacy_remove_fb(fb);
 
-		drm_for_each_plane(plane, dev) {
-			if (plane->fb == fb)
-				drm_plane_force_disable(plane);
-		}
-		drm_modeset_unlock_all(dev);
+		if (ret)
+			DRM_ERROR("failed to remove fb %i/%p with %i\n", fb->base.id, fb, ret);
 	}
 
 	drm_framebuffer_unreference(fb);
diff --git a/drivers/gpu/drm/exynos/exynos_drm_drv.c b/drivers/gpu/drm/exynos/exynos_drm_drv.c
index 5344940c8a07..2705a315f1ec 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_drv.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_drv.c
@@ -423,6 +423,7 @@ static struct drm_driver exynos_drm_driver = {
 	.dumb_create		= exynos_drm_gem_dumb_create,
 	.dumb_map_offset	= exynos_drm_gem_dumb_map_offset,
 	.dumb_destroy		= drm_gem_dumb_destroy,
+	.remove_fb		= drm_atomic_helper_remove_fb,
 	.prime_handle_to_fd	= drm_gem_prime_handle_to_fd,
 	.prime_fd_to_handle	= drm_gem_prime_fd_to_handle,
 	.gem_prime_export	= drm_gem_prime_export,
diff --git a/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.c b/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.c
index e8d9337a66d8..f7562b178819 100644
--- a/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.c
+++ b/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.c
@@ -24,6 +24,7 @@
 #include <drm/drmP.h>
 #include <drm/drm_crtc_helper.h>
 #include <drm/drm_gem_cma_helper.h>
+#include <drm/drm_atomic_helper.h>
 
 #include "fsl_dcu_drm_crtc.h"
 #include "fsl_dcu_drm_drv.h"
@@ -194,6 +195,7 @@ static struct drm_driver fsl_dcu_drm_driver = {
 	.dumb_create		= drm_gem_cma_dumb_create,
 	.dumb_map_offset	= drm_gem_cma_dumb_map_offset,
 	.dumb_destroy		= drm_gem_dumb_destroy,
+	.remove_fb		= drm_atomic_helper_remove_fb,
 	.fops			= &fsl_dcu_drm_fops,
 	.name			= "fsl-dcu-drm",
 	.desc			= "Freescale DCU DRM",
diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 2a076b005af9..9cbf88adf280 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -42,6 +42,7 @@
 #include <linux/vgaarb.h>
 #include <linux/vga_switcheroo.h>
 #include <drm/drm_crtc_helper.h>
+#include <drm/drm_atomic_helper.h>
 
 static struct drm_driver driver;
 
@@ -1712,6 +1713,7 @@ static struct drm_driver driver = {
 	.dumb_create = i915_gem_dumb_create,
 	.dumb_map_offset = i915_gem_mmap_gtt,
 	.dumb_destroy = drm_gem_dumb_destroy,
+	.remove_fb = drm_atomic_helper_remove_fb,
 	.ioctls = i915_ioctls,
 	.fops = &i915_driver_fops,
 	.name = DRIVER_NAME,
diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c
index d52910e2c26c..fab3d7f036ae 100644
--- a/drivers/gpu/drm/msm/msm_drv.c
+++ b/drivers/gpu/drm/msm/msm_drv.c
@@ -973,6 +973,7 @@ static struct drm_driver msm_driver = {
 	.dumb_create        = msm_gem_dumb_create,
 	.dumb_map_offset    = msm_gem_dumb_map_offset,
 	.dumb_destroy       = drm_gem_dumb_destroy,
+	.remove_fb          = drm_atomic_helper_remove_fb,
 	.prime_handle_to_fd = drm_gem_prime_handle_to_fd,
 	.prime_fd_to_handle = drm_gem_prime_fd_to_handle,
 	.gem_prime_export   = drm_gem_prime_export,
diff --git a/drivers/gpu/drm/omapdrm/omap_drv.c b/drivers/gpu/drm/omapdrm/omap_drv.c
index 80398a684cae..9f3bacbad118 100644
--- a/drivers/gpu/drm/omapdrm/omap_drv.c
+++ b/drivers/gpu/drm/omapdrm/omap_drv.c
@@ -817,6 +817,7 @@ static struct drm_driver omap_drm_driver = {
 	.dumb_create = omap_gem_dumb_create,
 	.dumb_map_offset = omap_gem_dumb_map_offset,
 	.dumb_destroy = drm_gem_dumb_destroy,
+	.remove_fb = drm_atomic_helper_remove_fb,
 	.ioctls = ioctls,
 	.num_ioctls = DRM_OMAP_NUM_IOCTLS,
 	.fops = &omapdriver_fops,
diff --git a/drivers/gpu/drm/rcar-du/rcar_du_drv.c b/drivers/gpu/drm/rcar-du/rcar_du_drv.c
index ed6006bf6bd8..3b7388d87815 100644
--- a/drivers/gpu/drm/rcar-du/rcar_du_drv.c
+++ b/drivers/gpu/drm/rcar-du/rcar_du_drv.c
@@ -25,6 +25,7 @@
 #include <drm/drm_crtc_helper.h>
 #include <drm/drm_fb_cma_helper.h>
 #include <drm/drm_gem_cma_helper.h>
+#include <drm/drm_atomic_helper.h>
 
 #include "rcar_du_crtc.h"
 #include "rcar_du_drv.h"
@@ -231,6 +232,7 @@ static struct drm_driver rcar_du_driver = {
 	.dumb_create		= rcar_du_dumb_create,
 	.dumb_map_offset	= drm_gem_cma_dumb_map_offset,
 	.dumb_destroy		= drm_gem_dumb_destroy,
+	.remove_fb		= drm_atomic_helper_remove_fb,
 	.fops			= &rcar_du_fops,
 	.name			= "rcar-du",
 	.desc			= "Renesas R-Car Display Unit",
diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_drv.c b/drivers/gpu/drm/rockchip/rockchip_drm_drv.c
index 896da09e49ee..bf2162e4b131 100644
--- a/drivers/gpu/drm/rockchip/rockchip_drm_drv.c
+++ b/drivers/gpu/drm/rockchip/rockchip_drm_drv.c
@@ -19,6 +19,7 @@
 #include <drm/drmP.h>
 #include <drm/drm_crtc_helper.h>
 #include <drm/drm_fb_helper.h>
+#include <drm/drm_atomic_helper.h>
 #include <linux/dma-mapping.h>
 #include <linux/pm_runtime.h>
 #include <linux/module.h>
@@ -290,6 +291,7 @@ static struct drm_driver rockchip_drm_driver = {
 	.dumb_create		= rockchip_gem_dumb_create,
 	.dumb_map_offset	= rockchip_gem_dumb_map_offset,
 	.dumb_destroy		= drm_gem_dumb_destroy,
+	.remove_fb		= drm_atomic_helper_remove_fb,
 	.prime_handle_to_fd	= drm_gem_prime_handle_to_fd,
 	.prime_fd_to_handle	= drm_gem_prime_fd_to_handle,
 	.gem_prime_import	= drm_gem_prime_import,
diff --git a/drivers/gpu/drm/tegra/drm.c b/drivers/gpu/drm/tegra/drm.c
index 8e6b18caa706..c60f86c8f73d 100644
--- a/drivers/gpu/drm/tegra/drm.c
+++ b/drivers/gpu/drm/tegra/drm.c
@@ -944,6 +944,8 @@ static struct drm_driver tegra_drm_driver = {
 	.dumb_map_offset = tegra_bo_dumb_map_offset,
 	.dumb_destroy = drm_gem_dumb_destroy,
 
+	.remove_fb = drm_atomic_helper_remove_fb,
+
 	.ioctls = tegra_drm_ioctls,
 	.num_ioctls = ARRAY_SIZE(tegra_drm_ioctls),
 	.fops = &tegra_drm_fops,
diff --git a/drivers/gpu/drm/vc4/vc4_drv.c b/drivers/gpu/drm/vc4/vc4_drv.c
index b7d2ff0e6e1f..4dde5d924d51 100644
--- a/drivers/gpu/drm/vc4/vc4_drv.c
+++ b/drivers/gpu/drm/vc4/vc4_drv.c
@@ -15,6 +15,7 @@
 #include <linux/of_platform.h>
 #include <linux/platform_device.h>
 #include "drm_fb_cma_helper.h"
+#include "drm/drm_atomic_helper.h"
 
 #include "uapi/drm/vc4_drm.h"
 #include "vc4_drv.h"
@@ -115,6 +116,8 @@ static struct drm_driver vc4_drm_driver = {
 	.dumb_map_offset = drm_gem_cma_dumb_map_offset,
 	.dumb_destroy = drm_gem_dumb_destroy,
 
+	.remove_fb = drm_atomic_helper_remove_fb,
+
 	.ioctls = vc4_drm_ioctls,
 	.num_ioctls = ARRAY_SIZE(vc4_drm_ioctls),
 	.fops = &vc4_drm_fops,
diff --git a/drivers/gpu/drm/virtio/virtgpu_drv.c b/drivers/gpu/drm/virtio/virtgpu_drv.c
index 7f898cfdc746..56912941eaf8 100644
--- a/drivers/gpu/drm/virtio/virtgpu_drv.c
+++ b/drivers/gpu/drm/virtio/virtgpu_drv.c
@@ -31,6 +31,7 @@
 #include <linux/pci.h>
 #include "drmP.h"
 #include "drm/drm.h"
+#include "drm/drm_atomic_helper.h"
 
 #include "virtgpu_drv.h"
 static struct drm_driver driver;
@@ -129,6 +130,8 @@ static struct drm_driver driver = {
 	.dumb_map_offset = virtio_gpu_mode_dumb_mmap,
 	.dumb_destroy = virtio_gpu_mode_dumb_destroy,
 
+	.remove_fb = drm_atomic_helper_remove_fb,
+
 #if defined(CONFIG_DEBUG_FS)
 	.debugfs_init = virtio_gpu_debugfs_init,
 	.debugfs_cleanup = virtio_gpu_debugfs_takedown,
diff --git a/include/drm/drmP.h b/include/drm/drmP.h
index 3c8422c69572..31483c2fef51 100644
--- a/include/drm/drmP.h
+++ b/include/drm/drmP.h
@@ -638,6 +638,9 @@ struct drm_driver {
 			    struct drm_device *dev,
 			    uint32_t handle);
 
+	/* rmfb and drm_framebuffer_remove hook */
+	int (*remove_fb)(struct drm_framebuffer *fb);
+
 	/* Driver private ops for this object */
 	const struct vm_operations_struct *gem_vm_ops;
 
diff --git a/include/drm/drm_atomic_helper.h b/include/drm/drm_atomic_helper.h
index 9054598c9a7a..2d5ff5c80c76 100644
--- a/include/drm/drm_atomic_helper.h
+++ b/include/drm/drm_atomic_helper.h
@@ -87,6 +87,8 @@ int drm_atomic_helper_set_config(struct drm_mode_set *set);
 int __drm_atomic_helper_set_config(struct drm_mode_set *set,
 		struct drm_atomic_state *state);
 
+int drm_atomic_helper_remove_fb(struct drm_framebuffer *fb);
+
 int drm_atomic_helper_disable_all(struct drm_device *dev,
 				  struct drm_modeset_acquire_ctx *ctx);
 struct drm_atomic_state *drm_atomic_helper_suspend(struct drm_device *dev);

Re: [PATCH] drm/core: Do not preserve framebuffer on rmfb.

[Maarten Lankhorst]

Op 22-03-16 om 11:50 schreef Daniel Vetter:
> On Tue, Mar 22, 2016 at 10:32:32AM +0100, Maarten Lankhorst wrote:
>> Op 21-03-16 om 18:37 schreef Daniel Vetter:
>>> On Mon, Mar 21, 2016 at 03:11:17PM +0100, Maarten Lankhorst wrote:
>>>> It turns out that preserving framebuffers after the rmfb call breaks
>>>> vmwgfx userspace. This was originally introduced because it was thought
>>>> nobody relied on the behavior, but unfortunately it seems there are
>>>> exceptions.
>>>>
>>>> drm_framebuffer_remove may fail with -EINTR now, so a straight revert
>>>> is impossible. There is no way to remove the framebuffer from the lists
>>>> and active planes without introducing a race because of the different
>>>> locking requirements. Instead call drm_framebuffer_remove from a
>>>> workqueue, which is unaffected by signals.
>>>>
>>>> Cc: stable@vger.kernel.org #v4.4+
>>>> Fixes: 13803132818c ("drm/core: Preserve the framebuffer after removing it.")
>>>> Testcase: kms_flip.flip-vs-rmfb-interruptible
>>>> References: https://lists.freedesktop.org/archives/dri-devel/2016-March/102876.html
>>>> Cc: Thomas Hellstrom <thellstrom@vmware.com>
>>>> Cc: David Herrmann <dh.herrmann@gmail.com>
>>>> ---
>>>>  drivers/gpu/drm/drm_crtc.c | 20 +++++++++++++++++++-
>>>>  1 file changed, 19 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
>>>> index e08f962288d9..b7d0b959f088 100644
>>>> --- a/drivers/gpu/drm/drm_crtc.c
>>>> +++ b/drivers/gpu/drm/drm_crtc.c
>>>> @@ -3434,6 +3434,18 @@ int drm_mode_addfb2(struct drm_device *dev,
>>>>   return 0;
>>>>  }
>>>>
>>>> +struct drm_mode_rmfb_work {
>>>> + struct work_struct work;
>>>> + struct drm_framebuffer *fb;
>>>> +};
>>>> +
>>>> +static void drm_mode_rmfb_work_fn(struct work_struct *w)
>>>> +{
>>>> + struct drm_mode_rmfb_work *arg = container_of(w, typeof(*arg), work);
>>>> +
>>>> + drm_framebuffer_remove(arg->fb);
>>> drm_framebuffer_remove still has the problem of not working correctly with
>>> atomic since atomic commit will complain if we try to do more than 1
>>> commit per ww_acquire_ctx. I think we still need an atomic version of
>>> this. Also probably a more nasty igt testcase which uses the same fb on
>>> more than one plane to be able to hit this case.
>> That's true, but a separate bug. :)
> Atm we only use drm_framebuffer_remove in atomic drivers to nuke the fbdev
> one at unload. With your patch userspace can't get there easily, and hence
> it must be fixed. Maybe separate prep patch (also cc: stable) upfront?
>
Something like this?

Unfortunately I need to collect acks first.

commit ed242f92c2e7571a6a5f649c2a67031debc73e44
Author: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Date:   Thu Mar 17 13:42:08 2016 +0100

    drm/atomic: Add remove_fb function pointer.
    
    Use this in drm_framebuffer_remove, this is to remove the fb in an atomic way.
    
    Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>

diff --git a/drivers/gpu/drm/arm/hdlcd_drv.c b/drivers/gpu/drm/arm/hdlcd_drv.c
index 56b829f97699..50ba6adb74e8 100644
--- a/drivers/gpu/drm/arm/hdlcd_drv.c
+++ b/drivers/gpu/drm/arm/hdlcd_drv.c
@@ -324,6 +324,7 @@ static struct drm_driver hdlcd_driver = {
 	.dumb_create = drm_gem_cma_dumb_create,
 	.dumb_map_offset = drm_gem_cma_dumb_map_offset,
 	.dumb_destroy = drm_gem_dumb_destroy,
+	.remove_fb = drm_atomic_helper_remove_fb,
 	.prime_handle_to_fd = drm_gem_prime_handle_to_fd,
 	.prime_fd_to_handle = drm_gem_prime_fd_to_handle,
 	.gem_prime_export = drm_gem_prime_export,
diff --git a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c
index 3d8d16402d07..5d357f729114 100644
--- a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c
+++ b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c
@@ -711,6 +711,7 @@ static struct drm_driver atmel_hlcdc_dc_driver = {
 	.dumb_create = drm_gem_cma_dumb_create,
 	.dumb_map_offset = drm_gem_cma_dumb_map_offset,
 	.dumb_destroy = drm_gem_dumb_destroy,
+	.remove_fb = drm_atomic_helper_remove_fb,
 	.fops = &fops,
 	.name = "atmel-hlcdc",
 	.desc = "Atmel HLCD Controller DRM",
diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
index 4befe25c81c7..eb3b413560df 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -1960,6 +1960,72 @@ commit:
 	return 0;
 }
 
+int drm_atomic_helper_remove_fb(struct drm_framebuffer *fb)
+{
+	struct drm_modeset_acquire_ctx ctx;
+	struct drm_device *dev = fb->dev;
+	struct drm_atomic_state *state;
+	struct drm_plane *plane;
+	int ret = 0;
+	unsigned plane_mask;
+
+	state = drm_atomic_state_alloc(dev);
+	if (!state)
+		return -ENOMEM;
+
+	drm_modeset_acquire_init(&ctx, 0);
+
+retry:
+	plane_mask = 0;
+	ret = drm_modeset_lock_all_ctx(dev, &ctx);
+	if (ret)
+		goto unlock;
+
+	drm_for_each_plane(plane, dev) {
+		struct drm_plane_state *plane_state;
+
+		if (plane->state->fb != fb)
+			continue;
+
+		plane_state = drm_atomic_get_plane_state(state, plane);
+		if (IS_ERR(plane_state)) {
+			ret = PTR_ERR(plane_state);
+			goto unlock;
+		}
+
+		drm_atomic_set_fb_for_plane(plane_state, NULL);
+		ret = drm_atomic_set_crtc_for_plane(plane_state, NULL);
+		if (ret)
+			goto unlock;
+
+		plane_mask |= BIT(drm_plane_index(plane));
+
+		plane->old_fb = plane->fb;
+		plane->fb = NULL;
+	}
+
+	if (plane_mask)
+		ret = drm_atomic_commit(state);
+
+unlock:
+	if (plane_mask)
+		drm_atomic_clean_old_fb(dev, plane_mask, ret);
+
+	if (ret == -EDEADLK) {
+		drm_modeset_backoff(&ctx);
+		goto retry;
+	}
+
+	if (ret || !plane_mask)
+		drm_atomic_state_free(state);
+
+	drm_modeset_drop_locks(&ctx);
+	drm_modeset_acquire_fini(&ctx);
+
+	return ret;
+}
+EXPORT_SYMBOL(drm_atomic_helper_remove_fb);
+
 /**
  * drm_atomic_helper_disable_all - disable all currently active outputs
  * @dev: DRM device
diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
index aaf6ab42f2c1..51c5a00ffdff 100644
--- a/drivers/gpu/drm/drm_crtc.c
+++ b/drivers/gpu/drm/drm_crtc.c
@@ -577,6 +577,42 @@ void drm_framebuffer_cleanup(struct drm_framebuffer *fb)
 }
 EXPORT_SYMBOL(drm_framebuffer_cleanup);
 
+static int
+legacy_remove_fb(struct drm_framebuffer *fb)
+{
+	struct drm_device *dev = fb->dev;
+	struct drm_crtc *crtc;
+	struct drm_plane *plane;
+	struct drm_mode_set set;
+	int ret = 0;
+
+	/* atomic drivers must use drm_atomic_helper_remove_fb */
+	WARN_ON(drm_core_check_feature(dev, DRIVER_ATOMIC));
+
+	drm_modeset_lock_all(dev);
+	/* remove from any CRTC */
+	drm_for_each_crtc(crtc, dev) {
+		if (crtc->primary->fb == fb) {
+			/* should turn off the crtc */
+			memset(&set, 0, sizeof(struct drm_mode_set));
+			set.crtc = crtc;
+			set.fb = NULL;
+			ret = drm_mode_set_config_internal(&set);
+			if (ret)
+				goto out;
+		}
+	}
+
+	drm_for_each_plane(plane, dev) {
+		if (plane->fb == fb)
+			drm_plane_force_disable(plane);
+	}
+
+out:
+	drm_modeset_unlock_all(dev);
+	return ret;
+}
+
 /**
  * drm_framebuffer_remove - remove and unreference a framebuffer object
  * @fb: framebuffer to remove
@@ -592,9 +628,6 @@ EXPORT_SYMBOL(drm_framebuffer_cleanup);
 void drm_framebuffer_remove(struct drm_framebuffer *fb)
 {
 	struct drm_device *dev;
-	struct drm_crtc *crtc;
-	struct drm_plane *plane;
-	struct drm_mode_set set;
 	int ret;
 
 	if (!fb)
@@ -620,25 +653,13 @@ void drm_framebuffer_remove(struct drm_framebuffer *fb)
 	 * in this manner.
 	 */
 	if (atomic_read(&fb->refcount.refcount) > 1) {
-		drm_modeset_lock_all(dev);
-		/* remove from any CRTC */
-		drm_for_each_crtc(crtc, dev) {
-			if (crtc->primary->fb == fb) {
-				/* should turn off the crtc */
-				memset(&set, 0, sizeof(struct drm_mode_set));
-				set.crtc = crtc;
-				set.fb = NULL;
-				ret = drm_mode_set_config_internal(&set);
-				if (ret)
-					DRM_ERROR("failed to reset crtc %p when fb was deleted\n", crtc);
-			}
-		}
+		if (dev->driver->remove_fb)
+			ret = dev->driver->remove_fb(fb);
+		else
+			ret = legacy_remove_fb(fb);
 
-		drm_for_each_plane(plane, dev) {
-			if (plane->fb == fb)
-				drm_plane_force_disable(plane);
-		}
-		drm_modeset_unlock_all(dev);
+		if (ret)
+			DRM_ERROR("failed to remove fb %i/%p with %i\n", fb->base.id, fb, ret);
 	}
 
 	drm_framebuffer_unreference(fb);
diff --git a/drivers/gpu/drm/exynos/exynos_drm_drv.c b/drivers/gpu/drm/exynos/exynos_drm_drv.c
index 5344940c8a07..2705a315f1ec 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_drv.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_drv.c
@@ -423,6 +423,7 @@ static struct drm_driver exynos_drm_driver = {
 	.dumb_create		= exynos_drm_gem_dumb_create,
 	.dumb_map_offset	= exynos_drm_gem_dumb_map_offset,
 	.dumb_destroy		= drm_gem_dumb_destroy,
+	.remove_fb		= drm_atomic_helper_remove_fb,
 	.prime_handle_to_fd	= drm_gem_prime_handle_to_fd,
 	.prime_fd_to_handle	= drm_gem_prime_fd_to_handle,
 	.gem_prime_export	= drm_gem_prime_export,
diff --git a/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.c b/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.c
index e8d9337a66d8..f7562b178819 100644
--- a/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.c
+++ b/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.c
@@ -24,6 +24,7 @@
 #include <drm/drmP.h>
 #include <drm/drm_crtc_helper.h>
 #include <drm/drm_gem_cma_helper.h>
+#include <drm/drm_atomic_helper.h>
 
 #include "fsl_dcu_drm_crtc.h"
 #include "fsl_dcu_drm_drv.h"
@@ -194,6 +195,7 @@ static struct drm_driver fsl_dcu_drm_driver = {
 	.dumb_create		= drm_gem_cma_dumb_create,
 	.dumb_map_offset	= drm_gem_cma_dumb_map_offset,
 	.dumb_destroy		= drm_gem_dumb_destroy,
+	.remove_fb		= drm_atomic_helper_remove_fb,
 	.fops			= &fsl_dcu_drm_fops,
 	.name			= "fsl-dcu-drm",
 	.desc			= "Freescale DCU DRM",
diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 2a076b005af9..9cbf88adf280 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -42,6 +42,7 @@
 #include <linux/vgaarb.h>
 #include <linux/vga_switcheroo.h>
 #include <drm/drm_crtc_helper.h>
+#include <drm/drm_atomic_helper.h>
 
 static struct drm_driver driver;
 
@@ -1712,6 +1713,7 @@ static struct drm_driver driver = {
 	.dumb_create = i915_gem_dumb_create,
 	.dumb_map_offset = i915_gem_mmap_gtt,
 	.dumb_destroy = drm_gem_dumb_destroy,
+	.remove_fb = drm_atomic_helper_remove_fb,
 	.ioctls = i915_ioctls,
 	.fops = &i915_driver_fops,
 	.name = DRIVER_NAME,
diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c
index d52910e2c26c..fab3d7f036ae 100644
--- a/drivers/gpu/drm/msm/msm_drv.c
+++ b/drivers/gpu/drm/msm/msm_drv.c
@@ -973,6 +973,7 @@ static struct drm_driver msm_driver = {
 	.dumb_create        = msm_gem_dumb_create,
 	.dumb_map_offset    = msm_gem_dumb_map_offset,
 	.dumb_destroy       = drm_gem_dumb_destroy,
+	.remove_fb          = drm_atomic_helper_remove_fb,
 	.prime_handle_to_fd = drm_gem_prime_handle_to_fd,
 	.prime_fd_to_handle = drm_gem_prime_fd_to_handle,
 	.gem_prime_export   = drm_gem_prime_export,
diff --git a/drivers/gpu/drm/omapdrm/omap_drv.c b/drivers/gpu/drm/omapdrm/omap_drv.c
index 80398a684cae..9f3bacbad118 100644
--- a/drivers/gpu/drm/omapdrm/omap_drv.c
+++ b/drivers/gpu/drm/omapdrm/omap_drv.c
@@ -817,6 +817,7 @@ static struct drm_driver omap_drm_driver = {
 	.dumb_create = omap_gem_dumb_create,
 	.dumb_map_offset = omap_gem_dumb_map_offset,
 	.dumb_destroy = drm_gem_dumb_destroy,
+	.remove_fb = drm_atomic_helper_remove_fb,
 	.ioctls = ioctls,
 	.num_ioctls = DRM_OMAP_NUM_IOCTLS,
 	.fops = &omapdriver_fops,
diff --git a/drivers/gpu/drm/rcar-du/rcar_du_drv.c b/drivers/gpu/drm/rcar-du/rcar_du_drv.c
index ed6006bf6bd8..3b7388d87815 100644
--- a/drivers/gpu/drm/rcar-du/rcar_du_drv.c
+++ b/drivers/gpu/drm/rcar-du/rcar_du_drv.c
@@ -25,6 +25,7 @@
 #include <drm/drm_crtc_helper.h>
 #include <drm/drm_fb_cma_helper.h>
 #include <drm/drm_gem_cma_helper.h>
+#include <drm/drm_atomic_helper.h>
 
 #include "rcar_du_crtc.h"
 #include "rcar_du_drv.h"
@@ -231,6 +232,7 @@ static struct drm_driver rcar_du_driver = {
 	.dumb_create		= rcar_du_dumb_create,
 	.dumb_map_offset	= drm_gem_cma_dumb_map_offset,
 	.dumb_destroy		= drm_gem_dumb_destroy,
+	.remove_fb		= drm_atomic_helper_remove_fb,
 	.fops			= &rcar_du_fops,
 	.name			= "rcar-du",
 	.desc			= "Renesas R-Car Display Unit",
diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_drv.c b/drivers/gpu/drm/rockchip/rockchip_drm_drv.c
index 896da09e49ee..bf2162e4b131 100644
--- a/drivers/gpu/drm/rockchip/rockchip_drm_drv.c
+++ b/drivers/gpu/drm/rockchip/rockchip_drm_drv.c
@@ -19,6 +19,7 @@
 #include <drm/drmP.h>
 #include <drm/drm_crtc_helper.h>
 #include <drm/drm_fb_helper.h>
+#include <drm/drm_atomic_helper.h>
 #include <linux/dma-mapping.h>
 #include <linux/pm_runtime.h>
 #include <linux/module.h>
@@ -290,6 +291,7 @@ static struct drm_driver rockchip_drm_driver = {
 	.dumb_create		= rockchip_gem_dumb_create,
 	.dumb_map_offset	= rockchip_gem_dumb_map_offset,
 	.dumb_destroy		= drm_gem_dumb_destroy,
+	.remove_fb		= drm_atomic_helper_remove_fb,
 	.prime_handle_to_fd	= drm_gem_prime_handle_to_fd,
 	.prime_fd_to_handle	= drm_gem_prime_fd_to_handle,
 	.gem_prime_import	= drm_gem_prime_import,
diff --git a/drivers/gpu/drm/tegra/drm.c b/drivers/gpu/drm/tegra/drm.c
index 8e6b18caa706..c60f86c8f73d 100644
--- a/drivers/gpu/drm/tegra/drm.c
+++ b/drivers/gpu/drm/tegra/drm.c
@@ -944,6 +944,8 @@ static struct drm_driver tegra_drm_driver = {
 	.dumb_map_offset = tegra_bo_dumb_map_offset,
 	.dumb_destroy = drm_gem_dumb_destroy,
 
+	.remove_fb = drm_atomic_helper_remove_fb,
+
 	.ioctls = tegra_drm_ioctls,
 	.num_ioctls = ARRAY_SIZE(tegra_drm_ioctls),
 	.fops = &tegra_drm_fops,
diff --git a/drivers/gpu/drm/vc4/vc4_drv.c b/drivers/gpu/drm/vc4/vc4_drv.c
index b7d2ff0e6e1f..4dde5d924d51 100644
--- a/drivers/gpu/drm/vc4/vc4_drv.c
+++ b/drivers/gpu/drm/vc4/vc4_drv.c
@@ -15,6 +15,7 @@
 #include <linux/of_platform.h>
 #include <linux/platform_device.h>
 #include "drm_fb_cma_helper.h"
+#include "drm/drm_atomic_helper.h"
 
 #include "uapi/drm/vc4_drm.h"
 #include "vc4_drv.h"
@@ -115,6 +116,8 @@ static struct drm_driver vc4_drm_driver = {
 	.dumb_map_offset = drm_gem_cma_dumb_map_offset,
 	.dumb_destroy = drm_gem_dumb_destroy,
 
+	.remove_fb = drm_atomic_helper_remove_fb,
+
 	.ioctls = vc4_drm_ioctls,
 	.num_ioctls = ARRAY_SIZE(vc4_drm_ioctls),
 	.fops = &vc4_drm_fops,
diff --git a/drivers/gpu/drm/virtio/virtgpu_drv.c b/drivers/gpu/drm/virtio/virtgpu_drv.c
index 7f898cfdc746..56912941eaf8 100644
--- a/drivers/gpu/drm/virtio/virtgpu_drv.c
+++ b/drivers/gpu/drm/virtio/virtgpu_drv.c
@@ -31,6 +31,7 @@
 #include <linux/pci.h>
 #include "drmP.h"
 #include "drm/drm.h"
+#include "drm/drm_atomic_helper.h"
 
 #include "virtgpu_drv.h"
 static struct drm_driver driver;
@@ -129,6 +130,8 @@ static struct drm_driver driver = {
 	.dumb_map_offset = virtio_gpu_mode_dumb_mmap,
 	.dumb_destroy = virtio_gpu_mode_dumb_destroy,
 
+	.remove_fb = drm_atomic_helper_remove_fb,
+
 #if defined(CONFIG_DEBUG_FS)
 	.debugfs_init = virtio_gpu_debugfs_init,
 	.debugfs_cleanup = virtio_gpu_debugfs_takedown,
diff --git a/include/drm/drmP.h b/include/drm/drmP.h
index 3c8422c69572..31483c2fef51 100644
--- a/include/drm/drmP.h
+++ b/include/drm/drmP.h
@@ -638,6 +638,9 @@ struct drm_driver {
 			    struct drm_device *dev,
 			    uint32_t handle);
 
+	/* rmfb and drm_framebuffer_remove hook */
+	int (*remove_fb)(struct drm_framebuffer *fb);
+
 	/* Driver private ops for this object */
 	const struct vm_operations_struct *gem_vm_ops;
 
diff --git a/include/drm/drm_atomic_helper.h b/include/drm/drm_atomic_helper.h
index 9054598c9a7a..2d5ff5c80c76 100644
--- a/include/drm/drm_atomic_helper.h
+++ b/include/drm/drm_atomic_helper.h
@@ -87,6 +87,8 @@ int drm_atomic_helper_set_config(struct drm_mode_set *set);
 int __drm_atomic_helper_set_config(struct drm_mode_set *set,
 		struct drm_atomic_state *state);
 
+int drm_atomic_helper_remove_fb(struct drm_framebuffer *fb);
+
 int drm_atomic_helper_disable_all(struct drm_device *dev,
 				  struct drm_modeset_acquire_ctx *ctx);
 struct drm_atomic_state *drm_atomic_helper_suspend(struct drm_device *dev);

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/core: Do not preserve framebuffer on rmfb.

[Daniel Vetter]

On Tue, Mar 22, 2016 at 11:53:53AM +0100, Maarten Lankhorst wrote:
> Op 22-03-16 om 11:50 schreef Daniel Vetter:
> > On Tue, Mar 22, 2016 at 10:32:32AM +0100, Maarten Lankhorst wrote:
> >> Op 21-03-16 om 18:37 schreef Daniel Vetter:
> >>> On Mon, Mar 21, 2016 at 03:11:17PM +0100, Maarten Lankhorst wrote:
> >>>> It turns out that preserving framebuffers after the rmfb call breaks
> >>>> vmwgfx userspace. This was originally introduced because it was thought
> >>>> nobody relied on the behavior, but unfortunately it seems there are
> >>>> exceptions.
> >>>>
> >>>> drm_framebuffer_remove may fail with -EINTR now, so a straight revert
> >>>> is impossible. There is no way to remove the framebuffer from the lists
> >>>> and active planes without introducing a race because of the different
> >>>> locking requirements. Instead call drm_framebuffer_remove from a
> >>>> workqueue, which is unaffected by signals.
> >>>>
> >>>> Cc: stable@vger.kernel.org #v4.4+
> >>>> Fixes: 13803132818c ("drm/core: Preserve the framebuffer after removing it.")
> >>>> Testcase: kms_flip.flip-vs-rmfb-interruptible
> >>>> References: https://lists.freedesktop.org/archives/dri-devel/2016-March/102876.html
> >>>> Cc: Thomas Hellstrom <thellstrom@vmware.com>
> >>>> Cc: David Herrmann <dh.herrmann@gmail.com>
> >>>> ---
> >>>>  drivers/gpu/drm/drm_crtc.c | 20 +++++++++++++++++++-
> >>>>  1 file changed, 19 insertions(+), 1 deletion(-)
> >>>>
> >>>> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
> >>>> index e08f962288d9..b7d0b959f088 100644
> >>>> --- a/drivers/gpu/drm/drm_crtc.c
> >>>> +++ b/drivers/gpu/drm/drm_crtc.c
> >>>> @@ -3434,6 +3434,18 @@ int drm_mode_addfb2(struct drm_device *dev,
> >>>>   return 0;
> >>>>  }
> >>>>
> >>>> +struct drm_mode_rmfb_work {
> >>>> + struct work_struct work;
> >>>> + struct drm_framebuffer *fb;
> >>>> +};
> >>>> +
> >>>> +static void drm_mode_rmfb_work_fn(struct work_struct *w)
> >>>> +{
> >>>> + struct drm_mode_rmfb_work *arg = container_of(w, typeof(*arg), work);
> >>>> +
> >>>> + drm_framebuffer_remove(arg->fb);
> >>> drm_framebuffer_remove still has the problem of not working correctly with
> >>> atomic since atomic commit will complain if we try to do more than 1
> >>> commit per ww_acquire_ctx. I think we still need an atomic version of
> >>> this. Also probably a more nasty igt testcase which uses the same fb on
> >>> more than one plane to be able to hit this case.
> >> That's true, but a separate bug. :)
> > Atm we only use drm_framebuffer_remove in atomic drivers to nuke the fbdev
> > one at unload. With your patch userspace can't get there easily, and hence
> > it must be fixed. Maybe separate prep patch (also cc: stable) upfront?
> >
> Something like this?
> 
> Unfortunately I need to collect acks first.

Oh dear, we're back to that discussion about how to pull this off :( I
forgot about all that, silly me ...

For a much more minimal fix, what about a default rmfb helper which
implements the right thing automatically depending upon DRIVER_ATOMIC,
plus the hook only for i915 to get atomic behaviour? With that we only
need ack for drm core + i915, which we can get fast ;-)

Would then also mean that drm_atomic_remove_framebuffer() would need to be
in drm_atomic.c probably, so that drm_crtc.c can call it.

	if (dev->driver->remove_fb)
		ret = dev->driver->remove_fb(fb);
	else if (drm_core_check_feature(dev, DRIVER_ATOMIC))
		ret = drm_atomic_remove_fb(fb);
	else
		ret = legacy_remove_fb(fb);

Besides this, need some minimal kerneldoc from
drm_atomic_remove_framebuffer().

Cheers, Daniel

> 
> commit ed242f92c2e7571a6a5f649c2a67031debc73e44
> Author: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Date:   Thu Mar 17 13:42:08 2016 +0100
> 
>     drm/atomic: Add remove_fb function pointer.
>     
>     Use this in drm_framebuffer_remove, this is to remove the fb in an atomic way.
>     
>     Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> 
> diff --git a/drivers/gpu/drm/arm/hdlcd_drv.c b/drivers/gpu/drm/arm/hdlcd_drv.c
> index 56b829f97699..50ba6adb74e8 100644
> --- a/drivers/gpu/drm/arm/hdlcd_drv.c
> +++ b/drivers/gpu/drm/arm/hdlcd_drv.c
> @@ -324,6 +324,7 @@ static struct drm_driver hdlcd_driver = {
>  	.dumb_create = drm_gem_cma_dumb_create,
>  	.dumb_map_offset = drm_gem_cma_dumb_map_offset,
>  	.dumb_destroy = drm_gem_dumb_destroy,
> +	.remove_fb = drm_atomic_helper_remove_fb,
>  	.prime_handle_to_fd = drm_gem_prime_handle_to_fd,
>  	.prime_fd_to_handle = drm_gem_prime_fd_to_handle,
>  	.gem_prime_export = drm_gem_prime_export,
> diff --git a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c
> index 3d8d16402d07..5d357f729114 100644
> --- a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c
> +++ b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c
> @@ -711,6 +711,7 @@ static struct drm_driver atmel_hlcdc_dc_driver = {
>  	.dumb_create = drm_gem_cma_dumb_create,
>  	.dumb_map_offset = drm_gem_cma_dumb_map_offset,
>  	.dumb_destroy = drm_gem_dumb_destroy,
> +	.remove_fb = drm_atomic_helper_remove_fb,
>  	.fops = &fops,
>  	.name = "atmel-hlcdc",
>  	.desc = "Atmel HLCD Controller DRM",
> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> index 4befe25c81c7..eb3b413560df 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -1960,6 +1960,72 @@ commit:
>  	return 0;
>  }
>  
> +int drm_atomic_helper_remove_fb(struct drm_framebuffer *fb)
> +{
> +	struct drm_modeset_acquire_ctx ctx;
> +	struct drm_device *dev = fb->dev;
> +	struct drm_atomic_state *state;
> +	struct drm_plane *plane;
> +	int ret = 0;
> +	unsigned plane_mask;
> +
> +	state = drm_atomic_state_alloc(dev);
> +	if (!state)
> +		return -ENOMEM;
> +
> +	drm_modeset_acquire_init(&ctx, 0);
> +
> +retry:
> +	plane_mask = 0;
> +	ret = drm_modeset_lock_all_ctx(dev, &ctx);
> +	if (ret)
> +		goto unlock;
> +
> +	drm_for_each_plane(plane, dev) {
> +		struct drm_plane_state *plane_state;
> +
> +		if (plane->state->fb != fb)
> +			continue;
> +
> +		plane_state = drm_atomic_get_plane_state(state, plane);
> +		if (IS_ERR(plane_state)) {
> +			ret = PTR_ERR(plane_state);
> +			goto unlock;
> +		}
> +
> +		drm_atomic_set_fb_for_plane(plane_state, NULL);
> +		ret = drm_atomic_set_crtc_for_plane(plane_state, NULL);
> +		if (ret)
> +			goto unlock;
> +
> +		plane_mask |= BIT(drm_plane_index(plane));
> +
> +		plane->old_fb = plane->fb;
> +		plane->fb = NULL;
> +	}
> +
> +	if (plane_mask)
> +		ret = drm_atomic_commit(state);
> +
> +unlock:
> +	if (plane_mask)
> +		drm_atomic_clean_old_fb(dev, plane_mask, ret);
> +
> +	if (ret == -EDEADLK) {
> +		drm_modeset_backoff(&ctx);
> +		goto retry;
> +	}
> +
> +	if (ret || !plane_mask)
> +		drm_atomic_state_free(state);
> +
> +	drm_modeset_drop_locks(&ctx);
> +	drm_modeset_acquire_fini(&ctx);
> +
> +	return ret;
> +}
> +EXPORT_SYMBOL(drm_atomic_helper_remove_fb);
> +
>  /**
>   * drm_atomic_helper_disable_all - disable all currently active outputs
>   * @dev: DRM device
> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
> index aaf6ab42f2c1..51c5a00ffdff 100644
> --- a/drivers/gpu/drm/drm_crtc.c
> +++ b/drivers/gpu/drm/drm_crtc.c
> @@ -577,6 +577,42 @@ void drm_framebuffer_cleanup(struct drm_framebuffer *fb)
>  }
>  EXPORT_SYMBOL(drm_framebuffer_cleanup);
>  
> +static int
> +legacy_remove_fb(struct drm_framebuffer *fb)
> +{
> +	struct drm_device *dev = fb->dev;
> +	struct drm_crtc *crtc;
> +	struct drm_plane *plane;
> +	struct drm_mode_set set;
> +	int ret = 0;
> +
> +	/* atomic drivers must use drm_atomic_helper_remove_fb */
> +	WARN_ON(drm_core_check_feature(dev, DRIVER_ATOMIC));
> +
> +	drm_modeset_lock_all(dev);
> +	/* remove from any CRTC */
> +	drm_for_each_crtc(crtc, dev) {
> +		if (crtc->primary->fb == fb) {
> +			/* should turn off the crtc */
> +			memset(&set, 0, sizeof(struct drm_mode_set));
> +			set.crtc = crtc;
> +			set.fb = NULL;
> +			ret = drm_mode_set_config_internal(&set);
> +			if (ret)
> +				goto out;
> +		}
> +	}
> +
> +	drm_for_each_plane(plane, dev) {
> +		if (plane->fb == fb)
> +			drm_plane_force_disable(plane);
> +	}
> +
> +out:
> +	drm_modeset_unlock_all(dev);
> +	return ret;
> +}
> +
>  /**
>   * drm_framebuffer_remove - remove and unreference a framebuffer object
>   * @fb: framebuffer to remove
> @@ -592,9 +628,6 @@ EXPORT_SYMBOL(drm_framebuffer_cleanup);
>  void drm_framebuffer_remove(struct drm_framebuffer *fb)
>  {
>  	struct drm_device *dev;
> -	struct drm_crtc *crtc;
> -	struct drm_plane *plane;
> -	struct drm_mode_set set;
>  	int ret;
>  
>  	if (!fb)
> @@ -620,25 +653,13 @@ void drm_framebuffer_remove(struct drm_framebuffer *fb)
>  	 * in this manner.
>  	 */
>  	if (atomic_read(&fb->refcount.refcount) > 1) {
> -		drm_modeset_lock_all(dev);
> -		/* remove from any CRTC */
> -		drm_for_each_crtc(crtc, dev) {
> -			if (crtc->primary->fb == fb) {
> -				/* should turn off the crtc */
> -				memset(&set, 0, sizeof(struct drm_mode_set));
> -				set.crtc = crtc;
> -				set.fb = NULL;
> -				ret = drm_mode_set_config_internal(&set);
> -				if (ret)
> -					DRM_ERROR("failed to reset crtc %p when fb was deleted\n", crtc);
> -			}
> -		}
> +		if (dev->driver->remove_fb)
> +			ret = dev->driver->remove_fb(fb);
> +		else
> +			ret = legacy_remove_fb(fb);
>  
> -		drm_for_each_plane(plane, dev) {
> -			if (plane->fb == fb)
> -				drm_plane_force_disable(plane);
> -		}
> -		drm_modeset_unlock_all(dev);
> +		if (ret)
> +			DRM_ERROR("failed to remove fb %i/%p with %i\n", fb->base.id, fb, ret);
>  	}
>  
>  	drm_framebuffer_unreference(fb);
> diff --git a/drivers/gpu/drm/exynos/exynos_drm_drv.c b/drivers/gpu/drm/exynos/exynos_drm_drv.c
> index 5344940c8a07..2705a315f1ec 100644
> --- a/drivers/gpu/drm/exynos/exynos_drm_drv.c
> +++ b/drivers/gpu/drm/exynos/exynos_drm_drv.c
> @@ -423,6 +423,7 @@ static struct drm_driver exynos_drm_driver = {
>  	.dumb_create		= exynos_drm_gem_dumb_create,
>  	.dumb_map_offset	= exynos_drm_gem_dumb_map_offset,
>  	.dumb_destroy		= drm_gem_dumb_destroy,
> +	.remove_fb		= drm_atomic_helper_remove_fb,
>  	.prime_handle_to_fd	= drm_gem_prime_handle_to_fd,
>  	.prime_fd_to_handle	= drm_gem_prime_fd_to_handle,
>  	.gem_prime_export	= drm_gem_prime_export,
> diff --git a/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.c b/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.c
> index e8d9337a66d8..f7562b178819 100644
> --- a/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.c
> +++ b/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.c
> @@ -24,6 +24,7 @@
>  #include <drm/drmP.h>
>  #include <drm/drm_crtc_helper.h>
>  #include <drm/drm_gem_cma_helper.h>
> +#include <drm/drm_atomic_helper.h>
>  
>  #include "fsl_dcu_drm_crtc.h"
>  #include "fsl_dcu_drm_drv.h"
> @@ -194,6 +195,7 @@ static struct drm_driver fsl_dcu_drm_driver = {
>  	.dumb_create		= drm_gem_cma_dumb_create,
>  	.dumb_map_offset	= drm_gem_cma_dumb_map_offset,
>  	.dumb_destroy		= drm_gem_dumb_destroy,
> +	.remove_fb		= drm_atomic_helper_remove_fb,
>  	.fops			= &fsl_dcu_drm_fops,
>  	.name			= "fsl-dcu-drm",
>  	.desc			= "Freescale DCU DRM",
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index 2a076b005af9..9cbf88adf280 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -42,6 +42,7 @@
>  #include <linux/vgaarb.h>
>  #include <linux/vga_switcheroo.h>
>  #include <drm/drm_crtc_helper.h>
> +#include <drm/drm_atomic_helper.h>
>  
>  static struct drm_driver driver;
>  
> @@ -1712,6 +1713,7 @@ static struct drm_driver driver = {
>  	.dumb_create = i915_gem_dumb_create,
>  	.dumb_map_offset = i915_gem_mmap_gtt,
>  	.dumb_destroy = drm_gem_dumb_destroy,
> +	.remove_fb = drm_atomic_helper_remove_fb,
>  	.ioctls = i915_ioctls,
>  	.fops = &i915_driver_fops,
>  	.name = DRIVER_NAME,
> diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c
> index d52910e2c26c..fab3d7f036ae 100644
> --- a/drivers/gpu/drm/msm/msm_drv.c
> +++ b/drivers/gpu/drm/msm/msm_drv.c
> @@ -973,6 +973,7 @@ static struct drm_driver msm_driver = {
>  	.dumb_create        = msm_gem_dumb_create,
>  	.dumb_map_offset    = msm_gem_dumb_map_offset,
>  	.dumb_destroy       = drm_gem_dumb_destroy,
> +	.remove_fb          = drm_atomic_helper_remove_fb,
>  	.prime_handle_to_fd = drm_gem_prime_handle_to_fd,
>  	.prime_fd_to_handle = drm_gem_prime_fd_to_handle,
>  	.gem_prime_export   = drm_gem_prime_export,
> diff --git a/drivers/gpu/drm/omapdrm/omap_drv.c b/drivers/gpu/drm/omapdrm/omap_drv.c
> index 80398a684cae..9f3bacbad118 100644
> --- a/drivers/gpu/drm/omapdrm/omap_drv.c
> +++ b/drivers/gpu/drm/omapdrm/omap_drv.c
> @@ -817,6 +817,7 @@ static struct drm_driver omap_drm_driver = {
>  	.dumb_create = omap_gem_dumb_create,
>  	.dumb_map_offset = omap_gem_dumb_map_offset,
>  	.dumb_destroy = drm_gem_dumb_destroy,
> +	.remove_fb = drm_atomic_helper_remove_fb,
>  	.ioctls = ioctls,
>  	.num_ioctls = DRM_OMAP_NUM_IOCTLS,
>  	.fops = &omapdriver_fops,
> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_drv.c b/drivers/gpu/drm/rcar-du/rcar_du_drv.c
> index ed6006bf6bd8..3b7388d87815 100644
> --- a/drivers/gpu/drm/rcar-du/rcar_du_drv.c
> +++ b/drivers/gpu/drm/rcar-du/rcar_du_drv.c
> @@ -25,6 +25,7 @@
>  #include <drm/drm_crtc_helper.h>
>  #include <drm/drm_fb_cma_helper.h>
>  #include <drm/drm_gem_cma_helper.h>
> +#include <drm/drm_atomic_helper.h>
>  
>  #include "rcar_du_crtc.h"
>  #include "rcar_du_drv.h"
> @@ -231,6 +232,7 @@ static struct drm_driver rcar_du_driver = {
>  	.dumb_create		= rcar_du_dumb_create,
>  	.dumb_map_offset	= drm_gem_cma_dumb_map_offset,
>  	.dumb_destroy		= drm_gem_dumb_destroy,
> +	.remove_fb		= drm_atomic_helper_remove_fb,
>  	.fops			= &rcar_du_fops,
>  	.name			= "rcar-du",
>  	.desc			= "Renesas R-Car Display Unit",
> diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_drv.c b/drivers/gpu/drm/rockchip/rockchip_drm_drv.c
> index 896da09e49ee..bf2162e4b131 100644
> --- a/drivers/gpu/drm/rockchip/rockchip_drm_drv.c
> +++ b/drivers/gpu/drm/rockchip/rockchip_drm_drv.c
> @@ -19,6 +19,7 @@
>  #include <drm/drmP.h>
>  #include <drm/drm_crtc_helper.h>
>  #include <drm/drm_fb_helper.h>
> +#include <drm/drm_atomic_helper.h>
>  #include <linux/dma-mapping.h>
>  #include <linux/pm_runtime.h>
>  #include <linux/module.h>
> @@ -290,6 +291,7 @@ static struct drm_driver rockchip_drm_driver = {
>  	.dumb_create		= rockchip_gem_dumb_create,
>  	.dumb_map_offset	= rockchip_gem_dumb_map_offset,
>  	.dumb_destroy		= drm_gem_dumb_destroy,
> +	.remove_fb		= drm_atomic_helper_remove_fb,
>  	.prime_handle_to_fd	= drm_gem_prime_handle_to_fd,
>  	.prime_fd_to_handle	= drm_gem_prime_fd_to_handle,
>  	.gem_prime_import	= drm_gem_prime_import,
> diff --git a/drivers/gpu/drm/tegra/drm.c b/drivers/gpu/drm/tegra/drm.c
> index 8e6b18caa706..c60f86c8f73d 100644
> --- a/drivers/gpu/drm/tegra/drm.c
> +++ b/drivers/gpu/drm/tegra/drm.c
> @@ -944,6 +944,8 @@ static struct drm_driver tegra_drm_driver = {
>  	.dumb_map_offset = tegra_bo_dumb_map_offset,
>  	.dumb_destroy = drm_gem_dumb_destroy,
>  
> +	.remove_fb = drm_atomic_helper_remove_fb,
> +
>  	.ioctls = tegra_drm_ioctls,
>  	.num_ioctls = ARRAY_SIZE(tegra_drm_ioctls),
>  	.fops = &tegra_drm_fops,
> diff --git a/drivers/gpu/drm/vc4/vc4_drv.c b/drivers/gpu/drm/vc4/vc4_drv.c
> index b7d2ff0e6e1f..4dde5d924d51 100644
> --- a/drivers/gpu/drm/vc4/vc4_drv.c
> +++ b/drivers/gpu/drm/vc4/vc4_drv.c
> @@ -15,6 +15,7 @@
>  #include <linux/of_platform.h>
>  #include <linux/platform_device.h>
>  #include "drm_fb_cma_helper.h"
> +#include "drm/drm_atomic_helper.h"
>  
>  #include "uapi/drm/vc4_drm.h"
>  #include "vc4_drv.h"
> @@ -115,6 +116,8 @@ static struct drm_driver vc4_drm_driver = {
>  	.dumb_map_offset = drm_gem_cma_dumb_map_offset,
>  	.dumb_destroy = drm_gem_dumb_destroy,
>  
> +	.remove_fb = drm_atomic_helper_remove_fb,
> +
>  	.ioctls = vc4_drm_ioctls,
>  	.num_ioctls = ARRAY_SIZE(vc4_drm_ioctls),
>  	.fops = &vc4_drm_fops,
> diff --git a/drivers/gpu/drm/virtio/virtgpu_drv.c b/drivers/gpu/drm/virtio/virtgpu_drv.c
> index 7f898cfdc746..56912941eaf8 100644
> --- a/drivers/gpu/drm/virtio/virtgpu_drv.c
> +++ b/drivers/gpu/drm/virtio/virtgpu_drv.c
> @@ -31,6 +31,7 @@
>  #include <linux/pci.h>
>  #include "drmP.h"
>  #include "drm/drm.h"
> +#include "drm/drm_atomic_helper.h"
>  
>  #include "virtgpu_drv.h"
>  static struct drm_driver driver;
> @@ -129,6 +130,8 @@ static struct drm_driver driver = {
>  	.dumb_map_offset = virtio_gpu_mode_dumb_mmap,
>  	.dumb_destroy = virtio_gpu_mode_dumb_destroy,
>  
> +	.remove_fb = drm_atomic_helper_remove_fb,
> +
>  #if defined(CONFIG_DEBUG_FS)
>  	.debugfs_init = virtio_gpu_debugfs_init,
>  	.debugfs_cleanup = virtio_gpu_debugfs_takedown,
> diff --git a/include/drm/drmP.h b/include/drm/drmP.h
> index 3c8422c69572..31483c2fef51 100644
> --- a/include/drm/drmP.h
> +++ b/include/drm/drmP.h
> @@ -638,6 +638,9 @@ struct drm_driver {
>  			    struct drm_device *dev,
>  			    uint32_t handle);
>  
> +	/* rmfb and drm_framebuffer_remove hook */
> +	int (*remove_fb)(struct drm_framebuffer *fb);
> +
>  	/* Driver private ops for this object */
>  	const struct vm_operations_struct *gem_vm_ops;
>  
> diff --git a/include/drm/drm_atomic_helper.h b/include/drm/drm_atomic_helper.h
> index 9054598c9a7a..2d5ff5c80c76 100644
> --- a/include/drm/drm_atomic_helper.h
> +++ b/include/drm/drm_atomic_helper.h
> @@ -87,6 +87,8 @@ int drm_atomic_helper_set_config(struct drm_mode_set *set);
>  int __drm_atomic_helper_set_config(struct drm_mode_set *set,
>  		struct drm_atomic_state *state);
>  
> +int drm_atomic_helper_remove_fb(struct drm_framebuffer *fb);
> +
>  int drm_atomic_helper_disable_all(struct drm_device *dev,
>  				  struct drm_modeset_acquire_ctx *ctx);
>  struct drm_atomic_state *drm_atomic_helper_suspend(struct drm_device *dev);
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [PATCH] drm/core: Do not preserve framebuffer on rmfb.

[Daniel Vetter]

On Tue, Mar 22, 2016 at 11:53:53AM +0100, Maarten Lankhorst wrote:
> Op 22-03-16 om 11:50 schreef Daniel Vetter:
> > On Tue, Mar 22, 2016 at 10:32:32AM +0100, Maarten Lankhorst wrote:
> >> Op 21-03-16 om 18:37 schreef Daniel Vetter:
> >>> On Mon, Mar 21, 2016 at 03:11:17PM +0100, Maarten Lankhorst wrote:
> >>>> It turns out that preserving framebuffers after the rmfb call breaks
> >>>> vmwgfx userspace. This was originally introduced because it was thought
> >>>> nobody relied on the behavior, but unfortunately it seems there are
> >>>> exceptions.
> >>>>
> >>>> drm_framebuffer_remove may fail with -EINTR now, so a straight revert
> >>>> is impossible. There is no way to remove the framebuffer from the lists
> >>>> and active planes without introducing a race because of the different
> >>>> locking requirements. Instead call drm_framebuffer_remove from a
> >>>> workqueue, which is unaffected by signals.
> >>>>
> >>>> Cc: stable@vger.kernel.org #v4.4+
> >>>> Fixes: 13803132818c ("drm/core: Preserve the framebuffer after removing it.")
> >>>> Testcase: kms_flip.flip-vs-rmfb-interruptible
> >>>> References: https://lists.freedesktop.org/archives/dri-devel/2016-March/102876.html
> >>>> Cc: Thomas Hellstrom <thellstrom@vmware.com>
> >>>> Cc: David Herrmann <dh.herrmann@gmail.com>
> >>>> ---
> >>>>  drivers/gpu/drm/drm_crtc.c | 20 +++++++++++++++++++-
> >>>>  1 file changed, 19 insertions(+), 1 deletion(-)
> >>>>
> >>>> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
> >>>> index e08f962288d9..b7d0b959f088 100644
> >>>> --- a/drivers/gpu/drm/drm_crtc.c
> >>>> +++ b/drivers/gpu/drm/drm_crtc.c
> >>>> @@ -3434,6 +3434,18 @@ int drm_mode_addfb2(struct drm_device *dev,
> >>>>   return 0;
> >>>>  }
> >>>>
> >>>> +struct drm_mode_rmfb_work {
> >>>> + struct work_struct work;
> >>>> + struct drm_framebuffer *fb;
> >>>> +};
> >>>> +
> >>>> +static void drm_mode_rmfb_work_fn(struct work_struct *w)
> >>>> +{
> >>>> + struct drm_mode_rmfb_work *arg = container_of(w, typeof(*arg), work);
> >>>> +
> >>>> + drm_framebuffer_remove(arg->fb);
> >>> drm_framebuffer_remove still has the problem of not working correctly with
> >>> atomic since atomic commit will complain if we try to do more than 1
> >>> commit per ww_acquire_ctx. I think we still need an atomic version of
> >>> this. Also probably a more nasty igt testcase which uses the same fb on
> >>> more than one plane to be able to hit this case.
> >> That's true, but a separate bug. :)
> > Atm we only use drm_framebuffer_remove in atomic drivers to nuke the fbdev
> > one at unload. With your patch userspace can't get there easily, and hence
> > it must be fixed. Maybe separate prep patch (also cc: stable) upfront?
> >
> Something like this?
> 
> Unfortunately I need to collect acks first.

Oh dear, we're back to that discussion about how to pull this off :( I
forgot about all that, silly me ...

For a much more minimal fix, what about a default rmfb helper which
implements the right thing automatically depending upon DRIVER_ATOMIC,
plus the hook only for i915 to get atomic behaviour? With that we only
need ack for drm core + i915, which we can get fast ;-)

Would then also mean that drm_atomic_remove_framebuffer() would need to be
in drm_atomic.c probably, so that drm_crtc.c can call it.

	if (dev->driver->remove_fb)
		ret = dev->driver->remove_fb(fb);
	else if (drm_core_check_feature(dev, DRIVER_ATOMIC))
		ret = drm_atomic_remove_fb(fb);
	else
		ret = legacy_remove_fb(fb);

Besides this, need some minimal kerneldoc from
drm_atomic_remove_framebuffer().

Cheers, Daniel

> 
> commit ed242f92c2e7571a6a5f649c2a67031debc73e44
> Author: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Date:   Thu Mar 17 13:42:08 2016 +0100
> 
>     drm/atomic: Add remove_fb function pointer.
>     
>     Use this in drm_framebuffer_remove, this is to remove the fb in an atomic way.
>     
>     Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> 
> diff --git a/drivers/gpu/drm/arm/hdlcd_drv.c b/drivers/gpu/drm/arm/hdlcd_drv.c
> index 56b829f97699..50ba6adb74e8 100644
> --- a/drivers/gpu/drm/arm/hdlcd_drv.c
> +++ b/drivers/gpu/drm/arm/hdlcd_drv.c
> @@ -324,6 +324,7 @@ static struct drm_driver hdlcd_driver = {
>  	.dumb_create = drm_gem_cma_dumb_create,
>  	.dumb_map_offset = drm_gem_cma_dumb_map_offset,
>  	.dumb_destroy = drm_gem_dumb_destroy,
> +	.remove_fb = drm_atomic_helper_remove_fb,
>  	.prime_handle_to_fd = drm_gem_prime_handle_to_fd,
>  	.prime_fd_to_handle = drm_gem_prime_fd_to_handle,
>  	.gem_prime_export = drm_gem_prime_export,
> diff --git a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c
> index 3d8d16402d07..5d357f729114 100644
> --- a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c
> +++ b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c
> @@ -711,6 +711,7 @@ static struct drm_driver atmel_hlcdc_dc_driver = {
>  	.dumb_create = drm_gem_cma_dumb_create,
>  	.dumb_map_offset = drm_gem_cma_dumb_map_offset,
>  	.dumb_destroy = drm_gem_dumb_destroy,
> +	.remove_fb = drm_atomic_helper_remove_fb,
>  	.fops = &fops,
>  	.name = "atmel-hlcdc",
>  	.desc = "Atmel HLCD Controller DRM",
> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> index 4befe25c81c7..eb3b413560df 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -1960,6 +1960,72 @@ commit:
>  	return 0;
>  }
>  
> +int drm_atomic_helper_remove_fb(struct drm_framebuffer *fb)
> +{
> +	struct drm_modeset_acquire_ctx ctx;
> +	struct drm_device *dev = fb->dev;
> +	struct drm_atomic_state *state;
> +	struct drm_plane *plane;
> +	int ret = 0;
> +	unsigned plane_mask;
> +
> +	state = drm_atomic_state_alloc(dev);
> +	if (!state)
> +		return -ENOMEM;
> +
> +	drm_modeset_acquire_init(&ctx, 0);
> +
> +retry:
> +	plane_mask = 0;
> +	ret = drm_modeset_lock_all_ctx(dev, &ctx);
> +	if (ret)
> +		goto unlock;
> +
> +	drm_for_each_plane(plane, dev) {
> +		struct drm_plane_state *plane_state;
> +
> +		if (plane->state->fb != fb)
> +			continue;
> +
> +		plane_state = drm_atomic_get_plane_state(state, plane);
> +		if (IS_ERR(plane_state)) {
> +			ret = PTR_ERR(plane_state);
> +			goto unlock;
> +		}
> +
> +		drm_atomic_set_fb_for_plane(plane_state, NULL);
> +		ret = drm_atomic_set_crtc_for_plane(plane_state, NULL);
> +		if (ret)
> +			goto unlock;
> +
> +		plane_mask |= BIT(drm_plane_index(plane));
> +
> +		plane->old_fb = plane->fb;
> +		plane->fb = NULL;
> +	}
> +
> +	if (plane_mask)
> +		ret = drm_atomic_commit(state);
> +
> +unlock:
> +	if (plane_mask)
> +		drm_atomic_clean_old_fb(dev, plane_mask, ret);
> +
> +	if (ret == -EDEADLK) {
> +		drm_modeset_backoff(&ctx);
> +		goto retry;
> +	}
> +
> +	if (ret || !plane_mask)
> +		drm_atomic_state_free(state);
> +
> +	drm_modeset_drop_locks(&ctx);
> +	drm_modeset_acquire_fini(&ctx);
> +
> +	return ret;
> +}
> +EXPORT_SYMBOL(drm_atomic_helper_remove_fb);
> +
>  /**
>   * drm_atomic_helper_disable_all - disable all currently active outputs
>   * @dev: DRM device
> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
> index aaf6ab42f2c1..51c5a00ffdff 100644
> --- a/drivers/gpu/drm/drm_crtc.c
> +++ b/drivers/gpu/drm/drm_crtc.c
> @@ -577,6 +577,42 @@ void drm_framebuffer_cleanup(struct drm_framebuffer *fb)
>  }
>  EXPORT_SYMBOL(drm_framebuffer_cleanup);
>  
> +static int
> +legacy_remove_fb(struct drm_framebuffer *fb)
> +{
> +	struct drm_device *dev = fb->dev;
> +	struct drm_crtc *crtc;
> +	struct drm_plane *plane;
> +	struct drm_mode_set set;
> +	int ret = 0;
> +
> +	/* atomic drivers must use drm_atomic_helper_remove_fb */
> +	WARN_ON(drm_core_check_feature(dev, DRIVER_ATOMIC));
> +
> +	drm_modeset_lock_all(dev);
> +	/* remove from any CRTC */
> +	drm_for_each_crtc(crtc, dev) {
> +		if (crtc->primary->fb == fb) {
> +			/* should turn off the crtc */
> +			memset(&set, 0, sizeof(struct drm_mode_set));
> +			set.crtc = crtc;
> +			set.fb = NULL;
> +			ret = drm_mode_set_config_internal(&set);
> +			if (ret)
> +				goto out;
> +		}
> +	}
> +
> +	drm_for_each_plane(plane, dev) {
> +		if (plane->fb == fb)
> +			drm_plane_force_disable(plane);
> +	}
> +
> +out:
> +	drm_modeset_unlock_all(dev);
> +	return ret;
> +}
> +
>  /**
>   * drm_framebuffer_remove - remove and unreference a framebuffer object
>   * @fb: framebuffer to remove
> @@ -592,9 +628,6 @@ EXPORT_SYMBOL(drm_framebuffer_cleanup);
>  void drm_framebuffer_remove(struct drm_framebuffer *fb)
>  {
>  	struct drm_device *dev;
> -	struct drm_crtc *crtc;
> -	struct drm_plane *plane;
> -	struct drm_mode_set set;
>  	int ret;
>  
>  	if (!fb)
> @@ -620,25 +653,13 @@ void drm_framebuffer_remove(struct drm_framebuffer *fb)
>  	 * in this manner.
>  	 */
>  	if (atomic_read(&fb->refcount.refcount) > 1) {
> -		drm_modeset_lock_all(dev);
> -		/* remove from any CRTC */
> -		drm_for_each_crtc(crtc, dev) {
> -			if (crtc->primary->fb == fb) {
> -				/* should turn off the crtc */
> -				memset(&set, 0, sizeof(struct drm_mode_set));
> -				set.crtc = crtc;
> -				set.fb = NULL;
> -				ret = drm_mode_set_config_internal(&set);
> -				if (ret)
> -					DRM_ERROR("failed to reset crtc %p when fb was deleted\n", crtc);
> -			}
> -		}
> +		if (dev->driver->remove_fb)
> +			ret = dev->driver->remove_fb(fb);
> +		else
> +			ret = legacy_remove_fb(fb);
>  
> -		drm_for_each_plane(plane, dev) {
> -			if (plane->fb == fb)
> -				drm_plane_force_disable(plane);
> -		}
> -		drm_modeset_unlock_all(dev);
> +		if (ret)
> +			DRM_ERROR("failed to remove fb %i/%p with %i\n", fb->base.id, fb, ret);
>  	}
>  
>  	drm_framebuffer_unreference(fb);
> diff --git a/drivers/gpu/drm/exynos/exynos_drm_drv.c b/drivers/gpu/drm/exynos/exynos_drm_drv.c
> index 5344940c8a07..2705a315f1ec 100644
> --- a/drivers/gpu/drm/exynos/exynos_drm_drv.c
> +++ b/drivers/gpu/drm/exynos/exynos_drm_drv.c
> @@ -423,6 +423,7 @@ static struct drm_driver exynos_drm_driver = {
>  	.dumb_create		= exynos_drm_gem_dumb_create,
>  	.dumb_map_offset	= exynos_drm_gem_dumb_map_offset,
>  	.dumb_destroy		= drm_gem_dumb_destroy,
> +	.remove_fb		= drm_atomic_helper_remove_fb,
>  	.prime_handle_to_fd	= drm_gem_prime_handle_to_fd,
>  	.prime_fd_to_handle	= drm_gem_prime_fd_to_handle,
>  	.gem_prime_export	= drm_gem_prime_export,
> diff --git a/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.c b/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.c
> index e8d9337a66d8..f7562b178819 100644
> --- a/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.c
> +++ b/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.c
> @@ -24,6 +24,7 @@
>  #include <drm/drmP.h>
>  #include <drm/drm_crtc_helper.h>
>  #include <drm/drm_gem_cma_helper.h>
> +#include <drm/drm_atomic_helper.h>
>  
>  #include "fsl_dcu_drm_crtc.h"
>  #include "fsl_dcu_drm_drv.h"
> @@ -194,6 +195,7 @@ static struct drm_driver fsl_dcu_drm_driver = {
>  	.dumb_create		= drm_gem_cma_dumb_create,
>  	.dumb_map_offset	= drm_gem_cma_dumb_map_offset,
>  	.dumb_destroy		= drm_gem_dumb_destroy,
> +	.remove_fb		= drm_atomic_helper_remove_fb,
>  	.fops			= &fsl_dcu_drm_fops,
>  	.name			= "fsl-dcu-drm",
>  	.desc			= "Freescale DCU DRM",
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index 2a076b005af9..9cbf88adf280 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -42,6 +42,7 @@
>  #include <linux/vgaarb.h>
>  #include <linux/vga_switcheroo.h>
>  #include <drm/drm_crtc_helper.h>
> +#include <drm/drm_atomic_helper.h>
>  
>  static struct drm_driver driver;
>  
> @@ -1712,6 +1713,7 @@ static struct drm_driver driver = {
>  	.dumb_create = i915_gem_dumb_create,
>  	.dumb_map_offset = i915_gem_mmap_gtt,
>  	.dumb_destroy = drm_gem_dumb_destroy,
> +	.remove_fb = drm_atomic_helper_remove_fb,
>  	.ioctls = i915_ioctls,
>  	.fops = &i915_driver_fops,
>  	.name = DRIVER_NAME,
> diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c
> index d52910e2c26c..fab3d7f036ae 100644
> --- a/drivers/gpu/drm/msm/msm_drv.c
> +++ b/drivers/gpu/drm/msm/msm_drv.c
> @@ -973,6 +973,7 @@ static struct drm_driver msm_driver = {
>  	.dumb_create        = msm_gem_dumb_create,
>  	.dumb_map_offset    = msm_gem_dumb_map_offset,
>  	.dumb_destroy       = drm_gem_dumb_destroy,
> +	.remove_fb          = drm_atomic_helper_remove_fb,
>  	.prime_handle_to_fd = drm_gem_prime_handle_to_fd,
>  	.prime_fd_to_handle = drm_gem_prime_fd_to_handle,
>  	.gem_prime_export   = drm_gem_prime_export,
> diff --git a/drivers/gpu/drm/omapdrm/omap_drv.c b/drivers/gpu/drm/omapdrm/omap_drv.c
> index 80398a684cae..9f3bacbad118 100644
> --- a/drivers/gpu/drm/omapdrm/omap_drv.c
> +++ b/drivers/gpu/drm/omapdrm/omap_drv.c
> @@ -817,6 +817,7 @@ static struct drm_driver omap_drm_driver = {
>  	.dumb_create = omap_gem_dumb_create,
>  	.dumb_map_offset = omap_gem_dumb_map_offset,
>  	.dumb_destroy = drm_gem_dumb_destroy,
> +	.remove_fb = drm_atomic_helper_remove_fb,
>  	.ioctls = ioctls,
>  	.num_ioctls = DRM_OMAP_NUM_IOCTLS,
>  	.fops = &omapdriver_fops,
> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_drv.c b/drivers/gpu/drm/rcar-du/rcar_du_drv.c
> index ed6006bf6bd8..3b7388d87815 100644
> --- a/drivers/gpu/drm/rcar-du/rcar_du_drv.c
> +++ b/drivers/gpu/drm/rcar-du/rcar_du_drv.c
> @@ -25,6 +25,7 @@
>  #include <drm/drm_crtc_helper.h>
>  #include <drm/drm_fb_cma_helper.h>
>  #include <drm/drm_gem_cma_helper.h>
> +#include <drm/drm_atomic_helper.h>
>  
>  #include "rcar_du_crtc.h"
>  #include "rcar_du_drv.h"
> @@ -231,6 +232,7 @@ static struct drm_driver rcar_du_driver = {
>  	.dumb_create		= rcar_du_dumb_create,
>  	.dumb_map_offset	= drm_gem_cma_dumb_map_offset,
>  	.dumb_destroy		= drm_gem_dumb_destroy,
> +	.remove_fb		= drm_atomic_helper_remove_fb,
>  	.fops			= &rcar_du_fops,
>  	.name			= "rcar-du",
>  	.desc			= "Renesas R-Car Display Unit",
> diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_drv.c b/drivers/gpu/drm/rockchip/rockchip_drm_drv.c
> index 896da09e49ee..bf2162e4b131 100644
> --- a/drivers/gpu/drm/rockchip/rockchip_drm_drv.c
> +++ b/drivers/gpu/drm/rockchip/rockchip_drm_drv.c
> @@ -19,6 +19,7 @@
>  #include <drm/drmP.h>
>  #include <drm/drm_crtc_helper.h>
>  #include <drm/drm_fb_helper.h>
> +#include <drm/drm_atomic_helper.h>
>  #include <linux/dma-mapping.h>
>  #include <linux/pm_runtime.h>
>  #include <linux/module.h>
> @@ -290,6 +291,7 @@ static struct drm_driver rockchip_drm_driver = {
>  	.dumb_create		= rockchip_gem_dumb_create,
>  	.dumb_map_offset	= rockchip_gem_dumb_map_offset,
>  	.dumb_destroy		= drm_gem_dumb_destroy,
> +	.remove_fb		= drm_atomic_helper_remove_fb,
>  	.prime_handle_to_fd	= drm_gem_prime_handle_to_fd,
>  	.prime_fd_to_handle	= drm_gem_prime_fd_to_handle,
>  	.gem_prime_import	= drm_gem_prime_import,
> diff --git a/drivers/gpu/drm/tegra/drm.c b/drivers/gpu/drm/tegra/drm.c
> index 8e6b18caa706..c60f86c8f73d 100644
> --- a/drivers/gpu/drm/tegra/drm.c
> +++ b/drivers/gpu/drm/tegra/drm.c
> @@ -944,6 +944,8 @@ static struct drm_driver tegra_drm_driver = {
>  	.dumb_map_offset = tegra_bo_dumb_map_offset,
>  	.dumb_destroy = drm_gem_dumb_destroy,
>  
> +	.remove_fb = drm_atomic_helper_remove_fb,
> +
>  	.ioctls = tegra_drm_ioctls,
>  	.num_ioctls = ARRAY_SIZE(tegra_drm_ioctls),
>  	.fops = &tegra_drm_fops,
> diff --git a/drivers/gpu/drm/vc4/vc4_drv.c b/drivers/gpu/drm/vc4/vc4_drv.c
> index b7d2ff0e6e1f..4dde5d924d51 100644
> --- a/drivers/gpu/drm/vc4/vc4_drv.c
> +++ b/drivers/gpu/drm/vc4/vc4_drv.c
> @@ -15,6 +15,7 @@
>  #include <linux/of_platform.h>
>  #include <linux/platform_device.h>
>  #include "drm_fb_cma_helper.h"
> +#include "drm/drm_atomic_helper.h"
>  
>  #include "uapi/drm/vc4_drm.h"
>  #include "vc4_drv.h"
> @@ -115,6 +116,8 @@ static struct drm_driver vc4_drm_driver = {
>  	.dumb_map_offset = drm_gem_cma_dumb_map_offset,
>  	.dumb_destroy = drm_gem_dumb_destroy,
>  
> +	.remove_fb = drm_atomic_helper_remove_fb,
> +
>  	.ioctls = vc4_drm_ioctls,
>  	.num_ioctls = ARRAY_SIZE(vc4_drm_ioctls),
>  	.fops = &vc4_drm_fops,
> diff --git a/drivers/gpu/drm/virtio/virtgpu_drv.c b/drivers/gpu/drm/virtio/virtgpu_drv.c
> index 7f898cfdc746..56912941eaf8 100644
> --- a/drivers/gpu/drm/virtio/virtgpu_drv.c
> +++ b/drivers/gpu/drm/virtio/virtgpu_drv.c
> @@ -31,6 +31,7 @@
>  #include <linux/pci.h>
>  #include "drmP.h"
>  #include "drm/drm.h"
> +#include "drm/drm_atomic_helper.h"
>  
>  #include "virtgpu_drv.h"
>  static struct drm_driver driver;
> @@ -129,6 +130,8 @@ static struct drm_driver driver = {
>  	.dumb_map_offset = virtio_gpu_mode_dumb_mmap,
>  	.dumb_destroy = virtio_gpu_mode_dumb_destroy,
>  
> +	.remove_fb = drm_atomic_helper_remove_fb,
> +
>  #if defined(CONFIG_DEBUG_FS)
>  	.debugfs_init = virtio_gpu_debugfs_init,
>  	.debugfs_cleanup = virtio_gpu_debugfs_takedown,
> diff --git a/include/drm/drmP.h b/include/drm/drmP.h
> index 3c8422c69572..31483c2fef51 100644
> --- a/include/drm/drmP.h
> +++ b/include/drm/drmP.h
> @@ -638,6 +638,9 @@ struct drm_driver {
>  			    struct drm_device *dev,
>  			    uint32_t handle);
>  
> +	/* rmfb and drm_framebuffer_remove hook */
> +	int (*remove_fb)(struct drm_framebuffer *fb);
> +
>  	/* Driver private ops for this object */
>  	const struct vm_operations_struct *gem_vm_ops;
>  
> diff --git a/include/drm/drm_atomic_helper.h b/include/drm/drm_atomic_helper.h
> index 9054598c9a7a..2d5ff5c80c76 100644
> --- a/include/drm/drm_atomic_helper.h
> +++ b/include/drm/drm_atomic_helper.h
> @@ -87,6 +87,8 @@ int drm_atomic_helper_set_config(struct drm_mode_set *set);
>  int __drm_atomic_helper_set_config(struct drm_mode_set *set,
>  		struct drm_atomic_state *state);
>  
> +int drm_atomic_helper_remove_fb(struct drm_framebuffer *fb);
> +
>  int drm_atomic_helper_disable_all(struct drm_device *dev,
>  				  struct drm_modeset_acquire_ctx *ctx);
>  struct drm_atomic_state *drm_atomic_helper_suspend(struct drm_device *dev);
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/core: Do not preserve framebuffer on rmfb.

[Maarten Lankhorst]

Op 22-03-16 om 12:09 schreef Daniel Vetter:
> On Tue, Mar 22, 2016 at 11:53:53AM +0100, Maarten Lankhorst wrote:
>> Op 22-03-16 om 11:50 schreef Daniel Vetter:
>>> On Tue, Mar 22, 2016 at 10:32:32AM +0100, Maarten Lankhorst wrote:
>>>> Op 21-03-16 om 18:37 schreef Daniel Vetter:
>>>>> On Mon, Mar 21, 2016 at 03:11:17PM +0100, Maarten Lankhorst wrote:
>>>>>> It turns out that preserving framebuffers after the rmfb call breaks
>>>>>> vmwgfx userspace. This was originally introduced because it was thought
>>>>>> nobody relied on the behavior, but unfortunately it seems there are
>>>>>> exceptions.
>>>>>>
>>>>>> drm_framebuffer_remove may fail with -EINTR now, so a straight revert
>>>>>> is impossible. There is no way to remove the framebuffer from the lists
>>>>>> and active planes without introducing a race because of the different
>>>>>> locking requirements. Instead call drm_framebuffer_remove from a
>>>>>> workqueue, which is unaffected by signals.
>>>>>>
>>>>>> Cc: stable@vger.kernel.org #v4.4+
>>>>>> Fixes: 13803132818c ("drm/core: Preserve the framebuffer after removing it.")
>>>>>> Testcase: kms_flip.flip-vs-rmfb-interruptible
>>>>>> References: https://lists.freedesktop.org/archives/dri-devel/2016-March/102876.html
>>>>>> Cc: Thomas Hellstrom <thellstrom@vmware.com>
>>>>>> Cc: David Herrmann <dh.herrmann@gmail.com>
>>>>>> ---
>>>>>>  drivers/gpu/drm/drm_crtc.c | 20 +++++++++++++++++++-
>>>>>>  1 file changed, 19 insertions(+), 1 deletion(-)
>>>>>>
>>>>>> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
>>>>>> index e08f962288d9..b7d0b959f088 100644
>>>>>> --- a/drivers/gpu/drm/drm_crtc.c
>>>>>> +++ b/drivers/gpu/drm/drm_crtc.c
>>>>>> @@ -3434,6 +3434,18 @@ int drm_mode_addfb2(struct drm_device *dev,
>>>>>>   return 0;
>>>>>>  }
>>>>>>
>>>>>> +struct drm_mode_rmfb_work {
>>>>>> + struct work_struct work;
>>>>>> + struct drm_framebuffer *fb;
>>>>>> +};
>>>>>> +
>>>>>> +static void drm_mode_rmfb_work_fn(struct work_struct *w)
>>>>>> +{
>>>>>> + struct drm_mode_rmfb_work *arg = container_of(w, typeof(*arg), work);
>>>>>> +
>>>>>> + drm_framebuffer_remove(arg->fb);
>>>>> drm_framebuffer_remove still has the problem of not working correctly with
>>>>> atomic since atomic commit will complain if we try to do more than 1
>>>>> commit per ww_acquire_ctx. I think we still need an atomic version of
>>>>> this. Also probably a more nasty igt testcase which uses the same fb on
>>>>> more than one plane to be able to hit this case.
>>>> That's true, but a separate bug. :)
>>> Atm we only use drm_framebuffer_remove in atomic drivers to nuke the fbdev
>>> one at unload. With your patch userspace can't get there easily, and hence
>>> it must be fixed. Maybe separate prep patch (also cc: stable) upfront?
>>>
>> Something like this?
>>
>> Unfortunately I need to collect acks first.
> Oh dear, we're back to that discussion about how to pull this off :( I
> forgot about all that, silly me ...
>
> For a much more minimal fix, what about a default rmfb helper which
> implements the right thing automatically depending upon DRIVER_ATOMIC,
> plus the hook only for i915 to get atomic behaviour? With that we only
> need ack for drm core + i915, which we can get fast ;-)
>
> Would then also mean that drm_atomic_remove_framebuffer() would need to be
> in drm_atomic.c probably, so that drm_crtc.c can call it.
>
> 	if (dev->driver->remove_fb)
> 		ret = dev->driver->remove_fb(fb);
> 	else if (drm_core_check_feature(dev, DRIVER_ATOMIC))
> 		ret = drm_atomic_remove_fb(fb);
> 	else
> 		ret = legacy_remove_fb(fb);
>
> Besides this, need some minimal kerneldoc from
> drm_atomic_remove_framebuffer().
>
Would work, also means less cruft. :) After i915 is converted it could go away..

~Maarten