Re: [PATCH 26/28] media: ti-vpe: cal: init ctx->v_fmt correctly in MC mode

[Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>]

On 18/04/2021 16:21, Laurent Pinchart wrote:
> Hi Tomi,
> 
> Thank you for the patch.
> 
> On Mon, Apr 12, 2021 at 02:34:55PM +0300, Tomi Valkeinen wrote:
>> CAL driver enumerates mbus codes in the connected subdev to create a
>> list of supported formats reported to userspace, and initializes
>> ctx->v_fmt and ctx->fmtinfo to one of those formats.
>>
>> This works fine for legacy mode, but is not correct for MC mode, and the
>> list is not even used in MC mode.
>>
>> Fix this by adding a new function, cal_ctx_v4l2_init_mc_format, which
>> only initializes ctx->v_fmt and ctx->fmtinfo to a default value.
>>
>> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
>> ---
>>   drivers/media/platform/ti-vpe/cal-video.c | 45 ++++++++++++++++++++---
>>   drivers/media/platform/ti-vpe/cal.h       |  2 +-
>>   2 files changed, 40 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/media/platform/ti-vpe/cal-video.c b/drivers/media/platform/ti-vpe/cal-video.c
>> index 0494cd04b9a5..95066cca128a 100644
>> --- a/drivers/media/platform/ti-vpe/cal-video.c
>> +++ b/drivers/media/platform/ti-vpe/cal-video.c
>> @@ -879,26 +879,59 @@ static int cal_ctx_v4l2_init_formats(struct cal_ctx *ctx)
>>   	return 0;
>>   }
>>   
>> +static int cal_ctx_v4l2_init_mc_format(struct cal_ctx *ctx)
>> +{
>> +	const struct cal_format_info *fmtinfo;
>> +	struct v4l2_pix_format *pix_fmt = &ctx->v_fmt.fmt.pix;
>> +
>> +	fmtinfo = cal_format_by_code(MEDIA_BUS_FMT_UYVY8_2X8);
> 
> 	if (WARN_ON(!fmt_info))
> 		return;
> 
> As this really can't happen.

Can't, but if it does and we don't handle the error, the driver will 
probably crash somewhere as ctx->fmtinfo is NULL. So maybe just drop the 
'if' totally, and we'll get that crash early, below via fmtinfo->fourcc.

> 
>> +	if (!fmtinfo)
>> +		return -EINVAL;
>> +
>> +	pix_fmt->width = 640;
>> +	pix_fmt->height = 480;
>> +	pix_fmt->field = V4L2_FIELD_NONE;
>> +	pix_fmt->colorspace = V4L2_COLORSPACE_SRGB;
>> +	pix_fmt->ycbcr_enc = V4L2_YCBCR_ENC_601;
>> +	pix_fmt->quantization = V4L2_QUANTIZATION_LIM_RANGE;
>> +	pix_fmt->xfer_func = V4L2_XFER_FUNC_SRGB;
>> +	pix_fmt->pixelformat = fmtinfo->fourcc;
>> +
>> +	ctx->v_fmt.type = V4L2_BUF_TYPE_VIDEO_CAPTURE;
>> +
>> +	/* Save current format */
>> +	cal_calc_format_size(ctx, fmtinfo, &ctx->v_fmt);
>> +	ctx->fmtinfo = fmtinfo;
>> +
>> +	return 0;
>> +}
>> +
>>   void cal_ctx_v4l2_register(struct cal_ctx *ctx)
>>   {
>>   	struct video_device *vfd = &ctx->vdev;
>>   	int ret;
>>   
>> -	ret = cal_ctx_v4l2_init_formats(ctx);
>> -	if (ret) {
>> -		ctx_err(ctx, "Failed to init formats: %d\n", ret);
>> -		return;
>> -	}
>> -
>>   	if (!cal_mc_api) {
>>   		struct v4l2_ctrl_handler *hdl = &ctx->ctrl_handler;
>>   
>> +		ret = cal_ctx_v4l2_init_formats(ctx);
>> +		if (ret) {
>> +			ctx_err(ctx, "Failed to init formats: %d\n", ret);
>> +			return;
>> +		}
>> +
>>   		ret = v4l2_ctrl_add_handler(hdl, ctx->phy->source->ctrl_handler,
>>   					    NULL, true);
>>   		if (ret < 0) {
>>   			ctx_err(ctx, "Failed to add source ctrl handler\n");
>>   			return;
>>   		}
>> +	} else {
>> +		ret = cal_ctx_v4l2_init_mc_format(ctx);
>> +		if (ret) {
>> +			ctx_err(ctx, "Failed to init format: %d\n", ret);
>> +			return;
>> +		}
> 
> And you can drop the error check here.
> 
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> 
>>   	}
>>   
>>   	ret = video_register_device(vfd, VFL_TYPE_VIDEO, cal_video_nr);
>> diff --git a/drivers/media/platform/ti-vpe/cal.h b/drivers/media/platform/ti-vpe/cal.h
>> index ad7d26c803eb..c941d2aec79f 100644
>> --- a/drivers/media/platform/ti-vpe/cal.h
>> +++ b/drivers/media/platform/ti-vpe/cal.h
>> @@ -213,7 +213,7 @@ struct cal_ctx {
>>   	/* Used to store current pixel format */
>>   	struct v4l2_format	v_fmt;
>>   
>> -	/* Current subdev enumerated format */
>> +	/* Current subdev enumerated format (legacy) */
>>   	const struct cal_format_info	**active_fmt;
>>   	unsigned int		num_active_fmt;
>>   
>