From: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
To: Sylwester Nawrocki <s.nawrocki@samsung.com>
Cc: linux-media@vger.kernel.org, hj210.choi@samsung.com,
dh09.lee@samsung.com, a.hajda@samsung.com,
shaik.ameer@samsung.com, arun.kk@samsung.com,
Kyungmin Park <kyungmin.park@samsung.com>
Subject: Re: [PATCH 09/13] media: Change media device link_notify behaviour
Date: Thu, 30 May 2013 04:02:13 +0200 [thread overview]
Message-ID: <3488951.XA3XI6iGCP@avalon> (raw)
In-Reply-To: <1368113805-20233-10-git-send-email-s.nawrocki@samsung.com>
Hi Sylwester,
Thank you for the patch, and sorry for the late reply.
On Thursday 09 May 2013 17:36:41 Sylwester Nawrocki wrote:
> Currently the media device link_notify callback is invoked before the
> actual change of state of a link when the link is being enabled, and
> after the actual change of state when the link is being disabled.
>
> This doesn't allow a media device driver to perform any operations
> on a full graph before a link is disabled, as well as performing
> any tasks on a modified graph right after a link's state is changed.
>
> This patch modifies signature of the link_notify callback. This
> callback is now called always before and after a link's state change.
> To distinguish the notifications a 'notification' argument is added
> to the link_notify callback: MEDIA_DEV_NOTIFY_PRE_LINK_CH indicates
> notification before link's state change and
> MEDIA_DEV_NOTIFY_POST_LINK_CH corresponds to a notification after
> link flags change.
>
> Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
> Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
> ---
> drivers/media/media-entity.c | 18 +++--------
> drivers/media/platform/exynos4-is/media-dev.c | 16 +++++-----
> drivers/media/platform/omap3isp/isp.c | 41 +++++++++++++---------
> include/media/media-device.h | 9 ++++--
> 4 files changed, 46 insertions(+), 38 deletions(-)
>
> diff --git a/drivers/media/media-entity.c b/drivers/media/media-entity.c
> index 7c2b51c..0fcf4c0 100644
> --- a/drivers/media/media-entity.c
> +++ b/drivers/media/media-entity.c
> @@ -547,25 +547,17 @@ int __media_entity_setup_link(struct media_link *link,
> u32 flags)
>
> mdev = source->parent;
>
> - if ((flags & MEDIA_LNK_FL_ENABLED) && mdev->link_notify) {
> - ret = mdev->link_notify(link->source, link->sink,
> - MEDIA_LNK_FL_ENABLED);
> + if (mdev->link_notify) {
> + ret = mdev->link_notify(link, MEDIA_LNK_FL_ENABLED,
> + MEDIA_DEV_NOTIFY_PRE_LINK_CH);
As you correctly pointed out in a self-reply to your patch, you should pass
the flags here instead of MEDIA_LNK_FL_ENABLED.
> if (ret < 0)
> return ret;
> }
>
> ret = __media_entity_setup_link_notify(link, flags);
> - if (ret < 0)
> - goto err;
>
> - if (!(flags & MEDIA_LNK_FL_ENABLED) && mdev->link_notify)
> - mdev->link_notify(link->source, link->sink, 0);
> -
> - return 0;
> -
> -err:
> - if ((flags & MEDIA_LNK_FL_ENABLED) && mdev->link_notify)
> - mdev->link_notify(link->source, link->sink, 0);
> + if (mdev->link_notify)
> + mdev->link_notify(link, flags, MEDIA_DEV_NOTIFY_POST_LINK_CH);
>
> return ret;
> }
> diff --git a/drivers/media/platform/exynos4-is/media-dev.c
> b/drivers/media/platform/exynos4-is/media-dev.c index e95a6d5..ca58dfc
> 100644
> --- a/drivers/media/platform/exynos4-is/media-dev.c
> +++ b/drivers/media/platform/exynos4-is/media-dev.c
> @@ -1274,34 +1274,36 @@ int fimc_md_set_camclk(struct v4l2_subdev *sd, bool
> on) return __fimc_md_set_camclk(fmd, si, on);
> }
>
> -static int fimc_md_link_notify(struct media_pad *source,
> - struct media_pad *sink, u32 flags)
> +static int fimc_md_link_notify(struct media_link *link, unsigned int flags,
> + unsigned int notification)
> {
> + struct media_entity *sink = link->sink->entity;
> struct exynos_video_entity *ve;
> struct video_device *vdev;
> struct fimc_pipeline *pipeline;
> int i, ret = 0;
>
> - if (media_entity_type(sink->entity) != MEDIA_ENT_T_DEVNODE_V4L)
> + if (media_entity_type(sink) != MEDIA_ENT_T_DEVNODE_V4L ||
> + notification == MEDIA_DEV_NOTIFY_LINK_PRE_CH)
Don't you need to call __fimc_pipeline_open() on post-notify instead of pre-
notified below ?
> return 0;
>
> - vdev = media_entity_to_video_device(sink->entity);
> + vdev = media_entity_to_video_device(sink);
> ve = vdev_to_exynos_video_entity(vdev);
> pipeline = to_fimc_pipeline(ve->pipe);
>
> if (!(flags & MEDIA_LNK_FL_ENABLED) && pipeline->subdevs[IDX_SENSOR]) {
> - if (sink->entity->use_count > 0)
> + if (sink->use_count > 0)
> ret = __fimc_pipeline_close(ve->pipe);
>
> for (i = 0; i < IDX_MAX; i++)
> pipeline->subdevs[i] = NULL;
> - } else if (sink->entity->use_count > 0) {
> + } else if (sink->use_count > 0) {
> /*
> * Link activation. Enable power of pipeline elements only if
> * the pipeline is already in use, i.e. its video node is open.
> * Recreate the controls destroyed during the link deactivation.
> */
> - ret = __fimc_pipeline_open(ve->pipe, sink->entity, true);
> + ret = __fimc_pipeline_open(ve->pipe, sink, true);
> }
>
> return ret ? -EPIPE : ret;
> diff --git a/drivers/media/platform/omap3isp/isp.c
> b/drivers/media/platform/omap3isp/isp.c index 6e5ad8e..a762aeb 100644
> --- a/drivers/media/platform/omap3isp/isp.c
> +++ b/drivers/media/platform/omap3isp/isp.c
> @@ -670,9 +670,9 @@ int omap3isp_pipeline_pm_use(struct media_entity
> *entity, int use)
>
> /*
> * isp_pipeline_link_notify - Link management notification callback
> - * @source: Pad at the start of the link
> - * @sink: Pad at the end of the link
> + * @link: The link
> * @flags: New link flags that will be applied
> + * @notification: The link's state change notification type
> (MEDIA_DEV_NOTIFY_*) *
> * React to link management on powered pipelines by updating the use count
> * of all entities in the source and sink sides of the link. Entities are
> * powered
> @@ -682,29 +682,38 @@ int omap3isp_pipeline_pm_use(struct
> media_entity *entity, int use)
> * off is assumed to never fail. This function will not fail for
> * disconnection events.
> */
> -static int isp_pipeline_link_notify(struct media_pad *source,
> - struct media_pad *sink, u32 flags)
> +static int isp_pipeline_link_notify(struct media_link *link, unsigned int
> + flags, unsigned int notification)
> {
> - int source_use = isp_pipeline_pm_use_count(source->entity);
> - int sink_use = isp_pipeline_pm_use_count(sink->entity);
> + struct media_entity *source = link->source->entity;
> + struct media_entity *sink = link->sink->entity;
> + int source_use = isp_pipeline_pm_use_count(source);
> + int sink_use = isp_pipeline_pm_use_count(sink);
> int ret;
>
> - if (!(flags & MEDIA_LNK_FL_ENABLED)) {
> + if (notification == MEDIA_DEV_NOTIFY_PRE_LINK_CH &&
> + !(flags & MEDIA_LNK_FL_ENABLED)) {
Shouldn't the condition be
if (notification == MEDIA_DEV_NOTIFY_PRE_LINK_CH &&
(flags & MEDIA_LNK_FL_ENABLED))
here ? The code currently pre-notifies on link enable and post-notifies on
link disable.
> /* Powering off entities is assumed to never fail. */
> - isp_pipeline_pm_power(source->entity, -sink_use);
> - isp_pipeline_pm_power(sink->entity, -source_use);
> + isp_pipeline_pm_power(source, -sink_use);
> + isp_pipeline_pm_power(sink, -source_use);
> return 0;
> }
>
> - ret = isp_pipeline_pm_power(source->entity, sink_use);
> - if (ret < 0)
> - return ret;
> + if (notification == MEDIA_DEV_NOTIFY_POST_LINK_CH &&
> + (flags & MEDIA_LNK_FL_ENABLED)) {
Similarly, shouldn't this be
if (notification == MEDIA_DEV_NOTIFY_POST_LINK_CH &&
!(flags & MEDIA_LNK_FL_ENABLED))
> - ret = isp_pipeline_pm_power(sink->entity, source_use);
> - if (ret < 0)
> - isp_pipeline_pm_power(source->entity, -sink_use);
> + ret = isp_pipeline_pm_power(source, sink_use);
> + if (ret < 0)
> + return ret;
>
> - return ret;
> + ret = isp_pipeline_pm_power(sink, source_use);
> + if (ret < 0)
> + isp_pipeline_pm_power(source, -sink_use);
> +
> + return ret;
> + }
> +
> + return 0;
> }
>
> /*
> ---------------------------------------------------------------------------
> -- diff --git a/include/media/media-device.h b/include/media/media-device.h
> index eaade98..353c4ee 100644
> --- a/include/media/media-device.h
> +++ b/include/media/media-device.h
> @@ -45,6 +45,7 @@ struct device;
> * @entities: List of registered entities
> * @lock: Entities list lock
> * @graph_mutex: Entities graph operation lock
> + * @link_notify: Link state change notification callback
> *
> * This structure represents an abstract high-level media device. It allows
> easy * access to entities and provides basic media device-level support.
> The @@ -75,10 +76,14 @@ struct media_device {
> /* Serializes graph operations. */
> struct mutex graph_mutex;
>
> - int (*link_notify)(struct media_pad *source,
> - struct media_pad *sink, u32 flags);
> + int (*link_notify)(struct media_link *link, unsigned int flags,
> + unsigned int notification);
> };
>
> +/* Supported link_notify @notification values. */
> +#define MEDIA_DEV_NOTIFY_PRE_LINK_CH 0
> +#define MEDIA_DEV_NOTIFY_POST_LINK_CH 1
> +
> /* media_devnode to media_device */
> #define to_media_device(node) container_of(node, struct media_device,
> devnode)
--
Regards,
Laurent Pinchart
next prev parent reply other threads:[~2013-05-30 2:02 UTC|newest]
Thread overview: 17+ messages / expand[flat|nested] mbox.gz Atom feed top
2013-05-09 15:36 [PATCH RFC 00/13] Media link_notify behaviour change an exynos4-is updates Sylwester Nawrocki
2013-05-09 15:36 ` [PATCH 01/13] exynos4-is: Remove platform_device_id table at fimc-lite driver Sylwester Nawrocki
2013-05-09 15:36 ` [PATCH 02/13] exynos4-is: Correct querycap ioctl handling " Sylwester Nawrocki
2013-05-09 15:36 ` [PATCH 03/13] exynos4-is: Move common functions to a separate module Sylwester Nawrocki
2013-05-09 15:36 ` [PATCH 04/13] exynos4-is: Add struct exynos_video_entity Sylwester Nawrocki
2013-05-09 15:36 ` [PATCH 05/13] exynos4-is: Preserve state of controls between /dev/video open/close Sylwester Nawrocki
2013-05-09 15:36 ` [PATCH 06/13] exynos4-is: Media graph/video device locking rework Sylwester Nawrocki
2013-05-09 15:36 ` [PATCH 07/13] exynos4-is: Do not use asynchronous runtime PM in release fop Sylwester Nawrocki
2013-05-09 15:36 ` [PATCH 08/13] exynos4-is: Use common exynos_media_pipeline data structure Sylwester Nawrocki
2013-05-09 15:36 ` [PATCH 09/13] media: Change media device link_notify behaviour Sylwester Nawrocki
2013-05-28 17:39 ` Sylwester Nawrocki
2013-05-30 2:02 ` Laurent Pinchart [this message]
2013-05-31 10:02 ` Sylwester Nawrocki
2013-05-09 15:36 ` [PATCH 10/13] exynos4-is: Extend link_notify handler to support fimc-is/lite pipelines Sylwester Nawrocki
2013-05-09 15:36 ` [PATCH 11/13] exynos4-is: Fix sensor subdev -> FIMC notification setup Sylwester Nawrocki
2013-05-09 15:36 ` [PATCH 12/13] exynos4-is: Add locking at fimc(-lite) subdev unregistered handler Sylwester Nawrocki
2013-05-09 15:36 ` [PATCH 13/13] exynos4-is: Remove WARN_ON() from __fimc_pipeline_close() Sylwester Nawrocki
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=3488951.XA3XI6iGCP@avalon \
--to=laurent.pinchart@ideasonboard.com \
--cc=a.hajda@samsung.com \
--cc=arun.kk@samsung.com \
--cc=dh09.lee@samsung.com \
--cc=hj210.choi@samsung.com \
--cc=kyungmin.park@samsung.com \
--cc=linux-media@vger.kernel.org \
--cc=s.nawrocki@samsung.com \
--cc=shaik.ameer@samsung.com \
/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