diff mbox series

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

Message ID 20240221160215.484151-2-panikiel@google.com
State New
Headers
Series Add Chameleon v3 video support |

Commit Message

Paweł Anikiel Feb. 21, 2024, 4:02 p.m. UTC 
  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.

Signed-off-by: Paweł Anikiel <panikiel@google.com>
---
 drivers/media/v4l2-core/v4l2-subdev.c | 11 +++++++++++
 include/media/v4l2-subdev.h           |  5 +++++
 2 files changed, 16 insertions(+)

Comments

Hans Verkuil Feb. 28, 2024, 11:25 a.m. UTC | #1 
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.

Regards,

	Hans

> 
> Signed-off-by: Paweł Anikiel <panikiel@google.com>
> ---
>  drivers/media/v4l2-core/v4l2-subdev.c | 11 +++++++++++
>  include/media/v4l2-subdev.h           |  5 +++++
>  2 files changed, 16 insertions(+)
> 
> diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c
> index 4c6198c48dd6..11f865dd19b4 100644
> --- a/drivers/media/v4l2-core/v4l2-subdev.c
> +++ b/drivers/media/v4l2-core/v4l2-subdev.c
> @@ -389,6 +389,16 @@ static int call_enum_dv_timings(struct v4l2_subdev *sd,
>  	       sd->ops->pad->enum_dv_timings(sd, dvt);
>  }
>  
> +static int call_query_dv_timings(struct v4l2_subdev *sd, unsigned int pad,
> +				 struct v4l2_dv_timings *timings)
> +{
> +	if (!timings)
> +		return -EINVAL;
> +
> +	return check_pad(sd, pad) ? :
> +	       sd->ops->pad->query_dv_timings(sd, pad, timings);
> +}
> +
>  static int call_get_mbus_config(struct v4l2_subdev *sd, unsigned int pad,
>  				struct v4l2_mbus_config *config)
>  {
> @@ -489,6 +499,7 @@ static const struct v4l2_subdev_pad_ops v4l2_subdev_call_pad_wrappers = {
>  	.set_edid		= call_set_edid,
>  	.dv_timings_cap		= call_dv_timings_cap,
>  	.enum_dv_timings	= call_enum_dv_timings,
> +	.query_dv_timings	= call_query_dv_timings,
>  	.get_frame_desc		= call_get_frame_desc,
>  	.get_mbus_config	= call_get_mbus_config,
>  };
> diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
> index a9e6b8146279..dc8963fa5a06 100644
> --- a/include/media/v4l2-subdev.h
> +++ b/include/media/v4l2-subdev.h
> @@ -797,6 +797,9 @@ struct v4l2_subdev_state {
>   * @enum_dv_timings: callback for VIDIOC_SUBDEV_ENUM_DV_TIMINGS() ioctl handler
>   *		     code.
>   *
> + * @query_dv_timings: same as query_dv_timings() from v4l2_subdev_video_ops,
> + *		      but with additional pad argument.
> + *
>   * @link_validate: used by the media controller code to check if the links
>   *		   that belongs to a pipeline can be used for stream.
>   *
> @@ -868,6 +871,8 @@ struct v4l2_subdev_pad_ops {
>  			      struct v4l2_dv_timings_cap *cap);
>  	int (*enum_dv_timings)(struct v4l2_subdev *sd,
>  			       struct v4l2_enum_dv_timings *timings);
> +	int (*query_dv_timings)(struct v4l2_subdev *sd, unsigned int pad,
> +				struct v4l2_dv_timings *timings);
>  #ifdef CONFIG_MEDIA_CONTROLLER
>  	int (*link_validate)(struct v4l2_subdev *sd, struct media_link *link,
>  			     struct v4l2_subdev_format *source_fmt,
Paweł Anikiel Feb. 28, 2024, 3:34 p.m. UTC | #2 
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.
2. If I do switch all current users to the pad op, I can't test those
changes. Isn't that a problem?
3. Would I need to get ACK from all the driver maintainers?
Hans Verkuil Feb. 29, 2024, 8:02 a.m. UTC | #3 
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
diff mbox series

Patch

diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c
index 4c6198c48dd6..11f865dd19b4 100644
--- a/drivers/media/v4l2-core/v4l2-subdev.c
+++ b/drivers/media/v4l2-core/v4l2-subdev.c
@@ -389,6 +389,16 @@  static int call_enum_dv_timings(struct v4l2_subdev *sd,
 	       sd->ops->pad->enum_dv_timings(sd, dvt);
 }
 
+static int call_query_dv_timings(struct v4l2_subdev *sd, unsigned int pad,
+				 struct v4l2_dv_timings *timings)
+{
+	if (!timings)
+		return -EINVAL;
+
+	return check_pad(sd, pad) ? :
+	       sd->ops->pad->query_dv_timings(sd, pad, timings);
+}
+
 static int call_get_mbus_config(struct v4l2_subdev *sd, unsigned int pad,
 				struct v4l2_mbus_config *config)
 {
@@ -489,6 +499,7 @@  static const struct v4l2_subdev_pad_ops v4l2_subdev_call_pad_wrappers = {
 	.set_edid		= call_set_edid,
 	.dv_timings_cap		= call_dv_timings_cap,
 	.enum_dv_timings	= call_enum_dv_timings,
+	.query_dv_timings	= call_query_dv_timings,
 	.get_frame_desc		= call_get_frame_desc,
 	.get_mbus_config	= call_get_mbus_config,
 };
diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
index a9e6b8146279..dc8963fa5a06 100644
--- a/include/media/v4l2-subdev.h
+++ b/include/media/v4l2-subdev.h
@@ -797,6 +797,9 @@  struct v4l2_subdev_state {
  * @enum_dv_timings: callback for VIDIOC_SUBDEV_ENUM_DV_TIMINGS() ioctl handler
  *		     code.
  *
+ * @query_dv_timings: same as query_dv_timings() from v4l2_subdev_video_ops,
+ *		      but with additional pad argument.
+ *
  * @link_validate: used by the media controller code to check if the links
  *		   that belongs to a pipeline can be used for stream.
  *
@@ -868,6 +871,8 @@  struct v4l2_subdev_pad_ops {
 			      struct v4l2_dv_timings_cap *cap);
 	int (*enum_dv_timings)(struct v4l2_subdev *sd,
 			       struct v4l2_enum_dv_timings *timings);
+	int (*query_dv_timings)(struct v4l2_subdev *sd, unsigned int pad,
+				struct v4l2_dv_timings *timings);
 #ifdef CONFIG_MEDIA_CONTROLLER
 	int (*link_validate)(struct v4l2_subdev *sd, struct media_link *link,
 			     struct v4l2_subdev_format *source_fmt,