Messages in this thread | | | Date | Thu, 21 Jul 2022 19:05:39 +0200 | From | Sebastian Fricke <> | Subject | Re: [PATCH 3/6] bitops: bitmap helper to set variable length values |
| |
Hey Yury and Andy,
thanks for you review and discussion, after some further communication with Nicolas Dufresne and Andrzej Pietrasiewicz, we came to the conclusion that I will prepare a more general bit-writer API, which fits better to the use-case we have on a lot of multimedia hardware.
I'll probably reuse the bitmap API as the backbone of that implementation and use the `*_set8` to write the content back to memory, but as we have a few more potential users for such an API, I'll strive towards creating a new kernel API instead of extending an existing API that isn't designed for our use-case.
Thanks a lot for your input and ideas.
Greetings, Sebastian
On 14.07.2022 14:24, Andy Shevchenko wrote: >On Wed, Jul 13, 2022 at 01:42:17PM -0700, Yury Norov wrote: >> On Wed, Jul 13, 2022 at 10:14:24PM +0200, Andy Shevchenko wrote: >> > On Wed, Jul 13, 2022 at 9:44 PM Yury Norov <yury.norov@gmail.com> wrote: >> > > On Wed, Jul 13, 2022 at 09:10:33PM +0200, Andy Shevchenko wrote: >> > > > On Wed, Jul 13, 2022 at 8:56 PM Yury Norov <yury.norov@gmail.com> wrote: >> > > > > On Wed, Jul 13, 2022 at 06:31:59PM +0200, Sebastian Fricke wrote: > >... > >> > > > > I'd suggest you to try implementing >> > > > > bitmap_copy_from(dst, src, dst_off, len) >> > > > > or even >> > > > > bitmap_copy_from(dst, dst_off, src, src_off, len) >> > > > > if you expect that you'll need more flexibility in the future. >> > > > >> > > > Do you think it would be useful? >> > > > >> > > > We have bitmap_replace() & bitmap_remap(). Wouldn't that be enough? >> > > >> > > bitmap_replace and bitmap_remap have no an 'offset' parameter. >> > >> > True. >> > >> > But then it's a bit too generic to have this src_off, no? >> >> That's why I said: >> >> > > > > if you expect that you'll need more flexibility in the future. >> >> My preferred option is bitmap_copy_from(dst, src, dst_off, len). >> >> > I would rather expect for asymmetrical bitmaps that the other side >> > will be either one of the fixed width types (it makes sense to have >> > for 32- or 64-bit arguments. >> >> Look at patch #6 - it copies 1,4,5,9,10,32,37... - pretty much a random >> number number of bits. > >It's too poor randomness, as u64 covers all what in patch 6. > >> > When you have a source bitmap of x bits and you would like to copy it >> > into a y-bit one, I would think that either you have a small amount of >> > bits in x anyway, or x is a full-sized bitmap (same order as y). >> >> It sounds like a speculation to me. Why shouldn't we let people to >> copy with an offset any number of bits? > >Because it's a common case. You have a value in the register / variable, which >naturally is one of the POD types. Now you want to inject this into bitmap at >the arbitrary offset. Value itself also needs to be variadic size in bits. > >Basically what he is trying to achieve is something like bitfield.h API over >bitmaps. Dunno, if actually bitfield.h in the certain driver wouldn't be >enough. > >> > Also >> > keep in mind that granularity is long, so less than long it makes no >> > sense. >> > >> > bitmap_copy_from_T(unsigned long *map, start, len, T src), >> > >> > where T is type, start is the offset in map, len is the amount of bits >> > from src starting from 0. That's what is required in most of the cases >> > I believe. >> >> But not in Sebastian's case, according to patch #6. > >I think it's a case, see above. > >-- >With Best Regards, >Andy Shevchenko > >
| |