Re: [PATCH v2 33/34] staging: bcm2835-isp: Add support for BC2835 ISP

[Hans Verkuil <hverkuil@xs4all.nl>]

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, &param_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