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

Hi Daniel,

Thank you for the patch.

On Monday 28 September 2015 21:46:35 Daniel Vetter wrote:

->load is deprecated, bus functions are deprecated and everyone should use drm_dev_alloc&register.

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 ;-)

v2: Reword the deprecation note for ->load a bit, using Laurent's suggestion as an example (but making the wording a bit stronger even). Fix spelling in commit message.

Cc: Laurent Pinchart laurent.pinchart@ideasonboard.com Cc: David Herrmann dh.herrmann@gmail.com Acked-by: David Herrmann dh.herrmann@gmail.com Signed-off-by: Daniel Vetter daniel.vetter@intel.com

---

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 5395cde6905e..6d24ebb97cda 100644 --- a/Documentation/DocBook/drm.tmpl +++ b/Documentation/DocBook/drm.tmpl @@ -138,14 +138,10 @@ <para> At the core of every DRM driver is a <structname>drm_driver</structname> structure. Drivers typically statically initialize a drm_driver structure, - and then pass it to one of the <function>drm_*_init()</function> functions - to register it with the DRM subsystem.

• </para>

• <para>

•  Newer drivers that no longer require a
  

<structname>drm_bus</structname> - structure can alternatively use the low-level device initialization and - registration functions such as <function>drm_dev_alloc()</function> and - <function>drm_dev_register()</function> directly.

•  and then pass it to <function>drm_dev_alloc()</function> to allocate
  

a + device instance. After the device instance is fully initialized it can be + registered (which makes it accessible from userspace) using + <function>drm_dev_register()</function>. </para> <para> The <structname>drm_driver</structname> structure contains static @@ -296,83 +292,12 @@ char *date;</synopsis> </sect3> </sect2> <sect2>

•  <title>Device Registration</title>
  

•  <para>
  

•    A number of functions are provided to help with device
  

registration. - The functions deal with PCI and platform devices, respectively. - </para> -!Edrivers/gpu/drm/drm_pci.c -!Edrivers/gpu/drm/drm_platform.c

•  <para>
  

•    New drivers that no longer rely on the services provided by the
  

•    <structname>drm_bus</structname> structure can call the low-level
  

•    device registration functions directly. The
  

•    <function>drm_dev_alloc()</function> function can be used to
  

allocate - and initialize a new <structname>drm_device</structname> 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 <structfield>dev_private</structfield> - field. Drivers should also set the device's unique name using the - <function>drm_dev_set_unique()</function> function. After it has been - set up a device can be registered with the DRM subsystem by calling - <function>drm_dev_register()</function>. This will cause the device to

•    be exposed to userspace and will call the driver's
  

•    <structfield>.load()</structfield> implementation. When a device is
  

•    removed, the DRM device can safely be unregistered and freed by
  

calling - <function>drm_dev_unregister()</function> followed by a call to - <function>drm_dev_unref()</function>.

•  </para>
  

•  <title>Device Instance and Driver Handling</title>
  

+!Pdrivers/gpu/drm/drm_drv.c driver instance overview !Edrivers/gpu/drm/drm_drv.c </sect2> <sect2> <title>Driver Load</title>

•  <para>
  

•    The <methodname>load</methodname> 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 (<xref linkend="drm-memory-management"/>), installing - the IRQ handler (<xref linkend="drm-irq-registration"/>), setting up - vertical blanking handling (<xref linkend="drm-vertical-blank"/>), mode - setting (<xref linkend="drm-mode-setting"/>) and initial output

• configuration (<xref linkend="drm-kms-init"/>).

•  </para>
  

•  <note><para>
  

•    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 - &amp; 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.

•  </para></note>
  

•  <synopsis>int (*load) (struct drm_device *, unsigned long
  

flags);</synopsis> - <para>

•    The method takes two arguments, a pointer to the newly created
  

• <structname>drm_device</structname> and flags. The flags are used to
• pass the <structfield>driver_data</structfield> field of the device id
• corresponding to the device passed to <function>drm_*_init()</function>.
• Only PCI devices currently use this, USB and platform DRM drivers have
• their <methodname>load</methodname> method called with flags to 0.

•  </para>
  

•  <sect3>
  

•    <title>Driver Private Data</title>
  

•    <para>
  

•      The driver private hangs off the main
  

•      <structname>drm_device</structname> structure and can be used for
  

•      tracking various device-specific bits of information, like
  

register

•      offsets, command buffer status, register state for
  

suspend/resume, etc.

•      At load time, a driver may simply allocate one and set
  

<structname>drm_device</structname>.<structfield>dev_priv</structfield> - appropriately; it should be freed and

<structname>drm_device</structname>.<structfield>dev_priv</structfield> - set to NULL when the driver is unloaded.

•    </para>
  

•  </sect3>
  

Shouldn't we keep the "Driver Private Data" section ?

   <sect3 id="drm-irq-registration">
     <title>IRQ Registration</title>
     <para>

@@ -465,6 +390,18 @@ char *date;</synopsis> </para> </sect3> </sect2>

• <sect2>

•  <title>Bus-specific Device Registration and PCI Support</title>
  

•  <para>
  

•    A number of functions are provided to help with device
  

registration. + The functions deal with PCI and platform devices respectively and are + only provided for historical reasons. These are all deprecated and + shouldn't be used in new drivers. Besides that there's a few

• helpers for pci drivers.

•  </para>
  

+!Edrivers/gpu/drm/drm_pci.c +!Edrivers/gpu/drm/drm_platform.c

• </sect2> </sect1>

  <!-- Internals: memory management -->

diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c index 9ad823fcde87..031c69b71547 100644 --- a/drivers/gpu/drm/drm_drv.c +++ b/drivers/gpu/drm/drm_drv.c @@ -397,15 +397,51 @@ void drm_minor_release(struct drm_minor *minor) }

/**

• • DOC: driver instance overview
• • A device instance for a drm driver is represented by struct &drm_device.

This

• • is allocated with drm_dev_alloc(), usually from bus-specific ->probe()
• • callbacks implemented by the driver. The driver then needs to initialize

all

• • the various subsystems for the drm device like memory management, vblank
• • handling, modesetting support and intial output configuration plus

obviously

• • initialize all the corresponding hardware bits. An important part of

this is

• • also calling drm_dev_set_unique() to set the userspace-visible unique

name of

• • this device instance. Finally when everything is up and running and

ready for

• • userspace the device instance can be published using drm_dev_register().
• • There is also deprecated support for initalizing device instances using
• • bus-specific helpers and the ->load() callback. But due to
• • backwards-compatibility needs the device instance has to be published

too

• • early, which requires unpretty global locking to make safe and is

therefore

• • only support for existing drivers not yet converted to the new scheme.
• • When cleaning up a device instance everything needs to be done in

revers:

s/revers/reverse/

• • First unpublish the device instance with drm_dev_unregister(). Then

clean up

• • any other resources allocated at device initialization and drop the

driver's

• • reference to &drm_device using drm_dev_unref().
• • Note that the lifetime rules for &drm_device instance has still a lot of

s/has/have/

• • historical baggage. Hence use the reference counting provided by
• • drm_dev_ref() and drm_dev_unref() only carefully.
• • Also note that embedding of &drm_device is currently not (yet) supported

(but

• • it would be easy to add). Drivers can store driver-private data in the
• • dev_priv field of &drm_device.
• */

+/**

• drm_put_dev - Unregister and release a DRM device
• @dev: DRM device
• Called at module unload time or when a PCI device is unplugged.

• • Use of this function is discouraged. It will eventually go away

completely.

• • Please use drm_dev_unregister() and drm_dev_unref() explicitly instead.
• • Cleans up all DRM device, calling drm_lastclose().

• • Note: Use of this function is deprecated. It will eventually go away
• • completely. Please use drm_dev_unregister() and drm_dev_unref()

explicitly

• • instead to make sure that the device isn't userspace accessible any more
• • while teardown is in progress, to make sure userspace can't access an

I'd s/to make sure/ensuring that/ in order to avoid the double "to make sure" in the same sentence.

• • inconsisten state.

s/inconsisten/inconsistent/

*/ void drm_put_dev(struct drm_device *dev) { @@ -518,7 +554,9 @@ static void drm_fs_inode_free(struct inode *inode)

• Allocate and initialize a new DRM device. No device registration is

done. * Call drm_dev_register() to advertice the device to user space and register it - * with other core subsystems.

• • with other core subsystems. This should be done last in the device
• • initialization sequence to make sure userspace can't access an

inconsistent + * state.

• The initial ref-count of the object is 1. Use drm_dev_ref() and
• drm_dev_unref() to take and drop further ref-counts.

@@ -673,6 +711,12 @@ EXPORT_SYMBOL(drm_dev_unref);

• Never call this twice on any device!

• • NOTE: To ensure backward compatibility with existing drivers method this
• • function calls the ->load() method after registering the device nodes,
• • creating race conditions. Usage of the ->load() methods is therefore +

• deprecated, drivers must perform all initialization before calling + *

drm_dev_register().

• • RETURNS:
  • 0 on success, negative error code on failure.
  */

@@ -720,6 +764,9 @@ EXPORT_SYMBOL(drm_dev_register);

• Unregister the DRM device from the system. This does the reverse of
• drm_dev_register() but does not deallocate the device. The caller must

call * drm_dev_unref() to drop their final reference.

• • This should be called first in the device teardown code to make sure
• • userspace can't access the device instance any more.
  */

void drm_dev_unregister(struct drm_device *dev) { diff --git a/drivers/gpu/drm/drm_pci.c b/drivers/gpu/drm/drm_pci.c index 1b1bd42b0368..fcd2a86acd2c 100644 --- a/drivers/gpu/drm/drm_pci.c +++ b/drivers/gpu/drm/drm_pci.c @@ -266,6 +266,9 @@ void drm_pci_agp_destroy(struct drm_device *dev)

• then register the character device and inter module information.
• Try and register, if we fail to register, backout previous work.

• • NOTE: This function is deprecated, please use drm_dev_alloc() and
• • drm_dev_register() instead and remove your ->load() callback.
• • Return: 0 on success or a negative error code on failure.
  */

int drm_get_pci_dev(struct pci_dev *pdev, const struct pci_device_id *ent, @@ -326,6 +329,10 @@ EXPORT_SYMBOL(drm_get_pci_dev);

• Initializes a drm_device structures, registering the stubs and

initializing * the AGP device.

• • NOTE: This function is deprecated. Modern modesetting drm drivers should

use + * pci_register_driver() directly, this function only provides shadow-binding + * support for old legacy drivers on top of that core pci function. + *

• Return: 0 on success or a negative error code on failure.

*/ int drm_pci_init(struct drm_driver *driver, struct pci_driver *pdriver) @@ -435,6 +442,10 @@ EXPORT_SYMBOL(drm_pci_init);

• Unregisters one or more devices matched by a PCI driver from the DRM
• subsystem.

• • NOTE: This function is deprecated. Modern modesetting drm drivers should

use + * pci_unregister_driver() directly, this function only provides shadow-binding + * support for old legacy drivers on top of that core pci function. */ void drm_pci_exit(struct drm_driver *driver, struct pci_driver *pdriver) { diff --git a/drivers/gpu/drm/drm_platform.c b/drivers/gpu/drm/drm_platform.c index 5314c9d5fef4..644169e1a029 100644 --- a/drivers/gpu/drm/drm_platform.c +++ b/drivers/gpu/drm/drm_platform.c @@ -95,6 +95,9 @@ EXPORT_SYMBOL(drm_platform_set_busid);

• subsystem, initializing a drm_device structure and calling the driver's
• .load() function.

• • NOTE: This function is deprecated, please use drm_dev_alloc() and
• • drm_dev_register() instead and remove your ->load() callback.
• • Return: 0 on success or a negative error code on failure.
  */

int drm_platform_init(struct drm_driver *driver, struct platform_device *platform_device)