[Mesa-dev] [PATCH 3/4] i965/bufmgr: Add a set_tiling helper

[Kenneth Graunke]

On Thursday, January 18, 2018 7:05:46 PM PST Jason Ekstrand wrote:
> On Thu, Jan 18, 2018 at 4:30 PM, Kenneth Graunke <kenneth at whitecape.org>
> wrote:
> 
> > 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-stable at 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.
> > > -Chris
> >
> > I 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.
> >
> 
> I can rework things to do it that way.  I thought about it briefly but I
> got hung up on how to handle the "use the tiling from the BO" case.  I
> think I can use an int and pass -1 or something.  Or maybe add a
> create_from_prime_tiled helper.
> 
> --Jason

Hmm, right...I was thinking the obvious solution would be to have
intel_create_image_from_fds_common do GET_TILING if modifier ==
DRM_FORMAT_MOD_INVALID.  But, it doesn't have the GEM handle.

Passing -1 for the tiling to mean "get the BO tiling" seems fine to me.

--Ken
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: This is a digitally signed message part.
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20180118/dbfae029/attachment-0001.sig>