On Wed, May 23, 2018 at 9:41 PM, Christian König
<christian.koenig@xxxxxxx> 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@xxxxxxxxx> 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@xxxxxxxxx>
>>>> ---
>>>> 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);
>>>
>>>
>
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
