Hi Niklas,
Thank you for the patch.
On Monday, 29 January 2018 18:34:32 EET Niklas Söderlund wrote:
> The procedure to start or stop streaming using the non-MC single
> subdevice and the MC graph and multiple subdevices are quite different.
> Create a new function to abstract which method is used based on which
> mode the driver is running in and add logic to start the MC graph.
>
> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@xxxxxxxxxxxx>
> ---
> drivers/media/platform/rcar-vin/rcar-dma.c | 123 +++++++++++++++++++++++--
> 1 file changed, 116 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/media/platform/rcar-vin/rcar-dma.c
> b/drivers/media/platform/rcar-vin/rcar-dma.c index
> 811d8f8638d21200..6784e7eb3d96e1c0 100644
> --- a/drivers/media/platform/rcar-vin/rcar-dma.c
> +++ b/drivers/media/platform/rcar-vin/rcar-dma.c
> @@ -1087,15 +1087,126 @@ static void rvin_buffer_queue(struct vb2_buffer
> *vb) spin_unlock_irqrestore(&vin->qlock, flags);
> }
>
> +static int rvin_set_stream(struct rvin_dev *vin, int on)
> +{
> + struct v4l2_subdev_format fmt = {
> + .which = V4L2_SUBDEV_FORMAT_ACTIVE,
> + };
> + struct media_pipeline *pipe;
> + struct media_device *mdev;
> + struct v4l2_subdev *sd;
> + struct media_pad *pad;
> + int ret;
> +
> + /* No media controller used, simply pass operation to subdevice */
> + if (!vin->info->use_mc) {
> + ret = v4l2_subdev_call(vin->digital->subdev, video, s_stream,
> + on);
> +
> + return ret == -ENOIOCTLCMD ? 0 : ret;
> + }
> +
> + pad = media_entity_remote_pad(&vin->pad);
> + if (!pad)
> + return -EPIPE;
> +
> + sd = media_entity_to_v4l2_subdev(pad->entity);
> +
> + if (!on) {
> + media_pipeline_stop(&vin->vdev.entity);
> + return v4l2_subdev_call(sd, video, s_stream, 0);
> + }
> +
> + fmt.pad = pad->index;
> + if (v4l2_subdev_call(sd, pad, get_fmt, NULL, &fmt))
> + return -EPIPE;
> +
> + switch (fmt.format.code) {
> + case MEDIA_BUS_FMT_YUYV8_1X16:
> + case MEDIA_BUS_FMT_UYVY8_2X8:
> + case MEDIA_BUS_FMT_UYVY10_2X10:
> + case MEDIA_BUS_FMT_RGB888_1X24:
> + vin->code = fmt.format.code;
> + break;
> + default:
> + return -EPIPE;
> + }
> +
> + switch (fmt.format.field) {
> + case V4L2_FIELD_TOP:
> + case V4L2_FIELD_BOTTOM:
> + case V4L2_FIELD_NONE:
> + case V4L2_FIELD_INTERLACED_TB:
> + case V4L2_FIELD_INTERLACED_BT:
> + case V4L2_FIELD_INTERLACED:
> + case V4L2_FIELD_SEQ_TB:
> + case V4L2_FIELD_SEQ_BT:
> + /* Supported natively */
> + break;
> + case V4L2_FIELD_ALTERNATE:
> + switch (vin->format.field) {
> + case V4L2_FIELD_TOP:
> + case V4L2_FIELD_BOTTOM:
> + case V4L2_FIELD_NONE:
> + break;
> + case V4L2_FIELD_INTERLACED_TB:
> + case V4L2_FIELD_INTERLACED_BT:
> + case V4L2_FIELD_INTERLACED:
> + case V4L2_FIELD_SEQ_TB:
> + case V4L2_FIELD_SEQ_BT:
> + /* Use VIN hardware to combine the two fields */
> + fmt.format.height *= 2;
> + break;
> + default:
> + return -EPIPE;
> + }
> + break;
> + default:
> + return -EPIPE;
> + }
> +
> + if (fmt.format.width != vin->format.width ||
> + fmt.format.height != vin->format.height ||
> + fmt.format.code != vin->code)
> + return -EPIPE;
I'd create a rvin_mc_validate_format() function and move this code there. This
function is growing a bit too big.
> + mdev = vin->vdev.entity.graph_obj.mdev;
> +
> + /*
> + * The graph lock needs to be taken to protect concurrent
> + * starts of multiple VIN instances as they might share
> + * a common subdevice down the line and then should use
> + * the same pipe.
> + */
> + mutex_lock(&mdev->graph_mutex);
I'd say mutex_lock_interruptible(), but videobuf2 won't support that :-S
> + pipe = sd->entity.pipe ? sd->entity.pipe : &vin->vdev.pipe;
> + ret = __media_pipeline_start(&vin->vdev.entity, pipe);
> + mutex_unlock(&mdev->graph_mutex);
> + if (ret)
> + return ret;
> +
> + ret = v4l2_subdev_call(sd, video, s_stream, 1);
> + if (ret == -ENOIOCTLCMD)
> + ret = 0;
> + if (ret)
> + media_pipeline_stop(&vin->vdev.entity);
> +
> + return ret;
> +}
> +
> static int rvin_start_streaming(struct vb2_queue *vq, unsigned int count)
> {
> struct rvin_dev *vin = vb2_get_drv_priv(vq);
> - struct v4l2_subdev *sd;
> unsigned long flags;
> int ret;
>
> - sd = vin_to_source(vin);
> - v4l2_subdev_call(sd, video, s_stream, 1);
> + ret = rvin_set_stream(vin, 1);
> + if (ret) {
> + spin_lock_irqsave(&vin->qlock, flags);
> + return_all_buffers(vin, VB2_BUF_STATE_QUEUED);
> + spin_unlock_irqrestore(&vin->qlock, flags);
> + return ret;
> + }
>
> spin_lock_irqsave(&vin->qlock, flags);
>
> @@ -1104,7 +1215,7 @@ static int rvin_start_streaming(struct vb2_queue *vq,
> unsigned int count) ret = rvin_capture_start(vin);
> if (ret) {
> return_all_buffers(vin, VB2_BUF_STATE_QUEUED);
> - v4l2_subdev_call(sd, video, s_stream, 0);
> + rvin_set_stream(vin, 0);
> }
>
> spin_unlock_irqrestore(&vin->qlock, flags);
> @@ -1115,7 +1226,6 @@ static int rvin_start_streaming(struct vb2_queue *vq,
> unsigned int count) static void rvin_stop_streaming(struct vb2_queue *vq)
> {
> struct rvin_dev *vin = vb2_get_drv_priv(vq);
> - struct v4l2_subdev *sd;
> unsigned long flags;
> int retries = 0;
>
> @@ -1154,8 +1264,7 @@ static void rvin_stop_streaming(struct vb2_queue *vq)
>
> spin_unlock_irqrestore(&vin->qlock, flags);
>
> - sd = vin_to_source(vin);
> - v4l2_subdev_call(sd, video, s_stream, 0);
> + rvin_set_stream(vin, 0);
>
> /* disable interrupts */
> rvin_disable_interrupts(vin);
With this small issue fixed,
Reviewed-by: Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx>
--
Regards,
Laurent Pinchart
