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

[Qiang Yu <yuq825@gmail.com>]

On Wed, May 23, 2018 at 9:41 PM, Christian König
<christian.koenig@amd.com> wrote:
> Am 23.05.2018 um 15:13 schrieb Qiang Yu:
>>
>> On Wed, May 23, 2018 at 5:35 PM, Christian König
>> <ckoenig.leichtzumerken@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.

In current lima implementation, user space driver is responsible not unmap/free
buffer before task is complete. And VM map/unmap is not differed.

This works simple and fine except the case that user press Ctrl+C to terminate
the application which will force to close drm fd.

I'd more prefer to wait buffer fence before vm unmap and filter like
amdgpu_sync_resv() compared to implement refcount in kernel task.
But these two ways are all not as simple as preclose.

So I still don't understand why you don't want to get preclose back even
have to introduce other complicated mechanism to cover the case free/unmap
buffer before this process's task is done?

Regards,
Qiang

>
> 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@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);
>>>
>>>
>
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel