[PATCH RFC 05/24] Revert "drm: Nerf the preclose callback for modern drivers"

[Christian König]

Am 23.05.2018 um 15:13 schrieb Qiang Yu:
> On Wed, May 23, 2018 at 5:35 PM, Christian König
> <ckoenig.leichtzumerken at gmail.com> wrote:
>> Well NAK, that brings back a callback we worked quite hard on getting rid
>> of.
>>
>> It looks like the problem isn't that you need the preclose callback, but you
>> rather seem to misunderstood how TTM works.
>>
>> All you need to do is to cleanup your command submission path so that the
>> caller of lima_sched_context_queue_task() adds the resulting scheduler fence
>> to TTMs buffer objects.
> You mean adding the finished dma fence to the buffer's reservation object then
> waiting it before unmap the buffer from GPU VM in the drm_release()'s buffer
> close callback?

That is one possibility, but also not necessary.

TTM has a destroy callback which is called from a workqueue when all 
fences on that BOs have signaled.

Depending on your VM management you can use it to delay unmapping the 
buffer until it is actually not used any more.

> Adding fence is done already, and I did wait it before unmap. But then
> I see when
> the buffer is shared between processes, the "perfect wait" is just
> wait the fence
> from this process's task, so it's better to also distinguish fences.
> If so, I just think
> why we don't just wait tasks from this process in the preclose before unmap/free
> buffer in the drm_release()?

Well it depends on your VM management. When userspace expects that the 
VM space the BO used is reusable immediately than the TTM callback won't 
work.

On the other hand you can just grab the list of fences on a BO and 
filter out the ones from your current process and wait for those. See 
amdgpu_sync_resv() as an example how to do that.

Christian.

>
> Regards,
> Qiang
>
>>
>> Am 18.05.2018 um 11:27 schrieb Qiang Yu:
>>> This reverts commit 45c3d213a400c952ab7119f394c5293bb6877e6b.
>>>
>>> lima driver need preclose to wait all task in the context
>>> created within closing file to finish before free all the
>>> buffer object. Otherwise pending tesk may fail and get
>>> noisy MMU fault message.
>>>
>>> Move this wait to each buffer object free function can
>>> achieve the same result but some buffer object is shared
>>> with other file context, but we only want to wait the
>>> closing file context's tasks. So the implementation is
>>> not that straight forword compared to the preclose one.
>>>
>>> Signed-off-by: Qiang Yu <yuq825 at gmail.com>
>>> ---
>>>    drivers/gpu/drm/drm_file.c |  8 ++++----
>>>    include/drm/drm_drv.h      | 23 +++++++++++++++++++++--
>>>    2 files changed, 25 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/drm_file.c b/drivers/gpu/drm/drm_file.c
>>> index e394799979a6..0a43107396b9 100644
>>> --- a/drivers/gpu/drm/drm_file.c
>>> +++ b/drivers/gpu/drm/drm_file.c
>>> @@ -361,8 +361,9 @@ void drm_lastclose(struct drm_device * dev)
>>>     *
>>>     * This function must be used by drivers as their
>>> &file_operations.release
>>>     * method. It frees any resources associated with the open file, and
>>> calls the
>>> - * &drm_driver.postclose driver callback. If this is the last open file
>>> for the
>>> - * DRM device also proceeds to call the &drm_driver.lastclose driver
>>> callback.
>>> + * &drm_driver.preclose and &drm_driver.lastclose driver callbacks. If
>>> this is
>>> + * the last open file for the DRM device also proceeds to call the
>>> + * &drm_driver.lastclose driver callback.
>>>     *
>>>     * RETURNS:
>>>     *
>>> @@ -382,8 +383,7 @@ int drm_release(struct inode *inode, struct file
>>> *filp)
>>>          list_del(&file_priv->lhead);
>>>          mutex_unlock(&dev->filelist_mutex);
>>>    -     if (drm_core_check_feature(dev, DRIVER_LEGACY) &&
>>> -           dev->driver->preclose)
>>> +       if (dev->driver->preclose)
>>>                  dev->driver->preclose(dev, file_priv);
>>>          /* ========================================================
>>> diff --git a/include/drm/drm_drv.h b/include/drm/drm_drv.h
>>> index d23dcdd1bd95..8d6080f97ed4 100644
>>> --- a/include/drm/drm_drv.h
>>> +++ b/include/drm/drm_drv.h
>>> @@ -107,6 +107,23 @@ struct drm_driver {
>>>           */
>>>          int (*open) (struct drm_device *, struct drm_file *);
>>>    +     /**
>>> +        * @preclose:
>>> +        *
>>> +        * One of the driver callbacks when a new &struct drm_file is
>>> closed.
>>> +        * Useful for tearing down driver-private data structures
>>> allocated in
>>> +        * @open like buffer allocators, execution contexts or similar
>>> things.
>>> +        *
>>> +        * Since the display/modeset side of DRM can only be owned by
>>> exactly
>>> +        * one &struct drm_file (see &drm_file.is_master and
>>> &drm_device.master)
>>> +        * there should never be a need to tear down any modeset related
>>> +        * resources in this callback. Doing so would be a driver design
>>> bug.
>>> +        *
>>> +        * FIXME: It is not really clear why there's both @preclose and
>>> +        * @postclose. Without a really good reason, use @postclose only.
>>> +        */
>>> +       void (*preclose) (struct drm_device *, struct drm_file
>>> *file_priv);
>>> +
>>>          /**
>>>           * @postclose:
>>>           *
>>> @@ -118,6 +135,9 @@ struct drm_driver {
>>>           * one &struct drm_file (see &drm_file.is_master and
>>> &drm_device.master)
>>>           * there should never be a need to tear down any modeset related
>>>           * resources in this callback. Doing so would be a driver design
>>> bug.
>>> +        *
>>> +        * FIXME: It is not really clear why there's both @preclose and
>>> +        * @postclose. Without a really good reason, use @postclose only.
>>>           */
>>>          void (*postclose) (struct drm_device *, struct drm_file *);
>>>    @@ -134,7 +154,7 @@ struct drm_driver {
>>>           * state changes, e.g. in conjunction with the
>>> :ref:`vga_switcheroo`
>>>           * infrastructure.
>>>           *
>>> -        * This is called after @postclose hook has been called.
>>> +        * This is called after @preclose and @postclose have been called.
>>>           *
>>>           * NOTE:
>>>           *
>>> @@ -601,7 +621,6 @@ struct drm_driver {
>>>          /* List of devices hanging off this driver with stealth attach. */
>>>          struct list_head legacy_dev_list;
>>>          int (*firstopen) (struct drm_device *);
>>> -       void (*preclose) (struct drm_device *, struct drm_file
>>> *file_priv);
>>>          int (*dma_ioctl) (struct drm_device *dev, void *data, struct
>>> drm_file *file_priv);
>>>          int (*dma_quiescent) (struct drm_device *);
>>>          int (*context_dtor) (struct drm_device *dev, int context);
>>