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

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

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.