[PATCH 10/10] drm/fb-helper: Acquire modeset lock around shadow-buffer flushing

[Christian König]

Am 16.11.20 um 21:48 schrieb Daniel Vetter:
> On Mon, Nov 16, 2020 at 09:04:37PM +0100, Thomas Zimmermann wrote:
>> Flushing the fbdev's shadow buffer requires vmap'ing the BO memory, which
>> in turn requires pinning the BO. While being pinned, the BO cannot be moved
>> into VRAM for scanout. Consequently, a concurrent modeset operation that
>> involves the fbdev framebuffer would likely fail.
>>
>> Resolve this problem be acquiring the modeset lock of the planes that use
>> the fbdev framebuffer. On non-atomic drivers, also acquire the mode-config
>> lock. This serializes the flushing of the framebuffer with concurrent
>> modeset operations.
>>
>> Signed-off-by: Thomas Zimmermann <tzimmermann at suse.de>
> I think this is the wrong lock. What you want is the buffer lock, the
> dma_resv lock of the underlying gem_bo underneath the fb we have. And hold
> that from _vmap to the end of _vunmap.

+1 Yes exactly that came to my mind as well while reading this.

If you want to prevent a BO from moving while inside the kernel taking 
the reservation lock is usually the right thing to do.

Only when you need to return to userspace AND keep the BO in the same 
place then it is valid to pin it.

Regards,
Christian.

>
> Unfortuantely that's going to be one nasty refactoring adventure :-/
>
> So I think for plan B we need something nasty like this, but this here has
> disadvantage that fbdev activity in the background can seriously hurt the
> native kms client (despite that you're trying to filter a bit, you're
> creating a big lock across all planes and we've really worked hard to make
> those stand-alone as much as possible).
>
> I think we can do better here, since we're only worried about modesets
> from fbdev itself. Nothing else needs to be able to pull the buffer from
> system memory into vram while we have it pinned here.
>
> Holding mutex_lock(fb_helper->lock) here instead, with a big comment
> explaining why that's enough, and that the clean fix would be holding the
> dma_resv_lock, should  do the trick.
> -Daniel
>
>> ---
>>   drivers/gpu/drm/drm_fb_helper.c | 43 +++++++++++++++++++++++++++++++--
>>   1 file changed, 41 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
>> index 5a22c744378c..af485c71a42a 100644
>> --- a/drivers/gpu/drm/drm_fb_helper.c
>> +++ b/drivers/gpu/drm/drm_fb_helper.c
>> @@ -394,20 +394,59 @@ static void drm_fb_helper_damage_blit_real(struct drm_fb_helper *fb_helper,
>>   static int drm_fb_helper_damage_blit(struct drm_fb_helper *fb_helper,
>>   				     struct drm_clip_rect *clip)
>>   {
>> +	struct drm_device *dev = fb_helper->dev;
>> +	struct drm_framebuffer *fb = fb_helper->fb;
>>   	struct drm_client_buffer *buffer = fb_helper->buffer;
>> +	struct drm_modeset_acquire_ctx ctx;
>> +	struct drm_framebuffer *plane_fb;
>> +	struct drm_plane *plane;
>>   	struct dma_buf_map map, dst;
>>   	int ret;
>>   
>> +	if (!drm_drv_uses_atomic_modeset(dev))
>> +		mutex_lock(&dev->mode_config.mutex);
>> +
>> +	drm_modeset_acquire_init(&ctx, 0);
>> +
>> +retry:
>> +	drm_for_each_plane(plane, dev) {
>> +		ret = drm_modeset_lock(&plane->mutex, &ctx);
>> +		if (ret == -EDEADLK) {
>> +			ret = drm_modeset_backoff(&ctx);
>> +			if (!ret)
>> +				goto retry;
>> +		} else if (ret) {
>> +			goto out;
>> +		}
>> +
>> +		if (drm_drv_uses_atomic_modeset(dev))
>> +			plane_fb = plane->state->fb;
>> +		else
>> +			plane_fb = plane->fb;
>> +
>> +		if (plane_fb != fb) {
>> +			drm_modeset_unlock(&plane->mutex);
>> +			continue;
>> +		}
>> +	}
>> +
>>   	ret = drm_client_buffer_vmap(buffer, &map);
>>   	if (ret)
>> -		return ret;
>> +		goto out;
>>   
>>   	dst = map;
>>   	drm_fb_helper_damage_blit_real(fb_helper, clip, &dst);
>>   
>>   	drm_client_buffer_vunmap(buffer);
>>   
>> -	return 0;
>> +out:
>> +	drm_modeset_drop_locks(&ctx);
>> +	drm_modeset_acquire_fini(&ctx);
>> +
>> +	if (!drm_drv_uses_atomic_modeset(dev))
>> +		mutex_unlock(&dev->mode_config.mutex);
>> +
>> +	return ret;
>>   }
>>   
>>   static void drm_fb_helper_damage_work(struct work_struct *work)
>> -- 
>> 2.29.2
>>