Hi Michael, On 15/11/2023 22:46, Michael Grzeschik wrote: > On Wed, Nov 01, 2023 at 12:43:25PM +0900, Tomasz Figa wrote: >> On Tue, May 16, 2023 at 7:50 PM Tomasz Figa <tfiga@xxxxxxxxxxxx> wrote: >>> >>> Hi Michael, >>> >>> On Wed, May 10, 2023 at 11:25 PM Michael Grzeschik <mgr@xxxxxxxxxxxxxx> wrote: >>> > >>> > Sorry for the late comeback, however here are some thoughts. >>> > >>> > On Fri, Dec 02, 2022 at 06:01:02PM +0900, Tomasz Figa wrote: >>> > >On Thu, Nov 24, 2022 at 10:35 PM Hans Verkuil <hverkuil@xxxxxxxxx> wrote: >>> > >> >>> > >> On 21/11/2022 00:44, Michael Grzeschik wrote: >>> > >> > The comments before the vm_map_ram function state that it should be used >>> > >> > for up to 256 KB only, and video buffers are definitely much larger. It >>> > >> > recommends using vmap in that case. >>> > >> > >>> > >> > Signed-off-by: Michael Grzeschik <m.grzeschik@xxxxxxxxxxxxxx> >>> > >> > --- >>> > >> > drivers/media/common/videobuf2/videobuf2-dma-sg.c | 7 ++++--- >>> > >> >>> > >> drivers/media/common/videobuf2/videobuf2-vmalloc.c uses it as well, >>> > >> probably also incorrectly. It makes sense to change that one as well. >>> > > >>> > >Comparing vm_map_ram() and vmap(..., VM_MAP, PAGE_KERNEL), for blocks >>> > >bigger than VMAP_MAX_ALLOC they're equivalent and for smaller blocks >>> > >the former should be faster, so I don't see what's wrong with the >>> > >current code. >>> > >>> > I got another comment on this from Andrzej Pietrasiewicz >>> > where he expands the comment on the use of vmap over vm_map_ram. >>> > >>> > https://lore.kernel.org/linux-media/64375ff4-dbbb-3d5b-eaf6-32d6780fd496@xxxxxxxxxxxxx >>> > >>> > As I understand this, we should probably update the vm_map_ram to vmap, >>> > due to the expectation that video buffers are long-living objects. >>> > >>> > Since there are some more places that would probably need to be updated >>> > if we should decide to use vmap over vm_map_ram in the whole >>> > videbuf2-* users, I would like to clarify on this before making >>> > a series. >>> >>> Ah, I see. Thanks for the pointer. >>> >>> VB2 buffers would usually require long-lived mappings, so based on >>> that, we should switch to vmap() indeed. >>> >>> As a side note, not directly related to this patch, I wonder if we >>> should also call invalidate/flush_kernel_vmap_range() in >>> vb2_dma_sg_prepare/finish(). (In principle we shouldn't, but so far >>> our drivers don't explicitly call begin/end_cpu_access() and rely on >>> prepare/finish to handle the cache maintenance of the kernel >>> mapping...) Let me also add Sergey on CC for visibility. >> >> Sorry, I forgot last time, so maybe it wasn't clear I'm good with this patch: >> >> Acked-by: Tomasz Figa <tfiga@xxxxxxxxxxxx> >> >> Hans, will you pick it? Thanks! > > Gentle Ping! > This patch is marked with "Changes Requested" in patchwork: https://patchwork.linuxtv.org/project/linux-media/patch/20221120234441.550908-1-m.grzeschik@xxxxxxxxxxxxxx/ Looking at the comments, there is a request to improve a comment and a request from me to make the same change to videobuf2-vmalloc.c. I have no problem with the change itself, it makes sense to use vmap. In any case, a v2 is needed. Regards, Hans
