[PATCH 1/2] drm/edid: Use ARRAY_SIZE in drm_add_modes_noedid

[PATCH 1/2] drm/edid: Use ARRAY_SIZE in drm_add_modes_noedid

[Daniel Vetter]

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)
-- 
2.5.0

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Daniel Vetter]

->load is depracated, bus functionst 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 ;-)

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
-        &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>
       <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)
-- 
2.5.0

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

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

[Thierry Reding]

[-- Attachment #1.1: Type: text/plain, Size: 376 bytes --]

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&register.

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

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

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

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

[Daniel Vetter]

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&register.
> 
> 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
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Thierry Reding]

[-- Attachment #1.1: Type: text/plain, Size: 1858 bytes --]

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&register.
> > 
> > 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

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

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

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

[Daniel Vetter]

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&register.
> > > 
> > > 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
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[David Herrmann]

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&register.
>> > >
>> > > 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
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[shuang.he]

Tested-By: Intel Graphics QA PRTS (Patch Regression Test System Contact: shuang.he@intel.com)
Task id: 7129
-------------------------------------Summary-------------------------------------
Platform          Delta          drm-intel-nightly          Series Applied
ILK                 -1              302/302              301/302
SNB                                  315/315              315/315
IVB                                  336/336              336/336
BYT                                  283/283              283/283
HSW                                  378/378              378/378
-------------------------------------Detailed-------------------------------------
Platform  Test                                drm-intel-nightly          Series Applied
*ILK  igt@kms_flip@wf_vblank-vs-modeset-interruptible      PASS(1)      DMESG_WARN(1)
Note: You need to pay more attention to line start with '*'
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Laurent Pinchart]

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&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 ;-)
> 
> 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 -        &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>
>        <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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Daniel Vetter]

->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@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
-        &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>
       <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)
-- 
2.5.1

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

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

[Laurent Pinchart]

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@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 -        &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

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Daniel Vetter]

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&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@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 -        &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 ?

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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/2] drm/edid: Use ARRAY_SIZE in drm_add_modes_noedid

[Thierry Reding]

[-- Attachment #1.1: Type: text/plain, Size: 862 bytes --]

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>

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

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

Re: [PATCH 1/2] drm/edid: Use ARRAY_SIZE in drm_add_modes_noedid

[Daniel Vetter]

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
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx