RE: [PATCH v3 02/14] v4l: ti-vpe: register video device only when firmware is loaded

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi,

> From: Archit Taneja [mailto:archit@xxxxxx]
> 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@xxxxxx]
> >> 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

--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html




[Index of Archives]     [Linux Input]     [Video for Linux]     [Gstreamer Embedded]     [Mplayer Users]     [Linux USB Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [Yosemite Backpacking]
  Powered by Linux