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

[Tomi Valkeinen]

On 29/06/2022 15:39, Marek Vasut wrote:
> On 6/29/22 14:26, Tomi Valkeinen wrote:
> 
> [...]
> 
>>>> Perhaps the best way to solve this is just to remove the underscores
>>>> from __v4l2_subdev_state_alloc, and change all the drivers which create
>>>> temporary v4l2_subdev_states to use that (and the free) functions. And
>>>> also create the helper macro which can be used in those cases where the
>>>> call is simple (the state is not modified or accessed by the caller).
>>>
>>> As long as we prevent any new driver from using that API, that's fine
>>> with me.
>>
>> An alternative would be to keep the v4l2_subdev_state as a local 
>> variable (allocated in the stack), and add a new function, 
>> v4l2_subdev_state_local_init() or such. The function would initialize 
>> the given state, expecting the allocatable fields to be already 
>> allocated (state->pads, which in the above cases points to another 
>> local variable, i.e. stack).
>>
>> This would prevent the need of a free call, which, while not complex 
>> as such, might cause a bigger amount of changes in some cases to 
>> handle the error paths correctly.
>>
>> Of course, if the above-mentioned macro works, then that's the easiest 
>> solution. But that won't work for all drivers.
> 
> Don't you think a driver fix shouldn't involve "rework the subsystem" 
> requirement to be applicable ?

No, but we should think what's the best way to do the fix, if the fix
is controversial. Otherwise we might just break things even worse.
Adding the macro seems like a much better way, and far from "rework the
subsystem". Granted, this was just a quick edit without testing so it may
fail miserably...

Can you try this out?

Also, a bit unrelated thing: dcmi_get_sensor_format gets the active format
from the source subdev, but dcmi_set_sensor_format sets the try format. That
sounds like a bug. Is it on purpose?


diff --git a/drivers/media/platform/st/stm32/stm32-dcmi.c b/drivers/media/platform/st/stm32/stm32-dcmi.c
index 09a743cd7004..eb831b5932e7 100644
--- a/drivers/media/platform/st/stm32/stm32-dcmi.c
+++ b/drivers/media/platform/st/stm32/stm32-dcmi.c
@@ -999,10 +999,6 @@ static int dcmi_try_fmt(struct stm32_dcmi *dcmi, struct v4l2_format *f,
  	const struct dcmi_format *sd_fmt;
  	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_format format = {
  		.which = V4L2_SUBDEV_FORMAT_TRY,
  	};
@@ -1037,8 +1033,7 @@ 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_state_try(dcmi->source, pad, set_fmt, &format);
  	if (ret < 0)
  		return ret;
  
@@ -1187,10 +1182,6 @@ static int dcmi_set_sensor_format(struct stm32_dcmi *dcmi,
  	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_fmt = find_format_by_fourcc(dcmi, pix->pixelformat);
@@ -1203,8 +1194,7 @@ static int dcmi_set_sensor_format(struct stm32_dcmi *dcmi,
  	}
  
  	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_state_try(dcmi->source, pad, set_fmt, &format);
  	if (ret < 0)
  		return ret;
  
diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
index b661e1817470..68676d173047 100644
--- a/include/media/v4l2-subdev.h
+++ b/include/media/v4l2-subdev.h
@@ -1433,6 +1433,19 @@ extern const struct v4l2_subdev_ops v4l2_subdev_call_wrappers;
  		__result;						\
  	})
  
+#define v4l2_subdev_call_state_try(sd, o, f, args...)                 \
+	({                                                            \
+		int __result;                                         \
+		static struct lock_class_key __key;                   \
+		const char *name = KBUILD_BASENAME                    \
+			":" __stringify(__LINE__) ":state->lock";     \
+		struct v4l2_subdev_state *state =                     \
+			__v4l2_subdev_state_alloc(sd, name, &__key);  \
+		__result = v4l2_subdev_call(sd, o, f, state, ##args); \
+		__v4l2_subdev_state_free(state);                      \
+		__result;                                             \
+	})
+
  /**
   * v4l2_subdev_has_op - Checks if a subdev defines a certain operation.
   *


  Tomi