linux.git/drivers/gpu/drm/drm_drv.c, branch v5.8

drm: Add devm_drm_dev_alloc macro

2020-04-28T13:05:19+00:00

Add a new macro helper to combine the usual init sequence in drivers,
consisting of a kzalloc + devm_drm_dev_init + drmm_add_final_kfree
triplet. This allows us to remove the rather unsightly
drmm_add_final_kfree from all currently merged drivers.

The kerneldoc is only added for this new function. Existing kerneldoc
and examples will be udated at the very end, since once all drivers
are converted over to devm_drm_dev_alloc we can unexport a lot of
interim functions and make the documentation for driver authors a lot
cleaner and less confusing. There will be only one true way to
initialize a drm_device at the end of this, which is going to be
devm_drm_dev_alloc.

v2:
- Actually explain what this is for in the commit message (Sam)
- Fix checkpatch issues (Sam)

Acked-by: Noralf Trønnes 
Cc: Noralf Trønnes 
Reviewed-by: Thomas Zimmermann 
Reviewed-by: Sam Ravnborg 
Cc: Sam Ravnborg 
Cc: Paul Kocialkowski 
Cc: Laurent Pinchart 
Signed-off-by: Daniel Vetter 
Link: https://patchwork.freedesktop.org/patch/msgid/20200415074034.175360-2-daniel.vetter@ffwll.ch

drm: Fix wrong kfree() in managed resource usage example

2020-04-02T11:04:57+00:00

The example code showing how to use the managed resource API calls
kfree() on the wrong pointer. Fix it.

Signed-off-by: Laurent Pinchart 
Signed-off-by: Daniel Vetter 
Link: https://patchwork.freedesktop.org/patch/msgid/20200402095325.5266-1-laurent.pinchart+renesas@ideasonboard.com

drm: Add docs for managed resources

2020-03-26T15:09:48+00:00

All collected together to provide a consistent story in one patch,
instead of the somewhat bumpy refactor-evolution leading to this.

Also some thoughts on what the next steps could be:

- Create a macro called devm_drm_dev_alloc() which essentially wraps
  the kzalloc(); devm_drm_dev_init(); drmm_add_final_kfree() combo.
  Needs to be a macro since we'll have to do some typeof trickery and
  casting to make this fully generic for all drivers that embed struct
  drm_device into their own thing.

- A lot of the simple drivers now have essentially just
  drm_dev_unplug(); drm_atomic_helper_shutdown(); as their
  $bus_driver->remove hook. We could create a devm_mode_config_reset
  which sets drm_atomic_helper_shutdown as it's cleanup action, and a
  devm_drm_dev_register with drm_dev_unplug as it's cleanup action,
  and simple drivers wouldn't have a need for a ->remove function at
  all, and we could delete them.

- For more complicated drivers we need drmm_ versions of a _lot_ more
  things. All the userspace visible objects (crtc, plane, encoder,
  crtc), anything else hanging of those (maybe a drmm_get_edid, at
  least for panels and other built-in stuff).

Also some more thoughts on why we're not reusing devm_ with maybe a
fake struct device embedded into the drm_device (we can't use the
kdev, since that's in each drm_minor).

- Code review gets extremely tricky, since every time you see a devm_
  you need to carefully check whether the fake device (with the
  drm_device lifetim) or the real device (with the lifetim of the
  underlying physical device and driver binding) are used. That's not
  going to help at all, and we have enormous amounts of drivers who
  use devm_ where they really shouldn't. Having different types makes
  sure the compiler type checks this for us and ensures correctness.

- The set of functions are very much non-overlapping. E.g.
  devm_ioremap makes total sense, drmm_ioremap has the wrong lifetime,
  since hw resources need to be cleaned out at driver unbind and wont
  outlive that like a drm_device. Similar, but other way round for
  drmm_connector_init (which is the only correct version, devm_ for
  drm_connector is just buggy). Simply not having the wrong version
  again prevents bugs.

Finally I guess this opens a huge todo for all the drivers. I'm
semi-tempted to do a tree-wide s/devm_kzalloc/drmm_kzalloc/ since most
likely that'll fix an enormous amount of bugs and most likely not
cause any issues at all (aside from maybe holding onto memory slightly
too long).

v2:
- Doc improvements from Laurent.
- Also add kerneldoc for the new drmm_add_action_or_reset.

v3:
- Remove kerneldoc for drmm_remove_action.

Reviewed-by: Sam Ravnborg 
Cc: Sam Ravnborg 
Cc: Jonathan Corbet 
Cc: linux-doc@vger.kernel.org
Cc: Laurent Pinchart 
Cc: Greg Kroah-Hartman 
Cc: "Rafael J. Wysocki" 
Signed-off-by: Daniel Vetter 

fixup docs
Link: https://patchwork.freedesktop.org/patch/msgid/20200323144950.3018436-52-daniel.vetter@ffwll.ch

drm: Manage drm_mode_config_init with drmm_

2020-03-26T14:45:43+00:00

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 now 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().

Also with drmm_ explicit drm_driver->release hooks are kinda not the
best option: Drivers can always just register their current release
hook with drmm_add_action, but even better they could split them up to
simplify the unwinding for the driver load failure case. So deprecate
that hook to discourage future users.

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 drmm_add_action_or_reset macro (Noralf)

v4: Review from Sam:
- drmm_mode_config_init wrapper (also suggested by Thomas)
- improve commit message, explain better why ->relase is deprecated

v5:
- Make drmm_ the main function, with the old one as compat wrapper
  (Sam)
- Add FIXME comments to drm_mode_config_cleanup/init() that drivers
  shouldn't use these anymore.
- Move drmm_add_action_or_reset helper to an earlier patch.

Reviewed-by: Sam Ravnborg 
Cc: Laurent Pinchart 
Cc: "Noralf Trønnes" 
Cc: Sam Ravnborg 
Cc: Thomas Zimmermann 
Acked-by: Noralf Trønnes 
Signed-off-by: Daniel Vetter 
Link: https://patchwork.freedesktop.org/patch/msgid/20200323144950.3018436-27-daniel.vetter@ffwll.ch

drm: Garbage collect drm_dev_fini

2020-03-26T14:45:36+00:00

It has become empty. Given the few users I figured not much point
splitting this up.

v2: Rebase over i915 changes.

v3: Rebase over patch split fix.

Acked-by: Sam Ravnborg 
Signed-off-by: Daniel Vetter 
Link: https://patchwork.freedesktop.org/patch/msgid/20200323144950.3018436-26-daniel.vetter@ffwll.ch

drm: Manage drm_vblank_cleanup with drmm_

2020-03-26T14:38:17+00:00

Nothing special here, except that this is the first time that we
automatically clean up something that's initialized with an explicit
driver call. But the cleanup was done at the very end of the release
sequence for all drivers, and that's still the case. At least without
more uses of drmm_ through explicit driver calls.

Also for this one we need drmm_kcalloc, so lets add those.

The motivation here is to allow us to remove the explicit calls to
drm_dev_fini() from all drivers.

v2: Sort includes (Laurent)

v3: Motivate the change in the commit message better (Sam)

Acked-by: Sam Ravnborg 
Signed-off-by: Daniel Vetter 
Link: https://patchwork.freedesktop.org/patch/msgid/20200323144950.3018436-25-daniel.vetter@ffwll.ch

drm: Manage drm_gem_init with drmm_

2020-03-26T14:38:09+00:00

We might want to look into pushing this down into drm_mm_init, but
that would mean rolling out return codes to a pile of functions
unfortunately. So let's leave that for now.

Acked-by: Sam Ravnborg 
Signed-off-by: Daniel Vetter 
Link: https://patchwork.freedesktop.org/patch/msgid/20200323144950.3018436-24-daniel.vetter@ffwll.ch

drm: manage drm_minor cleanup with drmm_

2020-03-26T14:37:31+00:00

The cleanup here is somewhat tricky, since we can't tell apart the
allocated minor index from 0. So register a cleanup action first, and
if the index allocation fails, unregister that cleanup action again to
avoid bad mistakes.

The kdev for the minor already handles NULL, so no problem there.

Hence add drmm_remove_action() to the drm_managed library.

v2: Make pointer math around void ** consistent with what Laurent
suggested.

v3: Use drmm_add_action_or_reset and remove drmm_remove_action. Noticed
because of some questions from Thomas. This also means we need to move
the drmm_add_action_or_reset helper earlier in the series.

v4: Uh ... fix slightly embarrassing bug CI spotted.

Acked-by: Thomas Zimmermann 
Reviewed-by: Sam Ravnborg 
Cc: Thomas Zimmermann 
Cc: Laurent Pinchart 
Signed-off-by: Daniel Vetter 
Link: https://patchwork.freedesktop.org/patch/msgid/20200324203936.3330994-1-daniel.vetter@ffwll.ch

drm: Use drmm_ for drm_dev_init cleanup

2020-03-26T14:36:47+00:00

Well for the simple stuff at least, vblank, gem and minor cleanup I
want to further split up as a demonstration.

v2: We need to clear drm_device->dev otherwise the debug drm printing
after our cleanup hook (e.g. in drm_manged_release) will chase
released memory and result in a use-after-free. Not really pretty, but
oh well.

Reviewed-by: Sam Ravnborg 
Signed-off-by: Daniel Vetter 
Link: https://patchwork.freedesktop.org/patch/msgid/20200323144950.3018436-22-daniel.vetter@ffwll.ch

drm: Handle dev->unique with drmm_

2020-03-26T14:36:20+00:00

We need to add a drmm_kstrdup for this, but let's start somewhere.

This is not exactly perfect onion unwinding, but it's jsut a kfree so
doesn't really matter at all.

Reviewed-by: Sam Ravnborg 
Acked-by: Thomas Zimmermann 
Signed-off-by: Daniel Vetter 
Link: https://patchwork.freedesktop.org/patch/msgid/20200323144950.3018436-21-daniel.vetter@ffwll.ch