| 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?
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. When replacing DRM_WAIT_ON() I took care to not change the uapi, by continue to use the original return codes. The following table was used for the transition: DRM_WAIT_ON wait_event_interruptible_timeout ----------- --------------------------------- condition OK: ret >= 1 timeout: -EBUSY 0 interrupted: -EINTR -ERESTARTSYS The changes has been build tested only. Testing on a real device would be highly appreciated. I had preferred that the via driver was replaced by the openchrome driver, but until we have it then we need to deal with the legacy via driver to remove old cruft in the drm subsystem. Note: A simpler approach had been to copy DRM_WAIT_ON to via_drv.h, but then the actual solution is presumeably better. Sam Sam Ravnborg (6): drm/via: drop use of DRM(READ|WRITE) macros drm/via: make via_drv.h self-contained drm/via: drop use of drmP.h drm/via: drop DRM_WAIT_ON() in via_dmablit.c drm/via: drop DRM_WAIT_ON() in via_irq drm/via: drop DRM_WAIT_ON() in via_video drivers/gpu/drm/via/via_dma.c | 9 +++++- drivers/gpu/drm/via/via_dmablit.c | 66 +++++++++++++++++++++++++++----------- drivers/gpu/drm/via/via_drv.c | 7 ++-- drivers/gpu/drm/via/via_drv.h | 21 +++++++++--- drivers/gpu/drm/via/via_irq.c | 37 +++++++++++++++------ drivers/gpu/drm/via/via_map.c | 6 +++- drivers/gpu/drm/via/via_mm.c | 7 +++- drivers/gpu/drm/via/via_verifier.c | 10 +++--- drivers/gpu/drm/via/via_video.c | 23 ++++++++++--- 9 files changed, 137 insertions(+), 49 deletions(-)