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

[Daniel Vetter]

On Fri, Jul 19, 2019 at 1:32 PM Sam Ravnborg <sam at 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
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch