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@xxxxxxxxxxxxxxx #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@xxxxxxxxxx> >>>>>> Cc: David Herrmann <dh.herrmann@xxxxxxxxx> >>>>>> --- >>>>>> 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 -- To unsubscribe from this list: send the line "unsubscribe stable" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html