Received: by 2002:a25:868d:0:0:0:0:0 with SMTP id z13csp1356626ybk; Thu, 21 May 2020 05:05:33 -0700 (PDT) X-Google-Smtp-Source: ABdhPJxScDzr+LDANZntYLONfgHtWfMuI07zRrtiLgHHt1ryeMy2BpDlmvMk7WwpUlfw2zfre45r X-Received: by 2002:a05:6402:30b2:: with SMTP id df18mr7206168edb.323.1590062733703; Thu, 21 May 2020 05:05:33 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1590062733; cv=none; d=google.com; s=arc-20160816; b=MyhqCupzonpgyACbitIOgUtmOrzOzZgRE51MdP6FBgAuC7DtWZyC2zhTlicmOON8GB C/zCfE3ud3K0OTCK6GpZxXt1oDjPSZZXSGCL4AG7/r1oNIB22uUczYcQd0+YaGFsRuRV Ef5f1hVX0nls6b2sZVkzv4Lj/Z0pOway4t3JdD1LSW2B/70ZTIBjegX4RBIJTVDGKFcX WaGu+BUQqAopzSgtcdF/aFLIvAqr3LtrcS6AfyEKbiALuv4Mug0DVqbKZmmLkmCI2qlX rdNDBMIBhCVouqD5hUM/nt0Tl8yBf/WH3raJ72pwEmLnsJ5R6KZ9wHj4ev+q8OF0V1Wv 3SsQ== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:sender:content-transfer-encoding :content-language:in-reply-to:mime-version:user-agent:date :message-id:from:references:cc:to:subject; bh=aAo+jPPe5BUqUl9tikBwU1e8nOV540zkQAHa8wTUjAI=; b=fzuZGSceZVsRwwuXtyRB+8xbzhygD5TPqmRqtXuJc93ah1PRjXKRczqoWKFBNeNthJ OfaMP8bk7KA9qjRd8A7AY6DqQsf28NJuQQsGa7W2J7IMU2icTIW5xRoUUs7YCKe1ZCCm crc9g0grOKOlroIXPRuoNuMDnvk0Tg3BfPhss3y0IBOQ4RDUKTkuoDTCsCgNvh6lU1RP qNKHsAjoYlqlUI5xPpJLcJF5Tp8gCpSe/0qivENXRzdMuQMTY0STfopYHrG90bulMKfE s0qm8AG+1ZSddlMtg4ROnaSTdwJvx/McKDK6+5Js4nQB/Jcjbq+683aan7xMxJP9/qh7 4j0A== ARC-Authentication-Results: i=1; mx.google.com; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=fail (p=NONE sp=NONE dis=NONE) header.from=collabora.com Return-Path: Received: from vger.kernel.org (vger.kernel.org. [23.128.96.18]) by mx.google.com with ESMTP id a3si2867332edx.205.2020.05.21.05.05.09; Thu, 21 May 2020 05:05:33 -0700 (PDT) Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18; Authentication-Results: mx.google.com; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=fail (p=NONE sp=NONE dis=NONE) header.from=collabora.com Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1729287AbgEUMDg (ORCPT + 99 others); Thu, 21 May 2020 08:03:36 -0400 Received: from bhuna.collabora.co.uk ([46.235.227.227]:48312 "EHLO bhuna.collabora.co.uk" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1729100AbgEUMDf (ORCPT ); Thu, 21 May 2020 08:03:35 -0400 Received: from [127.0.0.1] (localhost [127.0.0.1]) (Authenticated sender: dafna) with ESMTPSA id 2E32D2A333F Subject: Re: [PATCH v3 2/4] media: v4l2-common: add helper functions to call s_stream() callbacks To: Helen Koike , linux-media@vger.kernel.org Cc: kernel@collabora.com, linux-kernel@vger.kernel.org, linux-rockchip@lists.infradead.org, hans.verkuil@cisco.com, skhan@linuxfoundation.org, niklas.soderlund@ragnatech.se, mchehab@kernel.org, Laurent Pinchart , Tomasz Figa , Ezequiel Garcia References: <20200415013044.1778572-1-helen.koike@collabora.com> <20200415013044.1778572-3-helen.koike@collabora.com> From: Dafna Hirschfeld Message-ID: <31557623-2b8c-6cfe-19f7-7854ed51bc86@collabora.com> Date: Thu, 21 May 2020 14:03:29 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.7.0 MIME-Version: 1.0 In-Reply-To: <20200415013044.1778572-3-helen.koike@collabora.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hi On 15.04.20 03:30, Helen Koike wrote: > Add v4l2_pipeline_stream_{enable,disable} helper functions to iterate > through the subdevices in a given stream (i.e following links from sink > to source) and call .s_stream() callback. > > Add stream_count on the subdevice object for simultaneous streaming from > different video devices which shares subdevices. > > Prevent calling .s_stream(true) if it was already called previously by > another stream. > > Prevent calling .s_stream(false) from one stream when there are still > others active. > > If .s_stream(true) fails, call .s_stream(false) in the reverse order. > > Signed-off-by: Helen Koike > --- > > Changes in v3: > - re-write helpers to use media walkers as in v1, but with > v4l2_pipeline_subdevs_get() to reverse the order we call s_stream(true) > in subdevices. > - rename size to max_size (and update docs) in v4l2_pipeline_subdevs_get() to > reflect the meaning of the argument. > - add if defined(CONFIG_MEDIA_CONTROLLER) around helpers > > Changes in v2: > - re-write helpers to not use media walkers > > drivers/media/v4l2-core/v4l2-common.c | 125 ++++++++++++++++++++++++++ > include/media/v4l2-common.h | 43 +++++++++ > include/media/v4l2-subdev.h | 2 + > 3 files changed, 170 insertions(+) > > diff --git a/drivers/media/v4l2-core/v4l2-common.c b/drivers/media/v4l2-core/v4l2-common.c > index 9e8eb45a5b03c..2f991a1a57d7c 100644 > --- a/drivers/media/v4l2-core/v4l2-common.c > +++ b/drivers/media/v4l2-core/v4l2-common.c > @@ -442,3 +442,128 @@ int v4l2_fill_pixfmt(struct v4l2_pix_format *pixfmt, u32 pixelformat, > return 0; > } > EXPORT_SYMBOL_GPL(v4l2_fill_pixfmt); > + > +#if defined(CONFIG_MEDIA_CONTROLLER) > + > +/* > + * v4l2_pipeline_subdevs_get - Assemble a list of subdevices in a pipeline > + * @subdevs: the array to be filled. > + * @max_size: max number of elements that can fit in subdevs array. > + * > + * Walk from a video node, following links from sink to source and fill the > + * array with subdevices in the path. > + * The walker performs a depth-first traversal, which means that, in a topology > + * sd1->sd2->sd3->vdev, sd1 will be the first element placed in the array. > + * > + * Return the number of subdevices filled in the array. > + */ > +static int v4l2_pipeline_subdevs_get(struct video_device *vdev, > + struct media_pipeline *pipe, > + struct v4l2_subdev **subdevs, > + unsigned int max_size) > +{ > + struct media_entity *entity = &vdev->entity; > + struct media_device *mdev = entity->graph_obj.mdev; > + int idx = 0; > + int ret; > + > + mutex_lock(&mdev->graph_mutex); > + > + if (!pipe->streaming_count) { > + ret = media_graph_walk_init(&pipe->graph, mdev); > + if (ret) { > + mutex_unlock(&mdev->graph_mutex); > + return ret; > + } > + } > + > + media_graph_walk_start(&pipe->graph, entity); > + > + while ((entity = media_graph_walk_next_stream(&pipe->graph))) { > + if (!is_media_entity_v4l2_subdev(entity)) > + continue; > + if (WARN_ON(idx >= max_size)) { > + mutex_unlock(&mdev->graph_mutex); > + return -EINVAL; > + } > + subdevs[idx++] = media_entity_to_v4l2_subdev(entity); > + } > + > + if (!pipe->streaming_count) > + media_graph_walk_cleanup(&pipe->graph); > + > + mutex_unlock(&mdev->graph_mutex); > + > + return idx; > +} > + > +__must_check int v4l2_pipeline_stream_enable(struct video_device *vdev, > + struct media_pipeline *pipe) > +{ > + struct media_device *mdev = vdev->entity.graph_obj.mdev; > + struct v4l2_subdev *subdevs[MEDIA_ENTITY_ENUM_MAX_DEPTH]; > + struct v4l2_subdev *sd; > + int i, size, ret; > + > + size = v4l2_pipeline_subdevs_get(vdev, pipe, > + subdevs, ARRAY_SIZE(subdevs)); > + if (size <= 0) > + return size; > + > + /* Call s_stream() in reverse order to enable sensors last */ > + for (i = size - 1; i >= 0; i--) { > + sd = subdevs[i]; > + if (sd->stream_count++) > + continue; > + dev_dbg(mdev->dev, > + "enabling stream for '%s'\n", sd->entity.name); > + ret = v4l2_subdev_call(sd, video, s_stream, true); > + if (ret && ret != -ENOIOCTLCMD) { > + sd->stream_count = 0; > + goto err_stream_disable; > + } > + } > + > + return 0; > + > +err_stream_disable: > + for (i++; i < size; i++) { > + sd = subdevs[i]; > + if (--sd->stream_count) > + continue; > + dev_dbg(mdev->dev, > + "disabling stream for '%s'\n", sd->entity.name); > + v4l2_subdev_call(sd, video, s_stream, false); > + }; > + > + return ret; > +} > +EXPORT_SYMBOL_GPL(v4l2_pipeline_stream_enable); > + > +void v4l2_pipeline_stream_disable(struct video_device *vdev, > + struct media_pipeline *pipe) > +{ > + struct media_device *mdev = vdev->entity.graph_obj.mdev; > + struct v4l2_subdev *subdevs[MEDIA_ENTITY_ENUM_MAX_DEPTH]; > + unsigned int i; > + int size; > + > + size = v4l2_pipeline_subdevs_get(vdev, pipe, > + subdevs, ARRAY_SIZE(subdevs)); > + if (WARN_ON(size < 0)) > + return; > + > + /* Call s_stream() in order to disable sensors first */ > + for (i = 0; i < size; i++) { > + struct v4l2_subdev *sd = subdevs[i]; > + > + if (--sd->stream_count) > + continue; > + dev_dbg(mdev->dev, > + "disabling stream for '%s'\n", sd->entity.name); > + v4l2_subdev_call(sd, video, s_stream, false); > + } > +} > +EXPORT_SYMBOL_GPL(v4l2_pipeline_stream_disable); > + > +#endif /* CONFIG_MEDIA_CONTROLLER */ > diff --git a/include/media/v4l2-common.h b/include/media/v4l2-common.h > index 150ee16ebd811..dc46812120cdc 100644 > --- a/include/media/v4l2-common.h > +++ b/include/media/v4l2-common.h > @@ -539,4 +539,47 @@ static inline void v4l2_buffer_set_timestamp(struct v4l2_buffer *buf, > buf->timestamp.tv_usec = ts.tv_nsec / NSEC_PER_USEC; > } > > +#if defined(CONFIG_MEDIA_CONTROLLER) > + > +/** > + * v4l2_pipeline_stream_enable - Call .s_stream(true) callbacks in the stream > + * @vdev: Starting video device. > + * @pipe: Pipeline this entity belongs to. > + * > + * Call .s_stream(true) callback in all the subdevices participating in the > + * stream, i.e. following links from sink to source. > + * > + * .s_stream(true) is also called from sink to source, i.e. in a topology > + * sd1->sd2->sd3->vdev, .s_stream(true) of sd3 is called first. > + * > + * Calls to this function can be nested, in which case the same number of > + * v4l2_pipeline_stream_disable() calls will be required to disable streaming > + * through subdevices in the pipeline. > + * The pipeline pointer must be identical for all nested calls to > + * v4l2_pipeline_stream_enable(). > + */ > +__must_check int v4l2_pipeline_stream_enable(struct video_device *vdev, > + struct media_pipeline *pipe); > + > +/** > + * v4l2_pipeline_stream_disable - Call .s_stream(false) callbacks in the stream > + * @vdev: Starting video device. > + * @pipe: Pipeline this entity belongs to. > + * > + * Call .s_stream(false) callback in all the subdevices participating in the > + * stream, i.e. following links from sink to source. > + * > + * s_stream(false) is called in a reverse order from > + * v4l2_pipeline_stream_enable(), i.e. in a topology sd1->sd2->sd3->vdev, > + * .s_stream(false) of sd1 is called first. > + * > + * If multiple calls to v4l2_pipeline_stream_enable() have been made, the same > + * number of calls to this function are required to disable streaming through > + * subdevices in the pipeline. > + */ > +void v4l2_pipeline_stream_disable(struct video_device *vdev, > + struct media_pipeline *pipe); > + > +#endif /* CONFIG_MEDIA_CONTROLLER */ > + > #endif /* V4L2_COMMON_H_ */ > diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h > index a4848de598521..20f913a9f70c5 100644 > --- a/include/media/v4l2-subdev.h > +++ b/include/media/v4l2-subdev.h > @@ -838,6 +838,7 @@ struct v4l2_subdev_platform_data { > * @subdev_notifier: A sub-device notifier implicitly registered for the sub- > * device using v4l2_device_register_sensor_subdev(). > * @pdata: common part of subdevice platform data > + * @stream_count: Stream count for the subdevice. > * > * Each instance of a subdev driver should create this struct, either > * stand-alone or embedded in a larger struct. > @@ -869,6 +870,7 @@ struct v4l2_subdev { > struct v4l2_async_notifier *notifier; > struct v4l2_async_notifier *subdev_notifier; > struct v4l2_subdev_platform_data *pdata; > + unsigned int stream_count; I wonder if it is worth it to add a new field to v4l2_subdev that is used only for the specific case, it seems that except of rkisp1 and vimc, the other drivers you pointed to that could use the helpers don't need this counter and they call s_stream without checking if it was already called. This counter is updated only by the new introduced functions and not when s_stream is called from other places. Maybe we can add a helper that just return the next subdevice connected to the current entity as a source, and drivers can iterate it and can decide themselves if s_stream should be called or not Thanks, Dafna > }; > > >