Re: [PATCH 1/2] media: Add timberdale video-in driver

[Mauro Carvalho Chehab <mchehab@redhat.com>]

Richard Röjfors wrote:
> On 04/25/2010 05:24 PM, Mauro Carvalho Chehab wrote:
>> Richard Röjfors wrote:
>>> This patch adds the timberdale video-in driver.
>>>
>>> The video IP of timberdale delivers the video data via DMA.
>>> The driver uses the DMA api to handle DMA transfers, and make use
>>> of the V4L2 videobuffers to handle buffers against user space.
>>> Due to some timing constraint it makes sense to do DMA into an
>>> intermediate buffer and then copy the data to vmalloc:ed buffers.
>>>
>>> If available the driver uses an encoder to get/set the video standard
>>>
>>> Signed-off-by: Richard Röjfors<richard.rojfors@pelagicore.com>
>>> +#define TIMBLOGIW_DMA_BUFFER_SIZE    (TIMBLOGIW_BYTES_PER_LINE * 576)
>>
>> ...
>>
>>> +static int __timblogiw_alloc_dma(struct timblogiw_fh *fh, struct
>>> device *dev)
>>> +{
>>> +    dma_addr_t addr;
>>> +    int err, i, pos;
>>> +    int bytes_per_desc = TIMBLOGIW_LINES_PER_DESC *
>>> +        timblogiw_bytes_per_line(fh->cur_norm);
>>> +
>>> +    fh->dma.cookie = -1;
>>> +    fh->dma.dev = dev;
>>> +
>>> +    fh->dma.buf = kzalloc(TIMBLOGIW_DMA_BUFFER_SIZE, GFP_KERNEL);
>>> +    if (!fh->dma.buf)
>>> +        return -ENOMEM;
>>
>>
>> Why do you need a fixed DMA buffer size? Just allocate the buffer size
>> dynamically at
>> buffer_prepare callback.
>>> +    videobuf_queue_vmalloc_init(&fh->vb_vidq,&timblogiw_video_qops,
>>> +            NULL,&fh->queue_lock, V4L2_BUF_TYPE_VIDEO_CAPTURE,
>>> +            V4L2_FIELD_NONE, sizeof(struct videobuf_buffer), fh);
>>
>> You should be using, instead, videobuf_dma_sg or videobuf_cont,
>> instead of
>> using videobuf-vmalloc. This way, you'll avoid double buffering.
> 
> 1. dma_sg can not be used, the DMA engine requires the memory blocks to
> be aligned on a factor of bytes per line, so 4K pages wouldn't work.
> 
> 2.
> I tried using videobuf-dma-contig, but got poor performance. I can not
> really explain why, I though it's due to the fact that the contiguous
> buffer is allocated coherent -> no caching.
> I saw both gstreamer and mplayer perform very badly.
> The frame grabber requires the DMA transfer for a frame beeing started
> while the frame is decoded. When I tested using contigous buffers
> gstreamer sometimes was that slow that it sometimes missed to have a
> frame queued when a transfer was finished, so I got frame drops. Any
> other ideas of the poor performance? otherwise I would like to go for
> the double buffered solution.

The better is to fix videobuf-dma_contig to better work on your hardware.
It makes sense to add a flag to allow specifying if it should use coherent
or non-coherent memory for the dma buffer alloc/free calls.

-- 

Cheers,
Mauro