Re: [PATCH/RFC v2 07/15] rcar-csi2: use frame description information to configure CSI-2 bus

[Niklas Söderlund]

Hi Jacopo,

Thanks for your comments.

On 2017-12-15 15:15:31 +0100, jacopo mondi wrote:
> Hi Niklas,
>thanks for the patch!
> 
> On Thu, Dec 14, 2017 at 08:08:27PM +0100, Niklas Söderlund wrote:
> > The driver now have access to frame descriptor information, use it. Only
> > enable the virtual channels which are described in the frame descriptor
> > and calculate the link based on all enabled streams.
> >
> > With multiplexed stream support it's now possible to have different
> > formats on the different source pads. Make source formats independent
> > off each other and disallowing a format on the multiplexed sink.
> >
> > Signed-off-by: Niklas Söderlund 
> > ---
> >  drivers/media/platform/rcar-vin/rcar-csi2.c | 112 
> > ++--
> >  1 file changed, 58 insertions(+), 54 deletions(-)
> >
> > diff --git a/drivers/media/platform/rcar-vin/rcar-csi2.c 
> > b/drivers/media/platform/rcar-vin/rcar-csi2.c
> > index 6b607b2e31e26063..2dd7d03d622d5510 100644
> > --- a/drivers/media/platform/rcar-vin/rcar-csi2.c
> > +++ b/drivers/media/platform/rcar-vin/rcar-csi2.c
> > @@ -296,24 +296,22 @@ static const struct phtw_testdin_data 
> > testdin_data_v3m_e3[] = {
> >  #define CSI0CLKFREQRANGE(n)((n & 0x3f) << 16)
> >
> >  struct rcar_csi2_format {
> > -   unsigned int code;
> > unsigned int datatype;
> > unsigned int bpp;
> >  };
> >
> >  static const struct rcar_csi2_format rcar_csi2_formats[] = {
> > -   { .code = MEDIA_BUS_FMT_RGB888_1X24,.datatype = 0x24, .bpp = 24 },
> > -   { .code = MEDIA_BUS_FMT_UYVY8_1X16, .datatype = 0x1e, .bpp = 16 },
> > -   { .code = MEDIA_BUS_FMT_UYVY8_2X8,  .datatype = 0x1e, .bpp = 16 },
> > -   { .code = MEDIA_BUS_FMT_YUYV10_2X10,.datatype = 0x1e, .bpp = 16 },
> > +   { .datatype = 0x1e, .bpp = 16 },
> > +   { .datatype = 0x24, .bpp = 24 },
> >  };
> >
> > -static const struct rcar_csi2_format *rcar_csi2_code_to_fmt(unsigned int 
> > code)
> > +static const struct rcar_csi2_format
> > +*rcar_csi2_datatype_to_fmt(unsigned int datatype)
> >  {
> > unsigned int i;
> >
> > for (i = 0; i < ARRAY_SIZE(rcar_csi2_formats); i++)
> > -   if (rcar_csi2_formats[i].code == code)
> > +   if (rcar_csi2_formats[i].datatype == datatype)
> > return rcar_csi2_formats + i;
> >
> > return NULL;
> > @@ -355,7 +353,7 @@ struct rcar_csi2 {
> > struct v4l2_async_notifier notifier;
> > struct v4l2_async_subdev remote;
> >
> > -   struct v4l2_mbus_framefmt mf;
> > +   struct v4l2_mbus_framefmt mf[4];
> >
> > struct mutex lock;
> > int stream_count[4];
> > @@ -411,25 +409,14 @@ static int rcar_csi2_wait_phy_start(struct rcar_csi2 
> > *priv)
> > return -ETIMEDOUT;
> >  }
> >
> > -static int rcar_csi2_calc_mbps(struct rcar_csi2 *priv, unsigned int bpp)
> > +static int rcar_csi2_calc_mbps(struct rcar_csi2 *priv,
> > +  struct v4l2_subdev *source,
> > +  struct v4l2_mbus_frame_desc *fd)
> >  {
> > -   struct media_pad *pad, *source_pad;
> > -   struct v4l2_subdev *source = NULL;
> > struct v4l2_ctrl *ctrl;
> > +   unsigned int i, bpp = 0;
> > u64 mbps;
> >
> > -   /* Get remote subdevice */
> > -   pad = >subdev.entity.pads[RCAR_CSI2_SINK];
> > -   source_pad = media_entity_remote_pad(pad);
> > -   if (!source_pad) {
> > -   dev_err(priv->dev, "Could not find remote source pad\n");
> > -   return -ENODEV;
> > -   }
> > -   source = media_entity_to_v4l2_subdev(source_pad->entity);
> > -
> > -   dev_dbg(priv->dev, "Using source %s pad: %u\n", source->name,
> > -   source_pad->index);
> > -
> > /* Read the pixel rate control from remote */
> > ctrl = v4l2_ctrl_find(source->ctrl_handler, V4L2_CID_PIXEL_RATE);
> > if (!ctrl) {
> > @@ -438,6 +425,21 @@ static int rcar_csi2_calc_mbps(struct rcar_csi2 *priv, 
> > unsigned int bpp)
> > return -EINVAL;
> > }
> >
> > +   /* Calculate total bpp */
> > +   for (i = 0; i < fd->num_entries; i++) {
> > +   const struct rcar_csi2_format *format;
> > +
> > +   format = rcar_csi2_datatype_to_fmt(
> > +   fd->entry[i].bus.csi2.data_type);
> > +   if (!format) {
> > +   dev_err(priv->dev, "Unknown data type: %d\n",
> > +   fd->entry[i].bus.csi2.data_type);
> > +   return -EINVAL;
> > +   }
> > +
> > +   bpp += format->bpp;
> > +   }
> > +
> > /* Calculate the phypll */
> > mbps = v4l2_ctrl_g_ctrl_int64(ctrl) * bpp;
> > do_div(mbps, priv->lanes * 100);
> > @@ -489,39 +491,33 @@ static int rcar_csi2_set_phtw(struct rcar_csi2 *priv, 
> > unsigned int mbps)
> > return 0;
> >  }
> >
> > -static int rcar_csi2_start(struct rcar_csi2 *priv)
> > +static int rcar_csi2_start(struct rcar_csi2 *priv, struct v4l2_subdev 
> > *source,
> > +  struct 

Re: [PATCH/RFC v2 07/15] rcar-csi2: use frame description information to configure CSI-2 bus

[jacopo mondi]

Hi Niklas,
   thanks for the patch!

On Thu, Dec 14, 2017 at 08:08:27PM +0100, Niklas Söderlund wrote:
> The driver now have access to frame descriptor information, use it. Only
> enable the virtual channels which are described in the frame descriptor
> and calculate the link based on all enabled streams.
>
> With multiplexed stream support it's now possible to have different
> formats on the different source pads. Make source formats independent
> off each other and disallowing a format on the multiplexed sink.
>
> Signed-off-by: Niklas Söderlund 
> ---
>  drivers/media/platform/rcar-vin/rcar-csi2.c | 112 
> ++--
>  1 file changed, 58 insertions(+), 54 deletions(-)
>
> diff --git a/drivers/media/platform/rcar-vin/rcar-csi2.c 
> b/drivers/media/platform/rcar-vin/rcar-csi2.c
> index 6b607b2e31e26063..2dd7d03d622d5510 100644
> --- a/drivers/media/platform/rcar-vin/rcar-csi2.c
> +++ b/drivers/media/platform/rcar-vin/rcar-csi2.c
> @@ -296,24 +296,22 @@ static const struct phtw_testdin_data 
> testdin_data_v3m_e3[] = {
>  #define CSI0CLKFREQRANGE(n)  ((n & 0x3f) << 16)
>
>  struct rcar_csi2_format {
> - unsigned int code;
>   unsigned int datatype;
>   unsigned int bpp;
>  };
>
>  static const struct rcar_csi2_format rcar_csi2_formats[] = {
> - { .code = MEDIA_BUS_FMT_RGB888_1X24,.datatype = 0x24, .bpp = 24 },
> - { .code = MEDIA_BUS_FMT_UYVY8_1X16, .datatype = 0x1e, .bpp = 16 },
> - { .code = MEDIA_BUS_FMT_UYVY8_2X8,  .datatype = 0x1e, .bpp = 16 },
> - { .code = MEDIA_BUS_FMT_YUYV10_2X10,.datatype = 0x1e, .bpp = 16 },
> + { .datatype = 0x1e, .bpp = 16 },
> + { .datatype = 0x24, .bpp = 24 },
>  };
>
> -static const struct rcar_csi2_format *rcar_csi2_code_to_fmt(unsigned int 
> code)
> +static const struct rcar_csi2_format
> +*rcar_csi2_datatype_to_fmt(unsigned int datatype)
>  {
>   unsigned int i;
>
>   for (i = 0; i < ARRAY_SIZE(rcar_csi2_formats); i++)
> - if (rcar_csi2_formats[i].code == code)
> + if (rcar_csi2_formats[i].datatype == datatype)
>   return rcar_csi2_formats + i;
>
>   return NULL;
> @@ -355,7 +353,7 @@ struct rcar_csi2 {
>   struct v4l2_async_notifier notifier;
>   struct v4l2_async_subdev remote;
>
> - struct v4l2_mbus_framefmt mf;
> + struct v4l2_mbus_framefmt mf[4];
>
>   struct mutex lock;
>   int stream_count[4];
> @@ -411,25 +409,14 @@ static int rcar_csi2_wait_phy_start(struct rcar_csi2 
> *priv)
>   return -ETIMEDOUT;
>  }
>
> -static int rcar_csi2_calc_mbps(struct rcar_csi2 *priv, unsigned int bpp)
> +static int rcar_csi2_calc_mbps(struct rcar_csi2 *priv,
> +struct v4l2_subdev *source,
> +struct v4l2_mbus_frame_desc *fd)
>  {
> - struct media_pad *pad, *source_pad;
> - struct v4l2_subdev *source = NULL;
>   struct v4l2_ctrl *ctrl;
> + unsigned int i, bpp = 0;
>   u64 mbps;
>
> - /* Get remote subdevice */
> - pad = >subdev.entity.pads[RCAR_CSI2_SINK];
> - source_pad = media_entity_remote_pad(pad);
> - if (!source_pad) {
> - dev_err(priv->dev, "Could not find remote source pad\n");
> - return -ENODEV;
> - }
> - source = media_entity_to_v4l2_subdev(source_pad->entity);
> -
> - dev_dbg(priv->dev, "Using source %s pad: %u\n", source->name,
> - source_pad->index);
> -
>   /* Read the pixel rate control from remote */
>   ctrl = v4l2_ctrl_find(source->ctrl_handler, V4L2_CID_PIXEL_RATE);
>   if (!ctrl) {
> @@ -438,6 +425,21 @@ static int rcar_csi2_calc_mbps(struct rcar_csi2 *priv, 
> unsigned int bpp)
>   return -EINVAL;
>   }
>
> + /* Calculate total bpp */
> + for (i = 0; i < fd->num_entries; i++) {
> + const struct rcar_csi2_format *format;
> +
> + format = rcar_csi2_datatype_to_fmt(
> + fd->entry[i].bus.csi2.data_type);
> + if (!format) {
> + dev_err(priv->dev, "Unknown data type: %d\n",
> + fd->entry[i].bus.csi2.data_type);
> + return -EINVAL;
> + }
> +
> + bpp += format->bpp;
> + }
> +
>   /* Calculate the phypll */
>   mbps = v4l2_ctrl_g_ctrl_int64(ctrl) * bpp;
>   do_div(mbps, priv->lanes * 100);
> @@ -489,39 +491,33 @@ static int rcar_csi2_set_phtw(struct rcar_csi2 *priv, 
> unsigned int mbps)
>   return 0;
>  }
>
> -static int rcar_csi2_start(struct rcar_csi2 *priv)
> +static int rcar_csi2_start(struct rcar_csi2 *priv, struct v4l2_subdev 
> *source,
> +struct v4l2_mbus_frame_desc *fd)

I'm not sure I got this right, but with the new s_stream pad
operation, and with rcar-csi2 endpoints connected to differenct VIN
instances, is it possible for "rcar_csi2_s_stream()" to be called on
the same 

[PATCH/RFC v2 07/15] rcar-csi2: use frame description information to configure CSI-2 bus

[Niklas Söderlund]

The driver now have access to frame descriptor information, use it. Only
enable the virtual channels which are described in the frame descriptor
and calculate the link based on all enabled streams.

With multiplexed stream support it's now possible to have different
formats on the different source pads. Make source formats independent
off each other and disallowing a format on the multiplexed sink.

Signed-off-by: Niklas Söderlund 
---
 drivers/media/platform/rcar-vin/rcar-csi2.c | 112 ++--
 1 file changed, 58 insertions(+), 54 deletions(-)

diff --git a/drivers/media/platform/rcar-vin/rcar-csi2.c 
b/drivers/media/platform/rcar-vin/rcar-csi2.c
index 6b607b2e31e26063..2dd7d03d622d5510 100644
--- a/drivers/media/platform/rcar-vin/rcar-csi2.c
+++ b/drivers/media/platform/rcar-vin/rcar-csi2.c
@@ -296,24 +296,22 @@ static const struct phtw_testdin_data 
testdin_data_v3m_e3[] = {
 #define CSI0CLKFREQRANGE(n)((n & 0x3f) << 16)
 
 struct rcar_csi2_format {
-   unsigned int code;
unsigned int datatype;
unsigned int bpp;
 };
 
 static const struct rcar_csi2_format rcar_csi2_formats[] = {
-   { .code = MEDIA_BUS_FMT_RGB888_1X24,.datatype = 0x24, .bpp = 24 },
-   { .code = MEDIA_BUS_FMT_UYVY8_1X16, .datatype = 0x1e, .bpp = 16 },
-   { .code = MEDIA_BUS_FMT_UYVY8_2X8,  .datatype = 0x1e, .bpp = 16 },
-   { .code = MEDIA_BUS_FMT_YUYV10_2X10,.datatype = 0x1e, .bpp = 16 },
+   { .datatype = 0x1e, .bpp = 16 },
+   { .datatype = 0x24, .bpp = 24 },
 };
 
-static const struct rcar_csi2_format *rcar_csi2_code_to_fmt(unsigned int code)
+static const struct rcar_csi2_format
+*rcar_csi2_datatype_to_fmt(unsigned int datatype)
 {
unsigned int i;
 
for (i = 0; i < ARRAY_SIZE(rcar_csi2_formats); i++)
-   if (rcar_csi2_formats[i].code == code)
+   if (rcar_csi2_formats[i].datatype == datatype)
return rcar_csi2_formats + i;
 
return NULL;
@@ -355,7 +353,7 @@ struct rcar_csi2 {
struct v4l2_async_notifier notifier;
struct v4l2_async_subdev remote;
 
-   struct v4l2_mbus_framefmt mf;
+   struct v4l2_mbus_framefmt mf[4];
 
struct mutex lock;
int stream_count[4];
@@ -411,25 +409,14 @@ static int rcar_csi2_wait_phy_start(struct rcar_csi2 
*priv)
return -ETIMEDOUT;
 }
 
-static int rcar_csi2_calc_mbps(struct rcar_csi2 *priv, unsigned int bpp)
+static int rcar_csi2_calc_mbps(struct rcar_csi2 *priv,
+  struct v4l2_subdev *source,
+  struct v4l2_mbus_frame_desc *fd)
 {
-   struct media_pad *pad, *source_pad;
-   struct v4l2_subdev *source = NULL;
struct v4l2_ctrl *ctrl;
+   unsigned int i, bpp = 0;
u64 mbps;
 
-   /* Get remote subdevice */
-   pad = >subdev.entity.pads[RCAR_CSI2_SINK];
-   source_pad = media_entity_remote_pad(pad);
-   if (!source_pad) {
-   dev_err(priv->dev, "Could not find remote source pad\n");
-   return -ENODEV;
-   }
-   source = media_entity_to_v4l2_subdev(source_pad->entity);
-
-   dev_dbg(priv->dev, "Using source %s pad: %u\n", source->name,
-   source_pad->index);
-
/* Read the pixel rate control from remote */
ctrl = v4l2_ctrl_find(source->ctrl_handler, V4L2_CID_PIXEL_RATE);
if (!ctrl) {
@@ -438,6 +425,21 @@ static int rcar_csi2_calc_mbps(struct rcar_csi2 *priv, 
unsigned int bpp)
return -EINVAL;
}
 
+   /* Calculate total bpp */
+   for (i = 0; i < fd->num_entries; i++) {
+   const struct rcar_csi2_format *format;
+
+   format = rcar_csi2_datatype_to_fmt(
+   fd->entry[i].bus.csi2.data_type);
+   if (!format) {
+   dev_err(priv->dev, "Unknown data type: %d\n",
+   fd->entry[i].bus.csi2.data_type);
+   return -EINVAL;
+   }
+
+   bpp += format->bpp;
+   }
+
/* Calculate the phypll */
mbps = v4l2_ctrl_g_ctrl_int64(ctrl) * bpp;
do_div(mbps, priv->lanes * 100);
@@ -489,39 +491,33 @@ static int rcar_csi2_set_phtw(struct rcar_csi2 *priv, 
unsigned int mbps)
return 0;
 }
 
-static int rcar_csi2_start(struct rcar_csi2 *priv)
+static int rcar_csi2_start(struct rcar_csi2 *priv, struct v4l2_subdev *source,
+  struct v4l2_mbus_frame_desc *fd)
 {
-   const struct rcar_csi2_format *format;
-   u32 phycnt, tmp;
-   u32 vcdt = 0, vcdt2 = 0;
+   u32 phycnt, vcdt = 0, vcdt2 = 0;
unsigned int i;
int mbps, ret;
 
-   dev_dbg(priv->dev, "Input size (%ux%u%c)\n",
-   priv->mf.width, priv->mf.height,
-   priv->mf.field == V4L2_FIELD_NONE ? 'p' : 'i');
-
-   /* Code is validated in set_ftm */
-