diff mbox

rcar_vin: propagate querystd() error upstream

Message ID 1650569.JYNQd5Bi8T@wasted.cogentembedded.com (mailing list archive)
State Superseded
Delegated to: Geert Uytterhoeven
Headers show

Commit Message

Sergei Shtylyov Aug. 19, 2015, 9:02 p.m. UTC 
rcar_vin_set_fmt() defaults to  PAL when the subdevice's querystd() method call
fails (e.g. due to I2C error).  This doesn't work very well when a camera being
used  outputs NTSC which has different order of fields and resolution.  Let  us
stop  pretending and return the actual error (which would prevent video capture
on at least Renesas Henninger/Porter board where I2C seems particularly buggy).

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

---
The patch is against the 'media_tree.git' repo's 'fixes' branch.

 drivers/media/platform/soc_camera/rcar_vin.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)


--
To unsubscribe from this list: send the line "unsubscribe linux-sh" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Comments

Laurent Pinchart Aug. 20, 2015, 9:51 p.m. UTC | #1 
Hi Sergei,

Thank you for the patch.

On Thursday 20 August 2015 00:02:17 Sergei Shtylyov wrote:
> rcar_vin_set_fmt() defaults to  PAL when the subdevice's querystd() method
> call fails (e.g. due to I2C error).  This doesn't work very well when a
> camera being used  outputs NTSC which has different order of fields and
> resolution.  Let  us stop  pretending and return the actual error (which
> would prevent video capture on at least Renesas Henninger/Porter board
> where I2C seems particularly buggy).
> 
> Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>
> 
> ---
> The patch is against the 'media_tree.git' repo's 'fixes' branch.
> 
>  drivers/media/platform/soc_camera/rcar_vin.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> 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
> @@ -1592,7 +1592,7 @@ static int rcar_vin_set_fmt(struct soc_c
>  		/* Query for standard if not explicitly mentioned _TB/_BT */
>  		ret = v4l2_subdev_call(sd, video, querystd, &std);
>  		if (ret < 0)
> -			std = V4L2_STD_625_50;
> +			return ret;

What if the subdev doesn't implement querystd ? That's the case of camera 
sensors for instance. In that case we should default to V4L2_FIELD_NONE.

>  		field = std & V4L2_STD_625_50 ? V4L2_FIELD_INTERLACED_TB :
>  						V4L2_FIELD_INTERLACED_BT;
Sergei Shtylyov Aug. 24, 2015, 9:58 p.m. UTC | #2 
hello.

On 08/21/2015 12:51 AM, Laurent Pinchart wrote:

>> rcar_vin_set_fmt() defaults to  PAL when the subdevice's querystd() method
>> call fails (e.g. due to I2C error).  This doesn't work very well when a
>> camera being used  outputs NTSC which has different order of fields and
>> resolution.  Let  us stop  pretending and return the actual error (which
>> would prevent video capture on at least Renesas Henninger/Porter board
>> where I2C seems particularly buggy).
>>
>> Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>
>>
>> ---
>> The patch is against the 'media_tree.git' repo's 'fixes' branch.
>>
>>   drivers/media/platform/soc_camera/rcar_vin.c |    2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> 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
>> @@ -1592,7 +1592,7 @@ static int rcar_vin_set_fmt(struct soc_c
>>   		/* Query for standard if not explicitly mentioned _TB/_BT */
>>   		ret = v4l2_subdev_call(sd, video, querystd, &std);
>>   		if (ret < 0)
>> -			std = V4L2_STD_625_50;
>> +			return ret;
>
> What if the subdev doesn't implement querystd ? That's the case of camera
> sensors for instance.

    Indeed.

> In that case we should default to V4L2_FIELD_NONE.

    Hmm, even if the set_fmt() method is called with V4L2_FIELD_INTERLACED 
already, like in this case?

>>   		field = std & V4L2_STD_625_50 ? V4L2_FIELD_INTERLACED_TB :
>>   						V4L2_FIELD_INTERLACED_BT;

MBR, Sergei

--
To unsubscribe from this list: send the line "unsubscribe linux-sh" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Hans Verkuil Aug. 25, 2015, 2:25 p.m. UTC | #3 
On 08/19/15 23:02, Sergei Shtylyov wrote:
> rcar_vin_set_fmt() defaults to  PAL when the subdevice's querystd() method call
> fails (e.g. due to I2C error).  This doesn't work very well when a camera being
> used  outputs NTSC which has different order of fields and resolution.  Let  us
> stop  pretending and return the actual error (which would prevent video capture
> on at least Renesas Henninger/Porter board where I2C seems particularly buggy).
> 
> Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>
> 
> ---
> The patch is against the 'media_tree.git' repo's 'fixes' branch.
> 
>  drivers/media/platform/soc_camera/rcar_vin.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> 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
> @@ -1592,7 +1592,7 @@ static int rcar_vin_set_fmt(struct soc_c
>  		/* Query for standard if not explicitly mentioned _TB/_BT */
>  		ret = v4l2_subdev_call(sd, video, querystd, &std);

Ouch, this should never be done like this.

Instead the decision should be made using the last set std, never by querying.
So querystd should be replaced by g_std in the v4l2_subdev_call above.

The only place querystd can be called is in the QUERYSTD ioctl, all other
ioctls should use the last set standard.

Regards,

	Hans

>  		if (ret < 0)
> -			std = V4L2_STD_625_50;
> +			return ret;
>  
>  		field = std & V4L2_STD_625_50 ? V4L2_FIELD_INTERLACED_TB :
>  						V4L2_FIELD_INTERLACED_BT;
> 
> --
> 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
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-sh" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Laurent Pinchart Aug. 25, 2015, 7:42 p.m. UTC | #4 
Hi Sergei,

On Tuesday 25 August 2015 00:58:07 Sergei Shtylyov wrote:
> On 08/21/2015 12:51 AM, Laurent Pinchart wrote:
> >> rcar_vin_set_fmt() defaults to  PAL when the subdevice's querystd()
> >> method call fails (e.g. due to I2C error). This doesn't work very well
> >> when a camera being used  outputs NTSC which has different order of
> >> fields and resolution. Let us stop pretending and return the actual
> >> error (which would prevent video capture on at least Renesas
> >> Henninger/Porter board where I2C seems particularly buggy).
> >> 
> >> Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>
> >> 
> >> ---
> >> The patch is against the 'media_tree.git' repo's 'fixes' branch.
> >> 
> >>   drivers/media/platform/soc_camera/rcar_vin.c |    2 +-
> >>   1 file changed, 1 insertion(+), 1 deletion(-)
> >> 
> >> 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
> >> @@ -1592,7 +1592,7 @@ static int rcar_vin_set_fmt(struct soc_c
> >> 
> >>   		/* Query for standard if not explicitly mentioned _TB/_BT */
> >>   		ret = v4l2_subdev_call(sd, video, querystd, &std);
> >>   		if (ret < 0)
> >> -			std = V4L2_STD_625_50;
> >> +			return ret;
> > 
> > What if the subdev doesn't implement querystd ? That's the case of camera
> > sensors for instance.
> 
> Indeed.
> 
> > In that case we should default to V4L2_FIELD_NONE.
> 
> Hmm, even if the set_fmt() method is called with V4L2_FIELD_INTERLACED
> already, like in this case?

Yes. If the device doesn't support interlacing then the field passed to 
set_fmt() should be set to V4L2_FIELD_NONE. The V4L2 API requires drivers to 
fix unsupported parameters passed to the VIDIOC_S_FMT ioctl instead of 
returning an error like commonly done in such situation.

> >>   		field = std & V4L2_STD_625_50 ? V4L2_FIELD_INTERLACED_TB :
> >>   						V4L2_FIELD_INTERLACED_BT;
Sergei Shtylyov Aug. 31, 2015, 10:13 p.m. UTC | #5 
Hello.

On 08/25/2015 05:25 PM, Hans Verkuil wrote:

>> rcar_vin_set_fmt() defaults to  PAL when the subdevice's querystd() method call
>> fails (e.g. due to I2C error).  This doesn't work very well when a camera being
>> used  outputs NTSC which has different order of fields and resolution.  Let  us
>> stop  pretending and return the actual error (which would prevent video capture
>> on at least Renesas Henninger/Porter board where I2C seems particularly buggy).
>>
>> Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>
>>
>> ---
>> The patch is against the 'media_tree.git' repo's 'fixes' branch.
>>
>>   drivers/media/platform/soc_camera/rcar_vin.c |    2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> 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
>> @@ -1592,7 +1592,7 @@ static int rcar_vin_set_fmt(struct soc_c
>>   		/* Query for standard if not explicitly mentioned _TB/_BT */
>>   		ret = v4l2_subdev_call(sd, video, querystd, &std);

> Ouch, this should never be done like this.

    Too late. :-)

> Instead the decision should be made using the last set std, never by querying.
> So querystd should be replaced by g_std in the v4l2_subdev_call above.

    Hm, then this code will stop working, as adv7180.c and ml86v7667.c we use 
don't support the g_std() method yet...

> The only place querystd can be called is in the QUERYSTD ioctl, all other
> ioctls should use the last set standard.

    OK, I'll have to fix all the drivers involved...

> Regards,
> 	Hans

MBR, Sergei

--
To unsubscribe from this list: send the line "unsubscribe linux-sh" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

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
@@ -1592,7 +1592,7 @@  static int rcar_vin_set_fmt(struct soc_c
 		/* Query for standard if not explicitly mentioned _TB/_BT */
 		ret = v4l2_subdev_call(sd, video, querystd, &std);
 		if (ret < 0)
-			std = V4L2_STD_625_50;
+			return ret;
 
 		field = std & V4L2_STD_625_50 ? V4L2_FIELD_INTERLACED_TB :
 						V4L2_FIELD_INTERLACED_BT;