On Wed, Dec 22, 2021 at 4:05 PM Daniel Vetter <daniel@xxxxxxxx> wrote:
> On Tue, Dec 07, 2021 at 01:34:07PM +0100, Christian König wrote:
> > Add an usage for kernel submissions. Waiting for those
> > are mandatory for dynamic DMA-bufs.
> >
> > Signed-off-by: Christian König <christian.koenig@xxxxxxx>
>
> Again just skipping to the doc bikeshedding, maybe with more cc others
> help with some code review too.
>
> > EXPORT_SYMBOL(ib_umem_dmabuf_map_pages);
> > diff --git a/include/linux/dma-resv.h b/include/linux/dma-resv.h
> > index 4f3a6abf43c4..29d799991496 100644
> > --- a/include/linux/dma-resv.h
> > +++ b/include/linux/dma-resv.h
> > @@ -54,8 +54,30 @@ struct dma_resv_list;
> > *
> > * This enum describes the different use cases for a dma_resv object and
> > * controls which fences are returned when queried.
> > + *
> > + * An important fact is that there is the order KERNEL<WRITE<READ and
> > + * when the dma_resv object is asked for fences for one use case the
> fences
> > + * for the lower use case are returned as well.
> > + *
> > + * For example when asking for WRITE fences then the KERNEL fences are
> returned
> > + * as well. Similar when asked for READ fences then both WRITE and
> KERNEL
> > + * fences are returned as well.
> > */
> > enum dma_resv_usage {
> > + /**
> > + * @DMA_RESV_USAGE_KERNEL: For in kernel memory management only.
> > + *
> > + * This should only be used for things like copying or clearing
> memory
> > + * with a DMA hardware engine for the purpose of kernel memory
> > + * management.
> > + *
> > + * Drivers *always* need to wait for those fences before
> accessing the
>
super-nit: Your whitespace is wrong here.
> s/need to/must/ to stay with usual RFC wording. It's a hard requirement or
> there's a security bug somewhere.
>
Yeah, probably. I like *must* but that's because that's what we use in the
VK spec. Do whatever's usual for kernel docs.
Not sure where to put this comment but I feel like the way things are
framed is a bit the wrong way around. Specifically, I don't think we
should be talking about what fences you must wait on so much as what fences
you can safely skip. In the previous model, the exclusive 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 exclusive fence. In this
new world of "it's just a bucket of fences", we need to be very sure the
waiting is happening on the right things. It sounds (I could be wrong)
like USAGE_KERNEL is the new exclusive fence. If so, we need to make it
virtually impossible to ignore.
Sorry if that's a bit of a ramble. I think what I'm saying is this: In
whatever helpers or iterators we have, be that get_singleton or iter_begin
or whatever, we need to be sure we specify things in terms of exclusion and
not inclusion. "Give me everything except implicit sync read fences"
rather than "give me implicit sync write fences". If having a single,
well-ordered enum is sufficient for that, great. If we think we'll ever
end up with something other than a strict ordering, we may need to re-think
a bit.
Concerning well-ordering... I'm a bit surprised to only see three values
here. I expected 4:
- kernel exclusive, used for memory moves and the like
- kernel shared, used for "I'm using this right now, don't yank it out
from under me" which may not have any implicit sync implications whatsoever
- implicit sync write
- implicit sync read
If we had those four, I don't think the strict ordering works anymore.
