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@xxxxxxxxxxxxxxxxxx>
>>
>> ---
>> 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
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
