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
