Re: [PATCH 1/6] v4l: ti-vpe: Create a vpdma helper library
Hi,
On Tuesday 20 August 2013 07:26 PM, Laurent Pinchart wrote:
Hi Archit,
On Tuesday 20 August 2013 18:46:38 Archit Taneja wrote:
On Tuesday 20 August 2013 05:09 PM, Laurent Pinchart wrote:
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.
Sorry, I sent the previous mail bit too early.
With request_firmware() and the driver built-in, I see that the kernel
stalls for 10 seconds at the driver's probe, and the firware loading fails
since we didn't enter userspace where the file is.
The probing of devices asynchronously with kernel threads makes sense, so
it's possible that I'm doing something wrong here. I'll give it a try again
I might have spoken too fast. It looks like module initcalls are not run in
threads. I've most probably mistaken that with asynchronous probing of hot-
pluggable devices.
If your driver is built-in then it looks like the correct solution is to build
the firmware in the kernel image as well, or use the asynchronous API as you
did.
Okay, thanks for clarifying that.
We could use the request_firmware synchronous version if we call it in
the open v4l2 file op.
Maybe I could load the firmware when the device is opened the first
time(one instance).
I'll have to see whether it slows things down, and if I'd need to load
firmware more often. But I'd probably leave this experiment for later.
Archit
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/6] v4l: ti-vpe: Create a vpdma helper library
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 arc...@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)
Re: [PATCH 1/6] v4l: ti-vpe: Create a vpdma helper library
On Tuesday 20 August 2013 05:09 PM, Laurent Pinchart wrote:
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 arc...@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) ?
Actually, I just thought about this again. We use this api to allocate a
motion vector buffer for the de-interlacer, that's a buffer which is
proportional to the size of the frame, it takes up 4 bits per pixel. So
for a 1080i frame(our limit), it would take around 51 kilobytes for it.
I should probably use dma_alloc_coherent there.
+ 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
Re: [PATCH 1/6] v4l: ti-vpe: Create a vpdma helper library
Hi Laurent,
On Tuesday 20 August 2013 05:09 PM, Laurent Pinchart wrote:
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.
Sorry, I sent the previous mail bit too early.
With request_firmware() and the driver built-in, I see that the kernel
stalls for 10 seconds at the driver's probe, and the firware loading
fails since we didn't enter userspace where the file is.
The probing of devices asynchronously with kernel threads makes sense,
so it's possible that I'm doing something wrong here. I'll give it a try
again
Archit
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/6] v4l: ti-vpe: Create a vpdma helper library
Hi Archit,
On Tuesday 20 August 2013 18:46:38 Archit Taneja wrote:
On Tuesday 20 August 2013 05:09 PM, Laurent Pinchart wrote:
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.
Sorry, I sent the previous mail bit too early.
With request_firmware() and the driver built-in, I see that the kernel
stalls for 10 seconds at the driver's probe, and the firware loading fails
since we didn't enter userspace where the file is.
The probing of devices asynchronously with kernel threads makes sense, so
it's possible that I'm doing something wrong here. I'll give it a try again
I might have spoken too fast. It looks like module initcalls are not run in
threads. I've most probably mistaken that with asynchronous probing of hot-
pluggable devices.
If your driver is built-in then it looks like the correct solution is to build
the firmware in the kernel image as well, or use the asynchronous API as you
did.
--
Regards,
Laurent Pinchart
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/6] v4l: ti-vpe: Create a vpdma helper library
On Friday 09 August 2013 03:05 AM, Laurent Pinchart wrote:
Hi Archit,
On Monday 05 August 2013 16:56:46 Archit Taneja wrote:
On Monday 05 August 2013 01:43 PM, Tomi Valkeinen wrote:
On 02/08/13 17:03, Archit Taneja wrote:
+struct vpdma_data_format vpdma_yuv_fmts[] = {
+ [VPDMA_DATA_FMT_Y444] = {
+ .data_type = DATA_TYPE_Y444,
+ .depth = 8,
+ },
This, and all the other tables, should probably be consts?
That's true, I'll fix those.
+static void insert_field(u32 *valp, u32 field, u32 mask, int shift)
+{
+ u32 val = *valp;
+
+ val = ~(mask shift);
+ val |= (field mask) shift;
+ *valp = val;
+}
I think insert normally means, well, inserting a thing in between
something. What you do here is overwriting.
Why not just call it write_field?
sure, will change it.
+ * Allocate a DMA buffer
+ */
+int vpdma_buf_alloc(struct vpdma_buf *buf, size_t size)
+{
+ buf-size = size;
+ buf-mapped = 0;
Maybe true/false is clearer here that 0/1.
okay.
+/*
+ * submit a list of DMA descriptors to the VPE VPDMA, do not wait for
completion + */
+int vpdma_submit_descs(struct vpdma_data *vpdma, struct vpdma_desc_list
*list) +{
+ /* we always use the first list */
+ int list_num = 0;
+ int list_size;
+
+ if (vpdma_list_busy(vpdma, list_num))
+ return -EBUSY;
+
+ /* 16-byte granularity */
+ list_size = (list-next - list-buf.addr) 4;
+
+ write_reg(vpdma, VPDMA_LIST_ADDR, (u32) list-buf.dma_addr);
+ wmb();
What is the wmb() for?
VPDMA_LIST_ADDR needs to be written before VPDMA_LIST_ATTR, otherwise
VPDMA doesn't work. wmb() ensures the ordering.
write_reg() calls iowrite32(), which already includes an __iowmb().
I wasn't aware of that. I'll remove the wmb() call. Thanks for sharing
this info.
Archit
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/6] v4l: ti-vpe: Create a vpdma helper library
On Friday 09 August 2013 03:34 AM, Laurent Pinchart wrote:
Hi Archit,
Thank you for the patch.
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 arc...@ti.com
---
drivers/media/platform/ti-vpe/vpdma.c | 589 ++
drivers/media/platform/ti-vpe/vpdma.h | 154
drivers/media/platform/ti-vpe/vpdma_priv.h | 119 ++
3 files changed, 862 insertions(+)
create mode 100644 drivers/media/platform/ti-vpe/vpdma.c
create mode 100644 drivers/media/platform/ti-vpe/vpdma.h
create mode 100644 drivers/media/platform/ti-vpe/vpdma_priv.h
diff --git a/drivers/media/platform/ti-vpe/vpdma.c
b/drivers/media/platform/ti-vpe/vpdma.c new file mode 100644
index 000..b15b3dd
--- /dev/null
+++ b/drivers/media/platform/ti-vpe/vpdma.c
@@ -0,0 +1,589 @@
[snip]
+static int get_field(u32 value, u32 mask, int shift)
+{
+ return (value (mask shift)) shift;
+}
+
+static int get_field_reg(struct vpdma_data *vpdma, int offset,
+ u32 mask, int shift)
I would call this read_field_reg().
+{
+ return get_field(read_reg(vpdma, offset), mask, shift);
+}
+
+static void insert_field(u32 *valp, u32 field, u32 mask, int shift)
+{
+ u32 val = *valp;
+
+ val = ~(mask shift);
+ val |= (field mask) shift;
+ *valp = val;
+}
get_field() and insert_field() are used in a single location, you can manually
inline them.
Thanks, I'll make these modifications.
+static void insert_field_reg(struct vpdma_data *vpdma, int offset, u32
field,
+ u32 mask, int shift)
+{
+ u32 val = read_reg(vpdma, offset);
+
+ insert_field(val, field, mask, shift);
+
+ write_reg(vpdma, offset, val);
+}
[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
Re: [PATCH 1/6] v4l: ti-vpe: Create a vpdma helper library
Hi Archit,
On Monday 05 August 2013 16:56:46 Archit Taneja wrote:
On Monday 05 August 2013 01:43 PM, Tomi Valkeinen wrote:
On 02/08/13 17:03, Archit Taneja wrote:
+struct vpdma_data_format vpdma_yuv_fmts[] = {
+ [VPDMA_DATA_FMT_Y444] = {
+ .data_type = DATA_TYPE_Y444,
+ .depth = 8,
+ },
This, and all the other tables, should probably be consts?
That's true, I'll fix those.
+static void insert_field(u32 *valp, u32 field, u32 mask, int shift)
+{
+ u32 val = *valp;
+
+ val = ~(mask shift);
+ val |= (field mask) shift;
+ *valp = val;
+}
I think insert normally means, well, inserting a thing in between
something. What you do here is overwriting.
Why not just call it write_field?
sure, will change it.
+ * Allocate a DMA buffer
+ */
+int vpdma_buf_alloc(struct vpdma_buf *buf, size_t size)
+{
+ buf-size = size;
+ buf-mapped = 0;
Maybe true/false is clearer here that 0/1.
okay.
+/*
+ * submit a list of DMA descriptors to the VPE VPDMA, do not wait for
completion + */
+int vpdma_submit_descs(struct vpdma_data *vpdma, struct vpdma_desc_list
*list) +{
+ /* we always use the first list */
+ int list_num = 0;
+ int list_size;
+
+ if (vpdma_list_busy(vpdma, list_num))
+ return -EBUSY;
+
+ /* 16-byte granularity */
+ list_size = (list-next - list-buf.addr) 4;
+
+ write_reg(vpdma, VPDMA_LIST_ADDR, (u32) list-buf.dma_addr);
+ wmb();
What is the wmb() for?
VPDMA_LIST_ADDR needs to be written before VPDMA_LIST_ATTR, otherwise
VPDMA doesn't work. wmb() ensures the ordering.
write_reg() calls iowrite32(), which already includes an __iowmb().
+ write_reg(vpdma, VPDMA_LIST_ATTR,
+ (list_num VPDMA_LIST_NUM_SHFT) |
+ (list-type VPDMA_LIST_TYPE_SHFT) |
+ list_size);
+
+ return 0;
+}
+static void vpdma_firmware_cb(const struct firmware *f, void *context)
+{
+ struct vpdma_data *vpdma = context;
+ struct vpdma_buf fw_dma_buf;
+ int i, r;
+
+ dev_dbg(vpdma-pdev-dev, firmware callback\n);
+
+ if (!f || !f-data) {
+ dev_err(vpdma-pdev-dev, couldn't get firmware\n);
+ return;
+ }
+
+ /* already initialized */
+ if (get_field_reg(vpdma, VPDMA_LIST_ATTR, VPDMA_LIST_RDY_MASK,
+ VPDMA_LIST_RDY_SHFT)) {
+ vpdma-ready = true;
+ return;
+ }
+
+ r = vpdma_buf_alloc(fw_dma_buf, f-size);
+ if (r) {
+ dev_err(vpdma-pdev-dev,
+ failed to allocate dma buffer for firmware\n);
+ goto rel_fw;
+ }
+
+ memcpy(fw_dma_buf.addr, f-data, f-size);
+
+ vpdma_buf_map(vpdma, fw_dma_buf);
+
+ write_reg(vpdma, VPDMA_LIST_ADDR, (u32) fw_dma_buf.dma_addr);
+
+ for (i = 0; i 100; i++) { /* max 1 second */
+ msleep_interruptible(10);
You call interruptible version here, but you don't handle the
interrupted case. I believe the loop will just continue looping, even if
the user interrupted.
Okay. I think I don't understand the interruptible version correctly. We
don't need to msleep_interruptible here, we aren't waiting on any wake
up event, we just want to wait till a bit gets set.
I am thinking of implementing something similar to wait_for_bit_change()
in 'drivers/video/omap2/dss/dsi.c'
--
Regards,
Laurent Pinchart
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/6] v4l: ti-vpe: Create a vpdma helper library
Hi Archit,
Thank you for the patch.
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 arc...@ti.com
---
drivers/media/platform/ti-vpe/vpdma.c | 589 ++
drivers/media/platform/ti-vpe/vpdma.h | 154
drivers/media/platform/ti-vpe/vpdma_priv.h | 119 ++
3 files changed, 862 insertions(+)
create mode 100644 drivers/media/platform/ti-vpe/vpdma.c
create mode 100644 drivers/media/platform/ti-vpe/vpdma.h
create mode 100644 drivers/media/platform/ti-vpe/vpdma_priv.h
diff --git a/drivers/media/platform/ti-vpe/vpdma.c
b/drivers/media/platform/ti-vpe/vpdma.c new file mode 100644
index 000..b15b3dd
--- /dev/null
+++ b/drivers/media/platform/ti-vpe/vpdma.c
@@ -0,0 +1,589 @@
[snip]
+static int get_field(u32 value, u32 mask, int shift)
+{
+ return (value (mask shift)) shift;
+}
+
+static int get_field_reg(struct vpdma_data *vpdma, int offset,
+ u32 mask, int shift)
I would call this read_field_reg().
+{
+ return get_field(read_reg(vpdma, offset), mask, shift);
+}
+
+static void insert_field(u32 *valp, u32 field, u32 mask, int shift)
+{
+ u32 val = *valp;
+
+ val = ~(mask shift);
+ val |= (field mask) shift;
+ *valp = val;
+}
get_field() and insert_field() are used in a single location, you can manually
inline them.
+static void insert_field_reg(struct vpdma_data *vpdma, int offset, u32
field,
+ u32 mask, int shift)
+{
+ u32 val = read_reg(vpdma, offset);
+
+ insert_field(val, field, mask, shift);
+
+ write_reg(vpdma, offset, val);
+}
[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.
+ if (!buf-addr)
+ return -ENOMEM;
+
+ WARN_ON((u32) buf-addr VPDMA_DESC_ALIGN);
+
+ return 0;
+}
+
Re: [PATCH 1/6] v4l: ti-vpe: Create a vpdma helper library
Hi,
On 02/08/13 17:03, Archit Taneja wrote:
+struct vpdma_data_format vpdma_yuv_fmts[] = {
+ [VPDMA_DATA_FMT_Y444] = {
+ .data_type = DATA_TYPE_Y444,
+ .depth = 8,
+ },
This, and all the other tables, should probably be consts?
+static void insert_field(u32 *valp, u32 field, u32 mask, int shift)
+{
+ u32 val = *valp;
+
+ val = ~(mask shift);
+ val |= (field mask) shift;
+ *valp = val;
+}
I think insert normally means, well, inserting a thing in between
something. What you do here is overwriting.
Why not just call it write_field?
+ * Allocate a DMA buffer
+ */
+int vpdma_buf_alloc(struct vpdma_buf *buf, size_t size)
+{
+ buf-size = size;
+ buf-mapped = 0;
Maybe true/false is clearer here that 0/1.
+/*
+ * submit a list of DMA descriptors to the VPE VPDMA, do not wait for
completion
+ */
+int vpdma_submit_descs(struct vpdma_data *vpdma, struct vpdma_desc_list
*list)
+{
+ /* we always use the first list */
+ int list_num = 0;
+ int list_size;
+
+ if (vpdma_list_busy(vpdma, list_num))
+ return -EBUSY;
+
+ /* 16-byte granularity */
+ list_size = (list-next - list-buf.addr) 4;
+
+ write_reg(vpdma, VPDMA_LIST_ADDR, (u32) list-buf.dma_addr);
+ wmb();
What is the wmb() for?
+ write_reg(vpdma, VPDMA_LIST_ATTR,
+ (list_num VPDMA_LIST_NUM_SHFT) |
+ (list-type VPDMA_LIST_TYPE_SHFT) |
+ list_size);
+
+ return 0;
+}
+static void vpdma_firmware_cb(const struct firmware *f, void *context)
+{
+ struct vpdma_data *vpdma = context;
+ struct vpdma_buf fw_dma_buf;
+ int i, r;
+
+ dev_dbg(vpdma-pdev-dev, firmware callback\n);
+
+ if (!f || !f-data) {
+ dev_err(vpdma-pdev-dev, couldn't get firmware\n);
+ return;
+ }
+
+ /* already initialized */
+ if (get_field_reg(vpdma, VPDMA_LIST_ATTR, VPDMA_LIST_RDY_MASK,
+ VPDMA_LIST_RDY_SHFT)) {
+ vpdma-ready = true;
+ return;
+ }
+
+ r = vpdma_buf_alloc(fw_dma_buf, f-size);
+ if (r) {
+ dev_err(vpdma-pdev-dev,
+ failed to allocate dma buffer for firmware\n);
+ goto rel_fw;
+ }
+
+ memcpy(fw_dma_buf.addr, f-data, f-size);
+
+ vpdma_buf_map(vpdma, fw_dma_buf);
+
+ write_reg(vpdma, VPDMA_LIST_ADDR, (u32) fw_dma_buf.dma_addr);
+
+ for (i = 0; i 100; i++) { /* max 1 second */
+ msleep_interruptible(10);
You call interruptible version here, but you don't handle the
interrupted case. I believe the loop will just continue looping, even if
the user interrupted.
+ if (get_field_reg(vpdma, VPDMA_LIST_ATTR, VPDMA_LIST_RDY_MASK,
+ VPDMA_LIST_RDY_SHFT))
+ break;
+ }
+
+ if (i == 100) {
+ dev_err(vpdma-pdev-dev, firmware upload failed\n);
+ goto free_buf;
+ }
+
+ vpdma-ready = true;
+
+free_buf:
+ vpdma_buf_unmap(vpdma, fw_dma_buf);
+
+ vpdma_buf_free(fw_dma_buf);
+rel_fw:
+ release_firmware(f);
+}
Tomi
signature.asc
Description: OpenPGP digital signature
Re: [PATCH 1/6] v4l: ti-vpe: Create a vpdma helper library
On Monday 05 August 2013 01:43 PM, Tomi Valkeinen wrote:
Hi,
On 02/08/13 17:03, Archit Taneja wrote:
+struct vpdma_data_format vpdma_yuv_fmts[] = {
+ [VPDMA_DATA_FMT_Y444] = {
+ .data_type = DATA_TYPE_Y444,
+ .depth = 8,
+ },
This, and all the other tables, should probably be consts?
That's true, I'll fix those.
+static void insert_field(u32 *valp, u32 field, u32 mask, int shift)
+{
+ u32 val = *valp;
+
+ val = ~(mask shift);
+ val |= (field mask) shift;
+ *valp = val;
+}
I think insert normally means, well, inserting a thing in between
something. What you do here is overwriting.
Why not just call it write_field?
sure, will change it.
+ * Allocate a DMA buffer
+ */
+int vpdma_buf_alloc(struct vpdma_buf *buf, size_t size)
+{
+ buf-size = size;
+ buf-mapped = 0;
Maybe true/false is clearer here that 0/1.
okay.
+/*
+ * submit a list of DMA descriptors to the VPE VPDMA, do not wait for
completion
+ */
+int vpdma_submit_descs(struct vpdma_data *vpdma, struct vpdma_desc_list *list)
+{
+ /* we always use the first list */
+ int list_num = 0;
+ int list_size;
+
+ if (vpdma_list_busy(vpdma, list_num))
+ return -EBUSY;
+
+ /* 16-byte granularity */
+ list_size = (list-next - list-buf.addr) 4;
+
+ write_reg(vpdma, VPDMA_LIST_ADDR, (u32) list-buf.dma_addr);
+ wmb();
What is the wmb() for?
VPDMA_LIST_ADDR needs to be written before VPDMA_LIST_ATTR, otherwise
VPDMA doesn't work. wmb() ensures the ordering.
+ write_reg(vpdma, VPDMA_LIST_ATTR,
+ (list_num VPDMA_LIST_NUM_SHFT) |
+ (list-type VPDMA_LIST_TYPE_SHFT) |
+ list_size);
+
+ return 0;
+}
+static void vpdma_firmware_cb(const struct firmware *f, void *context)
+{
+ struct vpdma_data *vpdma = context;
+ struct vpdma_buf fw_dma_buf;
+ int i, r;
+
+ dev_dbg(vpdma-pdev-dev, firmware callback\n);
+
+ if (!f || !f-data) {
+ dev_err(vpdma-pdev-dev, couldn't get firmware\n);
+ return;
+ }
+
+ /* already initialized */
+ if (get_field_reg(vpdma, VPDMA_LIST_ATTR, VPDMA_LIST_RDY_MASK,
+ VPDMA_LIST_RDY_SHFT)) {
+ vpdma-ready = true;
+ return;
+ }
+
+ r = vpdma_buf_alloc(fw_dma_buf, f-size);
+ if (r) {
+ dev_err(vpdma-pdev-dev,
+ failed to allocate dma buffer for firmware\n);
+ goto rel_fw;
+ }
+
+ memcpy(fw_dma_buf.addr, f-data, f-size);
+
+ vpdma_buf_map(vpdma, fw_dma_buf);
+
+ write_reg(vpdma, VPDMA_LIST_ADDR, (u32) fw_dma_buf.dma_addr);
+
+ for (i = 0; i 100; i++) { /* max 1 second */
+ msleep_interruptible(10);
You call interruptible version here, but you don't handle the
interrupted case. I believe the loop will just continue looping, even if
the user interrupted.
Okay. I think I don't understand the interruptible version correctly. We
don't need to msleep_interruptible here, we aren't waiting on any wake
up event, we just want to wait till a bit gets set.
I am thinking of implementing something similar to wait_for_bit_change()
in 'drivers/video/omap2/dss/dsi.c'
Archit
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/6] v4l: ti-vpe: Create a vpdma helper library
On 05/08/13 14:26, Archit Taneja wrote:
On Monday 05 August 2013 01:43 PM, Tomi Valkeinen wrote:
+/*
+ * submit a list of DMA descriptors to the VPE VPDMA, do not wait
for completion
+ */
+int vpdma_submit_descs(struct vpdma_data *vpdma, struct
vpdma_desc_list *list)
+{
+/* we always use the first list */
+int list_num = 0;
+int list_size;
+
+if (vpdma_list_busy(vpdma, list_num))
+return -EBUSY;
+
+/* 16-byte granularity */
+list_size = (list-next - list-buf.addr) 4;
+
+write_reg(vpdma, VPDMA_LIST_ADDR, (u32) list-buf.dma_addr);
+wmb();
What is the wmb() for?
VPDMA_LIST_ADDR needs to be written before VPDMA_LIST_ATTR, otherwise
VPDMA doesn't work. wmb() ensures the ordering.
Are you sure it's needed? Here's an interesting thread about writing and
reading to registers: http://marc.info/?t=13058859492r=1w=2
+
+for (i = 0; i 100; i++) {/* max 1 second */
+msleep_interruptible(10);
You call interruptible version here, but you don't handle the
interrupted case. I believe the loop will just continue looping, even if
the user interrupted.
Okay. I think I don't understand the interruptible version correctly. We
don't need to msleep_interruptible here, we aren't waiting on any wake
up event, we just want to wait till a bit gets set.
Well, I think the interruptible versions should be used when the user
(wel, userspace program) initiates the action. The user should have the
option to interrupt a possibly long running operation, which is what
msleep_interruptible() makes possible.
Tomi
signature.asc
Description: OpenPGP digital signature
