On 04/24/2016 11:51 PM, Laurent Pinchart wrote:
> Hi Hans,
>
> On Saturday 23 Apr 2016 12:37:10 Hans Verkuil wrote:
>> On 04/23/2016 02:14 AM, Laurent Pinchart wrote:
>>> On Friday 22 Apr 2016 10:38:08 Hans Verkuil wrote:
>>>> From: Hans Verkuil <hans.verkuil@xxxxxxxxx>
>>>>
>>>> The allocation context is nothing more than a per-plane device pointer
>>>> to use when allocating buffers. So just provide a dev pointer in
>>>> vb2_queue for that purpose and drivers can skip
>>>> allocating/releasing/filling in the allocation context unless they
>>>> require different per-plane device pointers as used by some Samsung
>>>> SoCs.
>>>>
>>>> Signed-off-by: Hans Verkuil <hans.verkuil@xxxxxxxxx>
>>>> Cc: Laurent Pinchart <Laurent.pinchart@xxxxxxxxxxxxxxxx>
>>>> Cc: Sakari Ailus <sakari.ailus@xxxxxx>
>>>> Cc: Mauro Carvalho Chehab <mchehab@xxxxxxxxxxxxxxx>
>>>> Cc: Florian Echtler <floe@xxxxxxxxxxxxxx>
>>>> Cc: Federico Vaga <federico.vaga@xxxxxxxxx>
>>>> Cc: "Lad, Prabhakar" <prabhakar.csengg@xxxxxxxxx>
>>>> Cc: Scott Jiang <scott.jiang.linux@xxxxxxxxx>
>>>> Cc: Philipp Zabel <p.zabel@xxxxxxxxxxxxxx>
>>>> Cc: Fabien Dessenne <fabien.dessenne@xxxxxx>
>>>> Cc: Benoit Parrot <bparrot@xxxxxx>
>>>> Cc: Mikhail Ulyanov <mikhail.ulyanov@xxxxxxxxxxxxxxxxxx>
>>>> Cc: Guennadi Liakhovetski <g.liakhovetski@xxxxxx>
>>>> Cc: Javier Martin <javier.martin@xxxxxxxxxxxxxxxxx>
>>>> Cc: Jonathan Corbet <corbet@xxxxxxx>
>>>> Cc: Ludovic Desroches <ludovic.desroches@xxxxxxxxx>
>>>> Cc: Sergei Shtylyov <sergei.shtylyov@xxxxxxxxxxxxxxxxxx>
>>>> Cc: Kyungmin Park <kyungmin.park@xxxxxxxxxxx>
>>>> Cc: Sylwester Nawrocki <s.nawrocki@xxxxxxxxxxx>
>>>> ---
>>>>
>>>> drivers/media/v4l2-core/videobuf2-core.c | 16 +++++++++-------
>>>> include/media/videobuf2-core.h | 3 +++
>>>> 2 files changed, 12 insertions(+), 7 deletions(-)
>>>>
>>>> diff --git a/drivers/media/v4l2-core/videobuf2-core.c
>>>> b/drivers/media/v4l2-core/videobuf2-core.c index 5d016f4..88b5e48 100644
>>>> --- a/drivers/media/v4l2-core/videobuf2-core.c
>>>> +++ b/drivers/media/v4l2-core/videobuf2-core.c
>>>> @@ -206,8 +206,9 @@ static int __vb2_buf_mem_alloc(struct vb2_buffer *vb)
>>>>
>>>> for (plane = 0; plane < vb->num_planes; ++plane) {
>>>>
>>>> unsigned long size = PAGE_ALIGN(vb->planes[plane].length);
>>>>
>>>> - mem_priv = call_ptr_memop(vb, alloc, q->alloc_ctx[plane],
>>>> - size, dma_dir, q->gfp_flags);
>>>> + mem_priv = call_ptr_memop(vb, alloc,
>>>> + q->alloc_ctx[plane] ? : &q->dev,
>>>> + size, dma_dir, q->gfp_flags);
>>>
>>> While the videobuf2-dma-sg allocation context indeed only contains a
>>> pointer to the device, the videobuf2-dma-contig context also contains a
>>> dma_attrs. This patch will break the videobuf2-dma-contig alloc
>>> implementation.
>>
>> Good point. I fixed this in the last patch, but that would mean dma-contig
>> would be broken for the patches in between.
>>
>> I'm moving dma_attrs to struct vb2_queue as the first patch, then the rest
>> will work fine.
>
> Couldn't a driver require different dma attributes per plane ? Would it make
> sense to keep the allocation context structure, and use the struct device and
> dma attributes stored in the queue when no allocation context is provided ?
I kept the dma_attrs part simple for two reasons:
1) No driver in the kernel uses it.
2) I really can't think of any scenario where you get different DMA attrs per plane.
Perhaps if we make it possible to have a variable number of planes, but all
that is in the future and I rather take care of it when we actually know what
we need.
The 'allocation context' idea was simply a bad one: you're stuck with void pointers
(I hate those) and always having to check for ENOMEM when allocating them. When all
you need in almost all cases is just a device pointer.
Regards,
Hans
--
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
