Hi Laurent
On Fri, Jul 04, 2025 at 03:18:07AM +0300, Laurent Pinchart wrote:
> The media bus code passed to the .enum_frame_size() operation for the
> sink pad is required to be supported by the device, but not to match the
> current format. All entities that use the vsp1_subdev_enum_frame_size()
> helper, as well as the SRU and UDS entities that implement the operation
> manually, perform the check incorrectly.
>
> Fix the issue by implementing the correct code check in the
> vsp1_subdev_enum_frame_size(). For the SRU and UDS, to avoid duplicating
> code, use the vsp1_subdev_enum_frame_size() as a base and override the
> enumerated size on the source pad with entity-specific constraints.
>
> While at it, include the missing <linux/mutex.h> as the code locks
> mutexes.
>
> Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@xxxxxxxxxxxxxxxx>
> ---
> Changes since v1:
>
> - Include <linux/cleanup.h> and <linux/mutex.h>
> ---
> .../media/platform/renesas/vsp1/vsp1_entity.c | 49 ++++++++++++-------
> .../media/platform/renesas/vsp1/vsp1_sru.c | 38 +++++++-------
> .../media/platform/renesas/vsp1/vsp1_uds.c | 38 +++++++-------
> 3 files changed, 64 insertions(+), 61 deletions(-)
>
> diff --git a/drivers/media/platform/renesas/vsp1/vsp1_entity.c b/drivers/media/platform/renesas/vsp1/vsp1_entity.c
> index 04b7ae6fb935..892a2adfdf3a 100644
> --- a/drivers/media/platform/renesas/vsp1/vsp1_entity.c
> +++ b/drivers/media/platform/renesas/vsp1/vsp1_entity.c
> @@ -7,8 +7,10 @@
> * Contact: Laurent Pinchart (laurent.pinchart@xxxxxxxxxxxxxxxx)
> */
>
> +#include <linux/cleanup.h>
> #include <linux/device.h>
> #include <linux/gfp.h>
> +#include <linux/mutex.h>
>
> #include <media/media-entity.h>
> #include <media/v4l2-ctrls.h>
> @@ -238,42 +240,51 @@ int vsp1_subdev_enum_frame_size(struct v4l2_subdev *subdev,
> struct v4l2_subdev_frame_size_enum *fse)
> {
> struct vsp1_entity *entity = to_vsp1_entity(subdev);
> - struct v4l2_subdev_state *state;
> - struct v4l2_mbus_framefmt *format;
> - int ret = 0;
>
> - state = vsp1_entity_get_state(entity, sd_state, fse->which);
> - if (!state)
> + if (fse->index)
> return -EINVAL;
>
> - format = v4l2_subdev_state_get_format(state, fse->pad);
> -
> - mutex_lock(&entity->lock);
> -
> - if (fse->index || fse->code != format->code) {
> - ret = -EINVAL;
> - goto done;
> - }
> -
> if (fse->pad == 0) {
> + unsigned int i;
> +
> + for (i = 0; i < entity->num_codes; ++i) {
> + if (fse->code == entity->codes[i])
> + break;
> + }
> +
> + if (i == entity->num_codes)
> + return -EINVAL;
> +
> fse->min_width = entity->min_width;
> fse->max_width = entity->max_width;
> fse->min_height = entity->min_height;
> fse->max_height = entity->max_height;
> } else {
> + struct v4l2_subdev_state *state;
> + struct v4l2_mbus_framefmt *format;
> +
> + state = vsp1_entity_get_state(entity, sd_state, fse->which);
> + if (!state)
> + return -EINVAL;
> +
> /*
> - * The size on the source pad are fixed and always identical to
> - * the size on the sink pad.
> + * The media bus code and size on the source pad are fixed and
> + * always identical to the sink pad.
> */
> + format = v4l2_subdev_state_get_format(state, 0);
> +
> + guard(mutex)(&entity->lock);
> +
> + if (fse->code != format->code)
> + return -EINVAL;
> +
> fse->min_width = format->width;
> fse->max_width = format->width;
> fse->min_height = format->height;
> fse->max_height = format->height;
> }
>
> -done:
> - mutex_unlock(&entity->lock);
> - return ret;
> + return 0;
> }
>
> /*
> diff --git a/drivers/media/platform/renesas/vsp1/vsp1_sru.c b/drivers/media/platform/renesas/vsp1/vsp1_sru.c
> index 1dc34e6a510d..37fd36d09045 100644
> --- a/drivers/media/platform/renesas/vsp1/vsp1_sru.c
> +++ b/drivers/media/platform/renesas/vsp1/vsp1_sru.c
> @@ -7,8 +7,10 @@
> * Contact: Laurent Pinchart (laurent.pinchart@xxxxxxxxxxxxxxxx)
> */
>
> +#include <linux/cleanup.h>
> #include <linux/device.h>
> #include <linux/gfp.h>
> +#include <linux/mutex.h>
>
> #include <media/v4l2-subdev.h>
>
> @@ -116,29 +118,25 @@ static int sru_enum_frame_size(struct v4l2_subdev *subdev,
> struct v4l2_subdev_frame_size_enum *fse)
> {
> struct vsp1_sru *sru = to_sru(subdev);
> - struct v4l2_subdev_state *state;
> - struct v4l2_mbus_framefmt *format;
> - int ret = 0;
> + int ret;
>
> - state = vsp1_entity_get_state(&sru->entity, sd_state, fse->which);
> - if (!state)
> - return -EINVAL;
> + ret = vsp1_subdev_enum_frame_size(subdev, sd_state, fse);
> + if (ret)
> + return ret;
Here, in case of PAD_SOURCE, we have validated that the fse->code
matches the format on the sink
>
> - format = v4l2_subdev_state_get_format(state, SRU_PAD_SINK);
> + if (fse->pad == SRU_PAD_SOURCE) {
> + struct v4l2_subdev_state *state;
> + struct v4l2_mbus_framefmt *format;
>
> - mutex_lock(&sru->entity.lock);
> + state = vsp1_entity_get_state(&sru->entity, sd_state,
> + fse->which);
> + if (!state)
> + return -EINVAL;
>
> - if (fse->index || fse->code != format->code) {
> - ret = -EINVAL;
> - goto done;
> - }
> + format = v4l2_subdev_state_get_format(state, SRU_PAD_SINK);
> +
> + guard(mutex)(&sru->entity.lock);
and we get here where we do SRU-specific adjustments to the sizes but
we don't re-check for the code.
Can the code on the sink change between the above call to
vsp1_subdev_enum_frame_size() and here ? Is this a concern ?
Same for UDS I guess..
Thanks
j
>
> - if (fse->pad == SRU_PAD_SINK) {
> - fse->min_width = SRU_MIN_SIZE;
> - fse->max_width = SRU_MAX_SIZE;
> - fse->min_height = SRU_MIN_SIZE;
> - fse->max_height = SRU_MAX_SIZE;
> - } else {
> fse->min_width = format->width;
> fse->min_height = format->height;
> if (format->width <= SRU_MAX_SIZE / 2 &&
> @@ -151,9 +149,7 @@ static int sru_enum_frame_size(struct v4l2_subdev *subdev,
> }
> }
>
> -done:
> - mutex_unlock(&sru->entity.lock);
> - return ret;
> + return 0;
> }
>
> static void sru_try_format(struct vsp1_sru *sru,
> diff --git a/drivers/media/platform/renesas/vsp1/vsp1_uds.c b/drivers/media/platform/renesas/vsp1/vsp1_uds.c
> index 8006d49ffbea..dd4722315c56 100644
> --- a/drivers/media/platform/renesas/vsp1/vsp1_uds.c
> +++ b/drivers/media/platform/renesas/vsp1/vsp1_uds.c
> @@ -7,8 +7,10 @@
> * Contact: Laurent Pinchart (laurent.pinchart@xxxxxxxxxxxxxxxx)
> */
>
> +#include <linux/cleanup.h>
> #include <linux/device.h>
> #include <linux/gfp.h>
> +#include <linux/mutex.h>
>
> #include <media/v4l2-subdev.h>
>
> @@ -121,38 +123,32 @@ static int uds_enum_frame_size(struct v4l2_subdev *subdev,
> struct v4l2_subdev_frame_size_enum *fse)
> {
> struct vsp1_uds *uds = to_uds(subdev);
> - struct v4l2_subdev_state *state;
> - struct v4l2_mbus_framefmt *format;
> - int ret = 0;
> + int ret;
>
> - state = vsp1_entity_get_state(&uds->entity, sd_state, fse->which);
> - if (!state)
> - return -EINVAL;
> + ret = vsp1_subdev_enum_frame_size(subdev, sd_state, fse);
> + if (ret)
> + return ret;
>
> - format = v4l2_subdev_state_get_format(state, UDS_PAD_SINK);
> + if (fse->pad == UDS_PAD_SOURCE) {
> + struct v4l2_subdev_state *state;
> + struct v4l2_mbus_framefmt *format;
>
> - mutex_lock(&uds->entity.lock);
> + state = vsp1_entity_get_state(&uds->entity, sd_state,
> + fse->which);
> + if (!state)
> + return -EINVAL;
>
> - if (fse->index || fse->code != format->code) {
> - ret = -EINVAL;
> - goto done;
> - }
> + format = v4l2_subdev_state_get_format(state, UDS_PAD_SINK);
> +
> + guard(mutex)(&uds->entity.lock);
>
> - if (fse->pad == UDS_PAD_SINK) {
> - fse->min_width = UDS_MIN_SIZE;
> - fse->max_width = UDS_MAX_SIZE;
> - fse->min_height = UDS_MIN_SIZE;
> - fse->max_height = UDS_MAX_SIZE;
> - } else {
> uds_output_limits(format->width, &fse->min_width,
> &fse->max_width);
> uds_output_limits(format->height, &fse->min_height,
> &fse->max_height);
> }
>
> -done:
> - mutex_unlock(&uds->entity.lock);
> - return ret;
> + return 0;
> }
>
> static void uds_try_format(struct vsp1_uds *uds,
> --
> Regards,
>
> Laurent Pinchart
>
>
