[PATCH 0/3] VIN: call g std() method as requested by Hans

[PATCH 0/3] VIN: call g std() method as requested by Hans

[Sergei Shtylyov]

Hello.

   Here are 3 patches against the 'fixes' branch of the 'media_tree.git' repo
plus the VIN driver patch to propagate querystd() error. First we add support
for the g_std() video method to the ADV7180 and ML86V7667 I2C drivers which
is needed for the full 'soc_camera' compliance, then we replace the quesrystd()
video method call with g_std() in the R-Car VIN driver; 2 former patches are
pre-requisites for the 3rd, otherwise that patch would break the frame capture.

[1/3] adv7180: implement g_std() method
[2/3] ml86v7667-implement-g_std-method
[3/3/] rcar_vin: call g_std() instead of querystd()

MBR, Sergei

[PATCH 1/3] adv7180: implement g_std() method

[Sergei Shtylyov]

Commit cccb83f7a184 ([media] adv7180: add more subdev video ops) forgot to add
the g_std() video method. Its implementation seems identical to the querystd()
method,  so we  can just  point at adv7180_querystd()...

Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>

---
 drivers/media/i2c/adv7180.c |    1 +
 1 file changed, 1 insertion(+)

Index: media_tree/drivers/media/i2c/adv7180.c
===================================================================
--- media_tree.orig/drivers/media/i2c/adv7180.c
+++ media_tree/drivers/media/i2c/adv7180.c
@@ -717,6 +717,7 @@ static int adv7180_g_mbus_config(struct
 }
 
 static const struct v4l2_subdev_video_ops adv7180_video_ops = {
+	.g_std = adv7180_querystd,
 	.s_std = adv7180_s_std,
 	.querystd = adv7180_querystd,
 	.g_input_status = adv7180_g_input_status,

[PATCH 2/3] ml86v7667: implement g_std() method

[Sergei Shtylyov]

The driver was written with the 'soc_camera' use in mind, however the g_std()
video method was forgotten. Implement it at last...

Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>

---
 drivers/media/i2c/ml86v7667.c |   10 ++++++++++
 1 file changed, 10 insertions(+)

Index: media_tree/drivers/media/i2c/ml86v7667.c
===================================================================
--- media_tree.orig/drivers/media/i2c/ml86v7667.c
+++ media_tree/drivers/media/i2c/ml86v7667.c
@@ -233,6 +233,15 @@ static int ml86v7667_g_mbus_config(struc
 	return 0;
 }
 
+static int ml86v7667_g_std(struct v4l2_subdev *sd, v4l2_std_id *std)
+{
+	struct ml86v7667_priv *priv = to_ml86v7667(sd);
+
+	*std = priv->std;
+
+	return 0;
+}
+
 static int ml86v7667_s_std(struct v4l2_subdev *sd, v4l2_std_id std)
 {
 	struct ml86v7667_priv *priv = to_ml86v7667(sd);
@@ -282,6 +291,7 @@ static const struct v4l2_ctrl_ops ml86v7
 };
 
 static struct v4l2_subdev_video_ops ml86v7667_subdev_video_ops = {
+	.g_std = ml86v7667_g_std,
 	.s_std = ml86v7667_s_std,
 	.querystd = ml86v7667_querystd,
 	.g_input_status = ml86v7667_g_input_status,

[PATCH 3/3] rcar_vin: call g_std() instead of querystd()

[Sergei Shtylyov]

Hans Verkuil says: "The only place querystd can be called  is in the QUERYSTD
ioctl, all other ioctls should use the last set standard." So call the g_std()
subdevice method instead of querystd() in the driver's set_fmt() method.

Reported-by: Hans Verkuil <hverkuil@xs4all.nl>
Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>

---
 drivers/media/platform/soc_camera/rcar_vin.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Index: media_tree/drivers/media/platform/soc_camera/rcar_vin.c
===================================================================
--- media_tree.orig/drivers/media/platform/soc_camera/rcar_vin.c
+++ media_tree/drivers/media/platform/soc_camera/rcar_vin.c
@@ -1589,8 +1589,8 @@ static int rcar_vin_set_fmt(struct soc_c
 		field = pix->field;
 		break;
 	case V4L2_FIELD_INTERLACED:
-		/* Query for standard if not explicitly mentioned _TB/_BT */
-		ret = v4l2_subdev_call(sd, video, querystd, &std);
+		/* Get the last standard if not explicitly mentioned _TB/_BT */
+		ret = v4l2_subdev_call(sd, video, g_std, &std);
 		if (ret == -ENOIOCTLCMD) {
 			field = V4L2_FIELD_NONE;
 		} else if (ret < 0) {

Re: [PATCH 2/3] ml86v7667: implement g_std() method

[Hans Verkuil]

On 09/04/2015 01:16 AM, Sergei Shtylyov wrote:
> The driver was written with the 'soc_camera' use in mind, however the g_std()
> video method was forgotten. Implement it at last...
> 
> Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>

Acked-by: Hans Verkuil <hans.verkuil@cisco.com>

> 
> ---
>  drivers/media/i2c/ml86v7667.c |   10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> Index: media_tree/drivers/media/i2c/ml86v7667.c
> ===================================================================
> --- media_tree.orig/drivers/media/i2c/ml86v7667.c
> +++ media_tree/drivers/media/i2c/ml86v7667.c
> @@ -233,6 +233,15 @@ static int ml86v7667_g_mbus_config(struc
>  	return 0;
>  }
>  
> +static int ml86v7667_g_std(struct v4l2_subdev *sd, v4l2_std_id *std)
> +{
> +	struct ml86v7667_priv *priv = to_ml86v7667(sd);
> +
> +	*std = priv->std;
> +
> +	return 0;
> +}
> +
>  static int ml86v7667_s_std(struct v4l2_subdev *sd, v4l2_std_id std)
>  {
>  	struct ml86v7667_priv *priv = to_ml86v7667(sd);
> @@ -282,6 +291,7 @@ static const struct v4l2_ctrl_ops ml86v7
>  };
>  
>  static struct v4l2_subdev_video_ops ml86v7667_subdev_video_ops = {
> +	.g_std = ml86v7667_g_std,
>  	.s_std = ml86v7667_s_std,
>  	.querystd = ml86v7667_querystd,
>  	.g_input_status = ml86v7667_g_input_status,
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-media" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [PATCH 3/3] rcar_vin: call g_std() instead of querystd()

[Hans Verkuil]

On 09/04/2015 01:18 AM, Sergei Shtylyov wrote:
> Hans Verkuil says: "The only place querystd can be called  is in the QUERYSTD
> ioctl, all other ioctls should use the last set standard." So call the g_std()
> subdevice method instead of querystd() in the driver's set_fmt() method.
> 
> Reported-by: Hans Verkuil <hverkuil@xs4all.nl>
> Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>

Acked-by: Hans Verkuil <hans.verkuil@cisco.com>

> 
> ---
>  drivers/media/platform/soc_camera/rcar_vin.c |    4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> Index: media_tree/drivers/media/platform/soc_camera/rcar_vin.c
> ===================================================================
> --- media_tree.orig/drivers/media/platform/soc_camera/rcar_vin.c
> +++ media_tree/drivers/media/platform/soc_camera/rcar_vin.c
> @@ -1589,8 +1589,8 @@ static int rcar_vin_set_fmt(struct soc_c
>  		field = pix->field;
>  		break;
>  	case V4L2_FIELD_INTERLACED:
> -		/* Query for standard if not explicitly mentioned _TB/_BT */
> -		ret = v4l2_subdev_call(sd, video, querystd, &std);
> +		/* Get the last standard if not explicitly mentioned _TB/_BT */
> +		ret = v4l2_subdev_call(sd, video, g_std, &std);
>  		if (ret == -ENOIOCTLCMD) {
>  			field = V4L2_FIELD_NONE;
>  		} else if (ret < 0) {
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-media" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [PATCH 1/3] adv7180: implement g_std() method

[Hans Verkuil]

On 09/04/2015 01:14 AM, Sergei Shtylyov wrote:
> Commit cccb83f7a184 ([media] adv7180: add more subdev video ops) forgot to add
> the g_std() video method. Its implementation seems identical to the querystd()
> method,  so we  can just  point at adv7180_querystd()...
> 
> Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>
> 
> ---
>  drivers/media/i2c/adv7180.c |    1 +
>  1 file changed, 1 insertion(+)
> 
> Index: media_tree/drivers/media/i2c/adv7180.c
> ===================================================================
> --- media_tree.orig/drivers/media/i2c/adv7180.c
> +++ media_tree/drivers/media/i2c/adv7180.c
> @@ -717,6 +717,7 @@ static int adv7180_g_mbus_config(struct
>  }
>  
>  static const struct v4l2_subdev_video_ops adv7180_video_ops = {
> +	.g_std = adv7180_querystd,

No, this isn't right. Not your fault, the adv7180 driver is badly coded.

The adv7180 driver implements this 'autodetect' mode which is enabled
when s_std is called with V4L2_STD_ALL. This is illegal according to the
spec. Digging deeper shows that only the sta2x11_vip.c driver uses this
'feature':

static int vidioc_s_std(struct file *file, void *priv, v4l2_std_id std)
{
        struct sta2x11_vip *vip = video_drvdata(file);
        v4l2_std_id oldstd = vip->std, newstd;
        int status;

        if (V4L2_STD_ALL == std) {
                v4l2_subdev_call(vip->decoder, video, s_std, std);
                ssleep(2);
                v4l2_subdev_call(vip->decoder, video, querystd, &newstd);
                v4l2_subdev_call(vip->decoder, video, g_input_status, &status);
                if (status & V4L2_IN_ST_NO_SIGNAL)
                        return -EIO;
                std = vip->std = newstd;
                if (oldstd != std) {
                        if (V4L2_STD_525_60 & std)
                                vip->format = formats_60[0];
                        else
                                vip->format = formats_50[0];
                }
                return 0;
        }

        if (oldstd != std) {
                if (V4L2_STD_525_60 & std)
                        vip->format = formats_60[0];
                else
                        vip->format = formats_50[0];
        }

        return v4l2_subdev_call(vip->decoder, video, s_std, std);
}

So it enables the autodetect, queries the standard and uses the resulting
standard.

It leaves the autodetect feature on as well which can be very dangerous
if it switches from NTSC to PAL since the buffer size increases in that
case, potentially leading to buffer overruns.

What you should do is to completely remove the autodetect feature from the
adv7180 driver, then change the code in sta2x11_vip.c to:

static int vidioc_s_std(struct file *file, void *priv, v4l2_std_id std)
{
        struct sta2x11_vip *vip = video_drvdata(file);
        int status;
	int err;

        if (V4L2_STD_ALL == std) {
		/*
		 * Note: this behavior is out-of-spec! It's kept to preserve
		 * backwards compatibility.
		 */
                v4l2_subdev_call(vip->decoder, video, querystd, &std);
                v4l2_subdev_call(vip->decoder, video, g_input_status, &status);
                if (status & V4L2_IN_ST_NO_SIGNAL)
                        return -EIO;
        }

        if (vip->std == std)
		return 0;

	err = v4l2_subdev_call(vip->decoder, video, s_std, std);
	if (err)
		return err;

	vip->std = std;
        if (V4L2_STD_525_60 & std)
                vip->format = formats_60[0];
        else
                vip->format = formats_50[0];
        return 0;
}

For this code:

Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>

Note: I haven't compiled this code, and I can't test it either (no hardware).

Regards,

	Hans

Re: [PATCH 1/3] adv7180: implement g_std() method

[Hans Verkuil]

On 09/04/2015 02:08 PM, Hans Verkuil wrote:
> On 09/04/2015 01:14 AM, Sergei Shtylyov wrote:
>> Commit cccb83f7a184 ([media] adv7180: add more subdev video ops) forgot to add
>> the g_std() video method. Its implementation seems identical to the querystd()
>> method,  so we  can just  point at adv7180_querystd()...
>>
>> Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>
>>
>> ---
>>  drivers/media/i2c/adv7180.c |    1 +
>>  1 file changed, 1 insertion(+)
>>
>> Index: media_tree/drivers/media/i2c/adv7180.c
>> ===================================================================
>> --- media_tree.orig/drivers/media/i2c/adv7180.c
>> +++ media_tree/drivers/media/i2c/adv7180.c
>> @@ -717,6 +717,7 @@ static int adv7180_g_mbus_config(struct
>>  }
>>  
>>  static const struct v4l2_subdev_video_ops adv7180_video_ops = {
>> +	.g_std = adv7180_querystd,
> 
> No, this isn't right. Not your fault, the adv7180 driver is badly coded.
> 
> The adv7180 driver implements this 'autodetect' mode which is enabled
> when s_std is called with V4L2_STD_ALL. This is illegal according to the
> spec. Digging deeper shows that only the sta2x11_vip.c driver uses this
> 'feature':
> 
> static int vidioc_s_std(struct file *file, void *priv, v4l2_std_id std)
> {
>         struct sta2x11_vip *vip = video_drvdata(file);
>         v4l2_std_id oldstd = vip->std, newstd;
>         int status;
> 
>         if (V4L2_STD_ALL == std) {
>                 v4l2_subdev_call(vip->decoder, video, s_std, std);
>                 ssleep(2);
>                 v4l2_subdev_call(vip->decoder, video, querystd, &newstd);
>                 v4l2_subdev_call(vip->decoder, video, g_input_status, &status);
>                 if (status & V4L2_IN_ST_NO_SIGNAL)
>                         return -EIO;
>                 std = vip->std = newstd;
>                 if (oldstd != std) {
>                         if (V4L2_STD_525_60 & std)
>                                 vip->format = formats_60[0];
>                         else
>                                 vip->format = formats_50[0];
>                 }
>                 return 0;
>         }
> 
>         if (oldstd != std) {
>                 if (V4L2_STD_525_60 & std)
>                         vip->format = formats_60[0];
>                 else
>                         vip->format = formats_50[0];
>         }
> 
>         return v4l2_subdev_call(vip->decoder, video, s_std, std);
> }
> 
> So it enables the autodetect, queries the standard and uses the resulting
> standard.
> 
> It leaves the autodetect feature on as well which can be very dangerous
> if it switches from NTSC to PAL since the buffer size increases in that
> case, potentially leading to buffer overruns.
> 
> What you should do is to completely remove the autodetect feature from the
> adv7180 driver, 

A quick follow-up to this: in adv7180_irq it does:

        if (isr3 & ADV7180_IRQ3_AD_CHANGE && state->autodetect)
                __adv7180_status(state, NULL, &state->curr_norm);

If we remove the autodetect 'feature', then this would be removed as well,
however, what would be better is to replace the call to __adv7180_status()
with generating a V4L2_EVENT_SOURCE_CHANGE event (v4l2_subdev_notify_event).

That way applications can be informed of source change events (assuming the
bridge driver will pass it through, but that's not the concern of the
adv7180 driver).

Regards,

	Hans