Re: devm actions and hw clenaup (was Re: [PATCH 01/11] drm: Add devm_drm_dev_init/register)
On Wed, Jan 30, 2019 at 12:14:46AM +0100, Daniel Vetter wrote: > On Tue, Jan 29, 2019 at 8:27 PM Greg KH wrote: > > On Tue, Jan 29, 2019 at 07:10:55PM +0100, Daniel Vetter wrote: > > > The problem is when drivers use devm_ not to allocate hw resources and > > > related things, but structures for objects with other lifetimes. Like > > > open file descriptors shared with the world. > > > > And irqs, which bites everyone in the end. You have to be careful here, > > never tie a devm allocation to an object with another reference count, > > that's just a bug. > > The classic "I forgot to shut down the interrupt before releasing > driver structures and now the irq handler oopsed" or something more > sinister? The classic is always the best, and most common :) > gpus definitely needs lots of interrupt stuff, so if that is > fundamentally broken then our devm ideas won't work out at all. You better not be using devm for your irqs right now then. good luck! greg k-h ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: devm actions and hw clenaup (was Re: [PATCH 01/11] drm: Add devm_drm_dev_init/register)
On Tue, Jan 29, 2019 at 8:27 PM Greg KH wrote:
>
> On Tue, Jan 29, 2019 at 07:10:55PM +0100, Daniel Vetter wrote:
> > On Tue, Jan 29, 2019 at 6:36 PM Greg KH wrote:
> > >
> > > On Tue, Jan 29, 2019 at 05:50:05PM +0100, Daniel Vetter wrote:
> > > > On Tue, Jan 29, 2019 at 03:34:46PM +0100, Noralf Trønnes wrote:
> > > > >
> > > > >
> > > > > Den 24.01.2019 18.57, skrev Daniel Vetter:
> > > > > > On Thu, Jan 24, 2019 at 6:46 PM Greg KH
> > > > > > wrote:
> > > > > >>
> > > > > >> On Thu, Jan 24, 2019 at 11:43:12AM +0100, Daniel Vetter wrote:
> > > > > >>> On Wed, Jan 23, 2019 at 11:54:07AM +0100, Noralf Trønnes wrote:
> > > > >
> > > > >
> > > > > Den 22.01.2019 20.30, skrev Daniel Vetter:
> > > > > > On Tue, Jan 22, 2019 at 8:07 PM Noralf Trønnes
> > > > > > wrote:
> > > > > >>
> > > > > >>
> > > > > >>
> > > > > >> Den 22.01.2019 10.32, skrev Daniel Vetter:
> > > > > >>> On Mon, Jan 21, 2019 at 01:21:46PM +0100, Noralf Trønnes
> > > > > >>> wrote:
> > > > >
> > > > >
> > > > > Den 21.01.2019 10.55, skrev Daniel Vetter:
> > > > > > On Mon, Jan 21, 2019 at 10:10:14AM +0100, Daniel Vetter
> > > > > > wrote:
> > > > > >> On Sun, Jan 20, 2019 at 12:43:08PM +0100, Noralf Trønnes
> > > > > >> wrote:
> > > > > >>> This adds resource managed (devres) versions of
> > > > > >>> drm_dev_init() and
> > > > > >>> drm_dev_register().
> > > > > >>>
> > > > > >>> Also added is devm_drm_dev_register_with_fbdev() which
> > > > > >>> sets up generic
> > > > > >>> fbdev emulation as well.
> > > > > >>>
> > > > > >>> devm_drm_dev_register() isn't exported since there are no
> > > > > >>> users.
> > > > > >>>
> > > > > >>> Signed-off-by: Noralf Trønnes
> > > > > >>
> > > > >
> > > > >
> > > > >
> > > > > >>> diff --git a/drivers/gpu/drm/drm_drv.c
> > > > > >>> b/drivers/gpu/drm/drm_drv.c
> > > > > >>> index 381581b01d48..12129772be45 100644
> > > > > >>> --- a/drivers/gpu/drm/drm_drv.c
> > > > > >>> +++ b/drivers/gpu/drm/drm_drv.c
> > > > > >>> @@ -36,6 +36,7 @@
> > > > > >>>
> > > > > >>> #include
> > > > > >>> #include
> > > > > >>> +#include
> > > > > >>> #include
> > > > > >>>
> > > > > >>> #include "drm_crtc_internal.h"
> > > > > >>> @@ -871,6 +872,111 @@ void drm_dev_unregister(struct
> > > > > >>> drm_device *dev)
> > > > > >>> }
> > > > > >>> EXPORT_SYMBOL(drm_dev_unregister);
> > > > > >>>
> > > > > >>> +static void devm_drm_dev_init_release(void *data)
> > > > > >>> +{
> > > > > >>> + drm_dev_put(data);
> > > > > >
> > > > > > We need drm_dev_unplug() here, or this isn't safe.
> > > > >
> > > > > This function is only used to cover the error path if probe
> > > > > fails before
> > > > > devm_drm_dev_register() is called.
> > > > > devm_drm_dev_register_release() is
> > > > > the one that calls unplug. There are comments about this in
> > > > > the functions.
> > > > > >>>
> > > > > >>> I think I get a prize for being ignorant and blind :-/
> > > > > >>>
> > > > >
> > > > > >
> > > > > >>> +}
> > > > > >>> +
> > > > > >>> +/**
> > > > > >>> + * 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 released on driver detach. You must
> > > > > >>> supply a
> > > > > >
> > > > > > I think a bit more clarity here would be good:
> > > > > >
> > > > > > "... automatically released on driver unbind by callind
> > > > > > drm_dev_unplug()."
> > > > > >
> > > > > >>> + * _driver.release callback to control the
> > > > > >>> finalization explicitly.
> > > > > >
> > > > > > I think a loud warning for these is in order:
> > > > > >
> > > > > > "WARNING:
> > > > > >
> > > > > > "In generally it is unsafe to use devm functions for drm
> > > > > > structures
> > > > > > because the lifetimes of _device and the underlying
> > > > > > do not
> > > > > > match. This here works because it doesn't immediately free
> > > > > > anything, but
> > > > > > only calls drm_dev_unplug(), which internally decrements
> > > > > > the _device
> > > > > > refcount through
Re: devm actions and hw clenaup (was Re: [PATCH 01/11] drm: Add devm_drm_dev_init/register)
On Tue, Jan 29, 2019 at 07:10:55PM +0100, Daniel Vetter wrote:
> On Tue, Jan 29, 2019 at 6:36 PM Greg KH wrote:
> >
> > On Tue, Jan 29, 2019 at 05:50:05PM +0100, Daniel Vetter wrote:
> > > On Tue, Jan 29, 2019 at 03:34:46PM +0100, Noralf Trønnes wrote:
> > > >
> > > >
> > > > Den 24.01.2019 18.57, skrev Daniel Vetter:
> > > > > On Thu, Jan 24, 2019 at 6:46 PM Greg KH
> > > > > wrote:
> > > > >>
> > > > >> On Thu, Jan 24, 2019 at 11:43:12AM +0100, Daniel Vetter wrote:
> > > > >>> On Wed, Jan 23, 2019 at 11:54:07AM +0100, Noralf Trønnes wrote:
> > > >
> > > >
> > > > Den 22.01.2019 20.30, skrev Daniel Vetter:
> > > > > On Tue, Jan 22, 2019 at 8:07 PM Noralf Trønnes
> > > > > wrote:
> > > > >>
> > > > >>
> > > > >>
> > > > >> Den 22.01.2019 10.32, skrev Daniel Vetter:
> > > > >>> On Mon, Jan 21, 2019 at 01:21:46PM +0100, Noralf Trønnes wrote:
> > > >
> > > >
> > > > Den 21.01.2019 10.55, skrev Daniel Vetter:
> > > > > On Mon, Jan 21, 2019 at 10:10:14AM +0100, Daniel Vetter wrote:
> > > > >> On Sun, Jan 20, 2019 at 12:43:08PM +0100, Noralf Trønnes
> > > > >> wrote:
> > > > >>> This adds resource managed (devres) versions of
> > > > >>> drm_dev_init() and
> > > > >>> drm_dev_register().
> > > > >>>
> > > > >>> Also added is devm_drm_dev_register_with_fbdev() which sets
> > > > >>> up generic
> > > > >>> fbdev emulation as well.
> > > > >>>
> > > > >>> devm_drm_dev_register() isn't exported since there are no
> > > > >>> users.
> > > > >>>
> > > > >>> Signed-off-by: Noralf Trønnes
> > > > >>
> > > >
> > > >
> > > >
> > > > >>> diff --git a/drivers/gpu/drm/drm_drv.c
> > > > >>> b/drivers/gpu/drm/drm_drv.c
> > > > >>> index 381581b01d48..12129772be45 100644
> > > > >>> --- a/drivers/gpu/drm/drm_drv.c
> > > > >>> +++ b/drivers/gpu/drm/drm_drv.c
> > > > >>> @@ -36,6 +36,7 @@
> > > > >>>
> > > > >>> #include
> > > > >>> #include
> > > > >>> +#include
> > > > >>> #include
> > > > >>>
> > > > >>> #include "drm_crtc_internal.h"
> > > > >>> @@ -871,6 +872,111 @@ void drm_dev_unregister(struct
> > > > >>> drm_device *dev)
> > > > >>> }
> > > > >>> EXPORT_SYMBOL(drm_dev_unregister);
> > > > >>>
> > > > >>> +static void devm_drm_dev_init_release(void *data)
> > > > >>> +{
> > > > >>> + drm_dev_put(data);
> > > > >
> > > > > We need drm_dev_unplug() here, or this isn't safe.
> > > >
> > > > This function is only used to cover the error path if probe
> > > > fails before
> > > > devm_drm_dev_register() is called.
> > > > devm_drm_dev_register_release() is
> > > > the one that calls unplug. There are comments about this in
> > > > the functions.
> > > > >>>
> > > > >>> I think I get a prize for being ignorant and blind :-/
> > > > >>>
> > > >
> > > > >
> > > > >>> +}
> > > > >>> +
> > > > >>> +/**
> > > > >>> + * 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 released on driver detach. You must
> > > > >>> supply a
> > > > >
> > > > > I think a bit more clarity here would be good:
> > > > >
> > > > > "... automatically released on driver unbind by callind
> > > > > drm_dev_unplug()."
> > > > >
> > > > >>> + * _driver.release callback to control the
> > > > >>> finalization explicitly.
> > > > >
> > > > > I think a loud warning for these is in order:
> > > > >
> > > > > "WARNING:
> > > > >
> > > > > "In generally it is unsafe to use devm functions for drm
> > > > > structures
> > > > > because the lifetimes of _device and the underlying
> > > > > do not
> > > > > match. This here works because it doesn't immediately free
> > > > > anything, but
> > > > > only calls drm_dev_unplug(), which internally decrements the
> > > > > _device
> > > > > refcount through drm_dev_put().
> > > > >
> > > > > "All other drm structures must still be explicitly released
> > > > > in the
> > > > > _driver.release callback."
> > > > >
> > > > > While thinking about this I just realized that with this
> > > > > design we have no
> > > >
Re: devm actions and hw clenaup (was Re: [PATCH 01/11] drm: Add devm_drm_dev_init/register)
On Tue, Jan 29, 2019 at 6:36 PM Greg KH wrote:
>
> On Tue, Jan 29, 2019 at 05:50:05PM +0100, Daniel Vetter wrote:
> > On Tue, Jan 29, 2019 at 03:34:46PM +0100, Noralf Trønnes wrote:
> > >
> > >
> > > Den 24.01.2019 18.57, skrev Daniel Vetter:
> > > > On Thu, Jan 24, 2019 at 6:46 PM Greg KH
> > > > wrote:
> > > >>
> > > >> On Thu, Jan 24, 2019 at 11:43:12AM +0100, Daniel Vetter wrote:
> > > >>> On Wed, Jan 23, 2019 at 11:54:07AM +0100, Noralf Trønnes wrote:
> > >
> > >
> > > Den 22.01.2019 20.30, skrev Daniel Vetter:
> > > > On Tue, Jan 22, 2019 at 8:07 PM Noralf Trønnes
> > > > wrote:
> > > >>
> > > >>
> > > >>
> > > >> Den 22.01.2019 10.32, skrev Daniel Vetter:
> > > >>> On Mon, Jan 21, 2019 at 01:21:46PM +0100, Noralf Trønnes wrote:
> > >
> > >
> > > Den 21.01.2019 10.55, skrev Daniel Vetter:
> > > > On Mon, Jan 21, 2019 at 10:10:14AM +0100, Daniel Vetter wrote:
> > > >> On Sun, Jan 20, 2019 at 12:43:08PM +0100, Noralf Trønnes wrote:
> > > >>> This adds resource managed (devres) versions of
> > > >>> drm_dev_init() and
> > > >>> drm_dev_register().
> > > >>>
> > > >>> Also added is devm_drm_dev_register_with_fbdev() which sets
> > > >>> up generic
> > > >>> fbdev emulation as well.
> > > >>>
> > > >>> devm_drm_dev_register() isn't exported since there are no
> > > >>> users.
> > > >>>
> > > >>> Signed-off-by: Noralf Trønnes
> > > >>
> > >
> > >
> > >
> > > >>> diff --git a/drivers/gpu/drm/drm_drv.c
> > > >>> b/drivers/gpu/drm/drm_drv.c
> > > >>> index 381581b01d48..12129772be45 100644
> > > >>> --- a/drivers/gpu/drm/drm_drv.c
> > > >>> +++ b/drivers/gpu/drm/drm_drv.c
> > > >>> @@ -36,6 +36,7 @@
> > > >>>
> > > >>> #include
> > > >>> #include
> > > >>> +#include
> > > >>> #include
> > > >>>
> > > >>> #include "drm_crtc_internal.h"
> > > >>> @@ -871,6 +872,111 @@ void drm_dev_unregister(struct
> > > >>> drm_device *dev)
> > > >>> }
> > > >>> EXPORT_SYMBOL(drm_dev_unregister);
> > > >>>
> > > >>> +static void devm_drm_dev_init_release(void *data)
> > > >>> +{
> > > >>> + drm_dev_put(data);
> > > >
> > > > We need drm_dev_unplug() here, or this isn't safe.
> > >
> > > This function is only used to cover the error path if probe
> > > fails before
> > > devm_drm_dev_register() is called.
> > > devm_drm_dev_register_release() is
> > > the one that calls unplug. There are comments about this in the
> > > functions.
> > > >>>
> > > >>> I think I get a prize for being ignorant and blind :-/
> > > >>>
> > >
> > > >
> > > >>> +}
> > > >>> +
> > > >>> +/**
> > > >>> + * 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 released on driver detach. You must supply a
> > > >
> > > > I think a bit more clarity here would be good:
> > > >
> > > > "... automatically released on driver unbind by callind
> > > > drm_dev_unplug()."
> > > >
> > > >>> + * _driver.release callback to control the finalization
> > > >>> explicitly.
> > > >
> > > > I think a loud warning for these is in order:
> > > >
> > > > "WARNING:
> > > >
> > > > "In generally it is unsafe to use devm functions for drm
> > > > structures
> > > > because the lifetimes of _device and the underlying
> > > > do not
> > > > match. This here works because it doesn't immediately free
> > > > anything, but
> > > > only calls drm_dev_unplug(), which internally decrements the
> > > > _device
> > > > refcount through drm_dev_put().
> > > >
> > > > "All other drm structures must still be explicitly released in
> > > > the
> > > > _driver.release callback."
> > > >
> > > > While thinking about this I just realized that with this design
> > > > we have no
> > > > good place to call drm_atomic_helper_shutdown(). Which we need
> > > > to, or all
> > > > kinds of things will leak badly (connectors, fb, ...), but
> > > > there's no
> > > > place to call it:
> > > > - unbind is too early, since we haven't yet called
> > > > drm_dev_unplug, and
Re: devm actions and hw clenaup (was Re: [PATCH 01/11] drm: Add devm_drm_dev_init/register)
On Tue, Jan 29, 2019 at 05:50:05PM +0100, Daniel Vetter wrote:
> On Tue, Jan 29, 2019 at 03:34:46PM +0100, Noralf Trønnes wrote:
> >
> >
> > Den 24.01.2019 18.57, skrev Daniel Vetter:
> > > On Thu, Jan 24, 2019 at 6:46 PM Greg KH
> > > wrote:
> > >>
> > >> On Thu, Jan 24, 2019 at 11:43:12AM +0100, Daniel Vetter wrote:
> > >>> On Wed, Jan 23, 2019 at 11:54:07AM +0100, Noralf Trønnes wrote:
> >
> >
> > Den 22.01.2019 20.30, skrev Daniel Vetter:
> > > On Tue, Jan 22, 2019 at 8:07 PM Noralf Trønnes
> > > wrote:
> > >>
> > >>
> > >>
> > >> Den 22.01.2019 10.32, skrev Daniel Vetter:
> > >>> On Mon, Jan 21, 2019 at 01:21:46PM +0100, Noralf Trønnes wrote:
> >
> >
> > Den 21.01.2019 10.55, skrev Daniel Vetter:
> > > On Mon, Jan 21, 2019 at 10:10:14AM +0100, Daniel Vetter wrote:
> > >> On Sun, Jan 20, 2019 at 12:43:08PM +0100, Noralf Trønnes wrote:
> > >>> This adds resource managed (devres) versions of drm_dev_init()
> > >>> and
> > >>> drm_dev_register().
> > >>>
> > >>> Also added is devm_drm_dev_register_with_fbdev() which sets up
> > >>> generic
> > >>> fbdev emulation as well.
> > >>>
> > >>> devm_drm_dev_register() isn't exported since there are no users.
> > >>>
> > >>> Signed-off-by: Noralf Trønnes
> > >>
> >
> >
> >
> > >>> diff --git a/drivers/gpu/drm/drm_drv.c
> > >>> b/drivers/gpu/drm/drm_drv.c
> > >>> index 381581b01d48..12129772be45 100644
> > >>> --- a/drivers/gpu/drm/drm_drv.c
> > >>> +++ b/drivers/gpu/drm/drm_drv.c
> > >>> @@ -36,6 +36,7 @@
> > >>>
> > >>> #include
> > >>> #include
> > >>> +#include
> > >>> #include
> > >>>
> > >>> #include "drm_crtc_internal.h"
> > >>> @@ -871,6 +872,111 @@ void drm_dev_unregister(struct drm_device
> > >>> *dev)
> > >>> }
> > >>> EXPORT_SYMBOL(drm_dev_unregister);
> > >>>
> > >>> +static void devm_drm_dev_init_release(void *data)
> > >>> +{
> > >>> + drm_dev_put(data);
> > >
> > > We need drm_dev_unplug() here, or this isn't safe.
> >
> > This function is only used to cover the error path if probe fails
> > before
> > devm_drm_dev_register() is called. devm_drm_dev_register_release()
> > is
> > the one that calls unplug. There are comments about this in the
> > functions.
> > >>>
> > >>> I think I get a prize for being ignorant and blind :-/
> > >>>
> >
> > >
> > >>> +}
> > >>> +
> > >>> +/**
> > >>> + * 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 released on driver detach. You must supply a
> > >
> > > I think a bit more clarity here would be good:
> > >
> > > "... automatically released on driver unbind by callind
> > > drm_dev_unplug()."
> > >
> > >>> + * _driver.release callback to control the finalization
> > >>> explicitly.
> > >
> > > I think a loud warning for these is in order:
> > >
> > > "WARNING:
> > >
> > > "In generally it is unsafe to use devm functions for drm
> > > structures
> > > because the lifetimes of _device and the underlying
> > > do not
> > > match. This here works because it doesn't immediately free
> > > anything, but
> > > only calls drm_dev_unplug(), which internally decrements the
> > > _device
> > > refcount through drm_dev_put().
> > >
> > > "All other drm structures must still be explicitly released in the
> > > _driver.release callback."
> > >
> > > While thinking about this I just realized that with this design
> > > we have no
> > > good place to call drm_atomic_helper_shutdown(). Which we need
> > > to, or all
> > > kinds of things will leak badly (connectors, fb, ...), but
> > > there's no
> > > place to call it:
> > > - unbind is too early, since we haven't yet called
> > > drm_dev_unplug, and the
> > > drm_dev_unregister in there must be called _before_ we start to
> > > shut
> > > down anything.
> > > - drm_driver.release is way too late.
> > >
> > > Ofc for a real hotunplug there's no point in shutting down the hw
> > > (it's
> > > already
Re: devm actions and hw clenaup (was Re: [PATCH 01/11] drm: Add devm_drm_dev_init/register)
Den 29.01.2019 17.50, skrev Daniel Vetter:
> On Tue, Jan 29, 2019 at 03:34:46PM +0100, Noralf Trønnes wrote:
>>
>>
>> Den 24.01.2019 18.57, skrev Daniel Vetter:
>>> On Thu, Jan 24, 2019 at 6:46 PM Greg KH wrote:
On Thu, Jan 24, 2019 at 11:43:12AM +0100, Daniel Vetter wrote:
> On Wed, Jan 23, 2019 at 11:54:07AM +0100, Noralf Trønnes wrote:
>>
>>
>> Den 22.01.2019 20.30, skrev Daniel Vetter:
>>> On Tue, Jan 22, 2019 at 8:07 PM Noralf Trønnes
>>> wrote:
Den 22.01.2019 10.32, skrev Daniel Vetter:
> On Mon, Jan 21, 2019 at 01:21:46PM +0100, Noralf Trønnes wrote:
>>
>>
>> Den 21.01.2019 10.55, skrev Daniel Vetter:
>>> On Mon, Jan 21, 2019 at 10:10:14AM +0100, Daniel Vetter wrote:
On Sun, Jan 20, 2019 at 12:43:08PM +0100, Noralf Trønnes wrote:
> This adds resource managed (devres) versions of drm_dev_init() and
> drm_dev_register().
>
> Also added is devm_drm_dev_register_with_fbdev() which sets up
> generic
> fbdev emulation as well.
>
> devm_drm_dev_register() isn't exported since there are no users.
>
> Signed-off-by: Noralf Trønnes
>>
>>
>>
> diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
> index 381581b01d48..12129772be45 100644
> --- a/drivers/gpu/drm/drm_drv.c
> +++ b/drivers/gpu/drm/drm_drv.c
> @@ -36,6 +36,7 @@
>
> #include
> #include
> +#include
> #include
>
> #include "drm_crtc_internal.h"
> @@ -871,6 +872,111 @@ void drm_dev_unregister(struct drm_device
> *dev)
> }
> EXPORT_SYMBOL(drm_dev_unregister);
>
> +static void devm_drm_dev_init_release(void *data)
> +{
> + drm_dev_put(data);
>>>
>>> We need drm_dev_unplug() here, or this isn't safe.
>>
>> This function is only used to cover the error path if probe fails
>> before
>> devm_drm_dev_register() is called. devm_drm_dev_register_release() is
>> the one that calls unplug. There are comments about this in the
>> functions.
>
> I think I get a prize for being ignorant and blind :-/
>
>>
>>>
> +}
> +
> +/**
> + * 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 released on driver detach. You must supply a
>>>
>>> I think a bit more clarity here would be good:
>>>
>>> "... automatically released on driver unbind by callind
>>> drm_dev_unplug()."
>>>
> + * _driver.release callback to control the finalization
> explicitly.
>>>
>>> I think a loud warning for these is in order:
>>>
>>> "WARNING:
>>>
>>> "In generally it is unsafe to use devm functions for drm structures
>>> because the lifetimes of _device and the underlying do
>>> not
>>> match. This here works because it doesn't immediately free
>>> anything, but
>>> only calls drm_dev_unplug(), which internally decrements the
>>> _device
>>> refcount through drm_dev_put().
>>>
>>> "All other drm structures must still be explicitly released in the
>>> _driver.release callback."
>>>
>>> While thinking about this I just realized that with this design we
>>> have no
>>> good place to call drm_atomic_helper_shutdown(). Which we need to,
>>> or all
>>> kinds of things will leak badly (connectors, fb, ...), but there's
>>> no
>>> place to call it:
>>> - unbind is too early, since we haven't yet called drm_dev_unplug,
>>> and the
>>> drm_dev_unregister in there must be called _before_ we start to
>>> shut
>>> down anything.
>>> - drm_driver.release is way too late.
>>>
>>> Ofc for a real hotunplug there's no point in shutting down the hw
>>> (it's
>>> already gone), but for a driver unload/unbind it would be nice if
>>> this
>>> happens automatically and in the right order.
>>>
>>> So not sure what to do here really.
>>
>> How about this change: (it breaks the rule of pulling helpers into
>> the
>> core, so maybe we
Re: devm actions and hw clenaup (was Re: [PATCH 01/11] drm: Add devm_drm_dev_init/register)
On Tue, Jan 29, 2019 at 03:34:46PM +0100, Noralf Trønnes wrote:
>
>
> Den 24.01.2019 18.57, skrev Daniel Vetter:
> > On Thu, Jan 24, 2019 at 6:46 PM Greg KH wrote:
> >>
> >> On Thu, Jan 24, 2019 at 11:43:12AM +0100, Daniel Vetter wrote:
> >>> On Wed, Jan 23, 2019 at 11:54:07AM +0100, Noralf Trønnes wrote:
>
>
> Den 22.01.2019 20.30, skrev Daniel Vetter:
> > On Tue, Jan 22, 2019 at 8:07 PM Noralf Trønnes
> > wrote:
> >>
> >>
> >>
> >> Den 22.01.2019 10.32, skrev Daniel Vetter:
> >>> On Mon, Jan 21, 2019 at 01:21:46PM +0100, Noralf Trønnes wrote:
>
>
> Den 21.01.2019 10.55, skrev Daniel Vetter:
> > On Mon, Jan 21, 2019 at 10:10:14AM +0100, Daniel Vetter wrote:
> >> On Sun, Jan 20, 2019 at 12:43:08PM +0100, Noralf Trønnes wrote:
> >>> This adds resource managed (devres) versions of drm_dev_init() and
> >>> drm_dev_register().
> >>>
> >>> Also added is devm_drm_dev_register_with_fbdev() which sets up
> >>> generic
> >>> fbdev emulation as well.
> >>>
> >>> devm_drm_dev_register() isn't exported since there are no users.
> >>>
> >>> Signed-off-by: Noralf Trønnes
> >>
>
>
>
> >>> diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
> >>> index 381581b01d48..12129772be45 100644
> >>> --- a/drivers/gpu/drm/drm_drv.c
> >>> +++ b/drivers/gpu/drm/drm_drv.c
> >>> @@ -36,6 +36,7 @@
> >>>
> >>> #include
> >>> #include
> >>> +#include
> >>> #include
> >>>
> >>> #include "drm_crtc_internal.h"
> >>> @@ -871,6 +872,111 @@ void drm_dev_unregister(struct drm_device
> >>> *dev)
> >>> }
> >>> EXPORT_SYMBOL(drm_dev_unregister);
> >>>
> >>> +static void devm_drm_dev_init_release(void *data)
> >>> +{
> >>> + drm_dev_put(data);
> >
> > We need drm_dev_unplug() here, or this isn't safe.
>
> This function is only used to cover the error path if probe fails
> before
> devm_drm_dev_register() is called. devm_drm_dev_register_release() is
> the one that calls unplug. There are comments about this in the
> functions.
> >>>
> >>> I think I get a prize for being ignorant and blind :-/
> >>>
>
> >
> >>> +}
> >>> +
> >>> +/**
> >>> + * 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 released on driver detach. You must supply a
> >
> > I think a bit more clarity here would be good:
> >
> > "... automatically released on driver unbind by callind
> > drm_dev_unplug()."
> >
> >>> + * _driver.release callback to control the finalization
> >>> explicitly.
> >
> > I think a loud warning for these is in order:
> >
> > "WARNING:
> >
> > "In generally it is unsafe to use devm functions for drm structures
> > because the lifetimes of _device and the underlying do
> > not
> > match. This here works because it doesn't immediately free
> > anything, but
> > only calls drm_dev_unplug(), which internally decrements the
> > _device
> > refcount through drm_dev_put().
> >
> > "All other drm structures must still be explicitly released in the
> > _driver.release callback."
> >
> > While thinking about this I just realized that with this design we
> > have no
> > good place to call drm_atomic_helper_shutdown(). Which we need to,
> > or all
> > kinds of things will leak badly (connectors, fb, ...), but there's
> > no
> > place to call it:
> > - unbind is too early, since we haven't yet called drm_dev_unplug,
> > and the
> > drm_dev_unregister in there must be called _before_ we start to
> > shut
> > down anything.
> > - drm_driver.release is way too late.
> >
> > Ofc for a real hotunplug there's no point in shutting down the hw
> > (it's
> > already gone), but for a driver unload/unbind it would be nice if
> > this
> > happens automatically and in the right order.
> >
> > So not sure what to do here really.
>
> How about this change: (it breaks the rule of pulling helpers into
> the
> core, so maybe we should put the devm_ functions into the simple KMS
>
Re: devm actions and hw clenaup (was Re: [PATCH 01/11] drm: Add devm_drm_dev_init/register)
On Tue, Jan 29, 2019 at 03:34:46PM +0100, Noralf Trønnes wrote:
>
>
> Den 24.01.2019 18.57, skrev Daniel Vetter:
> > On Thu, Jan 24, 2019 at 6:46 PM Greg KH wrote:
> >>
> >> On Thu, Jan 24, 2019 at 11:43:12AM +0100, Daniel Vetter wrote:
> >>> On Wed, Jan 23, 2019 at 11:54:07AM +0100, Noralf Trønnes wrote:
>
>
> Den 22.01.2019 20.30, skrev Daniel Vetter:
> > On Tue, Jan 22, 2019 at 8:07 PM Noralf Trønnes
> > wrote:
> >>
> >>
> >>
> >> Den 22.01.2019 10.32, skrev Daniel Vetter:
> >>> On Mon, Jan 21, 2019 at 01:21:46PM +0100, Noralf Trønnes wrote:
>
>
> Den 21.01.2019 10.55, skrev Daniel Vetter:
> > On Mon, Jan 21, 2019 at 10:10:14AM +0100, Daniel Vetter wrote:
> >> On Sun, Jan 20, 2019 at 12:43:08PM +0100, Noralf Trønnes wrote:
> >>> This adds resource managed (devres) versions of drm_dev_init() and
> >>> drm_dev_register().
> >>>
> >>> Also added is devm_drm_dev_register_with_fbdev() which sets up
> >>> generic
> >>> fbdev emulation as well.
> >>>
> >>> devm_drm_dev_register() isn't exported since there are no users.
> >>>
> >>> Signed-off-by: Noralf Trønnes
> >>
>
>
>
> >>> diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
> >>> index 381581b01d48..12129772be45 100644
> >>> --- a/drivers/gpu/drm/drm_drv.c
> >>> +++ b/drivers/gpu/drm/drm_drv.c
> >>> @@ -36,6 +36,7 @@
> >>>
> >>> #include
> >>> #include
> >>> +#include
> >>> #include
> >>>
> >>> #include "drm_crtc_internal.h"
> >>> @@ -871,6 +872,111 @@ void drm_dev_unregister(struct drm_device
> >>> *dev)
> >>> }
> >>> EXPORT_SYMBOL(drm_dev_unregister);
> >>>
> >>> +static void devm_drm_dev_init_release(void *data)
> >>> +{
> >>> + drm_dev_put(data);
> >
> > We need drm_dev_unplug() here, or this isn't safe.
>
> This function is only used to cover the error path if probe fails
> before
> devm_drm_dev_register() is called. devm_drm_dev_register_release() is
> the one that calls unplug. There are comments about this in the
> functions.
> >>>
> >>> I think I get a prize for being ignorant and blind :-/
> >>>
>
> >
> >>> +}
> >>> +
> >>> +/**
> >>> + * 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 released on driver detach. You must supply a
> >
> > I think a bit more clarity here would be good:
> >
> > "... automatically released on driver unbind by callind
> > drm_dev_unplug()."
> >
> >>> + * _driver.release callback to control the finalization
> >>> explicitly.
> >
> > I think a loud warning for these is in order:
> >
> > "WARNING:
> >
> > "In generally it is unsafe to use devm functions for drm structures
> > because the lifetimes of _device and the underlying do
> > not
> > match. This here works because it doesn't immediately free
> > anything, but
> > only calls drm_dev_unplug(), which internally decrements the
> > _device
> > refcount through drm_dev_put().
> >
> > "All other drm structures must still be explicitly released in the
> > _driver.release callback."
> >
> > While thinking about this I just realized that with this design we
> > have no
> > good place to call drm_atomic_helper_shutdown(). Which we need to,
> > or all
> > kinds of things will leak badly (connectors, fb, ...), but there's
> > no
> > place to call it:
> > - unbind is too early, since we haven't yet called drm_dev_unplug,
> > and the
> > drm_dev_unregister in there must be called _before_ we start to
> > shut
> > down anything.
> > - drm_driver.release is way too late.
> >
> > Ofc for a real hotunplug there's no point in shutting down the hw
> > (it's
> > already gone), but for a driver unload/unbind it would be nice if
> > this
> > happens automatically and in the right order.
> >
> > So not sure what to do here really.
>
> How about this change: (it breaks the rule of pulling helpers into
> the
> core, so maybe we should put the devm_ functions into the simple KMS
>
Re: devm actions and hw clenaup (was Re: [PATCH 01/11] drm: Add devm_drm_dev_init/register)
Den 24.01.2019 18.57, skrev Daniel Vetter:
> On Thu, Jan 24, 2019 at 6:46 PM Greg KH wrote:
>>
>> On Thu, Jan 24, 2019 at 11:43:12AM +0100, Daniel Vetter wrote:
>>> On Wed, Jan 23, 2019 at 11:54:07AM +0100, Noralf Trønnes wrote:
Den 22.01.2019 20.30, skrev Daniel Vetter:
> On Tue, Jan 22, 2019 at 8:07 PM Noralf Trønnes wrote:
>>
>>
>>
>> Den 22.01.2019 10.32, skrev Daniel Vetter:
>>> On Mon, Jan 21, 2019 at 01:21:46PM +0100, Noralf Trønnes wrote:
Den 21.01.2019 10.55, skrev Daniel Vetter:
> On Mon, Jan 21, 2019 at 10:10:14AM +0100, Daniel Vetter wrote:
>> On Sun, Jan 20, 2019 at 12:43:08PM +0100, Noralf Trønnes wrote:
>>> This adds resource managed (devres) versions of drm_dev_init() and
>>> drm_dev_register().
>>>
>>> Also added is devm_drm_dev_register_with_fbdev() which sets up
>>> generic
>>> fbdev emulation as well.
>>>
>>> devm_drm_dev_register() isn't exported since there are no users.
>>>
>>> Signed-off-by: Noralf Trønnes
>>
>>> diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
>>> index 381581b01d48..12129772be45 100644
>>> --- a/drivers/gpu/drm/drm_drv.c
>>> +++ b/drivers/gpu/drm/drm_drv.c
>>> @@ -36,6 +36,7 @@
>>>
>>> #include
>>> #include
>>> +#include
>>> #include
>>>
>>> #include "drm_crtc_internal.h"
>>> @@ -871,6 +872,111 @@ void drm_dev_unregister(struct drm_device
>>> *dev)
>>> }
>>> EXPORT_SYMBOL(drm_dev_unregister);
>>>
>>> +static void devm_drm_dev_init_release(void *data)
>>> +{
>>> + drm_dev_put(data);
>
> We need drm_dev_unplug() here, or this isn't safe.
This function is only used to cover the error path if probe fails
before
devm_drm_dev_register() is called. devm_drm_dev_register_release() is
the one that calls unplug. There are comments about this in the
functions.
>>>
>>> I think I get a prize for being ignorant and blind :-/
>>>
>
>>> +}
>>> +
>>> +/**
>>> + * 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 released on driver detach. You must supply a
>
> I think a bit more clarity here would be good:
>
> "... automatically released on driver unbind by callind
> drm_dev_unplug()."
>
>>> + * _driver.release callback to control the finalization
>>> explicitly.
>
> I think a loud warning for these is in order:
>
> "WARNING:
>
> "In generally it is unsafe to use devm functions for drm structures
> because the lifetimes of _device and the underlying do not
> match. This here works because it doesn't immediately free anything,
> but
> only calls drm_dev_unplug(), which internally decrements the
> _device
> refcount through drm_dev_put().
>
> "All other drm structures must still be explicitly released in the
> _driver.release callback."
>
> While thinking about this I just realized that with this design we
> have no
> good place to call drm_atomic_helper_shutdown(). Which we need to, or
> all
> kinds of things will leak badly (connectors, fb, ...), but there's no
> place to call it:
> - unbind is too early, since we haven't yet called drm_dev_unplug,
> and the
> drm_dev_unregister in there must be called _before_ we start to shut
> down anything.
> - drm_driver.release is way too late.
>
> Ofc for a real hotunplug there's no point in shutting down the hw
> (it's
> already gone), but for a driver unload/unbind it would be nice if this
> happens automatically and in the right order.
>
> So not sure what to do here really.
How about this change: (it breaks the rule of pulling helpers into the
core, so maybe we should put the devm_ functions into the simple KMS
helper instead?)
>>>
>>> Yeah smells a bit much like midlayer ... What would work is having a
>>> pile
>>> more devm_ helper functions, so that we onion-unwrap everything
>>> correctly,
>>> and in the right order. So:
>>>
>>> - devm_drm_dev_init (always does a drm_dev_put())
>>>
>>> - devm_drm_poll_enable (shuts down the poll helper
Re: devm actions and hw clenaup (was Re: [PATCH 01/11] drm: Add devm_drm_dev_init/register)
On Thu, Jan 24, 2019 at 6:46 PM Greg KH wrote:
>
> On Thu, Jan 24, 2019 at 11:43:12AM +0100, Daniel Vetter wrote:
> > On Wed, Jan 23, 2019 at 11:54:07AM +0100, Noralf Trønnes wrote:
> > >
> > >
> > > Den 22.01.2019 20.30, skrev Daniel Vetter:
> > > > On Tue, Jan 22, 2019 at 8:07 PM Noralf Trønnes
> > > > wrote:
> > > >>
> > > >>
> > > >>
> > > >> Den 22.01.2019 10.32, skrev Daniel Vetter:
> > > >>> On Mon, Jan 21, 2019 at 01:21:46PM +0100, Noralf Trønnes wrote:
> > >
> > >
> > > Den 21.01.2019 10.55, skrev Daniel Vetter:
> > > > On Mon, Jan 21, 2019 at 10:10:14AM +0100, Daniel Vetter wrote:
> > > >> On Sun, Jan 20, 2019 at 12:43:08PM +0100, Noralf Trønnes wrote:
> > > >>> This adds resource managed (devres) versions of drm_dev_init() and
> > > >>> drm_dev_register().
> > > >>>
> > > >>> Also added is devm_drm_dev_register_with_fbdev() which sets up
> > > >>> generic
> > > >>> fbdev emulation as well.
> > > >>>
> > > >>> devm_drm_dev_register() isn't exported since there are no users.
> > > >>>
> > > >>> Signed-off-by: Noralf Trønnes
> > > >>
> > >
> > >
> > >
> > > >>> diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
> > > >>> index 381581b01d48..12129772be45 100644
> > > >>> --- a/drivers/gpu/drm/drm_drv.c
> > > >>> +++ b/drivers/gpu/drm/drm_drv.c
> > > >>> @@ -36,6 +36,7 @@
> > > >>>
> > > >>> #include
> > > >>> #include
> > > >>> +#include
> > > >>> #include
> > > >>>
> > > >>> #include "drm_crtc_internal.h"
> > > >>> @@ -871,6 +872,111 @@ void drm_dev_unregister(struct drm_device
> > > >>> *dev)
> > > >>> }
> > > >>> EXPORT_SYMBOL(drm_dev_unregister);
> > > >>>
> > > >>> +static void devm_drm_dev_init_release(void *data)
> > > >>> +{
> > > >>> + drm_dev_put(data);
> > > >
> > > > We need drm_dev_unplug() here, or this isn't safe.
> > >
> > > This function is only used to cover the error path if probe fails
> > > before
> > > devm_drm_dev_register() is called. devm_drm_dev_register_release() is
> > > the one that calls unplug. There are comments about this in the
> > > functions.
> > > >>>
> > > >>> I think I get a prize for being ignorant and blind :-/
> > > >>>
> > >
> > > >
> > > >>> +}
> > > >>> +
> > > >>> +/**
> > > >>> + * 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 released on driver detach. You must supply a
> > > >
> > > > I think a bit more clarity here would be good:
> > > >
> > > > "... automatically released on driver unbind by callind
> > > > drm_dev_unplug()."
> > > >
> > > >>> + * _driver.release callback to control the finalization
> > > >>> explicitly.
> > > >
> > > > I think a loud warning for these is in order:
> > > >
> > > > "WARNING:
> > > >
> > > > "In generally it is unsafe to use devm functions for drm structures
> > > > because the lifetimes of _device and the underlying do
> > > > not
> > > > match. This here works because it doesn't immediately free
> > > > anything, but
> > > > only calls drm_dev_unplug(), which internally decrements the
> > > > _device
> > > > refcount through drm_dev_put().
> > > >
> > > > "All other drm structures must still be explicitly released in the
> > > > _driver.release callback."
> > > >
> > > > While thinking about this I just realized that with this design we
> > > > have no
> > > > good place to call drm_atomic_helper_shutdown(). Which we need to,
> > > > or all
> > > > kinds of things will leak badly (connectors, fb, ...), but there's
> > > > no
> > > > place to call it:
> > > > - unbind is too early, since we haven't yet called drm_dev_unplug,
> > > > and the
> > > > drm_dev_unregister in there must be called _before_ we start to
> > > > shut
> > > > down anything.
> > > > - drm_driver.release is way too late.
> > > >
> > > > Ofc for a real hotunplug there's no point in shutting down the hw
> > > > (it's
> > > > already gone), but for a driver unload/unbind it would be nice if
> > > > this
> > > > happens automatically and in the right order.
> > > >
> > > > So not sure what to do here really.
> > >
> > > How about this change: (it breaks the rule of pulling helpers into
> > > the
> > > core, so maybe we should put the devm_ functions into the simple KMS
> > > helper instead?)
> > > >>>
> > > >>> Yeah smells a bit much like midlayer ... What would work is having a
> > > >>> pile
Re: devm actions and hw clenaup (was Re: [PATCH 01/11] drm: Add devm_drm_dev_init/register)
On Thu, Jan 24, 2019 at 11:43:12AM +0100, Daniel Vetter wrote:
> On Wed, Jan 23, 2019 at 11:54:07AM +0100, Noralf Trønnes wrote:
> >
> >
> > Den 22.01.2019 20.30, skrev Daniel Vetter:
> > > On Tue, Jan 22, 2019 at 8:07 PM Noralf Trønnes wrote:
> > >>
> > >>
> > >>
> > >> Den 22.01.2019 10.32, skrev Daniel Vetter:
> > >>> On Mon, Jan 21, 2019 at 01:21:46PM +0100, Noralf Trønnes wrote:
> >
> >
> > Den 21.01.2019 10.55, skrev Daniel Vetter:
> > > On Mon, Jan 21, 2019 at 10:10:14AM +0100, Daniel Vetter wrote:
> > >> On Sun, Jan 20, 2019 at 12:43:08PM +0100, Noralf Trønnes wrote:
> > >>> This adds resource managed (devres) versions of drm_dev_init() and
> > >>> drm_dev_register().
> > >>>
> > >>> Also added is devm_drm_dev_register_with_fbdev() which sets up
> > >>> generic
> > >>> fbdev emulation as well.
> > >>>
> > >>> devm_drm_dev_register() isn't exported since there are no users.
> > >>>
> > >>> Signed-off-by: Noralf Trønnes
> > >>
> >
> >
> >
> > >>> diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
> > >>> index 381581b01d48..12129772be45 100644
> > >>> --- a/drivers/gpu/drm/drm_drv.c
> > >>> +++ b/drivers/gpu/drm/drm_drv.c
> > >>> @@ -36,6 +36,7 @@
> > >>>
> > >>> #include
> > >>> #include
> > >>> +#include
> > >>> #include
> > >>>
> > >>> #include "drm_crtc_internal.h"
> > >>> @@ -871,6 +872,111 @@ void drm_dev_unregister(struct drm_device
> > >>> *dev)
> > >>> }
> > >>> EXPORT_SYMBOL(drm_dev_unregister);
> > >>>
> > >>> +static void devm_drm_dev_init_release(void *data)
> > >>> +{
> > >>> + drm_dev_put(data);
> > >
> > > We need drm_dev_unplug() here, or this isn't safe.
> >
> > This function is only used to cover the error path if probe fails
> > before
> > devm_drm_dev_register() is called. devm_drm_dev_register_release() is
> > the one that calls unplug. There are comments about this in the
> > functions.
> > >>>
> > >>> I think I get a prize for being ignorant and blind :-/
> > >>>
> >
> > >
> > >>> +}
> > >>> +
> > >>> +/**
> > >>> + * 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 released on driver detach. You must supply a
> > >
> > > I think a bit more clarity here would be good:
> > >
> > > "... automatically released on driver unbind by callind
> > > drm_dev_unplug()."
> > >
> > >>> + * _driver.release callback to control the finalization
> > >>> explicitly.
> > >
> > > I think a loud warning for these is in order:
> > >
> > > "WARNING:
> > >
> > > "In generally it is unsafe to use devm functions for drm structures
> > > because the lifetimes of _device and the underlying do not
> > > match. This here works because it doesn't immediately free anything,
> > > but
> > > only calls drm_dev_unplug(), which internally decrements the
> > > _device
> > > refcount through drm_dev_put().
> > >
> > > "All other drm structures must still be explicitly released in the
> > > _driver.release callback."
> > >
> > > While thinking about this I just realized that with this design we
> > > have no
> > > good place to call drm_atomic_helper_shutdown(). Which we need to, or
> > > all
> > > kinds of things will leak badly (connectors, fb, ...), but there's no
> > > place to call it:
> > > - unbind is too early, since we haven't yet called drm_dev_unplug,
> > > and the
> > > drm_dev_unregister in there must be called _before_ we start to shut
> > > down anything.
> > > - drm_driver.release is way too late.
> > >
> > > Ofc for a real hotunplug there's no point in shutting down the hw
> > > (it's
> > > already gone), but for a driver unload/unbind it would be nice if this
> > > happens automatically and in the right order.
> > >
> > > So not sure what to do here really.
> >
> > How about this change: (it breaks the rule of pulling helpers into the
> > core, so maybe we should put the devm_ functions into the simple KMS
> > helper instead?)
> > >>>
> > >>> Yeah smells a bit much like midlayer ... What would work is having a
> > >>> pile
> > >>> more devm_ helper functions, so that we onion-unwrap everything
> > >>> correctly,
> > >>> and in the right order. So:
> > >>>
> > >>> - devm_drm_dev_init (always does a drm_dev_put())
> > >>>
> > >>> - devm_drm_poll_enable (shuts down the poll helper with a devm action)
> > >>>
> > >>> - devm_drm_mode_config_reset (does an atomic_helper_shutdown() as
devm actions and hw clenaup (was Re: [PATCH 01/11] drm: Add devm_drm_dev_init/register)
On Wed, Jan 23, 2019 at 11:54:07AM +0100, Noralf Trønnes wrote:
>
>
> Den 22.01.2019 20.30, skrev Daniel Vetter:
> > On Tue, Jan 22, 2019 at 8:07 PM Noralf Trønnes wrote:
> >>
> >>
> >>
> >> Den 22.01.2019 10.32, skrev Daniel Vetter:
> >>> On Mon, Jan 21, 2019 at 01:21:46PM +0100, Noralf Trønnes wrote:
>
>
> Den 21.01.2019 10.55, skrev Daniel Vetter:
> > On Mon, Jan 21, 2019 at 10:10:14AM +0100, Daniel Vetter wrote:
> >> On Sun, Jan 20, 2019 at 12:43:08PM +0100, Noralf Trønnes wrote:
> >>> This adds resource managed (devres) versions of drm_dev_init() and
> >>> drm_dev_register().
> >>>
> >>> Also added is devm_drm_dev_register_with_fbdev() which sets up generic
> >>> fbdev emulation as well.
> >>>
> >>> devm_drm_dev_register() isn't exported since there are no users.
> >>>
> >>> Signed-off-by: Noralf Trønnes
> >>
>
>
>
> >>> diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
> >>> index 381581b01d48..12129772be45 100644
> >>> --- a/drivers/gpu/drm/drm_drv.c
> >>> +++ b/drivers/gpu/drm/drm_drv.c
> >>> @@ -36,6 +36,7 @@
> >>>
> >>> #include
> >>> #include
> >>> +#include
> >>> #include
> >>>
> >>> #include "drm_crtc_internal.h"
> >>> @@ -871,6 +872,111 @@ void drm_dev_unregister(struct drm_device *dev)
> >>> }
> >>> EXPORT_SYMBOL(drm_dev_unregister);
> >>>
> >>> +static void devm_drm_dev_init_release(void *data)
> >>> +{
> >>> + drm_dev_put(data);
> >
> > We need drm_dev_unplug() here, or this isn't safe.
>
> This function is only used to cover the error path if probe fails before
> devm_drm_dev_register() is called. devm_drm_dev_register_release() is
> the one that calls unplug. There are comments about this in the
> functions.
> >>>
> >>> I think I get a prize for being ignorant and blind :-/
> >>>
>
> >
> >>> +}
> >>> +
> >>> +/**
> >>> + * 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 released on driver detach. You must supply a
> >
> > I think a bit more clarity here would be good:
> >
> > "... automatically released on driver unbind by callind
> > drm_dev_unplug()."
> >
> >>> + * _driver.release callback to control the finalization
> >>> explicitly.
> >
> > I think a loud warning for these is in order:
> >
> > "WARNING:
> >
> > "In generally it is unsafe to use devm functions for drm structures
> > because the lifetimes of _device and the underlying do not
> > match. This here works because it doesn't immediately free anything, but
> > only calls drm_dev_unplug(), which internally decrements the _device
> > refcount through drm_dev_put().
> >
> > "All other drm structures must still be explicitly released in the
> > _driver.release callback."
> >
> > While thinking about this I just realized that with this design we have
> > no
> > good place to call drm_atomic_helper_shutdown(). Which we need to, or
> > all
> > kinds of things will leak badly (connectors, fb, ...), but there's no
> > place to call it:
> > - unbind is too early, since we haven't yet called drm_dev_unplug, and
> > the
> > drm_dev_unregister in there must be called _before_ we start to shut
> > down anything.
> > - drm_driver.release is way too late.
> >
> > Ofc for a real hotunplug there's no point in shutting down the hw (it's
> > already gone), but for a driver unload/unbind it would be nice if this
> > happens automatically and in the right order.
> >
> > So not sure what to do here really.
>
> How about this change: (it breaks the rule of pulling helpers into the
> core, so maybe we should put the devm_ functions into the simple KMS
> helper instead?)
> >>>
> >>> Yeah smells a bit much like midlayer ... What would work is having a pile
> >>> more devm_ helper functions, so that we onion-unwrap everything correctly,
> >>> and in the right order. So:
> >>>
> >>> - devm_drm_dev_init (always does a drm_dev_put())
> >>>
> >>> - devm_drm_poll_enable (shuts down the poll helper with a devm action)
> >>>
> >>> - devm_drm_mode_config_reset (does an atomic_helper_shutdown() as it's
> >>> cleanup action)
> >>>
> >>> - devm_drm_dev_register (grabs an additional drm_dev_get() reference so it
> >>> can call drm_dev_unplug() unconditionally).
> >>>
> >>
> >> Beautiful! I really like this, it's very flexible.
> >>
> >> Where should devm_drm_mode_config_reset() live? It will pull in the
> >> atomic helper...
> >
> > I think a new drm_devm.c helper would be nice for all
Re: [PATCH 01/11] drm: Add devm_drm_dev_init/register
Den 22.01.2019 20.30, skrev Daniel Vetter:
> On Tue, Jan 22, 2019 at 8:07 PM Noralf Trønnes wrote:
>>
>>
>>
>> Den 22.01.2019 10.32, skrev Daniel Vetter:
>>> On Mon, Jan 21, 2019 at 01:21:46PM +0100, Noralf Trønnes wrote:
Den 21.01.2019 10.55, skrev Daniel Vetter:
> On Mon, Jan 21, 2019 at 10:10:14AM +0100, Daniel Vetter wrote:
>> On Sun, Jan 20, 2019 at 12:43:08PM +0100, Noralf Trønnes wrote:
>>> This adds resource managed (devres) versions of drm_dev_init() and
>>> drm_dev_register().
>>>
>>> Also added is devm_drm_dev_register_with_fbdev() which sets up generic
>>> fbdev emulation as well.
>>>
>>> devm_drm_dev_register() isn't exported since there are no users.
>>>
>>> Signed-off-by: Noralf Trønnes
>>
>>> diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
>>> index 381581b01d48..12129772be45 100644
>>> --- a/drivers/gpu/drm/drm_drv.c
>>> +++ b/drivers/gpu/drm/drm_drv.c
>>> @@ -36,6 +36,7 @@
>>>
>>> #include
>>> #include
>>> +#include
>>> #include
>>>
>>> #include "drm_crtc_internal.h"
>>> @@ -871,6 +872,111 @@ void drm_dev_unregister(struct drm_device *dev)
>>> }
>>> EXPORT_SYMBOL(drm_dev_unregister);
>>>
>>> +static void devm_drm_dev_init_release(void *data)
>>> +{
>>> + drm_dev_put(data);
>
> We need drm_dev_unplug() here, or this isn't safe.
This function is only used to cover the error path if probe fails before
devm_drm_dev_register() is called. devm_drm_dev_register_release() is
the one that calls unplug. There are comments about this in the functions.
>>>
>>> I think I get a prize for being ignorant and blind :-/
>>>
>
>>> +}
>>> +
>>> +/**
>>> + * 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 released on driver detach. You must supply a
>
> I think a bit more clarity here would be good:
>
> "... automatically released on driver unbind by callind drm_dev_unplug()."
>
>>> + * _driver.release callback to control the finalization explicitly.
>
> I think a loud warning for these is in order:
>
> "WARNING:
>
> "In generally it is unsafe to use devm functions for drm structures
> because the lifetimes of _device and the underlying do not
> match. This here works because it doesn't immediately free anything, but
> only calls drm_dev_unplug(), which internally decrements the _device
> refcount through drm_dev_put().
>
> "All other drm structures must still be explicitly released in the
> _driver.release callback."
>
> While thinking about this I just realized that with this design we have no
> good place to call drm_atomic_helper_shutdown(). Which we need to, or all
> kinds of things will leak badly (connectors, fb, ...), but there's no
> place to call it:
> - unbind is too early, since we haven't yet called drm_dev_unplug, and the
> drm_dev_unregister in there must be called _before_ we start to shut
> down anything.
> - drm_driver.release is way too late.
>
> Ofc for a real hotunplug there's no point in shutting down the hw (it's
> already gone), but for a driver unload/unbind it would be nice if this
> happens automatically and in the right order.
>
> So not sure what to do here really.
How about this change: (it breaks the rule of pulling helpers into the
core, so maybe we should put the devm_ functions into the simple KMS
helper instead?)
>>>
>>> Yeah smells a bit much like midlayer ... What would work is having a pile
>>> more devm_ helper functions, so that we onion-unwrap everything correctly,
>>> and in the right order. So:
>>>
>>> - devm_drm_dev_init (always does a drm_dev_put())
>>>
>>> - devm_drm_poll_enable (shuts down the poll helper with a devm action)
>>>
>>> - devm_drm_mode_config_reset (does an atomic_helper_shutdown() as it's
>>> cleanup action)
>>>
>>> - devm_drm_dev_register (grabs an additional drm_dev_get() reference so it
>>> can call drm_dev_unplug() unconditionally).
>>>
>>
>> Beautiful! I really like this, it's very flexible.
>>
>> Where should devm_drm_mode_config_reset() live? It will pull in the
>> atomic helper...
>
> I think a new drm_devm.c helper would be nice for all this stuff.
> Especially since you can't freely mix devm-based setup/cleanup with
> normal cleanup I think it'd be good to have it all together in one
> place. And perhaps even a code example in the DOC: overview.
>
>>> We'd need to make sure some of the cleanup actions dtrt when the device is
>>> gone, but I think we can achieve that by liberally sprinkling
>>>
Re: [PATCH 01/11] drm: Add devm_drm_dev_init/register
On Tue, Jan 22, 2019 at 8:07 PM Noralf Trønnes wrote:
>
>
>
> Den 22.01.2019 10.32, skrev Daniel Vetter:
> > On Mon, Jan 21, 2019 at 01:21:46PM +0100, Noralf Trønnes wrote:
> >>
> >>
> >> Den 21.01.2019 10.55, skrev Daniel Vetter:
> >>> On Mon, Jan 21, 2019 at 10:10:14AM +0100, Daniel Vetter wrote:
> On Sun, Jan 20, 2019 at 12:43:08PM +0100, Noralf Trønnes wrote:
> > This adds resource managed (devres) versions of drm_dev_init() and
> > drm_dev_register().
> >
> > Also added is devm_drm_dev_register_with_fbdev() which sets up generic
> > fbdev emulation as well.
> >
> > devm_drm_dev_register() isn't exported since there are no users.
> >
> > Signed-off-by: Noralf Trønnes
>
> >>
> >>
> >>
> > diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
> > index 381581b01d48..12129772be45 100644
> > --- a/drivers/gpu/drm/drm_drv.c
> > +++ b/drivers/gpu/drm/drm_drv.c
> > @@ -36,6 +36,7 @@
> >
> > #include
> > #include
> > +#include
> > #include
> >
> > #include "drm_crtc_internal.h"
> > @@ -871,6 +872,111 @@ void drm_dev_unregister(struct drm_device *dev)
> > }
> > EXPORT_SYMBOL(drm_dev_unregister);
> >
> > +static void devm_drm_dev_init_release(void *data)
> > +{
> > + drm_dev_put(data);
> >>>
> >>> We need drm_dev_unplug() here, or this isn't safe.
> >>
> >> This function is only used to cover the error path if probe fails before
> >> devm_drm_dev_register() is called. devm_drm_dev_register_release() is
> >> the one that calls unplug. There are comments about this in the functions.
> >
> > I think I get a prize for being ignorant and blind :-/
> >
> >>
> >>>
> > +}
> > +
> > +/**
> > + * 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 released on driver detach. You must supply a
> >>>
> >>> I think a bit more clarity here would be good:
> >>>
> >>> "... automatically released on driver unbind by callind drm_dev_unplug()."
> >>>
> > + * _driver.release callback to control the finalization explicitly.
> >>>
> >>> I think a loud warning for these is in order:
> >>>
> >>> "WARNING:
> >>>
> >>> "In generally it is unsafe to use devm functions for drm structures
> >>> because the lifetimes of _device and the underlying do not
> >>> match. This here works because it doesn't immediately free anything, but
> >>> only calls drm_dev_unplug(), which internally decrements the _device
> >>> refcount through drm_dev_put().
> >>>
> >>> "All other drm structures must still be explicitly released in the
> >>> _driver.release callback."
> >>>
> >>> While thinking about this I just realized that with this design we have no
> >>> good place to call drm_atomic_helper_shutdown(). Which we need to, or all
> >>> kinds of things will leak badly (connectors, fb, ...), but there's no
> >>> place to call it:
> >>> - unbind is too early, since we haven't yet called drm_dev_unplug, and the
> >>> drm_dev_unregister in there must be called _before_ we start to shut
> >>> down anything.
> >>> - drm_driver.release is way too late.
> >>>
> >>> Ofc for a real hotunplug there's no point in shutting down the hw (it's
> >>> already gone), but for a driver unload/unbind it would be nice if this
> >>> happens automatically and in the right order.
> >>>
> >>> So not sure what to do here really.
> >>
> >> How about this change: (it breaks the rule of pulling helpers into the
> >> core, so maybe we should put the devm_ functions into the simple KMS
> >> helper instead?)
> >
> > Yeah smells a bit much like midlayer ... What would work is having a pile
> > more devm_ helper functions, so that we onion-unwrap everything correctly,
> > and in the right order. So:
> >
> > - devm_drm_dev_init (always does a drm_dev_put())
> >
> > - devm_drm_poll_enable (shuts down the poll helper with a devm action)
> >
> > - devm_drm_mode_config_reset (does an atomic_helper_shutdown() as it's
> > cleanup action)
> >
> > - devm_drm_dev_register (grabs an additional drm_dev_get() reference so it
> > can call drm_dev_unplug() unconditionally).
> >
>
> Beautiful! I really like this, it's very flexible.
>
> Where should devm_drm_mode_config_reset() live? It will pull in the
> atomic helper...
I think a new drm_devm.c helper would be nice for all this stuff.
Especially since you can't freely mix devm-based setup/cleanup with
normal cleanup I think it'd be good to have it all together in one
place. And perhaps even a code example in the DOC: overview.
> > We'd need to make sure some of the cleanup actions dtrt when the device is
> > gone, but I think we can achieve that by liberally sprinkling
> > drm_dev_enter/exit over them, e.g. the the cleanup action for
> >
Re: [PATCH 01/11] drm: Add devm_drm_dev_init/register
Den 22.01.2019 10.32, skrev Daniel Vetter:
> On Mon, Jan 21, 2019 at 01:21:46PM +0100, Noralf Trønnes wrote:
>>
>>
>> Den 21.01.2019 10.55, skrev Daniel Vetter:
>>> On Mon, Jan 21, 2019 at 10:10:14AM +0100, Daniel Vetter wrote:
On Sun, Jan 20, 2019 at 12:43:08PM +0100, Noralf Trønnes wrote:
> This adds resource managed (devres) versions of drm_dev_init() and
> drm_dev_register().
>
> Also added is devm_drm_dev_register_with_fbdev() which sets up generic
> fbdev emulation as well.
>
> devm_drm_dev_register() isn't exported since there are no users.
>
> Signed-off-by: Noralf Trønnes
>>
>>
>>
> diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
> index 381581b01d48..12129772be45 100644
> --- a/drivers/gpu/drm/drm_drv.c
> +++ b/drivers/gpu/drm/drm_drv.c
> @@ -36,6 +36,7 @@
>
> #include
> #include
> +#include
> #include
>
> #include "drm_crtc_internal.h"
> @@ -871,6 +872,111 @@ void drm_dev_unregister(struct drm_device *dev)
> }
> EXPORT_SYMBOL(drm_dev_unregister);
>
> +static void devm_drm_dev_init_release(void *data)
> +{
> + drm_dev_put(data);
>>>
>>> We need drm_dev_unplug() here, or this isn't safe.
>>
>> This function is only used to cover the error path if probe fails before
>> devm_drm_dev_register() is called. devm_drm_dev_register_release() is
>> the one that calls unplug. There are comments about this in the functions.
>
> I think I get a prize for being ignorant and blind :-/
>
>>
>>>
> +}
> +
> +/**
> + * 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 released on driver detach. You must supply a
>>>
>>> I think a bit more clarity here would be good:
>>>
>>> "... automatically released on driver unbind by callind drm_dev_unplug()."
>>>
> + * _driver.release callback to control the finalization explicitly.
>>>
>>> I think a loud warning for these is in order:
>>>
>>> "WARNING:
>>>
>>> "In generally it is unsafe to use devm functions for drm structures
>>> because the lifetimes of _device and the underlying do not
>>> match. This here works because it doesn't immediately free anything, but
>>> only calls drm_dev_unplug(), which internally decrements the _device
>>> refcount through drm_dev_put().
>>>
>>> "All other drm structures must still be explicitly released in the
>>> _driver.release callback."
>>>
>>> While thinking about this I just realized that with this design we have no
>>> good place to call drm_atomic_helper_shutdown(). Which we need to, or all
>>> kinds of things will leak badly (connectors, fb, ...), but there's no
>>> place to call it:
>>> - unbind is too early, since we haven't yet called drm_dev_unplug, and the
>>> drm_dev_unregister in there must be called _before_ we start to shut
>>> down anything.
>>> - drm_driver.release is way too late.
>>>
>>> Ofc for a real hotunplug there's no point in shutting down the hw (it's
>>> already gone), but for a driver unload/unbind it would be nice if this
>>> happens automatically and in the right order.
>>>
>>> So not sure what to do here really.
>>
>> How about this change: (it breaks the rule of pulling helpers into the
>> core, so maybe we should put the devm_ functions into the simple KMS
>> helper instead?)
>
> Yeah smells a bit much like midlayer ... What would work is having a pile
> more devm_ helper functions, so that we onion-unwrap everything correctly,
> and in the right order. So:
>
> - devm_drm_dev_init (always does a drm_dev_put())
>
> - devm_drm_poll_enable (shuts down the poll helper with a devm action)
>
> - devm_drm_mode_config_reset (does an atomic_helper_shutdown() as it's
> cleanup action)
>
> - devm_drm_dev_register (grabs an additional drm_dev_get() reference so it
> can call drm_dev_unplug() unconditionally).
>
Beautiful! I really like this, it's very flexible.
Where should devm_drm_mode_config_reset() live? It will pull in the
atomic helper...
> We'd need to make sure some of the cleanup actions dtrt when the device is
> gone, but I think we can achieve that by liberally sprinkling
> drm_dev_enter/exit over them, e.g. the the cleanup action for
> drm_mode_config_reset would be:
>
> {
> if (drm_dev_enter())
> return;
>
> drm_atomic_helper_shutdown();
>
> drm_dev_exit();
> }
>
drm_dev_enter() can only be used to check whether the drm_device is
registered or not, it doesn't say anything about the state of the parent
device.
All we know is that the device is being unbound from the driver, we
don't know if it's the device that's being removed or if it's the driver
that's unregistered.
I have looked at the various call chains:
driver_unregister ->
Re: [PATCH 01/11] drm: Add devm_drm_dev_init/register
On Sun, Jan 20, 2019 at 12:43:08PM +0100, Noralf Trønnes wrote:
> This adds resource managed (devres) versions of drm_dev_init() and
> drm_dev_register().
>
> Also added is devm_drm_dev_register_with_fbdev() which sets up generic
> fbdev emulation as well.
>
> devm_drm_dev_register() isn't exported since there are no users.
>
> Signed-off-by: Noralf Trønnes
> ---
> Documentation/driver-model/devres.txt | 4 +
> drivers/gpu/drm/drm_drv.c | 106 ++
> include/drm/drm_drv.h | 6 ++
> 3 files changed, 116 insertions(+)
>
> diff --git a/Documentation/driver-model/devres.txt
> b/Documentation/driver-model/devres.txt
> index b277cafce71e..6eebc28d4c21 100644
> --- a/Documentation/driver-model/devres.txt
> +++ b/Documentation/driver-model/devres.txt
> @@ -254,6 +254,10 @@ DMA
>dmam_pool_create()
>dmam_pool_destroy()
>
> +DRM
> + devm_drm_dev_init()
> + devm_drm_dev_register_with_fbdev()
> +
> GPIO
>devm_gpiod_get()
>devm_gpiod_get_index()
> diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
> index 381581b01d48..12129772be45 100644
> --- a/drivers/gpu/drm/drm_drv.c
> +++ b/drivers/gpu/drm/drm_drv.c
> @@ -36,6 +36,7 @@
>
> #include
> #include
> +#include
> #include
>
> #include "drm_crtc_internal.h"
> @@ -871,6 +872,111 @@ void drm_dev_unregister(struct drm_device *dev)
> }
> EXPORT_SYMBOL(drm_dev_unregister);
>
> +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 released on driver detach. You must supply a
> + * _driver.release callback to control the finalization explicitly.
> + *
> + * Note: This function must be used together with
> + * devm_drm_dev_register_with_fbdev().
> + *
> + * 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)
> +{
> + int ret;
> +
> + if (WARN_ON(!parent || !driver->release))
> + return -EINVAL;
> +
> + ret = drm_dev_init(dev, driver, parent);
> + if (ret)
> + return ret;
> +
> + /*
> + * This is a temporary release action that is used if probing fails
> + * before devm_drm_dev_register() is called.
> + */
> + ret = devm_add_action(parent, devm_drm_dev_init_release, dev);
> + if (ret)
> + devm_drm_dev_init_release(dev);
> +
> + return ret;
> +}
> +EXPORT_SYMBOL(devm_drm_dev_init);
> +
> +static void devm_drm_dev_register_release(void *data)
> +{
> + drm_dev_unplug(data);
> +}
> +
> +static int devm_drm_dev_register(struct drm_device *dev)
> +{
> + int ret;
> +
> + ret = drm_dev_register(dev, 0);
> + if (ret)
> + return ret;
> +
> + /*
> + * This has now served it's purpose, remove it to not mess up ref
> + * counting.
> + */
> + devm_remove_action(dev->dev, devm_drm_dev_init_release, dev);
> +
> + ret = devm_add_action(dev->dev, devm_drm_dev_register_release, dev);
If this fails I think the cleanup would go wrong. I think simpler if you
never remove the devm action from dev_init, and just grab an additional
drm_dev_get() reference here for drm_dev_unplug. We also need that for
correct onion cleanup in, so that for a normal unbind/driver unload we can
still do:
1. drm_dev_unregister() through drm_dev_unplug()
2. drm_atomic_helper_shutdown() and similar things (needs the drm_device
to still be around)
3. drm_dev_put()
I think that'll give us the cleaner onion unwrapping in the
unload/hotunplug/error case cleanup cases.
Also, this allows drivers to start using devm_drm_dev_init without having
to use devm_drm_dev_register(), which they have to do if they still have
non-devm-ified things in step 2 above in there unbind callback.
-Daniel
> + if (ret)
> + devm_drm_dev_register_release(dev);
> +
> + return ret;
> +}
> +
> +/**
> + * devm_drm_dev_register_with_fbdev - Resource managed drm_dev_register()
> + *including generic fbdev emulation
> + * @dev: DRM device to register
> + * @fbdev_bpp: Preferred bits per pixel for fbdev (optional)
> + *
> + * Managed drm_dev_register() that also calls drm_fbdev_generic_setup().
> + * The DRM device registered with this function is automatically
> unregistered on
> + * driver detach using drm_dev_unplug().
> + *
> + * Note: This function must be used together with devm_drm_dev_init().
> + *
> + * For testing driver detach can be triggered manually by writing to the
> driver
> + * 'unbind' file.
> + *
> + * RETURNS:
> + * 0 on success, negative error code on failure.
> + */
> +int
Re: [PATCH 01/11] drm: Add devm_drm_dev_init/register
On Mon, Jan 21, 2019 at 01:21:46PM +0100, Noralf Trønnes wrote:
>
>
> Den 21.01.2019 10.55, skrev Daniel Vetter:
> > On Mon, Jan 21, 2019 at 10:10:14AM +0100, Daniel Vetter wrote:
> >> On Sun, Jan 20, 2019 at 12:43:08PM +0100, Noralf Trønnes wrote:
> >>> This adds resource managed (devres) versions of drm_dev_init() and
> >>> drm_dev_register().
> >>>
> >>> Also added is devm_drm_dev_register_with_fbdev() which sets up generic
> >>> fbdev emulation as well.
> >>>
> >>> devm_drm_dev_register() isn't exported since there are no users.
> >>>
> >>> Signed-off-by: Noralf Trønnes
> >>
>
>
>
> >>> diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
> >>> index 381581b01d48..12129772be45 100644
> >>> --- a/drivers/gpu/drm/drm_drv.c
> >>> +++ b/drivers/gpu/drm/drm_drv.c
> >>> @@ -36,6 +36,7 @@
> >>>
> >>> #include
> >>> #include
> >>> +#include
> >>> #include
> >>>
> >>> #include "drm_crtc_internal.h"
> >>> @@ -871,6 +872,111 @@ void drm_dev_unregister(struct drm_device *dev)
> >>> }
> >>> EXPORT_SYMBOL(drm_dev_unregister);
> >>>
> >>> +static void devm_drm_dev_init_release(void *data)
> >>> +{
> >>> + drm_dev_put(data);
> >
> > We need drm_dev_unplug() here, or this isn't safe.
>
> This function is only used to cover the error path if probe fails before
> devm_drm_dev_register() is called. devm_drm_dev_register_release() is
> the one that calls unplug. There are comments about this in the functions.
I think I get a prize for being ignorant and blind :-/
>
> >
> >>> +}
> >>> +
> >>> +/**
> >>> + * 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 released on driver detach. You must supply a
> >
> > I think a bit more clarity here would be good:
> >
> > "... automatically released on driver unbind by callind drm_dev_unplug()."
> >
> >>> + * _driver.release callback to control the finalization explicitly.
> >
> > I think a loud warning for these is in order:
> >
> > "WARNING:
> >
> > "In generally it is unsafe to use devm functions for drm structures
> > because the lifetimes of _device and the underlying do not
> > match. This here works because it doesn't immediately free anything, but
> > only calls drm_dev_unplug(), which internally decrements the _device
> > refcount through drm_dev_put().
> >
> > "All other drm structures must still be explicitly released in the
> > _driver.release callback."
> >
> > While thinking about this I just realized that with this design we have no
> > good place to call drm_atomic_helper_shutdown(). Which we need to, or all
> > kinds of things will leak badly (connectors, fb, ...), but there's no
> > place to call it:
> > - unbind is too early, since we haven't yet called drm_dev_unplug, and the
> > drm_dev_unregister in there must be called _before_ we start to shut
> > down anything.
> > - drm_driver.release is way too late.
> >
> > Ofc for a real hotunplug there's no point in shutting down the hw (it's
> > already gone), but for a driver unload/unbind it would be nice if this
> > happens automatically and in the right order.
> >
> > So not sure what to do here really.
>
> How about this change: (it breaks the rule of pulling helpers into the
> core, so maybe we should put the devm_ functions into the simple KMS
> helper instead?)
Yeah smells a bit much like midlayer ... What would work is having a pile
more devm_ helper functions, so that we onion-unwrap everything correctly,
and in the right order. So:
- devm_drm_dev_init (always does a drm_dev_put())
- devm_drm_poll_enable (shuts down the poll helper with a devm action)
- devm_drm_mode_config_reset (does an atomic_helper_shutdown() as it's cleanup
action)
- devm_drm_dev_register (grabs an additional drm_dev_get() reference so it
can call drm_dev_unplug() unconditionally).
We'd need to make sure some of the cleanup actions dtrt when the device is
gone, but I think we can achieve that by liberally sprinkling
drm_dev_enter/exit over them, e.g. the the cleanup action for
drm_mode_config_reset would be:
{
if (drm_dev_enter())
return;
drm_atomic_helper_shutdown();
drm_dev_exit();
}
This would be analog to your shutdown parameter below.
Essentially I think we can only do the drm_dev_unplug autocleanup in a
given driver from devm_drm_dev_register iff all the cleanup actions have
been devm-ified, and there's nothing left in it's unbind callback. Because
if anything is left in its unbind callback that's a bug, since
drm_dev_unregister() (called through drm_dev_unplug) is the very first (or
at least one of the very first, before we start cleanup up) functions that
need to be called.
-Daniel
>
> diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
> index 12129772be45..7ed9550baff6 100644
> ---
Re: [PATCH 01/11] drm: Add devm_drm_dev_init/register
Den 21.01.2019 07.11, skrev Sam Ravnborg:
> Hi Noralf.
>
> On Sun, Jan 20, 2019 at 12:43:08PM +0100, Noralf Trønnes wrote:
>> This adds resource managed (devres) versions of drm_dev_init() and
>> drm_dev_register().
>>
>> Also added is devm_drm_dev_register_with_fbdev() which sets up generic
>> fbdev emulation as well.
>>
>> devm_drm_dev_register() isn't exported since there are no users.
> Is it relevant to use it outside this patchset - then it should be
> documented/exported now to enable use.
As a rule we don't export functions that doesn't have any users. It can
be exported when a user shows up. But maybe it would make sense to put
all the documentation in devm_drm_dev_register() instead of duplicating
it all in the two functions when they are both in use. Or it can be
moved to devm_drm_dev_register() when it gets a user. I'm good with
either, don't know what Daniel prefers.
>
>>
>> Signed-off-by: Noralf Trønnes
>> ---
>> Documentation/driver-model/devres.txt | 4 +
>> drivers/gpu/drm/drm_drv.c | 106 ++
>> include/drm/drm_drv.h | 6 ++
>> 3 files changed, 116 insertions(+)
>>
>> diff --git a/Documentation/driver-model/devres.txt
>> b/Documentation/driver-model/devres.txt
>> index b277cafce71e..6eebc28d4c21 100644
>> --- a/Documentation/driver-model/devres.txt
>> +++ b/Documentation/driver-model/devres.txt
>> @@ -254,6 +254,10 @@ DMA
>>dmam_pool_create()
>>dmam_pool_destroy()
>>
>> +DRM
>> + devm_drm_dev_init()
>> + devm_drm_dev_register_with_fbdev()
>> +
>> GPIO
>>devm_gpiod_get()
>>devm_gpiod_get_index()
>> diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
>> index 381581b01d48..12129772be45 100644
>> --- a/drivers/gpu/drm/drm_drv.c
>> +++ b/drivers/gpu/drm/drm_drv.c
>> @@ -36,6 +36,7 @@
>>
>> #include
>> #include
>> +#include
>> #include
>>
>> #include "drm_crtc_internal.h"
>> @@ -871,6 +872,111 @@ void drm_dev_unregister(struct drm_device *dev)
>> }
>> EXPORT_SYMBOL(drm_dev_unregister);
>>
>> +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 released on driver detach. You must supply a
>> + * _driver.release callback to control the finalization explicitly.
>> + *
>> + * Note: This function must be used together with
>> + * devm_drm_dev_register_with_fbdev().
> *must* be used, or can be used?
> Reading the code this looks like something drivers that do not offer
> fbdev emulation could also benefit from.
They must be used together to not break ref counting. When
devm_drm_dev_register() gets a user it will read:
* Note: This function must be used together with
* devm_drm_dev_register() or devm_drm_dev_register_with_fbdev().
Noralf.
>
> With the two comments processed this has:
> Reviewed-by: Sam Ravnborg
>
> Sam
>
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 01/11] drm: Add devm_drm_dev_init/register
Den 21.01.2019 10.55, skrev Daniel Vetter:
> On Mon, Jan 21, 2019 at 10:10:14AM +0100, Daniel Vetter wrote:
>> On Sun, Jan 20, 2019 at 12:43:08PM +0100, Noralf Trønnes wrote:
>>> This adds resource managed (devres) versions of drm_dev_init() and
>>> drm_dev_register().
>>>
>>> Also added is devm_drm_dev_register_with_fbdev() which sets up generic
>>> fbdev emulation as well.
>>>
>>> devm_drm_dev_register() isn't exported since there are no users.
>>>
>>> Signed-off-by: Noralf Trønnes
>>
>>> diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
>>> index 381581b01d48..12129772be45 100644
>>> --- a/drivers/gpu/drm/drm_drv.c
>>> +++ b/drivers/gpu/drm/drm_drv.c
>>> @@ -36,6 +36,7 @@
>>>
>>> #include
>>> #include
>>> +#include
>>> #include
>>>
>>> #include "drm_crtc_internal.h"
>>> @@ -871,6 +872,111 @@ void drm_dev_unregister(struct drm_device *dev)
>>> }
>>> EXPORT_SYMBOL(drm_dev_unregister);
>>>
>>> +static void devm_drm_dev_init_release(void *data)
>>> +{
>>> + drm_dev_put(data);
>
> We need drm_dev_unplug() here, or this isn't safe.
This function is only used to cover the error path if probe fails before
devm_drm_dev_register() is called. devm_drm_dev_register_release() is
the one that calls unplug. There are comments about this in the functions.
>
>>> +}
>>> +
>>> +/**
>>> + * 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 released on driver detach. You must supply a
>
> I think a bit more clarity here would be good:
>
> "... automatically released on driver unbind by callind drm_dev_unplug()."
>
>>> + * _driver.release callback to control the finalization explicitly.
>
> I think a loud warning for these is in order:
>
> "WARNING:
>
> "In generally it is unsafe to use devm functions for drm structures
> because the lifetimes of _device and the underlying do not
> match. This here works because it doesn't immediately free anything, but
> only calls drm_dev_unplug(), which internally decrements the _device
> refcount through drm_dev_put().
>
> "All other drm structures must still be explicitly released in the
> _driver.release callback."
>
> While thinking about this I just realized that with this design we have no
> good place to call drm_atomic_helper_shutdown(). Which we need to, or all
> kinds of things will leak badly (connectors, fb, ...), but there's no
> place to call it:
> - unbind is too early, since we haven't yet called drm_dev_unplug, and the
> drm_dev_unregister in there must be called _before_ we start to shut
> down anything.
> - drm_driver.release is way too late.
>
> Ofc for a real hotunplug there's no point in shutting down the hw (it's
> already gone), but for a driver unload/unbind it would be nice if this
> happens automatically and in the right order.
>
> So not sure what to do here really.
How about this change: (it breaks the rule of pulling helpers into the
core, so maybe we should put the devm_ functions into the simple KMS
helper instead?)
diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
index 12129772be45..7ed9550baff6 100644
--- a/drivers/gpu/drm/drm_drv.c
+++ b/drivers/gpu/drm/drm_drv.c
@@ -34,7 +34,9 @@
#include
#include
+#include
#include
+#include
#include
#include
#include
@@ -355,17 +357,7 @@ void drm_dev_exit(int idx)
}
EXPORT_SYMBOL(drm_dev_exit);
-/**
- * drm_dev_unplug - unplug a DRM device
- * @dev: DRM device
- *
- * This unplugs a hotpluggable DRM device, which makes it inaccessible to
- * userspace operations. Entry-points can use drm_dev_enter() and
- * drm_dev_exit() to protect device resources in a race free manner. This
- * essentially unregisters the device like drm_dev_unregister(), but can be
- * called while there are still open users of @dev.
- */
-void drm_dev_unplug(struct drm_device *dev)
+static void __drm_dev_unplug(struct drm_device *dev, bool shutdown)
{
/*
* After synchronizing any critical read section is guaranteed
to see
@@ -378,11 +370,32 @@ void drm_dev_unplug(struct drm_device *dev)
drm_dev_unregister(dev);
+ if (shutdown)
+ drm_kms_helper_poll_fini(dev);
+
mutex_lock(_global_mutex);
- if (dev->open_count == 0)
+ if (dev->open_count == 0) {
+ if (shutdown)
+ drm_atomic_helper_shutdown(dev);
drm_dev_put(dev);
+ }
mutex_unlock(_global_mutex);
}
+
+/**
+ * drm_dev_unplug - unplug a DRM device
+ * @dev: DRM device
+ *
+ * This unplugs a hotpluggable DRM device, which makes it inaccessible to
+ * userspace operations. Entry-points can use drm_dev_enter() and
+ * drm_dev_exit() to protect device resources in a race free manner. This
+ * essentially unregisters the device like drm_dev_unregister(), but can be
+ * called
Re: [PATCH 01/11] drm: Add devm_drm_dev_init/register
On Mon, Jan 21, 2019 at 10:10:14AM +0100, Daniel Vetter wrote:
> On Sun, Jan 20, 2019 at 12:43:08PM +0100, Noralf Trønnes wrote:
> > This adds resource managed (devres) versions of drm_dev_init() and
> > drm_dev_register().
> >
> > Also added is devm_drm_dev_register_with_fbdev() which sets up generic
> > fbdev emulation as well.
> >
> > devm_drm_dev_register() isn't exported since there are no users.
> >
> > Signed-off-by: Noralf Trønnes
>
> devm_ considered harmful unfortunately. You cannot allocate any structures
> which might outlive the lifetime of your device using devm_ on the device.
> Since drm_device has it's own lifetime, that structure and _everything_ we
> allocate tied to it (i.e. drm_encoder/plane/connector/crtc/whatever)
> cannot be allocated using devm_ infrastructure tied to the underlying
> parent device.
>
> Yes this means almost all the devm usage in drm drivers is fundamentally
> broken. You also generally don't unplug gpus, so no one cares :-/
>
> What could instead is add a struct device to drm_device somehow, use that
> struct device to manage the lifetime of the drm_device (would still need
> our special open counter maybe), and then use devm to allocate all the drm
> bits tied to the drm_device.
>
> This here unfortunataly doesn't work, even if it looks like a really neat
> idea.
Strike all that, because I wasn't awak yet. Sorry for the confusion.
> > ---
> > Documentation/driver-model/devres.txt | 4 +
> > drivers/gpu/drm/drm_drv.c | 106 ++
> > include/drm/drm_drv.h | 6 ++
> > 3 files changed, 116 insertions(+)
> >
> > diff --git a/Documentation/driver-model/devres.txt
> > b/Documentation/driver-model/devres.txt
> > index b277cafce71e..6eebc28d4c21 100644
> > --- a/Documentation/driver-model/devres.txt
> > +++ b/Documentation/driver-model/devres.txt
> > @@ -254,6 +254,10 @@ DMA
> >dmam_pool_create()
> >dmam_pool_destroy()
> >
> > +DRM
> > + devm_drm_dev_init()
> > + devm_drm_dev_register_with_fbdev()
> > +
> > GPIO
> >devm_gpiod_get()
> >devm_gpiod_get_index()
> > diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
> > index 381581b01d48..12129772be45 100644
> > --- a/drivers/gpu/drm/drm_drv.c
> > +++ b/drivers/gpu/drm/drm_drv.c
> > @@ -36,6 +36,7 @@
> >
> > #include
> > #include
> > +#include
> > #include
> >
> > #include "drm_crtc_internal.h"
> > @@ -871,6 +872,111 @@ void drm_dev_unregister(struct drm_device *dev)
> > }
> > EXPORT_SYMBOL(drm_dev_unregister);
> >
> > +static void devm_drm_dev_init_release(void *data)
> > +{
> > + drm_dev_put(data);
We need drm_dev_unplug() here, or this isn't safe.
> > +}
> > +
> > +/**
> > + * 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 released on driver detach. You must supply a
I think a bit more clarity here would be good:
"... automatically released on driver unbind by callind drm_dev_unplug()."
> > + * _driver.release callback to control the finalization explicitly.
I think a loud warning for these is in order:
"WARNING:
"In generally it is unsafe to use devm functions for drm structures
because the lifetimes of _device and the underlying do not
match. This here works because it doesn't immediately free anything, but
only calls drm_dev_unplug(), which internally decrements the _device
refcount through drm_dev_put().
"All other drm structures must still be explicitly released in the
_driver.release callback."
While thinking about this I just realized that with this design we have no
good place to call drm_atomic_helper_shutdown(). Which we need to, or all
kinds of things will leak badly (connectors, fb, ...), but there's no
place to call it:
- unbind is too early, since we haven't yet called drm_dev_unplug, and the
drm_dev_unregister in there must be called _before_ we start to shut
down anything.
- drm_driver.release is way too late.
Ofc for a real hotunplug there's no point in shutting down the hw (it's
already gone), but for a driver unload/unbind it would be nice if this
happens automatically and in the right order.
So not sure what to do here really.
-Daniel
> > + *
> > + * Note: This function must be used together with
> > + * devm_drm_dev_register_with_fbdev().
> > + *
> > + * 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)
> > +{
> > + int ret;
> > +
> > + if (WARN_ON(!parent || !driver->release))
> > + return -EINVAL;
> > +
> > + ret = drm_dev_init(dev, driver, parent);
> > + if (ret)
> > + return ret;
> > +
> > + /*
> > +* This is a temporary release action that is used if probing fails
Re: [PATCH 01/11] drm: Add devm_drm_dev_init/register
On Sun, Jan 20, 2019 at 12:43:08PM +0100, Noralf Trønnes wrote:
> This adds resource managed (devres) versions of drm_dev_init() and
> drm_dev_register().
>
> Also added is devm_drm_dev_register_with_fbdev() which sets up generic
> fbdev emulation as well.
>
> devm_drm_dev_register() isn't exported since there are no users.
>
> Signed-off-by: Noralf Trønnes
devm_ considered harmful unfortunately. You cannot allocate any structures
which might outlive the lifetime of your device using devm_ on the device.
Since drm_device has it's own lifetime, that structure and _everything_ we
allocate tied to it (i.e. drm_encoder/plane/connector/crtc/whatever)
cannot be allocated using devm_ infrastructure tied to the underlying
parent device.
Yes this means almost all the devm usage in drm drivers is fundamentally
broken. You also generally don't unplug gpus, so no one cares :-/
What could instead is add a struct device to drm_device somehow, use that
struct device to manage the lifetime of the drm_device (would still need
our special open counter maybe), and then use devm to allocate all the drm
bits tied to the drm_device.
This here unfortunataly doesn't work, even if it looks like a really neat
idea.
-Daniel
> ---
> Documentation/driver-model/devres.txt | 4 +
> drivers/gpu/drm/drm_drv.c | 106 ++
> include/drm/drm_drv.h | 6 ++
> 3 files changed, 116 insertions(+)
>
> diff --git a/Documentation/driver-model/devres.txt
> b/Documentation/driver-model/devres.txt
> index b277cafce71e..6eebc28d4c21 100644
> --- a/Documentation/driver-model/devres.txt
> +++ b/Documentation/driver-model/devres.txt
> @@ -254,6 +254,10 @@ DMA
>dmam_pool_create()
>dmam_pool_destroy()
>
> +DRM
> + devm_drm_dev_init()
> + devm_drm_dev_register_with_fbdev()
> +
> GPIO
>devm_gpiod_get()
>devm_gpiod_get_index()
> diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
> index 381581b01d48..12129772be45 100644
> --- a/drivers/gpu/drm/drm_drv.c
> +++ b/drivers/gpu/drm/drm_drv.c
> @@ -36,6 +36,7 @@
>
> #include
> #include
> +#include
> #include
>
> #include "drm_crtc_internal.h"
> @@ -871,6 +872,111 @@ void drm_dev_unregister(struct drm_device *dev)
> }
> EXPORT_SYMBOL(drm_dev_unregister);
>
> +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 released on driver detach. You must supply a
> + * _driver.release callback to control the finalization explicitly.
> + *
> + * Note: This function must be used together with
> + * devm_drm_dev_register_with_fbdev().
> + *
> + * 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)
> +{
> + int ret;
> +
> + if (WARN_ON(!parent || !driver->release))
> + return -EINVAL;
> +
> + ret = drm_dev_init(dev, driver, parent);
> + if (ret)
> + return ret;
> +
> + /*
> + * This is a temporary release action that is used if probing fails
> + * before devm_drm_dev_register() is called.
> + */
> + ret = devm_add_action(parent, devm_drm_dev_init_release, dev);
> + if (ret)
> + devm_drm_dev_init_release(dev);
> +
> + return ret;
> +}
> +EXPORT_SYMBOL(devm_drm_dev_init);
> +
> +static void devm_drm_dev_register_release(void *data)
> +{
> + drm_dev_unplug(data);
> +}
> +
> +static int devm_drm_dev_register(struct drm_device *dev)
> +{
> + int ret;
> +
> + ret = drm_dev_register(dev, 0);
> + if (ret)
> + return ret;
> +
> + /*
> + * This has now served it's purpose, remove it to not mess up ref
> + * counting.
> + */
> + devm_remove_action(dev->dev, devm_drm_dev_init_release, dev);
> +
> + ret = devm_add_action(dev->dev, devm_drm_dev_register_release, dev);
> + if (ret)
> + devm_drm_dev_register_release(dev);
> +
> + return ret;
> +}
> +
> +/**
> + * devm_drm_dev_register_with_fbdev - Resource managed drm_dev_register()
> + *including generic fbdev emulation
> + * @dev: DRM device to register
> + * @fbdev_bpp: Preferred bits per pixel for fbdev (optional)
> + *
> + * Managed drm_dev_register() that also calls drm_fbdev_generic_setup().
> + * The DRM device registered with this function is automatically
> unregistered on
> + * driver detach using drm_dev_unplug().
> + *
> + * Note: This function must be used together with devm_drm_dev_init().
> + *
> + * For testing driver detach can be triggered manually by writing to the
> driver
> + * 'unbind' file.
> + *
> + * RETURNS:
> + * 0 on
Re: [PATCH 01/11] drm: Add devm_drm_dev_init/register
Hi Noralf.
On Sun, Jan 20, 2019 at 12:43:08PM +0100, Noralf Trønnes wrote:
> This adds resource managed (devres) versions of drm_dev_init() and
> drm_dev_register().
>
> Also added is devm_drm_dev_register_with_fbdev() which sets up generic
> fbdev emulation as well.
>
> devm_drm_dev_register() isn't exported since there are no users.
Is it relevant to use it outside this patchset - then it should be
documented/exported now to enable use.
>
> Signed-off-by: Noralf Trønnes
> ---
> Documentation/driver-model/devres.txt | 4 +
> drivers/gpu/drm/drm_drv.c | 106 ++
> include/drm/drm_drv.h | 6 ++
> 3 files changed, 116 insertions(+)
>
> diff --git a/Documentation/driver-model/devres.txt
> b/Documentation/driver-model/devres.txt
> index b277cafce71e..6eebc28d4c21 100644
> --- a/Documentation/driver-model/devres.txt
> +++ b/Documentation/driver-model/devres.txt
> @@ -254,6 +254,10 @@ DMA
>dmam_pool_create()
>dmam_pool_destroy()
>
> +DRM
> + devm_drm_dev_init()
> + devm_drm_dev_register_with_fbdev()
> +
> GPIO
>devm_gpiod_get()
>devm_gpiod_get_index()
> diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
> index 381581b01d48..12129772be45 100644
> --- a/drivers/gpu/drm/drm_drv.c
> +++ b/drivers/gpu/drm/drm_drv.c
> @@ -36,6 +36,7 @@
>
> #include
> #include
> +#include
> #include
>
> #include "drm_crtc_internal.h"
> @@ -871,6 +872,111 @@ void drm_dev_unregister(struct drm_device *dev)
> }
> EXPORT_SYMBOL(drm_dev_unregister);
>
> +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 released on driver detach. You must supply a
> + * _driver.release callback to control the finalization explicitly.
> + *
> + * Note: This function must be used together with
> + * devm_drm_dev_register_with_fbdev().
*must* be used, or can be used?
Reading the code this looks like something drivers that do not offer
fbdev emulation could also benefit from.
With the two comments processed this has:
Reviewed-by: Sam Ravnborg
Sam
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
