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@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 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
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.
> */
--
Regards,
Laurent Pinchart
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/dri-devel
