On Friday, January 12, 2018 1:28:00 AM PST Chris Wilson wrote:
> Quoting Jason Ekstrand (2018-01-12 01:40:52)
> > This helper should be used carefully as setting tiling is a racy
> > operation since it potentially interacts with other processes. Still,
> > it is a useful thing to be able to do.
> >
> > Cc: mesa-sta...@lists.freedesktop.org
> > ---
> > src/mesa/drivers/dri/i965/brw_bufmgr.c | 27 +++++++++++++++++++++++++++
> > src/mesa/drivers/dri/i965/brw_bufmgr.h | 10 ++++++++++
> > 2 files changed, 37 insertions(+)
> >
> > diff --git a/src/mesa/drivers/dri/i965/brw_bufmgr.c
> > b/src/mesa/drivers/dri/i965/brw_bufmgr.c
> > index 469895e..dbd13dd 100644
> > --- a/src/mesa/drivers/dri/i965/brw_bufmgr.c
> > +++ b/src/mesa/drivers/dri/i965/brw_bufmgr.c
> > @@ -1133,6 +1133,33 @@ brw_bo_get_tiling(struct brw_bo *bo, uint32_t
> > *tiling_mode,
> > return 0;
> > }
> >
> > +int
> > +brw_bo_set_tiling(struct brw_bo *bo, uint32_t tiling_mode,
> > + uint32_t stride)
> > +{
> > + struct brw_bufmgr *bufmgr = bo->bufmgr;
> > +
> > + mtx_lock(&bufmgr->lock);
>
> This mutex protects the buffer cache, which should not be containing
> this bo. Changing the tiling of a shared active bo also should not
> happen because the other parties will have already derived state from
> the older tiling. So I don't see the purpose of this mutex here.
>
> If we will need exclusive access to a bo, let's have a bo->lock.
> -ChrisI agree with Chris, I don't like this locking. Looking more closely, I don't think patches 3-4 are what you want at all. intel_create_image_from_fds_common calls brw_bo_gem_create_from_prime. It's the only caller of that function. brw_bo_gem_create_from_prime allocates a new BO, does GET_TILING, and inserts it into the cache. This is pointless after your patch 4, because you immediately whack it. So...instead...just make brw_bo_gem_create_from_prime take a tiling, and SET_TILING it...don't bother with GET_TILING at all. Then, your BO isn't in the cache yet (or you have the cache locked), and it's safe without a race and redundant work.
signature.asc
Description: This is a digitally signed message part.
_______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
