From: hverkuil at xs4all.nl (Hans Verkuil)
Subject: [Linux-kernel-mentees] [PATCH v2 2/2] vivid: Add metadata output support
Date: Fri, 4 Oct 2019 11:55:01 +0200 [thread overview]
Message-ID: <787ff1cb-dde3-800a-9dde-68f1db5e4897@xs4all.nl> (raw)
In-Reply-To: <1955ad5e-9359-d7be-5c92-98f652097331@linuxfoundation.org>
On 10/3/19 7:38 PM, Shuah Khan wrote:
> On 10/3/19 5:08 AM, Vandana BN wrote:
>> Support metadata output in vivid driver.
>> Metadata output is used to set brightness, contrast, saturation
>> and hue.
>> Adds new files for metadata output.
>>
>> Signed-off-by: Vandana BN <bnvandana at gmail.com>
>> ---
>> drivers/media/platform/vivid/Makefile | 2 +-
>> drivers/media/platform/vivid/vivid-core.c | 98 +++++++++-
>> drivers/media/platform/vivid/vivid-core.h | 10 +
>> drivers/media/platform/vivid/vivid-ctrls.c | 14 +-
>> .../media/platform/vivid/vivid-kthread-out.c | 50 ++++-
>> drivers/media/platform/vivid/vivid-meta-out.c | 174 ++++++++++++++++++
>> drivers/media/platform/vivid/vivid-meta-out.h | 25 +++
>> drivers/media/platform/vivid/vivid-vid-out.c | 5 +-
>> 8 files changed, 366 insertions(+), 12 deletions(-)
>> create mode 100644 drivers/media/platform/vivid/vivid-meta-out.c
>> create mode 100644 drivers/media/platform/vivid/vivid-meta-out.h
>>
>> diff --git a/drivers/media/platform/vivid/Makefile b/drivers/media/platform/vivid/Makefile
>> index af94abf9bce6..e8a50c506dc9 100644
>> --- a/drivers/media/platform/vivid/Makefile
>> +++ b/drivers/media/platform/vivid/Makefile
>> @@ -3,7 +3,7 @@ vivid-objs := vivid-core.o vivid-ctrls.o vivid-vid-common.o vivid-vbi-gen.o \
>> vivid-vid-cap.o vivid-vid-out.o vivid-kthread-cap.o vivid-kthread-out.o \
>> vivid-radio-rx.o vivid-radio-tx.o vivid-radio-common.o \
>> vivid-rds-gen.o vivid-sdr-cap.o vivid-vbi-cap.o vivid-vbi-out.o \
>> - vivid-osd.o vivid-meta-cap.o
>> + vivid-osd.o vivid-meta-cap.o vivid-meta-out.o
>> ifeq ($(CONFIG_VIDEO_VIVID_CEC),y)
>> vivid-objs += vivid-cec.o
>> endif
>> diff --git a/drivers/media/platform/vivid/vivid-core.c b/drivers/media/platform/vivid/vivid-core.c
>> index 97ab197bdec0..7f08e8ae5446 100644
>> --- a/drivers/media/platform/vivid/vivid-core.c
>> +++ b/drivers/media/platform/vivid/vivid-core.c
>> @@ -38,6 +38,7 @@
>> #include "vivid-cec.h"
>> #include "vivid-ctrls.h"
>> #include "vivid-meta-cap.h"
>> +#include "vivid-meta-out.h"
>>
>> #define VIVID_MODULE_NAME "vivid"
>>
>> @@ -84,6 +85,10 @@ static int meta_cap_nr[VIVID_MAX_DEVS] = { [0 ... (VIVID_MAX_DEVS - 1)] = -1 };
>> module_param_array(meta_cap_nr, int, NULL, 0444);
>> MODULE_PARM_DESC(meta_cap_nr, " videoX start number, -1 is autodetect");
>>
>> +static int meta_out_nr[VIVID_MAX_DEVS] = { [0 ... (VIVID_MAX_DEVS - 1)] = -1 };
>> +module_param_array(meta_out_nr, int, NULL, 0444);
>> +MODULE_PARM_DESC(meta_out_nr, " videoX start number, -1 is autodetect");
>> +
>> static int ccs_cap_mode[VIVID_MAX_DEVS] = { [0 ... (VIVID_MAX_DEVS - 1)] = -1 };
>> module_param_array(ccs_cap_mode, int, NULL, 0444);
>> MODULE_PARM_DESC(ccs_cap_mode, " capture crop/compose/scale mode:\n"
>> @@ -105,10 +110,10 @@ MODULE_PARM_DESC(multiplanar, " 1 (default) creates a single planar device, 2 cr
>> * vbi-out + vid-out + meta-cap
>> */
>> static unsigned int node_types[VIVID_MAX_DEVS] = {
>> - [0 ... (VIVID_MAX_DEVS - 1)] = 0x21d3d
>> + [0 ... (VIVID_MAX_DEVS - 1)] = 0x61d3d
>
> Why not add a define that is human friendly to describe the node type?
This is something for a separate patch and shouldn't be mixed in with this.
It would certainly be useful to have defines for this.
>
>> };
>> module_param_array(node_types, uint, NULL, 0444);
>> -MODULE_PARM_DESC(node_types, " node types, default is 0x1d3d. Bitmask with the following meaning:\n"
>> +MODULE_PARM_DESC(node_types, " node types, default is 0x61d3d. Bitmask with the following meaning:\n"
>> "\t\t bit 0: Video Capture node\n"
>> "\t\t bit 2-3: VBI Capture node: 0 = none, 1 = raw vbi, 2 = sliced vbi, 3 = both\n"
>> "\t\t bit 4: Radio Receiver node\n"
>> @@ -117,7 +122,8 @@ MODULE_PARM_DESC(node_types, " node types, default is 0x1d3d. Bitmask with the f
>> "\t\t bit 10-11: VBI Output node: 0 = none, 1 = raw vbi, 2 = sliced vbi, 3 = both\n"
>> "\t\t bit 12: Radio Transmitter node\n"
>> "\t\t bit 16: Framebuffer for testing overlays\n"
>> - "\t\t bit 17: Metadata Capture node\n");
>> + "\t\t bit 17: Metadata Capture node\n"
>> + "\t\t bit 18: Metadata Output node\n");
>>
>> /* Default: 4 inputs */
>> static unsigned num_inputs[VIVID_MAX_DEVS] = { [0 ... (VIVID_MAX_DEVS - 1)] = 4 };
>> @@ -216,7 +222,8 @@ static int vidioc_querycap(struct file *file, void *priv,
>> cap->capabilities = dev->vid_cap_caps | dev->vid_out_caps |
>> dev->vbi_cap_caps | dev->vbi_out_caps |
>> dev->radio_rx_caps | dev->radio_tx_caps |
>> - dev->sdr_cap_caps | dev->meta_cap_caps | V4L2_CAP_DEVICE_CAPS;
>> + dev->sdr_cap_caps | dev->meta_cap_caps |
>> + dev->meta_out_caps | V4L2_CAP_DEVICE_CAPS;
>> return 0;
>> }
>>
>> @@ -445,7 +452,8 @@ static bool vivid_is_last_user(struct vivid_dev *dev)
>> vivid_is_in_use(&dev->sdr_cap_dev) +
>> vivid_is_in_use(&dev->radio_rx_dev) +
>> vivid_is_in_use(&dev->radio_tx_dev) +
>> - vivid_is_in_use(&dev->meta_cap_dev);
>> + vivid_is_in_use(&dev->meta_cap_dev) +
>> + vivid_is_in_use(&dev->meta_out_dev);
>>
>> return uses == 1;
>> }
>> @@ -472,6 +480,7 @@ static int vivid_fop_release(struct file *file)
>> set_bit(V4L2_FL_REGISTERED, &dev->radio_rx_dev.flags);
>> set_bit(V4L2_FL_REGISTERED, &dev->radio_tx_dev.flags);
>> set_bit(V4L2_FL_REGISTERED, &dev->meta_cap_dev.flags);
>> + set_bit(V4L2_FL_REGISTERED, &dev->meta_out_dev.flags);
>> }
>> mutex_unlock(&dev->mutex);
>> if (file->private_data == dev->overlay_cap_owner)
>> @@ -622,6 +631,11 @@ static const struct v4l2_ioctl_ops vivid_ioctl_ops = {
>> .vidioc_g_fmt_meta_cap = vidioc_g_fmt_meta_cap,
>> .vidioc_s_fmt_meta_cap = vidioc_g_fmt_meta_cap,
>> .vidioc_try_fmt_meta_cap = vidioc_g_fmt_meta_cap,
>> +
>
> Do you need this blank line here?
>
>> + .vidioc_enum_fmt_meta_out = vidioc_enum_fmt_meta_out,
>> + .vidioc_g_fmt_meta_out = vidioc_g_fmt_meta_out,
>> + .vidioc_s_fmt_meta_out = vidioc_g_fmt_meta_out,
>> + .vidioc_try_fmt_meta_out = vidioc_g_fmt_meta_out,
>> };
>>
>> /* -----------------------------------------------------------------
>> @@ -839,6 +853,9 @@ static int vivid_create_instance(struct platform_device *pdev, int inst)
>> /* do we create a meta capture device */
>> dev->has_meta_cap = node_type & 0x20000;
>
> Again same comment on this 0x20000 - please make this a define
> that describes what this is.
>
>>
>> + /* do we create a metadata output device */
>> + dev->has_meta_out = node_type & 0x40000;
>
> Same comment here.
>
>> +
>> /* end detecting feature set */
>>
>> if (dev->has_vid_cap) {
>> @@ -905,6 +922,13 @@ static int vivid_create_instance(struct platform_device *pdev, int inst)
>
> This is a very loooong routine! It is very difficult to figure out
> if the error handling is correct and review is rather hard.
Also out-of-scope of this patch. It can certainly be split up in smaller functions,
but that shouldn't be mixed in with this.
>
>> if (in_type_counter[TV])
>> dev->meta_cap_caps |= V4L2_CAP_TUNER;
>> }
>> + /* set up the capablilities of meta output device*/
>> + if (dev->has_meta_out) {
>> + dev->meta_out_caps = V4L2_CAP_META_OUTPUT |
>> + V4L2_CAP_STREAMING | V4L2_CAP_READWRITE;
>
> Add V4L2_CAP_READWRITE on the next line?
>
>> + if (dev->has_audio_outputs)
>> + dev->meta_out_caps |= V4L2_CAP_AUDIO;
>> + }
>>
>> ret = -ENOMEM;
>> /* initialize the test pattern generator */
>> @@ -976,6 +1000,9 @@ static int vivid_create_instance(struct platform_device *pdev, int inst)
>> v4l2_disable_ioctl(&dev->vbi_out_dev, VIDIOC_S_AUDOUT);
>> v4l2_disable_ioctl(&dev->vbi_out_dev, VIDIOC_G_AUDOUT);
>> v4l2_disable_ioctl(&dev->vbi_out_dev, VIDIOC_ENUMAUDOUT);
>> + v4l2_disable_ioctl(&dev->meta_out_dev, VIDIOC_S_AUDOUT);
>> + v4l2_disable_ioctl(&dev->meta_out_dev, VIDIOC_G_AUDOUT);
>> + v4l2_disable_ioctl(&dev->meta_out_dev, VIDIOC_ENUMAUDOUT);
>> }
>> if (!in_type_counter[TV] && !in_type_counter[SVID]) {
>> v4l2_disable_ioctl(&dev->vid_cap_dev, VIDIOC_S_STD);
>> @@ -1035,6 +1062,8 @@ static int vivid_create_instance(struct platform_device *pdev, int inst)
>> v4l2_disable_ioctl(&dev->vid_out_dev, VIDIOC_ENUM_FRAMEINTERVALS);
>> v4l2_disable_ioctl(&dev->vbi_out_dev, VIDIOC_S_FREQUENCY);
>> v4l2_disable_ioctl(&dev->vbi_out_dev, VIDIOC_G_FREQUENCY);
>> + v4l2_disable_ioctl(&dev->meta_out_dev, VIDIOC_S_FREQUENCY);
>> + v4l2_disable_ioctl(&dev->meta_out_dev, VIDIOC_G_FREQUENCY);
>>
>> /* configure internal data */
>> dev->fmt_cap = &vivid_formats[0];
>> @@ -1118,6 +1147,7 @@ static int vivid_create_instance(struct platform_device *pdev, int inst)
>> INIT_LIST_HEAD(&dev->vbi_out_active);
>> INIT_LIST_HEAD(&dev->sdr_cap_active);
>> INIT_LIST_HEAD(&dev->meta_cap_active);
>> + INIT_LIST_HEAD(&dev->meta_out_active);
>>
>> INIT_LIST_HEAD(&dev->cec_work_list);
>> spin_lock_init(&dev->cec_slock);
>> @@ -1286,6 +1316,27 @@ static int vivid_create_instance(struct platform_device *pdev, int inst)
>> goto unreg_dev;
>> }
>>
>> + if (dev->has_meta_out) {
>> + /* initialize vbi_out queue */
>> + q = &dev->vb_meta_out_q;
>> + q->type = V4L2_BUF_TYPE_META_OUTPUT;
>> + q->io_modes = VB2_MMAP | VB2_DMABUF | VB2_WRITE;
>> + if (!allocator)
>> + q->io_modes |= VB2_USERPTR;
>> + q->drv_priv = dev;
>> + q->buf_struct_size = sizeof(struct vivid_buffer);
>> + q->ops = &vivid_meta_out_qops;
>> + q->mem_ops = vivid_mem_ops[allocator];
>> + q->timestamp_flags = V4L2_BUF_FLAG_TIMESTAMP_MONOTONIC;
>> + q->min_buffers_needed = 2;
>> + q->lock = &dev->mutex;
>> + q->dev = dev->v4l2_dev.dev;
>> + q->supports_requests = true;
>> + ret = vb2_queue_init(q);
> Make sure queue is released in error leg
Not needed, it's only required when you close a filehandle that was streaming.
And that's already done in vb2_fop_release.
>
>> + if (ret)
>> + goto unreg_dev;
>
> I think release is isn't there in unreg_dev handling
>
>> + }
>> +
>> #ifdef CONFIG_VIDEO_VIVID_CEC
>> if (dev->has_vid_cap && in_type_counter[HDMI]) {
>> struct cec_adapter *adap;
>> @@ -1327,6 +1378,7 @@ static int vivid_create_instance(struct platform_device *pdev, int inst)
>> v4l2_ctrl_handler_setup(&dev->ctrl_hdl_radio_tx);
>> v4l2_ctrl_handler_setup(&dev->ctrl_hdl_sdr_cap);
>> v4l2_ctrl_handler_setup(&dev->ctrl_hdl_meta_cap);
>> + v4l2_ctrl_handler_setup(&dev->ctrl_hdl_meta_out);/* is this needed */
>
> Tab between the routine and comment
>
>>
>> /* finally start creating the device nodes */
>> if (dev->has_vid_cap) {
>> @@ -1583,6 +1635,36 @@ static int vivid_create_instance(struct platform_device *pdev, int inst)
>> video_device_node_name(vfd));
>> }
>>
>> + if (dev->has_meta_out) {
>> + vfd = &dev->meta_out_dev;
>> + snprintf(vfd->name, sizeof(vfd->name),
>> + "vivid-%03d-meta-out", inst);
>> + vfd->vfl_dir = VFL_DIR_TX;
>> + vfd->fops = &vivid_fops;
>> + vfd->ioctl_ops = &vivid_ioctl_ops;
>> + vfd->device_caps = dev->meta_out_caps;
>> + vfd->release = video_device_release_empty;
>> + vfd->v4l2_dev = &dev->v4l2_dev;
>> + vfd->queue = &dev->vb_meta_out_q;
>> + vfd->lock = &dev->mutex;
>> + vfd->tvnorms = tvnorms_out;
>> + video_set_drvdata(vfd, dev);
>> +#ifdef CONFIG_MEDIA_CONTROLLER
>> + dev->meta_out_pad.flags = MEDIA_PAD_FL_SOURCE;
>> + ret = media_entity_pads_init(&vfd->entity, 1,
>> + &dev->meta_out_pad);
>> + if (ret)
>> + goto unreg_dev;
>
> Where do we do cleanup of pads if something fails? I didn't see it.
Also out-of-scope of this patch. Vivid never calls media_entity_cleanup().
It should, though. Even though media_entity_cleanup() is currently a nop.
Should be added in a separate patch.
>
>> +#endif
>> + ret = video_register_device(vfd, VFL_TYPE_GRABBER,
>> + meta_out_nr[inst]);
>> + if (ret < 0)
>> + goto unreg_dev;
>> + v4l2_info(&dev->v4l2_dev,
>> + "V4L2 metadata output device registered as %s\n",
>> + video_device_node_name(vfd));
>> + }
>> +
>> #ifdef CONFIG_MEDIA_CONTROLLER
>> /* Register the media device */
>> ret = media_device_register(&dev->mdev);
>> @@ -1599,6 +1681,7 @@ static int vivid_create_instance(struct platform_device *pdev, int inst)
>> return 0;
>>
>> unreg_dev:
>> + video_unregister_device(&dev->meta_out_dev);
>> video_unregister_device(&dev->meta_cap_dev);
>> video_unregister_device(&dev->radio_tx_dev);
>> video_unregister_device(&dev->radio_rx_dev);
>> @@ -1721,6 +1804,11 @@ static int vivid_remove(struct platform_device *pdev)
>> video_device_node_name(&dev->meta_cap_dev));
>> video_unregister_device(&dev->meta_cap_dev);
>> }
>> + if (dev->has_meta_out) {
>> + v4l2_info(&dev->v4l2_dev, "unregistering %s\n",
>> + video_device_node_name(&dev->meta_out_dev));
>> + video_unregister_device(&dev->meta_out_dev);
>> + }
>> cec_unregister_adapter(dev->cec_rx_adap);
>> for (j = 0; j < MAX_OUTPUTS; j++)
>> cec_unregister_adapter(dev->cec_tx_adap[j]);
>> diff --git a/drivers/media/platform/vivid/vivid-core.h b/drivers/media/platform/vivid/vivid-core.h
>> index fd601345a17c..d57066ed31f0 100644
>> --- a/drivers/media/platform/vivid/vivid-core.h
>> +++ b/drivers/media/platform/vivid/vivid-core.h
>> @@ -132,6 +132,7 @@ struct vivid_dev {
>> struct media_pad vbi_out_pad;
>> struct media_pad sdr_cap_pad;
>> struct media_pad meta_cap_pad;
>> + struct media_pad meta_out_pad;
>> #endif
>> struct v4l2_ctrl_handler ctrl_hdl_user_gen;
>> struct v4l2_ctrl_handler ctrl_hdl_user_vid;
>> @@ -156,6 +157,8 @@ struct vivid_dev {
>> struct v4l2_ctrl_handler ctrl_hdl_sdr_cap;
>> struct video_device meta_cap_dev;
>> struct v4l2_ctrl_handler ctrl_hdl_meta_cap;
>> + struct video_device meta_out_dev;
>> + struct v4l2_ctrl_handler ctrl_hdl_meta_out;
>>
>> spinlock_t slock;
>> struct mutex mutex;
>> @@ -169,6 +172,7 @@ struct vivid_dev {
>> u32 radio_rx_caps;
>> u32 radio_tx_caps;
>> u32 meta_cap_caps;
>> + u32 meta_out_caps;
>>
>> /* supported features */
>> bool multiplanar;
>> @@ -195,6 +199,7 @@ struct vivid_dev {
>> bool has_sdr_cap;
>> bool has_fb;
>> bool has_meta_cap;
>> + bool has_meta_out;
>>
>> bool can_loop_video;
>>
>> @@ -432,6 +437,8 @@ struct vivid_dev {
>> struct list_head vid_out_active;
>> struct vb2_queue vb_vbi_out_q;
>> struct list_head vbi_out_active;
>> + struct vb2_queue vb_meta_out_q;
>> + struct list_head meta_out_active;
>>
>> /* video loop precalculated rectangles */
>>
>> @@ -472,6 +479,9 @@ struct vivid_dev {
>> u32 vbi_out_seq_count;
>> bool vbi_out_streaming;
>> bool stream_sliced_vbi_out;
>> + u32 meta_out_seq_start;
>> + u32 meta_out_seq_count;
>> + bool meta_out_streaming;
>>
>> /* SDR capture */
>> struct vb2_queue vb_sdr_cap_q;
>> diff --git a/drivers/media/platform/vivid/vivid-ctrls.c b/drivers/media/platform/vivid/vivid-ctrls.c
>> index 2ab6a680485c..a7d42e139a4f 100644
>> --- a/drivers/media/platform/vivid/vivid-ctrls.c
>> +++ b/drivers/media/platform/vivid/vivid-ctrls.c
>> @@ -114,7 +114,7 @@ static int vivid_user_gen_s_ctrl(struct v4l2_ctrl *ctrl)
>> clear_bit(V4L2_FL_REGISTERED, &dev->radio_rx_dev.flags);
>> clear_bit(V4L2_FL_REGISTERED, &dev->radio_tx_dev.flags);
>> clear_bit(V4L2_FL_REGISTERED, &dev->meta_cap_dev.flags);
>> - clear_bit(V4L2_FL_REGISTERED, &dev->meta_cap_dev.flags);
>> + clear_bit(V4L2_FL_REGISTERED, &dev->meta_out_dev.flags);
>> break;
>> case VIVID_CID_BUTTON:
>> dev->button_pressed = 30;
>> @@ -1493,6 +1493,7 @@ int vivid_create_controls(struct vivid_dev *dev, bool show_ccs_cap,
>> struct v4l2_ctrl_handler *hdl_radio_tx = &dev->ctrl_hdl_radio_tx;
>> struct v4l2_ctrl_handler *hdl_sdr_cap = &dev->ctrl_hdl_sdr_cap;
>> struct v4l2_ctrl_handler *hdl_meta_cap = &dev->ctrl_hdl_meta_cap;
>> + struct v4l2_ctrl_handler *hdl_meta_out = &dev->ctrl_hdl_meta_out;
>>
>> struct v4l2_ctrl_config vivid_ctrl_dv_timings = {
>> .ops = &vivid_vid_cap_ctrl_ops,
>> @@ -1534,6 +1535,8 @@ int vivid_create_controls(struct vivid_dev *dev, bool show_ccs_cap,
>> v4l2_ctrl_new_custom(hdl_sdr_cap, &vivid_ctrl_class, NULL);
>> v4l2_ctrl_handler_init(hdl_meta_cap, 2);
>> v4l2_ctrl_new_custom(hdl_meta_cap, &vivid_ctrl_class, NULL);
>> + v4l2_ctrl_handler_init(hdl_meta_out, 2);
>> + v4l2_ctrl_new_custom(hdl_meta_out, &vivid_ctrl_class, NULL);
>>
>> /* User Controls */
>> dev->volume = v4l2_ctrl_new_std(hdl_user_aud, NULL,
>> @@ -1879,7 +1882,13 @@ int vivid_create_controls(struct vivid_dev *dev, bool show_ccs_cap,
>> return hdl_meta_cap->error;
>> dev->meta_cap_dev.ctrl_handler = hdl_meta_cap;
>> }
>> -
>> + if (dev->has_meta_out) {
>> + v4l2_ctrl_add_handler(hdl_meta_out, hdl_user_gen, NULL, false);
>> + v4l2_ctrl_add_handler(hdl_meta_out, hdl_streaming, NULL, false);
>> + if (hdl_meta_out->error)
>> + return hdl_meta_out->error;
>> + dev->meta_out_dev.ctrl_handler = hdl_meta_out;
>> + }
>> return 0;
>> }
>>
>> @@ -1900,4 +1909,5 @@ void vivid_free_controls(struct vivid_dev *dev)
>> v4l2_ctrl_handler_free(&dev->ctrl_hdl_loop_cap);
>> v4l2_ctrl_handler_free(&dev->ctrl_hdl_fb);
>> v4l2_ctrl_handler_free(&dev->ctrl_hdl_meta_cap);
>> + v4l2_ctrl_handler_free(&dev->ctrl_hdl_meta_out);
>> }
>> diff --git a/drivers/media/platform/vivid/vivid-kthread-out.c b/drivers/media/platform/vivid/vivid-kthread-out.c
>> index ce5bcda2348c..d63addb062d1 100644
>> --- a/drivers/media/platform/vivid/vivid-kthread-out.c
>> +++ b/drivers/media/platform/vivid/vivid-kthread-out.c
>> @@ -38,11 +38,13 @@
>> #include "vivid-osd.h"
>> #include "vivid-ctrls.h"
>> #include "vivid-kthread-out.h"
>> +#include "vivid-meta-out.h"
>>
>> static void vivid_thread_vid_out_tick(struct vivid_dev *dev)
>> {
>> struct vivid_buffer *vid_out_buf = NULL;
>> struct vivid_buffer *vbi_out_buf = NULL;
>> + struct vivid_buffer *meta_out_buf = NULL;
>>
>> dprintk(dev, 1, "Video Output Thread Tick\n");
>>
>> @@ -69,9 +71,15 @@ static void vivid_thread_vid_out_tick(struct vivid_dev *dev)
>> struct vivid_buffer, list);
>> list_del(&vbi_out_buf->list);
>> }
>> + if (!list_empty(&dev->meta_out_active) &&
>> + (dev->meta_out_seq_count & 1)) {
>> + meta_out_buf = list_entry(dev->meta_out_active.next,
>> + struct vivid_buffer, list);
>> + list_del(&meta_out_buf->list);
>> + }
>> spin_unlock(&dev->slock);
>>
>> - if (!vid_out_buf && !vbi_out_buf)
>> + if (!vid_out_buf && !vbi_out_buf && !meta_out_buf)
>> return;
>>
>> if (vid_out_buf) {
>> @@ -111,6 +119,21 @@ static void vivid_thread_vid_out_tick(struct vivid_dev *dev)
>> dprintk(dev, 2, "vbi_out buffer %d done\n",
>> vbi_out_buf->vb.vb2_buf.index);
>> }
>> + if (meta_out_buf) {
>> + v4l2_ctrl_request_setup(meta_out_buf->vb.vb2_buf.req_obj.req,
>> + &dev->ctrl_hdl_meta_out);
>> + v4l2_ctrl_request_complete(meta_out_buf->vb.vb2_buf.req_obj.req,
>> + &dev->ctrl_hdl_meta_out);
>> + vivid_meta_out_process(dev, meta_out_buf);
>> + meta_out_buf->vb.sequence = dev->meta_out_seq_count;
>> + meta_out_buf->vb.vb2_buf.timestamp =
>> + ktime_get_ns() + dev->time_wrap_offset;
>> + vb2_buffer_done(&meta_out_buf->vb.vb2_buf, dev->dqbuf_error ?
>> + VB2_BUF_STATE_ERROR : VB2_BUF_STATE_DONE);
>> + dprintk(dev, 2, "meta_out buffer %d done\n",
>> + meta_out_buf->vb.vb2_buf.index);
>> + }
>> +
>> dev->dqbuf_error = false;
>> }
>>
>> @@ -136,6 +159,7 @@ static int vivid_thread_vid_out(void *data)
>> dev->out_seq_count = 0xffffff80U;
>> dev->jiffies_vid_out = jiffies;
>> dev->vid_out_seq_start = dev->vbi_out_seq_start = 0;
>> + dev->meta_out_seq_start = 0;
>> dev->out_seq_resync = false;
>>
>> for (;;) {
>> @@ -178,6 +202,7 @@ static int vivid_thread_vid_out(void *data)
>> dev->out_seq_count = buffers_since_start + dev->out_seq_offset;
>> dev->vid_out_seq_count = dev->out_seq_count - dev->vid_out_seq_start;
>> dev->vbi_out_seq_count = dev->out_seq_count - dev->vbi_out_seq_start;
>> + dev->meta_out_seq_count = dev->out_seq_count - dev->meta_out_seq_start;
>>
>> vivid_thread_vid_out_tick(dev);
>> mutex_unlock(&dev->mutex);
>> @@ -229,8 +254,10 @@ int vivid_start_generating_vid_out(struct vivid_dev *dev, bool *pstreaming)
>>
>> if (pstreaming == &dev->vid_out_streaming)
>> dev->vid_out_seq_start = seq_count;
>> - else
>> + else if (pstreaming == &dev->vbi_out_streaming)
>> dev->vbi_out_seq_start = seq_count;
>> + else
>> + dev->meta_out_seq_start = seq_count;
>> *pstreaming = true;
>> return 0;
>> }
>> @@ -239,6 +266,7 @@ int vivid_start_generating_vid_out(struct vivid_dev *dev, bool *pstreaming)
>> dev->jiffies_vid_out = jiffies;
>> dev->vid_out_seq_start = dev->seq_wrap * 128;
>> dev->vbi_out_seq_start = dev->seq_wrap * 128;
>> + dev->meta_out_seq_start = dev->seq_wrap * 128;
>
> What is this 128? It is a good idea to have define for this to describe
> what it is so it is easier to maintain later.
Also out-of-scope of this patch. And I just discovered that this feature is broken :-(
I need to fix this, but I rather wait until metadata capture/output is merged first.
>
>>
>> dev->kthread_vid_out = kthread_run(vivid_thread_vid_out, dev,
>> "%s-vid-out", dev->v4l2_dev.name);
>> @@ -296,7 +324,23 @@ void vivid_stop_generating_vid_out(struct vivid_dev *dev, bool *pstreaming)
>> }
>> }
>>
>> - if (dev->vid_out_streaming || dev->vbi_out_streaming)
>> + if (pstreaming == &dev->meta_out_streaming) {
>> + while (!list_empty(&dev->meta_out_active)) {
>> + struct vivid_buffer *buf;
>> +
>> + buf = list_entry(dev->meta_out_active.next,
>> + struct vivid_buffer, list);
>> + list_del(&buf->list);
>> + v4l2_ctrl_request_complete(buf->vb.vb2_buf.req_obj.req,
>> + &dev->ctrl_hdl_meta_out);
>> + vb2_buffer_done(&buf->vb.vb2_buf, VB2_BUF_STATE_ERROR);
>> + dprintk(dev, 2, "meta_out buffer %d done\n",
>> + buf->vb.vb2_buf.index);
>> + }
>> + }
>> +
>> + if (dev->vid_out_streaming || dev->vbi_out_streaming ||
>> + dev->meta_out_streaming)
>> return;
>>
>> /* shutdown control thread */
>> diff --git a/drivers/media/platform/vivid/vivid-meta-out.c b/drivers/media/platform/vivid/vivid-meta-out.c
>> new file mode 100644
>> index 000000000000..e127757dd21d
>> --- /dev/null
>> +++ b/drivers/media/platform/vivid/vivid-meta-out.c
>> @@ -0,0 +1,174 @@
>> +// SPDX-License-Identifier: GPL-2.0-only
>> +/*
>> + * vivid-meta-out.c - meta output support functions.
>> + */
>> +
>> +#include <linux/errno.h>
>> +#include <linux/kernel.h>
>> +#include <linux/videodev2.h>
>> +#include <media/v4l2-common.h>
>> +#include <linux/usb/video.h>
>> +
>> +#include "vivid-core.h"
>> +#include "vivid-kthread-out.h"
>> +#include "vivid-meta-out.h"
>> +
>> +static int meta_out_queue_setup(struct vb2_queue *vq, unsigned int *nbuffers,
>> + unsigned int *nplanes, unsigned int sizes[],
>> + struct device *alloc_devs[])
>> +{
>> + struct vivid_dev *dev = vb2_get_drv_priv(vq);
>> + unsigned int size = sizeof(struct vivid_meta_out_buf);
>> +
>> + if (!vivid_is_webcam(dev))
>> + return -EINVAL;
>> +
>> + if (*nplanes) {
>> + if (sizes[0] < size)
>> + return -EINVAL;
>> + } else {
>> + sizes[0] = size;
>> + }
>> +
>> + if (vq->num_buffers + *nbuffers < 2)
>> + *nbuffers = 2 - vq->num_buffers;
>> +
>> + *nplanes = 1;
>> + return 0;
>> +}
>> +
>> +static int meta_out_buf_prepare(struct vb2_buffer *vb)
>> +{
>> + struct vivid_dev *dev = vb2_get_drv_priv(vb->vb2_queue);
>> + unsigned int size = sizeof(struct vivid_meta_out_buf);
>> +
>> + dprintk(dev, 1, "%s\n", __func__);
>> +
>> + if (dev->buf_prepare_error) {
>> + /*
>> + * Error injection: test what happens if buf_prepare() returns
>> + * an error.
>> + */
>> + dev->buf_prepare_error = false;
>> + return -EINVAL;
>> + }
>> + if (vb2_plane_size(vb, 0) < size) {
>> + dprintk(dev, 1, "%s data will not fit into plane (%lu < %u)\n",
>> + __func__, vb2_plane_size(vb, 0), size);
>> + return -EINVAL;
>> + }
>> + vb2_set_plane_payload(vb, 0, size);
>> +
>> + return 0;
>> +}
>> +
>> +static void meta_out_buf_queue(struct vb2_buffer *vb)
>> +{
>> + struct vb2_v4l2_buffer *vbuf = to_vb2_v4l2_buffer(vb);
>> + struct vivid_dev *dev = vb2_get_drv_priv(vb->vb2_queue);
>> + struct vivid_buffer *buf = container_of(vbuf, struct vivid_buffer, vb);
>> +
>> + dprintk(dev, 1, "%s\n", __func__);
>> +
>> + spin_lock(&dev->slock);
>> + list_add_tail(&buf->list, &dev->meta_out_active);
>> + spin_unlock(&dev->slock);
>> +}
>> +
>> +static int meta_out_start_streaming(struct vb2_queue *vq, unsigned int count)
>> +{
>> + struct vivid_dev *dev = vb2_get_drv_priv(vq);
>> + int err;
>> +
>> + dprintk(dev, 1, "%s\n", __func__);
>> + dev->meta_out_seq_count = 0;
>> + if (dev->start_streaming_error) {
>> + dev->start_streaming_error = false;
>> + err = -EINVAL;
>
> Just add goto err:
>
>> + } else {
>> + err = vivid_start_generating_vid_out(dev,
>> + &dev->meta_out_streaming);
>> + }
>
> You don't need these braces around else block. Didn't checkpatch tell
> you that. I thought it would.
Actually, if one part of a 'if'...'else' needs braces, then you should put
them around both. checkpatch only warns if both parts are single statements.
>
> You can simplify this logic by adding goto err: You don't even need
> the else.
>
> make this err:
>
>> + if (err) {
>> + struct vivid_buffer *buf, *tmp;
>> +
>> + list_for_each_entry_safe(buf, tmp,
>> + &dev->meta_out_active, list) {
>> + list_del(&buf->list);
>> + vb2_buffer_done(&buf->vb.vb2_buf,
>> + VB2_BUF_STATE_QUEUED);
>> + }
>> + }
>> + return err;
>> +}
>> +
>> +/* abort streaming and wait for last buffer */
>> +static void meta_out_stop_streaming(struct vb2_queue *vq)
>> +{
>> + struct vivid_dev *dev = vb2_get_drv_priv(vq);
>> +
>> + dprintk(dev, 1, "%s\n", __func__);
>> + vivid_stop_generating_vid_out(dev, &dev->meta_out_streaming);
>> +}
>> +
>> +static void meta_out_buf_request_complete(struct vb2_buffer *vb)
>> +{
>> + struct vivid_dev *dev = vb2_get_drv_priv(vb->vb2_queue);
>> +
>> + v4l2_ctrl_request_complete(vb->req_obj.req, &dev->ctrl_hdl_meta_out);
>> +}
>> +
>> +const struct vb2_ops vivid_meta_out_qops = {
>> + .queue_setup = meta_out_queue_setup,
>> + .buf_prepare = meta_out_buf_prepare,
>> + .buf_queue = meta_out_buf_queue,
>> + .start_streaming = meta_out_start_streaming,
>> + .stop_streaming = meta_out_stop_streaming,
>> + .buf_request_complete = meta_out_buf_request_complete,
>> + .wait_prepare = vb2_ops_wait_prepare,
>> + .wait_finish = vb2_ops_wait_finish,
>> +};
>> +
>> +int vidioc_enum_fmt_meta_out(struct file *file, void *priv,
>> + struct v4l2_fmtdesc *f)
>> +{
>> + struct vivid_dev *dev = video_drvdata(file);
>> +
>> + if (!vivid_is_webcam(dev))
>> + return -EINVAL;
>> +
>> + if (f->index > 0)
>> + return -EINVAL;
>
> You could collapase these two into one single check.
I don't have a strong opinion on this. Very different things are tested here,
so I am OK with two 'if's.
>
>> +
>> + f->type = V4L2_BUF_TYPE_META_OUTPUT;
>> + f->pixelformat = V4L2_META_FMT_VIVID;/* V4L2_META_FMT_UVC */
>> + return 0;
>> +}
>> +
>> +int vidioc_g_fmt_meta_out(struct file *file, void *priv,
>> + struct v4l2_format *f)
>> +{
>> + struct vivid_dev *dev = video_drvdata(file);
>> + struct v4l2_meta_format *meta = &f->fmt.meta;
>> +
>> + if (!vivid_is_webcam(dev) || !dev->has_meta_out)
>> + return -EINVAL;
>> +
>> + meta->dataformat = V4L2_META_FMT_VIVID;
>> + meta->buffersize = sizeof(struct vivid_meta_out_buf);
>> + return 0;
>> +}
>> +
>> +void vivid_meta_out_process(struct vivid_dev *dev,
>> + struct vivid_buffer *buf)
>> +{
>> + struct vivid_meta_out_buf *meta = vb2_plane_vaddr(&buf->vb.vb2_buf, 0);
>> +
>> + tpg_s_brightness(&dev->tpg, meta->brightness);
>> + tpg_s_contrast(&dev->tpg, meta->contrast);
>> + tpg_s_saturation(&dev->tpg, meta->saturation);
>> + tpg_s_hue(&dev->tpg, meta->hue);
>> + dprintk(dev, 2, " %s brightness %u contrast %u saturation %u hue %d\n",
>> + __func__, meta->brightness, meta->contrast,
>> + meta->saturation, meta->hue);
>> +}
>> diff --git a/drivers/media/platform/vivid/vivid-meta-out.h b/drivers/media/platform/vivid/vivid-meta-out.h
>> new file mode 100644
>> index 000000000000..0c639b7c2842
>> --- /dev/null
>> +++ b/drivers/media/platform/vivid/vivid-meta-out.h
>> @@ -0,0 +1,25 @@
>> +/* SPDX-License-Identifier: GPL-2.0-only */
>> +/*
>> + * vivid-meta-out.h - meta output support functions.
>> + */
>> +#ifndef _VIVID_META_OUT_H_
>> +#define _VIVID_META_OUT_H_
>> +
>> +struct vivid_meta_out_buf {
>> + u16 brightness;
>> + u16 contrast;
>> + u16 saturation;
>> + s16 hue;
>> +};
>> +
>> +void vivid_meta_out_process(struct vivid_dev *dev, struct vivid_buffer *buf);
>> +int vidioc_enum_fmt_meta_out(struct file *file, void *priv,
>> + struct v4l2_fmtdesc *f);
>> +int vidioc_g_fmt_meta_out(struct file *file, void *priv,
>> + struct v4l2_format *f);
>> +int vidioc_s_fmt_meta_out(struct file *file, void *priv,
>> + struct v4l2_format *f);
>> +
>> +extern const struct vb2_ops vivid_meta_out_qops;
>> +
>> +#endif
>> diff --git a/drivers/media/platform/vivid/vivid-vid-out.c b/drivers/media/platform/vivid/vivid-vid-out.c
>> index a0364ac497f9..ee3446e3217c 100644
>> --- a/drivers/media/platform/vivid/vivid-vid-out.c
>> +++ b/drivers/media/platform/vivid/vivid-vid-out.c
>> @@ -1079,7 +1079,9 @@ int vidioc_s_output(struct file *file, void *priv, unsigned o)
>> if (o == dev->output)
>> return 0;
>>
>> - if (vb2_is_busy(&dev->vb_vid_out_q) || vb2_is_busy(&dev->vb_vbi_out_q))
>> + if (vb2_is_busy(&dev->vb_vid_out_q) ||
>> + vb2_is_busy(&dev->vb_vbi_out_q) ||
>> + vb2_is_busy(&dev->vb_meta_out_q))
>> return -EBUSY;
>>
>> dev->output = o;
>> @@ -1090,6 +1092,7 @@ int vidioc_s_output(struct file *file, void *priv, unsigned o)
>> dev->vid_out_dev.tvnorms = 0;
>>
>> dev->vbi_out_dev.tvnorms = dev->vid_out_dev.tvnorms;
>> + dev->meta_out_dev.tvnorms = dev->vid_out_dev.tvnorms;
>> vivid_update_format_out(dev);
>>
>> v4l2_ctrl_activate(dev->ctrl_display_present, vivid_is_hdmi_out(dev));
>>
>
> thanks,
> -- Shuah
>
Regards,
Hans
WARNING: multiple messages have this Message-ID (diff)
From: hverkuil@xs4all.nl (Hans Verkuil)
Subject: [Linux-kernel-mentees] [PATCH v2 2/2] vivid: Add metadata output support
Date: Fri, 4 Oct 2019 11:55:01 +0200 [thread overview]
Message-ID: <787ff1cb-dde3-800a-9dde-68f1db5e4897@xs4all.nl> (raw)
Message-ID: <20191004095501._lFwe9lujkHifWaQpgCIi99FE2JFl0oy2vBaOFGu18I@z> (raw)
In-Reply-To: <1955ad5e-9359-d7be-5c92-98f652097331@linuxfoundation.org>
On 10/3/19 7:38 PM, Shuah Khan wrote:
> On 10/3/19 5:08 AM, Vandana BN wrote:
>> Support metadata output in vivid driver.
>> Metadata output is used to set brightness, contrast, saturation
>> and hue.
>> Adds new files for metadata output.
>>
>> Signed-off-by: Vandana BN <bnvandana at gmail.com>
>> ---
>> drivers/media/platform/vivid/Makefile | 2 +-
>> drivers/media/platform/vivid/vivid-core.c | 98 +++++++++-
>> drivers/media/platform/vivid/vivid-core.h | 10 +
>> drivers/media/platform/vivid/vivid-ctrls.c | 14 +-
>> .../media/platform/vivid/vivid-kthread-out.c | 50 ++++-
>> drivers/media/platform/vivid/vivid-meta-out.c | 174 ++++++++++++++++++
>> drivers/media/platform/vivid/vivid-meta-out.h | 25 +++
>> drivers/media/platform/vivid/vivid-vid-out.c | 5 +-
>> 8 files changed, 366 insertions(+), 12 deletions(-)
>> create mode 100644 drivers/media/platform/vivid/vivid-meta-out.c
>> create mode 100644 drivers/media/platform/vivid/vivid-meta-out.h
>>
>> diff --git a/drivers/media/platform/vivid/Makefile b/drivers/media/platform/vivid/Makefile
>> index af94abf9bce6..e8a50c506dc9 100644
>> --- a/drivers/media/platform/vivid/Makefile
>> +++ b/drivers/media/platform/vivid/Makefile
>> @@ -3,7 +3,7 @@ vivid-objs := vivid-core.o vivid-ctrls.o vivid-vid-common.o vivid-vbi-gen.o \
>> vivid-vid-cap.o vivid-vid-out.o vivid-kthread-cap.o vivid-kthread-out.o \
>> vivid-radio-rx.o vivid-radio-tx.o vivid-radio-common.o \
>> vivid-rds-gen.o vivid-sdr-cap.o vivid-vbi-cap.o vivid-vbi-out.o \
>> - vivid-osd.o vivid-meta-cap.o
>> + vivid-osd.o vivid-meta-cap.o vivid-meta-out.o
>> ifeq ($(CONFIG_VIDEO_VIVID_CEC),y)
>> vivid-objs += vivid-cec.o
>> endif
>> diff --git a/drivers/media/platform/vivid/vivid-core.c b/drivers/media/platform/vivid/vivid-core.c
>> index 97ab197bdec0..7f08e8ae5446 100644
>> --- a/drivers/media/platform/vivid/vivid-core.c
>> +++ b/drivers/media/platform/vivid/vivid-core.c
>> @@ -38,6 +38,7 @@
>> #include "vivid-cec.h"
>> #include "vivid-ctrls.h"
>> #include "vivid-meta-cap.h"
>> +#include "vivid-meta-out.h"
>>
>> #define VIVID_MODULE_NAME "vivid"
>>
>> @@ -84,6 +85,10 @@ static int meta_cap_nr[VIVID_MAX_DEVS] = { [0 ... (VIVID_MAX_DEVS - 1)] = -1 };
>> module_param_array(meta_cap_nr, int, NULL, 0444);
>> MODULE_PARM_DESC(meta_cap_nr, " videoX start number, -1 is autodetect");
>>
>> +static int meta_out_nr[VIVID_MAX_DEVS] = { [0 ... (VIVID_MAX_DEVS - 1)] = -1 };
>> +module_param_array(meta_out_nr, int, NULL, 0444);
>> +MODULE_PARM_DESC(meta_out_nr, " videoX start number, -1 is autodetect");
>> +
>> static int ccs_cap_mode[VIVID_MAX_DEVS] = { [0 ... (VIVID_MAX_DEVS - 1)] = -1 };
>> module_param_array(ccs_cap_mode, int, NULL, 0444);
>> MODULE_PARM_DESC(ccs_cap_mode, " capture crop/compose/scale mode:\n"
>> @@ -105,10 +110,10 @@ MODULE_PARM_DESC(multiplanar, " 1 (default) creates a single planar device, 2 cr
>> * vbi-out + vid-out + meta-cap
>> */
>> static unsigned int node_types[VIVID_MAX_DEVS] = {
>> - [0 ... (VIVID_MAX_DEVS - 1)] = 0x21d3d
>> + [0 ... (VIVID_MAX_DEVS - 1)] = 0x61d3d
>
> Why not add a define that is human friendly to describe the node type?
This is something for a separate patch and shouldn't be mixed in with this.
It would certainly be useful to have defines for this.
>
>> };
>> module_param_array(node_types, uint, NULL, 0444);
>> -MODULE_PARM_DESC(node_types, " node types, default is 0x1d3d. Bitmask with the following meaning:\n"
>> +MODULE_PARM_DESC(node_types, " node types, default is 0x61d3d. Bitmask with the following meaning:\n"
>> "\t\t bit 0: Video Capture node\n"
>> "\t\t bit 2-3: VBI Capture node: 0 = none, 1 = raw vbi, 2 = sliced vbi, 3 = both\n"
>> "\t\t bit 4: Radio Receiver node\n"
>> @@ -117,7 +122,8 @@ MODULE_PARM_DESC(node_types, " node types, default is 0x1d3d. Bitmask with the f
>> "\t\t bit 10-11: VBI Output node: 0 = none, 1 = raw vbi, 2 = sliced vbi, 3 = both\n"
>> "\t\t bit 12: Radio Transmitter node\n"
>> "\t\t bit 16: Framebuffer for testing overlays\n"
>> - "\t\t bit 17: Metadata Capture node\n");
>> + "\t\t bit 17: Metadata Capture node\n"
>> + "\t\t bit 18: Metadata Output node\n");
>>
>> /* Default: 4 inputs */
>> static unsigned num_inputs[VIVID_MAX_DEVS] = { [0 ... (VIVID_MAX_DEVS - 1)] = 4 };
>> @@ -216,7 +222,8 @@ static int vidioc_querycap(struct file *file, void *priv,
>> cap->capabilities = dev->vid_cap_caps | dev->vid_out_caps |
>> dev->vbi_cap_caps | dev->vbi_out_caps |
>> dev->radio_rx_caps | dev->radio_tx_caps |
>> - dev->sdr_cap_caps | dev->meta_cap_caps | V4L2_CAP_DEVICE_CAPS;
>> + dev->sdr_cap_caps | dev->meta_cap_caps |
>> + dev->meta_out_caps | V4L2_CAP_DEVICE_CAPS;
>> return 0;
>> }
>>
>> @@ -445,7 +452,8 @@ static bool vivid_is_last_user(struct vivid_dev *dev)
>> vivid_is_in_use(&dev->sdr_cap_dev) +
>> vivid_is_in_use(&dev->radio_rx_dev) +
>> vivid_is_in_use(&dev->radio_tx_dev) +
>> - vivid_is_in_use(&dev->meta_cap_dev);
>> + vivid_is_in_use(&dev->meta_cap_dev) +
>> + vivid_is_in_use(&dev->meta_out_dev);
>>
>> return uses == 1;
>> }
>> @@ -472,6 +480,7 @@ static int vivid_fop_release(struct file *file)
>> set_bit(V4L2_FL_REGISTERED, &dev->radio_rx_dev.flags);
>> set_bit(V4L2_FL_REGISTERED, &dev->radio_tx_dev.flags);
>> set_bit(V4L2_FL_REGISTERED, &dev->meta_cap_dev.flags);
>> + set_bit(V4L2_FL_REGISTERED, &dev->meta_out_dev.flags);
>> }
>> mutex_unlock(&dev->mutex);
>> if (file->private_data == dev->overlay_cap_owner)
>> @@ -622,6 +631,11 @@ static const struct v4l2_ioctl_ops vivid_ioctl_ops = {
>> .vidioc_g_fmt_meta_cap = vidioc_g_fmt_meta_cap,
>> .vidioc_s_fmt_meta_cap = vidioc_g_fmt_meta_cap,
>> .vidioc_try_fmt_meta_cap = vidioc_g_fmt_meta_cap,
>> +
>
> Do you need this blank line here?
>
>> + .vidioc_enum_fmt_meta_out = vidioc_enum_fmt_meta_out,
>> + .vidioc_g_fmt_meta_out = vidioc_g_fmt_meta_out,
>> + .vidioc_s_fmt_meta_out = vidioc_g_fmt_meta_out,
>> + .vidioc_try_fmt_meta_out = vidioc_g_fmt_meta_out,
>> };
>>
>> /* -----------------------------------------------------------------
>> @@ -839,6 +853,9 @@ static int vivid_create_instance(struct platform_device *pdev, int inst)
>> /* do we create a meta capture device */
>> dev->has_meta_cap = node_type & 0x20000;
>
> Again same comment on this 0x20000 - please make this a define
> that describes what this is.
>
>>
>> + /* do we create a metadata output device */
>> + dev->has_meta_out = node_type & 0x40000;
>
> Same comment here.
>
>> +
>> /* end detecting feature set */
>>
>> if (dev->has_vid_cap) {
>> @@ -905,6 +922,13 @@ static int vivid_create_instance(struct platform_device *pdev, int inst)
>
> This is a very loooong routine! It is very difficult to figure out
> if the error handling is correct and review is rather hard.
Also out-of-scope of this patch. It can certainly be split up in smaller functions,
but that shouldn't be mixed in with this.
>
>> if (in_type_counter[TV])
>> dev->meta_cap_caps |= V4L2_CAP_TUNER;
>> }
>> + /* set up the capablilities of meta output device*/
>> + if (dev->has_meta_out) {
>> + dev->meta_out_caps = V4L2_CAP_META_OUTPUT |
>> + V4L2_CAP_STREAMING | V4L2_CAP_READWRITE;
>
> Add V4L2_CAP_READWRITE on the next line?
>
>> + if (dev->has_audio_outputs)
>> + dev->meta_out_caps |= V4L2_CAP_AUDIO;
>> + }
>>
>> ret = -ENOMEM;
>> /* initialize the test pattern generator */
>> @@ -976,6 +1000,9 @@ static int vivid_create_instance(struct platform_device *pdev, int inst)
>> v4l2_disable_ioctl(&dev->vbi_out_dev, VIDIOC_S_AUDOUT);
>> v4l2_disable_ioctl(&dev->vbi_out_dev, VIDIOC_G_AUDOUT);
>> v4l2_disable_ioctl(&dev->vbi_out_dev, VIDIOC_ENUMAUDOUT);
>> + v4l2_disable_ioctl(&dev->meta_out_dev, VIDIOC_S_AUDOUT);
>> + v4l2_disable_ioctl(&dev->meta_out_dev, VIDIOC_G_AUDOUT);
>> + v4l2_disable_ioctl(&dev->meta_out_dev, VIDIOC_ENUMAUDOUT);
>> }
>> if (!in_type_counter[TV] && !in_type_counter[SVID]) {
>> v4l2_disable_ioctl(&dev->vid_cap_dev, VIDIOC_S_STD);
>> @@ -1035,6 +1062,8 @@ static int vivid_create_instance(struct platform_device *pdev, int inst)
>> v4l2_disable_ioctl(&dev->vid_out_dev, VIDIOC_ENUM_FRAMEINTERVALS);
>> v4l2_disable_ioctl(&dev->vbi_out_dev, VIDIOC_S_FREQUENCY);
>> v4l2_disable_ioctl(&dev->vbi_out_dev, VIDIOC_G_FREQUENCY);
>> + v4l2_disable_ioctl(&dev->meta_out_dev, VIDIOC_S_FREQUENCY);
>> + v4l2_disable_ioctl(&dev->meta_out_dev, VIDIOC_G_FREQUENCY);
>>
>> /* configure internal data */
>> dev->fmt_cap = &vivid_formats[0];
>> @@ -1118,6 +1147,7 @@ static int vivid_create_instance(struct platform_device *pdev, int inst)
>> INIT_LIST_HEAD(&dev->vbi_out_active);
>> INIT_LIST_HEAD(&dev->sdr_cap_active);
>> INIT_LIST_HEAD(&dev->meta_cap_active);
>> + INIT_LIST_HEAD(&dev->meta_out_active);
>>
>> INIT_LIST_HEAD(&dev->cec_work_list);
>> spin_lock_init(&dev->cec_slock);
>> @@ -1286,6 +1316,27 @@ static int vivid_create_instance(struct platform_device *pdev, int inst)
>> goto unreg_dev;
>> }
>>
>> + if (dev->has_meta_out) {
>> + /* initialize vbi_out queue */
>> + q = &dev->vb_meta_out_q;
>> + q->type = V4L2_BUF_TYPE_META_OUTPUT;
>> + q->io_modes = VB2_MMAP | VB2_DMABUF | VB2_WRITE;
>> + if (!allocator)
>> + q->io_modes |= VB2_USERPTR;
>> + q->drv_priv = dev;
>> + q->buf_struct_size = sizeof(struct vivid_buffer);
>> + q->ops = &vivid_meta_out_qops;
>> + q->mem_ops = vivid_mem_ops[allocator];
>> + q->timestamp_flags = V4L2_BUF_FLAG_TIMESTAMP_MONOTONIC;
>> + q->min_buffers_needed = 2;
>> + q->lock = &dev->mutex;
>> + q->dev = dev->v4l2_dev.dev;
>> + q->supports_requests = true;
>> + ret = vb2_queue_init(q);
> Make sure queue is released in error leg
Not needed, it's only required when you close a filehandle that was streaming.
And that's already done in vb2_fop_release.
>
>> + if (ret)
>> + goto unreg_dev;
>
> I think release is isn't there in unreg_dev handling
>
>> + }
>> +
>> #ifdef CONFIG_VIDEO_VIVID_CEC
>> if (dev->has_vid_cap && in_type_counter[HDMI]) {
>> struct cec_adapter *adap;
>> @@ -1327,6 +1378,7 @@ static int vivid_create_instance(struct platform_device *pdev, int inst)
>> v4l2_ctrl_handler_setup(&dev->ctrl_hdl_radio_tx);
>> v4l2_ctrl_handler_setup(&dev->ctrl_hdl_sdr_cap);
>> v4l2_ctrl_handler_setup(&dev->ctrl_hdl_meta_cap);
>> + v4l2_ctrl_handler_setup(&dev->ctrl_hdl_meta_out);/* is this needed */
>
> Tab between the routine and comment
>
>>
>> /* finally start creating the device nodes */
>> if (dev->has_vid_cap) {
>> @@ -1583,6 +1635,36 @@ static int vivid_create_instance(struct platform_device *pdev, int inst)
>> video_device_node_name(vfd));
>> }
>>
>> + if (dev->has_meta_out) {
>> + vfd = &dev->meta_out_dev;
>> + snprintf(vfd->name, sizeof(vfd->name),
>> + "vivid-%03d-meta-out", inst);
>> + vfd->vfl_dir = VFL_DIR_TX;
>> + vfd->fops = &vivid_fops;
>> + vfd->ioctl_ops = &vivid_ioctl_ops;
>> + vfd->device_caps = dev->meta_out_caps;
>> + vfd->release = video_device_release_empty;
>> + vfd->v4l2_dev = &dev->v4l2_dev;
>> + vfd->queue = &dev->vb_meta_out_q;
>> + vfd->lock = &dev->mutex;
>> + vfd->tvnorms = tvnorms_out;
>> + video_set_drvdata(vfd, dev);
>> +#ifdef CONFIG_MEDIA_CONTROLLER
>> + dev->meta_out_pad.flags = MEDIA_PAD_FL_SOURCE;
>> + ret = media_entity_pads_init(&vfd->entity, 1,
>> + &dev->meta_out_pad);
>> + if (ret)
>> + goto unreg_dev;
>
> Where do we do cleanup of pads if something fails? I didn't see it.
Also out-of-scope of this patch. Vivid never calls media_entity_cleanup().
It should, though. Even though media_entity_cleanup() is currently a nop.
Should be added in a separate patch.
>
>> +#endif
>> + ret = video_register_device(vfd, VFL_TYPE_GRABBER,
>> + meta_out_nr[inst]);
>> + if (ret < 0)
>> + goto unreg_dev;
>> + v4l2_info(&dev->v4l2_dev,
>> + "V4L2 metadata output device registered as %s\n",
>> + video_device_node_name(vfd));
>> + }
>> +
>> #ifdef CONFIG_MEDIA_CONTROLLER
>> /* Register the media device */
>> ret = media_device_register(&dev->mdev);
>> @@ -1599,6 +1681,7 @@ static int vivid_create_instance(struct platform_device *pdev, int inst)
>> return 0;
>>
>> unreg_dev:
>> + video_unregister_device(&dev->meta_out_dev);
>> video_unregister_device(&dev->meta_cap_dev);
>> video_unregister_device(&dev->radio_tx_dev);
>> video_unregister_device(&dev->radio_rx_dev);
>> @@ -1721,6 +1804,11 @@ static int vivid_remove(struct platform_device *pdev)
>> video_device_node_name(&dev->meta_cap_dev));
>> video_unregister_device(&dev->meta_cap_dev);
>> }
>> + if (dev->has_meta_out) {
>> + v4l2_info(&dev->v4l2_dev, "unregistering %s\n",
>> + video_device_node_name(&dev->meta_out_dev));
>> + video_unregister_device(&dev->meta_out_dev);
>> + }
>> cec_unregister_adapter(dev->cec_rx_adap);
>> for (j = 0; j < MAX_OUTPUTS; j++)
>> cec_unregister_adapter(dev->cec_tx_adap[j]);
>> diff --git a/drivers/media/platform/vivid/vivid-core.h b/drivers/media/platform/vivid/vivid-core.h
>> index fd601345a17c..d57066ed31f0 100644
>> --- a/drivers/media/platform/vivid/vivid-core.h
>> +++ b/drivers/media/platform/vivid/vivid-core.h
>> @@ -132,6 +132,7 @@ struct vivid_dev {
>> struct media_pad vbi_out_pad;
>> struct media_pad sdr_cap_pad;
>> struct media_pad meta_cap_pad;
>> + struct media_pad meta_out_pad;
>> #endif
>> struct v4l2_ctrl_handler ctrl_hdl_user_gen;
>> struct v4l2_ctrl_handler ctrl_hdl_user_vid;
>> @@ -156,6 +157,8 @@ struct vivid_dev {
>> struct v4l2_ctrl_handler ctrl_hdl_sdr_cap;
>> struct video_device meta_cap_dev;
>> struct v4l2_ctrl_handler ctrl_hdl_meta_cap;
>> + struct video_device meta_out_dev;
>> + struct v4l2_ctrl_handler ctrl_hdl_meta_out;
>>
>> spinlock_t slock;
>> struct mutex mutex;
>> @@ -169,6 +172,7 @@ struct vivid_dev {
>> u32 radio_rx_caps;
>> u32 radio_tx_caps;
>> u32 meta_cap_caps;
>> + u32 meta_out_caps;
>>
>> /* supported features */
>> bool multiplanar;
>> @@ -195,6 +199,7 @@ struct vivid_dev {
>> bool has_sdr_cap;
>> bool has_fb;
>> bool has_meta_cap;
>> + bool has_meta_out;
>>
>> bool can_loop_video;
>>
>> @@ -432,6 +437,8 @@ struct vivid_dev {
>> struct list_head vid_out_active;
>> struct vb2_queue vb_vbi_out_q;
>> struct list_head vbi_out_active;
>> + struct vb2_queue vb_meta_out_q;
>> + struct list_head meta_out_active;
>>
>> /* video loop precalculated rectangles */
>>
>> @@ -472,6 +479,9 @@ struct vivid_dev {
>> u32 vbi_out_seq_count;
>> bool vbi_out_streaming;
>> bool stream_sliced_vbi_out;
>> + u32 meta_out_seq_start;
>> + u32 meta_out_seq_count;
>> + bool meta_out_streaming;
>>
>> /* SDR capture */
>> struct vb2_queue vb_sdr_cap_q;
>> diff --git a/drivers/media/platform/vivid/vivid-ctrls.c b/drivers/media/platform/vivid/vivid-ctrls.c
>> index 2ab6a680485c..a7d42e139a4f 100644
>> --- a/drivers/media/platform/vivid/vivid-ctrls.c
>> +++ b/drivers/media/platform/vivid/vivid-ctrls.c
>> @@ -114,7 +114,7 @@ static int vivid_user_gen_s_ctrl(struct v4l2_ctrl *ctrl)
>> clear_bit(V4L2_FL_REGISTERED, &dev->radio_rx_dev.flags);
>> clear_bit(V4L2_FL_REGISTERED, &dev->radio_tx_dev.flags);
>> clear_bit(V4L2_FL_REGISTERED, &dev->meta_cap_dev.flags);
>> - clear_bit(V4L2_FL_REGISTERED, &dev->meta_cap_dev.flags);
>> + clear_bit(V4L2_FL_REGISTERED, &dev->meta_out_dev.flags);
>> break;
>> case VIVID_CID_BUTTON:
>> dev->button_pressed = 30;
>> @@ -1493,6 +1493,7 @@ int vivid_create_controls(struct vivid_dev *dev, bool show_ccs_cap,
>> struct v4l2_ctrl_handler *hdl_radio_tx = &dev->ctrl_hdl_radio_tx;
>> struct v4l2_ctrl_handler *hdl_sdr_cap = &dev->ctrl_hdl_sdr_cap;
>> struct v4l2_ctrl_handler *hdl_meta_cap = &dev->ctrl_hdl_meta_cap;
>> + struct v4l2_ctrl_handler *hdl_meta_out = &dev->ctrl_hdl_meta_out;
>>
>> struct v4l2_ctrl_config vivid_ctrl_dv_timings = {
>> .ops = &vivid_vid_cap_ctrl_ops,
>> @@ -1534,6 +1535,8 @@ int vivid_create_controls(struct vivid_dev *dev, bool show_ccs_cap,
>> v4l2_ctrl_new_custom(hdl_sdr_cap, &vivid_ctrl_class, NULL);
>> v4l2_ctrl_handler_init(hdl_meta_cap, 2);
>> v4l2_ctrl_new_custom(hdl_meta_cap, &vivid_ctrl_class, NULL);
>> + v4l2_ctrl_handler_init(hdl_meta_out, 2);
>> + v4l2_ctrl_new_custom(hdl_meta_out, &vivid_ctrl_class, NULL);
>>
>> /* User Controls */
>> dev->volume = v4l2_ctrl_new_std(hdl_user_aud, NULL,
>> @@ -1879,7 +1882,13 @@ int vivid_create_controls(struct vivid_dev *dev, bool show_ccs_cap,
>> return hdl_meta_cap->error;
>> dev->meta_cap_dev.ctrl_handler = hdl_meta_cap;
>> }
>> -
>> + if (dev->has_meta_out) {
>> + v4l2_ctrl_add_handler(hdl_meta_out, hdl_user_gen, NULL, false);
>> + v4l2_ctrl_add_handler(hdl_meta_out, hdl_streaming, NULL, false);
>> + if (hdl_meta_out->error)
>> + return hdl_meta_out->error;
>> + dev->meta_out_dev.ctrl_handler = hdl_meta_out;
>> + }
>> return 0;
>> }
>>
>> @@ -1900,4 +1909,5 @@ void vivid_free_controls(struct vivid_dev *dev)
>> v4l2_ctrl_handler_free(&dev->ctrl_hdl_loop_cap);
>> v4l2_ctrl_handler_free(&dev->ctrl_hdl_fb);
>> v4l2_ctrl_handler_free(&dev->ctrl_hdl_meta_cap);
>> + v4l2_ctrl_handler_free(&dev->ctrl_hdl_meta_out);
>> }
>> diff --git a/drivers/media/platform/vivid/vivid-kthread-out.c b/drivers/media/platform/vivid/vivid-kthread-out.c
>> index ce5bcda2348c..d63addb062d1 100644
>> --- a/drivers/media/platform/vivid/vivid-kthread-out.c
>> +++ b/drivers/media/platform/vivid/vivid-kthread-out.c
>> @@ -38,11 +38,13 @@
>> #include "vivid-osd.h"
>> #include "vivid-ctrls.h"
>> #include "vivid-kthread-out.h"
>> +#include "vivid-meta-out.h"
>>
>> static void vivid_thread_vid_out_tick(struct vivid_dev *dev)
>> {
>> struct vivid_buffer *vid_out_buf = NULL;
>> struct vivid_buffer *vbi_out_buf = NULL;
>> + struct vivid_buffer *meta_out_buf = NULL;
>>
>> dprintk(dev, 1, "Video Output Thread Tick\n");
>>
>> @@ -69,9 +71,15 @@ static void vivid_thread_vid_out_tick(struct vivid_dev *dev)
>> struct vivid_buffer, list);
>> list_del(&vbi_out_buf->list);
>> }
>> + if (!list_empty(&dev->meta_out_active) &&
>> + (dev->meta_out_seq_count & 1)) {
>> + meta_out_buf = list_entry(dev->meta_out_active.next,
>> + struct vivid_buffer, list);
>> + list_del(&meta_out_buf->list);
>> + }
>> spin_unlock(&dev->slock);
>>
>> - if (!vid_out_buf && !vbi_out_buf)
>> + if (!vid_out_buf && !vbi_out_buf && !meta_out_buf)
>> return;
>>
>> if (vid_out_buf) {
>> @@ -111,6 +119,21 @@ static void vivid_thread_vid_out_tick(struct vivid_dev *dev)
>> dprintk(dev, 2, "vbi_out buffer %d done\n",
>> vbi_out_buf->vb.vb2_buf.index);
>> }
>> + if (meta_out_buf) {
>> + v4l2_ctrl_request_setup(meta_out_buf->vb.vb2_buf.req_obj.req,
>> + &dev->ctrl_hdl_meta_out);
>> + v4l2_ctrl_request_complete(meta_out_buf->vb.vb2_buf.req_obj.req,
>> + &dev->ctrl_hdl_meta_out);
>> + vivid_meta_out_process(dev, meta_out_buf);
>> + meta_out_buf->vb.sequence = dev->meta_out_seq_count;
>> + meta_out_buf->vb.vb2_buf.timestamp =
>> + ktime_get_ns() + dev->time_wrap_offset;
>> + vb2_buffer_done(&meta_out_buf->vb.vb2_buf, dev->dqbuf_error ?
>> + VB2_BUF_STATE_ERROR : VB2_BUF_STATE_DONE);
>> + dprintk(dev, 2, "meta_out buffer %d done\n",
>> + meta_out_buf->vb.vb2_buf.index);
>> + }
>> +
>> dev->dqbuf_error = false;
>> }
>>
>> @@ -136,6 +159,7 @@ static int vivid_thread_vid_out(void *data)
>> dev->out_seq_count = 0xffffff80U;
>> dev->jiffies_vid_out = jiffies;
>> dev->vid_out_seq_start = dev->vbi_out_seq_start = 0;
>> + dev->meta_out_seq_start = 0;
>> dev->out_seq_resync = false;
>>
>> for (;;) {
>> @@ -178,6 +202,7 @@ static int vivid_thread_vid_out(void *data)
>> dev->out_seq_count = buffers_since_start + dev->out_seq_offset;
>> dev->vid_out_seq_count = dev->out_seq_count - dev->vid_out_seq_start;
>> dev->vbi_out_seq_count = dev->out_seq_count - dev->vbi_out_seq_start;
>> + dev->meta_out_seq_count = dev->out_seq_count - dev->meta_out_seq_start;
>>
>> vivid_thread_vid_out_tick(dev);
>> mutex_unlock(&dev->mutex);
>> @@ -229,8 +254,10 @@ int vivid_start_generating_vid_out(struct vivid_dev *dev, bool *pstreaming)
>>
>> if (pstreaming == &dev->vid_out_streaming)
>> dev->vid_out_seq_start = seq_count;
>> - else
>> + else if (pstreaming == &dev->vbi_out_streaming)
>> dev->vbi_out_seq_start = seq_count;
>> + else
>> + dev->meta_out_seq_start = seq_count;
>> *pstreaming = true;
>> return 0;
>> }
>> @@ -239,6 +266,7 @@ int vivid_start_generating_vid_out(struct vivid_dev *dev, bool *pstreaming)
>> dev->jiffies_vid_out = jiffies;
>> dev->vid_out_seq_start = dev->seq_wrap * 128;
>> dev->vbi_out_seq_start = dev->seq_wrap * 128;
>> + dev->meta_out_seq_start = dev->seq_wrap * 128;
>
> What is this 128? It is a good idea to have define for this to describe
> what it is so it is easier to maintain later.
Also out-of-scope of this patch. And I just discovered that this feature is broken :-(
I need to fix this, but I rather wait until metadata capture/output is merged first.
>
>>
>> dev->kthread_vid_out = kthread_run(vivid_thread_vid_out, dev,
>> "%s-vid-out", dev->v4l2_dev.name);
>> @@ -296,7 +324,23 @@ void vivid_stop_generating_vid_out(struct vivid_dev *dev, bool *pstreaming)
>> }
>> }
>>
>> - if (dev->vid_out_streaming || dev->vbi_out_streaming)
>> + if (pstreaming == &dev->meta_out_streaming) {
>> + while (!list_empty(&dev->meta_out_active)) {
>> + struct vivid_buffer *buf;
>> +
>> + buf = list_entry(dev->meta_out_active.next,
>> + struct vivid_buffer, list);
>> + list_del(&buf->list);
>> + v4l2_ctrl_request_complete(buf->vb.vb2_buf.req_obj.req,
>> + &dev->ctrl_hdl_meta_out);
>> + vb2_buffer_done(&buf->vb.vb2_buf, VB2_BUF_STATE_ERROR);
>> + dprintk(dev, 2, "meta_out buffer %d done\n",
>> + buf->vb.vb2_buf.index);
>> + }
>> + }
>> +
>> + if (dev->vid_out_streaming || dev->vbi_out_streaming ||
>> + dev->meta_out_streaming)
>> return;
>>
>> /* shutdown control thread */
>> diff --git a/drivers/media/platform/vivid/vivid-meta-out.c b/drivers/media/platform/vivid/vivid-meta-out.c
>> new file mode 100644
>> index 000000000000..e127757dd21d
>> --- /dev/null
>> +++ b/drivers/media/platform/vivid/vivid-meta-out.c
>> @@ -0,0 +1,174 @@
>> +// SPDX-License-Identifier: GPL-2.0-only
>> +/*
>> + * vivid-meta-out.c - meta output support functions.
>> + */
>> +
>> +#include <linux/errno.h>
>> +#include <linux/kernel.h>
>> +#include <linux/videodev2.h>
>> +#include <media/v4l2-common.h>
>> +#include <linux/usb/video.h>
>> +
>> +#include "vivid-core.h"
>> +#include "vivid-kthread-out.h"
>> +#include "vivid-meta-out.h"
>> +
>> +static int meta_out_queue_setup(struct vb2_queue *vq, unsigned int *nbuffers,
>> + unsigned int *nplanes, unsigned int sizes[],
>> + struct device *alloc_devs[])
>> +{
>> + struct vivid_dev *dev = vb2_get_drv_priv(vq);
>> + unsigned int size = sizeof(struct vivid_meta_out_buf);
>> +
>> + if (!vivid_is_webcam(dev))
>> + return -EINVAL;
>> +
>> + if (*nplanes) {
>> + if (sizes[0] < size)
>> + return -EINVAL;
>> + } else {
>> + sizes[0] = size;
>> + }
>> +
>> + if (vq->num_buffers + *nbuffers < 2)
>> + *nbuffers = 2 - vq->num_buffers;
>> +
>> + *nplanes = 1;
>> + return 0;
>> +}
>> +
>> +static int meta_out_buf_prepare(struct vb2_buffer *vb)
>> +{
>> + struct vivid_dev *dev = vb2_get_drv_priv(vb->vb2_queue);
>> + unsigned int size = sizeof(struct vivid_meta_out_buf);
>> +
>> + dprintk(dev, 1, "%s\n", __func__);
>> +
>> + if (dev->buf_prepare_error) {
>> + /*
>> + * Error injection: test what happens if buf_prepare() returns
>> + * an error.
>> + */
>> + dev->buf_prepare_error = false;
>> + return -EINVAL;
>> + }
>> + if (vb2_plane_size(vb, 0) < size) {
>> + dprintk(dev, 1, "%s data will not fit into plane (%lu < %u)\n",
>> + __func__, vb2_plane_size(vb, 0), size);
>> + return -EINVAL;
>> + }
>> + vb2_set_plane_payload(vb, 0, size);
>> +
>> + return 0;
>> +}
>> +
>> +static void meta_out_buf_queue(struct vb2_buffer *vb)
>> +{
>> + struct vb2_v4l2_buffer *vbuf = to_vb2_v4l2_buffer(vb);
>> + struct vivid_dev *dev = vb2_get_drv_priv(vb->vb2_queue);
>> + struct vivid_buffer *buf = container_of(vbuf, struct vivid_buffer, vb);
>> +
>> + dprintk(dev, 1, "%s\n", __func__);
>> +
>> + spin_lock(&dev->slock);
>> + list_add_tail(&buf->list, &dev->meta_out_active);
>> + spin_unlock(&dev->slock);
>> +}
>> +
>> +static int meta_out_start_streaming(struct vb2_queue *vq, unsigned int count)
>> +{
>> + struct vivid_dev *dev = vb2_get_drv_priv(vq);
>> + int err;
>> +
>> + dprintk(dev, 1, "%s\n", __func__);
>> + dev->meta_out_seq_count = 0;
>> + if (dev->start_streaming_error) {
>> + dev->start_streaming_error = false;
>> + err = -EINVAL;
>
> Just add goto err:
>
>> + } else {
>> + err = vivid_start_generating_vid_out(dev,
>> + &dev->meta_out_streaming);
>> + }
>
> You don't need these braces around else block. Didn't checkpatch tell
> you that. I thought it would.
Actually, if one part of a 'if'...'else' needs braces, then you should put
them around both. checkpatch only warns if both parts are single statements.
>
> You can simplify this logic by adding goto err: You don't even need
> the else.
>
> make this err:
>
>> + if (err) {
>> + struct vivid_buffer *buf, *tmp;
>> +
>> + list_for_each_entry_safe(buf, tmp,
>> + &dev->meta_out_active, list) {
>> + list_del(&buf->list);
>> + vb2_buffer_done(&buf->vb.vb2_buf,
>> + VB2_BUF_STATE_QUEUED);
>> + }
>> + }
>> + return err;
>> +}
>> +
>> +/* abort streaming and wait for last buffer */
>> +static void meta_out_stop_streaming(struct vb2_queue *vq)
>> +{
>> + struct vivid_dev *dev = vb2_get_drv_priv(vq);
>> +
>> + dprintk(dev, 1, "%s\n", __func__);
>> + vivid_stop_generating_vid_out(dev, &dev->meta_out_streaming);
>> +}
>> +
>> +static void meta_out_buf_request_complete(struct vb2_buffer *vb)
>> +{
>> + struct vivid_dev *dev = vb2_get_drv_priv(vb->vb2_queue);
>> +
>> + v4l2_ctrl_request_complete(vb->req_obj.req, &dev->ctrl_hdl_meta_out);
>> +}
>> +
>> +const struct vb2_ops vivid_meta_out_qops = {
>> + .queue_setup = meta_out_queue_setup,
>> + .buf_prepare = meta_out_buf_prepare,
>> + .buf_queue = meta_out_buf_queue,
>> + .start_streaming = meta_out_start_streaming,
>> + .stop_streaming = meta_out_stop_streaming,
>> + .buf_request_complete = meta_out_buf_request_complete,
>> + .wait_prepare = vb2_ops_wait_prepare,
>> + .wait_finish = vb2_ops_wait_finish,
>> +};
>> +
>> +int vidioc_enum_fmt_meta_out(struct file *file, void *priv,
>> + struct v4l2_fmtdesc *f)
>> +{
>> + struct vivid_dev *dev = video_drvdata(file);
>> +
>> + if (!vivid_is_webcam(dev))
>> + return -EINVAL;
>> +
>> + if (f->index > 0)
>> + return -EINVAL;
>
> You could collapase these two into one single check.
I don't have a strong opinion on this. Very different things are tested here,
so I am OK with two 'if's.
>
>> +
>> + f->type = V4L2_BUF_TYPE_META_OUTPUT;
>> + f->pixelformat = V4L2_META_FMT_VIVID;/* V4L2_META_FMT_UVC */
>> + return 0;
>> +}
>> +
>> +int vidioc_g_fmt_meta_out(struct file *file, void *priv,
>> + struct v4l2_format *f)
>> +{
>> + struct vivid_dev *dev = video_drvdata(file);
>> + struct v4l2_meta_format *meta = &f->fmt.meta;
>> +
>> + if (!vivid_is_webcam(dev) || !dev->has_meta_out)
>> + return -EINVAL;
>> +
>> + meta->dataformat = V4L2_META_FMT_VIVID;
>> + meta->buffersize = sizeof(struct vivid_meta_out_buf);
>> + return 0;
>> +}
>> +
>> +void vivid_meta_out_process(struct vivid_dev *dev,
>> + struct vivid_buffer *buf)
>> +{
>> + struct vivid_meta_out_buf *meta = vb2_plane_vaddr(&buf->vb.vb2_buf, 0);
>> +
>> + tpg_s_brightness(&dev->tpg, meta->brightness);
>> + tpg_s_contrast(&dev->tpg, meta->contrast);
>> + tpg_s_saturation(&dev->tpg, meta->saturation);
>> + tpg_s_hue(&dev->tpg, meta->hue);
>> + dprintk(dev, 2, " %s brightness %u contrast %u saturation %u hue %d\n",
>> + __func__, meta->brightness, meta->contrast,
>> + meta->saturation, meta->hue);
>> +}
>> diff --git a/drivers/media/platform/vivid/vivid-meta-out.h b/drivers/media/platform/vivid/vivid-meta-out.h
>> new file mode 100644
>> index 000000000000..0c639b7c2842
>> --- /dev/null
>> +++ b/drivers/media/platform/vivid/vivid-meta-out.h
>> @@ -0,0 +1,25 @@
>> +/* SPDX-License-Identifier: GPL-2.0-only */
>> +/*
>> + * vivid-meta-out.h - meta output support functions.
>> + */
>> +#ifndef _VIVID_META_OUT_H_
>> +#define _VIVID_META_OUT_H_
>> +
>> +struct vivid_meta_out_buf {
>> + u16 brightness;
>> + u16 contrast;
>> + u16 saturation;
>> + s16 hue;
>> +};
>> +
>> +void vivid_meta_out_process(struct vivid_dev *dev, struct vivid_buffer *buf);
>> +int vidioc_enum_fmt_meta_out(struct file *file, void *priv,
>> + struct v4l2_fmtdesc *f);
>> +int vidioc_g_fmt_meta_out(struct file *file, void *priv,
>> + struct v4l2_format *f);
>> +int vidioc_s_fmt_meta_out(struct file *file, void *priv,
>> + struct v4l2_format *f);
>> +
>> +extern const struct vb2_ops vivid_meta_out_qops;
>> +
>> +#endif
>> diff --git a/drivers/media/platform/vivid/vivid-vid-out.c b/drivers/media/platform/vivid/vivid-vid-out.c
>> index a0364ac497f9..ee3446e3217c 100644
>> --- a/drivers/media/platform/vivid/vivid-vid-out.c
>> +++ b/drivers/media/platform/vivid/vivid-vid-out.c
>> @@ -1079,7 +1079,9 @@ int vidioc_s_output(struct file *file, void *priv, unsigned o)
>> if (o == dev->output)
>> return 0;
>>
>> - if (vb2_is_busy(&dev->vb_vid_out_q) || vb2_is_busy(&dev->vb_vbi_out_q))
>> + if (vb2_is_busy(&dev->vb_vid_out_q) ||
>> + vb2_is_busy(&dev->vb_vbi_out_q) ||
>> + vb2_is_busy(&dev->vb_meta_out_q))
>> return -EBUSY;
>>
>> dev->output = o;
>> @@ -1090,6 +1092,7 @@ int vidioc_s_output(struct file *file, void *priv, unsigned o)
>> dev->vid_out_dev.tvnorms = 0;
>>
>> dev->vbi_out_dev.tvnorms = dev->vid_out_dev.tvnorms;
>> + dev->meta_out_dev.tvnorms = dev->vid_out_dev.tvnorms;
>> vivid_update_format_out(dev);
>>
>> v4l2_ctrl_activate(dev->ctrl_display_present, vivid_is_hdmi_out(dev));
>>
>
> thanks,
> -- Shuah
>
Regards,
Hans
next prev parent reply other threads:[~2019-10-04 9:55 UTC|newest]
Thread overview: 34+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-10-03 7:18 [Linux-kernel-mentees] [PATCH 0/2] vivid: Metadata output support bnvandana
2019-10-03 7:18 ` Vandana BN
2019-10-03 7:18 ` [Linux-kernel-mentees] [PATCH 1/2] v4l2-core: Add new metadata format bnvandana
2019-10-03 7:18 ` Vandana BN
2019-10-03 7:23 ` hverkuil
2019-10-03 7:23 ` Hans Verkuil
2019-10-03 7:24 ` hverkuil
2019-10-03 7:24 ` Hans Verkuil
2019-10-03 11:08 ` [Linux-kernel-mentees] [PATCH v2 0/2] vivid: Metadata output support bnvandana
2019-10-03 11:08 ` Vandana BN
2019-10-03 11:08 ` [Linux-kernel-mentees] [PATCH v2 1/2] v4l2-core: Add new metadata format bnvandana
2019-10-03 11:08 ` Vandana BN
2019-10-03 12:58 ` hverkuil
2019-10-03 12:58 ` Hans Verkuil
2019-10-03 16:22 ` skhan
2019-10-03 16:22 ` Shuah Khan
2019-10-03 11:08 ` [Linux-kernel-mentees] [PATCH v2 2/2] vivid: Add metadata output support bnvandana
2019-10-03 11:08 ` Vandana BN
2019-10-03 13:06 ` hverkuil
2019-10-03 13:06 ` Hans Verkuil
2019-10-03 17:38 ` skhan
2019-10-03 17:38 ` Shuah Khan
2019-10-04 9:55 ` hverkuil [this message]
2019-10-04 9:55 ` Hans Verkuil
2019-10-04 12:08 ` [Linux-kernel-mentees] [PATCH v3 0/2] vivid: Metadata " bnvandana
2019-10-04 12:08 ` Vandana BN
2019-10-04 12:08 ` [Linux-kernel-mentees] [PATCH v3 1/2] v4l2-core: Add new metadata format bnvandana
2019-10-04 12:08 ` Vandana BN
2019-10-04 12:08 ` [Linux-kernel-mentees] [PATCH v3 2/2] vivid: Add metadata output support bnvandana
2019-10-04 12:08 ` Vandana BN
2019-10-07 14:26 ` hverkuil
2019-10-07 14:26 ` Hans Verkuil
2019-10-03 7:18 ` [Linux-kernel-mentees] [PATCH " bnvandana
2019-10-03 7:18 ` Vandana BN
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=787ff1cb-dde3-800a-9dde-68f1db5e4897@xs4all.nl \
--to=linux-kernel-mentees@lists.linux-foundation.org \
/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).