* [PATCH v2 0/3] media: rcar-vin: Enable MEDIA_BUS_FMT_SRGGB8_1X8 format and support for matching fwnode against endpoints/nodes
@ 2020-03-10 11:06 Lad Prabhakar
2020-03-10 11:06 ` [PATCH v2 1/3] media: rcar-csi2: Add support to match fwnode against remote or parent port Lad Prabhakar
` (2 more replies)
0 siblings, 3 replies; 17+ messages in thread
From: Lad Prabhakar @ 2020-03-10 11:06 UTC (permalink / raw)
To: Mauro Carvalho Chehab, Niklas
Cc: linux-media, linux-renesas-soc, linux-kernel, Lad Prabhakar,
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
^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH v2 1/3] media: rcar-csi2: Add support to match fwnode against remote or parent port
2020-03-10 11:06 [PATCH v2 0/3] media: rcar-vin: Enable MEDIA_BUS_FMT_SRGGB8_1X8 format and support for matching fwnode against endpoints/nodes Lad Prabhakar
@ 2020-03-10 11:06 ` Lad Prabhakar
2020-03-10 12:43 ` Niklas
2020-03-10 11:06 ` [PATCH v2 2/3] media: rcar-vin: Add support for MEDIA_BUS_FMT_SRGGB8_1X8 format Lad Prabhakar
2020-03-10 11:06 ` [PATCH v2 3/3] media: rcar-vin: rcar-csi2: " Lad Prabhakar
2 siblings, 1 reply; 17+ messages in thread
From: Lad Prabhakar @ 2020-03-10 11:06 UTC (permalink / raw)
To: Mauro Carvalho Chehab, Niklas
Cc: linux-media, linux-renesas-soc, linux-kernel, Lad Prabhakar,
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
^ permalink raw reply related [flat|nested] 17+ messages in thread
* [PATCH v2 2/3] media: rcar-vin: Add support for MEDIA_BUS_FMT_SRGGB8_1X8 format
2020-03-10 11:06 [PATCH v2 0/3] media: rcar-vin: Enable MEDIA_BUS_FMT_SRGGB8_1X8 format and support for matching fwnode against endpoints/nodes Lad Prabhakar
2020-03-10 11:06 ` [PATCH v2 1/3] media: rcar-csi2: Add support to match fwnode against remote or parent port Lad Prabhakar
@ 2020-03-10 11:06 ` Lad Prabhakar
2020-03-10 12:46 ` Niklas
2020-03-10 11:06 ` [PATCH v2 3/3] media: rcar-vin: rcar-csi2: " Lad Prabhakar
2 siblings, 1 reply; 17+ messages in thread
From: Lad Prabhakar @ 2020-03-10 11:06 UTC (permalink / raw)
To: Mauro Carvalho Chehab, Niklas
Cc: linux-media, linux-renesas-soc, linux-kernel, Lad Prabhakar,
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
^ permalink raw reply related [flat|nested] 17+ messages in thread
* [PATCH v2 3/3] media: rcar-vin: rcar-csi2: Add support for MEDIA_BUS_FMT_SRGGB8_1X8 format
2020-03-10 11:06 [PATCH v2 0/3] media: rcar-vin: Enable MEDIA_BUS_FMT_SRGGB8_1X8 format and support for matching fwnode against endpoints/nodes Lad Prabhakar
2020-03-10 11:06 ` [PATCH v2 1/3] media: rcar-csi2: Add support to match fwnode against remote or parent port Lad Prabhakar
2020-03-10 11:06 ` [PATCH v2 2/3] media: rcar-vin: Add support for MEDIA_BUS_FMT_SRGGB8_1X8 format Lad Prabhakar
@ 2020-03-10 11:06 ` Lad Prabhakar
2020-03-10 12:48 ` Niklas
2 siblings, 1 reply; 17+ messages in thread
From: Lad Prabhakar @ 2020-03-10 11:06 UTC (permalink / raw)
To: Mauro Carvalho Chehab, Niklas
Cc: linux-media, linux-renesas-soc, linux-kernel, Lad Prabhakar,
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
^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH v2 1/3] media: rcar-csi2: Add support to match fwnode against remote or parent port
2020-03-10 11:06 ` [PATCH v2 1/3] media: rcar-csi2: Add support to match fwnode against remote or parent port Lad Prabhakar
@ 2020-03-10 12:43 ` Niklas
2020-03-10 14:54 ` Prabhakar Mahadev Lad
0 siblings, 1 reply; 17+ messages in thread
From: Niklas @ 2020-03-10 12:43 UTC (permalink / raw)
To: Lad Prabhakar
Cc: Mauro Carvalho Chehab, linux-media, linux-renesas-soc,
linux-kernel, Lad Prabhakar
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
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v2 2/3] media: rcar-vin: Add support for MEDIA_BUS_FMT_SRGGB8_1X8 format
2020-03-10 11:06 ` [PATCH v2 2/3] media: rcar-vin: Add support for MEDIA_BUS_FMT_SRGGB8_1X8 format Lad Prabhakar
@ 2020-03-10 12:46 ` Niklas
2020-03-10 13:42 ` Prabhakar Mahadev Lad
0 siblings, 1 reply; 17+ messages in thread
From: Niklas @ 2020-03-10 12:46 UTC (permalink / raw)
To: Lad Prabhakar
Cc: Mauro Carvalho Chehab, linux-media, linux-renesas-soc,
linux-kernel, Lad Prabhakar
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
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v2 3/3] media: rcar-vin: rcar-csi2: Add support for MEDIA_BUS_FMT_SRGGB8_1X8 format
2020-03-10 11:06 ` [PATCH v2 3/3] media: rcar-vin: rcar-csi2: " Lad Prabhakar
@ 2020-03-10 12:48 ` Niklas
2020-03-10 13:32 ` Prabhakar Mahadev Lad
0 siblings, 1 reply; 17+ messages in thread
From: Niklas @ 2020-03-10 12:48 UTC (permalink / raw)
To: Lad Prabhakar
Cc: Mauro Carvalho Chehab, linux-media, linux-renesas-soc,
linux-kernel, Lad Prabhakar
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
^ permalink raw reply [flat|nested] 17+ messages in thread
* RE: [PATCH v2 3/3] media: rcar-vin: rcar-csi2: Add support for MEDIA_BUS_FMT_SRGGB8_1X8 format
2020-03-10 12:48 ` Niklas
@ 2020-03-10 13:32 ` Prabhakar Mahadev Lad
0 siblings, 0 replies; 17+ messages in thread
From: Prabhakar Mahadev Lad @ 2020-03-10 13:32 UTC (permalink / raw)
To: Niklas
Cc: Mauro Carvalho Chehab, linux-media, linux-renesas-soc,
linux-kernel, Lad Prabhakar
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
^ permalink raw reply [flat|nested] 17+ messages in thread
* RE: [PATCH v2 2/3] media: rcar-vin: Add support for MEDIA_BUS_FMT_SRGGB8_1X8 format
2020-03-10 12:46 ` Niklas
@ 2020-03-10 13:42 ` Prabhakar Mahadev Lad
2020-03-10 14:06 ` Niklas
0 siblings, 1 reply; 17+ messages in thread
From: Prabhakar Mahadev Lad @ 2020-03-10 13:42 UTC (permalink / raw)
To: Niklas
Cc: Mauro Carvalho Chehab, linux-media, linux-renesas-soc,
linux-kernel, Lad Prabhakar
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
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v2 2/3] media: rcar-vin: Add support for MEDIA_BUS_FMT_SRGGB8_1X8 format
2020-03-10 13:42 ` Prabhakar Mahadev Lad
@ 2020-03-10 14:06 ` Niklas
[not found] ` <CA+V-a8vsYGdx6AtgqwS0LXREn4hu-EjVh2D5Dp_rHmpazBYG5A@mail.gmail.com>
0 siblings, 1 reply; 17+ messages in thread
From: Niklas @ 2020-03-10 14:06 UTC (permalink / raw)
To: Prabhakar Mahadev Lad
Cc: Mauro Carvalho Chehab, linux-media, linux-renesas-soc,
linux-kernel, Lad Prabhakar
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
^ permalink raw reply [flat|nested] 17+ messages in thread
* RE: [PATCH v2 1/3] media: rcar-csi2: Add support to match fwnode against remote or parent port
2020-03-10 12:43 ` Niklas
@ 2020-03-10 14:54 ` Prabhakar Mahadev Lad
0 siblings, 0 replies; 17+ messages in thread
From: Prabhakar Mahadev Lad @ 2020-03-10 14:54 UTC (permalink / raw)
To: Niklas
Cc: Mauro Carvalho Chehab, linux-media, linux-renesas-soc,
linux-kernel, Lad Prabhakar
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
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v2 2/3] media: rcar-vin: Add support for MEDIA_BUS_FMT_SRGGB8_1X8 format
[not found] ` <CA+V-a8u8=H-6WfaYMLWH73zo5ehP8cu9D-tdGULk=Hkvq4KuAQ@mail.gmail.com>
@ 2020-03-30 12:07 ` Niklas
2020-03-30 13:13 ` Lad, Prabhakar
2020-04-06 17:20 ` Lad, Prabhakar
0 siblings, 2 replies; 17+ messages in thread
From: Niklas @ 2020-03-30 12:07 UTC (permalink / raw)
To: Lad, Prabhakar
Cc: Prabhakar Mahadev Lad, Mauro Carvalho Chehab, linux-media,
linux-renesas-soc, linux-kernel
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
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v2 2/3] media: rcar-vin: Add support for MEDIA_BUS_FMT_SRGGB8_1X8 format
2020-03-30 12:07 ` Niklas
@ 2020-03-30 13:13 ` Lad, Prabhakar
2020-04-06 17:20 ` Lad, Prabhakar
1 sibling, 0 replies; 17+ messages in thread
From: Lad, Prabhakar @ 2020-03-30 13:13 UTC (permalink / raw)
To: Niklas
Cc: Prabhakar Mahadev Lad, Mauro Carvalho Chehab, linux-media,
linux-renesas-soc, linux-kernel
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
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v2 2/3] media: rcar-vin: Add support for MEDIA_BUS_FMT_SRGGB8_1X8 format
2020-03-30 12:07 ` Niklas
2020-03-30 13:13 ` Lad, Prabhakar
@ 2020-04-06 17:20 ` Lad, Prabhakar
2020-04-07 9:56 ` Niklas
1 sibling, 1 reply; 17+ messages in thread
From: Lad, Prabhakar @ 2020-04-06 17:20 UTC (permalink / raw)
To: Niklas
Cc: Prabhakar Mahadev Lad, Mauro Carvalho Chehab, linux-media,
linux-renesas-soc, linux-kernel
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
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v2 2/3] media: rcar-vin: Add support for MEDIA_BUS_FMT_SRGGB8_1X8 format
2020-04-06 17:20 ` Lad, Prabhakar
@ 2020-04-07 9:56 ` Niklas
2020-04-14 19:39 ` Niklas
0 siblings, 1 reply; 17+ messages in thread
From: Niklas @ 2020-04-07 9:56 UTC (permalink / raw)
To: Lad, Prabhakar
Cc: Prabhakar Mahadev Lad, Mauro Carvalho Chehab, linux-media,
linux-renesas-soc, linux-kernel
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
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v2 2/3] media: rcar-vin: Add support for MEDIA_BUS_FMT_SRGGB8_1X8 format
2020-04-07 9:56 ` Niklas
@ 2020-04-14 19:39 ` Niklas
2020-04-15 8:21 ` Lad, Prabhakar
0 siblings, 1 reply; 17+ messages in thread
From: Niklas @ 2020-04-14 19:39 UTC (permalink / raw)
To: Lad, Prabhakar
Cc: Prabhakar Mahadev Lad, Mauro Carvalho Chehab, linux-media,
linux-renesas-soc, linux-kernel
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
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v2 2/3] media: rcar-vin: Add support for MEDIA_BUS_FMT_SRGGB8_1X8 format
2020-04-14 19:39 ` Niklas
@ 2020-04-15 8:21 ` Lad, Prabhakar
0 siblings, 0 replies; 17+ messages in thread
From: Lad, Prabhakar @ 2020-04-15 8:21 UTC (permalink / raw)
To: Niklas
Cc: Prabhakar Mahadev Lad, Mauro Carvalho Chehab, linux-media,
linux-renesas-soc, linux-kernel
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
^ permalink raw reply [flat|nested] 17+ messages in thread
end of thread, other threads:[~2020-04-15 8:22 UTC | newest]
Thread overview: 17+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-03-10 11:06 [PATCH v2 0/3] media: rcar-vin: Enable MEDIA_BUS_FMT_SRGGB8_1X8 format and support for matching fwnode against endpoints/nodes Lad Prabhakar
2020-03-10 11:06 ` [PATCH v2 1/3] media: rcar-csi2: Add support to match fwnode against remote or parent port Lad Prabhakar
2020-03-10 12:43 ` Niklas
2020-03-10 14:54 ` Prabhakar Mahadev Lad
2020-03-10 11:06 ` [PATCH v2 2/3] media: rcar-vin: Add support for MEDIA_BUS_FMT_SRGGB8_1X8 format Lad Prabhakar
2020-03-10 12:46 ` Niklas
2020-03-10 13:42 ` Prabhakar Mahadev Lad
2020-03-10 14:06 ` Niklas
[not found] ` <CA+V-a8vsYGdx6AtgqwS0LXREn4hu-EjVh2D5Dp_rHmpazBYG5A@mail.gmail.com>
[not found] ` <20200319150329.GB3192108@oden.dyn.berto.se>
[not found] ` <CA+V-a8u8=H-6WfaYMLWH73zo5ehP8cu9D-tdGULk=Hkvq4KuAQ@mail.gmail.com>
2020-03-30 12:07 ` Niklas
2020-03-30 13:13 ` Lad, Prabhakar
2020-04-06 17:20 ` Lad, Prabhakar
2020-04-07 9:56 ` Niklas
2020-04-14 19:39 ` Niklas
2020-04-15 8:21 ` Lad, Prabhakar
2020-03-10 11:06 ` [PATCH v2 3/3] media: rcar-vin: rcar-csi2: " Lad Prabhakar
2020-03-10 12:48 ` Niklas
2020-03-10 13:32 ` Prabhakar Mahadev Lad
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).