* [PATCH v3 01/14] v4l: ti-vpe: Make sure in job_ready that we have the needed number of dst_bufs
2014-03-11 8:33 ` [PATCH v3 00/14] v4l: ti-vpe: Some VPE fixes and enhancements Archit Taneja
@ 2014-03-11 8:33 ` Archit Taneja
2014-03-13 14:36 ` Kamil Debski
2014-03-11 8:33 ` [PATCH v3 02/14] v4l: ti-vpe: register video device only when firmware is loaded Archit Taneja
` (13 subsequent siblings)
14 siblings, 1 reply; 88+ messages in thread
From: Archit Taneja @ 2014-03-11 8:33 UTC (permalink / raw)
To: k.debski, hverkuil; +Cc: linux-media, linux-omap, Archit Taneja
VPE has a ctrl parameter which decides how many mem to mem transactions the
active job from the job queue can perform.
The driver's job_ready() made sure that the number of ready source buffers are
sufficient for the job to execute successfully. But it didn't make sure if
there are sufficient ready destination buffers in the capture queue for the
VPE output.
If the time taken by VPE to process a single frame is really slow, then it's
possible that we don't need to imply such a restriction on the dst queue, but
really fast transactions(small resolution, no de-interlacing) may cause us to
hit the condition where we don't have any free buffers for the VPE to write on.
Add the extra check in job_ready() to make sure we have the sufficient amount
of destination buffers.
Signed-off-by: Archit Taneja <archit@ti.com>
---
drivers/media/platform/ti-vpe/vpe.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/drivers/media/platform/ti-vpe/vpe.c b/drivers/media/platform/ti-vpe/vpe.c
index 7a77a5b..f3143ac 100644
--- a/drivers/media/platform/ti-vpe/vpe.c
+++ b/drivers/media/platform/ti-vpe/vpe.c
@@ -887,6 +887,9 @@ static int job_ready(void *priv)
if (v4l2_m2m_num_src_bufs_ready(ctx->m2m_ctx) < needed)
return 0;
+ if (v4l2_m2m_num_dst_bufs_ready(ctx->m2m_ctx) < needed)
+ return 0;
+
return 1;
}
--
1.8.3.2
^ permalink raw reply related [flat|nested] 88+ messages in thread
* RE: [PATCH v3 01/14] v4l: ti-vpe: Make sure in job_ready that we have the needed number of dst_bufs
2014-03-11 8:33 ` [PATCH v3 01/14] v4l: ti-vpe: Make sure in job_ready that we have the needed number of dst_bufs Archit Taneja
@ 2014-03-13 14:36 ` Kamil Debski
0 siblings, 0 replies; 88+ messages in thread
From: Kamil Debski @ 2014-03-13 14:36 UTC (permalink / raw)
To: 'Archit Taneja', hverkuil; +Cc: linux-media, linux-omap
Hi,
> From: Archit Taneja [mailto:archit@ti.com]
> Sent: Tuesday, March 11, 2014 9:34 AM
>
> VPE has a ctrl parameter which decides how many mem to mem transactions
> the active job from the job queue can perform.
>
> The driver's job_ready() made sure that the number of ready source
> buffers are sufficient for the job to execute successfully. But it
> didn't make sure if there are sufficient ready destination buffers in
> the capture queue for the VPE output.
>
> If the time taken by VPE to process a single frame is really slow, then
> it's possible that we don't need to imply such a restriction on the dst
> queue, but really fast transactions(small resolution, no de-interlacing)
> may cause us to hit the condition where we don't have any free buffers
> for the VPE to write on.
>
> Add the extra check in job_ready() to make sure we have the sufficient
> amount of destination buffers.
>
> Signed-off-by: Archit Taneja <archit@ti.com>
Acked-by: Kamil Debski <k.debski@samsung.com>
Best wishes,
--
Kamil Debski
Samsung R&D Institute Poland
> ---
> drivers/media/platform/ti-vpe/vpe.c | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/drivers/media/platform/ti-vpe/vpe.c
> b/drivers/media/platform/ti-vpe/vpe.c
> index 7a77a5b..f3143ac 100644
> --- a/drivers/media/platform/ti-vpe/vpe.c
> +++ b/drivers/media/platform/ti-vpe/vpe.c
> @@ -887,6 +887,9 @@ static int job_ready(void *priv)
> if (v4l2_m2m_num_src_bufs_ready(ctx->m2m_ctx) < needed)
> return 0;
>
> + if (v4l2_m2m_num_dst_bufs_ready(ctx->m2m_ctx) < needed)
> + return 0;
> +
> return 1;
> }
>
> --
> 1.8.3.2
^ permalink raw reply [flat|nested] 88+ messages in thread
* [PATCH v3 02/14] v4l: ti-vpe: register video device only when firmware is loaded
2014-03-11 8:33 ` [PATCH v3 00/14] v4l: ti-vpe: Some VPE fixes and enhancements Archit Taneja
2014-03-11 8:33 ` [PATCH v3 01/14] v4l: ti-vpe: Make sure in job_ready that we have the needed number of dst_bufs Archit Taneja
@ 2014-03-11 8:33 ` Archit Taneja
2014-03-13 11:48 ` Kamil Debski
2014-03-11 8:33 ` [PATCH v3 03/14] v4l: ti-vpe: Use video_device_release_empty Archit Taneja
` (12 subsequent siblings)
14 siblings, 1 reply; 88+ messages in thread
From: Archit Taneja @ 2014-03-11 8:33 UTC (permalink / raw)
To: k.debski, hverkuil; +Cc: linux-media, linux-omap, Archit Taneja
vpe fops(vpe_open in particular) should be called only when VPDMA firmware
is loaded. File operations on the video device are possible the moment it is
registered.
Currently, we register the video device for VPE at driver probe, after calling
a vpdma helper to initialize VPDMA and load firmware. This function is
non-blocking(it calls request_firmware_nowait()), and doesn't ensure that the
firmware is actually loaded when it returns.
We remove the device registration from vpe probe, and move it to a callback
provided by the vpe driver to the vpdma library, through vpdma_create().
The ready field in vpdma_data is no longer needed since we always have firmware
loaded before the device is registered.
A minor problem with this approach is that if the video_register_device
fails(which doesn't really happen), the vpe platform device would be registered.
however, there won't be any v4l2 device corresponding to it.
Signed-off-by: Archit Taneja <archit@ti.com>
---
drivers/media/platform/ti-vpe/vpdma.c | 8 +++--
drivers/media/platform/ti-vpe/vpdma.h | 7 +++--
drivers/media/platform/ti-vpe/vpe.c | 55 ++++++++++++++++++++---------------
3 files changed, 41 insertions(+), 29 deletions(-)
diff --git a/drivers/media/platform/ti-vpe/vpdma.c b/drivers/media/platform/ti-vpe/vpdma.c
index e8175e7..73dd38e 100644
--- a/drivers/media/platform/ti-vpe/vpdma.c
+++ b/drivers/media/platform/ti-vpe/vpdma.c
@@ -781,7 +781,7 @@ static void vpdma_firmware_cb(const struct firmware *f, void *context)
/* already initialized */
if (read_field_reg(vpdma, VPDMA_LIST_ATTR, VPDMA_LIST_RDY_MASK,
VPDMA_LIST_RDY_SHFT)) {
- vpdma->ready = true;
+ vpdma->cb(vpdma->pdev);
return;
}
@@ -811,7 +811,7 @@ static void vpdma_firmware_cb(const struct firmware *f, void *context)
goto free_buf;
}
- vpdma->ready = true;
+ vpdma->cb(vpdma->pdev);
free_buf:
vpdma_unmap_desc_buf(vpdma, &fw_dma_buf);
@@ -839,7 +839,8 @@ static int vpdma_load_firmware(struct vpdma_data *vpdma)
return 0;
}
-struct vpdma_data *vpdma_create(struct platform_device *pdev)
+struct vpdma_data *vpdma_create(struct platform_device *pdev,
+ void (*cb)(struct platform_device *pdev))
{
struct resource *res;
struct vpdma_data *vpdma;
@@ -854,6 +855,7 @@ struct vpdma_data *vpdma_create(struct platform_device *pdev)
}
vpdma->pdev = pdev;
+ vpdma->cb = cb;
res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "vpdma");
if (res == NULL) {
diff --git a/drivers/media/platform/ti-vpe/vpdma.h b/drivers/media/platform/ti-vpe/vpdma.h
index cf40f11..bf5f8bb 100644
--- a/drivers/media/platform/ti-vpe/vpdma.h
+++ b/drivers/media/platform/ti-vpe/vpdma.h
@@ -35,8 +35,8 @@ struct vpdma_data {
struct platform_device *pdev;
- /* tells whether vpdma firmware is loaded or not */
- bool ready;
+ /* callback to VPE driver when the firmware is loaded */
+ void (*cb)(struct platform_device *pdev);
};
enum vpdma_data_format_type {
@@ -208,6 +208,7 @@ void vpdma_set_frame_start_event(struct vpdma_data *vpdma,
void vpdma_dump_regs(struct vpdma_data *vpdma);
/* initialize vpdma, passed with VPE's platform device pointer */
-struct vpdma_data *vpdma_create(struct platform_device *pdev);
+struct vpdma_data *vpdma_create(struct platform_device *pdev,
+ void (*cb)(struct platform_device *pdev));
#endif
diff --git a/drivers/media/platform/ti-vpe/vpe.c b/drivers/media/platform/ti-vpe/vpe.c
index f3143ac..f1eae67 100644
--- a/drivers/media/platform/ti-vpe/vpe.c
+++ b/drivers/media/platform/ti-vpe/vpe.c
@@ -1817,11 +1817,6 @@ static int vpe_open(struct file *file)
vpe_dbg(dev, "vpe_open\n");
- if (!dev->vpdma->ready) {
- vpe_err(dev, "vpdma firmware not loaded\n");
- return -ENODEV;
- }
-
ctx = kzalloc(sizeof(*ctx), GFP_KERNEL);
if (!ctx)
return -ENOMEM;
@@ -2039,10 +2034,40 @@ static void vpe_runtime_put(struct platform_device *pdev)
WARN_ON(r < 0 && r != -ENOSYS);
}
+static void vpe_fw_cb(struct platform_device *pdev)
+{
+ struct vpe_dev *dev = platform_get_drvdata(pdev);
+ struct video_device *vfd;
+ int ret;
+
+ vfd = &dev->vfd;
+ *vfd = vpe_videodev;
+ vfd->lock = &dev->dev_mutex;
+ vfd->v4l2_dev = &dev->v4l2_dev;
+
+ ret = video_register_device(vfd, VFL_TYPE_GRABBER, 0);
+ if (ret) {
+ vpe_err(dev, "Failed to register video device\n");
+
+ vpe_set_clock_enable(dev, 0);
+ vpe_runtime_put(pdev);
+ pm_runtime_disable(&pdev->dev);
+ v4l2_m2m_release(dev->m2m_dev);
+ vb2_dma_contig_cleanup_ctx(dev->alloc_ctx);
+ v4l2_device_unregister(&dev->v4l2_dev);
+
+ return;
+ }
+
+ video_set_drvdata(vfd, dev);
+ snprintf(vfd->name, sizeof(vfd->name), "%s", vpe_videodev.name);
+ dev_info(dev->v4l2_dev.dev, "Device registered as /dev/video%d\n",
+ vfd->num);
+}
+
static int vpe_probe(struct platform_device *pdev)
{
struct vpe_dev *dev;
- struct video_device *vfd;
int ret, irq, func;
dev = devm_kzalloc(&pdev->dev, sizeof(*dev), GFP_KERNEL);
@@ -2123,28 +2148,12 @@ static int vpe_probe(struct platform_device *pdev)
goto runtime_put;
}
- dev->vpdma = vpdma_create(pdev);
+ dev->vpdma = vpdma_create(pdev, vpe_fw_cb);
if (IS_ERR(dev->vpdma)) {
ret = PTR_ERR(dev->vpdma);
goto runtime_put;
}
- vfd = &dev->vfd;
- *vfd = vpe_videodev;
- vfd->lock = &dev->dev_mutex;
- vfd->v4l2_dev = &dev->v4l2_dev;
-
- ret = video_register_device(vfd, VFL_TYPE_GRABBER, 0);
- if (ret) {
- vpe_err(dev, "Failed to register video device\n");
- goto runtime_put;
- }
-
- video_set_drvdata(vfd, dev);
- snprintf(vfd->name, sizeof(vfd->name), "%s", vpe_videodev.name);
- dev_info(dev->v4l2_dev.dev, "Device registered as /dev/video%d\n",
- vfd->num);
-
return 0;
runtime_put:
--
1.8.3.2
^ permalink raw reply related [flat|nested] 88+ messages in thread
* RE: [PATCH v3 02/14] v4l: ti-vpe: register video device only when firmware is loaded
2014-03-11 8:33 ` [PATCH v3 02/14] v4l: ti-vpe: register video device only when firmware is loaded Archit Taneja
@ 2014-03-13 11:48 ` Kamil Debski
2014-03-13 12:09 ` Archit Taneja
0 siblings, 1 reply; 88+ messages in thread
From: Kamil Debski @ 2014-03-13 11:48 UTC (permalink / raw)
To: 'Archit Taneja', hverkuil; +Cc: linux-media, linux-omap
Hi Archit,
> From: Archit Taneja [mailto:archit@ti.com]
> Sent: Tuesday, March 11, 2014 9:34 AM
>
> vpe fops(vpe_open in particular) should be called only when VPDMA
> firmware is loaded. File operations on the video device are possible
> the moment it is registered.
>
> Currently, we register the video device for VPE at driver probe, after
> calling a vpdma helper to initialize VPDMA and load firmware. This
> function is non-blocking(it calls request_firmware_nowait()), and
> doesn't ensure that the firmware is actually loaded when it returns.
>
> We remove the device registration from vpe probe, and move it to a
> callback provided by the vpe driver to the vpdma library, through
> vpdma_create().
>
> The ready field in vpdma_data is no longer needed since we always have
> firmware loaded before the device is registered.
>
> A minor problem with this approach is that if the video_register_device
> fails(which doesn't really happen), the vpe platform device would be
> registered.
> however, there won't be any v4l2 device corresponding to it.
Could you explain to me one thing. request_firmware cannot be used in
probe, thus you are using request_firmware_nowait. Why cannot the firmware
be
loaded on open with a regular request_firmware that is waiting?
This patch seems to swap one problem for another. The possibility that open
fails (because firmware is not yet loaded) is swapped for a vague
possibility
that video_register_device.
Best wishes,
--
Kamil Debski
Samsung R&D Institute Poland
>
> Signed-off-by: Archit Taneja <archit@ti.com>
> ---
> drivers/media/platform/ti-vpe/vpdma.c | 8 +++--
> drivers/media/platform/ti-vpe/vpdma.h | 7 +++--
> drivers/media/platform/ti-vpe/vpe.c | 55 ++++++++++++++++++++-------
> --------
> 3 files changed, 41 insertions(+), 29 deletions(-)
>
> diff --git a/drivers/media/platform/ti-vpe/vpdma.c
> b/drivers/media/platform/ti-vpe/vpdma.c
> index e8175e7..73dd38e 100644
> --- a/drivers/media/platform/ti-vpe/vpdma.c
> +++ b/drivers/media/platform/ti-vpe/vpdma.c
> @@ -781,7 +781,7 @@ static void vpdma_firmware_cb(const struct firmware
> *f, void *context)
> /* already initialized */
> if (read_field_reg(vpdma, VPDMA_LIST_ATTR, VPDMA_LIST_RDY_MASK,
> VPDMA_LIST_RDY_SHFT)) {
> - vpdma->ready = true;
> + vpdma->cb(vpdma->pdev);
> return;
> }
>
> @@ -811,7 +811,7 @@ static void vpdma_firmware_cb(const struct firmware
> *f, void *context)
> goto free_buf;
> }
>
> - vpdma->ready = true;
> + vpdma->cb(vpdma->pdev);
>
> free_buf:
> vpdma_unmap_desc_buf(vpdma, &fw_dma_buf); @@ -839,7 +839,8 @@
> static int vpdma_load_firmware(struct vpdma_data *vpdma)
> return 0;
> }
>
> -struct vpdma_data *vpdma_create(struct platform_device *pdev)
> +struct vpdma_data *vpdma_create(struct platform_device *pdev,
> + void (*cb)(struct platform_device *pdev))
> {
> struct resource *res;
> struct vpdma_data *vpdma;
> @@ -854,6 +855,7 @@ struct vpdma_data *vpdma_create(struct
> platform_device *pdev)
> }
>
> vpdma->pdev = pdev;
> + vpdma->cb = cb;
>
> res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "vpdma");
> if (res == NULL) {
> diff --git a/drivers/media/platform/ti-vpe/vpdma.h
> b/drivers/media/platform/ti-vpe/vpdma.h
> index cf40f11..bf5f8bb 100644
> --- a/drivers/media/platform/ti-vpe/vpdma.h
> +++ b/drivers/media/platform/ti-vpe/vpdma.h
> @@ -35,8 +35,8 @@ struct vpdma_data {
>
> struct platform_device *pdev;
>
> - /* tells whether vpdma firmware is loaded or not */
> - bool ready;
> + /* callback to VPE driver when the firmware is loaded */
> + void (*cb)(struct platform_device *pdev);
> };
>
> enum vpdma_data_format_type {
> @@ -208,6 +208,7 @@ void vpdma_set_frame_start_event(struct vpdma_data
> *vpdma, void vpdma_dump_regs(struct vpdma_data *vpdma);
>
> /* initialize vpdma, passed with VPE's platform device pointer */ -
> struct vpdma_data *vpdma_create(struct platform_device *pdev);
> +struct vpdma_data *vpdma_create(struct platform_device *pdev,
> + void (*cb)(struct platform_device *pdev));
>
> #endif
> diff --git a/drivers/media/platform/ti-vpe/vpe.c
> b/drivers/media/platform/ti-vpe/vpe.c
> index f3143ac..f1eae67 100644
> --- a/drivers/media/platform/ti-vpe/vpe.c
> +++ b/drivers/media/platform/ti-vpe/vpe.c
> @@ -1817,11 +1817,6 @@ static int vpe_open(struct file *file)
>
> vpe_dbg(dev, "vpe_open\n");
>
> - if (!dev->vpdma->ready) {
> - vpe_err(dev, "vpdma firmware not loaded\n");
> - return -ENODEV;
> - }
> -
> ctx = kzalloc(sizeof(*ctx), GFP_KERNEL);
> if (!ctx)
> return -ENOMEM;
> @@ -2039,10 +2034,40 @@ static void vpe_runtime_put(struct
> platform_device *pdev)
> WARN_ON(r < 0 && r != -ENOSYS);
> }
>
> +static void vpe_fw_cb(struct platform_device *pdev) {
> + struct vpe_dev *dev = platform_get_drvdata(pdev);
> + struct video_device *vfd;
> + int ret;
> +
> + vfd = &dev->vfd;
> + *vfd = vpe_videodev;
> + vfd->lock = &dev->dev_mutex;
> + vfd->v4l2_dev = &dev->v4l2_dev;
> +
> + ret = video_register_device(vfd, VFL_TYPE_GRABBER, 0);
> + if (ret) {
> + vpe_err(dev, "Failed to register video device\n");
> +
> + vpe_set_clock_enable(dev, 0);
> + vpe_runtime_put(pdev);
> + pm_runtime_disable(&pdev->dev);
> + v4l2_m2m_release(dev->m2m_dev);
> + vb2_dma_contig_cleanup_ctx(dev->alloc_ctx);
> + v4l2_device_unregister(&dev->v4l2_dev);
> +
> + return;
> + }
> +
> + video_set_drvdata(vfd, dev);
> + snprintf(vfd->name, sizeof(vfd->name), "%s", vpe_videodev.name);
> + dev_info(dev->v4l2_dev.dev, "Device registered as /dev/video%d\n",
> + vfd->num);
> +}
> +
> static int vpe_probe(struct platform_device *pdev) {
> struct vpe_dev *dev;
> - struct video_device *vfd;
> int ret, irq, func;
>
> dev = devm_kzalloc(&pdev->dev, sizeof(*dev), GFP_KERNEL); @@ -
> 2123,28 +2148,12 @@ static int vpe_probe(struct platform_device *pdev)
> goto runtime_put;
> }
>
> - dev->vpdma = vpdma_create(pdev);
> + dev->vpdma = vpdma_create(pdev, vpe_fw_cb);
> if (IS_ERR(dev->vpdma)) {
> ret = PTR_ERR(dev->vpdma);
> goto runtime_put;
> }
>
> - vfd = &dev->vfd;
> - *vfd = vpe_videodev;
> - vfd->lock = &dev->dev_mutex;
> - vfd->v4l2_dev = &dev->v4l2_dev;
> -
> - ret = video_register_device(vfd, VFL_TYPE_GRABBER, 0);
> - if (ret) {
> - vpe_err(dev, "Failed to register video device\n");
> - goto runtime_put;
> - }
> -
> - video_set_drvdata(vfd, dev);
> - snprintf(vfd->name, sizeof(vfd->name), "%s", vpe_videodev.name);
> - dev_info(dev->v4l2_dev.dev, "Device registered as /dev/video%d\n",
> - vfd->num);
> -
> return 0;
>
> runtime_put:
> --
> 1.8.3.2
^ permalink raw reply [flat|nested] 88+ messages in thread
* Re: [PATCH v3 02/14] v4l: ti-vpe: register video device only when firmware is loaded
2014-03-13 11:48 ` Kamil Debski
@ 2014-03-13 12:09 ` Archit Taneja
2014-03-13 14:29 ` Kamil Debski
0 siblings, 1 reply; 88+ messages in thread
From: Archit Taneja @ 2014-03-13 12:09 UTC (permalink / raw)
To: Kamil Debski, hverkuil; +Cc: linux-media, linux-omap
Hi Kamil,
On Thursday 13 March 2014 05:18 PM, Kamil Debski wrote:
> Hi Archit,
>
>> From: Archit Taneja [mailto:archit@ti.com]
>> Sent: Tuesday, March 11, 2014 9:34 AM
>>
>> vpe fops(vpe_open in particular) should be called only when VPDMA
>> firmware is loaded. File operations on the video device are possible
>> the moment it is registered.
>>
>> Currently, we register the video device for VPE at driver probe, after
>> calling a vpdma helper to initialize VPDMA and load firmware. This
>> function is non-blocking(it calls request_firmware_nowait()), and
>> doesn't ensure that the firmware is actually loaded when it returns.
>>
>> We remove the device registration from vpe probe, and move it to a
>> callback provided by the vpe driver to the vpdma library, through
>> vpdma_create().
>>
>> The ready field in vpdma_data is no longer needed since we always have
>> firmware loaded before the device is registered.
>>
>> A minor problem with this approach is that if the video_register_device
>> fails(which doesn't really happen), the vpe platform device would be
>> registered.
>> however, there won't be any v4l2 device corresponding to it.
>
> Could you explain to me one thing. request_firmware cannot be used in
> probe, thus you are using request_firmware_nowait. Why cannot the firmware
> be
> loaded on open with a regular request_firmware that is waiting?
I totally agree with you here. Placing the firmware in open() would
probably make more sense.
The reason I didn't place it in open() is because I didn't want to
release firmware in a corresponding close(), and be in a situation where
the firmware is loaded multiple times in the driver's lifetime.
There are some reasons for doing this. First, it will require more
testing with respect to whether the firmware is loaded correctly
successive times :), the second is that loading firmware might probably
take a bit of time, and we don't want to make applications too slow(I
haven't measured the time taken, so I don't have a strong case for this
either)
>
> This patch seems to swap one problem for another. The possibility that open
> fails (because firmware is not yet loaded) is swapped for a vague
> possibility
> that video_register_device.
The driver will work fine in most cases even without this patch(apart
from the possibility mentioned as above).
We could discard this patch from this series, and I can work on a patch
which moves firmware loading to the vpe_open() call, and hence solving
the issue in the right manner. Does that sound fine?
Thanks,
Archit
^ permalink raw reply [flat|nested] 88+ messages in thread
* RE: [PATCH v3 02/14] v4l: ti-vpe: register video device only when firmware is loaded
2014-03-13 12:09 ` Archit Taneja
@ 2014-03-13 14:29 ` Kamil Debski
2014-03-14 9:51 ` Archit Taneja
0 siblings, 1 reply; 88+ messages in thread
From: Kamil Debski @ 2014-03-13 14:29 UTC (permalink / raw)
To: 'Archit Taneja', hverkuil; +Cc: linux-media, linux-omap
Hi,
> From: Archit Taneja [mailto:archit@ti.com]
> Sent: Thursday, March 13, 2014 1:09 PM
>
> Hi Kamil,
>
> On Thursday 13 March 2014 05:18 PM, Kamil Debski wrote:
> > Hi Archit,
> >
> >> From: Archit Taneja [mailto:archit@ti.com]
> >> Sent: Tuesday, March 11, 2014 9:34 AM
> >>
> >> vpe fops(vpe_open in particular) should be called only when VPDMA
> >> firmware is loaded. File operations on the video device are possible
> >> the moment it is registered.
> >>
> >> Currently, we register the video device for VPE at driver probe,
> >> after calling a vpdma helper to initialize VPDMA and load firmware.
> >> This function is non-blocking(it calls request_firmware_nowait()),
> >> and doesn't ensure that the firmware is actually loaded when it
> returns.
> >>
> >> We remove the device registration from vpe probe, and move it to a
> >> callback provided by the vpe driver to the vpdma library, through
> >> vpdma_create().
> >>
> >> The ready field in vpdma_data is no longer needed since we always
> >> have firmware loaded before the device is registered.
> >>
> >> A minor problem with this approach is that if the
> >> video_register_device fails(which doesn't really happen), the vpe
> >> platform device would be registered.
> >> however, there won't be any v4l2 device corresponding to it.
> >
> > Could you explain to me one thing. request_firmware cannot be used in
> > probe, thus you are using request_firmware_nowait. Why cannot the
> > firmware be loaded on open with a regular request_firmware that is
> > waiting?
>
> I totally agree with you here. Placing the firmware in open() would
> probably make more sense.
>
> The reason I didn't place it in open() is because I didn't want to
> release firmware in a corresponding close(), and be in a situation
> where the firmware is loaded multiple times in the driver's lifetime.
Would it be possible to load firmware in open and release it in remove?
I know that this would disturb the symmetry between open-release and
probe-remove. But this could work.
> There are some reasons for doing this. First, it will require more
> testing with respect to whether the firmware is loaded correctly
> successive times :), the second is that loading firmware might probably
> take a bit of time, and we don't want to make applications too slow(I
> haven't measured the time taken, so I don't have a strong case for this
> either)
>
> >
> > This patch seems to swap one problem for another. The possibility
> that
> > open fails (because firmware is not yet loaded) is swapped for a
> vague
> > possibility that video_register_device.
>
> The driver will work fine in most cases even without this patch(apart
> from the possibility mentioned as above).
>
> We could discard this patch from this series, and I can work on a patch
> which moves firmware loading to the vpe_open() call, and hence solving
> the issue in the right manner. Does that sound fine?
I agree, this should be a good solution.
>
> Thanks,
> Archit
Best wishes,
--
Kamil Debski
Samsung R&D Institute Poland
^ permalink raw reply [flat|nested] 88+ messages in thread
* Re: [PATCH v3 02/14] v4l: ti-vpe: register video device only when firmware is loaded
2014-03-13 14:29 ` Kamil Debski
@ 2014-03-14 9:51 ` Archit Taneja
0 siblings, 0 replies; 88+ messages in thread
From: Archit Taneja @ 2014-03-14 9:51 UTC (permalink / raw)
To: Kamil Debski, hverkuil; +Cc: linux-media, linux-omap
Hi,
On Thursday 13 March 2014 07:59 PM, Kamil Debski wrote:
> Hi,
>
>> From: Archit Taneja [mailto:archit@ti.com]
>> Sent: Thursday, March 13, 2014 1:09 PM
>>
>> Hi Kamil,
>>
>> On Thursday 13 March 2014 05:18 PM, Kamil Debski wrote:
>>> Hi Archit,
>>>
>>>> From: Archit Taneja [mailto:archit@ti.com]
>>>> Sent: Tuesday, March 11, 2014 9:34 AM
>>>>
>>>> vpe fops(vpe_open in particular) should be called only when VPDMA
>>>> firmware is loaded. File operations on the video device are possible
>>>> the moment it is registered.
>>>>
>>>> Currently, we register the video device for VPE at driver probe,
>>>> after calling a vpdma helper to initialize VPDMA and load firmware.
>>>> This function is non-blocking(it calls request_firmware_nowait()),
>>>> and doesn't ensure that the firmware is actually loaded when it
>> returns.
>>>>
>>>> We remove the device registration from vpe probe, and move it to a
>>>> callback provided by the vpe driver to the vpdma library, through
>>>> vpdma_create().
>>>>
>>>> The ready field in vpdma_data is no longer needed since we always
>>>> have firmware loaded before the device is registered.
>>>>
>>>> A minor problem with this approach is that if the
>>>> video_register_device fails(which doesn't really happen), the vpe
>>>> platform device would be registered.
>>>> however, there won't be any v4l2 device corresponding to it.
>>>
>>> Could you explain to me one thing. request_firmware cannot be used in
>>> probe, thus you are using request_firmware_nowait. Why cannot the
>>> firmware be loaded on open with a regular request_firmware that is
>>> waiting?
>>
>> I totally agree with you here. Placing the firmware in open() would
>> probably make more sense.
>>
>> The reason I didn't place it in open() is because I didn't want to
>> release firmware in a corresponding close(), and be in a situation
>> where the firmware is loaded multiple times in the driver's lifetime.
>
> Would it be possible to load firmware in open and release it in remove?
> I know that this would disturb the symmetry between open-release and
> probe-remove. But this could work.
That might work.
Currently, the driver doesn't do any clock management, it just enables
the clocks in probe, and disables them in remove. I need to check how
the firmware is dependent on clocks. We could make a better decision
about where to release the firmware with that information.
Archit
^ permalink raw reply [flat|nested] 88+ messages in thread
* [PATCH v3 03/14] v4l: ti-vpe: Use video_device_release_empty
2014-03-11 8:33 ` [PATCH v3 00/14] v4l: ti-vpe: Some VPE fixes and enhancements Archit Taneja
2014-03-11 8:33 ` [PATCH v3 01/14] v4l: ti-vpe: Make sure in job_ready that we have the needed number of dst_bufs Archit Taneja
2014-03-11 8:33 ` [PATCH v3 02/14] v4l: ti-vpe: register video device only when firmware is loaded Archit Taneja
@ 2014-03-11 8:33 ` Archit Taneja
2014-03-11 12:02 ` Hans Verkuil
2014-03-11 8:33 ` [PATCH v3 04/14] v4l: ti-vpe: Allow DMABUF buffer type support Archit Taneja
` (11 subsequent siblings)
14 siblings, 1 reply; 88+ messages in thread
From: Archit Taneja @ 2014-03-11 8:33 UTC (permalink / raw)
To: k.debski, hverkuil; +Cc: linux-media, linux-omap, Archit Taneja
The video_device struct is currently embedded in the driver data struct vpe_dev.
A vpe_dev instance is allocated by the driver, and the memory for the vfd is a
part of this struct.
The v4l2 core, however, manages the removal of the vfd region, through the
video_device's .release() op, which currently is the helper
video_device_release. This causes memory corruption, and leads to issues when
we try to re-insert the vpe module.
Use the video_device_release_empty helper function instead
Signed-off-by: Archit Taneja <archit@ti.com>
---
drivers/media/platform/ti-vpe/vpe.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/media/platform/ti-vpe/vpe.c b/drivers/media/platform/ti-vpe/vpe.c
index f1eae67..0363df6 100644
--- a/drivers/media/platform/ti-vpe/vpe.c
+++ b/drivers/media/platform/ti-vpe/vpe.c
@@ -2000,7 +2000,7 @@ static struct video_device vpe_videodev = {
.fops = &vpe_fops,
.ioctl_ops = &vpe_ioctl_ops,
.minor = -1,
- .release = video_device_release,
+ .release = video_device_release_empty,
.vfl_dir = VFL_DIR_M2M,
};
--
1.8.3.2
^ permalink raw reply related [flat|nested] 88+ messages in thread
* Re: [PATCH v3 03/14] v4l: ti-vpe: Use video_device_release_empty
2014-03-11 8:33 ` [PATCH v3 03/14] v4l: ti-vpe: Use video_device_release_empty Archit Taneja
@ 2014-03-11 12:02 ` Hans Verkuil
0 siblings, 0 replies; 88+ messages in thread
From: Hans Verkuil @ 2014-03-11 12:02 UTC (permalink / raw)
To: Archit Taneja; +Cc: k.debski, linux-media, linux-omap
On 03/11/14 09:33, Archit Taneja wrote:
> The video_device struct is currently embedded in the driver data struct vpe_dev.
> A vpe_dev instance is allocated by the driver, and the memory for the vfd is a
> part of this struct.
>
> The v4l2 core, however, manages the removal of the vfd region, through the
> video_device's .release() op, which currently is the helper
> video_device_release. This causes memory corruption, and leads to issues when
> we try to re-insert the vpe module.
>
> Use the video_device_release_empty helper function instead
>
> Signed-off-by: Archit Taneja <archit@ti.com>
Reviewed-by: Hans Verkuil <hans.verkuil@cisco.com>
> ---
> drivers/media/platform/ti-vpe/vpe.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/media/platform/ti-vpe/vpe.c b/drivers/media/platform/ti-vpe/vpe.c
> index f1eae67..0363df6 100644
> --- a/drivers/media/platform/ti-vpe/vpe.c
> +++ b/drivers/media/platform/ti-vpe/vpe.c
> @@ -2000,7 +2000,7 @@ static struct video_device vpe_videodev = {
> .fops = &vpe_fops,
> .ioctl_ops = &vpe_ioctl_ops,
> .minor = -1,
> - .release = video_device_release,
> + .release = video_device_release_empty,
> .vfl_dir = VFL_DIR_M2M,
> };
>
>
^ permalink raw reply [flat|nested] 88+ messages in thread
* [PATCH v3 04/14] v4l: ti-vpe: Allow DMABUF buffer type support
2014-03-11 8:33 ` [PATCH v3 00/14] v4l: ti-vpe: Some VPE fixes and enhancements Archit Taneja
` (2 preceding siblings ...)
2014-03-11 8:33 ` [PATCH v3 03/14] v4l: ti-vpe: Use video_device_release_empty Archit Taneja
@ 2014-03-11 8:33 ` Archit Taneja
2014-03-11 12:03 ` Hans Verkuil
2014-03-11 8:33 ` [PATCH v3 05/14] v4l: ti-vpe: Allow usage of smaller images Archit Taneja
` (10 subsequent siblings)
14 siblings, 1 reply; 88+ messages in thread
From: Archit Taneja @ 2014-03-11 8:33 UTC (permalink / raw)
To: k.debski, hverkuil; +Cc: linux-media, linux-omap, Archit Taneja
For OMAP and DRA7x, we generally allocate video and graphics buffers through
omapdrm since the corresponding omap-gem driver provides DMM-Tiler backed
contiguous buffers. omapdrm is a dma-buf exporter. These buffers are used by
other drivers in the video pipeline.
Add VB2_DMABUF flag to the io_modes of the vb2 output and capture queues. This
allows the driver to import dma shared buffers.
Signed-off-by: Archit Taneja <archit@ti.com>
---
drivers/media/platform/ti-vpe/vpe.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/media/platform/ti-vpe/vpe.c b/drivers/media/platform/ti-vpe/vpe.c
index 0363df6..0e7573a 100644
--- a/drivers/media/platform/ti-vpe/vpe.c
+++ b/drivers/media/platform/ti-vpe/vpe.c
@@ -1770,7 +1770,7 @@ static int queue_init(void *priv, struct vb2_queue *src_vq,
memset(src_vq, 0, sizeof(*src_vq));
src_vq->type = V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE;
- src_vq->io_modes = VB2_MMAP;
+ src_vq->io_modes = VB2_MMAP | VB2_DMABUF;
src_vq->drv_priv = ctx;
src_vq->buf_struct_size = sizeof(struct v4l2_m2m_buffer);
src_vq->ops = &vpe_qops;
@@ -1783,7 +1783,7 @@ static int queue_init(void *priv, struct vb2_queue *src_vq,
memset(dst_vq, 0, sizeof(*dst_vq));
dst_vq->type = V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE;
- dst_vq->io_modes = VB2_MMAP;
+ dst_vq->io_modes = VB2_MMAP | VB2_DMABUF;
dst_vq->drv_priv = ctx;
dst_vq->buf_struct_size = sizeof(struct v4l2_m2m_buffer);
dst_vq->ops = &vpe_qops;
--
1.8.3.2
^ permalink raw reply related [flat|nested] 88+ messages in thread
* Re: [PATCH v3 04/14] v4l: ti-vpe: Allow DMABUF buffer type support
2014-03-11 8:33 ` [PATCH v3 04/14] v4l: ti-vpe: Allow DMABUF buffer type support Archit Taneja
@ 2014-03-11 12:03 ` Hans Verkuil
0 siblings, 0 replies; 88+ messages in thread
From: Hans Verkuil @ 2014-03-11 12:03 UTC (permalink / raw)
To: Archit Taneja; +Cc: k.debski, linux-media, linux-omap
On 03/11/14 09:33, Archit Taneja wrote:
> For OMAP and DRA7x, we generally allocate video and graphics buffers through
> omapdrm since the corresponding omap-gem driver provides DMM-Tiler backed
> contiguous buffers. omapdrm is a dma-buf exporter. These buffers are used by
> other drivers in the video pipeline.
>
> Add VB2_DMABUF flag to the io_modes of the vb2 output and capture queues. This
> allows the driver to import dma shared buffers.
>
> Signed-off-by: Archit Taneja <archit@ti.com>
Reviewed-by: Hans Verkuil <hans.verkuil@cisco.com>
> ---
> drivers/media/platform/ti-vpe/vpe.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/media/platform/ti-vpe/vpe.c b/drivers/media/platform/ti-vpe/vpe.c
> index 0363df6..0e7573a 100644
> --- a/drivers/media/platform/ti-vpe/vpe.c
> +++ b/drivers/media/platform/ti-vpe/vpe.c
> @@ -1770,7 +1770,7 @@ static int queue_init(void *priv, struct vb2_queue *src_vq,
>
> memset(src_vq, 0, sizeof(*src_vq));
> src_vq->type = V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE;
> - src_vq->io_modes = VB2_MMAP;
> + src_vq->io_modes = VB2_MMAP | VB2_DMABUF;
> src_vq->drv_priv = ctx;
> src_vq->buf_struct_size = sizeof(struct v4l2_m2m_buffer);
> src_vq->ops = &vpe_qops;
> @@ -1783,7 +1783,7 @@ static int queue_init(void *priv, struct vb2_queue *src_vq,
>
> memset(dst_vq, 0, sizeof(*dst_vq));
> dst_vq->type = V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE;
> - dst_vq->io_modes = VB2_MMAP;
> + dst_vq->io_modes = VB2_MMAP | VB2_DMABUF;
> dst_vq->drv_priv = ctx;
> dst_vq->buf_struct_size = sizeof(struct v4l2_m2m_buffer);
> dst_vq->ops = &vpe_qops;
>
^ permalink raw reply [flat|nested] 88+ messages in thread
* [PATCH v3 05/14] v4l: ti-vpe: Allow usage of smaller images
2014-03-11 8:33 ` [PATCH v3 00/14] v4l: ti-vpe: Some VPE fixes and enhancements Archit Taneja
` (3 preceding siblings ...)
2014-03-11 8:33 ` [PATCH v3 04/14] v4l: ti-vpe: Allow DMABUF buffer type support Archit Taneja
@ 2014-03-11 8:33 ` Archit Taneja
2014-03-11 12:03 ` Hans Verkuil
2014-03-11 8:33 ` [PATCH v3 06/14] v4l: ti-vpe: Fix some params in VPE data descriptors Archit Taneja
` (9 subsequent siblings)
14 siblings, 1 reply; 88+ messages in thread
From: Archit Taneja @ 2014-03-11 8:33 UTC (permalink / raw)
To: k.debski, hverkuil; +Cc: linux-media, linux-omap, Archit Taneja
The minimum width and height for VPE input/output was kept as 128 pixels. VPE
doesn't have a constraint on the image height, it requires the image width to
be at least 16 bytes.
Change the minimum supported dimensions to 32x32. This allows us to de-interlace
qcif content. A smaller image size than 32x32 didn't make much sense, so stopped
at this.
Signed-off-by: Archit Taneja <archit@ti.com>
---
drivers/media/platform/ti-vpe/vpe.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/media/platform/ti-vpe/vpe.c b/drivers/media/platform/ti-vpe/vpe.c
index 0e7573a..dbdc338 100644
--- a/drivers/media/platform/ti-vpe/vpe.c
+++ b/drivers/media/platform/ti-vpe/vpe.c
@@ -49,8 +49,8 @@
#define VPE_MODULE_NAME "vpe"
/* minimum and maximum frame sizes */
-#define MIN_W 128
-#define MIN_H 128
+#define MIN_W 32
+#define MIN_H 32
#define MAX_W 1920
#define MAX_H 1080
--
1.8.3.2
^ permalink raw reply related [flat|nested] 88+ messages in thread
* Re: [PATCH v3 05/14] v4l: ti-vpe: Allow usage of smaller images
2014-03-11 8:33 ` [PATCH v3 05/14] v4l: ti-vpe: Allow usage of smaller images Archit Taneja
@ 2014-03-11 12:03 ` Hans Verkuil
0 siblings, 0 replies; 88+ messages in thread
From: Hans Verkuil @ 2014-03-11 12:03 UTC (permalink / raw)
To: Archit Taneja; +Cc: k.debski, linux-media, linux-omap
On 03/11/14 09:33, Archit Taneja wrote:
> The minimum width and height for VPE input/output was kept as 128 pixels. VPE
> doesn't have a constraint on the image height, it requires the image width to
> be at least 16 bytes.
>
> Change the minimum supported dimensions to 32x32. This allows us to de-interlace
> qcif content. A smaller image size than 32x32 didn't make much sense, so stopped
> at this.
>
> Signed-off-by: Archit Taneja <archit@ti.com>
Reviewed-by: Hans Verkuil <hans.verkuil@cisco.com>
> ---
> drivers/media/platform/ti-vpe/vpe.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/media/platform/ti-vpe/vpe.c b/drivers/media/platform/ti-vpe/vpe.c
> index 0e7573a..dbdc338 100644
> --- a/drivers/media/platform/ti-vpe/vpe.c
> +++ b/drivers/media/platform/ti-vpe/vpe.c
> @@ -49,8 +49,8 @@
> #define VPE_MODULE_NAME "vpe"
>
> /* minimum and maximum frame sizes */
> -#define MIN_W 128
> -#define MIN_H 128
> +#define MIN_W 32
> +#define MIN_H 32
> #define MAX_W 1920
> #define MAX_H 1080
>
>
^ permalink raw reply [flat|nested] 88+ messages in thread
* [PATCH v3 06/14] v4l: ti-vpe: Fix some params in VPE data descriptors
2014-03-11 8:33 ` [PATCH v3 00/14] v4l: ti-vpe: Some VPE fixes and enhancements Archit Taneja
` (4 preceding siblings ...)
2014-03-11 8:33 ` [PATCH v3 05/14] v4l: ti-vpe: Allow usage of smaller images Archit Taneja
@ 2014-03-11 8:33 ` Archit Taneja
2014-03-11 8:33 ` [PATCH v3 07/14] v4l: ti-vpe: Add selection API in VPE driver Archit Taneja
` (8 subsequent siblings)
14 siblings, 0 replies; 88+ messages in thread
From: Archit Taneja @ 2014-03-11 8:33 UTC (permalink / raw)
To: k.debski, hverkuil; +Cc: linux-media, linux-omap, Archit Taneja
Some parameters of the VPE descriptors were understood incorrectly. They are now
fixed. The fixes are explained as follows:
- When adding an inbound data descriptor to the VPDMA descriptor list, we intend
to use c_rect as the cropped region fetched by VPDMA. Therefore, c_rect->width
shouldn't be used to calculate the line stride, the original image width
should be used for that. We add a 'width' argument which gives the buffer
width in memory.
- frame_width and frame_height describe the complete width and height of the
client to which the channel is connected. If there are multiple channels
fetching data and providing to the same client, the above 2 arguments should
be the width and height of the region covered by all the channels. In the case
where there is only one channel providing pixel data to the client
(like in VPE), frame_width and frame_height should be the cropped width and
cropped height respectively. The calculation of these params is done in the
vpe driver now.
- start_h and start_v is also used in the case of multiple channels to describe
where each channel should start filling pixel data. We don't use this in VPE,
and pass 0s to the vpdma_add_in_dtd() helper.
- Some minor changes are made to the vpdma_add_out_dtd() helper. The c_rect
param is used for specifying the 'composition' target, and 'width' is added
to calculate the line stride.
Signed-off-by: Archit Taneja <archit@ti.com>
---
drivers/media/platform/ti-vpe/vpdma.c | 60 +++++++++++++++++++++++++++--------
drivers/media/platform/ti-vpe/vpdma.h | 10 +++---
drivers/media/platform/ti-vpe/vpe.c | 18 +++++++----
3 files changed, 64 insertions(+), 24 deletions(-)
diff --git a/drivers/media/platform/ti-vpe/vpdma.c b/drivers/media/platform/ti-vpe/vpdma.c
index 73dd38e..a51a013 100644
--- a/drivers/media/platform/ti-vpe/vpdma.c
+++ b/drivers/media/platform/ti-vpe/vpdma.c
@@ -614,8 +614,17 @@ static void dump_dtd(struct vpdma_dtd *dtd)
/*
* append an outbound data transfer descriptor to the given descriptor list,
* this sets up a 'client to memory' VPDMA transfer for the given VPDMA channel
+ *
+ * @list: vpdma desc list to which we add this decriptor
+ * @width: width of the image in pixels in memory
+ * @c_rect: compose params of output image
+ * @fmt: vpdma data format of the buffer
+ * dma_addr: dma address as seen by VPDMA
+ * chan: VPDMA channel
+ * flags: VPDMA flags to configure some descriptor fileds
*/
-void vpdma_add_out_dtd(struct vpdma_desc_list *list, struct v4l2_rect *c_rect,
+void vpdma_add_out_dtd(struct vpdma_desc_list *list, int width,
+ const struct v4l2_rect *c_rect,
const struct vpdma_data_format *fmt, dma_addr_t dma_addr,
enum vpdma_channel chan, u32 flags)
{
@@ -623,6 +632,7 @@ void vpdma_add_out_dtd(struct vpdma_desc_list *list, struct v4l2_rect *c_rect,
int field = 0;
int notify = 1;
int channel, next_chan;
+ struct v4l2_rect rect = *c_rect;
int depth = fmt->depth;
int stride;
struct vpdma_dtd *dtd;
@@ -630,11 +640,15 @@ void vpdma_add_out_dtd(struct vpdma_desc_list *list, struct v4l2_rect *c_rect,
channel = next_chan = chan_info[chan].num;
if (fmt->type == VPDMA_DATA_FMT_TYPE_YUV &&
- fmt->data_type == DATA_TYPE_C420)
+ fmt->data_type == DATA_TYPE_C420) {
+ rect.height >>= 1;
+ rect.top >>= 1;
depth = 8;
+ }
- stride = ALIGN((depth * c_rect->width) >> 3, VPDMA_STRIDE_ALIGN);
- dma_addr += (c_rect->left * depth) >> 3;
+ stride = ALIGN((depth * width) >> 3, VPDMA_STRIDE_ALIGN);
+
+ dma_addr += rect.top * stride + (rect.left * depth >> 3);
dtd = list->next;
WARN_ON((void *)(dtd + 1) > (list->buf.addr + list->buf.size));
@@ -664,31 +678,48 @@ void vpdma_add_out_dtd(struct vpdma_desc_list *list, struct v4l2_rect *c_rect,
/*
* append an inbound data transfer descriptor to the given descriptor list,
* this sets up a 'memory to client' VPDMA transfer for the given VPDMA channel
+ *
+ * @list: vpdma desc list to which we add this decriptor
+ * @width: width of the image in pixels in memory(not the cropped width)
+ * @c_rect: crop params of input image
+ * @fmt: vpdma data format of the buffer
+ * dma_addr: dma address as seen by VPDMA
+ * chan: VPDMA channel
+ * field: top or bottom field info of the input image
+ * flags: VPDMA flags to configure some descriptor fileds
+ * frame_width/height: the complete width/height of the image presented to the
+ * client (this makes sense when multiple channels are
+ * connected to the same client, forming a larger frame)
+ * start_h, start_v: position where the given channel starts providing pixel
+ * data to the client (makes sense when multiple channels
+ * contribute to the client)
*/
-void vpdma_add_in_dtd(struct vpdma_desc_list *list, int frame_width,
- int frame_height, struct v4l2_rect *c_rect,
+void vpdma_add_in_dtd(struct vpdma_desc_list *list, int width,
+ const struct v4l2_rect *c_rect,
const struct vpdma_data_format *fmt, dma_addr_t dma_addr,
- enum vpdma_channel chan, int field, u32 flags)
+ enum vpdma_channel chan, int field, u32 flags, int frame_width,
+ int frame_height, int start_h, int start_v)
{
int priority = 0;
int notify = 1;
int depth = fmt->depth;
int channel, next_chan;
+ struct v4l2_rect rect = *c_rect;
int stride;
- int height = c_rect->height;
struct vpdma_dtd *dtd;
channel = next_chan = chan_info[chan].num;
if (fmt->type == VPDMA_DATA_FMT_TYPE_YUV &&
fmt->data_type == DATA_TYPE_C420) {
- height >>= 1;
- frame_height >>= 1;
+ rect.height >>= 1;
+ rect.top >>= 1;
depth = 8;
}
- stride = ALIGN((depth * c_rect->width) >> 3, VPDMA_STRIDE_ALIGN);
- dma_addr += (c_rect->left * depth) >> 3;
+ stride = ALIGN((depth * width) >> 3, VPDMA_STRIDE_ALIGN);
+
+ dma_addr += rect.top * stride + (rect.left * depth >> 3);
dtd = list->next;
WARN_ON((void *)(dtd + 1) > (list->buf.addr + list->buf.size));
@@ -701,13 +732,14 @@ void vpdma_add_in_dtd(struct vpdma_desc_list *list, int frame_width,
!!(flags & VPDMA_DATA_ODD_LINE_SKIP),
stride);
- dtd->xfer_length_height = dtd_xfer_length_height(c_rect->width, height);
+ dtd->xfer_length_height = dtd_xfer_length_height(rect.width,
+ rect.height);
dtd->start_addr = (u32) dma_addr;
dtd->pkt_ctl = dtd_pkt_ctl(!!(flags & VPDMA_DATA_MODE_TILED),
DTD_DIR_IN, channel, priority, next_chan);
dtd->frame_width_height = dtd_frame_width_height(frame_width,
frame_height);
- dtd->start_h_v = dtd_start_h_v(c_rect->left, c_rect->top);
+ dtd->start_h_v = dtd_start_h_v(start_h, start_v);
dtd->client_attr0 = 0;
dtd->client_attr1 = 0;
diff --git a/drivers/media/platform/ti-vpe/vpdma.h b/drivers/media/platform/ti-vpe/vpdma.h
index bf5f8bb..2bd8fb0 100644
--- a/drivers/media/platform/ti-vpe/vpdma.h
+++ b/drivers/media/platform/ti-vpe/vpdma.h
@@ -186,13 +186,15 @@ void vpdma_add_cfd_adb(struct vpdma_desc_list *list, int client,
struct vpdma_buf *adb);
void vpdma_add_sync_on_channel_ctd(struct vpdma_desc_list *list,
enum vpdma_channel chan);
-void vpdma_add_out_dtd(struct vpdma_desc_list *list, struct v4l2_rect *c_rect,
+void vpdma_add_out_dtd(struct vpdma_desc_list *list, int width,
+ const struct v4l2_rect *c_rect,
const struct vpdma_data_format *fmt, dma_addr_t dma_addr,
enum vpdma_channel chan, u32 flags);
-void vpdma_add_in_dtd(struct vpdma_desc_list *list, int frame_width,
- int frame_height, struct v4l2_rect *c_rect,
+void vpdma_add_in_dtd(struct vpdma_desc_list *list, int width,
+ const struct v4l2_rect *c_rect,
const struct vpdma_data_format *fmt, dma_addr_t dma_addr,
- enum vpdma_channel chan, int field, u32 flags);
+ enum vpdma_channel chan, int field, u32 flags, int frame_width,
+ int frame_height, int start_h, int start_v);
/* vpdma list interrupt management */
void vpdma_enable_list_complete_irq(struct vpdma_data *vpdma, int list_num,
diff --git a/drivers/media/platform/ti-vpe/vpe.c b/drivers/media/platform/ti-vpe/vpe.c
index dbdc338..ece9b96 100644
--- a/drivers/media/platform/ti-vpe/vpe.c
+++ b/drivers/media/platform/ti-vpe/vpe.c
@@ -986,7 +986,6 @@ static void add_out_dtd(struct vpe_ctx *ctx, int port)
struct vpe_q_data *q_data = &ctx->q_data[Q_DATA_DST];
const struct vpe_port_data *p_data = &port_data[port];
struct vb2_buffer *vb = ctx->dst_vb;
- struct v4l2_rect *c_rect = &q_data->c_rect;
struct vpe_fmt *fmt = q_data->fmt;
const struct vpdma_data_format *vpdma_fmt;
int mv_buf_selector = !ctx->src_mv_buf_selector;
@@ -1015,8 +1014,8 @@ static void add_out_dtd(struct vpe_ctx *ctx, int port)
if (q_data->flags & Q_DATA_MODE_TILED)
flags |= VPDMA_DATA_MODE_TILED;
- vpdma_add_out_dtd(&ctx->desc_list, c_rect, vpdma_fmt, dma_addr,
- p_data->channel, flags);
+ vpdma_add_out_dtd(&ctx->desc_list, q_data->width, &q_data->c_rect,
+ vpdma_fmt, dma_addr, p_data->channel, flags);
}
static void add_in_dtd(struct vpe_ctx *ctx, int port)
@@ -1024,11 +1023,11 @@ static void add_in_dtd(struct vpe_ctx *ctx, int port)
struct vpe_q_data *q_data = &ctx->q_data[Q_DATA_SRC];
const struct vpe_port_data *p_data = &port_data[port];
struct vb2_buffer *vb = ctx->src_vbs[p_data->vb_index];
- struct v4l2_rect *c_rect = &q_data->c_rect;
struct vpe_fmt *fmt = q_data->fmt;
const struct vpdma_data_format *vpdma_fmt;
int mv_buf_selector = ctx->src_mv_buf_selector;
int field = vb->v4l2_buf.field == V4L2_FIELD_BOTTOM;
+ int frame_width, frame_height;
dma_addr_t dma_addr;
u32 flags = 0;
@@ -1055,8 +1054,15 @@ static void add_in_dtd(struct vpe_ctx *ctx, int port)
if (q_data->flags & Q_DATA_MODE_TILED)
flags |= VPDMA_DATA_MODE_TILED;
- vpdma_add_in_dtd(&ctx->desc_list, q_data->width, q_data->height,
- c_rect, vpdma_fmt, dma_addr, p_data->channel, field, flags);
+ frame_width = q_data->c_rect.width;
+ frame_height = q_data->c_rect.height;
+
+ if (p_data->vb_part && fmt->fourcc == V4L2_PIX_FMT_NV12)
+ frame_height /= 2;
+
+ vpdma_add_in_dtd(&ctx->desc_list, q_data->width, &q_data->c_rect,
+ vpdma_fmt, dma_addr, p_data->channel, field, flags, frame_width,
+ frame_height, 0, 0);
}
/*
--
1.8.3.2
^ permalink raw reply related [flat|nested] 88+ messages in thread
* [PATCH v3 07/14] v4l: ti-vpe: Add selection API in VPE driver
2014-03-11 8:33 ` [PATCH v3 00/14] v4l: ti-vpe: Some VPE fixes and enhancements Archit Taneja
` (5 preceding siblings ...)
2014-03-11 8:33 ` [PATCH v3 06/14] v4l: ti-vpe: Fix some params in VPE data descriptors Archit Taneja
@ 2014-03-11 8:33 ` Archit Taneja
2014-03-11 12:21 ` Hans Verkuil
2014-03-11 8:33 ` [PATCH v3 08/14] v4l: ti-vpe: Rename csc memory resource name Archit Taneja
` (7 subsequent siblings)
14 siblings, 1 reply; 88+ messages in thread
From: Archit Taneja @ 2014-03-11 8:33 UTC (permalink / raw)
To: k.debski, hverkuil; +Cc: linux-media, linux-omap, Archit Taneja
Add selection ioctl ops. For VPE, cropping makes sense only for the input to
VPE(or V4L2_BUF_TYPE_VIDEO_OUTPUT/MPLANE buffers) and composing makes sense
only for the output of VPE(or V4L2_BUF_TYPE_VIDEO_CAPTURE/MPLANE buffers).
For the CAPTURE type, V4L2_SEL_TGT_COMPOSE results in VPE writing the output
in a rectangle within the capture buffer. For the OUTPUT type, V4L2_SEL_TGT_CROP
results in selecting a rectangle region within the source buffer.
Setting the crop/compose rectangles should successfully result in
re-configuration of registers which are affected when either source or
destination dimensions change, set_srcdst_params() is called for this purpose.
Signed-off-by: Archit Taneja <archit@ti.com>
---
drivers/media/platform/ti-vpe/vpe.c | 141 ++++++++++++++++++++++++++++++++++++
1 file changed, 141 insertions(+)
diff --git a/drivers/media/platform/ti-vpe/vpe.c b/drivers/media/platform/ti-vpe/vpe.c
index ece9b96..4abb85c 100644
--- a/drivers/media/platform/ti-vpe/vpe.c
+++ b/drivers/media/platform/ti-vpe/vpe.c
@@ -410,8 +410,10 @@ static struct vpe_q_data *get_q_data(struct vpe_ctx *ctx,
{
switch (type) {
case V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE:
+ case V4L2_BUF_TYPE_VIDEO_OUTPUT:
return &ctx->q_data[Q_DATA_SRC];
case V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE:
+ case V4L2_BUF_TYPE_VIDEO_CAPTURE:
return &ctx->q_data[Q_DATA_DST];
default:
BUG();
@@ -1587,6 +1589,142 @@ static int vpe_s_fmt(struct file *file, void *priv, struct v4l2_format *f)
return set_srcdst_params(ctx);
}
+static int __vpe_try_selection(struct vpe_ctx *ctx, struct v4l2_selection *s)
+{
+ struct vpe_q_data *q_data;
+
+ if ((s->type != V4L2_BUF_TYPE_VIDEO_CAPTURE) &&
+ (s->type != V4L2_BUF_TYPE_VIDEO_OUTPUT))
+ return -EINVAL;
+
+ q_data = get_q_data(ctx, s->type);
+ if (!q_data)
+ return -EINVAL;
+
+ switch (s->target) {
+ case V4L2_SEL_TGT_COMPOSE:
+ /*
+ * COMPOSE target is only valid for capture buffer type, for
+ * output buffer type, assign existing crop parameters to the
+ * selection rectangle
+ */
+ if (s->type == V4L2_BUF_TYPE_VIDEO_CAPTURE)
+ break;
+
+ s->r = q_data->c_rect;
+ return 0;
+
+ case V4L2_SEL_TGT_CROP:
+ /*
+ * CROP target is only valid for output buffer type, for capture
+ * buffer type, assign existing compose parameters to the
+ * selection rectangle
+ */
+ if (s->type == V4L2_BUF_TYPE_VIDEO_OUTPUT)
+ break;
+
+ s->r = q_data->c_rect;
+ return 0;
+
+ /*
+ * bound and default crop/compose targets are invalid targets to
+ * try/set
+ */
+ default:
+ return -EINVAL;
+ }
+
+ if (s->r.top < 0 || s->r.left < 0) {
+ vpe_err(ctx->dev, "negative values for top and left\n");
+ s->r.top = s->r.left = 0;
+ }
+
+ v4l_bound_align_image(&s->r.width, MIN_W, q_data->width, 1,
+ &s->r.height, MIN_H, q_data->height, H_ALIGN, S_ALIGN);
+
+ /* adjust left/top if cropping rectangle is out of bounds */
+ if (s->r.left + s->r.width > q_data->width)
+ s->r.left = q_data->width - s->r.width;
+ if (s->r.top + s->r.height > q_data->height)
+ s->r.top = q_data->height - s->r.height;
+
+ return 0;
+}
+
+static int vpe_g_selection(struct file *file, void *fh,
+ struct v4l2_selection *s)
+{
+ struct vpe_ctx *ctx = file2ctx(file);
+ struct vpe_q_data *q_data;
+
+ if ((s->type != V4L2_BUF_TYPE_VIDEO_CAPTURE) &&
+ (s->type != V4L2_BUF_TYPE_VIDEO_OUTPUT))
+ return -EINVAL;
+
+ q_data = get_q_data(ctx, s->type);
+ if (!q_data)
+ return -EINVAL;
+
+ switch (s->target) {
+ /* return width and height from S_FMT of the respective buffer type */
+ case V4L2_SEL_TGT_COMPOSE_DEFAULT:
+ case V4L2_SEL_TGT_COMPOSE_BOUNDS:
+ case V4L2_SEL_TGT_CROP_BOUNDS:
+ case V4L2_SEL_TGT_CROP_DEFAULT:
+ s->r.left = 0;
+ s->r.top = 0;
+ s->r.width = q_data->width;
+ s->r.height = q_data->height;
+ return 0;
+
+ /*
+ * CROP target holds for the output buffer type, and COMPOSE target
+ * holds for the capture buffer type. We still return the c_rect params
+ * for both the target types.
+ */
+ case V4L2_SEL_TGT_COMPOSE:
+ case V4L2_SEL_TGT_CROP:
+ s->r.left = q_data->c_rect.left;
+ s->r.top = q_data->c_rect.top;
+ s->r.width = q_data->c_rect.width;
+ s->r.height = q_data->c_rect.height;
+ return 0;
+ }
+
+ return -EINVAL;
+}
+
+
+static int vpe_s_selection(struct file *file, void *fh,
+ struct v4l2_selection *s)
+{
+ struct vpe_ctx *ctx = file2ctx(file);
+ struct vpe_q_data *q_data;
+ struct v4l2_selection sel = *s;
+ int ret;
+
+ ret = __vpe_try_selection(ctx, &sel);
+ if (ret)
+ return ret;
+
+ q_data = get_q_data(ctx, sel.type);
+ if (!q_data)
+ return -EINVAL;
+
+ if ((q_data->c_rect.left == sel.r.left) &&
+ (q_data->c_rect.top == sel.r.top) &&
+ (q_data->c_rect.width == sel.r.width) &&
+ (q_data->c_rect.height == sel.r.height)) {
+ vpe_dbg(ctx->dev,
+ "requested crop/compose values are already set\n");
+ return 0;
+ }
+
+ q_data->c_rect = sel.r;
+
+ return set_srcdst_params(ctx);
+}
+
static int vpe_reqbufs(struct file *file, void *priv,
struct v4l2_requestbuffers *reqbufs)
{
@@ -1674,6 +1812,9 @@ static const struct v4l2_ioctl_ops vpe_ioctl_ops = {
.vidioc_try_fmt_vid_out_mplane = vpe_try_fmt,
.vidioc_s_fmt_vid_out_mplane = vpe_s_fmt,
+ .vidioc_g_selection = vpe_g_selection,
+ .vidioc_s_selection = vpe_s_selection,
+
.vidioc_reqbufs = vpe_reqbufs,
.vidioc_querybuf = vpe_querybuf,
--
1.8.3.2
^ permalink raw reply related [flat|nested] 88+ messages in thread
* Re: [PATCH v3 07/14] v4l: ti-vpe: Add selection API in VPE driver
2014-03-11 8:33 ` [PATCH v3 07/14] v4l: ti-vpe: Add selection API in VPE driver Archit Taneja
@ 2014-03-11 12:21 ` Hans Verkuil
2014-03-11 12:46 ` Archit Taneja
0 siblings, 1 reply; 88+ messages in thread
From: Hans Verkuil @ 2014-03-11 12:21 UTC (permalink / raw)
To: Archit Taneja; +Cc: k.debski, linux-media, linux-omap
Hi Archit,
A few small comments below...
On 03/11/14 09:33, Archit Taneja wrote:
> Add selection ioctl ops. For VPE, cropping makes sense only for the input to
> VPE(or V4L2_BUF_TYPE_VIDEO_OUTPUT/MPLANE buffers) and composing makes sense
> only for the output of VPE(or V4L2_BUF_TYPE_VIDEO_CAPTURE/MPLANE buffers).
>
> For the CAPTURE type, V4L2_SEL_TGT_COMPOSE results in VPE writing the output
> in a rectangle within the capture buffer. For the OUTPUT type, V4L2_SEL_TGT_CROP
> results in selecting a rectangle region within the source buffer.
>
> Setting the crop/compose rectangles should successfully result in
> re-configuration of registers which are affected when either source or
> destination dimensions change, set_srcdst_params() is called for this purpose.
>
> Signed-off-by: Archit Taneja <archit@ti.com>
> ---
> drivers/media/platform/ti-vpe/vpe.c | 141 ++++++++++++++++++++++++++++++++++++
> 1 file changed, 141 insertions(+)
>
> diff --git a/drivers/media/platform/ti-vpe/vpe.c b/drivers/media/platform/ti-vpe/vpe.c
> index ece9b96..4abb85c 100644
> --- a/drivers/media/platform/ti-vpe/vpe.c
> +++ b/drivers/media/platform/ti-vpe/vpe.c
> @@ -410,8 +410,10 @@ static struct vpe_q_data *get_q_data(struct vpe_ctx *ctx,
> {
> switch (type) {
> case V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE:
> + case V4L2_BUF_TYPE_VIDEO_OUTPUT:
> return &ctx->q_data[Q_DATA_SRC];
> case V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE:
> + case V4L2_BUF_TYPE_VIDEO_CAPTURE:
> return &ctx->q_data[Q_DATA_DST];
> default:
> BUG();
> @@ -1587,6 +1589,142 @@ static int vpe_s_fmt(struct file *file, void *priv, struct v4l2_format *f)
> return set_srcdst_params(ctx);
> }
>
> +static int __vpe_try_selection(struct vpe_ctx *ctx, struct v4l2_selection *s)
> +{
> + struct vpe_q_data *q_data;
> +
> + if ((s->type != V4L2_BUF_TYPE_VIDEO_CAPTURE) &&
> + (s->type != V4L2_BUF_TYPE_VIDEO_OUTPUT))
> + return -EINVAL;
> +
> + q_data = get_q_data(ctx, s->type);
> + if (!q_data)
> + return -EINVAL;
> +
> + switch (s->target) {
> + case V4L2_SEL_TGT_COMPOSE:
> + /*
> + * COMPOSE target is only valid for capture buffer type, for
> + * output buffer type, assign existing crop parameters to the
> + * selection rectangle
> + */
> + if (s->type == V4L2_BUF_TYPE_VIDEO_CAPTURE)
> + break;
Shouldn't this return -EINVAL?
> +
> + s->r = q_data->c_rect;
> + return 0;
> +
> + case V4L2_SEL_TGT_CROP:
> + /*
> + * CROP target is only valid for output buffer type, for capture
> + * buffer type, assign existing compose parameters to the
> + * selection rectangle
> + */
> + if (s->type == V4L2_BUF_TYPE_VIDEO_OUTPUT)
> + break;
Ditto.
> +
> + s->r = q_data->c_rect;
> + return 0;
> +
> + /*
> + * bound and default crop/compose targets are invalid targets to
> + * try/set
> + */
> + default:
> + return -EINVAL;
> + }
> +
> + if (s->r.top < 0 || s->r.left < 0) {
> + vpe_err(ctx->dev, "negative values for top and left\n");
> + s->r.top = s->r.left = 0;
> + }
> +
> + v4l_bound_align_image(&s->r.width, MIN_W, q_data->width, 1,
> + &s->r.height, MIN_H, q_data->height, H_ALIGN, S_ALIGN);
> +
> + /* adjust left/top if cropping rectangle is out of bounds */
> + if (s->r.left + s->r.width > q_data->width)
> + s->r.left = q_data->width - s->r.width;
> + if (s->r.top + s->r.height > q_data->height)
> + s->r.top = q_data->height - s->r.height;
> +
> + return 0;
> +}
> +
> +static int vpe_g_selection(struct file *file, void *fh,
> + struct v4l2_selection *s)
> +{
> + struct vpe_ctx *ctx = file2ctx(file);
> + struct vpe_q_data *q_data;
> +
> + if ((s->type != V4L2_BUF_TYPE_VIDEO_CAPTURE) &&
> + (s->type != V4L2_BUF_TYPE_VIDEO_OUTPUT))
> + return -EINVAL;
> +
> + q_data = get_q_data(ctx, s->type);
> + if (!q_data)
> + return -EINVAL;
> +
> + switch (s->target) {
> + /* return width and height from S_FMT of the respective buffer type */
> + case V4L2_SEL_TGT_COMPOSE_DEFAULT:
> + case V4L2_SEL_TGT_COMPOSE_BOUNDS:
> + case V4L2_SEL_TGT_CROP_BOUNDS:
> + case V4L2_SEL_TGT_CROP_DEFAULT:
> + s->r.left = 0;
> + s->r.top = 0;
> + s->r.width = q_data->width;
> + s->r.height = q_data->height;
The crop targets only make sense for type OUTPUT and the compose only for
type CAPTURE. Add some checks for that.
> + return 0;
> +
> + /*
> + * CROP target holds for the output buffer type, and COMPOSE target
> + * holds for the capture buffer type. We still return the c_rect params
> + * for both the target types.
> + */
> + case V4L2_SEL_TGT_COMPOSE:
> + case V4L2_SEL_TGT_CROP:
> + s->r.left = q_data->c_rect.left;
> + s->r.top = q_data->c_rect.top;
> + s->r.width = q_data->c_rect.width;
> + s->r.height = q_data->c_rect.height;
> + return 0;
> + }
> +
> + return -EINVAL;
> +}
> +
> +
> +static int vpe_s_selection(struct file *file, void *fh,
> + struct v4l2_selection *s)
> +{
> + struct vpe_ctx *ctx = file2ctx(file);
> + struct vpe_q_data *q_data;
> + struct v4l2_selection sel = *s;
> + int ret;
> +
> + ret = __vpe_try_selection(ctx, &sel);
> + if (ret)
> + return ret;
> +
> + q_data = get_q_data(ctx, sel.type);
> + if (!q_data)
> + return -EINVAL;
> +
> + if ((q_data->c_rect.left == sel.r.left) &&
> + (q_data->c_rect.top == sel.r.top) &&
> + (q_data->c_rect.width == sel.r.width) &&
> + (q_data->c_rect.height == sel.r.height)) {
> + vpe_dbg(ctx->dev,
> + "requested crop/compose values are already set\n");
> + return 0;
> + }
> +
> + q_data->c_rect = sel.r;
> +
> + return set_srcdst_params(ctx);
> +}
> +
> static int vpe_reqbufs(struct file *file, void *priv,
> struct v4l2_requestbuffers *reqbufs)
> {
> @@ -1674,6 +1812,9 @@ static const struct v4l2_ioctl_ops vpe_ioctl_ops = {
> .vidioc_try_fmt_vid_out_mplane = vpe_try_fmt,
> .vidioc_s_fmt_vid_out_mplane = vpe_s_fmt,
>
> + .vidioc_g_selection = vpe_g_selection,
> + .vidioc_s_selection = vpe_s_selection,
> +
> .vidioc_reqbufs = vpe_reqbufs,
> .vidioc_querybuf = vpe_querybuf,
>
>
^ permalink raw reply [flat|nested] 88+ messages in thread
* Re: [PATCH v3 07/14] v4l: ti-vpe: Add selection API in VPE driver
2014-03-11 12:21 ` Hans Verkuil
@ 2014-03-11 12:46 ` Archit Taneja
2014-03-11 12:49 ` Hans Verkuil
0 siblings, 1 reply; 88+ messages in thread
From: Archit Taneja @ 2014-03-11 12:46 UTC (permalink / raw)
To: Hans Verkuil; +Cc: k.debski, linux-media, linux-omap
On Tuesday 11 March 2014 05:51 PM, Hans Verkuil wrote:
> Hi Archit,
>
> A few small comments below...
>
> On 03/11/14 09:33, Archit Taneja wrote:
>> Add selection ioctl ops. For VPE, cropping makes sense only for the input to
>> VPE(or V4L2_BUF_TYPE_VIDEO_OUTPUT/MPLANE buffers) and composing makes sense
>> only for the output of VPE(or V4L2_BUF_TYPE_VIDEO_CAPTURE/MPLANE buffers).
>>
>> For the CAPTURE type, V4L2_SEL_TGT_COMPOSE results in VPE writing the output
>> in a rectangle within the capture buffer. For the OUTPUT type, V4L2_SEL_TGT_CROP
>> results in selecting a rectangle region within the source buffer.
>>
>> Setting the crop/compose rectangles should successfully result in
>> re-configuration of registers which are affected when either source or
>> destination dimensions change, set_srcdst_params() is called for this purpose.
>>
>> Signed-off-by: Archit Taneja <archit@ti.com>
>> ---
>> drivers/media/platform/ti-vpe/vpe.c | 141 ++++++++++++++++++++++++++++++++++++
>> 1 file changed, 141 insertions(+)
>>
>> diff --git a/drivers/media/platform/ti-vpe/vpe.c b/drivers/media/platform/ti-vpe/vpe.c
>> index ece9b96..4abb85c 100644
>> --- a/drivers/media/platform/ti-vpe/vpe.c
>> +++ b/drivers/media/platform/ti-vpe/vpe.c
>> @@ -410,8 +410,10 @@ static struct vpe_q_data *get_q_data(struct vpe_ctx *ctx,
>> {
>> switch (type) {
>> case V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE:
>> + case V4L2_BUF_TYPE_VIDEO_OUTPUT:
>> return &ctx->q_data[Q_DATA_SRC];
>> case V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE:
>> + case V4L2_BUF_TYPE_VIDEO_CAPTURE:
>> return &ctx->q_data[Q_DATA_DST];
>> default:
>> BUG();
>> @@ -1587,6 +1589,142 @@ static int vpe_s_fmt(struct file *file, void *priv, struct v4l2_format *f)
>> return set_srcdst_params(ctx);
>> }
>>
>> +static int __vpe_try_selection(struct vpe_ctx *ctx, struct v4l2_selection *s)
>> +{
>> + struct vpe_q_data *q_data;
>> +
>> + if ((s->type != V4L2_BUF_TYPE_VIDEO_CAPTURE) &&
>> + (s->type != V4L2_BUF_TYPE_VIDEO_OUTPUT))
>> + return -EINVAL;
>> +
>> + q_data = get_q_data(ctx, s->type);
>> + if (!q_data)
>> + return -EINVAL;
>> +
>> + switch (s->target) {
>> + case V4L2_SEL_TGT_COMPOSE:
>> + /*
>> + * COMPOSE target is only valid for capture buffer type, for
>> + * output buffer type, assign existing crop parameters to the
>> + * selection rectangle
>> + */
>> + if (s->type == V4L2_BUF_TYPE_VIDEO_CAPTURE)
>> + break;
>
> Shouldn't this return -EINVAL?
compose only makes sense for CAPTURE. So, it breaks and performs the
calculations after the switch statement.
>
>> +
>> + s->r = q_data->c_rect;
>> + return 0;
The above 2 lines are called for if we try to set compose for OUTPUT. I
don't return an error here, just keep the size as the original rect
size, and return 0.
I'll replace these 2 lines with 'return -EINVAL;'
>> +
>> + case V4L2_SEL_TGT_CROP:
>> + /*
>> + * CROP target is only valid for output buffer type, for capture
>> + * buffer type, assign existing compose parameters to the
>> + * selection rectangle
>> + */
>> + if (s->type == V4L2_BUF_TYPE_VIDEO_OUTPUT)
>> + break;
>
> Ditto.
>
>> +
>> + s->r = q_data->c_rect;
>> + return 0;
>> +
>> + /*
>> + * bound and default crop/compose targets are invalid targets to
>> + * try/set
>> + */
>> + default:
>> + return -EINVAL;
>> + }
>> +
>> + if (s->r.top < 0 || s->r.left < 0) {
>> + vpe_err(ctx->dev, "negative values for top and left\n");
>> + s->r.top = s->r.left = 0;
>> + }
>> +
>> + v4l_bound_align_image(&s->r.width, MIN_W, q_data->width, 1,
>> + &s->r.height, MIN_H, q_data->height, H_ALIGN, S_ALIGN);
>> +
>> + /* adjust left/top if cropping rectangle is out of bounds */
>> + if (s->r.left + s->r.width > q_data->width)
>> + s->r.left = q_data->width - s->r.width;
>> + if (s->r.top + s->r.height > q_data->height)
>> + s->r.top = q_data->height - s->r.height;
>> +
>> + return 0;
>> +}
>> +
>> +static int vpe_g_selection(struct file *file, void *fh,
>> + struct v4l2_selection *s)
>> +{
>> + struct vpe_ctx *ctx = file2ctx(file);
>> + struct vpe_q_data *q_data;
>> +
>> + if ((s->type != V4L2_BUF_TYPE_VIDEO_CAPTURE) &&
>> + (s->type != V4L2_BUF_TYPE_VIDEO_OUTPUT))
>> + return -EINVAL;
>> +
>> + q_data = get_q_data(ctx, s->type);
>> + if (!q_data)
>> + return -EINVAL;
>> +
>> + switch (s->target) {
>> + /* return width and height from S_FMT of the respective buffer type */
>> + case V4L2_SEL_TGT_COMPOSE_DEFAULT:
>> + case V4L2_SEL_TGT_COMPOSE_BOUNDS:
>> + case V4L2_SEL_TGT_CROP_BOUNDS:
>> + case V4L2_SEL_TGT_CROP_DEFAULT:
>> + s->r.left = 0;
>> + s->r.top = 0;
>> + s->r.width = q_data->width;
>> + s->r.height = q_data->height;
>
> The crop targets only make sense for type OUTPUT and the compose only for
> type CAPTURE. Add some checks for that.
I return the image size for G_FMT irrespective of what combination of
crop/compose CAPTURE/OUTPUT it is. I suppose it is better to return an
error if the user tries to configure a wrong combination.
Thanks,
Archit
^ permalink raw reply [flat|nested] 88+ messages in thread
* Re: [PATCH v3 07/14] v4l: ti-vpe: Add selection API in VPE driver
2014-03-11 12:46 ` Archit Taneja
@ 2014-03-11 12:49 ` Hans Verkuil
2014-03-11 13:10 ` Archit Taneja
0 siblings, 1 reply; 88+ messages in thread
From: Hans Verkuil @ 2014-03-11 12:49 UTC (permalink / raw)
To: Archit Taneja; +Cc: k.debski, linux-media, linux-omap
On 03/11/14 13:46, Archit Taneja wrote:
> On Tuesday 11 March 2014 05:51 PM, Hans Verkuil wrote:
>> Hi Archit,
>>
>> A few small comments below...
>>
>> On 03/11/14 09:33, Archit Taneja wrote:
>>> Add selection ioctl ops. For VPE, cropping makes sense only for the input to
>>> VPE(or V4L2_BUF_TYPE_VIDEO_OUTPUT/MPLANE buffers) and composing makes sense
>>> only for the output of VPE(or V4L2_BUF_TYPE_VIDEO_CAPTURE/MPLANE buffers).
>>>
>>> For the CAPTURE type, V4L2_SEL_TGT_COMPOSE results in VPE writing the output
>>> in a rectangle within the capture buffer. For the OUTPUT type, V4L2_SEL_TGT_CROP
>>> results in selecting a rectangle region within the source buffer.
>>>
>>> Setting the crop/compose rectangles should successfully result in
>>> re-configuration of registers which are affected when either source or
>>> destination dimensions change, set_srcdst_params() is called for this purpose.
>>>
>>> Signed-off-by: Archit Taneja <archit@ti.com>
>>> ---
>>> drivers/media/platform/ti-vpe/vpe.c | 141 ++++++++++++++++++++++++++++++++++++
>>> 1 file changed, 141 insertions(+)
>>>
>>> diff --git a/drivers/media/platform/ti-vpe/vpe.c b/drivers/media/platform/ti-vpe/vpe.c
>>> index ece9b96..4abb85c 100644
>>> --- a/drivers/media/platform/ti-vpe/vpe.c
>>> +++ b/drivers/media/platform/ti-vpe/vpe.c
>>> @@ -410,8 +410,10 @@ static struct vpe_q_data *get_q_data(struct vpe_ctx *ctx,
>>> {
>>> switch (type) {
>>> case V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE:
>>> + case V4L2_BUF_TYPE_VIDEO_OUTPUT:
>>> return &ctx->q_data[Q_DATA_SRC];
>>> case V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE:
>>> + case V4L2_BUF_TYPE_VIDEO_CAPTURE:
>>> return &ctx->q_data[Q_DATA_DST];
>>> default:
>>> BUG();
>>> @@ -1587,6 +1589,142 @@ static int vpe_s_fmt(struct file *file, void *priv, struct v4l2_format *f)
>>> return set_srcdst_params(ctx);
>>> }
>>>
>>> +static int __vpe_try_selection(struct vpe_ctx *ctx, struct v4l2_selection *s)
>>> +{
>>> + struct vpe_q_data *q_data;
>>> +
>>> + if ((s->type != V4L2_BUF_TYPE_VIDEO_CAPTURE) &&
>>> + (s->type != V4L2_BUF_TYPE_VIDEO_OUTPUT))
>>> + return -EINVAL;
>>> +
>>> + q_data = get_q_data(ctx, s->type);
>>> + if (!q_data)
>>> + return -EINVAL;
>>> +
>>> + switch (s->target) {
>>> + case V4L2_SEL_TGT_COMPOSE:
>>> + /*
>>> + * COMPOSE target is only valid for capture buffer type, for
>>> + * output buffer type, assign existing crop parameters to the
>>> + * selection rectangle
>>> + */
>>> + if (s->type == V4L2_BUF_TYPE_VIDEO_CAPTURE)
>>> + break;
>>
>> Shouldn't this return -EINVAL?
>
> compose only makes sense for CAPTURE. So, it breaks and performs the calculations after the switch statement.
It's so easy to get confused here.
>
>>
>>> +
>>> + s->r = q_data->c_rect;
>>> + return 0;
>
> The above 2 lines are called for if we try to set compose for OUTPUT. I don't return an error here, just keep the size as the original rect size, and return 0.
>
> I'll replace these 2 lines with 'return -EINVAL;'
Yes, that makes more sense.
>
>>> +
>>> + case V4L2_SEL_TGT_CROP:
>>> + /*
>>> + * CROP target is only valid for output buffer type, for capture
>>> + * buffer type, assign existing compose parameters to the
>>> + * selection rectangle
>>> + */
>>> + if (s->type == V4L2_BUF_TYPE_VIDEO_OUTPUT)
>>> + break;
>>
>> Ditto.
>>
>>> +
>>> + s->r = q_data->c_rect;
>>> + return 0;
>>> +
>>> + /*
>>> + * bound and default crop/compose targets are invalid targets to
>>> + * try/set
>>> + */
>>> + default:
>>> + return -EINVAL;
>>> + }
>>> +
>>> + if (s->r.top < 0 || s->r.left < 0) {
>>> + vpe_err(ctx->dev, "negative values for top and left\n");
>>> + s->r.top = s->r.left = 0;
>>> + }
>>> +
>>> + v4l_bound_align_image(&s->r.width, MIN_W, q_data->width, 1,
>>> + &s->r.height, MIN_H, q_data->height, H_ALIGN, S_ALIGN);
>>> +
>>> + /* adjust left/top if cropping rectangle is out of bounds */
>>> + if (s->r.left + s->r.width > q_data->width)
>>> + s->r.left = q_data->width - s->r.width;
>>> + if (s->r.top + s->r.height > q_data->height)
>>> + s->r.top = q_data->height - s->r.height;
>>> +
>>> + return 0;
>>> +}
>>> +
>>> +static int vpe_g_selection(struct file *file, void *fh,
>>> + struct v4l2_selection *s)
>>> +{
>>> + struct vpe_ctx *ctx = file2ctx(file);
>>> + struct vpe_q_data *q_data;
>>> +
>>> + if ((s->type != V4L2_BUF_TYPE_VIDEO_CAPTURE) &&
>>> + (s->type != V4L2_BUF_TYPE_VIDEO_OUTPUT))
>>> + return -EINVAL;
>>> +
>>> + q_data = get_q_data(ctx, s->type);
>>> + if (!q_data)
>>> + return -EINVAL;
>>> +
>>> + switch (s->target) {
>>> + /* return width and height from S_FMT of the respective buffer type */
>>> + case V4L2_SEL_TGT_COMPOSE_DEFAULT:
>>> + case V4L2_SEL_TGT_COMPOSE_BOUNDS:
>>> + case V4L2_SEL_TGT_CROP_BOUNDS:
>>> + case V4L2_SEL_TGT_CROP_DEFAULT:
>>> + s->r.left = 0;
>>> + s->r.top = 0;
>>> + s->r.width = q_data->width;
>>> + s->r.height = q_data->height;
>>
>> The crop targets only make sense for type OUTPUT and the compose only for
>> type CAPTURE. Add some checks for that.
>
> I return the image size for G_FMT irrespective of what combination of
> crop/compose CAPTURE/OUTPUT it is. I suppose it is better to return
> an error if the user tries to configure a wrong combination.
Yes. If for no other reason that I plan on adding crop/compose/scaling support
to v4l2-compliance soon, and this will probably be one of the tests.
>
> Thanks,
> Archit
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-media" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 88+ messages in thread
* Re: [PATCH v3 07/14] v4l: ti-vpe: Add selection API in VPE driver
2014-03-11 12:49 ` Hans Verkuil
@ 2014-03-11 13:10 ` Archit Taneja
0 siblings, 0 replies; 88+ messages in thread
From: Archit Taneja @ 2014-03-11 13:10 UTC (permalink / raw)
To: Hans Verkuil; +Cc: k.debski, linux-media, linux-omap
On Tuesday 11 March 2014 06:19 PM, Hans Verkuil wrote:
> On 03/11/14 13:46, Archit Taneja wrote:
>> On Tuesday 11 March 2014 05:51 PM, Hans Verkuil wrote:
>>> Hi Archit,
>>>
>>> A few small comments below...
>>>
>>> On 03/11/14 09:33, Archit Taneja wrote:
<snip>
> Yes. If for no other reason that I plan on adding crop/compose/scaling support
> to v4l2-compliance soon, and this will probably be one of the tests.
Thanks, will update. Thanks for reviewing of the series.
Archit
>
>>
>> Thanks,
>> Archit
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-media" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
^ permalink raw reply [flat|nested] 88+ messages in thread
* [PATCH v3 08/14] v4l: ti-vpe: Rename csc memory resource name
2014-03-11 8:33 ` [PATCH v3 00/14] v4l: ti-vpe: Some VPE fixes and enhancements Archit Taneja
` (6 preceding siblings ...)
2014-03-11 8:33 ` [PATCH v3 07/14] v4l: ti-vpe: Add selection API in VPE driver Archit Taneja
@ 2014-03-11 8:33 ` Archit Taneja
2014-03-11 8:33 ` [PATCH v3 09/14] v4l: ti-vpe: report correct capabilities in querycap Archit Taneja
` (6 subsequent siblings)
14 siblings, 0 replies; 88+ messages in thread
From: Archit Taneja @ 2014-03-11 8:33 UTC (permalink / raw)
To: k.debski, hverkuil; +Cc: linux-media, linux-omap, Archit Taneja
Rename the memory block resource "vpe_csc" to "csc" since it also exists within
the VIP IP block. This would make the name more generic, and both VPE and VIP DT
nodes in the future can use it.
Signed-off-by: Archit Taneja <archit@ti.com>
---
drivers/media/platform/ti-vpe/csc.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/media/platform/ti-vpe/csc.c b/drivers/media/platform/ti-vpe/csc.c
index acfea50..0333339 100644
--- a/drivers/media/platform/ti-vpe/csc.c
+++ b/drivers/media/platform/ti-vpe/csc.c
@@ -180,7 +180,7 @@ struct csc_data *csc_create(struct platform_device *pdev)
csc->pdev = pdev;
csc->res = platform_get_resource_byname(pdev, IORESOURCE_MEM,
- "vpe_csc");
+ "csc");
if (csc->res == NULL) {
dev_err(&pdev->dev, "missing platform resources data\n");
return ERR_PTR(-ENODEV);
--
1.8.3.2
^ permalink raw reply related [flat|nested] 88+ messages in thread
* [PATCH v3 09/14] v4l: ti-vpe: report correct capabilities in querycap
2014-03-11 8:33 ` [PATCH v3 00/14] v4l: ti-vpe: Some VPE fixes and enhancements Archit Taneja
` (7 preceding siblings ...)
2014-03-11 8:33 ` [PATCH v3 08/14] v4l: ti-vpe: Rename csc memory resource name Archit Taneja
@ 2014-03-11 8:33 ` Archit Taneja
2014-03-11 12:23 ` Hans Verkuil
2014-03-11 8:33 ` [PATCH v3 10/14] v4l: ti-vpe: Use correct bus_info name for the device " Archit Taneja
` (5 subsequent siblings)
14 siblings, 1 reply; 88+ messages in thread
From: Archit Taneja @ 2014-03-11 8:33 UTC (permalink / raw)
To: k.debski, hverkuil; +Cc: linux-media, linux-omap, Archit Taneja
querycap currently returns V4L2_CAP_VIDEO_M2M as a capability, this should be
V4L2_CAP_VIDEO_M2M_MPLANE instead, as the driver supports multiplanar formats.
Signed-off-by: Archit Taneja <archit@ti.com>
---
drivers/media/platform/ti-vpe/vpe.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/media/platform/ti-vpe/vpe.c b/drivers/media/platform/ti-vpe/vpe.c
index 4abb85c..46b9d44 100644
--- a/drivers/media/platform/ti-vpe/vpe.c
+++ b/drivers/media/platform/ti-vpe/vpe.c
@@ -1347,7 +1347,7 @@ static int vpe_querycap(struct file *file, void *priv,
strncpy(cap->driver, VPE_MODULE_NAME, sizeof(cap->driver) - 1);
strncpy(cap->card, VPE_MODULE_NAME, sizeof(cap->card) - 1);
strlcpy(cap->bus_info, VPE_MODULE_NAME, sizeof(cap->bus_info));
- cap->device_caps = V4L2_CAP_VIDEO_M2M | V4L2_CAP_STREAMING;
+ cap->device_caps = V4L2_CAP_VIDEO_M2M_MPLANE | V4L2_CAP_STREAMING;
cap->capabilities = cap->device_caps | V4L2_CAP_DEVICE_CAPS;
return 0;
}
--
1.8.3.2
^ permalink raw reply related [flat|nested] 88+ messages in thread
* Re: [PATCH v3 09/14] v4l: ti-vpe: report correct capabilities in querycap
2014-03-11 8:33 ` [PATCH v3 09/14] v4l: ti-vpe: report correct capabilities in querycap Archit Taneja
@ 2014-03-11 12:23 ` Hans Verkuil
0 siblings, 0 replies; 88+ messages in thread
From: Hans Verkuil @ 2014-03-11 12:23 UTC (permalink / raw)
To: Archit Taneja; +Cc: k.debski, linux-media, linux-omap
On 03/11/14 09:33, Archit Taneja wrote:
> querycap currently returns V4L2_CAP_VIDEO_M2M as a capability, this should be
> V4L2_CAP_VIDEO_M2M_MPLANE instead, as the driver supports multiplanar formats.
>
> Signed-off-by: Archit Taneja <archit@ti.com>
Reviewed-by: Hans Verkuil <hans.verkuil@cisco.com>
> ---
> drivers/media/platform/ti-vpe/vpe.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/media/platform/ti-vpe/vpe.c b/drivers/media/platform/ti-vpe/vpe.c
> index 4abb85c..46b9d44 100644
> --- a/drivers/media/platform/ti-vpe/vpe.c
> +++ b/drivers/media/platform/ti-vpe/vpe.c
> @@ -1347,7 +1347,7 @@ static int vpe_querycap(struct file *file, void *priv,
> strncpy(cap->driver, VPE_MODULE_NAME, sizeof(cap->driver) - 1);
> strncpy(cap->card, VPE_MODULE_NAME, sizeof(cap->card) - 1);
> strlcpy(cap->bus_info, VPE_MODULE_NAME, sizeof(cap->bus_info));
> - cap->device_caps = V4L2_CAP_VIDEO_M2M | V4L2_CAP_STREAMING;
> + cap->device_caps = V4L2_CAP_VIDEO_M2M_MPLANE | V4L2_CAP_STREAMING;
> cap->capabilities = cap->device_caps | V4L2_CAP_DEVICE_CAPS;
> return 0;
> }
>
^ permalink raw reply [flat|nested] 88+ messages in thread
* [PATCH v3 10/14] v4l: ti-vpe: Use correct bus_info name for the device in querycap
2014-03-11 8:33 ` [PATCH v3 00/14] v4l: ti-vpe: Some VPE fixes and enhancements Archit Taneja
` (8 preceding siblings ...)
2014-03-11 8:33 ` [PATCH v3 09/14] v4l: ti-vpe: report correct capabilities in querycap Archit Taneja
@ 2014-03-11 8:33 ` Archit Taneja
2014-03-11 12:23 ` Hans Verkuil
2014-03-11 8:33 ` [PATCH v3 11/14] v4l: ti-vpe: Fix initial configuration queue data Archit Taneja
` (4 subsequent siblings)
14 siblings, 1 reply; 88+ messages in thread
From: Archit Taneja @ 2014-03-11 8:33 UTC (permalink / raw)
To: k.debski, hverkuil; +Cc: linux-media, linux-omap, Archit Taneja
The bus_info parameter in v4l2_capabilities expects a 'platform_' prefix. This
wasn't done in the driver and hence was breaking compliance. Update the bus_info
parameter accordingly.
Signed-off-by: Archit Taneja <archit@ti.com>
---
drivers/media/platform/ti-vpe/vpe.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/media/platform/ti-vpe/vpe.c b/drivers/media/platform/ti-vpe/vpe.c
index 46b9d44..5591d04 100644
--- a/drivers/media/platform/ti-vpe/vpe.c
+++ b/drivers/media/platform/ti-vpe/vpe.c
@@ -1346,7 +1346,8 @@ static int vpe_querycap(struct file *file, void *priv,
{
strncpy(cap->driver, VPE_MODULE_NAME, sizeof(cap->driver) - 1);
strncpy(cap->card, VPE_MODULE_NAME, sizeof(cap->card) - 1);
- strlcpy(cap->bus_info, VPE_MODULE_NAME, sizeof(cap->bus_info));
+ snprintf(cap->bus_info, sizeof(cap->bus_info), "platform:%s",
+ VPE_MODULE_NAME);
cap->device_caps = V4L2_CAP_VIDEO_M2M_MPLANE | V4L2_CAP_STREAMING;
cap->capabilities = cap->device_caps | V4L2_CAP_DEVICE_CAPS;
return 0;
--
1.8.3.2
^ permalink raw reply related [flat|nested] 88+ messages in thread
* Re: [PATCH v3 10/14] v4l: ti-vpe: Use correct bus_info name for the device in querycap
2014-03-11 8:33 ` [PATCH v3 10/14] v4l: ti-vpe: Use correct bus_info name for the device " Archit Taneja
@ 2014-03-11 12:23 ` Hans Verkuil
0 siblings, 0 replies; 88+ messages in thread
From: Hans Verkuil @ 2014-03-11 12:23 UTC (permalink / raw)
To: Archit Taneja; +Cc: k.debski, linux-media, linux-omap
On 03/11/14 09:33, Archit Taneja wrote:
> The bus_info parameter in v4l2_capabilities expects a 'platform_' prefix. This
> wasn't done in the driver and hence was breaking compliance. Update the bus_info
> parameter accordingly.
>
> Signed-off-by: Archit Taneja <archit@ti.com>
Reviewed-by: Hans Verkuil <hans.verkuil@cisco.com>
> ---
> drivers/media/platform/ti-vpe/vpe.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/media/platform/ti-vpe/vpe.c b/drivers/media/platform/ti-vpe/vpe.c
> index 46b9d44..5591d04 100644
> --- a/drivers/media/platform/ti-vpe/vpe.c
> +++ b/drivers/media/platform/ti-vpe/vpe.c
> @@ -1346,7 +1346,8 @@ static int vpe_querycap(struct file *file, void *priv,
> {
> strncpy(cap->driver, VPE_MODULE_NAME, sizeof(cap->driver) - 1);
> strncpy(cap->card, VPE_MODULE_NAME, sizeof(cap->card) - 1);
> - strlcpy(cap->bus_info, VPE_MODULE_NAME, sizeof(cap->bus_info));
> + snprintf(cap->bus_info, sizeof(cap->bus_info), "platform:%s",
> + VPE_MODULE_NAME);
> cap->device_caps = V4L2_CAP_VIDEO_M2M_MPLANE | V4L2_CAP_STREAMING;
> cap->capabilities = cap->device_caps | V4L2_CAP_DEVICE_CAPS;
> return 0;
>
^ permalink raw reply [flat|nested] 88+ messages in thread
* [PATCH v3 11/14] v4l: ti-vpe: Fix initial configuration queue data
2014-03-11 8:33 ` [PATCH v3 00/14] v4l: ti-vpe: Some VPE fixes and enhancements Archit Taneja
` (9 preceding siblings ...)
2014-03-11 8:33 ` [PATCH v3 10/14] v4l: ti-vpe: Use correct bus_info name for the device " Archit Taneja
@ 2014-03-11 8:33 ` Archit Taneja
2014-03-11 12:24 ` Hans Verkuil
2014-03-11 8:33 ` [PATCH v3 12/14] v4l: ti-vpe: zero out reserved fields in try_fmt Archit Taneja
` (3 subsequent siblings)
14 siblings, 1 reply; 88+ messages in thread
From: Archit Taneja @ 2014-03-11 8:33 UTC (permalink / raw)
To: k.debski, hverkuil; +Cc: linux-media, linux-omap, Archit Taneja
The vpe output and capture queues are initially configured to default values in
vpe_open(). A G_FMT before any S_FMTs will result in these values being
populated.
The colorspace and bytesperline parameter of this initial configuration are
incorrect. This breaks compliance when as we get 'TRY_FMT(G_FMT) != G_FMT'.
Fix the initial queue configuration such that it wouldn't need to be fixed by
try_fmt.
Signed-off-by: Archit Taneja <archit@ti.com>
---
drivers/media/platform/ti-vpe/vpe.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/drivers/media/platform/ti-vpe/vpe.c b/drivers/media/platform/ti-vpe/vpe.c
index 5591d04..85d1122 100644
--- a/drivers/media/platform/ti-vpe/vpe.c
+++ b/drivers/media/platform/ti-vpe/vpe.c
@@ -2012,9 +2012,11 @@ static int vpe_open(struct file *file)
s_q_data->fmt = &vpe_formats[2];
s_q_data->width = 1920;
s_q_data->height = 1080;
- s_q_data->sizeimage[VPE_LUMA] = (s_q_data->width * s_q_data->height *
+ s_q_data->bytesperline[VPE_LUMA] = (s_q_data->width *
s_q_data->fmt->vpdma_fmt[VPE_LUMA]->depth) >> 3;
- s_q_data->colorspace = V4L2_COLORSPACE_SMPTE170M;
+ s_q_data->sizeimage[VPE_LUMA] = (s_q_data->bytesperline[VPE_LUMA] *
+ s_q_data->height);
+ s_q_data->colorspace = V4L2_COLORSPACE_REC709;
s_q_data->field = V4L2_FIELD_NONE;
s_q_data->c_rect.left = 0;
s_q_data->c_rect.top = 0;
--
1.8.3.2
^ permalink raw reply related [flat|nested] 88+ messages in thread
* Re: [PATCH v3 11/14] v4l: ti-vpe: Fix initial configuration queue data
2014-03-11 8:33 ` [PATCH v3 11/14] v4l: ti-vpe: Fix initial configuration queue data Archit Taneja
@ 2014-03-11 12:24 ` Hans Verkuil
0 siblings, 0 replies; 88+ messages in thread
From: Hans Verkuil @ 2014-03-11 12:24 UTC (permalink / raw)
To: Archit Taneja; +Cc: k.debski, linux-media, linux-omap
On 03/11/14 09:33, Archit Taneja wrote:
> The vpe output and capture queues are initially configured to default values in
> vpe_open(). A G_FMT before any S_FMTs will result in these values being
> populated.
>
> The colorspace and bytesperline parameter of this initial configuration are
> incorrect. This breaks compliance when as we get 'TRY_FMT(G_FMT) != G_FMT'.
>
> Fix the initial queue configuration such that it wouldn't need to be fixed by
> try_fmt.
>
> Signed-off-by: Archit Taneja <archit@ti.com>
Reviewed-by: Hans Verkuil <hans.verkuil@cisco.com>
> ---
> drivers/media/platform/ti-vpe/vpe.c | 6 ++++--
> 1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/media/platform/ti-vpe/vpe.c b/drivers/media/platform/ti-vpe/vpe.c
> index 5591d04..85d1122 100644
> --- a/drivers/media/platform/ti-vpe/vpe.c
> +++ b/drivers/media/platform/ti-vpe/vpe.c
> @@ -2012,9 +2012,11 @@ static int vpe_open(struct file *file)
> s_q_data->fmt = &vpe_formats[2];
> s_q_data->width = 1920;
> s_q_data->height = 1080;
> - s_q_data->sizeimage[VPE_LUMA] = (s_q_data->width * s_q_data->height *
> + s_q_data->bytesperline[VPE_LUMA] = (s_q_data->width *
> s_q_data->fmt->vpdma_fmt[VPE_LUMA]->depth) >> 3;
> - s_q_data->colorspace = V4L2_COLORSPACE_SMPTE170M;
> + s_q_data->sizeimage[VPE_LUMA] = (s_q_data->bytesperline[VPE_LUMA] *
> + s_q_data->height);
> + s_q_data->colorspace = V4L2_COLORSPACE_REC709;
> s_q_data->field = V4L2_FIELD_NONE;
> s_q_data->c_rect.left = 0;
> s_q_data->c_rect.top = 0;
>
^ permalink raw reply [flat|nested] 88+ messages in thread
* [PATCH v3 12/14] v4l: ti-vpe: zero out reserved fields in try_fmt
2014-03-11 8:33 ` [PATCH v3 00/14] v4l: ti-vpe: Some VPE fixes and enhancements Archit Taneja
` (10 preceding siblings ...)
2014-03-11 8:33 ` [PATCH v3 11/14] v4l: ti-vpe: Fix initial configuration queue data Archit Taneja
@ 2014-03-11 8:33 ` Archit Taneja
2014-03-11 12:25 ` Hans Verkuil
2014-03-11 8:33 ` [PATCH v3 13/14] v4l: ti-vpe: Set correct field parameter for output and capture buffers Archit Taneja
` (2 subsequent siblings)
14 siblings, 1 reply; 88+ messages in thread
From: Archit Taneja @ 2014-03-11 8:33 UTC (permalink / raw)
To: k.debski, hverkuil; +Cc: linux-media, linux-omap, Archit Taneja
Zero out the reserved formats in v4l2_pix_format_mplane and
v4l2_plane_pix_format members of the returned v4l2_format pointer when passed
through TRY_FMT ioctl.
This ensures that the user doesn't interpret the non-zero fields as some data
passed by the driver, and ensures compliance.
Signed-off-by: Archit Taneja <archit@ti.com>
---
drivers/media/platform/ti-vpe/vpe.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/drivers/media/platform/ti-vpe/vpe.c b/drivers/media/platform/ti-vpe/vpe.c
index 85d1122..970408a 100644
--- a/drivers/media/platform/ti-vpe/vpe.c
+++ b/drivers/media/platform/ti-vpe/vpe.c
@@ -1488,6 +1488,7 @@ static int __vpe_try_fmt(struct vpe_ctx *ctx, struct v4l2_format *f,
}
}
+ memset(pix->reserved, 0, sizeof(pix->reserved));
for (i = 0; i < pix->num_planes; i++) {
plane_fmt = &pix->plane_fmt[i];
depth = fmt->vpdma_fmt[i]->depth;
@@ -1499,6 +1500,8 @@ static int __vpe_try_fmt(struct vpe_ctx *ctx, struct v4l2_format *f,
plane_fmt->sizeimage =
(pix->height * pix->width * depth) >> 3;
+
+ memset(plane_fmt->reserved, 0, sizeof(plane_fmt->reserved));
}
return 0;
--
1.8.3.2
^ permalink raw reply related [flat|nested] 88+ messages in thread
* Re: [PATCH v3 12/14] v4l: ti-vpe: zero out reserved fields in try_fmt
2014-03-11 8:33 ` [PATCH v3 12/14] v4l: ti-vpe: zero out reserved fields in try_fmt Archit Taneja
@ 2014-03-11 12:25 ` Hans Verkuil
0 siblings, 0 replies; 88+ messages in thread
From: Hans Verkuil @ 2014-03-11 12:25 UTC (permalink / raw)
To: Archit Taneja; +Cc: k.debski, linux-media, linux-omap
On 03/11/14 09:33, Archit Taneja wrote:
> Zero out the reserved formats in v4l2_pix_format_mplane and
> v4l2_plane_pix_format members of the returned v4l2_format pointer when passed
> through TRY_FMT ioctl.
>
> This ensures that the user doesn't interpret the non-zero fields as some data
> passed by the driver, and ensures compliance.
>
> Signed-off-by: Archit Taneja <archit@ti.com>
Reviewed-by: Hans Verkuil <hans.verkuil@cisco.com>
> ---
> drivers/media/platform/ti-vpe/vpe.c | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/drivers/media/platform/ti-vpe/vpe.c b/drivers/media/platform/ti-vpe/vpe.c
> index 85d1122..970408a 100644
> --- a/drivers/media/platform/ti-vpe/vpe.c
> +++ b/drivers/media/platform/ti-vpe/vpe.c
> @@ -1488,6 +1488,7 @@ static int __vpe_try_fmt(struct vpe_ctx *ctx, struct v4l2_format *f,
> }
> }
>
> + memset(pix->reserved, 0, sizeof(pix->reserved));
> for (i = 0; i < pix->num_planes; i++) {
> plane_fmt = &pix->plane_fmt[i];
> depth = fmt->vpdma_fmt[i]->depth;
> @@ -1499,6 +1500,8 @@ static int __vpe_try_fmt(struct vpe_ctx *ctx, struct v4l2_format *f,
>
> plane_fmt->sizeimage =
> (pix->height * pix->width * depth) >> 3;
> +
> + memset(plane_fmt->reserved, 0, sizeof(plane_fmt->reserved));
> }
>
> return 0;
>
^ permalink raw reply [flat|nested] 88+ messages in thread
* [PATCH v3 13/14] v4l: ti-vpe: Set correct field parameter for output and capture buffers
2014-03-11 8:33 ` [PATCH v3 00/14] v4l: ti-vpe: Some VPE fixes and enhancements Archit Taneja
` (11 preceding siblings ...)
2014-03-11 8:33 ` [PATCH v3 12/14] v4l: ti-vpe: zero out reserved fields in try_fmt Archit Taneja
@ 2014-03-11 8:33 ` Archit Taneja
2014-03-11 12:29 ` Hans Verkuil
2014-03-11 8:33 ` [PATCH v3 14/14] v4l: ti-vpe: retain v4l2_buffer flags for captured buffers Archit Taneja
2014-03-13 11:44 ` [PATCH v4 00/14] v4l: ti-vpe: Some VPE fixes and enhancements Archit Taneja
14 siblings, 1 reply; 88+ messages in thread
From: Archit Taneja @ 2014-03-11 8:33 UTC (permalink / raw)
To: k.debski, hverkuil; +Cc: linux-media, linux-omap, Archit Taneja
The vpe driver wasn't setting the correct field parameter for dequed CAPTURE
type buffers for the case where the captured output is progressive.
Set the field to V4L2_FIELD_NONE for the completed destination buffers when
the captured output is progressive.
For OUTPUT type buffers, a queued buffer's field is forced to V4L2_FIELD_NONE
if the pixel format(configured through s_fmt for the buffer type
V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE specifies) the field type isn't interlaced.
If the pixel format specified was V4L2_FIELD_ALTERNATE, and the queued buffer's
field isn't V4L2_FIELD_TOP or V4L2_FIELD_BOTTOM, the vb2 buf_prepare op returns
an error.
This ensures compliance, and that the dequeued output and captured buffers
contain the field type that the driver used internally.
Signed-off-by: Archit Taneja <archit@ti.com>
---
drivers/media/platform/ti-vpe/vpe.c | 13 ++++++++++++-
1 file changed, 12 insertions(+), 1 deletion(-)
diff --git a/drivers/media/platform/ti-vpe/vpe.c b/drivers/media/platform/ti-vpe/vpe.c
index 970408a..c884910 100644
--- a/drivers/media/platform/ti-vpe/vpe.c
+++ b/drivers/media/platform/ti-vpe/vpe.c
@@ -1296,10 +1296,10 @@ static irqreturn_t vpe_irq(int irq_vpe, void *data)
d_buf->timecode = s_buf->timecode;
}
d_buf->sequence = ctx->sequence;
- d_buf->field = ctx->field;
d_q_data = &ctx->q_data[Q_DATA_DST];
if (d_q_data->flags & Q_DATA_INTERLACED) {
+ d_buf->field = ctx->field;
if (ctx->field == V4L2_FIELD_BOTTOM) {
ctx->sequence++;
ctx->field = V4L2_FIELD_TOP;
@@ -1308,6 +1308,7 @@ static irqreturn_t vpe_irq(int irq_vpe, void *data)
ctx->field = V4L2_FIELD_BOTTOM;
}
} else {
+ d_buf->field = V4L2_FIELD_NONE;
ctx->sequence++;
}
@@ -1871,6 +1872,16 @@ static int vpe_buf_prepare(struct vb2_buffer *vb)
q_data = get_q_data(ctx, vb->vb2_queue->type);
num_planes = q_data->fmt->coplanar ? 2 : 1;
+ if (vb->vb2_queue->type == V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE) {
+ if (!(q_data->flags & Q_DATA_INTERLACED)) {
+ vb->v4l2_buf.field = V4L2_FIELD_NONE;
+ } else {
+ if (vb->v4l2_buf.field != V4L2_FIELD_TOP ||
+ vb->v4l2_buf.field != V4L2_FIELD_BOTTOM)
+ return -EINVAL;
+ }
+ }
+
for (i = 0; i < num_planes; i++) {
if (vb2_plane_size(vb, i) < q_data->sizeimage[i]) {
vpe_err(ctx->dev,
--
1.8.3.2
^ permalink raw reply related [flat|nested] 88+ messages in thread
* Re: [PATCH v3 13/14] v4l: ti-vpe: Set correct field parameter for output and capture buffers
2014-03-11 8:33 ` [PATCH v3 13/14] v4l: ti-vpe: Set correct field parameter for output and capture buffers Archit Taneja
@ 2014-03-11 12:29 ` Hans Verkuil
0 siblings, 0 replies; 88+ messages in thread
From: Hans Verkuil @ 2014-03-11 12:29 UTC (permalink / raw)
To: Archit Taneja; +Cc: k.debski, linux-media, linux-omap
On 03/11/14 09:33, Archit Taneja wrote:
> The vpe driver wasn't setting the correct field parameter for dequed CAPTURE
> type buffers for the case where the captured output is progressive.
>
> Set the field to V4L2_FIELD_NONE for the completed destination buffers when
> the captured output is progressive.
>
> For OUTPUT type buffers, a queued buffer's field is forced to V4L2_FIELD_NONE
> if the pixel format(configured through s_fmt for the buffer type
> V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE specifies) the field type isn't interlaced.
> If the pixel format specified was V4L2_FIELD_ALTERNATE, and the queued buffer's
> field isn't V4L2_FIELD_TOP or V4L2_FIELD_BOTTOM, the vb2 buf_prepare op returns
> an error.
>
> This ensures compliance, and that the dequeued output and captured buffers
> contain the field type that the driver used internally.
>
> Signed-off-by: Archit Taneja <archit@ti.com>
Reviewed-by: Hans Verkuil <hans.verkuil@cisco.com>
> ---
> drivers/media/platform/ti-vpe/vpe.c | 13 ++++++++++++-
> 1 file changed, 12 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/media/platform/ti-vpe/vpe.c b/drivers/media/platform/ti-vpe/vpe.c
> index 970408a..c884910 100644
> --- a/drivers/media/platform/ti-vpe/vpe.c
> +++ b/drivers/media/platform/ti-vpe/vpe.c
> @@ -1296,10 +1296,10 @@ static irqreturn_t vpe_irq(int irq_vpe, void *data)
> d_buf->timecode = s_buf->timecode;
> }
> d_buf->sequence = ctx->sequence;
> - d_buf->field = ctx->field;
>
> d_q_data = &ctx->q_data[Q_DATA_DST];
> if (d_q_data->flags & Q_DATA_INTERLACED) {
> + d_buf->field = ctx->field;
> if (ctx->field == V4L2_FIELD_BOTTOM) {
> ctx->sequence++;
> ctx->field = V4L2_FIELD_TOP;
> @@ -1308,6 +1308,7 @@ static irqreturn_t vpe_irq(int irq_vpe, void *data)
> ctx->field = V4L2_FIELD_BOTTOM;
> }
> } else {
> + d_buf->field = V4L2_FIELD_NONE;
> ctx->sequence++;
> }
>
> @@ -1871,6 +1872,16 @@ static int vpe_buf_prepare(struct vb2_buffer *vb)
> q_data = get_q_data(ctx, vb->vb2_queue->type);
> num_planes = q_data->fmt->coplanar ? 2 : 1;
>
> + if (vb->vb2_queue->type == V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE) {
> + if (!(q_data->flags & Q_DATA_INTERLACED)) {
> + vb->v4l2_buf.field = V4L2_FIELD_NONE;
> + } else {
> + if (vb->v4l2_buf.field != V4L2_FIELD_TOP ||
> + vb->v4l2_buf.field != V4L2_FIELD_BOTTOM)
> + return -EINVAL;
> + }
> + }
> +
> for (i = 0; i < num_planes; i++) {
> if (vb2_plane_size(vb, i) < q_data->sizeimage[i]) {
> vpe_err(ctx->dev,
>
^ permalink raw reply [flat|nested] 88+ messages in thread
* [PATCH v3 14/14] v4l: ti-vpe: retain v4l2_buffer flags for captured buffers
2014-03-11 8:33 ` [PATCH v3 00/14] v4l: ti-vpe: Some VPE fixes and enhancements Archit Taneja
` (12 preceding siblings ...)
2014-03-11 8:33 ` [PATCH v3 13/14] v4l: ti-vpe: Set correct field parameter for output and capture buffers Archit Taneja
@ 2014-03-11 8:33 ` Archit Taneja
2014-03-11 12:31 ` Hans Verkuil
2014-03-13 11:44 ` [PATCH v4 00/14] v4l: ti-vpe: Some VPE fixes and enhancements Archit Taneja
14 siblings, 1 reply; 88+ messages in thread
From: Archit Taneja @ 2014-03-11 8:33 UTC (permalink / raw)
To: k.debski, hverkuil; +Cc: linux-media, linux-omap, Archit Taneja
The dequed CAPTURE_MPLANE type buffers don't contain the flags that the
originally queued OUTPUT_MPLANE type buffers have. This breaks compliance.
Copy the source v4l2_buffer flags to the destination v4l2_buffer flags before
they are dequed.
Signed-off-by: Archit Taneja <archit@ti.com>
---
drivers/media/platform/ti-vpe/vpe.c | 9 ++++-----
1 file changed, 4 insertions(+), 5 deletions(-)
diff --git a/drivers/media/platform/ti-vpe/vpe.c b/drivers/media/platform/ti-vpe/vpe.c
index c884910..f7759e8 100644
--- a/drivers/media/platform/ti-vpe/vpe.c
+++ b/drivers/media/platform/ti-vpe/vpe.c
@@ -1288,13 +1288,12 @@ static irqreturn_t vpe_irq(int irq_vpe, void *data)
s_buf = &s_vb->v4l2_buf;
d_buf = &d_vb->v4l2_buf;
+ d_buf->flags = s_buf->flags;
+
d_buf->timestamp = s_buf->timestamp;
- d_buf->flags &= ~V4L2_BUF_FLAG_TSTAMP_SRC_MASK;
- d_buf->flags |= s_buf->flags & V4L2_BUF_FLAG_TSTAMP_SRC_MASK;
- if (s_buf->flags & V4L2_BUF_FLAG_TIMECODE) {
- d_buf->flags |= V4L2_BUF_FLAG_TIMECODE;
+ if (s_buf->flags & V4L2_BUF_FLAG_TIMECODE)
d_buf->timecode = s_buf->timecode;
- }
+
d_buf->sequence = ctx->sequence;
d_q_data = &ctx->q_data[Q_DATA_DST];
--
1.8.3.2
^ permalink raw reply related [flat|nested] 88+ messages in thread
* Re: [PATCH v3 14/14] v4l: ti-vpe: retain v4l2_buffer flags for captured buffers
2014-03-11 8:33 ` [PATCH v3 14/14] v4l: ti-vpe: retain v4l2_buffer flags for captured buffers Archit Taneja
@ 2014-03-11 12:31 ` Hans Verkuil
0 siblings, 0 replies; 88+ messages in thread
From: Hans Verkuil @ 2014-03-11 12:31 UTC (permalink / raw)
To: Archit Taneja; +Cc: k.debski, linux-media, linux-omap
On 03/11/14 09:33, Archit Taneja wrote:
> The dequed CAPTURE_MPLANE type buffers don't contain the flags that the
> originally queued OUTPUT_MPLANE type buffers have. This breaks compliance.
>
> Copy the source v4l2_buffer flags to the destination v4l2_buffer flags before
> they are dequed.
>
> Signed-off-by: Archit Taneja <archit@ti.com>
Reviewed-by: Hans Verkuil <hans.verkuil@cisco.com>
> ---
> drivers/media/platform/ti-vpe/vpe.c | 9 ++++-----
> 1 file changed, 4 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/media/platform/ti-vpe/vpe.c b/drivers/media/platform/ti-vpe/vpe.c
> index c884910..f7759e8 100644
> --- a/drivers/media/platform/ti-vpe/vpe.c
> +++ b/drivers/media/platform/ti-vpe/vpe.c
> @@ -1288,13 +1288,12 @@ static irqreturn_t vpe_irq(int irq_vpe, void *data)
> s_buf = &s_vb->v4l2_buf;
> d_buf = &d_vb->v4l2_buf;
>
> + d_buf->flags = s_buf->flags;
> +
> d_buf->timestamp = s_buf->timestamp;
> - d_buf->flags &= ~V4L2_BUF_FLAG_TSTAMP_SRC_MASK;
> - d_buf->flags |= s_buf->flags & V4L2_BUF_FLAG_TSTAMP_SRC_MASK;
> - if (s_buf->flags & V4L2_BUF_FLAG_TIMECODE) {
> - d_buf->flags |= V4L2_BUF_FLAG_TIMECODE;
> + if (s_buf->flags & V4L2_BUF_FLAG_TIMECODE)
> d_buf->timecode = s_buf->timecode;
> - }
> +
> d_buf->sequence = ctx->sequence;
>
> d_q_data = &ctx->q_data[Q_DATA_DST];
>
^ permalink raw reply [flat|nested] 88+ messages in thread
* [PATCH v4 00/14] v4l: ti-vpe: Some VPE fixes and enhancements
2014-03-11 8:33 ` [PATCH v3 00/14] v4l: ti-vpe: Some VPE fixes and enhancements Archit Taneja
` (13 preceding siblings ...)
2014-03-11 8:33 ` [PATCH v3 14/14] v4l: ti-vpe: retain v4l2_buffer flags for captured buffers Archit Taneja
@ 2014-03-13 11:44 ` Archit Taneja
2014-03-13 11:44 ` [PATCH v4 01/14] v4l: ti-vpe: Make sure in job_ready that we have the needed number of dst_bufs Archit Taneja
` (13 more replies)
14 siblings, 14 replies; 88+ messages in thread
From: Archit Taneja @ 2014-03-13 11:44 UTC (permalink / raw)
To: k.debski, hverkuil; +Cc: linux-media, linux-omap, Archit Taneja
This patch set mainly consists of minor fixes for the VPE driver. These fixes
ensure the following:
- The VPE module can be inserted and removed successively.
- Make sure that smaller resolutions like qcif work correctly.
- Prevent race condition between firmware loading and an open call to the v4l2
device.
- Prevent the possibility of output m2m queue not having sufficient 'ready'
buffers.
- Some VPDMA data descriptor fields weren't understood correctly before. They
are now used correctly.
The rest of the patches add some minor features like DMA buf support and
cropping/composing.
Reference branch:
git@github.com:boddob/linux.git vpe_for_315
Changes in v4:
- More legible code for selection API
- Stricter checking for target type vs crop type in g_selection
- Minor fix in patch 13/14, there was a logical bug in buf_prepare when
checking validity of top and bottom fields.
Changes in v3:
- improvements in selection API patch.
- querycap fixes for v4l2 compliance.
- v4l2_buffer 'field' and flags' fixes for compliance.
- fixes in try_fmt vpe_open for compliance.
- rename a IOMEM resource for better DT compatibility.
Changes in v2:
- selection API used instead of older cropping API.
- Typo fix.
- Some changes in patch 6/7 to support composing on the capture side of VPE.
Archit Taneja (14):
v4l: ti-vpe: Make sure in job_ready that we have the needed number of
dst_bufs
v4l: ti-vpe: register video device only when firmware is loaded
v4l: ti-vpe: Use video_device_release_empty
v4l: ti-vpe: Allow DMABUF buffer type support
v4l: ti-vpe: Allow usage of smaller images
v4l: ti-vpe: Fix some params in VPE data descriptors
v4l: ti-vpe: Add selection API in VPE driver
v4l: ti-vpe: Rename csc memory resource name
v4l: ti-vpe: report correct capabilities in querycap
v4l: ti-vpe: Use correct bus_info name for the device in querycap
v4l: ti-vpe: Fix initial configuration queue data
v4l: ti-vpe: zero out reserved fields in try_fmt
v4l: ti-vpe: Set correct field parameter for output and capture
buffers
v4l: ti-vpe: retain v4l2_buffer flags for captured buffers
drivers/media/platform/ti-vpe/csc.c | 2 +-
drivers/media/platform/ti-vpe/vpdma.c | 68 ++++++---
drivers/media/platform/ti-vpe/vpdma.h | 17 ++-
drivers/media/platform/ti-vpe/vpe.c | 272 ++++++++++++++++++++++++++++------
4 files changed, 290 insertions(+), 69 deletions(-)
--
1.8.3.2
^ permalink raw reply [flat|nested] 88+ messages in thread
* [PATCH v4 01/14] v4l: ti-vpe: Make sure in job_ready that we have the needed number of dst_bufs
2014-03-13 11:44 ` [PATCH v4 00/14] v4l: ti-vpe: Some VPE fixes and enhancements Archit Taneja
@ 2014-03-13 11:44 ` Archit Taneja
2014-03-13 14:38 ` Kamil Debski
2014-03-13 11:44 ` [PATCH v4 02/14] v4l: ti-vpe: register video device only when firmware is loaded Archit Taneja
` (12 subsequent siblings)
13 siblings, 1 reply; 88+ messages in thread
From: Archit Taneja @ 2014-03-13 11:44 UTC (permalink / raw)
To: k.debski, hverkuil; +Cc: linux-media, linux-omap, Archit Taneja
VPE has a ctrl parameter which decides how many mem to mem transactions the
active job from the job queue can perform.
The driver's job_ready() made sure that the number of ready source buffers are
sufficient for the job to execute successfully. But it didn't make sure if
there are sufficient ready destination buffers in the capture queue for the
VPE output.
If the time taken by VPE to process a single frame is really slow, then it's
possible that we don't need to imply such a restriction on the dst queue, but
really fast transactions(small resolution, no de-interlacing) may cause us to
hit the condition where we don't have any free buffers for the VPE to write on.
Add the extra check in job_ready() to make sure we have the sufficient amount
of destination buffers.
Signed-off-by: Archit Taneja <archit@ti.com>
---
drivers/media/platform/ti-vpe/vpe.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/drivers/media/platform/ti-vpe/vpe.c b/drivers/media/platform/ti-vpe/vpe.c
index 7a77a5b..f3143ac 100644
--- a/drivers/media/platform/ti-vpe/vpe.c
+++ b/drivers/media/platform/ti-vpe/vpe.c
@@ -887,6 +887,9 @@ static int job_ready(void *priv)
if (v4l2_m2m_num_src_bufs_ready(ctx->m2m_ctx) < needed)
return 0;
+ if (v4l2_m2m_num_dst_bufs_ready(ctx->m2m_ctx) < needed)
+ return 0;
+
return 1;
}
--
1.8.3.2
^ permalink raw reply related [flat|nested] 88+ messages in thread
* RE: [PATCH v4 01/14] v4l: ti-vpe: Make sure in job_ready that we have the needed number of dst_bufs
2014-03-13 11:44 ` [PATCH v4 01/14] v4l: ti-vpe: Make sure in job_ready that we have the needed number of dst_bufs Archit Taneja
@ 2014-03-13 14:38 ` Kamil Debski
0 siblings, 0 replies; 88+ messages in thread
From: Kamil Debski @ 2014-03-13 14:38 UTC (permalink / raw)
To: 'Archit Taneja', hverkuil; +Cc: linux-media, linux-omap
> From: Archit Taneja [mailto:archit@ti.com]
> Sent: Thursday, March 13, 2014 12:44 PM
>
> VPE has a ctrl parameter which decides how many mem to mem transactions
> the active job from the job queue can perform.
>
> The driver's job_ready() made sure that the number of ready source
> buffers are sufficient for the job to execute successfully. But it
> didn't make sure if there are sufficient ready destination buffers in
> the capture queue for the VPE output.
>
> If the time taken by VPE to process a single frame is really slow, then
> it's possible that we don't need to imply such a restriction on the dst
> queue, but really fast transactions(small resolution, no de-interlacing)
> may cause us to hit the condition where we don't have any free buffers
> for the VPE to write on.
>
> Add the extra check in job_ready() to make sure we have the sufficient
> amount of destination buffers.
>
> Signed-off-by: Archit Taneja <archit@ti.com>
Acked-by: Kamil Debski <k.debski@samsung.com>
> ---
> drivers/media/platform/ti-vpe/vpe.c | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/drivers/media/platform/ti-vpe/vpe.c
> b/drivers/media/platform/ti-vpe/vpe.c
> index 7a77a5b..f3143ac 100644
> --- a/drivers/media/platform/ti-vpe/vpe.c
> +++ b/drivers/media/platform/ti-vpe/vpe.c
> @@ -887,6 +887,9 @@ static int job_ready(void *priv)
> if (v4l2_m2m_num_src_bufs_ready(ctx->m2m_ctx) < needed)
> return 0;
>
> + if (v4l2_m2m_num_dst_bufs_ready(ctx->m2m_ctx) < needed)
> + return 0;
> +
> return 1;
> }
>
> --
> 1.8.3.2
^ permalink raw reply [flat|nested] 88+ messages in thread
* [PATCH v4 02/14] v4l: ti-vpe: register video device only when firmware is loaded
2014-03-13 11:44 ` [PATCH v4 00/14] v4l: ti-vpe: Some VPE fixes and enhancements Archit Taneja
2014-03-13 11:44 ` [PATCH v4 01/14] v4l: ti-vpe: Make sure in job_ready that we have the needed number of dst_bufs Archit Taneja
@ 2014-03-13 11:44 ` Archit Taneja
2014-03-13 11:44 ` [PATCH v4 03/14] v4l: ti-vpe: Use video_device_release_empty Archit Taneja
` (11 subsequent siblings)
13 siblings, 0 replies; 88+ messages in thread
From: Archit Taneja @ 2014-03-13 11:44 UTC (permalink / raw)
To: k.debski, hverkuil; +Cc: linux-media, linux-omap, Archit Taneja
vpe fops(vpe_open in particular) should be called only when VPDMA firmware
is loaded. File operations on the video device are possible the moment it is
registered.
Currently, we register the video device for VPE at driver probe, after calling
a vpdma helper to initialize VPDMA and load firmware. This function is
non-blocking(it calls request_firmware_nowait()), and doesn't ensure that the
firmware is actually loaded when it returns.
We remove the device registration from vpe probe, and move it to a callback
provided by the vpe driver to the vpdma library, through vpdma_create().
The ready field in vpdma_data is no longer needed since we always have firmware
loaded before the device is registered.
A minor problem with this approach is that if the video_register_device
fails(which doesn't really happen), the vpe platform device would be registered.
however, there won't be any v4l2 device corresponding to it.
Signed-off-by: Archit Taneja <archit@ti.com>
---
drivers/media/platform/ti-vpe/vpdma.c | 8 +++--
drivers/media/platform/ti-vpe/vpdma.h | 7 +++--
drivers/media/platform/ti-vpe/vpe.c | 55 ++++++++++++++++++++---------------
3 files changed, 41 insertions(+), 29 deletions(-)
diff --git a/drivers/media/platform/ti-vpe/vpdma.c b/drivers/media/platform/ti-vpe/vpdma.c
index e8175e7..73dd38e 100644
--- a/drivers/media/platform/ti-vpe/vpdma.c
+++ b/drivers/media/platform/ti-vpe/vpdma.c
@@ -781,7 +781,7 @@ static void vpdma_firmware_cb(const struct firmware *f, void *context)
/* already initialized */
if (read_field_reg(vpdma, VPDMA_LIST_ATTR, VPDMA_LIST_RDY_MASK,
VPDMA_LIST_RDY_SHFT)) {
- vpdma->ready = true;
+ vpdma->cb(vpdma->pdev);
return;
}
@@ -811,7 +811,7 @@ static void vpdma_firmware_cb(const struct firmware *f, void *context)
goto free_buf;
}
- vpdma->ready = true;
+ vpdma->cb(vpdma->pdev);
free_buf:
vpdma_unmap_desc_buf(vpdma, &fw_dma_buf);
@@ -839,7 +839,8 @@ static int vpdma_load_firmware(struct vpdma_data *vpdma)
return 0;
}
-struct vpdma_data *vpdma_create(struct platform_device *pdev)
+struct vpdma_data *vpdma_create(struct platform_device *pdev,
+ void (*cb)(struct platform_device *pdev))
{
struct resource *res;
struct vpdma_data *vpdma;
@@ -854,6 +855,7 @@ struct vpdma_data *vpdma_create(struct platform_device *pdev)
}
vpdma->pdev = pdev;
+ vpdma->cb = cb;
res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "vpdma");
if (res == NULL) {
diff --git a/drivers/media/platform/ti-vpe/vpdma.h b/drivers/media/platform/ti-vpe/vpdma.h
index cf40f11..bf5f8bb 100644
--- a/drivers/media/platform/ti-vpe/vpdma.h
+++ b/drivers/media/platform/ti-vpe/vpdma.h
@@ -35,8 +35,8 @@ struct vpdma_data {
struct platform_device *pdev;
- /* tells whether vpdma firmware is loaded or not */
- bool ready;
+ /* callback to VPE driver when the firmware is loaded */
+ void (*cb)(struct platform_device *pdev);
};
enum vpdma_data_format_type {
@@ -208,6 +208,7 @@ void vpdma_set_frame_start_event(struct vpdma_data *vpdma,
void vpdma_dump_regs(struct vpdma_data *vpdma);
/* initialize vpdma, passed with VPE's platform device pointer */
-struct vpdma_data *vpdma_create(struct platform_device *pdev);
+struct vpdma_data *vpdma_create(struct platform_device *pdev,
+ void (*cb)(struct platform_device *pdev));
#endif
diff --git a/drivers/media/platform/ti-vpe/vpe.c b/drivers/media/platform/ti-vpe/vpe.c
index f3143ac..f1eae67 100644
--- a/drivers/media/platform/ti-vpe/vpe.c
+++ b/drivers/media/platform/ti-vpe/vpe.c
@@ -1817,11 +1817,6 @@ static int vpe_open(struct file *file)
vpe_dbg(dev, "vpe_open\n");
- if (!dev->vpdma->ready) {
- vpe_err(dev, "vpdma firmware not loaded\n");
- return -ENODEV;
- }
-
ctx = kzalloc(sizeof(*ctx), GFP_KERNEL);
if (!ctx)
return -ENOMEM;
@@ -2039,10 +2034,40 @@ static void vpe_runtime_put(struct platform_device *pdev)
WARN_ON(r < 0 && r != -ENOSYS);
}
+static void vpe_fw_cb(struct platform_device *pdev)
+{
+ struct vpe_dev *dev = platform_get_drvdata(pdev);
+ struct video_device *vfd;
+ int ret;
+
+ vfd = &dev->vfd;
+ *vfd = vpe_videodev;
+ vfd->lock = &dev->dev_mutex;
+ vfd->v4l2_dev = &dev->v4l2_dev;
+
+ ret = video_register_device(vfd, VFL_TYPE_GRABBER, 0);
+ if (ret) {
+ vpe_err(dev, "Failed to register video device\n");
+
+ vpe_set_clock_enable(dev, 0);
+ vpe_runtime_put(pdev);
+ pm_runtime_disable(&pdev->dev);
+ v4l2_m2m_release(dev->m2m_dev);
+ vb2_dma_contig_cleanup_ctx(dev->alloc_ctx);
+ v4l2_device_unregister(&dev->v4l2_dev);
+
+ return;
+ }
+
+ video_set_drvdata(vfd, dev);
+ snprintf(vfd->name, sizeof(vfd->name), "%s", vpe_videodev.name);
+ dev_info(dev->v4l2_dev.dev, "Device registered as /dev/video%d\n",
+ vfd->num);
+}
+
static int vpe_probe(struct platform_device *pdev)
{
struct vpe_dev *dev;
- struct video_device *vfd;
int ret, irq, func;
dev = devm_kzalloc(&pdev->dev, sizeof(*dev), GFP_KERNEL);
@@ -2123,28 +2148,12 @@ static int vpe_probe(struct platform_device *pdev)
goto runtime_put;
}
- dev->vpdma = vpdma_create(pdev);
+ dev->vpdma = vpdma_create(pdev, vpe_fw_cb);
if (IS_ERR(dev->vpdma)) {
ret = PTR_ERR(dev->vpdma);
goto runtime_put;
}
- vfd = &dev->vfd;
- *vfd = vpe_videodev;
- vfd->lock = &dev->dev_mutex;
- vfd->v4l2_dev = &dev->v4l2_dev;
-
- ret = video_register_device(vfd, VFL_TYPE_GRABBER, 0);
- if (ret) {
- vpe_err(dev, "Failed to register video device\n");
- goto runtime_put;
- }
-
- video_set_drvdata(vfd, dev);
- snprintf(vfd->name, sizeof(vfd->name), "%s", vpe_videodev.name);
- dev_info(dev->v4l2_dev.dev, "Device registered as /dev/video%d\n",
- vfd->num);
-
return 0;
runtime_put:
--
1.8.3.2
^ permalink raw reply related [flat|nested] 88+ messages in thread
* [PATCH v4 03/14] v4l: ti-vpe: Use video_device_release_empty
2014-03-13 11:44 ` [PATCH v4 00/14] v4l: ti-vpe: Some VPE fixes and enhancements Archit Taneja
2014-03-13 11:44 ` [PATCH v4 01/14] v4l: ti-vpe: Make sure in job_ready that we have the needed number of dst_bufs Archit Taneja
2014-03-13 11:44 ` [PATCH v4 02/14] v4l: ti-vpe: register video device only when firmware is loaded Archit Taneja
@ 2014-03-13 11:44 ` Archit Taneja
2014-03-13 11:44 ` [PATCH v4 04/14] v4l: ti-vpe: Allow DMABUF buffer type support Archit Taneja
` (10 subsequent siblings)
13 siblings, 0 replies; 88+ messages in thread
From: Archit Taneja @ 2014-03-13 11:44 UTC (permalink / raw)
To: k.debski, hverkuil; +Cc: linux-media, linux-omap, Archit Taneja
The video_device struct is currently embedded in the driver data struct vpe_dev.
A vpe_dev instance is allocated by the driver, and the memory for the vfd is a
part of this struct.
The v4l2 core, however, manages the removal of the vfd region, through the
video_device's .release() op, which currently is the helper
video_device_release. This causes memory corruption, and leads to issues when
we try to re-insert the vpe module.
Use the video_device_release_empty helper function instead.
Reviewed-by: Hans Verkuil <hans.verkuil@cisco.com>
Signed-off-by: Archit Taneja <archit@ti.com>
---
drivers/media/platform/ti-vpe/vpe.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/media/platform/ti-vpe/vpe.c b/drivers/media/platform/ti-vpe/vpe.c
index f1eae67..0363df6 100644
--- a/drivers/media/platform/ti-vpe/vpe.c
+++ b/drivers/media/platform/ti-vpe/vpe.c
@@ -2000,7 +2000,7 @@ static struct video_device vpe_videodev = {
.fops = &vpe_fops,
.ioctl_ops = &vpe_ioctl_ops,
.minor = -1,
- .release = video_device_release,
+ .release = video_device_release_empty,
.vfl_dir = VFL_DIR_M2M,
};
--
1.8.3.2
^ permalink raw reply related [flat|nested] 88+ messages in thread
* [PATCH v4 04/14] v4l: ti-vpe: Allow DMABUF buffer type support
2014-03-13 11:44 ` [PATCH v4 00/14] v4l: ti-vpe: Some VPE fixes and enhancements Archit Taneja
` (2 preceding siblings ...)
2014-03-13 11:44 ` [PATCH v4 03/14] v4l: ti-vpe: Use video_device_release_empty Archit Taneja
@ 2014-03-13 11:44 ` Archit Taneja
2014-03-13 11:44 ` [PATCH v4 05/14] v4l: ti-vpe: Allow usage of smaller images Archit Taneja
` (9 subsequent siblings)
13 siblings, 0 replies; 88+ messages in thread
From: Archit Taneja @ 2014-03-13 11:44 UTC (permalink / raw)
To: k.debski, hverkuil; +Cc: linux-media, linux-omap, Archit Taneja
For OMAP and DRA7x, we generally allocate video and graphics buffers through
omapdrm since the corresponding omap-gem driver provides DMM-Tiler backed
contiguous buffers. omapdrm is a dma-buf exporter. These buffers are used by
other drivers in the video pipeline.
Add VB2_DMABUF flag to the io_modes of the vb2 output and capture queues. This
allows the driver to import dma shared buffers.
Reviewed-by: Hans Verkuil <hans.verkuil@cisco.com>
Signed-off-by: Archit Taneja <archit@ti.com>
---
drivers/media/platform/ti-vpe/vpe.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/media/platform/ti-vpe/vpe.c b/drivers/media/platform/ti-vpe/vpe.c
index 0363df6..0e7573a 100644
--- a/drivers/media/platform/ti-vpe/vpe.c
+++ b/drivers/media/platform/ti-vpe/vpe.c
@@ -1770,7 +1770,7 @@ static int queue_init(void *priv, struct vb2_queue *src_vq,
memset(src_vq, 0, sizeof(*src_vq));
src_vq->type = V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE;
- src_vq->io_modes = VB2_MMAP;
+ src_vq->io_modes = VB2_MMAP | VB2_DMABUF;
src_vq->drv_priv = ctx;
src_vq->buf_struct_size = sizeof(struct v4l2_m2m_buffer);
src_vq->ops = &vpe_qops;
@@ -1783,7 +1783,7 @@ static int queue_init(void *priv, struct vb2_queue *src_vq,
memset(dst_vq, 0, sizeof(*dst_vq));
dst_vq->type = V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE;
- dst_vq->io_modes = VB2_MMAP;
+ dst_vq->io_modes = VB2_MMAP | VB2_DMABUF;
dst_vq->drv_priv = ctx;
dst_vq->buf_struct_size = sizeof(struct v4l2_m2m_buffer);
dst_vq->ops = &vpe_qops;
--
1.8.3.2
^ permalink raw reply related [flat|nested] 88+ messages in thread
* [PATCH v4 05/14] v4l: ti-vpe: Allow usage of smaller images
2014-03-13 11:44 ` [PATCH v4 00/14] v4l: ti-vpe: Some VPE fixes and enhancements Archit Taneja
` (3 preceding siblings ...)
2014-03-13 11:44 ` [PATCH v4 04/14] v4l: ti-vpe: Allow DMABUF buffer type support Archit Taneja
@ 2014-03-13 11:44 ` Archit Taneja
2014-03-13 11:44 ` [PATCH v4 06/14] v4l: ti-vpe: Fix some params in VPE data descriptors Archit Taneja
` (8 subsequent siblings)
13 siblings, 0 replies; 88+ messages in thread
From: Archit Taneja @ 2014-03-13 11:44 UTC (permalink / raw)
To: k.debski, hverkuil; +Cc: linux-media, linux-omap, Archit Taneja
The minimum width and height for VPE input/output was kept as 128 pixels. VPE
doesn't have a constraint on the image height, it requires the image width to
be at least 16 bytes.
Change the minimum supported dimensions to 32x32. This allows us to de-interlace
qcif content. A smaller image size than 32x32 didn't make much sense, so stopped
at this.
Reviewed-by: Hans Verkuil <hans.verkuil@cisco.com>
Signed-off-by: Archit Taneja <archit@ti.com>
---
drivers/media/platform/ti-vpe/vpe.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/media/platform/ti-vpe/vpe.c b/drivers/media/platform/ti-vpe/vpe.c
index 0e7573a..dbdc338 100644
--- a/drivers/media/platform/ti-vpe/vpe.c
+++ b/drivers/media/platform/ti-vpe/vpe.c
@@ -49,8 +49,8 @@
#define VPE_MODULE_NAME "vpe"
/* minimum and maximum frame sizes */
-#define MIN_W 128
-#define MIN_H 128
+#define MIN_W 32
+#define MIN_H 32
#define MAX_W 1920
#define MAX_H 1080
--
1.8.3.2
^ permalink raw reply related [flat|nested] 88+ messages in thread
* [PATCH v4 06/14] v4l: ti-vpe: Fix some params in VPE data descriptors
2014-03-13 11:44 ` [PATCH v4 00/14] v4l: ti-vpe: Some VPE fixes and enhancements Archit Taneja
` (4 preceding siblings ...)
2014-03-13 11:44 ` [PATCH v4 05/14] v4l: ti-vpe: Allow usage of smaller images Archit Taneja
@ 2014-03-13 11:44 ` Archit Taneja
2014-03-13 15:01 ` Kamil Debski
2014-03-13 11:44 ` [PATCH v4 07/14] v4l: ti-vpe: Add selection API in VPE driver Archit Taneja
` (7 subsequent siblings)
13 siblings, 1 reply; 88+ messages in thread
From: Archit Taneja @ 2014-03-13 11:44 UTC (permalink / raw)
To: k.debski, hverkuil; +Cc: linux-media, linux-omap, Archit Taneja
Some parameters of the VPE descriptors were understood incorrectly. They are now
fixed. The fixes are explained as follows:
- When adding an inbound data descriptor to the VPDMA descriptor list, we intend
to use c_rect as the cropped region fetched by VPDMA. Therefore, c_rect->width
shouldn't be used to calculate the line stride, the original image width
should be used for that. We add a 'width' argument which gives the buffer
width in memory.
- frame_width and frame_height describe the complete width and height of the
client to which the channel is connected. If there are multiple channels
fetching data and providing to the same client, the above 2 arguments should
be the width and height of the region covered by all the channels. In the case
where there is only one channel providing pixel data to the client
(like in VPE), frame_width and frame_height should be the cropped width and
cropped height respectively. The calculation of these params is done in the
vpe driver now.
- start_h and start_v is also used in the case of multiple channels to describe
where each channel should start filling pixel data. We don't use this in VPE,
and pass 0s to the vpdma_add_in_dtd() helper.
- Some minor changes are made to the vpdma_add_out_dtd() helper. The c_rect
param is used for specifying the 'composition' target, and 'width' is added
to calculate the line stride.
Signed-off-by: Archit Taneja <archit@ti.com>
---
drivers/media/platform/ti-vpe/vpdma.c | 60 +++++++++++++++++++++++++++--------
drivers/media/platform/ti-vpe/vpdma.h | 10 +++---
drivers/media/platform/ti-vpe/vpe.c | 18 +++++++----
3 files changed, 64 insertions(+), 24 deletions(-)
diff --git a/drivers/media/platform/ti-vpe/vpdma.c b/drivers/media/platform/ti-vpe/vpdma.c
index 73dd38e..a51a013 100644
--- a/drivers/media/platform/ti-vpe/vpdma.c
+++ b/drivers/media/platform/ti-vpe/vpdma.c
@@ -614,8 +614,17 @@ static void dump_dtd(struct vpdma_dtd *dtd)
/*
* append an outbound data transfer descriptor to the given descriptor list,
* this sets up a 'client to memory' VPDMA transfer for the given VPDMA channel
+ *
+ * @list: vpdma desc list to which we add this decriptor
+ * @width: width of the image in pixels in memory
+ * @c_rect: compose params of output image
+ * @fmt: vpdma data format of the buffer
+ * dma_addr: dma address as seen by VPDMA
+ * chan: VPDMA channel
+ * flags: VPDMA flags to configure some descriptor fileds
*/
-void vpdma_add_out_dtd(struct vpdma_desc_list *list, struct v4l2_rect *c_rect,
+void vpdma_add_out_dtd(struct vpdma_desc_list *list, int width,
+ const struct v4l2_rect *c_rect,
const struct vpdma_data_format *fmt, dma_addr_t dma_addr,
enum vpdma_channel chan, u32 flags)
{
@@ -623,6 +632,7 @@ void vpdma_add_out_dtd(struct vpdma_desc_list *list, struct v4l2_rect *c_rect,
int field = 0;
int notify = 1;
int channel, next_chan;
+ struct v4l2_rect rect = *c_rect;
int depth = fmt->depth;
int stride;
struct vpdma_dtd *dtd;
@@ -630,11 +640,15 @@ void vpdma_add_out_dtd(struct vpdma_desc_list *list, struct v4l2_rect *c_rect,
channel = next_chan = chan_info[chan].num;
if (fmt->type == VPDMA_DATA_FMT_TYPE_YUV &&
- fmt->data_type == DATA_TYPE_C420)
+ fmt->data_type == DATA_TYPE_C420) {
+ rect.height >>= 1;
+ rect.top >>= 1;
depth = 8;
+ }
- stride = ALIGN((depth * c_rect->width) >> 3, VPDMA_STRIDE_ALIGN);
- dma_addr += (c_rect->left * depth) >> 3;
+ stride = ALIGN((depth * width) >> 3, VPDMA_STRIDE_ALIGN);
+
+ dma_addr += rect.top * stride + (rect.left * depth >> 3);
dtd = list->next;
WARN_ON((void *)(dtd + 1) > (list->buf.addr + list->buf.size));
@@ -664,31 +678,48 @@ void vpdma_add_out_dtd(struct vpdma_desc_list *list, struct v4l2_rect *c_rect,
/*
* append an inbound data transfer descriptor to the given descriptor list,
* this sets up a 'memory to client' VPDMA transfer for the given VPDMA channel
+ *
+ * @list: vpdma desc list to which we add this decriptor
+ * @width: width of the image in pixels in memory(not the cropped width)
+ * @c_rect: crop params of input image
+ * @fmt: vpdma data format of the buffer
+ * dma_addr: dma address as seen by VPDMA
+ * chan: VPDMA channel
+ * field: top or bottom field info of the input image
+ * flags: VPDMA flags to configure some descriptor fileds
+ * frame_width/height: the complete width/height of the image presented to the
+ * client (this makes sense when multiple channels are
+ * connected to the same client, forming a larger frame)
+ * start_h, start_v: position where the given channel starts providing pixel
+ * data to the client (makes sense when multiple channels
+ * contribute to the client)
*/
-void vpdma_add_in_dtd(struct vpdma_desc_list *list, int frame_width,
- int frame_height, struct v4l2_rect *c_rect,
+void vpdma_add_in_dtd(struct vpdma_desc_list *list, int width,
+ const struct v4l2_rect *c_rect,
const struct vpdma_data_format *fmt, dma_addr_t dma_addr,
- enum vpdma_channel chan, int field, u32 flags)
+ enum vpdma_channel chan, int field, u32 flags, int frame_width,
+ int frame_height, int start_h, int start_v)
{
int priority = 0;
int notify = 1;
int depth = fmt->depth;
int channel, next_chan;
+ struct v4l2_rect rect = *c_rect;
int stride;
- int height = c_rect->height;
struct vpdma_dtd *dtd;
channel = next_chan = chan_info[chan].num;
if (fmt->type == VPDMA_DATA_FMT_TYPE_YUV &&
fmt->data_type == DATA_TYPE_C420) {
- height >>= 1;
- frame_height >>= 1;
+ rect.height >>= 1;
+ rect.top >>= 1;
depth = 8;
}
- stride = ALIGN((depth * c_rect->width) >> 3, VPDMA_STRIDE_ALIGN);
- dma_addr += (c_rect->left * depth) >> 3;
+ stride = ALIGN((depth * width) >> 3, VPDMA_STRIDE_ALIGN);
+
+ dma_addr += rect.top * stride + (rect.left * depth >> 3);
dtd = list->next;
WARN_ON((void *)(dtd + 1) > (list->buf.addr + list->buf.size));
@@ -701,13 +732,14 @@ void vpdma_add_in_dtd(struct vpdma_desc_list *list, int frame_width,
!!(flags & VPDMA_DATA_ODD_LINE_SKIP),
stride);
- dtd->xfer_length_height = dtd_xfer_length_height(c_rect->width, height);
+ dtd->xfer_length_height = dtd_xfer_length_height(rect.width,
+ rect.height);
dtd->start_addr = (u32) dma_addr;
dtd->pkt_ctl = dtd_pkt_ctl(!!(flags & VPDMA_DATA_MODE_TILED),
DTD_DIR_IN, channel, priority, next_chan);
dtd->frame_width_height = dtd_frame_width_height(frame_width,
frame_height);
- dtd->start_h_v = dtd_start_h_v(c_rect->left, c_rect->top);
+ dtd->start_h_v = dtd_start_h_v(start_h, start_v);
dtd->client_attr0 = 0;
dtd->client_attr1 = 0;
diff --git a/drivers/media/platform/ti-vpe/vpdma.h b/drivers/media/platform/ti-vpe/vpdma.h
index bf5f8bb..2bd8fb0 100644
--- a/drivers/media/platform/ti-vpe/vpdma.h
+++ b/drivers/media/platform/ti-vpe/vpdma.h
@@ -186,13 +186,15 @@ void vpdma_add_cfd_adb(struct vpdma_desc_list *list, int client,
struct vpdma_buf *adb);
void vpdma_add_sync_on_channel_ctd(struct vpdma_desc_list *list,
enum vpdma_channel chan);
-void vpdma_add_out_dtd(struct vpdma_desc_list *list, struct v4l2_rect *c_rect,
+void vpdma_add_out_dtd(struct vpdma_desc_list *list, int width,
+ const struct v4l2_rect *c_rect,
const struct vpdma_data_format *fmt, dma_addr_t dma_addr,
enum vpdma_channel chan, u32 flags);
-void vpdma_add_in_dtd(struct vpdma_desc_list *list, int frame_width,
- int frame_height, struct v4l2_rect *c_rect,
+void vpdma_add_in_dtd(struct vpdma_desc_list *list, int width,
+ const struct v4l2_rect *c_rect,
const struct vpdma_data_format *fmt, dma_addr_t dma_addr,
- enum vpdma_channel chan, int field, u32 flags);
+ enum vpdma_channel chan, int field, u32 flags, int frame_width,
+ int frame_height, int start_h, int start_v);
/* vpdma list interrupt management */
void vpdma_enable_list_complete_irq(struct vpdma_data *vpdma, int list_num,
diff --git a/drivers/media/platform/ti-vpe/vpe.c b/drivers/media/platform/ti-vpe/vpe.c
index dbdc338..ece9b96 100644
--- a/drivers/media/platform/ti-vpe/vpe.c
+++ b/drivers/media/platform/ti-vpe/vpe.c
@@ -986,7 +986,6 @@ static void add_out_dtd(struct vpe_ctx *ctx, int port)
struct vpe_q_data *q_data = &ctx->q_data[Q_DATA_DST];
const struct vpe_port_data *p_data = &port_data[port];
struct vb2_buffer *vb = ctx->dst_vb;
- struct v4l2_rect *c_rect = &q_data->c_rect;
struct vpe_fmt *fmt = q_data->fmt;
const struct vpdma_data_format *vpdma_fmt;
int mv_buf_selector = !ctx->src_mv_buf_selector;
@@ -1015,8 +1014,8 @@ static void add_out_dtd(struct vpe_ctx *ctx, int port)
if (q_data->flags & Q_DATA_MODE_TILED)
flags |= VPDMA_DATA_MODE_TILED;
- vpdma_add_out_dtd(&ctx->desc_list, c_rect, vpdma_fmt, dma_addr,
- p_data->channel, flags);
+ vpdma_add_out_dtd(&ctx->desc_list, q_data->width, &q_data->c_rect,
+ vpdma_fmt, dma_addr, p_data->channel, flags);
}
static void add_in_dtd(struct vpe_ctx *ctx, int port)
@@ -1024,11 +1023,11 @@ static void add_in_dtd(struct vpe_ctx *ctx, int port)
struct vpe_q_data *q_data = &ctx->q_data[Q_DATA_SRC];
const struct vpe_port_data *p_data = &port_data[port];
struct vb2_buffer *vb = ctx->src_vbs[p_data->vb_index];
- struct v4l2_rect *c_rect = &q_data->c_rect;
struct vpe_fmt *fmt = q_data->fmt;
const struct vpdma_data_format *vpdma_fmt;
int mv_buf_selector = ctx->src_mv_buf_selector;
int field = vb->v4l2_buf.field == V4L2_FIELD_BOTTOM;
+ int frame_width, frame_height;
dma_addr_t dma_addr;
u32 flags = 0;
@@ -1055,8 +1054,15 @@ static void add_in_dtd(struct vpe_ctx *ctx, int port)
if (q_data->flags & Q_DATA_MODE_TILED)
flags |= VPDMA_DATA_MODE_TILED;
- vpdma_add_in_dtd(&ctx->desc_list, q_data->width, q_data->height,
- c_rect, vpdma_fmt, dma_addr, p_data->channel, field, flags);
+ frame_width = q_data->c_rect.width;
+ frame_height = q_data->c_rect.height;
+
+ if (p_data->vb_part && fmt->fourcc == V4L2_PIX_FMT_NV12)
+ frame_height /= 2;
+
+ vpdma_add_in_dtd(&ctx->desc_list, q_data->width, &q_data->c_rect,
+ vpdma_fmt, dma_addr, p_data->channel, field, flags, frame_width,
+ frame_height, 0, 0);
}
/*
--
1.8.3.2
^ permalink raw reply related [flat|nested] 88+ messages in thread
* RE: [PATCH v4 06/14] v4l: ti-vpe: Fix some params in VPE data descriptors
2014-03-13 11:44 ` [PATCH v4 06/14] v4l: ti-vpe: Fix some params in VPE data descriptors Archit Taneja
@ 2014-03-13 15:01 ` Kamil Debski
0 siblings, 0 replies; 88+ messages in thread
From: Kamil Debski @ 2014-03-13 15:01 UTC (permalink / raw)
To: 'Archit Taneja', hverkuil; +Cc: linux-media, linux-omap
Hi,
> From: Archit Taneja [mailto:archit@ti.com]
> Sent: Thursday, March 13, 2014 12:44 PM
> To: k.debski@samsung.com; hverkuil@xs4all.nl
> Cc: linux-media@vger.kernel.org; linux-omap@vger.kernel.org; Archit
> Taneja
> Subject: [PATCH v4 06/14] v4l: ti-vpe: Fix some params in VPE data
> descriptors
>
> Some parameters of the VPE descriptors were understood incorrectly.
> They are now fixed. The fixes are explained as follows:
>
> - When adding an inbound data descriptor to the VPDMA descriptor list,
> we intend
> to use c_rect as the cropped region fetched by VPDMA. Therefore,
> c_rect->width
> shouldn't be used to calculate the line stride, the original image
> width
> should be used for that. We add a 'width' argument which gives the
> buffer
> width in memory.
>
> - frame_width and frame_height describe the complete width and height
> of the
> client to which the channel is connected. If there are multiple
> channels
> fetching data and providing to the same client, the above 2 arguments
> should
> be the width and height of the region covered by all the channels. In
> the case
> where there is only one channel providing pixel data to the client
> (like in VPE), frame_width and frame_height should be the cropped
> width and
> cropped height respectively. The calculation of these params is done
> in the
> vpe driver now.
>
> - start_h and start_v is also used in the case of multiple channels to
> describe
> where each channel should start filling pixel data. We don't use this
> in VPE,
> and pass 0s to the vpdma_add_in_dtd() helper.
>
> - Some minor changes are made to the vpdma_add_out_dtd() helper. The
> c_rect
> param is used for specifying the 'composition' target, and 'width'
> is added
> to calculate the line stride.
This patch looks ok. Passes checkpatch and compiles. I can't say much more
as I have limited knowledge of the internals of VPE and don't have the
hardware.
> Signed-off-by: Archit Taneja <archit@ti.com>
Acked-by: Kamil Debski <k.debski@samsung.com>
Best wishes,
--
Kamil Debski
Samsung R&D Institute Poland
> ---
> drivers/media/platform/ti-vpe/vpdma.c | 60
> +++++++++++++++++++++++++++--------
> drivers/media/platform/ti-vpe/vpdma.h | 10 +++---
> drivers/media/platform/ti-vpe/vpe.c | 18 +++++++----
> 3 files changed, 64 insertions(+), 24 deletions(-)
>
> diff --git a/drivers/media/platform/ti-vpe/vpdma.c
> b/drivers/media/platform/ti-vpe/vpdma.c
> index 73dd38e..a51a013 100644
> --- a/drivers/media/platform/ti-vpe/vpdma.c
> +++ b/drivers/media/platform/ti-vpe/vpdma.c
> @@ -614,8 +614,17 @@ static void dump_dtd(struct vpdma_dtd *dtd)
> /*
> * append an outbound data transfer descriptor to the given descriptor
> list,
> * this sets up a 'client to memory' VPDMA transfer for the given
> VPDMA channel
> + *
> + * @list: vpdma desc list to which we add this decriptor
> + * @width: width of the image in pixels in memory
> + * @c_rect: compose params of output image
> + * @fmt: vpdma data format of the buffer
> + * dma_addr: dma address as seen by VPDMA
> + * chan: VPDMA channel
> + * flags: VPDMA flags to configure some descriptor fileds
> */
> -void vpdma_add_out_dtd(struct vpdma_desc_list *list, struct v4l2_rect
> *c_rect,
> +void vpdma_add_out_dtd(struct vpdma_desc_list *list, int width,
> + const struct v4l2_rect *c_rect,
> const struct vpdma_data_format *fmt, dma_addr_t dma_addr,
> enum vpdma_channel chan, u32 flags)
> {
> @@ -623,6 +632,7 @@ void vpdma_add_out_dtd(struct vpdma_desc_list *list,
> struct v4l2_rect *c_rect,
> int field = 0;
> int notify = 1;
> int channel, next_chan;
> + struct v4l2_rect rect = *c_rect;
> int depth = fmt->depth;
> int stride;
> struct vpdma_dtd *dtd;
> @@ -630,11 +640,15 @@ void vpdma_add_out_dtd(struct vpdma_desc_list
> *list, struct v4l2_rect *c_rect,
> channel = next_chan = chan_info[chan].num;
>
> if (fmt->type == VPDMA_DATA_FMT_TYPE_YUV &&
> - fmt->data_type == DATA_TYPE_C420)
> + fmt->data_type == DATA_TYPE_C420) {
> + rect.height >>= 1;
> + rect.top >>= 1;
> depth = 8;
> + }
>
> - stride = ALIGN((depth * c_rect->width) >> 3, VPDMA_STRIDE_ALIGN);
> - dma_addr += (c_rect->left * depth) >> 3;
> + stride = ALIGN((depth * width) >> 3, VPDMA_STRIDE_ALIGN);
> +
> + dma_addr += rect.top * stride + (rect.left * depth >> 3);
>
> dtd = list->next;
> WARN_ON((void *)(dtd + 1) > (list->buf.addr + list->buf.size));
> @@ -664,31 +678,48 @@ void vpdma_add_out_dtd(struct vpdma_desc_list
> *list, struct v4l2_rect *c_rect,
> /*
> * append an inbound data transfer descriptor to the given descriptor
> list,
> * this sets up a 'memory to client' VPDMA transfer for the given
> VPDMA channel
> + *
> + * @list: vpdma desc list to which we add this decriptor
> + * @width: width of the image in pixels in memory(not the cropped
> + width)
> + * @c_rect: crop params of input image
> + * @fmt: vpdma data format of the buffer
> + * dma_addr: dma address as seen by VPDMA
> + * chan: VPDMA channel
> + * field: top or bottom field info of the input image
> + * flags: VPDMA flags to configure some descriptor fileds
> + * frame_width/height: the complete width/height of the image
> presented to the
> + * client (this makes sense when multiple channels are
> + * connected to the same client, forming a larger
frame)
> + * start_h, start_v: position where the given channel starts providing
> pixel
> + * data to the client (makes sense when multiple
> channels
> + * contribute to the client)
> */
> -void vpdma_add_in_dtd(struct vpdma_desc_list *list, int frame_width,
> - int frame_height, struct v4l2_rect *c_rect,
> +void vpdma_add_in_dtd(struct vpdma_desc_list *list, int width,
> + const struct v4l2_rect *c_rect,
> const struct vpdma_data_format *fmt, dma_addr_t dma_addr,
> - enum vpdma_channel chan, int field, u32 flags)
> + enum vpdma_channel chan, int field, u32 flags, int
> frame_width,
> + int frame_height, int start_h, int start_v)
> {
> int priority = 0;
> int notify = 1;
> int depth = fmt->depth;
> int channel, next_chan;
> + struct v4l2_rect rect = *c_rect;
> int stride;
> - int height = c_rect->height;
> struct vpdma_dtd *dtd;
>
> channel = next_chan = chan_info[chan].num;
>
> if (fmt->type == VPDMA_DATA_FMT_TYPE_YUV &&
> fmt->data_type == DATA_TYPE_C420) {
> - height >>= 1;
> - frame_height >>= 1;
> + rect.height >>= 1;
> + rect.top >>= 1;
> depth = 8;
> }
>
> - stride = ALIGN((depth * c_rect->width) >> 3, VPDMA_STRIDE_ALIGN);
> - dma_addr += (c_rect->left * depth) >> 3;
> + stride = ALIGN((depth * width) >> 3, VPDMA_STRIDE_ALIGN);
> +
> + dma_addr += rect.top * stride + (rect.left * depth >> 3);
>
> dtd = list->next;
> WARN_ON((void *)(dtd + 1) > (list->buf.addr + list->buf.size));
> @@ -701,13 +732,14 @@ void vpdma_add_in_dtd(struct vpdma_desc_list
> *list, int frame_width,
> !!(flags &
VPDMA_DATA_ODD_LINE_SKIP),
> stride);
>
> - dtd->xfer_length_height = dtd_xfer_length_height(c_rect->width,
> height);
> + dtd->xfer_length_height = dtd_xfer_length_height(rect.width,
> + rect.height);
> dtd->start_addr = (u32) dma_addr;
> dtd->pkt_ctl = dtd_pkt_ctl(!!(flags & VPDMA_DATA_MODE_TILED),
> DTD_DIR_IN, channel, priority, next_chan);
> dtd->frame_width_height = dtd_frame_width_height(frame_width,
> frame_height);
> - dtd->start_h_v = dtd_start_h_v(c_rect->left, c_rect->top);
> + dtd->start_h_v = dtd_start_h_v(start_h, start_v);
> dtd->client_attr0 = 0;
> dtd->client_attr1 = 0;
>
> diff --git a/drivers/media/platform/ti-vpe/vpdma.h
> b/drivers/media/platform/ti-vpe/vpdma.h
> index bf5f8bb..2bd8fb0 100644
> --- a/drivers/media/platform/ti-vpe/vpdma.h
> +++ b/drivers/media/platform/ti-vpe/vpdma.h
> @@ -186,13 +186,15 @@ void vpdma_add_cfd_adb(struct vpdma_desc_list
> *list, int client,
> struct vpdma_buf *adb);
> void vpdma_add_sync_on_channel_ctd(struct vpdma_desc_list *list,
> enum vpdma_channel chan);
> -void vpdma_add_out_dtd(struct vpdma_desc_list *list, struct v4l2_rect
> *c_rect,
> +void vpdma_add_out_dtd(struct vpdma_desc_list *list, int width,
> + const struct v4l2_rect *c_rect,
> const struct vpdma_data_format *fmt, dma_addr_t dma_addr,
> enum vpdma_channel chan, u32 flags);
> -void vpdma_add_in_dtd(struct vpdma_desc_list *list, int frame_width,
> - int frame_height, struct v4l2_rect *c_rect,
> +void vpdma_add_in_dtd(struct vpdma_desc_list *list, int width,
> + const struct v4l2_rect *c_rect,
> const struct vpdma_data_format *fmt, dma_addr_t dma_addr,
> - enum vpdma_channel chan, int field, u32 flags);
> + enum vpdma_channel chan, int field, u32 flags, int
> frame_width,
> + int frame_height, int start_h, int start_v);
>
> /* vpdma list interrupt management */
> void vpdma_enable_list_complete_irq(struct vpdma_data *vpdma, int
> list_num, diff --git a/drivers/media/platform/ti-vpe/vpe.c
> b/drivers/media/platform/ti-vpe/vpe.c
> index dbdc338..ece9b96 100644
> --- a/drivers/media/platform/ti-vpe/vpe.c
> +++ b/drivers/media/platform/ti-vpe/vpe.c
> @@ -986,7 +986,6 @@ static void add_out_dtd(struct vpe_ctx *ctx, int
> port)
> struct vpe_q_data *q_data = &ctx->q_data[Q_DATA_DST];
> const struct vpe_port_data *p_data = &port_data[port];
> struct vb2_buffer *vb = ctx->dst_vb;
> - struct v4l2_rect *c_rect = &q_data->c_rect;
> struct vpe_fmt *fmt = q_data->fmt;
> const struct vpdma_data_format *vpdma_fmt;
> int mv_buf_selector = !ctx->src_mv_buf_selector; @@ -1015,8
> +1014,8 @@ static void add_out_dtd(struct vpe_ctx *ctx, int port)
> if (q_data->flags & Q_DATA_MODE_TILED)
> flags |= VPDMA_DATA_MODE_TILED;
>
> - vpdma_add_out_dtd(&ctx->desc_list, c_rect, vpdma_fmt, dma_addr,
> - p_data->channel, flags);
> + vpdma_add_out_dtd(&ctx->desc_list, q_data->width, &q_data->c_rect,
> + vpdma_fmt, dma_addr, p_data->channel, flags);
> }
>
> static void add_in_dtd(struct vpe_ctx *ctx, int port) @@ -1024,11
> +1023,11 @@ static void add_in_dtd(struct vpe_ctx *ctx, int port)
> struct vpe_q_data *q_data = &ctx->q_data[Q_DATA_SRC];
> const struct vpe_port_data *p_data = &port_data[port];
> struct vb2_buffer *vb = ctx->src_vbs[p_data->vb_index];
> - struct v4l2_rect *c_rect = &q_data->c_rect;
> struct vpe_fmt *fmt = q_data->fmt;
> const struct vpdma_data_format *vpdma_fmt;
> int mv_buf_selector = ctx->src_mv_buf_selector;
> int field = vb->v4l2_buf.field == V4L2_FIELD_BOTTOM;
> + int frame_width, frame_height;
> dma_addr_t dma_addr;
> u32 flags = 0;
>
> @@ -1055,8 +1054,15 @@ static void add_in_dtd(struct vpe_ctx *ctx, int
> port)
> if (q_data->flags & Q_DATA_MODE_TILED)
> flags |= VPDMA_DATA_MODE_TILED;
>
> - vpdma_add_in_dtd(&ctx->desc_list, q_data->width, q_data->height,
> - c_rect, vpdma_fmt, dma_addr, p_data->channel, field, flags);
> + frame_width = q_data->c_rect.width;
> + frame_height = q_data->c_rect.height;
> +
> + if (p_data->vb_part && fmt->fourcc == V4L2_PIX_FMT_NV12)
> + frame_height /= 2;
> +
> + vpdma_add_in_dtd(&ctx->desc_list, q_data->width, &q_data->c_rect,
> + vpdma_fmt, dma_addr, p_data->channel, field, flags,
> frame_width,
> + frame_height, 0, 0);
> }
>
> /*
> --
> 1.8.3.2
^ permalink raw reply [flat|nested] 88+ messages in thread
* [PATCH v4 07/14] v4l: ti-vpe: Add selection API in VPE driver
2014-03-13 11:44 ` [PATCH v4 00/14] v4l: ti-vpe: Some VPE fixes and enhancements Archit Taneja
` (5 preceding siblings ...)
2014-03-13 11:44 ` [PATCH v4 06/14] v4l: ti-vpe: Fix some params in VPE data descriptors Archit Taneja
@ 2014-03-13 11:44 ` Archit Taneja
2014-03-13 11:44 ` [PATCH v4 08/14] v4l: ti-vpe: Rename csc memory resource name Archit Taneja
` (6 subsequent siblings)
13 siblings, 0 replies; 88+ messages in thread
From: Archit Taneja @ 2014-03-13 11:44 UTC (permalink / raw)
To: k.debski, hverkuil; +Cc: linux-media, linux-omap, Archit Taneja
Add selection ioctl ops. For VPE, cropping makes sense only for the input to
VPE(or V4L2_BUF_TYPE_VIDEO_OUTPUT/MPLANE buffers) and composing makes sense
only for the output of VPE(or V4L2_BUF_TYPE_VIDEO_CAPTURE/MPLANE buffers).
For the CAPTURE type, V4L2_SEL_TGT_COMPOSE results in VPE writing the output
in a rectangle within the capture buffer. For the OUTPUT type, V4L2_SEL_TGT_CROP
results in selecting a rectangle region within the source buffer.
Setting the crop/compose rectangles should successfully result in
re-configuration of registers which are affected when either source or
destination dimensions change, set_srcdst_params() is called for this purpose.
Signed-off-by: Archit Taneja <archit@ti.com>
---
drivers/media/platform/ti-vpe/vpe.c | 150 ++++++++++++++++++++++++++++++++++++
1 file changed, 150 insertions(+)
diff --git a/drivers/media/platform/ti-vpe/vpe.c b/drivers/media/platform/ti-vpe/vpe.c
index ece9b96..f3f1c10 100644
--- a/drivers/media/platform/ti-vpe/vpe.c
+++ b/drivers/media/platform/ti-vpe/vpe.c
@@ -410,8 +410,10 @@ static struct vpe_q_data *get_q_data(struct vpe_ctx *ctx,
{
switch (type) {
case V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE:
+ case V4L2_BUF_TYPE_VIDEO_OUTPUT:
return &ctx->q_data[Q_DATA_SRC];
case V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE:
+ case V4L2_BUF_TYPE_VIDEO_CAPTURE:
return &ctx->q_data[Q_DATA_DST];
default:
BUG();
@@ -1587,6 +1589,151 @@ static int vpe_s_fmt(struct file *file, void *priv, struct v4l2_format *f)
return set_srcdst_params(ctx);
}
+static int __vpe_try_selection(struct vpe_ctx *ctx, struct v4l2_selection *s)
+{
+ struct vpe_q_data *q_data;
+
+ if ((s->type != V4L2_BUF_TYPE_VIDEO_CAPTURE) &&
+ (s->type != V4L2_BUF_TYPE_VIDEO_OUTPUT))
+ return -EINVAL;
+
+ q_data = get_q_data(ctx, s->type);
+ if (!q_data)
+ return -EINVAL;
+
+ switch (s->target) {
+ case V4L2_SEL_TGT_COMPOSE:
+ /*
+ * COMPOSE target is only valid for capture buffer type, return
+ * error for output buffer type
+ */
+ if (s->type == V4L2_BUF_TYPE_VIDEO_OUTPUT)
+ return -EINVAL;
+ break;
+ case V4L2_SEL_TGT_CROP:
+ /*
+ * CROP target is only valid for output buffer type, return
+ * error for capture buffer type
+ */
+ if (s->type == V4L2_BUF_TYPE_VIDEO_CAPTURE)
+ return -EINVAL;
+ break;
+ /*
+ * bound and default crop/compose targets are invalid targets to
+ * try/set
+ */
+ default:
+ return -EINVAL;
+ }
+
+ if (s->r.top < 0 || s->r.left < 0) {
+ vpe_err(ctx->dev, "negative values for top and left\n");
+ s->r.top = s->r.left = 0;
+ }
+
+ v4l_bound_align_image(&s->r.width, MIN_W, q_data->width, 1,
+ &s->r.height, MIN_H, q_data->height, H_ALIGN, S_ALIGN);
+
+ /* adjust left/top if cropping rectangle is out of bounds */
+ if (s->r.left + s->r.width > q_data->width)
+ s->r.left = q_data->width - s->r.width;
+ if (s->r.top + s->r.height > q_data->height)
+ s->r.top = q_data->height - s->r.height;
+
+ return 0;
+}
+
+static int vpe_g_selection(struct file *file, void *fh,
+ struct v4l2_selection *s)
+{
+ struct vpe_ctx *ctx = file2ctx(file);
+ struct vpe_q_data *q_data;
+ bool use_c_rect = false;
+
+ if ((s->type != V4L2_BUF_TYPE_VIDEO_CAPTURE) &&
+ (s->type != V4L2_BUF_TYPE_VIDEO_OUTPUT))
+ return -EINVAL;
+
+ q_data = get_q_data(ctx, s->type);
+ if (!q_data)
+ return -EINVAL;
+
+ switch (s->target) {
+ case V4L2_SEL_TGT_COMPOSE_DEFAULT:
+ case V4L2_SEL_TGT_COMPOSE_BOUNDS:
+ if (s->type == V4L2_BUF_TYPE_VIDEO_OUTPUT)
+ return -EINVAL;
+ break;
+ case V4L2_SEL_TGT_CROP_BOUNDS:
+ case V4L2_SEL_TGT_CROP_DEFAULT:
+ if (s->type == V4L2_BUF_TYPE_VIDEO_CAPTURE)
+ return -EINVAL;
+ break;
+ case V4L2_SEL_TGT_COMPOSE:
+ if (s->type == V4L2_BUF_TYPE_VIDEO_OUTPUT)
+ return -EINVAL;
+ use_c_rect = true;
+ break;
+ case V4L2_SEL_TGT_CROP:
+ if (s->type == V4L2_BUF_TYPE_VIDEO_CAPTURE)
+ return -EINVAL;
+ use_c_rect = true;
+ break;
+ default:
+ return -EINVAL;
+ }
+
+ if (use_c_rect) {
+ /*
+ * for CROP/COMPOSE target type, return c_rect params from the
+ * respective buffer type
+ */
+ s->r = q_data->c_rect;
+ } else {
+ /*
+ * for DEFAULT/BOUNDS target type, return width and height from
+ * S_FMT of the respective buffer type
+ */
+ s->r.left = 0;
+ s->r.top = 0;
+ s->r.width = q_data->width;
+ s->r.height = q_data->height;
+ }
+
+ return 0;
+}
+
+
+static int vpe_s_selection(struct file *file, void *fh,
+ struct v4l2_selection *s)
+{
+ struct vpe_ctx *ctx = file2ctx(file);
+ struct vpe_q_data *q_data;
+ struct v4l2_selection sel = *s;
+ int ret;
+
+ ret = __vpe_try_selection(ctx, &sel);
+ if (ret)
+ return ret;
+
+ q_data = get_q_data(ctx, sel.type);
+ if (!q_data)
+ return -EINVAL;
+
+ if ((q_data->c_rect.left == sel.r.left) &&
+ (q_data->c_rect.top == sel.r.top) &&
+ (q_data->c_rect.width == sel.r.width) &&
+ (q_data->c_rect.height == sel.r.height)) {
+ vpe_dbg(ctx->dev,
+ "requested crop/compose values are already set\n");
+ return 0;
+ }
+
+ q_data->c_rect = sel.r;
+
+ return set_srcdst_params(ctx);
+}
+
static int vpe_reqbufs(struct file *file, void *priv,
struct v4l2_requestbuffers *reqbufs)
{
@@ -1674,6 +1821,9 @@ static const struct v4l2_ioctl_ops vpe_ioctl_ops = {
.vidioc_try_fmt_vid_out_mplane = vpe_try_fmt,
.vidioc_s_fmt_vid_out_mplane = vpe_s_fmt,
+ .vidioc_g_selection = vpe_g_selection,
+ .vidioc_s_selection = vpe_s_selection,
+
.vidioc_reqbufs = vpe_reqbufs,
.vidioc_querybuf = vpe_querybuf,
--
1.8.3.2
^ permalink raw reply related [flat|nested] 88+ messages in thread
* [PATCH v4 08/14] v4l: ti-vpe: Rename csc memory resource name
2014-03-13 11:44 ` [PATCH v4 00/14] v4l: ti-vpe: Some VPE fixes and enhancements Archit Taneja
` (6 preceding siblings ...)
2014-03-13 11:44 ` [PATCH v4 07/14] v4l: ti-vpe: Add selection API in VPE driver Archit Taneja
@ 2014-03-13 11:44 ` Archit Taneja
2014-03-13 14:44 ` Kamil Debski
2014-03-13 11:44 ` [PATCH v4 09/14] v4l: ti-vpe: report correct capabilities in querycap Archit Taneja
` (5 subsequent siblings)
13 siblings, 1 reply; 88+ messages in thread
From: Archit Taneja @ 2014-03-13 11:44 UTC (permalink / raw)
To: k.debski, hverkuil; +Cc: linux-media, linux-omap, Archit Taneja
Rename the memory block resource "vpe_csc" to "csc" since it also exists within
the VIP IP block. This would make the name more generic, and both VPE and VIP DT
nodes in the future can use it.
Signed-off-by: Archit Taneja <archit@ti.com>
---
drivers/media/platform/ti-vpe/csc.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/media/platform/ti-vpe/csc.c b/drivers/media/platform/ti-vpe/csc.c
index acfea50..0333339 100644
--- a/drivers/media/platform/ti-vpe/csc.c
+++ b/drivers/media/platform/ti-vpe/csc.c
@@ -180,7 +180,7 @@ struct csc_data *csc_create(struct platform_device *pdev)
csc->pdev = pdev;
csc->res = platform_get_resource_byname(pdev, IORESOURCE_MEM,
- "vpe_csc");
+ "csc");
if (csc->res == NULL) {
dev_err(&pdev->dev, "missing platform resources data\n");
return ERR_PTR(-ENODEV);
--
1.8.3.2
^ permalink raw reply related [flat|nested] 88+ messages in thread
* RE: [PATCH v4 08/14] v4l: ti-vpe: Rename csc memory resource name
2014-03-13 11:44 ` [PATCH v4 08/14] v4l: ti-vpe: Rename csc memory resource name Archit Taneja
@ 2014-03-13 14:44 ` Kamil Debski
2014-03-14 6:18 ` Archit Taneja
0 siblings, 1 reply; 88+ messages in thread
From: Kamil Debski @ 2014-03-13 14:44 UTC (permalink / raw)
To: 'Archit Taneja', hverkuil; +Cc: linux-media, linux-omap
Hi,
> From: Archit Taneja [mailto:archit@ti.com]
> Sent: Thursday, March 13, 2014 12:44 PM
>
> Rename the memory block resource "vpe_csc" to "csc" since it also
> exists within the VIP IP block. This would make the name more generic,
> and both VPE and VIP DT nodes in the future can use it.
I understand that this is not yet used in any public dts files. Right?
Best wishes,
--
Kamil Debski
Samsung R&D Institute Poland
>
> Signed-off-by: Archit Taneja <archit@ti.com>
> ---
> drivers/media/platform/ti-vpe/csc.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/media/platform/ti-vpe/csc.c
> b/drivers/media/platform/ti-vpe/csc.c
> index acfea50..0333339 100644
> --- a/drivers/media/platform/ti-vpe/csc.c
> +++ b/drivers/media/platform/ti-vpe/csc.c
> @@ -180,7 +180,7 @@ struct csc_data *csc_create(struct platform_device
> *pdev)
> csc->pdev = pdev;
>
> csc->res = platform_get_resource_byname(pdev, IORESOURCE_MEM,
> - "vpe_csc");
> + "csc");
> if (csc->res == NULL) {
> dev_err(&pdev->dev, "missing platform resources data\n");
> return ERR_PTR(-ENODEV);
> --
> 1.8.3.2
^ permalink raw reply [flat|nested] 88+ messages in thread
* Re: [PATCH v4 08/14] v4l: ti-vpe: Rename csc memory resource name
2014-03-13 14:44 ` Kamil Debski
@ 2014-03-14 6:18 ` Archit Taneja
0 siblings, 0 replies; 88+ messages in thread
From: Archit Taneja @ 2014-03-14 6:18 UTC (permalink / raw)
To: Kamil Debski, hverkuil; +Cc: linux-media, linux-omap
Hi Kamil,
On Thursday 13 March 2014 08:14 PM, Kamil Debski wrote:
> Hi,
>
>> From: Archit Taneja [mailto:archit@ti.com]
>> Sent: Thursday, March 13, 2014 12:44 PM
>>
>> Rename the memory block resource "vpe_csc" to "csc" since it also
>> exists within the VIP IP block. This would make the name more generic,
>> and both VPE and VIP DT nodes in the future can use it.
>
> I understand that this is not yet used in any public dts files. Right?
>
> Best wishes,
>
Yes, a VPE DT node doesn't exist in any public dts files yet. So it's
safe to change the name.
It should eventually come in dra7.dtsi. There is a dependency on a
crossbar IP module, which provides us with an IRQ line for VPE going to
the GIC. Once that is merged, I can add the VPE DT node.
Thanks,
Archit
^ permalink raw reply [flat|nested] 88+ messages in thread
* [PATCH v4 09/14] v4l: ti-vpe: report correct capabilities in querycap
2014-03-13 11:44 ` [PATCH v4 00/14] v4l: ti-vpe: Some VPE fixes and enhancements Archit Taneja
` (7 preceding siblings ...)
2014-03-13 11:44 ` [PATCH v4 08/14] v4l: ti-vpe: Rename csc memory resource name Archit Taneja
@ 2014-03-13 11:44 ` Archit Taneja
2014-03-13 11:44 ` [PATCH v4 10/14] v4l: ti-vpe: Use correct bus_info name for the device " Archit Taneja
` (4 subsequent siblings)
13 siblings, 0 replies; 88+ messages in thread
From: Archit Taneja @ 2014-03-13 11:44 UTC (permalink / raw)
To: k.debski, hverkuil; +Cc: linux-media, linux-omap, Archit Taneja
querycap currently returns V4L2_CAP_VIDEO_M2M as a capability, this should be
V4L2_CAP_VIDEO_M2M_MPLANE instead, as the driver supports multiplanar formats.
Reviewed-by: Hans Verkuil <hans.verkuil@cisco.com>
Signed-off-by: Archit Taneja <archit@ti.com>
---
drivers/media/platform/ti-vpe/vpe.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/media/platform/ti-vpe/vpe.c b/drivers/media/platform/ti-vpe/vpe.c
index f3f1c10..c066eb8 100644
--- a/drivers/media/platform/ti-vpe/vpe.c
+++ b/drivers/media/platform/ti-vpe/vpe.c
@@ -1347,7 +1347,7 @@ static int vpe_querycap(struct file *file, void *priv,
strncpy(cap->driver, VPE_MODULE_NAME, sizeof(cap->driver) - 1);
strncpy(cap->card, VPE_MODULE_NAME, sizeof(cap->card) - 1);
strlcpy(cap->bus_info, VPE_MODULE_NAME, sizeof(cap->bus_info));
- cap->device_caps = V4L2_CAP_VIDEO_M2M | V4L2_CAP_STREAMING;
+ cap->device_caps = V4L2_CAP_VIDEO_M2M_MPLANE | V4L2_CAP_STREAMING;
cap->capabilities = cap->device_caps | V4L2_CAP_DEVICE_CAPS;
return 0;
}
--
1.8.3.2
^ permalink raw reply related [flat|nested] 88+ messages in thread
* [PATCH v4 10/14] v4l: ti-vpe: Use correct bus_info name for the device in querycap
2014-03-13 11:44 ` [PATCH v4 00/14] v4l: ti-vpe: Some VPE fixes and enhancements Archit Taneja
` (8 preceding siblings ...)
2014-03-13 11:44 ` [PATCH v4 09/14] v4l: ti-vpe: report correct capabilities in querycap Archit Taneja
@ 2014-03-13 11:44 ` Archit Taneja
2014-03-13 11:44 ` [PATCH v4 11/14] v4l: ti-vpe: Fix initial configuration queue data Archit Taneja
` (3 subsequent siblings)
13 siblings, 0 replies; 88+ messages in thread
From: Archit Taneja @ 2014-03-13 11:44 UTC (permalink / raw)
To: k.debski, hverkuil; +Cc: linux-media, linux-omap, Archit Taneja
The bus_info parameter in v4l2_capabilities expects a 'platform_' prefix. This
wasn't done in the driver and hence was breaking compliance. Update the bus_info
parameter accordingly.
Reviewed-by: Hans Verkuil <hans.verkuil@cisco.com>
Signed-off-by: Archit Taneja <archit@ti.com>
---
drivers/media/platform/ti-vpe/vpe.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/media/platform/ti-vpe/vpe.c b/drivers/media/platform/ti-vpe/vpe.c
index c066eb8..3a312ea 100644
--- a/drivers/media/platform/ti-vpe/vpe.c
+++ b/drivers/media/platform/ti-vpe/vpe.c
@@ -1346,7 +1346,8 @@ static int vpe_querycap(struct file *file, void *priv,
{
strncpy(cap->driver, VPE_MODULE_NAME, sizeof(cap->driver) - 1);
strncpy(cap->card, VPE_MODULE_NAME, sizeof(cap->card) - 1);
- strlcpy(cap->bus_info, VPE_MODULE_NAME, sizeof(cap->bus_info));
+ snprintf(cap->bus_info, sizeof(cap->bus_info), "platform:%s",
+ VPE_MODULE_NAME);
cap->device_caps = V4L2_CAP_VIDEO_M2M_MPLANE | V4L2_CAP_STREAMING;
cap->capabilities = cap->device_caps | V4L2_CAP_DEVICE_CAPS;
return 0;
--
1.8.3.2
^ permalink raw reply related [flat|nested] 88+ messages in thread
* [PATCH v4 11/14] v4l: ti-vpe: Fix initial configuration queue data
2014-03-13 11:44 ` [PATCH v4 00/14] v4l: ti-vpe: Some VPE fixes and enhancements Archit Taneja
` (9 preceding siblings ...)
2014-03-13 11:44 ` [PATCH v4 10/14] v4l: ti-vpe: Use correct bus_info name for the device " Archit Taneja
@ 2014-03-13 11:44 ` Archit Taneja
2014-03-13 11:44 ` [PATCH v4 12/14] v4l: ti-vpe: zero out reserved fields in try_fmt Archit Taneja
` (2 subsequent siblings)
13 siblings, 0 replies; 88+ messages in thread
From: Archit Taneja @ 2014-03-13 11:44 UTC (permalink / raw)
To: k.debski, hverkuil; +Cc: linux-media, linux-omap, Archit Taneja
The vpe output and capture queues are initially configured to default values in
vpe_open(). A G_FMT before any S_FMTs will result in these values being
populated.
The colorspace and bytesperline parameter of this initial configuration are
incorrect. This breaks compliance when as we get 'TRY_FMT(G_FMT) != G_FMT'.
Fix the initial queue configuration such that it wouldn't need to be fixed by
try_fmt.
Reviewed-by: Hans Verkuil <hans.verkuil@cisco.com>
Signed-off-by: Archit Taneja <archit@ti.com>
---
drivers/media/platform/ti-vpe/vpe.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/drivers/media/platform/ti-vpe/vpe.c b/drivers/media/platform/ti-vpe/vpe.c
index 3a312ea..d7befb9 100644
--- a/drivers/media/platform/ti-vpe/vpe.c
+++ b/drivers/media/platform/ti-vpe/vpe.c
@@ -2021,9 +2021,11 @@ static int vpe_open(struct file *file)
s_q_data->fmt = &vpe_formats[2];
s_q_data->width = 1920;
s_q_data->height = 1080;
- s_q_data->sizeimage[VPE_LUMA] = (s_q_data->width * s_q_data->height *
+ s_q_data->bytesperline[VPE_LUMA] = (s_q_data->width *
s_q_data->fmt->vpdma_fmt[VPE_LUMA]->depth) >> 3;
- s_q_data->colorspace = V4L2_COLORSPACE_SMPTE170M;
+ s_q_data->sizeimage[VPE_LUMA] = (s_q_data->bytesperline[VPE_LUMA] *
+ s_q_data->height);
+ s_q_data->colorspace = V4L2_COLORSPACE_REC709;
s_q_data->field = V4L2_FIELD_NONE;
s_q_data->c_rect.left = 0;
s_q_data->c_rect.top = 0;
--
1.8.3.2
^ permalink raw reply related [flat|nested] 88+ messages in thread
* [PATCH v4 12/14] v4l: ti-vpe: zero out reserved fields in try_fmt
2014-03-13 11:44 ` [PATCH v4 00/14] v4l: ti-vpe: Some VPE fixes and enhancements Archit Taneja
` (10 preceding siblings ...)
2014-03-13 11:44 ` [PATCH v4 11/14] v4l: ti-vpe: Fix initial configuration queue data Archit Taneja
@ 2014-03-13 11:44 ` Archit Taneja
2014-03-13 11:44 ` [PATCH v4 13/14] v4l: ti-vpe: Set correct field parameter for output and capture buffers Archit Taneja
2014-03-13 11:44 ` [PATCH v4 14/14] v4l: ti-vpe: retain v4l2_buffer flags for captured buffers Archit Taneja
13 siblings, 0 replies; 88+ messages in thread
From: Archit Taneja @ 2014-03-13 11:44 UTC (permalink / raw)
To: k.debski, hverkuil; +Cc: linux-media, linux-omap, Archit Taneja
Zero out the reserved formats in v4l2_pix_format_mplane and
v4l2_plane_pix_format members of the returned v4l2_format pointer when passed
through TRY_FMT ioctl.
This ensures that the user doesn't interpret the non-zero fields as some data
passed by the driver, and ensures compliance.
Reviewed-by: Hans Verkuil <hans.verkuil@cisco.com>
Signed-off-by: Archit Taneja <archit@ti.com>
---
drivers/media/platform/ti-vpe/vpe.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/drivers/media/platform/ti-vpe/vpe.c b/drivers/media/platform/ti-vpe/vpe.c
index d7befb9..c0ae847 100644
--- a/drivers/media/platform/ti-vpe/vpe.c
+++ b/drivers/media/platform/ti-vpe/vpe.c
@@ -1488,6 +1488,7 @@ static int __vpe_try_fmt(struct vpe_ctx *ctx, struct v4l2_format *f,
}
}
+ memset(pix->reserved, 0, sizeof(pix->reserved));
for (i = 0; i < pix->num_planes; i++) {
plane_fmt = &pix->plane_fmt[i];
depth = fmt->vpdma_fmt[i]->depth;
@@ -1499,6 +1500,8 @@ static int __vpe_try_fmt(struct vpe_ctx *ctx, struct v4l2_format *f,
plane_fmt->sizeimage =
(pix->height * pix->width * depth) >> 3;
+
+ memset(plane_fmt->reserved, 0, sizeof(plane_fmt->reserved));
}
return 0;
--
1.8.3.2
^ permalink raw reply related [flat|nested] 88+ messages in thread
* [PATCH v4 13/14] v4l: ti-vpe: Set correct field parameter for output and capture buffers
2014-03-13 11:44 ` [PATCH v4 00/14] v4l: ti-vpe: Some VPE fixes and enhancements Archit Taneja
` (11 preceding siblings ...)
2014-03-13 11:44 ` [PATCH v4 12/14] v4l: ti-vpe: zero out reserved fields in try_fmt Archit Taneja
@ 2014-03-13 11:44 ` Archit Taneja
2014-03-13 11:44 ` [PATCH v4 14/14] v4l: ti-vpe: retain v4l2_buffer flags for captured buffers Archit Taneja
13 siblings, 0 replies; 88+ messages in thread
From: Archit Taneja @ 2014-03-13 11:44 UTC (permalink / raw)
To: k.debski, hverkuil; +Cc: linux-media, linux-omap, Archit Taneja
The vpe driver wasn't setting the correct field parameter for dequed CAPTURE
type buffers for the case where the captured output is progressive.
Set the field to V4L2_FIELD_NONE for the completed destination buffers when
the captured output is progressive.
For OUTPUT type buffers, a queued buffer's field is forced to V4L2_FIELD_NONE
if the pixel format(configured through s_fmt for the buffer type
V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE specifies) the field type isn't interlaced.
If the pixel format specified was V4L2_FIELD_ALTERNATE, and the queued buffer's
field isn't V4L2_FIELD_TOP or V4L2_FIELD_BOTTOM, the vb2 buf_prepare op returns
an error.
This ensures compliance, and that the dequeued output and captured buffers
contain the field type that the driver used internally.
Signed-off-by: Archit Taneja <archit@ti.com>
---
drivers/media/platform/ti-vpe/vpe.c | 13 ++++++++++++-
1 file changed, 12 insertions(+), 1 deletion(-)
diff --git a/drivers/media/platform/ti-vpe/vpe.c b/drivers/media/platform/ti-vpe/vpe.c
index c0ae847..362d5be 100644
--- a/drivers/media/platform/ti-vpe/vpe.c
+++ b/drivers/media/platform/ti-vpe/vpe.c
@@ -1296,10 +1296,10 @@ static irqreturn_t vpe_irq(int irq_vpe, void *data)
d_buf->timecode = s_buf->timecode;
}
d_buf->sequence = ctx->sequence;
- d_buf->field = ctx->field;
d_q_data = &ctx->q_data[Q_DATA_DST];
if (d_q_data->flags & Q_DATA_INTERLACED) {
+ d_buf->field = ctx->field;
if (ctx->field == V4L2_FIELD_BOTTOM) {
ctx->sequence++;
ctx->field = V4L2_FIELD_TOP;
@@ -1308,6 +1308,7 @@ static irqreturn_t vpe_irq(int irq_vpe, void *data)
ctx->field = V4L2_FIELD_BOTTOM;
}
} else {
+ d_buf->field = V4L2_FIELD_NONE;
ctx->sequence++;
}
@@ -1880,6 +1881,16 @@ static int vpe_buf_prepare(struct vb2_buffer *vb)
q_data = get_q_data(ctx, vb->vb2_queue->type);
num_planes = q_data->fmt->coplanar ? 2 : 1;
+ if (vb->vb2_queue->type == V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE) {
+ if (!(q_data->flags & Q_DATA_INTERLACED)) {
+ vb->v4l2_buf.field = V4L2_FIELD_NONE;
+ } else {
+ if (vb->v4l2_buf.field != V4L2_FIELD_TOP &&
+ vb->v4l2_buf.field != V4L2_FIELD_BOTTOM)
+ return -EINVAL;
+ }
+ }
+
for (i = 0; i < num_planes; i++) {
if (vb2_plane_size(vb, i) < q_data->sizeimage[i]) {
vpe_err(ctx->dev,
--
1.8.3.2
^ permalink raw reply related [flat|nested] 88+ messages in thread
* [PATCH v4 14/14] v4l: ti-vpe: retain v4l2_buffer flags for captured buffers
2014-03-13 11:44 ` [PATCH v4 00/14] v4l: ti-vpe: Some VPE fixes and enhancements Archit Taneja
` (12 preceding siblings ...)
2014-03-13 11:44 ` [PATCH v4 13/14] v4l: ti-vpe: Set correct field parameter for output and capture buffers Archit Taneja
@ 2014-03-13 11:44 ` Archit Taneja
13 siblings, 0 replies; 88+ messages in thread
From: Archit Taneja @ 2014-03-13 11:44 UTC (permalink / raw)
To: k.debski, hverkuil; +Cc: linux-media, linux-omap, Archit Taneja
The dequed CAPTURE_MPLANE type buffers don't contain the flags that the
originally queued OUTPUT_MPLANE type buffers have. This breaks compliance.
Copy the source v4l2_buffer flags to the destination v4l2_buffer flags before
they are dequed.
Reviewed-by: Hans Verkuil <hans.verkuil@cisco.com>
Signed-off-by: Archit Taneja <archit@ti.com>
---
drivers/media/platform/ti-vpe/vpe.c | 9 ++++-----
1 file changed, 4 insertions(+), 5 deletions(-)
diff --git a/drivers/media/platform/ti-vpe/vpe.c b/drivers/media/platform/ti-vpe/vpe.c
index 362d5be..972f43f 100644
--- a/drivers/media/platform/ti-vpe/vpe.c
+++ b/drivers/media/platform/ti-vpe/vpe.c
@@ -1288,13 +1288,12 @@ static irqreturn_t vpe_irq(int irq_vpe, void *data)
s_buf = &s_vb->v4l2_buf;
d_buf = &d_vb->v4l2_buf;
+ d_buf->flags = s_buf->flags;
+
d_buf->timestamp = s_buf->timestamp;
- d_buf->flags &= ~V4L2_BUF_FLAG_TSTAMP_SRC_MASK;
- d_buf->flags |= s_buf->flags & V4L2_BUF_FLAG_TSTAMP_SRC_MASK;
- if (s_buf->flags & V4L2_BUF_FLAG_TIMECODE) {
- d_buf->flags |= V4L2_BUF_FLAG_TIMECODE;
+ if (s_buf->flags & V4L2_BUF_FLAG_TIMECODE)
d_buf->timecode = s_buf->timecode;
- }
+
d_buf->sequence = ctx->sequence;
d_q_data = &ctx->q_data[Q_DATA_DST];
--
1.8.3.2
^ permalink raw reply related [flat|nested] 88+ messages in thread