On 2020-09-03 10:26 a.m., Daniel Vetter wrote:

On Wed, Sep 02, 2020 at 09:26:27AM +0200, Daniel Vetter wrote:

...

Following functions are only used internally, not by drivers:

• devm_drm_dev_init

Also, now that we have a very slick and polished way to allocate a drm_device with devm_drm_dev_alloc, update all the docs to reflect the new reality. Mostly this consists of deleting old and misleading hints. Two main ones:

• it is no longer required that the drm_device base class is first in the structure. devm_drm_dev_alloc can cope with it being anywhere

• obviously embedded now strongly recommends using devm_drm_dev_alloc

v2: Fix typos (Noralf)

v3: Split out the removal of drm_dev_init, that's blocked on some discussions on how to convert vgem/vkms/i915-selftests. Adjust commit message to reflect that.

Cc: Noralf Trønnes noralf@tronnes.org Acked-by: Noralf Trønnes noralf@tronnes.org (v2) Acked-by: Sam Ravnborg sam@ravnborg.org Cc: Luben Tuikov luben.tuikov@amd.com Cc: amd-gfx@lists.freedesktop.org Signed-off-by: Daniel Vetter daniel.vetter@intel.com

Ok pushed that now to drm-misc-next. I'm also working on getting the remaining bits of the basic drm managed conversion resubmitted. That was unfortunately massively sidelined for the dma-fence discussions.

Quick heads-up: drmm_add_final_kfree and drm_dev_init will both disappear, please use devm_drm_dev_alloc.

Hi Daniel,

In drm_drv.c, in the "DOC: driver instance overview" section, it would help a lot, if you could add/summarize/clarify, succinctly, the two paths, device instantiation:

devm_drm_dev_alloc(); ... drm_dev_init(); ... drm_dev_register(); ...

And, device destruction, and where/how the "release" method of drm_driver plays out.

The allocation part is mostly there, that's good, but the release/destruction part is not. That is, the platform driver callbacks are there, but not the DRM driver part. In this patch, there is no mention of drm_dev_init(), and the documentation update of this patch doesn't mention it, while it is being used by at least one driver.

If, on the other hand, you're thinking of removing the "release" callback, a lucid explanation on kref reaching 0--what should be done by DRM and what should be done by drivers, would be very nice and helpful to low-level DRM device driver maintainers/writers.

Also, consider renaming "drm_add_action()" to something qualifying the action: either a bitmask as an argument, or right in the name, "drm_add_action_release()", else one begs the question "What action?"

It would be helpful, to describe the order of "release" actions on kref reaching 0, and what is expected of DRM and what of drivers, in the order of the expected callbacks.

Regards, Luben

Cheers, Daniel

...

---

.../driver-api/driver-model/devres.rst | 2 +- drivers/gpu/drm/drm_drv.c | 78 +++++-------------- drivers/gpu/drm/drm_managed.c | 2 +- include/drm/drm_device.h | 2 +- include/drm/drm_drv.h | 16 ++-- 5 files changed, 30 insertions(+), 70 deletions(-)

diff --git a/Documentation/driver-api/driver-model/devres.rst b/Documentation/driver-api/driver-model/devres.rst index eaaaafc21134..aa4d2420f79e 100644 --- a/Documentation/driver-api/driver-model/devres.rst +++ b/Documentation/driver-api/driver-model/devres.rst @@ -263,7 +263,7 @@ DMA dmam_pool_destroy()

DRM

• devm_drm_dev_init()

• devm_drm_dev_alloc()

GPIO devm_gpiod_get() diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c index d4506f7a234e..7c1689842ec0 100644 --- a/drivers/gpu/drm/drm_drv.c +++ b/drivers/gpu/drm/drm_drv.c @@ -240,13 +240,13 @@ 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 initialized with drm_dev_init(), 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. Finally when everything is up
• • and running and ready for userspace the device instance can be published
• • using drm_dev_register().

• • is allocated and initialized with devm_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 initial output
• • configuration plus obviously initialize all the corresponding hardware bits.
• • 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 &drm_driver.load callback. But due to

@@ -274,7 +274,7 @@ void drm_minor_release(struct drm_minor *minor)

• The following example shows a typical structure of a DRM display driver.
• The example focus on the probe() function and the other functions that is

• • almost always present and serves as a demonstration of devm_drm_dev_init().

• • almost always present and serves as a demonstration of devm_drm_dev_alloc().
  • .. code-block:: c

@@ -294,22 +294,12 @@ void drm_minor_release(struct drm_minor *minor)

• struct drm_device *drm;
  

• int ret;
  

• • // devm_kzalloc() can't be used here because the drm_device '
    

• • // lifetime can exceed the device lifetime if driver unbind
    

• • // happens when userspace still has open file descriptors.
    

• • priv = kzalloc(sizeof(*priv), GFP_KERNEL);
    

• • if (!priv)
    

• • 	return -ENOMEM;
    

• • priv = devm_drm_dev_alloc(&pdev->dev, &driver_drm_driver,
    

• • 			  struct driver_device, drm);
    

• • if (IS_ERR(priv))
    

• • 	return PTR_ERR(priv);
    

  • drm = &priv->drm;
    

• • ret = devm_drm_dev_init(&pdev->dev, drm, &driver_drm_driver);
    

• • if (ret) {
    

• • 	kfree(priv);
    

• • 	return ret;
    

• • }
    

• • drmm_add_final_kfree(drm, priv);
    

• • ret = drmm_mode_config_init(drm);
    

  • if (ret)
    

  • 	return ret;
    

@@ -550,9 +540,9 @@ static void drm_fs_inode_free(struct inode *inode)

• following guidelines apply:
• • The entire device initialization procedure should be run from the

• • &component_master_ops.master_bind callback, starting with drm_dev_init(),
• • then binding all components with component_bind_all() and finishing with
• • drm_dev_register().

• • &component_master_ops.master_bind callback, starting with
• • devm_drm_dev_alloc(), then binding all components with
• • component_bind_all() and finishing with drm_dev_register().
  • • The opaque pointer passed to all components through component_bind_all()
  • should point at &struct drm_device of the device instance, not some driver

@@ -706,24 +696,9 @@ static void devm_drm_dev_init_release(void *data) drm_dev_put(data); }

-/**

• • devm_drm_dev_init - Resource managed drm_dev_init()
• • @parent: Parent device object
• • @dev: DRM device
• • @driver: DRM driver
• • Managed drm_dev_init(). The DRM device initialized with this function is
• • automatically put on driver detach using drm_dev_put().
• • Note that drivers must call drmm_add_final_kfree() after this function has
• • completed successfully.
• • RETURNS:
• • 0 on success, or error code on failure.
• */

-int devm_drm_dev_init(struct device *parent,

•       struct drm_device *dev,
  

•       struct drm_driver *driver)
  

+static int devm_drm_dev_init(struct device *parent,

• 	     struct drm_device *dev,
  

• 	     struct drm_driver *driver)
  

{ int ret;

@@ -737,7 +712,6 @@ int devm_drm_dev_init(struct device *parent,

return ret; } -EXPORT_SYMBOL(devm_drm_dev_init);

void *__devm_drm_dev_alloc(struct device *parent, struct drm_driver *driver, size_t size, size_t offset) @@ -767,19 +741,9 @@ EXPORT_SYMBOL(__devm_drm_dev_alloc);

• @driver: DRM driver to allocate device for
• @parent: Parent device object

• • 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. 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_get() and
• • drm_dev_put() to take and drop further ref-counts.
• • Note that for purely virtual devices @parent can be NULL.
• • Drivers that wish to subclass or embed &struct drm_device into their
• • own struct should look at using drm_dev_init() instead.

• • This is the deprecated version of devm_drm_dev_alloc(), which does not support
• • subclassing through embedding the struct &drm_device in a driver private
• • structure, and which does not support automatic cleanup through devres.
  • RETURNS:
  • Pointer to new DRM device, or ERR_PTR on failure.

diff --git a/drivers/gpu/drm/drm_managed.c b/drivers/gpu/drm/drm_managed.c index 1e1356560c2e..c36e3d98fd71 100644 --- a/drivers/gpu/drm/drm_managed.c +++ b/drivers/gpu/drm/drm_managed.c @@ -27,7 +27,7 @@

• be done directly with drmm_kmalloc() and the related functions. Everything
• will be released on the final drm_dev_put() in reverse order of how the
• release actions have been added and memory has been allocated since driver

• • loading started with drm_dev_init().

• • loading started with devm_drm_dev_alloc().
  • Note that release actions and managed memory can also be added and removed
  • during the lifetime of the driver, all the functions are fully concurrent

diff --git a/include/drm/drm_device.h b/include/drm/drm_device.h index 0988351d743c..f4f68e7a9149 100644 --- a/include/drm/drm_device.h +++ b/include/drm/drm_device.h @@ -92,7 +92,7 @@ struct drm_device { * NULL. * * Instead of using this pointer it is recommended that drivers use

• * drm_dev_init() and embed struct &drm_device in their larger
  

• * devm_drm_dev_alloc() and embed struct &drm_device in their larger
  

  • per-device structure.
  */ void *dev_private;

diff --git a/include/drm/drm_drv.h b/include/drm/drm_drv.h index 6ba7dd11384d..533c6e1a5a95 100644 --- a/include/drm/drm_drv.h +++ b/include/drm/drm_drv.h @@ -163,13 +163,12 @@ struct drm_driver { /** * @load: *

• * Backward-compatible driver callback to complete
  

• * initialization steps after the driver is registered.  For
  

• * this reason, may suffer from race conditions and its use is
  

• * deprecated for new drivers.  It is therefore only supported
  

• * for existing drivers not yet converted to the new scheme.
  

• * See drm_dev_init() and drm_dev_register() for proper and
  

• * race-free way to set up a &struct drm_device.
  

• * Backward-compatible driver callback to complete initialization steps
  

• * after the driver is registered.  For this reason, may suffer from
  

• * race conditions and its use is deprecated for new drivers.  It is
  

• * therefore only supported for existing drivers not yet converted to
  

• * the new scheme.  See devm_drm_dev_alloc() and drm_dev_register() for
  

• * proper and race-free way to set up a &struct drm_device.
  

  • This is deprecated, do not use!

@@ -595,9 +594,6 @@ struct drm_driver { int drm_dev_init(struct drm_device *dev, struct drm_driver *driver, struct device *parent); -int devm_drm_dev_init(struct device *parent,

•       struct drm_device *dev,
  

•       struct drm_driver *driver);
  
  

void *__devm_drm_dev_alloc(struct device *parent, struct drm_driver *driver, size_t size, size_t offset); -- 2.28.0