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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi Daniel,

Thank you for the patch.

On Monday 28 September 2015 21:46:35 Daniel Vetter wrote:
> ->load is deprecated, bus functions are deprecated and everyone
> should use drm_dev_alloc&register.
> 
> So update the .tmpl (and pull a bunch of the overview docs into the
> sourcecode to increase chances that it'll stay in sync in the future)
> and add notes to functions which are deprecated. I didn't bother to
> clean up and document the unload sequence similarly since that one is
> still a bit a mess: drm_dev_unregister does way too much,
> drm_unplug_dev does what _unregister should be doing but then has the
> complication of promising something it doesn't actually do (it doesn't
> unplug existing open fds for instance, only prevents new ones).
> 
> Motivated since I don't want to hunt every new driver for usage of
> drm_platform_init any more ;-)
> 
> v2: Reword the deprecation note for ->load a bit, using Laurent's
> suggestion as an example (but making the wording a bit stronger even).
> Fix spelling in commit message.
> 
> Cc: Laurent Pinchart <laurent.pinchart@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 -        &amp; configuration at load time.
> Likewise, if user-level drivers -        unaware of memory management are
> in use, memory management and command -        buffer setup may need to be
> omitted. These requirements are -        driver-specific, and care needs to
> be taken to keep both old and new -        applications and libraries
> working.
> -      </para></note>
> -      <synopsis>int (*load) (struct drm_device *, unsigned long
> flags);</synopsis> -      <para>
> -        The method takes two arguments, a pointer to the newly created
> -	<structname>drm_device</structname> and flags. The flags are used to
> -	pass the <structfield>driver_data</structfield> field of the device id
> -	corresponding to the device passed to <function>drm_*_init()</function>.
> -	Only PCI devices currently use this, USB and platform DRM drivers have
> -	their <methodname>load</methodname> method called with flags to 0.
> -      </para>
> -      <sect3>
> -        <title>Driver Private Data</title>
> -        <para>
> -          The driver private hangs off the main
> -          <structname>drm_device</structname> structure and can be used for
> -          tracking various device-specific bits of information, like
> register
> -          offsets, command buffer status, register state for
> suspend/resume, etc.
> -          At load time, a driver may simply allocate one and set
> -         
> <structname>drm_device</structname>.<structfield>dev_priv</structfield> -  
>        appropriately; it should be freed and
> -         
> <structname>drm_device</structname>.<structfield>dev_priv</structfield> -  
>        set to NULL when the driver is unloaded.
> -        </para>
> -      </sect3>

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

>        <sect3 id="drm-irq-registration">
>          <title>IRQ Registration</title>
>          <para>
> @@ -465,6 +390,18 @@ char *date;</synopsis>
>          </para>
>        </sect3>
>      </sect2>
> +    <sect2>
> +      <title>Bus-specific Device Registration and PCI Support</title>
> +      <para>
> +        A number of functions are provided to help with device
> registration. +	The functions deal with PCI and platform devices
> respectively and are +	only provided for historical reasons. These are all
> deprecated and +	shouldn't be used in new drivers. Besides that there's a
> few
> +	helpers for pci drivers.
> +      </para>
> +!Edrivers/gpu/drm/drm_pci.c
> +!Edrivers/gpu/drm/drm_platform.c
> +    </sect2>
>    </sect1>
> 
>    <!-- Internals: memory management -->
> diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
> index 9ad823fcde87..031c69b71547 100644
> --- a/drivers/gpu/drm/drm_drv.c
> +++ b/drivers/gpu/drm/drm_drv.c
> @@ -397,15 +397,51 @@ void drm_minor_release(struct drm_minor *minor)
>  }
> 
>  /**
> + * DOC: driver instance overview
> + *
> + * A device instance for a drm driver is represented by struct &drm_device.
> This
> + * is allocated with drm_dev_alloc(), usually from bus-specific ->probe()
> + * callbacks implemented by the driver. The driver then needs to initialize
> all
> + * the various subsystems for the drm device like memory management, vblank
> + * handling, modesetting support and intial output configuration plus
> obviously
> + * initialize all the corresponding hardware bits. An important part of
> this is
> + * also calling drm_dev_set_unique() to set the userspace-visible unique
> name of
> + * this device instance. Finally when everything is up and running and
> ready for
> + * userspace the device instance can be published using drm_dev_register().
> + *
> + * There is also deprecated support for initalizing device instances using
> + * bus-specific helpers and the ->load() callback. But due to
> + * backwards-compatibility needs the device instance has to be published
> too
> + * early, which requires unpretty global locking to make safe and is
> therefore
> + * only support for existing drivers not yet converted to the new scheme.
> + *
> + * When cleaning up a device instance everything needs to be done in
> revers:

s/revers/reverse/

> + * First unpublish the device instance with drm_dev_unregister(). Then
> clean up
> + * any other resources allocated at device initialization and drop the
> driver's
> + * reference to &drm_device using drm_dev_unref().
> + *
> + * Note that the lifetime rules for &drm_device instance has still a lot of

s/has/have/

> + * historical baggage. Hence use the reference counting provided by
> + * drm_dev_ref() and drm_dev_unref() only carefully.
> + *
> + * Also note that embedding of &drm_device is currently not (yet) supported
> (but
> + * it would be easy to add). Drivers can store driver-private data in the
> + * dev_priv field of &drm_device.
> + */
> +
> +/**
>   * drm_put_dev - Unregister and release a DRM device
>   * @dev: DRM device
>   *
>   * Called at module unload time or when a PCI device is unplugged.
>   *
> - * Use of this function is discouraged. It will eventually go away
> completely.
> - * Please use drm_dev_unregister() and drm_dev_unref() explicitly instead.
> - *
>   * Cleans up all DRM device, calling drm_lastclose().
> + *
> + * Note: Use of this function is deprecated. It will eventually go away
> + * completely.  Please use drm_dev_unregister() and drm_dev_unref()
> explicitly
> + * instead to make sure that the device isn't userspace accessible any more
> + * while teardown is in progress, to make sure userspace can't access an

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

> + * inconsisten state.

s/inconsisten/inconsistent/

>   */
>  void drm_put_dev(struct drm_device *dev)
>  {
> @@ -518,7 +554,9 @@ static void drm_fs_inode_free(struct inode *inode)
>   *
>   * Allocate and initialize a new DRM device. No device registration is
> done. * Call drm_dev_register() to advertice the device to user space and
> register it - * with other core subsystems.
> + * with other core subsystems. This should be done last in the device
> + * initialization sequence to make sure userspace can't access an
> inconsistent + * state.
>   *
>   * The initial ref-count of the object is 1. Use drm_dev_ref() and
>   * drm_dev_unref() to take and drop further ref-counts.
> @@ -673,6 +711,12 @@ EXPORT_SYMBOL(drm_dev_unref);
>   *
>   * Never call this twice on any device!
>   *
> + * NOTE: To ensure backward compatibility with existing drivers method this
> + * function calls the ->load() method after registering the device nodes,
> + * creating race conditions. Usage of the ->load() methods is therefore +
> * deprecated, drivers must perform all initialization before calling + *
> drm_dev_register().
> + *
>   * RETURNS:
>   * 0 on success, negative error code on failure.
>   */
> @@ -720,6 +764,9 @@ EXPORT_SYMBOL(drm_dev_register);
>   * Unregister the DRM device from the system. This does the reverse of
>   * drm_dev_register() but does not deallocate the device. The caller must
> call * drm_dev_unref() to drop their final reference.
> + *
> + * This should be called first in the device teardown code to make sure
> + * userspace can't access the device instance any more.
>   */
>  void drm_dev_unregister(struct drm_device *dev)
>  {
> diff --git a/drivers/gpu/drm/drm_pci.c b/drivers/gpu/drm/drm_pci.c
> index 1b1bd42b0368..fcd2a86acd2c 100644
> --- a/drivers/gpu/drm/drm_pci.c
> +++ b/drivers/gpu/drm/drm_pci.c
> @@ -266,6 +266,9 @@ void drm_pci_agp_destroy(struct drm_device *dev)
>   * then register the character device and inter module information.
>   * Try and register, if we fail to register, backout previous work.
>   *
> + * NOTE: This function is deprecated, please use drm_dev_alloc() and
> + * drm_dev_register() instead and remove your ->load() callback.
> + *
>   * Return: 0 on success or a negative error code on failure.
>   */
>  int drm_get_pci_dev(struct pci_dev *pdev, const struct pci_device_id *ent,
> @@ -326,6 +329,10 @@ EXPORT_SYMBOL(drm_get_pci_dev);
>   * Initializes a drm_device structures, registering the stubs and
> initializing * the AGP device.
>   *
> + * NOTE: This function is deprecated. Modern modesetting drm drivers should
> use + * pci_register_driver() directly, this function only provides
> shadow-binding + * support for old legacy drivers on top of that core pci
> function. + *
>   * Return: 0 on success or a negative error code on failure.
>   */
>  int drm_pci_init(struct drm_driver *driver, struct pci_driver *pdriver)
> @@ -435,6 +442,10 @@ EXPORT_SYMBOL(drm_pci_init);
>   *
>   * Unregisters one or more devices matched by a PCI driver from the DRM
>   * subsystem.
> + *
> + * NOTE: This function is deprecated. Modern modesetting drm drivers should
> use + * pci_unregister_driver() directly, this function only provides
> shadow-binding + * support for old legacy drivers on top of that core pci
> function. */
>  void drm_pci_exit(struct drm_driver *driver, struct pci_driver *pdriver)
>  {
> diff --git a/drivers/gpu/drm/drm_platform.c b/drivers/gpu/drm/drm_platform.c
> index 5314c9d5fef4..644169e1a029 100644
> --- a/drivers/gpu/drm/drm_platform.c
> +++ b/drivers/gpu/drm/drm_platform.c
> @@ -95,6 +95,9 @@ EXPORT_SYMBOL(drm_platform_set_busid);
>   * subsystem, initializing a drm_device structure and calling the driver's
>   * .load() function.
>   *
> + * NOTE: This function is deprecated, please use drm_dev_alloc() and
> + * drm_dev_register() instead and remove your ->load() callback.
> + *
>   * Return: 0 on success or a negative error code on failure.
>   */
>  int drm_platform_init(struct drm_driver *driver, struct platform_device
> *platform_device)

-- 
Regards,

Laurent Pinchart

_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/dri-devel




[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux