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

[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?

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()?

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);
>
>