On Wed, Jun 23, 2021 at 5:38 PM Thomas Hellström <thomas.hellstrom@xxxxxxxxxxxxxxx> wrote: > > Thanks for reviewing, Daniel. > > On 6/23/21 5:09 PM, Daniel Vetter wrote: > > > >> > >> + unsigned int mem_flags:2; > > Is the entire bitfield array all protected by dma_resv_lock? If not I'd > > just go with a full field, avoids headaches and all that. > > > > Also kerneldoc for this would be really sweet. Means some work to get it > > going, > > Yeah, late documentation review comments after v9 ought to be forbidden ;) Well I think we should have locking and all that documented from the start maybe :-P But yeah I know it's a bit late, so totally fine if that's done as a follow up on top. But for new stuff or revised stuff we need to start somewhere, and "maybe later when we have time" just never cuts it ... > > > but somewhere we need to stop hacking together undocumented ad-hoc > > locking schemes :-/ > > Hmm, this was intended to replace the change of and access of object ops > *without* the lock held and with proper asserts added in the accessors, > so it was not really intended to be an ad-hoc locking scheme, It's > simply placement related things are updated under the lock. Yeah this was more meant as a general comment. E.g. in struct i915_vma we now have the situation that we have 2 overlapping locking schemes, and it's almost impossible to figure out which is infect for which pieces. I'd like to avoid that if at all possible. > I'll update the code and resend. Thanks, Daniel -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch
