| Message ID | 20240221160215.484151-2-panikiel@google.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v2,1/9] media: v4l2-subdev: Add a pad variant of .query_dv_timings() | expand |
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,
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?
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
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?
On 2/29/24 12:33, Paweł Anikiel wrote: > 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? I think I prefer a separate patchset for this. Regards, Hans
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,
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(+)