On Tue 5 March 2013 10:28:38 Marek Szyprowski wrote: > Hello, > > On 3/1/2013 7:44 PM, Hans Verkuil wrote: > > Hi all, > > > > This patch is based on an idea from Federico: > > > > http://www.mail-archive.com/davinci-linux-open-source@xxxxxxxxxxxxxxxxxxxx/msg24669.html > > > > While working on converting the solo6x10 driver to vb2 I realized that the > > same thing was needed for the dma-sg case: the solo6x10 has 32-bit PCI DMA, > > so you want to specify __GFP_DMA32 to prevent bounce buffers from being created. > > > > Rather than patching all drivers as the patch above does (error prone IMHO), > > I've decided to just add a gfp_flags field to vb2_queue and pass that to the > > alloc mem_op. The various alloc implementations will just OR it in. > > I agree that the gfp_flags is needed. It should be there from the > beginning, > but there is not DMA zone on our hardware and we missed that point. Our > fault. > However IMHO the better place for gfp_flags is the allocator context > structure > instead of vb2_queue. vb2_dma_contig_init_ctx() would need to be > extended and > similar function should be added for dma sg. Why is this better? It seems a huge amount of work for something that is useful for pretty much any allocator. Note that most PCI drivers are 32-bit only and need __GFP_DMA32. So this is not a rare case, it just that we haven't converted them yet. I don't mind doing the work, but I'd like to know the reasoning behind it. Regards, Hans > This reminds me that dma sg allocator needs to be seriously cleaned up > to match > the changes done in dma contig allocator from the beginning of its life. > I have > some work-in-progress patches, but I didn't manage to finish them due to > lack > of time... > > > This is urgently needed for any driver with DMA32 restrictions (which are most > > PCI drivers). > > > > Comments? > > > > Regards, > > > > Hans > > > > diff --git a/drivers/media/v4l2-core/videobuf2-core.c b/drivers/media/v4l2-core/videobuf2-core.c > > index db1235d..adde3e6 100644 > > --- a/drivers/media/v4l2-core/videobuf2-core.c > > +++ b/drivers/media/v4l2-core/videobuf2-core.c > > @@ -57,7 +57,7 @@ static int __vb2_buf_mem_alloc(struct vb2_buffer *vb) > > /* Allocate memory for all planes in this buffer */ > > for (plane = 0; plane < vb->num_planes; ++plane) { > > mem_priv = call_memop(q, alloc, q->alloc_ctx[plane], > > - q->plane_sizes[plane]); > > + q->plane_sizes[plane], q->gfp_flags); > > if (IS_ERR_OR_NULL(mem_priv)) > > goto free; > > > > diff --git a/drivers/media/v4l2-core/videobuf2-dma-contig.c b/drivers/media/v4l2-core/videobuf2-dma-contig.c > > index 10beaee..ae35d25 100644 > > --- a/drivers/media/v4l2-core/videobuf2-dma-contig.c > > +++ b/drivers/media/v4l2-core/videobuf2-dma-contig.c > > @@ -152,7 +152,7 @@ static void vb2_dc_put(void *buf_priv) > > kfree(buf); > > } > > > > -static void *vb2_dc_alloc(void *alloc_ctx, unsigned long size) > > +static void *vb2_dc_alloc(void *alloc_ctx, unsigned long size, gfp_t gfp_flags) > > { > > struct vb2_dc_conf *conf = alloc_ctx; > > struct device *dev = conf->dev; > > @@ -165,7 +165,8 @@ static void *vb2_dc_alloc(void *alloc_ctx, unsigned long size) > > /* align image size to PAGE_SIZE */ > > size = PAGE_ALIGN(size); > > > > - buf->vaddr = dma_alloc_coherent(dev, size, &buf->dma_addr, GFP_KERNEL); > > + buf->vaddr = dma_alloc_coherent(dev, size, &buf->dma_addr, > > + GFP_KERNEL | gfp_flags); > > if (!buf->vaddr) { > > dev_err(dev, "dma_alloc_coherent of size %ld failed\n", size); > > kfree(buf); > > diff --git a/drivers/media/v4l2-core/videobuf2-dma-sg.c b/drivers/media/v4l2-core/videobuf2-dma-sg.c > > index 25c3b36..952776f 100644 > > --- a/drivers/media/v4l2-core/videobuf2-dma-sg.c > > +++ b/drivers/media/v4l2-core/videobuf2-dma-sg.c > > @@ -33,7 +33,7 @@ struct vb2_dma_sg_buf { > > > > static void vb2_dma_sg_put(void *buf_priv); > > > > -static void *vb2_dma_sg_alloc(void *alloc_ctx, unsigned long size) > > +static void *vb2_dma_sg_alloc(void *alloc_ctx, unsigned long size, gfp_t gfp_flags) > > { > > struct vb2_dma_sg_buf *buf; > > int i; > > @@ -60,7 +60,8 @@ static void *vb2_dma_sg_alloc(void *alloc_ctx, unsigned long size) > > goto fail_pages_array_alloc; > > > > for (i = 0; i < buf->sg_desc.num_pages; ++i) { > > - buf->pages[i] = alloc_page(GFP_KERNEL | __GFP_ZERO | __GFP_NOWARN); > > + buf->pages[i] = alloc_page(GFP_KERNEL | __GFP_ZERO | > > + __GFP_NOWARN | gfp_flags); > > if (NULL == buf->pages[i]) > > goto fail_pages_alloc; > > sg_set_page(&buf->sg_desc.sglist[i], > > diff --git a/drivers/media/v4l2-core/videobuf2-vmalloc.c b/drivers/media/v4l2-core/videobuf2-vmalloc.c > > index a47fd4f..313d977 100644 > > --- a/drivers/media/v4l2-core/videobuf2-vmalloc.c > > +++ b/drivers/media/v4l2-core/videobuf2-vmalloc.c > > @@ -35,11 +35,11 @@ struct vb2_vmalloc_buf { > > > > static void vb2_vmalloc_put(void *buf_priv); > > > > -static void *vb2_vmalloc_alloc(void *alloc_ctx, unsigned long size) > > +static void *vb2_vmalloc_alloc(void *alloc_ctx, unsigned long size, gfp_t gfp_flags) > > { > > struct vb2_vmalloc_buf *buf; > > > > - buf = kzalloc(sizeof(*buf), GFP_KERNEL); > > + buf = kzalloc(sizeof(*buf), GFP_KERNEL | gfp_flags); > > if (!buf) > > return NULL; > > > > diff --git a/include/media/videobuf2-core.h b/include/media/videobuf2-core.h > > index 9cfd4ee..251d66b 100644 > > --- a/include/media/videobuf2-core.h > > +++ b/include/media/videobuf2-core.h > > @@ -27,7 +27,9 @@ struct vb2_fileio_data; > > * return NULL on failure or a pointer to allocator private, > > * per-buffer data on success; the returned private structure > > * will then be passed as buf_priv argument to other ops in this > > - * structure > > + * structure. Additional gfp_flags to use when allocating the > > + * are also passed to this operation. These flags are from the > > + * gfp_flags field of vb2_queue. > > * @put: inform the allocator that the buffer will no longer be used; > > * usually will result in the allocator freeing the buffer (if > > * no other users of this buffer are present); the buf_priv > > @@ -79,7 +81,7 @@ struct vb2_fileio_data; > > * unmap_dmabuf. > > */ > > struct vb2_mem_ops { > > - void *(*alloc)(void *alloc_ctx, unsigned long size); > > + void *(*alloc)(void *alloc_ctx, unsigned long size, gfp_t gfp_flags); > > void (*put)(void *buf_priv); > > struct dma_buf *(*get_dmabuf)(void *buf_priv); > > > > @@ -302,6 +304,9 @@ struct v4l2_fh; > > * @buf_struct_size: size of the driver-specific buffer structure; > > * "0" indicates the driver doesn't want to use a custom buffer > > * structure type, so sizeof(struct vb2_buffer) will is used > > + * @gfp_flags: additional gfp flags used when allocating the buffers. > > + * Typically this is 0, but it may be e.g. GFP_DMA or __GFP_DMA32 > > + * to force the buffer allocation to a specific memory zone. > > * > > * @memory: current memory type used > > * @bufs: videobuf buffer structures > > @@ -326,6 +331,7 @@ struct vb2_queue { > > const struct vb2_mem_ops *mem_ops; > > void *drv_priv; > > unsigned int buf_struct_size; > > + gfp_t gfp_flags; > > > > /* private: internal use only */ > > enum v4l2_memory memory; > > > > Best regards > -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
