[PATCH 1/3] v4l2-subdev: Add subdev ioctl support for ENUM/GET/SET INPUT

[PATCH 1/3] v4l2-subdev: Add subdev ioctl support for ENUM/GET/SET INPUT

[Ramzi BEN MEFTAH]

From: Steve Longerbeam <steve_longerbeam@mentor.com>

This commit enables VIDIOC_ENUMINPUT, VIDIOC_G_INPUT, and VIDIOC_S_INPUT
ioctls for use via v4l2 subdevice node.

This commit should probably not be pushed upstream, because the (old)
idea of video inputs conflicts with the newer concept of establishing
media links between src->sink pads.

However it might make sense for some subdevices to support enum/get/set
inputs. One example would be the analog front end subdevice for the
ADV748x. By providing these ioctls, selecting the ADV748x analog inputs
can be done without requiring the implementation of media entities that
would define the analog source for which to establish a media link.

Signed-off-by: Steve Longerbeam <steve_longerbeam@mentor.com>
---
 drivers/media/v4l2-core/v4l2-subdev.c |  9 +++++++++
 include/media/v4l2-subdev.h           | 11 +++++++++++
 2 files changed, 20 insertions(+)

diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c
index 6b989fe..73fbfe9 100644
--- a/drivers/media/v4l2-core/v4l2-subdev.c
+++ b/drivers/media/v4l2-core/v4l2-subdev.c
@@ -378,6 +378,15 @@ static long subdev_do_ioctl(struct file *file, unsigned int cmd, void *arg)
 			return -ENOTTY;
 		return v4l2_querymenu(vfh->ctrl_handler, arg);
 
+	case VIDIOC_ENUMINPUT:
+		return v4l2_subdev_call(sd, video, enuminput, arg);
+
+	case VIDIOC_G_INPUT:
+		return v4l2_subdev_call(sd, video, g_input, arg);
+
+	case VIDIOC_S_INPUT:
+		return v4l2_subdev_call(sd, video, s_input, *(u32 *)arg);
+
 	case VIDIOC_G_CTRL:
 		if (!vfh->ctrl_handler)
 			return -ENOTTY;
diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
index f7fe78a..6e1a9cd 100644
--- a/include/media/v4l2-subdev.h
+++ b/include/media/v4l2-subdev.h
@@ -383,6 +383,14 @@ struct v4l2_mbus_frame_desc {
  * @g_input_status: get input status. Same as the status field in the
  *	&struct &v4l2_input
  *
+ * @enuminput: enumerate inputs. Should return the same input status as
+ *      @g_input_status if the passed input index is the currently active
+ *      input.
+ *
+ * @g_input: returns the currently active input index.
+ *
+ * @s_input: set the active input.
+ *
  * @s_stream: used to notify the driver that a video stream will start or has
  *	stopped.
  *
@@ -423,6 +431,9 @@ struct v4l2_subdev_video_ops {
 	int (*g_tvnorms)(struct v4l2_subdev *sd, v4l2_std_id *std);
 	int (*g_tvnorms_output)(struct v4l2_subdev *sd, v4l2_std_id *std);
 	int (*g_input_status)(struct v4l2_subdev *sd, u32 *status);
+	int (*enuminput)(struct v4l2_subdev *sd, struct v4l2_input *input);
+	int (*g_input)(struct v4l2_subdev *sd, u32 *index);
+	int (*s_input)(struct v4l2_subdev *sd, u32 index);
 	int (*s_stream)(struct v4l2_subdev *sd, int enable);
 	int (*g_pixelaspect)(struct v4l2_subdev *sd, struct v4l2_fract *aspect);
 	int (*g_frame_interval)(struct v4l2_subdev *sd,
-- 
2.7.4

[PATCH 2/3] media: i2c: adv748x-afe: Implement enum/get/set input

[Ramzi BEN MEFTAH]

From: Steve Longerbeam <steve_longerbeam@mentor.com>

The adv748x-afe sub-device driver does not support changing the
analog input, so enuminput returns only the status of a single
input at index=0. Likewise g_input returns only index 0, and s_input
returns -EINVAL if index is not 0, and otherwise does nothing.

Signed-off-by: Steve Longerbeam <steve_longerbeam@mentor.com>
(cherry picked from ADIT v4.14 commit 8aadc35d3ae252a1eaed8506fbd1675911465bbd)
---
 drivers/media/i2c/adv748x/adv748x-afe.c | 42 ++++++++++++++++++++++++++++++---
 1 file changed, 39 insertions(+), 3 deletions(-)

diff --git a/drivers/media/i2c/adv748x/adv748x-afe.c b/drivers/media/i2c/adv748x/adv748x-afe.c
index dbbb1e4..6b090f4 100644
--- a/drivers/media/i2c/adv748x/adv748x-afe.c
+++ b/drivers/media/i2c/adv748x/adv748x-afe.c
@@ -154,7 +154,7 @@ static void adv748x_afe_set_video_standard(struct adv748x_state *state,
 		   (sdpstd & 0xf) << ADV748X_SDP_VID_SEL_SHIFT);
 }
 
-static int adv748x_afe_s_input(struct adv748x_afe *afe, unsigned int input)
+static int adv748x_afe_set_input(struct adv748x_afe *afe, unsigned int input)
 {
 	struct adv748x_state *state = adv748x_afe_to_state(afe);
 
@@ -267,6 +267,39 @@ static int adv748x_afe_g_input_status(struct v4l2_subdev *sd, u32 *status)
 	return ret;
 }
 
+static int adv748x_afe_enuminput(struct v4l2_subdev *sd,
+				 struct v4l2_input *input)
+{
+	struct adv748x_afe *afe = adv748x_sd_to_afe(sd);
+
+	if (input->index != 0)
+		return -EINVAL;
+
+	input->type = V4L2_INPUT_TYPE_CAMERA;
+	input->capabilities = V4L2_IN_CAP_STD;
+	input->status = V4L2_IN_ST_NO_SIGNAL;
+	/* API says we must return all supported standards */
+	input->std = V4L2_STD_ALL;
+
+	snprintf(input->name, sizeof(input->name), "%s AIN%u",
+		 sd->name, afe->input);
+
+	return adv748x_afe_g_input_status(sd, &input->status);
+}
+
+static int adv748x_afe_g_input(struct v4l2_subdev *sd, u32 *index)
+{
+	*index = 0;
+	return 0;
+}
+
+static int adv748x_afe_s_input(struct v4l2_subdev *sd, u32 index)
+{
+	if (index != 0)
+		return -EINVAL;
+	return 0;
+}
+
 static int adv748x_afe_s_stream(struct v4l2_subdev *sd, int enable)
 {
 	struct adv748x_afe *afe = adv748x_sd_to_afe(sd);
@@ -277,7 +310,7 @@ static int adv748x_afe_s_stream(struct v4l2_subdev *sd, int enable)
 	mutex_lock(&state->mutex);
 
 	if (enable) {
-		ret = adv748x_afe_s_input(afe, afe->input);
+		ret = adv748x_afe_set_input(afe, afe->input);
 		if (ret)
 			goto unlock;
 	}
@@ -306,6 +339,9 @@ static const struct v4l2_subdev_video_ops adv748x_afe_video_ops = {
 	.querystd = adv748x_afe_querystd,
 	.g_tvnorms = adv748x_afe_g_tvnorms,
 	.g_input_status = adv748x_afe_g_input_status,
+	.enuminput = adv748x_afe_enuminput,
+	.g_input = adv748x_afe_g_input,
+	.s_input = adv748x_afe_s_input,
 	.s_stream = adv748x_afe_s_stream,
 	.g_pixelaspect = adv748x_afe_g_pixelaspect,
 };
@@ -520,7 +556,7 @@ int adv748x_afe_init(struct adv748x_afe *afe)
 		}
 	}
 
-	adv748x_afe_s_input(afe, afe->input);
+	adv748x_afe_set_input(afe, afe->input);
 
 	adv_dbg(state, "AFE Default input set to %d\n", afe->input);
 
-- 
2.7.4

[PATCH 3/3] media: i2c: adv748x: add enuminput control to adv748x hdmi subdev

[Ramzi BEN MEFTAH]

This patch adds support for enuminput control to the adv748x hdmi subdev.
This will allow userspace for example to query input hdmi signal status.

Signed-off-by: Ramzi BEN MEFTAH <rbmeftah@de.adit-jv.com>
---
 drivers/media/i2c/adv748x/adv748x-hdmi.c | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/drivers/media/i2c/adv748x/adv748x-hdmi.c b/drivers/media/i2c/adv748x/adv748x-hdmi.c
index c557f8f..2d748b2 100644
--- a/drivers/media/i2c/adv748x/adv748x-hdmi.c
+++ b/drivers/media/i2c/adv748x/adv748x-hdmi.c
@@ -350,6 +350,22 @@ static int adv748x_hdmi_g_input_status(struct v4l2_subdev *sd, u32 *status)
 	return 0;
 }
 
+static int adv748x_hdmi_enuminput(struct v4l2_subdev *sd,
+				 struct v4l2_input *input)
+{
+	if (input->index != 0)
+		return -EINVAL;
+
+	input->type = V4L2_INPUT_TYPE_CAMERA;
+	input->capabilities = V4L2_IN_CAP_DV_TIMINGS;
+	input->status = V4L2_IN_ST_NO_SIGNAL;
+	input->std = V4L2_STD_UNKNOWN;
+
+	snprintf(input->name, sizeof(input->name), "%s", sd->name);
+
+	return adv748x_hdmi_g_input_status(sd, &input->status);
+}
+
 static int adv748x_hdmi_s_stream(struct v4l2_subdev *sd, int enable)
 {
 	struct adv748x_hdmi *hdmi = adv748x_sd_to_hdmi(sd);
@@ -386,6 +402,7 @@ static const struct v4l2_subdev_video_ops adv748x_video_ops_hdmi = {
 	.g_dv_timings = adv748x_hdmi_g_dv_timings,
 	.query_dv_timings = adv748x_hdmi_query_dv_timings,
 	.g_input_status = adv748x_hdmi_g_input_status,
+	.enuminput = adv748x_hdmi_enuminput,
 	.s_stream = adv748x_hdmi_s_stream,
 	.g_pixelaspect = adv748x_hdmi_g_pixelaspect,
 };
-- 
2.7.4

Re: [PATCH 1/3] v4l2-subdev: Add subdev ioctl support for ENUM/GET/SET INPUT

[Jacopo Mondi]

Hello

On Tue, Jun 16, 2020 at 12:00:15PM +0200, Ramzi BEN MEFTAH wrote:
> From: Steve Longerbeam <steve_longerbeam@mentor.com>
>

 +Niklas, +Laurent

Niklas, Laurent, how does this play with CAP_IO_MC ?

Thanks
  j

> This commit enables VIDIOC_ENUMINPUT, VIDIOC_G_INPUT, and VIDIOC_S_INPUT
> ioctls for use via v4l2 subdevice node.
>
> This commit should probably not be pushed upstream, because the (old)
> idea of video inputs conflicts with the newer concept of establishing
> media links between src->sink pads.
>
> However it might make sense for some subdevices to support enum/get/set
> inputs. One example would be the analog front end subdevice for the
> ADV748x. By providing these ioctls, selecting the ADV748x analog inputs
> can be done without requiring the implementation of media entities that
> would define the analog source for which to establish a media link.
>
> Signed-off-by: Steve Longerbeam <steve_longerbeam@mentor.com>
> ---
>  drivers/media/v4l2-core/v4l2-subdev.c |  9 +++++++++
>  include/media/v4l2-subdev.h           | 11 +++++++++++
>  2 files changed, 20 insertions(+)
>
> diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c
> index 6b989fe..73fbfe9 100644
> --- a/drivers/media/v4l2-core/v4l2-subdev.c
> +++ b/drivers/media/v4l2-core/v4l2-subdev.c
> @@ -378,6 +378,15 @@ static long subdev_do_ioctl(struct file *file, unsigned int cmd, void *arg)
>  			return -ENOTTY;
>  		return v4l2_querymenu(vfh->ctrl_handler, arg);
>
> +	case VIDIOC_ENUMINPUT:
> +		return v4l2_subdev_call(sd, video, enuminput, arg);
> +
> +	case VIDIOC_G_INPUT:
> +		return v4l2_subdev_call(sd, video, g_input, arg);
> +
> +	case VIDIOC_S_INPUT:
> +		return v4l2_subdev_call(sd, video, s_input, *(u32 *)arg);
> +
>  	case VIDIOC_G_CTRL:
>  		if (!vfh->ctrl_handler)
>  			return -ENOTTY;
> diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
> index f7fe78a..6e1a9cd 100644
> --- a/include/media/v4l2-subdev.h
> +++ b/include/media/v4l2-subdev.h
> @@ -383,6 +383,14 @@ struct v4l2_mbus_frame_desc {
>   * @g_input_status: get input status. Same as the status field in the
>   *	&struct &v4l2_input
>   *
> + * @enuminput: enumerate inputs. Should return the same input status as
> + *      @g_input_status if the passed input index is the currently active
> + *      input.
> + *
> + * @g_input: returns the currently active input index.
> + *
> + * @s_input: set the active input.
> + *
>   * @s_stream: used to notify the driver that a video stream will start or has
>   *	stopped.
>   *
> @@ -423,6 +431,9 @@ struct v4l2_subdev_video_ops {
>  	int (*g_tvnorms)(struct v4l2_subdev *sd, v4l2_std_id *std);
>  	int (*g_tvnorms_output)(struct v4l2_subdev *sd, v4l2_std_id *std);
>  	int (*g_input_status)(struct v4l2_subdev *sd, u32 *status);
> +	int (*enuminput)(struct v4l2_subdev *sd, struct v4l2_input *input);
> +	int (*g_input)(struct v4l2_subdev *sd, u32 *index);
> +	int (*s_input)(struct v4l2_subdev *sd, u32 index);
>  	int (*s_stream)(struct v4l2_subdev *sd, int enable);
>  	int (*g_pixelaspect)(struct v4l2_subdev *sd, struct v4l2_fract *aspect);
>  	int (*g_frame_interval)(struct v4l2_subdev *sd,
> --
> 2.7.4
>

Re: [PATCH 1/3] v4l2-subdev: Add subdev ioctl support for ENUM/GET/SET INPUT

[Laurent Pinchart]

Hi Jacopo,

On Wed, Jun 24, 2020 at 09:53:07AM +0200, Jacopo Mondi wrote:
> On Tue, Jun 16, 2020 at 12:00:15PM +0200, Ramzi BEN MEFTAH wrote:
> > From: Steve Longerbeam <steve_longerbeam@mentor.com>
> 
>  +Niklas, +Laurent
> 
> Niklas, Laurent, how does this play with CAP_IO_MC ?

I don't think it's related to CAP_IO_MC, but I don't think it's a good
idea either :-) Routing doesn't go through the subdev [gs]_input
operations in MC-based drivers. It should be configured through link
setup instead. This patch goes in the wrong direction, sorry Steve.

> > This commit enables VIDIOC_ENUMINPUT, VIDIOC_G_INPUT, and VIDIOC_S_INPUT
> > ioctls for use via v4l2 subdevice node.
> >
> > This commit should probably not be pushed upstream, because the (old)
> > idea of video inputs conflicts with the newer concept of establishing
> > media links between src->sink pads.
> >
> > However it might make sense for some subdevices to support enum/get/set
> > inputs. One example would be the analog front end subdevice for the
> > ADV748x. By providing these ioctls, selecting the ADV748x analog inputs
> > can be done without requiring the implementation of media entities that
> > would define the analog source for which to establish a media link.
> >
> > Signed-off-by: Steve Longerbeam <steve_longerbeam@mentor.com>
> > ---
> >  drivers/media/v4l2-core/v4l2-subdev.c |  9 +++++++++
> >  include/media/v4l2-subdev.h           | 11 +++++++++++
> >  2 files changed, 20 insertions(+)
> >
> > diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c
> > index 6b989fe..73fbfe9 100644
> > --- a/drivers/media/v4l2-core/v4l2-subdev.c
> > +++ b/drivers/media/v4l2-core/v4l2-subdev.c
> > @@ -378,6 +378,15 @@ static long subdev_do_ioctl(struct file *file, unsigned int cmd, void *arg)
> >  			return -ENOTTY;
> >  		return v4l2_querymenu(vfh->ctrl_handler, arg);
> >
> > +	case VIDIOC_ENUMINPUT:
> > +		return v4l2_subdev_call(sd, video, enuminput, arg);
> > +
> > +	case VIDIOC_G_INPUT:
> > +		return v4l2_subdev_call(sd, video, g_input, arg);
> > +
> > +	case VIDIOC_S_INPUT:
> > +		return v4l2_subdev_call(sd, video, s_input, *(u32 *)arg);
> > +
> >  	case VIDIOC_G_CTRL:
> >  		if (!vfh->ctrl_handler)
> >  			return -ENOTTY;
> > diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
> > index f7fe78a..6e1a9cd 100644
> > --- a/include/media/v4l2-subdev.h
> > +++ b/include/media/v4l2-subdev.h
> > @@ -383,6 +383,14 @@ struct v4l2_mbus_frame_desc {
> >   * @g_input_status: get input status. Same as the status field in the
> >   *	&struct &v4l2_input
> >   *
> > + * @enuminput: enumerate inputs. Should return the same input status as
> > + *      @g_input_status if the passed input index is the currently active
> > + *      input.
> > + *
> > + * @g_input: returns the currently active input index.
> > + *
> > + * @s_input: set the active input.
> > + *
> >   * @s_stream: used to notify the driver that a video stream will start or has
> >   *	stopped.
> >   *
> > @@ -423,6 +431,9 @@ struct v4l2_subdev_video_ops {
> >  	int (*g_tvnorms)(struct v4l2_subdev *sd, v4l2_std_id *std);
> >  	int (*g_tvnorms_output)(struct v4l2_subdev *sd, v4l2_std_id *std);
> >  	int (*g_input_status)(struct v4l2_subdev *sd, u32 *status);
> > +	int (*enuminput)(struct v4l2_subdev *sd, struct v4l2_input *input);
> > +	int (*g_input)(struct v4l2_subdev *sd, u32 *index);
> > +	int (*s_input)(struct v4l2_subdev *sd, u32 index);
> >  	int (*s_stream)(struct v4l2_subdev *sd, int enable);
> >  	int (*g_pixelaspect)(struct v4l2_subdev *sd, struct v4l2_fract *aspect);
> >  	int (*g_frame_interval)(struct v4l2_subdev *sd,

-- 
Regards,

Laurent Pinchart

Re: [PATCH 1/3] v4l2-subdev: Add subdev ioctl support for ENUM/GET/SET INPUT

[Ramzi Ben Meftah]

Hi Laurent,

On Thu, Jun 25, 2020 at 05:01:38AM +0300, Laurent Pinchart wrote:
> Hi Jacopo,
> 
> On Wed, Jun 24, 2020 at 09:53:07AM +0200, Jacopo Mondi wrote:
> > On Tue, Jun 16, 2020 at 12:00:15PM +0200, Ramzi BEN MEFTAH wrote:
> > > From: Steve Longerbeam <steve_longerbeam@mentor.com>
> > 
> >  +Niklas, +Laurent
> > 
> > Niklas, Laurent, how does this play with CAP_IO_MC ?
> 
> I don't think it's related to CAP_IO_MC, but I don't think it's a good
> idea either :-) Routing doesn't go through the subdev [gs]_input
> operations in MC-based drivers. It should be configured through link
> setup instead. This patch goes in the wrong direction, sorry Steve.

ENUMINPUT ioctl allow to get the input signal status. Is there an alternative
with Media Controller?

> 
> > > This commit enables VIDIOC_ENUMINPUT, VIDIOC_G_INPUT, and VIDIOC_S_INPUT
> > > ioctls for use via v4l2 subdevice node.
> > >
> > > This commit should probably not be pushed upstream, because the (old)
> > > idea of video inputs conflicts with the newer concept of establishing
> > > media links between src->sink pads.
> > >
> > > However it might make sense for some subdevices to support enum/get/set
> > > inputs. One example would be the analog front end subdevice for the
> > > ADV748x. By providing these ioctls, selecting the ADV748x analog inputs
> > > can be done without requiring the implementation of media entities that
> > > would define the analog source for which to establish a media link.
> > >
> > > Signed-off-by: Steve Longerbeam <steve_longerbeam@mentor.com>
> > > ---
> > >  drivers/media/v4l2-core/v4l2-subdev.c |  9 +++++++++
> > >  include/media/v4l2-subdev.h           | 11 +++++++++++
> > >  2 files changed, 20 insertions(+)
> > >
> > > diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c
> > > index 6b989fe..73fbfe9 100644
> > > --- a/drivers/media/v4l2-core/v4l2-subdev.c
> > > +++ b/drivers/media/v4l2-core/v4l2-subdev.c
> > > @@ -378,6 +378,15 @@ static long subdev_do_ioctl(struct file *file, unsigned int cmd, void *arg)
> > >  			return -ENOTTY;
> > >  		return v4l2_querymenu(vfh->ctrl_handler, arg);
> > >
> > > +	case VIDIOC_ENUMINPUT:
> > > +		return v4l2_subdev_call(sd, video, enuminput, arg);
> > > +
> > > +	case VIDIOC_G_INPUT:
> > > +		return v4l2_subdev_call(sd, video, g_input, arg);
> > > +
> > > +	case VIDIOC_S_INPUT:
> > > +		return v4l2_subdev_call(sd, video, s_input, *(u32 *)arg);
> > > +
> > >  	case VIDIOC_G_CTRL:
> > >  		if (!vfh->ctrl_handler)
> > >  			return -ENOTTY;
> > > diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
> > > index f7fe78a..6e1a9cd 100644
> > > --- a/include/media/v4l2-subdev.h
> > > +++ b/include/media/v4l2-subdev.h
> > > @@ -383,6 +383,14 @@ struct v4l2_mbus_frame_desc {
> > >   * @g_input_status: get input status. Same as the status field in the
> > >   *	&struct &v4l2_input
> > >   *
> > > + * @enuminput: enumerate inputs. Should return the same input status as
> > > + *      @g_input_status if the passed input index is the currently active
> > > + *      input.
> > > + *
> > > + * @g_input: returns the currently active input index.
> > > + *
> > > + * @s_input: set the active input.
> > > + *
> > >   * @s_stream: used to notify the driver that a video stream will start or has
> > >   *	stopped.
> > >   *
> > > @@ -423,6 +431,9 @@ struct v4l2_subdev_video_ops {
> > >  	int (*g_tvnorms)(struct v4l2_subdev *sd, v4l2_std_id *std);
> > >  	int (*g_tvnorms_output)(struct v4l2_subdev *sd, v4l2_std_id *std);
> > >  	int (*g_input_status)(struct v4l2_subdev *sd, u32 *status);
> > > +	int (*enuminput)(struct v4l2_subdev *sd, struct v4l2_input *input);
> > > +	int (*g_input)(struct v4l2_subdev *sd, u32 *index);
> > > +	int (*s_input)(struct v4l2_subdev *sd, u32 index);
> > >  	int (*s_stream)(struct v4l2_subdev *sd, int enable);
> > >  	int (*g_pixelaspect)(struct v4l2_subdev *sd, struct v4l2_fract *aspect);
> > >  	int (*g_frame_interval)(struct v4l2_subdev *sd,
> 
> -- 
> Regards,
> 
> Laurent Pinchart

-- 
Best Regards,
Ramzi Ben Meftah.

Re: [PATCH 1/3] v4l2-subdev: Add subdev ioctl support for ENUM/GET/SET INPUT

[Laurent Pinchart]

Hi Ramzi,

On Thu, Jun 25, 2020 at 11:30:46AM +0200, Ramzi Ben Meftah wrote:
> On Thu, Jun 25, 2020 at 05:01:38AM +0300, Laurent Pinchart wrote:
> > On Wed, Jun 24, 2020 at 09:53:07AM +0200, Jacopo Mondi wrote:
> >> On Tue, Jun 16, 2020 at 12:00:15PM +0200, Ramzi BEN MEFTAH wrote:
> >>> From: Steve Longerbeam <steve_longerbeam@mentor.com>
> >> 
> >>  +Niklas, +Laurent
> >> 
> >> Niklas, Laurent, how does this play with CAP_IO_MC ?
> > 
> > I don't think it's related to CAP_IO_MC, but I don't think it's a good
> > idea either :-) Routing doesn't go through the subdev [gs]_input
> > operations in MC-based drivers. It should be configured through link
> > setup instead. This patch goes in the wrong direction, sorry Steve.
> 
> ENUMINPUT ioctl allow to get the input signal status. Is there an alternative
> with Media Controller?

No there isn't at the moment. I'm not opposed to adding such a feature,
but VIDIOC_ENUMINPUT isn't the right choice. This would have to be a
subdev pad operation (v4l2_subdev_pad_ops), not a video operation
(v4l2_subdev_video_ops). We also likely shouldn't call it "enum" input,
as it would retrieve properties of the input corresponding to the pad,
not enumerate inputs.

> >>> This commit enables VIDIOC_ENUMINPUT, VIDIOC_G_INPUT, and VIDIOC_S_INPUT
> >>> ioctls for use via v4l2 subdevice node.
> >>>
> >>> This commit should probably not be pushed upstream, because the (old)
> >>> idea of video inputs conflicts with the newer concept of establishing
> >>> media links between src->sink pads.
> >>>
> >>> However it might make sense for some subdevices to support enum/get/set
> >>> inputs. One example would be the analog front end subdevice for the
> >>> ADV748x. By providing these ioctls, selecting the ADV748x analog inputs
> >>> can be done without requiring the implementation of media entities that
> >>> would define the analog source for which to establish a media link.
> >>>
> >>> Signed-off-by: Steve Longerbeam <steve_longerbeam@mentor.com>
> >>> ---
> >>>  drivers/media/v4l2-core/v4l2-subdev.c |  9 +++++++++
> >>>  include/media/v4l2-subdev.h           | 11 +++++++++++
> >>>  2 files changed, 20 insertions(+)
> >>>
> >>> diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c
> >>> index 6b989fe..73fbfe9 100644
> >>> --- a/drivers/media/v4l2-core/v4l2-subdev.c
> >>> +++ b/drivers/media/v4l2-core/v4l2-subdev.c
> >>> @@ -378,6 +378,15 @@ static long subdev_do_ioctl(struct file *file, unsigned int cmd, void *arg)
> >>>  			return -ENOTTY;
> >>>  		return v4l2_querymenu(vfh->ctrl_handler, arg);
> >>>
> >>> +	case VIDIOC_ENUMINPUT:
> >>> +		return v4l2_subdev_call(sd, video, enuminput, arg);
> >>> +
> >>> +	case VIDIOC_G_INPUT:
> >>> +		return v4l2_subdev_call(sd, video, g_input, arg);
> >>> +
> >>> +	case VIDIOC_S_INPUT:
> >>> +		return v4l2_subdev_call(sd, video, s_input, *(u32 *)arg);
> >>> +
> >>>  	case VIDIOC_G_CTRL:
> >>>  		if (!vfh->ctrl_handler)
> >>>  			return -ENOTTY;
> >>> diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
> >>> index f7fe78a..6e1a9cd 100644
> >>> --- a/include/media/v4l2-subdev.h
> >>> +++ b/include/media/v4l2-subdev.h
> >>> @@ -383,6 +383,14 @@ struct v4l2_mbus_frame_desc {
> >>>   * @g_input_status: get input status. Same as the status field in the
> >>>   *	&struct &v4l2_input
> >>>   *
> >>> + * @enuminput: enumerate inputs. Should return the same input status as
> >>> + *      @g_input_status if the passed input index is the currently active
> >>> + *      input.
> >>> + *
> >>> + * @g_input: returns the currently active input index.
> >>> + *
> >>> + * @s_input: set the active input.
> >>> + *
> >>>   * @s_stream: used to notify the driver that a video stream will start or has
> >>>   *	stopped.
> >>>   *
> >>> @@ -423,6 +431,9 @@ struct v4l2_subdev_video_ops {
> >>>  	int (*g_tvnorms)(struct v4l2_subdev *sd, v4l2_std_id *std);
> >>>  	int (*g_tvnorms_output)(struct v4l2_subdev *sd, v4l2_std_id *std);
> >>>  	int (*g_input_status)(struct v4l2_subdev *sd, u32 *status);
> >>> +	int (*enuminput)(struct v4l2_subdev *sd, struct v4l2_input *input);
> >>> +	int (*g_input)(struct v4l2_subdev *sd, u32 *index);
> >>> +	int (*s_input)(struct v4l2_subdev *sd, u32 index);
> >>>  	int (*s_stream)(struct v4l2_subdev *sd, int enable);
> >>>  	int (*g_pixelaspect)(struct v4l2_subdev *sd, struct v4l2_fract *aspect);
> >>>  	int (*g_frame_interval)(struct v4l2_subdev *sd,

-- 
Regards,

Laurent Pinchart

Re: [PATCH 1/3] v4l2-subdev: Add subdev ioctl support for ENUM/GET/SET INPUT

[Ramzi Ben Meftah]

Hi Laurent,

On Thu, Jun 25, 2020 at 12:47:24PM +0300, Laurent Pinchart wrote:
> Hi Ramzi,
> 
> On Thu, Jun 25, 2020 at 11:30:46AM +0200, Ramzi Ben Meftah wrote:
> > On Thu, Jun 25, 2020 at 05:01:38AM +0300, Laurent Pinchart wrote:
> > > On Wed, Jun 24, 2020 at 09:53:07AM +0200, Jacopo Mondi wrote:
> > >> On Tue, Jun 16, 2020 at 12:00:15PM +0200, Ramzi BEN MEFTAH wrote:
> > >>> From: Steve Longerbeam <steve_longerbeam@mentor.com>
> > >> 
> > >>  +Niklas, +Laurent
> > >> 
> > >> Niklas, Laurent, how does this play with CAP_IO_MC ?
> > > 
> > > I don't think it's related to CAP_IO_MC, but I don't think it's a good
> > > idea either :-) Routing doesn't go through the subdev [gs]_input
> > > operations in MC-based drivers. It should be configured through link
> > > setup instead. This patch goes in the wrong direction, sorry Steve.
> > 
> > ENUMINPUT ioctl allow to get the input signal status. Is there an alternative
> > with Media Controller?
> 
> No there isn't at the moment. I'm not opposed to adding such a feature,
> but VIDIOC_ENUMINPUT isn't the right choice. This would have to be a
> subdev pad operation (v4l2_subdev_pad_ops), not a video operation
> (v4l2_subdev_video_ops). We also likely shouldn't call it "enum" input,
> as it would retrieve properties of the input corresponding to the pad,
> not enumerate inputs.
> 

Looking to v4l2_subdev_pad_ops, there is g_input_status which seems to fulfill
this need. But, seems this is not expose to user space although many drivers
do implememt it.
Should I add VIDIOC_SUBDEV_G_INPUT_STATUS?

> > >>> This commit enables VIDIOC_ENUMINPUT, VIDIOC_G_INPUT, and VIDIOC_S_INPUT
> > >>> ioctls for use via v4l2 subdevice node.
> > >>>
> > >>> This commit should probably not be pushed upstream, because the (old)
> > >>> idea of video inputs conflicts with the newer concept of establishing
> > >>> media links between src->sink pads.
> > >>>
> > >>> However it might make sense for some subdevices to support enum/get/set
> > >>> inputs. One example would be the analog front end subdevice for the
> > >>> ADV748x. By providing these ioctls, selecting the ADV748x analog inputs
> > >>> can be done without requiring the implementation of media entities that
> > >>> would define the analog source for which to establish a media link.
> > >>>
> > >>> Signed-off-by: Steve Longerbeam <steve_longerbeam@mentor.com>
> > >>> ---
> > >>>  drivers/media/v4l2-core/v4l2-subdev.c |  9 +++++++++
> > >>>  include/media/v4l2-subdev.h           | 11 +++++++++++
> > >>>  2 files changed, 20 insertions(+)
> > >>>
> > >>> diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c
> > >>> index 6b989fe..73fbfe9 100644
> > >>> --- a/drivers/media/v4l2-core/v4l2-subdev.c
> > >>> +++ b/drivers/media/v4l2-core/v4l2-subdev.c
> > >>> @@ -378,6 +378,15 @@ static long subdev_do_ioctl(struct file *file, unsigned int cmd, void *arg)
> > >>>  			return -ENOTTY;
> > >>>  		return v4l2_querymenu(vfh->ctrl_handler, arg);
> > >>>
> > >>> +	case VIDIOC_ENUMINPUT:
> > >>> +		return v4l2_subdev_call(sd, video, enuminput, arg);
> > >>> +
> > >>> +	case VIDIOC_G_INPUT:
> > >>> +		return v4l2_subdev_call(sd, video, g_input, arg);
> > >>> +
> > >>> +	case VIDIOC_S_INPUT:
> > >>> +		return v4l2_subdev_call(sd, video, s_input, *(u32 *)arg);
> > >>> +
> > >>>  	case VIDIOC_G_CTRL:
> > >>>  		if (!vfh->ctrl_handler)
> > >>>  			return -ENOTTY;
> > >>> diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
> > >>> index f7fe78a..6e1a9cd 100644
> > >>> --- a/include/media/v4l2-subdev.h
> > >>> +++ b/include/media/v4l2-subdev.h
> > >>> @@ -383,6 +383,14 @@ struct v4l2_mbus_frame_desc {
> > >>>   * @g_input_status: get input status. Same as the status field in the
> > >>>   *	&struct &v4l2_input
> > >>>   *
> > >>> + * @enuminput: enumerate inputs. Should return the same input status as
> > >>> + *      @g_input_status if the passed input index is the currently active
> > >>> + *      input.
> > >>> + *
> > >>> + * @g_input: returns the currently active input index.
> > >>> + *
> > >>> + * @s_input: set the active input.
> > >>> + *
> > >>>   * @s_stream: used to notify the driver that a video stream will start or has
> > >>>   *	stopped.
> > >>>   *
> > >>> @@ -423,6 +431,9 @@ struct v4l2_subdev_video_ops {
> > >>>  	int (*g_tvnorms)(struct v4l2_subdev *sd, v4l2_std_id *std);
> > >>>  	int (*g_tvnorms_output)(struct v4l2_subdev *sd, v4l2_std_id *std);
> > >>>  	int (*g_input_status)(struct v4l2_subdev *sd, u32 *status);
> > >>> +	int (*enuminput)(struct v4l2_subdev *sd, struct v4l2_input *input);
> > >>> +	int (*g_input)(struct v4l2_subdev *sd, u32 *index);
> > >>> +	int (*s_input)(struct v4l2_subdev *sd, u32 index);
> > >>>  	int (*s_stream)(struct v4l2_subdev *sd, int enable);
> > >>>  	int (*g_pixelaspect)(struct v4l2_subdev *sd, struct v4l2_fract *aspect);
> > >>>  	int (*g_frame_interval)(struct v4l2_subdev *sd,
> 
> -- 
> Regards,
> 
> Laurent Pinchart

-- 
Best Regards,
Ramzi Ben Meftah.

Re: [PATCH 1/3] v4l2-subdev: Add subdev ioctl support for ENUM/GET/SET INPUT

[Laurent Pinchart]

Hi Ramzi,

On Thu, Jun 25, 2020 at 12:18:35PM +0200, Ramzi Ben Meftah wrote:
> On Thu, Jun 25, 2020 at 12:47:24PM +0300, Laurent Pinchart wrote:
> > On Thu, Jun 25, 2020 at 11:30:46AM +0200, Ramzi Ben Meftah wrote:
> >> On Thu, Jun 25, 2020 at 05:01:38AM +0300, Laurent Pinchart wrote:
> >>> On Wed, Jun 24, 2020 at 09:53:07AM +0200, Jacopo Mondi wrote:
> >>>> On Tue, Jun 16, 2020 at 12:00:15PM +0200, Ramzi BEN MEFTAH wrote:
> >>>>> From: Steve Longerbeam <steve_longerbeam@mentor.com>
> >>>> 
> >>>>  +Niklas, +Laurent
> >>>> 
> >>>> Niklas, Laurent, how does this play with CAP_IO_MC ?
> >>> 
> >>> I don't think it's related to CAP_IO_MC, but I don't think it's a good
> >>> idea either :-) Routing doesn't go through the subdev [gs]_input
> >>> operations in MC-based drivers. It should be configured through link
> >>> setup instead. This patch goes in the wrong direction, sorry Steve.
> >> 
> >> ENUMINPUT ioctl allow to get the input signal status. Is there an alternative
> >> with Media Controller?
> > 
> > No there isn't at the moment. I'm not opposed to adding such a feature,
> > but VIDIOC_ENUMINPUT isn't the right choice. This would have to be a
> > subdev pad operation (v4l2_subdev_pad_ops), not a video operation
> > (v4l2_subdev_video_ops). We also likely shouldn't call it "enum" input,
> > as it would retrieve properties of the input corresponding to the pad,
> > not enumerate inputs.
> 
> Looking to v4l2_subdev_pad_ops, there is g_input_status which seems to fulfill
> this need. But, seems this is not expose to user space although many drivers
> do implememt it.
> Should I add VIDIOC_SUBDEV_G_INPUT_STATUS?

Isn't g_input_status a video operation ? I would propose adding a
g_input_status pad operation, and expose that to userspace. We should
take that as an opportunity to consider designing that new operation
from scratch (possibly naming it differently) and make sure it could
address both analog and digital systems (for instance being able to
report the status of an SDI input).

> >>>>> This commit enables VIDIOC_ENUMINPUT, VIDIOC_G_INPUT, and VIDIOC_S_INPUT
> >>>>> ioctls for use via v4l2 subdevice node.
> >>>>>
> >>>>> This commit should probably not be pushed upstream, because the (old)
> >>>>> idea of video inputs conflicts with the newer concept of establishing
> >>>>> media links between src->sink pads.
> >>>>>
> >>>>> However it might make sense for some subdevices to support enum/get/set
> >>>>> inputs. One example would be the analog front end subdevice for the
> >>>>> ADV748x. By providing these ioctls, selecting the ADV748x analog inputs
> >>>>> can be done without requiring the implementation of media entities that
> >>>>> would define the analog source for which to establish a media link.
> >>>>>
> >>>>> Signed-off-by: Steve Longerbeam <steve_longerbeam@mentor.com>
> >>>>> ---
> >>>>>  drivers/media/v4l2-core/v4l2-subdev.c |  9 +++++++++
> >>>>>  include/media/v4l2-subdev.h           | 11 +++++++++++
> >>>>>  2 files changed, 20 insertions(+)
> >>>>>
> >>>>> diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c
> >>>>> index 6b989fe..73fbfe9 100644
> >>>>> --- a/drivers/media/v4l2-core/v4l2-subdev.c
> >>>>> +++ b/drivers/media/v4l2-core/v4l2-subdev.c
> >>>>> @@ -378,6 +378,15 @@ static long subdev_do_ioctl(struct file *file, unsigned int cmd, void *arg)
> >>>>>  			return -ENOTTY;
> >>>>>  		return v4l2_querymenu(vfh->ctrl_handler, arg);
> >>>>>
> >>>>> +	case VIDIOC_ENUMINPUT:
> >>>>> +		return v4l2_subdev_call(sd, video, enuminput, arg);
> >>>>> +
> >>>>> +	case VIDIOC_G_INPUT:
> >>>>> +		return v4l2_subdev_call(sd, video, g_input, arg);
> >>>>> +
> >>>>> +	case VIDIOC_S_INPUT:
> >>>>> +		return v4l2_subdev_call(sd, video, s_input, *(u32 *)arg);
> >>>>> +
> >>>>>  	case VIDIOC_G_CTRL:
> >>>>>  		if (!vfh->ctrl_handler)
> >>>>>  			return -ENOTTY;
> >>>>> diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
> >>>>> index f7fe78a..6e1a9cd 100644
> >>>>> --- a/include/media/v4l2-subdev.h
> >>>>> +++ b/include/media/v4l2-subdev.h
> >>>>> @@ -383,6 +383,14 @@ struct v4l2_mbus_frame_desc {
> >>>>>   * @g_input_status: get input status. Same as the status field in the
> >>>>>   *	&struct &v4l2_input
> >>>>>   *
> >>>>> + * @enuminput: enumerate inputs. Should return the same input status as
> >>>>> + *      @g_input_status if the passed input index is the currently active
> >>>>> + *      input.
> >>>>> + *
> >>>>> + * @g_input: returns the currently active input index.
> >>>>> + *
> >>>>> + * @s_input: set the active input.
> >>>>> + *
> >>>>>   * @s_stream: used to notify the driver that a video stream will start or has
> >>>>>   *	stopped.
> >>>>>   *
> >>>>> @@ -423,6 +431,9 @@ struct v4l2_subdev_video_ops {
> >>>>>  	int (*g_tvnorms)(struct v4l2_subdev *sd, v4l2_std_id *std);
> >>>>>  	int (*g_tvnorms_output)(struct v4l2_subdev *sd, v4l2_std_id *std);
> >>>>>  	int (*g_input_status)(struct v4l2_subdev *sd, u32 *status);
> >>>>> +	int (*enuminput)(struct v4l2_subdev *sd, struct v4l2_input *input);
> >>>>> +	int (*g_input)(struct v4l2_subdev *sd, u32 *index);
> >>>>> +	int (*s_input)(struct v4l2_subdev *sd, u32 index);
> >>>>>  	int (*s_stream)(struct v4l2_subdev *sd, int enable);
> >>>>>  	int (*g_pixelaspect)(struct v4l2_subdev *sd, struct v4l2_fract *aspect);
> >>>>>  	int (*g_frame_interval)(struct v4l2_subdev *sd,

-- 
Regards,

Laurent Pinchart

Re: [PATCH 1/3] v4l2-subdev: Add subdev ioctl support for ENUM/GET/SET INPUT

[Hans Verkuil]

On 25/06/2020 12:29, Laurent Pinchart wrote:
> Hi Ramzi,
> 
> On Thu, Jun 25, 2020 at 12:18:35PM +0200, Ramzi Ben Meftah wrote:
>> On Thu, Jun 25, 2020 at 12:47:24PM +0300, Laurent Pinchart wrote:
>>> On Thu, Jun 25, 2020 at 11:30:46AM +0200, Ramzi Ben Meftah wrote:
>>>> On Thu, Jun 25, 2020 at 05:01:38AM +0300, Laurent Pinchart wrote:
>>>>> On Wed, Jun 24, 2020 at 09:53:07AM +0200, Jacopo Mondi wrote:
>>>>>> On Tue, Jun 16, 2020 at 12:00:15PM +0200, Ramzi BEN MEFTAH wrote:
>>>>>>> From: Steve Longerbeam <steve_longerbeam@mentor.com>
>>>>>>
>>>>>>  +Niklas, +Laurent
>>>>>>
>>>>>> Niklas, Laurent, how does this play with CAP_IO_MC ?
>>>>>
>>>>> I don't think it's related to CAP_IO_MC, but I don't think it's a good
>>>>> idea either :-) Routing doesn't go through the subdev [gs]_input
>>>>> operations in MC-based drivers. It should be configured through link
>>>>> setup instead. This patch goes in the wrong direction, sorry Steve.
>>>>
>>>> ENUMINPUT ioctl allow to get the input signal status. Is there an alternative
>>>> with Media Controller?
>>>
>>> No there isn't at the moment. I'm not opposed to adding such a feature,
>>> but VIDIOC_ENUMINPUT isn't the right choice. This would have to be a
>>> subdev pad operation (v4l2_subdev_pad_ops), not a video operation
>>> (v4l2_subdev_video_ops). We also likely shouldn't call it "enum" input,
>>> as it would retrieve properties of the input corresponding to the pad,
>>> not enumerate inputs.
>>
>> Looking to v4l2_subdev_pad_ops, there is g_input_status which seems to fulfill
>> this need. But, seems this is not expose to user space although many drivers
>> do implememt it.
>> Should I add VIDIOC_SUBDEV_G_INPUT_STATUS?
> 
> Isn't g_input_status a video operation ? I would propose adding a
> g_input_status pad operation, and expose that to userspace. We should
> take that as an opportunity to consider designing that new operation
> from scratch (possibly naming it differently) and make sure it could
> address both analog and digital systems (for instance being able to
> report the status of an SDI input).

Yes, I was wondering the same. The status bits are ancient and we might
want to improve on it.

Ramzi, what exactly is your use-case? Is this for an HDMI input? Analog
video input? Before adding a new ioctl I'd like to know why you think
you need it :-)

Regards,

	Hans

> 
>>>>>>> This commit enables VIDIOC_ENUMINPUT, VIDIOC_G_INPUT, and VIDIOC_S_INPUT
>>>>>>> ioctls for use via v4l2 subdevice node.
>>>>>>>
>>>>>>> This commit should probably not be pushed upstream, because the (old)
>>>>>>> idea of video inputs conflicts with the newer concept of establishing
>>>>>>> media links between src->sink pads.
>>>>>>>
>>>>>>> However it might make sense for some subdevices to support enum/get/set
>>>>>>> inputs. One example would be the analog front end subdevice for the
>>>>>>> ADV748x. By providing these ioctls, selecting the ADV748x analog inputs
>>>>>>> can be done without requiring the implementation of media entities that
>>>>>>> would define the analog source for which to establish a media link.
>>>>>>>
>>>>>>> Signed-off-by: Steve Longerbeam <steve_longerbeam@mentor.com>
>>>>>>> ---
>>>>>>>  drivers/media/v4l2-core/v4l2-subdev.c |  9 +++++++++
>>>>>>>  include/media/v4l2-subdev.h           | 11 +++++++++++
>>>>>>>  2 files changed, 20 insertions(+)
>>>>>>>
>>>>>>> diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c
>>>>>>> index 6b989fe..73fbfe9 100644
>>>>>>> --- a/drivers/media/v4l2-core/v4l2-subdev.c
>>>>>>> +++ b/drivers/media/v4l2-core/v4l2-subdev.c
>>>>>>> @@ -378,6 +378,15 @@ static long subdev_do_ioctl(struct file *file, unsigned int cmd, void *arg)
>>>>>>>  			return -ENOTTY;
>>>>>>>  		return v4l2_querymenu(vfh->ctrl_handler, arg);
>>>>>>>
>>>>>>> +	case VIDIOC_ENUMINPUT:
>>>>>>> +		return v4l2_subdev_call(sd, video, enuminput, arg);
>>>>>>> +
>>>>>>> +	case VIDIOC_G_INPUT:
>>>>>>> +		return v4l2_subdev_call(sd, video, g_input, arg);
>>>>>>> +
>>>>>>> +	case VIDIOC_S_INPUT:
>>>>>>> +		return v4l2_subdev_call(sd, video, s_input, *(u32 *)arg);
>>>>>>> +
>>>>>>>  	case VIDIOC_G_CTRL:
>>>>>>>  		if (!vfh->ctrl_handler)
>>>>>>>  			return -ENOTTY;
>>>>>>> diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
>>>>>>> index f7fe78a..6e1a9cd 100644
>>>>>>> --- a/include/media/v4l2-subdev.h
>>>>>>> +++ b/include/media/v4l2-subdev.h
>>>>>>> @@ -383,6 +383,14 @@ struct v4l2_mbus_frame_desc {
>>>>>>>   * @g_input_status: get input status. Same as the status field in the
>>>>>>>   *	&struct &v4l2_input
>>>>>>>   *
>>>>>>> + * @enuminput: enumerate inputs. Should return the same input status as
>>>>>>> + *      @g_input_status if the passed input index is the currently active
>>>>>>> + *      input.
>>>>>>> + *
>>>>>>> + * @g_input: returns the currently active input index.
>>>>>>> + *
>>>>>>> + * @s_input: set the active input.
>>>>>>> + *
>>>>>>>   * @s_stream: used to notify the driver that a video stream will start or has
>>>>>>>   *	stopped.
>>>>>>>   *
>>>>>>> @@ -423,6 +431,9 @@ struct v4l2_subdev_video_ops {
>>>>>>>  	int (*g_tvnorms)(struct v4l2_subdev *sd, v4l2_std_id *std);
>>>>>>>  	int (*g_tvnorms_output)(struct v4l2_subdev *sd, v4l2_std_id *std);
>>>>>>>  	int (*g_input_status)(struct v4l2_subdev *sd, u32 *status);
>>>>>>> +	int (*enuminput)(struct v4l2_subdev *sd, struct v4l2_input *input);
>>>>>>> +	int (*g_input)(struct v4l2_subdev *sd, u32 *index);
>>>>>>> +	int (*s_input)(struct v4l2_subdev *sd, u32 index);
>>>>>>>  	int (*s_stream)(struct v4l2_subdev *sd, int enable);
>>>>>>>  	int (*g_pixelaspect)(struct v4l2_subdev *sd, struct v4l2_fract *aspect);
>>>>>>>  	int (*g_frame_interval)(struct v4l2_subdev *sd,
> 

Re: [PATCH 1/3] v4l2-subdev: Add subdev ioctl support for ENUM/GET/SET INPUT

[Ramzi Ben Meftah]

Hi Laurent, Hans,

On Thu, Jun 25, 2020 at 12:35:02PM +0200, Hans Verkuil wrote:
> On 25/06/2020 12:29, Laurent Pinchart wrote:
> > Hi Ramzi,
> > 
> > On Thu, Jun 25, 2020 at 12:18:35PM +0200, Ramzi Ben Meftah wrote:
> >> On Thu, Jun 25, 2020 at 12:47:24PM +0300, Laurent Pinchart wrote:
> >>> On Thu, Jun 25, 2020 at 11:30:46AM +0200, Ramzi Ben Meftah wrote:
> >>>> On Thu, Jun 25, 2020 at 05:01:38AM +0300, Laurent Pinchart wrote:
> >>>>> On Wed, Jun 24, 2020 at 09:53:07AM +0200, Jacopo Mondi wrote:
> >>>>>> On Tue, Jun 16, 2020 at 12:00:15PM +0200, Ramzi BEN MEFTAH wrote:
> >>>>>>> From: Steve Longerbeam <steve_longerbeam@mentor.com>
> >>>>>>
> >>>>>>  +Niklas, +Laurent
> >>>>>>
> >>>>>> Niklas, Laurent, how does this play with CAP_IO_MC ?
> >>>>>
> >>>>> I don't think it's related to CAP_IO_MC, but I don't think it's a good
> >>>>> idea either :-) Routing doesn't go through the subdev [gs]_input
> >>>>> operations in MC-based drivers. It should be configured through link
> >>>>> setup instead. This patch goes in the wrong direction, sorry Steve.
> >>>>
> >>>> ENUMINPUT ioctl allow to get the input signal status. Is there an alternative
> >>>> with Media Controller?
> >>>
> >>> No there isn't at the moment. I'm not opposed to adding such a feature,
> >>> but VIDIOC_ENUMINPUT isn't the right choice. This would have to be a
> >>> subdev pad operation (v4l2_subdev_pad_ops), not a video operation
> >>> (v4l2_subdev_video_ops). We also likely shouldn't call it "enum" input,
> >>> as it would retrieve properties of the input corresponding to the pad,
> >>> not enumerate inputs.
> >>
> >> Looking to v4l2_subdev_pad_ops, there is g_input_status which seems to fulfill
> >> this need. But, seems this is not expose to user space although many drivers
> >> do implememt it.
> >> Should I add VIDIOC_SUBDEV_G_INPUT_STATUS?
> > 
> > Isn't g_input_status a video operation ? I would propose adding a
> > g_input_status pad operation, and expose that to userspace. We should
> > take that as an opportunity to consider designing that new operation
> > from scratch (possibly naming it differently) and make sure it could
> > address both analog and digital systems (for instance being able to
> > report the status of an SDI input).

Sorry, my mistake. But, does it make sens that it belongs to video ops?
I think it should be part of pad ops, it is used now as alternative to 
ENUMINPUT and we agreed that it should(if we are going to implement it) be 
part of pad ops.

> 
> Yes, I was wondering the same. The status bits are ancient and we might
> want to improve on it.
> 
> Ramzi, what exactly is your use-case? Is this for an HDMI input? Analog
> video input? Before adding a new ioctl I'd like to know why you think
> you need it :-)

I need to know if the input signal(analog and digital) is there, before
starting streaming. I am not aware of other way to check for it.

> 
> Regards,
> 
> 	Hans
> 
> > 
> >>>>>>> This commit enables VIDIOC_ENUMINPUT, VIDIOC_G_INPUT, and VIDIOC_S_INPUT
> >>>>>>> ioctls for use via v4l2 subdevice node.
> >>>>>>>
> >>>>>>> This commit should probably not be pushed upstream, because the (old)
> >>>>>>> idea of video inputs conflicts with the newer concept of establishing
> >>>>>>> media links between src->sink pads.
> >>>>>>>
> >>>>>>> However it might make sense for some subdevices to support enum/get/set
> >>>>>>> inputs. One example would be the analog front end subdevice for the
> >>>>>>> ADV748x. By providing these ioctls, selecting the ADV748x analog inputs
> >>>>>>> can be done without requiring the implementation of media entities that
> >>>>>>> would define the analog source for which to establish a media link.
> >>>>>>>
> >>>>>>> Signed-off-by: Steve Longerbeam <steve_longerbeam@mentor.com>
> >>>>>>> ---
> >>>>>>>  drivers/media/v4l2-core/v4l2-subdev.c |  9 +++++++++
> >>>>>>>  include/media/v4l2-subdev.h           | 11 +++++++++++
> >>>>>>>  2 files changed, 20 insertions(+)
> >>>>>>>
> >>>>>>> diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c
> >>>>>>> index 6b989fe..73fbfe9 100644
> >>>>>>> --- a/drivers/media/v4l2-core/v4l2-subdev.c
> >>>>>>> +++ b/drivers/media/v4l2-core/v4l2-subdev.c
> >>>>>>> @@ -378,6 +378,15 @@ static long subdev_do_ioctl(struct file *file, unsigned int cmd, void *arg)
> >>>>>>>  			return -ENOTTY;
> >>>>>>>  		return v4l2_querymenu(vfh->ctrl_handler, arg);
> >>>>>>>
> >>>>>>> +	case VIDIOC_ENUMINPUT:
> >>>>>>> +		return v4l2_subdev_call(sd, video, enuminput, arg);
> >>>>>>> +
> >>>>>>> +	case VIDIOC_G_INPUT:
> >>>>>>> +		return v4l2_subdev_call(sd, video, g_input, arg);
> >>>>>>> +
> >>>>>>> +	case VIDIOC_S_INPUT:
> >>>>>>> +		return v4l2_subdev_call(sd, video, s_input, *(u32 *)arg);
> >>>>>>> +
> >>>>>>>  	case VIDIOC_G_CTRL:
> >>>>>>>  		if (!vfh->ctrl_handler)
> >>>>>>>  			return -ENOTTY;
> >>>>>>> diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
> >>>>>>> index f7fe78a..6e1a9cd 100644
> >>>>>>> --- a/include/media/v4l2-subdev.h
> >>>>>>> +++ b/include/media/v4l2-subdev.h
> >>>>>>> @@ -383,6 +383,14 @@ struct v4l2_mbus_frame_desc {
> >>>>>>>   * @g_input_status: get input status. Same as the status field in the
> >>>>>>>   *	&struct &v4l2_input
> >>>>>>>   *
> >>>>>>> + * @enuminput: enumerate inputs. Should return the same input status as
> >>>>>>> + *      @g_input_status if the passed input index is the currently active
> >>>>>>> + *      input.
> >>>>>>> + *
> >>>>>>> + * @g_input: returns the currently active input index.
> >>>>>>> + *
> >>>>>>> + * @s_input: set the active input.
> >>>>>>> + *
> >>>>>>>   * @s_stream: used to notify the driver that a video stream will start or has
> >>>>>>>   *	stopped.
> >>>>>>>   *
> >>>>>>> @@ -423,6 +431,9 @@ struct v4l2_subdev_video_ops {
> >>>>>>>  	int (*g_tvnorms)(struct v4l2_subdev *sd, v4l2_std_id *std);
> >>>>>>>  	int (*g_tvnorms_output)(struct v4l2_subdev *sd, v4l2_std_id *std);
> >>>>>>>  	int (*g_input_status)(struct v4l2_subdev *sd, u32 *status);
> >>>>>>> +	int (*enuminput)(struct v4l2_subdev *sd, struct v4l2_input *input);
> >>>>>>> +	int (*g_input)(struct v4l2_subdev *sd, u32 *index);
> >>>>>>> +	int (*s_input)(struct v4l2_subdev *sd, u32 index);
> >>>>>>>  	int (*s_stream)(struct v4l2_subdev *sd, int enable);
> >>>>>>>  	int (*g_pixelaspect)(struct v4l2_subdev *sd, struct v4l2_fract *aspect);
> >>>>>>>  	int (*g_frame_interval)(struct v4l2_subdev *sd,
> > 
> 

-- 
Best Regards,
Ramzi Ben Meftah.

Re: [PATCH 1/3] v4l2-subdev: Add subdev ioctl support for ENUM/GET/SET INPUT

[Ramzi Ben Meftah]

On Fri, Jun 26, 2020 at 11:09:13AM +0200, Ramzi Ben Meftah wrote:
> Hi Laurent, Hans,
> 
> On Thu, Jun 25, 2020 at 12:35:02PM +0200, Hans Verkuil wrote:
> > On 25/06/2020 12:29, Laurent Pinchart wrote:
> > > Hi Ramzi,
> > > 
> > > On Thu, Jun 25, 2020 at 12:18:35PM +0200, Ramzi Ben Meftah wrote:
> > >> On Thu, Jun 25, 2020 at 12:47:24PM +0300, Laurent Pinchart wrote:
> > >>> On Thu, Jun 25, 2020 at 11:30:46AM +0200, Ramzi Ben Meftah wrote:
> > >>>> On Thu, Jun 25, 2020 at 05:01:38AM +0300, Laurent Pinchart wrote:
> > >>>>> On Wed, Jun 24, 2020 at 09:53:07AM +0200, Jacopo Mondi wrote:
> > >>>>>> On Tue, Jun 16, 2020 at 12:00:15PM +0200, Ramzi BEN MEFTAH wrote:
> > >>>>>>> From: Steve Longerbeam <steve_longerbeam@mentor.com>
> > >>>>>>
> > >>>>>>  +Niklas, +Laurent
> > >>>>>>
> > >>>>>> Niklas, Laurent, how does this play with CAP_IO_MC ?
> > >>>>>
> > >>>>> I don't think it's related to CAP_IO_MC, but I don't think it's a good
> > >>>>> idea either :-) Routing doesn't go through the subdev [gs]_input
> > >>>>> operations in MC-based drivers. It should be configured through link
> > >>>>> setup instead. This patch goes in the wrong direction, sorry Steve.
> > >>>>
> > >>>> ENUMINPUT ioctl allow to get the input signal status. Is there an alternative
> > >>>> with Media Controller?
> > >>>
> > >>> No there isn't at the moment. I'm not opposed to adding such a feature,
> > >>> but VIDIOC_ENUMINPUT isn't the right choice. This would have to be a
> > >>> subdev pad operation (v4l2_subdev_pad_ops), not a video operation
> > >>> (v4l2_subdev_video_ops). We also likely shouldn't call it "enum" input,
> > >>> as it would retrieve properties of the input corresponding to the pad,
> > >>> not enumerate inputs.
> > >>
> > >> Looking to v4l2_subdev_pad_ops, there is g_input_status which seems to fulfill
> > >> this need. But, seems this is not expose to user space although many drivers
> > >> do implememt it.
> > >> Should I add VIDIOC_SUBDEV_G_INPUT_STATUS?
> > > 
> > > Isn't g_input_status a video operation ? I would propose adding a
> > > g_input_status pad operation, and expose that to userspace. We should
> > > take that as an opportunity to consider designing that new operation
> > > from scratch (possibly naming it differently) and make sure it could
> > > address both analog and digital systems (for instance being able to
> > > report the status of an SDI input).
> 
> Sorry, my mistake. But, does it make sens that it belongs to video ops?
> I think it should be part of pad ops, it is used now as alternative to 
> ENUMINPUT and we agreed that it should(if we are going to implement it) be 
> part of pad ops.
> 
> > 
> > Yes, I was wondering the same. The status bits are ancient and we might
> > want to improve on it.
> > 
> > Ramzi, what exactly is your use-case? Is this for an HDMI input? Analog
> > video input? Before adding a new ioctl I'd like to know why you think
> > you need it :-)
> 
> I need to know if the input signal(analog and digital) is there, before
> starting streaming. I am not aware of other way to check for it.

There is also no alternative for V4L2_IN_ST_NO_POWER and V4L2_IN_ST_NO_COLOR
I am not using these two, but these were available with ENUMINPUT ioctl
and a subdev driver that could offer these info has no way to pass them to
userspace.

> 
> > 
> > Regards,
> > 
> > 	Hans
> > 
> > > 
> > >>>>>>> This commit enables VIDIOC_ENUMINPUT, VIDIOC_G_INPUT, and VIDIOC_S_INPUT
> > >>>>>>> ioctls for use via v4l2 subdevice node.
> > >>>>>>>
> > >>>>>>> This commit should probably not be pushed upstream, because the (old)
> > >>>>>>> idea of video inputs conflicts with the newer concept of establishing
> > >>>>>>> media links between src->sink pads.
> > >>>>>>>
> > >>>>>>> However it might make sense for some subdevices to support enum/get/set
> > >>>>>>> inputs. One example would be the analog front end subdevice for the
> > >>>>>>> ADV748x. By providing these ioctls, selecting the ADV748x analog inputs
> > >>>>>>> can be done without requiring the implementation of media entities that
> > >>>>>>> would define the analog source for which to establish a media link.
> > >>>>>>>
> > >>>>>>> Signed-off-by: Steve Longerbeam <steve_longerbeam@mentor.com>
> > >>>>>>> ---
> > >>>>>>>  drivers/media/v4l2-core/v4l2-subdev.c |  9 +++++++++
> > >>>>>>>  include/media/v4l2-subdev.h           | 11 +++++++++++
> > >>>>>>>  2 files changed, 20 insertions(+)
> > >>>>>>>
> > >>>>>>> diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c
> > >>>>>>> index 6b989fe..73fbfe9 100644
> > >>>>>>> --- a/drivers/media/v4l2-core/v4l2-subdev.c
> > >>>>>>> +++ b/drivers/media/v4l2-core/v4l2-subdev.c
> > >>>>>>> @@ -378,6 +378,15 @@ static long subdev_do_ioctl(struct file *file, unsigned int cmd, void *arg)
> > >>>>>>>  			return -ENOTTY;
> > >>>>>>>  		return v4l2_querymenu(vfh->ctrl_handler, arg);
> > >>>>>>>
> > >>>>>>> +	case VIDIOC_ENUMINPUT:
> > >>>>>>> +		return v4l2_subdev_call(sd, video, enuminput, arg);
> > >>>>>>> +
> > >>>>>>> +	case VIDIOC_G_INPUT:
> > >>>>>>> +		return v4l2_subdev_call(sd, video, g_input, arg);
> > >>>>>>> +
> > >>>>>>> +	case VIDIOC_S_INPUT:
> > >>>>>>> +		return v4l2_subdev_call(sd, video, s_input, *(u32 *)arg);
> > >>>>>>> +
> > >>>>>>>  	case VIDIOC_G_CTRL:
> > >>>>>>>  		if (!vfh->ctrl_handler)
> > >>>>>>>  			return -ENOTTY;
> > >>>>>>> diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
> > >>>>>>> index f7fe78a..6e1a9cd 100644
> > >>>>>>> --- a/include/media/v4l2-subdev.h
> > >>>>>>> +++ b/include/media/v4l2-subdev.h
> > >>>>>>> @@ -383,6 +383,14 @@ struct v4l2_mbus_frame_desc {
> > >>>>>>>   * @g_input_status: get input status. Same as the status field in the
> > >>>>>>>   *	&struct &v4l2_input
> > >>>>>>>   *
> > >>>>>>> + * @enuminput: enumerate inputs. Should return the same input status as
> > >>>>>>> + *      @g_input_status if the passed input index is the currently active
> > >>>>>>> + *      input.
> > >>>>>>> + *
> > >>>>>>> + * @g_input: returns the currently active input index.
> > >>>>>>> + *
> > >>>>>>> + * @s_input: set the active input.
> > >>>>>>> + *
> > >>>>>>>   * @s_stream: used to notify the driver that a video stream will start or has
> > >>>>>>>   *	stopped.
> > >>>>>>>   *
> > >>>>>>> @@ -423,6 +431,9 @@ struct v4l2_subdev_video_ops {
> > >>>>>>>  	int (*g_tvnorms)(struct v4l2_subdev *sd, v4l2_std_id *std);
> > >>>>>>>  	int (*g_tvnorms_output)(struct v4l2_subdev *sd, v4l2_std_id *std);
> > >>>>>>>  	int (*g_input_status)(struct v4l2_subdev *sd, u32 *status);
> > >>>>>>> +	int (*enuminput)(struct v4l2_subdev *sd, struct v4l2_input *input);
> > >>>>>>> +	int (*g_input)(struct v4l2_subdev *sd, u32 *index);
> > >>>>>>> +	int (*s_input)(struct v4l2_subdev *sd, u32 index);
> > >>>>>>>  	int (*s_stream)(struct v4l2_subdev *sd, int enable);
> > >>>>>>>  	int (*g_pixelaspect)(struct v4l2_subdev *sd, struct v4l2_fract *aspect);
> > >>>>>>>  	int (*g_frame_interval)(struct v4l2_subdev *sd,
> > > 
> > 
> 
> -- 
> Best Regards,
> Ramzi Ben Meftah.

-- 
Best Regards,
Ramzi Ben Meftah.

Re: [PATCH 1/3] v4l2-subdev: Add subdev ioctl support for ENUM/GET/SET INPUT

[Hans Verkuil]

On 26/06/2020 11:09, Ramzi Ben Meftah wrote:
> Hi Laurent, Hans,
> 
> On Thu, Jun 25, 2020 at 12:35:02PM +0200, Hans Verkuil wrote:
>> On 25/06/2020 12:29, Laurent Pinchart wrote:
>>> Hi Ramzi,
>>>
>>> On Thu, Jun 25, 2020 at 12:18:35PM +0200, Ramzi Ben Meftah wrote:
>>>> On Thu, Jun 25, 2020 at 12:47:24PM +0300, Laurent Pinchart wrote:
>>>>> On Thu, Jun 25, 2020 at 11:30:46AM +0200, Ramzi Ben Meftah wrote:
>>>>>> On Thu, Jun 25, 2020 at 05:01:38AM +0300, Laurent Pinchart wrote:
>>>>>>> On Wed, Jun 24, 2020 at 09:53:07AM +0200, Jacopo Mondi wrote:
>>>>>>>> On Tue, Jun 16, 2020 at 12:00:15PM +0200, Ramzi BEN MEFTAH wrote:
>>>>>>>>> From: Steve Longerbeam <steve_longerbeam@mentor.com>
>>>>>>>>
>>>>>>>>  +Niklas, +Laurent
>>>>>>>>
>>>>>>>> Niklas, Laurent, how does this play with CAP_IO_MC ?
>>>>>>>
>>>>>>> I don't think it's related to CAP_IO_MC, but I don't think it's a good
>>>>>>> idea either :-) Routing doesn't go through the subdev [gs]_input
>>>>>>> operations in MC-based drivers. It should be configured through link
>>>>>>> setup instead. This patch goes in the wrong direction, sorry Steve.
>>>>>>
>>>>>> ENUMINPUT ioctl allow to get the input signal status. Is there an alternative
>>>>>> with Media Controller?
>>>>>
>>>>> No there isn't at the moment. I'm not opposed to adding such a feature,
>>>>> but VIDIOC_ENUMINPUT isn't the right choice. This would have to be a
>>>>> subdev pad operation (v4l2_subdev_pad_ops), not a video operation
>>>>> (v4l2_subdev_video_ops). We also likely shouldn't call it "enum" input,
>>>>> as it would retrieve properties of the input corresponding to the pad,
>>>>> not enumerate inputs.
>>>>
>>>> Looking to v4l2_subdev_pad_ops, there is g_input_status which seems to fulfill
>>>> this need. But, seems this is not expose to user space although many drivers
>>>> do implememt it.
>>>> Should I add VIDIOC_SUBDEV_G_INPUT_STATUS?
>>>
>>> Isn't g_input_status a video operation ? I would propose adding a
>>> g_input_status pad operation, and expose that to userspace. We should
>>> take that as an opportunity to consider designing that new operation
>>> from scratch (possibly naming it differently) and make sure it could
>>> address both analog and digital systems (for instance being able to
>>> report the status of an SDI input).
> 
> Sorry, my mistake. But, does it make sens that it belongs to video ops?
> I think it should be part of pad ops, it is used now as alternative to 
> ENUMINPUT and we agreed that it should(if we are going to implement it) be 
> part of pad ops.
> 
>>
>> Yes, I was wondering the same. The status bits are ancient and we might
>> want to improve on it.
>>
>> Ramzi, what exactly is your use-case? Is this for an HDMI input? Analog
>> video input? Before adding a new ioctl I'd like to know why you think
>> you need it :-)
> 
> I need to know if the input signal(analog and digital) is there, before
> starting streaming. I am not aware of other way to check for it.

You can check for digital timings using QUERY_DV_TIMINGS: it will return an
error if there is no lock. Subscribe to the SOURCE_CHANGE event to know when
the signal is found/lost.

For analog timings you have QUERYSTD and the same event. The main limitation
with analog (SDTV) video receivers is that not all analog receivers have implemented
this event. But the adv7180 and tvp5150 do support this.

This is the recommended way of implementing this and it has the advantage of
being interrupt-driven, so no need to poll for a status in the application.

Regards,

	Hans

> 
>>
>> Regards,
>>
>> 	Hans
>>
>>>
>>>>>>>>> This commit enables VIDIOC_ENUMINPUT, VIDIOC_G_INPUT, and VIDIOC_S_INPUT
>>>>>>>>> ioctls for use via v4l2 subdevice node.
>>>>>>>>>
>>>>>>>>> This commit should probably not be pushed upstream, because the (old)
>>>>>>>>> idea of video inputs conflicts with the newer concept of establishing
>>>>>>>>> media links between src->sink pads.
>>>>>>>>>
>>>>>>>>> However it might make sense for some subdevices to support enum/get/set
>>>>>>>>> inputs. One example would be the analog front end subdevice for the
>>>>>>>>> ADV748x. By providing these ioctls, selecting the ADV748x analog inputs
>>>>>>>>> can be done without requiring the implementation of media entities that
>>>>>>>>> would define the analog source for which to establish a media link.
>>>>>>>>>
>>>>>>>>> Signed-off-by: Steve Longerbeam <steve_longerbeam@mentor.com>
>>>>>>>>> ---
>>>>>>>>>  drivers/media/v4l2-core/v4l2-subdev.c |  9 +++++++++
>>>>>>>>>  include/media/v4l2-subdev.h           | 11 +++++++++++
>>>>>>>>>  2 files changed, 20 insertions(+)
>>>>>>>>>
>>>>>>>>> diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c
>>>>>>>>> index 6b989fe..73fbfe9 100644
>>>>>>>>> --- a/drivers/media/v4l2-core/v4l2-subdev.c
>>>>>>>>> +++ b/drivers/media/v4l2-core/v4l2-subdev.c
>>>>>>>>> @@ -378,6 +378,15 @@ static long subdev_do_ioctl(struct file *file, unsigned int cmd, void *arg)
>>>>>>>>>  			return -ENOTTY;
>>>>>>>>>  		return v4l2_querymenu(vfh->ctrl_handler, arg);
>>>>>>>>>
>>>>>>>>> +	case VIDIOC_ENUMINPUT:
>>>>>>>>> +		return v4l2_subdev_call(sd, video, enuminput, arg);
>>>>>>>>> +
>>>>>>>>> +	case VIDIOC_G_INPUT:
>>>>>>>>> +		return v4l2_subdev_call(sd, video, g_input, arg);
>>>>>>>>> +
>>>>>>>>> +	case VIDIOC_S_INPUT:
>>>>>>>>> +		return v4l2_subdev_call(sd, video, s_input, *(u32 *)arg);
>>>>>>>>> +
>>>>>>>>>  	case VIDIOC_G_CTRL:
>>>>>>>>>  		if (!vfh->ctrl_handler)
>>>>>>>>>  			return -ENOTTY;
>>>>>>>>> diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
>>>>>>>>> index f7fe78a..6e1a9cd 100644
>>>>>>>>> --- a/include/media/v4l2-subdev.h
>>>>>>>>> +++ b/include/media/v4l2-subdev.h
>>>>>>>>> @@ -383,6 +383,14 @@ struct v4l2_mbus_frame_desc {
>>>>>>>>>   * @g_input_status: get input status. Same as the status field in the
>>>>>>>>>   *	&struct &v4l2_input
>>>>>>>>>   *
>>>>>>>>> + * @enuminput: enumerate inputs. Should return the same input status as
>>>>>>>>> + *      @g_input_status if the passed input index is the currently active
>>>>>>>>> + *      input.
>>>>>>>>> + *
>>>>>>>>> + * @g_input: returns the currently active input index.
>>>>>>>>> + *
>>>>>>>>> + * @s_input: set the active input.
>>>>>>>>> + *
>>>>>>>>>   * @s_stream: used to notify the driver that a video stream will start or has
>>>>>>>>>   *	stopped.
>>>>>>>>>   *
>>>>>>>>> @@ -423,6 +431,9 @@ struct v4l2_subdev_video_ops {
>>>>>>>>>  	int (*g_tvnorms)(struct v4l2_subdev *sd, v4l2_std_id *std);
>>>>>>>>>  	int (*g_tvnorms_output)(struct v4l2_subdev *sd, v4l2_std_id *std);
>>>>>>>>>  	int (*g_input_status)(struct v4l2_subdev *sd, u32 *status);
>>>>>>>>> +	int (*enuminput)(struct v4l2_subdev *sd, struct v4l2_input *input);
>>>>>>>>> +	int (*g_input)(struct v4l2_subdev *sd, u32 *index);
>>>>>>>>> +	int (*s_input)(struct v4l2_subdev *sd, u32 index);
>>>>>>>>>  	int (*s_stream)(struct v4l2_subdev *sd, int enable);
>>>>>>>>>  	int (*g_pixelaspect)(struct v4l2_subdev *sd, struct v4l2_fract *aspect);
>>>>>>>>>  	int (*g_frame_interval)(struct v4l2_subdev *sd,
>>>
>>
> 

Re: [PATCH 1/3] v4l2-subdev: Add subdev ioctl support for ENUM/GET/SET INPUT

[Ramzi Ben Meftah]

On Fri, Jun 26, 2020 at 11:25:31AM +0200, Hans Verkuil wrote:
> On 26/06/2020 11:09, Ramzi Ben Meftah wrote:
> > Hi Laurent, Hans,
> > 
> > On Thu, Jun 25, 2020 at 12:35:02PM +0200, Hans Verkuil wrote:
> >> On 25/06/2020 12:29, Laurent Pinchart wrote:
> >>> Hi Ramzi,
> >>>
> >>> On Thu, Jun 25, 2020 at 12:18:35PM +0200, Ramzi Ben Meftah wrote:
> >>>> On Thu, Jun 25, 2020 at 12:47:24PM +0300, Laurent Pinchart wrote:
> >>>>> On Thu, Jun 25, 2020 at 11:30:46AM +0200, Ramzi Ben Meftah wrote:
> >>>>>> On Thu, Jun 25, 2020 at 05:01:38AM +0300, Laurent Pinchart wrote:
> >>>>>>> On Wed, Jun 24, 2020 at 09:53:07AM +0200, Jacopo Mondi wrote:
> >>>>>>>> On Tue, Jun 16, 2020 at 12:00:15PM +0200, Ramzi BEN MEFTAH wrote:
> >>>>>>>>> From: Steve Longerbeam <steve_longerbeam@mentor.com>
> >>>>>>>>
> >>>>>>>>  +Niklas, +Laurent
> >>>>>>>>
> >>>>>>>> Niklas, Laurent, how does this play with CAP_IO_MC ?
> >>>>>>>
> >>>>>>> I don't think it's related to CAP_IO_MC, but I don't think it's a good
> >>>>>>> idea either :-) Routing doesn't go through the subdev [gs]_input
> >>>>>>> operations in MC-based drivers. It should be configured through link
> >>>>>>> setup instead. This patch goes in the wrong direction, sorry Steve.
> >>>>>>
> >>>>>> ENUMINPUT ioctl allow to get the input signal status. Is there an alternative
> >>>>>> with Media Controller?
> >>>>>
> >>>>> No there isn't at the moment. I'm not opposed to adding such a feature,
> >>>>> but VIDIOC_ENUMINPUT isn't the right choice. This would have to be a
> >>>>> subdev pad operation (v4l2_subdev_pad_ops), not a video operation
> >>>>> (v4l2_subdev_video_ops). We also likely shouldn't call it "enum" input,
> >>>>> as it would retrieve properties of the input corresponding to the pad,
> >>>>> not enumerate inputs.
> >>>>
> >>>> Looking to v4l2_subdev_pad_ops, there is g_input_status which seems to fulfill
> >>>> this need. But, seems this is not expose to user space although many drivers
> >>>> do implememt it.
> >>>> Should I add VIDIOC_SUBDEV_G_INPUT_STATUS?
> >>>
> >>> Isn't g_input_status a video operation ? I would propose adding a
> >>> g_input_status pad operation, and expose that to userspace. We should
> >>> take that as an opportunity to consider designing that new operation
> >>> from scratch (possibly naming it differently) and make sure it could
> >>> address both analog and digital systems (for instance being able to
> >>> report the status of an SDI input).
> > 
> > Sorry, my mistake. But, does it make sens that it belongs to video ops?
> > I think it should be part of pad ops, it is used now as alternative to 
> > ENUMINPUT and we agreed that it should(if we are going to implement it) be 
> > part of pad ops.
> > 
> >>
> >> Yes, I was wondering the same. The status bits are ancient and we might
> >> want to improve on it.
> >>
> >> Ramzi, what exactly is your use-case? Is this for an HDMI input? Analog
> >> video input? Before adding a new ioctl I'd like to know why you think
> >> you need it :-)
> > 
> > I need to know if the input signal(analog and digital) is there, before
> > starting streaming. I am not aware of other way to check for it.
> 
> You can check for digital timings using QUERY_DV_TIMINGS: it will return an
> error if there is no lock. Subscribe to the SOURCE_CHANGE event to know when
> the signal is found/lost.
> 
> For analog timings you have QUERYSTD and the same event. The main limitation
> with analog (SDTV) video receivers is that not all analog receivers have implemented
> this event. But the adv7180 and tvp5150 do support this.
> 
> This is the recommended way of implementing this and it has the advantage of
> being interrupt-driven, so no need to poll for a status in the application.

Thank you Hans, things are clear now.

> 
> Regards,
> 
> 	Hans
> 
> > 
> >>
> >> Regards,
> >>
> >> 	Hans
> >>
> >>>
> >>>>>>>>> This commit enables VIDIOC_ENUMINPUT, VIDIOC_G_INPUT, and VIDIOC_S_INPUT
> >>>>>>>>> ioctls for use via v4l2 subdevice node.
> >>>>>>>>>
> >>>>>>>>> This commit should probably not be pushed upstream, because the (old)
> >>>>>>>>> idea of video inputs conflicts with the newer concept of establishing
> >>>>>>>>> media links between src->sink pads.
> >>>>>>>>>
> >>>>>>>>> However it might make sense for some subdevices to support enum/get/set
> >>>>>>>>> inputs. One example would be the analog front end subdevice for the
> >>>>>>>>> ADV748x. By providing these ioctls, selecting the ADV748x analog inputs
> >>>>>>>>> can be done without requiring the implementation of media entities that
> >>>>>>>>> would define the analog source for which to establish a media link.
> >>>>>>>>>
> >>>>>>>>> Signed-off-by: Steve Longerbeam <steve_longerbeam@mentor.com>
> >>>>>>>>> ---
> >>>>>>>>>  drivers/media/v4l2-core/v4l2-subdev.c |  9 +++++++++
> >>>>>>>>>  include/media/v4l2-subdev.h           | 11 +++++++++++
> >>>>>>>>>  2 files changed, 20 insertions(+)
> >>>>>>>>>
> >>>>>>>>> diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c
> >>>>>>>>> index 6b989fe..73fbfe9 100644
> >>>>>>>>> --- a/drivers/media/v4l2-core/v4l2-subdev.c
> >>>>>>>>> +++ b/drivers/media/v4l2-core/v4l2-subdev.c
> >>>>>>>>> @@ -378,6 +378,15 @@ static long subdev_do_ioctl(struct file *file, unsigned int cmd, void *arg)
> >>>>>>>>>  			return -ENOTTY;
> >>>>>>>>>  		return v4l2_querymenu(vfh->ctrl_handler, arg);
> >>>>>>>>>
> >>>>>>>>> +	case VIDIOC_ENUMINPUT:
> >>>>>>>>> +		return v4l2_subdev_call(sd, video, enuminput, arg);
> >>>>>>>>> +
> >>>>>>>>> +	case VIDIOC_G_INPUT:
> >>>>>>>>> +		return v4l2_subdev_call(sd, video, g_input, arg);
> >>>>>>>>> +
> >>>>>>>>> +	case VIDIOC_S_INPUT:
> >>>>>>>>> +		return v4l2_subdev_call(sd, video, s_input, *(u32 *)arg);
> >>>>>>>>> +
> >>>>>>>>>  	case VIDIOC_G_CTRL:
> >>>>>>>>>  		if (!vfh->ctrl_handler)
> >>>>>>>>>  			return -ENOTTY;
> >>>>>>>>> diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
> >>>>>>>>> index f7fe78a..6e1a9cd 100644
> >>>>>>>>> --- a/include/media/v4l2-subdev.h
> >>>>>>>>> +++ b/include/media/v4l2-subdev.h
> >>>>>>>>> @@ -383,6 +383,14 @@ struct v4l2_mbus_frame_desc {
> >>>>>>>>>   * @g_input_status: get input status. Same as the status field in the
> >>>>>>>>>   *	&struct &v4l2_input
> >>>>>>>>>   *
> >>>>>>>>> + * @enuminput: enumerate inputs. Should return the same input status as
> >>>>>>>>> + *      @g_input_status if the passed input index is the currently active
> >>>>>>>>> + *      input.
> >>>>>>>>> + *
> >>>>>>>>> + * @g_input: returns the currently active input index.
> >>>>>>>>> + *
> >>>>>>>>> + * @s_input: set the active input.
> >>>>>>>>> + *
> >>>>>>>>>   * @s_stream: used to notify the driver that a video stream will start or has
> >>>>>>>>>   *	stopped.
> >>>>>>>>>   *
> >>>>>>>>> @@ -423,6 +431,9 @@ struct v4l2_subdev_video_ops {
> >>>>>>>>>  	int (*g_tvnorms)(struct v4l2_subdev *sd, v4l2_std_id *std);
> >>>>>>>>>  	int (*g_tvnorms_output)(struct v4l2_subdev *sd, v4l2_std_id *std);
> >>>>>>>>>  	int (*g_input_status)(struct v4l2_subdev *sd, u32 *status);
> >>>>>>>>> +	int (*enuminput)(struct v4l2_subdev *sd, struct v4l2_input *input);
> >>>>>>>>> +	int (*g_input)(struct v4l2_subdev *sd, u32 *index);
> >>>>>>>>> +	int (*s_input)(struct v4l2_subdev *sd, u32 index);
> >>>>>>>>>  	int (*s_stream)(struct v4l2_subdev *sd, int enable);
> >>>>>>>>>  	int (*g_pixelaspect)(struct v4l2_subdev *sd, struct v4l2_fract *aspect);
> >>>>>>>>>  	int (*g_frame_interval)(struct v4l2_subdev *sd,
> >>>
> >>
> > 
> 

-- 
Best Regards,
Ramzi Ben Meftah.

Re: [PATCH 1/3] v4l2-subdev: Add subdev ioctl support for ENUM/GET/SET INPUT

[Steve Longerbeam]

Hi Laurent, all,

On 6/24/20 7:01 PM, Laurent Pinchart wrote:
> Hi Jacopo,
>
> On Wed, Jun 24, 2020 at 09:53:07AM +0200, Jacopo Mondi wrote:
>> On Tue, Jun 16, 2020 at 12:00:15PM +0200, Ramzi BEN MEFTAH wrote:
>>> From: Steve Longerbeam <steve_longerbeam@mentor.com>
>>   +Niklas, +Laurent
>>
>> Niklas, Laurent, how does this play with CAP_IO_MC ?
> I don't think it's related to CAP_IO_MC, but I don't think it's a good
> idea either :-) Routing doesn't go through the subdev [gs]_input
> operations in MC-based drivers. It should be configured through link
> setup instead. This patch goes in the wrong direction, sorry Steve.

That's OK! :) I didn't submit this patch, and as stated in the commit 
header, I never recommended this patch be submitted to upstream in the 
first place.

Selecting inputs at a subdev should normally make use of media link 
setup. But for selecting analog signal inputs, such as the ADV748x AFE 
standard definition inputs, that would  mean there would need to exist 
source "analog" subdevs that connect to the AFE inputs, if only for the 
purpose of selecting those inputs, which seems silly IMHO. The ADV748x 
AFE subdev defines these inputs as media pads, but have no connections, 
so link setup API can't be used to select those inputs.

So a new subdev pad API is clearly needed, not just to get input status 
at a subdev pad, but to select/make active such analog inputs without 
requiring link setup API.

Steve



>
>>> This commit enables VIDIOC_ENUMINPUT, VIDIOC_G_INPUT, and VIDIOC_S_INPUT
>>> ioctls for use via v4l2 subdevice node.
>>>
>>> This commit should probably not be pushed upstream, because the (old)
>>> idea of video inputs conflicts with the newer concept of establishing
>>> media links between src->sink pads.
>>>
>>> However it might make sense for some subdevices to support enum/get/set
>>> inputs. One example would be the analog front end subdevice for the
>>> ADV748x. By providing these ioctls, selecting the ADV748x analog inputs
>>> can be done without requiring the implementation of media entities that
>>> would define the analog source for which to establish a media link.
>>>
>>> Signed-off-by: Steve Longerbeam <steve_longerbeam@mentor.com>
>>> ---
>>>   drivers/media/v4l2-core/v4l2-subdev.c |  9 +++++++++
>>>   include/media/v4l2-subdev.h           | 11 +++++++++++
>>>   2 files changed, 20 insertions(+)
>>>
>>> diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c
>>> index 6b989fe..73fbfe9 100644
>>> --- a/drivers/media/v4l2-core/v4l2-subdev.c
>>> +++ b/drivers/media/v4l2-core/v4l2-subdev.c
>>> @@ -378,6 +378,15 @@ static long subdev_do_ioctl(struct file *file, unsigned int cmd, void *arg)
>>>   			return -ENOTTY;
>>>   		return v4l2_querymenu(vfh->ctrl_handler, arg);
>>>
>>> +	case VIDIOC_ENUMINPUT:
>>> +		return v4l2_subdev_call(sd, video, enuminput, arg);
>>> +
>>> +	case VIDIOC_G_INPUT:
>>> +		return v4l2_subdev_call(sd, video, g_input, arg);
>>> +
>>> +	case VIDIOC_S_INPUT:
>>> +		return v4l2_subdev_call(sd, video, s_input, *(u32 *)arg);
>>> +
>>>   	case VIDIOC_G_CTRL:
>>>   		if (!vfh->ctrl_handler)
>>>   			return -ENOTTY;
>>> diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
>>> index f7fe78a..6e1a9cd 100644
>>> --- a/include/media/v4l2-subdev.h
>>> +++ b/include/media/v4l2-subdev.h
>>> @@ -383,6 +383,14 @@ struct v4l2_mbus_frame_desc {
>>>    * @g_input_status: get input status. Same as the status field in the
>>>    *	&struct &v4l2_input
>>>    *
>>> + * @enuminput: enumerate inputs. Should return the same input status as
>>> + *      @g_input_status if the passed input index is the currently active
>>> + *      input.
>>> + *
>>> + * @g_input: returns the currently active input index.
>>> + *
>>> + * @s_input: set the active input.
>>> + *
>>>    * @s_stream: used to notify the driver that a video stream will start or has
>>>    *	stopped.
>>>    *
>>> @@ -423,6 +431,9 @@ struct v4l2_subdev_video_ops {
>>>   	int (*g_tvnorms)(struct v4l2_subdev *sd, v4l2_std_id *std);
>>>   	int (*g_tvnorms_output)(struct v4l2_subdev *sd, v4l2_std_id *std);
>>>   	int (*g_input_status)(struct v4l2_subdev *sd, u32 *status);
>>> +	int (*enuminput)(struct v4l2_subdev *sd, struct v4l2_input *input);
>>> +	int (*g_input)(struct v4l2_subdev *sd, u32 *index);
>>> +	int (*s_input)(struct v4l2_subdev *sd, u32 index);
>>>   	int (*s_stream)(struct v4l2_subdev *sd, int enable);
>>>   	int (*g_pixelaspect)(struct v4l2_subdev *sd, struct v4l2_fract *aspect);
>>>   	int (*g_frame_interval)(struct v4l2_subdev *sd,

Re: [PATCH 1/3] v4l2-subdev: Add subdev ioctl support for ENUM/GET/SET INPUT

[Laurent Pinchart]

Hi Steve,

On Thu, Jun 25, 2020 at 10:41:09AM -0700, Steve Longerbeam wrote:
> On 6/24/20 7:01 PM, Laurent Pinchart wrote:
> > On Wed, Jun 24, 2020 at 09:53:07AM +0200, Jacopo Mondi wrote:
> >> On Tue, Jun 16, 2020 at 12:00:15PM +0200, Ramzi BEN MEFTAH wrote:
> >>> From: Steve Longerbeam <steve_longerbeam@mentor.com>
> >>   +Niklas, +Laurent
> >>
> >> Niklas, Laurent, how does this play with CAP_IO_MC ?
> > I don't think it's related to CAP_IO_MC, but I don't think it's a good
> > idea either :-) Routing doesn't go through the subdev [gs]_input
> > operations in MC-based drivers. It should be configured through link
> > setup instead. This patch goes in the wrong direction, sorry Steve.
> 
> That's OK! :) I didn't submit this patch, and as stated in the commit 
> header, I never recommended this patch be submitted to upstream in the 
> first place.
> 
> Selecting inputs at a subdev should normally make use of media link 
> setup. But for selecting analog signal inputs, such as the ADV748x AFE 
> standard definition inputs, that would  mean there would need to exist 
> source "analog" subdevs that connect to the AFE inputs, if only for the 
> purpose of selecting those inputs, which seems silly IMHO. The ADV748x 
> AFE subdev defines these inputs as media pads, but have no connections, 
> so link setup API can't be used to select those inputs.
> 
> So a new subdev pad API is clearly needed, not just to get input status 
> at a subdev pad, but to select/make active such analog inputs without 
> requiring link setup API.

There was an attempt to create a subdev ioctl to configure internal
routing. See "[PATCH v4 19/31] media: Documentation: Add GS_ROUTING
documentation" ([1]) and the related patches in the series.

[1] https://lore.kernel.org/linux-media/20190328200608.9463-20-jacopo+renesas@jmondi.org/

> >>> This commit enables VIDIOC_ENUMINPUT, VIDIOC_G_INPUT, and VIDIOC_S_INPUT
> >>> ioctls for use via v4l2 subdevice node.
> >>>
> >>> This commit should probably not be pushed upstream, because the (old)
> >>> idea of video inputs conflicts with the newer concept of establishing
> >>> media links between src->sink pads.
> >>>
> >>> However it might make sense for some subdevices to support enum/get/set
> >>> inputs. One example would be the analog front end subdevice for the
> >>> ADV748x. By providing these ioctls, selecting the ADV748x analog inputs
> >>> can be done without requiring the implementation of media entities that
> >>> would define the analog source for which to establish a media link.
> >>>
> >>> Signed-off-by: Steve Longerbeam <steve_longerbeam@mentor.com>
> >>> ---
> >>>   drivers/media/v4l2-core/v4l2-subdev.c |  9 +++++++++
> >>>   include/media/v4l2-subdev.h           | 11 +++++++++++
> >>>   2 files changed, 20 insertions(+)
> >>>
> >>> diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c
> >>> index 6b989fe..73fbfe9 100644
> >>> --- a/drivers/media/v4l2-core/v4l2-subdev.c
> >>> +++ b/drivers/media/v4l2-core/v4l2-subdev.c
> >>> @@ -378,6 +378,15 @@ static long subdev_do_ioctl(struct file *file, unsigned int cmd, void *arg)
> >>>   			return -ENOTTY;
> >>>   		return v4l2_querymenu(vfh->ctrl_handler, arg);
> >>>
> >>> +	case VIDIOC_ENUMINPUT:
> >>> +		return v4l2_subdev_call(sd, video, enuminput, arg);
> >>> +
> >>> +	case VIDIOC_G_INPUT:
> >>> +		return v4l2_subdev_call(sd, video, g_input, arg);
> >>> +
> >>> +	case VIDIOC_S_INPUT:
> >>> +		return v4l2_subdev_call(sd, video, s_input, *(u32 *)arg);
> >>> +
> >>>   	case VIDIOC_G_CTRL:
> >>>   		if (!vfh->ctrl_handler)
> >>>   			return -ENOTTY;
> >>> diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
> >>> index f7fe78a..6e1a9cd 100644
> >>> --- a/include/media/v4l2-subdev.h
> >>> +++ b/include/media/v4l2-subdev.h
> >>> @@ -383,6 +383,14 @@ struct v4l2_mbus_frame_desc {
> >>>    * @g_input_status: get input status. Same as the status field in the
> >>>    *	&struct &v4l2_input
> >>>    *
> >>> + * @enuminput: enumerate inputs. Should return the same input status as
> >>> + *      @g_input_status if the passed input index is the currently active
> >>> + *      input.
> >>> + *
> >>> + * @g_input: returns the currently active input index.
> >>> + *
> >>> + * @s_input: set the active input.
> >>> + *
> >>>    * @s_stream: used to notify the driver that a video stream will start or has
> >>>    *	stopped.
> >>>    *
> >>> @@ -423,6 +431,9 @@ struct v4l2_subdev_video_ops {
> >>>   	int (*g_tvnorms)(struct v4l2_subdev *sd, v4l2_std_id *std);
> >>>   	int (*g_tvnorms_output)(struct v4l2_subdev *sd, v4l2_std_id *std);
> >>>   	int (*g_input_status)(struct v4l2_subdev *sd, u32 *status);
> >>> +	int (*enuminput)(struct v4l2_subdev *sd, struct v4l2_input *input);
> >>> +	int (*g_input)(struct v4l2_subdev *sd, u32 *index);
> >>> +	int (*s_input)(struct v4l2_subdev *sd, u32 index);
> >>>   	int (*s_stream)(struct v4l2_subdev *sd, int enable);
> >>>   	int (*g_pixelaspect)(struct v4l2_subdev *sd, struct v4l2_fract *aspect);
> >>>   	int (*g_frame_interval)(struct v4l2_subdev *sd,
> 

-- 
Regards,

Laurent Pinchart

Re: [PATCH 1/3] v4l2-subdev: Add subdev ioctl support for ENUM/GET/SET INPUT

[Ramzi Ben Meftah]

Hi Laurent,

On Fri, Jun 26, 2020 at 04:12:51AM +0300, Laurent Pinchart wrote:
> Hi Steve,
> 
> On Thu, Jun 25, 2020 at 10:41:09AM -0700, Steve Longerbeam wrote:
> > On 6/24/20 7:01 PM, Laurent Pinchart wrote:
> > > On Wed, Jun 24, 2020 at 09:53:07AM +0200, Jacopo Mondi wrote:
> > >> On Tue, Jun 16, 2020 at 12:00:15PM +0200, Ramzi BEN MEFTAH wrote:
> > >>> From: Steve Longerbeam <steve_longerbeam@mentor.com>
> > >>   +Niklas, +Laurent
> > >>
> > >> Niklas, Laurent, how does this play with CAP_IO_MC ?
> > > I don't think it's related to CAP_IO_MC, but I don't think it's a good
> > > idea either :-) Routing doesn't go through the subdev [gs]_input
> > > operations in MC-based drivers. It should be configured through link
> > > setup instead. This patch goes in the wrong direction, sorry Steve.
> > 
> > That's OK! :) I didn't submit this patch, and as stated in the commit 
> > header, I never recommended this patch be submitted to upstream in the 
> > first place.
> > 
> > Selecting inputs at a subdev should normally make use of media link 
> > setup. But for selecting analog signal inputs, such as the ADV748x AFE 
> > standard definition inputs, that would  mean there would need to exist 
> > source "analog" subdevs that connect to the AFE inputs, if only for the 
> > purpose of selecting those inputs, which seems silly IMHO. The ADV748x 
> > AFE subdev defines these inputs as media pads, but have no connections, 
> > so link setup API can't be used to select those inputs.
> > 
> > So a new subdev pad API is clearly needed, not just to get input status 
> > at a subdev pad, but to select/make active such analog inputs without 
> > requiring link setup API.
> 
> There was an attempt to create a subdev ioctl to configure internal
> routing. See "[PATCH v4 19/31] media: Documentation: Add GS_ROUTING
> documentation" ([1]) and the related patches in the series.
> 
> [1] https://lore.kernel.org/linux-media/20190328200608.9463-20-jacopo+renesas@jmondi.org/
> 

I was thinking why not just allowing linking pads of the same media entity.
This will avoid adding a new control, and will do the same as S_INPUT ioctl.

> > >>> This commit enables VIDIOC_ENUMINPUT, VIDIOC_G_INPUT, and VIDIOC_S_INPUT
> > >>> ioctls for use via v4l2 subdevice node.
> > >>>
> > >>> This commit should probably not be pushed upstream, because the (old)
> > >>> idea of video inputs conflicts with the newer concept of establishing
> > >>> media links between src->sink pads.
> > >>>
> > >>> However it might make sense for some subdevices to support enum/get/set
> > >>> inputs. One example would be the analog front end subdevice for the
> > >>> ADV748x. By providing these ioctls, selecting the ADV748x analog inputs
> > >>> can be done without requiring the implementation of media entities that
> > >>> would define the analog source for which to establish a media link.
> > >>>
> > >>> Signed-off-by: Steve Longerbeam <steve_longerbeam@mentor.com>
> > >>> ---
> > >>>   drivers/media/v4l2-core/v4l2-subdev.c |  9 +++++++++
> > >>>   include/media/v4l2-subdev.h           | 11 +++++++++++
> > >>>   2 files changed, 20 insertions(+)
> > >>>
> > >>> diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c
> > >>> index 6b989fe..73fbfe9 100644
> > >>> --- a/drivers/media/v4l2-core/v4l2-subdev.c
> > >>> +++ b/drivers/media/v4l2-core/v4l2-subdev.c
> > >>> @@ -378,6 +378,15 @@ static long subdev_do_ioctl(struct file *file, unsigned int cmd, void *arg)
> > >>>   			return -ENOTTY;
> > >>>   		return v4l2_querymenu(vfh->ctrl_handler, arg);
> > >>>
> > >>> +	case VIDIOC_ENUMINPUT:
> > >>> +		return v4l2_subdev_call(sd, video, enuminput, arg);
> > >>> +
> > >>> +	case VIDIOC_G_INPUT:
> > >>> +		return v4l2_subdev_call(sd, video, g_input, arg);
> > >>> +
> > >>> +	case VIDIOC_S_INPUT:
> > >>> +		return v4l2_subdev_call(sd, video, s_input, *(u32 *)arg);
> > >>> +
> > >>>   	case VIDIOC_G_CTRL:
> > >>>   		if (!vfh->ctrl_handler)
> > >>>   			return -ENOTTY;
> > >>> diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
> > >>> index f7fe78a..6e1a9cd 100644
> > >>> --- a/include/media/v4l2-subdev.h
> > >>> +++ b/include/media/v4l2-subdev.h
> > >>> @@ -383,6 +383,14 @@ struct v4l2_mbus_frame_desc {
> > >>>    * @g_input_status: get input status. Same as the status field in the
> > >>>    *	&struct &v4l2_input
> > >>>    *
> > >>> + * @enuminput: enumerate inputs. Should return the same input status as
> > >>> + *      @g_input_status if the passed input index is the currently active
> > >>> + *      input.
> > >>> + *
> > >>> + * @g_input: returns the currently active input index.
> > >>> + *
> > >>> + * @s_input: set the active input.
> > >>> + *
> > >>>    * @s_stream: used to notify the driver that a video stream will start or has
> > >>>    *	stopped.
> > >>>    *
> > >>> @@ -423,6 +431,9 @@ struct v4l2_subdev_video_ops {
> > >>>   	int (*g_tvnorms)(struct v4l2_subdev *sd, v4l2_std_id *std);
> > >>>   	int (*g_tvnorms_output)(struct v4l2_subdev *sd, v4l2_std_id *std);
> > >>>   	int (*g_input_status)(struct v4l2_subdev *sd, u32 *status);
> > >>> +	int (*enuminput)(struct v4l2_subdev *sd, struct v4l2_input *input);
> > >>> +	int (*g_input)(struct v4l2_subdev *sd, u32 *index);
> > >>> +	int (*s_input)(struct v4l2_subdev *sd, u32 index);
> > >>>   	int (*s_stream)(struct v4l2_subdev *sd, int enable);
> > >>>   	int (*g_pixelaspect)(struct v4l2_subdev *sd, struct v4l2_fract *aspect);
> > >>>   	int (*g_frame_interval)(struct v4l2_subdev *sd,
> > 
> 
> -- 
> Regards,
> 
> Laurent Pinchart

-- 
Best Regards,
Ramzi Ben Meftah.

Re: [PATCH 1/3] v4l2-subdev: Add subdev ioctl support for ENUM/GET/SET INPUT

[Laurent Pinchart]

Hi Ramzi,

On Mon, Jun 29, 2020 at 11:24:13AM +0200, Ramzi Ben Meftah wrote:
> On Fri, Jun 26, 2020 at 04:12:51AM +0300, Laurent Pinchart wrote:
> > On Thu, Jun 25, 2020 at 10:41:09AM -0700, Steve Longerbeam wrote:
> >> On 6/24/20 7:01 PM, Laurent Pinchart wrote:
> >>> On Wed, Jun 24, 2020 at 09:53:07AM +0200, Jacopo Mondi wrote:
> >>>> On Tue, Jun 16, 2020 at 12:00:15PM +0200, Ramzi BEN MEFTAH wrote:
> >>>>> From: Steve Longerbeam <steve_longerbeam@mentor.com>
> >>>>   +Niklas, +Laurent
> >>>>
> >>>> Niklas, Laurent, how does this play with CAP_IO_MC ?
> >>> I don't think it's related to CAP_IO_MC, but I don't think it's a good
> >>> idea either :-) Routing doesn't go through the subdev [gs]_input
> >>> operations in MC-based drivers. It should be configured through link
> >>> setup instead. This patch goes in the wrong direction, sorry Steve.
> >> 
> >> That's OK! :) I didn't submit this patch, and as stated in the commit 
> >> header, I never recommended this patch be submitted to upstream in the 
> >> first place.
> >> 
> >> Selecting inputs at a subdev should normally make use of media link 
> >> setup. But for selecting analog signal inputs, such as the ADV748x AFE 
> >> standard definition inputs, that would  mean there would need to exist 
> >> source "analog" subdevs that connect to the AFE inputs, if only for the 
> >> purpose of selecting those inputs, which seems silly IMHO. The ADV748x 
> >> AFE subdev defines these inputs as media pads, but have no connections, 
> >> so link setup API can't be used to select those inputs.
> >> 
> >> So a new subdev pad API is clearly needed, not just to get input status 
> >> at a subdev pad, but to select/make active such analog inputs without 
> >> requiring link setup API.
> > 
> > There was an attempt to create a subdev ioctl to configure internal
> > routing. See "[PATCH v4 19/31] media: Documentation: Add GS_ROUTING
> > documentation" ([1]) and the related patches in the series.
> > 
> > [1] https://lore.kernel.org/linux-media/20190328200608.9463-20-jacopo+renesas@jmondi.org/
> 
> I was thinking why not just allowing linking pads of the same media entity.
> This will avoid adding a new control, and will do the same as S_INPUT ioctl.

The MC link setup API doesn't offer a way to configure multiple links in
one operation, which is often needed for internal routing. There's also
the issue that extensive changes would likely be needed for the MC core,
although that's "just" a matter of implementing it.

> >>>>> This commit enables VIDIOC_ENUMINPUT, VIDIOC_G_INPUT, and VIDIOC_S_INPUT
> >>>>> ioctls for use via v4l2 subdevice node.
> >>>>>
> >>>>> This commit should probably not be pushed upstream, because the (old)
> >>>>> idea of video inputs conflicts with the newer concept of establishing
> >>>>> media links between src->sink pads.
> >>>>>
> >>>>> However it might make sense for some subdevices to support enum/get/set
> >>>>> inputs. One example would be the analog front end subdevice for the
> >>>>> ADV748x. By providing these ioctls, selecting the ADV748x analog inputs
> >>>>> can be done without requiring the implementation of media entities that
> >>>>> would define the analog source for which to establish a media link.
> >>>>>
> >>>>> Signed-off-by: Steve Longerbeam <steve_longerbeam@mentor.com>
> >>>>> ---
> >>>>>   drivers/media/v4l2-core/v4l2-subdev.c |  9 +++++++++
> >>>>>   include/media/v4l2-subdev.h           | 11 +++++++++++
> >>>>>   2 files changed, 20 insertions(+)
> >>>>>
> >>>>> diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c
> >>>>> index 6b989fe..73fbfe9 100644
> >>>>> --- a/drivers/media/v4l2-core/v4l2-subdev.c
> >>>>> +++ b/drivers/media/v4l2-core/v4l2-subdev.c
> >>>>> @@ -378,6 +378,15 @@ static long subdev_do_ioctl(struct file *file, unsigned int cmd, void *arg)
> >>>>>   			return -ENOTTY;
> >>>>>   		return v4l2_querymenu(vfh->ctrl_handler, arg);
> >>>>>
> >>>>> +	case VIDIOC_ENUMINPUT:
> >>>>> +		return v4l2_subdev_call(sd, video, enuminput, arg);
> >>>>> +
> >>>>> +	case VIDIOC_G_INPUT:
> >>>>> +		return v4l2_subdev_call(sd, video, g_input, arg);
> >>>>> +
> >>>>> +	case VIDIOC_S_INPUT:
> >>>>> +		return v4l2_subdev_call(sd, video, s_input, *(u32 *)arg);
> >>>>> +
> >>>>>   	case VIDIOC_G_CTRL:
> >>>>>   		if (!vfh->ctrl_handler)
> >>>>>   			return -ENOTTY;
> >>>>> diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
> >>>>> index f7fe78a..6e1a9cd 100644
> >>>>> --- a/include/media/v4l2-subdev.h
> >>>>> +++ b/include/media/v4l2-subdev.h
> >>>>> @@ -383,6 +383,14 @@ struct v4l2_mbus_frame_desc {
> >>>>>    * @g_input_status: get input status. Same as the status field in the
> >>>>>    *	&struct &v4l2_input
> >>>>>    *
> >>>>> + * @enuminput: enumerate inputs. Should return the same input status as
> >>>>> + *      @g_input_status if the passed input index is the currently active
> >>>>> + *      input.
> >>>>> + *
> >>>>> + * @g_input: returns the currently active input index.
> >>>>> + *
> >>>>> + * @s_input: set the active input.
> >>>>> + *
> >>>>>    * @s_stream: used to notify the driver that a video stream will start or has
> >>>>>    *	stopped.
> >>>>>    *
> >>>>> @@ -423,6 +431,9 @@ struct v4l2_subdev_video_ops {
> >>>>>   	int (*g_tvnorms)(struct v4l2_subdev *sd, v4l2_std_id *std);
> >>>>>   	int (*g_tvnorms_output)(struct v4l2_subdev *sd, v4l2_std_id *std);
> >>>>>   	int (*g_input_status)(struct v4l2_subdev *sd, u32 *status);
> >>>>> +	int (*enuminput)(struct v4l2_subdev *sd, struct v4l2_input *input);
> >>>>> +	int (*g_input)(struct v4l2_subdev *sd, u32 *index);
> >>>>> +	int (*s_input)(struct v4l2_subdev *sd, u32 index);
> >>>>>   	int (*s_stream)(struct v4l2_subdev *sd, int enable);
> >>>>>   	int (*g_pixelaspect)(struct v4l2_subdev *sd, struct v4l2_fract *aspect);
> >>>>>   	int (*g_frame_interval)(struct v4l2_subdev *sd,

-- 
Regards,

Laurent Pinchart

Re: [PATCH 1/3] v4l2-subdev: Add subdev ioctl support for ENUM/GET/SET INPUT

[Hans Verkuil]

On 16/06/2020 12:00, Ramzi BEN MEFTAH wrote:
> From: Steve Longerbeam <steve_longerbeam@mentor.com>
> 
> This commit enables VIDIOC_ENUMINPUT, VIDIOC_G_INPUT, and VIDIOC_S_INPUT
> ioctls for use via v4l2 subdevice node.
> 
> This commit should probably not be pushed upstream, because the (old)
> idea of video inputs conflicts with the newer concept of establishing
> media links between src->sink pads.
> 
> However it might make sense for some subdevices to support enum/get/set
> inputs. One example would be the analog front end subdevice for the
> ADV748x. By providing these ioctls, selecting the ADV748x analog inputs
> can be done without requiring the implementation of media entities that
> would define the analog source for which to establish a media link.
> 
> Signed-off-by: Steve Longerbeam <steve_longerbeam@mentor.com>

Nacked-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>

This doesn't work: these ioctls refer to physical inputs on a backplane
of a device. But subdevices have no idea about that. This is high-level
information that is typically only known to a bridge driver based on
board information.

For PCI/USB drivers this comes from card definitions in the driver itself.

For platform drivers this should come from the device tree, but this hasn't
been fully implemented yet.

So if this is something that you want to implement, then look at:

Documentation/devicetree/bindings/display/connector/hdmi-connector.txt

and add this to the DT for your board, and implement code to query this
in the platform driver.

Regards,

	Hans

> ---
>  drivers/media/v4l2-core/v4l2-subdev.c |  9 +++++++++
>  include/media/v4l2-subdev.h           | 11 +++++++++++
>  2 files changed, 20 insertions(+)
> 
> diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c
> index 6b989fe..73fbfe9 100644
> --- a/drivers/media/v4l2-core/v4l2-subdev.c
> +++ b/drivers/media/v4l2-core/v4l2-subdev.c
> @@ -378,6 +378,15 @@ static long subdev_do_ioctl(struct file *file, unsigned int cmd, void *arg)
>  			return -ENOTTY;
>  		return v4l2_querymenu(vfh->ctrl_handler, arg);
>  
> +	case VIDIOC_ENUMINPUT:
> +		return v4l2_subdev_call(sd, video, enuminput, arg);
> +
> +	case VIDIOC_G_INPUT:
> +		return v4l2_subdev_call(sd, video, g_input, arg);
> +
> +	case VIDIOC_S_INPUT:
> +		return v4l2_subdev_call(sd, video, s_input, *(u32 *)arg);
> +
>  	case VIDIOC_G_CTRL:
>  		if (!vfh->ctrl_handler)
>  			return -ENOTTY;
> diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
> index f7fe78a..6e1a9cd 100644
> --- a/include/media/v4l2-subdev.h
> +++ b/include/media/v4l2-subdev.h
> @@ -383,6 +383,14 @@ struct v4l2_mbus_frame_desc {
>   * @g_input_status: get input status. Same as the status field in the
>   *	&struct &v4l2_input
>   *
> + * @enuminput: enumerate inputs. Should return the same input status as
> + *      @g_input_status if the passed input index is the currently active
> + *      input.
> + *
> + * @g_input: returns the currently active input index.
> + *
> + * @s_input: set the active input.
> + *
>   * @s_stream: used to notify the driver that a video stream will start or has
>   *	stopped.
>   *
> @@ -423,6 +431,9 @@ struct v4l2_subdev_video_ops {
>  	int (*g_tvnorms)(struct v4l2_subdev *sd, v4l2_std_id *std);
>  	int (*g_tvnorms_output)(struct v4l2_subdev *sd, v4l2_std_id *std);
>  	int (*g_input_status)(struct v4l2_subdev *sd, u32 *status);
> +	int (*enuminput)(struct v4l2_subdev *sd, struct v4l2_input *input);
> +	int (*g_input)(struct v4l2_subdev *sd, u32 *index);
> +	int (*s_input)(struct v4l2_subdev *sd, u32 index);
>  	int (*s_stream)(struct v4l2_subdev *sd, int enable);
>  	int (*g_pixelaspect)(struct v4l2_subdev *sd, struct v4l2_fract *aspect);
>  	int (*g_frame_interval)(struct v4l2_subdev *sd,
> 

Re: [PATCH 1/3] v4l2-subdev: Add subdev ioctl support for ENUM/GET/SET INPUT

[Hans Verkuil]

On 24/06/2020 12:08, Hans Verkuil wrote:
> On 16/06/2020 12:00, Ramzi BEN MEFTAH wrote:
>> From: Steve Longerbeam <steve_longerbeam@mentor.com>
>>
>> This commit enables VIDIOC_ENUMINPUT, VIDIOC_G_INPUT, and VIDIOC_S_INPUT
>> ioctls for use via v4l2 subdevice node.
>>
>> This commit should probably not be pushed upstream, because the (old)
>> idea of video inputs conflicts with the newer concept of establishing
>> media links between src->sink pads.
>>
>> However it might make sense for some subdevices to support enum/get/set
>> inputs. One example would be the analog front end subdevice for the
>> ADV748x. By providing these ioctls, selecting the ADV748x analog inputs
>> can be done without requiring the implementation of media entities that
>> would define the analog source for which to establish a media link.
>>
>> Signed-off-by: Steve Longerbeam <steve_longerbeam@mentor.com>
> 
> Nacked-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
> 
> This doesn't work: these ioctls refer to physical inputs on a backplane
> of a device. But subdevices have no idea about that. This is high-level
> information that is typically only known to a bridge driver based on
> board information.
> 
> For PCI/USB drivers this comes from card definitions in the driver itself.
> 
> For platform drivers this should come from the device tree, but this hasn't
> been fully implemented yet.
> 
> So if this is something that you want to implement, then look at:
> 
> Documentation/devicetree/bindings/display/connector/hdmi-connector.txt
> 
> and add this to the DT for your board, and implement code to query this
> in the platform driver.

Follow-up: in system with a media device and v4l-subdev devices (so MC-centric)
this might make sense, provided the connector data is obtained from the DT.

An alternative is to expose the connectors in the media topology and use
SETUP_LINK to choose which connector to pick.

I don't have a strong preference, but in both cases this information should
come from the device tree.

Regards,

	Hans

> 
> Regards,
> 
> 	Hans
> 
>> ---
>>  drivers/media/v4l2-core/v4l2-subdev.c |  9 +++++++++
>>  include/media/v4l2-subdev.h           | 11 +++++++++++
>>  2 files changed, 20 insertions(+)
>>
>> diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c
>> index 6b989fe..73fbfe9 100644
>> --- a/drivers/media/v4l2-core/v4l2-subdev.c
>> +++ b/drivers/media/v4l2-core/v4l2-subdev.c
>> @@ -378,6 +378,15 @@ static long subdev_do_ioctl(struct file *file, unsigned int cmd, void *arg)
>>  			return -ENOTTY;
>>  		return v4l2_querymenu(vfh->ctrl_handler, arg);
>>  
>> +	case VIDIOC_ENUMINPUT:
>> +		return v4l2_subdev_call(sd, video, enuminput, arg);
>> +
>> +	case VIDIOC_G_INPUT:
>> +		return v4l2_subdev_call(sd, video, g_input, arg);
>> +
>> +	case VIDIOC_S_INPUT:
>> +		return v4l2_subdev_call(sd, video, s_input, *(u32 *)arg);
>> +
>>  	case VIDIOC_G_CTRL:
>>  		if (!vfh->ctrl_handler)
>>  			return -ENOTTY;
>> diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
>> index f7fe78a..6e1a9cd 100644
>> --- a/include/media/v4l2-subdev.h
>> +++ b/include/media/v4l2-subdev.h
>> @@ -383,6 +383,14 @@ struct v4l2_mbus_frame_desc {
>>   * @g_input_status: get input status. Same as the status field in the
>>   *	&struct &v4l2_input
>>   *
>> + * @enuminput: enumerate inputs. Should return the same input status as
>> + *      @g_input_status if the passed input index is the currently active
>> + *      input.
>> + *
>> + * @g_input: returns the currently active input index.
>> + *
>> + * @s_input: set the active input.
>> + *
>>   * @s_stream: used to notify the driver that a video stream will start or has
>>   *	stopped.
>>   *
>> @@ -423,6 +431,9 @@ struct v4l2_subdev_video_ops {
>>  	int (*g_tvnorms)(struct v4l2_subdev *sd, v4l2_std_id *std);
>>  	int (*g_tvnorms_output)(struct v4l2_subdev *sd, v4l2_std_id *std);
>>  	int (*g_input_status)(struct v4l2_subdev *sd, u32 *status);
>> +	int (*enuminput)(struct v4l2_subdev *sd, struct v4l2_input *input);
>> +	int (*g_input)(struct v4l2_subdev *sd, u32 *index);
>> +	int (*s_input)(struct v4l2_subdev *sd, u32 index);
>>  	int (*s_stream)(struct v4l2_subdev *sd, int enable);
>>  	int (*g_pixelaspect)(struct v4l2_subdev *sd, struct v4l2_fract *aspect);
>>  	int (*g_frame_interval)(struct v4l2_subdev *sd,
>>
>