On 18/05/2020 16:36, Dave Stevenson wrote:
> Hi Hans.
>
> Thanks for the review.
> A few of these I need to leave for Naush to answer.
> Ideas On Board have been contracted to drive this upstreaming process,
> so many of these will be left to them to address and generate a v2.
<snip>
>>> +static int bcm2835_isp_node_s_selection(struct file *file, void *fh,
>>> + struct v4l2_selection *s)
>>> +{
>>> + struct mmal_parameter_crop crop;
>>> + struct bcm2835_isp_node *node = video_drvdata(file);
>>> + struct bcm2835_isp_dev *dev = node_get_dev(node);
>>> + struct vchiq_mmal_port *port = get_port_data(node);
>>> +
>>> + /* This return value is required fro V4L2 compliance. */
>>> + if (node_is_stats(node))
>>> + return -ENOTTY;
>>> +
>>> + if (!s->r.width || !s->r.height)
>>> + return -EINVAL;
>>
>> I'm missing a check for s->target.
>
> Ack
>
>>> +
>>> + /* Adjust the crop window if goes outside the frame dimensions. */
>>> + s->r.left = min((unsigned int)max(s->r.left, 0),
>>> + node->q_data.width - MIN_DIM);
>>> + s->r.top = min((unsigned int)max(s->r.top, 0),
>>> + node->q_data.height - MIN_DIM);
>>> + s->r.width = max(min(s->r.width, node->q_data.width - s->r.left),
>>> + MIN_DIM);
>>> + s->r.height = max(min(s->r.height, node->q_data.height - s->r.top),
>>> + MIN_DIM);
>>> +
>>> + crop.rect.x = s->r.left;
>>> + crop.rect.y = s->r.top;
>>> + crop.rect.width = s->r.width;
>>> + crop.rect.height = s->r.height;
>>> +
>>> + return vchiq_mmal_port_parameter_set(dev->mmal_instance, port,
>>> + MMAL_PARAMETER_CROP,
>>> + &crop, sizeof(crop));
>>> +}
>>> +
>>> +static int bcm2835_isp_node_g_selection(struct file *file, void *fh,
>>> + struct v4l2_selection *s)
>>> +{
>>> + struct bcm2835_isp_node *node = video_drvdata(file);
>>> + struct bcm2835_isp_dev *dev = node_get_dev(node);
>>> + struct vchiq_mmal_port *port = get_port_data(node);
>>> + struct mmal_parameter_crop crop;
>>> + u32 crop_size = sizeof(crop);
>>> + int ret;
>>> +
>>> + /* This return value is required for V4L2 compliance. */
>>> + if (node_is_stats(node))
>>> + return -ENOTTY;
>>> +
>>> + /* We can only return out an input crop. */
>>> + if (s->target != V4L2_SEL_TGT_CROP)
>>> + return -EINVAL;
>>
>> No support for _TGT_CROP_DEFAULT/BOUNDS? Those are usually supported
>> by drivers and are typically set to the width/height of the current
>> format.
>>
>> I recommend adding support for these targets.
>
> Trying to find any good M2M drivers to use as an example was tricky,
> but if the return value can be as simple as the current width/height,
> then that's easy.
>
>>> +
>>> + ret = vchiq_mmal_port_parameter_get(dev->mmal_instance, port,
>>> + MMAL_PARAMETER_CROP,
>>> + &crop, &crop_size);
>>> + if (!ret)
>>> + return -EINVAL;
>>> +
>>> + s->r.left = crop.rect.x;
>>> + s->r.top = crop.rect.y;
>>> + s->r.width = crop.rect.width;
>>> + s->r.height = crop.rect.height;
>>> +
>>> + return 0;
>>> +}
>>> +
>>> +static int bcm3285_isp_subscribe_event(struct v4l2_fh *fh,
>>> + const struct v4l2_event_subscription *s)
>>> +{
>>> + switch (s->type) {
>>> + /* Cannot change source parameters dynamically at runtime. */
>>> + case V4L2_EVENT_SOURCE_CHANGE:
>>> + return -EINVAL;
>>> + case V4L2_EVENT_CTRL:
>>> + return v4l2_ctrl_subscribe_event(fh, s);
>>> + default:
>>> + return v4l2_event_subscribe(fh, s, 4, NULL);
>>> + }
>>> +}
>>> +
>>> +static const struct v4l2_ioctl_ops bcm2835_isp_node_ioctl_ops = {
>>> + .vidioc_querycap = bcm2835_isp_node_querycap,
>>> + .vidioc_g_fmt_vid_cap = bcm2835_isp_node_g_fmt,
>>> + .vidioc_g_fmt_vid_out = bcm2835_isp_node_g_fmt,
>>> + .vidioc_g_fmt_meta_cap = bcm2835_isp_node_g_fmt,
>>> + .vidioc_s_fmt_vid_cap = bcm2835_isp_node_s_fmt,
>>> + .vidioc_s_fmt_vid_out = bcm2835_isp_node_s_fmt,
>>> + .vidioc_s_fmt_meta_cap = bcm2835_isp_node_s_fmt,
>>> + .vidioc_try_fmt_vid_cap = bcm2835_isp_node_try_fmt,
>>> + .vidioc_try_fmt_vid_out = bcm2835_isp_node_try_fmt,
>>> + .vidioc_try_fmt_meta_cap = bcm2835_isp_node_try_fmt,
>>> + .vidioc_s_selection = bcm2835_isp_node_s_selection,
>>> + .vidioc_g_selection = bcm2835_isp_node_g_selection,
>>> +
>>> + .vidioc_enum_fmt_vid_cap = bcm2835_isp_node_enum_fmt,
>>> + .vidioc_enum_fmt_vid_out = bcm2835_isp_node_enum_fmt,
>>> + .vidioc_enum_fmt_meta_cap = bcm2835_isp_node_enum_fmt,
>>> + .vidioc_enum_framesizes = bcm2835_isp_enum_framesizes,
>>> +
>>> + .vidioc_reqbufs = vb2_ioctl_reqbufs,
>>> + .vidioc_querybuf = vb2_ioctl_querybuf,
>>> + .vidioc_qbuf = vb2_ioctl_qbuf,
>>> + .vidioc_dqbuf = vb2_ioctl_dqbuf,
>>> + .vidioc_expbuf = vb2_ioctl_expbuf,
>>> + .vidioc_create_bufs = vb2_ioctl_create_bufs,
>>> + .vidioc_prepare_buf = vb2_ioctl_prepare_buf,
>>> +
>>> + .vidioc_streamon = vb2_ioctl_streamon,
>>> + .vidioc_streamoff = vb2_ioctl_streamoff,
>>> +
>>> + .vidioc_subscribe_event = bcm3285_isp_subscribe_event,
>>> + .vidioc_unsubscribe_event = v4l2_event_unsubscribe,
>>> +};
>>> +
>>> +/*
>>> + * Size of the array to provide to the VPU when asking for the list of supported
>>> + * formats.
>>> + *
>>> + * The ISP component currently advertises 33 input formats, so add a small
>>> + * overhead on that.
>>> + */
>>> +#define MAX_SUPPORTED_ENCODINGS 40
>>> +
>>> +/* Populate node->supported_fmts with the formats supported by those ports. */
>>> +static int bcm2835_isp_get_supported_fmts(struct bcm2835_isp_node *node)
>>> +{
>>> + struct bcm2835_isp_dev *dev = node_get_dev(node);
>>> + struct bcm2835_isp_fmt const **list;
>>> + unsigned int i, j, num_encodings;
>>> + u32 fourccs[MAX_SUPPORTED_ENCODINGS];
>>> + u32 param_size = sizeof(fourccs);
>>> + int ret;
>>> +
>>> + ret = vchiq_mmal_port_parameter_get(dev->mmal_instance,
>>> + get_port_data(node),
>>> + MMAL_PARAMETER_SUPPORTED_ENCODINGS,
>>> + &fourccs, ¶m_size);
>>> +
>>> + if (ret) {
>>> + if (ret == MMAL_MSG_STATUS_ENOSPC) {
>>> + v4l2_err(&dev->v4l2_dev,
>>> + "%s: port has more encoding than we provided space for. Some are dropped.\n",
>>> + __func__);
>>> + num_encodings = MAX_SUPPORTED_ENCODINGS;
>>> + } else {
>>> + v4l2_err(&dev->v4l2_dev, "%s: get_param ret %u.\n",
>>> + __func__, ret);
>>> + return -EINVAL;
>>> + }
>>> + } else {
>>> + num_encodings = param_size / sizeof(u32);
>>> + }
>>> +
>>> + /*
>>> + * Assume at this stage that all encodings will be supported in V4L2.
>>> + * Any that aren't supported will waste a very small amount of memory.
>>> + */
>>> + list = devm_kzalloc(dev->dev,
>>> + sizeof(struct bcm2835_isp_fmt *) * num_encodings,
>>> + GFP_KERNEL);
>>> + if (!list)
>>> + return -ENOMEM;
>>> + node->supported_fmts.list = list;
>>> +
>>> + for (i = 0, j = 0; i < num_encodings; i++) {
>>> + const struct bcm2835_isp_fmt *fmt = get_fmt(fourccs[i]);
>>> +
>>> + if (fmt) {
>>> + list[j] = fmt;
>>> + j++;
>>> + }
>>> + }
>>> + node->supported_fmts.num_entries = j;
>>> +
>>> + return 0;
>>> +}
>>> +
>>> +/*
>>> + * Register a device node /dev/video<N> to go along with one of the ISP's input
>>> + * or output nodes.
>>> + */
>>> +static int register_node(struct bcm2835_isp_dev *dev,
>>> + struct bcm2835_isp_node *node,
>>> + int index)
>>> +{
>>> + struct video_device *vfd;
>>> + struct vb2_queue *queue;
>>> + int ret;
>>> +
>>> + mutex_init(&node->lock);
>>> +
>>> + node->dev = dev;
>>> + vfd = &node->vfd;
>>> + queue = &node->queue;
>>> + queue->type = index_to_queue_type(index);
>>> + /*
>>> + * Setup the node type-specific params.
>>> + *
>>> + * Only the OUTPUT node can set controls and crop windows. However,
>>> + * we must allow the s/g_selection ioctl on the stats node as v4l2
>>> + * compliance expects it to return a -ENOTTY, and the framework
>>> + * does not handle it if the ioctl is disabled.
>>> + */
>>> + switch (queue->type) {
>>> + case V4L2_BUF_TYPE_VIDEO_OUTPUT:
>>> + vfd->device_caps = V4L2_CAP_VIDEO_OUTPUT | V4L2_CAP_STREAMING;
>>> + node->id = index;
>>> + node->vfl_dir = VFL_DIR_TX;
>>> + node->name = "output";
>>> + break;
>>> + case V4L2_BUF_TYPE_VIDEO_CAPTURE:
>>> + vfd->device_caps = V4L2_CAP_VIDEO_CAPTURE | V4L2_CAP_STREAMING;
>>> + /* First Capture node starts at id 0, etc. */
>>> + node->id = index - BCM2835_ISP_NUM_OUTPUTS;
>>> + node->vfl_dir = VFL_DIR_RX;
>>> + node->name = "capture";
>>> + v4l2_disable_ioctl(&node->vfd, VIDIOC_S_CTRL);
>>> + v4l2_disable_ioctl(&node->vfd, VIDIOC_S_SELECTION);
>>> + v4l2_disable_ioctl(&node->vfd, VIDIOC_G_SELECTION);
>>> + break;
>>> + case V4L2_BUF_TYPE_META_CAPTURE:
>>> + vfd->device_caps = V4L2_CAP_META_CAPTURE | V4L2_CAP_STREAMING;
>>> + node->id = index - BCM2835_ISP_NUM_OUTPUTS;
>>> + node->vfl_dir = VFL_DIR_RX;
>>> + node->name = "stats";
>>> + v4l2_disable_ioctl(&node->vfd, VIDIOC_S_CTRL);
>>
>> Why not disable S/G_SELECTION for meta capture here rather than test for it
>> in the op functions?
>
> I'd have to ask Naush. There were some very odd interactions with
> v4l2-compliance that he was struggling with.
>
>>> + break;
>>> + }
>>> +
>>> + /* We use the selection API instead of the old crop API. */
>>> + v4l2_disable_ioctl(vfd, VIDIOC_CROPCAP);
>>> + v4l2_disable_ioctl(vfd, VIDIOC_G_CROP);
>>> + v4l2_disable_ioctl(vfd, VIDIOC_S_CROP);
>>
>> No need for this: the core handles this and will disable these ioctls
>> automatically.
>
> The core will ENABLE these if g_selection and s_selection are defined,
> and uses mapping functions to try and convert between the two APIs.
> This may be down to missing the target check in s_selection, but again
> I seem to recall v4l2-compliance was reporting failures on these
> ioctls.
I suspect that the missing target check in g/s_selection is the cause of
the v4l2-compliance failures.
If there are still compliance issues after adding that check, then just
reach out to me for help.
Regards,
Hans
