On 11/27/13 11:29, Valentine wrote: > On 11/27/2013 12:21 PM, Hans Verkuil wrote: >> On 11/26/2013 10:28 PM, Valentine wrote: >>> On 11/20/2013 07:53 PM, Valentine wrote: >>>> On 11/20/2013 07:42 PM, Hans Verkuil wrote: >>>>> Hi Valentine, > > Hi Hans, > >>> >>> Hi Hans, >>> >>>>> >>>>> Did you ever look at this adv7611 driver: >>>>> >>>>> https://github.com/Xilinx/linux-xlnx/commit/610b9d5de22ae7c0047c65a07e4afa42af2daa12 >>>> >>>> No, I missed that one somehow, although I did search for the adv7611/7612 before implementing this one. >>>> I'm going to look closer at the patch and test it. >>>> >>> >>> I've tried the patch and I doubt that it was ever tested on adv7611. >>> I haven't been able to make it work so far. Here's the description of some of the issues >>> I've encountered. >>> >>> The patch does not apply cleanly so I had to make small adjustments just to make it apply >>> without changing the functionality. >>> >>> First of all the driver (adv7604_dummy_client function) does not set default I2C slave addresses >>> in the I/O map in case they are not set in the platform data. >>> This is not needed for 7604, since the default addresses are already set in the I/O map after chip reset. >>> However, the map is zeroed on 7611/7612 after power up, and we always have to set it manually. >> >> So, the platform data for the 7611/2 should always give i2c addresses. That seems >> reasonable. > > Yes, but currently the comment in include/media/adv7604.h says > "i2c addresses: 0 == use default", and this is true for 7604, but > it doesn't work for 7611. > > Probably the recommended value from the docs should be set by > the driver in case an I2C address is zero in the platform data. > This will help us to keep the same approach across all 76xx chips. That would work for me. > >> >>> I had to implement the IRQ handler since the soc_camera model does not use >>> interrupt_service_routine subdevice callback and R-Car VIN knows nothing about adv7612 >>> interrupt routed to a GPIO pin. >>> So I had to schedule a workqueue and call adv7604_isr from there in case an interrupt happens. >> >> For our systems the adv7604 interrupts is not always hooked up to a gpio irq, instead >> a register has to be read to figure out which device actually produced the irq. So I >> want to keep the interrupt_service_routine(). However, adding a gpio field to the >> platform_data that, if set, will tell the driver to request an irq and setup a >> workqueue that calls interrupt_service_routine() would be fine with me. That will >> benefit a lot of people since using gpios is much more common. >> >>> The driver enables multiple interrupts on the chip, however, the adv7604_isr callback doesn't >>> seem to handle them correctly. >>> According to the docs: >>> "If an interrupt event occurs, and then a second interrupt event occurs before the system controller >>> has cleared or masked the first interrupt event, the ADV7611 does not generate a second interrupt signal." >>> >>> However, the interrupt_service_routine doesn't account for that. >>> For example, in case fmt_change interrupt happens while fmt_change_digital interrupt is being >>> processed by the adv7604_isr routine. If fmt_change status is set just before we clear fmt_change_digital, >>> we never clear fmt_change. Thus, we end up with fmt_change interrupt missed and therefore further interrupts disabled. >>> I've tried to call the adv7604_isr routine in a loop and return from the worlqueue only when all interrupt status bits are cleared. >>> This did help a bit, but sometimes I started getting lots of I2C read/write errors for some reason. >> >> I'm not sure if there is much that can be done about this. The code reads the >> interrupt status, then clears the interrupts right after. There is always a >> race condition there since this isn't atomic ('read and clear'). Unless Lars-Peter >> has a better idea? >> >> What can be improved, though, is to clear not just the interrupts that were >> read, but all the interrupts that are unmasked. You are right, you could >> loose an interrupt that way. >> >>> I'm also not sure how the dv_timing API should be used. >>> The internal adv7604 state->timings structure is used when getting mbus format. >>> However, the driver does not set the structure neither at start-up nor in the interrupt service callback when format changes. >>> Is it supposed to be set by the upper level camera driver? >> >> It would be nice if the adv7604 would set up an initial timings format. In our >> case it is indeed the bridge driver that sets it up, but in the general case it >> is better if the i2c driver also sets an initial timings struct. 720p60 is >> generally a good initial value. >> >> The irq certainly shouldn't change timings: changing timings will most likely >> require changes in the video buffer sizes, which generally requires stopping >> streaming, reconfiguring the pipeline and restarting streaming. That's not >> something the i2c driver can do. >> >> The confusion you have with mbus vs dv_timings is that soc_camera lacks dv_timings >> support. It was designed for sensors, although there is now some support for SDTV >> receivers (s/g_std ioctls), but dv_timings support has to be added there as well >> along the lines of what is done for s/g_std. Basically it is just a passthrough. >> >> The way s_mbus_fmt is defined in adv7604 today is correct. s_dv_timings should be >> called to change the format, s_mbus_fmt should just return the current width/height. >> For HDTV there is more involved than just width and height when changing formats, >> just as SDTV. >> >> So the right approach is to add support for query/enum/s/g_dv_timings and dv_timings_cap >> to soc_camera (just passthroughs). Then you can use it the way you are supposed to. >> >>> For example, when the camera driver receives v4l2_subdev_notify(sd, ADV7604_FMT_CHANGE, NULL); >>> does it have to do the following: >>> v4l2_subdev_call(sd, video, query_dv_timings, timings); >>> v4l2_subdev_call(sd, video, s_dv_timings, timings);? >>> >>> I don't think that this is how it should work. >> >> And it shouldn't work like that. The soc_camera driver has to send out a FMT_CHANGE >> event to the application. It is the application that will receive that event, and >> will have to call QUERY_DV_TIMINGS, stop streaming, allocate new buffers to accomodate >> the new buffer size, call S_DV_TIMINGS and STREAMON to restart the newly configured >> pipeline. >> > > IIUC, we can't just use VIDIOC_S_FMT, prepare the buffers and read a frame from HDTV. > We have to query dv_timings instead. Right. > It looks like VIDIOC_G_FMT, VIDIOC_S_FMT, VIDIOC_TRY_FMT may in fact return incorrect > data unless the application queries and sets DV timings before using S/G/TRY FMT ioctls. > What's the use of the FMT ioctls then? The FMT ioctls define the size of your memory buffers. Depending on your use case (crop/compose settings, stride, padding) you might want to allocate different sized buffers than you would expect from just the timings. Also, different DV timings will map to identical resolutions: different frame rates/pixelclocks, reduced blanking modes, differences in polarities, etc. It's no different than e.g. PAL vs SECAM: same resolution, but very different otherwise. In other words: the DV_TIMINGS and STD ioctls negotiate the video signal detection/setup and the FMT ioctls deal with the memory format. Setting the video signal (S_STD or S_DV_TIMINGS) must be done first and it will typically reset the v4l2_format settings. If streaming is in progress S_STD or S_DV_TIMINGS must return EBUSY (unless there is no change, in which case they can return 0 immediately). > >> And we still haven't defined that FMT_CHANGE event. This is literally the first time >> that an upstreamed bridge driver has to support this. >> >> I will make an RFC for this today or tomorrow. It's really time that we add it. > > Thanks! > >> >>> >>> Anyways, I've tried to call query_dv_timings to initialize state->timings from the interrupt service workqueue. >> >> That's absolutely a no-go. Drivers should never change format midstream. >> >>> I've been able to catch format change events though it looks very sloppy at the moment. >>> >>> BTW, the driver doesn't provide any locking for reading/writing the state->settings which I believe could cause >>> some issues reading incomplete format when it changes asynchronously to the subdevice g_mbus_fmt operation. >> >> That shouldn't happen asynchronously. If you have asynchronous behavior like that, then >> that needs a close look. >> >> Another issue you have is how to set up the EDID in the receiver. Currently this >> is done through the v4l2-subdevX device node of the subdev and the VIDIOC_SUBDEV_G/S_EDID >> ioctls. However, soc_camera does not create those device nodes at the moment. >> For simple pipelines this may be overkill anyway. In our (non-upstreamed) bridge >> drivers we just implement VIDIOC_SUBDEV_G/S_EDID in the bridge driver and >> pass it through to the subdev. I have to think about this a bit more, perhaps >> I should create VIDIOC_G/S_EDID ioctls that can be used by standard, simple >> v4l2 devices as well. I'll add it to the RFC. >> >> Regardless, soc_camera needs to add support for setting/getting EDIDs one way >> or another. >> >>> >>>>> >>>>> It adds adv761x support to the adv7604 in a pretty clean way. >>> >>> Doesn't seem that clean to me after having a look at it. >>> It tries to handle both 7604 and 7611 chips in the same way, though, >>> I'm not exactly sure if it's a good idea since 7611/12 is a pure HDMI receiver with no analog inputs. >> >> The analog support of the adv7604 is pretty separate from the HDMI part, so >> I don't see that as a big deal. That said, I do have to reevaluate that when >> I see the latest version of this patch from Analog Devices later this week. >> >>> >>>>> >>>>> Thinking it over I prefer to use that code (although you will have to >>>>> add the soc-camera hack for the time being) over your driver. >>>>> >>>>> Others need adv7611 support as well, but with all the dv_timings etc. features >>>>> that are removed in your driver. So I am thinking that it is easier to merge >>>>> the xilinx version and add whatever you need on top of that. >>> >>> To be honest I'm more inclined to drop non-soc camera support from my driver and >>> move it to media/i2c/soc_camera/ the moment. That would be easier. >> >> I won't accept that, sorry.The issues you have derive more from misunderstanding >> the way an HDTV receiver is supposed to work (it's not all that easy to wrap your >> head around it) and from missing functionality for HDTV in soc_camera and even in >> the V4L2 API (because certain bridge drivers that demonstrate how it works couldn't >> be upstreamed and we want to avoid adding API additions without having drivers >> using it). > > Thank you very much for your explanations. > Yes, it's kind of hard to get a hold of it with some functionality missing in the API > and the drivers. Your help is much appreciated. No problem, I'd like to get this sorted as well. Regards, Hans -- 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
