Re: [PATCH v3 24/24] media: ti-vpe: cal: Implement media controller centric API

From: Hans Verkuil <hverkuil@xs4all.nl>
To: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Cc: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>,
	linux-media@vger.kernel.org, Benoit Parrot <bparrot@ti.com>
Subject: Re: [PATCH v3 24/24] media: ti-vpe: cal: Implement media controller centric API
Date: Wed, 3 Mar 2021 16:51:13 +0100	[thread overview]
Message-ID: <5cfce379-626a-832d-33ac-e39bc3a17309@xs4all.nl> (raw)
In-Reply-To: <YD+ptwH0g79zLzIn@pendragon.ideasonboard.com>

On 03/03/2021 16:22, Laurent Pinchart wrote:
> Hi Hans,
> 
> On Wed, Mar 03, 2021 at 04:15:37PM +0100, Hans Verkuil wrote:
>> 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.
> 
> I'll work on this.
> 
>>> 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 don't think generic distros will care. We're talking about SoC drivers
> here, coming from a downstream kernel, so it's only the vendor's
> ecosystem that matters, and whether they want to support both in their
> camera stack or move it to MC-only.

Sorry, I wasn't clear: I meant SoC vendor distros. Now most of these have
few users and/or are awful and often very quickly abandoned. I don't care
about those.

But Raspbian and to a lesser extent Linux4Tegra are widely used by hobbyists
as well as professionals, and Raspbian in particular is using a fairly recent
kernel. Making a mainline driver that doesn't fit that ecosystem will almost
certainly cause it to be forked and I think that is something that should be
avoided, even at the expense of additional complexity. It's one area where
we should be willing to go the extra mile.

How many extra miles we are willing to go is another matter, and I don't
know enough to come to a conclusion on that.

In any case, a mainline RPi driver should start out as an MC driver, while
keeping in mind that a video-centric mode might be added later.

If adding a video centric mode turns out to be too much work, then so be it.

Since the vast majority of RPi users will just use Raspbian, I'm afraid that
creating an MC-only RPi driver will just mean that almost nobody will actually
use it, because everyone will stick to the Raspbian video-centric driver.

Anyway, this is just my opinion.

Regards,

	Hans