<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Thu, Jan 18, 2018 at 4:30 PM, Kenneth Graunke <span dir="ltr"><<a href="mailto:kenneth@whitecape.org" target="_blank">kenneth@whitecape.org</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="HOEnZb"><div class="h5">On Friday, January 12, 2018 1:28:00 AM PST Chris Wilson wrote:<br>
> Quoting Jason Ekstrand (2018-01-12 01:40:52)<br>
> > This helper should be used carefully as setting tiling is a racy<br>
> > operation since it potentially interacts with other processes.  Still,<br>
> > it is a useful thing to be able to do.<br>
> ><br>
> > Cc: <a href="mailto:mesa-stable@lists.freedesktop.org">mesa-stable@lists.freedesktop.<wbr>org</a><br>
> > ---<br>
> >  src/mesa/drivers/dri/i965/brw_<wbr>bufmgr.c | 27 +++++++++++++++++++++++++++<br>
> >  src/mesa/drivers/dri/i965/brw_<wbr>bufmgr.h | 10 ++++++++++<br>
> >  2 files changed, 37 insertions(+)<br>
> ><br>
> > diff --git a/src/mesa/drivers/dri/i965/<wbr>brw_bufmgr.c b/src/mesa/drivers/dri/i965/<wbr>brw_bufmgr.c<br>
> > index 469895e..dbd13dd 100644<br>
> > --- a/src/mesa/drivers/dri/i965/<wbr>brw_bufmgr.c<br>
> > +++ b/src/mesa/drivers/dri/i965/<wbr>brw_bufmgr.c<br>
> > @@ -1133,6 +1133,33 @@ brw_bo_get_tiling(struct brw_bo *bo, uint32_t *tiling_mode,<br>
> >     return 0;<br>
> >  }<br>
> ><br>
> > +int<br>
> > +brw_bo_set_tiling(struct brw_bo *bo, uint32_t tiling_mode,<br>
> > +                  uint32_t stride)<br>
> > +{<br>
> > +   struct brw_bufmgr *bufmgr = bo->bufmgr;<br>
> > +<br>
> > +   mtx_lock(&bufmgr->lock);<br>
><br>
> This mutex protects the buffer cache, which should not be containing<br>
> this bo. Changing the tiling of a shared active bo also should not<br>
> happen because the other parties will have already derived state from<br>
> the older tiling. So I don't see the purpose of this mutex here.<br>
><br>
> If we will need exclusive access to a bo, let's have a bo->lock.<br>
> -Chris<br>
<br>
</div></div>I agree with Chris, I don't like this locking.  Looking more closely,<br>
I don't think patches 3-4 are what you want at all.<br>
<br>
intel_create_image_from_fds_<wbr>common calls brw_bo_gem_create_from_prime.<br>
It's the only caller of that function.<br>
<br>
brw_bo_gem_create_from_prime allocates a new BO, does GET_TILING, and<br>
inserts it into the cache.  This is pointless after your patch 4,<br>
because you immediately whack it.<br>
<br>
So...instead...just make brw_bo_gem_create_from_prime take a tiling,<br>
and SET_TILING it...don't bother with GET_TILING at all.  Then, your<br>
BO isn't in the cache yet (or you have the cache locked), and it's<br>
safe without a race and redundant work.<br>
</blockquote></div></div><div class="gmail_extra"><br></div><div class="gmail_extra">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.</div><div class="gmail_extra"><br></div><div class="gmail_extra">--Jason<br></div></div>