diff mbox series

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

Message ID 20210412113457.328012-27-tomi.valkeinen@ideasonboard.com (mailing list archive)
State New, archived
Headers show
Series media: ti-vpe: cal: prepare for multistream support | expand

Commit Message

Tomi Valkeinen April 12, 2021, 11:34 a.m. UTC 
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(-)

Comments

Laurent Pinchart April 18, 2021, 1:21 p.m. UTC | #1 
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.

> +	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;
>
Tomi Valkeinen April 19, 2021, 1:01 p.m. UTC | #2 
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;
>>   
>
diff mbox series

Patch

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 (!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;
+		}
 	}
 
 	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;