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

Message ID 20190718153737.28657-1-sam@ravnborg.org
Headers show
Series
  • drm/via: drop use of deprecated headers drmP.h and drm_os_linux.h
Related show

Message

Sam Ravnborg July 18, 2019, 3:37 p.m. UTC
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(-)

Comments

Sam Ravnborg July 19, 2019, 6:07 a.m. UTC | #1
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
Michel Dänzer July 19, 2019, 9:05 a.m. UTC | #2
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.
Daniel Vetter July 19, 2019, 9:36 a.m. UTC | #3
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
Sam Ravnborg July 19, 2019, 11:32 a.m. UTC | #4
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
Daniel Vetter July 19, 2019, 12:37 p.m. UTC | #5
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
Michel Dänzer July 19, 2019, 3:44 p.m. UTC | #6
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?