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. :)
>> +}
>> +
>> /**
>> * 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.
--
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