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@xxxxxxxxxxxxxxxx>
> > Cc: David Herrmann <dh.herrmann@xxxxxxxxx>
> > Acked-by: David Herrmann <dh.herrmann@xxxxxxxxx>
> > Signed-off-by: Daniel Vetter <daniel.vetter@xxxxxxxxx>
> > ---
> > Documentation/DocBook/drm.tmpl | 99 +++++++--------------------------------
> > drivers/gpu/drm/drm_drv.c | 55 +++++++++++++++++++++--
> > drivers/gpu/drm/drm_pci.c | 11 +++++
> > drivers/gpu/drm/drm_platform.c | 3 ++
> > 4 files changed, 83 insertions(+), 85 deletions(-)
> >
> > diff --git a/Documentation/DocBook/drm.tmpl b/Documentation/DocBook/drm.tmpl
> > index 5395cde6905e..6d24ebb97cda 100644
> > --- a/Documentation/DocBook/drm.tmpl
> > +++ b/Documentation/DocBook/drm.tmpl
> > @@ -138,14 +138,10 @@
> > <para>
> > At the core of every DRM driver is a
> > <structname>drm_driver</structname> structure. Drivers typically statically
> > initialize a drm_driver structure, - and then pass it to one of the
> > <function>drm_*_init()</function> functions - to register it with the
> > DRM subsystem.
> > - </para>
> > - <para>
> > - Newer drivers that no longer require a
> > <structname>drm_bus</structname> - structure can alternatively use the
> > low-level device initialization and - registration functions such as
> > <function>drm_dev_alloc()</function> and -
> > <function>drm_dev_register()</function> directly.
> > + and then pass it to <function>drm_dev_alloc()</function> to allocate
> > a + device instance. After the device instance is fully initialized it
> > can be + registered (which makes it accessible from userspace) using +
> > <function>drm_dev_register()</function>.
> > </para>
> > <para>
> > The <structname>drm_driver</structname> structure contains static
> > @@ -296,83 +292,12 @@ char *date;</synopsis>
> > </sect3>
> > </sect2>
> > <sect2>
> > - <title>Device Registration</title>
> > - <para>
> > - A number of functions are provided to help with device
> > registration. - The functions deal with PCI and platform devices,
> > respectively. - </para>
> > -!Edrivers/gpu/drm/drm_pci.c
> > -!Edrivers/gpu/drm/drm_platform.c
> > - <para>
> > - New drivers that no longer rely on the services provided by the
> > - <structname>drm_bus</structname> structure can call the low-level
> > - device registration functions directly. The
> > - <function>drm_dev_alloc()</function> function can be used to
> > allocate - and initialize a new <structname>drm_device</structname>
> > structure. - Drivers will typically want to perform some additional
> > setup on this - structure, such as allocating driver-specific data
> > and storing a - pointer to it in the DRM device's
> > <structfield>dev_private</structfield> - field. Drivers should also
> > set the device's unique name using the -
> > <function>drm_dev_set_unique()</function> function. After it has been -
> > set up a device can be registered with the DRM subsystem by calling -
> > <function>drm_dev_register()</function>. This will cause the device to
> > - be exposed to userspace and will call the driver's
> > - <structfield>.load()</structfield> implementation. When a device is
> > - removed, the DRM device can safely be unregistered and freed by
> > calling - <function>drm_dev_unregister()</function> followed by a
> > call to - <function>drm_dev_unref()</function>.
> > - </para>
> > + <title>Device Instance and Driver Handling</title>
> > +!Pdrivers/gpu/drm/drm_drv.c driver instance overview
> > !Edrivers/gpu/drm/drm_drv.c
> > </sect2>
> > <sect2>
> > <title>Driver Load</title>
> > - <para>
> > - The <methodname>load</methodname> method is the driver and device
> > - initialization entry point. The method is responsible for
> > allocating and - initializing driver private data, performing resource
> > allocation and - mapping (e.g. acquiring
> > - clocks, mapping registers or allocating command buffers),
> > initializing - the memory manager (<xref
> > linkend="drm-memory-management"/>), installing - the IRQ handler
> > (<xref linkend="drm-irq-registration"/>), setting up - vertical
> > blanking handling (<xref linkend="drm-vertical-blank"/>), mode - setting
> > (<xref linkend="drm-mode-setting"/>) and initial output
> > - configuration (<xref linkend="drm-kms-init"/>).
> > - </para>
> > - <note><para>
> > - If compatibility is a concern (e.g. with drivers converted over
> > from - User Mode Setting to Kernel Mode Setting), care must be taken
> > to prevent - device initialization and control that is incompatible
> > with currently - active userspace drivers. For instance, if user
> > level mode setting - drivers are in use, it would be problematic to
> > perform output discovery - & configuration at load time.
> > Likewise, if user-level drivers - unaware of memory management are
> > in use, memory management and command - buffer setup may need to be
> > omitted. These requirements are - driver-specific, and care needs to
> > be taken to keep both old and new - applications and libraries
> > working.
> > - </para></note>
> > - <synopsis>int (*load) (struct drm_device *, unsigned long
> > flags);</synopsis> - <para>
> > - The method takes two arguments, a pointer to the newly created
> > - <structname>drm_device</structname> and flags. The flags are used to
> > - pass the <structfield>driver_data</structfield> field of the device id
> > - corresponding to the device passed to <function>drm_*_init()</function>.
> > - Only PCI devices currently use this, USB and platform DRM drivers have
> > - their <methodname>load</methodname> method called with flags to 0.
> > - </para>
> > - <sect3>
> > - <title>Driver Private Data</title>
> > - <para>
> > - The driver private hangs off the main
> > - <structname>drm_device</structname> structure and can be used for
> > - tracking various device-specific bits of information, like
> > register
> > - offsets, command buffer status, register state for
> > suspend/resume, etc.
> > - At load time, a driver may simply allocate one and set
> > -
> > <structname>drm_device</structname>.<structfield>dev_priv</structfield> -
> > appropriately; it should be freed and
> > -
> > <structname>drm_device</structname>.<structfield>dev_priv</structfield> -
> > set to NULL when the driver is unloaded.
> > - </para>
> > - </sect3>
>
> Shouldn't we keep the "Driver Private Data" section ?
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 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
>
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/dri-devel
