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
