Spotted while reading code for random reasons.
Signed-off-by: Daniel Vetter daniel.vetter@intel.com --- drivers/gpu/drm/drm_edid.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c index 4a403eb90ded..4780b1924bef 100644 --- a/drivers/gpu/drm/drm_edid.c +++ b/drivers/gpu/drm/drm_edid.c @@ -3810,7 +3810,7 @@ int drm_add_modes_noedid(struct drm_connector *connector, struct drm_display_mode *mode; struct drm_device *dev = connector->dev;
- count = sizeof(drm_dmt_modes) / sizeof(struct drm_display_mode); + count = ARRAY_SIZE(drm_dmt_modes); if (hdisplay < 0) hdisplay = 0; if (vdisplay < 0)
->load is depracated, bus functionst 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 ;-)
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 ec34b9becebd..f1884038b90f 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> <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 53d09a19f7e1..b7c9b71048d7 100644 --- a/drivers/gpu/drm/drm_drv.c +++ b/drivers/gpu/drm/drm_drv.c @@ -398,15 +398,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: + * 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 + * 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 + * inconsisten state. */ void drm_put_dev(struct drm_device *dev) { @@ -519,7 +555,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. @@ -672,6 +710,12 @@ EXPORT_SYMBOL(drm_dev_unref); * * Never call this twice on any device! * + * NOTE: This function still calls the ->load() callback to provide backwards + * compatibility with existing drivers. Using that callback is depracated and + * when using drm_dev_alloc() and drm_dev_regiter() directly actually unsafe: + * Becuase of backwards compatibility needs ->load() is only called _after_ the + * device instance is already registered. + * * RETURNS: * 0 on success, negative error code on failure. */ @@ -719,6 +763,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)
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®ister.
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
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®ister.
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
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®ister.
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
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®ister.
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
Hi
On Mon, Aug 10, 2015 at 4:30 PM, Daniel Vetter daniel@ffwll.ch 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®ister.
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 dh.herrmann@gmail.com
Thanks David
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®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 ;-)
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 ec34b9becebd..f1884038b90f 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 allocatea + 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 deviceregistration. - 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-leveldevice registration functions directly. The<function>drm_dev_alloc()</function> function can be used toallocate - 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 isremoved, the DRM device can safely be unregistered and freed bycalling - <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 deviceinitialization entry point. The method is responsible forallocating 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 overfrom - 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 longflags);</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 fortracking various device-specific bits of information, likeregister - 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> <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 deviceregistration. + 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 53d09a19f7e1..b7c9b71048d7 100644 --- a/drivers/gpu/drm/drm_drv.c +++ b/drivers/gpu/drm/drm_drv.c @@ -398,15 +398,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: + * 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
- 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 + * inconsisten state. */ void drm_put_dev(struct drm_device *dev) { @@ -519,7 +555,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.
@@ -672,6 +710,12 @@ EXPORT_SYMBOL(drm_dev_unref);
- Never call this twice on any device!
- NOTE: This function still calls the ->load() callback to provide
backwards
- compatibility with existing drivers. Using that callback is depracated
s/depracated/deprecated/
and
- when using drm_dev_alloc() and drm_dev_regiter() directly actually
s/regiter/register/
unsafe:
- Becuase of backwards compatibility needs ->load() is only called _after_
the
s/Becuase/Because/
- device instance is already registered.
This could be stated more clearly. I'd write
NOTE: To ensure backward compatibility with existing drivers that still rely on the deprecated .load() method this function calls the .load() method after registering the device nodes, creating race conditions. Usage of the .load() (and .unload()) methods is deprecated, drivers should perform device initialization before calling drm_dev_register().
- RETURNS:
- 0 on success, negative error code on failure.
*/
->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@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 - & 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> <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: + * 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 + * 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 + * inconsisten state. */ 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)
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@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 allocatea + 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 deviceregistration. - 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-leveldevice registration functions directly. The<function>drm_dev_alloc()</function> function can be used toallocate - 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 isremoved, the DRM device can safely be unregistered and freed bycalling - <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 deviceinitialization entry point. The method is responsible forallocating 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 overfrom - 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 longflags);</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 fortracking various device-specific bits of information, likeregister
offsets, command buffer status, register state forsuspend/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 deviceregistration. + 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)
On Tue, Sep 29, 2015 at 04:28:15PM +0300, Laurent Pinchart wrote:
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@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 allocatea + 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 deviceregistration. - 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-leveldevice registration functions directly. The<function>drm_dev_alloc()</function> function can be used toallocate - 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 isremoved, the DRM device can safely be unregistered and freed bycalling - <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 deviceinitialization entry point. The method is responsible forallocating 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 overfrom - 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 longflags);</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 fortracking various device-specific bits of information, likeregister
offsets, command buffer status, register state forsuspend/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 ?
It's moved into just one paragraf in the new DOC section which explains that subclassing is unfortunately not yet supported (we need to demidlayer the unload sequence first), but drivers can stuff private data into dev_priv.
I also applied all your spelling fixes below and applied the patch to drm-misc.
Thanks, Daniel
<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 deviceregistration. + 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
On Mon, Aug 10, 2015 at 11:55:37AM +0200, Daniel Vetter wrote:
Spotted while reading code for random reasons.
Signed-off-by: Daniel Vetter daniel.vetter@intel.com
drivers/gpu/drm/drm_edid.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c index 4a403eb90ded..4780b1924bef 100644 --- a/drivers/gpu/drm/drm_edid.c +++ b/drivers/gpu/drm/drm_edid.c @@ -3810,7 +3810,7 @@ int drm_add_modes_noedid(struct drm_connector *connector, struct drm_display_mode *mode; struct drm_device *dev = connector->dev;
- count = sizeof(drm_dmt_modes) / sizeof(struct drm_display_mode);
- count = ARRAY_SIZE(drm_dmt_modes); if (hdisplay < 0) hdisplay = 0; if (vdisplay < 0)
Reviewed-by: Thierry Reding treding@nvidia.com
On Mon, Aug 10, 2015 at 01:57:21PM +0200, Thierry Reding wrote:
On Mon, Aug 10, 2015 at 11:55:37AM +0200, Daniel Vetter wrote:
Spotted while reading code for random reasons.
Signed-off-by: Daniel Vetter daniel.vetter@intel.com
drivers/gpu/drm/drm_edid.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c index 4a403eb90ded..4780b1924bef 100644 --- a/drivers/gpu/drm/drm_edid.c +++ b/drivers/gpu/drm/drm_edid.c @@ -3810,7 +3810,7 @@ int drm_add_modes_noedid(struct drm_connector *connector, struct drm_display_mode *mode; struct drm_device *dev = connector->dev;
- count = sizeof(drm_dmt_modes) / sizeof(struct drm_display_mode);
- count = ARRAY_SIZE(drm_dmt_modes); if (hdisplay < 0) hdisplay = 0; if (vdisplay < 0)
Reviewed-by: Thierry Reding treding@nvidia.com
Thanks for the review, applied to drm-misc. -Danie
dri-devel@lists.freedesktop.org
-
Daniel Vetter -
Daniel Vetter -
David Herrmann -
Laurent Pinchart -
Thierry Reding