On Fri, Feb 28, 2020 at 9:26 PM Sam Ravnborg <sam@xxxxxxxxxxxx> wrote:
>
> Hi Daniel.
>
> Some bikeshedding in the following.
> with or with addressing (IMHO valid points) consider the patch:
>
> Reviewed-by: Sam Ravnborg <sam@xxxxxxxxxxxx>
>
> Sam
>
> On Thu, Feb 27, 2020 at 07:14:57PM +0100, Daniel Vetter wrote:
> > drm_mode_config_cleanup is idempotent, so no harm in calling this
> > twice. This allows us to gradually switch drivers over by removing
> > explicit drm_mode_config_cleanup calls.
> >
>
> > With this step it's not also possible that (at least for simple
> > drivers) automatic resource cleanup can be done correctly without a
> > drm_driver->release hook. Therefore allow this now in
> > devm_drm_dev_init().
> I am not really sure what you try to explain here?
> Should the "not" be deleted?
s/not/now/
somehow that's a typo I do all the time, dunno why.
> > Also with drmm_ explicit drm_driver->release hooks are kinda not the
> > best option, so deprecate that hook to discourage future users.
> The ->release hooks has others uses until everything is moved over to
> drmm_, or so I think. So deprecation seems a lttle too soon.
You can just add a drmm action which calls your release function. The
upshot is that you can be more fine-grained (useful for unwinding when
driver load fails halfway through). That's why I think new drivers
after this patch shouldn't use ->release anymore, it's strictly less
useful than drmm actions. The less unwind code I have to review
carefully to make sure the right stuff gets released (and not more!)
the better.
> > v2: Fixup the example in the kerneldoc too.
> >
> > v3:
> > - For paranoia, double check that minor->dev == dev in the release
> > hook, because I botched the pointer math in the drmm library.
> > - Call drm_mode_config_cleanup when drmm_add_action fails, we'd be
> > missing some mutex_destroy and ida_cleanup otherwise (Laurent)
> >
> > v4: Add a drmm_add_action_or_reset (like devm_ has) to encapsulate this
> > pattern (Noralf).
> >
> > v5: Fix oversight in the new add_action_or_reset macro (Noralf)
> ^ drmm_add_action_or_reset
> >
> > Cc: Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx>
> > Cc: "Noralf Trønnes" <noralf@xxxxxxxxxxx>
> > Cc: Sam Ravnborg <sam@xxxxxxxxxxxx>
> > Cc: Thomas Zimmermann <tzimmermann@xxxxxxx>
> > Acked-by: Noralf Trønnes <noralf@xxxxxxxxxxx>
> > Signed-off-by: Daniel Vetter <daniel.vetter@xxxxxxxxx>
> > ---
> > drivers/gpu/drm/drm_drv.c | 23 +++++++----------------
> > drivers/gpu/drm/drm_managed.c | 14 ++++++++++++++
> > drivers/gpu/drm/drm_mode_config.c | 13 ++++++++++++-
> > include/drm/drm_managed.h | 7 +++++++
> > include/drm/drm_mode_config.h | 2 +-
> > 5 files changed, 41 insertions(+), 18 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
> > index 3cf40864d4a6..bb326b9bcde0 100644
> > --- a/drivers/gpu/drm/drm_drv.c
> > +++ b/drivers/gpu/drm/drm_drv.c
> > @@ -98,6 +98,8 @@ static void drm_minor_alloc_release(struct drm_device *dev, void *data)
> > struct drm_minor *minor = data;
> > unsigned long flags;
> >
> > + WARN_ON(dev != minor->dev);
> > +
> > put_device(minor->kdev);
> >
> > spin_lock_irqsave(&drm_minor_lock, flags);
> > @@ -267,8 +269,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()
> > - * usage with its accompanying drm_driver->release callback.
> > + * almost always present and serves as a demonstration of devm_drm_dev_init().
> > *
> > * .. code-block:: c
> > *
> > @@ -278,16 +279,8 @@ void drm_minor_release(struct drm_minor *minor)
> > * struct clk *pclk;
> > * };
> > *
> > - * static void driver_drm_release(struct drm_device *drm)
> > - * {
> > - * struct driver_device *priv = container_of(...);
> > - *
> > - * drm_mode_config_cleanup(drm);
> > - * }
> > - *
> > * static struct drm_driver driver_drm_driver = {
> > * [...]
> > - * .release = driver_drm_release,
> > * };
> > *
> > * static int driver_probe(struct platform_device *pdev)
> > @@ -312,7 +305,9 @@ void drm_minor_release(struct drm_minor *minor)
> > * }
> > * drmm_add_final_kfree(drm, priv);
> > *
> > - * drm_mode_config_init(drm);
> > + * ret = drm_mode_config_init(drm);
> > + * if (ret)
> > + * return ret;
> We do not print anything in drm_mode_config_init() - so should
> we do it here?
> Otherwise we only get the more generic error from the driver core.
I can add a printk to drm_mode_config if people feel like. But it's
guaranteed dead code in reality, because of linux' small memory
allocation guarantee. Small mallocs like this one here of just 2
cachelines never fail (at least not with GFP_KERNEL).
> > *
> > * priv->userspace_facing = drmm_kzalloc(..., GFP_KERNEL);
> > * if (!priv->userspace_facing)
> > @@ -710,8 +705,7 @@ static void devm_drm_dev_init_release(void *data)
> > * @driver: DRM driver
> > *
> > * Managed drm_dev_init(). The DRM device initialized with this function is
> > - * automatically put on driver detach using drm_dev_put(). You must supply a
> > - * &drm_driver.release callback to control the finalization explicitly.
> > + * automatically put on driver detach using drm_dev_put().
> > *
> > * RETURNS:
> > * 0 on success, or error code on failure.
> > @@ -722,9 +716,6 @@ int devm_drm_dev_init(struct device *parent,
> > {
> > int ret;
> >
> > - if (WARN_ON(!driver->release))
> > - return -EINVAL;
> > -
> > ret = drm_dev_init(dev, driver, parent);
> > if (ret)
> > return ret;
> > diff --git a/drivers/gpu/drm/drm_managed.c b/drivers/gpu/drm/drm_managed.c
> > index 626656369f0b..6376be01bbc8 100644
> > --- a/drivers/gpu/drm/drm_managed.c
> > +++ b/drivers/gpu/drm/drm_managed.c
> > @@ -134,6 +134,20 @@ int __drmm_add_action(struct drm_device *dev,
> > }
> > EXPORT_SYMBOL(__drmm_add_action);
> >
> > +int __drmm_add_action_or_reset(struct drm_device *dev,
> > + drmres_release_t action,
> > + void *data, const char *name)
> > +{
> > + int ret;
> > +
> > + ret = __drmm_add_action(dev, action, data, name);
> > + if (ret)
> > + action(dev, data);
> > +
> > + return ret;
> > +}
> > +EXPORT_SYMBOL(__drmm_add_action_or_reset);
>
> Bikeshedding - but why oh why prefixing the function with two
> underscores?
> - It makes it less readable
> - It says "internal", at least to me - but it is exported
> - It makes the casual reader wonder why, removing focus from other more
> relevant things
> - It makes me writing several lines of rant
>
> drmm_add_action_or_reset_named(...) would do the trick.
It _is_ an internal function. I don't think drivers should ever call
it. Hence __ and hence no kerneldoc in the last patch. Now if someone
finds a legit use for this I guess we cold rename it, and give it some
nice kerneldoc. But imo no point before that.
A thing that might be useful is drm_add_action_f() with a format
string at the end, for cases where you set a non-NULL data pointer,
and want to include that somehow in the name. This might be useful for
cleanup actions for stuff like connectors and so on. But we're not
there yet at all, and you can also do that kind of meaningful debug
output from cleanup function directly.
> Same rant above goes for __drmm_add_action()...
>
> > +
> > void drmm_remove_action(struct drm_device *dev,
> > drmres_release_t action,
> > void *data)
> > diff --git a/drivers/gpu/drm/drm_mode_config.c b/drivers/gpu/drm/drm_mode_config.c
> > index 08e6eff6a179..6f7005bc597f 100644
> > --- a/drivers/gpu/drm/drm_mode_config.c
> > +++ b/drivers/gpu/drm/drm_mode_config.c
> > @@ -25,6 +25,7 @@
> > #include <drm/drm_drv.h>
> > #include <drm/drm_encoder.h>
> > #include <drm/drm_file.h>
> > +#include <drm/drm_managed.h>
> > #include <drm/drm_mode_config.h>
> > #include <drm/drm_print.h>
> > #include <linux/dma-resv.h>
> > @@ -373,6 +374,11 @@ static int drm_mode_create_standard_properties(struct drm_device *dev)
> > return 0;
> > }
> >
> > +static void drm_mode_config_init_release(struct drm_device *dev, void *ptr)
> > +{
> > + drm_mode_config_cleanup(dev);
> > +}
> > +
> > /**
> > * drm_mode_config_init - initialize DRM mode_configuration structure
> > * @dev: DRM device
> > @@ -384,8 +390,10 @@ static int drm_mode_create_standard_properties(struct drm_device *dev)
> > * problem, since this should happen single threaded at init time. It is the
> > * driver's problem to ensure this guarantee.
> > *
> > + * Cleanup is automatically handled through registering drm_mode_config_cleanup
> > + * with drmm_add_action().
> > */
> > -void drm_mode_config_init(struct drm_device *dev)
> > +int drm_mode_config_init(struct drm_device *dev)
> > {
> > mutex_init(&dev->mode_config.mutex);
> > drm_modeset_lock_init(&dev->mode_config.connection_mutex);
> > @@ -443,6 +451,9 @@ void drm_mode_config_init(struct drm_device *dev)
> > drm_modeset_acquire_fini(&modeset_ctx);
> > dma_resv_fini(&resv);
> > }
> > +
> > + return drmm_add_action_or_reset(dev, drm_mode_config_init_release,
> > + NULL);
> > }
> > EXPORT_SYMBOL(drm_mode_config_init);
> As this is now a drmm_ managed function it should be named such.
> Maybe add a small drm_mode_config_init() wrapper in the header file for
> those that has not migrated yet.
> It is confusing if we are not consistent in naming and everywhere else
> the drm managed functions are named drmm_
Yeah ok I guess I'm convinced on this :-)
Cheers, Daniel
>
>
> >
> > diff --git a/include/drm/drm_managed.h b/include/drm/drm_managed.h
> > index 2b1ba2ad5582..1e6291407586 100644
> > --- a/include/drm/drm_managed.h
> > +++ b/include/drm/drm_managed.h
> > @@ -18,6 +18,13 @@ int __must_check __drmm_add_action(struct drm_device *dev,
> > drmres_release_t action,
> > void *data, const char *name);
> >
> > +#define drmm_add_action_or_reset(dev, action, data) \
> > + __drmm_add_action_or_reset(dev, action, data, #action)
> > +
> > +int __must_check __drmm_add_action_or_reset(struct drm_device *dev,
> > + drmres_release_t action,
> > + void *data, const char *name);
> > +
> > void drmm_remove_action(struct drm_device *dev,
> > drmres_release_t action,
> > void *data);
> > diff --git a/include/drm/drm_mode_config.h b/include/drm/drm_mode_config.h
> > index 3bcbe30339f0..160a3e4b51c3 100644
> > --- a/include/drm/drm_mode_config.h
> > +++ b/include/drm/drm_mode_config.h
> > @@ -929,7 +929,7 @@ struct drm_mode_config {
> > const struct drm_mode_config_helper_funcs *helper_private;
> > };
> >
> > -void drm_mode_config_init(struct drm_device *dev);
> > +int drm_mode_config_init(struct drm_device *dev);
> > void drm_mode_config_reset(struct drm_device *dev);
> > void drm_mode_config_cleanup(struct drm_device *dev);
> >
> > --
> > 2.24.1
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
