Re: [PATCH v6] V4L2: soc_camera: Renesas R-Car VIN driver

[Vladimir Barinov <vladimir.barinov@cogentembedded.com>]

Hi Guennadi,

Sorry for the response delay and thank you for new review.

Guennadi Liakhovetski wrote:
>> +	/* output format */
>> +	switch (icd->current_fmt->host_fmt->fourcc) {
>> +	case V4L2_PIX_FMT_NV16:
>> +		iowrite32(ALIGN(cam->width * cam->height, 0x80),
>> +			  priv->base + VNUVAOF_REG);
>> +		dmr = VNDMR_DTMD_YCSEP;
>> +		output_is_yuv = true;
>> +		break;
>> +	case V4L2_PIX_FMT_YUYV:
>> +		dmr = VNDMR_BPSM;
>> +		output_is_yuv = true;
>> +		break;
>> +	case V4L2_PIX_FMT_UYVY:
>> +		dmr = 0;
>> +		output_is_yuv = true;
>> +		break;
>> +	case V4L2_PIX_FMT_RGB555X:
>> +		dmr = VNDMR_DTMD_ARGB1555;
>> +		break;
>> +	case V4L2_PIX_FMT_RGB565:
>> +		dmr = 0;
>> +		break;
>> +	case V4L2_PIX_FMT_RGB32:
>> +		if (priv->chip = RCAR_H1 || priv->chip = RCAR_E1) {
>> +			dmr = VNDMR_EXRGB;
>> +			break;
>> +		}
>> +	default:
>> +		dev_warn(icd->parent, "Invalid fourcc format (0x%x)\n",
>> +			 icd->current_fmt->host_fmt->fourcc);
>>     
>
> I'll put a marker here for now: I don't understand the logic - either you 
> don't support this case, then you should either fail somehow or switch to 
> a supported case, or you do support it, then you don't need a warning
>   
Yes, the default case is not supported.
Don't you think the current logic should be replaced with BUG() callback?

> [snip]
>
>   
>> +static void rcar_vin_videobuf_queue(struct vb2_buffer *vb)
>> +{
>> +	struct soc_camera_device *icd = soc_camera_from_vb2q(vb->vb2_queue);
>> +	struct soc_camera_host *ici = to_soc_camera_host(icd->parent);
>> +	struct rcar_vin_priv *priv = ici->priv;
>> +	unsigned long size;
>> +	int bytes_per_line;
>> +
>> +	bytes_per_line = soc_mbus_bytes_per_line(icd->user_width,
>> +						 icd->current_fmt->host_fmt);
>> +	if (bytes_per_line < 0)
>> +		goto error;
>> +
>> +	size = icd->user_height * bytes_per_line;
>>     
>
> You haven't fixed this
>   
Sorry for the miss. Will replace with icd->sizeimage.

>> +static const struct soc_mbus_pixelfmt rcar_vin_formats[] = {
>> +	{
>> +		.fourcc			= V4L2_PIX_FMT_NV16,
>> +		.name			= "NV16",
>> +		.bits_per_sample	= 16,
>> +		.packing		= SOC_MBUS_PACKING_NONE,
>> +		.order			= SOC_MBUS_ORDER_LE,
>> +		.layout			= SOC_MBUS_LAYOUT_PACKED,
>>     
>
> This should be SOC_MBUS_LAYOUT_PLANAR_Y_C
>   
Shouldn't the ".packing"  be changed here to SOC_MBUS_PACKING_2X8_PADHI ?

>> +	if (!icd->host_priv) {
>> +		struct v4l2_mbus_framefmt mf;
>> +		struct v4l2_rect rect;
>> +		struct device *dev = icd->parent;
>> +		int shift;
>> +
>> +		ret = v4l2_subdev_call(sd, video, g_mbus_fmt, &mf);
>> +		if (ret < 0)
>> +			return ret;
>> +
>> +		/* Cache current client geometry */
>> +		ret = soc_camera_client_g_rect(sd, &rect);
>> +		if (ret < 0) {
>> +			/* Sensor driver doesn't support cropping */
>>     
>
> I don't think it's right. soc_camera_client_g_rect() should only return an 
> error, if the subdevice driver implements g_crop or cropcap and returns an 
> error from them. If those methods are just unimplemented, you get a 0 
> back. Do you see anything different?
>   
No.
In case the subdevice drivers does not implement cropping (i.e there is 
no both methods g_crop and cropcap) then the return value is -ENOIOCTLCMD.
Don't you suggest to continue for (ret = -ENOIOCTLCMD) and return for 
other (ret < 0) ?

>> +	switch (pixfmt) {
>> +	case V4L2_PIX_FMT_NV16:
>> +		can_scale = false;
>> +		break;
>> +	case V4L2_PIX_FMT_RGB32:
>> +		can_scale = priv->chip != RCAR_E1;
>> +		break;
>> +	default:
>> +		can_scale = true;
>>     
>
> You also get here in the pass-through mode, right? I don't think you can 
> scale then.
>   
Yes, thank you for pointing to this. I will add only supported formats 
for scaling capability.

>   
>> +		break;
>> +	}
>> +
>> +	dev_dbg(dev, "request camera output %ux%u\n", mf.width, mf.height);
>> +
>> +	ret = soc_camera_client_scale(icd, &cam->rect, &cam->subrect,
>> +				      &mf, &vin_sub_width, &vin_sub_height,
>> +				      can_scale, 12);
>> +
>> +	/* Done with the camera. Now see if we can improve the result */
>> +	dev_dbg(dev, "Camera %d fmt %ux%u, requested %ux%u\n",
>> +		ret, mf.width, mf.height, pix->width, pix->height);
>> +
>> +	if (ret < 0)
>> +		dev_dbg(dev, "Sensor doesn't support cropping\n");
>>     
>
> Are you sure this print is correct?
>   
Probably it should be the same like above for the case if subdevice 
driver does not support cropping (see soc_scale_crop.c -> client_s_fmt() ).

Regards,
Vladimir