[PATCH 1/2] drm/fb-helper: Synchronize dirty worker with vblank

[Noralf Trønnes]

Den 10.09.2019 15.51, skrev Thomas Zimmermann:
> Hi
> 
> Am 10.09.19 um 15:34 schrieb Noralf Trønnes:
>>
>>
>> Den 10.09.2019 14.48, skrev Thomas Zimmermann:
>>> Hi
>>>
>>> Am 10.09.19 um 13:52 schrieb Gerd Hoffmann:
>>>> On Mon, Sep 09, 2019 at 04:06:32PM +0200, Thomas Zimmermann wrote:
>>>>> Before updating the display from the console's shadow buffer, the dirty
>>>>> worker now waits for vblank. This allows several screen updates to pile
>>>>> up and acts as a rate limiter.
>>>>>
>>>>> Signed-off-by: Thomas Zimmermann <tzimmermann at suse.de>
>>>>> ---
>>>>>  drivers/gpu/drm/drm_fb_helper.c | 12 ++++++++++++
>>>>>  1 file changed, 12 insertions(+)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
>>>>> index a7ba5b4902d6..017e2f6bd1b9 100644
>>>>> --- a/drivers/gpu/drm/drm_fb_helper.c
>>>>> +++ b/drivers/gpu/drm/drm_fb_helper.c
>>>>> @@ -402,8 +402,20 @@ static void drm_fb_helper_dirty_work(struct work_struct *work)
>>>>>  						    dirty_work);
>>>>>  	struct drm_clip_rect *clip = &helper->dirty_clip;
>>>>>  	struct drm_clip_rect clip_copy;
>>>>> +	struct drm_crtc *crtc;
>>>>>  	unsigned long flags;
>>>>>  	void *vaddr;
>>>>> +	int ret;
>>>>> +
>>>>> +	/* rate-limit update frequency */
>>>>> +	mutex_lock(&helper->lock);
>>>>> +	crtc = helper->client.modesets[0].crtc;
>>>>> +	ret = drm_crtc_vblank_get(crtc);
>>>>> +	if (!ret) {
>>>>> +		drm_crtc_wait_one_vblank(crtc);
>>>>> +		drm_crtc_vblank_put(crtc);
>>>>> +	}
>>>>> +	mutex_unlock(&helper->lock);
>>>>
>>>> Hmm, not sure it is the best plan to sleep for a while in the worker,
>>>> especially while holding the lock.
>>>>
>>>> What does the lock protect against here?  Accessing
>>>
>>> This lock is hold by the fbdev code during mode-setting operations (but
>>> not drawing operations). So *crtc might be gone if we don't hold it here.
>>>
>>>> helper->client.modesets?  If so then you can unlock before going to
>>>> sleep in drm_crtc_wait_one_vblank() I think.
>>>
>>> I looked, but I cannot find any code that protects crtc while vblank is
>>> active. I'd rather not change the current code until someone can clarify.
>>>
>>
>> The client->modesets array and the crtc struct member are invariant over
>> the lifetime of client (drm_client_modeset_create()). No need to take a
>> lock for access. See drm_client_modeset_release() for the things that
>> _can_ change and needs protection (protected by client->modeset_mutex).
> 
> Thanks for the reply. So we don't need the lock? But why does
> drm_fb_helper_ioctl() take it? ioctl exclusiveness?
> 

Because of drm_master_internal_acquire() it's necessary to take the lock
first. That's the locking rules of drm_fb_helper. First take the fb
helper lock, then the dev->master_mutex. We stay away if there's a
userspace master and if there's none, we prevent userspace from becoming
master while we do stuff.

But looking at drm_fb_helper_ioctl() now, I see that it's not strictly
necessary to do this since all this function can do is vblank wait and
that's fine even if userspace is master. The locking was necessary
before I refactored and moved stuff to drm_client, because at that time
the modeset array was deleted and recreated when probing connectors.
But it doesn't hurt either in case more functionality is added to the
ioctl. One wouldn't think that would ever happen, since fbdev is going
away soon, but still we keep polishing it ;)

Noralf.

>> I don't see a problem with sleeping in the worker though, but I might
>> miss out on something. AFAICS changes will just pile up in >dirty_clip
>> and the worker will be scheduled for a new run to happen when it's done
>> with the current update.
> 
> Yes, that's the intention of the patch. We hope to reduce the number of
> memcpys by handling several of them at once.
> 
> Best regards
> Thomas
> 
>>
>> Noralf.
>>
>>> Best regards
>>> Thomas
>>>
>>>>
>>>> cheers,
>>>>   Gerd
>>>>
>>>
>