Re: [PATCH 1/6] v4l: ti-vpe: Create a vpdma helper library

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

Hi Archit,

On Wednesday 14 August 2013 16:27:57 Archit Taneja wrote:
> On Friday 09 August 2013 03:34 AM, Laurent Pinchart wrote:
> > On Friday 02 August 2013 19:33:38 Archit Taneja wrote:
> >> The primary function of VPDMA is to move data between external memory and
> >> internal processing modules(in our case, VPE) that source or sink data.
> >> VPDMA is capable of buffering this data and then delivering the data as
> >> demanded to the modules as programmed. The modules that source or sink
> >> data are referred to as clients or ports. A channel is setup inside the
> >> VPDMA to connect a specific memory buffer to a specific client. The VPDMA
> >> centralizes the DMA control functions and buffering required to allow all
> >> the clients to minimize the effect of long latency times.
> >> 
> >> Add the following to the VPDMA helper:
> >> 
> >> - A data struct which describe VPDMA channels. For now, these channels
> >> are the ones used only by VPE, the list of channels will increase when
> >> VIP(Video Input Port) also uses the VPDMA library. This channel
> >> information will be used to populate fields required by data descriptors.
> >> 
> >> - Data structs which describe the different data types supported by
> >> VPDMA. This data type information will be used to populate fields
> >> required by data descriptors and used by the VPE driver to map a V4L2
> >> format to the corresponding VPDMA data type.
> >> 
> >> - Provide VPDMA register offset definitions, functions to read, write and
> >> modify VPDMA registers.
> >> 
> >> - Functions to create and submit a VPDMA list. A list is a group of
> >> descriptors that makes up a set of DMA transfers that need to be
> >> completed. Each descriptor will either perform a DMA transaction to fetch
> >> input buffers and write to output buffers(data descriptors), or configure
> >> the MMRs of sub blocks of VPE(configuration descriptors), or provide
> >> control information to VPDMA (control descriptors).
> >> 
> >> - Functions to allocate, map and unmap buffers needed for the descriptor
> >> list, payloads containing MMR values and motion vector buffers. These use
> >> the DMA mapping APIs to ensure exclusive access to VPDMA.
> >> 
> >> - Functions to enable VPDMA interrupts. VPDMA can trigger an interrupt on
> >> the VPE interrupt line when a descriptor list is parsed completely and
> >> the DMA transactions are completed. This requires masking the events in
> >> VPDMA registers and configuring some top level VPE interrupt registers.
> >> 
> >> - Enable some VPDMA specific parameters: frame start event(when to start
> >> DMA for a client) and line mode(whether each line fetched should be
> >> mirrored or not).
> >> 
> >> - Function to load firmware required by VPDMA. VPDMA requires a firmware
> >> for it's internal list manager. We add the required request_firmware
> >> apis to fetch this firmware from user space.
> >> 
> >> - Function to dump VPDMA registers.
> >> 
> >> - A function to initialize VPDMA, this will be called by the VPE driver
> >> with it's platform device pointer, this function will take care of
> >> loading VPDMA firmware and returning a handle back to the VPE driver.
> >> The VIP driver will also call the same init function to initialize it's
> >> own VPDMA instance.
> >> 
> >> Signed-off-by: Archit Taneja <archit@ti.com>

[snip]

> >> +/*
> >> + * Allocate a DMA buffer
> >> + */
> >> +int vpdma_buf_alloc(struct vpdma_buf *buf, size_t size)
> >> +{
> >> +	buf->size = size;
> >> +	buf->mapped = 0;
> >> +	buf->addr = kzalloc(size, GFP_KERNEL);
> > 
> > You should use the dma allocation API (depending on your needs,
> > dma_alloc_coherent for instance) to allocate DMA-able buffers.
> 
> I'm not sure about this, dma_map_single() api works fine on kzalloc'd
> buffers. The above function is used to allocate small contiguous buffers
> (never more than 1024 bytes) needed for descriptors for the DMA IP. I
> thought of using DMA pool, but that creates small buffers of the same size.
> So I finally went with kzalloc.

OK, I mistakenly thought it would allocate larger buffers as well. If it's 
used to allocate descriptors only, would it be better to rename it to 
vpdma_desc_alloc() (or something similar) ?

> >> +	if (!buf->addr)
> >> +		return -ENOMEM;
> >> +
> >> +	WARN_ON((u32) buf->addr & VPDMA_DESC_ALIGN);
> >> +
> >> +	return 0;
> >> +}

[snip]

> >> +static int vpdma_load_firmware(struct vpdma_data *vpdma)
> >> +{
> >> +	int r;
> >> +	struct device *dev = &vpdma->pdev->dev;
> >> +
> >> +	r = request_firmware_nowait(THIS_MODULE, 1,
> >> +		(const char *) VPDMA_FIRMWARE, dev, GFP_KERNEL, vpdma,
> >> +		vpdma_firmware_cb);
> > 
> > Is there a reason not to use the synchronous interface ? That would
> > simplify both this code and the callers, as they won't have to check
> > whether the firmware has been correctly loaded.
> 
> I'm not clear what you mean by that, the firmware would be stored in the
> filesystem. If the driver is built-in, then the synchronous interface
> wouldn't work unless the firmware is appended to the kernel image. Am I
> missing something here? I'm not very aware of the firmware api.

request_firmware() would just sleep (with a 30 seconds timeout if I'm not 
mistaken) until userspace provides the firmware. As devices are probed 
asynchronously (in kernel threads) the system will just boot normally, and the 
request_firmware() call will return when the firmware is available.

> >> +	if (r) {
> >> +		dev_err(dev, "firmware not available %s\n", VPDMA_FIRMWARE);
> >> +		return r;
> >> +	} else {
> >> +		dev_info(dev, "loading firmware %s\n", VPDMA_FIRMWARE);
> >> +	}
> >> +
> >> +	return 0;
> >> +}

-- 
Regards,

Laurent Pinchart