From: Hans Verkuil <hverkuil@xs4all.nl>
To: Dave Stevenson <dave.stevenson@raspberrypi.com>
Cc: "Laurent Pinchart" <laurent.pinchart@ideasonboard.com>,
"Linux Media Mailing List" <linux-media@vger.kernel.org>,
"Kieran Bingham" <kieran.bingham@ideasonboard.com>,
"Jacopo Mondi" <jacopo@jmondi.org>,
"Niklas Söderlund" <niklas.soderlund@ragnatech.se>,
"Naushir Patuck" <naush@raspberrypi.com>
Subject: Re: [PATCH v2 33/34] staging: bcm2835-isp: Add support for BC2835 ISP
Date: Mon, 18 May 2020 17:07:20 +0200 [thread overview]
Message-ID: <bdcb471b-e066-373f-aa16-b7caf5c52795@xs4all.nl> (raw)
In-Reply-To: <CAPY8ntBDss5ZKPo3mRUP0-9KNHA6kQEQwrnvyUpOh0Ru7O5CKA@mail.gmail.com>
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
next prev parent reply other threads:[~2020-05-18 15:07 UTC|newest]
Thread overview: 104+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-05-04 9:25 [PATCH v2 00/34] Drivers for the BCM283x CSI-2/CCP2 receiver and ISP Laurent Pinchart
2020-05-04 9:25 ` [PATCH v2 01/34] media: uapi: v4l2-core: Add sensor ancillary data V4L2 fourcc type Laurent Pinchart
2020-05-04 13:48 ` Hans Verkuil
2020-05-04 14:39 ` Dave Stevenson
2020-05-04 15:32 ` Hans Verkuil
2020-05-04 16:08 ` Laurent Pinchart
2020-05-05 11:20 ` Dave Stevenson
2020-05-04 9:25 ` [PATCH v2 02/34] media: uapi: Add MEDIA_BUS_FMT_SENSOR_DATA media bus format Laurent Pinchart
2020-05-04 9:25 ` [PATCH v2 03/34] dt-bindings: media: Document BCM283x CSI2/CCP2 receiver Laurent Pinchart
2020-05-04 9:25 ` [PATCH v2 04/34] media: bcm2835-unicam: Driver for CCP2/CSI2 camera interface Laurent Pinchart
2020-05-04 15:21 ` Hans Verkuil
2020-05-05 1:26 ` kbuild test robot
2020-05-06 18:01 ` Nicolas Saenz Julienne
2020-08-29 11:20 ` Jacopo Mondi
2020-08-29 18:32 ` Laurent Pinchart
2020-08-31 7:38 ` Jacopo Mondi
2020-08-31 14:17 ` Laurent Pinchart
2020-08-31 14:46 ` Jacopo Mondi
2020-08-31 14:56 ` Laurent Pinchart
2020-09-01 8:41 ` Dave Stevenson
2020-09-01 10:22 ` Jacopo Mondi
2020-09-01 16:37 ` Dave Stevenson
2020-09-01 17:11 ` Laurent Pinchart
2020-09-15 7:03 ` Sakari Ailus
2020-09-15 9:32 ` Laurent Pinchart
2020-09-15 13:28 ` Dave Stevenson
2020-10-30 17:53 ` Jacopo Mondi
2020-09-15 17:30 ` Dave Stevenson
2020-05-04 9:25 ` [PATCH v2 05/34] ARM: dts: bcm2711: Add Unicam DT nodes Laurent Pinchart
2020-05-04 9:25 ` [PATCH v2 06/34] staging: vc04_services: Add new vc-sm-cma driver Laurent Pinchart
2020-05-05 2:38 ` kbuild test robot
2020-05-05 12:17 ` kbuild test robot
2020-05-06 3:05 ` kbuild test robot
2020-05-06 18:04 ` Nicolas Saenz Julienne
2020-05-06 19:24 ` Dave Stevenson
2020-05-08 0:11 ` Laurent Pinchart
2020-05-18 12:06 ` Hans Verkuil
2020-08-24 16:39 ` Jacopo Mondi
2020-08-25 17:52 ` Dave Stevenson
2020-08-27 10:38 ` Jacopo Mondi
2020-08-27 12:51 ` Dave Stevenson
2020-08-27 16:46 ` Jacopo Mondi
2020-08-27 17:19 ` Dave Stevenson
2020-05-11 18:42 ` Nicolas Saenz Julienne
2020-05-18 15:48 ` Dave Stevenson
2020-05-20 14:41 ` Nicolas Saenz Julienne
2020-05-21 11:01 ` Dave Stevenson
2020-05-04 9:25 ` [PATCH v2 07/34] staging: bcm2835: Break MMAL support out from camera Laurent Pinchart
2020-05-04 9:25 ` [PATCH v2 08/34] staging: mmal-vchiq: Allocate and free components as required Laurent Pinchart
2020-05-04 9:25 ` [PATCH v2 09/34] staging: mmal-vchiq: Avoid use of bool in structures Laurent Pinchart
2020-05-04 9:25 ` [PATCH v2 10/34] staging: mmal-vchiq: Make timeout a defined parameter Laurent Pinchart
2020-05-04 9:25 ` [PATCH v2 11/34] staging: mmal-vchiq: Make a mmal_buf struct for passing parameters Laurent Pinchart
2020-05-04 9:25 ` [PATCH v2 12/34] staging: mmal-vchiq: Add support for event callbacks Laurent Pinchart
2020-05-04 9:25 ` [PATCH v2 13/34] staging: mmal-vchiq: Support sending data to MMAL ports Laurent Pinchart
2020-05-04 9:25 ` [PATCH v2 14/34] staging: mmal-vchiq: Fixup vchiq-mmal include ordering Laurent Pinchart
2020-05-04 9:25 ` [PATCH v2 15/34] staging: mmal-vchiq: Use vc-sm-cma to support zero copy Laurent Pinchart
2020-05-04 16:30 ` Nicolas Saenz Julienne
2020-05-05 16:07 ` kbuild test robot
2020-05-11 19:15 ` Nicolas Saenz Julienne
2020-05-04 9:25 ` [PATCH v2 16/34] staging: mmal-vchiq: Fix client_component for 64 bit kernel Laurent Pinchart
2020-05-18 10:19 ` Hans Verkuil
2020-05-04 9:25 ` [PATCH v2 17/34] staging: mmal_vchiq: Add in the Bayer encoding formats Laurent Pinchart
2020-05-04 9:25 ` [PATCH v2 18/34] staging: mmal-vchiq: Always return the param size from param_get Laurent Pinchart
2020-05-04 9:25 ` [PATCH v2 19/34] staging: mmal-vchiq: If the VPU returns an error, don't negate it Laurent Pinchart
2020-05-04 9:25 ` [PATCH v2 20/34] staging: mmal-vchiq: Fix handling of VB2_MEMORY_DMABUF buffers Laurent Pinchart
2020-05-04 9:25 ` [PATCH v2 21/34] staging: mmal-vchiq: Update mmal_parameters.h with recently defined params Laurent Pinchart
2020-05-04 9:25 ` [PATCH v2 22/34] staging: mmal-vchiq: Free the event context for control ports Laurent Pinchart
2020-05-04 9:26 ` [PATCH v2 23/34] staging: mmal-vchiq: Fix memory leak in error path Laurent Pinchart
2020-05-04 9:26 ` [PATCH v2 24/34] staging: mmal-vchiq: Fix formatting errors in mmal_parameters.h Laurent Pinchart
2020-05-04 9:26 ` [PATCH v2 25/34] staging: vchiq_arm: Register vcsm-cma as a platform driver Laurent Pinchart
2020-05-04 9:26 ` [PATCH v2 26/34] staging: vchiq_arm: Set up dma ranges on child devices Laurent Pinchart
2020-05-04 16:54 ` Nicolas Saenz Julienne
2020-08-25 16:57 ` Jacopo Mondi
2020-05-04 9:26 ` [PATCH v2 27/34] staging: vchiq: Use the old dma controller for OF config on platform devices Laurent Pinchart
2020-05-04 15:44 ` Nicolas Saenz Julienne
2020-05-04 9:26 ` [PATCH v2 28/34] staging: vchiq_2835_arm: Implement a DMA pool for small bulk transfers Laurent Pinchart
2020-05-04 9:26 ` [PATCH v2 29/34] staging: vchiq: Add 36-bit address support Laurent Pinchart
2020-05-04 17:40 ` Nicolas Saenz Julienne
2020-05-04 20:46 ` Phil Elwell
2020-05-05 10:13 ` Nicolas Saenz Julienne
2020-05-05 10:57 ` Phil Elwell
2020-05-05 13:22 ` kbuild test robot
2020-05-04 9:26 ` [PATCH v2 30/34] staging: vchiq_arm: Give vchiq children DT nodes Laurent Pinchart
2020-05-04 17:12 ` Nicolas Saenz Julienne
2020-05-04 19:42 ` Phil Elwell
2020-05-05 10:37 ` Nicolas Saenz Julienne
2020-05-05 10:50 ` Phil Elwell
2020-05-04 9:26 ` [PATCH v2 31/34] staging: vchiq_arm: Add a matching unregister call Laurent Pinchart
2020-05-04 9:26 ` [PATCH v2 32/34] media: videobuf2: Allow exporting of a struct dmabuf Laurent Pinchart
2020-05-04 13:36 ` Hans Verkuil
2020-05-04 9:26 ` [PATCH v2 33/34] staging: bcm2835-isp: Add support for BC2835 ISP Laurent Pinchart
2020-05-11 19:19 ` Nicolas Saenz Julienne
2020-05-18 13:38 ` Dave Stevenson
2020-05-20 13:46 ` Nicolas Saenz Julienne
2020-05-18 12:02 ` Hans Verkuil
2020-05-18 14:36 ` Dave Stevenson
2020-05-18 15:07 ` Hans Verkuil [this message]
2020-05-19 12:47 ` Naushir Patuck
2020-05-19 13:03 ` Jacopo Mondi
2020-06-24 11:28 ` Hans Verkuil
2020-05-04 9:26 ` [PATCH v2 34/34] staging: vchiq: Load bcm2835_isp driver from vchiq Laurent Pinchart
2020-05-04 15:15 ` [PATCH v2 00/34] Drivers for the BCM283x CSI-2/CCP2 receiver and ISP Nicolas Saenz Julienne
2020-05-04 15:38 ` Laurent Pinchart
2020-05-04 16:15 ` Nicolas Saenz Julienne
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=bdcb471b-e066-373f-aa16-b7caf5c52795@xs4all.nl \
--to=hverkuil@xs4all.nl \
--cc=dave.stevenson@raspberrypi.com \
--cc=jacopo@jmondi.org \
--cc=kieran.bingham@ideasonboard.com \
--cc=laurent.pinchart@ideasonboard.com \
--cc=linux-media@vger.kernel.org \
--cc=naush@raspberrypi.com \
--cc=niklas.soderlund@ragnatech.se \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).