Message ID | 20190718153737.28657-1-sam@ravnborg.org (mailing list archive) |
---|---|
Headers | show |
Series | drm/via: drop use of deprecated headers drmP.h and drm_os_linux.h | expand |
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. I will post a v2. Sam
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. 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.
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. > > 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. The return values differ, latest patch from Sam passes CI: https://patchwork.freedesktop.org/patch/318909/ I have vague memories from exactly this fun when I ripped out DRM_WAIT_ON from i915. -Daniel
Hi Michael. 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. drm_vblank_enable() is called outside an interrupt - no need for wake_up() drm_crtc_accurate_vblank_count() is called outside interrupt - no need for wake_up() drm_vblank_disable_and_save() is called outside interrupt - no need for wake_up()' That is all functions I could dig up that updates the vblank counter. So based on this short analysis I actually start to think that I can use the variant that uses wait_event_interruptible_timeout() anyway. I will post a v3 and await feedback on that version. As for via - introducing VIA_WAIT_ON() is the simple solution. So I will post a v2 of this series using VIA_WAIT_ON(). Sam
On Fri, Jul 19, 2019 at 1:32 PM Sam Ravnborg <sam@ravnborg.org> wrote: > > Hi Michael. > > 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. > > drm_vblank_enable() is called outside an interrupt - no need for > wake_up() > > drm_crtc_accurate_vblank_count() is called outside interrupt - no need > for wake_up() > > drm_vblank_disable_and_save() is called outside interrupt - no need for > wake_up()' > > That is all functions I could dig up that updates the vblank counter. > > So based on this short analysis I actually start to think that > I can use the variant that uses wait_event_interruptible_timeout() > anyway. > I will post a v3 and await feedback on that version. Furthermore the new vblank sequence ioctls that Keith added don't use DRM_WAIT_ON(), so maybe that ship sailed already. -Daniel
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?