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