Re: [PATCH v2 1/9] media: v4l2-subdev: Add a pad variant of .query_dv_timings()

From: Hans Verkuil <hverkuil-cisco@xs4all.nl>
To: "Paweł Anikiel" <panikiel@google.com>
Cc: airlied@gmail.com, akpm@linux-foundation.org,
	conor+dt@kernel.org, daniel@ffwll.ch, dinguyen@kernel.org,
	krzysztof.kozlowski+dt@linaro.org,
	maarten.lankhorst@linux.intel.com, mchehab@kernel.org,
	mripard@kernel.org, robh+dt@kernel.org, tzimmermann@suse.de,
	devicetree@vger.kernel.org, dri-devel@lists.freedesktop.org,
	linux-kernel@vger.kernel.org, linux-media@vger.kernel.org,
	chromeos-krk-upstreaming@google.com, ribalda@chromium.org
Subject: Re: [PATCH v2 1/9] media: v4l2-subdev: Add a pad variant of .query_dv_timings()
Date: Thu, 29 Feb 2024 09:02:49 +0100	[thread overview]
Message-ID: <03f65fbc-9cf8-4347-8277-e53cb01b00a5@xs4all.nl> (raw)
In-Reply-To: <CAM5zL5qrMNfyiXMOJHUzLySm_U2U8kbD=D_Cyn0WjkvpikiYpQ@mail.gmail.com>

On 28/02/2024 16:34, Paweł Anikiel wrote:
> On Wed, Feb 28, 2024 at 12:25 PM Hans Verkuil <hverkuil-cisco@xs4all.nl> wrote:
>>
>> Hi Paweł,
>>
>> On 21/02/2024 17:02, Paweł Anikiel wrote:
>>> Currently, .query_dv_timings() is defined as a video callback without
>>> a pad argument. This is a problem if the subdevice can have different
>>> dv timings for each pad (e.g. a DisplayPort receiver with multiple
>>> virtual channels).
>>>
>>> To solve this, add a pad variant of this callback which includes
>>> the pad number as an argument.
>>
>> So now we have two query_dv_timings ops: one for video ops, and one
>> for pad ops. That's not very maintainable. I would suggest switching
>> all current users of the video op over to the pad op.
> 
> I agree it would be better if there was only one. However, I have some concerns:
> 1. Isn't there a problem with backwards compatibility? For example, an
> out-of-tree driver is likely to use this callback, which would break.
> I'm asking because I'm not familiar with how such API changes are
> handled.

It's out of tree, so they will just have to adapt. That's how life is if
you are not part of the mainline kernel.

> 2. If I do switch all current users to the pad op, I can't test those
> changes. Isn't that a problem?

I can test one or two drivers, but in general I don't expect this to be
a problem.

> 3. Would I need to get ACK from all the driver maintainers?

CC the patches to the maintainers. Generally you will get back Acks from
some but not all maintainers, but that's OK. For changes affecting multiple
drivers you never reach 100% on that. I can review the remainder. The DV
Timings API is my expert area, so that shouldn't be a problem.

A quick grep gives me these subdev drivers that implement it:

adv748x, adv7604, adv7842, tc358743, tda1997x, tvp7002, gs1662.

And these bridge drivers that call the subdevs:

cobalt, rcar-vin, vpif_capture.

The bridge drivers can use the following pad when calling query_dv_timings:

cobalt: ADV76XX_PAD_HDMI_PORT_A
rcar_vin: vin->parallel.sink_pad
vpif_capture: 0

The converted subdev drivers should check if the pad is an input pad.
Ideally it should check if the pad is equal to the current input pad
since most devices can only query the timings for the currently selected
input pad. But some older drivers predate the pad concept and they
ignore the pad value.

Regards,

	Hans