| Message ID | 20171208010842.20047-20-niklas.soderlund+renesas@ragnatech.se (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Niklas, Thank you for the patch. On Friday, 8 December 2017 03:08:33 EET Niklas Söderlund wrote: > When the driver runs in media controller mode it should not directly > control the subdevice instead userspace will be responsible for > configuring the pipeline. To be able to run in this mode a different set > of v4l2 operations needs to be used. > > Add a new set of v4l2 operations to support the running without directly Maybe s/the running/operation/ ? > interacting with the source subdevice. > > Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se> > Reviewed-by: Hans Verkuil <hans.verkuil@cisco.com> > --- > drivers/media/platform/rcar-vin/rcar-dma.c | 3 +- > drivers/media/platform/rcar-vin/rcar-v4l2.c | 155 ++++++++++++++++++++++++- > drivers/media/platform/rcar-vin/rcar-vin.h | 1 + > 3 files changed, 155 insertions(+), 4 deletions(-) > > diff --git a/drivers/media/platform/rcar-vin/rcar-dma.c > b/drivers/media/platform/rcar-vin/rcar-dma.c index > d2788d8bb9565aaa..6c5df13b30d6dd14 100644 > --- a/drivers/media/platform/rcar-vin/rcar-dma.c > +++ b/drivers/media/platform/rcar-vin/rcar-dma.c > @@ -628,7 +628,8 @@ static int rvin_setup(struct rvin_dev *vin) > /* Default to TB */ > vnmc = VNMC_IM_FULL; > /* Use BT if video standard can be read and is 60 Hz format */ > - if (!v4l2_subdev_call(vin_to_source(vin), video, g_std, &std)) { > + if (!vin->info->use_mc && > + !v4l2_subdev_call(vin_to_source(vin), video, g_std, &std)) { > if (std & V4L2_STD_525_60) > vnmc = VNMC_IM_FULL | VNMC_FOC; > } I think the subdev should be queried in rcar-v4l2.c and the information cached in the rvin_dev structure instead of queried here. Interactions with the subdev should be minimized in rvin-dma.c. You can fix this in a separate patch if you prefer. > diff --git a/drivers/media/platform/rcar-vin/rcar-v4l2.c > b/drivers/media/platform/rcar-vin/rcar-v4l2.c index > 0ffbf0c16fb7b00e..5fea2856fd61030f 100644 > --- a/drivers/media/platform/rcar-vin/rcar-v4l2.c > +++ b/drivers/media/platform/rcar-vin/rcar-v4l2.c > @@ -23,6 +23,9 @@ > #include "rcar-vin.h" > > #define RVIN_DEFAULT_FORMAT V4L2_PIX_FMT_YUYV > +#define RVIN_DEFAULT_WIDTH 800 > +#define RVIN_DEFAULT_HEIGHT 600 > +#define RVIN_DEFAULT_COLORSPACE V4L2_COLORSPACE_SRGB > > /* ------------------------------------------------------------------------ > * Format Conversions > @@ -671,6 +674,84 @@ static const struct v4l2_ioctl_ops rvin_ioctl_ops = { > .vidioc_unsubscribe_event = v4l2_event_unsubscribe, > }; > > +/* ------------------------------------------------------------------------ > + * V4L2 Media Controller > + */ > + > +static int __rvin_mc_try_format(struct rvin_dev *vin, > + struct v4l2_pix_format *pix) > +{ > + /* Keep current field if no specific one is asked for */ > + if (pix->field == V4L2_FIELD_ANY) > + pix->field = vin->format.field; You should set a default value instead, trying a format should return the same result regardless of the current state of the device. > + return rvin_format_align(vin, pix); > +} > + > +static int rvin_mc_try_fmt_vid_cap(struct file *file, void *priv, > + struct v4l2_format *f) > +{ > + struct rvin_dev *vin = video_drvdata(file); > + > + return __rvin_mc_try_format(vin, &f->fmt.pix); > +} > + > +static int rvin_mc_s_fmt_vid_cap(struct file *file, void *priv, > + struct v4l2_format *f) > +{ > + struct rvin_dev *vin = video_drvdata(file); > + int ret; > + > + if (vb2_is_busy(&vin->queue)) > + return -EBUSY; > + > + ret = __rvin_mc_try_format(vin, &f->fmt.pix); > + if (ret) > + return ret; > + > + vin->format = f->fmt.pix; > + > + return 0; > +} > + > +static int rvin_mc_enum_input(struct file *file, void *priv, > + struct v4l2_input *i) > +{ > + if (i->index != 0) > + return -EINVAL; > + > + i->type = V4L2_INPUT_TYPE_CAMERA; > + strlcpy(i->name, "Camera", sizeof(i->name)); > + > + return 0; > +} > + > +static const struct v4l2_ioctl_ops rvin_mc_ioctl_ops = { > + .vidioc_querycap = rvin_querycap, > + .vidioc_try_fmt_vid_cap = rvin_mc_try_fmt_vid_cap, > + .vidioc_g_fmt_vid_cap = rvin_g_fmt_vid_cap, > + .vidioc_s_fmt_vid_cap = rvin_mc_s_fmt_vid_cap, > + .vidioc_enum_fmt_vid_cap = rvin_enum_fmt_vid_cap, > + > + .vidioc_enum_input = rvin_mc_enum_input, > + .vidioc_g_input = rvin_g_input, > + .vidioc_s_input = rvin_s_input, The input API makes no sense for MC-based devices. > + .vidioc_reqbufs = vb2_ioctl_reqbufs, > + .vidioc_create_bufs = vb2_ioctl_create_bufs, > + .vidioc_querybuf = vb2_ioctl_querybuf, > + .vidioc_qbuf = vb2_ioctl_qbuf, > + .vidioc_dqbuf = vb2_ioctl_dqbuf, > + .vidioc_expbuf = vb2_ioctl_expbuf, > + .vidioc_prepare_buf = vb2_ioctl_prepare_buf, > + .vidioc_streamon = vb2_ioctl_streamon, > + .vidioc_streamoff = vb2_ioctl_streamoff, > + > + .vidioc_log_status = v4l2_ctrl_log_status, > + .vidioc_subscribe_event = rvin_subscribe_event, > + .vidioc_unsubscribe_event = v4l2_event_unsubscribe, > +}; > + > /* ------------------------------------------------------------------------ > * File Operations > */ > @@ -814,6 +895,60 @@ static const struct v4l2_file_operations rvin_fops = { > .read = vb2_fop_read, > }; > > +/* ------------------------------------------------------------------------ > + * Media controller file Operations I'm personally fine with capitalizing all words in a title, or only the first one, but I don't see a reason to capitalize the first and last only :-) > + */ > + > +static int rvin_mc_open(struct file *file) > +{ > + struct rvin_dev *vin = video_drvdata(file); > + int ret; > + > + mutex_lock(&vin->lock); > + > + file->private_data = vin; > + > + ret = v4l2_fh_open(file); > + if (ret) > + goto unlock; > + > + pm_runtime_get_sync(vin->dev); Can't this function return an error ? > + v4l2_pipeline_pm_use(&vin->vdev.entity, 1); > + > +unlock: > + mutex_unlock(&vin->lock); > + > + return ret; > +} > + > +static int rvin_mc_release(struct file *file) > +{ > + struct rvin_dev *vin = video_drvdata(file); > + int ret; > + > + mutex_lock(&vin->lock); > + > + /* the release helper will cleanup any on-going streaming */ > + ret = _vb2_fop_release(file, NULL); > + > + v4l2_pipeline_pm_use(&vin->vdev.entity, 0); > + pm_runtime_put(vin->dev); > + > + mutex_unlock(&vin->lock); > + > + return ret; > +} > + > +static const struct v4l2_file_operations rvin_mc_fops = { > + .owner = THIS_MODULE, > + .unlocked_ioctl = video_ioctl2, > + .open = rvin_mc_open, > + .release = rvin_mc_release, > + .poll = vb2_fop_poll, > + .mmap = vb2_fop_mmap, > + .read = vb2_fop_read, > +}; > + > void rvin_v4l2_unregister(struct rvin_dev *vin) > { > if (!video_is_registered(&vin->vdev)) > @@ -849,19 +984,33 @@ int rvin_v4l2_register(struct rvin_dev *vin) > vin->v4l2_dev.notify = rvin_notify; > > /* video node */ > - vdev->fops = &rvin_fops; > vdev->v4l2_dev = &vin->v4l2_dev; > vdev->queue = &vin->queue; > snprintf(vdev->name, sizeof(vdev->name), "%s %s", KBUILD_MODNAME, > dev_name(vin->dev)); > vdev->release = video_device_release_empty; > - vdev->ioctl_ops = &rvin_ioctl_ops; > vdev->lock = &vin->lock; > vdev->device_caps = V4L2_CAP_VIDEO_CAPTURE | V4L2_CAP_STREAMING | > V4L2_CAP_READWRITE; > > + /* Set some form of default format */ "Some form of" ? :-) > vin->format.pixelformat = RVIN_DEFAULT_FORMAT; > - rvin_reset_format(vin); > + vin->format.width = RVIN_DEFAULT_WIDTH; > + vin->format.height = RVIN_DEFAULT_HEIGHT; > + vin->format.colorspace = RVIN_DEFAULT_COLORSPACE; > + > + if (vin->info->use_mc) { > + vdev->fops = &rvin_mc_fops; > + vdev->ioctl_ops = &rvin_mc_ioctl_ops; > + } else { > + vdev->fops = &rvin_fops; > + vdev->ioctl_ops = &rvin_ioctl_ops; > + rvin_reset_format(vin); > + } > + > + ret = rvin_format_align(vin, &vin->format); > + if (ret) > + return ret; > > ret = video_register_device(&vin->vdev, VFL_TYPE_GRABBER, -1); > if (ret) { > diff --git a/drivers/media/platform/rcar-vin/rcar-vin.h > b/drivers/media/platform/rcar-vin/rcar-vin.h index > 0747873c2b9cb74c..fd3cd781be0ab1cf 100644 > --- a/drivers/media/platform/rcar-vin/rcar-vin.h > +++ b/drivers/media/platform/rcar-vin/rcar-vin.h > @@ -21,6 +21,7 @@ > #include <media/v4l2-ctrls.h> > #include <media/v4l2-dev.h> > #include <media/v4l2-device.h> > +#include <media/v4l2-mc.h> Can't you include this header in the .c files instead ? Minimizing the headers included by other headers lowers (pre)compilation time. > #include <media/videobuf2-v4l2.h> > > /* Number of HW buffers */
Hi Laurent, >> +static const struct v4l2_ioctl_ops rvin_mc_ioctl_ops = { >> + .vidioc_querycap = rvin_querycap, >> + .vidioc_try_fmt_vid_cap = rvin_mc_try_fmt_vid_cap, >> + .vidioc_g_fmt_vid_cap = rvin_g_fmt_vid_cap, >> + .vidioc_s_fmt_vid_cap = rvin_mc_s_fmt_vid_cap, >> + .vidioc_enum_fmt_vid_cap = rvin_enum_fmt_vid_cap, >> + >> + .vidioc_enum_input = rvin_mc_enum_input, >> + .vidioc_g_input = rvin_g_input, >> + .vidioc_s_input = rvin_s_input, > > The input API makes no sense for MC-based devices. We've had this discussion before: https://patchwork.linuxtv.org/patch/41857/ There was never a v3 of that patch, so nothing was done with it. The issue here is that the spec requires G/S_INPUT to be present for video nodes. There currently is no exception for MC devices. Regards, Hans
Hi Hans, On Friday, 8 December 2017 12:24:26 EET Hans Verkuil wrote: > Hi Laurent, > > >> +static const struct v4l2_ioctl_ops rvin_mc_ioctl_ops = { > >> + .vidioc_querycap = rvin_querycap, > >> + .vidioc_try_fmt_vid_cap = rvin_mc_try_fmt_vid_cap, > >> + .vidioc_g_fmt_vid_cap = rvin_g_fmt_vid_cap, > >> + .vidioc_s_fmt_vid_cap = rvin_mc_s_fmt_vid_cap, > >> + .vidioc_enum_fmt_vid_cap = rvin_enum_fmt_vid_cap, > >> + > >> + .vidioc_enum_input = rvin_mc_enum_input, > >> + .vidioc_g_input = rvin_g_input, > >> + .vidioc_s_input = rvin_s_input, > > > > The input API makes no sense for MC-based devices. > > We've had this discussion before: > > https://patchwork.linuxtv.org/patch/41857/ > > There was never a v3 of that patch, so nothing was done with it. > > The issue here is that the spec requires G/S_INPUT to be present for > video nodes. There currently is no exception for MC devices. I think we both agree that we should fix the spec :-) It shouldn't be a big deal as MC-enabled applications running with an MC-enabled driver don't use the input API anyway.
Hi Laurent, Thanks for your comments. Apart from the issue with the input API Hans pointed which indicates I need to keep that around until it's fixed in the framework I agree with all your comments but one. <snip> On 2017-12-08 12:14:05 +0200, Laurent Pinchart wrote: > > @@ -628,7 +628,8 @@ static int rvin_setup(struct rvin_dev *vin) > > /* Default to TB */ > > vnmc = VNMC_IM_FULL; > > /* Use BT if video standard can be read and is 60 Hz format */ > > - if (!v4l2_subdev_call(vin_to_source(vin), video, g_std, &std)) { > > + if (!vin->info->use_mc && > > + !v4l2_subdev_call(vin_to_source(vin), video, g_std, &std)) { > > if (std & V4L2_STD_525_60) > > vnmc = VNMC_IM_FULL | VNMC_FOC; > > } > > I think the subdev should be queried in rcar-v4l2.c and the information cached > in the rvin_dev structure instead of queried here. Interactions with the > subdev should be minimized in rvin-dma.c. You can fix this in a separate patch > if you prefer. > This can't be a cached value it needs to be read at stream on time. The input source could have changed format, e.g. the user may have disconnected a NTSC source and plugged in a PAL. This is a shame as I otherwise agree with you that interactions with the subdevice should be kept at a minimum. <snip>
Hi Niklas, On Friday, 19 January 2018 02:46:03 EET Niklas Söderlund wrote: > Hi Laurent, > > Thanks for your comments. > > Apart from the issue with the input API Hans pointed which indicates I > need to keep that around until it's fixed in the framework I agree with > all your comments but one. > > <snip> > > On 2017-12-08 12:14:05 +0200, Laurent Pinchart wrote: > >> @@ -628,7 +628,8 @@ static int rvin_setup(struct rvin_dev *vin) > >> /* Default to TB */ > >> vnmc = VNMC_IM_FULL; > >> /* Use BT if video standard can be read and is 60 Hz format */ > >> - if (!v4l2_subdev_call(vin_to_source(vin), video, g_std, &std)) { > >> + if (!vin->info->use_mc && > >> + !v4l2_subdev_call(vin_to_source(vin), video, g_std, &std)) { > >> if (std & V4L2_STD_525_60) > >> vnmc = VNMC_IM_FULL | VNMC_FOC; > >> } > > > > I think the subdev should be queried in rcar-v4l2.c and the information > > cached in the rvin_dev structure instead of queried here. Interactions > > with the subdev should be minimized in rvin-dma.c. You can fix this in a > > separate patch if you prefer. > > This can't be a cached value it needs to be read at stream on time. The > input source could have changed format, e.g. the user may have > disconnected a NTSC source and plugged in a PAL. Please note that in that case the source subdev will not have changed its format yet, it only does so when the s_std operation is called. > This is a shame as I otherwise agree with you that interactions with the > subdevice should be kept at a minimum. > > <snip>
diff --git a/drivers/media/platform/rcar-vin/rcar-dma.c b/drivers/media/platform/rcar-vin/rcar-dma.c index d2788d8bb9565aaa..6c5df13b30d6dd14 100644 --- a/drivers/media/platform/rcar-vin/rcar-dma.c +++ b/drivers/media/platform/rcar-vin/rcar-dma.c @@ -628,7 +628,8 @@ static int rvin_setup(struct rvin_dev *vin) /* Default to TB */ vnmc = VNMC_IM_FULL; /* Use BT if video standard can be read and is 60 Hz format */ - if (!v4l2_subdev_call(vin_to_source(vin), video, g_std, &std)) { + if (!vin->info->use_mc && + !v4l2_subdev_call(vin_to_source(vin), video, g_std, &std)) { if (std & V4L2_STD_525_60) vnmc = VNMC_IM_FULL | VNMC_FOC; } diff --git a/drivers/media/platform/rcar-vin/rcar-v4l2.c b/drivers/media/platform/rcar-vin/rcar-v4l2.c index 0ffbf0c16fb7b00e..5fea2856fd61030f 100644 --- a/drivers/media/platform/rcar-vin/rcar-v4l2.c +++ b/drivers/media/platform/rcar-vin/rcar-v4l2.c @@ -23,6 +23,9 @@ #include "rcar-vin.h" #define RVIN_DEFAULT_FORMAT V4L2_PIX_FMT_YUYV +#define RVIN_DEFAULT_WIDTH 800 +#define RVIN_DEFAULT_HEIGHT 600 +#define RVIN_DEFAULT_COLORSPACE V4L2_COLORSPACE_SRGB /* ----------------------------------------------------------------------------- * Format Conversions @@ -671,6 +674,84 @@ static const struct v4l2_ioctl_ops rvin_ioctl_ops = { .vidioc_unsubscribe_event = v4l2_event_unsubscribe, }; +/* ----------------------------------------------------------------------------- + * V4L2 Media Controller + */ + +static int __rvin_mc_try_format(struct rvin_dev *vin, + struct v4l2_pix_format *pix) +{ + /* Keep current field if no specific one is asked for */ + if (pix->field == V4L2_FIELD_ANY) + pix->field = vin->format.field; + + return rvin_format_align(vin, pix); +} + +static int rvin_mc_try_fmt_vid_cap(struct file *file, void *priv, + struct v4l2_format *f) +{ + struct rvin_dev *vin = video_drvdata(file); + + return __rvin_mc_try_format(vin, &f->fmt.pix); +} + +static int rvin_mc_s_fmt_vid_cap(struct file *file, void *priv, + struct v4l2_format *f) +{ + struct rvin_dev *vin = video_drvdata(file); + int ret; + + if (vb2_is_busy(&vin->queue)) + return -EBUSY; + + ret = __rvin_mc_try_format(vin, &f->fmt.pix); + if (ret) + return ret; + + vin->format = f->fmt.pix; + + return 0; +} + +static int rvin_mc_enum_input(struct file *file, void *priv, + struct v4l2_input *i) +{ + if (i->index != 0) + return -EINVAL; + + i->type = V4L2_INPUT_TYPE_CAMERA; + strlcpy(i->name, "Camera", sizeof(i->name)); + + return 0; +} + +static const struct v4l2_ioctl_ops rvin_mc_ioctl_ops = { + .vidioc_querycap = rvin_querycap, + .vidioc_try_fmt_vid_cap = rvin_mc_try_fmt_vid_cap, + .vidioc_g_fmt_vid_cap = rvin_g_fmt_vid_cap, + .vidioc_s_fmt_vid_cap = rvin_mc_s_fmt_vid_cap, + .vidioc_enum_fmt_vid_cap = rvin_enum_fmt_vid_cap, + + .vidioc_enum_input = rvin_mc_enum_input, + .vidioc_g_input = rvin_g_input, + .vidioc_s_input = rvin_s_input, + + .vidioc_reqbufs = vb2_ioctl_reqbufs, + .vidioc_create_bufs = vb2_ioctl_create_bufs, + .vidioc_querybuf = vb2_ioctl_querybuf, + .vidioc_qbuf = vb2_ioctl_qbuf, + .vidioc_dqbuf = vb2_ioctl_dqbuf, + .vidioc_expbuf = vb2_ioctl_expbuf, + .vidioc_prepare_buf = vb2_ioctl_prepare_buf, + .vidioc_streamon = vb2_ioctl_streamon, + .vidioc_streamoff = vb2_ioctl_streamoff, + + .vidioc_log_status = v4l2_ctrl_log_status, + .vidioc_subscribe_event = rvin_subscribe_event, + .vidioc_unsubscribe_event = v4l2_event_unsubscribe, +}; + /* ----------------------------------------------------------------------------- * File Operations */ @@ -814,6 +895,60 @@ static const struct v4l2_file_operations rvin_fops = { .read = vb2_fop_read, }; +/* ----------------------------------------------------------------------------- + * Media controller file Operations + */ + +static int rvin_mc_open(struct file *file) +{ + struct rvin_dev *vin = video_drvdata(file); + int ret; + + mutex_lock(&vin->lock); + + file->private_data = vin; + + ret = v4l2_fh_open(file); + if (ret) + goto unlock; + + pm_runtime_get_sync(vin->dev); + v4l2_pipeline_pm_use(&vin->vdev.entity, 1); + +unlock: + mutex_unlock(&vin->lock); + + return ret; +} + +static int rvin_mc_release(struct file *file) +{ + struct rvin_dev *vin = video_drvdata(file); + int ret; + + mutex_lock(&vin->lock); + + /* the release helper will cleanup any on-going streaming */ + ret = _vb2_fop_release(file, NULL); + + v4l2_pipeline_pm_use(&vin->vdev.entity, 0); + pm_runtime_put(vin->dev); + + mutex_unlock(&vin->lock); + + return ret; +} + +static const struct v4l2_file_operations rvin_mc_fops = { + .owner = THIS_MODULE, + .unlocked_ioctl = video_ioctl2, + .open = rvin_mc_open, + .release = rvin_mc_release, + .poll = vb2_fop_poll, + .mmap = vb2_fop_mmap, + .read = vb2_fop_read, +}; + void rvin_v4l2_unregister(struct rvin_dev *vin) { if (!video_is_registered(&vin->vdev)) @@ -849,19 +984,33 @@ int rvin_v4l2_register(struct rvin_dev *vin) vin->v4l2_dev.notify = rvin_notify; /* video node */ - vdev->fops = &rvin_fops; vdev->v4l2_dev = &vin->v4l2_dev; vdev->queue = &vin->queue; snprintf(vdev->name, sizeof(vdev->name), "%s %s", KBUILD_MODNAME, dev_name(vin->dev)); vdev->release = video_device_release_empty; - vdev->ioctl_ops = &rvin_ioctl_ops; vdev->lock = &vin->lock; vdev->device_caps = V4L2_CAP_VIDEO_CAPTURE | V4L2_CAP_STREAMING | V4L2_CAP_READWRITE; + /* Set some form of default format */ vin->format.pixelformat = RVIN_DEFAULT_FORMAT; - rvin_reset_format(vin); + vin->format.width = RVIN_DEFAULT_WIDTH; + vin->format.height = RVIN_DEFAULT_HEIGHT; + vin->format.colorspace = RVIN_DEFAULT_COLORSPACE; + + if (vin->info->use_mc) { + vdev->fops = &rvin_mc_fops; + vdev->ioctl_ops = &rvin_mc_ioctl_ops; + } else { + vdev->fops = &rvin_fops; + vdev->ioctl_ops = &rvin_ioctl_ops; + rvin_reset_format(vin); + } + + ret = rvin_format_align(vin, &vin->format); + if (ret) + return ret; ret = video_register_device(&vin->vdev, VFL_TYPE_GRABBER, -1); if (ret) { diff --git a/drivers/media/platform/rcar-vin/rcar-vin.h b/drivers/media/platform/rcar-vin/rcar-vin.h index 0747873c2b9cb74c..fd3cd781be0ab1cf 100644 --- a/drivers/media/platform/rcar-vin/rcar-vin.h +++ b/drivers/media/platform/rcar-vin/rcar-vin.h @@ -21,6 +21,7 @@ #include <media/v4l2-ctrls.h> #include <media/v4l2-dev.h> #include <media/v4l2-device.h> +#include <media/v4l2-mc.h> #include <media/videobuf2-v4l2.h> /* Number of HW buffers */