Daniel Thanks! I agree the PATCH 1/2 needs some more work. What do you think about the PATCH 2/2 (suspend/resume) -- would it make sense to review it as a single standalone patch? Regards, Haixia On Wed, Aug 31, 2016 at 2:17 PM, Daniel Vetter <daniel@xxxxxxxx> wrote: > On Wed, Aug 31, 2016 at 11:05 PM, Daniel Vetter <daniel@xxxxxxxx> wrote: >> On Wed, Aug 31, 2016 at 10:45 PM, Haixia Shi <hshi@xxxxxxxxxxxx> wrote: >>> For details see https://bugs.chromium.org/p/chromium/issues/detail?id=468050 >>> >>> So drm_mode_config_cleanup() is called from udl_driver_unload() in >>> which we found there's still a framebuffer left, hence the WARN in >>> drm_crtc.c:5495. This also forcefully releases all the buffers. >>> >>> A bit later the actual drm_buf_release comes in which attempts to >>> release the buffer again. >> >> Leaving a drm_framebuffer behind on unload is definitely a bug, but >> not fixed with this patch here I think. >> >> The dma-buf lifetime issue is far worse, since we simply don't >> handling those leaking past the lifetime of the exporting drm_device >> at all. The dma-buf has references to a lot more than just the vma >> manager. What we probably need is that every exported dma-buf holds a >> ref on the underlying drm_device, but that means untangling the >> refcounting&cleanup of that vs unplugging it. > > Just noticed that these problems started only when dma-buf export > support was added (by you) to udl. That dma-buf support is a bit > hack-ish (e.g. it leaks sg mappings since udl_unmap_dma_buf isn't > implemented. Rough sketch of a fix: > > - fix up udl_dmabuf.c. We could/should probably put most of these into > the core as a set of helpers for drivers which use plain shmem gem > objects. > > - fix udl load/unload to no longer be midlayered, i.e. get rid of the > ->load/unload hooks. There's tons of examples and drivers out there > for templates. > > - fix up the unplug hook to correctly unregister everything. With the > fixed load/unload all the unplugged tracking is probably no longer > neeeded. Also with this you can drop the drm_global_mutex dance from > drm_unplug_dev. Also the sequence in drm_unplug_dev is wrong like the > midlayered ->unload. First it should do all the unregistering, then > release internal resources. Atm it's the other way round. > > - make sure _all_ public objects (open files, counted by > dev->open_count, dma-bufs, ...) hold a full reference onto the > drm_device. For the dma-buf case this probably needs changes in > drm_prime.c. > > - Fix that little ordering issue with the leaking fb. It's probably > not getting cleanup up as it should in udl_fbdev_unplug. > > tada, bug fixed for real! > > Cheers, Daniel > -- > Daniel Vetter > Software Engineer, Intel Corporation > +41 (0) 79 365 57 48 - http://blog.ffwll.ch _______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/dri-devel
