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®ister. > > 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@xxxxxxxxxxxxxxxx> > Cc: David Herrmann <dh.herrmann@xxxxxxxxx> > Acked-by: David Herrmann <dh.herrmann@xxxxxxxxx> > Signed-off-by: Daniel Vetter <daniel.vetter@xxxxxxxxx> > --- > 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 - & 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) -- Regards, Laurent Pinchart _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/intel-gfx