Re: [PATCH/RFT v1 0/6] drm/via: drop use of deprecated headers drmP.h and drm_os_linux.h

["Michel Dänzer" <michel@daenzer.net>]

On 2019-07-19 2:37 p.m., Daniel Vetter wrote:
> On Fri, Jul 19, 2019 at 1:32 PM Sam Ravnborg <sam@ravnborg.org> wrote:
>> On Fri, Jul 19, 2019 at 11:05:44AM +0200, Michel Dänzer wrote:
>>> On 2019-07-19 8:07 a.m., Sam Ravnborg wrote:
>>>> On Thu, Jul 18, 2019 at 05:37:31PM +0200, Sam Ravnborg wrote:
>>>>> This is some janitorial updates to the via driver
>>>>> that is required to get rid of deprecated headers
>>>>> in the drm subsystem.
>>>>>
>>>>> The first three patches are trivial, where
>>>>> the dependencies on drmP.h and drm_os_linux are dropped.
>>>>>
>>>>> The remaining three patches drop use of DRM_WAIT_ON().
>>>>> They are replaced by wait_event_interruptible_timeout().
>>>>> These patches could use a more critical review.
>>>>
>>>> The differences between DRM_WAIT_ON() and
>>>> wait_event_interruptible_timeout() are bigger than anticipated.
>>>>
>>>> The conversion I did for drm_vblank.c is bogus thus I expect
>>>> the conversion done for via is also bogus.
>>>
>>> What exactly is the problem though? Can you share information about the
>>> failures you're seeing?
>>>
>>> There was some discussion about DRM_WAIT_ON() "polling" on IRC. I assume
>>> that refers to it only sleeping for up to 0.01s before checking the
>>> condition again. In contrast, wait_event_interruptible_timeout() checks
>>> the condition once, then sleeps up to the full timeout before checking
>>> it again.
>> Correct - it was based on the feedback on irc from airlied and ickle
>> that made me conclude that the via part may not be good.
>> I cannot say if the polling versus timeout is properly dealt with in the
>> via driver and I am inclided to just move DRM_WAIT_ON() to via_drv.h and
>> name it VIA_WAIT_ON().
>> Then the changes to this legacy driver is minimal and it will not
>> prevent us from gettting rid of drm_os_linux.h
>>
>>>
>>> If that makes a difference for drm_wait_vblank_ioctl, it indicates that
>>> some other code which updates the vblank count or clears vblank->enabled
>>> doesn't wake up the vblank->queue.
>> Let me analyse a little...
>>
>> In drm_handle_vblank() there is a call to wake_up(&vblank->queue);
>> And this is called from an interrupt - OK.

I'm not sure why it's relevant whether or not a function can be called
from an interrupt handler.


>> drm_vblank_enable() is called outside an interrupt - no need for
>> wake_up()

There is no need here because there can be no sleeping waiters in the
queue, because vblank->enabled == false immediately terminates any waits.


>> drm_crtc_accurate_vblank_count() is called outside interrupt - no need
>> for wake_up()

This is called from interrupt handlers, at least from
amdgpu_dm.c:dm_pflip_high_irq(). Not sure it needs to wake up the queue
though, the driver should call drm_(crtc_)_handle_vblank anyway.


>> drm_vblank_disable_and_save() is called outside interrupt - no need for
>> wake_up()'

It can be called from an interrupt, via drm_handle_vblank ->
vblank_disable_fn. However, the only place where
drm_vblank_disable_and_save can be called with sleeping waiters in the
queue is in drm_crtc_vblank_off, which wakes up the queue afterwards
(which terminates all waits, because vblank->enabled == false at this
point).


>> That is all functions I could dig up that updates the vblank counter.

I agree, this should also cover everything which clears vblank->enabled.


So, are there still failures with v2 of the drm_wait_vblank_ioctl patch
(which I haven't seen after all BTW)? If yes, can you share information
about them? If not, why do you want to send a v3?


-- 
Earthling Michel Dänzer               |              https://www.amd.com
Libre software enthusiast             |             Mesa and X developer
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel