On 15/02/2021 16:23, Tomi Valkeinen wrote: > On 08/12/2020 18:15, Laurent Pinchart wrote: > >>>>> I noticed that this defaults to video centric. >>>>> >>>>> To come back to the discussion of the v2 of this patch, I believe we >>>>> need to decide what to do here so we have a good template for future >>>>> drivers that need this. >>>>> >>>>> My opinion is that you want a Kconfig option to set the default for >>>>> this, so this becomes something like this: >>>>> >>>>> bool cal_mc_api = CONFIG_TI_CAL_MC_API; >>>>> >>>>> What do you think? >>>>> >>>>> I will make a PR for v5.12 for patches 1-23, but I would like to have this >>>>> remaining issue resolved before merging this final patch. >>>>> >>>>> I do think that a Kconfig option is very desirable, but whether the default >>>>> of this option should be y or n is less clear. Since this driver has always >>>>> been video-centric I can imagine that it makes sense to set it to n. But >>>>> for e.g. a new driver like the tegra-video driver (currently in staging), >>>>> it would make sense to set it to y since it is a new driver. Ditto for the >>>>> rpi camera driver. >>>> >>>> For this driver I think video-centric mode is the best default to start >>>> with, to avoid changing the behaviour all of a sudden. We can switch it >>>> to MC-centric by default later if desired, after userspace gets a chance >>>> to adapt. >>> >>> Fair enough. >>> >>>> >>>>> In that case the rule would be that for new mainline drivers the default >>>>> should always be y (MC-centric), but if the driver was already in mainline >>>>> and MC support is added (like for this driver), then the default remains n >>>>> for backwards compatibility. >>>> >>>> I think that for new drivers we shouldn't support video-centric mode at >>>> all. It should only be supported in downstream (vendor) kernels, and >>>> only if backward compatibility with existing userspace needs to be >>>> ensured. The unicam driver, for instance, fits in that category. Drivers >>>> whose development is ongoing (or hasn't started) should only use the MC >>>> API. Whether the option should be y or n by default would then be a >>>> vendor decision, it wouldn't affect upstream. >>> >>> No, that I strongly disagree with. Vendors would have to carry those patches >>> for a long time, and if past experience is any guide, they will mess it up. >>> Or even refuse to upgrade to the mainline code because it is too much hassle >>> and instead keep using their own driver. >>> >>> In my opinion the mainline driver should be MC-centric, and it is up to the >>> vendor to decide whether video-centric is also supported: this should only >>> be done if there is a long history of video-centric behavior in the past. >>> In that case a Kconfig option is needed to select MC, and in the mainline >>> kernel this should default to y for such new drivers. >>> >>> In both Raspbian and Linux4Tegra video-centric has been the norm for many >>> years, so there are many userspace applications that expect that behavior. >>> You want those distros to use the mainline driver (eventually...) since >>> those distros are widely used so you also get a large installed base and >>> (hopefully) bug reports and bug fixes for the driver. If you decide to >>> require the distro to carry a patch to turn a driver into a video-centric >>> variant, then I am afraid they will not bother upgrading to the mainline >>> driver and just keep their own driver. >> >> For Raspberry Pi, and the Unicam driver in particular, that won't be >> possible. A video-centric API will require quite a few hacks that >> shouldn't be upstreamed, in particular to support multiple CSI-2 data >> types. The current implementation uses two sink pad in the CSI-2 >> receiver subdevs to model the image and embedded data multiplexed over >> the CSI-2 virtual channel. This requires corresponding changes to sensor >> drivers to use two source pads. Sakari has reviewed this, and the >> implementation will need to move to the V4L2 multiplexed streams support >> API (which has been proposed but not merged yet), and I can't see this >> working well with a video-centric approach. >> >> I suspect the same would apply to any CSI-2 receiver, and thus to Tegra >> as well, but I can't comment on that as I'm not familiar with the >> hardware and driver. >> >>> In any case, I really like your approach, all I want is a Kconfig option >>> and it is good to go. > > Waking up this thread, as I'm writing new patches based on these =). > > For this series, afaiu there are no open questions. We can add a kconfig > option to choose the default option (in addition to the module > parameter), and as discussed, this one should default to video mode. Can someone make a v4 of this patch? It would be nice to get this last remaining patch merged. > > For new drivers, I think we should require support for MC (and default > to MC), but leave the decision about video support to the > vendor/developer. Makes sense. > I have the same concerns as Hans if we reject new > drivers with video support by default. > Then again, I think it's sensible to require the video support to be... > well, "sensible". The code for video support should be quite > straightforward and simple. It should be a valid reason to reject the > driver if the driver tries to support complex HW setups with video model > and ends up creating all kinds of hacks which are not needed with MC. > (the Unicam case Laurent described above sounds like this). And there is a grey area between 'sensible' and 'not sensible'. If there is already a large video-centric ecosystem, then there is a good reason to allow for more complexity to avoid distros forking the driver. I guess I would have to see the complexity to be able to say if it is worth it in my opinion (which I am sure differs from other people's opinion!). Regards, Hans
