[4/4] drm/TODO: add a task to kill DRM_UNLOCKED

Submitted by Emil Velikov on May 22, 2019, 3:47 p.m.

Details

Message ID 20190522154702.16269-4-emil.l.velikov@gmail.com
State New
Headers show
Series "Series without cover letter" ( rev: 1 ) in DRI devel

Not browsing as part of any series.

Commit Message

Emil Velikov May 22, 2019, 3:47 p.m.
From: Emil Velikov <emil.velikov@collabora.com>

Should minimise the copy/paste mistakes, fixed with previous patches.

Cc: Daniel Vetter <daniel@ffwll.ch>
Signed-off-by: Emil Velikov <emil.velikov@collabora.com>
---
Daniel, not 100% sold on the idea. That plus listing you as a contact
point ;-)

What do you thing?
Emil
---
 Documentation/gpu/todo.rst | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

Patch hide | download patch | download mbox

diff --git a/Documentation/gpu/todo.rst b/Documentation/gpu/todo.rst
index 66f05f4e469f..9e67d125f2fd 100644
--- a/Documentation/gpu/todo.rst
+++ b/Documentation/gpu/todo.rst
@@ -397,6 +397,25 @@  Some of these date from the very introduction of KMS in 2008 ...
   end, for which we could add drm_*_cleanup_kfree(). And then there's the (for
   historical reasons) misnamed drm_primary_helper_destroy() function.
 
+Use DRM_LOCKED instead of DRM_UNLOCKED
+--------------------------------------
+
+DRM_UNLOCKED is a remainder from the legacy DRM drivers. Seemingly drivers get
+tricked by it and it ends up in the driver private ioctls.
+
+Today no more legacy drivers are allowed and most core DRM ioctls are unlocked.
+
+Introduce DRM_LOCKED, use it to annotate only the relevant ioctls and kill the
+old DRM_UNLOCKED.
+
+Patch series should be split as follows:
+ - Patch 1: drm: add the new DRM_LOCKED flag and honour it
+ - Patch 2: drm: convert core ioctls from DRM_UNLOCKED to DRM_LOCKED
+ - Patch 3-...: drm/driverX: convert driver ioctls from ...
+ - Patch X: drm: remove no longer used DRM_UNLOCKED, drop todo item
+
+Contact: Emil Velikov, Daniel Vetter
+
 Better Testing
 ==============
 

Comments

On Wed, May 22, 2019 at 04:47:02PM +0100, Emil Velikov wrote:
> From: Emil Velikov <emil.velikov@collabora.com>
> 
> Should minimise the copy/paste mistakes, fixed with previous patches.
> 
> Cc: Daniel Vetter <daniel@ffwll.ch>
> Signed-off-by: Emil Velikov <emil.velikov@collabora.com>
> ---
> Daniel, not 100% sold on the idea. That plus listing you as a contact
> point ;-)
> 
> What do you thing?
> Emil
> ---
>  Documentation/gpu/todo.rst | 19 +++++++++++++++++++
>  1 file changed, 19 insertions(+)
> 
> diff --git a/Documentation/gpu/todo.rst b/Documentation/gpu/todo.rst
> index 66f05f4e469f..9e67d125f2fd 100644
> --- a/Documentation/gpu/todo.rst
> +++ b/Documentation/gpu/todo.rst
> @@ -397,6 +397,25 @@ Some of these date from the very introduction of KMS in 2008 ...
>    end, for which we could add drm_*_cleanup_kfree(). And then there's the (for
>    historical reasons) misnamed drm_primary_helper_destroy() function.
>  
> +Use DRM_LOCKED instead of DRM_UNLOCKED
> +--------------------------------------
> +
> +DRM_UNLOCKED is a remainder from the legacy DRM drivers. Seemingly drivers get
> +tricked by it and it ends up in the driver private ioctls.
> +
> +Today no more legacy drivers are allowed and most core DRM ioctls are unlocked.
> +
> +Introduce DRM_LOCKED, use it to annotate only the relevant ioctls and kill the
> +old DRM_UNLOCKED.
> +
> +Patch series should be split as follows:
> + - Patch 1: drm: add the new DRM_LOCKED flag and honour it
> + - Patch 2: drm: convert core ioctls from DRM_UNLOCKED to DRM_LOCKED
> + - Patch 3-...: drm/driverX: convert driver ioctls from ...
> + - Patch X: drm: remove no longer used DRM_UNLOCKED, drop todo item

Seems like too much bother for legacy drivers ... What I'd do is something
a lot cheaper, since all we're touching here are legacy drivers:

Remove DRM_UNLOCKED from everything except the old vblank ioctl. That one
we need to keep, because it freezes X:

commit 8f4ff2b06afcd6f151868474a432c603057eaf56
Author: Ilija Hadzic <ihadzic@research.bell-labs.com>
Date:   Mon Oct 31 17:46:18 2011 -0400

    drm: do not sleep on vblank while holding a mutex

Anything else I think is either never used by legacy userspace, or just
doesn't matter. That's simple enough that I don't think we really need a
todo for it :-) I guess if you want to kill DRM_UNLOCKED you could replace
the check with ioctl->func == drm_vblank_ioctl, ofc only in the
DRIVER_LEGACY path.

On patches 1-3 in your series:

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

Cheers, Daniel



> +
> +Contact: Emil Velikov, Daniel Vetter
> +
>  Better Testing
>  ==============
>  
> -- 
> 2.21.0
>
On 2019/05/22, Daniel Vetter wrote:
> On Wed, May 22, 2019 at 04:47:02PM +0100, Emil Velikov wrote:
> > From: Emil Velikov <emil.velikov@collabora.com>
> > 
> > Should minimise the copy/paste mistakes, fixed with previous patches.
> > 
> > Cc: Daniel Vetter <daniel@ffwll.ch>
> > Signed-off-by: Emil Velikov <emil.velikov@collabora.com>
> > ---
> > Daniel, not 100% sold on the idea. That plus listing you as a contact
> > point ;-)
> > 
> > What do you thing?
> > Emil
> > ---
> >  Documentation/gpu/todo.rst | 19 +++++++++++++++++++
> >  1 file changed, 19 insertions(+)
> > 
> > diff --git a/Documentation/gpu/todo.rst b/Documentation/gpu/todo.rst
> > index 66f05f4e469f..9e67d125f2fd 100644
> > --- a/Documentation/gpu/todo.rst
> > +++ b/Documentation/gpu/todo.rst
> > @@ -397,6 +397,25 @@ Some of these date from the very introduction of KMS in 2008 ...
> >    end, for which we could add drm_*_cleanup_kfree(). And then there's the (for
> >    historical reasons) misnamed drm_primary_helper_destroy() function.
> >  
> > +Use DRM_LOCKED instead of DRM_UNLOCKED
> > +--------------------------------------
> > +
> > +DRM_UNLOCKED is a remainder from the legacy DRM drivers. Seemingly drivers get
> > +tricked by it and it ends up in the driver private ioctls.
> > +
> > +Today no more legacy drivers are allowed and most core DRM ioctls are unlocked.
> > +
> > +Introduce DRM_LOCKED, use it to annotate only the relevant ioctls and kill the
> > +old DRM_UNLOCKED.
> > +
> > +Patch series should be split as follows:
> > + - Patch 1: drm: add the new DRM_LOCKED flag and honour it
> > + - Patch 2: drm: convert core ioctls from DRM_UNLOCKED to DRM_LOCKED
> > + - Patch 3-...: drm/driverX: convert driver ioctls from ...
> > + - Patch X: drm: remove no longer used DRM_UNLOCKED, drop todo item
> 
> Seems like too much bother for legacy drivers ... What I'd do is something
> a lot cheaper, since all we're touching here are legacy drivers:
> 
> Remove DRM_UNLOCKED from everything except the old vblank ioctl. That one
> we need to keep, because it freezes X:
> 
> commit 8f4ff2b06afcd6f151868474a432c603057eaf56
> Author: Ilija Hadzic <ihadzic@research.bell-labs.com>
> Date:   Mon Oct 31 17:46:18 2011 -0400
> 
>     drm: do not sleep on vblank while holding a mutex
> 
> Anything else I think is either never used by legacy userspace, or just
> doesn't matter. That's simple enough that I don't think we really need a
> todo for it :-) I guess if you want to kill DRM_UNLOCKED you could replace
> the check with ioctl->func == drm_vblank_ioctl, ofc only in the
> DRIVER_LEGACY path.
> 
Sounds like a much simpler solution indeed. Sadly I don't have much time to
double-check that this won't cause problems elsewhere, so I'll leave it that
to someone else.

> On patches 1-3 in your series:
> 
> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> 
Thank you very much.

-Emil
On Fri, May 24, 2019 at 04:17:16PM +0100, Emil Velikov wrote:
> On 2019/05/22, Daniel Vetter wrote:
> > On Wed, May 22, 2019 at 04:47:02PM +0100, Emil Velikov wrote:
> > > From: Emil Velikov <emil.velikov@collabora.com>
> > > 
> > > Should minimise the copy/paste mistakes, fixed with previous patches.
> > > 
> > > Cc: Daniel Vetter <daniel@ffwll.ch>
> > > Signed-off-by: Emil Velikov <emil.velikov@collabora.com>
> > > ---
> > > Daniel, not 100% sold on the idea. That plus listing you as a contact
> > > point ;-)
> > > 
> > > What do you thing?
> > > Emil
> > > ---
> > >  Documentation/gpu/todo.rst | 19 +++++++++++++++++++
> > >  1 file changed, 19 insertions(+)
> > > 
> > > diff --git a/Documentation/gpu/todo.rst b/Documentation/gpu/todo.rst
> > > index 66f05f4e469f..9e67d125f2fd 100644
> > > --- a/Documentation/gpu/todo.rst
> > > +++ b/Documentation/gpu/todo.rst
> > > @@ -397,6 +397,25 @@ Some of these date from the very introduction of KMS in 2008 ...
> > >    end, for which we could add drm_*_cleanup_kfree(). And then there's the (for
> > >    historical reasons) misnamed drm_primary_helper_destroy() function.
> > >  
> > > +Use DRM_LOCKED instead of DRM_UNLOCKED
> > > +--------------------------------------
> > > +
> > > +DRM_UNLOCKED is a remainder from the legacy DRM drivers. Seemingly drivers get
> > > +tricked by it and it ends up in the driver private ioctls.
> > > +
> > > +Today no more legacy drivers are allowed and most core DRM ioctls are unlocked.
> > > +
> > > +Introduce DRM_LOCKED, use it to annotate only the relevant ioctls and kill the
> > > +old DRM_UNLOCKED.
> > > +
> > > +Patch series should be split as follows:
> > > + - Patch 1: drm: add the new DRM_LOCKED flag and honour it
> > > + - Patch 2: drm: convert core ioctls from DRM_UNLOCKED to DRM_LOCKED
> > > + - Patch 3-...: drm/driverX: convert driver ioctls from ...
> > > + - Patch X: drm: remove no longer used DRM_UNLOCKED, drop todo item
> > 
> > Seems like too much bother for legacy drivers ... What I'd do is something
> > a lot cheaper, since all we're touching here are legacy drivers:
> > 
> > Remove DRM_UNLOCKED from everything except the old vblank ioctl. That one
> > we need to keep, because it freezes X:
> > 
> > commit 8f4ff2b06afcd6f151868474a432c603057eaf56
> > Author: Ilija Hadzic <ihadzic@research.bell-labs.com>
> > Date:   Mon Oct 31 17:46:18 2011 -0400
> > 
> >     drm: do not sleep on vblank while holding a mutex
> > 
> > Anything else I think is either never used by legacy userspace, or just
> > doesn't matter. That's simple enough that I don't think we really need a
> > todo for it :-) I guess if you want to kill DRM_UNLOCKED you could replace
> > the check with ioctl->func == drm_vblank_ioctl, ofc only in the
> > DRIVER_LEGACY path.
> > 
> Sounds like a much simpler solution indeed. Sadly I don't have much time to
> double-check that this won't cause problems elsewhere, so I'll leave it that
> to someone else.

I did dig through enough history that I'm confident. I'll type a patch and
some awkward-long commit message :-)
-Daniel

> 
> > On patches 1-3 in your series:
> > 
> > Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> > 
> Thank you very much.
> 
> -Emil