Re: [PATCH v9 3/4] [media] cxusb: add analog mode support for Medion MD95700

[Maciej S. Szmigiero]

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.

>> +	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.
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.

>> +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.

>> +		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