On 4/16/19 1:40 AM, Maciej S. Szmigiero wrote:
> On 12.04.2019 11:22, Hans Verkuil wrote:
>> On 3/30/19 12:51 AM, Maciej S. Szmigiero wrote:
>>> This patch adds support for analog part of Medion 95700 in the cxusb
>>> driver.
>>>
>>> What works:
>>> * Video capture at various sizes with sequential fields,
>>> * Input switching (TV Tuner, Composite, S-Video),
>>> * TV and radio tuning,
>>> * Video standard switching and auto detection,
>>> * Radio mode switching (stereo / mono),
>>> * Unplugging while capturing,
>>> * DVB / analog coexistence.
>>>
>>> What does not work yet:
>>> * Audio,
>>> * VBI,
>>> * Picture controls.
>>>
>>> Signed-off-by: Maciej S. Szmigiero <mail@xxxxxxxxxxxxxxxxxxxxx>
>>> ---
> (..)
>>> +static int cxusb_medion_try_s_fmt_vid_cap(struct file *file,
>>> + struct v4l2_format *f,
>>> + bool isset)
>>> +{
>>> + struct dvb_usb_device *dvbdev = video_drvdata(file);
>>> + struct cxusb_medion_dev *cxdev = dvbdev->priv;
>>> + struct v4l2_subdev_format subfmt;
>>> + int ret;
>>> +
>>> + if (isset && (vb2_start_streaming_called(&cxdev->videoqueue) ||
>>> + cxdev->stop_streaming))
>>
>> This should be:
>>
>> if (isset && vb2_is_busy(&cxdev->videoqueue))
>>
>> As long as buffers are allocated you can no longer change the format.
>>
>>> + return -EBUSY;
>>> +
>>> + memset(&subfmt, 0, sizeof(subfmt));
>>> + subfmt.which = isset ? V4L2_SUBDEV_FORMAT_ACTIVE :
>>> + V4L2_SUBDEV_FORMAT_TRY;
>>> + subfmt.format.width = f->fmt.pix.width & ~1;
>>> + subfmt.format.height = f->fmt.pix.height & ~1;
>>> + subfmt.format.code = MEDIA_BUS_FMT_FIXED;
>>> + subfmt.format.field = V4L2_FIELD_SEQ_TB;
>>
>> Why FIELD_SEQ_TB? If memory serves the cx25840 always uses FIELD_INTERLACED.
>
> It looks like all the existing cx25840 users use an intelligent bridge
> chip (or a MPEG encoder), which normalize the video data received to
> a nice V4L2_FIELD_INTERLACED.
>
> However, this device uses a "dumb" USB bridge chip which does not do any
> video processing (other than dropping parts of it if the host PC is not
> receiving it fast enough), so the output data matches what the cx25840
> chip produces - and it produces a sequential fields format.
Ah. But in that case it is SEQ_BT for NTSC and SEQ_TB for all other standards.
>
>>> + subfmt.format.colorspace = V4L2_COLORSPACE_SMPTE170M;
>>> +
>>> + ret = v4l2_subdev_call(cxdev->cx25840, pad, set_fmt, NULL, &subfmt);
>>> + if (ret != 0) {
>>> + if (ret != -ERANGE)
>>> + return ret;
>>> +
>>> + /* try some common formats */
>>> + subfmt.format.width = 720;
>>> + subfmt.format.height = 576;
>>> + ret = v4l2_subdev_call(cxdev->cx25840, pad, set_fmt, NULL,
>>> + &subfmt);
>>> + if (ret != 0) {
>>> + if (ret != -ERANGE)
>>> + return ret;
>>> +
>>> + subfmt.format.width = 640;
>>> + subfmt.format.height = 480;
>>> + ret = v4l2_subdev_call(cxdev->cx25840, pad, set_fmt,
>>> + NULL, &subfmt);
>>> + if (ret != 0)
>>> + return ret;
>>> + }
>>
>> Why these fallbacks? Is this really needed?
>
> V4L2 docs say that VIDIOC_{S,TRY}_FMT ioctls "should not return an error
> code unless the type field is invalid", that is, they should not return
> an error for invalid or unsupported image widths or heights.
> They should instead return something sensible for these image parameters.
>
> However, cx25840 driver set_fmt callback simply returns -ERANGE if it
> does not like the provided width or height.
I think the right approach here is to modify cx25840 so that it updates
the width/height to stay within the limits of the HW.
BTW, a quick scan shows that none of the drivers that call set_fmt actually
check the return value :-)
So changing the cx25840 behavior certainly won't make things worse for
existing drivers.
> In this case the code above simply tries first the bigger PAL capture
> resolution then the smaller NTSC one.
> Which one will be accepted by the cx25840 depends on the currently set
> broadcast standard and parameters of the last signal that was received,
> at least one of these resolutions seem to work even without any
> signal being received since the chip was powered up.
>
> This way the API guarantees should be kept by the driver.
This is basically a work-around for a cx25840 bug.
In any case, since the driver initializes to PAL the 720x576 resolution
should be fine.
BTW, I noticed that cxdev->norm is initialized to 0. Why isn't that
PAL?
>
>>> +int cxusb_medion_analog_init(struct dvb_usb_device *dvbdev)
>>> +{
>>> + struct cxusb_medion_dev *cxdev = dvbdev->priv;
>>> + u8 tuner_analog_msg_data[] = { 0x9c, 0x60, 0x85, 0x54 };
>>> + struct i2c_msg tuner_analog_msg = { .addr = 0x61, .flags = 0,
>>> + .buf = tuner_analog_msg_data,
>>> + .len =
>>> + sizeof(tuner_analog_msg_data) };
>>> + struct v4l2_subdev_format subfmt;
>>> + int ret;
>>> +
>>> + /* switch tuner to analog mode so IF demod will become accessible */
>>> + ret = i2c_transfer(&dvbdev->i2c_adap, &tuner_analog_msg, 1);
>>> + if (ret != 1)
>>> + dev_warn(&dvbdev->udev->dev,
>>> + "tuner analog switch failed (%d)\n", ret);
>>> +
>>> + /*
>>> + * cx25840 might have lost power during mode switching so we need
>>> + * to set it again
>>> + */
>>> + ret = v4l2_subdev_call(cxdev->cx25840, core, reset, 0);
>>> + if (ret != 0)
>>> + dev_warn(&dvbdev->udev->dev,
>>> + "cx25840 reset failed (%d)\n", ret);
>>> +
>>> + ret = v4l2_subdev_call(cxdev->cx25840, video, s_routing,
>>> + CX25840_COMPOSITE1, 0, 0);
>>> + if (ret != 0)
>>> + dev_warn(&dvbdev->udev->dev,
>>> + "cx25840 initial input setting failed (%d)\n", ret);
>>> +
>>> + /* composite */
>>> + cxdev->input = 1;
>>> + cxdev->norm = 0;
>>> +
>>> + /* TODO: setup audio samples insertion */
>>> +
>>> + ret = v4l2_subdev_call(cxdev->cx25840, core, s_io_pin_config,
>>> + sizeof(cxusub_medion_pin_config) /
>>> + sizeof(cxusub_medion_pin_config[0]),
>>> + cxusub_medion_pin_config);
>>> + if (ret != 0)
>>> + dev_warn(&dvbdev->udev->dev,
>>> + "cx25840 pin config failed (%d)\n", ret);
>>> +
>>> + /* make sure that we aren't in radio mode */
>>> + v4l2_subdev_call(cxdev->tda9887, video, s_std, V4L2_STD_PAL);
>>> + v4l2_subdev_call(cxdev->tuner, video, s_std, V4L2_STD_PAL);
>>> + v4l2_subdev_call(cxdev->cx25840, video, s_std, cxdev->norm);
>>> +
>>> + memset(&subfmt, 0, sizeof(subfmt));
>>> + subfmt.which = V4L2_SUBDEV_FORMAT_ACTIVE;
>>> + subfmt.format.width = cxdev->width;
>>> + subfmt.format.height = cxdev->height;
>>> + subfmt.format.code = MEDIA_BUS_FMT_FIXED;
>>> + subfmt.format.field = V4L2_FIELD_SEQ_TB;
>>> + subfmt.format.colorspace = V4L2_COLORSPACE_SMPTE170M;
>>> +
>>> + ret = v4l2_subdev_call(cxdev->cx25840, pad, set_fmt, NULL, &subfmt);
>>> + if (ret != 0) {
>>> + subfmt.format.width = 640;
>>> + subfmt.format.height = 480;
>>
>> Why the fallback to 640x480?
>
> This resolution seems to work even without any signal being received,
> and it is a sensible NTSC-like resolution so it makes a good fallback
> if restoring the previously used resolution has failed.
But it is really a work-around for a cx25840 bug. Just fix set_fmt
and you should not need this anymore.
Regards.
Hans
>
>>> + ret = v4l2_subdev_call(cxdev->cx25840, pad, set_fmt, NULL,
>>> + &subfmt);
>>> + if (ret != 0)
>>> + dev_warn(&dvbdev->udev->dev,
>>> + "cx25840 format set failed (%d)\n", ret);
>>> + }
>>> +
>>> + if (ret == 0) {
>>> + cxdev->width = subfmt.format.width;
>>> + cxdev->height = subfmt.format.height;
>>> + }
>>> +
>>> + return 0;
>>> +}
>>> +
>
>>
>> Regards,
>>
>> Hans
>>
>
> Thanks and best regards,
> Maciej
>
