| Message ID | 20241001140919.206139-14-prabhakar.mahadev-lad.rj@bp.renesas.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Kieran Bingham |
| Headers | show |
| Series | media: platform: rzg2l-cru: CSI-2 and CRU enhancements | expand |
Hi Prabhakar, Thank you for the patch. On Tue, Oct 01, 2024 at 03:09:15PM +0100, Prabhakar wrote: > From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> > > Implement the `.link_validate()` callback for the video node and move the > format checking into this function. This change allows the removal of > `rzg2l_cru_mc_validate_format()`. > > Suggested-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com> > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> > --- > v2->v3 > - New patch > --- > .../platform/renesas/rzg2l-cru/rzg2l-video.c | 99 ++++++++++--------- > 1 file changed, 55 insertions(+), 44 deletions(-) > > diff --git a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c > index ceb9012c9d70..c6c82b9b130a 100644 > --- a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c > +++ b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c > @@ -189,46 +189,6 @@ static void rzg2l_cru_buffer_queue(struct vb2_buffer *vb) > spin_unlock_irqrestore(&cru->qlock, flags); > } > > -static int rzg2l_cru_mc_validate_format(struct rzg2l_cru_dev *cru, > - struct v4l2_subdev *sd, > - struct media_pad *pad) > -{ > - struct v4l2_subdev_format fmt = { > - .which = V4L2_SUBDEV_FORMAT_ACTIVE, > - }; > - > - fmt.pad = pad->index; > - if (v4l2_subdev_call_state_active(sd, pad, get_fmt, &fmt)) > - return -EPIPE; > - > - switch (fmt.format.code) { > - case MEDIA_BUS_FMT_UYVY8_1X16: > - break; > - default: > - return -EPIPE; > - } > - > - switch (fmt.format.field) { > - case V4L2_FIELD_TOP: > - case V4L2_FIELD_BOTTOM: > - case V4L2_FIELD_NONE: > - case V4L2_FIELD_INTERLACED_TB: > - case V4L2_FIELD_INTERLACED_BT: > - case V4L2_FIELD_INTERLACED: > - case V4L2_FIELD_SEQ_TB: > - case V4L2_FIELD_SEQ_BT: > - break; > - default: > - return -EPIPE; > - } > - > - if (fmt.format.width != cru->format.width || > - fmt.format.height != cru->format.height) > - return -EPIPE; > - > - return 0; > -} > - > static void rzg2l_cru_set_slot_addr(struct rzg2l_cru_dev *cru, > int slot, dma_addr_t addr) > { > @@ -531,10 +491,6 @@ static int rzg2l_cru_set_stream(struct rzg2l_cru_dev *cru, int on) > return stream_off_ret; > } > > - ret = rzg2l_cru_mc_validate_format(cru, sd, pad); > - if (ret) > - return ret; > - > pipe = media_entity_pipeline(&sd->entity) ? : &cru->vdev.pipe; > ret = video_device_pipeline_start(&cru->vdev, pipe); > if (ret) > @@ -995,6 +951,60 @@ static const struct v4l2_file_operations rzg2l_cru_fops = { > .read = vb2_fop_read, > }; > > +/* ----------------------------------------------------------------------------- > + * Media entity operations > + */ > + > +static int rzg2l_cru_video_link_validate(struct media_link *link) > +{ > + struct v4l2_subdev_format fmt = { > + .which = V4L2_SUBDEV_FORMAT_ACTIVE, > + }; > + struct v4l2_subdev *subdev; > + struct media_entity *entity; > + struct rzg2l_cru_dev *cru; > + struct media_pad *remote; > + int ret; > + > + entity = link->sink->entity; > + remote = link->source; > + > + subdev = media_entity_to_v4l2_subdev(remote->entity); > + fmt.pad = remote->index; > + ret = v4l2_subdev_call(subdev, pad, get_fmt, NULL, &fmt); > + if (ret < 0) > + return ret == -ENOIOCTLCMD ? -EINVAL : ret; > + > + if (!rzg2l_cru_ip_code_to_fmt(fmt.format.code)) > + return -EPIPE; Here you should check that the format on the subdev matches the format on the video device. > + > + switch (fmt.format.field) { > + case V4L2_FIELD_TOP: > + case V4L2_FIELD_BOTTOM: > + case V4L2_FIELD_NONE: > + case V4L2_FIELD_INTERLACED_TB: > + case V4L2_FIELD_INTERLACED_BT: > + case V4L2_FIELD_INTERLACED: > + case V4L2_FIELD_SEQ_TB: > + case V4L2_FIELD_SEQ_BT: > + break; > + default: > + return -EPIPE; > + } Instead of checking the field here, shouldn't it be forced to a valid value in the subdev .set_fmt() function ? The link validation handler is responsible for validating that the configuration of the two sides of the link (IP subdev and video device) match. The driver shouldn't allow setting formats that can't be supported. What you should check here is that the field of the subdev and the field of the video device match. > + > + cru = container_of(media_entity_to_video_device(entity), You can drop the local entity variable and write cru = container_of(media_entity_to_video_device(link->sink->entity), > + struct rzg2l_cru_dev, vdev); > + if (fmt.format.width != cru->format.width || > + fmt.format.height != cru->format.height) > + return -EPIPE; > + > + return 0; > +} > + > +static const struct media_entity_operations rzg2l_cru_video_media_ops = { > + .link_validate = rzg2l_cru_video_link_validate, > +}; > + > static void rzg2l_cru_v4l2_init(struct rzg2l_cru_dev *cru) > { > struct video_device *vdev = &cru->vdev; > @@ -1006,6 +1016,7 @@ static void rzg2l_cru_v4l2_init(struct rzg2l_cru_dev *cru) > vdev->lock = &cru->lock; > vdev->device_caps = V4L2_CAP_VIDEO_CAPTURE | V4L2_CAP_STREAMING; > vdev->device_caps |= V4L2_CAP_IO_MC; > + vdev->entity.ops = &rzg2l_cru_video_media_ops; > vdev->fops = &rzg2l_cru_fops; > vdev->ioctl_ops = &rzg2l_cru_ioctl_ops; >
Hi Laurent, Thank you for the review. On Thu, Oct 3, 2024 at 3:51 PM Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > > Hi Prabhakar, > > Thank you for the patch. > > On Tue, Oct 01, 2024 at 03:09:15PM +0100, Prabhakar wrote: > > From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> > > > > Implement the `.link_validate()` callback for the video node and move the > > format checking into this function. This change allows the removal of > > `rzg2l_cru_mc_validate_format()`. > > > > Suggested-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com> > > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> > > --- > > v2->v3 > > - New patch > > --- > > .../platform/renesas/rzg2l-cru/rzg2l-video.c | 99 ++++++++++--------- > > 1 file changed, 55 insertions(+), 44 deletions(-) > > > > diff --git a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c > > index ceb9012c9d70..c6c82b9b130a 100644 > > --- a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c > > +++ b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c > > @@ -189,46 +189,6 @@ static void rzg2l_cru_buffer_queue(struct vb2_buffer *vb) > > spin_unlock_irqrestore(&cru->qlock, flags); > > } > > > > -static int rzg2l_cru_mc_validate_format(struct rzg2l_cru_dev *cru, > > - struct v4l2_subdev *sd, > > - struct media_pad *pad) > > -{ > > - struct v4l2_subdev_format fmt = { > > - .which = V4L2_SUBDEV_FORMAT_ACTIVE, > > - }; > > - > > - fmt.pad = pad->index; > > - if (v4l2_subdev_call_state_active(sd, pad, get_fmt, &fmt)) > > - return -EPIPE; > > - > > - switch (fmt.format.code) { > > - case MEDIA_BUS_FMT_UYVY8_1X16: > > - break; > > - default: > > - return -EPIPE; > > - } > > - > > - switch (fmt.format.field) { > > - case V4L2_FIELD_TOP: > > - case V4L2_FIELD_BOTTOM: > > - case V4L2_FIELD_NONE: > > - case V4L2_FIELD_INTERLACED_TB: > > - case V4L2_FIELD_INTERLACED_BT: > > - case V4L2_FIELD_INTERLACED: > > - case V4L2_FIELD_SEQ_TB: > > - case V4L2_FIELD_SEQ_BT: > > - break; > > - default: > > - return -EPIPE; > > - } > > - > > - if (fmt.format.width != cru->format.width || > > - fmt.format.height != cru->format.height) > > - return -EPIPE; > > - > > - return 0; > > -} > > - > > static void rzg2l_cru_set_slot_addr(struct rzg2l_cru_dev *cru, > > int slot, dma_addr_t addr) > > { > > @@ -531,10 +491,6 @@ static int rzg2l_cru_set_stream(struct rzg2l_cru_dev *cru, int on) > > return stream_off_ret; > > } > > > > - ret = rzg2l_cru_mc_validate_format(cru, sd, pad); > > - if (ret) > > - return ret; > > - > > pipe = media_entity_pipeline(&sd->entity) ? : &cru->vdev.pipe; > > ret = video_device_pipeline_start(&cru->vdev, pipe); > > if (ret) > > @@ -995,6 +951,60 @@ static const struct v4l2_file_operations rzg2l_cru_fops = { > > .read = vb2_fop_read, > > }; > > > > +/* ----------------------------------------------------------------------------- > > + * Media entity operations > > + */ > > + > > +static int rzg2l_cru_video_link_validate(struct media_link *link) > > +{ > > + struct v4l2_subdev_format fmt = { > > + .which = V4L2_SUBDEV_FORMAT_ACTIVE, > > + }; > > + struct v4l2_subdev *subdev; > > + struct media_entity *entity; > > + struct rzg2l_cru_dev *cru; > > + struct media_pad *remote; > > + int ret; > > + > > + entity = link->sink->entity; > > + remote = link->source; > > + > > + subdev = media_entity_to_v4l2_subdev(remote->entity); > > + fmt.pad = remote->index; > > + ret = v4l2_subdev_call(subdev, pad, get_fmt, NULL, &fmt); > > + if (ret < 0) > > + return ret == -ENOIOCTLCMD ? -EINVAL : ret; > > + > > + if (!rzg2l_cru_ip_code_to_fmt(fmt.format.code)) > > + return -EPIPE; > > Here you should check that the format on the subdev matches the format > on the video device. > OK, I'll use the return value from rzg2l_cru_ip_code_to_fmt() and match it with the video device node. > > + > > + switch (fmt.format.field) { > > + case V4L2_FIELD_TOP: > > + case V4L2_FIELD_BOTTOM: > > + case V4L2_FIELD_NONE: > > + case V4L2_FIELD_INTERLACED_TB: > > + case V4L2_FIELD_INTERLACED_BT: > > + case V4L2_FIELD_INTERLACED: > > + case V4L2_FIELD_SEQ_TB: > > + case V4L2_FIELD_SEQ_BT: > > + break; > > + default: > > + return -EPIPE; > > + } > > Instead of checking the field here, shouldn't it be forced to a valid > value in the subdev .set_fmt() function ? The link validation handler is > responsible for validating that the configuration of the two sides of > the link (IP subdev and video device) match. The driver shouldn't allow > setting formats that can't be supported. > Agreed, this is already taken care of in .set_fmt(), so I'll drop this check here. > What you should check here is that the field of the subdev and the > field of the video device match. > OK, I'll add a check for this. > > + > > + cru = container_of(media_entity_to_video_device(entity), > > You can drop the local entity variable and write > OK. Cheers, Prabhakar
diff --git a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c index ceb9012c9d70..c6c82b9b130a 100644 --- a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c +++ b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c @@ -189,46 +189,6 @@ static void rzg2l_cru_buffer_queue(struct vb2_buffer *vb) spin_unlock_irqrestore(&cru->qlock, flags); } -static int rzg2l_cru_mc_validate_format(struct rzg2l_cru_dev *cru, - struct v4l2_subdev *sd, - struct media_pad *pad) -{ - struct v4l2_subdev_format fmt = { - .which = V4L2_SUBDEV_FORMAT_ACTIVE, - }; - - fmt.pad = pad->index; - if (v4l2_subdev_call_state_active(sd, pad, get_fmt, &fmt)) - return -EPIPE; - - switch (fmt.format.code) { - case MEDIA_BUS_FMT_UYVY8_1X16: - break; - default: - return -EPIPE; - } - - switch (fmt.format.field) { - case V4L2_FIELD_TOP: - case V4L2_FIELD_BOTTOM: - case V4L2_FIELD_NONE: - case V4L2_FIELD_INTERLACED_TB: - case V4L2_FIELD_INTERLACED_BT: - case V4L2_FIELD_INTERLACED: - case V4L2_FIELD_SEQ_TB: - case V4L2_FIELD_SEQ_BT: - break; - default: - return -EPIPE; - } - - if (fmt.format.width != cru->format.width || - fmt.format.height != cru->format.height) - return -EPIPE; - - return 0; -} - static void rzg2l_cru_set_slot_addr(struct rzg2l_cru_dev *cru, int slot, dma_addr_t addr) { @@ -531,10 +491,6 @@ static int rzg2l_cru_set_stream(struct rzg2l_cru_dev *cru, int on) return stream_off_ret; } - ret = rzg2l_cru_mc_validate_format(cru, sd, pad); - if (ret) - return ret; - pipe = media_entity_pipeline(&sd->entity) ? : &cru->vdev.pipe; ret = video_device_pipeline_start(&cru->vdev, pipe); if (ret) @@ -995,6 +951,60 @@ static const struct v4l2_file_operations rzg2l_cru_fops = { .read = vb2_fop_read, }; +/* ----------------------------------------------------------------------------- + * Media entity operations + */ + +static int rzg2l_cru_video_link_validate(struct media_link *link) +{ + struct v4l2_subdev_format fmt = { + .which = V4L2_SUBDEV_FORMAT_ACTIVE, + }; + struct v4l2_subdev *subdev; + struct media_entity *entity; + struct rzg2l_cru_dev *cru; + struct media_pad *remote; + int ret; + + entity = link->sink->entity; + remote = link->source; + + subdev = media_entity_to_v4l2_subdev(remote->entity); + fmt.pad = remote->index; + ret = v4l2_subdev_call(subdev, pad, get_fmt, NULL, &fmt); + if (ret < 0) + return ret == -ENOIOCTLCMD ? -EINVAL : ret; + + if (!rzg2l_cru_ip_code_to_fmt(fmt.format.code)) + return -EPIPE; + + switch (fmt.format.field) { + case V4L2_FIELD_TOP: + case V4L2_FIELD_BOTTOM: + case V4L2_FIELD_NONE: + case V4L2_FIELD_INTERLACED_TB: + case V4L2_FIELD_INTERLACED_BT: + case V4L2_FIELD_INTERLACED: + case V4L2_FIELD_SEQ_TB: + case V4L2_FIELD_SEQ_BT: + break; + default: + return -EPIPE; + } + + cru = container_of(media_entity_to_video_device(entity), + struct rzg2l_cru_dev, vdev); + if (fmt.format.width != cru->format.width || + fmt.format.height != cru->format.height) + return -EPIPE; + + return 0; +} + +static const struct media_entity_operations rzg2l_cru_video_media_ops = { + .link_validate = rzg2l_cru_video_link_validate, +}; + static void rzg2l_cru_v4l2_init(struct rzg2l_cru_dev *cru) { struct video_device *vdev = &cru->vdev; @@ -1006,6 +1016,7 @@ static void rzg2l_cru_v4l2_init(struct rzg2l_cru_dev *cru) vdev->lock = &cru->lock; vdev->device_caps = V4L2_CAP_VIDEO_CAPTURE | V4L2_CAP_STREAMING; vdev->device_caps |= V4L2_CAP_IO_MC; + vdev->entity.ops = &rzg2l_cru_video_media_ops; vdev->fops = &rzg2l_cru_fops; vdev->ioctl_ops = &rzg2l_cru_ioctl_ops;