[v3,02/14] v4l: ti-vpe: register video device only when firmware is loaded
Commit Message
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(-)
Comments
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,
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
--
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
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,
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
--
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
@@ -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) {
@@ -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
@@ -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: