[PATCH 2/2] drm/doc: Update docs about device instance setup

[David Herrmann]

Hi

On Mon, Aug 10, 2015 at 4:30 PM, Daniel Vetter  wrote:
> On Mon, Aug 10, 2015 at 02:34:18PM +0200, Thierry Reding wrote:
>> On Mon, Aug 10, 2015 at 02:07:49PM +0200, Daniel Vetter wrote:
>> > On Mon, Aug 10, 2015 at 01:59:07PM +0200, Thierry Reding wrote:
>> > > On Mon, Aug 10, 2015 at 11:55:38AM +0200, Daniel Vetter wrote:
>> > > > ->load is depracated, bus functionst are deprecated and everyone
>> > > > should use drm_dev_alloc
>> > >
>> > > Why would you want to deprecated ->load()? Even if you use
>> > > drm_dev_alloc() and drm_dev_register(), there's still use for ->load()
>> > > because it gives you the subsystem-level initialization entry point.
>> >
>> > ->load is called after the drm /dev node is registered and hence you can't
>> > really do any driver setup in there without risking races. We paper over
>> > that using drm_global_mutex, but that doesn't work for any other
>> > driver/userspace interface like sysfs/debugfs because of deadlocks.
>> >
>> > And we can't just reorder ->load to happen before the /dev nodes are
>> > registered because a lot of drivers would fall over if we do that.
>> >
>> > This is typical midlayer fail where the core calls into the driver instead
>> > of the other way round.
>>
>> Okay, but then if we're going to deprecate ->load(), I think we should
>> also come up with an upgrade plan. As it is, this just says that users
>> shouldn't do ->load(), but it doesn't tell them what to do instead.
>>
>> Would the proper procedure be to fix drivers so that ->load() can be
>> called between drm_dev_alloc() and drm_dev_register()? I suppose we
>> could add some sort of (temporary) flag for drivers to signal this and
>> then have drm_dev_register() call ->load() at the right time. If drivers
>> don't support it, we can keep what we have.
>>
>> That, of course, doesn't get rid of the midlayer, so perhaps a better
>> way forward would be to tell driver writers that they should be doing
>> subsystem-level setup between drm_dev_alloc() and drm_dev_register().
>
> That's exactly what this patch tries to accomplish by updating the
> kerneldoc and docbook. New sequence should be
>
> device_probe_callback_or_whatever()
> {
> drm_dev_alloc();
>
> ... driver init code ...
>
> drm_dev_register();
>
> return 0;
> }

_Yes_!

Acked-by: David Herrmann 

Thanks
David

[PATCH 2/2] drm/doc: Update docs about device instance setup

[Laurent Pinchart]

Hi Daniel,

Thank you for the patch.

On Monday 10 August 2015 11:55:38 Daniel Vetter wrote:
> ->load is depracated, bus functionst are deprecated and everyone

s/depracated/deprecated/

s/functionst/functions/

> should use drm_dev_alloc
> 
> So update the .tmpl (and pull a bunch of the overview docs into the
> sourcecode to increase chances that it'll stay in sync in the future)
> and add notes to functions which are deprecated. I didn't bother to
> clean up and document the unload sequence similarly since that one is
> still a bit a mess: drm_dev_unregister does way too much,
> drm_unplug_dev does what _unregister should be doing but then has the
> complication of promising something it doesn't actually do (it doesn't
> unplug existing open fds for instance, only prevents new ones).
> 
> Motivated since I don't want to hunt every new driver for usage of
> drm_platform_init any more ;-)
> 
> Signed-off-by: Daniel Vetter 
> ---
>  Documentation/DocBook/drm.tmpl | 99 ---
>  drivers/gpu/drm/drm_drv.c  | 55 +--
>  drivers/gpu/drm/drm_pci.c  | 11 +
>  drivers/gpu/drm/drm_platform.c |  3 ++
>  4 files changed, 83 insertions(+), 85 deletions(-)
> 
> diff --git a/Documentation/DocBook/drm.tmpl b/Documentation/DocBook/drm.tmpl
> index ec34b9becebd..f1884038b90f 100644
> --- a/Documentation/DocBook/drm.tmpl
> +++ b/Documentation/DocBook/drm.tmpl
> @@ -138,14 +138,10 @@
>  
>At the core of every DRM driver is a
> drm_driver structure. Drivers typically statically
> initialize a drm_driver structure, -  and then pass it to one of the
> drm_*_init() functions -  to register it with the
> DRM subsystem.
> -
> -
> -  Newer drivers that no longer require a
> drm_bus -  structure can alternatively use the
> low-level device initialization and -  registration functions such as
> drm_dev_alloc() and - 
> drm_dev_register() directly.
> +  and then pass it to drm_dev_alloc() to allocate
> a +  device instance. After the device instance is fully initialized it
> can be +  registered (which makes it accessible from userspace) using +
>  drm_dev_register().
>  
>  
>The drm_driver structure contains static
> @@ -296,83 +292,12 @@ char *date;
>
>  
>  
> -  Device Registration
> -  
> -A number of functions are provided to help with device
> registration. -The functions deal with PCI and platform devices,
> respectively. -  
> -!Edrivers/gpu/drm/drm_pci.c
> -!Edrivers/gpu/drm/drm_platform.c
> -  
> -New drivers that no longer rely on the services provided by the
> -drm_bus structure can call the low-level
> -device registration functions directly. The
> -drm_dev_alloc() function can be used to
> allocate -and initialize a new drm_device
> structure. -Drivers will typically want to perform some additional
> setup on this -structure, such as allocating driver-specific data
> and storing a -pointer to it in the DRM device's
> dev_private -field. Drivers should also
> set the device's unique name using the -   
> drm_dev_set_unique() function. After it has been -
>set up a device can be registered with the DRM subsystem by calling -   
> drm_dev_register(). This will cause the device to
> -be exposed to userspace and will call the driver's
> -.load() implementation. When a device is
> -removed, the DRM device can safely be unregistered and freed by
> calling -drm_dev_unregister() followed by a
> call to -drm_dev_unref().
> -  
> +  Device Instance and Driver Handling
> +!Pdrivers/gpu/drm/drm_drv.c driver instance overview
>  !Edrivers/gpu/drm/drm_drv.c
>  
>  
>Driver Load
> -  
> -The load method is the driver and device
> -initialization entry point. The method is responsible for
> allocating and -  initializing driver private data, performing resource
> allocation and -  mapping (e.g. acquiring
> -clocks, mapping registers or allocating command buffers),
> initializing -the memory manager ( linkend="drm-memory-management"/>), installing -the IRQ handler
> (), setting up -vertical
> blanking handling (), mode -  setting
> () and initial output
> - configuration ().
> -  
> -  
> -If compatibility is a concern (e.g. with drivers converted over
> from -User Mode Setting to Kernel Mode Setting), care must be taken
> to prevent -device initialization and control that is incompatible
> with currently -active userspace drivers. For instance, if user
> level mode setting -drivers are in use, it would be problematic to
> perform output discovery - configuration at load time.
> Likewise, if user-level drivers -unaware of memory management are
> in use, memory management 

[PATCH 2/2] drm/doc: Update docs about device instance setup

[Daniel Vetter]

On Mon, Aug 10, 2015 at 02:34:18PM +0200, Thierry Reding wrote:
> On Mon, Aug 10, 2015 at 02:07:49PM +0200, Daniel Vetter wrote:
> > On Mon, Aug 10, 2015 at 01:59:07PM +0200, Thierry Reding wrote:
> > > On Mon, Aug 10, 2015 at 11:55:38AM +0200, Daniel Vetter wrote:
> > > > ->load is depracated, bus functionst are deprecated and everyone
> > > > should use drm_dev_alloc
> > > 
> > > Why would you want to deprecated ->load()? Even if you use
> > > drm_dev_alloc() and drm_dev_register(), there's still use for ->load()
> > > because it gives you the subsystem-level initialization entry point.
> > 
> > ->load is called after the drm /dev node is registered and hence you can't
> > really do any driver setup in there without risking races. We paper over
> > that using drm_global_mutex, but that doesn't work for any other
> > driver/userspace interface like sysfs/debugfs because of deadlocks.
> > 
> > And we can't just reorder ->load to happen before the /dev nodes are
> > registered because a lot of drivers would fall over if we do that.
> > 
> > This is typical midlayer fail where the core calls into the driver instead
> > of the other way round.
> 
> Okay, but then if we're going to deprecate ->load(), I think we should
> also come up with an upgrade plan. As it is, this just says that users
> shouldn't do ->load(), but it doesn't tell them what to do instead.
> 
> Would the proper procedure be to fix drivers so that ->load() can be
> called between drm_dev_alloc() and drm_dev_register()? I suppose we
> could add some sort of (temporary) flag for drivers to signal this and
> then have drm_dev_register() call ->load() at the right time. If drivers
> don't support it, we can keep what we have.
> 
> That, of course, doesn't get rid of the midlayer, so perhaps a better
> way forward would be to tell driver writers that they should be doing
> subsystem-level setup between drm_dev_alloc() and drm_dev_register().

That's exactly what this patch tries to accomplish by updating the
kerneldoc and docbook. New sequence should be

device_probe_callback_or_whatever()
{
drm_dev_alloc();

... driver init code ...

drm_dev_register();

return 0;
}

Unfortunately the kerneldoc markdown isn't merged yet, otherwise I'd have
added this code snippet to the docs too.

Cheers, Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

[PATCH 2/2] drm/doc: Update docs about device instance setup

[Thierry Reding]

On Mon, Aug 10, 2015 at 02:07:49PM +0200, Daniel Vetter wrote:
> On Mon, Aug 10, 2015 at 01:59:07PM +0200, Thierry Reding wrote:
> > On Mon, Aug 10, 2015 at 11:55:38AM +0200, Daniel Vetter wrote:
> > > ->load is depracated, bus functionst are deprecated and everyone
> > > should use drm_dev_alloc
> > 
> > Why would you want to deprecated ->load()? Even if you use
> > drm_dev_alloc() and drm_dev_register(), there's still use for ->load()
> > because it gives you the subsystem-level initialization entry point.
> 
> ->load is called after the drm /dev node is registered and hence you can't
> really do any driver setup in there without risking races. We paper over
> that using drm_global_mutex, but that doesn't work for any other
> driver/userspace interface like sysfs/debugfs because of deadlocks.
> 
> And we can't just reorder ->load to happen before the /dev nodes are
> registered because a lot of drivers would fall over if we do that.
> 
> This is typical midlayer fail where the core calls into the driver instead
> of the other way round.

Okay, but then if we're going to deprecate ->load(), I think we should
also come up with an upgrade plan. As it is, this just says that users
shouldn't do ->load(), but it doesn't tell them what to do instead.

Would the proper procedure be to fix drivers so that ->load() can be
called between drm_dev_alloc() and drm_dev_register()? I suppose we
could add some sort of (temporary) flag for drivers to signal this and
then have drm_dev_register() call ->load() at the right time. If drivers
don't support it, we can keep what we have.

That, of course, doesn't get rid of the midlayer, so perhaps a better
way forward would be to tell driver writers that they should be doing
subsystem-level setup between drm_dev_alloc() and drm_dev_register().

Thierry
-- next part --
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: 

[PATCH 2/2] drm/doc: Update docs about device instance setup

[Daniel Vetter]

On Mon, Aug 10, 2015 at 01:59:07PM +0200, Thierry Reding wrote:
> On Mon, Aug 10, 2015 at 11:55:38AM +0200, Daniel Vetter wrote:
> > ->load is depracated, bus functionst are deprecated and everyone
> > should use drm_dev_alloc
> 
> Why would you want to deprecated ->load()? Even if you use
> drm_dev_alloc() and drm_dev_register(), there's still use for ->load()
> because it gives you the subsystem-level initialization entry point.

->load is called after the drm /dev node is registered and hence you can't
really do any driver setup in there without risking races. We paper over
that using drm_global_mutex, but that doesn't work for any other
driver/userspace interface like sysfs/debugfs because of deadlocks.

And we can't just reorder ->load to happen before the /dev nodes are
registered because a lot of drivers would fall over if we do that.

This is typical midlayer fail where the core calls into the driver instead
of the other way round.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

[PATCH 2/2] drm/doc: Update docs about device instance setup

[Thierry Reding]

On Mon, Aug 10, 2015 at 11:55:38AM +0200, Daniel Vetter wrote:
> ->load is depracated, bus functionst are deprecated and everyone
> should use drm_dev_alloc

Why would you want to deprecated ->load()? Even if you use
drm_dev_alloc() and drm_dev_register(), there's still use for ->load()
because it gives you the subsystem-level initialization entry point.

Thierry
-- next part --
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: 

[PATCH 2/2] drm/doc: Update docs about device instance setup

[Daniel Vetter]

->load is depracated, bus functionst are deprecated and everyone
should use drm_dev_alloc

So update the .tmpl (and pull a bunch of the overview docs into the
sourcecode to increase chances that it'll stay in sync in the future)
and add notes to functions which are deprecated. I didn't bother to
clean up and document the unload sequence similarly since that one is
still a bit a mess: drm_dev_unregister does way too much,
drm_unplug_dev does what _unregister should be doing but then has the
complication of promising something it doesn't actually do (it doesn't
unplug existing open fds for instance, only prevents new ones).

Motivated since I don't want to hunt every new driver for usage of
drm_platform_init any more ;-)

Signed-off-by: Daniel Vetter 
---
 Documentation/DocBook/drm.tmpl | 99 --
 drivers/gpu/drm/drm_drv.c  | 55 +--
 drivers/gpu/drm/drm_pci.c  | 11 +
 drivers/gpu/drm/drm_platform.c |  3 ++
 4 files changed, 83 insertions(+), 85 deletions(-)

diff --git a/Documentation/DocBook/drm.tmpl b/Documentation/DocBook/drm.tmpl
index ec34b9becebd..f1884038b90f 100644
--- a/Documentation/DocBook/drm.tmpl
+++ b/Documentation/DocBook/drm.tmpl
@@ -138,14 +138,10 @@
 
   At the core of every DRM driver is a drm_driver
   structure. Drivers typically statically initialize a drm_driver 
structure,
-  and then pass it to one of the drm_*_init() 
functions
-  to register it with the DRM subsystem.
-
-
-  Newer drivers that no longer require a drm_bus
-  structure can alternatively use the low-level device initialization and
-  registration functions such as drm_dev_alloc() and
-  drm_dev_register() directly.
+  and then pass it to drm_dev_alloc() to allocate a
+  device instance. After the device instance is fully initialized it can be
+  registered (which makes it accessible from userspace) using
+  drm_dev_register().
 
 
   The drm_driver structure contains static
@@ -296,83 +292,12 @@ char *date;
   
 
 
-  Device Registration
-  
-A number of functions are provided to help with device registration.
-The functions deal with PCI and platform devices, respectively.
-  
-!Edrivers/gpu/drm/drm_pci.c
-!Edrivers/gpu/drm/drm_platform.c
-  
-New drivers that no longer rely on the services provided by the
-drm_bus structure can call the low-level
-device registration functions directly. The
-drm_dev_alloc() function can be used to allocate
-and initialize a new drm_device structure.
-Drivers will typically want to perform some additional setup on this
-structure, such as allocating driver-specific data and storing a
-pointer to it in the DRM device's 
dev_private
-field. Drivers should also set the device's unique name using the
-drm_dev_set_unique() function. After it has been
-set up a device can be registered with the DRM subsystem by calling
-drm_dev_register(). This will cause the device to
-be exposed to userspace and will call the driver's
-.load() implementation. When a device is
-removed, the DRM device can safely be unregistered and freed by calling
-drm_dev_unregister() followed by a call to
-drm_dev_unref().
-  
+  Device Instance and Driver Handling
+!Pdrivers/gpu/drm/drm_drv.c driver instance overview
 !Edrivers/gpu/drm/drm_drv.c
 
 
   Driver Load
-  
-The load method is the driver and device
-initialization entry point. The method is responsible for allocating 
and
-   initializing driver private data, performing resource allocation and
-   mapping (e.g. acquiring
-clocks, mapping registers or allocating command buffers), initializing
-the memory manager (), 
installing
-the IRQ handler (), setting up
-vertical blanking handling (), mode
-   setting () and initial output
-   configuration ().
-  
-  
-If compatibility is a concern (e.g. with drivers converted over from
-User Mode Setting to Kernel Mode Setting), care must be taken to 
prevent
-device initialization and control that is incompatible with currently
-active userspace drivers. For instance, if user level mode setting
-drivers are in use, it would be problematic to perform output discovery
- configuration at load time. Likewise, if user-level drivers
-unaware of memory management are in use, memory management and command
-buffer setup may need to be omitted. These requirements are
-driver-specific, and care needs to be taken to keep both old and new
-applications and libraries working.
-  
-  int (*load) (struct drm_device *, unsigned long 
flags);
-  
-The method takes two arguments, a pointer to the newly created
-