On 20/07/2020 20:44, Janusz Krzysztofik wrote:
> On Monday, July 20, 2020 10:48:36 A.M. CEST Hans Verkuil wrote:
>> On 19/07/2020 13:18, Janusz Krzysztofik wrote:
>>> Hi Hans,
>>>
>>> On Wednesday, July 15, 2020 5:08:05 P.M. CEST Hans Verkuil wrote:
>>>> On 14/07/2020 15:58, Jacopo Mondi wrote:
>>>>> Introduce two new pad operations to allow retrieving and configuring the
>>>>> media bus parameters on a subdevice pad.
>>>>>
>>>>> The newly introduced operations aims to replace the s/g_mbus_config video
>>>>> operations, which have been on their way for deprecation since a long
>>>>> time.
>>>>>
>>>>> Signed-off-by: Jacopo Mondi <jacopo+renesas@xxxxxxxxxx>
>>>>> ---
>>>>> include/media/v4l2-subdev.h | 27 +++++++++++++++++++++++++++
>>>>> 1 file changed, 27 insertions(+)
>>>>>
>>>>> diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
>>>>> index f7fe78a6f65a..d8b9d5735307 100644
>>>>> --- a/include/media/v4l2-subdev.h
>>>>> +++ b/include/media/v4l2-subdev.h
>>>>> @@ -670,6 +670,29 @@ struct v4l2_subdev_pad_config {
>>>>> *
>>>>> * @set_frame_desc: set the low level media bus frame parameters, @fd
> array
>>>>> * may be adjusted by the subdev driver to device
> capabilities.
>>>>> + *
>>>>> + * @get_mbus_config: get the media bus configuration of a remote sub-
> device.
>>>>> + * The media bus configuration is usually retrieved
> from the
>>>>> + * firmware interface at sub-device probe time,
> immediately
>>>>> + * applied to the hardware and eventually adjusted
> by the
>>>>> + * driver. Remote sub-devices (usually video
> receivers) shall
>>>>> + * use this operation to query the transmitting end
> bus
>>>>> + * configuration in order to adjust their own one
> accordingly.
>>>>> + * Callers should make sure they get the most up-to-
> date as
>>>>> + * possible configuration from the remote end, likely
> calling
>>>>> + * this operation as close as possible to stream on
> time. The
>>>>> + * operation shall fail if the pad index it has been
> called on
>>>>> + * is not valid.
>>>>> + *
>>>>> + * @set_mbus_config: set the media bus configuration of a remote sub-
> device.
>>>>> + * This operations is intended to allow, in
> combination with
>>>>> + * the get_mbus_config operation, the negotiation of
> media bus
>>>>> + * configuration parameters between media sub-
> devices. The
>>>>> + * operation shall not fail if the requested
> configuration is
>>>>> + * not supported, but the driver shall update the
> content of
>>>>> + * the %config argument to reflect what has been
> actually
>>>>> + * applied to the hardware. The operation shall fail
> if the
>>>>> + * pad index it has been called on is not valid.
>>>>> */
>>>>
>>>> Hm, I'd hoped I could merge this, but I think include/media/v4l2-
> mediabus.h
>>>> also needs to be updated.
>>>>
>>>> So the old g_mbus_config returned all supported configurations, while the
>>>> new get_mbus_config returns the *current* configuration.
>>>>
>>>> That's fine, but that means that the meaning of the struct v4l2_mbus_config
>>>> flags field changes as well and several comments in v4l2-mediabus.h need to
>>>> be updated to reflect this.
>>>>
>>>> E.g. instead of '/* How many lanes the client can use */' it becomes
>>>> '/* How many lanes the client uses */'.
>>>>
>>>> Frankly, these flags can be redesigned now since you only need a single
>>>> e.g. V4L2_MBUS_HSYNC_ACTIVE_HIGH flag since if it is not set, then that
>>>> means ACTIVE_LOW. And since it is now a single bit, it is also easy to
>>>> make each flag a bit field.
>>>
>>> Even if this makes sense for .get_mbus_config() which returns current
>>> configuration, how about keeping the old semantics of the flags and let
>>> .set_mbus_config() accept a potentially incomplete or redundant set of flags
>>> specified by the caller to select a supported configuration from? That
> approach
>>> was actually proposed before by Jacopo when he argued against my
> suggestion to
>>> add a wrapper with a check for mutually exclusive flags[1], and I found it
> a
>>> very good alternative to my other rejected suggestion of adding TRY
> support.
>>
>> The information on how a sensor (or similar device) is wired up is not
> something
>> that should be negotiated. Even if a combination is theoretically possible,
> it
>> may not have been tested by the board designer and in fact it might not
> work.
>> (Yes, that happens)
>>
>> It is just a bad design trying to negotiate this.
>>
>> In fact, the only values that can be set as far as I am concerned are lanes
> and
>> channels. I wouldn't mind if the other settings are purely read-only. The
> only
>> driver that actively sets this is the pxa_camera driver and I wish it
> didn't.
>>
>> But there are still two pxa boards that use this mechanism, so I guess we
> still
>> have to allow this.
>>
>> Anyway, do you have a specific use-case in mind?
>
> Non-DT platforms in general. You repeat quite often that configuration
> should come from DT. While that's obvious, does that mean media is soon going
> to drop non-DT support completely? If not then I think that it may be better
It's close to dropping non-DT already. The ov6650 is the only sensor driver that
supports s_mbus_config today, so trying to negotiate these settings is only possible
with that sensor. In any case, this is really not something that should be negotiated,
regardless of DT vs platform. Negotiating this was and is a bad idea and instead you
should have a well defined and tested configuration defined in either the DT or in
platform data.
> to allow negotiation where possible than require each platform driver that
> still wishes to support non-DT platforms to define its own platform data
> structure. At least parallel link settings seem to be a good candidate to me.
>
> Anyway, as long as .set_mbus_config() is going to be supported, it is still
> possible for a platform driver to iterate through its supported configurations
> if passing incomplete or redundant flags and let the sensor handle that is not
> allowed. More complicated, more time consuming, more error prone, I believe,
> but still possible.
It's possible, but it is just not a good idea. As I mentioned before, typically
only one combination is actually tested, and negotiating this might end up
choosing a combination of settings that, while valid, is actually not working
due to HW bugs that were never noticed because the designer never tested that
combination. The other reason is that this makes things so much more complicated.
It is much easier and safer to just be explicit in the dts or platform data.
Regards,
Hans
>
> Thanks,
> Janusz
