Re: [PATCH v3 02/14] v4l: ti-vpe: register video device only when firmware is loaded
Hi, On Thursday 13 March 2014 07:59 PM, Kamil Debski wrote: Hi, From: Archit Taneja [mailto:arc...@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:arc...@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 -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH v3 02/14] v4l: ti-vpe: register video device only when firmware is loaded
Hi Archit,
From: Archit Taneja [mailto:arc...@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 RD Institute Poland
Signed-off-by: Archit Taneja arc...@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;
Re: [PATCH v3 02/14] v4l: ti-vpe: register video device only when firmware is loaded
Hi Kamil, On Thursday 13 March 2014 05:18 PM, Kamil Debski wrote: Hi Archit, From: Archit Taneja [mailto:arc...@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 -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH v3 02/14] v4l: ti-vpe: register video device only when firmware is loaded
Hi, From: Archit Taneja [mailto:arc...@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:arc...@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 RD Institute Poland -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v3 02/14] v4l: ti-vpe: register video device only when firmware is loaded
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 arc...@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;
+ }
+
+
