Hi Chris,
On Thursday 19 Jan 2017 11:20:37 Chris Wilson wrote:
> On Wed, Dec 14, 2016 at 12:04:38AM +0200, Laurent Pinchart wrote:
> > On Thursday 08 Dec 2016 08:18:40 Chris Wilson wrote:
> >> Some state is coupled into the device lifetime outside of the
> >> load/unload timeframe and requires teardown during final unreference
> >> from drm_dev_release(). For example, dmabufs hold both a device and
> >> module reference and may live longer than expected (i.e. the current
> >> pattern of the driver tearing down its state and then releasing a
> >> reference to the drm device) and yet touch driver private state when
> >> destroyed.
> >>
> >> Signed-off-by: Chris Wilson <ch...@chris-wilson.co.uk>
> >> ---
> >>
> >> drivers/gpu/drm/drm_drv.c | 3 +++
> >> include/drm/drm_drv.h | 8 ++++++++
> >> 2 files changed, 11 insertions(+)
> >>
> >> diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
> >> index f74b7d06ec01..f945bbcc8eb3 100644
> >> --- a/drivers/gpu/drm/drm_drv.c
> >> +++ b/drivers/gpu/drm/drm_drv.c
> >> @@ -595,6 +595,9 @@ static void drm_dev_release(struct kref *ref)
> >> {
> >> struct drm_device *dev = container_of(ref, struct drm_device, ref);
> >>
> >> + if (dev->driver->release)
> >> + dev->driver->release(dev);
> >> +
> >> if (drm_core_check_feature(dev, DRIVER_GEM))
> >> drm_gem_destroy(dev);
> >
> > For drivers embedding the drm_device structure, you should only call
> > .release() at the very end of this function, as the callback will free
> > memory, including the embedded struct drm_device.
>
> Not quite. The layering is wrong as the driver needs to release its
> state prior to e.g. drm_gem_destroy. It is not for freeing the memory.
drm_dev_release() is the drm_device kref's release function. It is the last
function called on a drm_device when the last reference goes away. It's
responsible for performing the last cleanups, and turning the lights off.
Turning the lights off involves freeing the drm_device memory. This is
currently done by drm_dev_release() unconditionally. When drivers allocate the
drm_device structure dynamically with drm_dev_alloc() the existing code is
fine. However, when drivers embed the drm_device structure in a driver-
specific structure, the kfree() at the end of drm_dev_release() only does the
right thing if the driver-specific structure embeds the drm_device as the very
first field. This is a hack, and should eventually be fixed, by propagating
the release call to the driver and having the driver free the memory it has
allocated. This can only be done as the very last operation in
drm_dev_release().
When drm_dev_release() is called all GEM objects should have been destroyed
already. The ones exported through dmabuf should hold a reference to the
drm_device instance, which is dropped when the dmabufs are destroyed. It
should thus be safe to call drm_gem_destroy() before the driver's release()
callback.
--
Regards,
Laurent Pinchart
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
