On Wed, May 28, 2025 at 04:45:58PM +0200, Jacopo Mondi wrote:
> On Wed, Apr 30, 2025 at 02:53:18AM +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.
> >
> > Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@xxxxxxxxxxxxxxxx>
> > ---
> > .../media/platform/renesas/vsp1/vsp1_entity.c | 47 +++++++++++--------
> > .../media/platform/renesas/vsp1/vsp1_sru.c | 36 ++++++--------
> > .../media/platform/renesas/vsp1/vsp1_uds.c | 36 ++++++--------
> > 3 files changed, 58 insertions(+), 61 deletions(-)
> >
> > diff --git a/drivers/media/platform/renesas/vsp1/vsp1_entity.c b/drivers/media/platform/renesas/vsp1/vsp1_entity.c
> > index 0d4fe0028b13..a3d4bf2887ec 100644
> > --- a/drivers/media/platform/renesas/vsp1/vsp1_entity.c
> > +++ b/drivers/media/platform/renesas/vsp1/vsp1_entity.c
> > @@ -233,42 +233,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);
>
> Should you now include cleanup.h ?
I'm in two minds about that. Yes, the guard() macro is provided by
cleanup.h, but the API here is really guard(mutex), which is provided by
mutex.h by using DEFINE_GUARD(), provided by cleanup.h. There's
therefore a guarantee cleanup.h is included.
I think I'll still include cleanup.h in v2, and while at it I'll also
include mutex.h that was missing.
> > +
> > + 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..e821eac1cbc2 100644
> > --- a/drivers/media/platform/renesas/vsp1/vsp1_sru.c
> > +++ b/drivers/media/platform/renesas/vsp1/vsp1_sru.c
> > @@ -116,29 +116,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;
> >
> > - 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);
> >
> > - 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 +147,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..95c9939ae077 100644
> > --- a/drivers/media/platform/renesas/vsp1/vsp1_uds.c
> > +++ b/drivers/media/platform/renesas/vsp1/vsp1_uds.c
> > @@ -121,38 +121,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
