[PATCH] media: stm32: dcmi: Switch to __v4l2_subdev_state_alloc()

[PATCH] media: stm32: dcmi: Switch to __v4l2_subdev_state_alloc()

[Marek Vasut]

Any local subdev state should be allocated and free'd using
__v4l2_subdev_state_alloc()/__v4l2_subdev_state_free(), which
takes care of calling .init_cfg() subdev op. Without this,
subdev internal state might be uninitialized by the time
any other subdev op is called.

Signed-off-by: Marek Vasut <marex@denx.de>
Cc: Alain Volmat <alain.volmat@foss.st.com>
Cc: Alexandre Torgue <alexandre.torgue@foss.st.com>
Cc: Amelie DELAUNAY <amelie.delaunay@foss.st.com>
Cc: Hugues FRUCHET <hugues.fruchet@foss.st.com>
Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Cc: Philippe CORNU <philippe.cornu@foss.st.com>
Cc: linux-stm32@st-md-mailman.stormreply.com
Cc: linux-arm-kernel@lists.infradead.org
---
 drivers/media/platform/st/stm32/stm32-dcmi.c | 51 +++++++++++---------
 1 file changed, 29 insertions(+), 22 deletions(-)

diff --git a/drivers/media/platform/st/stm32/stm32-dcmi.c b/drivers/media/platform/st/stm32/stm32-dcmi.c
index ec54b5ecfd544..ef795c12fb233 100644
--- a/drivers/media/platform/st/stm32/stm32-dcmi.c
+++ b/drivers/media/platform/st/stm32/stm32-dcmi.c
@@ -996,22 +996,26 @@ static int dcmi_try_fmt(struct stm32_dcmi *dcmi, struct v4l2_format *f,
 			struct dcmi_framesize *sd_framesize)
 {
 	const struct dcmi_format *sd_fmt;
+	static struct lock_class_key key;
 	struct dcmi_framesize sd_fsize;
 	struct v4l2_pix_format *pix = &f->fmt.pix;
-	struct v4l2_subdev_pad_config pad_cfg;
-	struct v4l2_subdev_state pad_state = {
-		.pads = &pad_cfg
-		};
+	struct v4l2_subdev_state *sd_state;
 	struct v4l2_subdev_format format = {
 		.which = V4L2_SUBDEV_FORMAT_TRY,
 	};
 	bool do_crop;
 	int ret;
 
+	sd_state = __v4l2_subdev_state_alloc(dcmi->source, "dcmi:state->lock", &key);
+	if (IS_ERR(sd_state))
+		return PTR_ERR(sd_state);
+
 	sd_fmt = find_format_by_fourcc(dcmi, pix->pixelformat);
 	if (!sd_fmt) {
-		if (!dcmi->num_of_sd_formats)
-			return -ENODATA;
+		if (!dcmi->num_of_sd_formats) {
+			ret = -ENODATA;
+			goto done;
+		}
 
 		sd_fmt = dcmi->sd_formats[dcmi->num_of_sd_formats - 1];
 		pix->pixelformat = sd_fmt->fourcc;
@@ -1036,10 +1040,9 @@ static int dcmi_try_fmt(struct stm32_dcmi *dcmi, struct v4l2_format *f,
 	}
 
 	v4l2_fill_mbus_format(&format.format, pix, sd_fmt->mbus_code);
-	ret = v4l2_subdev_call(dcmi->source, pad, set_fmt,
-			       &pad_state, &format);
+	ret = v4l2_subdev_call(dcmi->source, pad, set_fmt, sd_state, &format);
 	if (ret < 0)
-		return ret;
+		goto done;
 
 	/* Update pix regarding to what sensor can do */
 	v4l2_fill_pix_format(pix, &format.format);
@@ -1079,7 +1082,9 @@ static int dcmi_try_fmt(struct stm32_dcmi *dcmi, struct v4l2_format *f,
 	if (sd_framesize)
 		*sd_framesize = sd_fsize;
 
-	return 0;
+done:
+	__v4l2_subdev_state_free(sd_state);
+	return ret;
 }
 
 static int dcmi_set_fmt(struct stm32_dcmi *dcmi, struct v4l2_format *f)
@@ -1183,31 +1188,33 @@ static int dcmi_set_sensor_format(struct stm32_dcmi *dcmi,
 				  struct v4l2_pix_format *pix)
 {
 	const struct dcmi_format *sd_fmt;
+	static struct lock_class_key key;
+	struct v4l2_subdev_state *sd_state;
 	struct v4l2_subdev_format format = {
 		.which = V4L2_SUBDEV_FORMAT_TRY,
 	};
-	struct v4l2_subdev_pad_config pad_cfg;
-	struct v4l2_subdev_state pad_state = {
-		.pads = &pad_cfg
-		};
 	int ret;
 
+	sd_state = __v4l2_subdev_state_alloc(dcmi->source, "dcmi:state->lock", &key);
+	if (IS_ERR(sd_state))
+		return PTR_ERR(sd_state);
+
 	sd_fmt = find_format_by_fourcc(dcmi, pix->pixelformat);
 	if (!sd_fmt) {
-		if (!dcmi->num_of_sd_formats)
-			return -ENODATA;
+		if (!dcmi->num_of_sd_formats) {
+			ret = -ENODATA;
+			goto done;
+		}
 
 		sd_fmt = dcmi->sd_formats[dcmi->num_of_sd_formats - 1];
 		pix->pixelformat = sd_fmt->fourcc;
 	}
 
 	v4l2_fill_mbus_format(&format.format, pix, sd_fmt->mbus_code);
-	ret = v4l2_subdev_call(dcmi->source, pad, set_fmt,
-			       &pad_state, &format);
-	if (ret < 0)
-		return ret;
-
-	return 0;
+	ret = v4l2_subdev_call(dcmi->source, pad, set_fmt, sd_state, &format);
+done:
+	__v4l2_subdev_state_free(sd_state);
+	return ret;
 }
 
 static int dcmi_get_sensor_bounds(struct stm32_dcmi *dcmi,
-- 
2.35.1

[PATCH] media: stm32: dcmi: Switch to __v4l2_subdev_state_alloc()

[Marek Vasut]

Any local subdev state should be allocated and free'd using
__v4l2_subdev_state_alloc()/__v4l2_subdev_state_free(), which
takes care of calling .init_cfg() subdev op. Without this,
subdev internal state might be uninitialized by the time
any other subdev op is called.

Signed-off-by: Marek Vasut <marex@denx.de>
Cc: Alain Volmat <alain.volmat@foss.st.com>
Cc: Alexandre Torgue <alexandre.torgue@foss.st.com>
Cc: Amelie DELAUNAY <amelie.delaunay@foss.st.com>
Cc: Hugues FRUCHET <hugues.fruchet@foss.st.com>
Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Cc: Philippe CORNU <philippe.cornu@foss.st.com>
Cc: linux-stm32@st-md-mailman.stormreply.com
Cc: linux-arm-kernel@lists.infradead.org
---
 drivers/media/platform/st/stm32/stm32-dcmi.c | 51 +++++++++++---------
 1 file changed, 29 insertions(+), 22 deletions(-)

diff --git a/drivers/media/platform/st/stm32/stm32-dcmi.c b/drivers/media/platform/st/stm32/stm32-dcmi.c
index ec54b5ecfd544..ef795c12fb233 100644
--- a/drivers/media/platform/st/stm32/stm32-dcmi.c
+++ b/drivers/media/platform/st/stm32/stm32-dcmi.c
@@ -996,22 +996,26 @@ static int dcmi_try_fmt(struct stm32_dcmi *dcmi, struct v4l2_format *f,
 			struct dcmi_framesize *sd_framesize)
 {
 	const struct dcmi_format *sd_fmt;
+	static struct lock_class_key key;
 	struct dcmi_framesize sd_fsize;
 	struct v4l2_pix_format *pix = &f->fmt.pix;
-	struct v4l2_subdev_pad_config pad_cfg;
-	struct v4l2_subdev_state pad_state = {
-		.pads = &pad_cfg
-		};
+	struct v4l2_subdev_state *sd_state;
 	struct v4l2_subdev_format format = {
 		.which = V4L2_SUBDEV_FORMAT_TRY,
 	};
 	bool do_crop;
 	int ret;
 
+	sd_state = __v4l2_subdev_state_alloc(dcmi->source, "dcmi:state->lock", &key);
+	if (IS_ERR(sd_state))
+		return PTR_ERR(sd_state);
+
 	sd_fmt = find_format_by_fourcc(dcmi, pix->pixelformat);
 	if (!sd_fmt) {
-		if (!dcmi->num_of_sd_formats)
-			return -ENODATA;
+		if (!dcmi->num_of_sd_formats) {
+			ret = -ENODATA;
+			goto done;
+		}
 
 		sd_fmt = dcmi->sd_formats[dcmi->num_of_sd_formats - 1];
 		pix->pixelformat = sd_fmt->fourcc;
@@ -1036,10 +1040,9 @@ static int dcmi_try_fmt(struct stm32_dcmi *dcmi, struct v4l2_format *f,
 	}
 
 	v4l2_fill_mbus_format(&format.format, pix, sd_fmt->mbus_code);
-	ret = v4l2_subdev_call(dcmi->source, pad, set_fmt,
-			       &pad_state, &format);
+	ret = v4l2_subdev_call(dcmi->source, pad, set_fmt, sd_state, &format);
 	if (ret < 0)
-		return ret;
+		goto done;
 
 	/* Update pix regarding to what sensor can do */
 	v4l2_fill_pix_format(pix, &format.format);
@@ -1079,7 +1082,9 @@ static int dcmi_try_fmt(struct stm32_dcmi *dcmi, struct v4l2_format *f,
 	if (sd_framesize)
 		*sd_framesize = sd_fsize;
 
-	return 0;
+done:
+	__v4l2_subdev_state_free(sd_state);
+	return ret;
 }
 
 static int dcmi_set_fmt(struct stm32_dcmi *dcmi, struct v4l2_format *f)
@@ -1183,31 +1188,33 @@ static int dcmi_set_sensor_format(struct stm32_dcmi *dcmi,
 				  struct v4l2_pix_format *pix)
 {
 	const struct dcmi_format *sd_fmt;
+	static struct lock_class_key key;
+	struct v4l2_subdev_state *sd_state;
 	struct v4l2_subdev_format format = {
 		.which = V4L2_SUBDEV_FORMAT_TRY,
 	};
-	struct v4l2_subdev_pad_config pad_cfg;
-	struct v4l2_subdev_state pad_state = {
-		.pads = &pad_cfg
-		};
 	int ret;
 
+	sd_state = __v4l2_subdev_state_alloc(dcmi->source, "dcmi:state->lock", &key);
+	if (IS_ERR(sd_state))
+		return PTR_ERR(sd_state);
+
 	sd_fmt = find_format_by_fourcc(dcmi, pix->pixelformat);
 	if (!sd_fmt) {
-		if (!dcmi->num_of_sd_formats)
-			return -ENODATA;
+		if (!dcmi->num_of_sd_formats) {
+			ret = -ENODATA;
+			goto done;
+		}
 
 		sd_fmt = dcmi->sd_formats[dcmi->num_of_sd_formats - 1];
 		pix->pixelformat = sd_fmt->fourcc;
 	}
 
 	v4l2_fill_mbus_format(&format.format, pix, sd_fmt->mbus_code);
-	ret = v4l2_subdev_call(dcmi->source, pad, set_fmt,
-			       &pad_state, &format);
-	if (ret < 0)
-		return ret;
-
-	return 0;
+	ret = v4l2_subdev_call(dcmi->source, pad, set_fmt, sd_state, &format);
+done:
+	__v4l2_subdev_state_free(sd_state);
+	return ret;
 }
 
 static int dcmi_get_sensor_bounds(struct stm32_dcmi *dcmi,
-- 
2.35.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] media: stm32: dcmi: Switch to __v4l2_subdev_state_alloc()

[Laurent Pinchart]

Hi Marek,

CC'ing Tomi to get his opinion.

On Sun, Jun 19, 2022 at 12:24:42AM +0200, Marek Vasut wrote:
> Any local subdev state should be allocated and free'd using
> __v4l2_subdev_state_alloc()/__v4l2_subdev_state_free(), which
> takes care of calling .init_cfg() subdev op. Without this,
> subdev internal state might be uninitialized by the time
> any other subdev op is called.
> 
> Signed-off-by: Marek Vasut <marex@denx.de>
> Cc: Alain Volmat <alain.volmat@foss.st.com>
> Cc: Alexandre Torgue <alexandre.torgue@foss.st.com>
> Cc: Amelie DELAUNAY <amelie.delaunay@foss.st.com>
> Cc: Hugues FRUCHET <hugues.fruchet@foss.st.com>
> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Cc: Philippe CORNU <philippe.cornu@foss.st.com>
> Cc: linux-stm32@st-md-mailman.stormreply.com
> Cc: linux-arm-kernel@lists.infradead.org
> ---
>  drivers/media/platform/st/stm32/stm32-dcmi.c | 51 +++++++++++---------
>  1 file changed, 29 insertions(+), 22 deletions(-)
> 
> diff --git a/drivers/media/platform/st/stm32/stm32-dcmi.c b/drivers/media/platform/st/stm32/stm32-dcmi.c
> index ec54b5ecfd544..ef795c12fb233 100644
> --- a/drivers/media/platform/st/stm32/stm32-dcmi.c
> +++ b/drivers/media/platform/st/stm32/stm32-dcmi.c
> @@ -996,22 +996,26 @@ static int dcmi_try_fmt(struct stm32_dcmi *dcmi, struct v4l2_format *f,
>  			struct dcmi_framesize *sd_framesize)
>  {
>  	const struct dcmi_format *sd_fmt;
> +	static struct lock_class_key key;
>  	struct dcmi_framesize sd_fsize;
>  	struct v4l2_pix_format *pix = &f->fmt.pix;
> -	struct v4l2_subdev_pad_config pad_cfg;
> -	struct v4l2_subdev_state pad_state = {
> -		.pads = &pad_cfg
> -		};
> +	struct v4l2_subdev_state *sd_state;
>  	struct v4l2_subdev_format format = {
>  		.which = V4L2_SUBDEV_FORMAT_TRY,
>  	};
>  	bool do_crop;
>  	int ret;
>  
> +	sd_state = __v4l2_subdev_state_alloc(dcmi->source, "dcmi:state->lock", &key);
> +	if (IS_ERR(sd_state))
> +		return PTR_ERR(sd_state);
> +
>  	sd_fmt = find_format_by_fourcc(dcmi, pix->pixelformat);
>  	if (!sd_fmt) {
> -		if (!dcmi->num_of_sd_formats)
> -			return -ENODATA;
> +		if (!dcmi->num_of_sd_formats) {
> +			ret = -ENODATA;
> +			goto done;
> +		}
>  
>  		sd_fmt = dcmi->sd_formats[dcmi->num_of_sd_formats - 1];
>  		pix->pixelformat = sd_fmt->fourcc;
> @@ -1036,10 +1040,9 @@ static int dcmi_try_fmt(struct stm32_dcmi *dcmi, struct v4l2_format *f,
>  	}
>  
>  	v4l2_fill_mbus_format(&format.format, pix, sd_fmt->mbus_code);
> -	ret = v4l2_subdev_call(dcmi->source, pad, set_fmt,
> -			       &pad_state, &format);
> +	ret = v4l2_subdev_call(dcmi->source, pad, set_fmt, sd_state, &format);
>  	if (ret < 0)
> -		return ret;
> +		goto done;
>  
>  	/* Update pix regarding to what sensor can do */
>  	v4l2_fill_pix_format(pix, &format.format);
> @@ -1079,7 +1082,9 @@ static int dcmi_try_fmt(struct stm32_dcmi *dcmi, struct v4l2_format *f,
>  	if (sd_framesize)
>  		*sd_framesize = sd_fsize;
>  
> -	return 0;
> +done:
> +	__v4l2_subdev_state_free(sd_state);
> +	return ret;
>  }
>  
>  static int dcmi_set_fmt(struct stm32_dcmi *dcmi, struct v4l2_format *f)
> @@ -1183,31 +1188,33 @@ static int dcmi_set_sensor_format(struct stm32_dcmi *dcmi,
>  				  struct v4l2_pix_format *pix)
>  {
>  	const struct dcmi_format *sd_fmt;
> +	static struct lock_class_key key;
> +	struct v4l2_subdev_state *sd_state;
>  	struct v4l2_subdev_format format = {
>  		.which = V4L2_SUBDEV_FORMAT_TRY,
>  	};
> -	struct v4l2_subdev_pad_config pad_cfg;
> -	struct v4l2_subdev_state pad_state = {
> -		.pads = &pad_cfg
> -		};
>  	int ret;
>  
> +	sd_state = __v4l2_subdev_state_alloc(dcmi->source, "dcmi:state->lock", &key);
> +	if (IS_ERR(sd_state))
> +		return PTR_ERR(sd_state);
> +
>  	sd_fmt = find_format_by_fourcc(dcmi, pix->pixelformat);
>  	if (!sd_fmt) {
> -		if (!dcmi->num_of_sd_formats)
> -			return -ENODATA;
> +		if (!dcmi->num_of_sd_formats) {
> +			ret = -ENODATA;
> +			goto done;
> +		}
>  
>  		sd_fmt = dcmi->sd_formats[dcmi->num_of_sd_formats - 1];
>  		pix->pixelformat = sd_fmt->fourcc;
>  	}
>  
>  	v4l2_fill_mbus_format(&format.format, pix, sd_fmt->mbus_code);
> -	ret = v4l2_subdev_call(dcmi->source, pad, set_fmt,
> -			       &pad_state, &format);
> -	if (ret < 0)
> -		return ret;
> -
> -	return 0;
> +	ret = v4l2_subdev_call(dcmi->source, pad, set_fmt, sd_state, &format);
> +done:
> +	__v4l2_subdev_state_free(sd_state);
> +	return ret;
>  }
>  
>  static int dcmi_get_sensor_bounds(struct stm32_dcmi *dcmi,

-- 
Regards,

Laurent Pinchart

Re: [PATCH] media: stm32: dcmi: Switch to __v4l2_subdev_state_alloc()

[Laurent Pinchart]

Hi Marek,

CC'ing Tomi to get his opinion.

On Sun, Jun 19, 2022 at 12:24:42AM +0200, Marek Vasut wrote:
> Any local subdev state should be allocated and free'd using
> __v4l2_subdev_state_alloc()/__v4l2_subdev_state_free(), which
> takes care of calling .init_cfg() subdev op. Without this,
> subdev internal state might be uninitialized by the time
> any other subdev op is called.
> 
> Signed-off-by: Marek Vasut <marex@denx.de>
> Cc: Alain Volmat <alain.volmat@foss.st.com>
> Cc: Alexandre Torgue <alexandre.torgue@foss.st.com>
> Cc: Amelie DELAUNAY <amelie.delaunay@foss.st.com>
> Cc: Hugues FRUCHET <hugues.fruchet@foss.st.com>
> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Cc: Philippe CORNU <philippe.cornu@foss.st.com>
> Cc: linux-stm32@st-md-mailman.stormreply.com
> Cc: linux-arm-kernel@lists.infradead.org
> ---
>  drivers/media/platform/st/stm32/stm32-dcmi.c | 51 +++++++++++---------
>  1 file changed, 29 insertions(+), 22 deletions(-)
> 
> diff --git a/drivers/media/platform/st/stm32/stm32-dcmi.c b/drivers/media/platform/st/stm32/stm32-dcmi.c
> index ec54b5ecfd544..ef795c12fb233 100644
> --- a/drivers/media/platform/st/stm32/stm32-dcmi.c
> +++ b/drivers/media/platform/st/stm32/stm32-dcmi.c
> @@ -996,22 +996,26 @@ static int dcmi_try_fmt(struct stm32_dcmi *dcmi, struct v4l2_format *f,
>  			struct dcmi_framesize *sd_framesize)
>  {
>  	const struct dcmi_format *sd_fmt;
> +	static struct lock_class_key key;
>  	struct dcmi_framesize sd_fsize;
>  	struct v4l2_pix_format *pix = &f->fmt.pix;
> -	struct v4l2_subdev_pad_config pad_cfg;
> -	struct v4l2_subdev_state pad_state = {
> -		.pads = &pad_cfg
> -		};
> +	struct v4l2_subdev_state *sd_state;
>  	struct v4l2_subdev_format format = {
>  		.which = V4L2_SUBDEV_FORMAT_TRY,
>  	};
>  	bool do_crop;
>  	int ret;
>  
> +	sd_state = __v4l2_subdev_state_alloc(dcmi->source, "dcmi:state->lock", &key);
> +	if (IS_ERR(sd_state))
> +		return PTR_ERR(sd_state);
> +
>  	sd_fmt = find_format_by_fourcc(dcmi, pix->pixelformat);
>  	if (!sd_fmt) {
> -		if (!dcmi->num_of_sd_formats)
> -			return -ENODATA;
> +		if (!dcmi->num_of_sd_formats) {
> +			ret = -ENODATA;
> +			goto done;
> +		}
>  
>  		sd_fmt = dcmi->sd_formats[dcmi->num_of_sd_formats - 1];
>  		pix->pixelformat = sd_fmt->fourcc;
> @@ -1036,10 +1040,9 @@ static int dcmi_try_fmt(struct stm32_dcmi *dcmi, struct v4l2_format *f,
>  	}
>  
>  	v4l2_fill_mbus_format(&format.format, pix, sd_fmt->mbus_code);
> -	ret = v4l2_subdev_call(dcmi->source, pad, set_fmt,
> -			       &pad_state, &format);
> +	ret = v4l2_subdev_call(dcmi->source, pad, set_fmt, sd_state, &format);
>  	if (ret < 0)
> -		return ret;
> +		goto done;
>  
>  	/* Update pix regarding to what sensor can do */
>  	v4l2_fill_pix_format(pix, &format.format);
> @@ -1079,7 +1082,9 @@ static int dcmi_try_fmt(struct stm32_dcmi *dcmi, struct v4l2_format *f,
>  	if (sd_framesize)
>  		*sd_framesize = sd_fsize;
>  
> -	return 0;
> +done:
> +	__v4l2_subdev_state_free(sd_state);
> +	return ret;
>  }
>  
>  static int dcmi_set_fmt(struct stm32_dcmi *dcmi, struct v4l2_format *f)
> @@ -1183,31 +1188,33 @@ static int dcmi_set_sensor_format(struct stm32_dcmi *dcmi,
>  				  struct v4l2_pix_format *pix)
>  {
>  	const struct dcmi_format *sd_fmt;
> +	static struct lock_class_key key;
> +	struct v4l2_subdev_state *sd_state;
>  	struct v4l2_subdev_format format = {
>  		.which = V4L2_SUBDEV_FORMAT_TRY,
>  	};
> -	struct v4l2_subdev_pad_config pad_cfg;
> -	struct v4l2_subdev_state pad_state = {
> -		.pads = &pad_cfg
> -		};
>  	int ret;
>  
> +	sd_state = __v4l2_subdev_state_alloc(dcmi->source, "dcmi:state->lock", &key);
> +	if (IS_ERR(sd_state))
> +		return PTR_ERR(sd_state);
> +
>  	sd_fmt = find_format_by_fourcc(dcmi, pix->pixelformat);
>  	if (!sd_fmt) {
> -		if (!dcmi->num_of_sd_formats)
> -			return -ENODATA;
> +		if (!dcmi->num_of_sd_formats) {
> +			ret = -ENODATA;
> +			goto done;
> +		}
>  
>  		sd_fmt = dcmi->sd_formats[dcmi->num_of_sd_formats - 1];
>  		pix->pixelformat = sd_fmt->fourcc;
>  	}
>  
>  	v4l2_fill_mbus_format(&format.format, pix, sd_fmt->mbus_code);
> -	ret = v4l2_subdev_call(dcmi->source, pad, set_fmt,
> -			       &pad_state, &format);
> -	if (ret < 0)
> -		return ret;
> -
> -	return 0;
> +	ret = v4l2_subdev_call(dcmi->source, pad, set_fmt, sd_state, &format);
> +done:
> +	__v4l2_subdev_state_free(sd_state);
> +	return ret;
>  }
>  
>  static int dcmi_get_sensor_bounds(struct stm32_dcmi *dcmi,

-- 
Regards,

Laurent Pinchart

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] media: stm32: dcmi: Switch to __v4l2_subdev_state_alloc()

[Tomi Valkeinen]

Hi,

On 19/06/2022 02:16, Laurent Pinchart wrote:
> Hi Marek,
> 
> CC'ing Tomi to get his opinion.
> 
> On Sun, Jun 19, 2022 at 12:24:42AM +0200, Marek Vasut wrote:
>> Any local subdev state should be allocated and free'd using
>> __v4l2_subdev_state_alloc()/__v4l2_subdev_state_free(), which
>> takes care of calling .init_cfg() subdev op. Without this,
>> subdev internal state might be uninitialized by the time
>> any other subdev op is called.

Does this fix a bug you have? Wasn't this broken even before the active 
state, as init_cfg was not called?

In any case, I think we have to do something like this, as the source 
subdev might depend on a valid subdev state.

It's not very nice to have the drivers using __v4l2_subdev_state_alloc, 
though. But if non-MC drivers are not going away, and if they are going 
to be calling ops in other subdevs with V4L2_SUBDEV_FORMAT_TRY, they 
need to pass a valid subdev state...

I don't see a better way right away, so I think this is fine.

Do we need a v4l2_subdev_call_state_try(), similar to 
v4l2_subdev_call_state_active()? It would handle allocating the state, 
calling the op and freeing the state.

  Tomi

Re: [PATCH] media: stm32: dcmi: Switch to __v4l2_subdev_state_alloc()

[Tomi Valkeinen]

Hi,

On 19/06/2022 02:16, Laurent Pinchart wrote:
> Hi Marek,
> 
> CC'ing Tomi to get his opinion.
> 
> On Sun, Jun 19, 2022 at 12:24:42AM +0200, Marek Vasut wrote:
>> Any local subdev state should be allocated and free'd using
>> __v4l2_subdev_state_alloc()/__v4l2_subdev_state_free(), which
>> takes care of calling .init_cfg() subdev op. Without this,
>> subdev internal state might be uninitialized by the time
>> any other subdev op is called.

Does this fix a bug you have? Wasn't this broken even before the active 
state, as init_cfg was not called?

In any case, I think we have to do something like this, as the source 
subdev might depend on a valid subdev state.

It's not very nice to have the drivers using __v4l2_subdev_state_alloc, 
though. But if non-MC drivers are not going away, and if they are going 
to be calling ops in other subdevs with V4L2_SUBDEV_FORMAT_TRY, they 
need to pass a valid subdev state...

I don't see a better way right away, so I think this is fine.

Do we need a v4l2_subdev_call_state_try(), similar to 
v4l2_subdev_call_state_active()? It would handle allocating the state, 
calling the op and freeing the state.

  Tomi

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] media: stm32: dcmi: Switch to __v4l2_subdev_state_alloc()

[Laurent Pinchart]

On Mon, Jun 20, 2022 at 12:44:02PM +0300, Tomi Valkeinen wrote:
> On 19/06/2022 02:16, Laurent Pinchart wrote:
> > Hi Marek,
> > 
> > CC'ing Tomi to get his opinion.
> > 
> > On Sun, Jun 19, 2022 at 12:24:42AM +0200, Marek Vasut wrote:
> >> Any local subdev state should be allocated and free'd using
> >> __v4l2_subdev_state_alloc()/__v4l2_subdev_state_free(), which
> >> takes care of calling .init_cfg() subdev op. Without this,
> >> subdev internal state might be uninitialized by the time
> >> any other subdev op is called.
> 
> Does this fix a bug you have? Wasn't this broken even before the active 
> state, as init_cfg was not called?
> 
> In any case, I think we have to do something like this, as the source 
> subdev might depend on a valid subdev state.
> 
> It's not very nice to have the drivers using __v4l2_subdev_state_alloc, 
> though. But if non-MC drivers are not going away,

You know my opinion on this :-) We shouldn't have any new user of
__v4l2_subdev_state_alloc() in new drivers, but fixing issues in
existing drivers is a valid use case. I'd like if the dcmi driver could
be converted to be MC-centric, but that will likely not happen.

> and if they are going 
> to be calling ops in other subdevs with V4L2_SUBDEV_FORMAT_TRY, they 
> need to pass a valid subdev state...
> 
> I don't see a better way right away, so I think this is fine.
> 
> Do we need a v4l2_subdev_call_state_try(), similar to 
> v4l2_subdev_call_state_active()? It would handle allocating the state, 
> calling the op and freeing the state.

I could imagine the user trying multiple operations on the same state,
but maybe a single call is a common enough use case to implement a
dedicated helper ?

-- 
Regards,

Laurent Pinchart

Re: [PATCH] media: stm32: dcmi: Switch to __v4l2_subdev_state_alloc()

[Laurent Pinchart]

On Mon, Jun 20, 2022 at 12:44:02PM +0300, Tomi Valkeinen wrote:
> On 19/06/2022 02:16, Laurent Pinchart wrote:
> > Hi Marek,
> > 
> > CC'ing Tomi to get his opinion.
> > 
> > On Sun, Jun 19, 2022 at 12:24:42AM +0200, Marek Vasut wrote:
> >> Any local subdev state should be allocated and free'd using
> >> __v4l2_subdev_state_alloc()/__v4l2_subdev_state_free(), which
> >> takes care of calling .init_cfg() subdev op. Without this,
> >> subdev internal state might be uninitialized by the time
> >> any other subdev op is called.
> 
> Does this fix a bug you have? Wasn't this broken even before the active 
> state, as init_cfg was not called?
> 
> In any case, I think we have to do something like this, as the source 
> subdev might depend on a valid subdev state.
> 
> It's not very nice to have the drivers using __v4l2_subdev_state_alloc, 
> though. But if non-MC drivers are not going away,

You know my opinion on this :-) We shouldn't have any new user of
__v4l2_subdev_state_alloc() in new drivers, but fixing issues in
existing drivers is a valid use case. I'd like if the dcmi driver could
be converted to be MC-centric, but that will likely not happen.

> and if they are going 
> to be calling ops in other subdevs with V4L2_SUBDEV_FORMAT_TRY, they 
> need to pass a valid subdev state...
> 
> I don't see a better way right away, so I think this is fine.
> 
> Do we need a v4l2_subdev_call_state_try(), similar to 
> v4l2_subdev_call_state_active()? It would handle allocating the state, 
> calling the op and freeing the state.

I could imagine the user trying multiple operations on the same state,
but maybe a single call is a common enough use case to implement a
dedicated helper ?

-- 
Regards,

Laurent Pinchart

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] media: stm32: dcmi: Switch to __v4l2_subdev_state_alloc()

[Marek Vasut]

On 6/20/22 11:44, Tomi Valkeinen wrote:
> Hi,

Hello all,

>> On Sun, Jun 19, 2022 at 12:24:42AM +0200, Marek Vasut wrote:
>>> Any local subdev state should be allocated and free'd using
>>> __v4l2_subdev_state_alloc()/__v4l2_subdev_state_free(), which
>>> takes care of calling .init_cfg() subdev op. Without this,
>>> subdev internal state might be uninitialized by the time
>>> any other subdev op is called.
> 
> Does this fix a bug you have?

Yes

> Wasn't this broken even before the active 
> state, as init_cfg was not called?

Yes, this was always broken. I suspect nobody tested this mode of 
operation before. In my case, I have this DCMI driver connected directly 
to MT9P006 sensor.

> In any case, I think we have to do something like this, as the source 
> subdev might depend on a valid subdev state.

Right.

[...]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] media: stm32: dcmi: Switch to __v4l2_subdev_state_alloc()

[Marek Vasut]

On 6/20/22 11:44, Tomi Valkeinen wrote:
> Hi,

Hello all,

>> On Sun, Jun 19, 2022 at 12:24:42AM +0200, Marek Vasut wrote:
>>> Any local subdev state should be allocated and free'd using
>>> __v4l2_subdev_state_alloc()/__v4l2_subdev_state_free(), which
>>> takes care of calling .init_cfg() subdev op. Without this,
>>> subdev internal state might be uninitialized by the time
>>> any other subdev op is called.
> 
> Does this fix a bug you have?

Yes

> Wasn't this broken even before the active 
> state, as init_cfg was not called?

Yes, this was always broken. I suspect nobody tested this mode of 
operation before. In my case, I have this DCMI driver connected directly 
to MT9P006 sensor.

> In any case, I think we have to do something like this, as the source 
> subdev might depend on a valid subdev state.

Right.

[...]

Re: [PATCH] media: stm32: dcmi: Switch to __v4l2_subdev_state_alloc()

[Hugues FRUCHET]

Hi Marek,


On 6/20/22 16:06, Marek Vasut wrote:
> On 6/20/22 11:44, Tomi Valkeinen wrote:
>> Hi,
> 
> Hello all,
> 
>>> On Sun, Jun 19, 2022 at 12:24:42AM +0200, Marek Vasut wrote:
>>>> Any local subdev state should be allocated and free'd using
>>>> __v4l2_subdev_state_alloc()/__v4l2_subdev_state_free(), which
>>>> takes care of calling .init_cfg() subdev op. Without this,
>>>> subdev internal state might be uninitialized by the time
>>>> any other subdev op is called.
>>
>> Does this fix a bug you have?
> 
> Yes

Which bug did you encounter exactly ?
This is strange that we have not yet encounter any problems around that 
through our tests campaigns... or we have to enforce with a new test, so 
better to know what your problem was exactly.

> 
>> Wasn't this broken even before the active state, as init_cfg was not 
>> called?
> 
> Yes, this was always broken. I suspect nobody tested this mode of 
> operation before. In my case, I have this DCMI driver connected directly 
> to MT9P006 sensor.

As far as I see, MT9P006 sensor is a 12 bits parallel interface sensor.
I don't see the difference with our OV5640 used in parallel mode which 
is a mainline config on our side, so one more time I wonder what the 
problem was.

> 
>> In any case, I think we have to do something like this, as the source 
>> subdev might depend on a valid subdev state.
> 
> Right.
> 
> [...]

BR,
Hugues.

Re: [PATCH] media: stm32: dcmi: Switch to __v4l2_subdev_state_alloc()

[Hugues FRUCHET]

Hi Marek,


On 6/20/22 16:06, Marek Vasut wrote:
> On 6/20/22 11:44, Tomi Valkeinen wrote:
>> Hi,
> 
> Hello all,
> 
>>> On Sun, Jun 19, 2022 at 12:24:42AM +0200, Marek Vasut wrote:
>>>> Any local subdev state should be allocated and free'd using
>>>> __v4l2_subdev_state_alloc()/__v4l2_subdev_state_free(), which
>>>> takes care of calling .init_cfg() subdev op. Without this,
>>>> subdev internal state might be uninitialized by the time
>>>> any other subdev op is called.
>>
>> Does this fix a bug you have?
> 
> Yes

Which bug did you encounter exactly ?
This is strange that we have not yet encounter any problems around that 
through our tests campaigns... or we have to enforce with a new test, so 
better to know what your problem was exactly.

> 
>> Wasn't this broken even before the active state, as init_cfg was not 
>> called?
> 
> Yes, this was always broken. I suspect nobody tested this mode of 
> operation before. In my case, I have this DCMI driver connected directly 
> to MT9P006 sensor.

As far as I see, MT9P006 sensor is a 12 bits parallel interface sensor.
I don't see the difference with our OV5640 used in parallel mode which 
is a mainline config on our side, so one more time I wonder what the 
problem was.

> 
>> In any case, I think we have to do something like this, as the source 
>> subdev might depend on a valid subdev state.
> 
> Right.
> 
> [...]

BR,
Hugues.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] media: stm32: dcmi: Switch to __v4l2_subdev_state_alloc()

[Marek Vasut]

On 6/27/22 11:14, Hugues FRUCHET wrote:
> Hi Marek,

Hi,

>>>> On Sun, Jun 19, 2022 at 12:24:42AM +0200, Marek Vasut wrote:
>>>>> Any local subdev state should be allocated and free'd using
>>>>> __v4l2_subdev_state_alloc()/__v4l2_subdev_state_free(), which
>>>>> takes care of calling .init_cfg() subdev op. Without this,
>>>>> subdev internal state might be uninitialized by the time
>>>>> any other subdev op is called.
>>>
>>> Does this fix a bug you have?
>>
>> Yes
> 
> Which bug did you encounter exactly ?

The DCMI driver does set_fmt subdev call on the sensor driver instance.

The mt9p031 sensor driver set_fmt depends on crop rectangle to be 
initialized _before_ set_fmt subdev call is made. Currently this 
initialization is done in open callback, which is too late, so when the 
DCMI does set_fmt subdev call, it operates on uninitialized private data.

There is patch to mt9p031 driver which move the initialization to the 
right place in .init_cfg:
[PATCH v2] media: mt9p031: Move open subdev op init code into init_cfg

However, the .init_cfg is not called by DCMI right now. For that to be 
called in the right place, __v4l2_subdev_state_alloc() must be added, 
hence this patch.

You won't trigger the problem on OV5640 because that one driver does not 
implement .init_cfg v4l2_subdev_ops .

> This is strange that we have not yet encounter any problems around that 
> through our tests campaigns... or we have to enforce with a new test, so 
> better to know what your problem was exactly.

You need a sensor driver which implements struct v4l2_subdev_ops 
.init_cfg and then have something in set_fmt depend on the 
initialization done in the .init_cfg callback . Then you would see the 
problem.

>>> Wasn't this broken even before the active state, as init_cfg was not 
>>> called?
>>
>> Yes, this was always broken. I suspect nobody tested this mode of 
>> operation before. In my case, I have this DCMI driver connected 
>> directly to MT9P006 sensor.
> 
> As far as I see, MT9P006 sensor is a 12 bits parallel interface sensor.
> I don't see the difference with our OV5640 used in parallel mode which 
> is a mainline config on our side, so one more time I wonder what the 
> problem was.

See above.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] media: stm32: dcmi: Switch to __v4l2_subdev_state_alloc()

[Marek Vasut]

On 6/27/22 11:14, Hugues FRUCHET wrote:
> Hi Marek,

Hi,

>>>> On Sun, Jun 19, 2022 at 12:24:42AM +0200, Marek Vasut wrote:
>>>>> Any local subdev state should be allocated and free'd using
>>>>> __v4l2_subdev_state_alloc()/__v4l2_subdev_state_free(), which
>>>>> takes care of calling .init_cfg() subdev op. Without this,
>>>>> subdev internal state might be uninitialized by the time
>>>>> any other subdev op is called.
>>>
>>> Does this fix a bug you have?
>>
>> Yes
> 
> Which bug did you encounter exactly ?

The DCMI driver does set_fmt subdev call on the sensor driver instance.

The mt9p031 sensor driver set_fmt depends on crop rectangle to be 
initialized _before_ set_fmt subdev call is made. Currently this 
initialization is done in open callback, which is too late, so when the 
DCMI does set_fmt subdev call, it operates on uninitialized private data.

There is patch to mt9p031 driver which move the initialization to the 
right place in .init_cfg:
[PATCH v2] media: mt9p031: Move open subdev op init code into init_cfg

However, the .init_cfg is not called by DCMI right now. For that to be 
called in the right place, __v4l2_subdev_state_alloc() must be added, 
hence this patch.

You won't trigger the problem on OV5640 because that one driver does not 
implement .init_cfg v4l2_subdev_ops .

> This is strange that we have not yet encounter any problems around that 
> through our tests campaigns... or we have to enforce with a new test, so 
> better to know what your problem was exactly.

You need a sensor driver which implements struct v4l2_subdev_ops 
.init_cfg and then have something in set_fmt depend on the 
initialization done in the .init_cfg callback . Then you would see the 
problem.

>>> Wasn't this broken even before the active state, as init_cfg was not 
>>> called?
>>
>> Yes, this was always broken. I suspect nobody tested this mode of 
>> operation before. In my case, I have this DCMI driver connected 
>> directly to MT9P006 sensor.
> 
> As far as I see, MT9P006 sensor is a 12 bits parallel interface sensor.
> I don't see the difference with our OV5640 used in parallel mode which 
> is a mainline config on our side, so one more time I wonder what the 
> problem was.

See above.

Re: [PATCH] media: stm32: dcmi: Switch to __v4l2_subdev_state_alloc()

[Hugues FRUCHET]

Hi Marek,

Thanks for explanation, I understand now.

Please note that dcmi is not atmel-isi.c has same code structure, hence 
same problem:

static int isi_try_fmt(struct atmel_isi *isi, struct v4l2_format *f,
	struct v4l2_subdev_state pad_state = {
		.pads = &pad_cfg
		};
[...]
	ret = v4l2_subdev_call(isi->entity.subdev, pad, set_fmt,


Moreover, searching for __v4l2_subdev_state_alloc() I see those "FIXME":

drivers/media/platform/renesas/vsp1/vsp1_entity.c
	/*
	 * FIXME: Drop this call, drivers are not supposed to use
	 * __v4l2_subdev_state_alloc().
	 */
	entity->config = __v4l2_subdev_state_alloc(&entity->subdev,
						   "vsp1:config->lock", &key);


drivers/media/platform/renesas/rcar-vin/rcar-v4l2.c
	/*
	 * FIXME: Drop this call, drivers are not supposed to use
	 * __v4l2_subdev_state_alloc().
	 */
	sd_state = __v4l2_subdev_state_alloc(sd, "rvin:state->lock", &key);


So I wonder about introducing this new change in dcmi while it is marked 
as "FIXME" in other camera interface drivers ?

Best regards,
Hugues.

On 6/27/22 13:30, Marek Vasut wrote:
> On 6/27/22 11:14, Hugues FRUCHET wrote:
>> Hi Marek,
> 
> Hi,
> 
>>>>> On Sun, Jun 19, 2022 at 12:24:42AM +0200, Marek Vasut wrote:
>>>>>> Any local subdev state should be allocated and free'd using
>>>>>> __v4l2_subdev_state_alloc()/__v4l2_subdev_state_free(), which
>>>>>> takes care of calling .init_cfg() subdev op. Without this,
>>>>>> subdev internal state might be uninitialized by the time
>>>>>> any other subdev op is called.
>>>>
>>>> Does this fix a bug you have?
>>>
>>> Yes
>>
>> Which bug did you encounter exactly ?
> 
> The DCMI driver does set_fmt subdev call on the sensor driver instance.
> 
> The mt9p031 sensor driver set_fmt depends on crop rectangle to be 
> initialized _before_ set_fmt subdev call is made. Currently this 
> initialization is done in open callback, which is too late, so when the 
> DCMI does set_fmt subdev call, it operates on uninitialized private data.
> 
> There is patch to mt9p031 driver which move the initialization to the 
> right place in .init_cfg:
> [PATCH v2] media: mt9p031: Move open subdev op init code into init_cfg
> 
> However, the .init_cfg is not called by DCMI right now. For that to be 
> called in the right place, __v4l2_subdev_state_alloc() must be added, 
> hence this patch.
> 
> You won't trigger the problem on OV5640 because that one driver does not 
> implement .init_cfg v4l2_subdev_ops .
> 
>> This is strange that we have not yet encounter any problems around 
>> that through our tests campaigns... or we have to enforce with a new 
>> test, so better to know what your problem was exactly.
> 
> You need a sensor driver which implements struct v4l2_subdev_ops 
> .init_cfg and then have something in set_fmt depend on the 
> initialization done in the .init_cfg callback . Then you would see the 
> problem.
> 
>>>> Wasn't this broken even before the active state, as init_cfg was not 
>>>> called?
>>>
>>> Yes, this was always broken. I suspect nobody tested this mode of 
>>> operation before. In my case, I have this DCMI driver connected 
>>> directly to MT9P006 sensor.
>>
>> As far as I see, MT9P006 sensor is a 12 bits parallel interface sensor.
>> I don't see the difference with our OV5640 used in parallel mode which 
>> is a mainline config on our side, so one more time I wonder what the 
>> problem was.
> 
> See above.

Re: [PATCH] media: stm32: dcmi: Switch to __v4l2_subdev_state_alloc()

[Hugues FRUCHET]

Hi Marek,

Thanks for explanation, I understand now.

Please note that dcmi is not atmel-isi.c has same code structure, hence 
same problem:

static int isi_try_fmt(struct atmel_isi *isi, struct v4l2_format *f,
	struct v4l2_subdev_state pad_state = {
		.pads = &pad_cfg
		};
[...]
	ret = v4l2_subdev_call(isi->entity.subdev, pad, set_fmt,


Moreover, searching for __v4l2_subdev_state_alloc() I see those "FIXME":

drivers/media/platform/renesas/vsp1/vsp1_entity.c
	/*
	 * FIXME: Drop this call, drivers are not supposed to use
	 * __v4l2_subdev_state_alloc().
	 */
	entity->config = __v4l2_subdev_state_alloc(&entity->subdev,
						   "vsp1:config->lock", &key);


drivers/media/platform/renesas/rcar-vin/rcar-v4l2.c
	/*
	 * FIXME: Drop this call, drivers are not supposed to use
	 * __v4l2_subdev_state_alloc().
	 */
	sd_state = __v4l2_subdev_state_alloc(sd, "rvin:state->lock", &key);


So I wonder about introducing this new change in dcmi while it is marked 
as "FIXME" in other camera interface drivers ?

Best regards,
Hugues.

On 6/27/22 13:30, Marek Vasut wrote:
> On 6/27/22 11:14, Hugues FRUCHET wrote:
>> Hi Marek,
> 
> Hi,
> 
>>>>> On Sun, Jun 19, 2022 at 12:24:42AM +0200, Marek Vasut wrote:
>>>>>> Any local subdev state should be allocated and free'd using
>>>>>> __v4l2_subdev_state_alloc()/__v4l2_subdev_state_free(), which
>>>>>> takes care of calling .init_cfg() subdev op. Without this,
>>>>>> subdev internal state might be uninitialized by the time
>>>>>> any other subdev op is called.
>>>>
>>>> Does this fix a bug you have?
>>>
>>> Yes
>>
>> Which bug did you encounter exactly ?
> 
> The DCMI driver does set_fmt subdev call on the sensor driver instance.
> 
> The mt9p031 sensor driver set_fmt depends on crop rectangle to be 
> initialized _before_ set_fmt subdev call is made. Currently this 
> initialization is done in open callback, which is too late, so when the 
> DCMI does set_fmt subdev call, it operates on uninitialized private data.
> 
> There is patch to mt9p031 driver which move the initialization to the 
> right place in .init_cfg:
> [PATCH v2] media: mt9p031: Move open subdev op init code into init_cfg
> 
> However, the .init_cfg is not called by DCMI right now. For that to be 
> called in the right place, __v4l2_subdev_state_alloc() must be added, 
> hence this patch.
> 
> You won't trigger the problem on OV5640 because that one driver does not 
> implement .init_cfg v4l2_subdev_ops .
> 
>> This is strange that we have not yet encounter any problems around 
>> that through our tests campaigns... or we have to enforce with a new 
>> test, so better to know what your problem was exactly.
> 
> You need a sensor driver which implements struct v4l2_subdev_ops 
> .init_cfg and then have something in set_fmt depend on the 
> initialization done in the .init_cfg callback . Then you would see the 
> problem.
> 
>>>> Wasn't this broken even before the active state, as init_cfg was not 
>>>> called?
>>>
>>> Yes, this was always broken. I suspect nobody tested this mode of 
>>> operation before. In my case, I have this DCMI driver connected 
>>> directly to MT9P006 sensor.
>>
>> As far as I see, MT9P006 sensor is a 12 bits parallel interface sensor.
>> I don't see the difference with our OV5640 used in parallel mode which 
>> is a mainline config on our side, so one more time I wonder what the 
>> problem was.
> 
> See above.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] media: stm32: dcmi: Switch to __v4l2_subdev_state_alloc()

[Marek Vasut]

On 6/27/22 14:53, Hugues FRUCHET wrote:
> Hi Marek,

Hi,

> Thanks for explanation, I understand now.
> 
> Please note that dcmi is not atmel-isi.c has same code structure, hence 
> same problem:
> 
> static int isi_try_fmt(struct atmel_isi *isi, struct v4l2_format *f,
>      struct v4l2_subdev_state pad_state = {
>          .pads = &pad_cfg
>          };
> [...]
>      ret = v4l2_subdev_call(isi->entity.subdev, pad, set_fmt,
> 
> 
> Moreover, searching for __v4l2_subdev_state_alloc() I see those "FIXME":
> 
> drivers/media/platform/renesas/vsp1/vsp1_entity.c
>      /*
>       * FIXME: Drop this call, drivers are not supposed to use
>       * __v4l2_subdev_state_alloc().
>       */
>      entity->config = __v4l2_subdev_state_alloc(&entity->subdev,
>                             "vsp1:config->lock", &key);
> 
> 
> drivers/media/platform/renesas/rcar-vin/rcar-v4l2.c
>      /*
>       * FIXME: Drop this call, drivers are not supposed to use
>       * __v4l2_subdev_state_alloc().
>       */
>      sd_state = __v4l2_subdev_state_alloc(sd, "rvin:state->lock", &key);
> 
> 
> So I wonder about introducing this new change in dcmi while it is marked 
> as "FIXME" in other camera interface drivers ?

This is probably something Tomi/Laurent can answer better. It should be 
OK for this driver as far as I understand the discussion in this thread.

Re: [PATCH] media: stm32: dcmi: Switch to __v4l2_subdev_state_alloc()

[Marek Vasut]

On 6/27/22 14:53, Hugues FRUCHET wrote:
> Hi Marek,

Hi,

> Thanks for explanation, I understand now.
> 
> Please note that dcmi is not atmel-isi.c has same code structure, hence 
> same problem:
> 
> static int isi_try_fmt(struct atmel_isi *isi, struct v4l2_format *f,
>      struct v4l2_subdev_state pad_state = {
>          .pads = &pad_cfg
>          };
> [...]
>      ret = v4l2_subdev_call(isi->entity.subdev, pad, set_fmt,
> 
> 
> Moreover, searching for __v4l2_subdev_state_alloc() I see those "FIXME":
> 
> drivers/media/platform/renesas/vsp1/vsp1_entity.c
>      /*
>       * FIXME: Drop this call, drivers are not supposed to use
>       * __v4l2_subdev_state_alloc().
>       */
>      entity->config = __v4l2_subdev_state_alloc(&entity->subdev,
>                             "vsp1:config->lock", &key);
> 
> 
> drivers/media/platform/renesas/rcar-vin/rcar-v4l2.c
>      /*
>       * FIXME: Drop this call, drivers are not supposed to use
>       * __v4l2_subdev_state_alloc().
>       */
>      sd_state = __v4l2_subdev_state_alloc(sd, "rvin:state->lock", &key);
> 
> 
> So I wonder about introducing this new change in dcmi while it is marked 
> as "FIXME" in other camera interface drivers ?

This is probably something Tomi/Laurent can answer better. It should be 
OK for this driver as far as I understand the discussion in this thread.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] media: stm32: dcmi: Switch to __v4l2_subdev_state_alloc()

[Tomi Valkeinen]

On 27/06/2022 16:01, Marek Vasut wrote:
> On 6/27/22 14:53, Hugues FRUCHET wrote:
>> Hi Marek,
> 
> Hi,
> 
>> Thanks for explanation, I understand now.
>>
>> Please note that dcmi is not atmel-isi.c has same code structure, 
>> hence same problem:
>>
>> static int isi_try_fmt(struct atmel_isi *isi, struct v4l2_format *f,
>>      struct v4l2_subdev_state pad_state = {
>>          .pads = &pad_cfg
>>          };
>> [...]
>>      ret = v4l2_subdev_call(isi->entity.subdev, pad, set_fmt,
>>
>>
>> Moreover, searching for __v4l2_subdev_state_alloc() I see those "FIXME":
>>
>> drivers/media/platform/renesas/vsp1/vsp1_entity.c
>>      /*
>>       * FIXME: Drop this call, drivers are not supposed to use
>>       * __v4l2_subdev_state_alloc().
>>       */
>>      entity->config = __v4l2_subdev_state_alloc(&entity->subdev,
>>                             "vsp1:config->lock", &key);
>>
>>
>> drivers/media/platform/renesas/rcar-vin/rcar-v4l2.c
>>      /*
>>       * FIXME: Drop this call, drivers are not supposed to use
>>       * __v4l2_subdev_state_alloc().
>>       */
>>      sd_state = __v4l2_subdev_state_alloc(sd, "rvin:state->lock", &key);
>>
>>
>> So I wonder about introducing this new change in dcmi while it is 
>> marked as "FIXME" in other camera interface drivers ?
> 
> This is probably something Tomi/Laurent can answer better. It should be 
> OK for this driver as far as I understand the discussion in this thread.

Yes and no. We shouldn't use __ funcs in the drivers. 
__v4l2_subdev_state_alloc() calls exist in the current drivers as it 
wasn't trivial to change the driver to do it otherwise while adding the 
subdev state feature.

If I recall right, the other users (at least some of them) were storing 
internal state in the state, not passing it forward. And, of course, the 
drivers were themselves interested in the state stored there.

Here, we only need to allocate the state so that the driver is able to 
call set_fmt on another subdev, so it's a bit different case.

Anyway, I think it's _not_ ok to add __v4l2_subdev_state_alloc() without 
a FIXME comment. However, I think it's ok to add a helper func, similar 
to v4l2_subdev_call_state_active(), which in turn uses 
__v4l2_subdev_state_alloc.

However, if we end up in a situation where we think it's "normal" for 
drivers to call __v4l2_subdev_state_alloc, we need to rename it and drop 
the two underscores. But I think we're not there yet (and hopefully never).

  Tomi

Re: [PATCH] media: stm32: dcmi: Switch to __v4l2_subdev_state_alloc()

[Tomi Valkeinen]

On 27/06/2022 16:01, Marek Vasut wrote:
> On 6/27/22 14:53, Hugues FRUCHET wrote:
>> Hi Marek,
> 
> Hi,
> 
>> Thanks for explanation, I understand now.
>>
>> Please note that dcmi is not atmel-isi.c has same code structure, 
>> hence same problem:
>>
>> static int isi_try_fmt(struct atmel_isi *isi, struct v4l2_format *f,
>>      struct v4l2_subdev_state pad_state = {
>>          .pads = &pad_cfg
>>          };
>> [...]
>>      ret = v4l2_subdev_call(isi->entity.subdev, pad, set_fmt,
>>
>>
>> Moreover, searching for __v4l2_subdev_state_alloc() I see those "FIXME":
>>
>> drivers/media/platform/renesas/vsp1/vsp1_entity.c
>>      /*
>>       * FIXME: Drop this call, drivers are not supposed to use
>>       * __v4l2_subdev_state_alloc().
>>       */
>>      entity->config = __v4l2_subdev_state_alloc(&entity->subdev,
>>                             "vsp1:config->lock", &key);
>>
>>
>> drivers/media/platform/renesas/rcar-vin/rcar-v4l2.c
>>      /*
>>       * FIXME: Drop this call, drivers are not supposed to use
>>       * __v4l2_subdev_state_alloc().
>>       */
>>      sd_state = __v4l2_subdev_state_alloc(sd, "rvin:state->lock", &key);
>>
>>
>> So I wonder about introducing this new change in dcmi while it is 
>> marked as "FIXME" in other camera interface drivers ?
> 
> This is probably something Tomi/Laurent can answer better. It should be 
> OK for this driver as far as I understand the discussion in this thread.

Yes and no. We shouldn't use __ funcs in the drivers. 
__v4l2_subdev_state_alloc() calls exist in the current drivers as it 
wasn't trivial to change the driver to do it otherwise while adding the 
subdev state feature.

If I recall right, the other users (at least some of them) were storing 
internal state in the state, not passing it forward. And, of course, the 
drivers were themselves interested in the state stored there.

Here, we only need to allocate the state so that the driver is able to 
call set_fmt on another subdev, so it's a bit different case.

Anyway, I think it's _not_ ok to add __v4l2_subdev_state_alloc() without 
a FIXME comment. However, I think it's ok to add a helper func, similar 
to v4l2_subdev_call_state_active(), which in turn uses 
__v4l2_subdev_state_alloc.

However, if we end up in a situation where we think it's "normal" for 
drivers to call __v4l2_subdev_state_alloc, we need to rename it and drop 
the two underscores. But I think we're not there yet (and hopefully never).

  Tomi

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] media: stm32: dcmi: Switch to __v4l2_subdev_state_alloc()

[Hugues FRUCHET]

Thanks Tomi for details,

OK for me with a FIXME on top, for the sake of uniformity with other 
drivers.

BR,
Hugues.

On 6/27/22 15:30, Tomi Valkeinen wrote:
> On 27/06/2022 16:01, Marek Vasut wrote:
>> On 6/27/22 14:53, Hugues FRUCHET wrote:
>>> Hi Marek,
>>
>> Hi,
>>
>>> Thanks for explanation, I understand now.
>>>
>>> Please note that dcmi is not atmel-isi.c has same code structure, 
>>> hence same problem:
>>>
>>> static int isi_try_fmt(struct atmel_isi *isi, struct v4l2_format *f,
>>>      struct v4l2_subdev_state pad_state = {
>>>          .pads = &pad_cfg
>>>          };
>>> [...]
>>>      ret = v4l2_subdev_call(isi->entity.subdev, pad, set_fmt,
>>>
>>>
>>> Moreover, searching for __v4l2_subdev_state_alloc() I see those "FIXME":
>>>
>>> drivers/media/platform/renesas/vsp1/vsp1_entity.c
>>>      /*
>>>       * FIXME: Drop this call, drivers are not supposed to use
>>>       * __v4l2_subdev_state_alloc().
>>>       */
>>>      entity->config = __v4l2_subdev_state_alloc(&entity->subdev,
>>>                             "vsp1:config->lock", &key);
>>>
>>>
>>> drivers/media/platform/renesas/rcar-vin/rcar-v4l2.c
>>>      /*
>>>       * FIXME: Drop this call, drivers are not supposed to use
>>>       * __v4l2_subdev_state_alloc().
>>>       */
>>>      sd_state = __v4l2_subdev_state_alloc(sd, "rvin:state->lock", &key);
>>>
>>>
>>> So I wonder about introducing this new change in dcmi while it is 
>>> marked as "FIXME" in other camera interface drivers ?
>>
>> This is probably something Tomi/Laurent can answer better. It should 
>> be OK for this driver as far as I understand the discussion in this 
>> thread.
> 
> Yes and no. We shouldn't use __ funcs in the drivers. 
> __v4l2_subdev_state_alloc() calls exist in the current drivers as it 
> wasn't trivial to change the driver to do it otherwise while adding the 
> subdev state feature.
> 
> If I recall right, the other users (at least some of them) were storing 
> internal state in the state, not passing it forward. And, of course, the 
> drivers were themselves interested in the state stored there.
> 
> Here, we only need to allocate the state so that the driver is able to 
> call set_fmt on another subdev, so it's a bit different case.
> 
> Anyway, I think it's _not_ ok to add __v4l2_subdev_state_alloc() without 
> a FIXME comment. However, I think it's ok to add a helper func, similar 
> to v4l2_subdev_call_state_active(), which in turn uses 
> __v4l2_subdev_state_alloc.
> 
> However, if we end up in a situation where we think it's "normal" for 
> drivers to call __v4l2_subdev_state_alloc, we need to rename it and drop 
> the two underscores. But I think we're not there yet (and hopefully never).
> 
>   Tomi

Re: [PATCH] media: stm32: dcmi: Switch to __v4l2_subdev_state_alloc()

[Hugues FRUCHET]

Thanks Tomi for details,

OK for me with a FIXME on top, for the sake of uniformity with other 
drivers.

BR,
Hugues.

On 6/27/22 15:30, Tomi Valkeinen wrote:
> On 27/06/2022 16:01, Marek Vasut wrote:
>> On 6/27/22 14:53, Hugues FRUCHET wrote:
>>> Hi Marek,
>>
>> Hi,
>>
>>> Thanks for explanation, I understand now.
>>>
>>> Please note that dcmi is not atmel-isi.c has same code structure, 
>>> hence same problem:
>>>
>>> static int isi_try_fmt(struct atmel_isi *isi, struct v4l2_format *f,
>>>      struct v4l2_subdev_state pad_state = {
>>>          .pads = &pad_cfg
>>>          };
>>> [...]
>>>      ret = v4l2_subdev_call(isi->entity.subdev, pad, set_fmt,
>>>
>>>
>>> Moreover, searching for __v4l2_subdev_state_alloc() I see those "FIXME":
>>>
>>> drivers/media/platform/renesas/vsp1/vsp1_entity.c
>>>      /*
>>>       * FIXME: Drop this call, drivers are not supposed to use
>>>       * __v4l2_subdev_state_alloc().
>>>       */
>>>      entity->config = __v4l2_subdev_state_alloc(&entity->subdev,
>>>                             "vsp1:config->lock", &key);
>>>
>>>
>>> drivers/media/platform/renesas/rcar-vin/rcar-v4l2.c
>>>      /*
>>>       * FIXME: Drop this call, drivers are not supposed to use
>>>       * __v4l2_subdev_state_alloc().
>>>       */
>>>      sd_state = __v4l2_subdev_state_alloc(sd, "rvin:state->lock", &key);
>>>
>>>
>>> So I wonder about introducing this new change in dcmi while it is 
>>> marked as "FIXME" in other camera interface drivers ?
>>
>> This is probably something Tomi/Laurent can answer better. It should 
>> be OK for this driver as far as I understand the discussion in this 
>> thread.
> 
> Yes and no. We shouldn't use __ funcs in the drivers. 
> __v4l2_subdev_state_alloc() calls exist in the current drivers as it 
> wasn't trivial to change the driver to do it otherwise while adding the 
> subdev state feature.
> 
> If I recall right, the other users (at least some of them) were storing 
> internal state in the state, not passing it forward. And, of course, the 
> drivers were themselves interested in the state stored there.
> 
> Here, we only need to allocate the state so that the driver is able to 
> call set_fmt on another subdev, so it's a bit different case.
> 
> Anyway, I think it's _not_ ok to add __v4l2_subdev_state_alloc() without 
> a FIXME comment. However, I think it's ok to add a helper func, similar 
> to v4l2_subdev_call_state_active(), which in turn uses 
> __v4l2_subdev_state_alloc.
> 
> However, if we end up in a situation where we think it's "normal" for 
> drivers to call __v4l2_subdev_state_alloc, we need to rename it and drop 
> the two underscores. But I think we're not there yet (and hopefully never).
> 
>   Tomi

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel