Linux-ARM-Kernel Archive on lore.kernel.org
 help / color / Atom feed
From: Sakari Ailus <sakari.ailus@linux.intel.com>
To: Helen Koike <helen.koike@collabora.com>
Cc: devicetree@vger.kernel.org, eddie.cai.linux@gmail.com,
	kernel@collabora.com, heiko@sntech.de,
	jacob2.chen@rock-chips.com, jeffy.chen@rock-chips.com,
	zyc@rock-chips.com, linux-kernel@vger.kernel.org,
	tfiga@chromium.org, linux-rockchip@lists.infradead.org,
	Allon Huang <allon.huang@rock-chips.com>,
	Jacob Chen <cc@rock-chips.com>,
	hans.verkuil@cisco.com, laurent.pinchart@ideasonboard.com,
	zhengsq@rock-chips.com, mchehab@kernel.org,
	ezequiel@collabora.com, linux-arm-kernel@lists.infradead.org,
	linux-media@vger.kernel.org
Subject: Re: [PATCH v8 05/14] media: rkisp1: add Rockchip ISP1 subdev driver
Date: Thu, 15 Aug 2019 11:24:22 +0300
Message-ID: <20190815082422.GM6133@paasikivi.fi.intel.com> (raw)
In-Reply-To: <da6c1d01-e3f6-ad73-db55-145d7832a665@collabora.com>

Hi Helen,

On Wed, Aug 14, 2019 at 09:58:05PM -0300, Helen Koike wrote:

...

> >> +static int rkisp1_isp_sd_set_fmt(struct v4l2_subdev *sd,
> >> +				 struct v4l2_subdev_pad_config *cfg,
> >> +				 struct v4l2_subdev_format *fmt)
> >> +{
> >> +	struct rkisp1_device *isp_dev = sd_to_isp_dev(sd);
> >> +	struct rkisp1_isp_subdev *isp_sd = &isp_dev->isp_sdev;
> >> +	struct v4l2_mbus_framefmt *mf = &fmt->format;
> >> +
> > 
> > Note that for sub-device nodes, the driver is itself responsible for
> > serialising the access to its data structures.
> 
> But looking at subdev_do_ioctl_lock(), it seems that it serializes the
> ioctl calls for subdevs, no? Or I'm misunderstanding something (which is
> most probably) ?

Good question. I had missed this change --- subdev_do_ioctl_lock() is
relatively new. But setting that lock is still not possible as the struct
is allocated in the framework and the device is registered before the
driver gets hold of it. It's a good idea to provide the same serialisation
for subdevs as well.

I'll get back to this later.

...

> >> +static int rkisp1_isp_sd_s_power(struct v4l2_subdev *sd, int on)
> > 
> > If you support runtime PM, you shouldn't implement the s_power op.
> 
> Is is ok to completly remove the usage of runtime PM then?
> Like this http://ix.io/1RJb ?

Please use runtime PM instead. In the long run we should get rid of the
s_power op. Drivers themselves know better when the hardware they control
should be powered on or off.

> 
> tbh I'm not that familar with runtime PM and I'm not sure what is the
> difference of it and using s_power op (and Documentation/power/runtime_pm.rst
> is not being that helpful tbh).

You can find a simple example e.g. in
drivers/media/platform/atmel/atmel-isi.c .

> 
> > 
> > You'll still need to call s_power on external subdevs though.
> > 
> >> +{
> >> +	struct rkisp1_device *isp_dev = sd_to_isp_dev(sd);
> >> +	int ret;
> >> +
> >> +	v4l2_dbg(1, rkisp1_debug, &isp_dev->v4l2_dev, "s_power: %d\n", on);
> >> +
> >> +	if (on) {
> >> +		ret = pm_runtime_get_sync(isp_dev->dev);
> 
> If this is not ok to remove suport for runtime PM, then where should I put
> the call to pm_runtime_get_sync() if not in this s_power op ?

Basically the runtime_resume and runtime_suspend callbacks are where the
device power state changes are implemented, and pm_runtime_get_sync and
pm_runtime_put are how the driver controls the power state.

So you no longer need the s_power() op at all. The op needs to be called on
the pipeline however, as there are drivers that still use it.

> 
> >> +		if (ret < 0)
> >> +			return ret;
> >> +
> >> +		rkisp1_config_clk(isp_dev);
> >> +	} else {
> >> +		ret = pm_runtime_put(isp_dev->dev);
> >> +		if (ret < 0)
> >> +			return ret;
> >> +	}
> >> +
> >> +	return 0;
> >> +}
> >> +
> >> +static int rkisp1_subdev_link_validate(struct media_link *link)
> >> +{
> >> +	if (link->source->index == RKISP1_ISP_PAD_SINK_PARAMS)
> > 
> > Is this test correct? The source is the source end of the link, i.e. the
> > video node.
> 
> Ah yes, it should be link->sink->index (and not source), thanks for spotting this.
> 
> > 
> > How about the links that end in a video node?
> 
> I thought that the only possibilities were sensor->isp1 and params->isp1 (where params
> is an output video node that should be catched by the corrected version of the if
> statement above.
> 
> Or do you mean another thing?

The link_validate of the sink entity will be called only, for the knowledge
what is possible is generally in that end.

So you'll need this for all the sink pads this driver is in control of.
I suppose this means the sub-devices as well as capture video nodes in
practice.

> 
> > 
> >> +		return 0;
> >> +
> >> +	return v4l2_subdev_link_validate(link);
> >> +}
> >> +
> >> +static int rkisp1_subdev_fmt_link_validate(struct v4l2_subdev *sd,
> >> +					struct media_link *link,
> >> +					struct v4l2_subdev_format *source_fmt,
> >> +					struct v4l2_subdev_format *sink_fmt)
> >> +{
> >> +	if (source_fmt->format.code != sink_fmt->format.code)
> >> +		return -EINVAL;
> 
> ops, should be -EPIPE
> 
> >> +
> >> +	/* Crop is available */
> >> +	if (source_fmt->format.width < sink_fmt->format.width ||
> >> +	    source_fmt->format.height < sink_fmt->format.height)
> >> +		return -EINVAL;
> 
> -EPIPE
> 
> >> +
> > 
> > Could you use v4l2_subdev_link_validate_default()?
> 
> v4l2_subdev_link_validate_default() only allows for an exact width/height match,
> but here we allow the sink to be smaller then the source for cropping, no?

The width and height generally must match over a link. But cropping takes
place inside a sub-device, it is not a concern in link validation as such.

> 
> Thanks again for your review!

You're welcome!

-- 
Kind regards,

Sakari Ailus
sakari.ailus@linux.intel.com

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

  reply index

Thread overview: 52+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-07-30 18:42 [PATCH v8 00/14] Rockchip ISP1 Driver Helen Koike
2019-07-30 18:42 ` [PATCH v8 01/14] media: videodev2.h, v4l2-ioctl: add rkisp1 meta buffer format Helen Koike
2019-08-15 13:30   ` Laurent Pinchart
2019-07-30 18:42 ` [PATCH v8 02/14] media: doc: add document for " Helen Koike
2019-08-07 13:09   ` Sakari Ailus
2019-08-15 13:51   ` Laurent Pinchart
2019-07-30 18:42 ` [PATCH v8 03/14] media: rkisp1: Add user space ABI definitions Helen Koike
2019-08-15 18:46   ` Laurent Pinchart
2019-07-30 18:42 ` [PATCH v8 04/14] media: rkisp1: add Rockchip MIPI Synopsys DPHY driver Helen Koike
2019-08-07 13:05   ` Sakari Ailus
2019-08-07 13:37     ` Helen Koike
2019-08-15 17:54       ` Laurent Pinchart
2019-08-15 18:26         ` Heiko Stübner
2019-08-21 21:46         ` Helen Koike
2019-08-22  2:32           ` Laurent Pinchart
2019-07-30 18:42 ` [PATCH v8 05/14] media: rkisp1: add Rockchip ISP1 subdev driver Helen Koike
2019-08-06 18:51   ` Helen Koike
2019-08-07 10:39     ` Hans Verkuil
2019-08-15 19:35       ` Laurent Pinchart
2019-08-08  9:14   ` Sakari Ailus
2019-08-15  0:58     ` Helen Koike
2019-08-15  8:24       ` Sakari Ailus [this message]
2019-08-15 10:29         ` Tomasz Figa
2019-08-15 10:45           ` Sakari Ailus
2019-08-15 13:17         ` Sakari Ailus
2019-08-16  0:13   ` Laurent Pinchart
2019-07-30 18:42 ` [PATCH v8 06/14] media: rkisp1: add ISP1 statistics driver Helen Koike
2019-08-08  9:37   ` Sakari Ailus
2019-07-30 18:42 ` [PATCH v8 07/14] media: rkisp1: add ISP1 params driver Helen Koike
2019-07-30 18:42 ` [PATCH v8 09/14] media: rkisp1: add rockchip isp1 core driver Helen Koike
2019-08-07 15:27   ` Sakari Ailus
2019-08-08 21:59     ` Helen Koike
2019-08-09 12:05       ` Sakari Ailus
2019-08-07 15:36   ` Sakari Ailus
2019-07-30 18:42 ` [PATCH v8 10/14] dt-bindings: Document the Rockchip ISP1 bindings Helen Koike
2019-08-16  0:21   ` Laurent Pinchart
2019-07-30 18:42 ` [PATCH v8 11/14] dt-bindings: Document the Rockchip MIPI RX D-PHY bindings Helen Koike
2019-08-15 18:14   ` Laurent Pinchart
2019-07-30 18:42 ` [PATCH v8 12/14] arm64: dts: rockchip: add isp0 node for rk3399 Helen Koike
2019-07-30 18:42 ` [PATCH v8 13/14] arm64: dts: rockchip: add rx0 mipi-phy " Helen Koike
2019-07-30 18:42 ` [PATCH v8 14/14] MAINTAINERS: add entry for Rockchip ISP1 driver Helen Koike
2019-08-15 13:56   ` Laurent Pinchart
2019-07-30 20:15 ` [PATCH v8 00/14] Rockchip ISP1 Driver Hans Verkuil
2019-07-30 20:50   ` Helen Koike
2019-07-31  0:08     ` Helen Koike
2019-07-31  4:29       ` Hans Verkuil
2019-07-31  4:33         ` Hans Verkuil
2019-07-31  4:55           ` Hans Verkuil
2019-07-31 14:42             ` Helen Koike
2019-08-07 15:37 ` Sakari Ailus
2019-08-07 17:57   ` Helen Koike
2019-08-09 18:45 ` Manivannan Sadhasivam

Reply instructions:

You may reply publically 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=20190815082422.GM6133@paasikivi.fi.intel.com \
    --to=sakari.ailus@linux.intel.com \
    --cc=allon.huang@rock-chips.com \
    --cc=cc@rock-chips.com \
    --cc=devicetree@vger.kernel.org \
    --cc=eddie.cai.linux@gmail.com \
    --cc=ezequiel@collabora.com \
    --cc=hans.verkuil@cisco.com \
    --cc=heiko@sntech.de \
    --cc=helen.koike@collabora.com \
    --cc=jacob2.chen@rock-chips.com \
    --cc=jeffy.chen@rock-chips.com \
    --cc=kernel@collabora.com \
    --cc=laurent.pinchart@ideasonboard.com \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-media@vger.kernel.org \
    --cc=linux-rockchip@lists.infradead.org \
    --cc=mchehab@kernel.org \
    --cc=tfiga@chromium.org \
    --cc=zhengsq@rock-chips.com \
    --cc=zyc@rock-chips.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

Linux-ARM-Kernel Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-arm-kernel/0 linux-arm-kernel/git/0.git
	git clone --mirror https://lore.kernel.org/linux-arm-kernel/1 linux-arm-kernel/git/1.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-arm-kernel linux-arm-kernel/ https://lore.kernel.org/linux-arm-kernel \
		linux-arm-kernel@lists.infradead.org
	public-inbox-index linux-arm-kernel

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.infradead.lists.linux-arm-kernel


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git