[PATCH v2 0/3] media: rcar-vin: Enable MEDIA_BUS_FMT_SRGGB8_1X8 format and support for matching fwnode against endpoints/nodes

[PATCH v2 0/3] media: rcar-vin: Enable MEDIA_BUS_FMT_SRGGB8_1X8 format and support for matching fwnode against endpoints/nodes

[Lad Prabhakar]

First patch of the series adds support for matching fwnode against
endpoints/nodes which are registered in v4l2-async and the later adds
supports for MEDIA_BUS_FMT_SRGGB8_1X8 format.

Changes for v2:
1: Added support for matching fwnode against endpoints/nodes.
2: Seperated patch for rcar-vin and rcar-csi2.c which added
   MEDIA_BUS_FMT_SRGGB8_1X8.

Lad Prabhakar (3):
  media: rcar-csi2: Add support to match fwnode against remote or parent
    port
  media: rcar-vin: Add support for MEDIA_BUS_FMT_SRGGB8_1X8 format
  media: rcar-vin: rcar-csi2: Add support for MEDIA_BUS_FMT_SRGGB8_1X8
    format

 drivers/media/platform/rcar-vin/rcar-core.c |  1 +
 drivers/media/platform/rcar-vin/rcar-csi2.c | 42 ++++++++++++++++++++++++++---
 drivers/media/platform/rcar-vin/rcar-dma.c  |  9 ++++++-
 drivers/media/platform/rcar-vin/rcar-v4l2.c | 13 ++++++++-
 4 files changed, 60 insertions(+), 5 deletions(-)

-- 
2.7.4

[PATCH v2 1/3] media: rcar-csi2: Add support to match fwnode against remote or parent port

[Lad Prabhakar]

The rcar-csi2 driver uses the v4l2-async framework to do endpoint matching
instead of node matching. This is needed as it needs to work with the
adv748x driver which register it self in v4l2-async using endpoints
instead of nodes. The reason for this is that from a single DT node it
creates multiple subdevices, one for each endpoint.

But when using subdevs which register itself in v4l2-async using nodes,
the rcar-csi2 driver failed to find the matching endpoint because the
match.fwnode was pointing to remote endpoint instead of remote parent
port.

This commit adds support in rcar-csi2 driver to handle both the cases
where subdev registers in v4l2-async using endpoints/nodes, by using
match_type as V4L2_ASYNC_MATCH_CUSTOM and implementing the match()
callback to compare the fwnode of either remote/parent.

Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
---
 drivers/media/platform/rcar-vin/rcar-csi2.c | 41 ++++++++++++++++++++++++++---
 1 file changed, 38 insertions(+), 3 deletions(-)

diff --git a/drivers/media/platform/rcar-vin/rcar-csi2.c b/drivers/media/platform/rcar-vin/rcar-csi2.c
index faa9fb2..39e1639 100644
--- a/drivers/media/platform/rcar-vin/rcar-csi2.c
+++ b/drivers/media/platform/rcar-vin/rcar-csi2.c
@@ -808,6 +808,41 @@ static int rcsi2_parse_v4l2(struct rcar_csi2 *priv,
 	return 0;
 }
 
+static bool rcsi2_asd_match(struct device *dev,
+			    struct v4l2_async_subdev *async_sd)
+{
+	struct rcar_csi2 *priv = (struct rcar_csi2 *)
+				  async_sd->match.custom.priv;
+	struct fwnode_handle *endpoint;
+	struct fwnode_handle *remote;
+	struct fwnode_handle *parent;
+	struct device_node *np;
+	bool matched = false;
+
+	np = of_graph_get_endpoint_by_regs(priv->dev->of_node, 0, 0);
+	if (!np) {
+		dev_err(priv->dev, "Not connected to subdevice\n");
+		return -EINVAL;
+	}
+
+	endpoint = of_fwnode_handle(np);
+	remote = fwnode_graph_get_remote_endpoint(endpoint);
+	parent = fwnode_graph_get_remote_port_parent(endpoint);
+	if (parent) {
+		if (parent == dev->fwnode ||
+		    parent == &dev->of_node->fwnode)
+			matched = true;
+	} else if (remote && !matched) {
+		if (remote == dev->fwnode ||
+		    remote == &dev->of_node->fwnode)
+			matched = true;
+	}
+
+	of_node_put(np);
+
+	return matched;
+}
+
 static int rcsi2_parse_dt(struct rcar_csi2 *priv)
 {
 	struct device_node *ep;
@@ -833,9 +868,9 @@ static int rcsi2_parse_dt(struct rcar_csi2 *priv)
 		return ret;
 	}
 
-	priv->asd.match.fwnode =
-		fwnode_graph_get_remote_endpoint(of_fwnode_handle(ep));
-	priv->asd.match_type = V4L2_ASYNC_MATCH_FWNODE;
+	priv->asd.match.custom.match = &rcsi2_asd_match;
+	priv->asd.match.custom.priv = priv;
+	priv->asd.match_type = V4L2_ASYNC_MATCH_CUSTOM;
 
 	of_node_put(ep);
 
-- 
2.7.4

Re: [PATCH v2 1/3] media: rcar-csi2: Add support to match fwnode against remote or parent port

[Niklas]

Hi Lad,

Thanks for your work.

On 2020-03-10 11:06:02 +0000, Lad Prabhakar wrote:
> The rcar-csi2 driver uses the v4l2-async framework to do endpoint matching
> instead of node matching. This is needed as it needs to work with the
> adv748x driver which register it self in v4l2-async using endpoints
> instead of nodes. The reason for this is that from a single DT node it
> creates multiple subdevices, one for each endpoint.
> 
> But when using subdevs which register itself in v4l2-async using nodes,
> the rcar-csi2 driver failed to find the matching endpoint because the
> match.fwnode was pointing to remote endpoint instead of remote parent
> port.
> 
> This commit adds support in rcar-csi2 driver to handle both the cases
> where subdev registers in v4l2-async using endpoints/nodes, by using
> match_type as V4L2_ASYNC_MATCH_CUSTOM and implementing the match()
> callback to compare the fwnode of either remote/parent.

This is a novel approach to the solution, and I won't object to it out 
right. But I think the proper solution is to move this logic into 
v4l2-async instead of adding a custom match handler in rcar-csi2.

Think of the reveres use-case, a different CSI-2 receiver who wish to 
use the ADV748x would still have this node vs. endpoint issue.

> 
> Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> ---
>  drivers/media/platform/rcar-vin/rcar-csi2.c | 41 ++++++++++++++++++++++++++---
>  1 file changed, 38 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/media/platform/rcar-vin/rcar-csi2.c b/drivers/media/platform/rcar-vin/rcar-csi2.c
> index faa9fb2..39e1639 100644
> --- a/drivers/media/platform/rcar-vin/rcar-csi2.c
> +++ b/drivers/media/platform/rcar-vin/rcar-csi2.c
> @@ -808,6 +808,41 @@ static int rcsi2_parse_v4l2(struct rcar_csi2 *priv,
>  	return 0;
>  }
>  
> +static bool rcsi2_asd_match(struct device *dev,
> +			    struct v4l2_async_subdev *async_sd)
> +{
> +	struct rcar_csi2 *priv = (struct rcar_csi2 *)
> +				  async_sd->match.custom.priv;
> +	struct fwnode_handle *endpoint;
> +	struct fwnode_handle *remote;
> +	struct fwnode_handle *parent;
> +	struct device_node *np;
> +	bool matched = false;
> +
> +	np = of_graph_get_endpoint_by_regs(priv->dev->of_node, 0, 0);
> +	if (!np) {
> +		dev_err(priv->dev, "Not connected to subdevice\n");
> +		return -EINVAL;

You can't return -EINVAL here as it will be interpreted as a match by 
the caller ;-).  You should not even register a device with v4l2-async 
if it's not connected to an endpoint.

> +	}
> +
> +	endpoint = of_fwnode_handle(np);
> +	remote = fwnode_graph_get_remote_endpoint(endpoint);
> +	parent = fwnode_graph_get_remote_port_parent(endpoint);
> +	if (parent) {

This is wrong, we will always have a parent and will always take this 
code path. Hence reducing this to the equivalent of node only matching.  
I applied this patch and tried on M3-N with a ADv748x and the wrong 
endpoints of the ADV7482 is routed to the two CSI-2 receivers, breaking 
it.

I added some debug printouts to explain whats going on:

    * First call
        dev: rcar-csi2 fea80000.csi2
        endpoint: /soc/csi2@feaa0000/ports/port@0/endpoint
        remote: /soc/i2c@e66d8000/video-receiver@70/port@a/endpoint
        parent: /soc/i2c@e66d8000/video-receiver@70
        dev->fwnode: /soc/csi2@fea80000
        dev->of_node: /soc/csi2@fea80000
        match: false

    * Second call
        dev: adv748x 4-0070
        endpoint: /soc/csi2@feaa0000/ports/port@0/endpoint
        remote: /soc/i2c@e66d8000/video-receiver@70/port@a/endpoint
        parent: /soc/i2c@e66d8000/video-receiver@70
        dev->fwnode: /soc/i2c@e66d8000/video-receiver@70
        dev->of_node: /soc/i2c@e66d8000/video-receiver@70
        match: true

    * Third call
        dev: adv748x 4-0070
        endpoint: /soc/csi2@fea80000/ports/port@0/endpoint
        remote: /soc/i2c@e66d8000/video-receiver@70/port@b/endpoint
        parent: /soc/i2c@e66d8000/video-receiver@70
        dev->fwnode: /soc/i2c@e66d8000/video-receiver@70
        dev->of_node: /soc/i2c@e66d8000/video-receiver@70
        match: true

Now we have a media graph that is completely probed and video devices 
register in the system but you are not able to stream video as the wrong 
CSI-2 transmitter is described in the graph to be connected to the wrong 
receiver.

This only strengthens my view that this should not be fixed with a 
custom matcher in rcar-csi2 but directly in v4l-async. Please see if you 
can't address the issue in the framework to allow node and endpoint 
matching to co-exists.

> +		if (parent == dev->fwnode ||
> +		    parent == &dev->of_node->fwnode)
> +			matched = true;
> +	} else if (remote && !matched) {

No need to check !matched here ;-)

> +		if (remote == dev->fwnode ||
> +		    remote == &dev->of_node->fwnode)
> +			matched = true;
> +	}



> +
> +	of_node_put(np);
> +
> +	return matched;
> +}
> +
>  static int rcsi2_parse_dt(struct rcar_csi2 *priv)
>  {
>  	struct device_node *ep;
> @@ -833,9 +868,9 @@ static int rcsi2_parse_dt(struct rcar_csi2 *priv)
>  		return ret;
>  	}
>  
> -	priv->asd.match.fwnode =
> -		fwnode_graph_get_remote_endpoint(of_fwnode_handle(ep));
> -	priv->asd.match_type = V4L2_ASYNC_MATCH_FWNODE;
> +	priv->asd.match.custom.match = &rcsi2_asd_match;
> +	priv->asd.match.custom.priv = priv;
> +	priv->asd.match_type = V4L2_ASYNC_MATCH_CUSTOM;
>  
>  	of_node_put(ep);
>  
> -- 
> 2.7.4
> 

-- 
Regards,
Niklas Söderlund

RE: [PATCH v2 1/3] media: rcar-csi2: Add support to match fwnode against remote or parent port

[Prabhakar Mahadev Lad]

Hi Niklas,

Thank you for the review.

> -----Original Message-----
> From: Niklas <niklas.soderlund@ragnatech.se>
> Sent: 10 March 2020 12:44
> To: Prabhakar Mahadev Lad <prabhakar.mahadev-lad.rj@bp.renesas.com>
> Cc: Mauro Carvalho Chehab <mchehab@kernel.org>; linux-
> media@vger.kernel.org; linux-renesas-soc@vger.kernel.org; linux-
> kernel@vger.kernel.org; Lad Prabhakar <prabhakar.csengg@gmail.com>
> Subject: Re: [PATCH v2 1/3] media: rcar-csi2: Add support to match fwnode
> against remote or parent port
>
> Hi Lad,
>
> Thanks for your work.
>
> On 2020-03-10 11:06:02 +0000, Lad Prabhakar wrote:
> > The rcar-csi2 driver uses the v4l2-async framework to do endpoint
> > matching instead of node matching. This is needed as it needs to work
> > with the adv748x driver which register it self in v4l2-async using
> > endpoints instead of nodes. The reason for this is that from a single
> > DT node it creates multiple subdevices, one for each endpoint.
> >
> > But when using subdevs which register itself in v4l2-async using
> > nodes, the rcar-csi2 driver failed to find the matching endpoint
> > because the match.fwnode was pointing to remote endpoint instead of
> > remote parent port.
> >
> > This commit adds support in rcar-csi2 driver to handle both the cases
> > where subdev registers in v4l2-async using endpoints/nodes, by using
> > match_type as V4L2_ASYNC_MATCH_CUSTOM and implementing the
> match()
> > callback to compare the fwnode of either remote/parent.
>
> This is a novel approach to the solution, and I won't object to it out right. But I
> think the proper solution is to move this logic into v4l2-async instead of
> adding a custom match handler in rcar-csi2.
>
> Think of the reveres use-case, a different CSI-2 receiver who wish to use the
> ADV748x would still have this node vs. endpoint issue.
>
Agreed.

> >
> > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-
> lad.rj@bp.renesas.com>
> > ---
> >  drivers/media/platform/rcar-vin/rcar-csi2.c | 41
> > ++++++++++++++++++++++++++---
> >  1 file changed, 38 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/media/platform/rcar-vin/rcar-csi2.c
> > b/drivers/media/platform/rcar-vin/rcar-csi2.c
> > index faa9fb2..39e1639 100644
> > --- a/drivers/media/platform/rcar-vin/rcar-csi2.c
> > +++ b/drivers/media/platform/rcar-vin/rcar-csi2.c
> > @@ -808,6 +808,41 @@ static int rcsi2_parse_v4l2(struct rcar_csi2 *priv,
> >  return 0;
> >  }
> >
> > +static bool rcsi2_asd_match(struct device *dev,
> > +    struct v4l2_async_subdev *async_sd) {
> > +struct rcar_csi2 *priv = (struct rcar_csi2 *)
> > +  async_sd->match.custom.priv;
> > +struct fwnode_handle *endpoint;
> > +struct fwnode_handle *remote;
> > +struct fwnode_handle *parent;
> > +struct device_node *np;
> > +bool matched = false;
> > +
> > +np = of_graph_get_endpoint_by_regs(priv->dev->of_node, 0, 0);
> > +if (!np) {
> > +dev_err(priv->dev, "Not connected to subdevice\n");
> > +return -EINVAL;
>
> You can't return -EINVAL here as it will be interpreted as a match by the caller
> ;-).  You should not even register a device with v4l2-async if it's not
> connected to an endpoint.
>
My bad.

> > +}
> > +
> > +endpoint = of_fwnode_handle(np);
> > +remote = fwnode_graph_get_remote_endpoint(endpoint);
> > +parent = fwnode_graph_get_remote_port_parent(endpoint);
> > +if (parent) {
>
> This is wrong, we will always have a parent and will always take this code
> path. Hence reducing this to the equivalent of node only matching.
> I applied this patch and tried on M3-N with a ADv748x and the wrong
> endpoints of the ADV7482 is routed to the two CSI-2 receivers, breaking it.
>
I did try the media-tree on M3N but it has many issues for booting, could you please
point me to a tree which I can use for testing.

> I added some debug printouts to explain whats going on:
>
>     * First call
>         dev: rcar-csi2 fea80000.csi2
>         endpoint: /soc/csi2@feaa0000/ports/port@0/endpoint
>         remote: /soc/i2c@e66d8000/video-receiver@70/port@a/endpoint
>         parent: /soc/i2c@e66d8000/video-receiver@70
>         dev->fwnode: /soc/csi2@fea80000
>         dev->of_node: /soc/csi2@fea80000
>         match: false
>
>     * Second call
>         dev: adv748x 4-0070
>         endpoint: /soc/csi2@feaa0000/ports/port@0/endpoint
>         remote: /soc/i2c@e66d8000/video-receiver@70/port@a/endpoint
>         parent: /soc/i2c@e66d8000/video-receiver@70
>         dev->fwnode: /soc/i2c@e66d8000/video-receiver@70
>         dev->of_node: /soc/i2c@e66d8000/video-receiver@70
>         match: true
>
>     * Third call
>         dev: adv748x 4-0070
>         endpoint: /soc/csi2@fea80000/ports/port@0/endpoint
>         remote: /soc/i2c@e66d8000/video-receiver@70/port@b/endpoint
>         parent: /soc/i2c@e66d8000/video-receiver@70
>         dev->fwnode: /soc/i2c@e66d8000/video-receiver@70
>         dev->of_node: /soc/i2c@e66d8000/video-receiver@70
>         match: true
>
Thank you for the above.

> Now we have a media graph that is completely probed and video devices
> register in the system but you are not able to stream video as the wrong
> CSI-2 transmitter is described in the graph to be connected to the wrong
> receiver.
>
> This only strengthens my view that this should not be fixed with a custom
> matcher in rcar-csi2 but directly in v4l-async. Please see if you can't address
> the issue in the framework to allow node and endpoint matching to co-
> exists.
>
Ill do some more digging 😊

> > +if (parent == dev->fwnode ||
> > +    parent == &dev->of_node->fwnode)
> > +matched = true;
> > +} else if (remote && !matched) {
>
> No need to check !matched here ;-)
>
Agreed.

Cheers,
--Prabhakar

> > +if (remote == dev->fwnode ||
> > +    remote == &dev->of_node->fwnode)
> > +matched = true;
> > +}
>
>
>
> > +
> > +of_node_put(np);
> > +
> > +return matched;
> > +}
> > +
> >  static int rcsi2_parse_dt(struct rcar_csi2 *priv)  {
> >  struct device_node *ep;
> > @@ -833,9 +868,9 @@ static int rcsi2_parse_dt(struct rcar_csi2 *priv)
> >  return ret;
> >  }
> >
> > -priv->asd.match.fwnode =
> > -
> fwnode_graph_get_remote_endpoint(of_fwnode_handle(ep));
> > -priv->asd.match_type = V4L2_ASYNC_MATCH_FWNODE;
> > +priv->asd.match.custom.match = &rcsi2_asd_match;
> > +priv->asd.match.custom.priv = priv;
> > +priv->asd.match_type = V4L2_ASYNC_MATCH_CUSTOM;
> >
> >  of_node_put(ep);
> >
> > --
> > 2.7.4
> >
>
> --
> Regards,
> Niklas Söderlund


Renesas Electronics Europe GmbH, Geschaeftsfuehrer/President: Carsten Jauch, Sitz der Gesellschaft/Registered office: Duesseldorf, Arcadiastrasse 10, 40472 Duesseldorf, Germany, Handelsregister/Commercial Register: Duesseldorf, HRB 3708 USt-IDNr./Tax identification no.: DE 119353406 WEEE-Reg.-Nr./WEEE reg. no.: DE 14978647

[PATCH v2 2/3] media: rcar-vin: Add support for MEDIA_BUS_FMT_SRGGB8_1X8 format

[Lad Prabhakar]

Add support for MEDIA_BUS_FMT_SRGGB8_1X8 format in rcar-vin by setting
format type to RAW8 in VNMC register and appropriately setting the
bpp, bytesperline to enable V4L2_PIX_FMT_SRGGB8.

Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
---
 drivers/media/platform/rcar-vin/rcar-core.c |  1 +
 drivers/media/platform/rcar-vin/rcar-dma.c  |  9 ++++++++-
 drivers/media/platform/rcar-vin/rcar-v4l2.c | 13 ++++++++++++-
 3 files changed, 21 insertions(+), 2 deletions(-)

diff --git a/drivers/media/platform/rcar-vin/rcar-core.c b/drivers/media/platform/rcar-vin/rcar-core.c
index 7440c89..76daf2f 100644
--- a/drivers/media/platform/rcar-vin/rcar-core.c
+++ b/drivers/media/platform/rcar-vin/rcar-core.c
@@ -469,6 +469,7 @@ static int rvin_parallel_subdevice_attach(struct rvin_dev *vin,
 		case MEDIA_BUS_FMT_UYVY8_2X8:
 		case MEDIA_BUS_FMT_UYVY10_2X10:
 		case MEDIA_BUS_FMT_RGB888_1X24:
+		case MEDIA_BUS_FMT_SRGGB8_1X8:
 			vin->mbus_code = code.code;
 			vin_dbg(vin, "Found media bus format for %s: %d\n",
 				subdev->name, vin->mbus_code);
diff --git a/drivers/media/platform/rcar-vin/rcar-dma.c b/drivers/media/platform/rcar-vin/rcar-dma.c
index 1a30cd0..1c1fafa 100644
--- a/drivers/media/platform/rcar-vin/rcar-dma.c
+++ b/drivers/media/platform/rcar-vin/rcar-dma.c
@@ -85,6 +85,7 @@
 #define VNMC_INF_YUV8_BT601	(1 << 16)
 #define VNMC_INF_YUV10_BT656	(2 << 16)
 #define VNMC_INF_YUV10_BT601	(3 << 16)
+#define VNMC_INF_RAW8		(4 << 16)
 #define VNMC_INF_YUV16		(5 << 16)
 #define VNMC_INF_RGB888		(6 << 16)
 #define VNMC_VUP		(1 << 10)
@@ -587,7 +588,6 @@ void rvin_crop_scale_comp(struct rvin_dev *vin)
 	rvin_write(vin, vin->crop.top, VNSLPRC_REG);
 	rvin_write(vin, vin->crop.top + vin->crop.height - 1, VNELPRC_REG);
 
-
 	/* TODO: Add support for the UDS scaler. */
 	if (vin->info->model != RCAR_GEN3)
 		rvin_crop_scale_comp_gen2(vin);
@@ -676,6 +676,9 @@ static int rvin_setup(struct rvin_dev *vin)
 
 		input_is_yuv = true;
 		break;
+	case MEDIA_BUS_FMT_SRGGB8_1X8:
+		vnmc |= VNMC_INF_RAW8;
+		break;
 	default:
 		break;
 	}
@@ -737,6 +740,9 @@ static int rvin_setup(struct rvin_dev *vin)
 	case V4L2_PIX_FMT_ABGR32:
 		dmr = VNDMR_A8BIT(vin->alpha) | VNDMR_EXRGB | VNDMR_DTMD_ARGB;
 		break;
+	case V4L2_PIX_FMT_SRGGB8:
+		dmr = 0;
+		break;
 	default:
 		vin_err(vin, "Invalid pixelformat (0x%x)\n",
 			vin->format.pixelformat);
@@ -1110,6 +1116,7 @@ static int rvin_mc_validate_format(struct rvin_dev *vin, struct v4l2_subdev *sd,
 	case MEDIA_BUS_FMT_UYVY8_2X8:
 	case MEDIA_BUS_FMT_UYVY10_2X10:
 	case MEDIA_BUS_FMT_RGB888_1X24:
+	case MEDIA_BUS_FMT_SRGGB8_1X8:
 		vin->mbus_code = fmt.format.code;
 		break;
 	default:
diff --git a/drivers/media/platform/rcar-vin/rcar-v4l2.c b/drivers/media/platform/rcar-vin/rcar-v4l2.c
index 5151a3c..4698099 100644
--- a/drivers/media/platform/rcar-vin/rcar-v4l2.c
+++ b/drivers/media/platform/rcar-vin/rcar-v4l2.c
@@ -66,6 +66,10 @@ static const struct rvin_video_format rvin_formats[] = {
 		.fourcc			= V4L2_PIX_FMT_ABGR32,
 		.bpp			= 4,
 	},
+	{
+		.fourcc			= V4L2_PIX_FMT_SRGGB8,
+		.bpp			= 2,
+	},
 };
 
 const struct rvin_video_format *rvin_format_from_pixel(struct rvin_dev *vin,
@@ -102,6 +106,7 @@ static u32 rvin_format_bytesperline(struct rvin_dev *vin,
 {
 	const struct rvin_video_format *fmt;
 	u32 align;
+	u8 div;
 
 	fmt = rvin_format_from_pixel(vin, pix->pixelformat);
 
@@ -112,16 +117,22 @@ static u32 rvin_format_bytesperline(struct rvin_dev *vin,
 	case V4L2_PIX_FMT_NV12:
 	case V4L2_PIX_FMT_NV16:
 		align = 0x20;
+		div = 1;
+		break;
+	case V4L2_PIX_FMT_SRGGB8:
+		align = 0x10;
+		div = 2;
 		break;
 	default:
 		align = 0x10;
+		div = 1;
 		break;
 	}
 
 	if (V4L2_FIELD_IS_SEQUENTIAL(pix->field))
 		align = 0x80;
 
-	return ALIGN(pix->width, align) * fmt->bpp;
+	return ALIGN(pix->width / div, align) * fmt->bpp;
 }
 
 static u32 rvin_format_sizeimage(struct v4l2_pix_format *pix)
-- 
2.7.4

Re: [PATCH v2 2/3] media: rcar-vin: Add support for MEDIA_BUS_FMT_SRGGB8_1X8 format

[Niklas]

Hi Lad,

Thanks for your work.

On 2020-03-10 11:06:03 +0000, Lad Prabhakar wrote:
> Add support for MEDIA_BUS_FMT_SRGGB8_1X8 format in rcar-vin by setting
> format type to RAW8 in VNMC register and appropriately setting the
> bpp, bytesperline to enable V4L2_PIX_FMT_SRGGB8.
> 
> Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> ---
>  drivers/media/platform/rcar-vin/rcar-core.c |  1 +
>  drivers/media/platform/rcar-vin/rcar-dma.c  |  9 ++++++++-
>  drivers/media/platform/rcar-vin/rcar-v4l2.c | 13 ++++++++++++-
>  3 files changed, 21 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/media/platform/rcar-vin/rcar-core.c b/drivers/media/platform/rcar-vin/rcar-core.c
> index 7440c89..76daf2f 100644
> --- a/drivers/media/platform/rcar-vin/rcar-core.c
> +++ b/drivers/media/platform/rcar-vin/rcar-core.c
> @@ -469,6 +469,7 @@ static int rvin_parallel_subdevice_attach(struct rvin_dev *vin,
>  		case MEDIA_BUS_FMT_UYVY8_2X8:
>  		case MEDIA_BUS_FMT_UYVY10_2X10:
>  		case MEDIA_BUS_FMT_RGB888_1X24:
> +		case MEDIA_BUS_FMT_SRGGB8_1X8:
>  			vin->mbus_code = code.code;
>  			vin_dbg(vin, "Found media bus format for %s: %d\n",
>  				subdev->name, vin->mbus_code);
> diff --git a/drivers/media/platform/rcar-vin/rcar-dma.c b/drivers/media/platform/rcar-vin/rcar-dma.c
> index 1a30cd0..1c1fafa 100644
> --- a/drivers/media/platform/rcar-vin/rcar-dma.c
> +++ b/drivers/media/platform/rcar-vin/rcar-dma.c
> @@ -85,6 +85,7 @@
>  #define VNMC_INF_YUV8_BT601	(1 << 16)
>  #define VNMC_INF_YUV10_BT656	(2 << 16)
>  #define VNMC_INF_YUV10_BT601	(3 << 16)
> +#define VNMC_INF_RAW8		(4 << 16)
>  #define VNMC_INF_YUV16		(5 << 16)
>  #define VNMC_INF_RGB888		(6 << 16)
>  #define VNMC_VUP		(1 << 10)
> @@ -587,7 +588,6 @@ void rvin_crop_scale_comp(struct rvin_dev *vin)
>  	rvin_write(vin, vin->crop.top, VNSLPRC_REG);
>  	rvin_write(vin, vin->crop.top + vin->crop.height - 1, VNELPRC_REG);
>  
> -
>  	/* TODO: Add support for the UDS scaler. */
>  	if (vin->info->model != RCAR_GEN3)
>  		rvin_crop_scale_comp_gen2(vin);
> @@ -676,6 +676,9 @@ static int rvin_setup(struct rvin_dev *vin)
>  
>  		input_is_yuv = true;
>  		break;
> +	case MEDIA_BUS_FMT_SRGGB8_1X8:
> +		vnmc |= VNMC_INF_RAW8;
> +		break;
>  	default:
>  		break;
>  	}
> @@ -737,6 +740,9 @@ static int rvin_setup(struct rvin_dev *vin)
>  	case V4L2_PIX_FMT_ABGR32:
>  		dmr = VNDMR_A8BIT(vin->alpha) | VNDMR_EXRGB | VNDMR_DTMD_ARGB;
>  		break;
> +	case V4L2_PIX_FMT_SRGGB8:
> +		dmr = 0;
> +		break;
>  	default:
>  		vin_err(vin, "Invalid pixelformat (0x%x)\n",
>  			vin->format.pixelformat);
> @@ -1110,6 +1116,7 @@ static int rvin_mc_validate_format(struct rvin_dev *vin, struct v4l2_subdev *sd,
>  	case MEDIA_BUS_FMT_UYVY8_2X8:
>  	case MEDIA_BUS_FMT_UYVY10_2X10:
>  	case MEDIA_BUS_FMT_RGB888_1X24:
> +	case MEDIA_BUS_FMT_SRGGB8_1X8:
>  		vin->mbus_code = fmt.format.code;
>  		break;
>  	default:
> diff --git a/drivers/media/platform/rcar-vin/rcar-v4l2.c b/drivers/media/platform/rcar-vin/rcar-v4l2.c
> index 5151a3c..4698099 100644
> --- a/drivers/media/platform/rcar-vin/rcar-v4l2.c
> +++ b/drivers/media/platform/rcar-vin/rcar-v4l2.c
> @@ -66,6 +66,10 @@ static const struct rvin_video_format rvin_formats[] = {
>  		.fourcc			= V4L2_PIX_FMT_ABGR32,
>  		.bpp			= 4,
>  	},
> +	{
> +		.fourcc			= V4L2_PIX_FMT_SRGGB8,
> +		.bpp			= 2,

This does not look right, is not bytes-per-pixel 1 for a SRGGB8?

> +	},
>  };
>  
>  const struct rvin_video_format *rvin_format_from_pixel(struct rvin_dev *vin,
> @@ -102,6 +106,7 @@ static u32 rvin_format_bytesperline(struct rvin_dev *vin,
>  {
>  	const struct rvin_video_format *fmt;
>  	u32 align;
> +	u8 div;
>  
>  	fmt = rvin_format_from_pixel(vin, pix->pixelformat);
>  
> @@ -112,16 +117,22 @@ static u32 rvin_format_bytesperline(struct rvin_dev *vin,
>  	case V4L2_PIX_FMT_NV12:
>  	case V4L2_PIX_FMT_NV16:
>  		align = 0x20;
> +		div = 1;
> +		break;
> +	case V4L2_PIX_FMT_SRGGB8:
> +		align = 0x10;
> +		div = 2;

Yes this does not look right at all, I think you should set bpp to 1 and 
drop the div handling here.

>  		break;
>  	default:
>  		align = 0x10;
> +		div = 1;
>  		break;
>  	}
>  
>  	if (V4L2_FIELD_IS_SEQUENTIAL(pix->field))
>  		align = 0x80;
>  
> -	return ALIGN(pix->width, align) * fmt->bpp;
> +	return ALIGN(pix->width / div, align) * fmt->bpp;
>  }
>  
>  static u32 rvin_format_sizeimage(struct v4l2_pix_format *pix)
> -- 
> 2.7.4
> 

-- 
Regards,
Niklas Söderlund

RE: [PATCH v2 2/3] media: rcar-vin: Add support for MEDIA_BUS_FMT_SRGGB8_1X8 format

[Prabhakar Mahadev Lad]

Hi Niklas,

Thank for the review.

> -----Original Message-----
> From: Niklas <niklas.soderlund@ragnatech.se>
> Sent: 10 March 2020 12:46
> To: Prabhakar Mahadev Lad <prabhakar.mahadev-lad.rj@bp.renesas.com>
> Cc: Mauro Carvalho Chehab <mchehab@kernel.org>; linux-
> media@vger.kernel.org; linux-renesas-soc@vger.kernel.org; linux-
> kernel@vger.kernel.org; Lad Prabhakar <prabhakar.csengg@gmail.com>
> Subject: Re: [PATCH v2 2/3] media: rcar-vin: Add support for
> MEDIA_BUS_FMT_SRGGB8_1X8 format
>
> Hi Lad,
>
> Thanks for your work.
>
> On 2020-03-10 11:06:03 +0000, Lad Prabhakar wrote:
> > Add support for MEDIA_BUS_FMT_SRGGB8_1X8 format in rcar-vin by
> setting
> > format type to RAW8 in VNMC register and appropriately setting the
> > bpp, bytesperline to enable V4L2_PIX_FMT_SRGGB8.
> >
> > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-
> lad.rj@bp.renesas.com>
> > ---
> >  drivers/media/platform/rcar-vin/rcar-core.c |  1 +
> > drivers/media/platform/rcar-vin/rcar-dma.c  |  9 ++++++++-
> > drivers/media/platform/rcar-vin/rcar-v4l2.c | 13 ++++++++++++-
> >  3 files changed, 21 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/media/platform/rcar-vin/rcar-core.c
> > b/drivers/media/platform/rcar-vin/rcar-core.c
> > index 7440c89..76daf2f 100644
> > --- a/drivers/media/platform/rcar-vin/rcar-core.c
> > +++ b/drivers/media/platform/rcar-vin/rcar-core.c
> > @@ -469,6 +469,7 @@ static int rvin_parallel_subdevice_attach(struct
> rvin_dev *vin,
> >  case MEDIA_BUS_FMT_UYVY8_2X8:
> >  case MEDIA_BUS_FMT_UYVY10_2X10:
> >  case MEDIA_BUS_FMT_RGB888_1X24:
> > +case MEDIA_BUS_FMT_SRGGB8_1X8:
> >  vin->mbus_code = code.code;
> >  vin_dbg(vin, "Found media bus format for %s: %d\n",
> >  subdev->name, vin->mbus_code);
> > diff --git a/drivers/media/platform/rcar-vin/rcar-dma.c
> > b/drivers/media/platform/rcar-vin/rcar-dma.c
> > index 1a30cd0..1c1fafa 100644
> > --- a/drivers/media/platform/rcar-vin/rcar-dma.c
> > +++ b/drivers/media/platform/rcar-vin/rcar-dma.c
> > @@ -85,6 +85,7 @@
> >  #define VNMC_INF_YUV8_BT601(1 << 16)
> >  #define VNMC_INF_YUV10_BT656(2 << 16)
> >  #define VNMC_INF_YUV10_BT601(3 << 16)
> > +#define VNMC_INF_RAW8(4 << 16)
> >  #define VNMC_INF_YUV16(5 << 16)
> >  #define VNMC_INF_RGB888(6 << 16)
> >  #define VNMC_VUP(1 << 10)
> > @@ -587,7 +588,6 @@ void rvin_crop_scale_comp(struct rvin_dev *vin)
> >  rvin_write(vin, vin->crop.top, VNSLPRC_REG);
> >  rvin_write(vin, vin->crop.top + vin->crop.height - 1, VNELPRC_REG);
> >
> > -
> >  /* TODO: Add support for the UDS scaler. */
> >  if (vin->info->model != RCAR_GEN3)
> >  rvin_crop_scale_comp_gen2(vin);
> > @@ -676,6 +676,9 @@ static int rvin_setup(struct rvin_dev *vin)
> >
> >  input_is_yuv = true;
> >  break;
> > +case MEDIA_BUS_FMT_SRGGB8_1X8:
> > +vnmc |= VNMC_INF_RAW8;
> > +break;
> >  default:
> >  break;
> >  }
> > @@ -737,6 +740,9 @@ static int rvin_setup(struct rvin_dev *vin)
> >  case V4L2_PIX_FMT_ABGR32:
> >  dmr = VNDMR_A8BIT(vin->alpha) | VNDMR_EXRGB |
> VNDMR_DTMD_ARGB;
> >  break;
> > +case V4L2_PIX_FMT_SRGGB8:
> > +dmr = 0;
> > +break;
> >  default:
> >  vin_err(vin, "Invalid pixelformat (0x%x)\n",
> >  vin->format.pixelformat);
> > @@ -1110,6 +1116,7 @@ static int rvin_mc_validate_format(struct
> rvin_dev *vin, struct v4l2_subdev *sd,
> >  case MEDIA_BUS_FMT_UYVY8_2X8:
> >  case MEDIA_BUS_FMT_UYVY10_2X10:
> >  case MEDIA_BUS_FMT_RGB888_1X24:
> > +case MEDIA_BUS_FMT_SRGGB8_1X8:
> >  vin->mbus_code = fmt.format.code;
> >  break;
> >  default:
> > diff --git a/drivers/media/platform/rcar-vin/rcar-v4l2.c
> > b/drivers/media/platform/rcar-vin/rcar-v4l2.c
> > index 5151a3c..4698099 100644
> > --- a/drivers/media/platform/rcar-vin/rcar-v4l2.c
> > +++ b/drivers/media/platform/rcar-vin/rcar-v4l2.c
> > @@ -66,6 +66,10 @@ static const struct rvin_video_format rvin_formats[]
> = {
> >  .fourcc= V4L2_PIX_FMT_ABGR32,
> >  .bpp= 4,
> >  },
> > +{
> > +.fourcc= V4L2_PIX_FMT_SRGGB8,
> > +.bpp= 2,
>
> This does not look right, is not bytes-per-pixel 1 for a SRGGB8?
>
I guessed the bpp's were picked from VnIS table as I result I did the same.

> > +},
> >  };
> >
> >  const struct rvin_video_format *rvin_format_from_pixel(struct
> > rvin_dev *vin, @@ -102,6 +106,7 @@ static u32
> > rvin_format_bytesperline(struct rvin_dev *vin,  {
> >  const struct rvin_video_format *fmt;
> >  u32 align;
> > +u8 div;
> >
> >  fmt = rvin_format_from_pixel(vin, pix->pixelformat);
> >
> > @@ -112,16 +117,22 @@ static u32 rvin_format_bytesperline(struct
> rvin_dev *vin,
> >  case V4L2_PIX_FMT_NV12:
> >  case V4L2_PIX_FMT_NV16:
> >  align = 0x20;
> > +div = 1;
> > +break;
> > +case V4L2_PIX_FMT_SRGGB8:
> > +align = 0x10;
> > +div = 2;
>
> Yes this does not look right at all, I think you should set bpp to 1 and drop the
> div handling here.
>
If I set bpp as 1 and drop the div VNIS_REG will be wrongly configured in
rvin_crop_scale_comp() and the image captured will be wrong.

For example for 640x480:

With the current patch bpp = 2:
bytesperline = 640
image size = 307200
stride = 320

And with bpp = 1 and div removed
bytesperline = 640
image size = 307200
stride = 640

Cheers,
--Prabhakar

> >  break;
> >  default:
> >  align = 0x10;
> > +div = 1;
> >  break;
> >  }
> >
> >  if (V4L2_FIELD_IS_SEQUENTIAL(pix->field))
> >  align = 0x80;
> >
> > -return ALIGN(pix->width, align) * fmt->bpp;
> > +return ALIGN(pix->width / div, align) * fmt->bpp;
> >  }
> >
> >  static u32 rvin_format_sizeimage(struct v4l2_pix_format *pix)
> > --
> > 2.7.4
> >
>
> --
> Regards,
> Niklas Söderlund


Renesas Electronics Europe GmbH, Geschaeftsfuehrer/President: Carsten Jauch, Sitz der Gesellschaft/Registered office: Duesseldorf, Arcadiastrasse 10, 40472 Duesseldorf, Germany, Handelsregister/Commercial Register: Duesseldorf, HRB 3708 USt-IDNr./Tax identification no.: DE 119353406 WEEE-Reg.-Nr./WEEE reg. no.: DE 14978647

Re: [PATCH v2 2/3] media: rcar-vin: Add support for MEDIA_BUS_FMT_SRGGB8_1X8 format

[Niklas]

Hi Lad,

On 2020-03-10 13:42:20 +0000, Prabhakar Mahadev Lad wrote:
> Hi Niklas,
> 
> Thank for the review.
> 
> > -----Original Message-----
> > From: Niklas <niklas.soderlund@ragnatech.se>
> > Sent: 10 March 2020 12:46
> > To: Prabhakar Mahadev Lad <prabhakar.mahadev-lad.rj@bp.renesas.com>
> > Cc: Mauro Carvalho Chehab <mchehab@kernel.org>; linux-
> > media@vger.kernel.org; linux-renesas-soc@vger.kernel.org; linux-
> > kernel@vger.kernel.org; Lad Prabhakar <prabhakar.csengg@gmail.com>
> > Subject: Re: [PATCH v2 2/3] media: rcar-vin: Add support for
> > MEDIA_BUS_FMT_SRGGB8_1X8 format
> >
> > Hi Lad,
> >
> > Thanks for your work.
> >
> > On 2020-03-10 11:06:03 +0000, Lad Prabhakar wrote:
> > > Add support for MEDIA_BUS_FMT_SRGGB8_1X8 format in rcar-vin by
> > setting
> > > format type to RAW8 in VNMC register and appropriately setting the
> > > bpp, bytesperline to enable V4L2_PIX_FMT_SRGGB8.
> > >
> > > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-
> > lad.rj@bp.renesas.com>
> > > ---
> > >  drivers/media/platform/rcar-vin/rcar-core.c |  1 +
> > > drivers/media/platform/rcar-vin/rcar-dma.c  |  9 ++++++++-
> > > drivers/media/platform/rcar-vin/rcar-v4l2.c | 13 ++++++++++++-
> > >  3 files changed, 21 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/drivers/media/platform/rcar-vin/rcar-core.c
> > > b/drivers/media/platform/rcar-vin/rcar-core.c
> > > index 7440c89..76daf2f 100644
> > > --- a/drivers/media/platform/rcar-vin/rcar-core.c
> > > +++ b/drivers/media/platform/rcar-vin/rcar-core.c
> > > @@ -469,6 +469,7 @@ static int rvin_parallel_subdevice_attach(struct
> > rvin_dev *vin,
> > >  case MEDIA_BUS_FMT_UYVY8_2X8:
> > >  case MEDIA_BUS_FMT_UYVY10_2X10:
> > >  case MEDIA_BUS_FMT_RGB888_1X24:
> > > +case MEDIA_BUS_FMT_SRGGB8_1X8:
> > >  vin->mbus_code = code.code;
> > >  vin_dbg(vin, "Found media bus format for %s: %d\n",
> > >  subdev->name, vin->mbus_code);
> > > diff --git a/drivers/media/platform/rcar-vin/rcar-dma.c
> > > b/drivers/media/platform/rcar-vin/rcar-dma.c
> > > index 1a30cd0..1c1fafa 100644
> > > --- a/drivers/media/platform/rcar-vin/rcar-dma.c
> > > +++ b/drivers/media/platform/rcar-vin/rcar-dma.c
> > > @@ -85,6 +85,7 @@
> > >  #define VNMC_INF_YUV8_BT601(1 << 16)
> > >  #define VNMC_INF_YUV10_BT656(2 << 16)
> > >  #define VNMC_INF_YUV10_BT601(3 << 16)
> > > +#define VNMC_INF_RAW8(4 << 16)
> > >  #define VNMC_INF_YUV16(5 << 16)
> > >  #define VNMC_INF_RGB888(6 << 16)
> > >  #define VNMC_VUP(1 << 10)
> > > @@ -587,7 +588,6 @@ void rvin_crop_scale_comp(struct rvin_dev *vin)
> > >  rvin_write(vin, vin->crop.top, VNSLPRC_REG);
> > >  rvin_write(vin, vin->crop.top + vin->crop.height - 1, VNELPRC_REG);
> > >
> > > -
> > >  /* TODO: Add support for the UDS scaler. */
> > >  if (vin->info->model != RCAR_GEN3)
> > >  rvin_crop_scale_comp_gen2(vin);
> > > @@ -676,6 +676,9 @@ static int rvin_setup(struct rvin_dev *vin)
> > >
> > >  input_is_yuv = true;
> > >  break;
> > > +case MEDIA_BUS_FMT_SRGGB8_1X8:
> > > +vnmc |= VNMC_INF_RAW8;
> > > +break;
> > >  default:
> > >  break;
> > >  }
> > > @@ -737,6 +740,9 @@ static int rvin_setup(struct rvin_dev *vin)
> > >  case V4L2_PIX_FMT_ABGR32:
> > >  dmr = VNDMR_A8BIT(vin->alpha) | VNDMR_EXRGB |
> > VNDMR_DTMD_ARGB;
> > >  break;
> > > +case V4L2_PIX_FMT_SRGGB8:
> > > +dmr = 0;
> > > +break;
> > >  default:
> > >  vin_err(vin, "Invalid pixelformat (0x%x)\n",
> > >  vin->format.pixelformat);
> > > @@ -1110,6 +1116,7 @@ static int rvin_mc_validate_format(struct
> > rvin_dev *vin, struct v4l2_subdev *sd,
> > >  case MEDIA_BUS_FMT_UYVY8_2X8:
> > >  case MEDIA_BUS_FMT_UYVY10_2X10:
> > >  case MEDIA_BUS_FMT_RGB888_1X24:
> > > +case MEDIA_BUS_FMT_SRGGB8_1X8:
> > >  vin->mbus_code = fmt.format.code;
> > >  break;
> > >  default:
> > > diff --git a/drivers/media/platform/rcar-vin/rcar-v4l2.c
> > > b/drivers/media/platform/rcar-vin/rcar-v4l2.c
> > > index 5151a3c..4698099 100644
> > > --- a/drivers/media/platform/rcar-vin/rcar-v4l2.c
> > > +++ b/drivers/media/platform/rcar-vin/rcar-v4l2.c
> > > @@ -66,6 +66,10 @@ static const struct rvin_video_format rvin_formats[]
> > = {
> > >  .fourcc= V4L2_PIX_FMT_ABGR32,
> > >  .bpp= 4,
> > >  },
> > > +{
> > > +.fourcc= V4L2_PIX_FMT_SRGGB8,
> > > +.bpp= 2,
> >
> > This does not look right, is not bytes-per-pixel 1 for a SRGGB8?
> >
> I guessed the bpp's were picked from VnIS table as I result I did the same.
> 
> > > +},
> > >  };
> > >
> > >  const struct rvin_video_format *rvin_format_from_pixel(struct
> > > rvin_dev *vin, @@ -102,6 +106,7 @@ static u32
> > > rvin_format_bytesperline(struct rvin_dev *vin,  {
> > >  const struct rvin_video_format *fmt;
> > >  u32 align;
> > > +u8 div;
> > >
> > >  fmt = rvin_format_from_pixel(vin, pix->pixelformat);
> > >
> > > @@ -112,16 +117,22 @@ static u32 rvin_format_bytesperline(struct
> > rvin_dev *vin,
> > >  case V4L2_PIX_FMT_NV12:
> > >  case V4L2_PIX_FMT_NV16:
> > >  align = 0x20;
> > > +div = 1;
> > > +break;
> > > +case V4L2_PIX_FMT_SRGGB8:
> > > +align = 0x10;
> > > +div = 2;
> >
> > Yes this does not look right at all, I think you should set bpp to 1 and drop the
> > div handling here.
> >
> If I set bpp as 1 and drop the div VNIS_REG will be wrongly configured in
> rvin_crop_scale_comp() and the image captured will be wrong.
> 
> For example for 640x480:
> 
> With the current patch bpp = 2:
> bytesperline = 640

This is wrong, if we have a line of 640 pixels and 2 bytes per pixel 
then bytesperline must be at least 1280 bytes right?

> image size = 307200
> stride = 320

But this is incorrect, the VNIS_REG shall be at least the number of 
pixels in a line (EPPrC - SPPrC -> 640 - 0 = 640). Then we need to align 
it to the pixel unit (16, 32, 64, 128) depending on the output pixel 
format.

This usually result in a stride that is larger then the line length.  
Thus you need a test application that knows the difference between width 
* bpp and bytesperline. I use qv4l2 without opengl support when I do quick 
tests and it does not support this hence I get a incorrect visual view 
of the stream when testing.

How does the image capture fail with bpp = 1?

> 
> And with bpp = 1 and div removed
> bytesperline = 640
> image size = 307200
> stride = 640


> 
> Cheers,
> --Prabhakar
> 
> > >  break;
> > >  default:
> > >  align = 0x10;
> > > +div = 1;
> > >  break;
> > >  }
> > >
> > >  if (V4L2_FIELD_IS_SEQUENTIAL(pix->field))
> > >  align = 0x80;
> > >
> > > -return ALIGN(pix->width, align) * fmt->bpp;
> > > +return ALIGN(pix->width / div, align) * fmt->bpp;
> > >  }
> > >
> > >  static u32 rvin_format_sizeimage(struct v4l2_pix_format *pix)
> > > --
> > > 2.7.4
> > >
> >
> > --
> > Regards,
> > Niklas Söderlund
> 
> 
> Renesas Electronics Europe GmbH, Geschaeftsfuehrer/President: Carsten Jauch, Sitz der Gesellschaft/Registered office: Duesseldorf, Arcadiastrasse 10, 40472 Duesseldorf, Germany, Handelsregister/Commercial Register: Duesseldorf, HRB 3708 USt-IDNr./Tax identification no.: DE 119353406 WEEE-Reg.-Nr./WEEE reg. no.: DE 14978647

-- 
Regards,
Niklas Söderlund

Re: [PATCH v2 2/3] media: rcar-vin: Add support for MEDIA_BUS_FMT_SRGGB8_1X8 format

[Niklas]

[-- Attachment #1: Type: text/plain, Size: 9993 bytes --]

Hi Prabhakar,

Thanks for the sample files, sorry I did not have time before now to 
look at them. After doing so I believe I know what is wrong, see bellow.

On 2020-03-15 19:35:58 +0000, Lad, Prabhakar wrote:
> Hi Niklas,
> 
> On Tue, Mar 10, 2020 at 2:06 PM Niklas <niklas.soderlund@ragnatech.se> wrote:
> >
> > Hi Lad,
> >
> > On 2020-03-10 13:42:20 +0000, Prabhakar Mahadev Lad wrote:
> > > Hi Niklas,
> > >
> > > Thank for the review.
> > >
> > > > -----Original Message-----
> > > > From: Niklas <niklas.soderlund@ragnatech.se>
> > > > Sent: 10 March 2020 12:46
> > > > To: Prabhakar Mahadev Lad <prabhakar.mahadev-lad.rj@bp.renesas.com>
> > > > Cc: Mauro Carvalho Chehab <mchehab@kernel.org>; linux-
> > > > media@vger.kernel.org; linux-renesas-soc@vger.kernel.org; linux-
> > > > kernel@vger.kernel.org; Lad Prabhakar <prabhakar.csengg@gmail.com>
> > > > Subject: Re: [PATCH v2 2/3] media: rcar-vin: Add support for
> > > > MEDIA_BUS_FMT_SRGGB8_1X8 format
> > > >
> > > > Hi Lad,
> > > >
> > > > Thanks for your work.
> > > >
> > > > On 2020-03-10 11:06:03 +0000, Lad Prabhakar wrote:
> > > > > Add support for MEDIA_BUS_FMT_SRGGB8_1X8 format in rcar-vin by
> > > > setting
> > > > > format type to RAW8 in VNMC register and appropriately setting the
> > > > > bpp, bytesperline to enable V4L2_PIX_FMT_SRGGB8.
> > > > >
> > > > > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-
> > > > lad.rj@bp.renesas.com>
> > > > > ---
> > > > >  drivers/media/platform/rcar-vin/rcar-core.c |  1 +
> > > > > drivers/media/platform/rcar-vin/rcar-dma.c  |  9 ++++++++-
> > > > > drivers/media/platform/rcar-vin/rcar-v4l2.c | 13 ++++++++++++-
> > > > >  3 files changed, 21 insertions(+), 2 deletions(-)
> > > > >
> > > > > diff --git a/drivers/media/platform/rcar-vin/rcar-core.c
> > > > > b/drivers/media/platform/rcar-vin/rcar-core.c
> > > > > index 7440c89..76daf2f 100644
> > > > > --- a/drivers/media/platform/rcar-vin/rcar-core.c
> > > > > +++ b/drivers/media/platform/rcar-vin/rcar-core.c
> > > > > @@ -469,6 +469,7 @@ static int rvin_parallel_subdevice_attach(struct
> > > > rvin_dev *vin,
> > > > >  case MEDIA_BUS_FMT_UYVY8_2X8:
> > > > >  case MEDIA_BUS_FMT_UYVY10_2X10:
> > > > >  case MEDIA_BUS_FMT_RGB888_1X24:
> > > > > +case MEDIA_BUS_FMT_SRGGB8_1X8:
> > > > >  vin->mbus_code = code.code;
> > > > >  vin_dbg(vin, "Found media bus format for %s: %d\n",
> > > > >  subdev->name, vin->mbus_code);
> > > > > diff --git a/drivers/media/platform/rcar-vin/rcar-dma.c
> > > > > b/drivers/media/platform/rcar-vin/rcar-dma.c
> > > > > index 1a30cd0..1c1fafa 100644
> > > > > --- a/drivers/media/platform/rcar-vin/rcar-dma.c
> > > > > +++ b/drivers/media/platform/rcar-vin/rcar-dma.c
> > > > > @@ -85,6 +85,7 @@
> > > > >  #define VNMC_INF_YUV8_BT601(1 << 16)
> > > > >  #define VNMC_INF_YUV10_BT656(2 << 16)
> > > > >  #define VNMC_INF_YUV10_BT601(3 << 16)
> > > > > +#define VNMC_INF_RAW8(4 << 16)
> > > > >  #define VNMC_INF_YUV16(5 << 16)
> > > > >  #define VNMC_INF_RGB888(6 << 16)
> > > > >  #define VNMC_VUP(1 << 10)
> > > > > @@ -587,7 +588,6 @@ void rvin_crop_scale_comp(struct rvin_dev *vin)
> > > > >  rvin_write(vin, vin->crop.top, VNSLPRC_REG);
> > > > >  rvin_write(vin, vin->crop.top + vin->crop.height - 1, VNELPRC_REG);
> > > > >
> > > > > -
> > > > >  /* TODO: Add support for the UDS scaler. */
> > > > >  if (vin->info->model != RCAR_GEN3)
> > > > >  rvin_crop_scale_comp_gen2(vin);
> > > > > @@ -676,6 +676,9 @@ static int rvin_setup(struct rvin_dev *vin)
> > > > >
> > > > >  input_is_yuv = true;
> > > > >  break;
> > > > > +case MEDIA_BUS_FMT_SRGGB8_1X8:
> > > > > +vnmc |= VNMC_INF_RAW8;
> > > > > +break;
> > > > >  default:
> > > > >  break;
> > > > >  }
> > > > > @@ -737,6 +740,9 @@ static int rvin_setup(struct rvin_dev *vin)
> > > > >  case V4L2_PIX_FMT_ABGR32:
> > > > >  dmr = VNDMR_A8BIT(vin->alpha) | VNDMR_EXRGB |
> > > > VNDMR_DTMD_ARGB;
> > > > >  break;
> > > > > +case V4L2_PIX_FMT_SRGGB8:
> > > > > +dmr = 0;
> > > > > +break;
> > > > >  default:
> > > > >  vin_err(vin, "Invalid pixelformat (0x%x)\n",
> > > > >  vin->format.pixelformat);
> > > > > @@ -1110,6 +1116,7 @@ static int rvin_mc_validate_format(struct
> > > > rvin_dev *vin, struct v4l2_subdev *sd,
> > > > >  case MEDIA_BUS_FMT_UYVY8_2X8:
> > > > >  case MEDIA_BUS_FMT_UYVY10_2X10:
> > > > >  case MEDIA_BUS_FMT_RGB888_1X24:
> > > > > +case MEDIA_BUS_FMT_SRGGB8_1X8:
> > > > >  vin->mbus_code = fmt.format.code;
> > > > >  break;
> > > > >  default:
> > > > > diff --git a/drivers/media/platform/rcar-vin/rcar-v4l2.c
> > > > > b/drivers/media/platform/rcar-vin/rcar-v4l2.c
> > > > > index 5151a3c..4698099 100644
> > > > > --- a/drivers/media/platform/rcar-vin/rcar-v4l2.c
> > > > > +++ b/drivers/media/platform/rcar-vin/rcar-v4l2.c
> > > > > @@ -66,6 +66,10 @@ static const struct rvin_video_format rvin_formats[]
> > > > = {
> > > > >  .fourcc= V4L2_PIX_FMT_ABGR32,
> > > > >  .bpp= 4,
> > > > >  },
> > > > > +{
> > > > > +.fourcc= V4L2_PIX_FMT_SRGGB8,
> > > > > +.bpp= 2,
> > > >
> > > > This does not look right, is not bytes-per-pixel 1 for a SRGGB8?
> > > >
> > > I guessed the bpp's were picked from VnIS table as I result I did the same.
> > >
> > > > > +},
> > > > >  };
> > > > >
> > > > >  const struct rvin_video_format *rvin_format_from_pixel(struct
> > > > > rvin_dev *vin, @@ -102,6 +106,7 @@ static u32
> > > > > rvin_format_bytesperline(struct rvin_dev *vin,  {
> > > > >  const struct rvin_video_format *fmt;
> > > > >  u32 align;
> > > > > +u8 div;
> > > > >
> > > > >  fmt = rvin_format_from_pixel(vin, pix->pixelformat);
> > > > >
> > > > > @@ -112,16 +117,22 @@ static u32 rvin_format_bytesperline(struct
> > > > rvin_dev *vin,
> > > > >  case V4L2_PIX_FMT_NV12:
> > > > >  case V4L2_PIX_FMT_NV16:
> > > > >  align = 0x20;
> > > > > +div = 1;
> > > > > +break;
> > > > > +case V4L2_PIX_FMT_SRGGB8:
> > > > > +align = 0x10;
> > > > > +div = 2;
> > > >
> > > > Yes this does not look right at all, I think you should set bpp to 1 and drop the
> > > > div handling here.
> > > >
> > > If I set bpp as 1 and drop the div VNIS_REG will be wrongly configured in
> > > rvin_crop_scale_comp() and the image captured will be wrong.
> > >
> > > For example for 640x480:
> > >
> > > With the current patch bpp = 2:
> > > bytesperline = 640
> >
> > This is wrong, if we have a line of 640 pixels and 2 bytes per pixel
> > then bytesperline must be at least 1280 bytes right?
> >
> > > image size = 307200
> > > stride = 320
> >
> > But this is incorrect, the VNIS_REG shall be at least the number of
> > pixels in a line (EPPrC - SPPrC -> 640 - 0 = 640). Then we need to align
> > it to the pixel unit (16, 32, 64, 128) depending on the output pixel
> > format.
> >
> > This usually result in a stride that is larger then the line length.
> > Thus you need a test application that knows the difference between width
> > * bpp and bytesperline. I use qv4l2 without opengl support when I do quick
> > tests and it does not support this hence I get a incorrect visual view
> > of the stream when testing.
> >
> > How does the image capture fail with bpp = 1?
> >
> Attached is the captured bayer images 640x480 with bpp set to 1, for
> file1bppstridediv1.raw
> VNIS_REG stride set to 640 and for file file1bppstridediv2.raw
> VNIS_REF stride  set to (640 * 1) / 2.
> When the file1bppstridediv1.raw image is converted to png colors are incorrect
> but whereas file1bppstridediv2.raw converted to png the picture is clear.
> 
> Also while doing a loop-back to fbdevsink with the below pipeline:
> gst-launch-1.0 -vvv v4l2src device=/dev/video0 io-mode=dmabuf ! 'video/x-bayer,
> format=rggb,width=640,height=480,framerate=30/1' ! queue ! bayer2rgb !
> videoconvert
> ! fbdevsink sync=false
> 
> width = 640 height = 480 bpp = 1, bytesperline = 640 stride = 320
> works correctly
> width = 640 height = 480 bpp = 1, bytesperline = 640 stride = 640
> image displayed is incorrect

It's very unlogical to have a stride that is less then the width, which 
got me interested why the second one gave you better results. I wrote a 
small python hack which converts the raw SRGGB8 to PNG without any 
debyaer, just rows of RGRGRG and BGBGBG.

Looking at the output of that seems your sensor is not sending frames of 
640x480 but 480x640. Both the raw files you sent have holes in them.  
The first line is always 480 pixels of data and then there are sections 
of no data, followed by good data. Some rows are chopped and some have 
their 480 bytes of good data on the "left" and some on the "right" side 
of the frame.

So for rcar-vin I think the following settings are what you want,

    width = 480 height = 640 bpp = 1, bytesperline = 480* stride = 480
    * = I have not checked if this fits with alignment for VNIS

I have attached the python hack and the two generated png files from 
your raw files so you can play with them yourself.

> 
> Cheers,
> --Prabhakar
> 
> > >
> > > And with bpp = 1 and div removed
> > > bytesperline = 640
> > > image size = 307200
> > > stride = 640
> >
> >
> > >
> > > Cheers,
> > > --Prabhakar
> > >
> > > > >  break;
> > > > >  default:
> > > > >  align = 0x10;
> > > > > +div = 1;
> > > > >  break;
> > > > >  }
> > > > >
> > > > >  if (V4L2_FIELD_IS_SEQUENTIAL(pix->field))
> > > > >  align = 0x80;
> > > > >
> > > > > -return ALIGN(pix->width, align) * fmt->bpp;
> > > > > +return ALIGN(pix->width / div, align) * fmt->bpp;
> > > > >  }
> > > > >
> > > > >  static u32 rvin_format_sizeimage(struct v4l2_pix_format *pix)
> > > > > --
> > > > > 2.7.4
> > > > >
> > > >
> > > > --
> > > > Regards,
> > > > Niklas Söderlund
> > >
> > >
> > > Renesas Electronics Europe GmbH, Geschaeftsfuehrer/President: Carsten Jauch, Sitz der Gesellschaft/Registered office: Duesseldorf, Arcadiastrasse 10, 40472 Duesseldorf, Germany, Handelsregister/Commercial Register: Duesseldorf, HRB 3708 USt-IDNr./Tax identification no.: DE 119353406 WEEE-Reg.-Nr./WEEE reg. no.: DE 14978647
> >
> > --
> > Regards,
> > Niklas Söderlund




-- 
Regards,
Niklas Söderlund

[-- Attachment #2: rggb2png.py --]
[-- Type: text/plain, Size: 1514 bytes --]

#!/usr/bin/python3

import argparse
import os
os.environ['PYGAME_HIDE_SUPPORT_PROMPT'] = "hide"
import pygame

def bytes_from_file(filename, chunksize=8192):
    with open(filename, "rb") as f:
        while True:
            chunk = f.read(chunksize)
            if chunk:
                for b in chunk:
                    yield b
            else:
                break

def work(raw, width, height, png):
    pygame.init()

    screen = pygame.display.set_mode((width, height))
    screen.fill((0, 0, 0))

    pos = 0
    for byte in bytes_from_file(raw):
        x = pos % width
        y = int(pos / height)
        pos += 1

        if y % 2 == 0:
            if x % 2 == 0:
                color = (byte, 0, 0)
            else:
                color = (0, byte, 0)
        else:
            if x % 2 == 0:
                color = (0, byte, 0)
            else:
                color = (0, 0, byte)

        screen.set_at((x, y), color)

    pygame.display.update()

    if (png):
        pygame.image.save(screen, png)

    running = True
    while running:
        for event in pygame.event.get():
            if event.type == pygame.QUIT:
                running = False

def main():
    parser = argparse.ArgumentParser(description='View SRGGB8 and optionally save to PNG')
    parser.add_argument('-w', '--write', dest='output', help='Write png file')
    parser.add_argument('input')
    args = parser.parse_args()

    work(args.input, 640, 480, args.output)

if __name__ == '__main__':
    main()

[-- Attachment #3: div1.png --]
[-- Type: image/png, Size: 69165 bytes --]

[-- Attachment #4: div2.png --]
[-- Type: image/png, Size: 111515 bytes --]

Re: [PATCH v2 2/3] media: rcar-vin: Add support for MEDIA_BUS_FMT_SRGGB8_1X8 format

[Niklas]

Hi Lad,

Thanks for your reply.

On 2020-03-27 12:59:52 +0000, Lad, Prabhakar wrote:
> Hi Niklas,
> 
> On Thu, Mar 19, 2020 at 3:03 PM Niklas <niklas.soderlund@ragnatech.se> wrote:
> >
> > Hi Prabhakar,
> >
> > Thanks for the sample files, sorry I did not have time before now to
> > look at them. After doing so I believe I know what is wrong, see bellow.
> >
> > On 2020-03-15 19:35:58 +0000, Lad, Prabhakar wrote:
> > > Hi Niklas,
> > >
> > > On Tue, Mar 10, 2020 at 2:06 PM Niklas <niklas.soderlund@ragnatech.se> wrote:
> > > >
> > > > Hi Lad,
> > > >
> > > > On 2020-03-10 13:42:20 +0000, Prabhakar Mahadev Lad wrote:
> > > > > Hi Niklas,
> > > > >
> > > > > Thank for the review.
> > > > >
> > > > > > -----Original Message-----
> > > > > > From: Niklas <niklas.soderlund@ragnatech.se>
> > > > > > Sent: 10 March 2020 12:46
> > > > > > To: Prabhakar Mahadev Lad <prabhakar.mahadev-lad.rj@bp.renesas.com>
> > > > > > Cc: Mauro Carvalho Chehab <mchehab@kernel.org>; linux-
> > > > > > media@vger.kernel.org; linux-renesas-soc@vger.kernel.org; linux-
> > > > > > kernel@vger.kernel.org; Lad Prabhakar <prabhakar.csengg@gmail.com>
> > > > > > Subject: Re: [PATCH v2 2/3] media: rcar-vin: Add support for
> > > > > > MEDIA_BUS_FMT_SRGGB8_1X8 format
> > > > > >
> > > > > > Hi Lad,
> > > > > >
> > > > > > Thanks for your work.
> > > > > >
> > > > > > On 2020-03-10 11:06:03 +0000, Lad Prabhakar wrote:
> > > > > > > Add support for MEDIA_BUS_FMT_SRGGB8_1X8 format in rcar-vin by
> > > > > > setting
> > > > > > > format type to RAW8 in VNMC register and appropriately setting the
> > > > > > > bpp, bytesperline to enable V4L2_PIX_FMT_SRGGB8.
> > > > > > >
> > > > > > > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-
> > > > > > lad.rj@bp.renesas.com>
> > > > > > > ---
> > > > > > >  drivers/media/platform/rcar-vin/rcar-core.c |  1 +
> > > > > > > drivers/media/platform/rcar-vin/rcar-dma.c  |  9 ++++++++-
> > > > > > > drivers/media/platform/rcar-vin/rcar-v4l2.c | 13 ++++++++++++-
> > > > > > >  3 files changed, 21 insertions(+), 2 deletions(-)
> > > > > > >
> > > > > > > diff --git a/drivers/media/platform/rcar-vin/rcar-core.c
> > > > > > > b/drivers/media/platform/rcar-vin/rcar-core.c
> > > > > > > index 7440c89..76daf2f 100644
> > > > > > > --- a/drivers/media/platform/rcar-vin/rcar-core.c
> > > > > > > +++ b/drivers/media/platform/rcar-vin/rcar-core.c
> > > > > > > @@ -469,6 +469,7 @@ static int rvin_parallel_subdevice_attach(struct
> > > > > > rvin_dev *vin,
> > > > > > >  case MEDIA_BUS_FMT_UYVY8_2X8:
> > > > > > >  case MEDIA_BUS_FMT_UYVY10_2X10:
> > > > > > >  case MEDIA_BUS_FMT_RGB888_1X24:
> > > > > > > +case MEDIA_BUS_FMT_SRGGB8_1X8:
> > > > > > >  vin->mbus_code = code.code;
> > > > > > >  vin_dbg(vin, "Found media bus format for %s: %d\n",
> > > > > > >  subdev->name, vin->mbus_code);
> > > > > > > diff --git a/drivers/media/platform/rcar-vin/rcar-dma.c
> > > > > > > b/drivers/media/platform/rcar-vin/rcar-dma.c
> > > > > > > index 1a30cd0..1c1fafa 100644
> > > > > > > --- a/drivers/media/platform/rcar-vin/rcar-dma.c
> > > > > > > +++ b/drivers/media/platform/rcar-vin/rcar-dma.c
> > > > > > > @@ -85,6 +85,7 @@
> > > > > > >  #define VNMC_INF_YUV8_BT601(1 << 16)
> > > > > > >  #define VNMC_INF_YUV10_BT656(2 << 16)
> > > > > > >  #define VNMC_INF_YUV10_BT601(3 << 16)
> > > > > > > +#define VNMC_INF_RAW8(4 << 16)
> > > > > > >  #define VNMC_INF_YUV16(5 << 16)
> > > > > > >  #define VNMC_INF_RGB888(6 << 16)
> > > > > > >  #define VNMC_VUP(1 << 10)
> > > > > > > @@ -587,7 +588,6 @@ void rvin_crop_scale_comp(struct rvin_dev *vin)
> > > > > > >  rvin_write(vin, vin->crop.top, VNSLPRC_REG);
> > > > > > >  rvin_write(vin, vin->crop.top + vin->crop.height - 1, VNELPRC_REG);
> > > > > > >
> > > > > > > -
> > > > > > >  /* TODO: Add support for the UDS scaler. */
> > > > > > >  if (vin->info->model != RCAR_GEN3)
> > > > > > >  rvin_crop_scale_comp_gen2(vin);
> > > > > > > @@ -676,6 +676,9 @@ static int rvin_setup(struct rvin_dev *vin)
> > > > > > >
> > > > > > >  input_is_yuv = true;
> > > > > > >  break;
> > > > > > > +case MEDIA_BUS_FMT_SRGGB8_1X8:
> > > > > > > +vnmc |= VNMC_INF_RAW8;
> > > > > > > +break;
> > > > > > >  default:
> > > > > > >  break;
> > > > > > >  }
> > > > > > > @@ -737,6 +740,9 @@ static int rvin_setup(struct rvin_dev *vin)
> > > > > > >  case V4L2_PIX_FMT_ABGR32:
> > > > > > >  dmr = VNDMR_A8BIT(vin->alpha) | VNDMR_EXRGB |
> > > > > > VNDMR_DTMD_ARGB;
> > > > > > >  break;
> > > > > > > +case V4L2_PIX_FMT_SRGGB8:
> > > > > > > +dmr = 0;
> > > > > > > +break;
> > > > > > >  default:
> > > > > > >  vin_err(vin, "Invalid pixelformat (0x%x)\n",
> > > > > > >  vin->format.pixelformat);
> > > > > > > @@ -1110,6 +1116,7 @@ static int rvin_mc_validate_format(struct
> > > > > > rvin_dev *vin, struct v4l2_subdev *sd,
> > > > > > >  case MEDIA_BUS_FMT_UYVY8_2X8:
> > > > > > >  case MEDIA_BUS_FMT_UYVY10_2X10:
> > > > > > >  case MEDIA_BUS_FMT_RGB888_1X24:
> > > > > > > +case MEDIA_BUS_FMT_SRGGB8_1X8:
> > > > > > >  vin->mbus_code = fmt.format.code;
> > > > > > >  break;
> > > > > > >  default:
> > > > > > > diff --git a/drivers/media/platform/rcar-vin/rcar-v4l2.c
> > > > > > > b/drivers/media/platform/rcar-vin/rcar-v4l2.c
> > > > > > > index 5151a3c..4698099 100644
> > > > > > > --- a/drivers/media/platform/rcar-vin/rcar-v4l2.c
> > > > > > > +++ b/drivers/media/platform/rcar-vin/rcar-v4l2.c
> > > > > > > @@ -66,6 +66,10 @@ static const struct rvin_video_format rvin_formats[]
> > > > > > = {
> > > > > > >  .fourcc= V4L2_PIX_FMT_ABGR32,
> > > > > > >  .bpp= 4,
> > > > > > >  },
> > > > > > > +{
> > > > > > > +.fourcc= V4L2_PIX_FMT_SRGGB8,
> > > > > > > +.bpp= 2,
> > > > > >
> > > > > > This does not look right, is not bytes-per-pixel 1 for a SRGGB8?
> > > > > >
> > > > > I guessed the bpp's were picked from VnIS table as I result I did the same.
> > > > >
> > > > > > > +},
> > > > > > >  };
> > > > > > >
> > > > > > >  const struct rvin_video_format *rvin_format_from_pixel(struct
> > > > > > > rvin_dev *vin, @@ -102,6 +106,7 @@ static u32
> > > > > > > rvin_format_bytesperline(struct rvin_dev *vin,  {
> > > > > > >  const struct rvin_video_format *fmt;
> > > > > > >  u32 align;
> > > > > > > +u8 div;
> > > > > > >
> > > > > > >  fmt = rvin_format_from_pixel(vin, pix->pixelformat);
> > > > > > >
> > > > > > > @@ -112,16 +117,22 @@ static u32 rvin_format_bytesperline(struct
> > > > > > rvin_dev *vin,
> > > > > > >  case V4L2_PIX_FMT_NV12:
> > > > > > >  case V4L2_PIX_FMT_NV16:
> > > > > > >  align = 0x20;
> > > > > > > +div = 1;
> > > > > > > +break;
> > > > > > > +case V4L2_PIX_FMT_SRGGB8:
> > > > > > > +align = 0x10;
> > > > > > > +div = 2;
> > > > > >
> > > > > > Yes this does not look right at all, I think you should set bpp to 1 and drop the
> > > > > > div handling here.
> > > > > >
> > > > > If I set bpp as 1 and drop the div VNIS_REG will be wrongly configured in
> > > > > rvin_crop_scale_comp() and the image captured will be wrong.
> > > > >
> > > > > For example for 640x480:
> > > > >
> > > > > With the current patch bpp = 2:
> > > > > bytesperline = 640
> > > >
> > > > This is wrong, if we have a line of 640 pixels and 2 bytes per pixel
> > > > then bytesperline must be at least 1280 bytes right?
> > > >
> > > > > image size = 307200
> > > > > stride = 320
> > > >
> > > > But this is incorrect, the VNIS_REG shall be at least the number of
> > > > pixels in a line (EPPrC - SPPrC -> 640 - 0 = 640). Then we need to align
> > > > it to the pixel unit (16, 32, 64, 128) depending on the output pixel
> > > > format.
> > > >
> > > > This usually result in a stride that is larger then the line length.
> > > > Thus you need a test application that knows the difference between width
> > > > * bpp and bytesperline. I use qv4l2 without opengl support when I do quick
> > > > tests and it does not support this hence I get a incorrect visual view
> > > > of the stream when testing.
> > > >
> > > > How does the image capture fail with bpp = 1?
> > > >
> > > Attached is the captured bayer images 640x480 with bpp set to 1, for
> > > file1bppstridediv1.raw
> > > VNIS_REG stride set to 640 and for file file1bppstridediv2.raw
> > > VNIS_REF stride  set to (640 * 1) / 2.
> > > When the file1bppstridediv1.raw image is converted to png colors are incorrect
> > > but whereas file1bppstridediv2.raw converted to png the picture is clear.
> > >
> > > Also while doing a loop-back to fbdevsink with the below pipeline:
> > > gst-launch-1.0 -vvv v4l2src device=/dev/video0 io-mode=dmabuf ! 'video/x-bayer,
> > > format=rggb,width=640,height=480,framerate=30/1' ! queue ! bayer2rgb !
> > > videoconvert
> > > ! fbdevsink sync=false
> > >
> > > width = 640 height = 480 bpp = 1, bytesperline = 640 stride = 320
> > > works correctly
> > > width = 640 height = 480 bpp = 1, bytesperline = 640 stride = 640
> > > image displayed is incorrect
> >
> > It's very unlogical to have a stride that is less then the width, which
> > got me interested why the second one gave you better results. I wrote a
> > small python hack which converts the raw SRGGB8 to PNG without any
> > debyaer, just rows of RGRGRG and BGBGBG.
> >
> Finally I have some information from the hardware team, the VIN process RAW8
> in 2 pixel units as a result the stride for RAW8 needs to be
> configured as bytesperline/2.

Interesting, that is not how I have interpreted the datasheet. But 
rereading it now after our discussion I see how it could be so. I will 
dig into it during the week and see if I get make it all click in my 
head. Thanks for pointing this out.

> 
> The python script which you attached doesn't seem to do the right
> conversion. I discovered
> that Shotwell Viewer on Ubuntu can open raw files. I also confirmed
> this by bayer2rg [1]

Oops, you are right. My bad sorry for sending you down that path.

> 
> # ./bayer2rgb --input=file1bppstridediv1.raw --output=file1.tiff
> --width=640 --height=480 --bpp=8 --first=RGGB --method=BILINEAR --tiff
> # ./bayer2rgb --input=file1bppstridediv2.raw --output=file2.tiff
> --width=640 --height=480 --bpp=8 --first=RGGB --method=BILINEAR --tiff
> 
> # convert file1.tiff file1bppstridediv1.png
> # convert file2.tiff file1bppstridediv2.png
> 
> Attached are the png images for reference.
> 
> > Looking at the output of that seems your sensor is not sending frames of
> > 640x480 but 480x640. Both the raw files you sent have holes in them.
> > The first line is always 480 pixels of data and then there are sections
> > of no data, followed by good data. Some rows are chopped and some have
> > their 480 bytes of good data on the "left" and some on the "right" side
> > of the frame.
> >
> I can confirm the sensor is sending 640x480 as the support for same
> was added recently in IMX219 driver
> and  was  was tested on raspi by the maintainer.
> 
> [1] https://github.com/jdthomas/bayer2rgb
> 
> Cheers,
> --Prabhakar
> 
> > So for rcar-vin I think the following settings are what you want,
> >
> >     width = 480 height = 640 bpp = 1, bytesperline = 480* stride = 480
> >     * = I have not checked if this fits with alignment for VNIS
> >
> > I have attached the python hack and the two generated png files from
> > your raw files so you can play with them yourself.
> >
> > >
> > > Cheers,
> > > --Prabhakar
> > >
> > > > >
> > > > > And with bpp = 1 and div removed
> > > > > bytesperline = 640
> > > > > image size = 307200
> > > > > stride = 640
> > > >
> > > >
> > > > >
> > > > > Cheers,
> > > > > --Prabhakar
> > > > >
> > > > > > >  break;
> > > > > > >  default:
> > > > > > >  align = 0x10;
> > > > > > > +div = 1;
> > > > > > >  break;
> > > > > > >  }
> > > > > > >
> > > > > > >  if (V4L2_FIELD_IS_SEQUENTIAL(pix->field))
> > > > > > >  align = 0x80;
> > > > > > >
> > > > > > > -return ALIGN(pix->width, align) * fmt->bpp;
> > > > > > > +return ALIGN(pix->width / div, align) * fmt->bpp;
> > > > > > >  }
> > > > > > >
> > > > > > >  static u32 rvin_format_sizeimage(struct v4l2_pix_format *pix)
> > > > > > > --
> > > > > > > 2.7.4
> > > > > > >
> > > > > >
> > > > > > --
> > > > > > Regards,
> > > > > > Niklas Söderlund
> > > > >
> > > > >
> > > > > Renesas Electronics Europe GmbH, Geschaeftsfuehrer/President: Carsten Jauch, Sitz der Gesellschaft/Registered office: Duesseldorf, Arcadiastrasse 10, 40472 Duesseldorf, Germany, Handelsregister/Commercial Register: Duesseldorf, HRB 3708 USt-IDNr./Tax identification no.: DE 119353406 WEEE-Reg.-Nr./WEEE reg. no.: DE 14978647
> > > >
> > > > --
> > > > Regards,
> > > > Niklas Söderlund
> >
> >
> >
> >
> > --
> > Regards,
> > Niklas Söderlund




-- 
Regards,
Niklas Söderlund

Re: [PATCH v2 2/3] media: rcar-vin: Add support for MEDIA_BUS_FMT_SRGGB8_1X8 format

[Lad, Prabhakar]

Hi Niklas,

On Mon, Mar 30, 2020 at 1:07 PM Niklas <niklas.soderlund@ragnatech.se> wrote:
>
> Hi Lad,
>
> Thanks for your reply.
>
> On 2020-03-27 12:59:52 +0000, Lad, Prabhakar wrote:
> > Hi Niklas,
> >
> > On Thu, Mar 19, 2020 at 3:03 PM Niklas <niklas.soderlund@ragnatech.se> wrote:
> > >
> > > Hi Prabhakar,
> > >
> > > Thanks for the sample files, sorry I did not have time before now to
> > > look at them. After doing so I believe I know what is wrong, see bellow.
> > >
> > > On 2020-03-15 19:35:58 +0000, Lad, Prabhakar wrote:
> > > > Hi Niklas,
> > > >
> > > > On Tue, Mar 10, 2020 at 2:06 PM Niklas <niklas.soderlund@ragnatech.se> wrote:
> > > > >
> > > > > Hi Lad,
> > > > >
> > > > > On 2020-03-10 13:42:20 +0000, Prabhakar Mahadev Lad wrote:
> > > > > > Hi Niklas,
> > > > > >
> > > > > > Thank for the review.
> > > > > >
> > > > > > > -----Original Message-----
> > > > > > > From: Niklas <niklas.soderlund@ragnatech.se>
> > > > > > > Sent: 10 March 2020 12:46
> > > > > > > To: Prabhakar Mahadev Lad <prabhakar.mahadev-lad.rj@bp.renesas.com>
> > > > > > > Cc: Mauro Carvalho Chehab <mchehab@kernel.org>; linux-
> > > > > > > media@vger.kernel.org; linux-renesas-soc@vger.kernel.org; linux-
> > > > > > > kernel@vger.kernel.org; Lad Prabhakar <prabhakar.csengg@gmail.com>
> > > > > > > Subject: Re: [PATCH v2 2/3] media: rcar-vin: Add support for
> > > > > > > MEDIA_BUS_FMT_SRGGB8_1X8 format
> > > > > > >
> > > > > > > Hi Lad,
> > > > > > >
> > > > > > > Thanks for your work.
> > > > > > >
> > > > > > > On 2020-03-10 11:06:03 +0000, Lad Prabhakar wrote:
> > > > > > > > Add support for MEDIA_BUS_FMT_SRGGB8_1X8 format in rcar-vin by
> > > > > > > setting
> > > > > > > > format type to RAW8 in VNMC register and appropriately setting the
> > > > > > > > bpp, bytesperline to enable V4L2_PIX_FMT_SRGGB8.
> > > > > > > >
> > > > > > > > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-
> > > > > > > lad.rj@bp.renesas.com>
> > > > > > > > ---
> > > > > > > >  drivers/media/platform/rcar-vin/rcar-core.c |  1 +
> > > > > > > > drivers/media/platform/rcar-vin/rcar-dma.c  |  9 ++++++++-
> > > > > > > > drivers/media/platform/rcar-vin/rcar-v4l2.c | 13 ++++++++++++-
> > > > > > > >  3 files changed, 21 insertions(+), 2 deletions(-)
> > > > > > > >
> > > > > > > > diff --git a/drivers/media/platform/rcar-vin/rcar-core.c
> > > > > > > > b/drivers/media/platform/rcar-vin/rcar-core.c
> > > > > > > > index 7440c89..76daf2f 100644
> > > > > > > > --- a/drivers/media/platform/rcar-vin/rcar-core.c
> > > > > > > > +++ b/drivers/media/platform/rcar-vin/rcar-core.c
> > > > > > > > @@ -469,6 +469,7 @@ static int rvin_parallel_subdevice_attach(struct
> > > > > > > rvin_dev *vin,
> > > > > > > >  case MEDIA_BUS_FMT_UYVY8_2X8:
> > > > > > > >  case MEDIA_BUS_FMT_UYVY10_2X10:
> > > > > > > >  case MEDIA_BUS_FMT_RGB888_1X24:
> > > > > > > > +case MEDIA_BUS_FMT_SRGGB8_1X8:
> > > > > > > >  vin->mbus_code = code.code;
> > > > > > > >  vin_dbg(vin, "Found media bus format for %s: %d\n",
> > > > > > > >  subdev->name, vin->mbus_code);
> > > > > > > > diff --git a/drivers/media/platform/rcar-vin/rcar-dma.c
> > > > > > > > b/drivers/media/platform/rcar-vin/rcar-dma.c
> > > > > > > > index 1a30cd0..1c1fafa 100644
> > > > > > > > --- a/drivers/media/platform/rcar-vin/rcar-dma.c
> > > > > > > > +++ b/drivers/media/platform/rcar-vin/rcar-dma.c
> > > > > > > > @@ -85,6 +85,7 @@
> > > > > > > >  #define VNMC_INF_YUV8_BT601(1 << 16)
> > > > > > > >  #define VNMC_INF_YUV10_BT656(2 << 16)
> > > > > > > >  #define VNMC_INF_YUV10_BT601(3 << 16)
> > > > > > > > +#define VNMC_INF_RAW8(4 << 16)
> > > > > > > >  #define VNMC_INF_YUV16(5 << 16)
> > > > > > > >  #define VNMC_INF_RGB888(6 << 16)
> > > > > > > >  #define VNMC_VUP(1 << 10)
> > > > > > > > @@ -587,7 +588,6 @@ void rvin_crop_scale_comp(struct rvin_dev *vin)
> > > > > > > >  rvin_write(vin, vin->crop.top, VNSLPRC_REG);
> > > > > > > >  rvin_write(vin, vin->crop.top + vin->crop.height - 1, VNELPRC_REG);
> > > > > > > >
> > > > > > > > -
> > > > > > > >  /* TODO: Add support for the UDS scaler. */
> > > > > > > >  if (vin->info->model != RCAR_GEN3)
> > > > > > > >  rvin_crop_scale_comp_gen2(vin);
> > > > > > > > @@ -676,6 +676,9 @@ static int rvin_setup(struct rvin_dev *vin)
> > > > > > > >
> > > > > > > >  input_is_yuv = true;
> > > > > > > >  break;
> > > > > > > > +case MEDIA_BUS_FMT_SRGGB8_1X8:
> > > > > > > > +vnmc |= VNMC_INF_RAW8;
> > > > > > > > +break;
> > > > > > > >  default:
> > > > > > > >  break;
> > > > > > > >  }
> > > > > > > > @@ -737,6 +740,9 @@ static int rvin_setup(struct rvin_dev *vin)
> > > > > > > >  case V4L2_PIX_FMT_ABGR32:
> > > > > > > >  dmr = VNDMR_A8BIT(vin->alpha) | VNDMR_EXRGB |
> > > > > > > VNDMR_DTMD_ARGB;
> > > > > > > >  break;
> > > > > > > > +case V4L2_PIX_FMT_SRGGB8:
> > > > > > > > +dmr = 0;
> > > > > > > > +break;
> > > > > > > >  default:
> > > > > > > >  vin_err(vin, "Invalid pixelformat (0x%x)\n",
> > > > > > > >  vin->format.pixelformat);
> > > > > > > > @@ -1110,6 +1116,7 @@ static int rvin_mc_validate_format(struct
> > > > > > > rvin_dev *vin, struct v4l2_subdev *sd,
> > > > > > > >  case MEDIA_BUS_FMT_UYVY8_2X8:
> > > > > > > >  case MEDIA_BUS_FMT_UYVY10_2X10:
> > > > > > > >  case MEDIA_BUS_FMT_RGB888_1X24:
> > > > > > > > +case MEDIA_BUS_FMT_SRGGB8_1X8:
> > > > > > > >  vin->mbus_code = fmt.format.code;
> > > > > > > >  break;
> > > > > > > >  default:
> > > > > > > > diff --git a/drivers/media/platform/rcar-vin/rcar-v4l2.c
> > > > > > > > b/drivers/media/platform/rcar-vin/rcar-v4l2.c
> > > > > > > > index 5151a3c..4698099 100644
> > > > > > > > --- a/drivers/media/platform/rcar-vin/rcar-v4l2.c
> > > > > > > > +++ b/drivers/media/platform/rcar-vin/rcar-v4l2.c
> > > > > > > > @@ -66,6 +66,10 @@ static const struct rvin_video_format rvin_formats[]
> > > > > > > = {
> > > > > > > >  .fourcc= V4L2_PIX_FMT_ABGR32,
> > > > > > > >  .bpp= 4,
> > > > > > > >  },
> > > > > > > > +{
> > > > > > > > +.fourcc= V4L2_PIX_FMT_SRGGB8,
> > > > > > > > +.bpp= 2,
> > > > > > >
> > > > > > > This does not look right, is not bytes-per-pixel 1 for a SRGGB8?
> > > > > > >
> > > > > > I guessed the bpp's were picked from VnIS table as I result I did the same.
> > > > > >
> > > > > > > > +},
> > > > > > > >  };
> > > > > > > >
> > > > > > > >  const struct rvin_video_format *rvin_format_from_pixel(struct
> > > > > > > > rvin_dev *vin, @@ -102,6 +106,7 @@ static u32
> > > > > > > > rvin_format_bytesperline(struct rvin_dev *vin,  {
> > > > > > > >  const struct rvin_video_format *fmt;
> > > > > > > >  u32 align;
> > > > > > > > +u8 div;
> > > > > > > >
> > > > > > > >  fmt = rvin_format_from_pixel(vin, pix->pixelformat);
> > > > > > > >
> > > > > > > > @@ -112,16 +117,22 @@ static u32 rvin_format_bytesperline(struct
> > > > > > > rvin_dev *vin,
> > > > > > > >  case V4L2_PIX_FMT_NV12:
> > > > > > > >  case V4L2_PIX_FMT_NV16:
> > > > > > > >  align = 0x20;
> > > > > > > > +div = 1;
> > > > > > > > +break;
> > > > > > > > +case V4L2_PIX_FMT_SRGGB8:
> > > > > > > > +align = 0x10;
> > > > > > > > +div = 2;
> > > > > > >
> > > > > > > Yes this does not look right at all, I think you should set bpp to 1 and drop the
> > > > > > > div handling here.
> > > > > > >
> > > > > > If I set bpp as 1 and drop the div VNIS_REG will be wrongly configured in
> > > > > > rvin_crop_scale_comp() and the image captured will be wrong.
> > > > > >
> > > > > > For example for 640x480:
> > > > > >
> > > > > > With the current patch bpp = 2:
> > > > > > bytesperline = 640
> > > > >
> > > > > This is wrong, if we have a line of 640 pixels and 2 bytes per pixel
> > > > > then bytesperline must be at least 1280 bytes right?
> > > > >
> > > > > > image size = 307200
> > > > > > stride = 320
> > > > >
> > > > > But this is incorrect, the VNIS_REG shall be at least the number of
> > > > > pixels in a line (EPPrC - SPPrC -> 640 - 0 = 640). Then we need to align
> > > > > it to the pixel unit (16, 32, 64, 128) depending on the output pixel
> > > > > format.
> > > > >
> > > > > This usually result in a stride that is larger then the line length.
> > > > > Thus you need a test application that knows the difference between width
> > > > > * bpp and bytesperline. I use qv4l2 without opengl support when I do quick
> > > > > tests and it does not support this hence I get a incorrect visual view
> > > > > of the stream when testing.
> > > > >
> > > > > How does the image capture fail with bpp = 1?
> > > > >
> > > > Attached is the captured bayer images 640x480 with bpp set to 1, for
> > > > file1bppstridediv1.raw
> > > > VNIS_REG stride set to 640 and for file file1bppstridediv2.raw
> > > > VNIS_REF stride  set to (640 * 1) / 2.
> > > > When the file1bppstridediv1.raw image is converted to png colors are incorrect
> > > > but whereas file1bppstridediv2.raw converted to png the picture is clear.
> > > >
> > > > Also while doing a loop-back to fbdevsink with the below pipeline:
> > > > gst-launch-1.0 -vvv v4l2src device=/dev/video0 io-mode=dmabuf ! 'video/x-bayer,
> > > > format=rggb,width=640,height=480,framerate=30/1' ! queue ! bayer2rgb !
> > > > videoconvert
> > > > ! fbdevsink sync=false
> > > >
> > > > width = 640 height = 480 bpp = 1, bytesperline = 640 stride = 320
> > > > works correctly
> > > > width = 640 height = 480 bpp = 1, bytesperline = 640 stride = 640
> > > > image displayed is incorrect
> > >
> > > It's very unlogical to have a stride that is less then the width, which
> > > got me interested why the second one gave you better results. I wrote a
> > > small python hack which converts the raw SRGGB8 to PNG without any
> > > debyaer, just rows of RGRGRG and BGBGBG.
> > >
> > Finally I have some information from the hardware team, the VIN process RAW8
> > in 2 pixel units as a result the stride for RAW8 needs to be
> > configured as bytesperline/2.
>
> Interesting, that is not how I have interpreted the datasheet. But
> rereading it now after our discussion I see how it could be so. I will
> dig into it during the week and see if I get make it all click in my
> head. Thanks for pointing this out.
>
Great, In that case Ill wait before I post a V4.

> >
> > The python script which you attached doesn't seem to do the right
> > conversion. I discovered
> > that Shotwell Viewer on Ubuntu can open raw files. I also confirmed
> > this by bayer2rg [1]
>
> Oops, you are right. My bad sorry for sending you down that path.
>
That gave me an opportunity to explore a bit :)

Cheers,
--Prabhakar

> >
> > # ./bayer2rgb --input=file1bppstridediv1.raw --output=file1.tiff
> > --width=640 --height=480 --bpp=8 --first=RGGB --method=BILINEAR --tiff
> > # ./bayer2rgb --input=file1bppstridediv2.raw --output=file2.tiff
> > --width=640 --height=480 --bpp=8 --first=RGGB --method=BILINEAR --tiff
> >
> > # convert file1.tiff file1bppstridediv1.png
> > # convert file2.tiff file1bppstridediv2.png
> >
> > Attached are the png images for reference.
> >
> > > Looking at the output of that seems your sensor is not sending frames of
> > > 640x480 but 480x640. Both the raw files you sent have holes in them.
> > > The first line is always 480 pixels of data and then there are sections
> > > of no data, followed by good data. Some rows are chopped and some have
> > > their 480 bytes of good data on the "left" and some on the "right" side
> > > of the frame.
> > >
> > I can confirm the sensor is sending 640x480 as the support for same
> > was added recently in IMX219 driver
> > and  was  was tested on raspi by the maintainer.
> >
> > [1] https://github.com/jdthomas/bayer2rgb
> >
> > Cheers,
> > --Prabhakar
> >
> > > So for rcar-vin I think the following settings are what you want,
> > >
> > >     width = 480 height = 640 bpp = 1, bytesperline = 480* stride = 480
> > >     * = I have not checked if this fits with alignment for VNIS
> > >
> > > I have attached the python hack and the two generated png files from
> > > your raw files so you can play with them yourself.
> > >
> > > >
> > > > Cheers,
> > > > --Prabhakar
> > > >
> > > > > >
> > > > > > And with bpp = 1 and div removed
> > > > > > bytesperline = 640
> > > > > > image size = 307200
> > > > > > stride = 640
> > > > >
> > > > >
> > > > > >
> > > > > > Cheers,
> > > > > > --Prabhakar
> > > > > >
> > > > > > > >  break;
> > > > > > > >  default:
> > > > > > > >  align = 0x10;
> > > > > > > > +div = 1;
> > > > > > > >  break;
> > > > > > > >  }
> > > > > > > >
> > > > > > > >  if (V4L2_FIELD_IS_SEQUENTIAL(pix->field))
> > > > > > > >  align = 0x80;
> > > > > > > >
> > > > > > > > -return ALIGN(pix->width, align) * fmt->bpp;
> > > > > > > > +return ALIGN(pix->width / div, align) * fmt->bpp;
> > > > > > > >  }
> > > > > > > >
> > > > > > > >  static u32 rvin_format_sizeimage(struct v4l2_pix_format *pix)
> > > > > > > > --
> > > > > > > > 2.7.4
> > > > > > > >
> > > > > > >
> > > > > > > --
> > > > > > > Regards,
> > > > > > > Niklas Söderlund
> > > > > >
> > > > > >
> > > > > > Renesas Electronics Europe GmbH, Geschaeftsfuehrer/President: Carsten Jauch, Sitz der Gesellschaft/Registered office: Duesseldorf, Arcadiastrasse 10, 40472 Duesseldorf, Germany, Handelsregister/Commercial Register: Duesseldorf, HRB 3708 USt-IDNr./Tax identification no.: DE 119353406 WEEE-Reg.-Nr./WEEE reg. no.: DE 14978647
> > > > >
> > > > > --
> > > > > Regards,
> > > > > Niklas Söderlund
> > >
> > >
> > >
> > >
> > > --
> > > Regards,
> > > Niklas Söderlund
>
>
>
>
> --
> Regards,
> Niklas Söderlund

Re: [PATCH v2 2/3] media: rcar-vin: Add support for MEDIA_BUS_FMT_SRGGB8_1X8 format

[Lad, Prabhakar]

HI Niklas,

On Mon, Mar 30, 2020 at 1:07 PM Niklas <niklas.soderlund@ragnatech.se> wrote:
>
> Hi Lad,
>
> Thanks for your reply.
>
> On 2020-03-27 12:59:52 +0000, Lad, Prabhakar wrote:
> > Hi Niklas,
> >
> > On Thu, Mar 19, 2020 at 3:03 PM Niklas <niklas.soderlund@ragnatech.se> wrote:
> > >
> > > Hi Prabhakar,
> > >
> > > Thanks for the sample files, sorry I did not have time before now to
> > > look at them. After doing so I believe I know what is wrong, see bellow.
> > >
> > > On 2020-03-15 19:35:58 +0000, Lad, Prabhakar wrote:
> > > > Hi Niklas,
> > > >
> > > > On Tue, Mar 10, 2020 at 2:06 PM Niklas <niklas.soderlund@ragnatech.se> wrote:
> > > > >
> > > > > Hi Lad,
> > > > >
> > > > > On 2020-03-10 13:42:20 +0000, Prabhakar Mahadev Lad wrote:
> > > > > > Hi Niklas,
> > > > > >
> > > > > > Thank for the review.
> > > > > >
> > > > > > > -----Original Message-----
> > > > > > > From: Niklas <niklas.soderlund@ragnatech.se>
> > > > > > > Sent: 10 March 2020 12:46
> > > > > > > To: Prabhakar Mahadev Lad <prabhakar.mahadev-lad.rj@bp.renesas.com>
> > > > > > > Cc: Mauro Carvalho Chehab <mchehab@kernel.org>; linux-
> > > > > > > media@vger.kernel.org; linux-renesas-soc@vger.kernel.org; linux-
> > > > > > > kernel@vger.kernel.org; Lad Prabhakar <prabhakar.csengg@gmail.com>
> > > > > > > Subject: Re: [PATCH v2 2/3] media: rcar-vin: Add support for
> > > > > > > MEDIA_BUS_FMT_SRGGB8_1X8 format
> > > > > > >
> > > > > > > Hi Lad,
> > > > > > >
> > > > > > > Thanks for your work.
> > > > > > >
> > > > > > > On 2020-03-10 11:06:03 +0000, Lad Prabhakar wrote:
> > > > > > > > Add support for MEDIA_BUS_FMT_SRGGB8_1X8 format in rcar-vin by
> > > > > > > setting
> > > > > > > > format type to RAW8 in VNMC register and appropriately setting the
> > > > > > > > bpp, bytesperline to enable V4L2_PIX_FMT_SRGGB8.
> > > > > > > >
> > > > > > > > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-
> > > > > > > lad.rj@bp.renesas.com>
> > > > > > > > ---
> > > > > > > >  drivers/media/platform/rcar-vin/rcar-core.c |  1 +
> > > > > > > > drivers/media/platform/rcar-vin/rcar-dma.c  |  9 ++++++++-
> > > > > > > > drivers/media/platform/rcar-vin/rcar-v4l2.c | 13 ++++++++++++-
> > > > > > > >  3 files changed, 21 insertions(+), 2 deletions(-)
> > > > > > > >
> > > > > > > > diff --git a/drivers/media/platform/rcar-vin/rcar-core.c
> > > > > > > > b/drivers/media/platform/rcar-vin/rcar-core.c
> > > > > > > > index 7440c89..76daf2f 100644
> > > > > > > > --- a/drivers/media/platform/rcar-vin/rcar-core.c
> > > > > > > > +++ b/drivers/media/platform/rcar-vin/rcar-core.c
> > > > > > > > @@ -469,6 +469,7 @@ static int rvin_parallel_subdevice_attach(struct
> > > > > > > rvin_dev *vin,
> > > > > > > >  case MEDIA_BUS_FMT_UYVY8_2X8:
> > > > > > > >  case MEDIA_BUS_FMT_UYVY10_2X10:
> > > > > > > >  case MEDIA_BUS_FMT_RGB888_1X24:
> > > > > > > > +case MEDIA_BUS_FMT_SRGGB8_1X8:
> > > > > > > >  vin->mbus_code = code.code;
> > > > > > > >  vin_dbg(vin, "Found media bus format for %s: %d\n",
> > > > > > > >  subdev->name, vin->mbus_code);
> > > > > > > > diff --git a/drivers/media/platform/rcar-vin/rcar-dma.c
> > > > > > > > b/drivers/media/platform/rcar-vin/rcar-dma.c
> > > > > > > > index 1a30cd0..1c1fafa 100644
> > > > > > > > --- a/drivers/media/platform/rcar-vin/rcar-dma.c
> > > > > > > > +++ b/drivers/media/platform/rcar-vin/rcar-dma.c
> > > > > > > > @@ -85,6 +85,7 @@
> > > > > > > >  #define VNMC_INF_YUV8_BT601(1 << 16)
> > > > > > > >  #define VNMC_INF_YUV10_BT656(2 << 16)
> > > > > > > >  #define VNMC_INF_YUV10_BT601(3 << 16)
> > > > > > > > +#define VNMC_INF_RAW8(4 << 16)
> > > > > > > >  #define VNMC_INF_YUV16(5 << 16)
> > > > > > > >  #define VNMC_INF_RGB888(6 << 16)
> > > > > > > >  #define VNMC_VUP(1 << 10)
> > > > > > > > @@ -587,7 +588,6 @@ void rvin_crop_scale_comp(struct rvin_dev *vin)
> > > > > > > >  rvin_write(vin, vin->crop.top, VNSLPRC_REG);
> > > > > > > >  rvin_write(vin, vin->crop.top + vin->crop.height - 1, VNELPRC_REG);
> > > > > > > >
> > > > > > > > -
> > > > > > > >  /* TODO: Add support for the UDS scaler. */
> > > > > > > >  if (vin->info->model != RCAR_GEN3)
> > > > > > > >  rvin_crop_scale_comp_gen2(vin);
> > > > > > > > @@ -676,6 +676,9 @@ static int rvin_setup(struct rvin_dev *vin)
> > > > > > > >
> > > > > > > >  input_is_yuv = true;
> > > > > > > >  break;
> > > > > > > > +case MEDIA_BUS_FMT_SRGGB8_1X8:
> > > > > > > > +vnmc |= VNMC_INF_RAW8;
> > > > > > > > +break;
> > > > > > > >  default:
> > > > > > > >  break;
> > > > > > > >  }
> > > > > > > > @@ -737,6 +740,9 @@ static int rvin_setup(struct rvin_dev *vin)
> > > > > > > >  case V4L2_PIX_FMT_ABGR32:
> > > > > > > >  dmr = VNDMR_A8BIT(vin->alpha) | VNDMR_EXRGB |
> > > > > > > VNDMR_DTMD_ARGB;
> > > > > > > >  break;
> > > > > > > > +case V4L2_PIX_FMT_SRGGB8:
> > > > > > > > +dmr = 0;
> > > > > > > > +break;
> > > > > > > >  default:
> > > > > > > >  vin_err(vin, "Invalid pixelformat (0x%x)\n",
> > > > > > > >  vin->format.pixelformat);
> > > > > > > > @@ -1110,6 +1116,7 @@ static int rvin_mc_validate_format(struct
> > > > > > > rvin_dev *vin, struct v4l2_subdev *sd,
> > > > > > > >  case MEDIA_BUS_FMT_UYVY8_2X8:
> > > > > > > >  case MEDIA_BUS_FMT_UYVY10_2X10:
> > > > > > > >  case MEDIA_BUS_FMT_RGB888_1X24:
> > > > > > > > +case MEDIA_BUS_FMT_SRGGB8_1X8:
> > > > > > > >  vin->mbus_code = fmt.format.code;
> > > > > > > >  break;
> > > > > > > >  default:
> > > > > > > > diff --git a/drivers/media/platform/rcar-vin/rcar-v4l2.c
> > > > > > > > b/drivers/media/platform/rcar-vin/rcar-v4l2.c
> > > > > > > > index 5151a3c..4698099 100644
> > > > > > > > --- a/drivers/media/platform/rcar-vin/rcar-v4l2.c
> > > > > > > > +++ b/drivers/media/platform/rcar-vin/rcar-v4l2.c
> > > > > > > > @@ -66,6 +66,10 @@ static const struct rvin_video_format rvin_formats[]
> > > > > > > = {
> > > > > > > >  .fourcc= V4L2_PIX_FMT_ABGR32,
> > > > > > > >  .bpp= 4,
> > > > > > > >  },
> > > > > > > > +{
> > > > > > > > +.fourcc= V4L2_PIX_FMT_SRGGB8,
> > > > > > > > +.bpp= 2,
> > > > > > >
> > > > > > > This does not look right, is not bytes-per-pixel 1 for a SRGGB8?
> > > > > > >
> > > > > > I guessed the bpp's were picked from VnIS table as I result I did the same.
> > > > > >
> > > > > > > > +},
> > > > > > > >  };
> > > > > > > >
> > > > > > > >  const struct rvin_video_format *rvin_format_from_pixel(struct
> > > > > > > > rvin_dev *vin, @@ -102,6 +106,7 @@ static u32
> > > > > > > > rvin_format_bytesperline(struct rvin_dev *vin,  {
> > > > > > > >  const struct rvin_video_format *fmt;
> > > > > > > >  u32 align;
> > > > > > > > +u8 div;
> > > > > > > >
> > > > > > > >  fmt = rvin_format_from_pixel(vin, pix->pixelformat);
> > > > > > > >
> > > > > > > > @@ -112,16 +117,22 @@ static u32 rvin_format_bytesperline(struct
> > > > > > > rvin_dev *vin,
> > > > > > > >  case V4L2_PIX_FMT_NV12:
> > > > > > > >  case V4L2_PIX_FMT_NV16:
> > > > > > > >  align = 0x20;
> > > > > > > > +div = 1;
> > > > > > > > +break;
> > > > > > > > +case V4L2_PIX_FMT_SRGGB8:
> > > > > > > > +align = 0x10;
> > > > > > > > +div = 2;
> > > > > > >
> > > > > > > Yes this does not look right at all, I think you should set bpp to 1 and drop the
> > > > > > > div handling here.
> > > > > > >
> > > > > > If I set bpp as 1 and drop the div VNIS_REG will be wrongly configured in
> > > > > > rvin_crop_scale_comp() and the image captured will be wrong.
> > > > > >
> > > > > > For example for 640x480:
> > > > > >
> > > > > > With the current patch bpp = 2:
> > > > > > bytesperline = 640
> > > > >
> > > > > This is wrong, if we have a line of 640 pixels and 2 bytes per pixel
> > > > > then bytesperline must be at least 1280 bytes right?
> > > > >
> > > > > > image size = 307200
> > > > > > stride = 320
> > > > >
> > > > > But this is incorrect, the VNIS_REG shall be at least the number of
> > > > > pixels in a line (EPPrC - SPPrC -> 640 - 0 = 640). Then we need to align
> > > > > it to the pixel unit (16, 32, 64, 128) depending on the output pixel
> > > > > format.
> > > > >
> > > > > This usually result in a stride that is larger then the line length.
> > > > > Thus you need a test application that knows the difference between width
> > > > > * bpp and bytesperline. I use qv4l2 without opengl support when I do quick
> > > > > tests and it does not support this hence I get a incorrect visual view
> > > > > of the stream when testing.
> > > > >
> > > > > How does the image capture fail with bpp = 1?
> > > > >
> > > > Attached is the captured bayer images 640x480 with bpp set to 1, for
> > > > file1bppstridediv1.raw
> > > > VNIS_REG stride set to 640 and for file file1bppstridediv2.raw
> > > > VNIS_REF stride  set to (640 * 1) / 2.
> > > > When the file1bppstridediv1.raw image is converted to png colors are incorrect
> > > > but whereas file1bppstridediv2.raw converted to png the picture is clear.
> > > >
> > > > Also while doing a loop-back to fbdevsink with the below pipeline:
> > > > gst-launch-1.0 -vvv v4l2src device=/dev/video0 io-mode=dmabuf ! 'video/x-bayer,
> > > > format=rggb,width=640,height=480,framerate=30/1' ! queue ! bayer2rgb !
> > > > videoconvert
> > > > ! fbdevsink sync=false
> > > >
> > > > width = 640 height = 480 bpp = 1, bytesperline = 640 stride = 320
> > > > works correctly
> > > > width = 640 height = 480 bpp = 1, bytesperline = 640 stride = 640
> > > > image displayed is incorrect
> > >
> > > It's very unlogical to have a stride that is less then the width, which
> > > got me interested why the second one gave you better results. I wrote a
> > > small python hack which converts the raw SRGGB8 to PNG without any
> > > debyaer, just rows of RGRGRG and BGBGBG.
> > >
> > Finally I have some information from the hardware team, the VIN process RAW8
> > in 2 pixel units as a result the stride for RAW8 needs to be
> > configured as bytesperline/2.
>
> Interesting, that is not how I have interpreted the datasheet. But
> rereading it now after our discussion I see how it could be so. I will
> dig into it during the week and see if I get make it all click in my
> head. Thanks for pointing this out.
>
Did you manage to get the required information on this ?

Cheers,
--Prabhakar

Re: [PATCH v2 2/3] media: rcar-vin: Add support for MEDIA_BUS_FMT_SRGGB8_1X8 format

[Niklas]

Hi Lad,

On 2020-04-06 18:20:33 +0100, Lad, Prabhakar wrote:
> Did you manage to get the required information on this ?

I'm still working on it, sorry for not completing it last week. I will 
let you know as soon as I can.

-- 
Regards,
Niklas Söderlund

Re: [PATCH v2 2/3] media: rcar-vin: Add support for MEDIA_BUS_FMT_SRGGB8_1X8 format

[Niklas]

Hi Lad,

I spent all day playing with different solutions to how to move forward 
with this. My main problem is I have no setup where I can produce RAW 
image formats to test. But reading the datasheet I see the problem you 
are trying to solve.

I think for now the best solution might be to in rvin_crop_scale_comp() 
add a check for if the pixelformat is RAW and cut the value written to 
VNIS_REG in half. The bpp for the format shall still be set to 1.


    fmt = rvin_format_from_pixel(vin, vin->format.pixelformat);
    stride = vin->format.bytesperline / fmt->bpp;

    if (vin->format.pixelformat == V4L2_PIX_FMT_SRGGB8)
        stride /= 2;

    rvin_write(vin, stride, VNIS_REG);

I would also add a nice big comment above the if () that explains why 
the stride is cut in half for raw.

On 2020-04-07 11:56:23 +0200, Niklas wrote:
> Hi Lad,
> 
> On 2020-04-06 18:20:33 +0100, Lad, Prabhakar wrote:
> > Did you manage to get the required information on this ?
> 
> I'm still working on it, sorry for not completing it last week. I will 
> let you know as soon as I can.
> 
> -- 
> Regards,
> Niklas Söderlund

-- 
Regards,
Niklas Söderlund

Re: [PATCH v2 2/3] media: rcar-vin: Add support for MEDIA_BUS_FMT_SRGGB8_1X8 format

[Lad, Prabhakar]

Hi Niklas,

On Tue, Apr 14, 2020 at 8:39 PM Niklas <niklas.soderlund@ragnatech.se> wrote:
>
> Hi Lad,
>
> I spent all day playing with different solutions to how to move forward
> with this. My main problem is I have no setup where I can produce RAW
> image formats to test. But reading the datasheet I see the problem you
> are trying to solve.
>
Thank you for looking into this.

> I think for now the best solution might be to in rvin_crop_scale_comp()
> add a check for if the pixelformat is RAW and cut the value written to
> VNIS_REG in half. The bpp for the format shall still be set to 1.
>
>
>     fmt = rvin_format_from_pixel(vin, vin->format.pixelformat);
>     stride = vin->format.bytesperline / fmt->bpp;
>
>     if (vin->format.pixelformat == V4L2_PIX_FMT_SRGGB8)
>         stride /= 2;
>
>     rvin_write(vin, stride, VNIS_REG);
>
> I would also add a nice big comment above the if () that explains why
> the stride is cut in half for raw.
>
Agreed shall do that as above.

Cheers,
--Prabhakar

> On 2020-04-07 11:56:23 +0200, Niklas wrote:
> > Hi Lad,
> >
> > On 2020-04-06 18:20:33 +0100, Lad, Prabhakar wrote:
> > > Did you manage to get the required information on this ?
> >
> > I'm still working on it, sorry for not completing it last week. I will
> > let you know as soon as I can.
> >
> > --
> > Regards,
> > Niklas Söderlund
>
> --
> Regards,
> Niklas Söderlund

[PATCH v2 3/3] media: rcar-vin: rcar-csi2: Add support for MEDIA_BUS_FMT_SRGGB8_1X8 format

[Lad Prabhakar]

This patch adds support for MEDIA_BUS_FMT_SRGGB8_1X8 format for CSI2
input.

Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
---
 drivers/media/platform/rcar-vin/rcar-csi2.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/media/platform/rcar-vin/rcar-csi2.c b/drivers/media/platform/rcar-vin/rcar-csi2.c
index 39e1639..b030ef6 100644
--- a/drivers/media/platform/rcar-vin/rcar-csi2.c
+++ b/drivers/media/platform/rcar-vin/rcar-csi2.c
@@ -320,6 +320,7 @@ static const struct rcar_csi2_format rcar_csi2_formats[] = {
 	{ .code = MEDIA_BUS_FMT_YUYV8_1X16,	.datatype = 0x1e, .bpp = 16 },
 	{ .code = MEDIA_BUS_FMT_UYVY8_2X8,	.datatype = 0x1e, .bpp = 16 },
 	{ .code = MEDIA_BUS_FMT_YUYV10_2X10,	.datatype = 0x1e, .bpp = 20 },
+	{ .code = MEDIA_BUS_FMT_SRGGB8_1X8,     .datatype = 0x2a, .bpp = 8 },
 };
 
 static const struct rcar_csi2_format *rcsi2_code_to_fmt(unsigned int code)
-- 
2.7.4

Re: [PATCH v2 3/3] media: rcar-vin: rcar-csi2: Add support for MEDIA_BUS_FMT_SRGGB8_1X8 format

[Niklas]

Hi Lad,

Thanks for your work.

On 2020-03-10 11:06:04 +0000, Lad Prabhakar wrote:
> This patch adds support for MEDIA_BUS_FMT_SRGGB8_1X8 format for CSI2
> input.
> 
> Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>

Small nit, you can drop rcar-vin from the subject as this patch is for 
the rcar-csi2 driver. With this fixed,

Reviewed-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>

> ---
>  drivers/media/platform/rcar-vin/rcar-csi2.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/media/platform/rcar-vin/rcar-csi2.c b/drivers/media/platform/rcar-vin/rcar-csi2.c
> index 39e1639..b030ef6 100644
> --- a/drivers/media/platform/rcar-vin/rcar-csi2.c
> +++ b/drivers/media/platform/rcar-vin/rcar-csi2.c
> @@ -320,6 +320,7 @@ static const struct rcar_csi2_format rcar_csi2_formats[] = {
>  	{ .code = MEDIA_BUS_FMT_YUYV8_1X16,	.datatype = 0x1e, .bpp = 16 },
>  	{ .code = MEDIA_BUS_FMT_UYVY8_2X8,	.datatype = 0x1e, .bpp = 16 },
>  	{ .code = MEDIA_BUS_FMT_YUYV10_2X10,	.datatype = 0x1e, .bpp = 20 },
> +	{ .code = MEDIA_BUS_FMT_SRGGB8_1X8,     .datatype = 0x2a, .bpp = 8 },
>  };
>  
>  static const struct rcar_csi2_format *rcsi2_code_to_fmt(unsigned int code)
> -- 
> 2.7.4
> 

-- 
Regards,
Niklas Söderlund

RE: [PATCH v2 3/3] media: rcar-vin: rcar-csi2: Add support for MEDIA_BUS_FMT_SRGGB8_1X8 format

[Prabhakar Mahadev Lad]

Hi Niklas,

Thank you for the review.

> -----Original Message-----
> From: Niklas <niklas.soderlund@ragnatech.se>
> Sent: 10 March 2020 12:49
> To: Prabhakar Mahadev Lad <prabhakar.mahadev-lad.rj@bp.renesas.com>
> Cc: Mauro Carvalho Chehab <mchehab@kernel.org>; linux-
> media@vger.kernel.org; linux-renesas-soc@vger.kernel.org; linux-
> kernel@vger.kernel.org; Lad Prabhakar <prabhakar.csengg@gmail.com>
> Subject: Re: [PATCH v2 3/3] media: rcar-vin: rcar-csi2: Add support for
> MEDIA_BUS_FMT_SRGGB8_1X8 format
>
> Hi Lad,
>
> Thanks for your work.
>
> On 2020-03-10 11:06:04 +0000, Lad Prabhakar wrote:
> > This patch adds support for MEDIA_BUS_FMT_SRGGB8_1X8 format for
> CSI2
> > input.
> >
> > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-
> lad.rj@bp.renesas.com>
>
> Small nit, you can drop rcar-vin from the subject as this patch is for the rcar-
> csi2 driver. With this fixed,
>
Sure will update it for next iteration.

Cheers,
--Prabhakar

> Reviewed-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
>
> > ---
> >  drivers/media/platform/rcar-vin/rcar-csi2.c | 1 +
> >  1 file changed, 1 insertion(+)
> >
> > diff --git a/drivers/media/platform/rcar-vin/rcar-csi2.c
> > b/drivers/media/platform/rcar-vin/rcar-csi2.c
> > index 39e1639..b030ef6 100644
> > --- a/drivers/media/platform/rcar-vin/rcar-csi2.c
> > +++ b/drivers/media/platform/rcar-vin/rcar-csi2.c
> > @@ -320,6 +320,7 @@ static const struct rcar_csi2_format
> rcar_csi2_formats[] = {
> >  { .code = MEDIA_BUS_FMT_YUYV8_1X16,.datatype = 0x1e,
> .bpp = 16 },
> >  { .code = MEDIA_BUS_FMT_UYVY8_2X8,.datatype = 0x1e,
> .bpp = 16 },
> >  { .code = MEDIA_BUS_FMT_YUYV10_2X10,.datatype = 0x1e,
> .bpp = 20 },
> > +{ .code = MEDIA_BUS_FMT_SRGGB8_1X8,     .datatype = 0x2a, .bpp =
> 8 },
> >  };
> >
> >  static const struct rcar_csi2_format *rcsi2_code_to_fmt(unsigned int
> > code)
> > --
> > 2.7.4
> >
>
> --
> Regards,
> Niklas Söderlund


Renesas Electronics Europe GmbH, Geschaeftsfuehrer/President: Carsten Jauch, Sitz der Gesellschaft/Registered office: Duesseldorf, Arcadiastrasse 10, 40472 Duesseldorf, Germany, Handelsregister/Commercial Register: Duesseldorf, HRB 3708 USt-IDNr./Tax identification no.: DE 119353406 WEEE-Reg.-Nr./WEEE reg. no.: DE 14978647