--Jason > > + * resource protected by the dma_resv object. The only exception > for > > + * that is when the resource is known to be locked down in place = by > > + * pinning it previously. > > Is this true? This sounds more confusing than helpful, because afaik in > general our pin interfaces do not block for any kernel fences. dma_buf_pi= n > doesn't do that for sure. And I don't think ttm does that either. > > I think the only safe thing here is to state that it's safe if a) the > resource is pinned down and b) the callers has previously waited for the > kernel fences. > > I also think we should put that wait for kernel fences into dma_buf_pin()= , > but that's maybe a later patch. > -Daniel > > > > > + */ > > + DMA_RESV_USAGE_KERNEL, > > + > > /** > > * @DMA_RESV_USAGE_WRITE: Implicit write synchronization. > > * > > -- > > 2.25.1 > > > > -- > Daniel Vetter > Software Engineer, Intel Corporation > http://blog.ffwll.ch > --000000000000ae9a2305d9403789 Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable <div dir=3D"ltr"><div class=3D"gmail_quote"><div dir=3D"ltr" class=3D"gmail= _attr">On Wed, Dec 22, 2021 at 4:05 PM Daniel Vetter <<a href=3D"mailto:= daniel@xxxxxxxx">daniel@xxxxxxxx</a>> wrote:<br></div><blockquote class= =3D"gmail_quote" style=3D"margin:0px 0px 0px 0.8ex;border-left:1px solid rg= b(204,204,204);padding-left:1ex">On Tue, Dec 07, 2021 at 01:34:07PM +0100, = Christian K=C3=B6nig wrote:<br> > Add an usage for kernel submissions. Waiting for those<br> > are mandatory for dynamic DMA-bufs.<br> > <br> > Signed-off-by: Christian K=C3=B6nig <<a href=3D"mailto:christian.ko= enig@xxxxxxx" target=3D"_blank">christian.koenig@xxxxxxx</a>><br> <br> Again just skipping to the doc bikeshedding, maybe with more cc others<br> help with some code review too.<br> <br> >=C2=A0 EXPORT_SYMBOL(ib_umem_dmabuf_map_pages);<br> > diff --git a/include/linux/dma-resv.h b/include/linux/dma-resv.h<br> > index 4f3a6abf43c4..29d799991496 100644<br> > --- a/include/linux/dma-resv.h<br> > +++ b/include/linux/dma-resv.h<br> > @@ -54,8 +54,30 @@ struct dma_resv_list;<br> >=C2=A0 =C2=A0*<br> >=C2=A0 =C2=A0* This enum describes the different use cases for a dma_re= sv object and<br> >=C2=A0 =C2=A0* controls which fences are returned when queried.<br> > + *<br> > + * An important fact is that there is the order KERNEL<WRITE<RE= AD and<br> > + * when the dma_resv object is asked for fences for one use case the = fences<br> > + * for the lower use case are returned as well.<br> > + *<br> > + * For example when asking for WRITE fences then the KERNEL fences ar= e returned<br> > + * as well. Similar when asked for READ fences then both WRITE and KE= RNEL<br> > + * fences are returned as well.<br> >=C2=A0 =C2=A0*/<br> >=C2=A0 enum dma_resv_usage {<br> > +=C2=A0 =C2=A0 =C2=A0/**<br> > +=C2=A0 =C2=A0 =C2=A0 * @DMA_RESV_USAGE_KERNEL: For in kernel memory m= anagement only.<br> > +=C2=A0 =C2=A0 =C2=A0 *<br> > +=C2=A0 =C2=A0 =C2=A0 * This should only be used for things like copyi= ng or clearing memory<br> > +=C2=A0 =C2=A0 =C2=A0 * with a DMA hardware engine for the purpose of = kernel memory<br> > +=C2=A0 =C2=A0 =C2=A0 * management.<br> > +=C2=A0 =C2=A0 =C2=A0 *<br> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0* Drivers *always* need to wait for= those fences before accessing the<br></blockquote><div><br></div><div>supe= r-nit: Your whitespace is wrong here.<br></div><div>=C2=A0</div><blockquote= class=3D"gmail_quote" style=3D"margin:0px 0px 0px 0.8ex;border-left:1px so= lid rgb(204,204,204);padding-left:1ex"> s/need to/must/ to stay with usual RFC wording. It's a hard requirement= or<br> there's a security bug somewhere.<br></blockquote><div><br></div><div>Y= eah, probably.=C2=A0 I like *must* but that's because that's what w= e use in the VK spec.=C2=A0 Do whatever's usual for kernel docs.</div><= div><br></div><div>Not sure where to put this comment but I feel like the w= ay things are framed is a bit the wrong way around.=C2=A0 Specifically, I d= on't think we should be talking about what fences you must wait on so m= uch as what fences you can safely skip.=C2=A0 In the previous model, the ex= clusive fence had to be waited on at all times and the shared fences could = be skipped unless you were doing something that would result in a new exclu= sive fence.=C2=A0 In this new world of "it's just a bucket of fenc= es", we need to be very sure the waiting is happening on the right thi= ngs.=C2=A0 It sounds (I could be wrong) like USAGE_KERNEL is the new exclus= ive fence.=C2=A0 If so, we need to make it virtually impossible to ignore.<= /div><div><br></div><div>Sorry if that's a bit of a ramble.=C2=A0 I thi= nk what I'm saying is this:=C2=A0 In whatever helpers or iterators we h= ave, be that get_singleton or iter_begin or whatever, we need to be sure we= specify things in terms of exclusion and not inclusion.=C2=A0 "Give m= e everything except implicit sync read fences" rather than "give = me implicit sync write fences".=C2=A0 If having a single, well-ordered= enum is sufficient for that, great.=C2=A0 If we think we'll ever end u= p with something other than a strict ordering, we may need to re-think a bi= t.</div><div><br></div><div>Concerning well-ordering... I'm a bit surpr= ised to only see three values here.=C2=A0 I expected 4:</div><div><br></div= ><div>=C2=A0- kernel exclusive, used for memory moves and the like</div><di= v>=C2=A0- kernel shared, used for "I'm using this right now, don&#= 39;t yank it out from under me" which may not have any implicit sync i= mplications whatsoever</div><div>=C2=A0- implicit sync write</div><div><div= >=C2=A0- implicit sync read</div></div><div><br></div><div>If we had those = four, I don't think the strict ordering works anymore.=C2=A0 From the P= OV of implicit sync, they would look at the implicit sync read/write fences= and maybe not even kernel exclusive.=C2=A0 From the POV of some doing a BO= move, they'd look at all of them.=C2=A0 From the POV of holding on to = memory while Vulkan is using it, you want to set a kernel shared fence but = it doesn't need to interact with implicit sync at all.=C2=A0 Am I missi= ng something obvious here?</div><div><br></div><div>--Jason</div><div><br><= /div><div>=C2=A0<br></div><blockquote class=3D"gmail_quote" style=3D"margin= :0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"= > > +=C2=A0 =C2=A0 =C2=A0 * resource protected by the dma_resv object. The= only exception for<br> > +=C2=A0 =C2=A0 =C2=A0 * that is when the resource is known to be locke= d down in place by<br> > +=C2=A0 =C2=A0 =C2=A0 * pinning it previously.<br> <br> Is this true? This sounds more confusing than helpful, because afaik in<br> general our pin interfaces do not block for any kernel fences. dma_buf_pin<= br> doesn't do that for sure. And I don't think ttm does that either.<b= r> <br> I think the only safe thing here is to state that it's safe if a) the<b= r> resource is pinned down and b) the callers has previously waited for the<br= > kernel fences.<br> <br> I also think we should put that wait for kernel fences into dma_buf_pin(),<= br> but that's maybe a later patch.<br> -Daniel<br> <br> <br> <br> > +=C2=A0 =C2=A0 =C2=A0 */<br> > +=C2=A0 =C2=A0 =C2=A0DMA_RESV_USAGE_KERNEL,<br> > +<br> >=C2=A0 =C2=A0 =C2=A0 =C2=A0/**<br> >=C2=A0 =C2=A0 =C2=A0 =C2=A0 * @DMA_RESV_USAGE_WRITE: Implicit write syn= chronization.<br> >=C2=A0 =C2=A0 =C2=A0 =C2=A0 *<br> > -- <br> > 2.25.1<br> > <br> <br> -- <br> Daniel Vetter<br> Software Engineer, Intel Corporation<br> <a href=3D"http://blog.ffwll.ch"; rel=3D"noreferrer" target=3D"_blank">http:= //blog.ffwll.ch</a><br> </blockquote></div></div> --000000000000ae9a2305d9403789--
