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

From: "Paweł Anikiel" <panikiel@google.com>
To: Hans Verkuil <hverkuil-cisco@xs4all.nl>
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 12:33:24 +0100	[thread overview]
Message-ID: <CAM5zL5r5JtA2HojaYZkSfUvoMTSNWALQM8HVuuXq-Znu7+TvGw@mail.gmail.com> (raw)
In-Reply-To: <03f65fbc-9cf8-4347-8277-e53cb01b00a5@xs4all.nl>

On Thu, Feb 29, 2024 at 9:02 AM Hans Verkuil <hverkuil-cisco@xs4all.nl> wrote:
>
> 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.

Thank you for the helpful info. I will switch all these drivers to the
pad op, then. Would you like me to prepare a separate patchset, or
should I include the changes in this one?