[v1,4/6] dma-buf: Acquire wait-wound context on attachment
Commit Message
Intel i915 GPU driver uses wait-wound mutex to lock multiple GEMs on the
attachment to the i915 dma-buf. In order to let all drivers utilize shared
wait-wound context during attachment in a general way, make dma-buf core to
acquire the ww context internally for the attachment operation and update
i915 driver to use the importer's ww context instead of the internal one.
From now on all dma-buf exporters shall use the importer's ww context for
the attachment operation.
Signed-off-by: Dmitry Osipenko <dmitry.osipenko@collabora.com>
---
drivers/dma-buf/dma-buf.c | 8 +++++-
drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c | 2 +-
.../gpu/drm/i915/gem/i915_gem_execbuffer.c | 2 +-
drivers/gpu/drm/i915/gem/i915_gem_object.h | 6 ++---
drivers/gpu/drm/i915/i915_gem_evict.c | 2 +-
drivers/gpu/drm/i915/i915_gem_ww.c | 26 +++++++++++++++----
drivers/gpu/drm/i915/i915_gem_ww.h | 15 +++++++++--
7 files changed, 47 insertions(+), 14 deletions(-)
Comments
Hi Dmitry,
I love your patch! Yet something to improve:
[auto build test ERROR on next-20220714]
[also build test ERROR on v5.19-rc6]
[cannot apply to drm-misc/drm-misc-next drm-intel/for-linux-next media-tree/master linus/master v5.19-rc6 v5.19-rc5 v5.19-rc4]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Dmitry-Osipenko/Move-all-drivers-to-a-common-dma-buf-locking-convention/20220715-085556
base: 37b355fdaf31ee18bda9a93c2a438dc1cbf57ec9
config: x86_64-allmodconfig (https://download.01.org/0day-ci/archive/20220715/202207151112.Yi2gyyRX-lkp@intel.com/config)
compiler: gcc-11 (Debian 11.3.0-3) 11.3.0
reproduce (this is a W=1 build):
# https://github.com/intel-lab-lkp/linux/commit/ed55f535b8492ef30d7e94aae5811c772403ab4f
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Dmitry-Osipenko/Move-all-drivers-to-a-common-dma-buf-locking-convention/20220715-085556
git checkout ed55f535b8492ef30d7e94aae5811c772403ab4f
# save the config file
mkdir build_dir && cp config build_dir/.config
make W=1 O=build_dir ARCH=x86_64 SHELL=/bin/bash drivers/gpu/drm/i915/
If you fix the issue, kindly add following tag where applicable
Reported-by: kernel test robot <lkp@intel.com>
All errors (new ones prefixed by >>):
>> drivers/gpu/drm/i915/i915_gem_ww.c:52:6: error: no previous prototype for 'i915_gem_ww_ctx_fini2' [-Werror=missing-prototypes]
52 | void i915_gem_ww_ctx_fini2(struct i915_gem_ww_ctx *ww)
| ^~~~~~~~~~~~~~~~~~~~~
cc1: all warnings being treated as errors
vim +/i915_gem_ww_ctx_fini2 +52 drivers/gpu/drm/i915/i915_gem_ww.c
51
> 52 void i915_gem_ww_ctx_fini2(struct i915_gem_ww_ctx *ww)
53 {
54 i915_gem_ww_ctx_unlock_all(ww);
55 WARN_ON(ww->contended);
56 }
57
Am 15.07.22 um 02:52 schrieb Dmitry Osipenko:
> Intel i915 GPU driver uses wait-wound mutex to lock multiple GEMs on the
> attachment to the i915 dma-buf. In order to let all drivers utilize shared
> wait-wound context during attachment in a general way, make dma-buf core to
> acquire the ww context internally for the attachment operation and update
> i915 driver to use the importer's ww context instead of the internal one.
>
> From now on all dma-buf exporters shall use the importer's ww context for
> the attachment operation.
>
> Signed-off-by: Dmitry Osipenko <dmitry.osipenko@collabora.com>
> ---
> drivers/dma-buf/dma-buf.c | 8 +++++-
> drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c | 2 +-
> .../gpu/drm/i915/gem/i915_gem_execbuffer.c | 2 +-
> drivers/gpu/drm/i915/gem/i915_gem_object.h | 6 ++---
> drivers/gpu/drm/i915/i915_gem_evict.c | 2 +-
> drivers/gpu/drm/i915/i915_gem_ww.c | 26 +++++++++++++++----
> drivers/gpu/drm/i915/i915_gem_ww.h | 15 +++++++++--
> 7 files changed, 47 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c
> index 0ee588276534..37545ecb845a 100644
> --- a/drivers/dma-buf/dma-buf.c
> +++ b/drivers/dma-buf/dma-buf.c
> @@ -807,6 +807,8 @@ static struct sg_table * __map_dma_buf(struct dma_buf_attachment *attach,
> * Optionally this calls &dma_buf_ops.attach to allow device-specific attach
> * functionality.
> *
> + * Exporters shall use ww_ctx acquired by this function.
> + *
> * Returns:
> *
> * A pointer to newly created &dma_buf_attachment on success, or a negative
> @@ -822,6 +824,7 @@ dma_buf_dynamic_attach_unlocked(struct dma_buf *dmabuf, struct device *dev,
> void *importer_priv)
> {
> struct dma_buf_attachment *attach;
> + struct ww_acquire_ctx ww_ctx;
> int ret;
>
> if (WARN_ON(!dmabuf || !dev))
> @@ -841,7 +844,8 @@ dma_buf_dynamic_attach_unlocked(struct dma_buf *dmabuf, struct device *dev,
> attach->importer_ops = importer_ops;
> attach->importer_priv = importer_priv;
>
> - dma_resv_lock(dmabuf->resv, NULL);
> + ww_acquire_init(&ww_ctx, &reservation_ww_class);
> + dma_resv_lock(dmabuf->resv, &ww_ctx);
That won't work like this. The core property of a WW context is that you
need to unwind all the locks and re-quire them with the contended one first.
When you statically lock the imported one here you can't do that any more.
Regards,
Christian.
>
> if (dmabuf->ops->attach) {
> ret = dmabuf->ops->attach(dmabuf, attach);
> @@ -876,11 +880,13 @@ dma_buf_dynamic_attach_unlocked(struct dma_buf *dmabuf, struct device *dev,
> }
>
> dma_resv_unlock(dmabuf->resv);
> + ww_acquire_fini(&ww_ctx);
>
> return attach;
>
> err_attach:
> dma_resv_unlock(attach->dmabuf->resv);
> + ww_acquire_fini(&ww_ctx);
> kfree(attach);
> return ERR_PTR(ret);
>
> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c b/drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c
> index c199bf71c373..9173f0232b16 100644
> --- a/drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c
> +++ b/drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c
> @@ -173,7 +173,7 @@ static int i915_gem_dmabuf_attach(struct dma_buf *dmabuf,
> if (!i915_gem_object_can_migrate(obj, INTEL_REGION_SMEM))
> return -EOPNOTSUPP;
>
> - for_i915_gem_ww(&ww, err, true) {
> + for_i915_dmabuf_ww(&ww, dmabuf, err, true) {
> err = i915_gem_object_migrate(obj, &ww, INTEL_REGION_SMEM);
> if (err)
> continue;
> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
> index 30fe847c6664..ad7d602fc43a 100644
> --- a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
> +++ b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
> @@ -3409,7 +3409,7 @@ i915_gem_do_execbuffer(struct drm_device *dev,
> goto err_vma;
> }
>
> - ww_acquire_done(&eb.ww.ctx);
> + ww_acquire_done(eb.ww.ctx);
> eb_capture_stage(&eb);
>
> out_fence = eb_requests_create(&eb, in_fence, out_fence_fd);
> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_object.h b/drivers/gpu/drm/i915/gem/i915_gem_object.h
> index e11d82a9f7c3..5ae38f94a5c7 100644
> --- a/drivers/gpu/drm/i915/gem/i915_gem_object.h
> +++ b/drivers/gpu/drm/i915/gem/i915_gem_object.h
> @@ -178,9 +178,9 @@ static inline int __i915_gem_object_lock(struct drm_i915_gem_object *obj,
> int ret;
>
> if (intr)
> - ret = dma_resv_lock_interruptible(obj->base.resv, ww ? &ww->ctx : NULL);
> + ret = dma_resv_lock_interruptible(obj->base.resv, ww ? ww->ctx : NULL);
> else
> - ret = dma_resv_lock(obj->base.resv, ww ? &ww->ctx : NULL);
> + ret = dma_resv_lock(obj->base.resv, ww ? ww->ctx : NULL);
>
> if (!ret && ww) {
> i915_gem_object_get(obj);
> @@ -216,7 +216,7 @@ static inline bool i915_gem_object_trylock(struct drm_i915_gem_object *obj,
> if (!ww)
> return dma_resv_trylock(obj->base.resv);
> else
> - return ww_mutex_trylock(&obj->base.resv->lock, &ww->ctx);
> + return ww_mutex_trylock(&obj->base.resv->lock, ww->ctx);
> }
>
> static inline void i915_gem_object_unlock(struct drm_i915_gem_object *obj)
> diff --git a/drivers/gpu/drm/i915/i915_gem_evict.c b/drivers/gpu/drm/i915/i915_gem_evict.c
> index f025ee4fa526..047f72e32d47 100644
> --- a/drivers/gpu/drm/i915/i915_gem_evict.c
> +++ b/drivers/gpu/drm/i915/i915_gem_evict.c
> @@ -458,7 +458,7 @@ int i915_gem_evict_vm(struct i915_address_space *vm, struct i915_gem_ww_ctx *ww)
> * need the object ref.
> */
> if (dying_vma(vma) ||
> - (ww && (dma_resv_locking_ctx(vma->obj->base.resv) == &ww->ctx))) {
> + (ww && (dma_resv_locking_ctx(vma->obj->base.resv) == ww->ctx))) {
> __i915_vma_pin(vma);
> list_add(&vma->evict_link, &locked_eviction_list);
> continue;
> diff --git a/drivers/gpu/drm/i915/i915_gem_ww.c b/drivers/gpu/drm/i915/i915_gem_ww.c
> index 3f6ff139478e..c47898993c7d 100644
> --- a/drivers/gpu/drm/i915/i915_gem_ww.c
> +++ b/drivers/gpu/drm/i915/i915_gem_ww.c
> @@ -6,12 +6,20 @@
> #include "i915_gem_ww.h"
> #include "gem/i915_gem_object.h"
>
> -void i915_gem_ww_ctx_init(struct i915_gem_ww_ctx *ww, bool intr)
> +void i915_gem_ww_ctx_prep(struct i915_gem_ww_ctx *ww,
> + struct ww_acquire_ctx *ww_ctx,
> + bool intr)
> {
> - ww_acquire_init(&ww->ctx, &reservation_ww_class);
> INIT_LIST_HEAD(&ww->obj_list);
> ww->intr = intr;
> ww->contended = NULL;
> + ww->ctx = ww_ctx;
> +}
> +
> +void i915_gem_ww_ctx_init(struct i915_gem_ww_ctx *ww, bool intr)
> +{
> + ww_acquire_init(&ww->ww_ctx, &reservation_ww_class);
> + i915_gem_ww_ctx_prep(ww, &ww->ww_ctx, intr);
> }
>
> static void i915_gem_ww_ctx_unlock_all(struct i915_gem_ww_ctx *ww)
> @@ -36,7 +44,15 @@ void i915_gem_ww_ctx_fini(struct i915_gem_ww_ctx *ww)
> {
> i915_gem_ww_ctx_unlock_all(ww);
> WARN_ON(ww->contended);
> - ww_acquire_fini(&ww->ctx);
> +
> + if (ww->ctx == &ww->ww_ctx)
> + ww_acquire_fini(ww->ctx);
> +}
> +
> +void i915_gem_ww_ctx_fini2(struct i915_gem_ww_ctx *ww)
> +{
> + i915_gem_ww_ctx_unlock_all(ww);
> + WARN_ON(ww->contended);
> }
>
> int __must_check i915_gem_ww_ctx_backoff(struct i915_gem_ww_ctx *ww)
> @@ -48,9 +64,9 @@ int __must_check i915_gem_ww_ctx_backoff(struct i915_gem_ww_ctx *ww)
>
> i915_gem_ww_ctx_unlock_all(ww);
> if (ww->intr)
> - ret = dma_resv_lock_slow_interruptible(ww->contended->base.resv, &ww->ctx);
> + ret = dma_resv_lock_slow_interruptible(ww->contended->base.resv, ww->ctx);
> else
> - dma_resv_lock_slow(ww->contended->base.resv, &ww->ctx);
> + dma_resv_lock_slow(ww->contended->base.resv, ww->ctx);
>
> if (!ret)
> list_add_tail(&ww->contended->obj_link, &ww->obj_list);
> diff --git a/drivers/gpu/drm/i915/i915_gem_ww.h b/drivers/gpu/drm/i915/i915_gem_ww.h
> index 86f0fe343de6..e9b0fd4debbf 100644
> --- a/drivers/gpu/drm/i915/i915_gem_ww.h
> +++ b/drivers/gpu/drm/i915/i915_gem_ww.h
> @@ -8,13 +8,17 @@
> #include <drm/drm_drv.h>
>
> struct i915_gem_ww_ctx {
> - struct ww_acquire_ctx ctx;
> + struct ww_acquire_ctx *ctx;
> + struct ww_acquire_ctx ww_ctx;
> struct list_head obj_list;
> struct drm_i915_gem_object *contended;
> bool intr;
> };
>
> -void i915_gem_ww_ctx_init(struct i915_gem_ww_ctx *ctx, bool intr);
> +void i915_gem_ww_ctx_prep(struct i915_gem_ww_ctx *ww,
> + struct ww_acquire_ctx *ww_ctx,
> + bool intr);
> +void i915_gem_ww_ctx_init(struct i915_gem_ww_ctx *ww, bool intr);
> void i915_gem_ww_ctx_fini(struct i915_gem_ww_ctx *ctx);
> int __must_check i915_gem_ww_ctx_backoff(struct i915_gem_ww_ctx *ctx);
> void i915_gem_ww_unlock_single(struct drm_i915_gem_object *obj);
> @@ -38,4 +42,11 @@ static inline int __i915_gem_ww_fini(struct i915_gem_ww_ctx *ww, int err)
> for (i915_gem_ww_ctx_init(_ww, _intr), (_err) = -EDEADLK; \
> (_err) == -EDEADLK; \
> (_err) = __i915_gem_ww_fini(_ww, _err))
> +
> +#define for_i915_dmabuf_ww(_ww, _dmabuf, _err, _intr) \
> + for (i915_gem_ww_ctx_prep(_ww, dma_resv_locking_ctx((_dmabuf)->resv), _intr), \
> + (_err) = -EDEADLK; \
> + (_err) == -EDEADLK; \
> + (_err) = __i915_gem_ww_fini(_ww, _err))
> +
> #endif
On 7/15/22 09:50, Christian König wrote:
> Am 15.07.22 um 02:52 schrieb Dmitry Osipenko:
>> Intel i915 GPU driver uses wait-wound mutex to lock multiple GEMs on the
>> attachment to the i915 dma-buf. In order to let all drivers utilize
>> shared
>> wait-wound context during attachment in a general way, make dma-buf
>> core to
>> acquire the ww context internally for the attachment operation and update
>> i915 driver to use the importer's ww context instead of the internal one.
>>
>> From now on all dma-buf exporters shall use the importer's ww context
>> for
>> the attachment operation.
>>
>> Signed-off-by: Dmitry Osipenko <dmitry.osipenko@collabora.com>
>> ---
>> drivers/dma-buf/dma-buf.c | 8 +++++-
>> drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c | 2 +-
>> .../gpu/drm/i915/gem/i915_gem_execbuffer.c | 2 +-
>> drivers/gpu/drm/i915/gem/i915_gem_object.h | 6 ++---
>> drivers/gpu/drm/i915/i915_gem_evict.c | 2 +-
>> drivers/gpu/drm/i915/i915_gem_ww.c | 26 +++++++++++++++----
>> drivers/gpu/drm/i915/i915_gem_ww.h | 15 +++++++++--
>> 7 files changed, 47 insertions(+), 14 deletions(-)
>>
>> diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c
>> index 0ee588276534..37545ecb845a 100644
>> --- a/drivers/dma-buf/dma-buf.c
>> +++ b/drivers/dma-buf/dma-buf.c
>> @@ -807,6 +807,8 @@ static struct sg_table * __map_dma_buf(struct
>> dma_buf_attachment *attach,
>> * Optionally this calls &dma_buf_ops.attach to allow
>> device-specific attach
>> * functionality.
>> *
>> + * Exporters shall use ww_ctx acquired by this function.
>> + *
>> * Returns:
>> *
>> * A pointer to newly created &dma_buf_attachment on success, or a
>> negative
>> @@ -822,6 +824,7 @@ dma_buf_dynamic_attach_unlocked(struct dma_buf
>> *dmabuf, struct device *dev,
>> void *importer_priv)
>> {
>> struct dma_buf_attachment *attach;
>> + struct ww_acquire_ctx ww_ctx;
>> int ret;
>> if (WARN_ON(!dmabuf || !dev))
>> @@ -841,7 +844,8 @@ dma_buf_dynamic_attach_unlocked(struct dma_buf
>> *dmabuf, struct device *dev,
>> attach->importer_ops = importer_ops;
>> attach->importer_priv = importer_priv;
>> - dma_resv_lock(dmabuf->resv, NULL);
>> + ww_acquire_init(&ww_ctx, &reservation_ww_class);
>> + dma_resv_lock(dmabuf->resv, &ww_ctx);
>
> That won't work like this. The core property of a WW context is that you
> need to unwind all the locks and re-quire them with the contended one
> first.
>
> When you statically lock the imported one here you can't do that any more.
You're right. I felt that something is missing here, but couldn't
notice. I'll think more about this and enable
CONFIG_DEBUG_WW_MUTEX_SLOWPATH. Thank you!
On 7/15/22 09:59, Dmitry Osipenko wrote:
> On 7/15/22 09:50, Christian König wrote:
>> Am 15.07.22 um 02:52 schrieb Dmitry Osipenko:
>>> Intel i915 GPU driver uses wait-wound mutex to lock multiple GEMs on the
>>> attachment to the i915 dma-buf. In order to let all drivers utilize
>>> shared
>>> wait-wound context during attachment in a general way, make dma-buf
>>> core to
>>> acquire the ww context internally for the attachment operation and update
>>> i915 driver to use the importer's ww context instead of the internal one.
>>>
>>> From now on all dma-buf exporters shall use the importer's ww context
>>> for
>>> the attachment operation.
>>>
>>> Signed-off-by: Dmitry Osipenko <dmitry.osipenko@collabora.com>
>>> ---
>>> drivers/dma-buf/dma-buf.c | 8 +++++-
>>> drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c | 2 +-
>>> .../gpu/drm/i915/gem/i915_gem_execbuffer.c | 2 +-
>>> drivers/gpu/drm/i915/gem/i915_gem_object.h | 6 ++---
>>> drivers/gpu/drm/i915/i915_gem_evict.c | 2 +-
>>> drivers/gpu/drm/i915/i915_gem_ww.c | 26 +++++++++++++++----
>>> drivers/gpu/drm/i915/i915_gem_ww.h | 15 +++++++++--
>>> 7 files changed, 47 insertions(+), 14 deletions(-)
>>>
>>> diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c
>>> index 0ee588276534..37545ecb845a 100644
>>> --- a/drivers/dma-buf/dma-buf.c
>>> +++ b/drivers/dma-buf/dma-buf.c
>>> @@ -807,6 +807,8 @@ static struct sg_table * __map_dma_buf(struct
>>> dma_buf_attachment *attach,
>>> * Optionally this calls &dma_buf_ops.attach to allow
>>> device-specific attach
>>> * functionality.
>>> *
>>> + * Exporters shall use ww_ctx acquired by this function.
>>> + *
>>> * Returns:
>>> *
>>> * A pointer to newly created &dma_buf_attachment on success, or a
>>> negative
>>> @@ -822,6 +824,7 @@ dma_buf_dynamic_attach_unlocked(struct dma_buf
>>> *dmabuf, struct device *dev,
>>> void *importer_priv)
>>> {
>>> struct dma_buf_attachment *attach;
>>> + struct ww_acquire_ctx ww_ctx;
>>> int ret;
>>> if (WARN_ON(!dmabuf || !dev))
>>> @@ -841,7 +844,8 @@ dma_buf_dynamic_attach_unlocked(struct dma_buf
>>> *dmabuf, struct device *dev,
>>> attach->importer_ops = importer_ops;
>>> attach->importer_priv = importer_priv;
>>> - dma_resv_lock(dmabuf->resv, NULL);
>>> + ww_acquire_init(&ww_ctx, &reservation_ww_class);
>>> + dma_resv_lock(dmabuf->resv, &ww_ctx);
>>
>> That won't work like this. The core property of a WW context is that you
>> need to unwind all the locks and re-quire them with the contended one
>> first.
>>
>> When you statically lock the imported one here you can't do that any more.
>
> You're right. I felt that something is missing here, but couldn't
> notice. I'll think more about this and enable
> CONFIG_DEBUG_WW_MUTEX_SLOWPATH. Thank you!
>
Christian, do you think we could make an excuse for the attach()
callback and make the exporter responsible for taking the resv lock? It
will be inconsistent with the rest of the callbacks, where importer
takes the lock, but it will be the simplest and least invasive solution.
It's very messy to do a cross-driver ww locking, I don't think it's the
right approach.
Am 19.07.22 um 22:05 schrieb Dmitry Osipenko:
> On 7/15/22 09:59, Dmitry Osipenko wrote:
>> On 7/15/22 09:50, Christian König wrote:
>>> Am 15.07.22 um 02:52 schrieb Dmitry Osipenko:
>>>> Intel i915 GPU driver uses wait-wound mutex to lock multiple GEMs on the
>>>> attachment to the i915 dma-buf. In order to let all drivers utilize
>>>> shared
>>>> wait-wound context during attachment in a general way, make dma-buf
>>>> core to
>>>> acquire the ww context internally for the attachment operation and update
>>>> i915 driver to use the importer's ww context instead of the internal one.
>>>>
>>>> From now on all dma-buf exporters shall use the importer's ww context
>>>> for
>>>> the attachment operation.
>>>>
>>>> Signed-off-by: Dmitry Osipenko <dmitry.osipenko@collabora.com>
>>>> ---
>>>> drivers/dma-buf/dma-buf.c | 8 +++++-
>>>> drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c | 2 +-
>>>> .../gpu/drm/i915/gem/i915_gem_execbuffer.c | 2 +-
>>>> drivers/gpu/drm/i915/gem/i915_gem_object.h | 6 ++---
>>>> drivers/gpu/drm/i915/i915_gem_evict.c | 2 +-
>>>> drivers/gpu/drm/i915/i915_gem_ww.c | 26 +++++++++++++++----
>>>> drivers/gpu/drm/i915/i915_gem_ww.h | 15 +++++++++--
>>>> 7 files changed, 47 insertions(+), 14 deletions(-)
>>>>
>>>> diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c
>>>> index 0ee588276534..37545ecb845a 100644
>>>> --- a/drivers/dma-buf/dma-buf.c
>>>> +++ b/drivers/dma-buf/dma-buf.c
>>>> @@ -807,6 +807,8 @@ static struct sg_table * __map_dma_buf(struct
>>>> dma_buf_attachment *attach,
>>>> * Optionally this calls &dma_buf_ops.attach to allow
>>>> device-specific attach
>>>> * functionality.
>>>> *
>>>> + * Exporters shall use ww_ctx acquired by this function.
>>>> + *
>>>> * Returns:
>>>> *
>>>> * A pointer to newly created &dma_buf_attachment on success, or a
>>>> negative
>>>> @@ -822,6 +824,7 @@ dma_buf_dynamic_attach_unlocked(struct dma_buf
>>>> *dmabuf, struct device *dev,
>>>> void *importer_priv)
>>>> {
>>>> struct dma_buf_attachment *attach;
>>>> + struct ww_acquire_ctx ww_ctx;
>>>> int ret;
>>>> if (WARN_ON(!dmabuf || !dev))
>>>> @@ -841,7 +844,8 @@ dma_buf_dynamic_attach_unlocked(struct dma_buf
>>>> *dmabuf, struct device *dev,
>>>> attach->importer_ops = importer_ops;
>>>> attach->importer_priv = importer_priv;
>>>> - dma_resv_lock(dmabuf->resv, NULL);
>>>> + ww_acquire_init(&ww_ctx, &reservation_ww_class);
>>>> + dma_resv_lock(dmabuf->resv, &ww_ctx);
>>> That won't work like this. The core property of a WW context is that you
>>> need to unwind all the locks and re-quire them with the contended one
>>> first.
>>>
>>> When you statically lock the imported one here you can't do that any more.
>> You're right. I felt that something is missing here, but couldn't
>> notice. I'll think more about this and enable
>> CONFIG_DEBUG_WW_MUTEX_SLOWPATH. Thank you!
>>
> Christian, do you think we could make an excuse for the attach()
> callback and make the exporter responsible for taking the resv lock? It
> will be inconsistent with the rest of the callbacks, where importer
> takes the lock, but it will be the simplest and least invasive solution.
> It's very messy to do a cross-driver ww locking, I don't think it's the
> right approach.
So to summarize the following calls will require that the caller hold
the resv lock:
1. dma_buf_pin()/dma_buf_unpin()
2. dma_buf_map_attachment()/dma_buf_unmap_attachment()
3. dma_buf_vmap()/dma_buf_vunmap()
4. dma_buf_move_notify()
The following calls require that caller does not held the resv lock:
1. dma_buf_attach()/dma_buf_dynamic_attach()/dma_buf_detach()
2. dma_buf_export()/dma_buf_fd()
3. dma_buf_get()/dma_buf_put()
4. dma_buf_begin_cpu_access()/dma_buf_end_cpu_access()
If that's correct than that would work for me as well, but we should
probably document this.
Or let me ask the other way around: What calls exactly do you need to
change to solve your original issue? That was vmap/vunmap, wasn't it? If
yes then let's concentrate on those for the moment.
Regards,
Christian.
On 7/20/22 11:29, Christian König wrote:
> Am 19.07.22 um 22:05 schrieb Dmitry Osipenko:
>> On 7/15/22 09:59, Dmitry Osipenko wrote:
>>> On 7/15/22 09:50, Christian König wrote:
>>>> Am 15.07.22 um 02:52 schrieb Dmitry Osipenko:
>>>>> Intel i915 GPU driver uses wait-wound mutex to lock multiple GEMs
>>>>> on the
>>>>> attachment to the i915 dma-buf. In order to let all drivers utilize
>>>>> shared
>>>>> wait-wound context during attachment in a general way, make dma-buf
>>>>> core to
>>>>> acquire the ww context internally for the attachment operation and
>>>>> update
>>>>> i915 driver to use the importer's ww context instead of the
>>>>> internal one.
>>>>>
>>>>> From now on all dma-buf exporters shall use the importer's ww
>>>>> context
>>>>> for
>>>>> the attachment operation.
>>>>>
>>>>> Signed-off-by: Dmitry Osipenko <dmitry.osipenko@collabora.com>
>>>>> ---
>>>>> drivers/dma-buf/dma-buf.c | 8 +++++-
>>>>> drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c | 2 +-
>>>>> .../gpu/drm/i915/gem/i915_gem_execbuffer.c | 2 +-
>>>>> drivers/gpu/drm/i915/gem/i915_gem_object.h | 6 ++---
>>>>> drivers/gpu/drm/i915/i915_gem_evict.c | 2 +-
>>>>> drivers/gpu/drm/i915/i915_gem_ww.c | 26
>>>>> +++++++++++++++----
>>>>> drivers/gpu/drm/i915/i915_gem_ww.h | 15 +++++++++--
>>>>> 7 files changed, 47 insertions(+), 14 deletions(-)
>>>>>
>>>>> diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c
>>>>> index 0ee588276534..37545ecb845a 100644
>>>>> --- a/drivers/dma-buf/dma-buf.c
>>>>> +++ b/drivers/dma-buf/dma-buf.c
>>>>> @@ -807,6 +807,8 @@ static struct sg_table * __map_dma_buf(struct
>>>>> dma_buf_attachment *attach,
>>>>> * Optionally this calls &dma_buf_ops.attach to allow
>>>>> device-specific attach
>>>>> * functionality.
>>>>> *
>>>>> + * Exporters shall use ww_ctx acquired by this function.
>>>>> + *
>>>>> * Returns:
>>>>> *
>>>>> * A pointer to newly created &dma_buf_attachment on success, or a
>>>>> negative
>>>>> @@ -822,6 +824,7 @@ dma_buf_dynamic_attach_unlocked(struct dma_buf
>>>>> *dmabuf, struct device *dev,
>>>>> void *importer_priv)
>>>>> {
>>>>> struct dma_buf_attachment *attach;
>>>>> + struct ww_acquire_ctx ww_ctx;
>>>>> int ret;
>>>>> if (WARN_ON(!dmabuf || !dev))
>>>>> @@ -841,7 +844,8 @@ dma_buf_dynamic_attach_unlocked(struct dma_buf
>>>>> *dmabuf, struct device *dev,
>>>>> attach->importer_ops = importer_ops;
>>>>> attach->importer_priv = importer_priv;
>>>>> - dma_resv_lock(dmabuf->resv, NULL);
>>>>> + ww_acquire_init(&ww_ctx, &reservation_ww_class);
>>>>> + dma_resv_lock(dmabuf->resv, &ww_ctx);
>>>> That won't work like this. The core property of a WW context is that
>>>> you
>>>> need to unwind all the locks and re-quire them with the contended one
>>>> first.
>>>>
>>>> When you statically lock the imported one here you can't do that any
>>>> more.
>>> You're right. I felt that something is missing here, but couldn't
>>> notice. I'll think more about this and enable
>>> CONFIG_DEBUG_WW_MUTEX_SLOWPATH. Thank you!
>>>
>> Christian, do you think we could make an excuse for the attach()
>> callback and make the exporter responsible for taking the resv lock? It
>> will be inconsistent with the rest of the callbacks, where importer
>> takes the lock, but it will be the simplest and least invasive solution.
>> It's very messy to do a cross-driver ww locking, I don't think it's the
>> right approach.
>
> So to summarize the following calls will require that the caller hold
> the resv lock:
> 1. dma_buf_pin()/dma_buf_unpin()
> 2. dma_buf_map_attachment()/dma_buf_unmap_attachment()
> 3. dma_buf_vmap()/dma_buf_vunmap()
> 4. dma_buf_move_notify()
>
> The following calls require that caller does not held the resv lock:
> 1. dma_buf_attach()/dma_buf_dynamic_attach()/dma_buf_detach()
> 2. dma_buf_export()/dma_buf_fd()
> 3. dma_buf_get()/dma_buf_put()
> 4. dma_buf_begin_cpu_access()/dma_buf_end_cpu_access()
>
> If that's correct than that would work for me as well, but we should
> probably document this.
Looks good, thank you. I'll try this variant.
> Or let me ask the other way around: What calls exactly do you need to
> change to solve your original issue? That was vmap/vunmap, wasn't it? If
> yes then let's concentrate on those for the moment.
Originally, Daniel Vetter asked to sort out the dma-buf lockings across
all drivers, so we could replace custom locks in DRM-SHMEM with the resv
lock, otherwise there were no guarantees that we won't have deadlocks in
the dma-buf code paths.
The vmap/vunmap is one of the paths that needs to be sorted out, there
is no particular issue with it, just need to specify the convention. The
mmaping was the other questionable path and we concluded that it's
better to prohibit dma-buf mappings for DRM entirely. Lastly, there is
i915 attach() that uses the ww locking.
@@ -807,6 +807,8 @@ static struct sg_table * __map_dma_buf(struct dma_buf_attachment *attach,
* Optionally this calls &dma_buf_ops.attach to allow device-specific attach
* functionality.
*
+ * Exporters shall use ww_ctx acquired by this function.
+ *
* Returns:
*
* A pointer to newly created &dma_buf_attachment on success, or a negative
@@ -822,6 +824,7 @@ dma_buf_dynamic_attach_unlocked(struct dma_buf *dmabuf, struct device *dev,
void *importer_priv)
{
struct dma_buf_attachment *attach;
+ struct ww_acquire_ctx ww_ctx;
int ret;
if (WARN_ON(!dmabuf || !dev))
@@ -841,7 +844,8 @@ dma_buf_dynamic_attach_unlocked(struct dma_buf *dmabuf, struct device *dev,
attach->importer_ops = importer_ops;
attach->importer_priv = importer_priv;
- dma_resv_lock(dmabuf->resv, NULL);
+ ww_acquire_init(&ww_ctx, &reservation_ww_class);
+ dma_resv_lock(dmabuf->resv, &ww_ctx);
if (dmabuf->ops->attach) {
ret = dmabuf->ops->attach(dmabuf, attach);
@@ -876,11 +880,13 @@ dma_buf_dynamic_attach_unlocked(struct dma_buf *dmabuf, struct device *dev,
}
dma_resv_unlock(dmabuf->resv);
+ ww_acquire_fini(&ww_ctx);
return attach;
err_attach:
dma_resv_unlock(attach->dmabuf->resv);
+ ww_acquire_fini(&ww_ctx);
kfree(attach);
return ERR_PTR(ret);
@@ -173,7 +173,7 @@ static int i915_gem_dmabuf_attach(struct dma_buf *dmabuf,
if (!i915_gem_object_can_migrate(obj, INTEL_REGION_SMEM))
return -EOPNOTSUPP;
- for_i915_gem_ww(&ww, err, true) {
+ for_i915_dmabuf_ww(&ww, dmabuf, err, true) {
err = i915_gem_object_migrate(obj, &ww, INTEL_REGION_SMEM);
if (err)
continue;
@@ -3409,7 +3409,7 @@ i915_gem_do_execbuffer(struct drm_device *dev,
goto err_vma;
}
- ww_acquire_done(&eb.ww.ctx);
+ ww_acquire_done(eb.ww.ctx);
eb_capture_stage(&eb);
out_fence = eb_requests_create(&eb, in_fence, out_fence_fd);
@@ -178,9 +178,9 @@ static inline int __i915_gem_object_lock(struct drm_i915_gem_object *obj,
int ret;
if (intr)
- ret = dma_resv_lock_interruptible(obj->base.resv, ww ? &ww->ctx : NULL);
+ ret = dma_resv_lock_interruptible(obj->base.resv, ww ? ww->ctx : NULL);
else
- ret = dma_resv_lock(obj->base.resv, ww ? &ww->ctx : NULL);
+ ret = dma_resv_lock(obj->base.resv, ww ? ww->ctx : NULL);
if (!ret && ww) {
i915_gem_object_get(obj);
@@ -216,7 +216,7 @@ static inline bool i915_gem_object_trylock(struct drm_i915_gem_object *obj,
if (!ww)
return dma_resv_trylock(obj->base.resv);
else
- return ww_mutex_trylock(&obj->base.resv->lock, &ww->ctx);
+ return ww_mutex_trylock(&obj->base.resv->lock, ww->ctx);
}
static inline void i915_gem_object_unlock(struct drm_i915_gem_object *obj)
@@ -458,7 +458,7 @@ int i915_gem_evict_vm(struct i915_address_space *vm, struct i915_gem_ww_ctx *ww)
* need the object ref.
*/
if (dying_vma(vma) ||
- (ww && (dma_resv_locking_ctx(vma->obj->base.resv) == &ww->ctx))) {
+ (ww && (dma_resv_locking_ctx(vma->obj->base.resv) == ww->ctx))) {
__i915_vma_pin(vma);
list_add(&vma->evict_link, &locked_eviction_list);
continue;
@@ -6,12 +6,20 @@
#include "i915_gem_ww.h"
#include "gem/i915_gem_object.h"
-void i915_gem_ww_ctx_init(struct i915_gem_ww_ctx *ww, bool intr)
+void i915_gem_ww_ctx_prep(struct i915_gem_ww_ctx *ww,
+ struct ww_acquire_ctx *ww_ctx,
+ bool intr)
{
- ww_acquire_init(&ww->ctx, &reservation_ww_class);
INIT_LIST_HEAD(&ww->obj_list);
ww->intr = intr;
ww->contended = NULL;
+ ww->ctx = ww_ctx;
+}
+
+void i915_gem_ww_ctx_init(struct i915_gem_ww_ctx *ww, bool intr)
+{
+ ww_acquire_init(&ww->ww_ctx, &reservation_ww_class);
+ i915_gem_ww_ctx_prep(ww, &ww->ww_ctx, intr);
}
static void i915_gem_ww_ctx_unlock_all(struct i915_gem_ww_ctx *ww)
@@ -36,7 +44,15 @@ void i915_gem_ww_ctx_fini(struct i915_gem_ww_ctx *ww)
{
i915_gem_ww_ctx_unlock_all(ww);
WARN_ON(ww->contended);
- ww_acquire_fini(&ww->ctx);
+
+ if (ww->ctx == &ww->ww_ctx)
+ ww_acquire_fini(ww->ctx);
+}
+
+void i915_gem_ww_ctx_fini2(struct i915_gem_ww_ctx *ww)
+{
+ i915_gem_ww_ctx_unlock_all(ww);
+ WARN_ON(ww->contended);
}
int __must_check i915_gem_ww_ctx_backoff(struct i915_gem_ww_ctx *ww)
@@ -48,9 +64,9 @@ int __must_check i915_gem_ww_ctx_backoff(struct i915_gem_ww_ctx *ww)
i915_gem_ww_ctx_unlock_all(ww);
if (ww->intr)
- ret = dma_resv_lock_slow_interruptible(ww->contended->base.resv, &ww->ctx);
+ ret = dma_resv_lock_slow_interruptible(ww->contended->base.resv, ww->ctx);
else
- dma_resv_lock_slow(ww->contended->base.resv, &ww->ctx);
+ dma_resv_lock_slow(ww->contended->base.resv, ww->ctx);
if (!ret)
list_add_tail(&ww->contended->obj_link, &ww->obj_list);
@@ -8,13 +8,17 @@
#include <drm/drm_drv.h>
struct i915_gem_ww_ctx {
- struct ww_acquire_ctx ctx;
+ struct ww_acquire_ctx *ctx;
+ struct ww_acquire_ctx ww_ctx;
struct list_head obj_list;
struct drm_i915_gem_object *contended;
bool intr;
};
-void i915_gem_ww_ctx_init(struct i915_gem_ww_ctx *ctx, bool intr);
+void i915_gem_ww_ctx_prep(struct i915_gem_ww_ctx *ww,
+ struct ww_acquire_ctx *ww_ctx,
+ bool intr);
+void i915_gem_ww_ctx_init(struct i915_gem_ww_ctx *ww, bool intr);
void i915_gem_ww_ctx_fini(struct i915_gem_ww_ctx *ctx);
int __must_check i915_gem_ww_ctx_backoff(struct i915_gem_ww_ctx *ctx);
void i915_gem_ww_unlock_single(struct drm_i915_gem_object *obj);
@@ -38,4 +42,11 @@ static inline int __i915_gem_ww_fini(struct i915_gem_ww_ctx *ww, int err)
for (i915_gem_ww_ctx_init(_ww, _intr), (_err) = -EDEADLK; \
(_err) == -EDEADLK; \
(_err) = __i915_gem_ww_fini(_ww, _err))
+
+#define for_i915_dmabuf_ww(_ww, _dmabuf, _err, _intr) \
+ for (i915_gem_ww_ctx_prep(_ww, dma_resv_locking_ctx((_dmabuf)->resv), _intr), \
+ (_err) = -EDEADLK; \
+ (_err) == -EDEADLK; \
+ (_err) = __i915_gem_ww_fini(_ww, _err))
+
#endif