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

[Maarten Lankhorst <maarten.lankhorst@linux.intel.com>]

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