subject:"Re\: \[PATCH v3 17\/24\] media\: imx\: Add CSI subdev driver"

Re: [PATCH v3 17/24] media: imx: Add CSI subdev driver

2017-01-31 Thread Steve Longerbeam




On 01/31/2017 08:48 AM, Philipp Zabel wrote:

On Tue, 2017-01-31 at 16:51 +0100, Philipp Zabel wrote:

On Fri, 2017-01-06 at 18:11 -0800, Steve Longerbeam wrote:
[...]

+static int csi_set_fmt(struct v4l2_subdev *sd,
+  struct v4l2_subdev_pad_config *cfg,
+  struct v4l2_subdev_format *sdformat)
+{
+   struct csi_priv *priv = v4l2_get_subdevdata(sd);
+   struct v4l2_mbus_framefmt *infmt, *outfmt;
+   struct v4l2_rect crop;
+   int ret;
+
+   if (sdformat->pad >= CSI_NUM_PADS)
+   return -EINVAL;
+
+   if (priv->stream_on)
+   return -EBUSY;
+
+   infmt = >format_mbus[priv->input_pad];
+   outfmt = >format_mbus[priv->output_pad];
+
+   if (sdformat->pad == priv->output_pad) {
+   sdformat->format.code = infmt->code;
+   sdformat->format.field = infmt->field;
+   crop.left = priv->crop.left;
+   crop.top = priv->crop.top;
+   crop.width = sdformat->format.width;
+   crop.height = sdformat->format.height;
+   ret = csi_try_crop(priv, );

This is the wrong way around, see also below.

Here the the output sdformat->format.width/height should be set to the
priv->crop.width/height (or priv->crop.width/height / 2, to enable
downscaling). The crop rectangle should not be changed by an output pad
set_fmt.

[...]

The crop rectangle instead should be reset to the full input frame when
the input pad format is set.

How about this:


Thanks for the patch, I'll try it tomorrow.

Steve



--8<--
diff --git a/drivers/staging/media/imx/imx-media-csi.c 
b/drivers/staging/media/imx/imx-media-csi.c
index 5e80a0871b139..8220e4204a125 100644
--- a/drivers/staging/media/imx/imx-media-csi.c
+++ b/drivers/staging/media/imx/imx-media-csi.c
@@ -484,6 +484,8 @@ static int csi_setup(struct csi_priv *priv)
if_fmt.field = outfmt->field;
  
  	ipu_csi_set_window(priv->csi, >crop);

+   ipu_csi_set_downsize(priv->csi, priv->crop.width == 2 * outfmt->width,
+   priv->crop.height == 2 * 
outfmt->height);
  
  	ipu_csi_init_interface(priv->csi, _mbus_cfg, _fmt);
  
@@ -830,15 +832,14 @@ static int csi_set_fmt(struct v4l2_subdev *sd,

switch (sdformat->pad) {
case CSI_SRC_PAD_DIRECT:
case CSI_SRC_PAD_IDMAC:
-   crop.left = priv->crop.left;
-   crop.top = priv->crop.top;
-   crop.width = sdformat->format.width;
-   crop.height = sdformat->format.height;
-   ret = csi_try_crop(priv, , sensor);
-   if (ret)
-   return ret;
-   sdformat->format.width = crop.width;
-   sdformat->format.height = crop.height;
+   if (sdformat->format.width < priv->crop.width * 3 / 4)
+   sdformat->format.width = priv->crop.width / 2;
+   else
+   sdformat->format.width = priv->crop.width;
+   if (sdformat->format.height < priv->crop.height * 3 / 4)
+   sdformat->format.height = priv->crop.height / 2;
+   else
+   sdformat->format.height = priv->crop.height;
  
  		if (sdformat->pad == CSI_SRC_PAD_IDMAC) {

cc = imx_media_find_format(0, sdformat->format.code,
@@ -887,6 +888,14 @@ static int csi_set_fmt(struct v4l2_subdev *sd,
}
break;
case CSI_SINK_PAD:
+   crop.left = 0;
+   crop.top = 0;
+   crop.width = sdformat->format.width;
+   crop.height = sdformat->format.height;
+   ret = csi_try_crop(priv, , sensor);
+   if (ret)
+   return ret;
+
cc = imx_media_find_format(0, sdformat->format.code,
   true, false);
if (!cc) {
@@ -904,9 +913,8 @@ static int csi_set_fmt(struct v4l2_subdev *sd,
} else {
priv->format_mbus[sdformat->pad] = sdformat->format;
priv->cc[sdformat->pad] = cc;
-   /* Update the crop window if this is an output pad  */
-   if (sdformat->pad == CSI_SRC_PAD_DIRECT ||
-   sdformat->pad == CSI_SRC_PAD_IDMAC)
+   /* Reset the crop window if this is the input pad  */
+   if (sdformat->pad == CSI_SINK_PAD)
priv->crop = crop;
}
  
-->8--


regards
Philipp


___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v3 17/24] media: imx: Add CSI subdev driver

2017-01-31 Thread Russell King - ARM Linux

On Tue, Jan 31, 2017 at 04:31:55PM -0800, Steve Longerbeam wrote:
> 
> 
> On 01/31/2017 03:20 AM, Russell King - ARM Linux wrote:
> >On Fri, Jan 06, 2017 at 06:11:35PM -0800, Steve Longerbeam wrote:
> >>+static int csi_link_validate(struct v4l2_subdev *sd,
> >>+struct media_link *link,
> >>+struct v4l2_subdev_format *source_fmt,
> >>+struct v4l2_subdev_format *sink_fmt)
> >>+{
> >>+   struct csi_priv *priv = v4l2_get_subdevdata(sd);
> >>+   bool is_csi2;
> >>+   int ret;
> >>+
> >>+   ret = v4l2_subdev_link_validate_default(sd, link, source_fmt, sink_fmt);
> >>+   if (ret)
> >>+   return ret;
> >>+
> >>+   priv->sensor = __imx_media_find_sensor(priv->md, >sd.entity);
> >>+   if (IS_ERR(priv->sensor)) {
> >>+   v4l2_err(>sd, "no sensor attached\n");
> >>+   ret = PTR_ERR(priv->sensor);
> >>+   priv->sensor = NULL;
> >>+   return ret;
> >>+   }
> >>+
> >>+   ret = v4l2_subdev_call(priv->sensor->sd, video, g_mbus_config,
> >>+  >sensor_mbus_cfg);
> >>+   if (ret)
> >>+   return ret;
> >>+
> >>+   is_csi2 = (priv->sensor_mbus_cfg.type == V4L2_MBUS_CSI2);
> >>+
> >>+   if (is_csi2) {
> >>+   int vc_num = 0;
> >>+   /*
> >>+* NOTE! It seems the virtual channels from the mipi csi-2
> >>+* receiver are used only for routing by the video mux's,
> >>+* or for hard-wired routing to the CSI's. Once the stream
> >>+* enters the CSI's however, they are treated internally
> >>+* in the IPU as virtual channel 0.
> >>+*/
> >>+#if 0
> >>+   vc_num = imx_media_find_mipi_csi2_channel(priv->md,
> >>+ >sd.entity);
> >>+   if (vc_num < 0)
> >>+   return vc_num;
> >>+#endif
> >>+   ipu_csi_set_mipi_datatype(priv->csi, vc_num,
> >>+ >format_mbus[priv->input_pad]);
> >This seems (at least to me) to go against the spirit of the subdev
> >negotiation.  Here, you seem to bypass the negotiation performed
> >between the CSI and the attached device, wanting to grab the
> >format from the CSI2 sensor directly.  That bypasses negotiation
> >performed at the CSI2 subdev and video muxes.
> 
> This isn't so much grabbing a pad format, it is determining
> which source pad at the imx6-mipi-csi2 receiver subdev is
> reached from this CSI (which determines the virtual channel
> the CSI is receiving).
> 
> If there was a way to determine the vc# via a status register
> in the CSI, that would be perfect, but there isn't. In fact, the CSIs
> seem to be ignoring the meta-data bus sent by the CSI2IPU gasket
> which contains this info, or that bus is not being routed to the CSIs.
> As the note says, the CSIs only accept vc0 at the CSI_MIPI_DI register.
> Any other value programmed there results in no data from the CSI.
> 
> And even the presence of CSI_MIPI_DI register makes no sense to me,
> the CSIs are given the vc# and MIPI datatype from the CSI2IPU meta-data
> bus, so I don't understand why it needs to be told.

The CSI_MIPI_DI register selects the data stream we want from the
CSI2 camera.

CSI2 cameras can produce many different data streams - for example,
a CSI2 camera can, for the same image, produce a YUV encoded frame
and a jpeg-encoded frame.  These are sent over the CSI2 serial bus
using two different data types.

The CSI2IPU converts the serial data streams into a parallel data
stream, feeding that into the CSI layer.  The CSI layer, in
conjunction with the CSI_MIPI_DI register, selects one of these
streams to pass on to the SMFC and other blocks.

>From what I've read, the CSI can also be programmed to pass other
streams on as well, but I've never tried that.

In my particular case, the IMX219 camera produces a complete frame
using the RAW8 or RAW10 MIPI data type, and also produces two lines
of register data per frame, encoded using another data type.  I
think it should be possible to program the CSI to pass this other
data on as "generic data" through a different FIFO and have it
written to memory, which makes it possible to know the camera
settings for each frame.  (This isn't something I'm interested in
though, I'm just using it as an example of why, possibly, there's
that register in the CSI block.)

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v3 17/24] media: imx: Add CSI subdev driver

2017-01-31 Thread Steve Longerbeam




On 01/31/2017 03:20 AM, Russell King - ARM Linux wrote:

On Fri, Jan 06, 2017 at 06:11:35PM -0800, Steve Longerbeam wrote:

+static int csi_link_validate(struct v4l2_subdev *sd,
+struct media_link *link,
+struct v4l2_subdev_format *source_fmt,
+struct v4l2_subdev_format *sink_fmt)
+{
+   struct csi_priv *priv = v4l2_get_subdevdata(sd);
+   bool is_csi2;
+   int ret;
+
+   ret = v4l2_subdev_link_validate_default(sd, link, source_fmt, sink_fmt);
+   if (ret)
+   return ret;
+
+   priv->sensor = __imx_media_find_sensor(priv->md, >sd.entity);
+   if (IS_ERR(priv->sensor)) {
+   v4l2_err(>sd, "no sensor attached\n");
+   ret = PTR_ERR(priv->sensor);
+   priv->sensor = NULL;
+   return ret;
+   }
+
+   ret = v4l2_subdev_call(priv->sensor->sd, video, g_mbus_config,
+  >sensor_mbus_cfg);
+   if (ret)
+   return ret;
+
+   is_csi2 = (priv->sensor_mbus_cfg.type == V4L2_MBUS_CSI2);
+
+   if (is_csi2) {
+   int vc_num = 0;
+   /*
+* NOTE! It seems the virtual channels from the mipi csi-2
+* receiver are used only for routing by the video mux's,
+* or for hard-wired routing to the CSI's. Once the stream
+* enters the CSI's however, they are treated internally
+* in the IPU as virtual channel 0.
+*/
+#if 0
+   vc_num = imx_media_find_mipi_csi2_channel(priv->md,
+ >sd.entity);
+   if (vc_num < 0)
+   return vc_num;
+#endif
+   ipu_csi_set_mipi_datatype(priv->csi, vc_num,
+ >format_mbus[priv->input_pad]);

This seems (at least to me) to go against the spirit of the subdev
negotiation.  Here, you seem to bypass the negotiation performed
between the CSI and the attached device, wanting to grab the
format from the CSI2 sensor directly.  That bypasses negotiation
performed at the CSI2 subdev and video muxes.


This isn't so much grabbing a pad format, it is determining
which source pad at the imx6-mipi-csi2 receiver subdev is
reached from this CSI (which determines the virtual channel
the CSI is receiving).

If there was a way to determine the vc# via a status register
in the CSI, that would be perfect, but there isn't. In fact, the CSIs
seem to be ignoring the meta-data bus sent by the CSI2IPU gasket
which contains this info, or that bus is not being routed to the CSIs.
As the note says, the CSIs only accept vc0 at the CSI_MIPI_DI register.
Any other value programmed there results in no data from the CSI.

And even the presence of CSI_MIPI_DI register makes no sense to me,
the CSIs are given the vc# and MIPI datatype from the CSI2IPU meta-data
bus, so I don't understand why it needs to be told.

Anyway I can just yank this disabled code, it's only there for documentation
purposes.




The same goes for the frame rate monitoring code - imho, the frame
rate should be something that results from negotiation with the
neighbouring element, not by poking at some entity that is several
entities away.


I will fix this once the g_frame_interval op is implemented in every
subdev. I believe you mentioned this already, but g_frame_interval
will need to be chained until it reaches the originating sensor.

Steve


___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v3 17/24] media: imx: Add CSI subdev driver

2017-01-31 Thread Philipp Zabel

On Tue, 2017-01-31 at 16:51 +0100, Philipp Zabel wrote:
> On Fri, 2017-01-06 at 18:11 -0800, Steve Longerbeam wrote:
> [...]
> > +static int csi_set_fmt(struct v4l2_subdev *sd,
> > +  struct v4l2_subdev_pad_config *cfg,
> > +  struct v4l2_subdev_format *sdformat)
> > +{
> > +   struct csi_priv *priv = v4l2_get_subdevdata(sd);
> > +   struct v4l2_mbus_framefmt *infmt, *outfmt;
> > +   struct v4l2_rect crop;
> > +   int ret;
> > +
> > +   if (sdformat->pad >= CSI_NUM_PADS)
> > +   return -EINVAL;
> > +
> > +   if (priv->stream_on)
> > +   return -EBUSY;
> > +
> > +   infmt = >format_mbus[priv->input_pad];
> > +   outfmt = >format_mbus[priv->output_pad];
> > +
> > +   if (sdformat->pad == priv->output_pad) {
> > +   sdformat->format.code = infmt->code;
> > +   sdformat->format.field = infmt->field;
> > +   crop.left = priv->crop.left;
> > +   crop.top = priv->crop.top;
> > +   crop.width = sdformat->format.width;
> > +   crop.height = sdformat->format.height;
> > +   ret = csi_try_crop(priv, );
> 
> This is the wrong way around, see also below.
> 
> Here the the output sdformat->format.width/height should be set to the
> priv->crop.width/height (or priv->crop.width/height / 2, to enable
> downscaling). The crop rectangle should not be changed by an output pad
> set_fmt.
[...]
> The crop rectangle instead should be reset to the full input frame when
> the input pad format is set.

How about this:

--8<--
diff --git a/drivers/staging/media/imx/imx-media-csi.c 
b/drivers/staging/media/imx/imx-media-csi.c
index 5e80a0871b139..8220e4204a125 100644
--- a/drivers/staging/media/imx/imx-media-csi.c
+++ b/drivers/staging/media/imx/imx-media-csi.c
@@ -484,6 +484,8 @@ static int csi_setup(struct csi_priv *priv)
if_fmt.field = outfmt->field;
 
ipu_csi_set_window(priv->csi, >crop);
+   ipu_csi_set_downsize(priv->csi, priv->crop.width == 2 * outfmt->width,
+   priv->crop.height == 2 * 
outfmt->height);
 
ipu_csi_init_interface(priv->csi, _mbus_cfg, _fmt);
 
@@ -830,15 +832,14 @@ static int csi_set_fmt(struct v4l2_subdev *sd,
switch (sdformat->pad) {
case CSI_SRC_PAD_DIRECT:
case CSI_SRC_PAD_IDMAC:
-   crop.left = priv->crop.left;
-   crop.top = priv->crop.top;
-   crop.width = sdformat->format.width;
-   crop.height = sdformat->format.height;
-   ret = csi_try_crop(priv, , sensor);
-   if (ret)
-   return ret;
-   sdformat->format.width = crop.width;
-   sdformat->format.height = crop.height;
+   if (sdformat->format.width < priv->crop.width * 3 / 4)
+   sdformat->format.width = priv->crop.width / 2;
+   else
+   sdformat->format.width = priv->crop.width;
+   if (sdformat->format.height < priv->crop.height * 3 / 4)
+   sdformat->format.height = priv->crop.height / 2;
+   else
+   sdformat->format.height = priv->crop.height;
 
if (sdformat->pad == CSI_SRC_PAD_IDMAC) {
cc = imx_media_find_format(0, sdformat->format.code,
@@ -887,6 +888,14 @@ static int csi_set_fmt(struct v4l2_subdev *sd,
}
break;
case CSI_SINK_PAD:
+   crop.left = 0;
+   crop.top = 0;
+   crop.width = sdformat->format.width;
+   crop.height = sdformat->format.height;
+   ret = csi_try_crop(priv, , sensor);
+   if (ret)
+   return ret;
+
cc = imx_media_find_format(0, sdformat->format.code,
   true, false);
if (!cc) {
@@ -904,9 +913,8 @@ static int csi_set_fmt(struct v4l2_subdev *sd,
} else {
priv->format_mbus[sdformat->pad] = sdformat->format;
priv->cc[sdformat->pad] = cc;
-   /* Update the crop window if this is an output pad  */
-   if (sdformat->pad == CSI_SRC_PAD_DIRECT ||
-   sdformat->pad == CSI_SRC_PAD_IDMAC)
+   /* Reset the crop window if this is the input pad  */
+   if (sdformat->pad == CSI_SINK_PAD)
priv->crop = crop;
}
 
-->8--

regards
Philipp
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v3 17/24] media: imx: Add CSI subdev driver

2017-01-31 Thread Philipp Zabel

On Fri, 2017-01-06 at 18:11 -0800, Steve Longerbeam wrote:
[...]
> +static int csi_set_fmt(struct v4l2_subdev *sd,
> +struct v4l2_subdev_pad_config *cfg,
> +struct v4l2_subdev_format *sdformat)
> +{
> + struct csi_priv *priv = v4l2_get_subdevdata(sd);
> + struct v4l2_mbus_framefmt *infmt, *outfmt;
> + struct v4l2_rect crop;
> + int ret;
> +
> + if (sdformat->pad >= CSI_NUM_PADS)
> + return -EINVAL;
> +
> + if (priv->stream_on)
> + return -EBUSY;
> +
> + infmt = >format_mbus[priv->input_pad];
> + outfmt = >format_mbus[priv->output_pad];
> +
> + if (sdformat->pad == priv->output_pad) {
> + sdformat->format.code = infmt->code;
> + sdformat->format.field = infmt->field;
> + crop.left = priv->crop.left;
> + crop.top = priv->crop.top;
> + crop.width = sdformat->format.width;
> + crop.height = sdformat->format.height;
> + ret = csi_try_crop(priv, );

This is the wrong way around, see also below.

Here the the output sdformat->format.width/height should be set to the
priv->crop.width/height (or priv->crop.width/height / 2, to enable
downscaling). The crop rectangle should not be changed by an output pad
set_fmt.

> + if (ret)
> + return ret;
> + sdformat->format.width = crop.width;
> + sdformat->format.height = crop.height;
> + }
> +
> + if (sdformat->which == V4L2_SUBDEV_FORMAT_TRY) {
> + cfg->try_fmt = sdformat->format;
> + } else {
> + priv->format_mbus[sdformat->pad] = sdformat->format;
> + /* Update the crop window if this is output pad  */
> + if (sdformat->pad == priv->output_pad)
> + priv->crop = crop;

The crop rectangle instead should be reset to the full input frame when
the input pad format is set.

regards
Philipp

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v3 17/24] media: imx: Add CSI subdev driver

2017-01-31 Thread Russell King - ARM Linux

On Fri, Jan 06, 2017 at 06:11:35PM -0800, Steve Longerbeam wrote:
> +static int csi_link_validate(struct v4l2_subdev *sd,
> +  struct media_link *link,
> +  struct v4l2_subdev_format *source_fmt,
> +  struct v4l2_subdev_format *sink_fmt)
> +{
> + struct csi_priv *priv = v4l2_get_subdevdata(sd);
> + bool is_csi2;
> + int ret;
> +
> + ret = v4l2_subdev_link_validate_default(sd, link, source_fmt, sink_fmt);
> + if (ret)
> + return ret;
> +
> + priv->sensor = __imx_media_find_sensor(priv->md, >sd.entity);
> + if (IS_ERR(priv->sensor)) {
> + v4l2_err(>sd, "no sensor attached\n");
> + ret = PTR_ERR(priv->sensor);
> + priv->sensor = NULL;
> + return ret;
> + }
> +
> + ret = v4l2_subdev_call(priv->sensor->sd, video, g_mbus_config,
> +>sensor_mbus_cfg);
> + if (ret)
> + return ret;
> +
> + is_csi2 = (priv->sensor_mbus_cfg.type == V4L2_MBUS_CSI2);
> +
> + if (is_csi2) {
> + int vc_num = 0;
> + /*
> +  * NOTE! It seems the virtual channels from the mipi csi-2
> +  * receiver are used only for routing by the video mux's,
> +  * or for hard-wired routing to the CSI's. Once the stream
> +  * enters the CSI's however, they are treated internally
> +  * in the IPU as virtual channel 0.
> +  */
> +#if 0
> + vc_num = imx_media_find_mipi_csi2_channel(priv->md,
> +   >sd.entity);
> + if (vc_num < 0)
> + return vc_num;
> +#endif
> + ipu_csi_set_mipi_datatype(priv->csi, vc_num,
> +   >format_mbus[priv->input_pad]);

This seems (at least to me) to go against the spirit of the subdev
negotiation.  Here, you seem to bypass the negotiation performed
between the CSI and the attached device, wanting to grab the
format from the CSI2 sensor directly.  That bypasses negotiation
performed at the CSI2 subdev and video muxes.

The same goes for the frame rate monitoring code - imho, the frame
rate should be something that results from negotiation with the
neighbouring element, not by poking at some entity that is several
entities away.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v3 17/24] media: imx: Add CSI subdev driver

2017-01-31 Thread Russell King - ARM Linux

On Fri, Jan 06, 2017 at 06:11:35PM -0800, Steve Longerbeam wrote:
> This is a media entity subdevice for the i.MX Camera
> Serial Interface module.
> 
> Signed-off-by: Steve Longerbeam 
> ---

The lack of s_frame_interval/g_frame_interval in this driver means:

media-ctl -v -d /dev/media1 --set-v4l2 
'"imx6-mipi-csi2":1[fmt:SGBRG8/512x512@1/30]'

Opening media device /dev/media1
Enumerating entities
Found 29 entities
Enumerating pads and links
Setting up format SGBRG8 512x512 on pad imx6-mipi-csi2/1
Format set: SGBRG8 512x512
Setting up frame interval 1/30 on entity imx6-mipi-csi2
Unable to set frame interval: Inappropriate ioctl for device (-25)Unable to 
setup formats: Inappropriate ioctl for device (25)

which causes the setup of the next element in the chain to also fail
(because the above media-ctl command doesn't set the sink on the
ipu1 csi0 mux.)

It seems to me that without the frame interval supported throughout the
chain, there's no way for an application to properly negotiate the
capture parameters via the "try" mechanism, since it has no idea whether
the frame rate it wants is supported throughout the subdev chain.  Eg,
the sensor may be able to do 100fps but there could be something in the
pipeline that restricts it due to bandwidth.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v3 17/24] media: imx: Add CSI subdev driver

2017-01-30 Thread Russell King - ARM Linux

On Fri, Jan 06, 2017 at 06:11:35PM -0800, Steve Longerbeam wrote:
> This is a media entity subdevice for the i.MX Camera
> Serial Interface module.
> 
> Signed-off-by: Steve Longerbeam 

warning: 3 lines add whitespace errors.
Applying: media: imx: Add CSI subdev driver
.git/rebase-apply/patch:38: new blank line at EOF.
+
warning: 1 line adds whitespace errors.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v3 17/24] media: imx: Add CSI subdev driver

2017-01-16 Thread Steve Longerbeam




On 01/16/2017 07:03 AM, Philipp Zabel wrote:

On Fri, 2017-01-06 at 18:11 -0800, Steve Longerbeam wrote:

This is a media entity subdevice for the i.MX Camera
Serial Interface module.

s/Serial/Sensor/


done.


Signed-off-by: Steve Longerbeam 
---
  drivers/staging/media/imx/Kconfig   |  13 +
  drivers/staging/media/imx/Makefile  |   2 +
  drivers/staging/media/imx/imx-csi.c | 644 
  3 files changed, 659 insertions(+)
  create mode 100644 drivers/staging/media/imx/imx-csi.c

diff --git a/drivers/staging/media/imx/Kconfig 
b/drivers/staging/media/imx/Kconfig
index bfde58d..ce2d2c8 100644
--- a/drivers/staging/media/imx/Kconfig
+++ b/drivers/staging/media/imx/Kconfig
@@ -6,3 +6,16 @@ config VIDEO_IMX_MEDIA
  Say yes here to enable support for video4linux media controller
  driver for the i.MX5/6 SOC.
  
+if VIDEO_IMX_MEDIA

+menu "i.MX5/6 Media Sub devices"
+
+config VIDEO_IMX_CAMERA

s/CAMERA/CSI/ ?


done.


+   tristate "i.MX5/6 Camera driver"

i.MX5/6 Camera Sensor Interface driver


done.




+
+struct csi_priv {
+   struct device *dev;
+   struct ipu_soc *ipu;
+   struct imx_media_dev *md;
+   struct v4l2_subdev sd;
+   struct media_pad pad[CSI_NUM_PADS];
+   struct v4l2_mbus_framefmt format_mbus[CSI_NUM_PADS];
+   struct v4l2_mbus_config sensor_mbus_cfg;
+   struct v4l2_rect crop;
+   struct ipu_csi *csi;
+   int csi_id;
+   int input_pad;
+   int output_pad;
+   bool power_on;  /* power is on */
+   bool stream_on; /* streaming is on */
+
+   /* the sink for the captured frames */
+   struct v4l2_subdev *sink_sd;
+   enum ipu_csi_dest dest;
+   struct v4l2_subdev *src_sd;

src_sd is not used except that its presence marks an enabled input link.
-> could be changed to bool.


For now I prefer to keep it a pointer to the src/sink subdevs.
At some point the CSI may have some reason to know the
identity of the source.




+   struct v4l2_ctrl_handler ctrl_hdlr;
+   struct imx_media_fim *fim;
+
+   /* the attached sensor at stream on */
+   struct imx_media_subdev *sensor;
+};
+
+static inline struct csi_priv *sd_to_dev(struct v4l2_subdev *sdev)
+{
+   return container_of(sdev, struct csi_priv, sd);
+}
+
+/* Update the CSI whole sensor and active windows */
+static int csi_setup(struct csi_priv *priv)
+{
+   struct v4l2_mbus_framefmt infmt;
+
+   ipu_csi_set_window(priv->csi, >crop);
+
+   /*
+* the ipu-csi doesn't understand ALTERNATE, but it only
+* needs to know whether the stream is interlaced, so set
+* to INTERLACED if infmt field is ALTERNATE.
+*/
+   infmt = priv->format_mbus[priv->input_pad];
+   if (infmt.field == V4L2_FIELD_ALTERNATE)
+   infmt.field = V4L2_FIELD_INTERLACED;

That should be SEQ_TB/BT depending on video standard.


fixed.


+
+static int csi_s_stream(struct v4l2_subdev *sd, int enable)
+{
+   struct csi_priv *priv = v4l2_get_subdevdata(sd);
+   int ret = 0;
+
+   if (!priv->src_sd || !priv->sink_sd)
+   return -EPIPE;
+
+   v4l2_info(sd, "stream %s\n", enable ? "ON" : "OFF");

These could be silenced a bit.


yeah, I think it is time for that. I've silenced all the
v4l2_info()'s for stream on/off, power on/off, as well
as some others.




+static int csi_s_power(struct v4l2_subdev *sd, int on)
+{
+   struct csi_priv *priv = v4l2_get_subdevdata(sd);
+   int ret = 0;
+
+   v4l2_info(sd, "power %s\n", on ? "ON" : "OFF");
+
+   if (priv->fim && on != priv->power_on)
+   ret = imx_media_fim_set_power(priv->fim, on);
+
+   if (!ret)
+   priv->power_on = on;
+   return ret;
+}

Is this called multiple times? I'd expect a poweron during open and a
poweroff during close, so no need for priv->power_on.


It is actually called multiple times. The s_power subdev callbacks are
made every time there is a new link established, in the imx-media core's
link_notify(), in order to re-establish power to the active subdevs in the
new pipeline.

This might change after I look into using v4l2_pipeline_pm_use().




+static int csi_link_setup(struct media_entity *entity,
+ const struct media_pad *local,
+ const struct media_pad *remote, u32 flags)
+{
+   struct v4l2_subdev *sd = media_entity_to_v4l2_subdev(entity);
+   struct csi_priv *priv = v4l2_get_subdevdata(sd);
+   struct v4l2_subdev *remote_sd;
+
+   dev_dbg(priv->dev, "link setup %s -> %s", remote->entity->name,
+   local->entity->name);
+
+   remote_sd = media_entity_to_v4l2_subdev(remote->entity);
+
+   if (local->flags & MEDIA_PAD_FL_SINK) {
+   if (flags & MEDIA_LNK_FL_ENABLED) {
+   if (priv->src_sd)
+   return -EBUSY;
+   priv->src_sd = remote_sd;
+

Re: [PATCH v3 17/24] media: imx: Add CSI subdev driver

2017-01-16 Thread Philipp Zabel

On Fri, 2017-01-06 at 18:11 -0800, Steve Longerbeam wrote:
> This is a media entity subdevice for the i.MX Camera
> Serial Interface module.

s/Serial/Sensor/

> Signed-off-by: Steve Longerbeam 
> ---
>  drivers/staging/media/imx/Kconfig   |  13 +
>  drivers/staging/media/imx/Makefile  |   2 +
>  drivers/staging/media/imx/imx-csi.c | 644 
> 
>  3 files changed, 659 insertions(+)
>  create mode 100644 drivers/staging/media/imx/imx-csi.c
> 
> diff --git a/drivers/staging/media/imx/Kconfig 
> b/drivers/staging/media/imx/Kconfig
> index bfde58d..ce2d2c8 100644
> --- a/drivers/staging/media/imx/Kconfig
> +++ b/drivers/staging/media/imx/Kconfig
> @@ -6,3 +6,16 @@ config VIDEO_IMX_MEDIA
> Say yes here to enable support for video4linux media controller
> driver for the i.MX5/6 SOC.
>  
> +if VIDEO_IMX_MEDIA
> +menu "i.MX5/6 Media Sub devices"
> +
> +config VIDEO_IMX_CAMERA

s/CAMERA/CSI/ ?

> + tristate "i.MX5/6 Camera driver"

i.MX5/6 Camera Sensor Interface driver

> + depends on VIDEO_IMX_MEDIA && VIDEO_DEV && I2C
> + select VIDEOBUF2_DMA_CONTIG
> + default y
> + ---help---
> +   A video4linux camera capture driver for i.MX5/6.
> +
> +endmenu
> +endif
> diff --git a/drivers/staging/media/imx/Makefile 
> b/drivers/staging/media/imx/Makefile
> index ef9f11b..133672a 100644
> --- a/drivers/staging/media/imx/Makefile
> +++ b/drivers/staging/media/imx/Makefile
> @@ -4,3 +4,5 @@ imx-media-objs := imx-media-dev.o imx-media-fim.o 
> imx-media-internal-sd.o \
>  obj-$(CONFIG_VIDEO_IMX_MEDIA) += imx-media.o
>  obj-$(CONFIG_VIDEO_IMX_MEDIA) += imx-media-common.o
>  
> +obj-$(CONFIG_VIDEO_IMX_CAMERA) += imx-csi.o
> +
> diff --git a/drivers/staging/media/imx/imx-csi.c 
> b/drivers/staging/media/imx/imx-csi.c
> new file mode 100644
> index 000..64ef862
> --- /dev/null
> +++ b/drivers/staging/media/imx/imx-csi.c
> @@ -0,0 +1,644 @@
> +/*
> + * V4L2 Capture CSI Subdev for Freescale i.MX5/6 SOC
> + *
> + * Copyright (c) 2014-2016 Mentor Graphics Inc.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + */
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include "imx-media.h"
> +
> +#define CSI_NUM_PADS 2
> +
> +struct csi_priv {
> + struct device *dev;
> + struct ipu_soc *ipu;
> + struct imx_media_dev *md;
> + struct v4l2_subdev sd;
> + struct media_pad pad[CSI_NUM_PADS];
> + struct v4l2_mbus_framefmt format_mbus[CSI_NUM_PADS];
> + struct v4l2_mbus_config sensor_mbus_cfg;
> + struct v4l2_rect crop;
> + struct ipu_csi *csi;
> + int csi_id;
> + int input_pad;
> + int output_pad;
> + bool power_on;  /* power is on */
> + bool stream_on; /* streaming is on */
> +
> + /* the sink for the captured frames */
> + struct v4l2_subdev *sink_sd;
> + enum ipu_csi_dest dest;
> + struct v4l2_subdev *src_sd;

src_sd is not used except that its presence marks an enabled input link.
-> could be changed to bool.

> + struct v4l2_ctrl_handler ctrl_hdlr;
> + struct imx_media_fim *fim;
> +
> + /* the attached sensor at stream on */
> + struct imx_media_subdev *sensor;
> +};
> +
> +static inline struct csi_priv *sd_to_dev(struct v4l2_subdev *sdev)
> +{
> + return container_of(sdev, struct csi_priv, sd);
> +}
> +
> +/* Update the CSI whole sensor and active windows */
> +static int csi_setup(struct csi_priv *priv)
> +{
> + struct v4l2_mbus_framefmt infmt;
> +
> + ipu_csi_set_window(priv->csi, >crop);
> +
> + /*
> +  * the ipu-csi doesn't understand ALTERNATE, but it only
> +  * needs to know whether the stream is interlaced, so set
> +  * to INTERLACED if infmt field is ALTERNATE.
> +  */
> + infmt = priv->format_mbus[priv->input_pad];
> + if (infmt.field == V4L2_FIELD_ALTERNATE)
> + infmt.field = V4L2_FIELD_INTERLACED;

That should be SEQ_TB/BT depending on video standard.

> + ipu_csi_init_interface(priv->csi, >sensor_mbus_cfg, );
> +
> + ipu_csi_set_dest(priv->csi, priv->dest);
> +
> + ipu_csi_dump(priv->csi);
> +
> + return 0;
> +}
> +
> +static int csi_start(struct csi_priv *priv)
> +{
> + int ret;
> +
> + if (!priv->sensor) {
> + v4l2_err(>sd, "no sensor attached\n");
> + return -EINVAL;
> + }
> +
> + ret = csi_setup(priv);
> + if (ret)
> + return ret;
> +
> + /* start the frame interval monitor */
> + if (priv->fim) {
> + ret = imx_media_fim_set_stream(priv->fim, priv->sensor, true);
> + if (ret)
> + return ret;
> + }
> +
> + ret = ipu_csi_enable(priv->csi);
> + if (ret) {
> +

Re: [PATCH v3 17/24] media: imx: Add CSI subdev driver

Re: [PATCH v3 17/24] media: imx: Add CSI subdev driver

Re: [PATCH v3 17/24] media: imx: Add CSI subdev driver

Re: [PATCH v3 17/24] media: imx: Add CSI subdev driver

Re: [PATCH v3 17/24] media: imx: Add CSI subdev driver

Re: [PATCH v3 17/24] media: imx: Add CSI subdev driver

Re: [PATCH v3 17/24] media: imx: Add CSI subdev driver

Re: [PATCH v3 17/24] media: imx: Add CSI subdev driver

Re: [PATCH v3 17/24] media: imx: Add CSI subdev driver

Re: [PATCH v3 17/24] media: imx: Add CSI subdev driver

10 matches

Site Navigation

Mail list logo

Footer information