Am 24.11.20 um 14:56 schrieb Thomas Zimmermann:
Hi
Am 24.11.20 um 14:36 schrieb Christian König:
Am 24.11.20 um 13:15 schrieb Thomas Zimmermann:
[SNIP]
First I wanted to put this into drm_gem_ttm_vmap/vunmap(), but
then wondered why ttm_bo_vmap() doe not acquire the lock
internally? I'd expect that vmap/vunmap are close together and
do not overlap for the same BO.
We have use cases like the following during command submission:
1. lock
2. map
3. copy parts of the BO content somewhere else or patch it with
additional information
4. unmap
5. submit BO to the hardware
6. add hardware fence to the BO to make sure it doesn't move
7. unlock
That use case won't be possible with vmap/vunmap if we move the
lock/unlock into it and I hope to replace the kmap/kunmap
functions with them in the near term.
Otherwise, acquiring the reservation lock would require another
ref-counting variable or per-driver code.
Hui, why that? Just put this into drm_gem_ttm_vmap/vunmap()
helper as you initially planned.
Given your example above, step one would acquire the lock, and
step two would also acquire the lock as part of the vmap
implementation. Wouldn't this fail (At least during unmap or
unlock steps) ?
Oh, so you want to nest them? No, that is a rather bad no-go.
I don't want to nest/overlap them. My question was whether that
would be required. Apparently not.
While the console's BO is being set for scanout, it's protected from
movement via the pin/unpin implementation, right?
Yes, correct.
The driver does not acquire the resv lock for longer periods. I'm
asking because this would prevent any console-buffer updates while
the console is being displayed.
Correct as well, we only hold the lock for things like command
submission, pinning, unpinning etc etc....
Thanks for answering my questions.
You need to make sure that the lock is only taken from the FB path
which wants to vmap the object.
Why don't you lock the GEM object from the caller in the generic FB
implementation?
With the current blitter code, it breaks abstraction. if vmap/vunmap
hold the lock implicitly, things would be easier.
Do you have a link to the code?
It's the damage blitter in the fbdev code. [1] While it flushes the
shadow buffer into the BO, the BO has to be kept in place. I already
changed it to lock struct drm_fb_helper.lock, but I don't think this
is enough. TTM could still evict the BO concurrently.
Yeah, that's correct.
But I still don't fully understand the problem. You just need to change
the code like this:
mutex_lock(&fb_helper->lock);
dma_resv_lock(buffer->gem->resv, NULL);
ret = drm_client_buffer_vmap(buffer, &map);
if (ret)
goto out;
dst = map;
drm_fb_helper_damage_blit_real(fb_helper, clip, &dst);
drm_client_buffer_vunmap(buffer);
out:
dma_resv_unlock(buffer->gem->resv);
mutex_unlock(&fb_helper->lock);
You could abstract that in drm_client functions as well, but I don't
really see the value in that.
Regards,
Christian.
There's no recursion taking place, so I guess the reservation lock
could be acquired/release in drm_client_buffer_vmap/vunmap(), or a
separate pair of DRM client functions could do the locking.
Best regards
Thomas
[1]
https://cgit.freedesktop.org/drm/drm-tip/tree/drivers/gpu/drm/drm_fb_helper.c?id=ac60f3f3090115d21f028bffa2dcfb67f695c4f2#n394
Please note that the reservation lock you need to take here is part
of the GEM object.
Usually we design things in the way that the code needs to take a
lock which protects an object, then do some operations with the
object and then release the lock again.
Having in the lock inside the operation can be done as well, but
returning with it is kind of unusual design.
Sorry for the noob questions. I'm still trying to understand the
implications of acquiring these locks.
Well this is the reservation lock of the GEM object we are talking
about here. We need to take that for a couple of different
operations, vmap/vunmap doesn't sound like a special case to me.
Regards,
Christian.
Best regards
Thomas
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
